feat(education): process-time user education (#90)#135
Merged
Conversation
Wire the education feature into HomeScreen:
- Provide CoachmarkRegistry via CompositionLocal so the sync card
registers its bounds for the SyncCoachmark overlay
- Anchor the sync progress card with coachmarkAnchor("sync_card")
- Add `?` help icons to the Sync and Activity section headers,
opening the EducationSheet for the matching topic
- Replace the user-facing block-height label in the sync card with
plain-language status (Catching up / Up to date) — power users can
still see the block height on the Node Status screen
- Wire the SyncOptionsSheet `onTopicHelp` callback (both settings and
post-import call sites) to close-and-reopen via the EducationSheet,
with the settings path resuming the picker on Got it
- Plumb onNavigateToFaq through NavGraph -> MainScreen -> HomeScreen
so the EducationSheet's "Open FAQ" button deeplinks to the FAQ
with the topic's anchor pre-expanded
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an education/FAQ system with coachmarks and a sync-options bottom sheet, tracks the first time syncing enters a "catching up" state via a timestamp on SyncProgress, persists coachmark acknowledgement in preferences, and wires FAQ deep-link routing and related UI/components. Changes
Sequence DiagramssequenceDiagram
participant User
participant HomeScreen
participant HomeViewModel
participant WalletPreferences
participant Coachmark as SyncCoachmark
User->>HomeScreen: observe sync progress
HomeScreen->>HomeViewModel: subscribe to combined flows
HomeViewModel->>HomeViewModel: computeFirstCatchingUpAtMs(prev, catching, nowMs)
alt catching detected and grace elapsed
HomeViewModel->>HomeScreen: emit showSyncCoachmark=true
HomeScreen->>Coachmark: show=true (anchorKey)
Coachmark->>HomeScreen: render scrim + tooltip
end
User->>Coachmark: Tap "Got it"
Coachmark->>HomeViewModel: onCoachmarkDismissed()
HomeViewModel->>WalletPreferences: markSyncCoachmarkSeen()
WalletPreferences->>WalletPreferences: persist KEY_SYNC_COACHMARK_SEEN=true
HomeViewModel->>HomeScreen: emit showSyncCoachmark=false
sequenceDiagram
participant User
participant Screen as Home/Settings
participant SyncSheet as SyncOptionsSheet
participant EduSheet as EducationSheet
participant Nav as NavHost
User->>Screen: Open sync options
Screen->>SyncSheet: show=true
User->>SyncSheet: Tap help icon
SyncSheet->>EduSheet: open(topic)
EduSheet->>Screen: onDismiss() (resume flag)
alt user taps "Learn more"
EduSheet->>Nav: navigate to Faq.routeWithAnchor(topic.faqAnchor)
Nav->>Nav: open FaqScreen(anchor)
FaqScreen->>FaqScreen: expand & scroll to anchor
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 49 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.kt (1)
73-77: Derive custom-input visibility fromselectedModeto avoid state drift.Line 77 introduces
showCustomInputas a second state source for what is already represented byselectedMode. Keeping both mutable risks divergence in future edits.♻️ Proposed simplification
- var showCustomInput by remember(initialMode) { mutableStateOf(initialMode == SyncMode.CUSTOM) } @@ - onClick = { selectedMode = SyncMode.NEW_WALLET; showCustomInput = false }, + onClick = { selectedMode = SyncMode.NEW_WALLET }, @@ - onClick = { selectedMode = SyncMode.RECENT; showCustomInput = false }, + onClick = { selectedMode = SyncMode.RECENT }, @@ - onClick = { selectedMode = SyncMode.CUSTOM; showCustomInput = true }, + onClick = { selectedMode = SyncMode.CUSTOM }, @@ - onClick = { selectedMode = SyncMode.FULL_HISTORY; showCustomInput = false }, + onClick = { selectedMode = SyncMode.FULL_HISTORY }, @@ - if (showCustomInput) { + if (selectedMode == SyncMode.CUSTOM) {Also applies to: 108-131, 146-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.kt` around lines 73 - 77, The visibility state showCustomInput must be removed and derived directly from selectedMode to avoid state drift: delete the remember/mutableStateOf declaration for showCustomInput and replace all usages with a derived expression (selectedMode == SyncMode.CUSTOM) inside SyncOptionsSheet (and other occurrences in the 108-131 and 146-154 areas); ensure any code that previously updated showCustomInput now updates selectedMode instead (e.g., when selecting the custom radio button or clearing input), and keep customBlockHeight as its own state but render its input conditional on (selectedMode == SyncMode.CUSTOM).android/app/src/main/java/com/rjnr/pocketnode/ui/navigation/NavGraph.kt (1)
67-68: Apply URI encoding to theanchorparameter.Query parameter values in Jetpack Navigation routes must be URI-encoded to safely handle reserved characters (?, &, #, spaces). While current
faqAnchorvalues are safe, this change defensively protects against future unsafe values and follows official Navigation library conventions.🔧 Proposed fix
object Faq : Screen("faq?anchor={anchor}") { fun routeWithAnchor(anchor: String?): String = - if (anchor.isNullOrBlank()) "faq" else "faq?anchor=$anchor" + if (anchor.isNullOrBlank()) "faq" else "faq?anchor=${android.net.Uri.encode(anchor)}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/navigation/NavGraph.kt` around lines 67 - 68, The routeWithAnchor(anchor: String?) function must URI-encode the anchor value before embedding it into the navigation route to avoid unsafe characters; update routeWithAnchor to return "faq" for null/blank anchors and otherwise return "faq?anchor=" + URLEncoder.encode(anchor, "UTF-8") (or URLEncoder.encode(anchor, StandardCharsets.UTF_8.toString())) and add the necessary import for java.net.URLEncoder (or java.nio.charset.StandardCharsets) so encoding is applied consistently.android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt (1)
207-224: Consider replacingresumeSyncSheetboolean with a typed resume target.Lines 207-224 introduce a second help entry path (
showPostImportSyncDialog), but resume behavior is modeled as a single boolean and only reopensshowSyncOptions(). A small enum/state-sealed target would make resume behavior explicit and future-proof for multiple picker origins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt` around lines 207 - 224, Replace the boolean resumeSyncSheet with a typed resume target (sealed class or enum, e.g., ResumeTarget { ImportPostSync, OtherOrigin }) and thread it through the UI state and viewModel so the origin of the SyncOptionsSheet is explicit; update the code paths that currently toggle resumeSyncSheet and call showSyncOptions() to instead set the resume target (setResumeTarget(ResumeTarget.ImportPostSync) or similar) and call showSyncOptions(target), change viewModel.hidePostImportSyncDialog() usages to clear the resume target via a dedicated viewModel method, and in the SyncOptionsSheet callbacks (onDismiss/onSelectMode) use the resume target to decide the resume action (e.g., call viewModel.changeSyncMode(mode, customBlock, target) or route back to the specific origin) so resume behavior is explicit and future-proof for multiple picker origins (refer to SyncOptionsSheet, showSyncOptions(), viewModel.hidePostImportSyncDialog(), viewModel.changeSyncMode()).android/app/src/test/java/com/rjnr/pocketnode/ui/screens/help/FaqViewModelTest.kt (1)
10-43: Consider adding direct coverage forinitialScrollIndex().This suite covers expansion state well, but it currently doesn’t assert the
initialScrollIndex()behavior for valid/invalid anchors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/test/java/com/rjnr/pocketnode/ui/screens/help/FaqViewModelTest.kt` around lines 10 - 43, Add tests that directly exercise FaqViewModel.initialScrollIndex(): create VMs with a valid anchor (e.g., SavedStateHandle(mapOf("anchor" to "sync_mode"))), a blank anchor, and no anchor, then call initialScrollIndex() and assert that a valid anchor yields a non-negative index while blank or missing anchors yield a negative index (or the sentinel your implementation uses); update FaqViewModelTest to include these assertions alongside the existing expansion-state tests to cover both valid and invalid anchor cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt`:
- Around line 220-223: Reset the process-lifetime tracker firstCatchingUpAtMs
when polling stops to avoid stale timestamps: in the stopSyncPolling()
implementation, set firstCatchingUpAtMs = null (same place where wallet switch
clears it) so when start/restart polling begins the 2s coachmark grace is
honored; ensure the field name firstCatchingUpAtMs and the method
stopSyncPolling() are referenced exactly so the change is applied to the correct
code path.
In
`@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletPreferences.kt`:
- Around line 253-260: The flow can become stale after prefs are cleared because
_hasSeenSyncCoachmark is only initialized once and updated only by
markSyncCoachmarkSeen; register a
SharedPreferences.OnSharedPreferenceChangeListener in this class to keep the
StateFlow in sync: in an init block register a listener that, when
KEY_SYNC_COACHMARK_SEEN changes (or is removed), reads
prefs.getBoolean(KEY_SYNC_COACHMARK_SEEN, false) and sets
_hasSeenSyncCoachmark.value accordingly; ensure the listener references the same
prefs instance and consider unregistering it when the owning object is disposed
(if applicable).
In
`@android/app/src/main/java/com/rjnr/pocketnode/ui/education/coachmark/CoachmarkAnchor.kt`:
- Around line 36-41: The coachmarkAnchor modifier adds bounds to
registry.bounds[key] but never removes them; make the function `@Composable` (if
not already) and add a DisposableEffect tied to the key/registry that clears the
entry on unmount: after computing registry = LocalCoachmarkRegistry.current and
updating registry.bounds in Modifier.onGloballyPositioned, create
DisposableEffect(key, registry) { onDispose { registry.bounds.remove(key) } } so
registry.bounds entries are removed when the anchor composable disposes.
In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/help/FaqScreen.kt`:
- Around line 59-61: The back IconButton currently has Icon(...
contentDescription = null) which makes it inaccessible to screen readers; update
the Icon to use a localized content description by adding a "back" string to
resources (e.g., strings.xml) and set contentDescription =
stringResource(R.string.back) on the Icon (or IconButton) so screen readers
announce it; ensure you import androidx.compose.ui.res.stringResource and
reference the existing
onBack/IconButton/Icon(Icons.AutoMirrored.Filled.ArrowBack) symbols when making
the change.
In
`@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/onboarding/MnemonicImportScreen.kt`:
- Around line 236-244: The onTopicHelp parameter of SyncOptionsSheet is
currently a no-op, so help taps do nothing; wire it to an actual help action by
passing a handler that triggers the existing UI navigation or view model help
action (e.g., call viewModel.openSyncHelp() or viewModel.onHelpRequested("sync")
or navController.navigate("faq/sync") depending on your app navigation setup)
and ensure the viewModel exposes/implements that method (e.g.,
openSyncHelp/onHelpRequested) to show the education/FAQ UI; alternatively, hide
the help affordance in SyncOptionsSheet if no help flow is available.
---
Nitpick comments:
In
`@android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.kt`:
- Around line 73-77: The visibility state showCustomInput must be removed and
derived directly from selectedMode to avoid state drift: delete the
remember/mutableStateOf declaration for showCustomInput and replace all usages
with a derived expression (selectedMode == SyncMode.CUSTOM) inside
SyncOptionsSheet (and other occurrences in the 108-131 and 146-154 areas);
ensure any code that previously updated showCustomInput now updates selectedMode
instead (e.g., when selecting the custom radio button or clearing input), and
keep customBlockHeight as its own state but render its input conditional on
(selectedMode == SyncMode.CUSTOM).
In `@android/app/src/main/java/com/rjnr/pocketnode/ui/navigation/NavGraph.kt`:
- Around line 67-68: The routeWithAnchor(anchor: String?) function must
URI-encode the anchor value before embedding it into the navigation route to
avoid unsafe characters; update routeWithAnchor to return "faq" for null/blank
anchors and otherwise return "faq?anchor=" + URLEncoder.encode(anchor, "UTF-8")
(or URLEncoder.encode(anchor, StandardCharsets.UTF_8.toString())) and add the
necessary import for java.net.URLEncoder (or java.nio.charset.StandardCharsets)
so encoding is applied consistently.
In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt`:
- Around line 207-224: Replace the boolean resumeSyncSheet with a typed resume
target (sealed class or enum, e.g., ResumeTarget { ImportPostSync, OtherOrigin
}) and thread it through the UI state and viewModel so the origin of the
SyncOptionsSheet is explicit; update the code paths that currently toggle
resumeSyncSheet and call showSyncOptions() to instead set the resume target
(setResumeTarget(ResumeTarget.ImportPostSync) or similar) and call
showSyncOptions(target), change viewModel.hidePostImportSyncDialog() usages to
clear the resume target via a dedicated viewModel method, and in the
SyncOptionsSheet callbacks (onDismiss/onSelectMode) use the resume target to
decide the resume action (e.g., call viewModel.changeSyncMode(mode, customBlock,
target) or route back to the specific origin) so resume behavior is explicit and
future-proof for multiple picker origins (refer to SyncOptionsSheet,
showSyncOptions(), viewModel.hidePostImportSyncDialog(),
viewModel.changeSyncMode()).
In
`@android/app/src/test/java/com/rjnr/pocketnode/ui/screens/help/FaqViewModelTest.kt`:
- Around line 10-43: Add tests that directly exercise
FaqViewModel.initialScrollIndex(): create VMs with a valid anchor (e.g.,
SavedStateHandle(mapOf("anchor" to "sync_mode"))), a blank anchor, and no
anchor, then call initialScrollIndex() and assert that a valid anchor yields a
non-negative index while blank or missing anchors yield a negative index (or the
sentinel your implementation uses); update FaqViewModelTest to include these
assertions alongside the existing expansion-state tests to cover both valid and
invalid anchor cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0ddffd1-e680-4ab6-b2e2-e37580c43530
📒 Files selected for processing (25)
android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.ktandroid/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletPreferences.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/MainScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsDialog.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/education/EducationSheet.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/education/EducationTopic.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/education/coachmark/CoachmarkAnchor.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/education/coachmark/SyncCoachmark.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/navigation/NavGraph.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/help/FaqEntry.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/help/FaqScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/help/FaqViewModel.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeViewModel.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/onboarding/MnemonicImportScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/settings/SettingsScreen.ktandroid/app/src/main/res/values/strings.xmlandroid/app/src/test/java/com/rjnr/pocketnode/data/gateway/SyncProgressFirstCatchingUpTest.ktandroid/app/src/test/java/com/rjnr/pocketnode/data/wallet/WalletPreferencesCoachmarkTest.ktandroid/app/src/test/java/com/rjnr/pocketnode/ui/education/EducationTopicFaqLinkTest.ktandroid/app/src/test/java/com/rjnr/pocketnode/ui/education/EducationTopicTest.ktandroid/app/src/test/java/com/rjnr/pocketnode/ui/navigation/FaqRouteTest.ktandroid/app/src/test/java/com/rjnr/pocketnode/ui/screens/help/FaqViewModelTest.ktandroid/app/src/test/java/com/rjnr/pocketnode/ui/screens/home/HomeViewModelCoachmarkTest.kt
💤 Files with no reviewable changes (1)
- android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsDialog.kt
Comment on lines
+253
to
+260
| private val _hasSeenSyncCoachmark = | ||
| MutableStateFlow(prefs.getBoolean(KEY_SYNC_COACHMARK_SEEN, false)) | ||
| val hasSeenSyncCoachmarkFlow: StateFlow<Boolean> = _hasSeenSyncCoachmark.asStateFlow() | ||
|
|
||
| fun markSyncCoachmarkSeen() { | ||
| prefs.edit().putBoolean(KEY_SYNC_COACHMARK_SEEN, true).apply() | ||
| _hasSeenSyncCoachmark.value = true | ||
| } |
Contributor
There was a problem hiding this comment.
Coachmark flow can become stale after preference resets.
hasSeenSyncCoachmarkFlow is a one-time snapshot + manual setter path. If preferences are cleared/reset (clear()), this flow is not synchronized back to false, so UI can read stale state until process restart.
Suggested fix
fun clear() {
prefs.edit().clear().apply()
+ _hasSeenSyncCoachmark.value = false
+ _themeMode.value = readThemeMode()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletPreferences.kt`
around lines 253 - 260, The flow can become stale after prefs are cleared
because _hasSeenSyncCoachmark is only initialized once and updated only by
markSyncCoachmarkSeen; register a
SharedPreferences.OnSharedPreferenceChangeListener in this class to keep the
StateFlow in sync: in an init block register a listener that, when
KEY_SYNC_COACHMARK_SEEN changes (or is removed), reads
prefs.getBoolean(KEY_SYNC_COACHMARK_SEEN, false) and sets
_hasSeenSyncCoachmark.value accordingly; ensure the listener references the same
prefs instance and consider unregistering it when the owning object is disposed
(if applicable).
Owner
Author
Code reviewFound 2 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
stopSyncPolling cancels the polling job that drives the computeFirstCatchingUpAtMs edge-tracker, so the field had no natural way to return to null after sync mode change or network switch. Stale timestamp could immediately satisfy the 2s coachmark grace window on next polling start. Match the reset pattern already used in onActiveWalletChanged: clear the field plus strip it from the last-emitted SyncProgress so HomeViewModel's combine doesn't see stale state during the gap.
Issue #90 specifies block height is hidden on Home and only visible on Node Status. SyncProgressBar honored that, but SyncingChip in the TopAppBar still rendered Block N during every active sync cycle, contradicting the design and the in-file comment one block above. Replace the Block N text with a plain Syncing label.
clear() wiped SharedPreferences but left _hasSeenSyncCoachmark and _themeMode StateFlows holding their pre-clear values, so any collector reading them between clear() and process restart saw stale state. Re-seed both flows to match the post-clear prefs state.
Without DisposableEffect, an anchor that left the composition left its bounds in CoachmarkRegistry forever — a coachmark for that key would aim at coordinates that no longer correspond to a visible component.
Screen readers announced the FAQ back IconButton as unlabeled.
The post-import sync sheet had onTopicHelp wired to a no-op so the help icons rendered but did nothing. Add showHelpIcons param (default true) and pass false from MnemonicImportScreen so the icons are not surfaced where there's no education flow. Re-opening the sheet from an EducationSheet stack would be risky in this constrained context, so suppressing the affordance is preferred over wiring it up.
Contributor
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)
android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt (1)
209-227:⚠️ Potential issue | 🟠 MajorDisable help affordances in the post-import sheet or restore the sheet on dismiss.
This path hides
showPostImportSyncDialogbefore opening education, but unlike the settings flow it never setsresumeSyncSheet, so dismissing the help sheet drops the user out of the required post-import sync selection entirely.Suggested fix
SyncOptionsSheet( currentMode = SyncMode.RECENT, title = "Choose Sync Start Point", description = "Select how far back to sync your imported wallet's history. If your wallet is older than 30 days, choose Custom.", availableModes = listOf(SyncMode.RECENT, SyncMode.CUSTOM), onDismiss = { viewModel.hidePostImportSyncDialog() }, onSelectMode = { mode, customBlock -> viewModel.hidePostImportSyncDialog() if (mode != SyncMode.RECENT) { viewModel.changeSyncMode(mode, customBlock) } }, onTopicHelp = { topic -> viewModel.hidePostImportSyncDialog() educationTopic = topic }, tipBlockNumber = tipBlockNumberLong, - onLookupAddressOnExplorer = onLookupBlockHeight + onLookupAddressOnExplorer = onLookupBlockHeight, + showHelpIcons = false, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt` around lines 209 - 227, The post-import SyncOptionsSheet currently hides the sheet then opens education (viewModel.hidePostImportSyncDialog() and educationTopic = topic) but never restores the sheet on dismiss, causing the flow to drop out; fix by either disabling the help affordance (remove or no-op onTopicHelp) or—preferably—mirror the settings flow by setting the resume flag before opening education (e.g., set resumeSyncSheet = true or call viewModel.setResumeSyncSheet(true)) and then hide the sheet, so when the education UI is dismissed the sheet is restored; ensure onDismiss still calls viewModel.hidePostImportSyncDialog() and that the resume flag is consumed when re-showing SyncOptionsSheet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt`:
- Around line 2477-2495: The race occurs because an in-flight
getAccountStatus().onSuccess lambda can write firstCatchingUpAtMs after
stopSyncPolling() cancels; fix by introducing a polling generation/token: add a
mutable pollingGeneration counter incremented when starting/stopping polling (in
start/stopSyncPolling), capture the current generation in the onSuccess lambda
before calling computeFirstCatchingUpAtMs and _syncProgress.value, and only
update firstCatchingUpAtMs/_syncProgress when the captured generation matches
the repository's current pollingGeneration; reference
computeFirstCatchingUpAtMs, firstCatchingUpAtMs, getAccountStatus().onSuccess,
stopSyncPolling()/cancel(), and the _syncProgress.value assignment so the
in-flight lambda ignores stale emissions.
---
Outside diff comments:
In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt`:
- Around line 209-227: The post-import SyncOptionsSheet currently hides the
sheet then opens education (viewModel.hidePostImportSyncDialog() and
educationTopic = topic) but never restores the sheet on dismiss, causing the
flow to drop out; fix by either disabling the help affordance (remove or no-op
onTopicHelp) or—preferably—mirror the settings flow by setting the resume flag
before opening education (e.g., set resumeSyncSheet = true or call
viewModel.setResumeSyncSheet(true)) and then hide the sheet, so when the
education UI is dismissed the sheet is restored; ensure onDismiss still calls
viewModel.hidePostImportSyncDialog() and that the resume flag is consumed when
re-showing SyncOptionsSheet.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a206192-ae97-41ef-ba31-62ec584d7a9e
📒 Files selected for processing (8)
android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.ktandroid/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletPreferences.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/education/coachmark/CoachmarkAnchor.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/help/FaqScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/onboarding/MnemonicImportScreen.ktandroid/app/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (2)
- android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletPreferences.kt
- android/app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.kt
- android/app/src/main/java/com/rjnr/pocketnode/ui/screens/help/FaqScreen.kt
- android/app/src/main/java/com/rjnr/pocketnode/ui/screens/onboarding/MnemonicImportScreen.kt
- android/app/src/main/java/com/rjnr/pocketnode/ui/education/coachmark/CoachmarkAnchor.kt
Coroutine cancellation is cooperative — getAccountStatus() can have already returned a successful Result before stopSyncPolling() calls cancel(), in which case the .onSuccess lambda still runs to completion and resurrects firstCatchingUpAtMs / _syncProgress after stopSyncPolling cleared them. Add @volatile syncPollingGeneration. Each start captures the current generation; stopSyncPolling bumps the counter before cancel(); the onSuccess lambda's first action is a generation check that returns early on mismatch. Closes the race window flagged by code review on the prior fix.
Replace the 4-rect Canvas scrim with a single Path using Difference op so the spotlight has rounded corners matching the sync card. Apply explicit ButtonDefaults colors and a fontWeight on Got it so it renders visibly against the card background. Reported in manual smoke.
Manual smoke wanted a sense of progress on the sync card. Replace plain Catching up... with the dynamic "Catching up from <current> to <tip>", with the values rendered in SemiBold via AnnotatedString. Block numbers formatted with locale-aware thousands separators.
…nge (#90) The outer try/catch in updateStatus swallowed JNI exceptions (common during sync-mode restart) and short-circuited before _uiState.update. Old tipHeader / peers / scripts stuck on stale values until the user backed out and reopened Node Status. Wrap each repository call individually in runCatching with safe defaults so the state update always runs and reflects the current JNI state — including a deliberate null tipHeader during the brief restart window.
When the user tapped "Look up on explorer" from the CUSTOM block-height field, Chrome opened and the OEM memory manager + auth gate killed the process. On return, PIN unlock dropped them on Home with the dialog gone — they had to reopen and retype. Persist showSyncOptionsDialog into HomeViewModel.SavedStateHandle so the dialog re-opens on ViewModel re-creation. Switch the sheet's typed customBlockHeight / selectedMode / showCustomInput to rememberSaveable so partial input survives process death too.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.kt`:
- Around line 67-68: SyncOptionsSheet's parameter tipBlockNumber was changed
from String/Int to Long; update every call site (notably the SettingsScreen call
that currently passes uiState.tipBlockNumber) to supply a Long. In
SettingsScreen (and any other callers), convert the string/nullable value the
same way HomeScreen does: use toLongOrNull() ?: 0L (or otherwise coerce/parse to
a Long) and pass that result into SyncOptionsSheet; ensure the parameter name
tipBlockNumber is used so the correct overload is selected.
In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt`:
- Around line 214-215: The two hardcoded UI strings in HomeScreen.kt (the
sheet's title and description currently set to "Choose Sync Start Point" and the
longer sentence) should be moved into strings.xml and referenced from the
Compose UI using the resource API; add two entries like
choose_sync_start_point_title and choose_sync_start_point_description to
strings.xml, then replace the literal title and description in the HomeScreen's
sheet call with stringResource(R.string.choose_sync_start_point_title) and
stringResource(R.string.choose_sync_start_point_description) (or
context.getString(...) if not using Compose's stringResource) so the sheet copy
is localizable and consistent with other resource-backed text.
- Around line 224-227: The onTopicHelp handler currently closes the post-import
flow (calls viewModel.hidePostImportSyncDialog()) and sets educationTopic, but
there is no logic to reopen the post-import sheet (showPostImportSyncDialog) so
users cannot return to choose CUSTOM; update the handler in HomeScreen.kt (the
onTopicHelp lambda) to either: (A) record a resume flag (e.g., set a boolean
like resumePostImportAfterHelp in the ViewModel) and reopen the post-import
sheet when the help dialog is dismissed (or when educationTopic is cleared), or
(B) suppress/disable the help icon in the post-import/ CUSTOM variant so
onTopicHelp cannot be triggered there; implement the chosen path by modifying
viewModel.hidePostImportSyncDialog() calls and the ViewModel state to support
resumePostImportAfterHelp and reopening showPostImportSyncDialog, or by gating
the help UI when the post-import variant is active.
In
`@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeViewModel.kt`:
- Around line 76-97: The code only persists showSyncOptionsDialog across process
death; add persistent save/restore for showPostImportSyncDialog as well by
introducing a new key (e.g. SAVED_KEY_SHOW_POST_IMPORT_SYNC), initialize
HomeUiState's showPostImportSyncDialog from
savedStateHandle.get<Boolean>(SAVED_KEY_SHOW_POST_IMPORT_SYNC) ?: false, and
update any places that toggle or set showPostImportSyncDialog to also write the
boolean into savedStateHandle.put(SAVED_KEY_SHOW_POST_IMPORT_SYNC, value) so the
post-import SyncOptionsSheet is restored the same way showSyncOptionsDialog is.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4345e0a0-4c44-438f-9f6e-b11977372287
📒 Files selected for processing (7)
android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/education/coachmark/SyncCoachmark.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeViewModel.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/status/NodeStatusViewModel.ktandroid/app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/res/values/strings.xml
- android/app/src/main/java/com/rjnr/pocketnode/ui/education/coachmark/SyncCoachmark.kt
- android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt
Comment on lines
+67
to
+68
| savedCustomBlockHeight: Long? = null, | ||
| tipBlockNumber: Long = 0L, |
Contributor
There was a problem hiding this comment.
Update all SyncOptionsSheet call sites to pass a Long.
tipBlockNumber is now a Long, but android/app/src/main/java/com/rjnr/pocketnode/ui/screens/settings/SettingsScreen.kt:191-205 still passes uiState.tipBlockNumber directly. With the current API, that screen will stop compiling unless it does the same toLongOrNull() ?: 0L conversion as HomeScreen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsSheet.kt`
around lines 67 - 68, SyncOptionsSheet's parameter tipBlockNumber was changed
from String/Int to Long; update every call site (notably the SettingsScreen call
that currently passes uiState.tipBlockNumber) to supply a Long. In
SettingsScreen (and any other callers), convert the string/nullable value the
same way HomeScreen does: use toLongOrNull() ?: 0L (or otherwise coerce/parse to
a Long) and pass that result into SyncOptionsSheet; ensure the parameter name
tipBlockNumber is used so the correct overload is selected.
Comment on lines
214
to
215
| title = "Choose Sync Start Point", | ||
| description = "Select how far back to sync your imported wallet's history. If your wallet is older than 30 days, choose Custom.", |
Contributor
There was a problem hiding this comment.
Move the new post-import sheet copy into strings.xml.
These are new user-facing strings in the education flow, but they're still hardcoded here. That leaves this path non-localizable and inconsistent with the rest of the sheet copy, which is resource-backed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/home/HomeScreen.kt`
around lines 214 - 215, The two hardcoded UI strings in HomeScreen.kt (the
sheet's title and description currently set to "Choose Sync Start Point" and the
longer sentence) should be moved into strings.xml and referenced from the
Compose UI using the resource API; add two entries like
choose_sync_start_point_title and choose_sync_start_point_description to
strings.xml, then replace the literal title and description in the HomeScreen's
sheet call with stringResource(R.string.choose_sync_start_point_title) and
stringResource(R.string.choose_sync_start_point_description) (or
context.getString(...) if not using Compose's stringResource) so the sheet copy
is localizable and consistent with other resource-backed text.
The smoke fix landed save/restore for showSyncOptionsDialog, but the user's actual flow was the post-import sheet (showPostImportSyncDialog) — it shows right after wallet import to ask which start point to sync from. That sheet ALSO surfaces the explorer-lookup link, so it has the same Chrome → process death → return failure mode, and the previous fix didn't cover it. Adds: - SAVED_KEY_SHOW_POST_IMPORT_SYNC + restore on init. - savedStateHandle write on every showPostImportSyncDialog mutation (hidePostImportSyncDialog, showPostImportSyncDialog, importWallet success path). - resumePostImportSheet flag in HomeScreen so EducationSheet dismiss reopens the right sheet (post-import vs. settings).
Adds home_post_import_sync_title and home_post_import_sync_description so the post-import flow is localizable alongside the rest of the sync sheet copy. Per CodeRabbit review on PR #135.
3 tasks
RaheemJnr
added a commit
that referenced
this pull request
Apr 29, 2026
* chore(release): bump to 1.6.0 (versionCode 7) Bumps the app version for the user-education release covering PRs #134 and #135, which closed #90. Adds the F-Droid changelog entry for versionCode 7. * chore(release): retarget version bump to 1.5.1 More changes are queued before 1.6.0; this release ships the user-education work as a patch instead.
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
Closes #90. Ships the user-education feature (process-time confusions only — sync, sync mode, block height, balance delay, "Pending", confirmations).
What's in this PR:
WalletPreferences.hasSeenSyncCoachmark). 2-second grace prevents flashing forNEW_WALLETmode that completes in milliseconds.?bottom-sheet help — section?on Home Sync + Activity sections; inline?per option in the sync mode picker; inline?on the CUSTOM block-height field.SyncOptionsDialog → SyncOptionsSheet— AlertDialog → ModalBottomSheet. New "Pick this if…" copy per mode. RECENT now carries a "Recommended" badge.?bottom sheets.strings.xml-backed (single English locale; non-English locales tracked in i18n: add translations for non-English locales (Yoruba, Hausa, Pidgin English, Mandarin, Spanish, etc.) #132).Setup-time confusions (mnemonic flow refresh, "what if I lose my phone") are intentionally deferred to a separate brainstorm + PR.
Architecture highlights
ui/education/EducationTopic.kt—enum class(Saver-compatible withrememberSaveable) carryingtitleRes,bodyRes,faqAnchordirectly. Single source of truth.ui/education/EducationSheet.kt— stateless ModalBottomSheet; caller controls visibility via the topic param.ui/education/coachmark/—Modifier.coachmarkAnchor(key)records bounds into aCompositionLocal-scoped registry;SyncCoachmarkoverlay reads them and draws a 4-rect Canvas scrim with tap-swallow.HomeViewModel.shouldShowSyncCoachmark(seen, isCatchingUp, firstCatchingUpAtMs, nowMs)— pure helper, deterministically testable. Driven by a 1-second clock tick combined with prefs flag + sync progress.SyncProgress.firstCatchingUpAtMs— process-lifetime wall-clock timestamp captured the first timeisCatchingUpflips true. Reset on wallet switch.SyncOptionsSheet↔EducationSheet— picker closes, education sheet opens, picker reopens with previous selection preserved on dismiss.rememberSaveablesurvives rotation.Test plan
cd android && ./gradlew testDebugUnitTest— 548/548 passing. New tests added:WalletPreferencesCoachmarkTest(2 tests)SyncProgressFirstCatchingUpTest(5 tests on the pure edge-trigger helper)EducationTopicTest(3 tests — non-zero res ids + uniqueness)EducationTopicFaqLinkTest(2 tests — every topic anchor maps to a real FaqEntry; FAQ anchors are unique)HomeViewModelCoachmarkTest(6 tests on the pure derivation, including clock-skew defensive case)FaqRouteTest(3 tests — null/blank/populated anchor)FaqViewModelTest(5 tests — initial state, blank handling, toggle, preserve-others)cd android && ./gradlew assembleDebug— BUILD SUCCESSFUL.cd android && ./gradlew lintDebug— BUILD SUCCESSFUL, no new warnings introduced.?on Home Sync → education sheet opens; "Open FAQ" deep-links tosyncentry expanded + scrolled into view.?on Home Activity → same flow withactivityentry.?on each option → picker closes, education sheet opens; dismiss → picker reopens with the previous selection.rememberSaveableround-trip).Commit structure
15 conventional commits on this branch — one per task in the implementation plan:
c72bb36feat(prefs): hasSeenSyncCoachmark flag95c5161feat(sync): expose firstCatchingUpAtMs4467d1d+b75cbd8feat(education): EducationTopic+ collapse-into-enum refactor50707f0feat(faq): FaqEntry data + round-trip test71b5b00+77adbd4feat(education): EducationSheet+ width-cap refactor2176ff4feat(coachmark): Modifier.coachmarkAnchor + registryd7f7a69feat(coachmark): SyncCoachmark scrim + tooltipeee5511feat(home): showSyncCoachmark derivationf45053dfeat(sync): SyncOptionsSheet (ModalBottomSheet) with inline ? helpabd1f80feat(nav): Screen.Faq route6f4c201feat(faq): FaqScreen accordion + FaqViewModelf47eadafeat(home): integrate coachmark + ? help + education sheete4f1316feat(settings): Help & FAQ row + EducationSheet integrationRelated
Spec / plan
docs/superpowers/specs/2026-04-28-user-education-process-time-design.mddocs/superpowers/plans/2026-04-28-user-education-process-time.mdSummary by CodeRabbit
New Features
Bug Fixes
Tests