feat(Content-sharing): Add event callbacks to Content Sharing#4477
feat(Content-sharing): Add event callbacks to Content Sharing#4477mergify[bot] merged 5 commits intomasterfrom
Conversation
|
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 optional callbacks Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ContentSharingV2
participant API as ItemAPI
participant Modal as UnifiedShareModal
participant Notif as NotificationService
rect rgba(200,200,255,0.5)
UI->>API: fetch item data
API-->>UI: success (item)
UI->>UI: map owner -> {email, id, name}
UI->>UI: call onLoad()
end
rect rgba(255,200,200,0.5)
UI->>API: fetch item data
API-->>UI: error
UI->>UI: call onError(error)
UI->>Notif: show error (styledText in payload)
end
rect rgba(200,255,200,0.5)
UI->>Modal: set onOpenChange = handleOpenChange
Modal->>UI: onOpenChange(closed)
UI->>UI: call onClose()
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
257-290: Consider adding test coverage foronClosecallback.The new
onCloseprop is added toContentSharingV2but has no corresponding test coverage in this suite. Consider adding a test that simulates modal close and verifiesonCloseis invoked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx` around lines 257 - 290, Add a test for the new onClose prop on ContentSharingV2 by rendering the component with a jest.fn() passed as onClose via renderComponent({ onClose }), then simulate the user closing the modal (e.g., click the modal close button or backdrop using the same query used elsewhere in the suite) and await a waitFor(() => expect(onClose).toHaveBeenCalled()) to assert it was invoked; ensure the test isolates onClose (use fresh jest.fn()) and does not rely on other callbacks.src/elements/content-sharing/ContentSharingV2.tsx (2)
193-193: RemoveonErrorfrom dependency array—it's unused in this effect.The
useEffectfor fetching the current user (lines 170-193) does not invokeonErroranywhere in its body, yetonErroris included in the dependency array. This causes unnecessary effect re-runs when theonErrorprop reference changes.Proposed fix
- }, [api, currentUser, item, itemId, itemType, sharedLink, getError, onError]); + }, [api, currentUser, item, itemId, itemType, sharedLink, getError]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` at line 193, The useEffect in ContentSharingV2.tsx that fetches the current user includes onError in its dependency array but never uses it; remove onError from the dependency array of that useEffect so the dependencies are [api, currentUser, item, itemId, itemType, sharedLink, getError] (leave the effect body and getError usage unchanged) to prevent unnecessary re-renders when the onError prop reference changes.
161-167: LGTM on callback invocations, but verify necessity ofonErrorin dependency array.The
onLoad?.()andonError?.(error)calls are correctly placed. However, the dependency array at line 167 includes bothonErrorandonLoad, which is correct since they are used in the effect body.Note: Line 193 also includes
onErrorin its dependency array, butonErroris not called within that effect. Consider removingonErrorfrom line 193's dependencies to avoid unnecessary effect re-runs when the callback reference changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` around lines 161 - 167, The effect at ContentSharingV2.tsx that includes onError in its dependency array but does not call onError should have onError removed from that effect's dependency list; locate the useEffect (the second effect around the later block referenced in the review) and remove onError from its dependencies (verify that onError is not referenced in that effect's body such as in any callbacks or nested functions), then run the linter/tests to ensure no missing dependency warnings and confirm behavior is unchanged.
🤖 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/elements/content-sharing/__tests__/ContentSharingV2.test.tsx`:
- Around line 281-289: The test "should call onError and not onLoad when item
data fails to load" calls renderComponent with { api: errorApi, onError } but
never provides onLoad, so the negative assertion on onLoad is invalid; update
the test to pass both callbacks (provide the mocked onLoad alongside onError)
when calling renderComponent (the symbol to change is the renderComponent
invocation in this test) so the component's error path is verified to call
onError(error) and not invoke onLoad.
- Around line 271-279: The test "should call onLoad and not onError when item
data loads successfully" currently asserts onError wasn't called but never
passes onError into renderComponent; update the test to pass both mocks into
renderComponent (e.g., call renderComponent({ onLoad, onError })) so the
component receives the onError prop, then keep the existing assertions; locate
the test function and the renderComponent helper and ensure both onLoad and
onError mocks are provided when rendering.
---
Nitpick comments:
In `@src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx`:
- Around line 257-290: Add a test for the new onClose prop on ContentSharingV2
by rendering the component with a jest.fn() passed as onClose via
renderComponent({ onClose }), then simulate the user closing the modal (e.g.,
click the modal close button or backdrop using the same query used elsewhere in
the suite) and await a waitFor(() => expect(onClose).toHaveBeenCalled()) to
assert it was invoked; ensure the test isolates onClose (use fresh jest.fn())
and does not rely on other callbacks.
In `@src/elements/content-sharing/ContentSharingV2.tsx`:
- Line 193: The useEffect in ContentSharingV2.tsx that fetches the current user
includes onError in its dependency array but never uses it; remove onError from
the dependency array of that useEffect so the dependencies are [api,
currentUser, item, itemId, itemType, sharedLink, getError] (leave the effect
body and getError usage unchanged) to prevent unnecessary re-renders when the
onError prop reference changes.
- Around line 161-167: The effect at ContentSharingV2.tsx that includes onError
in its dependency array but does not call onError should have onError removed
from that effect's dependency list; locate the useEffect (the second effect
around the later block referenced in the review) and remove onError from its
dependencies (verify that onError is not referenced in that effect's body such
as in any callbacks or nested functions), then run the linter/tests to ensure no
missing dependency warnings and confirm behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9537cb82-73aa-4836-ab3d-16a8a1e7778b
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
Outdated
Show resolved
Hide resolved
0e2454b to
4ce8b39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
193-193: UnnecessaryonErrorin dependency array.The
onErrorcallback is not used within thisuseEffect(fetching current user). Including it in the dependency array is harmless but misleading—consider removing it for clarity.Proposed fix
- }, [api, currentUser, item, itemId, itemType, sharedLink, getError, onError]); + }, [api, currentUser, item, itemId, itemType, sharedLink, getError]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` at line 193, The dependency array for the useEffect that fetches the current user incorrectly includes onError even though onError is not referenced inside that effect; update the useEffect dependency array (the one currently listing api, currentUser, item, itemId, itemType, sharedLink, getError, onError) to remove onError so it only contains the actual dependencies (api, currentUser, item, itemId, itemType, sharedLink, getError), keeping the effect behavior unchanged and avoiding misleading dependencies.
🤖 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/elements/content-sharing/__tests__/ContentSharingV2.test.tsx`:
- Around line 292-299: ContentSharingV2 is passing the wrong prop name to
UnifiedShareModal: change the prop passed from onOpenChange to onRequestClose so
the modal receives the close handler (wire ContentSharingV2's onClose into
UnifiedShareModal as onRequestClose), ensuring the close button triggers the
onClose callback in tests; update any prop forwarding in ContentSharingV2 where
UnifiedShareModal is instantiated to use onRequestClose instead of onOpenChange.
---
Nitpick comments:
In `@src/elements/content-sharing/ContentSharingV2.tsx`:
- Line 193: The dependency array for the useEffect that fetches the current user
incorrectly includes onError even though onError is not referenced inside that
effect; update the useEffect dependency array (the one currently listing api,
currentUser, item, itemId, itemType, sharedLink, getError, onError) to remove
onError so it only contains the actual dependencies (api, currentUser, item,
itemId, itemType, sharedLink, getError), keeping the effect behavior unchanged
and avoiding misleading dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6231f835-da25-4ad6-a6f7-af9d4a53df96
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
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 (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)
109-139:⚠️ Potential issue | 🟠 MajorStabilize these effect dependencies before adding callback props.
getErrorchanges identity whenhasErrorflips, andonLoad/onErrorwill also change whenever a parent passes fresh inline callbacks. WhileitemorcurrentUseris stillnull, that re-runs these effects and can issue duplicatefetchItem()/fetchCurrentUser()requests. It can also fireonErrormore than once.onErroron Line 193 is especially problematic because it is not even used in that effect body.🔧 One way to keep the fetch effects stable
+ const onLoadRef = React.useRef(onLoad); + const onErrorRef = React.useRef(onError); + const hasErrorRef = React.useRef(false); + + React.useEffect(() => { + onLoadRef.current = onLoad; + onErrorRef.current = onError; + }, [onLoad, onError]); + const getError = React.useCallback( (error: ElementsXhrError) => { - if (hasError) { + if (hasErrorRef.current) { return; } + hasErrorRef.current = true;- [addNotification, formatMessage, hasError], + [addNotification, formatMessage], );React.useEffect(() => { + hasErrorRef.current = false; setHasError(false); setItem(null); setSharedLink(null); setCurrentUser(null); setCollaborationRoles(null); setAvatarUrlMap(null); setCollaborators(null); setCollaboratorsData(null); }, [api]);- onLoad?.(); + onLoadRef.current?.(); } catch (error) { - onError?.(error); + onErrorRef.current?.(error); getError(error); } })(); - }, [api, item, itemId, itemType, sharedLink, getError, handleGetItemSuccess, onError, onLoad]); + }, [api, item, itemId, itemType, sharedLink, getError, handleGetItemSuccess]);- }, [api, currentUser, item, itemId, itemType, sharedLink, getError, onError]); + }, [api, currentUser, item, itemId, itemType, sharedLink, getError]);Also applies to: 154-167, 170-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` around lines 109 - 139, getError currently changes identity when hasError flips which invalidates effects (like the fetchItem/fetchCurrentUser effects and onError/onLoad props) and can trigger duplicate requests or duplicate notifications; fix by making getError stable: remove hasError from its dependency list and replace the local boolean dependency with a ref (e.g., hasErrorRef) or read hasError via a functional state update so getError no longer re-creates when hasError changes, ensure addNotification and formatMessage are stable (memoize or capture refs if they come from props) and, if parent inline onError/onLoad cause instability, capture them via refs or require callers to memoize so the effects that call fetchItem/fetchCurrentUser reference stable callbacks instead of recreating on every render.
243-258:⚠️ Potential issue | 🔴 CriticalUse
onRequestCloseinstead ofonOpenChangefor the UnifiedShareModal callback.The in-repo
UnifiedShareModalcomponent expects the close handler via theonRequestCloseprop (seesrc/features/unified-share-modal/flowTypes.js:396andUnifiedShareModal.js:193, 212). PassingonOpenChangewill have no effect andonClosewill never fire.Change:
onOpenChange={handleOpenChange}to:
onRequestClose={handleOpenChange}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` around lines 243 - 258, The modal close handler is being passed via the wrong prop so UnifiedShareModal never calls it; update the UnifiedShareModal usage to pass the existing handleOpenChange function through the onRequestClose prop instead of onOpenChange (change the prop on the UnifiedShareModal component from onOpenChange to onRequestClose so handleOpenChange will be invoked when the modal closes).
🤖 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/elements/content-sharing/ContentSharingV2.tsx`:
- Around line 159-164: The catch block in ContentSharingV2 (around
fetchItem/handleGetItemSuccess) currently calls the consumer onError directly
which can throw and prevent the internal fallback getError from running; wrap
the consumer callback invocation in its own try/catch (or call it inside a
Promise.resolve().catch) so any exception it throws is contained, then ensure
getError(error) is always called afterwards (or use a finally block) so the
built-in error path in ContentSharingV2 runs regardless of onError failures;
reference the onError invocation and getError call in the catch for this change.
---
Outside diff comments:
In `@src/elements/content-sharing/ContentSharingV2.tsx`:
- Around line 109-139: getError currently changes identity when hasError flips
which invalidates effects (like the fetchItem/fetchCurrentUser effects and
onError/onLoad props) and can trigger duplicate requests or duplicate
notifications; fix by making getError stable: remove hasError from its
dependency list and replace the local boolean dependency with a ref (e.g.,
hasErrorRef) or read hasError via a functional state update so getError no
longer re-creates when hasError changes, ensure addNotification and
formatMessage are stable (memoize or capture refs if they come from props) and,
if parent inline onError/onLoad cause instability, capture them via refs or
require callers to memoize so the effects that call fetchItem/fetchCurrentUser
reference stable callbacks instead of recreating on every render.
- Around line 243-258: The modal close handler is being passed via the wrong
prop so UnifiedShareModal never calls it; update the UnifiedShareModal usage to
pass the existing handleOpenChange function through the onRequestClose prop
instead of onOpenChange (change the prop on the UnifiedShareModal component from
onOpenChange to onRequestClose so handleOpenChange will be invoked when the
modal closes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4880170-ce20-407e-bfdf-e2771a530778
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/tests/ContentSharingV2.test.tsx
4ce8b39 to
82b8a90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)
243-247: Consider memoizinghandleOpenChangewithuseCallback.This handler is recreated on every render, which could trigger unnecessary re-renders of
UnifiedShareModal. Wrapping it inuseCallbackwithonCloseas a dependency follows the pattern used for other handlers in this file.♻️ Suggested refactor
- const handleOpenChange = (open: boolean) => { - if (!open) { - onClose?.(); - } - }; + const handleOpenChange = React.useCallback((open: boolean) => { + if (!open) { + onClose?.(); + } + }, [onClose]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` around lines 243 - 247, handleOpenChange is recreated each render which can cause unnecessary re-renders of UnifiedShareModal; wrap it in React.useCallback to memoize it and include onClose in the dependency array so the callback updates when onClose changes. Locate the handleOpenChange function in ContentSharingV2 and replace the inline arrow handler with a useCallback-wrapped version (keeping the same behavior: call onClose?.() when open is false) to match the other memoized handlers in this file.
193-193: Unnecessary dependency:onErroris not used in this effect.The
onErrorcallback is included in the dependency array but never invoked within this effect. While thecurrentUserguard prevents actual side effects from re-runs, removing unused dependencies improves clarity.♻️ Suggested fix
- }, [api, currentUser, item, itemId, itemType, sharedLink, getError, onError]); + }, [api, currentUser, item, itemId, itemType, sharedLink, getError]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` at line 193, The effect with dependency array [api, currentUser, item, itemId, itemType, sharedLink, getError, onError] includes onError but never uses it; remove onError from the dependency list in the useEffect declaration in ContentSharingV2 (the effect guarded by currentUser) so the hook only depends on the actually used symbols (api, currentUser, item, itemId, itemType, sharedLink, getError); if onError is meant to be used, instead call it inside the effect where errors are handled and keep it in the deps—otherwise delete it from the deps to satisfy correctness and clarity.
🤖 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/elements/content-sharing/ContentSharingV2.tsx`:
- Around line 43-44: The onError prop in ContentSharingV2.tsx is typed as
(error: Error) but the API errorHandler passes either response.data (plain
object) or an AxiosError; update the onError signature to accept the actual
shape (e.g., ElementsXhrError | AxiosError) and adjust any related types/imports
accordingly so consumers can safely access error fields; locate the onError
declaration in ContentSharingV2 (and any references where errorHandler is
invoked) and replace Error with the correct union type (ElementsXhrError |
AxiosError) or a shared alias that matches the errorHandler output.
---
Nitpick comments:
In `@src/elements/content-sharing/ContentSharingV2.tsx`:
- Around line 243-247: handleOpenChange is recreated each render which can cause
unnecessary re-renders of UnifiedShareModal; wrap it in React.useCallback to
memoize it and include onClose in the dependency array so the callback updates
when onClose changes. Locate the handleOpenChange function in ContentSharingV2
and replace the inline arrow handler with a useCallback-wrapped version (keeping
the same behavior: call onClose?.() when open is false) to match the other
memoized handlers in this file.
- Line 193: The effect with dependency array [api, currentUser, item, itemId,
itemType, sharedLink, getError, onError] includes onError but never uses it;
remove onError from the dependency list in the useEffect declaration in
ContentSharingV2 (the effect guarded by currentUser) so the hook only depends on
the actually used symbols (api, currentUser, item, itemId, itemType, sharedLink,
getError); if onError is meant to be used, instead call it inside the effect
where errors are handled and keep it in the deps—otherwise delete it from the
deps to satisfy correctness and clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57e08d56-75ac-414c-bb2b-40148ba81135
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/tests/ContentSharingV2.test.tsx
82b8a90 to
f0b4c9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
243-258: Consider memoizinghandleOpenChangewithuseCallback.The handler is recreated on every render, potentially causing unnecessary re-renders of
UnifiedShareModal. Wrapping it inuseCallbackimproves referential stability.♻️ Suggested refactor
- const handleOpenChange = (open: boolean) => { - if (!open) { - onClose?.(); - } - }; + const handleOpenChange = React.useCallback((open: boolean) => { + if (!open) { + onClose?.(); + } + }, [onClose]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/ContentSharingV2.tsx` around lines 243 - 258, The handleOpenChange inline function is recreated on every render which can cause UnifiedShareModal to re-render unnecessarily; wrap handleOpenChange in React.useCallback (depend on onClose) to memoize it and pass the stable callback to UnifiedShareModal so referential identity is preserved; update the declaration of handleOpenChange to use useCallback and ensure its dependency array includes onClose (and any other used values).
🤖 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/elements/content-sharing/ContentSharingV2.tsx`:
- Line 193: The dependency array for the useEffect in ContentSharingV2 currently
includes onError even though the effect never calls onError; remove onError from
the dependency list and keep only the actual dependencies (api, currentUser,
item, itemId, itemType, sharedLink, getError) so the effect doesn't re-run when
onError's reference changes; verify the effect still calls getError(error) in
its catch block and update tests or usages if they assumed onError triggered
here.
---
Nitpick comments:
In `@src/elements/content-sharing/ContentSharingV2.tsx`:
- Around line 243-258: The handleOpenChange inline function is recreated on
every render which can cause UnifiedShareModal to re-render unnecessarily; wrap
handleOpenChange in React.useCallback (depend on onClose) to memoize it and pass
the stable callback to UnifiedShareModal so referential identity is preserved;
update the declaration of handleOpenChange to use useCallback and ensure its
dependency array includes onClose (and any other used values).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c2f5a60-6a4e-4485-aa28-669586113a4c
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/tests/ContentSharingV2.test.tsx
f0b4c9a to
8156b62
Compare
3bf81a1 to
43cabae
Compare
43cabae to
bdb6d35
Compare
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
Show resolved
Hide resolved
Merge Queue Status
This pull request spent 7 seconds in the queue, with no time running CI. Required conditions to merge
|
What
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests