feat(web): expose semantic search with IME-safe submission - #101
Conversation
Add a labelled natural-language search form using existing Workspace hybrid retrieval. Preserve debouncing, cancel pending submission timers, guard IME confirmation, and close mobile navigation on explicit search. Fixes #100
Cancel the pending debounced search from every query-context reset (tab change, type drill, route navigation, opening a Memory, Workspace change) through a shared cancelable debounce hook and an App-held cancel handle. Bound the Safari keyCode 229 Enter guard to a 500ms post-composition window so soft keyboards reporting 229 outside composition still submit, skip rescheduling when a composition changed nothing, and recover the composition guard on blur or a non-composing input event. Align the search label, submit button hover, and focus ring with the DESIGN.md token contract, and let a short desktop viewport scroll the sidebar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
corespeed-agent-ensemble
Bot
changed the title
|
Reviewed and pushed review fixes in 57c7edf (requested by Yunpeng). What the review found
What the fix does
Also retitled the PR Validation on 57c7edf: typecheck, full-repo lint, design:check, all 17 sidebar-search tests (5 new: Android bare-229 Enter, 229-window expiry + unchanged-composition skip, blur recovery, aborted-composition recovery, cancel handle), and the full vitest suite — 614/614 passing ( Left as-is, deliberately: GraphView's declarative 250ms debounce (its effect cleanup already cancels by construction; unifying it with the sidebar's IME handling is a separate, riskier change), and the search hint stays 12px sans — it is descriptive body copy, not a label. 🤖 Generated with Claude Code |
bun audit's high gate now fails on advisories published after this branch's last green run: Next.js <16.3.3 (two critical RCEs), sharp <0.35.4 via next and miniflare's exact 0.35.2 pin, and js-yaml <4.3.2 via openapi-typescript. Bump next to ^16.3.3 (resolves 16.3.4) and raise the js-yaml override; sharp needs a new override because miniflare pins it exactly. Wrangler itself stays at 4.119.0 per the documented cf:typegen Buffer-collision hold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![corespeed-hopper[bot]](https://avatars.githubusercontent.com/in/4370362?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved at d8a92b8. This is the right change for #100: it adds a discoverable entry point over the existing Workspace-scoped hybrid retrieval instead of building a second retrieval path, so no authorization or ranking surface moves. Search still goes out as GET /api/memories?q with x-lore-workspace-id; RLS, scope, and server ranking are untouched.
Verified
Sidebaris rendered exactly once fromApp(App.tsx:421), soid="memory-search"stays unique, and its<form>is not nested inside any other form.searchCancelRefis dropped from every query-context reset inApp— theactiveWorkspaceIdeffect,openMemory,applyRoute,handleTabChange,drillType— andSidebarkeeps its own cancel-on-Workspace-change/unmount as a backstop. Child effects commit before parent effects, so the ref is assigned beforeApp's mount effect first calls it.- CSS:
.sidebar { overflow-y: auto }is safe here — the aside has no absolutely positioned popovers (the Workspace picker is a native<select>), and the mobile drawer already carried the same rule..sidebar-search-submitreuses the existing secondary-control recipe rather than inventing a token. - Lockfile:
happy-dom@20.10.6resolves cleanly,nextis a 16.3.1→16.3.4 patch, and wrangler stayed at 4.119.0 — thecf:typegen/ globalBuffercollision documented in AGENTS.md is not reintroduced. - Docs updated in the same PR (DESIGN.md behavioral contract + AGENTS.md module note and the happy-dom test gotcha), per house rule.
- CI covers everything the PR description lists as locally unrun (
build,packages:smoke,bun audit, opennext build, wrangler dry-run), so that gap closes on thecheckjob rather than on review.
P2 — should fix, can be a follow-up
Sidebar.tsx:202:e.preventDefault()runs before the IME guards, so a composition-confirming keydown gets cancelled too. See the inline note; it is a one-line reorder that loses nothing, and it is exactly the class of bug the happy-dom suite cannot detect because it fabricatesisComposing/keyCode.
P3 — notes, not blocking
use-debounced-callback.ts:17writes a ref during render.Sidebar.tsx:330: an aborted composition can silently drop a search that was already pending whencompositionstartcancelled it.- The button reads
Semantic searchunconditionally, but AGENTS.md defines a deployment with no working embedding provider as degraded to lexical retrieval (English FTS + the CJK substring channel). The label over-claims there. DESIGN.md is honest about it being hybrid; the UI string is the shorthand. Worth a thought, not a change I'd gate on. d8a92b8(Next/sharp/js-yaml) is unrelated to #100. Understandable —bun audit --audit-level=highgates CI — but it makes the PR two changes.
| if (debounce.current) clearTimeout(debounce.current); | ||
| onSearch(inputRef.current?.value ?? ""); | ||
| if (e.key !== "Enter") return; | ||
| e.preventDefault(); |
There was a problem hiding this comment.
P2 (non-blocking). preventDefault() fires before the composition guards below, so the keydown the IME is using to confirm a candidate is also cancelled. Browsers are widely believed to ignore preventDefault() on a keydown they dispatched with keyCode 229, and in Chrome/Firefox that keydown often reports key: "Process" and returns on the line above anyway — so this is probably inert. But it is unverifiable from synthetic events (the suite supplies isComposing/keyCode by hand), and the PR states live browser IME behaviour was not exercised.
The reorder costs nothing: handleSubmit already returns on composing.current, and browsers do not perform implicit submission for an Enter the IME consumed, so cancelling the composing keydown buys no protection you don't already have. Only the Safari path — compositionend then a bare keyCode 229 keydown, where composing.current is already false — actually needs the cancel, and that Enter is not composing.
Suggest running the guard first and calling preventDefault() only on the paths that reach requestSubmit() or the 229 short-circuit.
| const latest = useRef(callback); | ||
| // Assigned during render so a timer firing before the next effect flush still | ||
| // sees the current closure. | ||
| latest.current = callback; |
There was a problem hiding this comment.
P3. Writing a ref during render is the well-worn "latest callback" pattern, but React explicitly asks you not to: a render that is interrupted or discarded still lands its closure in latest.current. Harmless in practice here — onSearch is handleSearch, and the next committed render overwrites it — so this is a note, not a change request. The comment already explains the motivation (a timer firing before the effect flush), which is the reason not to move it into useEffect naively.
| compositionEndedAt.current = Date.now(); | ||
| // A cancelled composition restores the pre-composition text; | ||
| // nothing new to search then. | ||
| if (e.currentTarget.value !== compositionStartValue.current) { |
There was a problem hiding this comment.
P3. compositionstart cancels whatever was pending, and this guard then declines to reschedule when the composition ends with the value unchanged. An aborted composition (Escape) restores the pre-composition text, so a search the user had already committed to by typing — e.g. "hello", then a composition started and escaped inside the 220 ms window — is silently dropped, leaving stale results until the next keystroke or Enter. Narrow, and recoverable by the user. Rescheduling the restored value instead of skipping would close it.
2 participants
The standalone frontend now exposes a labelled Semantic search button and a natural-language hint, using the existing Workspace-scoped hybrid retrieval endpoint and server ranking. Typing retains the 220 ms debounce; Enter and button submission cancel pending timers, respect CJK IME composition (including Safari's compositionend-before-Enter sequence), and close the mobile drawer.
Review fixes hardened the interaction model:
src/lib/use-debounced-callback.ts); App drops the pending search throughsearchCancelRefon every query-context reset — Workspace change, tab change, type drill-down, route navigation (popstate), and opening a Memory — so a stale query can no longer fire after navigation.isComposinginput flag; an unchanged composition no longer reschedules a search.--hairline-softand the focus ring uses the contract's two-pixel--linkring; the desktop sidebar scrolls on short viewports.bun audit --audit-level=highCI gate (advisories published after this branch's last green run): Next.js → 16.3.4, sharp override → ^0.35.4, js-yaml override → ^4.3.2. Wrangler stays at 4.119.0 per the documentedcf:typegenBuffer-collision hold.Adds Sidebar interaction coverage (17 tests, happy-dom) and an API regression test for natural-language queries, and updates DESIGN.md and AGENTS.md.
Closes #100
Validation
Passed on d8a92b8: typecheck, full-repo lint, design:check,
bun run test(614 vitest + 25 Python SDK),bun run build(incl. sdk:check),bun audit --audit-level=highclean, and full CI (bothcheckjobs + python-sdk matrix). Live IME/browser interaction against a configured deployment remains unverified; the reviewer's P2 (preventDefault ordering ahead of the IME guards) is noted for a follow-up.