Repository navigation
#159 — strategy-correct incremental expect-semantics tooltip - #161
Merged
Merged
Conversation
The expect-semantics tooltip (templates/report.html) said Expected is "the rows that will be merged or inserted ... (the final table after the merge)". That wording is merge/append-centric: it is mechanically wrong for `insert_overwrite` (replaces whole partitions — nothing merges) and `microbatch` (per-window batches), and loose for `delete+insert`. Generalize to the strategy-invariant truth (true for all five strategies — append/merge/delete+insert/insert_overwrite/microbatch): "Expected is the output of the model's compiled SELECT on the incremental branch — the rows the configured incremental strategy will apply to the table — not the table's final state after the run." A dbt unit test compares the compiled SELECT for the chosen is_incremental branch and never exercises the materialization strategy (dbt-core#8664), so this is a copy-precision correctness fix, not a per-strategy code path. The single `tip` string drives both the visible CSS bubble and the aria-label (the #146 focusable-button pattern), so one edit keeps them in sync. - headless: extend incremental_badges_modes_tooltip_and_this_given to assert the new strategy-invariant phrase renders in BOTH the bubble and the aria-label, and that the old "merged or inserted" wording is absent (RED before the template edit, GREEN after). - BDD: update incremental_models.feature prose + matching step regexes to strategy-generic phrasing (the step bodies assert the is_incremental_mode payload bool — behaviour unchanged). - byte-identity regen: the 3 golden examples (jaffle-shop/playground/diff-showcase) + the chrome HTML insta snapshot — intended-only (the 3-line tip flip, nothing else). - dogfood: add explicit incremental_strategy='merge' to the live dbt-project order_events_incremental model (config-only; `dbt parse` validated) so the --pr-diff self-dogfood exercises the tooltip on a fresh CI-compiled manifest. Closes #159 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 38 minutes and 55 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 (8)
✨ 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 5 individual chapters for you: 
Chapters generated by Stage for commit 44a5f2d on Jun 9, 2026 5:29am UTC. |
![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 updates the incremental model unit-test tooltip copy to be strategy-invariant, making it accurate for all five incremental strategies instead of being specific to merge or append. The changes are applied across the HTML report template, Cucumber feature files, step definitions, integration test snapshots, and headless tests. There are no review comments, and I have no feedback to provide.
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
📄 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 27185910075 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
This was referenced Jun 10, 2026
Closed
Closed
cmbays
added a commit
that referenced
this pull request
Jun 10, 2026
…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>
This was referenced Jun 11, 2026
cmbays
added a commit
that referenced
this pull request
Jun 11, 2026
… reconstruction, overrides badge, info tips (#220) Three body-appended singleton tips (#model-tooltip / #fmt-tooltip / #ov-tooltip) on mouseenter AND focusin behind focusable triggers: the Given header renders ref( + .gt-model pill + ), the Expected panel gains its model pill (header fallback or fixture-view bar, bar order [model pill][mode badge][row count] with the Diff/File toggle far right), the format badge shows for ALL formats incl. dict and reconstructs the fixture client-side (sql/csv/yaml), and the test badge row gains the 'overrides . N' badge with grouped key = value rows. Founder decision (epic #197) applied: the incremental-branch mode badge and the prior-model-state this-badge keep the #146 focusable-trigger CSS-bubble mechanism with the pass-2 badge-borne styling - the separate expected-panel i button is retired, the pass-2's JS info-tip twin for those badges is not ported (exactly one mechanism), and the LOCKED D4 invariant holds (tooltip rides is_incremental_mode === true, never the this-given proxy, never full-refresh). Column-header tooltips: detail strings now render as ct-vals/ct-val chips (ct-detail retired); per-table column_meta sourcing unchanged. The #161 idempotent clear-list extends to .expected-model-badge. Every interpolated tip value passes through escapeHtml (payload strings are attacker-adjacent YAML). Goldens regenerated (template-only deltas, payload byte-stable, double-render byte-identical); explore pages untouched. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 11, 2026
cmbays
added a commit
that referenced
this pull request
Jun 11, 2026
…ted badges left, 13.44px tip text (#234) * fix(adapters): pass-2 conformance — edge-aware tooltip bubbles, expected badges left, 13.44px tip text (#232) Executes the three fix recipes from the 2026-06-11 design-conformance audit (epic #197 pass-2 spec): - R2 (audit D2): renderExpectedPanel always emits the .fixture-view-bar meta row for table renders — [model pill][mode badge][N rows] reading left under the Expected title, mirroring Given; the right-aligned, order-reversed .panel-header fallback is retired (sql-format / external-fixture early-return paths keep the header placement, spec-consistent). The #178 persistent-rowcount rescue is untouched. - R1 (audit D1): edge-aware CSS bubbles — belt-and-braces right-anchor for header-placed badges (the spec's base.css precedent), a geometry- only data-tip-edge tagger on the delegated mouseenter/focusin path (visibility stays pure CSS — the #146/#161 contract intact), and the shared bubble max-width capped at min(70vw, calc(100vw - 16px)). Opportunistic D18: positionTipNear resets left before measuring offsetWidth (stale-width clamp overshoot). - R3 (audit D3): bubble text 0.78rem (7.8px at Sakura's 62.5% root) → 13.44px, matching the column-tooltip description (12px shell base × 1.12em). Headless: three new RED-first pins (375px-class viewport containment, computed-size equality vs #col-tooltip .ct-desc, no-diff meta-row structure) + existing placement pins updated to the spec-true bar location; the #146 focus-reveals-bubble guard stays green. Goldens regenerated with the CI commands; every byte delta traces to the R1/R2/R3/D18 blocks. Closes #232 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: 1px tolerance on #232 viewport-edge assertions (review suggestion) getBoundingClientRect returns fractional px in headless Chrome; the exact bounds (right <= vw, left >= 0.0) were a flake vector. 1px tolerance mirrors the #157 sub-pixel precedent — the guarded regression is 23-47px of clipping, so the teeth are intact. 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>
cmbays
added a commit
that referenced
this pull request
Jun 11, 2026
…l-tip ct anatomy (#236) Concern 1 — given column-header tooltips: the JS binder never regressed; the given-side column_meta payload (#165/#166) only ever resolved ref()-to-MODEL and 'this' inputs. A seed-ref given (the committed jaffle-shop ref('raw_customers') wire shape) and a source('a','b') given carried NO column metadata, so their headers offered no tooltip while the expected table's did — the symptom found reviewing PR #230. Fix: - domain: SourceNode ingests authored per-column descriptions (with_column_descriptions builder, the Node precedent; fusion ManifestSource.columns via serialize_dbt_columns, dbt-schemas manifest_nodes.rs @ 9977b6cb, verified on the committed playground fixture's synthea_raw.patients.Id) - adapter: WireSource parses the columns map (empty-string unset descriptions dropped, the #165 rule) - render: resolve_given_ref_node resolves givens over dbt's refable set (model | seed | snapshot); column_meta_for_source merges source column descriptions + column-scoped tests attached to the source node; shared attached_column_tests keeps both arms deterministic - honest degrade preserved: a metadata-less given column renders NO trigger — never an empty bubble Concern 2 — model-badge tooltip anatomy: the model-ref tip's test rows now ride the COLUMN tooltip's own .ct-test/.ct-key/.ct-vals anatomy via the shared ctTestHtml builder (never a parallel chip system); test names keep the readable-on-dark color-mix accent treatment; the .mt-mtests/.dt-* dress is retired inside the tip (the shelf card keeps its light-surface .dt-* chips). The .col-tooltip .ct-key color stays #233's lane — untouched. Guards: 3 new headless tests (seed+source given tooltips on hover AND focus per the #146/#161 contract, honest-degrade no-empty-bubble, model tip chip anatomy + computed accent color), 3 new render payload tests, domain/adapter ingestion tests; the #202 model-tip pin updated to the new anatomy. Goldens regenerated (the playground source given now ships a real Id tooltip); insta chrome snapshot rebaselined. Closes #235 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 11, 2026
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
The incremental expect-semantics tooltip (
templates/report.html, the #146 focusable-button + CSS-bubble) was merge/append-centric. It is mechanically wrong forinsert_overwrite(replaces whole partitions — nothing merges) andmicrobatch(per-window batches), and loose fordelete+insert. A dbt unit test compares the compiled SELECT for the chosenis_incrementalbranch and never exercises the materialization strategy (dbt-core#8664), so this is a copy-precision correctness fix — not a per-strategy code path (Bucket 1a, Bar A).Before / after copy
Before:
After (strategy-invariant — true for all 5 strategies):
The single
tipvar drives both the visible CSS bubble and thearia-label(the #146 pattern), so one edit keeps them in sync.Changes
tipstring (one edit → bubble + aria-label).tests/headless_toggle.rs): extendincremental_badges_modes_tooltip_and_this_givento assert the new strategy-invariant phrase renders in both the bubble and the aria-label, and that the oldmerged or insertedwording is absent. RED before the template edit, GREEN after.features/incremental_models.feature+ steps): update prose + matching step regexes to strategy-generic phrasing (step bodies still assert theis_incremental_modepayload bool — behaviour unchanged).jaffle-shop/playground/diff-showcase) + the chrome HTML insta snapshot. Intended-only — verified each is exactly the 3-line tip flip, no unrelated churn.incremental_strategy='merge'to the livedbt-project/models/marts/order_events_incremental.sql(config-only;dbt parsevalidated) so the--pr-diffself-dogfood exercises the tooltip on a fresh CI-compiled manifest.Verification
All gates green locally (lefthook skipped on the worktree push-range quirk, so each gate run directly):
cargo fmt --all --check— EXIT 0cargo clippy --all-targets --locked -- -D warnings— EXIT 0cargo nextest run— 731 passed, 27 skippedcargo test --test bdd— 85 scenarios / 543 steps passedRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --locked— EXIT 0-- --ignored(RED→GREEN) +headless_zero_egress(no regression) — passcargo deny check— advisories/bans/licenses/sources oknon-mirror-guard,baseline-required-grep,feature-count(12),mdbook build book,fixture-manifestgates — all passgit diff --text -U0(each example: -3 +3, the tip flip only)Closes #159
🤖 Generated with Claude Code