Fix orphaned features when deleting worktrees#820
Merged
gsxdsm merged 7 commits intoAutoMaker-Org:v1.0.0rcfrom Feb 28, 2026
Merged
Fix orphaned features when deleting worktrees#820gsxdsm merged 7 commits intoAutoMaker-Org:v1.0.0rcfrom
gsxdsm merged 7 commits intoAutoMaker-Org:v1.0.0rcfrom
Conversation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughcreateWorktreeRoutes now accepts an optional FeatureLoader and wires it into the worktree delete handler; worktree deletion migrates branch-scoped features to the main worktree and emits new events. UI and store changes add createdAt timestamps, newest-first sorting, OpenCode provider support, settings sync for dynamic models, and multiple UI/UX badge/layout refinements. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/UI
participant Server as Server
participant FL as FeatureLoader
participant Events as EventEmitter
Client->>Server: DELETE /worktree/:id
activate Server
Server->>FL: loadAllFeatures()
activate FL
FL-->>Server: features[]
deactivate FL
Server->>Server: filter features with branchName == deletedBranch
loop per feature to migrate
Server->>FL: update feature (clear branchName)
activate FL
FL-->>Server: success / error
deactivate FL
Server->>Events: emit feature:migrated (feature)
end
Server->>Events: emit worktree:deleted (worktree info)
Server-->>Client: 200 + { featuresMovedToMain }
deactivate Server
sequenceDiagram
participant User as User
participant UI as ModelSelector UI
participant Store as App Store
participant API as Server API
User->>UI: choose OpenCode provider
UI->>Store: request opencode models
activate Store
Store->>API: GET /opencode/models
activate API
API-->>Store: dynamic models
deactivate API
Store->>Store: sanitize, dedupe, filter by enabled/known IDs
Store-->>UI: merged model list
UI->>Store: setOpencodeDefaultModel(selectedModel)
Store->>API: PATCH /settings (persist)
API-->>Store: persisted
deactivate Store
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 introduces several significant improvements across the application, focusing on data integrity, user experience, and model management. It ensures that no features are lost when Git worktrees are deleted by migrating them to the main branch. Additionally, it refines the display of OpenCode AI models for better clarity and enhances the application's mobile responsiveness, making key interfaces more accessible on smaller screens. Finally, it adds visual cues for plan approvals and improves the backend handling and synchronization of dynamic OpenCode models. 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 several valuable improvements. The backend fix to prevent orphaned features when deleting worktrees is well-implemented, and the logic to move features to the main branch is sound. I've suggested a small performance improvement to parallelize feature updates. The enhancements to OpenCode model name display and the frontend responsiveness for mobile are great additions that will improve the user experience. The new plan approval badge is also a nice touch for better visual feedback.
Comment on lines
145
to
149
| for (const feature of affectedFeatures) { | ||
| await featureLoader.update(projectPath, feature.id, { | ||
| branchName: null, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
The feature updates are currently performed sequentially in a for...of loop. For a branch with many associated features, this could be slow as each update waits for the previous one to complete. You can improve performance by running these updates in parallel using Promise.all.
await Promise.all(
affectedFeatures.map((feature) =>
featureLoader.update(projectPath, feature.id, {
branchName: null,
})
)
);![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/agent-view/components/agent-header.tsx (1)
61-70:⚠️ Potential issue | 🟠 MajorAdd an accessible name for the mobile icon-only Clear button.
On Line 69, the label is hidden on small screens, so this becomes icon-only with no accessible name. Add
aria-label(or screen-reader text) on the button.Suggested fix
{currentSessionId && messagesCount > 0 && ( <Button variant="ghost" size="sm" onClick={onClearChat} disabled={isProcessing} + aria-label="Clear chat" className="text-muted-foreground hover:text-foreground h-8 w-8 p-0 sm:w-auto sm:px-3" > <Trash2 className="w-4 h-4 sm:mr-2" /> - <span className="hidden sm:inline">Clear</span> + <span className="sr-only sm:not-sr-only sm:inline">Clear</span> </Button> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/agent-view/components/agent-header.tsx` around lines 61 - 70, The Clear button becomes icon-only on small screens and lacks an accessible name; update the Button component (the one with props onClearChat, disabled={isProcessing}, and child <Trash2 ... /> in agent-header.tsx) to include an accessible label by adding an aria-label (e.g., aria-label="Clear chat") or a visually hidden screen-reader-only span so the icon-only button is properly announced to assistive technologies.apps/ui/src/hooks/use-settings-migration.ts (1)
882-965:⚠️ Potential issue | 🟠 MajorMissing persistence of
knownDynamicModelIdsin sync function.The
knownDynamicModelIdsfield is hydrated from settings (line 786) but is not included inbuildSettingsUpdateFromStore(). This means the field won't be persisted when settings are synced to the server, defeating its purpose of tracking "all dynamic model IDs ever seen" across sessions.🐛 Proposed fix to include knownDynamicModelIds in persistence
enabledDynamicModelIds: state.enabledDynamicModelIds, + knownDynamicModelIds: state.knownDynamicModelIds, disabledProviders: state.disabledProviders,Add after line 923 where
enabledDynamicModelIdsis already included.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-settings-migration.ts` around lines 882 - 965, buildSettingsUpdateFromStore is not persisting knownDynamicModelIds so that field (which is hydrated from settings) won’t be synced; update buildSettingsUpdateFromStore to include knownDynamicModelIds in the returned object (place it next to enabledDynamicModelIds) so the store’s knownDynamicModelIds is persisted to the server when settings are synced.
🧹 Nitpick comments (2)
apps/ui/src/store/types/state-types.ts (1)
621-625: Action signatures updated to async for persistence.The OpenCode-related actions now return
Promise<void>to support asynchronous persistence. Note that this creates an API inconsistency with similar actions for other providers (e.g.,setCursorDefaultModel,toggleCursorModel,setCodexDefaultModel) which remain synchronous.If this is intentional (only OpenCode dynamic models require persistence), consider adding a brief comment explaining why these specific actions are async. If all provider model actions should eventually be async for persistence, consider tracking that as a follow-up task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/types/state-types.ts` around lines 621 - 625, The OpenCode action signatures (setOpencodeDefaultModel, toggleOpencodeModel, setDynamicOpencodeModels, setEnabledDynamicModelIds, toggleDynamicModel) were changed to return Promise<void> for async persistence while other provider actions remain synchronous; either make them consistent or document the intent. Fix by either (A) updating the other provider action types (e.g., setCursorDefaultModel, toggleCursorModel, setCodexDefaultModel) to return Promise<void> as well to standardize async persistence across providers, or (B) add a short inline comment above the OpenCode action declarations explaining these specific actions are async due to persistence requirements and file a follow-up TODO to harmonize provider APIs; reference the exact symbols above when applying the change.apps/ui/src/store/app-store.ts (1)
2919-2924: Deduplicate merged dynamic model IDs before storing.
updatedEnabledIds(Line 2921) is built via array concat; adding a uniqueness guard avoids duplicate IDs persisting in settings payloads.Proposed refactor
- const updatedEnabledIds = - trulyNewModelIds.length > 0 - ? [...currentEnabledIds, ...trulyNewModelIds] - : currentEnabledIds; + const updatedEnabledIds = + trulyNewModelIds.length > 0 + ? Array.from(new Set([...currentEnabledIds, ...trulyNewModelIds])) + : currentEnabledIds;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/app-store.ts` around lines 2919 - 2924, The merged enabled-model IDs built in updatedEnabledIds currently just concatenates currentEnabledIds and trulyNewModelIds which can introduce duplicates; change its construction to deduplicate (e.g., create from a Set) so updatedEnabledIds becomes the unique union of currentEnabledIds and trulyNewModelIds (preserving order if needed), similar to how updatedKnownIds uses [...new Set([...currentKnownIds, ...allFetchedIds])]; update the code that produces updatedEnabledIds to use a Set-based dedupe (referencing updatedEnabledIds, currentEnabledIds, trulyNewModelIds) before storing the settings.
🤖 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/index.ts`:
- Line 74: Replace the relative import of FeatureLoader with the shared package
import as per guidelines: change the import that currently reads import type {
FeatureLoader } from '../../services/feature-loader.js' to import type {
FeatureLoader } from '@automaker/feature-loader' (or the canonical `@automaker`
package that exports FeatureLoader). Ensure you only change the import statement
and keep the rest of the code using the FeatureLoader type unchanged.
- Around line 99-103: The route wiring for the delete endpoint must forward the
shared EventEmitter so the handler can emit websocket events: update the
router.post call that currently passes only featureLoader to call
createDeleteHandler with the EventEmitter instance (use createEventEmitter()
from lib/events.ts per guideline) alongside featureLoader; ensure you
import/create the emitter in this module and pass that emitter into
createDeleteHandler (which should match the updated signature
createDeleteHandler(events: EventEmitter, featureLoader?: FeatureLoader)),
leaving validatePathParams('projectPath','worktreePath') unchanged.
In `@apps/server/src/routes/worktree/routes/delete.ts`:
- Line 13: Replace the relative import of the FeatureLoader type with the shared
`@automaker` package import: update the import statement that currently reads
"import type { FeatureLoader } from '../../../services/feature-loader.js';" to
import the same symbol from the appropriate `@automaker` package (e.g. "import
type { FeatureLoader } from '@automaker/feature-loader'"), ensuring the
FeatureLoader type is referenced from the shared package rather than a relative
path.
- Around line 141-171: The current loop calling featureLoader.update in the try
block aborts on the first failure and leaves featuresMovedToMain inaccurate;
change the migration to iterate over affectedFeatures and, for each feature,
call featureLoader.update inside its own try/catch so a failing update logs a
warning (include feature.id and branchName) but does not stop the loop, only
increment featuresMovedToMain when an update succeeds, and retain the outer
try/catch for getAll failures; update logging to reflect per-feature failures if
any.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx`:
- Around line 292-294: The tooltip text in TooltipContent currently uses
mobile-specific wording ("tap to approve"); update the copy in the
TooltipContent instance inside card-badges.tsx to a neutral or action-agnostic
phrase such as "click or tap to approve" or "approve this plan" so it reads
correctly on both desktop and mobile; locate the TooltipContent element
(className="text-xs max-w-[250px]") and replace the inner <p> string
accordingly.
In `@apps/ui/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 143-147: When switching to the 'opencode' provider in the provider
change logic, the fallback order incorrectly prefers allOpencodeModels[0] over
the configured opencodeDefaultModel; update the selection to prefer
opencodeDefaultModel first by setting defaultModelId to opencodeDefaultModel ||
allOpencodeModels[0]?.id || 'opencode-big-pickle' and then call
onModelSelect(defaultModelId) (references: provider, selectedProvider,
allOpencodeModels, opencodeDefaultModel, onModelSelect).
In `@apps/ui/src/lib/agent-context-parser.ts`:
- Around line 109-116: The code currently strips tier suffixes from lastSegment
(e.g., ":free", ":extended") and never re-attaches them, causing loss of tier
context; update the logic around lastSegment (the colonIdx slice and the final
return) to detect and save the suffix (e.g., using
lastSegment.match(/:(free|extended|beta|preview)$/i)), remove it only from the
base name for cleaning, then re-append the original suffix (formatted or left
as-is) to the cleaned display name before returning so the tier remains visible.
---
Outside diff comments:
In `@apps/ui/src/components/views/agent-view/components/agent-header.tsx`:
- Around line 61-70: The Clear button becomes icon-only on small screens and
lacks an accessible name; update the Button component (the one with props
onClearChat, disabled={isProcessing}, and child <Trash2 ... /> in
agent-header.tsx) to include an accessible label by adding an aria-label (e.g.,
aria-label="Clear chat") or a visually hidden screen-reader-only span so the
icon-only button is properly announced to assistive technologies.
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Around line 882-965: buildSettingsUpdateFromStore is not persisting
knownDynamicModelIds so that field (which is hydrated from settings) won’t be
synced; update buildSettingsUpdateFromStore to include knownDynamicModelIds in
the returned object (place it next to enabledDynamicModelIds) so the store’s
knownDynamicModelIds is persisted to the server when settings are synced.
---
Nitpick comments:
In `@apps/ui/src/store/app-store.ts`:
- Around line 2919-2924: The merged enabled-model IDs built in updatedEnabledIds
currently just concatenates currentEnabledIds and trulyNewModelIds which can
introduce duplicates; change its construction to deduplicate (e.g., create from
a Set) so updatedEnabledIds becomes the unique union of currentEnabledIds and
trulyNewModelIds (preserving order if needed), similar to how updatedKnownIds
uses [...new Set([...currentKnownIds, ...allFetchedIds])]; update the code that
produces updatedEnabledIds to use a Set-based dedupe (referencing
updatedEnabledIds, currentEnabledIds, trulyNewModelIds) before storing the
settings.
In `@apps/ui/src/store/types/state-types.ts`:
- Around line 621-625: The OpenCode action signatures (setOpencodeDefaultModel,
toggleOpencodeModel, setDynamicOpencodeModels, setEnabledDynamicModelIds,
toggleDynamicModel) were changed to return Promise<void> for async persistence
while other provider actions remain synchronous; either make them consistent or
document the intent. Fix by either (A) updating the other provider action types
(e.g., setCursorDefaultModel, toggleCursorModel, setCodexDefaultModel) to return
Promise<void> as well to standardize async persistence across providers, or (B)
add a short inline comment above the OpenCode action declarations explaining
these specific actions are async due to persistence requirements and file a
follow-up TODO to harmonize provider APIs; reference the exact symbols above
when applying the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/server/src/index.tsapps/server/src/providers/opencode-provider.tsapps/server/src/routes/worktree/index.tsapps/server/src/routes/worktree/routes/delete.tsapps/ui/src/components/views/agent-view/components/agent-header.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsxapps/ui/src/components/views/board-view/components/list-view/list-row.tsxapps/ui/src/components/views/board-view/dialogs/plan-approval-dialog.tsxapps/ui/src/components/views/board-view/shared/model-selector.tsxapps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/lib/agent-context-parser.tsapps/ui/src/store/app-store.tsapps/ui/src/store/types/state-types.tslibs/types/src/settings.ts
| import { createSyncHandler } from './routes/sync.js'; | ||
| import { createUpdatePRNumberHandler } from './routes/update-pr-number.js'; | ||
| import type { SettingsService } from '../../services/settings-service.js'; | ||
| import type { FeatureLoader } from '../../services/feature-loader.js'; |
Contributor
There was a problem hiding this comment.
Use a shared @automaker/* import for FeatureLoader.
This newly added relative import conflicts with the TS/JS import rule.
As per coding guidelines **/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths like '../services/feature-loader' or '../lib/logger'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/worktree/index.ts` at line 74, Replace the relative
import of FeatureLoader with the shared package import as per guidelines: change
the import that currently reads import type { FeatureLoader } from
'../../services/feature-loader.js' to import type { FeatureLoader } from
'@automaker/feature-loader' (or the canonical `@automaker` package that exports
FeatureLoader). Ensure you only change the import statement and keep the rest of
the code using the FeatureLoader type unchanged.
Comment on lines
99
to
103
| router.post( | ||
| '/delete', | ||
| validatePathParams('projectPath', 'worktreePath'), | ||
| createDeleteHandler(featureLoader) | ||
| ); |
Contributor
There was a problem hiding this comment.
Forward events to delete handler so this operation can emit WebSocket events.
The delete flow now mutates feature state, but this route wiring only passes featureLoader, so the handler cannot emit operation events through the shared emitter.
💡 Suggested wiring update
router.post(
'/delete',
validatePathParams('projectPath', 'worktreePath'),
- createDeleteHandler(featureLoader)
+ createDeleteHandler(events, featureLoader)
);// Companion update in apps/server/src/routes/worktree/routes/delete.ts
export function createDeleteHandler(events: EventEmitter, featureLoader?: FeatureLoader) { ... }As per coding guidelines apps/server/src/**/*.{ts,js}: Use createEventEmitter() from lib/events.ts for all server operations to emit events that stream to the frontend via WebSocket.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/worktree/index.ts` around lines 99 - 103, The route
wiring for the delete endpoint must forward the shared EventEmitter so the
handler can emit websocket events: update the router.post call that currently
passes only featureLoader to call createDeleteHandler with the EventEmitter
instance (use createEventEmitter() from lib/events.ts per guideline) alongside
featureLoader; ensure you import/create the emitter in this module and pass that
emitter into createDeleteHandler (which should match the updated signature
createDeleteHandler(events: EventEmitter, featureLoader?: FeatureLoader)),
leaving validatePathParams('projectPath','worktreePath') unchanged.
| import { getErrorMessage, logError, isValidBranchName } from '../common.js'; | ||
| import { execGitCommand } from '../../../lib/git.js'; | ||
| import { createLogger } from '@automaker/utils'; | ||
| import type { FeatureLoader } from '../../../services/feature-loader.js'; |
Contributor
There was a problem hiding this comment.
Use a shared @automaker/* import for FeatureLoader.
This new relative service import violates the repository import rule for TS/JS files.
As per coding guidelines **/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths like '../services/feature-loader' or '../lib/logger'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/worktree/routes/delete.ts` at line 13, Replace the
relative import of the FeatureLoader type with the shared `@automaker` package
import: update the import statement that currently reads "import type {
FeatureLoader } from '../../../services/feature-loader.js';" to import the same
symbol from the appropriate `@automaker` package (e.g. "import type {
FeatureLoader } from '@automaker/feature-loader'"), ensuring the FeatureLoader
type is referenced from the shared package rather than a relative path.
apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/shared/model-selector.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
apps/server/src/routes/worktree/index.ts (1)
74-74:⚠️ Potential issue | 🟠 MajorUse a shared
@automaker/*import forFeatureLoader.This still uses a relative service path that violates the repository import rule.
As per coding guidelines
**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths like '../services/feature-loader' or '../lib/logger'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/index.ts` at line 74, Replace the relative import of FeatureLoader with the shared package export: locate the import statement importing FeatureLoader (the line with "import type { FeatureLoader } from '../../services/feature-loader.js'") and change it to import the type from the shared package (e.g. `@automaker/feature-loader`) so the module follows the `@automaker/`* import rule; ensure the imported symbol remains FeatureLoader and update any path references or build configs if necessary.apps/server/src/routes/worktree/routes/delete.ts (1)
13-13:⚠️ Potential issue | 🟠 MajorUse the shared package import for
FeatureLoaderinstead of a relative service path.This import path violates the repository TS/JS import rule and was previously flagged.
As per coding guidelines
**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths like '../services/feature-loader' or '../lib/logger'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/delete.ts` at line 13, The file imports the type FeatureLoader via a relative path; replace that relative import with the canonical shared package import (e.g. import type { FeatureLoader } from '@automaker/feature-loader' or the appropriate `@automaker/`* package that exports FeatureLoader) so it follows the repository TS/JS import rule; update the import statement that references FeatureLoader in the delete route module to use the package export instead of '../../../services/feature-loader.js'.
🧹 Nitpick comments (1)
apps/ui/src/store/types/state-types.ts (1)
217-221: Update the dynamic-model persistence comment to match current behavior.Line 217 says dynamic models are “session-only (not persisted)”, but the new action contracts and store logic now persist enabled/known dynamic model IDs. This comment is stale and misleading.
Proposed fix
- // Dynamic models are session-only (not persisted) because they're discovered at runtime - // from `opencode models` CLI and depend on current provider authentication state + // Dynamic models are discovered at runtime from `opencode models` CLI. + // Enabled/known dynamic model IDs are persisted to preserve user choices + // across sessions while still allowing runtime discovery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/types/state-types.ts` around lines 217 - 221, Update the stale comment above the dynamic model fields to reflect that enabledDynamicModelIds and knownDynamicModelIds are now persisted (no longer session-only) instead of saying dynamic models are "session-only (not persisted)"; specifically mention that dynamicOpencodeModels are discovered at runtime while enabledDynamicModelIds and knownDynamicModelIds are persisted via the new action contracts/store logic to track user selections across sessions, and remove or reword the "not persisted" phrasing accordingly.
🤖 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/delete.ts`:
- Around line 19-20: The createDeleteHandler currently receives events
(EventEmitter) but never emits events after deletion or migration; update the
handler to emit appropriate events using the injected events emitter: after the
worktree deletion succeeds in createDeleteHandler emit a "worktree:deleted"
event with identifying payload (e.g., worktreeId and repo/project context) and
after any feature migration logic emits a "feature:migrated" (or
"feature:migration") event with the feature id, status and any migrated-to
worktree id; ensure these emits occur where delete/migrate operations complete
(inside the async handler code paths that currently perform deletion/migration)
so frontend subscribers via createEventEmitter() will receive realtime updates.
In `@apps/ui/src/components/views/board-view/components/list-view/list-row.tsx`:
- Line 127: Replace the device-specific tooltip text on the badge in
list-row.tsx: locate the Badge (or status badge) that sets tooltip: 'Plan ready
for review - tap to approve' and change it to neutral wording for all platforms
(e.g., 'Plan ready for review and approval' or 'Ready for review and approval')
so it no longer references touch-only actions; update the tooltip prop value on
the Badge/component rendering the status.
In `@apps/ui/src/components/views/board-view/components/list-view/list-view.tsx`:
- Around line 245-248: When sortNewestCardOnTop is true the UI uses
effectiveSortConfig ({ column: 'createdAt', direction: 'desc' }) for row
ordering but ListHeader still receives the original sortConfig, causing the
header indicator to diverge; update the component usage so ListHeader receives
effectiveSortConfig (or a derived displaySort equal to effectiveSortConfig)
instead of sortConfig and ensure any other places that render sort state (e.g.,
where sortConfig is passed down or used) also use effectiveSortConfig so the
header and rows remain consistent.
In `@apps/ui/src/components/views/board-view/hooks/use-board-background.ts`:
- Around line 14-22: backgroundSettings currently forces sortNewestCardOnTop to
the global default (defaultSortNewestCardOnTop) and thereby ignores any
persisted per-project value; either respect the per-project override by reading
perProjectSettings.sortNewestCardOnTop with a fallback to
defaultSortNewestCardOnTop in the useMemo that defines backgroundSettings (use
currentProject, boardBackgroundByProject and defaultBackgroundSettings to locate
perProjectSettings), or remove the per-project API setSortNewestCardOnTop in
use-board-background-settings.ts if you intend it to be global-only; update
backgroundSettings or the setter accordingly so saved values are actually
applied.
In `@apps/ui/src/components/views/board-view/hooks/use-board-column-features.ts`:
- Around line 300-309: The sorting loop controlled by sortNewestCardOnTop is
clobbering the dependency-aware ordering previously applied to the backlog
column; update the loop in use-board-column-features so that when
sortNewestCardOnTop is true you skip sorting the backlog column (i.e., add a
guard like if (columnId === <backlogColumnKey>) continue) so the backlog column
retains the dependency/blocking order established earlier (use the same backlog
column key used in the dependency-ordering code), or alternatively add a clear
comment/documentation near sortNewestCardOnTop explaining that newest-first
intentionally overrides backlog dependency ordering.
In `@apps/ui/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 62-67: Effect currently refires whenever opencodeModelsLoading
toggles to false while dynamicOpencodeModelsList is empty, causing repeated
fetch retries; modify the effect to perform the fetch only once (or until an
explicit reset) by adding a local flag/ref (e.g., attemptedOpencodeFetchRef or
opencodeFetchTried state) and set it true immediately when calling
fetchOpencodeModels; update the condition in the useEffect (which references
opencodeModelsLoading, dynamicOpencodeModelsList.length, fetchOpencodeModels) to
require that this "not yet attempted" flag is false before calling
fetchOpencodeModels, so subsequent transitions to loading=false won’t retrigger
fetches unless the flag is cleared intentionally.
In `@apps/ui/src/lib/agent-context-parser.ts`:
- Around line 109-118: The code currently appends the raw tier token
(tierSuffix) to the cleaned model name, exposing ID syntax; change this to
convert the captured tier token (use tierMatch[1]) into a human-friendly label
and append it in parentheses (e.g., " (Free)", " (Preview)", " (Extended)", "
(Beta)") with proper capitalization and spacing. Locate the logic around
tierMatch / tierSuffix / cleanedName in agent-context-parser.ts and replace the
raw suffix re-append with a small mapping or capitalize the captured group and
return `${cleanedName} (${capitalizedTier})` when tierMatch exists.
In `@apps/ui/src/store/app-store.ts`:
- Around line 1372-1380: toggleOpencodeModel currently appends model IDs to
enabledOpencodeModels without checking for existing entries, causing duplicates
to be saved; change the add-path in toggleOpencodeModel to only add when the
model is not already present (e.g., check
state.enabledOpencodeModels.includes(model) before pushing or build the new
array as [...new Set([...state.enabledOpencodeModels, model])]), and ensure the
same deduplicated array is what you pass to
getHttpApiClient().settings.updateGlobal and any other code paths that modify
enabledOpencodeModels so persisted settings never contain duplicates.
- Around line 1836-1843: The current setAllProjectsSortNewestCardOnTop only
updates existing keys in state.boardBackgroundByProject; change it to build
updated = { ...state.boardBackgroundByProject } and then iterate over the union
of project paths from state.boardBackgroundByProject and the global project list
(e.g. state.projects or state.projectsByPath — whichever holds all projects) and
set updated[projectPath] = { ...(updated[projectPath] ??
defaultBoardBackgroundSettings), sortNewestCardOnTop: enabled } so projects
without prior settings get an entry; keep references to
setAllProjectsSortNewestCardOnTop and state.boardBackgroundByProject when making
the change.
---
Duplicate comments:
In `@apps/server/src/routes/worktree/index.ts`:
- Line 74: Replace the relative import of FeatureLoader with the shared package
export: locate the import statement importing FeatureLoader (the line with
"import type { FeatureLoader } from '../../services/feature-loader.js'") and
change it to import the type from the shared package (e.g.
`@automaker/feature-loader`) so the module follows the `@automaker/`* import rule;
ensure the imported symbol remains FeatureLoader and update any path references
or build configs if necessary.
In `@apps/server/src/routes/worktree/routes/delete.ts`:
- Line 13: The file imports the type FeatureLoader via a relative path; replace
that relative import with the canonical shared package import (e.g. import type
{ FeatureLoader } from '@automaker/feature-loader' or the appropriate
`@automaker/`* package that exports FeatureLoader) so it follows the repository
TS/JS import rule; update the import statement that references FeatureLoader in
the delete route module to use the package export instead of
'../../../services/feature-loader.js'.
---
Nitpick comments:
In `@apps/ui/src/store/types/state-types.ts`:
- Around line 217-221: Update the stale comment above the dynamic model fields
to reflect that enabledDynamicModelIds and knownDynamicModelIds are now
persisted (no longer session-only) instead of saying dynamic models are
"session-only (not persisted)"; specifically mention that dynamicOpencodeModels
are discovered at runtime while enabledDynamicModelIds and knownDynamicModelIds
are persisted via the new action contracts/store logic to track user selections
across sessions, and remove or reword the "not persisted" phrasing accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apps/server/src/routes/worktree/index.tsapps/server/src/routes/worktree/routes/delete.tsapps/server/src/services/feature-loader.tsapps/ui/src/components/dialogs/board-background-modal.tsxapps/ui/src/components/ui/sheet.tsxapps/ui/src/components/views/agent-view/components/agent-header.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsxapps/ui/src/components/views/board-view/components/list-view/list-row.tsxapps/ui/src/components/views/board-view/components/list-view/list-view.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/hooks/use-board-background.tsapps/ui/src/components/views/board-view/hooks/use-board-column-features.tsapps/ui/src/components/views/board-view/shared/model-selector.tsxapps/ui/src/components/views/settings-view/appearance/appearance-section.tsxapps/ui/src/hooks/use-board-background-settings.tsapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/lib/agent-context-parser.tsapps/ui/src/store/app-store.tsapps/ui/src/store/defaults/background-settings.tsapps/ui/src/store/types/state-types.tsapps/ui/src/store/types/ui-types.tslibs/types/src/feature.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/types/src/settings.ts
apps/ui/src/components/views/board-view/components/list-view/list-row.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/components/list-view/list-view.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/hooks/use-board-background.ts
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/hooks/use-board-column-features.ts
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/shared/model-selector.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/server/src/routes/worktree/routes/delete.ts (1)
13-13:⚠️ Potential issue | 🟠 MajorReplace the relative
FeatureLoaderimport with the shared package import.This reintroduces the old relative service-path pattern in a TS file.
As per coding guidelines
**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths like '../services/feature-loader' or '../lib/logger'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/delete.ts` at line 13, Replace the relative import of FeatureLoader with the shared package import; specifically, change the import line that references '../../../services/feature-loader.js' to import type { FeatureLoader } from '@automaker/feature-loader' (or the org-scoped shared package that exports FeatureLoader) so the module uses the canonical `@automaker` package instead of the old relative service path.apps/ui/src/store/app-store.ts (1)
1386-1404:⚠️ Potential issue | 🟡 MinorDeduplicate dynamic model IDs before writing local/server state.
setEnabledDynamicModelIdsandtoggleDynamicModelcan still persist duplicate IDs.💡 Suggested fix
setEnabledDynamicModelIds: async (ids) => { - set({ enabledDynamicModelIds: ids }); + const dedupedIds = [...new Set(ids)]; + set({ enabledDynamicModelIds: dedupedIds }); try { const httpApi = getHttpApiClient(); - await httpApi.settings.updateGlobal({ enabledDynamicModelIds: ids }); + await httpApi.settings.updateGlobal({ enabledDynamicModelIds: dedupedIds }); } catch (error) { logger.error('Failed to sync enabledDynamicModelIds:', error); } }, toggleDynamicModel: async (modelId, enabled) => { set((state) => ({ enabledDynamicModelIds: enabled - ? [...state.enabledDynamicModelIds, modelId] + ? [...new Set([...state.enabledDynamicModelIds, modelId])] : state.enabledDynamicModelIds.filter((id) => id !== modelId), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/app-store.ts` around lines 1386 - 1404, Both setEnabledDynamicModelIds and toggleDynamicModel can persist duplicate model IDs; ensure you dedupe before updating local state and before sending to the server. In setEnabledDynamicModelIds, replace direct set({ enabledDynamicModelIds: ids }) with a deduped list (e.g., Array.from(new Set(ids))) and pass that deduped list to httpApi.settings.updateGlobal; in toggleDynamicModel, when adding modelId compute the new list by merging current get().enabledDynamicModelIds with modelId then dedupe (or only add if not present) before calling set(...) and before await httpApi.settings.updateGlobal(...). Use the existing functions get(), set(), setEnabledDynamicModelIds/toggleDynamicModel and getHttpApiClient() to locate the changes.
🧹 Nitpick comments (1)
apps/ui/src/components/views/board-view/shared/model-selector.tsx (1)
4-4: Prefer barrel export for intra-app UI component imports.Line 4 adds another direct UI-component path import. In this app area, we should prefer the local components barrel export for better refactor stability.
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/src/components/views/board-view/shared/model-selector.tsx` at line 4, Replace the direct UI-component path import for AnthropicIcon, CursorIcon, OpenAIIcon, and OpenCodeIcon with the apps/ui local components barrel export: update the import that currently pulls those symbols from '@/components/ui/provider-icon' to import the same named exports from the app-level components index (the ../components barrel) so intra-app imports use the centralized barrel for better refactor stability.
🤖 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/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 66-75: The useEffect may call fetchOpencodeModels() even when the
OpenCode provider is disabled; update the effect that currently checks
opencodeModelsLoading, dynamicOpencodeModelsList.length and
opencodeFetchTriedRef to also verify the provider-enabled flag before triggering
fetchOpencodeModels() — e.g., add a guard such as opencodeProviderEnabled (or
the existing isOpenCodeEnabled flag) to the condition and to the dependency
array so fetchOpencodeModels() only runs when the provider is enabled.
- Around line 43-44: Destructure the error from useOpencodeModels (e.g., const {
data: dynamicOpencodeModelsList = [], isLoading: dynamicOpencodeLoading, error:
dynamicOpencodeError } = useOpencodeModels()) and add an error render branch in
the OpenCode models section of model-selector.tsx that shows when
dynamicOpencodeError is truthy and dynamicOpencodeLoading is false; follow the
same pattern used by the Codex error handling block (render a user-facing error
message plus a retry action or a link to Settings) so users see the failure and
can retry or navigate to settings.
---
Duplicate comments:
In `@apps/server/src/routes/worktree/routes/delete.ts`:
- Line 13: Replace the relative import of FeatureLoader with the shared package
import; specifically, change the import line that references
'../../../services/feature-loader.js' to import type { FeatureLoader } from
'@automaker/feature-loader' (or the org-scoped shared package that exports
FeatureLoader) so the module uses the canonical `@automaker` package instead of
the old relative service path.
In `@apps/ui/src/store/app-store.ts`:
- Around line 1386-1404: Both setEnabledDynamicModelIds and toggleDynamicModel
can persist duplicate model IDs; ensure you dedupe before updating local state
and before sending to the server. In setEnabledDynamicModelIds, replace direct
set({ enabledDynamicModelIds: ids }) with a deduped list (e.g., Array.from(new
Set(ids))) and pass that deduped list to httpApi.settings.updateGlobal; in
toggleDynamicModel, when adding modelId compute the new list by merging current
get().enabledDynamicModelIds with modelId then dedupe (or only add if not
present) before calling set(...) and before await
httpApi.settings.updateGlobal(...). Use the existing functions get(), set(),
setEnabledDynamicModelIds/toggleDynamicModel and getHttpApiClient() to locate
the changes.
---
Nitpick comments:
In `@apps/ui/src/components/views/board-view/shared/model-selector.tsx`:
- Line 4: Replace the direct UI-component path import for AnthropicIcon,
CursorIcon, OpenAIIcon, and OpenCodeIcon with the apps/ui local components
barrel export: update the import that currently pulls those symbols from
'@/components/ui/provider-icon' to import the same named exports from the
app-level components index (the ../components barrel) so intra-app imports use
the centralized barrel for better refactor stability.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/server/src/routes/worktree/routes/delete.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/hooks/use-board-background.tsapps/ui/src/components/views/board-view/hooks/use-board-column-features.tsapps/ui/src/components/views/board-view/shared/model-selector.tsxapps/ui/src/lib/agent-context-parser.tsapps/ui/src/store/app-store.tsapps/ui/src/store/types/state-types.tslibs/types/src/event.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/src/lib/agent-context-parser.ts
- apps/ui/src/components/views/board-view/hooks/use-board-background.ts
apps/ui/src/components/views/board-view/shared/model-selector.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/shared/model-selector.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ui/src/components/views/board-view/shared/model-selector.tsx (1)
489-495: Model list may render while loading is still in progress.The loading state (lines 489-495) only displays when
allOpencodeModels.length === 0. However, the model list (line 525) renders wheneverallOpencodeModels.length > 0, regardless of loading state. If static models are enabled, users might see an incomplete list (static only) while dynamic models are still loading.Consider guarding the model list to avoid showing a partial list during loading:
Proposed fix
{/* Model list */} - {allOpencodeModels.length > 0 && ( + {!opencodeModelsLoading && !dynamicOpencodeLoading && allOpencodeModels.length > 0 && ( <div className="flex flex-col gap-2">Also applies to: 524-525
🤖 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/shared/model-selector.tsx` around lines 489 - 495, The model list is rendered even while dynamic or static model loading flags are true, causing partial lists; update the JSX so the model list is only rendered when loading is finished by changing its condition to require !opencodeModelsLoading && !dynamicOpencodeLoading && allOpencodeModels.length > 0 (apply the same guard where the list is currently shown), referencing the opencodeModelsLoading, dynamicOpencodeLoading and allOpencodeModels variables in model-selector.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ui/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 489-495: The model list is rendered even while dynamic or static
model loading flags are true, causing partial lists; update the JSX so the model
list is only rendered when loading is finished by changing its condition to
require !opencodeModelsLoading && !dynamicOpencodeLoading &&
allOpencodeModels.length > 0 (apply the same guard where the list is currently
shown), referencing the opencodeModelsLoading, dynamicOpencodeLoading and
allOpencodeModels variables in model-selector.tsx.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/views/board-view/shared/model-selector.tsxapps/ui/src/store/app-store.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/ui/src/components/views/board-view/shared/model-selector.tsx (1)
161-165:⚠️ Potential issue | 🟠 MajorValidate OpenCode default model before selecting it.
At Line 164,
opencodeDefaultModelis used even if it is no longer enabled/available inallOpencodeModels. That can select a model not present in the rendered list.💡 Proposed fix
- const defaultModelId = - opencodeDefaultModel || allOpencodeModels[0]?.id || 'opencode-big-pickle'; + const defaultModelId = allOpencodeModels.some((m) => m.id === opencodeDefaultModel) + ? opencodeDefaultModel + : allOpencodeModels[0]?.id || 'opencode-big-pickle'; onModelSelect(defaultModelId);🤖 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/shared/model-selector.tsx` around lines 161 - 165, When switching to provider === 'opencode' in the branch that handles selectedProvider !== 'opencode', validate that opencodeDefaultModel is actually present in allOpencodeModels (e.g., check ids via allOpencodeModels.some(m => m.id === opencodeDefaultModel)) before calling onModelSelect; if it’s not present fall back to the first available model id (allOpencodeModels[0]?.id) or the hardcoded 'opencode-big-pickle' default. Update the selection logic around opencodeDefaultModel / allOpencodeModels / onModelSelect to perform this presence check and only select opencodeDefaultModel when it is enabled.
🧹 Nitpick comments (1)
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx (1)
484-496: Consider addingtype="button"for defensive coding.The button element lacks an explicit
type="button"attribute. While this matches the existing expand button pattern (lines 497-509), buttons without an explicit type default totype="submit", which could cause unintended form submissions if this component is ever rendered within a form context.🛡️ Suggested defensive fix
<button onClick={(e) => { e.stopPropagation(); setIsSummaryDialogOpen(true); }} onPointerDown={(e) => e.stopPropagation()} onMouseDown={(e) => e.stopPropagation()} className="flex items-center gap-1 text-[10px] text-[var(--status-success)] min-w-0 hover:opacity-80 transition-opacity cursor-pointer" title="View full summary" + type="button" >Consider applying the same to the expand button at line 497 for consistency.
🤖 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/components/kanban-card/agent-info-panel.tsx` around lines 484 - 496, The Summary button handler currently uses a plain <button> (rendering the Sparkles icon and calling setIsSummaryDialogOpen) but lacks an explicit type, so add type="button" to that button element to prevent unintended form submissions; also apply the same type="button" defensive change to the adjacent expand button (the other button that uses onClick to toggle expansion) to keep behavior consistent.
🤖 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/src/components/views/board-view/components/selection-action-bar.tsx`:
- Around line 56-57: The handler handleConfirmVerify currently returns early
when onVerify is undefined, leaving the confirmation dialog open; update it to
either call the dialog close routine (e.g., invoke the existing
close/closeDialog/onClose function) or programmatically disable/hide the Confirm
action when onVerify is not provided. Specifically, change handleConfirmVerify
to check if (!onVerify) then close the dialog (or call the component's close
method) instead of returning no-op, and also update the Confirm button/element
to be disabled when onVerify is falsy so the user cannot trigger the dialog
without a handler.
In `@apps/ui/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 498-506: The retry button is calling the Zustand action
fetchOpencodeModels() while the OpenCode error state comes from the React Query
hook useOpencodeModels(); replace the store call with the hook's refetch:
destructure refetch from useOpencodeModels() (alongside data/isLoading/error)
and invoke refetch() in the button onClick instead of fetchOpencodeModels();
alternatively, if you must keep the store action, make fetchOpencodeModels()
call queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() })
after a successful fetch (as done in opencode-settings-tab.tsx) so the React
Query error state (dynamicOpencodeError/dynamicOpencodeLoading) is properly
refreshed.
---
Duplicate comments:
In `@apps/ui/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 161-165: When switching to provider === 'opencode' in the branch
that handles selectedProvider !== 'opencode', validate that opencodeDefaultModel
is actually present in allOpencodeModels (e.g., check ids via
allOpencodeModels.some(m => m.id === opencodeDefaultModel)) before calling
onModelSelect; if it’s not present fall back to the first available model id
(allOpencodeModels[0]?.id) or the hardcoded 'opencode-big-pickle' default.
Update the selection logic around opencodeDefaultModel / allOpencodeModels /
onModelSelect to perform this presence check and only select
opencodeDefaultModel when it is enabled.
---
Nitpick comments:
In
`@apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx`:
- Around line 484-496: The Summary button handler currently uses a plain
<button> (rendering the Sparkles icon and calling setIsSummaryDialogOpen) but
lacks an explicit type, so add type="button" to that button element to prevent
unintended form submissions; also apply the same type="button" defensive change
to the adjacent expand button (the other button that uses onClick to toggle
expansion) to keep behavior consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsxapps/ui/src/components/views/board-view/components/selection-action-bar.tsxapps/ui/src/components/views/board-view/shared/model-selector.tsx
apps/ui/src/components/views/board-view/components/selection-action-bar.tsx
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/ui/src/components/views/board-view/components/selection-action-bar.tsx (1)
121-126:⚠️ Potential issue | 🟡 MinorDisable the Verify launcher when no handler is provided.
onVerifyis optional, but the launcher button can still open the dialog when a handler is absent. Align launcher behavior with confirm button disablement to avoid dead-end UX.💡 Suggested patch
<Button variant="default" size="sm" onClick={handleVerifyClick} - disabled={selectedCount === 0} + disabled={selectedCount === 0 || !onVerify || isVerifying} className="h-8 bg-green-600 hover:bg-green-700 disabled:opacity-50" data-testid="selection-verify-button" >🤖 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/components/selection-action-bar.tsx` around lines 121 - 126, The Verify launcher currently enables when there are selections even if no onVerify handler exists; update the Button in selection-action-bar (the Button with onClick={handleVerifyClick}) to be disabled when either selectedCount === 0 or onVerify is falsy (e.g., disabled={selectedCount === 0 || !onVerify}) and also add a defensive guard inside handleVerifyClick to return early if onVerify is not provided before attempting to open the dialog, ensuring the UI never opens a dead-end dialog when no handler is supplied.
🤖 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/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 72-80: The effect currently calls fetchOpencodeModels() when
isOpencodeEnabled && !opencodeModelsLoading && dynamicOpencodeModelsList.length
=== 0 && !opencodeFetchTriedRef.current, but it omits the dynamicOpencodeLoading
flag from the guard and can fire while the React Query hook is still in-flight;
update the condition(s) around opencodeFetchTriedRef.current and
fetchOpencodeModels() to also require !dynamicOpencodeLoading so you never
trigger a second fetch while the useOpencodeModels() query is loading, and apply
the same change to the similar block that mirrors lines 82-86 (referencing
isOpencodeEnabled, opencodeModelsLoading, dynamicOpencodeModelsList,
opencodeFetchTriedRef, dynamicOpencodeLoading, and fetchOpencodeModels).
- Around line 164-169: The current logic may call onModelSelect with a hardcoded
'opencode-big-pickle' when opencodeDefaultModel isn't available, which can
select a disabled model; change the selection logic in the block that computes
isDefaultModelAvailable / defaultModelId so you only choose an id that exists in
allOpencodeModels (use allOpencodeModels[0]?.id if present) and do not fall back
to the literal 'opencode-big-pickle'; call onModelSelect only when a valid model
id is found (i.e., defaultModelId is defined and present in allOpencodeModels)
so we never programmatically select a disabled/non-loaded model (refer to
isDefaultModelAvailable, opencodeDefaultModel, allOpencodeModels,
defaultModelId, and onModelSelect).
---
Duplicate comments:
In `@apps/ui/src/components/views/board-view/components/selection-action-bar.tsx`:
- Around line 121-126: The Verify launcher currently enables when there are
selections even if no onVerify handler exists; update the Button in
selection-action-bar (the Button with onClick={handleVerifyClick}) to be
disabled when either selectedCount === 0 or onVerify is falsy (e.g.,
disabled={selectedCount === 0 || !onVerify}) and also add a defensive guard
inside handleVerifyClick to return early if onVerify is not provided before
attempting to open the dialog, ensuring the UI never opens a dead-end dialog
when no handler is supplied.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsxapps/ui/src/components/views/board-view/components/selection-action-bar.tsxapps/ui/src/components/views/board-view/shared/model-selector.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx
apps/ui/src/components/views/board-view/shared/model-selector.tsx
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
gsxdsm
added a commit
to gsxdsm/automaker
that referenced
this pull request
Mar 1, 2026
* Changes from fix/orphaned-features * fix: Handle feature migration failures and improve UI accessibility * feat: Add event emission for worktree deletion and feature migration * fix: Handle OpenCode model errors and prevent duplicate model IDs * feat: Add summary dialog and async verify with loading state * fix: Add type attributes to buttons and improve OpenCode model selection * fix: Add null checks for onVerify callback and opencode model selection
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
Backend - Worktree Deletion (Fix for Orphaned Features)
createWorktreeRoutesto acceptFeatureLoaderdependencycreateDeleteHandlerto move features from deleted branches to main worktreebranchName: null)featuresMovedToMaincount in the deletion responseBackend - OpenCode Model Display Names
formatModelDisplayNameinOpencodeProviderto properly parse nested model identifiers:free,:extended,:beta,:previewFrontend - Mobile Responsiveness
Summary by CodeRabbit
New Features
Improvements