fix(projects): prevent duplicate folders and preserve project data - #764
SpookySandwich wants to merge 5 commits into
Conversation
SDSLeon
left a comment
There was a problem hiding this comment.
Deep review — Request changes
Scope: PR #764 @ 693a44c, 7 files (5 source + 2 screenshots). Verdict: 6 Important — must fix before merge.
Must fix
src/renderer/state/appStore.ts:60— deduping on hydrate emits a project deletion against the pre-merge snapshot;dbSyncChangesdeletes the project row before upserting the rehomed threads, andON DELETE CASCADEdestroys those threads' runtime items. Threads survive with empty transcripts, irreversibly, for exactly the users this PR targets.src/renderer/app.tsx:455— the broadcast path remaps threads only: the current view, draft panes, draft contents and experiments keep pointing at the dropped id, so the user is bounced to Home, loses unsent draft text, and loses experiment grouping.src/renderer/state/slices/projectSlice.ts:88— the early return dropsnameOverride/workspaceIdand gives callers no create-vs-reuse signal: re-adding a folder wipes its custom setup script and actions, and re-adding from another workspace is a silent no-op.src/shared/projectIdentity.ts:15— identity is enforced only in the renderer;projectCommands.register()(MCPcreate_project, mobile/remote clients) still inserts duplicates that the renderer then deletes, socreate_projectreturns an id that stops existing. Headless backend hosts never dedupe at all.src/shared/projectIdentity.ts:27— the synthetic Home row carries the home directory as its location and has no exemption, so a project at~deduplicates Home away (or gets swallowed by it), orphaning every Home thread.src/shared/projectIdentity.ts:38-45—normalizePathcollapses the UNC//prefix (false merge of two distinct folders) and, becausekindfollows the host platform, leaves macOS posix paths case-sensitive — the duplicate this PR targets still occurs on macOS.
Verified good
- The identity concept is right: location, not name, as the identity boundary, with
remoteServerIdincluded so equal paths on different hosts stay distinct — andremoteProjection.ts:50-58does set bothremoteServerIdfields synchronously, so remote projects do not collapse. - Remote mirror rows are excluded from desktop persistence (
appStorePersistence.ts:14), so the dedupe cannot delete a mirrored remote project. mergecorrectly remapsviewviaremapProjectViewand rewritesthread.projectId— the hydration path's in-memory result is consistent (the gaps are at the DB boundary and in the sibling broadcast path).- Targeted suites pass on the PR tree:
vitest run src/renderer/state/appStore.test.ts src/shared/projectIdentity.test.ts→ 94 passed;tsc --noEmit→ 0 errors. src/sharedis the right home for the module — it is consumed by both renderer and main, with no provider branching.
Rejected after validation
- Persist
versionnot bumped to 6 for the dedupe repair:mergealready performs unconditional repair-on-read (normalizeStoredThreadStatus) and the on-disk shape is unchanged, and the PR adds a hydration regression test from the old shape. Moving the repair intomigrateis still the better shape and is recommended in theappStore.ts:60comment, but it is not a standalone blocker. projectIdentityKeyreadsproject.remoteServerIdrather thanlocation.remoteServerId: both fields are written together at every current write site; no live divergence exists.
Nit-level notes omitted by policy — Important only.
|
@SDSLeon, this follow-up is ready for another look. Addressed all six findings in 0427baf:
Two independent reviews also found collisions in notes, PR watches, and pending composer seeds. Those are fixed and covered by regressions. When two merged project records have different recorded PR fixes, both conversations remain and automation pauses until a mode is selected again. Validation: 394 tests across 17 targeted suites pass, along with type checking and full lint. The final isolated Electron smoke run passed its automated scenarios and 10 mocked integration gates with zero console/runtime errors. All touched files pass formatting; the full formatter still reports the unchanged CLAUDE.md issue. The PR description now includes refreshed screenshots pinned to this commit. The test instance is stopped. The new CI run is currently marked action_required with no jobs started. |
SDSLeon
left a comment
There was a problem hiding this comment.
Deep review — Request changes
Scope: PR #764 @ 6cec306 (fix commit 0427bafc + merge of master), 55 files. Verdict: 1 Important — must fix before merge.
Must fix
src/main/db/projectDeduplication.ts:13— v42 picks canonical bysort_order(newest-first fromaddProjectprepend) and deletes the other project row without mergingscripts/ MCP / search / git-account fields. Upgrade of the exact duplicate population this PR targets drops the original project's custom settings while rehoming threads onto the empty re-add. See inline.
Verified good
- Hydrate/sync cascade (
#1): v42 +rehomeProjectReferencesbeforeDELETE,dbSyncAllrehomes before dropping missing project ids;thread_runtime_itemsasserted inmigrations.test.tsandsync.test.ts. - Live remaps (
#2):applyProjectStateSnapshotrewrites view, draft panes, mounted composer contents, experiments (remap then reconcile), and panel contexts. - Reuse (
#3):addProjectWithResultreturnscreated, applies name/workspace, gatesautoDetectSetupScript/project.added, toasts the localized already-registered message. - Authoritative identity (
#4):register()reuses byprojectIdentityKey; relocate 409s on conflict; MCP returnscreated: false. - Home (
#5):projectIdentityKeyshort-circuitsHOME_PROJECT_ID; Home is never a duplicate or canonical target. - Path keys (
#6): UNC prefix preserved,C:≠C:\, macOS POSIX opt-in, remotes do not inherit the client's case policy, WSL distro fold + Linux path case.
Rejected after validation
- Prior 6 Important findings on
693a44c— all fixed on0427bafc. - Optional remote
createdwithout a protocol bump — additive field; local MCP always sets it.
Nit-level notes omitted by policy — Important only.
| .prepare("SELECT * FROM projects ORDER BY sort_order ASC, rowid ASC") | ||
| .all() as ProjectRow[]; | ||
| const { duplicateIds } = dedupeProjects(rows.map(rowToProject), { | ||
| caseInsensitivePosix: process.platform === "darwin", | ||
| }); | ||
| rehomeProjectReferences(sqlite, duplicateIds); | ||
| const deleteProject = sqlite.prepare("DELETE FROM projects WHERE id = ?"); | ||
| for (const duplicateId of duplicateIds.keys()) deleteProject.run(duplicateId); |
There was a problem hiding this comment.
Important — data loss — migration keeps the newest duplicate and deletes the original project's settings.
What's wrong: repairDuplicateProjects chooses canonical as the first row in ORDER BY sort_order ASC (src/main/db/projectDeduplication.ts:13-17), then deletes every other row (:19-20) after rehoming threads/notes/watches. It never copies scripts, mcp_servers, search_settings, gh_account, worktree_location, icon, or last_draft_config onto the survivor (notes are merged; project-row fields are not).
addProject prepends (projectSlice.ts:125), and dbSyncAll writes sort_order from array index. A user who re-added /repo therefore has: newest nearly-empty duplicate at sort 0 (auto-detected setup script, actions: []) and the original configured project at sort ≥1 (custom setup script and actions, most threads). v42 makes the empty new row canonical and drops the original row. Threads survive on the new id; custom actions/MCP/search/git-account settings do not.
Concrete: P_old created 2024 with 4 actions + 200 threads, P_new created 2026 by re-adding the folder, sort_order 0. First launch after this change → chats reappear under P_new with an empty Actions list, permanently.
Why guards don't stop it: mergeNotes only covers project_notes. Tests seed the keeper at sort 0 (migrations.test.ts:126-127), which is the opposite of production prepend order.
Fix: within each identity group, pick canonical by earliest created_at (stable original), and before delete copy non-empty project settings from discarded rows onto canonical (same idea as mergeNotes). Do not use sidebar sort_order as the keeper.
Regression check: seed two projects at /repo — older row with custom scripts.actions and 3 runtime items, newer row with sort_order = 0 and default/empty scripts; run v42; assert the surviving row still has the custom actions and the 3 runtime items.
There was a problem hiding this comment.
Fixed in 16d835a. The repair now keeps the earliest-created project, regardless of sidebar order, and merges settings before deleting the other rows. The original project's non-empty choices win conflicts; missing settings, distinct actions/MCP entries, and search exclusions are recovered from the other copies. Explicit false values are preserved, and account/model/worktree configurations stay together.
The upgrade regression now uses production ordering: a newer default project at sort 0, an older project with four custom actions at sort 1, and three saved runtime items. It verifies the original ID, actions, recovered settings, and all three messages survive. Hydration and full-state sync use the same repair policy.
GPT-6 Astra's adversarial review also caught a desktop reconciliation gap and a queued-save race. Main now sends repaired projects and recovered conversations back to the desktop together, and protects recovered conversations until a later save acknowledges them. Its final check passed the original race reproducer and project-deletion cases, with no remaining blockers in that scope.
Validation: 414 targeted tests across 21 suites passed across the final runs; the app's 34-test suite passed separately after hydration timeouts while Electron was building. Type checking and full lint pass. The final isolated Electron smoke run passed its automated checks and five mocked integration gates with zero console/runtime errors, then shut down cleanly. All touched files pass formatting; the unchanged CLAUDE.md remains the only full-format warning. Ready for another look.
Summary
Opening the same folder again now reuses its existing project. Different folders can still share a name, and projects on different machines stay separate.
Reopening a project keeps its custom setup script, actions, and settings. An explicit name or workspace selection is applied to the existing project. Desktop, remote, and MCP creation use the same identity rule, and moving a project to an already registered folder is rejected.
Motivation
Each folder used to receive a new project ID whenever it was added. Preventing new duplicates also needs a safe upgrade for profiles that already have them.
The new database migration moves thread ownership before deleting duplicate rows, preserving conversation history. It also merges notes and to-do lists and repairs saved views, pane layouts, and experiments. Live project updates preserve unsent rich text and attachments, including two populated drafts in split panes. Home remains a separate synthetic project.
The repair keeps the oldest project, regardless of its sidebar position. Its non-empty settings win conflicts; missing settings and distinct actions, MCP entries, and search exclusions are recovered from the other copies. The open app receives the repaired projects and conversations together, and queued saves preserve recovered conversations until the app acknowledges them.
If duplicate projects have watches for the same PR, the repair keeps the watch associated with a recorded fix, including its settings and progress. If both watches record different fixes, both conversations survive and automation pauses until the user chooses a mode again.
Path comparisons cover Windows separators and drive roots, UNC paths, macOS casing, and WSL paths. A desktop's casing rules are not applied to a different remote machine.
Testing
CLAUDE.mdissue.Screenshots
Multiple projects remain available in the project switcher:
Reopening the same folder shows an “already registered” notice and keeps one Kurdish-to-English project. This check supplied a fixture folder to the production registration action, with changed casing, separators, and a trailing slash; the project IDs stayed unchanged in memory and in SQLite: