Scaffold #55
Scaffold #55
Conversation
…eatures for improved code generation and test suggestions
… for improved clarity; remove unused utility files.
…asses for alignment, background, border, shadow, and insets
…ffoldLayout for enhanced layout management
…Gradient for enhanced color manipulation
…ping, and border functionality
…ensure proper functionality and default values
… constructors, methods, and properties
…alization and routing functionality
…tter organization
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive set of changes to the Oui framework, focusing on enhancing coding practices, restructuring the codebase, and modularizing components. Key updates include the introduction of coding principles and task instructions, modifications to exported entities, and the removal of deprecated components. New core modules for colors, geometry, routing, and component management have been added, promoting a more component-driven approach to UI development. Changes
Possibly related PRs
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
Documentation and Community
|
There was a problem hiding this comment.
Copilot reviewed 79 out of 97 changed files in this pull request and generated no comments.
Files not reviewed (18)
- .vscode/settings.json: Language not supported
- oui/lib/oui.dart: Language not supported
- oui/lib/src/actions/_index.dart: Language not supported
- oui/lib/src/actions/oui_action.dart: Language not supported
- oui/lib/src/app/_index.dart: Language not supported
- oui/lib/src/app/oui_app.dart: Language not supported
- oui/lib/src/app/oui_app_context.dart: Language not supported
- oui/lib/src/auth/_index.dart: Language not supported
- oui/lib/src/auth/oui_auth_provider.dart: Language not supported
- oui/lib/src/components/_index.dart: Language not supported
- oui/lib/src/components/aligner.dart: Language not supported
- oui/lib/src/components/box.dart: Language not supported
- oui/lib/src/components/label.dart: Language not supported
- oui/lib/src/components/oui_pressable.dart: Language not supported
- oui/lib/src/config/_index.dart: Language not supported
- oui/lib/src/config/config_container.dart: Language not supported
- oui/lib/src/config/oui_config.dart: Language not supported
- oui/lib/src/core/_index.dart: Language not supported
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (96)
oui/test/src/core/colors/palette/color_palette_test.dart (1)
1-1: 💡 Codebase verification
Remove unnecessary blank line at the start of the file
The blank line at the start of
color_palette_test.dartis inconsistent with the formatting pattern across all other test files in the codebase. Other test files either start with import statements directly or have blank lines after the imports, but none start with a blank line.🔗 Analysis chain
LGTM! Minor formatting change.
The addition of a blank line appears to be a minor formatting adjustment. Given the broader context of architectural refactoring, this change doesn't impact functionality.
Let's verify if this is consistent with the project's formatting guidelines:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Dart formatting guidelines in similar test files # Look for blank lines at the start of other test files to verify consistency echo "Checking other test files for similar formatting patterns..." fd -e dart -t f . test/src | while read -r file; do echo "=== $file ===" head -n 3 "$file" doneLength of output: 270
Script:
#!/bin/bash # First, check if the directory exists and then look for similar test files if [ -d "oui" ]; then echo "Checking test files in oui/test/src..." cd oui && fd -e dart -t f . test/src | while read -r file; do echo "=== $file ===" head -n 3 "$file" done else echo "Directory 'oui' not found. Searching in current directory..." fd -e dart -t f . | grep -i "test" | while read -r file; do echo "=== $file ===" head -n 3 "$file" done fiLength of output: 7160
oui/test/src/core/corners/corner_border_radius_test.dart (3)
8-24: 🧹 Nitpick (assertive)
Enhance test coverage with diverse test cases.
While the basic constructor test is good, consider adding test cases for:
- Different radius and smoothing values for each corner
- Edge cases (zero radius, zero smoothing)
- The
CornerBorderRadius.allconstructorExample addition:
test('handles different corner values', () { const borderRadius = CornerBorderRadius.only( topLeft: CornerRadius(radius: 10.0, smoothing: 0.5), topRight: CornerRadius(radius: 20.0, smoothing: 0.6), bottomLeft: CornerRadius(radius: 0.0, smoothing: 0.0), bottomRight: CornerRadius(radius: 15.0, smoothing: 1.0), ); expect(borderRadius.topLeft.radius, 10.0); expect(borderRadius.topRight.smoothing, 0.6); expect(borderRadius.bottomLeft.radius, 0.0); expect(borderRadius.bottomRight.smoothing, 1.0); });
1-51: 🧹 Nitpick (assertive)
Good alignment with architectural goals.
The test file structure and location in
core/cornersaligns well with the architectural refactoring objectives. The separation of corner-related functionality into a dedicated module promotes better organization and maintainability.Consider documenting the architectural decisions:
- Why corners are treated as a core concept
- The relationship between
CornerBorderRadius,CornerRadius, andCornerBorder- The module's role in the broader Oui framework
26-49: 💡 Codebase verification
🛠️ Refactor suggestion
Golden test implementation needs improvement
The verification confirms the review's concerns:
- No platform-specific golden files or handling exists
- Missing documentation for golden file maintenance
- Limited test coverage with single configuration
- Using bright green (0xFF00FF00) may not be ideal for visual testing
Suggested improvements:
/// To update golden files: /// 1. Run: flutter test --update-goldens /// 2. Verify the generated images in test/goldens/ /// /// Note: Golden tests might behave differently across platforms. /// Consider using platform-specific golden files if needed.Consider:
- Adding platform-specific golden files (e.g.,
corner_border_radius_linux.png)- Using a custom comparator for platform-specific handling
- Adding test cases with different corner configurations
- Using a more neutral color for better visual comparison
🔗 Analysis chain
Improve golden test reliability and documentation.
The golden test implementation could be enhanced in several ways:
- Add a comment explaining how to generate/update golden files
- Consider using a more neutral color for better visual testing
- Add platform-specific golden files to handle rendering differences
- Include additional test cases with different corner configurations
Let's verify if there are platform-specific golden files:
Also, consider adding this documentation above the test:
/// To update golden files: /// 1. Run: flutter test --update-goldens /// 2. Verify the generated images in test/goldens/ /// /// Note: Golden tests might behave differently across platforms. /// Consider using platform-specific golden files if needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for platform-specific golden files fd "corner_border_radius.*\.png" test/Length of output: 133
Script:
#!/bin/bash # First, find all test directories fd -t d "test" # Then, find all PNG files in the repository to locate golden files fd -e pngLength of output: 3029
Script:
#!/bin/bash # Check test file for platform-specific handling rg -B2 -A2 "matchesGoldenFile|goldenFileComparator" "oui/test/src/core/corners/corner_border_radius_test.dart"Length of output: 247
oui/test/src/core/corners/corner_path_test.dart (3)
23-176: 🧹 Nitpick (assertive)
Reduce code duplication by extracting common test setup.
Multiple test cases share similar setup code for creating
ProcessedCornerRadius. Consider extracting this into helper methods orsetUp.void main() { group('CornerExtensions on Path', () { ProcessedCornerRadius createProcessedRadius({ double radius = 8, double smoothing = 0.5, double width = 100, double height = 100, }) { return ProcessedCornerRadius( CornerRadius(radius: radius, smoothing: smoothing), width: width, height: height, ); } // Use in tests: // final processed = createProcessedRadius(); // final processed = createProcessedRadius(radius: 0); // etc. }); }
210-226: 🛠️ Refactor suggestion
Improve stress test to validate real-world scenarios.
The current stress test adds 100 corners at the same coordinates, which doesn't reflect real-world usage. Consider:
- Creating multiple paths with different corners
- Varying the rectangle dimensions and corner properties
- Reducing the iteration count to a reasonable number (e.g., 10)
test('addCorner stress test with many corners', () { for (int i = 0; i < 10; i++) { final path = Path(); final processed = ProcessedCornerRadius( CornerRadius(radius: 8.0 * (i + 1), smoothing: 0.5), width: 100.0 * (i + 1), height: 100.0 * (i + 1), ); for (final corner in Corner.values) { path.addCorner( corner, processed, Rect.fromLTWH(0, 0, 100.0 * (i + 1), 100.0 * (i + 1)), ); } // Add assertions here } });
7-242:
⚠️ Potential issueAdd meaningful assertions to validate path properties.
While the test cases cover various scenarios, they only assert that the path is not null. This provides minimal confidence in the correctness of the
addCornermethod. Each test should include assertions to validate:
- Path commands and their sequence
- Path coordinates and dimensions
- Corner properties (radius, smoothing)
- Path closure and continuity
Here's an example of how to enhance the first test case:
test('addCorner modifies path as expected', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 100, height: 100, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); - expect(path, isNotNull); - // Add more assertions to validate path commands + final pathMetrics = path.computeMetrics().single; + expect(pathMetrics.isClosed, isFalse); + expect(pathMetrics.length, closeTo(16.0, 0.1)); + + // Verify corner position + final cornerBounds = path.getBounds(); + expect(cornerBounds.right, equals(100.0)); + expect(cornerBounds.top, equals(0.0)); + + // Verify path contains the corner point + expect(path.contains(Offset(92.0, 8.0)), isTrue); });Would you like me to provide similar assertion examples for the other test cases?
📝 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.test('addCorner modifies path as expected', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 100, height: 100, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); final pathMetrics = path.computeMetrics().single; expect(pathMetrics.isClosed, isFalse); expect(pathMetrics.length, closeTo(16.0, 0.1)); // Verify corner position final cornerBounds = path.getBounds(); expect(cornerBounds.right, equals(100.0)); expect(cornerBounds.top, equals(0.0)); // Verify path contains the corner point expect(path.contains(Offset(92.0, 8.0)), isTrue); }); test('addCorner creates a closed path for all corners', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 100, height: 100, ); path.addCorner( Corner.topLeft, processed, const Rect.fromLTWH(0, 0, 100, 100), ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); path.addCorner( Corner.bottomRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); path.addCorner( Corner.bottomLeft, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner handles zero cornerRadius', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 0, smoothing: 0.5), width: 100, height: 100, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner handles extreme cornerRadius values', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 200, smoothing: 0.5), width: 100, height: 100, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner handles varying rectangle sizes', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 200, height: 200, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 200, 200), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner ensures path continuity for adjacent corners', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 100, height: 100, ); path.addCorner( Corner.topLeft, processed, const Rect.fromLTWH(0, 0, 100, 100), ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner ensures no gaps for large rectangle dimensions', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 1000, height: 1000, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 1000, 1000), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner produces mirrored results for opposite corners', () { final path1 = Path(); final path2 = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 100, height: 100, ); path1.addCorner( Corner.topLeft, processed, const Rect.fromLTWH(0, 0, 100, 100), ); path2.addCorner( Corner.bottomRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path1, isNotNull); expect(path2, isNotNull); // Add more assertions to validate path commands }); test('addCorner behaves identically for square rectangles', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 100, height: 100, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner handles varying smoothing values', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 1.0), width: 100, height: 100, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner handles zero cornerRadius with non-zero smoothing', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 0, smoothing: 1.0), width: 100, height: 100, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner stress test with many corners', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 8, smoothing: 0.5), width: 100, height: 100, ); for (int i = 0; i < 100; i++) { path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 100, 100), ); } expect(path, isNotNull); // Add more assertions to validate path commands }); test('addCorner benchmark for large dimensions', () { final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 1000, smoothing: 0.5), width: 10000, height: 10000, ); path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 10000, 10000), ); expect(path, isNotNull); // Add more assertions to validate path commands });oui/lib/src/core/corners.dart (5)
287-428: 🧹 Nitpick (assertive)
Add unit tests for complex path calculations
The
addCornermethod in theCornerExtensionsclass involves intricate mathematical computations for drawing custom corners. To ensure correctness across various scenarios, comprehensive unit tests are essential.Consider adding unit tests for
addCornerto validate its behavior with different corner radii and smoothing values.
1020-1045: 🛠️ Refactor suggestion
Return original decoration when no changes are applied
In the
CornerModifierclass, thedecoratemethod returnsnullwhen the provideddecorationis neither aShapeDecorationnor aBoxDecoration. This could lead to unexpectednullvalues and potential errors downstream. It's better to return the originaldecorationif no modifications are made.Apply this diff to ensure the original decoration is returned:
if (decoration is ShapeDecoration) { return decoration.copyWith( shape: shape, ); } if (decoration is BoxDecoration) { return ShapeDecoration( shape: shape, color: decoration.color, image: decoration.image, gradient: decoration.gradient, shadows: decoration.boxShadow, ); } - return null; + return decoration; }📝 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.Decoration? decorate( Decoration decoration, ComponentContext context, ) { final shape = corner.copyWith( radius: corner.borderRadius, ); if (decoration is ShapeDecoration) { return decoration.copyWith( shape: shape, ); } if (decoration is BoxDecoration) { return ShapeDecoration( shape: shape, color: decoration.color, image: decoration.image, gradient: decoration.gradient, shadows: decoration.boxShadow, ); } return decoration; }
475-492:
⚠️ Potential issueClamp smoothing values to valid range after arithmetic operations
Arithmetic operations in the
CornerRadiusclass can result insmoothingvalues outside the valid range of 0.0 to 1.0. This can cause assertions to fail in the constructor.Clamp the
smoothingvalue to ensure it stays within the valid range:@override CornerRadius operator +(Radius other) { double newSmoothing; if (other is CornerRadius) { newSmoothing = (smoothing + other.smoothing) / 2; } else { newSmoothing = smoothing; } + newSmoothing = newSmoothing.clamp(0.0, 1.0); return CornerRadius( radius: cornerRadius + other.x, smoothing: newSmoothing, ); }Committable suggestion skipped: line range outside the PR's diff.
910-925:
⚠️ Potential issueAdd assertion to prevent division by zero
In the
CornerBorderRadiusclass, the integer division operator (~/) does not check for division by zero, which could lead to runtime exceptions.Add an assertion to ensure
otheris not zero before performing the division:@override CornerBorderRadius operator ~/(double other) { + assert(other != 0, 'Operand must not be zero'); return CornerBorderRadius.only( topLeft: _topLeft ~/ other, topRight: _topRight ~/ other, bottomLeft: _bottomLeft ~/ other, bottomRight: _bottomRight ~/ other, ); }📝 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.topLeft: _topLeft / other, topRight: _topRight / other, bottomLeft: _bottomLeft / other, bottomRight: _bottomRight / other, ); } /// Integer divides the [CornerBorderRadius] by a scalar. @override CornerBorderRadius operator ~/(double other) { assert(other != 0, 'Operand must not be zero'); return CornerBorderRadius.only( topLeft: _topLeft ~/ other, topRight: _topRight ~/ other, bottomLeft: _bottomLeft ~/ other, bottomRight: _bottomRight ~/ other, );
432-573: 🧹 Nitpick (assertive)
Ensure consistent handling of smoothing in arithmetic operations
In the
CornerRadiusclass, when performing arithmetic operations with anotherRadius, thesmoothingproperty may not be handled consistently, especially whenotheris not aCornerRadius. This could lead to unintended smoothing values after operations.Consider updating the arithmetic operators to handle the
smoothingproperty appropriately:@override CornerRadius operator -(Radius other) { if (other is CornerRadius) { return CornerRadius( radius: cornerRadius - other.cornerRadius, smoothing: (smoothing + other.smoothing) / 2, ); } - return CornerRadius( - radius: cornerRadius - other.x, - smoothing: smoothing, - ); + // Adjust smoothing based on some logic or keep it unchanged + return CornerRadius( + radius: cornerRadius - other.x, + smoothing: smoothing, + ); }Similarly, review other operators like
+,*,/, and ensuresmoothingremains within the valid range [0.0, 1.0].Committable suggestion skipped: line range outside the PR's diff.
oui/test/src/core/corners/corner_clip_test.dart (1)
93-112: 🧹 Nitpick (assertive)
Assess performance impact of large child trees in tests
The test creates a widget tree with 1000
Containerwidgets. While this tests efficiency, it might introduce unnecessary load during testing.Consider reducing the number of widgets or mock the large tree to minimize resource usage during tests.
oui/test/src/core/corners/corner_border_test.dart (3)
10-47: 🧹 Nitpick (assertive)
Strengthen border alignment tests with path verification
The alignment tests only verify that paths are non-null but don't validate the actual path values or dimensions. Consider adding assertions to verify that the paths have the expected dimensions based on the border alignment.
Example enhancement:
final bounds = path.getBounds(); expect(bounds.width, equals(expectedWidth)); expect(bounds.height, equals(expectedHeight));
59-71: 🧹 Nitpick (assertive)
Improve paint method test coverage
The current test only verifies that no errors occur during painting. Consider:
- Testing with different border colors and thicknesses
- Verifying the painted output using golden image testing
- Testing with different TextDirection values
108-112: 🧹 Nitpick (assertive)
Enhance scale test verification
The current scale test only verifies the type of the result. Consider adding assertions to verify:
- The scaled border thickness
- The scaled corner radius values
- That the smoothing factor remains unchanged
oui/test/src/core/corners/corner_radius_test.dart (3)
35-35: 🧹 Nitpick (assertive)
Remove "Oui" prefix from test descriptions
The test descriptions contain "OuiCornerRadius" in comments while the class is named "CornerRadius". Update the comments to maintain consistency with the actual class name.
Also applies to: 43-43, 51-51, 58-58, 65-65, 72-72
35-77: 🧹 Nitpick (assertive)
Enhance arithmetic operation test coverage
Consider adding these test cases for arithmetic operations:
- Operations with zero radius but non-zero smoothing
- Operations that would result in values requiring rounding
- Edge cases where smoothing approaches 1.0
6-138: 🧹 Nitpick (assertive)
Add missing test scenarios
Consider adding tests for:
- Copy constructor or clone functionality
- Conversion to/from JSON if serialization is supported
- Factory constructors if any exist
- Behavior with double.infinity or double.nan values
oui/lib/src/core/metadata.dart (1)
37-40:
⚠️ Potential issueCorrect the assignment of
attributesinMetadata.alwaysconstructor.In the
Metadata.alwaysconstructor,attributesis of typeLocalizedMap<String, dynamic>, but it is being assignedLocalized.always(attributes), which returns aLocalized<Map<String, dynamic>>. This may lead to a type mismatch. UseLocalizedMap.always(attributes)instead.Apply this diff to fix the assignment:
tags = Localized.always(tags), - attributes = Localized.always(attributes); + attributes = LocalizedMap.always(attributes);📝 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.}) : name = Localized.always(name), icon = Localized.always(icon), tags = Localized.always(tags), attributes = LocalizedMap.always(attributes);oui/lib/src/components/box.dart (1)
68-75:
⚠️ Potential issueEnsure unknown modifiers are sorted correctly in
sortModifiersmethod.In the
sortModifiersmethod, modifiers not found in_modifierOrderare assigned an index of1. This may cause them to be incorrectly interleaved with known modifiers at index1. To ensure unknown modifiers are sorted after all known modifiers, consider assigning them an index greater than any in_modifierOrder.Apply this diff to fix the sorting logic:
modifiers.sort((a, b) { final aIndex = _modifierOrder.indexOf(a.runtimeType); final bIndex = _modifierOrder.indexOf(b.runtimeType); - return (aIndex == -1 ? 1 : aIndex).compareTo(bIndex == -1 ? 1 : bIndex); + final maxIndex = _modifierOrder.length; + return (aIndex == -1 ? maxIndex : aIndex).compareTo(bIndex == -1 ? maxIndex : bIndex); });📝 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.modifiers.sort((a, b) { final aIndex = _modifierOrder.indexOf(a.runtimeType); final bIndex = _modifierOrder.indexOf(b.runtimeType); final maxIndex = _modifierOrder.length; return (aIndex == -1 ? maxIndex : aIndex).compareTo(bIndex == -1 ? maxIndex : bIndex); }); return modifiers; }oui/lib/src/core/component.dart (2)
16-28: 🧹 Nitpick (assertive)
Refactor Suggestion: Remove redundant
boxColorsparameter inComponentContext.The
ComponentContextclass includes bothboxColorsand a gettercolorsthat returnsbuildContext.colors. Sincecolorscan be accessed directly frombuildContext, theboxColorsparameter may be redundant.Consider removing the
boxColorsfield and parameter to simplify the class:class ComponentContext { final Type componentType; final BuildContext buildContext; - final BoxColors boxColors; - BoxColors get colors => buildContext.colors; + BoxColors get colors => buildContext.colors; ComponentContext( this.componentType, this.buildContext, - this.boxColors, ); }Update the instantiation accordingly:
final componentCtx = ComponentContext( runtimeType, context, - context.colors, );📝 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.class ComponentContext { final Type componentType; final BuildContext buildContext; BoxColors get colors => buildContext.colors; ComponentContext( this.componentType, this.buildContext, ); }
81-93:
⚠️ Potential issueCritical Issue: Usage of undefined
SizeModifierinsortModifiersmethod.In the
sortModifiersmethod,SizeModifieris referenced, but it is neither defined nor imported in this file. This will lead to a compilation error due to the undefined identifier.To fix this issue, ensure that
SizeModifieris properly defined or imported. If it should be part of the modifier type order, import it from its respective module. If not needed, remove its references from the code.static const _defaultModifierTypeOrder = [ ChildProviderModifier, ChildModifier, DecorationModifier, + SizeModifier, // Ensure SizeModifier is included if it exists ]; - // Ensure SizeModifier is always last - if (a is SizeModifier) return 1; - if (b is SizeModifier) return -1;Committable suggestion skipped: line range outside the PR's diff.
oui/lib/src/core/app.dart (2)
119-122: 🧹 Nitpick (assertive)
Nitpick: Remove unused
themegetter in_DynamicAppContextProviderState.The
themegetter returnsDynamicTheme.lightbut is not used within_DynamicAppContextProviderState. To keep the code clean, consider removing it.class _DynamicAppContextProviderState extends widgets.State<DynamicAppContextProvider> { - DynamicTheme get theme => DynamicTheme.light; @override Widget build(BuildContext context) {📝 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.@override Widget build(BuildContext context) {
3-13:
⚠️ Potential issueCritical Issue: Missing imports for
Routerand related classes.Classes such as
Router,RouteInformationParser, andRouterDelegateare used but not imported, which will lead to compilation errors.Please import the necessary classes from the Flutter framework:
import 'package:flutter/widgets.dart' show + Router, + RouterDelegate, + RouteInformationParser, BuildContext, FlutterError, InheritedWidget, StatefulWidget, StatelessWidget, Widget, WidgetsApp;📝 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.import 'package:flutter/widgets.dart' show Router, RouterDelegate, RouteInformationParser, BuildContext, FlutterError, InheritedWidget, StatefulWidget, StatelessWidget, Widget, WidgetsApp; import 'package:flutter/widgets.dart' as widgets show State; import 'package:hooks_riverpod/hooks_riverpod.dart';oui/lib/src/core/background.dart (5)
62-63: 🧹 Nitpick (assertive)
Redundant Property Check
The
shouldRendergetter checks if any of the background properties are non-null. This might be redundant since the same check is performed in other methods. Ensure consistent usage.Consider refactoring to use
shouldRenderin methods where appropriate, or evaluate ifshouldRenderis necessary.
47-49: 🧹 Nitpick (assertive)
Typographical Error in Documentation
There appears to be a typo in the documentation for the
Background.gradientconstructor. The commentw tseems unintended and should be removed for clarity.Apply this diff to correct the documentation:
/// Creates an [Background] with a gradient. -/// w t /// /// [gradient] is the gradient to be used as the background.📝 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./// Creates an [Background] with a gradient. /// /// [gradient] is the gradient to be used as the background.
175-177:
⚠️ Potential issueUndefined Method or Property:
boxFitIn the
decorationImagegetter of theBackgroundImageclass, thefitproperty is accessed usingfit.boxFit. Sincefitis expected to be of typeBoxFit(after the previous correction), there is noboxFitproperty onBoxFit. Instead, usefitdirectly.Apply this diff to correct the usage of
fit:return DecorationImage( image: image, alignment: alignment.uiAlignment, repeat: repeat.imageRepeat, - fit: fit.boxFit, + fit: fit, opacity: opacity, scale: scale, );📝 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.alignment: alignment.uiAlignment, repeat: repeat.imageRepeat, fit: fit,
35-39: 🛠️ Refactor suggestion
Potential Null Reference in Private Constructor
In the private constructor
Background._, all parameters are optional, which could lead to aBackgroundinstance where all properties are null. This might cause unexpected behavior in methods that depend on at least one property being non-null.Consider adding an assertion to ensure that at least one background property is provided:
const Background._({ this.color, this.gradient, this.image, this.custom, +}) : assert( + color != null || gradient != null || image != null || custom != null, + 'Background requires at least one non-null property.', );📝 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.this.color, this.gradient, this.image, this.custom, }) : assert( color != null || gradient != null || image != null || custom != null, 'Background requires at least one non-null property.', );
96-97:
⚠️ Potential issueInconsistent Return Value in
modifyMethodThe
modifymethod returnsnullifcustom == null. This could lead to unexpected behavior if the method's return value is used without null checking.Update the return statement to return the
childwhencustomis null:if (custom == null) return child; return Stack( children: [ custom!, if (child != null) child, ], );Committable suggestion skipped: line range outside the PR's diff.
oui/lib/src/core/utils.dart (4)
117-152: 🧹 Nitpick (assertive)
Incomplete Implementation of
EnumContainerThe
EnumContainerclass lacks certain essential methods and properties for effective usage, such as iteration capabilities or proper error handling when accessing values.Consider implementing
Iterableor providing methods to access all key-value pairs, and ensure that error handling is robust when keys are missing.
270-289:
⚠️ Potential issueImproper Initialization in
SizedContainerIn the
SizedContainerclass, thekeysgetter is overridden to returnSizeLevel.values, but this may conflict with the keys provided to the superclassEnumContainer.Ensure that the
keysprovided in the superclass match those returned by thekeysgetter:class SizedContainer<T> extends EnumContainer<SizeLevel, T> { SizedContainer(super.values); SizedContainer.generate( T Function(SizeLevel) generator, + ) : super.generate( + generator, + SizeLevel.values, + ); @override - List<SizeLevel> get keys => SizeLevel.values; + List<SizeLevel> get keys => _values.keys.toList(); }Also, ensure that the superclass constructor properly initializes with the correct keys.
Committable suggestion skipped: line range outside the PR's diff.
186-197:
⚠️ Potential issuePossible Index Calculation Error in
LeveledContainer.getThe index calculation in the
getmethod may produce incorrect results if_levelStepis not 1 or iflevelis less than_firstLevel. Additionally, the method accesses_items.firsteven when the list might be empty.Consider refining the index calculation and adding checks for empty
_items:T get(int level) { + if (isEmpty) { + throw StateError('LeveledContainer is empty'); + } final index = ((level - _firstLevel) / _levelStep).floor(); if (index < 0) { return _items.first; } else if (index >= _items.length) { return _items.last; } - if (isEmpty) { - throw StateError('LeveledContainer is empty'); - } return _items.elementAt(index); }📝 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.T get(int level) { if (isEmpty) { throw StateError('LeveledContainer is empty'); } final index = ((level - _firstLevel) / _levelStep).floor(); if (index < 0) { return _items.first; } else if (index >= _items.length) { return _items.last; } return _items.elementAt(index); }
50-115:
⚠️ Potential issueGeneric Type Constraints Should Specify the Comparable Type
In the
Range<T extends Comparable>class,Comparableshould be specified with a type parameter to enforce type safety.Update the generic constraint to
T extends Comparable<T>:-class Range<T extends Comparable> { +class Range<T extends Comparable<T>> {📝 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.class Range<T extends Comparable<T>> { final T start; final T end; const Range( this.start, this.end, ); bool get isFixed => start == end; bool get isDynamic => !isFixed; bool get isUnbounded => start == double.negativeInfinity && end == double.infinity; bool within(T value) { return value.compareTo(start) >= 0 && value.compareTo(end) <= 0; } bool outside(T value) { return !within(value); } T clamp(T value) { if (value.compareTo(start) < 0) { return start; } else if (value.compareTo(end) > 0) { return end; } return value; } /// Checks if two ranges are equal. @override bool operator ==(Object other) { if (identical(this, other)) return true; if (other is! Range<T>) return false; return start == other.start && end == other.end; } /// Returns the hash code for the range. @override int get hashCode => start.hashCode ^ end.hashCode; /// Combines two ranges into a new range that spans from the minimum start to the maximum end. Range<T> operator +(Range<T> other) { final newStart = (start.compareTo(other.start) < 0) ? start : other.start; final newEnd = (end.compareTo(other.end) > 0) ? end : other.end; return Range(newStart, newEnd); } /// Subtracts one range from another, resulting in a new range. /// If the ranges do not overlap, returns the original range. Range<T>? operator -(Range<T> other) { if (other.end.compareTo(start) < 0 || other.start.compareTo(end) > 0) { return this; } final newStart = (start.compareTo(other.start) > 0) ? start : other.start; final newEnd = (end.compareTo(other.end) < 0) ? end : other.end; return (newStart.compareTo(newEnd) <= 0) ? Range(newStart, newEnd) : null; } @override String toString() { return 'Range{start: $start, end: $end}'; } }oui/lib/src/core/scaffold.dart (4)
230-247: 🧹 Nitpick (assertive)
Potential Performance Issue with Expansion
In the
_buildPanelsmethod, usingexpandandtoListmay have performance implications, especially with large numbers of panels.Consider using a
Listand adding elements directly for better performance:List<Widget> _buildPanels(BuildContext context, List<Screen> panels) { final widgets = <Widget>[]; for (var i = 0; i < panels.length; i++) { final screen = panels[i]; widgets.add( Flexible( flex: screen.width?.weight ?? 1, fit: FlexFit.loose, child: Padding( padding: const EdgeInsets.all(8.0), child: screen, ), ), ); if (i != panels.length - 1) { widgets.add( const Padding( padding: EdgeInsets.symmetric(vertical: 16.0), child: ScaffoldDivider(), ), ); } } return widgets; }
261-289: 🧹 Nitpick (assertive)
Complexity in the Stack Widget Hierarchy
The widget hierarchy inside the
Stackin_buildScaffoldis complex and may be difficult to maintain or understand.Consider refactoring the code to simplify the widget tree, perhaps by extracting parts into separate methods or widgets for clarity.
227-228:
⚠️ Potential issueMissing Null Check for
panelsParameterIn the
_buildPanelsmethod, there is no null check for thepanelsparameter. Ifpanelsis null, it could cause a runtime error.Add a null check or assert to ensure
panelsis not null:List<Widget> _buildPanels( BuildContext context, List<Screen> panels, ) { + assert(panels != null);Committable suggestion skipped: line range outside the PR's diff.
143-155:
⚠️ Potential issueInefficient Panel Width Calculation
In the
_buildLayoutmethod, thecanAddPanelfunction uses an iteratorpanels.iteratorand callspanels.iterator.moveNext(), which may not correctly check if the list is empty. This could lead to incorrect calculations when adding panels.Replace the iterator check with a simple list length check:
bool canAddPanel(Screen screen) { - if (!panels.iterator.moveNext()) { + if (panels.isEmpty) { return true; } final totalMinWidth = panels.fold<double>( 0, (previousValue, screen) => previousValue + (screen.width?.start ?? minPanelWidth), ); return totalMinWidth + (screen.width?.start ?? minPanelWidth) <= size.width.start; }📝 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.bool canAddPanel(Screen screen) { if (panels.isEmpty) { return true; } final totalMinWidth = panels.fold<double>( 0, (previousValue, screen) => previousValue + (screen.width?.start ?? minPanelWidth), ); return totalMinWidth + (screen.width?.start ?? minPanelWidth) <= size.width.start; }oui/lib/src/core/border.dart (1)
57-57:
⚠️ Potential issueEnsure null safety in
shouldRendergetterIn the
shouldRendergetter,coloris nullable. Comparing nullablecolordirectly may lead to unintended behavior. Update the condition to handlenullvalues explicitly.Apply this diff to fix the issue:
- bool get shouldRender => thickness > 0 && color != Color.clear; + bool get shouldRender => thickness > 0 && color != null && color != Color.clear;📝 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.bool get shouldRender => thickness > 0 && color != null && color != Color.clear;oui/lib/src/core/screen.dart (2)
107-118: 🧹 Nitpick (assertive)
Consider allowing customization of default modifiers
In the
Screen._private constructor, default modifiers are set. Ensure these defaults are appropriate for allScreeninstances or allow customization to enhance flexibility.Consider adding parameters to adjust the default modifiers or providing methods to modify them as needed.
279-281: 🧹 Nitpick (assertive)
Improve exception message for duplicate screen IDs
In the
addScreenfunction withinScreenRegistry, the exception message for duplicate screen IDs could be more descriptive to aid debugging.Apply this diff to enhance the exception message:
- throw Exception('Duplicate screen ID: ${screen.id}'); + throw Exception('Duplicate screen ID "${screen.id}" detected in the screen hierarchy.');📝 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.if (entries.any((entry) => entry.screen.id == screen.id)) { throw Exception('Duplicate screen ID "${screen.id}" detected in the screen hierarchy.'); }oui/lib/src/core/routing.dart (2)
178-179: 🧹 Nitpick (assertive)
Handle regular expression groups safely
In
_matchParametricSegment, when extracting thevalue, ensure that capturing groups are handled correctly to preventnullvalues when there are no groups.Consider modifying the code to default to
match.group(0)whenmatch.group(1)isnull.Apply this diff:
- final value = match.groupCount > 0 ? match.group(1) : match.group(0); + final value = match.group(1) ?? match.group(0) ?? '';📝 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 value = match.group(1) ?? match.group(0) ?? ''; matches.add(
75-76: 🧹 Nitpick (assertive)
Avoid use of assertions for input validation
The assertion checking
isMultiLinein the regular expression pattern may not be effective in production code. Consider throwing aFormatExceptioninstead to handle invalid patterns.Apply this diff to replace the assertion with an exception:
- assert( - RegExp(pattern).isMultiLine == false, - 'The pattern for an argument segment cannot be multiline.', - ); + if (RegExp(pattern).isMultiLine) { + throw FormatException( + 'The pattern for an argument segment cannot be multiline: $pattern', + ); + }Committable suggestion skipped: line range outside the PR's diff.
oui/lib/src/core/geometry.dart (5)
737-767: 🧹 Nitpick (assertive)
Add documentation for
RectangleSidesextension.The extension on
RectangleSideslacks documentation. Providing comments enhances code readability and helps maintain consistency.Consider adding documentation like:
/// Extension methods for `RectangleSides` to provide utility constants.
253-263: 🧹 Nitpick (assertive)
Consolidate redundant getters in
ModifiableAlignmentmixin.The getters
centerContentandcenteredboth returnalignment(Alignment.center). To simplify the code and avoid redundancy, consider keeping only one of them.Apply this diff to remove one of the redundant getters:
- Type get centerContent => alignment(Alignment.center); Type get centered => alignment(Alignment.center);📝 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.mixin ModifiableAlignment<Type extends Component> on Component<Type> { Type get centered => alignment(Alignment.center); Type alignment(Alignment alignment) { return withModifier( AlignmentModifier(alignment), ); } }
417-419:
⚠️ Potential issueEnsure the
modifymethod returns the child when insets are zero.In the
InsetModifier, when the insets have zero values, themodifymethod returnsnull, which may lead to the absence of the widget in the widget tree. If the insets are zero, it should return the unmodifiedchildinstead ofnull.Apply this diff to fix the issue:
- if (!insets.shouldRender || child == null) return null; + if (child == null) return null; + if (!insets.shouldRender) return child;📝 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.Widget? modify(Widget? child, ComponentContext context) { if (child == null) return null; if (!insets.shouldRender) return child;
335-335: 🧹 Nitpick (assertive)
Prefer using
isNotEmptyover checking individual properties.In the
Insetsclass, you can simplify theshouldRendergetter by introducing anisNotEmptymethod or by comparing againstEdgeInsets.zerofor clarity.Apply this diff to simplify the check:
- bool get shouldRender => top > 0 || right > 0 || bottom > 0 || left > 0; + bool get isNotEmpty => top != 0 || right != 0 || bottom != 0 || left != 0;Update the usage in the
InsetModifieraccordingly.Committable suggestion skipped: line range outside the PR's diff.
1-12: 🧹 Nitpick (assertive)
Organize imports for clarity and efficiency.
Consider grouping and ordering the imports to enhance readability and maintain consistency with Dart conventions. You can combine imports from the same package and remove any unused imports.
Apply this diff to organize the imports:
-import 'package:flutter/rendering.dart' as rendering - show Alignment, AlignmentGeometry, EdgeInsets, Offset, BoxFit; -import 'package:flutter/widgets.dart' - show - Align, - AlignmentGeometry, - BoxConstraints, - ConstrainedBox, - EdgeInsets, - Padding, - SizedBox, - Widget; +import 'package:flutter/widgets.dart' as widgets; +import 'package:flutter/rendering.dart' as rendering; +Update the code to use the aliased imports accordingly.
Committable suggestion skipped: line range outside the PR's diff.
oui/lib/src/core/colors.dart (4)
417-437: 🧹 Nitpick (assertive)
Optimize color conversion calculations in
HslColor.fromHSV.In the
fromHSVfactory constructor ofHslColor, the calculation ofnewSaturationcould be optimized to handle edge cases wherelightnessorvalueequals zero.Review the formula for
newSaturationand ensure it handles division by zero and produces expected results in all cases.
1489-1518: 🧹 Nitpick (assertive)
Avoid redundant override annotation.
In the
DynamicContainerclass, there is a redundant@overrideannotation before thetoStringmethod.Apply this diff to remove the redundant annotation:
@override - @override String toString() {📝 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.class DynamicContainer<T> extends EnumContainer<DynamicTheme, T> { DynamicContainer(super.values); DynamicContainer.generate( T Function(DynamicTheme) generator, ) : super.generate( generator, DynamicTheme.values, ); DynamicContainer<R> map<R>(R Function(T value) mapper) { return DynamicContainer<R>.generate( (theme) => mapper(get(theme)), ); } @override List<DynamicTheme> get keys => DynamicTheme.values; @override String toString() { return 'DynamicContainer(' 'light: ${get(DynamicTheme.light)}, ' 'muted: ${get(DynamicTheme.muted)}, ' 'dimmed: ${get(DynamicTheme.dimmed)}, ' 'dark: ${get(DynamicTheme.dark)})'; } }
90-122: 🧹 Nitpick (assertive)
Handle invalid hex strings more robustly in
Color.fromHex.The
fromHexmethod may throw an exception if the hex string is not in the expected format. To improve robustness, consider adding error handling or validation to provide clearer feedback to the user.Apply this diff to enhance error handling:
// Parse the hex string - int value = int.parse(hex, radix: 16); + int value; + try { + value = int.parse(hex, radix: 16); + } catch (e) { + throw ArgumentError('Invalid hex string: $hex'); + }📝 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.factory Color.fromHex( String hex, [ double? alpha, RgbColorSpace colorSpace = RgbColorSpace.sRGB, ]) { // Remove the leading '#' if present if (hex.startsWith('#')) { hex = hex.substring(1); } // Ensure the hex string is either 6 or 8 characters long if (hex.length == 6) { hex = 'FF$hex'; // Add default alpha value if not provided } else if (hex.length != 8) { throw ArgumentError('Invalid hex string: $hex'); } // Parse the hex string int value; try { value = int.parse(hex, radix: 16); } catch (e) { throw ArgumentError('Invalid hex string: $hex'); } int hexAlpha = (value & 0xFF000000) >> 24; int red = (value & 0x00FF0000) >> 16; int green = (value & 0x0000FF00) >> 8; int blue = value & 0x000000FF; // Create and return the Color object return Color.fromRGB( red / 255, green / 255, blue / 255, alpha ?? hexAlpha / 255, colorSpace, ); }
1374-1463: 🧹 Nitpick (assertive)
Simplify the
uiGradientmethod in theGradientclass.The
uiGradientgetter creates a list ofOffsetobjects only to extract thedxvalues for stops. You can simplify this by directly mapping the stops to their offsets.Apply this diff to simplify the method:
- final List<rendering.Offset> offsets = stops.map((stop) { - return rendering.Offset(stop.offset, 0); - }).toList(); final List<ui.Color> colors = stops.map((stop) { return stop.color.uiColor; }).toList(); + final List<double> stopsList = stops.map((stop) => stop.offset).toList(); switch (type) { case GradientType.linear: return rendering.LinearGradient( colors: colors, - stops: offsets.map((offset) => offset.dx).toList(), + stops: stopsList, begin: direction.begin, end: direction.end, );📝 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./// The list of gradient stops. final List<GradientStop> stops; /// The direction of the gradient flow. final FlowDirection direction; /// The type of the gradient. final GradientType type; /// Creates a gradient with the given stops, direction, and type. const Gradient({ required this.stops, this.direction = FlowDirection.leftToRight, this.type = GradientType.linear, }); /// Creates a linear gradient with the given stops and direction. const Gradient.linear({ required List<GradientStop> stops, FlowDirection direction = FlowDirection.topToBottom, }) : this( stops: stops, direction: direction, type: GradientType.linear, ); /// Creates a radial gradient with the given stops. const Gradient.radial({ required List<GradientStop> stops, }) : this( stops: stops, type: GradientType.radial, ); /// Creates a sweep gradient with the given stops. const Gradient.sweep({ required List<GradientStop> stops, }) : this( stops: stops, type: GradientType.sweep, ); /// Converts the Gradient to a Flutter Gradient. rendering.Gradient get uiGradient { final List<ui.Color> colors = stops.map((stop) { return stop.color.uiColor; }).toList(); final List<double> stopsList = stops.map((stop) => stop.offset).toList(); switch (type) { case GradientType.linear: return rendering.LinearGradient( colors: colors, stops: stopsList, begin: direction.begin, end: direction.end, ); case GradientType.radial: return rendering.RadialGradient( colors: colors, stops: offsets.map((offset) => offset.dx).toList(), radius: 1, ); case GradientType.sweep: return rendering.SweepGradient( colors: colors, stops: offsets.map((offset) => offset.dx).toList(), startAngle: 0, endAngle: 2 * 3.14, ); } } @override int get hashCode { return Object.hash(stops, direction, type); } @override bool operator ==(Object other) { if (identical(this, other)) return true; if (other is! Gradient) return false; return stops == other.stops && direction == other.direction && type == other.type; } }oui/lib/src/core/typography.dart (4)
3-3: 🧹 Nitpick (assertive)
Consider documenting the purpose of this placeholder class
The empty
Typographyclass appears to be a placeholder. Consider adding documentation explaining its intended purpose and future implementation plans.
11-11: 🧹 Nitpick (assertive)
Consider documenting the TypographyGroup type alias
Add documentation explaining the purpose of using
SizedContainer<TypographyStyle>as a typography group.
5-9: 🛠️ Refactor suggestion
Add validation for font weight values
The
FontWeightclass should validate that the weight value falls within the standard font weight range (100-900 in increments of 100).class FontWeight { final double weight; - const FontWeight(this.weight); + const FontWeight(this.weight) : assert( + weight % 100 == 0 && weight >= 100 && weight <= 900, + 'Font weight must be a multiple of 100 between 100 and 900', + ); }📝 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.class FontWeight { final double weight; const FontWeight(this.weight) : assert( weight % 100 == 0 && weight >= 100 && weight <= 900, 'Font weight must be a multiple of 100 between 100 and 900', ); }
13-23: 🧹 Nitpick (assertive)
Add documentation and consider making properties non-nullable
The
TypographyStyleclass looks good with sensible defaults, but could benefit from:
- Documentation explaining the purpose and usage
- Making properties non-nullable since they all have default values
+/// Defines the style properties for typography elements class TypographyStyle { - final double size; - final FontWeight weight; - final bool italic; + final double size; + final FontWeight weight; + final bool italic; const TypographyStyle({ this.size = 14, this.weight = const FontWeight(400), this.italic = false, }); }Committable suggestion skipped: line range outside the PR's diff.
oui/test/src/core/screen/screen_testing_utils.dart (1)
5-20: 🧹 Nitpick (assertive)
Add documentation and consider parameter validation
The
testScreenutility function would benefit from:
- Documentation explaining its purpose and usage
- Validation for the
idparameter to ensure it's not empty+/// Creates a test Screen instance with the given ID and optional segments and children. +/// +/// The screen's path will be derived from the provided segments, or if none are provided, +/// a static path segment will be created from the ID. Screen testScreen( String id, { List<PathSegment> segments = const [], List<Screen> children = const [], }) { + assert(id.isNotEmpty, 'Screen ID cannot be empty'); return Screen( id: id, metadata: ScreenMetadata( path: Localized.always( segments.isEmpty ? [PathSegment.static(id)] : segments, ), name: Localized.always(id), ), children: children, ); }📝 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./// Creates a test Screen instance with the given ID and optional segments and children. /// /// The screen's path will be derived from the provided segments, or if none are provided, /// a static path segment will be created from the ID. Screen testScreen( String id, { List<PathSegment> segments = const [], List<Screen> children = const [], }) { assert(id.isNotEmpty, 'Screen ID cannot be empty'); return Screen( id: id, metadata: ScreenMetadata( path: Localized.always( segments.isEmpty ? [PathSegment.static(id)] : segments, ), name: Localized.always(id), ), children: children, ); }oui/lib/src/components/label.dart (1)
5-31: 🧹 Nitpick (assertive)
Add documentation and consider text validation
The
Labelcomponent implementation looks clean but would benefit from:
- Class and method documentation
- Text validation to prevent empty strings
- Consider applying text style modifiers in buildWithModifiers
+/// A component that displays a text label. +/// +/// The label's appearance can be customized through modifiers. class Label extends Component<Label> { final String text; const Label( this.text, { super.key, super.modifiers, - }); + }) : assert(text.isNotEmpty, 'Label text cannot be empty'); @override Label copyWith({ComponentModifiers? modifiers}) { return Label( text, key: key, modifiers: modifiers ?? this.modifiers, ); } @override Widget buildWithModifiers( ComponentContext context, [ Widget? child, ]) { - final label = Text(text); + final style = context.resolveTextStyle(); + final label = Text(text, style: style); return label; } }Committable suggestion skipped: line range outside the PR's diff.
oui/lib/src/core/interpolation.dart (4)
6-13: 🛠️ Refactor suggestion
Add input validation to LinearCurve
The
LinearCurveimplementation should validate that the input parameter t is in the valid range.class LinearCurve implements Curve { const LinearCurve(); @override double transform(double t) { + assert(t >= 0.0 && t <= 1.0, 'Parameter t must be in the range [0.0, 1.0]'); return t; } }📝 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.class LinearCurve implements Curve { const LinearCurve(); @override double transform(double t) { assert(t >= 0.0 && t <= 1.0, 'Parameter t must be in the range [0.0, 1.0]'); return t; } }
1-4: 🧹 Nitpick (assertive)
Add documentation for the Curve abstraction
The
Curveabstraction would benefit from documentation explaining:
- Its purpose in the interpolation system
- The expected behavior of transform method
- The valid range for the input parameter t
+/// Defines a curve that transforms a linear interpolation parameter. +/// +/// The transform method takes a value t in the range [0.0, 1.0] and returns +/// a transformed value that can be used to control the rate of interpolation. abstract class Curve { const Curve(); + /// Transforms the interpolation parameter t. + /// + /// The input parameter t must be in the range [0.0, 1.0]. double transform(double t); }📝 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./// Defines a curve that transforms a linear interpolation parameter. /// /// The transform method takes a value t in the range [0.0, 1.0] and returns /// a transformed value that can be used to control the rate of interpolation. abstract class Curve { const Curve(); /// Transforms the interpolation parameter t. /// /// The input parameter t must be in the range [0.0, 1.0]. double transform(double t); }
15-19: 🧹 Nitpick (assertive)
Add documentation for the Interpolable mixin
The
Interpolablemixin would benefit from documentation explaining:
- Its purpose and usage
- The contract that implementing classes must follow
- The relationship between lerp, lerpTo, and lerpFrom methods
+/// A mixin that provides linear interpolation capabilities. +/// +/// Classes that implement this mixin must provide a lerp implementation that +/// performs linear interpolation between two values of type T. mixin Interpolable<T> { + /// Linearly interpolates between a and b by t. + /// + /// The parameter t must be in the range [0.0, 1.0]. T lerp(T a, T b, double t); T lerpTo(T b, double t) => lerp(this as T, b, t); T lerpFrom(T a, double t) => lerp(a, this as T, t); }📝 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./// A mixin that provides linear interpolation capabilities. /// /// Classes that implement this mixin must provide a lerp implementation that /// performs linear interpolation between two values of type T. mixin Interpolable<T> { /// Linearly interpolates between a and b by t. /// /// The parameter t must be in the range [0.0, 1.0]. T lerp(T a, T b, double t); T lerpTo(T b, double t) => lerp(this as T, b, t); T lerpFrom(T a, double t) => lerp(a, this as T, t); }
21-39: 🛠️ Refactor suggestion
Add input validation and documentation to interpolators
Both
InterpolatorandDoubleInterpolatorshould:
- Have proper documentation
- Validate the input parameter t
- Consider adding convenience methods for common cases
+/// An interpolator that works with any type implementing the Interpolable mixin. class Interpolator<T extends Interpolable<T>> { final Curve curve; const Interpolator({ this.curve = const LinearCurve(), }); T resolve(T a, T b, double t) { + assert(t >= 0.0 && t <= 1.0, 'Parameter t must be in the range [0.0, 1.0]'); return a.lerpTo(b, curve.transform(t)); } } +/// An interpolator specifically designed for double values. class DoubleInterpolator { final Curve curve; const DoubleInterpolator({ this.curve = const LinearCurve(), }); double resolve(double a, double b, double t) { + assert(t >= 0.0 && t <= 1.0, 'Parameter t must be in the range [0.0, 1.0]'); return a + (b - a) * curve.transform(t); } + + /// Convenience method to interpolate to zero + double lerpToZero(double a, double t) => resolve(a, 0.0, t); + + /// Convenience method to interpolate from zero + double lerpFromZero(double b, double t) => resolve(0.0, b, t); }📝 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./// An interpolator that works with any type implementing the Interpolable mixin. class Interpolator<T extends Interpolable<T>> { final Curve curve; const Interpolator({ this.curve = const LinearCurve(), }); T resolve(T a, T b, double t) { assert(t >= 0.0 && t <= 1.0, 'Parameter t must be in the range [0.0, 1.0]'); return a.lerpTo(b, curve.transform(t)); } } /// An interpolator specifically designed for double values. class DoubleInterpolator { final Curve curve; const DoubleInterpolator({ this.curve = const LinearCurve(), }); double resolve(double a, double b, double t) { assert(t >= 0.0 && t <= 1.0, 'Parameter t must be in the range [0.0, 1.0]'); return a + (b - a) * curve.transform(t); } /// Convenience method to interpolate to zero double lerpToZero(double a, double t) => resolve(a, 0.0, t); /// Convenience method to interpolate from zero double lerpFromZero(double b, double t) => resolve(0.0, b, t); }oui/test/src/core/interpolation_test.dart (3)
5-12: 🧹 Nitpick (assertive)
Consider adding edge case tests for LinearCurve.
While the basic functionality is tested, consider adding tests for:
- Negative values
- Values outside the [0,1] range
- Very small values near zero
- Very large values
14-24: 🧹 Nitpick (assertive)
Add error handling tests for Interpolator.
The current tests only cover the happy path. Consider adding tests for:
- Null inputs
- Invalid interpolation values (t < 0 or t > 1)
- Edge cases where a and b are the same value
37-49: 🧹 Nitpick (assertive)
Consider implementing equals() and hashCode() for InterpolableDouble.
Since this class represents a value type, it would benefit from proper value equality implementation.
class InterpolableDouble with Interpolable<InterpolableDouble> { final double value; InterpolableDouble(this.value); + + @override + bool operator ==(Object other) { + if (identical(this, other)) return true; + return other is InterpolableDouble && other.value == value; + } + + @override + int get hashCode => value.hashCode;📝 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.class InterpolableDouble with Interpolable<InterpolableDouble> { final double value; InterpolableDouble(this.value); @override bool operator ==(Object other) { if (identical(this, other)) return true; return other is InterpolableDouble && other.value == value; } @override int get hashCode => value.hashCode; @override InterpolableDouble lerp( InterpolableDouble a, InterpolableDouble b, double t, ) { return InterpolableDouble(a.value + (b.value - a.value) * t); } }oui/lib/src/core/config.dart (2)
66-75: 🛠️ Refactor suggestion
Make defaultRailWidth configurable.
The
defaultRailWidthis declared as a non-final constant. Consider making it configurable through the constructor.class ScaffoldConfig { final bool usePanels; final Size defaultPanelSize; - final double defaultRailWidth = 68; + final double defaultRailWidth; const ScaffoldConfig({ this.usePanels = true, this.defaultPanelSize = Size.infinite, + this.defaultRailWidth = 68, }); }📝 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.class ScaffoldConfig { final bool usePanels; final Size defaultPanelSize; final double defaultRailWidth; const ScaffoldConfig({ this.usePanels = true, this.defaultPanelSize = Size.infinite, this.defaultRailWidth = 68, }); }
6-39: 🧹 Nitpick (assertive)
Consider enhancing Version class with parsing capabilities.
The Version class could benefit from:
- A factory constructor to parse version strings
- Comparison operators for version ordering
- Methods to check compatibility
class Version { // ... existing code ... + + factory Version.parse(String version) { + final parts = version.split('.'); + if (parts.length != 3) { + throw FormatException('Invalid version format'); + } + return Version( + int.parse(parts[0]), + int.parse(parts[1]), + int.parse(parts[2]), + ); + } + + bool operator <(Version other) => + major < other.major || + (major == other.major && minor < other.minor) || + (major == other.major && minor == other.minor && patch < other.patch);Committable suggestion skipped: line range outside the PR's diff.
oui/test/src/components/aligner_test.dart (2)
33-36:
⚠️ Potential issueGolden test files are missing.
The pipeline failure indicates missing golden files. Ensure you:
- Generate the initial golden files
- Commit them to the repository
- Document the process for updating golden files
🧰 Tools
🪛 GitHub Actions: Oui! Tests
[error] 33-33: Golden test failure: Could not compare widget against non-existent golden file 'goldens/aligner_right_to_left.png'
8-37: 🧹 Nitpick (assertive)
Enhance golden tests coverage.
Consider adding tests for:
- Different screen sizes
- Different numbers of children
- Empty child list
- Single child
🧰 Tools
🪛 GitHub Actions: Oui! Tests
[error] 33-33: Golden test failure: Could not compare widget against non-existent golden file 'goldens/aligner_right_to_left.png'
oui/test/src/core/screen/screen_metadata_test.dart (2)
8-23: 🧹 Nitpick (assertive)
Add tests for invalid path scenarios.
Consider adding tests for:
- Empty path segments
- Invalid path characters
- Duplicate path segments
25-42: 🧹 Nitpick (assertive)
Add tests for localization scenarios.
Current tests only use
Localized.always(). Consider adding tests for:
- Different locales
- Fallback behavior
- Missing translations
oui/test/src/core/geometry/size_test.dart (1)
4-101: 🧹 Nitpick (assertive)
Consider adding edge case tests for Size class.
The test suite is well-structured but could benefit from additional test cases:
- Negative dimensions
- Infinite dimensions (besides the default case)
- Zero dimensions (explicit vs default)
Example test cases to add:
+ test('should handle negative dimensions correctly', () { + final size = Size.fixed(width: -100, height: -200); + expect(size.width.start, -100); + expect(size.height.start, -200); + }); + + test('should handle infinite dimensions explicitly', () { + final size = Size.fixed( + width: double.infinity, + height: double.infinity, + ); + expect(size.width.end, double.infinity); + expect(size.height.end, double.infinity); + }); + + test('should handle zero dimensions explicitly', () { + final size = Size.fixed(width: 0, height: 0); + expect(size.width.start, 0); + expect(size.height.start, 0); + });📝 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.void main() { group('Size', () { test('fixed constructor initializes correctly', () { const width = 100.0; const height = 200.0; final size = Size.fixed(width: width, height: height); expect(size.width.start, width); expect(size.width.end, width); expect(size.height.start, height); expect(size.height.end, height); }); test('fixed constructor initializes with only width', () { const width = 100.0; final size = Size.fixed(width: width); expect(size.width.start, width); expect(size.width.end, width); expect(size.height.start, 0); expect(size.height.end, double.infinity); }); test('fixed constructor initializes with only height', () { const height = 200.0; final size = Size.fixed(height: height); expect(size.width.start, 0); expect(size.width.end, double.infinity); expect(size.height.start, height); expect(size.height.end, height); }); test('dynamic constructor initializes correctly', () { const minWidth = 50.0; const maxWidth = 150.0; const minHeight = 100.0; const maxHeight = 300.0; final size = Size.dynamic( minWidth: minWidth, maxWidth: maxWidth, minHeight: minHeight, maxHeight: maxHeight, ); expect(size.width.start, minWidth); expect(size.width.end, maxWidth); expect(size.height.start, minHeight); expect(size.height.end, maxHeight); }); test('fixed constructor defaults to zeroToInfinity', () { final size = Size.fixed(); expect(size.width.start, 0); expect(size.width.end, double.infinity); expect(size.height.start, 0); expect(size.height.end, double.infinity); }); test('dynamic constructor defaults to zero and infinity', () { final size = Size.dynamic(); expect(size.width.start, 0); expect(size.width.end, double.infinity); expect(size.height.start, 0); expect(size.height.end, double.infinity); }); test('equality operator returns true for equal sizes', () { const width = 100.0; const height = 200.0; final size1 = Size.fixed(width: width, height: height); final size2 = Size.fixed(width: width, height: height); expect(size1, equals(size2)); }); test('hashCode returns same value for equal sizes', () { const width = 100.0; const height = 200.0; final size1 = Size.fixed(width: width, height: height); final size2 = Size.fixed(width: width, height: height); expect(size1.hashCode, equals(size2.hashCode)); }); test('toString returns correct string representation', () { const width = 100.0; const height = 200.0; final size = Size.fixed(width: width, height: height); expect( size.toString(), 'Size(width: RangedDimension.fixed($width), height: RangedDimension.fixed($height))', ); }); test('should handle negative dimensions correctly', () { final size = Size.fixed(width: -100, height: -200); expect(size.width.start, -100); expect(size.height.start, -200); }); test('should handle infinite dimensions explicitly', () { final size = Size.fixed( width: double.infinity, height: double.infinity, ); expect(size.width.end, double.infinity); expect(size.height.end, double.infinity); }); test('should handle zero dimensions explicitly', () { final size = Size.fixed(width: 0, height: 0); expect(size.width.start, 0); expect(size.height.start, 0); }); });oui/lib/src/components/aligner.dart (2)
54-111: 🧹 Nitpick (assertive)
Simplify complex positioning logic in paintChildren.
The positioning logic in
paintChildrenis complex and could be simplified by extracting helper methods.Consider refactoring into smaller, focused methods:
@override void paintChildren(FlowPaintingContext context) { + final LayoutState state = initializeLayoutState(context); + + for (var i = 0; i < context.childCount; i++) { + final childSize = context.getChildSize(i); + if (childSize == null) continue; + + if (shouldWrapToNextLine(state, childSize)) { + wrapToNextLine(state); + if (state.currentLine > maxLines) break; + } + + paintChild(context, i, state, childSize); + updateLayoutState(state, childSize); + } + } + + LayoutState initializeLayoutState(FlowPaintingContext context) { + return LayoutState( + x: flowDirection.isReversed && flowDirection.isHorizontal + ? context.size.width + : 0.0, + y: flowDirection.isReversed && flowDirection.isVertical + ? context.size.height + : 0.0, + ); + }Committable suggestion skipped: line range outside the PR's diff.
16-40: 🛠️ Refactor suggestion
Document public API and handle unused property.
The
Alignerclass needs documentation for its public API, and thescrollableproperty is currently unused.+/// A widget that aligns its children in a flow layout. +/// +/// The [Aligner] widget arranges its children according to the specified +/// [alignment] and [flowDirection]. Children are laid out in a flow layout +/// that can wrap to multiple lines if needed. class Aligner extends StatelessWidget { + /// The widgets to be arranged in the flow layout. final List<Widget> children; + + /// The alignment of the entire flow layout within its parent. final Alignment alignment; + + /// The direction in which children should flow. final FlowDirection flowDirection; + + /// Whether the layout should be scrollable. + /// + /// Note: This property is currently not implemented. + @Deprecated('This property is not yet implemented') final bool scrollable;📝 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./// A widget that aligns its children in a flow layout. /// /// The [Aligner] widget arranges its children according to the specified /// [alignment] and [flowDirection]. Children are laid out in a flow layout /// that can wrap to multiple lines if needed. class Aligner extends StatelessWidget { /// The widgets to be arranged in the flow layout. final List<Widget> children; /// The alignment of the entire flow layout within its parent. final Alignment alignment; /// The direction in which children should flow. final FlowDirection flowDirection; /// Whether the layout should be scrollable. /// /// Note: This property is currently not implemented. @Deprecated('This property is not yet implemented') final bool scrollable; const Aligner({ super.key, required this.children, this.alignment = Alignment.center, this.flowDirection = FlowDirection.leftToRight, this.scrollable = false, }); @override Widget build(BuildContext context) { return Align( alignment: alignment.uiAlignment, child: Flow( delegate: AlignerDelegate(flowDirection), children: children, ), ); } }oui/test/src/core/localization/localized_test.dart (1)
4-103: 🧹 Nitpick (assertive)
Enhance locale test coverage with additional scenarios.
Consider adding test cases for:
- Script codes (e.g., 'zh-Hans-CN')
- Variant codes (e.g., 'en-US-POSIX')
- Extension codes (e.g., 'en-US-u-ca-buddhist')
+ test('should handle script codes correctly', () { + const locale = Locale('zh', 'CN', script: 'Hans'); + expect(locale.toString(), 'zh_Hans_CN'); + expect(locale, equals(const Locale('zh', 'CN', script: 'Hans'))); + }); + + test('should handle variant codes correctly', () { + const locale = Locale('en', 'US', variant: 'POSIX'); + expect(locale.toString(), 'en_US_POSIX'); + expect(locale, equals(const Locale('en', 'US', variant: 'POSIX'))); + });Committable suggestion skipped: line range outside the PR's diff.
oui/lib/src/core/shadow.dart (3)
23-31:
⚠️ Potential issueAdd validation for negative blur/spread values.
The constructor should validate that blur and spread values are non-negative.
const Shadow({ required this.blur, required this.spread, required this.color, required this.offset, - }); + }) : assert(blur >= 0, 'Blur must be non-negative'), + assert(spread >= 0, 'Spread must be non-negative');📝 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./// Creates a new [Shadow] instance with the given properties. /// /// All parameters are required. const Shadow({ required this.blur, required this.spread, required this.color, required this.offset, }) : assert(blur >= 0, 'Blur must be non-negative'), assert(spread >= 0, 'Spread must be non-negative');
92-99: 🧹 Nitpick (assertive)
Optimize list operations in decoration handling.
Using spread operator with nullable lists can create unnecessary intermediate lists.
- return decoration.copyWith( - boxShadow: [ - normalizedShadow.boxShadow, - ...?decoration.boxShadow, - ], - ); + final existingShadows = decoration.boxShadow; + return decoration.copyWith( + boxShadow: existingShadows == null + ? [normalizedShadow.boxShadow] + : [normalizedShadow.boxShadow, ...existingShadows], + );📝 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.if (decoration is BoxDecoration) { final existingShadows = decoration.boxShadow; return decoration.copyWith( boxShadow: existingShadows == null ? [normalizedShadow.boxShadow] : [normalizedShadow.boxShadow, ...existingShadows], ); }
9-62: 🛠️ Refactor suggestion
Implement equality and hashCode for Shadow class.
The
Shadowclass should implement==operator andhashCodefor proper equality comparison in collections.class Shadow { final double blur; final double spread; final Color color; final Offset offset; const Shadow({ required this.blur, required this.spread, required this.color, required this.offset, }); + @override + bool operator ==(Object other) { + if (identical(this, other)) return true; + return other is Shadow && + other.blur == blur && + other.spread == spread && + other.color == color && + other.offset == offset; + } + + @override + int get hashCode => Object.hash(blur, spread, color, offset);📝 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./// A class representing a shadow effect. class Shadow { /// The blur radius of the shadow. final double blur; /// The spread radius of the shadow. final double spread; /// The color of the shadow. final Color color; /// The offset of the shadow. final Offset offset; /// Creates a new [Shadow] instance with the given properties. /// /// All parameters are required. const Shadow({ required this.blur, required this.spread, required this.color, required this.offset, }); Shadow withColor(Color color) { return Shadow( blur: blur, spread: spread, color: color, offset: offset, ); } /// Determines whether the shadow should be rendered. /// /// Returns `true` if the shadow should be rendered, otherwise `false`. bool get shouldRender { if (color.isVisible != true) return false; if (blur == 0 && spread == 0 && offset == Offset.zero) return false; return true; } /// Converts this [Shadow] to a [BoxShadow] object. /// /// Returns a [BoxShadow] with the same properties as this [Shadow]. painting.BoxShadow get boxShadow { return painting.BoxShadow( blurRadius: blur, spreadRadius: spread, color: color.uiColor, offset: offset.uiOffset, ); } @override bool operator ==(Object other) { if (identical(this, other)) return true; return other is Shadow && other.blur == blur && other.spread == spread && other.color == color && other.offset == offset; } @override int get hashCode => Object.hash(blur, spread, color, offset); }oui/test/src/core/colors/color_hsv_test.dart (2)
25-29: 🧹 Nitpick (assertive)
Add edge case tests for hue rotation.
Consider adding tests for edge cases in hue rotation:
- Rotation that results in exactly 360°
- Multiple full rotations (>360°)
- Large negative rotations
test('should handle edge cases in hue rotation', () { final color = HsvColor.fromHSV(350); expect(color.rotateHue(370).hue, 0); // Full rotation plus 10 expect(color.rotateHue(-710).hue, 0); // Two full negative rotations minus 10 });
90-98: 🧹 Nitpick (assertive)
Consider using a test data table for value clamping tests.
The test would be more maintainable and easier to extend using a table-driven approach.
test('should clamp value correctly', () { final testCases = [ (input: 1.0, min: 0.0, max: 0.8, expected: 0.8), (input: 0.3, min: 0.4, max: 0.8, expected: 0.4), (input: 0.6, min: 0.4, max: 0.8, expected: 0.6), ]; for (final (input, min, max, expected) in testCases) { final color = HsvColor.fromHSV(120, 0.5, input); final clampedColor = color.clampingLightness(min, max); expect(clampedColor.value, expected); } });oui/test/src/core/colors/palette/accentable_color_test.dart (1)
68-80: 🧹 Nitpick (assertive)
Reconsider the large number test approach.
Testing with 1000 color levels might be excessive and could slow down the test suite. Consider:
- Testing with a smaller but still significant number (e.g., 100)
- Testing specific edge cases instead of bulk testing
test('Handles reasonable number of levels', () { final normalColor = Color.fromHex('#0000FF'); final levels = LeveledContainer<Color>( List.generate( 100, // Reduced from 1000 (index) => Color.fromHex('#${index.toRadixString(16).padLeft(6, '0')}'), ), ); final accentableColor = AccentableColor(normalColor, levels); // Test first, middle, and last indices expect(accentableColor.accented(0), levels[0]); expect(accentableColor.accented(49), levels[49]); expect(accentableColor.accented(99), levels[99]); });oui/test/src/core/colors/color_test.dart (1)
110-125: 🧹 Nitpick (assertive)
Enhance contrast ratio test coverage.
The contrast ratio tests could be more comprehensive and better structured:
- Use a data-driven approach
- Include edge cases
- Add tests for accessibility standards (WCAG)
test('contrastAgainst with WCAG levels', () { final testCases = [ ( background: Color.white, foreground: Color.black, expected: 21.0, meetsWCAGAAA: true, ), ( background: Color.white, foreground: const Color.fromRGB(0.0, 0.0, 0.7), expected: 4.5, meetsWCAGAA: true, ), // Add more cases... ]; for (final (background, foreground, expected, meetsWCAGAAA) in testCases) { final contrast = background.contrastAgainst(foreground); expect(contrast, closeTo(expected, 0.1)); if (meetsWCAGAAA) { expect(contrast >= 7.0, true, reason: 'Should meet WCAG AAA'); } } });oui/test/src/core/colors/gradient_test.dart (2)
51-68: 🧹 Nitpick (assertive)
Add validation tests for sweep gradient angles.
The sweep gradient tests should include:
- Custom start/end angles
- Full circle validation
- Negative angle handling
test('sweep gradient with custom angles', () { final gradient = Gradient.sweep( stops: [ GradientStop(0.0, customRed), GradientStop(1.0, customBlue), ], startAngle: pi / 2, // 90 degrees endAngle: pi, // 180 degrees ); final flutterGradient = gradient.uiGradient as SweepGradient; expect(flutterGradient.startAngle, pi / 2); expect(flutterGradient.endAngle, pi); }); test('sweep gradient with negative angles', () { final gradient = Gradient.sweep( stops: [ GradientStop(0.0, customRed), GradientStop(1.0, customBlue), ], startAngle: -pi / 2, endAngle: pi / 2, ); final flutterGradient = gradient.uiGradient as SweepGradient; expect(flutterGradient.startAngle, -pi / 2); expect(flutterGradient.endAngle, pi / 2); });
33-49: 🧹 Nitpick (assertive)
Add tests for custom center points in radial gradients.
The radial gradient tests should include validation of custom center points and focal points.
test('radial gradient with custom center', () { final gradient = Gradient.radial( stops: [ GradientStop(0.0, customRed), GradientStop(1.0, customBlue), ], center: const Point(0.25, 0.75), focal: const Point(0.4, 0.6), ); final flutterGradient = gradient.uiGradient as RadialGradient; expect(flutterGradient.center.dx, 0.25); expect(flutterGradient.center.dy, 0.75); expect(flutterGradient.focal?.dx, 0.4); expect(flutterGradient.focal?.dy, 0.6); });oui/test/src/core/colors/color_hsl_test.dart (3)
6-11: 🧹 Nitpick (assertive)
Consider adding edge case tests for invalid input combinations.
The test suite would benefit from additional test cases that verify behavior with invalid input combinations, such as:
- NaN or infinite values
- Very large numbers
- Negative values (where not already tested)
89-96: 🧹 Nitpick (assertive)
Add tests for floating-point precision handling.
Consider adding test cases that verify the behavior of color interpolation with floating-point values that might cause precision issues.
98-104: 🧹 Nitpick (assertive)
Add tests for color equality comparison.
The test suite should include cases that verify color equality comparison, especially for colors that are visually identical but might have slightly different numeric representations.
oui/test/src/core/geometry/ranged_dimension_test.dart (2)
148-162: 🧹 Nitpick (assertive)
Add tests for edge cases with special numeric values.
Consider adding test cases for:
- NaN values in dynamic ranges
- Infinite values in range bounds
- Very large numbers that might cause overflow
148-162: 🧹 Nitpick (assertive)
Enhance operator test coverage.
The addition and subtraction operator tests could be more comprehensive by including:
- Edge cases (e.g., adding/subtracting zero or infinite dimensions)
- Chaining multiple operations
- Order of operations
oui/test/src/core/router/path_test.dart (2)
81-87: 🧹 Nitpick (assertive)
Add tests for path normalization.
Consider adding test cases that verify:
- Handling of multiple consecutive slashes
- Normalization of relative paths
- Handling of dot segments (. and ..)
89-93: 🧹 Nitpick (assertive)
Add tests for Unicode path handling.
The test suite should include cases that verify proper handling of:
- Unicode characters in paths
- URL-encoded characters
- International domain names
oui/test/src/core/colors/palette/box_colors_test.dart (2)
252-275: 🧹 Nitpick (assertive)
Add missing edge case tests.
The test suite would benefit from additional edge cases:
- Test with empty
LeveledContainer- Test with null values in the container
- Test with invalid hex colors
test('should handle empty LeveledContainer', () { final color = AccentableColor( Color.black, LeveledContainer<Color>([]), ); final boxColors = BoxColors( content: color, surface: color, decoration: color, shadow: color, edge: color, placeholder: color, ); expect(boxColors.content, equals(color)); }); test('should handle invalid hex colors', () { expect( () => Color.fromHex("invalid"), throwsFormatException, ); });
8-41: 🧹 Nitpick (assertive)
Consider extracting common test setup code.
The creation of
AccentableColorinstances with the same values is repeated across multiple test cases. Consider usingsetUpto reduce code duplication.group('BoxColors', () { + late AccentableColor color1; + late AccentableColor color2; + + setUp(() { + color1 = AccentableColor( + Color.black, + LeveledContainer<Color>([ + Color.fromHex("#123456"), + ]), + ); + + color2 = AccentableColor( + Color.white, + LeveledContainer<Color>([ + Color.fromHex("#654321"), + ]), + ); + });Committable suggestion skipped: line range outside the PR's diff.
oui/test/src/core/screen/screen_registry_test.dart (1)
49-77: 🧹 Nitpick (assertive)
Enhance test descriptions for clarity.
The test descriptions could be more specific about the scenarios being tested. Consider using the "Given-When-Then" format for better clarity.
-test('should handle single-level tree with one screen', () { +test('given a single screen, when matching exact path, then returns correct screen', () {📝 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.group('ScreenRegistry', () { group('Single-level tree', () { test('given a single screen, when matching exact path, then returns correct screen', () { final screen = testScreen('home'); final registry = ScreenRegistry(screen); final match = registry.match(['home']); expect(match.screens.first, screen); }); test('should handle single-level dynamic tree with one screen', () { final screen = testScreen( 'profile', segments: [PathSegment.argument('userId')], ); final registry = ScreenRegistry(screen); final match = registry.match(['123']); expect(match.screens.first, screen); }); test('should handle pattern matching', () { final screen = testScreen( 'profile', segments: [PathSegment.argument('userId', pattern: r'\d+')], ); final registry = ScreenRegistry(screen); final match = registry.match(['123']); expect(match.screens.first, screen); });.vscode/settings.json (1)
16-21: 🧹 Nitpick (assertive)
Consider enabling additional Copilot features for enhanced productivity.
The current Copilot configuration is good, but could be enhanced further.
Consider adding these settings for better code suggestions:
+ "github.copilot.editor.enableAutoCompletions": true, + "github.copilot.enable": { + "*": true, + "yaml": false, + "plaintext": false + }📝 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."github.copilot.chat.codeGeneration.useInstructionFiles": true, "github.copilot.chat.edits.suggestRelatedFilesForTests": true, "github.copilot.chat.edits.suggestRelatedFilesFromGitHistory": true, "github.copilot.chat.fixTestFailure.enabled": true, "github.copilot.chat.generateTests.codeLens": true, "github.copilot.chat.search.semanticTextResults": true, "github.copilot.editor.enableAutoCompletions": true, "github.copilot.enable": { "*": true, "yaml": false, "plaintext": false }.github/copilot-instructions.md (1)
1-21: 🧹 Nitpick (assertive)
Add a top-level heading to comply with markdown standards.
The static analysis tool correctly identifies a missing top-level heading.
Add this at the beginning of the file:
+ # GitHub Copilot Instructions + 1. **Plan Before Coding**: Design and outline steps before coding.📝 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.# GitHub Copilot Instructions 1. **Plan Before Coding**: Design and outline steps before coding. 2. **Keep It Simple**: Write clear, maintainable, and modular code. 3. **Reuse Code**: Avoid duplication; create reusable components. 4. **Document**: Add meaningful comments and documentation. 5. **Assert Everything**: Validate logic and inputs with assertions. 6. **Follow Conventions**: Use established coding styles and validate inputs. 7. **Optimize Later**: Focus on correctness first, optimize later. 8. **Secure Code**: Sanitize inputs and protect sensitive data. 9. **Immutability**: Prefer immutable data structures. 10. **Clarity Over Cleverness**: Prioritize readability and maintainability. ## Task Instructions - Follow the above principles. - Ensure code is well-structured and documented. - Validate inputs and use assertions. - Focus on secure and maintainable code. - Write a brief explanation of your approach and trade-offs. - Follow the below instructions as closely as possible. --- Instructions Below ---🧰 Tools
🪛 Markdownlint (0.37.0)
1-1: null
First line in a file should be a top-level heading(MD041, first-line-heading, first-line-h1)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (79)
.github/workflows/ci.yml (1)
47-47: 🧹 Nitpick (assertive)
Coverage report path is correctly updated
The path change aligns with the new project structure. Consider adding path validation to ensure the coverage file exists before upload.
- name: Upload coverage to Codecov if: github.ref == 'refs/heads/main' + run: | + if [ ! -f "packages/oui/coverage/lcov.info" ]; then + echo "Coverage file not found!" + exit 1 + fi uses: codecov/codecov-action@v5Committable suggestion skipped: line range outside the PR's diff.
packages/oui/test/src/core/colors/palette/accentable_color_test.dart (2)
7-67: 🧹 Nitpick (assertive)
Consider reducing test setup duplication.
Many test cases repeat the same setup code for creating colors and levels. Consider extracting common setup into helper methods or using test fixtures.
Example refactor:
void main() { group('AccentableColor Tests', () { late Color normalColor; late LeveledContainer<Color> levels; late AccentableColor accentableColor; setUp(() { normalColor = Color.fromHex('#0000FF'); levels = LeveledContainer<Color>([ Color.fromHex('#0000FF'), Color.fromHex('#00FF00'), Color.fromHex('#FF0000'), ]); accentableColor = AccentableColor(normalColor, levels); }); // Your existing tests can now use these pre-setup variables // ... }); }Also applies to: 81-115
68-80: 🧹 Nitpick (assertive)
Consider refactoring the large number test.
A few suggestions for improvement:
- Extract the magic number 1000 into a named constant
- The test name implies performance testing, but it only verifies correctness
- The expected hex color
#0003E7seems incorrect for index 999 (should be#0003E7be#0003E8?)Consider this refactor:
+ // Define constants at the top of the file + const int _largeNumberOfLevels = 1000; + test('Handles large number of levels', () { final normalColor = Color.fromHex('#0000FF'); final levels = LeveledContainer<Color>( List.generate( - 1000, + _largeNumberOfLevels, (index) => Color.fromHex('#${index.toRadixString(16).padLeft(6, '0')}'), ), ); final accentableColor = AccentableColor(normalColor, levels); - expect(accentableColor.accented(999), Color.fromHex('#0003E7')); + // Verify the last color is correct + expect( + accentableColor.accented(_largeNumberOfLevels - 1), + Color.fromHex('#0003E8'), + ); });Committable suggestion skipped: line range outside the PR's diff.
packages/oui/test/src/core/colors/gradient_test.dart (2)
67-67: 🧹 Nitpick (assertive)
Replace magic number with dart:math pi constant
Instead of using the magic number
2 * 3.14for the sweep gradient's end angle, use2 * pifrom dart:math for better precision and clarity.- expect(flutterGradient.endAngle, 2 * 3.14); + expect(flutterGradient.endAngle, 2 * pi);Add this import at the top:
import 'dart:math' show pi;
137-155: 🧹 Nitpick (assertive)
Consider adding tests for other flow directions
While the rightToLeft direction test is good, consider adding test cases for other FlowDirection values to ensure comprehensive coverage. This could include:
- leftToRight (default case)
- topToBottom
- bottomToTop
Example test case:
test('linear gradient with bottomToTop direction', () { final gradient = Gradient.linear( stops: [ GradientStop(0.0, customRed), GradientStop(1.0, customBlue), ], direction: FlowDirection.bottomToTop, ); final flutterGradient = gradient.uiGradient as LinearGradient; expect( flutterGradient.colors, [customRed.uiColor, customBlue.uiColor], ); expect(flutterGradient.stops, [0.0, 1.0]); expect(flutterGradient.begin, rendering.Alignment.bottomCenter); expect(flutterGradient.end, rendering.Alignment.topCenter); });packages/oui/test/src/core/corners/corner_border_radius_test.dart (2)
8-24: 🧹 Nitpick (assertive)
Enhance test coverage with additional test cases.
While the basic initialization test is good, consider adding:
- Test cases with different radius/smoothing combinations for each corner
- Edge cases (zero radius, negative values)
- Test for the
.all()constructorExample test cases to add:
test('handles different corner configurations', () { final borderRadius = CornerBorderRadius.only( topLeft: CornerRadius(radius: 10.0, smoothing: 0.5), topRight: CornerRadius(radius: 20.0, smoothing: 0.3), bottomLeft: CornerRadius(radius: 0.0, smoothing: 0.0), bottomRight: CornerRadius(radius: 15.0, smoothing: 1.0), ); expect(borderRadius.topLeft.radius, 10.0); expect(borderRadius.topRight.smoothing, 0.3); expect(borderRadius.bottomLeft.radius, 0.0); expect(borderRadius.bottomRight.smoothing, 1.0); });
1-51: 🧹 Nitpick (assertive)
Consider separating golden tests into a dedicated file.
For better maintainability and organization, consider:
- Moving golden tests to a separate file (e.g.,
corner_border_radius_golden_test.dart)- Creating a shared test configuration for golden tests
- Adding a README.md in the goldens directory explaining how to update golden files
packages/oui/lib/src/core/screen.dart (1)
284-285: 🧹 Nitpick (assertive)
Enhance exception message for duplicate screen IDs
When a duplicate screen ID is detected, the exception message could provide more context to assist in debugging. Including the path or parent screens in the message can help identify where the duplication occurred.
Consider updating the exception message:
- throw Exception('Duplicate screen ID: ${screen.id}'); + throw Exception('Duplicate screen ID "${screen.id}" found with path: ${path.segments.join('/')}. Parent screens: ${parents?.map((s) => s.id).join(' -> ') ?? 'None'}');Committable suggestion skipped: line range outside the PR's diff.
packages/oui/test/src/shared/metadata_test.dart (2)
9-12: 🧹 Nitpick (assertive)
Redundant null and empty checks in test
In the test case, checking
metadata.icon.baseforisNullandmetadata.attributes.baseforisEmptymay be redundant since these are the default values when not provided. Simplifying the test can make it more focused on the primary functionality.
40-47: 🧹 Nitpick (assertive)
Improve clarity in attributes manipulation test
The manipulation of attributes using
copyWithcould be clearer. Explicitly demonstrating the before and after states or adding comments can enhance readability and understanding of the test.packages/oui/test/src/core/screen/screen_metadata_test.dart (1)
61-77: 🧹 Nitpick (assertive)
Add tests for
copyWithmethod inScreenMetadataWhile the current tests verify instance creation, they do not cover the
copyWithmethod. Adding tests forcopyWithwould ensure that updates toScreenMetadatainstances function as intended.packages/oui/lib/src/core/utils.dart (5)
121-125:
⚠️ Potential issueCorrect the error message in
EnumContainerconstructorThe error message
'Values must be unique'does not correspond to the condition being checked, which verifies that the keys in_valuesmatch the expectedkeys. This can be confusing for users.Update the error message for clarity:
EnumContainer(this._values) { if (_values.keys.toSet() != keys.toSet()) { - throw ArgumentError('Values must be unique'); + throw ArgumentError('Keys in provided values must match the expected enum keys'); } }If the intention is to ensure that the values are unique, an additional check should be implemented:
final valuesSet = _values.values.toSet(); if (valuesSet.length != _values.values.length) { throw ArgumentError('Values must be unique'); }This will accurately validate the uniqueness of the values.
186-197:
⚠️ Potential issueHandle empty
_itemslist to prevent runtime errorsAccessing
_items.firstor_items.lastwhen_itemsis empty will result in a runtime error. The check forisEmptyshould be performed before attempting to access these elements.Modify the code to ensure safety:
T get(int level) { if (isEmpty) { throw StateError('LeveledContainer is empty'); } final index = (level - _firstLevel) ~/ _levelStep; if (index < 0) { return _items.first; } else if (index >= _items.length) { return _items.last; } return _items[index]; }This adjustment checks if
_itemsis empty at the beginning and prevents accessing elements when the list is empty.
261-267: 🧹 Nitpick (assertive)
Remove unused
indentparameter intoStringmethodThe
indentparameter in thetoStringmethod is declared but not utilized, which might lead to confusion.Simplify the method signature:
- String toString([int indent = 0]) { + @override + String toString() { return 'LeveledContainer(\n' ' firstLevel: $_firstLevel,\n' ' levelStep: $_levelStep,\n' ' items: $_items\n' ')'; }This makes the code cleaner and avoids potential misunderstandings about the purpose of the parameter.
📝 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.@override String toString() { return 'LeveledContainer(\n' ' firstLevel: $_firstLevel,\n' ' levelStep: $_levelStep,\n' ' items: $_items\n' ')'; }
90-91: 🧹 Nitpick (assertive)
Use
Object.hashfor betterhashCodeimplementationThe current
hashCodeimplementation in theRangeclass uses the bitwise XOR (^) operator, which may lead to hash collisions and poor hash distribution. It's recommended to useObject.hashfor combining hash codes to improve distribution and reduce collision probability.Apply this diff to enhance the
hashCodemethod:- int get hashCode => start.hashCode ^ end.hashCode; + @override + int get hashCode => Object.hash(start, end);📝 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.@override int get hashCode => Object.hash(start, end);
94-109: 🛠️ Refactor suggestion
Replace operator overloading with descriptive methods
Overloading the
+and-operators for range union and intersection may be unclear to users, as these operations don't align with typical arithmetic usage. It's clearer and more maintainable to define methods likeunionandintersectionto perform these operations.Consider modifying the code as follows:
class Range<T extends Comparable> { ... - Range<T> operator +(Range<T> other) { + Range<T> union(Range<T> other) { final newStart = (start.compareTo(other.start) < 0) ? start : other.start; final newEnd = (end.compareTo(other.end) > 0) ? end : other.end; return Range(newStart, newEnd); } - Range<T>? operator -(Range<T> other) { + Range<T>? intersection(Range<T> other) { if (other.end.compareTo(start) < 0 || other.start.compareTo(end) > 0) { return null; // No overlap } final newStart = (start.compareTo(other.start) > 0) ? start : other.start; final newEnd = (end.compareTo(other.end) < 0) ? end : other.end; return (newStart.compareTo(newEnd) <= 0) ? Range(newStart, newEnd) : null; } }This change enhances code readability and aligns method names with their functionality.
📝 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.Range<T> union(Range<T> other) { final newStart = (start.compareTo(other.start) < 0) ? start : other.start; final newEnd = (end.compareTo(other.end) > 0) ? end : other.end; return Range(newStart, newEnd); } /// Subtracts one range from another, resulting in a new range. /// If the ranges do not overlap, returns the original range. Range<T>? intersection(Range<T> other) { if (other.end.compareTo(start) < 0 || other.start.compareTo(end) > 0) { return this; } final newStart = (start.compareTo(other.start) > 0) ? start : other.start; final newEnd = (end.compareTo(other.end) < 0) ? end : other.end; return (newStart.compareTo(newEnd) <= 0) ? Range(newStart, newEnd) : null; }packages/oui/lib/src/core/metadata.dart (1)
20-24: 🧹 Nitpick (assertive)
Update constructor documentation to include
tagsparameterThe constructor documentation mentions
name,icon, andattributesbut omits thetagsparameter. To maintain accurate and helpful documentation, please includetagsin the description.Apply this diff to update the documentation:
/// Creates an instance of [Metadata]. /// /// The [name] parameter must be provided and cannot be null. /// The [icon] parameter defaults to a localized null value if not provided. + /// The [tags] parameter defaults to an empty localized list if not provided. /// The [attributes] parameter defaults to an empty localized map if not provided.📝 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./// Creates an instance of [Metadata]. /// /// The [name] parameter must be provided and cannot be null. /// The [icon] parameter defaults to a localized null value if not provided. /// The [tags] parameter defaults to an empty localized list if not provided. /// The [attributes] parameter defaults to an empty localized map if not provided.packages/oui/lib/src/core/localization.dart (1)
50-52: 🧹 Nitpick (assertive)
Make
fromUiLocalea static methodThe
fromUiLocalemethod does not depend on any instance members of the class. Making it a static method clarifies its utility and allows it to be used without requiring an instance ofAppLocale.Apply this diff:
- Locale fromUiLocale(widgets.Locale uiLocale) { + static AppLocale fromUiLocale(widgets.Locale uiLocale) { - return Locale(uiLocale.languageCode, uiLocale.countryCode); + return AppLocale(uiLocale.languageCode, uiLocale.countryCode); }Committable suggestion skipped: line range outside the PR's diff.
packages/oui/lib/src/core/context.dart (1)
6-38: 🛠️ Refactor suggestion
Rename the
Stateenum to avoid conflict with Flutter'sStateclass.The name
Stateis a core class in Flutter, representing the state of aStatefulWidget. Using the same name for your enum can lead to confusion and potential conflicts. Consider renaming the enum to something likeComponentStateTypeorComponentStatusto improve clarity.packages/oui/lib/src/components/box.dart (2)
120-140: 🧹 Nitpick (assertive)
Clarify behavior when both
contentandbuilderare provided inContentModifier.Currently, if both
contentandbuilderare provided,buildertakes precedence andcontentis ignored. This might lead to confusion for users of the class. Consider adding assertion checks to ensure that only one ofcontentorbuilderis provided, or update the documentation to explicitly state the precedence.
68-75:
⚠️ Potential issueCorrect the sorting logic in
sortModifiersto handle unlisted modifiers properly.In the
sortModifiersmethod, when a modifier's type is not found in_modifierOrder, the index is set to1. This can incorrectly prioritize unlisted modifiers, placing them ahead of those explicitly ordered. To ensure that unlisted modifiers are placed after the listed ones, consider assigning them a high index value.Apply this diff to fix the sorting logic:
- return (aIndex == -1 ? 1 : aIndex).compareTo(bIndex == -1 ? 1 : bIndex); + final maxIndex = _modifierOrder.length; + final adjustedAIndex = aIndex == -1 ? maxIndex : aIndex; + final adjustedBIndex = bIndex == -1 ? maxIndex : bIndex; + return adjustedAIndex.compareTo(adjustedBIndex);📝 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.modifiers.sort((a, b) { final aIndex = _modifierOrder.indexOf(a.runtimeType); final bIndex = _modifierOrder.indexOf(b.runtimeType); final maxIndex = _modifierOrder.length; final adjustedAIndex = aIndex == -1 ? maxIndex : aIndex; final adjustedBIndex = bIndex == -1 ? maxIndex : bIndex; return adjustedAIndex.compareTo(adjustedBIndex); }); return modifiers; }packages/oui/lib/src/core/component.dart (1)
82-94:
⚠️ Potential issueRefine the sorting logic in
sortModifiersto ensure consistent modifier order.The current implementation may not sort unlisted modifiers as intended. Additionally, the special handling of
SizeModifiercould lead to inconsistent results. To address this, adjust the sorting logic to assign unlisted modifiers a higher index and ensure thatSizeModifieris always placed last.Apply this diff to refine the sorting logic:
- if (a is SizeModifier) return 1; - if (b is SizeModifier) return -1; + if (a is SizeModifier && b is SizeModifier) return 0; + if (a is SizeModifier) return 1; + if (b is SizeModifier) return -1; - return (aIndex == -1 ? 1 : aIndex).compareTo(bIndex == -1 ? 1 : bIndex); + final maxIndex = _defaultModifierTypeOrder.length; + final adjustedAIndex = aIndex == -1 ? maxIndex : aIndex; + final adjustedBIndex = bIndex == -1 ? maxIndex : bIndex; + return adjustedAIndex.compareTo(adjustedBIndex);📝 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.modifiers.sort((a, b) { final aIndex = _defaultModifierTypeOrder.indexOf(a.runtimeType); final bIndex = _defaultModifierTypeOrder.indexOf(b.runtimeType); // Ensure SizeModifier is always last if (a is SizeModifier && b is SizeModifier) return 0; if (a is SizeModifier) return 1; if (b is SizeModifier) return -1; final maxIndex = _defaultModifierTypeOrder.length; final adjustedAIndex = aIndex == -1 ? maxIndex : aIndex; final adjustedBIndex = bIndex == -1 ? maxIndex : bIndex; return adjustedAIndex.compareTo(adjustedBIndex); }); return modifiers; }packages/oui/lib/src/core/app.dart (4)
80-82:
⚠️ Potential issueUndefined variables in
colorsgetterIn the
colorsgetter withinAppContextExtension, the variablestheme,boxLevel, andstateare referenced but not defined in this scope. Ensure that these variables are available or passed appropriately to avoidUndefined nameerrors.Consider modifying the getter to include the necessary context or parameters:
BoxColors get colors { final currentTheme = ...; // Obtain the current theme final currentBoxLevel = ...; // Obtain the current box level final currentState = ...; // Obtain the current state return palette.levels.get(currentTheme).get(currentBoxLevel).get(currentState); }Or, if these values are intended to be accessed from elsewhere, ensure they are properly defined and accessible.
119-120: 🧹 Nitpick (assertive)
Remove unused
themegetter in_DynamicAppContextProviderStateThe
themegetter in_DynamicAppContextProviderStateis defined but not used. Consider removing it to clean up the code.Apply this change to remove the unused getter:
-class _DynamicAppContextProviderState extends widgets.State<DynamicAppContextProvider> { - DynamicTheme get theme => DynamicTheme.light; - - @override - Widget build(BuildContext context) { - // ... - } +class _DynamicAppContextProviderState extends widgets.State<DynamicAppContextProvider> { + @override + Widget build(BuildContext context) { + // ... + }Committable suggestion skipped: line range outside the PR's diff.
51-54: 🛠️ Refactor suggestion
Include
colorPaletteinupdateShouldNotifycomparisonIn the
updateShouldNotifymethod ofStaticAppContext, onlyrouterandconfigare compared. ShouldcolorPalettealso be included to ensure that changes to the color palette trigger widget tree updates?Apply this change to include
colorPalettein the comparison:@override bool updateShouldNotify(StaticAppContext oldWidget) { return router != oldWidget.router || config != oldWidget.config || + colorPalette != oldWidget.colorPalette; }📝 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.@override bool updateShouldNotify(StaticAppContext oldWidget) { return router != oldWidget.router || config != oldWidget.config || colorPalette != oldWidget.colorPalette; }
12-12: 🧹 Nitpick (assertive)
Simplify import of
Statewithout aliasingImporting
Stateaswidgets.Stateis unconventional since there are no naming conflicts. Consider importingStatedirectly to improve readability.Modify the import statement:
-import 'package:flutter/widgets.dart' as widgets show State; +import 'package:flutter/widgets.dart' show State;Then, update the usage in your code:
-widgets.State<DynamicAppContextProvider> createState() => +State<DynamicAppContextProvider> createState() =>Committable suggestion skipped: line range outside the PR's diff.
packages/oui/lib/src/core/background.dart (2)
65-71:
⚠️ Potential issue
decoratemethod does not considercustombackgroundIn the
Backgroundclass, thedecoratemethod returnsnullifcolor,gradient, andimageare allnull, even ifcustomis notnull. This means that when a custom background is set, the decoration is not applied. Is this intentional?If you want the
decoratemethod to handlecustombackgrounds, consider modifying it accordingly or documenting whycustomis excluded.
195-211: 🛠️ Refactor suggestion
Unexpected default background color when
backgroundis nullIn
BackgroundModifier, whenbackgroundisnull, a default color is applied usingcontext.colors.surface.normal. This might not be expected, as absence of a background could imply that no background should be applied. Consider whether applying a default background should be the default behavior.If you want to apply a background only when specified, modify the
decoratemethod:if (background == null) { - final color = context.colors.surface.normal; - return Background.color(color).decorate( - decoration, - context.buildContext, - ); + return decoration; }📝 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.Decoration? decorate( Decoration decoration, ComponentContext context, ) { if (background == null) { return decoration; } return background?.decorate( decoration, context.buildContext, ); }packages/oui/lib/src/core/scaffold.dart (2)
314-316:
⚠️ Potential issueUse
Colors.blackinstead ofColor.blackIn the
ScaffoldDividerclass,Color.blackis used, butColordoes not have a static member namedblack. Instead, useColors.blackfrom theColorsclass.Apply this correction:
const BackgroundModifier( - Background.color(Color.black), + Background.color(Colors.black), ),Ensure you have imported the
materialorpaintingpackage:import 'package:flutter/material.dart'; // or import 'package:flutter/painting.dart';
143-146:
⚠️ Potential issueIncorrect use of iterator to check for empty
panelslistIn the
_buildLayoutmethod,!panels.iterator.moveNext()is used to check if thepanelslist is empty. This approach advances the iterator and can lead to unintended side effects.Replace with
panels.isEmptyto check if the list is empty without affecting the iterator:bool canAddPanel(Screen screen) { - if (!panels.iterator.moveNext()) { + if (panels.isEmpty) { return true; } // ... }📝 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.bool canAddPanel(Screen screen) { if (panels.isEmpty) { return true; }packages/oui/lib/src/core/text.dart (1)
320-347:
⚠️ Potential issueReconsider Extending
ui.Textwith acopyWithMethodDefining an extension method
copyWithonui.Textmay lead to unexpected behavior becauseui.Textis a widget and typically immutable. Modifying widgets in this way is unconventional and could cause issues with widget rebuilding and the Flutter rendering pipeline.Consider creating a custom widget or a helper method that encapsulates the desired functionality without altering the existing
Textwidget.packages/oui/lib/src/core/routing.dart (1)
402-432: 🛠️ Refactor suggestion
Avoid Class Name Conflict with Flutter's
RouterNaming your class
Routermay conflict with Flutter's built-inRouterwidget, leading to confusion and potential namespace issues. It's advisable to rename your class to prevent any conflicts.Apply this diff to rename the class and its constructor:
-class Router extends RouterDelegate<PathMatch> with ChangeNotifier { +class AppRouter extends RouterDelegate<PathMatch> with ChangeNotifier { PathMatch _activeMatch = PathMatch.noMatch; PathMatch get match => _activeMatch; - Router(); + AppRouter(); @override PathMatch? get currentConfiguration => _activeMatch;Ensure to update all references to this class throughout your codebase.
📝 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.class AppRouter extends RouterDelegate<PathMatch> with ChangeNotifier { PathMatch _activeMatch = PathMatch.noMatch; PathMatch get match => _activeMatch; AppRouter(); @override PathMatch? get currentConfiguration => _activeMatch; @override Future<void> setNewRoutePath(PathMatch configuration) { _activeMatch = configuration; notifyListeners(); return SynchronousFuture(null); } @override Widget build(BuildContext context) { return Scaffold(_activeMatch); } @override Future<bool> popRoute() { final willPop = _activeMatch.canPop; if (willPop) { _activeMatch = _activeMatch.pop(); notifyListeners(); } return SynchronousFuture(willPop); } }packages/oui/lib/src/core/geometry.dart (2)
485-530:
⚠️ Potential issueUndefined Superclass
Range<double>inRangedDimensionThe class
RangedDimensionextendsRange<double>, butRangeis not part of the Dart standard library or Flutter framework. This will result in a compilation error unlessRangeis defined elsewhere in your codebase.Define the
Rangeclass or modifyRangedDimensionto not rely on an undefined superclass. If your intention is to represent a range between two double values, consider defining aRangeclass like the following:class Range<T extends num> { final T start; final T end; const Range(this.start, this.end); bool get isUnbounded => start == double.negativeInfinity && end == double.infinity; bool get isFixed => start == end; }Alternatively, if you only need the functionality within
RangedDimension, embed the necessary properties and methods directly within theRangedDimensionclass.
220-232: 🧹 Nitpick (assertive)
Enhance Readability by Replacing Nested Ternary Operators
The
uiAlignmentgetter in theAlignmentclass uses nested ternary operators, which can reduce code readability. Refactoring this logic using switch statements improves clarity.Apply this diff to refactor the code:
rendering.AlignmentGeometry get uiAlignment { - return rendering.Alignment( - main == Position.leading - ? -1 - : main == Position.middle - ? 0 - : 1, - cross == Position.leading - ? -1 - : cross == Position.middle - ? 0 - : 1, - ); + double mainValue; + switch (main) { + case Position.leading: + mainValue = -1; + break; + case Position.middle: + mainValue = 0; + break; + case Position.trailing: + mainValue = 1; + break; + } + + double crossValue; + switch (cross) { + case Position.leading: + crossValue = -1; + break; + case Position.middle: + crossValue = 0; + break; + case Position.trailing: + crossValue = 1; + break; + } + return rendering.Alignment(mainValue, crossValue); }📝 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.rendering.AlignmentGeometry get uiAlignment { double mainValue; switch (main) { case Position.leading: mainValue = -1; break; case Position.middle: mainValue = 0; break; case Position.trailing: mainValue = 1; break; } double crossValue; switch (cross) { case Position.leading: crossValue = -1; break; case Position.middle: crossValue = 0; break; case Position.trailing: crossValue = 1; break; } return rendering.Alignment(mainValue, crossValue); }packages/oui/lib/src/core/corners.dart (3)
229-248: 🧹 Nitpick (assertive)
Handle
BorderStyle.nonecase appropriately inpaintmethodIn the
paintmethod ofCornerBorder, whenside.styleisBorderStyle.none, the method currently breaks out of the switch statement without any action. Consider explicitly handling this case to ensure no border is painted and to make the code more readable.Apply this diff to handle
BorderStyle.noneexplicitly:switch (side.style) { case BorderStyle.none: + // No border to paint + return; case BorderStyle.solid: final adjustedRect = _adjustRect(rect, side.width, borderAlign); // ... rest of the code ...📝 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.void paint(Canvas canvas, Rect rect, {TextDirection? textDirection}) { if (rect.isEmpty) { return; } switch (side.style) { case BorderStyle.none: // No border to paint return; case BorderStyle.solid: final adjustedRect = _adjustRect(rect, side.width, borderAlign); final adjustedBorderRadius = _adjustRadius(borderRadius, side.width, borderAlign); final outerPath = _createPath( adjustedRect, adjustedBorderRadius, textDirection: textDirection, ); canvas.drawPath(outerPath, side.toPaint()); break; } }
970-985: 🧹 Nitpick (assertive)
Optimize
toStringmethod inCornerBorderRadiusIn the
toStringmethod, when all corners are equal, you are listing each corner individually. Consider simplifying the output by usingCornerBorderRadius.allfor better readability.Apply this diff:
return 'CornerBorderRadius(' - 'topLeft: $_topLeft,' - 'topRight: $_topRight,' - 'bottomLeft: $_bottomLeft,' - 'bottomRight: $_bottomRight,' - ')'; + 'all: $_topLeft' + ')';Committable suggestion skipped: line range outside the PR's diff.
36-40: 🛠️ Refactor suggestion
Use the
requiredkeyword for non-nullable parameterchildThe constructor for
ClipCornerRectexpectschildto be non-null, as indicated in the documentation, but it is not marked as required and is nullable. This could lead to unintended null values and runtime errors.Apply this diff to enforce non-nullability:
class ClipCornerRect extends StatelessWidget { /// Creates a widget that clips its child using a custom corner border shape. /// - /// The [child] parameter must not be null. + /// The [child] parameter is required and must not be null. /// The [radius] parameter defaults to [CornerBorderRadius.zero]. /// The [clipBehavior] parameter defaults to [Clip.antiAlias]. const ClipCornerRect({ super.key, - this.child, + required this.child, this.radius = CornerBorderRadius.zero, this.clipBehavior = Clip.antiAlias, });Also, update the
childproperty:- final Widget? child; + final Widget child;Committable suggestion skipped: line range outside the PR's diff.
packages/oui/lib/src/core/colors.dart (2)
966-971: 🧹 Nitpick (assertive)
Adjust hue interpolation to account for shortest path
In the
lerpmethod ofHsvColor, ensure that hue interpolation considers the shortest path around the color wheel. This prevents sudden jumps in hue values and results in smoother color transitions.Apply this diff:
return HsvColor._( - const HueInterpolator().resolve(a.hue, b.hue, t), + const HueInterpolator(shortestPath: true).resolve(a.hue, b.hue, t), const DoubleInterpolator().resolve(a.saturation, b.saturation, t), const DoubleInterpolator().resolve(a.value, b.value, t), );📝 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.return HsvColor._( const HueInterpolator(shortestPath: true).resolve(a.hue, b.hue, t), const DoubleInterpolator().resolve(a.saturation, b.saturation, t), const DoubleInterpolator().resolve(a.value, b.value, t), ); }
90-122: 🧹 Nitpick (assertive)
Extend hex string parsing to support shorthand formats
The
Color.fromHexfactory currently only supports 6 or 8-character hex codes. Consider adding support for 3 or 4-character shorthand hex codes (e.g.,#FFF,#0F0F).Apply this diff to enhance hex parsing:
// Remove the leading '#' if present if (hex.startsWith('#')) { hex = hex.substring(1); } + // Handle shorthand hex codes + if (hex.length == 3) { + hex = '${hex[0]}${hex[0]}${hex[1]}${hex[1]}${hex[2]}${hex[2]}'; + hex = 'FF$hex'; // Default alpha value + } else if (hex.length == 4) { + final alphaHex = '${hex[0]}${hex[0]}'; + hex = '${hex[1]}${hex[1]}${hex[2]}${hex[2]}${hex[3]}${hex[3]}'; + hex = '$alphaHex$hex'; + } else if (hex.length == 6) { hex = 'FF$hex'; // Add default alpha value if not provided } else if (hex.length != 8) { throw ArgumentError('Invalid hex string: $hex'); }📝 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.factory Color.fromHex( String hex, [ double? alpha, RgbColorSpace colorSpace = RgbColorSpace.sRGB, ]) { // Remove the leading '#' if present if (hex.startsWith('#')) { hex = hex.substring(1); } // Handle shorthand hex codes if (hex.length == 3) { hex = '${hex[0]}${hex[0]}${hex[1]}${hex[1]}${hex[2]}${hex[2]}'; hex = 'FF$hex'; // Default alpha value } else if (hex.length == 4) { final alphaHex = '${hex[0]}${hex[0]}'; hex = '${hex[1]}${hex[1]}${hex[2]}${hex[2]}${hex[3]}${hex[3]}'; hex = '$alphaHex$hex'; } else if (hex.length == 6) { hex = 'FF$hex'; // Add default alpha value if not provided } else if (hex.length != 8) { throw ArgumentError('Invalid hex string: $hex'); } // Parse the hex string int value = int.parse(hex, radix: 16); int hexAlpha = (value & 0xFF000000) >> 24; int red = (value & 0x00FF0000) >> 16; int green = (value & 0x0000FF00) >> 8; int blue = value & 0x000000FF; // Create and return the Color object return Color.fromRGB( red / 255, green / 255, blue / 255, alpha ?? hexAlpha / 255, colorSpace, ); }packages/oui_showcase/lib/screens/another_sub_screen.dart (1)
4-10: 🧹 Nitpick (assertive)
Add type annotation for
anotherSubScreenThe
anotherSubScreenvariable lacks an explicit type annotation. While Dart can infer the type, adding the type enhances readability and maintainability.Apply this diff:
-final anotherSubScreen = Screen( +final Screen anotherSubScreen = Screen( "another", ).content( Center( child: Text("Another Sub Screen"), ), );📝 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 Screen anotherSubScreen = Screen( "another", ).content( Center( child: Text("Another Sub Screen"), ), );packages/oui/test/src/core/screen/screen_testing_utils.dart (1)
4-12: 🧹 Nitpick (assertive)
Add documentation for the utility function.
The implementation looks good and follows the builder pattern consistently. Consider adding documentation comments to describe the purpose and usage of this test utility function.
+/// Creates a test screen with the specified ID and optional configuration. +/// +/// Parameters: +/// - [id]: Unique identifier for the screen +/// - [segments]: Optional path segments for routing (defaults to [id] as static segment) +/// - [children]: Optional child screens (defaults to empty list) Screen testScreen( String id, { List<PathSegment> segments = const [], List<Screen> children = const [], }) {📝 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./// Creates a test screen with the specified ID and optional configuration. /// /// Parameters: /// - [id]: Unique identifier for the screen /// - [segments]: Optional path segments for routing (defaults to [id] as static segment) /// - [children]: Optional child screens (defaults to empty list) Screen testScreen( String id, { List<PathSegment> segments = const [], List<Screen> children = const [], }) { return Screen(id) .pathSegments(segments.isEmpty ? [PathSegment.static(id)] : segments) .children(children); }packages/oui_showcase/lib/screens/sub_screen.dart (1)
5-15: 🧹 Nitpick (assertive)
Consider extracting configuration values and improving text handling.
A few suggestions to improve maintainability and internationalization:
- Extract magic numbers (40, 0) to named constants
- Use localized strings for text content
- Consider adding proper text styling
+const double kSubScreenCornerRadius = 40.0; +const double kSubScreenCornerSmoothing = 0.0; + final subScreen = Screen( "sub", ) .content( Center( child: Text("Sub Screen"), + child: Text( + LocalizedString.always("Sub Screen"), + style: TextStyle(fontSize: 16), + ), ), ) .child(anotherSubScreen) .backgroundColor(Color.black) - .allCorners(40, smoothing: 0); + .allCorners(kSubScreenCornerRadius, smoothing: kSubScreenCornerSmoothing);Committable suggestion skipped: line range outside the PR's diff.
packages/oui_showcase/lib/screens/test_screen.dart (1)
5-21: 🧹 Nitpick (assertive)
Extract configuration values and consider breaking down complex builders.
The screen configuration contains multiple magic numbers and a long chain of builders. Consider:
- Extracting dimensions and styling values to named constants
- Breaking down the box configuration into logical groups
- Adding comments to explain the purpose of different styling groups
+// Screen configuration constants +const double kMaxScreenWidth = 800.0; +const double kScreenCornerRadius = 18.0; +const double kBoxWidth = 300.0; +const double kBoxHeight = 600.0; +const double kBoxCornerRadius = 12.0; +const double kBoxInset = 18.0; + +// Box styling configuration +final boxStyle = Box() + .background() + .border() + .alignment(Alignment.center) + .shadow(spread: -5, blur: 15) + .allCorners(kBoxCornerRadius); + final testScreen = Screen("test") .path("testing") .child(subScreen) .content( - Box() - .background() - .border() - .alignment(Alignment.center) - .shadow(spread: -5, blur: 15) - .allCorners(12) + boxStyle .content(Label("Testing")) - .inset(Insets.all(18)) - .fixedSize(width: 300, height: 600), + .inset(Insets.all(kBoxInset)) + .fixedSize(width: kBoxWidth, height: kBoxHeight), ) .shadow(blur: 10) - .dynamicSize(maxWidth: 800) - .allCorners(18); + .dynamicSize(maxWidth: kMaxScreenWidth) + .allCorners(kScreenCornerRadius);📝 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.// Screen configuration constants const double kMaxScreenWidth = 800.0; const double kScreenCornerRadius = 18.0; const double kBoxWidth = 300.0; const double kBoxHeight = 600.0; const double kBoxCornerRadius = 12.0; const double kBoxInset = 18.0; // Box styling configuration final boxStyle = Box() .background() .border() .alignment(Alignment.center) .shadow(spread: -5, blur: 15) .allCorners(kBoxCornerRadius); final testScreen = Screen("test") .path("testing") .child(subScreen) .content( boxStyle .content(Label("Testing")) .inset(Insets.all(kBoxInset)) .fixedSize(width: kBoxWidth, height: kBoxHeight), ) .shadow(blur: 10) .dynamicSize(maxWidth: kMaxScreenWidth) .allCorners(kScreenCornerRadius);packages/oui_showcase/lib/main.dart (3)
6-9: 🧹 Nitpick (assertive)
Remove or document commented code.
The commented
usePathUrlStrategy()call should either be removed or documented explaining why it's commented out and when it should be uncommented.
14-14: 🧹 Nitpick (assertive)
Improve class documentation.
The comment "This widget is the root of your application" could be more descriptive.
- // This widget is the root of your application + /// Root widget of the Oui Showcase application. + /// Configures the application theme, routing, and initial screen.📝 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./// Root widget of the Oui Showcase application. /// Configures the application theme, routing, and initial screen.
19-29: 🧹 Nitpick (assertive)
Consider improving color configuration readability.
The color configuration using HSL values could be more descriptive:
config: Config( details: AppDetails( name: LocalizedString.always("Oui Showcase"), version: Version(0, 0, 0), ), colors: ColorConfig( - seed: Color.fromHSL( - HslColor.fromHSL(0.0, 0.2, 0.95), - ), + // Light gray color with slight warmth + seed: Color.fromHSL( + HslColor.fromHSL( + hue: 0.0, // Red hue + saturation: 0.2, // Low saturation for subtle warmth + lightness: 0.95, // Very light shade + ), + ), ), ),📝 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.config: Config( details: AppDetails( name: LocalizedString.always("Oui Showcase"), version: Version(0, 0, 0), ), colors: ColorConfig( // Light gray color with slight warmth seed: Color.fromHSL( HslColor.fromHSL( hue: 0.0, // Red hue saturation: 0.2, // Low saturation for subtle warmth lightness: 0.95, // Very light shade ), ), ), ),packages/oui/test/src/core/corners/corner_processed_radius_test.dart (1)
6-10: 🧹 Nitpick (assertive)
Add edge case tests for radius clamping.
While the current test verifies basic clamping, consider adding tests for:
- Zero dimensions
- Equal width and height
- Very large radius values
test('Constructor clamps radius with min(width, height)/2', () { const radius = CornerRadius(radius: 50, smoothing: 0.5); final processed = ProcessedCornerRadius(radius, width: 80, height: 60); expect(processed.cornerRadius, lessThanOrEqualTo(30)); + + // Test with zero dimensions + final zeroProcessed = ProcessedCornerRadius(radius, width: 0, height: 60); + expect(zeroProcessed.cornerRadius, equals(0)); + + // Test with equal dimensions + final equalProcessed = ProcessedCornerRadius(radius, width: 100, height: 100); + expect(equalProcessed.cornerRadius, equals(50)); + + // Test with very large radius + const largeRadius = CornerRadius(radius: 1000, smoothing: 0.5); + final largeProcessed = ProcessedCornerRadius(largeRadius, width: 80, height: 60); + expect(largeProcessed.cornerRadius, equals(30)); });📝 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.test('Constructor clamps radius with min(width, height)/2', () { const radius = CornerRadius(radius: 50, smoothing: 0.5); final processed = ProcessedCornerRadius(radius, width: 80, height: 60); expect(processed.cornerRadius, lessThanOrEqualTo(30)); // Test with zero dimensions final zeroProcessed = ProcessedCornerRadius(radius, width: 0, height: 60); expect(zeroProcessed.cornerRadius, equals(0)); // Test with equal dimensions final equalProcessed = ProcessedCornerRadius(radius, width: 100, height: 100); expect(equalProcessed.cornerRadius, equals(50)); // Test with very large radius const largeRadius = CornerRadius(radius: 1000, smoothing: 0.5); final largeProcessed = ProcessedCornerRadius(largeRadius, width: 80, height: 60); expect(largeProcessed.cornerRadius, equals(30)); });packages/oui/lib/src/components/label.dart (2)
40-44: 🧹 Nitpick (assertive)
Consider type-safe modifier handling.
The current implementation iterates through all modifiers and type-checks each one. Consider using a more type-safe approach by filtering modifiers upfront.
- for (final modifier in modifiers) { - if (modifier is LabelModifier) { - label = modifier.modify(label, context); - } - } + final labelModifiers = modifiers.whereType<LabelModifier>(); + for (final modifier in labelModifiers) { + label = modifier.modify(label, context); + }📝 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 labelModifiers = modifiers.whereType<LabelModifier>(); for (final modifier in labelModifiers) { label = modifier.modify(label, context); }
6-15: 🧹 Nitpick (assertive)
Add documentation for the Label class and its mixins.
The class lacks documentation explaining its purpose and usage. Consider adding class-level documentation describing the component's role and how the mixins affect text rendering.
+/// A text label component that supports various text modifications through mixins. +/// +/// This component allows customization of text properties such as: +/// * Maximum lines through [ModifiableMaxLines] +/// * Text alignment through [ModifiableTextAlign] +/// * Text wrapping through [ModifiableSoftWrap] +/// * And more through additional text modification mixins class Label extends Component<Label> with ModifiableMaxLines<Label>,📝 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./// A text label component that supports various text modifications through mixins. /// /// This component allows customization of text properties such as: /// * Maximum lines through [ModifiableMaxLines] /// * Text alignment through [ModifiableTextAlign] /// * Text wrapping through [ModifiableSoftWrap] /// * And more through additional text modification mixins class Label extends Component<Label> with ModifiableMaxLines<Label>, ModifiableTextAlign<Label>, ModifiableSoftWrap<Label>, ModifiableTextOverflow<Label>, ModifiableTextScaler<Label>, ModifiableTextWidthMode<Label>, ModifiableTextHeightBehavior<Label>, ModifiableSemanticsLabel<Label> {packages/oui/test/src/core/interpolation_test.dart (1)
5-12: 🧹 Nitpick (assertive)
Enhance LinearCurve test coverage.
Add tests for edge cases and invalid inputs:
group('LinearCurve', () { test('transform returns input value', () { const curve = LinearCurve(); expect(curve.transform(0.0), 0.0); expect(curve.transform(0.5), 0.5); expect(curve.transform(1.0), 1.0); + + // Test edge cases + expect(curve.transform(-1.0), -1.0); + expect(curve.transform(double.infinity), double.infinity); + expect(curve.transform(double.nan).isNaN, true); }); });📝 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.group('LinearCurve', () { test('transform returns input value', () { const curve = LinearCurve(); expect(curve.transform(0.0), 0.0); expect(curve.transform(0.5), 0.5); expect(curve.transform(1.0), 1.0); // Test edge cases expect(curve.transform(-1.0), -1.0); expect(curve.transform(double.infinity), double.infinity); expect(curve.transform(double.nan).isNaN, true); }); });packages/oui/lib/src/core/interpolation.dart (3)
69-71: 🧹 Nitpick (assertive)
Add input validation to DoubleInterpolator.
The resolve method should validate the interpolation parameter.
double resolve(double a, double b, double t) { + if (t.isNaN) return double.nan; + if (t == 0.0) return a; + if (t == 1.0) return b; return a + (b - a) * curve.transform(t); }📝 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.double resolve(double a, double b, double t) { if (t.isNaN) return double.nan; if (t == 0.0) return a; if (t == 1.0) return b; return a + (b - a) * curve.transform(t); }
25-30: 🧹 Nitpick (assertive)
Optimize CubicCurve calculations and add input validation.
The current implementation could benefit from input validation and performance optimization.
@override double transform(double t) { + if (t.isNaN) return double.nan; + if (t == 0.0) return p3; + if (t == 1.0) return p0 + p1 + p2 + p3; + final t2 = t * t; // Precompute t^2 final t3 = t2 * t; // Precompute t^3 return p0 * t3 + p1 * t2 + p2 * t + p3; }📝 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.@override double transform(double t) { if (t.isNaN) return double.nan; if (t == 0.0) return p3; if (t == 1.0) return p0 + p1 + p2 + p3; final t2 = t * t; // Precompute t^2 final t3 = t2 * t; // Precompute t^3 return p0 * t3 + p1 * t2 + p2 * t + p3; }
48-52: 🧹 Nitpick (assertive)
Add documentation for the Interpolable mixin.
The mixin lacks documentation explaining its purpose and contract.
+/// A mixin that provides linear interpolation capabilities to a type. +/// +/// Implementers must provide a [lerp] method that defines how to interpolate +/// between two values of type [T]. The mixin provides convenience methods +/// [lerpTo] and [lerpFrom] that use the implementing instance as one of the +/// interpolation endpoints. mixin Interpolable<T> { + /// Interpolates between values [a] and [b] using parameter [t]. + /// + /// The parameter [t] typically ranges from 0.0 to 1.0, where: + /// * t = 0.0 returns a value equivalent to [a] + /// * t = 1.0 returns a value equivalent to [b] + /// * t = 0.5 returns a value halfway between [a] and [b] T lerp(T a, T b, double t);📝 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./// A mixin that provides linear interpolation capabilities to a type. /// /// Implementers must provide a [lerp] method that defines how to interpolate /// between two values of type [T]. The mixin provides convenience methods /// [lerpTo] and [lerpFrom] that use the implementing instance as one of the /// interpolation endpoints. mixin Interpolable<T> { /// Interpolates between values [a] and [b] using parameter [t]. /// /// The parameter [t] typically ranges from 0.0 to 1.0, where: /// * t = 0.0 returns a value equivalent to [a] /// * t = 1.0 returns a value equivalent to [b] /// * t = 0.5 returns a value halfway between [a] and [b] T lerp(T a, T b, double t); T lerpTo(T b, double t) => lerp(this as T, b, t); T lerpFrom(T a, double t) => lerp(a, this as T, t); }packages/oui/lib/src/core/config.dart (1)
52-64: 🧹 Nitpick (assertive)
Consider adding documentation for the levels parameter.
While the implementation is clean, it would be helpful to document what the
levelsparameter represents and how it affects the color palette generation.class ColorConfig { final Color seed; + /// The number of color variations to generate in the palette. + /// A higher number will result in more granular color steps. final int levels;📝 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.class ColorConfig { final Color seed; /// The number of color variations to generate in the palette. /// A higher number will result in more granular color steps. final int levels; const ColorConfig({ this.seed = Color.white, this.levels = 6, }); ColorPalette get palette { return ColorPalette.generate(this); } }packages/oui/lib/src/core/typography.dart (1)
4-18: 🧹 Nitpick (assertive)
Add validation for min/max values.
Consider adding validation to ensure that minimum values are less than maximum values.
const TypographyConfigGroup({ this.curve = Curve.linear, this.minimumSize = 12.0, this.maximumSize = 24.0, this.minimumWeight = 300.0, this.maximumWeight = 500.0, -}); +}) : assert(minimumSize <= maximumSize), + assert(minimumWeight <= maximumWeight);📝 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.class TypographyConfigGroup { final Curve curve; final double minimumSize; final double maximumSize; final double minimumWeight; final double maximumWeight; const TypographyConfigGroup({ this.curve = Curve.linear, this.minimumSize = 12.0, this.maximumSize = 24.0, this.minimumWeight = 300.0, this.maximumWeight = 500.0, }) : assert(minimumSize <= maximumSize), assert(minimumWeight <= maximumWeight); }packages/oui/test/src/core/geometry/size_test.dart (1)
4-102: 🧹 Nitpick (assertive)
Consider adding more edge cases to the test suite.
While the current test coverage is good, consider adding tests for:
- Negative dimensions
- NaN/Infinity handling
- Maximum value constraints
Example test cases:
test('handles negative dimensions correctly', () { final size = Size.fixed(width: -100, height: -200); expect(size.width.start, 0); expect(size.height.start, 0); }); test('handles NaN and Infinity correctly', () { final size = Size.fixed(width: double.nan, height: double.infinity); expect(size.width.start.isFinite, true); expect(size.height.end.isFinite, false); });packages/oui/test/src/core/corners/corner_clip_test.dart (2)
75-91:
⚠️ Potential issueFix failing golden test.
The golden test is failing with a 0.09% pixel difference. This could be due to:
- Platform-specific rendering differences
- Different Flutter versions
- Different screen DPI settings
Consider:
- Regenerating the golden file on the CI environment
- Adding platform-specific golden files
- Using a more tolerant comparison threshold
🧰 Tools
🪛 GitHub Actions: Oui! Tests
[error] 87-87: Golden test 'clip_corner_rect.png' failed with 0.09% pixel difference (3724px diff detected). Failure feedback available at /home/runner/work/oui/oui/packages/oui/test/src/core/corners/failures
93-112: 🧹 Nitpick (assertive)
Improve performance test robustness.
While testing with 1000 containers is good, consider:
- Making the count configurable for different test environments
- Adding frame timing assertions
- Testing scrolling performance
+const int kTestItemCount = 1000; + testWidgets('handles large child trees efficiently', (tester) async { + await tester.binding.setSurfaceSize(const Size(400, 600)); await tester.pumpWidget( Directionality( textDirection: TextDirection.ltr, child: ClipCornerRect( radius: CornerBorderRadius.zero, child: SingleChildScrollView( child: Column( children: List.generate( - 1000, + kTestItemCount, (index) => Container(height: 1, color: const Color(0xFF0000FF)), ), ), ), ), ), ); - expect(find.byType(Container), findsNWidgets(1000)); + expect(find.byType(Container), findsNWidgets(kTestItemCount)); + + // Test scrolling performance + final stopwatch = Stopwatch()..start(); + await tester.fling(find.byType(SingleChildScrollView), const Offset(0, -500), 1000); + await tester.pumpAndSettle(); + stopwatch.stop(); + expect(stopwatch.elapsedMilliseconds, lessThan(500)); // Should complete within 500ms📝 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.const int kTestItemCount = 1000; testWidgets('handles large child trees efficiently', (tester) async { await tester.binding.setSurfaceSize(const Size(400, 600)); await tester.pumpWidget( Directionality( textDirection: TextDirection.ltr, child: ClipCornerRect( radius: CornerBorderRadius.zero, child: SingleChildScrollView( child: Column( children: List.generate( kTestItemCount, (index) => Container(height: 1, color: const Color(0xFF0000FF)), ), ), ), ), ), ); expect(find.byType(Container), findsNWidgets(kTestItemCount)); // Test scrolling performance final stopwatch = Stopwatch()..start(); await tester.fling(find.byType(SingleChildScrollView), const Offset(0, -500), 1000); await tester.pumpAndSettle(); stopwatch.stop(); expect(stopwatch.elapsedMilliseconds, lessThan(500)); // Should complete within 500mspackages/oui/lib/src/components/aligner.dart (2)
16-40:
⚠️ Potential issueRemove unused parameter
scrollableThe
scrollableparameter is declared but never used in the implementation. Either implement the scrolling functionality or remove the parameter.class Aligner extends StatelessWidget { final List<Widget> children; final Alignment alignment; final FlowDirection flowDirection; - final bool scrollable; const Aligner({ super.key, required this.children, this.alignment = Alignment.center, this.flowDirection = FlowDirection.leftToRight, - this.scrollable = false, });📝 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.class Aligner extends StatelessWidget { final List<Widget> children; final Alignment alignment; final FlowDirection flowDirection; const Aligner({ super.key, required this.children, this.alignment = Alignment.center, this.flowDirection = FlowDirection.leftToRight, }); @override Widget build(BuildContext context) { return Align( alignment: alignment.uiAlignment, child: Flow( delegate: AlignerDelegate(flowDirection), children: children, ), ); } }
42-119: 🧹 Nitpick (assertive)
⚠️ Potential issueRemove unused parameter
alignmentfrom AlignerDelegateThe
alignmentparameter is declared but never used in the implementation. This could lead to confusion as users might expect it to affect the layout.class AlignerDelegate extends FlowDelegate { final FlowDirection flowDirection; - final Alignment alignment; final int maxLines; const AlignerDelegate( this.flowDirection, [ - this.alignment = Alignment.center, this.maxLines = 1, ]);Consider performance optimization for large lists
The
paintChildrenmethod iterates through all children sequentially. For large lists, this could impact performance. Consider implementing virtual scrolling or pagination for better performance with large datasets.📝 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.class AlignerDelegate extends FlowDelegate { final FlowDirection flowDirection; final int maxLines; const AlignerDelegate( this.flowDirection, [ this.maxLines = 1, ]); @override void paintChildren(FlowPaintingContext context) { double x = flowDirection.isReversed && flowDirection.isHorizontal ? context.size.width : 0.0; double y = flowDirection.isReversed && flowDirection.isVertical ? context.size.height : 0.0; double maxWidth = 0.0; double maxHeight = 0.0; int currentLine = 1; for (var i = 0; i < context.childCount; i++) { final childSize = context.getChildSize(i); if (childSize == null) continue; final overflow = flowDirection.isHorizontal ? x + childSize.width > context.size.width : y + childSize.height > context.size.height; if (overflow) { currentLine++; if (currentLine > maxLines) break; if (flowDirection.isHorizontal) { x = flowDirection.isReversed ? context.size.width : 0.0; y += maxHeight; maxHeight = 0.0; } else { y = flowDirection.isReversed ? context.size.height : 0.0; x += maxWidth; maxWidth = 0.0; } } context.paintChild( i, transform: Matrix4.translationValues( x - (flowDirection.isReversed && flowDirection.isHorizontal ? childSize.width : 0.0), y - (flowDirection.isReversed && flowDirection.isVertical ? childSize.height : 0.0), 0.0, ), ); if (flowDirection.isHorizontal) { x += flowDirection.isReversed ? -childSize.width : childSize.width; maxHeight = max(maxHeight, childSize.height); } else { y += flowDirection.isReversed ? -childSize.height : childSize.height; maxWidth = max(maxWidth, childSize.width); } } } @override bool shouldRepaint(covariant FlowDelegate oldDelegate) { return oldDelegate is! AlignerDelegate || oldDelegate.flowDirection != flowDirection || oldDelegate.maxLines != maxLines; } }packages/oui/test/src/core/corners/corner_border_test.dart (1)
59-71: 🛠️ Refactor suggestion
Strengthen test assertions for paint method
The current test only verifies that no errors occur during painting. Consider adding assertions to verify the actual painting outcome.
test('Paint method handles all alignments', () { const border = CornerBorder( side: BorderSide(thickness: 4, color: Color.black), borderRadius: CornerBorderRadius.all( CornerRadius(radius: 10, smoothing: 0.5), ), ); const rect = Rect.fromLTWH(0, 0, 100, 100); final recorder = PictureRecorder(); final canvas = Canvas(recorder, const Rect.fromLTWH(0, 0, 100, 100)); border.paint(canvas, rect, textDirection: TextDirection.ltr); - expect(true, isTrue); // No errors should occur + final picture = recorder.endRecording(); + expect(picture, isNotNull); + // Add assertions to verify the painted content + // For example, verify the bounds of the painted content + expect(picture.approximateBytesUsed, greaterThan(0)); });Committable suggestion skipped: line range outside the PR's diff.
packages/oui/lib/src/core/shadow.dart (1)
64-77: 🧹 Nitpick (assertive)
Rename
autoparameter for clarityThe boolean parameter
autois not descriptive enough. Consider renaming it to better reflect its purpose.class ShadowModifier extends ComponentModifier with DecorationModifier { - final bool auto; + final bool useContextColor; /// The shadow effect. final Shadow shadow; const ShadowModifier( - this.auto, + this.useContextColor, this.shadow, );📝 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./// A class representing a shadow effect that can be applied to a widget. class ShadowModifier extends ComponentModifier with DecorationModifier { final bool useContextColor; /// The shadow effect. final Shadow shadow; /// Creates a new [ShadowModifier] instance with the given properties. /// /// All parameters are required. const ShadowModifier( this.useContextColor, this.shadow, );packages/oui/test/src/core/colors/color_hsv_test.dart (1)
96-97: 🧹 Nitpick (assertive)
Standardize floating-point comparison tolerances
Different tolerance values are used for floating-point comparisons (0.001 vs 0.00001). Consider standardizing the tolerance value across all floating-point comparisons for consistency.
- expect(hsvColor.saturation, closeTo(0.666666, 0.001)); + expect(hsvColor.saturation, closeTo(0.666666, 0.00001)); expect(hsvColor.value, 0.75);Also applies to: 112-113
packages/oui/test/src/core/corners/corner_radius_test.dart (2)
51-56: 🧹 Nitpick (assertive)
Add tests for negative values in arithmetic operations.
Consider adding test cases for multiplication and division with negative scalars to ensure correct behavior with negative numbers.
test('Multiplication with negative scalar', () { const radius = CornerRadius(radius: 2, smoothing: 0.5); final result = radius * -2; expect(result.cornerRadius, -4); expect(result.smoothing, -1.0); });
113-118: 🧹 Nitpick (assertive)
Add tests for extreme values and overflow conditions.
Consider adding test cases for operations with very large numbers and potential overflow scenarios to ensure robust error handling.
test('Operations with extreme values', () { const radius = CornerRadius(radius: double.maxFinite, smoothing: 0.5); final result = radius * 2; expect(result.cornerRadius.isInfinite, true); });packages/oui/test/src/core/colors/color_test.dart (2)
110-125: 🧹 Nitpick (assertive)
Enhance contrast tests with WCAG accessibility guidelines.
The contrast tests should include verification against WCAG 2.1 accessibility guidelines for text readability. Consider adding test cases for minimum contrast ratios:
- 4.5:1 for normal text
- 3:1 for large text
- 7:1 for enhanced contrast
test('WCAG contrast guidelines', () { final textColor = Color.black; final backgroundColor = const Color.fromRGB(0.6, 0.6, 0.6); final contrast = textColor.contrastAgainst(backgroundColor); expect(contrast >= 4.5, true, reason: 'Minimum contrast for normal text'); });
89-103: 🧹 Nitpick (assertive)
Add tests for alpha channel operations.
Consider adding tests for alpha channel blending and transparency operations to ensure correct behavior with partially transparent colors.
test('alpha blending', () { final color1 = Color.fromRGB(1.0, 0.0, 0.0, 0.5); final color2 = Color.fromRGB(0.0, 1.0, 0.0, 0.5); final blended = color1.blend(color2); expect(blended.alpha, 0.75); });packages/oui/test/src/core/geometry/ranged_dimension_test.dart (1)
148-162: 🧹 Nitpick (assertive)
Add tests for floating-point precision and NaN handling.
Consider adding test cases for:
- Floating-point precision comparisons
- NaN handling in range operations
test('floating point precision', () { const dimension = RangedDimension.dynamic(minimum: 0.1 + 0.2, maximum: 0.3); expect(dimension.within(0.3), true); }); test('NaN handling', () { const dimension = RangedDimension.dynamic(minimum: 0, maximum: 10); expect(dimension.within(double.nan), false); expect(dimension.clamp(double.nan), 0); });packages/oui/test/src/core/router/path_match_test.dart (1)
20-21: 🛠️ Refactor suggestion
Add path command validations.
Multiple test cases are missing specific assertions to validate path commands. Consider adding detailed assertions to verify the exact behavior of the path operations.
For example, for the
rawSegmentstest:// Add assertions to validate path commands expect(pathMatch.rawSegments.length, 1); expect(pathMatch.rawSegments.first, matches(RegExp(r'^[0-9]+$')));Also applies to: 51-52, 67-68, 83-84, 99-100, 120-121, 136-137, 159-160, 175-176, 191-192, 207-208, 225-226, 241-242
packages/oui/test/src/core/router/path_test.dart (1)
68-77: 🧹 Nitpick (assertive)
Improve error message assertions.
The error assertions could be more specific by checking the error messages.
expect( () => PathSegment.argument('id', pattern: r'['), - throwsA(isA<FormatException>()), + throwsA(predicate((e) => + e is FormatException && + e.message.contains('Invalid pattern') + )), ); expect( () => PathSegment.static(''), - throwsA(isA<AssertionError>()), + throwsA(predicate((e) => + e is AssertionError && + e.message!.contains('id cannot be empty') + )), );📝 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.() => PathSegment.argument('id', pattern: r'['), throwsA(predicate((e) => e is FormatException && e.message.contains('Invalid pattern') )), ); }); test('static segment with empty string', () { expect( () => PathSegment.static(''), throwsA(predicate((e) => e is AssertionError && e.message!.contains('id cannot be empty') )), );packages/oui/test/src/core/colors/palette/box_colors_test.dart (2)
8-29: 🧹 Nitpick (assertive)
Optimize test data creation.
Multiple test cases create similar color instances. Consider extracting common test data to reduce code duplication and improve maintainability.
+// At the top of the test file +AccentableColor createTestColor(Color base, String accentHex) { + return AccentableColor( + base, + LeveledContainer<Color>([ + Color.fromHex(accentHex), + ]), + ); +} + +BoxColors createTestBoxColors(AccentableColor color) { + return BoxColors( + content: color, + surface: color, + decoration: color, + shadow: color, + edge: color, + placeholder: color, + ); +}Also applies to: 31-38, 44-72, 74-81, 87-108, 126-140, 142-149, 156-170, 183-190, 203-210, 231-239
1-277: 🛠️ Refactor suggestion
Add tests for color transformations and invalid values.
Consider adding tests for:
- Color transformations (e.g., darkening, lightening)
- Invalid hex color values
- Null handling in color containers
Example test cases to add:
test('should handle invalid hex colors', () { expect( () => Color.fromHex("#12345"), // Invalid length throwsA(isA<FormatException>()), ); }); test('should handle color transformations', () { final color = createTestColor(Color.black, "#123456"); final boxColors = createTestBoxColors(color); final transformed = boxColors.copyWith( content: boxColors.content.withOpacity(0.5), ); expect(transformed.content.opacity, equals(0.5)); });packages/oui/test/src/core/corners/corner_path_test.dart (3)
20-21: 🛠️ Refactor suggestion
Add specific path command validations.
Replace the "Add more assertions" comments with actual path command validations to ensure the corner paths are correctly constructed.
Example for the first test:
// Add assertions to validate path commands final metrics = path.computeMetrics(); expect(metrics.length, equals(1)); final firstMetric = metrics.first; expect(firstMetric.isClosed, isFalse); expect(firstMetric.length, isPositive); // Validate specific path points final tangent = firstMetric.getTangentForOffset(0); expect(tangent, isNotNull); expect(tangent!.position.dx, equals(100)); // Right edge expect(tangent.position.dy, equals(8)); // Radius from topAlso applies to: 51-52, 67-68, 83-84, 99-100, 120-121, 136-137, 159-160, 175-176, 191-192, 207-208, 225-226, 241-242
210-226: 🧹 Nitpick (assertive)
Enhance stress test coverage.
The stress test could be more comprehensive by testing different corner combinations and varying parameters.
test('addCorner stress test with many corners', () { final path = Path(); - final processed = ProcessedCornerRadius( - const CornerRadius(radius: 8, smoothing: 0.5), - width: 100, - height: 100, - ); - for (int i = 0; i < 100; i++) { + // Test with varying parameters + for (int i = 0; i < 100; i++) { + final processed = ProcessedCornerRadius( + CornerRadius(radius: i.toDouble(), smoothing: i / 100), + width: 100 + i, + height: 100 + i, + ); path.addCorner( - Corner.topRight, + Corner.values[i % Corner.values.length], processed, - const Rect.fromLTWH(0, 0, 100, 100), + Rect.fromLTWH(0, 0, 100.0 + i, 100.0 + i), ); } expect(path, isNotNull); - // Add more assertions to validate path commands + final metrics = path.computeMetrics(); + expect(metrics.length, equals(100)); });📝 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.test('addCorner stress test with many corners', () { final path = Path(); // Test with varying parameters for (int i = 0; i < 100; i++) { final processed = ProcessedCornerRadius( CornerRadius(radius: i.toDouble(), smoothing: i / 100), width: 100 + i, height: 100 + i, ); path.addCorner( Corner.values[i % Corner.values.length], processed, Rect.fromLTWH(0, 0, 100.0 + i, 100.0 + i), ); } expect(path, isNotNull); final metrics = path.computeMetrics(); expect(metrics.length, equals(100)); });
228-242: 🛠️ Refactor suggestion
Improve benchmark test implementation.
The benchmark test lacks actual performance measurements. Consider using proper benchmarking techniques.
-test('addCorner benchmark for large dimensions', () { +test('addCorner benchmark for large dimensions', () { + final stopwatch = Stopwatch()..start(); + final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 1000, smoothing: 0.5), width: 10000, height: 10000, ); + + // Measure multiple iterations + for (var i = 0; i < 100; i++) { path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 10000, 10000), ); + } + + stopwatch.stop(); expect(path, isNotNull); - // Add more assertions to validate path commands + expect(stopwatch.elapsedMilliseconds, lessThan(1000), + reason: 'Corner addition should complete within 1 second'); });📝 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.test('addCorner benchmark for large dimensions', () { final stopwatch = Stopwatch()..start(); final path = Path(); final processed = ProcessedCornerRadius( const CornerRadius(radius: 1000, smoothing: 0.5), width: 10000, height: 10000, ); // Measure multiple iterations for (var i = 0; i < 100; i++) { path.addCorner( Corner.topRight, processed, const Rect.fromLTWH(0, 0, 10000, 10000), ); } stopwatch.stop(); expect(path, isNotNull); expect(stopwatch.elapsedMilliseconds, lessThan(1000), reason: 'Corner addition should complete within 1 second'); });packages/oui/lib/src/core/border.dart (2)
217-218: 🧹 Nitpick (assertive)
Document the
autoparameter.The purpose of the
autoparameter isn't clear from the code. Please add documentation explaining its usage and impact.
97-98: 🧹 Nitpick (assertive)
Remove commented code.
The commented
lerpDoublealternative should be removed since theDoubleInterpolatorimplementation is being used.- // thickness: lerpDouble(a.thickness, b.thickness, t) thickness: const DoubleInterpolator().resolve(📝 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.thickness: const DoubleInterpolator().resolve(packages/oui/lib/src/core/_index.dart (2)
1-16: 🧹 Nitpick (assertive)
Consider documenting the architectural changes
This barrel file represents a significant architectural restructuring, consolidating core functionality into a single entry point. This change:
- Simplifies imports for consumers
- Creates a clear boundary for core functionality
- Provides better organization of fundamental building blocks
Consider adding documentation (either in this file or in README) to:
- Explain the purpose and scope of the core module
- Provide guidelines for what belongs in core vs. other modules
- Document any breaking changes for existing consumers
1-16: 💡 Codebase verification
Circular dependency detected in core modules 🔄
Found a circular dependency chain:
app.dartimportscontext.dartcontext.dartimportscomponent.dartcomponent.dartimportsapp.dartThis circular dependency should be resolved by reviewing the relationships between these modules and potentially extracting shared functionality into a separate module to break the cycle.
🔗 Analysis chain
Verify for potential circular dependencies
Let's check for potential circular dependencies between the exported modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential circular dependencies # Expected: No circular imports between core modules cd "$(dirname "$(fd -t f "packages/oui/lib/src/core/_index.dart")")" || exit 1 # For each Dart file, check if it imports other core files for file in *.dart; do if [ "$file" != "_index.dart" ]; then echo "Checking imports in $file:" rg "^import.*core/" "$file" || true fi doneLength of output: 277
Script:
#!/bin/bash # Check for circular dependencies in core modules # Find all Dart files in the core directory except _index.dart echo "Core Dart files and their imports:" for file in $(fd -e dart -E "_index.dart" . "packages/oui/lib/src/core/"); do echo -e "\nFile: $file" rg "^import.*core/" "$file" || true doneLength of output: 5424
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/oui/test/src/core/corners/corner_clip_test.dart(1 hunks)
🔇 Additional comments (1)
packages/oui/test/src/core/corners/corner_clip_test.dart (1)
1-15: Well-structured imports and test organization!The imports are clean, specific, and well-organized using the show clause. The test file follows Flutter testing best practices.
| testWidgets('respects clipBehavior', (tester) async { | ||
| for (var clipBehavior in Clip.values) { | ||
| await tester.pumpWidget( | ||
| Directionality( | ||
| textDirection: TextDirection.ltr, | ||
| child: ClipCornerRect( | ||
| radius: CornerBorderRadius.zero, | ||
| clipBehavior: clipBehavior, | ||
| child: Container(), | ||
| ), | ||
| ), | ||
| ); | ||
| expect(find.byType(ClipCornerRect), findsOneWidget); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Verify clip behavior application.
The test iterates through all clip behaviors but only verifies widget existence. Consider adding assertions to verify that each clip behavior is actually applied correctly.
Example enhancement:
testWidgets('respects clipBehavior', (tester) async {
for (var clipBehavior in Clip.values) {
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ClipCornerRect(
radius: CornerBorderRadius.zero,
clipBehavior: clipBehavior,
child: Container(),
),
),
);
expect(find.byType(ClipCornerRect), findsOneWidget);
+ final widget = tester.widget<ClipCornerRect>(find.byType(ClipCornerRect));
+ expect(widget.clipBehavior, equals(clipBehavior),
+ reason: 'ClipBehavior should match ${clipBehavior.name}');
}
});📝 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.
| testWidgets('respects clipBehavior', (tester) async { | |
| for (var clipBehavior in Clip.values) { | |
| await tester.pumpWidget( | |
| Directionality( | |
| textDirection: TextDirection.ltr, | |
| child: ClipCornerRect( | |
| radius: CornerBorderRadius.zero, | |
| clipBehavior: clipBehavior, | |
| child: Container(), | |
| ), | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| } | |
| }); | |
| testWidgets('respects clipBehavior', (tester) async { | |
| for (var clipBehavior in Clip.values) { | |
| await tester.pumpWidget( | |
| Directionality( | |
| textDirection: TextDirection.ltr, | |
| child: ClipCornerRect( | |
| radius: CornerBorderRadius.zero, | |
| clipBehavior: clipBehavior, | |
| child: Container(), | |
| ), | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| final widget = tester.widget<ClipCornerRect>(find.byType(ClipCornerRect)); | |
| expect(widget.clipBehavior, equals(clipBehavior), | |
| reason: 'ClipBehavior should match ${clipBehavior.name}'); | |
| } | |
| }); |
| testWidgets('handles large child trees efficiently', (tester) async { | ||
| await tester.pumpWidget( | ||
| Directionality( | ||
| textDirection: TextDirection.ltr, | ||
| child: ClipCornerRect( | ||
| radius: CornerBorderRadius.zero, | ||
| child: SingleChildScrollView( | ||
| child: Column( | ||
| children: List.generate( | ||
| 1000, | ||
| (index) => | ||
| Container(height: 1, color: const Color(0xFF0000FF)), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| expect(find.byType(Container), findsNWidgets(1000)); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance performance test with metrics.
While testing with 1000 widgets is good for stress testing, consider adding actual performance measurements:
- Frame build time
- Memory usage
- Frame drops during scroll
Example enhancement:
testWidgets('handles large child trees efficiently', (tester) async {
+ final stopwatch = Stopwatch()..start();
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ClipCornerRect(
radius: CornerBorderRadius.zero,
child: SingleChildScrollView(
child: Column(
children: List.generate(
1000,
(index) =>
Container(height: 1, color: const Color(0xFF0000FF)),
),
),
),
),
),
);
+ stopwatch.stop();
expect(find.byType(Container), findsNWidgets(1000));
+ expect(stopwatch.elapsedMilliseconds, lessThan(100),
+ reason: 'Widget tree should build quickly');
+
+ // Test smooth scrolling
+ await tester.fling(find.byType(SingleChildScrollView), Offset(0, -500), 1000);
+ await tester.pumpAndSettle();
+ expect(tester.hasRunningAnimations, isFalse);
});📝 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.
| testWidgets('handles large child trees efficiently', (tester) async { | |
| await tester.pumpWidget( | |
| Directionality( | |
| textDirection: TextDirection.ltr, | |
| child: ClipCornerRect( | |
| radius: CornerBorderRadius.zero, | |
| child: SingleChildScrollView( | |
| child: Column( | |
| children: List.generate( | |
| 1000, | |
| (index) => | |
| Container(height: 1, color: const Color(0xFF0000FF)), | |
| ), | |
| ), | |
| ), | |
| ), | |
| ), | |
| ); | |
| expect(find.byType(Container), findsNWidgets(1000)); | |
| }); | |
| testWidgets('handles large child trees efficiently', (tester) async { | |
| final stopwatch = Stopwatch()..start(); | |
| await tester.pumpWidget( | |
| Directionality( | |
| textDirection: TextDirection.ltr, | |
| child: ClipCornerRect( | |
| radius: CornerBorderRadius.zero, | |
| child: SingleChildScrollView( | |
| child: Column( | |
| children: List.generate( | |
| 1000, | |
| (index) => | |
| Container(height: 1, color: const Color(0xFF0000FF)), | |
| ), | |
| ), | |
| ), | |
| ), | |
| ), | |
| ); | |
| stopwatch.stop(); | |
| expect(find.byType(Container), findsNWidgets(1000)); | |
| expect(stopwatch.elapsedMilliseconds, lessThan(100), | |
| reason: 'Widget tree should build quickly'); | |
| // Test smooth scrolling | |
| await tester.fling(find.byType(SingleChildScrollView), Offset(0, -500), 1000); | |
| await tester.pumpAndSettle(); | |
| expect(tester.hasRunningAnimations, isFalse); | |
| }); |
| testWidgets('builds a clipped widget', (tester) async { | ||
| await tester.pumpWidget( | ||
| Directionality( | ||
| textDirection: TextDirection.ltr, | ||
| child: ClipCornerRect( | ||
| radius: CornerBorderRadius.zero, | ||
| child: Container(), | ||
| ), | ||
| ), | ||
| ); | ||
| expect(find.byType(ClipCornerRect), findsOneWidget); | ||
| }); | ||
|
|
||
| testWidgets('applies a non-zero radius', (tester) async { | ||
| await tester.pumpWidget( | ||
| const ClipCornerRect( | ||
| radius: CornerBorderRadius.all( | ||
| CornerRadius( | ||
| radius: 10, | ||
| smoothing: 1, | ||
| ), | ||
| ), | ||
| child: SizedBox(), | ||
| ), | ||
| ); | ||
| expect(find.byType(ClipCornerRect), findsOneWidget); | ||
| }); | ||
|
|
||
| testWidgets('handles null child', (tester) async { | ||
| await tester.pumpWidget( | ||
| const ClipCornerRect( | ||
| radius: CornerBorderRadius.all( | ||
| CornerRadius( | ||
| radius: 10, | ||
| smoothing: 1, | ||
| ), | ||
| ), | ||
| child: null, | ||
| ), | ||
| ); | ||
| expect(find.byType(ClipCornerRect), findsOneWidget); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance test coverage with additional assertions.
While the basic test structure is good, consider adding more specific assertions to verify the actual properties and behavior:
-
For the "builds a clipped widget" test:
- Verify the radius is actually zero
- Check if the container is properly clipped
-
For the "applies a non-zero radius" test:
- Verify the applied radius matches the expected value (10)
- Verify the smoothing value is applied correctly (1)
-
For the "handles null child" test:
- Verify the widget's size/constraints when child is null
Example enhancement for the non-zero radius test:
testWidgets('applies a non-zero radius', (tester) async {
+ const radius = 10.0;
+ const smoothing = 1.0;
await tester.pumpWidget(
const ClipCornerRect(
radius: CornerBorderRadius.all(
CornerRadius(
- radius: 10,
- smoothing: 1,
+ radius: radius,
+ smoothing: smoothing,
),
),
child: SizedBox(),
),
);
expect(find.byType(ClipCornerRect), findsOneWidget);
+ final widget = tester.widget<ClipCornerRect>(find.byType(ClipCornerRect));
+ expect(widget.radius.topLeft.radius, equals(radius));
+ expect(widget.radius.topLeft.smoothing, equals(smoothing));
});📝 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.
| testWidgets('builds a clipped widget', (tester) async { | |
| await tester.pumpWidget( | |
| Directionality( | |
| textDirection: TextDirection.ltr, | |
| child: ClipCornerRect( | |
| radius: CornerBorderRadius.zero, | |
| child: Container(), | |
| ), | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| }); | |
| testWidgets('applies a non-zero radius', (tester) async { | |
| await tester.pumpWidget( | |
| const ClipCornerRect( | |
| radius: CornerBorderRadius.all( | |
| CornerRadius( | |
| radius: 10, | |
| smoothing: 1, | |
| ), | |
| ), | |
| child: SizedBox(), | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| }); | |
| testWidgets('handles null child', (tester) async { | |
| await tester.pumpWidget( | |
| const ClipCornerRect( | |
| radius: CornerBorderRadius.all( | |
| CornerRadius( | |
| radius: 10, | |
| smoothing: 1, | |
| ), | |
| ), | |
| child: null, | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| }); | |
| testWidgets('builds a clipped widget', (tester) async { | |
| await tester.pumpWidget( | |
| Directionality( | |
| textDirection: TextDirection.ltr, | |
| child: ClipCornerRect( | |
| radius: CornerBorderRadius.zero, | |
| child: Container(), | |
| ), | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| }); | |
| testWidgets('applies a non-zero radius', (tester) async { | |
| const radius = 10.0; | |
| const smoothing = 1.0; | |
| await tester.pumpWidget( | |
| const ClipCornerRect( | |
| radius: CornerBorderRadius.all( | |
| CornerRadius( | |
| radius: radius, | |
| smoothing: smoothing, | |
| ), | |
| ), | |
| child: SizedBox(), | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| final widget = tester.widget<ClipCornerRect>(find.byType(ClipCornerRect)); | |
| expect(widget.radius.topLeft.radius, equals(radius)); | |
| expect(widget.radius.topLeft.smoothing, equals(smoothing)); | |
| }); | |
| testWidgets('handles null child', (tester) async { | |
| await tester.pumpWidget( | |
| const ClipCornerRect( | |
| radius: CornerBorderRadius.all( | |
| CornerRadius( | |
| radius: 10, | |
| smoothing: 1, | |
| ), | |
| ), | |
| child: null, | |
| ), | |
| ); | |
| expect(find.byType(ClipCornerRect), findsOneWidget); | |
| }); |
| // testWidgets('matches golden file', (tester) async { | ||
| // await tester.pumpWidget( | ||
| // ClipCornerRect( | ||
| // radius: const CornerBorderRadius.all( | ||
| // CornerRadius( | ||
| // radius: 100, | ||
| // smoothing: 1, | ||
| // ), | ||
| // ), | ||
| // child: Container(color: const Color(0xFF0000FF)), | ||
| // ), | ||
| // ); | ||
| // await expectLater( | ||
| // find.byType(ClipCornerRect), | ||
| // matchesGoldenFile('goldens/clip_corner_rect.png'), | ||
| // ); | ||
| // }); |
There was a problem hiding this comment.
💡 Codebase verification
Implement the commented golden test
The golden test should be implemented as it:
- Follows the established testing pattern in the corners module
- Has the required golden file already in place at
packages/oui/test/src/core/corners/goldens/clip_corner_rect.png - Provides valuable visual regression testing for corner clipping behavior
🔗 Analysis chain
Remove or implement the commented golden test.
The commented golden test should either be:
- Implemented if visual regression testing is needed
- Removed if not required
If you decide to keep it, ensure the golden file path is correct:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the golden file directory exists
fd -t d "goldens" packages/oui/test/src/core/corners/
Length of output: 97
Script:
#!/bin/bash
# Check if the golden file exists
fd "clip_corner_rect.png" packages/oui/test/src/core/corners/goldens/
# Look for other golden tests in the codebase
rg "matchesGoldenFile" -A 2 packages/oui/test/
Length of output: 717
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
Aligner,Box, andLabelcomponents for layout flexibility.Core Improvements
Localehandling.Performance Optimizations
Documentation
Breaking Changes
OuiApp,OuiAction, and various utility classes.