Uh oh!
There was an error while loading. Please reload this page.
Make Transport Shortcuts Canonical and Discoverable - #28
Conversation
Starting initial review of |
There was a problem hiding this comment.
Architecture checkpoint
The new host shortcut port keeps ownership in the right place: the host is authoritative and the editor settings store is a scoped runtime mirror. The synchronization logic is necessarily more involved than a local preference because it has to cover asynchronous hydration, host replacement, subscription updates, queued writes, and disposal. Here that machinery is localized to one adapter, fences stale owners, normalizes bindings through the canonical resolver, and has targeted coverage for the meaningful races. I do not see a source-of-truth split that warrants restructuring this PR.
I found no actionable correctness issues in the reviewed diff. The canonical J/K/L defaults, Shift+K keyframe move, split-command migration, runtime conflict graph, host round trip, and dynamic shortcut labels line up across the touched surfaces.
I am leaving this as a comment rather than approving because the PR’s own real-browser checks are still outstanding: verify J/K/L routing in both monitors and a settings remap through controls, help text, and host subscription updates before promoting the stacked change.
Reviewed `bf0f2986`...`0f5602d9`. Diff collection and GitHub posting were handled by the automation.Review head |
Starting new changes review of |
There was a problem hiding this comment.
Follow-up review
The new physical-chord conflict graph and the retry reconciliation remain contained at the correct boundary: the host owns persisted shortcut settings, while the editor store is a scoped mirror. The added retry timer is fenced by the existing ownership epoch and is cancelled on acknowledgement, replacement, and disposal; I found no blocking regression in that flow.
- Warning — explicit Ctrl/Meta bindings cannot be recorded faithfully (
src/config/hotkeys.ts:763): the resolver now treatsctrlandmetaas distinct physical modifiers, but event capture still turns either into portablemod. Recording Ctrl+K on macOS becomes Cmd+K, and recording Meta+K on Windows becomes Ctrl+K. Preserve the physical modifier in capture (and provide an explicit portable-modchoice if that remains part of the UI contract), with platform-specific tests.
I am leaving this as a comment rather than approving because the PR still lists the real-browser transport and end-to-end remap checks as outstanding. Those checks should verify both monitors and the settings-to-host round trip at this head.
Reviewed `0f5602d9`...`0e711268`. Diff collection and GitHub posting were handled by the automation.Starting new changes review of |
There was a problem hiding this comment.
The physical-chord graph and runtime-only registration map are contained at the canonical hotkey boundary. Declaration-order ownership makes invalid legacy collision handling deterministic while preserving the raw bindings used by persistence and UI, so this does not introduce a competing source of truth.
- Warning — explicit Ctrl/Meta bindings still cannot be recorded faithfully (
src/config/hotkeys.ts:768): event capture continues to turn either physical modifier into portablemod. Recording Ctrl+K on macOS therefore saves Cmd+K, and recording Meta+K on Windows saves Ctrl+K, despite the resolver and runtime matcher now preserving those explicit bindings. Capture Ctrl and Meta separately, with a deliberate portable-modpath if desired, and cover each platform in the recorder tests.
The PR's real-browser checks are still listed as outstanding; those should cover J/K/L in both monitors and a settings remap through controls, help text, and host subscription updates.
Reviewed `0e711268`...`93d57a2d`. Diff collection and GitHub posting were handled by the automation.Starting new changes review of |
There was a problem hiding this comment.
Summary
The new live-claim ownership graph correctly releases aliases from rejected candidates and the centralized registration adapter keeps runtime binding resolution contained. One nonblocking recorder mismatch remains, and the PR's listed real-browser verification is still outstanding.
Architecture Assessment
Review mode:ARCHITECTURE_CHECKPOINT
Subsystems:shortcut-modifier-capture
Decision:KEEP_CURRENT
The hotkey registry remains the right boundary: it now resolves persisted bindings, physical aliases, derived preview chords, and deterministic runtime ownership in one place, while the registration adapter is a scoped consumer. The remaining recorder mismatch is local to event capture and does not require another source of truth or a redesign.
Accumulated mechanisms:
- Canonical shortcut registry and resolved conflict graph
- Platform-specific physical-chord expansion for mod/ctrl/meta collisions
- Deterministic declaration-order runtime ownership
- Runtime-only disabled bindings for invalid legacy collisions
- Canonical override normalization at persistence and import boundaries
- Legacy split-command and keyframe-binding migration
- Single runtime registration adapter for command bindings
Findings
- [Warning](shortcut-modifier-capture)
src/config/hotkeys.ts:768- The resolver, formatter, and runtime matcher now preserve explicitctrlandmeta, but event capture still turns either physical modifier intomod. Recording Ctrl+K on macOS therefore saves Cmd+K, while recording Meta+K on Windows saves Ctrl+K, so users cannot create or faithfully re-record the explicit bindings the new model supports. Preserve the physical modifier during capture (with an intentional portable-mod path if desired) and add platform-specific recorder coverage.
Starting new changes review of |
There was a problem hiding this comment.
Architecture checkpoint
The new registration adapter is the right boundary. Command registrations now receive their runtime binding from the canonical resolver, while the only non-command path is a closed dopesheet-local key map. The AST-backed coverage also protects that boundary from future direct react-hotkeys-hook imports. This reduces scattered registration ownership rather than introducing another source of truth, so I would keep the current shape.
I found no new actionable issue in the supplied incremental diff. The earlier explicit Ctrl/Meta recorder limitation is outside this incremental file set and remains a nonblocking follow-up.
I am leaving this as a comment because the PR still lists its real-browser checks as outstanding: verify J/K/L in both source and program monitors, then remap transport and split shortcuts through Settings and confirm the controls, help text, and host subscription round trip at this head.
Reviewed `83880829`...`dc40a835`. Diff collection and GitHub posting were handled by the automation.Starting new changes review of |
There was a problem hiding this comment.
Follow-up review
The new registration adapter keeps runtime binding resolution at the canonical hotkey boundary. Command callers are limited to typed command keys, the modifier-derived preview path remains explicitly scoped to Mark In/Out, and the only raw registrations are a closed dopesheet-local map. The AST-backed import check makes that ownership boundary enforceable across production sources rather than relying on convention.
I found no new actionable issue in the supplied incremental diff.
I am leaving this as a comment because the PR still lists its real-browser checks as outstanding. Before this is promoted, verify J/K/L routing in both source and program monitors, then remap transport and split shortcuts through Settings and confirm the controls, help text, and host subscription round trip at this head.
Reviewed `dc40a835`...`026c62d4`. Diff collection and GitHub posting were handled by the automation.Starting new changes review of |
There was a problem hiding this comment.
Follow-up review
The scope-aware import-boundary checker remains a focused guard around the existing registration adapter: it evaluates only static module expressions that Rolldown resolves, keeps function/class capture boundaries explicit, and adds parity coverage for lexical aliases and shadow barriers. I found no new actionable issue in the supplied incremental diff.
I am leaving this as a comment because the PR still lists its real-browser checks as outstanding. Before promotion, verify J/K/L routing in both source and program monitors, then remap transport and split shortcuts through Settings and confirm the controls, help text, and host subscription round trip at this head.
Reviewed `026c62d4`...`0eda4ba7`. Diff collection and GitHub posting were handled by the automation.Review of |
Starting new changes review of |
There was a problem hiding this comment.
Follow-up review
The repaired scope-aware boundary checker remains a focused guard around the canonical hotkey registration adapter. Its static evaluation is limited to expressions Rolldown can resolve, and the expanded fixture set covers the relevant lexical aliases, temporal-dead-zone cases, loop/catch barriers, require shadowing, and type-only imports. I found no new actionable issue in the supplied incremental diff.
I am leaving this as a comment because the PR still lists its real-browser checks as outstanding. Before promoting the stacked change, verify J/K/L routing in both source and program monitors, then remap transport and split shortcuts through Settings and confirm the controls, help text, and host subscription round trip at this head.
Reviewed `0eda4ba7`...`62e6c1d6`. Diff collection and GitHub posting were handled by the automation.Starting new changes review of |
There was a problem hiding this comment.
Follow-up review
The mutable-binding expansion remains within the existing import-boundary checker: it still has one canonical adapter, parses production sources in isolation, and checks its supported folding behavior against the pinned Rolldown version. I found one bounded guard-coverage mismatch.
- Warning — statically determined logical assignments evade the import boundary (
scripts/runtime-hotkey-import-boundary.mjs:421): every assignment beneath&&,||, or??is classified as uncertain. That includeslet pkg; true && (pkg = 'react-hotkeys-hook'); import(pkg): Rolldown 1.1.5 resolves the import, while the checker reports no violation. This permits a direct runtime import outside the adapter to pass the production guard. Model statically determined logical operands (or an equivalent folded assignment path) and add that case to the parity fixtures.
This is nonblocking because it requires a deliberately shaped future bypass and does not alter current runtime behavior, but the parity suite should cover it before relying on the checker for that form.
Reviewed `62e6c1d6`...`25f751d4`. Diff collection and GitHub posting were handled by the automation.Starting new changes review of |
There was a problem hiding this comment.
Follow-up review
The import-boundary checker remains a focused, read-only guard around the canonical hotkey registration adapter. The new control-flow ownership model distinguishes function/class-local writes from their enclosing dynamic branches, while resolving statically selected if, conditional, and short-circuit paths. That keeps the implementation at the existing enforcement boundary rather than creating another registration or binding path.
The previously reported bypass is addressed: true && (pkg = 'react-hotkeys-hook') is now classified as a reachable simple assignment, and the parity fixtures cover it alongside static branches, declaration ordering, and exception paths against the pinned Rolldown version.
I found no new actionable issue in the supplied incremental diff.
Reviewed `25f751d4`...`3726994b`. Diff collection and GitHub posting were handled by the automation.
Summary
FreeCut's core playback keys are now consistent with professional editors and discoverable in the interface. J, K, and L control reverse, pause, and forward playback by default, while hosts can read, update, and subscribe to the same shortcut settings they expose to users.
Technical details
Test plan
npm run check,npm run build, package consumer smoke coverage, and the repository pre-push gates.Open items
fix/host-shortcuts-roundtrip-5319, the exact PR fix(editor): host bridge round-trip stability + host-mode timeline shortcuts #13 source base. It must not land on or retargetmain.codepress-main, followed by a reviewed package version bump and CodePress dependency update.Demo
Focused remap/runtime video (WebM)