android: move taildrop directory selector out of onboarding #669

kari-ts · 2025-06-14T00:01:13Z

android: move taildrop directory selector out of onboarding

-ShareFileHelper manages directory readiness; when a file is being shared to the device, it emits a signal to prompt the user to pick a directory
-Remove MDM auth key check; there is no longer any need to make assumptions about Taildrop usage, and we only show the directory selector when they are receiving a Taildropped file
-Listen for Taildrop receipt in application view model (formerly VpnViewModel, now renamed due to its expanded scope), since Taildrop can occur even without MainActivity, and move dir picker out of MainView
-Switch from StateFlow to SharedFlow since this is an event that only needs to be handled once rather than a persistent UI state.
-ShareFileHelper keeps track of Taildrop dir rather than the Taildrop extension managerOptions; this allows the correct directory to be used without having to send a new request or restart LocalBackend
-Don't restart LocalBackend on Taildrop dir selection because this is no longer necessary
Follow-up: implement resume Taildrop in SAF

Updates tailscale/corp#29211

review-ai-agent · 2025-06-14T00:01:16Z

Pull Request Revisions

Revision	Description
r3	Removed log and updated WindowInsets reference Code changes include removing a debug log statement in App.kt and updating a WindowInsets reference to use explicit companion object in MainView.kt
r2	Go toolchain revision updated Updated Go toolchain revision from `98e8c99c` to `1cd3bf1a` commit hash
r1	Refactored VpnViewModel to AppViewModel Replaced VpnViewModel with more comprehensive AppViewModel, adding Taildrop directory management and merging VPN-related state tracking

✅ AI review completed for r3

Help

React with emojis to give feedback on AI-generated reviews:

👍 means the feedback was helpful and actionable
👎 means the feedback was incorrect or unhelpful

💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at [email protected].

android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt

android/src/main/java/com/tailscale/ipn/App.kt

android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt

review-ai-agent · 2025-06-14T00:02:11Z

android/src/main/java/com/tailscale/ipn/MainActivity.kt

          }
        }

-    viewModel.setDirectoryPickerLauncher(directoryPickerLauncher)
+    appViewModel.setDirectoryPickerLauncher(directoryPickerLauncher)


In the MainActivity.kt (lines 221-224), you've moved the directory picker launcher assignment from viewModel to appViewModel, but I don't see corresponding changes in the AppViewModel class to handle this functionality. Make sure the method exists in the new class.

android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt

review-ai-agent · 2025-06-14T00:02:13Z

android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt

The generateNewFilename method in ShareFileHelper.kt currently uses UUID for generating unique filenames. Would using a more user-friendly approach like adding a simple incrementing number (e.g., 'file(1).txt') provide a better user experience for duplicate files?

review-ai-agent · 2025-06-14T00:05:25Z

libtailscale/backend.go

@@ -372,7 +372,7 @@ func (a *App) watchFileOpsChanges() {
 			a.directFileRoot = newPath
 			a.backendRestartCh <- struct{}{}
 		case helper := <-onShareFileHelper:
-			log.Printf("Got shareFIleHelper")
+			log.Printf("Got shareFileHelper")


In backend.go, there's a simple typo fix from 'shareFIleHelper' to 'shareFileHelper' which is good. Consider adding a simple comment before this log statement to describe the purpose of this branch in the select statement for better code maintainability.

android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt

review-ai-agent · 2025-06-14T00:05:26Z

android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt

In MainView.kt, the WindowInsets access has been changed from WindowInsets.statusBars to WindowInsets.Companion.statusBars. This appears to be a regression as the Companion access is typically needed for older Kotlin versions, whereas direct property access is the modern approach. Unless there's a specific compatibility issue, consider reverting to the direct property access.

barnstar

Needs a rebase and a quick check against the lastest main. I think I probably broke this with some of the temporary 1.84 patches.

-ShareFileHelper manages directory readiness; when a file is being shared to the device, it emits a signal to prompt the user to pick a directory -Remove MDM auth key check; there is no longer any need to make assumptions about Taildrop usage, and we only show the directory selector when they are receiving a Taildropped file -Listen for Taildrop receipt in application view model (formerly VpnViewModel, now renamed due to its expanded scope), since Taildrop can occur even without MainActivity, and move dir picker out of MainView -Switch from StateFlow to SharedFlow since this is an event that only needs to be handled once rather than a persistent UI state. -ShareFileHelper keeps track of Taildrop dir rather than the Taildrop extension managerOptions; this allows the correct directory to be used without having to send a new request or restart LocalBackend -Don't restart LocalBackend on Taildrop dir selection because this is no longer necessary Follow-up: implement resume Taildrop in SAF Updates tailscale/corp#29211 Signed-off-by: kari-ts <[email protected]>

nickkhyl · 2025-06-30T20:42:24Z

libtailscale/backend.go

@@ -337,8 +320,12 @@ func (a *App) newBackend(dataDir string, appCtx AppContext, store *stateStore,
 	}
 	lb, err := ipnlocal.NewLocalBackend(logf, logID.Public(), sys, 0)
 	if ext, ok := ipnlocal.GetExt[*taildrop.Extension](lb); ok {
+		defer func() {
+			if r := recover(); r != nil {
+				log.Printf("panic in taildrop extension init: %v", r)


Why do we need this? How do we know that the panic is in taildrop extension init?

nickkhyl · 2025-06-30T20:45:14Z

libtailscale/backend.go

 		}
 	}
 }

+func (a *App) WaitForTaildropReady() {
+	<-a.taildropReady


The channel is never signaled, so WaitForTaildropReady will never return. But it also looks like it's never called?

nickkhyl · 2025-06-30T20:58:58Z

libtailscale/interfaces.go

@@ -125,6 +125,8 @@ type Application interface {
 	// on every new ipn.Notify message. The returned NotificationManager
 	// allows the watcher to stop watching notifications.
 	WatchNotifications(mask int, cb NotificationCallback) NotificationManager
+
+	WaitForTaildropReady()


Can you please either remove it from the interface and implementation, or add a docstring if you plan to fully implement it in this PR?

nickkhyl · 2025-06-30T21:15:44Z

android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt

      context.contentResolver.openInputStream(partialUriObj)?.use { input ->
        context.contentResolver.openOutputStream(destFile.uri)?.use { output ->
          input.copyTo(output)
-        } ?: throw IOException("Unable to open output stream for URI: ${destFile.uri}")
-      } ?: throw IOException("Unable to open input stream for URI: $partialUri")
+        } ?: throw IOException("Unable to open output stream for URI: $finalTargetName")
+      } ?: throw IOException("Unable to open input stream for URI $partialUri")

      DocumentFile.fromSingleUri(context, partialUriObj)?.delete()


Maybe use DocumentFile.renameTo instead of copying the content and deleting the source file?

kari-ts requested a review from barnstar June 14, 2025 00:01

review-ai-agent bot reviewed Jun 14, 2025

View reviewed changes

kari-ts force-pushed the kari/movedirsel branch 2 times, most recently from da2e5c3 to d68b2b6 Compare June 14, 2025 00:04

review-ai-agent bot reviewed Jun 14, 2025

View reviewed changes

barnstar requested changes Jun 16, 2025

View reviewed changes

kari-ts force-pushed the kari/movedirsel branch 3 times, most recently from 136d472 to 620ad22 Compare June 18, 2025 23:30

kari-ts requested review from barnstar and nickkhyl June 18, 2025 23:31

kari-ts force-pushed the kari/movedirsel branch from 620ad22 to 0f5fbf0 Compare June 27, 2025 21:53

kari-ts force-pushed the kari/movedirsel branch from 0f5fbf0 to b3c6414 Compare June 27, 2025 22:08

nickkhyl reviewed Jun 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

android: move taildrop directory selector out of onboarding #669

android: move taildrop directory selector out of onboarding #669

kari-ts commented Jun 14, 2025 •

edited

Loading

Uh oh!

review-ai-agent bot commented Jun 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

review-ai-agent bot Jun 14, 2025

Uh oh!

Uh oh!

review-ai-agent bot Jun 14, 2025

Uh oh!

review-ai-agent bot Jun 14, 2025

Uh oh!

Uh oh!

review-ai-agent bot Jun 14, 2025

Uh oh!

barnstar left a comment

Uh oh!

nickkhyl Jun 30, 2025

Uh oh!

nickkhyl Jun 30, 2025

Uh oh!

nickkhyl Jun 30, 2025

Uh oh!

nickkhyl Jun 30, 2025

Uh oh!

Uh oh!

android: move taildrop directory selector out of onboarding #669

Are you sure you want to change the base?

android: move taildrop directory selector out of onboarding #669

Conversation

kari-ts commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

review-ai-agent bot commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Revisions

Uh oh!

Uh oh!

Uh oh!

Uh oh!

review-ai-agent bot Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

review-ai-agent bot Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

review-ai-agent bot Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

review-ai-agent bot Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

barnstar left a comment

Choose a reason for hiding this comment

Uh oh!

nickkhyl Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

nickkhyl Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

nickkhyl Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

nickkhyl Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kari-ts commented Jun 14, 2025 •

edited

Loading

review-ai-agent bot commented Jun 14, 2025 •

edited

Loading