Improve Avalonia Support#562
Merged
Daniel-Svensson merged 12 commits intoOpenRIAServices:mainfrom Apr 8, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEntityCollection, EntitySet, and LoadResult were changed to expose indexable interfaces and indexers (IList / IReadOnlyList), made removals index-aware (TryRemoveEntity / RemoveEntity(entity, index)), and adjusted collection-change notification flows for per-item remove and batched add notifications. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Coll as EntityCollection<T>
participant Source as SourceSet
participant Events as NotificationSystem
rect rgba(100,150,200,0.5)
Note over Client,Coll: Client requests removal
Client->>Coll: Remove(entity)
Coll->>Coll: TryRemoveEntity(entity, out idx)
alt removed (idx >= 0)
Coll->>Events: Notify Remove at idx
else not present
Coll->>Events: (no notification)
end
end
rect rgba(150,100,200,0.5)
Note over Source,Coll: SourceSet collection changed
Source->>Coll: SourceSet_CollectionChanged(args)
alt items added
Coll->>Coll: materialize new items (ToList), compute newStartingIdx = Entities.Count
Coll->>Events: Notify Add range starting at newStartingIdx
else items removed
loop per removed entity
Coll->>Coll: TryRemoveEntity(entity, out idx)
alt removed (idx >= 0)
Coll->>Events: Notify Remove at idx for that entity
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OpenRiaServices.Client/Framework/EntityCollection.cs (1)
1018-1051: 🧹 Nitpick | 🔵 Trivial
IReadOnlyList<TEntity>indexer does not callLoad().Line 1019: The indexer accesses
Entities[index]directly without callingLoad(). If accessed beforeCount(which triggersLoad()), this could return incorrect results or throw. Consider callingLoad()for safety, or document thatCountmust be accessed first.♻️ Option to add Load() call for safety
`#region` ICollection<TEntity>, IReadOnlyList<TEntity> Members - TEntity IReadOnlyList<TEntity>.this[int index] => Entities[index]; + TEntity IReadOnlyList<TEntity>.this[int index] + { + get + { + Load(); + return Entities[index]; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1018 - 1051, The IReadOnlyList<TEntity> indexer getter currently returns Entities[index] directly (IReadOnlyList<TEntity>.this[int index] => Entities[index]); modify this getter to call Load() before accessing Entities to ensure the collection is initialized (i.e., call this.Load() then return Entities[index]); update any related tests or callers if necessary to account for potential lazy loading side-effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1116-1119: The IList.Remove implementation in EntityCollection
currently casts the incoming object directly and will throw InvalidCastException
for non-TEntity values; update the IList.Remove method to first check the type
(e.g., use "is TEntity" or "as TEntity") and only call Remove(TEntity) when the
value is a TEntity, otherwise do nothing (mirror the safe behavior of
IList.Contains) to avoid exceptions for wrong types.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 985-988: The NotSupportedException in IList.RemoveAt (in class
EntitySet) has a typo in its message ("RemoteAt"); update the format argument to
"RemoveAt" so the Resource.IsNotSupported message correctly references the
RemoveAt operation (i.e., change the string passed into the string.Format call
from "RemoteAt" to "RemoveAt" in the IList.RemoveAt method).
- Around line 1271-1272: The List property currently references itself causing
infinite recursion; change its getter to return the base class list cast to
List<TEntity> (i.e., use base.List) so the property reads the inherited
collection instead of calling itself; update the property declared as "private
new List<TEntity> List" to return (List<TEntity>)base.List to fix
StackOverflowException when accessing List in EntitySet or the List property
accessor.
- Around line 11-12: Remove the unused using directive "using
System.Diagnostics.Contracts;" from the top of the file; locate the EntitySet
class in src/OpenRiaServices.Client/Framework/EntitySet.cs and delete that using
line so only actually referenced namespaces remain, ensuring no other code in
EntitySet (constructors, methods, properties) depends on
System.Diagnostics.Contracts.
---
Outside diff comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1018-1051: The IReadOnlyList<TEntity> indexer getter currently
returns Entities[index] directly (IReadOnlyList<TEntity>.this[int index] =>
Entities[index]); modify this getter to call Load() before accessing Entities to
ensure the collection is initialized (i.e., call this.Load() then return
Entities[index]); update any related tests or callers if necessary to account
for potential lazy loading side-effects.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee3e524b-b64f-4539-83bf-d2f17b0ef8bf
📒 Files selected for processing (2)
src/OpenRiaServices.Client/Framework/EntityCollection.cssrc/OpenRiaServices.Client/Framework/EntitySet.cs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Member
Author
|
Todo: look at LoadOperation and LoadResult |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Line 21: The IList capability flags on EntitySet are inconsistent with
ListCollectionViewProxy<T>; change the IList implementations on EntitySet so
IList.IsFixedSize and IList.IsReadOnly mirror the underlying add/remove
capabilities the same way the proxy does: compute both properties as
!(this.CanAdd || this.CanRemove) (or equivalent using the EntitySet's
CanAdd/CanRemove members) rather than hardcoding IsFixedSize=false and relying
on IsReadOnly fallback; update the IList explicit interface implementations in
EntitySet to return that computed value so direct IList callers see the same
behavior as ListCollectionViewProxy<T>.
- Around line 955-960: IList.Add(object) currently assumes only one item is
added and returns -1 when Add(Entity) (which may call AddAttachInferrer.Infer())
expands the set by more than one; change the implementation so if Count !=
countBefore + 1 it returns the actual index of the inserted entity by calling
IndexOf((Entity)value) (or similar) instead of -1, otherwise keep returning
countBefore; update the IList.Add(object) method to use IndexOf when infer-add
expanded the collection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38e05d74-91e8-4043-b060-6c969c8a7281
📒 Files selected for processing (1)
src/OpenRiaServices.Client/Framework/EntitySet.cs
do some cleanup on ListCollectionViewProxy
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/OpenRiaServices.Client/Framework/EntitySet.cs (1)
946-950:⚠️ Potential issue | 🟠 MajorAdvertise fixed-size correctly on read-only sets.
When neither add nor remove is supported,
IList.IsReadOnlybecomestruebutIList.IsFixedSizestaysfalse.ListCollectionViewProxy<T>now forwards these flags fromEntitySet, so view code still sees immutable sets as resizable. Mirror the same capability check in both properties.♻️ Proposed fix
-bool IList.IsFixedSize => false; - -bool IList.IsReadOnly => (_supportedOperations & (EntitySetOperations.Remove | EntitySetOperations.Add)) == 0; +bool IList.IsFixedSize => !(this.CanAdd || this.CanRemove); + +bool IList.IsReadOnly => !(this.CanAdd || this.CanRemove);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntitySet.cs` around lines 946 - 950, IList.IsFixedSize is incorrectly always false while IList.IsReadOnly correctly checks supported operations; make IsFixedSize mirror the same capability check as IsReadOnly by returning true when neither EntitySetOperations.Add nor EntitySetOperations.Remove is supported. Update the IList.IsFixedSize getter in EntitySet (and keep IList.IsReadOnly as-is) to use the same bitmask check against _supportedOperations (EntitySetOperations.Remove | EntitySetOperations.Add) so immutable sets advertise fixed size consistently for ListCollectionViewProxy<T> and other consumers.src/OpenRiaServices.Client/Framework/EntityCollection.cs (1)
1116-1119:⚠️ Potential issue | 🟠 MajorMake direct
IList.Removetolerant of probe calls.This still casts
objectdirectly and then delegates to the strictRemove(TEntity)path. That turns wrong-type or already-removed values into exceptions on the newIListsurface instead of a no-op.♻️ Proposed fix
void IList.Remove(object value) { - Remove((TEntity)value); + if (value is TEntity entity) + { + ((ICollection<TEntity>)this).Remove(entity); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1116 - 1119, The IList.Remove implementation should be tolerant of probe/wrong-type calls: instead of unconditionally casting object to TEntity, first check the runtime type and only call Remove(TEntity) when value is a TEntity (e.g., use "if (value is TEntity entity) Remove(entity);" or equivalent); otherwise return/no-op so wrong-type or already-removed/null probe calls do not throw on the IList surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1084-1090: IList.Add currently skips validation and returns
countBefore when it assumes exactly one item added; instead mirror
EntityCollection<TEntity>.ListCollectionViewProxy<T>.Add by validating/casting
the input (throwing the same ArgumentException/InvalidCast as Add(TEntity)
does), call the collection add logic (Add((TEntity)value) or underlying
Source.Add), then call IndexOf on the added entity to determine and return the
actual insertion index (or -1 if not found) so side-effects that add multiple
items still yield the correct index; use the same validation and IndexOf lookup
code paths as the proxy Add to keep behavior consistent.
- Around line 1018-1020: The public indexer TEntity this[int index] currently
returns Entities[index] without ensuring the collection is loaded; modify the
indexer to call Load() before accessing Entities to guarantee the cache is
populated (matching Count and GetEnumerator behavior) and update its comment to
document that Load() is invoked; apply the same change to the other public
indexer occurrences around lines referenced (e.g., the IList indexer already
documents expectation—mirror that behavior by invoking Load() in the public
indexer and any similar accessors such as the one at 1074-1082).
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 985-988: IList.Remove(object) currently casts value to Entity and
can throw InvalidCastException; change its implementation to mirror
Contains/IndexOf by first checking "value is Entity e" and only call Remove(e)
when true, otherwise do nothing (silently return) so incompatible types or null
don't cause exceptions—update the IList.Remove(object) method that currently
calls Remove((Entity)value) to use the safe type-check pattern.
---
Duplicate comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1116-1119: The IList.Remove implementation should be tolerant of
probe/wrong-type calls: instead of unconditionally casting object to TEntity,
first check the runtime type and only call Remove(TEntity) when value is a
TEntity (e.g., use "if (value is TEntity entity) Remove(entity);" or
equivalent); otherwise return/no-op so wrong-type or already-removed/null probe
calls do not throw on the IList surface.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 946-950: IList.IsFixedSize is incorrectly always false while
IList.IsReadOnly correctly checks supported operations; make IsFixedSize mirror
the same capability check as IsReadOnly by returning true when neither
EntitySetOperations.Add nor EntitySetOperations.Remove is supported. Update the
IList.IsFixedSize getter in EntitySet (and keep IList.IsReadOnly as-is) to use
the same bitmask check against _supportedOperations (EntitySetOperations.Remove
| EntitySetOperations.Add) so immutable sets advertise fixed size consistently
for ListCollectionViewProxy<T> and other consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffcc4c47-7b9f-4bb7-9755-aaea82d1e515
📒 Files selected for processing (3)
src/OpenRiaServices.Client/Framework/EntityCollection.cssrc/OpenRiaServices.Client/Framework/EntitySet.cssrc/OpenRiaServices.Client/Framework/LoadResult.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/OpenRiaServices.Client/Framework/EntitySet.cs (1)
985-995:⚠️ Potential issue | 🟡 MinorShort-circuit incompatible or missing items before
IList.Remove.
value is Entitystill lets a different entity subtype fall through toRemove, which then throwsArgumentExceptioneven thoughContains/IndexOftreat that case as a miss. A containment guard also lets you drop the localized-message exception filter.♻️ Suggested fix
void IList.Remove(object value) { - try - { - if (value is Entity entity) - Remove(entity); - } - catch (InvalidOperationException ioe) when (ioe.Message == Resource.EntitySet_EntityNotInSet) - { - // Don't throw if item was not in the collection - } + if (value is Entity entity && this.Contains(entity)) + { + this.Remove(entity); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntitySet.cs` around lines 985 - 995, The IList.Remove implementation should short-circuit non-Entity or non-contained items instead of calling Remove and relying on an exception filter; update the IList.Remove method so it first checks that value is an Entity and that this.Contains((Entity)value) (or this.IndexOf((Entity)value) != -1) before calling Remove(entity), and simply return/no-op for null or incompatible types — drop the localized InvalidOperationException filter and related catch block because containment check prevents the exception path; reference the IList.Remove implementation, the Remove(Entity) method and Contains/IndexOf helpers when making the change.src/OpenRiaServices.Client/Framework/EntityCollection.cs (2)
1018-1024:⚠️ Potential issue | 🟠 MajorLoad before serving the new indexer.
Entitiesonly materializes the backing list; it does not populate it. On an unloaded association,collection[index]can therefore throw or expose a partial view until some other call happens to triggerLoad(). The explicitIList.this[int]getter now forwards here too, so both indexer surfaces inherit the same fragile precondition.♻️ Suggested fix
- /// <remarks>**Important**: Make sure to check <see cref="Count"/> first to ensure the collection is initialized</remarks> /// <param name="index">The zero-based index of the entity to retrieve.</param> /// <returns>The entity located at the specified index.</returns> - public TEntity this[int index] => Entities[index]; + public TEntity this[int index] + { + get + { + this.Load(); + return this.Entities[index]; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1018 - 1024, The indexer getter (public TEntity this[int index] => Entities[index]) must ensure the collection is loaded before accessing the backing list; modify the TEntity indexer and any IList.this[int] forwarding to call the collection's load/ensure method (e.g., Load() or EnsureLoaded()) first and only then return Entities[index], so an unloaded association no longer exposes a partial view or throws.
1086-1096:⚠️ Potential issue | 🟡 MinorGuard the direct
IListobject paths instead of casting blindly.
IList.AddandIList.Removestill castvaluestraight toTEntity. On the new object-based list surface, that means incompatible inputs can blow up withInvalidCastException, even thoughIList.ContainsandListCollectionViewProxy<T>.Addalready do type-aware handling.♻️ Suggested fix
int IList.Add(object value) { + if (value is null) + { + throw new ArgumentNullException(nameof(value)); + } + if (value is not TEntity entity) + { + throw new ArgumentException( + string.Format(CultureInfo.CurrentCulture, Resource.MustBeAnEntity, nameof(value)), + nameof(value)); + } + int countBefore = this.Count; - Add((TEntity)value); + this.Add(entity); if (this.Count == countBefore + 1) return countBefore; else if (this.Count == countBefore) return -1; else - return Entities.IndexOf((TEntity)value, countBefore); + return this.Entities.IndexOf(entity, countBefore); }void IList.Remove(object value) { - Remove((TEntity)value); + if (value is TEntity entity) + { + this.Remove(entity); + } }Also applies to: 1123-1125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1086 - 1096, IList.Add and IList.Remove currently cast value directly to TEntity which can throw InvalidCastException for incompatible inputs; change both to type-guard before casting: in IList.Add, check "if (value is TEntity entity) { Add(entity); /* existing return logic */ } else throw new ArgumentException(\"Value is of incorrect type\", nameof(value));" and in IList.Remove, check "if (value is TEntity entity) return Remove(entity); else return false;"; mirror the type-aware pattern used by IList.Contains and ListCollectionViewProxy<T>.Add and keep the existing post-add count/return logic in Add (use the same Entities.IndexOf path when added).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1018-1024: The indexer getter (public TEntity this[int index] =>
Entities[index]) must ensure the collection is loaded before accessing the
backing list; modify the TEntity indexer and any IList.this[int] forwarding to
call the collection's load/ensure method (e.g., Load() or EnsureLoaded()) first
and only then return Entities[index], so an unloaded association no longer
exposes a partial view or throws.
- Around line 1086-1096: IList.Add and IList.Remove currently cast value
directly to TEntity which can throw InvalidCastException for incompatible
inputs; change both to type-guard before casting: in IList.Add, check "if (value
is TEntity entity) { Add(entity); /* existing return logic */ } else throw new
ArgumentException(\"Value is of incorrect type\", nameof(value));" and in
IList.Remove, check "if (value is TEntity entity) return Remove(entity); else
return false;"; mirror the type-aware pattern used by IList.Contains and
ListCollectionViewProxy<T>.Add and keep the existing post-add count/return logic
in Add (use the same Entities.IndexOf path when added).
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 985-995: The IList.Remove implementation should short-circuit
non-Entity or non-contained items instead of calling Remove and relying on an
exception filter; update the IList.Remove method so it first checks that value
is an Entity and that this.Contains((Entity)value) (or
this.IndexOf((Entity)value) != -1) before calling Remove(entity), and simply
return/no-op for null or incompatible types — drop the localized
InvalidOperationException filter and related catch block because containment
check prevents the exception path; reference the IList.Remove implementation,
the Remove(Entity) method and Contains/IndexOf helpers when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f742f43a-f819-4f0a-8d67-933a202ec32d
📒 Files selected for processing (3)
src/OpenRiaServices.Client/Framework/EntityCollection.cssrc/OpenRiaServices.Client/Framework/EntitySet.cssrc/OpenRiaServices.Client/Framework/LoadResult.cs
…k if items are allowed to be added or removed by that instance (not if somebody else can change the list)
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/OpenRiaServices.Client/Framework/EntityCollection.cs (2)
1018-1024:⚠️ Potential issue | 🟠 MajorLoad before serving the new indexer.
Line 1024 returns
Entities[index]without callingLoad(). On an unloaded collection,Entitiesjust materializes an empty cache, socollection[index]can fail even thoughCountand enumeration would have loaded the source set. Because Lines 1080-1083 delegate here, both new indexers inherit it.Proposed fix
-public TEntity this[int index] => Entities[index]; +public TEntity this[int index] +{ + get + { + this.Load(); + return this.Entities[index]; + } +}Also applies to: 1079-1083
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1018 - 1024, The indexer this[int index] currently returns Entities[index] without ensuring the collection is loaded; change it to call Load() first (i.e., ensure the collection is initialized before accessing the Entities cache) so that an unloaded collection won't produce an empty materialized cache and fail on index access; update the indexer implementation (and any other delegating indexers at the same code paths around the methods referencing Entities and Lines 1079–1083) to invoke Load() prior to reading Entities or Count.
1086-1097:⚠️ Potential issue | 🟠 MajorRoute the direct
IListmutators through the existing safe paths.Lines 1089 and 1125 still direct-cast
object. That drops the validation already present inListCollectionViewProxy<T>.Addat Lines 850-865 and bypasses the non-throwingICollection<TEntity>.Removepath at Lines 1047-1056, so the new Avalonia-facingIListsurface can still throw on wrong types or non-members.Proposed fix
int IList.Add(object value) { - int countBefore = this.Count; - Add((TEntity)value); + TEntity entity = value as TEntity; + if (entity == null) + { + throw new ArgumentException( + string.Format(CultureInfo.CurrentCulture, Resource.MustBeAnEntity, "value"), + nameof(value)); + } + + int countBefore = this.Count; + Add(entity); if (this.Count == countBefore + 1) return countBefore; else if (this.Count == countBefore) return -1; else - return Entities.IndexOf((TEntity)value, countBefore); + return Entities.IndexOf(entity, countBefore); } void IList.Remove(object value) { - Remove((TEntity)value); + TEntity entity = value as TEntity; + if (entity != null) + { + ((ICollection<TEntity>)this).Remove(entity); + } }Run the following script to verify the mismatch against the safe paths already present in this file:
#!/bin/bash sed -n '850,865p' src/OpenRiaServices.Client/Framework/EntityCollection.cs printf '\n---\n' sed -n '1047,1056p' src/OpenRiaServices.Client/Framework/EntityCollection.cs printf '\n---\n' sed -n '1086,1126p' src/OpenRiaServices.Client/Framework/EntityCollection.csExpected result: the proxy
Addvalidatesvalue,ICollection<TEntity>.Removereturnsfalsewhen the item is absent, and the directIListmutators still cast directly.Also applies to: 1123-1126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1086 - 1097, The IList mutators are bypassing existing validation and non-throwing behavior by direct-casting object values; change IList.Add(object) and the IList.Remove(object) path to route through the safe helpers already in this file: call the existing ListCollectionViewProxy<T>.Add validation path (the Add(TEntity) / proxy Add logic around lines ~850-865) before computing the return index so wrong-type inputs get the same validation/exception behavior, and for removal route through ICollection<TEntity>.Remove (the non-throwing path around lines ~1047-1056) instead of casting and removing directly; only use Entities.IndexOf(...) after the safe Add completed to determine the return index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1018-1024: The indexer this[int index] currently returns
Entities[index] without ensuring the collection is loaded; change it to call
Load() first (i.e., ensure the collection is initialized before accessing the
Entities cache) so that an unloaded collection won't produce an empty
materialized cache and fail on index access; update the indexer implementation
(and any other delegating indexers at the same code paths around the methods
referencing Entities and Lines 1079–1083) to invoke Load() prior to reading
Entities or Count.
- Around line 1086-1097: The IList mutators are bypassing existing validation
and non-throwing behavior by direct-casting object values; change
IList.Add(object) and the IList.Remove(object) path to route through the safe
helpers already in this file: call the existing ListCollectionViewProxy<T>.Add
validation path (the Add(TEntity) / proxy Add logic around lines ~850-865)
before computing the return index so wrong-type inputs get the same
validation/exception behavior, and for removal route through
ICollection<TEntity>.Remove (the non-throwing path around lines ~1047-1056)
instead of casting and removing directly; only use Entities.IndexOf(...) after
the safe Add completed to determine the return index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 078bb1dc-09ef-4aa9-b585-462fb71e20a2
📒 Files selected for processing (3)
src/OpenRiaServices.Client/Framework/EntityCollection.cssrc/OpenRiaServices.Client/Framework/EntitySet.cssrc/OpenRiaServices.Client/Framework/LoadResult.cs
|
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
2 participants
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.
When binding collections avalonia requires any collection that does implement INotifyCollectionChanged to also implement IList.
Contains(entity)publicSummary by CodeRabbit
New Features
Bug Fixes / Behavior