-
Notifications
You must be signed in to change notification settings - Fork 135
fix(simulator): syncing webview and native #456
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughDelegates WebView URL decision-making to a new async handler: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/ui/views/simulator/simulator_view.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
Pull Request Test Coverage Report for Build 20578349741Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/viewmodels/simulator/simulator_viewmodel.dart (1)
295-322: Implementation looks good with a minor suggestion for error handling.The navigation handler correctly implements the PR objective. The sign-out flow (lines 316-320) properly clears cookies before closing the view.
Consider adding error handling around the
clearCookies()call to ensure the view closes even if cookie clearing fails:🔎 Suggested error handling
} else if (url.contains('sign_out')) { - await clearCookies(); + try { + await clearCookies(); + } catch (e) { + debugPrint('Failed to clear cookies: $e'); + } Navigator.of(context).pop(); return 'cancel'; }Optional: The string literal
'cancel'is repeated multiple times. Consider extracting it to a constant for maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/ui/views/simulator/simulator_view.dartlib/viewmodels/simulator/simulator_viewmodel.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ui/views/simulator/simulator_view.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
🔇 Additional comments (1)
lib/viewmodels/simulator/simulator_viewmodel.dart (1)
291-293: Good improvement to the method signature.Changing the return type from
voidtoFuture<void>properly represents the async nature of this method and follows Dart best practices.
|
@JatsuAkaYashvant , please review |
|
@ShashwatXD Is there a reproducible case where this PR now works and earlier didn’t? Please attach the video. |
|
Earlier:
Record_2025-12-30-15-24-05.mp4
Record_2025-12-30-15-26-36.mp4Now, we have a better UX as user can handle things natively, can also come out of the simulator if he tries to sign out, i have also added a few more checks , if the user is already signed in to our app, and he tries to login in the simulator, it automatically fetches the token from app's preferences and lauches the simulator on his account.
Record_2025-12-30-15-21-46.mp4
VID_20251230152909.mp4 |
|
We need to better here, login should from the app interface, it should not depend on the web view to render web login page |
Their's a very small possibility for that to happen best i can do is disable the |
Fixes #447
Changes:
sign_outURL in theshouldOverrideUrlLoadingcallbackScreenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.