fix(persistence): suppress transient root cwd from polluting recent_folders#229
Conversation
…olders
Issue: On every terminal spawn the shell briefly reports `/` as its cwd
before startup scripts cd to the target directory. The per-frame cwd
tracker picked this up and called appendRecentFolder("/"), so `/`
accumulated a high visit count and floated to the top of the Cmd+O overlay.
Solution: Added cwd_settled flag to SessionState, set to true once
updateCwd observes any non-root path. The appendRecentFolder call is gated
on that flag, so the transient slash is never recorded. Manual cd / still
works because cwd_settled is already true by then. A migration pass strips
any pre-existing "/" entry from recent_folders on first load.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5c0f977f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/config.zig
Outdated
| } | ||
|
|
||
| // V4 migration: strip spurious "/" entries added by transient shell startup cwd | ||
| persistence.removeRecentFolder(allocator, "/"); |
There was a problem hiding this comment.
Keep
/ in recent folders when it is intentionally used
In the V3 load path, Persistence.load now unconditionally calls removeRecentFolder("/"), so every app startup deletes the root entry even if the user explicitly visited / and saved it in persistence.toml. This turns the migration into a permanent filter and causes / to disappear again after each restart, which contradicts the intended behavior that explicit cd / should be tracked normally once startup settles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed both removeRecentFolder("/") calls from the load path. cwd_settled on SessionState already prevents the transient startup / from being recorded, so the load-time strip was redundant. A real one-time migration would need version tracking in the TOML — that's out of scope here. The removeRecentFolder helper and its tests stay, but since it's no longer called on load, an explicit cd / now survives restarts.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the root directory / was polluting the recent_folders list due to shells briefly reporting / as their working directory during startup before executing startup scripts that cd to the intended target directory.
Changes:
- Adds a
cwd_settledflag toSessionStatethat prevents transient root directory visits from being recorded until the shell has settled into a non-root directory - Implements a V4 persistence migration that removes spurious
/entries from existingrecent_foldersdata on first load after upgrading - Updates documentation to note the migration behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/session/state.zig | Adds cwd_settled flag that starts false and becomes true when updateCwd observes a non-root path; flag is reset when session despawns |
| src/app/runtime.zig | Gates appendRecentFolder call on both directory change detection and cwd_settled flag being true |
| src/config.zig | Adds removeRecentFolder helper and calls it during V3 and V2 persistence migrations to strip / entries; includes tests for migration and removal logic |
| docs/configuration.md | Documents the V4 migration that removes spurious / entries from recent_folders |
Comments suppressed due to low confidence (1)
src/config.zig:1107
- The test name "Persistence.appendRecentFolder skipping logic: removeRecentFolder leaves other entries intact" is misleading. This test doesn't actually test "appendRecentFolder skipping logic" - there is no skipping logic in appendRecentFolder. The test only verifies that removeRecentFolder correctly removes the "/" entry while leaving other entries intact. Consider renaming to something like "Persistence.removeRecentFolder leaves other entries intact" for clarity.
test "Persistence.appendRecentFolder skipping logic: removeRecentFolder leaves other entries intact" {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!self.cwd_settled and !std.mem.eql(u8, new_path, "/")) { | ||
| self.cwd_settled = true; | ||
| } |
There was a problem hiding this comment.
There is no automated test coverage for the new cwd_settled flag behavior. While the PR includes tests for the migration logic in config.zig, there are no tests verifying that:
cwd_settledstarts as false when a session is createdcwd_settledbecomes true when updateCwd observes a non-root path- A settled session allows "/" to be tracked in recent_folders after an explicit
cd / - The flag is properly reset when a session despawns
Given that updateCwd is macOS-only (line 443), cross-platform testing may be challenging, but consider adding at least basic unit tests for the flag logic or documenting why automated tests are not feasible.
There was a problem hiding this comment.
cwd_settled is set inside updateCwd, which returns early if !spawned or dead and then calls into the macOS-only getCwd syscall. There's no injection point for a mock cwd, so a unit test would need either OS-level mocking or a live shell process — both out of scope for the existing test style. The initial value (false) and the reset on despawn are plain struct field defaults, visible directly in the struct definition.
Issue: removeRecentFolder("/") was called unconditionally in Persistence.load,
so any "/" entry the user added via explicit cd / would be deleted again on
the next restart.
Solution: Remove the call from the load path. The cwd_settled flag already
prevents the transient startup "/" from being recorded, so no load-time
cleanup is needed. A safe one-time migration would require version tracking
in the TOML, which is out of scope. The removeRecentFolder helper and its
tests are kept for completeness.
Addresses: #229 (comment)
On every terminal spawn, the shell process briefly reports
/as its working directory before startup scripts runcdto the intended target. The per-frame cwd tracker was picking up this transient state and callingappendRecentFolder("/"), so/accumulated a high visit count and floated to the top of the Cmd+O overlay for users who never intentionally navigated there.The fix adds a
cwd_settledflag toSessionState. It startsfalseand flips totruethe first timeupdateCwdobserves a non-root path. TheappendRecentFoldercall in the per-frame loop is gated on this flag, so the transient slash is never recorded. Once a session has moved past root, all subsequent cwd changes — including an explicitcd /— are tracked normally. The flag resets when a session despawns, so a restarted terminal gets a fresh settling cycle. A one-time migration strips any existing"/"entry fromrecent_folderson the first load after upgrading.Closes #228
Test plan
~/.config/architect/persistence.toml), open a new terminal, then open the Cmd+O overlay —/should not appear in the list.cd /, then open Cmd+O —/should appear and increment normally on repeated visits."/" = 5under[recent_folders]inpersistence.toml, restart Architect, and confirm/is gone from the overlay.