Skip to content

fix: hide sidebar shortcut hint without a target#447

Merged
cpcloud merged 6 commits into
mainfrom
codex/ctrl-key-activity-hint
Jun 8, 2026
Merged

fix: hide sidebar shortcut hint without a target#447
cpcloud merged 6 commits into
mainfrom
codex/ctrl-key-activity-hint

Conversation

@cpcloud

@cpcloud cpcloud commented Jun 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator
  • Hide the Ctrl+[ keyboard hint and command listings when the active page or compact presentation has no sidebar target.
  • Keep Ctrl/Cmd+[ reserved on those pages so browser Back does not fire, without toggling persisted sidebar state.
  • Keep the sidebar toggle active on pages where it can collapse or expand the sidebar, with regression coverage for Activity and compact PR lists.
  • Use real Playwright click interactions in diff e2e helpers so covered or disabled targets still fail tests.

@roborev-ci

roborev-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (6ba05b0)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 2m3s), codex_security (codex/security, done, 10s) | Total: 2m13s

@roborev-ci

roborev-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (8d8771a)

Medium finding remains.

  • Medium - packages/ui/src/components/diff/DiffReviewThreadInlineComment.svelte:40
    setupThreadLayout captures the layout container synchronously when the action is created. Inline review threads mounted for Pierre annotations are first mounted into a detached annotation host, so layoutContainer(node) can be null; the real .file-content/.diff-area is then never observed or subscribed for scroll, leaving thread widths stale after container resize or horizontal scrolling.
    Fix: Defer or refresh container attachment once the node is connected, and reattach observers/listeners whenever layoutContainer(node) changes.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 5m37s), codex_security (codex/security, done, 15s) | Total: 6m0s

@roborev-ci

roborev-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (e45b2a0)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 3m46s), codex_security (codex/security, done, 12s) | Total: 3m58s

@roborev-ci

roborev-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (7b85bff)

Medium-risk issue found: the change can let Cmd+[ fall through to browser Back on pages without a sidebar target.

Medium

  • frontend/src/lib/stores/keyboard/actions.ts:259
    Gating sidebar.toggle off on pages without a sidebar target also stops the dispatcher from calling preventDefault(). On macOS, Cmd+[ is the browser Back shortcut, so pressing the now-hidden/inactive sidebar chord on Activity, Repos, Board, etc. can navigate browser/app history instead of doing nothing.

    Fix: Keep reserving/consuming the chord when sidebar shortcuts are enabled, but make the handler no-op when there is no sidebar target; hide the visual hint/cheatsheet entry via separate visibility logic.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 6m7s), codex_security (codex/security, done, 1m2s) | Total: 7m17s

@roborev-ci

roborev-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (f260ce0)

Medium confidence: the PR is mostly clean, but there are two medium-risk frontend/test issues to address.

Medium

  • frontend/src/lib/stores/keyboard/actions.ts:36
    hasSidebarShortcutTarget uses only the route shape, so compact responsive /pulls and /issues views still expose and run the sidebar shortcut even though App.svelte renders those routes without a sidebar (hideSidebar=true or FocusListView). That leaves the shortcut visible and toggles persisted state on a layout with no target.
    Fix: Thread actual sidebar availability into the keyboard context, or make this predicate account for responsive focus/mobile presentation before returning true. Add a narrow-viewport e2e case.

  • frontend/tests/e2e-full/diff-view.spec.ts:440
    dispatchComposedMouseClick directly dispatches mouse/pointer events, bypassing Playwright actionability and browser hit testing. The diff e2e tests can now pass when file tree items, context expanders, or line comment buttons are covered, disabled, or otherwise not clickable by a real user.
    Fix: Use real Playwright interactions after scrolling/hovering, or click via page.mouse at the target’s bounding-box center so overlays and hit-target regressions still fail the test.

Panel: ci_default_security | Synthesis: codex, 11s | Members: codex_default (codex/default, done, 6m8s), codex_security (codex/security, done, 15s) | Total: 6m34s

@roborev-ci

roborev-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (bcc058a)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 5m36s), codex_security (codex/security, done, 12s) | Total: 5m48s

@cpcloud cpcloud force-pushed the codex/ctrl-key-activity-hint branch from bcc058a to adb8730 Compare June 7, 2026 15:39
@roborev-ci

roborev-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (adb8730)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 5m12s), codex_security (codex/security, done, 30s) | Total: 5m42s

cpcloud added 5 commits June 8, 2026 05:14
@cpcloud
fix: hide sidebar shortcut where it has no target
323fb89 
The sidebar shortcut was registered as a global action and the header rendered its Ctrl+[ badge whenever the persisted sidebar state was collapsed. On Activity, toggling that state has no visible sidebar to affect, so the shortcut only made the header affordance appear or disappear.

Gate the shortcut to PR list, issue, workspace, and terminal sidebar routes, and remove the badge from the generic header fallback. Activity now keeps the header stable while pages with real sidebars continue to receive the shortcut.
@cpcloud
fix: restore frontend CI checks
15c6f37 
The current mainline frontend CI is failing deterministically in lint and unit tests. Two test files only needed the checked-in formatter style, while DiffFile.test exposed a Svelte 5 teardown error from a dynamically mounted inline review thread component.

Move the inline review thread layout observer from onMount to an element action so imperatively mounted annotations can be cleaned up without creating a new effect during teardown. The action keeps the same resize, scroll, and width calculation behavior while avoiding the effect_in_teardown failure.
@cpcloud
fix: stabilize browser diff e2e checks
87f80b0 
The remaining red PR checks were the Firefox and WebKit browser lanes. Their failures were concentrated in diff-view interactions that click through Pierre shadow DOM or virtualized tree nodes, plus a Firefox inline-composer focus race.

Dispatch composed mouse events from the e2e helpers after the target is visible so those tests exercise the same delegated app handlers without relying on Playwright's cross-browser hit testing inside the third-party shadow widgets. The inline composer now schedules focus after mount across the next frames and a short timer, with a unit assertion covering the expected focused textarea. The CI dropdown test also waits for the expanded toggle text before asserting expanded row count so the assertion observes the state transition.
@cpcloud
fix: reserve hidden sidebar shortcut
051b35e 
Hiding sidebar.toggle by making its when predicate false also removed the dispatcher's preventDefault path. On macOS that left Cmd+[ free to become browser Back on Activity, Repos, Board, and other pages without a sidebar target.

Keep the action dispatchable whenever sidebar shortcuts are enabled, but make its handler no-op unless the current route has a sidebar target. UI command lists now use an optional visible predicate so the shortcut stays hidden where it would do nothing.
@cpcloud
fix: honor rendered sidebar availability
e27c51c 
The sidebar shortcut still used route shape to decide whether it had a target. Compact pulls and issues routes render the focus presentation without a sidebar, so Cmd+[ could be shown and could mutate the persisted sidebar preference on a layout where nothing would change.

Add actual sidebar availability to the keyboard context and keep compact pages in the reserved-but-hidden path. Also replace the diff e2e synthetic click helper with real Playwright hover/click interactions so covered or disabled targets still fail the tests.
@cpcloud cpcloud force-pushed the codex/ctrl-key-activity-hint branch from adb8730 to e27c51c Compare June 8, 2026 09:23
@roborev-ci

roborev-ci Bot commented Jun 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (e27c51c)

Medium confidence: the code changes look functionally reasonable, but two user-visible behaviors need stronger browser-level coverage before merge.

Medium

  • frontend/src/lib/components/keyboard/Palette.svelte:74
    The visibility change that hides Toggle sidebar from the command palette/cheatsheet on no-target pages is not covered by e2e tests. Current browser tests verify dispatch/header behavior, so these dialogs could still show the hidden shortcut and pass.
    Fix: Add Playwright coverage that opens the command palette and keyboard cheatsheet on Activity or compact PR list and asserts Toggle sidebar is absent while the chord remains reserved.

  • packages/ui/src/components/diff/DiffInlineCommentComposer.svelte:34
    The delayed focus retry is only asserted in a jsdom unit test, which does not exercise the browser timing/focus race this change is meant to fix. Existing diff e2e tests only check visibility before using fill().
    Fix: Extend the full-stack diff e2e composer flow to assert page.getByPlaceholder("Leave a comment") is focused immediately after opening.

Panel: ci_default_security | Synthesis: codex, 10s | Members: codex_default (codex/default, done, 5m57s), codex_security (codex/security, done, 54s) | Total: 7m1s

@cpcloud
test: cover hidden shortcut dialogs in browsers
9f1651b 
roborev-ci flagged that the no-target sidebar shortcut behavior was only covered through dispatch and header assertions, leaving the palette and cheatsheet surfaces unprotected. Extend the Activity shortcut e2e flow to open both dialogs, assert Toggle sidebar is absent, and still prove Cmd+[ remains reserved.

The diff composer focus retry also needs browser-level coverage, so the git-backed diff flow now checks that each newly opened inline composer has focus before filling it. Local Playwright execution in this workspace still stops at the missing ms-playwright Chromium executable, so CI remains the browser runtime for these assertions.
@roborev-ci

roborev-ci Bot commented Jun 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (9f1651b)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 6m12s), codex_security (codex/security, done, 40s) | Total: 6m52s

@cpcloud cpcloud merged commit 33a08ab into main Jun 8, 2026
16 checks passed
@cpcloud cpcloud deleted the codex/ctrl-key-activity-hint branch June 8, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cpcloud