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.

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

AI Chat handoff #3785

Merged
merged 19 commits into from
Jan 14, 2025
Merged

AI Chat handoff #3785

merged 19 commits into from
Jan 14, 2025

Conversation

Bunn
Copy link
Contributor

@Bunn Bunn commented Jan 9, 2025

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

Description:
Add handoff support using the C-S-S communication layer

Steps to test this PR:
This feature depends on some FE changes in the SERP, so to test it you'll need to do a bit of initial setup:

  1. Use this privacy config
  2. Host this locally and expose it via ngrok (the config file above is setup to allow the ngrok-free domain, feel free to adjust if you're using something else)
  3. Apply this patch which will: Remove the webview callback from within the AI Chat custom view, setup the ngrok URL

  1. Open the test website, tap on "open with payload". Validate that the AI chat view displays a large payload
  2. Open the test website, tap on "open without payload". Validate that the AI chat view display null payload
  3. Open the test website, tap on "open with random payload". Validate that the AI chat view display a random payload each time (Do this a couple times)
  4. When doing the above, you can also validate if the payload is correctly consume by tapping on "Update settings".

Copy link

github-actions bot commented Jan 9, 2025

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

Generated by 🚫 dangerJS against d620815

@Bunn Bunn marked this pull request as ready for review January 10, 2025 17:19
@Bunn Bunn changed the title WIP: AI Chat handoff AI Chat handoff Jan 10, 2025
@brindy brindy self-requested a review January 14, 2025 10:35
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

Nice work. Please fix the dependency injection but otherwise looks good.

private var payloadHandler = AIChatPayloadHandler()

@MainActor
lazy var aiChatViewController: AIChatViewController = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation

Comment on lines +41 to +44
/// Default rule for DuckDuckGo AI Chat
if let ddgDomain = URL.ddg.host {
rules.append(.exact(hostname: ddgDomain))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just acknowledging we discussed this. No action required.

Comment on lines 38 to 39
let settings = AIChatSettings(privacyConfigurationManager: ContentBlocking.shared.privacyConfigurationManager,
internalUserDecider: AppDependencyProvider.shared.internalUserDecider)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we inject these instead? Since this is lazy they should be available by this point.

@@ -66,6 +67,8 @@ final class UserScripts: UserScriptsProvider {
properties: sourceProvider.contentScopeProperties,
isIsolated: true)
autoconsentUserScript = AutoconsentUserScript(config: sourceProvider.privacyConfigurationManager.privacyConfig)
aiChatUserScript = AIChatUserScript(handler: AIChatUserScriptHandler(featureFlagger: AppDependencyProvider.shared.featureFlagger))
Copy link
Contributor

Choose a reason for hiding this comment

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

please inject :)

@Bunn Bunn requested a review from brindy January 14, 2025 12:12
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

LGTM

@Bunn Bunn merged commit e662540 into main Jan 14, 2025
13 checks passed
@Bunn Bunn deleted the bunn/aichat/handoff branch January 14, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants