Refine attachment preview and metadata actions#2334
Merged
Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds document visibility checks across multiple components and hooks to prevent read-marking operations (wave/drop reads and notification invalidations) from executing when the browser tab is hidden. Additionally, the attachment display component now fetches and renders metadata from IPFS gateway URLs with abort handling, and reorganizes UI controls into a dropdown menu. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 12 minutes and 37 seconds.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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.ts (1)
25-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHidden-tab skip currently has no retry path on visibility restore
At Line 26, hidden tabs return early, but
syncReadState()is only called once. If the hook mounts while hidden, it may never mark read after becoming visible unless deps change.Suggested fix
useEffect(() => { if (!enabled) { return; } const syncReadState = async () => { if (document.visibilityState !== "visible") { return; } @@ .catch((error) => console.error("Failed to mark feed as read:", error)); }; - syncReadState(); + void syncReadState(); + const onVisibilityChange = () => { + if (document.visibilityState === "visible") { + void syncReadState(); + } + }; + document.addEventListener("visibilitychange", onVisibilityChange); + return () => { + document.removeEventListener("visibilitychange", onVisibilityChange); + }; }, [ enabled, waveId, removeWaveDeliveredNotifications, invalidateNotifications, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.ts` around lines 25 - 46, The effect’s syncReadState in useWaveDropsNotificationRead returns early when document.visibilityState !== "visible", so if the hook mounts while hidden it never retries when the tab becomes visible; modify the effect to register a "visibilitychange" listener that calls syncReadState when document.visibilityState === "visible" (and still call syncReadState immediately on mount), ensure the listener is cleaned up in the effect return, and keep the existing logic that calls removeWaveDeliveredNotifications(waveId) and then commonApiPostWithoutBodyAndResponse(...) followed by invalidateNotifications() with error handling intact.
🧹 Nitpick comments (3)
components/drops/view/item/content/attachments/DropAttachmentDisplay.tsx (3)
643-644: ⚖️ Poor tradeoffComplex regex for JSON tokenization.
The regex complexity (35) exceeds the SonarCloud threshold (20). While this pattern correctly handles JSON syntax highlighting needs, consider extracting it into a separate utility or using a dedicated tokenizer library if maintenance becomes difficult.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/drops/view/item/content/attachments/DropAttachmentDisplay.tsx` around lines 643 - 644, The JSON_TOKEN_PATTERN regex in DropAttachmentDisplay.tsx is flagged for complexity; extract this constant into a dedicated utility module (e.g., jsonTokenizeUtils or jsonHighlighter) and import it into DropAttachmentDisplay to reduce complexity in the component, or replace usage with a stable JSON tokenizer/highlighter library; update references to JSON_TOKEN_PATTERN in DropAttachmentDisplay (and any helper functions that use it) to import from the new module and add a small unit test in the util to cover tokenization behavior.
308-327: ⚖️ Poor tradeoffConsider adding a size limit for metadata responses.
Unlike
fetchCsvPreviewText, this function doesn't enforce any size limit. A malicious or unexpectedly largemetadata.jsoncould consume excessive memory. Consider adding a similar size check or using streaming with a cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/drops/view/item/content/attachments/DropAttachmentDisplay.tsx` around lines 308 - 327, fetchMetadataPreviewText currently reads the entire response into memory with no size checks; add a cap similar to fetchCsvPreviewText to prevent huge or malicious payloads by enforcing a MAX_METADATA_BYTES (e.g. 1_000_000) and refusing responses larger than that: first check response.headers.get("content-length") and throw if it exceeds the limit, then if unknown read response.body as a stream and accumulate bytes up to the same limit, aborting/throwing when exceeded; only after confirming the payload is within the limit convert to text and attempt JSON.parse so fetchMetadataPreviewText enforces a hard size cap while preserving JSON detection.
69-69: 💤 Low valuePrefer
indexOfoverfindIndexfor simple string search.When searching for a specific value rather than matching a predicate,
indexOfis more idiomatic and slightly more efficient.Suggested change
- const ipfsIndex = pathSegments.findIndex((segment) => segment === "ipfs"); + const ipfsIndex = pathSegments.indexOf("ipfs");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/drops/view/item/content/attachments/DropAttachmentDisplay.tsx` at line 69, The code uses pathSegments.findIndex((segment) => segment === "ipfs") to locate the "ipfs" segment; replace this with the simpler, more idiomatic pathSegments.indexOf("ipfs") in DropAttachmentDisplay (variable ipfsIndex) so the intent is clearer and slightly more efficient—update any subsequent logic that reads ipfsIndex (e.g., checks for -1) to remain correct after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx`:
- Around line 254-272: The test "does not call the read endpoint on unmount when
the tab is hidden" is missing an assertion that delivered-notification cleanup
is skipped; update that test (the one rendering MyStreamWaveChat and calling
unmount()) to also assert mockRemoveWaveDeliveredNotifications was not called by
adding expect(mockRemoveWaveDeliveredNotifications).not.toHaveBeenCalled()
alongside the existing assertions for commonApiPostWithoutBodyAndResponse and
invalidateNotificationsMock.
In
`@__tests__/components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.test.tsx`:
- Around line 93-109: The test for the hidden-tab case is missing an assertion
that delivered notifications aren't removed; update the "does not call the read
endpoint when the tab is hidden" test to also assert
removeWaveDeliveredNotifications was not called by adding
expect(removeWaveDeliveredNotifications).not.toHaveBeenCalled(), locating this
near the existing expects for commonApiPostWithoutBodyAndResponse and
invalidateNotifications in the test that renders TestComponent (which exercises
useWaveDropsNotificationRead).
In `@__tests__/useWaveRealtimeUpdater.test.ts`:
- Around line 239-258: The test for hidden-wave behavior should also assert that
the delivered-notification side effect doesn't run: after invoking
useWaveRealtimeUpdater's processIncomingDrop (see processIncomingDrop and
ProcessIncomingDropType) and flushing promises, add an assertion that
props.removeWaveDeliveredNotifications was not called (in addition to the
existing expect on commonApiPostWithoutBodyAndResponse) to fully verify
hidden-tab behavior.
In `@components/drops/view/item/content/attachments/DropAttachmentDisplay.tsx`:
- Around line 1151-1157: In handleMenuCopyLink, remove the nested
globalThis.window.setTimeout(() => setCopiedLink(false), 0) so the copiedLink
state isn’t immediately cleared; instead call await handleCopyLink(), then close
the menu with setIsMoreMenuOpen(false) and rely on the existing auto-reset
effect for copiedLink (lines around handleMenuCopyLink, handleCopyLink,
setIsMoreMenuOpen, setCopiedLink) to clear the “Copied” feedback after its
1500ms timeout.
In `@contexts/wave/hooks/useWaveRealtimeUpdater.ts`:
- Around line 190-193: The code only guards markWaveAsRead with
document.visibilityState but still removes "delivered-notification" entries
while the tab is hidden, which can clear local unread state without a
server-side read; inside useWaveRealtimeUpdater, add the same visibility check
before performing the delivered-notification removal (or move the visibility
guard to encompass both actions) so that delivered notifications are not removed
when document.visibilityState !== "visible" — update the logic around the
delivered-notification removal code to consult document.visibilityState (reuse
the markWaveAsRead guard or wrap the removal block) to ensure local unread state
isn't cleared while hidden.
---
Outside diff comments:
In `@components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.ts`:
- Around line 25-46: The effect’s syncReadState in useWaveDropsNotificationRead
returns early when document.visibilityState !== "visible", so if the hook mounts
while hidden it never retries when the tab becomes visible; modify the effect to
register a "visibilitychange" listener that calls syncReadState when
document.visibilityState === "visible" (and still call syncReadState immediately
on mount), ensure the listener is cleaned up in the effect return, and keep the
existing logic that calls removeWaveDeliveredNotifications(waveId) and then
commonApiPostWithoutBodyAndResponse(...) followed by invalidateNotifications()
with error handling intact.
---
Nitpick comments:
In `@components/drops/view/item/content/attachments/DropAttachmentDisplay.tsx`:
- Around line 643-644: The JSON_TOKEN_PATTERN regex in DropAttachmentDisplay.tsx
is flagged for complexity; extract this constant into a dedicated utility module
(e.g., jsonTokenizeUtils or jsonHighlighter) and import it into
DropAttachmentDisplay to reduce complexity in the component, or replace usage
with a stable JSON tokenizer/highlighter library; update references to
JSON_TOKEN_PATTERN in DropAttachmentDisplay (and any helper functions that use
it) to import from the new module and add a small unit test in the util to cover
tokenization behavior.
- Around line 308-327: fetchMetadataPreviewText currently reads the entire
response into memory with no size checks; add a cap similar to
fetchCsvPreviewText to prevent huge or malicious payloads by enforcing a
MAX_METADATA_BYTES (e.g. 1_000_000) and refusing responses larger than that:
first check response.headers.get("content-length") and throw if it exceeds the
limit, then if unknown read response.body as a stream and accumulate bytes up to
the same limit, aborting/throwing when exceeded; only after confirming the
payload is within the limit convert to text and attempt JSON.parse so
fetchMetadataPreviewText enforces a hard size cap while preserving JSON
detection.
- Line 69: The code uses pathSegments.findIndex((segment) => segment === "ipfs")
to locate the "ipfs" segment; replace this with the simpler, more idiomatic
pathSegments.indexOf("ipfs") in DropAttachmentDisplay (variable ipfsIndex) so
the intent is clearer and slightly more efficient—update any subsequent logic
that reads ipfsIndex (e.g., checks for -1) to remain correct after the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 085b4fce-c91f-40ba-9994-0e6060859ffe
📒 Files selected for processing (8)
__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx__tests__/components/drops/view/item/content/attachments/DropAttachmentDisplay.test.tsx__tests__/components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.test.tsx__tests__/useWaveRealtimeUpdater.test.tscomponents/brain/my-stream/MyStreamWaveChat.tsxcomponents/drops/view/item/content/attachments/DropAttachmentDisplay.tsxcomponents/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.tscontexts/wave/hooks/useWaveRealtimeUpdater.ts
|
ragnep
approved these changes
Apr 30, 2026
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
2 participants
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.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features