Skip to content

fix(spec): correct PDF search result pages - #20

Merged
caio-pizzol merged 5 commits into
mainfrom
caio/fix-pdf-page-navigation
Aug 12, 2026
Merged

fix(spec): correct PDF search result pages#20
caio-pizzol merged 5 commits into
mainfrom
caio/fix-pdf-page-navigation

Conversation

@caio-pizzol

@caio-pizzol caio-pizzol commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What was wrong

Search results could open the wrong PDF page because:

  • stored page numbers did not include PDF front matter;
  • contents rows were indexed as real sections;
  • every chunk used its section's first page.

Fix

  • Convert printed pages to PDF sheets in the viewer.
  • Extract and chunk page-by-page, and ignore contents rows.
  • Replace each part during re-ingestion so old rows are not duplicated.
  • Use the same hosted PDFs and Voyage model as production search.

Ben's example, Part 1 §17.3.1.12, now returns printed pages 219, 221, and 223. Page 219 opens PDF sheet 229.

Supersedes #12 after changes to main. Based on @benglewis's report and original work; Ben is credited as a co-author.

Tested

  • bun run test
  • bun run pdf:test
  • bun run typecheck
  • bun run lint
  • bun run build
  • Regenerated and replaced all four production corpus parts.
  • Verified the live API returns the real §17.3.1.12 rows and ranks page 219 first.

Preview: https://caio-fix-pdf-navigation.ooxml-dev.pages.dev/spec?section=17.3.1.12&part=1

Co-authored-by: Ben Lewis <blewis@hirundo.io>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/web/pdf-navigation.test.ts Outdated
Co-authored-by: Ben Lewis <blewis@hirundo.io>
@caio-pizzol caio-pizzol changed the title fix(spec): open results on the correct PDF page fix(spec): rebuild page-aware PDF corpus Aug 12, 2026
@qodo-code-review

qodo-code-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)



🔴 High

1. Missing offset corrupts navigation 🐞
Description
When header detection finds no page-number votes, extraction silently records an offset of zero,
causing physical sheet numbers to be stored as printed pages and then offset again by the viewer.
Any extraction/layout change that prevents all header matches therefore makes every result in that
ingested part open too far into the PDF.
Code

scripts/ingest-pdf/extract.py[R66-67]

+    if not votes:
+        return 0
Evidence
Offset votes come only from printed_page_in_header, and the no-vote branch returns zero. That
value is written to metadata and used by chunk.ts to convert physical markers, while
pdfNavigation.ts adds nonzero offsets of 6–14 sheets for every supported part.

scripts/ingest-pdf/extract.py[50-76]
scripts/ingest-pdf/extract.py[159-165]
scripts/ingest-pdf/chunk.ts[110-112]
apps/web/src/components/pdfNavigation.ts[10-34]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Do not treat an undetectable page offset as zero. Abort extraction or require an explicit validated offset so the pipeline cannot successfully upload systematically incorrect page numbers.

## Issue Context
Every supported viewer PDF has a nonzero configured front-matter offset. The detected value is persisted to metadata and subtracted by the chunker before the viewer adds its configured offset back.

## Fix Focus Areas
- scripts/ingest-pdf/extract.py[50-76]
- scripts/ingest-pdf/extract.py[159-165]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Source sync relabels corpus 🐞
Description
Changing the stable ecma-376-partN entries from fifth to fourth edition makes sources:sync
update their existing rows in place, while already-ingested spec_content keeps pointing at those
IDs. If source sync runs before, without, or after a failed reingestion, existing fifth-edition
content is falsely attributed to the fourth edition.
Code

data/sources.json[R7-9]

+      "edition": "4th",
+      "version": "2012-12",
+      "url": "https://cdn.ooxml.dev/ecma-376/part1.pdf",
Evidence
The repository defines sources.json as the canonical manifest, and sources-sync.ts upserts by
the unique stable name while replacing edition/version/URL. Both existing backfilled content and
newly uploaded content reference that same row ID, so changing its edition immediately changes the
provenance of preexisting rows.

data/README.md[5-9]
scripts/sources-sync.ts[53-64]
scripts/sources-sync.ts[71-82]
scripts/ingest-pdf/upload.ts[67-80]
db/schema.sql[18-21]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Prevent the manifest update from relabeling content already attached to the stable source IDs. Represent each edition with a distinct source identity, or coordinate the metadata change and corpus replacement as one enforced migration.

## Issue Context
The sync operation conflicts on `name` and mutates edition, version, and URL in place. Existing content references the source row by ID and is not edition-isolated.

## Fix Focus Areas
- data/sources.json[4-38]
- scripts/sources-sync.ts[53-64]
- scripts/ingest-pdf/upload.ts[67-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



View medium (4)
🟠 **Medium**
3. Whole PDF loaded into memory 🐞
Description
verifyPdf materializes the entire input PDF as an ArrayBuffer solely to calculate its SHA-256
digest. On the large multi-thousand-page corpus, this can cause excessive peak memory use or
allocation failure before extraction starts, preventing an otherwise valid ingestion run.
Code

scripts/ingest-pdf/pipeline.ts[R26-27]

+	const bytes = await Bun.file(pdfPath).arrayBuffer();
+	const actual = new Bun.CryptoHasher("sha256").update(bytes).digest("hex");
Evidence
Bun.file(pdfPath).arrayBuffer() necessarily allocates an in-memory buffer proportional to the
complete PDF, and this new verification step runs for every ingestion before extraction. The target
corpus includes multi-thousand-page PDFs, so peak memory now scales with the entire input rather
than bounded read chunks.

scripts/ingest-pdf/pipeline.ts[19-30]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PDF verifier eagerly loads the complete input file into memory before ingestion, which can make large corpus runs fail from memory pressure.

## Issue Context
The verification only needs a SHA-256 digest and runs before extraction. Preserve the existing manifest-hash comparison while avoiding a full-file `ArrayBuffer` allocation.

## Fix Focus Areas
- scripts/ingest-pdf/pipeline.ts[26-27]

Use a streaming or chunked file read to update the hasher incrementally, then compare the final digest with `source.sha256`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Chunk page number can misattribute overlap text 🐞
Description
In splitIntoChunks, when a chunk overflows CHUNK_SIZE, the new chunk is seeded with overlap (tail
text from the previous page/paragraph) but chunkPage is immediately set to currentPage, the page
of the paragraph that triggered the overflow. If a page marker occurs between the overlap's original
page and the new paragraph, the stored pageNumber for that chunk describes only the new
paragraph's page, not the page the overlap text (which leads the chunk's content) actually came
from, so citations built from this chunk can point at the wrong printed page for content near the
boundary.
Code

scripts/ingest-pdf/chunk.ts[R150-155]

+			pushChunk();

-			// Start new chunk with overlap
+			// Start new chunk with overlap, on whichever page we have reached
			const overlap = currentChunk.slice(-CHUNK_OVERLAP);
			currentChunk = `${overlap}\n\n${trimmedPara}`;
+			chunkPage = currentPage;
Evidence
pushChunk() finalizes the previous chunk, then currentChunk is rebuilt as overlap + trimmedPara
while chunkPage is set to currentPage, which was just updated to the new paragraph's page in the
loop body above (lines 137-138). The new chunk's leading content (the overlap) can therefore be
tagged with a page number that belongs to the paragraph following it rather than the page the
overlap text was extracted from.

scripts/ingest-pdf/chunk.ts[150-155]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When `splitIntoChunks` starts a new chunk with overlap text from the end of the previous chunk, it assigns `chunkPage = currentPage`, which reflects the page of the paragraph that just triggered the chunk split - not necessarily the page the overlap text came from. This can cause the stored `pageNumber` for a chunk to not match the page of its leading (overlap) content when a page marker falls between the overlap's source paragraph and the new paragraph.

## Issue Context
`chunkPage` drives `spec_content.page_number`, which the web viewer uses to build the `#page=` PDF link for search results. A wrong page number here reintroduces the same class of bug this PR is trying to fix, just at chunk boundaries instead of at the section level.

## Fix Focus Areas
- scripts/ingest-pdf/chunk.ts[116-165]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Concurrent replacements duplicate corpus 🐞
Description
Two concurrent replacements for an empty part can both delete zero rows and then insert complete
corpora because the operation has no per-part serialization. With no uniqueness constraint
preventing duplicate chunks, both transactions can commit and produce duplicate search results.
Code

packages/shared/src/db/index.ts[R76-77]

+				const deleted =
+					await transaction`DELETE FROM spec_content WHERE part_number = ${partNumber}`;
Evidence
replacePart begins directly with a part-wide delete and batch inserts without acquiring any
per-part lock. The spec_content schema has indexes but no uniqueness constraint capable of
rejecting the two inserted copies.

packages/shared/src/db/index.ts[72-88]
db/schema.sql[35-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Serialize `replacePart` calls by part number before deleting or inserting. Use a transaction-scoped advisory lock or a dedicated lock row so only one replacement for a given part can execute at a time.

## Issue Context
A transaction makes each individual replacement atomic but does not coordinate two replacements when neither delete finds an existing row. The schema intentionally allows multiple rows per section and has no corpus-level uniqueness guard.

## Fix Focus Areas
- packages/shared/src/db/index.ts[72-88]
- db/schema.sql[35-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Invalid batch size hangs 🐞
Description
replacePart accepts an unchecked batchSize, so zero, a negative number, or a non-finite value
prevents the insertion loop from advancing to completion. Because deletion occurs first inside the
transaction, the call then runs indefinitely while retaining transactional resources and locks.
Code

packages/shared/src/db/index.ts[R80-82]

+				for (let index = 0; index < items.length; index += batchSize) {
+					const batch = items.slice(index, index + batchSize).map(specContentRow);
+					await transaction`INSERT INTO spec_content ${transaction(batch)}`;
Evidence
The public options type accepts any number, the value is not validated, and it is used directly in
index += batchSize. The transaction has already issued its delete before entering that loop.

packages/shared/src/db/index.ts[59-72]
packages/shared/src/db/index.ts[76-82]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Validate `batchSize` before opening the transaction. Reject values that are not finite positive integers so the insertion loop always advances and terminates.

## Issue Context
The option is part of the newly exposed `replacePart` API and is used directly as the loop increment after the part has been deleted transactionally.

## Fix Focus Areas
- packages/shared/src/db/index.ts[59-72]
- packages/shared/src/db/index.ts[76-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Context
Review mode: 🚀 Fast: This is a small, self-contained PDF integrity check added at one ingestion boundary, with localized behavior and no security, schema, API, or concurrency change.

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗


Previous review results

Review updated until commit 17481c0

Results up to commit 6749067 🧠 Deep


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)



🔴 High

1. Source sync relabels corpus 🐞
Description
Changing the stable ecma-376-partN entries from fifth to fourth edition makes sources:sync
update their existing rows in place, while already-ingested spec_content keeps pointing at those
IDs. If source sync runs before, without, or after a failed reingestion, existing fifth-edition
content is falsely attributed to the fourth edition.
Code

data/sources.json[R7-9]

+      "edition": "4th",
+      "version": "2012-12",
+      "url": "https://cdn.ooxml.dev/ecma-376/part1.pdf",
Evidence
The repository defines sources.json as the canonical manifest, and sources-sync.ts upserts by
the unique stable name while replacing edition/version/URL. Both existing backfilled content and
newly uploaded content reference that same row ID, so changing its edition immediately changes the
provenance of preexisting rows.

data/README.md[5-9]
scripts/sources-sync.ts[53-64]
scripts/sources-sync.ts[71-82]
scripts/ingest-pdf/upload.ts[67-80]
db/schema.sql[18-21]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Prevent the manifest update from relabeling content already attached to the stable source IDs. Represent each edition with a distinct source identity, or coordinate the metadata change and corpus replacement as one enforced migration.

## Issue Context
The sync operation conflicts on `name` and mutates edition, version, and URL in place. Existing content references the source row by ID and is not edition-isolated.

## Fix Focus Areas
- data/sources.json[4-38]
- scripts/sources-sync.ts[53-64]
- scripts/ingest-pdf/upload.ts[67-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Missing offset corrupts navigation 🐞
Description
When header detection finds no page-number votes, extraction silently records an offset of zero,
causing physical sheet numbers to be stored as printed pages and then offset again by the viewer.
Any extraction/layout change that prevents all header matches therefore makes every result in that
ingested part open too far into the PDF.
Code

scripts/ingest-pdf/extract.py[R66-67]

+    if not votes:
+        return 0
Evidence
Offset votes come only from printed_page_in_header, and the no-vote branch returns zero. That
value is written to metadata and used by chunk.ts to convert physical markers, while
pdfNavigation.ts adds nonzero offsets of 6–14 sheets for every supported part.

scripts/ingest-pdf/extract.py[50-76]
scripts/ingest-pdf/extract.py[159-165]
scripts/ingest-pdf/chunk.ts[110-112]
apps/web/src/components/pdfNavigation.ts[10-34]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Do not treat an undetectable page offset as zero. Abort extraction or require an explicit validated offset so the pipeline cannot successfully upload systematically incorrect page numbers.

## Issue Context
Every supported viewer PDF has a nonzero configured front-matter offset. The detected value is persisted to metadata and subtracted by the chunker before the viewer adds its configured offset back.

## Fix Focus Areas
- scripts/ingest-pdf/extract.py[50-76]
- scripts/ingest-pdf/extract.py[159-165]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



View medium (3)
🟠 **Medium**
3. Invalid batch size hangs 🐞
Description
replacePart accepts an unchecked batchSize, so zero, a negative number, or a non-finite value
prevents the insertion loop from advancing to completion. Because deletion occurs first inside the
transaction, the call then runs indefinitely while retaining transactional resources and locks.
Code

packages/shared/src/db/index.ts[R80-82]

+				for (let index = 0; index < items.length; index += batchSize) {
+					const batch = items.slice(index, index + batchSize).map(specContentRow);
+					await transaction`INSERT INTO spec_content ${transaction(batch)}`;
Evidence
The public options type accepts any number, the value is not validated, and it is used directly in
index += batchSize. The transaction has already issued its delete before entering that loop.

packages/shared/src/db/index.ts[59-72]
packages/shared/src/db/index.ts[76-82]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Validate `batchSize` before opening the transaction. Reject values that are not finite positive integers so the insertion loop always advances and terminates.

## Issue Context
The option is part of the newly exposed `replacePart` API and is used directly as the loop increment after the part has been deleted transactionally.

## Fix Focus Areas
- packages/shared/src/db/index.ts[59-72]
- packages/shared/src/db/index.ts[76-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Chunk page number can misattribute overlap text 🐞
Description
In splitIntoChunks, when a chunk overflows CHUNK_SIZE, the new chunk is seeded with overlap (tail
text from the previous page/paragraph) but chunkPage is immediately set to currentPage, the page
of the paragraph that triggered the overflow. If a page marker occurs between the overlap's original
page and the new paragraph, the stored pageNumber for that chunk describes only the new
paragraph's page, not the page the overlap text (which leads the chunk's content) actually came
from, so citations built from this chunk can point at the wrong printed page for content near the
boundary.
Code

scripts/ingest-pdf/chunk.ts[R150-155]

+			pushChunk();

-			// Start new chunk with overlap
+			// Start new chunk with overlap, on whichever page we have reached
			const overlap = currentChunk.slice(-CHUNK_OVERLAP);
			currentChunk = `${overlap}\n\n${trimmedPara}`;
+			chunkPage = currentPage;
Evidence
pushChunk() finalizes the previous chunk, then currentChunk is rebuilt as overlap + trimmedPara
while chunkPage is set to currentPage, which was just updated to the new paragraph's page in the
loop body above (lines 137-138). The new chunk's leading content (the overlap) can therefore be
tagged with a page number that belongs to the paragraph following it rather than the page the
overlap text was extracted from.

scripts/ingest-pdf/chunk.ts[150-155]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When `splitIntoChunks` starts a new chunk with overlap text from the end of the previous chunk, it assigns `chunkPage = currentPage`, which reflects the page of the paragraph that just triggered the chunk split - not necessarily the page the overlap text came from. This can cause the stored `pageNumber` for a chunk to not match the page of its leading (overlap) content when a page marker falls between the overlap's source paragraph and the new paragraph.

## Issue Context
`chunkPage` drives `spec_content.page_number`, which the web viewer uses to build the `#page=` PDF link for search results. A wrong page number here reintroduces the same class of bug this PR is trying to fix, just at chunk boundaries instead of at the section level.

## Fix Focus Areas
- scripts/ingest-pdf/chunk.ts[116-165]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Concurrent replacements duplicate corpus 🐞
Description
Two concurrent replacements for an empty part can both delete zero rows and then insert complete
corpora because the operation has no per-part serialization. With no uniqueness constraint
preventing duplicate chunks, both transactions can commit and produce duplicate search results.
Code

packages/shared/src/db/index.ts[R76-77]

+				const deleted =
+					await transaction`DELETE FROM spec_content WHERE part_number = ${partNumber}`;
Evidence
replacePart begins directly with a part-wide delete and batch inserts without acquiring any
per-part lock. The spec_content schema has indexes but no uniqueness constraint capable of
rejecting the two inserted copies.

packages/shared/src/db/index.ts[72-88]
db/schema.sql[35-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Serialize `replacePart` calls by part number before deleting or inserting. Use a transaction-scoped advisory lock or a dedicated lock row so only one replacement for a given part can execute at a time.

## Issue Context
A transaction makes each individual replacement atomic but does not coordinate two replacements when neither delete finds an existing row. The schema intentionally allows multiple rows per section and has no corpus-level uniqueness guard.

## Fix Focus Areas
- packages/shared/src/db/index.ts[72-88]
- db/schema.sql[35-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Powered by Qodo

Comment thread scripts/ingest-pdf/extract.py Outdated
Comment thread data/sources.json

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 20 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/db/spec-content-replace.test.ts Outdated
Comment thread scripts/ingest-pdf/upload.ts
Comment thread packages/shared/src/db/index.ts Outdated
Comment thread scripts/ingest-pdf/audit.ts Outdated
Comment thread scripts/ingest-pdf/extract.py Outdated
Comment thread tests/ingest-pdf/corpus-audit.test.ts Outdated
Co-authored-by: Ben Lewis <blewis@hirundo.io>
@caio-pizzol caio-pizzol changed the title fix(spec): rebuild page-aware PDF corpus fix(spec): correct PDF search result pages Aug 12, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 13 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread scripts/ingest-pdf/extract.py
Comment thread scripts/ingest-pdf/pipeline.ts
Comment thread scripts/ingest-pdf/pipeline.ts
caiopizzol and others added 2 commits August 12, 2026 15:35
Co-authored-by: Ben Lewis <blewis@hirundo.io>
Co-authored-by: Ben Lewis <blewis@hirundo.io>
@caio-pizzol
caio-pizzol merged commit f6443f5 into main Aug 12, 2026
2 checks passed
@caio-pizzol
caio-pizzol deleted the caio/fix-pdf-page-navigation branch August 12, 2026 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants