feat(web): auto-add bookmark to list on selection#2629
feat(web): auto-add bookmark to list on selection#2629xingzihai wants to merge 7 commits intokarakeep-app:mainfrom
Conversation
Remove the 'Add' button from Manage Lists modal. When a user selects a list from the dropdown, the bookmark is now automatically added to that list, simplifying the user experience. Users can still remove a bookmark from a list by clicking the X button next to the list name. Fixes karakeep-app#2604
…sModal - Added isPending state from useAddBookmarkToList hook - Added duplicate submission prevention in onChange handler - Added disabled prop to BookmarkListSelector component
- Fix incorrect indentation of onSuccess and onError callbacks - Add trailing newline at end of file
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoved the form/zod/react-hook-form submit flow from ManageListsModal; adding to a list now happens immediately via BookmarkListSelector.onChange. BookmarkListSelector gained a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 simplifies the "Manage Lists" modal by removing the Confidence Score: 5/5Safe to merge — no logic bugs or data-integrity concerns; only P2 style feedback remains. All findings are P2 (missing newline and a harmless redundant guard). The core auto-add logic, the duplicate-submission prevention, and the disabled state handling are all correct. The removal of the form layer introduces no regressions. No files require special attention. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/web/components/dashboard/bookmarks/ManageListsModal.tsx
Line: 183
Comment:
**Missing newline at end of file**
The file is missing a trailing newline. Most editors and linters expect a newline at the end of every text file (POSIX convention), and the diff explicitly shows `\ No newline at end of file`.
```suggestion
}
```
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/web/components/dashboard/bookmarks/ManageListsModal.tsx
Line: 142-149
Comment:
**Redundant in-flight guard**
The `if (!isAddingToListPending)` check inside `onChange` is already made unreachable by `disabled={isAddingToListPending}` on the trigger button — a disabled button won't open the popover, so `onChange` can't fire while the mutation is in flight. You can simplify to a direct call:
```suggestion
onChange={(listId) => {
addToList({
bookmarkId: bookmarkId,
listId: listId,
});
}}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "style: fix indentation in useAddBookmark..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ), | ||
| }; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The file is missing a trailing newline. Most editors and linters expect a newline at the end of every text file (POSIX convention), and the diff explicitly shows \ No newline at end of file.
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/components/dashboard/bookmarks/ManageListsModal.tsx
Line: 183
Comment:
**Missing newline at end of file**
The file is missing a trailing newline. Most editors and linters expect a newline at the end of every text file (POSIX convention), and the diff explicitly shows `\ No newline at end of file`.
```suggestion
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| onChange={(listId) => { | ||
| if (!isAddingToListPending) { | ||
| addToList({ | ||
| bookmarkId: bookmarkId, | ||
| listId: listId, | ||
| }); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The if (!isAddingToListPending) check inside onChange is already made unreachable by disabled={isAddingToListPending} on the trigger button — a disabled button won't open the popover, so onChange can't fire while the mutation is in flight. You can simplify to a direct call:
| onChange={(listId) => { | |
| if (!isAddingToListPending) { | |
| addToList({ | |
| bookmarkId: bookmarkId, | |
| listId: listId, | |
| }); | |
| } | |
| }} | |
| onChange={(listId) => { | |
| addToList({ | |
| bookmarkId: bookmarkId, | |
| listId: listId, | |
| }); | |
| }} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/components/dashboard/bookmarks/ManageListsModal.tsx
Line: 142-149
Comment:
**Redundant in-flight guard**
The `if (!isAddingToListPending)` check inside `onChange` is already made unreachable by `disabled={isAddingToListPending}` on the trigger button — a disabled button won't open the popover, so `onChange` can't fire while the mutation is in flight. You can simplify to a direct call:
```suggestion
onChange={(listId) => {
addToList({
bookmarkId: bookmarkId,
listId: listId,
});
}}
```
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/web/components/dashboard/bookmarks/ManageListsModal.tsx`:
- Around line 139-152: The selection can be triggered while membership data is
still loading; update the ManageListsModal usage of BookmarkListSelector so
interactions are disabled during loading and the onChange guard also checks
loading. Specifically, change the disabled prop to combine isAddingToListPending
|| isLoading, and update the onChange callback to return early unless both
!isAddingToListPending and !isLoading before calling addToList(bookmarkId,
listId); keep references to BookmarkListSelector, hideBookmarkIds
(alreadyInList?.lists.map), onChange, isAddingToListPending, isLoading,
addToList, and bookmarkId so the fix is easy to locate.
- Around line 117-120: The code assumes allLists.getPathById(list.id) always
returns an array and directly calls .map(...), which will crash if it returns
undefined/null; update the render in ManageListsModal to first store the result
of allLists.getPathById(list.id) (or call it into a local variable), guard it
with a null check (e.g., if path is falsy render a safe fallback like list.name
or an empty string), and only call path.map(...) and .join(" / ") when path is
an array—this prevents crashes for stale/missing paths.
🪄 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: 4058e129-f284-4a71-92d8-5d7f26364ff8
📒 Files selected for processing (2)
apps/web/components/dashboard/bookmarks/ManageListsModal.tsxapps/web/components/dashboard/lists/BookmarkListSelector.tsx
- Add trailing newline at end of file - Remove redundant in-flight guard check (disabled prop handles it) - Add guard for getPathById to prevent crash on stale cache - Disable interactions while list membership data is loading
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/web/components/dashboard/bookmarks/ManageListsModal.tsx`:
- Around line 103-105: Replace the hardcoded modal title in
ManageListsModal.tsx: instead of <DialogTitle>Manage Lists</DialogTitle> call
the translation function used elsewhere (t) — e.g. <DialogTitle>{t("Manage
Lists")}</DialogTitle> (or use the existing i18n key you use for list headings,
like t("manageLists") if that key exists) so the DialogTitle uses localized
text; ensure t is in scope where DialogTitle is rendered.
🪄 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: b4160672-40de-4c88-804f-cd972a8d9093
📒 Files selected for processing (1)
apps/web/components/dashboard/bookmarks/ManageListsModal.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/web/components/dashboard/bookmarks/ManageListsModal.tsx`:
- Around line 124-134: The ActionButton that renders the X icon is icon-only and
lacks an accessible name; add an aria-label (or equivalent sr-only text) to the
ActionButton in ManageListsModal so screen readers announce its purpose — e.g.,
set aria-label={`Remove bookmark from ${list.name}`} or a generic
aria-label="Remove bookmark" on the ActionButton that calls deleteFromList({
bookmarkId, listId: list.id }) to make the remove action accessible.
- Around line 109-121: The list rows are currently hidden because the whole UL
is gated on allLists; instead, render the rows unconditionally using
alreadyInList.lists and only use allLists when available to compute the display
path. Modify the render in ManageListsModal.tsx so that you iterate over
alreadyInList?.lists (or alreadyInList.lists) regardless of allLists, call
allLists?.getPathById(list.id) (optional chaining) to build the path if present,
and fall back to rendering list.name when getPathById returns undefined; keep
the existing remove control/handlers intact so they remain visible when the tree
query is empty.
🪄 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: b8b6565a-5b47-40ac-a8d2-0d1dca6a2e85
📒 Files selected for processing (1)
apps/web/components/dashboard/bookmarks/ManageListsModal.tsx
- Add aria-label to remove button for accessibility (CodeRabbit comment 3006737430) - Remove allLists gating to allow list.name fallback to render (CodeRabbit comment 3006737429) - Use optional chaining for path lookup
1 participant
Fixes #2604
Summary
Automatically add bookmark to the selected list when user clicks on a list item in the ManageListsModal.
Changes
Testing