feat(ui): 0.89.0 catch-up — jobs/config actions, trash & restore, tokens, palette - #658
Conversation
…ens, palette
Catches the NERD web UI up with serve routes that shipped without a UI
surface, and adds the keyboard entry point the app was missing.
- Jobs: per-row and in-drawer re-run (POST /jobs/{p}/run) and terminate
(POST /jobs/{p}/terminate), the latter behind ConfirmModal and only
offered for created/waiting/processing. SSE log stream untouched.
- Configs: the detail view is now a Drawer with Run job and Delete
(soft-delete, DELETE /configs/...), plus a Trash tab
(GET /configs/trash/{p}) with per-row restore.
- Tokens: new page under MANAGE over /token/{p}/list|create|delete|refresh.
Secrets are revealed once in a copy-to-clipboard block; the
derive-last-used toggle is opt-in and renders never/unknown/error as
distinct pills.
- Storage: the table drawer renders the raw `definition` layout (#621)
and the schema tab's Description cell is click-to-edit through the
native describe-columns route (#624).
- Dashboard: fifth stat tile for the PAYG credit balance; a non-PAYG
project degrades to a muted n/a pill.
- Flows: read-only Notifications tab, with filter-less project-wide
subscriptions kept in their own warning-pilled group.
- Command palette (ctrl/cmd+k) over pages, projects and a few actions.
- Cleanups: ConfirmModal replaces the last window.confirm; the dead
useManageTokenPrompt helper is gone.
| onSuccess: () => { | ||
| qc.invalidateQueries({ queryKey: ["table-detail"] }); | ||
| }, |
There was a problem hiding this comment.
🟡 Edited column description hides later server values
The optimistic entry written by save is never removed once the write succeeds; describe.onSuccess only invalidates the query (Storage.tsx). The rendered value stays overrides[c.name] ?? c.description, so after an edit the locally typed text keeps overriding any newer server value for that column while the drawer is open.
Prompt for agents
In SchemaTab (web/frontend/src/pages/Storage.tsx), the optimistic `overrides` map is written in save() but never cleared after a successful describe mutation. The rendered value is `overrides[c.name] ?? c.description`, so the local value permanently masks the server's `column_details[].description` for any edited column while the drawer remains mounted. The inline comment claims the override lasts only 'until the refetch lands'. Fix by clearing the override for that column in the describe mutation's onSuccess handler (e.g. delete overrides[column] after invalidating/refetching table-detail), so the refetched server value is what renders.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <ConfirmModal | ||
| danger | ||
| busy={del.isPending} | ||
| title="Delete configuration?" | ||
| body={ | ||
| <> | ||
| <span className="font-mono text-accent"> | ||
| {componentId}/{configId} | ||
| </span>{" "} | ||
| moves to the trash. This is reversible — restore it from the Trash tab. Any schedule | ||
| or flow still pointing at it will start failing until it is restored. | ||
| </> | ||
| } | ||
| confirmLabel="Move to trash" | ||
| onConfirm={() => del.mutate()} | ||
| onCancel={() => setConfirmDelete(false)} | ||
| /> | ||
| ) : null} | ||
| </Drawer> |
There was a problem hiding this comment.
🔍 Confirm modals in drawers not portaled like Agents
The Agents discard-confirm was portaled to <body> because the drawer's backdrop-blur containing block plus overflow-auto can clip a nested fixed modal. The new config-delete and in-drawer job-terminate ConfirmModals are plain drawer children, not portaled. The fixed modal's containing block resolves to the viewport-spanning drawer root so it likely renders fine, but the inconsistency is worth a cross-browser glance.
Was this helpful? React with 👍 or 👎 to provide feedback.
Two review findings on the previous commit. - SchemaTab kept the optimistically written description in `overrides` forever, so an edited column masked every later server value for as long as the drawer stayed mounted. The entry is now dropped after the table-detail refetch resolves — awaiting the invalidation avoids flashing the stale value for one render. - ConfirmModal now portals to <body> itself, the way Drawer does, so a confirm raised from inside a drawer never depends on where it was declared: the drawer's backdrop-blur is a containing block for fixed descendants and its body scrolls under overflow-auto. This drops the ad-hoc createPortal wrapper the Agents discard-confirm needed and makes the config-delete and job-terminate confirms behave the same.
The Queue API job resource names the configuration id `config`, and
JobService.list_jobs adds only `project_alias` to the row -- everything
else is the API resource verbatim. types.ts had declared `configId`,
which is why the Jobs table's Config column had always rendered empty.
The re-run button added in the previous commit inherited that mismatch:
`canRerun = !!job.component && !!job.configId` was therefore always
false, so the button never rendered at all. Renaming the field fixes
both, and the gate is now load-bearing rather than accidental -- the
router's JobRun model requires `config_id`, so a job started from an
inline configData payload (no stored configuration) genuinely cannot be
re-run and must not offer the button.
- types.ts: `configId: string` -> `config: string | null`, documented.
- Jobs.tsx: every consumer updated. A new `jobLabel()` helper renders
"component ・ config <id>" and drops the config half when there is
none, replacing the drawer subtitle that rendered a literal
"config undefined"; the Config column and the Config ID card row
degrade the same way.
- Storage.tsx: report the partition count only when the list is
non-empty, matching the CLI's render_table_layout.
Re-verified the other new payload assumptions against the routers and
services: the run body {component_id, config_id} and terminate body
{job_ids, dry_run} match the JobRun / JobTerminate models; trash rows,
token fields (id/description/created/refreshed/expires/isMasterToken/
bucketPermissions/componentAccess/lastUsed*), notification rows and
credit rows (remaining/remaining_minutes) all match their services.
padak
left a comment
There was a problem hiding this comment.
Review of #658 — feat(ui): 0.89.0 catch-up — jobs/config actions, trash & restore, tokens, palette
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check/tsc/vite build,
not duplicated here.
Summary
This is a large, frontend-only catch-up PR (web/frontend, 2305/-398, no Python touched) adding job re-run/terminate, a config Drawer with Run-job/Delete + a Trash&Restore tab, a new Tokens page, table definition rendering, click-to-edit column descriptions, a billing-credits dashboard tile, a Flows Notifications tab, and a Ctrl+K command palette. The PR description is unusually precise about which server route backs each feature, and every claim I checked against the actual FastAPI router/Pydantic body in src/keboola_agent_cli/server/routers/*.py held up — including the job.configId → job.config field-name fix that the PR itself calls out. tsc --noEmit and vite build both pass clean. I found one genuine payload-correctness bug that fits squarely in this review's focus area: the per-job "re-run" action drops the job's branch_id, so re-running a job that originally executed on a dev branch silently re-runs it against the default/production branch instead. Verdict is REQUEST CHANGES for that one item; everything else (destructive-confirm coverage, secret handling on Tokens, design-language consistency) checked out clean.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 2
- Nits: 1
Blocking findings
[B-1] web/frontend/src/pages/Jobs.tsx:169-174 — per-job "re-run" drops branch_id, silently retargets the default branch
JobActions's rerun mutation posts only component_id and config_id to POST /jobs/{p}/run:
mutationFn: () =>
api.post(`/jobs/${encodeURIComponent(job.project_alias)}/run`, {
component_id: job.component,
config_id: job.config,
}),The Queue API job resource the row is built from does carry branchId (confirmed live: job_service.py:718 filters jobs by j.get("branchId")), but the Job TS interface (types.ts) never declares it, so it's silently unavailable to this call site. JobRun.branch_id on the server (server/routers/jobs.py) defaults to None when omitted, which resolves to the default/production branch. Concretely: a job originally run against a dev-branch config, re-run via this button, executes against the production config instead — a materially different configuration, potentially writing to the wrong tables/branch. This is a real behavior gap, not a display bug — the docstring above JobActions only calls out the configData-replay limitation, not this one.
Contrast with ConfigDetail's "Run job" action a few files over (Configs.tsx), which correctly threads branch_id: branchId ?? undefined from useUIState() because that Drawer is opened from a branch-scoped configs query. JobActions has no equivalent context to reuse, which is exactly why the job's own branchId needs to be surfaced and sent.
Fix: add branchId?: number | null to the Job interface (it's already on the wire, just untyped) and send branch_id: job.branchId ?? undefined in the rerun mutation body.
Non-blocking findings
[NB-1] No CI coverage for web/frontend (tsc / build)
.github/workflows/ci.yml has no step that runs tsc --noEmit or vite build against web/frontend (grepped for frontend/web/ — only a comment mentions it). This PR passes both when run manually, but it means a future frontend PR's type errors ship straight to main undetected. Pre-existing gap, not introduced by this PR — flagging per the review's "verify, don't assume" mandate since I had to run the build myself to confirm this PR is clean. Worth a follow-up issue, not a blocker here.
[NB-2] web/frontend/src/state.tsx — useManageTokenPrompt removed outside the PR's stated scope
The PR description doesn't mention removing the manage-token prompt helper. It's confirmed dead code (grep found zero remaining call sites, tsc is clean), so it's safe, but it's an unrelated cleanup riding along in a large feature PR. Worth a one-line mention in the PR description so a future git blame on this deletion doesn't have to reconstruct the "why" from an 18-file diff.
Nits
[NIT-1]web/frontend/src/pages/Tokens.tsx— thenever/unknown/errorstatus semantics (fromSTATUS_TITLES) are excellent and match the CLI's--with-last-useddocumentation precisely; no action needed, calling it out only because it's the kind of fidelity the rest of the PR should be held to.
Verification log
gh auth status→ authenticated aspadak✓- Read
CONTRIBUTING.md(Checklist for new CLI commands, Plugin synchronization map, Releasing a new version) andCLAUDE.md§17/## All CLI Commands— this PR adds/removes/renames zero CLI commands (frontend-only), soOPERATION_REGISTRY,AGENT_CONTEXT,keboola-expert.md,commands-reference.md,gotchas.mdare all correctly untouched. No silent-drift findings. gh pr view 658 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN,main←feat/ui-0890-catchup, +2305/-398, 15 files, conventionalfeat(ui):prefix matches (new page + several new actions) ✓gh pr diff 658→ 3199-line diff fetched to scratch, cross-checked file-by-file againstgh pr view --json files✓- Isolated detached
git worktree add ... origin/feat/ui-0890-catchup --detach— invoking worktree's HEAD (claude/keboola-cli-issues-pr-review-eb4380) confirmed unmoved before and after ✓ git log --oneline origin/main..HEAD→ 3 commits (feat(ui): ... catch-up,fix(ui): clear optimistic column override; portal ConfirmModal itself,fix(ui): job rows carry config, not configId) — all UI-scoped, consistent with the PR ✓- Cross-checked every claimed route in the PR body against the live FastAPI router source:
jobs.py(JobRun/JobTerminatebodies),configs.py(delete/restore/trash-list),token.py(CreateTokenBody/TokenIdBody),storage.py(DescribeColumns,table_detail),billing.py(get_creditsresponse fieldsremaining/consumed/total/remaining_minutes/error_code: PAYG_NOT_AVAILABLE"),notifications.py(list_subscriptionsunfiltered-by-design +project_wide_excluded) — all field names and shapes match the frontend types byte-for-byte ✓ - Confirmed the PR's headline claim:
services/job_service.pyraw job dicts useconfig(notconfigId) —job_service.py:718(j.get("branchId")) also confirmsbranchIdIS present on the raw dict, which is what B-1 leans on ✓ cd web/frontend && npm ci→ 392 packages, 0 vulnerabilities ✓npx tsc --noEmit→ clean, no output, exit 0 ✓npm run build(tsc -b && vite build) → built in 1.42s, only pre-existing chunk-size warning (mermaid/cytoscape vendor chunks, unrelated to this PR) ✓npx vitest run→ "No test files found" — confirmed pre-existing (zero*.test.*/*.spec.*files anywhere inweb/frontend/src), not a regression introduced by this PR- Grepped the full diff for
console.log/debugger/TODO/FIXMEin added lines → none ✓ - Grepped the full diff for token/secret/password patterns → all matches are the intentional Tokens-page UI (
SecretPanel,RevealedSecret, etc.); secret is held in React state only, cleared oncloseDrawer()/onClose(), never sent toconsoleor a query string ✓ - Manually traced
ConfirmModalcoverage: config delete (Configs.tsx), token delete + token refresh (Tokens.tsx, bothdanger), job terminate (Jobs.tsx,danger) — all four destructive/high-consequence actions gate throughConfirmModal; config restore and column-description edits (non-destructive writes) correctly do NOT require a confirm ✓ - Verified the
ConfirmModal→createPortal(..., document.body)fix — a genuine correctness fix for confirms raised from inside aDrawer(stacking/backdrop-blur containing-block issue), applies to every consumer listed above ✓ - Grepped
Sidebar.tsxSECTIONS(now exported, shared withCommandPalette.tsx) —Tokensentry present under "Manage" section, matches PR description "sidebar entry under MANAGE" ✓
Open questions for the author
- Is the missing
branch_idon job re-run (B-1) intentional for some reason not captured in the docstring (e.g. "re-run always targets production by design")? If so, worth a one-line comment next to thererunmutation saying so explicitly, since the current comment only addresses theconfigDatalimitation and reads as if branch fidelity were preserved.
The per-job re-run posted only component_id + config_id. JobRun.branch_id
defaults to None server-side, which resolves to the DEFAULT branch — so
re-running a job that had executed against a dev-branch config silently
re-ran it against the production config instead: a different
configuration, writing to different tables. Nothing in the UI said so.
The branch was on the wire the whole time (`branchId` on the raw Queue
API row — JobService._fetch_project_jobs filters on it) and the router
accepts it (`JobRun.branch_id: int | None`), so this is a pass-through,
not a new capability.
- types.ts: declare `branchId?: number | string | null` on Job. Typed
loosely on purpose — the Queue API is inconsistent about the
numeric-vs-string form, which is why the service compares it as
`str(j.get("branchId"))`.
- Jobs.tsx: new `jobBranchId()` coerces that to the `int | None` the
router declares, dropping anything non-numeric rather than sending a
value FastAPI would reject. The re-run body threads it through, and
the button's tooltip now names the branch it will target so the
affordance cannot mislead before the click.
|
Thanks — B-1 is a real bug and is fixed. Mapping each finding to its commit:
|
…g field (#662) The human-mode table for `kbagent job list` built the Config ID cell from `configId` (with a `config_id` fallback) -- but the Queue API job resource names the field `config`, and JobService returns the API row verbatim, so the column was always blank. Read `config` first with a tolerant `configId` fallback, matching the job-detail renderer. The unit-test fixtures in test_output.py and test_services.py hand-wrote the same invented `configId` key the renderer read, so they passed while real output was empty. Fixtures now use `config` (the real API shape, same as JOB_DETAIL_RESPONSE in test_cli.py) and the jobs-table test asserts the config values actually render. Python-side counterpart of the frontend fix in #658.
The CI workflow covers only the Python side; a frontend PR's type errors shipped to main undetected (PR #658 NB-1 -- a types.ts field-name mismatch made a table column render empty and a button never render, caught only by manual tsc/build runs). New path-filtered workflow runs npm ci, npx tsc --noEmit and npm run build in web/frontend on changes under web/. Separate file because paths: filters are trigger-level; safe because the main ruleset has no required status checks, so a skipped run cannot block a merge. No vitest step on purpose: the suite is empty and vitest run exits 1 on "No test files found", so it would fail every frontend PR rather than pass vacuously. Add npm test here when the first test lands.
* chore(release): 0.90.0 Bumps pyproject.toml to 0.90.0 and adds the changelog entry covering every PR merged since v0.89.0 (#658, #662, #661, #663, #665, #666, #664, #668, #667, #623), resolves the vNEXT placeholders those PRs left behind, and adds the curated What's new reel for the release. * docs(web-server): keep the What's-new anchor stable across releases The '### What's-new popup *(since vNEXT)*' heading put the version gate in the heading itself, so resolving the placeholder to 0.90.0 changed the generated slug to 'whats-new-popup-since-0900' and broke the in-page link at line 138 -- and would have broken it again on every future release. Moved the '(since 0.90.0)' tag to the first body line: the anchor is now the stable 'whats-new-popup', the gate stays visible, and check_version_gates.py still sees it (it scans the whole file, not just headings).
Frontend-only catch-up for the NERD web UI (
web/frontend). Every item below is wired to a serve route that already exists — no Python was touched. Verified againstsrc/keboola_agent_cli/server/routers/before wiring.What shipped
re-runandterminate. Terminate is offered only forcreated/waiting/processing(a terminal job has nothing to stop) and goes throughConfirmModalwith an explicitjob_idslist. Re-run starts a fresh job from the config as it stands now — the Queue API offers no replay of the historicalconfigData, and the code says so. The SSE log stream is unchanged.POST /jobs/{p}/run,POST /jobs/{p}/terminateDrawer(it was an inline card, against the design contract) and gained a Run job action. Fire-and-return (wait=false), then a success line with anopen Jobs →jump.POST /jobs/{p}/runConfirmModal(soft-delete; the modal says so), plus a Trash tab listingdeleted_at+versionwith per-row Restore. Empty state: “Trash is empty — deletes are reversible here.”DELETE /configs/{p}/{component}/{id},GET /configs/trash/{p},POST /configs/{p}/{component}/{id}/restorePageId+ sidebar entry under MANAGE) — fast list by default; a derive last-used toggle re-fetches withwith_last_used=true.never/unknown/errorrender as distinct pills with tooltips and are never collapsed (they lead to opposite decisions), and no client-side re-sort happens because the server already returns dormant-first. Create / rotate / delete; the secret is shown once in a copy-to-clipboardnerd-codeblock with a “shown once” warning, held in React state only and dropped on close. The clipboard call degrades to a manual-copy hint on a non-secure origin.GET /token/{p}/list,POST /token/{p}/create|delete|refreshTableDetailgaineddefinition; the Info tab renders Time partitioning / Range partitioning / Clustering / Partition filter required / Partitions (a count —partitions[]is unbounded). Renders nothing at all when there is no layout.GET /storage/table-detail/{p}/{id}ErrorBoxon failure. A non-emptylegacy_column_descriptionssurfaces a one-linedescribe-migratehint.POST /storage/columns/{p}/{table_id}/describeStatTilescoped to the active project, showing remaining credits and derived minutes.PAYG_NOT_AVAILABLErenders as a mutedn/apill, not an error — it is the normal state on most stacks.GET /billing/creditsconfig_idto the API drops the filter-less catch-alls server-side, and those fire for every job in the project, so a filtered fetch would silently under-report who gets paged. Project-wide subscriptions get their own group with a warning pill, and a note records that abranch.idvalue alone does not mean “dev branch” (production writes the default branch’s numeric id).GET /notificationsCtrl+K/Cmd+Kopens a centered overlay with subsequence fuzzy matching over all pages, all registered projects, and a couple of actions (toggle theme, open Swagger/docs). Arrows + enter, esc closes, cyan match highlighting, green selection bar. The page list is the sidebar’s now-exportedSECTIONS, so a new page can never appear in one surface and not the other. Footer hint added to theStatusBar.window.confirmatpages/Agents.tsx:566replaced withConfirmModal(portaled to<body>: the drawer’sbackdrop-blurmakes it a containing block, so a nested fixed modal would be clipped). DeaduseManageTokenPromptdeleted fromstate.tsx— verified zero consumers first.Verification
npm ci && npx tsc --noEmit && npm run build— all clean.web/frontend;tsc -b && vite buildis the only gate, and it passes.web/frontend/distandsrc/keboola_agent_cli/_ui_distare gitignored and untracked — nothing built was committed.docs/web-server.md“Web UI” section updated with the Tokens page, the command palette, and one line per new capability.Part of the serve/UI audit follow-up (#655/#656/#657 context).
Scope note
Item 10's cleanup also deletes
useManageTokenPromptfromstate.tsx— awindow.prompt-based manage-token helper with zero remaining call sites (verified by grep before removal;tscclean after). It is unrelated to the features above and is called out here so a futuregit blameon that deletion does not have to reconstruct the "why" from an 18-file diff. TheManageTokenModalcomponent that superseded it is untouched.Review follow-ups (commits 2-4)
e65fcc6— Devin: the optimistic column-description override was never cleared on success (masked later server values while the drawer stayed mounted);ConfirmModalnow portals to<body>itself likeDrawer, so confirms raised inside a drawer no longer depend on where they were declared.c44a4f9— live browser verification: the Queue API job row names the config idconfig, notconfigId.types.tshad the wrong name, so the Jobs table's Config column had always rendered empty and the new re-run button'scanRerungate was permanently false — the button never rendered at all.ed66cc2—kbagent-pr-reviewerB-1: re-run droppedbranch_idand silently retargeted the default branch.