Add bookmark support and bookmarks screen#144
Merged
malkoG merged 10 commits intohackers-pub:mainfrom Apr 21, 2026
Merged
Conversation
Add bookmark fields and mutations to the GraphQL layer. Extend repository and overlay mapping so screens can load and optimistically reflect bookmark state. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Show bookmark controls in cards and post detail actions. Hook optimistic toggles into the existing feed and detail view models so bookmark state updates immediately. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Add a dedicated bookmarks screen with All, Articles, and Notes tabs. Wire it into Settings so bookmarked posts remain discoverable from the app. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
📝 WalkthroughWalkthroughThis pull request introduces a complete bookmarking feature for posts. It adds GraphQL operations and schema definitions for bookmark queries and mutations, extends the data layer with repository methods and paging support, updates the domain model with a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Post Card<br/>UI
participant VM as ViewModel<br/>(e.g., Timeline)
participant Overlay as Overlay<br/>Store
participant Repo as Repository
participant Apollo as Apollo<br/>Client
User->>UI: Tap Bookmark Button
UI->>VM: toggleBookmark(post)
rect rgba(0, 150, 136, 0.5)
Note over VM,Overlay: Optimistic Update
VM->>Overlay: mutate(postId)
Overlay->>Overlay: Set viewerHasBookmarked = true
end
Note over UI: UI Re-composes<br/>with New State
rect rgba(33, 150, 243, 0.5)
Note over Repo,Apollo: Async Persistence
VM->>Repo: bookmarkPost(postId)
Repo->>Apollo: Execute Mutation
Apollo->>Repo: Return Result
end
alt Success
Repo->>VM: Result.success(Unit)
VM->>VM: Overlay persisted
else Failure
Repo->>VM: Result.failure(error)
rect rgba(244, 67, 54, 0.5)
Note over VM,Overlay: Rollback
VM->>Overlay: mutate(postId)
Overlay->>Overlay: Set viewerHasBookmarked = false
end
Overlay->>UI: Notify Observers
UI->>UI: Re-compose with Reverted State
end
sequenceDiagram
participant User
participant Screen as Bookmarks<br/>Screen
participant VM as Bookmarks<br/>ViewModel
participant Pager as Cursor<br/>Pager
participant Repo as Repository
participant Apollo as Apollo<br/>Client
User->>Screen: Open Bookmarks
Screen->>VM: Select Tab (ALL/ARTICLES/NOTES)
VM->>Pager: Load Page 1
Pager->>Repo: bookmarksPage(after=null, postType)
Repo->>Apollo: Execute BookmarksQuery
Apollo->>Repo: Return Edges + PageInfo
Repo->>VM: Return Posts + endCursor + hasNextPage
VM->>Screen: Emit PagingData<Post>
Screen->>Screen: Render LazyColumn with Posts
User->>Screen: Scroll to Bottom (Pagination)
Pager->>Repo: bookmarksPage(after=cursor, postType)
Repo->>Apollo: Execute BookmarksQuery
Apollo->>Repo: Return Next Edges + PageInfo
Repo->>VM: Return Additional Posts
VM->>Screen: Emit Updated PagingData
Screen->>Screen: Append Posts to List
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/pub/hackers/android/data/paging/PostOverlay.kt (1)
23-29:⚠️ Potential issue | 🟠 MajorAvoid clearing unrelated optimistic bookmark state.
Adding
viewerHasBookmarkedmakes one overlay entry hold independent fields. Existing reaction rollback paths that calloverlayStore.clear(target.id)will now also drop bookmark/share overlays, so a failed reaction can visually undo an unrelated bookmark action. Add field-specific clearing for reactions and use it from reaction failure handlers.Proposed field-specific rollback helper
class PostOverlayStore { private val _overlays = MutableStateFlow<Map<String, PostOverlay>>(emptyMap()) val overlays: StateFlow<Map<String, PostOverlay>> = _overlays fun mutate(postId: String, block: (PostOverlay) -> PostOverlay) { _overlays.update { current -> current + (postId to block(current[postId] ?: PostOverlay())) } } + fun clearReaction(postId: String) { + _overlays.update { current -> + val existing = current[postId] ?: return@update current + val next = existing.copy( + reactionOverride = null, + reactionCountOverride = null, + ) + if (next == PostOverlay()) current - postId else current + (postId to next) + } + } + fun clear(postId: String) { _overlays.update { it - postId } } }Then replace reaction failure rollbacks like:
- overlayStore.clear(target.id) + overlayStore.clearReaction(target.id)Also applies to: 74-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/data/paging/PostOverlay.kt` around lines 23 - 29, The PostOverlay data class now contains independent fields (viewerHasBookmarked, viewerHasShared, reactionOverride, reactionCountOverride, shareDelta) but existing rollback code calls overlayStore.clear(target.id) which removes all overlay fields and can unintentionally clear bookmarks/shares; add a field-specific clearing method on the overlay store (e.g., clearReactionOverlay or removeReactionFields) that only clears reactionOverride and reactionCountOverride (and any reaction-related deltas), then replace calls to overlayStore.clear(target.id) from reaction failure handlers with this new method so only reaction overlays are rolled back while viewerHasBookmarked/viewerHasShared remain intact.
🧹 Nitpick comments (5)
app/src/main/java/pub/hackers/android/data/paging/CursorPagingSource.kt (1)
118-122: Nit: importPostTypeinstead of fully-qualifying.The other adapters in this file use short type names; inline
pub.hackers.android.graphql.type.PostTypebreaks the pattern. Add animport pub.hackers.android.graphql.type.PostTypeat the top and usepostType: PostType?.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/data/paging/CursorPagingSource.kt` around lines 118 - 122, The function signature in CursorPagingSource.kt uses a fully-qualified type for postType; update it to use a short import by adding import pub.hackers.android.graphql.type.PostType at the top of the file and change the bookmarksPage parameter from postType: pub.hackers.android.graphql.type.PostType? to postType: PostType? in the suspend fun HackersPubRepository.bookmarksPage(...) declaration so it matches the other adapters' style.app/src/main/java/pub/hackers/android/ui/screens/explore/ExploreScreen.kt (1)
219-235: Duplicate of TimelineScreen bookmark handler; same premature-toast issue.This is byte-for-byte the same handler as
TimelineScreen.ktlines 268-282 and has the same defect: toast fires before the repository call resolves, so on failure the user sees a success message while the optimistic update is rolled back, and thefailed_to_bookmark/failed_to_remove_bookmarkstrings added in this PR are never surfaced.Extract a shared helper (e.g., a composable/extension that takes
post+ atogglelambda) and route failure feedback through the ViewModel so both screens stay in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/ui/screens/explore/ExploreScreen.kt` around lines 219 - 235, The onBookmarkClick handler in ExploreScreen (byte-for-byte duplicate of TimelineScreen's handler) shows a success Toast immediately before viewModel.toggleBookmark completes, so failures never surface; extract a shared helper (e.g., a composable or extension function) that takes the Post (use post/sharedPost resolution logic currently in the handler) and a toggle lambda, replace the inline handlers in both ExploreScreen and TimelineScreen with this helper, and move success/failure Toast emission into the ViewModel's result path so viewModel.toggleBookmark publishes success or error states (use the existing failed_to_bookmark / failed_to_remove_bookmark strings) which the helper observes to show the correct Toast. Ensure the helper calls viewModel.toggleBookmark only once and does not optimistic-toast prior to the repository response.app/src/test/java/pub/hackers/android/ui/screens/postdetail/PostDetailContentTest.kt (1)
161-161: Consider adding a dedicated test coveringonBookmarkClickwiring.Passing
{}keeps the existing tests compiling, but there's no assertion that the bookmark button actually invokes the callback. A small test that finds the bookmark action and verifies the lambda fires would prevent regressions in the new UI wiring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/pub/hackers/android/ui/screens/postdetail/PostDetailContentTest.kt` at line 161, Add a test in PostDetailContentTest that verifies the onBookmarkClick lambda is invoked: render the PostDetailContent composable with a side-effecting lambda (e.g., set an AtomicBoolean or mutableState flag) passed as onBookmarkClick, locate the bookmark button node via its test tag or content description (e.g., onNodeWithTag("bookmark") or onNodeWithContentDescription("Bookmark")), call performClick() on that node, and assert the flag changed to true; reference the onBookmarkClick parameter in the PostDetailContent invocation and the test node selector you use to wire the assertion.app/src/main/graphql/pub/hackers/android/operations.graphql (1)
542-565: Consider selecting the error fragments on the bookmark result unions for clearer error surfacing.Per schema,
BookmarkPostResultandUnbookmarkPostResultalso includeInvalidInputErrorandNotAuthenticatedError(lines 1937 and 1953 inschema.graphqls). Peer mutations likeSharePost/UnsharePost(lines 609–650) explicitly match on those variants so the repository can return a meaningful message. Here,HackersPubRepository.bookmarkPost/unbookmarkPosthas to fall back to a generic"Failed to bookmark"/"Failed to remove bookmark"message whenever the union resolves to an error type, which makes auth failures indistinguishable from validation failures in the UI.♻️ Suggested additions
mutation BookmarkPost($postId: ID!) { bookmarkPost(input: { postId: $postId }) { __typename ... on BookmarkPostPayload { post { id viewerHasBookmarked } } + ... on InvalidInputError { + inputPath + } + ... on NotAuthenticatedError { + notAuthenticated + } } } mutation UnbookmarkPost($postId: ID!) { unbookmarkPost(input: { postId: $postId }) { __typename ... on UnbookmarkPostPayload { post { id viewerHasBookmarked } unbookmarkedPostId } + ... on InvalidInputError { + inputPath + } + ... on NotAuthenticatedError { + notAuthenticated + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/graphql/pub/hackers/android/operations.graphql` around lines 542 - 565, Add explicit selections for the error union variants to the BookmarkPost and UnbookmarkPost mutations so the client can distinguish InvalidInputError and NotAuthenticatedError; update the BookmarkPost/UnbookmarkPost operations (mutations named BookmarkPost and UnbookmarkPost in app/src/main/graphql/pub/hackers/android/operations.graphql) to match on the union variants (InvalidInputError and NotAuthenticatedError) and return their message/code fields, and then propagate those fields through HackersPubRepository.bookmarkPost and HackersPubRepository.unbookmarkPost so callers can show specific error messages instead of the generic "Failed to bookmark"/"Failed to remove bookmark".app/src/main/java/pub/hackers/android/data/repository/HackersPubRepository.kt (1)
257-288: Minor consistency nits ongetBookmarks.Two small points, both non-blocking:
- The signature uses the fully qualified
pub.hackers.android.graphql.type.PostType?, while the rest of this file imports GraphQL types (e.g.GqlPostVisibilityalias at line 66). Consider adding an import such asimport pub.hackers.android.graphql.type.PostType as GqlPostTypefor consistency with the conventions already in use.- Peer methods like
getActorPosts(L496–498) andgetPostReplies(L459–461) defensively apply.distinctBy { it.id }on edges because the server can return duplicates across pages.getBookmarksskips this. It is probably fine becauseBookmarksViewModelappliesdistinctByEffectiveId()at the paging layer, but matching peer methods here would keep repository-level behavior uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/data/repository/HackersPubRepository.kt` around lines 257 - 288, Add a GraphQL PostType import alias and apply the same deduplication as other repo methods: import pub.hackers.android.graphql.type.PostType as GqlPostType and update the getBookmarks(...) implementation to deduplicate edges before mapping (e.g. call .distinctBy { it.node.postFields.id } or .distinctBy { it.id } on data?.edges) so TimelineResult(posts = ...) produces unique posts like getActorPosts/getPostReplies; keep the rest of the logic (mapping to TimelineResult and using postFields.toPost(...)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/pub/hackers/android/ui/screens/bookmarks/BookmarksScreen.kt`:
- Around line 207-221: The Toast is shown optimistically in the composable
(inside onBookmarkClick) before the mutation (viewModel.toggleBookmark)
resolves, causing UI/Toast mismatch on rollback; remove the synchronous
Toast.makeText(...) call from the onBookmarkClick handler and instead emit a
one-shot success/failure event from BookmarksViewModel.toggleBookmark (e.g., a
SharedFlow/Channel/SingleLiveEvent) after the repository call completes or after
rollback, carrying enough info to pick the correct string (use
displayPost.viewerHasBookmarked or the result state). In the composable
subscribe/collect that one-shot event and show the Toast there so the toast
reflects the final resolved outcome.
In
`@app/src/main/java/pub/hackers/android/ui/screens/bookmarks/BookmarksViewModel.kt`:
- Around line 142-152: The failure handler in toggleReaction currently calls
overlayStore.clear(target.id) which wipes all optimistic state; instead, in
result.onFailure replace that clear call with a scoped rollback using
overlayStore.mutate(target.id) { prev -> prev.copy(...) } so only
reaction-related fields are reverted: set reactionOverride back to null (or to
its previous value) and adjust reactionCountOverride to undo the optimistic
increment/decrement applied when toggling the reaction, leaving other fields
(viewerHasBookmarked, viewerHasShared, shareDelta, etc.) untouched.
In
`@app/src/main/java/pub/hackers/android/ui/screens/explore/ExploreViewModel.kt`:
- Around line 106-126: toggleBookmark currently fires requests immediately and
can interleave; introduce a per-post in-flight guard + “last desired state”
store keyed by target.id (e.g., a MutableMap<Long, Boolean> in the ViewModel) to
serialize mutations. On toggleBookmark update the overlayStore immediately and
set lastDesired[target.id] = willBookmark; if inFlight[target.id] is true
return; otherwise set inFlight[target.id]=true and launch the network call using
repository.bookmarkPost/repository.unbookmarkPost; on completion (success or
failure) clear inFlight[target.id], compare lastDesired[target.id] with the
state you just applied and if they differ enqueue/send another request to
converge to lastDesired, and on failure only revert overlayStore if lastDesired
indicates the opposite state (to avoid clobbering newer toggles). Use the same
symbols: toggleBookmark, overlayStore.mutate, repository.bookmarkPost,
repository.unbookmarkPost, viewModelScope.launch, and per-post maps (inFlight
and lastDesired) to locate where to implement this.
In
`@app/src/main/java/pub/hackers/android/ui/screens/postdetail/PostDetailScreen.kt`:
- Around line 491-504: The bookmark tap currently always shows a success toast
and calls viewModel.toggleBookmark() even when the user is not logged in; update
the onBookmarkClick handler in PostDetailScreen to first check the screen's
isLoggedIn flag and only proceed with the toast and viewModel.toggleBookmark()
when isLoggedIn is true, otherwise short-circuit (e.g., show a login-required
message or open auth flow) so unauthenticated users cannot trigger the mutation;
locate the onBookmarkClick block that references
resolvedPost.viewerHasBookmarked and viewModel.toggleBookmark() and add the
guard around those actions.
In
`@app/src/main/java/pub/hackers/android/ui/screens/postdetail/PostDetailViewModel.kt`:
- Around line 289-312: toggleBookmark currently performs an optimistic update
but neither surfaces errors to the UI nor prevents reentrant taps; add an
isBookmarking Boolean in PostDetailViewModel to early-return while a request is
in flight, update it around the network call, and emit a transient error
event/string resource on failure so PostDetailScreen can show a snackbar;
specifically: in toggleBookmark guard with isBookmarking, set isBookmarking =
true before calling repository.bookmarkPost/post.unbookmarkPost and false in
finally, keep the optimistic _uiState update using post.copy(viewerHasBookmarked
= willBookmark), and on result.onFailure emit the appropriate
R.string.failed_to_bookmark or R.string.failed_to_remove_bookmark via the
ViewModel’s event/transient error channel while rolling back the optimistic
change as already implemented.
In `@app/src/main/java/pub/hackers/android/ui/screens/settings/SettingsScreen.kt`:
- Around line 237-258: Wrap the Bookmarks Row in a feature-flag check so it only
renders when bookmarks are supported: add a boolean like bookmarksEnabled =
FeatureFlags.BOOKMARKS_ENABLED (mirroring the existing passkeyEnabled pattern)
and change the unconditional Row that calls onBookmarksClick() to be inside if
(bookmarksEnabled) { ... }. Ensure you reference FeatureFlags.BOOKMARKS_ENABLED,
keep the existing onBookmarksClick() callback and UI contents intact, and follow
the same conditional/gating style used for passkeyEnabled in SettingsScreen.kt.
In `@app/src/main/java/pub/hackers/android/ui/screens/timeline/TimelineScreen.kt`:
- Around line 268-282: The toast in TimelineScreen's onBookmarkClick handler is
shown before TimelineViewModel.toggleBookmark completes, causing misleading
success messages; move toast/snackbar emission into the ViewModel (or expose a
failure Flow) so success vs failure messages are emitted after
repository.bookmarkPost/unbookmarkPost returns, using a one-shot event (e.g.,
Channel/SharedFlow) or a SnackbarHostState event for R.string.bookmarked,
R.string.bookmark_removed, R.string.failed_to_bookmark, and
R.string.failed_to_remove_bookmark; update TimelineViewModel.toggleBookmark to
emit the appropriate success/failure event after the mutation (rollback still
allowed) and have TimelineScreen observe that event to show the toast/snackbar,
and also extract the shared onBookmarkClick handler into a common function used
by ExploreScreen.kt to avoid duplication.
---
Outside diff comments:
In `@app/src/main/java/pub/hackers/android/data/paging/PostOverlay.kt`:
- Around line 23-29: The PostOverlay data class now contains independent fields
(viewerHasBookmarked, viewerHasShared, reactionOverride, reactionCountOverride,
shareDelta) but existing rollback code calls overlayStore.clear(target.id) which
removes all overlay fields and can unintentionally clear bookmarks/shares; add a
field-specific clearing method on the overlay store (e.g., clearReactionOverlay
or removeReactionFields) that only clears reactionOverride and
reactionCountOverride (and any reaction-related deltas), then replace calls to
overlayStore.clear(target.id) from reaction failure handlers with this new
method so only reaction overlays are rolled back while
viewerHasBookmarked/viewerHasShared remain intact.
---
Nitpick comments:
In `@app/src/main/graphql/pub/hackers/android/operations.graphql`:
- Around line 542-565: Add explicit selections for the error union variants to
the BookmarkPost and UnbookmarkPost mutations so the client can distinguish
InvalidInputError and NotAuthenticatedError; update the
BookmarkPost/UnbookmarkPost operations (mutations named BookmarkPost and
UnbookmarkPost in app/src/main/graphql/pub/hackers/android/operations.graphql)
to match on the union variants (InvalidInputError and NotAuthenticatedError) and
return their message/code fields, and then propagate those fields through
HackersPubRepository.bookmarkPost and HackersPubRepository.unbookmarkPost so
callers can show specific error messages instead of the generic "Failed to
bookmark"/"Failed to remove bookmark".
In `@app/src/main/java/pub/hackers/android/data/paging/CursorPagingSource.kt`:
- Around line 118-122: The function signature in CursorPagingSource.kt uses a
fully-qualified type for postType; update it to use a short import by adding
import pub.hackers.android.graphql.type.PostType at the top of the file and
change the bookmarksPage parameter from postType:
pub.hackers.android.graphql.type.PostType? to postType: PostType? in the suspend
fun HackersPubRepository.bookmarksPage(...) declaration so it matches the other
adapters' style.
In
`@app/src/main/java/pub/hackers/android/data/repository/HackersPubRepository.kt`:
- Around line 257-288: Add a GraphQL PostType import alias and apply the same
deduplication as other repo methods: import
pub.hackers.android.graphql.type.PostType as GqlPostType and update the
getBookmarks(...) implementation to deduplicate edges before mapping (e.g. call
.distinctBy { it.node.postFields.id } or .distinctBy { it.id } on data?.edges)
so TimelineResult(posts = ...) produces unique posts like
getActorPosts/getPostReplies; keep the rest of the logic (mapping to
TimelineResult and using postFields.toPost(...)) unchanged.
In `@app/src/main/java/pub/hackers/android/ui/screens/explore/ExploreScreen.kt`:
- Around line 219-235: The onBookmarkClick handler in ExploreScreen
(byte-for-byte duplicate of TimelineScreen's handler) shows a success Toast
immediately before viewModel.toggleBookmark completes, so failures never
surface; extract a shared helper (e.g., a composable or extension function) that
takes the Post (use post/sharedPost resolution logic currently in the handler)
and a toggle lambda, replace the inline handlers in both ExploreScreen and
TimelineScreen with this helper, and move success/failure Toast emission into
the ViewModel's result path so viewModel.toggleBookmark publishes success or
error states (use the existing failed_to_bookmark / failed_to_remove_bookmark
strings) which the helper observes to show the correct Toast. Ensure the helper
calls viewModel.toggleBookmark only once and does not optimistic-toast prior to
the repository response.
In
`@app/src/test/java/pub/hackers/android/ui/screens/postdetail/PostDetailContentTest.kt`:
- Line 161: Add a test in PostDetailContentTest that verifies the
onBookmarkClick lambda is invoked: render the PostDetailContent composable with
a side-effecting lambda (e.g., set an AtomicBoolean or mutableState flag) passed
as onBookmarkClick, locate the bookmark button node via its test tag or content
description (e.g., onNodeWithTag("bookmark") or
onNodeWithContentDescription("Bookmark")), call performClick() on that node, and
assert the flag changed to true; reference the onBookmarkClick parameter in the
PostDetailContent invocation and the test node selector you use to wire the
assertion.
🪄 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: b0f5797b-fe77-45de-94c7-e1a9a4b0ee2a
📒 Files selected for processing (21)
app/src/main/graphql/pub/hackers/android/operations.graphqlapp/src/main/graphql/pub/hackers/android/schema.graphqlsapp/src/main/java/pub/hackers/android/data/paging/CursorPagingSource.ktapp/src/main/java/pub/hackers/android/data/paging/PostOverlay.ktapp/src/main/java/pub/hackers/android/data/repository/HackersPubRepository.ktapp/src/main/java/pub/hackers/android/domain/model/Models.ktapp/src/main/java/pub/hackers/android/ui/HackersPubApp.ktapp/src/main/java/pub/hackers/android/ui/components/ArticleCard.ktapp/src/main/java/pub/hackers/android/ui/components/PostCard.ktapp/src/main/java/pub/hackers/android/ui/screens/bookmarks/BookmarksScreen.ktapp/src/main/java/pub/hackers/android/ui/screens/bookmarks/BookmarksViewModel.ktapp/src/main/java/pub/hackers/android/ui/screens/explore/ExploreScreen.ktapp/src/main/java/pub/hackers/android/ui/screens/explore/ExploreViewModel.ktapp/src/main/java/pub/hackers/android/ui/screens/postdetail/PostDetailScreen.ktapp/src/main/java/pub/hackers/android/ui/screens/postdetail/PostDetailViewModel.ktapp/src/main/java/pub/hackers/android/ui/screens/profile/ProfileViewModel.ktapp/src/main/java/pub/hackers/android/ui/screens/settings/SettingsScreen.ktapp/src/main/java/pub/hackers/android/ui/screens/timeline/TimelineScreen.ktapp/src/main/java/pub/hackers/android/ui/screens/timeline/TimelineViewModel.ktapp/src/main/res/values/strings.xmlapp/src/test/java/pub/hackers/android/ui/screens/postdetail/PostDetailContentTest.kt
Comment on lines
+207
to
+221
| onBookmarkClick = { | ||
| val displayPost = post.sharedPost ?: post | ||
| Toast.makeText( | ||
| context, | ||
| context.getString( | ||
| if (displayPost.viewerHasBookmarked) { | ||
| R.string.bookmark_removed | ||
| } else { | ||
| R.string.bookmarked | ||
| } | ||
| ), | ||
| Toast.LENGTH_SHORT | ||
| ).show() | ||
| viewModel.toggleBookmark(post) | ||
| }, |
There was a problem hiding this comment.
Toast fires before the mutation resolves; it can contradict the final state on rollback.
Toast.makeText(...).show() is called synchronously before viewModel.toggleBookmark(post) runs its repository call. If the network mutation fails, BookmarksViewModel.toggleBookmark rolls the overlay back, but the user will have already seen a "Bookmarked" / "Bookmark removed" toast. Because this screen also filters the list on viewerHasBookmarked, a failed unbookmark will briefly remove the row and then restore it, compounding the mismatch.
Consider driving the toast from the ViewModel (e.g., a one-shot event Flow / Channel emitted on success/failure) instead of firing it optimistically from the composable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/pub/hackers/android/ui/screens/bookmarks/BookmarksScreen.kt`
around lines 207 - 221, The Toast is shown optimistically in the composable
(inside onBookmarkClick) before the mutation (viewModel.toggleBookmark)
resolves, causing UI/Toast mismatch on rollback; remove the synchronous
Toast.makeText(...) call from the onBookmarkClick handler and instead emit a
one-shot success/failure event from BookmarksViewModel.toggleBookmark (e.g., a
SharedFlow/Channel/SingleLiveEvent) after the repository call completes or after
rollback, carrying enough info to pick the correct string (use
displayPost.viewerHasBookmarked or the result state). In the composable
subscribe/collect that one-shot event and show the Toast there so the toast
reflects the final resolved outcome.
Comment on lines
+142
to
+152
| viewModelScope.launch { | ||
| val result = if (wasReacted) { | ||
| repository.removeReactionFromPost(target.id, emoji) | ||
| } else { | ||
| repository.addReactionToPost(target.id, emoji) | ||
| } | ||
| result.onFailure { | ||
| overlayStore.clear(target.id) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify PostOverlay field names and the semantics of `clear` vs `mutate` so the fix uses the correct "no override" value.
fd -t f 'PostOverlay*.kt' -x cat {}Repository: hackers-pub/android
Length of output: 4227
🏁 Script executed:
cat -n app/src/main/java/pub/hackers/android/ui/screens/bookmarks/BookmarksViewModel.kt | head -200Repository: hackers-pub/android
Length of output: 8197
overlayStore.clear(target.id) wipes unrelated optimistic state on reaction failure.
On reaction mutation failure, clear(target.id) removes the entire overlay entry for this post — including any optimistic viewerHasBookmarked, viewerHasShared, shareDelta, etc. that the user set independently via toggleBookmark / sharePost and that are still in-flight or already confirmed locally. A failed reaction should only revert reactionOverride / reactionCountOverride.
Unlike toggleBookmark and sharePost / unsharePost, which correctly revert only their specific fields via mutate(...) { prev -> prev.copy(...) }, toggleReaction incorrectly blanks the entire overlay on failure.
🐛 Suggested rollback scoped to reaction fields
result.onFailure {
- overlayStore.clear(target.id)
+ overlayStore.mutate(target.id) { prev ->
+ prev.copy(
+ reactionOverride = null,
+ reactionCountOverride = null,
+ )
+ }
}📝 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.
Suggested change
| viewModelScope.launch { | |
| val result = if (wasReacted) { | |
| repository.removeReactionFromPost(target.id, emoji) | |
| } else { | |
| repository.addReactionToPost(target.id, emoji) | |
| } | |
| result.onFailure { | |
| overlayStore.clear(target.id) | |
| } | |
| } | |
| } | |
| viewModelScope.launch { | |
| val result = if (wasReacted) { | |
| repository.removeReactionFromPost(target.id, emoji) | |
| } else { | |
| repository.addReactionToPost(target.id, emoji) | |
| } | |
| result.onFailure { | |
| overlayStore.mutate(target.id) { prev -> | |
| prev.copy( | |
| reactionOverride = null, | |
| reactionCountOverride = null, | |
| ) | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/pub/hackers/android/ui/screens/bookmarks/BookmarksViewModel.kt`
around lines 142 - 152, The failure handler in toggleReaction currently calls
overlayStore.clear(target.id) which wipes all optimistic state; instead, in
result.onFailure replace that clear call with a scoped rollback using
overlayStore.mutate(target.id) { prev -> prev.copy(...) } so only
reaction-related fields are reverted: set reactionOverride back to null (or to
its previous value) and adjust reactionCountOverride to undo the optimistic
increment/decrement applied when toggling the reaction, leaving other fields
(viewerHasBookmarked, viewerHasShared, shareDelta, etc.) untouched.
Comment on lines
+289
to
+312
| fun toggleBookmark() { | ||
| val post = _uiState.value.post ?: return | ||
| val willBookmark = !post.viewerHasBookmarked | ||
|
|
||
| _uiState.update { | ||
| it.copy(post = post.copy(viewerHasBookmarked = willBookmark)) | ||
| } | ||
|
|
||
| viewModelScope.launch { | ||
| val result = if (willBookmark) { | ||
| repository.bookmarkPost(postId) | ||
| } else { | ||
| repository.unbookmarkPost(postId) | ||
| } | ||
|
|
||
| result.onFailure { | ||
| _uiState.update { state -> | ||
| state.copy( | ||
| post = state.post?.copy(viewerHasBookmarked = !willBookmark) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
No failure feedback on bookmark toggle; inconsistent with other screens.
Two concerns:
-
Silent failure: Unlike
sharePost/unsharePosthere (which only act on success) or the toast in Timeline/Explore screens,toggleBookmarkrolls back the optimistic change on failure but never surfaces an error to the UI. The PR addsR.string.failed_to_bookmark/R.string.failed_to_remove_bookmarkstrings — consider emitting those via a transient error/event state soPostDetailScreencan show a snackbar on failure. -
Reentrancy: There's no guard against rapid re-taps while a mutation is in flight (compare with
isReactingontoggleReaction). Rapid toggles can produce interleaved bookmark/unbookmark calls with out-of-order rollbacks leaving UI state inconsistent with the server. Consider anisBookmarkingflag to no-op during a pending request.
Proposed sketch
fun toggleBookmark() {
+ if (_uiState.value.isBookmarking) return
val post = _uiState.value.post ?: return
val willBookmark = !post.viewerHasBookmarked
_uiState.update {
- it.copy(post = post.copy(viewerHasBookmarked = willBookmark))
+ it.copy(post = post.copy(viewerHasBookmarked = willBookmark), isBookmarking = true)
}
viewModelScope.launch {
val result = if (willBookmark) repository.bookmarkPost(postId)
else repository.unbookmarkPost(postId)
result.onFailure {
_uiState.update { state ->
state.copy(
post = state.post?.copy(viewerHasBookmarked = !willBookmark),
+ isBookmarking = false,
+ bookmarkError = if (willBookmark) R.string.failed_to_bookmark
+ else R.string.failed_to_remove_bookmark,
)
}
- }
+ }.onSuccess {
+ _uiState.update { it.copy(isBookmarking = false) }
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/pub/hackers/android/ui/screens/postdetail/PostDetailViewModel.kt`
around lines 289 - 312, toggleBookmark currently performs an optimistic update
but neither surfaces errors to the UI nor prevents reentrant taps; add an
isBookmarking Boolean in PostDetailViewModel to early-return while a request is
in flight, update it around the network call, and emit a transient error
event/string resource on failure so PostDetailScreen can show a snackbar;
specifically: in toggleBookmark guard with isBookmarking, set isBookmarking =
true before calling repository.bookmarkPost/post.unbookmarkPost and false in
finally, keep the optimistic _uiState update using post.copy(viewerHasBookmarked
= willBookmark), and on result.onFailure emit the appropriate
R.string.failed_to_bookmark or R.string.failed_to_remove_bookmark via the
ViewModel’s event/transient error channel while rolling back the optimistic
change as already implemented.
Comment on lines
+268
to
+282
| onBookmarkClick = { | ||
| val displayPost = post.sharedPost ?: post | ||
| Toast.makeText( | ||
| context, | ||
| context.getString( | ||
| if (displayPost.viewerHasBookmarked) { | ||
| R.string.bookmark_removed | ||
| } else { | ||
| R.string.bookmarked | ||
| } | ||
| ), | ||
| Toast.LENGTH_SHORT | ||
| ).show() | ||
| viewModel.toggleBookmark(post) | ||
| }, |
There was a problem hiding this comment.
Toast is shown before the mutation completes, so failures silently lie to the user.
TimelineViewModel.toggleBookmark optimistically flips viewerHasBookmarked and rolls back on failure, but this toast fires unconditionally based on the pre-toggle state. If repository.bookmarkPost/unbookmarkPost fails, the user has already seen "Bookmarked" / "Bookmark removed" while the UI silently reverts — there's no corresponding failed_to_bookmark / failed_to_remove_bookmark feedback (despite those strings being added in this PR).
Consider either:
- Moving the toast to the ViewModel so it can emit success vs. the
failed_to_*string via a one-shot event/SnackbarHostState, or - Exposing a failure flow the screen can observe and surface the existing
R.string.failed_to_bookmark/R.string.failed_to_remove_bookmarkstrings.
Also note this handler is duplicated verbatim in ExploreScreen.kt (lines 219-235) — worth extracting once the failure path is added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/pub/hackers/android/ui/screens/timeline/TimelineScreen.kt`
around lines 268 - 282, The toast in TimelineScreen's onBookmarkClick handler is
shown before TimelineViewModel.toggleBookmark completes, causing misleading
success messages; move toast/snackbar emission into the ViewModel (or expose a
failure Flow) so success vs failure messages are emitted after
repository.bookmarkPost/unbookmarkPost returns, using a one-shot event (e.g.,
Channel/SharedFlow) or a SnackbarHostState event for R.string.bookmarked,
R.string.bookmark_removed, R.string.failed_to_bookmark, and
R.string.failed_to_remove_bookmark; update TimelineViewModel.toggleBookmark to
emit the appropriate success/failure event after the mutation (rollback still
allowed) and have TimelineScreen observe that event to show the toast/snackbar,
and also extract the shared onBookmarkClick handler into a common function used
by ExploreScreen.kt to avoid duplication.
Add a dedicated bookmark color token to the app theme instead of reusing the accent color. Apply it to card and detail bookmark icons so saved items read as filled yellow or amber across light, dark, and dynamic themes. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Replace the blank bookmarks result with a dedicated empty state that includes an icon and explanatory copy. Adjust the load-state branching so the screen shows the empty state whenever loading has finished with zero bookmarks. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Move bookmark toast strings from LocalContext.getString calls to stringResource in Compose screens. This avoids stale resource reads on configuration changes and clears the Compose lint violations. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Only render bookmark controls when the current surface can actually invoke bookmark actions. This keeps logged-out screens from showing disabled bookmark affordances while preserving the logged-in behavior. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Add bookmark support to the documented feature list so the README matches the current app capabilities. This also calls out the saved-posts view and its article or note filtering behavior. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Introduce a minimal AGENTS guide that points contributors to the project README and enforced conventions. Keep CLAUDE.md as a symlink so both entry points stay aligned without duplicate content. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
Coordinate bookmark toggles per post so overlapping requests do not clobber newer optimistic state. Apply the same mutation policy across timeline, explore, profile, bookmarks, and post detail to keep bookmark behavior consistent. Co-authored-by: Codex <noreply@openai.com> Assisted-By: Codex(GPT-5)
malkoG
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add Android bookmark support for posts and articles, including optimistic toggles across feed/detail surfaces, a dedicated bookmarks screen, and the supporting GraphQL/domain/repository plumbing. Follow-up commits refine the UX with bookmark-state tinting, empty-state handling, logged-out gating, toast fixes, and mutation serialization for more predictable bookmark toggles.
Why
FetchPolicy.NetworkOnlyso stale cached bookmark state does not leave the saved-items screen out of sync right after bookmark toggles.Before and After (if applicable)
Commit History
Test Plan
./gradlew :app:compileDebugKotlin./gradlew :app:testDebugUnitTest --tests pub.hackers.android.ui.components.PostCardTest --tests pub.hackers.android.ui.screens.settings.SettingsScreenTest --tests pub.hackers.android.ui.screens.postdetail.PostDetailContentTestAssisted-By: Codex(GPT-5)