Feat/thumbnail #3797
Feat/thumbnail #3797
Conversation
📝 WalkthroughWalkthroughAdds thumbnail support to the file upload table: new translation keys for "Thumbnail", a CSS module and React AttachmentThumbnail component, FileTable/FileTableRow updated to render thumbnails conditionally, and a new metadata field on IData to store attachment metadata. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Points to review closely:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 6
🧹 Nitpick comments (5)
src/types/shared.ts (2)
24-27: Make metadata fields required (call sites assume presence)Downstream code checks
meta.key === 'thumbnailLink'. Optionalkey/valueweakens safety. Make both required (and optionally narrow key type).-export interface IMetadata { - key?: string; - value?: string; -} +export type MetadataKey = 'thumbnailLink' | (string & {}); +export interface IMetadata { + key: MetadataKey; + value: string; +}
59-59: Sanitize/validate thumbnail URL before useIf
metadatawill carry external URLs, ensure the renderer only accepts http(s) and same-origin (or approved) hosts to avoid mixed content/SSRF/image beacons. Add a small helper to validate before rendering.Would you like me to add a safe
isTrustedThumbnailUrl(url: string)and wire it in AttachmentThumbnail?src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.module.css (1)
8-13: Prevent layout shift and improve perceived performanceAdd explicit width/height (or aspect-ratio) so cells reserve space before images load.
.thumbnail { - max-width: 100px; - max-height: 70px; + inline-size: 100px; + block-size: 70px; + aspect-ratio: 100 / 70; object-fit: contain; border-radius: 2px; } .thumbnailMobile { - max-width: 80px; - max-height: 60px; + inline-size: 80px; + block-size: 60px; + aspect-ratio: 80 / 60; object-fit: contain; border-radius: 2px; }Also applies to: 15-20
src/layout/FileUpload/FileUploadTable/FileTable.tsx (1)
90-93: Handle missing tagTitle gracefullyIf
tagTitleis undefined,<Lang id={undefined} />may render poorly. Fallback or skip the column.- {hasTag && !mobileView && ( + {hasTag && !mobileView && tagTitle && ( <th> <Lang id={tagTitle} /> </th> )}src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx (1)
49-51: Remove redundant null check.The
thumbnailUrlwill always be a string since bothgetDataElementUrlandmakeUrlRelativeIfSameDomainalways return strings. This check is redundant and can be safely removed.Apply this diff to remove the unnecessary check:
const thumbnailUrl = makeUrlRelativeIfSameDomain(getDataElementUrl(instanceId, thumbnailDataElement.id, language)); - if (!thumbnailUrl) { - return null; - } - return (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.module.css(1 hunks)src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTable.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTableRow.tsx(1 hunks)src/types/shared.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.module.css
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/language/texts/nb.tssrc/types/shared.tssrc/language/texts/nn.tssrc/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsxsrc/layout/FileUpload/FileUploadTable/FileTableRow.tsxsrc/language/texts/en.tssrc/layout/FileUpload/FileUploadTable/FileTable.tsx
🧬 Code graph analysis (4)
src/types/shared.ts (2)
src/types/index.ts (1)
LooseAutocomplete(51-51)src/features/attachments/types.ts (1)
FileScanResult(1-1)
src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx (5)
src/features/attachments/index.ts (3)
IAttachment(22-22)isAttachmentUploaded(29-31)UploadedAttachment(20-20)src/features/instance/InstanceContext.tsx (2)
useInstanceDataElements(139-143)useLaxInstanceId(58-62)src/features/language/LanguageProvider.tsx (1)
useCurrentLanguage(79-79)src/utils/urls/urlHelper.ts (1)
makeUrlRelativeIfSameDomain(107-117)src/utils/urls/appUrlHelper.ts (1)
getDataElementUrl(78-79)
src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (13)
src/features/attachments/index.ts (2)
IAttachment(22-22)isAttachmentUploaded(29-31)src/utils/layout/hooks.ts (1)
useExternalItem(16-22)src/features/pdf/PDFWrapper.tsx (1)
usePdfModeActive(11-15)src/utils/attachmentsUtils.ts (1)
getSizeWithUnit(72-82)src/core/contexts/TaskOverrides.tsx (1)
useTaskOverrides(32-32)src/features/attachments/types.ts (1)
FileScanResults(3-8)src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx (1)
AttachmentThumbnail(15-65)src/layout/Summary2/CommonSummaryComponents/EditButton.tsx (1)
EditButton(48-116)src/layout/FileUpload/FileUploadTable/AttachmentFileName.tsx (1)
AttachmentFileName(16-56)src/theme/altinnAppTheme.tsx (1)
AltinnPalette(2-25)src/components/AltinnLoader.tsx (1)
AltinnLoader(17-29)src/layout/FileUpload/FileUploadTable/FileTableRowContext.tsx (1)
useFileTableRow(12-12)src/layout/FileUpload/FileUploadTable/FileTableButtons.tsx (1)
FileTableButtons(26-109)
src/layout/FileUpload/FileUploadTable/FileTable.tsx (3)
src/features/attachments/index.ts (2)
IAttachment(22-22)isAttachmentUploaded(29-31)src/utils/formComponentUtils.ts (1)
atLeastOneTagExists(20-29)src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (1)
FileTableRow(32-126)
⏰ 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)
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (6)
src/language/texts/en.ts (1)
77-77: LGTMKey name and placement align with existing patterns.
src/layout/FileUpload/FileUploadTable/FileTable.tsx (1)
99-103: A11y: column header aligns to data cell semanticsThumb column is purely decorative; ensure cells use
role="presentation"or images have emptyaltwhen decorative. Verify AttachmentThumbnail sets appropriatealt.Would you like a quick sweep to enforce
alt=""andaria-hiddenwhere appropriate?src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx (2)
1-13: LGTM!The imports and interface definition are well-structured and appropriate for the component's functionality.
53-64: LGTM!The rendering logic is well-implemented with proper accessibility (alt text), conditional styling for mobile/desktop views, and test ID for testing purposes.
src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (2)
13-13: LGTM!The new import and optional
hasImagesprop are correctly integrated into the component interface.Also applies to: 29-29
32-39: LGTM!The
hasImagesprop is properly destructured and used for conditional rendering of the thumbnail cell.
| const calculateColSpan = () => { | ||
| if (mobileView) { | ||
| return hasImages ? 4 : 3; | ||
| } | ||
| return hasImages ? 7 : 6; | ||
| }; |
There was a problem hiding this comment.
colSpan is off; causes misalignment, especially on mobile
Compute colSpan from the same conditions as headers to avoid drift.
- const calculateColSpan = () => {
- if (mobileView) {
- return hasImages ? 4 : 3;
- }
- return hasImages ? 7 : 6;
- };
+ const calculateColSpan = () => {
+ let count = 1; // Name
+ if (!mobileView) count += 1; // File size
+ if (hasTag && !mobileView) count += 1; // Tag
+ if (!pdfModeActive && !mobileView) count += 1; // Status
+ if (hasImages) count += 1; // Thumbnail
+ if (!pdfModeActive) count += 1; // Delete/Action
+ return count;
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const calculateColSpan = () => { | |
| if (mobileView) { | |
| return hasImages ? 4 : 3; | |
| } | |
| return hasImages ? 7 : 6; | |
| }; | |
| const calculateColSpan = () => { | |
| let count = 1; // Name | |
| if (!mobileView) count += 1; // File size | |
| if (hasTag && !mobileView) count += 1; // Tag | |
| if (!pdfModeActive && !mobileView) count += 1; // Status | |
| if (hasImages) count += 1; // Thumbnail | |
| if (!pdfModeActive) count += 1; // Delete/Action | |
| return count; | |
| }; |
🤖 Prompt for AI Agents
In src/layout/FileUpload/FileUploadTable/FileTable.tsx around lines 62 to 67,
the calculateColSpan function returns hardcoded numbers that drift from the
actual headers and cause misalignment on mobile; change it to compute colspan
from the same visible headers used to render the table (e.g., derive the headers
array based on mobileView and hasImages and return headers.length or
visibleHeaders.length) instead of returning fixed constants so colspan always
matches header count.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/layout/FileUpload/FileUploadTable/FileTable.tsx (1)
62-67:calculateColSpanis inconsistent with actual column count (misaligned edit rows).The current hard-coded values (
mobileView ? 4/3 : 7/6) don’t match the real number of<td>cells rendered inFileTableRowacross combinations ofmobileView,hasTag,pdfModeActive,hasImages, andisSummary. This causes the edit row (<EditWindowComponent>) to span too many columns, particularly on mobile and when tags/thumbnails are toggled.Recommend deriving
colSpanfrom the same conditions used to render row cells so it always matches:- const calculateColSpan = () => { - if (mobileView) { - return hasImages ? 4 : 3; - } - return hasImages ? 7 : 6; - }; + const calculateColSpan = () => { + let count = 1; // Name + + if (!mobileView) { + count += 1; // File size + if (hasTag) { + count += 1; // Tag + } + if (!pdfModeActive) { + count += 1; // Status + } + } + + if (hasImages) { + count += 1; // Thumbnail + } + + const hasActionColumn = !isSummary || (!pdfModeActive && isSummary); + if (hasActionColumn) { + count += 1; // Delete/Edit actions + } + + return count; + };This keeps
colSpanin lockstep with both header and body cells for all view modes.
🧹 Nitpick comments (3)
src/layout/FileUpload/FileUploadTable/FileTable.tsx (1)
43-52: Optional: precompute options lookup map to avoid repeatedfind.
labeldoes anoptions?.findfor each attachment; with many attachments this is O(n²) over options×attachments. Not a bug, just slightly wasteful.If options can grow, consider precomputing a value→label map once per render:
- const label = (attachment: IAttachment) => { + const optionLabelByValue = React.useMemo( + () => new Map(options?.map((o) => [o.value, o.label])), + [options], + ); + + const label = (attachment: IAttachment) => { if (!isAttachmentUploaded(attachment)) { return undefined; } const firstTag = attachment.data.tags && attachment.data.tags[0]; - return options?.find((option) => option.value === firstTag)?.label; + return (firstTag && optionLabelByValue.get(firstTag)) || undefined; };Not required for correctness, but keeps render cost flat as counts grow.
src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (2)
23-39:hasImagesprop usage looks good; consider defaulting tofalseon destructuring.Using an optional
hasImages?: booleanand checking{hasImages && ...}is safe (undefined is falsy) and keeps older call sites compatible. If you want to make intent explicit and help future readers, you could default it during destructuring:-export function FileTableRow({ +export function FileTableRow({ baseComponentId, attachment, mobileView, tagLabel, isSummary, - hasImages, + hasImages = false, }: IFileUploadTableRowProps) {Not required, but clarifies that
hasImagesis effectively boolean.
129-185: Clean up unreachable / redundant conditions inNameCellmobile branch.Inside the
mobileView && (...)block:
tagLabel && mobileViewrepeats themobileViewcheck, which is already guaranteed true.hasTag && !mobileViewcan never be true here (we’re in themobileViewbranch), so that block is dead code.You can simplify and drop the unreachable part without behavior change:
- {mobileView && ( + {mobileView && ( <div style={{ color: AltinnPalette.grey, }} > {attachment.uploaded ? ( <div> - {tagLabel && mobileView && ( + {tagLabel && ( <div> <Lang id={tagLabel} /> </div> )} - {`${readableSize} ${mobileView ? uploadStatus : ''}`} - {hasTag && !mobileView && ( - <div data-testid='status-success'> - <Lang id='form_filler.file_uploader_list_status_done' /> - </div> - )} + {`${readableSize} ${uploadStatus}`} </div> ) : (This keeps the mobile behavior identical while reducing noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTable.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTableRow.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/language/texts/nb.ts
- src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx
- src/language/texts/nn.ts
🧰 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/FileUpload/FileUploadTable/FileTable.tsxsrc/layout/FileUpload/FileUploadTable/FileTableRow.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/FileUpload/FileUploadTable/FileTable.tsxsrc/layout/FileUpload/FileUploadTable/FileTableRow.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-14T09:01:40.985Z
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.
Applied to files:
src/layout/FileUpload/FileUploadTable/FileTable.tsx
🧬 Code graph analysis (1)
src/layout/FileUpload/FileUploadTable/FileTable.tsx (5)
src/features/attachments/index.ts (2)
IAttachment(22-22)isAttachmentUploaded(29-31)src/features/pdf/PDFWrapper.tsx (1)
usePdfModeActive(11-15)src/utils/formComponentUtils.ts (1)
atLeastOneTagExists(20-29)src/layout/FileUpload/FileUploadTable/FileTableRowContext.tsx (2)
FileTableRowContext(3-7)FileTableRowProvider(11-11)src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (1)
FileTableRow(32-127)
|
/publish |
PR release:
|
…nto Feat/Thumbnail
walldenfilippa
added
the
backport-ignore
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| <Lang id={tagLabel} /> | ||
| </div> | ||
| )} | ||
| {`${readableSize} ${mobileView ? uploadStatus : ''}`} |
Check warning
Code scanning / CodeQL
Useless conditional Warning
walldenfilippa
added
the
ignore-for-release
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/types/shared.ts (1)
24-27: Consider making thekeyfield required inIMetadata.The
IMetadatainterface has bothkeyandvalueas optional, allowing metadata entries like{}or{value: 'bar'}without a key. Typically, metadata requires at least a key identifier for lookup purposes.Consider this refinement:
export interface IMetadata { - key?: string; + key: string; value?: string; }This ensures every metadata entry has a key while keeping the value optional, which aligns better with the usage pattern
metadata?.find((meta) => meta.key === 'thumbnailLink')seen in the codebase. However, if the backend API contract requires both fields to be optional, the current definition is acceptable.src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (1)
101-108: Consider adding visual feedback for empty thumbnail cells.The thumbnail cell is always rendered when
hasImagesis true, but theAttachmentThumbnailcomponent may returnnullfor attachments without thumbnail metadata. This creates empty<td>elements that maintain table alignment but lack visual indicators.Consider wrapping the thumbnail in a container that provides consistent visual presentation:
{hasImages && ( <td> <AttachmentThumbnail attachment={attachment} mobileView={mobileView} /> {/* Or conditionally render a placeholder */} </td> )}Alternatively, enhance the
AttachmentThumbnailcomponent to render a placeholder (e.g., an empty state icon or space preserver) instead of returningnullwhen no thumbnail is available. This would ensure consistent table cell appearance across all rows when the thumbnail column is visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTable.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTableRow.tsx(1 hunks)src/types/shared.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/language/texts/nn.ts
- src/layout/FileUpload/FileUploadTable/FileTable.tsx
- src/language/texts/en.ts
- src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx
🧰 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/language/texts/nb.tssrc/types/shared.tssrc/layout/FileUpload/FileUploadTable/FileTableRow.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/language/texts/nb.tssrc/types/shared.tssrc/layout/FileUpload/FileUploadTable/FileTableRow.tsx
🧬 Code graph analysis (1)
src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (6)
src/features/attachments/index.ts (2)
IAttachment(22-22)isAttachmentUploaded(29-31)src/utils/attachmentsUtils.ts (1)
getSizeWithUnit(72-82)src/features/attachments/types.ts (1)
FileScanResults(3-8)src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx (1)
AttachmentThumbnail(15-58)src/layout/FileUpload/FileUploadTable/AttachmentFileName.tsx (1)
AttachmentFileName(16-56)src/layout/FileUpload/FileUploadTable/FileTableRowContext.tsx (1)
useFileTableRow(12-12)
⏰ 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)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/language/texts/nb.ts (1)
78-78: LGTM! Translation properly uses native Bokmål term.The Norwegian Bokmål translation correctly uses "Miniatyrbilde" instead of the English "Thumbnail", ensuring consistency with other localized strings in the application.
src/types/shared.ts (1)
59-59: Field addition looks good.The
metadatafield is appropriately optional and supports the thumbnail feature integration described in the PR. The inline comment clearly documents its purpose.src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (2)
1-30: Imports and interface changes look good.The new
AttachmentThumbnailimport and thehasImagesprop addition properly support the thumbnail feature. The interface follows TypeScript best practices with the optionalhasImagesfield.
84-92: File size cell properly added, resolving past misalignment issue.The file size cell at line 92 correctly addresses the column misalignment issue flagged in the previous review. The cell is conditionally rendered when
!mobileView, matching the header structure in FileTable.
walldenfilippa
added
kind/product-feature
|
/publish |
PR release:
|
|
new PR: #3898 |
2 participants
Description
Added metadata to the data element, along with functionality to store the metadata within it.
Additional functionality in app-lib (Altinn/app-lib-dotnet#1531) and localtest(Altinn/app-localtest#183). Storage is in process.
Design in figma, pre development:
Mobileview:
Current layout:
Mobilview:
Related Issue(s)
#792
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.