🎨 Palette: Improve accessibility and loading feedback#525
🎨 Palette: Improve accessibility and loading feedback#525ngoiyaeric wants to merge 1 commit intomainfrom
Conversation
This change implements several micro-UX improvements focused on accessibility and user feedback: 1. Added \`aria-label\` and \`title\` to multiple icon-only buttons across the application (Header, ChatPanel, MobileIconsBar, History) to improve accessibility and provide tooltips. 2. Implemented a loading state (spinner) for the chat submission action on both desktop and mobile, providing immediate visual feedback during async operations. 3. Ensured branding elements like the logo have descriptive labels when functioning as interactive controls. 4. Added a Palette journal for tracking critical UX learnings. 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.
|
|
|
WalkthroughThis PR enhances accessibility across multiple UI components by adding ARIA labels and titles to icon-only buttons and interactive elements. It introduces submitting state management in ChatPanel, propagates it through Chat to MobileIconsBar, adds visual feedback via a Spinner during form submission, and documents accessibility findings in the project palette. Changes
Sequence DiagramsequenceDiagram
actor User
participant ChatPanel
participant Chat
participant MobileIconsBar
participant UI
User->>ChatPanel: Click submit button
ChatPanel->>ChatPanel: Set isSubmitting = true
ChatPanel->>Chat: Call onSubmitting(true)
Chat->>Chat: Set isSubmittingAction = true
Chat->>MobileIconsBar: Pass isSubmitting = true
MobileIconsBar->>UI: Render Spinner (disable submit)
ChatPanel->>ChatPanel: Process form submission
ChatPanel->>ChatPanel: Set isSubmitting = false
ChatPanel->>Chat: Call onSubmitting(false)
Chat->>Chat: Set isSubmittingAction = false
Chat->>MobileIconsBar: Pass isSubmitting = false
MobileIconsBar->>UI: Render ArrowRight (enable submit)
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
🤖 Fix all issues with AI agents
In `@components/chat-panel.tsx`:
- Around line 127-135: The submit call currently lacks a catch, so if
submit(formData) rejects the error bubbles as an unhandled rejection; wrap the
await submit(...) in a try/catch (keeping the existing finally) and in the catch
block handle the failure: log the error, call any error callback if available
(e.g., onSubmitting/onError), and update the messages state via
setMessages(currentMessages => [...currentMessages, /* an error/result marker
message to surface the failure */]) or set an error state so the UI shows
feedback. Ensure setIsSubmitting and onSubmitting toggles remain in the finally
block and reference the existing submit, setMessages and onSubmitting
identifiers when implementing.
In `@components/chat.tsx`:
- Around line 40-41: Rename the two confusing boolean states to make their roles
explicit: change const [isSubmitting, setIsSubmitting] to const
[shouldTriggerSubmit, setShouldTriggerSubmit] (this state is used by the
suggestion onSelect and the useEffect that triggers form submission), and change
const [isSubmittingAction, setIsSubmittingAction] to const [isSubmitting,
setIsSubmitting] (this state reflects the real async submit status from
ChatPanel/MobileIconsBar). Update all references accordingly: replace
setIsSubmitting(true) calls in suggestion onSelect with
setShouldTriggerSubmit(true), update the useEffect that watches submission
triggers to use shouldTriggerSubmit/setShouldTriggerSubmit, and wire the
ChatPanel and MobileIconsBar props/state to the new isSubmitting/setIsSubmitting
identifiers.
In `@components/mobile-icons-bar.tsx`:
- Around line 64-72: The Search button (Button with data-testid
"mobile-search-button" and the Search icon) has no onClick and is
non-functional; add an onClick handler that triggers the component's search
action—either call an existing prop like onSearchClick/onOpenSearch if
MobileIconsBar accepts one, or wire it to the local search-open function/state
(e.g., call openSearch()/setSearchOpen(true) or the hook that opens the search
modal). Ensure you update MobileIconsBar's props type to include the handler
name you choose (onSearchClick/onOpenSearch) if needed and pass that handler
into the Button as onClick.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.Jules/palette.mdcomponents/chat-panel.tsxcomponents/chat.tsxcomponents/header-search-button.tsxcomponents/header.tsxcomponents/history.tsxcomponents/mobile-icons-bar.tsx
🔇 Additional comments (8)
components/header-search-button.tsx (1)
157-157: LGTM! Addingaria-labelto complement the existingtitleon this icon-only button is consistent with the PR's accessibility goals.components/history.tsx (1)
22-23: LGTM! Consistentaria-labelandtitlepairing on the icon-only history button..Jules/palette.md (1)
1-3: LGTM! Good practice to document accessibility learnings for the team.components/header.tsx (2)
55-70: Good catch on the logo-as-control pattern. Adding descriptivearia-labelandtitleto the logo button that doubles as a history toggle addresses a real discoverability gap.
81-102: LGTM! Consistent aria-label/title additions for calendar and usage toggle buttons.components/mobile-icons-bar.tsx (1)
93-107: LGTM! The submit button correctly shows aSpinnerduring submission and is disabled viaisSubmitting, providing clear visual feedback.components/chat-panel.tsx (1)
239-240: LGTM! Accessibility attributes on the attachment, submit, and remove-attachment buttons are well-applied. The submit button correctly shows aSpinnerand is disabled during submission.Also applies to: 297-302, 312-319
components/chat.tsx (1)
136-149: LGTM! The submitting state is correctly wired fromChatPanelthrough toMobileIconsBar, providing consistent loading feedback across the mobile layout.
| setIsSubmitting(true) | ||
| onSubmitting?.(true) | ||
| try { | ||
| const responseMessage = await submit(formData) | ||
| setMessages(currentMessages => [...currentMessages, responseMessage as any]) | ||
| } finally { | ||
| setIsSubmitting(false) | ||
| onSubmitting?.(false) | ||
| } |
There was a problem hiding this comment.
Missing error handling in submit flow — unhandled rejection if submit() throws.
The try/finally resets the submitting state correctly, but there's no catch block. If submit(formData) rejects, the error propagates as an unhandled promise rejection. The user has already seen their message added optimistically (lines 108–114) and the input cleared (line 124), but will receive no error feedback.
🐛 Proposed fix: add error handling
setIsSubmitting(true)
onSubmitting?.(true)
try {
const responseMessage = await submit(formData)
setMessages(currentMessages => [...currentMessages, responseMessage as any])
+ } catch (error) {
+ console.error('Failed to submit message:', error)
+ // Consider showing a toast or inline error to the user
} finally {
setIsSubmitting(false)
onSubmitting?.(false)
}📝 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.
| setIsSubmitting(true) | |
| onSubmitting?.(true) | |
| try { | |
| const responseMessage = await submit(formData) | |
| setMessages(currentMessages => [...currentMessages, responseMessage as any]) | |
| } finally { | |
| setIsSubmitting(false) | |
| onSubmitting?.(false) | |
| } | |
| setIsSubmitting(true) | |
| onSubmitting?.(true) | |
| try { | |
| const responseMessage = await submit(formData) | |
| setMessages(currentMessages => [...currentMessages, responseMessage as any]) | |
| } catch (error) { | |
| console.error('Failed to submit message:', error) | |
| // Consider showing a toast or inline error to the user | |
| } finally { | |
| setIsSubmitting(false) | |
| onSubmitting?.(false) | |
| } |
🤖 Prompt for AI Agents
In `@components/chat-panel.tsx` around lines 127 - 135, The submit call currently
lacks a catch, so if submit(formData) rejects the error bubbles as an unhandled
rejection; wrap the await submit(...) in a try/catch (keeping the existing
finally) and in the catch block handle the failure: log the error, call any
error callback if available (e.g., onSubmitting/onError), and update the
messages state via setMessages(currentMessages => [...currentMessages, /* an
error/result marker message to surface the failure */]) or set an error state so
the UI shows feedback. Ensure setIsSubmitting and onSubmitting toggles remain in
the finally block and reference the existing submit, setMessages and
onSubmitting identifiers when implementing.
| const [isSubmitting, setIsSubmitting] = useState(false) | ||
| const [isSubmittingAction, setIsSubmittingAction] = useState(false) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Confusing naming between isSubmitting and isSubmittingAction.
isSubmitting (line 40) drives suggestion-based form submission via useEffect, while isSubmittingAction (line 41) tracks the actual async submit state from ChatPanel. These names don't convey their distinct purposes. Consider renaming for clarity, e.g., shouldTriggerSubmit for the former and isSubmitting for the latter.
♻️ Proposed rename for clarity
- const [isSubmitting, setIsSubmitting] = useState(false)
- const [isSubmittingAction, setIsSubmittingAction] = useState(false)
+ const [shouldTriggerSubmit, setShouldTriggerSubmit] = useState(false)
+ const [isSubmitting, setIsSubmitting] = useState(false)Then update all references accordingly: setIsSubmitting(true) in suggestion onSelect → setShouldTriggerSubmit(true), and the useEffect at lines 89–94 would use shouldTriggerSubmit/setShouldTriggerSubmit. The MobileIconsBar and ChatPanel wiring would use isSubmitting/setIsSubmitting.
🤖 Prompt for AI Agents
In `@components/chat.tsx` around lines 40 - 41, Rename the two confusing boolean
states to make their roles explicit: change const [isSubmitting,
setIsSubmitting] to const [shouldTriggerSubmit, setShouldTriggerSubmit] (this
state is used by the suggestion onSelect and the useEffect that triggers form
submission), and change const [isSubmittingAction, setIsSubmittingAction] to
const [isSubmitting, setIsSubmitting] (this state reflects the real async submit
status from ChatPanel/MobileIconsBar). Update all references accordingly:
replace setIsSubmitting(true) calls in suggestion onSelect with
setShouldTriggerSubmit(true), update the useEffect that watches submission
triggers to use shouldTriggerSubmit/setShouldTriggerSubmit, and wire the
ChatPanel and MobileIconsBar props/state to the new isSubmitting/setIsSubmitting
identifiers.
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| data-testid="mobile-search-button" | ||
| aria-label="Search" | ||
| title="Search" | ||
| > | ||
| <Search className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> |
There was a problem hiding this comment.
Search button has no onClick handler — it's non-functional.
This button renders but does nothing when clicked. Was this intentional as a placeholder, or should it be wired to a search action?
#!/bin/bash
# Check if the mobile search button had an onClick in previous versions or if there's a related handler
rg -n 'mobile-search-button' --type=tsx --type=ts -C3🤖 Prompt for AI Agents
In `@components/mobile-icons-bar.tsx` around lines 64 - 72, The Search button
(Button with data-testid "mobile-search-button" and the Search icon) has no
onClick and is non-functional; add an onClick handler that triggers the
component's search action—either call an existing prop like
onSearchClick/onOpenSearch if MobileIconsBar accepts one, or wire it to the
local search-open function/state (e.g., call openSearch()/setSearchOpen(true) or
the hook that opens the search modal). Ensure you update MobileIconsBar's props
type to include the handler name you choose (onSearchClick/onOpenSearch) if
needed and pass that handler into the Button as onClick.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Main concerns are around submission state consistency: parent/child submission flags can get out of sync (especially on unmount) and Chat now maintains two similar isSubmitting states. Additionally, responseMessage as any in the core submit path weakens maintainability and can mask runtime issues. Accessibility additions look solid overall.
Additional notes (2)
-
Readability |
components/chat.tsx:44-44
The mobile submit button now callschatPanelRef.current?.submitForm()regardless of whether submission is allowed, and relies on the child to ignore empty submissions. This is OK, but with the new loading state you should also prevent double-submission at the entry point: if the user taps rapidly, you can end up enqueueing multiplesubmitForm()calls beforeisSubmittingActionflips (React state updates are async). -
Maintainability |
components/mobile-icons-bar.tsx:33-33
MobileIconsBardisables the submit button whenisSubmittingis true, but it does not disable the other actions (e.g., New Chat / Clear Chat equivalent) during submission. Clearing chat while a submit is in-flight can create confusing UI state (you clear the UI, then the in-flight response appends a message back). If this is unintended, you should lock down conflicting actions during submission.
Summary of changes
Summary
This PR focuses on accessibility and submission feedback across the chat UI:
- Added a UX journal entry at
.Jules/palette.mddocumenting the need foraria-label+titleon icon-only controls. - Introduced a shared loading indicator via
Spinnerduring chat submission:components/chat-panel.tsxnow tracksisSubmitting, disables the send button, and swaps the send icon for<Spinner />.components/chat.tsxplumbs anonSubmittingcallback fromChatPanelto drive the mobile submit button state.components/mobile-icons-bar.tsxacceptsisSubmittingto disable the submit button and render a spinner.
- Added
aria-label/titleto multiple icon-only buttons (header, history toggle, attachment, send, etc.) to improve screen reader support and tooltips.
| const responseMessage = await submit(formData) | ||
| setMessages(currentMessages => [...currentMessages, responseMessage as any]) |
There was a problem hiding this comment.
The response message is appended with responseMessage as any. Even if tsc passes, this is an unsafe escape hatch that can hide real shape/compat issues at runtime and makes the state harder to reason about.
Since this code path is central (chat submission), avoiding any here is worth it.
Suggestion
Avoid as any by tightening the return type of submit (or by normalizing the message into the expected UI message type before pushing into state).
If submit returns a union, consider a small runtime discriminator/normalizer so setMessages always receives the same message shape.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const [input, setInput] = useState('') | ||
| const [showEmptyScreen, setShowEmptyScreen] = useState(false) | ||
| const [isSubmitting, setIsSubmitting] = useState(false) | ||
| const [isSubmittingAction, setIsSubmittingAction] = useState(false) |
There was a problem hiding this comment.
There are now two similarly-named submission states in Chat: isSubmitting and isSubmittingAction. Only isSubmittingAction is wired into the new spinner/disable flow, while isSubmitting is still toggled from EmptyScreen.
This is a correctness risk because different UI elements may consult different flags (now or in future edits), leading to inconsistent submission UX.
Suggestion
Consolidate to a single submission flag in Chat (e.g., keep only isSubmittingAction) and update the EmptyScreen handler to set that same flag.
If isSubmitting is still required for other logic, rename both to clarify intent (e.g., isEmptyScreenSubmitting vs isChatActionSubmitting) and document which drives which UI.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
2 participants
Added ARIA labels and titles to icon-only buttons for better accessibility and tooltips. Implemented a consistent loading spinner for chat submission on both desktop and mobile layouts. Added a UX journal to track learnings.
PR created automatically by Jules for task 5587028694497156105 started by @ngoiyaeric
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes