Bug fixes and stability improvements #815
Merged
gsxdsm merged 31 commits intoAutoMaker-Org:v1.0.0rcfrom Feb 28, 2026
Merged
Conversation
The CopilotProvider was using incorrect event type and data structure for tool execution completion events from the @github/copilot-sdk, causing tool call outputs to be empty. Changes: - Update event type from 'tool.execution_end' to 'tool.execution_complete' - Fix data structure to use nested result.content instead of flat result - Fix error structure to use error.message instead of flat error - Add success field to match SDK event structure - Add tests for empty and missing result handling This aligns with the official @github/copilot-sdk v0.1.16 types defined in session-events.d.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code review improvements: - Extract magic string '[ERROR]' to TOOL_ERROR_PREFIX constant - Add null-safe error handling with direct error variable assignment - Include error codes in error messages for better debugging - Add JSDoc documentation for tool.execution_complete handler - Update tests to verify error codes are displayed - Add missing tool_use_id assertion in error test These changes improve: - Code maintainability (no magic strings) - Debugging experience (error codes now visible) - Type safety (explicit null checks) - Test coverage (verify error code formatting) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes provider/context resolution and threads providerId through server and UI; adds async, persistent DevServerService with per-worktree start tracking and new dev-server:starting event; improves worktree branch normalization and detached‑HEAD recovery; expands model-prefix validation exceptions; and lands wide UI, type, and test infra updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Board/UI
participant Settings as SettingsService
participant Resolver as settings-helpers
participant Provider as ProviderFactory
participant Agent as AgentExecutor
UI->>Resolver: resolveProviderContext(modelId, providerId?)
Resolver->>Settings: load global/project settings & credentials
Resolver->>Provider: map model / resolve provider and modelConfig
Resolver-->>UI: { provider, credentials, resolvedModel, modelConfig }
UI->>Agent: runAgentFn({ model: resolvedModel, providerId })
Agent->>Provider: executeQuery(options with provider credential)
sequenceDiagram
participant Server as apps/server
participant Dev as DevServerService
participant Disk as dev-servers.json
participant Emitter as EventEmitter
Server->>Dev: initialize(dataDir, emitter)
Dev->>Disk: loadState()
Disk-->>Dev: persisted entries
Dev->>Dev: reattach ports / prune stale
Dev-->>Emitter: dev-server:starting (worktreePath, timestamp)
Dev->>Disk: saveState()
Dev-->>Emitter: dev-server:started / dev-server:url-detected
sequenceDiagram
participant UI as Worktree List UI
participant API as worktree list route
participant Git as git CLI / execGitCommand
participant Resolver as WorktreeResolver
UI->>API: GET /api/worktrees
API->>Git: git worktree list --porcelain
alt detached HEAD
API->>Resolver: recoverBranchForDetachedWorktree(path)
Resolver->>Git: read .git/rebase-merge/head-name
Resolver->>Git: read .git/rebase-apply/head-name (fallback)
Resolver-->>API: recoveredBranch or "(detached)"
else normal
API->>Resolver: normalizeBranchName(from porcelain)
Resolver-->>API: normalizedBranch
end
API-->>UI: worktree list with branch, isDetached flags
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Summary of ChangesHello @gsxdsm, 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 significantly enhances the stability and reliability of the application by addressing several critical bugs and introducing key architectural improvements. The changes focus on improving the handling of Git worktrees, ensuring dev server state persistence across restarts, and centralizing model and provider resolution logic. These updates contribute to a more robust and predictable user experience, particularly in scenarios involving Git operations and long-running development servers, while also refining the UI's feedback mechanisms for complex states like merge conflicts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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}
Contributor
There was a problem hiding this comment.
Code Review
This is an excellent pull request that delivers a wide range of valuable bug fixes and stability improvements across both the server and UI. The enhancements to the DevServerService to persist state and handle race conditions are particularly strong. The worktree handling for detached HEAD states is a clever and important fix for UI stability. Architecturally, the introduction of the centralized resolveProviderContext helper and the refactoring of WebSocket logic into the useAgentOutputWebSocket hook are great improvements. The addition of comprehensive unit and integration tests for these complex features is commendable and significantly boosts confidence in the changes. I have one minor suggestion regarding a validation function's logic, but overall, this is a high-quality contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ui/src/components/views/file-editor-view/file-editor-view.tsx (1)
697-746:⚠️ Potential issue | 🟠 MajorAdd
providerIdto thefeatureDatatype definition.Line 734 accesses
featureData.providerId, but the type definition forfeatureData(lines 698-714) does not include aproviderIdproperty. TheAddFeatureDialogcomponent passes aFeatureDataobject that includesproviderId?: string, creating a type mismatch. Theas anycast on line 749 currently masks this error.🐛 Proposed fix to add providerId to the type
async (featureData: { title: string; category: string; description: string; priority: number; model: string; thinkingLevel: string; reasoningEffort: string; + providerId?: string; skipTests: boolean; branchName: string; planningMode: string; requirePlanApproval: boolean; excludedPipelineSteps?: string[]; workMode: string; imagePaths?: Array<{ id: string; path: string; description?: string }>; textFilePaths?: Array<{ id: string; path: string; description?: string }>; }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 697 - 746, The featureData parameter type for handleAddFeatureFromEditor is missing providerId, causing a type mismatch; update the inline featureData type (the object literal in the useCallback parameter) to include providerId?: string (optional) so featureData.providerId is correctly typed, and ensure the constructed feature object continues to set providerId from featureData; remove any workaround casts that were masking this error (e.g., the as any) after the type is fixed.apps/server/src/routes/worktree/routes/list.ts (1)
442-504:⚠️ Potential issue | 🟠 MajorLast worktree entry can be dropped when porcelain output lacks a trailing separator.
The parser only flushes on
line === '', so a final non-terminated entry is silently skipped.Proposed fix
- const lines = stdout.split('\n'); + const lines = [...stdout.split('\n'), ''];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/list.ts` around lines 442 - 504, The parser currently only flushes a worktree when it encounters an empty line, so the final worktree is lost if stdout lacks a trailing blank; after the for-loop in list.ts (where variables current, isFirst, removedWorktrees, worktrees, currentBranch and secureFs are in scope), add a final flush that runs the same logic as the empty-line branch: treat isMainWorktree = isFirst, check existence with secureFs.access, push to removedWorktrees if not main and missing and branch present, otherwise push to worktrees (handling detached via recoverBranchForDetachedWorktree(current.path) when current.isDetached) and update isFirst accordingly. Ensure you only flush when current.path or current.branch or current.isDetached is set so empty state is ignored.apps/ui/src/components/views/github-issues-view.tsx (1)
176-192:⚠️ Potential issue | 🟠 MajorMissing
providerIdin function parameter type definition.The
featureDataparameter type at lines 176-192 does not includeproviderId, but line 212 accessesfeatureData.providerId. This will result inproviderIdbeingundefinedat runtime since TypeScript won't catch this due to the@ts-nocheckdirective at line 1.🐛 Proposed fix: Add providerId to the type definition
async (featureData: { title: string; category: string; description: string; priority: number; model: string; thinkingLevel: string; reasoningEffort: string; + providerId?: string; skipTests: boolean; branchName: string; planningMode: string; requirePlanApproval: boolean; excludedPipelineSteps?: string[]; workMode: string; imagePaths?: Array<{ id: string; path: string; description?: string }>; textFilePaths?: Array<{ id: string; path: string; description?: string }>; }) => {Also applies to: 212-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-issues-view.tsx` around lines 176 - 192, The featureData parameter type for the async handler is missing providerId, causing featureData.providerId to be undefined at runtime; update the parameter type (the object passed into the async function where featureData is declared) to include providerId: string (or the correct type used elsewhere) so any access to featureData.providerId (referenced later around the call at line where featureData.providerId is used) is properly typed and present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/list.ts`:
- Around line 441-443: The code currently skips adding missing detached
worktrees because it only pushes entries into removedWorktrees when a branch
exists; update the parsing logic around the variables removedWorktrees and
current so that if a missing worktree line yields a path (current.path) but no
branch (detached), you still push an entry into removedWorktrees (e.g., use
current.branch ?? '<detached>' or empty string) and preserve current.isDetached
flag; ensure the subsequent prune/cleanup logic that calls git worktree prune
runs whenever removedWorktrees is non-empty (i.e., detect missing worktrees by
current.path presence, not by branch existence).
In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 31-36: Remove the unused imports getProviderById and
getProviderByModelId from the import list in this file (they are no longer
referenced; resolveProviderContext is used instead). Update the import block
where getProviderById and getProviderByModelId are declared so only the needed
symbols (e.g., resolveProviderContext, getMCPServersFromSettings,
getDefaultMaxTurnsSetting) are imported; ensure there are no leftover references
or comments that imply these functions are used elsewhere in this file (remove
or update the comment on line mentioning them if applicable).
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 859-865: The fallback URL host should not use
process.env.HOSTNAME; update the code constructing the DevServerInfo URL (the
hostname constant and the serverInfo.url assignment used when creating
serverInfo) to always use 'localhost' as the fallback host (and similarly
replace any other fallbacks around the detectUrlFromOutput logic referenced
later on lines ~996-997) so that the initial generated URL is routable when
regex detection fails; keep the rest of serverInfo (worktreePath, allocatedPort,
port, process) unchanged and allow detectUrlFromOutput to overwrite the URL if
it finds a better one.
- Around line 229-233: Reattached DevServerInfo entries are stored with process:
null so stopDevServer() can't signal the real process; update stopDevServer() to
detect when server.process is null and perform a fallback kill-by-port using the
DevServerInfo.port (or other identifying socket) to find and terminate the
process (e.g., via a platform-appropriate lsof/fuser or a kill-port utility),
then clean up state as usual; reference DevServerInfo, the property process, and
stopDevServer() so you add the fallback branch that attempts to locate PIDs
bound to server.port, kill them, and remove the server entry when process handle
is unavailable.
- Around line 755-761: The dev-server:starting event is emitted too early via
this.emitter.emit('dev-server:starting') and can be left dangling on
preflight/port validation failures; move the
this.emitter.emit('dev-server:starting', { worktreePath, timestamp }) call so it
runs only after all preflight checks/port validation succeed (i.e., after the
validation/ensure-port steps complete) or alternatively add a matching clearing
event on every early return path so subscribers are always informed of failure;
update the logic around the preflight/port-check code paths to either relocate
the emit or ensure every return emits a terminal event.
In `@apps/server/tests/unit/lib/file-editor-store-logic.test.ts`:
- Around line 22-47: Tests duplicate the store's dirty-state logic by
reimplementing computeIsDirty, updateTabContent, and markTabSaved in the test
file, so fix by removing these local implementations and importing the real
functions from the production module (or extract the shared pure helpers from
the store and import them into both places); update tests to call the production
store functions (e.g., computeIsDirty, updateTabContent, markTabSaved) or the
exported helpers and assert their behavior instead of the duplicated copies.
- Around line 337-355: The test uses a strict wall-clock assertion on duration
which is flaky on CI; modify the test in file
tests/unit/lib/file-editor-store-logic.test.ts to remove or relax the timing
assertion: locate the test using updateTabContent (the it block that generates
largeOriginal/largeModified and measures performance.now()), and either delete
the expect(duration).toBeLessThan(100) assertion, or replace it with a
non-deterministic-friendly check (e.g., assert duration is a finite number or
use a very loose threshold like <5000), or move this timing check to a separate
benchmark/perf-only test harness; keep the functional assertions
(expect(tab.isDirty).toBe(true)) intact.
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 1153-1164: The current prUrl assignment is race-prone because it
diffs feature sets to find the new feature and also doesn't await/save errors
from persistence; change handleAddAndStartFeature to return the created feature
id (e.g., Promise<string|null>) so callers receive the explicit newFeatureId,
then replace the set-diff lookup with that id and call
updateFeature(newFeatureId, { prUrl }) followed by await
persistFeatureUpdate(newFeatureId, { prUrl }) wrapped in a try/catch to
surface/log failures; update call sites that invoke handleAddAndStartFeature to
use the returned id and handle null appropriately.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-mobile-dropdown.tsx`:
- Around line 115-117: The Globe icon can render when devServerInfo is undefined
because devServerInfo?.urlDetected !== false evaluates to true for undefined;
update the render condition to only show the icon when url detection is
explicitly true (e.g. change the check in the JSX where Globe is rendered to use
devServerRunning && devServerInfo?.urlDetected === true or
Boolean(devServerInfo?.urlDetected)) so the Globe only appears after explicit
detection; modify the conditional around the Globe component in
worktree-mobile-dropdown.tsx (referencing devServerRunning, devServerInfo, and
Globe) accordingly.
In `@apps/ui/src/components/views/spec-view/hooks/use-spec-generation.ts`:
- Line 423: The effect that sets up the streaming event listener (useEffect with
dependencies [currentProject, loadSpec, errorMessage, currentPhase]) is
re-subscribing whenever errorMessage or currentPhase update; remove errorMessage
and currentPhase from that dependency array so the listener isn’t torn down on
each state change, and create refs (e.g., errorMessageRef and currentPhaseRef)
that are kept in sync (assign in appropriate places where
errorMessage/currentPhase are set) so the streaming callback inside the effect
can read the latest values from those refs instead of the state variables;
update the callback used by the streaming listener (the function that reads
errorMessage/currentPhase at the conditional checks around the lines you noted)
to reference errorMessageRef.current and currentPhaseRef.current for its
decisions, while keeping currentProject and loadSpec as dependencies.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 2429-2430: The select-mode overlay condition only renders for
mobile (isSelectMode && isMobile) while the shortcuts bar (showShortcutsBar) and
the select toggle (onToggleSelectMode) are shown on tablets, so tapping the
toggle on tablets does nothing; update the overlay rendering to include tablets
(change condition to isSelectMode && (isMobile || isTablet)) or alternatively
hide/disable the select-mode toggle on tablet by adjusting the shortcuts
bar/toggle render condition (showShortcutsBar or the toggle button render) to
exclude isTablet so the button isn't shown when the overlay won't render.
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Around line 286-338: The restore loop is applying saved isRunning flags for
every branch in the project, which can leave non-active worktrees marked
running; change restore logic to only target the current worktree: compute the
session key for the currently active branch (use the same MAIN_WORKTREE_MARKER
mapping for null/main) and check only that key in readAutoModeSession() instead
of iterating all entries, then call setAutoModeRunning(currentProject.id,
branchName, true) only when that single key exists and is true; use symbols
readAutoModeSession, SESSION_KEY_DELIMITER/MAIN_WORKTREE_MARKER mapping logic,
getWorktreeSessionKey, currentProject and the current branch identifier (e.g.,
currentBranch or equivalent in this hook), and keep the existing error logging.
In `@apps/ui/src/hooks/use-test-runners.ts`:
- Around line 93-101: The two useMemo hooks for activeSession and isRunning are
missing sessions in their dependency arrays, causing stale values when sessions
change; update the dependency arrays for the activeSession memo (which calls
getActiveSession) and the isRunning memo (which calls isWorktreeRunning) to
include sessions alongside worktreePath and the respective store functions so
they recompute when sessions state updates.
In `@apps/ui/tests/features/success-log-contrast.spec.ts`:
- Around line 257-258: The two tests that click the close button
('[data-testid="close-agent-output-modal"]') lack a follow-up assertion to
confirm the modal is actually closed; after the click in both places (the test
near the existing click at that location and the second occurrence around lines
303-304), add an assertion against the modal locator (e.g. the agent output
modal selector such as '[data-testid="agent-output-modal"]' or the locator used
elsewhere in this spec) to verify it is hidden or not visible (e.g.
expect(...).toBeHidden()/toBeVisible(false) or expect(...).toHaveCount(0)) so
the tests fail if the close action regresses.
In `@apps/ui/tests/unit/components/agent-output-modal-constants.test.ts`:
- Around line 7-8: Playwright is discovering Vitest unit tests under ./tests
causing CJS/ESM conflicts; update the Playwright config (playwright.config.ts)
for the apps/ui project to exclude the unit tests by adding a testIgnore pattern
such as '**/unit/**' or narrow testDir to only e2e folders (e.g.,
'./tests/e2e'), or alternatively move the unit tests (like
tests/unit/components/agent-output-modal-constants.test.ts) out of the
Playwright test tree so Playwright won’t attempt to load Vitest ESM files.
In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx`:
- Around line 317-323: The test sets window.__currentProject but never restores
it, risking cross-test leakage; update the test file to reset this global by
either adding an afterEach that deletes or restores window.__currentProject or
modify this specific test to save the original value (const prev =
window.__currentProject), set the fallback, then in a finally or after assertion
restore window.__currentProject = prev (or delete it if prev was undefined);
target the test referencing AgentOutputModal and the spec block containing
"should fallback to window.__currentProject when projectPath is not provided".
In `@apps/ui/tests/unit/components/card-actions.test.tsx`:
- Around line 1-4: Playwright is picking up Vitest unit tests and failing;
update the Playwright config (where testDir is set) to add a testIgnore pattern
that excludes the Vitest unit tests (e.g. add testIgnore:
['apps/ui/tests/unit/**'] or equivalent glob) so Playwright will not attempt to
run files under the unit tests directory; ensure the pattern is applied to the
exported config object used by the CI runner.
In `@apps/ui/tests/unit/components/mobile-terminal-shortcuts.test.tsx`:
- Around line 53-63: The defaultProps object uses an invalid value 'none' for
activeModifier which must be of type StickyModifier ('ctrl' | 'alt' | null);
update defaultProps.activeModifier to null and ensure any tests or usages
expecting 'none' are adjusted to handle null (look for defaultProps and
activeModifier in mobile-terminal-shortcuts.test.tsx and the component that
consumes activeModifier).
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 182-188: The assertions in the responsive-modal test compare
computed styles to CSS declarations (e.g., expecting "90vw" or "calc(...)")
which will fail because window.getComputedStyle returns resolved pixel values;
update the checks in the block that handles fromViewport/toViewport to parse
numeric pixels from newDimensions.width and newDimensions.maxWidth (e.g.,
extract parseFloat from strings like "921.6px") and assert against expected
pixel ranges or use toBeCloseTo with calculated pixel targets for mobile→tablet
and tablet→mobile transitions; alternatively, assert responsive behavior by
checking a deterministic indicator (class name or applied breakpoint state)
instead of raw CSS text—adjust the expectations where newDimensions is compared
so they validate resolved pixel numbers or breakpoint flags rather than CSS
expressions.
---
Outside diff comments:
In `@apps/server/src/routes/worktree/routes/list.ts`:
- Around line 442-504: The parser currently only flushes a worktree when it
encounters an empty line, so the final worktree is lost if stdout lacks a
trailing blank; after the for-loop in list.ts (where variables current, isFirst,
removedWorktrees, worktrees, currentBranch and secureFs are in scope), add a
final flush that runs the same logic as the empty-line branch: treat
isMainWorktree = isFirst, check existence with secureFs.access, push to
removedWorktrees if not main and missing and branch present, otherwise push to
worktrees (handling detached via recoverBranchForDetachedWorktree(current.path)
when current.isDetached) and update isFirst accordingly. Ensure you only flush
when current.path or current.branch or current.isDetached is set so empty state
is ignored.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 697-746: The featureData parameter type for
handleAddFeatureFromEditor is missing providerId, causing a type mismatch;
update the inline featureData type (the object literal in the useCallback
parameter) to include providerId?: string (optional) so featureData.providerId
is correctly typed, and ensure the constructed feature object continues to set
providerId from featureData; remove any workaround casts that were masking this
error (e.g., the as any) after the type is fixed.
In `@apps/ui/src/components/views/github-issues-view.tsx`:
- Around line 176-192: The featureData parameter type for the async handler is
missing providerId, causing featureData.providerId to be undefined at runtime;
update the parameter type (the object passed into the async function where
featureData is declared) to include providerId: string (or the correct type used
elsewhere) so any access to featureData.providerId (referenced later around the
call at line where featureData.providerId is used) is properly typed and
present.
---
Nitpick comments:
In `@apps/server/src/lib/settings-helpers.ts`:
- Around line 804-812: The dynamic import of resolveModelString inside the block
that checks modelConfig.mapsToClaudeModel causes repeated module loads; instead
move the import to the module top-level (import { resolveModelString } from
'@automaker/model-resolver') and update the references in this file (the
resolveModelString usage in the mapsToClaudeModel branch and the duplicate
dynamic import used by getProviderByModelId) to use the top-level import,
ensuring you remove the in-function await import(...) calls so both code paths
reuse the same module instance.
- Around line 784-802: The comment above the fallback loop is misleading because
the loop explicitly skips the provider with p.id === providerId (so it does not
re-check an explicitly-selected provider); update the comment to accurately
describe behavior: say that the loop searches other enabled providers for
modelConfig (skipping the already-checked explicit provider), and that we only
override provider when none was explicitly selected; reference the surrounding
symbols (providers, providerId, provider, modelConfig, isProviderEnabled,
findModelInProvider, logger.debug, logPrefix) so the intent is clear.
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 742-745: The comment is misleading about the codex prefix; update
the inline comment above the validateBareModelId call so it states that
validateBareModelId(options.model, 'CodexProvider', 'codex') allows/handles a
'codex' provider prefix (it will be treated/stripped by the validator) while
other provider prefixes should be stripped by AgentService before calling
providers; reference validateBareModelId, options.model, 'CodexProvider', and
'codex' in the comment to make the actual behavior explicit.
In `@apps/server/src/providers/gemini-provider.ts`:
- Around line 549-550: The comment above the call to
validateBareModelId(options.model, 'GeminiProvider', 'gemini') is misleading;
update it to clearly state that models must not include a provider prefix and
that 'gemini' is explicitly allowed here (i.e., explain that the function
enforces no provider prefix except the allowed 'gemini' provider identifier),
and place this clarified comment immediately above the validateBareModelId call
near the GeminiProvider logic.
In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 321-325: The debug log in createRunAgentFn that warns when sdkOpts
is undefined should be hardened: change the logger level to warn or error and
immediately fail-fast instead of continuing; e.g., after logging in the
createRunAgentFn block checking sdkOpts (and referencing createAutoModeOptions)
either throw a descriptive Error or return early with a rejected Promise so
downstream code won’t run without sdkOpts. Ensure the thrown error message
includes context (createRunAgentFn, createAutoModeOptions, sdkOpts) to aid
debugging.
In `@apps/server/tests/unit/services/auto-mode/facade-agent-runner.test.ts`:
- Around line 162-206: The mock setup has a mismatch between
mockClaudeProvider.models[0].mapsToClaudeModel ('claude-3-opus') and the
resolvedModel returned by settingsHelpers.resolveProviderContext
('claude-3-5-opus'), which makes the test unclear; either update the
mapsToClaudeModel to 'claude-3-5-opus' to match resolvedModel or add a brief
inline comment near the settingsHelpers.resolveProviderContext mock explaining
that the test intentionally uses a different resolvedModel to simulate
resolution logic, and ensure assertions against sdkOptions.createAutoModeOptions
and mockAgentExecutor.execute still reflect the intended mapping behavior.
In `@apps/server/tests/unit/services/dev-server-persistence.test.ts`:
- Around line 196-204: Replace the arbitrary 100ms timeout in the test with a
deterministic wait: either have the service under test expose/return a Promise
that resolves when the pruning/saveState operation completes and await that, or
implement a short polling/retry loop that repeatedly reads path
'dev-servers.json' (using fs.readFile) and parses JSON until the expected
condition (state.length === 0) is met or a reasonable timeout is reached; update
the test to use that promise or polling loop instead of await new
Promise((resolve) => setTimeout(resolve, 100)).
- Around line 218-227: Replace the fixed 300ms sleep in the test with a
deterministic wait: instead of "await new Promise((resolve) =>
setTimeout(resolve, 300));" poll the persisted file (path.join(testDataDir,
'dev-servers.json')) or call the service's persistence API until the expected
condition is met (e.g., parsed state[0].urlDetected === true or state[0].url ===
'http://localhost:5555/'), looping with a short delay and a reasonable overall
timeout (e.g., 2–5s) and fail the test if the timeout elapses; alternatively, if
the persistence service exposes a completion hook (e.g.,
flush/saveQueue/awaitPersistence), call that directly to await completion before
reading the file.
In
`@apps/server/tests/unit/services/pipeline-orchestrator-status-provider.test.ts`:
- Around line 206-210: The test uses a fragile hardcoded index when extracting
the options arg from mockRunAgentFn (mockRunAgentFn.mock.calls[0][7]); replace
this with a stable approach: declare a named constant like OPTIONS_ARG_INDEX = 7
(or compute the index via runAgentFn signature comments) and use
mockRunAgentFn.mock.calls[0][OPTIONS_ARG_INDEX], or alternatively retrieve the
last call via mockRunAgentFn.mock.lastCall and locate the options argument by
position with that constant; update the assertion lines that reference options
to use that constant and add a short comment documenting the runAgentFn
signature and what index represents the options parameter.
- Around line 264-270: The test sets an explicit 15s timeout for a unit test
that uses fully mocked dependencies; change the timeout on the test that calls
orchestrator.executeTestStep (and the other test instance referenced) to a much
smaller value or remove it so the Jest default applies—locate the test invoking
orchestrator.executeTestStep and using mockRunAgentFn and replace the 15000
timeout with a lower value (e.g., 2000) or delete the timeout argument for both
occurrences to tighten the test timing.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 594-599: The handler handleModelChange currently calls
normalizeThinkingLevelForModel directly, creating a second normalization path;
change handleModelChange to call normalizeModelEntry instead (passing the
PhaseModelEntry) and then call setModelEntry with the returned normalized entry
so both handleModelChange and handleSubmit use the same normalization logic
provided by normalizeModelEntry; reference normalizeModelEntry,
normalizeThinkingLevelForModel, handleModelChange, handleSubmit, setModelEntry
and PhaseModelEntry when locating the code to update.
In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts`:
- Line 70: Remove the unused destructured variable `_projects` from the object
destructuring in use-navigation (hooks/use-navigation.ts) to reduce noise;
locate the destructuring that currently contains "projects: _projects" and
delete that entry so the code only extracts the truly used props (e.g., keep
"projects" out entirely or use the existing "projects" name if needed later),
ensuring no references to `_projects` remain elsewhere in the module.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx`:
- Around line 31-75: The merge conflict badge (render guarded by
showMergeConflict, using mergeConflictOffsetClass and uniformBadgeClass) is
absolutely positioned while the error badge (rendered when feature.error is
truthy) is in the normal flow, causing overlap; make their positioning
consistent by moving the error badge into the same absolute positioning scheme
(or conversely make the merge conflict badge use the same flex/layout as the
error badge) — e.g., render the error badge inside a container that reuses
mergeConflictOffsetClass and the same absolute/top-2 wrapper (or change the
merge conflict wrapper to the flex container) so both badges share the same
positioning approach and visual offsets.
In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx`:
- Around line 261-263: Replace the hardcoded threshold used in handleScroll with
the centralized constant to keep behavior consistent: locate the handleScroll
function (and any other scroll handlers) and replace the literal 50 with
MODAL_CONSTANTS.AUTOSCROLL_THRESHOLD so both the isAtBottom calculation and
handleScroll use the same value (refer to setSummaryAutoScroll and the
isAtBottom computation that already uses MODAL_CONSTANTS.AUTOSCROLL_THRESHOLD).
In `@apps/ui/src/components/views/board-view/dialogs/stash-changes-dialog.tsx`:
- Around line 512-520: Replace the inline platform-detection expression with the
shared isMac utility: import the exported isMac constant from '@/lib/utils' and
use it in the JSX expression (replace the long navigator check in
stash-changes-dialog.tsx with isMac ? '⌘' : 'Ctrl'), removing the duplicated
logic and keeping behavior unchanged.
In `@apps/ui/src/components/views/board-view/hooks/use-board-column-features.ts`:
- Around line 31-68: Remove the unused prevFeatureIdsRef: delete the declaration
const prevFeatureIdsRef = useRef<Set<string>>(new Set()); and the assignment
prevFeatureIdsRef.current = currentIds; inside the useEffect since its value is
never read; keep the rest of the effect (features, recentlyCompletedFeatures,
clearRecentlyCompletedFeatures) unchanged so clearing logic remains intact.
In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx`:
- Around line 490-492: Replace the dynamic toast title with a stable, fixed
title when stop-tests fails: instead of passing result.error as the first
argument to toast.error, call toast.error with a fixed string like "Failed to
stop tests" and keep result.error (or a fallback like "Unknown error") in the
description option; update the call site that currently reads
toast.error(result.error || 'Failed to stop tests', { description: result.error
|| 'Unknown error' }) so the title is constant while the description contains
the backend error.
In `@apps/ui/src/components/views/github-issues-view/hooks/use-issues-filter.ts`:
- Line 198: The lint suppression exists because
hasActiveFilterCheck(filterState) depends on specific fields of filterState;
refactor hasActiveFilterCheck to accept the exact destructured properties (e.g.,
pass filterState.query, filterState.labels, etc.) and update its signature
accordingly, then call it with those individual values from useIssuesFilter
(replace hasActiveFilterCheck(filterState) with hasActiveFilterCheck(query,
labels, ...) ) and remove the ESLint disable; apply the same change for the
other occurrences around lines 230-240 so dependency arrays no longer require
the disable.
In
`@apps/ui/src/components/views/project-settings-view/project-bulk-replace-dialog.tsx`:
- Around line 229-230: generatePreviewItem is recreated each render and the
eslint-disable-next-line comment hides a real dependency issue; wrap
generatePreviewItem in useCallback (or inline its logic into the existing
useMemo that computes preview data) so it becomes stable, remove the //
eslint-disable-next-line react-hooks/exhaustive-deps line, and then include
generatePreviewItem in the useEffect dependency array where it’s currently used
(the effect surrounding the comment) so the hook deps are correct.
In `@apps/ui/src/components/views/running-agents-view.tsx`:
- Line 25: The logger is being created inside the RunningAgentsView component on
every render via createLogger('RunningAgentsView'), making the "logger is
stable" ESLint disable inaccurate; move the logger creation to module scope by
adding a top-level const logger = createLogger('RunningAgentsView') outside the
RunningAgentsView function (and remove the ESLint disable comments), and apply
the same change for the other in-component createLogger calls referenced (the
occurrences around the other noted lines) so the logger instances are truly
stable across renders.
In
`@apps/ui/src/components/views/settings-view/model-defaults/bulk-replace-dialog.tsx`:
- Around line 217-224: generatePreviewItem is an un-memoized function being
listed in the useMemo dependency array for computing preview, which causes
preview to recompute every render; wrap generatePreviewItem in useCallback
(e.g., const generatePreviewItem = useCallback(..., [selectedProviderConfig,
enabledProviders, /* any other real deps */])) so it has a stable identity, then
keep it in the useMemo deps (and remove the eslint-disable-next-line
react-hooks/exhaustive-deps comment) so preview is correctly memoized; ensure
the useCallback dependency list matches the values used inside
generatePreviewItem (such as selectedProviderConfig and enabledProviders).
In
`@apps/ui/src/components/views/settings-view/providers/opencode-model-configuration.tsx`:
- Around line 265-281: The dynamicProviderOrder array is created with
useMemo([]) but is constant; hoist it to a module-level constant (e.g., place it
near DYNAMIC_PROVIDER_CONFIG) and remove the useMemo call inside the component,
then update references (dynamicProviderOrder and any use of
sortedDynamicProviders) to use the new constant and clean up dependencies
accordingly so the component no longer allocates a memo for a static array.
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Line 1454: The effect that defines handleKeyDown incorrectly includes
getTerminalIds in its dependency array even though the effect body only uses
navigateToTerminal; remove getTerminalIds from the dependency list so the effect
reads "}, [navigateToTerminal]);" (or equivalent) to avoid unnecessary
re-runs—locate the effect around the handleKeyDown creation in terminal-view.tsx
and update the dependency array to include only navigateToTerminal.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 1374-1382: The effect's dependency array currently includes
runCommandOnConnect and onCommandRan which can trigger unnecessary WebSocket
reconnections if parents pass non-memoized callbacks; create refs (e.g.
runCommandOnConnectRef and onCommandRanRef) via useRef, update their .current
each render to the latest callbacks, read those refs inside the effect (and when
invoking callbacks), remove runCommandOnConnect and onCommandRan from the
useEffect dependency array, and keep hasRunInitialCommandRef logic unchanged so
you avoid reconnects while still using fresh callback values.
In `@apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts`:
- Around line 14-42: The test repeats setup steps (setupRealProject,
page.goto('/board'), clicking add-feature, filling feature-input, clicking
confirm-add-feature, waitForAgentOutputModal); extract these into a shared
helper (e.g., setupModalTest) that accepts the Page, project name and feature
text, implements the sequence, and replace the repeated block in this spec with
a call to that helper to reduce duplication and improve maintainability.
- Around line 243-256: Replace the brittle page.waitForTimeout(100) waits with a
polling wait that checks the modal's computed width until it matches the
expected value: use page.waitForFunction to repeatedly evaluate
window.getComputedStyle(document.querySelector('[data-testid="agent-output-modal"]')).width
(or use the locator's element) and wait until it contains "90vw" before reading
modalComputedStyle; update both occurrences that currently call
page.waitForTimeout (the ones setting modalComputedStyle and the other at line
~284) and then assert modalComputedStyle as before.
- Around line 311-318: The test currently couples to Tailwind classes by
selecting modal content with '.flex-1' and asserting '/overflow-y-auto/'; add a
stable data-testid (e.g. data-testid="agent-output-modal-content") to the
content area element in the component and update the test to locate it via
page.locator('[data-testid="agent-output-modal-content"]') instead of '.flex-1';
then replace the class assertion with a more stable check (either assert
presence of the data-testid and test scrollability by comparing scrollHeight vs
clientHeight via page.evaluate on that locator, or assert the element's computed
style overflowY equals 'auto') so tests no longer depend on Tailwind class
names.
In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx`:
- Around line 207-231: The spinner assertions in the AgentOutputModal tests are
too broad—replace the generic document.querySelectorAll('svg') checks with a
targeted query for the actual loading indicator rendered by AgentOutputModal
(e.g., getByRole('status') or getByLabelText('Loading') or
getByTestId('agent-output-spinner') depending on the component's accessibility
attributes); if the component lacks a specific accessible label/test id, add a
stable attribute (data-testid="agent-output-spinner" or aria-label/role) to the
spinner inside AgentOutputModal so the tests (in the "should show spinner when
status is running" and the non-show tests for "verified" and "waiting_approval")
can assert presence/absence of that specific element rather than any SVG.
In `@apps/ui/tests/unit/components/card-badges.test.tsx`:
- Around line 1-39: Add unit tests in
apps/ui/tests/unit/components/card-badges.test.tsx to cover the missing
scenarios: render a feature with both status 'merge_conflict' and a non-empty
error and assert both the merge-conflict badge (getByTestId like
'merge-conflict-badge-<id>') and the error badge (getByTestId like
'error-badge-<id>') exist; add a test that passes a feature with priority set
(e.g., priority > 0) and assert the merge-conflict badge has the expected
positioning class ('left-9' vs 'left-2') by checking the badge element's
classList; and add a standalone test for error-only features to assert only the
error badge renders. Use the existing TooltipProvider wrapper and the CardBadges
component to mirror current tests and reuse IDs based on feature.id to locate
elements.
In `@apps/ui/tests/unit/components/phase-model-selector.test.tsx`:
- Line 73: The test declares _capturedSelector but never uses it; either remove
the unused variable declaration or add an assertion to validate it (e.g., inside
the test that sets it, add expect(_capturedSelector).toBeDefined() or a more
specific expect against the captured selector result). Locate the declaration of
_capturedSelector in the phase-model-selector.test.tsx test and either delete
that line or add the appropriate expect(...) assertion where the selector is
set.
- Around line 296-320: The tests read the component file to assert the presence
of useShallow and the useAppStore(useShallow(...)) pattern but currently assume
the file read always succeeds; wrap the fs.readFileSync call for
phase-model-selector.tsx in a try/catch (or use fs.existsSync before reading)
and, on failure, throw or assert with a clear, descriptive error that includes
the resolved component path and suggests the test file may have moved, so the
failure message guides debugging of path issues for the useShallow/useAppStore
checks.
In `@apps/ui/tests/unit/components/worktree-panel-props.test.ts`:
- Around line 108-111: Remove the unused variable declaration _elementIndent
from the test; locate where _elementIndent is declared alongside currentJsxProps
and inJsxElement (the let _elementIndent = 0; line) and delete it so the test no
longer declares or assigns an unused variable used for duplicate prop detection
logic.
- Around line 129-147: The prop-extraction regex used in the loop that builds
currentJsxProps and propMatches only matches curly-brace values and misses
string-valued props; update the pattern used in trimmed.matchAll (where
propMatches is created) so it also recognizes props assigned with quotes (e.g.,
prop="value" or prop='value') in addition to prop={...}; keep the rest of the
duplicate detection logic (currentJsxProps, duplicates push) unchanged and
ensure the new regex still captures the prop name as the first capture group so
existing references to match[1] continue to work.
In `@apps/ui/tests/unit/hooks/use-board-column-features.test.ts`:
- Around line 303-306: The destructured variable _clearRecentlyCompletedFeatures
from the useAppStore.getState() call is unused; remove it from the destructuring
so only addRecentlyCompletedFeature is extracted (i.e., change the const {
addRecentlyCompletedFeature } = useAppStore.getState();), leaving any other code
that references useAppStore unchanged.
In `@apps/ui/tests/unit/lint-fixes-type-safety.test.ts`:
- Around line 83-103: This test mutates globals (window.matchMedia via
mockMatchMedia, globalThis.ResizeObserver, and globalThis.IntersectionObserver)
and must restore them to avoid cross-test pollution: capture the originals
(e.g., const _origMatchMedia = window.matchMedia, const _origResizeObserver =
globalThis.ResizeObserver, const _origIntersectionObserver =
globalThis.IntersectionObserver) before assigning mocks, and add an afterEach
that restores window.matchMedia, globalThis.ResizeObserver, and
globalThis.IntersectionObserver to those saved originals and calls
vi.restoreAllMocks()/vi.resetAllMocks() to fully clean up the mocks.
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 171-172: The call to getModalComputedStyle(page) returns
dimensions that are never used; either remove the call if it’s unnecessary, or
capture the return value (e.g., const initialDims = await
getModalComputedStyle(page)) and use it in subsequent comparisons, or explicitly
mark the ignored result with a comment or underscore (e.g., await
getModalComputedStyle(page) // trigger layout side-effect) so intent is clear;
update the call site in the responsive-modal test helper (getModalComputedStyle)
accordingly.
In `@libs/platform/tests/terminal-theme-colors.test.ts`:
- Around line 215-260: Extract the repeated override-construction into a small
test helper (e.g., applyCustomColors or buildCustomTheme) that accepts
baseTheme, customBgColor, and customFgColor and returns the merged theme using
the same logic currently duplicated in the tests (spread baseTheme and
conditionally add background/foreground only when truthy). Replace the three
inline blocks that compute customTheme in the tests with calls to this helper to
keep behavior identical while removing duplication; reference the existing
getTerminalThemeColors call and the variables customBgColor/customFgColor when
implementing and using the helper.
- Around line 97-114: The tests in terminal-theme-colors.test.ts currently use
object identity assertions (expect(...).toBe(...)) which couples aliases to the
same reference; update the alias assertions to compare values instead (e.g., use
expect(...).toEqual(...) or compare specific theme color properties) so aliases
are validated by content, not reference. Locate the alias assertions referencing
terminalThemeColors.forest, .ocean, .ember, .solarizedlight, .github, .sand, and
.peach and replace the toBe checks with deep-equality checks against their base
themes (terminalThemeColors.gruvbox, .nord, .monokai, .light, .cream) to ensure
tests assert equivalent theme values rather than same object identity.
In `@libs/prompts/src/defaults.ts`:
- Around line 258-259: Extract the duplicated "Process Protection" warning block
into a single exported constant (e.g., PROCESS_PROTECTION_WARNING) in
libs/prompts/src/defaults.ts and replace the inline copies in the prompt
template strings with an interpolation/reference to that constant; update all
occurrences (the duplicated block around the current template at lines ~258 and
the other copy around ~371-373) so they import/use PROCESS_PROTECTION_WARNING
instead of embedding the literal text, ensuring one source of truth for the
warning.
In `@libs/types/src/provider-utils.ts`:
- Around line 454-462: The conditional currently checks "expectedProvider in
PROVIDER_PREFIX_EXCEPTIONS" then casts the map to Record and calls includes;
simplify this by removing the 'in' check and using optional chaining on
PROVIDER_PREFIX_EXCEPTIONS with expectedProvider to call includes directly
(e.g., use PROVIDER_PREFIX_EXCEPTIONS[expectedProvider]?.includes(provider));
update the condition that references PROVIDER_PREFIX_EXCEPTIONS,
expectedProvider and provider accordingly and remove the unnecessary type
assertion/cast.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
libs/types/tests/unit/__snapshots__/provider-utils.test.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (147)
apps/server/src/index.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/codex-provider.tsapps/server/src/providers/copilot-provider.tsapps/server/src/providers/cursor-provider.tsapps/server/src/providers/gemini-provider.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/worktree/routes/list.tsapps/server/src/services/auto-mode/facade.tsapps/server/src/services/dev-server-service.tsapps/server/src/services/execution-service.tsapps/server/src/services/execution-types.tsapps/server/src/services/pipeline-orchestrator.tsapps/server/src/services/worktree-resolver.tsapps/server/tests/unit/lib/file-editor-store-logic.test.tsapps/server/tests/unit/lib/settings-helpers.test.tsapps/server/tests/unit/providers/codex-provider.test.tsapps/server/tests/unit/providers/copilot-provider.test.tsapps/server/tests/unit/providers/cursor-provider.test.tsapps/server/tests/unit/providers/gemini-provider.test.tsapps/server/tests/unit/routes/app-spec/parse-and-create-features-defaults.test.tsapps/server/tests/unit/routes/worktree/list-detached-head.test.tsapps/server/tests/unit/services/agent-executor.test.tsapps/server/tests/unit/services/auto-mode/facade-agent-runner.test.tsapps/server/tests/unit/services/dev-server-event-types.test.tsapps/server/tests/unit/services/dev-server-persistence.test.tsapps/server/tests/unit/services/execution-service.test.tsapps/server/tests/unit/services/pipeline-orchestrator-provider-id.test.tsapps/server/tests/unit/services/pipeline-orchestrator-status-provider.test.tsapps/server/tests/unit/services/worktree-resolver.test.tsapps/ui/package.jsonapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsxapps/ui/src/components/layout/sidebar/hooks/use-navigation.tsapps/ui/src/components/shared/model-override-trigger.tsxapps/ui/src/components/ui/description-image-dropzone.tsxapps/ui/src/components/ui/git-diff-panel.tsxapps/ui/src/components/ui/xterm-log-viewer.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-header.tsxapps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsxapps/ui/src/components/views/board-view/components/list-view/row-actions.tsxapps/ui/src/components/views/board-view/components/list-view/status-badge.tsxapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.constants.tsapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/components/views/board-view/dialogs/backlog-plan-dialog.tsxapps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/event-content-formatter.tsapps/ui/src/components/views/board-view/dialogs/mass-edit-dialog.tsxapps/ui/src/components/views/board-view/dialogs/stash-changes-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/hooks/use-board-column-features.tsapps/ui/src/components/views/board-view/hooks/use-board-drag-drop.tsapps/ui/src/components/views/board-view/hooks/use-board-effects.tsapps/ui/src/components/views/board-view/hooks/use-board-features.tsapps/ui/src/components/views/board-view/kanban-board.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown-item.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-mobile-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsxapps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.tsapps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.tsapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/ui/src/components/views/file-editor-view/components/code-editor.tsxapps/ui/src/components/views/file-editor-view/file-editor-view.tsxapps/ui/src/components/views/github-issues-view.tsxapps/ui/src/components/views/github-issues-view/hooks/use-issues-filter.tsapps/ui/src/components/views/github-prs-view.tsxapps/ui/src/components/views/graph-view/hooks/use-graph-nodes.tsapps/ui/src/components/views/interview-view.tsxapps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/project-settings-view/project-bulk-replace-dialog.tsxapps/ui/src/components/views/running-agents-view.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/model-defaults/bulk-replace-dialog.tsxapps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsxapps/ui/src/components/views/settings-view/providers/opencode-model-configuration.tsxapps/ui/src/components/views/setup-view/steps/cli-setup-step.tsxapps/ui/src/components/views/spec-view/hooks/use-spec-generation.tsapps/ui/src/components/views/terminal-view.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/hooks/queries/use-features.tsapps/ui/src/hooks/use-agent-output-websocket.tsapps/ui/src/hooks/use-auto-mode.tsapps/ui/src/hooks/use-guided-prompts.tsapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/hooks/use-test-runners.tsapps/ui/src/lib/agent-context-parser.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/log-parser.tsapps/ui/src/lib/settings-utils.tsapps/ui/src/lib/utils.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/store/types/project-types.tsapps/ui/src/store/types/state-types.tsapps/ui/src/store/ui-cache-store.tsapps/ui/tests/features/responsive/agent-output-modal-responsive.spec.tsapps/ui/tests/features/success-log-contrast.spec.tsapps/ui/tests/setup.tsapps/ui/tests/unit/components/agent-info-panel-merge-conflict.test.tsxapps/ui/tests/unit/components/agent-info-panel.test.tsxapps/ui/tests/unit/components/agent-output-modal-constants.test.tsapps/ui/tests/unit/components/agent-output-modal-integration.test.tsxapps/ui/tests/unit/components/agent-output-modal-responsive.test.tsxapps/ui/tests/unit/components/card-actions.test.tsxapps/ui/tests/unit/components/card-badges.test.tsxapps/ui/tests/unit/components/event-content-formatter.test.tsapps/ui/tests/unit/components/feature-creation-defaults.test.tsapps/ui/tests/unit/components/mobile-terminal-shortcuts.test.tsxapps/ui/tests/unit/components/phase-model-selector.test.tsxapps/ui/tests/unit/components/pr-comment-resolution-pr-info.test.tsapps/ui/tests/unit/components/worktree-panel-props.test.tsapps/ui/tests/unit/hooks/use-board-column-features.test.tsapps/ui/tests/unit/hooks/use-dev-servers.test.tsapps/ui/tests/unit/hooks/use-features-cache.test.tsapps/ui/tests/unit/hooks/use-guided-prompts.test.tsapps/ui/tests/unit/hooks/use-media-query.test.tsapps/ui/tests/unit/hooks/use-test-runners-deps.test.tsapps/ui/tests/unit/lib/agent-context-parser.test.tsapps/ui/tests/unit/lib/settings-utils.test.tsapps/ui/tests/unit/lib/summary-selection.test.tsapps/ui/tests/unit/lint-fixes-navigator-type.test.tsapps/ui/tests/unit/lint-fixes-type-safety.test.tsapps/ui/tests/unit/store/app-store-recently-completed.test.tsapps/ui/tests/unit/store/ui-cache-store-worktree.test.tsapps/ui/tests/utils/components/responsive-modal.tsapps/ui/vitest.config.tslibs/platform/tests/terminal-theme-colors.test.tslibs/prompts/src/defaults.tslibs/types/package.jsonlibs/types/src/event.tslibs/types/src/feature.tslibs/types/src/index.tslibs/types/src/model.tslibs/types/src/pipeline.tslibs/types/src/provider-utils.tslibs/types/tests/unit/provider-utils.test.tslibs/types/vitest.config.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- apps/ui/src/components/views/settings-view.tsx
Comment on lines
+1
to
+4
| import { describe, it, expect, vi } from 'vitest'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import { CardActions } from '../../../src/components/views/board-view/components/kanban-card/card-actions'; | ||
| import type { Feature } from '@automaker/types'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Playwright config discovery =="
fd -HI 'playwright*.{ts,js,mjs,cjs}' -t f | while read -r f; do
echo "--- $f ---"
sed -n '1,240p' "$f"
done
echo
echo "== Test discovery settings (testDir/testMatch/testIgnore) =="
rg -n -C2 'testDir|testMatch|testIgnore' -g '**/playwright*.{ts,js,mjs,cjs}'
echo
echo "== Any CommonJS vitest imports =="
rg -n -C2 "require\\(['\"]vitest['\"]\\)|require\\(['\"]vitest/config['\"]\\)" -g '**/*.{js,cjs,ts,mts,cts}'
echo
echo "== Unit test paths that may overlap E2E globs =="
fd -HI 'apps/ui/tests/unit/**/*.test.ts*' -t fRepository: AutoMaker-Org/automaker
Length of output: 444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Playwright config discovery =="
find . -maxdepth 4 -name 'playwright*.config.*' -type f 2>/dev/null | head -20 | while read -r f; do
echo "--- $f ---"
head -n 100 "$f"
done
echo
echo "== Test discovery settings (testDir/testMatch/testIgnore) =="
rg 'testDir|testMatch|testIgnore' --type ts --type js -C2 2>/dev/null || echo "No test discovery settings found"
echo
echo "== Any CommonJS vitest imports =="
rg 'require.*vitest' --type ts --type js --type cjs 2>/dev/null || echo "No CJS vitest imports found"
echo
echo "== Unit test paths that may overlap E2E globs =="
find apps/ui/tests/unit -name '*.test.ts*' -type f 2>/dev/null | head -20 || echo "No unit test files found"
echo
echo "== vitest config file =="
find . -maxdepth 3 -name 'vitest.config.*' -type f 2>/dev/null
echo
echo "== Check apps/ui package.json test scripts =="
if [ -f apps/ui/package.json ]; then
jq '.scripts' apps/ui/package.json 2>/dev/null || cat apps/ui/package.json | head -50
fiRepository: AutoMaker-Org/automaker
Length of output: 49908
Add testIgnore pattern to Playwright config to exclude Vitest unit tests.
The CI error (Vitest cannot be imported in a CommonJS module using require()) is caused by Playwright's testDir: './tests' configuration including apps/ui/tests/unit/** without exclusion. Playwright's CJS runner encounters ESM Vitest files and fails. Add a testIgnore pattern to exclude unit tests:
Playwright config update
export default defineConfig({
testDir: './tests',
+ testIgnore: '**/unit/**',
fullyParallel: true,🧰 Tools
🪛 GitHub Actions: E2E Tests
[error] 1-1: Vitest cannot be imported in a CommonJS module using require(). Please use "import" instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/unit/components/card-actions.test.tsx` around lines 1 - 4,
Playwright is picking up Vitest unit tests and failing; update the Playwright
config (where testDir is set) to add a testIgnore pattern that excludes the
Vitest unit tests (e.g. add testIgnore: ['apps/ui/tests/unit/**'] or equivalent
glob) so Playwright will not attempt to run files under the unit tests
directory; ensure the pattern is applied to the exported config object used by
the CI runner.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
200-204: Tighten breakpoint intent forshowShortcutsBar.At Line 203,
useIsTablet()already includes mobile widths (max-width: 1024px), soisMobile || isTabletis redundant. Simplifying this makes viewport logic clearer and less error-prone for future edits.♻️ Suggested simplification
const isMobile = useIsMobile(); const isTablet = useIsTablet(); - const showShortcutsBar = isMobile || isTablet; + const showShortcutsBar = isTablet;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx` around lines 200 - 204, The showShortcutsBar calculation is redundant because useIsTablet() already returns true for mobile sizes; replace the combined check (isMobile || isTablet) with just isTablet to tighten the breakpoint intent, update the declaration of showShortcutsBar to use isTablet only, and remove any now-unused isMobile variable/imports (and related hooks) from the component (referencing isMobile, isTablet, showShortcutsBar, and useIsTablet to locate the changes).apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx (2)
115-119: This test does not validate close behavior.The assertion only checks that the mock function exists, not that the modal actually invokes
onClose. Please trigger a real close action (e.g., ESC/backdrop/close button) and assert the callback call.Proposed test improvement
it('should have onClose callback available', () => { render(<AgentOutputModal {...defaultProps} />); - // Verify the onClose function is provided - expect(defaultProps.onClose).toBeDefined(); + fireEvent.keyDown(window, { key: 'Escape' }); + expect(defaultProps.onClose).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx` around lines 115 - 119, The test currently only checks that the onClose mock exists; update it to simulate a real close action on the AgentOutputModal and assert that defaultProps.onClose was called. Render the component (render(<AgentOutputModal {...defaultProps} />)), then trigger a user close event such as pressing Escape (fireEvent.keyDown(document, { key: 'Escape' }) or userEvent.keyboard('{Escape}')), or clicking the modal close button/backdrop (userEvent.click(...) or fireEvent.click on the close button obtained via screen.getByRole or text), and finally expect(defaultProps.onClose).toHaveBeenCalled(); repeat for any other close mechanisms you want to validate.
10-10: Prefer a stable barrel import for the UI component.This deep relative import is brittle during refactors. Use the app’s component barrel export (or existing app alias barrel) instead of a deep file path.
Based on learnings: In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from
../componentsrather than direct path imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx` at line 10, The test currently imports the component via a brittle deep path: AgentOutputModal from '../../../src/components/views/board-view/dialogs/agent-output-modal'; update the import to use the app's stable barrel export instead (import AgentOutputModal from the components barrel) so refactors won't break; locate the import in apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx and replace the deep relative path with the barrel export (the components index/alias) that re-exports AgentOutputModal.apps/ui/tests/unit/components/mobile-terminal-shortcuts.test.tsx (1)
8-9: Preferapps/uicomponent barrel imports over deep relative paths.These deep paths are fragile during refactors. If exported, import
MobileTerminalShortcuts/StickyModifierfrom the app’scomponentsbarrel instead.Based on learnings: "In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/unit/components/mobile-terminal-shortcuts.test.tsx` around lines 8 - 9, Replace the deep relative imports with the app's components barrel: import MobileTerminalShortcuts and StickyModifier from the components index instead of '../../../src/components/views/terminal-view/mobile-terminal-shortcuts.tsx' and '../../../src/components/views/terminal-view/sticky-modifier-keys.tsx'; update the test to import { MobileTerminalShortcuts } and type { StickyModifier } from the app's components barrel entry so the imports are stable during refactors.apps/ui/tests/utils/components/responsive-modal.ts (1)
171-173: Remove or use the pre-resize dimensions snapshot.The initial
getModalComputedStyle(page)result is currently discarded, so this step has no effect.Minimal cleanup
- // Get initial modal dimensions (used for comparison context) - await getModalComputedStyle(page); + // Initial dimensions read removed (unused).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 171 - 173, The call to getModalComputedStyle(page) at the start of the test is being discarded; either store its return value and use it for pre-resize comparison or remove the call entirely. Update the test to assign the result to a variable (e.g., const beforeDims = await getModalComputedStyle(page)) and then use beforeDims in the subsequent assertions that compare pre- and post-resize dimensions, or if no comparison is needed, delete the unused await getModalComputedStyle(page) invocation.apps/server/tests/unit/routes/worktree/list-detached-head.test.ts (1)
603-638: Add an explicit prune assertion in the removed-detached test.This case validates
removedWorktrees, but not thatgit worktree prunewas actually triggered. Asserting prune invocation would better guard the regression path.Suggested test addition
const handler = createListHandler(); await handler(req, res); + const pruneCalled = (exec as unknown as Mock).mock.calls.some(([cmd]) => + String(cmd).includes('git worktree prune') + ); + expect(pruneCalled).toBe(true); + const response = vi.mocked(res.json).mock.calls[0][0] as { worktrees: Array<{ branch: string }>; removedWorktrees?: Array<{ path: string; branch: string }>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts` around lines 603 - 638, The test currently verifies removedWorktrees but doesn't assert that a prune was invoked; update the test that uses setupStandardExec and createListHandler to also assert the mocked git execution was called for pruning (e.g., verify the mocked exec/spawn function recorded a call including "worktree" and "prune" or the exact args "git worktree prune"). Locate where setupStandardExec sets up the command mock and add an expectation after await handler(req, res) that the mocked git executor (the same mocked function used by setupStandardExec) was invoked with the prune command to ensure git worktree prune ran.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/list.ts`:
- Around line 253-255: The git invocation calling execAsync('git rev-parse
--abbrev-ref HEAD', { cwd: worktreePath }) can hang; add a timeout option to
that execAsync call (e.g., timeout: 5000) and update the surrounding code to
catch a timeout error and fall back to the detached-head behavior (keep existing
fallback logic that handles an error and treats headRef as unknown/detached).
Refer to the execAsync call, the headRef variable and worktreePath in the list
route handler and ensure the execAsync options include the timeout and errors
are caught and handled gracefully.
- Around line 178-189: The code that recovers a branch from head-name (the try
blocks reading headNamePath/head-name in list.ts, variables
headNamePath/headName/branch) only strips 'refs/heads/' and misses refs like
'refs/remotes/origin/...'; update the normalization to defensively remove common
ref prefixes (e.g., 'refs/heads/', 'refs/remotes/origin/', 'refs/remotes/',
'refs/') and then take the last path segment (the actual branch name) before
returning, ensuring you still check for a non-empty result.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 877-888: The stream data handlers call the async method
handleProcessOutput(serverInfo, data) without capturing rejections; wrap those
calls so any promise rejection is caught and logged to avoid unhandled promise
rejections — update the devProcess.stdout.on('data', ...) and
devProcess.stderr.on('data', ...) handlers inside the DevServerService (or the
containing class) to invoke handleProcessOutput(...) and attach a .catch(...)
(or use an async wrapper with try/catch) that reports the error via the service
logger/error reporter and prevents the rejection from escaping the event
emitter.
In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx`:
- Around line 270-293: The test currently only re-renders AgentOutputModal and
checks presence; it must assert the auto-scroll side effect: locate the output
container (the element inside AgentOutputModal used to display logs, e.g., test
id or selector used in the component), spy/mock its scrollTo method or check its
scrollTop before and after update, then update mockUseAgentOutput (the mock
returning data) and rerender AgentOutputModal to trigger the update; finally
assert that scrollTo was called with expected arguments or that scrollTop
increased to the container's scrollHeight. Use references to AgentOutputModal,
mockUseAgentOutput, and the output container element to implement the assertion.
- Around line 207-231: The tests in agent-output-modal-integration.test.tsx are
asserting generic modal text and SVG counts which can be true regardless of
spinner state; update the three tests for AgentOutputModal to assert the spinner
element specifically (use the spinner's test id, accessible name, role, or a
unique SVG/data attribute used by the Loader2 component) — for the "verified"
and "waiting_approval" cases use queryByTestId/queryByRole to assert spinner is
not present (null/not in DOM or aria-hidden="true"), and for the "running" case
use getByTestId/getByRole (or querySelector for the loader's unique SVG/data
attribute) to assert the spinner is present and visible; change references in
the tests around AgentOutputModal, defaultProps, and the three it(...) blocks
accordingly.
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 113-123: The desktop branches currently skip validation which
allows regressions to pass; update the helpers that use the viewport variable
(look for the block checking viewport and asserting modalWidth/modalHeight) to
add an explicit branch for 'desktop' and 'desktopLarge' that performs width and
height assertions: verify modalWidth and modalHeight are > 0 and less than or
equal to the viewport dimensions (viewportWidth/viewportHeight) and enforce the
desktop max width constraint (e.g., <= 1200) so desktop cases do not silently
pass; apply the same guard to the second helper block around the modalHeight
checks (the similar code at lines ~147-156) referencing the same symbols
(viewport, modalWidth, modalHeight, viewportWidth, viewportHeight).
- Around line 87-89: The loop that asserts individual class tokens uses
toHaveClass, which does exact full-class matching; update the assertions inside
the for (const className of expectedClasses) loop to use toContainClass instead
so each class token is checked for presence regardless of other classes or order
— modify the expectation on the modal variable (the expect(modal)... call) to
call toContainClass(className) for each className from expectedClasses.
In `@libs/types/src/provider-utils.ts`:
- Around line 43-45: Tighten types to fix TS7053 by making
PROVIDER_PREFIX_EXCEPTIONS keyed by the provider union and using ModelProvider
for indexing: change PROVIDER_PREFIX_EXCEPTIONS's type so its keys are the
ModelProvider union (not string), change the expectedProvider parameter type
from string to ModelProvider where it's used, and when iterating
PROVIDER_PREFIXES narrow the loop variable to ModelProvider (or use a typed cast
like `for (const p of PROVIDER_PREFIXES as ModelProvider[])`) so indexing
PROVIDER_PREFIX_EXCEPTIONS[expectedProvider] no longer errors; update any
related function signatures that accept expectedProvider to use ModelProvider as
well.
---
Nitpick comments:
In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts`:
- Around line 603-638: The test currently verifies removedWorktrees but doesn't
assert that a prune was invoked; update the test that uses setupStandardExec and
createListHandler to also assert the mocked git execution was called for pruning
(e.g., verify the mocked exec/spawn function recorded a call including
"worktree" and "prune" or the exact args "git worktree prune"). Locate where
setupStandardExec sets up the command mock and add an expectation after await
handler(req, res) that the mocked git executor (the same mocked function used by
setupStandardExec) was invoked with the prune command to ensure git worktree
prune ran.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 200-204: The showShortcutsBar calculation is redundant because
useIsTablet() already returns true for mobile sizes; replace the combined check
(isMobile || isTablet) with just isTablet to tighten the breakpoint intent,
update the declaration of showShortcutsBar to use isTablet only, and remove any
now-unused isMobile variable/imports (and related hooks) from the component
(referencing isMobile, isTablet, showShortcutsBar, and useIsTablet to locate the
changes).
In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx`:
- Around line 115-119: The test currently only checks that the onClose mock
exists; update it to simulate a real close action on the AgentOutputModal and
assert that defaultProps.onClose was called. Render the component
(render(<AgentOutputModal {...defaultProps} />)), then trigger a user close
event such as pressing Escape (fireEvent.keyDown(document, { key: 'Escape' }) or
userEvent.keyboard('{Escape}')), or clicking the modal close button/backdrop
(userEvent.click(...) or fireEvent.click on the close button obtained via
screen.getByRole or text), and finally
expect(defaultProps.onClose).toHaveBeenCalled(); repeat for any other close
mechanisms you want to validate.
- Line 10: The test currently imports the component via a brittle deep path:
AgentOutputModal from
'../../../src/components/views/board-view/dialogs/agent-output-modal'; update
the import to use the app's stable barrel export instead (import
AgentOutputModal from the components barrel) so refactors won't break; locate
the import in
apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx and
replace the deep relative path with the barrel export (the components
index/alias) that re-exports AgentOutputModal.
In `@apps/ui/tests/unit/components/mobile-terminal-shortcuts.test.tsx`:
- Around line 8-9: Replace the deep relative imports with the app's components
barrel: import MobileTerminalShortcuts and StickyModifier from the components
index instead of
'../../../src/components/views/terminal-view/mobile-terminal-shortcuts.tsx' and
'../../../src/components/views/terminal-view/sticky-modifier-keys.tsx'; update
the test to import { MobileTerminalShortcuts } and type { StickyModifier } from
the app's components barrel entry so the imports are stable during refactors.
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 171-173: The call to getModalComputedStyle(page) at the start of
the test is being discarded; either store its return value and use it for
pre-resize comparison or remove the call entirely. Update the test to assign the
result to a variable (e.g., const beforeDims = await
getModalComputedStyle(page)) and then use beforeDims in the subsequent
assertions that compare pre- and post-resize dimensions, or if no comparison is
needed, delete the unused await getModalComputedStyle(page) invocation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/server/src/routes/worktree/routes/list.tsapps/server/src/services/auto-mode/facade.tsapps/server/src/services/dev-server-service.tsapps/server/tests/unit/lib/file-editor-store-logic.test.tsapps/server/tests/unit/routes/worktree/list-detached-head.test.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-mobile-dropdown.tsxapps/ui/src/components/views/file-editor-view/file-editor-dirty-utils.tsapps/ui/src/components/views/file-editor-view/use-file-editor-store.tsapps/ui/src/components/views/spec-view/hooks/use-spec-generation.tsapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/hooks/use-auto-mode.tsapps/ui/tests/features/success-log-contrast.spec.tsapps/ui/tests/unit/components/agent-output-modal-integration.test.tsxapps/ui/tests/unit/components/mobile-terminal-shortcuts.test.tsxapps/ui/tests/utils/components/responsive-modal.tslibs/types/src/provider-utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/ui/src/components/views/board-view/worktree-panel/components/worktree-mobile-dropdown.tsx
- apps/server/src/services/auto-mode/facade.ts
- apps/ui/tests/features/success-log-contrast.spec.ts
- apps/ui/src/components/views/board-view.tsx
- apps/server/tests/unit/lib/file-editor-store-logic.test.ts
Comment on lines
+282
to
+297
| it('should send initial arrow key immediately on press', () => { | ||
| const onSendInput = vi.fn(); | ||
| render(<MobileTerminalShortcuts {...createDefaultProps({ onSendInput })} />); | ||
|
|
||
| const upButton = getArrowButton('up'); | ||
| expect(upButton).not.toBeNull(); | ||
| fireEvent.pointerDown(upButton!); | ||
|
|
||
| // Initial press should send immediately | ||
| expect(onSendInput).toHaveBeenCalledTimes(1); | ||
| expect(onSendInput).toHaveBeenCalledWith('\x1b[A'); | ||
|
|
||
| // Release the button - should not send more | ||
| fireEvent.pointerUp(upButton!); | ||
| expect(onSendInput).toHaveBeenCalledTimes(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
Arrow repeat tests don’t verify timer-driven behavior.
Line 282 and Line 299 assertions are immediate-only. If repeat uses timers, these tests can pass even when repeat/stop logic is broken.
🧪 Suggested test hardening with fake timers
it('should send initial arrow key immediately on press', () => {
const onSendInput = vi.fn();
+ vi.useFakeTimers();
render(<MobileTerminalShortcuts {...createDefaultProps({ onSendInput })} />);
const upButton = getArrowButton('up');
expect(upButton).not.toBeNull();
fireEvent.pointerDown(upButton!);
// Initial press should send immediately
expect(onSendInput).toHaveBeenCalledTimes(1);
expect(onSendInput).toHaveBeenCalledWith('\x1b[A');
// Release the button - should not send more
fireEvent.pointerUp(upButton!);
- expect(onSendInput).toHaveBeenCalledTimes(1);
+ vi.advanceTimersByTime(500);
+ expect(onSendInput).toHaveBeenCalledTimes(1);
+ vi.useRealTimers();
});
it('should stop repeating when pointer leaves button', () => {
const onSendInput = vi.fn();
+ vi.useFakeTimers();
render(<MobileTerminalShortcuts {...createDefaultProps({ onSendInput })} />);
const upButton = getArrowButton('up');
expect(upButton).not.toBeNull();
// Press and release via pointer leave
fireEvent.pointerDown(upButton!);
expect(onSendInput).toHaveBeenCalledTimes(1);
+ vi.advanceTimersByTime(400);
+ expect(onSendInput.mock.calls.length).toBeGreaterThan(1);
// Pointer leaves - should clear repeat timers
fireEvent.pointerLeave(upButton!);
// Only the initial press should have been sent
- expect(onSendInput).toHaveBeenCalledTimes(1);
+ const callsAtLeave = onSendInput.mock.calls.length;
+ vi.advanceTimersByTime(500);
+ expect(onSendInput).toHaveBeenCalledTimes(callsAtLeave);
+ vi.useRealTimers();
});Also applies to: 299-315
…d custom providers. Fixes AutoMaker-Org#816
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/routes/github/routes/validate-issue.ts (1)
377-389:⚠️ Potential issue | 🟠 MajorReject unknown
providerIdup front instead of treating any non-empty value as valid.At Line 382,
!!normalizedProviderIdmakes the request pass validation even when the provider is mistyped (orsettingsServiceis unavailable), which then fails later in background execution.Proposed fix
- const isValidModel = - isClaudeModel(model) || - isCursorModel(model) || - isCodexModel(model) || - isOpencodeModel(model) || - !!normalizedProviderId; + let hasValidProviderContext = false; + if (normalizedProviderId) { + if (!settingsService) { + res.status(400).json({ + success: false, + error: 'providerId is not supported in this environment', + }); + return; + } + + const providerContext = await resolveProviderContext( + settingsService, + model, + normalizedProviderId, + '[ValidateIssue]' + ); + if (!providerContext.provider) { + res.status(400).json({ + success: false, + error: `Unknown providerId "${normalizedProviderId}"`, + }); + return; + } + hasValidProviderContext = true; + } + + const isValidModel = + isClaudeModel(model) || + isCursorModel(model) || + isCodexModel(model) || + isOpencodeModel(model) || + hasValidProviderContext;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/github/routes/validate-issue.ts` around lines 377 - 389, The current validation treats any non-empty normalizedProviderId as valid (via !!normalizedProviderId) which lets mistyped or unknown providers through; update the isValidModel check to verify the provider actually exists and is Claude-compatible by querying the settings service (e.g., call settingsService.getProviderById(normalizedProviderId) or equivalent and confirm the returned provider supports Claude-compatible models) instead of just truthiness; adjust the surrounding code so the provider lookup is awaited before computing isValidModel and return the 400 error when no matching provider is found.
🧹 Nitpick comments (2)
apps/server/tests/unit/routes/backlog-plan/apply.test.ts (1)
33-43: Consider adding type safety to mock response.The mock response works but could benefit from explicit typing to catch potential issues at compile time.
♻️ Optional: Add Response type annotation
-function createMockRes() { - const res: { - status: ReturnType<typeof vi.fn>; - json: ReturnType<typeof vi.fn>; - } = { +function createMockRes(): { status: ReturnType<typeof vi.fn>; json: ReturnType<typeof vi.fn> } { + const res = { status: vi.fn(), json: vi.fn(), - }; + } as { status: ReturnType<typeof vi.fn>; json: ReturnType<typeof vi.fn> }; res.status.mockReturnValue(res); return res; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/routes/backlog-plan/apply.test.ts` around lines 33 - 43, The createMockRes helper lacks explicit return typing which reduces compile-time safety; update it to return a typed mock response (e.g., declare a MockRes interface/type with status: vi.Mock and json: vi.Mock or use vi.MockInstance types) and annotate createMockRes(): MockRes so res.status and res.json are strongly typed (keep the implementation that assigns vi.fn() and res.status.mockReturnValue(res) but replace ReturnType<typeof vi.fn> with the explicit MockRes type and vi.Mock types and ensure the function signature references createMockRes and the MockRes type).apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx (1)
397-412: Consider removing duplicated mobile “Create Feature” action.Line 398 renders a full mobile CTA while Line 196 already adds the same action in the header for mobile. Keeping one primary trigger would reduce visual noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx` around lines 397 - 412, Duplicate mobile "Create Feature" CTA: remove the secondary CTA block rendered in IssueDetailPanel (the JSX guarded by isMobile that contains the "Create Feature" card and Button) so only the header mobile action remains; locate the fragment that references isMobile and onCreateFeature in issue-detail-panel.tsx and delete that entire mobile CTA block (or refactor it into a shared component if you prefer a single source) to eliminate the duplicated action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/server/src/routes/github/routes/validate-issue.ts`:
- Around line 377-389: The current validation treats any non-empty
normalizedProviderId as valid (via !!normalizedProviderId) which lets mistyped
or unknown providers through; update the isValidModel check to verify the
provider actually exists and is Claude-compatible by querying the settings
service (e.g., call settingsService.getProviderById(normalizedProviderId) or
equivalent and confirm the returned provider supports Claude-compatible models)
instead of just truthiness; adjust the surrounding code so the provider lookup
is awaited before computing isValidModel and return the 400 error when no
matching provider is found.
---
Nitpick comments:
In `@apps/server/tests/unit/routes/backlog-plan/apply.test.ts`:
- Around line 33-43: The createMockRes helper lacks explicit return typing which
reduces compile-time safety; update it to return a typed mock response (e.g.,
declare a MockRes interface/type with status: vi.Mock and json: vi.Mock or use
vi.MockInstance types) and annotate createMockRes(): MockRes so res.status and
res.json are strongly typed (keep the implementation that assigns vi.fn() and
res.status.mockReturnValue(res) but replace ReturnType<typeof vi.fn> with the
explicit MockRes type and vi.Mock types and ensure the function signature
references createMockRes and the MockRes type).
In
`@apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx`:
- Around line 397-412: Duplicate mobile "Create Feature" CTA: remove the
secondary CTA block rendered in IssueDetailPanel (the JSX guarded by isMobile
that contains the "Create Feature" card and Button) so only the header mobile
action remains; locate the fragment that references isMobile and onCreateFeature
in issue-detail-panel.tsx and delete that entire mobile CTA block (or refactor
it into a shared component if you prefer a single source) to eliminate the
duplicated action.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/server/src/routes/backlog-plan/index.tsapps/server/src/routes/backlog-plan/routes/apply.tsapps/server/src/routes/github/routes/validate-issue.tsapps/server/tests/unit/routes/backlog-plan/apply.test.tsapps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsxapps/ui/src/components/views/github-issues-view/hooks/use-issue-validation.tsapps/ui/src/hooks/mutations/use-github-mutations.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tslibs/types/src/issue-validation.ts
Collaborator
Author
|
@claude review |
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Pull request overview
This PR delivers a broad set of stability fixes across the server + UI, with a focus on: (1) provider/model resolution (including Claude-compatible third-party providers), (2) worktree/dev-server lifecycle robustness, (3) better handling of merge_conflict features in the board UI, and (4) expanded unit/E2E test coverage using Vitest projects and Playwright scoping.
Changes:
- Add provider-aware context resolution (
resolveProviderContext) and threadproviderIdthrough feature execution, validation flows, and UI model formatting. - Improve worktree + dev-server UX (detached HEAD branch recovery, per-worktree “starting” state/events, persistence, and UI indicators).
- Introduce
merge_conflictfeature status end-to-end (types, board behavior, actions/badges/info panel, and tests).
Reviewed changes
Copilot reviewed 164 out of 165 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Forces NODE_ENV=test and adds UI Vitest project to monorepo test discovery. |
| libs/types/vitest.config.ts | Adds a dedicated Vitest project config for @automaker/types with coverage. |
| libs/types/tests/unit/snapshots/provider-utils.test.ts.snap | Adds snapshots for provider-prefix validation errors. |
| libs/types/src/provider-utils.ts | Adds provider prefix exceptions + extends validateBareModelId API. |
| libs/types/src/pipeline.ts | Adds merge_conflict to FeatureStatusWithPipeline. |
| libs/types/src/model.ts | Adds normalizeReasoningEffortForModel. |
| libs/types/src/issue-validation.ts | Adds optional providerId to issue validation input typing. |
| libs/types/src/index.ts | Exports new normalization helper + provider prefix exception map. |
| libs/types/src/feature.ts | Adds optional providerId to shared Feature type. |
| libs/types/src/event.ts | Adds dev-server:starting event type. |
| libs/types/package.json | Adds vitest dependency and a test script. |
| libs/prompts/src/defaults.ts | Adds guidance to avoid broad process-kill commands that would terminate the app. |
| apps/ui/vitest.config.ts | Adds UI Vitest config (React plugin, jsdom env, setup file, aliases). |
| apps/ui/tests/unit/store/ui-cache-store-worktree.test.ts | Tests worktree selection sanitization in UI cache store. |
| apps/ui/tests/unit/lint-fixes-type-safety.test.ts | Adds tests validating lint-driven type safety patterns in tests/mocks. |
| apps/ui/tests/unit/lint-fixes-navigator-type.test.ts | Adds tests for navigator.userAgentData typing pattern. |
| apps/ui/tests/unit/lib/summary-selection.test.ts | Tests getFirstNonEmptySummary priority/empty handling. |
| apps/ui/tests/unit/lib/settings-utils.test.ts | Tests sanitizeWorktreeByProject. |
| apps/ui/tests/unit/hooks/use-features-cache.test.ts | Tests persisted-feature cache sanitization. |
| apps/ui/tests/unit/components/pr-comment-resolution-pr-info.test.ts | Tests PR URL passthrough typing to feature creation. |
| apps/ui/tests/unit/components/card-badges.test.tsx | Tests merge-conflict badge rendering. |
| apps/ui/tests/unit/components/card-actions.test.tsx | Tests backlog “View Logs” button rendering when context exists. |
| apps/ui/tests/unit/components/agent-output-modal-constants.test.ts | Tests centralized agent output modal constants. |
| apps/ui/tests/unit/components/agent-info-panel-merge-conflict.test.tsx | Tests merge_conflict behavior in agent info panel (no polling, model display). |
| apps/ui/tests/setup.ts | Adds centralized UI unit-test setup/mocks (matchMedia, observers, localStorage). |
| apps/ui/src/store/ui-cache-store.ts | Replaces local worktree sanitizers with shared sanitizeWorktreeByProject. |
| apps/ui/src/store/types/state-types.ts | Adds recentlyCompletedFeatures tracking + actions to avoid backlog flash race. |
| apps/ui/src/store/types/project-types.ts | Adds explicit providerId typing to UI Feature override type. |
| apps/ui/src/store/app-store.ts | Initializes and implements actions for recentlyCompletedFeatures. |
| apps/ui/src/routes/__root.tsx | Adds hooks eslint-disable comment for one-time mount effect. |
| apps/ui/src/lib/utils.ts | Adds migrateModelId + normalizeModelEntry helpers for consistent model controls. |
| apps/ui/src/lib/settings-utils.ts | Adds worktree selection validation + sanitization helpers. |
| apps/ui/src/lib/log-parser.ts | Tweaks success log colors for improved contrast. |
| apps/ui/src/lib/http-api-client.ts | Adds dev-server:starting event subscription + threads providerId to validateIssue. |
| apps/ui/src/lib/electron.ts | Extends GitHub API validateIssue signature with providerId. |
| apps/ui/src/lib/agent-context-parser.ts | Makes formatModelName provider-aware (Claude-compatible provider display names). |
| apps/ui/src/hooks/use-test-runners.ts | Removes unused selector dependency from memo deps. |
| apps/ui/src/hooks/use-settings-sync.ts | Ensures claudeCompatibleProviders is synced to/from server settings. |
| apps/ui/src/hooks/use-settings-migration.ts | Migrates/hydrates agentModelBySession and persists it back to server. |
| apps/ui/src/hooks/use-guided-prompts.ts | Memoizes prompts/categories arrays to stabilize derived callbacks. |
| apps/ui/src/hooks/use-auto-mode.ts | Restores per-worktree session state + tracks recently completed features to avoid backlog flash. |
| apps/ui/src/hooks/use-agent-output-websocket.ts | Extracts WebSocket streaming logic into a reusable hook for AgentOutputModal. |
| apps/ui/src/hooks/queries/use-features.ts | Adds schema v2 + robust persisted-feature cache sanitization (incl. pipeline statuses). |
| apps/ui/src/hooks/mutations/use-github-mutations.ts | Threads providerId through validateIssue mutation. |
| apps/ui/src/components/views/terminal-view.tsx | Stabilizes getTerminalIds via useCallback and fixes deps. |
| apps/ui/src/components/views/spec-view/hooks/use-spec-generation.ts | Uses refs to prevent event listener resubscribe churn during streaming. |
| apps/ui/src/components/views/setup-view/steps/cli-setup-step.tsx | Fixes effect deps to include apiKey. |
| apps/ui/src/components/views/settings-view/providers/opencode-model-configuration.tsx | Memoizes dynamic provider ordering + fixes memo deps. |
| apps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsx | Uses useShallow selection to reduce store-driven rerenders. |
| apps/ui/src/components/views/settings-view/model-defaults/bulk-replace-dialog.tsx | Fixes memo deps / eslint-disable annotation for preview generation. |
| apps/ui/src/components/views/settings-view.tsx | Removes unused setFeatureTemplates destructure. |
| apps/ui/src/components/views/running-agents-view.tsx | Adds stable logger eslint-disable annotations and adjusts deps. |
| apps/ui/src/components/views/project-settings-view/project-bulk-replace-dialog.tsx | Adds eslint-disable annotation for stable helper dep. |
| apps/ui/src/components/views/login-view.tsx | Avoids conditional dep expression in effect deps (eslint-disable). |
| apps/ui/src/components/views/interview-view.tsx | Adds eslint-disable annotation for stable generateSpec callback. |
| apps/ui/src/components/views/graph-view/hooks/use-graph-nodes.ts | Fixes memo deps to include renderMode/edge animation toggles. |
| apps/ui/src/components/views/graph-view/graph-view.tsx | Adds optional worktreeSelector slot for top-right panel integration. |
| apps/ui/src/components/views/graph-view/graph-canvas.tsx | Renders worktree selector in top-right panel + responsive button width. |
| apps/ui/src/components/views/github-prs-view.tsx | Threads PR URL into created features + passes useWorktrees flag when running. |
| apps/ui/src/components/views/github-issues-view/hooks/use-issues-filter.ts | Adds eslint-disable comment for memo deps. |
| apps/ui/src/components/views/github-issues-view/hooks/use-issue-validation.ts | Threads providerId through validation call. |
| apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx | Makes Create Feature button accessible for mobile + aria-label. |
| apps/ui/src/components/views/github-issues-view.tsx | Threads providerId + sets defaults for planning/dependencies on created features. |
| apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts | Extracts dirty-state helpers to shared util functions. |
| apps/ui/src/components/views/file-editor-view/file-editor-view.tsx | Saves latest editor content via CodeMirror handle to avoid stale closure. |
| apps/ui/src/components/views/file-editor-view/file-editor-dirty-utils.ts | Adds pure helpers for dirty-state computation and updates. |
| apps/ui/src/components/views/file-editor-view/components/code-editor.tsx | Adds getValue() to editor handle. |
| apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx | Adds per-worktree dev-server starting state plumbing to UI components. |
| apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts | Memoizes worktrees array for stability. |
| apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts | Adds per-worktree starting set + dev-server:starting event handling. |
| apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx | Threads new dev-server starting props into actions dropdown. |
| apps/ui/src/components/views/board-view/worktree-panel/components/worktree-mobile-dropdown.tsx | Adds dev-server indicators (globe/spinner) to mobile dropdown items. |
| apps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown.tsx | Adds dev-server “starting” spinner indicator and prop wiring. |
| apps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown-item.tsx | Adds per-item dev-server “starting” indicator. |
| apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx | Disables start while starting globally or per-worktree; updates label/icon animation. |
| apps/ui/src/components/views/board-view/kanban-board.tsx | Adds eslint-disable for memo deps in virtualization measurement logic. |
| apps/ui/src/components/views/board-view/hooks/use-board-features.ts | Fixes deps and adds eslint-disable for stable React Query ref. |
| apps/ui/src/components/views/board-view/hooks/use-board-effects.ts | Includes merge_conflict in contexts check set. |
| apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts | Treats merge_conflict like backlog for drag-start/implement behavior. |
| apps/ui/src/components/views/board-view/hooks/use-board-column-features.ts | Adds race-condition guard for recently completed features + merge_conflict backlog handling. |
| apps/ui/src/components/views/board-view/hooks/use-board-actions.ts | Normalizes branch names (refs/remotes/origin prefixes) before worktree matching. |
| apps/ui/src/components/views/board-view/dialogs/stash-changes-dialog.tsx | Fixes typing for userAgentData platform access. |
| apps/ui/src/components/views/board-view/dialogs/mass-edit-dialog.tsx | Uses normalizeModelEntry and supports providerId in bulk updates. |
| apps/ui/src/components/views/board-view/dialogs/event-content-formatter.ts | Extracts event formatting logic for streamed output rendering. |
| apps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsx | Uses normalizeModelEntry + supports merge_conflict editing and providerId. |
| apps/ui/src/components/views/board-view/dialogs/backlog-plan-dialog.tsx | Fixes callback deps to include logger. |
| apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx | Uses MODAL_CONSTANTS for autoscroll threshold; fixes deps. |
| apps/ui/src/components/views/board-view/dialogs/agent-output-modal.constants.ts | Adds centralized AgentOutputModal constants. |
| apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx | Uses normalizeModelEntry and threads providerId into feature creation. |
| apps/ui/src/components/views/board-view/components/list-view/status-badge.tsx | Adds merge_conflict display + ordering. |
| apps/ui/src/components/views/board-view/components/list-view/row-actions.tsx | Adds merge_conflict actions (“Restart”) + memoizes setOpen. |
| apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx | Treats merge_conflict like backlog in selection/draggable conditions; threads hasContext. |
| apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx | Adds backlog logs button and provider-aware model display. |
| apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx | Adds merge-conflict warning badge. |
| apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx | Adds backlog “View Logs” + “Restart” label/icon for merge_conflict. |
| apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx | Shows model info for merge_conflict and provider-aware model naming; disables polling. |
| apps/ui/src/components/views/board-view.tsx | Makes feature creation return IDs and persists PR URLs safely; threads providerId. |
| apps/ui/src/components/ui/xterm-log-viewer.tsx | Adds eslint-disable note for mount-only initialization effect. |
| apps/ui/src/components/ui/spinner.tsx | Adds data-testid="spinner" for testability. |
| apps/ui/src/components/ui/git-diff-panel.tsx | Memoizes files array to reduce rerenders; fixes deps for stage/unstage. |
| apps/ui/src/components/ui/description-image-dropzone.tsx | Fixes callback deps to include setPreviewImages. |
| apps/ui/src/components/shared/model-override-trigger.tsx | Compares providerId when deciding whether to clear/set overrides. |
| apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts | Removes unused projects dependency usage. |
| apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx | Uses normalizeModelEntry, threads PR URL/planning defaults/dependencies, providerId. |
| apps/ui/src/components/dialogs/file-browser-dialog.tsx | Fixes callback deps to include addRecentFolder. |
| apps/ui/playwright.config.ts | Restricts Playwright discovery to *.spec.ts and ignores unit tests to avoid ESM/CJS conflicts. |
| apps/ui/package.json | Adds Testing Library + jsdom deps for Vitest UI unit tests. |
| apps/server/tests/unit/services/worktree-resolver.test.ts | Adds tests for branch-name normalization behavior. |
| apps/server/tests/unit/services/execution-service.test.ts | Tests providerId threading + worktree-not-found error behavior. |
| apps/server/tests/unit/services/dev-server-event-types.test.ts | Adds lifecycle event type tests including dev-server:starting. |
| apps/server/tests/unit/services/agent-executor.test.ts | Tests claudeCompatibleProvider is passed through to provider execution options. |
| apps/server/tests/unit/routes/backlog-plan/apply.test.ts | Tests defaults for backlog-plan apply (model/planning settings + providerId). |
| apps/server/tests/unit/providers/gemini-provider.test.ts | Adds validateBareModelId integration tests for Gemini provider. |
| apps/server/tests/unit/providers/cursor-provider.test.ts | Adds Cursor Gemini model handling + bare-model validation tests. |
| apps/server/tests/unit/providers/copilot-provider.test.ts | Updates tests for tool.execution_complete payload shape + error formatting. |
| apps/server/tests/unit/providers/codex-provider.test.ts | Adds validateBareModelId integration tests for Codex provider. |
| apps/server/src/services/worktree-resolver.ts | Normalizes branch names to improve consistent matching. |
| apps/server/src/services/pipeline-orchestrator.ts | Threads providerId and thinkingLevel through pipeline execution contexts. |
| apps/server/src/services/execution-types.ts | Adds providerId to RunAgentFn options. |
| apps/server/src/services/execution-service.ts | Errors when worktree is expected but not found; threads providerId through execution. |
| apps/server/src/services/auto-mode/facade.ts | Uses resolveProviderContext, threads providerId, and passes provider-resolved model to SDK. |
| apps/server/src/routes/github/routes/validate-issue.ts | Adds providerId support and validation for custom Claude-compatible providers. |
| apps/server/src/routes/backlog-plan/routes/apply.ts | Applies default model/planning settings when plan additions omit them; supports providerId. |
| apps/server/src/routes/backlog-plan/index.ts | Injects settingsService into apply handler. |
| apps/server/src/routes/app-spec/parse-and-create-features.ts | Sets defaults for planning mode and approval requirements. |
| apps/server/src/providers/gemini-provider.ts | Updates bare-model validation to allow expected provider prefix. |
| apps/server/src/providers/cursor-provider.ts | Updates bare-model validation with cursor exceptions (Cursor Gemini models). |
| apps/server/src/providers/copilot-provider.ts | Fixes tool event type (tool.execution_complete) + aligns payload/error formatting. |
| apps/server/src/providers/codex-provider.ts | Updates bare-model validation to allow expected provider prefix. |
| apps/server/src/providers/claude-provider.ts | Clarifies bare-model validation comment (no Claude provider prefix). |
| apps/server/src/lib/settings-helpers.ts | Adds centralized resolveProviderContext for provider/model resolution and mapping. |
| apps/server/src/index.ts | Initializes DevServerService via initialize(DATA_DIR, events) instead of setEventEmitter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+318
to
328
| if (!sdkOpts) { | ||
| logger.error( | ||
| `[createRunAgentFn] sdkOpts is UNDEFINED! createAutoModeOptions type: ${typeof createAutoModeOptions}` | ||
| ); | ||
| } | ||
|
|
||
| logger.info( | ||
| `[createRunAgentFn] Feature ${featureId}: model=${resolvedModel}, ` + | ||
| `[createRunAgentFn] Feature ${featureId}: model=${resolvedModel} (resolved=${providerResolvedModel || resolvedModel}), ` + | ||
| `maxTurns=${sdkOpts.maxTurns}, allowedTools=${(sdkOpts.allowedTools as string[])?.length ?? 'default'}, ` + | ||
| `provider=${provider.getName()}` | ||
| ); |
There was a problem hiding this comment.
The if (!sdkOpts) branch logs an error but then the code immediately dereferences sdkOpts.maxTurns/sdkOpts.allowedTools, which would still crash if sdkOpts were ever falsy. Since createAutoModeOptions returns an Options object (and throws on validation), this check appears unnecessary; either remove it or make it a real guard that returns/throws before using sdkOpts.
Comment on lines
+217
to
+224
| } else if (recentlyCompleted.has(f.id)) { | ||
| // Feature recently completed - skip placing in backlog to prevent race condition | ||
| // where stale cache has status='backlog' but feature actually completed. | ||
| // The feature will be placed correctly once the cache refreshes. | ||
| // Log for debugging (can remove after verification) | ||
| console.debug( | ||
| `Feature ${f.id} recently completed - skipping backlog placement during cache refresh` | ||
| ); |
There was a problem hiding this comment.
This console.debug runs in the feature-mapping loop and will log in production. Consider removing it or routing through the app logger behind a debug flag to avoid noisy consoles/perf overhead during normal board rendering.
Comment on lines
+401
to
+407
| // Remove from starting set | ||
| setStartingServers((prev) => { | ||
| if (!prev.has(key)) return prev; | ||
| const next = new Set(prev); | ||
| next.delete(key); | ||
| return next; | ||
| }); |
There was a problem hiding this comment.
startingServers entries are removed on dev-server:started, but they are not cleared on other terminal lifecycle events (e.g. dev-server:stopped). If the backend emits dev-server:starting and then fails fast / stops, the UI can remain stuck showing a “starting” spinner; consider also deleting the key in the stopped handler.
Comment on lines
+37
to
+68
| // Track previous feature IDs to detect when features list has been refreshed | ||
| const prevFeatureIdsRef = useRef<Set<string>>(new Set()); | ||
|
|
||
| // Clear recently completed features when the cache refreshes with updated statuses. | ||
| // | ||
| // RACE CONDITION SCENARIO THIS PREVENTS: | ||
| // 1. Feature completes on server -> status becomes 'verified'/'completed' on disk | ||
| // 2. Server emits auto_mode_feature_complete event | ||
| // 3. Frontend receives event -> removes feature from runningTasks, adds to recentlyCompletedFeatures | ||
| // 4. React Query invalidates features query, triggers async refetch | ||
| // 5. RACE: Before refetch completes, component may re-render with stale cache data | ||
| // where status='backlog' and feature is no longer in runningTasks | ||
| // 6. This hook prevents the feature from appearing in backlog during that window | ||
| // | ||
| // When the refetch completes with fresh data (status='verified'/'completed'), | ||
| // this effect clears the recentlyCompletedFeatures set since it's no longer needed. | ||
| useEffect(() => { | ||
| const currentIds = new Set(features.map((f) => f.id)); | ||
|
|
||
| // Check if any recently completed features now have terminal statuses in the new data | ||
| // If so, we can clear the tracking since the cache is now fresh | ||
| const hasUpdatedStatus = Array.from(recentlyCompletedFeatures).some((featureId) => { | ||
| const feature = features.find((f) => f.id === featureId); | ||
| return feature && (feature.status === 'verified' || feature.status === 'completed'); | ||
| }); | ||
|
|
||
| if (hasUpdatedStatus) { | ||
| clearRecentlyCompletedFeatures(); | ||
| } | ||
|
|
||
| prevFeatureIdsRef.current = currentIds; | ||
| }, [features, recentlyCompletedFeatures, clearRecentlyCompletedFeatures]); |
There was a problem hiding this comment.
prevFeatureIdsRef is assigned on each render but never read anywhere in this hook. This is dead code and will likely fail lint/TS unused-variable checks; either remove it or use it for an actual refresh-detection condition.
Comment on lines
+758
to
+776
| if (providerId) { | ||
| provider = providers.find((p) => p.id === providerId); | ||
| if (provider) { | ||
| if (!isProviderEnabled(provider)) { | ||
| logger.warn( | ||
| `${logPrefix} Explicitly requested provider "${provider.name}" (${providerId}) is disabled (enabled=${provider.enabled})` | ||
| ); | ||
| } else { | ||
| logger.debug( | ||
| `${logPrefix} Found provider "${provider.name}" (${providerId}), enabled=${provider.enabled ?? 'undefined (treated as enabled)'}` | ||
| ); | ||
| // Find the model config within this provider to check for mappings | ||
| modelConfig = findModelInProvider(provider, modelId); | ||
| if (!modelConfig && provider.models && provider.models.length > 0) { | ||
| logger.debug( | ||
| `${logPrefix} Model "${modelId}" not found in provider "${provider.name}". Available models: ${provider.models.map((m) => m.id).join(', ')}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
In resolveProviderContext, when an explicit providerId is found but enabled === false, the function logs a warning but still returns the disabled provider in the result. This contradicts the comment about searching "across enabled providers" and differs from getPhaseModelWithOverrides which drops disabled providers; consider setting provider back to undefined (and allowing fallback resolution) when the provider is disabled.
The hicolor icon theme index only lists sizes up to 512x512, so an icon installed only at 1024x1024 is invisible to GNOME/KDE's theme resolver, causing both the app launcher and taskbar to show a generic icon. Additionally, BrowserWindow.icon cannot be read by the window manager when the file is inside app.asar. - extraResources: copy logo_larger.png to resources/ (outside asar) so it lands at /opt/Automaker/resources/logo_larger.png on install - linux.desktop.Icon: set to the absolute resources path, bypassing the hicolor theme lookup and its size constraints entirely - icon-manager.ts: on Linux production use process.resourcesPath so BrowserWindow receives a real filesystem path the WM can read directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
electron-builder v26 rejects arbitrary keys in linux.desktop — the correct schema wraps custom .desktop overrides inside desktop.entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without app.setDesktopName(), the window manager cannot associate the
running Electron process with automaker.desktop. GNOME/KDE fall back to
_NET_WM_ICON which defaults to Electron's own bundled icon.
Calling app.setDesktopName('automaker.desktop') before any window is
created sets the _GTK_APPLICATION_ID hint and XDG app_id so the WM
picks up the desktop entry's Icon for the taskbar.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR AutoMaker-Org#813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
apps/ui/tests/utils/components/responsive-modal.ts (2)
87-89:⚠️ Potential issue | 🟠 MajorUse token-based class assertions instead of
new RegExp(className)withtoHaveClass.Line 88 is brittle: Tailwind class tokens (e.g.,
md:w-[90vw]) contain regex metacharacters, sonew RegExp(className)does not reliably assert class presence.Proposed fix
for (const className of expectedClasses) { - await expect(modal).toHaveClass(new RegExp(className)); + await expect(modal).toContainClass(className); }Playwright LocatorAssertions docs: for checking whether a single CSS class token exists among many classes, should `toContainClass()` be used instead of `toHaveClass()`? Also clarify behavior when passing RegExp to `toHaveClass()`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 87 - 89, The test is brittle because it constructs a RegExp from Tailwind tokens (in expectedClasses) and passes it to expect(modal).toHaveClass, which treats regex metacharacters literally; change the assertions to use token-based class checks by replacing the toHaveClass(new RegExp(className)) calls with Playwright's toContainClass (e.g., call expect(modal).toContainClass(className)) so each Tailwind token in expectedClasses is asserted as a single class token; update the loop that iterates expectedClasses and keep modal and expectedClasses identifiers unchanged.
113-123:⚠️ Potential issue | 🟡 MinorDesktop/desktopLarge branches still have no width/height assertions.
Lines 113-123 and Lines 147-156 only validate mobile/tablet. Desktop variants can pass with zero coverage.
Proposed fix
if (viewport === 'mobile' || viewport === 'mobileLarge') { // Mobile: should be close to full width with 2rem margins expect(modalWidth).toBeGreaterThan(viewportWidth - 40); expect(modalWidth).toBeLessThan(viewportWidth - 20); } else if (viewport === 'tablet' || viewport === 'tabletLarge') { // Tablet: should be around 90vw but not exceed max-w-[1200px] const expected90vw = Math.floor(viewportWidth * 0.9); expect(modalWidth).toBeLessThanOrEqual(expected90vw); expect(modalWidth).toBeLessThanOrEqual(1200); + } else if (viewport === 'desktop' || viewport === 'desktopLarge') { + expect(modalWidth).toBeGreaterThan(0); + expect(modalWidth).toBeLessThanOrEqual(Math.min(viewportWidth, 1200)); }if (viewport === 'mobile' || viewport === 'mobileLarge') { // Mobile: should be max-h-[85dvh] const expected85dvh = Math.floor(viewportHeight * 0.85); expect(modalHeight).toBeLessThanOrEqual(expected85dvh); } else if (viewport === 'tablet' || viewport === 'tabletLarge') { // Tablet: should be max-h-[85vh] const expected85vh = Math.floor(viewportHeight * 0.85); expect(modalHeight).toBeLessThanOrEqual(expected85vh); + } else if (viewport === 'desktop' || viewport === 'desktopLarge') { + expect(modalHeight).toBeGreaterThan(0); + expect(modalHeight).toBeLessThanOrEqual(viewportHeight); }Also applies to: 147-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 113 - 123, Add assertions for the desktop branches: inside the conditional for viewport === 'desktop' || viewport === 'desktopLarge' (same place that checks mobile/tablet) assert modalWidth is bounded (e.g., <= Math.floor(viewportWidth * 0.9) and <= 1200 or whatever max width your design requires) and assert modalHeight relative to viewportHeight (e.g., modalHeight <= Math.floor(viewportHeight * 0.9) and > 0) using the existing modalWidth, modalHeight, viewportWidth, and viewportHeight variables so desktop variants cannot pass with no coverage.
🧹 Nitpick comments (13)
apps/ui/scripts/setup-e2e-fixtures.mjs (1)
151-154: Use unconditional recursive mkdir to simplify and avoid check-then-act branching.This block can be reduced to one idempotent call and removes the
existsSyncpre-check.♻️ Proposed refactor
- if (!fs.existsSync(CONTEXT_DIR)) { - fs.mkdirSync(CONTEXT_DIR, { recursive: true }); - console.log(`Created directory: ${CONTEXT_DIR}`); - } + fs.mkdirSync(CONTEXT_DIR, { recursive: true }); + console.log(`Ensured directory exists: ${CONTEXT_DIR}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/scripts/setup-e2e-fixtures.mjs` around lines 151 - 154, Remove the check-then-act branching around CONTEXT_DIR by deleting the fs.existsSync guard and call fs.mkdirSync(CONTEXT_DIR, { recursive: true }) unconditionally (keep the existing console.log if you want the creation message); update the block that currently references fs.existsSync and fs.mkdirSync so it only calls fs.mkdirSync with the recursive option for the CONTEXT_DIR constant.apps/server/src/routes/setup/routes/verify-codex-auth.ts (1)
85-89: Clean mock mode implementation for E2E/CI.The early exit pattern is well-placed before rate limiting and session creation, preventing unnecessary resource allocation during tests. The response shape correctly matches the real success response at line 264.
Consider adding a debug log for traceability when mock mode is active—this helps diagnose unexpected behavior if the env var leaks to non-test environments:
💡 Optional: Add debug logging
// In E2E/CI mock mode, skip real API calls if (process.env.AUTOMAKER_MOCK_AGENT === 'true') { + logger.debug('[Setup] Mock mode active - skipping Codex auth verification'); res.json({ success: true, authenticated: true }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/setup/routes/verify-codex-auth.ts` around lines 85 - 89, Add a debug log when mock mode is active: in the verify-codex-auth route where it checks process.env.AUTOMAKER_MOCK_AGENT === 'true' (right before the early return that calls res.json({ success: true, authenticated: true })), emit a short debug message indicating mock-mode auth was returned (use the existing logger if available, e.g., processLogger.debug or fallback to console.debug) so test/CI runs are traceable without changing behavior.apps/ui/tests/context/context-file-management.spec.ts (1)
31-34: Consider removing redundant authentication call.The
navigateToContextfunction already "Calls authenticateForTests(page) to ensure authenticated session" internally. The explicit call on line 31 is redundant and adds unnecessary overhead.The reordering of
waitForNetworkIdleto after navigation is a sensible improvement.♻️ Proposed simplification
test('should create a new markdown context file', async ({ page }) => { await setupProjectWithFixture(page, getFixturePath()); - await authenticateForTests(page); await navigateToContext(page); await waitForNetworkIdle(page);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/context/context-file-management.spec.ts` around lines 31 - 34, Remove the redundant explicit call to authenticateForTests(page) before navigation: navigateToContext(page) already calls authenticateForTests(page) internally, so delete the standalone await authenticateForTests(page) invocation, keep await navigateToContext(page) and then await waitForNetworkIdle(page) (ensuring the network-idle wait remains after navigation), and run tests to confirm no regressions from removing the duplicate authentication call.apps/server/src/routes/fs/routes/read.ts (1)
30-32: Consider extractingisENOENTto a shared location.This utility is duplicated in
apps/server/src/routes/worktree/common.tswith an identical implementation. You could extract it toapps/server/src/routes/common.tsalongsidegetErrorMessageto avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/fs/routes/read.ts` around lines 30 - 32, Extract the isENOENT helper into a shared module alongside getErrorMessage (e.g., create or reuse a common module) instead of duplicating it; move the function implementation from read.ts (function isENOENT) into that shared module, export it, then update both read.ts and worktree common file to import isENOENT from the shared module and remove their local copies to eliminate duplication.apps/ui/tests/features/edit-feature.spec.ts (1)
101-102: Consider a type guard for better type narrowing and error clarity.The
expect(featureId).toBeTruthy()assertion validates at runtime but doesn't narrow TypeScript's type, requiring the|| ''fallback on line 142. A type guard would handle both concerns and provide a clearer failure message.🔧 Suggested refactor
const cardTestId = await featureCard.getAttribute('data-testid'); const featureId = cardTestId?.replace('kanban-card-', ''); - expect(featureId).toBeTruthy(); + if (!featureId) { + throw new Error('Could not extract feature ID from card test-id'); + }Then on line 142, you can safely use
featureIdwithout the fallback:'.automaker', 'features', - featureId || '', + featureId, 'feature.json'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/edit-feature.spec.ts` around lines 101 - 102, The current runtime check expect(featureId).toBeTruthy() doesn't narrow TypeScript types, causing the later use of featureId to require a fallback; replace this with a proper type guard or assertion that narrows the type (e.g., an isDefined<T>(v): v is T or assertDefined(value, message)) and apply it to cardTestId (or featureId) before computing featureId; update the test to call that guard (with a clear failure message like "cardTestId is missing") so featureId can be used without the "|| ''" fallback in subsequent code (referencing featureId and cardTestId in the test).apps/server/src/providers/provider-factory.ts (1)
70-78: MoveMockProviderimport to the top with other functional imports.
MockProvideris imported at line 294 alongside providers that are registered for side-effects, but unlike those, it's not registered viaregisterProvider(). It's used directly ingetMockProvider()(line 75). While ES module hoisting makes this work, the placement is misleading since line 293's comment states these imports are for "registration side-effects."♻️ Suggested refactor
Move the import near the top with other direct-use imports:
import { BaseProvider } from './base-provider.js'; import type { InstallationStatus, ModelDefinition } from './types.js'; +import { MockProvider } from './mock-provider.js'; import { isCursorModel,And remove it from line 294:
// Import providers for registration side-effects -import { MockProvider } from './mock-provider.js'; import { ClaudeProvider } from './claude-provider.js';Also applies to: 294-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/provider-factory.ts` around lines 70 - 78, The MockProvider import currently lives in the side-effect registration block but is directly instantiated by getMockProvider() using mockProviderInstance; move the MockProvider import up into the same section as other functional imports (near other direct-use imports) so its purpose is clear, update any import ordering to remove it from the side-effect imports area (the block that calls registerProvider()), and ensure getMockProvider() continues to reference the relocated MockProvider without changing its logic.apps/ui/tests/utils/helpers/temp-dir.ts (1)
9-11: Potential path collision in parallel test runs.Unlike
createTempDirPathinapps/ui/tests/utils/git/worktree.tswhich includesMath.random()for uniqueness, this version only usesprocess.pid. If multiple test suites call this function with the same prefix within a single process (or if tests are forked with the same PID across machines), paths may collide.Consider adding randomness for robustness:
🛠️ Suggested fix
export function createTempDirPath(prefix: string): string { - return path.join(os.tmpdir(), `automaker-test-${prefix}-${process.pid}`); + const uniqueId = `${process.pid}-${Math.random().toString(36).substring(2, 9)}`; + return path.join(os.tmpdir(), `automaker-test-${prefix}-${uniqueId}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/helpers/temp-dir.ts` around lines 9 - 11, createTempDirPath currently builds temp paths using only process.pid which can collide across parallel tests; update the function (createTempDirPath) to append a short random component (e.g., Math.random() hex, Date.now(), or crypto.randomUUID()) to the generated folder name so each invocation yields a unique path even within the same PID, and ensure the random piece is URL/file-system safe when constructing the returned path.apps/ui/tests/utils/git/worktree.ts (1)
39-45: Note: DuplicatecreateTempDirPathfunction exists.There's a similar
createTempDirPathfunction inapps/ui/tests/utils/helpers/temp-dir.tswith different behavior (no randomness, usesos.tmpdir()instead of workspacetest/dir). This could cause confusion about which helper to use.Consider either:
- Consolidating into a single location with clear naming
- Documenting the different use cases for each variant
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/git/worktree.ts` around lines 39 - 45, There are two conflicting helpers named createTempDirPath (one in createTempDirPath and another in temp-dir.ts); consolidate them by choosing a single source of truth (or rename one) and update callers to import that one: either merge the workspace-under-test behavior into a single helper (e.g., export createWorkspaceTempDirPath from the file currently containing createTempDirPath and remove the duplicate in temp-dir.ts), or keep both but rename to distinct, descriptive names (e.g., createWorkspaceTempDirPath vs createOsTmpDirPath) and document their use cases; then update all imports/usages to the chosen symbol(s) and remove the redundant function to avoid confusion.apps/ui/tests/utils/core/safe-paths.ts (1)
12-18: Potential false positive in workspace root detection.The check
cwd.includes('apps/ui')could incorrectly match paths like/my-apps/ui-lib/projector/home/user/apps/ui-tests/. Consider using a more precise check:🛠️ Suggested fix
export function getWorkspaceRoot(): string { const cwd = process.cwd(); - if (cwd.includes('apps/ui')) { + // Check if cwd ends with apps/ui or contains apps/ui as a path segment + if (cwd.endsWith(`${path.sep}apps${path.sep}ui`) || cwd.includes(`${path.sep}apps${path.sep}ui${path.sep}`)) { return path.resolve(cwd, '../..'); } return cwd; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/core/safe-paths.ts` around lines 12 - 18, The workspace-root detection in getWorkspaceRoot() is too permissive because cwd.includes('apps/ui') matches substrings; update getWorkspaceRoot to normalize the cwd (using path.normalize), split it by path.sep and check for the exact segment sequence ['apps','ui'] (or use a RegExp that enforces path boundaries such as (^|sep)appssepui(sep|$)) before resolving to '../..', otherwise return cwd; reference the getWorkspaceRoot function, process.cwd(), path.normalize and path.sep to locate and implement the fix.apps/ui/tests/global-setup.ts (1)
18-22: Preserve the actual failure reason when the readiness check fails.The catch currently always reports “nothing is listening,” which is misleading for reachable-but-unhealthy responses (for example HTTP 404/500). Include the original failure reason in the thrown error.
💡 Proposed tweak
- } catch { - throw new Error( - `TEST_REUSE_SERVER is set but nothing is listening at ${baseURL}. ` + - 'Start the UI and server first (e.g. from apps/ui: TEST_PORT=3107 TEST_SERVER_PORT=3108 pnpm dev; from apps/server: PORT=3108 pnpm run dev:test) or run tests without TEST_REUSE_SERVER.' - ); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw new Error( + `TEST_REUSE_SERVER is set but ${baseURL} is not ready (${reason}). ` + + 'Start the UI and server first (e.g. from apps/ui: TEST_PORT=3107 TEST_SERVER_PORT=3108 pnpm dev; from apps/server: PORT=3108 pnpm run dev:test) or run tests without TEST_REUSE_SERVER.' + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/global-setup.ts` around lines 18 - 22, The catch block in apps/ui/tests/global-setup.ts swallows the original failure and always throws “nothing is listening”; change the catch to capture the error (e.g., catch (err)) and include the original error details (err.message or err.toString(), and if available response.status/responseText) in the thrown Error along with the existing guidance text referencing baseURL and TEST_REUSE_SERVER so the thrown error shows the real failure reason.apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx (2)
10-10: Use a stable intra-app barrel import forAgentOutputModal.Line 10 uses a deep relative source path, which is brittle during refactors. Prefer importing from the app’s component barrel/index export instead of
../../../src/....Based on learnings: In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components rather than direct path imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx` at line 10, Replace the brittle deep import of AgentOutputModal with the app’s stable component barrel export: update the import of AgentOutputModal in the test (agent-output-modal-integration.test.tsx) to import from the intra-app barrel (components/index) instead of '../../../src/components/views/board-view/dialogs/agent-output-modal'; keep the imported symbol name AgentOutputModal unchanged so references in the test remain valid.
115-119: Replace theonCloseexistence check with an interaction assertion.This test currently verifies only that the mock function exists, not that the modal wiring invokes it. Trigger a close interaction (e.g., close button or Escape) and assert
onClosewas called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx` around lines 115 - 119, The test currently only checks that defaultProps.onClose exists; instead simulate a user closing the modal and assert the mock was invoked: render AgentOutputModal with defaultProps, locate the modal close control (e.g., the close button by role/label or use Escape key) using the same selectors used elsewhere in tests, perform the interaction with fireEvent or userEvent (click the close button or fire Escape on document), and assert defaultProps.onClose has been called; update the test name to reflect the interaction (e.g., "calls onClose when modal is closed").apps/server/src/services/dev-server-service.ts (1)
188-189: Prefer atomic writes fordev-servers.json.Line 188 writes directly to the target file. If interrupted, the JSON can be truncated/corrupted and break restore on next boot. Write to a temp file and
renameatomically.Suggested atomic write pattern
- await fs.writeFile(statePath, JSON.stringify(persistedInfo, null, 2)); + const tmpPath = `${statePath}.tmp`; + await fs.writeFile(tmpPath, JSON.stringify(persistedInfo, null, 2)); + await fs.rename(tmpPath, statePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 188 - 189, The current write using fs.writeFile(statePath, JSON.stringify(persistedInfo, null, 2)) can corrupt dev-servers.json if interrupted; change to an atomic write: write the JSON to a temp file (e.g., `${statePath}.tmp` or use a unique temp name), fsync the temp file if possible, then rename the temp to statePath using fs.rename (atomic on most platforms) and only log via logger.debug after the rename; update the logic around persistedInfo/statePath to use this temp->rename pattern and handle/propagate errors from write/fsync/rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/fs/routes/read.ts`:
- Around line 15-18: The isOptionalFile function incorrectly uses
filePath.endsWith(optionalFile) which can false-match filenames (e.g.,
"my-categories.json"); update isOptionalFile to compare exact basenames by using
path.basename(filePath) === optionalFile for each entry in OPTIONAL_FILES
(import Node's path if not already), return true when a match is found and false
otherwise to preserve current semantics.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 216-218: The code reads JSON from statePath into persistedInfo
typed as PersistedDevServerInfo[] without runtime checks; add a validation step
that verifies each entry has the expected keys/types (strings for IDs/paths,
numbers for ports/pids), enforces numeric ranges (e.g., port within 1–65535, pid
> 0), and drops or logs invalid rows before rehydration; apply the same
validation/hardening to the other parsing block around lines 229-233 so only
well-formed PersistedDevServerInfo objects are merged back into runtime state
(this prevents malformed data from reaching port/process-kill code paths).
- Around line 229-240: Reattached DevServerInfo entries are created with
process: null so the current stale-pruning (which checks
server.process.exitCode) never removes them; update server-read paths (where
runningServers is read/returned, e.g., any get/list/refresh functions that
reference runningServers) to treat process === null as a special liveness case:
perform a port probe against the allocatedPort (or URL) for that DevServerInfo
and if the probe fails, remove the entry from runningServers and allocatedPorts
(prune it); keep successful probes as confirmation of liveness and leave the
entry intact. Ensure you reference DevServerInfo, runningServers,
allocatedPorts, and the existing server.process.exitCode check when adding this
probe-and-prune path.
In `@apps/ui/src/components/views/graph-view-page.tsx`:
- Around line 117-129: The branchCardCounts computation is hardcoding 'main'
when feature.branchName is missing; update the useMemo in branchCardCounts to
use a dynamic default branch instead of the literal 'main' (e.g. accept or read
a repoDefaultBranch value from props/context or call an existing
getRepoDefaultBranch() helper) and fall back to 'main' only if that dynamic
value is undefined; specifically change the fallback for (feature.branchName as
string | undefined) ?? 'main' to use the dynamic default (e.g. ??
repoDefaultBranch ?? 'main') and ensure repoDefaultBranch is provided where
hookFeatures is available.
- Around line 167-195: loadBranchesForWorktree can apply stale async responses
to state; protect against races by adding a request-scoped token (e.g., a ref
counter or generated id stored in a useRef like latestLoadBranchesId) before
calling api.worktree.listBranches, capture that id in the async closure, and
only call setBranches and setIsLoadingBranches(false) if the id matches the
current latestLoadBranchesId; ensure you update latestLoadBranchesId when
starting a new request and clear/reset appropriately so older responses are
ignored.
In `@apps/ui/tests/context/mobile-context-view.spec.ts`:
- Around line 265-266: The width assertion in the test using fileListBox.width
is too permissive (currently toBeGreaterThan(200)); tighten it to reflect the
desktop target (w-64 = 256px) while still allowing chrome/sidebar tolerance —
for example change the expectation to
expect(fileListBox.width).toBeGreaterThan(240) (or toBeGreaterThanOrEqual(256)
if no tolerance needed) in the failing test in mobile-context-view.spec.ts so
the test will catch regressions smaller than the intended desktop size.
In `@apps/ui/tests/e2e-testing-guide.md`:
- Around line 84-85: The doc has inconsistent path-prefix usage: the bullet list
uses the literal "test/" while other sections use "tests/"; update the bullets
so they use the same prefix string used elsewhere (i.e., replace "test/" with
"tests/" or vice versa to match the rest of the guide) so examples like
createTempDirPath(...) and fixture paths are consistent across the document;
ensure both the "Allowed" and "Not allowed" examples and any inline code
snippets use the chosen prefix ("tests/") uniformly.
In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx`:
- Around line 354-369: The tests currently only assert static UI text and don't
verify that AgentOutputModal passes the project path to the hook; update both
tests to spy/mock the useAgentOutput hook (or the downstream function it calls)
and assert it was called with the expected project path: for the first test
confirm useAgentOutput (or the underlying fetch/loader) receives the explicit
projectPath '/custom/project/path', and for the second test set
window.__currentProject = { path: '/fallback/project' } and assert
useAgentOutput receives '/fallback/project'; keep using AgentOutputModal in the
render calls and restore any mocks and the original window.__currentProject
afterward.
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 183-194: The helper testModalResponsiveResize only asserts
behavior for two specific transitions and silently returns for all other
fromViewport/toViewport pairs; update the function to cover every viewport
transition (use the existing variables fromViewport, toViewport, newDimensions,
toSize) by adding assertions for the remaining combinations or a final default
branch that validates expected width/maxWidth constraints (or throws/fails the
test) so no transition passes without validation; implement this as a switch or
expanded if/else covering all viewport pairs to ensure each path asserts
expected dimensions.
In `@apps/ui/tests/utils/navigation/views.ts`:
- Around line 60-74: Replace the fixed 500ms sleep + single visibility probe
with a bounded wait that repeatedly checks for either the context-view or
context-view-loading to become visible (e.g., use page.waitForSelector /
page.waitForFunction with a reasonable timeout instead of await
page.waitForTimeout(500)). Update the logic around the noProject locator and the
viewNow/loadingNow checks so that when noProject.isVisible() is true you wait
until either '[data-testid="context-view"]' or
'[data-testid="context-view-loading"]' is visible (or the timeout elapses)
before throwing the error; keep the same failure message and keep references to
the navigateToContext / setupProjectWithFixture contract unchanged.
In `@apps/ui/tests/utils/views/context.ts`:
- Line 167: Remove the stray "q" token that precedes the chained call ending
with "}).toPass({ timeout, intervals: [200, 500, 1000] });" so the TypeScript
parses correctly; locate the expression containing "}).toPass(...)" in
apps/ui/tests/utils/views/context.ts and delete the extraneous "q" character so
the statement becomes a valid chained call.
---
Duplicate comments:
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 87-89: The test is brittle because it constructs a RegExp from
Tailwind tokens (in expectedClasses) and passes it to expect(modal).toHaveClass,
which treats regex metacharacters literally; change the assertions to use
token-based class checks by replacing the toHaveClass(new RegExp(className))
calls with Playwright's toContainClass (e.g., call
expect(modal).toContainClass(className)) so each Tailwind token in
expectedClasses is asserted as a single class token; update the loop that
iterates expectedClasses and keep modal and expectedClasses identifiers
unchanged.
- Around line 113-123: Add assertions for the desktop branches: inside the
conditional for viewport === 'desktop' || viewport === 'desktopLarge' (same
place that checks mobile/tablet) assert modalWidth is bounded (e.g., <=
Math.floor(viewportWidth * 0.9) and <= 1200 or whatever max width your design
requires) and assert modalHeight relative to viewportHeight (e.g., modalHeight
<= Math.floor(viewportHeight * 0.9) and > 0) using the existing modalWidth,
modalHeight, viewportWidth, and viewportHeight variables so desktop variants
cannot pass with no coverage.
---
Nitpick comments:
In `@apps/server/src/providers/provider-factory.ts`:
- Around line 70-78: The MockProvider import currently lives in the side-effect
registration block but is directly instantiated by getMockProvider() using
mockProviderInstance; move the MockProvider import up into the same section as
other functional imports (near other direct-use imports) so its purpose is
clear, update any import ordering to remove it from the side-effect imports area
(the block that calls registerProvider()), and ensure getMockProvider()
continues to reference the relocated MockProvider without changing its logic.
In `@apps/server/src/routes/fs/routes/read.ts`:
- Around line 30-32: Extract the isENOENT helper into a shared module alongside
getErrorMessage (e.g., create or reuse a common module) instead of duplicating
it; move the function implementation from read.ts (function isENOENT) into that
shared module, export it, then update both read.ts and worktree common file to
import isENOENT from the shared module and remove their local copies to
eliminate duplication.
In `@apps/server/src/routes/setup/routes/verify-codex-auth.ts`:
- Around line 85-89: Add a debug log when mock mode is active: in the
verify-codex-auth route where it checks process.env.AUTOMAKER_MOCK_AGENT ===
'true' (right before the early return that calls res.json({ success: true,
authenticated: true })), emit a short debug message indicating mock-mode auth
was returned (use the existing logger if available, e.g., processLogger.debug or
fallback to console.debug) so test/CI runs are traceable without changing
behavior.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 188-189: The current write using fs.writeFile(statePath,
JSON.stringify(persistedInfo, null, 2)) can corrupt dev-servers.json if
interrupted; change to an atomic write: write the JSON to a temp file (e.g.,
`${statePath}.tmp` or use a unique temp name), fsync the temp file if possible,
then rename the temp to statePath using fs.rename (atomic on most platforms) and
only log via logger.debug after the rename; update the logic around
persistedInfo/statePath to use this temp->rename pattern and handle/propagate
errors from write/fsync/rename.
In `@apps/ui/scripts/setup-e2e-fixtures.mjs`:
- Around line 151-154: Remove the check-then-act branching around CONTEXT_DIR by
deleting the fs.existsSync guard and call fs.mkdirSync(CONTEXT_DIR, { recursive:
true }) unconditionally (keep the existing console.log if you want the creation
message); update the block that currently references fs.existsSync and
fs.mkdirSync so it only calls fs.mkdirSync with the recursive option for the
CONTEXT_DIR constant.
In `@apps/ui/tests/context/context-file-management.spec.ts`:
- Around line 31-34: Remove the redundant explicit call to
authenticateForTests(page) before navigation: navigateToContext(page) already
calls authenticateForTests(page) internally, so delete the standalone await
authenticateForTests(page) invocation, keep await navigateToContext(page) and
then await waitForNetworkIdle(page) (ensuring the network-idle wait remains
after navigation), and run tests to confirm no regressions from removing the
duplicate authentication call.
In `@apps/ui/tests/features/edit-feature.spec.ts`:
- Around line 101-102: The current runtime check expect(featureId).toBeTruthy()
doesn't narrow TypeScript types, causing the later use of featureId to require a
fallback; replace this with a proper type guard or assertion that narrows the
type (e.g., an isDefined<T>(v): v is T or assertDefined(value, message)) and
apply it to cardTestId (or featureId) before computing featureId; update the
test to call that guard (with a clear failure message like "cardTestId is
missing") so featureId can be used without the "|| ''" fallback in subsequent
code (referencing featureId and cardTestId in the test).
In `@apps/ui/tests/global-setup.ts`:
- Around line 18-22: The catch block in apps/ui/tests/global-setup.ts swallows
the original failure and always throws “nothing is listening”; change the catch
to capture the error (e.g., catch (err)) and include the original error details
(err.message or err.toString(), and if available response.status/responseText)
in the thrown Error along with the existing guidance text referencing baseURL
and TEST_REUSE_SERVER so the thrown error shows the real failure reason.
In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx`:
- Line 10: Replace the brittle deep import of AgentOutputModal with the app’s
stable component barrel export: update the import of AgentOutputModal in the
test (agent-output-modal-integration.test.tsx) to import from the intra-app
barrel (components/index) instead of
'../../../src/components/views/board-view/dialogs/agent-output-modal'; keep the
imported symbol name AgentOutputModal unchanged so references in the test remain
valid.
- Around line 115-119: The test currently only checks that defaultProps.onClose
exists; instead simulate a user closing the modal and assert the mock was
invoked: render AgentOutputModal with defaultProps, locate the modal close
control (e.g., the close button by role/label or use Escape key) using the same
selectors used elsewhere in tests, perform the interaction with fireEvent or
userEvent (click the close button or fire Escape on document), and assert
defaultProps.onClose has been called; update the test name to reflect the
interaction (e.g., "calls onClose when modal is closed").
In `@apps/ui/tests/utils/core/safe-paths.ts`:
- Around line 12-18: The workspace-root detection in getWorkspaceRoot() is too
permissive because cwd.includes('apps/ui') matches substrings; update
getWorkspaceRoot to normalize the cwd (using path.normalize), split it by
path.sep and check for the exact segment sequence ['apps','ui'] (or use a RegExp
that enforces path boundaries such as (^|sep)appssepui(sep|$)) before resolving
to '../..', otherwise return cwd; reference the getWorkspaceRoot function,
process.cwd(), path.normalize and path.sep to locate and implement the fix.
In `@apps/ui/tests/utils/git/worktree.ts`:
- Around line 39-45: There are two conflicting helpers named createTempDirPath
(one in createTempDirPath and another in temp-dir.ts); consolidate them by
choosing a single source of truth (or rename one) and update callers to import
that one: either merge the workspace-under-test behavior into a single helper
(e.g., export createWorkspaceTempDirPath from the file currently containing
createTempDirPath and remove the duplicate in temp-dir.ts), or keep both but
rename to distinct, descriptive names (e.g., createWorkspaceTempDirPath vs
createOsTmpDirPath) and document their use cases; then update all imports/usages
to the chosen symbol(s) and remove the redundant function to avoid confusion.
In `@apps/ui/tests/utils/helpers/temp-dir.ts`:
- Around line 9-11: createTempDirPath currently builds temp paths using only
process.pid which can collide across parallel tests; update the function
(createTempDirPath) to append a short random component (e.g., Math.random() hex,
Date.now(), or crypto.randomUUID()) to the generated folder name so each
invocation yields a unique path even within the same PID, and ensure the random
piece is URL/file-system safe when constructing the returned path.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
apps/server/src/providers/mock-provider.tsapps/server/src/providers/provider-factory.tsapps/server/src/routes/fs/routes/read.tsapps/server/src/routes/setup/routes/verify-claude-auth.tsapps/server/src/routes/setup/routes/verify-codex-auth.tsapps/server/src/routes/worktree/routes/list.tsapps/server/src/services/dev-server-service.tsapps/ui/package.jsonapps/ui/playwright.config.tsapps/ui/scripts/kill-test-servers.mjsapps/ui/scripts/setup-e2e-fixtures.mjsapps/ui/src/components/ui/spinner.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsxapps/ui/src/components/views/graph-view-page.tsxapps/ui/src/components/views/graph-view/graph-canvas.tsxapps/ui/src/components/views/graph-view/graph-view.tsxapps/ui/tests/context/add-context-image.spec.tsapps/ui/tests/context/context-file-management.spec.tsapps/ui/tests/context/delete-context-file.spec.tsapps/ui/tests/context/desktop-context-view.spec.tsapps/ui/tests/context/mobile-context-operations.spec.tsapps/ui/tests/context/mobile-context-view.spec.tsapps/ui/tests/e2e-testing-guide.mdapps/ui/tests/features/edit-feature.spec.tsapps/ui/tests/features/running-task-card-display.spec.tsapps/ui/tests/global-setup.tsapps/ui/tests/memory/desktop-memory-view.spec.tsapps/ui/tests/memory/mobile-memory-operations.spec.tsapps/ui/tests/memory/mobile-memory-view.spec.tsapps/ui/tests/unit/components/agent-output-modal-integration.test.tsxapps/ui/tests/utils/components/responsive-modal.tsapps/ui/tests/utils/core/constants.tsapps/ui/tests/utils/core/safe-paths.tsapps/ui/tests/utils/git/worktree.tsapps/ui/tests/utils/helpers/temp-dir.tsapps/ui/tests/utils/index.tsapps/ui/tests/utils/navigation/views.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/project/setup.tsapps/ui/tests/utils/views/context.tsapps/ui/tests/utils/views/memory.tslibs/types/src/provider-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/package.json
- libs/types/src/provider-utils.ts
Comment on lines
+229
to
+240
| const serverInfo: DevServerInfo = { | ||
| ...info, | ||
| startedAt: new Date(info.startedAt), | ||
| process: null, // Process object is lost, but we know it's running | ||
| scrollbackBuffer: '', | ||
| outputBuffer: '', | ||
| flushTimeout: null, | ||
| stopping: false, | ||
| urlDetectionTimeout: null, | ||
| }; | ||
| this.runningServers.set(info.worktreePath, serverInfo); | ||
| this.allocatedPorts.add(info.allocatedPort); |
Contributor
There was a problem hiding this comment.
Reattached servers can remain stale indefinitely after later exits.
At Line 232, restored entries have process: null. Current stale-pruning logic relies on server.process.exitCode, so these entries won’t auto-prune if the external process dies after startup reattachment. Add a liveness path for process === null (port probe + prune) in server-read paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/dev-server-service.ts` around lines 229 - 240,
Reattached DevServerInfo entries are created with process: null so the current
stale-pruning (which checks server.process.exitCode) never removes them; update
server-read paths (where runningServers is read/returned, e.g., any
get/list/refresh functions that reference runningServers) to treat process ===
null as a special liveness case: perform a port probe against the allocatedPort
(or URL) for that DevServerInfo and if the probe fails, remove the entry from
runningServers and allocatedPorts (prune it); keep successful probes as
confirmation of liveness and leave the entry intact. Ensure you reference
DevServerInfo, runningServers, allocatedPorts, and the existing
server.process.exitCode check when adding this probe-and-prune path.
Comment on lines
+354
to
+369
| it('should use projectPath prop when provided', () => { | ||
| const projectPath = '/custom/project/path'; | ||
| render(<AgentOutputModal {...defaultProps} projectPath={projectPath} />); | ||
|
|
||
| expect(screen.getByText('Implement a responsive navigation menu')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should fallback to window.__currentProject when projectPath is not provided', () => { | ||
| const previousProject = window.__currentProject; | ||
| try { | ||
| window.__currentProject = { path: '/fallback/project' }; | ||
| render(<AgentOutputModal {...defaultProps} />); | ||
| expect(screen.getByText('Implement a responsive navigation menu')).toBeInTheDocument(); | ||
| } finally { | ||
| window.__currentProject = previousProject; | ||
| } |
Contributor
There was a problem hiding this comment.
Project-path tests don’t currently validate project-path resolution behavior.
Both tests assert static description text, so they can pass even if path resolution breaks. Assert useAgentOutput (or the downstream call) receives the explicit projectPath and fallback window.__currentProject.path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/unit/components/agent-output-modal-integration.test.tsx` around
lines 354 - 369, The tests currently only assert static UI text and don't verify
that AgentOutputModal passes the project path to the hook; update both tests to
spy/mock the useAgentOutput hook (or the downstream function it calls) and
assert it was called with the expected project path: for the first test confirm
useAgentOutput (or the underlying fetch/loader) receives the explicit
projectPath '/custom/project/path', and for the second test set
window.__currentProject = { path: '/fallback/project' } and assert
useAgentOutput receives '/fallback/project'; keep using AgentOutputModal in the
render calls and restore any mocks and the original window.__currentProject
afterward.
Comment on lines
+183
to
+194
| if (fromViewport === 'mobile' && toViewport === 'tablet') { | ||
| const widthPx = parseFloat(newDimensions.width); | ||
| const maxWidthPx = parseFloat(newDimensions.maxWidth); | ||
| const expected90vw = toSize.width * 0.9; | ||
| expect(widthPx).toBeLessThanOrEqual(expected90vw + 2); | ||
| expect(maxWidthPx).toBeGreaterThanOrEqual(1200); | ||
| } else if (fromViewport === 'tablet' && toViewport === 'mobile') { | ||
| const widthPx = parseFloat(newDimensions.width); | ||
| const maxWidthPx = parseFloat(newDimensions.maxWidth); | ||
| expect(widthPx).toBeGreaterThan(toSize.width - 60); | ||
| expect(maxWidthPx).toBeLessThan(1200); | ||
| } |
Contributor
There was a problem hiding this comment.
testModalResponsiveResize can silently pass for most viewport pairs.
Only two transitions are asserted. For other fromViewport/toViewport combinations, the helper exits without validation.
Proposed fix
if (fromViewport === 'mobile' && toViewport === 'tablet') {
const widthPx = parseFloat(newDimensions.width);
const maxWidthPx = parseFloat(newDimensions.maxWidth);
const expected90vw = toSize.width * 0.9;
expect(widthPx).toBeLessThanOrEqual(expected90vw + 2);
expect(maxWidthPx).toBeGreaterThanOrEqual(1200);
} else if (fromViewport === 'tablet' && toViewport === 'mobile') {
const widthPx = parseFloat(newDimensions.width);
const maxWidthPx = parseFloat(newDimensions.maxWidth);
expect(widthPx).toBeGreaterThan(toSize.width - 60);
expect(maxWidthPx).toBeLessThan(1200);
+ } else {
+ throw new Error(
+ `No resize assertion configured for transition: ${fromViewport} -> ${toViewport}`
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (fromViewport === 'mobile' && toViewport === 'tablet') { | |
| const widthPx = parseFloat(newDimensions.width); | |
| const maxWidthPx = parseFloat(newDimensions.maxWidth); | |
| const expected90vw = toSize.width * 0.9; | |
| expect(widthPx).toBeLessThanOrEqual(expected90vw + 2); | |
| expect(maxWidthPx).toBeGreaterThanOrEqual(1200); | |
| } else if (fromViewport === 'tablet' && toViewport === 'mobile') { | |
| const widthPx = parseFloat(newDimensions.width); | |
| const maxWidthPx = parseFloat(newDimensions.maxWidth); | |
| expect(widthPx).toBeGreaterThan(toSize.width - 60); | |
| expect(maxWidthPx).toBeLessThan(1200); | |
| } | |
| if (fromViewport === 'mobile' && toViewport === 'tablet') { | |
| const widthPx = parseFloat(newDimensions.width); | |
| const maxWidthPx = parseFloat(newDimensions.maxWidth); | |
| const expected90vw = toSize.width * 0.9; | |
| expect(widthPx).toBeLessThanOrEqual(expected90vw + 2); | |
| expect(maxWidthPx).toBeGreaterThanOrEqual(1200); | |
| } else if (fromViewport === 'tablet' && toViewport === 'mobile') { | |
| const widthPx = parseFloat(newDimensions.width); | |
| const maxWidthPx = parseFloat(newDimensions.maxWidth); | |
| expect(widthPx).toBeGreaterThan(toSize.width - 60); | |
| expect(maxWidthPx).toBeLessThan(1200); | |
| } else { | |
| throw new Error( | |
| `No resize assertion configured for transition: ${fromViewport} -> ${toViewport}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 183 - 194,
The helper testModalResponsiveResize only asserts behavior for two specific
transitions and silently returns for all other fromViewport/toViewport pairs;
update the function to cover every viewport transition (use the existing
variables fromViewport, toViewport, newDimensions, toSize) by adding assertions
for the remaining combinations or a final default branch that validates expected
width/maxWidth constraints (or throws/fails the test) so no transition passes
without validation; implement this as a switch or expanded if/else covering all
viewport pairs to ensure each path asserts expected dimensions.
… management - Updated .gitignore to include worker-specific test fixtures. - Modified e2e-tests.yml to implement test sharding for improved CI performance. - Refactored global setup to authenticate once and save session state for reuse across tests. - Introduced worker-isolated fixture paths to prevent conflicts during parallel test execution. - Improved test navigation and loading handling for better reliability. - Updated various test files to utilize new auth state management and fixture paths.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (3)
apps/ui/tests/context/mobile-context-view.spec.ts (1)
264-267:⚠️ Potential issue | 🟡 MinorWidth assertion on Line 267 is too permissive for the stated intent.
toBeGreaterThan(100)won’t catch many layout regressions and contradicts the comment’s desktop-width comparison.Suggested tightening
- // On mobile, the file list should be wider than the desktop fixed width (w-64 = 256px). - // When the sidebar is present (e.g. collapsed), the main content area can be narrower than - // full viewport, so we assert it's at least wider than a narrow strip (> 100px). - expect(fileListBox.width).toBeGreaterThan(100); + // Keep tolerance, but still catch regressions below expected sidebar-scale width. + expect(fileListBox.width).toBeGreaterThan(240);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/context/mobile-context-view.spec.ts` around lines 264 - 267, The test's width assertion is too lax; update the assertion on fileListBox.width in mobile-context-view.spec (the expect(fileListBox.width) check) to assert it's wider than the desktop fixed width (w-64 = 256px) instead of just >100, e.g. use toBeGreaterThan(256) (or toBeGreaterThanOrEqual(257) if you prefer strictness) so the test will catch layout regressions that violate the intended mobile-vs-desktop comparison.apps/server/src/services/dev-server-service.ts (1)
1166-1173:⚠️ Potential issue | 🟠 MajorReattached entries can still remain stale indefinitely.
Line 1167 and the equivalent checks in Line 1205, Line 1220, and Line 1253 only prune when
server.processexists and has exited. Rehydrated entries useprocess: null, so if that external process later dies, these paths never prune it and stale servers keep showing as running.Also applies to: 1205-1208, 1220-1223, 1253-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 1166 - 1173, The pruning only runs when server.process exists and has an exitCode, so rehydrated entries with server.process === null never get removed; update the pruning logic (the checks around server.process and the stalePaths.push(worktreePath) in dev-server-service.ts) to also handle rehydrated entries by detecting whether the external process for that entry still exists (e.g., if the server object stores a pid or similar identifier, attempt a non-destructive existence check like process.kill(pid, 0) or equivalent) and mark the path stale when that check fails; apply the same change to all equivalent blocks that currently only check typeof server.process.exitCode === 'number' so rehydrated entries are pruned too.apps/server/src/routes/worktree/routes/list.ts (1)
176-189:⚠️ Potential issue | 🟡 MinorNormalize detached refs consistently (
origin/still leaks through).
normalizeBranchFromHeadRef()does not strip plainorigin/, and Line 291 stores rawrev-parse --abbrev-ref HEADoutput. This can yield branch keys likeorigin/feature/x, which won’t match metadata/PR maps keyed by local branch names.Suggested fix
function normalizeBranchFromHeadRef(headRef: string): string | null { let normalized = headRef.trim(); - const prefixes = ['refs/heads/', 'refs/remotes/origin/', 'refs/remotes/', 'refs/']; + const prefixes = ['refs/heads/', 'refs/remotes/origin/', 'refs/remotes/', 'refs/', 'origin/']; for (const prefix of prefixes) { if (normalized.startsWith(prefix)) { normalized = normalized.slice(prefix.length); break; @@ const ref = headRef.trim(); - if (ref && ref !== 'HEAD') { - headBranch = ref; + const normalizedRef = normalizeBranchFromHeadRef(ref); + if (normalizedRef && normalizedRef !== 'HEAD') { + headBranch = normalizedRef; }Also applies to: 285-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/list.ts` around lines 176 - 189, normalizeBranchFromHeadRef currently misses stripping plain "origin/" (so values like "origin/feature/x" leak through); add "origin/" to the prefixes array (and consider "remotes/origin/" if used elsewhere) so the function removes that prefix, and ensure any place that stores the raw rev-parse output (the code path that captures `rev-parse --abbrev-ref HEAD`) passes that value through normalizeBranchFromHeadRef before using it as a branch key so local branch names match metadata/PR maps.
🧹 Nitpick comments (14)
.github/workflows/e2e-tests.yml (1)
182-189: Consider adding a merge job to combine shard reports.The blob reports are uploaded per-shard but there's no subsequent job to merge them into a unified HTML report. Without a merge step, these artifacts are uploaded but unused.
If full report merging is desired, add a dependent job that downloads all blob reports and runs
npx playwright merge-reports:merge-reports: if: always() needs: e2e runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 - name: Download blob reports uses: actions/download-artifact@v4 with: pattern: blob-report-shard-* path: all-blob-reports merge-multiple: true - name: Merge reports run: npx playwright merge-reports --reporter html ./all-blob-reports - name: Upload merged report uses: actions/upload-artifact@v4 with: name: playwright-report-merged path: playwright-report/If per-shard reports are sufficient for now, this can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-tests.yml around lines 182 - 189, Add a dependent GitHub Actions job that runs after the existing e2e shard uploads to download all per-shard artifacts and merge them using Playwright; specifically create a new job (e.g., merge-reports) that has needs: e2e, uses actions/download-artifact@v4 to pull artifacts named blob-report-shard-* into a single folder (merge-multiple: true), runs the merge command npx playwright merge-reports --reporter html on that folder, and then uploads the resulting unified report (e.g., playwright-report/) with actions/upload-artifact@v4; ensure the job is conditioned with if: always() so it runs even on failures.apps/ui/tests/utils/views/agent.ts (2)
85-93: Timeout semantics may exceed caller expectations.Each
waitForcall has an independent timeout of up totimeoutms. In the worst case, the function could take up to 2× the specified timeout (e.g., 20 seconds when 10 seconds is passed). If strict timeout adherence is important, consider tracking elapsed time and adjusting the second wait accordingly.♻️ Suggested approach using elapsed time
export async function waitForNewSession(page: Page, options?: { timeout?: number }): Promise<void> { const timeout = options?.timeout ?? 10000; + const start = Date.now(); // Ensure session list container is visible first (handles sidebar render delay) const sessionList = page.locator('[data-testid="session-list"]'); await sessionList.waitFor({ state: 'visible', timeout }); + const remaining = Math.max(0, timeout - (Date.now() - start)); // Wait for a session item to appear within the session list const sessionItem = sessionList.locator('[data-testid^="session-item-"]').first(); - await sessionItem.waitFor({ state: 'visible', timeout }); + await sessionItem.waitFor({ state: 'visible', timeout: remaining }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/views/agent.ts` around lines 85 - 93, The two independent waitFor calls (sessionList.waitFor and sessionItem.waitFor) each use the full options?.timeout value, allowing the function to take up to 2× timeout; change this by measuring elapsed time before the first wait (e.g., start = Date.now()), then compute a remaining timeout for the second wait as Math.max(0, timeout - (Date.now() - start)) and pass that to sessionItem.waitFor, and if remaining is 0 immediately fail/throw or return a deterministic timeout error to ensure the overall operation does not exceed the caller's specified timeout.
24-26: Consider reusing the existinggetSessionListhelper.The
getSessionListfunction (lines 7-9) returns the same locator. Reusing it would reduce duplication and improve maintainability.♻️ Suggested refactor
// Ensure session list (and thus SessionManager) is visible before clicking - const sessionList = page.locator('[data-testid="session-list"]'); + const sessionList = await getSessionList(page); await sessionList.waitFor({ state: 'visible', timeout: 10000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/views/agent.ts` around lines 24 - 26, Replace the duplicate locator in the setup with the existing helper by calling getSessionList() instead of creating a new locator; specifically, remove the manual const sessionList = page.locator('[data-testid="session-list"]') and use the locator returned by getSessionList() (and then await sessionList.waitFor({ state: 'visible', timeout: 10000 }) as before) so the test reuses the shared helper and avoids duplication.apps/server/src/routes/fs/routes/read.ts (1)
21-27: Redundantpath.basename()call.
basenameis already computed on line 16; reuse it instead of computingnameagain on line 22.♻️ Proposed fix
// Context and memory files may not exist yet during create/delete or test races if (filePath.includes('.automaker/context/') || filePath.includes('.automaker/memory/')) { - const name = path.basename(filePath); - const lower = name.toLowerCase(); + const lower = basename.toLowerCase(); if (lower.endsWith('.md') || lower.endsWith('.txt') || lower.endsWith('.markdown')) { return true; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/fs/routes/read.ts` around lines 21 - 27, The code redundantly calls path.basename(filePath) again; reuse the already-computed basename (the variable declared earlier) inside the conditional instead of creating const name = path.basename(filePath). Replace the second basename call with the existing basename variable (e.g., use basename.toLowerCase() for the lower check), remove the redundant const name declaration, and keep the rest of the logic (filePath checks and the lower.endsWith(...) return) unchanged.apps/ui/src/components/views/context-view.tsx (1)
242-252: Potential race condition when rapidly switching files.When
loadFileContentcompletes, it unconditionally overwritesselectedFileandeditedContentusing the capturedfilereference. If the user selects another file before the async read completes, stale content may briefly flash in the UI.Consider using a functional state update in
loadFileContentto guard against this:♻️ Proposed fix in loadFileContent
const loadFileContent = useCallback(async (file: ContextFile) => { try { const api = getElectronAPI(); const result = await api.readFile(file.path); if (result.success && result.content !== undefined) { - setEditedContent(result.content); - setSelectedFile({ ...file, content: result.content }); - setHasChanges(false); + // Only update if this file is still selected + setSelectedFile((current) => { + if (current?.path === file.path) { + setEditedContent(result.content); + setHasChanges(false); + return { ...file, content: result.content }; + } + return current; + }); } } catch (error) { logger.error('Failed to load file content:', error); } }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/context-view.tsx` around lines 242 - 252, The handler handleSelectFile sets selectedFile and then calls async loadFileContent which currently applies results unconditionally, causing stale content to overwrite newer selections if the user switches files quickly; modify loadFileContent to verify the component's current selectedFile (or use a functional state setter) before calling setSelectedFile/setEditedContent — e.g., capture an identifier (file.name or id) when starting the load and only apply the loaded content if it matches the current selectedFile (or use setSelectedFile(prev => prev?.id === id ? updated : prev)) so only the latest selection updates state.apps/ui/tests/utils/components/responsive-modal.ts (2)
122-133: Desktop validation now present, but height checks seem misplaced.The desktop branch correctly validates width constraints (addressing previous feedback). However, checking
modalHeightwithintestModalWidthAcrossViewportsis semantically inconsistent with the function name. Consider either renaming the function or moving height assertions totestModalHeightAcrossViewports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 122 - 133, The desktop branch in testModalWidthAcrossViewports includes modalHeight assertions that belong to height testing; either remove the modalHeight checks from testModalWidthAcrossViewports and add the corresponding assertions (expect(modalHeight)... using getModalHeight and expectedMaxHeight calculation) into testModalHeightAcrossViewports, or rename testModalWidthAcrossViewports to reflect it validates both dimensions; update references to getModalHeight, modalHeight, expectedMaxHeight, and the viewport branch logic accordingly so width and height assertions live in the appropriately named test.
137-168: Desktop height assertions are handled elsewhere.The desktop height checks were added to
testModalWidthAcrossViewportsrather than here. While functional, this asymmetry could be confusing for maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 137 - 168, The height assertions for desktop viewports are missing from testModalHeightAcrossViewports (they were added in testModalWidthAcrossViewports), so update testModalHeightAcrossViewports to include desktop height checks: after computing modalHeight and viewportHeight (using VIEWPORTS and getModalHeight), add branches for desktop viewports (e.g., 'desktop' and 'desktopLarge') that assert the modal height meets the same desktop constraints used in testModalWidthAcrossViewports; keep the same waitForTimeout(TIMEOUTS.animation) and viewport sizing logic to ensure parity between height and width tests.apps/ui/src/components/views/graph-view-page.tsx (1)
243-291: Consider extractinggraphWorktreeSelectorinto a dedicated hook/component.This block is doing rendering + state wiring + mutation callbacks in one place; splitting it out would reduce
GraphViewPagecomplexity and improve testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/graph-view-page.tsx` around lines 243 - 291, Extract the large useMemo JSX block named graphWorktreeSelector into a separate component (e.g., GraphWorktreeSelector) or a custom hook that returns the rendered element; move all rendering and local wiring (WorktreeMobileDropdown, BranchSwitchDropdown) into that new component and pass in the required props: graphWorktrees, isWorktreeSelected, branchCardCounts, selectedGraphWorktree, branches, filteredBranches, branchFilter, isLoadingBranches plus callbacks handleGraphSelectWorktree and handleGraphSwitchBranch and the status switchBranchMutation.isPending; keep GraphViewPage responsible for high-level state and mutations only and replace the useMemo declaration with a simple instantiation of the new component (or call to the hook) to restore readability and testability.apps/server/src/routes/worktree/routes/list-branches.ts (1)
133-164: Migrate remainingexecFileAsynccalls toexecGitCommandfor consistency.The file uses
execGitCommandfor most Git operations (lines 39, 46, 70, 76, 117) but still usesexecFileAsyncfor upstream/tracking checks (lines 133–137, 147–151, 160–164). For thetimeouton line 163, useAbortControllerinstead:Example pattern
const controller = new AbortController(); const timerId = setTimeout(() => controller.abort(), 5000); try { const output = await execGitCommand(['ls-remote', '--heads', 'origin', currentBranch], worktreePath, undefined, controller); } finally { clearTimeout(timerId); }This migration removes the
execFile/execFileAsyncimports and ensures consistent error handling and sandbox compatibility across all Git operations in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/list-branches.ts` around lines 133 - 164, Replace the remaining execFileAsync calls in the upstream/tracking check with execGitCommand: call execGitCommand(['rev-parse','--abbrev-ref', `${currentBranch}@{upstream}`], worktreePath) and use its stdout (trim) to set upstreamRef/hasRemoteBranch/trackingRemote, then call execGitCommand(['rev-list','--left-right','--count', `${currentBranch}@{upstream}...HEAD`], worktreePath) and parse its stdout into behindCount/aheadCount preserving the same fallback behavior; for the ls-remote call use an AbortController to implement the 5s timeout (create controller, setTimeout to controller.abort(), pass controller to execGitCommand, and clearTimeout in finally), and remove the now-unused execFileAsync import while keeping existing try/catch flow and variable names (currentBranch, worktreePath, aheadCount, behindCount, hasRemoteBranch, trackingRemote).apps/ui/tests/projects/new-project-creation.spec.ts (1)
97-131: Consider extractingfsJsonhelper to shared test utilities.The
fsJsonhelper is a clean abstraction for creating mock responses. Since multiple test files may need similar filesystem mocking for CI compatibility, consider extracting this to a shared test utility (e.g.,apps/ui/tests/utils/mock-helpers.ts) for reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/new-project-creation.spec.ts` around lines 97 - 131, Extract the inline fsJson helper from new-project-creation.spec.ts into a shared test utility (e.g., a mock-helpers module) that exports fsJson and any common route handler helpers (e.g., prebuilt handlers for /**/api/fs/exists, /stat, /mkdir, /write); then update new-project-creation.spec.ts to import fsJson (and/or the prebuilt handlers) and remove the local definition so tests reuse the shared helper across the suite.apps/server/src/routes/worktree/routes/list.ts (1)
662-677: Avoid hardcoding'main'in the ENOENT fallback payload.When git is unavailable, returning
branch: 'main'can mislabel non-main repos and pollute branch-keyed UI state. Prefer an explicit unknown sentinel value.Suggested fix
{ path: mainPath, - branch: 'main', + branch: '(git-unavailable)', isMain: true, isCurrent: true, hasWorktree: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/list.ts` around lines 662 - 677, The ENOENT fallback in the isSpawnENOENT branch is hardcoding branch: 'main' which can mislabel repos; update the returned worktree payload in the handler (the block that builds mainPath from (req.body as { projectPath?: string }) and calls res.json with worktrees) to use an explicit unknown sentinel (e.g., branch: 'unknown' or null) instead of 'main' so branch-keyed UI state isn't polluted; keep the rest of the fields (isMain, isCurrent, hasWorktree) intact.apps/ui/tests/utils/views/memory.ts (1)
208-210: Consider replacing fixed timeout with a condition-based wait.The 150ms
waitForTimeoutis a minor anti-pattern that could cause flakiness if the refresh takes longer under load. If callers always follow up with assertions (e.g.,waitForMemoryFile), this is likely fine in practice.💡 Optional: Wait for list refresh indicator
If a loading indicator exists during refresh, wait for it instead:
// Allow list to re-fetch - await page.waitForTimeout(150); + // Wait for any loading indicator to appear and disappear, or brief network settle + const listLoading = page.locator('[data-testid="memory-file-list-loading"]'); + await listLoading.waitFor({ state: 'hidden', timeout: 2000 }).catch(() => {});If no such indicator exists, the current approach is acceptable given the short duration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/views/memory.ts` around lines 208 - 210, Replace the fixed 150ms sleep (page.waitForTimeout(150)) with a condition-based wait: detect a known refresh completion signal (e.g., wait for a loading indicator to disappear via page.waitForSelector(..., { state: "detached" | "hidden" }) or wait for the expected DOM change using page.waitForFunction or for a specific element used by waitForMemoryFile to appear). Update the function containing page.waitForTimeout (the memory list refresh helper) to try the loading-indicator check first and fall back to a short timeout only if no indicator exists, keeping callers like waitForMemoryFile unaffected.apps/ui/tests/global-setup.ts (1)
12-16: Consider reusing shared test URL constants to avoid drift.
API_BASE_URLandWEB_BASE_URLduplicate logic already present inapps/ui/tests/utils/core/constants.ts(Line 13-Line 15 and Line 21-Line 23). Reusing one source would keep defaults/env handling aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/global-setup.ts` around lines 12 - 16, Remove the duplicated local URL constants and reuse the shared exports instead: import API_BASE_URL and WEB_BASE_URL (and, if available, TEST_PORT/TEST_SERVER_PORT) from the shared constants module and use those values in global-setup.ts rather than re-declaring TEST_PORT/TEST_SERVER_PORT/API_BASE_URL/WEB_BASE_URL; if the shared module doesn't expose TEST_PORT/TEST_SERVER_PORT, call through to its exported API_BASE_URL/WEB_BASE_URL so all env-default logic is centralized and reuseServer remains unchanged.apps/ui/tests/projects/board-background-persistence.spec.ts (1)
65-103: Consider extracting duplicated.automakerfixture setup into a helper.Both tests repeat near-identical project bootstrap logic; pulling this into one helper will reduce drift and simplify future updates.
♻️ Possible refactor
+function setupProjectAutomakerFixture( + projectPath: string, + options?: { withBackground?: boolean; cardOpacity?: number; columnOpacity?: number } +): { settingsPath: string; backgroundPath?: string } { + const automakerDir = path.join(projectPath, '.automaker'); + fs.mkdirSync(path.join(automakerDir, 'features'), { recursive: true }); + fs.mkdirSync(path.join(automakerDir, 'context'), { recursive: true }); + fs.mkdirSync(path.join(automakerDir, 'board'), { recursive: true }); + + let backgroundPath: string | undefined; + const settings: Record<string, unknown> = { version: 1 }; + if (options?.withBackground) { + backgroundPath = path.join(automakerDir, 'board', 'background.jpg'); + fs.copyFileSync(path.join(__dirname, '..', 'img', 'background.jpg'), backgroundPath); + settings.boardBackground = { + imagePath: backgroundPath, + cardOpacity: options.cardOpacity ?? 85, + columnOpacity: options.columnOpacity ?? 60, + imageVersion: Date.now(), + }; + } + + const settingsPath = path.join(automakerDir, 'settings.json'); + fs.writeFileSync(settingsPath, JSON.stringify(settings, null, 2)); + return { settingsPath, backgroundPath }; +}Also applies to: 95-103, 437-465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 65 - 103, Extract the duplicated .automaker setup into a reusable helper (e.g., createAutomakerFixture or setupAutomakerDir) that accepts the project path and an options object (flags for includeBoardImage, imageSourcePath, boardSettings overrides, and minimalOnly). Replace the inline blocks that construct automakerDirA/automakerDirB, create subfolders ('board', 'features', 'context'), copy the test image, and write settings.json (variables: automakerDirA, automakerDirB, backgroundPath, settingsPath, backgroundSettings) with calls to this helper so tests call the helper with includeBoardImage: true plus board settings for project A and includeBoardImage: false/minimal for project B; ensure the helper centralizes JSON structure (version and boardBackground) and file writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 241-244: The type-guard that currently only checks typeof
e.startedAt === 'string' must also verify the string is a parseable date (e.g.,
parse with new Date(e.startedAt) or Date.parse and ensure
!isNaN(date.getTime()/Date.parse result)) before accepting it, and the
rehydration code that does new Date(e.startedAt).toISOString() should first
create the Date, validate it is valid, and handle invalid values (reject entry
or provide fallback) to avoid throwing on toISOString(); update both the
type-check block (the guard around e.startedAt) and the rehydration/rehydrate
logic that constructs new Date(e.startedAt) to perform this validation.
- Around line 995-1001: The event payload for "dev-server:started" is using the
original local variable port which can be stale if output parsing later updated
serverInfo.port; update the emit payload to read the authoritative port from
serverInfo (use serverInfo.port) and include
serverInfo.url/worktreePath/timestamp as before so listeners receive the current
port value; change the emit in the block that references
this.emitter.emit('dev-server:started', ...) to source port from serverInfo.port
instead of the local port variable.
In `@apps/ui/playwright.config.ts`:
- Around line 42-44: The comment and the workers setting are inconsistent: the
Playwright config property workers currently uses a hardcoded number (workers:
process.env.CI ? 2 : 5) but the comment claims "local gets 50% of CPUs." Fix by
either updating the comment to reflect the hardcoded local value (e.g., "local
uses 5 workers") or change the workers value to use Playwright's percentage
syntax for dynamic scaling (workers: process.env.CI ? 2 : '50%'); ensure the
conditional on process.env.CI and the identifier workers remain intact.
In `@apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx`:
- Around line 223-231: selectedBranch may be a full ref (e.g.
refs/remotes/origin/main), so before calling api.worktree.pull normalize it into
a short branch name: check for prefixes in order and strip them (e.g. if
selectedBranch startsWith `refs/remotes/${selectedRemote}/` remove that prefix;
else if startsWith `${selectedRemote}/` remove that; else if startsWith
`refs/heads/` remove that; else if startsWith `refs/` remove `refs/`), assign
the result to remoteBranchShortName and pass that to api.worktree.pull (use the
existing variables selectedBranch, selectedRemote, remoteBranchShortName and the
call to api.worktree.pull(worktree.path, selectedRemote, true,
remoteBranchShortName)).
In `@apps/ui/tests/global-setup.ts`:
- Around line 54-65: The current health-check loop using start and
AbortSignal.timeout silently gives up after 30s; change it to fail-fast by
detecting that the loop exited without a successful health.ok and throwing an
error. Concretely, introduce a boolean (e.g., let healthy = false) set to true
when health.ok is observed inside the try block (where you call fetch with
AbortSignal.timeout), and after the while loop check healthy and if false throw
new Error(`Backend health check timed out after 30s for ${API_BASE_URL}`) so
setup fails immediately rather than continuing silently.
- Around line 87-117: The current flow persists storage state even when login
verification fails; before calling context.storageState({ path: AUTH_STATE_PATH
}) bail out if authentication did not succeed. Concretely, after checking
response and the statusJson (the existing response, statusJson and authenticated
check), throw an error or exit (e.g., throw new Error('Auth verification
failed')) when !statusJson?.authenticated or when response?.success/token are
missing so you never call context.storageState; keep the context.addCookies and
verification logic as-is but ensure the guard prevents writing AUTH_STATE_PATH
on auth failure.
In `@apps/ui/tests/memory/mobile-memory-view.spec.ts`:
- Line 43: The hardcoded 3000ms in the waitForSelector call for
'[data-testid="create-memory-dialog"]' is brittle; replace the explicit {
timeout: 3000 } on the page.waitForSelector(...) call with a more robust option
— either remove the timeout to use Playwright's default, increase it to a higher
value (e.g. 10000ms), or reference the existing shared test timeout constant
used elsewhere in the create/select flow so the dialog wait matches other tests
and avoids CI flakes.
In `@apps/ui/tests/projects/new-project-creation.spec.ts`:
- Around line 102-108: The mock for the '/api/fs/exists' route in
new-project-creation.spec.ts currently returns exists: true for all paths;
change the page.route handler to parse the request postData (or JSON body) and
inspect the filePath parameter so it returns exists: false for new project paths
checked by project-init.ts (the project directory check around line 48, the
'.git' check around line 76, and subsequent required file/dir checks around
lines 109, 151, 183) while returning exists: true for the parent workspace
directory; update the handler in the route where route.request().method() ===
'POST' to conditionally fulfill fsJson(200, { success: true, exists: <boolean>
}) based on the requested filePath so the test exercises mkdir and file-creation
flows.
In `@apps/ui/tests/projects/open-existing-project.spec.ts`:
- Around line 82-85: The test's GET interception uses getCount that increments
on every request, which can advance past the intended injection when a GET lacks
a settings payload; change the logic so you only increment the "first-response"
flag when the response is actually eligible (i.e., the intercepted response
contains json.settings). Update the interceptor code that uses getCount and the
conditional that injects the fake project to check for existence of
json.settings before toggling/advancing the first-response behavior (referencing
getCount and json.settings), and make the same fix for the other similar block
around the code referenced at lines 104-106.
In `@apps/ui/tests/utils/project/fixtures.ts`:
- Around line 20-34: getWorkerFixturePath currently calls
fs.cpSync(FIXTURE_PATH, ...) without validating that FIXTURE_PATH exists,
causing ENOENT CI failures; before attempting fs.cpSync in getWorkerFixturePath,
check fs.existsSync(FIXTURE_PATH) and if it does not exist throw a clear Error
that explains the source fixture is missing and instructs running the setup
script (e.g., mention setup-e2e-fixtures.mjs) so the developer knows how to
create test/fixtures/projectA; keep the rest of the worker isolation logic and
assignment to _workerFixturePath unchanged.
In `@apps/ui/tests/utils/views/memory.ts`:
- Around line 265-275: The visibility check using sandboxAcceptBtn.isVisible({
timeout: 1000 }) is using a deprecated timeout and returns immediately; change
the logic to await sandboxAcceptBtn.waitFor({ state: 'visible', timeout: 1000 })
(or wrap that in try/catch) to properly wait up to 1s for the button, then click
if found; update the dialog-wait branch that currently uses
page.locator('[role="dialog"][data-state="open"]').first().waitFor(...) to
remain after the click, and keep the else branch calling
closeDialogWithEscape(page, { timeout: 2000 }) for the not-visible case. Ensure
any waitFor rejections are caught (e.g., with try/catch or .catch(() => false))
to preserve existing fallback behavior.
---
Duplicate comments:
In `@apps/server/src/routes/worktree/routes/list.ts`:
- Around line 176-189: normalizeBranchFromHeadRef currently misses stripping
plain "origin/" (so values like "origin/feature/x" leak through); add "origin/"
to the prefixes array (and consider "remotes/origin/" if used elsewhere) so the
function removes that prefix, and ensure any place that stores the raw rev-parse
output (the code path that captures `rev-parse --abbrev-ref HEAD`) passes that
value through normalizeBranchFromHeadRef before using it as a branch key so
local branch names match metadata/PR maps.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 1166-1173: The pruning only runs when server.process exists and
has an exitCode, so rehydrated entries with server.process === null never get
removed; update the pruning logic (the checks around server.process and the
stalePaths.push(worktreePath) in dev-server-service.ts) to also handle
rehydrated entries by detecting whether the external process for that entry
still exists (e.g., if the server object stores a pid or similar identifier,
attempt a non-destructive existence check like process.kill(pid, 0) or
equivalent) and mark the path stale when that check fails; apply the same change
to all equivalent blocks that currently only check typeof
server.process.exitCode === 'number' so rehydrated entries are pruned too.
In `@apps/ui/tests/context/mobile-context-view.spec.ts`:
- Around line 264-267: The test's width assertion is too lax; update the
assertion on fileListBox.width in mobile-context-view.spec (the
expect(fileListBox.width) check) to assert it's wider than the desktop fixed
width (w-64 = 256px) instead of just >100, e.g. use toBeGreaterThan(256) (or
toBeGreaterThanOrEqual(257) if you prefer strictness) so the test will catch
layout regressions that violate the intended mobile-vs-desktop comparison.
---
Nitpick comments:
In @.github/workflows/e2e-tests.yml:
- Around line 182-189: Add a dependent GitHub Actions job that runs after the
existing e2e shard uploads to download all per-shard artifacts and merge them
using Playwright; specifically create a new job (e.g., merge-reports) that has
needs: e2e, uses actions/download-artifact@v4 to pull artifacts named
blob-report-shard-* into a single folder (merge-multiple: true), runs the merge
command npx playwright merge-reports --reporter html on that folder, and then
uploads the resulting unified report (e.g., playwright-report/) with
actions/upload-artifact@v4; ensure the job is conditioned with if: always() so
it runs even on failures.
In `@apps/server/src/routes/fs/routes/read.ts`:
- Around line 21-27: The code redundantly calls path.basename(filePath) again;
reuse the already-computed basename (the variable declared earlier) inside the
conditional instead of creating const name = path.basename(filePath). Replace
the second basename call with the existing basename variable (e.g., use
basename.toLowerCase() for the lower check), remove the redundant const name
declaration, and keep the rest of the logic (filePath checks and the
lower.endsWith(...) return) unchanged.
In `@apps/server/src/routes/worktree/routes/list-branches.ts`:
- Around line 133-164: Replace the remaining execFileAsync calls in the
upstream/tracking check with execGitCommand: call
execGitCommand(['rev-parse','--abbrev-ref', `${currentBranch}@{upstream}`],
worktreePath) and use its stdout (trim) to set
upstreamRef/hasRemoteBranch/trackingRemote, then call
execGitCommand(['rev-list','--left-right','--count',
`${currentBranch}@{upstream}...HEAD`], worktreePath) and parse its stdout into
behindCount/aheadCount preserving the same fallback behavior; for the ls-remote
call use an AbortController to implement the 5s timeout (create controller,
setTimeout to controller.abort(), pass controller to execGitCommand, and
clearTimeout in finally), and remove the now-unused execFileAsync import while
keeping existing try/catch flow and variable names (currentBranch, worktreePath,
aheadCount, behindCount, hasRemoteBranch, trackingRemote).
In `@apps/server/src/routes/worktree/routes/list.ts`:
- Around line 662-677: The ENOENT fallback in the isSpawnENOENT branch is
hardcoding branch: 'main' which can mislabel repos; update the returned worktree
payload in the handler (the block that builds mainPath from (req.body as {
projectPath?: string }) and calls res.json with worktrees) to use an explicit
unknown sentinel (e.g., branch: 'unknown' or null) instead of 'main' so
branch-keyed UI state isn't polluted; keep the rest of the fields (isMain,
isCurrent, hasWorktree) intact.
In `@apps/ui/src/components/views/context-view.tsx`:
- Around line 242-252: The handler handleSelectFile sets selectedFile and then
calls async loadFileContent which currently applies results unconditionally,
causing stale content to overwrite newer selections if the user switches files
quickly; modify loadFileContent to verify the component's current selectedFile
(or use a functional state setter) before calling
setSelectedFile/setEditedContent — e.g., capture an identifier (file.name or id)
when starting the load and only apply the loaded content if it matches the
current selectedFile (or use setSelectedFile(prev => prev?.id === id ? updated :
prev)) so only the latest selection updates state.
In `@apps/ui/src/components/views/graph-view-page.tsx`:
- Around line 243-291: Extract the large useMemo JSX block named
graphWorktreeSelector into a separate component (e.g., GraphWorktreeSelector) or
a custom hook that returns the rendered element; move all rendering and local
wiring (WorktreeMobileDropdown, BranchSwitchDropdown) into that new component
and pass in the required props: graphWorktrees, isWorktreeSelected,
branchCardCounts, selectedGraphWorktree, branches, filteredBranches,
branchFilter, isLoadingBranches plus callbacks handleGraphSelectWorktree and
handleGraphSwitchBranch and the status switchBranchMutation.isPending; keep
GraphViewPage responsible for high-level state and mutations only and replace
the useMemo declaration with a simple instantiation of the new component (or
call to the hook) to restore readability and testability.
In `@apps/ui/tests/global-setup.ts`:
- Around line 12-16: Remove the duplicated local URL constants and reuse the
shared exports instead: import API_BASE_URL and WEB_BASE_URL (and, if available,
TEST_PORT/TEST_SERVER_PORT) from the shared constants module and use those
values in global-setup.ts rather than re-declaring
TEST_PORT/TEST_SERVER_PORT/API_BASE_URL/WEB_BASE_URL; if the shared module
doesn't expose TEST_PORT/TEST_SERVER_PORT, call through to its exported
API_BASE_URL/WEB_BASE_URL so all env-default logic is centralized and
reuseServer remains unchanged.
In `@apps/ui/tests/projects/board-background-persistence.spec.ts`:
- Around line 65-103: Extract the duplicated .automaker setup into a reusable
helper (e.g., createAutomakerFixture or setupAutomakerDir) that accepts the
project path and an options object (flags for includeBoardImage,
imageSourcePath, boardSettings overrides, and minimalOnly). Replace the inline
blocks that construct automakerDirA/automakerDirB, create subfolders ('board',
'features', 'context'), copy the test image, and write settings.json (variables:
automakerDirA, automakerDirB, backgroundPath, settingsPath, backgroundSettings)
with calls to this helper so tests call the helper with includeBoardImage: true
plus board settings for project A and includeBoardImage: false/minimal for
project B; ensure the helper centralizes JSON structure (version and
boardBackground) and file writes.
In `@apps/ui/tests/projects/new-project-creation.spec.ts`:
- Around line 97-131: Extract the inline fsJson helper from
new-project-creation.spec.ts into a shared test utility (e.g., a mock-helpers
module) that exports fsJson and any common route handler helpers (e.g., prebuilt
handlers for /**/api/fs/exists, /stat, /mkdir, /write); then update
new-project-creation.spec.ts to import fsJson (and/or the prebuilt handlers) and
remove the local definition so tests reuse the shared helper across the suite.
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 122-133: The desktop branch in testModalWidthAcrossViewports
includes modalHeight assertions that belong to height testing; either remove the
modalHeight checks from testModalWidthAcrossViewports and add the corresponding
assertions (expect(modalHeight)... using getModalHeight and expectedMaxHeight
calculation) into testModalHeightAcrossViewports, or rename
testModalWidthAcrossViewports to reflect it validates both dimensions; update
references to getModalHeight, modalHeight, expectedMaxHeight, and the viewport
branch logic accordingly so width and height assertions live in the
appropriately named test.
- Around line 137-168: The height assertions for desktop viewports are missing
from testModalHeightAcrossViewports (they were added in
testModalWidthAcrossViewports), so update testModalHeightAcrossViewports to
include desktop height checks: after computing modalHeight and viewportHeight
(using VIEWPORTS and getModalHeight), add branches for desktop viewports (e.g.,
'desktop' and 'desktopLarge') that assert the modal height meets the same
desktop constraints used in testModalWidthAcrossViewports; keep the same
waitForTimeout(TIMEOUTS.animation) and viewport sizing logic to ensure parity
between height and width tests.
In `@apps/ui/tests/utils/views/agent.ts`:
- Around line 85-93: The two independent waitFor calls (sessionList.waitFor and
sessionItem.waitFor) each use the full options?.timeout value, allowing the
function to take up to 2× timeout; change this by measuring elapsed time before
the first wait (e.g., start = Date.now()), then compute a remaining timeout for
the second wait as Math.max(0, timeout - (Date.now() - start)) and pass that to
sessionItem.waitFor, and if remaining is 0 immediately fail/throw or return a
deterministic timeout error to ensure the overall operation does not exceed the
caller's specified timeout.
- Around line 24-26: Replace the duplicate locator in the setup with the
existing helper by calling getSessionList() instead of creating a new locator;
specifically, remove the manual const sessionList =
page.locator('[data-testid="session-list"]') and use the locator returned by
getSessionList() (and then await sessionList.waitFor({ state: 'visible',
timeout: 10000 }) as before) so the test reuses the shared helper and avoids
duplication.
In `@apps/ui/tests/utils/views/memory.ts`:
- Around line 208-210: Replace the fixed 150ms sleep (page.waitForTimeout(150))
with a condition-based wait: detect a known refresh completion signal (e.g.,
wait for a loading indicator to disappear via page.waitForSelector(..., { state:
"detached" | "hidden" }) or wait for the expected DOM change using
page.waitForFunction or for a specific element used by waitForMemoryFile to
appear). Update the function containing page.waitForTimeout (the memory list
refresh helper) to try the loading-indicator check first and fall back to a
short timeout only if no indicator exists, keeping callers like
waitForMemoryFile unaffected.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
.github/workflows/e2e-tests.yml.gitignoreapps/server/src/routes/fs/routes/read.tsapps/server/src/routes/fs/routes/stat.tsapps/server/src/routes/worktree/routes/list-branches.tsapps/server/src/routes/worktree/routes/list.tsapps/server/src/routes/worktree/routes/pull.tsapps/server/src/services/dev-server-service.tsapps/server/src/services/pull-service.tsapps/ui/.gitignoreapps/ui/playwright.config.tsapps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsxapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/graph-view-page.tsxapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/types/electron.d.tsapps/ui/tests/agent/start-new-chat-session.spec.tsapps/ui/tests/context/context-file-management.spec.tsapps/ui/tests/context/delete-context-file.spec.tsapps/ui/tests/context/desktop-context-view.spec.tsapps/ui/tests/context/file-extension-edge-cases.spec.tsapps/ui/tests/context/mobile-context-operations.spec.tsapps/ui/tests/context/mobile-context-view.spec.tsapps/ui/tests/e2e-testing-guide.mdapps/ui/tests/features/edit-feature.spec.tsapps/ui/tests/features/opus-thinking-level-none.spec.tsapps/ui/tests/global-setup.tsapps/ui/tests/memory/file-extension-edge-cases.spec.tsapps/ui/tests/memory/mobile-memory-operations.spec.tsapps/ui/tests/memory/mobile-memory-view.spec.tsapps/ui/tests/projects/board-background-persistence.spec.tsapps/ui/tests/projects/new-project-creation.spec.tsapps/ui/tests/projects/open-existing-project.spec.tsapps/ui/tests/settings/settings-startup-sync-race.spec.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/utils/components/responsive-modal.tsapps/ui/tests/utils/core/interactions.tsapps/ui/tests/utils/navigation/views.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/views/agent.tsapps/ui/tests/utils/views/context.tsapps/ui/tests/utils/views/memory.tstest/agent-session-test-115699-vyk2nk2/test-project-1768743000887/package.jsontest/feature-backlog-test-114171-aysp86y/test-project-1768742910934/package.jsontest/feature-backlog-test-80497-5rxs746/test-project-1767820775187/package.jsontest/fixtures/projectA/.gitkeeptest/running-task-display-test-805-6c4ockc/test-project-1772088506096/README.mdtest/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.jstest/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.test.js
💤 Files with no reviewable changes (4)
- apps/ui/tests/context/mobile-context-operations.spec.ts
- apps/ui/tests/memory/mobile-memory-operations.spec.ts
- apps/ui/tests/context/file-extension-edge-cases.spec.ts
- apps/ui/tests/memory/file-extension-edge-cases.spec.ts
✅ Files skipped from review due to trivial changes (2)
- apps/ui/.gitignore
- apps/ui/tests/e2e-testing-guide.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/tests/utils/views/context.ts
- apps/ui/tests/context/desktop-context-view.spec.ts
- apps/ui/tests/features/edit-feature.spec.ts
Comment on lines
+241
to
+244
| typeof e.url === 'string' && | ||
| typeof e.startedAt === 'string' && | ||
| typeof e.urlDetected === 'boolean' && | ||
| (e.customCommand === undefined || typeof e.customCommand === 'string'); |
Contributor
There was a problem hiding this comment.
Validate startedAt as a parseable date before rehydration.
Line 242 only checks startedAt is a string. Invalid strings become Invalid Date at Line 263 and can later throw on toISOString() in list/info/log flows.
Suggested fix
typeof e.url === 'string' &&
- typeof e.startedAt === 'string' &&
+ typeof e.startedAt === 'string' &&
+ !Number.isNaN(Date.parse(e.startedAt)) &&
typeof e.urlDetected === 'boolean' &&Also applies to: 263-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/dev-server-service.ts` around lines 241 - 244, The
type-guard that currently only checks typeof e.startedAt === 'string' must also
verify the string is a parseable date (e.g., parse with new Date(e.startedAt) or
Date.parse and ensure !isNaN(date.getTime()/Date.parse result)) before accepting
it, and the rehydration code that does new Date(e.startedAt).toISOString()
should first create the Date, validate it is valid, and handle invalid values
(reject entry or provide fallback) to avoid throwing on toISOString(); update
both the type-check block (the guard around e.startedAt) and the
rehydration/rehydrate logic that constructs new Date(e.startedAt) to perform
this validation.
Comment on lines
+995
to
+1001
| if (this.emitter) { | ||
| this.emitter.emit('dev-server:started', { | ||
| worktreePath, | ||
| port, | ||
| url: serverInfo.url, | ||
| timestamp: new Date().toISOString(), | ||
| }); |
Contributor
There was a problem hiding this comment.
dev-server:started may emit the wrong port after early detection.
At Line 998, the payload uses the original port variable. If output parsing already updated serverInfo.port before this emit, clients get a stale port and can regress UI state.
Suggested fix
if (this.emitter) {
this.emitter.emit('dev-server:started', {
worktreePath,
- port,
+ port: serverInfo.port,
url: serverInfo.url,
timestamp: new Date().toISOString(),
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/dev-server-service.ts` around lines 995 - 1001, The
event payload for "dev-server:started" is using the original local variable port
which can be stale if output parsing later updated serverInfo.port; update the
emit payload to read the authoritative port from serverInfo (use
serverInfo.port) and include serverInfo.url/worktreePath/timestamp as before so
listeners receive the current port value; change the emit in the block that
references this.emitter.emit('dev-server:started', ...) to source port from
serverInfo.port instead of the local port variable.
apps/ui/playwright.config.ts
Outdated
Comment on lines
+42
to
+44
| // Use multiple workers for parallelism. CI gets 2 workers (constrained resources), | ||
| // local gets 50% of CPUs. Each worker gets its own browser context with shared auth state. | ||
| workers: process.env.CI ? 2 : 5, |
Contributor
There was a problem hiding this comment.
Comment does not match the code.
The comment states "local gets 50% of CPUs" but the code uses a hardcoded value of 5. Either update the comment to reflect the actual behavior, or use Playwright's percentage syntax (e.g., '50%') if dynamic scaling is intended.
🔧 Proposed fix to align comment with code
- // Use multiple workers for parallelism. CI gets 2 workers (constrained resources),
- // local gets 50% of CPUs. Each worker gets its own browser context with shared auth state.
+ // Use multiple workers for parallelism. CI gets 2 workers (constrained resources),
+ // local gets 5 workers. Each worker gets its own browser context with shared auth state.
workers: process.env.CI ? 2 : 5,Or, to actually use 50% of CPUs:
- workers: process.env.CI ? 2 : 5,
+ workers: process.env.CI ? 2 : '50%',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Use multiple workers for parallelism. CI gets 2 workers (constrained resources), | |
| // local gets 50% of CPUs. Each worker gets its own browser context with shared auth state. | |
| workers: process.env.CI ? 2 : 5, | |
| // Use multiple workers for parallelism. CI gets 2 workers (constrained resources), | |
| // local gets 5 workers. Each worker gets its own browser context with shared auth state. | |
| workers: process.env.CI ? 2 : 5, |
Suggested change
| // Use multiple workers for parallelism. CI gets 2 workers (constrained resources), | |
| // local gets 50% of CPUs. Each worker gets its own browser context with shared auth state. | |
| workers: process.env.CI ? 2 : 5, | |
| workers: process.env.CI ? 2 : '50%', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/playwright.config.ts` around lines 42 - 44, The comment and the
workers setting are inconsistent: the Playwright config property workers
currently uses a hardcoded number (workers: process.env.CI ? 2 : 5) but the
comment claims "local gets 50% of CPUs." Fix by either updating the comment to
reflect the hardcoded local value (e.g., "local uses 5 workers") or change the
workers value to use Playwright's percentage syntax for dynamic scaling
(workers: process.env.CI ? 2 : '50%'); ensure the conditional on process.env.CI
and the identifier workers remain intact.
apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx
Outdated
Show resolved
Hide resolved
| await clickElement(page, 'create-memory-button-mobile'); | ||
| await page.waitForSelector('[data-testid="create-memory-dialog"]', { timeout: 5000 }); | ||
|
|
||
| await page.waitForSelector('[data-testid="create-memory-dialog"]', { timeout: 3000 }); |
Contributor
There was a problem hiding this comment.
Avoid a brittle 3s dialog timeout.
This timeout is shorter than the rest of the create/select flow and can cause CI flakes under load.
⏱️ Suggested fix
- await page.waitForSelector('[data-testid="create-memory-dialog"]', { timeout: 3000 });
+ await page.waitForSelector('[data-testid="create-memory-dialog"]', { timeout: 10000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await page.waitForSelector('[data-testid="create-memory-dialog"]', { timeout: 3000 }); | |
| await page.waitForSelector('[data-testid="create-memory-dialog"]', { timeout: 10000 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/memory/mobile-memory-view.spec.ts` at line 43, The hardcoded
3000ms in the waitForSelector call for '[data-testid="create-memory-dialog"]' is
brittle; replace the explicit { timeout: 3000 } on the page.waitForSelector(...)
call with a more robust option — either remove the timeout to use Playwright's
default, increase it to a higher value (e.g. 10000ms), or reference the existing
shared test timeout constant used elsewhere in the create/select flow so the
dialog wait matches other tests and avoids CI flakes.
Comment on lines
+102
to
+108
| await page.route('**/api/fs/exists', async (route) => { | ||
| if (route.request().method() === 'POST') { | ||
| await route.fulfill(fsJson(200, { success: true, exists: true })); | ||
| } else { | ||
| await route.continue(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for fs/exists route handler and usage patterns
rg -n -C5 "fs/exists" --type=ts -g '!**/node_modules/**'Repository: AutoMaker-Org/automaker
Length of output: 3809
🏁 Script executed:
#!/bin/bash
# Search for server-side fs/exists endpoint implementation
rg -n "/api/fs/exists" --type=ts -g '!**/node_modules/**' apps/server/
# Also search for the route handler pattern
rg -n "fs/exists|routes.*fs" --type=ts apps/server/src/routes -A 5Repository: AutoMaker-Org/automaker
Length of output: 1240
🏁 Script executed:
#!/bin/bash
# Search more broadly for fs endpoint
rg -n "api/fs|fs.*route" --type=ts apps/server/src -A 3
# Check for fs routes files
find apps/server/src -name "*fs*" -type f
# Search in routes directory structure
ls -la apps/server/src/routes/Repository: AutoMaker-Org/automaker
Length of output: 3494
🏁 Script executed:
#!/bin/bash
# List fs routes
ls -la apps/server/src/routes/fs/
# Check the fs routes index
cat -n apps/server/src/routes/fs/index.ts
# Search for exists handler
find apps/server/src/routes/fs -name "*.ts" -exec basename {} \;Repository: AutoMaker-Org/automaker
Length of output: 3412
🏁 Script executed:
#!/bin/bash
# Read the exists handler
cat -n apps/server/src/routes/fs/routes/exists.ts
# Also search for project creation related routes
rg -n "create.*project|new.*project" --type=ts apps/server/src/routes -A 5Repository: AutoMaker-Org/automaker
Length of output: 13724
🏁 Script executed:
#!/bin/bash
# Search for project creation endpoint
rg -n "project.*create|create.*project" --type=ts apps/server/src/routes/projects -A 5
# Also look at the test file more carefully
cat -n apps/ui/tests/projects/new-project-creation.spec.ts | head -150Repository: AutoMaker-Org/automaker
Length of output: 6543
🏁 Script executed:
#!/bin/bash
# Search for new project creation flow in UI
rg -n "create.*project|new.*project" --type=ts apps/ui/src -A 3 | head -100
# Search for where exists() is called
rg -n "\.exists\(" --type=ts apps/ui/src -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 13155
🏁 Script executed:
#!/bin/bash
# Read the full project-init.ts to understand the flow
cat -n apps/ui/src/lib/project-init.ts | head -80Repository: AutoMaker-Org/automaker
Length of output: 3067
🏁 Script executed:
#!/bin/bash
# Check if there are other tests that mock /api/fs/exists to see patterns
rg -n "api/fs/exists" --type=ts apps/ui/tests -B 5 -A 3 | head -80Repository: AutoMaker-Org/automaker
Length of output: 1948
Fix the /api/fs/exists mock to return exists: false for paths that don't yet exist in a new project.
The mock currently returns exists: true for all paths, which bypasses the actual creation logic. In a new project flow, api.exists() is called in project-init.ts to check if:
- The project directory exists (line 48) — should be
falseinitially for new projects - The
.gitdirectory exists (line 76) — should befalseuntil created - Required files and directories exist (lines 109, 151, 183) — should be
falseuntil created
Returning true for all requests causes the code to skip mkdir and file creation operations, defeating the test's purpose. Consider a conditional response based on the filePath parameter (similar to the pattern in board-background-persistence.spec.ts), returning false for new project paths and true for the parent workspace directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/projects/new-project-creation.spec.ts` around lines 102 - 108,
The mock for the '/api/fs/exists' route in new-project-creation.spec.ts
currently returns exists: true for all paths; change the page.route handler to
parse the request postData (or JSON body) and inspect the filePath parameter so
it returns exists: false for new project paths checked by project-init.ts (the
project directory check around line 48, the '.git' check around line 76, and
subsequent required file/dir checks around lines 109, 151, 183) while returning
exists: true for the parent workspace directory; update the handler in the route
where route.request().method() === 'POST' to conditionally fulfill fsJson(200, {
success: true, exists: <boolean> }) based on the requested filePath so the test
exercises mkdir and file-creation flows.
Comment on lines
+82
to
+85
| // Intercept settings API: only modify the FIRST GET so we start with no current project | ||
| // but our test project in the list. Subsequent GETs pass through so background refetch | ||
| // doesn't overwrite the store after we open the project (which would show "No project selected"). | ||
| let getCount = 0; |
Contributor
There was a problem hiding this comment.
Make first-response mutation eligibility-based, not call-order-based
getCount currently advances for every GET, even when json.settings is missing. That can skip injection entirely if an earlier GET hits first, and the test may still pass via fallback, masking regressions. Gate on the first eligible settings payload instead.
Suggested patch
- let getCount = 0;
+ let injectedSettings = false;
await page.route('**/api/settings/global', async (route) => {
if (route.request().method() !== 'GET') {
return route.continue();
}
@@
- getCount += 1;
- if (getCount === 1 && json.settings) {
+ if (!injectedSettings && json?.settings) {
json.settings.currentProjectId = null;
const testProject = {
id: projectId,
name: projectName,
path: projectPath,
lastOpened: new Date(Date.now() - 86400000).toISOString(),
};
@@
if (!hasProject) {
json.settings.projects = [testProject, ...existingProjects];
}
+ injectedSettings = true;
}Also applies to: 104-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/projects/open-existing-project.spec.ts` around lines 82 - 85,
The test's GET interception uses getCount that increments on every request,
which can advance past the intended injection when a GET lacks a settings
payload; change the logic so you only increment the "first-response" flag when
the response is actually eligible (i.e., the intercepted response contains
json.settings). Update the interceptor code that uses getCount and the
conditional that injects the fake project to check for existence of
json.settings before toggling/advancing the first-response behavior (referencing
getCount and json.settings), and make the same fix for the other similar block
around the code referenced at lines 104-106.
- Increased the number of workers in Playwright configuration for better parallelism in CI environments. - Enhanced the board background persistence test to ensure dropdown stability by waiting for the list to populate before interaction, improving test reliability.
- Updated e2e-tests.yml to run tests in a single shard for streamlined CI execution. - Enhanced unit tests for worktree list handling by introducing a mock for execGitCommand, improving test reliability and coverage. - Refactored setup functions to better manage command mocks for git operations in tests. - Improved error handling in mkdirSafe function to account for undefined stats in certain environments.
- Updated Playwright configuration to clear VITE_SERVER_URL, ensuring the frontend uses the Vite proxy and preventing cookie domain mismatches. - Enhanced MergeRebaseDialog logic to normalize selectedBranch for better handling of various ref formats. - Improved global setup with a more robust backend health check, throwing an error if the backend is not healthy after retries. - Refactored project creation tests to handle file existence checks more reliably. - Added error handling for missing E2E source fixtures to guide setup process. - Enhanced memory navigation to handle sandbox dialog visibility more effectively.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/tests/projects/board-background-persistence.spec.ts (1)
551-571:⚠️ Potential issue | 🟡 MinorAdd error handling for
route.fetch()to prevent flaky failures.The first test (lines 276-284) properly handles
route.fetch()failures during navigation with a try-catch. This route handler lacks equivalent protection, which could cause intermittent test failures.🛡️ Suggested fix
await page.route('**/api/settings/global', async (route) => { if (route.request().method() !== 'GET') { await route.continue(); return; } - const response = await route.fetch(); - const json = await response.json(); + let response; + let json; + try { + response = await route.fetch(); + json = await response.json(); + } catch { + await route.continue().catch(() => {}); + return; + } // Override to use our test project if (json.settings) { json.settings.currentProjectId = projectId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 551 - 571, The route handler for page.route('**/api/settings/global') does an unprotected await route.fetch() which can throw and cause flaky failures; wrap the route.fetch() call in a try-catch, check response.ok (or handle non-2xx) and on error or non-OK response either call route.continue() or fulfill with the original response (preserving headers/status) and only modify and fulfill with the JSON when fetch succeeds; update the callback where route.fetch(), json mutation, and route.fulfill() are used to use this safe flow.
♻️ Duplicate comments (3)
apps/ui/tests/utils/components/responsive-modal.ts (2)
199-207:⚠️ Potential issue | 🟡 MinorDesktop height assertions are still missing in
testModalHeightAcrossViewports.At Line 199, only mobile/tablet branches assert height.
desktop/desktopLargepaths currently do no validation and can pass silently.Proposed fix
if (viewport === 'mobile' || viewport === 'mobileLarge') { // Mobile: should be max-h-[85dvh] const expected85dvh = Math.floor(viewportHeight * 0.85); expect(modalHeight).toBeLessThanOrEqual(expected85dvh); } else if (viewport === 'tablet' || viewport === 'tabletLarge') { // Tablet: should be max-h-[85vh] const expected85vh = Math.floor(viewportHeight * 0.85); expect(modalHeight).toBeLessThanOrEqual(expected85vh); + } else if (viewport === 'desktop' || viewport === 'desktopLarge') { + // Desktop: still bounded by viewport constraints + const expected85vh = Math.floor(viewportHeight * 0.85); + expect(modalHeight).toBeLessThanOrEqual(expected85vh); + expect(modalHeight).toBeGreaterThan(0); + } else { + throw new Error(`No height assertion configured for viewport: ${viewport}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 199 - 207, The testModalHeightAcrossViewports helper is missing assertions for 'desktop' and 'desktopLarge' viewports so desktop cases pass without validation; add a desktop branch that computes the expected max height (use Math.floor(viewportHeight * 0.85) like mobile/tablet or the correct desktop spec) and assert modalHeight is <= expected value, referencing the testModalHeightAcrossViewports function and the modalHeight, viewportHeight, and viewport variables so the desktop and desktopLarge paths enforce the same height constraint as mobile/tablet.
235-246:⚠️ Potential issue | 🟡 Minor
testModalResponsiveResizestill skips assertions for most viewport transitions.At Lines 235-246, only
mobile -> tabletandtablet -> mobileare covered. Other transitions exit without verification.Proposed fix
if (fromViewport === 'mobile' && toViewport === 'tablet') { const widthPx = parseFloat(newDimensions.width); const maxWidthPx = parseFloat(newDimensions.maxWidth); const expected90vw = toSize.width * 0.9; expect(widthPx).toBeLessThanOrEqual(expected90vw + 2); expect(maxWidthPx).toBeGreaterThanOrEqual(1200); } else if (fromViewport === 'tablet' && toViewport === 'mobile') { const widthPx = parseFloat(newDimensions.width); const maxWidthPx = parseFloat(newDimensions.maxWidth); expect(widthPx).toBeGreaterThan(toSize.width - 60); expect(maxWidthPx).toBeLessThan(1200); + } else { + throw new Error( + `No resize assertion configured for transition: ${fromViewport} -> ${toViewport}` + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/components/responsive-modal.ts` around lines 235 - 246, The test currently only asserts behavior for the 'mobile -> tablet' and 'tablet -> mobile' branches, causing other transitions in testModalResponsiveResize to skip verification; update the branching on fromViewport/toViewport (the logic around newDimensions, toSize, widthPx, maxWidthPx) to include assertions for the remaining transitions (mobile->desktop, tablet->desktop, desktop->tablet, desktop->mobile) by computing parseFloat(newDimensions.width) and parseFloat(newDimensions.maxWidth) and asserting expected relationships (e.g., desktop transitions should clamp to desktop maxWidth thresholds, mobile->desktop should increase width close to toSize.width, desktop->tablet should reduce width and maxWidth appropriately) so every fromViewport/toViewport combination is covered using the same variable names (newDimensions, toSize, widthPx, maxWidthPx) and existing expectation helpers.apps/ui/tests/projects/new-project-creation.spec.ts (1)
104-114:⚠️ Potential issue | 🟠 Major
/api/fs/existsstill reports new project dirs as existing, so creation paths can be skipped.At Line 113,
isProjectDiris treated as existing. That recreates the earlier false-positive behavior for new project creation and can bypass mkdir/file-init logic.Proposed fix
- const isWorkspace = normalized === workspaceDir; - const isProjectDir = - normalized.startsWith(workspaceDir + '/') && - normalized.slice(workspaceDir.length + 1).indexOf('/') === -1; - const exists = isWorkspace || isProjectDir; + // Only workspace root exists initially; new project paths should not. + const exists = normalized === workspaceDir; await route.fulfill(fsJson(200, { success: true, exists }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/new-project-creation.spec.ts` around lines 104 - 114, The route handler currently treats any immediate child of workspaceDir as existing via the isProjectDir logic, causing new project dirs to be reported as existing; update the handler so that isProjectDir alone does not imply existence—introduce a set/array of preExistingProjects (e.g., preExistingProjects) and change the existence calculation to const exists = isWorkspace || (isProjectDir && preExistingProjects.has(normalized)) (or .includes(normalized)) so only known existing project paths return true; adjust the mock data for preExistingProjects where this test expects non-existing new project names.
🧹 Nitpick comments (6)
.github/workflows/e2e-tests.yml (1)
171-192: Consider includingshardTotalin artifact names for stronger uniqueness.This is fine as-is for the current matrix, but including both index and total reduces collision risk and improves traceability if matrix dimensions evolve.
Suggested naming tweak
- name: playwright-report-shard-${{ matrix.shardIndex }} + name: playwright-report-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }} - name: test-results-shard-${{ matrix.shardIndex }} + name: test-results-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }} - name: blob-report-shard-${{ matrix.shardIndex }} + name: blob-report-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-tests.yml around lines 171 - 192, Update the artifact name fields to include the matrix total for stronger uniqueness: change occurrences of "playwright-report-shard-${{ matrix.shardIndex }}", "test-results-shard-${{ matrix.shardIndex }}", and "blob-report-shard-${{ matrix.shardIndex }}" to include "-${{ matrix.shardTotal }}" (e.g., "playwright-report-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }}"), and ensure the workflow matrix defines shardTotal so the Upload artifact steps use both matrix.shardIndex and matrix.shardTotal.apps/server/src/lib/git.ts (1)
89-89: Preserve extended Git PATH even when a customPATHis provided.At Line 89, spreading
{ ...gitEnv, ...env }allowsenv.PATHto overwriteextendedPath, which can reintroduce Git lookup failures in constrained runtimes.Patch suggestion
- env: env !== undefined ? { ...gitEnv, ...env } : gitEnv, + env: + env !== undefined + ? { + ...gitEnv, + ...env, + PATH: [env.PATH ?? process.env.PATH, ...extraPaths] + .filter(Boolean) + .join(pathSeparator), + } + : gitEnv,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/git.ts` at line 89, The current env merge (env: env !== undefined ? { ...gitEnv, ...env } : gitEnv) allows a caller-provided env.PATH to fully overwrite the Git-specific extended PATH and reintroduce lookup failures; change the merge so PATH is composed rather than replaced: when env is provided, build the resulting PATH by joining gitEnv.PATH (or gitEnv.extendedPath if that's the symbol in this file) with env.PATH (preserving both, using path delimiter), then spread the other variables from gitEnv and env (i.e., { ...gitEnv, ...env, PATH: composedPath }) so the Git extended path is always preserved while still honoring caller-supplied PATH.apps/ui/tests/utils/cleanup-test-dirs.ts (1)
35-50: Consider reading the directory once outside the loop.The directory is read with
fs.readdirSync(testBase)inside the prefix loop, resulting in redundant I/O for each of the 13 prefixes. Reading once and filtering would be more efficient.♻️ Proposed optimization
export function cleanupLeftoverTestDirs(): void { const testBase = path.join(getWorkspaceRoot(), 'test'); if (!fs.existsSync(testBase)) return; + const entries = fs.readdirSync(testBase, { withFileTypes: true }); for (const prefix of TEST_DIR_PREFIXES) { const pattern = prefix + '-'; - const entries = fs.readdirSync(testBase, { withFileTypes: true }); for (const entry of entries) { if (entry.isDirectory() && entry.name.startsWith(pattern)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/cleanup-test-dirs.ts` around lines 35 - 50, The loop repeatedly calls fs.readdirSync(testBase) for each prefix in TEST_DIR_PREFIXES causing redundant I/O; change to read entries once into a variable (e.g., const entries = fs.readdirSync(testBase, { withFileTypes: true })) before iterating prefixes, then inside the TEST_DIR_PREFIXES loop filter that entries array (using entry.isDirectory() and entry.name.startsWith(pattern)) to determine which dirs to remove; update uses of entries, pattern, dirPath, and the existing fs.rmSync/console logging accordingly.apps/ui/playwright.config.ts (1)
42-43: Incomplete comment for workers configuration.The comment describes CI workers but doesn't complete the sentence for local workers. This appears to be leftover from removing the "50% of CPUs" claim.
📝 Suggested comment fix
- // Use multiple workers for parallelism. CI gets 2 workers (constrained resources), + // Use multiple workers for parallelism. CI gets 2 workers (constrained resources); + // local gets 8 workers. Each worker gets its own browser context with shared auth state. workers: process.env.CI ? 2 : 8,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/playwright.config.ts` around lines 42 - 43, The inline comment for the Playwright workers setting is incomplete; update the comment above the workers configuration (the workers property using process.env.CI) to clearly state the behavior for both CI and local runs—for example: "CI gets 2 workers (constrained resources), local runs use 8 workers"—so the comment fully documents the value chosen for workers in both environments.apps/ui/tests/projects/board-background-persistence.spec.ts (1)
324-334: Remove redundant board view visibility check.The board view visibility is checked twice consecutively (line 324 and lines 328-329). The
boardViewvariable declared on line 328 is not used after the visibility check.🧹 Suggested cleanup
// Wait for board view await expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 15000 }); - // Wait for settings to be loaded (useProjectSettingsLoader hook) - // Poll for the board view to be fully rendered and stable - const boardView = page.locator('[data-testid="board-view"]'); - await expect(boardView).toBeVisible({ timeout: 15000 }); - // Wait for settings API calls to complete (at least one settings call should have been made) await expect(async () => { expect(settingsApiCalls.length).toBeGreaterThan(0); }).toPass({ timeout: 10000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 324 - 334, Remove the duplicate visibility assertion for the board view: eliminate the second await expect(boardView).toBeVisible({ timeout: 15000 }) and the unused boardView constant (the locator assigned to boardView for '[data-testid="board-view"]'). Keep the initial await expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 15000 }) and retain the subsequent settings API poll (the expect(settingsApiCalls.length).toBeGreaterThan(0) wrapped in toPass).apps/server/tests/unit/routes/worktree/list-detached-head.test.ts (1)
77-112: Consider removing unusedsetupExecMockhelper.The
setupExecMockfunction is defined but never called in this file. The tests at lines 842 and 954 mockexecdirectly instead. If this helper isn't shared with other test files, consider removing it to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts` around lines 77 - 112, Remove the unused test helper setupExecMock: locate the function named setupExecMock in this test file and delete its entire definition since exec is mocked directly in the tests (the helper is not referenced anywhere in this file); if you intended the helper to be shared, instead export it and update the tests to call setupExecMock where applicable — otherwise simply remove the unused function to reduce maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/init-git.ts`:
- Around line 48-54: The current chained execAsync call in init-git.ts (calling
git init && git commit) can leave a .git dir with no commits if the commit
fails; update the logic in the function that invokes execAsync so you run
commands sequentially and handle failures: first call execAsync("git init
--initial-branch=main", {cwd: projectPath}), then set local config
(execAsync("git config user.name ...", {cwd: projectPath}) and execAsync("git
config user.email ...", {cwd: projectPath})) or detect and skip if already set,
then create the initial commit (execAsync("git commit --allow-empty -m 'Initial
commit'", {cwd: projectPath})); if any step fails, clean up the partial state by
removing the created .git directory (or detect an existing repo with no commits
via git rev-parse --verify HEAD and handle accordingly) so subsequent runs
behave deterministically.
In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts`:
- Around line 809-841: The test currently calls setupStandardExec(...) then
immediately overwrites that mock by calling
vi.mocked(execGitCommand).mockImplementation(...); remove the redundant
setupStandardExec call or move its expected behavior into setupStandardExec so
execGitCommand is mocked only once; specifically either delete the
setupStandardExec invocation around the porcelainOutput/worktreeBranches block,
or update setupStandardExec to return the same implementations used in the
inline mock for execGitCommand (handling 'rev-parse --git-dir', 'worktree list',
'branch --show-current', and 'status --porcelain') so there is no
duplicate/overwritten mocking.
- Around line 921-953: The test currently calls setupStandardExec(...) but
immediately replaces its behavior by calling
vi.mocked(execGitCommand).mockImplementation(...), so the standardized mock is
overwritten; fix this by either removing the direct mockImplementation and
relying on setupStandardExec to configure execGitCommand, or modify the
mockImplementation to delegate to the setupStandardExec-provided behavior for
unmatched args (e.g., call the original/mock from setupStandardExec or use
mockImplementationOnce for the special-case commands); update the code around
setupStandardExec and execGitCommand so that execGitCommand's mock preserves the
setupStandardExec behavior while adding only the specific overrides for
rev-parse, worktree list, branch --show-current, and status --porcelain.
- Around line 375-421: Remove the redundant call to setupStandardExec: the test
invokes setupStandardExec twice with the same args (the second call after
vi.mocked(secureFs.readFile).mockRejectedValue...). Delete the second
setupStandardExec invocation so the initial setupStandardExec call remains
(alongside disableWorktreesScan and the readFile mock) before creating the
handler via createListHandler and invoking handler(req, res).
In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts`:
- Around line 35-40: The cancelQueries calls in use-board-persistence.ts are
currently using prefix-based matching and thus cancel descendant keys like
features.single(...) and features.agentOutput(...); update each
queryClient.cancelQueries invocation that passes
queryKeys.features.all(currentProject.path) (three occurrences in this file) to
include the option exact: true so only the features-list key is cancelled (i.e.,
call cancelQueries with { queryKey: queryKeys.features.all(currentProject.path),
exact: true }).
In `@apps/ui/src/components/views/dashboard-view.tsx`:
- Line 600: The two DropdownMenuItem elements in dashboard-view.tsx both use
data-testid="quick-setup-option", creating ambiguous selectors; update each to
unique test IDs (e.g., assign one in the hasProjects branch and one in the
no-projects/quick-setup branch) so tests can target them
deterministically—locate the DropdownMenuItem with onClick={handleNewProject}
and the other occurrence and rename their data-testid values to distinct
identifiers like "quick-setup-option-has-projects" and
"quick-setup-option-no-projects".
In `@apps/ui/tests/features/edit-feature.spec.ts`:
- Around line 127-131: The test currently swallows failures by using .catch(()
=> {}) on the wait for the sidebar expand button (the
page.locator('button:has-text("Expand sidebar")').waitFor({...}).catch(() =>
{})) which hides real failures; remove the empty catch and instead assert
visibility using Playwright's expect (e.g. await
expect(page.locator('button:has-text("Expand sidebar")')).toBeVisible({ timeout:
5000 })) or, if you need custom handling, catch the error, log it with context,
and rethrow so the test fails visibly.
In `@apps/ui/tests/utils/git/worktree.ts`:
- Around line 172-180: The cleanup catch block for fs.rmSync may hit ENOENT
during the fallback and should ignore that case to avoid intermittent failures;
update the catch handling in the try/catch around fs.rmSync(tempDir...) to treat
code === 'ENOENT' as a no-op (in addition to existing ENOTEMPTY/EPERM/EBUSY
logic) before calling rmDirRecursive(tempDir) and fs.rmdirSync(tempDir), or
check for ENOENT after the fallback rmdir and swallow it instead, referencing
the same tempDir, rmDirRecursive and fs.rmdirSync calls so the cleanup no longer
throws when the dir was already removed.
---
Outside diff comments:
In `@apps/ui/tests/projects/board-background-persistence.spec.ts`:
- Around line 551-571: The route handler for
page.route('**/api/settings/global') does an unprotected await route.fetch()
which can throw and cause flaky failures; wrap the route.fetch() call in a
try-catch, check response.ok (or handle non-2xx) and on error or non-OK response
either call route.continue() or fulfill with the original response (preserving
headers/status) and only modify and fulfill with the JSON when fetch succeeds;
update the callback where route.fetch(), json mutation, and route.fulfill() are
used to use this safe flow.
---
Duplicate comments:
In `@apps/ui/tests/projects/new-project-creation.spec.ts`:
- Around line 104-114: The route handler currently treats any immediate child of
workspaceDir as existing via the isProjectDir logic, causing new project dirs to
be reported as existing; update the handler so that isProjectDir alone does not
imply existence—introduce a set/array of preExistingProjects (e.g.,
preExistingProjects) and change the existence calculation to const exists =
isWorkspace || (isProjectDir && preExistingProjects.has(normalized)) (or
.includes(normalized)) so only known existing project paths return true; adjust
the mock data for preExistingProjects where this test expects non-existing new
project names.
In `@apps/ui/tests/utils/components/responsive-modal.ts`:
- Around line 199-207: The testModalHeightAcrossViewports helper is missing
assertions for 'desktop' and 'desktopLarge' viewports so desktop cases pass
without validation; add a desktop branch that computes the expected max height
(use Math.floor(viewportHeight * 0.85) like mobile/tablet or the correct desktop
spec) and assert modalHeight is <= expected value, referencing the
testModalHeightAcrossViewports function and the modalHeight, viewportHeight, and
viewport variables so the desktop and desktopLarge paths enforce the same height
constraint as mobile/tablet.
- Around line 235-246: The test currently only asserts behavior for the 'mobile
-> tablet' and 'tablet -> mobile' branches, causing other transitions in
testModalResponsiveResize to skip verification; update the branching on
fromViewport/toViewport (the logic around newDimensions, toSize, widthPx,
maxWidthPx) to include assertions for the remaining transitions
(mobile->desktop, tablet->desktop, desktop->tablet, desktop->mobile) by
computing parseFloat(newDimensions.width) and parseFloat(newDimensions.maxWidth)
and asserting expected relationships (e.g., desktop transitions should clamp to
desktop maxWidth thresholds, mobile->desktop should increase width close to
toSize.width, desktop->tablet should reduce width and maxWidth appropriately) so
every fromViewport/toViewport combination is covered using the same variable
names (newDimensions, toSize, widthPx, maxWidthPx) and existing expectation
helpers.
---
Nitpick comments:
In @.github/workflows/e2e-tests.yml:
- Around line 171-192: Update the artifact name fields to include the matrix
total for stronger uniqueness: change occurrences of
"playwright-report-shard-${{ matrix.shardIndex }}", "test-results-shard-${{
matrix.shardIndex }}", and "blob-report-shard-${{ matrix.shardIndex }}" to
include "-${{ matrix.shardTotal }}" (e.g., "playwright-report-shard-${{
matrix.shardIndex }}-of-${{ matrix.shardTotal }}"), and ensure the workflow
matrix defines shardTotal so the Upload artifact steps use both
matrix.shardIndex and matrix.shardTotal.
In `@apps/server/src/lib/git.ts`:
- Line 89: The current env merge (env: env !== undefined ? { ...gitEnv, ...env }
: gitEnv) allows a caller-provided env.PATH to fully overwrite the Git-specific
extended PATH and reintroduce lookup failures; change the merge so PATH is
composed rather than replaced: when env is provided, build the resulting PATH by
joining gitEnv.PATH (or gitEnv.extendedPath if that's the symbol in this file)
with env.PATH (preserving both, using path delimiter), then spread the other
variables from gitEnv and env (i.e., { ...gitEnv, ...env, PATH: composedPath })
so the Git extended path is always preserved while still honoring
caller-supplied PATH.
In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts`:
- Around line 77-112: Remove the unused test helper setupExecMock: locate the
function named setupExecMock in this test file and delete its entire definition
since exec is mocked directly in the tests (the helper is not referenced
anywhere in this file); if you intended the helper to be shared, instead export
it and update the tests to call setupExecMock where applicable — otherwise
simply remove the unused function to reduce maintenance.
In `@apps/ui/playwright.config.ts`:
- Around line 42-43: The inline comment for the Playwright workers setting is
incomplete; update the comment above the workers configuration (the workers
property using process.env.CI) to clearly state the behavior for both CI and
local runs—for example: "CI gets 2 workers (constrained resources), local runs
use 8 workers"—so the comment fully documents the value chosen for workers in
both environments.
In `@apps/ui/tests/projects/board-background-persistence.spec.ts`:
- Around line 324-334: Remove the duplicate visibility assertion for the board
view: eliminate the second await expect(boardView).toBeVisible({ timeout: 15000
}) and the unused boardView constant (the locator assigned to boardView for
'[data-testid="board-view"]'). Keep the initial await
expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 15000
}) and retain the subsequent settings API poll (the
expect(settingsApiCalls.length).toBeGreaterThan(0) wrapped in toPass).
In `@apps/ui/tests/utils/cleanup-test-dirs.ts`:
- Around line 35-50: The loop repeatedly calls fs.readdirSync(testBase) for each
prefix in TEST_DIR_PREFIXES causing redundant I/O; change to read entries once
into a variable (e.g., const entries = fs.readdirSync(testBase, { withFileTypes:
true })) before iterating prefixes, then inside the TEST_DIR_PREFIXES loop
filter that entries array (using entry.isDirectory() and
entry.name.startsWith(pattern)) to determine which dirs to remove; update uses
of entries, pattern, dirPath, and the existing fs.rmSync/console logging
accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/e2e-tests.ymlapps/server/src/lib/git.tsapps/server/src/routes/features/routes/update.tsapps/server/src/routes/worktree/routes/init-git.tsapps/server/tests/unit/routes/worktree/list-detached-head.test.tsapps/ui/playwright.config.tsapps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-persistence.tsapps/ui/src/components/views/dashboard-view.tsxapps/ui/tests/context/mobile-context-view.spec.tsapps/ui/tests/features/edit-feature.spec.tsapps/ui/tests/global-setup.tsapps/ui/tests/global-teardown.tsapps/ui/tests/memory/mobile-memory-view.spec.tsapps/ui/tests/projects/board-background-persistence.spec.tsapps/ui/tests/projects/new-project-creation.spec.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/utils/cleanup-test-dirs.tsapps/ui/tests/utils/components/responsive-modal.tsapps/ui/tests/utils/git/worktree.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/project/setup.tsapps/ui/tests/utils/views/board.tsapps/ui/tests/utils/views/memory.tslibs/utils/src/atomic-writer.tslibs/utils/src/fs-utils.tslibs/utils/tests/atomic-writer.test.tstest/fixtures/projectA
💤 Files with no reviewable changes (2)
- apps/ui/tests/context/mobile-context-view.spec.ts
- apps/ui/tests/memory/mobile-memory-view.spec.ts
Comment on lines
+48
to
+54
| try { | ||
| await execAsync( | ||
| `git init --initial-branch=main && git commit --allow-empty -m "Initial commit"`, | ||
| { | ||
| cwd: projectPath, | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Potential partial initialization if commit fails.
The chained command can leave the repository in an inconsistent state: if git init succeeds but git commit fails (e.g., missing user.name/user.email config), the .git directory exists but contains no commits. Subsequent calls will return "already exists" without the initial commit.
Consider either:
- Setting local git config for the commit, or
- Handling the partial state explicitly
🔧 Option 1: Set git config inline for the commit
try {
await execAsync(
- `git init --initial-branch=main && git commit --allow-empty -m "Initial commit"`,
+ `git init --initial-branch=main && git -c user.name="AutoMaker" -c user.email="automaker@local" commit --allow-empty -m "Initial commit"`,
{
cwd: projectPath,
}
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| await execAsync( | |
| `git init --initial-branch=main && git commit --allow-empty -m "Initial commit"`, | |
| { | |
| cwd: projectPath, | |
| } | |
| ); | |
| try { | |
| await execAsync( | |
| `git init --initial-branch=main && git -c user.name="AutoMaker" -c user.email="automaker@local" commit --allow-empty -m "Initial commit"`, | |
| { | |
| cwd: projectPath, | |
| } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/worktree/routes/init-git.ts` around lines 48 - 54, The
current chained execAsync call in init-git.ts (calling git init && git commit)
can leave a .git dir with no commits if the commit fails; update the logic in
the function that invokes execAsync so you run commands sequentially and handle
failures: first call execAsync("git init --initial-branch=main", {cwd:
projectPath}), then set local config (execAsync("git config user.name ...",
{cwd: projectPath}) and execAsync("git config user.email ...", {cwd:
projectPath})) or detect and skip if already set, then create the initial commit
(execAsync("git commit --allow-empty -m 'Initial commit'", {cwd: projectPath}));
if any step fails, clean up the partial state by removing the created .git
directory (or detect an existing repo with no commits via git rev-parse --verify
HEAD and handle accordingly) so subsequent runs behave deterministically.
Comment on lines
+375
to
+421
| it('should fall back to (detached) when all branch recovery methods fail', async () => { | ||
| req.body = { projectPath: '/project' }; | ||
|
|
||
| setupStandardExec({ | ||
| porcelainOutput: [ | ||
| 'worktree /project', | ||
| 'branch refs/heads/main', | ||
| '', | ||
| 'worktree /project/.worktrees/unknown-wt', | ||
| 'detached', | ||
| '', | ||
| ].join('\n'), | ||
| worktreeBranches: { | ||
| '/project/.worktrees/unknown-wt': '', // empty = no branch | ||
| }, | ||
| }); | ||
| disableWorktreesScan(); | ||
|
|
||
| // All readFile calls fail (no gitDirs so rev-parse --git-dir will throw) | ||
| vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); | ||
|
|
||
| setupStandardExec({ | ||
| porcelainOutput: [ | ||
| 'worktree /project', | ||
| 'branch refs/heads/main', | ||
| '', | ||
| 'worktree /project/.worktrees/unknown-wt', | ||
| 'detached', | ||
| '', | ||
| ].join('\n'), | ||
| worktreeBranches: { | ||
| '/project/.worktrees/unknown-wt': '', // empty = no branch | ||
| }, | ||
| }); | ||
|
|
||
| const handler = createListHandler(); | ||
| await handler(req, res); | ||
|
|
||
| const response = vi.mocked(res.json).mock.calls[0][0] as { | ||
| worktrees: Array<{ branch: string; path: string }>; | ||
| }; | ||
| const detachedWt = response.worktrees.find( | ||
| (w) => w.path === '/project/.worktrees/unknown-wt' | ||
| ); | ||
| expect(detachedWt).toBeDefined(); | ||
| expect(detachedWt!.branch).toBe('(detached)'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Duplicate setupStandardExec call.
setupStandardExec is called twice with identical configuration (lines 378-390 and 396-408). The second call completely overwrites the first, making the first call redundant.
🔧 Proposed fix to remove duplicate setup
it('should fall back to (detached) when all branch recovery methods fail', async () => {
req.body = { projectPath: '/project' };
- setupStandardExec({
- porcelainOutput: [
- 'worktree /project',
- 'branch refs/heads/main',
- '',
- 'worktree /project/.worktrees/unknown-wt',
- 'detached',
- '',
- ].join('\n'),
- worktreeBranches: {
- '/project/.worktrees/unknown-wt': '', // empty = no branch
- },
- });
- disableWorktreesScan();
-
// All readFile calls fail (no gitDirs so rev-parse --git-dir will throw)
vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT'));
setupStandardExec({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should fall back to (detached) when all branch recovery methods fail', async () => { | |
| req.body = { projectPath: '/project' }; | |
| setupStandardExec({ | |
| porcelainOutput: [ | |
| 'worktree /project', | |
| 'branch refs/heads/main', | |
| '', | |
| 'worktree /project/.worktrees/unknown-wt', | |
| 'detached', | |
| '', | |
| ].join('\n'), | |
| worktreeBranches: { | |
| '/project/.worktrees/unknown-wt': '', // empty = no branch | |
| }, | |
| }); | |
| disableWorktreesScan(); | |
| // All readFile calls fail (no gitDirs so rev-parse --git-dir will throw) | |
| vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); | |
| setupStandardExec({ | |
| porcelainOutput: [ | |
| 'worktree /project', | |
| 'branch refs/heads/main', | |
| '', | |
| 'worktree /project/.worktrees/unknown-wt', | |
| 'detached', | |
| '', | |
| ].join('\n'), | |
| worktreeBranches: { | |
| '/project/.worktrees/unknown-wt': '', // empty = no branch | |
| }, | |
| }); | |
| const handler = createListHandler(); | |
| await handler(req, res); | |
| const response = vi.mocked(res.json).mock.calls[0][0] as { | |
| worktrees: Array<{ branch: string; path: string }>; | |
| }; | |
| const detachedWt = response.worktrees.find( | |
| (w) => w.path === '/project/.worktrees/unknown-wt' | |
| ); | |
| expect(detachedWt).toBeDefined(); | |
| expect(detachedWt!.branch).toBe('(detached)'); | |
| }); | |
| it('should fall back to (detached) when all branch recovery methods fail', async () => { | |
| req.body = { projectPath: '/project' }; | |
| // All readFile calls fail (no gitDirs so rev-parse --git-dir will throw) | |
| vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); | |
| setupStandardExec({ | |
| porcelainOutput: [ | |
| 'worktree /project', | |
| 'branch refs/heads/main', | |
| '', | |
| 'worktree /project/.worktrees/unknown-wt', | |
| 'detached', | |
| '', | |
| ].join('\n'), | |
| worktreeBranches: { | |
| '/project/.worktrees/unknown-wt': '', // empty = no branch | |
| }, | |
| }); | |
| const handler = createListHandler(); | |
| await handler(req, res); | |
| const response = vi.mocked(res.json).mock.calls[0][0] as { | |
| worktrees: Array<{ branch: string; path: string }>; | |
| }; | |
| const detachedWt = response.worktrees.find( | |
| (w) => w.path === '/project/.worktrees/unknown-wt' | |
| ); | |
| expect(detachedWt).toBeDefined(); | |
| expect(detachedWt!.branch).toBe('(detached)'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts` around
lines 375 - 421, Remove the redundant call to setupStandardExec: the test
invokes setupStandardExec twice with the same args (the second call after
vi.mocked(secureFs.readFile).mockRejectedValue...). Delete the second
setupStandardExec invocation so the initial setupStandardExec call remains
(alongside disableWorktreesScan and the readFile mock) before creating the
handler via createListHandler and invoking handler(req, res).
Comment on lines
+809
to
+841
| setupStandardExec({ | ||
| porcelainOutput: [ | ||
| 'worktree /project', | ||
| 'branch refs/heads/main', | ||
| '', | ||
| 'worktree /project/.worktrees/feature-a', | ||
| 'branch refs/heads/feature-a', | ||
| '', | ||
| ].join('\n'), | ||
| worktreeBranches: { '/project': 'main', '/project/.worktrees/feature-a': 'feature-a' }, | ||
| }); | ||
| vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { | ||
| if (args[0] === 'rev-parse' && args[1] === '--git-dir') { | ||
| throw new Error('no git dir'); | ||
| } | ||
| if (args[0] === 'worktree' && args[1] === 'list') { | ||
| return [ | ||
| 'worktree /project', | ||
| 'branch refs/heads/main', | ||
| '', | ||
| 'worktree /project/.worktrees/feature-a', | ||
| 'branch refs/heads/feature-a', | ||
| '', | ||
| ].join('\n'); | ||
| } | ||
| if (args[0] === 'branch' && args[1] === '--show-current') { | ||
| return cwd === '/project' ? 'main\n' : 'feature-a\n'; | ||
| } | ||
| if (args[0] === 'status' && args[1] === '--porcelain') { | ||
| return ''; | ||
| } | ||
| return ''; | ||
| }); |
Contributor
There was a problem hiding this comment.
Ineffective setupStandardExec call immediately overwritten.
The setupStandardExec call at lines 809-819 is immediately overwritten by the direct vi.mocked(execGitCommand).mockImplementation at lines 820-841. Remove the unused setupStandardExec call or consolidate the mock setup.
🔧 Proposed fix
- setupStandardExec({
- porcelainOutput: [
- 'worktree /project',
- 'branch refs/heads/main',
- '',
- 'worktree /project/.worktrees/feature-a',
- 'branch refs/heads/feature-a',
- '',
- ].join('\n'),
- worktreeBranches: { '/project': 'main', '/project/.worktrees/feature-a': 'feature-a' },
- });
vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts` around
lines 809 - 841, The test currently calls setupStandardExec(...) then
immediately overwrites that mock by calling
vi.mocked(execGitCommand).mockImplementation(...); remove the redundant
setupStandardExec call or move its expected behavior into setupStandardExec so
execGitCommand is mocked only once; specifically either delete the
setupStandardExec invocation around the porcelainOutput/worktreeBranches block,
or update setupStandardExec to return the same implementations used in the
inline mock for execGitCommand (handling 'rev-parse --git-dir', 'worktree list',
'branch --show-current', and 'status --porcelain') so there is no
duplicate/overwritten mocking.
Comment on lines
+921
to
+953
| setupStandardExec({ | ||
| porcelainOutput: [ | ||
| 'worktree /project-2', | ||
| 'branch refs/heads/main', | ||
| '', | ||
| 'worktree /project-2/.worktrees/feature-a', | ||
| 'branch refs/heads/feature-a', | ||
| '', | ||
| ].join('\n'), | ||
| worktreeBranches: { '/project-2': 'main', '/project-2/.worktrees/feature-a': 'feature-a' }, | ||
| }); | ||
| vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { | ||
| if (args[0] === 'rev-parse' && args[1] === '--git-dir') { | ||
| throw new Error('no git dir'); | ||
| } | ||
| if (args[0] === 'worktree' && args[1] === 'list') { | ||
| return [ | ||
| 'worktree /project-2', | ||
| 'branch refs/heads/main', | ||
| '', | ||
| 'worktree /project-2/.worktrees/feature-a', | ||
| 'branch refs/heads/feature-a', | ||
| '', | ||
| ].join('\n'); | ||
| } | ||
| if (args[0] === 'branch' && args[1] === '--show-current') { | ||
| return cwd === '/project-2' ? 'main\n' : 'feature-a\n'; | ||
| } | ||
| if (args[0] === 'status' && args[1] === '--porcelain') { | ||
| return ''; | ||
| } | ||
| return ''; | ||
| }); |
Contributor
There was a problem hiding this comment.
Same issue: setupStandardExec overwritten by direct mock.
Similar to the previous test, setupStandardExec (lines 921-931) is immediately overwritten by vi.mocked(execGitCommand).mockImplementation (lines 932-953).
🔧 Proposed fix
- setupStandardExec({
- porcelainOutput: [
- 'worktree /project-2',
- 'branch refs/heads/main',
- '',
- 'worktree /project-2/.worktrees/feature-a',
- 'branch refs/heads/feature-a',
- '',
- ].join('\n'),
- worktreeBranches: { '/project-2': 'main', '/project-2/.worktrees/feature-a': 'feature-a' },
- });
vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: 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.
Suggested change
| setupStandardExec({ | |
| porcelainOutput: [ | |
| 'worktree /project-2', | |
| 'branch refs/heads/main', | |
| '', | |
| 'worktree /project-2/.worktrees/feature-a', | |
| 'branch refs/heads/feature-a', | |
| '', | |
| ].join('\n'), | |
| worktreeBranches: { '/project-2': 'main', '/project-2/.worktrees/feature-a': 'feature-a' }, | |
| }); | |
| vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { | |
| if (args[0] === 'rev-parse' && args[1] === '--git-dir') { | |
| throw new Error('no git dir'); | |
| } | |
| if (args[0] === 'worktree' && args[1] === 'list') { | |
| return [ | |
| 'worktree /project-2', | |
| 'branch refs/heads/main', | |
| '', | |
| 'worktree /project-2/.worktrees/feature-a', | |
| 'branch refs/heads/feature-a', | |
| '', | |
| ].join('\n'); | |
| } | |
| if (args[0] === 'branch' && args[1] === '--show-current') { | |
| return cwd === '/project-2' ? 'main\n' : 'feature-a\n'; | |
| } | |
| if (args[0] === 'status' && args[1] === '--porcelain') { | |
| return ''; | |
| } | |
| return ''; | |
| }); | |
| vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { | |
| if (args[0] === 'rev-parse' && args[1] === '--git-dir') { | |
| throw new Error('no git dir'); | |
| } | |
| if (args[0] === 'worktree' && args[1] === 'list') { | |
| return [ | |
| 'worktree /project-2', | |
| 'branch refs/heads/main', | |
| '', | |
| 'worktree /project-2/.worktrees/feature-a', | |
| 'branch refs/heads/feature-a', | |
| '', | |
| ].join('\n'); | |
| } | |
| if (args[0] === 'branch' && args[1] === '--show-current') { | |
| return cwd === '/project-2' ? 'main\n' : 'feature-a\n'; | |
| } | |
| if (args[0] === 'status' && args[1] === '--porcelain') { | |
| return ''; | |
| } | |
| return ''; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/tests/unit/routes/worktree/list-detached-head.test.ts` around
lines 921 - 953, The test currently calls setupStandardExec(...) but immediately
replaces its behavior by calling
vi.mocked(execGitCommand).mockImplementation(...), so the standardized mock is
overwritten; fix this by either removing the direct mockImplementation and
relying on setupStandardExec to configure execGitCommand, or modify the
mockImplementation to delegate to the setupStandardExec-provided behavior for
unmatched args (e.g., call the original/mock from setupStandardExec or use
mockImplementationOnce for the special-case commands); update the code around
setupStandardExec and execGitCommand so that execGitCommand's mock preserves the
setupStandardExec behavior while adding only the specific overrides for
rev-parse, worktree list, branch --show-current, and status --porcelain.
Comment on lines
+35
to
+40
| // Cancel any in-flight refetches to prevent them from overwriting our optimistic update. | ||
| // Without this, a slow background refetch (e.g., from a prior create/invalidate) can | ||
| // resolve after setQueryData and overwrite the cache with stale data. | ||
| await queryClient.cancelQueries({ | ||
| queryKey: queryKeys.features.all(currentProject.path), | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts | sed -n '30,45p'Repository: AutoMaker-Org/automaker
Length of output: 930
🏁 Script executed:
cat -n apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts | sed -n '130,140p'Repository: AutoMaker-Org/automaker
Length of output: 611
🏁 Script executed:
cat -n apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts | sed -n '185,200p'Repository: AutoMaker-Org/automaker
Length of output: 846
🏁 Script executed:
find . -name "query-keys.ts" -o -name "query-keys.js" | head -5Repository: AutoMaker-Org/automaker
Length of output: 98
🌐 Web query:
TanStack Query v5 queryClient.cancelQueries exact key matching documentation
💡 Result:
In TanStack Query v5, queryClient.cancelQueries takes a QueryFilters object, which includes an exact?: boolean flag for exact key matching.
- Default behavior (inclusive / prefix matching):
queryKey: ['posts']matches['posts'],['posts', 1], etc. (unless you setexact: true). Theexactflag is documented on the Filters page. [1] - Exact matching: set
exact: trueto cancel only the query whose key is exactly the one you pass. Example from the QueryClient reference:[2]await queryClient.cancelQueries( { queryKey: ['posts'], exact: true }, { silent: true }, )
Docs: Filters (QueryFilters.exact) [1], QueryClient.cancelQueries reference [2]
Sources
[1] TanStack Query v5 React Docs — Filters: https://tanstack.com/query/v5/docs/framework/react/guides/filters
[2] TanStack Query v5 Docs — QueryClient: https://tanstack.com/query/v5/docs/reference/QueryClient
🏁 Script executed:
cat -n apps/ui/src/lib/query-keys.tsRepository: AutoMaker-Org/automaker
Length of output: 12471
Add exact: true to scope query cancellation to the features-list key only.
At lines 38, 135, and 192, cancelQueries with prefix-based key matching will cancel descendant keys including features.single(...) and features.agentOutput(...) queries, causing unnecessary retries.
Proposed patch
await queryClient.cancelQueries({
queryKey: queryKeys.features.all(currentProject.path),
+ exact: true,
});Apply to all three locations: lines 35–40, 133–137, and 190–194.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Cancel any in-flight refetches to prevent them from overwriting our optimistic update. | |
| // Without this, a slow background refetch (e.g., from a prior create/invalidate) can | |
| // resolve after setQueryData and overwrite the cache with stale data. | |
| await queryClient.cancelQueries({ | |
| queryKey: queryKeys.features.all(currentProject.path), | |
| }); | |
| // Cancel any in-flight refetches to prevent them from overwriting our optimistic update. | |
| // Without this, a slow background refetch (e.g., from a prior create/invalidate) can | |
| // resolve after setQueryData and overwrite the cache with stale data. | |
| await queryClient.cancelQueries({ | |
| queryKey: queryKeys.features.all(currentProject.path), | |
| exact: true, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts`
around lines 35 - 40, The cancelQueries calls in use-board-persistence.ts are
currently using prefix-based matching and thus cancel descendant keys like
features.single(...) and features.agentOutput(...); update each
queryClient.cancelQueries invocation that passes
queryKeys.features.all(currentProject.path) (three occurrences in this file) to
include the option exact: true so only the features-list key is cancelled (i.e.,
call cancelQueries with { queryKey: queryKeys.features.all(currentProject.path),
exact: true }).
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align="end" className="w-56"> | ||
| <DropdownMenuItem onClick={handleNewProject}> | ||
| <DropdownMenuItem onClick={handleNewProject} data-testid="quick-setup-option"> |
Contributor
There was a problem hiding this comment.
Use unique test IDs for Quick Setup to avoid ambiguous selectors.
Both locations currently use data-testid="quick-setup-option". In the hasProjects view, this can cause non-deterministic test queries.
Suggested update
- <DropdownMenuItem onClick={handleNewProject} data-testid="quick-setup-option">
+ <DropdownMenuItem onClick={handleNewProject} data-testid="quick-setup-option-mobile">
- <DropdownMenuItem onClick={handleNewProject} data-testid="quick-setup-option">
+ <DropdownMenuItem onClick={handleNewProject} data-testid="quick-setup-option-header">Also applies to: 763-763
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/dashboard-view.tsx` at line 600, The two
DropdownMenuItem elements in dashboard-view.tsx both use
data-testid="quick-setup-option", creating ambiguous selectors; update each to
unique test IDs (e.g., assign one in the hasProjects branch and one in the
no-projects/quick-setup branch) so tests can target them
deterministically—locate the DropdownMenuItem with onClick={handleNewProject}
and the other occurrence and rename their data-testid values to distinct
identifiers like "quick-setup-option-has-projects" and
"quick-setup-option-no-projects".
Comment on lines
+127
to
+131
| // Wait for sidebar to finish collapsing | ||
| await page | ||
| .locator('button:has-text("Expand sidebar")') | ||
| .waitFor({ state: 'visible', timeout: 5000 }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
Avoid swallowing collapse-state failures after clicking collapse.
At Line 131, .catch(() => {}) can hide a real sidebar-collapse failure and make downstream failures harder to diagnose.
Suggested tightening
if (await collapseSidebarButton.isVisible()) {
await collapseSidebarButton.click();
- // Wait for sidebar to finish collapsing
- await page
- .locator('button:has-text("Expand sidebar")')
- .waitFor({ state: 'visible', timeout: 5000 })
- .catch(() => {});
+ // Wait for sidebar to finish collapsing
+ await expect(page.locator('button:has-text("Expand sidebar")')).toBeVisible({
+ timeout: 5000,
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Wait for sidebar to finish collapsing | |
| await page | |
| .locator('button:has-text("Expand sidebar")') | |
| .waitFor({ state: 'visible', timeout: 5000 }) | |
| .catch(() => {}); | |
| // Wait for sidebar to finish collapsing | |
| await expect(page.locator('button:has-text("Expand sidebar")')).toBeVisible({ | |
| timeout: 5000, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/features/edit-feature.spec.ts` around lines 127 - 131, The test
currently swallows failures by using .catch(() => {}) on the wait for the
sidebar expand button (the page.locator('button:has-text("Expand
sidebar")').waitFor({...}).catch(() => {})) which hides real failures; remove
the empty catch and instead assert visibility using Playwright's expect (e.g.
await expect(page.locator('button:has-text("Expand sidebar")')).toBeVisible({
timeout: 5000 })) or, if you need custom handling, catch the error, log it with
context, and rethrow so the test fails visibly.
Comment on lines
+172
to
+180
| try { | ||
| fs.rmSync(tempDir, { recursive: true, force: true }); | ||
| } catch (err) { | ||
| const code = (err as NodeJS.ErrnoException)?.code; | ||
| if (code === 'ENOTEMPTY' || code === 'EPERM' || code === 'EBUSY') { | ||
| rmDirRecursive(tempDir); | ||
| fs.rmdirSync(tempDir); | ||
| } else { | ||
| throw err; |
Contributor
There was a problem hiding this comment.
Handle fallback ENOENT to avoid intermittent cleanup failures.
If the directory is removed between the first rmSync failure and fallback deletion, Line 177/Line 178 can throw ENOENT and fail test cleanup unnecessarily.
💡 Suggested patch
export function cleanupTempDir(tempDir: string): void {
if (!fs.existsSync(tempDir)) return;
try {
fs.rmSync(tempDir, { recursive: true, force: true });
} catch (err) {
const code = (err as NodeJS.ErrnoException)?.code;
if (code === 'ENOTEMPTY' || code === 'EPERM' || code === 'EBUSY') {
- rmDirRecursive(tempDir);
- fs.rmdirSync(tempDir);
+ try {
+ if (!fs.existsSync(tempDir)) return;
+ rmDirRecursive(tempDir);
+ fs.rmdirSync(tempDir);
+ } catch (fallbackErr) {
+ if ((fallbackErr as NodeJS.ErrnoException)?.code !== 'ENOENT') {
+ throw fallbackErr;
+ }
+ }
} else {
throw err;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| fs.rmSync(tempDir, { recursive: true, force: true }); | |
| } catch (err) { | |
| const code = (err as NodeJS.ErrnoException)?.code; | |
| if (code === 'ENOTEMPTY' || code === 'EPERM' || code === 'EBUSY') { | |
| rmDirRecursive(tempDir); | |
| fs.rmdirSync(tempDir); | |
| } else { | |
| throw err; | |
| try { | |
| fs.rmSync(tempDir, { recursive: true, force: true }); | |
| } catch (err) { | |
| const code = (err as NodeJS.ErrnoException)?.code; | |
| if (code === 'ENOTEMPTY' || code === 'EPERM' || code === 'EBUSY') { | |
| try { | |
| if (!fs.existsSync(tempDir)) return; | |
| rmDirRecursive(tempDir); | |
| fs.rmdirSync(tempDir); | |
| } catch (fallbackErr) { | |
| if ((fallbackErr as NodeJS.ErrnoException)?.code !== 'ENOENT') { | |
| throw fallbackErr; | |
| } | |
| } | |
| } else { | |
| throw err; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/utils/git/worktree.ts` around lines 172 - 180, The cleanup
catch block for fs.rmSync may hit ENOENT during the fallback and should ignore
that case to avoid intermittent failures; update the catch handling in the
try/catch around fs.rmSync(tempDir...) to treat code === 'ENOENT' as a no-op (in
addition to existing ENOTEMPTY/EPERM/EBUSY logic) before calling
rmDirRecursive(tempDir) and fs.rmdirSync(tempDir), or check for ENOENT after the
fallback rmdir and swallow it instead, referencing the same tempDir,
rmDirRecursive and fs.rmdirSync calls so the cleanup no longer throws when the
dir was already removed.
- Updated Git command execution to merge environment paths correctly, ensuring proper command execution context. - Refactored the Git initialization process to handle errors more gracefully and ensure user configuration is set before creating the initial commit. - Improved test configurations by updating Playwright test identifiers for better clarity and consistency across different project states. - Enhanced cleanup functions in tests to handle directory removal more robustly, preventing errors during test execution.
gsxdsm
added a commit
that referenced
this pull request
Mar 3, 2026
* fix(copilot): correct tool.execution_complete event handling The CopilotProvider was using incorrect event type and data structure for tool execution completion events from the @github/copilot-sdk, causing tool call outputs to be empty. Changes: - Update event type from 'tool.execution_end' to 'tool.execution_complete' - Fix data structure to use nested result.content instead of flat result - Fix error structure to use error.message instead of flat error - Add success field to match SDK event structure - Add tests for empty and missing result handling This aligns with the official @github/copilot-sdk v0.1.16 types defined in session-events.d.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(copilot): improve error handling and code quality Code review improvements: - Extract magic string '[ERROR]' to TOOL_ERROR_PREFIX constant - Add null-safe error handling with direct error variable assignment - Include error codes in error messages for better debugging - Add JSDoc documentation for tool.execution_complete handler - Update tests to verify error codes are displayed - Add missing tool_use_id assertion in error test These changes improve: - Code maintainability (no magic strings) - Debugging experience (error codes now visible) - Type safety (explicit null checks) - Test coverage (verify error code formatting) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * fix: Handle detached HEAD state in worktree discovery and recovery * fix: Remove unused isDevServerStarting prop and md: breakpoint classes * fix: Add missing dependency and sanitize persisted cache data * feat: Ensure NODE_ENV is set to test in vitest configs * feat: Configure Playwright to run only E2E tests * fix: Improve PR tracking and dev server lifecycle management * feat: Add settings-based defaults for planning mode, model config, and custom providers. Fixes #816 * feat: Add worktree and branch selector to graph view * fix: Add timeout and error handling for worktree HEAD ref resolution * fix: use absolute icon path and place icon outside asar on Linux The hicolor icon theme index only lists sizes up to 512x512, so an icon installed only at 1024x1024 is invisible to GNOME/KDE's theme resolver, causing both the app launcher and taskbar to show a generic icon. Additionally, BrowserWindow.icon cannot be read by the window manager when the file is inside app.asar. - extraResources: copy logo_larger.png to resources/ (outside asar) so it lands at /opt/Automaker/resources/logo_larger.png on install - linux.desktop.Icon: set to the absolute resources path, bypassing the hicolor theme lookup and its size constraints entirely - icon-manager.ts: on Linux production use process.resourcesPath so BrowserWindow receives a real filesystem path the WM can read directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: use linux.desktop.entry for custom desktop Icon field electron-builder v26 rejects arbitrary keys in linux.desktop — the correct schema wraps custom .desktop overrides inside desktop.entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: set desktop name on Linux so taskbar uses the correct app icon Without app.setDesktopName(), the window manager cannot associate the running Electron process with automaker.desktop. GNOME/KDE fall back to _NET_WM_ICON which defaults to Electron's own bundled icon. Calling app.setDesktopName('automaker.desktop') before any window is created sets the _GTK_APPLICATION_ID hint and XDG app_id so the WM picks up the desktop entry's Icon for the taskbar. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix: memory and context views mobile friendly (#818) * Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR #813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Improve test reliability and localhost handling * chore: Use explicit TEST_USE_EXTERNAL_BACKEND env var for server cleanup * feat: Add E2E/CI mock mode for provider factory and auth verification * feat: Add remoteBranch parameter to pull and rebase operations * chore: Enhance E2E testing setup with worker isolation and auth state management - Updated .gitignore to include worker-specific test fixtures. - Modified e2e-tests.yml to implement test sharding for improved CI performance. - Refactored global setup to authenticate once and save session state for reuse across tests. - Introduced worker-isolated fixture paths to prevent conflicts during parallel test execution. - Improved test navigation and loading handling for better reliability. - Updated various test files to utilize new auth state management and fixture paths. * fix: Update Playwright configuration and improve test reliability - Increased the number of workers in Playwright configuration for better parallelism in CI environments. - Enhanced the board background persistence test to ensure dropdown stability by waiting for the list to populate before interaction, improving test reliability. * chore: Simplify E2E test configuration and enhance mock implementations - Updated e2e-tests.yml to run tests in a single shard for streamlined CI execution. - Enhanced unit tests for worktree list handling by introducing a mock for execGitCommand, improving test reliability and coverage. - Refactored setup functions to better manage command mocks for git operations in tests. - Improved error handling in mkdirSafe function to account for undefined stats in certain environments. * refactor: Improve test configurations and enhance error handling - Updated Playwright configuration to clear VITE_SERVER_URL, ensuring the frontend uses the Vite proxy and preventing cookie domain mismatches. - Enhanced MergeRebaseDialog logic to normalize selectedBranch for better handling of various ref formats. - Improved global setup with a more robust backend health check, throwing an error if the backend is not healthy after retries. - Refactored project creation tests to handle file existence checks more reliably. - Added error handling for missing E2E source fixtures to guide setup process. - Enhanced memory navigation to handle sandbox dialog visibility more effectively. * refactor: Enhance Git command execution and improve test configurations - Updated Git command execution to merge environment paths correctly, ensuring proper command execution context. - Refactored the Git initialization process to handle errors more gracefully and ensure user configuration is set before creating the initial commit. - Improved test configurations by updating Playwright test identifiers for better clarity and consistency across different project states. - Enhanced cleanup functions in tests to handle directory removal more robustly, preventing errors during test execution. * fix: Resolve React hooks errors from duplicate instances in dependency tree * style: Format alias configuration for improved readability --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: DhanushSantosh <dhanushsantoshs05@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch contains a collection of bug fixes and stability improvements targeting issues found in v1.0.0rc.
Bug Fixes
tool.execution_completeevent handling (wastool.execution_end), update error/result payload shapes to match current SDK format, add[ERROR]prefix constant for consistent error formattingrebase-merge/head-nameandrebase-apply/head-namegit internalsrefs/heads/,refs/remotes/,origin/prefixes) so branch matching is consistent regardless of which git command produced the nameEnhancements
dev-servers.jsonso running servers survive application restarts; add "starting" state to prevent race conditions on concurrent start requests; re-attach to running processes on startup by checking port availabilityresolveProviderContext()for provider + model resolution; supports explicit provider ID lookup with fallback to model-based search across enabled providers; handlesmapsToClaudeModelfor Claude-compatible third-party providersresolveProviderContext()instead of ad-hoc provider lookup; properly pass resolved (mapped) model to SDK; validate provider context before executionuseAgentOutputWebSocket()hook to centralize streaming logic previously duplicated inAgentOutputModalUI: Merge Conflict Handling
merge_conflictstatusmerge_conflictstatus (previously only shown for running features)Tests
parse-and-create-featuresdefaults, copilot provider edge cases, execution service, worktree resolver branch normalizationSummary by CodeRabbit
New Features
Bug Fixes
Tests