Component system refactor#40
Merged
Merged
Conversation
WalkthroughThis change introduces a new JSON Patch system, including patch operation classes, semantics, and templates, and integrates it into the component registry workflow. The registry now computes and stores patch write operations ( Changes
Sequence Diagram(s)Component Registration and Patch WriteOps GenerationsequenceDiagram
participant Client
participant DefaultComponentRegistry
participant ComponentDescription
participant PatchTemplate
Client->>DefaultComponentRegistry: prepareComponent(componentJson)
DefaultComponentRegistry->>DefaultComponentRegistry: _collectWriteOps(layoutRoot, [])
DefaultComponentRegistry->>PatchTemplate: (for each ref in layout) create PatchTemplate
DefaultComponentRegistry->>ComponentDescription: create with base data and writeOps
DefaultComponentRegistry-->>Client: (registration complete)
JSON Patch ApplicationsequenceDiagram
participant Caller
participant JsonPatchApplier
participant PatchOp
Caller->>JsonPatchApplier: apply(base, [PatchOp...], ensureIntermediate)
loop For each PatchOp
JsonPatchApplier->>JsonPatchApplier: _applyAdd/_applyReplace/_applyRemove
end
JsonPatchApplier-->>Caller: returns new (patched) Map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes This review involves new core logic (JSON Patch system), significant refactoring of the component registry, removal of legacy reference code, updates to exports/imports, and thorough new test suites. The changes are spread across multiple subsystems and require careful attention to integration and correctness. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
lib/src/action_api/event.dart(0 hunks)lib/src/misc/index.dart(1 hunks)lib/src/misc/json_patch/index.dart(1 hunks)lib/src/misc/json_patch/json_patch.dart(1 hunks)lib/src/misc/json_patch/semantics.dart(1 hunks)lib/src/misc/json_patch/template.dart(1 hunks)lib/src/registry_api/component_description.dart(0 hunks)lib/src/registry_api/components/component_description.dart(1 hunks)lib/src/registry_api/components/component_registry.dart(0 hunks)lib/src/registry_api/components/component_registry_impl.dart(2 hunks)lib/src/registry_api/components/index.dart(0 hunks)lib/src/registry_api/components/reference_target.dart(0 hunks)lib/src/registry_api/index.dart(1 hunks)lib/src/registry_api/model_factory.dart(0 hunks)pubspec.yaml(1 hunks)test/component_registry_writeops_test.dart(1 hunks)test/data_source_test.dart(1 hunks)test/json_patch_test.dart(1 hunks)
💤 Files with no reviewable changes (6)
- lib/src/action_api/event.dart
- lib/src/registry_api/components/index.dart
- lib/src/registry_api/components/reference_target.dart
- lib/src/registry_api/components/component_registry.dart
- lib/src/registry_api/model_factory.dart
- lib/src/registry_api/component_description.dart
🔇 Additional comments (3)
lib/src/registry_api/index.dart (1)
3-3: No internal references toRefWithTargetormodel_factory.dart—safe to removeA repository-wide search (
rg -n 'RefWithTarget|model_factory\.dart' -S -g '!**/build/**') returned zero matches. There are no internal dependencies on those exports, so dropping them here won’t break this codebase. External consumers will see compile errors if they relied on these symbols—but that’s expected on a major version bump. No deprecation shim is needed for internal components.test/component_registry_writeops_test.dart (1)
93-101: No changes needed:equalsmatcher already deep-comparesSet<List>
Theexpect(actualSet, expectedSet)call in Dart’s test package uses theequalsmatcher, which performs a deep collection equality check on iterables and their contents—including lists inside a set. Converting each path to a string is purely stylistic and isn’t required for correctness.Likely an incorrect or invalid review comment.
lib/src/registry_api/components/component_registry_impl.dart (1)
58-71: Harden parsing ofrefsin component_registry_impl.dartGuard against malformed inputs by verifying
attrs['refs']is aListand filtering to valid maps before casting.• File: lib/src/registry_api/components/component_registry_impl.dart (lines 58–71)
• Updated snippet:- if (attrs.containsKey('refs')) { - final refs = attrs['refs'] as List; - for (final raw in refs) { - final vr = ValueReference.fromJson((raw as Map).cast<String, dynamic>()); + if (attrs['refs'] is List) { + final refs = (attrs['refs'] as List) + .whereType<Map>() // ignore non-map entries + .map((m) => m.cast<String, dynamic>()) + .toList(); + for (final raw in refs) { + final vr = ValueReference.fromJson(raw); out.add( PatchTemplate( path: <Object>[...currentPath, 'attributes', vr.attributeKey], sourceKey: vr.objectKey, defaultValue: vr.defaultValue, semantics: PatchSemantics.replace, ), ); }Please run the existing test suite to confirm there are no regressions with well-formed inputs.
Comment on lines
+39
to
+46
| /// Convenience factory for creating patch operations. | ||
| class PatchOps { | ||
| static AddOp add({required List<Object> path, Object? value}) => | ||
| AddOp(path: path, value: value); | ||
| static ReplaceOp replace({required List<Object> path, Object? value}) => | ||
| ReplaceOp(path: path, value: value); | ||
| static RemoveOp remove({required List<Object> path}) => RemoveOp(path: path); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prevent instantiation of the factory class.
The PatchOps class is used only for its static factory methods. Consider making it non-instantiable.
/// Convenience factory for creating patch operations.
-class PatchOps {
+abstract final class PatchOps {
static AddOp add({required List<Object> path, Object? value}) =>
AddOp(path: path, value: value);
static ReplaceOp replace({required List<Object> path, Object? value}) =>
ReplaceOp(path: path, value: value);
static RemoveOp remove({required List<Object> path}) => RemoveOp(path: path);
}🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 39 to 46, the PatchOps
class is intended only for static factory methods and should not be
instantiated. Make the class non-instantiable by adding a private constructor to
prevent creating instances of PatchOps.
Comment on lines
+74
to
+84
| if (obj['children'] is List) { | ||
| final List children = obj['children']; | ||
| for (var i = 0; i < children.length; i++) { | ||
| final child = children[i]; | ||
| if (child is Map<String, dynamic>) { | ||
| out.addAll( | ||
| _collectWriteOps(child, <Object>[...currentPath, 'children', i]), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: type children locally to improve IDE help and safety
Not critical, but makes intent clearer.
- if (obj['children'] is List) {
- final List children = obj['children'];
+ if (obj['children'] is List) {
+ final List<dynamic> children = obj['children'] as List;📝 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.
Suggested change
| if (obj['children'] is List) { | |
| final List children = obj['children']; | |
| for (var i = 0; i < children.length; i++) { | |
| final child = children[i]; | |
| if (child is Map<String, dynamic>) { | |
| out.addAll( | |
| _collectWriteOps(child, <Object>[...currentPath, 'children', i]), | |
| ); | |
| } | |
| } | |
| } | |
| if (obj['children'] is List) { | |
| final List<dynamic> children = obj['children'] as List; | |
| for (var i = 0; i < children.length; i++) { | |
| final child = children[i]; | |
| if (child is Map<String, dynamic>) { | |
| out.addAll( | |
| _collectWriteOps(child, <Object>[...currentPath, 'children', i]), | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/src/registry_api/components/component_registry_impl.dart around lines 74
to 84, the local variable 'children' is currently typed as a generic List. To
improve IDE support and type safety, explicitly type 'children' as
List<Map<String, dynamic>> or the most appropriate specific type based on the
expected contents. This clarifies intent and helps catch type errors earlier.
Comment on lines
+5262
to
+5263
| expect((action).script.functionName, "main"); | ||
| expect((action).script.meta, {"info": "metaInfo"}); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Restore the cast to avoid static type errors (and keep consistency).
getAction returns a nullable base type (likely ServerAction?), so action.script won't compile under strong typing. Line 5261 still uses an explicit cast; these two lines should, too.
Apply this minimal fix:
- expect((action).script.functionName, "main");
- expect((action).script.meta, {"info": "metaInfo"});
+ expect((action as ScriptAction).script.functionName, "main");
+ expect((action as ScriptAction).script.meta, {"info": "metaInfo"});Optionally, for readability, bind the cast once:
final scriptAction = action as ScriptAction;
expect(scriptAction.script.sourceCode, "print('hi')");
expect(scriptAction.script.functionName, "main");
expect(scriptAction.script.meta, {"info": "metaInfo"});🤖 Prompt for AI Agents
In test/data_source_test.dart around lines 5262 to 5263, the code accesses
properties on 'action.script' without casting 'action' from its nullable base
type, causing static type errors. Fix this by casting 'action' to 'ScriptAction'
before accessing 'script' properties, either by restoring the explicit cast on
each line or by assigning 'action as ScriptAction' to a local variable and using
that variable for the subsequent expects.
Comment on lines
+62
to
+113
| group('JsonPatchApplier - add', () { | ||
| test('add to map (terminal)', () { | ||
| final base = {'a': <String, dynamic>{}}; | ||
| final result = JsonPatchApplier.apply( | ||
| base, | ||
| [ | ||
| PatchOps.add(path: ['a', 'k'], value: 5) | ||
| ], | ||
| ); | ||
| expect(result['a']['k'], 5); | ||
| expect((base['a'] as Map).containsKey('k'), isFalse); | ||
| }); | ||
|
|
||
| test('add to list insert in middle', () { | ||
| final base = { | ||
| 'l': [1, 2, 3] | ||
| }; | ||
| final result = JsonPatchApplier.apply( | ||
| base, | ||
| [ | ||
| PatchOps.add(path: ['l', 1], value: 'x') | ||
| ], | ||
| ); | ||
| expect(result['l'], [1, 'x', 2, 3]); | ||
| expect(base['l'], [1, 2, 3]); | ||
| }); | ||
|
|
||
| test('add to list append at tail', () { | ||
| final base = { | ||
| 'l': [1, 2] | ||
| }; | ||
| final result = JsonPatchApplier.apply( | ||
| base, | ||
| [ | ||
| PatchOps.add(path: ['l', 2], value: 'x') | ||
| ], | ||
| ); | ||
| expect(result['l'], [1, 2, 'x']); | ||
| }); | ||
|
|
||
| test('add to list index>length with ensure', () { | ||
| final base = {'l': []}; | ||
| final result = JsonPatchApplier.apply( | ||
| base, | ||
| [ | ||
| PatchOps.add(path: ['l', 3], value: 'x') | ||
| ], | ||
| ensureIntermediate: true, | ||
| ); | ||
| expect(result['l'], [null, null, null, 'x']); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Broaden list edge-case coverage (negative index, non-int index).
Two more cases commonly bite implementers:
- Negative index: should be a no-op (or documented error).
- Non-int index for list operations: should be a no-op.
Recommend adding tests similar to:
test('replace in list negative index is no-op', () {
final base = {'l': [1, 2, 3]};
final result = JsonPatchApplier.apply(
base,
[PatchOps.replace(path: ['l', -1], value: 9)],
);
expect(result, base);
});
test('add to list with non-int index is no-op', () {
final base = {'l': [1, 2, 3]};
final result = JsonPatchApplier.apply(
base,
[PatchOps.add(path: ['l', 'not-an-index'], value: 9)],
);
expect(result, base);
});🤖 Prompt for AI Agents
In test/json_patch_test.dart between lines 62 and 113, the current tests for
list operations do not cover edge cases for negative indices and non-integer
indices. Add two new tests: one where a replace operation uses a negative index
on a list and should result in no changes, and another where an add operation
uses a non-integer index on a list and should also be a no-op. These tests
should verify that the result equals the original base object, ensuring these
edge cases are handled gracefully.
This was referenced Aug 10, 2025
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
lib/src/misc/json_patch/json_patch.dart (1)
1-297: Add missing edge–case tests for JsonPatchApplierIt looks like our existing tests in test/json_patch_test.dart only cover
ensureIntermediate: true. Please add targeted tests for these scenarios to lock in semantics and prevent regressions:• ensureIntermediate: false
– List index > length (no-op, does not throw)
– Negative list indices (no-op, does not throw)
• Empty path ([]) for add/replace/remove (should assert or throw)
• Replace root document with a non-Map value (throws as designed)[test/json_patch_test.dart]
♻️ Duplicate comments (5)
lib/src/misc/index.dart (1)
6-6: Nit: Quote consistency across this fileLine 6 uses double quotes (matching lines 4–5), while lines 1–3 use single quotes. Consider normalizing quotes across the file in a follow-up to reduce churn.
lib/src/misc/json_patch/json_patch.dart (4)
39-68: Make PatchOps non-instantiable.This class is only a namespace for static factories; it shouldn’t be instantiable.
- sealed class PatchOps { + abstract final class PatchOps { + const PatchOps._(); /// Add operation. static AddOp add({ required List<Object> path, Object? value, }) => AddOp( path: path, value: value, ); /// Replace operation. static ReplaceOp replace({ required List<Object> path, Object? value, }) => ReplaceOp( path: path, value: value, ); /// Remove operation. static RemoveOp remove({ required List<Object> path, }) => RemoveOp( path: path, ); }
276-281: Respectensurein _asMap to avoid masking structural mismatches.Currently returns
{}even whenensure=false. This can silently create maps where a non-map exists.static Map<String, dynamic> _asMap(Object? node, bool ensure) { if (node is Map<String, dynamic>) return node; if (node is Map) return node.cast<String, dynamic>(); - if (ensure) return <String, dynamic>{}; - return <String, dynamic>{}; + return ensure + ? <String, dynamic>{} + : throw StateError('Expected Map at path but found ${node.runtimeType}'); }
283-287: Respectensurein _asList to avoid masking structural mismatches.Likewise, return an empty list only when
ensure=true; otherwise, throw to surface type mismatches.static List<dynamic> _asList(Object? node, bool ensure) { if (node is List) return List<dynamic>.of(node); - if (ensure) return <dynamic>[]; - return <dynamic>[]; + return ensure + ? <dynamic>[] + : throw StateError('Expected List at path but found ${node.runtimeType}'); }
5-10: Document the path immutability contract (const constructor limits runtime enforcement).Const constructors prevent wrapping with List.unmodifiable at construction time. Make the immutability intent explicit in docs to avoid accidental external mutation of path.
abstract class PatchOp { /// JSON path segments from the document root to the target location. + /// + /// Important: Because these ops use const constructors, this list cannot be + /// wrapped in an unmodifiable view at runtime. Treat `path` as immutable and + /// prefer passing const list literals. final List<Object> path; const PatchOp(this.path); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
lib/src/misc/index.dart(1 hunks)lib/src/misc/json_patch/json_patch.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-10T12:49:06.602Z
Learnt from: lesleysin
PR: Duit-Foundation/duit_kernel#40
File: lib/src/misc/json_patch/json_patch.dart:5-10
Timestamp: 2025-08-10T12:49:06.602Z
Learning: In Dart, `List.unmodifiable()` cannot be used in const constructors because it creates a new object at runtime. Const constructors can only use compile-time constants and cannot execute code that creates new objects. When working with const constructors in Dart, immutability must be enforced through documentation or by using non-const alternatives.
Applied to files:
lib/src/misc/json_patch/json_patch.dart
📚 Learning: 2025-08-10T12:54:07.210Z
Learnt from: lesleysin
PR: Duit-Foundation/duit_kernel#40
File: lib/src/misc/json_patch/json_patch.dart:78-83
Timestamp: 2025-08-10T12:54:07.210Z
Learning: In Dart, switch expressions on non-sealed abstract classes require a default case to satisfy the analyzer's exhaustiveness check, even when all known subclasses are handled. This is enforced by the `non_exhaustive_switch_expression` diagnostic rule. The default case serves as a safety net for potential future subclasses.
Applied to files:
lib/src/misc/json_patch/json_patch.dart
🔇 Additional comments (3)
lib/src/misc/index.dart (1)
6-6: All checks passed for the json_patch export
- Confirmed
lib/src/misc/json_patch/index.dartexists- Verified it exports
json_patch.dart,semantics.dart, andtemplate.dart- No direct circular import back to
lib/src/misc/index.dartdetectedThis export is wired up correctly and ready to merge.
lib/src/misc/json_patch/json_patch.dart (2)
95-106: Keep the default case for exhaustiveness (correct for non-sealed base).Default fallback is required by the analyzer for non-sealed abstract base classes. Returning
baseas a safe noop is reasonable.
70-93: Overall approach looks solid (path-copy semantics, immutability).API shape and path-copying strategy are sound and fit the component system refactor goals.
Comment on lines
+16
to
+19
| class AddOp extends PatchOp { | ||
| final Object? value; | ||
| const AddOp({required List<Object> path, this.value}) : super(path); | ||
| } |
There was a problem hiding this comment.
Guard against empty paths for Add/Remove to prevent runtime errors.
_addAtPath/_removeAtPath access path[depth] unconditionally; an empty path will throw. Replace supports empty path, but Add/Remove do not. Add constructor-time asserts to fail fast.
class AddOp extends PatchOp {
final Object? value;
- const AddOp({required List<Object> path, this.value}) : super(path);
+ const AddOp({required List<Object> path, this.value})
+ : assert(path.length > 0, 'AddOp.path cannot be empty'),
+ super(path);
}
@@
class RemoveOp extends PatchOp {
- const RemoveOp({required List<Object> path}) : super(path);
+ const RemoveOp({required List<Object> path})
+ : assert(path.length > 0, 'RemoveOp.path cannot be empty'),
+ super(path);
}Also applies to: 35-37
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 16 to 19 and 35 to 37,
the AddOp and RemoveOp constructors currently do not guard against empty paths,
which causes runtime errors when _addAtPath or _removeAtPath access path[depth]
unconditionally. Add asserts in both constructors to ensure the path list is not
empty, so the code fails fast and prevents runtime errors from empty paths.
Comment on lines
+107
to
+114
| static Map<String, dynamic> _applyReplace( | ||
| Map<String, dynamic> base, | ||
| ReplaceOp op, | ||
| bool ensure, | ||
| ) { | ||
| final updated = _replaceAtPath(base, op.path, op.value, ensure, 0); | ||
| return updated is Map<String, dynamic> ? updated : base; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Root-level replace: avoid silent no-op when value is not a Map.
If op.path is empty and op.value isn’t a Map<String, dynamic>, _applyReplace returns base silently. Prefer a clear error to surface misuse.
static Map<String, dynamic> _applyReplace(
Map<String, dynamic> base,
ReplaceOp op,
bool ensure,
) {
+ // Replacing the document root: require a Map to preserve return type.
+ if (op.path.isEmpty) {
+ final val = op.value;
+ if (val is Map<String, dynamic>) return val;
+ throw StateError(
+ 'Replacing document root requires a Map<String, dynamic>, '
+ 'but got ${val.runtimeType}.',
+ );
+ }
final updated = _replaceAtPath(base, op.path, op.value, ensure, 0);
return updated is Map<String, dynamic> ? updated : base;
}Would you like me to update tests (or add new ones) to cover this case?
I’d like to inspect how _replaceAtPath behaves at the root and the definition of ReplaceOp.value to confirm this suggestion. Running:
🏁 Script executed:
#!/usr/bin/env bash
set -e
rg -n "_replaceAtPath" -B2 -A10 lib/src/misc/json_patch/json_patch.dart
rg -n "class ReplaceOp" -B2 -A5 lib/src/misc/json_patch/json_patch.dartLength of output: 1937
Enforce error on root replace of non-Map values
The current _applyReplace silently no-ops when op.path is empty and op.value isn’t a Map<String, dynamic>. That can mask incorrect usage—better to fail fast.
• File: lib/src/misc/json_patch/json_patch.dart
• Lines: 107–114
Proposed change:
static Map<String, dynamic> _applyReplace(
Map<String, dynamic> base,
ReplaceOp op,
bool ensure,
) {
+ // Replacing the document root must yield a Map<String, dynamic>.
+ if (op.path.isEmpty) {
+ final val = op.value;
+ if (val is Map<String, dynamic>) return val;
+ throw StateError(
+ 'Replacing document root requires a Map<String, dynamic>, '
+ 'but got ${val.runtimeType}.',
+ );
+ }
final updated = _replaceAtPath(base, op.path, op.value, ensure, 0);
return updated is Map<String, dynamic> ? updated : base;
}Would you like to add or update tests to cover:
- Replacing the root with a valid
Map<String, dynamic> - Attempting root replace with a non-Map (expecting a
StateError)
📝 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.
Suggested change
| static Map<String, dynamic> _applyReplace( | |
| Map<String, dynamic> base, | |
| ReplaceOp op, | |
| bool ensure, | |
| ) { | |
| final updated = _replaceAtPath(base, op.path, op.value, ensure, 0); | |
| return updated is Map<String, dynamic> ? updated : base; | |
| } | |
| static Map<String, dynamic> _applyReplace( | |
| Map<String, dynamic> base, | |
| ReplaceOp op, | |
| bool ensure, | |
| ) { | |
| // Replacing the document root must yield a Map<String, dynamic>. | |
| if (op.path.isEmpty) { | |
| final val = op.value; | |
| if (val is Map<String, dynamic>) return val; | |
| throw StateError( | |
| 'Replacing document root requires a Map<String, dynamic>, ' | |
| 'but got ${val.runtimeType}.', | |
| ); | |
| } | |
| final updated = _replaceAtPath(base, op.path, op.value, ensure, 0); | |
| return updated is Map<String, dynamic> ? updated : base; | |
| } |
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart lines 107 to 114, the _applyReplace
method currently does not throw an error when replacing the root with a non-Map
value, silently returning the original base. Modify the method to check if
op.path is empty and op.value is not a Map<String, dynamic>, and in that case
throw a StateError to fail fast. Also, add tests to cover replacing the root
with a valid Map and attempting a root replace with a non-Map expecting the
StateError.
Comment on lines
+128
to
+136
| final segment = path[depth]; | ||
| if (segment is String) { | ||
| final Map<String, dynamic> current = _asMap(node, ensure); | ||
| final Map<String, dynamic> clone = {...current}; | ||
| final nextNode = current[segment]; | ||
| final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1); | ||
| clone[segment] = replaced; | ||
| return clone; | ||
| } else if (segment is int) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid unnecessary cloning on replace when no change occurs.
Compute replaced first and clone only if the child actually changes. This reduces allocations along unaffected branches.
- if (segment is String) {
- final Map<String, dynamic> current = _asMap(node, ensure);
- final Map<String, dynamic> clone = {...current};
- final nextNode = current[segment];
- final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
- clone[segment] = replaced;
- return clone;
- }
+ if (segment is String) {
+ final Map<String, dynamic> current = _asMap(node, ensure);
+ final nextNode = current[segment];
+ final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
+ if (identical(nextNode, replaced)) return current; // no change
+ final Map<String, dynamic> clone = {...current};
+ clone[segment] = replaced;
+ return clone;
+ }🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 128 to 136, the code
clones the current map before checking if the replacement actually changes the
child node, causing unnecessary allocations. Modify the code to first compute
the replaced child node, then compare it with the existing child. Only clone the
current map and update the segment if the replaced child differs from the
original; otherwise, return the original current map to avoid unnecessary
cloning.
Comment on lines
+136
to
+151
| } else if (segment is int) { | ||
| final List<dynamic> current = _asList(node, ensure); | ||
| final List<dynamic> clone = List<dynamic>.of(current); | ||
| _ensureListLen(clone, segment, ensure); | ||
| final nextNode = segment < clone.length ? clone[segment] : null; | ||
| final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1); | ||
| if (segment < clone.length) { | ||
| clone[segment] = replaced; | ||
| } else { | ||
| // If ensure=false and index is out of range, keep original list | ||
| // but since we already ensured length when ensure=true, this else | ||
| // branch is effectively a no-op. | ||
| clone.add(replaced); | ||
| } | ||
| return clone; | ||
| } else { |
There was a problem hiding this comment.
Fix list handling in replace: negative indices and ensure=false out-of-range.
- Negative indices can cause RangeError when indexing the list.
- When
ensure=falseand index is out of range, current code still appends (contradicts comments and intended no-op).
} else if (segment is int) {
- final List<dynamic> current = _asList(node, ensure);
- final List<dynamic> clone = List<dynamic>.of(current);
- _ensureListLen(clone, segment, ensure);
- final nextNode = segment < clone.length ? clone[segment] : null;
+ final List<dynamic> current = _asList(node, ensure);
+ // Negative indices are invalid -> no-op
+ if (segment < 0) return current;
+ final List<dynamic> clone = List<dynamic>.of(current);
+ _ensureListLen(clone, segment, ensure);
+ final nextNode =
+ (segment >= 0 && segment < clone.length) ? clone[segment] : null;
final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
- if (segment < clone.length) {
+ if (segment >= 0 && segment < clone.length) {
clone[segment] = replaced;
- } else {
- // If ensure=false and index is out of range, keep original list
- // but since we already ensured length when ensure=true, this else
- // branch is effectively a no-op.
- clone.add(replaced);
+ } else if (ensure) {
+ // Only append when `ensure=true` expanded/ensured the length.
+ clone.add(replaced);
}
return clone;📝 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.
Suggested change
| } else if (segment is int) { | |
| final List<dynamic> current = _asList(node, ensure); | |
| final List<dynamic> clone = List<dynamic>.of(current); | |
| _ensureListLen(clone, segment, ensure); | |
| final nextNode = segment < clone.length ? clone[segment] : null; | |
| final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1); | |
| if (segment < clone.length) { | |
| clone[segment] = replaced; | |
| } else { | |
| // If ensure=false and index is out of range, keep original list | |
| // but since we already ensured length when ensure=true, this else | |
| // branch is effectively a no-op. | |
| clone.add(replaced); | |
| } | |
| return clone; | |
| } else { | |
| } else if (segment is int) { | |
| final List<dynamic> current = _asList(node, ensure); | |
| // Negative indices are invalid -> no-op | |
| if (segment < 0) return current; | |
| final List<dynamic> clone = List<dynamic>.of(current); | |
| _ensureListLen(clone, segment, ensure); | |
| final nextNode = | |
| (segment >= 0 && segment < clone.length) ? clone[segment] : null; | |
| final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1); | |
| if (segment >= 0 && segment < clone.length) { | |
| clone[segment] = replaced; | |
| } else if (ensure) { | |
| // Only append when `ensure=true` expanded/ensured the length. | |
| clone.add(replaced); | |
| } | |
| return clone; | |
| } else { |
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 136 to 151, the code
does not correctly handle negative indices which can cause RangeError, and when
ensure is false with an out-of-range index, it incorrectly appends to the list
instead of performing a no-op. Fix this by adding a check for negative indices
to prevent invalid access and modify the logic so that if ensure is false and
the index is out of range, the original list is returned unchanged without
appending.
Comment on lines
+186
to
+213
| } else if (segment is int) { | ||
| final List<dynamic> current = _asList(node, ensure); | ||
| final List<dynamic> clone = List<dynamic>.of(current); | ||
| if (isLast) { | ||
| // insert semantics: if index within [0, length], insert; if index | ||
| // greater than length and ensure=true, expand with nulls and append. | ||
| if (segment <= clone.length) { | ||
| clone.insert(segment, value); | ||
| } else if (ensure) { | ||
| while (clone.length < segment) { | ||
| clone.add(null); | ||
| } | ||
| // insert at tail (same as add) | ||
| clone.add(value); | ||
| } // else: no-op when ensure=false and index>length | ||
| return clone; | ||
| } | ||
| _ensureListLen(clone, segment, ensure); | ||
| final nextNode = segment < clone.length ? clone[segment] : null; | ||
| final next = _addAtPath(nextNode, path, value, ensure, depth + 1); | ||
| if (segment < clone.length) { | ||
| clone[segment] = next; | ||
| } else { | ||
| // ensure=false and index>length -> no-op; but for consistency, append | ||
| clone.add(next); | ||
| } | ||
| return clone; | ||
| } else { |
There was a problem hiding this comment.
Fix list handling in add: negative indices and ensure=false out-of-range.
List.insertwith a negative index will throw.- Non-last segment path currently appends even when
ensure=false; should be a no-op.
} else if (segment is int) {
final List<dynamic> current = _asList(node, ensure);
final List<dynamic> clone = List<dynamic>.of(current);
if (isLast) {
// insert semantics: if index within [0, length], insert; if index
// greater than length and ensure=true, expand with nulls and append.
- if (segment <= clone.length) {
+ if (segment >= 0 && segment <= clone.length) {
clone.insert(segment, value);
} else if (ensure) {
- while (clone.length < segment) {
+ while (clone.length < segment) {
clone.add(null);
}
// insert at tail (same as add)
clone.add(value);
- } // else: no-op when ensure=false and index>length
+ } // else: no-op when ensure=false or index<0
return clone;
}
- _ensureListLen(clone, segment, ensure);
- final nextNode = segment < clone.length ? clone[segment] : null;
+ // Non-terminal segment
+ if (segment < 0) return clone; // invalid index -> no-op
+ _ensureListLen(clone, segment, ensure);
+ final nextNode = (segment < clone.length) ? clone[segment] : null;
final next = _addAtPath(nextNode, path, value, ensure, depth + 1);
- if (segment < clone.length) {
+ if (segment < clone.length) {
clone[segment] = next;
- } else {
- // ensure=false and index>length -> no-op; but for consistency, append
- clone.add(next);
+ } else if (ensure) {
+ // Only append when `ensure=true` expanded/ensured the length.
+ clone.add(next);
}
return clone;📝 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.
Suggested change
| } else if (segment is int) { | |
| final List<dynamic> current = _asList(node, ensure); | |
| final List<dynamic> clone = List<dynamic>.of(current); | |
| if (isLast) { | |
| // insert semantics: if index within [0, length], insert; if index | |
| // greater than length and ensure=true, expand with nulls and append. | |
| if (segment <= clone.length) { | |
| clone.insert(segment, value); | |
| } else if (ensure) { | |
| while (clone.length < segment) { | |
| clone.add(null); | |
| } | |
| // insert at tail (same as add) | |
| clone.add(value); | |
| } // else: no-op when ensure=false and index>length | |
| return clone; | |
| } | |
| _ensureListLen(clone, segment, ensure); | |
| final nextNode = segment < clone.length ? clone[segment] : null; | |
| final next = _addAtPath(nextNode, path, value, ensure, depth + 1); | |
| if (segment < clone.length) { | |
| clone[segment] = next; | |
| } else { | |
| // ensure=false and index>length -> no-op; but for consistency, append | |
| clone.add(next); | |
| } | |
| return clone; | |
| } else { | |
| } else if (segment is int) { | |
| final List<dynamic> current = _asList(node, ensure); | |
| final List<dynamic> clone = List<dynamic>.of(current); | |
| if (isLast) { | |
| // insert semantics: if index within [0, length], insert; if index | |
| // greater than length and ensure=true, expand with nulls and append. | |
| if (segment >= 0 && segment <= clone.length) { | |
| clone.insert(segment, value); | |
| } else if (ensure) { | |
| while (clone.length < segment) { | |
| clone.add(null); | |
| } | |
| // insert at tail (same as add) | |
| clone.add(value); | |
| } // else: no-op when ensure=false or index<0 | |
| return clone; | |
| } | |
| // Non-terminal segment | |
| if (segment < 0) return clone; // invalid index -> no-op | |
| _ensureListLen(clone, segment, ensure); | |
| final nextNode = (segment < clone.length) ? clone[segment] : null; | |
| final next = _addAtPath(nextNode, path, value, ensure, depth + 1); | |
| if (segment < clone.length) { | |
| clone[segment] = next; | |
| } else if (ensure) { | |
| // Only append when `ensure=true` expanded/ensured the length. | |
| clone.add(next); | |
| } | |
| return clone; | |
| } else { |
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart lines 186 to 213, the code
incorrectly handles negative indices for list insertion and appends elements
even when ensure is false and the index is out of range for non-last segments.
Fix this by adding checks to prevent negative indices from being used in
List.insert, and modify the logic so that when ensure is false and the index is
out of range for non-last segments, the operation becomes a no-op instead of
appending to the list.
Comment on lines
+218
to
+225
| static Map<String, dynamic> _applyRemove( | ||
| Map<String, dynamic> base, | ||
| RemoveOp op, | ||
| bool ensure, | ||
| ) { | ||
| final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0); | ||
| return updated is Map<String, dynamic> ? updated : base; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify that ensureIntermediate is ignored for remove.
_applyRemove hardcodes ensure=false. If intentional, document this so callers don’t expect creation of intermediate nodes during remove.
static Map<String, dynamic> _applyRemove(
Map<String, dynamic> base,
RemoveOp op,
bool ensure,
) {
- final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0);
+ // Note: remove never creates intermediates; ignore `ensure`.
+ final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0);
return updated is Map<String, dynamic> ? updated : base;
}📝 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.
Suggested change
| static Map<String, dynamic> _applyRemove( | |
| Map<String, dynamic> base, | |
| RemoveOp op, | |
| bool ensure, | |
| ) { | |
| final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0); | |
| return updated is Map<String, dynamic> ? updated : base; | |
| } | |
| static Map<String, dynamic> _applyRemove( | |
| Map<String, dynamic> base, | |
| RemoveOp op, | |
| bool ensure, | |
| ) { | |
| // Note: remove never creates intermediate nodes; the `ensure` flag is ignored. | |
| final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0); | |
| return updated is Map<String, dynamic> ? updated : base; | |
| } |
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 218 to 225, the
_applyRemove method hardcodes the ensure parameter as false when calling
_removeAtPath, which means it does not create intermediate nodes during removal.
Add a comment in the method to explicitly document that ensureIntermediate is
intentionally ignored for remove operations, so callers understand that
intermediate node creation is not supported in this context.
lesleysin
added a commit
that referenced
this pull request
Nov 5, 2025
* breaking: DuitDataSource (#32) * experiment - removing attribute support * wip * wip * wip * refacror ViewAttributes class * Added tests * wip * Added early exit from methods * Added new functions for handling JSON parsing results, improved default result return logic, and added table mapping for BlendMode and FloatingActionButtonLocation. * docs * add tests * Added tests * Removed static mapping tables for Clip, TextAlign, TextOverflow and TextWidthBasis in the DuitDataSource class. * Added new methods for JSON processing, including support for textDecorationStyle, fontWeight, fontStyle and other attributes. Improved logic for returning default values and added corresponding tests. * Added tests * Removed unused methods and added new functions to handle theme rules and image filters. Updated tests to check new features and JSON processing logic. * Upd .gitignore * wip * Upd tests, added Tween parser * Removed ventilation methods, added new functions for widget handling. Updated tests to check new features and JSON handling logic. * Refactored functions for creating InputBorder and ShapeBorder from a map, as well as corresponding lookup tables. Simplified logic for selecting the border type. * Added docs * upd actions config * wip * wip * Update test/theme_test.dart Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * clean dead code * fix * Update lib/src/view_attributes/lookup.dart Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Added export of Command.dart file to action_api and removed it from animation_api. Updated data references in DuitDataSource class to use new "stretchModes" key. Fixes to tests to match changes in step data. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Remote commands API update (#33) * breaking: Themes refactoring (#34) * Registry API update: Added new RefWithTarget class, changed component initialization and registration functions, removed unused files, and improved export structure. Updated tests and removed deprecated topic tokens. * upd workflow * breaking: Reworking the widget model API and simplifying registration of custom widgets (#35) * Removed unused factory_record.dart file and updated component registration logic in DuitRegistry class, removed references to model factories. Updated factory getter methods to match new changes. * Small fix for tweens parser * rm unused API part * Update lib/src/registry_api/registry.dart Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * review fixes * fix --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Improve lootup tables for enums (#36) * Flutter types json encoding (#37) * Added encoding functions for: Duration, Size, EdgeInsets, TextStyle, Color, LinearGradient, BoxShadow, Offset, BoxDecoration, BorderRadius, Border, BorderSide, InputBorder, InputDecoration, VisualDensity, ScrollPhysics, ShapeBorder * wip * Added tests * Apply suggestions from code review Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Fix tests * Added docs * dart format --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * feat: action debounce and throttle configuration (#38) * Added execution options for actions: added ExecutionOptions class with throttle and debounce modifiers, updated ServerAction classes and their descendants to support the new options. Updated parsing methods and added corresponding lookup tables for execution modifiers. * - Added test - Refactor DuitDataSource methods with one-line return and fail-fast approach at lookup tables work * fix Color parsers behavior * refactoring: improved color handling in _colorFromList function Changed the typification function of the _colorFromList parameter from List<num> to List, added preprocessing of color data to ensure correct conversion. Updated the corresponding calls in the JSON parsing and processing methods in the DuitDataSource class. * feat: Fragments registration in DuitRegistry (#39) Added a new static method registerFragment for registering fragments by key and a method getFragment for receiving a fragment by key with logging in case of absence. Also added an internal registry of fragments _fragmentRegistry. * Component system refactor (#40) * Components refactoring via JSON patching instead of Map mutation * Added tests for json patching and components patches generation * Review fixes * Upd lookup table for Curves * Remover legacy API references * feat: DuitDataSource methods extension (#41) * Extend parser for BorderRadius type, added Radius type parsing, upd tests * fix tests * Added new Size parsing behavior * wip * feat: Custom json reviver (#42) * Refactor env variables, refactor analysis issues in test files, added FlutterPropertyKeys class * wip * wip * wip * Fixed analysis issues * wip * Fixed getActionDependencies and childObjects methods * Upd env variables keys * Code review fixes * Fix test * Upd post-merge workflow
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworked the component system to move away from a mutable template to a JSON Patch and path-copy approach
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests