fix: show disabled file chip when file no longer exists in timeline#19045
fix: show disabled file chip when file no longer exists in timeline#19045bugisthegod wants to merge 2 commits intotwentyhq:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR improves the timeline “Files” field diff rendering by correctly handling cases where a previously-uploaded file no longer exists on the record, ensuring the UI no longer shows a “clickable but broken” file chip.
Changes:
- Add
isDeletedmarker toFieldFilesValueand propagate it for missing files in timeline diffs. - In timeline diff rendering, resolve file items against the live record’s current files list (use current signed URL if present; mark as deleted if not).
- Update
FileChipto render a disabled/deleted visual state (strike-through) and show a “File no longer exists” tooltip.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/twenty-front/src/modules/ui/field/display/components/FileChip.tsx | Adds deleted/disabled chip rendering and tooltip behavior for missing files. |
| packages/twenty-front/src/modules/object-record/record-field/ui/types/FieldMetadata.ts | Extends FieldFilesValue with optional isDeleted flag. |
| packages/twenty-front/src/modules/activities/timeline-activities/rows/main-object/components/EventFieldDiffValueEffect.tsx | Resolves timeline diff file entries against the current record’s files and marks missing ones as deleted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {isDeleted && | ||
| shouldShowTooltip && | ||
| createPortal( | ||
| <AppTooltip | ||
| anchorSelect={tooltipAnchorSelect} | ||
| content={t`File no longer exists`} | ||
| delay={TooltipDelay.shortDelay} | ||
| noArrow | ||
| place={TooltipPosition.Top} | ||
| positionStrategy="fixed" | ||
| />, | ||
| document.body, | ||
| )} |
There was a problem hiding this comment.
AppTooltip is only mounted after onMouseEnter sets shouldShowTooltip, but no isOpen prop is provided. With react-tooltip, mounting the tooltip after the hover event often means it won’t show on the first hover because listeners are attached after the event. Consider either rendering the tooltip whenever isDeleted is true (letting react-tooltip manage hover), or keep the conditional render but pass isOpen={shouldShowTooltip} (similar to OverflowingTextWithTooltip) so it reliably appears on first hover.
There was a problem hiding this comment.
Make sense. Based on my testing, it does indeed display less promptly when I move the mouse quicly.
| if ( | ||
| fieldMetadataItem.type === FieldMetadataType.FILES && | ||
| isDefined(recordStore) && | ||
| Array.isArray(diffRecord) | ||
| ) { | ||
| const currentFiles = Array.isArray(recordStore[fieldMetadataItem.name]) | ||
| ? (recordStore[fieldMetadataItem.name] as FieldFilesValue[]) | ||
| : []; | ||
| const currentFileMap = new Map( | ||
| currentFiles.map((file) => [file.fileId, file]), | ||
| ); | ||
|
|
||
| fieldValue = (diffRecord as FieldFilesValue[]).map((file) => { | ||
| const currentFile = currentFileMap.get(file.fileId); | ||
| if (isDefined(currentFile)) { | ||
| return currentFile; | ||
| } | ||
| return { ...file, isDeleted: true, url: undefined }; | ||
| }); | ||
| } | ||
|
|
||
| const forgedObjectRecord = { | ||
| __typename: mainObjectMetadataItem.nameSingular, | ||
| id: diffArtificialRecordStoreId, | ||
| [fieldMetadataItem.name]: diffRecord, | ||
| [fieldMetadataItem.name]: fieldValue, | ||
| }; | ||
|
|
||
| setRecordStore(forgedObjectRecord); | ||
| }, [ | ||
| diffRecord, | ||
| diffArtificialRecordStoreId, | ||
| fieldMetadataItem.name, | ||
| fieldMetadataItem.type, | ||
| mainObjectMetadataItem.nameSingular, | ||
| setRecordStore, | ||
| recordStore, | ||
| ]); |
There was a problem hiding this comment.
The effect depends on the entire recordStore object, so any change to the record (even unrelated fields) will re-run the effect and call setRecordStore again. This can cause extra renders in the timeline. Consider narrowing the dependency to just the files field value you read (e.g., recordStore?.[fieldMetadataItem.name]) or memoizing the computed currentFileMap from that field only.
There was a problem hiding this comment.
It doesn't make sense. After I debug it, if we changed some fields (like city/link), they all go to useEffect(). even though the dependency changes from recordStore to recordStore?.[fieldMetadataItem.name]. I think there is no difference for them.
| let fieldValue: typeof diffRecord = diffRecord; | ||
|
|
||
| if ( | ||
| fieldMetadataItem.type === FieldMetadataType.FILES && | ||
| isDefined(recordStore) && | ||
| Array.isArray(diffRecord) | ||
| ) { |
There was a problem hiding this comment.
This code path handles Array.isArray(diffRecord) (FILES diffs), but diffRecord is still typed as Record<string, any> | undefined, which doesn’t reflect arrays and forces repeated casts. Consider widening the diffRecord prop type (e.g., unknown/any or a union including FieldFilesValue[]) so TypeScript matches the shapes this effect supports.
There was a problem hiding this comment.
It was previous-exsiting. It depends on maintainer.
Fixes: #18943
Follow-up pr: #19001
Summary:
Recording.at.2026-03-27.13.35.31.mp4