feat(admin): add browser admin UI and draft management#5
Conversation
Add backend draft-management and admin UI routes, plus a React-based browser admin interface for current state, config editing, and prompt file workflows. Document the operator workflow and frontend stack boundaries, and cover the new admin surface with frontend and backend tests.
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a browser-based admin UI and backend control plane: an in-memory draft/apply workflow (AdminDraftService), a Flask admin blueprint exposing draft/profile/instance/prompt/runtime endpoints and static UI serving, registry validation refactors, atomic prompt/profile/instance writes, and a full React+Vite admin SPA with query/mutation wiring and tests. Changes
Sequence DiagramsequenceDiagram
participant User as User (Browser)
participant SPA as React SPA
participant API as Flask Admin API
participant Draft as AdminDraftService
participant Registry as Instance/Profile Registry
participant YAML as Filesystem (YAML & prompts)
User->>SPA: Navigate to /admin/ui
SPA->>API: GET /admin/draft
API->>Draft: get_draft()
Draft->>Registry: load/reset from repo
Registry->>YAML: read profiles & instances YAML
YAML-->>Registry: parsed data
Registry-->>Draft: initial draft state
Draft-->>API: DraftState (profiles, instances, dirty)
API-->>SPA: JSON DraftState
SPA->>User: render dashboard
User->>SPA: Edit draft (profile/instance)
SPA->>API: PUT /admin/profiles/{id} or /admin/instances/{id}
API->>Draft: update_*()
Draft->>Draft: mutate in-memory, set dirty
Draft-->>API: updated DraftState
API-->>SPA: updated DraftState
User->>SPA: Validate/Preview/Apply
SPA->>API: POST /admin/draft/validate|/preview|/apply
API->>Draft: validate()/preview()/apply()
Draft->>Registry: normalize via validate_*_data
Draft->>YAML: stage serialized YAML -> atomic replace on apply
YAML-->>Draft: success/failure
Draft-->>API: validation/preview/result
API-->>SPA: response (errors or applied state)
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive browser-based admin UI for ChatMock, featuring a new operator-first console with distinct modes for viewing current state, editing configuration drafts, and managing prompt files. The backend is expanded with an AdminDraftService and dedicated admin routes to support in-memory draft/apply workflows and atomic YAML-backed configuration updates. Key feedback points include a critical issue with unstable function references in App.tsx that could cause infinite re-fetch loops, concerns regarding the lack of transactionality and atomicity in configuration and prompt file writes, and the maintenance burden of manual field mapping in the registry serializers.
| async function saveProfile(profileId: string, profile: Profile) { | ||
| clearDraftDerivedState(); | ||
| await runAction("Draft profile saved.", () => | ||
| profileMutations.saveProfile.mutateAsync({ profileId, profile }), | ||
| ); | ||
| } | ||
|
|
||
| async function createProfile(profile: Profile) { | ||
| clearDraftDerivedState(); | ||
| await runAction("Draft profile created.", () => profileMutations.createProfile.mutateAsync(profile)); | ||
| } | ||
|
|
||
| async function deleteProfile(profileId: string) { | ||
| clearDraftDerivedState(); | ||
| await runAction("Draft profile removed.", () => profileMutations.deleteProfile.mutateAsync(profileId)); | ||
| } | ||
|
|
||
| async function saveInstance(instanceId: string, instance: Instance) { | ||
| clearDraftDerivedState(); | ||
| await runAction("Draft instance saved.", () => | ||
| instanceMutations.saveInstance.mutateAsync({ instanceId, instance }), | ||
| ); | ||
| } | ||
|
|
||
| async function createInstance(instance: Instance) { | ||
| clearDraftDerivedState(); | ||
| await runAction("Draft instance created.", () => instanceMutations.createInstance.mutateAsync(instance)); | ||
| } | ||
|
|
||
| async function deleteInstance(instanceId: string) { | ||
| clearDraftDerivedState(); | ||
| await runAction("Draft instance removed.", () => instanceMutations.deleteInstance.mutateAsync(instanceId)); | ||
| } | ||
|
|
||
| async function validateDraft() { | ||
| const summary = await runAction("Draft validated.", () => draftMutations.validateDraft.mutateAsync()); | ||
| setDraftValidation(summary); | ||
| } | ||
|
|
||
| async function previewDraft() { | ||
| const preview = await runAction("Draft preview refreshed.", () => draftMutations.previewDraft.mutateAsync()); | ||
| setDraftPreview(preview); | ||
| setDraftValidation(preview.validation); | ||
| } | ||
|
|
||
| async function applyDraft() { | ||
| await runAction("Draft applied to YAML.", () => draftMutations.applyDraft.mutateAsync()); | ||
| clearDraftDerivedState(); | ||
| } | ||
|
|
||
| async function resetDraft() { | ||
| await runAction("Draft reset to current config.", () => draftMutations.resetDraft.mutateAsync()); | ||
| clearDraftDerivedState(); | ||
| } | ||
|
|
||
| async function validateRuntime() { | ||
| await runAction("Runtime validation refreshed.", () => runtimeMutations.validateRuntime.mutateAsync()); | ||
| } | ||
|
|
||
| async function reloadPrompts() { | ||
| await runAction("Prompt files reloaded.", () => promptMutations.reloadPrompts.mutateAsync()); | ||
| } | ||
|
|
||
| async function redeployRuntime() { | ||
| return runAction("Redeploy requested.", () => runtimeMutations.redeployRuntime.mutateAsync()); | ||
| } | ||
|
|
||
| async function loadPromptFiles(payload: { base_prompt_path: string; codex_prompt_path: string }) { | ||
| return runAction("Prompt files loaded.", () => promptMutations.readPromptFiles.mutateAsync(payload)); | ||
| } | ||
|
|
||
| async function savePromptFiles(payload: PromptFilePayload) { | ||
| const result = await runAction("Prompt files saved.", () => promptMutations.writePromptFiles.mutateAsync(payload)); | ||
| return result.next; | ||
| } |
There was a problem hiding this comment.
Several action functions (like saveProfile, loadPromptFiles, etc.) are recreated on every render of the App component. Since these are passed as dependencies to useEffect hooks in child components (e.g., in PromptFilesPage.tsx), this will trigger infinite re-fetch loops whenever an action updates the app state (like setting a notice). All functions passed to child components should be wrapped in useCallback.
References
- Functions passed as dependencies to hooks or as props to memoized components should be stable to prevent unnecessary effects or re-renders. (link)
| def apply(self) -> dict[str, Any]: | ||
| validation = self.validate() | ||
| if not validation["ok"]: | ||
| raise ValueError("; ".join(validation["errors"])) | ||
| service = self._build_draft_service() | ||
| self._write_yaml_directory( | ||
| self.profiles_root, | ||
| service.list_profiles(), | ||
| serializer=serialize_profile_config, | ||
| ) | ||
| self._write_yaml_directory( | ||
| self.instances_root, | ||
| service.list_instances(), | ||
| serializer=serialize_instance_config, | ||
| ) |
There was a problem hiding this comment.
The apply method lacks transactionality across the two configuration directories. If writing the profiles_root succeeds but writing the instances_root fails (e.g., due to disk space or permissions), the system will be left in an inconsistent state where profiles are updated but instances are not. This is particularly risky if an instance depends on a profile that was just deleted or renamed in the draft.
| for temp_path in temp_root.glob("*.yaml"): | ||
| os.replace(temp_path, root / temp_path.name) | ||
| for existing in root.glob("*.yaml"): | ||
| if existing.name not in expected_names: | ||
| existing.unlink() |
There was a problem hiding this comment.
The directory update logic in _write_yaml_directory is not atomic. It replaces files individually and then unlinks orphans in a separate loop. If the process crashes mid-loop, the configuration directory will be corrupted with a mix of old and new files. A more robust approach would be to prepare the entire directory in a temporary location and then perform a directory swap or use a more atomic synchronization method.
| base_path = _resolve_prompt_file_path(base_prompt_path) | ||
| codex_path = _resolve_prompt_file_path(codex_prompt_path) | ||
| write_prompt_text(base_path, base_prompt_text) | ||
| write_prompt_text(codex_path, codex_prompt_text) |
There was a problem hiding this comment.
Sequential file writes in admin_prompt_files_write are not atomic. If the first call to write_prompt_text succeeds but the second fails, the disk state will be inconsistent (one prompt file updated, the other not). Consider using a temporary staging area or a transaction-like wrapper if these files must be updated together.
| def serialize_instance_config(instance: dict[str, Any]) -> dict[str, Any]: | ||
| data: dict[str, Any] = { | ||
| "id": instance["id"], | ||
| "label": instance["label"], | ||
| "profile_id": instance["profile_id"], | ||
| "bind_host": instance["bind_host"], | ||
| "port": instance["port"], | ||
| "runtime": instance["runtime"], | ||
| "prompt_config_path": instance["prompt_config_path"], | ||
| "state_group": instance["state_group"], | ||
| "compose_service_name": instance["compose_service_name"], | ||
| "container_name": instance["container_name"], | ||
| "env_overrides": dict(instance["env_overrides"]), | ||
| "healthcheck": dict(instance["healthcheck"]), | ||
| "ui": { | ||
| "order": instance["ui"]["order"], | ||
| "mutable_fields": list(instance["ui"]["mutable_fields"]), | ||
| }, | ||
| "enabled": instance["enabled"], | ||
| } | ||
| env_prefix = instance.get("env_prefix") | ||
| if isinstance(env_prefix, str) and env_prefix.strip(): | ||
| data["env_prefix"] = env_prefix | ||
| return data |
There was a problem hiding this comment.
The serialize_instance_config function uses manual field mapping. This creates a maintenance burden where any schema changes in the registry must be manually mirrored here, or data will be lost during serialization. Consider using a more automated approach, such as a dataclass or a schema-based serializer that handles all fields by default.
| def serialize_profile_config(profile: dict[str, Any]) -> dict[str, Any]: | ||
| data = { | ||
| "id": profile["id"], | ||
| "label": profile["label"], | ||
| "description": profile["description"], | ||
| "prompt_dir": profile["prompt_dir"], | ||
| "base_prompt_file": Path(profile["base_prompt_path"]).name, | ||
| "codex_prompt_file": Path(profile["codex_prompt_path"]).name, | ||
| "runtime_defaults": dict(profile["runtime_defaults"]), | ||
| "ui": dict(profile["ui"]), | ||
| } | ||
| return data |
There was a problem hiding this comment.
Similar to the instance registry, serialize_profile_config relies on manual field mapping. This is error-prone and likely to lead to data loss if new fields are added to the profile schema but forgotten in this serializer.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc49250f39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| normalized["base_prompt_file"] = Path(base_prompt_path).name | ||
| if "codex_prompt_file" not in normalized and isinstance(codex_prompt_path, str) and codex_prompt_path.strip(): | ||
| normalized["codex_prompt_file"] = Path(codex_prompt_path).name |
There was a problem hiding this comment.
validate_profiles_data() routes draft entries through _coerce_profile_data(), and this conversion uses Path(...).name, which drops any nested directory segment from base_prompt_path/codex_prompt_path. If a profile intentionally uses subpaths under prompt_dir (for example nested/prompt.md), unchanged drafts can fail validation/preview because _normalize_profile() checks the wrong on-disk file.
Useful? React with 👍 / 👎.
| "base_prompt_file": Path(profile["base_prompt_path"]).name, | ||
| "codex_prompt_file": Path(profile["codex_prompt_path"]).name, |
There was a problem hiding this comment.
serialize_profile_config() also truncates prompt file paths to basename, so POST /admin/draft/apply rewrites base_prompt_file/codex_prompt_file without their directory components. For profiles that rely on nested prompt files, this silently corrupts YAML and can break subsequent loads even when the draft was otherwise valid.
Useful? React with 👍 / 👎.
| return deepcopy(draft) | ||
|
|
||
| def reset(self) -> dict[str, Any]: | ||
| service = build_instance_service(repo_root=self.repo_root) |
There was a problem hiding this comment.
AdminDraftService accepts profiles_root and instances_root, but reset() reloads state through build_instance_service(repo_root=self.repo_root) and ignores those explicit roots. When non-default roots are configured, the draft can be initialized from one location while apply() writes to another, causing the admin UI state and persisted config to diverge.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
ui/admin/src/features/edit-config/EditConfigForms.test.tsx-32-36 (1)
32-36:⚠️ Potential issue | 🟡 MinorAssert the callback after validation settles.
The current
not.toHaveBeenCalled()checks happen before the async validation message is awaited, so a delayed incorrect submit could slip past this assertion.🧪 Proposed test ordering fix
fireEvent.click(screen.getByRole("button", { name: "New Profile" })); fireEvent.click(screen.getByRole("button", { name: "Create Draft Profile" })); - expect(onCreateProfile).not.toHaveBeenCalled(); expect(await screen.findByText("Profile ID is required.")).toBeInTheDocument(); + expect(onCreateProfile).not.toHaveBeenCalled(); @@ fireEvent.click(screen.getByRole("button", { name: "New Instance" })); fireEvent.click(screen.getByRole("button", { name: "Create Draft Instance" })); - expect(onCreateInstance).not.toHaveBeenCalled(); expect(await screen.findByText("Instance ID is required.")).toBeInTheDocument(); + expect(onCreateInstance).not.toHaveBeenCalled();Also applies to: 82-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/features/edit-config/EditConfigForms.test.tsx` around lines 32 - 36, The test asserts onCreateProfile before async validation completes; wait for the validation message then assert the callback wasn't called. Specifically, in the "Create Draft Profile" flow use await screen.findByText("Profile ID is required.") (or the equivalent awaited query) before checking expect(onCreateProfile).not.toHaveBeenCalled(); apply the same ordering fix to the other similar block that triggers the create action (the second "Create Draft Profile" / validation sequence).docs/superpowers/plans/2026-04-22-chatmock-browser-admin-ui-operator-first-refresh.md-30-60 (1)
30-60:⚠️ Potential issue | 🟡 MinorUpdate the plan to match the React Router implementation.
These steps still describe hash routing and shell mounting in
App.tsx, but the added UI usesui/admin/src/router.tsxwithcreateBrowserRouter(..., { basename: "/admin/ui" })andAdminLayout. Please revise or mark this plan as historical so future workers don’t follow the obsolete route model.Also applies to: 169-208, 279-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-22-chatmock-browser-admin-ui-operator-first-refresh.md` around lines 30 - 60, The plan's steps assume hash routing in App.tsx but the admin UI now uses React Router in ui/admin/src/router.tsx (createBrowserRouter with basename "/admin/ui") and AdminLayout; update the plan to either: a) replace all hash-routing instructions (steps about default hash route, NAV_ITEMS in App.tsx, and shell mounting) with guidance that changes must be made in router.tsx and AdminLayout (reference createBrowserRouter and AdminLayout), or b) mark the current task as historical/obsolete and annotate that App.tsx no longer controls routing so future workers should edit ui/admin/src/router.tsx and related components instead; ensure the plan text references ui/admin/src/router.tsx, createBrowserRouter, AdminLayout, and App.tsx so readers know which files are deprecated versus authoritative.docs/superpowers/specs/2026-04-22-chatmock-admin-ui-icon-design.md-38-62 (1)
38-62:⚠️ Potential issue | 🟡 MinorMake the icon spec reproducible.
“The uploaded PNG” on Line 40 is not durable once this leaves the PR context. Please name the checked-in source asset path and required output dimensions/manifest metadata, especially
sizesandpurposefor maskable icons.Suggested spec tightening
### Source Asset -Use the uploaded PNG as the canonical source image. +Use the checked-in source image under `ui/admin/public/icons/` as the canonical source asset. ### Generated Outputs Generate and store install icon assets under an admin-UI-owned public path, for example: -- `ui/admin/public/icons/apple-touch-icon.png` -- `ui/admin/public/icons/icon-192.png` -- `ui/admin/public/icons/icon-512.png` -- `ui/admin/public/icons/icon-maskable-512.png` +- `ui/admin/public/icons/apple-touch-icon.png` (180x180) +- `ui/admin/public/icons/icon-192.png` (192x192) +- `ui/admin/public/icons/icon-512.png` (512x512) +- `ui/admin/public/icons/icon-maskable-512.png` (512x512, maskable safe zone) @@ -- icon entries for standard and maskable installs +- icon entries with explicit `src`, `sizes`, `type`, and `purpose` values for standard and maskable installs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-22-chatmock-admin-ui-icon-design.md` around lines 38 - 62, Specify a durable checked-in source file (e.g. ui/admin/public/source/icon.png) and list exact output image filenames with pixel sizes: icon-192.png (192x192), icon-512.png (512x512), apple-touch-icon.png (180x180), icon-maskable-512.png (512x512 maskable) and optional favicon.ico/32x32.png; update the manifest.webmanifest entry for name/short_name/start_url/scope/display/background_color/theme_color and add icons array entries with explicit "src", "sizes" (e.g. "192x192", "512x512"), "type" ("image/png") and "purpose" ("any" for standard icons, "maskable" or "maskable any" for the maskable icon) so the asset generation and manifest are fully reproducible.ui/admin/src/forms/profileForm.ts-15-29 (1)
15-29:⚠️ Potential issue | 🟡 Minor
nextOrderassumes the profiles array is sorted byui.order.
profiles[profiles.length - 1]?.ui.orderuses whatever profile happens to be last in the array. If the backend returns profiles in id/insertion order (or the consumer has resorted), the "next" order can be smaller than an existing one, violating the ordering invariant and producing duplicate/lower order values. Use the max across all profiles instead:Proposed fix
- const nextOrder = (profiles[profiles.length - 1]?.ui.order ?? 0) + 10; + const maxOrder = profiles.reduce( + (max, p) => (p.ui.order > max ? p.ui.order : max), + 0, + ); + const nextOrder = maxOrder + 10;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/forms/profileForm.ts` around lines 15 - 29, buildNewProfileFormValues assumes profiles is sorted and uses profiles[profiles.length - 1]?.ui.order for nextOrder; instead compute the maximum ui.order across all profiles to avoid duplicates or lower values. In buildNewProfileFormValues, iterate over profiles (or use profiles.map) to extract each p.ui.order, compute Math.max over them (defaulting to 0 when profiles is empty), then set nextOrder = maxOrder + 10 and use that for ui_order; keep the rest of the function unchanged.ui/admin/src/components/ui/button.tsx-8-22 (1)
8-22:⚠️ Potential issue | 🟡 MinorDefault variant has no working hover background for plain
<button>.Line 12:
default: "bg-primary text-primary-foreground [a]:hover:bg-primary/80". The arbitrary variant[a]:hover:compiles to a selector that only matches when the element is (or wraps) an<a>. That means whenasChildis false (the common case — a real<button>), there is no hover style at all. Compare to the other variants, which use plainhover:…. Almost certainly a copy/paste slip:Proposed fix
- default: "bg-primary text-primary-foreground [a]:hover:bg-primary/80", + default: "bg-primary text-primary-foreground hover:bg-primary/80",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/components/ui/button.tsx` around lines 8 - 22, The default variant string in the Button component currently uses an anchor-scoped selector ([a]:hover:bg-primary/80) so plain <button> elements (when asChild is false) get no hover style; update the default variant in the variants object (the entry named "default") to use a normal hover class (hover:bg-primary/80) instead of the [a]:hover: form so both real <button> and anchor children receive the intended hover background.ui/admin/src/lib/query/runtime.ts-8-30 (1)
8-30:⚠️ Potential issue | 🟡 MinorSeparate mutation keys and reconsider POST in query.
Two concerns:
useRuntimeValidationQueryusesuseQuerywith a POST endpoint. By default,useQuerywill automatically re-POST on mount, window focus, and reconnection. If the/admin/runtime/validateendpoint has side effects (logging, rate-limiting, state changes) or is expensive, this automatic refetching becomes problematic. Consider either disabling auto-refetch (enabled: false,refetchOnWindowFocus: false,refetchOnReconnect: false) or moving to a mutation-based pattern with explicit triggers.
validateRuntimeandredeployRuntimeboth usemutationKey: adminMutationKeys.runtime. Shared keys preventuseIsMutating({ mutationKey })and React Query DevTools from distinguishing between them, and future key-scoped logic (cancellation, retry, pausing) will collide. Split into distinct keys (e.g.,adminMutationKeys.runtimeValidateandadminMutationKeys.runtimeRedeploy).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/lib/query/runtime.ts` around lines 8 - 30, useRuntimeValidationQuery is calling the POST "/admin/runtime/validate" via useQuery which will auto-refetch (on mount, focus, reconnect); change this by either turning it into an explicit mutation or disable auto-refetch in useQuery options (set enabled: false and refetchOnWindowFocus/refetchOnReconnect: false) so the expensive/side-effecting POST is not run automatically, and update call sites to trigger it explicitly if you choose mutation. Also split the shared mutation key adminMutationKeys.runtime used by validateRuntime and redeployRuntime into two distinct keys (e.g., adminMutationKeys.runtimeValidate and adminMutationKeys.runtimeRedeploy) so useIsMutating, DevTools and key-scoped behaviors can distinguish validateRuntime and redeployRuntime.ui/admin/src/App.tsx-126-136 (1)
126-136:⚠️ Potential issue | 🟡 MinorStale notice can outlive a failed action.
runActiononly clearsactionErrorand setsnoticeon success; on failure it setsactionErrorbut leaves any previousnoticeuntouched.statusTextprioritizeserror, so the string shown during the failure is correct — but once the error clears (next successful action or manual reset), the previous unrelated notice could reappear as the current status. Consider clearingnoticeon failure (and/or on action start) so the banner always reflects the latest outcome.🩹 Proposed fix
async function runAction<T>(message: string, action: () => Promise<T>): Promise<T> { try { setActionError(null); + setNotice(null); const result = await action(); setNotice(message); return result; } catch (caught) { setActionError(toErrorMessage(caught)); throw caught; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/App.tsx` around lines 126 - 136, runAction currently only clears actionError and sets notice on success, which lets a stale notice persist after a failure; modify runAction (the async function runAction<T>) to clear notice at action start (call setNotice(null) before awaiting action) and also clear notice in the catch block (call setNotice(null) when setting setActionError) so the UI banner reflects the latest action outcome.ui/admin/src/features/instances/InstancesPage.tsx-64-79 (1)
64-79:⚠️ Potential issue | 🟡 MinorBackground refetches will discard unsaved edits.
This effect calls
reset()on every change toinstancesorprofiles. React Query refetches on window focus /staleTimeexpiry / cache invalidation (whichuseInstanceMutationstriggers indirectly viaadminQueryKeys.draftupdates), so an operator typing into a form can have their input wiped by a background refetch that returns the same instance they're editing. A safer pattern is to key the form byselectedIdand only reset when the selection actually changes, or to compare the incoming data against the current form values (formState.isDirty) before resetting.Also,
profilesis only read insidebuildNewInstanceFormValues, which runs only when there is no matching instance — so including it in the dep array causes resets on unrelated profile refetches too.🩹 Proposed fix sketch
- useEffect(() => { - if (isCreating) { - return; - } - - if (selectedId == null) { - const next = instances[0] ?? null; - setSelectedId(next?.id ?? null); - reset(next ? instanceToFormValues(next) : buildNewInstanceFormValues(instances, profiles)); - return; - } - - const next = instances.find((instance) => instance.id === selectedId) ?? instances[0] ?? null; - setSelectedId(next?.id ?? null); - reset(next ? instanceToFormValues(next) : buildNewInstanceFormValues(instances, profiles)); - }, [instances, isCreating, profiles, reset, selectedId]); + useEffect(() => { + if (isCreating) return; + const fallback = instances[0] ?? null; + const next = selectedId + ? instances.find((instance) => instance.id === selectedId) ?? fallback + : fallback; + if (next?.id !== selectedId) { + setSelectedId(next?.id ?? null); + reset(next ? instanceToFormValues(next) : buildNewInstanceFormValues(instances, profiles)); + } + // Intentionally skip resetting on every `instances` refetch to preserve in-flight edits. + }, [isCreating, selectedId, instances, profiles, reset]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/features/instances/InstancesPage.tsx` around lines 64 - 79, The effect in InstancesPage.tsx currently calls reset() whenever instances or profiles change, which can wipe in-progress edits; change the logic in the useEffect that references isCreating, selectedId, instances, reset, instanceToFormValues and buildNewInstanceFormValues so it only calls reset when the selectedId actually changes or when the incoming selected instance data changes AND the form isn't dirty (use formState.isDirty from the form hook to guard); remove profiles from the dependency array except when you explicitly need to rebuild a new-instance form (handle that case separately by triggering reset only when selectedId is null and profiles actually changed), and compare the new form values to current form values (or check object identity of the selected instance) before calling reset to avoid resetting on background refetches.ui/admin/src/features/draft-review/DraftReviewPage.tsx-39-43 (1)
39-43:⚠️ Potential issue | 🟡 MinorGate Apply on a validated dirty draft.
Right now Apply is enabled before validation has passed, and a never-run validation displays as “Needs attention.” This weakens the explicit validate/preview/apply workflow.
🧭 Proposed UI guardrail
+ const canApply = Boolean(draft?.dirty && validation?.ok); + @@ - <button className="primary-button" disabled={busy} onClick={onApply}> + <button className="primary-button" disabled={busy || !canApply} onClick={onApply}> Apply Draft </button> @@ - <dd>{validation?.ok ? "Passing" : "Needs attention"}</dd> + <dd>{validation == null ? "Not run" : validation.ok ? "Passing" : "Needs attention"}</dd>Also applies to: 60-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/features/draft-review/DraftReviewPage.tsx` around lines 39 - 43, The Apply button in DraftReviewPage is currently enabled before validation passes; update the UI to only enable the Apply action when the draft is both dirty and has a successful validation state. In the button render(s) referencing onApply and busy (the primary-button instances at the shown location and the similar block at lines 60-63), add a guard that disables the button unless draft.isDirty (or equivalent dirty flag) is true and validationStatus === 'passed' (or however successful validation is represented), and ensure the UI shows the proper validation state instead of "Needs attention" for never-run validations so Apply remains disabled until validation runs and passes.ui/admin/src/features/profiles/ProfilesPage.tsx-164-210 (1)
164-210:⚠️ Potential issue | 🟡 MinorValidate trimmed values, not just presence.
The
id,label,prompt_dir,base_prompt_path, andcodex_prompt_pathfields userequiredvalidation but are trimmed to empty strings byformValuesToProfile()before create/save. Whitespace-only values pass the required check, then are converted to empty strings, bypassing client-side validation.✅ Proposed trim-aware validation
+ const requiredTrimmed = (message: string) => (value: string) => value.trim().length > 0 || message; + {...register("id", { - required: "Profile ID is required.", + validate: requiredTrimmed("Profile ID is required."), })} @@ {...register("label", { - required: "Label is required.", + validate: requiredTrimmed("Label is required."), })} @@ {...register("prompt_dir", { - required: "Prompt directory is required.", + validate: requiredTrimmed("Prompt directory is required."), })} @@ {...register("base_prompt_path", { - required: "Base prompt path is required.", + validate: requiredTrimmed("Base prompt path is required."), })} @@ {...register("codex_prompt_path", { - required: "Codex prompt path is required.", + validate: requiredTrimmed("Codex prompt path is required."), })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/features/profiles/ProfilesPage.tsx` around lines 164 - 210, The required-only checks for form fields (id, label, prompt_dir, base_prompt_path, codex_prompt_path) allow whitespace-only input which later becomes empty via formValuesToProfile(); update the react-hook-form registrations in ProfilesPage.tsx to use a custom validate (or pattern) that trims the value and ensures it is non-empty (e.g., validate: v => v?.trim().length > 0 || "Field is required"), so the register(...) calls for "id", "label", "prompt_dir", "base_prompt_path", and "codex_prompt_path" enforce trim-aware validation before formValuesToProfile() runs.chatmock/admin_draft_service.py-147-162 (1)
147-162:⚠️ Potential issue | 🟡 Minor
apply()is not atomic across profiles and instances.
apply()calls_write_yaml_directoryfor profiles, then for instances. If the instance write fails mid-flight (disk full, permission error on a particular file, serializer raises), profile YAMLs on disk will already reflect the new draft while instance YAMLs remain stale, leaving operators in a half-applied state. The subsequentreset()would then rehydrate the draft from this inconsistent on-disk state.Consider writing both profile and instance payloads into a single staging area first (all files across both dirs) and only performing
os.replacemoves after every serialization succeeded, so a serializer/validation failure aborts before any mutation to the live config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chatmock/admin_draft_service.py` around lines 147 - 162, apply() currently writes profiles then instances directly to live dirs causing partial-apply risks; change it to stage both profile and instance files first and atomically swap them into place only after all serializations succeed. Concretely: in apply(), call _build_draft_service() then replace the two direct _write_yaml_directory calls with staging writes (e.g., use a new helper that serializes items from service.list_profiles() with serialize_profile_config into a temp directory under profiles_root and similarly for service.list_instances() with serialize_instance_config into a temp directory under instances_root), validate serialization succeeds for both dirs, and then atomically move/replace the staged directories/files into profiles_root and instances_root (os.replace or os.rename per file) before returning reset(); keep validate(), _build_draft_service(), reset() unchanged.ui/admin/src/features/prompt-files/PromptFilesPage.tsx-48-131 (1)
48-131:⚠️ Potential issue | 🟡 MinorUnhandled promise rejections from load/save callbacks.
onLoadPromptFilesin the effect (Line 54) and both button handlers (Lines 109-115, 121-127) don't catch rejections. If the mutation fails (404/500/network), the user sees a stale "Loading…" state or an empty editor with no error feedback, and React logs an unhandled rejection. Consider wrapping with try/catch and surfacing the error via toast/state, or wiring the button clicks tomutateAsyncwith anonErrorfrom the mutation layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/features/prompt-files/PromptFilesPage.tsx` around lines 48 - 131, The effect and both button handlers call the async callbacks onLoadPromptFiles and onSavePromptFiles without catching rejections, causing unhandled promise rejections and stale UI; wrap every await call to onLoadPromptFiles and onSavePromptFiles (in the useEffect block and in the Reload From Disk and Save Prompt Files click handlers) in try/catch (or use mutateAsync with an onError) and on error clear or restore appropriate state (e.g. keep/setPromptFiles to previous value or null), set busy loading flags consistently, and surface the error to the user (via a toast or an error state) so failures don’t leave a "Loading…" state or empty editor and no logged rejection.chatmock/admin_routes.py-419-429 (1)
419-429:⚠️ Potential issue | 🟡 MinorRedeploy callback errors surface as 500 with no JSON error shape.
Every other admin endpoint funnels exceptions into either
_registry_errororjsonify({"error": ...}), 400/404, butadmin_runtime_redeployinvokescallback()unwrapped. If the redeploy callback raises (docker unavailable, subprocess failure), the client receives Flask's default HTML 500, breaking the admin UI's JSON error-handling contract.🛡️ Proposed fix
callback = current_app.config.get("RUNTIME_REDEPLOY_CALLBACK") if callable(callback): - result = callback() - if isinstance(result, dict): - return jsonify(result) + try: + result = callback() + except Exception as exc: # noqa: BLE001 - surface as JSON per admin contract + return _registry_error(exc) + if isinstance(result, dict): + return jsonify(result) return jsonify({"ok": True, "status": "noop"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chatmock/admin_routes.py` around lines 419 - 429, The admin_runtime_redeploy handler calls the redeploy callback unguarded so exceptions become HTML 500s; update admin_runtime_redeploy to call callback() inside a try/except and on failure return a JSON error shape consistent with other admin endpoints (use _registry_error(...) or return jsonify({"error": str(e)}) with an appropriate status code) while keeping the existing behavior when the callback returns a dict; reference the admin_runtime_redeploy function and the RUNTIME_REDEPLOY_CALLBACK config key when making the change.chatmock/admin_routes.py-187-220 (1)
187-220:⚠️ Potential issue | 🟡 MinorValidate prompt text fields are strings before writing.
payload["base_prompt_text"]andpayload["codex_prompt_text"](Lines 196-197) are passed straight towrite_prompt_textwithout type checking. A client sendingnull, a number, or a nested object will either raise an uncaughtTypeErrorinsidewrite_prompt_text(500 response instead of the 400 contract enforced elsewhere) or, worse, serialize unexpected content to disk depending onwrite_prompt_text's implementation.🛡️ Proposed fix
base_prompt_path = str(payload["base_prompt_path"]) codex_prompt_path = str(payload["codex_prompt_path"]) - base_prompt_text = payload["base_prompt_text"] - codex_prompt_text = payload["codex_prompt_text"] + base_prompt_text = payload["base_prompt_text"] + codex_prompt_text = payload["codex_prompt_text"] + if not isinstance(base_prompt_text, str) or not isinstance(codex_prompt_text, str): + raise ValueError("base_prompt_text and codex_prompt_text must be strings")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chatmock/admin_routes.py` around lines 187 - 220, The admin_prompt_files_write handler currently passes payload["base_prompt_text"] and payload["codex_prompt_text"] straight to write_prompt_text; add explicit validation in admin_prompt_files_write to ensure both fields exist and are of type str (use isinstance(value, str)); if either is missing or not a string, return the same 400-style error response used elsewhere (e.g., jsonify({"error": {"message": "base_prompt_text must be a string"}}), 400) instead of calling write_prompt_text; keep the existing exception catch for filesystem errors and only call prompt_manager.reload() after successful validation and writes.
🧹 Nitpick comments (17)
ui/admin/src/lib/api/client.ts (1)
1-24: Error messages leak full response body;Content-Typesent on bodyless requests.Two small issues:
throw new Error(await response.text())surfaces the entire upstream error body to the UI/console. For admin endpoints this may include stack traces or config snippets. Consider wrapping with status + a trimmed body, and attaching the raw text as a property for callers that need it.apiJsonalways setsContent-Type: application/jsoneven whenbodyisundefined(e.g., DELETE or POST-trigger with no payload). Some servers/CDNs/proxies complain about a content-type header with no body. Only set the header when a body is being serialized.♻️ Proposed refactor
export async function apiJson<T>(path: string, method: string, body?: unknown): Promise<T> { + const hasBody = body !== undefined; const response = await fetch(path, { method, credentials: "same-origin", - headers: { - "Content-Type": "application/json", - }, - body: body === undefined ? undefined : JSON.stringify(body), + headers: hasBody ? { "Content-Type": "application/json" } : undefined, + body: hasBody ? JSON.stringify(body) : undefined, }); if (!response.ok) { - throw new Error(await response.text()); + const text = await response.text().catch(() => ""); + throw new Error(`${method} ${path} failed (${response.status}): ${text.slice(0, 500)}`); } return (await response.json()) as T; }(Apply the same error-wrapping idea to
apiGet.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/lib/api/client.ts` around lines 1 - 24, Both apiGet and apiJson currently throw the full response body and always set Content-Type; change them so that when response.ok is false you read the raw text, create a new Error containing the status and a trimmed snippet (e.g., first N chars) and attach the full raw text as a property (e.g., rawText) on that Error for callers that need it, and in apiJson only include the "Content-Type: application/json" header when body !== undefined (i.e., when you will JSON.stringify the body); update the error-handling path in apiGet and apiJson and the header-building logic in apiJson accordingly.ui/admin/src/lib/query-client.ts (1)
18-18: Module-level singleton instantiated at import time.Exporting
queryClientas a module-level constant means a client is created the moment this module is imported, including from tests. For Vitest it's usually preferable to create a freshQueryClientper test (viacreateQueryClient()) to avoid cache bleed between tests — the factory export already supports this, just make sure tests usecreateQueryClient()rather than importing the sharedqueryClient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/lib/query-client.ts` at line 18, The module currently exports a module-level singleton named queryClient that is created at import time; remove the eager export and ensure callers/tests use the createQueryClient() factory instead—delete or stop exporting the instantiated queryClient constant and only export createQueryClient(), update any imports to call createQueryClient() where a fresh QueryClient is required (e.g., tests and components), and keep the createQueryClient() function as the single source-of-truth for constructing new QueryClient instances.ui/admin/src/components/shared/StatCard.tsx (1)
7-13: Consider exportingStatCardProps.The type is only local; exporting it would help consumers compose wrappers or typed factory helpers without re-declaring the shape. Optional.
♻️ Proposed change
-type StatCardProps = { +export type StatCardProps = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/components/shared/StatCard.tsx` around lines 7 - 13, Export the StatCardProps type so other modules can import and reuse it: update the StatCardProps declaration to be exported (export type StatCardProps = { ... }) and ensure any local usages (like the StatCard component prop annotations) continue to reference StatCardProps; update any imports/exports if this file is re-exported from an index to expose the type to consumers.ui/admin/src/layouts/AdminLayout.tsx (1)
23-26: Expose status changes as a live region.
busyandstatusTextupdate asynchronously, but screen readers will not announce these changes reliably without a status/live-region role.♿ Proposed accessibility refinement
- <aside className="status-banner"> + <aside className="status-banner" role="status" aria-live="polite" aria-atomic="true"> <strong>{busy ? "Working..." : "Status"}</strong> <span>{statusText}</span> </aside>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/layouts/AdminLayout.tsx` around lines 23 - 26, The status banner in AdminLayout (the <aside> with className "status-banner") updates asynchronously but lacks an accessible live region; modify that element to expose changes to assistive tech by adding an appropriate live-region role/attributes (e.g., role="status" or aria-live="polite" and aria-atomic="true") so updates to busy and statusText are announced reliably by screen readers.ui/admin/src/components/ui/textarea.tsx (1)
5-14: Textarea ref forwarding is a best practice but not critical for current usage.Both
InputandTextareaare plain function components without ref forwarding. The current usage via react-hook-form'sregister()pattern works fine without it, as the library uses controlled components through event handlers rather than refs. However, addingforwardRefwould be good practice for future imperative use cases or consumers that need direct DOM access.🔧 Proposed ref-forwarding fix
-function Textarea({ className, ...props }: React.ComponentProps<"textarea">) { - return ( +const Textarea = React.forwardRef<HTMLTextAreaElement, React.ComponentProps<"textarea">>( + ({ className, ...props }, ref) => ( <textarea + ref={ref} data-slot="textarea" className={cn( "flex field-sizing-content min-h-16 w-full rounded-lg border border-input bg-transparent px-2.5 py-2 text-base transition-colors outline-none placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 disabled:cursor-not-allowed disabled:bg-input/50 disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 md:text-sm dark:bg-input/30 dark:disabled:bg-input/80 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40", className )} {...props} /> ) -} +) + +Textarea.displayName = "Textarea"Note: The same pattern could be applied to
Inputfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/components/ui/textarea.tsx` around lines 5 - 14, The Textarea component does not forward refs; convert the plain function Textarea to use React.forwardRef<HTMLTextAreaElement, React.ComponentProps<"textarea">> so the forwarded ref is passed to the underlying <textarea> element (i.e., accept (props, ref) and attach ref={ref} to the element), and set Textarea.displayName = "Textarea"; apply the same ref-forwarding pattern to the Input component for consistency (use HTMLInputElement for Input).ui/admin/src/styles.css (1)
327-327: Non-standard opacity stepborder-white/12.Tailwind v4 accepts arbitrary numeric opacity, but
/12is off the default scale (/10,/15,/20). This is valid but inconsistent with the rest of the file, which otherwise uses standard steps. Consider/10or/15for consistency, or use/[.12]to make the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/styles.css` at line 327, The CSS rule using `@apply` with the non-standard opacity step `border-white/12` should be made consistent: replace `border-white/12` with a standard Tailwind opacity step like `border-white/10` or `border-white/15`, or if the precise 12% opacity is required, make the intent explicit using the arbitrary value syntax `border-white/[.12]` in the same declaration (`@apply rounded-[var(--radius-pill)] border border-white/... bg-white/10 p-1;`) so the style matches the rest of the file's opacity conventions.ui/admin/src/lib/query/profiles.ts (1)
24-42: All three profile mutations share one mutation key.
saveProfile,createProfile, anddeleteProfileall setmutationKey: adminMutationKeys.profiles. That makesuseIsMutating({ mutationKey }), devtools inspection, and any key-scoped retry/cancel logic unable to distinguish them. Prefer distinct keys per mutation family (e.g.profilesCreate/profilesUpdate/profilesDelete), or drop the shared key.Also worth considering: on success you
setQueryData(adminQueryKeys.draft, draft)but do not invalidateadminQueryKeys.profiles. IfuseProfilesQueryever reads the applied (non-draft) list, a create/delete won't reflect there until the next manual refetch — verify thatGET /admin/profilesis intentionally decoupled from draft mutations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/lib/query/profiles.ts` around lines 24 - 42, The three mutations (saveProfile, createProfile, deleteProfile) all use the same mutationKey adminMutationKeys.profiles so callers and devtools can't distinguish them; change each mutation to a distinct key (e.g. adminMutationKeys.profilesUpdate / profilesCreate / profilesDelete or similar) in their useMutation configs (locate saveProfile, createProfile, deleteProfile) or remove the shared key entirely; additionally, after successful create/delete ensure the non-draft profiles list is updated or invalidated — either call queryClient.invalidateQueries(adminQueryKeys.profiles) in onSuccess (in syncDraft or alongside it) or confirm that GET /admin/profiles is intentionally decoupled from adminQueryKeys.draft used in setQueryData to avoid stale UI.ui/admin/src/lib/query/instances.ts (1)
28-37: One failed preview fails the whole dashboard.
Promise.allshort-circuits on the first rejection, so a single misconfigured instance whose/admin/instances/{id}/previewreturns non-2xx will wipe every preview card from the Current State dashboard (the very page operators rely on to diagnose that misconfiguration). ConsiderPromise.allSettledand filtering/placeholder-ing failed entries so the healthy ones still render, or fetching previews individually per instance.🩹 Proposed fix sketch
- return Promise.all( - instances.map((instance) => apiGet<InstancePreview>(`/admin/instances/${instance.id}/preview`)), - ); + const settled = await Promise.allSettled( + instances.map((instance) => apiGet<InstancePreview>(`/admin/instances/${instance.id}/preview`)), + ); + return settled.flatMap((result) => (result.status === "fulfilled" ? [result.value] : []));(If you'd rather surface the errors per card, return a discriminated union of
{ ok, preview } | { ok: false, instanceId, error }instead.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/lib/query/instances.ts` around lines 28 - 37, The queryFn currently uses Promise.all over instances.map(apiGet(...)) which causes one failed preview fetch to reject the whole batch; change queryFn to use Promise.allSettled on instances.map calling apiGet for `/admin/instances/${instance.id}/preview`, then map the results into an array of InstancePreview (or placeholders) by keeping fulfilled values and converting rejected entries into a safe placeholder or a discriminated error object (e.g., { ok: false, instanceId, error }) so healthy previews still render; update any callers expecting InstancePreview[] to handle the placeholder/error shape if you choose the discriminated-union approach.ui/admin/src/features/instances/InstancesPage.tsx (1)
131-144: Add explicittype="button"to sidebar list items.These
<button>elements default totype="submit". They're currently rendered as a sibling of the<form>(not inside it), so no form is submitted today, but the default is a sharp edge if the layout is ever nested differently. Settingtype="button"is a one-line defensive improvement.🩹 Proposed fix
<button key={instance.id} + type="button" className={`list-item ${instance.id === selectedId && !isCreating ? "active" : ""}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/features/instances/InstancesPage.tsx` around lines 131 - 144, The sidebar instance list buttons in InstancesPage.tsx default to type="submit", which is brittle; update the mapped <button> (the one that uses key={instance.id} and calls setIsCreating(false), setSelectedId(instance.id), reset(instanceToFormValues(instance))) to explicitly include type="button" so clicks won't accidentally submit any enclosing form in the future; leave the click handler and className logic (selectedId, isCreating) unchanged.ui/admin/src/features/runtime-actions/RuntimeActionsPage.tsx (1)
30-49: Use the sharedButtonprimitive for consistency.
DashboardPage(and the rest of the admin UI) uses the shadcnButtoncomponent, but this page drops down to raw<button>tags with.secondary-button/.danger-buttonclasses. This skips variant styling, focus/disabled rings, and accessibility behavior thatButtonalready provides, and makes the two runtime action surfaces diverge visually. Consider swapping toButtonwithvariant="outline"/variant="destructive".🩹 Proposed fix
+import { Button } from "@/components/ui/button"; import type { InstancePreview, PromptState, RuntimeRedeployResponse, ValidationSummary } from "../../lib/types/admin"; @@ - <div className="editor-actions"> - <button className="secondary-button" disabled={busy} onClick={onValidate}> - Validate Current Runtime - </button> - <button className="secondary-button" disabled={busy} onClick={onReloadPrompts}> - Reload Prompts - </button> - <button - className="danger-button" - disabled={busy} - onClick={async () => { - if (!window.confirm("Redeploy the running ChatMock stack now?")) { - return; - } - await onRedeploy(); - }} - > - Redeploy - </button> - </div> + <div className="editor-actions"> + <Button disabled={busy} variant="outline" onClick={onValidate}> + Validate Current Runtime + </Button> + <Button disabled={busy} variant="outline" onClick={onReloadPrompts}> + Reload Prompts + </Button> + <Button + disabled={busy} + variant="destructive" + onClick={async () => { + if (!window.confirm("Redeploy the running ChatMock stack now?")) { + return; + } + await onRedeploy(); + }} + > + Redeploy + </Button> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/features/runtime-actions/RuntimeActionsPage.tsx` around lines 30 - 49, Replace the raw <button> elements in RuntimeActionsPage with the shared Button primitive (use variant="outline" for the secondary actions and variant="destructive" for the redeploy action), preserve the disabled={busy} and onClick handlers (including the async window.confirm wrapper around onRedeploy), remove the .secondary-button/.danger-button classNames, and ensure you import Button from the shared UI library at the top of the file; update usages for onValidate, onReloadPrompts, and onRedeploy so they pass through to Button props exactly as before.ui/admin/src/App.tsx (1)
96-99:busywill flicker on background refetches.
useIsFetchingcounts background refetches (e.g. window focus,staleTimeexpiry,invalidateQueries), not just user-initiated loads, so every silent refresh will flipbusytotrueand disable every action button / form submit across the UI. If that's acceptable UX here, ignore this; otherwise consider scopingbusytouseIsMutatingplus only the initial fetch states (query.isPending) for the surfaces you care about.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/App.tsx` around lines 96 - 99, The current busy flag (const busy = fetchingCount > 0 || mutatingCount > 0) flips on background refetches because useIsFetching counts silent refreshes; change busy to only reflect active mutations and initial query loads by keeping mutatingCount but replacing useIsFetching/adminQueryKeys.root with a check for initial-pending query states (e.g., inspect the admin queries via the query client or useQuery hooks and test each query.state.isPending or fetchStatus === 'pending' / status === 'pending') and compute busy = mutatingCount > 0 || anyAdminQueryIsPending; update references to fetchingCount, busy, useIsFetching and adminQueryKeys.root accordingly.ui/admin/src/components/ui/card.tsx (1)
15-15: Use an explicit direct-child arbitrary selector for image rounding.
*:[img:first-child]looks brittle for Tailwind v4; use direct selectors so first/last direct<img>children are actually targeted. Tailwind documents*as the direct-child variant and arbitrary selectors with&; v4 stacked variants apply left-to-right. Sources: Tailwind child selectors/arbitrary variants and v4 upgrade guide.🎨 Proposed Tailwind selector cleanup
- "group/card flex flex-col gap-4 overflow-hidden rounded-xl bg-card py-4 text-sm text-card-foreground ring-1 ring-foreground/10 has-data-[slot=card-footer]:pb-0 has-[>img:first-child]:pt-0 data-[size=sm]:gap-3 data-[size=sm]:py-3 data-[size=sm]:has-data-[slot=card-footer]:pb-0 *:[img:first-child]:rounded-t-xl *:[img:last-child]:rounded-b-xl", + "group/card flex flex-col gap-4 overflow-hidden rounded-xl bg-card py-4 text-sm text-card-foreground ring-1 ring-foreground/10 has-data-[slot=card-footer]:pb-0 has-[>img:first-child]:pt-0 data-[size=sm]:gap-3 data-[size=sm]:py-3 data-[size=sm]:has-data-[slot=card-footer]:pb-0 [&>img:first-child]:rounded-t-xl [&>img:last-child]:rounded-b-xl",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/admin/src/components/ui/card.tsx` at line 15, The Tailwind selector using "*:[img:first-child]" and "*:[img:last-child]" is brittle—update the class string in the card component (the long class list in ui/admin/src/components/ui/card.tsx, e.g. where the string contains group/card ...) to use explicit direct-child arbitrary selectors '[&>img:first-child]' and '[&>img:last-child]' respectively so the first and last direct <img> children are targeted correctly under Tailwind v4 stacking rules.chatmock/admin_routes.py (1)
275-285:INSTANCE_SERVICEis cleared beforereset()runs.Line 282 sets
INSTANCE_SERVICE = Nonebeforeservice.reset()executes on Line 283. Ifreset()raises (e.g., the on-disk config became invalid between operations), the cached service has already been dropped; the next request to/admin/profileswill rebuild from disk and likely fail too, which is fine — but it means a transient reset error effectively invalidates any previously cached service. Consider reordering so the cache is cleared only on success, mirroringadmin_draft_apply(which clears afterresult = service.apply()completes on Line 322).♻️ Proposed fix
try: service = _get_draft_service() - current_app.config["INSTANCE_SERVICE"] = None - return jsonify(service.reset()) + result = service.reset() except (FileNotFoundError, OSError, ValueError, RuntimeError) as exc: return _registry_error(exc) + current_app.config["INSTANCE_SERVICE"] = None + return jsonify(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chatmock/admin_routes.py` around lines 275 - 285, The admin_draft_reset handler clears the cached INSTANCE_SERVICE before calling service.reset(), which can drop the cached service on a reset failure; change the order so you call service.reset() first and only set current_app.config["INSTANCE_SERVICE"] = None after reset() succeeds: obtain service via _get_draft_service(), call result = service.reset(), then clear INSTANCE_SERVICE and return jsonify(result); keep the existing exception handling (_registry_error) unchanged so cache is only evicted on successful reset (mirroring admin_draft_apply).tests/test_admin_routes.py (2)
137-147: Fallback test also matches the trivial case — consider a non-existing asset request.
/admin/ui/instancesis a path that doesn't exist on disk, which is what you want. But since the only file indist/isindex.html, this test would still pass if the route always returnedindex.htmlunconditionally. Adding an asset file underdist/(e.g.assets/app.js) and asserting it is served verbatim (not as HTML) would also pin the "serve actual file when present" branch in_serve_admin_ui_path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_admin_routes.py` around lines 137 - 147, Update the test_admin_ui_unknown_path_falls_back_to_index to also create a real asset under the dist directory (e.g., write assets/app.js via _write_admin_index or a helper) and then request that asset path (e.g., /admin/ui/assets/app.js) using the test client to assert the server returns the asset contents and correct non-HTML mimetype; keep the existing request to /admin/ui/instances to verify fallback to index.html, and ensure both behaviors exercise the _serve_admin_ui_path logic when creating the app with create_app(admin_ui_dist_dir=...).
110-121: Consider covering error and auth paths.The suite comprehensively covers happy paths for UI fallback, draft lifecycle, runtime, and prompt file endpoints, but nothing exercises the error branches (e.g., 400 on invalid JSON body to
/admin/profiles//admin/instances, 404 on unknown instance preview, 400 on apply after an invalid draft, or the_require_local_admindenial path). Adding a few negative tests would protect the new blueprint's error contract going forward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_admin_routes.py` around lines 110 - 121, Add negative tests to the admin test suite using the helper _build_admin_app/create_app to instantiate the app and exercise error and auth branches: send invalid JSON to POST /admin/profiles and POST /admin/instances and assert 400 responses; request preview for a non-existent instance ID and assert 404; attempt to apply a draft that fails validation and assert 400; and simulate the _require_local_admin denial path (e.g., by configuring the app or request context to fail local-admin checks) and assert the proper authorization rejection. Use the same test patterns as existing happy-path tests to keep setup/teardown consistent and reference the runtime_redeploy_callback path if needed to exercise runtime-related error behavior.chatmock/admin_draft_service.py (2)
183-207: Cross-directory cleanup ordering can delete files that are still valid.
_write_yaml_directorystages new files into a siblingTemporaryDirectory, thenos.replaces them intorootone by one (Line 203-204), then deletes any*.yamlinrootnot inexpected_names(Line 205-207). This is correct, but since the replace step is non-atomic across files, a crash between the two phases leaves new files applied without the stale-file prune. Lower risk, but worth noting alongside the apply-level atomicity concern.Minor robustness nit: iterate
expected_names/ theitemslist for the rename loop (Line 203) rather than re-globbing the temp dir, so the move set is derived from the same source of truth you just staged.♻️ Suggested refactor
- for temp_path in temp_root.glob("*.yaml"): - os.replace(temp_path, root / temp_path.name) + for filename in expected_names: + os.replace(temp_root / filename, root / filename) for existing in root.glob("*.yaml"): if existing.name not in expected_names: existing.unlink()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chatmock/admin_draft_service.py` around lines 183 - 207, The _write_yaml_directory function currently replaces staged files by globbing temp_root then prunes stale files, which can leave new files in place if a crash occurs between those phases; change the move loop to iterate over the same source of truth you used to create the files (use expected_names or the original items list) and perform os.replace(temp_root / filename, root / filename) only for those expected filenames, then after all expected files have been moved prune any root/*.yaml not in expected_names; reference symbols: _write_yaml_directory, expected_names, temp_root, os.replace, root.glob.
164-168: Replaceassertguard with an explicit check.
assert self._draft is not Noneon Line 167 is removed underpython -O, which would then let thereturn self._drafthand backNonesilently and downstream callers (e.g.,update_profile,validate,preview) would fail with opaqueTypeError/KeyErrorfar from the root cause. Since_ensure_draftis on the happy path of every mutation, a plainifis safer.♻️ Proposed fix
def _ensure_draft(self) -> dict[str, Any]: if self._draft is None: self.reset() - assert self._draft is not None + if self._draft is None: + raise RuntimeError("Draft state unavailable after reset") return self._draft🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chatmock/admin_draft_service.py` around lines 164 - 168, The _ensure_draft method currently uses assert self._draft is not None which is stripped under python -O and can return None to callers like update_profile, validate, and preview; replace the assert with an explicit runtime check: after calling self.reset() if self._draft is still None raise a clear RuntimeError (or ValueError) mentioning "_draft is None in _ensure_draft" so callers fail fast and with an actionable message; keep the existing reset() call and return self._draft after the explicit check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b16e837f-b56e-42c6-b026-9476460760f5
⛔ Files ignored due to path filters (6)
ui/admin/package-lock.jsonis excluded by!**/package-lock.jsonui/admin/public/icons/apple-touch-icon.pngis excluded by!**/*.pngui/admin/public/icons/favicon-32.pngis excluded by!**/*.pngui/admin/public/icons/icon-192.pngis excluded by!**/*.pngui/admin/public/icons/icon-512.pngis excluded by!**/*.pngui/admin/public/icons/icon-maskable-512.pngis excluded by!**/*.png
📒 Files selected for processing (65)
.gitignoreDOCKER.mdREADME.mdchatmock/admin_draft_service.pychatmock/admin_routes.pychatmock/app.pychatmock/config.pychatmock/instance_registry.pychatmock/profile_registry.pydocs/chatmock-admin-ui-frontend-conventions.mddocs/chatmock-admin-ui-stack-evaluation.mddocs/chatmock-instance-management.mddocs/superpowers/plans/2026-04-22-chatmock-browser-admin-ui-operator-first-refresh.mddocs/superpowers/plans/2026-04-23-chatmock-admin-ui-stack-migration.mddocs/superpowers/specs/2026-04-22-chatmock-admin-ui-icon-design.mddocs/superpowers/specs/2026-04-22-chatmock-browser-admin-ui-operator-first-refresh-design.mdtests/test_admin_draft_service.pytests/test_admin_routes.pytests/test_routes.pyui/admin/components.jsonui/admin/index.htmlui/admin/package.jsonui/admin/public/manifest.webmanifestui/admin/src/App.tsxui/admin/src/components/shared/StatCard.tsxui/admin/src/components/shared/SurfaceCard.tsxui/admin/src/components/ui/button.tsxui/admin/src/components/ui/card.tsxui/admin/src/components/ui/input.tsxui/admin/src/components/ui/select.tsxui/admin/src/components/ui/tabs.tsxui/admin/src/components/ui/textarea.tsxui/admin/src/features/dashboard/DashboardPage.tsxui/admin/src/features/draft-review/DraftReviewPage.tsxui/admin/src/features/edit-config/EditConfigForms.test.tsxui/admin/src/features/edit-config/EditConfigPage.tsxui/admin/src/features/instances/InstancesPage.tsxui/admin/src/features/profiles/ProfilesPage.tsxui/admin/src/features/prompt-files/PromptFilesPage.tsxui/admin/src/features/runtime-actions/RuntimeActionsPage.tsxui/admin/src/forms/instanceForm.tsui/admin/src/forms/profileForm.tsui/admin/src/layouts/AdminLayout.tsxui/admin/src/lib/api/client.tsui/admin/src/lib/query-client.tsui/admin/src/lib/query/draft.tsui/admin/src/lib/query/instances.tsui/admin/src/lib/query/keys.tsui/admin/src/lib/query/profiles.tsui/admin/src/lib/query/prompts.tsui/admin/src/lib/query/runtime.tsui/admin/src/lib/query/shared.tsui/admin/src/lib/types/admin.tsui/admin/src/lib/utils.tsui/admin/src/main.tsxui/admin/src/router.test.tsxui/admin/src/router.tsxui/admin/src/routes/CurrentStateRoute.tsxui/admin/src/routes/EditConfigRoute.tsxui/admin/src/routes/PromptFilesRoute.tsxui/admin/src/styles.cssui/admin/src/test/setup.tsui/admin/tsconfig.jsonui/admin/vite.config.tsui/admin/vitest.config.ts
| function Input({ className, type, ...props }: React.ComponentProps<"input">) { | ||
| return ( | ||
| <input | ||
| type={type} | ||
| data-slot="input" | ||
| className={cn( | ||
| "h-8 w-full min-w-0 rounded-lg border border-input bg-transparent px-2.5 py-1 text-base transition-colors outline-none file:inline-flex file:h-6 file:border-0 file:bg-transparent file:text-sm file:font-medium file:text-foreground placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 disabled:pointer-events-none disabled:cursor-not-allowed disabled:bg-input/50 disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 md:text-sm dark:bg-input/30 dark:disabled:bg-input/80 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40", | ||
| className | ||
| )} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find form register() spreads into Input/Textarea wrappers and verify wrapper ref handling.
rg -n --type tsx -C3 '<Input[^>]*\{\.\.\.register|<Textarea[^>]*\{\.\.\.register|register\("[^"]+"\)'
rg -n --type tsx -C3 'function Input|const Input|forwardRef<HTMLInputElement|function Textarea|const Textarea|forwardRef<HTMLTextAreaElement'Repository: DrJLabs/ChatMock
Length of output: 115
🏁 Script executed:
fd -name "input.tsx" -o -name "input.ts" -o -name "input.jsx" -o -name "input.js"Repository: DrJLabs/ChatMock
Length of output: 290
🏁 Script executed:
git ls-files | head -20Repository: DrJLabs/ChatMock
Length of output: 494
🏁 Script executed:
fd -name "*.tsx" | grep -E "(input|form)" | head -20Repository: DrJLabs/ChatMock
Length of output: 290
🏁 Script executed:
ls -la ui/ 2>&1 || echo "ui directory not found"Repository: DrJLabs/ChatMock
Length of output: 225
🏁 Script executed:
find . -type f -name "input.tsx" 2>/dev/null | head -5Repository: DrJLabs/ChatMock
Length of output: 98
🏁 Script executed:
ls -la 2>&1 | head -30Repository: DrJLabs/ChatMock
Length of output: 1955
🏁 Script executed:
cat -n ./ui/admin/src/components/ui/input.tsxRepository: DrJLabs/ChatMock
Length of output: 1160
🏁 Script executed:
find ./ui -name "package.json" -type fRepository: DrJLabs/ChatMock
Length of output: 83
🏁 Script executed:
grep -r "register(" ./ui/admin/src --include="*.tsx" --include="*.ts" -B2 -A2 | head -50Repository: DrJLabs/ChatMock
Length of output: 3973
🏁 Script executed:
cat -n ./ui/admin/package.jsonRepository: DrJLabs/ChatMock
Length of output: 1414
🏁 Script executed:
grep -i "react\|react-hook-form" ./ui/admin/package.jsonRepository: DrJLabs/ChatMock
Length of output: 392
🏁 Script executed:
grep -r "forwardRef" ./ui/admin/src --include="*.tsx" --include="*.ts"Repository: DrJLabs/ChatMock
Length of output: 42
🏁 Script executed:
find ./ui/admin/src/components/ui -type f -name "*.tsx" | head -20Repository: DrJLabs/ChatMock
Length of output: 296
🏁 Script executed:
cat -n ./ui/admin/src/components/ui/textarea.tsx 2>/dev/null || echo "textarea.tsx not found"Repository: DrJLabs/ChatMock
Length of output: 1027
Wrap Input in React.forwardRef to support React Hook Form refs.
register(...) passes a ref, but this plain function component drops it instead of forwarding to the native <input>. This breaks focus-on-error and field registration in forms consuming this component.
🔧 Proposed ref-forwarding fix
-function Input({ className, type, ...props }: React.ComponentProps<"input">) {
- return (
+const Input = React.forwardRef<HTMLInputElement, React.ComponentProps<"input">>(
+ ({ className, type, ...props }, ref) => (
<input
+ ref={ref}
type={type}
data-slot="input"
className={cn(
"h-8 w-full min-w-0 rounded-lg border border-input bg-transparent px-2.5 py-1 text-base transition-colors outline-none file:inline-flex file:h-6 file:border-0 file:bg-transparent file:text-sm file:font-medium file:text-foreground placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 disabled:pointer-events-none disabled:cursor-not-allowed disabled:bg-input/50 disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 md:text-sm dark:bg-input/30 dark:disabled:bg-input/80 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40",
className
)}
{...props}
/>
)
-}
+)
+
+Input.displayName = "Input"Note: Textarea (lines 5-15 in ui/admin/src/components/ui/textarea.tsx) has the identical issue and requires the same fix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Input({ className, type, ...props }: React.ComponentProps<"input">) { | |
| return ( | |
| <input | |
| type={type} | |
| data-slot="input" | |
| className={cn( | |
| "h-8 w-full min-w-0 rounded-lg border border-input bg-transparent px-2.5 py-1 text-base transition-colors outline-none file:inline-flex file:h-6 file:border-0 file:bg-transparent file:text-sm file:font-medium file:text-foreground placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 disabled:pointer-events-none disabled:cursor-not-allowed disabled:bg-input/50 disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 md:text-sm dark:bg-input/30 dark:disabled:bg-input/80 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40", | |
| className | |
| )} | |
| {...props} | |
| /> | |
| const Input = React.forwardRef<HTMLInputElement, React.ComponentProps<"input">>( | |
| ({ className, type, ...props }, ref) => ( | |
| <input | |
| ref={ref} | |
| type={type} | |
| data-slot="input" | |
| className={cn( | |
| "h-8 w-full min-w-0 rounded-lg border border-input bg-transparent px-2.5 py-1 text-base transition-colors outline-none file:inline-flex file:h-6 file:border-0 file:bg-transparent file:text-sm file:font-medium file:text-foreground placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 disabled:pointer-events-none disabled:cursor-not-allowed disabled:bg-input/50 disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-3 aria-invalid:ring-destructive/20 md:text-sm dark:bg-input/30 dark:disabled:bg-input/80 dark:aria-invalid:border-destructive/50 dark:aria-invalid:ring-destructive/40", | |
| className | |
| )} | |
| {...props} | |
| /> | |
| ) | |
| ) | |
| Input.displayName = "Input" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/admin/src/components/ui/input.tsx` around lines 5 - 15, The Input
functional component (Input) currently drops forwarded refs from React Hook
Form; update it to use React.forwardRef and forward the ref to the native
<input> element (accept the ref as the second parameter and pass it to the input
element) so register(...) refs and focus-on-error work; apply the identical
change to the Textarea component (Textarea) so both components accept (props,
ref) via React.forwardRef and attach ref to the underlying DOM element.
| <Textarea | ||
| className="prompt-textarea" | ||
| rows={18} | ||
| value={promptFiles.base_prompt_text} | ||
| onChange={(event) => | ||
| setPromptFiles((current) => | ||
| current ? { ...current, base_prompt_text: event.target.value } : current, | ||
| ) | ||
| } | ||
| /> | ||
| </SurfaceCardContent> | ||
| </SurfaceCard> | ||
|
|
||
| <SurfaceCard> | ||
| <SurfaceCardHeader> | ||
| <div> | ||
| <p className="eyebrow">Codex Prompt</p> | ||
| <SurfaceCardTitle>{selectedProfile.codex_prompt_path}</SurfaceCardTitle> | ||
| </div> | ||
| </SurfaceCardHeader> | ||
| <SurfaceCardContent> | ||
| <Textarea | ||
| className="prompt-textarea" | ||
| rows={18} | ||
| value={promptFiles.codex_prompt_text} | ||
| onChange={(event) => | ||
| setPromptFiles((current) => | ||
| current ? { ...current, codex_prompt_text: event.target.value } : current, | ||
| ) | ||
| } | ||
| /> | ||
| </SurfaceCardContent> | ||
| </SurfaceCard> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify what callback the route passes as onLoadPromptFiles.
fd -t f 'PromptFilesRoute' ui/admin/src/routes
rg -nP -C3 '\bonLoadPromptFiles\b|\breadPromptFiles\b' ui/admin/srcRepository: DrJLabs/ChatMock
Length of output: 4737
Wrap loadPromptFiles with useCallback in App.tsx to prevent unsaved edits from being lost.
The loadPromptFiles function (lines 205-206) is redefined on every render, causing the effect in PromptFilesPage (dependency on onLoadPromptFiles at line 65) to re-run unnecessarily and overwrite promptFiles, silently losing unsaved edits. Wrap it with useCallback at the route layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/admin/src/features/prompt-files/PromptFilesPage.tsx` around lines 187 -
219, The loadPromptFiles function is recreated on every render causing
PromptFilesPage's effect (which depends on onLoadPromptFiles) to re-run and
overwrite unsaved edits; wrap the loadPromptFiles function in App.tsx with
React.useCallback (exported/passed as the onLoadPromptFiles prop) so its
identity is stable across renders, ensuring PromptFilesPage (the component using
prop onLoadPromptFiles) only runs its effect when the actual load logic changes.
Fix the verified review issues across the admin draft service and browser UI, including root consistency, prompt subpath preservation, atomic write/rollback behavior, callback stability, and UI/form correctness. Add regression coverage for nested prompt paths, rollback on failed applies, atomic prompt writes, and stricter form validation cases.
|
Addressed the verified PR review findings in What changed in this follow-up:
Fresh verification:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new React-based browser admin UI and a backend draft service to manage prompt profiles and instances, enabling in-memory edits, validation, and atomic YAML updates. The review identified a bug in the UI form reset logic when switching between instances while the form is dirty. Additionally, improvements were suggested for YAML serialization to support Unicode characters and for the frontend error handler to correctly parse JSON error messages from the backend.
| const next = instances.find((instance) => instance.id === selectedId) ?? instances[0] ?? null; | ||
| if (next?.id !== selectedId) { | ||
| setSelectedId(next?.id ?? null); | ||
| reset(next ? instanceToFormValues(next) : buildNewInstanceFormValues(instances, profiles)); | ||
| return; | ||
| } | ||
|
|
||
| if (!isDirty && next != null) { | ||
| reset(instanceToFormValues(next)); | ||
| } |
There was a problem hiding this comment.
There is a bug in the form reset logic. When a user selects a different instance in the sidebar while the form is dirty (e.g., after editing the previous selection), the !isDirty condition prevents reset() from being called. Consequently, the form continues to display data from the previously selected instance even though the sidebar indicates a new selection. The form should always reset when the identity of the selected item changes.
const next = instances.find((instance) => instance.id === selectedId) ?? instances[0] ?? null;
if (next?.id !== selectedId) {
setSelectedId(next?.id ?? null);
}
if (next != null && (next.id !== watch("id") || !isDirty)) {
reset(instanceToFormValues(next));
}
| filename = f'{item["id"]}.yaml' | ||
| temp_path = temp_root / filename | ||
| temp_path.write_text( | ||
| yaml.safe_dump(payload, sort_keys=False, allow_unicode=False), |
There was a problem hiding this comment.
Setting allow_unicode=False in yaml.safe_dump causes non-ASCII characters to be escaped (e.g., \uXXXX). Setting this to True improves the readability of the generated YAML configuration files for operators who might inspect or edit them manually.
| yaml.safe_dump(payload, sort_keys=False, allow_unicode=False), | |
| yaml.safe_dump(payload, sort_keys=False, allow_unicode=True), |
| export function toErrorMessage(error: unknown): string { | ||
| if (typeof error === "string") { | ||
| return error; | ||
| } | ||
|
|
||
| return error instanceof Error ? error.message : "Unknown error"; | ||
| } |
There was a problem hiding this comment.
The toErrorMessage function returns the raw string if the error is a string. Since the Flask backend returns JSON error objects (e.g., {"error": {"message": "..."}}), and the API client throws the response text, the UI currently displays raw JSON strings to the user. This function should attempt to parse the JSON and extract the nested error message.
| export function toErrorMessage(error: unknown): string { | |
| if (typeof error === "string") { | |
| return error; | |
| } | |
| return error instanceof Error ? error.message : "Unknown error"; | |
| } | |
| export function toErrorMessage(error: unknown): string { | |
| if (typeof error === "string") { | |
| try { | |
| const parsed = JSON.parse(error); | |
| if (parsed?.error?.message) { | |
| return parsed.error.message; | |
| } | |
| } catch { | |
| // Not a JSON string, return as is | |
| } | |
| return error; | |
| } | |
| return error instanceof Error ? error.message : "Unknown error"; | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 874e7985d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| queryClient.invalidateQueries({ queryKey: adminQueryKeys.profiles }), | ||
| queryClient.invalidateQueries({ queryKey: adminQueryKeys.instances }), | ||
| queryClient.invalidateQueries({ queryKey: adminQueryKeys.runtimeValidation }), | ||
| queryClient.invalidateQueries({ queryKey: adminQueryKeys.prompts }), |
There was a problem hiding this comment.
invalidateCurrentState refreshes profiles/instances/runtime/prompts but never invalidates the current preview query, whose key only depends on instance IDs (adminQueryKeys.currentPreviews(instanceIds)). If an operator applies or resets a draft that changes fields like port/profile while IDs stay the same, the dashboard can keep showing stale preview data indefinitely because that query key does not change and is never invalidated.
Useful? React with 👍 / 👎.
| current_state.base_prompt_path == str(base_path) | ||
| or current_state.codex_prompt_path == str(codex_path) |
There was a problem hiding this comment.
The reload gate compares current_state.*_prompt_path directly to str(base_path/codex_path), but base_path/codex_path are resolved absolute paths while prompt manager state can legitimately hold relative paths (e.g., after /admin/prompts/config with relative values). In that case writing the active prompt files returns success but skips prompt_manager.reload(), leaving runtime instructions stale until a manual reload.
Useful? React with 👍 / 👎.
| write_prompt_texts_atomically( | ||
| [ | ||
| (base_path, base_prompt_text), | ||
| (codex_path, codex_prompt_text), | ||
| ] |
There was a problem hiding this comment.
This always stages two writes even if base_prompt_path and codex_prompt_path resolve to the same file. When that happens, the second tuple overwrites the first, so users can submit distinct base/codex text and silently lose one of them. Because identical base/codex paths are allowed by config, this endpoint should reject or coalesce duplicate targets before writing.
Useful? React with 👍 / 👎.
Address the remaining verified review findings around prompt path normalization, same-file prompt writes, preview invalidation, JSON error extraction, and prompt-editor error handling. Add focused regression tests for prompt config normalization and backend error extraction behavior.
|
Pushed follow-up review fixes in Fresh verification:
Review requests: |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive browser-based admin UI and a corresponding backend management layer for ChatMock. Key additions include an AdminDraftService for in-memory configuration editing, atomic file operations for prompt management, and a modern React frontend stack. The review feedback identifies critical security and reliability concerns: a path traversal vulnerability in prompt configuration normalization, an information leak via file existence probing in the UI asset server, and a potential resource leak in the draft application process where temporary directories may not be cleaned up during failures.
| def _normalize_config_path(raw_path: str) -> str: | ||
| candidate = Path(raw_path).expanduser() | ||
| if candidate.is_absolute(): | ||
| return str(candidate.resolve()) | ||
| return str(_resolve_prompt_file_path(raw_path)) |
There was a problem hiding this comment.
The _normalize_config_path function allows absolute paths to bypass the repository root boundary check. While _resolve_prompt_file_path correctly enforces that paths must stay within the repo_root, this nested helper explicitly returns absolute paths without validation. This could allow an operator to configure prompt paths to sensitive locations on the host filesystem.
def _normalize_config_path(raw_path: str) -> str:
expanded = str(Path(raw_path).expanduser())
return str(_resolve_prompt_file_path(expanded))| staged_profiles_root = self._stage_yaml_directory( | ||
| self.profiles_root, | ||
| service.list_profiles(), | ||
| serializer=serialize_profile_config, | ||
| ) | ||
| staged_instances_root = self._stage_yaml_directory( | ||
| self.instances_root, | ||
| service.list_instances(), | ||
| serializer=serialize_instance_config, | ||
| ) | ||
| backups: list[tuple[Path, Path | None]] = [] | ||
| try: | ||
| backups.append( | ||
| (self.profiles_root, self._swap_staged_yaml_directory(self.profiles_root, staged_profiles_root)) | ||
| ) | ||
| backups.append( | ||
| (self.instances_root, self._swap_staged_yaml_directory(self.instances_root, staged_instances_root)) | ||
| ) | ||
| except Exception: | ||
| for root, backup_root in reversed(backups): | ||
| self._restore_yaml_directory(root, backup_root) | ||
| raise | ||
| finally: | ||
| for staged_root in (staged_profiles_root, staged_instances_root): | ||
| if staged_root.exists(): | ||
| shutil.rmtree(staged_root) | ||
|
|
||
| for _, backup_root in backups: | ||
| if backup_root is not None and backup_root.exists(): | ||
| shutil.rmtree(backup_root) | ||
| return self.reset() |
There was a problem hiding this comment.
The apply method contains a resource leak where temporary directories created by _stage_yaml_directory are not cleaned up if an exception occurs before the main try block is entered. For example, if the second call to _stage_yaml_directory fails, the first directory (staged_profiles_root) will remain on disk indefinitely. The staging logic should be moved inside the try block to ensure the finally clause can perform cleanup.
staged_roots: list[Path] = []
backups: list[tuple[Path, Path | None]] = []
try:
staged_profiles_root = self._stage_yaml_directory(
self.profiles_root,
service.list_profiles(),
serializer=serialize_profile_config,
)
staged_roots.append(staged_profiles_root)
staged_instances_root = self._stage_yaml_directory(
self.instances_root,
service.list_instances(),
serializer=serialize_instance_config,
)
staged_roots.append(staged_instances_root)
backups.append(
(self.profiles_root, self._swap_staged_yaml_directory(self.profiles_root, staged_profiles_root))
)
backups.append(
(self.instances_root, self._swap_staged_yaml_directory(self.instances_root, staged_instances_root))
)
except Exception:
for root, backup_root in reversed(backups):
self._restore_yaml_directory(root, backup_root)
raise
finally:
for staged_root in staged_roots:
if staged_root.exists():
shutil.rmtree(staged_root)
for _, backup_root in backups:
if backup_root is not None and backup_root.exists():
shutil.rmtree(backup_root)
return self.reset()| normalized_path = path.strip("/") | ||
| if normalized_path: | ||
| asset_path = dist_dir / normalized_path | ||
| if asset_path.is_file(): | ||
| return send_from_directory(dist_dir, normalized_path) |
There was a problem hiding this comment.
The _serve_admin_ui_path function probes for file existence using asset_path.is_file() before calling send_from_directory. Because the path is not validated to be within dist_dir before this probe, a client can use parent directory references (..) to determine if specific files exist outside the intended directory. While send_from_directory prevents serving the content, the difference in response (404 vs. falling through to index.html) creates an information leak.
| normalized_path = path.strip("/") | |
| if normalized_path: | |
| asset_path = dist_dir / normalized_path | |
| if asset_path.is_file(): | |
| return send_from_directory(dist_dir, normalized_path) | |
| normalized_path = path.strip("/") | |
| if normalized_path: | |
| try: | |
| resolved_dist = dist_dir.resolve() | |
| asset_path = (resolved_dist / normalized_path).resolve() | |
| asset_path.relative_to(resolved_dist) | |
| if asset_path.is_file(): | |
| return send_from_directory(dist_dir, normalized_path) | |
| except (ValueError, OSError): | |
| pass |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Restrict admin prompt config normalization to repo-root paths, harden admin UI static path handling, and ensure draft staging directories are always cleaned up on failures. Add regression coverage for prompt-config boundary checks, SPA path traversal fallback, and repo-root-aware prompt directory switching.
|
Pushed another review-fix pass in Fresh verification:
Review requests: |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive browser-based admin UI and a backend management layer for ChatMock, enabling in-memory draft editing of profiles and instances before persisting changes to YAML. It adds atomic file writing for prompts, a new Flask blueprint for admin routes, and a modern React frontend stack utilizing TanStack Query and React Hook Form. The review feedback highlights a bug in the UI's error handling logic where JSON error messages are not correctly extracted from Error objects, and suggests a more robust implementation for generating unique temporary file paths in the backend configuration service.
| export function toErrorMessage(error: unknown): string { | ||
| if (typeof error === "string") { | ||
| try { | ||
| const parsed = JSON.parse(error); | ||
| if ( | ||
| typeof parsed === "object" && | ||
| parsed !== null && | ||
| "error" in parsed && | ||
| typeof parsed.error === "object" && | ||
| parsed.error !== null && | ||
| "message" in parsed.error && | ||
| typeof parsed.error.message === "string" | ||
| ) { | ||
| return parsed.error.message; | ||
| } | ||
| } catch { | ||
| // Keep the original string when it is not JSON. | ||
| } | ||
| return error; | ||
| } | ||
|
|
||
| return error instanceof Error ? error.message : "Unknown error"; | ||
| } |
There was a problem hiding this comment.
The toErrorMessage function fails to extract JSON error messages when the input is an Error object. Since apiGet and apiJson throw new Error(await response.text()), the error parameter in TanStack Query hooks will be an Error instance. The current implementation only attempts to parse JSON if typeof error === "string", otherwise it returns error.message as a raw string (which is often a JSON string like {"error":{"message":"..."}}).
export function toErrorMessage(error: unknown): string {
const rawMessage = error instanceof Error ? error.message : typeof error === "string" ? error : "";
if (rawMessage) {
try {
const parsed = JSON.parse(rawMessage);
if (
typeof parsed === "object" &&
parsed !== null &&
"error" in parsed &&
typeof parsed.error === "object" &&
parsed.error !== null &&
"message" in parsed.error &&
typeof parsed.error.message === "string"
) {
return parsed.error.message;
}
} catch {
// Not JSON, return raw message
}
return rawMessage;
}
return "Unknown error";
}| def _reserve_temp_path(parent: Path, stem: str, suffix: str) -> Path: | ||
| candidate = parent / f".{stem}.{suffix}.{time.time_ns()}" | ||
| while candidate.exists(): | ||
| candidate = parent / f".{stem}.{suffix}.{time.time_ns()}" | ||
| return candidate |
There was a problem hiding this comment.
The _reserve_temp_path function uses a busy-wait loop with time.time_ns() to find a unique path. While collisions are unlikely in a single-user context, it is more idiomatic and robust to use tempfile.mktemp or append a random string (e.g., via secrets.token_hex) to ensure uniqueness without relying on clock resolution.
| def _reserve_temp_path(parent: Path, stem: str, suffix: str) -> Path: | |
| candidate = parent / f".{stem}.{suffix}.{time.time_ns()}" | |
| while candidate.exists(): | |
| candidate = parent / f".{stem}.{suffix}.{time.time_ns()}" | |
| return candidate | |
| import secrets | |
| def _reserve_temp_path(parent: Path, stem: str, suffix: str) -> Path: | |
| return parent / f".{stem}.{suffix}.{secrets.token_hex(8)}" |
1 participant
What changed
ui/adminReact app with React Router, Tailwind v4, shadcn/ui, TanStack Query, and React Hook FormWhy
The live JSON admin endpoints and manual config workflow had grown large enough that ChatMock needed a dedicated operator UI with explicit draft/apply behavior and clearer frontend boundaries.
Impact
/admin/uiValidation
cd ui/admin && npm testcd ui/admin && npm run build/home/drj/tools/chatmock/.venv/bin/python -m pytest tests/test_admin_routes.py tests/test_admin_draft_service.py tests/test_profile_registry.py tests/test_instance_registry.py tests/test_instance_service.py tests/test_routes.py tests/test_cli.py -qhttp://127.0.0.1:18000/admin/ui/Summary by CodeRabbit
New Features
Documentation
Tests
Chore