Expression validation: Keep running when values are undefined or null#4066
Expression validation: Keep running when values are undefined or null#4066olemartinorg merged 1 commit intomainfrom
undefined or null#4066Conversation
📝 WalkthroughWalkthroughA new helper function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
olemartinorg
added
the
backport
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/features/formData/FormDataWrite.tsx (1)
704-725: UseemptyArrayinstead of[]for consistent referential stability.Line 718 returns a new array literal
[], while line 707 correctly returnsemptyArray. The new array literal creates a fresh reference on each call, which could cause unnecessary re-renders in React components relying on referential equality.♻️ Proposed fix
const formData = v.dataModels[reference.dataType]?.debouncedCurrentData; if (!formData) { - return []; + return emptyArray; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/formData/FormDataWrite.tsx` around lines 704 - 725, The function selectAllPaths returns a fresh empty array literal in the branch where formData is falsy; replace that literal with the shared emptyArray constant to preserve referential stability (same pattern used earlier in the function) — update the return in selectAllPaths (the branch that currently returns []) to return emptyArray; no other behavior changes needed, and this keeps components using FormDataContext and functions like collectMatchingFieldPaths from triggering unnecessary re-renders.src/features/formData/FormDataWrite.test.ts (1)
8-18: Type assertion is pragmatic for test mocks, but consider a type-safe alternative.The
as FormDataContextcast at line 17 bypasses type checking. While this is a common pattern for unit tests and avoids verbose full mocks, it could mask type errors ifFormDataContextchanges. Consider using a partial type orPick<>to explicitly declare the subset being mocked.♻️ Optional: Type-safe mock approach
+type PartialFormDataContext = Pick<FormDataContext, 'dataModels'>; + function makeContext(formData?: object): FormDataContext { - return { + const partial: PartialFormDataContext = { dataModels: formData ? { [dataType]: { debouncedCurrentData: formData, }, } : {}, - } as FormDataContext; + }; + return partial as FormDataContext; }As per coding guidelines: "Avoid using
anyor type casting (as type) in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/formData/FormDataWrite.test.ts` around lines 8 - 18, The test helper makeContext currently force-casts the object to FormDataContext (avoid using "as" here); change the signature to return a narrower, type-safe shape such as Partial<FormDataContext> or Pick<FormDataContext, "dataModels"> and construct the object with a matching type (set dataModels with [dataType] and debouncedCurrentData) so no top-level type assertion is needed; update callers of makeContext in tests if necessary to accept the narrower return type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/features/formData/FormDataWrite.test.ts`:
- Around line 8-18: The test helper makeContext currently force-casts the object
to FormDataContext (avoid using "as" here); change the signature to return a
narrower, type-safe shape such as Partial<FormDataContext> or
Pick<FormDataContext, "dataModels"> and construct the object with a matching
type (set dataModels with [dataType] and debouncedCurrentData) so no top-level
type assertion is needed; update callers of makeContext in tests if necessary to
accept the narrower return type.
In `@src/features/formData/FormDataWrite.tsx`:
- Around line 704-725: The function selectAllPaths returns a fresh empty array
literal in the branch where formData is falsy; replace that literal with the
shared emptyArray constant to preserve referential stability (same pattern used
earlier in the function) — update the return in selectAllPaths (the branch that
currently returns []) to return emptyArray; no other behavior changes needed,
and this keeps components using FormDataContext and functions like
collectMatchingFieldPaths from triggering unnecessary re-renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df8b0b79-9bf5-4553-b171-7138fd8bf4f8
📒 Files selected for processing (2)
src/features/formData/FormDataWrite.test.tssrc/features/formData/FormDataWrite.tsx
|
olemartinorg
changed the title
undefined or null
github-project-automation
bot
moved this from 👷 In progress
to ✅ Done
in Team Altinn Studio
…d as valid paths (#4066) Co-authored-by: Ole Martin Handeland <git@olemartin.org>
|
✅ Automatic backport successful! A backport PR has been automatically created for the The release branch The cherry-pick was clean with no conflicts. Please review the backport PR when it appears. |
olemartinorg
changed the title
undefined or null
…d as valid paths (#4066) Co-authored-by: Ole Martin Handeland <git@olemartin.org>
2 participants
Description
In #3735 the expression validation system was refactored and introduced a way to find all the valid/specific paths for a data model binding, i.e. find every row in repeating group structures and outputting all the valid full paths inside. This included a check for null/undefined which seemed smart, but it caused problems as expression validation would simply not run when a value was cleared by the user - so older expression validations would linger. See the referenced thread for a reproduction.
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
Tests
Chores