feat: add deep links to every screen - #1119
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Sheet.Hardware(HardwareRoute.Searching), | ||
| Sheet.Hardware(HardwareRoute.Paired), |
There was a problem hiding this comment.
When hardware/searching or hardware/paired is opened directly, the sheet bypasses the intro action that starts discovery and establishes the paired-device state, causing an indefinitely waiting search screen or a paired screen backed by default state.
| Sheet.Hardware(HardwareRoute.Searching), | |
| Sheet.Hardware(HardwareRoute.Paired), |
Knowledge Base Used: UI Screens: Navigation, Screens, and Components
There was a problem hiding this comment.
Confirmed both. Discovery is started by the intro's continue action rather than by the screen, so a deep-linked hardware/searching spins forever, and hardware/paired renders PAIRED TREZOR over default state with no device behind it.
Same class as send/confirm, which this PR already excluded for offering Swipe To Pay over a payment that was never built. My registry was inconsistent in keeping these two.
Removed both. hardware now exposes intro only.
Resolved in 511250c90.
|
|
||
| Sheet.Backup(BackupRoute.Intro), | ||
| Sheet.Backup(BackupRoute.Warning), | ||
| Sheet.Backup(BackupRoute.Success), |
There was a problem hiding this comment.
When backup/success is opened directly, the sheet starts at BackupRoute.Success without running or verifying a backup, causing the UI to report success for an action that never occurred.
| Sheet.Backup(BackupRoute.Success), |
Knowledge Base Used: UI Screens: Navigation, Screens, and Components
There was a problem hiding this comment.
Correct, and the write is worse than the render. onSuccessContinue persists backupVerified = true (BackupNavSheetViewModel.kt:139-141), so the OK button on a deep-linked success screen would mark a wallet backed up without the recovery phrase ever being shown. That breaks this PR's own rule that links never change a setting without the usual confirmation.
Removed backup/success, and also backup/warning, which is one tap upstream of the same write. backup now exposes intro, multiple-devices and metadata only.
Resolved in 511250c90.
Greptile SummaryAdds development-mode deep links for root navigation destinations and manually selected sheet routes.
Confidence Score: 4/5The backup-success and hardware mid-flow links should be removed or made to establish their required state before this PR is merged. Direct sheet entry can report a backup that never occurred or render hardware searching and paired states without starting discovery or establishing a paired device. Files Needing Attention: app/src/main/java/to/bitkit/ui/utils/SheetDeepLinks.kt
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/ui/utils/SheetDeepLinks.kt | Registers direct sheet entry points, including backup and hardware flow states that require work or initialization performed by earlier steps. |
| app/src/main/java/to/bitkit/ui/utils/ScreenDeepLinks.kt | Derives root-route deep links and excludes an explicit set of sensitive routes. |
| app/src/main/java/to/bitkit/ui/ContentView.kt | Replays queued links through either the sheet host or root NavController. |
| app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt | Gates screen links on development mode and stores one pending URI for UI consumption. |
| app/src/main/java/to/bitkit/ui/utils/Transitions.kt | Makes generated screen links the default for root composable and nested-graph registrations. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["bitkit://screen/... intent"] --> B["AppViewModel checks dev mode"]
B -->|disabled| C["Ignore link"]
B -->|enabled| D["pendingScreenDeepLink"]
D --> E{"SheetDeepLinks match?"}
E -->|yes| F["showSheet with nested start route"]
E -->|no| G["NavController.handleDeepLink"]
Reviews (1): Last reviewed commit: "feat: add deep links to bottom sheet scr..." | Re-trigger Greptile
There was a problem hiding this comment.
Thanks for addressing the existing backup and hardware findings; I confirmed both fixes on 511250c.
I found three blocking issues: cold-start links bypass the dev-mode gate, bitkit://screen/recovery-mode is consumed by the legacy parser before the gate, and external-confirm can reach invalid fresh state and crash. I also left two non-blocking corrections for Home registration and the adb -W guidance.
| return | ||
| } | ||
|
|
||
| intent.data?.let { if (ScreenDeepLinks.isScreenDeepLink(it)) intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TASK) } |
There was a problem hiding this comment.
I found that cold launches bypass the dev-mode gate. AndroidX automatically handles the Activity's data intent when this Activity-backed NavHost sets its graph; this line retains the screen URI, while AppViewModel gates only the later manual replay. A valid root link can therefore navigate with dev mode off. Please clear the screen URI from the Activity intent before graph creation and replay it only through the gated pipeline, with cold-start coverage for dev mode off and on.
There was a problem hiding this comment.
Good catch. Cold start went through AndroidX automatic intent handling rather than the gated replay, so links still navigated with dev mode off. The data is now cleared from the activity intent once the gate has read it, which also made the CLEAR_TASK flag unnecessary.
Resolved in 09239fb
|
|
||
| private val CAMEL_HUMP = Regex("(?<=[a-z0-9])(?=[A-Z])") | ||
|
|
||
| private val DENIED: Set<KClass<out Routes>> = setOf( |
There was a problem hiding this comment.
I found that bitkit://screen/external-confirm creates a fresh ExternalNav-scoped view model with peer = null. Its enabled swipe reaches onConfirm(), which calls requireNotNull(peer) and crashes; external-amount can advance into the same state. Please deny these cold-start routes or restore and validate the required peer state before enabling confirmation, and cover both URIs from fresh state.
There was a problem hiding this comment.
Denied both, and external-success too, since it reports a completed channel open over the same empty state. Restoring the peer instead would mean re-running the connect flow validation from a URI, which belongs with that flow.
Resolved in 09239fb
| inline fun <reified T : Any> NavGraphBuilder.composableWithDefaultTransitions( | ||
| typeMap: Map<KType, NavType<*>> = emptyMap(), | ||
| deepLinks: List<NavDeepLink> = emptyList(), | ||
| deepLinks: List<NavDeepLink> = ScreenDeepLinks.linksFor(T::class), |
There was a problem hiding this comment.
I found that bitkit://screen/home is generated but never attached to the graph. Routes.Home still uses raw composable<Routes.Home> in ContentView, so this default is skipped and the URI is unhandled from another screen. Please attach the link explicitly and test matching against the assembled graph.
There was a problem hiding this comment.
Right, Home is the last destination still on raw composable, so it never picked up the default and nothing was attached to the graph. Passed the links explicitly at that call site and confirmed it resolves from another screen now.
Resolved in 09239fb
| -d "bitkit://screen/settings" to.bitkit.dev | ||
| ``` | ||
|
|
||
| Escape `&` as `\&` when passing more than one query parameter. `-W` reports whether the intent |
There was a problem hiding this comment.
I found that -W verifies only Activity resolution, not the screen ID. The manifest accepts every bitkit: URI by scheme, so bitkit://screen/typo still resolves MainActivity; the in-app handler rejects it later. Please document a destination or logcat assertion instead of treating -W as route validation.
There was a problem hiding this comment.
You are right, -W only proves an activity resolved. The doc and the first QA note now assert on the Unhandled screen deeplink warning in logcat instead.
Resolved in 09239fb
| return@launch | ||
| } | ||
|
|
||
| if (ScreenDeepLinks.isScreenDeepLink(uri)) { |
There was a problem hiding this comment.
I found that bitkit://screen/recovery-mode never reaches this deny gate. isRecoveryModeDeeplink() runs first and accepts any URI whose only path segment is recovery-mode, so this URL calls setRecoveryMode(true) and navigates before the dev-mode check. Please classify the screen namespace before the legacy recovery parser, or restrict that parser to the exact legacy URI, and add this denied URL to AppViewModel coverage.
There was a problem hiding this comment.
Agreed. The legacy parser matched on the path segment alone, so it ran first and switched recovery mode on. The screen namespace is now classified ahead of it and that URL falls through to the deny list, with the legacy one unchanged.
Resolved in 09239fb
2 participants
Fixes #659
Refs #1118
This PR adds a
bitkit://screen/...URI for every screen in the app, bottom sheets included, gated on dev mode.Description
Screens in the root graph register themselves.
ScreenDeepLinksderives the id from the route type by kebab-casing its class name andcomposableWithDefaultTransitionsattaches the link, soRoutes.RgsServeris reachable atbitkit://screen/rgs-serverand a new screen works as soon as it joins the graph. Arguments without a default become path segments, arguments with a default become query parameters.Bottom sheets needed a second mechanism. They are not in the root graph: they are
Sheetvalues rendered bySheetHost, each carrying the start route of its own nestedNavHost, soNavController.handleDeepLinkcannot reach them.SheetDeepLinksmaps a path to aSheetandContentViewhands it toshowSheetbefore falling through to the nav graph. Paths are still derived from class names via the sharedkebabIdhelper, sobitkit://screen/widgets/price-editcomes fromSheet.WidgetsplusWidgetsRoute.PriceEdit. A bare sheet id opens its first registered route.send,receive,backup,widgets,hardwareand the three sheets with no nested graph.isDevModeEnabledinAppViewModel.handleDeeplinkIntent, and holds one until the wallet is loaded;ContentViewreplays it.FLAG_ACTIVITY_CLEAR_TASKto screen links inMainActivity. TheNavHostre-handles the intent when it sets its graph, and without the flag that restarts the whole task instead of navigating, costing an extra activity launch on every cold link.RecoveryMnemonic,AuthCheck,LegacyRnRecovery,LnurlChannel,CriticalUpdate,RecoveryModeand the threeExternal*transfer routes, thebackup/show-mnemonicandshow-passphrasesheet routes, thepin/change-pin/disable-pinsheets, andforce-transfer. Links navigate and prefill only, and never send, broadcast or change a setting without the usual confirmation.bitcoin:,lightning:,lnurl*) on the scanner decode path, untouched.docs/deeplinks.md.FeeNavsub-graph, and letting the confirm and liquidity screens tolerate an empty flow, are follow-ups.Sheet routes are registered by hand because not every route is a valid start destination. I fired all 51 against an emulator, then re-ran each rejected one in isolation against logcat. Thirteen are excluded, for three reasons:
SendRoute.FeeRateandFeeCustomsit insidenavigationWithDefaultTransitions<SendRoute.FeeNav>. A nested graph's child cannot be aNavHoststart destination, so both throwIllegalStateException: Cannot find startDestination ... from NavGraph.send/fee-navresolves but renders only the screen title.send/quick-paythrows onrequireNotNull(quickPayData)(SendSheet.kt:309).send/confirmand the four receive confirm/liquidity routes do not crash but render empty or zero-amount screens.send/confirmoffers "Swipe To Pay" over a payment that was never built.backup/successreports a backup that never ran, and its OK button persistsbackupVerified = true(BackupNavSheetViewModel.onSuccessContinue).backup/warningis one tap upstream of the same write.hardware/searchingwaits forever because discovery starts from the intro's continue action, andhardware/pairedclaims a paired device over default state.A test pins all thirteen, and a final sweep confirmed they no-op with the wallet overview intact and zero fatal exceptions.
Preview
N/A
QA Notes
Dev mode is on by default on debug builds (Settings ▸ Advanced ▸ Dev Settings). The app must be past onboarding.
Manual Tests
adb shell am start -a android.intent.action.VIEW -d "bitkit://screen/settings" to.bitkit.dev→ Settings opens. A mistyped id still resolvesMainActivity, because the manifest accepts everybitkit:URI by scheme, so check logcat forUnhandled screen deeplinkrather than relying on-W.bitkit://screen/send→ Send sheet on the recipient picker.bitkit://screen/widgets/price-edit→ Bitcoin Price editor.bitkit://screen/recovery-mnemonicandbitkit://screen/backup/show-mnemonic→ screen unchanged, recovery phrase never shown, logcat carriesUnhandled screen deeplink.bitkit://screen/send/fee-rateandbitkit://screen/backup/success→ screen unchanged, no crash, andbackupVerifiedis untouched.bitkit://screen/...link is ignored, on warm start and on cold start. Settings ▸ Support, tap Version five times to toggle.regression:scan abitcoin:/lightning:/lnurlURI → still decodes through the scanner path.Automated Checks
ScreenDeepLinksTest.kt: id derivation, path vs query argument placement, and that every denied route yields no link.SheetDeepLinksTest.kt: bare-id defaults, sub-route selection, case-insensitive lookup, and that sensitive and mid-flow paths resolve to null rather than falling back to the sheet default.journeys/deeplinks/.just compile,just test,just lintall pass, no new detekt findings.