fix: stabilize viewer navigation and timeline layout#313
Merged
Conversation
|
@flamerged is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds data: to viewer CSP img-src, implements hash-driven tab routing with URL/history sync, refactors header/observation card layout and CSS, and migrates feature-flag dismissal to an in-memory state with updated banner markup and styling. ChangesViewer Routing, UI Refinements, and Feature Flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
flamerged
force-pushed
the
fix/viewer-navigation-ui
branch
from
22df145 to
56ab412
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/viewer/index.html (2)
1178-1183: 💤 Low valueOptional: set
aria-currenton the active tab for screen-reader navigation.
switchTabtoggles the visual.activeclass on the matching[data-tab]button but does not surface that state to assistive tech. Reflecting it viaaria-current="page"(oraria-selectedif you want true tablist semantics) lets screen-reader users perceive the current tab.♻️ Proposed refactor
document.querySelectorAll('.tab-bar button').forEach(function(b) { - b.classList.toggle('active', b.dataset.tab === tab); + var isActive = b.dataset.tab === tab; + b.classList.toggle('active', isActive); + if (isActive) b.setAttribute('aria-current', 'page'); + else b.removeAttribute('aria-current'); });Also applies to: 948-961
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/viewer/index.html` around lines 1178 - 1183, The tab-switching code currently only toggles the visual .active class; update the logic in the function that contains the shown selectors (the switchTab handler that queries '.tab-bar button' and '.view') to also set aria-current="page" on the active button and remove that attribute from all other buttons (or use aria-selected="true"/"false" if you adopt full tablist semantics); ensure you set aria-current only when b.dataset.tab === tab and explicitly remove the attribute for non-active buttons so screen readers receive the current-tab state.
3418-3423: ⚡ Quick winPreserve modifier/middle-click new-tab behavior on
[data-tab-link].The brand link is now a real
<a href="#dashboard">, but the handler always callse.preventDefault(). That suppresses Ctrl/Cmd/middle-click "open in new tab" semantics — clicking with a modifier will just toggle the dashboard tab in the current view instead of opening a new tab at#dashboard. Letting modified clicks pass through preserves expected anchor behavior while keeping plain clicks SPA-style.♻️ Proposed refactor
document.querySelectorAll('[data-tab-link]').forEach(function(link) { link.addEventListener('click', function(e) { + // Let users open the link in a new tab/window with modifier or middle-click. + if (e.defaultPrevented || e.button !== 0 || e.metaKey || e.ctrlKey || e.shiftKey || e.altKey) return; e.preventDefault(); switchTab(link.getAttribute('data-tab-link')); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/viewer/index.html` around lines 3418 - 3423, The click handler on elements selected by document.querySelectorAll('[data-tab-link]') always calls e.preventDefault(), which breaks modifier/middle-click behavior; update the handler so it only prevents default and runs switchTab(link.getAttribute('data-tab-link')) for unmodified left-clicks. Concretely, detect modified clicks (e.metaKey || e.ctrlKey || e.shiftKey || e.altKey) and middle/right buttons (e.button !== 0 or e.button === 1 for middle) and early-return without calling e.preventDefault(), otherwise call e.preventDefault() and switchTab as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/viewer/index.html`:
- Around line 2411-2421: The obs-title span currently renders the truncated
title text without a tooltip; update the HTML generation where the span is
created (the line producing '<span class="obs-title">' + esc(title) + '</span>')
to include a title attribute set to the full escaped title (e.g., '<span
class="obs-title" title="' + esc(title) + '">' + esc(title) + '</span>'); apply
the same change to the other occurrence of the obs-title span around lines
474-483 so long titles show the full text on hover while preserving the existing
ellipsis styling.
---
Nitpick comments:
In `@src/viewer/index.html`:
- Around line 1178-1183: The tab-switching code currently only toggles the
visual .active class; update the logic in the function that contains the shown
selectors (the switchTab handler that queries '.tab-bar button' and '.view') to
also set aria-current="page" on the active button and remove that attribute from
all other buttons (or use aria-selected="true"/"false" if you adopt full tablist
semantics); ensure you set aria-current only when b.dataset.tab === tab and
explicitly remove the attribute for non-active buttons so screen readers receive
the current-tab state.
- Around line 3418-3423: The click handler on elements selected by
document.querySelectorAll('[data-tab-link]') always calls e.preventDefault(),
which breaks modifier/middle-click behavior; update the handler so it only
prevents default and runs switchTab(link.getAttribute('data-tab-link')) for
unmodified left-clicks. Concretely, detect modified clicks (e.metaKey ||
e.ctrlKey || e.shiftKey || e.altKey) and middle/right buttons (e.button !== 0 or
e.button === 1 for middle) and early-return without calling e.preventDefault(),
otherwise call e.preventDefault() and switchTab as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e007d40-a422-4b6d-8917-b421f42eb147
📒 Files selected for processing (3)
src/auth.tssrc/viewer/index.htmltest/viewer-security.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/viewer-security.test.ts
- src/auth.ts
2 tasks
rohitg00
added a commit
that referenced
this pull request
May 17, 2026
The viewer-session-id sandbox (added in #366) ran fine until #313 landed hash-routing in `switchTab` — `updateTabRoute` now calls `history.replaceState`, and the aria-selected toggle calls `removeAttribute` on the non-active tab buttons. Neither was stubbed in the vm sandbox, so the combined main produced 2 failures: TypeError: b.removeAttribute is not a function ReferenceError: history is not defined Both PRs (#366, #313) passed CI in isolation. The combined failure only surfaced after merge. Stub both globals. 987/987 tests now pass on main.
This was referenced May 17, 2026
rohitg00
added a commit
that referenced
this pull request
May 17, 2026
…#452) #313 added `data:` to the viewer img-src so an inline-SVG favicon encoded as `data:image/svg+xml,...` could load. Bare `data:` allows arbitrary data-URI image types (data:image/png;base64,..., etc), which the viewer does not need. Self-host the favicon at `/favicon.svg` and revert the CSP to `img-src 'self'`. Same artwork (dark rounded tile + green "AM"), served from a real URL with `Content-Type: image/svg+xml` and `Cache-Control: public, max-age=3600`. - new file: src/viewer/favicon.svg (extracted from the URL-decoded inline blob) - src/auth.ts: img-src reverted to 'self' - src/viewer/index.html: favicon link now href="/favicon.svg" - src/viewer/server.ts: new GET /favicon.svg route, candidate-path loader mirrors loadViewerTemplate() so it resolves both from source (vitest) and from dist/ (npm run start) - package.json: build script copies favicon.svg into dist/viewer/ alongside index.html - test/viewer-security.test.ts: CSP assertion now requires bare `img-src 'self'` and rejects any `data:` allowance; new e2e test hits /favicon.svg and verifies SVG payload + headers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AMfavicon and allow data-image favicons in the viewer CSP.Root Cause
The viewer is a single HTML/CSS/JS surface, and several fixed UI rows were normal flex children that could shrink under tall view content. Timeline cards also used an unconstrained flex header and preformatted payloads, so long tool names and JSON strings could overflow their card and push the metadata controls out of place.
Validation
node --checkon the extracted viewer scriptgit diff --check -- src/viewer/index.html src/auth.ts test/viewer-security.test.ts../agentmemory/node_modules/.bin/vitest run test/viewer-security.test.tsnpm run buildfrom the worktree using the existing localnode_modulesinstallFollow-up
The viewer remains hard to develop locally because the frontend is tightly coupled to the Node viewer proxy and REST port assumptions. This PR keeps the fix scoped, but a cleaner frontend/backend dev split would make these UI changes much easier to test.
Summary by CodeRabbit
New Features
Style
Security
Tests