Implement audio player for audio file attachments in Chat #618

[DaemonWoo]

Resolves #618

Synopsis

Display audio files as a player with audio playback option, cover with integration test.

Solution

Implement a global AudioPlayer class to handle audio files playback, including seek, pause, and duration tracking.
Cover the functionality with tests.

Checklist

• Created PR:
  • In draft mode
  • Name contains issue reference
  • Has type and k:: labels applied
• Before review:
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted or updated
  • Draft mode is removed
• Review is completed and changes are approved
  • FCM (final commit message) is approved
• Before merge:
  • Milestone is set
  • PR's name and description are correct and up-to-date
  • All temporary labels are removed

[SleepySquash]

@DaemonWoo, I kindly ask you to look at the whole code of the task, take a look at every single file, and try to fix the issues beforehand, try to reproduces the bugs I've described and do something with the mock, please. I did some renaming, because ActiveAudioSession sounds a little bit wrong to me, I'm used to think of an "audio session" as a whole audio hardware configuration the app uses currently.

And I would like to quote one of the comments I've left, since it seems to be important, if you don't mind!

It seems like you may in a hurry in resolving the comments. Please, don't do that. It leads to sometimes minor, sometimes major difficulties that may be hard to catch.

And such things could also lead to the long reviews 🥲

[SleepySquash]

Why would you put the MockAudioWorker in both onAfterRun and onBeforeRun? This hook is invoked two times per test:

onBeforeRun
/* test 1 executes */
onAfterRun // <- First time
onBeforeRun // <- Second time
/* test 2 executes */
... etc

Shouldn't it be put only once per run? Like it does at appInitializationFn() function. Actually, I don't see a reason to use this hook at all, since appInitializationFn() already should be invoked for every app, which happens once per each test, isn't it?

[DaemonWoo]

At first, it was used once in appInitializationFn(). However, i started getting an error after each scenario saying that AudioWorker is not found. I agree that there's no need to call it everywhere, i think it should only be called in OnAfterRun.

[SleepySquash]

@DaemonWoo, "not found" is a problem. It shouldn't be fixed with adding the worker every time manually. What is the reason, why does every single other mock work perfectly without such an error and only the audio worker throws that error? Such behaviour shouldn't be ignored, perhaps there's an issue somewhere deeper and more dangerous than it seems?

[DaemonWoo]

This happens because of Get.deleteAll() in restart_app. Even if we call Get.put<AudioWorker>() again (similar to how we do it for GraphQL and GeoLocation providers), the error still occurs. The reason is that AudioPlayerView is not destroyed yet and continues trying to find AudioWorker.
If we make AudioWorker permanent in appInitializationFn, or reregister it in onAfterScenario, the error no longer occurs.

[SleepySquash]

@DaemonWoo,

This happens because of Get.deleteAll() in restart_app

That's exactly how it should be. When an app is restarted, the whole dependency tree is destroyed, isn't it?

The reason is that AudioPlayerView is not destroyed yet and continues trying to find AudioWorker.

That's a problem. Why? The view isn't displayed to anyone. It should be destroyed when an app is restarted. When you destroy the process OS runs the app in, and then launch a new process, this view is created again. And that's how this restart step should work ideally.

[DaemonWoo]

@SleepySquash I think GetX dependencies are destroyed first, while the view widget is still offstage and able to rebuild. And this error occurs during that brief moment before this view is created again

[DaemonWoo]

If making AudioWorker dependency permanent is bad, we can add And I return to previous page step at the end of each scenario. This way view will dispose cleanly, and only after that Get.deleteAll() would be triggered

[SleepySquash]

@DaemonWoo,

I think GetX dependencies are destroyed first, while the view widget is still offstage and able to rebuild

Then a view widget should get destroyed first. How can this be done? Perhaps the restart step doesn't do what it does? And it bothers me that it's the only mock that fails this way, other ones are working as they should.

I don't like an idea to fix the results of the problem and to ignore the problem itself. At least we should have an idea of what's going on in order to determine what to do.

[DaemonWoo]

@SleepySquash I was wrong about restart_app step, we don't use it here. I think this error occurs when Flutter might still be in the process of unmounting or rebuilding AudioPlayerView during cleanup/navigation transitions. And when it tries to rebuild this view, GetX dependencies have already been cleaned up. If we navigate away from chat before onAfterScenario the error is gone, because AudioPlayerView has time to be fully destroyed.

[SleepySquash]

When doing such steps, you may use a single step with different parameters. It reduces the code and makes it more strict.

Also you should ideally do a await context.world.appDriver.waitUntil(() async {}), because E2E tests run in real-time, unlike unit or widget tests, and a lot of delaying stuff can happen in a real-time environment. Sometimes a network connection may drop, and in order for something to happen a ~1-2 seconds delay might happen, etc. So we ideally should "wait until the status is what we expect for the status to be".

[SleepySquash]

Also, in Web version I get the following behaviour:

Screen.Recording.2026-03-27.at.13.27.25.mov

After a second drag for some reason the audio pauses.

[DaemonWoo]

In HomeView, I access Playback directly via public getter and expose it to UI, while in AudioPlayerView I interact with it only through the controller, since each method needs to verify that exactly this view is active. Is it okay to have two different approaches for interacting with playback?

[SleepySquash]

@DaemonWoo, what's your opinion? Do you think it's ok or not? And why?

[DaemonWoo]

@SleepySquash I think it does introduce some inconsistency, so it would be better to encapsulate playback in HomeController and expose what the view actually needs. This way, if we change the playback implementation, we won't need to modify HomeView

[SleepySquash]

@DaemonWoo, we'll still need to modify the HomeController in this case, I guess?

[DaemonWoo]

@SleepySquash Yes, we will

[SleepySquash]

@DaemonWoo, you shouldn't push a commit after requesting a review without a notice. You can't be sure that no one is working with this branch currently after you've sent it for a review, and checking the comments and rebasing can be cumbersome things to do. Please, either notify about a commit being made beforehand, or request a review only when you are absolutely sure about the code.