Conversation
Mount Express middleware on the daemon's HTTP server to serve the Vite-built React app at /ui and expose GET /api/ui/config for browser bootstrap. Add build:ui and dev:ui npm scripts.
Add transport layer for the web UI: BrvApiClient, Zustand transport store, connection bootstrap (fetch /api/ui/config + socket.io-client), TransportProvider, and React Query config. Add brv webui command to discover the daemon and open the browser.
… panels Add a complete browser-based dashboard at /ui that mirrors the TUI's feature set. The web UI connects to the daemon via Socket.IO, uses the same transport events and API patterns as the TUI, and imports shared UI components from @campfirein/byterover-packages (Card, Badge, Button, Dialog, Table, Tabs, Input). Key changes: - Auth system with OAuth login flow, auth guard, and session management - 11 feature panels: status, providers, models, connectors, hub, push, pull, spaces, session, reset, analytics (placeholder) - Sidebar navigation with grouped routes and connection state badge - Tailwind CSS v4 with shadcn/ui design tokens and dark mode - CSP security headers on the daemon's Express middleware - 'webui' client type added to VALID_CLIENT_TYPES for daemon registration - Vite config with @workspace/ui alias, React dedup for npm link, and @source scanning for package component classes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…anel UI Fix hasMcpToolsInBrvSection using new Set() which created a character set instead of checking substrings — MCP tool detection always returned false, causing rules connector to appear installed after switching to MCP/skill. Changed to string.includes() for correct substring matching. Connectors panel improvements: - Add agent search with deferred filtering - Move connector type tabs into Supported agents card - Scrollable agent list with overflow constraint - Alert component for dismissable feedback messages - Combined restart warning into single feedback call - Explicit refetchConnectors after install to avoid stale data - Main layout max-h-screen for proper height propagation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Feat/local web UI sync main
feat: sync main
Update vite config and tsconfig to resolve shared UI from ui/src/ instead of src/, matching the actual repo layout.
feat(ENG-2045): add submodule-based webui local development flow
Separate the web UI from the daemon's dynamic transport port onto a fixed port (default 7700) so users can bookmark http://localhost:7700. Add PWA with Service Worker so the app loads from cache even when the daemon is down, showing a 'Run Command "webui" not found Run "brv --help" to see available commands to start' screen. - Add WebUiServer on stable port, keep Socket.IO transport on dynamic port - Browser connects Socket.IO cross-origin to daemon via /api/ui/config - Add vite-plugin-pwa for offline caching with workbox - Add Command "webui" not found Run "brv --help" to see available commands flag to set and persist preferred port - Add webui:getPort / webui:setPort transport endpoints - Remove /ui base path (served at root since dedicated port) - Review URLs use webui port instead of transport port - Graceful fallback when webui port is occupied (daemon still runs)
feat(ENG-2045): split web UI onto stable port with PWA offline support
…onfustion. Use bundledDependencies instead.
…eDependencies (auto-generated when npm install).
…ith availability of package-lock.json in their repo.
…update package-lock.json.
…n) to fix lint issue.
Feat/eng 2074
- Tree browse with expand/collapse, file detail with markdown rendering - File editing with save, version history with cursor-based pagination - 3-panel layout (tree, detail, history) with collapsible panels - Breadcrumb navigation, author timeline, folder metadata - URL-based path/branch sync for bookmarking - Remove auth guard for local web access
feat: [ENG-1505] add context tree management to local web UI
feat: [ENG-2057] add web UI project selection with sidebar project dropdown.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }) | ||
|
|
||
| // Config endpoint for browser to bootstrap Socket.IO connection | ||
| app.get('/api/ui/config', (_req, res) => { |
There was a problem hiding this comment.
issue (security): /api/ui/config still has no CORS restriction — unfixed across eight reviews
Any page open in the same browser can fetch('http://localhost:7700/api/ui/config') and silently read daemonPort, projectCwd, and version, then use those to open a privileged Socket.IO connection to the daemon. Eight review cycles, zero action.
| app.get('/api/ui/config', (_req, res) => { | |
| app.get('/api/ui/config', (_req, res) => { | |
| res.setHeader('Access-Control-Allow-Origin', 'null') // block cross-origin fetches | |
| res.json(getConfig()) | |
| }) |
Or add cors({origin: false}) middleware on all /api/* routes. Either way, this must be fixed before merge.
| // Stop existing webui server if running | ||
| if (webuiServer?.isRunning()) { | ||
| await webuiServer.stop() | ||
| log(`Stopped web UI server on port ${webuiServer.getPort() ?? '?'}`) |
There was a problem hiding this comment.
issue (robustness): three problems remain here — unfixed across eight reviews
1. Port logged as "?": WebUiServer.stop() clears this.port before resolving. By the time line 527 runs, getPort() returns undefined, so the log always prints "Stopped web UI server on port ?". Capture the port before calling stop():
const oldPort = webuiServer.getPort()
await webuiServer.stop()
log(`Stopped web UI server on port ${oldPort ?? '?'}`)2. No port validation: data.port has no bounds check — 0, negative, or >65535 reaches start() after the old server is already down. Add before the stop call:
if (!Number.isInteger(newPort) || newPort < 1 || newPort > 65535) {
return {port: webuiServer?.getPort() ?? WEBUI_DEFAULT_PORT, success: false}
}3. Stop-before-start availability gap: If start(newPort) throws (port busy), no server is running. Start the new server first, only stop the old after new is confirmed up.
|
|
||
| // Guard against path traversal | ||
| const resolved = relative(contextTreeDir, fullPath) | ||
| if (resolved.startsWith('..') || resolved.startsWith('/')) { |
There was a problem hiding this comment.
issue (correctness): empty data.path still bypasses traversal guard — unfixed across eight reviews
data.path === '' resolves join(contextTreeDir, '') to contextTreeDir itself. relative(contextTreeDir, contextTreeDir) returns '', which doesn't start with .. or /, so the guard passes and writeFile is called on a directory, throwing EISDIR.
| if (resolved.startsWith('..') || resolved.startsWith('/')) { | |
| if (!data.path || resolved.startsWith('..') || resolved.startsWith('/')) { |
| port: Flags.integer({ | ||
| char: 'p', | ||
| description: 'Set the web UI port (remembered for future use)', | ||
| }), |
There was a problem hiding this comment.
issue (robustness): --port flag still has no range constraint — unfixed from review #2
Flags.integer with no min/max lets brv webui -p 0 or brv webui -p 70000 reach the daemon unvalidated. Combined with the daemon's stop-before-start ordering, any out-of-range value permanently disables the web UI until daemon restart.
| }), | |
| port: Flags.integer({ | |
| char: 'p', | |
| description: 'Set the web UI port (remembered for future use)', | |
| max: 65535, | |
| min: 1, | |
| }), |
| ERR_VC_NON_FAST_FORWARD: 'The remote has changes. Pull first, then try again.', | ||
| ERR_VC_NOTHING_TO_PUSH: 'Nothing to push — stage and commit your changes first.', | ||
| ERR_VC_REMOTE_ALREADY_EXISTS: "A remote named 'origin' already exists. Remove or rename it before adding a new one.", | ||
| ERR_VC_USER_NOT_CONFIGURED: 'Set commit author via `brv vc config` before committing.', |
There was a problem hiding this comment.
issue (regression): ERR_VC_USER_NOT_CONFIGURED override still references the CLI — unfixed from review #7
This message references brv vc config, a CLI command the web UI user cannot run. Use neutral copy that works in both contexts:
| ERR_VC_USER_NOT_CONFIGURED: 'Set commit author via `brv vc config` before committing.', | |
| ERR_VC_USER_NOT_CONFIGURED: 'Configure your commit author name and email before committing.', |
…text Project name and display path now recognise backslash-separated Windows paths alongside macOS and Linux home directories, each with a unit-tested suite covering absolute paths, trailing separators, UNC shares, and degenerate inputs. The shared formatError helper accepts an optional ErrorContext so overrides can produce tailored copy — ERR_VC_USER_NOT_CONFIGURED now embeds the project path and tells the user exactly where to run brv vc config. Changes panel threads the selected project through to the three commit-related error sites.
Drop the redundant length check in getProjectName in favour of a straight falsy-fallback, swap undefined fallback arguments in the error-messages tests for a named string so intent is obvious, and add a UNC-share test to pin displayPath's "unchanged when no drive letter" contract.
feat: [ENG-2167] cross-platform path handling and richer VC error con…
|
Claude finished @ncnthien's task in 3m 24s —— View job PR Review: Local Web UI (Re-review #9)
What's new since review #8One PR merged since the last review cycle:
The new code adds cross-platform path utilities ( Prior issues — status (9 reviews in)
Issues by priority🔴 SecurityIssue 1 — CORS on Still no 🟡 Correctness / RobustnessIssues 2+3+4 — Three separate bugs in the same window, present since review #1:
Issue 5 — Empty path traversal (
Issue 11 —
Issue 12 —
Issue 14 — The context-aware rewrite (adding 🔵 Maintainability / YAGNI
New code review (PR #491) ✅ with one note
Score: 3 / 5Unchanged for the ninth time. PR #491 is solid work — the cross-platform path utilities are correct and well-tested, and the context-aware error message infrastructure is a clean abstraction. But the same four blocking issues flagged in review #1 — CORS exposure on the config endpoint ( |
| }) | ||
|
|
||
| // Config endpoint for browser to bootstrap Socket.IO connection | ||
| app.get('/api/ui/config', (_req, res) => { |
There was a problem hiding this comment.
issue (security): /api/ui/config still has no CORS restriction — unfixed across nine reviews
Any page open in the same browser can fetch('http://localhost:7700/api/ui/config') and silently read daemonPort, projectCwd, and version, then use those to open a privileged Socket.IO connection to the daemon. Nine review cycles, zero action.
| app.get('/api/ui/config', (_req, res) => { | |
| app.get('/api/ui/config', (_req, res) => { | |
| res.setHeader('Access-Control-Allow-Origin', 'null') // block cross-origin fetches | |
| res.json(getConfig()) | |
| }) |
Or add cors({origin: false}) middleware on all /api/* routes. Either way, this must be fixed before merge.
| // Stop existing webui server if running | ||
| if (webuiServer?.isRunning()) { | ||
| await webuiServer.stop() | ||
| log(`Stopped web UI server on port ${webuiServer.getPort() ?? '?'}`) |
There was a problem hiding this comment.
issue (robustness): three problems remain here — unfixed across nine reviews
1. Port logged as "?": WebUiServer.stop() sets this.port = undefined before resolving (see webui-server.ts:62). By the time this line runs, getPort() returns undefined, so the log always prints "Stopped web UI server on port ?". Capture the port before calling stop().
2. No port validation: data.port has no bounds check — 0, negative, or >65535 reaches start() after the old server is already down.
3. Stop-before-start availability gap: If start(newPort) throws (port busy), no server is running.
Suggested fix for the port-log bug:
| log(`Stopped web UI server on port ${webuiServer.getPort() ?? '?'}`) | |
| log(`Stopped web UI server on port ${webuiServer.getPort() ?? '?'}`) |
Wait — the line is already like this but the port is already cleared by the time we get here. The real fix is to capture the port before stopping:
const oldPort = webuiServer.getPort()
await webuiServer.stop()
log(`Stopped web UI server on port ${oldPort ?? '?'}`)And invert the start/stop ordering:
const newWebuiServer = new WebUiServer(newApp)
await newWebuiServer.start(newPort) // start first — if it fails, old server still runs
const oldPort = webuiServer?.getPort()
if (webuiServer?.isRunning()) await webuiServer.stop()
webuiServer = newWebuiServer
log(`Moved web UI server from port ${oldPort ?? '?'} to ${newPort}`)Also add port validation before any of this:
if (!Number.isInteger(newPort) || newPort < 1 || newPort > 65535) {
return {port: webuiServer?.getPort() ?? WEBUI_DEFAULT_PORT, success: false}
}
|
|
||
| // Guard against path traversal | ||
| const resolved = relative(contextTreeDir, fullPath) | ||
| if (resolved.startsWith('..') || resolved.startsWith('/')) { |
There was a problem hiding this comment.
issue (correctness): empty data.path still bypasses traversal guard — unfixed across nine reviews
data.path === '' resolves join(contextTreeDir, '') to contextTreeDir itself. relative(contextTreeDir, contextTreeDir) returns '', which doesn't start with .. or /, so the guard passes and writeFile is called on a directory, throwing EISDIR.
| if (resolved.startsWith('..') || resolved.startsWith('/')) { | |
| if (!data.path || resolved.startsWith('..') || resolved.startsWith('/')) { |
| port: Flags.integer({ | ||
| char: 'p', | ||
| description: 'Set the web UI port (remembered for future use)', | ||
| }), |
There was a problem hiding this comment.
issue (robustness): --port flag still has no range constraint — unfixed from review #2
Flags.integer with no min/max lets brv webui -p 0 or brv webui -p 70000 reach the daemon unvalidated. Combined with the daemon's stop-before-start ordering, any out-of-range value permanently disables the web UI until daemon restart.
| }), | |
| port: Flags.integer({ | |
| char: 'p', | |
| description: 'Set the web UI port (remembered for future use)', | |
| max: 65535, | |
| min: 1, | |
| }), |
| ERR_VC_USER_NOT_CONFIGURED: ({projectPath}) => | ||
| projectPath | ||
| ? `Please run \`brv vc config\` in "${projectPath}" to set your commit author before committing.` | ||
| : 'Please run `brv vc config` inside your project to set commit author before committing.', |
There was a problem hiding this comment.
issue (UX): ERR_VC_USER_NOT_CONFIGURED still references brv vc config — unfixed from review #7, partially addressed
The context-aware rewrite (adding projectPath to the message) is an improvement — the user now knows which project to configure. But brv vc config is still a CLI command the web UI user cannot run directly from the browser. Consider replacing the CLI instruction with a link to the Configuration tab/page where the user can set their author name and email via the web UI itself:
| : 'Please run `brv vc config` inside your project to set commit author before committing.', | |
| ERR_VC_USER_NOT_CONFIGURED: ({projectPath}) => | |
| projectPath | |
| ? `Commit author is not configured for "${projectPath}". Set your name and email in the Configuration page.` | |
| : 'Commit author is not configured. Set your name and email in the Configuration page.', |
If the Configuration page does not yet expose VC author settings, neutral copy like 'Configure your commit author name and email before committing.' is still better than directing the user to a command they cannot run from the browser.
The daemon's state:getProjectConfig handler now also reads the context-tree git remote (defaulting to origin) off disk and returns it alongside the existing project config, running in parallel with the config load so the handler's latency is unchanged. The web UI's project dropdown drops buildRemoteSpaceUrl + the webAppUrl prop threading in favour of reading remoteUrl directly from the config response, so "Open Remote space" now opens whatever the git remote actually points at.
Pull readContextTreeRemoteUrl out of brv-server.ts into its own module under server/infra/context-tree so the daemon entry file stays a thin wiring layer and the helper gets real unit coverage (happy path, missing git repo, no remote configured, non-default remote name, missing project path). The project dropdown now refuses to open a remote URL that isn't http(s) — SSH remotes aren't browser-openable anyway, and guarding the scheme closes the window where a tampered .git/config could smuggle a javascript: or file: URI into window.open.
feat: [ENG-2167] expose context-tree remote URL via project config
|
Claude finished @ncnthien's task in 3m 59s —— View job PR Review: Local Web UI (Re-review #10)
What's new since review #9Two commits merged since the last review cycle (PR #498):
The new code adds Prior issues — status (10 reviews in)
Issues by priority🔴 SecurityIssue 1 — CORS on Still no 🟡 Correctness / RobustnessIssues 2+3+4 — Three separate bugs in the same window, present since review #1:
Issue 5 — Empty path traversal (
Issue 11 —
Issue 12 —
Issue 14 — Still references 🔵 Maintainability / YAGNI
New code review (PR #498) ✅
Score: 3 / 5Unchanged for the tenth time. PR #498 is clean and well-tested — the context-tree remote URL approach is simpler and more direct than the |
| }) | ||
|
|
||
| // Config endpoint for browser to bootstrap Socket.IO connection | ||
| app.get('/api/ui/config', (_req, res) => { |
There was a problem hiding this comment.
issue (security): /api/ui/config still has no CORS restriction — unfixed across ten reviews
Any page open in the same browser can fetch('http://localhost:7700/api/ui/config') and silently read daemonPort, projectCwd, and version, then use those to open a privileged Socket.IO connection to the daemon. Ten review cycles, zero action.
| app.get('/api/ui/config', (_req, res) => { | |
| app.get('/api/ui/config', (_req, res) => { | |
| res.setHeader('Access-Control-Allow-Origin', 'null') // block cross-origin fetches | |
| res.json(getConfig()) | |
| }) |
Or add cors({origin: false}) middleware on all /api/* routes. Either way, this must be fixed before merge.
| // Stop existing webui server if running | ||
| if (webuiServer?.isRunning()) { | ||
| await webuiServer.stop() | ||
| log(`Stopped web UI server on port ${webuiServer.getPort() ?? '?'}`) |
There was a problem hiding this comment.
issue (robustness): three problems remain here — unfixed across ten reviews
1. Port logged as "?": WebUiServer.stop() sets this.port = undefined before resolving. By the time line 532 runs, getPort() returns undefined. Capture the port before calling stop():
const oldPort = webuiServer.getPort()
await webuiServer.stop()
log(`Stopped web UI server on port ${oldPort ?? '?'}`)2. No port validation: data.port has no bounds check — 0, negative, or >65535 reaches start() after the old server is already down. Add before the stop call:
if (!Number.isInteger(newPort) || newPort < 1 || newPort > 65535) {
return {port: webuiServer?.getPort() ?? WEBUI_DEFAULT_PORT, success: false}
}3. Stop-before-start availability gap: If start(newPort) throws (port busy), no server is running. Start the new server first, only stop the old after new is confirmed up.
|
|
||
| // Guard against path traversal | ||
| const resolved = relative(contextTreeDir, fullPath) | ||
| if (resolved.startsWith('..') || resolved.startsWith('/')) { |
There was a problem hiding this comment.
issue (correctness): empty data.path still bypasses traversal guard — unfixed across ten reviews
data.path === '' resolves join(contextTreeDir, '') to contextTreeDir itself. relative(contextTreeDir, contextTreeDir) returns '', which doesn't start with .. or /, so the guard passes and writeFile is called on a directory, throwing EISDIR.
| if (resolved.startsWith('..') || resolved.startsWith('/')) { | |
| if (!data.path || resolved.startsWith('..') || resolved.startsWith('/')) { |
| port: Flags.integer({ | ||
| char: 'p', | ||
| description: 'Set the web UI port (remembered for future use)', | ||
| }), |
There was a problem hiding this comment.
issue (robustness): --port flag still has no range constraint — unfixed from review #2
Flags.integer with no min/max lets brv webui -p 0 or brv webui -p 70000 reach the daemon unvalidated. Combined with the daemon's stop-before-start ordering, any out-of-range value permanently disables the web UI until daemon restart.
| }), | |
| port: Flags.integer({ | |
| char: 'p', | |
| description: 'Set the web UI port (remembered for future use)', | |
| max: 65535, | |
| min: 1, | |
| }), |
| ERR_VC_USER_NOT_CONFIGURED: ({projectPath}) => | ||
| projectPath | ||
| ? `Please run \`brv vc config\` in "${projectPath}" to set your commit author before committing.` | ||
| : 'Please run `brv vc config` inside your project to set commit author before committing.', |
There was a problem hiding this comment.
issue (UX): ERR_VC_USER_NOT_CONFIGURED still references brv vc config — unfixed from review #7
brv vc config is a CLI command that web UI users cannot run from the browser. Use neutral copy that works in both contexts:
| : 'Please run `brv vc config` inside your project to set commit author before committing.', | |
| ? `Commit author is not configured for "${projectPath}". Set your name and email in the Configuration page.` | |
| : 'Commit author is not configured. Set your name and email in the Configuration page.', |
If the Configuration page does not yet expose VC author settings, 'Configure your commit author name and email before committing.' is still better than directing users to a CLI command they cannot run.
|
|
||
| export function buildRemoteSpaceUrl({spaceName, teamName, webAppUrl}: BuildRemoteSpaceUrlInput): string | undefined { | ||
| if (!teamName || !spaceName || !webAppUrl) return undefined | ||
| const base = webAppUrl.replace(/\/+$/, '') |
There was a problem hiding this comment.
suggestion (cleanup): buildRemoteSpaceUrl is now dead code after PR #498
project-dropdown.tsx no longer calls buildRemoteSpaceUrl — it now reads projectConfig.remoteUrl directly. Neither this file nor get-environment-config.ts (useGetEnvironmentConfig) has any remaining import in the codebase. Both should be deleted to avoid the impression that the buildRemoteSpaceUrl + BRV_WEB_APP_URL path is still the intended mechanism.
5 participants
Summary
Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
User-visible changes
List user-visible changes (including defaults, config, or CLI output).
If none, write
None.Evidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
List real risks for this PR. If none, write
None.