fix(wasm-ops): fix example app login by improving JS call error handling#185
fix(wasm-ops): fix example app login by improving JS call error handling#185
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces new JS interop utilities and error/result mappers, integrates them into KDF WASM operations for startup/stop flows, updates JSON type normalization and error messages, and adds unit tests for stop-result mapping. Legacy parsing in KDF operations is removed in favor of centralized helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant K as KdfOperationsWasm
participant JS as JS (mm2_main)
participant U as Interop Utils
C->>K: start()
K->>JS: mm2_main(...)
JS-->>K: value or Promise
K->>U: parseJsInteropMaybePromise<int>(jsValue)
U-->>K: int result
K-->>C: KdfStartupResult.fromDefaultInt(result)
sequenceDiagram
participant C as Caller
participant K as KdfOperationsWasm
participant JS as JS (mm2_stop)
participant U as Interop Utils
participant M as Result Mapper
C->>K: stop()
K->>JS: mm2_stop()
JS-->>K: value or Promise
K->>U: parseJsInteropMaybePromise(jsValue, mapper)
U->>M: mapJsStopResult(dartValue)
M-->>U: StopStatus
U-->>K: StopStatus
alt status is ok or stoppingAlready
loop until notRunning or timeout
K->>K: poll kdfMainStatus()
end
end
K-->>C: StopStatus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes JS interop error handling in the WASM operations to resolve login issues in the example app. The changes improve robustness when dealing with type conversions and error responses from JavaScript calls.
- Adds numeric type normalization for int/double conversion in WASM interop scenarios
- Introduces centralized JS error handling utilities and result mappers
- Refactors WASM operations to use new error handling patterns for better reliability
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| json_type_utils.dart | Adds numeric type normalization and minor formatting fixes |
| js_result_mappers_test.dart | New test file for JS result mapping functions |
| kdf_operations_wasm.dart | Major refactor to use centralized JS error handling utilities |
| js_result_mappers.dart | New utility for mapping JS stop result responses |
| js_interop_utils.dart | New comprehensive JS interop utilities for type conversion |
| js_error_utils.dart | New utilities for extracting error codes and messages from JS values |
|
Visit the preview URL for this PR (updated for commit ddfc4f4): https://komodo-defi-sdk--pr185-bugfix-wasm-js-error-qenfk0bo.web.app (expires Tue, 19 Aug 2025 12:19:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f9f5ac39928f333b6e8fcefb7138575e24ed347 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/komodo_defi_framework/lib/src/js/js_interop_utils.dart (1)
144-145: Potential TypeError risk from unsafe castThe direct cast
dartValue as Twithout validation could throw a TypeError at runtime if the actual type doesn't match the expected type T. Consider adding type checking or wrapping in a try-catch to provide more informative error messages.// Fallback: attempt a direct cast; this will surface a clear type error - return dartValue as T; + try { + return dartValue as T; + } catch (e) { + throw FormatException( + 'Failed to cast dartValue of type ${dartValue.runtimeType} to expected type $T', + ); + }
🧹 Nitpick comments (6)
packages/komodo_defi_types/lib/src/utils/json_type_utils.dart (2)
115-127: Improve error message clarityThe error message mentions "Expected a JSON string to parse" but the actual expectation is that the string should be valid JSON that decodes to a Map. Consider making the error message more specific.
- throw ArgumentError( - 'Expected a JSON string to parse, but got an invalid type: ' - '${value.runtimeType}', - ); + throw ArgumentError( + 'Failed to parse string as JsonMap. Expected valid JSON string, ' + 'but parsing failed for value of type: ${value.runtimeType}', + );
133-143: Inconsistent error message between JsonMap and JsonList parsingThe error message for JsonList parsing mentions "JSON string representing a List" but doesn't clarify that the parsing failed. Consider aligning with the JsonMap error message pattern.
throw ArgumentError( - 'Expected a JSON string representing a List, ' - 'but got an invalid type: ${value.runtimeType}', + 'Failed to parse string as JsonList. Expected valid JSON array string, ' + 'but parsing failed for value of type: ${value.runtimeType}', );packages/komodo_defi_framework/lib/src/js/js_error_utils.dart (1)
52-57: Consider extracting the heuristic patterns to constantsThe TODO comment suggests future generalization. Consider extracting the matching patterns as constants now to make future extensions easier.
+const List<String> _alreadyRunningPatterns = [ + 'already running', + 'already_running', +]; + // TODO: generalise to a log/string-based watcher for other KDF errors /// Heuristic matcher for common "already running" messages. bool messageIndicatesAlreadyRunning(String message) { final lower = message.toLowerCase(); - return lower.contains('already running') || lower.contains('already_running'); + return _alreadyRunningPatterns.any((pattern) => lower.contains(pattern)); }packages/komodo_defi_framework/lib/src/js/js_result_mappers.dart (1)
30-43: Consider adding logging for unexpected map structuresWhen processing Map inputs, the function silently defaults to
StopStatus.okfor unexpected structures. Consider adding debug logging to help diagnose integration issues.+import 'package:logging/logging.dart'; + +final _logger = Logger('JsResultMappers'); + StopStatus mapJsStopResult(dynamic result) { // ... existing code ... if (result is Map) { final map = result; if (map.containsKey('error') && map['error'] != null) { return StopStatus.errorStopping; } final inner = map['result']; if (inner is String) return mapJsStopResult(inner); if (inner is num) return StopStatus.fromDefaultInt(inner.toInt()); final code = map['code']; if (code is num) return StopStatus.fromDefaultInt(code.toInt()); + // Log unexpected map structure for debugging + _logger.fine('Unexpected map structure in stop result, defaulting to ok: $map'); return StopStatus.ok; } + _logger.fine('Unrecognized stop result type ${result.runtimeType}, defaulting to ok'); return StopStatus.ok; }packages/komodo_defi_framework/lib/src/operations/kdf_operations_wasm.dart (2)
118-120: Consider extracting debug logging to a helper method.The repeated pattern of checking
KdfLoggingConfig.debugLoggingbefore logging could be extracted into a debug-specific logging method to reduce code duplication.Add a debug logging helper method:
void _log(String message) => (_logger ?? print).call(message); + + void _debugLog(String message) { + if (KdfLoggingConfig.debugLogging) { + _log(message); + } + }Then replace the conditional logging calls throughout the file:
- if (KdfLoggingConfig.debugLogging) { - _log('Handling JSAny error: [${jsError.runtimeType}] $jsError'); - } + _debugLog('Handling JSAny error: [${jsError.runtimeType}] $jsError');
178-184: Consider simplifying the type checking logic.The
isInstancemethod has redundant type checking that could be simplified.The current implementation checks both
obj is TandinstanceOfString. Since JavaScript types don't directly map to Dart types viaisoperator, the first check might be redundant:bool isInstance<T extends js_interop.JSAny?>( js_interop.JSAny? obj, [ String? typeString, ]) { - return obj is T || - obj.instanceOfString(typeString ?? T.runtimeType.toString()); + return obj.instanceOfString(typeString ?? T.runtimeType.toString()); }However, if the
obj is Tcheck is intentionally kept for Dart-side type checking, consider adding a comment explaining this dual-checking strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/komodo_defi_framework/lib/src/js/js_error_utils.dart(1 hunks)packages/komodo_defi_framework/lib/src/js/js_interop_utils.dart(1 hunks)packages/komodo_defi_framework/lib/src/js/js_result_mappers.dart(1 hunks)packages/komodo_defi_framework/lib/src/operations/kdf_operations_wasm.dart(3 hunks)packages/komodo_defi_framework/test/js/js_result_mappers_test.dart(1 hunks)packages/komodo_defi_types/lib/src/utils/json_type_utils.dart(4 hunks)
🔇 Additional comments (10)
packages/komodo_defi_types/lib/src/utils/json_type_utils.dart (1)
158-165: Good addition for WASM interop numeric normalizationThe numeric type normalization between int and double is a good addition that will help prevent type mismatches when dealing with JavaScript numeric values in WASM contexts.
packages/komodo_defi_framework/test/js/js_result_mappers_test.dart (1)
5-36: Comprehensive test coverage for mapJsStopResultThe test suite provides good coverage of various input types and edge cases, including numeric codes, string responses with different cases, map structures with various keys, and null handling. The tests align well with the implementation's behavior.
packages/komodo_defi_framework/lib/src/js/js_result_mappers.dart (1)
3-11: Well-documented function with clear input expectationsThe documentation clearly describes the various input shapes and their expected mappings, which is helpful for maintainability.
packages/komodo_defi_framework/lib/src/operations/kdf_operations_wasm.dart (7)
10-12: LGTM! Good centralization of JS interop utilities.The extraction of JS error handling and result mapping into separate utility modules improves maintainability and reusability across the codebase.
102-114: LGTM! Clean refactoring of KDF main execution.The new implementation properly handles both Promise and direct value returns using the centralized
parseJsInteropMaybePromiseutility, simplifying the previous Future/dartify flow.
123-132: LGTM! Robust handling of JSNumber errors.The error handling properly extracts numeric codes from JSNumber types using the centralized utility functions.
138-146: LGTM! Comprehensive JSObject error extraction.The code properly attempts multiple extraction strategies: dartify first, then numeric code extraction, and finally message extraction with already-running detection.
199-226: LGTM! Well-structured stop operation with proper polling.The implementation correctly:
- Uses the centralized result mapper for stop status
- Implements polling with timeout to ensure the node actually stops
- Handles various error cases appropriately
The 10-second timeout with 300ms polling intervals provides a good balance between responsiveness and avoiding excessive polling.
233-234: LGTM! Clean replacement of legacy parsing with centralized utility.The use of
parseJsInteropJsonsimplifies the JSON parsing logic and removes the need for the complex_parseDartResponse,_deepConvertMap, and_deepConvertListmethods.
241-289: LGTM! Comprehensive error handling and debugging support.The
_makeJsCallmethod provides excellent error handling with detailed error messages and optional debug logging for troubleshooting.
Fixes the login error when running the example app via Wasm compilation (
flutter run -d chrome --wasm) by improving JS call error handling and type checking.Komodo wallet did not suffer the same issue, likely because it configures auth listeners after successful login (in
AuthBloc)Before (current
dev)Screen.Recording.2025-08-12.at.13.23.28.mov
Console log snippet
After
Screen.Recording.2025-08-12.at.13.26.03.mov
Console logs snippet
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests