feat: array find and findIndex / reset#87
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a typed root accessor and a reset(data?) lifecycle method to RowModel (and TypedRowModel), exposes patches as a public property, and adds predicate search methods (find, findIndex) to ArrayValueNode and its typed variants; updates implementation, docs, and tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/model/table/row/RowModelImpl.ts (1)
123-127:resetwithnullsilently falls through to defaults — consider documenting or guarding.The nullish coalescing (
??) meansreset(null)will generate defaults instead of attempting to setnull. Whilereset(undefined)(i.e. no argument) → defaults is clearly intentional, thenullcase is implicit. This is likely fine in practice (null isn't a valid root value), but it may surprise callers.Consider either documenting this behavior or using an explicit
undefinedcheck if you want to allownullto be passed through:Option: explicit undefined check (only if null should be a valid value)
reset(data?: unknown): void { - const value = data ?? generateDefaultValue(this._schema, { refSchemas: this._refSchemas }); + const value = data === undefined + ? generateDefaultValue(this._schema, { refSchemas: this._refSchemas }) + : data; this._tree.setValue('', value); this._tree.commit(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/table/row/RowModelImpl.ts` around lines 123 - 127, The reset method currently treats null the same as undefined because it uses the nullish coalescing operator; decide whether null should be preserved or treated as "use defaults" and implement one of two fixes: (A) if null should be preserved, change the value selection to use an explicit undefined check so that reset(null) passes null through to this._tree.setValue('', data) instead of calling generateDefaultValue; (B) if null should be treated as "no value", add a short JSDoc comment on reset (and/or a runtime guard) stating that null is treated as a request to generate defaults and leave the existing nullish coalescing behavior; reference the reset method, generateDefaultValue, and this._tree.setValue when making the change.README.md (2)
331-331:TypedRowModel<S>description omitscommitandrevert.The
#### RowModeltable documents bothcommit()andrevert(), but theTypedRowModel<S>row in the Type Utilities table only listsroot,getValue,setValue,getPlainValue, andreset. Users may not realise those lifecycle methods are also accessible on a typed model. Consider adding them (or a brief note like "…pluscommit/revert"):📝 Suggested fix
-| `TypedRowModel<S>` | RowModel with typed `root`, `getValue`, `setValue`, `getPlainValue`, `reset` | +| `TypedRowModel<S>` | RowModel with typed `root`, `getValue`, `setValue`, `getPlainValue`, `reset`, `commit`, `revert` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 331, Update the README Type Utilities table entry for TypedRowModel<S> to include the lifecycle methods commit and revert (e.g., extend the description to "RowModel with typed root, getValue, setValue, getPlainValue, reset, plus commit/revert") so it matches the RowModel documentation; modify the TypedRowModel<S> row text to explicitly mention commit and revert to make the API surfaces consistent and discoverable.
265-277: Consider documenting return values forfindandfindIndex.
findreturns the matched node orundefined(if no match), andfindIndexreturns-1(if no match) — mirroring native JSArraysemantics. Noting this explicitly in the table would save callers a trip to source/tests:📝 Suggested clarification
-| `find(predicate)` | Find first element matching predicate | -| `findIndex(predicate)` | Find index of first element matching predicate | +| `find(predicate)` | Find first element matching predicate, or `undefined` | +| `findIndex(predicate)` | Find index of first matching element, or `-1` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 265 - 277, The documentation for ArrayValueNode's methods is missing return-value details for find and findIndex; update the README table entry for ArrayValueNode (methods `find` and `findIndex`) to explicitly state that `find(predicate)` returns the matched node or `undefined` if no match, and that `findIndex(predicate)` returns the index or `-1` when no match (mirroring native JS Array semantics) so callers know the exact return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 53: Update the inline comment for row.root to reflect the typed schema
return type: change the inaccurate reference to ObjectValueNode and state that
when the schema is created with obj() the root is of type InferNode<S> (the
typed resolved variant) rather than the untyped ObjectValueNode; locate the
usage of row.root in the Quick Start example and replace the parenthetical type
note to mention InferNode<S> and/or “typed root node” so it aligns with the API
table.
- Line 52: The RowModel API reference table is missing the patches property
referenced earlier; update the RowModel section to include a "patches" entry
describing that RowModel.patches is an array of JSON Patch operations (used to
inspect or apply diffs), indicate its type (e.g., Array/JSONPatchOperation[]),
and a short description and example usage so users can find row.patches in the
reference alongside root, reset, commit, and revert; locate the RowModel table
and add the patches row referencing RowModel.patches to match the Quick Start
usage.
---
Nitpick comments:
In `@README.md`:
- Line 331: Update the README Type Utilities table entry for TypedRowModel<S> to
include the lifecycle methods commit and revert (e.g., extend the description to
"RowModel with typed root, getValue, setValue, getPlainValue, reset, plus
commit/revert") so it matches the RowModel documentation; modify the
TypedRowModel<S> row text to explicitly mention commit and revert to make the
API surfaces consistent and discoverable.
- Around line 265-277: The documentation for ArrayValueNode's methods is missing
return-value details for find and findIndex; update the README table entry for
ArrayValueNode (methods `find` and `findIndex`) to explicitly state that
`find(predicate)` returns the matched node or `undefined` if no match, and that
`findIndex(predicate)` returns the index or `-1` when no match (mirroring native
JS Array semantics) so callers know the exact return behavior.
In `@src/model/table/row/RowModelImpl.ts`:
- Around line 123-127: The reset method currently treats null the same as
undefined because it uses the nullish coalescing operator; decide whether null
should be preserved or treated as "use defaults" and implement one of two fixes:
(A) if null should be preserved, change the value selection to use an explicit
undefined check so that reset(null) passes null through to
this._tree.setValue('', data) instead of calling generateDefaultValue; (B) if
null should be treated as "no value", add a short JSDoc comment on reset (and/or
a runtime guard) stating that null is treated as a request to generate defaults
and leave the existing nullish coalescing behavior; reference the reset method,
generateDefaultValue, and this._tree.setValue when making the change.
|
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.
Summary by cubic
Adds array search to ArrayValueNode and a simple one-call reset for RowModel. Also exposes a typed root node on RowModel for easier navigation.
).Written for commit 48998f2. Summary will update on new commits.