feat: add deep links to every screen - #1119
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
There was a problem hiding this comment.
Thanks for addressing the earlier findings. Three blocking issues remain:
- Cold-start coverage is missing at the Activity/
NavHostboundary. - Late transfer destinations allow entry without the flow state they require.
- Root-screen links route behind an active sheet.
ovitrif
left a comment
There was a problem hiding this comment.
One blocking navigation regression remains: rejected screen links dismiss an active sheet before route handling determines that the URI is invalid.
There was a problem hiding this comment.
The deep-link implementation currently splits navigation policy across the route models and parallel registries, so I came up with 4 required corrections to keep direct entry safe and make the route hierarchy the single source of truth, and fix the changelog:
- A denied root-screen link dismisses an active sheet before rejection, so rejected input changes the visible flow.
- Root-screen eligibility has a second owner in
ScreenDeepLinks.DENIED, so new routes become reachable without an explicit decision and every route change must synchronize a parallel policy. SheetDeepLinks.SHEETScorrectly fails closed while duplicating which nested states may start a flow outside the sealed route families that define them.- The changelog is incorrect because it describes an internal QA workflow rather than the public developer capability available to people running their own builds.
|
|
||
| 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.
ScreenDeepLinks becomes a second owner of the route model because DENIED duplicates direct-entry eligibility outside the sealed Routes hierarchy. Every future destination change therefore needs to synchronize the route hierarchy, navigation graph, this registry, its tests, the documentation, and the journeys.
That duplication does not guard future changes:
- A new route becomes externally reachable automatically because
composableWithDefaultTransitionscallslinksFor(T::class)by default. - A route that should remain internal can remain externally reachable because its author must also know to add it to
DENIED. - The every-route test does not enforce an explicit eligibility decision because every unlisted route is accepted as deep-linkable.
The navigation model should follow the same ownership principle we use from DDD: behavior and invariants live with the model that owns them. Within this navigation boundary, the sealed Routes hierarchy should own whether a destination may be entered directly, while ScreenDeepLinks only adapts external URIs.
The sealed hierarchy should make that choice explicit:
sealed interface Routes {
sealed interface DeepLinkable : Routes
sealed interface InternalOnly : Routes
@Serializable
data object Settings : DeepLinkable
@Serializable
data object SpendingConfirm : InternalOnly
}The graph helper should then accept only Routes.DeepLinkable, making every new destination declare its policy in the same model that defines it. Could we move root-screen eligibility into the sealed Routes hierarchy and constrain the graph helper to Routes.DeepLinkable, leaving ScreenDeepLinks responsible only for URI adaptation?
There was a problem hiding this comment.
Moved eligibility into the sealed hierarchy, so DENIED is gone.
I could not constrain the shared helper: it also registers the sheet and startup families, so 73 of its 171 call sites are not Routes, and a same-name overload clashes. Root screens use a separate deepLinkableComposable.
Resolved in c4f4bca
There was a problem hiding this comment.
Confirmed fixed on c755819: DENIED is gone and root-screen eligibility now lives on Routes.DeepLinkable / Routes.InternalOnly with deepLinkableComposable registration, though ExternalConnection still needs that wiring.
| import to.bitkit.ui.sheets.hardware.HardwareRoute | ||
|
|
||
| object SheetDeepLinks { | ||
| private val SHEETS: List<Sheet> = listOf( |
There was a problem hiding this comment.
SheetDeepLinks correctly fails closed because SHEETS is a positive allowlist. The maintenance defect is that this object becomes a second catalog of which nested states may safely start a flow, separate from the sealed SendRoute, ReceiveRoute, BackupRoute, WidgetsRoute, and HardwareRoute families that define those states.
Future flow changes therefore need synchronized maintenance:
- A startable state needs to be declared in its route family and registered in
SHEETS. - Reclassifying a flow-only state therefore changes this catalog and its parallel tests instead of one route contract.
- The route declaration does not communicate whether the state is a valid independent entry point.
The nested navigation models should follow the same DDD ownership principle: each sealed route family owns the invariant that determines whether a state can start independently, while SheetDeepLinks only adapts the external URI.
Each sealed route family should make that choice explicit and own its lookup:
sealed interface SendRoute {
sealed interface DeepLinkStart : SendRoute
sealed interface InternalOnly : SendRoute
@Serializable
data object Recipient : DeepLinkStart
@Serializable
data object Confirm : InternalOnly
companion object {
fun fromDeepLink(path: String): DeepLinkStart? = TODO()
}
}SheetDeepLinks should keep the standalone Sheet entries it directly owns. For nested flows, it should select the sheet family, delegate the child path to that family’s route-owned lookup, and wrap the eligible state in the corresponding Sheet. Could we move nested sheet-entry eligibility and lookup into the existing sealed route families, leaving SheetDeepLinks as the family and standalone URI adapter?
There was a problem hiding this comment.
Moved this into the route families too. Each now declares DeepLinkStart or InternalOnly and owns its fromDeepLink lookup, so SheetDeepLinks only picks the family and wraps the result. A state marked as a start but missing from its lookup now fails a test.
Resolved in 0e0a16b
There was a problem hiding this comment.
Confirmed fixed on c755819: sheet families now own DeepLinkStart / fromDeepLink, and SheetDeepLinks only selects the family and wraps the result.
# Conflicts: # app/src/main/java/to/bitkit/ui/ContentView.kt
|
The red e2e-status is not from this branch. e2e-tests-staging - pubky_paykit fails the same four contact specs on ContactViewName here and on #1123, which touches only channel-details time formatting. My #1111 passed that job on 29 Jul and ContactDetailScreen has not changed since May, so the break is on master or in the staging env rather than in either PR. The rest is green: build, build-local, build-staging, detekt, and both local e2e specs. |
There was a problem hiding this comment.
Thanks for addressing the earlier findings. One blocking issue and one docs cleanup remain:
ExternalConnectionis stillRoutes.DeepLinkablebut registered withcomposableWithDefaultTransitions, sobitkit://screen/external-connectionlost its Nav deep link after that helper stopped auto-registering links.docs/deeplinks.mdno longer matches the fail-closed route-owned model and should be removed rather than kept as a second source of truth for this developer-oriented feature.
PS. We can ignore the red e2e failures, it's caused by a premature merge of a PR in the e2e repo targeting paykit-related PRs that are not yet into master. Classic PR stacks issues 🙃 .
|
|
||
| @Serializable | ||
| data class ExternalConnection(val scannedNodeUri: String? = null) : Routes | ||
| data class ExternalConnection(val scannedNodeUri: String? = null) : Routes.DeepLinkable |
There was a problem hiding this comment.
ExternalConnection is marked Routes.DeepLinkable, but its NavHost registration still uses composableWithDefaultTransitions, which no longer attaches screen deep links by default. Before the ownership migration this URI was reachable; now bitkit://screen/external-connection is unhandled even though the route model says it may be entered directly. Could we register it with deepLinkableComposable like the other DeepLinkable leaves under nested graphs?
There was a problem hiding this comment.
Good catch, that one was imported bare as ExternalConnection so my sweep to the new helper missed it. Registered with deepLinkableComposable now and the URI opens Manual Setup again on device. The other 15 non-linking registrations are exactly the InternalOnly routes.
Resolved in ce7f8a8
| name, so the same rule holds as for screens. The bare id opens the sheet at its first registered | ||
| route (`bitkit://screen/send` is the recipient picker, `bitkit://screen/backup` the backup intro). | ||
|
|
||
| Each sealed route family owns which of its states may start a flow, through a `DeepLinkStart` marker |
There was a problem hiding this comment.
This ownership note is right, but the rest of docs/deeplinks.md still contradicts the fail-closed model (auto-registration via composableWithDefaultTransitions, hand-maintained SheetDeepLinks entries). For this developer-oriented feature the durable source of truth is the route markers, deepLinkableComposable, sheet-family DeepLinkStart / fromDeepLink, and the unit tests, so a long parallel doc will keep drifting. Could we delete docs/deeplinks.md (or shrink it to a short pointer to those code/tests) instead of maintaining it as a second catalog?
There was a problem hiding this comment.
Deleted it. The markers, deepLinkableComposable, the family lookups and the unit tests are the source of truth, and that doc drifted three times during this review alone. The journeys stay since they are executable.
Resolved in ce7f8a8
Fixes #659
Refs #1118
Refs #1126
This PR adds a
bitkit://screen/...URI for every screen in the app, bottom sheets included, gated on dev mode.Description
A destination declares whether it may be entered directly, in the model that defines it. 88 root routes are
Routes.DeepLinkableand 15 areRoutes.InternalOnly, anddeepLinkableComposableaccepts only the former, so a new screen has to make the choice rather than inherit one.ScreenDeepLinksadapts the URI and nothing else: it derives the id by kebab-casing the class name, soRoutes.RgsServeris reachable atbitkit://screen/rgs-server. 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. Each sealed route family declaresDeepLinkStartorInternalOnlyon its states and owns afromDeepLinklookup, andSheetDeepLinkspicks the family and wraps the result. Ids derive from class names throughout, sobitkit://screen/widgets/price-editcomes fromSheet.WidgetsplusWidgetsRoute.PriceEdit.send,receive,backup,widgets,hardwareand the three sheets with no nested graph.isDevModeEnabledinAppViewModel.handleDeeplinkIntent, and holds one until the wallet is loaded;ContentViewreplays it.MainActivityonce the gated pipeline has read it, so graph creation cannot navigate outside the dev-mode gate.bitcoin:,lightning:,lnurl*) on the scanner decode path, untouched.docs/deeplinks.md. The route markers,deepLinkableComposable, the sheet families'fromDeepLinkand the unit tests are the source of truth, and a parallel doc drifted three times during review.Not every nested state is a valid start destination. I fired all 51 against an emulator, then re-ran each rejected one in isolation against logcat. The states left internal fall into three groups:
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.Tests pin the classification, and a final sweep confirmed the rejected paths 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 route declares its eligibility.SheetDeepLinksTest.kt: bare-id defaults, sub-route selection, case-insensitive lookup, ids derived from theSheetclass names, and that every state marked as a start is registered in its family lookup.ScreenDeepLinkDetachmentTest: graph creation stays on Home after detachment, the captured URI reaches Settings only through the replay, and a denied route is not matched.journeys/deeplinks/, including cold-start cases for dev mode off and on.just compile,just test,just lintall pass, no new detekt findings.