Repository navigation
#98 — cell-level unit-test data-table diff: semantic, fusion-aligned, toggleable - #130
Merged
Merged
Conversation
…ne normalized)
The IR-construction layer of the cell-level data-table diff (cute-dbt#98,
Workflow 1 of 2). Turns a unit test's given/expect fixture rows — from
either the CURRENT manifest (NEW side) or a reconstructed OLD-side YAML
region — into a format- and engine-normalized FixtureTable (ordered
columns x ordered typed rows). The cell-diff algorithm (cell_diff.rs) is
a later PR.
New module src/domain/unit_test_table.rs (std + serde + serde_json::Value
only; a domain leaf):
- PODs: FixtureTable / TableRow / Cell / CellValue / FixtureFormat.
CellValue is adjacently tagged ({"t","v"}) and holds Number as a
canonical decimal String (Eq/Hash-clean, exact on large ints).
- Three typing entries converging on CellValue (semantic equality):
type_cell_value (NEW JSON; String stays Str verbatim), type_cell_scalar
(OLD dict tokens; quoted stays Str, else true/false/null/number coerce),
type_csv_token (csv on both engine encodings; stays Str, empty -> Null).
- Hand-rolled RFC 4180 parse_csv_rows (Rust port of report.html's
parseCsvRows, #66), plus parse_block_dict_rows + parse_inline_flow_row
for the OLD-side YAML; quote-stripping reuses unit_test_yaml::parse_yaml_scalar.
- Normalizers table_from_manifest_rows (absorbs the dbt-core
array-of-string-dicts vs dbt-fusion raw-csv-string divergence) and
table_from_yaml_fragment, both terminating in the same canonicalization
so format-only / engine-only differences yield an EQUAL table.
Cross-module: unit_test_yaml::parse_yaml_scalar -> pub(crate).
Gates (exclusions.md discipline): unit_test_table.rs added to
.cargo/mutants.toml examine_globs + a crap4rs.toml strict override
(threshold 15). The CSV/dict parsers were decomposed (CsvScanner /
BlockDictAcc) so every function lands under the strict CRAP threshold.
30 inline tests cover the typing chokepoint (canonicalizer arithmetic +
large-int precision, the three typing branches), the RFC 4180 edge cases
(mirroring tests/headless_csv_parser.rs), the block-dict + inline-flow
parsers, the cross-source / cross-engine equal-table guarantee, the
sql-opaque None case, and serde wire-shape round-trips.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… align + per-cell before/after) The structured-table sibling of the #96 inline YAML line diff. New src/domain/cell_diff.rs adds the cell-level data-table diff PODs (UnitTestDataDiff / NamedTableDiff / FixtureTableDiff / DiffColumn / ColumnStatus / RowChange / RowChangeKind / CellChange), the pure diff_fixture_tables, and the reconstruct_table_diffs driver. Row alignment: multiset key matching + positional residual pairing (NOT LCS). Both reorder requirements are pinned — a pure reorder is a non-event (all-Unchanged, has_real_change false → #96 yaml_diff fallback) and a rotation does not mis-render as Remove+Add (the LCS failure mode). NEW rows come from the CURRENT manifest; OLD rows are diff-sourced — the test's complete pre-edit YAML block (Context + Removed lines of the #96 reconstruction) is projected, then the given/expect rows: sub-regions are sliced out by indentation and parsed via the File-1 IR. - pr_diff.rs: add pub(crate) block_diff_for wrapping reconstruct_one so the table diff reuses the #96 splice (no duplication / silent drift). - 28 inline pure-diff tests (format-only=no-diff, single-cell, add / remove, reorder, two simultaneous edits paired correctly, duplicates, column add/remove, empty edges) + 9 tests/cell_table_diff.rs reconstruction-path tests (block-dict / inline-flow / csv keystone + sql-opaque / stale / untouched / absent / whitespace-only gating). - Wire-shape: exact-token enum tests + full POD serde round-trip. - Gate registration: cell_diff.rs into mutants.toml examine_globs + --test cell_table_diff into additional_cargo_test_args + a crap4rs strict-15 override. - Tracks the empty-csv typing divergence as cute-dbt#124 (a Str("") vs Null edge on the empty cell; a pinned test documents current behavior). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t branch) The given-side h-tests all leave expect Null, so build_data_diff's expect branch + the slice_named_region(_, "expect", None) -> data.expect = Some(...) path was unexercised end-to-end — exactly the kind of branch a mutation survivor hides in, now that cell_diff.rs is in the examine_globs strict-kill set. Add expect_side_single_cell_edit_ populates_data_expect: an `expect:` block-style dict whose count cell is edited 5 -> 7 reconstructs the OLD expect table from Context + Removed lines and populates data.expect with one Modified cell, the sibling label staying Context-unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cargo-mutants over the two #98 strict-gated modules surfaced survivors clustered in the boundary predicates the cell-diff/IR depend on for never-silently-miss-a-changed-cell correctness. All RED-killed with discriminating-input tests; no equivalents, so exclude_re stays empty. unit_test_table.rs: - is_quoted: a one-sided leading quote is NOT a matching pair (a7b) — kills the two `&&`→`||` flips that would strip a half-quoted token and collapse two distinct cell values to one. - CsvScanner::feed_unquoted: a `"` opens a quote ONLY at field start (g26b mid-field literal) + a lone CR terminates one row, not a CRLF pair (g26c) — kills the match-guard→true and the CRLF `&&`→`||`. - BlockDictAcc::feed_line: a `#` comment line is skipped, never a field (g28a) + field attribution uses leading-indent width, not line length (g28b) — kills the skip-guard `||`→`&&` and the indent `-`→`+`. These two masqueraded as timeouts under -j4 load; a clean -j1 rerun exposed them as genuine survivors. cell_diff.rs: - table_diff_if_changed: a one-sided-None (OLD sql/absent, NEW present — or vice versa) still diffs; only BOTH-None skips (table_diff_if_changed_one_sided_none_still_diffs) — kills the `&&`→`||` that would silently drop a whole table change. - unquote: strips only a matching surrounding quote pair, every branch pinned (unquote_strips_only_a_matching_surrounding_pair) + an end-to-end quoted-`input:` slicer match (slice_given_region_matches_a_quoted_input_value). The 13 unquote mutants all showed as -j4 timeouts; the -j1 rerun proved them MISSED. The new modules + strict CRAP/mutants overrides were already registered in .cargo/mutants.toml examine_globs + crap4rs.toml at 9e3cdc6 (the build commits); this commit adds the kill tests that give those gates teeth. Genuine non-termination mutants (feed*->0, scan_csv_matrix +=→*=) remain caught-by-timeout — the timeout IS the detection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…p twin A clean -j1 rerun (the -j4 full run had mislabeled these under load) surfaced two more cute-dbt#98 survivors in unit_test_table::dedent that the concurrent run buried — both `replace - with +` in the indent arithmetic: - 830:26 (the `min_indent` computation): RED-killed by dedent_strips_only_the_common_prefix_and_preserves_deeper_indent — a non-uniform region (header at indent 2, data at indent 4) where the inflated min_indent would over-dedent the deeper line. Discriminating. - 839:43 (the per-line slice-start clamp `min_indent.min(l.len() - l.trim_start().len())`): classified EQUIVALENT. `min_indent` is the minimum leading-ws over all non-blank lines, so every line's own indent is >= min_indent and the `.min()` always returns min_indent; the `+` mutation makes the clamped term larger still, so `.min()` still returns min_indent — byte-identical output for every input. The clamp only guards a slice-overflow panic that min_indent's definition already precludes. No discriminating test is constructible. Excluded via a LINE-PINNED exclude_re (`:839:`) in .cargo/mutants.toml so only the equivalent clamp mutant is dropped — the sibling 830 mutant shares the same mutant *name* but stays gated and killed. tracked: cute-dbt#129 — equivalent dedent clamp mutant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…king ref
Adds e_edit_plus_reorder_of_same_rows_stays_localized_documented_boundary:
the one documented v1 limitation (two rows BOTH edited AND swapping order
land crossed in the positional residual zip). Pins the load-bearing
invariant that survives the boundary — the diff stays LOCALIZED Modified
rows, never a false all-changed blowup or a panic — so a regression that
turns it into all-Added/all-Removed fails.
Also corrects cross_source_empty_csv_cell_divergence_is_documented's
assertion message: the greppable `tracked:` ref pointed at the CLOSED,
unrelated cute-dbt#85 (the --scope-from-pr-diff flag); the live tracking
issue for the empty-csv Str("")-vs-Null divergence is cute-dbt#124, which
the test docstring already cites. exclusions.md greppability hygiene.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…no-op (fusion-grounded) DELTA 1 — type_csv_token gains fusion's warehouse-numeric parse ladder (empty->Null; canonicalize_str_number->Number; case-insensitive true/false->Bool; else Str verbatim). canonicalize_str_number is unchanged (i128-first, strictly more exact than fusion's f64; we do NOT mirror fusion's Number::to_string()). DELTA 2 — the core-csv Value::Array path is now format-aware: a type_fn is threaded into table_from_value_objects (mirroring table_from_keyed_rows). format==csv routes string cells through a type_csv_value shim (-> type_csv_token); format==dict keeps type_cell_value verbatim, so a deliberately-quoted dict '1' stays Str while a csv 1 infers Number — format is the only discriminator. Result: dbt-core's array-of-string-dicts and dbt-fusion's raw-string body converge to the same typed table. Closes #124 (empty-csv cell typing divergence): the core-csv empty cell now maps ""->Null via type_csv_token, matching the OLD-YAML csv path, so the cross-source divergence test flips to assert zero-diff (id->Number, empty->Null both sides). The tracked: note is removed. Tests: 13-case dict<->csv equality matrix; flipped a4 (split into the dict-path Str guarantee + a4b csv Number/Bool inference); format discriminator ([{"id":"1"}] under dict->Str vs csv->Number); explicit mutation-kill tests per new branch (number, each bool arm via eq_ignore_ascii_case, empty-Null, csv-Array routing, non-string fallback) — verified RED under neutralized mutants. New committed synthetic fixture fusion-csv-raw-string.json (fusion's rows-as-raw-CSV- string encoding) + MANIFEST.toml registration, exercised end-to-end through FileManifestSource.load -> the IR (closes the spine's tracked E2E gap). 3 documented divergences (literal "null" text stays Str; ragged rows tolerated; i128 vs f64 reach) added as rustdoc + retained for the PR body. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ll diffs
Adversarial-verify gap: the dict<->csv equivalence acceptance was proven
only in the no-op direction (format-only reformat => has_real_change()==false).
The complementary clause — a GENUINE value change on the new csv-inference
path must STILL diff — was reasoned from reading has_real_change(), not
pinned by a test. Add one: two csv tables differing in one inferred numeric
cell ("1" vs "1.5", not "1" vs "alice") so it guards the
canonicalize_str_number path specifically, asserting Number("1") !=
Number("1.5") survives diff_fixture_tables(...).has_real_change()==true.
Test-only; no behavior change. 635 tests green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d + external-fixture guard Workflow-2 render plumbing for the cell-level unit-test data-table diff: - TestPayload.data_diff: Option<UnitTestDataDiff> (skip_serializing_if, mirrors yaml_diff/sql_diff). Set in build_test_payload from a new data_diffs map threaded render_report -> build_payload -> build_model_payload -> build_test_payload (all call sites incl. the empty-map test sites). - cli::execute reconstructs data_diffs via reconstruct_table_diffs on the PrDiff arm (empty map on Baseline), reusing the refined `changed` set + the authoring_yaml block map exactly like reconstruct_block_diffs, and threads it through render(...). - External-fixture guard (#126): UnitTestGiven/UnitTestExpect gain fixture: Option<String> (+ #[serde(default)] on rows so a reference-only external fixture with rows: null still deserializes). GivenPayload / ExpectedPayload surface `fixture` (skip_serializing_if). The signal the JS turns into a "data in external fixture file" affordance + YAML fallback is rows == null && fixture != null — carried on the payload, not inside UnitTestDataDiff (an external given produces no NamedTableDiff). Tests: serde wire-shape (adjacent {t,v} CellValue tags, absent => no "v", lowercase enum tokens), data_diff present/absent omission, external-fixture given (null rows + fixture name) vs inline expect (fixture omitted), and domain fixture-field deserialization. Golden manifest-digest snapshot re-accepted (additive "fixture": null only). Baseline-mode report chrome stays byte-stable; zero-egress BDD + resource-ref lint green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Diff toggle Render UnitTestDataDiff into the given/expect DataTables in the test-details surface, mirroring the #96 Authored↔Diff drawer idiom for a per-table Current↔Diff toggle. - buildFixtureView(cols, rows, cls, diff): no diff → the historic buildTable output byte-for-byte (baseline-mode reports never drift); a diff → a .cell-diff-toggle (Diff default, Current secondary) flipping exactly one view visible at a time via the #121 [hidden]{display:none!important} rule. - buildDiffTable / buildDiffCell / cellValueText render the adjacently-tagged {t,v} CellValue: a changed Modified cell shows inline old → new (tinted reusing the #96 strong-mark palette); Unchanged/Added show the new value, Removed the old; rows tinted by RowChangeKind; DiffColumn add/remove badged. - Per-table default is Diff iff that table's own entry exists in data_diff (given[] keyed by input, or expect) — the domain already gated on has_real_change(), so presence is the signal; defaults can differ within one test. - External-fixture guard (#126): fixture set + rows null → "data in external fixture file: <name>" affordance + a pointer to the Authoring YAML drawer, never a silently-empty grid. - DataTables: initDataTablesIn skips hidden tables (offsetParent === null) and the toggle lazy-inits + columns.adjust() the revealed view, so a table initialized while display:none never mis-measures column widths. Regenerated both example reports + accepted the render_integration chrome snapshot (inline JS/CSS shifted). Zero-egress intact (headless_zero_egress + resource-ref lint green); existing #91/#96/#111/#121 toggles unregressed (headless_toggle 7/7 under real Chrome). The new toggle's own headless test is the explicit next stage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ly=no-diff) + regenerate examples
Prove the cute-dbt#98 cell-level data-table diff end to end now that the
domain + render plumbing + template toggle are in place.
Headless (tests/headless_toggle.rs, real Chromium):
- cell_diff_toggle_defaults_to_diff_and_shows_exactly_one_view — the
Current↔Diff cell-diff toggle defaults to Diff and shows EXACTLY one of
{Diff, Current} (offsetHeight, the #121 oracle); clicking Current flips
the body and drops the inline old→new cell. Proven RED-first by
transiently removing the Current view's initial `hidden` (both grids
rendered, the exactly-one count failed), then restored.
- fusion_csv_format_only_shows_no_diff_cell_but_value_change_shows_old_to_new
— the centerpiece, over the committed fusion-csv-raw-string fixture and
driven through reconstruct_table_diffs: a format-only OLD reformat
(1 vs 1.00, false vs TRUE — value-inference converges) emits NO entry, so
the grid is the plain Current view (no .cell-diff-toggle, no .cell-old); a
genuine value change (9 → 1) emits an entry whose Diff view tints the cell
inline old→new.
BDD (features/cell_table_diff.feature + tests/steps/cell_table_diff.rs, a
self-contained row-bearing synthesizer that runs the real --pr-diff
subprocess and asserts on the test["data_diff"] payload): a changed cell
attaches a data diff (old→new); a format-only reformat attaches NONE; a
row add/remove attaches an Added/Removed row; the changed table is
foregrounded with a Modified row. feature-count 10→11 in ci.yml +
lefthook.yml (both mirrors).
headless_zero_egress stays green (no new network/asset/module). Examples +
insta snapshots unchanged (the template toggle landed in a prior commit;
the manifest-digest + DOM-skeleton snapshots are unmoved).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 19 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR implements cell-level fixture table diffs for unit-test PR reviews (cute-dbt#98). It adds a canonical fixture representation with format-aware cell typing, a deterministic diff algorithm with PR-diff reconstruction, and extends the report pipeline to emit and render per-test cell diffs with interactive Current↔Diff toggles in the PR report UI. ChangesCell-level fixture diffing and rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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 26867445705 -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 introduces a cell-level unit-test data-table diff capability (cute-dbt#98), adding new domain modules for cell-diffing and unit-test table normalization, alongside UI toggle support in the HTML report and extensive integration tests. The review feedback highlights a critical bug in float canonicalization where trimming trailing zeros can corrupt scientific notation values, and recommends replacing several instances of split('\n') with lines() to ensure robust handling of CRLF line endings and idiomatic line counting.
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.
Contributor
Author
Visual demo — Current↔Diff cell-level data-table diff (#98)A best-effort look at the new per-table cell-diff toggle, rendered headlessly over the committed Diff view (default) — the changed cell tints inline old → new (
Current view (after clicking the toggle) — the inline diff disappears, the plain Current grid shows
The live-DOM checks behind these shots: default active view = Regenerate locallyThe real proof lives in the headless suite — no demo scaffolding needed: CHROME="/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" \
cargo test --test headless_toggle \
fusion_csv_format_only_shows_no_diff_cell_but_value_change_shows_old_to_new -- --ignored --nocaptureThat test renders the same fusion-csv reports through Screenshots are static PNGs on the throwaway |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/domain/unit_test_yaml.rs (1)
292-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t reuse
parse_yaml_scalaras a general quoted-value parser.This helper still stops at the first matching quote and ignores YAML escapes/doubled quotes, so making it
pub(crate)lets valid fixture values like"a\"b"or'it''s'get truncated whenunit_test_tablecalls it. Either keep it local to test-name parsing or harden it before sharing it across fixture parsing paths.🤖 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_yaml.rs` around lines 292 - 315, The function parse_yaml_scalar is exported pub(crate) but incorrectly stops at the first matching quote and doesn't handle YAML escapes/doubled quotes, so either revert it to a local/private helper used only for test-name parsing or harden it before sharing: update parse_yaml_scalar to detect quoted scalars starting with '"' or '\'' and then iterate character-by-character consuming escaped characters (backslash escapes for double quotes) and doubled single-quotes for single-quoted scalars, only terminating on a real closing quote, unescape the content into the returned String, and keep its signature so callers like unit_test_table::parse_block_dict_rows get correct unescaped values.src/domain/unit_test.rs (1)
284-341: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd property-based serde coverage for the new
fixturewire shapes.The new
fixture/rowspermutations are only pinned by a few examples. Please add property tests that round-tripUnitTestGivenandUnitTestExpectacross{fixture present/absent} × {rows missing/null/inline}so the tolerant-deserialization contract can’t drift silently.As per coding guidelines
src/{domain,adapters}/**/*.rs: “All domain and adapter code must include property tests for JSON serde round-trip and StateComparator union semantics.”🤖 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.rs` around lines 284 - 341, Add property-based serde round-trip tests covering all combinations of fixture present/absent and rows missing/null/inline for UnitTestGiven and UnitTestExpect: implement two new proptest (or quickcheck) tests (e.g., property_given_fixture_rows_roundtrip and property_expect_fixture_rows_roundtrip) that generate either Some(fixture)/None and rows as Missing (omit the field), Null (Value::Null), or Inline (an array/object Value), construct the corresponding UnitTestGiven/UnitTestExpect (or build JSON map and deserialize), serialize with serde_json::to_string and deserialize with serde_json::from_str, and assert equality (back == original); reference sample_given/sample_expect for shape examples and add tests into src/domain/unit_test.rs alongside the existing unit tests so the tolerant-deserialization contract cannot regress.
🧹 Nitpick comments (1)
tests/steps/cell_table_diff.rs (1)
208-223: ⚡ Quick win
format == "csv"never exercises a CSV-authored fixture here.
working_tree_yaml()always serializesrows:as YAML inline maps, so a future"csv"scenario would still go through dict-shaped authored YAML instead of the CSV parsing path this feature is meant to validate. If this helper is intended to support bothdictandcsv, branch the YAML emission byplan.formatand write real CSV text for the CSV case.🤖 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 `@tests/steps/cell_table_diff.rs` around lines 208 - 223, The working_tree_yaml helper always emits rows as YAML maps, so when plan.format == "csv" modify working_tree_yaml to branch on plan.format: for non-csv keep the existing behavior (use row_line for each row and emit YAML maps), but for "csv" emit a proper CSV block—set the format field to "csv" and write rows as raw CSV text (e.g. a block scalar or literal block) using plan.new_rows converted to comma-separated lines instead of calling row_line; ensure indentation matches the surrounding YAML and keep the existing expect/other fields intact.
🤖 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 `@examples/jaffle-shop-report.html`:
- Around line 5299-5308: The current decorateValueCell function collapses cv.t
=== "absent" into the same rendering as null; instead split those cases so
absent is visually distinct: remove "absent" from the combined null check, add a
separate branch for cv.t === "absent" that applies a new class (e.g.,
"cell-absent") and a distinct text/placeholder (e.g., "ABSENT" or an
empty/italic placeholder) while keeping the existing "cell-null" + "NULL"
behavior for cv.t === "null"; update any CSS hooks that rely on
cell-null/cell-absent to style them differently.
In `@examples/playground-report.html`:
- Around line 5299-5304: In decorateValueCell, don't collapse cv.t === "absent"
into the same rendering as a real null: update the conditional so only cv.t ===
"null" (or falsy null-equivalent) adds the "cell-null" class and "NULL" text;
handle "absent" separately by rendering it as an empty cell (or delegating to
cellValueText(cv)) so added/removed-column and sparse-row semantics remain
distinct; locate this logic inside function decorateValueCell and adjust the
branch that currently checks cv.t === "null" || cv.t === "absent".
In `@src/adapters/render.rs`:
- Around line 2577-2755: Add a proptest JSON round-trip that exercises the new
optional/nested payload shapes: create a property test (e.g., proptest fn
payload_json_roundtrip) that generates arbitrary combinations of
UnitTest/UnitTestGiven (with fixture Some/None and rows null vs array),
UnitTestDataDiff present/None, and ReportPayload/build_test_payload outputs; for
each generated payload serialize to JSON and deserialize back and assert
equality (and also assert the specific wire-shape invariants: fixture present
implies rows == null, absent fixture omits the key, data_diff Some appears in
JSON and None omits it, and CellValue adjacent tagging remains). Locate and
extend the existing tests near
data_diff_payload_wire_shape_has_adjacent_cellvalue_tags and
build_test_payload_* to add this proptest so the serde round-trip and
omitted-vs-null semantics are property-checked for build_test_payload,
UnitTestDataDiff, and ReportPayload.
In `@src/domain/cell_diff.rs`:
- Around line 404-423: The current row_key() builds keys by joining
cell_key_token() outputs with '|' which can collide when cell strings contain
'|' or token-like prefixes; update row_key to produce an injective encoding
(e.g., length-prefix each token: for each token produced by cell_key_token(v)
prefix with its byte-length and a separator or use a structured tuple/Vec<byte>
+ hashless serialization) so concatenation is unambiguous, and ensure
cell_key_token stays the canonical token generator. Update any callers relying
on row_key (e.g., match_multiset) to use the new encoding and add a regression
test that constructs rows containing '|' and token-like strings to verify diffs
(Unchanged/Added/Removed) remain correct.
In `@src/domain/unit_test_table.rs`:
- Around line 712-738: split_quote_aware currently treats the first matching
quote as closing even when it is escaped (e.g. \" inside double quotes or two
single quotes '' inside single-quoted values), so modify split_quote_aware to
recognize and skip escaped quotes: iterate using a peekable iterator (or index)
so when inside a quote you check for backslash escapes (if quote is '"' and
previous char is '\' or current char is '\' then consume next char as literal)
and also handle SQL/YAML-style doubled single-quote escapes (if quote is '\''
and next char is '\'' consume both as one literal and remain in quoted state);
ensure you only clear quote when you see an unescaped matching quote, and keep
using cur, quote and parts as before.
- Around line 60-63: The domain module is performing wire-format parsing (uses
serde_json::Value and parse_yaml_scalar) which violates the domain-layer rule;
remove serde_json::Value and any parsing from src/domain/unit_test_table.rs and
instead accept plain owned, normalized inputs (e.g., String, Vec<Vec<String>>,
Option<String> or a dedicated FixtureTableRow/Cell types) in the FixtureTable
constructors and methods (reference: FixtureTable, any constructors or methods
that currently take serde_json::Value or call parse_yaml_scalar). Move
JSON/CSV/YAML normalization into the adapter layer (where manifests are parsed)
so FixtureTable only consumes already-normalized data; drop the import of
parse_yaml_scalar and update all callers to perform normalization before
constructing/using FixtureTable.
In `@templates/report.html`:
- Around line 1114-1117: The function givenDataDiff currently picks a diff by
matching g.input only (function givenDataDiff(dataDiff, input)), which
incorrectly collapses duplicate given entries; change the lookup to carry and
use a stable per-given identity (e.g., an ordinal or id on each given) rather
than the input text. Update the API of givenDataDiff (and callers) to accept a
secondary identifier (like givenIndex or givenId) and locate the entry via
dataDiff.given by that ordinal/id (or use find by matched id field on g instead
of g.input) so duplicate ref(...) givens resolve to the correct diff; keep
references to givenDataDiff, dataDiff.given and g.input when making the change.
- Around line 1672-1681: The renderer collapse in decorateValueCell incorrectly
treats absent cells the same as null; update decorateValueCell so it only maps
cv.t === "null" to the "cell-null" class and "NULL" text, and handle cv.t ===
"absent" specially (e.g., add a distinct "cell-absent" class and a visible
"ABSENT" label or other sentinel) so Absent and Null remain visually distinct;
modify the branches in decorateValueCell (and any call-sites that rely on
cell-null styling) to use cv.t checks for "null" vs "absent" and ensure number
handling (cv.t === "number") and default cellValueText(cv) behavior remain
unchanged.
In `@tests/steps/cell_table_diff.rs`:
- Around line 181-185: After loading the generated report into
world.report_html, call the static check helper common::assert_no_external_refs
on that HTML before finishing the step; specifically, immediately after setting
world.report_html = std::fs::read_to_string(&out).ok() invoke
common::assert_no_external_refs(world.report_html.as_deref().unwrap_or_default())
(or the appropriate Option-aware call) so the BDD step validates there are no
external asset references in the rendered HTML.
---
Outside diff comments:
In `@src/domain/unit_test_yaml.rs`:
- Around line 292-315: The function parse_yaml_scalar is exported pub(crate) but
incorrectly stops at the first matching quote and doesn't handle YAML
escapes/doubled quotes, so either revert it to a local/private helper used only
for test-name parsing or harden it before sharing: update parse_yaml_scalar to
detect quoted scalars starting with '"' or '\'' and then iterate
character-by-character consuming escaped characters (backslash escapes for
double quotes) and doubled single-quotes for single-quoted scalars, only
terminating on a real closing quote, unescape the content into the returned
String, and keep its signature so callers like
unit_test_table::parse_block_dict_rows get correct unescaped values.
In `@src/domain/unit_test.rs`:
- Around line 284-341: Add property-based serde round-trip tests covering all
combinations of fixture present/absent and rows missing/null/inline for
UnitTestGiven and UnitTestExpect: implement two new proptest (or quickcheck)
tests (e.g., property_given_fixture_rows_roundtrip and
property_expect_fixture_rows_roundtrip) that generate either Some(fixture)/None
and rows as Missing (omit the field), Null (Value::Null), or Inline (an
array/object Value), construct the corresponding UnitTestGiven/UnitTestExpect
(or build JSON map and deserialize), serialize with serde_json::to_string and
deserialize with serde_json::from_str, and assert equality (back == original);
reference sample_given/sample_expect for shape examples and add tests into
src/domain/unit_test.rs alongside the existing unit tests so the
tolerant-deserialization contract cannot regress.
---
Nitpick comments:
In `@tests/steps/cell_table_diff.rs`:
- Around line 208-223: The working_tree_yaml helper always emits rows as YAML
maps, so when plan.format == "csv" modify working_tree_yaml to branch on
plan.format: for non-csv keep the existing behavior (use row_line for each row
and emit YAML maps), but for "csv" emit a proper CSV block—set the format field
to "csv" and write rows as raw CSV text (e.g. a block scalar or literal block)
using plan.new_rows converted to comma-separated lines instead of calling
row_line; ensure indentation matches the surrounding YAML and keep the existing
expect/other fields intact.
🪄 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: ab32440a-1b7d-4f79-881c-7e4e18bb4f13
⛔ Files ignored due to path filters (2)
tests/snapshots/manifest_ingestion__golden_baseline_manifest_digest.snapis excluded by!**/*.snaptests/snapshots/render_integration__rendered_chrome_jaffle_shop.snapis excluded by!**/*.snap
📒 Files selected for processing (31)
.cargo/mutants.toml.github/workflows/ci.ymlcrap4rs.tomlexamples/jaffle-shop-report.htmlexamples/playground-report.htmlfeatures/cell_table_diff.featurelefthook.ymlsrc/adapters/render.rssrc/cli/mod.rssrc/domain/cell_diff.rssrc/domain/manifest.rssrc/domain/mod.rssrc/domain/pr_diff.rssrc/domain/preflight.rssrc/domain/scope.rssrc/domain/state.rssrc/domain/unit_test.rssrc/domain/unit_test_table.rssrc/domain/unit_test_yaml.rstemplates/report.htmltests/cell_table_diff.rstests/fixtures/MANIFEST.tomltests/fixtures/fusion-csv-raw-string.jsontests/golden_report.rstests/headless_toggle.rstests/render_integration.rstests/steps/builders.rstests/steps/cell_table_diff.rstests/steps/diff_scoping.rstests/steps/mod.rstests/steps/world.rs
4 tasks
…ize_float invariant test Disposition of Gemini + CodeRabbit review on PR #130 (cell-level data-table diff). Verified each finding against source; applied real fixes, kept all gates green. Gemini: - canonicalize_float "scientific notation" CRITICAL: false positive (Rust f64 Display never emits an exponent). Pinned the invariant with a defensive unit test over extreme magnitudes instead of changing the logic. - split('\n') -> lines() in slice_named_region (cell_diff.rs), parse_block_dict_rows (unit_test_table.rs), and the block_at test helper (tests/cell_table_diff.rs) for CRLF-robustness. No raw literal carries a trailing newline, so the block_end count is unchanged. CodeRabbit: - row_key collision (cell_diff.rs): the `|`-join was non-injective — a literal cell containing `|`/token-prefix could collapse distinct rows onto one key and drop a real diff. Switched to a length-prefixed injective encoding + two regression tests. - Absent-vs-Null in the diff grid: decorateValueCell collapsed Absent into NULL. Split the branches (Absent -> blank .cell-absent) at the template source; regenerated both example HTMLs + the insta chrome snapshot. - split_quote_aware / parse_yaml_scalar escaped quotes: both stopped at the first quote, truncating values like 'it''s' or "a\"b". Hardened both (doubled single-quote + backslash escape) + regression tests. - zero-egress: the cell-diff BDD step now runs common::assert_no_external_refs on the rendered report. - property serde round-trips: added combinatorial round-trip tests over the new fixture/rows wire shapes (unit_test.rs) and the data_diff CellValue x expect matrix (cell_diff.rs). Responded-as-intentional (not changed): - "Keep manifest/wire parsing out of src/domain": serde_json::Value is the documented normalized-input boundary (ADR-5) used across 9 domain modules; no gate restricts it; AGENTS.md lists "no AST-purity grep" as a conscious simplification. Deferred (tracking issue): - givenDataDiff duplicate same-ref given ambiguity — needs a per-given ordinal threaded through the payload (domain+adapter+template); filed cute-dbt#131. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Bot-review disposition for PR #130 — all findings verified against source; fixes in 09bb165. Full fast-gate set green (fmt, clippy --locked -D warnings, nextest 651 passed, BDD 79 scenarios / 497 steps, cargo doc -D warnings). CodeRabbit outside-diff comments (no inline thread — addressed here)
CodeRabbit nitpick
Responded-as-intentional"Keep manifest/wire parsing out of Deferred (tracking issue)
|
Closed
5 tasks
cmbays
added a commit
that referenced
this pull request
Jun 8, 2026
* fix(render): bind cell diff by per-given source ordinal, not input text The template's `givenDataDiff` matched a given's cell diff by `input` text and took the first hit — so two `given:` blocks against the same `ref(...)` both resolved to the first diff, and the second given-section rendered the wrong cell diff (CodeRabbit, PR #130). Identity is now the given's **source ordinal** (its position in the test's `given:` list), carried on `NamedTableDiff`: - `cell_diff::build_data_diff` tags each emitted `NamedTableDiff` with its source ordinal via `enumerate()` — robust to the fact that `UnitTestDataDiff::given` is *filtered* to changed givens, so a dense index would not align with the full given list the renderer iterates. - the template binds by ordinal: `renderAllInputs` passes the forEach index; the bound-CTE path recovers the source position via `t.given.indexOf(g)` (the `bound` list is a filtered view, so its own index is not the ordinal). Regression: `data_diff_ordinal_is_source_position_not_push_index` — an earlier *unchanged* given must not shift a later changed given's ordinal. Refs #131 (remaining: same-ref headless regression, examples/snapshot regen). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(render): same-ref per-given headless regression + mutants re-pin Completes the #131 tail on top of 7618c3d (the per-given-ordinal core fix). - tests/headless_toggle.rs: the END-TO-END guard for the template binding the bug actually lived in. `same_ref_givens_each_render_their_own_cell_diff` renders two `given:` blocks against the SAME `ref('a')`, both with a real (distinct) cell change, and asserts each given-section shows ITS OWN diff (section 0 -> 111, section 1 -> 222) — pre-#131 both resolved to the first `ref('a')` match. Also adds the now-required `ordinal: 0` to the three existing single-given `NamedTableDiff` constructors: the core commit added a non-defaulted field to the struct, which left the integration-test target non-compiling (only `--lib` was validated there). - .cargo/mutants.toml: re-pin the classified-equivalent dedent-clamp exclusion 839 -> 1532 (the `dedent` fn moved down the file); the discriminating sibling stays gated at 1523. Verified via `cargo mutants --list`. #129 stays OPEN as the exclusion tracker; #128 closed as a literal duplicate of it. - examples/*.html + the jaffle-shop render snapshot: regenerate for the inlined template-JS change (the fix lives in the report's own `<script>`, inlined into every report). Pure JS-binding change, zero data-payload drift; no example carries a given-side cell diff so none gains an `ordinal` data key. Refs #131. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> 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
Adds the cell-level unit-test data-table diff — the capstone of cute-dbt's PR-review (
--pr-diff) mode. When a unit test'sgiven/expectdata changes, the report now shows exactly which cells changed, inlineold → new, inside the existing DataTables grids, with a per-tableCurrent ↔ Difftoggle (just-the-result vs the before/after diff). The diff is semantic and aligned with dbt-fusion's type handling: a formatting-only change (e.g. the same values reformatted dict↔csv) shows no diff, while a real value change shows the before/after.Built overnight as an autonomous orchestration (multi-agent workflows, each TDD'd + verified). The scope behavior (a changed test brings its model into scope; a changed model with no test change stays in scope with no updated test shown) was already shipped in #91/#96 — this PR adds the structured cell diff on top.
Closes #98
Closes #127
Closes #124
What's in it (layered)
src/domain/unit_test_table.rs) —given/expectparsed intorows × typed cells, normalizing dict (native JSON types), csv (dbt-core array-of-string-dicts and dbt-fusion raw-CSV-string), and sql (opaque → falls back to the feature: block-precise PrDiff updated-test detection + inline YAML diff #96 YAML text diff).CellValueis a canonical,Eq-clean value (numbers are canonical decimal strings, notf64).src/domain/cell_diff.rs) —oldrows reconstructed from the PR diff's removed lines (via the feature: block-precise PrDiff updated-test detection + inline YAML diff #96 machinery, no base manifest),newfrom the current manifest; per-cell before/after with row add/remove and column add/remove. Mutation-gated (21 new survivors killed; 100% addressed)."100"→number,"true"→bool,""→null) so a dict↔csv reformat of equal values is a no-op, matching what fusion actually does. I read the dbt-fusion source and dogfooded it (ran real unit tests in both forms) to confirm: fusion compiles dict100and csv"100"to the identicalCAST(100 AS integer); the cast type comes from fusion's inferred schema (not the value, not the manifest), so cute-dbt models value equality rather than trying to reproduce the cast type. cute-dbt's existing number canonicalization already matches fusion's warehouse-numeric equality and is better on> i64integers.render.rs+templates/report.html) — the cell diff renders into the DataTables cells; the per-tableCurrent ↔ Difftoggle reuses the[hidden]{display:none!important}fix from #121 — fix: diff drawers render exactly one view + relabel toggle button to "File" #122 (the Sakurapre{display:block}trap) and is covered by a RED-first exactly-one-view-visible headless test.given/expectsourced from an externalfixture:file (no inlinerowsin the manifest) renders a "data in external fixture file" affordance instead of a silently-empty grid.Tests / gates (all green locally)
cargo fmt·clippy --all-targets --locked -D warnings·nextest(642+) ·render_integration·headless_toggle --ignored(real Chromium: one-view-visible + default-Diff + the centerpiece — over a fusion-csv-raw-string fixture, format-only ⇒ no diff vs value change ⇒ old→new) ·headless_zero_egress --ignored(zero-egress intact) ·bdd(74 scenarios / 477 steps) ·cargo doc -D warnings·deny· coverage ≥ 85 · both committed examples byte-stable. New fusion-csv-raw-string fixture added + registered (synthetic-only), closing the spine's tracked E2E gap.cargo mutantsandcrap4rsrun in CI only (cargo-mutants can't run in this repo's local sandbox —tests/fixture_manifest_listed.rsshellsgit ls-files, which errors in the mutants temp-copy; documented). New-branch mutation coverage rests on explicit hand-written kill-tests, two of which I empirically verified (mutate → RED → revert).Honest caveats / things to know
old → newin-cell, GitHub-like. Easily swapped for strikethrough / split-column / hover-reveal if you'd prefer — say the word."null"/"NULL"stays a string (fusion coerces it to SQL NULL — but a diff cell must be able to show the text "null"); ragged csv rows are tolerated (fusion errors); csv uses i128 (exact to ~1.7e38) while dict ints> u64::MAXfall to f64.cell_diff.rs). Row alignment uses multiset-key-match + positional residual (a sound deviation from the originally-designed LCS).fixture:with norows:key, handled by#[serde(default)]). The compiled-manifestrowsvalue for an external fixture is unconfirmed (no real fixture exists and dbt isn't run in CI) — the guard readsrows == nullfaithfully; if a real compiled fixture ever encodes it asrows: [], the predicate needs a tweak. Tracked by the follow-up.amount: "100") still diffs vs csv100— a deliberate, safe false-positive (it never hides a real change).Follow-ups (filed, out of scope here)
overrides(env_vars/macros/vars) changes in the YAML text-diff fallback #125 — surface unit-testoverrides(env_vars/macros/vars) changes via the YAML text-diff fallback.Reviewing the UX
The committed example reports are baseline-mode (no inline diff), so they don't showcase the toggle. A rendered
--pr-diffdemo report showing the cell-diff toggle is attached in a follow-up comment.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation