Skip to content

feat: add vim-like memory browse keybindings - #29

Merged
nonnil merged 3 commits into
fix/performfrom
feat/vim-like-key-bind
May 31, 2026
Merged

feat: add vim-like memory browse keybindings#29
nonnil merged 3 commits into
fix/performfrom
feat/vim-like-key-bind

Conversation

@nonnil

@nonnil nonnil commented May 31, 2026

Copy link
Copy Markdown
Member

Summary

  • Added Task 22 workflow docs for vim-like /memories browse keybindings.
  • Implemented route-local browse keyboard operation: j/k move the selected memory, / focuses search, Escape exits search focus, and l/Enter opens the selected memory.
  • Added component/source contract coverage and Playwright coverage for the keyboard flow.

Verification

  • bun run typecheck
  • bun run test -- tests/components/memory-browse-actions.test.ts
  • bun run test:e2e -- e2e/browse-shell.spec.ts -g "supports vim-like"
  • bun run verify
  • bun run test:e2e -- e2e/browse-shell.spec.ts

Notes

  • Base branch is fix/perform, matching the requested worktree base.
  • bun run verify completed successfully; build emitted the existing Node DEP0155 warning from defuddle -> temml.

@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4630b83d-cad4-41a8-91c1-b02dfa434c36

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/vim-like-key-bind

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nonnil
nonnil requested a review from Copilot May 31, 2026 08:18
@nonnil
nonnil marked this pull request as ready for review May 31, 2026 08:18
@nonnil

nonnil commented May 31, 2026

Copy link
Copy Markdown
Member Author

review @codex @coderabbitai

@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 29 --repo hauntedfail/Trauma

echo "---FILES CHANGED---"
gh pr diff 29 --repo hauntedfail/Trauma --name-only

echo "---FULL DIFF---"
gh pr diff 29 --repo hauntedfail/Trauma

Length of output: 22490



@nonnil: Here is my review of the PR.


Review: feat/vim-like-key-bind

The overall implementation is clean and well-structured. ID-based selection, proper SolidJS onCleanup placement, and the buildMemoryBrowseItemHref refactor are all solid choices. Three things worth addressing:


🔴 Medium — Enter suppression doesn't cover focused buttons/links

isBrowseKeyboardSuppressed ignores the keyboard event only for text-entry targets (input, textarea, select, contenteditable). When a <button> or <a> element has focus and the user presses Enter, the handler still fires openSelectedMemory and the focused element also activates, causing potential double navigation.

 function isTextEntryTarget(target: EventTarget | null): boolean {
   if (!(target instanceof HTMLElement)) {
     return false;
   }
-
-  return target.isContentEditable || target.matches("input, textarea, select");
+
+  return (
+    target.isContentEditable ||
+    target.matches("input, textarea, select, button, a[href], [tabindex]")
+  );
 }

Or alternatively, only suppress l/Enter when a focusable interactive element has focus:

const active = document.activeElement;
if (
  (event.key === "l" || event.key === "Enter") &&
  active instanceof HTMLElement &&
  active.matches("a[href], button, [tabindex]")
) {
  return; // let the focused element handle it natively
}

🟡 Minor — Escape in search bar may clear the input value in Chromium

type="search" inputs in Chromium fire the browser's default Escape behaviour (clears value, fires input event) before the element is blurred. Without event.preventDefault(), pressing Escape will reset the search query back to an empty string.

 onKeyDown={(event) => {
   if (event.key === "Escape") {
+    event.preventDefault();
     event.currentTarget.blur();
   }
 }}

🟡 Minor — memoryLinkRefs Map accumulates stale entries

The memoryLinkRefs Map is populated via ref callbacks but is never pruned. When memories are filtered out or deleted, their HTMLAnchorElement entries remain in the Map. In SolidJS, ref callbacks do not fire with null on unmount, so stale detached-DOM refs accumulate for the lifetime of the route.

+  onCleanup(() => {
+    memoryLinkRefs.delete(memory.id);
+  });

…added inside the <For> loop's render function (e.g., via a createEffect on the memory ID), or by clearing the whole Map on list changes. Functionally this is benign (scrollIntoView on a detached element is a no-op), but it is a memory leak for large or frequently-filtered lists.


✅ What's working well

  • ID-based selection (selectedMemoryId) correctly preserves/loses selection across filter changes without requiring array-index bookkeeping.
  • onCleanup inside onMount is idiomatic SolidJS — the listener is guaranteed to be removed when the route unmounts.
  • buildMemoryBrowseItemHref extraction cleanly deduplicates the flashback-anchor logic shared between MemoryItem and openSelectedMemory.
  • isBrowseKeyboardSuppressed dual-checking event.target and document.activeElement is a useful defensive guard.
  • E2E test covers the full keyboard flow end-to-end including the search-focus-then-Escape path.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d155b3751

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/components/memories/MemoryBrowse.tsx
Comment thread e2e/browse-shell.spec.ts
@nonnil

nonnil commented May 31, 2026

Copy link
Copy Markdown
Member Author

CodeRabbit follow-up for #29 (comment)

Handled in f36e8b1:

  • Enter/l now yield to focused native activation targets, so focused links/buttons are not overridden by the selected memory shortcut.
  • Escape in the search input now calls preventDefault before blur, preserving the current query.
  • memoryLinkRefs are pruned when filtered memories change so detached result links do not accumulate.
  • The vim-like E2E waits for result links before sending movement keys.

Validation: component source contract test, typecheck, targeted E2Es, full e2e/browse-shell.spec.ts, and bun run verify all pass.

@nonnil

nonnil commented May 31, 2026

Copy link
Copy Markdown
Member Author

@codex review
@coderabbitai review

Review feedback from the previous pass has been addressed in f36e8b1. Inline threads were replied to and resolved; local verification and the current PR check are complete.

@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the specific commit diff
gh api repos/hauntedfail/Trauma/commits/f36e8b1883c62677576de13e3833e1a0b41316a7 --jq '.files[] | "=== \(.filename) ===\n\(.patch)"'

Length of output: 5110



@nonnil All three issues from the previous pass are cleanly resolved in f36e8b1:

Issue Fix Verdict
Enter/l double-firing on focused links/buttons isNativeActivationTarget() guard via .closest("a,button,[role='button']") inserted before preventDefault + new E2E test
Escape clearing type="search" value in Chromium event.preventDefault() added before blur() in MemorySearchBar; E2E now asserts toHaveValue("reader") post-Escape
Stale memoryLinkRefs entries accumulating createEffect reactive to filteredMemories() prunes the Map; contract test asserts memoryLinkRefs.delete(memoryId) is present

One small note worth calling out as a positive: isNativeActivationTarget using .closest() is slightly more robust than the original suggestion — it correctly handles the case where the keyboard event target is a child element (e.g., a <span> inside an <a>), which a bare matches() check would miss. Good call.

The implementation looks solid and ready to merge.

[approve]

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@nonnil
nonnil merged commit 4e27a0c into fix/perform May 31, 2026
1 check passed
@nonnil
nonnil deleted the feat/vim-like-key-bind branch May 31, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants