-
Notifications
You must be signed in to change notification settings - Fork 295
Yield while parsing data so that the UI isn't blocked while loading larger shapes #3540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
KyleAMathews
wants to merge
5
commits into
main
Choose a base branch
from
claude/fix-parsing-ui-blocking-01BkCaSq8fSJ1wYbum3XFKV6
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
208afb4
fix(typescript-client): prevent UI blocking during parsing of large s…
claude 98cf8c1
fix(typescript-client): fix flaky yield test and prettier formatting
claude b3d1924
fix(typescript-client): remove unused variable in yield test
claude 89b4ed2
fix(typescript-client): address review feedback
claude 06b0482
refactor(typescript-client): remove yieldEvery configurability
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,3 +16,4 @@ export { | |
| snakeToCamel, | ||
| camelToSnake, | ||
| } from './column-mapper' | ||
| export { yieldToMain } from './yield' | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /** | ||
| * Utility for yielding control back to the main thread to prevent UI blocking. | ||
| * | ||
| * This is particularly useful when processing large amounts of data (like parsing | ||
| * large shapes) to ensure the UI remains responsive. | ||
| */ | ||
|
|
||
| /** | ||
| * Default number of items to process before yielding to the main thread. | ||
| * This value balances responsiveness with overhead from yielding. | ||
| */ | ||
| export const DEFAULT_YIELD_EVERY = 1000 | ||
|
|
||
| /** | ||
| * Yields control back to the main thread, allowing the browser to handle | ||
| * user interactions, rendering, and other tasks. | ||
| * | ||
| * Uses `scheduler.yield()` if available (Chrome 129+, behind flag in other browsers), | ||
| * otherwise falls back to `setTimeout(0)`. | ||
| * | ||
| * @returns A promise that resolves after yielding to the main thread | ||
| */ | ||
| export function yieldToMain(): Promise<void> { | ||
| // Check if scheduler.yield is available (modern browsers) | ||
| // Guard typeof globalThis first to avoid reference errors in exotic environments | ||
| if (typeof globalThis !== `undefined`) { | ||
| const g = globalThis as GlobalWithScheduler | ||
| if (g.scheduler && typeof g.scheduler.yield === `function`) { | ||
| return g.scheduler.yield() | ||
| } | ||
| } | ||
|
|
||
| // Fallback to setTimeout(0) which yields to the event loop | ||
| return new Promise((resolve) => setTimeout(resolve, 0)) | ||
| } | ||
|
|
||
| // Type definitions for the Scheduler API (available in some modern browsers) | ||
| interface GlobalWithScheduler { | ||
| scheduler?: { | ||
| yield: () => Promise<void> | ||
| } | ||
| } |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { describe, expect, it, vi, afterEach } from 'vitest' | ||
| import { yieldToMain } from '../src/yield' | ||
|
|
||
| describe(`yieldToMain`, () => { | ||
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| it(`should return a promise that resolves`, async () => { | ||
| const result = yieldToMain() | ||
| expect(result).toBeInstanceOf(Promise) | ||
| await expect(result).resolves.toBeUndefined() | ||
| }) | ||
|
|
||
| it(`should use scheduler.yield when available`, async () => { | ||
| const mockYield = vi.fn().mockResolvedValue(undefined) | ||
| type GlobalWithScheduler = { scheduler?: { yield?: () => Promise<void> } } | ||
| const g = globalThis as unknown as GlobalWithScheduler | ||
| const originalScheduler = g.scheduler | ||
|
|
||
| // Mock scheduler.yield | ||
| g.scheduler = { yield: mockYield } | ||
|
|
||
| await yieldToMain() | ||
| expect(mockYield).toHaveBeenCalled() | ||
|
|
||
| // Restore | ||
| if (originalScheduler) { | ||
| g.scheduler = originalScheduler | ||
| } else { | ||
| delete g.scheduler | ||
| } | ||
| }) | ||
|
|
||
| it(`should fall back to setTimeout when scheduler.yield is not available`, async () => { | ||
| type GlobalWithScheduler = { scheduler?: { yield?: () => Promise<void> } } | ||
| const g = globalThis as unknown as GlobalWithScheduler | ||
| const originalScheduler = g.scheduler | ||
|
|
||
| // Remove scheduler | ||
| delete g.scheduler | ||
|
|
||
| // Track that setTimeout is used | ||
| const setTimeoutSpy = vi.spyOn(globalThis, `setTimeout`) | ||
|
|
||
| await yieldToMain() | ||
|
|
||
| expect(setTimeoutSpy).toHaveBeenCalledWith(expect.any(Function), 0) | ||
|
|
||
| // Restore | ||
| if (originalScheduler) { | ||
| g.scheduler = originalScheduler | ||
| } | ||
| setTimeoutSpy.mockRestore() | ||
| }) | ||
|
|
||
| it(`should yield control to allow other tasks to run`, async () => { | ||
| // This test verifies that yieldToMain actually yields - we can't | ||
| // guarantee exact ordering of setTimeout callbacks, but we can verify | ||
| // that other tasks get a chance to run during the yield | ||
| const taskRan = { value: false } | ||
|
|
||
| const yieldingTask = (async () => { | ||
| await yieldToMain() | ||
| return taskRan.value | ||
| })() | ||
|
|
||
| // Queue a task that should have a chance to run during the yield | ||
| setTimeout(() => { | ||
| taskRan.value = true | ||
| }, 0) | ||
|
|
||
| // Wait for both the yield and the setTimeout | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| await yieldingTask | ||
|
|
||
| // The setTimeout should have had a chance to run | ||
| expect(taskRan.value).toBe(true) | ||
| }) | ||
| }) |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i'd prefer if
parseAsyncandparseSnapshotDataAsyncwould take the yield every configuration as an argument instead of relying on a global. We can still have a global configuration but i would pass that global as an argument when calling these functions.