feat: implement open/closing logic for modernized content uploader#4628
Conversation
WalkthroughAdds a modernized uploads manager panel with slide-in/slide-out CSS animations to ChangesModernized Uploads Manager Panel Dismiss Lifecycle
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentUploader
participant modernizedPanelState
participant DismissTimer
User->>ContentUploader: Uploads batch starts
ContentUploader->>modernizedPanelState: showModernizedPanel() → 'shown'
ContentUploader->>ContentUploader: componentDidUpdate detects batch complete
ContentUploader->>DismissTimer: startModernizedDismissTimer()
User->>ContentUploader: Mouse hover enter
ContentUploader->>DismissTimer: clearModernizedDismissTimer()
User->>ContentUploader: Mouse hover leave
ContentUploader->>DismissTimer: startModernizedDismissTimer()
DismissTimer->>modernizedPanelState: delay expires → 'dismissing'
Note over modernizedPanelState: slideOut animation plays
ContentUploader->>modernizedPanelState: animationend (slideOut) → finalizeModernizedDismiss() → 'hidden'
ContentUploader->>ContentUploader: Reset uploads queue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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-uploader/ContentUploader.tsx`:
- Around line 286-287: The componentDidUpdate method is calling
startModernizedDismissTimer() on every update while modernizedPanelState remains
'shown', which continuously restarts the dismiss timeout. Instead, only call
startModernizedDismissTimer() when modernizedPanelState transitions TO 'shown'
from a different state. Track the previous modernizedPanelState value in
componentDidUpdate and compare it with the current modernizedPanelState to
detect the actual state transition, ensuring the timer is only reset when
entering the 'shown' state rather than on every update.
In `@src/elements/content-uploader/ModernizedUploadsManagerPanel.scss`:
- Around line 1-44: Rename the keyframe animations in the SCSS file to use fully
kebab-case naming: change bcu-modernized-slideIn to bcu-modernized-slide-in and
bcu-modernized-slideOut to bcu-modernized-slide-out in all animation references
within the .bcu-modernized-panel class. Then update the SLIDE_OUT_ANIMATION_NAME
constant and its usage in the handleModernizedAnimationEnd function in the
corresponding TypeScript file to reference the renamed bcu-modernized-slide-out
keyframe to ensure the animation end event handling continues to work correctly.
🪄 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: 8d5492a4-d2b1-4b6c-a9a0-dfd5c4482668
📒 Files selected for processing (4)
src/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/ModernizedUploadsManagerPanel.scsssrc/elements/content-uploader/__tests__/ContentUploader.test.jssrc/elements/content-uploader/stories/ContentUploader.stories.js
dealwith
left a comment
There was a problem hiding this comment.
Looks good, didn't find blocking issues 👍
86959e8 to
fa35eec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/elements/content-uploader/__tests__/ContentUploader.test.js (1)
1445-1699: ⚡ Quick winAdd test coverage for STATUS_CANCELED items in the dismiss timer lifecycle.
The implementation treats
STATUS_CANCELEDitems as complete when determining whether to start the dismiss timer (as shown in the upstreamisUploadsBatchSuccessfullyCompletecheck). The test suite verifiesSTATUS_COMPLETE,STATUS_IN_PROGRESS, andSTATUS_ERRORbut doesn't explicitly test that a batch with onlySTATUS_CANCELEDitems triggers the dismiss timer.Consider adding a test case to verify this behavior.
🧪 Suggested test case
test('starts dismiss timer when all items are STATUS_CANCELED', () => { const wrapper = getWrapper({ enableModernizedUploads: true }); const instance = wrapper.instance(); wrapper.setState({ modernizedPanelState: 'shown', items: [makeItem('a.pdf', STATUS_CANCELED), makeItem('b.pdf', STATUS_CANCELED)], }); expect(instance.modernizedDismissTimer).not.toBeNull(); jest.advanceTimersByTime(HIDE_DELAY_MS); expect(wrapper.state('modernizedPanelState')).toBe('dismissing'); });🤖 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-uploader/__tests__/ContentUploader.test.js` around lines 1445 - 1699, Add a new test case within the 'modernized panel open/close lifecycle' describe block to verify that the dismiss timer is started when all items transition to STATUS_CANCELED status. The test should create a batch with only STATUS_CANCELED items (following the pattern of the existing test cases like 'starts dismiss timer when batch transitions to all-complete and panel is shown'), verify that the modernizedDismissTimer is set, advance the timer by HIDE_DELAY_MS, and then assert that the modernizedPanelState transitions to 'dismissing'. This ensures STATUS_CANCELED items are properly treated as complete items for the dismiss timer logic.
🤖 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-uploader/__tests__/ContentUploader.test.js`:
- Line 1614: The test file is using a hardcoded string 'bcu-modernized-slideOut'
in the handleModernizedAnimationEnd call instead of referencing the
implementation's constant. Import the SLIDE_OUT_ANIMATION_NAME constant from the
implementation module and replace the hardcoded string with this constant in the
test assertion to ensure the test remains synchronized if the constant value
changes in the future.
- Line 1446: The test file defines a local constant HIDE_DELAY_MS = 8000 instead
of importing the actual constant from the implementation. This creates a drift
risk where the test could pass even if the implementation constant
HIDE_UPLOAD_MANAGER_DELAY_MS_DEFAULT changes. Import
HIDE_UPLOAD_MANAGER_DELAY_MS_DEFAULT from the ContentUploader implementation
file at the top of the test file, then replace all uses of the locally defined
HIDE_DELAY_MS with the imported HIDE_UPLOAD_MANAGER_DELAY_MS_DEFAULT constant to
keep the test synchronized with the production code.
---
Nitpick comments:
In `@src/elements/content-uploader/__tests__/ContentUploader.test.js`:
- Around line 1445-1699: Add a new test case within the 'modernized panel
open/close lifecycle' describe block to verify that the dismiss timer is started
when all items transition to STATUS_CANCELED status. The test should create a
batch with only STATUS_CANCELED items (following the pattern of the existing
test cases like 'starts dismiss timer when batch transitions to all-complete and
panel is shown'), verify that the modernizedDismissTimer is set, advance the
timer by HIDE_DELAY_MS, and then assert that the modernizedPanelState
transitions to 'dismissing'. This ensures STATUS_CANCELED items are properly
treated as complete items for the dismiss timer logic.
🪄 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: a3a27d40-a1c6-4ffa-8be8-8c40477da90a
📒 Files selected for processing (4)
src/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/ModernizedUploadsManagerPanel.scsssrc/elements/content-uploader/__tests__/ContentUploader.test.jssrc/elements/content-uploader/stories/ContentUploader.stories.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-uploader/stories/ContentUploader.stories.js
- src/elements/content-uploader/ContentUploader.tsx
Merge Queue Status
This pull request spent 10 seconds in the queue, including 1 second running CI. Required conditions to merge
|
…ox#4628) * feat: implement open/closing logic for modernized content uploader * test: update storybook * fix: avoid starting timer on every re-render
Summary
Summary by CodeRabbit