fix: media uploads grid spinner#848
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughField rendering now uses the current summit ID as a fallback when building media-upload material links; a test was added to assert the link opens in a new tab using that fallback. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
Fixes navigation to media-upload “materials” pages by ensuring the summit ID used in the URL is always available (falling back to the currently selected summit when upload records don’t include summit_id).
Changes:
- Extend
fieldNamesto acceptcurrentSummitIdand use it as a fallback when building media upload materials links. - Add a unit test covering the new fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/pages/events/summit-event-list-page.js | Use currentSummit.id as a fallback when media_upload.summit_id (and row.summit_id) is missing so materials links open reliably. |
| src/pages/events/tests/summit-event-list-page.test.js | Adds coverage to assert the materials link uses the current summit ID fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const summitId = | ||
| m?.summit_id || row?.summit_id || currentSummitId; | ||
| if (!row?.id || !summitId) return false; | ||
| window.open( | ||
| `/app/summits/${m.summit_id}/events/${row.id}/materials/${m.id}`, | ||
| `/app/summits/${summitId}/events/${row.id}/materials/${m.id}`, | ||
| "_blank" |
There was a problem hiding this comment.
PR title mentions a “grid spinner”, but the actual change here is about constructing the media-upload materials URL with a summit-id fallback. Consider updating the PR title (or adding a note in the description) so the intent matches the code change and is searchable in history/release notes.
santipalenque
left a comment
There was a problem hiding this comment.
just a small request, overall looks good
| onClick={(ev) => { | ||
| ev.preventDefault(); | ||
| if (!row?.id) return false; | ||
| const summitId = |
There was a problem hiding this comment.
currentSummitId should be enough since there is no way a user can land here without one, lets just use that and forget about m.summit_id and row.summit_id
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@priscila-moneo please see comments
| const summitId = | ||
| m?.summit_id || row?.summit_id || currentSummitId; | ||
| if (!row?.id || !summitId) return false; | ||
| if (!row?.id) return false; |
There was a problem hiding this comment.
if (!row?.id || !currentSummitId) return false;
If the page renders before currentSummit is loaded, this can open /app/summits/undefined/events/.../materials/...
ref: https://app.clickup.com/t/86b90w2rq
Summary by CodeRabbit
Bug Fixes
Tests