Repository navigation
adapters: askama renderer reproducing Claude Design HTML (#11) - #38
Conversation
PR 8b — implement the v0.1 interactive `report.html` via askama 0.16. Closes #11. ## What ships - `src/adapters/render.rs` — composition layer between domain types, the CTE engine, and the askama template. Owns per-model payload assembly, node-role classification (`final` / `import` / `transform`), clean-import-CTE binding via `ref('NAME')` parsing, and the run-loop integration. CRAP threshold strict (15); the highest score on this file is 8.19. - `templates/report.html` — askama 0.16 template emitting the design's full DOM contract: diff-scope banner, model + test cascade selectors, CTE DAG host, two-panel region, JSON payload carrier (`<script type="application/json" id="cute-dbt-data">`). The vendored asset bundle (Sakura, jQuery, DataTables, Mermaid UMD) is inlined via `include_str!` constants from `asset_embed.rs`. `interaction.js` adapted verbatim from the design hand-back with the dormant dark-mode infrastructure stripped (YAGNI) and the Mermaid `<g>` selector runtime-constructed (`[id^="${svgId}-flowchart-"]`) per ADR-4 amendment 2026-05-22. - `tests/render_integration.rs` — fixture-driven integration coverage: asset bundle assembly, design DOM contract, payload shape, JSON carrier integrity, structured resource-ref egress lint, and a chrome- only insta snapshot of the rendered jaffle-shop report. ## What was removed - `MERMAID_INIT` constant + its pin test from `asset_embed.rs` — the real init lives in the inlined interaction script with `startOnLoad: false` (ADR-4 amendment); a Rust-side constant would invite drift. - `smoke_report_html` + its tests + the `escape_label` / `mermaid_block` helpers — the real renderer replaces them. - `tests/asset_embed.rs` — replaced by `tests/render_integration.rs` which exercises the real renderer end-to-end. - ARCHITECTURE.md §6 (synthetic-only fixture invariant) — the rule is contributor/build-hygiene rather than architectural; relocated to CONTRIBUTING.md (human contributors) and AGENTS.md (AI agents). Section numbers tightened. ## Run-loop wiring `src/cli/mod.rs::render` widens to thread `current` / `in_scope` / `models_in_scope` / `baseline_label` into `adapters::render::render_report`. `parse_ctes()` stays as a named no-op call site; per-model CTE parsing happens inside the renderer during payload assembly, so the four-stage diagram (`scope → preflight_compiled → parse_ctes → render`) stays greppable. ## Discovery #1 Folded into this PR: `features/report_generation.feature` dropped the stale "And the banner states the v0.1 fidelity limit 'model body changes'" line — the diff-scope banner no longer carries that wording (per AC). The body-checksum scoping note moved to README.md. ## Validation gates - `cargo test`: 254 passed across 12 suites - `cargo clippy --all-targets --locked -- -D warnings`: clean - `cargo fmt --check`: clean - `cargo deny check`: advisories ok, bans ok, licenses ok, sources ok - `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --locked`: clean - `lefthook run pre-push --all-files`: 9/9 gates green (fmt, clippy, test, deny, docs, baseline-required-grep, feature-count, fixture-manifest-gate, non-mirror-guard) - crap4rs strict scoring on `src/adapters/render.rs`: max 8.19, threshold 15 — PASS - `cargo mutants --file src/adapters/render.rs`: 161 mutants tested, 132 caught, 0 missed, 29 unviable — 100% kill on the strict scope Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous XSS test embedded `</script><script>alert(1)</script>` into a `WITH "<name>" AS (...)` clause. sqlparser refuses that as an identifier so `parse_cte_graph` returns Err, the renderer falls back to an empty graph, and the hostile compiled_code lands in `compiled_sql` under the model's bare name instead of taking the CTE-name-payload path the docstring claimed. Rewrite the test so the hostile value rides on a unit-test `tags` entry — manifest-side YAML metadata that does not pass through any parser before landing in the payload. Same structural property exercised (askama `| json` escapes `<>&'`), now under the path the docstring describes. Also tighten the assertion: the payload carrier `<script type="application/json" id="cute-dbt-data">` opens exactly once, so the hostile string can't smuggle a second carrier-open tag in. Tests: 254 passed (still 100% green). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 38 minutes and 14 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ 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 (4)
📝 WalkthroughWalkthroughThis PR delivers the v0.1 interactive HTML report by implementing an Askama 0.16 template and Rust renderer module that compiles manifest/CTE data into a per-model, per-test JSON payload and final HTML with DAG visualization, clickable node inspection, and SQL syntax highlighting. The renderer integrates into the CLI pipeline, removes obsolete stubs, and includes comprehensive integration tests with artifact integrity guardrails. ChangesInteractive HTML Report Renderer (Askama 0.16)
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI execute()
participant Scope as scope stage
participant Preflight as preflight_compiled
participant ParseCTE as parse_ctes (no-op)
participant Render as render stage
participant RenderReport as render_report()
participant Askama as Askama::render()
participant Output as report.html
CLI->>Scope: current, baseline, models_in_scope
Scope->>Preflight: in_scope, models_in_scope
Preflight->>ParseCTE: in_scope, models_in_scope (no mutation)
ParseCTE->>Render: ready for render
Render->>Render: derive baseline_label from path
Render->>RenderReport: current, baseline_label, in_scope, models_in_scope, out_path
RenderReport->>RenderReport: build_payload (walk models, bind tests)
RenderReport->>Askama: ReportTemplate + ReportPayload
Askama->>Askama: inline assets, embed JSON, render template
Askama->>Output: write HTML
Output-->>CLI: io::Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
PR 8b dropped `edge_label` (and `smoke_report_html`) from
`src/adapters/asset_embed.rs`; the `edge-vocab-completeness` CI gate's
grep against that file always failed.
- Add `edge_type_wire_key` to `src/adapters/render.rs` — an exhaustive
Rust match producing the snake_case wire key per `EdgeType` variant.
Compile-time exhaustiveness is now the primary mechanism (a new
variant fails to build); CI greps this function as belt-and-braces.
- Add a unit test pinning every variant's wire key against
`serde_json`'s snake_case serialization. If a future
`#[serde(rename_all)]` edit drifts the wire shape, the test fails
before the JS palette ships a broken color.
- Update the CI gate to enforce BOTH coverage paths:
1. Every variant appears in `render.rs::edge_type_wire_key`.
2. Every snake_case wire key appears in `templates/report.html`'s
`JOIN_COLORS` map (covers the JS palette + legend).
The two-side gate keeps the renderer's edge palette in lockstep with
the classifier vocabulary without re-introducing the Rust→Mermaid
label string the original `edge_label` function carried.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
/gemini review |
![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 replaces the placeholder smoke renderer with a full-featured askama 0.16 implementation, introducing src/adapters/render.rs and templates/report.html. The new renderer manages per-model payload assembly, CTE graph node-role classification, and the binding of unit test fixtures to import-CTEs. The CLI run loop has been updated to integrate this renderer, and documentation regarding synthetic-only fixtures was relocated to CONTRIBUTING.md. Review feedback highlighted logic issues in the SQL parsing heuristics, specifically noting that keyword detection in is_simple_from_select is fragile due to strict spacing requirements and that parse_ref_name lacks support for valid dbt whitespace variations.
| fn is_simple_from_select(sql: &str) -> bool { | ||
| let lower = sql.to_ascii_lowercase(); | ||
| if !lower.contains("select") { | ||
| return false; | ||
| } | ||
| if lower.contains(" join ") || lower.contains("\njoin ") { | ||
| return false; | ||
| } | ||
| let from_count = lower.matches(" from ").count() + lower.matches("\nfrom ").count(); | ||
| from_count == 1 | ||
| } |
There was a problem hiding this comment.
The is_simple_from_select heuristic contains a logic bug that will cause it to fail for most standard SQL strings. The use of matches(" from ") and contains(" join ") requires specific surrounding spaces that are often absent (e.g., at the end of a string or before a semicolon). For example, "select * from table" will result in a from_count of 0 because there is no space after table. Using split_whitespace() to iterate over words is a much more robust approach for keyword detection.
There was a problem hiding this comment.
Fixed in 31ddda6. Replaced the substring-with-required-spaces heuristic with a whitespace-tokenizing pass: sql.split_whitespace() → lowercase + strip trailing ,;) → classify as select/from/join. Trailing-punctuation and end-of-string edge cases (e.g. select * from x;, select 1 from x\n) now classify correctly. Verification: is_simple_from_select_* unit tests at src/adapters/render.rs:1130-1165.
| pub fn parse_ref_name(input: &str) -> Option<&str> { | ||
| let trimmed = input.trim(); | ||
| let rest = trimmed.strip_prefix("ref(").or_else(|| { | ||
| trimmed | ||
| .strip_prefix("REF(") | ||
| .or_else(|| trimmed.strip_prefix("Ref(")) | ||
| })?; | ||
| let rest = rest.strip_suffix(')')?; | ||
| let inner = rest.trim(); | ||
| let name = inner.strip_prefix('\'')?.strip_suffix('\'')?; | ||
| if name.is_empty() { None } else { Some(name) } | ||
| } |
There was a problem hiding this comment.
The current implementation of parse_ref_name is fragile. It only supports three specific case variants of the ref keyword and does not allow for whitespace between the keyword and the opening parenthesis (e.g., ref ('model')), which is valid in dbt. Additionally, using to_ascii_lowercase() on the prefix check would be more robust than hardcoding variants.
There was a problem hiding this comment.
Fixed in 31ddda6. parse_ref_name now (1) uses eq_ignore_ascii_case("ref") for case-insensitive keyword matching across any byte casing, and (2) calls trim_start() between the keyword and the opening paren so ref ('x'), REF\t('y'), etc. parse correctly. Verification: new tests parse_ref_name_tolerates_whitespace_between_ref_and_paren and the updated parse_ref_name_accepts_case_variant_keyword.
Two parallel changes in this commit: ## examples/ End-to-end demonstration of cute-dbt's output, generated against the already-committed jaffle-shop fixture pair. A reader evaluating the tool opens `examples/jaffle-shop-report.html` directly and sees the real renderer behavior — Mermaid DAG, DataTables panels, JSON payload, the in-scope `stg_customers` model with its 1 unit test carrying populated `tags` / `meta` / `defined_in`. - `examples/jaffle-shop-report.html` (3.6 MB) — generated by `cargo run --bin cute-dbt -- --manifest tests/fixtures/...current.json --baseline-manifest tests/fixtures/...baseline.json --out ...`. - `examples/README.md` — what each file demonstrates + how to regenerate + why we commit the artifact instead of generating in CI. - `.gitattributes` — marks `examples/*.html` as `-diff linguist-generated` so `git diff` and the GitHub UI suppress the unreadable wall of inlined-asset bytes; the new CI guard is the authoritative change detector. - CI job `example-report-up-to-date` — regenerates the report against the same committed fixtures and asserts byte-identical to `examples/jaffle-shop-report.html`. Drift fails the gate with a one-line remediation command. (The two previous skip-stub jobs for the headless zero-egress proof stay skipped; their tracking issue is unchanged.) The renderer is deterministic over fixture input (BTreeMap + BTreeSet-backed iteration in payload assembly), so byte-equality is the correct gate. ## Comment sweep Removed pipeline-private references that are not self-contained for a reader of the public repo: - "PR 8b" / "PR 8a" / "PR N" labels in module docs and integration test docs → either dropped or replaced with the GitHub issue number where the public anchor is meaningful. - "ADR-N" references → replaced with the matching `ARCHITECTURE.md §N` section (which IS public). Architectural intent is preserved. - "ADR-4 amendment 2026-05-22" date-stamped breadcrumbs → dropped; the in-file comments describe the actual behavior they were pointing at (Mermaid `<g>` id selector form, `securityLevel: 'strict'` constraints, the JS `JOIN_COLORS` keying contract). The insta chrome snapshot was regenerated to reflect the JS comment updates (no behavior change). ## Tests 255 passed (was 254 — one new `edge_type_wire_key_*` test added earlier this PR). Build / clippy / fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…elog - examples/README.md → link the richer-examples follow-up issue (#39). - CHANGELOG.md → drop "PR 8b" / "ADR-4 amendment" labels that don't read for someone arriving at the file from outside the build pipeline; add an Added bullet for the examples/ directory. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…count, union split
Five small fixes from a live walk-through of the example report:
1. **Compiled-SQL block: top padding.** The floating `.sql-copy` button
was overlapping the first line of SQL. Added `padding-top: 2.5rem`
to `.sql-block` — the button (~30 px from the wrapper's top edge)
now clears the first line cleanly.
2. **Import-CTE binding: pass-2 body match.** The existing jaffle-shop
fixture's import CTE is named `source` (dbt's idiomatic unwrapper
name), and the unit test's `given.input` is `ref('raw_customers')`.
Pass-1 name match couldn't resolve `raw_customers` against `source`,
so the rendered report showed the empty "no fixture provided" state.
Extended `find_import_node_id` with a pass-2 body-leaf match: when
pass-1 misses, scan each import-CTE's `raw_sql` for table refs (via
the new `extract_table_leaf_refs` helper) and pick the CTE whose
body references the ref'd model. The dbt-idiomatic compiled-SQL
shape `with source as (select * from "db"."schema"."MODEL")` now
binds cleanly without forcing fixtures to use the design's
sample-data CTE-name=model-name convention.
3. **Drop redundant test-count UI.** The model selector option labels
already carry each model's test count (`stg_customers (1 test)`).
The right-side `.test-select-count` element next to the unit-test
selector was duplicating the same signal; removed the span, the
JS update, and the (now-orphaned) CSS rule.
4. **UNION variants get distinct colors.** Previously `union_all` and
`union_distinct` both rendered dashed orange. Split: `union_all`
stays orange (duplicates preserved), `union_distinct` is now blue
(deduplicated). Both stay dashed so the row-concatenation cue is
preserved.
5. **Regenerated `examples/jaffle-shop-report.html`** — the report now
surfaces the `ref('raw_customers')` Given table when you click the
`source` import node, instead of the empty-state copy.
## Tests
261 passed (was 255 — 6 new):
- `build_payload_given_binds_import_cte_via_body_table_reference`
- 5 unit tests for `extract_table_leaf_refs`
Pre-push battery: 9/9 green. CI guard locally byte-identical against
the regenerated example.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/ci.yml:
- Around line 465-489: The CI jobs resource-ref-lint and headless-zero-egress
are currently disabled by the literal gate `if: false`; remove or change that
gate so the jobs run (for example remove the `if: false` lines or replace with a
proper conditional like `if: github.event_name == 'pull_request' ||
github.event_name == 'push'`), ensuring the Resource-ref lint job
(`resource-ref-lint`) executes to reject real-loading constructs and the
headless browser proof job (`headless-zero-egress`) runs with network blocked to
assert zero outbound requests; keep their steps (checkout + run) intact and
adjust any timeouts or runner settings if needed.
- Around line 445-447: The workflow uses mutable tags for third‑party
actions—replace each tag-based reference (the uses lines "actions/checkout@v4",
"dtolnay/rust-toolchain@stable", and "Swatinem/rust-cache@v2") with the
corresponding commit SHA pinned references; locate the exact commit SHA for each
action repository (e.g. the commit that matches the tag you intend to lock) and
update the uses entries in the new job in .github/workflows/ci.yml to use the
full SHA (repo@sha) so the CI job is pinned to immutable revisions.
In `@src/adapters/render.rs`:
- Around line 1233-1258: The test function
render_report_does_not_emit_external_resource_constructs is using fragile
substring greps to detect external references; replace those raw
contains/replace checks with the project's structured resource-ref lint utility
(the same linter used elsewhere to reject <script src>, <link href>, <img src>,
CSS `@import` and url(), and protocol-relative //), invoking it against the
generated chrome content from render_report (and still stripping the known
inlined assets like SAKURA_CSS, DATATABLES_CSS, JQUERY_JS, DATATABLES_JS,
MERMAID_JS beforehand); assert the linter reports no external references rather
than relying on plain .contains("http://")/.contains(" src=\"") greps.
In `@templates/report.html`:
- Line 239: The inline JSON script with id "cute-dbt-data" currently uses {{
payload|json|safe }} which allows a payload containing "</script>" to break out
and cause XSS; change the rendering to a safe JSON-encoding mechanism (e.g., use
the template engine's safe JSON serializer such as Jinja2's tojson filter or
otherwise escape closing script tags) so the payload is serialized as valid JSON
and any "</script>" sequences are escaped (for example by replacing "</script>"
with "<\/script>") before insertion; update the template expression that renders
payload (the {{ payload... }} expression) accordingly and remove any raw |safe
usage so the JSON blob cannot terminate the script tag.
🪄 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: 7d41ee93-b9cb-4380-ac82-bd27e96543a9
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktests/snapshots/render_integration__rendered_chrome_jaffle_shop.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
.cargo/mutants.toml.gitattributes.github/workflows/ci.ymlAGENTS.mdARCHITECTURE.mdCHANGELOG.mdCONTRIBUTING.mdCargo.tomlREADME.mdcrap4rs.tomlexamples/README.mdexamples/jaffle-shop-report.htmlfeatures/report_generation.featuresrc/adapters/asset_embed.rssrc/adapters/mod.rssrc/adapters/render.rssrc/cli/mod.rstemplates/report.htmltests/asset_embed.rstests/render_integration.rs
💤 Files with no reviewable changes (2)
- tests/asset_embed.rs
- features/report_generation.feature
cargo-mutants surfaced one surviving mutant after the body-match extension: relaxing the pass-1 predicate from `name match && role match` to `name match || role match` would spuriously bind a wrong-named import CTE to a unit test whose ref doesn't actually match it. No existing test reached that code path because every fixture either had ALL import-named-correctly or NO import-named-correctly. Add a discriminating fixture: an import CTE with the WRONG name + a transform CTE with the RIGHT name + a body that references an unrelated table. The correct return is `None` (neither pass-1 nor pass-2 matches); under the mutant, pass-1's `||` would accept the first import-role node regardless of name and return its identifier. Mutation testing on `src/adapters/render.rs`: 175 mutants, 146 caught, 0 missed, 29 unviable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Resolves the actionable findings from CodeRabbit + Gemini on PR #38. ## Rust-side `</` + `<!` escaping for the JSON payload CodeRabbit flagged that `{{ payload|json|safe }}` relied on askama's filter to escape script-terminating sequences. While the askama 0.16 book documents that `|json` escapes `<>&'`, encoding the safety property in askama's filter is an indirection that breaks the moment the template engine swaps or the docs drift. Move the escape to Rust: - New `payload_json_for_html_script(&ReportPayload) -> Result<String, _>` serializes via serde_json, then re-escapes any `<` followed by `/` or `!` as the JSON unicode escape `<`. These are the only two sequences HTML5's script-data state machine treats specially (`</alpha` terminates, `<!--` enters script-data-double-escape). - Template now takes `payload_json: &str` (pre-escaped) and renders it with `|safe`. The safety property is the Rust-side escape, no longer the filter. - Template no longer reads `payload.baseline`; the renderer passes `baseline_label: &str` directly. Cleaner separation: the template doesn't know the payload's structure. Tests cover the escape (`</script>` and `<!--` both get unicode- escaped, bare `<` stays bare, output round-trips through `JSON.parse`). ## Matcher robustness (Gemini) - `parse_ref_name` is now byte-case-insensitive on the `ref` keyword (`rEf` accepted, not just the hardcoded `ref`/`REF`/`Ref` triple) and tolerates whitespace between the keyword and the opening paren (`ref ('x')`, `REF\t('y')`) — matches dbt/Jinja semantics. - `is_simple_from_select` switches from substring-with-spaces (`" from "` / `"\nfrom "`) to a whitespace-tokenizing pass that classifies cleanly regardless of trailing punctuation or line endings. The token-stripping handles `from;` / `from,` / `from)` edge cases the original substring scan would miss. ## Widened egress test patterns (CodeRabbit) `render_report_does_not_emit_external_resource_constructs` and its integration-test sibling now match the structured-lint pattern set: `<script src` / `<link href` / `<img` / ` src="` / `@import` / `url(` / `http://` / `https://` / protocol-relative `//`. Comment references the canonical structured lint at `#12` so a future reader knows where the real gate lives. ## Validation - 267 tests pass (was 263 — 4 new for `payload_json_for_html_script`). - `cargo mutants --file src/adapters/render.rs`: 182 mutants, 153 caught, 0 missed, 29 unviable. - Example report regenerated; CI guard locally byte-identical. - clippy + fmt + docs + cargo-deny clean. ## Dispositions for non-fixed findings (posted as PR replies) - CodeRabbit: pin GitHub Actions to commit SHAs — won't fix; the rest of `.github/workflows/ci.yml` uses tag refs, so pinning only the new job is inconsistent. A workflow-wide SHA-pinning sweep belongs in its own PR. - CodeRabbit: enable `resource-ref-lint` / `headless-zero-egress` — deferred to `#12` (pre-existing stubs with `tracked:` comments; the real harness lands there). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@coderabbitai review /gemini review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 62919 🏁 Script executed: Length of output: 31974 🏁 Script executed: Length of output: 42410
🔴 Issues worth fixing before merge1. In s = s.replace(/('[^'\n]*')/g, '<span class="sql-string">$1</span>');
// ...
s = s.replace(SQL_KEYWORDS_RE, '<span class="sql-keyword">$1</span>');A literal like <span class="sql-string">'<span class="sql-keyword">from</span> me'</span>This is malformed HTML (a 2.
let ref_name = if role == NodeRole::Import {
Some(node.name().to_owned())
} else {
None
};Grepping 🟡 Issues worth addressing soon3. function buildMermaidSource(m, t) { // `t` never referenced in the body
var lines = ["graph LR"];
// ...
}The callsite passes only var src = buildMermaidSource(m);Either remove the parameter or use it (perhaps to highlight the currently-selected-test's bound nodes). As-is it suggests abandoned intent and will trip any future JSHint/ESLint pass. 4. function renderDag() {
// ...
window.mermaid.initialize({
startOnLoad: false,
securityLevel: "strict",
theme: "base",
fontFamily: '...'
});
var id = "mermaid-" + Math.random().toString(36).slice(2, 9);
window.mermaid.render(id, src).then(...);
}
5. function unwrapRows(rows) {
if (!rows) return [];
if (Array.isArray(rows)) return rows;
return []; // format: sql, format: csv, or any future shape → silent empty
}dbt unit tests support 6. fn find_import_node_id(graph: &CteGraph, ref_name: &str, _model_name: &str) -> Option<String> {The 🟢 Nits / low-priority7. let token = raw
.to_ascii_lowercase()
.trim_end_matches([',', ';', ')'])
.to_owned(); // ← String per token
match token.as_str() { ... }
8. DAG nodes are not keyboard-accessible g.style.cursor = "pointer";
g.addEventListener("click", function () { ... });No ✅ Things that look solid
|
There was a problem hiding this comment.
Code Review
This pull request implements a production-ready HTML report renderer using the askama template engine, replacing the previous placeholder smoke renderer. It introduces per-model payload assembly, CTE node role classification, and logic to bind unit test inputs to specific CTE nodes. The UI is enhanced with an interactive Mermaid DAG, searchable DataTables for test data, and a colorblind-safe legend for edge types. Feedback focuses on improving the robustness of the renderer's internal logic, specifically recommending the use of the sqlparser dependency instead of fragile whitespace-based heuristics for SQL classification and table reference extraction. Additionally, the reviewer suggests using expect() for node index lookups to document invariants and fail fast in case of logic errors, rather than providing silent fallbacks.
| let Some(node) = graph.nodes().get(node_index) else { | ||
| return NodeRole::Transform; | ||
| }; |
There was a problem hiding this comment.
This function silently falls back to NodeRole::Transform if the node index is out of bounds. Since this function is called with indices derived from the graph itself, an out-of-bounds index indicates a logic error or invariant violation. Using expect() would be more appropriate to document this invariant and fail fast if it is violated.
let node = graph.nodes().get(node_index).expect("node index is valid");References
- Prioritize explicit defensive guards that document code invariants over more concise idiomatic alternatives if the guard clarifies intent and ensures robustness against future changes in logic.
There was a problem hiding this comment.
Won't fix. Defensive fallback documents "this is unreachable but we don't panic" intent. classify_node_role is called from payload assembly where an out-of-bounds index is logically unreachable, but expect() would take down the entire report for a single-node anomaly. The current behavior degrades gracefully (the node simply gets classified as transform and renders with the neutral palette) rather than crashing the process. Full disposition: see my summary comment on the thread.
| fn is_simple_from_select(sql: &str) -> bool { | ||
| let mut has_select = false; | ||
| let mut from_count = 0usize; | ||
| for raw in sql.split_whitespace() { | ||
| let token = raw | ||
| .to_ascii_lowercase() | ||
| .trim_end_matches([',', ';', ')']) | ||
| .to_owned(); | ||
| match token.as_str() { | ||
| "select" => has_select = true, | ||
| "join" => return false, | ||
| "from" => from_count += 1, | ||
| _ => {} | ||
| } | ||
| } | ||
| has_select && from_count == 1 | ||
| } |
There was a problem hiding this comment.
The whitespace-tokenizing heuristic for identifying simple SELECT statements is fragile and will incorrectly classify SQL containing keywords like FROM or JOIN inside string literals or comments (e.g., SELECT 'from' AS col FROM table). Since sqlparser is already a project dependency and used in the cte_engine, it should be used here to ensure robust classification. This is important for correctly identifying 'Import' nodes and displaying their associated unit test inputs in the UI.
fn is_simple_from_select(sql: &str) -> bool {
use sqlparser::dialect::GenericDialect;
use sqlparser::parser::Parser;
use sqlparser::ast::{Statement, SetExpr, TableFactor};
let Ok(ast) = Parser::parse_sql(&GenericDialect {}, sql) else { return false; };
if let [Statement::Query(q)] = ast.as_slice() {
if let SetExpr::Select(s) = &*q.body {
return s.from.len() == 1
&& s.from[0].joins.is_empty()
&& matches!(s.from[0].relation, TableFactor::Table { .. });
}
}
false
}There was a problem hiding this comment.
Tracked to #40. Agreed — the whitespace heuristic does false-positive on select 'from' as col from x and on SELECT '/* from */' FROM x. Folded into the combined renderer-hygiene tracking issue alongside G-4 (same root cause: use sqlparser AST). The post-AST implementation will subsume CodeRabbit's CR-7 micro-perf nit (per-token String allocation) — no more tokenization at all. Full disposition: see my summary comment on the thread.
| let Some(node) = graph.nodes().get(index) else { | ||
| return String::new(); | ||
| }; |
There was a problem hiding this comment.
Returning an empty string when a node index is not found can lead to broken IDs in the JSON payload, which may cause the Mermaid diagram or interactive JS logic to fail silently. If this state is considered unreachable due to invariants in CteGraph, using expect() is preferred to document the invariant and ensure that any violation is caught during development or testing.
let node = graph.nodes().get(index).expect("edge references valid node index");References
- Prioritize explicit defensive guards that document code invariants over more concise idiomatic alternatives if the guard clarifies intent and ensures robustness against future changes in logic.
There was a problem hiding this comment.
Won't fix. Same philosophical disposition as G-1: an empty-string node id would cause one Mermaid edge to render with a stub endpoint (graceful), while expect() would crash the whole render (catastrophic). The CTE engine's acyclicity invariant (from < to) plus the bounds-check at construction time makes this branch logically unreachable; the silent fallback is intentional defense-in-depth. Full disposition: see my summary comment on the thread.
| fn extract_table_leaf_refs(sql: &str) -> Vec<String> { | ||
| let tokens: Vec<&str> = sql.split_whitespace().collect(); | ||
| let mut out = Vec::new(); | ||
| for (i, tok) in tokens.iter().enumerate() { | ||
| let lower = tok.to_ascii_lowercase(); | ||
| if lower != "from" && lower != "join" { | ||
| continue; | ||
| } | ||
| let Some(next) = tokens.get(i + 1) else { | ||
| continue; | ||
| }; | ||
| // Strip surrounding `(` and trailing `,`/`)`/`;`/`(`; take the | ||
| // last `.`-delimited segment; strip `"` quotes. | ||
| let cleaned = next | ||
| .trim_start_matches('(') | ||
| .trim_end_matches([',', ')', ';']); | ||
| let leaf = cleaned.rsplit('.').next().unwrap_or(""); | ||
| let leaf = leaf.trim_matches('"'); | ||
| if leaf.is_empty() { | ||
| continue; | ||
| } | ||
| if !leaf.chars().all(|c| c.is_alphanumeric() || c == '_') { | ||
| continue; | ||
| } | ||
| out.push(leaf.to_ascii_lowercase()); | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
Similar to is_simple_from_select, this function uses a whitespace-based heuristic that is susceptible to false positives and negatives when SQL comments, string literals, or quoted identifiers with spaces are present. This can lead to failures in binding unit test inputs to the correct CTE nodes, causing the report to show 'no fixture provided' even when fixtures are defined. Leveraging the existing sqlparser dependency to traverse the AST would provide a much more reliable implementation for extracting table references.
There was a problem hiding this comment.
Tracked to #40. Agreed — same robustness class as G-2 (whitespace-tokenizing heuristic misses table references inside string literals + quoted identifiers with spaces). Folded into the combined renderer-hygiene tracking issue. Likely needs the CTE engine to expose the per-CTE body's Query AST rather than the renderer re-parsing the string — captured in #40's acceptance criteria. Full disposition: see my summary comment on the thread.
|
@coderabbitai @gemini-code-assist — dispositions on the post-merge findings (per memory Tracked to #40 (combined renderer-hygiene issue, fold into next renderer PR):
Tracked separately to #41 (different scope class — accessibility, not renderer hygiene):
Won't fix:
Things you flagged that look solid — appreciated the explicit callouts on |
|
Tip For best results, initiate chat on the files or code changes.
Glad the escaping, two-pass binding, and CI guard callouts were useful — those were the parts most worth an external validation pass. 🐇 ✨ ✏️ Learnings added
|
1 participant
Closes #11.
PR 8b — implement the v0.1 interactive
report.htmlvia askama 0.16,reproducing the Claude Design hand-back.
What ships
src/adapters/render.rs— composition layer between domaintypes, the CTE engine, and the askama template. Owns per-model
payload assembly, node-role classification (
final/import/transform), clean-import-CTE binding viaref('NAME')parsing,and the run-loop integration.
templates/report.html— askama 0.16 template emitting thedesign's full DOM contract: diff-scope banner, model + test cascade
selectors, CTE DAG host, two-panel region (Node detail / All inputs
· Expected), JSON payload carrier
(
<script type="application/json" id="cute-dbt-data">). Thevendored asset bundle (Sakura · jQuery · DataTables · Mermaid UMD)
is inlined via
include_str!constants fromasset_embed.rs.interaction.jsadapted from the design hand-back verbatim,with the dormant dark-mode infrastructure stripped (YAGNI) and the
Mermaid
<g>selector runtime-constructed(
[id^="${svgId}-flowchart-"], fallback[id*='-flowchart-'])per ADR-4 amendment 2026-05-22.
tests/render_integration.rs— fixture-driven integrationcoverage: asset bundle assembly, design DOM contract, payload
shape, JSON carrier integrity, structured resource-ref egress
lint, and a chrome-only insta snapshot of the rendered
jaffle-shop report.
What was removed
MERMAID_INITconstant + its pin test fromasset_embed.rs— thereal init lives in the inlined interaction script with
startOnLoad: false(ADR-4 amendment).smoke_report_html+ its tests + theescape_label/mermaid_blockhelpers — the real renderer replaces them.tests/asset_embed.rs— replaced bytests/render_integration.rs.ARCHITECTURE.md§6 (synthetic-only fixture invariant) — relocatedto
CONTRIBUTING.md(human contributors) +AGENTS.md(AI agents).The mechanism (
tests/fixtures/MANIFEST.toml+ listed-file gate)is unchanged; the rule is contributor/build-hygiene, not
architectural.
Run-loop wiring
src/cli/mod.rs::renderwidens to threadcurrent/in_scope/models_in_scope/baseline_labelintoadapters::render::render_report.parse_ctes()stays as a namedno-op call site; per-model CTE parsing happens inside the renderer
during payload assembly, so the four-stage diagram
(
scope → preflight_compiled → parse_ctes → render) stays greppable.Discovery #1 — folded in
features/report_generation.featuredropped the stale "And thebanner states the v0.1 fidelity limit 'model body changes'" line.
The body-checksum scoping note moved to
README.md. The other four.featurefiles were scanned and need no corrections.Acceptance criteria
include_str!interaction.jsinlined verbatim, dark-mode stripped, escapehardening preserved (askama's
| jsonfilter escapes<>&')paging: false,info: false,searching: false,scrollX: false,ordering: trueref('NAME')parsingEdgeType(incl.union_all+union_distinctas dashed orange)ARCHITECTURE.md§6; relocate to CONTRIBUTING.md +AGENTS.md
MERMAID_INIT+smoke_report_html+ their testsrenderstepcargo-mutantson the strict scope: 0 surviving mutantsValidation gates (all green locally)
cargo testcargo clippy --all-targets --locked -- -D warningscargo fmt --checkcargo deny checkcargo doc -D warningscargo llvm-cov nextest --fail-under-lines 85lefthook run pre-push --all-filessrc/adapters/render.rscargo mutants --file src/adapters/render.rs🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores