fix: resolve unresponsive UI elements and Web Serial API issue by add…#3142
Closed
saad-aids wants to merge 2 commits into
Closed
fix: resolve unresponsive UI elements and Web Serial API issue by add…#3142saad-aids wants to merge 2 commits into
saad-aids wants to merge 2 commits into
Conversation
Contributor
Reviewer's GuideImplements explicit, user-gesture-driven USB connection on web, adds processing state handling and UI feedback, adjusts multimeter options callback to avoid null handlers, updates platform plugin registrants, and tweaks a localization string. Class diagram for BoardStateProvider and connection UI changesclassDiagram
class BoardStateProvider {
bool pslabIsConnected
int pslabVersion
int pslabFirmwareVersion
bool isProcessing
ValueNotifier legacyFirmwareNotifier
ConfigProvider configProvider
ScienceLabCommon scienceLabCommon
initialize()
initializeWiFi()
connectUSB() bool
}
class ConnectDeviceScreen {
}
class _HomeScreenState {
BuildContext context
_connectUSB(BoardStateProvider provider) void
build(BuildContext context) Widget
}
class MultimeterScreen {
}
class _MultimeterScreenState {
BuildContext context
_showOptionsMenu() void
_showInstrumentGuide() void
_toggleRecording() void
}
class ScienceLabCommon {
isConnected() bool
openDevice() bool
openWiFiDevice() bool
}
class ConfigProvider {
Config config
}
class Config {
bool autoStart
}
ConnectDeviceScreen --> _HomeScreenState : creates
MultimeterScreen --> _MultimeterScreenState : creates
_HomeScreenState --> BoardStateProvider : uses_via_Provider
_HomeScreenState --> ScienceLabCommon : uses_indirectly
BoardStateProvider --> ScienceLabCommon : manages_connection
BoardStateProvider --> ConfigProvider : reads_autoStart
ConfigProvider --> Config : provides
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
isProcessingflag inBoardStateProviderwas made public; consider keeping it private with a public getter/setter or dedicated methods so that only the provider controls its lifecycle and external code cannot accidentally desync state. - In
_MultimeterScreenStatethe newonOptionsPressedlogic seems inverted: it shows 'Start playback or connect device first' when!provider.isPlayingBack; please double-check the condition and the message to ensure it matches the intended UX. - All newly added user-facing strings (USB connect SnackBars, multimeter SnackBar, 'CONNECT USB' label) are hardcoded in English; consider wiring these through
AppLocalizationsto keep them translatable and consistent with the rest of the app.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `isProcessing` flag in `BoardStateProvider` was made public; consider keeping it private with a public getter/setter or dedicated methods so that only the provider controls its lifecycle and external code cannot accidentally desync state.
- In `_MultimeterScreenState` the new `onOptionsPressed` logic seems inverted: it shows 'Start playback or connect device first' when `!provider.isPlayingBack`; please double-check the condition and the message to ensure it matches the intended UX.
- All newly added user-facing strings (USB connect SnackBars, multimeter SnackBar, 'CONNECT USB' label) are hardcoded in English; consider wiring these through `AppLocalizations` to keep them translatable and consistent with the rest of the app.
## Individual Comments
### Comment 1
<location path="lib/providers/board_state_provider.dart" line_range="35-41" />
<code_context>
Future<void> initialize() async {
- if (_isProcessing) return;
- _isProcessing = true;
+ if (isProcessing) return;
+ isProcessing = true;
if (!scienceLabCommon.isConnected()) {
await scienceLabCommon.initialize();
- pslabIsConnected = await scienceLabCommon.openDevice();
- await setPSLabVersionIDs();
- await fetchFirmwareVersion();
+ if (!kIsWeb) {
+ pslabIsConnected = await scienceLabCommon.openDevice();
</code_context>
<issue_to_address>
**issue (bug_risk):** `isProcessing` is never reset to false if `attemptToConnectPSLab()` or `openDevice()` fail in the USB attach handler.
If `attemptToConnectPSLab()` returns `false` or any of `openDevice`, `setPSLabVersionIDs`, or `fetchFirmwareVersion` throws, `isProcessing` is never reset. This can leave the provider stuck in a processing state and block later connection attempts (including via the web button). Consider wrapping this logic in a `try`/`finally` to always reset `isProcessing` and notify listeners as needed.
</issue_to_address>
### Comment 2
<location path="lib/view/multimeter_screen.dart" line_range="290-299" />
<code_context>
key: const Key(multimeterScreenTitleKey),
- onOptionsPressed:
- provider.isPlayingBack ? null : _showOptionsMenu,
+ onOptionsPressed: () {
+ if (!provider.isPlayingBack) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(
+ content:
+ Text("Start playback or connect device first")),
+ );
+ return;
+ }
+
+ _showOptionsMenu();
+ },
onGuidePressed: _showInstrumentGuide,
</code_context>
<issue_to_address>
**question (bug_risk):** The enablement logic for the options menu seems inverted relative to the previous behavior and the snackbar message.
Previously, options were disabled while `isPlayingBack` was true, so they were only usable when *not* playing back. Now the snackbar blocks options when `!provider.isPlayingBack`, effectively reversing the behavior and conflicting with the existing UX. Also, the message mentions "connect device" but the condition only checks `isPlayingBack`. If you intend to allow options when either playback is active or a device is connected, consider encoding that combined condition explicitly and adjusting the message to match it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ing user-triggered USB connection (Fixes fossasia#3136)
saad-aids
force-pushed
the
fix-web-serial-ui-issue
branch
from
57647d9 to
4f24070
Compare
Author
|
Thanks for the review! 🙌
Please review again. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes unresponsive instrument UI actions and adjusts the Web USB/Serial connection flow to comply with browser user-gesture requirements, while updating generated platform plugin registrants and refreshing the PSLab description copy across locales.
Changes:
- Make Multimeter options always tappable, showing feedback instead of disabling the callback.
- Prevent automatic USB connection on Web; add an explicit user-triggered “CONNECT USB” flow with processing state + SnackBar feedback.
- Update generated platform plugin registration files and tweak
pslabDescriptioncopy in all localizations.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| windows/flutter/generated_plugins.cmake | Adds Windows plugin entries (incl. FFI plugin) in generated CMake list. |
| macos/Flutter/GeneratedPluginRegistrant.swift | Updates generated macOS plugin imports/registrations (but currently drops path_provider registration). |
| linux/flutter/generated_plugins.cmake | Updates Linux generated plugin list (switches audio plugin and adds FFI plugin). |
| linux/flutter/generated_plugin_registrant.cc | Updates Linux generated plugin registrant to register record_linux. |
| lib/view/multimeter_screen.dart | Ensures options button remains interactive; shows SnackBar when blocked by playback. |
| lib/view/connect_device_screen.dart | Adds Web-only CONNECT USB button and _connectUSB flow with UI progress state. |
| lib/providers/board_state_provider.dart | Adds connectUSB(), introduces processing state exposure, and skips auto-open on Web in initialize(). |
| lib/l10n/app_localizations.dart | Updates the documented English pslabDescription string. |
| lib/l10n/app_localizations_de.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_en.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_es.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_fr.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_he.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_hi.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_id.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_it.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_ja.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_ml.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_my.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_nb.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_pt.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_ru.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_te.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_uk.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_vi.dart | Updates pslabDescription copy. |
| lib/l10n/app_localizations_zh.dart | Updates pslabDescription copy. |
| ), | ||
| ) | ||
| : Text( | ||
| 'CONNECT USB', |
There was a problem hiding this comment.
Button label 'CONNECT USB' is hard-coded and not localized. Please add a localized string (similar to the WiFi button using appLocalizations.wifi.toUpperCase()) so this text is translated consistently.
Suggested change
| 'CONNECT USB', | |
| appLocalizations.usb.toUpperCase(), |
Comment on lines
23
to
26
| int pslabVersion = 0; | ||
| int pslabFirmwareVersion = 0; | ||
| bool _isProcessing = false; | ||
| bool isProcessing = false; | ||
|
|
There was a problem hiding this comment.
isProcessing was changed to a public mutable field. This makes it easy for external code to toggle it without calling notifyListeners, which can lead to stale UI state. Prefer keeping a private backing field with a read-only getter (and update it only via provider methods that always notify).
Comment on lines
34
to
96
| @@ -80,6 +96,28 @@ class BoardStateProvider extends ChangeNotifier { | |||
| }); | |||
There was a problem hiding this comment.
initialize() is called from MaterialApp.builder and from the main scaffold action (so it can be invoked many times). This method registers new UsbSerial/connectivity listeners on every call and never cancels previous subscriptions, which can cause duplicated event handling and leaks. Please ensure these listeners are registered only once (store StreamSubscriptions and guard/cancel on re-init).
Comment on lines
10
to
26
| import file_picker | ||
| import geolocator_apple | ||
| import package_info_plus | ||
| import path_provider_foundation | ||
| import record_macos | ||
| import share_plus | ||
| import shared_preferences_foundation | ||
| import url_launcher_macos | ||
| import wakelock_plus | ||
|
|
||
| func RegisterGeneratedPlugins(registry: FlutterPluginRegistry) { | ||
| ConnectivityPlusPlugin.register(with: registry.registrar(forPlugin: "ConnectivityPlusPlugin")) | ||
| DeviceInfoPlusMacosPlugin.register(with: registry.registrar(forPlugin: "DeviceInfoPlusMacosPlugin")) | ||
| FilePickerPlugin.register(with: registry.registrar(forPlugin: "FilePickerPlugin")) | ||
| GeolocatorPlugin.register(with: registry.registrar(forPlugin: "GeolocatorPlugin")) | ||
| FPPPackageInfoPlusPlugin.register(with: registry.registrar(forPlugin: "FPPPackageInfoPlusPlugin")) | ||
| PathProviderPlugin.register(with: registry.registrar(forPlugin: "PathProviderPlugin")) | ||
| RecordMacOsPlugin.register(with: registry.registrar(forPlugin: "RecordMacOsPlugin")) | ||
| SharePlusMacosPlugin.register(with: registry.registrar(forPlugin: "SharePlusMacosPlugin")) |
There was a problem hiding this comment.
GeneratedPluginRegistrant.swift no longer registers PathProviderPlugin, but pubspec.yaml still depends on path_provider and the app imports it (e.g., CsvService). This will likely break path_provider on macOS at runtime. Please re-run plugin generation / fix macOS plugin registration so path_provider_foundation is imported and PathProviderPlugin is registered.
Comment on lines
+292
to
+294
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text("Stop playback to use multimeter options"), |
There was a problem hiding this comment.
The SnackBar text is hard-coded and bypasses the app’s localization system. Please source this message from AppLocalizations (and consider hiding any existing SnackBar before showing a new one, as other parts of this screen do).
Suggested change
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text("Stop playback to use multimeter options"), | |
| ScaffoldMessenger.of(context).hideCurrentSnackBar(); | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text( | |
| appLocalizations.stopPlaybackToUseMultimeterOptions, | |
| ), |
Comment on lines
+92
to
+103
| if (success) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar(content: Text("Device connected successfully")), | ||
| ); | ||
| } else { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar(content: Text("Failed to connect device")), | ||
| ); | ||
| } | ||
| } catch (e) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar(content: Text("Error: $e")), |
There was a problem hiding this comment.
_connectUSB awaits provider.connectUSB() and then uses context (ScaffoldMessenger) without checking mounted; this can throw if the widget is disposed during the await. Align this with _connectWifi by adding a mounted check after the await, and consider routing messages through _showSnackBar and AppLocalizations instead of hard-coded strings / raw exception text.
Suggested change
| if (success) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar(content: Text("Device connected successfully")), | |
| ); | |
| } else { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar(content: Text("Failed to connect device")), | |
| ); | |
| } | |
| } catch (e) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar(content: Text("Error: $e")), | |
| if (!mounted) return; | |
| if (success) { | |
| _showSnackBar( | |
| "Device connected successfully", | |
| backgroundColor: Theme.of(context).colorScheme.primary, | |
| ); | |
| } else { | |
| _showSnackBar( | |
| "Failed to connect device", | |
| backgroundColor: Theme.of(context).colorScheme.error, | |
| ); | |
| } | |
| } catch (e) { | |
| if (!mounted) return; | |
| _showSnackBar( | |
| "Failed to connect device", | |
| backgroundColor: Theme.of(context).colorScheme.error, |
Member
|
Closing due to irrelevant changes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3136
Certain UI elements (Multimeter options, Dust Sensor, Gas Sensor) were unresponsive due to improper callback handling and Web Serial API restrictions.
Root Cause
Changes
Result
Checklist
Summary by Sourcery
Improve web USB connection flow and fix unresponsive multimeter options while updating platform plugins and copy text.
Bug Fixes:
Enhancements: