perf(web): remove duplicate video reads and run dashboard queries concurrently - #2216
Merged
Merged
Conversation
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
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.
Follow-up to #2213. Insights showed that after the index pass the remaining load is request shape, not the schema:
select * from videos where id = ?runs ~770k times a day, and the two most-visited authenticated pages issue their queries one after another. This PR removes the duplicate reads and serialises less, with results and error ordering kept identical.1. Policy checks reuse the row that was already loaded
Policy.withPolicy/withPublicPolicyrun the policy before the wrapped effect, and bothVideosPolicy.canViewandisOwnerload the video row that the effect then loads again.VideosPolicynow exposescanViewLoaded,isOwnerLoaded,getViewableByIdandgetOwnedById, which fetch once and decide on the loaded row.buildCanViewkeeps its behaviour; the shared decision logic is factored into one place.Videos.getByIdForViewing(share page metadata, preview, playlist),delete,duplicate,getDownloadInfo,getThumbnailURL,updateUploadProgress, and the five multipart upload handlers (presign-part is 44k requests/day). Outcomes are unchanged in every case: missing video still yields not-found, non-owner still yields 403, view rules and password prompts are evaluated on the same inputs as before.canViewwas anEffect.allwith no concurrency option, which runs sequentially; it now runs both queries concurrently.verifyPasswordCandidatesonly readsvideo.id, so its parameter is nowPick<Video, "id">.2. Share page
canViewpolicy (with its own video read and sub-queries) and then a joined select of the same video. The joined select now runs first andcanViewLoadeddecides on that row. The tombstone filter moved from theWHEREto a check after the policy so a denied or password-gated video on a deleted org still resolves exactly as before.passwordandownerIdare read for the check and stripped before the row leaves the loader.getDashboardData(user)was called only to read.spacesDatafor the share dialog, which pulled the whole dashboard dataset including a 5-queries-per-organization loop. It now callsgetDashboardSpacesData, built from the same helpers, which needs the org lookup, the role lookup, the spaces query and the two org counts.usersrow was re-fetched for the AI entitlement check even though the joined select already returns it in full.3. Dashboard
getDashboardDatais split into helpers shared with the share page. Independent queries now run concurrently: invites alongside the active-org work; role and notification lookup together; spaces, the "All org" entry counts and the user cap count together; and inside the per-organization loop the members, managers and owned-org reads run together, then the two count queries. The org settings row is taken from the organization row already in memory instead of a third query, anduserPreferencescomes from theuserrow that was passed in.Deliberately not included
getCurrentUserper request: 17 files update the user row and read it again inside the same request, so that needs its own audit before it is safe.Validation
pnpm typecheck(next typegen +tsc -b) clean.videos-policy.test.tsextended with a parity table assertingcanViewLoadedreturns the same decision ascanViewacross owner, anonymous, private, org and space membership, video and inherited passwords, and email-restriction scenarios, plus a test that the loaded variant never callsrepo.getById. 52/52 pass.slack-app-manifest.test.ts, compares the checked-inslack-app-manifest.jsonagainst fixed expectations and touches nothing in this change; it is pre-existing.Greptile Summary
This PR reduces redundant video and dashboard database reads while preserving existing authorization and response behavior.
Confidence Score: 5/5
The PR appears safe to merge, with no concrete changed-code regression identified.
The loaded-row policy paths preserve authorization and error outcomes, while the dashboard and caps concurrency changes retain required query dependencies and response shapes.
Important Files Changed
Reviews (1): Last reviewed commit: "perf(share): load the share page with on..." | Re-trigger Greptile