Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📝 WalkthroughWalkthroughRule extraction now returns structured rules with horizontal positions and vertical extents. Nearby rule fragments merge into continuous rules. Layout analysis derives ruled columns from row coverage and applies splits only where rules reach each row. Markdown generation passes this metadata through the grid pipeline. Tests cover rule filtering, bounded tables, shared cells, and duplicate text. PDF fixtures support bounded rule tuples. Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to This change substantially reduces duplicate table-cell writes and shrinks extracted text across the tested PDF corpus, which is a solid improvement. However, one confirmed gap remains: when two separate tables sit side by side and their row ranges overlap vertically, the layout logic can still merge them into a single table and duplicate captions or notes across columns—the very problem this PR is meant to fix. This should be addressed before merge to avoid reintroducing the regression in that scenario. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: 308ae4637d
ℹ️ 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".
| const line = lines.find( | ||
| (l) => Math.abs(l.x - piece.x) <= SAME_X && piece.bottom <= l.top + MEETS, | ||
| ); | ||
| if (line) { | ||
| line.top = Math.max(line.top, piece.top); |
There was a problem hiding this comment.
When fragments of the same rule have slightly different x coordinates, the x-first sort can visit a higher fragment before a lower one. The one-sided piece.bottom <= l.top + MEETS check then joins the lower fragment to the higher fragment even when they are vertically disjoint, and because only top is updated, the lower extent disappears. On row-by-row ruled tables with coordinate jitter, this can silently remove the rule from lower rows and assign shared values to the wrong model; require the candidate ranges to meet in both directions and preserve both bounds.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
| const split = across.filter((i, k) => k === 0 || runsPast(i)); | ||
| const into = across.length === columns.length ? split.slice(0, 1) : split; |
There was a problem hiding this comment.
On a page containing vertically separate ruled tables with different horizontal boundaries, columns is the union of every table's rules, so a caption spanning all columns of one table usually does not satisfy across.length === columns.length. For example, a table spanning rules at 100/200 and another spanning 50/150 causes a caption from x=80 through x=220 in the first table to be emitted three times. Determine full width against the rules active at the row rather than every ruled column on the page.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: cead965d-f497-4dd6-9418-d33a53e623be
📒 Files selected for processing (4)
apps/worker/src/layout.tsapps/worker/src/render.tsapps/worker/test/layout.test.tsapps/worker/test/pdf.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14f322ee97
ℹ️ 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".
| if (across.length > 0) { | ||
| for (const i of across) line[i] = line[i] ? `${line[i]} ${cell.text}` : cell.text; | ||
| // A new column only where the rule opening it runs past this row; else still the last one. | ||
| const split = across.filter((i, k) => k === 0 || runsPast(i)); |
There was a problem hiding this comment.
When vertically separate tables contribute interleaved x positions, this unconditional retention of the first geometric bucket assigns cells to rules that are inactive for the current row. For example, with an upper table ruled at x=100/200 and a lower table ruled at x=50/150, an upper-table label beginning at x=60 is placed in the phantom x=50 column while another label beginning at x=20 remains in the first column; the resulting Markdown silently gives equivalent rows different schemas and can associate values with the wrong model. Map the first bucket back to the nearest preceding rule active on this row rather than always retaining its global index.
Useful? React with 👍 / 👎.
| const standing = edges | ||
| .filter((edge, i) => { | ||
| const at = edge[0]?.x ?? 0; | ||
| const to = edges[i + 1]?.[0]?.x ?? Number.POSITIVE_INFINITY; |
There was a problem hiding this comment.
When vertically disjoint tables have interleaved boundaries, to is taken from the next edge anywhere on the page rather than the next edge active beside this rule. With upper rules at x=100/200 and lower rules at x=50/150, the x=100 edge is tested only against cells starting before x=150; valid values centered at x=160 in its 100–200 column are therefore excluded, and after two such rows the HOLDS check discards the real boundary. The grid then cannot recognize spanning cells across that boundary, so shared values can be emitted under only one model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: bdf37c7b-d22c-4b76-af16-35b348a725e5
📒 Files selected for processing (3)
apps/worker/src/layout.tsapps/worker/src/render.tsapps/worker/test/layout.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| if (table && rule.bottom <= table.top + PAST) { | ||
| table.top = Math.max(table.top, rule.top); | ||
| table.rules.push(rule); | ||
| } else tables.push({ bottom: rule.bottom, top: rule.top, rules: [rule] }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '380,520p' apps/worker/src/layout.tsRepository: origin89hq/data
Length of output: 7150
🏁 Script executed:
sed -n '300,390p' apps/worker/src/layout.ts
printf '\\n--- gridOf references ---\\n'
rg -n -C 8 'gridOf|active|spans' apps/worker/src/layout.tsRepository: origin89hq/data
Length of output: 10413
Preserve separate side-by-side tables.
The tables loop sorts all standing rules by bottom and checks only vertical overlap. Rules at different x coordinates can therefore enter the same table. Each rule in that table then receives the same table-wide span.
gridOf marks every ruled column whose span covers the row as active. When a cell crosses only one horizontal table, the active-rule count includes the other table, so into keeps every crossed column instead of collapsing the cell into one column.
Group rules by horizontal table region, or preserve table-specific spans when determining active rules in gridOf.
Change
Part of #230. The page reader split a page at every thin upright line drawn on it, and wrote a cell crossing such a line into every column it crossed.
rulesOnkept only where each line stood, not how far it ran, so any upright stroke anywhere on the page divided every row of it: the strokes of letters drawn as shapes (Victron's off-grid booklet has 57 on one page, 5 to 6 points tall), the sides of boxes, a page's frame, and the rules of a table far below. Across wave 2's conversions, 606 of 1,115 documents and 4,188 of 19,463 pages had a line with one cell written three times or more; on the PDFs measured below it was a third of all the text, and the reader was paid to read it.rulesOnkeeps each line's height and joins the pieces of a line drawn a row at a time: pieces a hair apart across are one line whatever order they come in, and join down it only where they meet.How characters are put into lines is unchanged:
rowsOfis main's. Two lines shuffled into one (Phosphate (P1o-)l,y Hetehxyaleflnueo) remain, and #230 stays open for them with what was learned trying. Two close columns of an unruled table written as one cell (5800 8500) is #236.Deploying this changes only documents converted from then on. A document converted before keeps its text, since its conversion records its page count and is not made again, and the readings of it stand. Wave 2 is best read again once, with the prompt fixes still to come, under a new prompt version.
Validation
just checkpasses.12/24VDC★ Auto-recognitionunder three models and the static losses under two, are as before. OutBack's FLEXmax settings keep their 24, 36 and 48 volt columns. The Victron page that wrote one sentence into 14 columns reads as prose.