feat(bento): Phase Ic — responsive polish and visual regression tests#75
Open
feat(bento): Phase Ic — responsive polish and visual regression tests#75
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideImplements responsive polish for the bento grid (mobile stat pills horizontal scrolling) and adds HTML-structure-focused visual regression tests to validate the new homepage bento grid layout and remove reliance on old components. Sequence diagram for bento visual regression structural testssequenceDiagram
actor Developer
participant JestRunner
participant BentoVisualTestSuite
participant HomePageRenderer
participant DOM
Developer->>JestRunner: run bento-visual.test
JestRunner->>BentoVisualTestSuite: execute test_hero_structure
BentoVisualTestSuite->>HomePageRenderer: render_homepage_bento
HomePageRenderer-->>DOM: HTML for homepage grid
BentoVisualTestSuite->>DOM: query_hero_bento_cells
DOM-->>BentoVisualTestSuite: hero_nodes
BentoVisualTestSuite->>JestRunner: assert_hero_structure
JestRunner->>BentoVisualTestSuite: execute test_stats_structure
BentoVisualTestSuite->>HomePageRenderer: render_homepage_bento
HomePageRenderer-->>DOM: HTML for homepage grid
BentoVisualTestSuite->>DOM: query_stat_pills
DOM-->>BentoVisualTestSuite: stat_pill_nodes
BentoVisualTestSuite->>JestRunner: assert_stat_pill_structure
JestRunner->>BentoVisualTestSuite: execute test_absence_of_legacy_components
BentoVisualTestSuite->>DOM: query_legacy_components
DOM-->>BentoVisualTestSuite: empty_node_list
BentoVisualTestSuite->>JestRunner: assert_legacy_components_absent
Updated class diagram for bento visual regression test structureclassDiagram
class HomePageBentoGridComponent {
+string id
+string layoutMode
+renderDesktop()
+renderTablet()
+renderMobile()
}
class BentoVisualTestSuite {
+testHeroStructure()
+testStatsStructure()
+testFeaturedCardsStructure()
+testControlsStructure()
+testPersonasStructure()
+testCtaStructure()
+testNoLegacyComponents()
+renderHomepageBento(layoutMode)
+queryHeroCells(dom)
+queryStatPills(dom)
+queryFeaturedCards(dom)
+queryControls(dom)
+queryPersonas(dom)
+queryCta(dom)
+queryLegacyComponents(dom)
}
class CSSStatPillsMobileRule {
+string selector
+string maxWidthBreakpoint
+bool flexWrap
+string overflowX
+bool webkitOverflowScrolling
+string paddingBottom
}
class DOM {
+querySelectorAll(selector)
}
BentoVisualTestSuite --> HomePageBentoGridComponent : renders
BentoVisualTestSuite --> DOM : queries
BentoVisualTestSuite --> CSSStatPillsMobileRule : validates_effect_on_structure
CSSStatPillsMobileRule : selector = .stat-pills
CSSStatPillsMobileRule : maxWidthBreakpoint = 767px
CSSStatPillsMobileRule : flexWrap = false
CSSStatPillsMobileRule : overflowX = auto
CSSStatPillsMobileRule : webkitOverflowScrolling = true
CSSStatPillsMobileRule : paddingBottom = var(--space-2xs)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The visual regression tests repeatedly read and parse
dist/index.htmlin eachitblock; consider reading it once (e.g., in abeforeAllor a sharedconst html = ...) to reduce duplication and make the tests a bit faster and easier to maintain. - The new bento structural tests assert on raw string contents like
Anthony James PadavanoandStart a conversation, which may be brittle as copy changes; you might want to key off more stable selectors or data attributes instead. - For the HTML structure checks, using a DOM parser (e.g., JSDOM) instead of regex/string matching would make the tests more robust to formatting changes and allow more precise assertions on element counts and attributes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The visual regression tests repeatedly read and parse `dist/index.html` in each `it` block; consider reading it once (e.g., in a `beforeAll` or a shared `const html = ...`) to reduce duplication and make the tests a bit faster and easier to maintain.
- The new bento structural tests assert on raw string contents like `Anthony James Padavano` and `Start a conversation`, which may be brittle as copy changes; you might want to key off more stable selectors or data attributes instead.
- For the HTML structure checks, using a DOM parser (e.g., JSDOM) instead of regex/string matching would make the tests more robust to formatting changes and allow more precise assertions on element counts and attributes.
## Individual Comments
### Comment 1
<location path="src/__tests__/bento-visual.test.ts" line_range="7" />
<code_context>
+
+const DIST = resolve(process.cwd(), 'dist');
+
+describe('bento grid structure', () => {
+ const indexPath = resolve(DIST, 'index.html');
+
</code_context>
<issue_to_address>
**suggestion:** Reuse the built HTML instead of reading index.html from disk in every test
Each test re-reads `dist/index.html`, adding unnecessary I/O and duplication. Consider loading it once in a `beforeAll` (e.g., into a shared `let html: string`) and reusing it across tests to keep them focused on structure and simplify future changes (like switching entry files).
Suggested implementation:
```typescript
import { existsSync, readFileSync } from 'fs';
import { resolve } from 'path';
import { beforeAll, describe, expect, it } from 'vitest';
const DIST = resolve(process.cwd(), 'dist');
describe('bento grid structure', () => {
const indexPath = resolve(DIST, 'index.html');
let html: string;
beforeAll(() => {
html = readFileSync(indexPath, 'utf-8');
});
it('dist/index.html exists', () => {
expect(existsSync(indexPath)).toBe(true);
});
it('has bento-grid section', () => {
```
Anywhere later in this file that calls `readFileSync(indexPath, ...)` or otherwise re-reads `dist/index.html` should be updated to use the shared `html` variable instead. For example, replace `const html = readFileSync(indexPath, 'utf-8');` with uses of the top-level `html` defined in the `describe` block (e.g., directly passing `html` into the DOM parser or string matchers).
</issue_to_address>
### Comment 2
<location path="src/__tests__/bento-visual.test.ts" line_range="27-32" />
<code_context>
+ expect(html).toContain('stat-pill');
+ });
+
+ it('has 2 featured project cards with transition:name', () => {
+ const html = readFileSync(indexPath, 'utf8');
+ const featured = html.match(/bento-card--featured/g);
+ expect(featured?.length).toBe(2);
+ expect(html).toContain('data-astro-transition-scope');
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Tighten featured cards assertion to ensure the transition scope is applied specifically to featured cards
This test only checks that `data-astro-transition-scope` exists somewhere in the HTML, so it would still pass if the attribute moved off the featured cards. Please assert that it’s present specifically on the `bento-card--featured` elements (or that the count of `data-astro-transition-scope` matches the number of featured cards) so the test better reflects the intended behavior.
```suggestion
it('has 2 featured project cards with transition:name', () => {
const html = readFileSync(indexPath, 'utf8');
// Count all featured cards
const featured = html.match(/bento-card--featured/g) ?? [];
expect(featured.length).toBe(2);
// Ensure each featured card has a data-astro-transition-scope attribute
const featuredWithTransitionScope =
html.match(/<[^>]*class="[^"]*\bbento-card--featured\b[^"]*"[^>]*data-astro-transition-scope[^>]*>/g) ?? [];
expect(featuredWithTransitionScope.length).toBe(featured.length);
});
```
</issue_to_address>
### Comment 3
<location path="src/__tests__/bento-visual.test.ts" line_range="41-45" />
<code_context>
+ expect(html).toContain('data-shibui-control');
+ });
+
+ it('has persona cell with 4 persona links', () => {
+ const html = readFileSync(indexPath, 'utf8');
+ expect(html).toContain('persona-cell');
+ const personaLinks = html.match(/persona-cell__link/g);
+ expect(personaLinks?.length).toBeGreaterThanOrEqual(4);
+ });
+
</code_context>
<issue_to_address>
**question (testing):** Clarify whether exactly 4 personas are expected and assert accordingly
The test name implies exactly 4 persona links, but the assertion uses `toBeGreaterThanOrEqual(4)`. Please either change the assertion to `toBe(4)` if exactly four are required, or update the test name to indicate an "at least 4" expectation.
</issue_to_address>
### Comment 4
<location path="src/__tests__/bento-visual.test.ts" line_range="66-71" />
<code_context>
+ expect(html).not.toContain('organ-group__toggle');
+ });
+
+ it('has dual-view data attributes', () => {
+ const html = readFileSync(indexPath, 'utf8');
+ expect(html).toContain('data-hero-view="engineering"');
+ expect(html).toContain('data-hero-view="creative"');
+ expect(html).toContain('data-stats-view="engineering"');
+ expect(html).toContain('data-stats-view="creative"');
+ });
+});
</code_context>
<issue_to_address>
**issue (testing):** Add a test to assert that bento cells do not use `data-reveal`, as mentioned in the PR description
The PR description specifies “No `data-reveal` on bento cells (above-fold, no scroll trigger needed)”, but there’s no test enforcing this. Please add a test that asserts `data-reveal` never appears on bento cell elements (or anywhere in the document, if that’s the requirement) to capture this behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| const DIST = resolve(process.cwd(), 'dist'); | ||
|
|
||
| describe('bento grid structure', () => { |
There was a problem hiding this comment.
suggestion: Reuse the built HTML instead of reading index.html from disk in every test
Each test re-reads dist/index.html, adding unnecessary I/O and duplication. Consider loading it once in a beforeAll (e.g., into a shared let html: string) and reusing it across tests to keep them focused on structure and simplify future changes (like switching entry files).
Suggested implementation:
import { existsSync, readFileSync } from 'fs';
import { resolve } from 'path';
import { beforeAll, describe, expect, it } from 'vitest';
const DIST = resolve(process.cwd(), 'dist');
describe('bento grid structure', () => {
const indexPath = resolve(DIST, 'index.html');
let html: string;
beforeAll(() => {
html = readFileSync(indexPath, 'utf-8');
});
it('dist/index.html exists', () => {
expect(existsSync(indexPath)).toBe(true);
});
it('has bento-grid section', () => {Anywhere later in this file that calls readFileSync(indexPath, ...) or otherwise re-reads dist/index.html should be updated to use the shared html variable instead. For example, replace const html = readFileSync(indexPath, 'utf-8'); with uses of the top-level html defined in the describe block (e.g., directly passing html into the DOM parser or string matchers).
Comment on lines
+27
to
+32
| it('has 2 featured project cards with transition:name', () => { | ||
| const html = readFileSync(indexPath, 'utf8'); | ||
| const featured = html.match(/bento-card--featured/g); | ||
| expect(featured?.length).toBe(2); | ||
| expect(html).toContain('data-astro-transition-scope'); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Tighten featured cards assertion to ensure the transition scope is applied specifically to featured cards
This test only checks that data-astro-transition-scope exists somewhere in the HTML, so it would still pass if the attribute moved off the featured cards. Please assert that it’s present specifically on the bento-card--featured elements (or that the count of data-astro-transition-scope matches the number of featured cards) so the test better reflects the intended behavior.
Suggested change
| it('has 2 featured project cards with transition:name', () => { | |
| const html = readFileSync(indexPath, 'utf8'); | |
| const featured = html.match(/bento-card--featured/g); | |
| expect(featured?.length).toBe(2); | |
| expect(html).toContain('data-astro-transition-scope'); | |
| }); | |
| it('has 2 featured project cards with transition:name', () => { | |
| const html = readFileSync(indexPath, 'utf8'); | |
| // Count all featured cards | |
| const featured = html.match(/bento-card--featured/g) ?? []; | |
| expect(featured.length).toBe(2); | |
| // Ensure each featured card has a data-astro-transition-scope attribute | |
| const featuredWithTransitionScope = | |
| html.match(/<[^>]*class="[^"]*\bbento-card--featured\b[^"]*"[^>]*data-astro-transition-scope[^>]*>/g) ?? []; | |
| expect(featuredWithTransitionScope.length).toBe(featured.length); | |
| }); |
Comment on lines
+41
to
+45
| it('has persona cell with 4 persona links', () => { | ||
| const html = readFileSync(indexPath, 'utf8'); | ||
| expect(html).toContain('persona-cell'); | ||
| const personaLinks = html.match(/persona-cell__link/g); | ||
| expect(personaLinks?.length).toBeGreaterThanOrEqual(4); |
There was a problem hiding this comment.
question (testing): Clarify whether exactly 4 personas are expected and assert accordingly
The test name implies exactly 4 persona links, but the assertion uses toBeGreaterThanOrEqual(4). Please either change the assertion to toBe(4) if exactly four are required, or update the test name to indicate an "at least 4" expectation.
Comment on lines
+66
to
+71
| it('has dual-view data attributes', () => { | ||
| const html = readFileSync(indexPath, 'utf8'); | ||
| expect(html).toContain('data-hero-view="engineering"'); | ||
| expect(html).toContain('data-hero-view="creative"'); | ||
| expect(html).toContain('data-stats-view="engineering"'); | ||
| expect(html).toContain('data-stats-view="creative"'); |
There was a problem hiding this comment.
issue (testing): Add a test to assert that bento cells do not use data-reveal, as mentioned in the PR description
The PR description specifies “No data-reveal on bento cells (above-fold, no scroll trigger needed)”, but there’s no test enforcing this. Please add a test that asserts data-reveal never appears on bento cell elements (or anywhere in the document, if that’s the requirement) to capture this behavior.
![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 adds a new visual test suite for the bento grid, updates quality and trust metrics, and introduces responsive horizontal scrolling for stat pills on mobile. The reviewer suggested improving test performance by reading the HTML file once at the suite level instead of repeatedly in every test block.
| }); | ||
|
|
||
| it('has bento-grid section', () => { | ||
| const html = readFileSync(indexPath, 'utf8'); |
There was a problem hiding this comment.
Reading the index.html file repeatedly within each it block can lead to unnecessary I/O operations and slow down test execution. It's more efficient to read the file once at the beginning of the describe block and reuse its content across all tests in that suite.
it('has bento-grid section', () => {
expect(html).toContain('class="bento-grid"');| describe('bento grid structure', () => { | ||
| const indexPath = resolve(DIST, 'index.html'); | ||
|
|
||
| it('dist/index.html exists', () => { |
There was a problem hiding this comment.
To improve test performance, the index.html file should be read only once for the entire test suite. Move the readFileSync call to the describe block scope so that html is available to all it blocks without redundant file reads.
const indexPath = resolve(DIST, 'index.html');
const html = readFileSync(indexPath, 'utf8');
it('dist/index.html exists', () => {
4444J99
force-pushed
the
feature/phase-ic-bento-polish
branch
from
e12c9e0 to
8deaf07
Compare
- Mobile: stat pills horizontal scroll (nowrap + overflow-x: auto) - Tablet row-2 and mobile span-2 resets already present (verified) - Featured card arrow position: relative already set (verified) - No data-reveal on bento components (verified clean) - 10 structural tests for bento grid HTML output - Verifies: hero, stats, featured cards, controls, personas, CTA, no old components Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4444J99
force-pushed
the
feature/phase-ic-bento-polish
branch
from
8deaf07 to
47ee989
Compare
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
data-revealon bento cells (above-fold, no scroll trigger needed)Part of
Alpha-to-Omega roadmap Phase I (Bento Grid Homepage). Spec:
docs/superpowers/specs/2026-03-26-alpha-omega-roadmap-design.mdTest plan
🤖 Generated with Claude Code
Summary by Sourcery
Polish the bento homepage’s responsive behavior and add structural regression tests for the generated bento grid HTML.
New Features:
Tests: