Skip to content

NTP: Limit attached tabs to three in NTP omnibar - #2898

Open
jotaemepereira wants to merge 7 commits into
mainfrom
juan/tab-attachments-limit
Open

NTP: Limit attached tabs to three in NTP omnibar#2898
jotaemepereira wants to merge 7 commits into
mainfrom
juan/tab-attachments-limit

Conversation

@jotaemepereira

@jotaemepereira jotaemepereira commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Asana Task/Github Issue: https://app.asana.com/1/137249556945/project/1204006570077678/task/1216831900874432?focus=true

Description

New tab page now has a limit on how many tabs can be attached; the number will be three, and it will be sent by native. We have a fallback just in case.

Testing Steps

  • Use Xcode to open the branch at macOS: Limit tab attachments to three in the native omnibar apple-browsers#5994
  • Point it to this code
  • When running the macOS browser, got to Debug -> Set Internal User
  • Go to Debug -> Feature Flags -> and turn on aiChatNtpAttachMoreTabas
  • Open four tabs in some sites
  • Go to new tab page, select the four tabs to be attached
  • Check that an error appears and the submit is disabled
Screenshot 2026-07-23 at 7 12 12 PM

Please tick all that apply:

  • I have tested this change locally
  • I have tested this change locally in all supported browsers
  • This change will be visible to users
  • I have added automated tests that cover this change
  • I have ensured the change is gated by config
  • This change was covered by a ship review
  • This change was covered by a tech design
  • Any dependent config has been merged

Note

Low Risk
UI-only attachment gating on NTP omnibar with config-driven limits and tests; no auth or payment paths touched.

Overview
The NTP omnibar now enforces a soft cap on attached open tabs (default 3 from native attachmentLimits.tabs.maxAttached), matching how file attachments already work: users can attach more than the limit, but a warning appears, submit stays disabled, and removing tabs clears the state.

useTabAttachments takes the configured max, exposes tabLimitExceeded and maxTabs, and getTabsForSubmission now walks attachedTabs (open tabs only) so closed persisted entries don’t count toward the cap or get sent as context.

Omnibar wires the limit from config, shows omnibar_tabAttachmentLimitWarning after image/file warnings in precedence, and extends data-attachment-warning / disabled submit logic. Types and omnibar config schema add optional tabs.maxAttached; mocks and omnibar.tabMaxAttached support tests. Integration and screenshot tests cover over-limit UX and avoid false warnings in overflow screenshots.

Reviewed by Cursor Bugbot for commit 56545f6. Bugbot is set up for automated code reviews on this repo. Configure here.

Mirror the file soft-cap pattern: add tabs.maxAttached to the AttachmentLimits
contract (files/images now optional, tabs always present), enforce via
useTabAttachments (tabLimitExceeded) with a warning + blocked submit.
Covers the configured cap being honored, the warning naming the limit, submit
blocked while over, and recovery on removal. Uses a cap of 1 (two attaches) to
avoid the picker-reopen flakiness that more attaches hit under parallel load.

@cursor cursor Bot left a comment

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.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1cf91b6. Configure here.

Comment thread special-pages/pages/new-tab/app/omnibar/components/Omnibar.js
Comment thread special-pages/pages/new-tab/app/omnibar/mocks/omnibar.mock-transport.js Outdated
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Cursor review was not successful.

This PR requires a manual review and approval from a member of one of the following teams:

  • @duckduckgo/content-scope-scripts-owners
  • @duckduckgo/apple-devs
  • @duckduckgo/android-devs
  • @duckduckgo/team-windows-development
  • @duckduckgo/extension-owners
  • @duckduckgo/config-aor
  • @duckduckgo/breakage-aor
  • @duckduckgo/breakage

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

[Beta] Generated file diff

Time updated: Mon, 27 Jul 2026 13:12:16 GMT

Apple
    - apple/pages/new-tab/dist/index.js
  • apple/pages/new-tab/locales/en/new-tab.json

File has changed

Integration
    - integration/pages/new-tab/dist/index.js
  • integration/pages/new-tab/locales/en/new-tab.json

File has changed

Windows
    - windows/pages/new-tab/dist/index.js
  • windows/pages/new-tab/locales/en/new-tab.json

File has changed

@cursor cursor Bot left a comment

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.

Stale comment

Web Compatibility & Security Review — PR #2898

Head: 1cf91b61 · Scope: special-pages/ NTP omnibar only (8 files, +105/−13). No injected/, messaging/ transport, captured-globals, or API override changes.

Web Compatibility Assessment

File Lines Severity Finding
special-pages/pages/new-tab/app/omnibar/components/chat-tools/tab-attachment/useTabAttachments.js 62–64 info Soft cap mirrors useFileAttachments: users may attach more than maxTabs, see a warning, and have submit blocked until excess tabs are removed. No hard stop in toggleTab. Intentional and consistent with existing file-attachment UX.
special-pages/pages/new-tab/app/omnibar/components/Omnibar.js 337–344, 490–496 info Submit blocking (disabled) and Enter-key guard in AiChatForm correctly include tabWarning. Warning uses role="alert" with correct precedence (image → file error → file warning → tab).
special-pages/pages/new-tab/app/omnibar/components/Omnibar.js 375 info data-attachment-warning still only reflects imageWarning || fileWarning — not tabWarning. Tab-limit state shows the text alert but omits the red popup border defined in Omnibar.module.css (.root:has([data-attachment-warning])). Minor visual inconsistency vs file limits.
special-pages/pages/new-tab/app/omnibar/strings.json 94–96 info Copy uses "{limit} tabs" — reads as "1 tabs" when maxAttached is 1 (same pluralization pattern as file warning).
special-pages/pages/new-tab/app/omnibar/components/Omnibar.js 242 info attachmentLimits?.tabs?.maxAttached with hook default MAX_TABS = 3 provides safe fallback if native omits the field; schema documents tabs as always present from native.

No API surface fidelity, prototype-chain, DOM timing, platform API, or third-party script compatibility concerns — this code runs in the first-party embedded NTP, not on arbitrary web pages.

Security Assessment

File Lines Severity Finding
(scope) info No injected runtime, message-bridge, nativeData, postMessage, or captured-global usage. Threat model for hostile third-party pages does not apply.
special-pages/pages/new-tab/messages/types/omnibar-config.json 8–24 info Schema adds required tabs.maxAttached; files/images demoted to optional (matching backend-sourced, fallback behavior). Native must ship tabs when sending attachmentLimits; web-side optional chaining + default limits breakage to default cap of 3.
special-pages/pages/new-tab/app/omnibar/components/chat-tools/tab-attachment/useTabAttachments.js 74–90 info getTabsForSubmission() still fetches content for all attached tabs regardless of limit; submit is UI-gated only. Acceptable — native/backend must enforce limits on omnibar_submitChat (same as file attachments).
special-pages/pages/new-tab/app/omnibar/mocks/omnibar.mock-transport.js 276–287 info Defensive config.attachmentLimits.images/files guards before assignment — prevents mock crashes when sub-limits are absent. Test-only.

No security vulnerabilities identified.

Risk Level

Low Risk — First-party special-pages UI change enforcing a native-configured tab attachment cap using the established file-attachment soft-limit pattern; no injection runtime or security-boundary changes.

Recommendations

  1. (info) Consider adding tabWarning to data-attachment-warning on the aiChatContent container for consistent red-border styling.
  2. (info) Confirm all native platforms ship attachmentLimits.tabs.maxAttached per updated schema before release.
  3. (info) Optional: ICU/plural-aware copy for limit === 1 ("1 tab" vs "N tabs").

Integration test tabs over the configured cap warn… passes locally.

Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

const disabled = !query || imageWarning || fileWarning;
// Only one attachment message shows at a time; the tab warning falls last in precedence.
const showTabWarning = tabWarning && !imageMessageShowing && !showFileError && !showFileWarning;
const disabled = !query || imageWarning || fileWarning || tabWarning;

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.

info — submit gating looks correct. tabWarning is included in disabled, which flows to both the submit button and AiChatForm's Enter-key / form-submit guards. This matches the file-attachment soft-cap pattern.


// No hard cap on attaching tabs; exceeding maxTabs warns and blocks submit until removed —
// mirroring the file-attachment soft cap (`useFileAttachments`).
const tabLimitExceeded = attachedTabs.length > maxTabs;

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.

info — intentional soft cap. toggleTab does not block adds past maxTabs; excess attachments persist until the user removes them. This mirrors useFileAttachments and is covered by the new integration test.

- getTabsForSubmission iterates attachedTabs (open) not attachedEntries, so a
  closed-but-persisted tab can't ship a context that isn't counted against the cap.
- data-attachment-warning now includes tabWarning, so the over-limit popup chrome
  (red border/layout) applies to the tab message too.
- Document omnibar.tabMaxAttached in the NTP readme.
The generated en/new-tab.json was out of sync with strings.json (the string was
added but translations.mjs not re-run), which test-clean-tree would flag.
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Build Branch

Branch pr-releases/juan/tab-attachments-limit
Commit 849e96b5ed
Updated July 27, 2026 at 1:11:38 PM UTC

Static preview entry points

QR codes (mobile preview)
Entry point QR code
Docs QR for docs preview
Static pages QR for static pages preview
Integration pages QR for integration pages preview

Integration commands

npm (Android / Extension):

npm i github:duckduckgo/content-scope-scripts#pr-releases/juan/tab-attachments-limit

Swift Package Manager (Apple):

.package(url: "https://github.com/duckduckgo/content-scope-scripts.git", branch: "pr-releases/juan/tab-attachments-limit")

git submodule (Windows):

git -C submodules/content-scope-scripts fetch origin pr-releases/juan/tab-attachments-limit
git -C submodules/content-scope-scripts checkout origin/pr-releases/juan/tab-attachments-limit
Pin to exact commit

npm (Android / Extension):

npm i github:duckduckgo/content-scope-scripts#849e96b5ed50dea90a04c4bf9ca4a1ebfa0db5bf

Swift Package Manager (Apple):

.package(url: "https://github.com/duckduckgo/content-scope-scripts.git", revision: "849e96b5ed50dea90a04c4bf9ca4a1ebfa0db5bf")

git submodule (Windows):

git -C submodules/content-scope-scripts fetch origin pr-releases/juan/tab-attachments-limit
git -C submodules/content-scope-scripts checkout 849e96b5ed50dea90a04c4bf9ca4a1ebfa0db5bf

daxtheduck
daxtheduck previously approved these changes Jul 23, 2026

@cursor cursor Bot left a comment

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.

Stale comment

Injected PR Evaluation: Web Compatibility & Security

Re-assessed on synchronize (8e0e31d). Low Risk — special-pages only; no injected/, messaging transport, or API override changes.

Web Compatibility Assessment

File Lines Severity Finding
special-pages/pages/new-tab/app/omnibar/components/Omnibar.js 493–497 info Warning string uses fixed plural "tabs" ("You can only attach {limit} tabs at a time."), so limit: 1 reads "1 tabs". Mirrors the existing file-attachment warning pattern — cosmetic only.
special-pages/pages/new-tab/messages/types/omnibar-config.json 9–23 info Schema now requires tabs when attachmentLimits is present (files/images optional). Native must ship attachmentLimits.tabs.maxAttached alongside any existing limits object; older payloads omitting tabs would fail schema validation. UI falls back to MAX_TABS = 3 when attachmentLimits is absent entirely.
special-pages/pages/new-tab/app/omnibar/components/chat-tools/tab-attachment/useTabAttachments.js 50–64 info Soft cap mirrors useFileAttachments: users can attach past the limit, see a warning, and submit is blocked via disabled (button + Enter key in AiChatForm). No hard stop in toggleTab.

No API surface fidelity, prototype chain, DOM safety, timing, platform-specific, or third-party script compatibility concerns — this code runs in the first-party NTP omnibar, not in injected page context.

Security Assessment

File Lines Severity Finding
special-pages/pages/new-tab/app/omnibar/components/chat-tools/tab-attachment/useTabAttachments.js 77–94 info Improvement: getTabsForSubmission now iterates attachedTabs (open tabs only) instead of attachedEntries. Closed-but-persisted entries no longer ship pageContext that isn't counted against the cap — closes a consistency gap between UI chips and submission payload.
special-pages/pages/new-tab/app/omnibar/components/Omnibar.js 242 info maxTabs is passed through from native config without positive-integer validation. A maxAttached: 0 would permanently block submit (same pattern as file limits). Trust boundary is native → NTP messaging, not hostile page scripts.
No changes to captured-globals, message-bridge trust boundaries, postMessage, nativeData forwarding, prototype pollution, or iframe access patterns.

Risk Level

Low Risk — UI-only cap enforcement in the embedded NTP omnibar with schema/types/test updates; no content-scope injection runtime changes.

Recommendations

  1. Native coordination (info): Ensure all platforms ship attachmentLimits.tabs.maxAttached when sending attachmentLimits (schema now requires it).
  2. Resolved since prior review: data-attachment-warning now includes tabWarning (commit 8a3e9332c).
  3. Optional (info): Add ICU/plural-aware copy for limit === 1 if product wants to fix "1 tabs" (pre-existing pattern for files).
  4. Test coverage: Integration test for cap=1 passes locally (tabs over the configured cap warn…). Consider a unit test asserting closed tabs are excluded from getTabsForSubmission.

Automated assessment — Injected PR Evaluation (Web Compatibility & Security)

Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

@jotaemepereira
jotaemepereira requested a review from moon0326 July 23, 2026 22:28
- Run prettier on omnibar-attachments.spec.js (lint check failed).
- The carousel-overflow screenshot test attaches 4 tabs, which now exceeds the
  default cap of 3 and rendered the over-limit warning. Raise the cap to 10 for
  that test so it stays a pure overflow test and the existing baseline holds.
@daxtheduck
daxtheduck dismissed their stale review July 23, 2026 22:30

Dismissing stale approval — new commits pushed, awaiting Cursor re-review.

daxtheduck
daxtheduck previously approved these changes Jul 23, 2026

@cursor cursor Bot left a comment

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.

Stale comment

Injected PR Evaluation: Web Compatibility & Security

PR: NTP: Limit attached tabs to three in NTP omnibar (b8ce6aec)

Re-assessed on synchronize. Delta since 8e0e31d: b8ce6aec fixes Prettier formatting and raises omnibar.tabMaxAttached to 10 in the overflow screenshot test so it stays a pure layout test (4 chips no longer trip the default cap of 3). No new runtime or security surface.


Web Compatibility Assessment

File Lines Severity Finding
(all) info Out of scope for third-party web compatibility. Changes are confined to special-pages/pages/new-tab/ (embedded first-party NTP omnibar). No injected/, wrapper-utils.js, captured-globals.js, or browser API overrides.
useTabAttachments.js 62–64 info Soft-cap pattern mirrors useFileAttachments: users can attach over the limit, see a warning, and submit is blocked until excess tabs are removed. Consistent UX; no hard stop in toggleTab.
useTabAttachments.js 77–94 info getTabsForSubmission iterates attachedTabs (open tabs only), aligning submission payload with chip count and cap enforcement. Closed-but-persisted entries no longer bypass the limit.
Omnibar.js 337–344, 375 info Tab warning participates in submit-disable and data-attachment-warning; message precedence (image → file error → file warning → tab warning) is documented and matches existing attachment UX.
strings.json / en/new-tab.json 94–97 info Copy uses fixed "{limit} tabs" pluralization (e.g. "1 tabs"), same pattern as the existing file-attachment warning.
omnibar-config.json 11–24 info Schema now requires tabs when attachmentLimits is present; files/images optional. Native clients must ship attachmentLimits.tabs.maxAttached alongside existing limits.
new-tab.screenshots.spec.js 471–473 info Screenshot overflow test correctly overrides cap via omnibar.tabMaxAttached=10 to avoid warning chrome polluting the layout snapshot.

No web compatibility errors or warnings.


Security Assessment

File Lines Severity Finding
(all) info No changes to messaging transports, message bridge, nativeData handling, postMessage, or content-scope injection.
useTabAttachments.js / Omnibar.js 23, 242 info maxAttached is native-supplied config (schema-validated integer); falls back to MAX_TABS = 3 when absent. Limit is enforced client-side for UX only — same trust model as file/image caps.
omnibar.mock-transport.js 276–287 info Mock transport gains omnibar.tabMaxAttached query-param override with > 0 guard; test-only, no production exposure.

No security errors, warnings, or critical findings.


Risk Level

Low Risk — Special-pages-only UI and schema change for a native-configured attachment cap; no injected runtime, API shimming, or messaging security boundary changes.


Recommendations

  1. (info) Ensure shipping native builds include attachmentLimits.tabs.maxAttached whenever attachmentLimits is sent (schema now requires tabs).
  2. (info) Consider ICU/plural-aware strings for tab (and file) limit warnings in a follow-up — not a blocker.
  3. (info) Optional: add minimum: 1 to maxAttached in the JSON schema if native should never send 0 — current > maxTabs logic would block all tab attachments for maxAttached: 0, matching file-limit behavior.

Automation assessment — Injected PR Evaluation (Web Compatibility & Security)

Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

@moon0326 moon0326 left a comment

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.

Code LGTM, and it tested as expected with the latest apple-browser changes.

When I attach more than three tabs, I get a warning that only three tabs can be attached. However, all of the tabs are still attached.

Looking at https://github.com/duckduckgo/content-scope-scripts/pull/2898/changes#diff-ef2b8f607a9dbda68aef39bad51326e2c4406786f6e173a5014f9879e803d47fR62, it looks like this is intended. I just wanted to double-check.

Other than that, 🚀

@moon0326 moon0326 left a comment

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.

@jotaemepereira 👋 Please don't forget to update the translations.

The native tab-attachment limit is now behind a default-on kill switch. When off,
native omits attachmentLimits.tabs, so make tabs optional in the schema and have
useTabAttachments apply no limit when maxAttached is absent (also matches
pre-feature / old-native behavior).
@daxtheduck
daxtheduck dismissed their stale review July 27, 2026 13:11

Dismissing stale approval — new commits pushed, awaiting Cursor re-review.

@github-actions github-actions Bot added the semver-minor New feature — triggers minor version bump label Jul 27, 2026

@cursor cursor Bot left a comment

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.

Injected PR Evaluation: Web Compatibility & Security

PR: NTP: Limit attached tabs to three in NTP omnibar (56545f6e)

Re-assessed on synchronize. Delta since b8ce6aec: e87b1776 makes attachmentLimits.tabs optional and treats an absent maxAttached as no limit (Number.POSITIVE_INFINITY), matching the native kill-switch-off path and pre-feature behavior. Mock transport guards config.attachmentLimits.tabs before applying the omnibar.tabMaxAttached override.


Web Compatibility Assessment

File Lines Severity Finding
(all) info Out of scope for third-party web compatibility. Changes are confined to special-pages/pages/new-tab/ (embedded first-party NTP omnibar). No injected/, wrapper-utils.js, captured-globals.js, or browser API overrides.
useTabAttachments.js 19–21, 62 info Soft-cap pattern mirrors useFileAttachments: users can attach over the limit, see a warning, and submit is blocked until excess tabs are removed. When native omits attachmentLimits.tabs (kill switch off), default is no cap — restores unlimited attachment for old native / rollback.
useTabAttachments.js 75–92 info getTabsForSubmission iterates attachedTabs (open tabs only), aligning submission payload with chip count and cap enforcement. Closed-but-persisted entries no longer bypass the limit.
Omnibar.js 337–344, 375, 493–496 info Tab warning participates in submit-disable and data-attachment-warning; message precedence (image → file error → file warning → tab warning) matches existing attachment UX. disabled is forwarded to AiChatForm, blocking both button and Enter-key submit.
strings.json / en/new-tab.json 94–97 info Copy uses fixed "{limit} tabs" pluralization (e.g. "1 tabs"), same pattern as the existing file-attachment warning.
omnibar-config.json 11–23 info AttachmentLimits fields (tabs, files, images) are all optional. Native sends tabs.maxAttached when the kill switch is on; omits tabs entirely when off.
new-tab.screenshots.spec.js 471–473 info Overflow screenshot test overrides cap via omnibar.tabMaxAttached=10 to avoid warning chrome polluting the layout snapshot.

No web compatibility errors or warnings.


Security Assessment

File Lines Severity Finding
(all) info No changes to messaging transports, message bridge, nativeData handling, postMessage, or content-scope injection.
useTabAttachments.js / Omnibar.js 21, 242 info maxAttached is native-supplied config (schema-validated integer). Limit is enforced client-side for UX only — same trust model as file/image caps. Kill-switch-off path intentionally removes the cap; native must still enforce server-side limits if needed.
omnibar.mock-transport.js 170–172, 286–287 info Mock defaults to maxAttached: 3; query-param override guarded with > 0 && config.attachmentLimits.tabs. Test-only, no production exposure.

No security errors, warnings, or critical findings.


Risk Level

Low Risk — Special-pages-only UI and schema change for a native-configured attachment cap with kill-switch rollback; no injected runtime, API shimming, or messaging security boundary changes.


Recommendations

  1. (info) Native clients with the kill switch on should send attachmentLimits.tabs.maxAttached; with the switch off, omit tabs entirely (do not send maxAttached: 0).
  2. (info) Consider ICU/plural-aware strings for tab (and file) limit warnings in a follow-up — not a blocker.
  3. (info) Optional: add minimum: 1 to maxAttached in the JSON schema — maxAttached: 0 would block all tab attachments via length > 0.

Automation assessment — Injected PR Evaluation (Web Compatibility & Security)

Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

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

Labels

semver-minor New feature — triggers minor version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants