lightbox: Offer to open browser if video player doesn't support a video format

[chrisbobbe]

Fixes #1208.

Tested with a video in this message:
https://chat.zulip.org/#narrow/channel/7-test-here/topic/Chris2/near/2443349
using an iPhone 15 Pro simulator running iOS 17.5. I obtained that video by following a link in the discussion that raised this issue:
https://chat.zulip.org/#narrow/channel/48-mobile/topic/Unable.20to.20play.20.2Ewebm.20videos/near/2017541

[chrisbobbe]

@claude review once

[chrisbobbe]

@claude review once

[claude]

🔴 When the URL launch fails (e.g., no app handles it on Android, or a PlatformException is thrown), PlatformActions.launchUrl pushes its 'Unable to open link' error dialog without awaiting it. _offerOpenInBrowser then returns mounted (true), so _initialize calls Navigator.pop(context), which pops the just-pushed error dialog instead of the lightbox — leaving the user with no feedback. This violates the dartdoc invariant on _offerOpenInBrowser and mirrors the pattern already handled for ZulipAction.getFileTemporaryUrl a few lines above; fixing it likely needs PlatformActions.launchUrl to surface launch failure to its caller (e.g., return bool) so this branch can return false.

Extended reasoning...

What goes wrong

In _VideoLightboxPageState._offerOpenInBrowser (lib/widgets/lightbox.dart:558-559), the launch step is:

await PlatformActions.launchUrl(context, urlToLaunch);
return mounted;

However, PlatformActions.launchUrl (lib/widgets/actions.dart:440-462) handles its own failure mode by showing an error dialog without awaiting it:

if (!launched) { // TODO(log)
  if (!context.mounted) return;
  final zulipLocalizations = ZulipLocalizations.of(context);
  showErrorDialog(context: context, // not awaited!
    title: zulipLocalizations.errorCouldNotOpenLinkTitle,
    message: [...].join("\n\n"));
}

showErrorDialog calls showDialog<void>(...) which synchronously pushes a DialogRoute onto the navigator and returns the future of the dialog's lifetime. Because we don't await the DialogStatus.result, the await PlatformActions.launchUrl(...) resolves immediately after the dialog has been pushed.

Step-by-step proof

User opens an unsupported off-realm video; _initialize catches the VideoPlayerController.initialize failure and runs _offerOpenInBrowser. Suppose the video URL has no installed handler on Android, so ZulipBinding.instance.launchUrl(...) returns false (or throws PlatformException). Then:

1. Lightbox is the topmost route. await showSuggestedActionDialog(...).result resolves to true.
2. We reach await PlatformActions.launchUrl(context, urlToLaunch).
3. Inside launchUrl, launched == false, so it enters the if (!launched) branch and synchronously calls showErrorDialog(...). This pushes a DialogRoute (default useRootNavigator: true) onto the same root navigator that holds the lightbox (the lightbox uses AccountPageRouteBuilder → root navigator). Stack now: [lightbox, errorDialog]. launchUrl returns.
4. await PlatformActions.launchUrl(...) resolves; _offerOpenInBrowser evaluates return mounted; — the State object is still in the tree (the lightbox is just covered by the dialog), so this returns true.
5. Back in _initialize, mounted is checked, then Navigator.pop(context) runs. Navigator.pop pops the topmost route on the enclosing navigator: the error dialog. Stack now: [lightbox].
6. The user sees the "Unable to open link" dialog flash and disappear; the lightbox stays put with no error feedback at all.

Why existing code doesn't prevent this

The PR author was aware of this hazard — the dartdoc on _offerOpenInBrowser says:

Returns false when an error dialog has been shown by a call made here, to avoid the lightbox pop dismissing that dialog along the way.

And that contract is honored for the getFileTemporaryUrl failure path (lines 552-555: if (tempUrl == null) return false;, with a matching test at "on-realm upload: temp URL fetch fails"). It is not honored for the launchUrl failure path because launchUrl returns void and silently consumes its own failure. The return mounted; line cannot distinguish "launch succeeded" from "launch failed and an error dialog is now on top of us".

Impact

Narrow trigger but a real UX regression for that case: the user explicitly asked to "Open in browser", the app failed to do so, the app prepared an error message — and then the app immediately discards that error message and leaves them on the now-useless lightbox. The trigger is realistic on Android (no installed handler for the URL scheme, or PlatformException from the platform channel).

How to fix

The cleanest fix mirrors the getFileTemporaryUrl handling: have PlatformActions.launchUrl signal launch failure to its caller (e.g., change its return type to Future<bool>), and in _offerOpenInBrowser do something like:

final launched = await PlatformActions.launchUrl(context, urlToLaunch);
if (!mounted) return false;
if (!launched) return false; // error dialog was shown; keep lightbox
return true;

Alternatively, hoist the error-dialog logic out of launchUrl into the caller for this code path.

[chrisbobbe]

Revision pushed, fixing that bug.

[rajveermalviya]

Thanks @chrisbobbe! All LGTM, and this is testing correctly on my iPhone 11 (running iOS 26.x) with this .webm video: #test here > video test @ 💬.

Moving over to Greg's review.

[gnprice]

Thanks for taking care of this, and thanks @rajveermalviya for the review! Looks good; merging.