[google_maps_flutter] Refactor the JavaScript interaction and verify integration tests - #1066
Conversation
370807c to
39802fe
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the JavaScript interaction layer of the google_maps_flutter_tizen plugin by introducing a GoogleMapsJsBridge to replace direct WebViewController calls. The review feedback highlights a critical serialization issue in the bridge where arguments are directly interpolated, potentially causing runtime crashes and security vulnerabilities. To resolve this, the reviewer suggests introducing a JsExpression class to handle raw JS code and properly encoding other arguments. Additionally, the feedback recommends marking MapsJsEvent as sealed for exhaustiveness, utilizing the new bridge.addListener abstraction across controllers instead of executing raw JavaScript, and wrapping JS references in JsExpression to prevent them from being treated as string literals.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
seungsoo47
changed the title
JSUYA
left a comment
JSUYA
left a comment
There was a problem hiding this comment.
Please review gemini's review and modify the code.
I will update the review and the code after merging #1071. |
…dListener consistently Address Gemini Code Assist review on PR flutter-tizen#1066. - Fix a critical bug where callMethod/setProperty interpolated arguments via List.toString(), so a plain String like a marker title was spliced into the JS snippet unquoted (e.g. ['My Title'] instead of ["My Title"]), breaking at runtime for any string containing whitespace or special characters and risking JS injection. Add JsExpression to mark raw JS code that must not be quoted, and JSON-encode plain String arguments so they're safely treated as string literals. - Wrap the few call sites that relied on the old unquoted behavior (setMap's 'map' JS-global reference, setPosition's LatLng constructor, setPixelOffset's GSize object literal) in JsExpression so they keep working under the new serialization. - Mark MapsJsEvent sealed so the exhaustive switch in GoogleMapsController._onJsEvent is statically checked when new event types are added. - Use the bridge.addListener abstraction in polyline/circle/polygon/ ground_overlay instead of hand-built runJavaScript strings, for consistency with the rest of the bridge.
39802fe to
174d20a
Compare
… based on upstream v2.17.0
…dListener consistently Address Gemini Code Assist review on PR flutter-tizen#1066. - Fix a critical bug where callMethod/setProperty interpolated arguments via List.toString(), so a plain String like a marker title was spliced into the JS snippet unquoted (e.g. ['My Title'] instead of ["My Title"]), breaking at runtime for any string containing whitespace or special characters and risking JS injection. Add JsExpression to mark raw JS code that must not be quoted, and JSON-encode plain String arguments so they're safely treated as string literals. - Wrap the few call sites that relied on the old unquoted behavior (setMap's 'map' JS-global reference, setPosition's LatLng constructor, setPixelOffset's GSize object literal) in JsExpression so they keep working under the new serialization. - Mark MapsJsEvent sealed so the exhaustive switch in GoogleMapsController._onJsEvent is statically checked when new event types are added. - Use the bridge.addListener abstraction in polyline/circle/polygon/ ground_overlay instead of hand-built runJavaScript strings, for consistency with the rest of the bridge.
…JsEvent Follow-up to 8b9c3079: GInfoWindowOptions.toString() still spliced the info window content string directly into the JS snippet unquoted, the same unsafe-serialization issue fixed elsewhere in that commit. Also mark MessageJsEvent sealed, matching MapsJsEvent, so exhaustive switches over its subclasses stay statically checked.
174d20a to
c04ae3a
Compare
c04ae3a to
e8d9e43
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8d9e43a34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…we 0.5.3 webview_flutter_lwe 0.5.3 requires Dart ^3.8.0 and Flutter >=3.32.0, but this package still advertised Dart >=3.4.0 and Flutter >=3.22.0. Consumers on the lower range would fail dependency resolution since the only version satisfying webview_flutter_lwe ^0.5.3 needs the higher SDKs. Address chatgpt-codex-connector review on PR flutter-tizen#1066.
dispose() closed the _events StreamController but left the JavaScript channels registered, so a message from an in-flight JS timer or postMessage call arriving after disposal would call _events.add() on a closed controller and throw an uncaught StateError. Route all channel callbacks through a helper that ignores emissions once the controller is closed. Address chatgpt-codex-connector review on PR flutter-tizen#1066.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e36835317d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Raising the SDK constraint to ^3.8.0 activates the new tall-style dart format output, which the CI format check enforces.
_infoWindowOptionsFromMarker wrapped content in manual JS string quotes, but its consumers (GInfoWindowOptions.toString and GInfoWindow._setContent) already JSON-encode the value, causing literal apostrophes to render in the InfoWindow.
The subscription to _bridge.events was never retained, so a JS event queued just before dispose() could still invoke _onJsEvent afterward. Handlers like _onIdle() add to _streamController unguarded, which throws a StateError once the controller is closed.
| final WebViewController controller = WebViewController(); | ||
| /// The bridge mediating all interaction with the Google Maps JavaScript | ||
| /// API running inside the WebView. | ||
| final GoogleMapsJsBridge _bridge = WebViewGoogleMapsJsBridge(); |
There was a problem hiding this comment.
Replacing the public GoogleMapsController.controller field with _bridge makes existing 0.1.14 consumers that use this field fail to compile. Since this is currently a 0.1.15 patch release, please preserve compatibility with a forwarding getter such as WebViewController get controller => _bridge.controller;, or release it as 0.2.0.
| @@ -0,0 +1,464 @@ | |||
| // Copyright 2021 Samsung Electronics Co., Ltd. All rights reserved. | |||
| /// This is the single seam through which JS commands are built and JS→Dart | ||
| /// events are dispatched, replacing ad hoc `runJavaScript` calls and | ||
| /// hand-built JS strings scattered across the plugin. | ||
| abstract class GoogleMapsJsBridge { |
There was a problem hiding this comment.
GoogleMapsJsBridge has only one implementation, and GoogleMapsController constructs that implementation directly, so the interface currently provides neither substitutability.
How about collapse this into one concrete GoogleMapsJsBridge for now.
|
|
||
| /// Base class for events dispatched from the Google Maps JavaScript runtime | ||
| /// back into Dart through a [GoogleMapsJsBridge]. | ||
| sealed class MapsJsEvent { |
There was a problem hiding this comment.
These 15 event classes only carry an event kind and an optional String payload, with no type-specific behavior. A MapsJsEventType enum plus a ({MapsJsEventType type, String? message}) record preserves the same exhaustive switch without the class hierarchy. Please replace these subclasses and collapse the repetitive channel callbacks behind one registration helper.
| dependencies: | ||
| google_maps_flutter: ^2.16.0 | ||
| google_maps_flutter_tizen: ^0.1.14 | ||
| google_maps_flutter_tizen: ^0.1.15 |
There was a problem hiding this comment.
The interface has changed, 0.2.0 seems appropriate.
|
The integration_test was on standby for 14 hours. Please let me know if the CI is not working properly. |
Replace the sealed-class MapsJsEvent hierarchy and the GoogleMapsJsBridge/WebViewGoogleMapsJsBridge interface split with a single enum+record event type and one concrete bridge class, since there was only ever one implementation. Also fixes the copyright header year on the newly added bridge file and bumps the package to 0.2.0 since removing GoogleMapsController.controller is a breaking change.
2 participants
GoogleMapsJsBridge, replacing directWebViewControllercalls.webview_flutterto ^4.13.1 andwebview_flutter_lweto ^0.5.3.