Repository navigation
#111 — feat: inline SQL diff for changed models in PR-review report - #113
Conversation
Apply the #96 line-diff substrate to a changed model's raw SQL (raw_code), rendered in the Model SQL section with a Raw↔Diff toggle. report-mode, diff-sourced (--pr-diff), no base manifest. - domain/pr_diff: rename YamlBlockDiff -> BlockDiff (content-agnostic; JSON field `yaml_diff` kept so #96 wire/JS/snapshots don't move). Widen block_aligns_with_hunks + reconstruct_one to (raw, start, end). Add reconstruct_model_sql_diffs over manifest raw_code (whole-file span; strip exactly one trailing \n for git's line frame — handles the dbt-core vs dbt-fusion trailing-newline divergence). N7b drift guard reused; stale/untouched/whitespace-only -> plain SQL view. - Whitespace ignored as standard, per line-pair (ws_equal, git --ignore-all-space semantics) across both the YAML (#96) and SQL (#111) diffs. N7b stays whitespace-exact. - render: ModelPayload.sql_diff (skip_serializing_if, mirrors TestPayload.yaml_diff); threaded through build_payload/render_report. PrDiff-arm-gated in cli; Baseline => empty map (digest snapshot unmoved). - template: renderYamlDiff -> renderBlockDiff(diff, highlightFn) with highlightSql/highlightYaml; highlight-bypass preserved. Model SQL Raw↔Diff toggle (default Diff when sql_diff present). - BDD: 4 scenarios appended to pr_diff_scoping.feature (count stays 10); headless SQL-diff toggle tests; book docs (raw_code source, whitespace standard + caveat, engine trailing-newline note). Closes #111 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n set) Close the focused-set DoD on src/domain/pr_diff.rs: - intra_line_span_suffix_stops_at_the_prefix_boundary: asymmetric shared-prefix+suffix cases that kill the 3 suffix-bound mutants (`< -> <=`, `a.len()-prefix -> +`, `b.len()-prefix -> +`). These were pre-existing #96 survivors (intra_line_span is byte-identical to #96); the function is the substrate #111 generalizes, so it's hardened in-PR. The remaining 2 `+= -> *=` survivors are infinite loops, classified equivalent inline. - block_misaligns_when_hunk_claims_a_line_the_block_lacks: covers the out-of-range guard the (raw,start,end) widening exposed, restoring block_aligns_with_hunks to 100% line coverage (CRAP 15.02 -> 15.00, unambiguously within the strict-15 gate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…review follow-ups BLOCKER (review 2026-05-31 — both reviewers reproduced an index-OOB panic): `fold_hunk_edits` looped `0..h.new_len` and indexed `added_lines[k]`/`removed_lines[k]` via `pair_is_ws_only`. `new_len` (from the `@@` range) is independent of `added_lines.len()` (counted `+` bodies), so a DEFAULT `git diff` (3 context lines, parser drops them) yields `new_len > added_lines.len()` and panics — regressing the shared #96 YAML drawer too (reconstruct_one is shared). Fix (both requirements): - No panic: `hunk_is_unified_zero(h)` = `new_len == added_lines.len()` (always true under --unified=0). `reconstruct_one` checks ALL touching hunks are unified-zero; if any isn't, it degrades the whole block to the plain view (all-Context -> has_real_change()==false -> caller shows plain text), consistent with the stale-diff degrade. - No silent mislabel: degrade is whole-block, so uncovered new-side lines are never labeled Added. - RED regression: a default-context git diff STRING end-to-end through parse_diff -> reconstruct_* (tests/changed_files_provider.rs) + domain-level context-bearing-hunk tests (SQL + YAML paths). The #96 malformed-empty-body test now asserts the consistent degrade. Review MINOR/NIT: - BlockSpan<'a> { raw, start, end } borrowed arg helper threaded through block_aligns_with_hunks / reconstruct_one (kills the adjacent-usize swap hazard; not owned/serialized POD). - BDD `model_carries_sql_diff` tightened: asserts removed-immediately- before-added ORDER + line text + surrounding Context. - Renamed test -> block_diff_serializes_to_the_exact_renderblockdiff_contract. - Added model_sql_diff empty-raw_code (`Some("")`) degrade test. - Hardened is_clean_1to1 (`&&`-mutant kill) + intra_line_span suffix-bound mutants + block_aligns out-of-range coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 45 minutes and 57 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. 📝 WalkthroughWalkthroughThis PR implements inline SQL diff rendering for the PR-review report, mirroring the unit-test YAML diff from ChangesInline SQL diff rendering for PR-review
Sequence DiagramsequenceDiagram
participant CLI as cli::execute
participant Reconstruct as reconstruct_model_sql_diffs
participant Render as render_report
participant Payload as ModelPayload
participant Template as renderModelSql
CLI->>Reconstruct: raw_code, hunks, scoped models
Reconstruct->>Reconstruct: hunk-to-BlockSpan alignment check
Reconstruct->>Reconstruct: whitespace-only filter
Reconstruct-->>CLI: HashMap~full_id, BlockDiff~
CLI->>Render: yaml_diffs, sql_diffs
Render->>Payload: sql_diff: Option~BlockDiff~
Render-->>CLI: ReportPayload
Template->>Template: detect m.sql_diff
Template->>Template: render Diff/Raw toggle
Template->>Template: renderBlockDiff(sql_diff, highlightSql)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
📄 Rendered report previewAll examples regenerated cleanly.
Click Download to fetch the rendered HTML. Each artifact Alternative: GitHub CLI# gh CLI >= 2.63 extracts into ./report-preview-playground/.
gh run download 26724330353 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
There was a problem hiding this comment.
Code Review
This pull request introduces an inline SQL diff feature for changed dbt models' raw Jinja source (raw_code), adding a Raw ↔ Diff toggle in the Model SQL section of the report. To support this, the existing inline YAML diff logic was generalized into a content-agnostic BlockDiff structure, and standard whitespace-only change filtering was implemented to ignore re-indentations and trailing whitespace. The review feedback points out an opportunity to simplify the pair_is_ws_only helper function by removing redundant carriage return trimming, as the downstream whitespace comparison already handles these characters.
cute-dbt#111 follow-up (Christopher-approved, same PR): a context-bearing diff silently degrades every affected block to the plain view (the reconstruct_one contract guard). Emit ONE actionable stderr line so a user who forgot --unified=0 isn't left thinking inline diffs are broken. - domain (pure, std-only, no I/O): NormalizedDiffIndex::context_bearing_ hunk_count() counts hunks where new_len != added_lines.len() — exact for "not --unified=0" (insert N==N / delete 0==0 / replace N==N always hold under -U0). Unit-tested (exact count on a mixed diff; 0 on a clean -U0 diff incl. a pure-deletion hunk); 3/3 mutants caught. - cli (the I/O lives here, not in domain): warn_if_not_unified_zero emits once on the PrDiff arm only: "cute-dbt: warning: the supplied diff is not `git diff --unified=0` (N context-bearing hunk(s)); inline diffs are disabled — showing plain views. Re-run the diff with --unified=0 for inline diffs." - integration (tests/changed_files_provider.rs): the note is PRESENT on a default-context diff and ABSENT on a clean --unified=0 diff. Examples + manifest-digest snapshot byte-stable (stderr-only, no payload or template change). Feature-count stays 10. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/domain/mod.rs (1)
53-56: 💤 Low valueReduce public API surface: don’t re-export
ws_equal(unless required externally)
ws_equalis only defined and used insidesrc/domain/pr_diff.rsand is merely re-exported fromsrc/domain/mod.rs; no other non-test code references it (the only test mention is in a comment). Consider making itpub(crate)or removing it from the domain re-exports if downstream consumers don’t rely on it.🤖 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/mod.rs` around lines 53 - 56, The public re-export list in src/domain/mod.rs currently exposes ws_equal even though it is only used internally in pr_diff; remove ws_equal from the pub use list (or change its visibility in src/domain/pr_diff.rs to pub(crate) if internal tests require access) so it is no longer part of the crate's public API; locate the re-export line that lists reconstruct_block_diffs, reconstruct_model_sql_diffs, refine_changed_by_hunks, ws_equal and either delete ws_equal from that list or adjust the ws_equal symbol in pr_diff to pub(crate) and run tests to ensure nothing outside the crate depends on it.
🤖 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.
Nitpick comments:
In `@src/domain/mod.rs`:
- Around line 53-56: The public re-export list in src/domain/mod.rs currently
exposes ws_equal even though it is only used internally in pr_diff; remove
ws_equal from the pub use list (or change its visibility in
src/domain/pr_diff.rs to pub(crate) if internal tests require access) so it is
no longer part of the crate's public API; locate the re-export line that lists
reconstruct_block_diffs, reconstruct_model_sql_diffs, refine_changed_by_hunks,
ws_equal and either delete ws_equal from that list or adjust the ws_equal symbol
in pr_diff to pub(crate) and run tests to ensure nothing outside the crate
depends on it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7d23cb4-89e3-48d3-8263-7e99fb09a0d3
⛔ Files ignored due to path filters (1)
tests/snapshots/render_integration__rendered_chrome_jaffle_shop.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
book/src/how-it-works.mdbook/src/recipes/github-actions-pr-review.mdexamples/jaffle-shop-report.htmlexamples/playground-report.htmlfeatures/pr_diff_scoping.featuresrc/adapters/render.rssrc/cli/mod.rssrc/domain/mod.rssrc/domain/pr_diff.rstemplates/report.htmltests/changed_files_provider.rstests/golden_report.rstests/headless_toggle.rstests/render_integration.rstests/steps/builders.rstests/steps/pr_diff_scoping.rstests/steps/world.rs
…iew) ws_equal compares split_whitespace token sequences, which already ignore a trailing carriage return, so the trim before it was dead. Gemini review finding on PR #113. Behavior-preserving: 543 tests green, clippy --locked clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Adds an inline SQL diff for changed models in
--pr-diff(report) mode, mirroring the inline YAML diff #96 shipped for unit tests. When a model's.sqlsource changed in the PR, its Model SQL section gains a Raw ↔ Diff toggle showing context / removed / added lines with intra-line emphasis. Diff-sourced (no base manifest): the current SQL comes from the manifest'sraw_code, the deltas from the PR diff hunks.Closes #111.
What's inside
YamlBlockDiff → BlockDiff(a content-agnostic line diff; theyaml_diffJSON field name is kept so feature: block-precise PrDiff updated-test detection + inline YAML diff #96's wire shape is unchanged);block_aligns_with_hunks/reconstruct_onewidened from&UnitTestYamlBlockto a borrowedBlockSpan { raw, start, end }. The same machinery now serves the YAML diff (feature: block-precise PrDiff updated-test detection + inline YAML diff #96) and the SQL diff (feature: inline SQL diff for changed models in PR-review report (report-mode, diff-sourced) #111). (A 2D cell/table diff — feature: cell-level data-table diff in PR-review report (report-mode, diff-sourced) #98 — will reuse this substrate but get its own representation; it is not crammed into a line list.)reconstruct_model_sql_diffsover the manifest'sraw_code(whole-file span). No--project-rootneeded — unlike the YAML drawer, dbt stores modelraw_codeverbatim (it normalizesunit_testsinto structured payloads, which is why the YAML diff must read the working tree).raw_codeis byte-verbatim across dbt-core and dbt-fusion, diverging only by a retained trailing newline; normalized withstrip_suffix('\n')so both engines yield an identical diff frame (pinned by a cross-engine test).--unified=0hunk shows only the real change. The N7b drift-guard stays whitespace-exact.The reconstruction is contracted on
git diff --unified=0. A defaultgit diff(3 lines of context) yields hunks wherenew_len > +-body-count — which the new indexing path and the already-shipped #96 YAML drawer (sharedreconstruct_one) would index out of bounds → panic. Both paths now detect a non---unified=0hunk (hunk_is_unified_zero) and degrade the whole block to the plain view (consistent with the existing stale-diff fallback) — never a panic, never a context-as-added mislabel. New regression tests drive a real default-contextgit diffend-to-end through the binary (exit 0 + report written) and exercise both the SQL and YAML reconstruction paths.Review
Built TDD; ran the
/atddgates (CRAP, mutation, architecture); then an independent multi-agent review before opening this PR (code-review, silent-failure, type-design, test-coverage). That review caught the panic above (a BLOCKER) plus several MINOR/NIT items — all addressed in this PR before it opened. The fix diff was read line-by-line.Versioning
No
BREAKING CHANGE— theBlockDiffrename is internal (the lib is internal-only in v0.x) and the SQL diff + whitespace handling are additive ⇒ a v0.x patch.Gates
nextest 541 · bdd 74 scenarios / 477 steps (feature-count stays 10) · headless toggle + zero-egress 7 (real Chromium) · coverage 98.79% lines (
--fail-under-lines 85) · crap4rs PASS ·cargo mutants0-missed on the changed reconstruction code · clippy--all-targets --locked0 · rustdoc-D warnings --document-private-items0 ·cargo denyok ·mdbook buildok · examples + manifest-digest snapshot byte-stable.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests