git_admin/obsidian-vault
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
0a6fe198575be74af140087c1facab7f258eef59
obsidian-vault/work/wiki/apple-browsers/chat-path-onboarding-tracker-blocking.md
T

18 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 Base Review status
#4544 demo-tracker-blocking-onboarding main Open — review required (Alessandro + Rachel left comments)
#4591 demo-tracker-blocking-onboarding-ui-polish #4544 Approved
#4664 demo-tracker-blocking-onboarding-chat-path-dialog-polish #4591 Approved
#4855 demo-tracker-blocking-onboarding-sr-feedback #4664 Open — no review yet. Has uncommitted local changes (see below).
#4668 demo-tracker-blocking-onboarding-uti-flow ? Open — no review yet. Has open Bugbot + reviewer comments (see below).

Uncommitted Local Changes — PR #4855 branch (sr-feedback)

All of the following are in the working tree but NOT committed or pushed. They must be committed before #4855 can be reviewed or merged.

1. NewTabPageViewController.swift — setBarsHidden + delegate pattern + loading guard

What changed:

  • Replaced (parent as? MainViewController)?.setChatPathVisitSiteControlsLocked(true/false) + chromeDelegate?.setNavigationBarHidden(true/false) with a single chromeDelegate?.setBarsHidden(true/false, animated: false, customAnimationDuration: nil) call.
  • Removed all (parent as? MainViewController)? casts from NewTabPageViewController.
  • Loading guard (guard ... isLoading != true else { return }) was a standalone guard that would abort dialog presentation entirely, leaving an orphaned hostingController reference. Folded into the outer if condition using the new delegate method instead.
  • dismiss() now calls setBarsHidden(false) when didHideBarsForChatPathVisitSiteDialog is true.

Why setChatPathVisitSiteControlsLocked calls were removed: Alessandro's review comment on #4855 line 280 asked to use NewTabPageControllerDelegate instead of casting (parent as? MainViewController). A separate review comment on #4855 line 477 noted Costas's preference to hide both address bar and toolbar via setBarsHidden(false). Using setBarsHidden via chromeDelegate removes the need for the cast entirely, making setChatPathVisitSiteControlsLocked unreachable. The method definition still exists in MainViewController+DuckAIExperiment.swift:166 as dead code — needs to be deleted.

Source: Alessandro review comments on PR #4855 (line 280, line 473, line 477) + Costas's preference cited therein + Bugbot #4855 "Guard early return aborts entire dialog" + Bugbot #4855 "Navigation bar not restored when NTP dismissed".

Current state: ✏️ Uncommitted local change on sr-feedback branch.

2. NewTabPageControllerDelegate.swift + MainViewController.swift — new delegate method

What changed: Added newTabPageControllerCurrentTabIsLoading(_ controller: NewTabPageViewController) -> Bool to the NewTabPageControllerDelegate protocol. Implemented in MainViewController.swift to return currentTab?.isLoading == true.

Source: Alessandro review comment on #4855 line 473: "should we add (parent as? MainViewController)?.currentTab?.isLoading != true as condition of the if instead of having the guard..." Also addresses Bugbot #4855 "Guard early return aborts entire dialog presentation" and removes the (parent as?) cast as requested on #4855 line 280.

Current state: ✏️ Uncommitted local change on sr-feedback branch.

3. OnboardingIntroViewModel.swift — returning-user guard + remove hardcoded cohort override

What changed:

  • Uncommented guard case .introDialog(isReturningUser: false) = introSteps.first in insertExperimentStepIfNeeded(). This ensures sync-restore / returning users are excluded from the Duck.ai query experiment step.
  • Removed hardcoded return .treatmentB override from resolveDuckAIQueryExperimentCohortID() (was left in for local testing).

Source (returning-user guard): Alessandro May 7 review on PR #4544, Asana task 1214682678629767, Bugbot on #4855 "Returning-user guard commented out despite PR intent". The guard was temporarily commented out during development; it must be active before shipping.

Source (hardcoded override removal): Bugbot on #4855 "Hardcoded debug return bypasses feature flag entirely". This was a local testing override (return .treatmentB) that must not ship.

Current state: ✏️ Uncommitted local change on sr-feedback branch.

4. UserText.swift + all 26 lproj files — searchAndDuckAIOption string reverted + "Ask AI" toggle label

What changed:

  • searchAndDuckAIOption reverted to "Toggle between\nSearch and Duck.ai" (was changed to "Toggle between\nSearch and Ask AI" in a prior commit).
  • DuckAIQuery screen toggle label changed from "Duck.ai" to "Ask AI" in OnboardingView+DuckAIExperimentSearchContent.swift.

Source (revert): Alessandro review comment on #4855 UserText.swift:2375: "the rename of 'Duck.ai' to 'Ask AI' is for the toggle we show in the Duck Ai Query screen. The copy for this screen should be reverted." — meaning the Search Experience screen copy stays as "Duck.ai", only the DuckAI query toggle label becomes "Ask AI".

Source (Ask AI label): GJ8 ship review: "change 'Duck.ai' to 'Ask AI' in the toggle screen in onboarding only."

⚠️ Unresolved copy question: Alessandro's comment says "Ask.ai" (with dot). Gary's GJ8 says "Ask AI" (no dot). Needs explicit confirmation from product/design before shipping.

Current state: ✏️ Uncommitted local change on sr-feedback branch.

5. Dead code: setChatPathVisitSiteControlsLocked in MainViewController+DuckAIExperiment.swift:166

What: The method has no callers now that NewTabPageViewController uses setBarsHidden via the chrome delegate instead. Should be deleted.

Source: Removal of callers in item #1 above.

Current state: ✏️ Still in code, needs deletion. Uncommitted.

Open GitHub Comment Threads — PR #4544

These Bugbot threads were on PR #4544 and are not resolved on GitHub. Status of each:

[Bugbot] Hardcoded return forces all users into treatment group (OnboardingIntroViewModel.swift)

Issue: return .treatmentA hardcoded override in resolveDuckAIQueryExperimentCohortID().
Code state: The .treatmentA override was removed on the #4544 branch (replaced with .treatmentB for testing, then also removed — see uncommitted change #3 above). The #4544 branch tip no longer has .treatmentA hardcoded.
Thread state: 🔴 Not resolved on GitHub. Bugbot comment was on an old commit — outdated. → Resolve thread.

[Bugbot] Subscription promo override always returns true (OnboardingSubscriptionPromotionHelper.swift)

Issue: shouldDisplay was hardcoded to return true for testing.
Code state: Reverted in commit f8d5ba2 on #4544 branch.
Thread state: 🔴 Not resolved on GitHub. Fix is in the branch. → Resolve thread.

[Bugbot] Chat-path subscription redirect commented out, variable unused (RebrandedNewTabDaxDialogFactory.swift)

Issue: isChatPathSubscriptionPromo was commented out, variable unused.
Code state: Dead code removed in commit e682475 on #4544 branch.
Thread state: 🔴 Not resolved on GitHub. Fix is in the branch. → Resolve thread.

[Bugbot] Missing pixel definitions (PixelEvent.swift:2120)

Issue: onboardingChatPathTryVisitSiteUnique and onboardingChatPathTrackersBlockedUnique missing from PixelEvent.
Code state: Both defined in commit ec42fd8 on #4544 branch (lines 268–269, 2119–2120 of PixelEvent.swift).
Thread state: 🔴 Not resolved on GitHub. Fix is in the branch. → Resolve thread.

[Bugbot] Chat-path completion dialog shown twice (MainViewController.swift:4711)

Issue: Two competing paths (tabDidRequestNewTab dispatch + direct call) could both invoke presentChatPathOnboardingCompletionIfNeeded().
Code state: presentChatPathOnboardingCompletionIfNeeded() is guarded by chatPathPhase == .trackerToEOJ. Whether the double-call race was fully resolved needs verification.
Thread state: 🔴 Not resolved on GitHub. → Verify code, then resolve thread.

[Bugbot r3234645543] Chat-path EOJ not shown after tapping tracker dialog CTA (TabViewController.swift:4214)

Issue: didTapDismissContextualOnboardingAction doesn't call tabDidRequestNewTab for chat path, so tapping "Got it" on tracker dialog doesn't open a new tab for the EOJ.
Code state: 🔴 NOT fixed on the #4544 branch. The fix (if chatPathPhase == .trackerToEOJ { tabDidRequestNewTab } in didTapDismissContextualOnboardingAction) exists only on the sr-feedback branch (commit 6c3774e4f8). It will be present when the full PR stack merges, but it is not in #4544 itself.
Thread state: 🔴 Not resolved on GitHub. Fix is not in this PR — it's in #4855. Note this in the thread or backport the fix to #4544.

Open GitHub Comment Threads — PR #4664

[Alessandro + Bugbot] Title changes for all paths, not just chat path (RebrandedNewTabDaxDialogFactory.swift)

Issue: createSubsequentDialog title was changed for all flows, not gated on isChatPath.
Code state: Fixed — isChatPath check added (daxDialogsFlowCoordinator.chatPathPhase == .visitSite). The rebranded factory now uses the chat-path title conditionally.
Thread state: Likely resolved by the fix but confirm on GitHub. → Check and resolve thread.

[Bugbot] Dismiss button appears on standard path too (RebrandedContextualOnboardingDialogs+SubscriptionPromo.swift:101)

Issue: Dismiss button added for chat path appeared on standard path too.
Code state: Needs verification on #4664 branch.
Thread state: 🔴 Not confirmed resolved. → Verify code.

Open GitHub Comment Threads — PR #4668

[aataraxiaa] Commented-out Config(...) line in FeatureFlag.swift:635

Issue: // Config(source: .remoteReleasable(.subfeature(AIChatSubfeature.unifiedToggleInput))) — a dead commented-out duplicate still in code.
Source: aataraxiaa review comment: "Obviously you just changed this for testing, but just a reminder to change it back before we merge."
Code state: 🔴 Still in code on #4668 branch. The defaultValue: .enabled override + commented-out original are both present.
Thread state: 🔴 Not resolved. → Remove the commented-out line and the defaultValue: .enabled before merging.

[Bugbot] Hardcoded debug return bypasses cohort resolution (OnboardingIntroViewModel.swift:473)

Issue: Same return .treatmentB override as above.
Code state: Removed in the uncommitted local change on sr-feedback branch. Will be present once committed and the stack is built. Not yet fixed on #4668 branch itself.
Thread state: 🔴 Not resolved. → Will be resolved when sr-feedback is merged down or the fix is backported.

[Bugbot] Protocol method embedInUnifiedInputEditingAreaIfActive never called (MainViewController.swift + BrowserChromeManager.swift)

Issue: Method added to BrowserChromeDelegate protocol and implemented in BrowserChromeManager, but never called anywhere.
Code state: On the sr-feedback branch, commit 82bb0f71e3 removed the embedDialogInEditingState dead code path. The method may now be truly unreachable. Needs verification on #4668 branch.
Thread state: 🔴 Not resolved. → Verify, then either add a caller or delete the method and its implementations.

Open GitHub Comment Threads — PR #4855

All four Alessandro review comments and all Bugbot comments are addressed by the uncommitted local changes listed above. The GitHub threads are not yet resolved because the changes haven't been committed/pushed.

Comment Fix location Thread resolved?
Use NewTabPageControllerDelegate instead of (parent as?) cast (line 280) Uncommitted — NewTabPageControllerDelegate.swift 🔴 No
Hide both bars via setBarsHidden per Costas (line 477) Uncommitted — NewTabPageViewController.swift 🔴 No
Fold loading check into if condition (line 473) Uncommitted — NewTabPageViewController.swift 🔴 No
Revert searchAndDuckAIOption copy (UserText.swift:2375) Uncommitted — UserText.swift + lproj files 🔴 No
Bugbot: Navigation bar not restored when NTP dismissed (line 281) Uncommitted — NewTabPageViewController.swift dismiss() 🔴 No
Bugbot: Guard early return aborts dialog presentation (line 473) Uncommitted — guard folded into if 🔴 No
Bugbot: Hardcoded debug return bypasses feature flag (OnboardingIntroViewModel:473) Uncommitted — hardcoded return removed 🔴 No
Bugbot: Returning-user guard commented out (OnboardingIntroViewModel:451) Uncommitted — guard uncommented 🔴 No

→ Commit and push all local changes first. Then resolve these threads.

Pre-Ship Blockers

1. Commit and push local changes on sr-feedback

All changes in the "Uncommitted Local Changes" section above must be committed to alex/demo-tracker-blocking-onboarding-sr-feedback and pushed before PR #4855 can be reviewed. Then the eight GitHub threads above can be resolved.

Current state: 🔴 Not done.

2. Tell UTI team the experiment name

Source: Pete Apr 30 comment: Pete's team will exclude users enrolled in this experiment from the UTI feature rollout, to avoid breaking their onboarding. They need the final experiment name.

Current state: 🔴 Blocked — experiment name not final (see #4 below). Once known: fill into O-J <> O-N Coordination and O-N Live Onboarding Experiment Details.

3. Privacy config: add US/EN targeting to override

Source: Task 1214682678629768: experiment must be limited to US English users. Without this, all locales worldwide would be enrolled.

What: In privacy-configuration/overrides/ios-override.json, add "targets": [{ "localeLanguage": "en", "localeCountry": "US" }] to the experiment entry.

Current state: 🔴 Local edit exists on wrong branch (alex/remove-autocomplete-tabs) in the privacy-configuration repo, not committed.

4. Privacy config: rename the experiment

Source: Task 1214682678629768: some internal users were enrolled under onboardingDuckAIQueryExperiment during testing. A new name gives production users a clean, separate enrollment.

What — three-part change:

  1. BSK (SharedPackages/BrowserServicesKit): add a new case to AIChatSubfeature enum with the new name string.
  2. App (iOS/Core/FeatureFlag.swift): update FeatureFlag.onboardingDuckAIQueryExperiment to reference the new subfeature case.
  3. privacy-configuration repo (overrides/ios-override.json): add new experiment entry with targets, and set the old onboardingDuckAIQueryExperiment entry to "state": "disabled".

Do this in the same PR as item #3 (both touch the same file in the same repo).

Current state: 🔴 Blocked — new experiment name not decided.

5. Fix commented-out feature flag line in PR #4668

Source: aataraxiaa review comment on #4668 (see Open Threads above).

What: Remove defaultValue: .enabled and the // Config(...) dead line from FeatureFlag.swift:635 for unifiedToggleInput.

Current state: 🔴 Still in code on #4668 branch. Not fixed.

6. Clarify "Ask AI" vs "Ask.ai" copy

Source: Conflict between GJ8 ("Ask AI") and Alessandro's review comment ("Ask.ai").

Current state: 🔴 Implemented as "Ask AI" (no dot) per GJ8. Awaiting explicit product/design confirmation.

Experiment Config (current, local override)

privacy-configuration/overrides/ios-override.json under aiChat subfeatures:

"onboardingDuckAIQueryExperiment": {
  "state": "enabled",
  "targets": [{ "localeLanguage": "en", "localeCountry": "US" }],
  "cohorts": [
    { "name": "control", "weight": 1 },
    { "name": "treatmentA", "weight": 1 },
    { "name": "treatmentB", "weight": 1 }
  ]
}

Feature flag: FeatureFlag.onboardingDuckAIQueryExperiment in iOS/Core/FeatureFlag.swift
Cohort type: FeatureFlag.DuckAIQueryExperimentCohort

Reference in New Issue View Git Blame Copy Permalink