chore: File attachment readability improvement#7051
Open
divyanshu-patil wants to merge 11 commits intoRocketChat:developfrom
Open
chore: File attachment readability improvement#7051divyanshu-patil wants to merge 11 commits intoRocketChat:developfrom
divyanshu-patil wants to merge 11 commits intoRocketChat:developfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds three utility functions (formatSize, getFileIcon, isDocumentFile) and updates the Reply attachment UI to detect document attachments, render a file icon, show formatted file size for document-only attachments, adjust attachment row layout/styling, and refine memoization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/containers/message/Components/Attachments/Reply.tsx (3)
164-167: Simplify redundant ternary expression.On line 165, the
isDocument ? formatSize(attachment.size ?? 0, 2) : textternary is inside a block that already guaranteesisDocumentis true (from the condition on line 164). The: textbranch is unreachable.♻️ Proposed simplification
if (isFileName && isDocument) { - const newText = isDocument ? formatSize(attachment.size ?? 0, 2) : text; - return <Text style={[styles.fileSizeText, { color: colors.fontSecondaryInfo }]}>{newText}</Text>; + return <Text style={[styles.fileSizeText, { color: colors.fontSecondaryInfo }]}>{formatSize(attachment.size ?? 0, 2)}</Text>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/message/Components/Attachments/Reply.tsx` around lines 164 - 167, The ternary inside the block guarded by if (isFileName && isDocument) is redundant; in the Reply component replace the unreachable conditional expression in the newText assignment (isDocument ? formatSize(attachment.size ?? 0, 2) : text) with a direct call to formatSize(attachment.size ?? 0, 2) so newText is assigned the formatted size and the returned <Text> (styles.fileSizeText) displays that value; update the code using the existing identifiers newText, formatSize, and attachment accordingly.
174-185: Memo comparison may be incomplete for new dependencies.The
Descriptioncomponent now depends onattachment.formatandattachment.size(viaisDocumentFileandformatSize), but the custom comparison function doesn't check these fields. If these values change, the component won't re-render.This is likely fine in practice since
formatandsizeare typically immutable after attachment creation, but worth noting for correctness.💡 Proposed update to memo comparison
(prevProps, nextProps) => { if (prevProps.attachment.text !== nextProps.attachment.text) { return false; } if (prevProps.attachment.title !== nextProps.attachment.title) { return false; } if (prevProps.attachment.type !== nextProps.attachment.type) { return false; } + if (prevProps.attachment.format !== nextProps.attachment.format) { + return false; + } + if (prevProps.attachment.size !== nextProps.attachment.size) { + return false; + } return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/message/Components/Attachments/Reply.tsx` around lines 174 - 185, The memo comparator for the Description/Reply attachment component only compares attachment.text, title, and type, so changes to attachment.format or attachment.size (used by isDocumentFile/formatSize) won't trigger re-renders; update the comparator (the anonymous function with signature (prevProps, nextProps) => { ... }) to also compare attachment.format and attachment.size (or replace the three-field checks with a shallow comparison of the attachment object) so updates to those fields cause the component to rerender.
241-243: Consolidate duplicateuseTheme()calls.
useTheme()is called twice: line 241 forcolorsand line 243 fortheme. These can be combined into a single destructuring.♻️ Proposed consolidation
- const { colors } = useTheme(); const [loading, setLoading] = useState(false); - const { theme } = useTheme(); + const { theme, colors } = useTheme(); const { baseUrl, user, id, e2e, isEncrypted } = useContext(MessageContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/message/Components/Attachments/Reply.tsx` around lines 241 - 243, Consolidate the duplicate useTheme() calls by replacing the two separate calls with a single destructure: call useTheme() once and extract both colors and theme (e.g., const { colors, theme } = useTheme()); remove the extra useTheme() invocation and update any references to colors and theme in the Reply component so they use the values from the single destructured object (look for useTheme, colors, and theme in Reply.tsx).app/lib/methods/getFileIcon.ts (1)
3-8: Consider case-insensitive extension matching.The
.endsWith('.pdf')check is case-sensitive, so files with uppercase extensions liketest.PDFwon't match. This is a minor edge case but could be worth handling for robustness.💡 Suggested improvement
export const getFileIcon = (filename: string): TIconsName => { - if (filename.endsWith('.pdf')) return 'adobe-reader-monochromatic'; + const lowerFilename = filename.toLowerCase(); + if (lowerFilename.endsWith('.pdf')) return 'adobe-reader-monochromatic'; // if (filename.endsWith('.doc') || filename.endsWith('.docx')) return 'another'; // if (filename.endsWith('.xls') || filename.endsWith('.xlsx')) return 'another'; // if (filename.endsWith('.zip')) return 'another'; return 'pin'; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/getFileIcon.ts` around lines 3 - 8, getFileIcon currently uses a case-sensitive filename.endsWith('.pdf') check which misses uppercase extensions like "TEST.PDF"; update the function (getFileIcon, parameter filename) to normalize the filename (e.g., convert to lowercase or use a case-insensitive comparison) before performing endsWith checks so '.pdf' matches regardless of case and apply the same normalization approach when you add other extension checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/methods/formatSize.ts`:
- Around line 1-11: The formatSize function can return NaN/undefined for
negative bytes; update the guard at the top of formatSize to handle non-positive
or invalid inputs (e.g., if bytes == null || bytes <= 0 return '0 Bytes') and
then clamp the computed index i to the valid range of sizes (use
Math.min(Math.max(i, 0), sizes.length - 1)) before using sizes[i]; this ensures
bytes, i, and sizes are safe and avoids NaN/undefined results.
---
Nitpick comments:
In `@app/containers/message/Components/Attachments/Reply.tsx`:
- Around line 164-167: The ternary inside the block guarded by if (isFileName &&
isDocument) is redundant; in the Reply component replace the unreachable
conditional expression in the newText assignment (isDocument ?
formatSize(attachment.size ?? 0, 2) : text) with a direct call to
formatSize(attachment.size ?? 0, 2) so newText is assigned the formatted size
and the returned <Text> (styles.fileSizeText) displays that value; update the
code using the existing identifiers newText, formatSize, and attachment
accordingly.
- Around line 174-185: The memo comparator for the Description/Reply attachment
component only compares attachment.text, title, and type, so changes to
attachment.format or attachment.size (used by isDocumentFile/formatSize) won't
trigger re-renders; update the comparator (the anonymous function with signature
(prevProps, nextProps) => { ... }) to also compare attachment.format and
attachment.size (or replace the three-field checks with a shallow comparison of
the attachment object) so updates to those fields cause the component to
rerender.
- Around line 241-243: Consolidate the duplicate useTheme() calls by replacing
the two separate calls with a single destructure: call useTheme() once and
extract both colors and theme (e.g., const { colors, theme } = useTheme());
remove the extra useTheme() invocation and update any references to colors and
theme in the Reply component so they use the values from the single destructured
object (look for useTheme, colors, and theme in Reply.tsx).
In `@app/lib/methods/getFileIcon.ts`:
- Around line 3-8: getFileIcon currently uses a case-sensitive
filename.endsWith('.pdf') check which misses uppercase extensions like
"TEST.PDF"; update the function (getFileIcon, parameter filename) to normalize
the filename (e.g., convert to lowercase or use a case-insensitive comparison)
before performing endsWith checks so '.pdf' matches regardless of case and apply
the same normalization approach when you add other extension checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1889740-9c81-41c8-8209-c4d30e84367f
⛔ Files ignored due to path filters (1)
app/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
app/containers/message/Components/Attachments/Reply.tsxapp/lib/methods/formatSize.tsapp/lib/methods/getFileIcon.ts
📜 Review details
🔇 Additional comments (2)
app/containers/message/Components/Attachments/Reply.tsx (2)
116-124: LGTM!The
isDocumentFilehelper is well-structured using a switch statement that makes it easy to extend for additional document formats in the future.
283-287: LGTM!The PDF icon rendering is well-implemented with proper conditional checks and theme-aware styling. The icon display integrates cleanly with the existing attachment layout.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/containers/message/Components/Attachments/Reply.tsx`:
- Line 247: Remove the render-time console.log that prints the attachment object
in the Reply component: locate the console.log(attachment,
'============================') inside the Reply component (or the render
function/method that uses the attachment prop) and delete it so attachments are
no longer logged on every render; if debugging info is still needed, replace
with a debug-only guard (e.g., conditional on a dev flag) or use a
non-production logger instead.
- Around line 282-285: The icon is chosen from attachment.title but document
detection uses attachment.format, so change the icon source to use
attachment.format (not title) when calling getFileIcon inside the conditional
that checks isDocumentFile(attachment) — e.g., pass attachment.format (or a
normalized extension derived from it) to getFileIcon in the JSX where
attachment.type === 'file' && isDocumentFile(attachment). Also update
app/lib/methods/getFileIcon.ts to accept plain format values like "pdf" or "csv"
(and fallback from dotted filenames) so getFileIcon(format) returns the correct
icon for formats as well as filenames.
- Around line 155-158: The reply preview currently shows "0 B" when
attachment.size is missing because formatSize is called with 0; update the
rendering in Reply.tsx (the component handling isFileName && isDocument) to only
call and render formatSize(attachment.size, 2) when attachment.size is not
null/undefined (e.g., attachment.size != null); if size is absent, render
nothing (omit the Text node) or an empty/placeholder string so unknown sizes are
not shown as "0 B".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abc2765a-abf3-4175-b10e-09a04f6696f6
📒 Files selected for processing (3)
app/containers/message/Components/Attachments/Reply.tsxapp/lib/methods/getFileIcon.tsapp/lib/methods/isDocumentFile.ts
divyanshu-patil
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/containers/message/Components/Attachments/Reply.tsx`:
- Around line 153-156: isDocumentFile(attachment) currently ignores the
filename-suffix fallback used for attachmentFormat, so attachments with no
IAttachment.format (e.g., report.pdf) miss the document icon and size label;
update the code to resolve the effective format first (use attachmentFormat
which falls back to the title/file suffix) and then check that resolved format
in isDocumentFile (or modify isDocumentFile to accept/compute the same
fallback). Apply the same change for the other occurrence that checks documents
so both icon and size label use the unified fallback.
- Around line 155-156: The current conditional in Reply.tsx (around isFileName
and isDocument checks) uses truthiness for attachment.size which hides valid
zero-length files; change the guard to explicitly check for null/undefined
(e.g., attachment.size != null or typeof attachment.size === 'number') so that
size === 0 is rendered by the Text component using formatSize; update the
condition that includes isFileName and isDocument to use this explicit null
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e50e277-40aa-46cc-bf64-86dadd306de5
📒 Files selected for processing (2)
app/containers/message/Components/Attachments/Reply.tsxapp/lib/methods/getFileIcon.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/methods/getFileIcon.ts
📜 Review details
🔇 Additional comments (1)
app/containers/message/Components/Attachments/Reply.tsx (1)
175-180: Nice memoization update.Including
formatandsizehere prevents the memoized description from getting stuck with stale document metadata when attachment details hydrate after the first render.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
this pr adds the pdf icon beside the attachment title if the file format is PDF to improve readability
Issue(s)
How to test or reproduce
Screenshots
Light
Dark Dark
Dark Black
Types of changes
Checklist
Further comments
if want to add icon support for more document format but not sure how to edit custom.ttf
Summary by CodeRabbit