feat(web): mobile responsive UI, slash autocomplete, and projects sidebar#1209
Closed
tboome33 wants to merge 45 commits intocoleam00:devfrom
Closed
feat(web): mobile responsive UI, slash autocomplete, and projects sidebar#1209tboome33 wants to merge 45 commits intocoleam00:devfrom
tboome33 wants to merge 45 commits intocoleam00:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge refactor adding a provider layer ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MessageInput
participant SlashCommandMenu
participant API
User->>MessageInput: Type "/"
MessageInput->>SlashCommandMenu: open(query="")
loop typing
User->>MessageInput: Type chars
MessageInput->>SlashCommandMenu: update(query)
SlashCommandMenu->>API: GET /workflows?cwd=...
API-->>SlashCommandMenu: workflows
SlashCommandMenu->>MessageInput: render filtered list
end
User->>SlashCommandMenu: Select item (Enter/Click)
SlashCommandMenu->>MessageInput: onSelect("/workflow ")
MessageInput->>User: insert "/workflow "
sequenceDiagram
participant User
participant TunnelPopover
participant FrontendAPI
participant ServerTunnel
participant Cloudflared
User->>TunnelPopover: Click "Start Tunnel"
TunnelPopover->>FrontendAPI: POST /api/tunnel/start
FrontendAPI->>ServerTunnel: startTunnel(5173)
ServerTunnel->>Cloudflared: spawn cloudflared --url http://localhost:5173
Cloudflared-->>ServerTunnel: stdout with trycloudflare URL
ServerTunnel->>FrontendAPI: { status: "active", url }
FrontendAPI-->>TunnelPopover: return status/url
User->>TunnelPopover: Copy URL / Click "Stop"
TunnelPopover->>FrontendAPI: DELETE /api/tunnel/stop
FrontendAPI->>ServerTunnel: stopTunnel()
ServerTunnel->>Cloudflared: kill process
ServerTunnel-->>FrontendAPI: { status: "inactive" }
FrontendAPI-->>TunnelPopover: update UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (10)
packages/web/src/components/workflows/BuilderToolbar.tsx (2)
242-244: Abbreviated button label "Val" may reduce clarity.The abbreviation "Val" for "Validate" is non-standard and may not be immediately recognizable to users. Given the toolbar now scrolls horizontally on mobile, the full label would still be accessible. Consider keeping "Validate" or using a more common abbreviation like a checkmark icon.
♻️ Suggested alternatives
Option 1: Keep the full label:
<Button variant="outline" size="xs" onClick={onValidate}> - Val + Validate </Button>Option 2: Use an icon with tooltip for compact display:
import { CheckCircle } from 'lucide-react'; <Button variant="outline" size="xs" onClick={onValidate} title="Validate"> <CheckCircle className="h-3 w-3" /> </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/BuilderToolbar.tsx` around lines 242 - 244, The button labeled "Val" is unclear; update the Button that calls onValidate to use a clearer label or compact icon+tooltip: either replace the text "Val" with "Validate" in the Button component or swap to an accessible icon (e.g., a check icon) and add a title/aria-label of "Validate" so screen readers and tooltips convey purpose; locate the Button with onClick={onValidate} in BuilderToolbar and apply the change.
67-82: Consider using ResizeObserver for more precise element resize detection.The current implementation listens to
window.resize, which works but won't catch cases where the toolbar's size changes without a window resize (e.g., sidebar toggle, dynamic content changes). AResizeObserveron the scroll container would be more robust.♻️ Optional enhancement using ResizeObserver
useEffect(() => { const el = scrollRef.current; if (!el) return; const check = (): void => { setCanScrollRight(el.scrollLeft + el.clientWidth < el.scrollWidth - 4); }; check(); el.addEventListener('scroll', check); - window.addEventListener('resize', check); + const resizeObserver = new ResizeObserver(check); + resizeObserver.observe(el); return (): void => { el.removeEventListener('scroll', check); - window.removeEventListener('resize', check); + resizeObserver.disconnect(); }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/BuilderToolbar.tsx` around lines 67 - 82, Replace the window resize listener with a ResizeObserver attached to the scroll container: inside the effect that reads scrollRef.current (the element used in check), create a ResizeObserver that calls the existing check function whenever the element's size changes, observe the element, and ensure you disconnect the observer in the cleanup along with removing the scroll event listener; keep using setCanScrollRight and the check helper as-is so logic remains unchanged.packages/web/src/index.css (1)
227-237: Aggressive scrollbar hiding on mobile — verify accessibility.Hiding all scrollbars via
* { scrollbar-width: none }removes visual scroll indicators globally on mobile. This relies on all scrollable content being within explicitScrollAreacomponents. If any native overflow:auto containers exist outside these wrappers, users may not realize content is scrollable.The inline comment acknowledges this design decision, so this is acceptable if verified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/index.css` around lines 227 - 237, The global mobile CSS rule hiding scrollbars is too broad (selectors '*' and '*::-webkit-scrollbar' inside the `@media` (max-width: 768px) block) and can remove essential visual scroll indicators; audit the codebase for any native overflow:auto containers not wrapped by your ScrollArea component and either restrict the rule to the ScrollArea-specific selector(s) (e.g., .scroll-area or the component's root class) or add exceptions for native scrollable elements, and update the comment to state that you verified there are no unwrapped native scroll containers.packages/web/index.html (1)
20-24: Hardcoded dark background may flash on light theme.The inline
background-color:#0f1117`` is dark-themed. Users with light theme preference (from localStorage or system) may see a brief dark flash before React hydrates and applies the correct theme class.Consider using a CSS variable or a small inline script that reads the theme preference before the body renders:
💡 Optional: Theme-aware initial background
<script> (function() { var theme = localStorage.getItem('archon-theme'); if (!theme) theme = window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; document.documentElement.classList.toggle('dark', theme === 'dark'); })(); </script> <style> body { background-color: var(--background, `#0f1117`); } </style>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/index.html` around lines 20 - 24, Replace the hardcoded inline body background-color (`#0f1117`) with a theme-aware approach: remove the static background-color from the <style> block and instead use a CSS variable (e.g. --background) for body background, and add a small inline script that runs before render to read the 'archon-theme' localStorage key (falling back to prefers-color-scheme) and set document.documentElement.classList.toggle('dark', ...) or set the --background variable accordingly so the initial paint matches the user's theme preference.packages/web/src/contexts/ThemeContext.tsx (1)
31-39: Consider consolidating effects or documenting the separate effect rationale.Two separate
useEffecthooks each write to localStorage and toggle a DOM class. This is correct but could be combined into a single effect if both states are always persisted together, reducing effect count. Current separation is also valid if you anticipate them diverging in behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/contexts/ThemeContext.tsx` around lines 31 - 39, The two useEffect hooks that update DOM classes and localStorage (the effects referencing theme and compactLayout, using document.documentElement.classList.toggle and localStorage.setItem) should either be consolidated into a single effect or have an explicit comment explaining why they remain separate; to fix, either merge into one useEffect that depends on [theme, compactLayout] and performs both class toggles and both localStorage.setItem calls, or add a short comment above each useEffect (referencing theme and compactLayout) describing the rationale for keeping them separate (e.g., expected divergent behavior or independent updates)..prettierignore (1)
45-47: Consider clarifying the comment for new file exclusions.The comment says "pre-existing intentional formatting exceptions," but
useVisualViewport.tsappears to be a new file introduced in this PR. If the file is new with intentional non-standard formatting, consider updating the comment to reflect this more accurately (e.g., "Intentional formatting exceptions — do not reformat").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.prettierignore around lines 45 - 47, Update the header comment above the explicit file exclusions in .prettierignore to clearly indicate these are intentional exceptions (e.g., change "Pre-existing intentional formatting exceptions (do not reformat)" to "Intentional formatting exceptions — do not reformat") so new files like packages/web/src/lib/useVisualViewport.ts are accurately described; ensure the comment text near the listed entries (packages/web/index.html and packages/web/src/lib/useVisualViewport.ts) reflects that these files should not be reformatted.packages/web/src/components/layout/Layout.tsx (1)
36-47: Consider adding keyboard dismissal for the backdrop.The backdrop correctly closes the drawer on click. For improved accessibility, consider allowing
Escapekey to close the drawer as well. This could be implemented with auseEffectthat listens for the Escape key whenopenis true.♻️ Optional: Add Escape key handler
+ // Close drawer on Escape key + useEffect(() => { + if (!open) return; + const handleKeyDown = (e: KeyboardEvent): void => { + if (e.key === 'Escape') setOpen(false); + }; + window.addEventListener('keydown', handleKeyDown); + return (): void => { + window.removeEventListener('keydown', handleKeyDown); + }; + }, [open]); + return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/Layout.tsx` around lines 36 - 47, Add an Escape-key handler in the Layout component: when open is true register a keydown listener (via useEffect) that calls setOpen(false) if event.key === 'Escape', and remove the listener on cleanup or when open becomes false; ensure the effect depends on open and setOpen so the listener is only active while the backdrop/drawer is open.packages/web/src/components/workflows/WorkflowBuilder.tsx (2)
575-588: Backdrop accessibility: inconsistent role and tabIndex.The backdrop has
role="button"buttabIndex={-1}, making it non-focusable. For a dismissal overlay, consider usingrole="presentation"or removing the role entirely since the primary interaction is click-to-dismiss and it shouldn't be in the tab order.♻️ Simplify backdrop semantics
<div className="absolute inset-0 z-20 bg-black/40" - role="button" - tabIndex={-1} - aria-label="Close library" + aria-hidden="true" onClick={(): void => { setShowLibrary(false); }} - onKeyDown={(e): void => { - if (e.key === 'Escape' || e.key === 'Enter' || e.key === ' ') { - setShowLibrary(false); - } - }} />The Escape key handling could be moved to a
useEffecton the component level instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/WorkflowBuilder.tsx` around lines 575 - 588, The backdrop div in WorkflowBuilder uses role="button" but tabIndex={-1}, which is inconsistent and makes it unfocusable; change the backdrop to a purely presentational element by removing role="button" (or set role="presentation") and ensure it is not focusable (keep tabIndex omitted or -1 removed), and keep click-to-dismiss calling setShowLibrary(false). Also move the Escape key handling out of the backdrop onKeyDown into a component-level useEffect inside WorkflowBuilder that listens for 'keydown' and calls setShowLibrary(false) when e.key === 'Escape' (preserve Enter/Space behavior only if you reintroduce a focusable dismiss control).
403-426: Duplicate node creation logic with handleNodeAdd.The
onAddPromptandonAddBashkeyboard actions duplicate the node creation pattern already encapsulated inhandleNodeAdd. Consider refactoring to reusehandleNodeAdd:♻️ Refactor to reuse handleNodeAdd
onAddPrompt: (): void => { - const id = `node-${crypto.randomUUID()}`; - const newNode: DagFlowNode = { - id, - type: 'dagNode', - position: { x: 200, y: 200 }, - data: { id, label: 'Prompt', nodeType: 'prompt' }, - }; - pushSnapshot({ nodes, edges }); - setNodes(nds => [...nds, newNode]); - markDirty(); + handleNodeAdd('prompt', 'Prompt'); }, onAddBash: (): void => { - const id = `node-${crypto.randomUUID()}`; - const newNode: DagFlowNode = { - id, - type: 'dagNode', - position: { x: 200, y: 200 }, - data: { id, label: 'Shell', nodeType: 'bash' }, - }; - pushSnapshot({ nodes, edges }); - setNodes(nds => [...nds, newNode]); - markDirty(); + handleNodeAdd('bash', 'Shell'); },Note: You'll need to add
handleNodeAddto theuseMemodependency array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/WorkflowBuilder.tsx` around lines 403 - 426, The onAddPrompt and onAddBash handlers duplicate the node creation logic; refactor them to call the existing handleNodeAdd(id, label, nodeType) (or adapt handleNodeAdd signature if needed) instead of repeating pushSnapshot/setNodes/markDirty. Replace the body of onAddPrompt and onAddBash with calls to handleNodeAdd with appropriate args (id, 'Prompt'/'Shell', 'prompt'/'bash') and ensure handleNodeAdd is added to the enclosing useMemo dependency array so the memoized handlers see updates.packages/web/src/components/conversations/SlashCommandMenu.tsx (1)
29-39: Consider showing loading state for first-time fetch.When workflows haven't been cached yet, the menu returns
null(line 92) while loading. Users might see the menu briefly disappear and reappear. Consider showing a minimal loading indicator:♻️ Optional: Add loading skeleton
+ const { data: workflows, isLoading } = useQuery({ - const { data: workflows } = useQuery({ queryKey: ['workflows', cwd ?? null], queryFn: () => listWorkflows(cwd), staleTime: 30_000, }); + if (isLoading) { + return ( + <div className="absolute bottom-full left-0 right-0 z-50 mb-1 rounded-lg border border-border bg-surface shadow-lg p-3"> + <div className="h-4 w-32 bg-surface-elevated animate-pulse rounded" /> + </div> + ); + } + if (filtered.length === 0) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/conversations/SlashCommandMenu.tsx` around lines 29 - 39, The menu currently returns null while the initial workflows fetch is in progress; update the useQuery call to capture isLoading/isFetching (e.g., const { data: workflows, isLoading } = useQuery(...)) and change the render path that currently returns null to show a minimal loading indicator or skeleton when isLoading is true and workflows is undefined; keep the existing filtering logic (filtered variable using workflows and MAX_RESULTS) and only fall back to the empty/null state when not loading and workflows is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.mjs`:
- Line 30: Remove 'packages/web/src/lib/useVisualViewport.ts' from the ESLint
ignore list in eslint.config.mjs (or update the justification comment) because
it was newly added and does not match the "pre-existing no-semicolon style"
claim; locate the ignore array entry for that exact string and either delete
that entry or replace the misleading comment with a correct justification, or
alternatively add a file-level eslint-disable comment inside
useVisualViewport.ts if a specific rule truly requires ignoring that file.
In `@packages/server/src/routes/api.ts`:
- Around line 2583-2598: These tunnel control handlers (getTunnelRoute,
postTunnelStartRoute, deleteTunnelStopRoute) are currently exposed via the
global /api/* CORS policy; restrict them by removing them from the wildcard CORS
surface and adding an explicit gate in their handlers: validate either a
one-time secret/token (passed in a header or body) or enforce local-only access
by checking the request Origin/Host/IP (allow only localhost/127.0.0.1) before
calling startTunnel/stopTunnel or returning getTunnelState; also ensure the
route registration via registerOpenApiRoute uses the restricted CORS settings or
a separate registration path so these endpoints are not reachable from arbitrary
origins.
- Around line 834-888: The inline z.object response schemas in getTunnelRoute,
postTunnelStartRoute, and deleteTunnelStopRoute must be moved into the system
domain schemas file under the routes/schemas directory as exported named schemas
(e.g., TunnelStatusResponseSchema, TunnelStartResponseSchema,
TunnelStopResponseSchema) authored with z from `@hono/zod-openapi`; then update
api.ts to import those named schemas and replace the inline schema definitions
with the imported ones, and ensure each route is registered via
registerOpenApiRoute(createRoute({...}), handler) per the project guideline.
In `@packages/server/src/tunnel.ts`:
- Around line 52-98: The startTunnel function can remain in 'starting'
indefinitely if cloudflared never emits a URL; add a timeout (e.g., 30s) inside
startTunnel that starts when proc is spawned and, if state.url is still null
when it fires, sets state.status = 'error', clears state.process/state.url, and
ensures proc is killed/cleaned up; when a URL is detected (extractUrl) or on
proc 'exit'/'error' clear that timeout to avoid races. Use the existing
variables/proc/state/handleChunk/extractUrl to locate where to add the timer and
the cleanup logic.
In `@packages/web/src/components/chat/MessageInput.tsx`:
- Around line 226-235: The current slash detection using lastIndexOf('/') with
checks on beforeSlash and afterSlash can still trigger inside URLs (e.g.,
"https://.../path"); update the logic around slashIdx and setSlashQuery so you
only treat a slash as a command trigger when it's the first slash after the last
whitespace and not part of a protocol or double-slash URL. Concretely, locate
the code using variables slashIdx, newValue, beforeSlash, afterSlash and add a
guard that finds lastWhitespaceIdx = newValue.lastIndexOf(' ', slashIdx - 1) (or
-1 if none) and ensure slashIdx > lastWhitespaceIdx, and also skip if the chars
immediately before slash indicate a URL (e.g., beforeSlash.endsWith('://') or
beforeSlash.endsWith('//') or beforeSlash.endsWith(':')). Only then call
setSlashQuery(afterSlash); otherwise setSlashQuery(null).
In `@packages/web/src/components/layout/TunnelPopover.tsx`:
- Around line 50-53: The handlers handleStart and handleStop should not
optimistically set tunnel status before confirming the API succeeded; instead,
call fetch('/api/tunnel/start') or fetch('/api/tunnel/stop'), check response.ok
and only then call setTunnel to 'starting' or 'inactive', and on non-ok
responses setTunnel to an 'error' status (or include an error message) and throw
or log the error so failures are surfaced; update both handleStart and the
corresponding handleStop implementation to gate state changes on response.ok,
move failures into the 'error' state, and ensure any thrown errors aren't
silently swallowed.
- Around line 4-9: The code currently trusts res.json() as TunnelState and
defines a local TunnelState/TunnelStatus that can drift from the server; replace
this with a Zod schema (e.g., tunnelStateSchema) that validates the payload
shape (status: literal union 'inactive'|'starting'|'active'|'error' and url:
string|null), derive the TypeScript type via z.infer<typeof tunnelStateSchema>
instead of the hand-written TunnelState, and validate the fetched data before
calling setTunnel (reject/log and handle invalid payloads). Update any uses of
TunnelState/TunnelStatus to the inferred type and ensure the fetch handler uses
tunnelStateSchema.parse or safeParse and reacts accordingly.
In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx`:
- Around line 260-263: The wrapper div in ProjectsSidebar.tsx currently has
onClick={onNavigate}, which closes the mobile drawer for any click inside the
expanded section; remove that onClick from the container and instead attach
onNavigate only to actual navigation controls (e.g., the project Link/NavLink
elements or the explicit navigation buttons inside the expanded project rows).
Also ensure non-navigation controls (like the "Project settings" toggle or any
future inputs/buttons) do not call onNavigate—either don't pass it to them or
call event.stopPropagation() in their handlers—so only real navigation clicks
(the link clicks that change routes) will close the drawer; reference the
onNavigate prop and the navigation Link/NavLink elements in ProjectsSidebar and
ChatPage components when making the change.
- Around line 74-77: The selection races the stale-selection cleanup because
setSelectedProjectId is called before the ['codebases'] cache contains the new
codebase; update the cache (or refetch) before selecting: either await
queryClient.invalidateQueries({ queryKey: ['codebases'] })/refetch the query
after addCodebase resolves, or optimistically insert the returned codebase into
the ['codebases'] cache with queryClient.setQueryData so the new id is present
when you call setSelectedProjectId; adjust the addCodebase.then handler around
queryClient.invalidateQueries, queryClient.setQueryData, and
setSelectedProjectId (and keep ProjectContext's stale-selection effect in mind).
In `@packages/web/src/components/workflows/WorkflowCanvas.tsx`:
- Around line 25-27: PICKER_WIDTH in WorkflowCanvas.tsx is set to 240px which
underestimates the widest QuickAddPicker variant (w-64 = 256px) and can let the
skill/MCP pane overflow; update the clamp to use 256px (or better, import/share
a single exported constant from QuickAddPicker.tsx) and apply the same change
where PICKER_WIDTH/PICKER_HEIGHT are used (including the other occurrences noted
around lines 253-257) so the canvas positioning uses the correct max width for
the widest picker variant.
In `@packages/web/src/contexts/MobileNavContext.tsx`:
- Around line 8-16: Make MobileNavContext nullable and add a fail-fast check in
useMobileNav by changing the context creation to accept MobileNavContextValue |
null (MobileNavContext) and updating useMobileNav to throw when the context is
null; update the default export value to null instead of a no-op setOpen so that
createContext<MobileNavContextValue | null>(null) is used and the hook
(useMobileNav) does a const ctx = useContext(MobileNavContext); if (!ctx) throw
new Error("useMobileNav must be used within a MobileNavProvider"); return ctx.
In `@packages/web/src/lib/useVisualViewport.ts`:
- Around line 13-30: The hook useVisualViewport currently reads
window.visualViewport during render and never registers a window resize fallback
when visualViewport is missing; make it SSR-safe by guarding initial state with
typeof window !== 'undefined' (use a safe default like 0) or initialize height
in a useEffect when window becomes available, and in the effect check for
window.visualViewport: if present attach vv resize/scroll listeners as before
(handler using vv.height), otherwise attach a window 'resize' listener that
calls setHeight(window.innerHeight); ensure the cleanup removes the correct
listeners and keep references to handler and vv to avoid stale closures.
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 60-73: The switch button with role="switch" (the element using
compactLayout state and toggleCompactLayout handler) is missing a programmatic
label; add an accessible name by adding either an aria-label (e.g.,
aria-label="Compact layout") on that button or wire it to the nearby descriptive
text using aria-labelledby referencing the id of that label element; ensure the
chosen label succinctly describes the toggle (e.g., "Compact layout") so screen
readers announce the switch state correctly.
In `@RESPONSIVE_UI_PATCH.md`:
- Around line 681-705: The fenced code blocks listing file paths (e.g.,
packages/web/src/contexts/MobileNavContext.tsx,
packages/web/src/components/layout/Layout.tsx,
packages/web/src/components/layout/TopNav.tsx, and the ChatPage.tsx block) lack
language tags causing MD040; update each triple-backtick fence that only
contains a path to include a language identifier (use "text") so the blocks
become ```text <path> ``` to satisfy markdownlint and preserve rendering
consistency.
---
Nitpick comments:
In @.prettierignore:
- Around line 45-47: Update the header comment above the explicit file
exclusions in .prettierignore to clearly indicate these are intentional
exceptions (e.g., change "Pre-existing intentional formatting exceptions (do not
reformat)" to "Intentional formatting exceptions — do not reformat") so new
files like packages/web/src/lib/useVisualViewport.ts are accurately described;
ensure the comment text near the listed entries (packages/web/index.html and
packages/web/src/lib/useVisualViewport.ts) reflects that these files should not
be reformatted.
In `@packages/web/index.html`:
- Around line 20-24: Replace the hardcoded inline body background-color
(`#0f1117`) with a theme-aware approach: remove the static background-color from
the <style> block and instead use a CSS variable (e.g. --background) for body
background, and add a small inline script that runs before render to read the
'archon-theme' localStorage key (falling back to prefers-color-scheme) and set
document.documentElement.classList.toggle('dark', ...) or set the --background
variable accordingly so the initial paint matches the user's theme preference.
In `@packages/web/src/components/conversations/SlashCommandMenu.tsx`:
- Around line 29-39: The menu currently returns null while the initial workflows
fetch is in progress; update the useQuery call to capture isLoading/isFetching
(e.g., const { data: workflows, isLoading } = useQuery(...)) and change the
render path that currently returns null to show a minimal loading indicator or
skeleton when isLoading is true and workflows is undefined; keep the existing
filtering logic (filtered variable using workflows and MAX_RESULTS) and only
fall back to the empty/null state when not loading and workflows is empty.
In `@packages/web/src/components/layout/Layout.tsx`:
- Around line 36-47: Add an Escape-key handler in the Layout component: when
open is true register a keydown listener (via useEffect) that calls
setOpen(false) if event.key === 'Escape', and remove the listener on cleanup or
when open becomes false; ensure the effect depends on open and setOpen so the
listener is only active while the backdrop/drawer is open.
In `@packages/web/src/components/workflows/BuilderToolbar.tsx`:
- Around line 242-244: The button labeled "Val" is unclear; update the Button
that calls onValidate to use a clearer label or compact icon+tooltip: either
replace the text "Val" with "Validate" in the Button component or swap to an
accessible icon (e.g., a check icon) and add a title/aria-label of "Validate" so
screen readers and tooltips convey purpose; locate the Button with
onClick={onValidate} in BuilderToolbar and apply the change.
- Around line 67-82: Replace the window resize listener with a ResizeObserver
attached to the scroll container: inside the effect that reads scrollRef.current
(the element used in check), create a ResizeObserver that calls the existing
check function whenever the element's size changes, observe the element, and
ensure you disconnect the observer in the cleanup along with removing the scroll
event listener; keep using setCanScrollRight and the check helper as-is so logic
remains unchanged.
In `@packages/web/src/components/workflows/WorkflowBuilder.tsx`:
- Around line 575-588: The backdrop div in WorkflowBuilder uses role="button"
but tabIndex={-1}, which is inconsistent and makes it unfocusable; change the
backdrop to a purely presentational element by removing role="button" (or set
role="presentation") and ensure it is not focusable (keep tabIndex omitted or -1
removed), and keep click-to-dismiss calling setShowLibrary(false). Also move the
Escape key handling out of the backdrop onKeyDown into a component-level
useEffect inside WorkflowBuilder that listens for 'keydown' and calls
setShowLibrary(false) when e.key === 'Escape' (preserve Enter/Space behavior
only if you reintroduce a focusable dismiss control).
- Around line 403-426: The onAddPrompt and onAddBash handlers duplicate the node
creation logic; refactor them to call the existing handleNodeAdd(id, label,
nodeType) (or adapt handleNodeAdd signature if needed) instead of repeating
pushSnapshot/setNodes/markDirty. Replace the body of onAddPrompt and onAddBash
with calls to handleNodeAdd with appropriate args (id, 'Prompt'/'Shell',
'prompt'/'bash') and ensure handleNodeAdd is added to the enclosing useMemo
dependency array so the memoized handlers see updates.
In `@packages/web/src/contexts/ThemeContext.tsx`:
- Around line 31-39: The two useEffect hooks that update DOM classes and
localStorage (the effects referencing theme and compactLayout, using
document.documentElement.classList.toggle and localStorage.setItem) should
either be consolidated into a single effect or have an explicit comment
explaining why they remain separate; to fix, either merge into one useEffect
that depends on [theme, compactLayout] and performs both class toggles and both
localStorage.setItem calls, or add a short comment above each useEffect
(referencing theme and compactLayout) describing the rationale for keeping them
separate (e.g., expected divergent behavior or independent updates).
In `@packages/web/src/index.css`:
- Around line 227-237: The global mobile CSS rule hiding scrollbars is too broad
(selectors '*' and '*::-webkit-scrollbar' inside the `@media` (max-width: 768px)
block) and can remove essential visual scroll indicators; audit the codebase for
any native overflow:auto containers not wrapped by your ScrollArea component and
either restrict the rule to the ScrollArea-specific selector(s) (e.g.,
.scroll-area or the component's root class) or add exceptions for native
scrollable elements, and update the comment to state that you verified there are
no unwrapped native scroll containers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c4e01b0-5148-482b-8c5a-ca91cd79b012
📒 Files selected for processing (28)
.prettierignoreRESPONSIVE_UI_PATCH.mddocs/cloudflare-tunnel.mdeslint.config.mjspackages/server/src/routes/api.tspackages/server/src/tunnel.tspackages/web/index.htmlpackages/web/src/App.tsxpackages/web/src/components/chat/MessageInput.tsxpackages/web/src/components/conversations/ConversationItem.tsxpackages/web/src/components/conversations/SlashCommandMenu.tsxpackages/web/src/components/layout/Layout.tsxpackages/web/src/components/layout/TopNav.tsxpackages/web/src/components/layout/TunnelPopover.tsxpackages/web/src/components/sidebar/ProjectsSidebar.tsxpackages/web/src/components/workflows/BuilderToolbar.tsxpackages/web/src/components/workflows/NodeLibrary.tsxpackages/web/src/components/workflows/QuickAddPicker.tsxpackages/web/src/components/workflows/WorkflowBuilder.tsxpackages/web/src/components/workflows/WorkflowCanvas.tsxpackages/web/src/contexts/MobileNavContext.tsxpackages/web/src/contexts/ThemeContext.tsxpackages/web/src/index.csspackages/web/src/lib/api.tspackages/web/src/lib/useVisualViewport.tspackages/web/src/routes/ChatPage.tsxpackages/web/src/routes/SettingsPage.tsxpackages/web/vite.config.ts
| 'packages/web/components.json', | ||
| 'packages/web/src/components/ui/**', // shadcn/ui auto-generated components | ||
| 'packages/web/src/lib/utils.ts', // shadcn/ui utility file | ||
| 'packages/web/src/lib/useVisualViewport.ts', // pre-existing no-semicolon style — do not reformat |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the useVisualViewport.ts file to understand why it needs to be ignored
cat packages/web/src/lib/useVisualViewport.ts 2>/dev/null | head -50Repository: coleam00/Archon
Length of output: 1022
🏁 Script executed:
#!/bin/bash
# Check git status to see if the file is new
git ls-files packages/web/src/lib/useVisualViewport.ts | wc -l
# Get the full file content
wc -l packages/web/src/lib/useVisualViewport.ts
cat -n packages/web/src/lib/useVisualViewport.ts
# Check eslint.config.mjs context
cat -n eslint.config.mjs | sed -n '25,35p'Repository: coleam00/Archon
Length of output: 1912
🏁 Script executed:
#!/bin/bash
# Check when the file was first added to the repository
git log --follow --oneline --all -- packages/web/src/lib/useVisualViewport.ts
# Also check current git status
git status packages/web/src/lib/useVisualViewport.ts 2>/dev/null || echo "Not in current status"
# Check if file appears in HEAD vs the current PR branch
git show HEAD:packages/web/src/lib/useVisualViewport.ts 2>/dev/null | head -5 || echo "File not in HEAD"Repository: coleam00/Archon
Length of output: 308
Remove useVisualViewport.ts from ESLint ignores or correct the justification.
The file was just added in this PR (not pre-existing) and consistently uses semicolons—contradicting the comment's claim about "pre-existing no-semicolon style." The file is properly typed, well-formatted, and follows project conventions, so there's no reason to ignore it. Either remove it from the ignore list or add a file-level /* eslint-disable */ comment if there's a specific linting rule that conflicts with the intended code style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.mjs` at line 30, Remove
'packages/web/src/lib/useVisualViewport.ts' from the ESLint ignore list in
eslint.config.mjs (or update the justification comment) because it was newly
added and does not match the "pre-existing no-semicolon style" claim; locate the
ignore array entry for that exact string and either delete that entry or replace
the misleading comment with a correct justification, or alternatively add a
file-level eslint-disable comment inside useVisualViewport.ts if a specific rule
truly requires ignoring that file.
Comment on lines
+681
to
+705
| ``` | ||
| packages/web/src/contexts/MobileNavContext.tsx | ||
| ``` | ||
|
|
||
| → Copier-coller le code de la section 1 ci-dessus. | ||
|
|
||
| 2. **Remplacer `Layout.tsx`** : | ||
|
|
||
| ``` | ||
| packages/web/src/components/layout/Layout.tsx | ||
| ``` | ||
|
|
||
| → Remplacer le contenu par le code de la section 2. | ||
|
|
||
| 3. **Remplacer `TopNav.tsx`** : | ||
|
|
||
| ``` | ||
| packages/web/src/components/layout/TopNav.tsx | ||
| ``` | ||
|
|
||
| → Remplacer le contenu par le code de la section 3. | ||
|
|
||
| 4. **Remplacer `ChatPage.tsx`** : | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language tags to these fenced code blocks.
The fences in this section are missing language identifiers, which is why markdownlint is raising MD040. Using text for path-only blocks here would clear the warning and keep the doc rendering consistent.
🧰 Tools
🪛 LanguageTool
[typographical] ~687-~687: Caractère d’apostrophe incorrect.
Context: ...la section 1 ci-dessus. 2. Remplacer Layout.tsx : ``` packages/web/src/compone...
(APOS_INCORRECT)
[typographical] ~695-~695: Caractère d’apostrophe incorrect.
Context: ...e code de la section 2. 3. Remplacer TopNav.tsx : ``` packages/web/src/compone...
(APOS_INCORRECT)
[typographical] ~703-~703: Caractère d’apostrophe incorrect.
Context: ...e code de la section 3. 4. Remplacer ChatPage.tsx : ``` packages/web/src/routes/...
(APOS_INCORRECT)
🪛 markdownlint-cli2 (0.22.0)
[warning] 681-681: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 689-689: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 697-697: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 705-705: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RESPONSIVE_UI_PATCH.md` around lines 681 - 705, The fenced code blocks
listing file paths (e.g., packages/web/src/contexts/MobileNavContext.tsx,
packages/web/src/components/layout/Layout.tsx,
packages/web/src/components/layout/TopNav.tsx, and the ChatPage.tsx block) lack
language tags causing MD040; update each triple-backtick fence that only
contains a path to include a language identifier (use "text") so the blocks
become ```text <path> ``` to satisfy markdownlint and preserve rendering
consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web/src/components/sidebar/ProjectsSidebar.tsx (1)
74-77:⚠️ Potential issue | 🟠 MajorRace condition: selection may be cleared before cache is updated.
setSelectedProjectId(codebase.id)is called immediately afterinvalidateQueries, butinvalidateQueriesis asynchronous and returns before the refetch completes. TheProjectContextstale-selection effect (lines 52-55 inProjectContext.tsx) clears anyselectedProjectIdnot found incodebases, so the newly added project may be deselected before the cache is populated.Either await the invalidation or optimistically insert the new codebase into the cache:
🔧 Proposed fix using optimistic cache update
void addCodebase(input) .then(codebase => { - void queryClient.invalidateQueries({ queryKey: ['codebases'] }); + // Optimistically insert new codebase so stale-selection cleanup doesn't clear it + queryClient.setQueryData<CodebaseResponse[]>(['codebases'], old => + old ? [...old, codebase] : [codebase] + ); + // Background refetch for consistency + void queryClient.invalidateQueries({ queryKey: ['codebases'] }); setSelectedProjectId(codebase.id);You'll also need to import the type:
-import { addCodebase, deleteCodebase } from '@/lib/api'; +import { addCodebase, deleteCodebase } from '@/lib/api'; +import type { CodebaseResponse } from '@/lib/api';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx` around lines 74 - 77, The addCodebase flow races with ProjectContext's stale-selection check: after calling queryClient.invalidateQueries(...) you setSelectedProjectId(codebase.id) immediately, which can be cleared before refetch finishes; fix by either awaiting the invalidation (await queryClient.invalidateQueries({ queryKey: ['codebases'] })) before calling setSelectedProjectId(codebase.id) or perform an optimistic cache update using queryClient.setQueryData(['codebases'], prev => insert the new codebase into the existing array) then call setSelectedProjectId; update the addCodebase handling in ProjectsSidebar.tsx and import any required Codebase type for the cache payload if you choose the optimistic route.
🧹 Nitpick comments (2)
packages/web/src/components/layout/Layout.tsx (2)
10-14: Consider extracting shared navigation config.The
navItemsarray duplicates thetabsarray fromTopNav.tsx(minus Settings). If these need to stay in sync, consider extracting to a shared constant file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/Layout.tsx` around lines 10 - 14, navItems in Layout.tsx duplicates tabs from TopNav.tsx and should be extracted to a shared constant to keep them in sync; create a shared export (e.g., NAV_TABS or sharedNavItems) that contains the common array shape, update Layout.tsx to import and use that constant instead of its local navItems, and update TopNav.tsx to import the same constant while keeping any extra entries like "Settings" appended locally in TopNav.tsx so behavior remains unchanged.
58-134: Consider adding focus trap for modal accessibility.When the drawer opens, focus should be trapped within it and returned to the trigger element when closed. Currently, keyboard users can Tab out of the drawer into the underlying content, which is unexpected for a modal. Libraries like
@radix-ui/react-dialogorfocus-trap-reactcan handle this automatically.This is not blocking but would improve accessibility compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/Layout.tsx` around lines 58 - 134, The mobile drawer (the aside element controlled by open and setOpen) needs a focus trap so keyboard users cannot tab out when it is open and focus is restored to the trigger when closed; wrap or replace the aside with a focus-trap (e.g., focus-trap-react) or a dialog component (e.g., `@radix-ui/react-dialog`), ensure initial focus lands on the first interactive element (the Close button/X or first NavLink), trap focus across NavLink items and Settings, and restore focus to the element that toggles open (the trigger that calls setOpen(true)) when setOpen(false) is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/components/layout/Layout.tsx`:
- Around line 59-68: The aside used for the mobile navigation in the Layout
component is missing a role attribute; add role="dialog" (or role="alertdialog"
if appropriate) to the aside element that currently has aria-modal="true" to
provide correct ARIA modal semantics and ensure screen readers recognize it as a
modal (locate the aside JSX in Layout.tsx where className and aria-modal are set
and add the role attribute).
---
Duplicate comments:
In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx`:
- Around line 74-77: The addCodebase flow races with ProjectContext's
stale-selection check: after calling queryClient.invalidateQueries(...) you
setSelectedProjectId(codebase.id) immediately, which can be cleared before
refetch finishes; fix by either awaiting the invalidation (await
queryClient.invalidateQueries({ queryKey: ['codebases'] })) before calling
setSelectedProjectId(codebase.id) or perform an optimistic cache update using
queryClient.setQueryData(['codebases'], prev => insert the new codebase into the
existing array) then call setSelectedProjectId; update the addCodebase handling
in ProjectsSidebar.tsx and import any required Codebase type for the cache
payload if you choose the optimistic route.
---
Nitpick comments:
In `@packages/web/src/components/layout/Layout.tsx`:
- Around line 10-14: navItems in Layout.tsx duplicates tabs from TopNav.tsx and
should be extracted to a shared constant to keep them in sync; create a shared
export (e.g., NAV_TABS or sharedNavItems) that contains the common array shape,
update Layout.tsx to import and use that constant instead of its local navItems,
and update TopNav.tsx to import the same constant while keeping any extra
entries like "Settings" appended locally in TopNav.tsx so behavior remains
unchanged.
- Around line 58-134: The mobile drawer (the aside element controlled by open
and setOpen) needs a focus trap so keyboard users cannot tab out when it is open
and focus is restored to the trigger when closed; wrap or replace the aside with
a focus-trap (e.g., focus-trap-react) or a dialog component (e.g.,
`@radix-ui/react-dialog`), ensure initial focus lands on the first interactive
element (the Close button/X or first NavLink), trap focus across NavLink items
and Settings, and restore focus to the element that toggles open (the trigger
that calls setOpen(true)) when setOpen(false) is called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24b090f8-c70b-4dc2-8080-5b791aa1a523
📒 Files selected for processing (6)
packages/web/src/components/chat/MessageInput.tsxpackages/web/src/components/conversations/SlashCommandMenu.tsxpackages/web/src/components/layout/Layout.tsxpackages/web/src/components/layout/TopNav.tsxpackages/web/src/components/sidebar/ProjectsSidebar.tsxpackages/web/src/routes/ChatPage.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/web/src/components/conversations/SlashCommandMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/src/components/chat/MessageInput.tsx
- packages/web/src/routes/ChatPage.tsx
6 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/web/src/components/sidebar/ProjectsSidebar.tsx (1)
74-80:⚠️ Potential issue | 🟠 MajorSelection may be cleared before cache is updated.
The
setSelectedProjectId(codebase.id)is called immediately afterinvalidateQueries, but the cache invalidation is not awaited. The stale-selection cleanup effect inProjectContext.tsx(lines 51-55) may clear this selection before the refetch completes because the new codebase isn't in the cache yet.🔧 Proposed fix: await invalidation before selecting
void addCodebase(input) - .then(codebase => { - void queryClient.invalidateQueries({ queryKey: ['codebases'] }); + .then(async codebase => { + await queryClient.invalidateQueries({ queryKey: ['codebases'] }); setSelectedProjectId(codebase.id); setShowAddInput(false); setAddValue(''); setAddError(null); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx` around lines 74 - 80, The selection can be cleared by the stale-selection effect because invalidateQueries is not awaited; update the addCodebase flow so that after addCodebase resolves you await queryClient.invalidateQueries({ queryKey: ['codebases'] }) before calling setSelectedProjectId(codebase.id), then proceed to setShowAddInput(false), setAddValue(''), and setAddError(null); reference addCodebase, queryClient.invalidateQueries, and setSelectedProjectId and keep the stale-selection cleanup in ProjectContext.tsx in mind when implementing the await.packages/web/src/components/layout/Layout.tsx (1)
138-147:⚠️ Potential issue | 🟡 MinorAdd
role="dialog"for proper ARIA modal semantics.The
aria-modal="true"attribute should be paired withrole="dialog"for screen readers to properly recognize this as a modal dialog. Without the role, the modal semantics may not be announced correctly.♿ Proposed fix
<aside className={cn( 'fixed top-12 bottom-0 left-0 z-50 flex w-72 flex-col border-r border-border shadow-2xl', 'transition-transform duration-300 ease-in-out', open ? 'translate-x-0' : '-translate-x-full' )} style={{ backgroundColor: 'var(--surface)' }} aria-label="Projects sidebar" + role="dialog" aria-modal="true" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/Layout.tsx` around lines 138 - 147, The aside used as the project modal in Layout.tsx lacks an explicit dialog role; update the aside element (the one with aria-modal="true" and className using cn and open) to include role="dialog" so screen readers recognize it as a modal dialog; ensure you add role="dialog" alongside the existing aria-modal="true" and keep the rest of the attributes (className, style, aria-label) unchanged.
🧹 Nitpick comments (3)
packages/web/src/components/sidebar/ProjectsSidebar.tsx (1)
103-118: Consider adding loading state during delete to prevent double-submit.The delete handler doesn't track a loading state, so users could click "Remove" multiple times before the operation completes, potentially triggering duplicate API calls.
♻️ Proposed improvement
const [deleteTargetId, setDeleteTargetId] = useState<string | null>(null); const [deleteError, setDeleteError] = useState<string | null>(null); + const [deleteLoading, setDeleteLoading] = useState(false); // ... const handleDeleteConfirm = useCallback((): void => { - if (!deleteTargetId) return; + if (!deleteTargetId || deleteLoading) return; const id = deleteTargetId; setDeleteError(null); + setDeleteLoading(true); void deleteCodebase(id) .then(() => { void queryClient.invalidateQueries({ queryKey: ['codebases'] }); if (id === selectedProjectId) { setSelectedProjectId(null); } setDeleteTargetId(null); }) .catch((err: Error) => { setDeleteError(err.message); - }); - }, [deleteTargetId, queryClient, selectedProjectId, setSelectedProjectId]); + }) + .finally(() => { + setDeleteLoading(false); + }); + }, [deleteTargetId, deleteLoading, queryClient, selectedProjectId, setSelectedProjectId]);Then disable the action button:
- <AlertDialogAction onClick={handleDeleteConfirm}>Remove</AlertDialogAction> + <AlertDialogAction onClick={handleDeleteConfirm} disabled={deleteLoading}> + {deleteLoading ? 'Removing...' : 'Remove'} + </AlertDialogAction>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx` around lines 103 - 118, The delete handler handleDeleteConfirm lacks a loading flag; add a local state like isDeleting and set it to true at the start and false in a finally block around the deleteCodebase(...) promise to prevent double submits and properly clear errors via setDeleteError; also short-circuit early if isDeleting or no deleteTargetId to avoid duplicate calls, update the hook dependency array to include the isDeleting setter, and disable the Remove/confirm button in the UI based on isDeleting so users cannot click while queryClient.invalidateQueries, setSelectedProjectId, and setDeleteTargetId are running.packages/web/src/components/layout/TopNav.tsx (1)
118-163: Consider adding keyboard navigation for the dropdown menu.The dropdown menu lacks keyboard navigation support (arrow keys to move between items, Escape to close from within the menu). While the outside-click handler closes on mousedown, keyboard users may find it difficult to navigate the menu items.
♿ Suggested improvement for keyboard accessibility
Consider adding an
onKeyDownhandler to the menu container:const handleMenuKeyDown = (e: React.KeyboardEvent): void => { if (e.key === 'Escape') { setMenuOpen(false); } }; // On the menu div: <div className="absolute right-0 top-full mt-1 z-50 w-48 ..." onKeyDown={handleMenuKeyDown} role="menu" >For full arrow-key navigation, consider using a headless UI library like Radix's
DropdownMenuwhich handles roving tabindex automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/TopNav.tsx` around lines 118 - 163, The dropdown lacks keyboard navigation: add an onKeyDown handler (e.g., handleMenuKeyDown) and attach it to the menu container div (the same element rendered when menuOpen is true) to handle Escape (call setMenuOpen(false)) and ArrowUp/ArrowDown to move focus between menu items; give each NavLink a role="menuitem" and tabIndex={-1}, collect the rendered item elements via refs or querySelectorAll from menuRef to implement roving focus (on ArrowDown focus the next item, ArrowUp the previous, wrapping as needed), and ensure the container has role="menu" so screen readers see it as a menu.packages/web/src/components/conversations/SlashCommandMenu.tsx (1)
33-39: Consider clampingselectedIndexto prevent stale selection on rapid filtering.When the
filteredarray shrinks (e.g., user types quickly),selectedIndexmay briefly exceed the new array length. The defensive check on line 77 prevents a crash, but pressing Enter in that window silently does nothing. Clamping the index inline ensures a valid selection is always available.♻️ Optional: Clamp selectedIndex within the useMemo
+ const safeSelectedIndex = Math.min(selectedIndex, Math.max(0, filtered.length - 1)); + // Keyboard navigation — capture phase so this fires before the textarea keydown handler useEffect(() => { const handleKeyDown = (e: KeyboardEvent): void => { if (filtered.length === 0) return; if (e.key === 'ArrowDown') { e.preventDefault(); setSelectedIndex(prev => (prev + 1) % filtered.length); } else if (e.key === 'ArrowUp') { e.preventDefault(); setSelectedIndex(prev => (prev - 1 + filtered.length) % filtered.length); } else if (e.key === 'Enter') { e.preventDefault(); - const entry = filtered[selectedIndex]; - if (entry) { - onSelect(entry.workflow.name); - } + onSelect(filtered[safeSelectedIndex].workflow.name); } else if (e.key === 'Escape') {Then use
safeSelectedIndexin the render foraria-activedescendantandaria-selectedas well.Also applies to: 70-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/conversations/SlashCommandMenu.tsx` around lines 33 - 39, Clamp the current selectedIndex whenever filtered changes so it never exceeds filtered.length - 1: after the useMemo that computes filtered, derive a safeSelectedIndex (e.g., Math.max(0, Math.min(selectedIndex, filtered.length - 1))) and use that everywhere instead of selectedIndex for rendering and actions. Replace uses in aria-activedescendant, aria-selected, and the Enter/selection handler to read safeSelectedIndex so rapid typing cannot leave a stale out-of-range selection that causes no-op Enter presses. Ensure filtered and selectedIndex names (from useMemo and the selection state) are the referenced symbols when implementing this clamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/components/conversations/SlashCommandMenu.tsx`:
- Around line 94-100: SlashCommandMenu currently sets aria-activedescendant on
the listbox; instead expose the active descendant ID and a stable listbox id so
the parent component (MessageInput.tsx) can apply aria-activedescendant to the
focused textarea and aria-controls to that listbox. Concretely, in
SlashCommandMenu compute and export/return an activeDescendantId (e.g.,
filtered.length ? `slash-option-${selectedIndex}` : undefined) and ensure the
listbox has an id (e.g., "slash-menu"); then update MessageInput.tsx to set the
textarea's aria-activedescendant to that activeDescendantId and aria-controls to
the listbox id. Ensure symbols referenced: SlashCommandMenu, selectedIndex,
filtered, activeDescendantId, and the textarea in MessageInput.tsx.
---
Duplicate comments:
In `@packages/web/src/components/layout/Layout.tsx`:
- Around line 138-147: The aside used as the project modal in Layout.tsx lacks
an explicit dialog role; update the aside element (the one with
aria-modal="true" and className using cn and open) to include role="dialog" so
screen readers recognize it as a modal dialog; ensure you add role="dialog"
alongside the existing aria-modal="true" and keep the rest of the attributes
(className, style, aria-label) unchanged.
In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx`:
- Around line 74-80: The selection can be cleared by the stale-selection effect
because invalidateQueries is not awaited; update the addCodebase flow so that
after addCodebase resolves you await queryClient.invalidateQueries({ queryKey:
['codebases'] }) before calling setSelectedProjectId(codebase.id), then proceed
to setShowAddInput(false), setAddValue(''), and setAddError(null); reference
addCodebase, queryClient.invalidateQueries, and setSelectedProjectId and keep
the stale-selection cleanup in ProjectContext.tsx in mind when implementing the
await.
---
Nitpick comments:
In `@packages/web/src/components/conversations/SlashCommandMenu.tsx`:
- Around line 33-39: Clamp the current selectedIndex whenever filtered changes
so it never exceeds filtered.length - 1: after the useMemo that computes
filtered, derive a safeSelectedIndex (e.g., Math.max(0, Math.min(selectedIndex,
filtered.length - 1))) and use that everywhere instead of selectedIndex for
rendering and actions. Replace uses in aria-activedescendant, aria-selected, and
the Enter/selection handler to read safeSelectedIndex so rapid typing cannot
leave a stale out-of-range selection that causes no-op Enter presses. Ensure
filtered and selectedIndex names (from useMemo and the selection state) are the
referenced symbols when implementing this clamp.
In `@packages/web/src/components/layout/TopNav.tsx`:
- Around line 118-163: The dropdown lacks keyboard navigation: add an onKeyDown
handler (e.g., handleMenuKeyDown) and attach it to the menu container div (the
same element rendered when menuOpen is true) to handle Escape (call
setMenuOpen(false)) and ArrowUp/ArrowDown to move focus between menu items; give
each NavLink a role="menuitem" and tabIndex={-1}, collect the rendered item
elements via refs or querySelectorAll from menuRef to implement roving focus (on
ArrowDown focus the next item, ArrowUp the previous, wrapping as needed), and
ensure the container has role="menu" so screen readers see it as a menu.
In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx`:
- Around line 103-118: The delete handler handleDeleteConfirm lacks a loading
flag; add a local state like isDeleting and set it to true at the start and
false in a finally block around the deleteCodebase(...) promise to prevent
double submits and properly clear errors via setDeleteError; also short-circuit
early if isDeleting or no deleteTargetId to avoid duplicate calls, update the
hook dependency array to include the isDeleting setter, and disable the
Remove/confirm button in the UI based on isDeleting so users cannot click while
queryClient.invalidateQueries, setSelectedProjectId, and setDeleteTargetId are
running.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63a5eaf5-f228-45e0-a5b9-03999a6556e4
📒 Files selected for processing (5)
packages/web/src/components/conversations/SlashCommandMenu.tsxpackages/web/src/components/layout/Layout.tsxpackages/web/src/components/layout/TopNav.tsxpackages/web/src/components/sidebar/ProjectsSidebar.tsxpackages/web/src/contexts/MobileNavContext.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/contexts/MobileNavContext.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/components/layout/TopNav.tsx (1)
25-36: Consider adding Escape key handling for the dropdown menu.The outside-click handler is well-implemented with proper cleanup. For improved keyboard accessibility, the dropdown could also close when the user presses Escape, aligning with the PR's stated behavior for the sidebar drawer.
♻️ Optional: Add Escape key handler
useEffect(() => { if (!menuOpen) return; const handler = (e: MouseEvent): void => { if (menuRef.current && !menuRef.current.contains(e.target as Node)) { setMenuOpen(false); } }; + const keyHandler = (e: KeyboardEvent): void => { + if (e.key === 'Escape') setMenuOpen(false); + }; document.addEventListener('mousedown', handler); + document.addEventListener('keydown', keyHandler); return (): void => { document.removeEventListener('mousedown', handler); + document.removeEventListener('keydown', keyHandler); }; }, [menuOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/TopNav.tsx` around lines 25 - 36, Add Escape key handling to the existing useEffect that closes the dropdown on outside clicks: inside the effect (where menuOpen, menuRef and setMenuOpen are used) register a 'keydown' listener when menuOpen is true that checks for e.key === 'Escape' (or e.key === 'Esc' for older browsers) and calls setMenuOpen(false); ensure you remove that keydown listener in the return cleanup along with the existing mousedown removal so listeners are not leaked. Use the existing symbols menuOpen, menuRef, setMenuOpen and keep the dependency array as [menuOpen].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/components/layout/TopNav.tsx`:
- Around line 25-36: Add Escape key handling to the existing useEffect that
closes the dropdown on outside clicks: inside the effect (where menuOpen,
menuRef and setMenuOpen are used) register a 'keydown' listener when menuOpen is
true that checks for e.key === 'Escape' (or e.key === 'Esc' for older browsers)
and calls setMenuOpen(false); ensure you remove that keydown listener in the
return cleanup along with the existing mousedown removal so listeners are not
leaked. Use the existing symbols menuOpen, menuRef, setMenuOpen and keep the
dependency array as [menuOpen].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f57afe1c-9d6e-4a53-93ec-d34196da408a
📒 Files selected for processing (2)
packages/web/src/components/layout/TopNav.tsxpackages/web/src/components/layout/TunnelPopover.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/components/layout/TunnelPopover.tsx
Rename the core AI provider interface and all related types, classes, factory functions, and directory from clients/ to providers/. Rename map: - IAssistantClient → IAgentProvider - ClaudeClient → ClaudeProvider - CodexClient → CodexProvider - getAssistantClient → getAgentProvider - AssistantRequestOptions → AgentRequestOptions - IWorkflowAssistantClient → IWorkflowAgentProvider - AssistantClientFactory → AgentProviderFactory - WorkflowAssistantOptions → WorkflowAgentOptions - packages/core/src/clients/ → packages/core/src/providers/ NOT renamed (user-facing/DB-stored): assistant config key, DEFAULT_AI_ASSISTANT env var, ai_assistant_type DB column. No behavioral changes — purely naming.
…nd docs - AssistantDefaults → ProviderDefaults, ClaudeAssistantDefaults → ClaudeProviderDefaults - Logger domains: client.claude → provider.claude, client.codex → provider.codex - Fix stale JSDoc, error messages, and references in architecture docs, CHANGELOG, testing rules
- ProviderDefaults → CodexProviderDefaults (symmetric with ClaudeProviderDefaults) - Fix stale "AI client" comments in orchestrator-agent.ts and orchestrator.test.ts - Remove dead createMockAgentProvider in test/mocks/streaming.ts (zero importers, wrong method names) - Fix irregular whitespace in .claude/rules/workflows.md
…oleam00#1137) * refactor: extract providers from @archon/core into @archon/providers Move Claude and Codex provider implementations, factory, and SDK dependencies into a new @archon/providers package. This establishes a clean boundary: providers own SDK translation, core owns business logic. Key changes: - New @archon/providers package with zero-dep contract layer (types.ts) - @archon/workflows imports from @archon/providers/types — no mirror types - dag-executor delegates option building to providers via nodeConfig - IAgentProvider gains getCapabilities() for provider-agnostic warnings - @archon/core no longer depends on SDK packages directly - UnknownProviderError standardizes error shape across all surfaces Zero user-facing changes — same providers, same config, same behavior. * refactor: remove config type duplication and backward-compat re-exports Address review findings: - Move ClaudeProviderDefaults and CodexProviderDefaults to the @archon/providers/types contract layer as the single source of truth. @archon/core/config/config-types.ts now imports from there. - Remove provider re-exports from @archon/core (index.ts and types/). Consumers should import from @archon/providers directly. - Update @archon/server to depend on @archon/providers for MessageChunk. * refactor: move structured output validation into providers Each provider now normalizes its own structured output semantics: - Claude already yields structuredOutput from the SDK's native field - Codex now parses inline agent_message text as JSON when outputFormat is set, populating structuredOutput on the result chunk This eliminates the last provider === 'codex' branch from dag-executor, making it fully provider-agnostic. The dag-executor checks structuredOutput uniformly regardless of provider. Also removes the ClaudeCodexProviderDefaults deprecated alias — all consumers now use ClaudeProviderDefaults directly. * fix: address PR review — restore warnings, fix loop options, cleanup Critical fixes: - Restore MCP missing env vars user-facing warning (was silently dropped) - Restore Haiku + MCP tool search warning - Fix buildLoopNodeOptions to pass workflow-level nodeConfig (effort, thinking, betas, sandbox were silently lost for loop nodes) - Add TODO(coleam00#1135) comments documenting env-leak gate gap Cleanup: - Remove backward-compat type aliases from deps.ts (keep WorkflowTokenUsage) - Remove 26 unnecessary eslint-disable comments from test files - Trim internal helpers from providers barrel (withFirstMessageTimeout, getProcessUid, loadMcpConfig, buildSDKHooksFromYAML) - Add @archon/providers dep to CLI package.json - Fix 8 stale documentation paths pointing to deleted core/src/providers/ - Add E2E smoke test workflows for both Claude and Codex providers * fix: forward provider system warnings to users in dag-executor The dag-executor only forwarded system chunks starting with "MCP server connection failed:" — all other provider warnings (missing env vars, Haiku+MCP, structured output issues) were logged but never reached the user. Now forwards all system chunks starting with⚠️ (the prefix providers use for user-actionable warnings). * fix: add providers package to Dockerfile and fix CI module resolution - Add packages/providers/ to all three Dockerfile stages (deps, production package.json copy, production source copy) - Replace wildcard export map (./*) with explicit subpath entries to fix module resolution in CI (bun workspace linking) * chore: update bun.lock for providers package exports
…1160) * fix: make env-integration test cross-platform (Windows CI) Check for Windows env var equivalents (Path instead of PATH, USERPROFILE instead of HOME) in scenario 3 assertions. Closes coleam00#1128 * fix: Windows PATH/HOME casing in provider subprocess env test Same cross-platform fix for ClaudeProvider test — spread objects lose Windows case-insensitive behavior (Path vs PATH, USERPROFILE vs HOME).
…ies (coleam00#1162) * refactor: decompose provider sendQuery() into explicit helper boundaries (coleam00#1139) sendQuery() in both Claude and Codex providers was a monolith mixing SDK option building, nodeConfig translation, stream normalization, and error classification. This makes it hard to safely extend for Phase 2 provider extensibility. Decompose both providers into focused internal helpers: Claude: - buildBaseClaudeOptions: SDK option construction - buildToolCaptureHooks: PostToolUse/PostToolUseFailure hook setup - applyNodeConfig: workflow nodeConfig → SDK translation + structured warnings - streamClaudeMessages: raw SDK event → MessageChunk normalization - classifyAndEnrichError: error classification with retry decisions Codex: - buildTurnOptions: per-turn option construction (output schema, abort) - streamCodexEvents: raw SDK event → MessageChunk normalization - classifyAndEnrichCodexError: error classification with retry decisions Also introduces ProviderWarning { code, message } replacing raw string warnings for machine-readable provider translation warnings. Adds 43 focused unit tests covering the extracted helpers directly. Fixes coleam00#1139 * fix: export ToolResultEntry type used in public buildBaseClaudeOptions API * fix: unexport internal helpers to prevent API surface leakage, fix retry state bug Review findings: 1. Internal helpers were exported and reachable through package.json subpath exports (./claude/provider, ./codex/provider), widening the public API. All new helpers are now file-local — the only public exports remain ClaudeProvider, CodexProvider, loadMcpConfig, buildSDKHooksFromYAML, withFirstMessageTimeout, getProcessUid. 2. Codex streamState (lastTodoListSignature) was shared across retry attempts, causing todo-list dedup to suppress output on retry. Now creates fresh state per attempt. Removed direct helper test imports — existing sendQuery e2e tests (51 Claude + 42 Codex) cover all behavior paths. * fix: address review findings — abort handling, retry bugs, error swallowing Fixes from CodeRabbit + multi-agent review: 1. classifyAndEnrichError preserves first-event timeout diagnostic instead of collapsing it into generic "Query aborted" (the timeout aborts the controller, but the original error carries the coleam00#1067 breadcrumb) 2. nodeConfigWarnings emitted once before retry loop, not per attempt 3. buildSubprocessEnv() called once before retry loop (was re-logging auth mode and rebuilding { ...process.env } per attempt) 4. Abort signal listener registered once with forwarding to current controller (was accumulating per-retry listeners) 5. PostToolUse hook wrapped in try/catch (JSON.stringify can throw on circular refs — was asymmetric with PostToolUseFailure which had it) 6. Codex streamCodexEvents throws on abort instead of silent break (callers were getting truncated stream with no result/error) 7. Both providers store enrichedError (not raw error) for retry exhaustion — preserves stderr context in final throw 8. Log is_error result events at error level in Claude stream normalizer * test: add black-box behavioral tests for sendQuery decomposition fixes Restore test coverage for the specific fixes from the decomposition review, exercised through sendQuery (black-box) since helpers are file-local: Claude (6 tests): - Timeout error preserved (not collapsed into "Query aborted") - nodeConfig warnings emitted once even when retries occur - Abort signal cancels across retries via single forwarding listener - Enriched error (with stderr) thrown at retry exhaustion - PostToolUse hook handles circular reference without crashing - is_error result events logged at error level Codex (3 tests): - Abort signal throws instead of silently truncating stream - Enriched error thrown at retry exhaustion - Todo-list dedup state resets between retry attempts
- e2e-all-nodes: exercises bash, prompt, script (bun), structured output, model override (haiku), effort control, and $nodeId.output refs - e2e-mixed-providers: tests Claude + Codex in the same workflow with cross-provider output references - echo-args.js: simple script node test helper
…leam00#1135) Remove the entire env-leak scanning/consent infrastructure: scanner, allow_env_keys DB column usage, allow_target_repo_keys config, PATCH consent route, --allow-env-keys CLI flag, and UI consent toggle. The env-leak gate was the wrong primitive. Target repo .env protection is already structural: - stripCwdEnv() at boot removes Bun-auto-loaded CWD .env keys - Archon loads its own env sources afterward (~/.archon/.env) - process.env is clean before any subprocess spawns - Managed env injection (config.yaml env: + DB vars) is unchanged No scanning, no consent, no blocking. Any repo can be registered and used. Subprocesses receive the already-clean process.env.
Replace window.location.hostname direct-connect with localhost-only check. When accessed via Cloudflare tunnel, use relative URL so requests go through Vite proxy which forwards to the backend (port 3090 not directly accessible).
Adds a slide-in conversations panel accessible from mobile via a 'Conversations' button in the chat topbar. Also fixes horizontal scroll parasite on mobile by adding overflow-x: hidden to html/body/#root.
Change drawer from 'fixed inset-y-0 z-50' to 'fixed top-12 bottom-0 z-40' so it opens below the 48px topbar instead of overlapping it.
Prevents the mobile hamburger drawer and its backdrop from overlapping the sticky topbar (z-50). Pattern matches ChatPage.tsx fix.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- MobileNavContext: add pinned + togglePin to context value - Layout: full pin/unpin sidebar behaviour — static column on desktop when pinned, slide-in drawer on mobile or when unpinned; pin state persisted to localStorage; escape key closes drawer; media-query listener collapses to mobile mode on resize - TopNav: remove hamburger (sidebar toggle now handled by logo/drawer); when sidebar is closed/unpinned the logo "A" acts as the reopen button with a ChevronRight indicator; Settings/Dashboard/Workflows moved to a hamburger dropdown menu at the right - Prettier formatting normalisation on changed files Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- TunnelPopover: add TunnelMenuItem export — inline collapsible tunnel controls for use inside dropdown menus (mobile). Expands in-place with ChevronDown/Up toggle instead of a floating popover. - TopNav: wrap TunnelPopover in hidden md:block so it's desktop-only; add TunnelMenuItem inside the hamburger dropdown wrapped in md:hidden so mobile gets inline tunnel controls without a separate popover. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tboome33
force-pushed
the
feat/responsive-mobile-ui
branch
from
255aad1 to
6a09a23
Compare
- C1: restrict tunnel start/stop endpoints to localhost-only (403 otherwise) - M1: extract tunnel Zod schemas to system.schemas.ts - M2: check response.ok in TunnelPopover handleStart/handleStop - M3: validate /api/tunnel poll response with isTunnelState() type guard - M4: await queryClient.invalidateQueries before setSelectedProjectId in ProjectsSidebar - M5: make MobileNavContext nullable; throw in useMobileNav if used outside Provider - M6: guard useVisualViewport for SSR; add window.resize fallback when visualViewport unavailable - m1: remove useVisualViewport.ts from eslint ignore list (new file, not pre-existing) - m2: add 30s timeout in tunnel.ts; kill process and set error if URL not detected - m3: clarify slash detection word-boundary comment in MessageInput - m4: correct PICKER_WIDTH to 256 (w-64) in WorkflowCanvas - m5: add aria-label to role=switch button in SettingsPage - m6: add role=dialog to overlay drawer aside in Layout - m7: remove aria-activedescendant from listbox; add id=slash-menu to SlashCommandMenu - m8: add language tags to unlabeled code fences in RESPONSIVE_UI_PATCH.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
All 16 CodeRabbit review issues addressed in commit 711b8ee. Summary: security (localhost-only guard on tunnel endpoints), bug fixes (response.ok checks, race condition on project creation, visualViewport fallback), accessibility (role=dialog, aria-label on switches, aria-activedescendant on correct element), and code quality (Zod schemas extracted, type guards added, 30s tunnel timeout, slash detection URL fix, PICKER_WIDTH corrected). |
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/orchestrator/orchestrator-isolation.test.ts (1)
174-183:⚠️ Potential issue | 🔴 CriticalFinish updating the
Codebasetest fixture.
Codebasestill requiresrepository_urlandai_assistant_type, but this fixture now omits both. In strict TS that makes the test file itself invalid after theallow_env_keysremoval. Add explicit defaults here.🧩 Suggested fix
function makeCodebase(overrides?: Partial<Codebase>): Codebase { return { id: 'cb-1', name: 'test-repo', + repository_url: null, default_cwd: '/workspace/test-repo', + ai_assistant_type: 'claude', commands: {}, created_at: new Date(), updated_at: new Date(), ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-isolation.test.ts` around lines 174 - 183, The test fixture function makeCodebase is missing required Codebase fields repository_url and ai_assistant_type; update the return object inside makeCodebase to include sensible defaults for repository_url (e.g., an empty string or 'https://example.com/repo') and ai_assistant_type (e.g., a default enum/string value used elsewhere) so the fixture satisfies the Codebase type—add those keys alongside id, name, default_cwd, commands, created_at, and updated_at and spread overrides afterward to allow tests to override them.packages/providers/src/claude/provider.test.ts (1)
462-490:⚠️ Potential issue | 🟡 MinorRestore the env mutation in a
finallyblock.If one of these assertions fails before the cleanup branch runs,
CUSTOM_USER_KEYleaks into later tests and makes the suite order-dependent.♻️ Suggested fix
test('subprocess env passes through all process.env keys (no allowlist filtering)', async () => { const originalKey = process.env.CUSTOM_USER_KEY; - process.env.CUSTOM_USER_KEY = 'user-trusted-value'; - - mockQuery.mockImplementation(async function* () { - // Empty generator - }); - - for await (const _ of client.sendQuery('test', '/workspace')) { - // consume - } - - const callArgs = mockQuery.mock.calls[0][0] as { - options: { env: NodeJS.ProcessEnv; executableArgs?: string[] }; - }; - // --no-env-file prevents Bun from auto-loading .env in subprocess CWD - expect(callArgs.options.executableArgs).toEqual(['--no-env-file']); - expect(callArgs.options.env.CUSTOM_USER_KEY).toBe('user-trusted-value'); - // Windows uses "Path" casing in spread objects and USERPROFILE instead of HOME - const envPath = callArgs.options.env.PATH ?? callArgs.options.env.Path; - const processPath = process.env.PATH ?? process.env.Path; - expect(envPath).toBe(processPath); - const envHome = callArgs.options.env.HOME ?? callArgs.options.env.USERPROFILE; - const processHome = process.env.HOME ?? process.env.USERPROFILE; - expect(envHome).toBe(processHome); - - // Cleanup - if (originalKey !== undefined) process.env.CUSTOM_USER_KEY = originalKey; - else delete process.env.CUSTOM_USER_KEY; + process.env.CUSTOM_USER_KEY = 'user-trusted-value'; + try { + mockQuery.mockImplementation(async function* () { + // Empty generator + }); + + for await (const _ of client.sendQuery('test', '/workspace')) { + // consume + } + + const callArgs = mockQuery.mock.calls[0][0] as { + options: { env: NodeJS.ProcessEnv; executableArgs?: string[] }; + }; + // --no-env-file prevents Bun from auto-loading .env in subprocess CWD + expect(callArgs.options.executableArgs).toEqual(['--no-env-file']); + expect(callArgs.options.env.CUSTOM_USER_KEY).toBe('user-trusted-value'); + // Windows uses "Path" casing in spread objects and USERPROFILE instead of HOME + const envPath = callArgs.options.env.PATH ?? callArgs.options.env.Path; + const processPath = process.env.PATH ?? process.env.Path; + expect(envPath).toBe(processPath); + const envHome = callArgs.options.env.HOME ?? callArgs.options.env.USERPROFILE; + const processHome = process.env.HOME ?? process.env.USERPROFILE; + expect(envHome).toBe(processHome); + } finally { + if (originalKey !== undefined) process.env.CUSTOM_USER_KEY = originalKey; + else delete process.env.CUSTOM_USER_KEY; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/claude/provider.test.ts` around lines 462 - 490, The test "subprocess env passes through all process.env keys (no allowlist filtering)" mutates process.env.CUSTOM_USER_KEY but only restores it conditionally at the end, so move the mutation cleanup into a try/finally: capture originalKey before setting process.env.CUSTOM_USER_KEY, wrap the mockQuery setup, the for-await consumption of client.sendQuery('test', '/workspace') and all assertions in a try block, and perform the restore of process.env.CUSTOM_USER_KEY in the finally block to guarantee cleanup even if assertions fail; reference the test name, the originalKey variable, process.env.CUSTOM_USER_KEY, mockQuery and client.sendQuery when locating where to add the try/finally.
♻️ Duplicate comments (6)
packages/web/src/components/sidebar/ProjectsSidebar.tsx (1)
74-77:⚠️ Potential issue | 🟠 MajorSelect the new project only after the cache knows about it.
setSelectedProjectId(codebase.id)still runs before['codebases']contains the new row. If the stale-selection cleanup inProjectContextruns first, the auto-selection is cleared and the freshly created project never opens. Seed the query cache with the returned codebase or await the refetch before selecting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx` around lines 74 - 77, After creating the new codebase via addCodebase, do not call setSelectedProjectId(codebase.id) until the codebases query cache is updated; either seed the cache with the returned codebase (use queryClient.setQueryData(['codebases'], prev => [...prev, codebase]) or the equivalent) or await the refetch/invalidate (await queryClient.invalidateQueries({ queryKey: ['codebases'] })) before calling setSelectedProjectId so ProjectContext’s stale-selection cleanup won't clear the new selection. Ensure you reference addCodebase, queryClient.invalidateQueries / queryClient.setQueryData, setSelectedProjectId and the ['codebases'] query key in your change.packages/web/src/routes/SettingsPage.tsx (1)
60-73:⚠️ Potential issue | 🟡 MinorGive the compact-layout switch an accessible name.
This
role="switch"control is still unnamed, so screen readers will announce an unlabeled switch. Addaria-labelor connect it to the nearby text witharia-labelledby.Minimal fix
- <button + <button + aria-label="Always use responsive layout" role="switch" aria-checked={compactLayout} onClick={toggleCompactLayout}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/SettingsPage.tsx` around lines 60 - 73, The switch button with role="switch" (using compactLayout and toggleCompactLayout) lacks an accessible name; update the button element to include an accessible name by adding either an aria-label (e.g., aria-label="Compact layout") or an aria-labelledby that points to the nearby label text node, ensuring screen readers announce the switch state; keep the existing aria-checked and onClick behavior unchanged and choose the approach consistent with other controls on the page.packages/server/src/routes/api.ts (2)
834-888: 🛠️ Refactor suggestion | 🟠 MajorMove the tunnel response schemas into
system.schemas.ts.These inline
z.object(...)contracts reintroduce schema drift inapi.ts. Please export named tunnel schemas from the system schema module and import them here like the other system routes.As per coding guidelines, "
packages/server/src/routes/**/*.{ts,tsx}: UseregisterOpenApiRoute(createRoute({...}), handler)for all new/modified API routes; schemas live inpackages/server/src/routes/schemas/(one file per domain)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.ts` around lines 834 - 888, The inline z.object tunnel response schemas in getTunnelRoute, postTunnelStartRoute, and deleteTunnelStopRoute should be moved to the system schemas module: create/export named schemas (e.g., TunnelStatusResponse, TunnelStartResponse, TunnelStopResponse) in packages/server/src/routes/schemas/system.schemas.ts and replace the inline z.object(...) usages by importing those named schemas into api.ts; also ensure these routes continue to use registerOpenApiRoute(createRoute({...}), handler) per routing guidelines if applicable.
2583-2598:⚠️ Potential issue | 🔴 CriticalDo not leave tunnel controls callable from arbitrary origins.
These new
/api/tunnel*handlers still sit behind the file-widecors({ origin: '*' })policy and do not enforce a local-only check or one-time secret. That lets any webpage start a tunnel, poll the state endpoint, and expose the user's local Archon instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.ts` around lines 2583 - 2598, The tunnel routes (getTunnelRoute, postTunnelStartRoute, deleteTunnelStopRoute) are currently callable under the global cors({ origin: '*' }) policy; enforce a local-only or authenticated guard before calling startTunnel/stopTunnel/getTunnelState by validating either (a) the request origin/IP is loopback (e.g., 127.0.0.1/::1) or (b) a one-time secret/admin token header; add this check in the registerOpenApiRoute handler wrapper (or as middleware) so POST to startTunnel(5173) and DELETE stopTunnel() are rejected unless the guard passes, and return a 403 for unauthorized attempts rather than allowing the global CORS policy to expose these endpoints.packages/web/src/components/layout/TunnelPopover.tsx (2)
50-57:⚠️ Potential issue | 🟠 MajorOnly flip the tunnel state after the request succeeds.
These handlers move local state to
starting/inactivewithout checkingresponse.ok. A failed start leaves the UI spinning, and a failed stop can hide a still-live tunnel until the next poll corrects it.Also applies to: 217-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/TunnelPopover.tsx` around lines 50 - 57, The handlers handleStart and handleStop currently set local tunnel state optimistically before checking the HTTP response; change them to await the fetch, check response.ok, and only call setTunnel to update status/url when the request succeeds (for handleStart set status to 'starting' and set returned url only on success; for handleStop set status to 'inactive' only on success). On non-ok responses throw or handle the error and revert or preserve the previous state (e.g., setTunnel back to prior status or show an error). Apply the same fix to the duplicate handlers referenced around lines 217-224.
4-9:⚠️ Potential issue | 🟠 MajorDon't trust
/api/tunnelas a casted local type.
TunnelStateis a hand-written copy of the server payload, and both pollers castres.json()straight into it before rendering. If the route shape drifts, this keeps compiling and fails at runtime. Please consume a shared/generated type or validate the payload before callingsetTunnel(...).Also applies to: 29-30, 38-41, 199-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/layout/TunnelPopover.tsx` around lines 4 - 9, The local TunnelState/TunnelStatus types are unsafe copies of the server payload; import the shared/generated server type (or rename to match it) and stop casting res.json() directly—create a runtime validator (e.g., isTunnelState(obj)) that checks obj.status is one of the allowed values and obj.url is string|null, use that validator in both pollers before calling setTunnel(...) and on failure log and set a safe fallback (e.g., status: 'error', url: null); update any references to TunnelState/TunnelStatus and the poller res.json() usages (the functions that call setTunnel at lines referenced) to use the imported type + runtime guard.
🧹 Nitpick comments (6)
packages/web/src/components/conversations/SlashCommandMenu.tsx (1)
41-44: Consider clampingselectedIndexwhen filtered list shrinks.The current reset happens when
querychanges, but if the filter narrows the list while keeping the same query prefix,selectedIndexcould briefly exceedfiltered.length - 1until the next render. Theif (entry)check prevents a crash, but the highlighted option would disappear momentarily.♻️ Suggested improvement
// Reset selected index when query changes useEffect(() => { setSelectedIndex(0); }, [query]); + + // Clamp selected index when filtered list shrinks + useEffect(() => { + if (filtered.length > 0 && selectedIndex >= filtered.length) { + setSelectedIndex(filtered.length - 1); + } + }, [filtered.length, selectedIndex]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/conversations/SlashCommandMenu.tsx` around lines 41 - 44, The useEffect that resets selectedIndex on query changes can leave selectedIndex out of range when the filtered list shrinks; update the effect (or add a new useEffect dependent on filtered.length) to clamp selectedIndex into [0, filtered.length - 1] instead of always setting to 0 — e.g., compute newIndex = Math.max(0, Math.min(selectedIndex, filtered.length - 1)) and call setSelectedIndex(newIndex); reference the selectedIndex state setter setSelectedIndex, the selectedIndex state, and the filtered array to locate where to apply this clamp.packages/workflows/src/deps.ts (1)
70-92: Use the canonical provider default types here instead of another mirror.
WorkflowConfig.assistantsis still a hand-maintained copy of the provider config surface, and it has already drifted by omittingcodexBinaryPath. Importing the provider default types here would keep this contract aligned automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/deps.ts` around lines 70 - 92, The assistants sub-schema in WorkflowConfig is a hand-maintained duplicate that has drifted (missing codexBinaryPath); replace this manual shape by importing and using the canonical provider types instead: remove the inline assistants definition and reference the provider-exported types (e.g., the Codex/Claude provider config type) so WorkflowConfig.assistants uses the exact provider types; ensure existing symbols referenced here (WorkflowConfig, assistants, ModelReasoningEffort, WebSearchMode, codexBinaryPath) are aligned by updating imports to pull the provider defaults and adjust any usages to the imported type names.packages/providers/src/claude/provider.ts (4)
325-330: Spurious warning when input hooks are empty.This warning fires even when
nodeHookswas empty or contained onlyundefinedvalues, which is a valid (not warning-worthy) scenario. The warning suggests something went wrong during building, but an empty input naturally produces an empty output.♻️ Only warn when non-empty input produces empty output
+ const hasDefinedMatchers = Object.values(nodeHooks).some(m => m !== undefined && m.length > 0); + - if (Object.keys(sdkHooks).length === 0) { + if (Object.keys(sdkHooks).length === 0 && hasDefinedMatchers) { getLog().warn( { nodeHooksKeys: Object.keys(nodeHooks) }, 'claude.hooks_build_produced_empty_map' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/claude/provider.ts` around lines 325 - 330, The warning triggers even for a valid empty input; change the check so we only call getLog().warn when the built sdkHooks is empty AND the original nodeHooks actually contained at least one meaningful entry. Concretely, before warning, compute whether nodeHooks has any non-undefined entries (e.g. filter Object.keys(nodeHooks) for values !== undefined or check Object.keys(nodeHooks).length > 0 and at least one value is defined) and only call getLog().warn({ nodeHooksKeys: Object.keys(nodeHooks) }, 'claude.hooks_build_produced_empty_map') when sdkHooks is empty AND that filtered nodeHooks set is non-empty.
543-564: Stderr error detection heuristic could have false positives.The check
output.includes('at ')at line 553 is intended to catch stack traces but could match normal output (e.g., "looking at data"). The combined checks andisInfoMessagefilter mitigate this, but consider using a more specific pattern.♻️ Optional: More specific stack trace detection
- output.includes('at ') || + /^\s+at\s/.test(output) || // Stack trace lines start with whitespace + 'at'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/claude/provider.ts` around lines 543 - 564, The stderr handler's loose check output.includes('at ') can produce false positives; update the isError detection inside the stderr arrow function to use a more specific stack-trace pattern (e.g., a regex that matches typical V8 stack frames like lines starting with optional whitespace followed by "at " and a symbol/file path) instead of the plain includes('at '), keeping the other checks (Error:, exception, fatal, failed) and the isInfoMessage exclusions intact; ensure you reference and modify the isError computation within the same stderr: (data: string): void => { ... } block so getLog().error({ stderr: output }, 'subprocess_error') only fires for real errors.
210-216: Uppercase-only env var expansion may miss lowercase variables.The regex
/\$([A-Z_][A-Z0-9_]*)/gonly matches uppercase environment variable names. If users have lowercase env vars (e.g.,$api_key), they won't be expanded and won't be added tomissingVars.If this is intentional (enforcing uppercase convention), consider documenting it. Otherwise:
♻️ Optional: Support case-insensitive env var names
- result[key] = val.replace(/\$([A-Z_][A-Z0-9_]*)/g, (_, varName: string) => { + result[key] = val.replace(/\$([A-Za-z_][A-Za-z0-9_]*)/g, (_, varName: string) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/claude/provider.ts` around lines 210 - 216, The current env var expansion in the result[key] assignment uses a regex that only matches uppercase names (/\$([A-Z_][A-Z0-9_]*)/g), so lowercase env vars like $api_key are skipped and not added to missingVars; update the regex in that replacement (the code around result[key] = val.replace(...), the capture name varName, and the missingVars push) to accept lowercase letters too (e.g., use /\$([A-Za-z_][A-Za-z0-9_]*)/g or use a case-insensitive approach) so process.env[varName] is looked up for mixed-case names and missingVars is populated correctly, or explicitly document/enforce the uppercase-only convention if that was intended.
638-641: Missing type parameter on AsyncGenerator.The
eventsparameter lacks a type argument, defaulting toAsyncGenerator<unknown, any, undefined>. For clarity and type safety per coding guidelines, consider adding the type parameter.♻️ Add explicit type parameter
async function* streamClaudeMessages( - events: AsyncGenerator, + events: AsyncGenerator<unknown>, toolResultQueue: ToolResultEntry[] ): AsyncGenerator<MessageChunk> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/claude/provider.ts` around lines 638 - 641, The events parameter in streamClaudeMessages is an untyped AsyncGenerator (defaults to unknown); update the parameter to use an explicit AsyncGenerator type that matches the generator's yielded values (the same type the function yields/handles) — e.g., change the events parameter to AsyncGenerator<MessageChunk, void, unknown> (or the appropriate resolved/return type used in your codebase) so the signature reads streamClaudeMessages(events: AsyncGenerator<MessageChunk, void, unknown>, toolResultQueue: ToolResultEntry[]): AsyncGenerator<MessageChunk>. Ensure you reference the MessageChunk type and adjust the middle/return generic positions if your generator returns a different value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.archon/workflows/e2e-all-nodes.yaml:
- Around line 36-39: The bash node "bash-read-output" is embedding
$structured.output inside single quotes which will produce nested-quote JSON and
break the shell command; update the bash command to reference the specific field
(e.g., $structured.output.category) and remove the surrounding single quotes so
the executor's shell-escaping can work correctly (i.e., use an unquoted
$structured.output.category or a properly quoted form handled by the executor
rather than wrapping the whole JSON in '...').
In @.archon/workflows/e2e-mixed-providers.yaml:
- Around line 19-27: The smoke test doesn't validate the cross-provider
reference because claude-reads-codex returns a constant string and verify only
echoes outputs; update the claude-reads-codex node prompt to require it to
return the actual codex output (e.g., "Return exactly: '$codex-node.output' and
nothing else") and change the verify node's bash command to assert equality
between claude-reads-codex.output and codex-node.output (exit non-zero on
mismatch and print a clear error), ensuring you edit the prompt field of
claude-reads-codex and the bash field of the verify node to perform the actual
equality check.
In `@packages/core/src/handlers/clone.ts`:
- Around line 40-44: The code currently auto-registers repositories in
registerRepoAtPath without a secret-scan/consent gate; add a guard that runs a
secrets scan (and requires explicit user consent if secrets are found or if
policy mandates) before persisting the codebase or loading repository commands.
Implement this by calling an existing secret-scan/consent utility (or add
secretScanAndRequireConsent(repoPath) invoked at the top of registerRepoAtPath)
and return/throw to abort registration when the scan fails or consent is denied;
also add the same pre-persist/pre-load gate at the other clone/register entry
points noted in the review (the other call sites around the lines referenced) so
no code path can persist or load commands without this check. Ensure the gate
logs the decision and fails closed (deny registration) on errors.
In `@packages/core/src/services/title-generator.ts`:
- Around line 50-56: The code unconditionally passes nodeConfig: {
allowed_tools: [] } to client.sendQuery which is Claude-specific and can be
ignored or mishandled for other providers; update generateAndSetTitle() to
inspect the provider type via the client returned by
getAgentProvider(assistantType) (use client.getType()) and only include
nodeConfig with allowed_tools when client.getType() === 'claude' (for other
provider types, omit nodeConfig or pass no allowed_tools) so tool access is
properly controlled for Claude without affecting Codex or other providers.
In `@packages/providers/src/codex/config.ts`:
- Around line 10-45: The parseCodexConfig function currently drops invalid
values silently; update it to validate presence of known fields and throw a
clear Error when a supported field is present but has an invalid value or type
(e.g., modelReasoningEffort not in ['minimal','low','medium','high','xhigh'],
webSearchMode not in ['disabled','cached','live'], non-string model or
codexBinaryPath, or additionalDirectories present but not an array of strings).
Locate parseCodexConfig and replace the silent-filtering logic for model,
modelReasoningEffort, webSearchMode, additionalDirectories, and codexBinaryPath
to perform explicit type/enum checks and throw descriptive errors (including the
field name and invalid value) instead of silently omitting them; still allow
omission when a field is truly absent. Ensure thrown errors are instances of
Error with messages easy to surface to users.
In `@packages/providers/src/codex/provider.ts`:
- Around line 42-57: getCodex currently returns a single global Codex instance
(codexInstance/codexInitPromise) so different requests with different
codexBinaryPath end up reusing the wrong executable; change the caching to be
keyed by the resolved binary path: call
resolveCodexBinaryPath(configCodexBinaryPath) first, use that string as the
cache key in a Map (e.g., codexInstances: Map<string, Codex> and
codexInitPromises: Map<string, Promise<Codex>>), create/store the Codex under
that key (new Codex({ codexPathOverride })), and on catch remove the failed
entry from codexInitPromises to allow retries; update getCodex to look up and
return the promise/instance from the maps instead of the single
codexInstance/codexInitPromise.
- Around line 213-218: The handler for event.type === 'turn.failed' should
surface an exception so callers like sendQuery() and the DAG executor see a
failure instead of treating the stream as a successful completion; replace the
current behavior that logs and yields a single system chunk then break with
logic that logs the error (using getLog().error) and then throw an Error (or
rethrow the underlying errorObj) after yielding any final system message if
needed so sendQuery() receives an exception; update the block referencing
event.type === 'turn.failed' / errorObj / errorMessage to throw the error rather
than using break so the DAG executor treats the node as failed.
In `@packages/server/src/routes/api.ts`:
- Around line 2589-2591: The route currently calls startTunnel(5173) which
hard-codes the Vite port; instead determine the tunnel target at runtime by
reading the server/runtime configuration or the active listener port and pass
that value into startTunnel. Update the handler registered by
registerOpenApiRoute(postTunnelStartRoute, ...) to query the runtime config
(e.g., the app/server config object or environment entry) or inspect the active
HTTP listener for its bound port, fall back to a sensible default only if
missing, and then call startTunnel(resolvedPort); keep the rest of the response
using getTunnelState().status. Ensure you update only the route handler (the
closure passed to registerOpenApiRoute) and reference startTunnel,
postTunnelStartRoute and getTunnelState when making the change.
In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx`:
- Around line 287-300: Remove the stale "Allow env keys" UI row from
ProjectsSidebar.tsx: delete the JSX block that reads project.allow_env_keys (the
span with text "Allow env keys" and the status badge using
project.allow_env_keys) and any imports/props that exclusively exist to support
it; ensure no other references to project.allow_env_keys remain in the
ProjectsSidebar component so TypeScript stops expecting that property and the
setting no longer renders.
- Around line 262-267: The ProjectDetail component is not receiving the
onNavigate handler from ProjectsSidebar, so in-component navigation (project
chats/sources) won't trigger the mobile auto-close path (currently used by
handleUnscopedChat). Pass the onNavigate prop into ProjectDetail where it is
rendered (ProjectDetail codebaseId={project.id} ... searchQuery={searchQuery} →
add onNavigate={onNavigate}), and update ProjectDetail's props and its internal
navigation handlers to call props.onNavigate() whenever it performs a route
change so the drawer auto-closes on mobile.
In `@packages/web/src/components/workflows/WorkflowBuilder.tsx`:
- Around line 575-587: The backdrop div used to close the mobile library
(role="button", onClick/onKeyDown) is not keyboard-accessible because
tabIndex={-1} prevents focus and its onKeyDown won't fire; update the behavior
so keyboard users can dismiss the overlay: either give the backdrop a focusable
tabIndex (e.g., tabIndex={0}) and ensure its onKeyDown handles
Escape/Enter/Space to call setShowLibrary(false), or add a window-level keydown
listener (registered when the library is shown and removed when hidden) that
listens for Escape and calls setShowLibrary(false); ensure you update the
component that contains setShowLibrary and the backdrop event handlers
(onKeyDown/onClick) so the chosen approach consistently enables keyboard
dismissal and cleanly removes any global listeners when the overlay closes.
In `@packages/web/src/contexts/ThemeContext.tsx`:
- Around line 21-29: ThemeProvider reads browser APIs during render
(localStorage and window.matchMedia) which breaks SSR/tests; update the lazy
initializers for theme and compactLayout (the useState callbacks in
ThemeProvider) to first check typeof window !== 'undefined' and only then access
localStorage or window.matchMedia, otherwise return safe defaults ('light' for
theme and false for compactLayout); optionally wrap localStorage access in
try/catch to handle quota/security errors and prefer the existing keys
('archon-theme' and 'archon-compact-layout') when present.
In `@packages/web/src/lib/api.ts`:
- Around line 21-24: SSE_BASE_URL currently reads window.location.hostname
during module evaluation which can throw in SSR/tests; replace the top-level
constant with a guarded helper like getSSEBaseUrl() that computes the base URL
lazily, checking typeof window !== 'undefined' before accessing
window.location.hostname, falling back to 'localhost' (or returning '' as
appropriate) when window is undefined, and use getSSEBaseUrl() wherever
SSE_BASE_URL was referenced.
In `@packages/web/src/routes/ChatPage.tsx`:
- Around line 89-110: The mobile drawer controlled by mobileConvOpen lacks
Escape-key dismissal; add a keydown listener in the ChatPage component (e.g., in
a useEffect) that watches mobileConvOpen and calls setMobileConvOpen(false) when
event.key === 'Escape', and ensure you remove the listener on cleanup to avoid
leaks; alternatively, replace the manual drawer div with an accessible
dialog/drawer primitive that provides built-in Escape handling and focus
management. Use the mobileConvOpen state, setMobileConvOpen setter, and the
drawer div/component as the integration points so the Escape handler only runs
while the drawer is open.
- Around line 103-110: The sidebar drawer uses the persisted width value (width)
directly, which can exceed small viewports and push controls off-screen; update
the style object on the element that builds the sidebar (the one using className
with mobileConvOpen and style={{ width: `${String(width)}px`, ... }}) to enforce
a mobile cap — e.g., add maxWidth: '100vw' or compute a clampedWidth
(Math.min(width, window.innerWidth) or a CSS clamp) and use that for width when
mobileConvOpen/viewport is narrow so the drawer never extends beyond the
viewport.
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 5266-5312: This test can be flaky if the parent process already
has LEAKED_REPO_SECRET set; before writing the .env or invoking
executeDagWorkflow (in the "bun script node does not leak repo .env from
execution cwd (`#1135`)" test), explicitly clear the environment key
(process.env.LEAKED_REPO_SECRET) so the only source of that variable would be
the repo .env file; do this by deleting or setting it to undefined early in the
test (before writeFile/executeDagWorkflow) so the final assertion that the node
output equals "CLEAN" is deterministic.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 1409-1431: buildLoopNodeOptions currently only injects
workflow-level options so per-loop node provider settings (mcp, hooks, skills,
allowed_tools/denied_tools, output_format, maxBudgetUsd, systemPrompt, effort,
thinking, sandbox, betas, fallbackModel, etc.) are lost; update
buildLoopNodeOptions to merge the loop node's provider-specific options into the
returned SendQueryOptions the same way resolveNodeProviderAndModel does: read
the loop node's config (e.g., node.loop or node.provider config) and copy
top-level fields (mcp, hooks, skills, allowed_tools, denied_tools,
output_format, maxBudgetUsd, systemPrompt) onto options and include per-node
advanced options inside options.nodeConfig (effort, thinking, sandbox, betas,
fallbackModel), preserving assistantConfig assignment, so loop nodes receive the
exact same provider options as regular nodes; apply the same merge logic at the
other affected spot referenced in the review (the similar block near the later
build/resolve usage).
---
Outside diff comments:
In `@packages/core/src/orchestrator/orchestrator-isolation.test.ts`:
- Around line 174-183: The test fixture function makeCodebase is missing
required Codebase fields repository_url and ai_assistant_type; update the return
object inside makeCodebase to include sensible defaults for repository_url
(e.g., an empty string or 'https://example.com/repo') and ai_assistant_type
(e.g., a default enum/string value used elsewhere) so the fixture satisfies the
Codebase type—add those keys alongside id, name, default_cwd, commands,
created_at, and updated_at and spread overrides afterward to allow tests to
override them.
In `@packages/providers/src/claude/provider.test.ts`:
- Around line 462-490: The test "subprocess env passes through all process.env
keys (no allowlist filtering)" mutates process.env.CUSTOM_USER_KEY but only
restores it conditionally at the end, so move the mutation cleanup into a
try/finally: capture originalKey before setting process.env.CUSTOM_USER_KEY,
wrap the mockQuery setup, the for-await consumption of client.sendQuery('test',
'/workspace') and all assertions in a try block, and perform the restore of
process.env.CUSTOM_USER_KEY in the finally block to guarantee cleanup even if
assertions fail; reference the test name, the originalKey variable,
process.env.CUSTOM_USER_KEY, mockQuery and client.sendQuery when locating where
to add the try/finally.
---
Duplicate comments:
In `@packages/server/src/routes/api.ts`:
- Around line 834-888: The inline z.object tunnel response schemas in
getTunnelRoute, postTunnelStartRoute, and deleteTunnelStopRoute should be moved
to the system schemas module: create/export named schemas (e.g.,
TunnelStatusResponse, TunnelStartResponse, TunnelStopResponse) in
packages/server/src/routes/schemas/system.schemas.ts and replace the inline
z.object(...) usages by importing those named schemas into api.ts; also ensure
these routes continue to use registerOpenApiRoute(createRoute({...}), handler)
per routing guidelines if applicable.
- Around line 2583-2598: The tunnel routes (getTunnelRoute,
postTunnelStartRoute, deleteTunnelStopRoute) are currently callable under the
global cors({ origin: '*' }) policy; enforce a local-only or authenticated guard
before calling startTunnel/stopTunnel/getTunnelState by validating either (a)
the request origin/IP is loopback (e.g., 127.0.0.1/::1) or (b) a one-time
secret/admin token header; add this check in the registerOpenApiRoute handler
wrapper (or as middleware) so POST to startTunnel(5173) and DELETE stopTunnel()
are rejected unless the guard passes, and return a 403 for unauthorized attempts
rather than allowing the global CORS policy to expose these endpoints.
In `@packages/web/src/components/layout/TunnelPopover.tsx`:
- Around line 50-57: The handlers handleStart and handleStop currently set local
tunnel state optimistically before checking the HTTP response; change them to
await the fetch, check response.ok, and only call setTunnel to update status/url
when the request succeeds (for handleStart set status to 'starting' and set
returned url only on success; for handleStop set status to 'inactive' only on
success). On non-ok responses throw or handle the error and revert or preserve
the previous state (e.g., setTunnel back to prior status or show an error).
Apply the same fix to the duplicate handlers referenced around lines 217-224.
- Around line 4-9: The local TunnelState/TunnelStatus types are unsafe copies of
the server payload; import the shared/generated server type (or rename to match
it) and stop casting res.json() directly—create a runtime validator (e.g.,
isTunnelState(obj)) that checks obj.status is one of the allowed values and
obj.url is string|null, use that validator in both pollers before calling
setTunnel(...) and on failure log and set a safe fallback (e.g., status:
'error', url: null); update any references to TunnelState/TunnelStatus and the
poller res.json() usages (the functions that call setTunnel at lines referenced)
to use the imported type + runtime guard.
In `@packages/web/src/components/sidebar/ProjectsSidebar.tsx`:
- Around line 74-77: After creating the new codebase via addCodebase, do not
call setSelectedProjectId(codebase.id) until the codebases query cache is
updated; either seed the cache with the returned codebase (use
queryClient.setQueryData(['codebases'], prev => [...prev, codebase]) or the
equivalent) or await the refetch/invalidate (await
queryClient.invalidateQueries({ queryKey: ['codebases'] })) before calling
setSelectedProjectId so ProjectContext’s stale-selection cleanup won't clear the
new selection. Ensure you reference addCodebase, queryClient.invalidateQueries /
queryClient.setQueryData, setSelectedProjectId and the ['codebases'] query key
in your change.
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 60-73: The switch button with role="switch" (using compactLayout
and toggleCompactLayout) lacks an accessible name; update the button element to
include an accessible name by adding either an aria-label (e.g.,
aria-label="Compact layout") or an aria-labelledby that points to the nearby
label text node, ensuring screen readers announce the switch state; keep the
existing aria-checked and onClick behavior unchanged and choose the approach
consistent with other controls on the page.
---
Nitpick comments:
In `@packages/providers/src/claude/provider.ts`:
- Around line 325-330: The warning triggers even for a valid empty input; change
the check so we only call getLog().warn when the built sdkHooks is empty AND the
original nodeHooks actually contained at least one meaningful entry. Concretely,
before warning, compute whether nodeHooks has any non-undefined entries (e.g.
filter Object.keys(nodeHooks) for values !== undefined or check
Object.keys(nodeHooks).length > 0 and at least one value is defined) and only
call getLog().warn({ nodeHooksKeys: Object.keys(nodeHooks) },
'claude.hooks_build_produced_empty_map') when sdkHooks is empty AND that
filtered nodeHooks set is non-empty.
- Around line 543-564: The stderr handler's loose check output.includes('at ')
can produce false positives; update the isError detection inside the stderr
arrow function to use a more specific stack-trace pattern (e.g., a regex that
matches typical V8 stack frames like lines starting with optional whitespace
followed by "at " and a symbol/file path) instead of the plain includes('at '),
keeping the other checks (Error:, exception, fatal, failed) and the
isInfoMessage exclusions intact; ensure you reference and modify the isError
computation within the same stderr: (data: string): void => { ... } block so
getLog().error({ stderr: output }, 'subprocess_error') only fires for real
errors.
- Around line 210-216: The current env var expansion in the result[key]
assignment uses a regex that only matches uppercase names
(/\$([A-Z_][A-Z0-9_]*)/g), so lowercase env vars like $api_key are skipped and
not added to missingVars; update the regex in that replacement (the code around
result[key] = val.replace(...), the capture name varName, and the missingVars
push) to accept lowercase letters too (e.g., use /\$([A-Za-z_][A-Za-z0-9_]*)/g
or use a case-insensitive approach) so process.env[varName] is looked up for
mixed-case names and missingVars is populated correctly, or explicitly
document/enforce the uppercase-only convention if that was intended.
- Around line 638-641: The events parameter in streamClaudeMessages is an
untyped AsyncGenerator (defaults to unknown); update the parameter to use an
explicit AsyncGenerator type that matches the generator's yielded values (the
same type the function yields/handles) — e.g., change the events parameter to
AsyncGenerator<MessageChunk, void, unknown> (or the appropriate resolved/return
type used in your codebase) so the signature reads streamClaudeMessages(events:
AsyncGenerator<MessageChunk, void, unknown>, toolResultQueue:
ToolResultEntry[]): AsyncGenerator<MessageChunk>. Ensure you reference the
MessageChunk type and adjust the middle/return generic positions if your
generator returns a different value.
In `@packages/web/src/components/conversations/SlashCommandMenu.tsx`:
- Around line 41-44: The useEffect that resets selectedIndex on query changes
can leave selectedIndex out of range when the filtered list shrinks; update the
effect (or add a new useEffect dependent on filtered.length) to clamp
selectedIndex into [0, filtered.length - 1] instead of always setting to 0 —
e.g., compute newIndex = Math.max(0, Math.min(selectedIndex, filtered.length -
1)) and call setSelectedIndex(newIndex); reference the selectedIndex state
setter setSelectedIndex, the selectedIndex state, and the filtered array to
locate where to apply this clamp.
In `@packages/workflows/src/deps.ts`:
- Around line 70-92: The assistants sub-schema in WorkflowConfig is a
hand-maintained duplicate that has drifted (missing codexBinaryPath); replace
this manual shape by importing and using the canonical provider types instead:
remove the inline assistants definition and reference the provider-exported
types (e.g., the Codex/Claude provider config type) so WorkflowConfig.assistants
uses the exact provider types; ensure existing symbols referenced here
(WorkflowConfig, assistants, ModelReasoningEffort, WebSearchMode,
codexBinaryPath) are aligned by updating imports to pull the provider defaults
and adjust any usages to the imported type names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca277b1a-06af-4b0e-aa88-7627546e4e28
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (126)
.archon/scripts/echo-args.js.archon/workflows/e2e-all-nodes.yaml.archon/workflows/e2e-claude-smoke.yaml.archon/workflows/e2e-codex-smoke.yaml.archon/workflows/e2e-mixed-providers.yaml.claude/commands/plan-feature.md.claude/commands/prime-backend.md.claude/commands/prime-workflows.md.claude/commands/prime.md.claude/commands/validate.md.claude/docs/architecture-deep-dive.md.claude/rules/adapters.md.claude/rules/cli.md.claude/rules/database.md.claude/rules/dx-quirks.md.claude/rules/isolation-patterns.md.claude/rules/isolation.md.claude/rules/orchestrator.md.claude/rules/server-api.md.claude/rules/testing.md.claude/rules/web-frontend.md.claude/rules/workflows.md.prettierignoreCHANGELOG.mdCLAUDE.mdDockerfileRESPONSIVE_UI_PATCH.mddocs/cloudflare-tunnel.mdeslint.config.mjshomebrew/archon.rbpackages/cli/package.jsonpackages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/package.jsonpackages/core/src/clients/claude.tspackages/core/src/clients/codex.tspackages/core/src/clients/factory.test.tspackages/core/src/clients/factory.tspackages/core/src/clients/index.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/adapters/sqlite.tspackages/core/src/db/codebases.test.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/handlers/command-handler.test.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator-isolation.test.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/services/title-generator.test.tspackages/core/src/services/title-generator.tspackages/core/src/test/mocks/streaming.tspackages/core/src/types/index.tspackages/core/src/utils/env-leak-scanner.test.tspackages/core/src/utils/env-leak-scanner.tspackages/core/src/workflows/store-adapter.test.tspackages/core/src/workflows/store-adapter.tspackages/docs-web/src/content/docs/guides/skills.mdpackages/docs-web/src/content/docs/reference/api.mdpackages/docs-web/src/content/docs/reference/architecture.mdpackages/docs-web/src/content/docs/reference/cli.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/docs-web/src/content/docs/reference/security.mdpackages/paths/src/env-integration.test.tspackages/providers/package.jsonpackages/providers/src/claude/config.tspackages/providers/src/claude/index.tspackages/providers/src/claude/provider.test.tspackages/providers/src/claude/provider.tspackages/providers/src/codex/binary-guard.test.tspackages/providers/src/codex/binary-resolver-dev.test.tspackages/providers/src/codex/binary-resolver.test.tspackages/providers/src/codex/binary-resolver.tspackages/providers/src/codex/config.tspackages/providers/src/codex/index.tspackages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/errors.tspackages/providers/src/factory.test.tspackages/providers/src/factory.tspackages/providers/src/index.tspackages/providers/src/test/mocks/logger.tspackages/providers/src/types.tspackages/providers/tsconfig.jsonpackages/server/package.jsonpackages/server/src/adapters/web.tspackages/server/src/index.tspackages/server/src/routes/api.codebases.test.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/codebase.schemas.tspackages/server/src/tunnel.tspackages/web/index.htmlpackages/web/src/App.tsxpackages/web/src/components/chat/MessageInput.tsxpackages/web/src/components/conversations/ConversationItem.tsxpackages/web/src/components/conversations/SlashCommandMenu.tsxpackages/web/src/components/layout/Layout.tsxpackages/web/src/components/layout/TopNav.tsxpackages/web/src/components/layout/TunnelPopover.tsxpackages/web/src/components/sidebar/ProjectsSidebar.tsxpackages/web/src/components/workflows/BuilderToolbar.tsxpackages/web/src/components/workflows/NodeLibrary.tsxpackages/web/src/components/workflows/QuickAddPicker.tsxpackages/web/src/components/workflows/WorkflowBuilder.tsxpackages/web/src/components/workflows/WorkflowCanvas.tsxpackages/web/src/contexts/MobileNavContext.tsxpackages/web/src/contexts/ThemeContext.tsxpackages/web/src/index.csspackages/web/src/lib/api.generated.d.tspackages/web/src/lib/api.tspackages/web/src/lib/useVisualViewport.tspackages/web/src/routes/ChatPage.tsxpackages/web/src/routes/SettingsPage.tsxpackages/web/vite.config.tspackages/workflows/package.jsonpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/deps.tspackages/workflows/src/executor-preamble.test.tspackages/workflows/src/executor-shared.tspackages/workflows/src/executor.test.tspackages/workflows/src/hooks.test.tspackages/workflows/src/script-node-deps.test.ts
💤 Files with no reviewable changes (32)
- packages/cli/src/cli.ts
- packages/providers/src/codex/binary-resolver.ts
- packages/docs-web/src/content/docs/reference/configuration.md
- packages/docs-web/src/content/docs/reference/api.md
- .claude/rules/adapters.md
- .claude/rules/dx-quirks.md
- packages/workflows/src/executor-shared.ts
- .claude/rules/database.md
- packages/core/src/clients/factory.test.ts
- .claude/rules/testing.md
- packages/core/src/clients/factory.ts
- .claude/rules/cli.md
- .claude/rules/isolation-patterns.md
- packages/core/src/clients/index.ts
- packages/docs-web/src/content/docs/reference/cli.md
- packages/core/src/handlers/command-handler.test.ts
- .claude/rules/workflows.md
- packages/server/src/index.ts
- packages/core/src/handlers/clone.test.ts
- .claude/rules/orchestrator.md
- packages/core/src/utils/env-leak-scanner.test.ts
- .claude/rules/web-frontend.md
- .claude/rules/isolation.md
- packages/server/src/routes/schemas/codebase.schemas.ts
- .claude/rules/server-api.md
- packages/core/src/index.ts
- packages/core/src/clients/codex.ts
- packages/core/src/db/adapters/sqlite.ts
- packages/core/src/test/mocks/streaming.ts
- packages/core/src/config/config-loader.ts
- packages/core/src/clients/claude.ts
- packages/core/src/utils/env-leak-scanner.ts
✅ Files skipped from review due to trivial changes (27)
- packages/cli/package.json
- .prettierignore
- .claude/commands/prime.md
- .claude/commands/validate.md
- packages/docs-web/src/content/docs/guides/skills.md
- packages/providers/src/codex/binary-resolver-dev.test.ts
- packages/workflows/package.json
- .claude/commands/plan-feature.md
- .claude/commands/prime-workflows.md
- packages/providers/tsconfig.json
- .archon/scripts/echo-args.js
- homebrew/archon.rb
- packages/server/src/adapters/web.ts
- packages/web/vite.config.ts
- packages/web/index.html
- CHANGELOG.md
- packages/providers/src/codex/index.ts
- .claude/commands/prime-backend.md
- .archon/workflows/e2e-codex-smoke.yaml
- .archon/workflows/e2e-claude-smoke.yaml
- packages/core/src/orchestrator/orchestrator.test.ts
- packages/web/src/contexts/MobileNavContext.tsx
- packages/server/package.json
- docs/cloudflare-tunnel.md
- packages/providers/package.json
- RESPONSIVE_UI_PATCH.md
- packages/web/src/components/layout/Layout.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- eslint.config.mjs
- packages/web/src/components/workflows/BuilderToolbar.tsx
- packages/web/src/components/conversations/ConversationItem.tsx
- packages/web/src/lib/useVisualViewport.ts
- packages/web/src/components/workflows/QuickAddPicker.tsx
- packages/web/src/components/workflows/WorkflowCanvas.tsx
- packages/web/src/index.css
- packages/server/src/tunnel.ts
- packages/web/src/App.tsx
Comment on lines
+36
to
+39
| # 5. Bash node using $nodeId.output from structured node | ||
| - id: bash-read-output | ||
| bash: "echo 'Structured output category: $structured.output'" | ||
| depends_on: [structured] |
There was a problem hiding this comment.
$structured.output is quoted in a way that will break the bash node.
For bash nodes, $nodeId.output is already shell-escaped by the executor. Embedding the full JSON output inside '...' here means the generated command will contain nested quotes once structured returns something like {"category":"greeting"}. You probably want the field value anyway, not the whole JSON blob.
💡 Suggested fix
- id: bash-read-output
- bash: "echo 'Structured output category: $structured.output'"
+ bash: "printf 'Structured output category: %s\n' $structured.output.category"
depends_on: [structured]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/workflows/e2e-all-nodes.yaml around lines 36 - 39, The bash node
"bash-read-output" is embedding $structured.output inside single quotes which
will produce nested-quote JSON and break the shell command; update the bash
command to reference the specific field (e.g., $structured.output.category) and
remove the surrounding single quotes so the executor's shell-escaping can work
correctly (i.e., use an unquoted $structured.output.category or a properly
quoted form handled by the executor rather than wrapping the whole JSON in
'...').
Comment on lines
+19
to
+27
| # 3. Claude node reads Codex output — cross-provider ref | ||
| - id: claude-reads-codex | ||
| prompt: "The codex node said: '$codex-node.output'. Confirm you received it by saying 'cross-provider-ok'. Say nothing else." | ||
| depends_on: [codex-node] | ||
|
|
||
| # 4. Bash node verifies both outputs | ||
| - id: verify | ||
| bash: "echo 'claude=$claude-node.output codex=$codex-node.output cross=$claude-reads-codex.output'" | ||
| depends_on: [claude-node, codex-node, claude-reads-codex] |
There was a problem hiding this comment.
This smoke test never proves the cross-provider reference resolved.
claude-reads-codex is asked to return a constant string, and verify only echoes outputs. The workflow can still pass when $codex-node.output is empty/wrong, which defeats the point of this E2E test.
💡 Proposed fix
- id: claude-reads-codex
- prompt: "The codex node said: '$codex-node.output'. Confirm you received it by saying 'cross-provider-ok'. Say nothing else."
+ prompt: "Reply with exactly: cross-provider:$codex-node.output"
depends_on: [codex-node]
# 4. Bash node verifies both outputs
- id: verify
- bash: "echo 'claude=$claude-node.output codex=$codex-node.output cross=$claude-reads-codex.output'"
+ bash: |
+ test "$claude-node.output" = "claude-ok" || {
+ echo "unexpected claude output: $claude-node.output"
+ exit 1
+ }
+ test "$codex-node.output" = "codex-ok" || {
+ echo "unexpected codex output: $codex-node.output"
+ exit 1
+ }
+ test "$claude-reads-codex.output" = "cross-provider:codex-ok" || {
+ echo "unexpected cross-provider output: $claude-reads-codex.output"
+ exit 1
+ }
depends_on: [claude-node, codex-node, claude-reads-codex]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/workflows/e2e-mixed-providers.yaml around lines 19 - 27, The smoke
test doesn't validate the cross-provider reference because claude-reads-codex
returns a constant string and verify only echoes outputs; update the
claude-reads-codex node prompt to require it to return the actual codex output
(e.g., "Return exactly: '$codex-node.output' and nothing else") and change the
verify node's bash command to assert equality between claude-reads-codex.output
and codex-node.output (exit non-zero on mismatch and print a clear error),
ensuring you edit the prompt field of claude-reads-codex and the bash field of
the verify node to perform the actual equality check.
Comment on lines
40
to
44
| async function registerRepoAtPath( | ||
| targetPath: string, | ||
| name: string, | ||
| repositoryUrl: string | null, | ||
| allowEnvKeys = false, | ||
| context: LeakErrorContext = 'register-ui' | ||
| repositoryUrl: string | null | ||
| ): Promise<RegisterResult> { |
There was a problem hiding this comment.
Please add a replacement secret-scan/consent gate before auto-registering repos.
This signature cleanup removes the only policy inputs on this path, and I don't see a replacement check before registerRepoAtPath() persists the codebase and loads repository commands. That turns clone/register into unconditional admission of whatever is in the repo, including checked-in .env files or provider credentials, which is a real security regression even if the old API surface is being simplified.
Also applies to: 193-197, 286-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/handlers/clone.ts` around lines 40 - 44, The code currently
auto-registers repositories in registerRepoAtPath without a secret-scan/consent
gate; add a guard that runs a secrets scan (and requires explicit user consent
if secrets are found or if policy mandates) before persisting the codebase or
loading repository commands. Implement this by calling an existing
secret-scan/consent utility (or add secretScanAndRequireConsent(repoPath)
invoked at the top of registerRepoAtPath) and return/throw to abort registration
when the scan fails or consent is denied; also add the same pre-persist/pre-load
gate at the other clone/register entry points noted in the review (the other
call sites around the lines referenced) so no code path can persist or load
commands without this check. Ensure the gate logs the decision and fails closed
(deny registration) on errors.
Comment on lines
+50
to
56
| const client = getAgentProvider(assistantType); | ||
| let generatedTitle = ''; | ||
|
|
||
| for await (const chunk of client.sendQuery(titlePrompt, cwd, undefined, { | ||
| model: titleModel, | ||
| tools: [], // No tool access — pure text generation | ||
| nodeConfig: { allowed_tools: [] }, // No tool access — pure text generation | ||
| })) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect provider sendQuery option handling:"
rg -n -C3 --type=ts 'sendQuery\(|nodeConfig|allowed_tools' \
packages/providers/src \
packages/core/src/services/title-generator.tsRepository: coleam00/Archon
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check IAgentProvider interface for type-checking methods ==="
rg -n -A 10 'interface IAgentProvider|class.*Provider {' packages/providers/src/types.ts packages/providers/src/claude/provider.ts packages/providers/src/codex/provider.ts | head -80
echo -e "\n=== Search for getType or similar methods ==="
rg -n 'getType|getProvider|providerType' packages/providers/src --type=tsRepository: coleam00/Archon
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check IAgentProvider interface ==="
rg -n 'interface IAgentProvider' packages/providers/src --type=ts -A 15
echo -e "\n=== Check provider class declarations ==="
rg -n 'class ClaudeProvider|class CodexProvider' packages/providers/src --type=ts -A 5
echo -e "\n=== Search for type/provider identification methods ==="
rg -n 'getType|providerType|instanceof' packages/providers/src --type=ts -B 2 -A 2Repository: coleam00/Archon
Length of output: 5259
Gate allowed_tools by provider type.
generateAndSetTitle() accepts both 'claude' and 'codex', but allowed_tools is Claude-specific. Passing nodeConfig: { allowed_tools: [] } unconditionally causes Codex to silently ignore it, leaving tool access uncontrolled for non-Claude providers.
The interface provides getType() to check provider type—use it:
Suggested fix
const client = getAgentProvider(assistantType);
+ const sendQueryOptions =
+ client.getType() === 'claude'
+ ? { model: titleModel, nodeConfig: { allowed_tools: [] } }
+ : { model: titleModel };
let generatedTitle = '';
- for await (const chunk of client.sendQuery(titlePrompt, cwd, undefined, {
- model: titleModel,
- nodeConfig: { allowed_tools: [] }, // No tool access — pure text generation
- })) {
+ for await (const chunk of client.sendQuery(titlePrompt, cwd, undefined, sendQueryOptions)) {📝 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.
Suggested change
| const client = getAgentProvider(assistantType); | |
| let generatedTitle = ''; | |
| for await (const chunk of client.sendQuery(titlePrompt, cwd, undefined, { | |
| model: titleModel, | |
| tools: [], // No tool access — pure text generation | |
| nodeConfig: { allowed_tools: [] }, // No tool access — pure text generation | |
| })) { | |
| const client = getAgentProvider(assistantType); | |
| const sendQueryOptions = | |
| client.getType() === 'claude' | |
| ? { model: titleModel, nodeConfig: { allowed_tools: [] } } | |
| : { model: titleModel }; | |
| let generatedTitle = ''; | |
| for await (const chunk of client.sendQuery(titlePrompt, cwd, undefined, sendQueryOptions)) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/services/title-generator.ts` around lines 50 - 56, The code
unconditionally passes nodeConfig: { allowed_tools: [] } to client.sendQuery
which is Claude-specific and can be ignored or mishandled for other providers;
update generateAndSetTitle() to inspect the provider type via the client
returned by getAgentProvider(assistantType) (use client.getType()) and only
include nodeConfig with allowed_tools when client.getType() === 'claude' (for
other provider types, omit nodeConfig or pass no allowed_tools) so tool access
is properly controlled for Claude without affecting Codex or other providers.
Comment on lines
+10
to
+45
| /** | ||
| * Parse raw assistantConfig into typed Codex defaults. | ||
| * Defensive: invalid fields are silently dropped. | ||
| */ | ||
| export function parseCodexConfig(raw: Record<string, unknown>): CodexProviderDefaults { | ||
| const result: CodexProviderDefaults = {}; | ||
|
|
||
| if (typeof raw.model === 'string') { | ||
| result.model = raw.model; | ||
| } | ||
|
|
||
| const validEfforts = ['minimal', 'low', 'medium', 'high', 'xhigh']; | ||
| if ( | ||
| typeof raw.modelReasoningEffort === 'string' && | ||
| validEfforts.includes(raw.modelReasoningEffort) | ||
| ) { | ||
| result.modelReasoningEffort = | ||
| raw.modelReasoningEffort as CodexProviderDefaults['modelReasoningEffort']; | ||
| } | ||
|
|
||
| const validSearchModes = ['disabled', 'cached', 'live']; | ||
| if (typeof raw.webSearchMode === 'string' && validSearchModes.includes(raw.webSearchMode)) { | ||
| result.webSearchMode = raw.webSearchMode as CodexProviderDefaults['webSearchMode']; | ||
| } | ||
|
|
||
| if (Array.isArray(raw.additionalDirectories)) { | ||
| result.additionalDirectories = raw.additionalDirectories.filter( | ||
| (d): d is string => typeof d === 'string' | ||
| ); | ||
| } | ||
|
|
||
| if (typeof raw.codexBinaryPath === 'string') { | ||
| result.codexBinaryPath = raw.codexBinaryPath; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reject invalid Codex config instead of silently ignoring it.
Right now a typo like modelReasoningEffort: "medum" or a bad webSearchMode just disappears and the provider runs with different behavior than the user asked for. That makes config mistakes very hard to diagnose. Please fail the parse or return validation errors when supported fields are present but invalid, rather than dropping them.
As per coding guidelines, "Fail fast with clear errors for unsupported or unsafe states; never silently swallow errors, broaden permissions, or fallback without documentation; prefer throwing early."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/codex/config.ts` around lines 10 - 45, The
parseCodexConfig function currently drops invalid values silently; update it to
validate presence of known fields and throw a clear Error when a supported field
is present but has an invalid value or type (e.g., modelReasoningEffort not in
['minimal','low','medium','high','xhigh'], webSearchMode not in
['disabled','cached','live'], non-string model or codexBinaryPath, or
additionalDirectories present but not an array of strings). Locate
parseCodexConfig and replace the silent-filtering logic for model,
modelReasoningEffort, webSearchMode, additionalDirectories, and codexBinaryPath
to perform explicit type/enum checks and throw descriptive errors (including the
field name and invalid value) instead of silently omitting them; still allow
omission when a field is truly absent. Ensure thrown errors are instances of
Error with messages easy to surface to users.
Comment on lines
+21
to
+24
| export const SSE_BASE_URL = | ||
| import.meta.env.DEV && window.location.hostname === 'localhost' | ||
| ? `http://localhost:${apiPort}` | ||
| : ''; |
There was a problem hiding this comment.
Guard window when computing SSE_BASE_URL.
This now touches window.location during module evaluation. In SSR/test imports that can throw before the app renders, and this file already had a safer lazy-helper pattern for browser-only globals.
💡 Proposed fix
const apiPort = (import.meta.env.VITE_API_PORT as string | undefined) ?? '3090';
-export const SSE_BASE_URL =
- import.meta.env.DEV && window.location.hostname === 'localhost'
- ? `http://localhost:${apiPort}`
- : '';
+function getSSEBaseUrl(): string {
+ const hostname = typeof window !== 'undefined' ? window.location.hostname : 'localhost';
+ return import.meta.env.DEV && hostname === 'localhost' ? `http://localhost:${apiPort}` : '';
+}
+
+export const SSE_BASE_URL = getSSEBaseUrl();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/lib/api.ts` around lines 21 - 24, SSE_BASE_URL currently
reads window.location.hostname during module evaluation which can throw in
SSR/tests; replace the top-level constant with a guarded helper like
getSSEBaseUrl() that computes the base URL lazily, checking typeof window !==
'undefined' before accessing window.location.hostname, falling back to
'localhost' (or returning '' as appropriate) when window is undefined, and use
getSSEBaseUrl() wherever SSE_BASE_URL was referenced.
Comment on lines
+89
to
+110
| {mobileConvOpen && ( | ||
| <div | ||
| className="fixed inset-x-0 top-12 bottom-0 z-40 bg-black/60 md:hidden" | ||
| onClick={(): void => { | ||
| setMobileConvOpen(false); | ||
| }} | ||
| aria-hidden="true" | ||
| /> | ||
| )} | ||
|
|
||
| {/* Left panel: projects sidebar | ||
| Desktop : inline sidebar (relative, h-full) | ||
| Mobile : fixed overlay drawer, slides from left */} | ||
| <div | ||
| className="relative flex h-full flex-col border-r border-border bg-surface overflow-hidden" | ||
| style={{ width: `${String(width)}px`, flexShrink: 0 }} | ||
| className={cn( | ||
| 'flex flex-col border-r border-border overflow-hidden', | ||
| 'fixed top-12 bottom-0 left-0 z-50', | ||
| 'md:relative md:inset-auto md:z-auto md:h-full', | ||
| 'transition-transform duration-300 ease-in-out', | ||
| mobileConvOpen ? 'translate-x-0' : '-translate-x-full md:translate-x-0' | ||
| )} | ||
| style={{ width: `${String(width)}px`, flexShrink: 0, backgroundColor: 'var(--surface)' }} |
There was a problem hiding this comment.
The mobile drawer still lacks Escape-to-close.
Backdrop click is handled, but this is still a plain div with no Escape listener, so keyboard users cannot dismiss it the way the feature spec describes. Add a keydown handler while mobileConvOpen is true, or switch to a drawer/dialog primitive that handles dismissal for you.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/routes/ChatPage.tsx` around lines 89 - 110, The mobile
drawer controlled by mobileConvOpen lacks Escape-key dismissal; add a keydown
listener in the ChatPage component (e.g., in a useEffect) that watches
mobileConvOpen and calls setMobileConvOpen(false) when event.key === 'Escape',
and ensure you remove the listener on cleanup to avoid leaks; alternatively,
replace the manual drawer div with an accessible dialog/drawer primitive that
provides built-in Escape handling and focus management. Use the mobileConvOpen
state, setMobileConvOpen setter, and the drawer div/component as the integration
points so the Escape handler only runs while the drawer is open.
Comment on lines
+103
to
+110
| className={cn( | ||
| 'flex flex-col border-r border-border overflow-hidden', | ||
| 'fixed top-12 bottom-0 left-0 z-50', | ||
| 'md:relative md:inset-auto md:z-auto md:h-full', | ||
| 'transition-transform duration-300 ease-in-out', | ||
| mobileConvOpen ? 'translate-x-0' : '-translate-x-full md:translate-x-0' | ||
| )} | ||
| style={{ width: `${String(width)}px`, flexShrink: 0, backgroundColor: 'var(--surface)' }} |
There was a problem hiding this comment.
Clamp the mobile drawer width to the viewport.
The persisted desktop sidebar width is reused here on mobile. If it was last saved near PANEL_MAX, narrow phones can render part of the drawer off-screen, which can hide the search row and close button. Add a mobile cap such as maxWidth: '100vw' or a dedicated full-width mobile style.
📏 Minimal fix
- style={{ width: `${String(width)}px`, flexShrink: 0, backgroundColor: 'var(--surface)' }}
+ style={{
+ width: `${String(width)}px`,
+ maxWidth: '100vw',
+ flexShrink: 0,
+ backgroundColor: 'var(--surface)',
+ }}📝 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.
Suggested change
| className={cn( | |
| 'flex flex-col border-r border-border overflow-hidden', | |
| 'fixed top-12 bottom-0 left-0 z-50', | |
| 'md:relative md:inset-auto md:z-auto md:h-full', | |
| 'transition-transform duration-300 ease-in-out', | |
| mobileConvOpen ? 'translate-x-0' : '-translate-x-full md:translate-x-0' | |
| )} | |
| style={{ width: `${String(width)}px`, flexShrink: 0, backgroundColor: 'var(--surface)' }} | |
| className={cn( | |
| 'flex flex-col border-r border-border overflow-hidden', | |
| 'fixed top-12 bottom-0 left-0 z-50', | |
| 'md:relative md:inset-auto md:z-auto md:h-full', | |
| 'transition-transform duration-300 ease-in-out', | |
| mobileConvOpen ? 'translate-x-0' : '-translate-x-full md:translate-x-0' | |
| )} | |
| style={{ | |
| width: `${String(width)}px`, | |
| maxWidth: '100vw', | |
| flexShrink: 0, | |
| backgroundColor: 'var(--surface)', | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/routes/ChatPage.tsx` around lines 103 - 110, The sidebar
drawer uses the persisted width value (width) directly, which can exceed small
viewports and push controls off-screen; update the style object on the element
that builds the sidebar (the one using className with mobileConvOpen and
style={{ width: `${String(width)}px`, ... }}) to enforce a mobile cap — e.g.,
add maxWidth: '100vw' or compute a clampedWidth (Math.min(width,
window.innerWidth) or a CSS clamp) and use that for width when
mobileConvOpen/viewport is narrow so the drawer never extends beyond the
viewport.
- C1: Add localhost-only guard (403) on /api/tunnel/start and /api/tunnel/stop - M1: Move tunnel Zod schemas to system.schemas.ts; import from api.ts - M2: Check response.ok in TunnelPopover handleStart/handleStop; set error status on failure - M3: Validate /api/tunnel poll response with isTunnelState() guard instead of unsafe cast - M4: Await queryClient.invalidateQueries before setSelectedProjectId in ProjectsSidebar - M5: MobileNavContext uses null default; useMobileNav throws if outside provider - M6: useVisualViewport guards typeof window, adds window resize fallback for no-visualViewport browsers - m1: Remove useVisualViewport.ts from eslint ignore list (now uses semicolons, no longer pre-existing) - m2: Add 30s timeout to cloudflared URL detection; kill process and set error on timeout - m3: Add explicit URL slash guard (reject :// pattern) in MessageInput slash detection - m4: Fix PICKER_WIDTH from 240 to 256 to match actual QuickAddPicker w-64 width - m5: Add aria-label to role=switch button in SettingsPage - m6: Add role="dialog" to mobile drawer aside in Layout - m7: Move aria-activedescendant from listbox to textarea; add aria-controls/aria-expanded on textarea - m8: Add language identifiers to all bare fenced code blocks in RESPONSIVE_UI_PATCH.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…uItem export Post-rebase fixes: MobileNavContextValue was missing pinned and togglePin fields (dropped when resolving rebase conflicts), and TunnelPopover was missing the TunnelMenuItem export required by TopNav for mobile hamburger menu. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… badges Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
|
This PR appears to fully address #1205. Consider adding |
Collaborator
|
This PR appears to fully address #1204. Consider adding |
Collaborator
|
Related to #1214 — overlapping area or partial fix. |
Collaborator
|
Related to #1080 — overlapping area or partial fix. |
This was referenced Apr 17, 2026
Collaborator
Collaborator
|
This is a real gap and something that will be addressed, but we will do a redisgn soon so will not merge this before the redesign |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR delivers a complete mobile-first UI overhaul for the Archon web interface, adding responsive layout management, a ChatGPT-style Projects experience, slash command autocomplete, and a redesigned navigation. All 16 CodeRabbit review issues have been addressed.
What changed and why
🗂️ ChatGPT-style Projects sidebar
Problem: The previous sidebar used an accordion component that was hard to scan on mobile and didn't give projects enough visual weight.
Solution: Projects are now listed flat (like ChatGPT), and clicking a project navigates to a dedicated
/projects/:idpage with two tabs:archon-*workflows collapse into anarchongroup with a select-all checkbox)The project header is compact and horizontal: a small icon on the left, URL and description on the right with a "See more / See less" toggle if the text overflows.
📱 Responsive sidebar with pin/unpin
Problem: The sidebar had no concept of screen size — it was either always hidden or always shown.
Solution:
A Archon ›logo in the top bar, closes automatically after navigationPinOfficon in the sidebar header lets the user detach it. When detached, theA Archon ›logo in the top bar reopens it. APinicon in the drawer header re-pins it. State is persisted inlocalStorage(archon-sidebar-pinned)⌨️ Slash command autocomplete
Problem: Users had to remember workflow names exactly to invoke them.
Solution: Typing
/in the chat input opens a floating popover listing all available workflows, filtered in real time as you type. Supports keyboard navigation (↑ ↓ Enter Escape) and mouse selection. Maximum 8 suggestions shown. The selected workflow replaces the/querytext in the input.🔀 Navigation redesign
Problem: Navigation was duplicated across the sidebar and the top bar, with two hamburger icons visible simultaneously on mobile.
Solution:
≡hamburger in the top-right opens a dropdown for Dashboard / Workflows / SettingsA Archonlogo becomes the only sidebar toggle, with a›chevron when the sidebar is closed🔧 Workflow Builder — naming convention hint
Problem: No guidance existed for naming workflows consistently.
Solution: A persistent hint bar below the toolbar explains the convention:
archon-→ built-in workflows (read-only)my-project-→ local to a project (e.g.dedibox-deploy)The input placeholder also shows examples:
e.g. dedibox-deploy, git-review.🌙 Dark / light theme toggle
Added a Sun/Moon toggle in the top bar that switches themes and persists the choice across sessions.
☁️ Cloudflare Quick Tunnel
Added a one-click tunnel button in the top bar that spawns a
cloudflaredquick tunnel and displays the public URL with a copy button. Useful for previewing the app on a mobile device without exposing a fixed port.🐛 iOS & mobile fixes
window.visualViewport(withresizefallback) so the layout shrinks correctly when the soft keyboard appears on iOS Safari / Chrome Androidfont-size: 16px(text-base) — prevents iOS from zooming in on focususeVisualViewportguardstypeof window !== 'undefined'for SSR/test safetyCodeRabbit review — all 16 issues addressed
localhost-only guard onPOST /api/tunnel/startandDELETE /api/tunnel/stoproutes/schemas/system.schemas.tsresponse.okcheck in tunnel handlersstatus='error'on failureisTunnelState()type guardawait queryClient.invalidateQueries()before selecting new projectMobileNavContextsilent no-ops outside providercreateContext(null)+ throw in hookuseVisualViewportunsafewindowaccesswindow.resizefallbackuseVisualViewportin ESLint no-semicolon exceptioncloudflaredprocesserrorand kills process/PICKER_WIDTHmismatch (240 vs 256)role="switch"missingaria-label<aside>missingrole="dialog"aria-modalaria-activedescendanton wrong elementonNavigatefires on non-navigation clicksFiles changed (summary)
Layout.tsx,TopNav.tsx,MobileNavContext.tsxProjectsSidebar.tsx,ProjectPage.tsx(new),App.tsxMessageInput.tsx,SlashCommandMenu.tsx(new)TunnelPopover.tsx,tunnel.ts,routes/api.ts,routes/schemas/system.schemas.tsuseVisualViewport.tsBuilderToolbar.tsx,WorkflowCanvas.tsxSettingsPage.tsxRESPONSIVE_UI_PATCH.md(new)How to test
A Archon ›to open, navigate to a project, check the Chats and Settings tabsPinOffto detach, then reopen viaA Archon ›; clickPinto re-pin/arch— autocomplete popover should appear with matching workflows