Skip to content

feat(bookmarks): add bulk open links functionality#2631

Open
xingzihai wants to merge 5 commits intokarakeep-app:mainfrom
xingzihai:feat/bulk-open-links-clean
Open

feat(bookmarks): add bulk open links functionality#2631
xingzihai wants to merge 5 commits intokarakeep-app:mainfrom
xingzihai:feat/bulk-open-links-clean

Conversation

@xingzihai
Copy link
Copy Markdown
Contributor

Fixes #2618

Summary

Add bulk open links functionality to bookmark actions.

Changes

  • Add "Open Links" button to bulk actions menu
  • Open selected link bookmarks in new tabs
  • URL validation (http/https only)
  • Maximum 10 links limit to prevent browser issues
  • Popup blocker detection with user-friendly messages

Testing

  • Tested with various bookmark selections
  • Verified popup blocker handling
  • Tested URL validation

- Add 'Open Links' button to bulk actions toolbar
- Allow users to open multiple link-type bookmarks in new tabs
- Handle popup blocker with user-friendly error message
- Add open_links translation to English locale
- Add ExternalLink to lucide-react imports
- Implement openLinks() function with:
  - URL validation (http/https only)
  - Maximum 10 links limit
  - Popup blocker detection
  - User-friendly toast messages
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

Walkthrough

Adds a bulk "Open Links" action to the bookmarks dashboard that filters selected bookmarks to link-type entries, validates http(s) URLs, defers confirmation when above a warning threshold, opens eligible URLs via window.open, reports opened/blocked/skipped counts via toasts, and adds English localization keys.

Changes

Cohort / File(s) Summary
Bulk Link Opening Feature
apps/web/components/dashboard/BulkBookmarksAction.tsx
Introduces a new bulk "Open Links" action: implements openLinks and confirmation flow, filters selected bookmarks to LINK type, validates http/https URLs with new URL(url), enforces a warning threshold (MAX_OPEN_LINKS_WARNING), handles deferred confirmation via dialog and pendingEligibleLinks, attempts window.open per URL with _blank and noopener,noreferrer, counts opened vs blocked vs skipped, and emits localized toasts. Registers action in actionList with an ExternalLink icon (hidden when bulk edit is disabled).
UI Localization (English)
apps/web/lib/i18n/locales/en/translation.json
Adds actions.open_links label and multiple toasts.bookmarks messages for opening-links flows: no_links_selected, too_many_links ({{max}}, {{count}}), links_opened ({{count}}), links_opened_blocked ({{opened}}, {{blocked}}), links_opened_skipped ({{opened}}, {{skipped}}), and links_opened_blocked_skipped ({{opened}}, {{blocked}}, {{skipped}}).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main feature addition: bulk open links functionality for bookmarks.
Description check ✅ Passed The description comprehensively covers the changes including the new button, URL validation, link limit, popup detection, and testing performed.
Linked Issues check ✅ Passed The PR successfully implements the core requirements from #2618: bulk opening of selected links in new tabs with URL validation and user-friendly messaging for edge cases.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing bulk open links functionality: the action component, translations for UI and toasts, and supporting logic for URL validation and confirmation dialogs.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR adds a "Open Links" bulk action that opens all selected link-type bookmarks in new browser tabs, with URL validation (http/https only), a 10-link safety cap, and popup-blocker detection. The implementation is clean and integrates naturally into the existing action list pattern.\n\nKey changes:\n- New openLinks() function in BulkBookmarksAction.tsx with safe URL parsing via new URL(), popup-blocker detection via window.open() return value check, and noopener,noreferrer rel flags for security.\n- A single open_links translation key added to en/translation.json.\n\nMinor concerns found:\n- The four feedback toast descriptions are hard-coded English strings rather than using the t() i18n system — inconsistent given the button label itself is already translated.\n- The limit 10 is a magic number duplicated in the guard condition and the error message; a named constant would be cleaner.\n- Links with invalid or non-http/https URLs are silently skipped and not counted in either opened or blocked, so the final "Opened N links" toast can undercount relative to how many links the user actually selected.

Confidence Score: 5/5

Safe to merge — no runtime errors or data-integrity issues; all findings are minor style and UX polish.

All three findings are P2 (style/UX suggestions): i18n coverage for toast strings, extracting a magic number, and making the skipped-URL count visible. None of these cause incorrect behaviour or data loss. The core logic — URL validation, tab opening, popup-blocker detection, and security flags — is correct.

No files require special attention; all concerns are in BulkBookmarksAction.tsx and are non-blocking.

Important Files Changed

Filename Overview
apps/web/components/dashboard/BulkBookmarksAction.tsx Adds openLinks() function and action entry for bulk-opening link bookmarks in new tabs; implementation is broadly correct with safe URL validation and popup-blocker detection, but toast messages bypass i18n, the limit is a magic number, and silently-skipped invalid/non-http URLs make the opened count potentially misleading to users.
apps/web/lib/i18n/locales/en/translation.json Adds a single open_links translation key in the correct location within the actions namespace; straightforward and correct.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/web/components/dashboard/BulkBookmarksAction.tsx
Line: 171-214

Comment:
**Toast messages bypass the i18n translation system**

All four toast descriptions in `openLinks` are hard-coded English strings, yet the action's button label (`t("actions.open_links")`) and the key in `translation.json` already use the i18n system. This means the feature is partially localised — the button label translates, but the feedback messages won't.

For consistency with the rest of the file, it would be better to add translation keys for these messages and use `t(...)`:

```
"open_links_none_selected": "No links selected",
"open_links_too_many": "Cannot open more than {{max}} links at once. You selected {{count}}.",
"open_links_success": "Opened {{count}} links in new tabs.",
"open_links_blocked": "Opened {{opened}} links. {{blocked}} were blocked by popup blocker."
```

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/BulkBookmarksAction.tsx
Line: 178-184

Comment:
**Magic number `10` should be a named constant**

The limit of 10 is used in two places (the condition and the toast message) without explanation. Extracting it to a named constant at the top of the file makes the intent clearer and keeps both occurrences in sync if the limit ever changes.

```suggestion
    const MAX_OPEN_LINKS = 10;

    // Limit maximum number of links
    if (links.length > MAX_OPEN_LINKS) {
      toast({
        variant: "destructive",
        description: `Cannot open more than ${MAX_OPEN_LINKS} links at once. You selected ${links.length}.`,
      });
      return;
    }
```

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/BulkBookmarksAction.tsx
Line: 186-215

Comment:
**Silently-skipped URLs make the success count misleading**

Links whose URLs are either invalid or use a non-http/https protocol are caught and silently discarded — they are counted in neither `opened` nor `blocked`. If a user selects 5 links and 2 have non-http URLs (e.g. `ftp://` or an empty string from a partially-saved bookmark), the toast would say "Opened 3 links in new tabs", which is confusing.

Consider tracking skipped URLs and surfacing them in the toast, or at minimum including them in the count so the numbers add up to `links.length`.

```ts
let opened = 0;
let blocked = 0;
let skipped = 0;

links.forEach((item) => {
  const url = item.content.url;
  try {
    const parsed = new URL(url);
    if (["http:", "https:"].includes(parsed.protocol)) {
      const win = window.open(url, "_blank", "noopener,noreferrer");
      if (win) { opened++; } else { blocked++; }
    } else {
      skipped++;
    }
  } catch {
    skipped++;
  }
});
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix: implement openLinks function and ad..." | Re-trigger Greptile

Comment on lines +171 to +214
toast({
description: "No links selected",
});
return;
}

// Limit maximum number of links
if (links.length > 10) {
toast({
variant: "destructive",
description: `Cannot open more than 10 links at once. You selected ${links.length}.`,
});
return;
}

let opened = 0;
let blocked = 0;

links.forEach((item) => {
const url = item.content.url;
try {
const parsed = new URL(url);
if (["http:", "https:"].includes(parsed.protocol)) {
const win = window.open(url, "_blank", "noopener,noreferrer");
if (win) {
opened++;
} else {
blocked++;
}
}
} catch {
// Ignore invalid URLs
}
});

if (blocked > 0) {
toast({
variant: "destructive",
description: `Opened ${opened} links. ${blocked} were blocked by popup blocker.`,
});
} else {
toast({
description: `Opened ${opened} links in new tabs.`,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Toast messages bypass the i18n translation system

All four toast descriptions in openLinks are hard-coded English strings, yet the action's button label (t("actions.open_links")) and the key in translation.json already use the i18n system. This means the feature is partially localised — the button label translates, but the feedback messages won't.

For consistency with the rest of the file, it would be better to add translation keys for these messages and use t(...):

"open_links_none_selected": "No links selected",
"open_links_too_many": "Cannot open more than {{max}} links at once. You selected {{count}}.",
"open_links_success": "Opened {{count}} links in new tabs.",
"open_links_blocked": "Opened {{opened}} links. {{blocked}} were blocked by popup blocker."
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/components/dashboard/BulkBookmarksAction.tsx
Line: 171-214

Comment:
**Toast messages bypass the i18n translation system**

All four toast descriptions in `openLinks` are hard-coded English strings, yet the action's button label (`t("actions.open_links")`) and the key in `translation.json` already use the i18n system. This means the feature is partially localised — the button label translates, but the feedback messages won't.

For consistency with the rest of the file, it would be better to add translation keys for these messages and use `t(...)`:

```
"open_links_none_selected": "No links selected",
"open_links_too_many": "Cannot open more than {{max}} links at once. You selected {{count}}.",
"open_links_success": "Opened {{count}} links in new tabs.",
"open_links_blocked": "Opened {{opened}} links. {{blocked}} were blocked by popup blocker."
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +178 to +184
if (links.length > 10) {
toast({
variant: "destructive",
description: `Cannot open more than 10 links at once. You selected ${links.length}.`,
});
return;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Magic number 10 should be a named constant

The limit of 10 is used in two places (the condition and the toast message) without explanation. Extracting it to a named constant at the top of the file makes the intent clearer and keeps both occurrences in sync if the limit ever changes.

Suggested change
if (links.length > 10) {
toast({
variant: "destructive",
description: `Cannot open more than 10 links at once. You selected ${links.length}.`,
});
return;
}
const MAX_OPEN_LINKS = 10;
// Limit maximum number of links
if (links.length > MAX_OPEN_LINKS) {
toast({
variant: "destructive",
description: `Cannot open more than ${MAX_OPEN_LINKS} links at once. You selected ${links.length}.`,
});
return;
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/components/dashboard/BulkBookmarksAction.tsx
Line: 178-184

Comment:
**Magic number `10` should be a named constant**

The limit of 10 is used in two places (the condition and the toast message) without explanation. Extracting it to a named constant at the top of the file makes the intent clearer and keeps both occurrences in sync if the limit ever changes.

```suggestion
    const MAX_OPEN_LINKS = 10;

    // Limit maximum number of links
    if (links.length > MAX_OPEN_LINKS) {
      toast({
        variant: "destructive",
        description: `Cannot open more than ${MAX_OPEN_LINKS} links at once. You selected ${links.length}.`,
      });
      return;
    }
```

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!

Comment on lines +186 to +215
let opened = 0;
let blocked = 0;

links.forEach((item) => {
const url = item.content.url;
try {
const parsed = new URL(url);
if (["http:", "https:"].includes(parsed.protocol)) {
const win = window.open(url, "_blank", "noopener,noreferrer");
if (win) {
opened++;
} else {
blocked++;
}
}
} catch {
// Ignore invalid URLs
}
});

if (blocked > 0) {
toast({
variant: "destructive",
description: `Opened ${opened} links. ${blocked} were blocked by popup blocker.`,
});
} else {
toast({
description: `Opened ${opened} links in new tabs.`,
});
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silently-skipped URLs make the success count misleading

Links whose URLs are either invalid or use a non-http/https protocol are caught and silently discarded — they are counted in neither opened nor blocked. If a user selects 5 links and 2 have non-http URLs (e.g. ftp:// or an empty string from a partially-saved bookmark), the toast would say "Opened 3 links in new tabs", which is confusing.

Consider tracking skipped URLs and surfacing them in the toast, or at minimum including them in the count so the numbers add up to links.length.

let opened = 0;
let blocked = 0;
let skipped = 0;

links.forEach((item) => {
  const url = item.content.url;
  try {
    const parsed = new URL(url);
    if (["http:", "https:"].includes(parsed.protocol)) {
      const win = window.open(url, "_blank", "noopener,noreferrer");
      if (win) { opened++; } else { blocked++; }
    } else {
      skipped++;
    }
  } catch {
    skipped++;
  }
});
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/components/dashboard/BulkBookmarksAction.tsx
Line: 186-215

Comment:
**Silently-skipped URLs make the success count misleading**

Links whose URLs are either invalid or use a non-http/https protocol are caught and silently discarded — they are counted in neither `opened` nor `blocked`. If a user selects 5 links and 2 have non-http URLs (e.g. `ftp://` or an empty string from a partially-saved bookmark), the toast would say "Opened 3 links in new tabs", which is confusing.

Consider tracking skipped URLs and surfacing them in the toast, or at minimum including them in the count so the numbers add up to `links.length`.

```ts
let opened = 0;
let blocked = 0;
let skipped = 0;

links.forEach((item) => {
  const url = item.content.url;
  try {
    const parsed = new URL(url);
    if (["http:", "https:"].includes(parsed.protocol)) {
      const win = window.open(url, "_blank", "noopener,noreferrer");
      if (win) { opened++; } else { blocked++; }
    } else {
      skipped++;
    }
  } catch {
    skipped++;
  }
});
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/BulkBookmarksAction.tsx`:
- Around line 189-214: The toast currently only reports opened and blocked
counts while invalid/unsupported URLs are ignored; update the links processing
in BulkBookmarksAction.tsx (the loop that calls links.forEach and the
surrounding opened/blocked counters) to also track a skipped counter: increment
skipped inside the catch for invalid URLs and whenever parsed.protocol is not
"http:" or "https:"; finally include the skipped count in the toast messages
(both the destructive and success branches) so users see how many links were
skipped due to invalid or non-HTTP(S) URLs.
🪄 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: 29a04d71-9fef-47f1-a389-5e681ee36f08

📥 Commits

Reviewing files that changed from the base of the PR and between 53a4598 and adf63f9.

📒 Files selected for processing (2)
  • apps/web/components/dashboard/BulkBookmarksAction.tsx
  • apps/web/lib/i18n/locales/en/translation.json

- Add i18n translation for openLinks toast messages
- Define MAX_OPEN_LINKS constant to replace magic number 10
- Track and report skipped URLs (invalid or non-HTTP) in toast
- Include all statistics (opened, blocked, skipped) in toast summary
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/BulkBookmarksAction.tsx`:
- Around line 167-187: The current cap uses links =
selectedBookmarks.filter(item => item.content.type === BookmarkTypes.LINK) and
checks links.length against MAX_OPEN_LINKS before validating URLs, so
non-HTTP(S) LINK items can incorrectly inflate the count; change the logic in
BulkBookmarksAction to first derive an openableLinks array by filtering links
for valid HTTP(S) URLs (e.g., test content.url with a URL parser or
startsWith('http://')/('https://')), then apply the MAX_OPEN_LINKS check and
toast using openableLinks.length, while preserving the original links/open
behavior to only attempt opening URLs from openableLinks.
🪄 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: 4948622a-5c86-4401-a52b-c23e8642b531

📥 Commits

Reviewing files that changed from the base of the PR and between adf63f9 and 52ce040.

📒 Files selected for processing (2)
  • apps/web/components/dashboard/BulkBookmarksAction.tsx
  • apps/web/lib/i18n/locales/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/web/lib/i18n/locales/en/translation.json

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
apps/web/components/dashboard/BulkBookmarksAction.tsx (1)

182-187: ⚠️ Potential issue | 🟡 Minor

Handle the “all skipped” branch with a skipped-aware message.

At Line 182, openableLinks.length === 0 currently maps to toasts.bookmarks.no_links_selected, which is inaccurate when links were selected but all were invalid/non-HTTP(S). This also hides skipped-count feedback in that path.

💡 Suggested patch
-    if (openableLinks.length === 0) {
-      toast({
-        description: t("toasts.bookmarks.no_links_selected"),
-      });
-      return;
-    }
+    if (openableLinks.length === 0) {
+      toast({
+        variant: "destructive",
+        description: t("toasts.bookmarks.links_opened_skipped", {
+          opened: 0,
+          skipped: links.length,
+        }),
+      });
+      return;
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/components/dashboard/BulkBookmarksAction.tsx` around lines 182 -
187, Replace the current single check for openableLinks.length === 0 with a
branch that distinguishes "nothing selected" vs "all selected were skipped": use
the selection count from the surrounding scope (e.g.,
selectedIds/selectedBookmarks or props) together with openableLinks to decide
the toast; if no items were selected keep using
t("toasts.bookmarks.no_links_selected"), but if items were selected and
openableLinks.length === 0 emit a skipped-aware toast (e.g.,
t("toasts.bookmarks.all_links_skipped", { skipped: selectedCount }) or
t("toasts.bookmarks.no_openable_links", { skipped: selectedCount })), ensuring
you call toast(...) with the appropriate message and include the skipped count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@apps/web/components/dashboard/BulkBookmarksAction.tsx`:
- Around line 182-187: Replace the current single check for openableLinks.length
=== 0 with a branch that distinguishes "nothing selected" vs "all selected were
skipped": use the selection count from the surrounding scope (e.g.,
selectedIds/selectedBookmarks or props) together with openableLinks to decide
the toast; if no items were selected keep using
t("toasts.bookmarks.no_links_selected"), but if items were selected and
openableLinks.length === 0 emit a skipped-aware toast (e.g.,
t("toasts.bookmarks.all_links_skipped", { skipped: selectedCount }) or
t("toasts.bookmarks.no_openable_links", { skipped: selectedCount })), ensuring
you call toast(...) with the appropriate message and include the skipped count.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ea67f3eb-9726-48be-92e1-533763124a9b

📥 Commits

Reviewing files that changed from the base of the PR and between 52ce040 and 974746c.

📒 Files selected for processing (1)
  • apps/web/components/dashboard/BulkBookmarksAction.tsx

@kranix0
Copy link
Copy Markdown

kranix0 commented Apr 1, 2026

Thanks for writing this improvement! It can make a big difference for managing research projects.

Maximum 10 links limit to prevent browser issues

Unfortunately, this is profoundly limiting and prevents the restoration of complex sessions. My browser does lazy loading, so there are no issues with opening many more tabs. Can you please make this limit configurable/optional?

Previously, opening more than 10 links would show an error and refuse.
Now it shows a confirmation dialog allowing users to proceed if they want.

Addresses user feedback requesting configurable/optional limit.
Users with lazy-loading browsers can now open more links if they choose.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
apps/web/components/dashboard/BulkBookmarksAction.tsx (1)

65-69: Consider clearing pendingOpenableLinks when dialog is cancelled.

If the user closes the confirmation dialog without confirming (e.g., clicks outside or presses Escape), pendingOpenableLinks retains stale references. While this doesn't affect correctness (the state is only read in confirmOpenManyLinks), clearing it improves hygiene.

♻️ Suggested change
 <ActionConfirmingDialog
   open={isTooManyLinksDialogOpen}
-  setOpen={setIsTooManyLinksDialogOpen}
+  setOpen={(open) => {
+    setIsTooManyLinksDialogOpen(open);
+    if (!open) {
+      setPendingOpenableLinks([]);
+    }
+  }}
   title={"Open Many Links"}

Also applies to: 252-256

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/components/dashboard/BulkBookmarksAction.tsx` around lines 65 - 69,
The confirmation dialog leaves stale references in pendingOpenableLinks when the
user cancels/escapes; update the dialog close/cancel handler (where you call
setIsTooManyLinksDialogOpen(false)) to also call setPendingOpenableLinks([]) so
pendingOpenableLinks is cleared on non-confirm closes, and do the same for the
other dialog instance referenced around lines 252-256; ensure
confirmOpenManyLinks still uses pendingOpenableLinks before clearing it after a
successful confirm.
🤖 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/BulkBookmarksAction.tsx`:
- Around line 171-207: In openLinks, avoid re-computing link lists from
selectedBookmarks when linksToOpen is provided: treat linksToOpen as the source
of truth by skipping the initial filter steps (the creation of links and
openableLinks) when linksToOpen is passed, set targetLinks = linksToOpen
directly, and compute skipped based on the difference between the original
provided list length and the final targetLinks length (e.g., skipped =
linksToOpen.length - targetLinks.length or similar) so
confirmOpenManyLinks/pendingOpenableLinks are honored and counts remain
accurate; update references to links, openableLinks, targetLinks, skipped,
pendingOpenableLinks and the early "no links selected" toast to use the provided
linksToOpen path.

---

Nitpick comments:
In `@apps/web/components/dashboard/BulkBookmarksAction.tsx`:
- Around line 65-69: The confirmation dialog leaves stale references in
pendingOpenableLinks when the user cancels/escapes; update the dialog
close/cancel handler (where you call setIsTooManyLinksDialogOpen(false)) to also
call setPendingOpenableLinks([]) so pendingOpenableLinks is cleared on
non-confirm closes, and do the same for the other dialog instance referenced
around lines 252-256; ensure confirmOpenManyLinks still uses
pendingOpenableLinks before clearing it after a successful confirm.
🪄 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: f991bfb3-317b-45c5-b089-942de01488fe

📥 Commits

Reviewing files that changed from the base of the PR and between 974746c and fe9c6be.

📒 Files selected for processing (1)
  • apps/web/components/dashboard/BulkBookmarksAction.tsx

Comment on lines +171 to +207
const openLinks = (linksToOpen?: typeof selectedBookmarks) => {
const links = selectedBookmarks.filter(
(item) => item.content.type === BookmarkTypes.LINK,
);

// First filter for valid HTTP(S) URLs that can actually be opened
const openableLinks = links.filter((item) => {
const url = item.content.url;
try {
const parsed = new URL(url);
return ["http:", "https:"].includes(parsed.protocol);
} catch {
return false;
}
});

if (openableLinks.length === 0) {
toast({
description: t("toasts.bookmarks.no_links_selected"),
});
return;
}

// If provided links to open (from confirmation dialog), use those
const targetLinks = linksToOpen ?? openableLinks;

// Show warning dialog if trying to open more than threshold
// (unless we're already in the confirmed path)
if (!linksToOpen && targetLinks.length > MAX_OPEN_LINKS_WARNING) {
setPendingOpenableLinks(targetLinks);
setIsTooManyLinksDialogOpen(true);
return;
}

let opened = 0;
let blocked = 0;
const skipped = links.length - openableLinks.length;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Logic inconsistency when called from confirmation dialog.

When openLinks(pendingOpenableLinks) is called from confirmOpenManyLinks, the function re-computes links and openableLinks from the current selectedBookmarks state, not from the provided linksToOpen. This creates two issues:

  1. Early return check (lines 187-192): If selection changes while dialog is open, openableLinks could be empty, causing the function to show "no links selected" and abort—even though valid linksToOpen were passed.

  2. Skipped count (line 207): skipped is calculated from re-computed links.length - openableLinks.length, which may differ from targetLinks. The reported skipped count could be incorrect.

💡 Proposed fix: skip re-computation when linksToOpen is provided
 const openLinks = (linksToOpen?: typeof selectedBookmarks) => {
+  // If links are already provided (from confirmation), use them directly
+  if (linksToOpen && linksToOpen.length > 0) {
+    let opened = 0;
+    let blocked = 0;
+
+    linksToOpen.forEach((item) => {
+      const win = window.open(item.content.url, "_blank", "noopener,noreferrer");
+      if (win) {
+        opened++;
+      } else {
+        blocked++;
+      }
+    });
+
+    if (blocked > 0) {
+      toast({
+        variant: "destructive",
+        description: t("toasts.bookmarks.links_opened_blocked", {
+          opened,
+          blocked,
+        }),
+      });
+    } else {
+      toast({
+        description: t("toasts.bookmarks.links_opened", {
+          count: opened,
+        }),
+      });
+    }
+    return;
+  }
+
   const links = selectedBookmarks.filter(
     (item) => item.content.type === BookmarkTypes.LINK,
   );
-
-  // First filter for valid HTTP(S) URLs that can actually be opened
-  const openableLinks = links.filter((item) => {
-    const url = item.content.url;
-    try {
-      const parsed = new URL(url);
-      return ["http:", "https:"].includes(parsed.protocol);
-    } catch {
-      return false;
-    }
-  });
-
-  if (openableLinks.length === 0) {
-    toast({
-      description: t("toasts.bookmarks.no_links_selected"),
-    });
-    return;
-  }
-
-  // If provided links to open (from confirmation dialog), use those
-  const targetLinks = linksToOpen ?? openableLinks;
-
-  // Show warning dialog if trying to open more than threshold
-  // (unless we're already in the confirmed path)
-  if (!linksToOpen && targetLinks.length > MAX_OPEN_LINKS_WARNING) {
-    setPendingOpenableLinks(targetLinks);
-    setIsTooManyLinksDialogOpen(true);
-    return;
-  }
+  // ... rest of original logic for the initial (non-confirmation) path

Alternatively, consider extracting the actual "open windows" logic into a separate function that doesn't depend on selectedBookmarks, making the data flow clearer.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/components/dashboard/BulkBookmarksAction.tsx` around lines 171 -
207, In openLinks, avoid re-computing link lists from selectedBookmarks when
linksToOpen is provided: treat linksToOpen as the source of truth by skipping
the initial filter steps (the creation of links and openableLinks) when
linksToOpen is passed, set targetLinks = linksToOpen directly, and compute
skipped based on the difference between the original provided list length and
the final targetLinks length (e.g., skipped = linksToOpen.length -
targetLinks.length or similar) so confirmOpenManyLinks/pendingOpenableLinks are
honored and counts remain accurate; update references to links, openableLinks,
targetLinks, skipped, pendingOpenableLinks and the early "no links selected"
toast to use the provided linksToOpen path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FR:Add Support bulk opening of links

2 participants