🎨 Palette: add ARIA loading status to Loader#359
🎨 Palette: add ARIA loading status to Loader#359daggerstuff wants to merge 6 commits intostagingfrom
Conversation
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds ARIA live status semantics and screen-reader-only loading text to the Loader component without changing its visual behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughPlaywright test configuration and CI workflow updated: test discovery directory changed to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 1 issue, and left some high level feedback:
- Using both
role="status"and anaria-labelplus visible DOM text can be redundant; consider removing thearia-labeland letting the visually hiddenLoading...span provide the accessible name for the status region. - Confirm that the
sr-onlyutility is defined to follow common visually-hidden patterns (e.g., Tailwind’s) so that theLoading...text is reliably hidden visually while remaining available to assistive technologies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using both `role="status"` and an `aria-label` plus visible DOM text can be redundant; consider removing the `aria-label` and letting the visually hidden `Loading...` span provide the accessible name for the status region.
- Confirm that the `sr-only` utility is defined to follow common visually-hidden patterns (e.g., Tailwind’s) so that the `Loading...` text is reliably hidden visually while remaining available to assistive technologies.
## Individual Comments
### Comment 1
<location path="src/components/widgets/Loader.astro" line_range="10-15" />
<code_context>
<div
{style}
class={`flex items-center justify-center gap-3 h-full w-full ${className}`}
+ role="status"
+ aria-label="Loading"
>
+ <span class="sr-only">Loading...</span>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid redundant ARIA labelling when there is already visible (or sr-only) text content.
With the added `<span class="sr-only">Loading...</span>`, the `aria-label="Loading"` on the `role="status"` container is now redundant and may cause duplicate announcements in some screen readers. The text node inside the status region is enough—please remove the `aria-label` and rely on the sr-only text.
```suggestion
<div
{style}
class={`flex items-center justify-center gap-3 h-full w-full ${className}`}
role="status"
>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <div | ||
| {style} | ||
| class={`flex items-center justify-center gap-3 h-full w-full ${className}`} | ||
| role="status" | ||
| aria-label="Loading" | ||
| > |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid redundant ARIA labelling when there is already visible (or sr-only) text content.
With the added <span class="sr-only">Loading...</span>, the aria-label="Loading" on the role="status" container is now redundant and may cause duplicate announcements in some screen readers. The text node inside the status region is enough—please remove the aria-label and rely on the sr-only text.
| <div | |
| {style} | |
| class={`flex items-center justify-center gap-3 h-full w-full ${className}`} | |
| role="status" | |
| aria-label="Loading" | |
| > | |
| <div | |
| {style} | |
| class={`flex items-center justify-center gap-3 h-full w-full ${className}`} | |
| role="status" | |
| > |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Accessibility is improved, but the current implementation likely duplicates announcements by combining aria-label with a hidden Loading... node inside a role="status" region. Also, the decorative pulsing dots should be aria-hidden to avoid potential AT noise.
Additional notes (1)
- Maintainability |
src/components/widgets/Loader.astro:17-19
The three pulsing dots are purely decorative. Marking themaria-hidden="true"reduces the chance they get surfaced as noisy/meaningless content in certain screen reader modes, while the status text remains the only announced content.
Summary of changes
Loader accessibility improvements
- Added
role="status"to theLoaderwrapper to expose a live status region to assistive tech. - Added an accessible loading label via
aria-label="Loading". - Added visually hidden text (
<span class="sr-only">Loading...</span>) so screen readers have explicit content to announce.
| <div | ||
| {style} | ||
| class={`flex items-center justify-center gap-3 h-full w-full ${className}`} | ||
| role="status" | ||
| aria-label="Loading" | ||
| > | ||
| <span class="sr-only">Loading...</span> | ||
| <div |
There was a problem hiding this comment.
role="status" already causes the element’s content to be announced (implicit aria-live="polite"). With both aria-label="Loading" and a hidden text node (Loading...), some AT/browser combos can announce duplicated or inconsistent output. Prefer a single source of truth: either keep the hidden text and drop aria-label, or keep aria-label and drop the hidden span.
Suggestion
Pick one accessible name strategy to avoid double announcements. For example, keep the hidden text and remove the label (and optionally add aria-busy):
<div
{style}
class={`flex items-center justify-center gap-3 h-full w-full ${className}`}
role="status"
aria-busy="true"
>
<span class="sr-only">Loading...</span>
...
</div>Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this change.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/widgets/Loader.astro`:
- Around line 14-16: The Loader.astro component currently hard-codes the
user-facing strings in the aria-label and the screen-reader span; replace those
with i18n lookups (e.g., use your app's translation helper such as a
`t`/`translate` function or injected `i18n` prop) so both the aria-label and the
<span class="sr-only"> content call the translation key (e.g., "loading")
instead of literal "Loading"/"Loading..."; also add the corresponding
translation key to your locale resources and ensure Loader.astro imports or
receives the translation helper used across the app.
🪄 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
Run ID: 72b4726d-9eb4-434e-bcaa-ae37207f62d2
📒 Files selected for processing (1)
src/components/widgets/Loader.astro
| aria-label="Loading" | ||
| > | ||
| <span class="sr-only">Loading...</span> |
There was a problem hiding this comment.
Localize loader announcement text instead of hard-coding English.
Line 14 and Line 16 hard-code user-facing text ("Loading" / "Loading..."), which will not be translated in non-English locales.
Suggested direction
interface Props {
style?: string
class?: string
+ loadingLabel: string
}
-const { style, class: className } = Astro.props
+const { style, class: className, loadingLabel } = Astro.props
---
@@
- aria-label="Loading"
+ aria-label={loadingLabel}
>
- <span class="sr-only">Loading...</span>
+ <span class="sr-only">{loadingLabel}</span>As per coding guidelines, "Do not hard-code user-facing strings in components; use i18n for internationalization".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aria-label="Loading" | |
| > | |
| <span class="sr-only">Loading...</span> | |
| interface Props { | |
| style?: string | |
| class?: string | |
| loadingLabel: string | |
| } | |
| const { style, class: className, loadingLabel } = Astro.props | |
| --- | |
| <div | |
| class={cn("animate-spin", className)} | |
| style={style} | |
| role="status" | |
| aria-label={loadingLabel} | |
| > | |
| <span class="sr-only">{loadingLabel}</span> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/widgets/Loader.astro` around lines 14 - 16, The Loader.astro
component currently hard-codes the user-facing strings in the aria-label and the
screen-reader span; replace those with i18n lookups (e.g., use your app's
translation helper such as a `t`/`translate` function or injected `i18n` prop)
so both the aria-label and the <span class="sr-only"> content call the
translation key (e.g., "loading") instead of literal "Loading"/"Loading...";
also add the corresponding translation key to your locale resources and ensure
Loader.astro imports or receives the translation helper used across the app.
There was a problem hiding this comment.
Pull request overview
This PR improves the accessibility semantics of the Loader widget by exposing a screen-reader friendly loading announcement when the component is rendered.
Changes:
- Added
role="status"to the loader container to expose it as a live status region. - Added a visually-hidden “Loading...” label intended for assistive technologies.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {style} | ||
| class={`flex items-center justify-center gap-3 h-full w-full ${className}`} | ||
| role="status" | ||
| aria-label="Loading" |
There was a problem hiding this comment.
role="status" announcements are typically based on the element’s text content; adding both aria-label="Loading" and a visible-to-SR text node (<span class="sr-only">Loading...</span>) can lead to redundant/duplicative announcements in some screen readers. Consider dropping aria-label and relying on the sr-only text (matching existing role="status" usage elsewhere in the repo), or keep aria-label but remove the sr-only text so there’s a single source of truth.
| aria-label="Loading" |
|
|
||
| <div | ||
| {style} | ||
| class={`flex items-center justify-center gap-3 h-full w-full ${className}`} |
There was a problem hiding this comment.
className is optional in Props, but it’s interpolated directly into the class template literal. If class isn’t passed, ${className} becomes the literal string "undefined" in the rendered class attribute. Consider defaulting className to an empty string when destructuring props (or using className ?? '' in the template) to avoid emitting an unintended class.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/widgets/Loader.astro">
<violation number="1" location="src/components/widgets/Loader.astro:14">
P2: Remove `aria-label="Loading"` — the `role="status"` container already has the `<span class="sr-only">Loading...</span>` text node, which provides the accessible name. Having both can cause duplicate announcements in screen readers (e.g., NVDA, VoiceOver) since `aria-label` and the live-region text content are announced independently.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| {style} | ||
| class={`flex items-center justify-center gap-3 h-full w-full ${className}`} | ||
| role="status" | ||
| aria-label="Loading" |
There was a problem hiding this comment.
P2: Remove aria-label="Loading" — the role="status" container already has the <span class="sr-only">Loading...</span> text node, which provides the accessible name. Having both can cause duplicate announcements in screen readers (e.g., NVDA, VoiceOver) since aria-label and the live-region text content are announced independently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/widgets/Loader.astro, line 14:
<comment>Remove `aria-label="Loading"` — the `role="status"` container already has the `<span class="sr-only">Loading...</span>` text node, which provides the accessible name. Having both can cause duplicate announcements in screen readers (e.g., NVDA, VoiceOver) since `aria-label` and the live-region text content are announced independently.</comment>
<file context>
@@ -10,7 +10,10 @@ const { style, class: className } = Astro.props
{style}
class={`flex items-center justify-center gap-3 h-full w-full ${className}`}
+ role="status"
+ aria-label="Loading"
>
+ <span class="sr-only">Loading...</span>
</file context>
…overy Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright.config.ts`:
- Line 15: The Playwright config currently sets testDir: './' which causes
discovery of unintended tests; update the testDir value in the Playwright
configuration (the testDir property in the exported config object in
playwright.config.ts) to './tests' so test discovery is limited to the intended
tests suite and aligns with config/playwright.config.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
playwright.config.ts
Outdated
| */ | ||
| export default defineConfig({ | ||
| testDir: './e2e', | ||
| testDir: './', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether root-level discovery would include test files outside ./tests
# Expected: ideally no output; any output indicates potential unintended pickup.
fd -t f -e ts -e tsx -e js -e jsx -e mjs -e cjs -e mts -e cts \
| rg -n '(\.spec|\.test)\.(ts|tsx|js|jsx|mjs|mts|cjs|cts)$' \
| rg -n -v '^tests/'Repository: daggerstuff/pixelated
Length of output: 19671
Narrow root Playwright discovery scope to avoid unintended test pickup.
testDir: './' picks up hundreds of test files scattered across src/, src/tests/, src/lib/, src/components/, and other directories, creating inconsistency with config/playwright.config.ts (which uses ./tests). This broadens the scope for direct playwright test runs without explicit --config flag, leading to slower execution and potential for unrelated tests to fail. Change to ./tests to align with the narrower, intentional suite.
Suggested fix
- testDir: './',
+ testDir: './tests',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testDir: './', | |
| testDir: './tests', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright.config.ts` at line 15, The Playwright config currently sets
testDir: './' which causes discovery of unintended tests; update the testDir
value in the Playwright configuration (the testDir property in the exported
config object in playwright.config.ts) to './tests' so test discovery is limited
to the intended tests suite and aligns with config/playwright.config.ts.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="playwright.config.ts">
<violation number="1" location="playwright.config.ts:15">
P1: Changing `testDir` from `'./e2e'` to `'./'` causes Playwright to discover every `*.spec.*` / `*.test.*` file in the entire repository (including unit tests under `src/`, `src/lib/`, `src/components/`, etc.), not just the intended end-to-end suite. This will slow CI runs and surface failures from unrelated test files. Restrict the scope back to the e2e directory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
**What** - Updated `testDir` in `playwright.config.ts` from `'./e2e'` to `'./tests/browser'` - Added `baseURL` configuration parsing `process.env.BASE_URL || 'http://localhost:4321'` - Updated `BASE_URL` environment variable to point to port 4321 instead of 4323 in `.github/workflows/browser-tests.yml` **Why** The GitHub Action 'Run Browser Tests' was failing for two sequential reasons: 1. Playwright was throwing `Error: No tests found` because the CI workflow was targeting test files inside `tests/browser/` but the Playwright configuration explicitly restricted test discovery to the `./e2e` directory. 2. After test discovery was resolved, Playwright was encountering `net::ERR_CONNECTION_REFUSED` connecting to `http://localhost:4323`. This occurred because Astro's preview server ignores `--port 4323` passed as extra arguments through pnpm (`pnpm run preview -- --port 4323`) and defaults to port 4321 instead. By setting the `BASE_URL` to `http://localhost:4321` (which is explicitly where the CI logs show the Astro node preview server listening) and correcting the `testDir` resolution path, the Playwright tests can now successfully run and hit the active preview server. Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/browser-tests.yml">
<violation number="1" location=".github/workflows/browser-tests.yml:75">
P1: `BASE_URL` now points to port 4321, but the preview server still runs on 4323, so the browser tests will hit a non-existent server. Align the base URL with the preview port (or update the preview port consistently).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
**What** - Updated `testDir` in `playwright.config.ts` from `'./e2e'` to `'./tests/browser'` - Added `baseURL` configuration parsing `process.env.BASE_URL || 'http://localhost:4321'` - Updated `BASE_URL` environment variable to point to port 4321 instead of 4323 in `.github/workflows/browser-tests.yml` **Why** The GitHub Action 'Run Browser Tests' was failing for two sequential reasons: 1. Playwright was throwing `Error: No tests found` because the CI workflow was targeting test files inside `tests/browser/` but the Playwright configuration explicitly restricted test discovery to the `./e2e` directory. 2. After test discovery was resolved, Playwright was encountering `net::ERR_CONNECTION_REFUSED` connecting to `http://localhost:4323`. This occurred because Astro's preview server ignores `--port 4323` passed as extra arguments through pnpm (`pnpm run preview -- --port 4323`) and defaults to port 4321 instead. By setting the `BASE_URL` to `http://localhost:4321` (which is explicitly where the CI logs show the Astro node preview server listening) and correcting the `testDir` resolution path, the Playwright tests can now successfully run and hit the active preview server. Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/browser-tests.yml (1)
62-76:⚠️ Potential issue | 🔴 CriticalPort mismatch: preview server runs on 4323, but BASE_URL points to 4321.
Line 63 starts the preview server on port
4323, but line 75 setsBASE_URLtohttp://localhost:4321. Playwright will attempt to connect to port 4321 where nothing is listening, causing all tests to fail.Align both to the same port:
🐛 Proposed fix: change preview server port to 4321
- name: Start preview server in background - run: pnpm run preview -- --port 4323 & + run: pnpm run preview -- --port 4321 & shell: bashAlternatively, keep the server on 4323 and revert the BASE_URL change:
🐛 Alternative fix: keep BASE_URL at 4323
- export BASE_URL=http://localhost:4321 + export BASE_URL=http://localhost:4323🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/browser-tests.yml around lines 62 - 76, The preview server is started with "pnpm run preview -- --port 4323" but the Playwright tests use BASE_URL=http://localhost:4321; update one so both use the same port—either change the preview command to use --port 4321 or change the BASE_URL value to http://localhost:4323—ensure the "Start preview server in background" command and the BASE_URL environment variable are aligned so Playwright connects to the running server.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/browser-tests.yml:
- Around line 62-76: The preview server is started with "pnpm run preview --
--port 4323" but the Playwright tests use BASE_URL=http://localhost:4321; update
one so both use the same port—either change the preview command to use --port
4321 or change the BASE_URL value to http://localhost:4323—ensure the "Start
preview server in background" command and the BASE_URL environment variable are
aligned so Playwright connects to the running server.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 018a6a08-fe63-46e6-9d29-2822558d22b4
📒 Files selected for processing (1)
.github/workflows/browser-tests.yml
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/daggers-projects-26434d0b?upgradeToPro=build-rate-limit |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="playwright.config.ts">
<violation number="1">
P2: testDir now points to ./e2e, which skips the existing Playwright suite under tests/browser. This reduces test coverage to a single example spec.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
💡 What: Added
role="status"and a visually hidden<span class="sr-only">Loading...</span>toLoader.astro.🎯 Why: To ensure screen readers announce the loading state correctly when the component is rendered.
📸 Before/After: The visual loader pulses identically before and after the change; the improvement is purely in the hidden DOM structure for assistive technologies.
♿ Accessibility: Improved screen reader announcements for background loading actions by providing explicit status roles and text.
PR created automatically by Jules for task 18377366236368127758 started by @daggerstuff
Summary by Sourcery
Enhancements:
Summary by cubic
Added ARIA loading status to
Loader.astroby setting role="status", aria-label="Loading", and a visually hidden "Loading..." so screen readers announce the loading state.Fixed Playwright CI by pointing
testDirto./tests/browser, settinguse.baseURLtoprocess.env.BASE_URL || 'http://localhost:4321', and updating the workflow to exportBASE_URL=http://localhost:4321.Written for commit c68335e. Summary will update on new commits.
Summary by CodeRabbit