feat(content-sharing): Display pending collab#4509
Open
reneshen0328 wants to merge 2 commits intomasterfrom
Open
feat(content-sharing): Display pending collab#4509reneshen0328 wants to merge 2 commits intomasterfrom
reneshen0328 wants to merge 2 commits intomasterfrom
Conversation
Contributor
WalkthroughExtended collaboration types with an optional Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (1)
58-73: LGTM — test coverage matches the new branches.Mocks cover all three new paths (pending with
invite_email, pending without, rejected), index arithmetic intomockCollaborationsis correct givenitemOwneris prepended, and the differingisExternalexpectations between the directconvertCollabtest (passesisCurrentUserOwner: false) and theconvertCollabsResponsetest (derivesisCurrentUserOwner: truesincecurrentUserId === ownerId) correctly reflect the implementation's short-circuit on ownership.Optional nit: the
convertCollabtest usesmockCollaborations[4],mockCollaborations[3],mockCollaborations[5]by numeric index, which gets fragile as the fixture grows. Consider pulling those entries into named constants (e.g.pendingWithInviteEmail,pendingWithoutInviteEmail,rejectedCollab) to make intent obvious and avoid off-by-one breakage if someone inserts another entry.Also applies to: 105-163, 243-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts` around lines 58 - 73, The test uses fragile numeric indexes into mockCollaborations (e.g., mockCollaborations[4], [3], [5]) inside the convertCollab test; extract the specific fixtures into named constants (for example pendingWithInviteEmail, pendingWithoutInviteEmail, rejectedCollab) and reference those constants in the convertCollab and convertCollabsResponse tests so intent is clear and adding entries to mockCollaborations won't break the indexes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts`:
- Around line 58-73: The test uses fragile numeric indexes into
mockCollaborations (e.g., mockCollaborations[4], [3], [5]) inside the
convertCollab test; extract the specific fixtures into named constants (for
example pendingWithInviteEmail, pendingWithoutInviteEmail, rejectedCollab) and
reference those constants in the convertCollab and convertCollabsResponse tests
so intent is clear and adding entries to mockCollaborations won't break the
indexes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 379997c1-39ac-4e50-a460-e698e2586764
📒 Files selected for processing (4)
src/common/types/core.jssrc/constants.jssrc/elements/content-sharing/utils/__tests__/convertCollaborators.test.tssrc/elements/content-sharing/utils/convertCollaborators.ts
reneshen0328
commented
Apr 21, 2026
jpan-box
reviewed
Apr 21, 2026
tjuanitas
reviewed
Apr 21, 2026
tjiang-box
previously approved these changes
Apr 21, 2026
jpan-box
reviewed
Apr 21, 2026
tjuanitas
reviewed
Apr 21, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/__tests__/convertCollaborators.test.ts`:
- Around line 347-363: The test currently passes mockOwnerId as the "current
user" which short-circuits owner logic; update the call to
convertCollabsResponse to pass a non-owner current user id (e.g., a new/mock
nonOwnerId) so the code exercises the "owner email has no domain" path using
ownerWithoutEmailDomain and verifies externalInvite.isExternal is computed from
the domain fallback rather than the owner short-circuit.
🪄 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: 1b0e8f2e-76aa-4eb6-b548-05c450d8f72a
📒 Files selected for processing (2)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.tssrc/elements/content-sharing/utils/convertCollaborators.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/utils/convertCollaborators.ts
reneshen0328
force-pushed
the
add-pending-collab-to-content-sharing
branch
from
21d18f2 to
a040981
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (1)
347-363:⚠️ Potential issue | 🟡 MinorMake this test avoid the owner short-circuit.
Line 356 still passes
mockOwnerId, soisExternal: falsecan be satisfied because the current user is the owner rather than because the owner email has no domain. Use a non-owner current user id here.Proposed test adjustment
const result = convertCollabsResponse( mockCollaborationsFromApi, - mockOwnerId, + 'non-owner-user-id', ownerWithoutEmailDomain, mockAvatarUrlMap, ); const externalInvite = result.find(collab => collab.email === 'bbear@external.example.com'); - expect(externalInvite.isExternal).toBe(false); + expect(externalInvite).toMatchObject({ isExternal: false });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts` around lines 347 - 363, The test currently passes mockOwnerId into convertCollabsResponse which short-circuits owner logic; replace that argument with a non-owner id (e.g., mockNonOwnerId or any id !== mockOwnerId) so the code path that checks ownerWithoutEmailDomain.email (the owner object) is exercised; keep the rest of the test (ownerWithoutEmailDomain, mockCollaborationsFromApi, mockAvatarUrlMap, and the expectation on externalInvite.isExternal) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts`:
- Around line 347-363: The test currently passes mockOwnerId into
convertCollabsResponse which short-circuits owner logic; replace that argument
with a non-owner id (e.g., mockNonOwnerId or any id !== mockOwnerId) so the code
path that checks ownerWithoutEmailDomain.email (the owner object) is exercised;
keep the rest of the test (ownerWithoutEmailDomain, mockCollaborationsFromApi,
mockAvatarUrlMap, and the expectation on externalInvite.isExternal) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b238ba66-32ff-4efa-86b0-ab63ed3b642f
📒 Files selected for processing (2)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.tssrc/elements/content-sharing/utils/convertCollaborators.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/utils/convertCollaborators.ts
reneshen0328
commented
Apr 21, 2026
reneshen0328
force-pushed
the
add-pending-collab-to-content-sharing
branch
from
a040981 to
074b213
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/convertCollaborators.ts`:
- Around line 35-49: The guard that only assigns collabId/collabEmail/collabName
when accessibleBy?.name is present drops identity for partial accessible_by
objects; update the logic in convertCollaborators (the block setting collabId,
collabEmail, collabName from accessibleBy) to populate collabId from
accessibleBy.id and collabEmail from accessibleBy.login whenever those fields
exist (independently of accessibleBy.name), and only override collabName if
accessibleBy.name is truthy; also ensure the later check that returns null uses
collabName || collabEmail to allow pending invites to survive (apply the same
fix to the similar branch around the other conditional referenced in the diff).
🪄 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: e875b75f-c282-4a60-9891-0251a92b7c2d
📒 Files selected for processing (2)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.tssrc/elements/content-sharing/utils/convertCollaborators.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/utils/tests/convertCollaborators.test.ts
reneshen0328
commented
Apr 22, 2026
jpan-box
approved these changes
Apr 22, 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
4 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.
Before: We skip any pending collaborators.
After:
Summary by CodeRabbit
New Features
Bug Fixes
Tests