fix(desktop): keep a nested folder as its own project - #3070
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughWhat problem this solvesThis PR fixes registration of folders nested inside existing Git projects. Selecting Repository roots and linked worktrees continue to share one Git identity. Historical session working directories continue to resolve to the enclosing repository. Source of truthThe PR extends the existing project-location resolution in It does not create a parallel project model or duplicate registration logic. User-selected paths and historical session paths use separate resolution rules because they have different requirements. Scope and complexityThis is the smallest coherent solution. It routes
No code or tests can be removed without weakening behavior or regression coverage. ValidationCoverage includes:
Reported verification includes 20/20 project-catalog and authority tests, 13/13 desktop add/select and new-session tests, and successful project usage recording through The desktop main build was not run because the current Review-relevant risksThe diff changes user-visible project registration, relinking, and touch behavior for folders inside Git worktrees. Material changes in this area require independent human review under repository policy. No public entity declaration changes were identified. No security, licensing, release, or governance effect was identified in the current diff. Implementation children in nested projects may still start at the worktree root. This is pre-existing behavior and is outside this PR. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe project catalog preserves explicitly selected Git subdirectories as separate folder projects. Registration, relinking, and explicit path touching use the updated resolution behavior. Desktop and storage tests cover nested project identities, paths, selection, relinking, and Git setup. ChangesNested project selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change preserves user-selected nested folders as independent projects while retaining repository identity behavior for roots and historical sessions; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DesktopProjectService
participant ProjectCatalog
participant Git
DesktopProjectService->>ProjectCatalog: register selected child directory
ProjectCatalog->>Git: resolve repository context
Git-->>ProjectCatalog: repository root and child path
ProjectCatalog-->>DesktopProjectService: child folder project
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
eab34ea to
6a995f8
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for fixing the distinction between an explicitly selected folder and a historical cwd. Keeping repo/child as its own project is the right direction, but there is one lifecycle gap to address before merge.
[P1] Keep registered project locations authoritative in touch()
After the child project is registered, creating a session for it still fails:
session.create → HostWorkspaceResolver.runWithUsageRecorded() → catalog.touch(projectId, workspace.cwd)
Here, workspace.cwd is the child project’s stored preferredPath. However, touch() runs it through resolveProjectLocation() again, which maps /repo/child back to the enclosing Git root /repo. That path is not one of the child project’s locations, so touch() throws ProjectPathMismatchError and session creation returns operation_conflict.
In other words, the PR currently allows the nested project to be registered and selected, but its primary runtime flow cannot start a session.
I suggest making the registered locations[] authoritative in touch(): canonicalize the supplied path, match it directly against the project’s stored locations, and reject it if no exact match exists. The only production caller passes the stored preferredPath, and touch is not exposed as a protocol operation, so Git-root fallback does not appear necessary here.
A focused regression test should cover either:
catalog.touch(childProject.id, childProject.preferredPath)succeeding after registering the parent and child; or- the full
session.createpath for the nested project.
Two optional simplifications would also keep the contract smaller:
- Keep
resolveUserSelectedProjectLocationmodule-private and test it throughcatalog.register()rather than exporting and directly testing the helper. - Remove the new assertion that
resolveHistoricalPath(child)returns the parent project. That API currently has no production caller, so this PR does not need to freeze that future behavior.
I do not think a broader project-catalog redesign is needed in this PR; fixing touch() at the ownership boundary and adding one production-shaped regression test should complete the lifecycle.
中文对照
感谢修正“用户明确选择的目录”和“历史 cwd”之间的区别。让 repo/child 保持为独立项目的方向是正确的，但合并前还需要补齐一个生命周期缺口。
[P1] touch() 应以项目已经登记的 location 为归属依据
child 项目注册完成后，为它创建 Session 仍然会失败：
session.create → HostWorkspaceResolver.runWithUsageRecorded() → catalog.touch(projectId, workspace.cwd)
此时 workspace.cwd 就是 child 项目已经保存的 preferredPath。但是 touch() 又调用 resolveProjectLocation()，把 /repo/child 重新解析成父 Git 根目录 /repo。父目录不在 child 项目的 locations 中，因此会抛出 ProjectPathMismatchError，最终令 Session 创建返回 operation_conflict。
也就是说，当前 PR 可以注册和选择嵌套项目，但该项目的主要运行流程无法真正创建 Session。
建议让 touch() 直接以已登记的 locations[] 为权威来源：canonicalize 输入路径后，在该项目的 locations 中精确匹配；不存在则拒绝。唯一的生产调用者传入的就是已保存的 preferredPath，而且 touch 也不是对外的 protocol operation，因此这里不需要保留 Git-root fallback。
回归测试可以选择以下任意一条真实 seam：
- 注册父项目和 child 后，验证
catalog.touch(childProject.id, childProject.preferredPath)成功；或 - 完整验证嵌套项目的
session.create。
另外有两个可选简化：
- 将
resolveUserSelectedProjectLocation保持为模块私有，通过catalog.register()测试行为，不扩大公开 API。 - 删除新增的
resolveHistoricalPath(child)必须返回父项目的断言。该接口目前没有生产调用者，本 PR 没有必要冻结其未来契约。
本 PR 不需要扩大成 project catalog 的整体重构；在 touch() 这个自然 owner 处修正归属判断，再补一条命中真实调用链的测试，就足以完整收口。
AI-assisted review disclosure: Codex, Claude, and Kimi K3-256k assisted with tracing the call chain. I verified the finding against the current head and made the final review decision.
Registering a directory inside an existing Git project used to walk up to the worktree root, so adding repo/child silently reopened repo. Keep the exact folder the user picked. Historical session cwds still resolve to the enclosing repository. Fixes apache#2660 Generated-by: Grok
session.create records usage through catalog.touch(projectId, cwd). Resolving that cwd through Git walked a nested folder back to the parent worktree and rejected the child's stored preferredPath. Canonicalize the supplied path and match it exactly against the project's registered locations. Generated-by: Grok
6a995f8 to
9b677f9
Compare
|
Addressed in 9b677f9. P1 Optional cleanups from the review:
Verified:
Generated-by: Grok |
|
Thanks for following up on the earlier review. I rechecked the current head, and the original I found two follow-up items. Neither needs to block this PR, but they seem worth recording: [P2] Preserve the nested Project cwd for worktree children A Project registered at I reproduced this with the actual executor at the current head. The behavior was already reachable through I think a separate follow-up is appropriate. A compact solution would keep the durable binding rooted at the managed worktree while returning a separate [P2] Add coverage for the changed relink paths Both A focused storage test could relink an unrelated Project to Two optional simplifications:
I would keep both newly added tests: the storage test covers the catalog/session lifecycle, while the desktop test uniquely verifies the user-visible selected path. Overall, the Project Catalog change is well-scoped and uses the right ownership boundary. I’m comfortable with this PR moving forward while the worktree behavior is tracked separately. 中文对照感谢继续跟进上一轮 review。我重新检查了当前 head，之前发现的 还有两项值得后续处理。它们都不需要阻塞本 PR，但建议明确记录： [P2] worktree 子会话应保留嵌套 Project 的 cwd 注册在 我使用当前 head 的真实 executor 复现了该行为。它此前已经可以通过 我认为这适合单独跟进。一个比较紧凑的方案是：持久化 binding 仍以受管 worktree 为根，同时额外返回根据原始路径在仓库中的相对位置计算出的 [P2] 为本次修改涉及的 relink 路径补充覆盖
可以在 storage 层增加一条聚焦测试：把一个无关 Project relink 到 另外有两项可选简化：
我建议保留两条新增测试：storage 测试覆盖 catalog/session 生命周期，desktop 测试则独立验证用户最终选中的可见路径。 总体而言，Project Catalog 的改动范围合理，也放在了正确的权威边界上。我认为本 PR 可以继续推进，同时把 worktree 行为作为独立事项跟进。 AI-assisted review disclosure: Codex performed the primary lifecycle trace and executor reproduction; an independent Codex reviewer and Claude performed adversarial review. The final assessment above was checked against the current PR head. |
relink() and relinkWithSessions() now have a nested-folder regression test so the explicit-selection resolver stays on the critical path. The chooser comparison already sees normalized paths, so the extra normalize() calls are gone. Generated-by: Grok
|
Addressed the in-scope P2 follow-ups in 66778b4. Relink coverage
Optional cleanups
Worktree child cwd (P2) Verified: Generated-by: Grok |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf5b8ee4-a5a4-49a5-b08c-1ed4a279cc39
📒 Files selected for processing (3)
apps/desktop/src/main/__tests__/project-management-service.test.tspackages/storage/src/__tests__/project-catalog.test.tspackages/storage/src/project-catalog.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/tests/project-management-service.test.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
The nested-path test now creates a Session on the source project and checks that relinkWithSessions reports that id and rewrites the header cwd to the nested folder. Generated-by: Grok
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I re-reviewed the latest head (c2e26a9) and the original concerns are now addressed.
The touch() path matches registered project locations directly, and the nested relinkWithSessions coverage now creates a real Session and verifies both the reported Session ID and the persisted projectId/cwd.
I found no remaining P0–P3 issues. The implementation stays within the existing Project Catalog authority and is the smallest complete fix for #2660. The repository-root behavior of Implementation children remains a separate, pre-existing executor concern and does not block this PR.
Targeted storage tests pass (20/20), CI is green, and the PR is merge-clean. Approved from a code-review perspective.
Reviewed with Codex as an AI-assisted adversarial review; I verified the final diff, tests, feedback resolutions, CI, and merge state.
中文版
感谢继续完善。我重新审查了最新提交 c2e26a9，此前的问题均已解决。
touch() 现在会直接匹配已登记的项目路径；嵌套目录的 relinkWithSessions 测试也已创建真实 Session，并验证返回的 Session ID 以及持久化后的 projectId 和 cwd。
当前没有剩余的 P0–P3 问题。实现沿用现有 Project Catalog 权威边界，是解决 #2660 的最小完整方案。Implementation child 从仓库 worktree 根目录启动属于既有 executor 行为，可单独跟进，不阻塞本 PR。
定向测试 20/20 通过，CI 全绿，且当前可干净合并。从代码审查角度同意合并。
2 participants
Summary
Adding a project from a folder that lives inside an existing Git project remapped the selection to the repository root.
registerusedresolveProjectLocation, which walks to the enclosing worktree, soparent/childbecameparentand the already-registered parent project was reopened.User-chosen add/relink paths now keep the exact selected folder when it is not the worktree root. A repository root and a linked worktree still share one Git identity. Historical session cwds still resolve to the enclosing repository, so a session that ran in
repo/srcdoes not become a new project.Fixes #2660
Verification
npm --workspace @maka/storage run buildnpm --workspace @maka/storage run typechecknode --test packages/storage/dist/__tests__/project-catalog.test.js— 19/19 (includes the new parent/child case)npx tsx --test apps/desktop/src/main/__tests__/project-management-service.test.ts— 6/6npx tsx --test apps/desktop/src/main/__tests__/new-session-project.test.ts— 7/7@maka/desktopbuild:main— currentmainfails typecheck on unrelated files (assistant-stream.test.ts,chat-readiness.ts,conversation-copy.tsmissingoauth_subscription_not_wired)AI use
Select exactly one:
Tool(s) and scope:
Grok authored the implementation, tests, and this PR description.
Checklist
Does this PR entail a change in behavior?