Fix/Required not working for image upload component #3893
Conversation
📝 WalkthroughWalkthroughAdds required-field validation for the ImageUpload component: a new validation hook, integration into the component, three i18n entries (en/nb/nn), and an end-to-end test that verifies the required-image behavior on navigation and after upload. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
Magnusrm
added
kind/bug
Magnusrm
added
backport
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/e2e/integration/component-library/image-upload.ts (1)
79-97: End‑to‑end coverage for required image behavior is solid; consider minor enhancementsThe test correctly:
- Marks the ImageUpload as required and enables
validateOnNext.- Verifies the required error appears when clicking next with no image.
- Uploads and saves an image, then verifies the error is cleared.
Two optional improvements:
- Also assert on navigation behavior (e.g., that you remain on the same page before upload, and can move on after upload) to align directly with the original bug description about navigation being allowed.
- Consider using an existing page object / helper (if available) for the "Next" button instead of the hard‑coded
/next/imatcher to avoid coupling to a specific language string.src/layout/ImageUpload/index.tsx (1)
7-42: Hook delegation from layout definition is functionally OK, but Sonar warning should be handled explicitly
useEmptyFieldValidationdelegates to the custom hookuseValidateRequiredImageUpload, which is the right place for the actual hook logic, and functionally this should work as long as callers invokeuseEmptyFieldValidationfrom a stable hook context (which matches how other layout defs are typically used).However, static analysis flags this as "calling a hook from a class component" because the call appears inside a class, even though
ImageUploadis a layout definition and not a React.Component subclass. To keep Sonar/ESLint noise under control, consider one of:
Add an inline disable with a short justification, e.g.:
// eslint-disable-next-line react-hooks/rules-of-hooks useEmptyFieldValidation(baseComponentId: string): ComponentValidation[] { return useValidateRequiredImageUpload(baseComponentId); }Or, if there is an established pattern elsewhere in this repo for calling hooks from layout defs, align with that pattern (for example, by centralizing the hook call in a top‑level helper function that Sonar already knows is safe, and having the class method just forward the result).
Functionally this looks correct; the main concern is resolving the static analysis warning in a controlled way.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx(1 hunks)src/layout/ImageUpload/index.tsx(2 hunks)test/e2e/integration/component-library/image-upload.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/language/texts/nb.tssrc/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsxsrc/layout/ImageUpload/index.tsxtest/e2e/integration/component-library/image-upload.tssrc/language/texts/nn.tssrc/language/texts/en.ts
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/language/texts/nb.tssrc/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsxsrc/layout/ImageUpload/index.tsxtest/e2e/integration/component-library/image-upload.tssrc/language/texts/nn.tssrc/language/texts/en.ts
src/layout/*/{config,Component,index,config.generated}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Layout components must follow standardized structure with
config.ts,Component.tsx,index.tsx, andconfig.generated.tsfiles
Files:
src/layout/ImageUpload/index.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: lassopicasso
Repo: Altinn/app-frontend-react PR: 3654
File: src/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsx:49-53
Timestamp: 2025-10-14T09:01:40.985Z
Learning: In the Altinn app-frontend-react codebase, when data integrity is guaranteed through business logic and parent components guard rendering (e.g., `storedImage ? <Component /> : undefined`), non-null assertions on guaranteed fields like `storedImage!.data?.filename` are acceptable and preferred over optional chaining with fallbacks.
📚 Learning: 2025-11-25T12:53:54.399Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:53:54.399Z
Learning: Applies to **/*.test.{ts,tsx} : Use `renderWithProviders` from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/layout/ImageUpload/index.tsxtest/e2e/integration/component-library/image-upload.ts
🧬 Code graph analysis (3)
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx (3)
src/features/validation/index.ts (1)
ComponentValidation(151-153)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/utils/layout/DataModelLocation.tsx (1)
useIndexedId(117-120)
src/layout/ImageUpload/index.tsx (2)
src/features/validation/index.ts (1)
ComponentValidation(151-153)src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx (1)
useValidateRequiredImageUpload(6-23)
test/e2e/integration/component-library/image-upload.ts (1)
test/e2e/support/apps/component-library/uploadImageAndVerify.ts (1)
uploadImageAndVerify(8-23)
🪛 GitHub Check: SonarCloud Code Analysis
src/layout/ImageUpload/index.tsx
[warning] 41-41: React Hook "useValidateRequiredImageUpload" cannot be called in a class component. React Hooks must be called in a React function component or a custom React Hook function.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install
🔇 Additional comments (4)
src/language/texts/en.ts (1)
211-223: New image upload required error key looks consistentKey naming and message fit the existing
image_upload_component.*namespace and match the new validation logic and tests.src/language/texts/nb.ts (1)
212-224: Bokmål translation aligns with usageThe added
image_upload_component.error_requiredkey and text match the e2e assertion and surrounding image upload messages.src/language/texts/nn.ts (1)
212-224: Nynorsk translation key is consistentThe
image_upload_component.error_requiredentry is correctly added and consistent with the en/nb variants and nearby image upload messages.src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx (1)
1-23: Required‑image validation hook is correctly wiredThe hook cleanly ties the
requiredflag and attachment list together and emits aComponentValidationwith the newimage_upload_component.error_requiredkey, severity, source, and category set appropriately. The implementation fits the existing validation infrastructure and matches the intended behavior.
lassopicasso
left a comment
lassopicasso
left a comment
There was a problem hiding this comment.
Good job, I tested it also.
Just a suggestion about adjustment for the new hook.
And should we take into account what sonarcloud is complaining about: "React Hook "useValidateRequiredImageUpload" cannot be called in a class component."?
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Lars <74791975+lassopicasso@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx
🧬 Code graph analysis (1)
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx (4)
src/features/validation/index.ts (1)
ComponentValidation(151-153)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/utils/layout/NodesContext.tsx (1)
NodesInternal(407-509)src/utils/layout/DataModelLocation.tsx (1)
useIndexedId(117-120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (3)
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx (3)
1-4: LGTM: Imports are clean and well-organized.All imports are properly typed and used in the implementation below.
6-8: LGTM: Hook setup follows React patterns correctly.The hooks are called unconditionally, and the type checking via
useItemWhenTypeensures type safety.
14-23: LGTM: Validation object is correctly structured.The validation object properly implements the
ComponentValidationinterface with appropriate severity, source, and category for a required field validation.
src/layout/ImageUpload/hooks/useValidateRequiredImageUpload.tsx
Outdated
Show resolved
Hide resolved
|
Magnusrm
deleted the
fix/required-not-working-for-image-upload-component
branch
github-project-automation
bot
moved this from 🔎 In review
to ✅ Done
in Team Altinn Studio
2 participants
Description
Added required validation handling that checks if the instance has any attachments that stems from the
ImageUpload component.
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.