feat(Content-sharing): Add event callbacks to Content Sharing#4477
feat(Content-sharing): Add event callbacks to Content Sharing#4477reneshen0328 wants to merge 1 commit intomasterfrom
Conversation
WalkthroughThe PR adds three new optional callbacks to ContentSharingV2: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 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
| test('should call onLoad and not onError when item data loads successfully', async () => { | ||
| renderComponent({ onLoad }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(onLoad).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| expect(onError).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Test assertion doesn't validate component behavior.
Line 278 asserts expect(onError).not.toHaveBeenCalled(), but onError was never passed to the component in this test. This assertion will always pass regardless of component behavior—it only confirms an unpassed mock wasn't called.
To properly test that the component doesn't call onError on success, pass both callbacks:
Proposed fix
test('should call onLoad and not onError when item data loads successfully', async () => {
- renderComponent({ onLoad });
+ renderComponent({ onLoad, onError });
await waitFor(() => {
expect(onLoad).toHaveBeenCalled();
});
expect(onError).not.toHaveBeenCalled();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should call onLoad and not onError when item data loads successfully', async () => { | |
| renderComponent({ onLoad }); | |
| await waitFor(() => { | |
| expect(onLoad).toHaveBeenCalled(); | |
| }); | |
| expect(onError).not.toHaveBeenCalled(); | |
| }); | |
| test('should call onLoad and not onError when item data loads successfully', async () => { | |
| renderComponent({ onLoad, onError }); | |
| await waitFor(() => { | |
| expect(onLoad).toHaveBeenCalled(); | |
| }); | |
| expect(onError).not.toHaveBeenCalled(); | |
| }); |
🤖 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 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.
| test('should call onError and not onLoad when item data fails to load', async () => { | ||
| renderComponent({ api: errorApi, onError }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(onError).toHaveBeenCalledWith(error); | ||
| }); | ||
|
|
||
| expect(onLoad).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Same issue: pass both callbacks to validate mutual exclusivity.
Line 288 asserts expect(onLoad).not.toHaveBeenCalled(), but onLoad was never passed. Pass both callbacks to properly verify the component's error path doesn't invoke onLoad.
Proposed fix
test('should call onError and not onLoad when item data fails to load', async () => {
- renderComponent({ api: errorApi, onError });
+ renderComponent({ api: errorApi, onLoad, onError });
await waitFor(() => {
expect(onError).toHaveBeenCalledWith(error);
});
expect(onLoad).not.toHaveBeenCalled();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should call onError and not onLoad when item data fails to load', async () => { | |
| renderComponent({ api: errorApi, onError }); | |
| await waitFor(() => { | |
| expect(onError).toHaveBeenCalledWith(error); | |
| }); | |
| expect(onLoad).not.toHaveBeenCalled(); | |
| }); | |
| test('should call onError and not onLoad when item data fails to load', async () => { | |
| renderComponent({ api: errorApi, onLoad, onError }); | |
| await waitFor(() => { | |
| expect(onError).toHaveBeenCalledWith(error); | |
| }); | |
| expect(onLoad).not.toHaveBeenCalled(); | |
| }); |
🤖 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 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.
| onClose?: () => void; | ||
| /** onError - Callback when item data fails to load, preventing USM from opening */ | ||
| onError?: (error: Error) => void; | ||
| /** onLoad - Callback when item data loads successfully, use to hide loading indicator */ |
There was a problem hiding this comment.
there is currently no loading indicator used in content sharing
| }); | ||
| }); | ||
|
|
||
| describe('callback props', () => { |
There was a problem hiding this comment.
should we add a test for onClose?
2 participants
What
Summary by CodeRabbit
Release Notes