Skip to content

fix: web build #2164

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

fix: web build #2164

wants to merge 17 commits into from

Conversation

Rutik7066
Copy link

@Rutik7066 Rutik7066 commented Apr 7, 2025

Changes

  • Updated Flutter web loading strategy
  • Package Version fixes
  • Added createUniversalSocket helper functions for web and io compatibility.
  • Implemented OmiFile file wrapper abstractions for web and IO compatibility. It uses Index db under the hood on the web and IO file on native platforms.
  • Removed all dart:io usage.
  • Added conditional execution for Analytics and Support packages that do not or partially support web.
  • Updated file uploading to utilize the http package, and modified other client code to also use http in place of IO.

/claim #2008

@Rutik7066 Rutik7066 marked this pull request as ready for review April 8, 2025 09:11
Copy link
Collaborator

@skywinder skywinder left a comment

Choose a reason for hiding this comment

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

Please check comments, the PR should be clean for merge in prod.

Please also add demo video and instructions how to run it for local tests.

Comment on lines 48 to 49
url = "https://cors-anywhere.herokuapp.com/$url";
headers["Origin"] = 'http://localhost:53844';
Copy link
Collaborator

Choose a reason for hiding this comment

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

1/ Looks you forgot to remove hardcoded things here?

Comment on lines 114 to 115
clientId: "1031333818730-l9meebge7tpc0qmquvq6t5qpoe09o1ra.apps.googleusercontent.com",
).signIn();
Copy link
Collaborator

Choose a reason for hiding this comment

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

its already in app/setup/prebuilt/google-services.json - why do we need it here? this hardcode is not prod ready.

Comment on lines 44 to 50
# Soniox's 'multi'
if language in soniox_multi_languages:
return STTService.soniox, 'multi', 'stt-rt-preview'

## Deepgram's 'multi'
#if language in deepgram_multi_languages:
# return STTService.deepgram, 'multi', 'nova-3'
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it realtes to this PR?

Comment on lines 1 to 4
<!-- This file is auto-generated from docs/docs/docs/developer/sdk/python.mdx. Do not edit manually. -->
# 🎧 Omi Python SDK

This SDK connects to an **Omi wearable device** over **Bluetooth**, decodes the **Opus-encoded audio**, and transcribes it in **real time using Deepgram**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

how did you autogenerate this readme? I believe it's not relate to this PR as well. please check this others and below

…sistency

This commit replaces all direct `Platform` checks with the custom `PlatformUtil` utility to ensure consistent platform detection across the application. This change improves maintainability and reduces redundancy in platform-specific logic.
@Rutik7066
Copy link
Author

Please check comments, the PR should be clean for merge in prod.

Please also add demo video and instructions how to run it for local tests.

Sure, once I complete File handling then I will let you know and update the video demo as well

@Rutik7066
Copy link
Author

output.mp4

@skywinder Here is the working video of the PR
Here is how to test the PR

  1. Setup CORS proxy
npm i local-cors-proxy
lcp --proxyUrl=https://backend-dt5lrfkkoa-uc.a.run.app/ --credentials                                
  1. Change env
API_BASE_URL=http://localhost:8010/proxy/
  1. Run app
flutter run -d chrome

Note: Due to CORS, images do not fetch

@Rutik7066 Rutik7066 requested a review from skywinder April 13, 2025 21:04
Copy link
Collaborator

@skywinder skywinder left a comment

Choose a reason for hiding this comment

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

Overall, that’s a good next step. You made the change I asked for.

I left a few things to consider.

But there are quite a lot of other changes too, so we need to test it more and wait for a review from the people who work with the backend.

So bring attetion to @thinhx , since there is a lot of api modifications, which i'm not familiar yet.

@@ -110,7 +110,9 @@ Future<UserCredential?> signInWithGoogle() async {
try {
debugPrint('Signing in with Google');
// Trigger the authentication flow
final GoogleSignInAccount? googleUser = await GoogleSignIn(scopes: ['profile', 'email']).signIn();
final GoogleSignInAccount? googleUser = await GoogleSignIn(scopes: ['profile', 'email'],
// clientId: "1031333818730-l9meebge7tpc0qmquvq6t5qpoe09o1ra.apps.googleusercontent.com",
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this comment mean should it be in prod?

Comment on lines +148 to +152
// To reduce unnecessary invoking of getIdToken() on web
IdTokenResult? tokenResult = await FirebaseAuth.instance.currentUser?.getIdTokenResult();
SharedPreferencesUtil().authToken = await FirebaseAuth.instance.currentUser?.getIdToken()?? '';
SharedPreferencesUtil().tokenExpirationTime = tokenResult?.expirationTime?.millisecondsSinceEpoch ?? 0 ;

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this change in general...

But the code guru, @thinhx, will probably ask to separate this into another PR—to keep the web version PR clean and do this as a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

getIdToken() token doesn't work on web properly. Sometime it returns null. But surprisingly i found that it works perfect if it called is same function where we do sign in. So it is necessary for web.

Comment on lines 182 to 203
// try {
// final transcript = await transcribeVoiceMessage(audioFile);
// if (mounted) {
// setState(() {
// _transcript = transcript;
// _state = RecordingState.transcribeSuccess;
// _isProcessing = false;
// });
// if (transcript.isNotEmpty) {
// widget.onTranscriptReady(transcript);
// }
// }
// } catch (e) {
// debugPrint('Error processing recording: $e');
// if (mounted) {
// setState(() {
// _state = RecordingState.transcribeFailed;
// _isProcessing = false;
// });
// }
// AppSnackbar.showSnackbarError('Failed to transcribe audio');
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

for that code i'm not sure if it's needed or not for the further dev. so let @mdmohsin7 or @thinhx to decide on this.

Copy link
Author

Choose a reason for hiding this comment

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

I forgot to undo this 😅

@Rutik7066
Copy link
Author

@skywinder I apologize for the delayed updates; I was unwell. I have now made the requested changes.

@Rutik7066 Rutik7066 requested a review from skywinder April 16, 2025 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants