Repository navigation
#155 — fix spurious DAG cycle when a CTE shares the model's name (stable node identity) - #156
Conversation
A model whose CTE shares the model's name — the idiomatic `with orders as (...)` on the `orders` model — rendered a spurious `orders ↔ final` cycle in the lineage DAG. The terminal node's id was set to the model's bare name, colliding with the same-named import CTE, so the two distinct engine nodes collapsed into one Mermaid node and the import node's compiled-SQL panel was clobbered with the terminal's `select * from final`. Key node identity by the engine's stable node name everywhere (Mermaid node id, edge endpoints, the compiled_sql map); the model's file name rides as a display-only `label` on the terminal node. CTE names are unique within a WITH, so terminal-vs-CTE was the only collision vector — now closed in general via stable-index identity rather than name-derived identity. Per Christopher's suggestion, the terminal node now displays `<model>.sql` (the model's file name) in every report — semantically "this node is the model's compiled output", and visually unmistakable from a same-named import CTE. The byte-gated example reports reflect the new terminal label. Tested: render-layer unit regression (unique ids, no self-cycle, import keeps its own SQL, terminal label `orders.sql`) + a real-Chromium headless regression reproducing the exact reported symptom (click the import node → its panel shows `raw_orders`, not `from final`). Closes #155 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-label `bindDagNodeClicks` built the node aria-label from the internal node id, so after the identity fix a screen reader announced the terminal as `(final select)` while it visibly reads `<model>.sql`. Thread the display label (`label || id`) into the aria-label so the announced and visible names match again. Headless regression asserts the terminal node's aria-label carries its `.sql` label. Examples + snapshot regenerated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 51 minutes and 47 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 selected for processing (1)
📝 WalkthroughWalkthroughThis PR fixes a DAG rendering bug ( ChangesSelf-Named CTE DAG Collision Fix
Sequence Diagram(s)The PR does not introduce new component interactions warranting a sequence diagram. The changes are structural refinements to node identity representation and display rendering, without new async workflows, API calls, or multi-component request/response flows. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit e737edf on Jun 9, 2026 2:15am UTC. |
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR's own
▶ Open ↗ opens the report in your browser in one click — The Pages preview may take ~1 min to update after this comment Alternative: GitHub CLI# gh CLI >= 2.63 extracts into ./report-preview-playground/.
gh run download 27179609085 -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 resolves an issue (cute-dbt#155) where a self-named import CTE (e.g., with orders as (...) in an orders model) would collapse into the terminal node in the DAG. It achieves this by decoupling the stable node ID from its display label: the terminal node's ID is now a stable engine name, while its display label is formatted as <model_name>.sql. The changes span the Rust adapter, HTML templates, and integration tests. Feedback was provided to update a doc comment in src/adapters/render.rs to accurately reflect that the terminal node's label is formatted with the .sql suffix rather than just being the bare model name.
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.
…ers model Touch the `orders` model (whose import CTE is named `orders`, the collision case) so the PR's report-preview renders its DAG — surfacing the fix live: `orders` (import) → `final` → `orders.sql` (terminal), three distinct nodes with no spurious self-cycle. The comment is purely documentary; the model's compiled SQL and output are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…olumn tooltips (#188) * feat(adapters): design integration PR-2 - engine merge (settings panel, unified/split diffs) + port-forward Replace the template's ~2100-line inline engine with two first-party include_str! assets: templates/interaction.js (the merged redesign interaction engine) and templates/theme.js (the appearance engine), wired through asset_embed::{INTERACTION_JS,THEME_JS} with banner-pin + end-of-file-sentinel tests (the REPORT_CSS pattern). Engine merge (handoff -> shipped, our behaviors win every conflict): - Settings panel: static Appearance markup (style/theme/accent/density/ diff-style/diff-layout) in the template, wired by theme.js; persists under cute-dbt.appearance.v1; DataTables dark syncs via html.dark. The cog + panel are now universal (themes apply to baseline reports too); the #139 context-lines/normalize rows stay PR-diff-only. - Diff renderers: GitHub-grade two-column line-number gutters, shared diffBody/diffNumbers helpers, hunk folds keep the #132/#136 contract, and a NEW split (side-by-side) renderer - diffViewsHtml emits BOTH layouts over the same BlockDiff lines verbatim (kind/text/emphasis; no Rust diff-engine change); CSS shows one per html[data-difflayout] with a responsive fallback. - Code cards: file-path headers (defined_in on the YAML drawer, <model>.sql on the Model-SQL card), single-gutter line numbers on plain blocks (highlightLines*), incremental badge relocated into the Model-SQL card header (static span removed per handoff README section 3). - DAG: theme-aware edge palette (JOIN_COLORS_LIGHT/_DARK + dagEdges), __cuteRerenderDag re-tint hook; Cytoscape surface deliberately excluded (Bucket 2). Port-forward audit (the fork predates these; shipped behavior kept): - #156 stable DAG node identity: already present in the handoff fork's JS; the Rust side never forked. Verified, no regression. - #158 minmax(0,1fr) stacked-panel fix: PR-1 chassis kept it; the 375px headless regression stays green. - #161 strategy-correct expect-semantics tooltip: kept OUR wording (the handoff carried the pre-#159 merge/insert copy - rejected). - #166 column tooltips: kept OUR payload shape + focusable-button/CSS- bubble renderer (the handoff's body-appended th.has-col-meta renderer targets the not-yet-existing ModelPayload.column_meta - rejected; belongs to handoff PR-3). Also: report.css drops its PR-1 bridge layer per its own contract and gains the .settings-row-stack chrome; the edge-vocab-completeness gate + BDD legend steps re-point at templates/interaction.js and now check both palettes; headless guards added for the appearance toggles, the split renderer, the gutters and the relocated badge; goldens + insta snapshots regenerated (byte-identity re-verified CI-style). Closes #178 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(adapters): column-header tooltips to the handoff spec (th-trigger, accent keys, value chips) + gemini fixes Christopher's review override on PR #188: the #166-tooltip deferral is reversed — the column-header tooltips must match the handoff spec in this PR, adapted to OUR shipped per-table column_meta payload (NOT ModelPayload.column_meta, which stays PR-3). Spec match (handoff engine/interaction.js + base.css, README section 2.2): - The WHOLE header cell is the hover/focus trigger — the info icon (buildColTooltip's focusable button + per-th bubble) is gone. decorateColHeader marks a metadata-bearing th with .has-col-meta + data-col-name/desc/tests; showColTip fills + positions the single body-appended #col-tooltip bubble with the handoff's positioning math verbatim (position:fixed, horizontal clamp, flip-above on bottom overflow — never clipped by the .table-fit scroller). - Bubble content per spec: .ct-desc (description, larger) -> "Data tests" label -> per-test .ct-test rows with .ct-key (accent color), .ct-vals > .ct-val chips for accepted_values args, .ct-detail muted mono for relationships/range detail. CSS ported from the handoff base.css (comment bodies kept free of close-sequences per the README porting-bug warning). - Render-layer POD (adapter, not domain): ColumnMetaPayload.tests is now Vec<ColumnTestPayload {name, values, detail}> built by column_test_payload, the section-2.2 display mapping: unique / not_null -> bare prose names; accepted_values -> "accepted values" + values chips; relationships -> detail "model.field" (ref()/source() unwrapped to the last quoted arg); accepted_range (any namespace) -> detail "min-max" / ">= min" / "<= max"; any other test keeps its package-qualified raw identifier with no values/detail. - A11y (the #166 contract carried onto the th): tabindex="0" so keyboard users reach the tip via focusin; aria-label on the trigger summarizes description + tests; the singleton bubble is aria-hidden; hover AND focus both reveal; decorated headers SHED the native title (undecorated headers keep theirs); :focus-visible outline added. Guards updated: - headless: column_header_tooltips_th_trigger_hover_focus_and_skip_ bare_columns — th-trigger semantics (no icon/button in any th), tabindex/aria/title assertions, focus AND hover reveal + hide, .ct-key/.ct-val/.ct-detail content over an accepted_values + relationships fixture, given-table input-model resolution. - BDD: the report_generation scenario matches the structured entries' display `name` ("not_null" -> "not null"); payload contract otherwise unchanged. - render.rs unit tests pin the full section-2.2 mapping table. Gemini review threads (PR #188, both applied): theme.js gains an ES5-safe qsaForEach helper replacing NodeList.prototype.forEach at every control-wiring site, and reflowTables binds jQuery locally. Goldens + the chrome insta snapshot regenerated; byte-identity re-verified CI-style; visual check against the handoff reference.html confirms the tooltip treatment matches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Fixes the spurious circular reference in the lineage DAG (cute-dbt#155): a
model whose CTE shares the model's name — the idiomatic
with orders as (...)on the
ordersmodel — collapsed the import CTE and the model's final-selectnode into one Mermaid node, showing a phantom
orders ↔ finalcycle andclobbering the import node's compiled-SQL panel with the terminal's
select * from final.Root cause
The terminal node's id was set to the model's bare name, colliding with
the same-named import CTE. Three render-layer sites keyed on that id then
collapsed: the Mermaid node id, the edge endpoints, and the
compiled_sqlmap — the last is why the IMPORT-badged node displayed the terminal's SQL.
The CTE engine itself was correct (it emits distinct, acyclic nodes); the
collapse was purely in the renderer's name-as-identity keying.
Fix
Key node identity by the engine's stable node name everywhere; the
model's file name rides as a display-only
labelon the terminal node. CTEnames are unique within a
WITH, so terminal-vs-CTE was the only collisionvector — now closed in general (stable identity, not name-derived).
Per Christopher's suggestion, the terminal node displays
<model>.sql—semantically "this node is the model's compiled output", and unmistakable
from a same-named import CTE. In the collision case the DAG now reads
orders(import) →final→orders.sql(the model).The terminal label change applies to every report, not just collision
cases: every terminal now reads
<model>.sqlinstead of<model>. Thebyte-gated examples reflect it. Tagged
fix→ patch (it's a bug fix andthe
.sqllabel is the disambiguation mechanism), but flagging it so you canconsciously pick patch vs. minor at squash given the v0.x output-shape rule.
Verification
import node keeps its own SQL, terminal label
orders.sql.symptom: click the import node → its panel shows
raw_orders, notfrom final; plus three distinct nodes + the a11y aria-label match.(
(final select)) instead of the visible<model>.sql— fixed + asserted.--locked, nextest (731), bdd (85), headless--ignored(toggle 25 + zero-egress),cargo doc -D warnings, deny,crap4rs (the touched fns are CRAP 1–5, far under render.rs's strict 15), all
three examples byte-identical on re-render.
Dogfood note (honest gap)
No committed golden reproduces the collision — all three golden fixtures
(jaffle-shop / playground / diff-showcase) are real-compile-provenance'd
(sha256 +
origin_url) and can't be hand-edited to add a self-named-CTE modelwithout breaking that provenance. The committed headless regression is the
durable guard; the live
dbt-project/ordersmodel (which surfaced this)renders correctly now and will show on the next
dbt-project-touching PR'spreview.
Closes #155
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Accessibility
Bug Fixes