feat(web): Studio Builder PR-3 — connected mode#2051
Open
seanrobertwright wants to merge 2 commits into
Open
Conversation
PR-3 of the Archon Studio integration. Replaces the fixture-backed builder route with a connected route that loads, edits, creates, renames, and deletes real workflow YAML through the existing workflow CRUD endpoints (no server changes). - BuilderConnected at /console/builder[/:name]: project picker (per-cwd discovery, persisted + ?project= deep-link), workflow open/New/Rename/Delete, explicit Save with a dirty indicator and beforeunload + confirm nav guard - skills/workflows.ts: loadWorkflow/saveWorkflow/deleteWorkflow/validateWorkflow verbs + pure buildWorkflowPath/buildSavePath helpers - connect/save-logic.ts: pure serverErrorToIssues/serverValidationToIssues/ blockingErrors/planRename/isValidWorkflowName (mirrors server isValidCommandName) - BuilderPage gains an additive extraIssues prop; import + server issues merge into the existing IssueList - bundled workflows open read-only, Save-as writes a project override; filename (:name) and in-YAML name: stay in sync - HttpError exposes bodySnippet; K.workflow(cwd,name) cache key - 19 new unit tests (save-logic, workflows path builders) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a connected workflow-builder route with live project selection, workflow CRUD, save/rename/delete flows, validation issue merging, and route wiring that replaces the fixture-based builder route. Supporting changes add workflow API helpers, pure save logic, cache keys, and updated builder documentation. ChangesConnected Builder Mode
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: coleam00, leex279 Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/web/src/experiments/console/builder/BuilderConnected.tsx (1)
95-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReplace arbitrary UI values with design tokens.
This new UI introduces arbitrary typography/radius/width classes such as
text-[12.5px],rounded-[8px], andmax-w-[220px]. Please use existing Archon token utilities or add tokens first. As per coding guidelines, “Use brand tokens, not ad-hoc values; colors, gradients, surfaces, typography must come from design tokens or brand guide, not hard-coded hex values.”Also applies to: 451-566, 592-607
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/experiments/console/builder/BuilderConnected.tsx` around lines 95 - 96, Replace the ad-hoc Tailwind values in BuilderConnected and the other affected sections with design-token-based classes or token utilities instead of arbitrary typography/width/radius values like text-[12.5px], rounded-[8px], and max-w-[220px]. Update the relevant UI wrappers and components in BuilderConnected to use existing Archon design tokens, or add the missing tokens first, and then switch the affected usages throughout the referenced areas to those named tokens.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/experiments/console/builder/BuilderConnected.tsx`:
- Around line 443-444: The current not-found logic in BuilderConnected treats
any `loadView.error` as a missing workflow, so update the workflow-loading flow
to distinguish a 404 from other failures. Use the existing
`loadView`/`workflowOpen`/`notFound` branching in `BuilderConnected` to only
show the “No workflow named…” empty state when the error is truly not found, and
render the `workflowLoadError` path for 500/network/auth failures the same way
the list-load banner does. Also update the related render block around the
workflow load error handling so non-404 errors surface as load failures instead
of not-found.
- Around line 412-417: The empty project selection in onPickProject is being
treated as an empty string instead of no selection, which breaks the useCallback
flow and the confirmIfDirty/setProjectId/navigation contract. Normalize the
“Select a project…” option to undefined before calling setProjectId and building
the /console/builder URL, and make sure the project query param is omitted when
there is no selection rather than persisting project=.
- Around line 286-299: In doDelete within BuilderConnected, the busy state is
set before deleteWorkflow but is only cleared in the catch path, so a successful
delete leaves the shared BuilderConnected instance stuck busy. Update the
success flow after deleteWorkflow, invalidate calls, and navigate to reset busy
to false (or use a finally block that always clears it) while preserving the
existing error handling and references to deleteWorkflow, invalidate, and
navigate.
In `@packages/web/src/experiments/console/builder/connect/use-builder-project.ts`:
- Around line 35-48: useBuilderProject only seeds projectId from the initial
?project= value, so later URL navigation is ignored and can be overwritten by
BuilderConnected. Update the hook to react to search param changes from
useSearchParams and keep projectId in sync when the project query changes while
the route stays mounted, while still preserving the existing
setProjectId/writeStored behavior for explicit user-driven updates.
In `@packages/web/src/experiments/console/store/keys.ts`:
- Line 14: The K.workflow key is ambiguous because it concatenates cwd and name
with ":" while workflow names can also contain ":". Update the workflow key
generation in the K.workflow helper to use an unambiguous encoding or delimiter
scheme, and make sure the save-logic flow in BuilderConnected still produces and
consumes the same key shape so distinct (cwd, name) pairs cannot collide.
---
Nitpick comments:
In `@packages/web/src/experiments/console/builder/BuilderConnected.tsx`:
- Around line 95-96: Replace the ad-hoc Tailwind values in BuilderConnected and
the other affected sections with design-token-based classes or token utilities
instead of arbitrary typography/width/radius values like text-[12.5px],
rounded-[8px], and max-w-[220px]. Update the relevant UI wrappers and components
in BuilderConnected to use existing Archon design tokens, or add the missing
tokens first, and then switch the affected usages throughout the referenced
areas to those named tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5eee84ed-d74e-47d9-acf7-11700e13806d
📒 Files selected for processing (14)
packages/web/src/experiments/console/ConsoleApp.tsxpackages/web/src/experiments/console/README.mdpackages/web/src/experiments/console/builder/BuilderConnected.tsxpackages/web/src/experiments/console/builder/BuilderPage.tsxpackages/web/src/experiments/console/builder/BuilderRoute.tsxpackages/web/src/experiments/console/builder/CONTEXT.mdpackages/web/src/experiments/console/builder/README.mdpackages/web/src/experiments/console/builder/connect/save-logic.test.tspackages/web/src/experiments/console/builder/connect/save-logic.tspackages/web/src/experiments/console/builder/connect/use-builder-project.tspackages/web/src/experiments/console/lib/http.tspackages/web/src/experiments/console/skills/workflows.test.tspackages/web/src/experiments/console/skills/workflows.tspackages/web/src/experiments/console/store/keys.ts
💤 Files with no reviewable changes (1)
- packages/web/src/experiments/console/builder/BuilderRoute.tsx
Contributor
Author
PR Review Summary — multi-agent (7 agents)7 specialized agents reviewed the diff ( Critical Issues (0)None. No correctness-blocking or security issues; isolation guard, no- Important Issues (2 — recommend fixing before merge)
Suggestions (nice to have)
Documentation Issues
(Confirmed correct: the new Strengths
VerdictNEEDS FIXES — no blockers, but the two Important items (silent blocked-save on empty Recommended Actions
🤖 Generated via /prp-review-agents (code-reviewer · docs-impact · pr-test-analyzer · comment-analyzer · silent-failure-hunter · type-design-analyzer · code-simplifier) |
…nnected mode CodeRabbit: - doDelete clears busy via finally (toolbar no longer locks after a delete) - normalize the empty "Select a project…" option to undefined (no bare project= shadowing the persisted default) - distinguish 404 from 500/403/network on workflow load — non-404 shows a load error state instead of misleading "not found → Create" - useBuilderProject follows later ?project= changes while mounted (back/forward) - K.workflow encodes both parts (names may contain ':', avoiding key collisions) Multi-agent review: - validateWorkflow valid:false with empty errors surfaces a fallback issue (no silent blocked Save); ValidateWorkflowResponse is now a discriminated union - re-export WorkflowSource from primitives/workflow (single source of truth) - extract errorToIssues/renameReasonMessage/clientIssue/errorDetail/ validationFailureToIssues into save-logic (pure + unit-tested) - drop unused RenamePlan.steps; readonly extraIssues; verb-neutral HttpError fallback; rename-delete warning uses parsed detail; comment precision + doc staleness (PR-2 merged; Status) + drop gitignored Spike refs Tests: +14 (352 web pass / 0 fail). type-check / format / console lint green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Follow-up: multi-agent review — actions takenAll findings from the 7-agent review above have been addressed in 583e587 (on top of 835f596). The two Important items and every actionable suggestion were applied; one simplification was intentionally skipped (justified below). Gates re-run green: repo-root Important (both applied)
Suggestions (applied)
Not applied (justified)
Thanks to the review agents — the silent-block and load-error items were the highest-value catches. 🤖 review-response generated with Claude Code |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds “connected mode” to the experimental Console workflow builder so it can load, validate, and persist real workflow YAMLs through existing /api/workflows* endpoints (plus project selection, explicit Save flow, and CRUD operations), completing the console-side wiring for the Archon Studio builder.
Changes:
- Introduces a new connected builder route (
/console/builder[/:name]) with project selection, load/save/rename/delete, dirty tracking, and a navigation guard. - Adds workflow CRUD/validation “skill” helpers (pure URL builders + request wrappers) and supporting pure save/rename/issue logic with unit tests.
- Extends
BuilderPageto accept additiveextraIssuesso server/import issues can be merged into the existing issue panel.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/experiments/console/store/keys.ts | Adds a safer cache key for single-workflow entities by encoding (cwd, name). |
| packages/web/src/experiments/console/skills/workflows.ts | Adds connected-mode workflow CRUD + validation helpers (build paths + load/save/delete/validate). |
| packages/web/src/experiments/console/skills/workflows.test.ts | Unit tests for workflow path builder helpers. |
| packages/web/src/experiments/console/README.md | Documents the new /console/builder routes and persisted builder project key. |
| packages/web/src/experiments/console/lib/http.ts | Enhances HttpError to retain a truncated server body snippet for better UX error surfacing. |
| packages/web/src/experiments/console/ConsoleApp.tsx | Switches routing from fixture-backed builder to BuilderConnected (and adds builder/:name). |
| packages/web/src/experiments/console/builder/README.md | Updates builder documentation to reflect PR-3 connected mode shipping in the console experiment. |
| packages/web/src/experiments/console/builder/CONTEXT.md | Adds a terminology glossary for the builder/Studio domain language. |
| packages/web/src/experiments/console/builder/connect/use-builder-project.ts | Adds persisted project selection state (localStorage + ?project= deep-link seeding). |
| packages/web/src/experiments/console/builder/connect/save-logic.ts | Adds pure helper logic for save/rename flows and mapping errors/validation into issues. |
| packages/web/src/experiments/console/builder/connect/save-logic.test.ts | Unit tests for the pure save/rename/issue helpers. |
| packages/web/src/experiments/console/builder/BuilderRoute.tsx | Removes the old fixture-backed builder route. |
| packages/web/src/experiments/console/builder/BuilderPage.tsx | Adds extraIssues prop and merges it into the issue panel with dedupe-by-id. |
| packages/web/src/experiments/console/builder/BuilderConnected.tsx | New connected route component implementing project picker, loading, CRUD, Save, dirty/nav guard, and issue surfacing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+30
to
+41
| export function serverErrorToIssues(err: HttpError): Issue[] { | ||
| let message = err.bodySnippet || `Request failed (${String(err.status)})`; | ||
| try { | ||
| const parsed = JSON.parse(err.bodySnippet) as { error?: string; detail?: string }; | ||
| if (parsed.error) { | ||
| message = parsed.detail ? `${parsed.error}: ${parsed.detail}` : parsed.error; | ||
| } | ||
| } catch { | ||
| /* truncated/non-JSON body — keep the raw snippet */ | ||
| } | ||
| return [serverIssue('server.validation', message)]; | ||
| } |
Comment on lines
+75
to
+78
| export function errorToIssues(e: unknown, rule: string, fallback: string): Issue[] { | ||
| if (e instanceof HttpError) return serverErrorToIssues(e); | ||
| return [serverIssue(rule, e instanceof Error ? e.message : fallback)]; | ||
| } |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/experiments/console/builder/BuilderConnected.tsx (1)
279-330: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRename is reachable on read-only bundled workflows — Delete guards
readOnly, Rename doesn't.The Rename button is gated only by
!isCreateMode(Line 493), while Delete correctly requires!readOnly && !isCreateMode(Line 505). For an unsaved bundled workflow (effectiveSource === 'bundled', soreadOnly === true), Rename is still clickable.
doRenamethen: saves the new name undersaveTargetFor(effectiveSource)='project'(fine, acts like a scoped save-as), but attempts to delete the old bundled name also undersource: 'project'— a project file with the old bundled name never existed, so this either surfaces a spurious "removing the old file failed... delete it manually" warning (misleading, since nothing needed deleting), or, if a staleexistingNames/source cache masks an actual same-named project file, could delete an unrelated file. "Rename" has no coherent semantics for a shared, immutable bundled default — it should be excluded the same way Delete is.🐛 Proposed fix
- if (name === undefined || cwd === undefined || currentWorkflow === null) return; + if (name === undefined || cwd === undefined || currentWorkflow === null || readOnly) return;- }, [name, cwd, currentWorkflow, existingNames, effectiveSource, navigate, projectQuery]); + }, [name, cwd, currentWorkflow, existingNames, effectiveSource, readOnly, navigate, projectQuery]);- {!isCreateMode ? ( + {!readOnly && !isCreateMode ? ( <button type="button" disabled={busy} onClick={(): void => { void doRename(); }}Also applies to: 493-504
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/experiments/console/builder/BuilderConnected.tsx` around lines 279 - 330, The Rename action in BuilderConnected should be disabled for read-only bundled workflows, matching the Delete guard. Update the rename button gating and the doRename flow so it exits early when readOnly is true or effectiveSource is bundled, using the existing readOnly/effectiveSource checks alongside doRename, planRename, and saveTargetFor. This prevents saving a misleading “rename” of an immutable bundled workflow and avoids the attempted delete of the old bundled name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/web/src/experiments/console/builder/BuilderConnected.tsx`:
- Around line 279-330: The Rename action in BuilderConnected should be disabled
for read-only bundled workflows, matching the Delete guard. Update the rename
button gating and the doRename flow so it exits early when readOnly is true or
effectiveSource is bundled, using the existing readOnly/effectiveSource checks
alongside doRename, planRename, and saveTargetFor. This prevents saving a
misleading “rename” of an immutable bundled workflow and avoids the attempted
delete of the old bundled name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5b94e6f-078b-4957-8a83-7a1af54ab1e7
📒 Files selected for processing (11)
packages/web/src/experiments/console/README.mdpackages/web/src/experiments/console/builder/BuilderConnected.tsxpackages/web/src/experiments/console/builder/BuilderPage.tsxpackages/web/src/experiments/console/builder/README.mdpackages/web/src/experiments/console/builder/connect/save-logic.test.tspackages/web/src/experiments/console/builder/connect/save-logic.tspackages/web/src/experiments/console/builder/connect/use-builder-project.tspackages/web/src/experiments/console/lib/http.tspackages/web/src/experiments/console/skills/workflows.test.tspackages/web/src/experiments/console/skills/workflows.tspackages/web/src/experiments/console/store/keys.ts
✅ Files skipped from review due to trivial changes (2)
- packages/web/src/experiments/console/builder/README.md
- packages/web/src/experiments/console/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/web/src/experiments/console/skills/workflows.test.ts
- packages/web/src/experiments/console/store/keys.ts
- packages/web/src/experiments/console/lib/http.ts
- packages/web/src/experiments/console/builder/BuilderPage.tsx
- packages/web/src/experiments/console/skills/workflows.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/console/builder[/:name]) that loads/saves real workflows via the existing CRUD endpoints, a project picker, explicit Save with a dirty + nav guard, full create/rename/delete, server-tier validation surfaced into the issue panel, and bundled→Save-as.GET/PUT/DELETE /api/workflows/:name,POST /api/workflows/validate,GET /api/commandsuntouched), no DB, no other package, no production/workflows/builderroute.BuilderPagestays a controlled component — its only change is an additiveextraIssuesprop.UX Journey
Before
After
Architecture Diagram
After
Connection inventory:
builder+builder/:nameextraIssuespropLabel Snapshot
risk: lowsize: Mwebweb:console-builderChange Metadata
featurewebLinked Issue
dev. This branch is rebased ontoupstream/dev, so the PR contains only the single PR-3 commit.Validation Evidence (required)
upstream/dev(59bbd00b) — type-check (0), format (0), web tests 338 pass / 0 fail, and the PR-3/console files lint clean. All gates green.bun run validate's only failure is a pre-existing, environment-specific Windows test in@archon/providers(pathKind > broken symlink→EPERM; symlink creation needs Developer Mode/admin). That package is untouched by this PR and the test passes on Linux CI.Security Impact (required)
NoNo(reuses existing workflow endpoints)NoNo(writes go through the existing server-validatedPUT, which validatescwdagainst registered codebases)Compatibility / Migration
YesNoNoHuman Verification (required)
source:'server'issue; blocking client errors gate Save; rename collision/no-op/invalid-name blocked; rename delete-fail → non-fatal warning; bundled Save-as; create-on-save flips to edit mode; controlled-select revert on cancelled nav-guard;beforeunloadteardown.Side Effects / Blast Radius (required)
packages/web/src/experiments/console/only.Rollback Plan (required)
af0ba60b); nothing outside the console experiment changes./console/*experiment surface).source:'server'error.Risks and Mitigations
ProjectRailclicks (the app is a non-data<BrowserRouter>, souseBlockeris unavailable).beforeunload+ confirm-on-intercept on the builder's own controls; documented as a known limitation; data-router migration is out of scope.🤖 Generated with Claude Code
Summary by CodeRabbit
?project=sync) and full CRUD: New, Save, Save as (bundled read-only), Rename, Delete.