Repository navigation
fix(persistence): preserve projectGroupOrder on restart for flat folder-scan groups - #23261
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe loading adaptation now assigns a project group order only when a repo’s group ID changes. Tests verify that existing orders survive reload and that migration assigns orders to moved repos while preserving the group assignments and orders of other repos and the folder repo. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains with preserving folder-scan project order across restarts. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change preserves custom ordering without adding a new access path or changing permissions. Remaining uncertainty concerns coverage of interrupted migrations, not an identified security defect. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Guard narrowed in
adaptFlatFolderScanProjectGroups()— assignment/re-indexing now happens only whenrepo.projectGroupId !== group.id, so repos already in a flat folder-scan root group keep their manualprojectGroupOrderinstead of being renumbered0,1,2…on every load. - New tests in
src/main/persistence-folder-scan-project-groups.test.tspin both the preservation path and the migration path (root orders kept, child repos indexed from 0).
The change is correct and minimal. The candidate set is filtered to repo.projectGroupId === rootGroup.id, and getGroupForRepo returns either the root group or a freshly created child, so the new condition is exactly "migrating into a new child group". I also verified the tests are not tautological: reverting the source hunk to the old guard makes both fail (r1 5 → 0, api 10 → 0). Downstream order consumers (getEffectiveProjectGroupManualRank, getNextProjectGroupOrder, the renderer project-group-sections.ts) tolerate sparse and undefined orders via the documented global-repo-order fallback, so nothing depends on contiguity.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…er-scan groups When repositories inside a flat folder-scan project group (without nested sub-groups) were manually reordered in the sidebar, adaptFlatFolderScanProjectGroups re-assigned sequential orders based on raw array index on every launch because childGroupIds.has(rootGroup.id) evaluated to false. Only re-assign and re-index projectGroupOrder when a repository actually migrates into a newly created child group. Repositories staying in the group retain their manual ordering.
f27f698 to
cf77dfa
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Expanded flat-group restart coverage — replaced the single reload check with a unit-level test (
leaves unchanged group membership and manual order clean on load) plus anit.each([false, true])test that closes and reopens a real Store and asserts both the persisted JSON and the reloaded repos keep the saved[1, 0]order, with and without a non-Git folder repo. - Hardened the migration test's cross-platform paths — switched from hard-coded
/workspace/...tojoin(parentPath, …)and wrapped the expected child-groupparentPathinnormalizeRuntimePathSeparators, matching the forward-slash normalization the resolver already applies. - Added a folder-repo assertion to the migration path — a
kind: 'folder'repo in the root group now asserts it stays in the root group with itsprojectGroupOrderuntouched, pinning that folder repos are excluded from migration candidates.
The source change is unchanged since the prior review. I re-ran the focused suite on head and confirmed the expanded tests still fail when the source hunk is reverted to the old guard (adaptFlatFolderScanProjectGroups() returns true, r1 1→0 / r2 0→1, api 10→0), so they pin the bug rather than passing tautologically. Child-group parentPath normalization was verified against resolveFolderPath → trimPathSeparators, so the Windows path expectation is sound.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…/sweep-pr23261-folder-group-order
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since Pullfrog's last review (cf77dfaa) is a single merge commit (93c2e97) that brings current main (f2257ffa) into the branch. The PR's two feature files are byte-identical to the previously reviewed state — the diff against the merge-base remains the one guard hunk in adaptFlatFolderScanProjectGroups() plus the test file.
- Merged
maininto the branch —93c2e97syncs 188 upstream files into the branch, but leaves the feature patch untouched; the load-order guard and its tests are unchanged from thecf77dfaareview. - Re-verified the focused suite on the merged head —
pnpm test src/main/persistence-folder-scan-project-groups.test.tsstill passes all 4 tests, so the main sync introduced no semantic conflict with the migration guard or its assertions.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Final merge review at Retain saved project ranks when the project remains in its folder-scan group. Merge risk and retained limits: Low: persisted ordering and nested-group migration. Contributor authors and existing co-author credit will be preserved explicitly in the squash commit. |
ELI5
If you drag beta above alpha in a folder-scan project group, restarting Orca should keep beta first.
What Changed
Before, loading a flat folder-scan group reset its projects to the order they were originally added. After, saved manual order survives restart. The existing load adapter now assigns a new order only when a project actually moves into a child group; it keeps using the existing nested-group resolver.
The original fix and initial tests are by @lurunzi. This update preserves their commit author, rebases the contribution onto current main, and strengthens the tests around closing and reopening the real Store.
Why
The adapter also runs for groups that need no migration. Limiting its order assignment to actual moves fixes the reset while preserving migration for older nested folder imports. Skipping the whole adapter would lose that migration.
Linked Issue
Related context: #22285, last updated September 23 and treated as stale context. The current production bug was independently reproduced from main
adf447d958b2before applying this recent contributor PR.Visual Proof
Both runs load the same saved order: alpha has rank 1 and beta has rank 0. Screenshots come from hidden Electron renderers with disposable homes and profiles; their rendered order was also asserted through Playwright CDP.
Before restart loading resets the order:
After the fix, restart loading keeps the saved order:
Testing
On macOS, before production edits the real load operation changed saved
[1,0]to[0,1]while both projects stayed in the same group. A real SQLite Store close/restart failed the same way; the nested-migration control passed. The identical probe passes all three checks after the fix. Exact failing command, output, and probe source.The committed regression checks unchanged membership and order, a closed Store restart with and without a non-Git folder, real child-group migration, preserved root/folder order, and another restart after migration. To run the focused coverage:
ORCA_BACKGROUND_LAUNCH=1 pnpm test src/main/persistence-folder-scan-project-groups.test.ts src/main/persistence-repo-lifecycle.test.ts src/main/persistence-worktree-meta-and-folder-workspaces.test.ts src/main/project-groups/nested-repo-import.test.ts ORCA_BACKGROUND_LAUNCH=1 pnpm tc:node ORCA_BACKGROUND_LAUNCH=1 pnpm run check:code-quality:changedAll 72 scoped product tests and 42 CI contract checks pass after integrating current main. Node typecheck, formatting, and changed-code quality pass; the previously verified feature bundle and sidebar proof remain unchanged. The hidden sidebar shows alpha then beta before, and beta then alpha after. The required repository guard and final verification passed on published head
93c2e97545cf70b19b074f9b70c83e4ff0125887. Linux and Windows apps were not launched locally.AI Disclosure
Original contribution assisted by Antigravity (Google DeepMind). Rebase, expanded restart tests, and verification assisted by Codex.
Review
Independent read-only review found a Windows-only test path-separator mismatch, which was corrected using the existing path utility. Re-review of the final source and tests is clean. The production change remains the contributor's original relocation guard.
Pullfrog's current-head review at
93c2e97545creports no new issues. After merging current main, both feature files and the entire feature patch still match the independent clean review byte for byte. Its current review confirms the unchanged feature files and passing restart tests after the main merge; its prior review also checked failures with the old guard and the Windows path expectation.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Merge risk: low; the change is limited to load-time order assignment and retains the existing migration and save path. Negative user-facing tradeoffs: none identified. Scope is local persistence, with no remote/SSH repair or wire-contract change. The branch includes main’s existing correction to two package-list contract assertions (#24785), which caused the prior CI failure; that correction adds no files to this feature diff.
Checklist
N/Awith reason