Skip to content

feat(Content-sharing): Add event callbacks to Content Sharing#4477

Merged
mergify[bot] merged 5 commits intomasterfrom
add-content-sharing-callbacks
Mar 13, 2026
Merged

feat(Content-sharing): Add event callbacks to Content Sharing#4477
mergify[bot] merged 5 commits intomasterfrom
add-content-sharing-callbacks

Conversation

@reneshen0328
Copy link
Contributor

@reneshen0328 reneshen0328 commented Mar 11, 2026

What

  • Error case: Item fetch fails → white screen with notification → consumer needs to know to ex. close webview/ render notification
  • Success case: User completes action (e.g., "Invite People") → USM closes → consumer needs to know to ex. close webview/ render notification
  • Pull the latest USM shared feature version

Summary by CodeRabbit

  • New Features

    • Content sharing modal now supports event callbacks: onError (item load failed), and onClose (modal closed).
  • Bug Fixes / Improvements

    • Error notifications include styled error text; load lifecycle and modal state more reliably trigger the appropriate callbacks.
  • Tests

    • Added tests verifying onError, and onClose behaviors.

@reneshen0328 reneshen0328 requested review from a team as code owners March 11, 2026 22:07
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds optional callbacks onLoad, onError, and onClose to ContentSharingV2; threads them into the item fetch lifecycle and UnifiedShareModal open-change handling, reorders owner object fields, moves styledText into notification payload, expands effect dependencies, and adds tests for the callbacks.

Changes

Cohort / File(s) Summary
ContentSharingV2 Component
src/elements/content-sharing/ContentSharingV2.tsx
Added onLoad, onError, onClose props; destructured and threaded them through fetch and modal handlers; added handleOpenChange wired as onOpenChange to UnifiedShareModal; call onLoad on successful fetch and onError on fetch failure (prior to existing error handling); reordered owner shape to { email, id, name }; moved styledText into notification payload; expanded useEffect dependency arrays to include callbacks; updated currentUser handling to include id for serverUrl construction.
Tests (ContentSharingV2)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
Added "callback props" test suite with mocks for onLoad, onError, onClose; tests verifying onLoad on success, onError on failure (with error passed), and onClose when modal closes.

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
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested labels

ready-to-merge

Suggested reviewers

  • jfox-box
  • jpan-box
  • tjuanitas

Poem

🐰 I hop along the code-lined trail,
I call onLoad when fetches hail.
If errors puff their stormy breath,
I shout onError before the rest.
When the modal folds, I whisper onClose. 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The pull request description is brief and covers the main objectives (error case, success case, and USM version update), but lacks structured detail typical of the repository's template expectations. Expand the description with more context: explain which callbacks are being added (onLoad, onError, onClose), describe the implementation approach, and clarify the relationship between the component changes and consumer use cases.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding event callbacks (onClose, onError, onLoad) to the Content Sharing component.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch add-content-sharing-callbacks
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)

257-290: Consider adding test coverage for onClose callback.

The new onClose prop is added to ContentSharingV2 but has no corresponding test coverage in this suite. Consider adding a test that simulates modal close and verifies onClose is 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: Remove onError from dependency array—it's unused in this effect.

The useEffect for fetching the current user (lines 170-193) does not invoke onError anywhere in its body, yet onError is included in the dependency array. This causes unnecessary effect re-runs when the onError prop 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 of onError in dependency array.

The onLoad?.() and onError?.(error) calls are correctly placed. However, the dependency array at line 167 includes both onError and onLoad, which is correct since they are used in the effect body.

Note: Line 193 also includes onError in its dependency array, but onError is not called within that effect. Consider removing onError from 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

📥 Commits

Reviewing files that changed from the base of the PR and between 10c815c and 4cda830.

📒 Files selected for processing (2)
  • src/elements/content-sharing/ContentSharingV2.tsx
  • src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx

@reneshen0328 reneshen0328 force-pushed the add-content-sharing-callbacks branch 3 times, most recently from 0e2454b to 4ce8b39 Compare March 12, 2026 16:37
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)

193-193: Unnecessary onError in dependency array.

The onError callback is not used within this useEffect (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

📥 Commits

Reviewing files that changed from the base of the PR and between 4cda830 and 2b29352.

📒 Files selected for processing (2)
  • src/elements/content-sharing/ContentSharingV2.tsx
  • src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Stabilize these effect dependencies before adding callback props.

getError changes identity when hasError flips, and onLoad / onError will also change whenever a parent passes fresh inline callbacks. While item or currentUser is still null, that re-runs these effects and can issue duplicate fetchItem() / fetchCurrentUser() requests. It can also fire onError more than once. onError on 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 | 🔴 Critical

Use onRequestClose instead of onOpenChange for the UnifiedShareModal callback.

The in-repo UnifiedShareModal component expects the close handler via the onRequestClose prop (see src/features/unified-share-modal/flowTypes.js:396 and UnifiedShareModal.js:193, 212). Passing onOpenChange will have no effect and onClose will 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2b29352 and 4ce8b39.

📒 Files selected for processing (2)
  • src/elements/content-sharing/ContentSharingV2.tsx
  • src/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

@reneshen0328 reneshen0328 force-pushed the add-content-sharing-callbacks branch from 4ce8b39 to 82b8a90 Compare March 12, 2026 16:58
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)

243-247: Consider memoizing handleOpenChange with useCallback.

This handler is recreated on every render, which could trigger unnecessary re-renders of UnifiedShareModal. Wrapping it in useCallback with onClose as 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: onError is not used in this effect.

The onError callback is included in the dependency array but never invoked within this effect. While the currentUser guard 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4ce8b39 and 82b8a90.

📒 Files selected for processing (2)
  • src/elements/content-sharing/ContentSharingV2.tsx
  • src/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

@reneshen0328 reneshen0328 force-pushed the add-content-sharing-callbacks branch from 82b8a90 to f0b4c9a Compare March 12, 2026 17:16
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)

243-258: Consider memoizing handleOpenChange with useCallback.

The handler is recreated on every render, potentially causing unnecessary re-renders of UnifiedShareModal. Wrapping it in useCallback improves 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

📥 Commits

Reviewing files that changed from the base of the PR and between 82b8a90 and f0b4c9a.

📒 Files selected for processing (2)
  • src/elements/content-sharing/ContentSharingV2.tsx
  • src/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

@reneshen0328 reneshen0328 force-pushed the add-content-sharing-callbacks branch from f0b4c9a to 8156b62 Compare March 12, 2026 18:16
@reneshen0328 reneshen0328 force-pushed the add-content-sharing-callbacks branch from 3bf81a1 to 43cabae Compare March 12, 2026 21:08
@reneshen0328 reneshen0328 force-pushed the add-content-sharing-callbacks branch from 43cabae to bdb6d35 Compare March 12, 2026 21:37
tjuanitas
tjuanitas previously approved these changes Mar 12, 2026
jfox-box
jfox-box previously approved these changes Mar 12, 2026
@reneshen0328 reneshen0328 dismissed stale reviews from jfox-box and tjuanitas via a897494 March 12, 2026 22:32
@reneshen0328 reneshen0328 requested a review from a team as a code owner March 12, 2026 22:32
jfox-box
jfox-box previously approved these changes Mar 12, 2026
jpan-box
jpan-box previously approved these changes Mar 12, 2026
@reneshen0328 reneshen0328 dismissed stale reviews from jpan-box and jfox-box via dde00cb March 12, 2026 23:50
@mergify mergify bot added the queued label Mar 13, 2026
@mergify mergify bot merged commit 8e222b8 into master Mar 13, 2026
11 checks passed
@mergify mergify bot deleted the add-content-sharing-callbacks branch March 13, 2026 16:45
@mergify
Copy link
Contributor

mergify bot commented Mar 13, 2026

Merge Queue Status

  • Entered queue2026-03-13 16:45 UTC · Rule: Automatic strict merge
  • Checks passed · in-place
  • Merged2026-03-13 16:45 UTC · at dde00cba81ceac7ca02d3f6ccdf7af2294498980

This pull request spent 7 seconds in the queue, with no time running CI.

Required conditions to merge

@mergify mergify bot removed the queued label Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants