Repository navigation
perf: wake Loom imports from the worker webhook instead of polling - #3119
Closed
richiemcilroy wants to merge 1 commit into
Closed
richiemcilroy wants to merge 1 commit into
richiemcilroy wants to merge 1 commit into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Member
Author
Comment on lines
+79
to
+81
| vi.mock("@/lib/loom-import-signal", () => ({ | ||
| signalLoomImportFinished: mocks.signal, | ||
| })); |
Contributor
There was a problem hiding this comment.
Signal checks lack direct tests
These tests replace signalLoomImportFinished entirely, so broken token checks or error branches would still pass. Add focused tests that mock resumeHook instead: missing and wrong-video tokens must not resume anything, matching tokens must pass the phase, and missing hooks and other errors must not escape. This would cover the promised behavior for stale worker callbacks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/__tests__/unit/media-server-progress.test.ts
Line: 79-81
Comment:
**Signal checks lack direct tests**
These tests replace `signalLoomImportFinished` entirely, so broken token checks or error branches would still pass. Add focused tests that mock `resumeHook` instead: missing and wrong-video tokens must not resume anything, matching tokens must pass the phase, and missing hooks and other errors must not escape. This would cover the promised behavior for stale worker callbacks.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch was successfully deployed
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.
Loom import workflows used to poll the database every 5 to 30 seconds until their media job finished. Each poll is a workflow step plus a replay on Vercel, so a ten minute import cost about 25 invocations, and a batch of 1,200 imports cost tens of thousands.
Now each processing attempt creates a workflow hook whose token goes into the worker's webhook URL. When the worker reports complete, error or cancelled, the progress route saves the state first and then resumes the hook. The workflow then reads the status once. A typical import now uses two status reads instead of dozens.
Safety nets:
Recording, instant and web uploads still use
waitForVideoProcessingand are unchanged.Stacked on #3118.
Tests: video-processing-status 15, media-server-progress 59, media-processing-workflows 27 pass. The 7 share-stage-identity failures also fail on main.
The PR appears safe to merge, with a non-blocking gap in tests for the new signal helper.
Findings
Fix with agent prompt
Summary
Loom imports now wait for worker callbacks instead of frequent database checks.
Reviews (1) · Last reviewed commit: "perf: wake Loom import workflows from th..." · Reviewed by Greptile