Make "Restart setup" button work + make choosing a configuration idempotent#478
Merged
Conversation
Juanpe Bolívar (arximboldi)
requested review from
cooper (czxtm) and
Scott McMaster (scottmcmaster)
Juanpe Bolívar (arximboldi)
changed the title
Contributor
🎨 Storybook previewUpdated for 2a4dbd9 🧭 Story changesCompared to ✏️ Changed stories (51)
|
Contributor
📋 PR Overview
🔬 Coverage
|
Scott McMaster (scottmcmaster)
approved these changes
Jul 6, 2026
Scott McMaster (scottmcmaster)
left a comment
Scott McMaster (scottmcmaster)
left a comment
Collaborator
There was a problem hiding this comment.
I wonder if it would be simpler if Onboarding had its very own in-memory data model and didn't start touching app state until you hit the big button at the end.
Contributor
Author
|
Scott McMaster (@scottmcmaster) I like your approach and was thinking in similar lines, but didn't want to affect too much the current structure. I'll try that out in a later PR. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
…dirs Record the clone/scaffold ROOT in a new GlobalPreferences field, onboarding_provisional_config_dir, whenever the app materializes the config directory (GitHub/zip import, new-dir scaffold). User-selected pre-existing directories never get the marker, and the first successful apply clears it: from that point the directory is the user's and restart must never delete it. The marker intentionally stores the materialization root rather than the (possibly nested) flake subdirectory, so releasing ownership can remove the whole clone. It is not exposed through UiPrefsUpdate — backend code paths only, like onboarding_last_build_at. Groundwork for an idempotent "Restart setup" and safe re-imports.
Resetting clears every durable fact the step machine derives progress from — config_dir, repo_root, host_attr, the three onboarding stamps — plus the legacy KV configDir key, which get_config_dir_if_set would otherwise fall back to and silently resurrect the old selection. When the active config directory is still owned by onboarding (provisional marker matches and no build has been applied from it), its contents are deleted too, so the next import finds an empty target. Guard rails: resets are refused while a rebuild stream is running, a stale marker that doesn't contain the active config dir never deletes anything, and the canonical /etc/nix-darwin is emptied in place rather than removed. Also adds watcher::stop_watching() — with no config dir there is nothing to poll, and polling a deleted one would emit git_state_error every tick — and clears the git/evolve cells the way a fresh launch starts out.
…hange Two mid-onboarding invariants when the config dir is replaced (before any successful apply): - The "scan this Mac" stamp described the outgoing repo, so it is cleared and the step machine re-surfaces the customizations step. A previously materialized provisional tree that the new selection does not live in is deleted rather than orphaned. - Import and scaffold targets must be empty, which made retrying an import fail on its own leftovers — the original non-idempotence. When the blocking tree is onboarding's own provisional materialization it is now released for reuse, so re-importing the same repo (after a restart or "Change source") just works. Post-onboarding config-dir changes are untouched: once a build has been applied, neither path rewinds onboarding or deletes anything.
Dead since e6a6b40 moved step progression into derived durable facts: the button only reset the transient UI store, which the step machine does not read. It now confirms with a dialog and calls the new onboarding.reset command; the preferences-changed event re-derives the flow back to the config-dir step with no navigation code. An import parked on the flake-dir chooser exists only on disk with no backend record, so its clone dir is mirrored into the transient store and discarded before the reset. The backend's refusal to reset while a build is running surfaces as a toast, and the restart telemetry event fires only after the reset succeeds instead of before a no-op.
…_completed computeOnboardingStep is what every reset/cascade behavior in this branch relies on, and it was untested; the new cases pin the gate order, the strictly-first-unsatisfied rule, the deferred-inference build gate, and the exact post-reset derivation. Also captures the onboarding_completed telemetry event when the celebration raises — it was declared in the event union but never emitted anywhere.
The dialog content carried button-style classes: bg-transparent made it translucent over whatever was behind it, font-bold applied to all the copy, and hover:-translate-y-1 overrode the centering translate-y so the whole dialog jumped on hover. Drop them and let the base DialogContent's solid background, border, and shadow apply; footer buttons use standard variants (the hardcoded text-white cancel label would have been invisible on the light background). Adds an optional bolded title so the question reads bold and the explanation doesn't; the restart-setup dialog splits its copy that way.
The gates above it (permissions, Nix install) are machine state a reset can't undo; "Restart setup" rewinds to config-dir. A light rule above that step hints at the boundary. The step is named by a RESTART_TARGET_STEP constant in the step registry so the semantics live next to the step definitions.
evolve-flow and drift-review catch up with the ConfirmationDialog restyle (solid background, standard button variants, normal-weight copy). rebuild-overlay-panel was already stale against the committed tree before this branch. Not included: the SaveContinueEditing interaction test in evolve-flow was already failing before this branch — it clicks a /continue editing/i button that no longer exists anywhere in product code.
A NeedsFlakeDirChoice clone is real on-disk state, but its only record lived in frontend component state plus a manually-synced copy in the transient onboarding store — two frontend copies, zero backend record. A remount or app restart mid-choice orphaned the tree, and onboarding_reset could not be self-contained: the frontend had to remember to discard the clone before calling it. Record the parked tree in GlobalPreferences.pending_import_dir when an import returns NeedsFlakeDirChoice; clear it on finalize/discard; discard it (with config_discard_import's safety rules) at the start of any new import and inside onboarding_reset. Import retries after an abandoned choice now find a free target instead of failing on the leftover clone.
The backend now records and discards parked NeedsFlakeDirChoice trees itself, so the transient store field, the manual mirroring in use-import-config, and the two-phase discard in the restart handler all disappear. The flake-dir chooser keeps its component-local view state, which is caller-owned query-result data per the state taxonomy.
…ites The in-memory cache in ipc/preferences.ts only invalidated on frontend setPrefs, but the backend also writes preferences (apply bookkeeping, onboarding reset, settings import) — those writes left the cache serving stale provider/toggle values. The preferences sync module now folds every global_preferences_changed payload into the cache, overwriting only keys the payloads actually carry: the keychain-backed fields the cache exists to protect are untouched, and merging instead of dropping avoids re-hitting the keychain on the next read.
… rule Main (#473) relaxed validate_new_dir_location to allow nested config dirs under home, which the PR merge build combines with this branch — asserting that a nested home path is not wipeable now fails there. The guard test keeps only the invariants that are its own: paths outside home are never wipeable, and the active config is never deleted from under itself. How deep an import target may nest is validate_new_dir_location's contract, tested where it lives.
Juanpe Bolívar (arximboldi)
force-pushed
the
jp/restart-setup
branch
from
5dcdcab to
2a4dbd9
Compare
Contributor
Author
|
There was a rebase issue that prevented it from merging. I fixed it I hope it works now. |
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
2 participants
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.
Summary
This addresses a couple of issues with the onboarding process:
Choosing a configuration is not forgiving. If, for example, you go back to a previous step to choose a different flake to import, it will fail to import the new flake because the target destination is already occupied. This is solved by marking the paths as "pending" until the workflow is done.
The "reset" button in the onboarding workflow was doing nothing. This makes it work again by wiring it up with the backend appropriately. This moves the user back to the first step where they actually need to make a choice ("chooose a configuration", which is why (1) was important at this stage). This is also marked in the UI with a slight separator.
Test Plan
Docs