feat: Improve UX across Beads, terminal, and settings#12
feat: Improve UX across Beads, terminal, and settings#12
Conversation
- Fix property name mismatches in hook parameters (_currentProject, _loadIssues) - Update drag event type compatibility for @dnd-kit/core - Add proper DragStartEvent and DragEndEvent type imports - Add validation, rate limiting, and JSON parsing middleware - Add unit tests for beads service and utilities 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit addresses all issues raised in the code review for PR #11: **Code Quality Improvements:** - Remove unused type imports from beads-service.test.ts - Remove unused _loadIssues parameter from useBeadsActions hook - Remove unused _currentProject parameter from useBeadsColumnIssues hook - Remove unused loadIssues variable from beads-view.tsx **Performance Optimization:** - Memoize getBlockingCounts calculation in BeadsKanbanBoard to avoid O(n²) complexity - Use useMemo to cache blocking counts map and recalculate only when issues change **Documentation Improvements:** - Update json-parser.ts documentation to clarify that type parameter is for TypeScript casting only, not runtime validation - Update BEADS_AUDIT_REPORT.md to reflect that basic unit tests have been added **Security Enhancements:** - Apply strictLimiter (5 req/min) to /api/setup endpoint - Apply strictLimiter (5 req/min) to /api/settings endpoint - These sensitive endpoints now have stricter rate limiting **Validation Improvements:** - Add refinement to listBeadsIssuesFiltersSchema to ensure priorityMin <= priorityMax - Adds clear error message when priority range is invalid **Feature Completeness:** - Add parentIssueId support to BeadsService.createIssue method - Pass --parent flag to bd CLI when parentIssueId is provided - Add parentIssueId validation to createBeadsIssueSchema All changes pass ESLint with no warnings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…dling The "should handle paths without trailing slash" test was a duplicate of the previous test. Updated it to actually test trailing slash handling: - Changed input path from '/my/project' to '/my/project/' - Keeps expected output as '/my/project/.beads/beads.db' since path.join() automatically normalizes trailing slashes - This now properly verifies that getDatabasePath correctly handles paths with trailing slashes All 3 unit tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ardize GitHub CLI PATH This commit resolves three interconnected issues identified through comprehensive agent research and tracked via Beads issues DevFlow-iyo, DevFlow-55v, DevFlow-xh4. **Beads API Routes (DevFlow-iyo)**: - Register 3 missing API routes: GET /show/:id, POST /connect, POST /sync - Fix validation regex bug: add missing quantifier and closing bracket - Fix database path inconsistency: data.db → beads.db **Claude CLI Installation (DevFlow-55v)**: - Add retry logic with exponential backoff (4 retries, 3s→10.5s delays) - Increase initial PATH wait time from 2s to 3s - Add detailed console logging for debugging installation issues **GitHub CLI PATH Configuration (DevFlow-xh4)**: - Create centralized github-cli-path.ts utility - Add Windows support (Git, GitHub CLI, Scoop paths) - Use proper path separators for each platform (: vs ;) - Update 3 files to use centralized configuration All quality checks passed: zero linting errors, zero TypeScript errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Enhance Beads integration with better diagnostics and error handling. Improve terminal connection reliability with WebSocket error handling. Refine UI styling with consistent scrollbars across themes. Add settings navigation visual improvements and CLI installation feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds server-side auth initialization, CORS validation, per-route rate limiting, Zod validation and validation middleware, JSON parsing helpers, GitHub CLI env utilities, expanded Beads service APIs and routes, UI Beads diagnostics and column logic, CLI install flow, tests, type adjustments, scripts, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Express as API Server
participant Validation as Zod Validator
participant Auth as AuthMiddleware
participant RateLimit as RateLimiter
participant BeadsSvc as BeadsService
participant BeadsCLI as Beads CLI / DB
Client->>Express: POST /api/beads/create (body)
Note right of Express: Global middlewares run
Express->>RateLimit: beadsLimiter checks quota
RateLimit-->>Express: allow
Express->>Auth: verify request auth
Auth-->>Express: authenticated
Express->>Validation: validateBody(createBeadsIssueSchema)
Validation-->>Express: parsed payload
Express->>BeadsSvc: createIssue(projectPath, parsedPayload)
BeadsSvc->>BeadsCLI: spawn CLI / read/write DB
BeadsCLI-->>BeadsSvc: stdout JSON
BeadsSvc->>Express: created BeadsIssue
Express-->>Client: 200 { success: true, issue }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @0xtsotsi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant overhaul to the user experience, security, and underlying robustness of the application, particularly focusing on the Beads Kanban board, terminal functionality, and server-side API interactions. Key improvements include the introduction of a detailed audit for the Beads feature, followed by the implementation of robust API validation and rate limiting to enhance security and data integrity. The PR also refines CLI tool management and installation processes, notably for the Claude CLI, and provides more informative error diagnostics for both Beads and the terminal, making troubleshooting easier for users. Overall, these changes aim to create a more stable, secure, and user-friendly development environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the Beads feature, focusing on security, robustness, and user experience, largely in response to a comprehensive audit report. Key changes include updating Beads CLI version to 0.35.0, adding a detailed BEADS_AUDIT_REPORT.md outlining critical and high-priority issues, and integrating express-rate-limit and zod for enhanced API security and input validation. The server now enforces API key authentication in production, validates CORS origins, and applies rate limiting across various API routes, including specific limits for Beads endpoints. New utility files beads-validation.ts, json-parser.ts, rate-limiter.ts, and validation-middleware.ts were added to standardize input validation, robust JSON parsing, and rate limiting. The BeadsService was refactored to use these new validation and parsing utilities, improve error handling (including a new BeadsCliError class), and introduce new functionalities like searchIssues, getBlockedIssues, and getStaleIssues, while also fixing a race condition in watchDatabase. The UI for Beads was enhanced with a new BeadsErrorDiagnostics component to provide better feedback on CLI and database issues, and the drag-and-drop logic was refined for more accurate column detection. Additionally, the Claude CLI installation process was automated and made more robust, and general UI components received minor accessibility and styling adjustments. A review comment highlighted an inconsistency in updateBeadsIssueSchema where a regex limited the title to 50 characters despite a schema validation allowing 200, which was suggested to be fixed by removing the length constraint from the regex. Another comment noted an unused verified variable in the Claude CLI installation logic, recommending its removal.
| .string() | ||
| .min(1, 'Title is required') | ||
| .max(200, 'Title must be 200 characters or less') | ||
| .regex(/^[^<>{}$]{1,50}$/, 'Title contains invalid characters') |
There was a problem hiding this comment.
The regex ^[^<>{}$]{1,50}$ for the title in updateBeadsIssueSchema is inconsistent with the .max(200, ...) validation on the preceding line. This regex limits the title to 50 characters, while the schema intends to allow up to 200. This also differs from createBeadsIssueSchema. To fix this and maintain consistency, the regex should not enforce a length limit, as the length is already handled by .min() and .max().
| .regex(/^[^<>{}$]{1,50}$/, 'Title contains invalid characters') | |
| .regex(/^[^<>{}$]*$/, 'Title contains invalid characters') |
|
|
||
| // Retry verification with exponential backoff (max 4 retries) | ||
| const maxRetries = 4; | ||
| let verified = false; |
There was a problem hiding this comment.
The verified variable is assigned a value but is never used. The function returns immediately upon successful verification, and this variable is not checked in the fallthrough case. It can be safely removed.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
apps/server/src/lib/rate-limiter.ts
Outdated
| let rateLimit: RateLimit; | ||
|
|
||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| rateLimit = require('express-rate-limit'); |
There was a problem hiding this comment.
Because the server is ESM (apps/server/package.json has "type": "module" and TS uses NodeNext), require is undefined at runtime. That means this try block throws every time, the catch path is taken, and all limiters become no-ops even though express-rate-limit is installed. As a result, the new rate limiting is silently disabled in production and dev. Use import() or createRequire so the module loads under ESM and the limits actually apply.
Useful? React with 👍 / 👎.
|
|
||
| if (!corsOrigin) { | ||
| console.warn('[CORS] No CORS_ORIGIN set, using localhost default'); | ||
| return 'http://localhost:3008'; | ||
| } |
There was a problem hiding this comment.
When CORS_ORIGIN is unset, the server now defaults to http://localhost:3008. The README documents the UI default as http://localhost:3000, so a fresh dev setup will hit CORS failures for browser requests unless an env var is added. This is a behavioral regression from the previous '*' default and breaks the documented workflow. Consider defaulting to http://localhost:3000 (or allowing a list) to keep the out‑of‑the‑box dev flow working.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/beads-view/hooks/use-beads-issues.ts (1)
24-35: Remove unused prevProjectPathRef.The
prevProjectPathRefis declared (line 25) and assigned (line 35) but never read. Since the project-switch logic was simplified, this ref is no longer needed and should be removed.🔎 Proposed cleanup
export function useBeadsIssues({ currentProject }: UseBeadsIssuesProps) { const { beadsByProject, setBeadsIssues } = useAppStore(); const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState<string | null>(null); - // Track previous project path to detect project switches - const prevProjectPathRef = useRef<string | null>(null); const isInitialLoadRef = useRef(true); // Load issues using beads API const loadIssues = useCallback(async () => { if (!currentProject) return; - const currentPath = currentProject.path; - - // Update the ref to track current project - prevProjectPathRef.current = currentPath; + const currentPath = currentProject.path; // Only show loading spinner on initial loadapps/server/src/services/beads-service.ts (1)
31-38: Use cross-platform approach for detecting CLI path—Beads CLI supports Windows.Both
isBeadsInstalled()andgetBeadsCliPath()use thewhichcommand, which doesn't exist on Windows. Since Beads officially supports Windows with prebuilt binaries, use the npmwhichpackage or detect the platform and usewhereon Windows andwhichon Unix systems.Affects lines 31-38 and 90-97.
🧹 Nitpick comments (12)
apps/ui/src/components/views/beads-view/beads-kanban-board.tsx (1)
67-88: Good performance optimization with correct logic.The memoization successfully avoids O(n²) recalculations on each render. The blocking count logic is correct:
blockingCounttracks issues that depend on the current issue, whileblockedCounttracks open/in-progress blockers.For maximum effectiveness, ensure the
issuesprop is memoized upstream (e.g., in the parent component). If theissuesarray is recreated on every render, this memoization will recompute on each render anyway.apps/ui/src/components/views/terminal-view.tsx (1)
316-339: Extract actual port from serverUrl for accurate diagnostics.The diagnostic block hard-codes port 3008, but
serverUrlcan be configured viaVITE_SERVER_URLto use any port. Users with non-standard configurations will see incorrect guidance.🔎 Extract port dynamically from serverUrl
+ {/* Extract port from serverUrl for diagnostic accuracy */} + {(() => { + const portMatch = serverUrl.match(/:(\d+)/); + const actualPort = portMatch ? portMatch[1] : '3008'; + return ( <div className="text-xs text-muted-foreground max-w-md mb-4 text-left bg-muted/50 p-3 rounded-md"> <p className="font-medium mb-1">Diagnostics:</p> <ul className="list-disc list-inside space-y-1"> <li>Server URL: {serverUrl}</li> - <li>Server must be running on port 3008</li> + <li>Server must be running on port {actualPort}</li> <li> Check:{' '} <code className="px-1 py-0.5 rounded bg-background">TERMINAL_ENABLED=true</code> in server .env </li> <li> node-pty native module required. Run:{' '} <code className="px-1 py-0.5 rounded bg-background"> cd apps/server && npm install </code> </li> </ul> </div> + ); + })()}apps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsx (1)
118-143: Consider externalizing configuration values.The server URL (
localhost:3008), port, and package name (@beadscli/beads) are hardcoded. For maintainability, consider extracting these to a shared constants file, especially if they might change or need to be consistent across the codebase.apps/server/src/routes/setup/routes/install-claude.ts (1)
97-135: Comment inaccuracy: This is linear backoff, not exponential.The comment on line 129 mentions "Exponential backoff" but the formula
3000 + i * 2500produces a linear progression (3s, 5.5s, 8s, 10.5s). Consider either updating the comment or implementing true exponential backoff if that's the intent.🔎 Proposed fix for true exponential backoff
- // Exponential backoff: 3s, 5.5s, 8s, 10.5s + // Linear backoff: 3s, 5.5s, 8s, 10.5s if (i < maxRetries - 1) { const waitTime = 3000 + i * 2500;Or for actual exponential backoff:
- // Exponential backoff: 3s, 5.5s, 8s, 10.5s + // Exponential backoff: 1s, 2s, 4s, 8s if (i < maxRetries - 1) { - const waitTime = 3000 + i * 2500; + const waitTime = 1000 * Math.pow(2, i);apps/server/src/routes/beads/routes/create.ts (1)
23-45: Consider validating projectPath before running Zod validation.The current implementation validates the issue body first (lines 24-35), then checks for
projectPath(lines 37-42). Since checking forprojectPathis a cheaper operation, consider validating it first to fail fast before the more expensive Zod schema validation.🔎 Suggested optimization
export function createCreateHandler(beadsService: BeadsService) { return async (req: Request, res: Response): Promise<void> => { try { + // Validate projectPath first (cheaper check) + const { projectPath } = req.body as { projectPath: string }; + if (!projectPath) { + res.status(400).json({ success: false, error: 'projectPath is required' }); + return; + } + // Validate and parse request body using Zod schema const validationResult = createBeadsIssueSchema.safeParse(req.body.issue); if (!validationResult.success) { res.status(400).json({ success: false, error: 'Validation failed', details: validationResult.error.issues.map((issue) => ({ path: issue.path.join('.'), message: issue.message, })), }); return; } - const { projectPath } = req.body as { projectPath: string }; - - if (!projectPath) { - res.status(400).json({ success: false, error: 'projectPath is required' }); - return; - } - const issue = validationResult.data; const createdIssue = await beadsService.createIssue(projectPath, issue);apps/server/src/routes/beads/routes/list.ts (1)
24-49: Consider validating projectPath before filters validation.Similar to the create endpoint, validating
projectPathfirst (lines 26-29) before running the Zod schema validation on filters (lines 33-47) would provide faster failure for missing required fields.🔎 Suggested optimization
export function createListHandler(beadsService: BeadsService) { return async (req: Request, res: Response): Promise<void> => { try { const { projectPath } = req.body as { projectPath: string; filters?: unknown }; if (!projectPath) { res.status(400).json({ success: false, error: 'projectPath is required' }); return; } // Validate filters if provided let filters: Parameters<BeadsService['listIssues']>[1] | undefined = undefined; if (req.body.filters !== undefined) { const filtersResult = listBeadsIssuesFiltersSchema.safeParse(req.body.filters); if (!filtersResult.success) { res.status(400).json({ success: false, error: 'Invalid filters', details: filtersResult.error.issues.map((issue) => ({ path: issue.path.join('.'), message: issue.message, })), }); return; } filters = filtersResult.data; } const issues = await beadsService.listIssues(projectPath, filters);Note: The current code already does this correctly! The comment is to acknowledge the good ordering.
apps/server/tests/unit/services/beads-service.test.ts (1)
39-49: Testing private method via type assertion.The use of
as anyto access the private method is acceptable for unit testing internal logic. The test coverage is good with both positive and negative cases.If you find yourself needing to test private methods frequently, consider making them
protectedfor testability while keeping them internal.apps/server/src/routes/beads/routes/update.ts (1)
19-24: Validate projectPath more thoroughly.The
projectPathcheck only verifies it's truthy, but doesn't validate it's a string or meets any format requirements. Consider using a schema validator for consistency with the other fields.🔎 Proposed validation enhancement
+import { z } from 'zod'; + +const projectPathSchema = z.string().min(1, 'projectPath is required'); + export function createUpdateHandler(beadsService: BeadsService) { return async (req: Request, res: Response): Promise<void> => { try { - const { projectPath, issueId } = req.body as { projectPath: string; issueId: string }; + const { projectPath, issueId } = req.body; - if (!projectPath) { + const projectPathResult = projectPathSchema.safeParse(projectPath); + if (!projectPathResult.success) { res.status(400).json({ success: false, error: 'projectPath is required' }); return; }apps/server/src/lib/github-cli-path.ts (1)
12-59: Guard against undefined HOME environment variable.On Line 46,
process.env.HOMEis used in a template literal without checking if it exists. While it's typically present on Unix systems, it could be undefined in some environments, leading to the pathundefined/.local/bin.🔎 Proposed fix
} else { // Unix/Mac paths additionalPaths.push( '/opt/homebrew/bin', // Homebrew on Apple Silicon '/usr/local/bin', // Homebrew on Intel Mac, common Linux - '/home/linuxbrew/.linuxbrew/bin', // Linuxbrew - `${process.env.HOME}/.local/bin` // pipx, user-local installs + '/home/linuxbrew/.linuxbrew/bin' // Linuxbrew ); + if (process.env.HOME) { + additionalPaths.push(`${process.env.HOME}/.local/bin`); // pipx, user-local installs + } }apps/server/tests/unit/lib/beads-validation.test.ts (2)
6-21: Missing tests forremoveDependencySchema.The schema is imported (line 17) but has no corresponding test suite. Consider adding tests to verify both valid removal inputs and invalid issue ID rejection.
🔎 Suggested test addition
describe('removeDependencySchema', () => { it('should accept valid removal input', () => { const result = removeDependencySchema.safeParse({ issueId: 'bd-issue1', depId: 'bd-issue2', }); expect(result.success).toBe(true); }); it('should reject invalid issue IDs', () => { const result = removeDependencySchema.safeParse({ issueId: 'invalid', depId: 'bd-issue2', }); expect(result.success).toBe(false); }); });
265-271: Consider adding test for priorityMin > priorityMax refinement.The
listBeadsIssuesFiltersSchemahas a refinement that rejects whenpriorityMin > priorityMax, but there's no test coverage for this edge case.🔎 Suggested test addition
it('should reject priorityMin greater than priorityMax', () => { const result = listBeadsIssuesFiltersSchema.safeParse({ priorityMin: 3, priorityMax: 1, }); expect(result.success).toBe(false); if (!result.success) { expect(result.error.issues[0].message).toContain('priorityMin must be less than or equal'); } });apps/ui/src/components/views/beads-view/hooks/use-beads-column-issues.ts (1)
42-48: Consider defensive access for optional description field.While
BeadsIssue.descriptionis typed as required, theCreateBeadsIssueInput.descriptionis optional. If an issue is created without a description and the API returns an empty/undefined value, this could throw at runtime.🔎 Suggested defensive access
const filteredIssues = normalizedQuery ? issues.filter( (issue) => issue.title.toLowerCase().includes(normalizedQuery) || - issue.description.toLowerCase().includes(normalizedQuery) || + issue.description?.toLowerCase().includes(normalizedQuery) || issue.labels?.some((label) => label.toLowerCase().includes(normalizedQuery)) ) : issues;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.beads/beads.dbis excluded by!**/*.db.beads/daemon.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
.beads/.local_version.prettierignoreBEADS_AUDIT_REPORT.mdapps/server/package.jsonapps/server/src/index.tsapps/server/src/lib/auth.tsapps/server/src/lib/beads-validation.tsapps/server/src/lib/github-cli-path.tsapps/server/src/lib/json-parser.tsapps/server/src/lib/rate-limiter.tsapps/server/src/lib/validation-middleware.tsapps/server/src/routes/beads/client/cli-wrapper.tsapps/server/src/routes/beads/index.tsapps/server/src/routes/beads/routes/create.tsapps/server/src/routes/beads/routes/list.tsapps/server/src/routes/beads/routes/update.tsapps/server/src/routes/github/routes/common.tsapps/server/src/routes/setup/routes/gh-status.tsapps/server/src/routes/setup/routes/install-claude.tsapps/server/src/routes/worktree/common.tsapps/server/src/services/beads-service.tsapps/server/tests/unit/lib/beads-validation.test.tsapps/server/tests/unit/lib/json-parser.test.tsapps/server/tests/unit/services/beads-service.test.tsapps/ui/src/components/views/beads-view.tsxapps/ui/src/components/views/beads-view/beads-header.tsxapps/ui/src/components/views/beads-view/beads-kanban-board.tsxapps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsxapps/ui/src/components/views/beads-view/hooks/use-beads-actions.tsapps/ui/src/components/views/beads-view/hooks/use-beads-column-issues.tsapps/ui/src/components/views/beads-view/hooks/use-beads-drag-drop.tsapps/ui/src/components/views/beads-view/hooks/use-beads-issues.tsapps/ui/src/components/views/beads-view/lib/column-utils.tsapps/ui/src/components/views/board-view/kanban-board.tsxapps/ui/src/components/views/settings-view/components/settings-navigation.tsxapps/ui/src/components/views/setup-view/hooks/use-cli-installation.tsapps/ui/src/components/views/terminal-view.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/hooks/use-board-background-settings.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/beads.tsxapps/ui/src/styles/global.csslibs/types/src/beads.tspackage.json
🧰 Additional context used
📓 Path-based instructions (4)
apps/server/src/routes/**
📄 CodeRabbit inference engine (CLAUDE.md)
API routes should be placed in
apps/server/src/routes/, with one file per route/resource
Files:
apps/server/src/routes/beads/client/cli-wrapper.tsapps/server/src/routes/beads/routes/create.tsapps/server/src/routes/worktree/common.tsapps/server/src/routes/github/routes/common.tsapps/server/src/routes/setup/routes/install-claude.tsapps/server/src/routes/beads/index.tsapps/server/src/routes/beads/routes/update.tsapps/server/src/routes/setup/routes/gh-status.tsapps/server/src/routes/beads/routes/list.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Run type checking withnpm run typecheckbefore syncing the Beads database as part of quality gates
Run linting withnpm run lintbefore syncing the Beads database as part of quality gates
Files:
apps/server/src/routes/beads/client/cli-wrapper.tsapps/ui/src/components/views/beads-view/hooks/use-beads-issues.tsapps/ui/src/components/views/board-view/kanban-board.tsxapps/server/src/lib/json-parser.tsapps/server/src/lib/auth.tsapps/server/src/lib/github-cli-path.tsapps/ui/src/components/views/beads-view/lib/column-utils.tslibs/types/src/beads.tsapps/server/src/routes/beads/routes/create.tsapps/ui/src/components/views/beads-view/hooks/use-beads-actions.tsapps/server/src/routes/worktree/common.tsapps/server/tests/unit/services/beads-service.test.tsapps/server/tests/unit/lib/beads-validation.test.tsapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/components/views/beads-view/beads-kanban-board.tsxapps/server/src/routes/github/routes/common.tsapps/server/src/lib/beads-validation.tsapps/server/src/routes/setup/routes/install-claude.tsapps/server/src/lib/rate-limiter.tsapps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsxapps/ui/src/components/views/setup-view/hooks/use-cli-installation.tsapps/server/src/lib/validation-middleware.tsapps/server/src/index.tsapps/ui/src/lib/http-api-client.tsapps/server/tests/unit/lib/json-parser.test.tsapps/ui/src/lib/electron.tsapps/server/src/routes/beads/index.tsapps/server/src/routes/beads/routes/update.tsapps/ui/src/components/views/terminal-view.tsxapps/ui/src/components/views/beads-view/hooks/use-beads-drag-drop.tsapps/ui/src/hooks/use-board-background-settings.tsapps/server/src/routes/setup/routes/gh-status.tsapps/ui/src/routes/beads.tsxapps/server/src/routes/beads/routes/list.tsapps/ui/src/components/views/beads-view/beads-header.tsxapps/ui/src/components/views/settings-view/components/settings-navigation.tsxapps/server/src/services/beads-service.tsapps/ui/src/components/views/beads-view/hooks/use-beads-column-issues.tsapps/ui/src/components/views/beads-view.tsx
apps/ui/src/components/**
📄 CodeRabbit inference engine (CLAUDE.md)
React components should be placed in
apps/ui/src/components/, grouped by feature
Files:
apps/ui/src/components/views/beads-view/hooks/use-beads-issues.tsapps/ui/src/components/views/board-view/kanban-board.tsxapps/ui/src/components/views/beads-view/lib/column-utils.tsapps/ui/src/components/views/beads-view/hooks/use-beads-actions.tsapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/components/views/beads-view/beads-kanban-board.tsxapps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsxapps/ui/src/components/views/setup-view/hooks/use-cli-installation.tsapps/ui/src/components/views/terminal-view.tsxapps/ui/src/components/views/beads-view/hooks/use-beads-drag-drop.tsapps/ui/src/components/views/beads-view/beads-header.tsxapps/ui/src/components/views/settings-view/components/settings-navigation.tsxapps/ui/src/components/views/beads-view/hooks/use-beads-column-issues.tsapps/ui/src/components/views/beads-view.tsx
apps/server/src/services/**
📄 CodeRabbit inference engine (CLAUDE.md)
Services should be placed in
apps/server/src/services/, with one service per file
Files:
apps/server/src/services/beads-service.ts
🧠 Learnings (14)
📚 Learning: 2025-12-24T19:32:07.586Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T19:32:07.586Z
Learning: Applies to .beads/beads.jsonl : Sync the Beads database to the JSONL file and commit changes with `bd sync` followed by `git add .beads/beads.jsonl` and `git commit -m 'Update beads database'`
Applied to files:
apps/server/src/routes/beads/client/cli-wrapper.ts.beads/.local_version
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Perform TypeScript type checking on server changes with `npx tsc -p apps/server/tsconfig.json --noEmit`
Applied to files:
apps/server/src/lib/json-parser.tspackage.json
📚 Learning: 2025-12-24T19:32:07.586Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T19:32:07.586Z
Learning: Create Beads issues for all substantive work (features, bugs, chores) using the `bd create` command
Applied to files:
apps/server/src/routes/beads/routes/create.tsapps/server/src/services/beads-service.ts
📚 Learning: 2025-12-24T19:32:07.586Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T19:32:07.586Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Run type checking with `npm run typecheck` before syncing the Beads database as part of quality gates
Applied to files:
apps/server/tests/unit/services/beads-service.test.tsapps/server/tests/unit/lib/beads-validation.test.tsapps/server/src/lib/beads-validation.tspackage.jsonapps/server/src/services/beads-service.ts
📚 Learning: 2025-12-24T19:32:07.586Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T19:32:07.586Z
Learning: Use `bd dep add <issue-id> blocks <issue-id>` to mark hard blocking dependencies where one issue must complete before another
Applied to files:
apps/ui/src/components/views/beads-view/beads-kanban-board.tsx
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Run linting and tests on server changes with `npm run lint --workspace=apps/server` and `npm run test:server`
Applied to files:
package.json
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Run build and tests on library changes with `npm run build:packages` and `npm run test:packages`
Applied to files:
package.json
📚 Learning: 2025-12-24T19:32:07.586Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T19:32:07.586Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Run linting with `npm run lint` before syncing the Beads database as part of quality gates
Applied to files:
package.json
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Format all changes with `npm run format` before committing
Applied to files:
package.json
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Validate the lockfile with `npm run lint:lockfile` before committing
Applied to files:
package.json
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Perform TypeScript type checking on UI changes via Vite build with `npm run build --workspace=apps/ui`
Applied to files:
package.json
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Applies to apps/server/src/routes/** : API routes should be placed in `apps/server/src/routes/`, with one file per route/resource
Applied to files:
apps/server/src/index.tsapps/ui/src/routes/beads.tsx
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Applies to libs/automaker/types/src/** : Shared types should be exported from `libs/automaker/types/src/`
Applied to files:
apps/ui/src/lib/electron.ts
📚 Learning: 2025-12-24T19:32:07.586Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T19:32:07.586Z
Learning: Check `bd ready` before starting new work to ensure you're working on unblocked tasks
Applied to files:
apps/server/src/services/beads-service.ts
🧬 Code graph analysis (21)
apps/server/src/lib/github-cli-path.ts (2)
init.mjs (1)
isWindows(33-33)apps/ui/src/lib/utils.ts (1)
isMac(60-65)
apps/ui/src/components/views/beads-view/lib/column-utils.ts (2)
libs/types/src/beads.ts (1)
BeadsIssue(36-65)apps/ui/src/components/views/beads-view/constants.ts (1)
BeadsColumnId(45-45)
libs/types/src/beads.ts (1)
libs/types/src/index.ts (1)
BeadsIssueType(89-89)
apps/server/src/routes/beads/routes/create.ts (1)
apps/server/src/lib/beads-validation.ts (1)
createBeadsIssueSchema(62-73)
apps/server/src/routes/worktree/common.ts (2)
apps/server/src/routes/github/routes/common.ts (1)
execEnv(14-14)apps/server/src/lib/github-cli-path.ts (1)
getGitHubCliEnv(65-70)
apps/server/tests/unit/lib/beads-validation.test.ts (1)
apps/server/src/lib/beads-validation.ts (12)
beadsIssueIdSchema(16-18)beadsIssueStatusSchema(23-23)beadsIssueTypeSchema(28-28)beadsIssuePrioritySchema(33-35)beadsLabelsSchema(40-43)createBeadsIssueSchema(62-73)updateBeadsIssueSchema(78-94)deleteBeadsIssueSchema(99-102)addDependencySchema(107-111)listBeadsIssuesFiltersSchema(124-144)searchBeadsIssuesSchema(149-153)getStaleIssuesSchema(158-160)
apps/server/src/routes/github/routes/common.ts (1)
apps/server/src/lib/github-cli-path.ts (1)
getGitHubCliEnv(65-70)
apps/server/src/lib/rate-limiter.ts (1)
init.mjs (1)
req(173-175)
apps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsx (1)
apps/ui/src/lib/electron.ts (1)
getElectronAPI(708-717)
apps/server/src/index.ts (2)
apps/server/src/lib/auth.ts (2)
initializeAuth(21-31)authMiddleware(39-66)apps/server/src/lib/rate-limiter.ts (4)
healthLimiter(52-61)apiLimiter(31-40)strictLimiter(73-82)beadsLimiter(94-103)
apps/server/tests/unit/lib/json-parser.test.ts (1)
apps/server/src/lib/json-parser.ts (2)
safeJsonParse(22-29)safeJsonParseOrDefault(43-49)
apps/server/src/routes/beads/routes/update.ts (1)
apps/server/src/lib/beads-validation.ts (2)
beadsIssueIdSchema(16-18)updateBeadsIssueSchema(78-94)
apps/ui/src/components/views/terminal-view.tsx (1)
init.mjs (1)
install(282-282)
apps/ui/src/components/views/beads-view/hooks/use-beads-drag-drop.ts (1)
apps/ui/src/components/views/beads-view/lib/column-utils.ts (1)
getIssueColumn(14-32)
apps/server/src/routes/setup/routes/gh-status.ts (3)
apps/server/src/routes/github/routes/common.ts (2)
execAsync(9-9)execEnv(14-14)apps/server/src/routes/worktree/common.ts (2)
execAsync(12-12)execEnv(31-31)apps/server/src/lib/github-cli-path.ts (1)
getGitHubCliEnv(65-70)
apps/ui/src/routes/beads.tsx (1)
apps/ui/src/components/views/beads-view.tsx (1)
BeadsView(32-252)
apps/server/src/routes/beads/routes/list.ts (1)
apps/server/src/lib/beads-validation.ts (1)
listBeadsIssuesFiltersSchema(124-144)
apps/ui/src/components/views/settings-view/components/settings-navigation.tsx (1)
apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/server/src/services/beads-service.ts (2)
libs/types/src/beads.ts (5)
ListBeadsIssuesFilters(118-134)BeadsIssue(36-65)CreateBeadsIssueInput(80-95)UpdateBeadsIssueInput(100-113)BeadsStats(153-166)apps/server/src/lib/json-parser.ts (1)
safeJsonParse(22-29)
apps/ui/src/components/views/beads-view/hooks/use-beads-column-issues.ts (1)
apps/ui/src/components/views/beads-view/lib/column-utils.ts (1)
getIssueColumn(14-32)
apps/ui/src/components/views/beads-view.tsx (4)
apps/server/src/lib/beads-validation.ts (1)
CreateBeadsIssueInput(185-185)libs/types/src/beads.ts (1)
CreateBeadsIssueInput(80-95)libs/types/src/index.ts (1)
CreateBeadsIssueInput(93-93)apps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsx (1)
BeadsErrorDiagnostics(22-194)
🪛 LanguageTool
BEADS_AUDIT_REPORT.md
[grammar] ~163-~163: Use a hyphen to join words.
Context: ...timated Effort:** 0.5 days --- ## High Priority Issues ### 5. Incomplete Error...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~283-~283: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...Estimated Effort: 0.5 days --- ## Medium Priority Issues ### 9. No Loading State for Dra...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~335-~335: Consider using “inaccessible” to avoid wordiness.
Context: ...essibility Issues Impact: MEDIUM - Not accessible to keyboard/screen reader users **Files...
(NOT_ABLE_PREMIUM)
[grammar] ~384-~384: Use a hyphen to join words.
Context: ...stimated Effort:** 0.5 days --- ## Low Priority Issues ### 13. Missing CLI Ope...
(QB_NEW_EN_HYPHEN)
[grammar] ~480-~480: Use a hyphen to join words.
Context: ... UI Improvements (2-3 days) 6. Fix drag and drop column detection (0.5 days) 7. ...
(QB_NEW_EN_HYPHEN)
[grammar] ~480-~480: Use a hyphen to join words.
Context: ...Improvements (2-3 days) 6. Fix drag and drop column detection (0.5 days) 7. Fix ...
(QB_NEW_EN_HYPHEN)
[style] ~512-~512: Consider a different adjective to strengthen your wording.
Context: ...s-ignore markers - Explore agents - Deep code analysis with 4 parallel agents - ...
(DEEP_PROFOUND)
🪛 markdownlint-cli2 (0.18.1)
BEADS_AUDIT_REPORT.md
529-529: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (60)
.beads/.local_version (1)
1-1: Version bump is appropriate and well-formed.The minor version bump from 0.34.0 to 0.35.0 aligns with the scope of changes in this PR (features, improvements, and bug fixes without breaking changes). The semantic versioning format is correct.
apps/ui/src/components/views/settings-view/components/settings-navigation.tsx (2)
37-38: Good accessibility improvement; verify complete tab pattern.The addition of
role="tab"andaria-selectedimproves keyboard navigation and screen reader support.For full ARIA tab pattern compliance, verify that:
- The parent
<nav>element hasrole="tablist"- Each tab button includes
aria-controlspointing to its corresponding tabpanel ID- The associated content panels have
role="tabpanel"andaria-labelledby
40-40: LGTM!The visual refinements enhance the navigation experience:
- Asymmetric padding (
pl-4 pr-3) provides better spacing for the left indicator bar- Wider indicator bar (
w-1vsw-0.5) and explicitz-10stacking improve visibility and layeringAlso applies to: 58-58
apps/ui/src/hooks/use-board-background-settings.ts (1)
46-46: LGTM: Consistent default for improved UX.The change to default
hideScrollbartotruealigns with the broader UI-wide scrollbar styling improvements mentioned in the PR objectives. Both fallback locations are updated consistently.Also applies to: 163-163
apps/ui/src/components/views/beads-view/beads-kanban-board.tsx (3)
1-10: LGTM: Improved type safety.Replacing
anywith explicitDragStartEventandDragEndEventtypes from@dnd-kit/corestrengthens type checking and aligns with the PR's TypeScript improvements.Also applies to: 23-24
53-53: LGTM: Consistent with board settings update.The default
hideScrollbar: truematches the change inuse-board-background-settings.tsfor a consistent UX improvement.
110-113: LGTM: Safe map lookup with proper fallback.The memoized map lookup with nullish coalescing provides a safe fallback and correctly replaces the previous inline calculations.
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
1077-1085: Good UX improvement for abnormal closures.The explicit handling of WebSocket close code 1006 provides clear user feedback when the connection drops unexpectedly. The early return appropriately prevents reconnection attempts for this type of closure.
apps/ui/src/components/views/terminal-view.tsx (1)
448-452: Good diagnostic logging improvements.The enhanced error message extraction and additional console logging provide helpful debugging context. The reminder about port 3008 is useful, though note that the actual port is determined by
VITE_SERVER_URLenvironment variable, which could theoretically use a different port..prettierignore (1)
26-26: LGTM! Appropriate exclusion of generated file.Excluding generated routing files from Prettier formatting is a standard best practice to avoid unnecessary formatting churn.
apps/ui/src/components/views/setup-view/hooks/use-cli-installation.ts (1)
1-114: LGTM! Enhanced error handling improves user experience.The addition of
manualCommandto the error response and the conditional error handling provides users with actionable manual installation instructions when automated installation fails. The implementation correctly checks for the presence ofmanualCommandbefore including it in the error message.apps/ui/src/lib/electron.ts (1)
1077-1084: LGTM! Type definition aligns with enhanced CLI installation.The additional optional fields (
version,details,manualCommand) in theinstallClaudereturn type provide better context for CLI installation outcomes and enable the enhanced error handling implemented inuse-cli-installation.ts.apps/server/tests/unit/lib/json-parser.test.ts (1)
1-103: LGTM! Comprehensive test coverage for JSON parser utilities.The test suite thoroughly covers both
safeJsonParseandsafeJsonParseOrDefaultfunctions with:
- Valid JSON parsing for various types (objects, arrays, custom types)
- Error handling with descriptive messages and context
- Edge cases (empty strings, whitespace, malformed JSON)
- Type preservation verification
- Default value fallback behavior
The tests are well-structured and follow Vitest best practices.
apps/ui/src/components/views/board-view/kanban-board.tsx (1)
104-104: Overflow handling is properly implemented in KanbanColumn.The
max-h-[calc(100vh-12rem)]constraint is correctly paired with the KanbanColumn'soverflow-y-autoclass, ensuring content overflow is handled appropriately when columns exceed the viewport height.apps/server/package.json (1)
34-38: The dependency versions are appropriate and current.Both
express-rate-limit@8.2.1andzod@4.2.1are latest stable releases. The caret ranges (^) allow for backward-compatible patch and minor updates, which is standard practice. No known security advisories detected for these versions.apps/server/src/lib/json-parser.ts (2)
22-29: LGTM - Clear utility with good error context.The implementation correctly wraps JSON.parse with descriptive error messages. The explicit documentation about no runtime validation being performed is helpful for consumers.
43-49: Good fallback pattern.This is a clean implementation for cases where a default value is acceptable. Consider whether debug-level logging of parse failures would aid troubleshooting, though silent fallback may be intentional for noisy scenarios.
BEADS_AUDIT_REPORT.md (2)
1-7: Comprehensive and actionable audit report.This is a well-structured audit document with clear severity categorization and effort estimates. The phased implementation plan provides a good roadmap for addressing the identified issues.
529-529: Static analysis note: Intentional emphasis usage.The markdownlint warning about "emphasis used instead of a heading" is a false positive here—the emphasis is intentionally used for attribution text, not as a section heading.
apps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsx (1)
171-193: Good UX with loading states.The component properly disables the button and shows a spinner during loading. The diagnostic panel is conditionally rendered based on loading state.
apps/server/src/lib/rate-limiter.ts (2)
46-61: Health limiter may be too restrictive for monitoring tools.At 10 requests per minute, aggressive monitoring tools or multiple health check sources could hit the limit. Kubernetes liveness/readiness probes, for example, might poll every few seconds. Consider whether 30-60 requests per minute would be more appropriate for health endpoints.
67-103: Well-structured rate limiter configurations.The strict limiter appropriately limits sensitive operations, and the beads limiter accommodates the higher request volume expected from frequent board interactions. Good use of consistent message formatting.
apps/server/src/routes/setup/routes/install-claude.ts (1)
11-46: Robust cross-platform installation flow.Good implementation with proper platform detection, extended timeout for installation, and graceful error handling. The use of
spawnProcesswith structured error handling provides good reliability.apps/ui/src/components/views/beads-view/beads-header.tsx (2)
1-1: Good cleanup of unused imports.Removing unused
useState,useCallback, andcnimports reduces bundle size and improves code clarity.
78-94: Filter dropdown is a placeholder.The type filter
Selectcomponent usesdefaultValue="all"but has noonValueChangehandler, making it non-functional. The comment indicates this is intentional for future implementation. Consider adding adisabledprop or TODO comment if this should remain a visual placeholder.apps/ui/src/components/views/beads-view/hooks/use-beads-drag-drop.ts (2)
5-5: Good refactor: Centralized column determination.Extracting column logic to a shared utility (
getIssueColumn) eliminates duplication and ensures consistent behavior across the Beads UI. This addresses the audit report's recommendation for safer column detection.
68-74: Correct usage of shared column utility.The refactored code properly delegates column determination to
getIssueColumn, which handles blocker checking and status-based column assignment. The comment update on line 69 accurately describes the change.apps/server/src/routes/beads/client/cli-wrapper.ts (1)
163-165: Database path normalized tobeads.db.All database path references are consistently updated. The
beads.dbpath is aligned acrossbeads-service.ts(line 151), unit tests (beads-service.test.ts lines 30, 35), and documentation (README.md).apps/server/src/routes/setup/routes/gh-status.ts (1)
12-17: LGTM! Centralized GitHub CLI PATH handling.The refactor to use
getGitHubCliEnv()for PATH configuration is a positive change that centralizes environment handling and reduces duplication across GitHub CLI-related routes.apps/ui/src/routes/beads.tsx (1)
1-6: LGTM! Clean route definition.The route file is properly structured for TanStack React Router and correctly wires the BeadsView component to the
/beadspath.apps/server/src/routes/beads/routes/create.ts (1)
8-8: Good addition of Zod validation.The introduction of schema-based validation using
createBeadsIssueSchemaimproves type safety and provides structured validation error responses.apps/server/src/routes/worktree/common.ts (1)
9-31: LGTM! Consistent environment centralization.The refactor aligns with the centralized GitHub CLI PATH handling pattern used across the codebase, improving maintainability and consistency.
apps/server/src/routes/beads/routes/list.ts (1)
8-47: Good addition of filter validation.The Zod-based validation for filters improves API robustness and provides clear error messages when invalid filter parameters are provided.
apps/ui/src/components/views/beads-view/hooks/use-beads-actions.ts (1)
12-26: LGTM! Simplified hook interface.Removing the
loadIssuesprop simplifies the hook's API and makes it more focused on action handlers. The separation of concerns betweenuse-beads-issues(data loading) anduse-beads-actions(mutations) is cleaner.apps/server/src/routes/beads/index.ts (1)
33-35: The/syncendpoint correctly does not requireprojectPathvalidation.The
createSyncHandler()implementation executes a globalbd synccommand with{ noDb: true }and does not extract or useprojectPathfrom the request. This differs from/connect, which explicitly extractsprojectPathfromreq.bodyand uses it for directory checks and configuration. The current implementation is correct—no validation is needed.apps/server/tests/unit/services/beads-service.test.ts (1)
23-37: LGTM!The database path tests correctly verify both standard paths and trailing slash handling.
apps/server/src/lib/auth.ts (2)
12-31: LGTM! Production safety check is critical.The initialization function correctly enforces API key requirement in production mode and provides clear logging feedback. This prevents accidental deployment without authentication.
78-83: LGTM! Extended status response.The addition of
productionModeto the auth status response provides useful context for health checks and debugging.apps/server/src/routes/github/routes/common.ts (1)
7-14: LGTM! Centralized PATH configuration.Replacing manual PATH construction with the centralized
getGitHubCliEnv()utility eliminates code duplication and ensures consistent GitHub CLI detection across all routes.apps/server/src/routes/beads/routes/update.ts (1)
26-54: LGTM! Comprehensive input validation.The validation flow correctly validates both
issueIdandupdateswith Zod schemas before processing, providing detailed error feedback. The extracted validated data ensures type safety downstream.apps/server/src/lib/github-cli-path.ts (1)
65-70: LGTM! Clean environment utility.The function correctly spreads the current environment and overrides PATH with the extended version, providing a consistent environment object for GitHub CLI operations.
apps/ui/src/lib/http-api-client.ts (1)
445-452: LGTM! Enhanced installation feedback.The extended return type for
installClaudenow includes optionalversion,details, andmanualCommandfields, enabling richer feedback to users about the installation process and manual fallback instructions when needed.apps/server/src/index.ts (3)
62-90: LGTM! Security initialization on startup.The initialization sequence correctly validates authentication setup in production mode and logs clear status messages. The ANTHROPIC_API_KEY check provides helpful guidance when credentials are missing.
96-125: LGTM! Robust CORS validation.The
validateCorsOriginfunction properly validates the CORS origin with appropriate defaults for development, URL format validation, and clear warnings for insecure configurations like wildcards.
180-207: LGTM! Well-structured rate limiting and authentication.The layered security approach is sound:
- Health endpoint: rate-limited but unauthenticated (appropriate for monitoring)
- All other API routes: authenticated via
authMiddleware- Sensitive endpoints (
/api/setup,/api/settings): stricter rate limits- High-volume endpoints (
/api/beads): higher limitsThe middleware ordering correctly applies authentication before route handlers.
apps/ui/src/components/views/beads-view/lib/column-utils.ts (2)
14-32: LGTM! Clear and correct column assignment logic.The function properly prioritizes column placement: closed issues to done, blocked issues to blocked (taking precedence over status), then status-based placement (in_progress → ready → backlog). The logic is easy to follow and correct.
41-56: LGTM! Blocker detection logic is correct.The function correctly identifies open blockers by checking 'blocks' type dependencies and verifying the blocking issue's status. The implementation is straightforward and handles edge cases (no dependencies, missing blocking issues) appropriately.
Note: The linear search through
allIssuesfor each dependency could become a performance bottleneck if there are many issues with many dependencies. If performance becomes an issue, consider memoizing this function in the calling hook or pre-building an issue ID→issue map.Based on learnings, the PR addresses review feedback about memoizing blocking-count calculations to avoid O(n²).
apps/server/src/lib/beads-validation.ts (1)
1-192: Well-structured validation layer with comprehensive schemas.Good implementation of Zod-based validation with:
- Reusable base schemas for consistent validation
- Cross-field refinements (priorityMin/Max, non-empty update)
- Strict mode on filter schema to reject unknown properties
- Type exports via
z.inferfor type-safe route handlerslibs/types/src/beads.ts (2)
84-88: LGTM - optional fields align with create operation semantics.Making
description,type, andpriorityoptional onCreateBeadsIssueInputcorrectly reflects that onlytitleis required for issue creation.
109-110: Priority type relaxed from literal union to number.The change from
BeadsIssuePriority(literal union0|1|2|3|4) tonumberreduces compile-time type safety but is acceptable since the Zod schema (beadsIssuePrioritySchema) validates the 0-4 range at runtime. The entity typeBeadsIssuecorrectly retainsBeadsIssuePriorityfor stored data.Also applies to: 125-127
apps/server/tests/unit/lib/beads-validation.test.ts (1)
23-339: Good test coverage for validation schemas.Comprehensive tests covering valid/invalid inputs, boundary conditions, and error message verification. The test structure is clean and follows consistent patterns.
apps/ui/src/components/views/beads-view.tsx (3)
15-21: LGTM - improved type safety and error handling.Good refactoring:
BeadsErrorDiagnosticsprovides structured error diagnostics instead of inline error UICreateBeadsIssueInputtype import ensures type-safe dialog handling
161-167: Improved type safety for issue creation.Using
CreateBeadsIssueInputinstead ofanyensures the dialog input matches the expected schema, catching type mismatches at compile time.
179-186: Enhanced error state with diagnostics component.The
BeadsErrorDiagnosticscomponent provides better user experience by showing diagnostic information (CLI status, database checks) rather than a generic error message.apps/server/src/lib/validation-middleware.ts (1)
1-185: Well-designed validation middleware with consistent error handling.Clean implementation with:
- Uniform error response format via
formatValidationError- Generic type inference from Zod schemas
- Proper Express middleware signature
- Non-Zod errors correctly delegated to error handler via
next(error)- Combined
validate()function for multi-target validation in a single middlewareapps/ui/src/components/views/beads-view/hooks/use-beads-column-issues.ts (1)
3-4: Good refactor to centralize column logic.Extracting column determination to
getIssueColumnutility promotes consistency across the codebase (also used in drag-drop logic) and reduces code duplication.apps/server/src/services/beads-service.ts (4)
114-129: Good defensive initialization check.The
canInitializeBeadsmethod correctly handles both scenarios: checking if.beadsdirectory is writable (for existing init) or if the parent project directory is writable (for new init). The nested try-catch pattern appropriately catchesENOENTwhen.beadsdoesn't exist.
233-235: Good addition of parentIssueId support.This enables creating subtasks/child issues via the CLI's
--parentflag, aligning with the schema update increateBeadsIssueSchema.
468-475: Good defensive callback execution.Wrapping the callback in try-catch prevents watcher errors from propagating and stopping the watch. Logging the error maintains observability.
495-502: Improved error type handling.Accepting
unknowninstead of implicitly assumingErroris safer and follows TypeScript best practices for catch blocks.
| title: z | ||
| .string() | ||
| .min(1, 'Title is required') | ||
| .max(200, 'Title must be 200 characters or less') | ||
| .regex(/^[^<>{}$]{1,50}$/, 'Title contains invalid characters') | ||
| .optional(), |
There was a problem hiding this comment.
Inconsistent title length constraint in regex vs max().
The title regex [^<>{}$]{1,50} limits length to 50 characters, but the .max(200) constraint allows up to 200 characters. This means valid titles between 51-200 characters will fail validation with a confusing "invalid characters" error instead of a length error.
🔎 Proposed fix
title: z
.string()
.min(1, 'Title is required')
.max(200, 'Title must be 200 characters or less')
- .regex(/^[^<>{}$]{1,50}$/, 'Title contains invalid characters')
+ .regex(/^[^<>{}$]*$/, 'Title contains invalid characters')
.optional(),🤖 Prompt for AI Agents
In apps/server/src/lib/beads-validation.ts around lines 80 to 85, the regex
limits title length to 50 characters while .max(200) allows 200, causing 51–200
char titles to fail with the wrong error; fix by aligning the regex with the max
length (e.g., change the quantifier to {1,200}) or remove the length quantifier
from the regex and rely on .min/.max for length checks (e.g., use /^[^<>{}$]+$/)
so length and character validation are consistent.
| // Temporary: Make rate limiting optional due to missing dependency | ||
| let rateLimit: RateLimit; | ||
|
|
||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| rateLimit = require('express-rate-limit'); | ||
| } catch { | ||
| console.warn('[rate-limiter] express-rate-limit not installed, rate limiting disabled'); | ||
| rateLimit = () => (req: Request, res: Response, next: NextFunction) => next(); | ||
| } |
There was a problem hiding this comment.
Silent rate-limit degradation could mask security gaps in production.
When express-rate-limit is missing, the fallback no-op middleware silently disables all rate limiting. While useful for development, this could be a security risk if the dependency is accidentally missing in production.
Consider failing hard in production mode or logging a more prominent warning:
🔎 Proposed fix
try {
// eslint-disable-next-line @typescript-eslint/no-require-imports
rateLimit = require('express-rate-limit');
} catch {
+ const isProduction = process.env.NODE_ENV === 'production';
+ if (isProduction) {
+ throw new Error('[rate-limiter] express-rate-limit is required in production');
+ }
console.warn('[rate-limiter] express-rate-limit not installed, rate limiting disabled');
rateLimit = () => (req: Request, res: Response, next: NextFunction) => next();
}📝 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.
| // Temporary: Make rate limiting optional due to missing dependency | |
| let rateLimit: RateLimit; | |
| try { | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports | |
| rateLimit = require('express-rate-limit'); | |
| } catch { | |
| console.warn('[rate-limiter] express-rate-limit not installed, rate limiting disabled'); | |
| rateLimit = () => (req: Request, res: Response, next: NextFunction) => next(); | |
| } | |
| // Temporary: Make rate limiting optional due to missing dependency | |
| let rateLimit: RateLimit; | |
| try { | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports | |
| rateLimit = require('express-rate-limit'); | |
| } catch { | |
| const isProduction = process.env.NODE_ENV === 'production'; | |
| if (isProduction) { | |
| throw new Error('[rate-limiter] express-rate-limit is required in production'); | |
| } | |
| console.warn('[rate-limiter] express-rate-limit not installed, rate limiting disabled'); | |
| rateLimit = () => (req: Request, res: Response, next: NextFunction) => next(); | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/lib/rate-limiter.ts around lines 10 to 19, the current
fallback silently disables rate limiting when express-rate-limit is missing,
which risks unnoticed degradation in production; change the behavior to throw an
error or exit when NODE_ENV is "production" (fail hard) and otherwise keep the
permissive noop for development, and always emit a prominent logger.error (not
just console.warn) that includes environment and remediation steps so CI/ops
will notice the missing dependency.
| // Check for network errors | ||
| const hasNetworkError = | ||
| stderr.includes('curl') || | ||
| stderr.includes('connection') || | ||
| stderr.includes('network') || | ||
| stderr.includes('failed to download'); | ||
|
|
||
| if (hasNetworkError && result.exitCode !== 0) { | ||
| res.json({ | ||
| success: false, | ||
| error: | ||
| 'Installation failed due to network error. Please check your connection and try again.', | ||
| manualCommand: isWindows | ||
| ? 'irm https://claude.ai/install.ps1 | iex' | ||
| : 'curl -fsSL https://claude.ai/install.sh | bash', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Network error detection may produce false positives.
The check stderr.includes('curl') could trigger false positives if "curl" appears in any error output (e.g., path names, version info). Consider checking for more specific error patterns.
🔎 Proposed fix
// Check for network errors
const hasNetworkError =
- stderr.includes('curl') ||
- stderr.includes('connection') ||
- stderr.includes('network') ||
+ stderr.includes('could not resolve') ||
+ stderr.includes('connection refused') ||
+ stderr.includes('network unreachable') ||
stderr.includes('failed to download');📝 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.
| // Check for network errors | |
| const hasNetworkError = | |
| stderr.includes('curl') || | |
| stderr.includes('connection') || | |
| stderr.includes('network') || | |
| stderr.includes('failed to download'); | |
| if (hasNetworkError && result.exitCode !== 0) { | |
| res.json({ | |
| success: false, | |
| error: | |
| 'Installation failed due to network error. Please check your connection and try again.', | |
| manualCommand: isWindows | |
| ? 'irm https://claude.ai/install.ps1 | iex' | |
| : 'curl -fsSL https://claude.ai/install.sh | bash', | |
| }); | |
| return; | |
| } | |
| // Check for network errors | |
| const hasNetworkError = | |
| stderr.includes('could not resolve') || | |
| stderr.includes('connection refused') || | |
| stderr.includes('network unreachable') || | |
| stderr.includes('failed to download'); | |
| if (hasNetworkError && result.exitCode !== 0) { | |
| res.json({ | |
| success: false, | |
| error: | |
| 'Installation failed due to network error. Please check your connection and try again.', | |
| manualCommand: isWindows | |
| ? 'irm https://claude.ai/install.ps1 | iex' | |
| : 'curl -fsSL https://claude.ai/install.sh | bash', | |
| }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/routes/setup/routes/install-claude.ts around lines 71–88, the
current network detection uses stderr.includes('curl') which can cause false
positives; replace the simple substring checks with a set of targeted,
case‑insensitive regex patterns (e.g. /curl:\s*(6|7|28|22)/i, /could not resolve
host/i, /failed to connect/i, /connection timed out/i, /name or service not
known/i, /failed to download/i) and test stderr against those patterns (using
word boundaries where appropriate) so only real network-related messages match;
implement an array of regexes, iterate or .some() to set hasNetworkError, and
keep the existing exitCode check and response behavior unchanged.
| interface BeadsDiagnostics { | ||
| cliInstalled: boolean; | ||
| cliVersion: string | null; | ||
| dbInitialized: boolean; | ||
| serverResponding: boolean; | ||
| canInitialize: boolean; | ||
| cliPath?: string; | ||
| dbPath?: string; | ||
| } |
There was a problem hiding this comment.
Unused interface field: canInitialize.
The BeadsDiagnostics interface defines canInitialize: boolean, but this field is never populated in the runDiagnostics function—it remains false from initialization. Either populate it from the validation response or remove it from the interface.
🔎 Proposed fix
If canInitialize should be derived from validation:
diag.cliVersion = validation.version || null;
diag.dbInitialized = validation.initialized === true;
+ diag.canInitialize = validation.canInitialize === true;Or if unused, remove from interface:
interface BeadsDiagnostics {
cliInstalled: boolean;
cliVersion: string | null;
dbInitialized: boolean;
serverResponding: boolean;
- canInitialize: boolean;
cliPath?: string;
dbPath?: string;
}📝 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.
| interface BeadsDiagnostics { | |
| cliInstalled: boolean; | |
| cliVersion: string | null; | |
| dbInitialized: boolean; | |
| serverResponding: boolean; | |
| canInitialize: boolean; | |
| cliPath?: string; | |
| dbPath?: string; | |
| } | |
| interface BeadsDiagnostics { | |
| cliInstalled: boolean; | |
| cliVersion: string | null; | |
| dbInitialized: boolean; | |
| serverResponding: boolean; | |
| cliPath?: string; | |
| dbPath?: string; | |
| } |
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/beads-view/components/beads-error-diagnostics.tsx
around lines 12 to 20, the BeadsDiagnostics interface includes canInitialize but
runDiagnostics never sets it; either remove canInitialize from the interface
(and any downstream usages) or set it in runDiagnostics by deriving it from the
validation response (e.g., diagnostics.canInitialize = validation.canInitialize)
or from the existing diagnostic flags (for example a boolean expression like
cliInstalled && !dbInitialized if that matches the intended logic); update the
interface and the runDiagnostics assignment consistently and ensure any UI
consumers read the updated property or are updated to the new shape.
| ws.onerror = (error) => { | ||
| console.error(`[Terminal] WebSocket error for session ${sessionId}:`, error); | ||
|
|
||
| // Show user-facing error | ||
| toast.error('Terminal connection error', { | ||
| description: 'WebSocket connection failed. Check that the server is running.', | ||
| duration: 5000, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Consider potential duplicate error notifications.
The WebSocket onerror handler typically fires before onclose events. For example, an abnormal closure (code 1006) will trigger both handlers, showing two toasts to the user:
- "Terminal connection error" from
onerror(line 1145) - "Terminal connection closed unexpectedly" from
onclose(line 1080)
Consider either removing the toast from one handler or adding logic to prevent duplicate notifications within a short time window.
🔎 Suggested approach to prevent duplicates
Track the last error toast time to debounce notifications:
const reconnectAttemptsRef = useRef(0);
+ const lastErrorToastRef = useRef(0);
const MAX_RECONNECT_ATTEMPTS = 5; ws.onerror = (error) => {
console.error(`[Terminal] WebSocket error for session ${sessionId}:`, error);
- // Show user-facing error
- toast.error('Terminal connection error', {
- description: 'WebSocket connection failed. Check that the server is running.',
- duration: 5000,
- });
+ // Show user-facing error (debounced to prevent duplicate with onclose)
+ const now = Date.now();
+ if (now - lastErrorToastRef.current > 5000) {
+ lastErrorToastRef.current = now;
+ toast.error('Terminal connection error', {
+ description: 'WebSocket connection failed. Check that the server is running.',
+ duration: 5000,
+ });
+ }
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/src/components/views/terminal-view/terminal-panel.tsx around lines
1141 to 1149, the WebSocket onerror handler shows a toast that can duplicate the
onclose toast; update the logic to prevent duplicate notifications by adding a
short debounce/guard (e.g., a module-scoped or component state timestamp/boolean
like lastToastTime or toastSuppressedUntil) checked/updated by both onerror and
onclose so only the first event within the debounce window shows a toast, or
alternatively remove the toast from onerror and keep it only in onclose;
implement the chosen approach consistently in both handlers so a single
user-facing toast appears for a single connection failure.
| /* General thin scrollbar for all themes (applies to light themes too) */ | ||
| :is( | ||
| .light, | ||
| .cream, | ||
| .solarizedlight, | ||
| .github, | ||
| .paper, | ||
| .rose, | ||
| .mint, | ||
| .lavender, | ||
| .sand, | ||
| .sky, | ||
| .peach, | ||
| .snow, | ||
| .sepia, | ||
| .gruvboxlight, | ||
| .nordlight, | ||
| .blossom, | ||
| .forest, | ||
| .ocean | ||
| ) | ||
| ::-webkit-scrollbar { | ||
| width: 8px; | ||
| height: 8px; | ||
| } | ||
|
|
||
| :is( | ||
| .light, | ||
| .cream, | ||
| .solarizedlight, | ||
| .github, | ||
| .paper, | ||
| .rose, | ||
| .mint, | ||
| .lavender, | ||
| .sand, | ||
| .sky, | ||
| .peach, | ||
| .snow, | ||
| .sepia, | ||
| .gruvboxlight, | ||
| .nordlight, | ||
| .blossom, | ||
| .forest, | ||
| .ocean | ||
| ) | ||
| ::-webkit-scrollbar-track { | ||
| background: var(--muted); | ||
| border-radius: 4px; | ||
| } | ||
|
|
||
| :is( | ||
| .light, | ||
| .cream, | ||
| .solarizedlight, | ||
| .github, | ||
| .paper, | ||
| .rose, | ||
| .mint, | ||
| .lavender, | ||
| .sand, | ||
| .sky, | ||
| .peach, | ||
| .snow, | ||
| .sepia, | ||
| .gruvboxlight, | ||
| .nordlight, | ||
| .blossom, | ||
| .forest, | ||
| .ocean | ||
| ) | ||
| ::-webkit-scrollbar-thumb { | ||
| background: var(--muted-foreground); | ||
| border-radius: 4px; | ||
| } | ||
|
|
||
| :is( | ||
| .light, | ||
| .cream, | ||
| .solarizedlight, | ||
| .github, | ||
| .paper, | ||
| .rose, | ||
| .mint, | ||
| .lavender, | ||
| .sand, | ||
| .sky, | ||
| .peach, | ||
| .snow, | ||
| .sepia, | ||
| .gruvboxlight, | ||
| .nordlight, | ||
| .blossom, | ||
| .forest, | ||
| .ocean | ||
| ) | ||
| ::-webkit-scrollbar-thumb:hover { | ||
| background: var(--foreground-secondary); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate duplicated scrollbar selectors.
The scrollbar styling for light themes contains significant code duplication. The same selector list (containing 18 theme classes) is repeated 4 times for different pseudo-elements. This should be consolidated into a single selector block for maintainability and reduced bundle size.
🔎 Proposed consolidation
-/* General thin scrollbar for all themes (applies to light themes too) */
-:is(
- .light,
- .cream,
- .solarizedlight,
- .github,
- .paper,
- .rose,
- .mint,
- .lavender,
- .sand,
- .sky,
- .peach,
- .snow,
- .sepia,
- .gruvboxlight,
- .nordlight,
- .blossom,
- .forest,
- .ocean
- )
- ::-webkit-scrollbar {
- width: 8px;
- height: 8px;
-}
-
-:is(
- .light,
- .cream,
- .solarizedlight,
- .github,
- .paper,
- .rose,
- .mint,
- .lavender,
- .sand,
- .sky,
- .peach,
- .snow,
- .sepia,
- .gruvboxlight,
- .nordlight,
- .blossom,
- .forest,
- .ocean
- )
- ::-webkit-scrollbar-track {
- background: var(--muted);
- border-radius: 4px;
-}
-
-:is(
- .light,
- .cream,
- .solarizedlight,
- .github,
- .paper,
- .rose,
- .mint,
- .lavender,
- .sand,
- .sky,
- .peach,
- .snow,
- .sepia,
- .gruvboxlight,
- .nordlight,
- .blossom,
- .forest,
- .ocean
- )
- ::-webkit-scrollbar-thumb {
- background: var(--muted-foreground);
- border-radius: 4px;
-}
-
-:is(
- .light,
- .cream,
- .solarizedlight,
- .github,
- .paper,
- .rose,
- .mint,
- .lavender,
- .sand,
- .sky,
- .peach,
- .snow,
- .sepia,
- .gruvboxlight,
- .nordlight,
- .blossom,
- .forest,
- .ocean
- )
- ::-webkit-scrollbar-thumb:hover {
- background: var(--foreground-secondary);
-}
+/* General thin scrollbar for all light themes */
+:is(
+ .light,
+ .cream,
+ .solarizedlight,
+ .github,
+ .paper,
+ .rose,
+ .mint,
+ .lavender,
+ .sand,
+ .sky,
+ .peach,
+ .snow,
+ .sepia,
+ .gruvboxlight,
+ .nordlight,
+ .blossom,
+ .forest,
+ .ocean
+ ) {
+ ::-webkit-scrollbar {
+ width: 8px;
+ height: 8px;
+ }
+
+ ::-webkit-scrollbar-track {
+ background: var(--muted);
+ border-radius: 4px;
+ }
+
+ ::-webkit-scrollbar-thumb {
+ background: var(--muted-foreground);
+ border-radius: 4px;
+ }
+
+ ::-webkit-scrollbar-thumb:hover {
+ background: var(--foreground-secondary);
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/src/styles/global.css around lines 414–512 the long :is(...) theme
selector list is duplicated four times for different scrollbar pseudo-elements;
replace this duplication by extracting that :is(...) list into a single reusable
selector and define the scrollbar pseudo-elements under it (use nesting or a
mixin if your build supports SCSS/PostCSS nesting) so the theme list appears
only once and the four pseudo-element blocks (:: -webkit-scrollbar,
::-webkit-scrollbar-track, ::-webkit-scrollbar-thumb,
::-webkit-scrollbar-thumb:hover) are defined beneath it with their respective
properties.
…loading - Remove unused platform variables in github-cli-path.ts - Add flexible .env loading from project root and current directory - Add PR creation documentation and helper script 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/server/src/index.ts (1)
108-111: CORS default port mismatch with documented UI port.As noted in a previous review, defaulting to
http://localhost:3008may cause CORS failures if the UI runs on port 3000 as documented elsewhere. Consider aligning the default with the UI's expected port or allowing multiple origins.
🧹 Nitpick comments (3)
apps/server/src/index.ts (1)
56-60: Environment loading may miss.envin some scenarios.
INIT_CWDis only set by npm/yarn during script execution. When running the server directly (e.g.,node dist/index.js), this variable won't be set, andprocess.cwd()becomes the fallback. The doubledotenv.config()call on line 60 is redundant if the first call already loads fromcwd.Consider simplifying or documenting the expected behavior:
🔎 Suggested simplification
// Load environment variables -// Try loading from project root (for development) and current directory -const projectRoot = process.env.INIT_CWD || process.cwd(); -dotenv.config({ path: path.join(projectRoot, '.env') }); -dotenv.config(); // Fallback to default behavior (loads from cwd) +// Load from project root (INIT_CWD set by npm) or current directory +const projectRoot = process.env.INIT_CWD || process.cwd(); +const envPath = path.join(projectRoot, '.env'); +const result = dotenv.config({ path: envPath }); +if (result.error && process.env.INIT_CWD) { + // Fallback to cwd if INIT_CWD path failed + dotenv.config(); +}create-pr.sh (1)
1-13: Add error handling to prevent silent failures.The script lacks
set -eor explicit error checks. Ifgh pr createfails (e.g., auth issues, network problems), the script will still print "PR created successfully!" which is misleading.🔎 Suggested improvements
#!/bin/bash +set -e + # Run this script when internet connectivity is restored echo "Creating PR for UX-improvements-#1..." +# Verify PR_DESCRIPTION.md exists +if [[ ! -f "PR_DESCRIPTION.md" ]]; then + echo "Error: PR_DESCRIPTION.md not found" + exit 1 +fi + # Create the PR gh pr create \ --title "feat: Comprehensive UX improvements - Beads integration, Kanban board, and stability enhancements" \ --body-file PR_DESCRIPTION.md \ --base main echo "PR created successfully!" echo "Don't forget to update Beads issue DevFlow-cto with the PR link."CREATE_PR_INSTRUCTIONS.md (1)
1-62: Documentation is clear and actionable.The instructions provide both CLI and web UI options for PR creation, which is helpful. The static analysis hints about bare URLs (lines 6, 36) and missing code block language (line 12) are minor markdown linting suggestions. Bare URLs are typically acceptable in documentation as they render as clickable links.
Consider adding a language identifier to the code block on line 12 for consistency:
### Title -``` +```text feat: Comprehensive UX improvements - Beads integration, Kanban board, and stability enhancements</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9fe8cfca71d822629e62fbcfa8f7ccf4262bc57d and 7dd2d2054f9f634c38558ae021fee3c2d46f6b29. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `.claude/commands/update-app.md` * `CREATE_PR_INSTRUCTIONS.md` * `PR_CREATION_SUMMARY.md` * `PR_DESCRIPTION.md` * `apps/server/src/index.ts` * `apps/server/src/lib/github-cli-path.ts` * `create-pr.sh` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * .claude/commands/update-app.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * apps/server/src/lib/github-cli-path.ts </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.{ts,tsx,js,jsx}</summary> **📄 CodeRabbit inference engine (AGENTS.md)** > `**/*.{ts,tsx,js,jsx}`: Run type checking with `npm run typecheck` before syncing the Beads database as part of quality gates > Run linting with `npm run lint` before syncing the Beads database as part of quality gates Files: - `apps/server/src/index.ts` </details> </details><details> <summary>🧠 Learnings (2)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T19:32:07.586Z
Learning: Create Beads issues for all substantive work (features, bugs, chores) using thebd createcommand</details> <details> <summary>📚 Learning: 2025-12-24T19:31:56.698Z</summary>Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Applies to apps/server/src/routes/** : API routes should be placed inapps/server/src/routes/, with one file per route/resource**Applied to files:** - `apps/server/src/index.ts` </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>PR_DESCRIPTION.md</summary> [uncategorized] ~60-~60: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ..., 2 high priority, 5 medium priority, 3 low priority recommendations ### Sentry Integration... (EN_COMPOUND_ADJECTIVE_INTERNAL) </details> <details> <summary>PR_CREATION_SUMMARY.md</summary> [uncategorized] ~86-~86: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...itical**: 0 - **High Priority**: 2 1. Rate limiting dependency should be required in produc... (EN_COMPOUND_ADJECTIVE_INTERNAL) --- [grammar] ~322-~322: Ensure spelling is correct Context: ...nd validated ✅ Ready for merge --- ## Greptile Review Summary **Security:** Excellent... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>CREATE_PR_INSTRUCTIONS.md</summary> 6-6: Bare URL used (MD034, no-bare-urls) --- 12-12: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 36-36: Bare URL used (MD034, no-bare-urls) </details> <details> <summary>PR_CREATION_SUMMARY.md</summary> 108-108: Bare URL used (MD034, no-bare-urls) --- 110-110: Bare URL used (MD034, no-bare-urls) --- 174-174: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 224-224: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 240-240: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 266-266: Bare URL used (MD034, no-bare-urls) </details> </details> </details> <details> <summary>🔇 Additional comments (6)</summary><blockquote> <details> <summary>apps/server/src/index.ts (2)</summary><blockquote> `105-127`: **CORS validation function is well-structured.** The `validateCorsOrigin()` function properly validates URL format, logs warnings for insecure configurations, and throws on invalid input. This is a good security practice. --- `90-94`: **Security initialization order looks correct.** `initAllowedPaths()` and `initializeAuth()` are called at startup before routes are mounted. This ensures security measures are in place before handling requests. </blockquote></details> <details> <summary>PR_DESCRIPTION.md (1)</summary><blockquote> `1-75`: **PR description document is comprehensive and well-structured.** The document provides clear summaries of changes, test plan status, critical changes, and statistics. The "Generated with Claude Code" attribution is appropriately disclosed. The static analysis hint about "high priority" hyphenation is a minor style suggestion that can be safely ignored. </blockquote></details> <details> <summary>PR_CREATION_SUMMARY.md (3)</summary><blockquote> `239-248`: **Test output files listed as untracked.** The note correctly identifies that `test-output.txt`, `test-results.txt`, and `test-server-results.txt` are temporary files that should be excluded from commits. Ensure these patterns are in `.gitignore` to prevent accidental commits. --- `1-380`: **Comprehensive PR summary with thorough quality documentation.** The document provides excellent visibility into the quality gates passed, including TypeScript checks, ESLint, test results, Greptile findings, and Sentry status. The actionable recommendations from the Greptile review (lines 83-101) are well-documented for future follow-up. The static analysis hints about bare URLs and missing code block languages are minor markdown linting issues that don't affect the document's utility. --- `224-237`: **Consider excluding runtime files from commits.** The modified files list includes `.beads/beads.db` (database file) and `.beads/daemon.lock` (lock file). These are typically runtime artifacts that shouldn't be committed to version control as they may cause merge conflicts and contain machine-specific state. Verify these files are intentionally tracked. If not, consider adding them to `.gitignore`: ```gitignore .beads/beads.db .beads/daemon.lock⛔ Skipped due to learnings
Learnt from: CR Repo: 0xtsotsi/DevFlow PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-24T19:32:07.586Z Learning: Applies to .beads/beads.jsonl : Sync the Beads database to the JSONL file and commit changes with `bd sync` followed by `git add .beads/beads.jsonl` and `git commit -m 'Update beads database'`
- Add rate limiting to all API endpoints (apiLimiter, strictLimiter, healthLimiter, beadsLimiter) - Fix TypeScript type errors in rate-limiter.ts with proper RateLimitFunction type - Remove unused variables in github-cli-path.ts (path, isMac, isLinux) - Remove unused 'verified' variable in install-claude.ts - Add check-dependencies.sh script for dependency health monitoring All endpoints now have appropriate rate limiting protection. Sensitive routes (setup, settings) use stricter limits. All TypeScript and ESLint checks pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/server/src/index.ts (1)
101-107: CORS default still mismatches documented UI port.The server defaults to
http://localhost:3008whenCORS_ORIGINis unset, but if the UI runs on port 3000 as documented in the README, fresh development setups will encounter CORS failures. Consider defaulting tohttp://localhost:3000or supporting a comma-separated list of origins to align with the documented development workflow.apps/server/src/routes/setup/routes/install-claude.ts (1)
71-88: Network error detection may produce false positives.The check
stderr.includes('curl')could trigger false positives if "curl" appears in any error output (e.g., path names, version info). Similarly, generic terms like "connection" and "network" may match unrelated messages. Consider using more specific error patterns.🔎 Proposed fix
// Check for network errors const hasNetworkError = - stderr.includes('curl') || - stderr.includes('connection') || - stderr.includes('network') || + stderr.includes('could not resolve') || + stderr.includes('connection refused') || + stderr.includes('network unreachable') || + stderr.includes('connection timed out') || stderr.includes('failed to download');
🧹 Nitpick comments (3)
scripts/check-dependencies.sh (1)
84-84: Add explicit file existence check.While the current implementation handles a missing
package-lock.jsonvia2>/dev/null, an explicit existence check would be clearer and more defensive.🔎 Make the check more explicit
-if grep -q 'git+ssh://' package-lock.json 2>/dev/null; then +if [ -f package-lock.json ] && grep -q 'git+ssh://' package-lock.json; then echo "⚠️ package-lock.json contains git+ssh:// URLs" echo "💡 Run 'npm run fix:lockfile' to fix" FAILED=1 +elif [ ! -f package-lock.json ]; then + echo "⚠️ package-lock.json not found" + FAILED=1 else echo "✅ Lockfile is properly formatted" fiapps/server/src/routes/setup/routes/install-claude.ts (2)
37-46: Remove redundant catch-and-rethrow pattern.The
.catch()block logs the error and re-throws it, but the outercatchblock at line 153 will handle all errors from the try block anyway. This creates duplicate error logging.🔎 Proposed fix
-const result = await spawnProcess({ - command, - args, - cwd, - timeout: 300000, -}).catch((error) => { - // Handle process spawn failures - console.error('[Install Claude] Failed to spawn process:', error); - throw error; -}); +const result = await spawnProcess({ + command, + args, + cwd, + timeout: 300000, +});
158-158: Remove redundant platform detection.The
isWindowsvariable is already declared at line 14 and is accessible throughout the function scope. No need to redefine it here.🔎 Proposed fix
logError(error, 'Install Claude CLI failed'); const errorMessage = getErrorMessage(error); // Check if the error is because command wasn't found -const isWindows = platform() === 'win32'; if ( errorMessage.includes('spawn') ||
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/server/src/index.tsapps/server/src/lib/rate-limiter.tsapps/server/src/routes/setup/routes/install-claude.tsscripts/check-dependencies.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/lib/rate-limiter.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/server/src/routes/**
📄 CodeRabbit inference engine (CLAUDE.md)
API routes should be placed in
apps/server/src/routes/, with one file per route/resource
Files:
apps/server/src/routes/setup/routes/install-claude.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Run type checking withnpm run typecheckbefore syncing the Beads database as part of quality gates
Run linting withnpm run lintbefore syncing the Beads database as part of quality gates
Files:
apps/server/src/routes/setup/routes/install-claude.tsapps/server/src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Applies to apps/server/src/routes/** : API routes should be placed in `apps/server/src/routes/`, with one file per route/resource
Applied to files:
apps/server/src/index.ts
🧬 Code graph analysis (2)
apps/server/src/routes/setup/routes/install-claude.ts (3)
init.mjs (1)
isWindows(33-33)apps/server/src/routes/github/routes/common.ts (2)
logError(23-25)getErrorMessage(16-21)apps/server/src/routes/worktree/common.ts (1)
logError(111-111)
apps/server/src/index.ts (2)
apps/server/src/lib/auth.ts (2)
initializeAuth(21-31)authMiddleware(39-66)apps/server/src/lib/rate-limiter.ts (4)
healthLimiter(56-65)apiLimiter(35-44)strictLimiter(77-86)beadsLimiter(98-107)
🔇 Additional comments (6)
apps/server/src/index.ts (4)
18-19: LGTM! Security imports properly added.The authentication and rate limiting imports are correctly structured and align with the enhanced security features being implemented throughout the server.
87-90: LGTM! Authentication initialization properly sequenced.The security initialization is well-positioned early in the startup flow, and the call to
initializeAuth()correctly follows path validation.
151-156: LGTM! CORS middleware now uses validated origin.Using the
CORS_ORIGINconstant ensures the origin has been validated throughvalidateCorsOrigin()before being applied to the middleware.
180-207: Excellent! Rate limiting now applied consistently across all API routes.All 21 API endpoints now have appropriate rate limiting:
- General routes use
apiLimiter(100 requests per 15 minutes)- Sensitive routes (
/api/setup,/api/settings) usestrictLimiter(5 requests per minute)- High-throughput route (
/api/beads) usesbeadsLimiter(200 requests per 15 minutes)- Health check uses
healthLimiter(10 requests per minute)This comprehensively addresses the previous concern about 17 routes lacking rate limiting protection. The chosen limits are appropriate for each route's sensitivity and expected usage patterns.
apps/server/src/routes/setup/routes/install-claude.ts (2)
1-9: LGTM! Clean imports.All imports are properly typed and utilized throughout the implementation.
90-142: LGTM! Robust verification with sensible retry strategy.The verification logic properly handles PATH update delays with exponential backoff retries. The decision to return success even when verification fails (but install succeeded) is pragmatic, as it accounts for real-world scenarios where a terminal restart is needed for PATH updates.
| const cwd = | ||
| process.env.HOME || process.env.USERPROFILE || (isWindows ? path.resolve('C:\\') : '/tmp'); |
There was a problem hiding this comment.
Improve working directory fallback for edge cases.
The current fallback to C:\ on Windows and /tmp on non-Windows systems may cause permission issues or unexpected behavior. Consider using os.tmpdir() or process.cwd() as a safer fallback.
🔎 Proposed fix
+import { tmpdir } from 'os';
+
// Determine working directory (use home directory)
const cwd =
- process.env.HOME || process.env.USERPROFILE || (isWindows ? path.resolve('C:\\') : '/tmp');
+ process.env.HOME || process.env.USERPROFILE || tmpdir();🤖 Prompt for AI Agents
In apps/server/src/routes/setup/routes/install-claude.ts around lines 17-18, the
current cwd fallback uses C:\ on Windows and /tmp on non-Windows which can cause
permission/unexpected issues; replace that fallback with a safe approach using
Node's os.tmpdir() and/or process.cwd(): import/require the 'os' module, prefer
process.cwd() if available (and writable) then fall back to os.tmpdir(), and use
that result instead of hardcoded C:\ or /tmp so the app uses a safer,
platform-appropriate temporary/work directory.
|
|
||
| if [ -n "$OUTDATED" ] && [ "$OUTDATED" != "{}" ]; then | ||
| echo "⚠️ Found outdated packages:" | ||
| echo "$OUTDATED" | jq -r 'to_entries[] | " - \(.key): current \(.value.current), latest \(.value.latest)"' 2>/dev/null || echo "$OUTDATED" |
There was a problem hiding this comment.
Add prerequisite check for jq.
The script relies heavily on jq for JSON parsing but never verifies it's installed. If jq is missing, the script will fail with cryptic errors or produce unexpected output.
🔎 Add jq availability check at script start
Add this check after line 13:
FAILED=0
+
+# Check for required tools
+if ! command -v jq &> /dev/null; then
+ echo "❌ Error: jq is required but not installed"
+ echo "💡 Install jq: sudo apt-get install jq (Ubuntu/Debian) or brew install jq (macOS)"
+ exit 1
+fi
# Check for outdated packagesAlso applies to: 34-34, 37-37, 41-41, 69-69
🤖 Prompt for AI Agents
In scripts/check-dependencies.sh around lines 21 (and also apply same fix at
lines 34, 37, 41, 69), the script uses jq without verifying it's installed; add
a prerequisite check near the top (after line 13) that tests for jq (e.g.,
command -v jq >/dev/null 2>&1) and if missing prints a clear error to stderr and
exits with non-zero status; ensure this early check prevents later cryptic
failures and document the dependency in the script header or README.
1 participant
See commit details
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.