fix: migrate live region to useAnnouncer #2268
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe pull request introduces accessibility announcements using the Nuxt announcer system. The root component now includes a Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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)
app/pages/search.vue (1)
654-661: Consider using snapshot values consistently.Lines 654 and 658 access live refs (
isRelevanceSort.value,visibleResults.value?.total) inside the watcher callback, while other values come from the snapshot. SinceeffectiveTotalalready equalsvisibleResults.value?.totalwhen relevance-sorted, you could derive the sort mode from the snapshot or addisRelevanceSortto the snapshot for consistency.This isn't a bug since the watcher runs synchronously, but it makes the data flow clearer.
♻️ Optional: Add isRelevanceSort to snapshot
watch( () => ({ rateLimited: isRateLimited.value, searchStatus: status.value, count: displayResults.value.length, searchQuery: query.value, mode: viewMode.value, pagMode: paginationMode.value, total: effectiveTotal.value, + relevanceSort: isRelevanceSort.value, }), - ({ rateLimited, searchStatus, count, searchQuery, mode, pagMode, total }) => { + ({ rateLimited, searchStatus, count, searchQuery, mode, pagMode, total, relevanceSort }) => { // ... use relevanceSort instead of isRelevanceSort.value
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9042886f-160d-4214-a7db-2daa3c7e39cb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
app/app.vueapp/pages/search.vuepackage.json
| pagMode: paginationMode.value, | ||
| total: effectiveTotal.value, | ||
| }), | ||
| ({ rateLimited, searchStatus, count, searchQuery, mode, pagMode, total }) => { |
There was a problem hiding this comment.
Rename searchQuery to avoid shadowing the outer scope variable.
The static analysis tool correctly flags that searchQuery in the watcher's destructuring shadows the outer searchQuery from useGlobalSearch() on line 49. Rename this to something like currentQuery or snapshotQuery for clarity.
🛠️ Proposed fix
- ({ rateLimited, searchStatus, count, searchQuery, mode, pagMode, total }) => {
+ ({ rateLimited, searchStatus, count, searchQuery: currentQuery, mode, pagMode, total }) => {
if (rateLimited) {
announcePolite($t('search.rate_limited'))
return
}
// ... rest of callback
} else if (searchStatus === 'success' || searchStatus === 'error') {
- if (searchQuery) {
- announcePolite($t('search.no_results', { query: searchQuery }))
+ if (currentQuery) {
+ announcePolite($t('search.no_results', { query: currentQuery }))
} else {
cancelPendingAnnouncements()
}
}🧰 Tools
🪛 GitHub Check: 🤖 Autofix code
[warning] 628-628: eslint(no-shadow)
'searchQuery' is already declared in the upper scope.
RYGRIT
force-pushed
the
fix/follow-up-1812
branch
from
4d04b34 to
a581219
Compare
|
I'll convert this to a draft PR for now, and reopen it once #2047 has been merged. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8661a4c3-7a93-40f8-a5fc-59689ec6c8bd
📒 Files selected for processing (2)
app/app.vueapp/pages/search.vue
✅ Files skipped from review due to trivial changes (1)
- app/app.vue
| function cancelPendingAnnouncements() { | ||
| announcePoliteDesktop.cancel() | ||
| announcePoliteMobile.cancel() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me locate and examine the search.vue file
fd -t f "search.vue" --exec wc -l {} \;Repository: npmx-dev/npmx.dev
Length of output: 125
🏁 Script executed:
# Read the specific lines mentioned in the review
cat -n app/pages/search.vue | sed -n '600,630p'Repository: npmx-dev/npmx.dev
Length of output: 1069
🏁 Script executed:
# Search for where announcePoliteDesktop and announcePoliteMobile are defined/used
rg "announcePoliteDesktop|announcePoliteMobile" app/pages/search.vue -B 5 -A 5Repository: npmx-dev/npmx.dev
Length of output: 770
🏁 Script executed:
# Check the entire file structure to understand the component
head -100 app/pages/search.vueRepository: npmx-dev/npmx.dev
Length of output: 3001
🏁 Script executed:
# Find where cancelPendingAnnouncements is called
rg "cancelPendingAnnouncements" app/pages/search.vue -B 3 -A 3Repository: npmx-dev/npmx.dev
Length of output: 645
🏁 Script executed:
# Look at the entire watch block that announces results
cat -n app/pages/search.vue | sed -n '618,750p'Repository: npmx-dev/npmx.dev
Length of output: 5801
Ensure screen reader announcements are triggered for repeated identical messages.
When cancelPendingAnnouncements() cancels the debounces, subsequent calls to announcePolite() with identical message text will not produce a fresh announcer update. Since multiple searches can generate the same announcement text (e.g., two searches each finding 100 results), the announcer state needs to be reset to trigger the screen reader update. Either reset the announcer or vary the message before reusing it.
2 participants
🔗 Linked issue
follow up #1812
🧭 Context
nuxt/nuxt#34318
📚 Description
<NuxtAnnouncer />global componentKnown issue: typed route type-checking after Nuxt 4.4 upgrade
After upgrading to Nuxt 4.4.2,
vue-tscreports many route param type errors (string | string[] | undefined) and typed-router related issues.@storybook-vue/nuxt@9.0.1currently declares:nuxt: ^3.13.0vue-router: ^4.3.0while this repo is on:
nuxt: 4.4.2vue-router: 5.x(typed pages enabled)So the workspace ends up with mixed router majors (v4 + v5), which likely affects typed-router / volar behavior and amplifies route param typing issues.