Files

T

Alexey Martemyanov 918a31286f 2026-05-16 media-toolbox: session 9 — cyr→lat fix, year-г fix, genre routing

2026-05-15 15:57:57 +06:00

35 KiB

Raw Blame History

iOS: Chat-Path Onboarding — Tracker Blocking Demo

Asana: Ship Review task 1214147157456478
Branch stack: demo-tracker-blocking-onboarding → ui-polish → chat-path-dialog-polish → sr-feedback → uti-flow
Due: 2026-05-15

PR Stack

PR	Branch	Review status
#4544	`demo-tracker-blocking-onboarding`	Open — review required (Alessandro + Rachel left comments)
#4591	`demo-tracker-blocking-onboarding-ui-polish`	Approved
#4664	`demo-tracker-blocking-onboarding-chat-path-dialog-polish`	Approved
#4855	`demo-tracker-blocking-onboarding-sr-feedback`	Open — no review yet
#4668	`demo-tracker-blocking-onboarding-uti-flow`	Open — no review yet

Open Items

✅ SR — GJ2/ Remove X button from chat-path dialogs

Source: Gary Apr 29

Fix: X (manual dismiss) button removed from "Try visiting a site!", trackers-blocked, and "Oh, before I forget…" dialogs on the chat path.

State: Done. Committed in PR #4664.

✅ SR — GJ4/ Copy: "Next, try visiting a site!" after trying AI

Source: Gary Apr 29

Fix: "Try visiting a site!" dialog title updated to "Next, try visiting a site!" for chat-path context.

State: Done. Committed in PR #4664.

Source: Gary Apr 29, confirmed by Gary May 7 and Costas May 8: hide address bar + toolbar for AI-chat flow only so user is forced to choose from presets. Without this: user could type a search (repeating the same dialog) or switch to Duck.ai.

Fix: chromeDelegate?.setBarsHidden(true) on show, setBarsHidden(false) on dismiss. Tracked as PR #4855 — Alessandro (line 477) item below.

State: sr-feedback / uncommitted — in working tree, NOT committed.

🟠 SR — GJ7/ Exclude sync-restore / returning users from Duck.ai experiment step

Source: Alessandro May 7: if user restored data from sync, Dax dialogs are disabled — enrolling them would break the flow. Alex confirmed addressed in May 13 build comment ("currently commented out in PR").

Fix: Uncommented guard case .introDialog(isReturningUser: false) = introSteps.first in insertExperimentStepIfNeeded(). Tracked as PR #4855 — Bugbot "Returning-user guard commented out" item below.

State: sr-feedback / uncommitted — in working tree, NOT committed.

🟠 SR — GJ8/ Change "Duck.ai" to "Ask AI" on toggle choice screen (onboarding only)

Source: Gary May 6: "Let's also change 'Duck.ai' to 'Ask AI' in the toggle screen in onboarding. Only change it in onboarding though, not the standard toggle."

Fix: DuckAI query toggle label updated to "Ask AI" in OnboardingView+DuckAIExperimentSearchContent.swift and UserText.swift. Tracked as PR #4855 — Alessandro (UserText.swift:2375) item below.

⚠️ Alessandro's review comment says "Ask.ai" (with dot) vs GJ8 "Ask AI" (no dot) — needs product confirmation.

State: sr-feedback / uncommitted — in working tree, NOT committed.

🔴 SR — GJ6b/ "You've got this" EOJ fails after tapping "Got it" on tracker blocking step

Source: Gary Apr 29: "This dialogue fails to appear after tapping 'Got it' on the tracker blocking step. As a result, we also skip the subscription upsell dialogue."

This is the same bug as PR #4544 — Bugbot r3234645543 below. Fix is in didTapDismissContextualOnboardingAction on sr-feedback branch, not yet on #4544 branch.

State: 🔴 Fix exists in PR #4855 (sr-feedback), NOT in PR #4544. GitHub thread on #4544 NOT resolved.

🔴 SR — GJ1/ Animated toggle thumbnail

Source: Gary Apr 29 + May 6 confirmed in scope. Costas May 8: task 1214627423981798 created for animated assets. If not in time, defer to follow-up project. Alessandro May 10: nice-to-have, defer if needed.

State: 🔴 NOT implemented. Nice-to-have — defer to follow-up if time doesn't allow.

🔴 SR — GJ6a/ UTI: keyboard appears after choosing AI, hiding response and fire button

Source: Gary Apr 29. Pete May 7: UTI experiment participants will be excluded from this experiment; UTI issues can be fixed in follow-up before June 1. Alessandro May 7: follow-up task 1214592943696234.

State: 🔴 NOT implemented here. Scoped to separate follow-up project (UTI + experiment winner work).

🔴 SR — GJ6/ UTI new breakage (visit-site dialog, bar state)

Source: Alex May 13: "new breakage since my last fixes, will do next." Tracked in PR #4668.

State: 🔴 In progress. PR #4668 has open comment threads (see aataraxiaa + Bugbot items below).

➖ SR — GJ5/ Add new Dax brand assets to onboarding

Source: Gary Apr 29.

State: Deferred. Costas/Alessandro agreed: out of scope for this project, addressed in follow-up.

➖ SR — CB1/ Welcome screen copy update

Source: Costas May 8. Alessandro May 10: comes for free once task 1213967368170629 merges.

State: Covered by separate task. No action needed here.

✏️ COMMIT-1 — All working-tree changes on `sr-feedback` are uncommitted

State: All of the items below marked sr-feedback / uncommitted are local-only on alex/demo-tracker-blocking-onboarding-sr-feedback. None have been committed or pushed.

iOS/DuckDuckGo/MainViewController.swift
iOS/DuckDuckGo/NewTabPageControllerDelegate.swift
iOS/DuckDuckGo/NewTabPageViewController.swift
iOS/DuckDuckGo/OnboardingFlow/LinearOnboarding/OnboardingIntroViewModel.swift — also needs !restorePromptHandler.isEligibleForRestorePrompt() added (see PR #4855 Alessandro line 450)
iOS/DuckDuckGo/OnboardingView+DuckAIExperimentSearchContent.swift — label currently "Ask AI", needs correction to "Ask.ai" (with dot)
iOS/DuckDuckGo/UserText.swift + all 26 lproj files

Action: Fix "Ask.ai" copy + add isEligibleForRestorePrompt guard, then commit and push before #4855 can be reviewed.

Color legend: 🟠 fix in working tree, uncommitted — 🟡 fix in another PR in stack, not merged to this one — 🔴 not fixed / blocked — ✅ done

PR #4544 Comments

Threads in GitHub order. Resolved threads grouped into tables.

Source: Four Bugbot comments on early commits of PR #4544.

Thread	Fix	Code state
Hardcoded `return .treatmentA` in `OnboardingIntroViewModel`	Removed in later commit on #4544	✅ Fixed (Bugbot commit is outdated)
`shouldDisplay` always returns `true` in `OnboardingSubscriptionPromotionHelper`	Reverted in commit `f8d5ba2` on #4544	✅ Fixed
Missing pixel definitions in `PixelEvent.swift`	Added in commit `ec42fd8` on #4544	✅ Fixed
`isChatPathSubscriptionPromo` commented out, variable unused in `RebrandedNewTabDaxDialogFactory`	Removed in commit `e682475` on #4544	✅ Fixed

State: All four fixed in code. All four GitHub threads are NOT resolved (outdated Bugbot comments never closed). → Resolve all four threads.

✅ [Resolved] PR #4544 — Bugbot: Chat-path completion dialog shown twice via competing paths

Source: Bugbot on PR #4544, MainViewController.swift:4711

Issue: Two independent paths both call presentChatPathOnboardingCompletionIfNeeded() — tabDidRequestNewTab dispatch and a direct call — potentially showing the completion dialog twice.

State: presentChatPathOnboardingCompletionIfNeeded() has a guard on chatPathPhase == .trackerToEOJ — once the EOJ fires, phase advances and the second call is a no-op. GitHub thread is [Resolved].

✅ [Resolved] PR #4544 — Bugbots + Alessandro (additional resolved threads)

All resolved on GitHub. Fixed in later commits on the #4544 branch.

Thread	Fix
`DefaultDaxDialogsSettings.chatPathPhase` missing `isChatFirstPath` guard	Added in later commit
Chat-path tests miss `isChatFirstPath` setup	Fixed in tests
Debug reset omits clearing `isChatFirstPath` flag	Fixed in `OnboardingDebugView`
Duplicated `chatPathPhase` logic across `DaxDialogs` + `ContextualDaxDialogsFactory`	Consolidated into `DaxDialogsSettings`
Alessandro: pixels `@rachelmcr` fyi (`PixelEvent.swift:268`)	Informational; no action required
Alessandro: tests replicating real `DaxDialogsSettings` logic	Simplified tests
`TrackerToEOJ` phase test also needs browsing dialog flag	Fixed in tests
Missing fallback when AI Chat disabled during chat path (`MainViewController.swift:4712`)	Fallback to `.final` added
`isChatPath` logic in `createFinalDialog` is unreachable	Dead branching removed

🔴 PR #4544 — Alessandro (DaxDialogs.swift:125): `DaxDialogProvider` refactoring suggestion

Source: Alessandro May 7 review on PR #4544, DaxDialogs.swift:125

Issue: Alessandro asked whether having a DaxDialogProvider that internally delegates to either DaxDialogs or a new DuckAIDaxDialogs would be a significant change — to separate responsibilities since the chat-path flow doesn't need search dialogs.

State: 🔴 [OPEN] on GitHub. Alessandro's own follow-up (May 10): "No time for this, let's re-assess once the experiment is live." → Deferred to post-experiment follow-up.

🔴 PR #4544 — Alessandro (ContextualOnboardingNewTabDialogFactoryTests.swift:172): Tests are for the legacy factory

Source: Alessandro May 7 review on PR #4544, ContextualOnboardingNewTabDialogFactoryTests.swift:172

Issue: "Just bear in mind that these tests are for the legacy factory." Tests added under // MARK: - Chat Path – Subsequent Dialog target NewTabDaxDialogFactory (legacy), not RebrandedNewTabDaxDialogFactory (active under the rebranding flag).

State: 🔴 [OPEN] on GitHub. Rebranded factory coverage may be insufficient.

🔴 PR #4544 — Bugbot: Dead property `pendingCompletionDialogMessage` never set non-nil (`NewTabDaxDialogFactory.swift:192`)

Source: Bugbot on PR #4544, NewTabDaxDialogFactory.swift:192

Issue: NewTabDaxDialogFactory.createFinalDialog was updated with chat-path conditional logic (chatPathPhase == .trackerToEOJ && isAIChatEnabled choosing a different message + pixel), but RebrandedNewTabDaxDialogFactory.createFinalDialog was not updated to match — despite its section mark being renamed to "Chat-Path Completion". The .final spec is currently unreachable for chat-path + AI-enabled users (the chat EOJ is driven by presentChatPathOnboardingCompletionIfNeeded instead), so pendingCompletionDialogMessage is always nil.

State: 🔴 [OPEN] on GitHub. The dead branching in NewTabDaxDialogFactory was removed in a later commit; verify RebrandedNewTabDaxDialogFactory is consistent.

🟡 PR #4544 — Bugbot (r3234645543) / SR GJ6b/: Chat-path EOJ not shown after tapping tracker dialog CTA

Source: Bugbot comment r3234645543 on PR #4544, TabViewController.swift:4214. Also reported as SR GJ6b/ by Gary Apr 29 — same bug.

Issue: When user taps "Got it" on the tracker-blocked dialog, the code goes through didTapDismissContextualOnboardingAction, which does not call tabDidRequestNewTab. The EOJ new tab is never opened.

Fix: Added if contextualOnboardingLogic.chatPathPhase == .trackerToEOJ { delegate?.tabDidRequestNewTab(self) } to didTapDismissContextualOnboardingAction. (Same block was already in didNavigateAwayFromContextualOnboardingDialog.)

State: Fix exists in commit 6c3774e4f8 on sr-feedback branch (PR #4855). NOT present on the #4544 branch itself. Fix will land in main when the full PR stack merges. GitHub thread on PR #4544 is NOT resolved.

🔴 PR #4544 — Bugbot: Rebranded factory's final dialog missing chat-path branching (`RebrandedNewTabDaxDialogFactory.swift:202`)

Source: Bugbot on PR #4544, RebrandedNewTabDaxDialogFactory.swift:202

Issue: NewTabDaxDialogFactory.createFinalDialog has chat-path branching (chatPathPhase == .trackerToEOJ && isAIChatEnabled), but RebrandedNewTabDaxDialogFactory.createFinalDialog does not — both are selected by the onboardingRebranding feature flag. While .final is currently unreachable for chat-path + AI-enabled users, divergence could cause the wrong message + pixel if the flow changes.

State: 🔴 [OPEN] on GitHub. Needs symmetry check between the two factories.

PR #4591 Comments

PR #4591 (demo-tracker-blocking-onboarding-ui-polish) is Approved. Open threads still need resolving before merge. Threads in GitHub order.

🔴 PR #4591 — Bugbot (SyncSettingsViewController.swift:738): Experiment pixel skips `.connect` receiver

Source: Bugbot on PR #4591, SyncSettingsViewController.swift:738

Issue: The experiment success pixel fires for .recovery sync path but skips .connect. mallexxx acknowledged: pixel originates in upstream PR #4575, not this PR.

State: 🔴 [OPEN] on GitHub. Acknowledged as upstream issue.

✅ [Resolved] PR #4591 — Bugbot (SyncSettingsViewController.swift:567): Missing pixel when `shouldShowSyncEnabled`

Source: Bugbot on PR #4591, SyncSettingsViewController.swift:567

State: ✅ [Resolved] on GitHub.

🔴 PR #4591 — Bugbot (SyncSettingsViewController.swift:588): Missing experiment success pixel in `controllerDidCreateSyncAccount`

Source: Bugbot on PR #4591, SyncSettingsViewController.swift:588

State: 🔴 [OPEN] on GitHub. Needs verification / fix.

🔴 PR #4591 — Bugbot (MainViewController.swift): Removed `performCancel` may leave omnibar in editing state

Source: Bugbot on PR #4591, MainViewController.swift

Issue: Removing performCancel call may leave the omnibar stuck in editing state in certain paths.

State: 🔴 [OPEN] on GitHub. Needs verification.

🔴 PR #4591 — Bugbot (HistoryCleaner.swift:74): nil-coalescing

Source: Bugbot on PR #4591, HistoryCleaner.swift:74

State: 🔴 [OPEN] on GitHub. Needs review.

✅ [Resolved] PR #4591 — Bugbot (NewTabPageViewController.swift): Duplicated editing-state embedding constraint logic

Source: Bugbot on PR #4591, NewTabPageViewController.swift

State: ✅ [Resolved] — embedDialogInEditingState removed in sr-feedback.

🔴 PR #4591 — Alessandro (RebrandedNewTabDaxDialogFactory.swift): Closure sent via notification

Source: Alessandro review on PR #4591, RebrandedNewTabDaxDialogFactory.swift

Issue: "Not a big fan of sending this closure in a notification, but I guess this would require some refactoring to change it." Settings deep-link callback dispatched via NotificationCenter with a closure payload.

State: 🔴 [OPEN] on GitHub. Alessandro acknowledged it's a refactor; no fix required to merge.

🔴 PR #4591 — Alessandro (MainViewController.swift): `as? (() -> Void)` runtime cast

Source: Alessandro review on PR #4591, MainViewController.swift

Issue: as? (() -> Void) cast is only checked at runtime — if the factory's closure signature ever changes, it will silently fail.

State: 🔴 [OPEN] on GitHub. Fixed in commit 482056566b on sr-feedback (wrapped in a typed SettingsDeepLinkCallback struct). Fix is NOT on #4591 branch; will land when stack merges.

🟡 PR #4591 — Alessandro (NewTabPageViewController.swift): GJ3 — hide address bar

Source: Alessandro review on PR #4591, NewTabPageViewController.swift

Issue: Alessandro flagged the GJ3 requirement to hide the address bar for the visit-site step.

State: 🟡 [OPEN] on GitHub thread. Fix implemented in sr-feedback branch (PR #4855) via setBarsHidden. Will land when stack merges.

🔴 PR #4591 — Bugbot (RebrandedNewTabDaxDialogFactory.swift): `onPresented` passes `false` but sheet was shown

Source: Bugbot on PR #4591, RebrandedNewTabDaxDialogFactory.swift

Issue: onPresented closure is called with false despite the subscription sheet having been shown, leading to incorrect state tracking.

State: 🔴 [OPEN] on GitHub. Needs fix.

✅ [Resolved] PR #4591 — Bugbots (additional resolved threads)

Thread	State
Verbose multi-line comments explain standard patterns (`NewTabPageViewController.swift`)	[Resolved]
Missing `?? parent` fallback when finding editing controller (`NewTabPageViewController.swift`)	[Resolved] — editing state approach removed
Private method `embedDialogInEditingState` is never called (`NewTabPageViewController.swift`)	[Resolved] — method removed in `sr-feedback`

PR #4664 Comments

Threads in GitHub order.

✅ [Resolved] PR #4664 — Bugbot: NTP dialog title changed for all paths, not just chat-path

Source: Bugbot + Alessandro review on PR #4664, RebrandedNewTabDaxDialogFactory.swift

Fix: isChatPath check added — title now conditional on chatPathPhase == .visitSite.

State: ✅ [Resolved] on GitHub. Fixed in PR #4664.

🔴 PR #4664 — Bugbot (RebrandedContextualOnboardingDialogs+SubscriptionPromo.swift:101): Dismiss button appears on standard path

Source: Bugbot on PR #4664, RebrandedContextualOnboardingDialogs+SubscriptionPromo.swift:101

Issue: Dismiss button is rendered unconditionally, but should only appear on the chat path.

State: 🔴 [OPEN] on GitHub. Needs a chat-path guard on the dismiss button.

PR #4668 Comments

Threads in GitHub order.

🔴 PR #4668 — Bugbot (OnboardingIntroViewModel.swift:473): Hardcoded debug `return`

Source: Bugbot on PR #4668, OnboardingIntroViewModel.swift:473

Issue: A hardcoded return .treatmentB debug shortcut bypasses the feature flag in resolveDuckAIQueryExperimentCohortID(). Must be removed before shipping.

State: 🔴 [OPEN] on GitHub. Same item appears on PR #4855 (Bugbot line 473). Not yet removed on #4668 branch.

🔴 PR #4668 — Bugbot (FeatureFlag.swift:635): Commented-out `Config(...)` line

Source: Bugbot on PR #4668, FeatureFlag.swift:635

Issue: defaultValue: .enabled override was added to unifiedToggleInput for testing, plus the old // Config(...) line still sits underneath as dead commented-out code. Both need to be removed before merging.

State: 🔴 Still in code on the #4668 branch. NOT fixed. GitHub thread NOT resolved.

🔴 PR #4668 — Bugbot (MainViewController.swift:3457): Protocol method `embedInUnifiedInputEditingAreaIfActive` never called

Source: Bugbot on PR #4668, MainViewController.swift:3457

Issue: Method added to BrowserChromeDelegate protocol and implemented, but no caller exists.

State: 🔴 [OPEN] on GitHub. See also BrowserChromeManager item below — same root issue.

🔴 PR #4668 — aataraxiaa (FeatureFlag.swift:635): Commented-out config line

Source: aataraxiaa review comment on PR #4668, FeatureFlag.swift:635

Issue: Same commented-out Config(...) dead code flagged by Bugbot above. aataraxiaa also requests it be removed.

State: 🔴 [OPEN] on GitHub. Not yet fixed.

🔴 PR #4668 — Bugbot (BrowserChromeManager.swift:43): New protocol method no callers

Source: Bugbot on PR #4668, BrowserChromeManager.swift:43

Issue: embedInUnifiedInputEditingAreaIfActive added to BrowserChromeDelegate protocol and implemented in BrowserChromeManager, but no caller exists anywhere in the codebase.

State: On sr-feedback branch commit 82bb0f71e3 removed embedDialogInEditingState dead code; the protocol method may now be entirely unreachable. Needs verification on #4668 branch. GitHub thread NOT resolved. → Either add a caller or remove the method and its implementation.

PR #4855 Comments

Threads in GitHub order.

Source: Bugbot on PR #4855, NewTabPageViewController.swift:281

Fix: dismiss() now checks didHideBarsForChatPathVisitSiteDialog and calls setBarsHidden(false) before removing the view. Previously only dismissHostingController() restored the bars, but dismiss() (called when the NTP is removed via removeHomeScreen()) did not.