Repository navigation
#157 — fix mobile viewport blowout on stacked given/expected panel - #158
Merged
Merged
Conversation
…ort blowout On a narrow (phone) viewport the report's JS responsive helper collapses the two-column Inspect/Expected `.panel-row` to one column by adding `.is-stacked`. The stacked rule used a bare `1fr` track, which in CSS Grid is shorthand for `minmax(auto, 1fr)`; the `auto` floor sized the single stacked track to the wide given/expected DataTable's min-content, dragging `<body>` past the viewport (horizontal page scroll on mobile). The desktop rule already uses `minmax(0, 1fr)`; the stacked rule dropped that protection. Fix: `.panel-row.is-stacked` track → `minmax(0, 1fr)` so the wide table scrolls inside its own `.table-fit` wrapper as designed. Selector-scoped to `.is-stacked` (added only when a table overflows a column), so desktop is provably unaffected. Regression test: a headless-Chromium 375px-class test renders a report with a wide given AND expect table, both panels populated, forces the reflow stack toggle, and asserts no page-level horizontal overflow plus `.table-fit` containment. On the unfixed CSS it FAILS (documentElement.scrollWidth 2468 vs innerWidth 500); GREEN after the fix. Added as a `#[test]` in tests/headless_toggle.rs so the existing `--test headless_toggle --ignored` CI invocation runs it with no new ci.yml/lefthook wiring. Regenerated the 3 golden example reports + the rendered-chrome insta snapshot (byte-identity gate); the only delta in each is the single grid-template-columns line. Closes #157 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The narrow-launch sanity assert (`innerWidth < 1000`) was tuned to the macOS harness value (~500) and is the only cross-platform-fragile guard. The three load-bearing assertions — `.is-stacked` present, relative `scrollWidth <= innerWidth + 1`, and `.table-fit` containment — are all width-independent (the wide fixture trips the stack at any width), so the width guard adds no coverage and only CI-platform risk. `inner_width` stays live in the scrollWidth comparison. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ 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 (5)
📝 WalkthroughWalkthroughThis PR fixes a mobile viewport blowout in stacked given/expected panels (issue ChangesMobile Viewport Stacked Panel Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
✨ 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/diff-showcase-report.htmlexamples/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 |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit 0bc1190 on Jun 9, 2026 4:18am UTC. |
Contributor
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR doesn't touch ▶ 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 27183553125 -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 addresses a mobile viewport blowout issue by changing the CSS grid track sizing of .panel-row.is-stacked from 1fr to minmax(0, 1fr), ensuring wide tables scroll within their container instead of expanding the page width. It also introduces a headless integration test to verify this layout behavior at a 375px viewport width. The review feedback suggests a minor optimization in the test setup to avoid a redundant PathBuf clone when configuring the browser path.
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
Author
|
Addressed the Gemini medium finding on Gates re-run green locally: Note: CodeRabbit's earlier review hit its rate/review limit and never posted inline findings, so the only actionable inline comment was the Gemini one above (now fixed). @coderabbitai review |
|
Kicking off a fresh review now to cover the full diff (including f8c485c). ✅ Action performedReview finished.
|
This was referenced Jun 10, 2026
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>
cmbays
added a commit
that referenced
this pull request
Jun 11, 2026
…217) * feat(adapters): design-2 layout restructure + DAG node-detail shelf (#201) Restructure the report body around the model -> test split (the pass-2 design canon, merged INTO shipped code - never a file replacement): - .dag-stage wraps .dag-canvas-col (Mermaid + Cytoscape hosts + legend) beside the new aside.dag-shelf (head: title + close; body). Clicking a DAG node opens the shelf: model-detail card from the #200 manifest_nodes lookup (tags -> name+materialized -> description -> model tests -> columns with click-to-expand test-count chips) + compiled SQL with the role badge riding the summary row. Given fixtures are deliberately NOT rendered in the shelf - they live in the Given panel (the #131 ordinal binding rides renderAllInputs' loop index). - Node->model mapping: final-select -> this model; import -> the ref() target of its bound given (the #131 bound_to_node seam); unknown -> card omitted, compiled SQL still shows. - window.__cuteSelectNode now opens the shelf - the #192 Cytoscape tap and the #194 findings pin both keep routing through this one seam (no renderDag per click; Mermaid's activate keeps its re-render). Model/test re-selection closes the shelf and re-centers the DAG. - section.test-section/.test-card below the DAG: the unit-test selector IS the card title (sizeSelectToWidest: static width, max-width clamped), the #91 scope toggle to its right (data-testid retained), the .test-badges row (tb-tag / untagged tb-muted / tb-meta - the overrides badge arrives with #202), the larger-font always-open description + details body. Founder decision on epic #197: always-open card, NOT a drawer; only the authoring-YAML drawer stays collapsible. - .model-findings (#194) preserved INTACT, placed ABOVE the model SQL + DAG near the model selection (founder decision on epic #197, superseding the audit's between-DAG-and-test-section proposal). - test-description-section and the left .panel-toggle (Inspect / All-inputs) retired: the left panel header reads Given and always renders all inputs; the #74 no-empty-landmark intent carries over via the hidden attribute on the in-card description. - The DAG hint subtitle becomes the #146-contract focusable button + CSS bubble (hover AND keyboard focus, aria-label parity). - Kept shipped: theme-aware cyto-dag.js palette, ModelPayload.path in renderModelSql (no <name>.sql regression), the authoring-YAML drawer labels and Diff/File toggles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: port-forward headless guards to the #201 shelf layout + new shelf guard Every re-target is deliberate, never collateral (#201): - findings_panel_renders_checklist_tiers_sketch_rationale_and_pin + cytoscape_hover_card_appears_and_tap_highlights_lineage_in_place: .left-panel-body .node-detail -> .dag-shelf-body .node-detail (the __cuteSelectNode seam survives; the pin still selects + scrolls + pin-flashes). - same_ref_givens / changed_cell_null-vs-string / cell_diff_toggle / fusion-csv / sql-literal / external-fixture / column-tooltip / incremental guards drop the show_all_inputs panel-toggle click (the Given panel always renders every input now); the helper is retired. - incremental_badges_modes_tooltip_and_this_given: BUBBLE_VIS scoped to the expected panel - the DAG hint is a second .expect-tooltip on the page since #201, so a bare first-match read the wrong bubble. - stacked_panel_does_not_blow_out_viewport_at_375px re-verified on the new layout (the .dag-stage flex row wraps below on narrow - the #157/#158 contract holds). - source_given fixture-card guard re-homed: the card renders in the Given panel; clicking the bound import CTE opens the shelf with compiled SQL and NO fixture card (the no-fixtures rule). - NEW dag_node_click_opens_shelf_with_model_card_and_close_clears_selection: shelf opens with compiled SQL + role badge on the summary; the final-select node renders the manifest_nodes model card (description + tags); an unmapped node omits the card but keeps compiled SQL; the close button clears body + baked Mermaid selection; a fresh test selection closes the shelf; the DAG-hint info button honors the #146 focus-reveals-bubble contract. - BDD: the #74 ordering scenario re-homed - the test section (hosting the description) renders between the cte-dag section and the panel-row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: regenerate golden examples + re-accept insta DOM snapshots for #201 examples/{jaffle-shop,playground,diff-showcase}-report.html regenerated with the exact ci.yml example-report-check recipe; every changed byte traces to the #201 templates (report.html DOM, interaction.js, report.css) - audited via git diff --text -U0. No root_path/username bytes. Insta skeleton + chrome snapshots re-accepted for the intended restructure only. 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 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
On a phone the bottom given/expected blocks rendered far wider than the report content above — a horizontal viewport blowout. This clamps the stacked-panel grid track so the wide fixture table scrolls inside its own wrapper instead of dragging the whole page wide.
Root cause
templates/report.html.panel-row.is-stackedused a bare1frtrack. In CSS Grid a bare1fris shorthand forminmax(auto, 1fr), and theautofloor sized the single stacked track to the wide given/expected DataTable's min-content, ballooning<body>past the viewport. The desktop rule (.panel-row) already usesminmax(0, 1fr); the stacked rule had dropped that protection. This is core template CSS, so it affected every report (baseline and--pr-diff).Fix
.panel-row.is-stacked { grid-template-columns: 1fr }→minmax(0, 1fr)— one line. The zero floor lets the table scroll inside its.table-fit(overflow-x: auto) wrapper as designed. The selector is scoped to.is-stacked(added by the JS reflow helper only when a table overflows a column, which never happens on desktop), so desktop is provably unaffected.Before / after
Measured by the new headless regression test, in a
--window-size=375,812-launched headless Chromium window (effectiveinnerWidth≈ 500 in this harness):documentElement.scrollWidth<body>past the viewport<= innerWidth + 1).table-fitwrapperVerification
tests/headless_toggle.rs::stacked_panel_does_not_blow_out_viewport_at_375px): renders a report with a WIDE given AND expect table, both panels populated, launches a narrow headless window, forces the reflow stack toggle, and asserts (a) the.is-stackedpath is exercised, (b) no page-level horizontal overflow (scrollWidth <= innerWidth + 1), (c) the table is contained (.table-fitscrolls internally). Confirmed RED on the unfixed CSS (scrollWidth 2468 > innerWidth) and GREEN after the fix. Added as a#[test]in the already-CI-wiredheadless_togglefile, so the existingcargo test --test headless_toggle -- --ignoredinvocation runs it — no new ci.yml / lefthook wiring needed (and no mirror-drift risk).jaffle-shop,playground,diff-showcase) and therender_integration__rendered_chrome_jaffle_shopinsta snapshot. The only delta in each artifact is the singlegrid-template-columnsline (verified via forced text diff).cargo fmt --all --check,cargo clippy --all-targets --locked -- -D warnings,cargo nextest run --all-targets --locked(731 passed),cargo test --test headless_toggle -- --ignored(26 passed, incl. the new test),cargo test --test headless_zero_egress -- --ignored(no regression),cargo test --test bdd(85 scenarios / 543 steps),RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --locked,cargo deny check, andlefthook run pre-push(all hooks pass).Deferred (cosmetic 🟡)
The diagnosis flagged two minor cosmetic mobile items — the settings-cog dropdown sitting near the right edge, and the expect-tooltip bubble potentially clipping on a right-edge trigger. Neither causes a page-level blowout. Per the minimal-proven-fix principle, this PR keeps the diff to the one-line grid-template fix + its regression test + the mandatory regen, so the byte-diff is trivially auditable. The two cosmetic items can be folded into a separate mobile-polish pass.
Closes #157
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests