feat: phase 15.1 client-side encrypted search #198
Merged
Conversation
Phase 15.1: Client-Side Search - Implementation decisions documented - Phase boundary established - Design mockups added to .pen file - Search Command Palette component spec added to DESIGN.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 56531c058705
Phase 15.1: Client-Side Search - 2 plans in 2 waves - Wave 1: search index service (minisearch + encrypted IndexedDB) - Wave 2: search UI (command palette) + app integration - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ba5461ccc093
- Add minisearch 7.2.0 as production dependency for web app - ~8KB TypeScript-native fuzzy search library, zero dependencies Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- MiniSearch-based fuzzy search over file/folder names with prefix matching - HKDF-derived AES-256-GCM key (info: cipherbox-search-index-v1) for zero-knowledge encrypted index storage in IndexedDB (cipherbox-search database) - buildFromFolderTree() walks FolderNode tree, indexes children with parent paths - search() returns scored results with match info for highlight support - persistEncrypted()/loadEncrypted() for cross-session index persistence - clear() wipes in-memory index and IndexedDB entry on logout - Singleton instance exported from services barrel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 - Install minisearch dependency - Create SearchIndexService with encrypted IndexedDB persistence SUMMARY: .planning/phases/15.1-client-side-search/15.1-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- useSearch: open/close, index build/load, 150ms debounce - SearchPalette: keyboard nav, click-outside dismiss - CSS: 600px palette, green border, terminal aesthetic - Module-level triggerSearchIndexRebuild callback - Barrel export from hooks/index.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Mount SearchPalette in AppShell with result navigation - Add search button with Cmd+K hint to AppHeader - Trigger search index rebuild after IPNS sync in FileBrowser - Auto-clear search index on logout via auth state watcher - Add header-search-btn styles to layout.css Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 3/3 - Create useSearch hook and SearchPalette component - Wire search into AppShell, AppHeader, FileBrowser sync, and logout - Human verification of complete search feature (approved) Phase 15.1 (Client-Side Search) is now COMPLETE. SUMMARY: .planning/phases/15.1-client-side-search/15.1-02-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: abf471173f1a
Phase 15.1 verification found 9/10 truths verified, missing only E2E test coverage. Plan 03 adds SearchPalettePage page object and search-workflow.spec.ts covering open/close, query/results, navigation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0a40968c20c5
- SearchPalettePage with locator and action methods for search palette DOM - Barrel export in dialogs/index.ts and page-objects/index.ts - Compiles cleanly with tsc --noEmit Entire-Checkpoint: ee84f9e00d91
- 10 serial tests covering full search workflow - Open palette via Cmd+K and header button - Query with results and no-results states - Click and keyboard navigation to select results - Escape and click-outside dismissal - Setup with unique test file upload, cleanup on teardown Entire-Checkpoint: 9f94dd1f5759
Tasks completed: 2/2 - Create SearchPalettePage page object with barrel exports - Create search-workflow.spec.ts E2E test suite SUMMARY: .planning/phases/15.1-client-side-search/15.1-03-SUMMARY.md Entire-Checkpoint: 2518b02bda3d
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 7455d2fa8f68
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Phase 15.1 client-side search: a MiniSearch-based SearchIndexService with HKDF/AES‑GCM encrypted IndexedDB persistence, a useSearch hook with cross-component rebuild callbacks, a keyboard-driven SearchPalette UI wired into AppHeader/AppShell, E2E Playwright tests, styles, and planning/state updates marking SRCH items complete. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant AppShell as AppShell
participant Hook as useSearch
participant Service as SearchIndexService
participant DB as IndexedDB
participant FileBrowser as FileBrowser
participant Palette as SearchPalette
User->>AppShell: press Cmd/Ctrl+K or click header search
AppShell->>Hook: open()
Hook->>Service: loadEncrypted(userPrivateKey)
Service->>DB: get encrypted blob
DB-->>Service: encrypted blob / null
alt index not found
Hook->>Service: buildFromFolderTree(folders)
Service-->>Hook: index ready
Hook->>Service: persistEncrypted(userPrivateKey)
Service->>DB: store encrypted index
end
Hook-->>AppShell: isIndexReady, results
AppShell->>Palette: render(open=true)
User->>Palette: type query
Palette->>Hook: setQuery(query)
Hook->>Service: search(query) (debounced)
Service-->>Hook: results[]
Hook-->>Palette: results
User->>Palette: select result
Palette->>AppShell: onSelectResult(result)
AppShell->>AppShell: navigate to result.parentFolderId or /files
alt after successful sync
FileBrowser->>Hook: triggerSearchIndexRebuild()
Hook->>Service: buildFromFolderTree(folders)
Hook->>Service: persistEncrypted(userPrivateKey)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
apps/web/src/components/layout/AppShell.tsx (2)
35-45:useCallbackdependency on entiresearchobject; narrow tosearch.close.
searchis a new object reference on every render, sohandleSelectResultis recreated whenever any search state changes (query, results, isBuilding, …), even though onlysearch.closeis consumed. Depend on the specific stable function reference instead.♻️ Proposed refactor
+ const { close: closeSearch } = search; const handleSelectResult = useCallback( (result: SearchResult) => { - search.close(); + closeSearch(); if (result.parentFolderId === 'root') { navigate('/files'); } else { navigate(`/files/${result.parentFolderId}`); } }, - [search, navigate] + [closeSearch, navigate] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/AppShell.tsx` around lines 35 - 45, handleSelectResult is being re-created on every render because useCallback depends on the whole search object; change the dependency to the stable function reference (search.close) instead. Update the callback to reference search.close directly (or destructure const { close } = search and call close) and replace search in the dependency array with the specific close function and navigate, so useCallback depends on [search.close, navigate] (or [close, navigate]) to avoid unnecessary recreations; keep the existing navigation logic that inspects result.parentFolderId and calls navigate('/files') or navigate(`/files/${result.parentFolderId}`).
61-71:SearchPaletteJSX duplicated across staging and non-staging branches.The same 11-line
SearchPaletteblock appears verbatim in both render paths. Extract it to a variable before the branches.♻️ Proposed refactor
+ const searchPalette = ( + <SearchPalette + isOpen={search.isOpen} + query={search.query} + onQueryChange={search.setQuery} + results={search.results} + resultCount={search.resultCount} + isBuilding={search.isBuilding} + isIndexReady={search.isIndexReady} + onClose={search.close} + onSelectResult={handleSelectResult} + /> + ); if (isStaging) { return ( ... <DeviceApprovalModal /> - <SearchPalette - isOpen={search.isOpen} - ... - /> + {searchPalette} ... ); } return ( ... <DeviceApprovalModal /> - <SearchPalette - isOpen={search.isOpen} - ... - /> + {searchPalette} );Also applies to: 86-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/AppShell.tsx` around lines 61 - 71, The SearchPalette JSX block is duplicated across the two render branches; extract it to a single constant (e.g., const searchPalette = <SearchPalette ... />) placed before the conditional render and reuse that variable in both branches to avoid duplication. Ensure you include the same props currently passed (search.isOpen, search.query, search.setQuery, search.results, search.resultCount, search.isBuilding, search.isIndexReady, search.close, onSelectResult={handleSelectResult}) and keep the handleSelectResult and search object references intact when replacing the inline blocks in both render paths.apps/web/src/styles/search-palette.css (1)
89-93:.search-result-itemitems are keyboard-navigable but not DOM-focusable —:focus-visiblestyles not needed for current design.The items lack
tabIndexand receive keyboard navigation through the parent'sonKeyDownhandler (arrow keys). The.selectedclass already provides visual feedback for keyboard selection. However, if items should be individually focusable via Tab, they would needtabIndex={0}, keyboard event handlers on individual items, and corresponding:focus-visiblestyles for accessibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/styles/search-palette.css` around lines 89 - 93, The CSS currently styles .search-result-item and .search-result-item.selected for hover/selected states but the items are not DOM-focusable (they rely on the parent's onKeyDown for arrow navigation), so add guidance: either keep as-is and avoid adding :focus-visible rules, or make items individually focusable by adding tabIndex={0} to the element rendering .search-result-item, move or add keyboard handlers to the item (instead of only the parent onKeyDown), and add a :focus-visible rule (mirroring .selected) in search-palette.css so keyboard-tab focus shows the same visual state; update the component that renders .search-result-item accordingly (add tabIndex and per-item key handlers) if you choose the tabbable approach.apps/web/src/components/file-browser/SearchPalette.tsx (2)
157-185: Expose the active option to assistive tech.
The listbox doesn’t announce the active option because items lack stable IDs and the listbox lacksaria-activedescendant.As per coding guidelines: `apps/web/**`: Focus on accessibility (a11y) concerns.♿ Suggested a11y enhancement
function SearchResultItem({ result, isSelected, onClick, onMouseEnter, + id, }: { result: SearchResult; isSelected: boolean; onClick: () => void; onMouseEnter: () => void; + id?: string; }) { const icon = getFileIcon(result.name, result.type); const dateStr = formatRelativeDate(result.modifiedAt); const highlightedName = highlightMatches(result.name, result.match); return ( <div + id={id} className={`search-result-item ${isSelected ? 'selected' : ''}`} onClick={onClick} onMouseEnter={onMouseEnter} role="option" aria-selected={isSelected} >export function SearchPalette({ isOpen, query, onQueryChange, results, @@ const resultsRef = useRef<HTMLDivElement>(null); const backdropRef = useRef<HTMLDivElement>(null); + const activeId = results[selectedIndex] ? `search-result-${selectedIndex}` : undefined; @@ - <div className="search-results" ref={resultsRef} role="listbox" aria-label="Search results"> + <div + className="search-results" + ref={resultsRef} + role="listbox" + aria-label="Search results" + aria-activedescendant={activeId} + > @@ {results.map((result, index) => ( <SearchResultItem key={result.id} + id={`search-result-${index}`} result={result} isSelected={index === selectedIndex} onClick={() => onSelectResult(result)} onMouseEnter={() => setSelectedIndex(index)} /> ))}Also applies to: 225-233, 303-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SearchPalette.tsx` around lines 157 - 185, SearchResultItem currently renders options without stable IDs so the parent listbox cannot use aria-activedescendant; add a stable id attribute to each item (e.g. `const itemId = \`search-result-${result.id || slugify(result.parentPath + '/' + result.name)}\``) and set it on the root div (`id={itemId}`) while keeping role="option" and aria-selected; then ensure the listbox component that renders SearchResultItem uses aria-activedescendant pointing to the selected itemId (update the selection management to expose the active itemId). Update references in SearchResultItem and the parent listbox to use `itemId` for accessibility.
246-256: Avoid negative selection index when the list is empty.
Arrow navigation can setselectedIndexto-1when there are zero results; clamping keeps state consistent.♻️ Suggested tweak
switch (e.key) { case 'ArrowDown': e.preventDefault(); - setSelectedIndex((i) => Math.min(i + 1, results.length - 1)); + if (results.length === 0) return; + setSelectedIndex((i) => Math.min(i + 1, results.length - 1)); break; case 'ArrowUp': e.preventDefault(); - setSelectedIndex((i) => Math.max(i - 1, 0)); + if (results.length === 0) return; + setSelectedIndex((i) => Math.max(i - 1, 0)); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SearchPalette.tsx` around lines 246 - 256, The ArrowUp/ArrowDown logic in SearchPalette.handleKeyDown can set selectedIndex to -1 when results.length === 0; add a guard at the top of handleKeyDown (e.g., if (results.length === 0) return;) so keyboard nav does nothing for an empty list, then keep the existing setSelectedIndex calls (Math.min(i + 1, results.length - 1) and Math.max(i - 1, 0)) to clamp within valid bounds when results exist.tests/e2e/page-objects/dialogs/search-palette.page.ts (1)
142-151:getResultNames()can be replaced withallTextContents().Playwright's built-in returns all text contents in one call without the manual loop.
♻️ Proposed simplification
async getResultNames(): Promise<string[]> { - const items = this.page.locator('.search-result-item .search-result-name'); - const count = await items.count(); - const names: string[] = []; - for (let i = 0; i < count; i++) { - const text = await items.nth(i).textContent(); - if (text) names.push(text); - } - return names; + return this.page + .locator('.search-result-item .search-result-name') + .allTextContents(); }Note:
allTextContents()returns''for elements with no text rather than omitting them, matching the existingif (text)guard's behavior for non-empty results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/dialogs/search-palette.page.ts` around lines 142 - 151, The getResultNames() function manually loops over locator items to collect text content; replace that loop with Playwright's locator.allTextContents() to return all texts in one call. Update the implementation in getResultNames() to call this.page.locator('.search-result-item .search-result-name').allTextContents() and return the resulting string[] (preserving the current behavior that empty elements yield '' entries).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/layout/AppHeader.tsx`:
- Around line 23-24: The aria-label and title currently hard-code "Cmd+K" which
is incorrect on Windows/Linux; update the AppHeader component (where the search
button attributes aria-label and title are set) to compute the displayed
shortcut string dynamically (e.g., detect platform via navigator.platform/
userAgent or reuse any existing helper from useSearch) and use that value in
both aria-label and title so they show "Cmd+K" on macOS and "Ctrl+K" on
Windows/Linux; ensure the logic is reused for both attributes so screen readers
and the tooltip remain consistent.
In `@apps/web/src/services/search-index.service.ts`:
- Around line 153-161: The current dedup logic builds deduped by iterating
documents and always overwriting by id, which causes later "folder-self" entries
(with modifiedAt = 0) to replace richer child entries; change the loop in
search-index.service.ts so the first document for a given id is kept and
subsequent entries are ignored except that if a later entry is a folder-self and
the id is missing you may add it—i.e., when iterating documents check
deduped.has(doc.id) and only set if absent (or selectively merge only missing
fields), then call this.miniSearch.addAll(Array.from(deduped.values())) so
folder metadata from the original child documents (e.g. modifiedAt) is
preserved.
In `@apps/web/src/styles/layout.css`:
- Line 104: The CSS value keyword casing violates Stylelint: update the border
declaration that currently reads 'border: 1px solid currentColor;' to use
lowercase keyword 'currentcolor' so the rule value-keyword-case passes; locate
the border property in apps/web/src/styles/layout.css (the line with "border:
1px solid currentColor;") and change only the keyword casing to "currentcolor".
In `@apps/web/src/styles/search-palette.css`:
- Around line 61-63: The rule .search-input:focus-visible currently suppresses
the native focus ring with outline: none; change it to provide an accessible
custom indicator instead: remove outline: none and add a visible but subtle
focus style (for example a 2px outline or a soft box-shadow using your theme
variables such as --focus-color and appropriate border-radius) so keyboard users
retain a clear focus state; update the .search-input:focus-visible selector to
use that custom outline/box-shadow and ensure sufficient contrast and no layout
shift.
In `@tests/e2e/page-objects/dialogs/search-palette.page.ts`:
- Around line 54-56: The selector in resultItem(index: number) is using
:nth-child(...) which is sibling-order sensitive; change it to use the existing
resultItems() accessor and return its nth(index) so the locator is scoped only
to .search-result-item matches (i.e., update the resultItem function to call
resultItems().nth(index) instead of
page.locator(`.search-result-item:nth-child(${index + 1})`)).
---
Nitpick comments:
In `@apps/web/src/components/file-browser/SearchPalette.tsx`:
- Around line 157-185: SearchResultItem currently renders options without stable
IDs so the parent listbox cannot use aria-activedescendant; add a stable id
attribute to each item (e.g. `const itemId = \`search-result-${result.id ||
slugify(result.parentPath + '/' + result.name)}\``) and set it on the root div
(`id={itemId}`) while keeping role="option" and aria-selected; then ensure the
listbox component that renders SearchResultItem uses aria-activedescendant
pointing to the selected itemId (update the selection management to expose the
active itemId). Update references in SearchResultItem and the parent listbox to
use `itemId` for accessibility.
- Around line 246-256: The ArrowUp/ArrowDown logic in
SearchPalette.handleKeyDown can set selectedIndex to -1 when results.length ===
0; add a guard at the top of handleKeyDown (e.g., if (results.length === 0)
return;) so keyboard nav does nothing for an empty list, then keep the existing
setSelectedIndex calls (Math.min(i + 1, results.length - 1) and Math.max(i - 1,
0)) to clamp within valid bounds when results exist.
In `@apps/web/src/components/layout/AppShell.tsx`:
- Around line 35-45: handleSelectResult is being re-created on every render
because useCallback depends on the whole search object; change the dependency to
the stable function reference (search.close) instead. Update the callback to
reference search.close directly (or destructure const { close } = search and
call close) and replace search in the dependency array with the specific close
function and navigate, so useCallback depends on [search.close, navigate] (or
[close, navigate]) to avoid unnecessary recreations; keep the existing
navigation logic that inspects result.parentFolderId and calls
navigate('/files') or navigate(`/files/${result.parentFolderId}`).
- Around line 61-71: The SearchPalette JSX block is duplicated across the two
render branches; extract it to a single constant (e.g., const searchPalette =
<SearchPalette ... />) placed before the conditional render and reuse that
variable in both branches to avoid duplication. Ensure you include the same
props currently passed (search.isOpen, search.query, search.setQuery,
search.results, search.resultCount, search.isBuilding, search.isIndexReady,
search.close, onSelectResult={handleSelectResult}) and keep the
handleSelectResult and search object references intact when replacing the inline
blocks in both render paths.
In `@apps/web/src/styles/search-palette.css`:
- Around line 89-93: The CSS currently styles .search-result-item and
.search-result-item.selected for hover/selected states but the items are not
DOM-focusable (they rely on the parent's onKeyDown for arrow navigation), so add
guidance: either keep as-is and avoid adding :focus-visible rules, or make items
individually focusable by adding tabIndex={0} to the element rendering
.search-result-item, move or add keyboard handlers to the item (instead of only
the parent onKeyDown), and add a :focus-visible rule (mirroring .selected) in
search-palette.css so keyboard-tab focus shows the same visual state; update the
component that renders .search-result-item accordingly (add tabIndex and
per-item key handlers) if you choose the tabbable approach.
In `@tests/e2e/page-objects/dialogs/search-palette.page.ts`:
- Around line 142-151: The getResultNames() function manually loops over locator
items to collect text content; replace that loop with Playwright's
locator.allTextContents() to return all texts in one call. Update the
implementation in getResultNames() to call
this.page.locator('.search-result-item .search-result-name').allTextContents()
and return the resulting string[] (preserving the current behavior that empty
elements yield '' entries).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/15.1-client-side-search/15.1-01-PLAN.md.planning/phases/15.1-client-side-search/15.1-01-SUMMARY.md.planning/phases/15.1-client-side-search/15.1-02-PLAN.md.planning/phases/15.1-client-side-search/15.1-02-SUMMARY.md.planning/phases/15.1-client-side-search/15.1-03-PLAN.md.planning/phases/15.1-client-side-search/15.1-03-SUMMARY.md.planning/phases/15.1-client-side-search/15.1-CONTEXT.md.planning/phases/15.1-client-side-search/15.1-VERIFICATION.mdapps/web/package.jsonapps/web/src/components/file-browser/FileBrowser.tsxapps/web/src/components/file-browser/SearchPalette.tsxapps/web/src/components/layout/AppHeader.tsxapps/web/src/components/layout/AppShell.tsxapps/web/src/hooks/index.tsapps/web/src/hooks/useSearch.tsapps/web/src/services/index.tsapps/web/src/services/search-index.service.tsapps/web/src/styles/layout.cssapps/web/src/styles/search-palette.cssdesigns/DESIGN.mdtests/e2e/page-objects/dialogs/index.tstests/e2e/page-objects/dialogs/search-palette.page.tstests/e2e/page-objects/index.tstests/e2e/tests/search-workflow.spec.ts
Align implementation with updated design spec: - Input row padding 14px 20px, gap 12px - Result item padding 10px 20px, gap 14px - Selected result: full accent border (2px left/right, 1px top/bottom) - Result name weight normal (semibold on selected only) - Result path color uses --color-text-secondary (#006644) - Footer/hint font sizes 9px Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f7e54a2e0eec
Remove Option A compact draft and selection labels. Rebuild Option B as canonical "Search Command Palette" screen with updated selected result border (2px left/right + 1px top/bottom accent). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6d451443f6e4
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 656495b0b2e3
FSM1
changed the title
- Detect platform for shortcut label (⌘K on Mac, Ctrl+K otherwise) - Preserve child doc metadata in search index dedup (keep first entry) - Fix CSS keyword casing (currentcolor) and add focus-visible indicator - Use .nth() instead of :nth-child() in E2E page object - Wait for index build completion before searching in CI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8d47be7cc413
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/web/src/components/layout/AppHeader.tsx (2)
22-32: LGTM — prior platform-shortcut concern is resolved.The dynamic
shortcutLabelnow correctly drivesaria-label,title, and the<kbd>display for both macOS and Windows/Linux. The native<button>element satisfies the keyboard interaction contract without an explicitonKeyDown, and{'>_'}is correctly braced per the BiomenoCommentTextrule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/AppHeader.tsx` around lines 22 - 32, The AppHeader button rendering using onSearchClick and shortcutLabel is correct and needs no change; leave the conditional JSX block in AppHeader.tsx (the <button> that uses onSearchClick, aria-label/title with {`Search files (${shortcutLabel})`} and {`Search (${shortcutLabel})`} and displays {'>_'} <kbd>{shortcutLabel}</kbd>) as-is since it satisfies accessibility and keyboard interaction requirements.
22-32: LGTM — prior platform-shortcut concern is resolved.The dynamic
shortcutLabelnow drivesaria-label,title, and the<kbd>display correctly for both macOS and Windows/Linux. The native<button>element satisfies the keyboard interaction contract without an explicitonKeyDown, and{'>_'}is correctly braced per the BiomenoCommentTextrule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/AppHeader.tsx` around lines 22 - 32, The review confirms the implementation in AppHeader.tsx is correct: keep the conditional render using onSearchClick, the native <button> with onClick (no onKeyDown needed), and the dynamic shortcutLabel used in aria-label, title and the <kbd> element; ensure symbols referenced (onSearchClick, shortcutLabel, header-search-btn, and the button JSX) remain unchanged so macOS and Windows/Linux shortcut labels and keyboard behavior stay correct.
🧹 Nitpick comments (4)
apps/web/src/components/layout/AppHeader.tsx (2)
7-8:navigator.platformis deprecated — consider auserAgentDatafallback.
navigator.platformshould almost always be avoided, but MDN itself acknowledges that detecting the Mac modifier key for keyboard shortcuts is the one case where it "may be the least-bad option." Since this is exactly that use-case the current code is fine in practice. For forward-compatibility, you could add auserAgentDataprimary withnavigator.platformas fallback, asnavigator.userAgentDatais not implemented in all browsers:♻️ Optional forward-compatible refactor
-const isMac = typeof navigator !== 'undefined' && /Mac/i.test(navigator.platform); +const isMac = + typeof navigator !== 'undefined' && + (/Mac/i.test((navigator as Navigator & { userAgentData?: { platform: string } }).userAgentData?.platform ?? '') || + /Mac/i.test(navigator.platform));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/AppHeader.tsx` around lines 7 - 8, The current detection uses navigator.platform which is deprecated; update isMac to first check navigator.userAgentData?.platform (when navigator is defined) and fall back to navigator.platform, e.g. compute a platform string from navigator.userAgentData.platform || navigator.platform, then set isMac = typeof navigator !== 'undefined' && /Mac/i.test(platformString); update references to isMac and shortcutLabel accordingly so browsers with User Agent Client Hints use userAgentData while older browsers still fall back to navigator.platform.
7-8:navigator.platformis deprecated — consider addingnavigator.userAgentData?.platformwith fallback for compatibility.
navigator.platformis marked deprecated by MDN and browsers may stop returning meaningful values. However, the modern replacement (navigator.userAgentData?.platform) is currently supported only in Chromium-based browsers (Chrome 93+, Edge, Opera); Firefox and Safari do not support it. A guarded check with fallback provides forward compatibility while maintaining cross-browser support:♻️ Proposed refactor
-const isMac = typeof navigator !== 'undefined' && /Mac/i.test(navigator.platform); +const isMac = + typeof navigator !== 'undefined' && + (/Mac/i.test(navigator.userAgentData?.platform ?? '') || + /Mac/i.test(navigator.platform));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/AppHeader.tsx` around lines 7 - 8, Update the platform detection used by isMac to use navigator.userAgentData?.platform with a safe fallback to navigator.platform and keep the typeof navigator !== 'undefined' guard for SSR; specifically, change the isMac calculation (used to derive shortcutLabel) to first read navigator.userAgentData?.platform, then fall back to navigator.platform (and then to an empty string) before running /Mac/i.test, so that Chromium-based userAgentData is preferred but other browsers and server-side rendering remain supported.tests/e2e/tests/search-workflow.spec.ts (2)
60-71: Guard against missing test credentials early.
Line 62 usesTEST_CREDENTIALS.email; if it’s empty, the failure happens later and is harder to diagnose. Add an explicit precondition to fail fast.🛠️ Proposed fix
test('0.1 Login and create test content', async () => { + expect(TEST_CREDENTIALS.email, 'WEB3AUTH_TEST_EMAIL must be set').not.toBe(''); // Login await loginViaEmail(page, TEST_CREDENTIALS.email);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/search-workflow.spec.ts` around lines 60 - 71, Add a fast precondition check before calling loginViaEmail in the "0.1 Login and create test content" test: verify TEST_CREDENTIALS.email is present and non-empty (and optionally TEST_CREDENTIALS.password) and throw/assert with a clear message if missing so the test fails early and clearly; place this check immediately before the loginViaEmail(page, TEST_CREDENTIALS.email) call to guard the test from misleading downstream failures.
243-266: Align delete selector with shared context-menu locator.
The.context-menu-itemselector diverges from the shared context menu locator (button[role="menuitem"]), which can lead to selector drift.🛠️ Proposed fix
- const deleteOption = page.locator('.context-menu-item', { hasText: 'Delete' }); + const deleteOption = page.locator('button[role="menuitem"]', { hasText: 'Delete' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/search-workflow.spec.ts` around lines 243 - 266, In the "9.1 Cleanup" test replace the divergent context-menu selector: when locating the Delete option (currently assigned to deleteOption), use the shared context-menu locator button[role="menuitem"] with the hasText('Delete') predicate instead of '.context-menu-item' so the test uses the common menu selector; keep the same click/confirm flow (confirmBtn and fileList.waitForItemToDisappear) and ensure the rightClickItem(fileList, testFileName) + the new locator are used to find and click the Delete menu entry before calling cleanupTestFiles().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/tests/search-workflow.spec.ts`:
- Around line 139-160: Replace the fixed sleep after typing the query with a
UI-driven wait: after calling searchPalette.typeQuery('zzzzxqwnofile999'), wait
for the no-results indicator to appear using the searchPalette.noResults()
locator (or equivalent waiter) instead of page.waitForTimeout(500); then assert
visibility and result count as before, keeping the existing open/close logic
(searchPalette.waitForOpen, searchPalette.getResultCount,
searchPalette.waitForClosed) unchanged.
- Around line 77-101: The shortcut tests use a hardcoded 'Meta+k' which fails on
Windows/Linux CI; update each test that calls page.keyboard.press('Meta+k')
(tests '1.1 Open search palette via Cmd+K', '1.2', '2.1', '2.2', '3.1', '3.2',
'4.1', '4.2') to choose the modifier at runtime: detect the platform (e.g., via
process.platform or Playwright's context/browser userAgent) and call
page.keyboard.press('Meta+k') when on macOS and page.keyboard.press('Control+k')
on other platforms so searchPalette.waitForOpen and searchPalette.input()
assertions keep working cross-platform.
---
Duplicate comments:
In `@apps/web/src/components/layout/AppHeader.tsx`:
- Around line 22-32: The AppHeader button rendering using onSearchClick and
shortcutLabel is correct and needs no change; leave the conditional JSX block in
AppHeader.tsx (the <button> that uses onSearchClick, aria-label/title with
{`Search files (${shortcutLabel})`} and {`Search (${shortcutLabel})`} and
displays {'>_'} <kbd>{shortcutLabel}</kbd>) as-is since it satisfies
accessibility and keyboard interaction requirements.
- Around line 22-32: The review confirms the implementation in AppHeader.tsx is
correct: keep the conditional render using onSearchClick, the native <button>
with onClick (no onKeyDown needed), and the dynamic shortcutLabel used in
aria-label, title and the <kbd> element; ensure symbols referenced
(onSearchClick, shortcutLabel, header-search-btn, and the button JSX) remain
unchanged so macOS and Windows/Linux shortcut labels and keyboard behavior stay
correct.
---
Nitpick comments:
In `@apps/web/src/components/layout/AppHeader.tsx`:
- Around line 7-8: The current detection uses navigator.platform which is
deprecated; update isMac to first check navigator.userAgentData?.platform (when
navigator is defined) and fall back to navigator.platform, e.g. compute a
platform string from navigator.userAgentData.platform || navigator.platform,
then set isMac = typeof navigator !== 'undefined' &&
/Mac/i.test(platformString); update references to isMac and shortcutLabel
accordingly so browsers with User Agent Client Hints use userAgentData while
older browsers still fall back to navigator.platform.
- Around line 7-8: Update the platform detection used by isMac to use
navigator.userAgentData?.platform with a safe fallback to navigator.platform and
keep the typeof navigator !== 'undefined' guard for SSR; specifically, change
the isMac calculation (used to derive shortcutLabel) to first read
navigator.userAgentData?.platform, then fall back to navigator.platform (and
then to an empty string) before running /Mac/i.test, so that Chromium-based
userAgentData is preferred but other browsers and server-side rendering remain
supported.
In `@tests/e2e/tests/search-workflow.spec.ts`:
- Around line 60-71: Add a fast precondition check before calling loginViaEmail
in the "0.1 Login and create test content" test: verify TEST_CREDENTIALS.email
is present and non-empty (and optionally TEST_CREDENTIALS.password) and
throw/assert with a clear message if missing so the test fails early and
clearly; place this check immediately before the loginViaEmail(page,
TEST_CREDENTIALS.email) call to guard the test from misleading downstream
failures.
- Around line 243-266: In the "9.1 Cleanup" test replace the divergent
context-menu selector: when locating the Delete option (currently assigned to
deleteOption), use the shared context-menu locator button[role="menuitem"] with
the hasText('Delete') predicate instead of '.context-menu-item' so the test uses
the common menu selector; keep the same click/confirm flow (confirmBtn and
fileList.waitForItemToDisappear) and ensure the rightClickItem(fileList,
testFileName) + the new locator are used to find and click the Delete menu entry
before calling cleanupTestFiles().
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.coderabbit.yamlapps/web/src/components/layout/AppHeader.tsxapps/web/src/services/search-index.service.tsapps/web/src/styles/layout.cssapps/web/src/styles/search-palette.cssdesigns/DESIGN.mddesigns/cipher-box-design.pentests/e2e/page-objects/dialogs/search-palette.page.tstests/e2e/tests/search-workflow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/styles/layout.css
- apps/web/src/services/search-index.service.ts
- apps/web/src/styles/search-palette.css
- tests/e2e/page-objects/dialogs/search-palette.page.ts
- Replace racy waitForIndexReady (point-in-time isVisible check) with expect().toPass() retry loops that handle index build + debounce timing - Use pressSequentially instead of fill for reliable React state updates in headless CI environments - Add waitForSearchComplete for no-results test case - Increase timeouts to 30s for CI resilience Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: be1c0944f685
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/e2e/tests/search-workflow.spec.ts (1)
77-80:⚠️ Potential issue | 🟠 MajorUse a cross‑platform Cmd/Ctrl+K shortcut helper.
Hardcoding
Meta+kwill fail on Windows/Linux runners. Prefer a shared helper (or a single constant) that choosesMeta+kon macOS andControl+kelsewhere, then reuse it across all shortcut opens.🛠️ Suggested fix (apply to all occurrences)
// Test data const runId = Date.now(); const testFileName = `search-test-${runId}.txt`; +const openSearchShortcut = process.platform === 'darwin' ? 'Meta+k' : 'Control+k'; ... - await page.keyboard.press('Meta+k'); + await page.keyboard.press(openSearchShortcut);Playwright 1.48 keyboard.press cross-platform modifier: is "ControlOrMeta" supported and recommended for Cmd/Ctrl shortcuts?Also applies to: 107-110, 133-136, 160-163, 182-185, 206-209, 221-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/search-workflow.spec.ts` around lines 77 - 80, Tests hardcode 'Meta+k' in keyboard.press which fails on Windows/Linux; create a shared helper/constant (e.g., KEY_CMD_CTRL_K) that returns 'Meta+k' when process.platform === 'darwin' and 'Control+k' otherwise, replace all occurrences of 'Meta+k' in tests/e2e/tests/search-workflow.spec.ts (where keyboard.press is used) with that constant, and reuse it for every shortcut invocation (also update other shortcut occurrences listed) so searchPalette.waitForOpen and related steps work cross-platform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/tests/search-workflow.spec.ts`:
- Around line 77-80: Tests hardcode 'Meta+k' in keyboard.press which fails on
Windows/Linux; create a shared helper/constant (e.g., KEY_CMD_CTRL_K) that
returns 'Meta+k' when process.platform === 'darwin' and 'Control+k' otherwise,
replace all occurrences of 'Meta+k' in tests/e2e/tests/search-workflow.spec.ts
(where keyboard.press is used) with that constant, and reuse it for every
shortcut invocation (also update other shortcut occurrences listed) so
searchPalette.waitForOpen and related steps work cross-platform.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/page-objects/dialogs/search-palette.page.tstests/e2e/tests/search-workflow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/page-objects/dialogs/search-palette.page.ts
…n E2E search tests - Replace hardcoded Meta+k with ControlOrMeta+k (resolves to Ctrl+K on Linux CI, Cmd+K on macOS) - Switch from pressSequentially to fill() for React controlled inputs — atomic value set is more reliable than sequential keystrokes - Add toHaveValue assertion to fail fast if React rejects the fill Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 75b6a55bfcd2
Expose searchIndexService on window and log folder store + index state before search in test 2.1. This will reveal whether the index is empty or the folder store lacks data when the test runs in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f2a2580c4068
The search index was built prematurely by triggerSearchIndexRebuild() from the sync handler during initial navigation, before test files were uploaded. Since isIndexReady was set to true, the palette-open effect skipped rebuilding, leaving newly added files unindexed. Fix: rebuild from current folder store every time the palette opens, using a ref-based guard to prevent concurrent builds. The IndexedDB cold-start optimization is preserved (only attempted when the index is empty). Stale results from a prior sync-triggered build are shown immediately while the fresh rebuild happens in the background. Also cleans up diagnostic logging from the CI debugging session. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0c2580c51a5a
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/hooks/useSearch.ts (1)
10-18: Module-level singleton callback silently overwrites on concurrentuseSearchinstances
_rebuildCallbackholds only one registration at a time. IfuseSearchis mounted more than once simultaneously (e.g., React StrictMode double-invoke, HMR, or a future layout that rendersSearchPalettein multiple panels), the lastuseEffectto run overwrites all previous registrations without warning.triggerSearchIndexRebuild()would then only reach the last-registered consumer.Consider either:
- Enforcing a single-instance invariant with a
DEVwarning on double-registration, or- Switching to a
Set-based multi-subscriber pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useSearch.ts` around lines 10 - 18, Currently a single module-level _rebuildCallback is overwritten by concurrent useSearch instances; change this to a Set-based multi-subscriber pattern: replace _rebuildCallback with e.g. a Set<() => void>, update registerRebuildCallback(cb) to add cb to the set and return (or expose) an unregister function that removes cb, and change triggerSearchIndexRebuild() to iterate the set and call each callback; also ensure the useSearch/SearchPalette useEffect uses the returned unregister function (or calls register/unregister) on cleanup so multiple mounts don't clobber each other.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/hooks/useSearch.ts`:
- Around line 160-165: The cleanup in the useEffect registers a no-op instead of
clearing the handler, so change the cleanup to call
registerRebuildCallback(null) (or the equivalent that sets the internal
_rebuildCallback to null) instead of registerRebuildCallback(() => {}) so the
absence of a handler is explicit; update any related logic in
triggerSearchIndexRebuild or code that invokes _rebuildCallback to continue
using the optional call pattern (e.g., _rebuildCallback?.()) or to explicitly
check for null before invoking to avoid silent no-op behavior.
- Around line 95-128: The init async function can resume after awaiting
loadEncrypted and then wrongly write state or persist key material after
clearIndex/logout; add a cancellation flag (useRef like isCancelledRef) that
clearIndex sets to true (and reset when starting a new build), then inside init
re-check that flag and buildingRef after every await (notably after await
searchIndexService.loadEncrypted(...)) and bail out if cancelled before calling
setIsIndexReady or searchIndexService.persistEncrypted(vaultKeypair.privateKey);
ensure you also avoid doing persistEncrypted or other writes when isCancelledRef
is true and reference searchIndexService.clear() as the operation that flips the
cancellation.
---
Nitpick comments:
In `@apps/web/src/hooks/useSearch.ts`:
- Around line 10-18: Currently a single module-level _rebuildCallback is
overwritten by concurrent useSearch instances; change this to a Set-based
multi-subscriber pattern: replace _rebuildCallback with e.g. a Set<() => void>,
update registerRebuildCallback(cb) to add cb to the set and return (or expose)
an unregister function that removes cb, and change triggerSearchIndexRebuild()
to iterate the set and call each callback; also ensure the
useSearch/SearchPalette useEffect uses the returned unregister function (or
calls register/unregister) on cleanup so multiple mounts don't clobber each
other.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/useSearch.tstests/e2e/tests/search-workflow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/tests/search-workflow.spec.ts
Comment on lines
+160
to
+165
| useEffect(() => { | ||
| registerRebuildCallback(rebuildIndex); | ||
| return () => { | ||
| registerRebuildCallback(() => {}); | ||
| }; | ||
| }, [rebuildIndex]); |
There was a problem hiding this comment.
Cleanup sets _rebuildCallback to () => {} instead of null
On unmount, the effect resets _rebuildCallback to a no-op rather than clearing it to null. This means triggerSearchIndexRebuild() silently calls an empty function rather than clearly indicating no handler is registered — making it harder to detect orphaned calls in future callers.
♻️ Proposed fix
useEffect(() => {
registerRebuildCallback(rebuildIndex);
return () => {
- registerRebuildCallback(() => {});
+ _rebuildCallback = null;
};
}, [rebuildIndex]);And update triggerSearchIndexRebuild (or leave it as-is since ?.() already handles null):
export function triggerSearchIndexRebuild(): void {
_rebuildCallback?.();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/useSearch.ts` around lines 160 - 165, The cleanup in the
useEffect registers a no-op instead of clearing the handler, so change the
cleanup to call registerRebuildCallback(null) (or the equivalent that sets the
internal _rebuildCallback to null) instead of registerRebuildCallback(() => {})
so the absence of a handler is explicit; update any related logic in
triggerSearchIndexRebuild or code that invokes _rebuildCallback to continue
using the optional call pattern (e.g., _rebuildCallback?.()) or to explicitly
check for null before invoking to avoid silent no-op behavior.
FSM1
added a commit
that referenced
this pull request
Feb 24, 2026
When clearIndex fires (on logout) while init() is suspended at await loadEncrypted(), the resumed init would overwrite isIndexReady with true and persist encrypted key material to IndexedDB after the user has logged out. Fix: add cancelledRef that clearIndex sets to true. init() checks this flag after each await and bails out before writing state or persisting. clearIndex also resets buildingRef to prevent deadlock if logout interrupts mid-build. Addresses: #198 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: be0b498729fa
3 tasks
FSM1
added a commit
that referenced
this pull request
Feb 24, 2026
* fix(15.1): prevent logout race in search index init When clearIndex fires (on logout) while init() is suspended at await loadEncrypted(), the resumed init would overwrite isIndexReady with true and persist encrypted key material to IndexedDB after the user has logged out. Fix: add cancelledRef that clearIndex sets to true. init() checks this flag after each await and bails out before writing state or persisting. clearIndex also resets buildingRef to prevent deadlock if logout interrupts mid-build. Addresses: #198 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: be0b498729fa * docs: capture todo and learning from search debug session - Todo: async/incremental search index build for large vaults - Learning: run E2E locally before push Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6f54eaf3475f --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Closed
4 tasks
Merged
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
1 participant
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
Test plan
pnpm buildpasses (verified by executor)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Design/Docs