Keep saves on the loaded profile when a profile-switch reload is declined - #578
Conversation
Code review — correctness + adversarial, findings appliedTwo reviewers, independent contexts. Both converged on the same two defects, and the adversarial pass then broke the design decision itself. Both reviewers found the same two regressionsThe change made
The adversarial pass broke the remaining design
Correct, and decisive. On a reload that lands, the document is replaced and the bootstrap sets So the gated repoint is gone entirely, and with it the boolean The spec that pinned the old behaviour ( Applied
Three tests added: the write path is never repointed, the reset targets the loaded slot, and the rename moves it (including the Not applied — filed as #582The deferred switch is invisible: the list marks the old profile active, the Retry toast is displaced by the app's own WebSocket-retry toasts within one backoff interval, the row that would cancel has no Switch button, and |
a5a880a to
6265848
Compare
Code review — 5 personas, findings appliedReviewers: correctness, testing, maintainability, project-standards, reliability. Fresh contexts, isolated worktree. The premise was checked, not assumedThe fix is a deletion, so it is only safe if the bootstrap really does establish the write path. The correctness reviewer traced it end to end: The testing reviewer mutation-checked the rest: reverting Verdict on that deleted test ( Four of five reviewers found the same regressionNot repointing the write path makes "loaded slot" and "persisted name" diverge for the first time, and Fixed, with a test using the divergent Two more consumers read the wrong name
Comments trimmed
Not actionedA deferred switch is still invisible and uncancellable — the profile list flags the old profile active while the device will boot into the new one, and the toast never names the profile. That is #582 and a design question about whether a pending switch should exist at all.
Gate on the head commit: 2042 tests pass, |
4cd493f to
cda9154
Compare
…ined Skip reloads to load a profile, and ReloadService refuses that reload when the server does not answer — during a Signal K restart, for example. setActiveProfile had already repointed the storage write path to the newly chosen profile by then, so the app carried on showing the old profile while every later save went to the new profile's slot, replacing its dashboards with the old one's content. The write path is now left alone. The bootstrap sets it from the persisted name on a reload that lands, so it always names the configuration that is actually in memory. The persisted name is still written, so a switch whose reload does not happen is deferred rather than lost. The two names can now differ, so the consumers that meant "the profile on screen" follow the loaded slot: resetSettings replaces the configuration the user is looking at, and renameProfile moves the write path with the slot it renames.
Not repointing the write path makes "loaded slot" and "persisted name" diverge for the first time, and three consumers read the wrong one of the pair. deleteProfile guarded only the loaded slot, so after a declined reload the profile the device is about to boot into was an ordinary deletable row. Deleting it left the persisted name on a slot that no longer exists and the next reload landed on the degraded recovery screen; the old eager repoint had covered that case by accident. setRemoteContextDemand keyed by the persisted name while computing the answer from the loaded profile's dashboards. A wrong key does not fail open, so the next boot into that profile subscribes to no AIS targets. renameProfile moved the write path after the old slot's removal had drained. patchConfig bakes the slot name in at enqueue time, so a save queued in that window targeted a slot the queue then deleted ahead of it.
cda9154 to
fd1b373
Compare
1 participant
Why
setActiveProfilerepointedstorage.sharedConfigNamebefore callingReloadService.reload(), which legitimately declines to navigate when the server probe fails or returns 5xx — a Traefik 502 during a Signal K restart, or a timeout over 4s.The app then kept running with the previous profile's config in memory while every later save PATCHed the newly selected slot, because
StorageService.patchConfigbuilds each path as/${sharedConfigName}/.... Switch profile during a brief SK restart → reload declined → SK returns → user edits a dashboard → the target profile is replaced by the old profile's content.What
ReloadService.reload()now reports whether it committed the navigation, andsetActiveProfilerepoints the storage write path only on a commit.The persisted name is still written before the reload either way, deliberately: the navigation does not return, so it has to be in place beforehand, and leaving it there means a declined switch is deferred rather than lost — the Retry on the "server is still unreachable" toast, or any later reload, boots onto the profile the user picked. Reverting it instead would have made Retry land back on the old profile, and would have pointed the device at a deleted slot on the
renameProfilepath.setActiveProfileis async now; its two callers inProfileServiceawait it, which also keeps the switch inside themutationInFlightguard for its full duration.Tests
Two added to
settings.service.spec.ts: a declined reload leaves the write path on the loaded profile (fails onmain), and it still persists the chosen name so a later boot lands on it. The existingReloadServicefake now models a committed reload rather than resolvingundefined.Full suite: 2031 passing.
Fixes #520