feat(web): make edit-field shortcuts discoverable with a which-key menu - #2762
Merged
Conversation
The `e`-then-letter field jumps existed but nothing taught them. Pressing the leader was silent for 600ms then gave up, and the seven legend rows were gated on live DOM focus, which the legend destroyed by focusing its own search input, so they were unreachable in the UI. - Unify the leader: `useEditSequenceShortcut` now owns both bare `e` and `Mod+E`. A second listener claiming `Mod+E` would have fired sequences twice, so `useEventFormFieldJumpShortcuts` (a near-duplicate) is gone. - After the 600ms fast path elapses, open a which-key menu anchored to the focused card (falling back to the form, then the viewport) and stay armed until a key, Escape, pointerdown, or blur. Height is measured rather than assumed so a flipped menu always clears its anchor. - Add `canArm` so a stray `e` with nothing to edit no longer swallows the next keystroke. - Drop the `eventFocused` gate instead of working around it, which also retires `useIsGridEventFocused` and collapses seven duplicate `form-jump-*` rows into one `Mod+E` row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ence No behavior change; verified the menu still flips above a bottom-edge event with zero overlap, that `e`-then-letter and `Mod+E`-then-letter still land on the right field, and that the legend rows are byte-identical. - Delegate placement to @floating-ui/react, already a dependency used the same way by ActionsMenu and the context menu. offset/flip/shift measure the real rendered box and autoUpdate tracks scroll, which deletes the hand-rolled rect math, the height estimate and its measuring layout effect, the duplicated off-screen predicate, and the scroll/resize tick copied from ShiftHintOverlay. - Derive the dispatch map, the which-key menu, and the seven legend rows from one `edit-sequence.fields` list. They had already drifted: the legend said "Edit recurrence" while the menu said "Repeat". - Reuse the existing `isBareLetterKey` helper instead of a hand-rolled modifier check, matching the two sibling leader hooks. - Drop the `announcement` field and two selectors that nothing read, hoist a `normalizeKey` helper, call `isModLeader` once, and drop a redundant guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Event jump already stands down for an armed sequence, but the reverse check was missing, so a bare `e` during jump mode armed the edit sequence beneath the jump hints. Harmless before, when the arm expired silently after 600ms; now it opens the which-key menu over the hints and steals the next day letter (`t` for Tuesday and `r` for Thursday are both edit-sequence keys). Mirrors the existing yield with an `isEventJumpActive()` read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolved `useEditSequenceShortcut.ts`: kept main's `LEADER_KEY` derived from `KEYMAP.editTitle.sequence.leader` and its exported `ARM_WINDOW_MS` (the showcase practises the real cadence off it), on top of this branch's store-based arm state and the field list moved out to `edit-sequence.fields.ts`. `keymap.test.ts`'s parity check now reads `EDIT_SEQUENCE_FIELD_BY_KEY`, the same key->field record under its new home. The showcase holds the app lock while it is up, so the unified leader stays inert during a lesson and the two cannot double-fire. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard avoids a store write and devtools entry on every grid click. An earlier comment claimed it was load-bearing for drag-to-create; that was a hypothesis about a pre-existing e2e failure and it was disproven, so the comment now says only what is true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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
The
e-then-letter field jumps existed but nothing taught them, and the oneplace that documented them was broken.
Three defects, all fixed here:
e-sequence rowswere gated
when: { eventFocused: true }, andeventFocusedcame from liveDOM focus. Opening the legend focuses its own search input, which fired
focusouton the event and filtered the rows away in the same tick. Theywere unreachable in the UI.
egave no feedback for 600ms, thendisarmed. There was no way to learn the second keys by experiment.
Mod+Elooked like a "show hints"chord but only armed the in-form variant, and
?cannot open the legendfrom inside a form field (TanStack's
ignoreInputsdefault), so those rowswere invisible too.
What changed:
useEditSequenceShortcutnow owns both baree(bails oneditable targets) and
Mod+E(works anywhere, including inside TipTap).A second listener claiming
Mod+Ewould fire sequences twice, so thenear-duplicate
useEventFormFieldJumpShortcutsis deleted. The form onlyever renders inside Week and Day, both of which mount the leader.
sees no UI. Past it, a menu of the seven second keys opens anchored to the
focused card (falling back to the form panel, then the viewport) and stays
armed until a key, Escape, pointerdown, or blur.
canArm, so a strayewith nothing to edit no longer swallows thenext keystroke.
useIsGridEventFocusedand its prop threading, removeseventFocusedfromShortcutContext, and collapses seven duplicateform-jump-*rows into oneMod+Erow.Also included: a one-line
.claude/launch.jsonfix so the dev server resolvesbunfrom PATH instead of a hardcoded/opt/homebrewpath.Simplicity
Net effect is two files deleted and seven duplicate registry rows removed
alongside the new menu.
/simplifyran as its own commit (60b2c3f) and madethree reuse cuts:
@floating-ui/react's, already a dependency used the sameway by
ActionsMenuand the context menu.offset/flip/shiftmeasurethe real rendered box and
autoUpdatetracks scroll, which deleted thehand-rolled rect math, a height estimate plus its measuring layout effect,
a duplicated off-screen predicate, and a scroll/resize tick copy-pasted from
ShiftHintOverlay.from
edit-sequence.fields.ts. They had already drifted: the legend said"Edit recurrence" while the menu said "Repeat".
isBareLetterKeyinstead of a hand-rolled modifier check, matchingthe two sibling leader hooks; dropped an
announcementfield and twoselectors nothing read.
Considered and skipped: a shared "keyboard mode" primitive across the three
leader hooks (their lifecycles genuinely differ, so it would be an abstraction
fitted to three shapes it does not share), and extracting the shared
suppressKeyUplogic (would mean editinguseShiftHoldEventHints, outsidethis diff).
Retained React state, per repo convention on justifying it:
useRefforgetAnchor/onSequence/canArmbecause the listeners mount once and muststill see fresh callbacks — the same idiom
useShiftHoldEventHintsuses.Automated validation
Browser session against the local dev server (
http://localhost:9094,frontend-only anonymous/IndexedDB mode), Week and Day views:
e, wait past 600mslMod+EthenRwith caret in the title inputaria-label="Edit recurrence")?with nothing focusedewith nothing focused, thenttnot swallowed (defaultPrevented === false)ttpasses through, no keyup suppression leakjjpasses through and navigates weeksConsole: only
ERR_CONNECTION_REFUSEDagainst the backend on :3014, expectedwith
dev:webalone. No app or React errors; server logs clean.Not covered in-browser: the grid path's
focusFieldAfterPaintuses a doublerequestAnimationFrame, and the preview tab reportsdocument.hidden === truewhere rAF never fires, so focus fell to the title's
autoFocusinstead of therequested field there. That helper is unchanged from
main; the behavior iscovered by unit tests and by the
Mod+Epath above, which takes no rAF.Independent review
A fresh reviewer read the final diff without the implementation notes and
reported no confirmed findings. It explicitly verified: Escape handling
does not break closing the form, the module-global arm state cannot diverge
from the per-instance timers (Day and Week are never co-mounted and each
cleanup disarms), deleting the form-side hook is safe because the form only
renders under Week/Day, the floating-ui reference is seated without a stale
closure, and the tests assert behavior rather than tautologies.
It raised one asymmetry below its confidence bar, which I judged real and
fixed in 777d58c: event jump yields to an armed sequence, but not the
reverse, so a bare
eduring jump mode armed underneath the hints. Harmlessbefore (the arm expired silently in 600ms); with the menu it steals the next
day letter, since
tandrare both jump letters and edit-sequence keys.An earlier
/code-reviewpass confirmed one finding, fixed before the reviewabove: the menu height was a hardcoded constant, so a flipped menu could cover
the event it points at. Superseded by the floating-ui move, which measures the
real box. One low-severity item is reported but not fixed: the menu's
aria-liveregion is inserted together with its content, so screen readers maymiss the announcement. Every existing indicator in the repo
(
EventJumpIndicator,ShortcutTipIndicator) does the same, so changing onlythis one would be inconsistent.
Test plan