feat(web): auto-add bookmark to list on selection#2629
feat(web): auto-add bookmark to list on selection#2629MohamedBassem merged 9 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:
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" flow by removing the react-hook-form/zod wrapper and the explicit "Add" button — selecting a list from the dropdown now immediately triggers Confidence Score: 5/5Safe to merge — no P0/P1 issues found; all remaining observations are P2 or lower. The refactor is straightforward and correct. The disabled guard prevents double-submission, the null-assertion bug in the old code is fixed, and the selector correctly displays the placeholder after a selection (no controlled value is needed for the auto-add pattern). No data-loss or logic errors identified. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "typecheck" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
|
@greptile review |
|
Thank you! |
2 participants
Fixes #2604
Summary
Automatically add bookmark to the selected list when user clicks on a list item in the ManageListsModal.
Changes
Testing