Skip to content

Duck.ai - support for standalone migration #7046

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
Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Duck.ai - support for standalone migration #7046

wants to merge 3 commits into from

Conversation

@shakyShane
Copy link
Collaborator

@shakyShane shakyShane commented Nov 3, 2025 •
edited
Loading

Task/Issue URL: https://app.asana.com/1/137249556945/project/1201141132935289/task/1211793505313505?focus=true

LLM disclosure

  • I used an LLM to find all the bits I needed to update
  • for example, the part where I allow navigations within the sheet between duckduckgo.com and duck.ai
  • I used it to create the implementations for the message handlers, I just made sure the output was matching current conventions and styles.

Description

  • added standaloneMigration feature flag, defaulted to false
  • added 4 methods to allow duckduckgo/duck.ai to use native to store serialized data

Steps to test this PR

  • Run this branch
  • Use chat as normal, adding a few chats
  • Then, enable the feature flag
  • Restart the App
  • Now when you load the chat app, you should see the migration modal
  • Complete the flow

Feature 1

  • [ ]
  • [ ]

UI changes

Before After
!(Upload before screenshot) (Upload after screenshot)

shakyShane
shakyShane commented Nov 3, 2025
View reviewed changes
duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/helper/DuckChatJSHelper.kt
private val duckChatPixels: DuckChatPixels,
private val dataStore: DuckChatDataStore,
) : DuckChatJSHelper {
private val migrationItems = mutableListOf<String>()
Copy link
Collaborator Author

@shakyShane shakyShane Nov 3, 2025

Choose a reason for hiding this comment

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

@joshliebe is this ok here? the data is not designed to stick around, so this seemed ok - but please let me know otherwise!

Copy link
Contributor

@malmstein malmstein Nov 3, 2025

Choose a reason for hiding this comment

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

@shakyShane what is being stored here? is it a blob?

Copy link
Collaborator Author

@shakyShane shakyShane Nov 6, 2025

Choose a reason for hiding this comment

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

yep, just a string

Copy link
Contributor

@joshliebe joshliebe Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What happens in the unhappy case? If the app is killed while this is processing will the user lose all their data?

malmstein
malmstein requested changes Nov 3, 2025
View reviewed changes
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

There are too many things that need changing / discussing before this is ready to be merged.

Might be easier to go over all this on Zoom with @joshliebe too and get it sorted.

duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/helper/DuckChatJSHelper.kt
private val duckChatPixels: DuckChatPixels,
private val dataStore: DuckChatDataStore,
) : DuckChatJSHelper {
private val migrationItems = mutableListOf<String>()
Copy link
Contributor

@malmstein malmstein Nov 3, 2025

Choose a reason for hiding this comment

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

@shakyShane what is being stored here? is it a blob?

@shakyShane
Copy link
Collaborator Author

shakyShane commented Nov 3, 2025

Sounds good, thanks!

joshliebe
joshliebe reviewed Nov 17, 2025
View reviewed changes
app/src/main/java/com/duckduckgo/app/browser/DuckDuckGoUrlDetector.kt
}

override fun isDuckAiUrl(url: String): Boolean {
return runCatching { AppUrl.Url.HOST_DUCKAI == url.toHttpUrl().topPrivateDomain() }.getOrElse { false }
Copy link
Contributor

@joshliebe joshliebe Nov 17, 2025

Choose a reason for hiding this comment

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

This shouldn’t be here, belongs in duck-chat module

joshliebe
joshliebe reviewed Nov 17, 2025
View reviewed changes
browser-api/src/main/java/com/duckduckgo/app/browser/DuckDuckGoUrlDetector.kt
* This method takes a [url] and returns `true` or `false`.
* @return `true` if the given [url] belongs to the duck.ai domain (apex or subdomain) and `false` otherwise.
*/
fun isDuckAiUrl(url: String): Boolean
Copy link
Contributor

@joshliebe joshliebe Nov 17, 2025

Choose a reason for hiding this comment

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

Shouldn’t be in the browser-api

joshliebe
joshliebe reviewed Nov 17, 2025
View reviewed changes
common/common-utils/src/main/java/com/duckduckgo/common/utils/AppUrl.kt

object Url {
const val HOST = "duckduckgo.com"
const val HOST_DUCKAI = "duck.ai"
Copy link
Contributor

@joshliebe joshliebe Nov 17, 2025

Choose a reason for hiding this comment

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

ditto

joshliebe
joshliebe reviewed Nov 17, 2025
View reviewed changes
duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/helper/DuckChatJSHelper.kt
}
}

/**
Copy link
Contributor

@joshliebe joshliebe Nov 17, 2025

Choose a reason for hiding this comment

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

These changes are susceptible to race conditions

Copy link
Collaborator Author

@shakyShane shakyShane Nov 17, 2025

Choose a reason for hiding this comment

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

there's at most 1 caller in the entire applications lifespan per message - that's by design. There's no situation in which 2 of these messages could be processed concurrently.

and if there was, it's easy to put behind a mutex? This is not like other FE features where things can be called at arbitrary times, it's forced to be in sequence

joshliebe
joshliebe reviewed Nov 17, 2025
View reviewed changes
duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/ui/DuckChatWebViewFragment.kt
val newWindowUrl = resultMsg?.data?.getString("url")
if (newWindowUrl != null) {
startActivity(browserNav.openInNewTab(requireContext(), newWindowUrl))
if (duckDuckGoUrlDetector.isDuckAiUrl(newWindowUrl)) {
Copy link
Contributor

@joshliebe joshliebe Nov 17, 2025

Choose a reason for hiding this comment

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

This logic should not be in the Fragment

joshliebe
joshliebe reviewed Nov 17, 2025
View reviewed changes
duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/RealDuckChat.kt
/**
* Returns whether standalone migration is supported.
*/
fun isStandaloneMigrationEnabled(): Boolean
Copy link
Contributor

@joshliebe joshliebe Nov 17, 2025

Choose a reason for hiding this comment

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

Needs tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshliebe joshliebe joshliebe left review comments

@malmstein malmstein malmstein requested changes

Requested changes must be addressed to merge this pull request.

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

@shakyShane @malmstein @joshliebe