Fix content sharing external badge#4589
Conversation
WalkthroughPass a full ChangesContent sharing: currentUser + external gating update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
| // External collaborator icons will only be displayed in the USM if the current user owns | ||
| // the item and if the collaborator's email domain differs from the owner's email domain. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elements/content-sharing/hooks/useSharingService.ts`:
- Around line 95-97: The invite-success path dereferences currentUser
(currentUser.id) without a null check; update the logic where the object with
currentUser, isCurrentUserOwner, and ownerEmailDomain is built (the block
referencing currentUser and ownerId) to guard currentUser first — compute
isCurrentUserOwner only if currentUser exists (e.g., currentUser &&
currentUser.id === ownerId or optional chaining), and only append to
collaborators or access any currentUser properties when currentUser is non-null;
ensure the invite callback in useSharingService.ts skips collaborator mutations
or uses a safe fallback when currentUser is null.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8ad0dee-2450-428a-8490-5ed350e7a6e7
📒 Files selected for processing (8)
src/constants.jssrc/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/apis/__tests__/fetchCurrentUser.test.tssrc/elements/content-sharing/apis/fetchCurrentUser.tssrc/elements/content-sharing/hooks/__tests__/useSharingService.test.tssrc/elements/content-sharing/hooks/useSharingService.tssrc/elements/content-sharing/utils/__tests__/convertCollaborators.test.tssrc/elements/content-sharing/utils/convertCollaborators.ts
✅ Files skipped from review due to trivial changes (1)
- src/elements/content-sharing/apis/fetchCurrentUser.ts
| currentUser, | ||
| isCurrentUserOwner: currentUser.id === ownerId, | ||
| ownerEmailDomain, |
There was a problem hiding this comment.
Guard currentUser before dereferencing in invite success path.
Line 96 can throw when currentUser is still null (caller initializes it as null). Add a null-safe guard before using currentUser.id and appending collaborators.
Proposed fix
setCollaborators(prevCollabs => {
+ if (!currentUser) {
+ return prevCollabs;
+ }
+
const nextCollab = convertCollab({
avatarUrlMap,
collab: response,
currentUser,
isCurrentUserOwner: currentUser.id === ownerId,
ownerEmailDomain,
});
- return nextCollab ? [...prevCollabs, nextCollab] : prevCollabs;
+ return nextCollab ? [...(prevCollabs ?? []), nextCollab] : prevCollabs;
});📝 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.
| currentUser, | |
| isCurrentUserOwner: currentUser.id === ownerId, | |
| ownerEmailDomain, | |
| setCollaborators(prevCollabs => { | |
| if (!currentUser) { | |
| return prevCollabs; | |
| } | |
| const nextCollab = convertCollab({ | |
| avatarUrlMap, | |
| collab: response, | |
| currentUser, | |
| isCurrentUserOwner: currentUser.id === ownerId, | |
| ownerEmailDomain, | |
| }); | |
| return nextCollab ? [...(prevCollabs ?? []), nextCollab] : prevCollabs; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/elements/content-sharing/hooks/useSharingService.ts` around lines 95 -
97, The invite-success path dereferences currentUser (currentUser.id) without a
null check; update the logic where the object with currentUser,
isCurrentUserOwner, and ownerEmailDomain is built (the block referencing
currentUser and ownerId) to guard currentUser first — compute isCurrentUserOwner
only if currentUser exists (e.g., currentUser && currentUser.id === ownerId or
optional chaining), and only append to collaborators or access any currentUser
properties when currentUser is non-null; ensure the invite callback in
useSharingService.ts skips collaborator mutations or uses a safe fallback when
currentUser is null.
Summary by CodeRabbit
Bug Fixes
Improvements