replace chunk with document when tokens are small - #167
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 18 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends the search functionality by adding document token counting and full document content retrieval. The Changes
Sequence DiagramsequenceDiagram
participant Client
participant SearchService
participant Database
Client->>SearchService: getSearchResultsElastic(query)
SearchService->>SearchService: Execute Elasticsearch query
SearchService->>SearchService: Filter by MAX_DOC_TOKENS threshold
SearchService->>SearchService: Deduplicate by metadata.url
SearchService->>SearchService: Identify eligible documents
loop For eligible documents (capped at MAX_FULL_DOCS)
SearchService->>Database: getDocumentByUrl(url)
Database->>SearchService: Return full document content
end
SearchService->>SearchService: addFullDocument() - replace PolicyIndex.text
SearchService->>Client: Return enhanced PolicyIndex[] results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 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: 3
🧹 Nitpick comments (1)
web/src/services/chatService.ts (1)
123-123: HoistMAX_DOC_TOKENSandMAX_FULL_DOCSto module scope.Both are thresholds that describe cross-function promotion policy and are re-created on every call. Pulling them out of the function bodies makes them easier to find, tune, and unit-test, and keeps
addFullDocument/getSearchResultsElasticfocused on logic.♻️ Suggested placement (near `indexName`)
const indexName = process.env.ELASTIC_INDEX ?? 'test_vectorstore4'; + +const MAX_DOC_TOKENS = 20_000; +const MAX_FULL_DOCS = 1;Then remove the local declarations inside
getSearchResultsElastic(line 123) andaddFullDocument(line 230).Also applies to: 230-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/services/chatService.ts` at line 123, Hoist the constants MAX_DOC_TOKENS and MAX_FULL_DOCS to module scope (place them near the existing indexName constant) so they are defined once for the module instead of being recreated on each call; then remove the local declarations of MAX_DOC_TOKENS inside getSearchResultsElastic and MAX_FULL_DOCS inside addFullDocument and update those functions to reference the module-level constants directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/models/chat.ts`:
- Line 78: The model field doc_tokens is declared required but some legacy
documents lack it, so either make the type optional in the chat model (change
doc_tokens to optional on the interface/type in web/src/models/chat.ts) or add
an explicit presence check in the token-gating logic in chatService.ts (before
comparing to MAX_DOC_TOKENS at the current check around line 190) to treat
missing/undefined as failing the gate; update references to use the optional
field or the explicit null/undefined check to avoid silently bypassing the token
limit.
In `@web/src/services/chatService.ts`:
- Around line 189-192: Guard against missing or non-numeric doc_tokens: in the
eligibility check that uses result.metadata.doc_tokens and MAX_DOC_TOKENS (the
block that currently returns when result.metadata.doc_tokens > MAX_DOC_TOKENS),
first verify that result.metadata.doc_tokens exists and is a number (e.g., using
typeof or Number.isFinite) before comparing; if it’s missing or not numeric,
treat the doc as ineligible (return) so legacy hits aren’t added to eligibleDocs
and later promoted to full-document fetches. Ensure the check is applied where
eligibleDocs is populated and references result.metadata.doc_tokens and
MAX_DOC_TOKENS.
- Around line 206-223: getDocumentByUrl currently uses
prisma.documents.findFirst({ where: { url } }) which can return an arbitrary row
because documents.url is not unique; make the selection deterministic by
ordering or by correlating via a stable id. Update getDocumentByUrl to either
query by a stable identifier (e.g., look up by documentId if you index that into
Elasticsearch) or add an explicit orderBy to the Prisma query (for example
orderBy: { lastUpdated: 'desc' } or orderBy: { id: 'desc' }) so the returned
documentContents.content is deterministic; adjust the call site accordingly if
you switch to documentId-based lookup.
---
Nitpick comments:
In `@web/src/services/chatService.ts`:
- Line 123: Hoist the constants MAX_DOC_TOKENS and MAX_FULL_DOCS to module scope
(place them near the existing indexName constant) so they are defined once for
the module instead of being recreated on each call; then remove the local
declarations of MAX_DOC_TOKENS inside getSearchResultsElastic and MAX_FULL_DOCS
inside addFullDocument and update those functions to reference the module-level
constants directly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72991a4d-d2b6-4371-8143-d1be377a9fd7
📒 Files selected for processing (2)
web/src/models/chat.tsweb/src/services/chatService.ts
| // filter out documents greater token limit | ||
| if (result.metadata.doc_tokens > MAX_DOC_TOKENS) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard against missing doc_tokens so legacy hits aren't silently promoted.
If a hit comes back without doc_tokens (e.g., indexed before this PR), result.metadata.doc_tokens > MAX_DOC_TOKENS is undefined > 20000 → false, so the URL is added to eligibleDocs and may get promoted to a potentially very large full-document fetch. Require the field to be present and numeric before considering the doc eligible.
🛡️ Proposed fix
- // filter out documents greater token limit
- if (result.metadata.doc_tokens > MAX_DOC_TOKENS) {
- return;
- }
+ // filter out docs missing a token count or exceeding the limit
+ const tokens = result.metadata.doc_tokens;
+ if (typeof tokens !== 'number' || tokens > MAX_DOC_TOKENS) {
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // filter out documents greater token limit | |
| if (result.metadata.doc_tokens > MAX_DOC_TOKENS) { | |
| return; | |
| } | |
| // filter out docs missing a token count or exceeding the limit | |
| const tokens = result.metadata.doc_tokens; | |
| if (typeof tokens !== 'number' || tokens > MAX_DOC_TOKENS) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/services/chatService.ts` around lines 189 - 192, Guard against
missing or non-numeric doc_tokens: in the eligibility check that uses
result.metadata.doc_tokens and MAX_DOC_TOKENS (the block that currently returns
when result.metadata.doc_tokens > MAX_DOC_TOKENS), first verify that
result.metadata.doc_tokens exists and is a number (e.g., using typeof or
Number.isFinite) before comparing; if it’s missing or not numeric, treat the doc
as ineligible (return) so legacy hits aren’t added to eligibleDocs and later
promoted to full-document fetches. Ensure the check is applied where
eligibleDocs is populated and references result.metadata.doc_tokens and
MAX_DOC_TOKENS.
| export const getDocumentByUrl = async (url: string): Promise<string | null> => { | ||
| if (!url) { | ||
| return null; | ||
| } | ||
|
|
||
| const document = await prisma.documents.findFirst({ | ||
| where: { url }, | ||
| select: { | ||
| documentContents: { | ||
| select: { | ||
| content: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| return document?.documentContents?.content ?? null; | ||
| }; |
There was a problem hiding this comment.
documents.url is not unique — findFirst may return an arbitrary match.
Per web/prisma/schema.prisma, documents.url is a nullable String? with only a non-unique index (ix_documents_url); nothing prevents multiple rows sharing the same URL (e.g., re-ingests, historical versions). findFirst({ where: { url } }) will pick one non-deterministically, and the returned documentContents.content may not correspond to the Elasticsearch hit that was actually matched.
Consider one of:
- Correlate by a stable identifier instead of URL (e.g., index the internal
documentIdinto the ES metadata and look up by primary key). - Add an explicit
orderBy(e.g., newestlastUpdatedor highestid) so selection is deterministic. - If URLs are expected to be unique in practice, enforce that with a DB constraint.
♻️ Minimal deterministic fallback
const document = await prisma.documents.findFirst({
where: { url },
+ orderBy: { lastUpdated: 'desc' },
select: {
documentContents: {
select: {
content: true,
},
},
},
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/services/chatService.ts` around lines 206 - 223, getDocumentByUrl
currently uses prisma.documents.findFirst({ where: { url } }) which can return
an arbitrary row because documents.url is not unique; make the selection
deterministic by ordering or by correlating via a stable id. Update
getDocumentByUrl to either query by a stable identifier (e.g., look up by
documentId if you index that into Elasticsearch) or add an explicit orderBy to
the Prisma query (for example orderBy: { lastUpdated: 'desc' } or orderBy: { id:
'desc' }) so the returned documentContents.content is deterministic; adjust the
call site accordingly if you switch to documentId-based lookup.
Co-authored-by: Copilot <copilot@github.com>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes