[codex] Improve site accessibility and mobile polish#15
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAccessibility, performance, and responsive layout improvements are applied to both the home page and AI app rescue page: a skip-to-main-content link is added with matching CSS; the mobile menu gains aria-expanded animations and dynamic label updates; proof images gain lazy loading and async decoding; a ChangesAccessibility, Performance, and Responsive Layout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a35234bcbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .package-head { | ||
| display: none; |
There was a problem hiding this comment.
On viewports up to 880px, this hides the only role="columnheader" row for the package table. The replacement labels are CSS-generated via content: attr(data-label), which is not a reliable programmatic label, so screen-reader users on mobile can encounter the package values without the Package/Range/Best for/Included context. Keep the headers in the accessibility tree, or add real hidden labels inside each cell.
Useful? React with 👍 / 👎.
![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 (2)
tests/html-metadata.test.js (2)
93-94: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid brittle exact-count assertions for additive optimizations.
loading="lazy"/hardened-link counts are currently exact. That makes tests fail when new compliant elements are added. Use lower-bound (>=) checks unless exact cardinality is a product invariant.Suggested test-hardening diff
- expect(html.match(/loading="lazy"/g)).toHaveLength(5); + expect(html.match(/loading="lazy"/g)?.length).toBeGreaterThanOrEqual(5); - expect(html.match(/loading="lazy"/g)).toHaveLength(2); - expect(html.match(/target="_blank" rel="noopener noreferrer"/g)).toHaveLength(5); + expect(html.match(/loading="lazy"/g)?.length).toBeGreaterThanOrEqual(2); + expect(html.match(/target="_blank"\s+rel="noopener noreferrer"/g)?.length).toBeGreaterThanOrEqual(5);Also applies to: 138-139
🤖 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/html-metadata.test.js` around lines 93 - 94, The test assertion for `loading="lazy"` attribute matches uses an exact count check with toHaveLength(5) which makes the test brittle and fails when new compliant elements are added. Change this exact count assertion to use toBeGreaterThanOrEqual with an appropriate lower-bound value instead, similar to how the decoding="async" check is already written. Apply the same fix pattern to the similar exact count assertions at lines 138-139 that are also mentioned in the comment.
91-92: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake structural HTML assertions order-insensitive.
These checks currently depend on exact attribute/class ordering (
toContain(...)andclass="section-container bordered-section"), which can fail on harmless markup reformatting. Prefer regex/token-based assertions that validate presence of required attributes/classes regardless of order.Suggested test-hardening diff
- expect(html).toContain('class="skip-link" href="`#main-content`"'); - expect(html).toContain('<main id="main-content" tabindex="-1">'); + expect(html).toMatch(/<a[^>]*class=["'][^"']*\bskip-link\b[^"']*["'][^>]*href=["']`#main-content`["'][^>]*>/i); + expect(html).toMatch(/<main[^>]*id=["']main-content["'][^>]*tabindex=["']-1["'][^>]*>/i); - expect(html).toContain('class="skip-link" href="`#main-content`"'); - expect(html).toContain('<main id="main-content" tabindex="-1">'); + expect(html).toMatch(/<a[^>]*class=["'][^"']*\bskip-link\b[^"']*["'][^>]*href=["']`#main-content`["'][^>]*>/i); + expect(html).toMatch(/<main[^>]*id=["']main-content["'][^>]*tabindex=["']-1["'][^>]*>/i); - expect(html.match(/class="section-container bordered-section/g)).toHaveLength(6); + expect(html.match(/class=["'][^"']*\bsection-container\b[^"']*\bbordered-section\b[^"']*["']/g)?.length).toBe(6);Also applies to: 134-137
🤖 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/html-metadata.test.js` around lines 91 - 92, The test assertions in the expect statements on lines 91-92 use toContain with exact string matching, making them fragile to harmless attribute reordering in the HTML output. Replace these exact string match assertions with regex-based or token-based assertions that validate the presence of required attributes and classes (such as class="skip-link", href="`#main-content`", id="main-content", and tabindex="-1") without depending on their specific order within the HTML elements. Apply the same fix to the similar assertions mentioned at lines 134-137 to ensure all structural HTML assertions are order-insensitive.
🤖 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 `@styles.css`:
- Around line 1362-1364: The .package-head class currently uses display: none
which removes the columnheader row from the accessibility tree, making it
inaccessible to assistive technologies. Replace display: none with a CSS
technique that visually hides the element while keeping it in the accessibility
tree, such as using position: absolute with off-screen positioning, clip-path:
inset(100%), or a standard screen-reader-only hiding pattern. This preserves the
structural header context for mobile table cells while maintaining visual
design.
---
Nitpick comments:
In `@tests/html-metadata.test.js`:
- Around line 93-94: The test assertion for `loading="lazy"` attribute matches
uses an exact count check with toHaveLength(5) which makes the test brittle and
fails when new compliant elements are added. Change this exact count assertion
to use toBeGreaterThanOrEqual with an appropriate lower-bound value instead,
similar to how the decoding="async" check is already written. Apply the same fix
pattern to the similar exact count assertions at lines 138-139 that are also
mentioned in the comment.
- Around line 91-92: The test assertions in the expect statements on lines 91-92
use toContain with exact string matching, making them fragile to harmless
attribute reordering in the HTML output. Replace these exact string match
assertions with regex-based or token-based assertions that validate the presence
of required attributes and classes (such as class="skip-link",
href="`#main-content`", id="main-content", and tabindex="-1") without depending on
their specific order within the HTML elements. Apply the same fix to the similar
assertions mentioned at lines 134-137 to ensure all structural HTML assertions
are order-insensitive.
🪄 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 Plus
Run ID: 410eff62-d4fb-4814-ad34-c39da41bccac
📒 Files selected for processing (5)
ai-app-rescue/index.htmlindex.htmlstyles.csstests/html-metadata.test.jstests/public-files.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/html-metadata.test.js (1)
167-170: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert media-block extraction explicitly before content checks.
mobileBlockcan beundefined; adding an explicit existence assertion makes failures clearer and avoids opaque matcher errors.Suggested diff
- const mobileBlock = css.match(/@media \(max-width: 880px\) \{[\s\S]*?`@media` \(max-width: 560px\)/)?.[0]; + const mobileBlockMatch = css.match(/@media \(max-width: 880px\) \{[\s\S]*?`@media` \(max-width: 560px\)/); + expect(mobileBlockMatch).not.toBeNull(); + const mobileBlock = mobileBlockMatch?.[0] ?? "";🤖 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/html-metadata.test.js` around lines 167 - 170, The variable mobileBlock can be undefined if the regex match in the `@media` query fails, which causes subsequent expect statements to fail with unclear error messages. Add an explicit assertion immediately after the mobileBlock assignment to verify that mobileBlock is defined before proceeding with the content checks (toContain, toMatch calls). This will provide a clearer failure message if the media block extraction fails, rather than letting the matcher errors mask the real problem.
🤖 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 `@tests/html-metadata.test.js`:
- Around line 155-159: The test assertions for bordered-section classes and
external-link attributes currently depend on specific attribute/class ordering.
Replace the regex pattern checking for both section-container and
bordered-section classes (which assumes section-container appears first) with
separate assertions that check for each class independently regardless of order.
Similarly, replace the regex pattern matching target="_blank" followed by
rel="noopener noreferrer" (which assumes target comes first) with a different
approach that verifies both attributes exist without depending on their relative
position, such as checking for each attribute separately or using a more
flexible pattern that allows any order.
---
Nitpick comments:
In `@tests/html-metadata.test.js`:
- Around line 167-170: The variable mobileBlock can be undefined if the regex
match in the `@media` query fails, which causes subsequent expect statements to
fail with unclear error messages. Add an explicit assertion immediately after
the mobileBlock assignment to verify that mobileBlock is defined before
proceeding with the content checks (toContain, toMatch calls). This will provide
a clearer failure message if the media block extraction fails, rather than
letting the matcher errors mask the real problem.
🪄 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 Plus
Run ID: 94478172-a70e-45dc-98a8-a7e28f4c48b4
📒 Files selected for processing (3)
index.htmlstyles.csstests/html-metadata.test.js
1 participant
Summary
Verification
Notes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
noopener noreferrer.Tests