Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AI Chat browsing menu #3635

Merged
merged 41 commits into from
Dec 4, 2024
Merged

AI Chat browsing menu #3635

merged 41 commits into from
Dec 4, 2024

Conversation

Bunn
Copy link
Contributor

@Bunn Bunn commented Nov 27, 2024 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1204167627774280/1208794395441049/f

Description:
Add AI Chat entry point in the browsing menu

Steps to test this PR:

  1. Open the browsing menu (the icon at the bottom left).
  2. Select "Chat."
  3. It should open AI Chat in a full-screen modal. Check if we're correctly sending the "browsingMenuAIChat" pixel
  4. Start a conversation with AI Chat.
  5. Close the UI, then open it again within 10 minutes. Check if we're correctly sending the "before10min" pixel
  6. Validate that your chat is still there.
  7. Close the UI, then open it again after 10 minutes. Check if we're correctly sending the "after10min" pixel
  8. Validate that your chat is no longer there.
  9. Check that the print button in the list is still working.
  10. In FeatureFlag.swift, return disabled in aiChatBrowsingToolbarShortcut.
  11. Open the browsing menu and validate that the Chat button is not there, and the print button is at the top header and not present in the list.
  1. Log on a website or set a cookie somehow
  2. Open AI Chat
  3. Start a conversation
  4. Close AI Chat
  5. Before 10min, use the fire button
  6. Validate that the website doesn't have a cookie or you're logged out
  7. Open AI Chat, check if the webview state is still intact (conversation still visible)
  1. Open AI Chat
  2. Inside the page, tap in the top right info button, then tap "Help Pages"
  3. Check if the AI Chat page is dismissed, and we open a new tap with the help pages.

Testing the new settings page:

  1. Set yourself as internal user
  2. Check if the feature is enabled by default (it should)
  3. Check if you can see the AI Chat button in the browsing menu
  4. Open settings -> AI Chat, disable AI Chat
  5. Check if the AI Chat button is gone and print is added back to it's place

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 27, 2024 •
edited
Loading

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against 906775c

@Bunn Bunn marked this pull request as ready for review November 29, 2024 21:05
@Bunn Bunn requested a review from samsymons November 29, 2024 21:05
Bunn
Bunn commented Nov 29, 2024
View reviewed changes
DuckDuckGo/BrowsingMenu/BrowsingMenuViewController.swift
@@ -249,8 +249,9 @@ extension BrowsingMenuViewController: UITableViewDelegate {

switch menuEntries[indexPath.row] {
case .regular(_, _, _, _, let action):
dismiss(animated: true)
action()
dismiss(animated: true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the action called without waiting for the dismissal was causing problems with actions that depended on the view stack, like presenting a ViewController

@Bunn Bunn changed the base branch from main to release/7.148.0 December 2, 2024 18:34
samsymons
samsymons reviewed Dec 4, 2024
View reviewed changes
LocalPackages/AIChat/Sources/AIChat/Public API/AIChatViewController.swift
Comment on lines +193 to +196
@objc private func closeAIChat() {
chatModel.startCleanupTimer()
dismiss(animated: true)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One note that is probably not a problem: when this VC is dismissed, it triggers viewDidAppear in MainViewController. This causes various things to happen, like m.debug.app-did-show-ui-time-more firing - I'd want to check with @jaceklyp whether this pollutes the data coming from that pixel, or whether this is not an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked with @jaceklyp and we decided to prevent this pixel from being fired more than once since it should have been like this in the first place: c075d3d

@samsymons
Copy link
Contributor

Added a few comments, only a couple are actionable though. Overall this looks solid, I've spent a bunch of time bashing on it on both iPhone and iPad, trying to break the lifecycle of it in various ways, no issues found. Once the comments are resolved where needed, I think we can approve this.

jaceklyp
jaceklyp approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@jaceklyp jaceklyp left a comment

Choose a reason for hiding this comment

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

thank you!

samsymons
samsymons approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

:shipit:

@Bunn Bunn merged commit edf899d into release/7.148.0 Dec 4, 2024
14 checks passed
@Bunn Bunn deleted the bunn/aichat/browsing-menu branch December 4, 2024 19:42
samsymons added a commit that referenced this pull request Dec 5, 2024
@samsymons
Merge branch 'main' into sam/widget-tint-color-improvement
ef73afb 
* main:
  Release 7.148.0-2 (#3676)
  Merge hotfix changes from 7.147.1 (#3674)
  Add `install` pixel (#3656)
  AI Chat browsing menu (#3635)
  Release 7.147.1-0 (#3672)
  Use actor isolation for `SubscriptionFeatureMappingCache` (#3670)
  cherry picked - already approved in #3666
  fix suggestion scrolling (#3654)
  Update autoconsent to v12.1.0 (#3650)
  Malware protection: bump BSK (#3617)
  Release 7.148.0-1 (#3661)
  Make a simple state machine to identify incorrect transitions  (#3660)
  support local storage fireproofing (#3612)
  Make NTP Grid layout adaptive for various screen sizes (#3657)
  Release 7.148.0-0 (#3659)
  Bump BSK, add Free Trials Feature Flag (#3655)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaceklyp jaceklyp jaceklyp approved these changes

@samsymons samsymons samsymons approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bunn @samsymons @jaceklyp