Let features own their navigation graphs - #80
Merged
Conversation
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
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.
Why
Every route in the app was declared in one
Routesobject in:app, andNavGraphs.ktbuilt every destination itself, importing screen composables from all seven feature modules. A feature could not describe its own navigation; adding a screen meant editing:app.What changed
Four features now own their routes and register their own destinations:
:feature:onboarding—OnboardingRoutes+onboardingGraph:feature:settings—SettingsRoutes+settingsGraph:feature:projects—ProjectsRoutes+projectsGraph:feature:terminal—TerminalRoutes+terminalGraph:appkeeps the singleNavHostand calls those graphs. Cross-feature navigation stays as callbacks supplied by:app, so no feature learns another feature's routes: onboarding takes anonFinished, projects takesonOpenProjectandonOpenPreferences.encodeRouteArgmoved to:core:commonso any module building a route can escape its arguments, rather than the helper being private to:app.ProjectsRoutesalso owns thepicked_foldersaved-state key. Its producer (the folder picker) and both consumers (hub, create-project) are all inside:feature:projects, so the protocol belongs there. Thegit_conflict_pathkey deliberately stays in:app— its producer and consumer are in different features, and moving it would make one import a constant from the other, recreating the coupling at the navigation layer.Behavior
No behavior change, and this was checked rather than assumed: all 15 relocated route strings are byte-identical to their previous values, verified by diffing the moved constants against
Routes.ktat the previous commit. Destination registration order, argument extraction and composable wiring are unchanged; only the file that declares them moved.Scope
Two deliberate exclusions.
Type-safe routes are not part of this change. The plan listed converting from string routes to type-safe routes as a preparatory step. It changes route matching and argument parsing, and the only meaningful verification for that is on a device. Keeping the strings identical is what makes this change provably behaviour-preserving, so the conversion is better done separately.
The editor and git destinations stay in
:appfor now. They are entangled: the editor's navigation callbacks target git destinations, and git's conflict screen hands a path back to the editor. Splitting them is part of the change that eliminates the editor-to-git dependency, where the contract between them is being designed anyway.Tests
RoutesTestcontinues to cover the route builders that remain in:app.Verification
From a clean worktree at the branch tip:
./gradlew test detekt verifyModuleBoundaries :app:assembleDebug— BUILD SUCCESSFULWhat is not verified here: back-stack behaviour, saved-state result delivery across process death, and predictive back are only observable on a device, and this change was not exercised on one. The byte-identical route strings and unchanged wiring are the argument that behaviour is preserved; a device pass over hub → create project → folder picker → back, and onboarding → hub, is still worth doing before release.
Note on included work
This branch also carries the route-argument encoder and its tests, which existed as uncommitted work in the tree from a separate task. Phase 4 needed a shared encoder for features to build routes, so it was moved to
:core:commonrather than left stranded in:app. The implementation and its tests are unchanged apart from the package.