Conditionally display share modal header based on user permissions#4141
Conditionally display share modal header based on user permissions#4141peterchinman wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughIn 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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 `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 992-1000: The fallback text "Document owner" in the Show component
at ShareButton.tsx does not accurately represent the card content that displays
below it. The card content renders unconditionally for both owners and
non-owners, showing both the document owner and all people with access, but the
fallback header only says "Document owner". Update the fallback attribute in the
Show component (which checks props.userPermissions === Permissions.OWNER) to use
text that accurately reflects the full content being displayed, such as "People
with access", so the header matches what users actually see in the card.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8797f48c-e6bd-4d95-9be7-59bb21f8cac0
📒 Files selected for processing (1)
js/app/packages/core/component/TopBar/ShareButton.tsx
| <Show | ||
| when={props.userPermissions === Permissions.OWNER} | ||
| fallback="Document owner" | ||
| > | ||
| People with access to this{' '} | ||
| {props.itemType === 'email' | ||
| ? 'email thread' | ||
| : props.itemType} | ||
| </Show> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of ShareModal to understand permission contexts
rg -nP --type=tsx --type=ts -C5 '<ShareModal\b' -g '!*.test.*' -g '!*.spec.*'Repository: macro-inc/macro
Length of output: 87
🏁 Script executed:
rg -nC5 'ShareModal' --type ts --type js -g '!*.test.*' -g '!*.spec.*'Repository: macro-inc/macro
Length of output: 15322
🏁 Script executed:
# Find the ShareModal component definition and its props
rg -nC10 'interface.*ShareModalProps\|type.*ShareModalProps' --type ts --type jsRepository: macro-inc/macro
Length of output: 41
🏁 Script executed:
# Check the mobile version (MobileShareDrawer) for similar conditional header logic
rg -nA20 'MobileShareDrawer' js/app/packages/core/component/TopBar/ShareButton.tsx | head -100Repository: macro-inc/macro
Length of output: 2465
🏁 Script executed:
sed -n '980,1120p' js/app/packages/core/component/TopBar/ShareButton.tsxRepository: macro-inc/macro
Length of output: 6907
🏁 Script executed:
# Also check GlobalShareModal to see what userPermissions value it passes
sed -n '65,90p' js/app/packages/app/component/global-share-modal/GlobalShareModal.tsxRepository: macro-inc/macro
Length of output: 782
🏁 Script executed:
# Check mobile version header/tab rendering
sed -n '282,350p' js/app/packages/core/component/TopBar/ShareButton.tsxRepository: macro-inc/macro
Length of output: 2491
🏁 Script executed:
# Search for other places where ShareModal might be instantiated (not via GlobalShareModal)
rg -n 'ShareModal\s*<|new\s+ShareModal\|<ShareModal' --type ts --type tsx -g '!*.test.*' -g '!*.spec.*'Repository: macro-inc/macro
Length of output: 87
🏁 Script executed:
rg -n '<ShareModal' -g '!*.test.*' -g '!*.spec.*' -C3 | head -60Repository: macro-inc/macro
Length of output: 1366
🏁 Script executed:
# Check the first local usage of ShareModal (around line 1327)
sed -n '1320,1340p' js/app/packages/core/component/TopBar/ShareButton.tsxRepository: macro-inc/macro
Length of output: 613
The fallback header "Document owner" misrepresents the card content.
When non-owners access ShareModal (via useShareModalContext at line 1327), they see the fallback header "Document owner"—but the card displays both the document owner and all recipients with access. This mismatch creates confusion about what information is being shown.
The card content (lines 1020–1073) renders unconditionally for both owners and non-owners; there is no permission-based filtering. The header should accurately reflect this.
Options to fix:
-
Update the fallback to match card content:
fallback="People with access"
-
Make the header consistent with mobile, which uses the "People" tab label instead of a conditional header.
Also note: the mobile version (MobileShareDrawer) avoids this issue by using a generic "People" tab label rather than a conditional header.
🤖 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 `@js/app/packages/core/component/TopBar/ShareButton.tsx` around lines 992 -
1000, The fallback text "Document owner" in the Show component at
ShareButton.tsx does not accurately represent the card content that displays
below it. The card content renders unconditionally for both owners and
non-owners, showing both the document owner and all people with access, but the
fallback header only says "Document owner". Update the fallback attribute in the
Show component (which checks props.userPermissions === Permissions.OWNER) to use
text that accurately reflects the full content being displayed, such as "People
with access", so the header matches what users actually see in the card.
2 participants
Summary
Updated the ShareModal component to conditionally render the access list header based on the user's permission level. When the user is not the owner, a simplified "Document owner" message is displayed instead of the detailed access information.
Key Changes
Showcomponent that checks if the user hasOWNERpermissionsImplementation Details
Showcomponent for conditional renderingprops.userPermissions === Permissions.OWNERhttps://claude.ai/code/session_01P9sbpmXEyBuK78sxxseQ9c