perf: remove upload, playback, and page waterfalls - #67
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe pull request updates playback recovery, concurrent uploads, dashboard presence and comment data handling, route-level Clerk/Convex providers, resource hints, variable fonts, navigation behavior, shared UI, and related tests and dependencies. ChangesPlayback recovery
Upload pipeline
Dashboard data and presence
Application infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/dashboard/-useVideoUploadManager.ts (1)
391-405: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep “Dismiss” non-destructive for retryable processing failures.
When
canRetryProcessingis true, the upload has completed and its object was preserved for retry. This handler currently callsabortVideoUpload, deleting that object even though the error-state button is labeled “Dismiss”. Remove only the local item in this case, or expose a separately labeled destructive action.Proposed fix
const cancelUpload = useCallback( (uploadId: string) => { const upload = uploadsRef.current.find((item) => item.id === uploadId); + if (upload?.canRetryProcessing) { + updateUploads((prev) => prev.filter((item) => item.id !== uploadId)); + return; + } + if (upload?.abortController) { upload.abortController.abort(); }🤖 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 `@app/routes/dashboard/-useVideoUploadManager.ts` around lines 391 - 405, Update cancelUpload so that when an upload has canRetryProcessing enabled, it only removes the local upload entry and does not call abortVideoUpload; retain the existing destructive abort behavior for uploads that are not retryable processing failures.
🧹 Nitpick comments (1)
app/routes/dashboard/-video.tsx (1)
1301-1363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the playback recovery design consistently.
The new recovery surfaces bypass the required palette and primary-UI treatment.
app/routes/dashboard/-video.tsx#L1301-L1363: replace dark brown banners with cream, near-black text, and 2px borders.app/routes/-watch.tsx#L397-L411: apply the same compliant status-banner treatment.app/routes/-share.tsx#L439-L483: update the banner and remove the fallback’s rounded corners and shadow.As per coding guidelines, primary UI uses
#f0f0e8,#1a1a1a, strong 2px borders, square edges, and no shadows.🤖 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 `@app/routes/dashboard/-video.tsx` around lines 1301 - 1363, Apply the compliant primary-UI treatment to the recovery banners in app/routes/dashboard/-video.tsx lines 1301-1363, app/routes/-watch.tsx lines 397-411, and app/routes/-share.tsx lines 439-483: use `#f0f0e8` backgrounds, `#1a1a1a` text, strong 2px borders, square edges, and no shadows. In the share fallback, specifically remove rounded corners and shadow while preserving its existing recovery behavior.Source: Coding guidelines
🤖 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 `@app/app.css`:
- Line 125: Update the --font-serif custom property in app.css to use lowercase
georgia, and make the same casing change for its repeated occurrence. Preserve
the existing serif fallback.
In `@app/routes/-share.tsx`:
- Around line 159-167: The automatic grant flow in the useEffect must recover
when acquireGrant resolves unsuccessfully instead of permanently relying on
hasAttemptedAutoGrant. Track the access error and expose a retry action or
bounded backoff that reattempts acquireGrant, keeping the user in the grant flow
rather than showing “Video not available” after transient or rate-limit
failures.
In `@app/routes/dashboard/-layout.tsx`:
- Around line 293-305: Update the fixed upload list container rendered when
uploads.length > 0 to constrain its height to the viewport and enable vertical
scrolling, while preserving its existing positioning, spacing, and responsive
width classes. Ensure all DashboardUploadProgressItem controls remain reachable
when many uploads are present.
In `@app/routes/dashboard/-project.tsx`:
- Around line 148-180: Move the assignments to scopeKeyRef.current and
recomputeSelectionRef.current out of the render body and into an effect that
runs after commit. Keep recomputeSelection using the refs for the latest
committed scope and callback, ensuring discarded concurrent renders cannot
affect pending animation-frame work.
---
Outside diff comments:
In `@app/routes/dashboard/-useVideoUploadManager.ts`:
- Around line 391-405: Update cancelUpload so that when an upload has
canRetryProcessing enabled, it only removes the local upload entry and does not
call abortVideoUpload; retain the existing destructive abort behavior for
uploads that are not retryable processing failures.
---
Nitpick comments:
In `@app/routes/dashboard/-video.tsx`:
- Around line 1301-1363: Apply the compliant primary-UI treatment to the
recovery banners in app/routes/dashboard/-video.tsx lines 1301-1363,
app/routes/-watch.tsx lines 397-411, and app/routes/-share.tsx lines 439-483:
use `#f0f0e8` backgrounds, `#1a1a1a` text, strong 2px borders, square edges, and no
shadows. In the share fallback, specifically remove rounded corners and shadow
while preserving its existing recovery behavior.
🪄 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: a24c2167-3bf4-4e91-a6e0-fc2062e404f6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
app/app.cssapp/routes/-home.tsxapp/routes/-share.tsxapp/routes/-watch.tsxapp/routes/__root.tsxapp/routes/dashboard/$teamSlug.$projectId.tsxapp/routes/dashboard/-layout.tsxapp/routes/dashboard/-project.data.tsapp/routes/dashboard/-project.tsxapp/routes/dashboard/-routeDataContracts.test.tsapp/routes/dashboard/-useVideoUploadManager.tsapp/routes/dashboard/-video.data.tsapp/routes/dashboard/-video.tsxapp/routes/dashboard/route.tsxapp/routes/index.tsxapp/routes/invite.$token.tsxapp/routes/mono.tsxapp/routes/share.$token.tsxapp/routes/sign-in.$.tsxapp/routes/sign-in.tsxapp/routes/sign-up.$.tsxapp/routes/sign-up.tsxapp/routes/watch.$publicId.tsxconvex/comments.tsconvex/videoActions.tsconvex/videoPresence.tsconvex/videoPresence.vitest.tspackage.jsonsrc/components/MarketingNav.tsxsrc/components/comments/CommentList.tsxsrc/components/ui/tooltip.tsxsrc/components/upload/UploadProgress.tsxsrc/components/video-player/VideoPlayer.tsxsrc/lib/clerk.tsxsrc/lib/convex.tsxsrc/lib/dashboardPlaybackSource.test.tssrc/lib/dashboardPlaybackSource.tssrc/lib/dashboardUploadContext.tsxsrc/lib/muxPlayback.test.tssrc/lib/muxPlayback.tssrc/lib/muxPlaybackServerContract.test.tssrc/lib/seo.tssrc/lib/videoUpload.test.tssrc/lib/videoUpload.ts
💤 Files with no reviewable changes (2)
- src/components/ui/tooltip.tsx
- src/lib/dashboardUploadContext.tsx
| --font-serif: "Instrument Serif", Georgia, serif; | ||
| --font-sans: "Geist Variable", system-ui, -apple-system, sans-serif; | ||
| --font-mono: "Geist Mono Variable", ui-monospace, monospace; | ||
| --font-serif: Georgia, serif; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use lowercase georgia to satisfy Stylelint.
Line 125 fails value-keyword-case; line 299 repeats the same value.
Proposed fix
- --font-serif: Georgia, serif;
+ --font-serif: georgia, serif;
...
- font-family: Georgia, serif;
+ font-family: georgia, serif;Also applies to: 299-299
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 125-125: Expected "Georgia" to be "georgia" (value-keyword-case)
(value-keyword-case)
🤖 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 `@app/app.css` at line 125, Update the --font-serif custom property in app.css
to use lowercase georgia, and make the same casing change for its repeated
occurrence. Preserve the existing serif fallback.
Source: Linters/SAST tools
listProjectOnlineCounts threw FORBIDDEN when any subscribed video was deleted or moved out of the project, which Convex useQuery rethrows in render — one teammate deleting a video crashed every open dashboard for that project to the root error page. Skip those videos instead (the client already tolerates missing count entries), and make videoIds optional so clients deployed before the argument existed keep a live subscription through the rollout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
font-display: swap; remove global Mux hints, remote font waterfalls, Video.js, and unused tooltip/dependency codeValidation
bun run formatbun run checkgit diff --checkclaude -pperformance audit plus three clean independent final review passesPer repository guidance, I did not run a dev server or production build.
Follow-up
Materialized comment-count and storage-accounting counters still deserve dedicated migrations/backfills. They are intentionally not mixed into this PR because storage accounting is billing-sensitive and both changes need deployment reconciliation, not compatibility guesses.
Note
Remove upload, playback, and page waterfalls by scoping providers per route and adding recovery-aware playback
sourceRevision, reports fatal HLS errors viaonPlaybackIssue, and triggers structured retry/recovery flows with user-visible status banners.prefetchHlsRuntimeandselectMuxPlaybackSourceabstractions; Mux HLS URLs now cap atmax_resolution=720p(previouslymin_resolution=720p).threadCommentshelper, removing a server-sidecomments.getThreadedprewarm query.useProjectPresenceViewporthook and a boundedvideoPresence.listProjectOnlineCountsquery.createAsyncTaskQueuewith concurrency 2; progress events are coalesced per animation frame viacreateFrameCoalescedPublisher.video.js,@radix-ui/react-tooltip,@stripe/stripe-js, and@tanstack/react-router-devtoolsfrom the dependency tree; switches fonts to local@fontsource-variablepackages.Macroscope summarized 8d641f9.
Summary by CodeRabbit