Show CPM Stats on new tab page #4 - Feature discovery dialog #2697
Conversation
…oconsentDailyUsagePack.swift Co-authored-by: Copilot <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Privacy Review task: https://app.asana.com/0/69071770703008/1212252138103836 |
macOS/LocalPackages/SwiftUIExtensions/Sources/SwiftUIExtensions/PopoverMessageView.swift
Show resolved
Hide resolved
...ackages/BrowserServicesKit/Sources/ContentScopeScripts/Resources/pages/new-tab/dist/index.js
Show resolved
Hide resolved
| console.warn("[Service] NOT broadcasting - broadcast is disabled. Source:", source); | ||
| return; | ||
| } | ||
| console.log("[Service] Broadcasting data event. Source:", source); |
There was a problem hiding this comment.
Bug: Debug console.log statements left in production JavaScript bundle
Numerous debug console.log statements have been added throughout the production JavaScript bundle. These logging statements (prefixed with [Service], [BatchedActivity], [BurnProvider], [ProtectionsProvider], [NormalizeDataProvider], [BurnToProtectionsDataBridge]) will output verbose debugging information to the browser console in production, which could impact performance and expose internal implementation details to users. These appear to be leftover debugging code from development that was accidentally included in the bundled index.js file.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (2)
| } | ||
| .buttonStyle(PlainButtonStyle()) | ||
| .padding(.leading, -6) | ||
| .padding(.trailing, 2) |
There was a problem hiding this comment.
Bug: Button style modifier incorrectly applied to VStack
In the featureDiscovery view, the .buttonStyle(PlainButtonStyle()) modifier is applied to the VStack container instead of the Button inside it. This is inconsistent with the other close button implementations in the same file (e.g., messageWithTitleBody at line 192 correctly applies buttonStyle directly to the Button). The close button in the feature discovery popover may not have the expected plain button appearance.
Please tell me if this was useful or not with a 👍 or 👎.
| if let newTabPageViewModel = self.windowControllersManager.mainWindowController?.mainViewController.browserTabViewController.newTabPageWebViewModel { | ||
| NSApp.delegateTyped.newTabPageProtectionsReportModel.scroller.scroll(for: newTabPageViewModel.webView) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Strong self capture in asyncAfter closure
The DispatchQueue.main.asyncAfter closure in openNewTabWithSpecialAction captures self strongly without using [weak self]. While the AutoconsentStatsPopoverCoordinator is stored on AppDelegate and has a long lifetime, this creates an implicit strong reference that could keep the coordinator alive if it were to be deallocated before the 1.5-second delay completes, potentially causing unexpected behavior.
Please tell me if this was useful or not with a 👍 or 👎.
| } else { | ||
| try container.encodeNil(forKey: .totalCookiePopUpsBlocked) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Explicit null encoding breaks component selection logic
The custom encode method explicitly encodes nil as JSON null for totalCookiePopUpsBlocked. However, the JavaScript code at line 10194 checks totalCookiePopUpsBlocked === void 0 to determine whether to use the legacy component. When Swift sends null instead of omitting the key entirely, JavaScript receives null (not undefined), causing the condition to be false when it should be true. This means when CPM is disabled, the new component is incorrectly rendered instead of the legacy component. The encodeNil call should be removed to preserve the original behavior where the key is omitted when the value is nil.
Please tell me if this was useful or not with a 👍 or 👎.
Task/Issue URL: https://app.asana.com/1/137249556945/task/1211708488121003
Tech Design URL:
CC:
Description
Testing Steps
Impact and Risks
What could go wrong?
Quality Considerations
Notes to Reviewer
Internal references:
Definition of Done | Engineering Expectations | Tech Design Template
Note
Introduces a feature‑discovery popover for cookie pop‑up stats and wiring to scroll the NTP Protections Report, with supporting pixels, UI hooks, popover API updates, and tests; also refines burn behavior for Autoconsent stats.
AutoconsentStatsPopoverCoordinatorandPresenterto show a feature‑discovery popover (with icon, i18n, auto‑dismiss, click/close handlers) after thresholds; triggered on app active; dismissed on new tab; debug menu hooks added.shown/closed/clicked/new-tab-opened/autodismissed).NewTabPageProtectionsReportScrollerand client messageprotections_scroll;ProtectionsHeadinglistens and scrolls into view.getExpandedPerformanceMetricsnow supports a timeout (viaexpandedTimeoutMsfeature setting); easing calc refactored.PopoverMessageView(Controller)withfeatureDiscoverystyle,clickAction,onClose, and auto‑dismiss callbacks; update existing usages (e.g., updates/broken‑site).Cookies-Blocked-Color-24asset and localizations; update project files; bumpcontent-scope-scriptsdependency.Written by Cursor Bugbot for commit 1abacdd. This will update automatically on new commits. Configure here.