fix(content-sharing): USM Expiration Permission For Individual Users#4526
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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/ContentSharingV2.tsx`:
- Around line 183-205: getUserSuccess only clamps canChangeExpiration during
initial user fetch, so later sharedLink updates can re-enable expiration;
persist the enterprise info in state (e.g., include enterprise on setCurrentUser
in getUserSuccess) and add a useEffect that watches sharedLink and the stored
enterprise and, when enterprise?.name is falsy, always call setSharedLink to set
settings.canChangeExpiration = false (safely returning prevSharedLink if
missing) so the flag is clamped on every sharedLink change as well as on initial
load.
🪄 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: 65ff497a-a6b2-4b52-be7b-a6c39db5162a
📒 Files selected for processing (3)
src/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsxsrc/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js
7fe70e7 to
d60c714
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
221-237: 💤 Low valueAddresses past review concern, but consider tightening the dependency array.
This useEffect correctly clamps
canChangeExpirationon everysharedLinkupdate for non-enterprise users, addressing the earlier feedback. However, sincesharedLinkis in the dependency array and we mutate it inside, this will cause one extra render cycle each timesharedLinkchanges withcanChangeExpiration: true.The current approach works (no infinite loop due to the early return), but you could avoid the extra render by moving this logic into
handleGetItemSuccessand the sharing service callbacks that updatesharedLink, rather than reacting to state 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 221 - 237, The effect currently watches sharedLink and flips canChangeExpiration via setSharedLink inside the useEffect, causing an extra render whenever sharedLink arrives with canChangeExpiration:true; move the clamping logic out of the useEffect and into the code paths that first set sharedLink instead (e.g. inside handleGetItemSuccess and the sharing service callbacks that produce/update sharedLink) so you normalize the sharedLink before calling setSharedLink (or before dispatching the update) instead of reacting to it in useEffect; update handleGetItemSuccess and the sharing service update handlers to ensure sharedLink.settings.canChangeExpiration is set to false for non-enterprise users prior to state update, and then remove sharedLink from the useEffect dependency to avoid the redundant render.
🤖 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 193-194: The boolean naming is inverted: isEnterpriseUser is set
true when no enterprise exists; rename the state and setter to reflect actual
semantics (e.g., isIndividualUser and setIsIndividualUser) and update all usages
accordingly (replace isEnterpriseUser/setIsEnterpriseUser with
isIndividualUser/setIsIndividualUser and adjust related checks such as the
conditional that currently reads if (!isEnterpriseUser || ...) to use if
(!isIndividualUser || ...)); alternatively, if you prefer to keep the current
name, invert the assignment logic in the initialization and anywhere you set the
flag (ensure setIsEnterpriseUser(false) is called where the user is not
enterprise) so the variable meaning matches its value.
---
Nitpick comments:
In `@src/elements/content-sharing/ContentSharingV2.tsx`:
- Around line 221-237: The effect currently watches sharedLink and flips
canChangeExpiration via setSharedLink inside the useEffect, causing an extra
render whenever sharedLink arrives with canChangeExpiration:true; move the
clamping logic out of the useEffect and into the code paths that first set
sharedLink instead (e.g. inside handleGetItemSuccess and the sharing service
callbacks that produce/update sharedLink) so you normalize the sharedLink before
calling setSharedLink (or before dispatching the update) instead of reacting to
it in useEffect; update handleGetItemSuccess and the sharing service update
handlers to ensure sharedLink.settings.canChangeExpiration is set to false for
non-enterprise users prior to state update, and then remove sharedLink from the
useEffect dependency to avoid the redundant render.
🪄 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: fe698bd7-2870-456b-bcce-585d55f4ac35
📒 Files selected for processing (1)
src/elements/content-sharing/ContentSharingV2.tsx
4b5dc68 to
cb6aebd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 185-187: The handler getUserSuccess currently destructures
userData (const { enterprise, hostname, id } = userData) but fetchCurrentUser
can return null; update getUserSuccess to guard for a null/undefined userData
before destructuring (e.g., return or setCurrentUser(null)/fallback when
userData is falsy) and apply the same null-check protection to the similar logic
around lines 198-199; reference the getUserSuccess function and any other
callbacks that consume fetchCurrentUser results to ensure no direct
destructuring happens without a preceding truthy check.
- Around line 212-216: The updater passed to setSharedLink can assume
prevSharedLink exists but may be null if a reset (setSharedLink(null))
interleaves; update the setSharedLink(prevSharedLink => ...) callback in
ContentSharingV2 to defensively handle prevSharedLink === null by returning an
appropriate value (e.g., null or a new object) instead of accessing
prevSharedLink.settings, ensuring you guard reads of prevSharedLink.settings and
prevSharedLink before spreading or modifying fields like settings and
canChangeExpiration.
🪄 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: 8e20469d-fd57-442e-adf9-ba08581b9c10
📒 Files selected for processing (1)
src/elements/content-sharing/ContentSharingV2.tsx
9e5a2eb to
8f6aa0b
Compare
Merge Queue Status
This pull request spent 10 minutes 3 seconds in the queue, including 9 minutes 50 seconds running CI. Required conditions to merge
|
What
Summary by CodeRabbit
New Features
Behavior
Tests