feat(p3): implement scoped, field-specific, and fuzzy search#15
feat(p3): implement scoped, field-specific, and fuzzy search#15ryota-murakami merged 4 commits intomainfrom
Conversation
Deliver Issue #3 search UX end-to-end with persisted search state, API/query integration, Fuse-powered collection matching, and E2E coverage so search behavior is consistent and verifiable across views.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughクライアント／UI、Redux、検索ユーティリティ、コレクションファジー検索、E2E モックとテストにまたがる新規検索機能（スコープ、フィールド指定、Fuse.js ファジー、ハイライト、API クエリ変換）を追加。検索は API モックで事前フィルタ→ソート→ページングされる。 Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as MainContent / GlobalSearch
participant Redux as Redux Store
participant API as GET /raindrops (E2E mock)
participant Filter as filterByScope()
participant Highlight as buildSubstringHighlightSegments()
participant Display as RaindropCard/ListItem
User->>UI: 入力 (query, scope, mode)
UI->>Redux: dispatch setSearchQuery / setSearchScope / setSearchMode
Redux-->>UI: updated state
UI->>API: GET /raindrops/{collectionId}?search=buildSearchQuery(...)
API->>API: applySearchFilter(items, rawSearch)
API-->>UI: filtered + paged raindrops
UI->>Filter: filterByScope(items, query, scope)
Filter-->>UI: scope-filtered items
UI->>Highlight: buildSubstringHighlightSegments(text, query)
Highlight-->>Display: HighlightSegment[]
Display->>User: ハイライト付き検索結果表示
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/raindrop/collection-search.tsx (1)
209-215:⚠️ Potential issue | 🟡 Minor閉じるボタンに
aria-labelがない
Xアイコンのみのボタンにアクセシブルなラベルがない。スクリーンリーダーがボタンの目的を認識できない。修正案
<button type="button" onClick={onClose} + aria-label="Close search" className="hover:bg-accent absolute top-1/2 right-1.5 -translate-y-1/2 rounded p-0.5" >As per coding guidelines, 「Accessibility (WCAG 2.2 AA+, Apple HIG, 44×44px tap targets)」。
🤖 Fix all issues with AI agents
In `@src/components/raindrop/raindrop-card.tsx`:
- Line 193: The code computes domainText via "const domainText = raindrop.domain
|| new URL(raindrop.url).hostname" which throws if raindrop.url is an invalid
URL; update the logic in raindrop-card.tsx to safely parse the hostname (same
fix as raindrop-list-item.tsx) by attempting to construct a URL inside a
try/catch or using an existing safeParseUrl helper, and fall back to
raindrop.url or an empty string when parsing fails so rendering never throws.
In `@src/components/raindrop/raindrop-list-item.tsx`:
- Line 186: The line computing domainText can throw when raindrop.url is empty
or invalid; update the logic in raindrop-list-item.tsx (the domainText
assignment that uses raindrop.domain and new URL(raindrop.url).hostname) to
safely parse the URL: check that raindrop.url is a non-empty string and wrap new
URL(...) in a try/catch (or validate with a regex/URL constructor test) and fall
back to raindrop.url or an empty string when parsing fails; ensure you still
prefer raindrop.domain when truthy and only attempt hostname extraction when
parsing succeeds.
🧹 Nitpick comments (12)
src/lib/search.ts (1)
267-297: Fuseインスタンスが毎回再生成される点
fuzzySearchByNameは呼び出しのたびにnew Fuse(items, ...)を生成する。呼び出し元がuseMemoで適切にメモ化しているため、コレクション数が妥当な範囲（E2Eで100件 <10ms を検証済み）では問題ない。大規模データセットで頻繁に呼ばれる場合は、Fuseインスタンスのキャッシュを検討してもよい。
src/lib/search.test.ts (1)
105-131: ハイライト関連テストのエッジケースカバレッジが薄い
buildSubstringHighlightSegmentsとbuildIndexedHighlightSegmentsのテストが各1ケースのみ。以下のエッジケースの追加を推奨:
- 空テキスト / 空クエリ
- マッチなし
- 複数マッチ（
buildSubstringHighlightSegmentsで同じ語が複数回出現）- 重複・隣接インデックス（
buildIndexedHighlightSegmentsのマージロジック）追加テスト例
describe('buildSubstringHighlightSegments', () => { it('builds matched and unmatched segments', () => { // ... existing test }) + + it('returns single unmatched segment when no match found', () => { + const segments = buildSubstringHighlightSegments('React Documentation', 'zzz') + expect(segments).toEqual([{ text: 'React Documentation', matched: false }]) + }) + + it('returns empty array for empty text', () => { + expect(buildSubstringHighlightSegments('', 'test')).toEqual([]) + }) + + it('returns unmatched segment for empty query', () => { + const segments = buildSubstringHighlightSegments('React', '') + expect(segments).toEqual([{ text: 'React', matched: false }]) + }) }) describe('buildIndexedHighlightSegments', () => { it('builds segments from inclusive indices', () => { // ... existing test }) + + it('returns single unmatched segment for empty indices', () => { + const segments = buildIndexedHighlightSegments('React', []) + expect(segments).toEqual([{ text: 'React', matched: false }]) + }) + + it('merges overlapping ranges', () => { + const segments = buildIndexedHighlightSegments('React', [[0, 2], [1, 3]]) + expect(segments).toEqual([ + { text: 'Reac', matched: true }, + { text: 't', matched: false }, + ]) + }) })src/components/raindrop/collection-selector.tsx (2)
32-112:collection-search.tsxとのコード重複
FlatCollectionOption、FlatCollectionSearchResult、flattenCollections、ハイライトレンダリング関数がcollection-search.tsxとほぼ同一の実装で重複している。共通モジュールへの抽出を推奨。#!/bin/bash # 両ファイルの重複箇所を確認 echo "=== collection-selector.tsx ===" rg -n 'flattenCollections|FlatCollection|renderHighlightedName|highlightMatch' src/components/raindrop/collection-selector.tsx echo "" echo "=== collection-search.tsx ===" rg -n 'flattenCollections|FlatCollection|highlightMatch' src/components/raindrop/collection-search.tsx
169-179:hasSearchQueryをuseMemoの依存配列に含めることについて
hasSearchQueryはsearchQueryから派生した値なので、useMemoの依存配列にsearchQueryがあればhasSearchQueryは不要。ただし、空クエリ時の早期リターンとして意図的に使っているなら実害はない。依存配列の簡略化案
const fuzzyCollections = useMemo(() => { - if (!hasSearchQuery) return [] + if (!searchQuery.trim()) return [] return fuzzySearchByName(flatCollections, searchQuery).map( (result): FlatCollectionSearchResult => ({ item: result.item, indices: result.indices, }), ) - }, [flatCollections, hasSearchQuery, searchQuery]) + }, [flatCollections, searchQuery])e2e/specs/search.spec.ts (4)
42-57:waitForTimeout(100)はフレーキーテストの原因になります。Line 47 の
waitForTimeout(100)は固定待機であり、CI環境では不安定になりがちです。ダイアログが閉じたことを明示的に待つ方が堅牢です。♻️ 提案
if (await commandDialog.isVisible()) { await page.keyboard.press('Escape') - await page.waitForTimeout(100) + await expect(commandDialog).not.toBeVisible() }As per coding guidelines, "Appropriate timeouts and waitFor usage".
209-211: CSSクラスベースのセレクタは脆弱です。
button:has(svg.lucide-search)はアイコンライブラリの内部クラスに依存しており、バージョンアップで壊れる可能性があります。data-testidやrole+nameの使用を推奨します。As per coding guidelines, "Resilient selectors (role, data-slot, data-testid)".
231-234:page.locator('strong').first()はスコープが広すぎます。ページ全体から最初の
<strong>を取得しているため、意図しない要素にマッチする可能性があります。親コンテナでスコープを絞るか、data-testidの付与を検討してください。As per coding guidelines, "Resilient selectors (role, data-slot, data-testid)".
268-294: パフォーマンステストはE2EではなくユニットテストとしてFuse.jsを直接実行しています。このテストはアプリのUIを経由せず、Node.js上でFuse.jsを直接呼び出しています。E2Eテストスイートに含める意図は理解できますが、実行環境の違い（Node vs Chromium renderer）により、実際のアプリ内パフォーマンスとの乖離が生じる可能性があります。ユニットテストファイルへの移動も検討してください。
src/components/main-app.tsx (1)
481-481:groups.flatMap(...)が毎レンダーで新しい配列参照を生成しています。
groups.flatMap((g) => g.collections)が3箇所（Line 481, 514, 556）で呼ばれており、それぞれ新しい配列参照を生成します。useMemoで一度だけ計算し、共有することでメモ化された子コンポーネントの不要な再レンダーを防げます。♻️ 提案
+ const allCollections = useMemo( + () => groups.flatMap((g) => g.collections), + [groups], + )そして各箇所で
allCollectionsを参照するように変更してください。Also applies to: 514-514, 556-556
src/components/raindrop/raindrop-list-item.tsx (1)
72-93:renderHighlightedTextが3ファイルで重複しています。同一の実装が
raindrop-card.tsx、raindrop-list-item.tsx、global-search-command.tsxに存在します。src/lib/search.tsや共有ユーティリティに抽出することでDRY原則に準拠できます。src/components/raindrop/global-search-command.tsx (1)
328-332:toggleSearchModeがuseCallbackでラップされていません。他のハンドラーは
useCallbackでメモ化されていますが、toggleSearchModeはプレーンな関数宣言です。React.memoされた親コンポーネント内で一貫性を保つため、useCallbackの使用を推奨します。♻️ 提案
- const toggleSearchMode = () => { - if (!canToggleGlobalSearch) return - const nextMode: SearchMode = isScopedSearch ? 'global' : 'scoped' - dispatch(setSearchModeAction(nextMode)) - } + const toggleSearchMode = useCallback(() => { + if (!canToggleGlobalSearch) return + const nextMode: SearchMode = isScopedSearch ? 'global' : 'scoped' + dispatch(setSearchModeAction(nextMode)) + }, [canToggleGlobalSearch, isScopedSearch, dispatch])src/components/raindrop/main-content.tsx (1)
395-409:handleSearchInputClearがuseCallbackでラップされていません。Line 399-402 の
handleSearchInputClearがプレーンなアロー関数として定義されています。同ファイル内の他のハンドラー（handleToggleSearchMode、handleSearchGlobally、clearSearch）はuseCallbackを使用しているため、一貫性を保つべきです。♻️ 提案
- const handleSearchInputClear = () => { - onSearchQueryChange('') - setIsAdvancedSearchOpen(false) - } + const handleSearchInputClear = useCallback(() => { + onSearchQueryChange('') + setIsAdvancedSearchOpen(false) + }, [onSearchQueryChange])
Prevent crashes in list and card render paths by using safe domain extraction with URL fallback when bookmark URLs are invalid.
Use text-first locators in smoke and CRUD E2E suites so readiness checks work consistently across environments where the accessible role can differ.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/specs/smoke.spec.ts (1)
17-19:getByText().first()はセレクタの堅牢性を下げる
getByRole('link', { name: 'All Bookmarks' })からgetByText('All Bookmarks').first()への変更は、コーディングガイドラインが推奨する「resilient selectors (role, data-slot, data-testid)」から離れています。.first()は DOM の順序に依存するため、レイアウト変更でサイレントに誤った要素をマッチする恐れがあります。環境間でロールが異なる問題への対処であれば、
data-testid="all-bookmarks-breadcrumb"のような専用属性を付与する方がより安定します。💡 提案: data-testid を使ったアプローチ
- await expect(page.getByText('All Bookmarks').first()).toBeVisible({ - timeout: 10_000, - }) + await expect(page.getByTestId('all-bookmarks-breadcrumb')).toBeVisible({ + timeout: 10_000, + })（対応する UI コンポーネント側に
data-testid="all-bookmarks-breadcrumb"の追加が必要です）As per coding guidelines,
e2e/**/*.{ts,spec.ts}: "Resilient selectors (role, data-slot, data-testid)".
Use breadcrumb-scoped readiness checks in E2E specs so tests verify the active collection context instead of matching sidebar text alone.
1 participant
Summary
buildSearchQuery,filterByScope, highlight helpers, fuzzy helpers), wire Redux-backed search mode/scope/query state through the app, and update E2E API mocks to honor search operators.e2e/specs/search.spec.tscoverage for F1/F2/F5 plus an F5.3 performance assertion for 100 collections (<10ms average).Closes #3.
Test plan
pnpm lintpnpm typecheckpnpm testpnpm buildpnpm knippnpm test:e2epnpm coverage:e2e-specSummary by CodeRabbit
新機能
テスト