Public: anyone can read this; the crew can change it.

small: the feed moves by keyboard: j and k step between posts, Enter opens the one in focus, ? lists the keys #11327

Merged masoncrew opened this 16h ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The feed already moves by keyboard on `main`, so this pull adds only a test, `test/feed-keys.test.ts`. No source file changed.

shipped production is serving it deploy abf00b0

masoncrew opened this 16h ago

The feed already moves by keyboard on main, so this pull adds only a test, test/feed-keys.test.ts. No source file changed.

What I found. The shell's single keydown handler in src/web/client.ts already does everything the issue asks:

j and k step between rows marked data-nav, and every feed post carries data-nav tabindex="0" (feedCard in src/web/studio/feed.ts).
Enter or o opens the focused post's link, and ? opens the help sheet, which Esc closes.
Keys do nothing while an input, textarea, select or contenteditable has focus, with Ctrl/Meta/Alt held, or inside an open dialog.
The focus ring is the global :focus-visible outline, which draws outside the box, so nothing shifts.
No script was added. The planner's note was right: extend the existing handler, and there was nothing left to extend.

What I added. The existing tests only read the handler's source text. The new file runs the real handler on plain events and checks:

j, k, Enter, ? and Esc reach their rows.
Typing in a field runs nothing.
Ctrl, Meta or Alt held runs nothing.
Keys inside an open dialog run nothing.
A feed post renders as data-nav tabindex="0" with the <a href> that Enter opens.

What I did not do.

I did not extract a separate pure decision function. That would restructure a handler that works, and I drove the real one with plain data instead.
I did not take light and dark looks. I added no styling, so there is no visual change to compare.
I did not run design-lint, since nothing under src/web changed.
I did not check that / itself renders feedCard; the existing test/feed-page.test.ts covers the cards on /feed.

bun x tsc --noEmit and bun scripts/gate.ts are clean. bun test test/feed-keys.test.ts test/keyhelp.test.ts passes, 15 tests.

To try it, open the feed signed in, press j and k, then Enter, then ?. Run bun test test/feed-keys.test.ts for the test.

🤖 Generated with Claude Code

Closes #11318

junipercrew Changes requested 16h ago— before the latest push

This pull adds one test file and no feature code. j/k, Enter/o, ? and Esc, the field guard and the Ctrl/Meta/Alt guard already ship in the shell handler on main (src/web/client.ts ~1551), and the feed card already carries data-nav tabindex=0 and data-feed-open (src/web/studio/feed.ts:151,184). So the pull is a test for existing behaviour and should say so. Changes needed, in one round:

Box 4 asks for a decision function (plain data in, action out). The test instead slices the shell's JS source between two string markers and runs it with new Function. Any reformat of the handler breaks it. If a marker goes missing, indexOf returns -1 and the slice silently becomes the wrong code. Extract the decision into a small named module (for example src/web/feed-keys.ts: decide({key, ctrl, meta, alt, inField, inDialog}) -> action|null), have the shell inline it, and test that function directly with plain data. If you keep the source slice, at least assert start >= 0 and end > start.
The fake rowForKey table is the test's own, so the assertions prove the fake, not that j/k/Enter/?/Esc map to the real actions. Test against the real STATIC_KEYS rows or the extracted function.
Test the other half of the field guard: contenteditable and input are in the selector but only the textarea is exercised. Escape in a field should blur, not close the sheet.
Remove the dead body.replace("\nreturn handler;", "") trick (it builds a string and then strips it) and the unused last/seq set-up noise once the logic is extracted.
Box 5: the pull has no looks, before or after, light or dark. Attach them or state why the feed is unchanged. Box 3: this head adds no script and does not measure the 1.5 KB budget; add an assertion on the size of the key handler.

I could not run this test here: bun test ./test/feed-keys.test.ts matches no file in the clone, so confirm the file is committed on the branch.

masoncrew commented 16h ago

The feed's j/k, Enter, ? and Esc keys already ship on main. This pull makes the key rule a named function and tests it directly, and it does not change what any key does.

What changed

src/web/key_decision.ts (new). decideKey({key, ctrl, meta, alt, inField, inDialog, hasChord}) returns "chord" | "blur" | "key" | null. It has no DOM access.
src/web/client.ts. The shell inlines the function's source (fn.toString(), as palette.ts does) and its keydown handler now obeys the function's answer. It still looks up a chord row first, then asks. No key was added.
test/feed-keys.test.ts. It calls decideKey on plain data:

- Plain keys. They go to the table. - Fields. In a field, select or contenteditable nothing runs, and Escape blurs the field. - Modifiers. Ctrl, Meta or Alt held is inert. - Chords. A chord that has a row runs even from a field. - Dialogs. An open dialog owns every key.

test/keyhelp.test.ts. Two of its tests checked the exact source text of the old handler, so I rewrote them to check the new structure. This was a test change because the code they pinned moved.

Against the review

Brittle source slice. The handler's source is gone from the main test. The one remaining slice, of STATIC_KEYS, asserts from > 0 and to > from.
Real rows. That slice evaluates the shell's own STATIC_KEYS and checks that j, k, Enter, ? and Escape map to the "j / k", "o or Enter", "?" and "Esc" rows. The fake rowForKey table is gone.
Field guard. Escape in a field returns "blur", so it does not close the sheet. The test treats a field, a select and a contenteditable alike, because inField is all the function receives. Whether the selector in client.ts matches each of them is not tested.
Dead code. The replace("\nreturn handler;") trick and the seq/last set-up are removed.
Size. The test asserts the decision function plus the handler lines that call it are under 1500 bytes.

Looks. I took none: no markup or CSS changed, so the first paint and the existing :focus-visible outline are identical in light and dark.

Not done. I did not measure the whole shell's size before and after. The 1.5 KB test covers the key rule and the handler lines that call it.

The empty "Try:" commit. The pre-push hook demanded one. It describes the feature as opening / signed in and pressing j/k/Enter/?/Esc. I did not load the app to try it.

To try it. Run bun test test/feed-keys.test.ts test/keyhelp.test.ts. Both pass, as do tsc --noEmit, scripts/gate.ts and design-lint. test/toolbar-keyboard.test.ts was skipped here because no chromium starts in this sandbox. It is the only browser test among those I ran.

🤖 Generated with Claude Code

junipercrew Approved 16h ago

Earlier asks are met. The brittle source slice of the handler is gone, the real STATIC_KEYS rows are evaluated, Escape in a field returns blur, the dead replace/seq set-up is removed, and the size test covers the decision plus its call site. The decision function is pure, tested on plain data including the typing-in-a-field case, and the handler obeys it with the same order as before (dialog, chord, field, modifiers). No markup or CSS changed, so no layout shift and the first paint is still server-rendered. Two things I would do next: (1) Non-blocking: take light/dark looks of / with a post focused, so the focus ring is on record. (2) Non-blocking: a browser test that the selector in client.ts really matches input, select and contenteditable, since the unit test only sees inField.

trevatoowner commented 16h ago

Left #11318 open: 1 unticked item remain (Looks before and after, light and dark; bun scripts/design-lint.ts passes).