fix: preserve loom csv retry quota - #2028
Merged
richiemcilroy merged 1 commit intoJul 24, 2026
Merged
Conversation
already imported rows spent the per-user budget before the duplicate check, so retries could make no progress on CapSoftware#2010.
|
|
||
| it("skips rate limit checks for csv rows that were already imported", async () => { | ||
| whereMock.mockImplementation((conditions: unknown) => { | ||
| const serializedConditions = JSON.stringify(conditions); |
There was a problem hiding this comment.
JSON.stringify(conditions) feels pretty brittle here (and can throw if the underlying Drizzle condition ever gains a circular ref). Since the where() call order is deterministic in this test, stubbing the 4 calls explicitly should be more robust.
Suggested change
| const serializedConditions = JSON.stringify(conditions); | |
| whereMock | |
| .mockReturnValueOnce( | |
| withLimit([{ userId: "member-123", email: "member@example.com" }]), | |
| ) | |
| .mockResolvedValueOnce([{ videoId: "existing-video" }]) | |
| .mockReturnValueOnce( | |
| withLimit([{ userId: "member-123", email: "member@example.com" }]), | |
| ) | |
| .mockResolvedValueOnce([]); |
| error: undefined, | ||
| }); | ||
| expect(checkRateLimitMock).toHaveBeenCalledTimes(1); | ||
| expect(fetchMock).toHaveBeenCalled(); |
There was a problem hiding this comment.
Minor: expect(fetchMock).toHaveBeenCalled() doesn’t really assert that the duplicate row avoided doing Loom network work. Might be worth tightening this to ensure no calls include loom-existing123.
Suggested change
| expect(fetchMock).toHaveBeenCalled(); | |
| expect(fetchMock).toHaveBeenCalled(); | |
| expect( | |
| fetchMock.mock.calls.some(([input]) => { | |
| const url = typeof input === "string" ? input : input.toString(); | |
| return url.includes("loom-existing123"); | |
| }), | |
| ).toBe(false); |
Member
|
thanks for your contribution! |
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.
what changed
csv retries now run the existing loom duplicate lookup before charging a rate limit token. rows with an existing cap return the same duplicate result, while new and stale rows still go through the limiter.
the limiter moved into
importLoomVideoForOwnerfor the csv path. this keeps the lookup and stale row handling in one place and avoids another database query.the regression test covers a duplicate row followed by a new row and checks that only the new import spends quota.
why
a retry of a partially imported csv spent its full budget on rows that had already succeeded. large imports could never reach the first unfinished row unless someone edited completed rows out of the csv.
closes #2010
testing
pnpm --filter @cap/web testpnpm --dir apps/web exec next typegenpnpm exec tsc -b apps/web/tsconfig.json --pretty falsepnpm exec biome check apps/web/actions/loom.ts apps/web/__tests__/unit/loom-import.test.tsGreptile Summary
This PR preserves Loom CSV retry quota by moving per-row rate-limit checks behind the existing duplicate lookup.
Confidence Score: 5/5
The PR appears safe to merge, with duplicate retries bypassing quota consumption while importable rows remain rate-limited.
The changed ordering preserves existing duplicate and stale-record behavior, keeps the single-video path unchanged, and applies the CSV limiter before any new import begins.
Important Files Changed
Reviews (1): Last reviewed commit: "fix: preserve loom csv retry quota" | Re-trigger Greptile