feat(web): per-project worker image dashboard control (MNG-1699)#1468
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
APPROVE — clean, well-scoped final plan (4/4) for spec 022. The new ProjectWorkerImage card correctly wires set/clear to projects.update, surfaces the pending → verified → failed lifecycle with 5s polling, and self-hides for non-superadmins. I verified the backend contract, the polling/rules-of-hooks correctness, the test, the typecheck, and the test-only vitest alias.
What I verified
- Backend contract is real:
projects.getByIdreturns all four worker-image columns (getProjectFullselects*,serializeProjectpasses them through);projects.defaultsexposes the globalworkerImage;auth.mereturnsrole. The component'sWorkerImageProjectcast matches the persisted column names. - Superadmin gate is defense-in-depth, not the only control: the UI
if (!isSuperadmin) return nullmirrors the backendprocessWorkerImageChange, which throwsFORBIDDENfor non-superadmins — so the hidden control can't be bypassed via a crafted request. - Polling correctness:
refetchInterval: (query) => …signature and the 5000ms cadence match the existing run-status pages (routes/runs/$runId.tsx); all hooks run before the earlyreturn null, so rules-of-hooks hold. - vitest alias is safe:
@tanstack/react-queryexists only inweb/node_modules, so the new dedupe alias just pins the single real copy. The other react-query web test (stats-page.test.ts) and the new test both pass. - Build/test: new test passes (7/7),
cd web && npx tsc -bpasses, CI 7/7 green. - Placement: the card sits outside
</form>withtype="button"controls — no accidental form submission, consistent with it owning its own mutation.
All MNG-1699 acceptance criteria (placeholder=global default, set/clear → projects.update, lifecycle badges, superadmin-only, docs walkthrough) are addressed.
Minor observations (non-blocking, no action required)
- The verified badge renders the stored digest verbatim, which is the full
repo@sha256:…reference (per the router validator'sresolveDigestFromRepoDigests), not a baresha256:…. That's more informative than the docs/CHANGELOG shorthand "pinned tosha256:…" — flagging only so the slightly longer real-world string isn't a surprise. - For a non-superadmin the
getById/defaultsqueries still fire before the early return (the component renders nothing). Cost is negligible —getByIddedupes with the parent,defaultsis public +staleTime: Infinity, andrefetchIntervalreturnsfalseso there's no ongoing polling.enabled: isSuperadminwould trim it but isn't worth a change.
🕵️ claude-code · claude-opus-4-8 · run details
|
@aaight address conflicts |
Resolve CHANGELOG.md conflict by keeping both Unreleased entries (MNG-1699 dashboard worker image + MNG-1701 setup timeout). The project-general-form.tsx changes from both branches auto-merged cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅ Merged What conflicted: only Auto-merged cleanly: Verification: Pushed as merge commit 🕵️ claude-code · claude-opus-4-8 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
APPROVE — clean, well-scoped final plan (4/4) of spec 022. The new ProjectWorkerImage card wires set/clear to projects.update, surfaces the pending → verified → failed lifecycle with 5s polling, and self-hides for non-superadmins. I verified the backend contract, polling/rules-of-hooks correctness, the test, the web typecheck, and the test-only vitest alias.
What I verified
- Backend contract is real:
projects.getByIdreturns all four worker-image columns (getProjectFulldoesdb.select()→ every column;serializeProjectonly renamesagentEngineSettings),projects.defaultsexposes the globalworkerImage(routerConfig.workerImage), andauth.mereturnsrole. TheWorkerImageProjectcast matches the persisted column names. - Superadmin gate is defense-in-depth, not the only control: the UI
if (!isSuperadmin) return nullmirrors the server —processWorkerImageChangethrowsFORBIDDENfor any non-superadmin actor onprojects.update, so hiding the control isn't bypassable by a crafted request. - Polling correctness: the
refetchInterval: (query) => …signature + 5000ms cadence match the run-status pages (routes/runs/$runId.tsx); all hooks run before the earlyreturn null, so rules-of-hooks hold. Sharing theprojects.getByIdkey with the parent route observer is fine — react-query polls at the shortest active interval and keeps both in sync. - Placement: the card sits outside
</form>(form closes at line 427, card at 458) and both controls aretype="button"— no accidental form submission. - vitest alias is safe:
@tanstack/react-queryexists only underweb/node_modules, so the new dedupe just pins the single real copy; it can't affect backend bundles (frontend-only dep). Both react-query web tests pass (project-worker-image+stats-page, 16/16). - Build/docs:
cd web && npx tsc -bpasses; the CLI flags the docs reference (--worker-image,--clear-worker-image,projects show) exist; CI 7/7 green.
Minor observations (non-blocking, no action required)
- The inline
useMutationre-implements the exactprojects.getById+projects.listFullinvalidation already encapsulated by the shareduseProjectUpdate(projectId)hook (whose doc comment says it exists "to ensure consistent cache invalidation and UX behaviour"). Composing that hook and passing per-callonSuccess/onErrorwould avoid a second copy of the invalidation rule — purely a reuse nicety. - The verified badge renders the stored digest verbatim, which is the full
repo@sha256:…launch reference (per the router validator'sresolveDigestFromRepoDigests), not the baresha256:…shorthand in the docs/CHANGELOG — more informative, just slightly longer than the copy implies.
🕵️ claude-code · claude-opus-4-8 · run details
|
|
||
| const [ref, setRef] = useState(''); | ||
|
|
||
| const mutation = useMutation({ |
There was a problem hiding this comment.
Non-blocking: this useMutation duplicates the projects.getById + projects.listFull invalidation already provided by the shared useProjectUpdate(projectId) hook (web/src/components/projects/use-project-update.ts), whose doc comment says it exists "to ensure consistent cache invalidation and UX behaviour." You could call const mutation = useProjectUpdate(projectId) and pass the worker-image-specific onSuccess/onError (toasts, setRef('')) per .mutate(vars, { ... }) call, keeping the invalidation rule in one place. Functionally identical today — the keys match — so this is just a reuse nicety, not a defect.
Summary
Final plan (4/4) of spec 022 per-project worker image (MNG-1699). Adds the dashboard control on top of the already-merged backend (plans 1–3) and the operator walkthrough docs — completing the feature end-to-end across CLI, API, and dashboard.
A superadmin can now set/clear a project's worker image from Project → Settings → General and watch the router-side validation lifecycle live.
Issue: https://linear.app/issue/MNG-1699
What changed
web/src/components/projects/project-worker-image.tsx(new) — a Worker Image card, superadmin-gated (self-hides for everyone else, mirroring the backendprojects.updategate):projects.defaults.projects.update({ workerImage }); Clear →projects.update({ workerImage: null })(reverts to the global default). Clear only shows when an image is configured.pending(the project query polls every 5 s viarefetchInterval, the same approach as the run-status pages), Verified — pinned tosha256:…with the digest, or Validation failed:<reason>.projects.getById+projects.listFull.workerImagePollInterval(status)helper (poll whilepending, stop otherwise) so the polling rule is unit-tested deterministically.web/src/components/projects/project-general-form.tsx— renders<ProjectWorkerImage projectId={project.id} />as a standalone card (it owns its own mutation, so it sits outside the form's Save/Reset).docs/getting-started.md— Per-Project Worker Image (Advanced) operator walkthrough: derive an imageFROMthe Cascade worker base (hard/soft runtime requirements + non-rootnodeuser), make it available in both the registry-backed and self-hosted/local topologies, set it from the dashboard (or CLI), and confirmverified; plus the deliberately-broken-image →failedcheck.CHANGELOG.md— Unreleased entry.vitest.config.ts— dedupe@tanstack/react-queryto the single (web-workspace) copy, mirroring the existing@trpc/clientdedupe. Without it avi.mock('@tanstack/react-query')resolved from atests/file misses the component's web-resolved import and the realuseQueryClientruns against a second React copy (Cannot read properties of null (useContext)). Test-only; no production-bundle impact.Tests
tests/unit/web/project-worker-image.test.ts(new, jsdom +@testing-library/react, react-query/tRPC mocked) covers every TDD case: placeholder = global default when unset; Set callsprojects.updatewith the reference; Clear calls it withnull; pending shows Verifying… and the poll predicate returns the interval; verified shows the digest; failed shows the reason; the control is hidden for a non-superadmin..test.ts(rendering viacreateElement, matching the repo's existing web-component test convention) rather than.test.tsx, so it is picked up by the existingtests/unit/web/**/*.test.tsglob with no test-runner JSX-transform changes.Verification
npm test— 10490 passed, 0 failed (full unit suite).npm run typecheck(root) ✓ ·cd web && npm run build(tsc -b && vite build) ✓.npx biome checkon all changed files ✓.Scope notes
web/+docs/+ the test + the test-only config alias.[manual]step by design.🕵️ claude-code · claude-opus-4-8 · run details