feat: add manual save toggle for browser extension#2693
Merged
MohamedBassem merged 4 commits intokarakeep-app:mainfrom Apr 20, 2026
Merged
feat: add manual save toggle for browser extension#2693MohamedBassem merged 4 commits intokarakeep-app:mainfrom
MohamedBassem merged 4 commits intokarakeep-app:mainfrom
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Greptile SummaryThis PR adds an "Auto-save on open" toggle to the browser extension. When disabled, the extension shows a confirmation panel (with editable title and notes fields) before saving, instead of immediately creating the bookmark on popup open. The implementation correctly separates settings loading from bookmark request loading using two Confidence Score: 5/5Safe to merge; all findings are P2 style/UX suggestions that don't affect correctness. Both open findings are minor: a cosmetic truncation bug and a missing cancel button. The core state logic (two-effect split, idle-state guard, settings schema) is correct and the feature works as described. apps/browser-extension/src/SavePage.tsx — text truncation and cancel-button UX Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/browser-extension/src/SavePage.tsx
Line: 174-177
Comment:
**Trailing ellipsis appended unconditionally**
`"..."` is always concatenated even when `pendingBookmark.text` is shorter than 150 characters, producing output like `"short text..."`. Use conditional logic or the built-in CSS `truncate` approach.
```suggestion
<p className="text-muted-foreground text-xs">
{pendingBookmark.text.length > 150
? pendingBookmark.text.substring(0, 150) + "..."
: pendingBookmark.text}
</p>
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/browser-extension/src/SavePage.tsx
Line: 209-212
Comment:
**No way to dismiss the confirmation UI**
The manual-save panel has only a "Save Bookmark" button. Users who opened the extension by accident, or who change their mind, have no cancel path short of closing the popup. A "Cancel" / "Dismiss" button would complete the confirmation UX.
```suggestion
<div className="flex gap-2">
<Button variant="outline" onClick={() => window.close()} className="w-full">
Cancel
</Button>
<Button onClick={handleManualSave} className="w-full">
Save Bookmark
</Button>
</div>
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: add manual save toggle for browser..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+209
to
+212
| </div> | ||
| <Button onClick={handleManualSave} className="w-full"> | ||
| Save Bookmark | ||
| </Button> |
There was a problem hiding this comment.
The manual-save panel has only a "Save Bookmark" button. Users who opened the extension by accident, or who change their mind, have no cancel path short of closing the popup. A "Cancel" / "Dismiss" button would complete the confirmation UX.
Suggested change
| </div> | |
| <Button onClick={handleManualSave} className="w-full"> | |
| Save Bookmark | |
| </Button> | |
| <div className="flex gap-2"> | |
| <Button variant="outline" onClick={() => window.close()} className="w-full"> | |
| Cancel | |
| </Button> | |
| <Button onClick={handleManualSave} className="w-full"> | |
| Save Bookmark | |
| </Button> | |
| </div> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/browser-extension/src/SavePage.tsx
Line: 209-212
Comment:
**No way to dismiss the confirmation UI**
The manual-save panel has only a "Save Bookmark" button. Users who opened the extension by accident, or who change their mind, have no cancel path short of closing the popup. A "Cancel" / "Dismiss" button would complete the confirmation UX.
```suggestion
<div className="flex gap-2">
<Button variant="outline" onClick={() => window.close()} className="w-full">
Cancel
</Button>
<Button onClick={handleManualSave} className="w-full">
Save Bookmark
</Button>
</div>
```
How can I resolve this? If you propose a fix, please make it concise.![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: 1
🧹 Nitpick comments (1)
apps/browser-extension/src/SavePage.tsx (1)
23-23: Confusing variable rename from hook.The hook returns
isPending: isInit, whereisInitisfalseinitially and becomestrueafter settings load. Renaming it toisSettingsLoadedmakes the usage at line 102 (if (!isSettingsLoaded) return;) read correctly, but the originalisPendingname in the hook is misleading—it actually means "isInitialized", not "is currently pending".Consider renaming the property in
usePluginSettingsfromisPendingtoisLoadedorisInitializedto avoid confusion for future maintainers.♻️ Suggested hook return value rename in settings.ts
- return { settings, setSettings, isPending: isInit }; + return { settings, setSettings, isLoaded: isInit };Then in SavePage:
- const { settings, isPending: isSettingsLoaded } = usePluginSettings(); + const { settings, isLoaded: isSettingsLoaded } = usePluginSettings();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-extension/src/SavePage.tsx` at line 23, Rename the hook return property to reflect its semantics: update usePluginSettings to return isLoaded or isInitialized instead of isPending (where the hook currently maps isInit -> isPending), and update all call sites like SavePage that destructure usePluginSettings (currently using isSettingsLoaded) to use the new name consistently; specifically change the property in the hook implementation in settings.ts (the place that defines isInit) and replace occurrences of isPending in consumers (e.g., the destructure in SavePage that now reads const { settings, isPending: isSettingsLoaded } = usePluginSettings()) so they destructure the new name or keep their local alias (e.g., const { settings, isLoaded: isSettingsLoaded } = usePluginSettings()) to retain existing usage semantics.
🤖 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/browser-extension/src/SavePage.tsx`:
- Around line 174-178: The UI always appends "..." to pendingBookmark.text even
when shorter than 150 chars; update the render in SavePage.tsx (the
pendingBookmark and BookmarkTypes.TEXT branch) to conditionally append the
ellipsis only when pendingBookmark.text.length > 150, e.g., render the truncated
substring when length > 150 and the full text otherwise so users aren't misled.
---
Nitpick comments:
In `@apps/browser-extension/src/SavePage.tsx`:
- Line 23: Rename the hook return property to reflect its semantics: update
usePluginSettings to return isLoaded or isInitialized instead of isPending
(where the hook currently maps isInit -> isPending), and update all call sites
like SavePage that destructure usePluginSettings (currently using
isSettingsLoaded) to use the new name consistently; specifically change the
property in the hook implementation in settings.ts (the place that defines
isInit) and replace occurrences of isPending in consumers (e.g., the destructure
in SavePage that now reads const { settings, isPending: isSettingsLoaded } =
usePluginSettings()) so they destructure the new name or keep their local alias
(e.g., const { settings, isLoaded: isSettingsLoaded } = usePluginSettings()) to
retain existing usage semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db560b7f-6234-4e76-8696-8335ab1d3e88
📒 Files selected for processing (3)
apps/browser-extension/src/OptionsPage.tsxapps/browser-extension/src/SavePage.tsxapps/browser-extension/src/utils/settings.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/browser-extension/src/SavePage.tsx (1)
57-66:⚠️ Potential issue | 🟡 MinorUnhandled parse exception may crash the component.
zNewBookmarkRequestSchema.parse(req)at line 65 throws if the stored request is malformed or corrupted. This would result in an unhandled promise rejection, potentially leaving users with a broken popup.🛡️ Proposed fix to handle parse failure gracefully
async function getNewBookmarkRequestFromBackgroundScriptIfAny(): Promise<ZNewBookmarkRequest | null> { const { [NEW_BOOKMARK_REQUEST_KEY_NAME]: req } = await chrome.storage.session.get(NEW_BOOKMARK_REQUEST_KEY_NAME); if (!req) { return null; } // Delete the request immediately to avoid issues with lingering values await chrome.storage.session.remove(NEW_BOOKMARK_REQUEST_KEY_NAME); - return zNewBookmarkRequestSchema.parse(req); + const parsed = zNewBookmarkRequestSchema.safeParse(req); + if (!parsed.success) { + console.warn("Invalid bookmark request in session storage:", parsed.error); + return null; + } + return parsed.data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-extension/src/SavePage.tsx` around lines 57 - 66, The call to zNewBookmarkRequestSchema.parse(req) inside getNewBookmarkRequestFromBackgroundScriptIfAny can throw on malformed data; wrap the parse in a try/catch inside that function, catch parsing errors, log or report the error (e.g., via console.error or a telemetry/processLogger), and return null so the component won't crash; keep the existing removal of NEW_BOOKMARK_REQUEST_KEY_NAME and ensure the function returns null on parse failure instead of letting the exception bubble.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/browser-extension/src/SavePage.tsx`:
- Around line 57-66: The call to zNewBookmarkRequestSchema.parse(req) inside
getNewBookmarkRequestFromBackgroundScriptIfAny can throw on malformed data; wrap
the parse in a try/catch inside that function, catch parsing errors, log or
report the error (e.g., via console.error or a telemetry/processLogger), and
return null so the component won't crash; keep the existing removal of
NEW_BOOKMARK_REQUEST_KEY_NAME and ensure the function returns null on parse
failure instead of letting the exception bubble.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e944873f-3a08-45fa-b7cc-c59539ac320d
📒 Files selected for processing (1)
apps/browser-extension/src/SavePage.tsx
Collaborator
|
Thank you! |
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
2 participants
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.
I tried to keep this intentionally light only including the options that can be saved on the initial create bookmark call (no tags/lists) but could add those as well.
Fixes #2692