diff --git a/docs-ai/062-workspace-child-diff/001-action.md b/docs-ai/062-workspace-child-diff/001-action.md index 7e8c0e8c..301f5d32 100644 --- a/docs-ai/062-workspace-child-diff/001-action.md +++ b/docs-ai/062-workspace-child-diff/001-action.md @@ -7,7 +7,8 @@ | 2026-08-19 | Plan aligned with onevcat (⌘⇧Y follows selected child; Outgoing Changes included; Hunk runs in workspace terminal with child cwd; natural degradation, no pre-checks) | tracker relay-tracker#154, issue #616 | | 2026-08-19 | Implemented `DiffTarget` routing end to end, with tests and `docs/` updates | #704 | | 2026-08-20 | Review round 1 (pi agent): scoped child targets by workspace, child `repositoryRootURL` follows recorded source root, added child outgoing coverage | #704 | -| 2026-08-20 | Review round 2 (pi agent): child roots now live-resolved via `gitClient.repoRoot` (cached per reload) since a recorded source location may be a subdirectory or nested worktree; selection pruning validates against the selected workspace's own children | #704 | +| 2026-08-20 | Review round 2 (pi agent): child roots live-resolved via `gitClient.repoRoot` (cached per reload) since a recorded source location may be a subdirectory or nested worktree; selection pruning validates against the selected workspace's own children | #704 | +| 2026-08-20 | Review round 3 (pi agent): dropped the root cache — canonicalization moved to the diff effects (`AppFeature.canonicalizedDiffTarget`) at invocation time, eliminating the startup window and cache-staleness class; empty-child reloads cancel the in-flight refresh and stale child updates are filtered | #704 | ## Outcome & current state (as of 2026-08-19) @@ -21,17 +22,21 @@ metadata → repository name; terminal host is the synthesized workspace worktree via `plainFolderWorktree(for:)`), `selectedDiffTargetID` (selected worktree, else selected workspace child), and `pullRequest(for:)` (per-target PR cache dispatch, wired from - `supacodeApp`). A child's `repositoryRootURL` preference is: live-resolved root → - metadata source root (`ProjectWorkspaceRepositoryEntry.localSourceURL`) → checkout - directory. The live root comes from `gitClient.repoRoot(childDirectory)` in the - workspace-children refresh pipeline, cached in `workspaceChildRepoRootByID` — the same - normalization that keys registered repositories (`RepositoriesFeature+RepositoryLoading`), - so the child's `repositorySettings` lookup matches its source repository by construction, - even when the recorded source location is a subdirectory or a nested worktree. The - metadata fallback covers the window before the first refresh lands. + `supacodeApp`). A child's sync-resolved `repositoryRootURL` is the metadata source root + (`ProjectWorkspaceRepositoryEntry.localSourceURL`) falling back to the checkout + directory; because a recorded source location may be a subdirectory or a nested + worktree, the diff effects canonicalize it at invocation time — + `AppFeature.canonicalizedDiffTarget` runs `gitClient.repoRoot(childDirectory)` inside + `openDiffEffect` / `openOutgoingChangesEffect`, the same normalization that keys + registered repositories (`RepositoriesFeature+RepositoryLoading`), so + `repositorySettings` and `{repoPath}` match the source repository by construction with + no cache to go stale and no startup window. A failed lookup keeps the metadata fallback. - `pruneWorkspaceChildInfo` validates `selectedWorkspaceChildID` against the selected workspace's own children (not the global path set), so a child removed from the selected workspace cannot survive as a ghost selection through another workspace sharing the path. +- Reloads that leave no workspace children cancel the in-flight children refresh, and + `workspaceChildrenInfoLoaded` filters updates to current children, so a late batch + cannot repopulate just-pruned maps. - `RepositoriesFeature.Delegate.showDiff` / `.showOutgoingChanges` carry `DiffTargetID`; `WorkspaceChildRowsView` + `RepositorySectionView` wire the child badge and new **Show Diff** / **Show Outgoing Changes** context-menu items. diff --git a/supacode/Domain/DiffTarget.swift b/supacode/Domain/DiffTarget.swift index bdaa24ff..909e1f80 100644 --- a/supacode/Domain/DiffTarget.swift +++ b/supacode/Domain/DiffTarget.swift @@ -22,7 +22,9 @@ nonisolated struct DiffTarget: Equatable, Sendable { /// Display branch; also fills `{branch}` in custom diff command templates. let branchName: String /// Fills `{repoPath}` in custom templates and keys `repositorySettings`. - let repositoryRootURL: URL + /// For workspace children this starts as a metadata approximation and is + /// canonicalized by the diff effects before use. + var repositoryRootURL: URL /// Worktree owning the terminal that the Hunk tool runs in. let terminalHost: Worktree /// Hunk cwd when it differs from the host's own directory. diff --git a/supacode/Features/App/Reducer/AppFeature+CommandPalette.swift b/supacode/Features/App/Reducer/AppFeature+CommandPalette.swift index 45ec47ac..0248f41b 100644 --- a/supacode/Features/App/Reducer/AppFeature+CommandPalette.swift +++ b/supacode/Features/App/Reducer/AppFeature+CommandPalette.swift @@ -190,6 +190,23 @@ extension AppFeature { } } + /// A workspace child's sync-resolved root comes from workspace metadata, + /// which may record a subdirectory or a nested worktree. Canonicalize it + /// through the same `repoRoot` normalization that keys registered + /// repositories, at the moment the action runs, so `repositorySettings` and + /// `{repoPath}` match the source repository. Worktree targets already carry + /// their registered root; a failed lookup keeps the metadata fallback. + func canonicalizedDiffTarget(_ target: DiffTarget) async -> DiffTarget { + guard case .workspaceChild = target.id, + let root = try? await gitClient.repoRoot(target.workingDirectory) + else { + return target + } + var target = target + target.repositoryRootURL = root + return target + } + func openDiffEffect( target: DiffTarget, resolvedKeybindings: ResolvedKeybindingMap @@ -200,6 +217,7 @@ extension AppFeature { customCommand: settingsFile.global.externalDiffCustomCommand ) return .run { send in + let target = await canonicalizedDiffTarget(target) await externalDiffToolClient.open(settings, target, resolvedKeybindings) { error in send(.openWorktreeFailed(error)) } @@ -228,6 +246,7 @@ extension AppFeature { } let resolvedKeybindings = state.resolvedKeybindings return .run { send in + let target = await canonicalizedDiffTarget(target) await outgoingChangesClient.open(target, resolvedKeybindings) { error in send(.openWorktreeFailed(error)) } diff --git a/supacode/Features/App/Reducer/AppFeature.swift b/supacode/Features/App/Reducer/AppFeature.swift index 037394f1..65059c30 100644 --- a/supacode/Features/App/Reducer/AppFeature.swift +++ b/supacode/Features/App/Reducer/AppFeature.swift @@ -114,6 +114,7 @@ struct AppFeature { @Dependency(CustomShortcutRegistryClient.self) var customShortcutRegistryClient @Dependency(ExternalDiffToolClient.self) var externalDiffToolClient @Dependency(OutgoingChangesClient.self) var outgoingChangesClient + @Dependency(GitClientDependency.self) var gitClient var body: some Reducer { let core = Reduce { state, action in diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+CoreReducer.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+CoreReducer.swift index 1919a84e..a985b62a 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+CoreReducer.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+CoreReducer.swift @@ -1004,7 +1004,10 @@ extension RepositoriesFeature { return .none case .workspaceChildrenInfoLoaded(let updates): - applyWorkspaceChildrenInfo(updates, state: &state) + // An in-flight refresh can land after a reload changed the child set; + // only merge updates that still belong to a current workspace child. + let validIDs = Set(state.allResolvedWorkspaceChildren().map(\.id)) + applyWorkspaceChildrenInfo(updates.filter { validIDs.contains($0.id) }, state: &state) return .none case .alert(.dismiss): diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+StateQueries.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+StateQueries.swift index 13bb0782..34f4fef1 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+StateQueries.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+StateQueries.swift @@ -244,13 +244,14 @@ extension RepositoriesFeature.State { else { return nil } - // Root preference: live-resolved root (same normalization that keys - // registered repositories) → metadata source root → checkout directory. + // The metadata-derived root is a synchronous approximation; the diff + // effects canonicalize it through `gitClient.repoRoot` when the action + // runs (`AppFeature.canonicalizedDiffTarget`). return DiffTarget( id: id, workingDirectory: child.workingDirectory, branchName: workspaceChildBranchByID[child.id] ?? child.metadataBranch ?? child.repositoryName, - repositoryRootURL: workspaceChildRepoRootByID[child.id] ?? child.repositoryRootURL, + repositoryRootURL: child.repositoryRootURL, terminalHost: Self.plainFolderWorktree(for: workspaceRepository), terminalWorkingDirectory: child.workingDirectory ) diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift index 90eee280..4b59faf1 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift @@ -30,7 +30,9 @@ extension RepositoriesFeature { func refreshWorkspaceChildrenEffect(state: State) -> Effect { let children = state.allResolvedWorkspaceChildren() guard !children.isEmpty else { - return .none + // A reload that removed the last child must also stop an in-flight + // refresh, or its late result would repopulate the just-pruned maps. + return .cancel(id: CancelID.workspaceChildrenRefresh) } let gitClient = self.gitClient let githubCLI = self.githubCLI @@ -43,9 +45,6 @@ extension RepositoriesFeature { group.addTask { async let branchTask = gitClient.branchName(child.workingDirectory) async let changesTask = gitClient.lineChanges(child.workingDirectory) - // Same normalization that keys registered repositories, so the - // child's repository-settings lookup matches its source repo. - async let rootTask = try? gitClient.repoRoot(child.workingDirectory) let changes = await changesTask let branch = await branchTask let pullRequest = await Self.fetchWorkspaceChildPullRequest( @@ -59,8 +58,7 @@ extension RepositoriesFeature { id: child.id, branch: branch, lineChanges: changes, - pullRequest: pullRequest, - repositoryRoot: await rootTask + pullRequest: pullRequest ) } } @@ -114,12 +112,6 @@ func applyWorkspaceChildrenInfo( state.workspaceChildBranchByID.removeValue(forKey: update.id) } - if let repositoryRoot = update.repositoryRoot { - state.workspaceChildRepoRootByID[update.id] = repositoryRoot - } else { - state.workspaceChildRepoRootByID.removeValue(forKey: update.id) - } - var entry = state.workspaceChildInfoByID[update.id] ?? WorktreeInfoEntry() if let changes = update.lineChanges, !changes.isEmpty { entry.addedLines = changes.added @@ -144,9 +136,6 @@ func pruneWorkspaceChildInfo(state: inout RepositoriesFeature.State) { let validIDs = Set(state.allResolvedWorkspaceChildren().map(\.id)) state.workspaceChildInfoByID = state.workspaceChildInfoByID.filter { validIDs.contains($0.key) } state.workspaceChildBranchByID = state.workspaceChildBranchByID.filter { validIDs.contains($0.key) } - state.workspaceChildRepoRootByID = state.workspaceChildRepoRootByID.filter { - validIDs.contains($0.key) - } // The selection is validated against the selected workspace's own children, // not the global path set: another workspace referencing the same path must // not keep a removed child selected here. diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift index 1ae075a2..2f05bb51 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift @@ -71,15 +71,13 @@ struct ForceDeleteBranchRequest: Equatable { } // Result of refreshing one workspace child repository's live status: current -// branch, uncommitted diff counts, (when GitHub integration is available) the -// PR for that branch, and the resolved repository root used to key -// repository-scoped diff settings. +// branch, uncommitted diff counts, and (when GitHub integration is available) +// the PR for that branch. nonisolated struct WorkspaceChildInfoUpdate: Equatable, Sendable { let id: String let branch: String? let lineChanges: GitLineChanges? let pullRequest: GithubPullRequest? - let repositoryRoot: URL? } struct RemoveWorkspaceConfirmation: Equatable { @@ -307,12 +305,6 @@ struct RepositoriesFeature { // folder. Refreshed by `refreshWorkspaceChildrenEffect` on each repo reload. var workspaceChildInfoByID: [String: WorktreeInfoEntry] = [:] var workspaceChildBranchByID: [String: String] = [:] - // Canonical repository root per child, resolved through the same - // `gitClient.repoRoot` normalization that keys registered repositories — - // so a child's `repositorySettings` lookup matches its source repository - // even when the recorded source location is a subdirectory or a nested - // worktree. Nil (falling back to metadata) until a refresh lands. - var workspaceChildRepoRootByID: [String: URL] = [:] var selectedWorkspaceChildID: String? var worktreeOrderByRepository: [Repository.ID: [Worktree.ID]] = [:] var isOpenPanelPresented = false diff --git a/supacodeTests/AppFeatureCommandPaletteTests.swift b/supacodeTests/AppFeatureCommandPaletteTests.swift index 4473039e..873eede0 100644 --- a/supacodeTests/AppFeatureCommandPaletteTests.swift +++ b/supacodeTests/AppFeatureCommandPaletteTests.swift @@ -1024,6 +1024,9 @@ struct AppFeatureCommandPaletteTests { $0.externalDiffToolClient.open = { _, target, _, _ in launched.withValue { $0.append(target) } } + // The effect canonicalizes the child's repository root at invocation + // time; the resolved root must reach the diff client. + $0.gitClient.repoRoot = { _ in URL(fileURLWithPath: "/tmp/child-source-root") } } store.exhaustivity = .off @@ -1041,7 +1044,7 @@ struct AppFeatureCommandPaletteTests { #expect(target.id == .workspaceChild(workspaceID: workspace.id, path: childID)) #expect(target.workingDirectory == childURL) #expect(target.branchName == "feature/child") - #expect(target.repositoryRootURL == childURL) + #expect(target.repositoryRootURL == URL(fileURLWithPath: "/tmp/child-source-root")) #expect(target.terminalHost.id == workspace.id) #expect(target.terminalWorkingDirectory == childURL) } @@ -1080,6 +1083,11 @@ struct AppFeatureCommandPaletteTests { $0.outgoingChangesClient.open = { target, _, _ in outgoingRequests.withValue { $0.append(target) } } + // A failed root canonicalization keeps the metadata fallback instead of + // blocking the action. + $0.gitClient.repoRoot = { _ in + throw NSError(domain: "test", code: 1) + } } store.exhaustivity = .off @@ -1093,10 +1101,12 @@ struct AppFeatureCommandPaletteTests { await store.send(.showSelectedWorktreeOutgoingChanges) await store.finish() + let childURL = URL(fileURLWithPath: childID) #expect(outgoingRequests.value.count == 2) for target in outgoingRequests.value { #expect(target.id == .workspaceChild(workspaceID: workspace.id, path: childID)) - #expect(target.workingDirectory == URL(fileURLWithPath: childID)) + #expect(target.workingDirectory == childURL) + #expect(target.repositoryRootURL == childURL) } } diff --git a/supacodeTests/RepositoriesFeatureTests.swift b/supacodeTests/RepositoriesFeatureTests.swift index 2dc90702..70f78bfa 100644 --- a/supacodeTests/RepositoriesFeatureTests.swift +++ b/supacodeTests/RepositoriesFeatureTests.swift @@ -710,13 +710,8 @@ struct RepositoriesFeatureTests { } $0.repositoryPersistence.saveRepositorySnapshot = { _ in } $0.gitClient.repoRoot = { url in - let path = url.path(percentEncoded: false) - // The child pipeline resolves the child's own repository root; only - // probing the workspace root itself is forbidden. - guard path == childID else { - Issue.record("workspace should load as plain without git probing: \(path)") - return url - } + Issue.record( + "workspace should load as plain without git probing: \(url.path(percentEncoded: false))") return url } $0.gitClient.worktrees = { url in @@ -724,8 +719,7 @@ struct RepositoriesFeatureTests { return [] } // The workspace's child repository is refreshed via the child pipeline - // (live branch + diff + repo root), distinct from the worktree probing - // above. + // (live branch + diff), distinct from the worktree probing above. $0.gitClient.branchName = { _ in "main" } $0.gitClient.lineChanges = { _ in nil } $0.gitClient.remoteInfo = { _ in nil } @@ -741,7 +735,6 @@ struct RepositoriesFeatureTests { await store.receive(\.delegate.repositoriesChanged) await store.receive(\.workspaceChildrenInfoLoaded) { $0.workspaceChildBranchByID = [childID: "main"] - $0.workspaceChildRepoRootByID = [childID: URL(fileURLWithPath: childID)] } await store.finish() } @@ -7651,15 +7644,13 @@ struct RepositoriesFeatureTests { id: "/ws/app", branch: "feature", lineChanges: GitLineChanges(added: 7, removed: 2), - pullRequest: pullRequest, - repositoryRoot: URL(fileURLWithPath: "/ws/app-source") + pullRequest: pullRequest ), WorkspaceChildInfoUpdate( id: "/ws/api", branch: " ", lineChanges: GitLineChanges(added: 0, removed: 0), - pullRequest: nil, - repositoryRoot: nil + pullRequest: nil ), ], state: &state @@ -7669,11 +7660,9 @@ struct RepositoriesFeatureTests { #expect(state.workspaceChildInfoByID["/ws/app"]?.addedLines == 7) #expect(state.workspaceChildInfoByID["/ws/app"]?.removedLines == 2) #expect(state.workspaceChildInfoByID["/ws/app"]?.pullRequest == pullRequest) - #expect(state.workspaceChildRepoRootByID["/ws/app"] == URL(fileURLWithPath: "/ws/app-source")) // Blank branch + empty diff + no PR → no entries. #expect(state.workspaceChildBranchByID["/ws/api"] == nil) #expect(state.workspaceChildInfoByID["/ws/api"] == nil) - #expect(state.workspaceChildRepoRootByID["/ws/api"] == nil) } @Test func workspaceChildRowsMergesLiveBranchAndInfo() { @@ -7841,7 +7830,7 @@ struct RepositoriesFeatureTests { id: "/tmp/ws-roots", children: [linkedEntry, remoteEntry] ) - var state = makeState(repositories: [repository]) + let state = makeState(repositories: [repository]) let linkedChildID = linkedEntry.resolvedURL(relativeTo: repository.rootURL) .path(percentEncoded: false) let remoteChildID = remoteEntry.resolvedURL(relativeTo: repository.rootURL) @@ -7857,14 +7846,6 @@ struct RepositoriesFeatureTests { #expect(linkedTarget?.repositoryRootURL == URL(fileURLWithPath: "/tmp/source-repo")) #expect(linkedTarget?.workingDirectory == URL(fileURLWithPath: linkedChildID)) #expect(remoteTarget?.repositoryRootURL == URL(fileURLWithPath: remoteChildID)) - - // A live-resolved root (recorded source was a subdirectory or a nested - // worktree) wins over the metadata source location. - state.workspaceChildRepoRootByID[linkedChildID] = URL(fileURLWithPath: "/tmp/true-root") - let resolvedTarget = state.diffTarget( - for: .workspaceChild(workspaceID: repository.id, path: linkedChildID) - ) - #expect(resolvedTarget?.repositoryRootURL == URL(fileURLWithPath: "/tmp/true-root")) } @Test func pruneClearsSelectedChildRemovedFromItsWorkspaceDespiteDuplicatePath() { @@ -8012,7 +7993,6 @@ struct RepositoriesFeatureTests { removedLines: 1, pullRequest: nil ) - initialState.workspaceChildRepoRootByID["/tmp/gone/app"] = URL(fileURLWithPath: "/tmp/gone") let store = TestStore(initialState: initialState) { RepositoriesFeature() } withDependencies: { @@ -8020,7 +8000,6 @@ struct RepositoriesFeatureTests { $0.gitClient.lineChanges = { _ in GitLineChanges(added: 7, removed: 2, skippedUntrackedFileCount: 1) } - $0.gitClient.repoRoot = { _ in URL(fileURLWithPath: "/tmp/resolved-root") } $0.repositoryPersistence.saveRepositorySnapshot = { _ in } } store.exhaustivity = .off @@ -8032,14 +8011,39 @@ struct RepositoriesFeatureTests { await store.finish() #expect(store.state.workspaceChildInfoByID["/tmp/gone/app"] == nil) - #expect(store.state.workspaceChildRepoRootByID["/tmp/gone/app"] == nil) #expect(store.state.workspaceChildBranchByID[childID] == "feature/live") #expect(store.state.workspaceChildInfoByID[childID]?.addedLines == 7) #expect(store.state.workspaceChildInfoByID[childID]?.removedLines == 2) #expect(store.state.workspaceChildInfoByID[childID]?.skippedUntrackedFileCount == 1) - #expect( - store.state.workspaceChildRepoRootByID[childID] == URL(fileURLWithPath: "/tmp/resolved-root") + } + + @Test func workspaceChildrenInfoLoadedIgnoresUpdatesForRemovedChildren() async { + // An in-flight refresh can land after a reload removed its child; the + // stale update must not repopulate the just-pruned maps. + let entry = ProjectWorkspace.RepositoryEntry( + id: "app", + name: "App", + path: "app", + sourceKind: .existingPath ) + let repository = makeWorkspaceRepository(id: "/tmp/ws-stale", children: [entry]) + let store = TestStore(initialState: makeState(repositories: [repository])) { + RepositoriesFeature() + } + + await store.send( + .workspaceChildrenInfoLoaded([ + WorkspaceChildInfoUpdate( + id: "/tmp/ws-removed/app", + branch: "stale", + lineChanges: GitLineChanges(added: 1, removed: 1), + pullRequest: nil + ) + ]) + ) + + #expect(store.state.workspaceChildBranchByID.isEmpty) + #expect(store.state.workspaceChildInfoByID.isEmpty) } @Test func openRepositoriesFinishedRefreshesWorkspaceChildren() async {