Repository navigation
Storage-architecture editing: safe hierarchy edit mode + nav rework (v0.1.9) - #41
Merged
Merged
Conversation
Ten TDD tasks: the data layer for rename/reorder/delete-with-strategy, the ordering rule, the delete-strategy dialog, edit mode on the shelves and locations lists, the tabs/list toggle, the household edit page, the nav rework, collapsible household groups, flow tests, and the CLAUDE.md rewrite. Two deliberate deviations from the spec, both flagged in the document: reorder uses move up/down buttons rather than drag (Compose has no built-in reorderable list, drag would need a new dependency and fight the existing SwipeToDismissBox, and it is the worst affordance for TalkBack); and preferences use SharedPreferences, because DataStore is not a dependency of this project.
The backend requires a delete strategy when a LOCATION has shelves -- not when it has products. The plan decided needsStrategy from productCount for both levels, so deleting a location containing one empty shelf would have sent a strategy-less request and 422'd every time. DeletePlan now takes an explicit contentCount (product_count for shelves, shelf_count for locations), and the DTOs carry the counts the server now exposes for exactly this purpose.
Adds rename/reorder/deleteWithStrategy to the location and shelf repositories, name to the household update, and a restore-by-batch endpoint. DELETE now carries a body (@http hasBody, since @delete cannot), because the strategy and the client-minted batch id have to travel with the request. The request DTOs deliberately have no property defaults: encodeDefaults=false would silently drop them and the server would 422.
Fixes review findings on the storage-architecture-editing data layer: - C-1 (critical): UpdateHouseholdRequest.name lacked a default, so encodeDefaults=false sent it as an explicit "name":null on every color/icon-only update. Laravel's `sometimes|required` rule rejects an explicit null (only an absent key passes), so every household theme change 422'd in production. Defaulted name back to null and documented why it's the deliberate exception to the no-defaults rule that the other request DTOs follow. - I-1: replaced DeleteRequestSerializationTest's body-string assertions (which couldn't discriminate a defaulted field from a real one) with descriptor-based checks (isElementOptional) across every request DTO, plus a dedicated test pinning the UpdateHouseholdRequest asymmetry. - I-2: documented that LocationDto.shelf_count/product_count and ShelfDto.product_count go cache-stale after a shelf/product mutation, so UI code must refresh before deriving needsStrategy from them. - M-1: replaced ShelfRepository/LocationRepository.deleteWithStrategy's long parameter lists with ShelfDeletion/LocationDeletion value objects, removing the LongParameterList baseline suppression. - M-2: rename() in both repository impls no longer fabricates a 1-element cache on a miss. - M-3: moved ReorderRequest out of ShelfDtos.kt into its own file, since it's shared with LocationApi. All three gates green: 139 unit tests, ktlintCheck, detekt.
Manual position wins; name is only the tie-break for never-reordered items. A star is deliberately not an input: it is a marker and a filter, never a sort. Shelf order is physical, and a star that floated the bottom shelf to the top would leave the list disagreeing with the actual fridge.
Asks what to do with the contents of a shelf/location being deleted: move them, keep shelf products in Unsorted (shelf level only), or delete them too. One dialog per gesture — a batch of three shelves is summarised once, not interrogated three times. Generic over the strategy type (StrategyOption<T>/DeleteStrategyDialog<T>) so the same dialog serves both ShelfDeleteStrategy and LocationDeleteStrategy, via the shelfStrategyOptions()/locationStrategyOptions() factories — the brief had hardcoded ShelfDeleteStrategy, which would have made Task 5's location delete impossible to type correctly. Never offers a MOVE_* option when there is nowhere to move to, defaults to the safest available option (factories list safest-first), and disables confirm while a target is required and unpicked. An empty container (needsStrategy false) still shows a plain confirm dialog with no strategy radios — it is never deleted silently, which was the bug that started this whole project.
…criptionRes Review of the delete-strategy dialog found its two safety behaviors — never offering "move" with nowhere to move to, and keeping confirm disabled while a target-requiring option has no target picked — had zero test coverage: a reviewer deleted both guards at once and the full suite stayed green. Adds DeleteStrategyDialogTest (androidTest, matching ProductFilterSortRowTest's isolated-composable approach) covering: the option filter, the confirm-enabled guard, and the empty-container plain-confirm path that must always emit (null, null) rather than silently skipping the dialog. Mutation-tested each guard on a real emulator to confirm the tests actually discriminate; findings recorded in .superpowers/sdd/task-3-report.md (gitignored). Also removes StrategyOption.descriptionRes: both factories set it equal to labelRes, so the conditional description row it fed never rendered. Dead field, dead branch, both gone. DeletePlan.canMove/hasOtherTargets was flagged as a similarly dead parallel signal, but left in place: Tasks 4 and 5 in the current plan doc construct/ read it directly, so removing it now would break not-yet-built, already- specified callers.
…d target list DeletePlan carried hasOtherTargets/canMove, but DeleteStrategyDialog never read it -- it decides move-availability from the List<MoveTarget> it is handed. Two parallel signals for one fact, and the dead one could disagree with the live one: a plan claiming canMove while the rendered target list is empty would offer a move with nowhere to go. Keeps the signal the user can actually see. The rule stays pinned by DeleteStrategyDialogTest.move_option_is_not_offered_when_there_are_no_targets, which is mutation-proven; the two DeletePlanTest cases it replaces asserted on the flag rather than the behaviour. Plan Tasks 4/5 amended so they no longer build against the removed field, and Task 5 gains the shelf_count-vs-product_count test (a location with 3 EMPTY shelves still needs a strategy).
Global (not per-location) tabs/list preference for the shelves screen, backed by SharedPreferences like the other settings stores. Registered in Hilt and wired into SessionCleaner.clear() so one account's view choice never survives into the next session.
… via strategy dialog Replaces deleteMode/selectedShelves/deleteSelected/enterDeleteMode/exitDeleteMode (the tester bug: bulk delete with NO confirmation dialog at all) with an edit mode that only ever deletes through requestDelete() -> DeleteStrategyDialog -> confirmDelete() — the top bar's Delete action can never bypass the dialog. Key invariants, each covered by a discriminating test (verified red/green by temporarily reintroducing the bug and reverting): - One user gesture mints exactly ONE deletion_batch_id, shared by every shelf deleted in that gesture, so a single Undo restores all of them. - The Unsorted shelf (is_system) can never be selected or renamed. - Display order always comes from orderByPosition (Task 2), with the system shelf forced last regardless of its own position value; reordering sends the complete non-system id list, excluding the system shelf from the payload. - requestDelete() only opens the dialog (sets pendingDelete + moveTargets); nothing is deleted until an explicit confirmDelete(). Entering edit mode forces the list view (tabs can't host reorder/rename), and remembers the prior view to restore on exit. Added ShelvesUiState.moveTargets: the live, non-system, non-selected shelves a delete's products could move to — this list, not a boolean, is what DeletePlan.kt says should gate the dialog's move option (its own doc: no canMove/hasOtherTargets by design). A shelf's DeletePlan.contentCount is its productCount (contentCount only differs for a location's shelf_count, in Task 5).
LocationDetailScreen renders either the tab strip + pager (unchanged browsing behavior) or a plain shelf list (EditableRow per shelf), chosen by the new listView preference and forced on whenever editMode is on. The top bar swaps its delete-mode block for edit mode: a Cancel TextButton, and an error-colored Delete button that calls requestDelete() — never confirmDelete() directly, so the strategy/confirm dialog can never be bypassed. Wires DeleteStrategyDialog (shelfStrategyOptions(), state.moveTargets) and an Undo snackbar keyed on lastBatchId. EditableRow (new) is the row-level gate for the Unsorted shelf: its checkbox, rename pencil, and up/down buttons are all withheld when isSystem is true, regardless of edit mode. (The equivalent "cannot be selected" rule is enforced independently in ShelvesViewModel.toggleSelection — this row just forwards taps, so both layers agree by construction rather than by convention.) Covered by an isolated Compose test (EditableRowTest, createComposeRule, no Activity/Hilt — same pattern as DeleteStrategyDialogTest). New strings (EN+NL): location_edit_shelves_cd, location_view_toggle_cd, shelf_move_up_cd, shelf_move_down_cd, shelf_rename_title, shelves_deleted. Removed the now-dead location_delete_shelves_cd.
…ps, flow test - EditableRow/LocationDetailScreen: rename and move up/down each fire a request immediately, so hold them off while state.loading is true (selecting stays enabled — it never touches the network). Matches the existing pattern on the edit-mode Delete button. - ShelvesViewModel.rename(): clamp to MAX_SHELF_NAME_LENGTH itself instead of relying solely on the Compose text field's take() — the method is now safe to call from anywhere, not only the one wired-up dialog. - Added the missing positive rename_updates_the_shelf_name test and the move_down_on_the_last_shelf_does_nothing boundary counterpart to move_up_on_the_first_shelf_does_nothing. - DeleteShelfFlowTest (the regression test for the original no-confirmation bulk-delete bug) now drives the actual new flow: enter edit mode, select via the list row, Delete opens the strategy/confirm dialog, and only confirming inside it fires the DELETE.
… partial-batch undo Critical: reorder's server response already includes the Unsorted (is_system) shelf (PATCH .../shelves/reorder ends with `return $this->index(...)`), and ShelvesViewModel.move() was appending its own local copy on top of that, producing a duplicate shelf id. Every location with an Unsorted shelf crashed on the first reorder (LazyColumn's itemsIndexed keys by shelf.id). Filter the server's own system-shelf entries out before re-adding ours. The test fake's reorder() was lying about this (returning only the reordered subset, with a comment claiming the opposite of what the real endpoint does) — fixed the fake to return the full list, faithfully, so the suite can actually catch this class of bug. Also pinned the optimistic reorder frame with a gated fake, since the synchronous fake previously let a broken optimistic update slip through. Important: requestDelete() built its delete-strategy plan from cached product_count, which product mutations elsewhere on the screen (add-product sheet, barcode scan, product delete) never update — a stale zero skipped the strategy prompt and produced a 422 deleting a non-empty shelf. requestDelete() now refreshes the shelf list from the server before building the plan. Minor: confirmDelete() now restores the pre-edit view like exitEditMode() does, instead of leaving listView forced on. A batch delete that fails partway now stops, reconciles the list with the server, and still surfaces Undo for whatever landed instead of silently losing it; confirmDelete() filters the selection against the live shelf list the same way requestDelete() does. Removed the two @Suppress annotations (ShelvesViewModel TooManyFunctions, SessionCleaner LongParameterList) in favor of the project's detekt-baseline mechanism, matching how AuthViewModel's TooManyFunctions is already handled. Added discriminating tests for every fix; each was verified red against the pre-fix code and green after (see task-4-report.md for the per-finding evidence).
Rename, retype, reorder and delete-with-strategy for storage locations, behind the same pencil pattern as the shelves list (Task 4). Removes swipe-to-delete on locations: a swipe should not be able to destroy a fridge full of food. Deletion now requires entering edit mode, selecting, and answering the strategy dialog (LocationDeleteStrategy — move or delete contents, deliberately no unsort at this level). DeletePlan.contentCount is built from shelf_count, not product_count: the server requires a strategy whenever a location HAS SHELVES, even completely empty ones. moveTargets is the household's other live locations, computed fresh in requestDelete() after a refresh (avoids a stale shelf_count skipping the strategy prompt). EditableRow gains optional rename/move-up/move-down content-description overrides (defaulting to the existing shelf strings) so the shared row component reads correctly for locations too, not "Rename shelf" on every container type.
Capture the renamingLocation lambda parameter directly instead of re-reading the mutable var inside the Save button's onClick (matches LocationDetailScreen's renamingShelf idiom); update EditableRow's KDoc now that it's genuinely shared between the shelves and locations screens rather than shelf-specific. No behavior change — confirmed via the full test/ktlint/detekt gate rerun and an independent review pass.
Home's swipe-to-delete (AllStoragesScreen -> DrawerViewModel.deleteLocation) was the second location-delete entry point on this branch, and nobody wired it up when DeleteLocationRequest made deletion_batch_id unconditionally required and a strategy required whenever a location has shelves. Every swipe-to-delete from the Home dashboard 422s on this branch as shipped. - AllStoragesScreen: swipe gesture removed; delete now lives behind an Edit toggle (per-row trash icon, since Home spans every household and a cross-household batch-select has no coherent move-target semantics), routed through the same DeleteStrategyDialog/locationStrategyOptions Task 5 uses for StorageOverviewScreen. - DrawerViewModel: deleteLocation() replaced with requestDelete/ confirmDelete/cancelDelete/undoDelete, mirroring StorageOverviewViewModel - mints one deletion_batch_id per gesture, sends LocationDeleteStrategy via LocationRepository.deleteWithStrategy(), and wires Undo through RestoreRepository. contentCount is built from shelf_count (not product_count) and requestDelete() re-fetches locations fresh rather than trusting HierarchyStore's cached entries, closing the same staleness gap Task 5 closed one screen over. - LocationRepository.delete() (the bodyless path) is now doc-commented as 422-always so a third screen doesn't make the same mistake. - Two now-dead strings (delete_dialog_location_title/text) removed from both locales. DrawerViewModelTest rewritten: 16 tests, including mutation-verified coverage of the batch-id/strategy requirement, the shelf_count-vs- product_count trap, the fresh-fetch staleness guard, and cross-household move-target isolation. FakeLocationRepository's delete() now throws (models the real DeleteLocationRequest 422) instead of silently succeeding, per the Task-4-crash lesson about fakes that lie about the server.
…flow test, refresh coverage Critical-1: DeleteLocationRequest/DeleteShelfRequest's strategy and target_*_id had no property default, so the app's Json (explicitNulls=true, encodeDefaults=false) always encoded them — including as an explicit null. That put a PRESENT-but-null key on the wire for every delete except move_contents/move_products, which the server's Rule::requiredIf validates as a type error rather than "absent", 422ing on every empty-container or delete-contents/unsort_products delete. Give both fields `= null` defaults so a value still equal to the default is OMITTED, matching what the server expects. DeleteRequestSerializationTest now asserts the actual encoded body for every strategy (empty / delete-contents / move+target) instead of just "has no default" — the old assertion happily pinned the broken shape. Important-2: DeleteLocationFlowTest still drove the old swipe-to-delete UI Task 5 removed; rewritten against the current edit-mode -> select -> Delete -> DeleteStrategyDialog -> confirm flow, mirroring DeleteShelfFlowTest. Minor-3: hierarchyStore.refresh()/store.refresh() calls throughout StorageOverviewViewModel and DrawerViewModel.confirmDelete had no test coverage — mutating any of them to Unit left the suite green. Added 6 tests (create/rename/reorder/confirm/undo in StorageOverviewViewModelTest, confirm in DrawerViewModelTest) that wait on HierarchyStore's own state, each verified to fail red when its call site is removed. Minor-4: StorageOverviewViewModelTest's fake LocationRepository.delete() silently succeeded, unlike the real (strategy-required) endpoint and unlike DrawerViewModelTest's equivalent fake. Now throws, matching the real 422. Minor-5: HierarchyStore hard-coded CoroutineScope(SupervisorJob() + Dispatchers.IO) — a real background thread outside any test's control. Injectable now, mirroring LiveUpdates' own constructor-overload shape; TestHierarchy.store() and DrawerViewModelTest default to an unconfined test dispatcher instead. Minor-6: StorageOverviewViewModel.confirmDelete's vanished-selection guard returned early without closing the dialog or exiting edit mode, leaving a dead Confirm button. Extracted closeDeleteFlow() and call it on that path too. All four Android gates green: testDebugUnitTest (217 tests), compileDebugAndroidTestKotlin, ktlintCheck, detekt.
A generic rename/re-theme entry point (thin pass-through to HouseholdRepository.update) plus the editMode flag HouseholdsScreen will use to gate row navigation into the upcoming edit screen. update() does NOT infer "leave alone" from null the way updateTheme() does — callers must pass every field's CURRENT value to avoid touching it, because UpdateHouseholdRequest's color/icon have no default and are always encoded on the wire (explicit null clears the theme server-side). updateTheme() now delegates to update() with name = null, relying on name's default to omit the key instead.
Descriptor-shape assertions alone ("this property has no default") already
proved insufficient once this branch — a prior review flagged that exact gap
as how Tasks 4/5 shipped a Critical 422. Add two tests that encode a real
instance the way each production call site builds one and assert the literal
JSON bytes for both directions: a rename must still encode the current
color/icon (not clear them), and a theme-only clear must omit `name`
entirely (not blank it, not 422).
Verified both directions discriminate: temporarily defaulting color/icon and
separately un-defaulting name each turned the new tests (and the pre-existing
descriptor test) red; reverted after confirming.
Rename plus the colour/icon picker (moved out of the palette-icon dialog), and a danger zone holding Leave. HouseholdsScreen's per-row palette/leave controls are gone; a top-bar edit-mode pencil now gates row navigation into the new screen, mirroring the shelves/locations edit-mode idiom. Leave is not an edit of the household — it ends your membership — so it sits in a separated, error-tinted danger zone (plain Card, not FrostCard — see FrostCard's own doc comment) rather than beside the rename field. Its confirm copy is now honest that leaving as the last member permanently deletes the household, not just "you'll need a new invite". Delete household still does not exist: nobody owns one today. That lands with roles (Spec 2) — this danger zone is where it will go, noted in a comment. The colour/icon swatches apply immediately on tap (existing updateTheme()); only the name has an explicit Save. Leave waits for HouseholdsUiState.left (set once leave() completes server-side) before navigating back, rather than navigating on tap — the same pattern ProductDetailViewModel uses for delete, needed here because the edit screen's own ViewModel instance (and its viewModelScope) would otherwise be torn down mid-request the moment navigation pops its back-stack entry. Updated LeaveHouseholdFlowTest for the new edit-mode -> row -> danger-zone flow (no longer a per-row Leave button).
Households leaves the bar — it is a management screen, not a daily destination, and the Storage tab already acts across households. Scan takes the freed centre slot (the primary-action slot; scanning is a weekly action, search an occasional one). Settings comes back into the bar as More, since it now holds households, join/invite and account. Also updates every androidTest flow test that navigated via the removed bottom-nav-households/bottom-nav-search tabs or the Dashboard settings gear (now gone from Dashboard/Home/Missing-items' top bars), and deletes SearchHouseholdPickerFlowTest — the multi-household search picker it covered is dead code now that Search is always opened already scoped to a household.
…7 review)
Task 7's bottom-nav rework shipped with Search only reachable through two
buried, pre-existing paths (Dashboard's products stat card and Storage
Overview's search icon) instead of the direct top-bar entry the approved spec
calls for ("Search loses its tab and keeps the top-bar icon it already has").
Adds a Search icon to the top bar of Dashboard, the Storage tab
(AllStoragesScreen) and Missing items, all wired to Routes.search(). Guards
reachability with a source-scan JVM test (no Robolectric/compose-ui-test on
this module's unit-test classpath) that fails if either MainActivity stops
wiring a screen's onOpenSearch to Routes.search(...) or a screen stops
actually invoking it from the UI.
Also fixes the Scan tab being a dead end: ScannerScreen had exactly one
delivery contract (write scanned_code to the previous back-stack entry and
pop), designed around its only caller, LocationDetailScreen. The bottom-bar
Scan tab opened the same route with no shelf screen underneath it, so
scanning from the bar silently did nothing. Makes the caller's intent an
explicit route argument (scanner?mode=add|lookup) instead of inferring it
from the back stack: ADD keeps the existing add-to-shelf delivery, LOOKUP
navigates to Search with the scanned code pre-filled and immediately run
(SearchViewModel.searchFor). The mode decision and the two delivery branches
are pure, unit-tested functions (ScannerMode, scanDeliveryActionFor); source
tests confirm the two branches can't be confused with each other and that
each entry point requests the right mode.
Removes now-dead nav_households and search_choose_household_title strings;
keeps nav_search, reused for the new icons' content description.
Every added/modified test was verified to go red on the bug it guards
(mutation-tested and reverted) before this commit.
Collapse state and household order persist per device. A household's physical structure is shared reality and lives on the server; which of your households you care about is a personal view preference and stays local.
Covers the bug the whole change exists to fix: deleting a shelf or location that holds contents now stops and asks, instead of destroying them silently. - DeleteShelfStrategyFlowTest: a non-empty shelf forces an explicit strategy choice, Move is never offered with nowhere to go, the Unsorted shelf can't be selected or offered as a move target, Undo restores a non-destructively deleted shelf WITH its products, and one multi-shelf gesture shares a single deletion_batch_id across every request. - DeleteLocationFlowTest: adds the location-side half of the contentCount asymmetry — a location holding only empty shelves still requires a strategy, because the server counts shelves, not products. The original test already drove edit mode correctly (its swipe-gesture staleness was fixed in 378159d, before this task); left untouched. - RenameLocationFlowTest: fills a real gap — zero flow-level coverage previously existed for the edit-mode rename affordance. - All wire-body assertions read the actual recorded MockWebServer requests (method/path/body), not just UI state, since several guarantees are about what the client puts on the wire (e.g. omitting null strategy/target keys).
…te invariants The navigation section still described the old 5-tab bar (Households on the bar, Settings behind a gear on every screen) and the scope guardrails still banned roles/permissions — both now contradicted by shipped behaviour and by the 2026-07-13 roles decision. Also documents, as LOCKED load-bearing rules, the delete-confirmation/strategy/batch-undo/null-omission invariants this branch introduced (each was a real Critical/Important bug here), plus two testing lessons (a lying fake, and asserting descriptor shape instead of wire bytes) and the detekt/androidTest coverage gap Task 9 found. Every claim was verified against the current code on this branch, not carried over from the plan — see .superpowers/sdd/task-10-report.md for the file:line trail. No production code changed; ./gradlew ktlintCheck is unaffected.
…r 3) ShelfRepository.delete() and LocationRepository.delete() (plus their ShelfApi/LocationApi backing endpoints) had zero production callers and sent a bodyless DELETE that the server unconditionally 422s now that deletion_batch_id is required. Left in place they were a landmine: the next caller to wire one up would reproduce the Task-5b bug. Removed the interface declarations, implementations, backing Retrofit endpoints, and every fake override across the unit-test suite (deleteWithStrategy is the only delete path either repository exposes now).
ShelvesViewModel.move() and StorageOverviewViewModel.move() applied the optimistic reorder frame and then, on a failed repository.reorder() call, did nothing but set an error string — the optimistic order stood forever, silently lying about server order for the rest of the screen visit. Both methods' own comments already asserted the snap-back happened; it didn't. Capture the pre-move list before applying the optimistic frame and restore it when the server call fails, in both ViewModels. Added a discriminating failure test to each VM's suite (confirmed red against the pre-fix code, now green) using a new failReorder flag on each fake. Splitting StorageOverviewViewModelTest's hierarchyStore.refresh() coverage into its own file (StorageOverviewViewModelHierarchyRefreshTest.kt) — the new failure test pushed the original class over detekt's LargeClass threshold.
Task 7's bottom-nav rework deleted the Search tab's household picker along with its sole test coverage (SearchHouseholdPickerFlowTest), and every replacement search entry point regressed to hard-coding the FIRST household: MainActivity's scanner LOOKUP branch, DashboardScreen's top-bar icon and products stat card, AllStoragesScreen's top-bar icon, and MissingItemsScreen's top-bar icon. A second household's search was only reachable by drilling Home -> that household's own "+" icon -> Storage overview -> its search icon. Reintroduces the picker as a shared ui/common/HouseholdPickerSheet (ported from git history's MainActivity ModalBottomSheet) and wires it into all four entry points: shown only when the household count is greater than one, skipped (direct navigation) otherwise. The scanner LOOKUP branch now holds the scanned code in NavHost-level state until a household is picked. Re-adds the deleted flow test's coverage as HouseholdPickerFlowTest, proving the picker's SECOND option actually hits that household's search endpoint (via Dashboard's icon, the one entry point drivable without a real camera scan).
…ocation title (ALSO FIX) Three string resources shipped and were never wired to the requirement they belonged to, because an unused string breaks no gate: - R.string.shelf_unsorted had zero usages; a Dutch user saw the raw English "Unsorted" everywhere a shelf name renders. Added ui/common/ShelfDisplayName.kt (mirrors storageTypeLabel's pure-lookup + composable split) and wired it into LocationDetailScreen's tab strip, shelf list and warning content description, DashboardScreen's favorite shelves, and ProductsViewModel's move-target list. MoveTarget now carries locationName/shelfName/isSystemShelf raw instead of a pre-baked label, since a ViewModel has no locale-correct way to resolve a string resource itself (this app's language switch is a per-Activity Configuration override, invisible to the singleton Application context a ViewModel could otherwise inject) — the composable renders the label. - delete_undone / delete_undo_failed were dead; a 409 from the restore endpoint fell through toUserMessage to a generic "Something went wrong." Added ui/hierarchy/UndoOutcome and threaded it through ShelvesViewModel, StorageOverviewViewModel and DrawerViewModel's undoDelete() (all three call the same restore endpoint and shared the bug); each screen now shows the specific localized snackbar instead of the generic fallback, without double-messaging via the generic error path. - LocationDetailScreen's top bar always showed the generic "Shelves" title even though the location itself became renamable this branch. It now reads the location's own name from drawerViewModel's already- loaded state, falling back to the generic title only until that loads. Adds StringResourceUsageTest: every string declared in values/strings.xml must have at least one Kotlin/XML reference, so an unwired string like these three fails the JVM suite instead of shipping silently. Also adds LocationTitleWiringTest (source-scan, mirrors NavigationWiringTest) and ShelfDisplayNameTest for the two changes with no other test surface.
…r 9)
Routes.SCANNER ("scanner?mode={mode}") is one route pattern shared by two
callers: LOOKUP (the bottom-bar Scan tab itself) and ADD (opened from a
shelf screen, with no bottom bar underneath it). NavController's own
destination.route is the bare pattern for both, so the bar's
`currentRoute == tab.route` check couldn't tell them apart — the bar
resurfaced over the camera in ADD mode, with Scan shown selected.
Adds scannerRouteIsTheBottomBarTab(mode), a pure function (mirrors
scanDeliveryActionFor's split between pure decision and NavController
side effect) resolved from the back stack entry's own `mode` argument,
and uses it to gate both the bar's visibility and the Scan tab's
selected state specifically for the SCANNER route.
…unused-string gate CLAUDE.md claimed Search was reachable via a top-bar icon on every household-scoped screen — Dashboard, Home and Missing items aren't actually household-scoped, and their icon (like the Scan tab) now opens a household picker when the account has more than one household (Blocker 2). Also documents scannerRouteIsTheBottomBarTab (Minor 9) and StringResourceUsageTest as a Testing-lessons entry, matching the existing section's format for a failure mode this branch hit.
…t one ProductController::destroy() (server) mints a batch-of-one deletion_batch_id and returns it specifically so the client can offer Undo, but ProductApi.delete() declared no return type, so the body — and the batch id inside it — was silently discarded. Product delete is the app's single most frequent destructive action and was the only one with no Undo. - ProductApi.delete() now returns ProductDeleteResponse (deletion_batch_id, snake_case to match the wire exactly); ProductRepository.delete() unwraps it to a batch id. - Both call sites now capture the batch id and offer Undo the same way Shelves/Storage/Drawer already do (RestoreRepository + a snackbar action): ProductsViewModel/ProductsPane (swipe-to-delete) and ProductDetailViewModel/ProductDetailScreen (detail screen's delete button). Confirm dialogs are unchanged; Undo is additive. - ProductDetailScreen navigates away on delete (unlike the others, which stay put) — sequenced the deleted/undo/outcome snackbars into one LaunchedEffect so onBack() only fires once that's fully resolved, instead of risking a mid-flight cancellation of undoDelete()'s coroutine. - Reused the existing delete_undo/delete_undone/delete_undo_failed strings; added product_deleted (EN+NL). - Fixed DeleteProductFlowTest's mock (the real endpoint returns 200 + a JSON body, not an empty 204) and updated 8 JVM fakes for the new ProductRepository.delete(): String signature. - Added ProductDeleteResponseSerializationTest pinning the real response body decodes correctly (and that a camelCase key does not).
…cher HierarchyStoreTest, DashboardViewModelTest, MissingItemsViewModelTest, ProductsViewModelTest, and SessionCleanerTest still constructed HierarchyStore via its 4-arg convenience constructor, which falls back to the real Dispatchers.IO. That scope lives outside runTest's control, so a late _state emission from a still-in-flight refresh() can wake a stateIn(viewModelScope) collector after MainDispatcherRule has already reset Dispatchers.Main — an intermittent "Dispatchers.Main was accessed..." failure surfacing in whatever unrelated test happens to be running at the time. DrawerViewModelTest/TestHierarchy.store() already carried the fix (Task 5/5b); this extends the same injected-dispatcher mechanism (HierarchyStore's 5-arg constructor + UnconfinedTestDispatcher()) to every remaining construction site instead of inventing a second one. Production behavior is unchanged — the @Inject constructor still defaults to Dispatchers.IO.
ProductsViewModel.delete()'s failure branch set state.error, then called refresh() to revert the optimistic removal — but refresh() routes through the shared launch() helper, which resets error = null the instant its coroutine starts. The error the user needed to see was wiped before it could ever be rendered, so a failed delete failed silently (exactly the class of bug this branch exists to eliminate). Switch the revert to refreshSilent(), which already exists in this file and reloads the product list without touching error/loading. Added a test that goes red without the fix (verified by reverting to refresh() and confirming the assertion fails, then restoring it). Checked ProductDetailViewModel (this file's sibling) for the same "set an error, then immediately clear it via a refresh" shape — none of its failure paths do this, so no further changes were needed there.
detekt's Gradle plugin defaults `source` to src/main + src/test only, so
app/src/androidTest (42 files, this branch's instrumented flow-test safety
net for destructive deletes) was never analysed — verified empirically by
planting a violation and seeing detekt report nothing. Wire `source`
explicitly to include src/androidTest/{java,kotlin} alongside the existing
dirs, then confirmed the fix takes effect via the same plant/detect/remove
cycle.
Turning it on surfaced 3 real findings, all fixed at the code level with no
behavior change: a too-long comment (MoveProductFlowTest), a 3-return
dispatcher rewritten as a single-return `when` (MockWebServerRule), and a
9-param test helper's params grouped into a data class, detekt's own
sanctioned fix for that shape (EditableRowTest). No @Suppress, no new
baseline entries, no detekt.yml rule tuning needed — most test-ergonomics
concerns (magic numbers, many small functions) turn out to already be
excluded for androidTest by detekt's own default config.
Also hand-removed 3 stale detekt-baseline.xml entries referencing
AppDrawer.kt, deleted before this branch.
…egression)
AllStoragesScreen's collapsible household header (Task 8) wrapped its clickable
Row in Modifier.clearAndSetSemantics { contentDescription = toggleCollapsedCd },
which discards every descendant's semantics — including Text(entry.name), the
only place a household's name is rendered on this screen. TalkBack lost the
name entirely, and every hasText("Home")/onNodeWithText("Home") flow-test
lookup on this screen started timing out, including tests this branch never
touched (they navigate by tapping through the household name). Replaced with
Modifier.clickable(role = Role.Button, onClickLabel = toggleCollapsedCd) so
the name stays readable while the collapse/expand action is still announced
distinctly. Added HouseholdHeaderSemanticsTest as a source-scan regression
guard (no Robolectric/compose-ui-test on this module's JVM classpath).
Also fixes two instrumented flow tests that were authored/rewritten this
branch and never executed (local emulator is wedged):
- DeleteShelfFlowTest was missing a re-registered mock route for the extra
GET .../shelves that ShelvesViewModel.requestDelete() (new this branch)
fires before opening the strategy dialog — it 500'd and the dialog never
opened.
- LeaveHouseholdFlowTest's navigation to the new HouseholdEditScreen was the
only cross-screen click in the file without the settle delay every other
flow test pairs with performClick(), and its dialog wait was the only 3s
budget in a file where every other wait is 5s — brought both in line with
the rest of the suite as a defensive fix (root cause not fully confirmed;
see .superpowers/sdd/fix-instrumented-regression.md for the full trace).
These 8 were authored blind — the emulator was wedged when they were written, so they had only ever been compiled, never once executed. All 8 failures were test bugs. No app bug; no assertion weakened. Three root causes, all in how the tests drive MockWebServerRule: 1. MockWebServerRule dispatches on PATH PREFIX and is method-blind, so DELETE /households/1/locations/10/shelves/100 prefix-matches the route registered for GET .../shelves and eats the response queued for confirmDelete()'s post-delete re-list. That GET then gets the fallback 500, launchLoading swallows it into state.error, state.shelves is never updated, and the deleted shelf stays on screen. Fixed by registering a dedicated longer route for the DELETE so it wins the longest-prefix match instead of cannibalising the GET queue. 2. The GET queues were under-counted. Each flow fires more GET .../shelves than was enqueued (load, requestDelete's pre-plan refresh, confirmDelete's post-delete re-list, undoDelete's re-list); a per-path queue is FIFO across every caller, so one missing response serves every later response one position out of step and the post-delete re-list returns a stale fixture still containing the deleted shelf. 3. HierarchyStore.refresh()'s background walk was unaccounted for. It walks the full tree under a single runCatching, so one unmocked request aborts the whole refresh before entries is ever published — and the symptom lands on an unrelated screen. DeleteLocationFlowTest needed the shelves route for its shelf_count=3 fixture; LeaveHouseholdFlowTest needed /households/2/locations, which was otherwise prefix-matching /households and stealing from its queue. Full instrumented suite: 57/57 passing. testDebugUnitTest, ktlintCheck and detekt all green. No new @Suppress, no detekt baseline regeneration.
HouseholdEditScreen called hiltViewModel() without scoping, so it silently constructed a SECOND HouseholdsViewModel instead of sharing the one HouseholdsScreen already owns. That gave the edit screen its own untracked background refresh, and an edit made on one screen was invisible to the other. Now scoped to the shared back-stack entry. Found while chasing a CI-only instrumented failure; it is a real product bug, not a test artifact. Also adds stable per-row testTags to the editable shelf/location rows. The rows had no test handle, so tests had to target them by their name text and click the node's geometric CENTRE -- which on a narrower screen lands on the trailing rename icon rather than the checkbox. A row deserves a handle that does not move with the layout.
All 8 were TEST bugs, confirmed by dumping the semantics tree from the real CI
run's logcat artifact rather than guessing -- they were never reproducible
locally (API 34/google_apis/pixel_6 all passed).
- 7 tests: performClick() on an EditableRow taps the row's geometric centre,
which on CI's narrower viewport lands on the trailing rename icon, not the
checkbox. The click succeeded, selected nothing, and the failure only
surfaced several assertions later as 'Delete (1) not found' -- because
toggleSelection() is a silent no-op for a shelf it cannot resolve. Now click
the row's name area, the same way the product rows already did for exactly
this reason.
- 1 test: the Leave button sits below the fold in a scrollable column, so on
CI's shorter viewport the tap hit nothing at all. Now scrolls to it first.
No assertion was weakened, softened or removed: these tests are the branch's
proof that a destructive delete cannot happen without the strategy dialog, that
one gesture shares one deletion_batch_id, and that Undo restores a shelf with
its products.
First tester build carrying the storage-architecture editing work: safe/undoable hierarchy deletes, edit mode on every list, households moved into More, collapsible household groups. Talks to the v0.1.11 backend now live in prod.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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.
Storage-architecture editing — safe hierarchy editing (v0.1.9)
Client half of the storage-editing feature. Pairs with inventory-laravel v0.1.11 (live in prod).
What changed
deletion_batch_id; an Undo snackbar restores the whole gesture — including "keep the products".Backward / forward compat
The DTOs omit null keys correctly (an earlier Critical:
explicitNullswas encodingstrategy:nulland 422ing every delete but "move").UpdateHouseholdRequestkeeps its deliberate asymmetry (name omitted-when-null = "don't touch"; colour/icon explicit-null = "clear"). Both pinned by byte-level tests.Testing
259 JVM unit tests + 57/57 instrumented green on CI. Whole-branch review passed after fixes (it caught a guaranteed reorder crash, a wire-format 422, a dead-end Scan tab, and a semantics regression that dropped the household name from the accessibility tree). Several Criticals survived a green suite and were only caught by mutation testing / probing the real server — see the commit history.
Distribution stays debug-only tester prereleases (tag-driven).