Fullscreen overlay for mermaid diagrams

ldraney commented

2026-05-22 11:36:38 +00:00

Author

Owner

PR #8 Review

DOMAIN REVIEW

Tech stack: Rails 8 + Stimulus + Importmap + CSS (Gruvbox dark theme). Two files changed: app/assets/stylesheets/application.css (CSS) and app/javascript/controllers/mermaid_controller.js (Stimulus controller). 115 additions, 0 deletions.

Stimulus / JavaScript

Escape listener leak on overlay.remove() -- The onEscape handler is cleaned up when Escape is pressed, but NOT when the overlay is dismissed via the close button or backdrop click. Both of those paths call overlay.remove() without document.removeEventListener("keydown", onEscape). This leaves an orphaned global keydown listener that will throw (or silently fail) on next Escape press since overlay is already detached. Fix: extract a dismiss() closure that both removes the overlay and removes the listener, then use it in all three dismiss paths.
innerHTML for SVG injection (line 35) -- content.innerHTML = diagramContainer.querySelector("svg").outerHTML works but is the brute-force path. Since the source is mermaid-generated SVG (not user input), this is not a security issue here, but cloneNode(true) would be cleaner and avoid a serialize-then-parse round-trip. Minor point -- not a blocker.
No guard if SVG is missing -- diagramContainer.querySelector("svg") on line 35 will throw a null-reference error if the mermaid render failed and no SVG exists in the container. A null check before accessing .outerHTML would be defensive.
Button innerHTML is a long inline SVG string (line 21) -- Readable enough for a single icon, but if more icons are added later this pattern will degrade. Acceptable for now.

CSS

Consistent use of design tokens -- Good. The overlay styles use var(--color-bg), var(--color-muted), var(--color-text), var(--color-border), var(--radius), and var(--spacing-sm). Theme-aligned.
Hardcoded values -- width: 32px; height: 32px on .mermaid-fullscreen-btn, padding: 2rem on .mermaid-overlay, top: 1rem; right: 1.5rem and font-size: 2rem on .mermaid-overlay-close. These are not pulled from design tokens. Not blocking since the existing codebase uses similar mixed approaches (e.g., border-radius: 3px on inline code), but worth noting for consistency.
z-index: 9999 / 10000 -- Works but magic numbers. No z-index scale exists in the codebase currently, so this is consistent with the project's maturity level.
Accessibility -- Good: both buttons have aria-label attributes. The hover-reveal pattern (opacity: 0 on the button until parent hover) means the button is invisible but still focusable via keyboard. Since opacity: 0 does not prevent focus, a keyboard user tabbing through the page will land on an invisible button. Consider adding :focus-within to the .mermaid-diagram selector to reveal the button on keyboard focus too:
```
.mermaid-diagram:hover .mermaid-fullscreen-btn,
.mermaid-diagram:focus-within .mermaid-fullscreen-btn {
    opacity: 1;
}
```
Focus trap -- The fullscreen overlay does not trap focus. A screen reader or keyboard user could tab behind the overlay to page content. For a dismiss-on-Escape overlay this is low severity but worth noting.

BLOCKERS

None. This is a pure UI enhancement (CSS + client-side JS). No auth paths, no user input processing, no server-side changes, no credentials.

Regarding the "no tests" blocker criterion: this project has no test infrastructure at all (no test/ or spec/ directory, no JS test runner configured, no system test setup in Gemfile). The PR is a client-side visual interaction (hover button + fullscreen overlay) that is inherently difficult to unit test without a browser test harness. The test plan in the PR body covers manual verification. Requiring automated tests here when the project has zero test infrastructure would be scope creep beyond this PR. Not a blocker for this PR, but the project should stand up at least system-level tests (Capybara/Selenium or Playwright) before the codebase grows further.

NITS

Event listener leak (item 1 above) -- This is the most important fix. Not a blocker because it is a minor memory/behavior leak, not a security or data issue, but it should be addressed.
Null guard on SVG query (item 3) -- Defensive coding. One-liner fix.
Keyboard accessibility for the expand button (item 8) -- Add :focus-within rule so keyboard users can see the button.
Focus trap on overlay (item 9) -- Low priority, but good practice.
cloneNode over innerHTML (item 2) -- Cleaner but not required.

SOP COMPLIANCE

Branch named after issue -- Branch is fullscreen-mermaid-diagrams. Convention is {issue-number}-{kebab-case-purpose}, so it should be 7-fullscreen-mermaid-diagrams. Missing the issue number prefix.
PR body follows template -- Has Summary, Changes, Test Plan, Related sections. Well structured.
Related references plan slug -- No plan slug exists for this work. PR body references ldraney/mdview #7 which is the issue, not a plan. Noted as N/A since caller confirmed no plan slug.
No secrets committed
No unnecessary file changes -- Exactly two files, both directly relevant.
Commit messages are descriptive (based on PR title/body quality)

PROCESS OBSERVATIONS

Test infrastructure gap: This is a Rails 8 app with zero automated tests. The default Rails test directory has been removed. As the app grows (mermaid rendering, link navigation, fullscreen overlays), manual testing will not scale. Recommend creating an issue to stand up system tests.
Change failure risk: Low. Pure additive CSS and JS. No server-side changes, no database, no deployment config. Worst case is a visual regression.
Deployment frequency: No CI pipeline visible. Manual deploy assumed.

VERDICT: APPROVED

The code is clean, well-structured, uses the existing design token system appropriately, and has good accessibility basics (aria-labels). The event listener leak (nit 1) and branch naming (SOP) are worth fixing but neither rises to blocker level for a UI-only enhancement in a project at this maturity stage.

## PR #8 Review ### DOMAIN REVIEW **Tech stack**: Rails 8 + Stimulus + Importmap + CSS (Gruvbox dark theme). Two files changed: `app/assets/stylesheets/application.css` (CSS) and `app/javascript/controllers/mermaid_controller.js` (Stimulus controller). 115 additions, 0 deletions. **Stimulus / JavaScript** 1. **Escape listener leak on overlay.remove()** -- The `onEscape` handler is cleaned up when Escape is pressed, but NOT when the overlay is dismissed via the close button or backdrop click. Both of those paths call `overlay.remove()` without `document.removeEventListener("keydown", onEscape)`. This leaves an orphaned global keydown listener that will throw (or silently fail) on next Escape press since `overlay` is already detached. Fix: extract a `dismiss()` closure that both removes the overlay and removes the listener, then use it in all three dismiss paths. 2. **innerHTML for SVG injection** (line 35) -- `content.innerHTML = diagramContainer.querySelector("svg").outerHTML` works but is the brute-force path. Since the source is mermaid-generated SVG (not user input), this is not a security issue here, but `cloneNode(true)` would be cleaner and avoid a serialize-then-parse round-trip. Minor point -- not a blocker. 3. **No guard if SVG is missing** -- `diagramContainer.querySelector("svg")` on line 35 will throw a null-reference error if the mermaid render failed and no SVG exists in the container. A null check before accessing `.outerHTML` would be defensive. 4. **Button innerHTML is a long inline SVG string** (line 21) -- Readable enough for a single icon, but if more icons are added later this pattern will degrade. Acceptable for now. **CSS** 5. **Consistent use of design tokens** -- Good. The overlay styles use `var(--color-bg)`, `var(--color-muted)`, `var(--color-text)`, `var(--color-border)`, `var(--radius)`, and `var(--spacing-sm)`. Theme-aligned. 6. **Hardcoded values** -- `width: 32px; height: 32px` on `.mermaid-fullscreen-btn`, `padding: 2rem` on `.mermaid-overlay`, `top: 1rem; right: 1.5rem` and `font-size: 2rem` on `.mermaid-overlay-close`. These are not pulled from design tokens. Not blocking since the existing codebase uses similar mixed approaches (e.g., `border-radius: 3px` on inline code), but worth noting for consistency. 7. **z-index: 9999 / 10000** -- Works but magic numbers. No z-index scale exists in the codebase currently, so this is consistent with the project's maturity level. 8. **Accessibility** -- Good: both buttons have `aria-label` attributes. The hover-reveal pattern (`opacity: 0` on the button until parent hover) means the button is invisible but still focusable via keyboard. Since `opacity: 0` does not prevent focus, a keyboard user tabbing through the page will land on an invisible button. Consider adding `:focus-within` to the `.mermaid-diagram` selector to reveal the button on keyboard focus too: ```css .mermaid-diagram:hover .mermaid-fullscreen-btn, .mermaid-diagram:focus-within .mermaid-fullscreen-btn { opacity: 1; } ``` 9. **Focus trap** -- The fullscreen overlay does not trap focus. A screen reader or keyboard user could tab behind the overlay to page content. For a dismiss-on-Escape overlay this is low severity but worth noting. ### BLOCKERS None. This is a pure UI enhancement (CSS + client-side JS). No auth paths, no user input processing, no server-side changes, no credentials. Regarding the "no tests" blocker criterion: this project has no test infrastructure at all (no `test/` or `spec/` directory, no JS test runner configured, no system test setup in Gemfile). The PR is a client-side visual interaction (hover button + fullscreen overlay) that is inherently difficult to unit test without a browser test harness. The test plan in the PR body covers manual verification. Requiring automated tests here when the project has zero test infrastructure would be scope creep beyond this PR. Not a blocker for this PR, but the project should stand up at least system-level tests (Capybara/Selenium or Playwright) before the codebase grows further. ### NITS 1. **Event listener leak** (item 1 above) -- This is the most important fix. Not a blocker because it is a minor memory/behavior leak, not a security or data issue, but it should be addressed. 2. **Null guard on SVG query** (item 3) -- Defensive coding. One-liner fix. 3. **Keyboard accessibility for the expand button** (item 8) -- Add `:focus-within` rule so keyboard users can see the button. 4. **Focus trap on overlay** (item 9) -- Low priority, but good practice. 5. **cloneNode over innerHTML** (item 2) -- Cleaner but not required. ### SOP COMPLIANCE - [ ] Branch named after issue -- Branch is `fullscreen-mermaid-diagrams`. Convention is `{issue-number}-{kebab-case-purpose}`, so it should be `7-fullscreen-mermaid-diagrams`. Missing the issue number prefix. - [x] PR body follows template -- Has Summary, Changes, Test Plan, Related sections. Well structured. - [ ] Related references plan slug -- No plan slug exists for this work. PR body references `ldraney/mdview #7` which is the issue, not a plan. Noted as N/A since caller confirmed no plan slug. - [x] No secrets committed - [x] No unnecessary file changes -- Exactly two files, both directly relevant. - [x] Commit messages are descriptive (based on PR title/body quality) ### PROCESS OBSERVATIONS - **Test infrastructure gap**: This is a Rails 8 app with zero automated tests. The default Rails test directory has been removed. As the app grows (mermaid rendering, link navigation, fullscreen overlays), manual testing will not scale. Recommend creating an issue to stand up system tests. - **Change failure risk**: Low. Pure additive CSS and JS. No server-side changes, no database, no deployment config. Worst case is a visual regression. - **Deployment frequency**: No CI pipeline visible. Manual deploy assumed. ### VERDICT: APPROVED The code is clean, well-structured, uses the existing design token system appropriately, and has good accessibility basics (aria-labels). The event listener leak (nit 1) and branch naming (SOP) are worth fixing but neither rises to blocker level for a UI-only enhancement in a project at this maturity stage.

2026-05-22 11:35:09 +00:00

Summary

Adds a hover-reveal expand button to each rendered mermaid diagram
Clicking opens a fullscreen overlay with the diagram scaled to fill the viewport
Dismiss via Escape, close button, or clicking the backdrop

Closes #7

Changes

app/javascript/controllers/mermaid_controller.js: inject fullscreen button per diagram, openFullscreen() clones SVG into overlay and strips mermaid's hardcoded dimensions
app/assets/stylesheets/application.css: styles for .mermaid-fullscreen-btn (hover-reveal), .mermaid-overlay (solid gruvbox bg), .mermaid-overlay-content svg (viewport-scaled)

Test Plan

Load README.md with mermaid diagram
Hover over diagram -- expand button appears top-right
Click button -- fullscreen overlay opens with diagram filling viewport
Press Escape -- overlay dismisses
Click backdrop -- overlay dismisses
Click X button -- overlay dismisses
Verify on 4K display that diagram is readable without zooming

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

ldraney/mdview #7 -- Mermaid diagrams too small on 4K displays

## Summary - Adds a hover-reveal expand button to each rendered mermaid diagram - Clicking opens a fullscreen overlay with the diagram scaled to fill the viewport - Dismiss via Escape, close button, or clicking the backdrop Closes #7 ## Changes - `app/javascript/controllers/mermaid_controller.js`: inject fullscreen button per diagram, `openFullscreen()` clones SVG into overlay and strips mermaid's hardcoded dimensions - `app/assets/stylesheets/application.css`: styles for `.mermaid-fullscreen-btn` (hover-reveal), `.mermaid-overlay` (solid gruvbox bg), `.mermaid-overlay-content svg` (viewport-scaled) ## Test Plan - [x] Load README.md with mermaid diagram - [x] Hover over diagram -- expand button appears top-right - [x] Click button -- fullscreen overlay opens with diagram filling viewport - [x] Press Escape -- overlay dismisses - [x] Click backdrop -- overlay dismisses - [x] Click X button -- overlay dismisses - [ ] Verify on 4K display that diagram is readable without zooming ## Review Checklist - [ ] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - `ldraney/mdview #7` -- Mermaid diagrams too small on 4K displays

ldraney added 1 commit

Fullscreen overlay for mermaid diagrams

CI / scan_ruby (pull_request) Has been cancelled

Details

CI / scan_js (pull_request) Has been cancelled

CI / lint (pull_request) Has been cancelled

3e3efab1f5

Diagrams in the readable column are too small on high-DPI displays.
Add a hover-reveal expand button that opens the diagram in a
fullscreen overlay, scaled to fill the viewport. Dismiss with
Escape, the close button, or clicking the backdrop.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney merged commit 0a05b131f6 into main

2026-05-22 11:38:26 +00:00

ldraney deleted branch fullscreen-mermaid-diagrams

ldraney referenced this pull request from a commit

Fullscreen overlay for mermaid diagrams (#8)

Rows
Columns

Fullscreen overlay for mermaid diagrams #8