Repository navigation
#138 — split fixture Cell into display/key axes + POD-render fixture tables - #140
Merged
Merged
Conversation
…ables (#138) A fixture cell carried only its canonical CellValue (1, 1.0, 1.00 all canonicalize to Number("1")). That is right for equality -- a format-only reformat must not diff -- but wrong for display: the Diff view rendered the normalized value, not what the author wrote. Split the cell into two axes: - display: the authored token (truth), rendered in BOTH Current and Diff views (a csv 1.00 now shows 1.00, not the normalized 1). - key: the existing canonical CellValue, used ONLY for the diff's equality decision and row alignment. A format-only cell change (1 -> 1.00; csv 1.0 -> 1.00) shows the current authored value AND is not flagged (keys equal); a real value change is flagged and shows old-display -> new-display. Row alignment stays Rust-computed on the canonical key (rows do not re-pair when the display lens changes -- foundation for the #139 normalize toggle). Both display and key ship per old/new cell so #139 can re-flag client-side. Domain (unit_test_table.rs): - Cell is now { display: String, key: CellValue } with Cell::new (display derived from key) and Cell::with_display (authored path). - cell_from_value / cell_from_scalar / cell_from_csv_token capture the raw authored token as display alongside the canonical key; the normalizers thread these Cell-producing fns. (serde_json has no arbitrary_precision, so a dict-numeric 1.00 arrives as 1.0 upstream; the csv/dict-string paths preserve trailing zeros -- the fidelity is demonstrated on the csv path.) Diff (cell_diff.rs): - CellChange now carries Cell old/new (display + key) per side; changed is computed on key only (o.key != n.key). row_key / match_multiset key on the canonical key exclusively, so the display lens never re-pairs rows. Render (render.rs): - GivenPayload/ExpectedPayload ship the Rust-computed FixtureTable POD (table) for the Current view; rows/format/fixture retained only for the non-tabulatable fallback (sql code block, external-fixture affordance). current_view_table returns None for sql/opaque and external fixtures. Template (report.html): - buildTable renders the POD: text = authored display, numeric/NULL styling + data-order sort key = canonical key.t (a Number key still sorts numerically even when displayed as 1.00). The JS csv/dict table parsers (parseCsvRows, extractRowsFromFixture, collectColumns) are RETIRED -- the template is a pure renderer. The sql code-block fallback stays. Tests: - tests/headless_csv_parser.rs retired with the JS parseCsvRows twin; CSV RFC 4180 correctness is owned by the Rust parse_csv_rows unit tests (g22-g26). Headless coverage asserts the authored 1.00 / TRUE render in the Current grid. Wire-shape test pins the dual-axis { display, key } contract via a serde JSON snapshot. The playground PR-diff example now shows authored 50.00 / 100.00 / 0.00 with canonical number keys. Zero-egress (headless + resource-ref lint), synthetic-only fixtures, and the input manifest-digest snapshot are unchanged. Closes #138 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Ready to review this PR? Stage has broken it down into 8 individual chapters for you: 
Chapters generated by Stage for commit 7fc313f on Jun 6, 2026 2:55pm UTC. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSplit Cell into authored display + canonical key, propagate Cells through diff logic and payloads, add optional FixtureTable POD to test payloads, switch templates/examples to render Rust-produced table PODs, retire JS CSV/tabulation seams, and update tests/CI/docs to match. ChangesDisplay/Key Cell Refactor & POD-Driven Rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
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 |
Contributor
📄 Rendered report previewAll examples regenerated cleanly. This PR doesn't touch
Click Download to fetch the rendered HTML. Each artifact Alternative: GitHub CLI# gh CLI >= 2.63 extracts into ./report-preview-playground/.
gh run download 27065495540 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
![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 shifts the CSV and dict table parsing logic from the JavaScript frontend to the Rust backend (domain layer). It introduces a dual-axis Cell structure containing both an authored display string (for rendering fidelity) and a canonical key (for semantic equality checks and diffing). This allows the HTML report template to act as a pure renderer of the Rust-computed FixtureTable POD, retiring several JS-side parsers and headless tests. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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
🧹 Nitpick comments (1)
src/domain/unit_test_table.rs (1)
162-169: ⚡ Quick winRound-trip
Cell::with_displayexplicitly.The local serde coverage still only exercises
Cell::new(...), so a regression that drops or rewrites non-canonical displays likeCell::with_display("1.00", Number("1"))would pass here. Please add the table-driven round-trip cases for divergentdisplay/keypairs, plus the empty-displayNull/Absentcases.🧪 Example cases to fold into the existing matrix
+TableRow::new(vec![ + Cell::with_display("1.00".into(), CellValue::Number("1".into())), + Cell::with_display(String::new(), CellValue::Null), + Cell::with_display(String::new(), CellValue::Absent), +])As per coding guidelines,
src/domain/**/*.rsmust write property tests for JSON serde round-trip. Based on learnings, this repo expects those as exhaustive table-driven matrices rather thanproptest.🤖 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/domain/unit_test_table.rs` around lines 162 - 169, Add explicit JSON serde round-trip table-driven tests that exercise Cell::with_display alongside existing Cell::new cases: extend the matrix in the unit tests in src/domain/unit_test_table.rs to include divergent display/key pairs (e.g., display "1.00" with numeric key Number("1"), display "true" with Bool(true), and other non-canonical presentations) and the empty-display variants mapping Null display to Absent key and vice versa; for each case serialize to JSON and deserialize back and assert equality to the original Cell (use the same test harness/table structure already present so the new rows follow the existing matrix pattern).Sources: Coding guidelines, Learnings
🤖 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 `@src/domain/cell_diff.rs`:
- Around line 470-480: The Added/Removed row builders (added_row and
removed_row) currently set CellChange.changed = true unconditionally, violating
the old.key != new.key contract and mislabeling Absent→Absent cells as changed;
change the constructors to compute changed by comparing the cell keys (e.g.,
changed: old.key != new.key or using the Cell.key accessor) instead of
hardcoding true for both added_row and removed_row so Absent→Absent cells remain
unchanged.
---
Nitpick comments:
In `@src/domain/unit_test_table.rs`:
- Around line 162-169: Add explicit JSON serde round-trip table-driven tests
that exercise Cell::with_display alongside existing Cell::new cases: extend the
matrix in the unit tests in src/domain/unit_test_table.rs to include divergent
display/key pairs (e.g., display "1.00" with numeric key Number("1"), display
"true" with Bool(true), and other non-canonical presentations) and the
empty-display variants mapping Null display to Absent key and vice versa; for
each case serialize to JSON and deserialize back and assert equality to the
original Cell (use the same test harness/table structure already present so the
new rows follow the existing matrix pattern).
🪄 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: cff3d89f-57a1-456a-8900-cbf6d6715c2c
⛔ Files ignored due to path filters (1)
tests/snapshots/render_integration__rendered_chrome_jaffle_shop.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
.github/workflows/ci.ymlAGENTS.mdexamples/jaffle-shop-report.htmlexamples/playground-pr-diff-report.htmlexamples/playground-report.htmlfeatures/unit_test_format_coverage.featuresrc/adapters/render.rssrc/domain/cell_diff.rssrc/domain/unit_test_table.rstemplates/report.htmltests/cell_table_diff.rstests/headless_csv_parser.rstests/headless_toggle.rstests/steps/cell_table_diff.rs
💤 Files with no reviewable changes (1)
- tests/headless_csv_parser.rs
…138) added_row/removed_row hardcoded changed: true, violating the changed == old.key != new.key contract that modified_row honors. When a row add/remove coincides with a column add/remove, the unified axis projects an Absent cell into the added/removed row; that Absent->Absent cell must not be marked changed. It also matters for the #139 normalize toggle, which re-derives changed from the key axis client-side -- a server/client mismatch on these cells would desync the toggle. Compute changed from the key axis (c.key != Absent) and add a targeted builder test pinning the contract. Caught by CodeRabbit on PR #140. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…umn overlap (#138) f_added_removed_rows_keep_absent_cells_unchanged pins the builder contract directly. This adds the integration counterpart: it drives diff_fixture_tables through the production normalizer so the unified column axis organically projects an Absent cell into an Added row (a row-add overlapping a column-add), proving the end-to-end path leaves that Absent->Absent cell unflagged. Surfaced by the stage-review pass on PR #140. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Summary
A fixture cell carried only its canonical
CellValue(1,1.0,1.00all canonicalize toNumber("1")) — right for equality (a format-only reformat must not diff) but wrong for display: the Diff view rendered the normalized value, not what the author wrote. This splits the cell into two axes:display— the authored token (truth), rendered in BOTH the Current and Diff views (a csv1.00now shows1.00, not the normalized1).key— the existing canonicalCellValue, used ONLY for the diff's equality decision and row alignment.This is the foundation for the settings normalize-toggle (#139) and the sql-given tables (#137).
Behaviors
1->1.00; csv1.0->1.00) shows the current authored value AND is not flagged (keys equal); a real value change is flagged and shows old-display -> new-display.key(rows do not re-pair when the display lens changes — see feat: report settings menu (cog) — diff context-lines + normalize-equality toggle #139).displayandkeyship per old/new cell so feat: report settings menu (cog) — diff context-lines + normalize-equality toggle #139 can re-flag client-side without a Rust round-trip; the dual-axis wire is pinned by a serde JSON snapshot test.FixtureTablePOD; the JS csv/dict parsers (parseCsvRows,extractRowsFromFixture,collectColumns) are retired — the template is a pure renderer. The sql code-block + external-fixture affordances stay as the non-tabulatable fallback.Key implementation notes
serde_jsonhas noarbitrary_precision, so a dict-numeric1.00arrives as1.0upstream of this layer. The fidelity fix is therefore demonstrated on the csv path (parse_csv_rowskeeps the raw token) and dict-string path — visible in the regeneratedexamples/playground-pr-diff-report.html(authored50.00/100.00/0.00with canonical number keys).CellChange.changedis computed onkeyonly (o.key != n.key);row_key/match_multisetkey on the canonicalkeyexclusively, so the display lens never re-pairs rows.cell-num, NULL/absent styling,data-ordersort key) come fromkey.t, never fromdisplay— so an authored1.00still sorts numerically.tests/headless_csv_parser.rsretired with the JSparseCsvRowstwin; CSV RFC 4180 correctness is now owned by the Rustparse_csv_rowsunit tests (g22-g26).Validation
1.00/TRUErender in the Current grid.cell_diff.rs+unit_test_table.rs): zero survivors.This is the FOUNDATION for #139 (settings menu) and #137 (sql tables) — their scope is intentionally NOT pulled in.
Closes #138
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Documentation
Tests / Chores