fix(layout): make .sf-bento and .sf-grid-cols-N actually respond to their container - #607
Conversation
…heir container Bento grid on mobile wasn't collapsing to a single column, and the root cause is bigger than mobile: .sf-bento and .sf-grid-cols-2/3/4/6 each declared `container: <name> / inline-size` on themselves and then used `@container` to change their OWN grid-template-columns. A size container can never be the subject of its own @container query — per spec, conditions only ever match descendants of the nearest ancestor container — so this was a silent no-op at every viewport, not just mobile. Confirmed empirically: .sf-grid-cols-4 rendered as a single 319px-wide column even inside a 2000px-wide ancestor. Fix: drop the self-established container from both primitives so their (already-unnamed) @container queries fall through to whatever ancestor container exists (.sf-container/.sf-cq/.sf-fluid-cq) — the same pattern .sf-alternate and .sf-grid-cols-1-2/2-1/1-3/3-1 already use correctly for restyling their children. This is what the framework's own docs describe ("layout primitives respond to @container") but the self-querying bug prevented in practice. Once the breakpoint actually fired, a second bug surfaced: .sf-bento-wide/-featured request `grid-column: span 2` unconditionally, which forces an implicit 2nd column even inside the 1-column mobile grid. Reset both to span 1 inside the same breakpoint (placed after the modifier declarations so it wins the equal-specificity/source-order tiebreak against them). .sf-grid-cols-2/3/4/6 had the identical self-query bug — fixed alongside .sf-bento since it's the exact same root cause and fix, one file, ~60 lines apart. Verified in a real browser at mobile/tablet/desktop container widths: single column + no overflow on mobile, 2 columns at the tablet breakpoint, full column count above it, spanning children never overflow. Added regression tests for the breakpoints themselves — the existing .sf-bento tests only checked --sf-bento-cols overrides, which route through the (unaffected) base rule and never exercised the broken @container path at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0113XBTydmejAnYJt9CTyYRc
… fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0113XBTydmejAnYJt9CTyYRc
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdates grid and bento primitives to use ancestor container queries, adds a mobile single-column span override for bento variants, and adds Playwright coverage for responsive column behavior. ChangesContainer Query Layout Behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryThis PR fixes a fundamental CSS container-query bug:
Confidence Score: 4/5Safe to merge; the CSS change is a genuine correction of a spec-invalid construct and the cascade ordering of the new mobile-override block is correct. The root fix is well-reasoned and correctly implemented — removing the self-established container makes the @container rules actually fire for the first time, and the second @container block that resets spanning children is placed after the base modifier rules so it wins on source order as intended. The two test gaps leave a small surface uncovered but do not indicate a defect in the CSS itself. tests/layout.spec.js — the phantom-column fix for .sf-bento-featured and the intermediate breakpoint behaviour of .sf-grid-cols-4 have no test coverage. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Browser evaluates @container rule\nfor .sf-bento / .sf-grid-cols-N"] --> B{Does element\nestablish a container?}
B -->|"Before fix:\nYES — container on itself"| C["Walks up to nearest ancestor\ncontainer — ITSELF"]
C --> D["Self-query: silent no-op\n(spec: container can't match itself)\nNo responsive columns — ever"]
B -->|"After fix:\nNO — container removed"| E["Walks up to nearest\nancestor container"]
E --> F[".sf-container / .sf-cq / .sf-fluid-cq"]
F --> G["Container width measured correctly"]
G --> H{Width check}
H -->|"< 30em"| I["grid-template-columns: 1fr\n+ span children reset to span 1"]
H -->|"30em–48em"| J[".sf-grid-cols-4: repeat(2,1fr)\n.sf-bento: repeat(2,1fr)"]
H -->|"> 48em"| K["Full column count\nrepeat(N, 1fr)"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Browser evaluates @container rule\nfor .sf-bento / .sf-grid-cols-N"] --> B{Does element\nestablish a container?}
B -->|"Before fix:\nYES — container on itself"| C["Walks up to nearest ancestor\ncontainer — ITSELF"]
C --> D["Self-query: silent no-op\n(spec: container can't match itself)\nNo responsive columns — ever"]
B -->|"After fix:\nNO — container removed"| E["Walks up to nearest\nancestor container"]
E --> F[".sf-container / .sf-cq / .sf-fluid-cq"]
F --> G["Container width measured correctly"]
G --> H{Width check}
H -->|"< 30em"| I["grid-template-columns: 1fr\n+ span children reset to span 1"]
H -->|"30em–48em"| J[".sf-grid-cols-4: repeat(2,1fr)\n.sf-bento: repeat(2,1fr)"]
H -->|"> 48em"| K["Full column count\nrepeat(N, 1fr)"]
Reviews (1): Last reviewed commit: "docs(changelog): note the .sf-bento / .s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| test('a spanning child does not force a phantom 2nd column at the mobile breakpoint', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:300px"> | ||
| <div class="sf-bento"> | ||
| <div id="w" class="sf-bento-wide">wide</div> | ||
| <div>B</div> | ||
| </div> | ||
| </div> | ||
| `); | ||
| const [gridWidth, itemWidth] = await Promise.all([ | ||
| page.locator('.sf-bento').evaluate(el => el.getBoundingClientRect().width), | ||
| page.locator('#w').evaluate(el => el.getBoundingClientRect().width), | ||
| ]); | ||
| expect(itemWidth).toBeLessThanOrEqual(gridWidth + 1); | ||
| }); |
There was a problem hiding this comment.
.sf-bento-featured mobile overflow not exercised
The CSS fix resets both .sf-bento-wide and .sf-bento-featured to span 1 at the mobile breakpoint, but only .sf-bento-wide is covered by this test. A featured child carries grid-column: span 2; grid-row: span 2;, so an item-width assertion analogous to the sf-bento-wide case would confirm its span 2 is also being overridden correctly and not creating a phantom column.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| test('stays 1 column below the breakpoint', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:300px"> | ||
| <div id="g" class="sf-grid-cols-4"><div>1</div><div>2</div><div>3</div><div>4</div></div> | ||
| </div> | ||
| `); | ||
| const cols = await page.locator('#g').evaluate(el => | ||
| getComputedStyle(el).gridTemplateColumns.split(' ').length | ||
| ); | ||
| expect(cols).toBe(1); | ||
| }); |
There was a problem hiding this comment.
At 300px, no grid-template-columns rule is set for .sf-grid-cols-4 (neither @container breakpoint fires), so getComputedStyle(el).gridTemplateColumns returns "none", and "none".split(' ').length === 1 is trivially true — the test would pass even without an ancestor container at all. A complementary test at 30em–47.99em (e.g. 600px) would verify the intermediate 2-column state and give the "stays below 30em" assertion real meaning by contrast.
…id breakpoint Greptile review on PR #607 caught two real gaps in the new tests: - Only .sf-bento-wide was checked for the phantom-column overflow fix; .sf-bento-featured carries the same grid-column: span 2 (plus grid-row: span 2) and needs the identical assertion. - The "stays 1 column below the breakpoint" test for .sf-grid-cols-4 passed for the wrong reason: below 30em neither @container rule matches at all, so gridTemplateColumns falls back to the unset "none" — which also satisfies a naive length-1 check even with a completely broken ancestor container. Added a mid-range (30-48em) case that only passes if the container query genuinely fired. Verified both against the real built dist/slashed.optimal.css bundle before committing (same sandbox limitation as the prior commit — no chrome-headless-shell binary here for the project's own Playwright runner). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0113XBTydmejAnYJt9CTyYRc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/layout.spec.js (1)
647-709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a mid-range bento test (30–48em → 2 columns).
The grid-cols suite includes a mid-range test (line 744) that verifies the 30em breakpoint fires against an ancestor container, but the bento suite only tests mobile (300px → 1 col) and desktop (900px → >1 col). A mid-range test at ~600px asserting 2 columns would close the same coverage gap and confirm the
@container (min-width: 30em) and (max-width: 47.99em)query at line 596 resolves correctly.🧪 Suggested mid-range bento test
test('uses the wider column count once past the mobile breakpoint', async ({ page }) => { await setup(page, ` <div style="container-type:inline-size; width:900px"> <div id="g" class="sf-bento"><div>A</div><div>B</div></div> </div> `); const cols = await page.locator('`#g`').evaluate(el => getComputedStyle(el).gridTemplateColumns.split(' ').length ); expect(cols).toBeGreaterThan(1); }); + + test('uses 2 columns in the mid-range between 30em and 48em', async ({ page }) => { + await setup(page, ` + <div style="container-type:inline-size; width:600px"> + <div id="g" class="sf-bento"><div>A</div><div>B</div></div> + </div> + `); + const cols = await page.locator('`#g`').evaluate(el => + getComputedStyle(el).gridTemplateColumns.split(' ').length + ); + expect(cols).toBe(2); + }); });🤖 Prompt for 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. In `@tests/layout.spec.js` around lines 647 - 709, Add a mid-range test to the bento grid suite around the existing mobile and desktop tests, using an ancestor inline-size container of approximately 600px and a .sf-bento with multiple children. Assert that computed gridTemplateColumns contains exactly 2 columns, verifying the 30em-to-47.99em container query resolves against the ancestor.
🤖 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 `@core/layout.css`:
- Around line 14-17: Update the header comment in core/layout.css to remove
`.sf-grid-cols-1-2` from the group described as establishing a container and
querying to restyle children. Describe it with the ancestor-container pattern
alongside `.sf-grid-cols-2/3/4/6` and `.sf-bento`, while preserving the existing
explanation for `.sf-alternate` and other self-established container components.
---
Nitpick comments:
In `@tests/layout.spec.js`:
- Around line 647-709: Add a mid-range test to the bento grid suite around the
existing mobile and desktop tests, using an ancestor inline-size container of
approximately 600px and a .sf-bento with multiple children. Assert that computed
gridTemplateColumns contains exactly 2 columns, verifying the 30em-to-47.99em
container query resolves against the ancestor.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa4ce0c3-774a-4b0e-a511-cade88a00a2c
📒 Files selected for processing (3)
CHANGELOG.mdcore/layout.csstests/layout.spec.js
CI failed on Firefox and WebKit: the "resolves its column count against an ancestor container" test for .sf-grid-cols-4 expected 4 columns at a 900px ancestor width but got 2. Root cause: `em` inside an @container condition resolves against the container's inherited font-size, and that's the fluid `--sf-text-m` here (~19px at this viewport), not a fixed 16px, because `body { font-size: var(--sf-text-m) }`. 900px sits close enough to the resulting ~48em boundary that Chromium and Firefox/WebKit disagreed on which side of it they landed. Not a product bug — just three engines evaluating a clamp()/pow() formula with slightly different floating-point precision right at a boundary. Fixed by widening the margins so the outcome is unambiguous regardless of engine: the "past the breakpoint" cases move from 900px to 1600px, the "mid-range" cases move from 600px to 750px (both grid-cols-4 and the new .sf-bento mid-range case CodeRabbit suggested). Verified all five breakpoint assertions against the real built bundle in Chromium before pushing (same sandbox limitation as prior commits — no Firefox/WebKit binaries here to reproduce the actual failure locally). Also tightened the SL-034 header comment per a CodeRabbit finding: it incorrectly grouped .sf-grid-cols-1-2 with .sf-alternate as "query a self-established container to restyle children" — .sf-grid-cols-1-2 never declared its own `container` at all (same ancestor-container pattern as the fixed primitives, just already correct), unlike .sf-alternate which genuinely does self-establish one for its children. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0113XBTydmejAnYJt9CTyYRc
2 participants
Summary
.sf-bentowas reported not collapsing to a single column on mobile. The actual root cause is bigger than mobile:.sf-bentoand.sf-grid-cols-2/3/4/6each declaredcontainer: <name> / inline-sizeon themselves and then used@containerto change their owngrid-template-columns. A size container can never be the subject of its own@containerquery — per spec, conditions only ever match descendants of the nearest ancestor container — so this was a silent no-op at every viewport, not just mobile. Confirmed empirically in a real browser:.sf-grid-cols-4rendered as a single 319px-wide column even inside a 2000px-wide ancestor container.containerfrom both primitives so their (already-unnamed)@containerqueries fall through to whatever ancestor container exists (.sf-container/.sf-cq/.sf-fluid-cq) — the same pattern.sf-alternateand.sf-grid-cols-1-2/2-1/1-3/3-1already use correctly for restyling their children. This is what the framework's own docs describe ("layout primitives respond to@container") but the self-querying bug prevented in practice..sf-bento-wide/-featuredrequestgrid-column: span 2unconditionally, forcing an implicit 2nd column even inside the 1-column mobile grid. Reset both tospan 1inside the same breakpoint..sf-grid-cols-2/3/4/6had the identical self-query bug — fixed alongside.sf-bentosince it's the exact same root cause and fix, ~60 lines apart in the same file.SL-034note at the top ofcore/layout.cssdocumenting the self-query constraint for future maintainers, following the file's existingSL-00Nconvention.Verified in a real browser at mobile/tablet/desktop container widths: single column + no overflow on mobile, 2 columns at the tablet breakpoint, full column count above it, spanning children never overflow.
Type
Checklist
feat:,fix:,docs:, …) — enforced by commitlintnpm run lint:csspasses (stylelint)npm run buildrebuildsdist/(bundles are git-ignored; CI rebuilds and stamps headers)npm run test:unitpasses (root); Playwright e2e couldn't be run in this sandbox (missing headless-shell binary) but was verified directly against the built bundle with the pre-installed Chromium executable — see PR notesnpm run check:version) — no version bump in this PRnpm run check:llm-guide) — no token/class API change, guide already describes this (previously broken) behavior correctlynpm run check:macros,check:registry,audit:check)CHANGELOG.mdupdated under## [Unreleased]Notes
.sf-bentotests intests/layout.spec.jsnever actually exercised the@containerbreakpoints (they only checkeddisplay:grid/--sf-bento-colsoverrides, which route through the base rule unaffected by this bug) — added new tests that do, plus a new.sf-grid-cols-2/3/4/6describe block (previously untested).chrome-headless-shellbinary that isn't installed here, sonpm test's e2e step couldn't be run locally. Every new/changed assertion intests/layout.spec.jswas independently verified against the real builtdist/slashed.optimal.cssbundle using the pre-installed Chromium executable before committing. CI has full browser installs and will run the real suite.https://claude.ai/code/session_0113XBTydmejAnYJt9CTyYRc
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests