feat(web): bulk-remove multiple projects from the serve UI#469
Conversation
Add checkbox selection to the Projects table (per-row + select-all with an
indeterminate state) and a "Remove from kbagent" action that removes several
projects at once. Replaces the native window.confirm() with a styled
ConfirmModal that lists the affected aliases and spells out that this only
unregisters them locally (edits kbagent config) and does NOT delete the
Keboola projects.
Backend: ProjectService.bulk_remove_projects (per-alias error accumulation +
dry_run, local config.json only) and POST /projects/bulk-delete, declared
before the /{alias} routes so the literal path is matched first. The single
trash button now flows through the same modal + endpoint.
Tests: service (partial failure, dry-run, dedup) + router (kwargs, dry_run
forwarding, route-not-shadowed).
ReviewOverall well-structured PR — clean separation between service/router/frontend, good test coverage, and the Summary
See inline comments for details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # Validate existence (and the ephemeral guard) without | ||
| # mutating: a missing alias has no persisted project. | ||
| if self._config_store.get_project(alias) is None: | ||
| raise ConfigError(f"Project '{alias}' not found.") |
There was a problem hiding this comment.
Bug: The dry-run path only checks get_project(alias) is None but skips the ephemeral (__env__) guard. The real removal path goes through config_store.remove_project() which calls _reject_ephemeral_mutation(config, alias, "removed") — so an __env__ project would show up in the removed list during dry-run but would actually fail with a ConfigError on real execution.
To match the live path's validation, you'd need something like:
if dry_run:
config = self._config_store.load()
project = config.projects.get(alias)
if project is None:
raise ConfigError(f"Project '{alias}' not found.")
if project.ephemeral:
raise ConfigError(
f"Project '{alias}' is synthesized from environment variables "
"and cannot be removed."
)
removed.append(alias)Or factor the validation out of config_store.remove_project into a reusable check so both paths stay in sync.
| alert(`Removed ${res.removed.length}; ${res.failed.length} failed:\n${lines}`); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Important: No onError handler on the mutation. If the POST request itself fails (network error, 500, etc.), the user gets no feedback — the modal just disappears via onSettled and nothing happens. The partial-failure handling in onSuccess is good, but a total request failure is silently swallowed.
Consider adding:
onError: (err) => {
alert(err instanceof ApiError ? err.message : (err as Error).message);
},Or better, show an inline error instead of alert() (see nit below).
| api.post<BulkDeleteResult>("/projects/bulk-delete", { aliases }), | ||
| onSuccess: (res) => { | ||
| setSelectedAliases(new Set()); | ||
| qc.invalidateQueries({ queryKey: ["projects"] }); |
There was a problem hiding this comment.
Important: After a successful bulk-delete, the selected detail panel (below the table) could still show a project that was just removed. The selectedAliases set is cleared, but selected (the Project | null state for the detail pane) is not.
Consider clearing it if the selected project was among those removed:
onSuccess: (res) => {
setSelectedAliases(new Set());
if (selected && res.removed.includes(selected.alias)) {
setSelected(null);
}
qc.invalidateQueries({ queryKey: ["projects"] });
// ...
},
| qc.invalidateQueries({ queryKey: ["projects"] }); | ||
| if (res.failed.length > 0) { | ||
| const lines = res.failed.map((f) => `• ${f.alias}: ${f.error}`).join("\n"); | ||
| alert(`Removed ${res.removed.length}; ${res.failed.length} failed:\n${lines}`); |
There was a problem hiding this comment.
Nit: alert() feels out of place in an app that otherwise uses styled modals and <ErrorBox>. Consider showing partial failures in an inline error banner or reusing the ConfirmModal pattern as a result dialog. Not blocking, but it's a jarring UX switch.
| const confirmRef = useRef<HTMLButtonElement>(null); | ||
|
|
||
| useEffect(() => { | ||
| confirmRef.current?.focus(); |
There was a problem hiding this comment.
Nit: The confirm button auto-focuses on mount, so pressing Enter immediately confirms the destructive action. For a danger modal this is a deliberate trade-off (speed vs. safety). If you want the safer default, consider focusing the Cancel button instead, or requiring an explicit click for danger actions. Current behavior matches what the PR description says, so this is just a "consider" note, not blocking.
- Service: dry-run now applies the same validation as the live remove via a new ConfigStore.ensure_removable() — an ephemeral `__env__` project is reported in `failed` (not `removed`), matching the real run that would ConfigError. Adds a regression test. - Frontend: bulk-delete mutation gains an onError handler so a total request failure (network/5xx) surfaces instead of silently closing the modal; partial and total failures now show in an inline, dismissible ErrorBox banner instead of a native alert(). After a successful remove, the detail pane is cleared if it was showing a just-removed project. - ConfirmModal: danger modals focus Cancel on open (a stray Enter no longer fires the destructive action); non-danger modals keep Confirm focused.
|
Thanks for the review — addressed in f69d4e0:
All green: affected pytest (115 passed) + ruff/ty clean; frontend |
Re-review (post f69d4e0)All five items from the first review are properly addressed:
Quality of the fixes
No new issues found. LGTM. |
1 participant
What
Adds multi-project removal to the
kbagent serveWeb UI. The Projects table gets per-row checkboxes plus a select-all header (with an indeterminate state), and a Remove from kbagent action that unregisters several projects in one go.A styled confirmation modal lists the affected aliases and makes the scope explicit: this only edits the local kbagent config — it does NOT delete the Keboola projects. The existing single-row trash button now flows through the same modal + endpoint.
Changes
ProjectService.bulk_remove_projects(aliases, dry_run): removes each alias, accumulating per-alias errors so one bad alias (missing / ephemeral__env__) never blocks the rest. Localconfig.jsononly, no remote API call. Returns{removed, failed, dry_run}.POST /projects/bulk-delete(body{aliases, dry_run}), declared before the/{alias}routes so the literal path is matched first (never resolves toremove_project("bulk-delete")).DataTablegains optional checkbox selection (per-row + select-all/indeterminate);Projects.tsxadds selection state, the bulk action, and a reusableConfirmModal(matches theManageTokenModalstyle) that lists aliases and spells out the local-only scope./{alias}).UX
Remove from kbagentbutton appears only when ≥1 project is selected. The confirm modal:Remove N projects from kbagentRemove from kbagentconfirm +Cancel.Verified visually in a running
serve --ui.Tests / checks
4190 passed, 135 skipped; ruff lint+format, ty (warnings only), skill-check, changelog-check, command-sync, error-codes all green. Frontendtsc -b+vite buildclean.Notes
make version-sync).feat/web-convert-to-partitionedand is not part of this PR.