fix(tui): apply scope dialog to all selected plugins#40
Conversation
Automated PR ReviewReviewed commit: Summary
go:implementation-tests (1 finding)Minor -
|
| Field | Value |
|---|---|
| Model | claude-sonnet-4-6 |
| Reviewers | go:implementation-tests |
| Engine | claude_cli · claude-sonnet-4-6 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 19s wall · 2m 18s compute |
| Cost | ~$0.17 (est.) |
| Tokens | 10 in / 753 out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-4-6 | 4 | 507 | 7.2k | 12.3k | ~$0.06 (est.) | 12s |
| go:implementation-tests | claude-sonnet-4-6 | 3 | 123 | 3.9k | 19.4k | ~$0.08 (est.) | 1m 54s |
| orchestrator-rollup | claude-sonnet-4-6 | 3 | 123 | 3.9k | 8.1k | ~$0.03 (est.) | 11s |
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR review completed with outcome: approved.
PR BuildVersion:
Built from 73b0872 |
Automated PR ReviewReviewed commit: Summary
go:implementation-tests (1 finding)Minor -
|
| Field | Value |
|---|---|
| Model | claude-sonnet-4-6 |
| Reviewers | go:implementation-tests |
| Engine | claude_cli · claude-sonnet-4-6 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 36s wall · 2m 35s compute |
| Cost | ~$0.26 (est.) |
| Tokens | 13 in / 2.3k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-4-6 | 5 | 1.3k | 20.1k | 15.1k | ~$0.08 (est.) | 30s |
| go:implementation-tests | claude-sonnet-4-6 | 4 | 682 | 27.7k | 29.8k | ~$0.13 (est.) | 1m 54s |
| orchestrator-rollup | claude-sonnet-4-6 | 4 | 407 | 7.7k | 11.4k | ~$0.05 (est.) | 10s |
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR review completed with outcome: approved.
Automated PR ReviewReviewed commit: Summary
2 PR discussion threads considered. 2 summarized; 2 resolved. Completed in 2m 51s | ~$0.16 (est.) | claude-sonnet-4-6 | cr 0.4.161
Per-workstream usage
|
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR review completed with outcome: approved.
Automated PR ReviewReviewed commit: Summary
4 PR discussion threads considered. 4 summarized; 4 resolved. Completed in 2m 09s | ~$0.30 (est.) | claude-sonnet-4-6 | cr 0.4.161
Per-workstream usage
|
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR review completed with outcome: approved.
The "select scope" dialog (Shift-S) operated only on the focused plugin: openScopeDialogForSelected called getSelectedPlugin (singular) and scopeDialogState carried a single pluginID, so confirming the dialog produced a pending operation for just the cursor item even when multiple plugins were bulk-selected. Extend the scope dialog to act on every bulk-selected plugin (falling back to the focused plugin when nothing is multi-selected, matching the existing bulk install/uninstall UX). scopeDialogState now tracks a list of targets, each with its own original scopes, and applyScopeDialogDelta computes the install/uninstall/scope-change delta per target relative to that plugin's current scopes. The dialog title reflects when multiple plugins are selected. Add regression tests for multi-select target loading, focused-item fallback, and per-target delta computation (install, uninstall, mixed scope-change). Closes #39
- Clone focused.InstalledScopes when seeding the scope dialog checkboxes, matching the maps.Clone style used for every target's originalScopes and avoiding aliasing live plugin state if the reference is later retained. - Add TestApplyScopeDialogDeltaTargetlessFallback to explicitly exercise the len(targets)==0 backward-compatible fallback in applyScopeDialogDelta, and document the path from the fallback comment.
The scopeDialogState.originalScopes field aliased targets[0].originalScopes, so mutating either backing map silently corrupted the other. The field was read only by a targetless fallback that existed solely for direct struct assignments in tests. Remove the mirror field (and the now-dead fallback), reading original scopes from each target instead, and convert the affected tests to populate targets directly. Also add coverage for openScopeDialogForSelected when the focused plugin is not bulk-selected: targets contain only the bulk selection and the dialog checkboxes seed from the first target's scopes, not the focused plugin's.
piekstra
force-pushed
the
fix/multiselect-scope-applies-to-all
branch
from
8c4e8e2 to
73b0872
Compare
2 participants
Closes #39
Summary
Root cause: The "select scope" dialog (Shift-S) only acted on the focused/cursor plugin, ignoring the rest of a multi-selection.
openScopeDialogForSelectedcalledgetSelectedPlugin()(singular), andscopeDialogStatecarried a singlepluginID. When the dialog was confirmed,applyScopeDialogDeltawrote a pending operation for just that one plugin — so with several plugins bulk-selected (space /a), Shift-S still changed the scope of only one of them.By contrast, the other bulk operations (
l/pinstall,uuninstall) already usedgetSelectedPlugins()(plural). Those paths even bailed out to the single-plugin scope dialog (return // Dialog handles single plugin at a time) precisely because the dialog itself couldn't handle more than one plugin.Fix: The scope dialog now acts on every bulk-selected plugin, falling back to the focused plugin when nothing is multi-selected (matching the existing bulk install/uninstall UX).
scopeDialogStategains atargets []scopeDialogTargetlist; each target carries its ownoriginalScopes.pluginID/originalScopesstill mirror the first target so rendering and existing single-plugin callers/tests are unchanged.openScopeDialogForSelectedgathersgetSelectedPlugins(), builds a target per non-header plugin, and seeds the checkboxes from the focused plugin's scopes.applyScopeDialogDeltaiterates over all targets, computing the install / uninstall / mixed-OpScopeChangedelta per target against that plugin's own original scopes. This is the correct semantics: the checkboxes describe the desired end-state scope set, and each plugin transitions toward it from wherever it currently is.Scopes for N selected pluginswhen more than one is targeted.openScopeDialog(single-plugin path used mid-loop by install/uninstall) is preserved as a thin wrapper over the new multi-target path, so behavior there is unchanged.Testing
All commands run from the repo root.
go vet ./...— passgofmt -l .— no files need formattinggo run mvdan.cc/gofumpt@v0.9.2 -l -extra .— no files need formatting (matchesmise run fmt)golangci-lint run ./...—0 issues.go test -race -count=1 ./...— all packages pass (matchesmise run test)go build ... -o cpm ./cmd/cpm— builds successfullyNew regression tests in
internal/tui/model_test.go:TestOpenScopeDialogForSelectedMultiSelectTargets— Shift-S loads all bulk-selected plugins as targets (not just the focused one)TestOpenScopeDialogForSelectedFallsBackToFocused— with no multi-selection, only the focused plugin is targetedTestApplyScopeDialogDeltaMultiSelectInstall/...Uninstall— confirming applies the change to every selected pluginTestApplyScopeDialogDeltaMultiSelectPerTargetDelta— each plugin's delta is computed against its own scopes (e.g. one getsOpScopeChange, another no-ops)TestRenderScopeDialogMultiTargetTitle— multi-plugin title renderingCaveats:
mise/gofumptweren't installed locally, so the documentedmise run ...tasks were executed via their underlying commands (go test -race,golangci-lint run,gofumptviago run,go build), which is exactly what those tasks invoke.