fix(content-sharing): Collaborator current user #4328
Conversation
WalkthroughI pity the fool — owner was added across the sharing flow: a new INVITEE_ROLE_OWNER constant, item conversion now exposes API Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant V as ContentSharingV2
participant I as Item API
participant C as Collab API
participant CI as convertItemResponse
participant CC as convertCollaborators
U->>V: Open Content Sharing
V->>I: Fetch item
I-->>V: Item JSON (includes owned_by)
V->>CI: convertItemResponse(item)
CI-->>V: { item, sharedLink, collaborationRoles, ownedBy }
Note over V: Set owner state (id, email, name)
V->>C: Fetch collaborations
C-->>V: Collaborations JSON
V->>CC: convertCollabsResponse(collabs, currentUserId, owner, avatarURLMap)
rect rgba(200,255,200,0.12)
Note over CC: Prepend owner entry (role: OWNER) and compute flags
end
CC-->>V: Collaborators list (with owner/current-user/external flags)
V-->>U: Render updated sharing UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
reneshen0328
force-pushed
the
fix-collab-current-user
branch
from
c706cb9 to
a655fd8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (1)
230-230: I pity the fool who thinks this external user ain't external!The test expects
isExternal: falsefor 'rqueen@external.com', but this user has a different email domain ('external.com') than the owner's domain ('example.com'). According to theconvertCollablogic (line 34 of convertCollaborators.ts), this should be marked as external when the current user is not the owner.Verify this behavior and update the test expectation if needed:
#!/bin/bash # Verify the isExternal logic for users with different email domains ast-grep --pattern $'const isExternal = $$$;'
🧹 Nitpick comments (1)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (1)
192-192: This comment needs fixing, sucka!The comment says "Only accepted collaborations" but the expected result includes the owner entry plus 2 accepted collaborators (3 total). Update the comment to be more accurate.
Apply this diff:
- expect(result).toHaveLength(3); // Only accepted collaborations + expect(result).toHaveLength(3); // Owner + 2 accepted collaborations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/constants.js(1 hunks)src/elements/content-sharing/ContentSharingV2.tsx(2 hunks)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js(4 hunks)src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts(8 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(2 hunks)src/elements/content-sharing/utils/convertCollaborators.ts(2 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T16:47:50.687Z
Learnt from: reneshen0328
PR: box/box-ui-elements#4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.687Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.tssrc/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.jssrc/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts
🧬 Code graph analysis (3)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (14)
mockOwnerId(36-36)mockOwnerId(36-36)mockOwnerEmail(34-34)mockOwnerEmail(34-34)mockOwnerName(35-35)mockOwnerName(35-35)collabUser1(38-43)collabUser1(38-43)collabUser2(45-49)collabUser2(45-49)collabUser3(51-55)collabUser3(51-55)mockAvatarURLMap(30-32)mockAvatarURLMap(30-32)src/elements/content-sharing/utils/convertCollaborators.ts (2)
convertCollab(16-51)convertCollabsResponse(53-81)
src/elements/content-sharing/ContentSharingV2.tsx (2)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(8-105)src/elements/content-sharing/utils/convertCollaborators.ts (1)
convertCollabsResponse(53-81)
src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
mockOwnerId(36-36)mockOwnerId(36-36)mockOwnerEmail(34-34)mockOwnerEmail(34-34)mockOwnerName(35-35)mockOwnerName(35-35)
🪛 Biome (2.1.2)
src/constants.js
[error] 203-203: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (16)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (3)
15-29: LGTM, fool! Test setup is solid.The owner-related test constants are properly structured and match the production logic where the owner entry gets prepended to the collaborations list. The itemOwner mock correctly mirrors the structure created in convertCollabsResponse.
31-60: This mock data looks good, sucka!The collaborations mock structure correctly includes the created_by field and the mockCollaborations array properly prepends the itemOwner entry, matching the production behavior.
64-181: Test updates are solid, fool!All the convertCollab tests correctly pass the new owner-related parameters. The id: '0' for the owner entry (line 127) is expected since the itemOwner object doesn't have an id field, defaulting to 0 as per the destructuring logic.
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (3)
35-36: Looking good, fool!The mockOwnerName and mockOwnerId exports are properly defined and match the structure used throughout the test files.
57-61: mockOwner structure is solid, sucka!The mockOwner object correctly aggregates the owner properties and will be used in the DEFAULT_ITEM_API_RESPONSE to represent the owned_by field.
102-102: LGTM! owned_by is properly set.The DEFAULT_ITEM_API_RESPONSE now includes owned_by: mockOwner, which aligns with the Box API structure and supports the owner-based collaboration logic.
src/elements/content-sharing/utils/convertCollaborators.ts (2)
8-14: Interface updates look solid, fool!The ConvertCollabProps interface properly reflects the new owner-based parameters needed for determining current user and external user status.
53-80: This owner logic is on point, sucka!The convertCollabsResponse function correctly:
- Destructures owner properties
- Determines if current user is owner
- Extracts owner email domain for external user detection
- Creates and prepends the owner entry to the collaborations list
- Passes all necessary context to convertCollab
src/constants.js (1)
203-203: This constant is legit, fool!The INVITEE_ROLE_OWNER constant follows the same pattern as INVITEE_ROLE_EDITOR and properly represents the owner role used in the collaboration logic.
Note: The Biome static analysis error about type annotations is a false positive - this file uses Flow syntax (see line 2
@flow), not TypeScript.src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts (2)
5-7: Import updates look good, sucka!The new owner-related imports are necessary for verifying the owned_by field in the test expectations.
39-43: Test expectations are solid, fool!The test correctly verifies that convertItemResponse returns the owned_by object at the top level with the expected structure (id, login, name).
src/elements/content-sharing/ContentSharingV2.tsx (3)
49-49: Check this owner state initialization, fool!The owner state is initialized with empty strings. Verify that this doesn't cause issues in the effect at line 130 where it checks
if (avatarURLMap && collaboratorsData && currentUser && owner). An object with empty strings is truthy and will pass the check even before real data loads.Consider using null for the initial state or updating the condition to check for non-empty owner values:
- const [owner, setOwner] = React.useState({ ownerId: '', ownerEmail: '', ownerName: '' }); + const [owner, setOwner] = React.useState<{ ownerId: string; ownerEmail: string; ownerName: string } | null>(null);And update the condition:
- if (avatarURLMap && collaboratorsData && currentUser && owner) { + if (avatarURLMap && collaboratorsData && currentUser && owner && owner.ownerId) {
52-64: handleGetItemSuccess is solid, sucka!The function correctly extracts owned_by from the converted item response and sets the owner state with the proper structure.
129-139: Effect logic looks good, fool!The effect properly waits for all required data before calling convertCollabsResponse and correctly passes the owner object. The dependency array is complete with all necessary dependencies.
src/elements/content-sharing/utils/convertItemResponse.ts (2)
16-16: LGTM! owned_by extraction is clean.The owned_by field is properly destructured from the API response data.
103-103: Return value is solid, fool!The owned_by field is correctly included in the top-level return object, making owner information available to downstream components.
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js
Outdated
Show resolved
Hide resolved
reneshen0328
force-pushed
the
fix-collab-current-user
branch
from
a655fd8 to
d392ce4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/elements/content-sharing/ContentSharingV2.tsx (1)
49-64: Owner extraction looks right, sucka.CamelCase fields and sourcing from owned_by are correct and align with prior feedback.
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (1)
47-49: Nice domain choice, chump.Using external.example.com avoids linking to a real domain.
🧹 Nitpick comments (3)
src/elements/content-sharing/ContentSharingV2.tsx (1)
129-140: Guard on a real owner before converting, fool.Avoid passing an empty owner shell. Check owner.id in the condition.
- if (avatarURLMap && collaboratorsData && currentUser && owner) { + if (avatarURLMap && collaboratorsData && currentUser && owner && owner.id) {src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (1)
65-80: Fix undefined collaboration type, fool.
type: user.typewill be undefined since users lack atype. Set the collaboration record type explicitly.export const MOCK_COLLABORATIONS_RESPONSE = { entries: MOCK_COLLABORATORS.map(user => ({ id: `record_${user.id}`, accessible_by: user, expires_at: user.expires_at, created_by: { id: mockOwnerId, login: mockOwnerEmail, name: mockOwnerName, type: 'user', }, role: user.id === mockOwnerId ? 'owner' : 'editor', status: 'accepted', - type: user.type, + type: 'collaboration', })), };src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (1)
247-255: Align null handling with types, fool.You pass
nullfor avatarURLMap. Either passundefinedhere or widen the function type to acceptnull(see suggested changes in convertCollaborators.ts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/constants.js(1 hunks)src/elements/content-sharing/ContentSharingV2.tsx(2 hunks)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js(4 hunks)src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts(8 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(2 hunks)src/elements/content-sharing/utils/convertCollaborators.ts(3 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sharing/utils/convertItemResponse.ts
- src/elements/content-sharing/utils/tests/convertItemResponse.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T16:47:50.687Z
Learnt from: reneshen0328
PR: box/box-ui-elements#4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.687Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.tssrc/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js
🧬 Code graph analysis (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(8-105)src/elements/content-sharing/utils/convertCollaborators.ts (1)
convertCollabsResponse(53-82)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (14)
mockOwnerId(36-36)mockOwnerId(36-36)mockOwnerEmail(34-34)mockOwnerEmail(34-34)mockOwnerName(35-35)mockOwnerName(35-35)collabUser1(38-43)collabUser1(38-43)collabUser2(45-49)collabUser2(45-49)collabUser3(51-55)collabUser3(51-55)mockAvatarURLMap(30-32)mockAvatarURLMap(30-32)src/elements/content-sharing/utils/convertCollaborators.ts (2)
convertCollab(16-51)convertCollabsResponse(53-82)
🪛 Biome (2.1.2)
src/constants.js
[error] 203-203: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/constants.js (1)
203-203: Owner invitee role constant — LGTM. Verify linter compatibility, fool.Matches existing Flow literal style. If Biome whines, either tune config or drop the literal annotation for this line.
Optional fallback diff if needed by linter:
-export const INVITEE_ROLE_OWNER: 'owner' = 'owner'; +export const INVITEE_ROLE_OWNER = 'owner';
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks. You may have to fix your CI before adding the pull request to the queue again. |
What
Testing
Notes: shared with should render collaborators without the owner, whereas the managed collaborators view should render all collaborators including the owner
Summary by CodeRabbit
New Features
Improvements
Tests