Update devtools dependencies #891
Conversation
WalkthroughThe changes encompass updates to several files in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Sameri11
force-pushed
the
update-devtools
branch
from
5ab41b4 to
848dc02
Compare
|
@rrousselGit Hello! Hope you're doing well. I understand that this PR is quite extensive, and it might be challenging to review. Or, maybe, you already have your own plans for maintaining the extension, I completely respect that and will decline PR if this is the case. However, I would love to get your feedback on this work if you have the time. Please let me know if you have any questions or if there's anything specific you’d like me to clarify. No pressure at all — your insights would be greatly appreciated! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
.vscode/launch.json (1)
73-82: LGTM! Consider adding a comment for clarity.The new launch configuration for running Flutter tests in Chrome is well-structured and aligns perfectly with the PR objectives. It addresses the need to run tests in a browser environment due to the introduction of
dart:js_interop.Consider adding a comment above the configuration to explain its purpose and when to use it. For example:
+ // Configuration for running Flutter tests in Chrome. + // Use this when testing features that depend on dart:js_interop.This addition would help other developers understand the configuration's purpose and when to use it.
packages/provider_devtools_extension/test/test_utils.dart (1)
Line range hint
44-62: Consider updating thewrapfunction for better test isolationWhile not directly related to the current changes, the
wrapfunction could be enhanced to provide better test isolation. Currently, it uses aMaterialAppwith a specific theme, which might not be ideal for all test scenarios.Consider refactoring this function to accept optional parameters for theme and other
MaterialAppproperties. This would allow for more flexible and isolated testing of widgets under various conditions.Example refactor:
Widget wrap( Widget widget, { ThemeData? theme, TextDirection textDirection = TextDirection.ltr, }) { return MaterialApp( theme: theme ?? themeFor( isDarkTheme: false, ideTheme: IdeTheme(), theme: ThemeData( useMaterial3: true, colorScheme: lightColorScheme, ), ), home: Directionality( textDirection: textDirection, child: widget, ), ); }This change would make the
wrapfunction more versatile for different testing scenarios.packages/provider_devtools_extension/lib/src/instance_viewer/instance_providers.dart (1)
409-409: Approve the null-safety improvement with a minor suggestion.The change to use the null-safe access operator (
?.) and the null coalescing operator (??) is a good improvement. It prevents potential null pointer exceptions and aligns with modern Dart null safety practices.Consider adding a log statement when the URI is null, as this might indicate an unexpected state:
- final ownerUri = owner.location?.script?.uri ?? ''; + final ownerUri = owner.location?.script?.uri ?? ''; + if (owner.location?.script?.uri == null) { + print('Warning: owner.location.script.uri is null for owner: ${owner.name}'); + }This will help in debugging if we encounter unexpected null URIs in the future.
packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart (2)
13-14: LGTM! Consider organizing imports.The new imports for
devtools_extensionsare appropriate for the changes made in this file. HidingshortHashfromflutter_riverpodis a good practice to avoid potential naming conflicts.Consider organizing the imports alphabetically for better readability:
import 'package:devtools_app_shared/service.dart'; import 'package:devtools_app_shared/ui.dart'; import 'package:devtools_app_shared/utils.dart'; import 'package:devtools_extensions/api.dart'; import 'package:devtools_extensions/devtools_extensions.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; -import 'package:flutter_riverpod/flutter_riverpod.dart' hide shortHash; +import 'package:flutter_riverpod/flutter_riverpod.dart' hide shortHash; import 'instance_details.dart'; import 'instance_providers.dart';Also applies to: 18-18
123-134: Improved error handling and debugging. Consider error message formatting.The changes to the
_buildErrormethod significantly improve error handling and debugging capabilities. Using the stack trace and sending detailed error information to DevTools are excellent additions.Consider formatting the error message for better readability:
- return [Text('<unknown error>\n$stack')]; + return [Text('Unknown error:\n${error.toString()}\n\nStack trace:\n$stack')];This change would provide a clearer distinction between the error message and the stack trace.
packages/provider_devtools_extension/lib/src/provider_screen.dart (1)
Line range hint
1-139: Consider adding documentation for the new error handling and UI changes.The changes introduce significant improvements to error handling, reporting, and UI responsiveness. To help future maintainers understand these changes, consider adding inline documentation or updating the existing documentation to cover:
- The new error handling and reporting logic in
_hasErrorProvider.- The connection check logic and its importance for the simulated environment.
- The UI states and their respective rendering conditions.
This will make the code more maintainable and easier to understand for developers working on this extension in the future.
packages/provider_devtools_extension/test/provider_screen_test.dart (2)
32-33: Remove unnecessaryasynckeyword from thesetUpfunctionThe
setUpfunction is marked asasyncbut does not contain anyawaitstatements or asynchronous operations. Removing theasynckeyword can prevent potential confusion and align with best practices.-setUp(() async { +setUp(() { setGlobal(IdeTheme, getIdeTheme()); });
421-423: Correct the capitalization in the error messageThe error message
'Devtools are not connected to VmService'has inconsistent capitalization. For clarity and to match branding guidelines, consider changing'Devtools'to'DevTools'.Apply the following change:
expect((finder.found.first.widget as Text).data, - equals('Devtools are not connected to VmService')); + equals('DevTools are not connected to VmService'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/provider_devtools_extension/test/goldens/provider_screen/loading_state.pngis excluded by!**/*.pngpackages/provider_devtools_extension/test/goldens/provider_screen/no_selected_provider.pngis excluded by!**/*.pngpackages/provider_devtools_extension/test/goldens/provider_screen/selected_provider.pngis excluded by!**/*.png
📒 Files selected for processing (15)
- .vscode/launch.json (1 hunks)
- packages/provider/example/.metadata (1 hunks)
- packages/provider/extension/devtools/config.yaml (1 hunks)
- packages/provider_devtools_extension/.gitignore (1 hunks)
- packages/provider_devtools_extension/.metadata (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/eval.dart (0 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_providers.dart (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart (3 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/result.freezed.dart (15 hunks)
- packages/provider_devtools_extension/lib/src/provider_screen.dart (3 hunks)
- packages/provider_devtools_extension/pubspec.yaml (2 hunks)
- packages/provider_devtools_extension/test/provider_screen_test.dart (16 hunks)
- packages/provider_devtools_extension/test/test_utils.dart (1 hunks)
- packages/provider_devtools_extension/web/index.html (2 hunks)
- packages/provider_devtools_extension/web/manifest.json (1 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- packages/provider_devtools_extension/lib/src/instance_viewer/eval.dart
✅ Files skipped from review due to trivial changes (1)
- packages/provider_devtools_extension/web/manifest.json
🔇 Additional comments (30)
packages/provider/extension/devtools/config.yaml (1)
5-5: Approved: Addition ofrequiresConnectionfield enhances extension functionalityThe addition of
requiresConnection: trueis a positive change that aligns with the PR objectives and addresses the issues mentioned in #882. This modification allows the extension to independently monitor the connection state, which is crucial for:
- Adapting to the new requirement of functioning without an initially provided
VmService.- Improving compatibility with the
simulated_environment.- Addressing the deprecation that affected backward compatibility by shifting connection check responsibility to the plugin.
This change is a step towards resolving the buildability issues with the latest versions of Flutter and Dart.
packages/provider_devtools_extension/pubspec.yaml (5)
14-14: LGTM: Updated collection package versionThe
collectionpackage has been updated from^1.15.0to^1.18.0. This minor version bump should maintain backwards compatibility while providing access to newer features and bug fixes.
15-16: Significant version updates for devtools packagesThe
devtools_extensionsanddevtools_app_sharedpackages have both been updated to^0.2.2. This is a significant version bump from their previous0.0.xversions, which aligns with the PR objective of resolving buildability issues.Please ensure that these updated versions are compatible with your current implementation. You may want to check the changelogs of these packages for any breaking changes or new features that might affect your code.
17-17: Updated flutter_riverpod to stable versionThe
flutter_riverpoddependency has been updated from the pre-release version2.0.0-dev.9to the stable version^2.5.1. This is a positive change as it moves to a stable release.However, please note that this is a significant version jump. Ensure that you've accounted for any breaking changes or new features introduced between these versions. Review the package's changelog and update your code accordingly if necessary.
19-19: Updated vm_service to specific versionThe
vm_servicedependency has been updated from a range of versions">=11.9.0 <14.0.0"to a specific version^14.2.5. This change aligns with the PR objective of resolving issues related to thevm_servicedependency.This update might be related to the adaptation mentioned in the PR objectives about functioning without an initially provided
VmService. Please ensure that your implementation has been updated accordingly to work with this specific version ofvm_service. You may want to run tests specifically targeting theVmService-related functionality to verify compatibility.
29-29: Added mockito for testingThe addition of
mockitoas a development dependency is a positive change. Mockito is a powerful mocking framework that can help improve the quality and coverage of your tests.Consider leveraging Mockito to create comprehensive unit tests, particularly for components that interact with external services or have complex dependencies. This can help ensure the robustness of your code, especially given the significant changes in other dependencies.
packages/provider_devtools_extension/.gitignore (1)
46-47: Appropriate addition to .gitignoreThe new entry to ignore the
test/failuresdirectory is a good practice. It helps keep the repository clean by excluding failure screenshots generated by golden tests from version control.This change:
- Aligns with common version control best practices.
- Prevents unnecessary artifacts from cluttering the repository.
- Is well-documented with a clear comment explaining its purpose.
packages/provider_devtools_extension/.metadata (4)
7-8: LGTM: Version update aligns with PR objectives.The revision update and change to the "stable" channel are consistent with the goal of modernizing dependencies and resolving compatibility issues. This shift to a stable version should provide a more reliable foundation for the devtools extension.
16-17: LGTM: Consistent revision update for root platform.The revision updates for the root platform migration are consistent with the main version change. This ensures proper alignment for future migrations and maintains project consistency.
19-20: LGTM: Consistent revision update across platforms.The revision updates for the web platform migration are consistent with both the main version change and the root platform update. This comprehensive alignment ensures proper versioning across all aspects of the project, which is crucial for maintaining consistency and facilitating smooth migrations.
7-20: Summary: Metadata updates support project modernization.The changes to this
.metadatafile consistently update the project revision and channel across all sections. These updates are crucial for:
- Ensuring the project uses the latest stable Flutter version.
- Maintaining consistency across different platforms (root and web).
- Facilitating proper migration and dependency resolution.
These changes directly support the PR's objectives of modernizing dependencies and resolving compatibility issues with the latest versions of Flutter and Dart.
packages/provider_devtools_extension/web/index.html (3)
21-21: Fixed syntax error in description meta tagThe extra quotation marks in the description content have been removed, correcting the syntax error. This is a minor but necessary fix that improves the HTML validity.
Line range hint
1-38: Summary of changes to index.htmlThe modifications to this file, while minimal, contribute positively to the overall objectives of the PR:
- The syntax error fix in the meta description improves the HTML validity.
- The update to the Flutter application initialization method (adding
flutter_bootstrap.js) aligns with modern web development practices and potentially improves performance.These changes support the goal of modernizing the devtools extension and improving its compatibility with the latest versions of Flutter and Dart. However, to ensure full alignment with the PR objectives, it's crucial to verify that these changes work seamlessly with the new
vm_serviceand the updated VM service protocol, as mentioned in the linked issue #882.
36-36: Updated Flutter application initialization methodThe addition of the
flutter_bootstrap.jsscript with theasyncattribute indicates a shift towards a more modern and efficient way of initializing the Flutter application. This change aligns with current best practices for web performance.However, it's important to note that the AI summary mentions the removal of a previous script that handled the loading of the Flutter application. This removal is not visible in the provided code snippet.
To ensure that the old script has been properly removed and there are no conflicts, please run the following verification script:
This script will help ensure that the transition to the new initialization method is complete and consistent.
✅ Verification successful
Verification Successful: The old Flutter loading scripts have been successfully removed, and the new
flutter_bootstrap.jsscript with theasyncattribute is correctly added toindex.html. No issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of the old Flutter loading script and the presence of the new bootstrap script. # Test 1: Check for any remaining references to the old script echo "Checking for old script references:" rg --type html "flutter.js|main.dart.js" packages/provider_devtools_extension/web/ # Test 2: Confirm the presence of the new bootstrap script echo "Confirming new bootstrap script:" rg --type html "flutter_bootstrap.js" packages/provider_devtools_extension/web/index.htmlLength of output: 383
packages/provider_devtools_extension/test/test_utils.dart (1)
33-33: Excellent change to improve font loading!This modification enhances the
loadFontsfunction by usingrootBundle.load(path)instead of the previous file-based approach. This change brings several benefits:
- Improved platform compatibility, especially for web environments.
- Better alignment with Flutter's asset management system.
- Simplified code by eliminating direct file system operations.
- Increased consistency with Flutter's standard practices for resource handling.
This update contributes to the PR's objectives by improving the extension's compatibility and buildability across different platforms.
packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart (2)
74-74: LGTM! Improved list index generation.The change to use
PathToProperty.listIndexfor generating list indices is a good refactoring. It likely provides more specific and appropriate functionality for handling list indices in the context of the instance viewer.
Line range hint
1-600: Overall improvements to instance viewer functionality and error handlingThe changes in this file significantly enhance the instance viewer's functionality and align well with the PR objectives. Key improvements include:
- Better integration with DevTools through new imports and error reporting.
- Enhanced error handling with more detailed stack trace information.
- Refactored list index generation for improved specificity.
These modifications contribute to the extension's ability to function without an initially provided
VmServiceand improve its compatibility with thesimulated_environment. The changes also address part of the buildability issues mentioned in the linked issue #882.packages/provider_devtools_extension/lib/src/provider_screen.dart (5)
18-31: Improve error handling and reporting.The changes to the
_hasErrorProviderenhance error handling by:
- Returning an
AsyncError?instead of abool, providing more detailed error information.- Checking for an
AsyncErrorin bothsortedProviderNodesProviderandinstanceProvider.- Sending an error message with the error details to the
extensionManagerbefore displaying an error banner.These improvements allow for better error reporting and user feedback.
57-66: Verify error handling and reporting logic.The added error handling logic in the
_hasErrorProviderlistener is a good improvement. It sends an error message with the error details to theextensionManagerbefore displaying an error banner.To ensure this logic works as expected, consider adding tests that simulate an error in
_hasErrorProviderand verify that the expected error message is sent to theextensionManagerand the error banner is displayed.
67-70: Ensure the connection check logic is correct.The added connection check logic is necessary for the extension to work in the simulated environment. However, verify that this logic is correct and handles all possible connection states appropriately.
Consider adding tests to cover different connection scenarios and ensure the UI responds correctly in each case.
73-139: Verify the UI changes and error handling.The changes to the UI rendering logic in the
buildmethod include:
- Using
SplitPaneinstead ofSplitfor layout.- Adding a loading state and an error state when connecting to the
VmService.- Conditionally rendering the
InstanceViewerbased on the presence of aselectedProviderId.These changes improve the user experience by providing appropriate feedback based on the connection status and selected provider.
To ensure these changes work as expected, consider the following:
- Manually test the UI in different states (loading, error, with and without a selected provider) to verify the correct behavior.
- Add integration tests that simulate these different states and verify the expected UI is rendered.
49-49: Verify the impact of theSplitPanechange.The change from
SplittoSplitPanereflects an update to the UI component used for layout. Ensure that this change is consistent with the rest of the codebase and does not introduce any unintended visual or behavioral changes.Run the following script to check for other occurrences of
SplitandSplitPanein the codebase:✅ Verification successful
Impact of
SplitPaneChange Verified
- The update to
SplitPaneis consistent across the codebase.- No unintended visual or behavioral changes detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences of `Split` and `SplitPane` in the codebase. # Test 1: Search for `Split`. Expect: No occurrences (except in comments or strings). rg --type dart $'Split' --glob '!*provider_screen.dart' # Test 2: Search for `SplitPane`. Expect: Occurrences in relevant UI code. rg --type dart $'SplitPane'Length of output: 1012
packages/provider_devtools_extension/lib/src/instance_viewer/result.freezed.dart (8)
3-4: Generated Code Linting DirectivesThe linting directives are appropriate for generated code and help suppress warnings that are irrelevant to this file.
15-15: Clarify the Private Constructor Error MessageThe error message is clear and provides guidance for developers who might mistakenly use the private constructor. This helps prevent misuse of the class constructors.
80-98: Validate CopyWith Implementation for_$$ResultDataImplThe
copyWithimplementation for_$$ResultDataImplcorrectly allows for shallow copies of the object with modified fields. The usage offreezedin the null check ensures immutability.
Line range hint
114-151: Ensure Consistency in_$ResultDataImplClassThe implementation of
_$ResultDataImpl<T>follows the Freezed conventions for data classes. Thewhen,map, and other pattern matching methods are correctly overridden.
230-248: Validate CopyWith Implementation for_$$ResultErrorImplThe
copyWithimplementation for_$$ResultErrorImplcorrectly handles the immutability of error and stack trace properties. The null checks and casting are appropriately used.
Line range hint
266-310: Review Changes to_ResultErrorImplementationThe class
_$ResultErrorImpl<T>correctly encapsulates error information with the associated stack trace. The overrides forwhen,map, and other methods ensure proper functionality in pattern matching scenarios.
376-386: Confirm Factory Constructors and Getter MethodsThe factory constructor and getters in
_ResultError<T>are properly defined, ensuring that error instances can be created and accessed consistently.
Line range hint
15-386: Verify Compatibility with Updated Class NamesThe renaming of classes (e.g.,
_ResultData<T>to_$ResultDataImpl<T>) might affect parts of the codebase that reference these classes directly. Ensure that all references are updated accordingly.To check for usages of the old class names in the codebase, run:
✅ Verification successful
Verification Successful: All class name references are correctly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find references to old class names that may need updating. # Search for the old class names in Dart files. rg --type dart '_ResultData<' -A 2 rg --type dart '_ResultError<' -A 2Length of output: 8695
packages/provider_devtools_extension/test/provider_screen_test.dart
Outdated
Show resolved
Hide resolved
Sameri11
force-pushed
the
update-devtools
branch
from
848dc02 to
c4cee18
Compare
packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart (2)
13-14: LGTM: New constants added for fake freezed annotations.The addition of
nullable,freezed, anduseResultconstants is appropriate for mimicking freezed annotations.Consider adding a brief comment explaining the purpose of these constants, similar to the file-level comment. For example:
// Marker constants to mimic freezed annotations const nullable = Object(); const freezed = Object(); const useResult = Object();
33-48: LGTM: New EqualUnmodifiableListView class added with custom equality.The
EqualUnmodifiableListViewclass is a well-implemented extension ofUnmodifiableListViewthat provides custom equality comparison. The overrides for==andhashCodeare correctly implemented.Consider adding a brief class-level documentation comment explaining the purpose of this class. For example:
/// An [UnmodifiableListView] that compares equality based on its contents /// rather than identity. class EqualUnmodifiableListView<T> extends UnmodifiableListView<T> { // ... rest of the class implementation ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/provider_devtools_extension/test/goldens/provider_screen/loading_state.pngis excluded by!**/*.pngpackages/provider_devtools_extension/test/goldens/provider_screen/no_selected_provider.pngis excluded by!**/*.pngpackages/provider_devtools_extension/test/goldens/provider_screen/selected_provider.pngis excluded by!**/*.png
📒 Files selected for processing (15)
- .vscode/launch.json (1 hunks)
- packages/provider/extension/devtools/config.yaml (1 hunks)
- packages/provider_devtools_extension/.gitignore (1 hunks)
- packages/provider_devtools_extension/.metadata (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/eval.dart (0 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart (2 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_providers.dart (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart (3 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/result.freezed.dart (15 hunks)
- packages/provider_devtools_extension/lib/src/provider_screen.dart (3 hunks)
- packages/provider_devtools_extension/pubspec.yaml (2 hunks)
- packages/provider_devtools_extension/test/provider_screen_test.dart (16 hunks)
- packages/provider_devtools_extension/test/test_utils.dart (1 hunks)
- packages/provider_devtools_extension/web/index.html (2 hunks)
- packages/provider_devtools_extension/web/manifest.json (1 hunks)
💤 Files with no reviewable changes (1)
- packages/provider_devtools_extension/lib/src/instance_viewer/eval.dart
🚧 Files skipped from review as they are similar to previous changes (12)
- .vscode/launch.json
- packages/provider/extension/devtools/config.yaml
- packages/provider_devtools_extension/.gitignore
- packages/provider_devtools_extension/.metadata
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_providers.dart
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart
- packages/provider_devtools_extension/lib/src/provider_screen.dart
- packages/provider_devtools_extension/pubspec.yaml
- packages/provider_devtools_extension/test/provider_screen_test.dart
- packages/provider_devtools_extension/test/test_utils.dart
- packages/provider_devtools_extension/web/index.html
- packages/provider_devtools_extension/web/manifest.json
🔇 Additional comments (8)
packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart (2)
10-10: LGTM: Import statement added for the 'collection' package.The addition of this import is appropriate, as it's likely needed for the new
EqualUnmodifiableListViewclass introduced later in the file.
28-29: LGTM: JsonKey constructor updated with new optional parameters.The addition of
includeFromJsonandincludeToJsonparameters to theJsonKeyconstructor enhances its functionality while maintaining backward compatibility. This change likely reflects updates in the actual freezed package.packages/provider_devtools_extension/lib/src/instance_viewer/result.freezed.dart (6)
3-4: LGTM: Appropriate lint ignore rules addedThe added lint ignore rules are appropriate for generated files and help prevent unnecessary warnings.
27-28: Improved type safety inwhenOrNullandmapOrNullmethodsThe return types of
whenOrNullandmapOrNullmethods have been updated to be nullable. This change enhances type safety by allowing these methods to return null when no matching case is found, which is more accurate and prevents potential runtime errors.Also applies to: 46-47
80-84: Improved type safety and serialization handling inResultDataThe changes to the
ResultDataimplementation offer several improvements:
- The class name change to
_$ResultDataImplaligns with current Freezed naming conventions.- The
copyWithmethod now returns a more specific type, enhancing type safety.- The new
@JsonKeyannotation on thecopyWithmethod prevents it from being included in JSON serialization, which is appropriate ascopyWithis a utility method, not a data field.These changes improve the overall robustness and correctness of the code.
Also applies to: 114-115, 147-151, 224-225
230-234: Enhanced type safety and performance inResultErrorThe changes to the
ResultErrorimplementation bring several improvements:
- The class name change to
_$ResultErrorImplaligns with current Freezed naming conventions.- The
copyWithmethod now returns a more specific type, enhancing type safety.- The new
@JsonKeyannotation on thecopyWithmethod prevents it from being included in JSON serialization, which is appropriate for this utility method.- The
callmethod in thecopyWithimplementation now uses null checks instead offreezed, which may offer performance benefits.These changes improve the overall robustness, correctness, and potentially the performance of the code.
Also applies to: 266-268, 305-310, 385-386
134-138: Improved type safety in equality comparisonsThe equality comparisons have been updated to use more specific types (e.g.,
_$ResultDataImpl<T>instead of_ResultData<T>). This change aligns with the new class names and enhances type safety. The hash code calculations remain unchanged, maintaining consistent behavior.Also applies to: 290-296
Line range hint
1-387: Overall assessment: Improved type safety and code robustnessThe changes in this generated file reflect updates to the Freezed package and bring several improvements:
- Enhanced type safety through more specific return types and nullable return values where appropriate.
- Improved JSON serialization handling by excluding utility methods.
- Potential performance optimizations in the
copyWithimplementations.- Consistent naming conventions aligned with the latest Freezed package.
These changes contribute to a more robust and maintainable codebase. As this is a generated file, no further action is required from the developer.
Sameri11
force-pushed
the
update-devtools
branch
from
c4cee18 to
36e6338
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
packages/provider_devtools_extension/lib/src/instance_viewer/instance_details.dart (8)
9-9: Avoid unused imports.The import of
package:flutter/foundation.darton line 6 might be unnecessary since the code uses@freezedfromfreezed_annotation. Consider removing it if it's not used elsewhere in the file.
Line range hint
27-31: Typographical error inPathToPropertyfactory constructor.The comment mentions "private properties defined in different libraries," but the code does not account for the possibility of name clashes due to private properties starting with an underscore. Ensure that the implementation correctly handles private properties and avoids potential conflicts.
Line range hint
49-49: Consider makingObjectFieldan immutable class.Since you're using the Freezed package and the class represents data that shouldn't be altered after creation, consider adding
constconstructors or ensuring all fields are final to makeObjectFieldimmutable.
Line range hint
56-58: Potential null safety issue withname.startsWith('_').In the getter
isPrivate, ensure thatnameis not null before callingstartsWith('_'). Ifnamecan never be null, no action is needed. Otherwise, handle the potential null case.Apply this diff to handle potential null values:
- bool get isPrivate => name.startsWith('_'); + bool get isPrivate => name?.startsWith('_') ?? false;
Line range hint
61-63: Typographical inconsistency: Usenilinstead ofnill.The factory constructor
InstanceDetails.nilland related usages should be corrected tonilto adhere to conventional terminology.Apply this diff to correct the typo:
- factory InstanceDetails.nill({ + factory InstanceDetails.nil({Ensure you update all references to
nillthroughout the code.
Line range hint
93-105: Simplify theisExpandablegetter inInstanceDetails.The
isExpandablegetter can be simplified by returning a boolean directly without defining an additionalfalsyfunction.Apply this diff to simplify the code:
- bool get isExpandable { - bool falsy(Object _) => false; - - return map( - nill: falsy, - boolean: falsy, - number: falsy, - string: falsy, - enumeration: falsy, - map: (instance) => instance.keys.isNotEmpty, - list: (instance) => instance.length > 0, - object: (instance) => instance.fields.isNotEmpty, - ); - } + bool get isExpandable => map( + nil: (_) => false, + boolean: (_) => false, + number: (_) => false, + string: (_) => false, + enumeration: (_) => false, + map: (instance) => instance.keys.isNotEmpty, + list: (instance) => instance.length > 0, + object: (instance) => instance.fields.isNotEmpty, + );Note: This also includes the correction from
nilltonil.
Line range hint
132-133: Consider usingunmodifiablelists forpathToProperty.Since
pathToPropertyis exposed, it might be safer to return an unmodifiable list to prevent external mutation.Apply this change:
- @Default([]) List<PathToProperty> pathToProperty, + @Default([]) List<PathToProperty> pathToProperty, + // Consider wrapping with UnmodifiableListView
Line range hint
142-144: Optimize theparentgetter inInstancePath.The current implementation creates a new list by iterating up to
pathToProperty.length - 1. This can be simplified usingsublist.Apply this diff to simplify the code:
- return copyWith( - pathToProperty: [ - for (var i = 0; i + 1 < pathToProperty.length; i++) pathToProperty[i], - ], - ); + return copyWith( + pathToProperty: pathToProperty.sublist(0, pathToProperty.length - 1), + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart (0 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_details.dart (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/result.dart (1 hunks)
- packages/provider_devtools_extension/pubspec.yaml (2 hunks)
💤 Files with no reviewable changes (1)
- packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/provider_devtools_extension/pubspec.yaml
🔇 Additional comments (4)
packages/provider_devtools_extension/lib/src/instance_viewer/result.dart (2)
Line range hint
11-85: Verify code generation with the officialfreezedpackage.While the
Resultclass and other code remain unchanged, it's crucial to ensure that the code generation still works correctly with the officialfreezedpackage.Please run the code generation command (typically
flutter pub run build_runner build) and verify that the generated files (e.g.,result.freezed.dart) are created without errors. You can use the following script to check for the presence of the generated file:#!/bin/bash # Description: Verify the presence of the generated freezed file # Test: Check if the result.freezed.dart file exists fd -t f 'result.freezed.dart' packages/provider_devtools_extension/lib/src/instance_viewer/If the file is not found, please run the code generation command and commit the generated files.
9-9: Approve the update to use the officialfreezed_annotationpackage.The change from a local import to the official package import is a positive step towards modernizing the dependencies. This aligns with the PR objectives and should improve code reliability and maintainability.
To ensure consistency, please verify that the
freezedandfreezed_annotationpackages are correctly specified in thepubspec.yamlfile. Run the following script to check:✅ Verification successful
Verified
freezedandfreezed_annotationpackage versions inpubspec.yaml.The
freezedpackage is correctly specified as version^2.5.7andfreezed_annotationas^2.4.4, ensuring compatibility and enhancing code reliability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify freezed package versions in pubspec.yaml # Test: Check for freezed and freezed_annotation in pubspec.yaml rg --type yaml 'freezed(_annotation)?:' packages/provider_devtools_extension/pubspec.yamlLength of output: 257
Script:
#!/bin/bash # Description: Verify freezed package versions in pubspec.yaml # Test: Check for freezed and freezed_annotation in pubspec.yaml rg --type yaml 'freezed(_annotation)?:' packages/provider_devtools_extension/pubspec.yamlLength of output: 138
packages/provider_devtools_extension/lib/src/instance_viewer/instance_details.dart (2)
9-9: Ensurefreezed_annotationis correctly added to dependencies.Make sure that
freezed_annotationis added to yourpubspec.yamlunderdependenciesordev_dependenciesas appropriate, and thatbuild_runneris configured to generate the necessary code.Run the following script to check if
freezed_annotationis declared inpubspec.yaml:
Line range hint
108-119: Inconsistent handling ofinstanceRefIdfornilinstances.In the
instanceRefIdgetter, the case fornilinstances returnsnull. Ensure that downstream code accounts for this potentialnullvalue to avoidNullReferenceException.Check where
instanceRefIdis used to ensurenullis properly handled:
| @@ -406,7 +406,7 @@ Future<List<ObjectField>> _parseFields( | |||
| final owner = | |||
| await eval.safeGetClass(fieldDeclaration.owner! as ClassRef, isAlive); | |||
|
|
|||
| final ownerUri = fieldDeclaration.location!.script!.uri!; | |||
| final ownerUri = owner.location?.script?.uri ?? ''; | |||
There was a problem hiding this comment.
Yeah, great question, should've take notes for myself :( But, story is like this: fieldDeclaration (field.decl) is deprecated field, and I thought it's great idea to start getting rid of it's usages. Apart of this, while I was testing, it was always null. Plus, it's kinda strange to gather location of field itself here, and not owner – I understand that they should not differ in this case, but from semantics point view it seems odd for me.
But! I will reconsider this part anyway, will come back with proper explanation at least.
There was a problem hiding this comment.
First, IMO switching from !. to ?. ?? '' isn't right. '' isn't a valid URI and it's just going to silently fail later.
We should fail early.
We do care about where the field is located, not where the owner is located. An owner and a field may be in a different location (as a field may be inherited by a superclass or a mixin).
The logic here is trying to write code to read a field. As such, it is mandatory to run the read operation in the context of where the field is defined. This is for one core reason: The field may be private.
Ultimately, the variable name is likely wrong here. It should be fieldUri not ownerUri.
packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart
Outdated
Show resolved
Hide resolved
| extensionManager.postMessageToDevTools(DevToolsExtensionEvent( | ||
| DevToolsExtensionEventType.unknown, | ||
| data: {'stacktrace': stack.toString(), 'error': error.toString()})); | ||
| return [Text('<unknown error>\n$stack')]; |
There was a problem hiding this comment.
| return [Text('<unknown error>\n$stack')]; | |
| return [Text('<unknown error>\n$stack')]; |
There was a problem hiding this comment.
I don't like the idea of showing the Stack like that. That's a multiline message on a single-line UI
And if we're going to show the stack, we should show the error.
| if (ref.watch(sortedProviderNodesProvider) is AsyncError) return true; | ||
| final _hasErrorProvider = Provider.autoDispose<AsyncError?>((ref) { | ||
| if (ref.watch(sortedProviderNodesProvider) is AsyncError) { | ||
| return ref.read(sortedProviderNodesProvider) as AsyncError; |
There was a problem hiding this comment.
You shouldn't use ref.read here.
|
|
||
| final _hasErrorProvider = Provider.autoDispose<bool>((ref) { | ||
| if (ref.watch(sortedProviderNodesProvider) is AsyncError) return true; | ||
| final _hasErrorProvider = Provider.autoDispose<AsyncError?>((ref) { |
There was a problem hiding this comment.
The name of the provider feels odd now.
And we probably shouldn't be returning AsyncError itself, but just the error/stack
|
|
||
| final instance = ref.watch( | ||
| instanceProvider(InstancePath.fromProviderId(selectedProviderId)), | ||
| ); | ||
|
|
||
| return instance is AsyncError; | ||
| return instance is AsyncError ? instance as AsyncError : null; |
packages/provider_devtools_extension/lib/src/provider_screen.dart
Outdated
Show resolved
Hide resolved
packages/provider_devtools_extension/lib/src/provider_screen.dart
Outdated
Show resolved
Hide resolved
| ref.listen<AsyncError?>(_hasErrorProvider, (_, hasError) { | ||
| if (hasError != null) { |
There was a problem hiding this comment.
As I said above, the provider name makes no sense to me anymore. So the value needs to be renamed too.
We don't have an "hasError" value but the raw error itself.
| InstancePath __, | ||
| ) { | ||
| if (error is SentinelException) { | ||
| final valueAsString = error.sentinel.valueAsString; | ||
| if (valueAsString != null) return [Text(valueAsString)]; | ||
| } | ||
|
|
||
| return const [Text('<unknown error>')]; | ||
| extensionManager.postMessageToDevTools(DevToolsExtensionEvent( |
There was a problem hiding this comment.
We shouldn't post messages inside widgets.
| serviceManager.connectedState.addListener(() { | ||
| if (serviceManager.connectedState.value.connected) { | ||
| setServiceConnectionForProviderScreen(serviceManager.service!); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Listeners cannot be added inside widgets like this. You'd be causing a memory leak and accumulate more and more listeners over time.
| // since new option `requiresConnection` to config.yml | ||
| // not works in this config and it is needed to check connection | ||
| // inside plugin itself. | ||
| return ref.watch(serviceProvider).when( |
There was a problem hiding this comment.
Don't usewhen. Use switch with pattern matching
| error: (_, stacktrace) => Center( | ||
| child: Text( | ||
| 'Error when connecting to VmService: \n${stacktrace.toString()}'), | ||
| ), |
There was a problem hiding this comment.
I don't think there's value in showing that in the UI. It probably should be a snackbar or something.
| collection: ^1.18.0 | ||
| devtools_extensions: ^0.2.2 | ||
| devtools_app_shared: ^0.2.2 | ||
| flutter_riverpod: ^2.5.1 | ||
| logging: ^1.1.1 | ||
| vm_service: ">=11.9.0 <14.0.0" | ||
| vm_service: ^14.2.5 | ||
| freezed_annotation: ^2.4.4 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/provider_devtools_extension/lib/src/provider_screen.dart (2)
73-76: Informative comment added, but consider clarifying further.The added comment provides valuable context about why connection checking is now done inside the plugin. This is helpful for understanding the implementation.
To make the comment even more informative, consider expanding it slightly:
// This change is needed for the extension to work in a simulated environment. // The new option `requiresConnection` in config.yml doesn't work in this // configuration, so we need to check the connection inside the plugin itself. // TODO: Investigate why `requiresConnection` doesn't work and if it can be fixed.This expanded comment provides more context and suggests a future action to potentially resolve the underlying issue.
Line range hint
1-139: Overall improvements with some remaining issues to address.The changes in this file significantly enhance the provider screen's functionality, error handling, and UI responsiveness. These improvements align well with the PR objectives, particularly in adapting the extension to function without an initially provided
VmServiceand improving compatibility with thesimulated_environment.Key improvements:
- Enhanced error handling and reporting.
- Better state management using
ref.watch(serviceProvider).when().- Improved UI feedback for different states (error, loading, connected).
- Updates to use newer UI components like
SplitPane.Remaining issues to address:
- Potential memory leaks from adding listeners in the
buildmethod.- Some code style preferences, such as using
switchinstead ofwhen.- Minor refactoring suggestions for error handling and type casting.
To fully realize the benefits of these changes and ensure long-term maintainability:
- Implement the suggested
ConsumerStatefulWidgetapproach to resolve potential memory leaks.- Apply the recommended refactorings for error handling and type casting.
- Consider using
switchwith pattern matching as suggested for improved readability.- Ensure thorough testing of the new error handling and connection management features, particularly in simulated environments.
These final adjustments will solidify the improvements made and align the code more closely with best practices and reviewer suggestions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart (3 hunks)
- packages/provider_devtools_extension/lib/src/provider_screen.dart (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart
🔇 Additional comments (2)
packages/provider_devtools_extension/lib/src/provider_screen.dart (2)
5-5: LGTM: New import added.The addition of
import 'package:devtools_extensions/api.dart';is appropriate for the new functionality in this file.
16-16: LGTM: New import added.The addition of
import 'instance_viewer/eval.dart';is appropriate for the new functionality in this file.
packages/provider_devtools_extension/lib/src/provider_screen.dart
Outdated
Show resolved
Hide resolved
| serviceManager.connectedState.addListener(() { | ||
| if (serviceManager.connectedState.value.connected) { | ||
| setServiceConnectionForProviderScreen(serviceManager.service!); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Connection state management added, but potential memory leak.
The addition of a listener for serviceManager.connectedState is good for managing the connection state of the provider screen. However, adding listeners inside the build method can cause memory leaks, as mentioned in a previous review comment.
To resolve this, incorporate this listener into the ConsumerStatefulWidget approach suggested in the previous comment. Here's how you can modify the earlier suggestion:
class _ProviderScreenBodyState extends ConsumerState<ProviderScreenBody> {
@override
void initState() {
super.initState();
ref.listen<AsyncError?>(_hasErrorProvider, (_, hasError) {
// Your error handling logic here
});
serviceManager.connectedState.addListener(_handleConnectionChange);
}
@override
void dispose() {
serviceManager.connectedState.removeListener(_handleConnectionChange);
super.dispose();
}
void _handleConnectionChange() {
if (serviceManager.connectedState.value.connected) {
setServiceConnectionForProviderScreen(serviceManager.service!);
}
}
@override
Widget build(BuildContext context) {
// Your build method here
}
}This approach will maintain the connection state management functionality while preventing potential memory leaks.
| return ref.watch(serviceProvider).when( | ||
| data: (_) => SplitPane( | ||
| axis: splitAxis, | ||
| initialFractions: const [0.33, 0.67], | ||
| children: [ | ||
| const RoundedOutlinedBorder( | ||
| clip: true, | ||
| child: Column( | ||
| children: [ | ||
| AreaPaneHeader( | ||
| roundedTopBorder: false, | ||
| includeTopBorder: false, | ||
| title: Text('Providers'), | ||
| ), | ||
| Expanded( | ||
| child: ProviderList(), | ||
| ), | ||
| ], | ||
| ), | ||
| ], | ||
| ), | ||
| if (selectedProviderId != null) | ||
| Expanded( | ||
| child: InstanceViewer( | ||
| rootPath: InstancePath.fromProviderId(selectedProviderId), | ||
| showInternalProperties: ref.watch(_showInternals), | ||
| ), | ||
| RoundedOutlinedBorder( | ||
| clip: true, | ||
| child: Column( | ||
| children: [ | ||
| AreaPaneHeader( | ||
| roundedTopBorder: false, | ||
| includeTopBorder: false, | ||
| title: Text(detailsTitleText), | ||
| actions: [ | ||
| IconButton( | ||
| icon: const Icon(Icons.settings), | ||
| onPressed: () { | ||
| unawaited( | ||
| showDialog( | ||
| context: context, | ||
| builder: (_) => | ||
| _StateInspectorSettingsDialog(), | ||
| ), | ||
| ); | ||
| }, | ||
| ), | ||
| ], | ||
| ), | ||
| if (selectedProviderId != null) | ||
| Expanded( | ||
| child: InstanceViewer( | ||
| rootPath: | ||
| InstancePath.fromProviderId(selectedProviderId), | ||
| showInternalProperties: ref.watch(_showInternals), | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ], | ||
| ); | ||
| ], | ||
| ), | ||
| error: (_, stacktrace) => Center( | ||
| child: Text( | ||
| 'Error when connecting to VmService: \n${stacktrace.toString()}'), | ||
| ), | ||
| loading: () => const Center( | ||
| child: Text('Devtools are not connected to VmService'), | ||
| )); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved state management and UI, but consider using switch instead of when.
The changes in this section significantly improve state management and UI responsiveness:
- The use of
ref.watch(serviceProvider).when()provides clear handling for different states. - The UI now includes proper feedback for error and loading states.
- The change to
SplitPaneis consistent with earlier updates.
These improvements enhance the user experience and make the code more robust.
However, as suggested in a previous review, consider using switch with pattern matching instead of when. This can make the code more readable and maintainable. Here's an example of how you could refactor this:
@override
Widget build(BuildContext context, WidgetRef ref) {
final serviceState = ref.watch(serviceProvider);
return switch (serviceState) {
AsyncData(_) => SplitPane(
// ... (your existing SplitPane code)
),
AsyncError(error: final e, stackTrace: final st) => Center(
child: Text('Error when connecting to VmService: \n$st'),
),
_ => const Center(
child: Text('Devtools are not connected to VmService'),
),
};
}This refactoring maintains the improved state management while following the suggested use of switch with pattern matching.
| final _hasErrorProvider = Provider.autoDispose<AsyncError?>((ref) { | ||
| if (ref.watch(sortedProviderNodesProvider) is AsyncError) { | ||
| return ref.read(sortedProviderNodesProvider) as AsyncError; | ||
| } | ||
|
|
||
| final selectedProviderId = ref.watch(selectedProviderIdProvider); | ||
|
|
||
| if (selectedProviderId == null) return false; | ||
| if (selectedProviderId == null) return null; | ||
|
|
||
| final instance = ref.watch( | ||
| instanceProvider(InstancePath.fromProviderId(selectedProviderId)), | ||
| ); | ||
|
|
||
| return instance is AsyncError; | ||
| return instance is AsyncError ? instance as AsyncError : null; |
There was a problem hiding this comment.
Improvements in error handling, but some issues remain.
The changes to _hasErrorProvider improve error handling by providing more detailed error information. However, there are a couple of issues that need to be addressed:
-
On line 20,
ref.readis used instead ofref.watch. This was flagged in a previous review and should be changed toref.watchto ensure reactivity. -
On line 31, the cast to
AsyncErroris unnecessary and was also flagged in a previous review.
Please apply the following changes:
- if (ref.watch(sortedProviderNodesProvider) is AsyncError) {
- return ref.read(sortedProviderNodesProvider) as AsyncError;
+ final sortedProviders = ref.watch(sortedProviderNodesProvider);
+ if (sortedProviders is AsyncError) {
+ return sortedProviders;
}
// ... (other code remains the same)
- return instance is AsyncError ? instance as AsyncError : null;
+ return instance is AsyncError ? instance : null;These changes will address the issues while maintaining the improved error handling logic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final _hasErrorProvider = Provider.autoDispose<AsyncError?>((ref) { | |
| if (ref.watch(sortedProviderNodesProvider) is AsyncError) { | |
| return ref.read(sortedProviderNodesProvider) as AsyncError; | |
| } | |
| final selectedProviderId = ref.watch(selectedProviderIdProvider); | |
| if (selectedProviderId == null) return false; | |
| if (selectedProviderId == null) return null; | |
| final instance = ref.watch( | |
| instanceProvider(InstancePath.fromProviderId(selectedProviderId)), | |
| ); | |
| return instance is AsyncError; | |
| return instance is AsyncError ? instance as AsyncError : null; | |
| final _hasErrorProvider = Provider.autoDispose<AsyncError?>((ref) { | |
| final sortedProviders = ref.watch(sortedProviderNodesProvider); | |
| if (sortedProviders is AsyncError) { | |
| return sortedProviders; | |
| } | |
| final selectedProviderId = ref.watch(selectedProviderIdProvider); | |
| if (selectedProviderId == null) return null; | |
| final instance = ref.watch( | |
| instanceProvider(InstancePath.fromProviderId(selectedProviderId)), | |
| ); | |
| return instance is AsyncError ? instance : null; |
Sameri11
force-pushed
the
update-devtools
branch
from
46259e6 to
d843013
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/provider_devtools_extension/lib/src/instance_viewer/eval.dart (1)
17-36: Approve changes with a minor suggestion for improvementThe new implementation of
serviceProvideris a significant improvement. It aligns well with the PR objectives by adapting to function without an initially providedVmService. The changes make the provider more robust by only yielding the service when it's available and connected, and properly handling the lifecycle of the connection state listener.Consider handling the case where the service might become unavailable after being added to the stream. You could do this by adding
nullto the stream when the connection is lost:void handleConnectionChange() { final isConnected = serviceManager.connectedState.value.connected; final isServiceAvailable = serviceManager.isServiceAvailable; if (isConnected && isServiceAvailable) { controller.add(serviceManager.service!); + } else { + controller.add(null); } }This change would allow consumers of the stream to react to service disconnections more easily.
packages/provider_devtools_extension/lib/src/provider_screen.dart (3)
16-30: LGTM! Consider minor readability improvement.The new
hasConnectionProvideris well-implemented. It correctly manages the connection state and handles its lifecycle appropriately. Good job on usingref.onDisposeto clean up the listener.For improved readability, consider extracting the listener function:
final hasConnectionProvider = Provider.autoDispose<bool>((ref) { final currentValue = serviceManager.connectedState.value.connected; - listener() { - ref.state = serviceManager.connectedState.value.connected; - } + void listener() => ref.state = serviceManager.connectedState.value.connected; serviceManager.connectedState.addListener(listener); ref.onDispose(() { serviceManager.connectedState.removeListener(listener); }); return currentValue; });This minor change makes the code slightly more concise and easier to read.
Line range hint
61-137: LGTM! Consider adding error handling for disconnection.The changes to
ProviderScreenBodyare well-implemented:
- The use of
SplitPaneis consistent with the updates mentioned in the summary.- The connection state check improves the extension's functionality in simulated environments.
- The UI now provides clear feedback when DevTools are not connected to VmService.
To further improve user experience, consider adding error handling for unexpected disconnections:
@override Widget build(BuildContext context, WidgetRef ref) { final splitAxis = SplitPane.axisFor(context, 0.85); final hasConnection = ref.watch(hasConnectionProvider); + ref.listen<bool>(hasConnectionProvider, (_, hasConnection) { + if (!hasConnection) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('DevTools disconnected from VmService'), + ), + ); + } + }); // Rest of the method remains the sameThis addition will notify users if the connection is lost unexpectedly during a session.
Line range hint
140-152: LGTM! Consider adding more specific error information.The
showProviderErrorBannerfunction remains unchanged and continues to provide useful error information to users. However, to make it more helpful, consider modifying it to accept and display more specific error details:-void showProviderErrorBanner() { +void showProviderErrorBanner({String? specificError}) { extensionManager.showBannerMessage( key: 'provider_unknown_error', type: 'error', message: ''' DevTools failed to connect with package:provider. This could be caused by an older version of package:provider; please make sure that you are using version >=5.0.0. + +${specificError != null ? 'Specific error: $specificError' : ''}''', extensionName: 'provider', ); }This change would allow you to pass more detailed error information when available, making it easier for users to diagnose and resolve issues.
packages/provider_devtools_extension/lib/src/instance_viewer/instance_details.dart (2)
13-13: Address the TODO: Add tests for Map mutation behaviorThe TODO comment indicates that tests need to be added to verify that mutating a
Mapdoes not collapse previously expanded keys. Ensuring this behavior is important for maintaining UI consistency when users interact with mutable maps.Would you like assistance in generating these tests? I can help create unit tests to cover this scenario or open a new GitHub issue to track this task.
Line range hint
31-31: Correct the typo: Rename 'nill' to 'nil'The variant name
nillinInstanceDetails.nillmight be a typo. The conventional spelling isnil. Consider renaming it tonilfor clarity and consistency.Apply this diff to fix the typo:
-factory InstanceDetails.nill({ +factory InstanceDetails.nil({Also, update all occurrences of
nillthroughout the code to reflect this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/provider_devtools_extension/test/goldens/provider_screen/loading_state.pngis excluded by!**/*.pngpackages/provider_devtools_extension/test/goldens/provider_screen/no_selected_provider.pngis excluded by!**/*.pngpackages/provider_devtools_extension/test/goldens/provider_screen/selected_provider.pngis excluded by!**/*.png
📒 Files selected for processing (16)
- .vscode/launch.json (1 hunks)
- packages/provider/extension/devtools/config.yaml (1 hunks)
- packages/provider_devtools_extension/.gitignore (1 hunks)
- packages/provider_devtools_extension/.metadata (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/eval.dart (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart (0 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_details.dart (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart (3 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/result.dart (1 hunks)
- packages/provider_devtools_extension/lib/src/instance_viewer/result.freezed.dart (15 hunks)
- packages/provider_devtools_extension/lib/src/provider_screen.dart (3 hunks)
- packages/provider_devtools_extension/pubspec.yaml (2 hunks)
- packages/provider_devtools_extension/test/provider_screen_test.dart (16 hunks)
- packages/provider_devtools_extension/test/test_utils.dart (1 hunks)
- packages/provider_devtools_extension/web/index.html (2 hunks)
- packages/provider_devtools_extension/web/manifest.json (1 hunks)
💤 Files with no reviewable changes (1)
- packages/provider_devtools_extension/lib/src/instance_viewer/fake_freezed_annotation.dart
🚧 Files skipped from review as they are similar to previous changes (10)
- .vscode/launch.json
- packages/provider/extension/devtools/config.yaml
- packages/provider_devtools_extension/.gitignore
- packages/provider_devtools_extension/.metadata
- packages/provider_devtools_extension/lib/src/instance_viewer/instance_viewer.dart
- packages/provider_devtools_extension/lib/src/instance_viewer/result.dart
- packages/provider_devtools_extension/pubspec.yaml
- packages/provider_devtools_extension/test/test_utils.dart
- packages/provider_devtools_extension/web/index.html
- packages/provider_devtools_extension/web/manifest.json
🔇 Additional comments (15)
packages/provider_devtools_extension/lib/src/provider_screen.dart (1)
Line range hint
154-196: LGTM! Existing implementation is solid.The
_StateInspectorSettingsDialogclass remains unchanged and continues to provide a well-implemented settings dialog for the state inspector. It correctly usesConsumerWidgetfor state management and provides a clear UI for toggling the visibility of private properties.The existing implementation is solid and doesn't require any immediate changes.
packages/provider_devtools_extension/test/provider_screen_test.dart (6)
5-8: LGTM! Tests adapted for browser environmentThe changes indicate a transition to browser-based testing, which aligns with the PR objectives of modernizing the devtools extension. The addition of
dart:asyncandmockitoimports suggests improved handling of asynchronous operations and mocking in the tests.
15-17: LGTM! Imports align with PR objectivesThe new imports for DevTools extensions, shared utilities, and
vm_servicealign well with the PR objectives. These changes support the adaptation of the extension to work independently of an initially providedVmServiceand integrate better with the DevTools ecosystem.Also applies to: 21-21
32-33: LGTM! Improved test setup and widget structureThe changes to make
setUpasynchronous and wrapProviderScreenBodyinDevToolsExtensionimprove the test setup and align the widget structure with DevTools extension integration. These modifications support more robust testing and better represent the actual usage of the component.Also applies to: 42-42
94-94: LGTM! Enhanced test flexibility and alignment with PR objectivesThe updates to provider overrides significantly improve test flexibility:
- Switching from
overrideWithValuetooverrideWithallows for more dynamic behavior in tests.- Using functions to generate
ProviderNodelists enables testing of various scenarios more easily.- The addition of providers for connection status and instance details aligns well with the PR objective of independent connection state monitoring.
These changes should make the tests more robust and better suited to verify the new functionality introduced in the PR.
Also applies to: 107-110, 112-112, 121-121, 137-141, 144-144, 156-159, 173-176, 180-181, 189-192, 208-214, 229-232, 248-249, 252-252, 258-261, 264-264, 275-280
303-303: LGTM! Improved test reliabilityThe addition of
await tester.pumpAndSettle()ensures that all animations and microtasks are completed before proceeding with the test, which can prevent flaky tests. The new expectation forSplitPaneverifies the presence of a key UI component, enhancing the test's comprehensiveness.Also applies to: 307-307
410-444: LGTM! Enhanced test coverage for ServiceManager statesThe addition of the "ServiceManager states" test group significantly improves the test coverage:
- The "Loading state" test ensures that the correct message is displayed when DevTools are not connected to VmService.
- The "Data state" test verifies that the SplitPane is present when the connection is established.
These new tests align well with the PR objective of allowing the extension to independently monitor the connection state. They provide valuable validation of the component's behavior under different connection scenarios.
packages/provider_devtools_extension/lib/src/instance_viewer/instance_details.dart (1)
9-9: Verify that Freezed code generation is properly configuredEnsure that the
freezedcode generation is set up correctly and that the generated fileinstance_details.freezed.dartis present. Missing generated files can lead to compilation errors.Run the following script to check for the presence of the generated file:
✅ Verification successful
Freezed code generation is properly configured
The generated file
instance_details.freezed.dartis present.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'instance_details.freezed.dart' is generated. # Expected result: The file 'instance_details.freezed.dart' should be listed. fd 'instance_details.freezed.dart' packages/provider_devtools_extension/lib/Length of output: 167
packages/provider_devtools_extension/lib/src/instance_viewer/result.freezed.dart (7)
147-151: Verify the necessity of@JsonKeyannotations oncopyWithmethodsThe
copyWithmethod is annotated with@JsonKey(includeFromJson: false, includeToJson: false):@JsonKey(includeFromJson: false, includeToJson: false) @override @pragma('vm:prefer-inline') _$$ResultDataImplCopyWith<T, _$ResultDataImpl<T>> get copyWith => __$$ResultDataImplCopyWithImpl<T, _$ResultDataImpl<T>>(this, _$identity);Since methods are not serialized to or from JSON, these annotations may be unnecessary. Please verify if these annotations are intentional and whether they might have unintended side effects on the serialization process.
225-225: Duplicate: Verify the necessity of@JsonKeyannotations oncopyWithmethods**
307-310: Duplicate: Verify the necessity of@JsonKeyannotations oncopyWithmethods**
114-115: Updated class names reflect changes in the Freezed packageThe class
_ResultData<T>has been renamed to_$ResultDataImpl<T>:class _$ResultDataImpl<T> extends _ResultData<T> with DiagnosticableTreeMixin { _$ResultDataImpl(this.value) : super._();This change aligns with updates in the Freezed code generation. Ensure that all references to
_ResultData<T>are updated accordingly in the codebase to prevent any inconsistencies.
266-268: Updated class names reflect changes in the Freezed packageSimilarly,
_ResultError<T>has been renamed to_$ResultErrorImpl<T>:class _$ResultErrorImpl<T> extends _ResultError<T> with DiagnosticableTreeMixin { _$ResultErrorImpl(this.error, this.stackTrace) : super._();This change is consistent with the updates from the Freezed package. Please ensure that all references to
_ResultError<T>are updated throughout the codebase.
27-28: Nullable return types inwhenOrNullandmapOrNullmethodsThe
whenOrNullandmapOrNullmethods now return nullable types:TResult? whenOrNull<TResult extends Object?>({ TResult? Function(T value)? data, TResult? Function(Object error, StackTrace stackTrace)? error, }) => throw _privateConstructorUsedError;This update improves null safety and allows for more flexible handling of cases where a match may not exist. The implementation looks correct.
Also applies to: 46-47
3-4: Updated lint ignore directivesThe modifications to lint ignore directives are appropriate:
// ignore_for_file: type=lint // ignore_for_file: unused_element, deprecated_member_use, ...These changes suppress linter warnings that are not relevant for generated code.
|
Ok, I decided to cut the scope a little here and simplify couple things:
Also, tried to address all comments (regarding wrong usage of providers and leaks etc) – hence completely new commit. I admit that previous work was rather horrible and hope that this attempt is better. |
This PR is an attempt to adapt provider's devtools extension to modern version of dependencies, especially dependencies related to DevTools itself.
Main changes:
Extension is adapted to situation when
VmServiceis not provided from the very start. Back then, this was impossible situation since extension (any extension, not just subj) was not standalone and was aware of connection. Now, it's extension's responsibility to monitor connection state. Because of that, it was very hard (or maybe even impossible) to work withsimulated_environmentas described hereTests were broken as described in Wrap dev tools extension screen with SelectionArea #869 (comment). My investigation led me to fact that
devtools_extensionsand other dependencies now depend ondart:js_interopand since this lib is imported, tests no longer can run onvm(as per [Flutter 3.22.0] [dart:js_interop] - "ExternalDartReference" and "toExternalReference" usage causes "flutter test" to fail due to "Dart library 'dart:js_interop' is not available on this platform." flutter/flutter#148670 (comment)). Hence, my attempt to adapt to this in this PR. Tests are runnable now withflutter test --platform chromeor throughvscodeconfig, which is also added in PR.Overall, main purpose of changes is to allow and unblock further work on devtools extension.
Presumably fixes #882 and #869 (comment)
Summary by CodeRabbit
Release Notes
New Features
VmService, enhancing user feedback during connection attempts.freezed_annotationpackage, improving data handling for instance details.Bug Fixes
index.htmlandmanifest.jsonfiles.Dependencies
Tests