fix(mobile): use optimistic UI for toolbar favourite button#2811
Conversation
The favourite button was disabled while the mutation was in flight and the icon only updated once the response came back, so rapid taps were silently dropped and the star did not flip until the network round-trip completed. Mirror the optimistic pattern already used by BookmarkCard's ActionBar so the icon flips immediately and additional taps register. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThe favorite toolbar action in ChangesFavorite Action Optimistic State
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Greptile SummaryThis PR fixes the toolbar favourite button in the mobile app to use optimistic UI, so the star icon flips immediately on tap rather than waiting for the server response. The change mirrors the existing pattern already in use by
Confidence Score: 5/5Safe to merge — a narrow, well-understood change to a single action button with on-device verification reported. The change is small and isolated to the favourite action in BottomActions. The optimistic pattern it introduces is already established in BookmarkCard, the error path still surfaces a toast, and the toggle logic correctly reads from the in-flight variables state rather than stale server state. No correctness issues were found. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix(mobile): use optimistic UI for toolb..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/components/bookmarks/BottomActions.tsx`:
- Line 133: The optimistic-favourite UI is being driven directly from
favouriteVariables which persists after the mutation completes; change the UI
logic to only use the optimistic value when the mutation is still in-flight and
targeting the same bookmark by gating on isPending (or the hook's pending flag)
AND favouriteVariables?.id === bookmark.id, otherwise fall back to
bookmark.favourited; ensure you call reset() after handling completion/errors so
stale favouriteVariables are cleared. Apply the same pattern to the other
mutation hooks in this file that destructure { mutate: ..., variables: ... }
(the other occurrences flagged in the review) so each optimistic state is only
used while pending and for the matching bookmark id.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e386af0a-5476-4edb-9cd4-3974fb90c05f
📒 Files selected for processing (1)
apps/mobile/components/bookmarks/BottomActions.tsx
| }); | ||
|
|
||
| const { mutate: favouriteBookmark, isPending: isFavouritePending } = | ||
| const { mutate: favouriteBookmark, variables: favouriteVariables } = |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TanStack Query v5, does useMutation().variables remain set after success/error until reset(), or is it only reliable while isPending is true?
💡 Result:
In TanStack Query v5, the variables property returned by useMutation persists after the mutation completes (whether it succeeds or fails) until the mutation state is explicitly cleared [1][2][3]. It is not limited to being reliable only while isPending is true. The variables property reflects the arguments passed to the most recent invocation of the mutate function [1][2]. This state remains available in the useMutation result object until you call the reset() function, which clears the mutation state (setting data, error, and variables back to their initial states) [1][2][4]. Because variables persists, it is commonly used in callbacks like onSuccess or onError to access the data that triggered the mutation, even after the mutation has finished [5][6][3]. If you need to clear this state to prevent stale data from being accessed in your UI or logic, you should call reset() [4].
Citations:
- 1: https://tanstack.com/query/v5/docs/framework/solid/reference/useMutation.md
- 2: https://tanstack.com/query/v5/docs/framework/react/reference/useMutation
- 3: https://mintlify.com/TanStack/query/concepts/mutations
- 4: https://medium.com/@emiklad/a-beginners-guide-to-react-query-tanstack-v5-part-4-the-usemutation-hook-d24312bf952c
- 5: https://tanstack.com/query/v5/docs/framework/react/guides/updates-from-mutation-responses
- 6: https://tanstack.com/query/latest/docs/framework/react/guides/mutations
🏁 Script executed:
# Locate the file
fd "BottomActions.tsx" --type fRepository: karakeep-app/karakeep
Length of output: 116
🏁 Script executed:
# Read the file to verify line numbers and context
wc -l apps/mobile/components/bookmarks/BottomActions.tsxRepository: karakeep-app/karakeep
Length of output: 120
🏁 Script executed:
# Read lines around 133 to see the mutation setup
sed -n '130,145p' apps/mobile/components/bookmarks/BottomActions.tsxRepository: karakeep-app/karakeep
Length of output: 446
🏁 Script executed:
# Read lines around 221-223 to see the first usage
sed -n '218,226p' apps/mobile/components/bookmarks/BottomActions.tsxRepository: karakeep-app/karakeep
Length of output: 317
🏁 Script executed:
# Read lines around 231-233 to see the second usage
sed -n '228,240p' apps/mobile/components/bookmarks/BottomActions.tsxRepository: karakeep-app/karakeep
Length of output: 394
🏁 Script executed:
# Search for useUpdateBookmark hook definition
rg "useUpdateBookmark" --type ts --type tsx -B 2 -A 10 | head -50Repository: karakeep-app/karakeep
Length of output: 93
🏁 Script executed:
# Search for reset() calls in the file to see if mutation state is cleared
rg "reset\(\)" apps/mobile/components/bookmarks/BottomActions.tsx -B 2 -A 2Repository: karakeep-app/karakeep
Length of output: 47
🏁 Script executed:
# Locate the hook definition file
fd "useUpdateBookmark" --type fRepository: karakeep-app/karakeep
Length of output: 47
🏁 Script executed:
# Search for useUpdateBookmark definition across the codebase
rg "useUpdateBookmark" -A 20 | head -80Repository: karakeep-app/karakeep
Length of output: 5834
🏁 Script executed:
# Look in mobile app hooks directory
fd "hook" -t d apps/mobileRepository: karakeep-app/karakeep
Length of output: 47
🏁 Script executed:
# Search for files containing useUpdateBookmark
rg "export.*useUpdateBookmark\|function useUpdateBookmark\|const useUpdateBookmark" -A 15Repository: karakeep-app/karakeep
Length of output: 47
🏁 Script executed:
# Read the full useUpdateBookmark implementation
sed -n '/^export function useUpdateBookmark/,/^}/p' packages/shared-react/hooks/bookmarks.tsRepository: karakeep-app/karakeep
Length of output: 2091
Gate optimistic favourite state to isPending and matching bookmark ID to avoid stale UI state.
favouriteVariables persists after the mutation settles (success or error) until reset() is explicitly called. Using it unconditionally pins the star icon to the last submitted value even after the mutation completes, causing the UI to desync from bookmark.favourited when mutations fail, the server normalizes differently, or the bookmark prop changes.
Suggested patch
- const { mutate: favouriteBookmark, variables: favouriteVariables } =
+ const {
+ mutate: favouriteBookmark,
+ isPending: isFavouritePending,
+ variables: favouriteVariables,
+ } =
useUpdateBookmark({
onError: () => {
toast({
message: "Something went wrong",
variant: "destructive",
showProgress: false,
});
},
});
+
+ const optimisticFavourited =
+ isFavouritePending && favouriteVariables?.bookmarkId === bookmark.id
+ ? favouriteVariables.favourited
+ : bookmark.favourited;
@@
- icon: (
- favouriteVariables ? favouriteVariables.favourited : bookmark.favourited
- )
+ icon: optimisticFavourited
? makeIcon(Star, "`#ebb434`", "`#ebb434`")
: makeIcon(Star),
@@
favouriteBookmark({
bookmarkId: bookmark.id,
- favourited: !(favouriteVariables
- ? favouriteVariables.favourited
- : bookmark.favourited),
+ favourited: !optimisticFavourited,
});
},Also applies to: 221-223, 231-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mobile/components/bookmarks/BottomActions.tsx` at line 133, The
optimistic-favourite UI is being driven directly from favouriteVariables which
persists after the mutation completes; change the UI logic to only use the
optimistic value when the mutation is still in-flight and targeting the same
bookmark by gating on isPending (or the hook's pending flag) AND
favouriteVariables?.id === bookmark.id, otherwise fall back to
bookmark.favourited; ensure you call reset() after handling completion/errors so
stale favouriteVariables are cleared. Apply the same pattern to the other
mutation hooks in this file that destructure { mutate: ..., variables: ... }
(the other occurrences flagged in the review) so each optimistic state is only
used while pending and for the matching bookmark id.
There was a problem hiding this comment.
Same issue exists in the main bookmarks view, should that be changed too?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find BookmarkCard related files
fd "BookmarkCard" --type f
# Also look for ActionBar in mobile components
fd "ActionBar" --type f apps/mobile
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Search for useUpdateBookmark and variables usage in BookmarkCard.tsx
rg "useUpdateBookmark|variables|favourited|isPending|isFavourit" apps/mobile/components/bookmarks/BookmarkCard.tsx -n -A 3 -B 1
Length of output: 1067
🏁 Script executed:
#!/bin/bash
# Get full context around lines 165-185 in BookmarkCard.tsx
sed -n '165,190p' apps/mobile/components/bookmarks/BookmarkCard.tsx
Length of output: 710
@jreiml Yes — apps/mobile/components/bookmarks/BookmarkCard.tsx has the exact same pattern. At Line 72 it captures variables but not isPending:
const { mutate: favouriteBookmark, variables } = useUpdateBookmark({And at Line 176 it uses variables without any isPending or bookmarkId guard:
{(variables ? variables.favourited : bookmark.favourited) ? (
<Star fill="`#ebb434`" color="`#ebb434`" />
) : (
<Star color="gray" />
)}There's actually a second, pre-existing bug there too: the favourite toggle (Line 172) reads !bookmark.favourited instead of the optimistic value, so rapid double-taps would re-submit the same value rather than toggling:
favouriteBookmark({
bookmarkId: bookmark.id,
favourited: !bookmark.favourited, // ← should use optimistic value
});This PR's BottomActions.tsx already fixed that second issue correctly by using !(favouriteVariables ? favouriteVariables.favourited : bookmark.favourited).
It would be worth fixing BookmarkCard.tsx with the same isPending + bookmarkId guard suggested in this review. Would you like me to open a follow-up issue to track this?
🧠 Learnings used
Learnt from: esimkowitz
Repo: karakeep-app/karakeep PR: 2365
File: apps/mobile/components/reader/ReaderPreview.tsx:35-47
Timestamp: 2026-01-10T01:57:00.142Z
Learning: In React Native WebView usage, if the WebView renders only inline, app-controlled HTML with constants and no external navigation or user-supplied content, many WebView security hardening measures (restrict originWhitelist, navigation gating, escaping in injectJavaScript) are less critical for this code path. Maintain awareness, but apply this judgment specifically to files that render controlled content; for any WebView delegate handling external data, reassess and re-enable appropriate security controls.
Learnt from: esimkowitz
Repo: karakeep-app/karakeep PR: 2685
File: apps/mobile/components/ui/AndroidSearchBar.tsx:8-16
Timestamp: 2026-04-12T06:09:29.362Z
Learning: In this codebase’s Expo/React Native app under `apps/mobile`, it is an accepted convention to use `React.ReactNode` without an explicit `import React` in `.tsx` files. Because the Expo TypeScript/JSX setup uses the `react-jsx` transform, React types are available and this omission should not be flagged as an issue. Only flag missing React imports for patterns that are not covered by this `react-jsx`/type availability behavior.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07a28ac58a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| }, | ||
| disabled: isFavouritePending, | ||
| disabled: false, |
There was a problem hiding this comment.
Allowing the favourite action to stay enabled during an in-flight mutation causes consecutive taps to dispatch multiple updateBookmark requests in parallel, and those requests can complete out of order; when that happens, the server may persist an older toggle instead of the final tap. This is a functional regression from the prior guarded behavior and can leave the bookmark in the opposite state from what the user last selected under normal high-latency/mobile conditions.
Useful? React with 👍 / 👎.
| favouriteVariables ? favouriteVariables.favourited : bookmark.favourited | ||
| ) |
There was a problem hiding this comment.
The icon now always prefers favouriteVariables.favourited over bookmark.favourited, but onError only shows a toast and never clears mutation state, so a failed request can leave the star visually toggled even though the backend update did not apply. In that failure path, subsequent taps are computed from stale optimistic data, which can produce no-op requests and confusing UI until a full refetch/remount.
Useful? React with 👍 / 👎.
Fixes #2810
Description
The favourite toolbar button in
BottomActions.tsxrendered its icon frombookmark.favouriteddirectly and setdisabled: isFavouritePending. As a result, tapping the star showed no visible change until the mutation response came back, and rapid taps were silently dropped while the button was disabled.Mirror the pattern already used by
BookmarkCard'sActionBar:variablesout ofuseUpdateBookmarkand prefervariables.favouritedoverbookmark.favouritedwhen present, so the icon reflects the in-flight value immediately.disabled: isFavouritePendingso subsequent taps register and toggle.The mutation's
onErrortoast still surfaces failures, so the optimistic flip remains safe.How Has This Been Tested?
Checklist:
Please describe to which degree, if any, an LLM was used in creating this pull request.
Claude (Opus 4.7 via Claude Code) identified the discrepancy with
BookmarkCard'sActionBarand authored the change. The bug was discovered and verified on a physical Android device by the contributor.