Image upload/resize#3114
Conversation
📝 WalkthroughWalkthroughThese changes enhance image upload handling with size validation and compression, add concurrency control to voting actions using a ref guard, and improve draft submission UX with immediate state feedback and spinner management across submission flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/upvotePopover/container/upvotePopover.tsx (2)
212-227:⚠️ Potential issue | 🟡 MinorPre-existing: invalid response early-return doesn't reset parent voting state.
When
vote()succeeds but returns an invalid/empty response (lines 212–227), the code shows a toast and returns without calling_onVotingStart(0). The.finally()will correctly unlockisVotingRef, but the parent component's voting UI (controlled via_onVotingStart) may be left in a stale "voting in progress" state. This predates this PR, but since you're already touching this function, it's worth addressing.Proposed fix
if (!response || !response.id) { dispatch( toastNotification( intl.formatMessage( { id: 'alert.something_wrong_msg' }, { message: intl.formatMessage({ id: 'alert.invalid_response', }), }, ), ), ); + _onVotingStart ? _onVotingStart(0) : null; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/upvotePopover/container/upvotePopover.tsx` around lines 212 - 227, In the vote() function in upvotePopover.tsx, when the response is missing or response.id is falsy, call _onVotingStart(0) to reset the parent voting UI before showing the toast and returning; ensure this reset happens immediately in that invalid-response branch so the parent isn't left in a "voting in progress" state (the existing .finally() still clears isVotingRef, but it doesn't reset the parent's UI).
183-289:⚠️ Potential issue | 🟡 MinorGuard could remain permanently locked if synchronous code throws before
vote()is called.
isVotingRef.current = trueis set at line 189, but the.finally()that resets it is chained on thevote()promise at line 203. If any of the synchronous statements between lines 190–201 throw (e.g.,_updateVoteCache,_setUpvotePercent), the promise chain never starts and the guard is stuck attrue, permanently blocking all future votes until the component remounts.Wrapping the entire body in a
try/finallywould be more robust:Proposed fix
const _upvoteContent = async () => { if (isVotingRef.current) { return; } if (!isDownVoted) { isVotingRef.current = true; - const _onVotingStart = onVotingStartRef.current; - _closePopover(); - _onVotingStart ? _onVotingStart(sliderValue) : null; - - _setUpvotePercent(sliderValue); - - const weight = sliderValue ? Math.trunc(sliderValue * 100) * 100 : 0; - const _author = content?.author; - const _permlink = content?.permlink; - - console.log(`casting up vote: ${weight}`); - _updateVoteCache(_author, _permlink, amount, false, CacheStatus.PENDING); - - vote(currentAccount, pinCode, _author, _permlink, weight) - .then((response) => { + try { + const _onVotingStart = onVotingStartRef.current; + _closePopover(); + _onVotingStart ? _onVotingStart(sliderValue) : null; + + _setUpvotePercent(sliderValue); + + const weight = sliderValue ? Math.trunc(sliderValue * 100) * 100 : 0; + const _author = content?.author; + const _permlink = content?.permlink; + + console.log(`casting up vote: ${weight}`); + _updateVoteCache(_author, _permlink, amount, false, CacheStatus.PENDING); + + const response = await vote(currentAccount, pinCode, _author, _permlink, weight); // ... success handling ... - }) - .catch((err) => { + } catch (err) { // ... error handling ... - }) - .finally(() => { - isVotingRef.current = false; - }); + } finally { + isVotingRef.current = false; + } } else { setIsDownVoted(false); } };The same pattern applies to
_downvoteContent. Since the function is alreadyasync, switching toawait+try/catch/finallyalso simplifies readability.In practice the risk is low (the synchronous calls are unlikely to throw), but since this is a guard that permanently locks out voting, the defensive pattern is warranted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/upvotePopover/container/upvotePopover.tsx` around lines 183 - 289, The guard is set by isVotingRef.current = true inside _upvoteContent but can remain true if any synchronous code before vote() throws; wrap the voting flow in a try/finally so the ref is always reset: move isVotingRef.current = true to the top of _upvoteContent, then use try { perform _closePopover, _onVotingStart, _setUpvotePercent, _updateVoteCache, await vote(...) and handle response } catch (err) { handle errors as current .catch logic } finally { isVotingRef.current = false }, and apply the same async/await + try/catch/finally pattern to _downvoteContent to ensure the guard is always cleared even on synchronous exceptions.src/components/uploadsGalleryModal/container/uploadsGalleryModal.tsx (1)
271-280:⚠️ Potential issue | 🟠 Major
element.sourceURLcan be null on iOS — use fallback toelement.pathfor HEIC conversion.The codebase already uses
sourceURL || pathfallback in multiple locations (speakUploaderModal.tsx,speak.ts), indicating thatsourceURLcan be null or undefined. The HEIC conversion code should follow the same pattern.If
sourceURLis null,RNHeicConverter.convert({ path: null })will fail. While the guard at line 274 (if (res && res.path)) prevents a crash, it silently leaves the element as HEIC mime-type and uploads it unconverted — the server is unlikely to accept raw HEIC.Update the conversion to use a fallback:
🛡️ Proposed fix
- const res = await RNHeicConverter.convert({ path: element.sourceURL }); + const heicPath = element.sourceURL || element.path; + const res = await RNHeicConverter.convert({ path: heicPath });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/uploadsGalleryModal/container/uploadsGalleryModal.tsx` around lines 271 - 280, The HEIC conversion currently calls RNHeicConverter.convert with element.sourceURL which can be null on iOS; change the call in the uploadsGalleryModal HEIC branch to pass a fallback path (use element.sourceURL || element.path) so convert never receives null, then proceed to update element.mime, element.path and element.filename and write back into media[i] as before; locate the code block handling image/heic conversion (variables: element, media, i, RNHeicConverter.convert) and replace the convert argument to use the fallback path.
🧹 Nitpick comments (4)
src/components/upvotePopover/container/upvotePopover.tsx (1)
291-358:_onVotingStartreference is captured outside the guard — minor nit.Line 296 captures
onVotingStartRef.currentbefore theif (isDownVoted)check at line 297. This is harmless but slightly inconsistent with_upvoteContentwhere the capture happens inside the guarded block (line 190). Consider moving line 296 inside theif (isDownVoted)block for symmetry and to avoid the unnecessary read when the else branch is taken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/upvotePopover/container/upvotePopover.tsx` around lines 291 - 358, Move the capture of onVotingStartRef.current into the guarded branch to mirror _upvoteContent: inside the _downvoteContent function, relocate the line const _onVotingStart = onVotingStartRef.current so it is declared within the if (isDownVoted) block (near the start of that block) rather than before the if; this avoids an unnecessary ref read when the else branch executes and keeps behavior symmetric between _downvoteContent, _upvoteContent, isDownVoted and onVotingStartRef.src/screens/editor/container/editorContainer.tsx (2)
1073-1075:setState({ isPostSending: true })at line 1075 makes the duplicate call at lines 1115-1117 dead code.Now that
isPostSending: trueis set unconditionally early (line 1075) and only reset tofalseon the authority-prompt path (line 1089), the existingsetState({ isPostSending: true })at lines 1115-1117 is unreachable in a state whereisPostSendingisfalse. Consider removing it to keep the intent clear.♻️ Proposed cleanup
} - this.setState({ - isPostSending: true, - }); - const { post } = this.state;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/editor/container/editorContainer.tsx` around lines 1073 - 1075, The early unconditional setting of the submission flags (this._isSubmitting = true and this.setState({ isPostSending: true }) in the submit flow) makes the later duplicate setState call that sets isPostSending to true (the setState at lines 1115-1117) dead code; remove that redundant setState to avoid confusion and clarify intent. Update the submit handler (the method that toggles this._isSubmitting and this.setState({ isPostSending })) by deleting the second setState({ isPostSending: true }) call and keep only the initial flag set, ensuring any path that resets isPostSending to false (the authority-prompt reset) remains unchanged.
673-676: Add anArray.isArrayguard before indexingresponse?.[0].Without a guard, if
addDraftever returns a non-array value with a truthy[0]property (or, pathologically, a bare string),_resDraftwill be set to that value, bypass theif (!_resDraft)check, and then silently produceundefinedfor_resDraft._id— corruptingdraftIdstate with no thrown error to surface the bug.♻️ Proposed fix
- const _resDraft = - response?.drafts?.[0] || // array wrapper format - response?.[0] || // direct array format - (response?._id ? response : null); // single object format + const _resDraft = + response?.drafts?.[0] || // array wrapper format + (Array.isArray(response) ? response[0] : null) || // direct array format + (response?._id ? response : null); // single object format🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/editor/container/editorContainer.tsx` around lines 673 - 676, The current fallback logic that sets _resDraft can index response via response?.[0] without ensuring response is an array; update the expression used to compute _resDraft (the variable assigned from response?.drafts?.[0] || response?.[0] || (response?._id ? response : null)) to only use the second branch when response is an array (e.g., replace response?.[0] with a guarded check such as Array.isArray(response) && response[0]); ensure the overall order still prefers response.drafts[0], then response[0] (only if response is an array), then the single-object fallback (response._id) so _resDraft and subsequent draftId logic cannot become undefined silently.src/components/uploadsGalleryModal/container/uploadsGalleryModal.tsx (1)
256-280: Size validation doesn't cover the HEIC → JPEG expansion path.The
item.sizecheck at line 256 uses the picker-reported size before HEIC conversion. WhenRNHeicConverter.convertconverts from the originalsourceURLasset, the resulting JPEG can be larger than the picker-compressed value that passed the pre-check, meaning a HEIC file that passes the 30 MB guard could produce an oversized JPEG. The existing 413 server-error handler (lines 352–357) will ultimately catch this, but a post-conversion size re-check would give a cleaner client-side signal without an unnecessary upload attempt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/uploadsGalleryModal/container/uploadsGalleryModal.tsx` around lines 256 - 280, After converting HEIC to JPEG in the loop that uses RNHeicConverter.convert, re-check the resulting file size against MAX_IMAGE_UPLOAD_SIZE (update element.size from the conversion result or stat the new path), and if it exceeds the limit remove it from media, show the same Alert.alert(intl.formatMessage({ id: 'alert.fail' }), intl.formatMessage({ id: 'alert.payloadTooLarge' })), and abort early if media becomes empty; ensure this logic is applied inside the loop that processes element.mime === 'image/heic' so oversized JPEGs are caught client-side before upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/uploadsGalleryModal/container/uploadsGalleryModal.tsx`:
- Line 255: The inline comment near the image size guard is stale (mentions 8MB)
while the code uses MAX_IMAGE_UPLOAD_SIZE = 30000000; update the comment in
uploadsGalleryModal (around the image-filtering logic) to accurately reflect the
30 MB limit or reference the MAX_IMAGE_UPLOAD_SIZE constant (e.g., "filter out
images larger than MAX_IMAGE_UPLOAD_SIZE (30 MB)") so the comment matches the
actual guard.
---
Outside diff comments:
In `@src/components/uploadsGalleryModal/container/uploadsGalleryModal.tsx`:
- Around line 271-280: The HEIC conversion currently calls
RNHeicConverter.convert with element.sourceURL which can be null on iOS; change
the call in the uploadsGalleryModal HEIC branch to pass a fallback path (use
element.sourceURL || element.path) so convert never receives null, then proceed
to update element.mime, element.path and element.filename and write back into
media[i] as before; locate the code block handling image/heic conversion
(variables: element, media, i, RNHeicConverter.convert) and replace the convert
argument to use the fallback path.
In `@src/components/upvotePopover/container/upvotePopover.tsx`:
- Around line 212-227: In the vote() function in upvotePopover.tsx, when the
response is missing or response.id is falsy, call _onVotingStart(0) to reset the
parent voting UI before showing the toast and returning; ensure this reset
happens immediately in that invalid-response branch so the parent isn't left in
a "voting in progress" state (the existing .finally() still clears isVotingRef,
but it doesn't reset the parent's UI).
- Around line 183-289: The guard is set by isVotingRef.current = true inside
_upvoteContent but can remain true if any synchronous code before vote() throws;
wrap the voting flow in a try/finally so the ref is always reset: move
isVotingRef.current = true to the top of _upvoteContent, then use try { perform
_closePopover, _onVotingStart, _setUpvotePercent, _updateVoteCache, await
vote(...) and handle response } catch (err) { handle errors as current .catch
logic } finally { isVotingRef.current = false }, and apply the same async/await
+ try/catch/finally pattern to _downvoteContent to ensure the guard is always
cleared even on synchronous exceptions.
---
Nitpick comments:
In `@src/components/uploadsGalleryModal/container/uploadsGalleryModal.tsx`:
- Around line 256-280: After converting HEIC to JPEG in the loop that uses
RNHeicConverter.convert, re-check the resulting file size against
MAX_IMAGE_UPLOAD_SIZE (update element.size from the conversion result or stat
the new path), and if it exceeds the limit remove it from media, show the same
Alert.alert(intl.formatMessage({ id: 'alert.fail' }), intl.formatMessage({ id:
'alert.payloadTooLarge' })), and abort early if media becomes empty; ensure this
logic is applied inside the loop that processes element.mime === 'image/heic' so
oversized JPEGs are caught client-side before upload.
In `@src/components/upvotePopover/container/upvotePopover.tsx`:
- Around line 291-358: Move the capture of onVotingStartRef.current into the
guarded branch to mirror _upvoteContent: inside the _downvoteContent function,
relocate the line const _onVotingStart = onVotingStartRef.current so it is
declared within the if (isDownVoted) block (near the start of that block) rather
than before the if; this avoids an unnecessary ref read when the else branch
executes and keeps behavior symmetric between _downvoteContent, _upvoteContent,
isDownVoted and onVotingStartRef.
In `@src/screens/editor/container/editorContainer.tsx`:
- Around line 1073-1075: The early unconditional setting of the submission flags
(this._isSubmitting = true and this.setState({ isPostSending: true }) in the
submit flow) makes the later duplicate setState call that sets isPostSending to
true (the setState at lines 1115-1117) dead code; remove that redundant setState
to avoid confusion and clarify intent. Update the submit handler (the method
that toggles this._isSubmitting and this.setState({ isPostSending })) by
deleting the second setState({ isPostSending: true }) call and keep only the
initial flag set, ensuring any path that resets isPostSending to false (the
authority-prompt reset) remains unchanged.
- Around line 673-676: The current fallback logic that sets _resDraft can index
response via response?.[0] without ensuring response is an array; update the
expression used to compute _resDraft (the variable assigned from
response?.drafts?.[0] || response?.[0] || (response?._id ? response : null)) to
only use the second branch when response is an array (e.g., replace
response?.[0] with a guarded check such as Array.isArray(response) &&
response[0]); ensure the overall order still prefers response.drafts[0], then
response[0] (only if response is an array), then the single-object fallback
(response._id) so _resDraft and subsequent draftId logic cannot become undefined
silently.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes