Improve auto-loop event emission and add ntfy notifications#821
Merged
gsxdsm merged 9 commits intoAutoMaker-Org:v1.0.0rcfrom Mar 1, 2026
Merged
Improve auto-loop event emission and add ntfy notifications#821gsxdsm merged 9 commits intoAutoMaker-Org:v1.0.0rcfrom
gsxdsm merged 9 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:
📝 WalkthroughWalkthroughAdds ntfy.sh notification support (server + UI), worktree-aware auto-mode idle detection, feature-error notifications and handling, deep-linking for board feature modals, numerous UI/store/type changes, extensive tests, and version bumps to 1.0.0. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Router as Board Router
participant BoardView as BoardView
participant Store as AppStore
participant Server as Backend API
participant FeatureDB as Feature Loader
Browser->>Router: Navigate /board?featureId=F
Router->>BoardView: render with initialFeatureId=F
BoardView->>Server: load feature metadata (by id)
Server->>FeatureDB: fetch feature
FeatureDB-->>Server: feature { id, branchName }
Server-->>BoardView: feature metadata
alt feature.branchName exists
BoardView->>Store: switch worktree to feature.branchName
else
BoardView->>Store: switch to main worktree
end
Store-->>BoardView: worktree switched
BoardView->>BoardView: open output modal for F
BoardView-->>Browser: modal displayed
sequenceDiagram
participant HookSvc as EventHookService
participant Settings as SettingsService
participant NtfySvc as NtfyService
participant NtfyServer as ntfy.sh
participant Logger as Logger
HookSvc->>Settings: getGlobalSettings()
Settings-->>HookSvc: returns hooks + ntfyEndpoints
HookSvc->>HookSvc: select ntfy action & resolve endpointId
HookSvc->>Settings: get endpoint by id
Settings-->>HookSvc: NtfyEndpointConfig
HookSvc->>NtfySvc: sendNotification(endpoint, options, context)
NtfySvc->>NtfySvc: validate endpoint, substitute variables, build headers/body
NtfySvc->>NtfyServer: POST /<topic> with payload
alt 2xx
NtfyServer-->>NtfySvc: success
NtfySvc->>Logger: log success
NtfySvc-->>HookSvc: { success: true }
else error
NtfyServer-->>NtfySvc: error/timeout
NtfySvc->>Logger: log error
NtfySvc-->>HookSvc: { success: false, error }
end
HookSvc->>Logger: record hook result
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 event notification system by integrating ntfy.sh for customizable push notifications, improving the accuracy of auto-loop idle event emissions, and enabling deep-linking to specific features within the board view. These changes aim to provide users with more timely and relevant information about their automated processes and improve overall navigation and maintainability. 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
Activity
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 significant improvements, including more robust event emission for the auto-loop, support for ntfy.sh notifications, and several UI/UX enhancements. The refactoring of sleep intervals into constants and the consolidation of notification logic are great for maintainability. The fix for premature auto_mode_idle events is a critical improvement. I've provided a couple of suggestions to further improve robustness and code clarity. Overall, this is a high-quality set of changes.
Comment on lines
+517
to
+524
| } catch (error) { | ||
| const errorInfo = classifyError(error); | ||
| logger.warn( | ||
| `Failed to load all features for idle check (projectPath=${projectPath}, branchName=${branchName}): ${errorInfo.message}`, | ||
| error | ||
| ); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
In the catch block, returning false could lead to prematurely emitting an auto_mode_idle event if loadAllFeaturesFn fails transiently. This behavior is confirmed by the test handles loadAllFeaturesFn error gracefully (falls back to emitting idle). However, to better align with the goal of preventing premature idle events, it would be more robust to return true in case of an error. This would prevent the system from becoming idle due to a temporary failure in loading features, making the auto-loop more resilient.
} catch (error) {
const errorInfo = classifyError(error);
logger.warn(
`Failed to load all features for idle check (projectPath=${projectPath}, branchName=${branchName}): ${errorInfo.message}`,
error
);
// Return true to be pessimistic and prevent premature idle event on transient error.
return true;
}
Comment on lines
344
to
348
| const effectiveBaseBranch = trimmedBaseBranch | ||
| ? selectedRemote !== 'local' && !trimmedBaseBranch.includes('/') | ||
| ? `${selectedRemote}/${trimmedBaseBranch}` | ||
| : trimmedBaseBranch | ||
| : undefined; |
Contributor
There was a problem hiding this comment.
This nested ternary for constructing effectiveBaseBranch is a bit dense and can be hard to read. For better maintainability and clarity, consider refactoring this logic into a more explicit if statement. This will make the intent clearer and reduce the chance of introducing bugs in the future.
let effectiveBaseBranch: string | undefined;
if (trimmedBaseBranch) {
if (selectedRemote !== 'local' && !trimmedBaseBranch.includes('/')) {
effectiveBaseBranch = `${selectedRemote}/${trimmedBaseBranch}`;
} else {
effectiveBaseBranch = trimmedBaseBranch;
}
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (4)
.gitignore (1)
74-75: Looks good; fixture artifact exclusion is correctly covered.Adding
test/fixtures/is appropriate and matches the PR goal of keeping generated test artifacts out of VCS. Optional cleanup:test/fixtures/.worker-*/is now redundant since the parent directory is already ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 74 - 75, The .gitignore now ignores the entire test/fixtures/ directory, so remove the redundant specific pattern `test/fixtures/.worker-*/` to avoid duplication; keep the `test/fixtures/` entry and delete the `test/fixtures/.worker-*/` line so only the parent directory pattern remains.apps/server/tests/unit/routes/running-agents.test.ts (1)
207-284: Test additions look good with minor redundancy.The new tests at lines 207-256 provide comprehensive coverage for multi-provider scenarios (claude, codex, cursor). The backward compatibility test at lines 258-284 partially overlaps with the existing test at lines 81-108, which already verifies
model: undefinedandprovider: undefined. Consider whether both backward compatibility tests are needed, or if one could be removed to reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/routes/running-agents.test.ts` around lines 207 - 284, The new backward-compatibility test "should handle agents without model information (backward compatibility)" duplicates an earlier test that already asserts model/provider are undefined; remove or consolidate the duplicate so only one test verifies legacy agents, keeping the multi-provider test ("should include model and provider information for running agents"); update references to mockAutoModeService.getRunningAgents and createIndexHandler in the remaining test as needed and ensure assertions still check response.runningAgents[0].model/provider and response.success.apps/ui/tests/features/feature-deep-link.spec.ts (1)
86-86: Replace fixed sleeps with condition-based waits to reduce flaky timing.Hardcoded
waitForTimeoutcalls can make these tests brittle and slower in CI.🔧 Suggested fix
- await page.waitForTimeout(2000); + // No fixed delay needed; the toPass block below already waits for card appearance. @@ - await page.keyboard.press('Escape'); - await page.waitForTimeout(500); + await page.keyboard.press('Escape'); + await expect(modal).toBeHidden({ timeout: 2000 }); @@ - await page.waitForTimeout(2000); - const modalVisible = await modal.isVisible(); - expect(modalVisible).toBe(false); + await expect(modal).toBeHidden({ timeout: 2000 }); @@ - await page.waitForTimeout(1000); - const modalVisible = await modal.isVisible(); - expect(modalVisible).toBe(false); + await expect(modal).toBeHidden({ timeout: 2000 });Also applies to: 111-112, 148-150, 172-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/feature-deep-link.spec.ts` at line 86, Replace the hardcoded sleeps (page.waitForTimeout) with condition-based waits to avoid flakes: find each usage of page.waitForTimeout (including the occurrences around lines referenced) and replace it with a targeted wait such as page.waitForSelector or page.waitForResponse or playwright's expect(locator).toBeVisible()/toHaveText() or page.waitForFunction that waits for the specific DOM element, network response, or state change your test needs; ensure you reference the element selector, API route, or condition used elsewhere in this spec so the test only proceeds when the actual condition is met instead of a fixed timeout.apps/server/tests/unit/services/settings-service.test.ts (1)
26-35: Use a typed endpoint helper instead ofanyto prevent silent schema drift in tests.The helper currently accepts/returns
any, which can hide type regressions in ntfy endpoint shape.💡 Proposed refactor
+import type { NtfyEndpointConfig } from '@/types/settings.js'; @@ - function createTestNtfyEndpoint(overrides: Partial<any> = {}): any { + function createTestNtfyEndpoint( + overrides: Partial<NtfyEndpointConfig> = {} + ): NtfyEndpointConfig { return { id: `endpoint-${Date.now()}-${Math.random().toString(36).slice(2, 7)}`, name: 'Test Endpoint', serverUrl: 'https://ntfy.sh', topic: 'test-topic', authType: 'none', enabled: true, ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/services/settings-service.test.ts` around lines 26 - 35, The helper createTestNtfyEndpoint currently uses untyped any which can mask schema drift; change its signature to accept overrides: Partial<NtfyEndpoint> and return type NtfyEndpoint (or the concrete ntfy endpoint interface used by the settings service tests), update the function body to satisfy that interface, and import the NtfyEndpoint/Endpoint type from the module that defines the ntfy endpoint shape so TypeScript will catch mismatches in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/auto-loop-coordinator.ts`:
- Around line 489-497: featureBelongsToWorktree currently treats the main
worktree by checking !feature.branchName, which misses features stored with
branchName === 'main'; update featureBelongsToWorktree to mirror
getWorktreeAutoLoopKey's normalization (treat branchName === null or branchName
=== 'main' as main): when the incoming branchName is null or 'main' return true
for features that have no branchName OR feature.branchName === 'main'; otherwise
keep the exact match path (feature.branchName === branchName). Use the function
name featureBelongsToWorktree and the same normalization pattern used around
getWorktreeAutoLoopKey.
In `@apps/server/src/services/ntfy-service.ts`:
- Around line 103-115: The try block in the ntfy send path creates an
AbortController and timeoutId but only calls clearTimeout(timeoutId) after a
successful fetch, leaving timers scheduled on errors/abort; update the request
logic in the ntfy service (where AbortController, timeoutId,
DEFAULT_NTFY_TIMEOUT and fetch are used) to ensure clearTimeout(timeoutId) is
invoked in all outcomes by moving clearTimeout into a finally block (or
otherwise calling it in both catch and after await), so the timeout is always
cleared even when fetch throws or is aborted.
In `@apps/server/tests/unit/services/auto-loop-coordinator.test.ts`:
- Around line 1265-1304: Rename the test description to match the assertion:
update the it(...) string currently "'does NOT emit auto_mode_idle when features
are in backlog or ready status'" to a positive description like "'emits
auto_mode_idle when only backlog/ready features exist and no running/in_progress
features'"; the test body using coordinator.startAutoLoopForProject,
coordinator.stopAutoLoopForProject and expecting
mockEventBus.emitAutoModeEvent('auto_mode_idle', ...) can remain unchanged.
In `@apps/server/tests/unit/services/feature-state-manager.test.ts`:
- Around line 1498-1506: The test currently wraps subscribeCallback in an async
arrow and uses .not.toThrow(), which doesn't work for promises and can miss
rejections; replace that pattern by directly awaiting the promise assertion:
call subscribeCallback('auto-mode:event', { type: 'auto_mode_error', ... }) and
assert it resolves (e.g. await
expect(subscribeCallback(...)).resolves.toBeUndefined() or another appropriate
resolved value) so rejected promises fail the test; update the test around the
subscribeCallback invocation accordingly.
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 1135-1136: The branchName prop currently coerces to an empty
string using "?? ''" when addFeatureUseSelectedWorktreeBranch is false; change
this to pass undefined instead so branchName is unset for workMode='current'
(consistent with handleAddFeature's finalBranchName logic and handleBulkUpdate
behavior). Update the two occurrences that use
(addFeatureUseSelectedWorktreeBranch ? selectedWorktreeBranch : undefined) ?? ''
to simply (addFeatureUseSelectedWorktreeBranch ? selectedWorktreeBranch :
undefined) so branchName is undefined when not provided; keep the rest of the
call sites (selectedWorktreeBranch, addFeatureUseSelectedWorktreeBranch,
branchName) unchanged.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx`:
- Line 88: The code uses realtimeSummary ?? task.summary which treats stored
null as absent, undoing "clear" operations; update the resolution logic to check
Map.has(task.id) instead of null-coalescing so a stored null is treated as an
explicit cleared summary. Locate usages around taskSummaryMap/setTaskSummaryMap
and replace patterns like `realtimeSummary ?? task.summary` with a check such as
`taskSummaryMap.has(task.id) ? realtimeSummary : task.summary` (apply the same
fix to the other occurrences around lines 203-207) so that a Map entry of null
preserves the cleared state.
In `@apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx`:
- Line 103: The dialog's expanded state variable is not cleared on reset: update
the resetState function in the create-pr-dialog component to also reset
isDescriptionExpanded by calling setIsDescriptionExpanded(false) alongside the
other state resets so the description is collapsed when the dialog is reopened;
reference the isDescriptionExpanded and setIsDescriptionExpanded hooks and
ensure resetState calls setIsDescriptionExpanded(false).
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Line 448: The collapsed base-branch label construction in CreateWorktreeDialog
is double-prefixing when baseBranch already includes the remote
(selectedRemote), so update the rendering logic that currently uses
{selectedRemote !== 'local' ? `${selectedRemote}/${baseBranch}` : baseBranch} to
conditionally prefix only when baseBranch does not already start with
`${selectedRemote}/`; use selectedRemote and baseBranch to guard with a
startsWith check and fall back to baseBranch when already qualified.
- Around line 341-348: Summary: Remote ref construction incorrectly skips
prepending the selected remote when the branch name contains a '/' (e.g.
feature/foo), causing refs like feature/foo instead of origin/feature/foo. Fix:
in the effectiveBaseBranch computation (variables trimmedBaseBranch,
selectedRemote, effectiveBaseBranch) replace the check
!trimmedBaseBranch.includes('/') with a check that the branch already starts
with the selected remote: !trimmedBaseBranch.startsWith(`${selectedRemote}/`) so
you only skip prepending when the branch is actually already qualified with the
chosen remote; otherwise prepend `${selectedRemote}/`.
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hook-dialog.tsx`:
- Around line 214-221: The current validation (isValid) only checks
ntfyEndpointId is non-empty, which allows saving stale IDs; update the
validation to verify that for actionType === 'ntfy' the chosen endpoint actually
exists in ntfyEndpoints by checking selectedEndpoint (or finding by
ntfyEndpointId) is truthy and its id matches ntfyEndpointId (and still trim the
id), so replace the ntfy branch in isValid with a check like
Boolean(selectedEndpoint) to prevent saving deleted/stale endpoints; ensure
selectedEndpoint is computed from ntfyEndpoints.find((e) => e.id ===
ntfyEndpointId) and used by the validation logic.
- Around line 267-277: The tab trigger spans currently use "hidden sm:inline"
which visually and accessibly hides the label on small viewports; update each
TabsTrigger (values "shell", "http", "ntfy") to keep labels available to
assistive tech by replacing the span className "hidden sm:inline" with "sr-only
sm:inline" (or alternatively add an appropriate aria-label on each TabsTrigger)
so the text is still exposed to screen readers on small screens while remaining
visually hidden until sm breakpoint.
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx`:
- Around line 435-441: The endpoint card root div in EventHooksSection (the div
that builds its className via cn('flex items-center gap-3 p-3 rounded-lg
border', 'bg-background/50 hover:bg-background/80 transition-colors',
!endpoint.enabled && 'opacity-60')) needs a data-testid to satisfy the
Playwright test; add data-testid="endpoint-card" to that root element so the
test can reliably select existing endpoint cards (keep the attribute regardless
of endpoint.enabled).
- Around line 510-559: Editing endpoints currently overwrite secrets with blank
values and the form allows saving invalid auth configs; update the save and
validation logic so that handleSave preserves existing secrets when
editingEndpoint exists and the password/token inputs are left blank (use
editingEndpoint.password or editingEndpoint.token rather than empty strings),
only include username/password/token in the NtfyEndpointConfig when authType
requires them and values are non-empty after trim, and tighten isValid to
require a non-empty username+password for authType === 'basic' and a non-empty
token for authType === 'token' (in addition to the existing name/topic checks);
reference handleSave, editingEndpoint, setPassword/setToken usage,
NtfyEndpointConfig construction, and isValid for where to change logic.
In `@apps/ui/src/hooks/use-agent-output-websocket.ts`:
- Line 101: Replace the unsafe cast when subscribing to api.backlogPlan.onEvent
by adding a runtime type guard that verifies the incoming unknown is a valid
BacklogPlanEvent before calling handleBacklogPlanEvent; modify the subscription
line that assigns unsubscribe = api.backlogPlan.onEvent(...) to pass a wrapper
callback which checks the payload shape (e.g., required fields used by
formatBacklogPlanEventContent) and only calls handleBacklogPlanEvent(event) when
the guard returns true, otherwise log or ignore the malformed event to avoid
runtime failures in formatBacklogPlanEventContent.
In `@apps/ui/src/lib/utils.ts`:
- Around line 107-116: The display mapping in apps/ui/src/lib/utils.ts is
missing canonical Claude IDs so getModelDisplayName falls back to raw strings;
update the displayNames Record (and/or the normalization logic inside
getModelDisplayName) to include keys 'claude-haiku', 'claude-sonnet', and
'claude-opus' mapping to 'Claude Haiku', 'Claude Sonnet', and 'Claude Opus'
respectively so canonical IDs resolve to friendly names; locate the displayNames
object and add those entries (or add a normalization step in getModelDisplayName
that maps canonical IDs to the existing alias/full-ID entries).
In `@apps/ui/src/main.ts`:
- Line 59: Remove the use of the undocumented internal API call on app (the
expression casting to { setDesktopName(...) } and calling
setDesktopName('automaker.desktop')) and instead add a "desktopName":
"automaker.desktop" entry to the shipped package.json; update packaging/build
config if needed so the packaged app uses that package.json field. Locate the
offending call by searching for the symbol setDesktopName and the app cast in
main.ts, delete that line, and ensure the project’s package.json contains the
desktopName key so Linux desktop integration uses the documented mechanism.
In `@apps/ui/tests/features/feature-deep-link.spec.ts`:
- Around line 69-70: authenticateForTests returns a boolean but its result is
ignored; update each setup to capture the return value (e.g., const ok = await
authenticateForTests(page)) and assert it succeeded before continuing (either
throw an error or call your test assertion like expect(ok).toBe(true)) so
failures stop early; apply this change to the occurrences around
authenticateForTests in the feature-deep-link.spec.ts setup blocks (the calls on
and near lines referencing authenticateForTests).
- Around line 104-116: The deep-link URL is constructed by directly
interpolating featureId into page.goto which can break if the ID contains
URL-reserved characters; update the call that builds the URL (where featureId is
derived from cardTestId and passed to page.goto) to use an encoded value (e.g.,
replace the current `/board?featureId=${featureId}` usage with
`/board?featureId=${encodeURIComponent(featureId)}`) so the featureId is safely
URL-encoded before navigation.
---
Nitpick comments:
In @.gitignore:
- Around line 74-75: The .gitignore now ignores the entire test/fixtures/
directory, so remove the redundant specific pattern `test/fixtures/.worker-*/`
to avoid duplication; keep the `test/fixtures/` entry and delete the
`test/fixtures/.worker-*/` line so only the parent directory pattern remains.
In `@apps/server/tests/unit/routes/running-agents.test.ts`:
- Around line 207-284: The new backward-compatibility test "should handle agents
without model information (backward compatibility)" duplicates an earlier test
that already asserts model/provider are undefined; remove or consolidate the
duplicate so only one test verifies legacy agents, keeping the multi-provider
test ("should include model and provider information for running agents");
update references to mockAutoModeService.getRunningAgents and createIndexHandler
in the remaining test as needed and ensure assertions still check
response.runningAgents[0].model/provider and response.success.
In `@apps/server/tests/unit/services/settings-service.test.ts`:
- Around line 26-35: The helper createTestNtfyEndpoint currently uses untyped
any which can mask schema drift; change its signature to accept overrides:
Partial<NtfyEndpoint> and return type NtfyEndpoint (or the concrete ntfy
endpoint interface used by the settings service tests), update the function body
to satisfy that interface, and import the NtfyEndpoint/Endpoint type from the
module that defines the ntfy endpoint shape so TypeScript will catch mismatches
in tests.
In `@apps/ui/tests/features/feature-deep-link.spec.ts`:
- Line 86: Replace the hardcoded sleeps (page.waitForTimeout) with
condition-based waits to avoid flakes: find each usage of page.waitForTimeout
(including the occurrences around lines referenced) and replace it with a
targeted wait such as page.waitForSelector or page.waitForResponse or
playwright's expect(locator).toBeVisible()/toHaveText() or page.waitForFunction
that waits for the specific DOM element, network response, or state change your
test needs; ensure you reference the element selector, API route, or condition
used elsewhere in this spec so the test only proceeds when the actual condition
is met instead of a fixed timeout.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
.gitignoreapps/server/src/services/auto-loop-coordinator.tsapps/server/src/services/event-hook-service.tsapps/server/src/services/feature-state-manager.tsapps/server/src/services/ntfy-service.tsapps/server/src/services/settings-service.tsapps/server/tests/unit/routes/running-agents.test.tsapps/server/tests/unit/services/auto-loop-coordinator.test.tsapps/server/tests/unit/services/auto-mode/facade-agent-runner.test.tsapps/server/tests/unit/services/event-hook-service.test.tsapps/server/tests/unit/services/feature-state-manager.test.tsapps/server/tests/unit/services/ntfy-service.test.tsapps/server/tests/unit/services/settings-service.test.tsapps/ui/src/components/layout/project-switcher/components/notification-bell.tsxapps/ui/src/components/session-manager.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsxapps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsxapps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown-item.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsxapps/ui/src/components/views/notifications-view.tsxapps/ui/src/components/views/project-settings-view/project-bulk-replace-dialog.tsxapps/ui/src/components/views/running-agents-view.tsxapps/ui/src/components/views/settings-view/event-hooks/event-hook-dialog.tsxapps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsxapps/ui/src/components/views/settings-view/model-defaults/bulk-replace-dialog.tsxapps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsxapps/ui/src/hooks/queries/use-worktrees.tsapps/ui/src/hooks/use-agent-output-websocket.tsapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/log-parser.tsapps/ui/src/lib/utils.tsapps/ui/src/main.tsapps/ui/src/routes/board.lazy.tsxapps/ui/src/routes/board.tsxapps/ui/src/store/app-store.tsapps/ui/src/store/types/state-types.tsapps/ui/src/types/electron.d.tsapps/ui/tests/agent/start-new-chat-session.spec.tsapps/ui/tests/features/feature-deep-link.spec.tsapps/ui/tests/settings/event-hooks-settings.spec.tslibs/types/src/index.tslibs/types/src/notification.tslibs/types/src/settings.tstest/fixtures/projectA
💤 Files with no reviewable changes (1)
- test/fixtures/projectA
Comment on lines
1498
to
1506
| // Should not throw - call the async function and verify it doesn't throw | ||
| await expect( | ||
| async () => | ||
| await subscribeCallback('auto-mode:event', { | ||
| type: 'auto_mode_error', | ||
| message: 'Error', | ||
| projectPath: '/project', | ||
| }) | ||
| ).not.toThrow(); |
Contributor
There was a problem hiding this comment.
Async assertion here is misleading and can pass on rejected promises.
For async callbacks, toThrow() is not the right matcher pattern. This test can pass even if subscribeCallback rejects.
✅ Assertion fix
- await expect(
- async () =>
- await subscribeCallback('auto-mode:event', {
- type: 'auto_mode_error',
- message: 'Error',
- projectPath: '/project',
- })
- ).not.toThrow();
+ await expect(
+ (async () => {
+ await subscribeCallback('auto-mode:event', {
+ type: 'auto_mode_error',
+ message: 'Error',
+ projectPath: '/project',
+ });
+ })()
+ ).resolves.toBeUndefined();📝 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
| // Should not throw - call the async function and verify it doesn't throw | |
| await expect( | |
| async () => | |
| await subscribeCallback('auto-mode:event', { | |
| type: 'auto_mode_error', | |
| message: 'Error', | |
| projectPath: '/project', | |
| }) | |
| ).not.toThrow(); | |
| // Should not throw - call the async function and verify it doesn't throw | |
| await expect( | |
| (async () => { | |
| await subscribeCallback('auto-mode:event', { | |
| type: 'auto_mode_error', | |
| message: 'Error', | |
| projectPath: '/project', | |
| }); | |
| })() | |
| ).resolves.toBeUndefined(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/tests/unit/services/feature-state-manager.test.ts` around lines
1498 - 1506, The test currently wraps subscribeCallback in an async arrow and
uses .not.toThrow(), which doesn't work for promises and can miss rejections;
replace that pattern by directly awaiting the promise assertion: call
subscribeCallback('auto-mode:event', { type: 'auto_mode_error', ... }) and
assert it resolves (e.g. await
expect(subscribeCallback(...)).resolves.toBeUndefined() or another appropriate
resolved value) so rejected promises fail the test; update the test around the
subscribeCallback invocation accordingly.
apps/ui/src/main.ts
Outdated
| // desktop entry's Icon for the taskbar instead of Electron's default. | ||
| // Must be called before any window is created. | ||
| app.setDesktopName('automaker.desktop'); | ||
| (app as unknown as { setDesktopName(name: string): void }).setDesktopName('automaker.desktop'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and check Electron version
find . -name "package.json" -type f | head -5 | xargs grep -l electron | head -1Repository: AutoMaker-Org/automaker
Length of output: 89
🏁 Script executed:
# Read the actual code context around line 59
cat -n apps/ui/src/main.ts | sed -n '50,70p'Repository: AutoMaker-Org/automaker
Length of output: 1033
🏁 Script executed:
# Check if there are type definition files or declarations for Electron
find . -type f -name "*.d.ts" | xargs grep -l "setDesktopName" 2>/dev/null | head -5Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Get Electron version from package.json
cat apps/ui/package.json | grep -A 2 -B 2 "electron"Repository: AutoMaker-Org/automaker
Length of output: 2114
🌐 Web query:
Electron 39 setDesktopName API Linux app documentation
💡 Result:
Electron (including v39) does not have an app.setDesktopName() / setDesktopName API. Instead, the Linux “desktop file name” Electron uses is configured via the desktopName field in your app’s embedded package.json. (docs.flatpak.org)
What to do (Linux)
Add this to your app’s package.json (the one shipped inside your app bundle / app.asar):
{
"desktopName": "com.example.MyApp.desktop"
}Electron otherwise defaults to {appname}.desktop. (docs.flatpak.org)
Why it matters
Using the correct desktop filename is important on Linux (e.g., Flatpak requires the desktop file name to match the Flatpak app id, and using the wrong name can cause issues like missing window icons under Wayland). (docs.flatpak.org)
It’s also tied to certain Linux integrations (e.g., progress/badge features implemented via UnityLauncherAPI expect a matching .desktop identity). (docs.flatpak.org)
Citations:
- 1: https://docs.flatpak.org/ko/latest/electron.html?utm_source=openai
- 2: https://docs.flatpak.org/ko/latest/electron.html?utm_source=openai
- 3: https://docs.flatpak.org/ko/latest/electron.html?utm_source=openai
- 4: https://docs.flatpak.org/zh_CN/latest/electron.html?utm_source=openai
🌐 Web query:
Electron app.setDesktopName method availability supported versions
💡 Result:
app.setDesktopName(name) exists in Electron, but it’s effectively an internal / undocumented API (it does not appear in the current public app API docs) and therefore not guaranteed stable across releases. It is present in Electron’s internal typings (internal-electron.d.ts) as an internal App method. [1]
What it does / platforms
In Electron’s implementation, SetDesktopName() only does anything on Linux: it sets the CHROME_DESKTOP environment variable. [2]
Supported Electron versions (confirmed)
From publicly visible Electron source/patch contexts, SetDesktopName is present in:
- Electron 22.x (e.g., FreeBSD’s Electron 22 port patch context includes
App::SetDesktopName). [3] - Electron 32.x / 37.x (multiple FreeBSD port update contexts still include
App::SetDesktopName). [4]
So, it’s available in at least Electron 22 → 37 (and likely earlier), but because it’s internal/undocumented you should treat it as unsupported for compatibility guarantees. [1]
Practical recommendation
If your goal is correct desktop integration on Linux, prefer setting the correct .desktop file metadata (often via your packager) rather than relying on this method, since it may change or disappear without a formal breaking-change notice. [1]
Sources: [1] [2] [3] [4]
Replace with documented desktopName in package.json instead of internal Electron API.
setDesktopName() is an internal, undocumented Electron API not present in public type definitions. The forced type cast bypasses type safety and depends on unstable internals that may change across versions. The recommended approach for Linux desktop integration is to set desktopName in the shipped package.json:
{
"desktopName": "automaker.desktop"
}This is the supported, stable method that survives Electron version upgrades.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/main.ts` at line 59, Remove the use of the undocumented internal
API call on app (the expression casting to { setDesktopName(...) } and calling
setDesktopName('automaker.desktop')) and instead add a "desktopName":
"automaker.desktop" entry to the shipped package.json; update packaging/build
config if needed so the packaged app uses that package.json field. Locate the
offending call by searching for the symbol setDesktopName and the app cast in
main.ts, delete that line, and ensure the project’s package.json contains the
desktopName key so Linux desktop integration uses the documented mechanism.
Comment on lines
+777
to
+783
| /** Username for basic auth (required if authType === 'basic') */ | ||
| username?: string; | ||
| /** Password for basic auth (required if authType === 'basic') */ | ||
| password?: string; | ||
| /** Access token (required if authType === 'token') */ | ||
| token?: string; | ||
| /** Default tags for notifications (comma-separated emoji codes) */ |
Contributor
There was a problem hiding this comment.
Ntfy credentials are modeled in general settings (plaintext risk).
password and token inside NtfyEndpointConfig place secrets in GlobalSettings flow. This conflicts with the file’s own separation of sensitive secrets into Credentials storage and increases leakage risk.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx (1)
341-348: Consider extracting remote-qualification logic into a shared helper.The same prefixing rule is implemented here and again in the collapsed label render, which can drift over time.
♻️ Suggested refactor
+ const qualifyBaseBranchWithRemote = useCallback( + (value: string) => + selectedRemote !== 'local' && !value.startsWith(`${selectedRemote}/`) + ? `${selectedRemote}/${value}` + : value, + [selectedRemote] + ); + // Pass the validated baseBranch if one was selected (otherwise defaults to HEAD). // When a remote is selected, prepend the remote name to form the full ref // (e.g. "main" with remote "origin" becomes "origin/main"). const effectiveBaseBranch = trimmedBaseBranch - ? selectedRemote !== 'local' && !trimmedBaseBranch.startsWith(`${selectedRemote}/`) - ? `${selectedRemote}/${trimmedBaseBranch}` - : trimmedBaseBranch + ? qualifyBaseBranchWithRemote(trimmedBaseBranch) : undefined;- {selectedRemote !== 'local' && !baseBranch.startsWith(`${selectedRemote}/`) - ? `${selectedRemote}/${baseBranch}` - : baseBranch} + {qualifyBaseBranchWithRemote(baseBranch)}🤖 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/dialogs/create-worktree-dialog.tsx` around lines 341 - 348, The remote-qualification logic that builds the full ref (currently in CreateWorktreeDialog when computing effectiveBaseBranch using trimmedBaseBranch and selectedRemote) should be extracted into a shared helper (e.g., qualifyRemoteBranch(remote: string, branch?: string): string | undefined) and used both where effectiveBaseBranch is computed and in the collapsed label render; update the code to call qualifyRemoteBranch(selectedRemote, trimmedBaseBranch) instead of duplicating the conditional, ensure the helper preserves the existing rules (no-op for 'local', avoid double-prefix if branch already starts with `${remote}/`, and return undefined when branch is empty).apps/ui/tests/features/feature-deep-link.spec.ts (1)
87-87: Replace condition checks at lines 150–152 and 175–177 with Playwright's assertion helpers.Lines 150–152 and 175–177 follow a pattern of sleeping before checking if a modal is visible. Replace these with
await expect(modal).toBeHidden({ timeout: ... })for more reliable waits:Suggested fix
const modal = page.locator('[data-testid="agent-output-modal"]'); - await page.waitForTimeout(2000); - const modalVisible = await modal.isVisible(); - expect(modalVisible).toBe(false); + await expect(modal).toBeHidden({ timeout: 2000 });const modal = page.locator('[data-testid="agent-output-modal"]'); - await page.waitForTimeout(1000); - const modalVisible = await modal.isVisible(); - expect(modalVisible).toBe(false); + await expect(modal).toBeHidden({ timeout: 1000 });Line 87's sleep serves a different purpose (allowing feature creation to complete) and should be left in place or replaced with a wait on the actual feature card element if flakiness is observed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/feature-deep-link.spec.ts` at line 87, Replace the manual sleep-and-check pattern around the modal visibility (the blocks referenced as lines 150–152 and 175–177) with Playwright's assertion helper by awaiting expect(modal).toBeHidden({ timeout: <reasonable_ms> }) and remove the preceding sleep/if visibility checks; keep the existing await page.waitForTimeout(2000); at line 87 (or replace it with an explicit wait for the feature card element if you prefer) so feature creation still completes before proceeding.
🤖 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.tsx`:
- Line 310: handledFeatureIdRef currently remains set after the first deep-link
is handled so reopening the same featureId never triggers the modal again; reset
handledFeatureIdRef.current = undefined when the feature context changes and
when the modal is closed. Specifically, in the effect or logic that watches
featureId (the code around where handledFeatureIdRef is read to decide to open
the modal) set handledFeatureIdRef.current = undefined when featureId becomes
undefined or changes, and also clear it in the modal close handler (e.g., the
function that closes the feature modal / onCloseFeatureModal or closeFeature
handler) so the same featureId can be handled again on a subsequent navigation.
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx`:
- Around line 465-476: The icon-only action controls (the Switch and the two
Buttons rendering Pencil and Trash2) lack accessible names; update the Switch
component (used with endpoint.enabled and onToggle) and the Buttons (onEdit,
onDelete) to include explicit accessible labels (e.g., aria-label or
aria-labelledby) that describe the action and include identifying context (like
endpoint.name or endpoint.id) so screen readers can distinguish "Enable endpoint
X", "Edit endpoint X", and "Delete endpoint X"; ensure labels are unique and
consider adding a visually-hidden text alternative if aria-label is not
supported by your Button/Switch primitives.
- Around line 567-575: The current isValid check (used to enable saving in the
event hooks form) omits validating serverUrl, so add a serverUrl validity check
to the same boolean: require serverUrl.trim().length > 0 and a simple URL
validity test (e.g., try new URL(serverUrl) or ensure it starts with "http" /
contains a host) before allowing save; update the isValid expression
(referencing isValid, serverUrl, name, topic, authType, username, password,
token, editingEndpoint) to include this serverUrl validation so empty/invalid
server URLs cannot be persisted.
In `@apps/ui/src/lib/utils.ts`:
- Around line 228-241: formatRelativeTime currently treats future dates as "just
now" because diffSec can be negative; add a guard at the top of the function
(before the diffSec < 60 check) that checks if diffSec < 0 and returns a
sensible value (e.g., date.toLocaleDateString() or a short "in the future"
string) so future timestamps aren't misreported; update the logic around the
diffSec variable in formatRelativeTime to handle this case.
---
Nitpick comments:
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Around line 341-348: The remote-qualification logic that builds the full ref
(currently in CreateWorktreeDialog when computing effectiveBaseBranch using
trimmedBaseBranch and selectedRemote) should be extracted into a shared helper
(e.g., qualifyRemoteBranch(remote: string, branch?: string): string | undefined)
and used both where effectiveBaseBranch is computed and in the collapsed label
render; update the code to call qualifyRemoteBranch(selectedRemote,
trimmedBaseBranch) instead of duplicating the conditional, ensure the helper
preserves the existing rules (no-op for 'local', avoid double-prefix if branch
already starts with `${remote}/`, and return undefined when branch is empty).
In `@apps/ui/tests/features/feature-deep-link.spec.ts`:
- Line 87: Replace the manual sleep-and-check pattern around the modal
visibility (the blocks referenced as lines 150–152 and 175–177) with
Playwright's assertion helper by awaiting expect(modal).toBeHidden({ timeout:
<reasonable_ms> }) and remove the preceding sleep/if visibility checks; keep the
existing await page.waitForTimeout(2000); at line 87 (or replace it with an
explicit wait for the feature card element if you prefer) so feature creation
still completes before proceeding.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.gitignoreapps/server/package.jsonapps/server/src/services/auto-loop-coordinator.tsapps/server/src/services/ntfy-service.tsapps/server/tests/unit/routes/running-agents.test.tsapps/server/tests/unit/services/auto-loop-coordinator.test.tsapps/server/tests/unit/services/feature-state-manager.test.tsapps/server/tests/unit/services/settings-service.test.tsapps/ui/package.jsonapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsxapps/ui/src/components/views/board-view/components/selection-action-bar.tsxapps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsxapps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsxapps/ui/src/components/views/settings-view/event-hooks/event-hook-dialog.tsxapps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsxapps/ui/src/hooks/use-agent-output-websocket.tsapps/ui/src/lib/utils.tsapps/ui/src/main.tsapps/ui/tests/features/feature-deep-link.spec.tspackage.json
💤 Files with no reviewable changes (1)
- apps/ui/src/main.ts
✅ Files skipped from review due to trivial changes (2)
- apps/server/package.json
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx
- .gitignore
- apps/ui/src/components/views/settings-view/event-hooks/event-hook-dialog.tsx
apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx
Show resolved
Hide resolved
* 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/hooks/use-settings-migration.ts (1)
178-203:⚠️ Potential issue | 🟠 MajorNew migrated fields can be dropped during merge-based migration.
These fields are now parsed/hydrated/synced, but
mergeSettings()still does not merge them from localStorage in the first-time merge path, so local values can be lost before sync-back.💡 Proposed fix
export function mergeSettings( serverSettings: GlobalSettings, localSettings: Partial<GlobalSettings> | null ): GlobalSettings { @@ if ( (!serverSettings.eventHooks || serverSettings.eventHooks.length === 0) && localSettings.eventHooks && localSettings.eventHooks.length > 0 ) { merged.eventHooks = localSettings.eventHooks; } + if ( + (!serverSettings.ntfyEndpoints || serverSettings.ntfyEndpoints.length === 0) && + localSettings.ntfyEndpoints && + localSettings.ntfyEndpoints.length > 0 + ) { + merged.ntfyEndpoints = localSettings.ntfyEndpoints; + } + + if ( + (!serverSettings.featureTemplates || serverSettings.featureTemplates.length === 0) && + localSettings.featureTemplates && + localSettings.featureTemplates.length > 0 + ) { + merged.featureTemplates = localSettings.featureTemplates; + } + + if ( + (!serverSettings.knownDynamicModelIds || serverSettings.knownDynamicModelIds.length === 0) && + localSettings.knownDynamicModelIds && + localSettings.knownDynamicModelIds.length > 0 + ) { + merged.knownDynamicModelIds = localSettings.knownDynamicModelIds; + } + + if ( + localSettings.defaultSortNewestCardOnTop != null && + merged.defaultSortNewestCardOnTop == null + ) { + merged.defaultSortNewestCardOnTop = localSettings.defaultSortNewestCardOnTop; + } + // Preserve new settings fields from localStorage if server has defaults // Use nullish coalescing to accept stored falsy values (e.g. false)Also applies to: 815-817, 926-957
🤖 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 178 - 203, The listed new settings (e.g., defaultSortNewestCardOnTop, enhancementModel, validationModel, phaseModels, enabledCursorModels, cursorDefaultModel, enabledOpencodeModels, opencodeDefaultModel, enabledDynamicModelIds, disabledProviders, autoLoadClaudeMd, useClaudeCodeSystemPrompt, codex* fields, keyboardShortcuts, mcpServers, promptCustomization, eventHooks, ntfyEndpoints, featureTemplates) are parsed but not merged during the first-time merge path, so local values can be dropped; update mergeSettings() (and the first-time merge logic in use-settings-migration.ts) to include these properties in the merge-from-localStorage step (i.e., explicitly copy/merge each of the above fields from state/localStorage into the merged result) so new migrated fields are preserved before sync-back.
♻️ Duplicate comments (2)
libs/types/src/settings.ts (1)
766-783:⚠️ Potential issue | 🟠 MajorAvoid storing ntfy credentials in
GlobalSettings.At Line 779 and Line 782, endpoint secrets (
password/token) are part ofNtfyEndpointConfig, and at Line 1499 that config is stored under global settings. This increases plaintext leakage risk in settings sync/storage paths; keep secrets inCredentialsand store only references in settings.Also applies to: 1493-1500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/src/settings.ts` around lines 766 - 783, NtfyEndpointConfig currently contains secret fields (password, token) that are being persisted into GlobalSettings; move secret storage out of NtfyEndpointConfig and into the credentials system instead: remove password and token from NtfyEndpointConfig, replace them with non-secret references (e.g., credentialId or secretRef), update any code that writes/reads GlobalSettings to store only the reference, and add logic in the credentials manager (Credentials) to persist/retrieve the actual password/token; update all usages of NtfyEndpointConfig (creation, validation, and consumers) to resolve secrets via the new reference rather than reading password/token directly.apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx (1)
579-588:⚠️ Potential issue | 🟠 MajorRestrict endpoint
serverUrlto HTTP(S) protocols.Line 583 currently accepts any valid URL scheme. That still allows invalid ntfy endpoint configs (e.g., non-http(s)) to pass form validation.
💡 Proposed fix
const isServerUrlValid = (() => { const trimmed = serverUrl.trim(); if (!trimmed) return false; try { - new URL(trimmed); - return true; + const parsed = new URL(trimmed); + return parsed.protocol === 'http:' || parsed.protocol === 'https:'; } catch { return false; } })();#!/bin/bash # Verify whether serverUrl validation enforces protocol restrictions. rg -n -C3 "isServerUrlValid|new URL\\(trimmed\\)|protocol === 'http:'|protocol === 'https:'" \ apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx` around lines 579 - 588, The current isServerUrlValid validation uses new URL(trimmed) but accepts any scheme; update isServerUrlValid to parse the URL (new URL(trimmed)) and then ensure the URL.protocol is either 'http:' or 'https:' before returning true, returning false for other protocols or parsing errors; reference the isServerUrlValid constant, serverUrl variable, and the new URL(trimmed) call when making this change.
🧹 Nitpick comments (3)
apps/ui/tests/features/feature-deep-link.spec.ts (2)
87-87: Replace hard wait with a condition-based wait.Using
waitForTimeout(2000)is discouraged in Playwright as it introduces unnecessary delays and can still be flaky if the operation takes longer. The subsequenttoPass()block on lines 90-96 already waits for the feature card to appear, so this hard wait may be redundant.♻️ Suggested improvement
Remove the hard wait since the following
toPass()assertion already handles waiting for the feature card:await confirmAddFeature(page); - await page.waitForTimeout(2000); // Wait for the feature to appear in the backlog🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/feature-deep-link.spec.ts` at line 87, Remove the hard sleep await page.waitForTimeout(2000); in the feature-deep-link.spec.ts test and rely on the existing condition-based wait instead (the subsequent toPass() block that asserts the feature card visibility), or if an explicit wait is needed use Playwright's waitForSelector/waitForLoadState tied to the same selector rather than a fixed timeout; update the test to remove the redundant waitForTimeout call and ensure the feature card assertion (the toPass() block) is the source of truth for waiting.
108-113: Consider using a more robust modal dismissal pattern.The
waitForTimeout(500)after pressing Escape can be flaky. Instead, wait for the modal to become hidden.♻️ Suggested improvement
// Close any open modals first const modal = page.locator('[data-testid="agent-output-modal"]'); if (await modal.isVisible()) { await page.keyboard.press('Escape'); - await page.waitForTimeout(500); + await modal.waitFor({ state: 'hidden', timeout: 2000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/features/feature-deep-link.spec.ts` around lines 108 - 113, Replace the flaky fixed timeout after pressing Escape with an explicit wait for the modal to be hidden: after calling page.keyboard.press('Escape') on the '[data-testid="agent-output-modal"]' locator (modal), await the modal becoming hidden (e.g., use modal.waitFor({ state: 'hidden' }) or page.waitForSelector('[data-testid="agent-output-modal"]', { state: 'hidden' }) ) instead of await page.waitForTimeout(500) so the test only proceeds once the modal is actually dismissed.apps/ui/src/store/types/state-types.ts (1)
632-633: Track the async action harmonization TODO with an issue.This TODO sits in a central contract file; consider linking it to a tracked issue to avoid long-term API inconsistency across providers.
If you want, I can draft a small follow-up issue template enumerating the remaining action signatures to standardize.
🤖 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 632 - 633, Replace the inline TODO about harmonizing provider action types with a tracked issue reference: create an issue in your tracker listing the action signatures to standardize (include setCursorDefaultModel, toggleCursorModel, setGeminiDefaultModel and any other provider actions), capture the desired change to return Promise<void>, and then update the TODO comment to reference that issue ID/URL (e.g., "tracked in ISSUE-ID") so the contract file points to the follow-up work rather than an untracked note. Ensure the issue includes a concise checklist of the functions to change and the expected API behavior for async persistence.
🤖 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`:
- Line 13: The import of FeatureLoader uses a relative service path; replace it
with the canonical shared-package import (import type { FeatureLoader } from
'@automaker/feature-loader') so the module is pulled from the shared package
namespace instead of the local services path; update the import statement
referencing FeatureLoader accordingly and ensure any tooling/tsconfig aliases
still resolve the '@automaker/feature-loader' package.
In `@apps/ui/src/components/views/board-view/components/list-view/list-view.tsx`:
- Around line 245-248: The header's displayed sort is out of sync because rows
use effectiveSortConfig (computed from sortNewestCardOnTop and sortConfig) but
ListHeader is still receiving the original sortConfig; update the component that
renders ListHeader to pass effectiveSortConfig (or a derived value from it)
instead of sortConfig so ListHeader's active column/direction reflects the
forced { column: 'createdAt', direction: 'desc' } when sortNewestCardOnTop is
true; ensure any handlers that update sort also reconcile with
effectiveSortConfig to keep UI and data ordering consistent (look for
effectiveSortConfig, sortNewestCardOnTop, sortConfig, and ListHeader in the
file).
In `@apps/ui/src/components/views/board-view/components/selection-action-bar.tsx`:
- Around line 57-68: The verify dialog is being closed unconditionally in the
finally block of handleConfirmVerify, which loses context on failure; change the
flow to await onVerify inside try, on success call setShowVerifyDialog(false),
in catch preserve the dialog (do not call setShowVerifyDialog(false)) and
optionally surface the error, and keep setIsVerifying(false) in a finally so
loading state is always cleared; update references in handleConfirmVerify to use
try { await onVerify(); setShowVerifyDialog(false); } catch (err) { /* keep
dialog open / set error state */ } finally { setIsVerifying(false); } to
implement this behavior.
In `@apps/ui/src/components/views/board-view/shared/model-selector.tsx`:
- Around line 164-173: When switching to the 'opencode' provider the current
code only calls onModelSelect(defaultModelId) if a model exists, so clicking
OpenCode does nothing when allOpencodeModels is empty; update the branch
handling provider === 'opencode' (and selectedProvider !== 'opencode') to always
switch the provider (call onProviderSelect('opencode')) first, then attempt to
pick a model as now (compute isDefaultModelAvailable/defaultModelId and call
onModelSelect(defaultModelId) if present), and if no models exist call
onModelSelect(null) or a dedicated clearModel handler so the UI enters OpenCode
mode and can render its empty/error state. Ensure you reference and use
opencodeDefaultModel, allOpencodeModels, onModelSelect, and onProviderSelect in
the change.
- Around line 42-48: The component currently uses two separate fetch paths
(useOpencodeModels and fetchOpencodeModels) which causes state drift between the
React Query data and the store-managed auto-enable/known-ID logic
(enabledDynamicModelIds, knownDynamicModelIds, dynamicOpencodeModels).
Consolidate into a single source of truth by either: (A) moving the auto-enable
and known-ID side effects into the React Query hook useOpencodeModels so it
updates the store (call the existing store actions that fetch/update
enabledDynamicModelIds and knownDynamicModelIds inside the hook), or (B) have
useOpencodeModels read from the store’s fetchOpencodeModels result (i.e., wrap
the store action in a react-query wrapper) and ensure opencode-settings-tab uses
the same unified path; update all callers (model-selector.tsx,
opencode-settings-tab) to consume only that unified API so auto-enable and
known-ID tracking always run.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Around line 720-724: persistedKnownDynamicModelIds may be non-array and
calling .filter will throw; wrap the value with an array guard so
sanitizedKnownDynamicModelIds only runs filter on an actual array — e.g. coerce
persistedKnownDynamicModelIds to
Array.isArray(serverSettings.knownDynamicModelIds) ?
serverSettings.knownDynamicModelIds : currentAppState.knownDynamicModelIds (or
fallback to []), then compute sanitizedKnownDynamicModelIds by filtering that
safe array; update the variables referenced (persistedKnownDynamicModelIds,
sanitizedKnownDynamicModelIds, serverSettings.knownDynamicModelIds,
currentAppState.knownDynamicModelIds) in use-settings-sync.ts accordingly.
---
Outside diff comments:
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Around line 178-203: The listed new settings (e.g.,
defaultSortNewestCardOnTop, enhancementModel, validationModel, phaseModels,
enabledCursorModels, cursorDefaultModel, enabledOpencodeModels,
opencodeDefaultModel, enabledDynamicModelIds, disabledProviders,
autoLoadClaudeMd, useClaudeCodeSystemPrompt, codex* fields, keyboardShortcuts,
mcpServers, promptCustomization, eventHooks, ntfyEndpoints, featureTemplates)
are parsed but not merged during the first-time merge path, so local values can
be dropped; update mergeSettings() (and the first-time merge logic in
use-settings-migration.ts) to include these properties in the
merge-from-localStorage step (i.e., explicitly copy/merge each of the above
fields from state/localStorage into the merged result) so new migrated fields
are preserved before sync-back.
---
Duplicate comments:
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx`:
- Around line 579-588: The current isServerUrlValid validation uses new
URL(trimmed) but accepts any scheme; update isServerUrlValid to parse the URL
(new URL(trimmed)) and then ensure the URL.protocol is either 'http:' or
'https:' before returning true, returning false for other protocols or parsing
errors; reference the isServerUrlValid constant, serverUrl variable, and the new
URL(trimmed) call when making this change.
In `@libs/types/src/settings.ts`:
- Around line 766-783: NtfyEndpointConfig currently contains secret fields
(password, token) that are being persisted into GlobalSettings; move secret
storage out of NtfyEndpointConfig and into the credentials system instead:
remove password and token from NtfyEndpointConfig, replace them with non-secret
references (e.g., credentialId or secretRef), update any code that writes/reads
GlobalSettings to store only the reference, and add logic in the credentials
manager (Credentials) to persist/retrieve the actual password/token; update all
usages of NtfyEndpointConfig (creation, validation, and consumers) to resolve
secrets via the new reference rather than reading password/token directly.
---
Nitpick comments:
In `@apps/ui/src/store/types/state-types.ts`:
- Around line 632-633: Replace the inline TODO about harmonizing provider action
types with a tracked issue reference: create an issue in your tracker listing
the action signatures to standardize (include setCursorDefaultModel,
toggleCursorModel, setGeminiDefaultModel and any other provider actions),
capture the desired change to return Promise<void>, and then update the TODO
comment to reference that issue ID/URL (e.g., "tracked in ISSUE-ID") so the
contract file points to the follow-up work rather than an untracked note. Ensure
the issue includes a concise checklist of the functions to change and the
expected API behavior for async persistence.
In `@apps/ui/tests/features/feature-deep-link.spec.ts`:
- Line 87: Remove the hard sleep await page.waitForTimeout(2000); in the
feature-deep-link.spec.ts test and rely on the existing condition-based wait
instead (the subsequent toPass() block that asserts the feature card
visibility), or if an explicit wait is needed use Playwright's
waitForSelector/waitForLoadState tied to the same selector rather than a fixed
timeout; update the test to remove the redundant waitForTimeout call and ensure
the feature card assertion (the toPass() block) is the source of truth for
waiting.
- Around line 108-113: Replace the flaky fixed timeout after pressing Escape
with an explicit wait for the modal to be hidden: after calling
page.keyboard.press('Escape') on the '[data-testid="agent-output-modal"]'
locator (modal), await the modal becoming hidden (e.g., use modal.waitFor({
state: 'hidden' }) or page.waitForSelector('[data-testid="agent-output-modal"]',
{ state: 'hidden' }) ) instead of await page.waitForTimeout(500) so the test
only proceeds once the modal is actually dismissed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
apps/server/src/index.tsapps/server/src/providers/opencode-provider.tsapps/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/agent-info-panel.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/components/selection-action-bar.tsxapps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsxapps/ui/src/components/views/board-view/dialogs/plan-approval-dialog.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/components/views/settings-view/event-hooks/event-hooks-section.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/lib/utils.tsapps/ui/src/store/app-store.tsapps/ui/src/store/types/state-types.tsapps/ui/tests/features/feature-deep-link.spec.tslibs/types/src/event.tslibs/types/src/feature.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/lib/utils.ts
| 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.
🛠️ Refactor suggestion | 🟠 Major
Use shared-package import instead of direct services/feature-loader path.
This added import uses an old-style relative service path and should follow the shared-package import policy.
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 import of
FeatureLoader uses a relative service path; replace it with the canonical
shared-package import (import type { FeatureLoader } from
'@automaker/feature-loader') so the module is pulled from the shared package
namespace instead of the local services path; update the import statement
referencing FeatureLoader accordingly and ensure any tooling/tsconfig aliases
still resolve the '@automaker/feature-loader' package.
apps/ui/src/components/views/board-view/components/list-view/list-view.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/components/selection-action-bar.tsx
Show resolved
Hide resolved
Comment on lines
42
to
48
| // Use React Query for OpenCode models so changes in settings are reflected immediately | ||
| const { | ||
| data: dynamicOpencodeModelsList = [], | ||
| isLoading: dynamicOpencodeLoading, | ||
| error: dynamicOpencodeError, | ||
| refetch: refetchOpencodeModels, | ||
| } = useOpencodeModels(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate useOpencodeModels implementation and call sites"
rg -n -C3 --type ts --type tsx '\buseOpencodeModels\b'
echo
echo "2) Check whether useOpencodeModels invokes or mirrors fetchOpencodeModels side effects"
rg -n -C4 --type ts --type tsx 'fetchOpencodeModels|knownDynamicModelIds|enabledDynamicModelIds'
echo
echo "Expected:"
echo "- Either the query hook calls into store fetch logic, OR"
echo "- query success path updates enabledDynamicModelIds/knownDynamicModelIds consistently."Repository: AutoMaker-Org/automaker
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate useOpencodeModels implementation and call sites"
rg -n -C3 'useOpencodeModels'
echo
echo "2) Check for fetchOpencodeModels, knownDynamicModelIds, enabledDynamicModelIds"
rg -n -C4 'fetchOpencodeModels|knownDynamicModelIds|enabledDynamicModelIds'
echo
echo "3) Search for the actual hook file in hooks directory"
fd -e ts -e tsx 'hook' apps/ui/src/hooks/ 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== useOpencodeModels hook implementation ==="
sed -n '73,95p' apps/ui/src/hooks/queries/use-models.ts
echo
echo "=== Full hook and its query function ==="
cat -n apps/ui/src/hooks/queries/use-models.ts | sed -n '73,110p'
echo
echo "=== Check if queryFn calls fetchOpencodeModels or implements separately ==="
grep -n "fetchOpencodeModels\|queryFn" apps/ui/src/hooks/queries/use-models.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 2696
🏁 Script executed:
#!/bin/bash
set -euoripefail
echo "=== model-selector.tsx lines 40-90 (context around lines 42-48 and 69-88) ==="
sed -n '40,90p' apps/ui/src/components/views/board-view/shared/model-selector.tsx
echo
echo "=== Check if fetchOpencodeModels is called in the component ==="
rg -n "fetchOpencodeModels" apps/ui/src/components/views/board-view/shared/model-selector.tsx -C 3Repository: AutoMaker-Org/automaker
Length of output: 741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== model-selector.tsx lines 40-90 (context around lines 42-48 and 69-88) ==="
sed -n '40,90p' apps/ui/src/components/views/board-view/shared/model-selector.tsx
echo
echo "=== Check if fetchOpencodeModels is called in the component ==="
rg -n "fetchOpencodeModels" apps/ui/src/components/views/board-view/shared/model-selector.tsx -C 3Repository: AutoMaker-Org/automaker
Length of output: 3248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== opencode-settings-tab.tsx - how it uses useOpencodeModels ==="
sed -n '36,65p' apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx
echo
echo "=== Search for sync logic in opencode-settings-tab ==="
rg -n "useEffect|fetchOpencodeModels|enabledDynamicModelIds" apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx | head -20Repository: AutoMaker-Org/automaker
Length of output: 1612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== fetchOpencodeModels implementation in app-store.ts ==="
sed -n '2879,2920p' apps/ui/src/store/app-store.ts
echo
echo "=== What API does fetchOpencodeModels call? ==="
sed -n '2900,2930p' apps/ui/src/store/app-store.ts | grep -A 20 "api\."Repository: AutoMaker-Org/automaker
Length of output: 1352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full fetchOpencodeModels with API call and side effects ==="
sed -n '2879,2975p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 3697
Unify OpenCode model fetch logic — two separate data paths create state drift risk.
The component uses both useOpencodeModels() (React Query via Electron API) and fetchOpencodeModels() (Zustand store via HTTP API), but these are entirely separate implementations:
useOpencodeModels()returns raw model data with no side effects or store updatesfetchOpencodeModels()includes critical auto-enable logic that updatesenabledDynamicModelIdsandknownDynamicModelIds, persisting to server settings
The opencode-settings-tab syncs query data to the store's dynamicOpencodeModels, but this one-way sync bypasses the auto-enable and known-ID tracking logic in fetchOpencodeModels(). This creates risk where:
- New model discovery and auto-enable behavior may not trigger consistently across the app
knownDynamicModelIdstracking becomes unreliable if some fetch paths skip the auto-enable logic- The model-selector.tsx calls both functions independently with no guarantee they stay in sync
Consider consolidating both into a single unified fetch mechanism (either enhance the React Query hook to include side effects, or have both consume the same store action).
🤖 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 42 - 48, The component currently uses two separate fetch paths
(useOpencodeModels and fetchOpencodeModels) which causes state drift between the
React Query data and the store-managed auto-enable/known-ID logic
(enabledDynamicModelIds, knownDynamicModelIds, dynamicOpencodeModels).
Consolidate into a single source of truth by either: (A) moving the auto-enable
and known-ID side effects into the React Query hook useOpencodeModels so it
updates the store (call the existing store actions that fetch/update
enabledDynamicModelIds and knownDynamicModelIds inside the hook), or (B) have
useOpencodeModels read from the store’s fetchOpencodeModels result (i.e., wrap
the store action in a react-query wrapper) and ensure opencode-settings-tab uses
the same unified path; update all callers (model-selector.tsx,
opencode-settings-tab) to consume only that unified API so auto-enable and
known-ID tracking always run.
Comment on lines
164
to
173
| } else if (provider === 'opencode' && selectedProvider !== 'opencode') { | ||
| // Switch to OpenCode's default model (prefer configured default if it is actually enabled) | ||
| const isDefaultModelAvailable = | ||
| opencodeDefaultModel && allOpencodeModels.some((m) => m.id === opencodeDefaultModel); | ||
| const defaultModelId = isDefaultModelAvailable | ||
| ? opencodeDefaultModel | ||
| : allOpencodeModels[0]?.id; | ||
| if (defaultModelId) { | ||
| onModelSelect(defaultModelId); | ||
| } |
Contributor
There was a problem hiding this comment.
OpenCode provider switch can silently fail when no model is currently enabled.
If allOpencodeModels is empty, clicking the OpenCode provider button does nothing, so users can’t enter OpenCode mode to see its empty/error state.
💡 Suggested fix
} else if (provider === 'opencode' && selectedProvider !== 'opencode') {
- // Switch to OpenCode's default model (prefer configured default if it is actually enabled)
- const isDefaultModelAvailable =
- opencodeDefaultModel && allOpencodeModels.some((m) => m.id === opencodeDefaultModel);
- const defaultModelId = isDefaultModelAvailable
- ? opencodeDefaultModel
- : allOpencodeModels[0]?.id;
- if (defaultModelId) {
- onModelSelect(defaultModelId);
- }
+ // Always switch provider context, even when no enabled model is currently available
+ const defaultModelId = opencodeDefaultModel ?? allOpencodeModels[0]?.id;
+ if (defaultModelId) 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 164 - 173, When switching to the 'opencode' provider the current code only
calls onModelSelect(defaultModelId) if a model exists, so clicking OpenCode does
nothing when allOpencodeModels is empty; update the branch handling provider ===
'opencode' (and selectedProvider !== 'opencode') to always switch the provider
(call onProviderSelect('opencode')) first, then attempt to pick a model as now
(compute isDefaultModelAvailable/defaultModelId and call
onModelSelect(defaultModelId) if present), and if no models exist call
onModelSelect(null) or a dedicated clearModel handler so the UI enters OpenCode
mode and can render its empty/error state. Ensure you reference and use
opencodeDefaultModel, allOpencodeModels, onModelSelect, and onProviderSelect in
the change.
Comment on lines
720
to
724
| const persistedKnownDynamicModelIds = | ||
| serverSettings.knownDynamicModelIds ?? currentAppState.knownDynamicModelIds; | ||
| const sanitizedKnownDynamicModelIds = persistedKnownDynamicModelIds.filter( | ||
| (modelId) => !modelId.startsWith('amazon-bedrock/') | ||
| ); |
Contributor
There was a problem hiding this comment.
Add a defensive array guard before filtering knownDynamicModelIds.
persistedKnownDynamicModelIds.filter(...) will throw if persisted settings are malformed (non-array), which can abort refresh/hydration.
🛡️ Suggested hardening
- const persistedKnownDynamicModelIds =
- serverSettings.knownDynamicModelIds ?? currentAppState.knownDynamicModelIds;
+ const persistedKnownDynamicModelIds = Array.isArray(serverSettings.knownDynamicModelIds)
+ ? serverSettings.knownDynamicModelIds
+ : currentAppState.knownDynamicModelIds;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/hooks/use-settings-sync.ts` around lines 720 - 724,
persistedKnownDynamicModelIds may be non-array and calling .filter will throw;
wrap the value with an array guard so sanitizedKnownDynamicModelIds only runs
filter on an actual array — e.g. coerce persistedKnownDynamicModelIds to
Array.isArray(serverSettings.knownDynamicModelIds) ?
serverSettings.knownDynamicModelIds : currentAppState.knownDynamicModelIds (or
fallback to []), then compute sanitizedKnownDynamicModelIds by filtering that
safe array; update the variables referenced (persistedKnownDynamicModelIds,
sanitizedKnownDynamicModelIds, serverSettings.knownDynamicModelIds,
currentAppState.knownDynamicModelIds) in use-settings-sync.ts accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx (1)
407-421:⚠️ Potential issue | 🟡 MinorAdd aria-labels to HookCard action controls for consistency.
EndpointCard(lines 465-488) includes aria-labels on its Switch and Buttons, butHookCarddoes not. Adding them here would ensure consistent accessibility across both card types.♿ Suggested fix
{/* Actions */} <div className="flex items-center gap-2"> - <Switch checked={hook.enabled} onCheckedChange={onToggle} /> - <Button variant="ghost" size="icon" className="h-8 w-8" onClick={onEdit}> + <Switch + checked={hook.enabled} + onCheckedChange={onToggle} + aria-label={`${hook.enabled ? 'Disable' : 'Enable'} hook ${hook.name || 'unnamed'}`} + /> + <Button + variant="ghost" + size="icon" + className="h-8 w-8" + onClick={onEdit} + aria-label={`Edit hook ${hook.name || 'unnamed'}`} + > <Pencil className="w-4 h-4" /> </Button> <Button variant="ghost" size="icon" className="h-8 w-8 text-destructive hover:text-destructive" onClick={onDelete} + aria-label={`Delete hook ${hook.name || 'unnamed'}`} > <Trash2 className="w-4 h-4" /> </Button> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx` around lines 407 - 421, The HookCard action controls are missing aria-labels; update the Switch and the two Button elements in the HookCard (the elements using onToggle, onEdit, and onDelete) to include meaningful aria-label attributes (e.g., "Enable hook", "Edit hook", "Delete hook" or include the hook identifier/name for uniqueness) matching the pattern used in EndpointCard so screen readers can identify each control consistently.
🧹 Nitpick comments (2)
apps/ui/src/components/views/board-view/components/list-view/list-view.tsx (1)
243-247: Consider decoupling effective sort deps to avoid unnecessary regroup/sort work.When
sortNewestCardOnTopistrue, updates tosortConfigcan still triggerstatusGroupsrecomputation even though effective ordering stayscreatedAt desc. Using primitive effective keys in deps keeps behavior the same with less churn.♻️ Suggested refactor
- const effectiveSortConfig: SortConfig = useMemo( - () => (sortNewestCardOnTop ? { column: 'createdAt', direction: 'desc' } : sortConfig), - [sortNewestCardOnTop, sortConfig] - ); + const effectiveSortColumn: SortColumn = sortNewestCardOnTop ? 'createdAt' : sortConfig.column; + const effectiveSortDirection: SortConfig['direction'] = sortNewestCardOnTop + ? 'desc' + : sortConfig.direction; + + const effectiveSortConfig: SortConfig = useMemo( + () => ({ column: effectiveSortColumn, direction: effectiveSortDirection }), + [effectiveSortColumn, effectiveSortDirection] + ); @@ - const sortedFeatures = sortFeatures( - features, - effectiveSortConfig.column, - effectiveSortConfig.direction - ); + const sortedFeatures = sortFeatures(features, effectiveSortColumn, effectiveSortDirection); @@ - }, [columnFeaturesMap, pipelineConfig, effectiveSortConfig]); + }, [columnFeaturesMap, pipelineConfig, effectiveSortColumn, effectiveSortDirection]);Also applies to: 257-262, 275-275
🤖 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/list-view/list-view.tsx` around lines 243 - 247, The memo for effectiveSortConfig and related memos (e.g., statusGroups) recompute when the whole sortConfig object identity changes even though the effective ordering is unchanged; update the useMemo dependency to use primitive keys instead of the full object: compute a simple effectiveSortKey (e.g., sortNewestCardOnTop ? 'createdAt:desc' : `${sortConfig.column}:${sortConfig.direction}`) and use that string (or the two primitives sortConfig.column and sortConfig.direction) in the dependency arrays for effectiveSortConfig, statusGroups, and any other related useMemo hooks so changes to unrelated fields or object identity won't trigger unnecessary regroup/sort work.apps/ui/src/components/views/board-view/components/selection-action-bar.tsx (1)
66-68: Consider surfacing a local failure message for retry clarity.The empty
catchkeeps context (good), but adding inline error text/toast here would make retry intent clearer when upstream handlers don’t show an error.🤖 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 66 - 68, In SelectionActionBar's try/catch around the action (the empty catch before finally), add a local user-facing error notification so failures are surfaced for retry clarity: either set a component state like localError (and render inline error text near the dialog controls) or call the app's toast/snackbar helper (e.g., showToast or useNotification) with a concise message and optional error details; keep the dialog open as before and ensure the finally block behavior is unchanged.
🤖 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/feature-deep-link.spec.ts`:
- Around line 28-58: The tests create shared filesystem fixtures in
test.beforeAll using projectPath and projectName, causing races under
fullyParallel; change to test.beforeEach to create a unique per-test project dir
(use TEST_TEMP_DIR and include testInfo.workerIndex and/or Date.now() or
testInfo.title in the name) and assign projectPath there, and move all
fs.writeFileSync/fs.mkdirSync fixture setup from test.beforeAll into that
beforeEach, ensuring teardown in test.afterEach removes the per-test directory;
update any usages that relied on the beforeAll-scoped projectPath to use the
per-test variable created in beforeEach.
---
Outside diff comments:
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx`:
- Around line 407-421: The HookCard action controls are missing aria-labels;
update the Switch and the two Button elements in the HookCard (the elements
using onToggle, onEdit, and onDelete) to include meaningful aria-label
attributes (e.g., "Enable hook", "Edit hook", "Delete hook" or include the hook
identifier/name for uniqueness) matching the pattern used in EndpointCard so
screen readers can identify each control consistently.
---
Nitpick comments:
In `@apps/ui/src/components/views/board-view/components/list-view/list-view.tsx`:
- Around line 243-247: The memo for effectiveSortConfig and related memos (e.g.,
statusGroups) recompute when the whole sortConfig object identity changes even
though the effective ordering is unchanged; update the useMemo dependency to use
primitive keys instead of the full object: compute a simple effectiveSortKey
(e.g., sortNewestCardOnTop ? 'createdAt:desc' :
`${sortConfig.column}:${sortConfig.direction}`) and use that string (or the two
primitives sortConfig.column and sortConfig.direction) in the dependency arrays
for effectiveSortConfig, statusGroups, and any other related useMemo hooks so
changes to unrelated fields or object identity won't trigger unnecessary
regroup/sort work.
In `@apps/ui/src/components/views/board-view/components/selection-action-bar.tsx`:
- Around line 66-68: In SelectionActionBar's try/catch around the action (the
empty catch before finally), add a local user-facing error notification so
failures are surfaced for retry clarity: either set a component state like
localError (and render inline error text near the dialog controls) or call the
app's toast/snackbar helper (e.g., showToast or useNotification) with a concise
message and optional error details; keep the dialog open as before and ensure
the finally block behavior is unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/ui/src/components/views/board-view/components/list-view/list-view.tsxapps/ui/src/components/views/board-view/components/selection-action-bar.tsxapps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsxapps/ui/tests/features/feature-deep-link.spec.ts
…link test isolation - Move project creation to beforeEach for per-test isolation - Include workerIndex in project name to prevent parallel test conflicts - Add afterEach cleanup to remove project directories after each test - Maintain beforeAll for temp directory creation
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/feature-deep-link.spec.ts`:
- Line 31: The beforeEach declaration uses an empty destructuring pattern async
({}, testInfo) which triggers the noEmptyPattern lint rule; replace the empty
pattern with a single intentionally ignored parameter name (prefixed with an
underscore) such as async (_fixtures, testInfo) or async (_unused, testInfo) in
the test.beforeEach callback to satisfy the linter while preserving the original
parameter order and behavior in feature-deep-link.spec.ts.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
AutoLoopCoordinator Service
SLEEP_INTERVAL_CAPACITY_MS,SLEEP_INTERVAL_IDLE_MS,SLEEP_INTERVAL_NORMAL_MS,SLEEP_INTERVAL_ERROR_MS) for improved readability and maintenancehasInProgressFeaturesForWorktree()method to check if features are transitioning states (e.g., from in_progress to completed)featureBelongsToWorktree()helper to properly filter features by branch context (handles main vs feature worktrees)projectPathandbranchNamefieldsEventHookService
executeNtfyHook()methodHookContexttype definition to shared@automaker/typesasEventHookContextfor reusabilityFeatureStateManager
NOTIFICATION_TYPE_WAITING_APPROVAL,NOTIFICATION_TYPE_VERIFIED, etc.)destroy()method for proper cleanup of event subscriptionsjustFinishedAt)Configuration
.gitignoreto exclude entiretest/fixtures/directory and removed outdated submodule referenceSummary by CodeRabbit
New Features
Improvements
UI/UX Polish