-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handling of nested array props overriding #133
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success219 tests passing in 7 suites. Report generated by 🧪jest coverage report action from 78813b8 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
===========================================
- Coverage 100.00% 91.50% -8.50%
===========================================
Files 14 18 +4
Lines 4565 5192 +627
Branches 1082 1145 +63
===========================================
+ Hits 4565 4751 +186
- Misses 0 431 +431
- Partials 0 10 +10 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/converter.ts (1)
377-379
: The changes effectively address the nested array props issue.The solution elegantly fixes the nested array properties overriding by:
- Ensuring proper validation before accessing array elements
- Adding an early return for regular selectors to prevent unnecessary processing
This maintains the JSON template engine's integrity while fixing the specific issue without introducing breaking changes.
Also applies to: 88-88
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 378-379: src/utils/converter.ts#L378-L379
Added lines #L378 - L379 were not covered by tests🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
[warning] 378-378: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 379-379: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 377-379: 🌿 Branch is not covered
Warning! Not covered branch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/converter.ts
(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/utils/converter.ts
[warning] 378-379: src/utils/converter.ts#L378-L379
Added lines #L378 - L379 were not covered by tests
🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
src/utils/converter.ts
[warning] 378-378: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 379-379: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 377-379: 🌿 Branch is not covered
Warning! Not covered branch
🔇 Additional comments (2)
src/utils/converter.ts (2)
88-88
: LGTM! Good reordering of variable declaration.Moving the variable declaration after validation is a logical improvement as it ensures we validate the array elements before accessing them.
377-379
: Add test coverage for the new conditional.The new conditional that handles regular selectors is a crucial part of fixing nested array props overriding, but it lacks test coverage. This could lead to potential regressions.
Let's verify the impact of this change:
Would you like me to help generate test cases to cover this new conditional? This should include:
- Test case with regular selectors that trigger the early return
- Test case with nested array properties to verify the fix
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 378-379: src/utils/converter.ts#L378-L379
Added lines #L378 - L379 were not covered by tests🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
[warning] 378-378: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 379-379: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 377-379: 🌿 Branch is not covered
Warning! Not covered branch
|
What are the changes introduced in this PR?
Before:
Input:
Output:
After fix:
Output:
Also now the following works and previously it was giving an error:
What is the related Linear task?
Resolves INT-3184
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new generic utility introduced or modified. Please explain the changes.
N/A
Any technical or performance related pointers to consider with the change?
N/A
@coderabbitai review
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
Note: These changes are primarily internal improvements that do not directly impact end-user functionality.