From eeede9801486d6a81222dbedf91b90f8714b83a0 Mon Sep 17 00:00:00 2001 From: Gaige Roberson Date: Thu, 23 Apr 2026 06:55:37 -0500 Subject: [PATCH 1/4] fix(review): harden mcp-bootstrap spawn and skip bootstrap when no claude config Two issues from the post-merge fresh-eyes review of phase 6: - The /spawn-sub-agent endpoint passed a user-supplied workingDir straight into child_process.spawn's cwd with no validation. An authenticated client could point claude at /etc, /var, or anywhere else the server uid can reach. Now we resolve the path, require it to live under $HOME (or DISPATCH_PROJECT_ROOTS), reject sensitive credential subdirs, and confirm it exists as a directory before spawning. - ensureRecommendedMCPs ran as a top-level module side-effect on import and would happily materialize a fresh ~/.claude.json containing only Dispatch's two MCPs for users who had never run claude. The bootstrap now refuses to create the file from nothing and defers via setImmediate so the import chain in server/index.js is not blocked by file IO. --- server/routes/mcp-bootstrap.js | 86 ++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 4 deletions(-) diff --git a/server/routes/mcp-bootstrap.js b/server/routes/mcp-bootstrap.js index fea60ad0ae..a39b42faae 100644 --- a/server/routes/mcp-bootstrap.js +++ b/server/routes/mcp-bootstrap.js @@ -80,6 +80,68 @@ async function readClaudeConfig() { return (await readJsonIfExists(CLAUDE_CONFIG_PATH)) || {}; } +// Returns true only when ~/.claude.json already exists. Bootstrap refuses to +// materialize a brand-new config for users who have never run Claude — the +// recommended-MCPs entries would be the *only* keys in that file, which the +// user did not opt into. Once Claude itself is installed, the file appears +// and the bootstrap may run on the next boot. +async function claudeConfigExists() { + try { + await fs.access(CLAUDE_CONFIG_PATH); + return true; + } catch (err) { + if (err && err.code === 'ENOENT') return false; + throw err; + } +} + +const ALLOWED_WORKING_DIR_ROOTS = (() => { + const roots = [os.homedir()]; + if (process.env.DISPATCH_PROJECT_ROOTS) { + for (const candidate of process.env.DISPATCH_PROJECT_ROOTS.split(path.delimiter)) { + const trimmed = candidate.trim(); + if (trimmed) roots.push(path.resolve(trimmed)); + } + } + return roots.map((p) => path.resolve(p)); +})(); + +const SENSITIVE_HOME_SUBDIRS = ['.ssh', '.aws', '.gnupg', '.config/gh', 'Library/Keychains']; + +// Returns the resolved absolute path if `input` is a usable working dir for a +// sub-agent spawn, or { error } describing why it isn't. Conservative gate: +// must be absolute, must resolve under HOME (or DISPATCH_PROJECT_ROOTS), and +// must not point at well-known credential stores. +async function resolveSafeWorkingDir(input) { + if (typeof input !== 'string' || !input.trim()) { + return { error: 'workingDir must be a non-empty string' }; + } + if (input.includes('\0')) { + return { error: 'workingDir contains a null byte' }; + } + const resolved = path.resolve(input); + const homeDir = path.resolve(os.homedir()); + const underAllowedRoot = ALLOWED_WORKING_DIR_ROOTS.some((root) => { + return resolved === root || resolved.startsWith(root + path.sep); + }); + if (!underAllowedRoot) { + return { error: 'workingDir must be under $HOME or DISPATCH_PROJECT_ROOTS' }; + } + for (const subdir of SENSITIVE_HOME_SUBDIRS) { + const sensitive = path.resolve(homeDir, subdir); + if (resolved === sensitive || resolved.startsWith(sensitive + path.sep)) { + return { error: `workingDir resolves to a sensitive directory (${subdir})` }; + } + } + try { + const stat = await fs.stat(resolved); + if (!stat.isDirectory()) return { error: 'workingDir is not a directory' }; + } catch { + return { error: 'workingDir does not exist' }; + } + return { path: resolved }; +} + async function writeClaudeConfig(config) { await writeJsonAtomic(CLAUDE_CONFIG_PATH, config); } @@ -98,6 +160,12 @@ function installedInConfig(config, name) { */ export async function ensureRecommendedMCPs() { const state = await readDispatchState(); + if (!(await claudeConfigExists())) { + // First-run users without Claude installed — do not materialize a + // ~/.claude.json containing only Dispatch's two MCPs. They get added + // on a later boot once the user has actually run Claude. + return { added: [], state, skipped: 'no-claude-config' }; + } let config; try { config = await readClaudeConfig(); @@ -226,10 +294,16 @@ router.post('/recommended/:name/toggle', async (req, res) => { router.post('/spawn-sub-agent', async (req, res) => { const subAgentType = typeof req.body?.subAgentType === 'string' ? req.body.subAgentType : 'general-purpose'; const userPrompt = typeof req.body?.prompt === 'string' ? req.body.prompt : ''; - const workingDir = typeof req.body?.workingDir === 'string' && req.body.workingDir + const requestedWorkingDir = typeof req.body?.workingDir === 'string' && req.body.workingDir ? req.body.workingDir : process.cwd(); + const validated = await resolveSafeWorkingDir(requestedWorkingDir); + if (validated.error) { + return res.status(400).json({ error: `Invalid workingDir: ${validated.error}` }); + } + const workingDir = validated.path; + res.setHeader('Content-Type', 'text/event-stream'); res.setHeader('Cache-Control', 'no-cache'); res.setHeader('Connection', 'keep-alive'); @@ -352,9 +426,13 @@ router.get('/session-files-touched/:projectName/:sessionId', async (req, res) => }); // Kick off bootstrap as a module-level side-effect so `import` in server/index.js -// registers the MCPs without needing a second wiring line. -ensureRecommendedMCPs().catch((err) => { - console.error('[mcp-bootstrap] ensureRecommendedMCPs failed:', err.message); +// registers the MCPs without needing a second wiring line. Deferred via +// setImmediate so the import chain is not blocked by file IO, and skipped when +// ~/.claude.json doesn't yet exist (see ensureRecommendedMCPs). +setImmediate(() => { + ensureRecommendedMCPs().catch((err) => { + console.error('[mcp-bootstrap] ensureRecommendedMCPs failed:', err.message); + }); }); export default router; From 785d81a3ba6063f7cedd6abda4b7143e65528db6 Mon Sep 17 00:00:00 2001 From: Gaige Roberson Date: Thu, 23 Apr 2026 06:56:08 -0500 Subject: [PATCH 2/4] fix(review): preview port allowlist, chrome viewer operator scoping, tasks path safety Three security fixes from the post-merge fresh-eyes review of phase 5: - Preview proxy used to forward any port from 1-65535 to 127.0.0.1, so any authenticated user could probe localhost services (mysql, postgres, redis, the cdp port itself, etc). Now defaults to a curated allowlist of common dev-server ports, blocks well-known infra ports outright, and exposes DISPATCH_PREVIEW_PORTS / DISPATCH_PREVIEW_ALLOW_ANY_HIGH_PORT for users who need something else. - Chrome screencast attaches to the operator's real browser over CDP and was reachable by any logged-in user. The Input.* dispatch path let any client drive the host browser (steal cookies via Runtime.evaluate, navigate to attacker URLs, etc). Now disabled by default; opt in via DISPATCH_CHROME_VIEW_ENABLED=true for view-only and additionally DISPATCH_CHROME_VIEW_ALLOW_INPUT=true for take-control. The HTTP /status and /tabs helpers report the disabled state cleanly so the client UI can render an explanation instead of a stack trace. - /api/tasks built the JSONL path from query params with no traversal guard. Now rejects path separators, null bytes, and '..' on projectName + sessionId, then prefix-checks the resolved path against ~/.claude/projects. --- server/routes/chrome-screencast.js | 41 ++++++++++++++++++++++++++++ server/routes/preview-proxy.js | 44 ++++++++++++++++++++++++++++-- server/routes/tasks.js | 15 ++++++++-- 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/server/routes/chrome-screencast.js b/server/routes/chrome-screencast.js index a259b74bf5..d6cf2b5e14 100644 --- a/server/routes/chrome-screencast.js +++ b/server/routes/chrome-screencast.js @@ -8,6 +8,15 @@ import { authenticateWebSocket } from '../middleware/auth.js'; const CDP_HOST = process.env.CHROME_CDP_HOST || '127.0.0.1'; const CDP_PORT = Number(process.env.CHROME_CDP_PORT || 9222); +// Opt-in flags. The screencast attaches to the operator's REAL Chrome over +// CDP — any authenticated Dispatch user could otherwise read cookies via +// Runtime.evaluate or drive the host browser (steal sessions, navigate to +// attacker URLs, etc.). Default is fully off; operator must explicitly enable. +// DISPATCH_CHROME_VIEW_ENABLED=true — turn the feature on (view-only) +// DISPATCH_CHROME_VIEW_ALLOW_INPUT=true — additionally accept Input.* events +const CHROME_VIEW_ENABLED = process.env.DISPATCH_CHROME_VIEW_ENABLED === 'true'; +const CHROME_VIEW_ALLOW_INPUT = process.env.DISPATCH_CHROME_VIEW_ALLOW_INPUT === 'true'; + function fetchChromeVersion() { return new Promise((resolve, reject) => { const req = http.get( @@ -212,6 +221,16 @@ async function handleChromeScreencastConnection(clientWs) { clientWs.on('message', async (raw) => { if (!cdp) return; + if (!CHROME_VIEW_ALLOW_INPUT) { + // View-only mode (default). Drop any Input.* dispatch attempts so a + // logged-in user cannot drive the operator's real browser. The first + // such message gets an explicit notice; subsequent ones are silent. + safeSend(clientWs, { + type: 'input-disabled', + error: 'Input forwarding disabled. Set DISPATCH_CHROME_VIEW_ALLOW_INPUT=true to enable.', + }); + return; + } let msg; try { msg = JSON.parse(raw.toString()); @@ -296,6 +315,12 @@ export function attachChromeScreencast(httpServer, rootWss) { const rawUrl = req.url || ''; if (!rawUrl.startsWith('/ws/chrome-view')) return; + if (!CHROME_VIEW_ENABLED) { + socket.write('HTTP/1.1 403 Forbidden\r\n\r\n'); + socket.destroy(); + return; + } + const parsed = new URL(rawUrl, 'http://localhost'); const token = parsed.searchParams.get('token') || @@ -319,16 +344,26 @@ export function attachChromeScreencast(httpServer, rootWss) { const router = express.Router(); router.get('/status', async (_req, res) => { + if (!CHROME_VIEW_ENABLED) { + return res.status(503).json({ + ok: false, + enabled: false, + hint: 'Chrome viewer disabled. Set DISPATCH_CHROME_VIEW_ENABLED=true to opt in.', + }); + } try { const version = await fetchChromeVersion(); res.json({ ok: true, + enabled: true, + inputEnabled: CHROME_VIEW_ALLOW_INPUT, browser: version.Browser, protocolVersion: version['Protocol-Version'], }); } catch (err) { res.status(503).json({ ok: false, + enabled: true, error: err && err.message ? err.message : 'CDP unreachable', hint: 'Launch Chrome with --remote-debugging-port=9222', }); @@ -336,6 +371,12 @@ router.get('/status', async (_req, res) => { }); router.get('/tabs', async (_req, res) => { + if (!CHROME_VIEW_ENABLED) { + return res.status(503).json({ + enabled: false, + hint: 'Chrome viewer disabled. Set DISPATCH_CHROME_VIEW_ENABLED=true to opt in.', + }); + } try { const tabs = await fetchChromeTabs(); res.json( diff --git a/server/routes/preview-proxy.js b/server/routes/preview-proxy.js index c42f526136..b29cc1a1b7 100644 --- a/server/routes/preview-proxy.js +++ b/server/routes/preview-proxy.js @@ -19,10 +19,47 @@ const HOP_BY_HOP_HEADERS = new Set([ 'upgrade', ]); +// Default-deny common service ports even within the dev range. Postgres, +// MySQL, Redis, MongoDB, Memcached, Elasticsearch, Kafka, the CDP itself, +// and a few other infra defaults — proxying to them via an authenticated +// browser session is an SSRF foothold, not a developer convenience. +const DEFAULT_BLOCKED_PORTS = new Set([ + 3306, 5432, 6379, 9092, 9200, 9300, 11211, 27017, 28017, 9222, 9223, 5984, 8086, +]); + +const DEFAULT_PORT_ALLOWLIST = new Set([ + 3000, 3001, 3002, 3003, 3030, 3333, + 4000, 4173, 4200, + 5000, 5001, 5173, 5174, 5500, 5555, + 6000, 6006, + 7000, 7777, + 8000, 8001, 8080, 8081, 8888, + 9000, 9001, 9090, 9876, +]); + +function parsePortListEnv(value) { + if (!value) return null; + const out = new Set(); + for (const piece of value.split(',')) { + const n = Number(piece.trim()); + if (Number.isInteger(n) && n >= 1 && n <= 65535) out.add(n); + } + return out.size > 0 ? out : null; +} + +const ENV_ALLOWLIST = parsePortListEnv(process.env.DISPATCH_PREVIEW_PORTS); +const ALLOW_ANY_HIGH_PORT = process.env.DISPATCH_PREVIEW_ALLOW_ANY_HIGH_PORT === 'true'; + function isValidPort(portStr) { if (!PORT_PATTERN.test(portStr)) return false; const port = Number(portStr); - return port >= 1 && port <= 65535; + if (port < 1 || port > 65535) return false; + if (DEFAULT_BLOCKED_PORTS.has(port)) return false; + if (ENV_ALLOWLIST) return ENV_ALLOWLIST.has(port); + if (DEFAULT_PORT_ALLOWLIST.has(port)) return true; + // Opt-in escape hatch for users running dev servers on uncommon ports. + if (ALLOW_ANY_HIGH_PORT && port >= 1024) return true; + return false; } function stripHopByHop(headers) { @@ -49,7 +86,10 @@ router.use('/:port', (req, res) => { const { port } = req.params; if (!isValidPort(port)) { - res.status(400).json({ error: 'Invalid port' }); + res.status(400).json({ + error: 'Port not allowed for preview proxy', + hint: 'Set DISPATCH_PREVIEW_PORTS or DISPATCH_PREVIEW_ALLOW_ANY_HIGH_PORT=true to allow custom ports.', + }); return; } diff --git a/server/routes/tasks.js b/server/routes/tasks.js index 984df3cfba..e3ed61230b 100644 --- a/server/routes/tasks.js +++ b/server/routes/tasks.js @@ -6,7 +6,12 @@ import express from 'express'; const router = express.Router(); -const CLAUDE_PROJECTS_DIR = path.join(os.homedir(), '.claude', 'projects'); +const CLAUDE_PROJECTS_DIR = path.resolve(os.homedir(), '.claude', 'projects'); + +// Reject anything that looks like path traversal or a separator before we +// try to open a file. Defense-in-depth: the resolved path is also prefix- +// checked against CLAUDE_PROJECTS_DIR below. +const UNSAFE_PATH_SEGMENT = /[\\/]|\0|\.\./; function slugFromProjectName(projectName) { // Claude stores JSONLs in `~/.claude/projects//`. The slug is the @@ -16,7 +21,13 @@ function slugFromProjectName(projectName) { async function readSessionFile(projectSlug, sessionId) { if (!projectSlug || !sessionId) return null; - const jsonlPath = path.join(CLAUDE_PROJECTS_DIR, projectSlug, `${sessionId}.jsonl`); + if (UNSAFE_PATH_SEGMENT.test(projectSlug) || UNSAFE_PATH_SEGMENT.test(sessionId)) { + return null; + } + const jsonlPath = path.resolve(CLAUDE_PROJECTS_DIR, projectSlug, `${sessionId}.jsonl`); + if (!jsonlPath.startsWith(CLAUDE_PROJECTS_DIR + path.sep)) { + return null; + } try { const raw = await fs.readFile(jsonlPath, 'utf8'); return raw.split('\n').filter(Boolean); From 412376233efd90ff9e81b2909c39daec8a7aa0f7 Mon Sep 17 00:00:00 2001 From: Gaige Roberson Date: Thu, 23 Apr 2026 06:56:29 -0500 Subject: [PATCH 3/4] feat(review): wire mobile preview/browser modals and worktree activity context Two phase 5 frontend follow-ups from the post-merge review: - PreviewModal and BrowserModal shipped in phase 5 but were never imported anywhere, so mobile users on the preview/browser tabs got the desktop pane shoved into the tab slot with no fullscreen treatment or close button. MainContent now mounts both modals, gated by isMobile + activeTab; the inline panes stay desktop-only. Closing a modal returns to the chat tab. - WorktreeList accepted activeSessions/processingSessions/blockedSessions /worktreeSessionMap props but its parent (SidebarProjectItem) is on the no-edit churn list, so the props were never passed and every dot stayed grey. New SessionActivityContext lives at the AppContent root and delivers the same protection sets to WorktreeList without prop drilling. worktreeSessionMap is best-effort for now: only the currently selected worktree's session can be attributed; cross-worktree resolution needs a server endpoint, tracked in docs/follow-ups.md. --- src/components/app/AppContent.tsx | 24 ++++++- .../main-content/view/MainContent.tsx | 20 ++++-- .../sidebar/worktrees/WorktreeList.tsx | 19 ++++-- src/contexts/SessionActivityContext.tsx | 67 +++++++++++++++++++ 4 files changed, 121 insertions(+), 9 deletions(-) create mode 100644 src/contexts/SessionActivityContext.tsx diff --git a/src/components/app/AppContent.tsx b/src/components/app/AppContent.tsx index dae663cc20..c944b32056 100644 --- a/src/components/app/AppContent.tsx +++ b/src/components/app/AppContent.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; import { useNavigate, useParams } from 'react-router-dom'; import { useTranslation } from 'react-i18next'; @@ -9,6 +9,7 @@ import MobileTabBar from '../layout/MobileTabBar'; import MobileSidebarSheet from '../layout/MobileSidebarSheet'; import type { AppNavSlot } from '../layout/useAppNavItems'; import { useWebSocket } from '../../contexts/WebSocketContext'; +import { SessionActivityProvider } from '../../contexts/SessionActivityContext'; import { useDeviceSettings } from '../../hooks/useDeviceSettings'; import { useSessionProtection } from '../../hooks/useSessionProtection'; import { useProjectsState } from '../../hooks/useProjectsState'; @@ -136,6 +137,21 @@ export default function AppContent() { return () => vv.removeEventListener('resize', update); }, []); + // Best-effort worktree → sessionId map for live activity dots in + // WorktreeList. We can confidently attribute the *currently selected* + // session to its project's fullPath; cross-worktree resolution requires a + // future server endpoint that returns the active session per worktree + // (tracked in docs/follow-ups.md). + const worktreeSessionMap = useMemo(() => { + const map: Record = {}; + const path = selectedProject?.fullPath; + const id = selectedSession?.id; + if (path && id && selectedProject?.isWorktree) { + map[path] = id; + } + return map; + }, [selectedProject?.fullPath, selectedProject?.isWorktree, selectedSession?.id]); + const handleNavSelect = useCallback((slot: AppNavSlot) => { switch (slot) { case 'chat': @@ -163,6 +179,11 @@ export default function AppContent() { }, [isMobile, openSettings, setActiveTab, setSidebarOpen]); return ( +
+ ); } diff --git a/src/components/main-content/view/MainContent.tsx b/src/components/main-content/view/MainContent.tsx index c903c311b9..299c3345f7 100644 --- a/src/components/main-content/view/MainContent.tsx +++ b/src/components/main-content/view/MainContent.tsx @@ -15,7 +15,9 @@ import EditorSidebar from '../../code-editor/view/EditorSidebar'; import type { Project } from '../../../types/app'; import { TaskMasterPanel } from '../../task-master'; import PreviewPane from '../../preview/PreviewPane'; +import PreviewModal from '../../preview/PreviewModal'; import BrowserPane from '../../browser/BrowserPane'; +import BrowserModal from '../../browser/BrowserModal'; import TasksPane from '../../tasks/TasksPane'; import TasksModal from '../../tasks/TasksModal'; @@ -172,12 +174,12 @@ function MainContent({ {shouldShowTasksTab && } -
- {activeTab === 'preview' && } +
+ {activeTab === 'preview' && !isMobile && }
-
- {activeTab === 'browser' && } +
+ {activeTab === 'browser' && !isMobile && }
{activeTab.startsWith('plugin:') && ( @@ -243,6 +245,16 @@ function MainContent({ sessionId={selectedSession?.id ?? null} ws={ws} /> + + setActiveTab('chat')} + /> + + setActiveTab('chat')} + />
); } diff --git a/src/components/sidebar/worktrees/WorktreeList.tsx b/src/components/sidebar/worktrees/WorktreeList.tsx index eac753c1e7..2168dcc581 100644 --- a/src/components/sidebar/worktrees/WorktreeList.tsx +++ b/src/components/sidebar/worktrees/WorktreeList.tsx @@ -1,6 +1,8 @@ import { useCallback, useEffect, useMemo, useState } from 'react'; import { Plus, Trash2, GitBranch, Loader2 } from 'lucide-react'; +import { useSessionActivity } from '../../../contexts/SessionActivityContext'; + export type Worktree = { path: string; branch: string | null; @@ -98,12 +100,21 @@ function dotAria(status: WorktreeStatus): string { export default function WorktreeList({ repoPath, - activeSessions, - processingSessions, - blockedSessions, - worktreeSessionMap = {}, + activeSessions: activeSessionsProp, + processingSessions: processingSessionsProp, + blockedSessions: blockedSessionsProp, + worktreeSessionMap: worktreeSessionMapProp, onSelect, }: WorktreeListProps) { + // Activity sets are normally piped through the SessionActivityProvider in + // AppContent — SidebarProjectItem (parent of this list) is a no-edit churn + // file per docs/CLAUDE.md, so the props can't be threaded through it. + const ctx = useSessionActivity(); + const activeSessions = activeSessionsProp ?? ctx.activeSessions; + const processingSessions = processingSessionsProp ?? ctx.processingSessions; + const blockedSessions = blockedSessionsProp ?? ctx.blockedSessions; + const worktreeSessionMap = worktreeSessionMapProp ?? ctx.worktreeSessionMap; + const [worktrees, setWorktrees] = useState([]); const [loading, setLoading] = useState(false); const [creating, setCreating] = useState(false); diff --git a/src/contexts/SessionActivityContext.tsx b/src/contexts/SessionActivityContext.tsx new file mode 100644 index 0000000000..fb86b0fa99 --- /dev/null +++ b/src/contexts/SessionActivityContext.tsx @@ -0,0 +1,67 @@ +import { createContext, useContext, useMemo, type ReactNode } from 'react'; + +/** + * Cross-cutting visibility into per-session protection state. Exists so deeply + * nested components like WorktreeList can light up activity dots without + * threading props through SidebarProjectItem (which is a no-edit churn file + * per docs/CLAUDE.md). The provider lives near the root in AppContent and is + * fed by the same `useSessionProtection` sets that drive ChatInterface. + */ +export type SessionActivityValue = { + activeSessions: Set; + processingSessions: Set; + blockedSessions: Set; + /** + * Optional map of worktree absolute path → sessionId currently running + * inside it. WorktreeList uses this to colour each worktree's dot. When + * unknown, callers should leave it empty and the dot will resolve to + * `idle`. + */ + worktreeSessionMap: Record; +}; + +const EMPTY_SET = new Set(); +const DEFAULT_VALUE: SessionActivityValue = { + activeSessions: EMPTY_SET, + processingSessions: EMPTY_SET, + blockedSessions: EMPTY_SET, + worktreeSessionMap: {}, +}; + +const SessionActivityContext = createContext(DEFAULT_VALUE); + +type ProviderProps = { + activeSessions?: Set; + processingSessions?: Set; + blockedSessions?: Set; + worktreeSessionMap?: Record; + children: ReactNode; +}; + +export function SessionActivityProvider({ + activeSessions, + processingSessions, + blockedSessions, + worktreeSessionMap, + children, +}: ProviderProps) { + const value = useMemo( + () => ({ + activeSessions: activeSessions ?? EMPTY_SET, + processingSessions: processingSessions ?? EMPTY_SET, + blockedSessions: blockedSessions ?? EMPTY_SET, + worktreeSessionMap: worktreeSessionMap ?? {}, + }), + [activeSessions, processingSessions, blockedSessions, worktreeSessionMap], + ); + + return ( + + {children} + + ); +} + +export function useSessionActivity(): SessionActivityValue { + return useContext(SessionActivityContext); +} From 18e31906fe04eb41e40154cee1d171ddd22efb4c Mon Sep 17 00:00:00 2001 From: Gaige Roberson Date: Thu, 23 Apr 2026 06:56:43 -0500 Subject: [PATCH 4/4] docs(review): track post-merge nice-to-haves in follow-ups.md Captures every NICE-TO-HAVE finding from the four phase reviews so the items aren't lost. Entries are grouped by phase with file:line citations; each is small enough to land in a polish PR or fold into the next phase. --- docs/follow-ups.md | 72 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 docs/follow-ups.md diff --git a/docs/follow-ups.md b/docs/follow-ups.md new file mode 100644 index 0000000000..49894df8d7 --- /dev/null +++ b/docs/follow-ups.md @@ -0,0 +1,72 @@ +# Post-merge follow-ups + +Tracking nice-to-have items surfaced by the post-merge review pass on +`review/post-merge-fixes`. None block production; each can be picked up by +the next phase or a polish PR. Citations point at file:line in the merged +commit; line numbers may drift after the review PR lands. + +## Phase 1 — Midnight skin + mobile-first + +- **Tasks slot missing from primary nav** — `useAppNavItems.ts:18-24` defines + five slots (Chat / Sessions / Preview / Browser / More). `AppContent.tsx:202` + honors `activeTab === 'tasks'` for accent, but no rail/tab-bar entry surfaces + the kanban view. The floating "Tasks" button in `MainContent.tsx` is the + only entry point. Decide: surface as a sixth slot or accept floating-only. +- **Sheet initial focus** — `MobileSidebarSheet.tsx:65` opens with no initial + focus target, so screen readers land on the trigger. Move focus to the sheet + container or first interactive child on open. +- **Drag-handle hit area** — `MobileSidebarSheet.tsx:84-92` is `pt-3 pb-2` + (~28px). Below the 44×44 minimum the rest of the app respects. +- **`visualViewport` listener cleanup** — `AppContent.tsx:128-137` registers + once; consider also listening to `scroll` (iOS sometimes only fires that on + keyboard show) and clearing `--keyboard-height` if `vv` becomes undefined. + +## Phase 2 — Sidebar tree + repo grouping + +- **Native `prompt`/`confirm` for topic rename/delete** — + `SidebarTopicGroup.tsx:138`. Works but jarring vs. Midnight; replace with + inline editable chip + custom modal. +- **Mobile search-scope segment uses ad-hoc styles** — `SidebarHeader.tsx` + mobile branch (~line 240). Desktop got the `.ds-segment` polish; apply the + same to mobile. +- **Aria mismatch for single-project repos** — `SidebarProjectTree.tsx:189`: + when no group header renders, `aria-labelledby={groupId}` points at a + nonexistent button. Drop the attr in that branch. +- **`crypto.randomUUID()` fallback** — `useTopicStorage.ts:106`. Add a + `Math.random()`-based fallback for insecure-context resilience. +- **Repo-grouper cache invalidation** — `repo-grouper.js:333` keys by + `fullPath` only. Add a TTL or git-mtime check so origin URL changes are + picked up without manual cache deletion. + +## Phase 5 — Preview + Chrome + Worktrees + Tasks + +- **Worktree → session map is partial** — only the *currently selected* + worktree's dot can light up. Cross-worktree resolution requires the + server-side activity stream to know which sessionId is running in each + `/.claude/worktrees/` cwd. Add `/api/worktrees/active-sessions` + (or include `__cwd` on every session payload) and feed it into + `SessionActivityProvider` in `AppContent.tsx`. +- **Tasks aside is fixed-width on desktop** — `MainContent.tsx:194-208` uses + `w-[380px]`. The phase-5 brief asks for a resizable right panel. +- **Preview proxy CSP/X-Frame strip is unconditional** — + `preview-proxy.js:79-81` strips CSP and X-Frame-Options on every response. + In production deployments that's broader than needed — consider gating to + `process.env.NODE_ENV !== 'production'` or to the configured port allowlist. +- **`SpawnSubAgentButton` SSE event narrowing** — `SpawnSubAgentButton.tsx:127` + casts `evt.event` straight to `StreamEvent['type']`, so a malformed server + event would silently produce a garbage payload. Narrow with a `switch`. +- **`SessionFilesTouchedChips` layout shift** — `SessionFilesTouchedChips.tsx:88` + reserves no height before chips arrive; once the IntersectionObserver + resolves, the row jumps. Reserve a min-height. +- **`spawn-sub-agent` has no time/output cap** — `mcp-bootstrap.js:259`. A + runaway sub-agent could stream forever. Add a hard timeout + byte cap. + +## Cross-cutting + +- **Bundle size warning** — main client chunk is 2.5 MB minified (~760 KB + gzipped). Vite warns above 1 MB. Worth a code-split pass: lazy-load + Preview/Browser/Tasks panes, split CodeMirror/xterm vendor chunks further. +- **No `npm test` script** — Phase briefs reference test runs but + `package.json` has none. Add at minimum a Vitest harness with smoke tests + for the new server routes (preview-proxy port allowlist, mcp-bootstrap + workingDir validation, tasks path-traversal guard).