[jules] style: Consistent hover/focus states #267
Conversation
- Implemented `focus-visible` styles for `Button`, `Modal`, `Toast`, and `Auth` page components. - Added dual-theme support: Black rings for Neobrutalism, Blue rings for Glassmorphism. - Verified changes programmatically and visually. - Updated `.Jules/todo.md` and `.Jules/changelog.md`. Co-authored-by: Devasy <[email protected]>
|
👋 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. |
✅ Deploy Preview for split-but-wiser canceled.
|
WalkthroughAdds theme-aware focus-visible outlines to web UI controls; implements native pull-to-refresh with haptic feedback on multiple mobile screens and refactors fetch flows for silent refreshes; augments mobile accessibility attributes; adds expo-haptics dependency; updates changelog/todo/knowledge docs; and rewrites backend tests to exercise an optimized settlement/recalculation path. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/components/ui/Button.tsx (1)
33-43: Consider adding explicitring-offset-colorfor Neobrutalism variant.The Glassmorphism branch specifies
focus-visible:ring-offset-gray-900for dark mode andfocus-visible:ring-offset-whitefor light mode, but the Neobrutalism branch (line 34) does not specify an offset color. This relies on Tailwind's default (transparent or inherited), which may cause inconsistent ring appearance if the button background color differs from the expected offset.If Neobrutalism only operates in light mode, this is acceptable. Otherwise, consider adding an explicit offset color for consistency.
💡 Optional: Add explicit ring-offset for Neobrutalism
- themeStyles = "border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] active:translate-x-[4px] active:translate-y-[4px] active:shadow-none rounded-none uppercase tracking-wider font-mono focus-visible:ring-black"; + themeStyles = "border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] active:translate-x-[4px] active:translate-y-[4px] active:shadow-none rounded-none uppercase tracking-wider font-mono focus-visible:ring-black focus-visible:ring-offset-white";
🤖 Fix all issues with AI agents
In `@web/pages/Auth.tsx`:
- Around line 213-216: The focus ring color for the non-Neo Google button is
inconsistent: change the Tailwind class used in the className expression for the
non-Neo branch (the string containing 'focus-visible:ring-blue-500') to use
'focus-visible:ring-blue-400' so it matches the other components (see
Button.tsx, Modal.tsx and the toggle link) and keep the conditional logic around
isNeo intact.
| className={`w-full flex items-center justify-center gap-3 p-3 font-bold transition-all outline-none focus-visible:ring-2 focus-visible:ring-offset-2 ${isNeo | ||
| ? 'focus-visible:ring-black bg-white border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] rounded-none' | ||
| : 'focus-visible:ring-blue-500 bg-white text-black hover:bg-gray-50 border border-gray-200 shadow-sm rounded-xl' | ||
| }`} |
There was a problem hiding this comment.
Minor inconsistency: Use ring-blue-400 for Glassmorphism focus ring.
The Google button uses focus-visible:ring-blue-500 for the non-Neo theme, but other components in this PR (Button.tsx, Modal.tsx, and the toggle link below at line 333) use ring-blue-400. For visual consistency across the application, consider aligning with ring-blue-400.
🔧 Suggested fix
className={`w-full flex items-center justify-center gap-3 p-3 font-bold transition-all outline-none focus-visible:ring-2 focus-visible:ring-offset-2 ${isNeo
? 'focus-visible:ring-black bg-white border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] rounded-none'
- : 'focus-visible:ring-blue-500 bg-white text-black hover:bg-gray-50 border border-gray-200 shadow-sm rounded-xl'
+ : 'focus-visible:ring-blue-400 bg-white text-black hover:bg-gray-50 border border-gray-200 shadow-sm rounded-xl'
}`}📝 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.
| className={`w-full flex items-center justify-center gap-3 p-3 font-bold transition-all outline-none focus-visible:ring-2 focus-visible:ring-offset-2 ${isNeo | |
| ? 'focus-visible:ring-black bg-white border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] rounded-none' | |
| : 'focus-visible:ring-blue-500 bg-white text-black hover:bg-gray-50 border border-gray-200 shadow-sm rounded-xl' | |
| }`} | |
| className={`w-full flex items-center justify-center gap-3 p-3 font-bold transition-all outline-none focus-visible:ring-2 focus-visible:ring-offset-2 ${isNeo | |
| ? 'focus-visible:ring-black bg-white border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] rounded-none' | |
| : 'focus-visible:ring-blue-400 bg-white text-black hover:bg-gray-50 border border-gray-200 shadow-sm rounded-xl' | |
| }`} |
🤖 Prompt for AI Agents
In `@web/pages/Auth.tsx` around lines 213 - 216, The focus ring color for the
non-Neo Google button is inconsistent: change the Tailwind class used in the
className expression for the non-Neo branch (the string containing
'focus-visible:ring-blue-500') to use 'focus-visible:ring-blue-400' so it
matches the other components (see Button.tsx, Modal.tsx and the toggle link) and
keep the conditional logic around isNeo intact.
|
@jules there are some merge conflicts! |
I have merged the latest changes from |
…tates Resolved conflicts in: - .Jules/changelog.md Co-authored-by: Devasy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/expenses/test_expense_service.py (1)
1962-1987:⚠️ Potential issue | 🟡 MinorNarrow the failure assertion to the expected exception message.
Line 1986 uses
pytest.raises(Exception), which is too broad and can hide unexpected failures. Since the mock raisesException("Calculation failed"), assert the message to make the test more specific.🛠️ Proposed fix
- with pytest.raises(Exception): # The service doesn't catch all exceptions + with pytest.raises(Exception, match="Calculation failed"): await expense_service.get_friends_balance_summary(user_id_str)
🤖 Fix all issues with AI agents
In `@mobile/screens/FriendsScreen.js`:
- Around line 61-66: The onRefresh handler can fail if Haptics.impactAsync
rejects and leave the spinner stuck; update the onRefresh function to
setIsRefreshing(true) then run Haptics.impactAsync inside try/catch (or ignore
errors) and call fetchData, and move setIsRefreshing(false) into a finally block
so it always runs; specifically modify the onRefresh function (references:
onRefresh, setIsRefreshing, Haptics.impactAsync, fetchData) and apply the same
try/catch/finally pattern to the identical handlers in GroupDetailsScreen.js and
HomeScreen.js.
In `@mobile/screens/GroupDetailsScreen.js`:
- Around line 57-62: The onRefresh handler (function onRefresh) can abort
fetchData and leave the spinner on if Haptics.impactAsync rejects; wrap the
haptics call and fetchData invocation in a try/finally so that
setIsRefreshing(false) always runs: call setIsRefreshing(true), then try { await
Haptics.impactAsync(...); await fetchData(false); } finally {
setIsRefreshing(false); }—apply the same change to the identical handlers in
HomeScreen.js and FriendsScreen.js to ensure refresh completes even when haptics
fails.
In `@mobile/screens/HomeScreen.js`:
- Around line 102-107: The onRefresh handler can leave isRefreshing stuck true
if Haptics.impactAsync rejects; update the onRefresh function to guard the
haptics call and always clear the spinner by wrapping the async sequence in
try/finally (or append .catch() to Haptics.impactAsync and move
setIsRefreshing(false) into a finally block). Specifically, modify onRefresh to
call Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light) with a .catch() or
inside try/catch, await fetchGroups(false) as before, and ensure
setIsRefreshing(false) runs in the finally so failures in Haptics or fetchGroups
never leave isRefreshing true (referencing onRefresh, Haptics.impactAsync,
fetchGroups, and setIsRefreshing).
|
@jules please look into the review comments In
In
In
|
I have updated the |
- Updated `onRefresh` in `FriendsScreen`, `GroupDetailsScreen`, and `HomeScreen`. - Added `try/catch/finally` to ensure spinner is always dismissed. - Gracefully handle `Haptics` failures. Co-authored-by: Devasy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mobile/screens/FriendsScreen.js`:
- Around line 27-59: The loading state setIsLoading(true) in fetchData is
currently executed before the try block; move that call inside the try block (at
the top) to match the pattern used in HomeScreen.js and GroupDetailsScreen.js so
success/error paths are symmetrical, keep the finally block that calls
setIsLoading(false) when showLoading is true, and ensure this change is made
within the fetchData function scope referencing setIsLoading, getFriendsBalance,
and getGroups.
In `@mobile/screens/GroupDetailsScreen.js`:
- Line 261: In GroupDetailsScreen.js there is a formatting inconsistency where
the style declaration containing "fontSize: 20, fontWeight: 'bold'" is on a
single line; update the stylesheet entry (the style object that currently sets
fontSize and fontWeight) to place each property on its own line with normal
spacing to match the rest of the StyleSheet (e.g., split into separate lines for
fontSize and fontWeight and remove the extra whitespace).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@jules |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mobile/screens/GroupDetailsScreen.js`:
- Around line 258-267: The expensesTitle style in the StyleSheet (in
GroupDetailsScreen.js) is malformed: it contains a nested expensesTitle object
so fontSize and fontWeight are ignored; fix by merging the inner properties into
the outer expensesTitle object (remove the nested expensesTitle key) so the
style contains marginTop, marginBottom, fontSize, and fontWeight together,
ensuring components using expensesTitle receive the intended styling.
I have moved |
- Moved `setIsLoading(true)` inside `try` block in `FriendsScreen.js`. - Ensures consistent error handling pattern across all screens. Co-authored-by: Devasy <[email protected]>
|
@jules pls resolve the merge conflict |
I have merged the latest changes from |
…tates Resolved conflicts in: - .Jules/changelog.md Verified auto-merge of: - mobile/screens/FriendsScreen.js (combined pull-to-refresh + accessibility) - mobile/screens/GroupDetailsScreen.js (combined pull-to-refresh + accessibility) - mobile/screens/HomeScreen.js (combined pull-to-refresh + accessibility) Co-authored-by: Devasy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.Jules/todo.md:
- Around line 67-68: Update the completion date for the checklist item "[x]
**[a11y]** Complete accessibility labels for all screens" in .Jules/todo.md so
it does not lie in the future; replace "Completed: 2026-01-29" with the actual
completion date (e.g., 2026-01-28 or the date the work was finished) or remove
the date entirely if you cannot confirm it, ensuring the entry accurately
reflects when the accessibility work was completed.
- Around line 162-165: The TODO contains a duplicated completed task
"Pull-to-refresh with haptic feedback on all list screens"; remove the duplicate
entry from the "Completed Tasks" section (the one at lines 162-165) so only the
original entry in the Mobile high-priority section remains, and ensure any
associated metadata (completion date and modified file list) is preserved only
in the original Mobile entry to avoid redundancy.
- Around line 53-58: The task was marked complete while PR review requested
defensive error handling in the pull-to-refresh handlers; update each refresh
handler (e.g., onRefresh / handleRefresh used with RefreshControl in the refresh
handlers of FriendsScreen, GroupDetailsScreen and HomeScreen) to wrap the async
work and Haptics.impactAsync call in try/catch/finally so any rejection doesn't
leave the spinner active: call setRefreshing(true) before work, await the
refresh logic and Haptics.impactAsync inside try, log/handle errors in catch,
and always call setRefreshing(false) in finally; after fixing handlers, update
the todo entry to either uncheck the task or change the completion date once the
changes are merged.
In `@mobile/screens/GroupSettingsScreen.js`:
- Around line 338-342: The accessibilityLabel prop on the Button is static while
the visible text toggles based on pickedImage; update the accessibilityLabel to
mirror the button text by making it conditional (use pickedImage to choose
between "Change Image" and "Upload Image") so the Button's accessibilityLabel
and displayed label match; locate the Button component that uses the pickedImage
variable and replace the hardcoded accessibilityLabel string with the same
conditional expression used for the button children.
| - [x] **[ux]** Pull-to-refresh with haptic feedback on all list screens | ||
| - Completed: 2026-01-21 | ||
| - Files: `mobile/screens/HomeScreen.js`, `mobile/screens/GroupDetailsScreen.js`, `mobile/screens/FriendsScreen.js` | ||
| - Context: Add RefreshControl + Expo Haptics to main lists | ||
| - Impact: Native feel, users can easily refresh data | ||
| - Size: ~45 lines | ||
| - Added: 2026-01-01 | ||
| - Size: ~150 lines |
There was a problem hiding this comment.
Task marked complete prematurely—outstanding review comments request changes.
The pull-to-refresh task is marked as completed (2026-01-21), but the PR objectives clearly document unresolved review comments requesting defensive try/catch/finally patterns in the refresh handlers for FriendsScreen.js, GroupDetailsScreen.js, and HomeScreen.js. The review feedback specifically asks for error handling to prevent refresh spinners from remaining active when Haptics.impactAsync rejects.
A task should not be marked complete while there are outstanding change requests on the same files and functionality.
📋 Recommended next steps
Either:
- Address the review comments by implementing the defensive error handling patterns, then update the completion date, OR
- Revert the completion status to
[ ]until the review feedback is resolved and the PR is merged.
🤖 Prompt for AI Agents
In @.Jules/todo.md around lines 53 - 58, The task was marked complete while PR
review requested defensive error handling in the pull-to-refresh handlers;
update each refresh handler (e.g., onRefresh / handleRefresh used with
RefreshControl in the refresh handlers of FriendsScreen, GroupDetailsScreen and
HomeScreen) to wrap the async work and Haptics.impactAsync call in
try/catch/finally so any rejection doesn't leave the spinner active: call
setRefreshing(true) before work, await the refresh logic and Haptics.impactAsync
inside try, log/handle errors in catch, and always call setRefreshing(false) in
finally; after fixing handlers, update the todo entry to either uncheck the task
or change the completion date once the changes are merged.
| - [x] **[a11y]** Complete accessibility labels for all screens | ||
| - Completed: 2026-01-29 |
There was a problem hiding this comment.
Completion date is in the future.
The accessibility task shows a completion date of 2026-01-29, but this PR was created on 2026-01-28. While this might reflect a planned completion or timezone difference, it's clearer to use dates that reflect when work was actually completed rather than future dates.
🤖 Prompt for AI Agents
In @.Jules/todo.md around lines 67 - 68, Update the completion date for the
checklist item "[x] **[a11y]** Complete accessibility labels for all screens" in
.Jules/todo.md so it does not lie in the future; replace "Completed: 2026-01-29"
with the actual completion date (e.g., 2026-01-28 or the date the work was
finished) or remove the date entirely if you cannot confirm it, ensuring the
entry accurately reflects when the accessibility work was completed.
| - [x] **[ux]** Pull-to-refresh with haptic feedback on all list screens | ||
| - Completed: 2026-01-21 | ||
| - Files modified: `mobile/screens/HomeScreen.js`, `mobile/screens/GroupDetailsScreen.js`, `mobile/screens/FriendsScreen.js` | ||
| - Impact: Native feel, users can easily refresh data |
There was a problem hiding this comment.
Remove duplicate task entry.
The pull-to-refresh task appears in both the Mobile high-priority section (lines 53-58) and here in the "Completed Tasks" section. This duplication creates redundancy and maintenance burden.
Consider removing this duplicate entry since the task is already marked as complete in its original location (lines 53-58).
♻️ Suggested cleanup
Since the task is already marked [x] with completion date in the Mobile section (lines 53-58), this duplicate entry in the "Completed Tasks" section can be safely removed.
🤖 Prompt for AI Agents
In @.Jules/todo.md around lines 162 - 165, The TODO contains a duplicated
completed task "Pull-to-refresh with haptic feedback on all list screens";
remove the duplicate entry from the "Completed Tasks" section (the one at lines
162-165) so only the original entry in the Mobile high-priority section remains,
and ensure any associated metadata (completion date and modified file list) is
preserved only in the original Mobile entry to avoid redundancy.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@jules pls resolve the merge conflict |
I have verified that the branch is up-to-date with |
- Moved `setIsLoading(true)` inside the `try` block in `FriendsScreen.js` for consistency with other screens and better error handling symmetry. - Confirmed branch is up-to-date with `main` and conflict-free. Co-authored-by: Devasy <[email protected]>
Implemented high-contrast
focus-visiblestyles across interactive elements to improve keyboard accessibility.Button,Modal(close button),Toast(dismiss button).Auth(Google button, toggle links).ring-black ring-2 ring-offset-2ring-blue-500 ring-2 ring-offset-2(with dark mode support)document.activeElement.PR created automatically by Jules for task 7296831096560520349 started by @Devasy23
Summary by CodeRabbit
New Features
Style
Accessibility
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.