Add orphaned features management routes and UI integration#819
Merged
gsxdsm merged 6 commits intoAutoMaker-Org:v1.0.0rcfrom Feb 28, 2026
Merged
Add orphaned features management routes and UI integration#819gsxdsm merged 6 commits intoAutoMaker-Org:v1.0.0rcfrom
gsxdsm merged 6 commits intoAutoMaker-Org:v1.0.0rcfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…roject initialization - Updated detectOrphanedFeatures method to accept preloaded features, reducing redundant disk reads. - Improved project initialization by creating required directories and files in parallel for better performance. - Adjusted planning mode handling in UI components to clarify approval requirements for different modes. - Added refresh functionality for file editor tabs to ensure content consistency with disk state. These changes enhance performance, maintainability, and user experience across the application.
…UI integration - Introduced new routes for managing orphaned features, including listing, resolving, and bulk resolving. - Updated the UI to include an Orphaned Features section in project settings and navigation. - Enhanced the execution service to support new orphaned feature functionalities. These changes improve the application's capability to handle orphaned features effectively, enhancing user experience and project management.
Contributor
📝 WalkthroughWalkthroughAdds orphaned-features detection and resolution end‑points and UI, updates auto-mode to accept preloaded features, normalizes editor line endings with tab refresh logic, refactors several UI dialogs and modal constants, and adds related tests and utilities. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as OrphanedFeaturesSection
participant Client as HttpApiClient
participant Server as Server API
participant Service as AutoModeService
participant Store as FeatureLoader
User->>UI: Click "Scan for Orphaned"
activate UI
UI->>Client: getOrphaned(projectPath)
activate Client
Client->>Server: POST /orphaned
activate Server
Server->>Service: detectOrphanedFeatures(projectPath, features)
activate Service
Service->>Store: getAll() (if features not preloaded)
activate Store
Store-->>Service: features[]
deactivate Store
Service->>Service: compareWithGitBranches()
Service-->>Server: orphanedFeatures[]
deactivate Service
Server-->>Client: { orphanedFeatures }
deactivate Server
Client-->>UI: orphanedFeatures
deactivate Client
UI->>UI: Render list
deactivate UI
User->>UI: Select features + choose action
activate UI
UI->>Client: bulkResolveOrphaned(projectPath, ids, action, target)
activate Client
Client->>Server: POST /orphaned/bulk-resolve
activate Server
Server->>Store: fetch/update/delete features (seq/parallel depending on action)
activate Store
Store-->>Server: per-feature results
deactivate Store
Server-->>Client: aggregated results
deactivate Server
Client-->>UI: results
deactivate Client
UI->>User: Show outcome/toast
deactivate UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 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, 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 application's feature management capabilities by introducing robust tools for handling 'orphaned' features—those without corresponding Git branches. It also refines the planning workflow with feature-specific prompts and improves the reliability and responsiveness of several core UI components, including the file editor's dirty state detection and the agent output modal's layout. Additionally, project initialization and server connectivity checks have been made more resilient. 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 pull request introduces a comprehensive feature for managing orphaned features, including new API endpoints and a dedicated UI section in project settings. The implementation is robust, with thoughtful considerations for performance (pre-loading features) and reliability (sequential git operations). The PR also includes several valuable improvements to planning prompt logic, UI responsiveness, network resilience, and editor state management. The code is well-structured and includes new tests. I've left a few minor suggestions to improve logging and remove an unused parameter.
Comment on lines
+21
to
+24
| export function createOrphanedListHandler( | ||
| featureLoader: FeatureLoader, | ||
| autoModeService?: AutoModeServiceCompat | ||
| ) { |
Contributor
There was a problem hiding this comment.
The featureLoader parameter is injected into createOrphanedListHandler but it's not used within the function. It can be removed to simplify the function signature. You'll also need to update the call site in apps/server/src/routes/features/index.ts.
export function createOrphanedListHandler(
autoModeService?: AutoModeServiceCompat
) {
Comment on lines
+126
to
+128
| } catch { | ||
| // Non-fatal | ||
| } |
Contributor
There was a problem hiding this comment.
This catch block is empty. While the failure to delete worktree metadata is non-fatal, logging the error as a warning would be helpful for debugging potential issues, such as file permission problems, without interrupting the feature deletion process.
} catch (error) {
// Non-fatal, but log for debugging
logger.warn(`Failed to delete worktree metadata for branch "${missingBranch}" during orphan deletion:`, error);
}
Comment on lines
+185
to
+187
| } catch { | ||
| // Non-fatal | ||
| } |
Contributor
There was a problem hiding this comment.
This catch block is also empty. Similar to the other case, logging a warning here would improve debuggability if metadata cleanup fails unexpectedly.
} catch (error) {
// Non-fatal, but log for debugging
logger.warn(`Failed to clean up old worktree metadata for branch "${missingBranch}" during move:`, error);
}![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: 9
🧹 Nitpick comments (4)
apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsx (1)
91-108: Consider adding user feedback for initialization failures.The fire-and-forget pattern with
startTransitionprovides excellent UX for instant project switching. However, ifinitializeProjectfails, the user continues unaware that the.automakerdirectory may be incomplete or missing.Consider surfacing initialization failures via a toast notification so users can take corrective action (e.g., retry or check permissions).
💡 Optional: Add toast notification for failures
+import { toast } from 'sonner'; // or your toast library const setCurrentProjectWithInit = useCallback( (p: Project) => { if (p.id === currentProject?.id) { return; } // Fire-and-forget: initialize .automaker directory structure in background // so the project switch is not blocked by filesystem operations initializeProject(p.path).catch((error) => { console.error('Failed to initialize project during switch:', error); + toast.error('Failed to initialize project structure', { + description: 'Some features may not work correctly until re-initialized.', + }); }); // Switch project immediately for instant UI response startTransition(() => { setCurrentProject(p); }); }, [currentProject?.id, setCurrentProject] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsx` around lines 91 - 108, The catch block in setCurrentProjectWithInit currently only logs errors; update it to surface failures to the user by invoking the app's toast/notification API (e.g., useToast/toast/addNotification) inside the initializeProject(p.path).catch handler so the user sees a clear message like "Failed to initialize project .automaker" with the error details, and optionally include a CTA to retry that calls initializeProject(p.path) again; keep the fire-and-forget + startTransition behavior (do not await initializeProject) and reference setCurrentProjectWithInit, initializeProject, startTransition and currentProject?.id when making the change.apps/ui/tests/features/success-log-contrast.spec.ts (1)
135-205: Extract repeated modal-open flow into a helperAll three tests repeat the same open/reload/modal steps. A small helper will reduce drift and make future selector/class updates safer.
Refactor sketch
+async function openParsedLogsForFeature(page: Page, featureId: string): Promise<Locator> { + await page.reload(); + await waitForNetworkIdle(page); + + const featureCard = page.locator(`[data-feature-id="${featureId}"]`); + await expect(featureCard).toBeVisible({ timeout: TIMEOUTS.medium }); + await featureCard.click(); + + await page.waitForSelector('[data-testid="agent-output-modal"]', { timeout: TIMEOUTS.default }); + await page.click('[data-testid="view-mode-parsed"]'); + await page.waitForSelector('[role="log"]', { timeout: TIMEOUTS.default }); + + return page.locator('[role="log"]'); +}Also applies to: 207-258, 260-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/success-log-contrast.spec.ts` around lines 135 - 205, Tests repeat the same "create feature + open agent output modal + switch to parsed logs" flow; extract that sequence into a helper (e.g., openAgentOutputModal or openFeatureAgentModal) that accepts the Playwright Page and testFeatureId (and optionally projectPath/mockOutput metadata), performs createTestFeature(...), page.reload(), waitForNetworkIdle(page), finds `[data-feature-id="${testFeatureId}"]`, clicks it, waits for `[data-testid="agent-output-modal"]`, clicks `[data-testid="view-mode-parsed"]`, waits for '[role="log"]', and returns the log container locator so tests can assert on classes; replace the repeated blocks in the current test 'should display success log output with improved contrast' and the other two tests (around the other ranges) to call the helper and assert on the returned locator instead of duplicating the flow.apps/server/src/routes/features/routes/orphaned.ts (1)
143-146: Use a collision-resistant worktree folder naming strategy.Current sanitization can map distinct branch names to the same worktree path.
Suggested refactor
+import { createHash } from 'crypto'; @@ - const sanitizedName = missingBranch.replace(/[^a-zA-Z0-9_-]/g, '-'); + const sanitizedBase = missingBranch.replace(/[^a-zA-Z0-9_-]/g, '-').slice(0, 64); + const branchHash = createHash('sha1').update(missingBranch).digest('hex').slice(0, 8); + const sanitizedName = `${sanitizedBase}-${branchHash}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/features/routes/orphaned.ts` around lines 143 - 146, Sanitization of missingBranch into sanitizedName can produce collisions; change worktree naming to include a short hash of missingBranch to make it collision-resistant: compute a stable hash (e.g., crypto.createHash('sha1').update(missingBranch).digest('hex').slice(0,8')) and append it to sanitizedName (e.g., `${sanitizedName}-${hash}`) before joining with projectPath/.worktrees to produce worktreePath; keep using sanitizedName for the human-readable prefix and the hash for uniqueness, so update the logic around sanitizedName, worktreesDir and worktreePath to build the new folder name.apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts (1)
15-407: Extract repeated “open modal” flow into a helper.The same setup/navigation/modal-open sequence is duplicated across most tests. Pulling this into a shared helper (or
beforeEach+ parameterized utility) will reduce drift and simplify future modal-flow changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts` around lines 15 - 407, Many tests repeat the same "open agent output modal" steps; create a helper (e.g., openAgentOutputModal or setupAndOpenAgentModal) that encapsulates setupRealProject(page, projectPath, projectName), page.goto('/board'), clicking '[data-testid="add-feature-button"]', filling '[data-testid="feature-input"]' and clicking '[data-testid="confirm-add-feature"]', then awaiting waitForAgentOutputModal(page); replace the duplicated sequences in each test with calls to that helper (or call it from a shared beforeEach and pass the feature text/project identifiers as params).
🤖 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/features/routes/orphaned.ts`:
- Around line 172-179: Before calling featureLoader.update in the
'move-to-branch' case, verify that targetBranch (when provided) actually exists
in the repository/worktree for projectPath; if targetBranch is null allow
clearing to main, but if it's a non-empty string, query the repo (or branch
listing API used elsewhere in this module) to confirm existence and return/throw
an error when the branch is missing so the update is not applied. Use the same
projectPath and featureId context, and only call featureLoader.update({
branchName: newBranch, status: 'pending' }) after the existence check succeeds.
In `@apps/server/src/services/execution-service.ts`:
- Around line 286-292: Ensure planningPrefix and the feature block are always
separated by an explicit separator when constructing prompt: in the block where
getPlanningPromptPrefixFn(feature) yields planningPrefix and you set prompt =
planningPrefix + this.buildFeatureDescription(feature), change it to append a
guaranteed separator (e.g., "\n\n" or a distinct delimiter) between
planningPrefix and this.buildFeatureDescription(feature) so the planning
instructions cannot be merged with the feature description; update code in the
execution-service.ts area that sets prompt to use that separator consistently.
In
`@apps/ui/src/components/views/project-settings-view/orphaned-features-section.tsx`:
- Around line 212-215: The forEach callback currently returns the boolean from
Set.delete (causing the Biome error); update both occurrences (inside the
setSelectedIds updater and the other block around lines ~241-244) so the
callback returns void—e.g., replace the inline expression callback with a block
that calls next.delete(id) without returning, or use a for...of loop over
resolvedIds to call next.delete(id); keep the rest of the setSelectedIds updater
logic unchanged.
- Around line 347-356: The select-all UI nests an interactive Checkbox inside a
clickable <button>, causing double events and accessibility issues; update the
rendering so interactive controls are not nested: remove the Checkbox from
inside the button and instead render a single interactive control (either make
the outer element a non-button wrapper and keep the Checkbox as the clickable
element, or keep the button and replace the inner Checkbox with a
non-interactive icon state), and wire selection toggling only via
toggleSelectAll (or the Checkbox's onCheckedChange) while using the allSelected
and someSelected booleans to decide the visual state; reference toggleSelectAll,
allSelected, someSelected, and the Checkbox/button elements when making the
change.
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Around line 301-309: The session key parsing in use-auto-mode.ts incorrectly
uses sessionKey.split(SESSION_KEY_DELIMITER, 2) which fails when projectPath
contains the delimiter; update the logic that derives
parts/keyProjectPath/keyBranchName to split from the right using lastIndexOf on
SESSION_KEY_DELIMITER (i.e., find the last delimiter index on sessionKey, slice
the left side as keyProjectPath and the right side as keyBranchName) and keep
the existing checks (warn on malformed key and compare keyProjectPath to
projectPath) so branch extraction is stable.
In `@apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts`:
- Around line 404-405: The locator for contentArea currently uses an unscoped
class selector '.flex-1' (see the contentArea variable/locator) which can match
unrelated containers; update the locator to scope it to the agent output modal
by selecting within the modal element (e.g.,
page.locator('[data-testid="agent-output-modal"] .flex-1') or better yet use a
dedicated content test id like
page.locator('[data-testid="agent-output-modal-content"]')) and keep the await
expect(...).toHaveText(/Agent Output/) assertion against that scoped locator.
- Around line 57-66: The assertions using window.getComputedStyle in
modal.evaluate (e.g., reading maxWidth) are comparing CSS tokens like
"calc(...)" or "60vw" but getComputedStyle returns resolved pixel values;
replace these with numeric viewport-relative checks by using
elementHandle.boundingBox() to get numeric width/height and compare against
expected viewport fractions (or parse the pixel string to number before
asserting), and adjust tests that used regexes against "60vw"/"90vw"/"80vh" to
numeric bounds; also fix invalid Playwright selector page.locator('role="log"')
to page.getByRole('log') or page.locator('[role="log"]'), and scope generic
selectors like '.flex-1' to the modal context (use the modal locator variable,
e.g., modal.locator('.flex-1')) to avoid matching unrelated elements.
- Line 374: The Playwright role selector usage is invalid: replace the incorrect
call to page.locator('role="log"') in the test (the locator check that expects
the log to be visible) with a proper role-aware selector—use
page.getByRole('log') (recommended) or, if you prefer locator syntax,
page.locator('[role="log"]')—so update the expectation that references
page.locator('role="log"') to use one of these valid selectors.
In `@apps/ui/tests/features/success-log-contrast.spec.ts`:
- Around line 36-39: The test currently interpolates error objects (e.g., in the
try/catch around fs.mkdirSync with featureDir) into template strings which can
yield “[object Object]”; update each catch to preserve structured error details
by including the error message and a safe serialization fallback (for example
use error.message and, if available, util.inspect(error) or
JSON.stringify(error, Object.getOwnPropertyNames(error))) in the thrown Error
text so failures show real diagnostic fields; apply the same change to the other
catch blocks that wrap filesystem ops (the catch blocks for mkdirSync,
writeFileSync, and similar file operations referenced in the diff).
---
Nitpick comments:
In `@apps/server/src/routes/features/routes/orphaned.ts`:
- Around line 143-146: Sanitization of missingBranch into sanitizedName can
produce collisions; change worktree naming to include a short hash of
missingBranch to make it collision-resistant: compute a stable hash (e.g.,
crypto.createHash('sha1').update(missingBranch).digest('hex').slice(0,8')) and
append it to sanitizedName (e.g., `${sanitizedName}-${hash}`) before joining
with projectPath/.worktrees to produce worktreePath; keep using sanitizedName
for the human-readable prefix and the hash for uniqueness, so update the logic
around sanitizedName, worktreesDir and worktreePath to build the new folder
name.
In
`@apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsx`:
- Around line 91-108: The catch block in setCurrentProjectWithInit currently
only logs errors; update it to surface failures to the user by invoking the
app's toast/notification API (e.g., useToast/toast/addNotification) inside the
initializeProject(p.path).catch handler so the user sees a clear message like
"Failed to initialize project .automaker" with the error details, and optionally
include a CTA to retry that calls initializeProject(p.path) again; keep the
fire-and-forget + startTransition behavior (do not await initializeProject) and
reference setCurrentProjectWithInit, initializeProject, startTransition and
currentProject?.id when making the change.
In `@apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts`:
- Around line 15-407: Many tests repeat the same "open agent output modal"
steps; create a helper (e.g., openAgentOutputModal or setupAndOpenAgentModal)
that encapsulates setupRealProject(page, projectPath, projectName),
page.goto('/board'), clicking '[data-testid="add-feature-button"]', filling
'[data-testid="feature-input"]' and clicking
'[data-testid="confirm-add-feature"]', then awaiting
waitForAgentOutputModal(page); replace the duplicated sequences in each test
with calls to that helper (or call it from a shared beforeEach and pass the
feature text/project identifiers as params).
In `@apps/ui/tests/features/success-log-contrast.spec.ts`:
- Around line 135-205: Tests repeat the same "create feature + open agent output
modal + switch to parsed logs" flow; extract that sequence into a helper (e.g.,
openAgentOutputModal or openFeatureAgentModal) that accepts the Playwright Page
and testFeatureId (and optionally projectPath/mockOutput metadata), performs
createTestFeature(...), page.reload(), waitForNetworkIdle(page), finds
`[data-feature-id="${testFeatureId}"]`, clicks it, waits for
`[data-testid="agent-output-modal"]`, clicks `[data-testid="view-mode-parsed"]`,
waits for '[role="log"]', and returns the log container locator so tests can
assert on classes; replace the repeated blocks in the current test 'should
display success log output with improved contrast' and the other two tests
(around the other ranges) to call the helper and assert on the returned locator
instead of duplicating the flow.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
apps/server/src/routes/features/index.tsapps/server/src/routes/features/routes/list.tsapps/server/src/routes/features/routes/orphaned.tsapps/server/src/services/auto-mode/compat.tsapps/server/src/services/auto-mode/facade.tsapps/server/src/services/execution-service.tsapps/server/tests/unit/lib/file-editor-store-logic.test.tsapps/server/tests/unit/services/execution-service.test.tsapps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsxapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/mass-edit-dialog.tsxapps/ui/src/components/views/board-view/shared/planning-mode-select.tsxapps/ui/src/components/views/file-editor-view/components/editor-tabs.tsxapps/ui/src/components/views/file-editor-view/file-editor-dirty-utils.tsapps/ui/src/components/views/file-editor-view/file-editor-view.tsxapps/ui/src/components/views/file-editor-view/use-file-editor-store.tsapps/ui/src/components/views/project-settings-view/config/navigation.tsapps/ui/src/components/views/project-settings-view/hooks/use-project-settings-view.tsapps/ui/src/components/views/project-settings-view/orphaned-features-section.tsxapps/ui/src/components/views/project-settings-view/project-settings-view.tsxapps/ui/src/hooks/use-auto-mode.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/project-init.tsapps/ui/src/lib/query-client.tsapps/ui/tests/features/responsive/agent-output-modal-responsive.spec.tsapps/ui/tests/features/success-log-contrast.spec.tsapps/ui/tests/unit/components/phase-model-selector.test.tsx
apps/ui/src/components/views/project-settings-view/orphaned-features-section.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/project-settings-view/orphaned-features-section.tsx
Show resolved
Hide resolved
apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts
Outdated
Show resolved
Hide resolved
apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts
Outdated
Show resolved
Hide resolved
apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+36
to
+39
| try { | ||
| fs.mkdirSync(featureDir, { recursive: true }); | ||
| } catch (error) { | ||
| throw new Error(`Failed to create feature directory at ${featureDir}: ${error}`); |
Contributor
There was a problem hiding this comment.
Preserve structured filesystem error details in messages
Directly interpolating error can collapse useful fields into [object Object], which hurts failure triage in CI logs.
Proposed patch
+function formatFsError(error: unknown): string {
+ if (error instanceof Error) {
+ const code = (error as NodeJS.ErrnoException).code;
+ return code ? `${error.name} (${code}): ${error.message}` : `${error.name}: ${error.message}`;
+ }
+ try {
+ return JSON.stringify(error);
+ } catch {
+ return String(error);
+ }
+}
+
function createTestFeature(
projectPath: string,
featureId: string,
@@
try {
fs.mkdirSync(featureDir, { recursive: true });
} catch (error) {
- throw new Error(`Failed to create feature directory at ${featureDir}: ${error}`);
+ throw new Error(`Failed to create feature directory at ${featureDir}: ${formatFsError(error)}`);
}
@@
try {
fs.writeFileSync(outputPath, outputContent, { encoding: 'utf-8' });
} catch (error) {
- throw new Error(`Failed to write agent output to ${outputPath}: ${error}`);
+ throw new Error(`Failed to write agent output to ${outputPath}: ${formatFsError(error)}`);
}
@@
try {
fs.writeFileSync(featureJsonPath, JSON.stringify(featureData, null, 2), { encoding: 'utf-8' });
} catch (error) {
- throw new Error(`Failed to write feature.json to ${featureJsonPath}: ${error}`);
+ throw new Error(`Failed to write feature.json to ${featureJsonPath}: ${formatFsError(error)}`);
}
}
@@
try {
fs.rmSync(dirPath, { recursive: true, force: true });
} catch (error) {
- console.warn(`Failed to clean up directory ${dirPath}: ${error}`);
+ console.warn(`Failed to clean up directory ${dirPath}: ${formatFsError(error)}`);
}
}
}Also applies to: 44-48, 59-63, 75-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/features/success-log-contrast.spec.ts` around lines 36 - 39,
The test currently interpolates error objects (e.g., in the try/catch around
fs.mkdirSync with featureDir) into template strings which can yield “[object
Object]”; update each catch to preserve structured error details by including
the error message and a safe serialization fallback (for example use
error.message and, if available, util.inspect(error) or JSON.stringify(error,
Object.getOwnPropertyNames(error))) in the thrown Error text so failures show
real diagnostic fields; apply the same change to the other catch blocks that
wrap filesystem ops (the catch blocks for mkdirSync, writeFileSync, and similar
file operations referenced in the diff).
- Added a blank line in .gitignore for better readability. - Introduced a hash to worktree paths in orphaned feature resolution to prevent conflicts. - Added validation for target branch existence during orphaned feature resolution. - Improved prompt formatting in execution service for clarity. - Enhanced error handling in project selector for project initialization failures. - Refactored orphaned features section to improve state management and UI responsiveness. These changes improve code maintainability and user experience when managing orphaned features.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/ui/src/components/views/project-settings-view/orphaned-features-section.tsx (1)
570-578: Inconsistent dialog dismissal between single and bulk delete.For single deletion, the dialog closes immediately (
setDeleteConfirm(null)on line 574) beforeresolveOrphancompletes. For bulk deletion, the dialog remains open during the operation (closed inbulkResolve'sfinallyblock). This inconsistency could confuse users if single deletion fails after the dialog closes.Consider keeping the dialog open until the operation completes for both cases:
♻️ Suggested fix
onClick={() => { if (deleteConfirm) { if (deleteConfirm.featureIds.length === 1) { - resolveOrphan(deleteConfirm.featureIds[0], 'delete'); - setDeleteConfirm(null); + resolveOrphan(deleteConfirm.featureIds[0], 'delete').finally(() => { + setDeleteConfirm(null); + }); } else { bulkResolve(deleteConfirm.featureIds, 'delete'); } } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/orphaned-features-section.tsx` around lines 570 - 578, The single-delete path closes the dialog immediately while the bulk path waits for operation completion; change the single-delete flow in the onClick handler so it mirrors bulkResolve: call resolveOrphan and only clear setDeleteConfirm(null) after the operation finishes (i.e., await resolveOrphan or move the dialog dismissal into resolveOrphan’s/finally logic), ensuring the dialog stays open until resolveOrphan completes or fails. Reference: deleteConfirm, resolveOrphan, bulkResolve, setDeleteConfirm, and the onClick handler.apps/server/src/routes/features/routes/orphaned.ts (1)
22-25: UnusedfeatureLoaderparameter.The
featureLoaderparameter is declared but never used increateOrphanedListHandler. Consider removing it to avoid confusion, or add a leading underscore (_featureLoader) to indicate it's intentionally unused (similar to_autoModeServiceincreateOrphanedResolveHandler).♻️ Suggested fix
export function createOrphanedListHandler( - featureLoader: FeatureLoader, + _featureLoader: FeatureLoader, autoModeService?: AutoModeServiceCompat ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/features/routes/orphaned.ts` around lines 22 - 25, The createOrphanedListHandler function declares a featureLoader parameter that is never used; either remove featureLoader from createOrphanedListHandler's signature (and update any call sites) or rename it to _featureLoader to signal it's intentionally unused (matching the existing _autoModeService naming pattern in createOrphanedResolveHandler); update the function signature and all invocations accordingly so TypeScript no longer reports an unused parameter.apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts (2)
120-283: Consider extracting reusable viewport assertion helpers.The resize + width/height assertion pattern is repeated heavily. A small helper for “set viewport + assert modal dimensions” would reduce duplication and future maintenance cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts` around lines 120 - 283, Extract the repeated viewport/responsive assertions into helpers to avoid duplication: add utility functions (e.g. setViewportAndAssertModalWidth(page, width, expectedRatio, tolerance), setViewportAndAssertModalClassContains(page, width, className), and setViewportAndAssertModalMaxHeight(page, width, expectedVh, tolerance)) and use them in tests that currently call setupAndOpenModal, page.setViewportSize, page.waitForTimeout, and then evaluate the modal locator '[data-testid="agent-output-modal"]' for offsetWidth/offsetHeight or className; replace the repeated blocks in the Mobile/Small/Tablet/Responsive Transitions tests with calls to these helpers to centralize resizing, waiting, and assertions.
90-118: SimplifysetupAndOpenModalreturn type.
setupAndOpenModal()returnsfeatureId, but current callers ignore it. Consider returningPromise<void>(or use the returned id) to keep intent clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts` around lines 90 - 118, The helper setupAndOpenModal currently returns the generated featureId but callers ignore it; change its signature from Promise<string> to Promise<void> and remove the final return of featureId (or alternatively update callers to use the returned id if they need it). Specifically update the async function setupAndOpenModal(page: Page) to return void, eliminate the "return featureId;" line, and adjust any tests that call setupAndOpenModal() (they should no longer expect a string) or explicitly call a separate helper to obtain featureId if needed.
🤖 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/ui/tests/features/success-log-contrast.spec.ts`:
- Around line 234-238: The test currently makes the badge assertion optional by
wrapping the check in an if guard using filterSection.isVisible(...).catch(() =>
false); to enforce the requirement, remove the conditional guard and directly
assert the badge visibility (call expect on filterSection.toBeVisible()
unconditionally) so the "success" badge is required for the test to pass; locate
the filterSection variable created with
modal.locator('button:has-text("success")') and replace the conditional
visibility gate with a direct expectation to make the assertion strict.
- Line 227: The assertion uses a non-deterministic locator
modal.locator('text=Summary') which can match multiple elements; change it to
disambiguate like the other tests by calling .first() on the locator (e.g.,
modal.locator('text=Summary').first()) so the test consistently targets the
content preview element; update the assertion that uses
modal.locator('text=Summary') at line 227 to use .first() and keep the same
timeout option.
---
Nitpick comments:
In `@apps/server/src/routes/features/routes/orphaned.ts`:
- Around line 22-25: The createOrphanedListHandler function declares a
featureLoader parameter that is never used; either remove featureLoader from
createOrphanedListHandler's signature (and update any call sites) or rename it
to _featureLoader to signal it's intentionally unused (matching the existing
_autoModeService naming pattern in createOrphanedResolveHandler); update the
function signature and all invocations accordingly so TypeScript no longer
reports an unused parameter.
In
`@apps/ui/src/components/views/project-settings-view/orphaned-features-section.tsx`:
- Around line 570-578: The single-delete path closes the dialog immediately
while the bulk path waits for operation completion; change the single-delete
flow in the onClick handler so it mirrors bulkResolve: call resolveOrphan and
only clear setDeleteConfirm(null) after the operation finishes (i.e., await
resolveOrphan or move the dialog dismissal into resolveOrphan’s/finally logic),
ensuring the dialog stays open until resolveOrphan completes or fails.
Reference: deleteConfirm, resolveOrphan, bulkResolve, setDeleteConfirm, and the
onClick handler.
In `@apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts`:
- Around line 120-283: Extract the repeated viewport/responsive assertions into
helpers to avoid duplication: add utility functions (e.g.
setViewportAndAssertModalWidth(page, width, expectedRatio, tolerance),
setViewportAndAssertModalClassContains(page, width, className), and
setViewportAndAssertModalMaxHeight(page, width, expectedVh, tolerance)) and use
them in tests that currently call setupAndOpenModal, page.setViewportSize,
page.waitForTimeout, and then evaluate the modal locator
'[data-testid="agent-output-modal"]' for offsetWidth/offsetHeight or className;
replace the repeated blocks in the Mobile/Small/Tablet/Responsive Transitions
tests with calls to these helpers to centralize resizing, waiting, and
assertions.
- Around line 90-118: The helper setupAndOpenModal currently returns the
generated featureId but callers ignore it; change its signature from
Promise<string> to Promise<void> and remove the final return of featureId (or
alternatively update callers to use the returned id if they need it).
Specifically update the async function setupAndOpenModal(page: Page) to return
void, eliminate the "return featureId;" line, and adjust any tests that call
setupAndOpenModal() (they should no longer expect a string) or explicitly call a
separate helper to obtain featureId if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignoreapps/server/src/routes/features/routes/orphaned.tsapps/server/src/services/execution-service.tsapps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsxapps/ui/src/components/views/project-settings-view/orphaned-features-section.tsxapps/ui/src/hooks/use-auto-mode.tsapps/ui/tests/features/responsive/agent-output-modal-responsive.spec.tsapps/ui/tests/features/success-log-contrast.spec.tsapps/ui/tests/global-setup.tsapps/ui/tests/global-teardown.tsapps/ui/tests/utils/cleanup-test-dirs.tsapps/ui/tests/utils/components/responsive-modal.tsapps/ui/tests/utils/helpers/temp-dir.ts
💤 Files with no reviewable changes (2)
- apps/ui/tests/utils/helpers/temp-dir.ts
- apps/ui/tests/utils/components/responsive-modal.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsx
- apps/server/src/services/execution-service.ts
| const modal = page.locator('[data-testid="agent-output-modal"]'); | ||
|
|
||
| // Verify the parsed log view shows content | ||
| await expect(modal.locator('text=Summary')).toBeVisible({ timeout: 5000 }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the area around line 227
cat -n apps/ui/tests/features/success-log-contrast.spec.ts | sed -n '220,240p'Repository: AutoMaker-Org/automaker
Length of output: 1095
🏁 Script executed:
# Also run the suggested verification script to find all Summary locator usages
rg -n "locator\('text=Summary'\)" apps/ui/tests/features/success-log-contrast.spec.ts -C2Repository: AutoMaker-Org/automaker
Length of output: 866
🏁 Script executed:
# Let's also check for any variant patterns like Summary with .first()
rg -n "Summary" apps/ui/tests/features/success-log-contrast.spec.ts -C1Repository: AutoMaker-Org/automaker
Length of output: 1012
🏁 Script executed:
# Check the imports at the top of the file for compliance with coding guidelines
head -20 apps/ui/tests/features/success-log-contrast.spec.tsRepository: AutoMaker-Org/automaker
Length of output: 564
Use a deterministic locator for repeated Summary text at line 227.
Line 227 is inconsistent with earlier tests (lines 157 and 195) that already disambiguate duplicated Summary matches using .first(). The file's own comments explicitly explain: "Use .first() because 'Summary' appears in both the badge and the content preview." Without this disambiguation, the assertion can fail depending on DOM structure.
Proposed fix
- await expect(modal.locator('text=Summary')).toBeVisible({ timeout: 5000 });
+ await expect(modal.locator('text=Summary').first()).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
| await expect(modal.locator('text=Summary')).toBeVisible({ timeout: 5000 }); | |
| await expect(modal.locator('text=Summary').first()).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/success-log-contrast.spec.ts` at line 227, The
assertion uses a non-deterministic locator modal.locator('text=Summary') which
can match multiple elements; change it to disambiguate like the other tests by
calling .first() on the locator (e.g., modal.locator('text=Summary').first()) so
the test consistently targets the content preview element; update the assertion
that uses modal.locator('text=Summary') at line 227 to use .first() and keep the
same timeout option.
Comment on lines
+234
to
+238
| const filterSection = modal.locator('button:has-text("success")'); | ||
| if (await filterSection.isVisible({ timeout: 2000 }).catch(() => false)) { | ||
| // Success filter badge is present, indicating logs were categorized correctly | ||
| await expect(filterSection).toBeVisible(); | ||
| } |
Contributor
There was a problem hiding this comment.
The badge assertion is optional, so this test can pass without validating the badge.
The current if (isVisible) guard makes the badge check non-enforcing. For a test named “should have consistent badge styling…”, this weakens coverage.
Suggested tightening
- const filterSection = modal.locator('button:has-text("success")');
- if (await filterSection.isVisible({ timeout: 2000 }).catch(() => false)) {
- // Success filter badge is present, indicating logs were categorized correctly
- await expect(filterSection).toBeVisible();
- }
+ const filterSection = modal.locator('button:has-text("success")');
+ await expect(filterSection).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
| const filterSection = modal.locator('button:has-text("success")'); | |
| if (await filterSection.isVisible({ timeout: 2000 }).catch(() => false)) { | |
| // Success filter badge is present, indicating logs were categorized correctly | |
| await expect(filterSection).toBeVisible(); | |
| } | |
| const filterSection = modal.locator('button:has-text("success")'); | |
| await expect(filterSection).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/success-log-contrast.spec.ts` around lines 234 - 238,
The test currently makes the badge assertion optional by wrapping the check in
an if guard using filterSection.isVisible(...).catch(() => false); to enforce
the requirement, remove the conditional guard and directly assert the badge
visibility (call expect on filterSection.toBeVisible() unconditionally) so the
"success" badge is required for the test to pass; locate the filterSection
variable created with modal.locator('button:has-text("success")') and replace
the conditional visibility gate with a direct expectation to make the assertion
strict.
gsxdsm
added a commit
that referenced
this pull request
Mar 3, 2026
* 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 * refactor(auto-mode): enhance orphaned feature detection and improve project initialization - Updated detectOrphanedFeatures method to accept preloaded features, reducing redundant disk reads. - Improved project initialization by creating required directories and files in parallel for better performance. - Adjusted planning mode handling in UI components to clarify approval requirements for different modes. - Added refresh functionality for file editor tabs to ensure content consistency with disk state. These changes enhance performance, maintainability, and user experience across the application. * feat(orphaned-features): add orphaned features management routes and UI integration - Introduced new routes for managing orphaned features, including listing, resolving, and bulk resolving. - Updated the UI to include an Orphaned Features section in project settings and navigation. - Enhanced the execution service to support new orphaned feature functionalities. These changes improve the application's capability to handle orphaned features effectively, enhancing user experience and project management. * fix: Normalize line endings and resolve stale dirty states in file editor * chore: Update .gitignore and enhance orphaned feature handling - Added a blank line in .gitignore for better readability. - Introduced a hash to worktree paths in orphaned feature resolution to prevent conflicts. - Added validation for target branch existence during orphaned feature resolution. - Improved prompt formatting in execution service for clarity. - Enhanced error handling in project selector for project initialization failures. - Refactored orphaned features section to improve state management and UI responsiveness. These changes improve code maintainability and user experience when managing orphaned features. --------- Co-authored-by: Copilot <223556219+Copilot@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
1 participant
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
Changes
New Orphaned Features Management Routes
/routes/features/routes/orphaned.tswith three handlers:POST /orphaned- Detects orphaned features in a projectPOST /orphaned/resolve- Resolves a single orphaned feature with actions: delete, create-worktree, or move-to-branchPOST /orphaned/bulk-resolve- Resolves multiple orphaned features at once (sequential for worktree creation, parallel for other actions)Orphaned Feature Detection Improvements
detectOrphanedFeatures()method to accept optionalpreloadedFeaturesparameter to avoid redundant disk readsPlanning Prompt Integration
getPlanningPromptPrefixFnin facade to support feature-specific planning modes (lite, spec, full)planningModeandrequirePlanApprovalsettingsSummary by CodeRabbit
New Features
Bug Fixes
Improvements