Conversation
This change improves the user experience by adding ARIA labels and Tooltips to the icon-only buttons in the main Header. It also enables the global TooltipProvider in the root layout to support these components. - Added TooltipProvider to app/layout.tsx - Added Tooltips and ARIA labels to History, Calendar, and Usage buttons in components/header.tsx - Ensured 50-line limit for micro-UX improvement. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@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.
|
|
|
WalkthroughAdded documentation for accessible header actions. Introduced TooltipProvider wrapper in root layout to support tooltip functionality. Enhanced header action buttons with Tooltip components and ARIA labels for improved accessibility across navigation elements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 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 |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Accessibility improvements are solid, but there are a couple of structural risks: TooltipProvider is mounted relatively deep (could break tooltips in portals/modals) and the logo button’s accessible name may be duplicated due to the inner image alt. There’s also duplicated tooltip-button markup (Usage desktop/mobile) that is likely to drift over time.
Additional notes (2)
- Maintainability |
components/header.tsx:107-107
There is duplicated tooltip+button markup for the Usage action in both desktop and mobile sections. This duplication increases the chance of drift (e.g., label changes, side placement, handlers).
Given this PR’s goal (micro-UX + a11y), it’s worth factoring into a small helper component to keep behavior consistent across breakpoints.
- Compatibility |
components/header.tsx:90-103
You removed the nativetitle="Open Calendar"attribute when adding the tooltip. For mouse users this is fine, buttitlecan still provide a basic fallback if tooltips fail to mount (provider missing, JS disabled, etc.).
Given this is a core navigation action, consider keeping title as a low-cost fallback (it won’t hurt accessibility when aria-label is present).
Summary of changes
Summary of changes
✅ Global tooltip support
- Added a global
TooltipProviderinapp/layout.tsxto enable tooltips across the app.
✅ Header accessibility + guidance
- Wrapped icon-only header buttons with
Tooltip/TooltipTrigger/TooltipContentincomponents/header.tsx. - Added
aria-labelattributes to icon-only buttons (History/Logo, Calendar, Usage) to improve screen-reader support.
🧹 Repo hygiene
- Added a short guidance note in
.jules/palette.mddocumenting the “ARIA + Tooltip” convention. - Removed
server.logfrom the repo.
| <TooltipProvider> | ||
| <Header /> | ||
| <ConditionalLottie /> | ||
| {children} | ||
| <HistorySidebar /> | ||
| <Footer /> | ||
| <Toaster /> | ||
| </TooltipProvider> |
There was a problem hiding this comment.
TooltipProvider is mounted inside several other providers and fairly deep in the tree. That works for header tooltips, but it can cause unexpected behavior if other parts of the app render tooltips in portals that assume a top-level provider (e.g., modals/overlays mounted outside this subtree). Generally, TooltipProvider should be as close to the root of <body> as possible.
Suggestion
Consider moving <TooltipProvider> to wrap the entire app shell (ideally directly under <body>), so all routes/portals/modals share a single provider context.
Example:
<body>
<TooltipProvider>
{/* all existing providers/components */}
</TooltipProvider>
</body>This reduces the risk of tooltip context gaps when components render outside the current provider subtree. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={toggleHistory} | ||
| data-testid="logo-history-toggle" | ||
| aria-label="Toggle history" | ||
| > | ||
| <Image | ||
| src="/images/logo.svg" | ||
| alt="Logo" | ||
| width={20} | ||
| height={20} | ||
| className="h-5 w-auto" | ||
| /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom">Toggle history</TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
The logo button now has both an aria-label ("Toggle history") and an <Image alt="Logo" />. Depending on the underlying Button implementation and how accessible names are computed, the alt text can end up being part of the button’s accessible name and potentially conflict with/duplicate the explicit aria-label.
For purely decorative icons inside an already-labeled control, the icon/image should typically be hidden from assistive tech.
Suggestion
Make the logo image decorative within the button so the button’s accessible name is solely controlled by aria-label.
Options:
- If Next/Image supports it in your version, set an empty alt:
<Image src="/images/logo.svg" alt="" ... />- Or add
aria-hidden:
<Image ... alt="Logo" aria-hidden="true" />(Prefer alt="" for decorative images.) 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: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
server.logis excluded by!**/*.log
📒 Files selected for processing (3)
.jules/palette.mdapp/layout.tsxcomponents/header.tsx
🧰 Additional context used
🪛 markdownlint-cli2 (0.21.0)
.jules/palette.md
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 2-2: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (1)
app/layout.tsx (1)
113-120: LGTM —TooltipProvideris correctly scoped.Wrapping all layout content (Header through Toaster) inside
TooltipProviderensures the tooltip context is available across the entire application without needing per-component providers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.jules/palette.md:
- Around line 1-4: Change the initial second-level heading to a top-level
heading and add a blank line after it: replace the leading "## 2026-02-20 -
[Accessible Header Actions]" with "# 2026-02-20 - [Accessible Header Actions]"
and insert an empty line between that heading and the following content to
satisfy MD041 and MD022 (the heading symbol and the blank-line-after-heading are
the unique items to change).
In `@components/header.tsx`:
- Around line 128-140: The Tooltip wrapper around the mobile usage Button
(Tooltip, TooltipTrigger, TooltipContent) is a no-op on touch devices; remove
the Tooltip and TooltipTrigger and the TooltipContent in the mobile section
while keeping the Button, its props (variant="ghost" size="icon"
onClick={handleUsageToggle} aria-label="Open usage") and the TentTree icon so
the interaction and screen-reader text remain intact (update the JSX around
handleUsageToggle and TentTree accordingly).
- Around line 109-114: The desktop and mobile usage toggle buttons are missing a
test id — add data-testid="usage-toggle" to the Button that calls
handleUsageToggle (the one with aria-label="Open usage") in the header
component, and also add the same data-testid to the mobile usage toggle Button
variant so both toggles can be targeted in tests.
- Around line 62-68: The three toggle buttons are missing aria-expanded props;
update the component to destructure and use the open-state flags from the hooks
and pass them into each Button: add isHistoryOpen to the existing destructure
from useHistoryToggle() and set aria-expanded={isHistoryOpen} on the history
Button (onClick={toggleHistory}), add isCalendarOpen to the destructure from
useCalendarToggle() and set aria-expanded={isCalendarOpen} on the calendar
Button, and ensure both usage toggle Buttons use the already-destructured
isUsageOpen by adding aria-expanded={isUsageOpen}; reference the functions/hooks
toggleHistory, useHistoryToggle, toggleCalendar, useCalendarToggle and the state
names isHistoryOpen, isCalendarOpen, isUsageOpen to locate where to apply these
changes.
|
|
||
| ## 2026-02-20 - [Accessible Header Actions] | ||
| **Learning:** Icon-only buttons in the navigation header lack clear visual and screen-reader context. While ARIA labels solve accessibility, they don't provide visual guidance for sighted users on hover. | ||
| **Action:** Always pair ARIA labels with Tooltips for icon-only navigation elements. Ensure a global TooltipProvider is available in the root layout to support these components. |
There was a problem hiding this comment.
Two markdownlint warnings flagged by static analysis.
- MD022 — The
##heading on line 2 is not followed by a blank line; content starts immediately on line 3. - MD041 — The first heading in the file should be a top-level
#heading, not##.
📝 Proposed fix
-
-## 2026-02-20 - [Accessible Header Actions]
-**Learning:** Icon-only buttons in the navigation header lack clear visual and screen-reader context. While ARIA labels solve accessibility, they don't provide visual guidance for sighted users on hover.
-**Action:** Always pair ARIA labels with Tooltips for icon-only navigation elements. Ensure a global TooltipProvider is available in the root layout to support these components.
+# Palette Notes
+
+## 2026-02-20 - Accessible Header Actions
+
+**Learning:** Icon-only buttons in the navigation header lack clear visual and screen-reader context. While ARIA labels solve accessibility, they don't provide visual guidance for sighted users on hover.
+
+**Action:** Always pair ARIA labels with Tooltips for icon-only navigation elements. Ensure a global TooltipProvider is available in the root layout to support these components.📝 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.
| ## 2026-02-20 - [Accessible Header Actions] | |
| **Learning:** Icon-only buttons in the navigation header lack clear visual and screen-reader context. While ARIA labels solve accessibility, they don't provide visual guidance for sighted users on hover. | |
| **Action:** Always pair ARIA labels with Tooltips for icon-only navigation elements. Ensure a global TooltipProvider is available in the root layout to support these components. | |
| # Palette Notes | |
| ## 2026-02-20 - Accessible Header Actions | |
| **Learning:** Icon-only buttons in the navigation header lack clear visual and screen-reader context. While ARIA labels solve accessibility, they don't provide visual guidance for sighted users on hover. | |
| **Action:** Always pair ARIA labels with Tooltips for icon-only navigation elements. Ensure a global TooltipProvider is available in the root layout to support these components. |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 2-2: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/palette.md around lines 1 - 4, Change the initial second-level
heading to a top-level heading and add a blank line after it: replace the
leading "## 2026-02-20 - [Accessible Header Actions]" with "# 2026-02-20 -
[Accessible Header Actions]" and insert an empty line between that heading and
the following content to satisfy MD041 and MD022 (the heading symbol and the
blank-line-after-heading are the unique items to change).
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={toggleHistory} | ||
| data-testid="logo-history-toggle" | ||
| aria-label="Toggle history" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the header component and read it
find . -name "header.tsx" -o -name "header.ts" | head -5Repository: QueueLab/QCX
Length of output: 79
🏁 Script executed:
# If found, read the file with line numbers, focusing on the relevant sections
wc -l components/header.tsxRepository: QueueLab/QCX
Length of output: 81
🏁 Script executed:
# Check what the hooks (useHistoryToggle, useCalendarToggle, useUsageToggle) export
rg -n "export.*useHistoryToggle|export.*useCalendarToggle|export.*useUsageToggle" --type ts --type tsx -A 5Repository: QueueLab/QCX
Length of output: 83
🏁 Script executed:
# Also search for hook definitions and what they return
rg -n "function useHistoryToggle|function useCalendarToggle|function useUsageToggle|const useHistoryToggle|const useCalendarToggle|const useUsageToggle" --type ts --type tsx -A 10Repository: QueueLab/QCX
Length of output: 83
🏁 Script executed:
cat -n components/header.tsxRepository: QueueLab/QCX
Length of output: 5561
🏁 Script executed:
# Search for hook definitions without type filtering
rg -n "useHistoryToggle|useCalendarToggle|useUsageToggle" -A 8Repository: QueueLab/QCX
Length of output: 13070
🏁 Script executed:
# Search for where these hooks are imported and used in header.tsx
rg -n "const.*useHistory|const.*useCalendar|const.*useUsage" -A 3Repository: QueueLab/QCX
Length of output: 3986
🏁 Script executed:
# Read the usage-toggle-context to see its interface
cat -n components/usage-toggle-context.tsxRepository: QueueLab/QCX
Length of output: 1156
🏁 Script executed:
# Read the history-toggle-context to see its interface
cat -n components/history-toggle-context.tsxRepository: QueueLab/QCX
Length of output: 1231
🏁 Script executed:
# Read the calendar-toggle-context to see its interface
cat -n components/calendar-toggle-context.tsxRepository: QueueLab/QCX
Length of output: 1288
Toggle buttons are missing aria-expanded — WCAG 4.1.2 gap.
Buttons that open/close panels must communicate their current state to assistive technologies. Without aria-expanded, screen reader users cannot determine whether the history/usage/calendar panel is open or closed after activation.
All three toggle state values are available from their respective hooks and should be destructured and applied:
- Usage buttons (lines 109–114, 130–135):
isUsageOpenis already destructured at line 32. Addaria-expanded={isUsageOpen}to both usage toggle buttons. - History button (line 62–68): Add
isHistoryOpento the destructuring at line 34 (const { toggleHistory, isHistoryOpen } = useHistoryToggle()), then addaria-expanded={isHistoryOpen}. - Calendar button (line 92–98): Add
isCalendarOpento the destructuring at line 30 (const { toggleCalendar, isCalendarOpen } = useCalendarToggle()), then addaria-expanded={isCalendarOpen}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/header.tsx` around lines 62 - 68, The three toggle buttons are
missing aria-expanded props; update the component to destructure and use the
open-state flags from the hooks and pass them into each Button: add
isHistoryOpen to the existing destructure from useHistoryToggle() and set
aria-expanded={isHistoryOpen} on the history Button (onClick={toggleHistory}),
add isCalendarOpen to the destructure from useCalendarToggle() and set
aria-expanded={isCalendarOpen} on the calendar Button, and ensure both usage
toggle Buttons use the already-destructured isUsageOpen by adding
aria-expanded={isUsageOpen}; reference the functions/hooks toggleHistory,
useHistoryToggle, toggleCalendar, useCalendarToggle and the state names
isHistoryOpen, isCalendarOpen, isUsageOpen to locate where to apply these
changes.
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={handleUsageToggle} | ||
| aria-label="Open usage" | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing data-testid on usage toggle buttons.
The logo-history button (data-testid="logo-history-toggle") and calendar button (data-testid="calendar-toggle") have test IDs, but neither the desktop nor mobile usage toggle has one, making it harder to target these buttons in automated tests.
🔧 Proposed fix
- onClick={handleUsageToggle}
- aria-label="Open usage"
+ data-testid="usage-toggle"
+ onClick={handleUsageToggle}
+ aria-label="Open usage"Apply the same data-testid="usage-toggle" to both desktop (line 109) and mobile (line 130) usage buttons.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/header.tsx` around lines 109 - 114, The desktop and mobile usage
toggle buttons are missing a test id — add data-testid="usage-toggle" to the
Button that calls handleUsageToggle (the one with aria-label="Open usage") in
the header component, and also add the same data-testid to the mobile usage
toggle Button variant so both toggles can be targeted in tests.
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={handleUsageToggle} | ||
| aria-label="Open usage" | ||
| > | ||
| <TentTree className="h-[1.2rem] w-[1.2rem]" /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom">Open usage</TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
Tooltip on mobile (md:hidden) is a no-op for touch users.
Radix UI's Tooltip fires on pointerenter / focus events; touch devices don't generate hover events, so the tooltip on the mobile usage button will never appear. The aria-label already covers screen-reader accessibility on mobile.
Consider whether this <Tooltip> wrapper adds value in the mobile section, or if it can be removed to keep the mobile markup leaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/header.tsx` around lines 128 - 140, The Tooltip wrapper around the
mobile usage Button (Tooltip, TooltipTrigger, TooltipContent) is a no-op on
touch devices; remove the Tooltip and TooltipTrigger and the TooltipContent in
the mobile section while keeping the Button, its props (variant="ghost"
size="icon" onClick={handleUsageToggle} aria-label="Open usage") and the
TentTree icon so the interaction and screen-reader text remain intact (update
the JSX around handleUsageToggle and TentTree accordingly).
2 participants
Improved the accessibility and user guidance of the navigation header by implementing Tooltips and ARIA labels for icon-only buttons. The changes are focused on the Header component and include enabling the global TooltipProvider. All changes are verified via linting and type-checking.
PR created automatically by Jules for task 10953669570692803336 started by @ngoiyaeric
Summary by CodeRabbit
New Features
Accessibility