Skip to content

feat(dashboard): route-history Back/Forward arrows and keyboard chords - #9550

Open
peterhieuvu wants to merge 1 commit into
kirodotdev:mainfrom
peterhieuvu:feat/nav-history-arrows
Open

feat(dashboard): route-history Back/Forward arrows and keyboard chords#9550
peterhieuvu wants to merge 1 commit into
kirodotdev:mainfrom
peterhieuvu:feat/nav-history-arrows

Conversation

@peterhieuvu

@peterhieuvu peterhieuvu commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Moving around the dashboard is a one-way street. Chat → Settings → an app page leaves no way back to where you came from: the desktop app has no browser chrome, no keyboard shortcut, and no visible control walks the route history. Some pages hand-roll a "Back" link, but it points at a fixed parent, not at where you actually were (#7884 is that gap on app pages).

Why it matters

Retracing your steps is a reflex every browser and most desktop apps honor. Without it, each wrong turn in the dashboard costs a manual re-navigation, and on desktop — where users expect OS conventions — there is no recovery gesture at all. #8258 asks for exactly this.

What changed (motivation → approach → change)

The header's top-left now has Back and Forward arrow buttons, and ⌘←/⌘→ (macOS) or Ctrl+←/Ctrl+→ (Windows/Linux) do the same thing from the keyboard. Both drive react-router's own history (navigate(±1)), so they work identically in the browser dashboard and the Electron renderer, and desktop session switches — which push real history entries — retrace along with pages.

Each arrow knows when it has nowhere to go. A new module store (src/lib/routeHistoryPosition.ts) reads react-router's history.state.idx through the same routerEntry() reader NavigationBackGuard uses: Back exists iff idx > 0 (exact, survives reloads), Forward iff idx is below a watermark of the highest index seen, which a PUSH resets because a push truncates the Forward branch. The watermark is kept in sessionStorage (per tab, dies with the tab — the stack's own lifetime), so a Forward branch built before a reload or a Back/Forward return is still offered afterwards. A fresh navigate arrival (typed URL, location.href, a popout that cloned the tab's storage) truncates that branch, so there the stored value is discarded and the arrow under-reports rather than promising a move that will not happen. A RouteHistoryTracker mounted next to NavigationBackGuard feeds the store; the arrows and the chord read it. On a very narrow header (below a 208px left-cluster width, the documented drop-rung pattern) the arrows hide — they are the one redundant control in that cluster, since the chords and browser chrome reach the same history.

Every step asks before it destroys. A pop is the one navigation the draft-discard trap (NavigationBackGuard) already guards — but only while its trap is armed, and the trap deliberately stays out of the stack when the user has a Forward branch or after a reload. The new useGuardedHistoryStep() closes that gap: it asks the page's leave guard up front unless the current entry is an armed trap duplicate, in which case the trap prompts instead. One prompt per click, wherever the ask lives. The chords are additionally gated out of text fields (where the same keys mean caret line-start/line-end or word-jump), terminals (the PTY owns them), and narrow viewports — the same condition that hides the arrows, because below the breakpoint the drill-in surfaces navigate by component state and a stack walk would move history the visible UI does not reflect.

Entering the dashboard through a ?token= link used to leave the arrows permanently disabled: ChatPage's two URL strippers (the ?prefill= consumer and the channel-token consumer) called history.replaceState({}, …), erasing react-router's history.state.idx bookkeeping so every later index was NaN. Both now pass window.history.state through — the query is still stripped, the router's state survives — and routerEntry() reads idx with Number.isFinite, so any future raw replace degrades the arrows to conservatively-disabled instead of poisoning every comparison.

The arrows advertise their chords only while they work: aria-keyshortcuts and the tooltip suffix ("Back (⌘←)") read the same shortcuts-enabled toggle the keydown handler honours, through the shared useShortcutsEnabled(), and read the LIVE binding (useShortcutBindings()) rather than the factory default, so a rebound chord is announced correctly and an unbound one not at all; the catalog tooltip can only spell the factory chord, so it drops to the bare label once the user rebinds.

The chords are dispatch: 'registry' entries in the shortcut registry (history-back / history-forward, actions group), so the #4608 rebind UI covers them for free; a text field un-claims the hit (the field consumes the caret chord), while a narrow viewport — where the arrows are hidden — claims the chord as a deliberate no-op, because on macOS an unclaimed ⌘← is native browser Back and would pop past an unarmed draft trap. ⌘[/⌘] stays on session cycling; the bracket-convention question from #8258 remains a maintainer call this PR does not preempt.

Tests

  • NavHistoryArrows.test.tsx — arrows over a real BrowserRouter: disabled on a fresh document, Back enables after a push, walking back enables Forward, a mid-stack push truncates the Forward branch, aria-keyshortcuts on both buttons; store unit tests (snapshot identity stability, subscriber notification only on change, null-idx claims nothing, live canGoBack vs watermark-dependent canGoForward).
  • Guard interplay (same file) — the exact review scenario: with a Forward branch blocking the trap, the arrow asks the leave guard before popping, a veto keeps the draft, an accept carries the pop; with the trap armed, exactly one prompt fires (the trap's), never two.
  • useKeyboardShortcuts.test.tsxhistoryNavStep platform/modifier discipline (⌘ vs Ctrl, rejects Shift/Alt/extra modifiers, vertical arrows); handler integration: navigates when history allows, claims the chord at the stack bottom so the arrows and keyboard agree, leaves the chord to text fields and terminals.

Manual verification

Verified live against a dev gateway with Playwright: fresh load shows both arrows disabled; navigating enables Back; the Back arrow pops and enables Forward; Forward returns; Ctrl+← navigates back with the same state flips. Screenshots below are from that session.

Additionally verified by hand on the macOS desktop app against a dev gateway (via a remote-instance connection): arrows render in the header's left cluster clear of the traffic lights, enable/disable correctly across navigation, and the token-entry regression fixed on this head stays fixed on that path.

Screenshots / video

Fresh load — both arrows disabled (nowhere to go):

fresh load, both arrows disabled

After navigating — Back enabled, Forward disabled:

after navigation, back enabled

After stepping Back — Forward enabled, Back disabled (dark theme):

after back, forward enabled

The two new shortcuts-modal rows under Actions:

shortcuts modal rows

Related Issues

Fixes #8258

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@peterhieuvu
peterhieuvu requested a review from a team September 8, 2026 21:21
@peterhieuvu
peterhieuvu requested a review from a team as a code owner September 8, 2026 21:21
@peterhieuvu

Copy link
Copy Markdown
Contributor Author

Intent: Give the dashboard a first-class Back/Forward over its own route history — visible top-left arrow buttons plus ⌘/Ctrl+←/→ — working identically in the browser and the Electron renderer, with disabled states that never promise a move that will not happen, and every step routed through the draft-discard guard so no pop silently destroys typed work.
Not a goal: Native Electron menu items, mouse button 4/5 handlers (Chromium already binds them in both surfaces), reassigning ⌘[/⌘] from session cycling, or closing the guard's own pre-existing browser-Back gaps (#8010).

@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 5fc3a9558002edf81c3de7d9f869a81a90210c6d via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in hand: the patch is fully read, the four added screenshots are confirmed absent from the base tree (fork lane — binary markers only, none openable), and sibling "Back" affordances checked (ListDetailBack.tsx uses "Back to X", so no label collision with the header arrow). No blind read runs in this lane. Nothing block-level: labels are truthful (both controls land on navigate(±1)), the destructive path is guarded (useGuardedHistoryStep asks the draft guard exactly once), arrow icons are the universal browser pair, and disabled-state flips happen in place on the same node (no hard swap). Final review:

UX-Verdict: CONCERNS

Solid browser-convention Back/Forward, but every screenshot lives only on the fork — no first-time reader has seen a single state of it.

Watch

  • ⌘/Ctrl+← is claimed even at the stack bottom (claims the chord even at the bottom of the stack test), so web users lose native browser-back out of the dashboard via keyboard — a habituated gesture that now silently no-ops; rare but permanent friction. Consider claiming only when canGoBack() on the web (non-Electron) build, keeping the claim on desktop where the draft-guard hole exists.
  • The documented stale-watermark residual (return via browser Back after an external link) yields an enabled Forward whose click does nothing — an action with zero acknowledgment (lens 5). Rare and self-healing, but if cheap, re-publish position after a no-op pop so the arrow disables on the dead click.

Evidence gaps

  • Back/Forward arrows, all four states (both-disabled, back-enabled, forward-enabled dark) — the PR's temp-screenshots/nav-history-arrows/*.png are not materialized in this lane and no blind read ran; push the branch to this repo to get the cold read.
  • Shortcuts-modal "Back"/"Forward" rows — shortcuts-modal-rows.png, same gap.
  • Tooltip with chord suffix ("Back · ⌘←") — shown in no screenshot at all; add a hover capture.
  • Narrow-header hidden state (the 208px tb-drop-navhistory rung) — no screenshot shows the arrows dropped while chords still work.

[UX-REVIEWED] 5fc3a95

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 5fc3a9558002edf81c3de7d9f869a81a90210c6d via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design review complete. The patch is a self-contained frontend feature (Back/Forward arrows + chords over react-router history), the temp-screenshots/ additions follow an established repo convention (1700+ files, own README, gitignore exemption), the react-router history.state.idx coupling pre-exists in NavigationBackGuard and this PR consolidates it into one reader with a conservative degrade path, and the ChatPage replaceState fix addresses the root cause of the state-wiping bug rather than papering over it. The feature-map doc is updated in the same commit, every description claim has backing code, and failure modes (missing/NaN idx, stale watermark, unarmed trap) each have a stated, tested story.

Design-Verdict: PASS

A real gap (#8258) closed at the right seams — one shared position store, the existing leave-guard contract honored, failure modes degrading conservatively rather than lying.

[DESIGN-REVIEWED] 5fc3a95

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 5fc3a9558002edf81c3de7d9f869a81a90210c6d via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

No network access from this environment, so #8258's content stays unverified — that becomes the one human check on an otherwise clean pass. Everything else checked out against the base tree: the moved helpers (routerEntry, isTrapEntry, arrivedByFreshNavigation) exist verbatim in NavigationLeaveGuard.tsx and are relocated without behavioral deletion, no pre-existing history chord conflicts (Alt+←/→ is chat-prev/next, ⌘[/] untouched), temp-screenshots/ and the feature-map row are documented conventions, and every reused symbol (ariaKeyshortcutsFor, useShortcutBindings, safeGetSessionItem, MOBILE_BREAKPOINT) exists on base.

First-Principles-Verdict: PASS

Confirm #8258 actually requests dashboard Back/Forward — it is the provenance for the whole addition and this environment could not fetch it.

What this change ships

Inventory (10 items) — 10 justified

Intent: give dashboard users a way to retrace their navigation — visible arrows plus keyboard chords — where the desktop app has no browser chrome (#8258). This is an ADDITION.

  1. Back/Forward arrows appear in the header's top-left cluster — justified
  2. ⌘/Ctrl+←/→ walk history, rebindable via the shortcuts modal — justified
  3. Each arrow disables when there is nowhere to go; the Forward state survives a reload (per-tab sessionStorage) — justified
  4. A back/forward step asks the page's draft guard before typed work can be discarded — justified
  5. Narrow windows hide the arrows and the chord deliberately does nothing there — justified
  6. Text fields and terminals keep the chord (caret/word-jump/PTY) — justified
  7. Entering via a ?token=/?prefill= link no longer wipes router history bookkeeping — justified
  8. Tooltips and screen-reader labels advertise the chord only while shortcuts are enabled, following live rebinds — justified
  9. New feature-map row for the control — justified
  10. Labels translated in 12 locales plus committed evidence screenshots — justified

[FIRST-PRINCIPLES-REVIEWED] 5fc3a95

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 5fc3a9558002edf81c3de7d9f869a81a90210c6d via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate's premise is that in embed mode the arrows mount (via {!isMobile && <NavHistoryArrows />}) while the chord is dead. But App.tsx:3126 short-circuits on isEmbed to a completely separate render tree (lines 3127–3138) containing only KiroCrewNavBridge, EmbedTabStrip, and the embed routes — no <header> topbar, so NavHistoryArrows is never rendered in embed mode. The {!isMobile && <NavHistoryArrows />} line lives only in the non-embed full-dashboard branch () : ( at line 3139), which /embed/* paths never reach. The claimed contradiction (visible, clickable arrows vs. a dead chord) cannot occur, so the observable wrong outcome (c) fails. Candidate dropped.

No new grounded defects at the 80+ bar in the changed lines.

No findings.

[OPUS-REVIEWED] 5fc3a95

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings (all downgraded on adjudication)

Reviewed 5fc3a9558002edf81c3de7d9f869a81a90210c6d via the fork AI-review pipeline; updated in place on each push.

Opus 4.8 adjudication downgraded every blocking finding to advisory. Adjudication downgraded all 1 blocking finding(s) to advisory: the remedy each one requires is disproportionate to the harm it prevents. Read them as advice, not as merge conditions.

Review details

BLOCKING -- docs/feature-map/README.md:90 -- Forward can leave the dashboard after a cross-document Back return

“Back and Forward over the dashboard's own route history”
External same-tab navigation from mid-stack -> browser Back restores stale maxIdx -> Forward opens the external document.
Anchor: feature-map-correctness
Fix: Disable Forward on cross-document back_forward arrival until in-app navigation reestablishes the watermark.
[BLOCK-MERGE-DOWNGRADED] 5fc3a95
[GPT-REVIEWED] 5fc3a95

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I've read the prompt, the finding, and the full diff. Let me verify the mechanism the finding describes against the code it names.

The finding (F1, feature-map-correctness at docs/feature-map/README.md:90) claims the Forward watermark, restored from sessionStorage after a browser Back from an external same-tab document, can enable a Forward step that navigates out of the dashboard to that external document — worse than the "one dead click" the module documents as its accepted residual.

Tracing it:

  • Mid-stack POP raises the watermark: routeHistoryPosition.ts:871 (setMaxIdx(Math.max(...))).
  • A same-tab external navigation truncates the real browser branch (browser behavior, not code), but the watermark persists in sessionStorage (MAX_IDX_KEY).
  • Returning by browser Back is a back_forward arrival, so arrivedByFreshNavigation() is false and loadMaxIdx() restores the stale watermark: routeHistoryPosition.ts:787, 805–813.
  • canGoForward() then returns true (idx < maxIdx): routeHistoryPosition.ts:888–892, and step(1)/navigate(1) walks forward into the external doc.

So the claim is real, and the module's "one dead click" note understates it for the mid-stack case.

Weighing: the consequence is a navigation to a document the user already visited in this same tab, fully recoverable by Back, visible when it happens, and self-correcting on the next in-app PUSH (routeHistoryPosition.ts:870 resets maxIdx). It requires a four-step rare combination and is largely a plain-browser-only path — the desktop app (the feature's stated target) opens external links out-of-tab. No credential/privilege/governance/silent-corruption/irreversible dimension. That is the LOW rung: a one-off recoverable, visible, self-correcting degradation under rare conditions.

The real fix ("disable Forward on cross-document back_forward arrival until in-app navigation reestablishes the watermark") adds cross-document-source detection — a distinction performance navigation timing does not cleanly give — as new branching/state inside a module whose entire design is a chain of deliberately-reasoned residuals, carrying permanent reader load. And the README row itself makes no false claim: it documents discard-on-fresh-navigation and does not assert this edge is handled; a feature map is not an exhaustive edge-case spec. Remedy cost clearly exceeds the harm prevented.

LOW rung; conditions confirmed at routeHistoryPosition.ts:871, :787/805–813, :888–892; recovery = browser/in-app Back plus watermark reset on next PUSH (:870), visible.
Real fix adds cross-document-arrival detection (source not cleanly available from nav timing) as new state/branches in an intricate residual-laden module, permanent maintenance load, to prevent a rare recoverable visible navigation.

[ADJUDICATION] 5fc3a95 total=1 uphold=0 downgrade=1
DOWNGRADE F1 docs/feature-map/README.md:90 reason=disproportionate-remedy
[GPT-ADJUDICATED] 5fc3a95

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@peterhieuvu
peterhieuvu force-pushed the feat/nav-history-arrows branch from f80dc6e to 7312061 Compare September 8, 2026 22:58
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 8, 2026
@peterhieuvu
peterhieuvu force-pushed the feat/nav-history-arrows branch from 7312061 to c37e082 Compare September 9, 2026 03:25
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 9, 2026
@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • aria-keyshortcuts advertises a dead chord when shortcuts are off span=1b2a6858544a — fixed
    self-added: yes
    mechanism: shared useShortcutsEnabled() hook exported from useNavShortcutHint.ts

Fixed in c37e082: both arrows now gate aria-keyshortcuts (and the tooltip chord suffix) on the same shortcuts-enabled snapshot the rail hints read, via a shared useShortcutsEnabled() subscribed to SHORTCUTS_ENABLED_EVENT — the one source the live keydown handler honours. With shortcuts off, the attribute is absent and the tooltip is the bare label. Locked by the new "advertises no chord while shortcuts are globally disabled" test in NavHistoryArrows.test.tsx. This ruling covers any control that advertises a chord: the advertisement must read the same toggle as the binding.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Append the chord to the tooltip span=06dcfdf60194 — fixed
    self-added: yes

Fixed in c37e082: tooltips now read "Back (⌘←)" / "Back (Ctrl + ←)" via formatShortcut() from the registry entry — derived, not hand-written per platform, matching the rail rows' hint pattern — and only while shortcuts are enabled (see the span=1b2a6858544a ruling). Asserted in the chord-labels test.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • The rationale that hides the arrows on mobile does not gate the keyboard chord span=edeae87154f0 — fixed
    self-added: yes
    mechanism: isNarrowViewport() — live matchMedia read of the useIsMobile breakpoint at keypress

Fixed in c37e082: the chord branch now bails on isNarrowViewport(), a live matchMedia read of the same MOBILE_BREAKPOINT useIsMobile uses (read at keypress so the document listener never re-registers on resize). Below the breakpoint the chord falls through unclaimed, exactly as the arrows are hidden — the two affordances now agree in both directions. Locked by the new "leaves the chord unclaimed on a narrow viewport" test.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • routerIdx re-derives what routerEntry already reads span=73260e3c5a7a — fixed
    self-added: yes

Fixed in c37e082: routerEntry() is now the single exported reader, living in lib/routeHistoryPosition.ts; NavigationLeaveGuard imports it and its local copy is deleted; routerIdx is gone. The history.state coupling to react-router internals is in exactly one place.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Arrows disappear below a 208px header cluster — undeclared span=ef3eef211e22 — fixed
    self-added: yes

Fixed in c37e082 (PR body): "What changed" now declares the 208px drop rung and its reason — the arrows are the one redundant control in the left cluster (chords and browser chrome reach the same history), following the documented narrow-viewport rung pattern.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Unexport routerIdx — zero external consumers span=1655015a3b4c — fixed
    self-added: yes

Fixed in c37e082: routerIdx no longer exists as an export or a function — folded into routerEntry(), whose consumers are real (the store internals, the guard, and the keydown reads).

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • routerIdx duplicates the guard's routerEntry reader — export one, delete the other span=eea23cd017e3 — fixed
    self-added: yes

Fixed in c37e082, in the direction that keeps layering clean: the reader lives in lib/routeHistoryPosition.ts (a lib module) and components/NavigationLeaveGuard.tsx imports it, rather than a lib importing from a component. One spelling, one place, same shape the guard documented.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author

Non-ledger notes on the remaining round-1 review items (no span ids — these were not findings):

  • UX evidence gaps: c37e082 adds the two missing captures — after-back-forward-enabled.png (Forward enabled, dark theme) and shortcuts-modal-rows.png (the two new Actions rows) — SHA-pinned in the body. The fork-lane blind-read half is a pipeline constraint (fork screenshots are not materialized for the lane); the SHA-pinned images in the body are the human review path.
  • First-Principles: verify ⌘←/⌘→ on a real macOS browser: accepted — flagged to the PR author for a manual pass on the macOS desktop app and Safari/Chrome (this box is Linux; manual verification so far used Ctrl+← in Chromium). The claim-at-stack-bottom behavior deliberately mirrors the existing ⌘[/⌘] session-cycling claim two lines above it.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author

CI note for a maintainer: the two reds on head c37e082 (Backend Tests Windows shard 4, and the E2E job's backend phase) are the same single failure — test/test_work_ledger.py::test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding, the Windows lock-file binding race that #9237 fixed a truncation in and #9257 + faf4d9b recently instrumented ("expected 3 threads to report already_bound, got 1"). This PR is frontend-only (website/ + temp-screenshots/; the backend suite runs here via the temp-screenshots catch-all classification, #8027) and touches no ledger or locking code. Could someone re-run the failed jobs? Fork PRs cannot self-serve gh run rerun --failed. All 5 AI review lanes pass on this head; this is the only blocker.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Forward reload behavior is documented incorrectly span=2f6082500a24 — fixed
    self-added: yes

Fixed in 39f4249: the feature-map row now states the actual behavior — the Forward watermark persists per tab in sessionStorage, so a reload keeps a Forward branch reachable. The stale 'under-reports after a reload by design' clause was the previous round's text surviving the sessionStorage fix; the PR body's matching sentence was corrected in the same pass. check_feature_map.py green against upstream/main.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • DEFAULT_SHORTCUTS.find advertises the default chord after it is rebound or unbound span=a9b3ff544cd9 — fixed
    self-added: yes
    mechanism: arrows read useShortcutBindings() (live resolved overrides) instead of DEFAULT_SHORTCUTS

Fixed in 39f4249, exactly as suggested: both hints derive from the live resolved binding via the existing useShortcutBindings() — the same source the keydown handler dispatches from, so hint and binding cannot disagree. A rebound chord is announced correctly in aria-keyshortcuts; an unbound one advertises nothing. The catalog tooltip can only spell the factory chord (localized modifier names), so it is shown while the live binding still equals the platform default and drops to the bare label once rebound. New test pins rebind + unbind. This ruling covers the class: any chord advertisement reads the resolved binding, never the registry default.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Registry comment ships the opposite of the narrow-viewport behavior span=a8a59be14dd2 — fixed
    self-added: yes

Fixed in 39f4249: the shortcutRegistry.ts entry comment and the PR description now both state claim-as-no-op on narrow viewports (with the why — an unclaimed ⌘← is native browser Back on macOS and would pop past an unarmed draft trap), matching the handler and the test that asserts defaultPrevented === true. Text-field unclaim is described separately as the one true unclaim.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • ChatPage token/prefill strippers preserve history.state — undeclared span=96bbdfbe2719 — fixed
    self-added: yes

Fixed in 39f4249 (PR body): 'What changed' now has a paragraph declaring the ChatPage edits — both replaceState({}, …) sites (the ?prefill= consumer and the channel-token consumer) now pass window.history.state through, why (the wipe made react-router compute every later idx as NaN, disabling the arrows on every ?token= entry), and the Number.isFinite hardening in routerEntry().

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Narrow-window chord claims ⌘←/Ctrl+← and does nothing, killing native Back silently span=33604a434d59 — rebutted
    self-added: yes

Confirmed as the accepted tradeoff, per the review's own 'smallest fix if wanted: none needed in code — confirm the tradeoff is acceptable.' The alternative — releasing the chord — is the data-loss hole the GPT round-5 blocker (span=516000879ef9) upheld: on macOS the released keystroke is native Back past an unarmed draft trap. Navigating would walk a stack the drill-in UI does not reflect. Claimed-and-inert is what remains, on a low-frequency surface (a narrow desktop browser window). Same ruling as span=70203bb22fc5 in round 6; the registry comment and PR body now document it explicitly (span=a8a59be14dd2).

@peterhieuvu
peterhieuvu force-pushed the feat/nav-history-arrows branch from 39f4249 to afb591d Compare September 10, 2026 02:07
@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Trap entries become reachable Forward destinations span=1934726aab1d — fixed
    self-added: yes
    mechanism: isTrapEntry moved into lib/routeHistoryPosition.ts; trap entries excluded from the Forward watermark and from both live reads

Fixed in afb591d, as the finding prescribes. The guard's trap duplicate is a real stack entry pushed through the router, and the tracker was counting it as a destination. Now recordRouteNavigation, canGoBack and canGoForward all recognise a trap entry (via the marker, isTrapEntry, which moved to the lib so the store and the guard share one definition) and treat standing on it as standing on the page beneath (idx − 1) — so a trap never raises the watermark and Forward never offers it. The reported chain is now unreachable: an accepted Back then Forward cannot land on the trap, so the guard's carry-through never no-ops at the stack top and selfMove cannot leak past the next native Back. New test walks the exact scenario (armed trap → Forward disabled → accepted Back → Forward to the page → Forward still absent); the guard's own suite passes on the moved helpers. This ruling covers the class: history bookkeeping never counts a trap entry as a position.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Conservative degrade is invisible in production — dev-only warning when idx reads null on a PUSH span=6fbd15497b04 — fixed
    self-added: yes

Fixed in afb591d: recordRouteNavigation now emits one console.warn per PUSH under import.meta.env.DEV when history.state carries no router idx, so a react-router upgrade that changes the state shape surfaces in the dev console instead of as a 'arrows never enable' report. Production behavior unchanged (silent conservative disable). Uses the codebase's existing DEV-gated warn idiom (petBridge.ts).

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Narrow-viewport claimed no-op suppresses native Back with no feedback span=e55971218bd4 — rebutted
    self-added: yes

Same tradeoff as rounds 6 and 8 (span=70203bb22fc5, span=33604a434d59), re-affirmed with the review's own framing that no code fix may be safe: releasing the chord re-opens the upheld round-5 data-loss hole (span=516000879ef9 — an unclaimed macOS ⌘← is native Back past an unarmed draft trap), and navigating would walk a stack the drill-in UI does not reflect. Claimed-and-inert on a narrow desktop-browser window is the accepted residual, now documented in the registry entry comment and the PR body. A human owns this one, as the review asks; it is stated plainly for the maintainer.

@peterhieuvu
peterhieuvu force-pushed the feat/nav-history-arrows branch from afb591d to 673fc2f Compare September 10, 2026 03:08
@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Module init restores the persisted watermark on every fresh same-tab document navigation, not just reloads span=e72ae8444f6f — fixed
    self-added: yes
    mechanism: loadMaxIdx() gated on arrivedByFreshNavigation() (moved to the lib, shared with NavigationBackGuard); stale value cleared on a fresh arrival

Fixed in 673fc2f, exactly as prescribed: on a document whose navigation-timing type is navigate the persisted watermark is discarded and the sessionStorage key cleared, so a location.href/typed-URL/popout arrival starts with no Forward instead of a lying one; reload and back_forward arrivals keep the restore. arrivedByFreshNavigation moved from the guard into the lib so both consumers share one definition. New test models the fresh-arrival case (jsdom exposes no timing entry, so it is stubbed) and asserts Forward stays absent and the key is reset. This ruling covers the class: a persisted stack fact is trusted only across arrivals that preserve the stack.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Fresh tabs can falsely enable Forward (cloned sessionStorage popout) span=2f6082500a24 — fixed
    self-added: yes

Fixed in 673fc2f by the same change (see the Opus disposition for span=e72ae8444f6f): a _blank popout that inherits the opener's sessionStorage arrives with navigation type navigate, so the cloned watermark is discarded at module init and Forward reads disabled. The feature-map row now states the persistence scope precisely — kept across a reload or Back/Forward return, discarded on a fresh navigation — so 'disables when there is nowhere to go' holds again.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • A truncation outside the tracker's life never resets the persisted watermark span=1828df9fb395 — fixed
    self-added: yes

Fixed in 673fc2f along the review's own 'clears when': the restore is gated on arrival type via arrivedByFreshNavigation() (now shared from the lib), so the typed-URL / fresh-entry case is closed, and the module header names the residual it asked to have named — leaving by an external link and returning by browser Back restores a watermark that PUSH truncated in another document; stale until the next in-app push, cost one dead click, self-healing.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Cap the restored watermark sanity-wise (discard when idx === 0 on a non-reload arrival) span=965137e20946 — fixed
    self-added: yes

Subsumed by the span=1828df9fb395 fix in 673fc2f: gating on arrival type discards the watermark on every fresh navigation, not only the idx-0 case, so the one-line cap is not needed as a separate mechanism.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Dead keystroke with native Back suppressed at the stack bottom / narrow viewport span=de6c07b6a4e5 — rebutted
    self-added: yes

Deliberate, on the same evidence as rounds 6, 8 and 9 (spans 70203bb22fc5, 33604a434d59, e55971218bd4): the suggested 'unclaim at stack bottom when no dirty stake' would release ⌘← to native Back exactly where the trap is UNARMED — post-reload with a stake that has not yet published, or in the calibration gap the guard documents — which is the upheld round-5 data-loss finding (span=516000879ef9). The claim mirrors the pre-existing ⌘[/⌘] session-cycling claim two branches above it, which already swallows native Back on macOS at every stack position. A narrow desktop-browser window at idx 0 pressing a history chord is the residual; noted for the maintainer to own.

@peterhieuvu
peterhieuvu force-pushed the feat/nav-history-arrows branch from 673fc2f to c445081 Compare September 10, 2026 04:07
@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Orphaned routerEntry docblock detached from its function span=04b2e2737b69 — fixed
    self-added: yes

Fixed in c445081: the routerEntry docblock now sits directly above routerEntry() again; the TrapEntryState type and isTrapEntry keep their own docblock above them. Comment-only change; suite green (31,326).

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Documented residual: return by browser Back after an external link leaves Forward enabled but inert span=8de41b708a21 — rebutted
    self-added: yes

Ratified as the accepted residual, stated for the maintainer: the truncation happens in ANOTHER document (the external page's history), where no code of this app runs, so no in-app signal exists to observe it; the return arrives as back_forward, indistinguishable from a legitimate stack-preserving return, which is exactly the case persistence exists for. Detecting it would require dropping persistence for every back_forward return — trading a rare one-dead-click (self-healing on the next in-app push) for the common desktop Forward dead-end this PR was asked to close (span=216905c885dd, round 6, taken from the UX lane's own suggestion). Named in the module header so the next reader finds it deliberate.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • Documented external-link residual: stale watermark enables an inert Forward after return-by-Back span=9b8d19e80831 — rebutted
    self-added: yes

Same residual ratified at span=8de41b708a21 (previous round), unchanged: the truncation happens in another document where no app code runs, and the return arrives as back_forward — indistinguishable from the stack-preserving return persistence exists for. On the 'refresh the snapshot if no popstate arrives' idea: the dead click is a navigate(1) that react-router turns into history.forward(), and the browser fires no event when there is nowhere to go, so the only observable is a timer racing a pop that may legitimately be slow — a heuristic that can also clear a live arrow. Rare, self-healing on the next in-app push, named in the module header; stated here for the maintainer to own.

@peterhieuvu

Copy link
Copy Markdown
Contributor Author
  • After a rebind the tooltip drops to the bare label; the real chord reaches only assistive tech span=00ad4d3b3409 — rebutted
    self-added: yes

Deliberate consequence of the i18n constraint already ruled on at span=06dcfdf60194 / round 3: composing label + formatShortcut() at render leaks raw Latin past the pseudolocale and fails the repo's i18n render gate on every route, and hardcodes modifier names locales rename (de: Strg). A catalog string can only spell the factory chord, so a rebound chord gets the bare label plus a correct aria-keyshortcuts. A rebinder has, by definition, just typed the chord they chose, and the shortcuts modal (⌥K) and Settings → Shortcuts both show the live binding for sighted rediscovery. Widening the tooltip to a localized chord-composition seam is a separate i18n change, not this PR's.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 10, 2026
Browser-style Back/Forward over the SPA's route history (kirodotdev#8258): two arrow
buttons in the header's left cluster (desktop layout), plus Cmd+Left/Right
on macOS and Ctrl+Left/Right elsewhere, both driving navigate(+-1) so the
NavigationBackGuard draft trap covers them identically.

Disabled states derive from react-router's history.state.idx with a
conservative Forward watermark (module store, useSyncExternalStore).
Chords are gated out of text fields and terminals to preserve caret and
PTY meanings. i18n keys in all 13 catalogs + en-XA + translator context.
@peterhieuvu
peterhieuvu force-pushed the feat/nav-history-arrows branch from c445081 to 5fc3a95 Compare September 10, 2026 18:32
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: Back/Forward navigation via Cmd+Left/Right and mouse back/forward buttons

2 participants