Fix ntfy endpoint saving and bump version#822
Conversation
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 addresses minor bug fixes and includes a significant version bump. It enhances the Agent Output Modal to display up-to-date feature information by fetching fresh data from the server, preventing stale content when accessed via deep links. Additionally, it improves the application's state management by correctly reconciling ntfy endpoint configurations from the server, ensuring consistency. 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
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (11)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. 📝 WalkthroughWalkthroughThis PR introduces settings persistence for default feature model, planning mode, and approval requirements; adds AI-driven conflict resolution UI wiring across worktree components; enhances board drag-drop with explicit stopFeature capability; modifies settings hydration to restore all worktree selections; and expands premium model tier categorization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for displaying fresh data in the agent output modal and adds logic to synchronize ntfy endpoints during fast hydration. The version is also bumped to 1.0.0.
My review focuses on the new synchronization logic. I've identified a potential issue where deleting all ntfy endpoints on the server might not reflect on the client due to a condition in the reconciliation logic. I've provided a suggestion to address this.
apps/ui/src/routes/__root.tsx
Outdated
| JSON.stringify(serverEndpoints) !== JSON.stringify(currentEndpoints) && | ||
| serverEndpoints.length > 0 |
There was a problem hiding this comment.
This condition prevents synchronization when all ntfy endpoints are deleted on the server. If a user removes all endpoints, the server will return an empty array, serverEndpoints.length will be 0, and this if block will not execute. This leaves stale endpoints on the client.
Removing this part of the condition will ensure that an empty array from the server correctly clears the client-side state.
JSON.stringify(serverEndpoints) !== JSON.stringify(currentEndpoints)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/src/routes/__root.tsx`:
- Around line 613-624: The reconciliation currently skips applying an empty
server ntfyEndpoints due to the additional serverEndpoints.length > 0 guard,
which preserves deleted endpoints; update the logic in the hydrate/reconcile
block that compares (finalSettings as GlobalSettings).ntfyEndpoints to
useAppStore.getState().ntfyEndpoints so that an empty array from the server is
applied as well—i.e., remove the serverEndpoints.length > 0 condition and keep
the JSON.stringify comparison and subsequent useAppStore.setState({
ntfyEndpoints: serverEndpoints }) call (references: finalSettings,
GlobalSettings, ntfyEndpoints, useAppStore, logger).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/routes/__root.tsxapps/ui/tests/features/feature-deep-link.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/server/src/routes/worktree/routes/list.ts (1)
157-234:conflictSourceBranchis computed but not surfaced to clients.This block performs extra fs/git resolution, but the field is not assigned onto
worktreein the response path. Consider wiring it through (or gating this logic until a consumer is in place) to avoid unnecessary polling overhead.🔧 Suggested wiring
interface WorktreeInfo { path: string; branch: string; isMain: boolean; isCurrent: boolean; // Is this the currently checked out branch in main? hasWorktree: boolean; // Always true for items in this list hasChanges?: boolean; changedFilesCount?: number; pr?: WorktreePRInfo; // PR info if a PR has been created for this branch /** Whether there are actual unresolved conflict files (conflictFiles.length > 0) */ hasConflicts?: boolean; /** Type of git operation in progress (merge/rebase/cherry-pick), set independently of hasConflicts */ conflictType?: 'merge' | 'rebase' | 'cherry-pick'; /** List of files with conflicts */ conflictFiles?: string[]; + /** Source branch involved in merge/rebase/cherry-pick, when resolvable */ + conflictSourceBranch?: string; } @@ worktree.hasConflicts = conflictState.hasConflicts; worktree.conflictFiles = conflictState.conflictFiles; + worktree.conflictSourceBranch = conflictState.conflictSourceBranch;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/list.ts` around lines 157 - 234, The code computes conflictSourceBranch but never attaches it to the worktree object returned to clients; wire it into the returned worktree (or the existing response object) so callers receive the value. Locate the local variable conflictSourceBranch in apps/server/src/routes/worktree/routes/list.ts and add that value to the worktree (or the returned object shape that includes hasConflicts/conflictType/conflictFiles) before the method returns; if you want to avoid extra work when unused, gate the filesystem/git resolution behind checking whether the consumer expects conflictSourceBranch or only populate it when a flag is present.apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts (1)
22-23: Consider preserving stop result semantics for more accurate user feedback.At Line 22 and Line 259,
stopFeatureis treated asPromise<void>, so any non-throwing failure path is indistinguishable from success and still logs “stopped”. If your stop path can resolve tofalse, this can produce misleading success messaging.♻️ Suggested contract + handling update
interface UseBoardDragDropProps { - stopFeature: (featureId: string) => Promise<void>; + stopFeature: (featureId: string) => Promise<boolean>; } ... - await stopFeature(featureId); - logger.info('Stopped running feature via drag to backlog:', featureId); + const stopped = await stopFeature(featureId); + if (stopped) { + logger.info('Stopped running feature via drag to backlog:', featureId); + } else { + logger.warn('Feature was not running by the time stop was requested:', featureId); + }Also applies to: 259-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts` around lines 22 - 23, Change the stopFeature contract to return a Promise<boolean> instead of Promise<void> so callers can distinguish success vs non-throwing failure; update the stopFeature declaration in the interface and its implementation (stopFeature) to resolve true on success and false on graceful failure, then update all callers (the use-board-drag-drop handler that currently awaits stopFeature and logs "stopped") to check the returned boolean and only log success when true and handle/log the false path as a failed stop (e.g., emit an error message or fallback behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/list.ts`:
- Around line 163-164: The TypeScript error comes from calling .trim() on the
Promise result of secureFs.readFile which is typed as string | Buffer; update
each occurrence (calls to secureFs.readFile in list.ts that read git refs like
'MERGE_HEAD') to ensure a string before trimming — for example, await the read
into a variable then coerce to string ((await
secureFs.readFile(path.join(gitDir, 'MERGE_HEAD'), 'utf-8')) as string).trim()
or use (await secureFs.readFile(...)).toString().trim(); apply the same change
to all similar calls (the other secureFs.readFile usages in this file) so
.trim() operates on a string.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 484-487: The payload to onCreateConflictResolutionFeature
incorrectly uses worktree.branch for both sourceBranch and targetBranch, causing
self-merge instructions; change sourceBranch to the actual conflicting branch
(e.g., the conflict/incoming branch value available in scope such as
conflict.sourceBranch or incomingBranch) when calling
onCreateConflictResolutionFeature, keeping targetBranch as worktree.branch and
preserving targetWorktreePath: worktree.path so the AI receives the real source
vs target branches for resolution.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/list.ts`:
- Around line 157-234: The code computes conflictSourceBranch but never attaches
it to the worktree object returned to clients; wire it into the returned
worktree (or the existing response object) so callers receive the value. Locate
the local variable conflictSourceBranch in
apps/server/src/routes/worktree/routes/list.ts and add that value to the
worktree (or the returned object shape that includes
hasConflicts/conflictType/conflictFiles) before the method returns; if you want
to avoid extra work when unused, gate the filesystem/git resolution behind
checking whether the consumer expects conflictSourceBranch or only populate it
when a flag is present.
In `@apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts`:
- Around line 22-23: Change the stopFeature contract to return a
Promise<boolean> instead of Promise<void> so callers can distinguish success vs
non-throwing failure; update the stopFeature declaration in the interface and
its implementation (stopFeature) to resolve true on success and false on
graceful failure, then update all callers (the use-board-drag-drop handler that
currently awaits stopFeature and logs "stopped") to check the returned boolean
and only log success when true and handle/log the false path as a failed stop
(e.g., emit an error message or fallback behavior).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/server/src/routes/worktree/routes/list.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/hooks/use-board-drag-drop.tsapps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.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/board-view/worktree-panel/worktree-panel.tsx
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Show resolved
Hide resolved
1 participant
Minor bug fixes
Summary by CodeRabbit
New Features
Improvements