feat(app): add navigation shell for multiple primary screens#78
Merged
Conversation
The app layout previously offered only a single central balance chart or a modal dialog over it — neither a home for screens a user returns to and works in over time. The milestone's feature screens (transactions, recurring rules) had nowhere coherent to live. Introduce NavigationShell, a content-agnostic container (left nav rail driving a StackLayout) that imports no Finch or QtCharts type, so it is unit-testable headless with injected placeholder screens while Main.qml composes the real chart/client-bound destinations into it. Migrate account creation off its modal dialog onto a non-modal Accounts screen as the first proof of the pattern; Transactions and Recurring Rules get placeholders until their issues land. The connected-only account-creation gate, previously on the toolbar button that opened the dialog, is preserved by injecting it into AccountsPanel as a bool so the panel stays Finch-free. The Overview's disconnected empty-state and ping spinner are preserved in a wrapper item. Tests run headless via per-executable QUICK_TEST_SOURCE_DIR directories so the two test executables don't cross-load each other's specs. Related to #76
A nested Layout defaults Layout.fillWidth to true, so the rail's ColumnLayout expanded to consume most of the window and starved the content StackLayout — the four nav buttons spanned the full width and each screen rendered in a thin strip at the right edge. Manual launch on a real display surfaced this; the headless specs only asserted button existence, not width. Pin the rail with explicit fillWidth: false and a fixed 180px min/max, and add a spec assertion bounding the button width so the regression is caught headless.
Pre-PR review hardening: - Add a spec emitting a nav button's clicked() signal and asserting the active destination switches, covering the onClicked -> currentIndex wiring that the offscreen platform can't reach via synthetic mouse events. - Tighten the rail-width regression assertion to the pinned 180px. - Suppress the redundant "No accounts yet." label while disconnected, where the "Connect to the daemon" hint already explains the empty list. - Document that NavigationShell.screens is write-once (reparented on completion).
Issue #76 requires preserving the gate that only allows account creation when the daemon is connected — previously enforced by the toolbar button that opened the modal dialog, now an injected createEnabled bool on AccountsPanel. It had no automated coverage. Add a spec asserting the embedded create form is disabled when the gate is closed and enabled when open, reusing the existing FinchFormTest module and mock (extended with an accounts property for AccountsPanel's list).
There was a problem hiding this comment.
Pull request overview
Adds a reusable navigation shell to the Qt app so multiple primary, non-modal screens can coexist (addressing issue #76’s layout constraint), migrating account creation from a modal dialog into an Accounts destination while introducing placeholder destinations for upcoming work.
Changes:
- Introduce
NavigationShell(left nav rail +StackLayout) andPlaceholderScreenfor unfinished destinations. - Add
AccountsPaneldestination embedding the existingCreateAccountFormand showing the accounts list. - Add headless Qt Quick Tests for
NavigationShell, and expand the existing form test module to coverAccountsPanel.
PR description checks (per repo conventions):
- Overview section present and clear: PASS
- Scope cohesion vs. stated purpose: PASS
- No private tooling references added: PASS
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| CHANGELOG.md | Documents the new navigation shell and the Accounts screen migration. |
| app/CMakeLists.txt | Registers new QML files in the app module; adds a new tst_navigationshell target and adjusts Quick Test source dirs. |
| app/qml/Main.qml | Replaces the single-screen layout + modal account dialog with NavigationShell destinations (Overview/Accounts/placeholders). |
| app/qml/NavigationShell.qml | New content-agnostic navigation container (nav rail + persistent destination stack). |
| app/qml/AccountsPanel.qml | New Accounts destination: inline create form + read-only accounts list, gated by connection. |
| app/qml/PlaceholderScreen.qml | New lightweight “Coming soon” destination used both in-app and in shell tests. |
| app/tests/tst_navigationshell.cpp | Quick Test entry point for the new NavigationShell QML specs. |
| app/tests/navigationshell/tst_NavigationShell.qml | Unit tests for shell composition, switching, rail button rendering, and state preservation. |
| app/tests/MockFinchClient.qml | Adds an accounts property so AccountsPanel can bind its list model cleanly in tests. |
| app/tests/createaccountform/tst_CreateAccountForm.qml | New QML test spec for CreateAccountForm behavior. |
| app/tests/createaccountform/tst_AccountsPanel.qml | New QML test spec verifying the create-enabled gate propagates to the embedded form. |
| screens[i].parent = contentStack | ||
| _warnOnTitleMismatch() | ||
| } | ||
| onDestinationTitlesChanged: _warnOnTitleMismatch() |
Owner
Author
There was a problem hiding this comment.
Fixed in 8be8a3f — gated the title-mismatch check on a _completed flag so it only runs after the screens are reparented into the stack.
The destinationTitles assignment during construction can fire onDestinationTitlesChanged before Component.onCompleted reparents the screens into the stack, when the destination count is still 0 — warning spuriously. Gate the check on a completion flag so it only runs once the stack is populated and on genuine post-construction title changes.
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.
Overview
The app layout previously offered only two homes for UI: the single central balance chart, or a modal dialog over it. Neither suits a screen a user returns to and works in over time, so each new feature had to either overload the chart or accrete as another modal. This adds a navigation structure so multiple primary screens can coexist.
A persistent left nav rail switches between four top-level destinations: Overview (the existing balance chart), Accounts, Transactions, and Recurring Rules. Account creation moves off its modal dialog onto the non-modal Accounts screen — the first screen to prove the pattern — while Transactions and Recurring Rules are placeholders until their own work lands. The connected-only gate that restricts account creation to a live daemon is preserved.
How it works
The navigation shell is a content-agnostic container: it imports no domain client and no charts, so it is unit-tested headless with lightweight placeholder screens while the main window composes the real chart- and client-bound destinations into it. Screens are shown through a StackLayout, which keeps each destination alive so state (chart selections, in-progress form input) survives switching away and back.
Closes #76.