Repository navigation
#121 — fix: diff drawers render exactly one view + relabel toggle button to "File" - #122
Merged
Merged
Conversation
…tton to "File" The Model SQL diff drawer (#111) and the Authoring YAML diff drawer (#96) each rendered BOTH <pre> views at once in PR-diff mode. The toggle JS sets `hidden` on the inactive <pre>, but the vendored Sakura CSS carries a bare `pre { display: block }` author rule that overrides the UA `[hidden] { display: none }` (author origin beats UA), so the hidden <pre> still rendered. Add `[hidden] { display: none !important; }` to the report <style> block to make the `hidden` attribute authoritative (pure CSS, no new asset — the zero-egress gate is unaffected). Relabel the non-diff toggle button to "File" in both drawers (was "Raw" for SQL, "Authored" for YAML); the `data-view` values and the "Diff" button are unchanged. New headless regression test `diff_drawers_render_exactly_one_view_each` renders a PR-diff report carrying both a model sql_diff and a unit-test yaml_diff, and asserts via `offsetHeight` that exactly one <pre> is visible per drawer (the existing tests asserted only the `.hidden` property, which is `true` either way — which is why the double-render shipped). Confirmed RED-first against the unfixed template. Closes #121 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR fixes diff drawers rendering both views simultaneously by adding an authoritative ChangesDiff Drawer Visibility and Labels
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)examples/jaffle-shop-report.htmlexamples/playground-report.htmlThanks 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 26847396440 -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 fixes an issue where hidden diff or raw code blocks would still render due to Sakura's CSS overriding the default [hidden] behavior. This is resolved by making the hidden attribute authoritative with display: none !important; in the report template. Additionally, the button labels 'Authored' and 'Raw' are standardized to 'File', and a headless integration test is added to verify the toggle visibility. The review feedback suggests minor improvements in the test setup, specifically avoiding an intermediate Vec allocation when creating InScopeSet and using lines().count() instead of split('\n').count() for more robust line counting.
Two test-only cleanups in the cute-dbt#121 headless helper, both from Gemini review on PR #122: - collect tests' ids directly into InScopeSet (FromIterator<String>) instead of routing through an intermediate Vec<String>. - count fixture YAML lines with raw.lines() instead of raw.split('\n') — idiomatic, CRLF-robust, empty-string-correct. No shipped-artifact, template, example, or snapshot impact. Headless toggle suite (7 tests) stays green; clippy --locked + fmt --check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 2, 2026
cmbays
added a commit
that referenced
this pull request
Jun 5, 2026
…-aware, vivid line markers), nested-jinja token-stream highlighter, hunk contraction (#133) * feat(adapters): diff-view visual pass -- cell bold-text, vivid line highlight, type-aware NULL, cool syntax palette cute-dbt#132 checkpoint (approved visual work; N-to-N emphasis + hunk contraction + tested layer + example/snapshot regen still to come): - Cell-grid changed values render as bold red/green TEXT (no fill); the inline old->new is null-aware via a new diffToken helper (a real Null reads italic muted-gray, a string 'null' reads as a normal string -- visually distinct even mid-diff). - YAML/SQL line-diff character emphasis uses a vivid red/green HIGHLIGHT marker (deliberate divergence from the cell-grid bold-text: a syntax-highlighted line needs a marker to stand apart from its own colors -- documented in the CSS so nobody re-unifies them). - Syntax palette reworked to cool/neutral hues (indigo keyword, teal string, violet jinja, graphite comment) that avoid red+green, so the syntax colors never clash with the diff highlight. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(domain): extend inline-diff emphasis to aligned N-to-N replacement hunks cute-dbt#132. Per-character intra-line emphasis previously fired only on a clean 1:1 replacement hunk; an aligned multi-line replacement (removed.len() == added.len()) rendered line-level +/- with no <strong>, so a 2-line YAML/SQL edit showed an inconsistent "some lines highlighted, some not". Generalize `is_clean_1to1` to `is_aligned_replacement` (equal, non-empty counts) and emphasize each removed[i] <-> added[i] pair in `removed_diff_lines` + `fold_hunk_edits`, honoring `pair_is_ws_only` so a re-indent pair stays Context. Unequal-count replacements (e.g. 2->1) still carry no emphasis (no sound positional pairing). Equal, non-empty counts keep every index in bounds, preserving the never-panic-on-a-bad-diff contract. Flips the multi-line pinning test to expect per-pair emphasis + adds a ws-only-pair-in-aligned boundary test; updates two pre-existing tests whose aligned multi-line hunks now correctly carry emphasis. 552 lib tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(adapters): token-stream SQL/YAML highlighters + highlighted diff lines Replace the single-pass String.replace highlighters with position-advancing, mode-stack tokenizers (tokenizeSql / tokenizeYaml) that return a gap-free token stream over the RAW input. Highlighters become thin emit wrappers (tokenize -> escape-at-emit -> join), so nested jinja is now correct: the inner string in {{ ref('stg_orders') }} reads as a string, not part of an opaque jinja blob (cute-dbt#132). renderBlockDiff now syntax-highlights CHANGED lines too (previously they bypassed highlighting and rendered plain escaped text). It tokenizes every line and overlays the codepoint emphasis range by splitting the token stream at [a,b): in-range pieces are wrapped in <strong> while each piece keeps its syntax class. The second arg to renderBlockDiff is now a TOKENIZER, not a highlight fn (model-SQL diff call site + renderYamlDiff updated). Two hard invariants hold by construction: - gap-free: tokenizeX(s).map(t=>t.text).join("") === s - raw-in / escape-at-emit: tokens carry raw slices; escaping happens only at emit, so the codepoint emphasis offsets never drift. Tokenizer details: - SQL: jinja comment {#..#}, expr {{..}} / stmt {%..%} (whitespace-control -/+), block /*..*/, line --, single-quoted strings ('' doubling, \' tolerance, jinja-wins-inside-string split), double-quoted identifiers (plaintext), whole-identifier keyword lookup (so from_date is NOT a keyword). Unterminated fragments flush-to-end (renderBlockDiff feeds per-line pieces of multi-line constructs) -- never hang, never backtrack. - Jinja mode: strings -> sql-string (the headline fix), control words + dbt functions + call-position identifiers -> sql-jinja, else plaintext. - YAML: strings before # comments (so a # inside a quoted scalar is not a comment), start-of-line key (with - list-sigil) -> yaml-key, gap-free. New headless seams: window.__cuteTokenizeSql / __cuteTokenizeYaml. __cuteHighlightYaml / __cuteRenderYamlDiff / __cuteRenderBlockDiff kept. Verified with a node harness (69 assertions: gap-free over a realistic dbt model + all listed edge cases incl. cross-token emphasis straddle and jinja-in-string) and the existing headless suites (headless_toggle x9 + headless_csv_parser, real Chrome). The expanded keyword set + multi-word keyword split (group by -> two spans) also change context-line SQL output; deferred snapshot/example regen (owned elsewhere) will reflect that plus nested jinja. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(adapters): hunk contraction (GitHub-style fold) in diff line rendering Collapse long unchanged context runs in renderBlockDiff behind a click/keyboard "Show N unchanged line(s)" control. Frontend-only over the existing DiffLine[] payload -- no domain change (cute-dbt#132). Algorithm (renderBlockDiff): - Constants FOLD_PAD=3 (context lines kept adjacent to a change) and FOLD_MIN_HIDDEN=2 (only fold when >=2 lines would hide, so short YAML test blocks never fold). - Changed lines pass through the shared per-line render (tokenize + emphasis overlay), factored into renderOneDiffLine(ln, fn, extraClass, hidden) so the normal path and the fold path reuse one code path (keeps non-folded output byte-identical -> existing assertions stay green). - For each MAXIMAL run of consecutive context lines [runStart, j): head = runStart>0 ? FOLD_PAD : 0; tail = j<n ? FOLD_PAD : 0; hidden = runLen-head-tail. If hidden<FOLD_MIN_HIDDEN render the run whole; else render head lines, a fold CONTROL, the hidden middle lines (each `.diff-line ... diff-folded fold-<id>` with the hidden attribute), then tail lines. Leading run (head=0) folds the top of file; trailing run (tail=0) folds to EOF; a run between two changes keeps PAD on both ends. - <id> is a counter LOCAL to each renderBlockDiff call, so ids never collide across the YAML drawer vs the model-SQL block. Reveal: ONE delegated listener (click + keydown Enter/Space, Space preventDefault) on `.diff-fold`, bound once in bindGlobalHandlers and delegated on `document` (diff <code> blocks are emptied and rebuilt on model/test switch). Reveal is PARENT-SCOPED: $(this).parent().find('.fold-'+id) -- that scoping is what makes the call-local ids safe. Folded lines hide via the hidden attribute relying on the #122 `[hidden]{display:none!important}` rule (already present; Sakura's `.diff-line{display:block}` defeats a bare [hidden]). CSS: `.diff-fold` is cursor:pointer, italic, a subtle indigo tint + hover lift; its sigil is muted. Verified with a node harness (34 fold-algorithm assertions: middle / leading / trailing folds, short-block no-fold, the hidden==2 boundary, parent-local fold-0/fold-1 ids, text reconstruction) and a new headless test (real Chrome) driving __cuteRenderBlockDiff: long-run fold + correct hidden count + diff-folded[hidden], short-block no-fold, click reveal, parent-scoped isolation (a second block sharing fold-0 stays hidden), and Enter + Space keyboard activation. All 11 headless tests stay green (headless_toggle x10 + headless_csv_parser); the existing YAML-diff tests do not start folding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(adapters): regenerate examples + render snapshot; add null-vs-string-null headless test cute-dbt#132. Regenerate examples/{jaffle-shop,playground}-report.html and the render_integration HTML snapshot for the new diff visual language (token-stream highlighting expands the keyword set and colors changed diff lines; the manifest-digest snapshot is unchanged). Add a headless test pinning the null-vs-string-'null' distinction in a changed cell (a real Null reads as italic muted-gray .cell-null; the string 'null' reads as a normal .cell-new) plus the no-fill emphasis (transparent background). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf(domain): avoid two String allocs per changed line in N-to-N emphasis cute-dbt#132 (gemini review). `intra_line_span` takes `&str`, so trim the CR with `trim_end_matches('\r')` directly instead of the owned `trim_cr` helper in the aligned-replacement emphasis paths. `trim_cr` is still used for the owned DiffLine `text:` fields. Offsets are byte-identical (same trim), so the reconstruct tests are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(adapters): darken diff-emphasis colors to WCAG-AA contrast The white-on-fill line-emphasis markers and the cell-grid green text sat below WCAG-AA: green #16a34a was 3.3:1 and red #e5484d was 3.9:1 white-on-fill, and the cell green text shared the 3.3:1 green (AA wants 4.5:1 for normal text; the 13px markers do not qualify for the bold large-text exemption). Darken to green #15803d (~5.0:1) and red #dc2626 (~4.8:1). This unifies the diff palette to one red + one green across the line-diff markers and the cell grid -- the cell red was already #dc2626 (AA-passing), so only the green darkens everywhere and the line red drops to match. Pin the rationale in the CSS comment so the colors are not re-brightened. Regenerate both example reports + the render snapshot (color/comment lines only). CodeRabbit review finding on #133. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(adapters): diff-view fold controls + SQL tokenizer pinning tests Add a PR-diff-mode diff-view toolbar with two frontend-only controls over the existing fold machinery: - Context lines: a configurable fold pad (0-20, default 3). renderBlockDiff takes an optional override; the .diff-context-input handler sets the module default and re-renders the active diffs. - Expand all / Collapse all: a global toggle implemented as a SYMMETRIC DOM MIRROR (setAllFolds), NOT a re-render, so it never disturbs the SQL File<->Diff view or mermaid. Per-hunk fold controls still work. The strip is is_pr_diff-gated (inline diffs exist only there), so baseline output stays byte-identical (#85). Exposes __cuteExpandAllFolds / __cuteCollapseAllFolds seams for headless verification. Also add characterization tests pinning the token-stream SQL highlighter that shipped earlier in this PR (6f4a756) -- it sits outside every Rust gate and had zero committed assertions. The suite pins gap-free tokenization, the safe keyword-as-substring case (from_date stays plain), nested-jinja strings, and the DELIBERATE keyword-as-column limitation (lexer-only, no render-time SQL parse -- a parse would fight zero-egress/zero-compute). Regenerate examples + render snapshot (CSS/JS additions only; the strip does not appear in the baseline-mode examples). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.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.
fix: diff drawers render exactly one view; relabel toggle non-diff button to "File"
Summary
In PR-diff mode the Model SQL drawer (#111) and the Authoring YAML
drawer (#96) each rendered both
<pre>views at once — the Diff viewand the non-diff view — instead of showing one at a time via the toggle.
This fix makes the
hiddenattribute authoritative so the toggle showsexactly one view, and relabels the non-diff button to "File" in both
drawers for consistency.
Root cause
The toggle JS correctly sets
el.hidden = trueon the inactive<pre>(
hiddenAttr: true, verified in-browser). But the vendored Sakura CSS(
assets/sakura-1.5.0.css) carries a barepre { display: block }authorrule. Author-origin rules beat the UA stylesheet's
[hidden] { display: none }, so the inactive<pre>'s computeddisplaystays
blockand it renders anyway. The<p hidden>affirmations wereunaffected only because nothing sets
displayon<p>.The fix
In
templates/report.html:[hidden] { display: none !important; }to the report<style>block (pure inlined CSS — no new asset, no
type=module, so thezero-egress gate is unaffected). The
!importantmakes thehiddenattribute beat Sakura's author-origin
pre { display: block }.(was "Raw" for SQL, "Authored" for YAML). The
data-viewattributevalues (
"raw"/"authored") and the "Diff" button are unchanged.Acceptance Criteria (from #121)
[hidden] { display: none !important; }added to the report<style>, commented to note it neutralizes Sakura'spre{display:block}.<pre>visible — Diff by default;toggling shows only the File view.
<pre>visible.drawers; the "Diff" button is unchanged.
tests/headless_toggle.rs): renders aPR-diff report carrying both a model
sql_diffand a unit-testyaml_diff; asserts exactly one<pre>is visible per drawer bydefault and after toggling. New test
diff_drawers_render_exactly_one_view_eachasserts visibility viaoffsetHeight(the existing tests asserted only the.hiddenproperty —
trueeither way — which is why the double-rendershipped). Confirmed RED-first against the unfixed template.
examples/*.htmlregenerated (the<style>change alters everyrendered report) and render snapshots accepted; manifest-digest
snapshot unchanged.
inlined
<style>, no new asset).Testing
diff_drawers_render_exactly_one_view_eachwasconfirmed RED first: pre-fix it fails both on the button label
(
"Raw"vs"File") and on visibility (the inactive.sql-raw-viewreports
offsetHeight > 0). Post-fix it is GREEN, and the fullheadless_togglesuite (7 tests) stays green.cargo nextest run(543 passed, 9 skipped),cargo test --test bdd(74 scenarios / 477 steps), the two--ignoredheadless suites,
render_integration, clippy--locked -D warnings,fmt --check,cargo doc -D warnings,cargo deny check, andlefthook run pre-push.(
tests/snapshots/manifest_ingestion__golden_baseline_manifest_digest.snap)did not change. Only the HTML-embedding render snapshot
(
render_integration__rendered_chrome_jaffle_shop.snap) and the twoexamples/*.htmlmoved — and only by the style rule + the two labelbytes (verified by reverting exactly those three edits and confirming
byte-identity to the prior committed artifacts).
Context
Closes #121
🤖 Generated with Claude Code
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements