feat(search): surface PDF highlight bodies in Global Search (#864) - #873
Merged
Conversation
Extend /api/search to also probe pdf_highlights.text with owner-scoping
and a defensive sources.kind='pdf_local' check, then plumb the new
discriminated rows through the client merge logic so the global search
dropdown and /search page render highlight hits and deep-link to either
the derived Zedi page or /sources/:sourceId/pdf#page=N.
- Server: add highlight query gated by PDF_HIGHLIGHT_SEARCH_DISABLED env
kill switch; tag every result row with kind ("page" / "pdf_highlight").
- Client: turn SearchSharedResponse into a discriminated union, route
highlight rows via resolveSearchResultUrl, and score them below page
matches to preserve "title > derived page > highlight body" ordering.
- Tests: add unit coverage for owner-scoping, kind="pdf_local" filter,
feature-flag kill switch, URL composition, and the merged result shape.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds PDF-highlight results end-to-end: new API union rows, server-side highlight query (owner + sources.kind='pdf_local') with kill-switch, reserved-budget merge with pages, client discriminated-union model and dedup changes, kind-aware routing, UI updates, and tests validating SQL, payload shapes, and merged limits. ChangesPDF Highlight Search Integration
Sequence Diagram(s)sequenceDiagram
participant SearchUI as Search UI
participant Endpoint as GET /api/search
participant PageQuery as Page Query
participant HighlightQuery as Highlight Query
participant Client as useGlobalSearch
SearchUI->>Endpoint: q=term&scope=own
Endpoint->>PageQuery: Query pages by scope
PageQuery-->>Endpoint: [{ kind: "page", ... }]
alt PDF_HIGHLIGHT_SEARCH_DISABLED not set
Endpoint->>HighlightQuery: Query pdf_highlights (owner_id, sources.kind='pdf_local')
HighlightQuery-->>Endpoint: [{ kind: "pdf_highlight", ... }]
end
Endpoint-->>Client: { results: SearchResultRow[] }
Client->>Client: buildGlobalSearchResults()
Client->>Client: dedupSharedRowsAgainstPersonal() (page-only)
Client->>Client: Merge personal + pages + highlights, sort & slice
Client-->>SearchUI: GlobalSearchResultItem[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates PDF highlights into the global search functionality by introducing a discriminated union for search results and adding backend support for querying the pdf_highlights table with a configurable kill switch. The frontend is updated to display these highlights with specific icons and deep-linking capabilities. Feedback suggests centralizing the formatting logic for PDF highlights to avoid duplication between the search hook and the results page.
Comment on lines
+136
to
+162
| const sharedHighlights: PdfHighlightSearchResultItem[] = dedupedShared | ||
| .filter((r): r is SearchPdfHighlightResultRow => r.kind === "pdf_highlight") | ||
| .map((r) => { | ||
| const snippet = extractSmartSnippet(r.text, keywords, 200); | ||
| const highlightedSnippet = highlightKeywords(snippet, keywords); | ||
| const file = | ||
| (r.source_display_name?.trim() || r.source_title?.trim() || null) ?? | ||
| t("common.pdfHighlightFallbackName", { defaultValue: "PDF" }); | ||
| return { | ||
| kind: "pdf_highlight", | ||
| highlightId: r.highlight_id, | ||
| sourceId: r.source_id, | ||
| pdfPage: r.pdf_page, | ||
| derivedPageId: r.derived_page_id, | ||
| title: t("common.pdfHighlightResultTitle", { | ||
| defaultValue: "{{file}} (p.{{page}})", | ||
| file, | ||
| page: r.pdf_page, | ||
| }), | ||
| snippet, | ||
| highlightedSnippet, | ||
| matchType: "content" as MatchType, | ||
| thumbnailUrl: undefined, | ||
| updatedAt: new Date(r.updated_at).getTime(), | ||
| score: PDF_HIGHLIGHT_BASE_SCORE, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The logic for formatting PDF highlight results is duplicated here from src/hooks/useGlobalSearch.ts. This includes i18n keys, default values, and snippet extraction. It is recommended to centralize this logic, for example by making buildPdfHighlightItem more flexible or creating a shared formatting helper, to ensure consistency and ease of maintenance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/layout/Header/HeaderSearchDropdownContent.tsx (1)
11-27: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd bilingual TSDoc for the exported props interface.
HeaderSearchDropdownContentPropsis part of the exported contract but currently has an empty doc block. Please add explicit JP/EN TSDoc describing key props (searchResults,onSelectItem, keyboard-indexing props).Proposed doc patch
/** - * + * Header search dropdown rendering contract. + * + * ヘッダー検索ドロップダウンの描画契約。 */ export interface HeaderSearchDropdownContentProps {As per coding guidelines,
**/*.{ts,tsx}: “Add TSDoc / JSDoc comments to exported functions, types, and interfaces” and**/*.{ts,tsx,js,md}: “Include both Japanese and English comments/documentation in code and documentation files”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/Header/HeaderSearchDropdownContent.tsx` around lines 11 - 27, Add bilingual (JP/EN) TSDoc for the exported interface HeaderSearchDropdownContentProps: describe the purpose of the interface and document key props such as searchResults (array of GlobalSearchResultItem returned by the search), onSelectItem (callback invoked when a result is chosen), keyboard-indexing props like activeIndex, getOptionId and setActiveIndex (how indexing/IDs are used for keyboard navigation), and other important props (hasContent, showEmpty, showResults, query, hasQuery, listRef, footerRef, closeDropdown, handleSearchSubmit, itemCount). Keep each prop comment concise and provide both Japanese and English sentences for the interface and each property, following existing TSDoc style in the repo.
🧹 Nitpick comments (2)
src/lib/api/types.ts (1)
242-242: ⚡ Quick winAdd a dedicated TSDoc block for
SearchResultRow.
SearchResultRowis exported but currently undocumented as its own symbol, which weakens contract discoverability for downstream users.As per coding guidelines
**/*.{ts,tsx}: Add TSDoc / JSDoc comments to all exported functions, types, and interfaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/api/types.ts` at line 242, Add a TSDoc block above the exported type SearchResultRow describing that it is a union of SearchPageResultRow and SearchPdfHighlightResultRow and explaining when each variant is returned (e.g., page-level results vs PDF highlight results), including any important fields or usage notes; ensure the comment references the related types SearchPageResultRow and SearchPdfHighlightResultRow and follows the project's TSDoc/JSDoc style for exported types.server/api/src/__tests__/routes/search.test.ts (1)
214-241: ⚡ Quick winStrengthen response-shape assertions for contract fields.
These tests currently pass even if page rows include unintended fields or miss required page fields. Please assert the required page shape (e.g.,
owner_id,thumbnail_url,source_url) and explicitly assertcontent_textis absent.Also applies to: 417-458
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/src/__tests__/routes/search.test.ts` around lines 214 - 241, Update the test "response page rows are tagged with kind='page' and include note_id" (the it block) to more strictly assert the page response shape: after parsing body.results (from the createSearchApp + authHeaders flow) assert that the result object contains required page fields owner_id, thumbnail_url, and source_url, and explicitly assert that content_text is absent (e.g., result.content_text is undefined or result does not have the key). Apply the same stronger assertions to the other similar test block referenced (the test around lines 417-458) so both tests validate presence of required page fields and absence of content_text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/src/routes/search.ts`:
- Around line 160-169: Replace the spreading of raw SQL rows in taggedPageRows
with an explicit mapping that builds each object only from the contract fields
defined by SearchPageResultRow (do not include pc.content_text or any other raw
columns); in other words, change the taggedPageRows mapping over pageRows to
pick and assign only the allowed properties (e.g., the exact keys declared in
SearchPageResultRow) and then add kind: "page" and the appropriate type
assertion to SearchPageResultRow, then merge with highlightRows and return via
c.json as before (leave runPdfHighlightSearch and the final c.json call
unchanged).
In `@src/components/search/SearchResultCard.tsx`:
- Around line 27-40: Add concise bilingual (Japanese and English) TSDoc comments
for the two newly exported interfaces: SearchResultCardPageItem and
SearchResultCardPdfHighlightItem; for each interface include a short summary in
EN/JP describing its purpose and list of fields (e.g., pageId, noteId, sourceUrl
for SearchResultCardPageItem and highlightId, sourceId, pdfPage, derivedPageId
for SearchResultCardPdfHighlightItem) so the public contract is explicit and
follows the repository's TSDoc/JSDoc guidelines.
In `@src/hooks/useGlobalSearch.test.ts`:
- Line 415: The section comment "Issue `#864`: PDF ハイライト統合" in the
useGlobalSearch.test file is Japanese-only; update that comment to include an
English counterpart (e.g., "Issue `#864`: PDF ハイライト統合 / PDF highlight integration"
or similar) so comments comply with the bilingual guideline; locate the Japanese
string "PDF ハイライト統合" in the test file and replace or append the English
translation adjacent to it.
---
Outside diff comments:
In `@src/components/layout/Header/HeaderSearchDropdownContent.tsx`:
- Around line 11-27: Add bilingual (JP/EN) TSDoc for the exported interface
HeaderSearchDropdownContentProps: describe the purpose of the interface and
document key props such as searchResults (array of GlobalSearchResultItem
returned by the search), onSelectItem (callback invoked when a result is
chosen), keyboard-indexing props like activeIndex, getOptionId and
setActiveIndex (how indexing/IDs are used for keyboard navigation), and other
important props (hasContent, showEmpty, showResults, query, hasQuery, listRef,
footerRef, closeDropdown, handleSearchSubmit, itemCount). Keep each prop comment
concise and provide both Japanese and English sentences for the interface and
each property, following existing TSDoc style in the repo.
---
Nitpick comments:
In `@server/api/src/__tests__/routes/search.test.ts`:
- Around line 214-241: Update the test "response page rows are tagged with
kind='page' and include note_id" (the it block) to more strictly assert the page
response shape: after parsing body.results (from the createSearchApp +
authHeaders flow) assert that the result object contains required page fields
owner_id, thumbnail_url, and source_url, and explicitly assert that content_text
is absent (e.g., result.content_text is undefined or result does not have the
key). Apply the same stronger assertions to the other similar test block
referenced (the test around lines 417-458) so both tests validate presence of
required page fields and absence of content_text.
In `@src/lib/api/types.ts`:
- Line 242: Add a TSDoc block above the exported type SearchResultRow describing
that it is a union of SearchPageResultRow and SearchPdfHighlightResultRow and
explaining when each variant is returned (e.g., page-level results vs PDF
highlight results), including any important fields or usage notes; ensure the
comment references the related types SearchPageResultRow and
SearchPdfHighlightResultRow and follows the project's TSDoc/JSDoc style for
exported types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c24a6dbf-09d1-406f-808a-8ec35f081bac
📒 Files selected for processing (11)
server/api/src/__tests__/routes/search.test.tsserver/api/src/routes/search.tssrc/components/layout/Header/HeaderSearchBar.tsxsrc/components/layout/Header/HeaderSearchDropdownContent.tsxsrc/components/search/SearchResultCard.tsxsrc/contexts/GlobalSearchContext.test.tssrc/contexts/GlobalSearchContext.tsxsrc/hooks/useGlobalSearch.test.tssrc/hooks/useGlobalSearch.tssrc/lib/api/types.tssrc/pages/SearchResults.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640d40d989
ℹ️ 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 on lines
+167
to
+169
| const highlightRows = await runPdfHighlightSearch(db, userId, pattern, limit); | ||
|
|
||
| return c.json({ results: [...taggedPageRows, ...highlightRows] }); |
There was a problem hiding this comment.
The new flow applies LIMIT ${limit} to the page query and again to the highlight query, then concatenates both arrays without a final cap, so /api/search can now return up to 2 * limit rows (e.g., 40 when limit is 20). This changes the endpoint’s effective limit semantics and can increase payload size/latency for callers that rely on limit as a hard bound; apply a final sort/slice (or split the limit budget) before returning results.
Useful? React with 👍 / 👎.
…ation - Drop `pc.content_text` from the page-query SELECT and whitelist response fields explicitly so the API never echoes full page bodies back to the client (PR #873 review: CodeRabbit). - Cap the merged page+highlight result list at `limit` after concatenation; each branch had its own LIMIT, so a naïve concat could return up to 2*limit rows (PR #873 review: codex). - Extract `formatPdfHighlightDisplay` so the header dropdown and /search page share one i18n / snippet helper instead of duplicating it (PR #873 review: Gemini). Translator function is injected so the helper works with both i18next's global `t` and react-i18next's hook. - Add bilingual TSDoc to `SearchResultCardPageItem` / `SearchResultCardPdfHighlightItem` (PR #873 review: CodeRabbit). - Add English text to the section comment in `useGlobalSearch.test.ts` (PR #873 review: CodeRabbit). - Add tests covering the whitelist, the final limit cap, and the shared display helper's fallback / length-override paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/src/routes/search.ts`:
- Around line 200-211: The current concat-and-slice (const merged =
[...taggedPageRows, ...highlightRows].slice(0, limit)) prematurely enforces the
global limit and can drop all highlightRows; instead stop clipping here so
cross-kind ranking can decide the final top N—remove the .slice(0, limit) and
return or propagate both taggedPageRows and highlightRows (or return the full
merged array) to the layer that performs final ranking; alternatively if you
must enforce a hard cap here, implement a reserved-budget strategy (e.g.,
allocate per-kind quotas from limit and slice each of taggedPageRows and
highlightRows accordingly) and document the reservation logic. Ensure
references: taggedPageRows, highlightRows, merged, runPdfHighlightSearch, and
limit are updated consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80d0bb40-f5c2-41e4-b545-654ea6998c27
📒 Files selected for processing (6)
server/api/src/__tests__/routes/search.test.tsserver/api/src/routes/search.tssrc/components/search/SearchResultCard.tsxsrc/hooks/useGlobalSearch.test.tssrc/hooks/useGlobalSearch.tssrc/pages/SearchResults.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hooks/useGlobalSearch.test.ts
- src/components/search/SearchResultCard.tsx
- src/hooks/useGlobalSearch.ts
… review) The previous `.slice(0, limit)` honored Codex's `limit` hard cap but let page rows starve the highlight branch entirely whenever there were `limit` or more page hits (CodeRabbit follow-up review on PR #873). Switch to a reserved-budget merge that satisfies both reviewers: - Highlights get up to `ceil(limit / 4)` reserved slots so the new feature is never silently dropped when pages dominate. - Pages get the remainder, and unused highlight slots spill back so pages can still fill the response when there are no highlights. - Total is always <= `limit` (codex's hard cap preserved). Adds three tests: highlight reservation under page saturation, page spill-back when highlights are empty, and full-highlight fill when pages are empty.
3 tasks
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
2 participants
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.
Extend /api/search to also probe pdf_highlights.text with owner-scoping
and a defensive sources.kind='pdf_local' check, then plumb the new
discriminated rows through the client merge logic so the global search
dropdown and /search page render highlight hits and deep-link to either
the derived Zedi page or /sources/:sourceId/pdf#page=N.
kill switch; tag every result row with kind ("page" / "pdf_highlight").
highlight rows via resolveSearchResultUrl, and score them below page
matches to preserve "title > derived page > highlight body" ordering.
feature-flag kill switch, URL composition, and the merged result shape.
Summary by CodeRabbit
New Features
Bug Fixes
Tests