feat: use yielding to main thread in more places#2585
Closed
pauldambra wants to merge 2 commits intomainfrom
Closed
feat: use yielding to main thread in more places#2585pauldambra wants to merge 2 commits intomainfrom
pauldambra wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Size Change: +26 kB (+0.49%) Total Size: 5.33 MB
ℹ️ View Unchanged
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new TaskQueue utility to improve page responsiveness by breaking up CPU-intensive operations and yielding to the main thread periodically. This helps reduce Interaction to Next Paint (INP) variability on pages with heavy processing.
- Adds a reusable
TaskQueueclass with time-budgeting (default 30ms) to avoid blocking the main thread - Implements helper functions
processWithYieldandprocessAsyncWithYieldfor common array processing patterns - Integrates the task queue into request queue flushing, extension initialization, performance entry processing, and session recording event handling
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/browser/src/utils/task-queue.ts | New task queue implementation with time-slicing support and helper functions for processing arrays with yielding |
| packages/browser/src/utils/tests/task-queue.test.ts | Comprehensive test suite covering task queue functionality, error handling, and yielding behavior |
| packages/browser/src/request-queue.ts | Updates flush callback to use processWithYield for processing request batches |
| packages/browser/src/posthog-core.ts | Refactors extension initialization to use TaskQueue class, removing custom time-slicing logic |
| packages/browser/src/extensions/replay/external/network-plugin.ts | Applies yielding to initial performance entry processing |
| packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts | Adds yielding to queued event processing |
| .changeset/smooth-wolves-mix.md | Changeset documenting the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
pauldambra
force-pushed
the
feat/more-defer
branch
from
5b464a3 to
dcd9d73
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
dcd9d73 to
e1fa634
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pauldambra
force-pushed
the
feat/more-defer
branch
from
e1fa634 to
6e5b1cb
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
6e5b1cb to
d0eb1ea
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
d0eb1ea to
48f90cd
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
48f90cd to
17209ef
Compare
rafaeelaudibert
approved these changes
Nov 19, 2025
Member
Author
|
Local INP testing Results Summary
|
pauldambra
force-pushed
the
feat/more-defer
branch
from
e8c0f0a to
470810c
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
470810c to
557fbbc
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
557fbbc to
dce124e
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
dce124e to
eea8cc5
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
eea8cc5 to
b72e302
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
b72e302 to
4825227
Compare
rafaeelaudibert
approved these changes
Dec 18, 2025
| supportedCompression: ['gzip'], | ||
| } as RemoteConfig | ||
| supportedCompression: [Compression.GZipJS], | ||
| } as Partial<RemoteConfig> as RemoteConfig |
Member
There was a problem hiding this comment.
Weird casting? Really needed?
| for (const key in requests) { | ||
| const req = requests[key] | ||
| const now = new Date().getTime() | ||
| const requestEntries = Object.entries(requests) |
Member
There was a problem hiding this comment.
You only use the values, so Object.values is all you need here
| logger.info(`Processing ${this._bufferedInvocations.length} events for site app with id ${loader.id}`) | ||
| this._bufferedInvocations.forEach((globals) => app.processEvent?.(globals)) | ||
| app.processedBuffer = true | ||
| scheduler.processEach(this._bufferedInvocations, (globals) => app.processEvent?.(globals), { |
Member
There was a problem hiding this comment.
You reacted with a +1 to my comment but didnt really explain whether keeping this duplicated was intended or not
|
|
||
| type IdleCallbackHandle = number | ReturnType<typeof setTimeout> | ||
|
|
||
| export const _requestIdleCallback = ( |
Member
There was a problem hiding this comment.
A comment explaining the fallback and difference between requestIdleCallback and setTimeout wouldnt hurt
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Contributor
Greptile SummaryThis PR introduces a new Key Changes
Critical Issues Found
This affects both Other Concerns
RecommendationThe PR implements a good architectural pattern for main thread yielding, but the critical data loss bug in request/retry queues must be fixed before merge. These queues should either:
|
Comment on lines
+68
to
+80
| scheduler.processEach( | ||
| requestEntries, | ||
| ([, req]) => { | ||
| if (req.data && isArray(req.data)) { | ||
| each(req.data, (data) => { | ||
| data['offset'] = Math.abs(data['timestamp'] - flushStartTime) | ||
| delete data['timestamp'] | ||
| }) | ||
| } | ||
| this._sendRequest(req) | ||
| }, | ||
| { priority: 'high' } | ||
| ) |
Contributor
There was a problem hiding this comment.
Critical data loss bug: Using scheduler.processEach here creates a race condition with the unload() method that can result in event loss.
The problem:
- When this timeout fires,
_formatQueue()is called (line 64), which clearsthis._queue(line 103 in _formatQueue) - The formatted requests are then passed to
scheduler.processEachto be sent asynchronously - The scheduler yields every 30ms to avoid blocking the main thread (scheduler.ts line 87)
- If the user navigates away/closes the page during this yielding period:
_handle_unload()is called (posthog-core.ts line 870)- Which calls
this._requestQueue?.unload()(posthog-core.ts line 884) unload()checksthis._queue.length > 0(line 39)- But
this._queueis already empty (cleared in step 1) - So it doesn't send the events that are still queued in the scheduler
- Those events are lost forever
The fix:
The scheduler should not be used for request flushing that needs to complete before page unload. Either:
- Process requests synchronously in the flush (original behavior)
- Or track pending scheduler work separately so
unload()can flush it - Or don't use the scheduler when
priority: 'high'means "must complete before unload"
This is particularly critical because the comment in scheduler.ts line 65 explicitly mentions "flushing events on page unload" as a use case for high priority.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/request-queue.ts
Line: 68:80
Comment:
Critical data loss bug: Using `scheduler.processEach` here creates a race condition with the `unload()` method that can result in event loss.
**The problem:**
1. When this timeout fires, `_formatQueue()` is called (line 64), which clears `this._queue` (line 103 in _formatQueue)
2. The formatted requests are then passed to `scheduler.processEach` to be sent asynchronously
3. The scheduler yields every 30ms to avoid blocking the main thread (scheduler.ts line 87)
4. If the user navigates away/closes the page during this yielding period:
- `_handle_unload()` is called (posthog-core.ts line 870)
- Which calls `this._requestQueue?.unload()` (posthog-core.ts line 884)
- `unload()` checks `this._queue.length > 0` (line 39)
- But `this._queue` is already empty (cleared in step 1)
- So it doesn't send the events that are still queued in the scheduler
- **Those events are lost forever**
**The fix:**
The scheduler should not be used for request flushing that needs to complete before page unload. Either:
1. Process requests synchronously in the flush (original behavior)
2. Or track pending scheduler work separately so `unload()` can flush it
3. Or don't use the scheduler when `priority: 'high'` means "must complete before unload"
This is particularly critical because the comment in scheduler.ts line 65 explicitly mentions "flushing events on page unload" as a use case for high priority.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+146
to
+148
| scheduler.processEach(toFlush, ({ requestOptions }) => this.retriableRequest(requestOptions), { | ||
| priority: 'high', | ||
| }) |
Contributor
There was a problem hiding this comment.
Same critical data loss bug as in request-queue.ts: Using scheduler.processEach here with asynchronous processing means that if the page unloads while the scheduler is processing retries, those retry attempts will be abandoned.
When unload() is called (line 152), it iterates through this._queue and sends requests via sendBeacon. However, if a flush was in progress via the scheduler:
- The retry items were removed from
this._queue(line 143:this._queue = notToFlush) - They're sitting in the scheduler's queue waiting to be processed
- The scheduler won't complete before the page unloads
- Those retry attempts are lost
The original synchronous for loop ensured all retries were at least attempted before moving on. The scheduler breaks this guarantee.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/retry-queue.ts
Line: 146:148
Comment:
Same critical data loss bug as in request-queue.ts: Using `scheduler.processEach` here with asynchronous processing means that if the page unloads while the scheduler is processing retries, those retry attempts will be abandoned.
When `unload()` is called (line 152), it iterates through `this._queue` and sends requests via sendBeacon. However, if a flush was in progress via the scheduler:
- The retry items were removed from `this._queue` (line 143: `this._queue = notToFlush`)
- They're sitting in the scheduler's queue waiting to be processed
- The scheduler won't complete before the page unloads
- **Those retry attempts are lost**
The original synchronous `for` loop ensured all retries were at least attempted before moving on. The scheduler breaks this guarantee.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+101
to
+108
| private _processNormal(deadline: { timeRemaining: () => number }): void { | ||
| this._normalScheduled = null | ||
|
|
||
| while (this._normalQueue.length > 0) { | ||
| // Always let high priority tasks run first | ||
| if (this._highQueue.length > 0) { | ||
| this._normalScheduled = _requestIdleCallback((d) => this._processNormal(d)) | ||
| return |
Contributor
There was a problem hiding this comment.
Potential starvation issue: When processing normal priority tasks, if high priority tasks keep getting added, normal priority tasks will keep yielding and may never complete.
The current logic:
- Check if high priority queue has tasks
- If yes, reschedule normal priority processing and return
- This repeats every time normal processing is attempted
If high priority tasks are added continuously (e.g., during heavy user interaction), normal priority tasks could be starved indefinitely. Consider adding a mechanism to:
- Process at least some normal priority tasks before yielding
- Or track how many times a normal priority task has been deferred and eventually force process it
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/utils/scheduler.ts
Line: 101:108
Comment:
Potential starvation issue: When processing normal priority tasks, if high priority tasks keep getting added, normal priority tasks will keep yielding and may never complete.
The current logic:
1. Check if high priority queue has tasks
2. If yes, reschedule normal priority processing and return
3. This repeats every time normal processing is attempted
If high priority tasks are added continuously (e.g., during heavy user interaction), normal priority tasks could be starved indefinitely. Consider adding a mechanism to:
- Process at least some normal priority tasks before yielding
- Or track how many times a normal priority task has been deferred and eventually force process it
How can I resolve this? If you propose a fix, please make it concise.| }) | ||
| ) | ||
|
|
||
| scheduler.setActive(!!this.config.__preview_extra_scheduling) |
Contributor
There was a problem hiding this comment.
The scheduler is only activated when __preview_extra_scheduling is enabled, which means the new time-slicing behavior won't be used in production by default. However, the PR description states "we tested yielding processing to the main thread periodically and it worked well" and suggests it's ready for broader use.
Consider:
- Is this intentionally behind a feature flag for gradual rollout?
- If so, the flag name with
__preview_suggests it's experimental - consider a clearer flag name - If the feature is ready, consider enabling it by default or documenting the rollout plan
Additionally, note that when the scheduler is inactive (_active: false), all the scheduler-using code (request-queue, retry-queue, site-apps, lazy-loaded-session-recorder) will process synchronously, which means they don't get the INP benefits mentioned in the PR description.
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/posthog-core.ts
Line: 521:521
Comment:
The scheduler is only activated when `__preview_extra_scheduling` is enabled, which means the new time-slicing behavior won't be used in production by default. However, the PR description states "we tested yielding processing to the main thread periodically and it worked well" and suggests it's ready for broader use.
Consider:
1. Is this intentionally behind a feature flag for gradual rollout?
2. If so, the flag name with `__preview_` suggests it's experimental - consider a clearer flag name
3. If the feature is ready, consider enabling it by default or documenting the rollout plan
Additionally, note that when the scheduler is inactive (`_active: false`), all the scheduler-using code (request-queue, retry-queue, site-apps, lazy-loaded-session-recorder) will process synchronously, which means they don't get the INP benefits mentioned in the PR description.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+23
to
+28
| const fastForwardTimeAndRunTimer = async (time = 3500) => { | ||
| now += time | ||
| jest.setSystemTime(now) | ||
| jest.runOnlyPendingTimers() | ||
| jest.runAllTimers() | ||
| // eslint-disable-next-line compat/compat | ||
| await Promise.resolve() |
Contributor
There was a problem hiding this comment.
Good fix to use jest.runAllTimers() instead of jest.runOnlyPendingTimers() since the scheduler creates new timers during processing. However, this introduces a risk: if there's a bug in the scheduler that causes it to continuously schedule work, tests will hang indefinitely.
Consider adding a timeout to these tests (e.g., jest.setTimeout(5000)) to catch infinite scheduling bugs early.
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/__tests__/retry-queue.test.ts
Line: 23:28
Comment:
Good fix to use `jest.runAllTimers()` instead of `jest.runOnlyPendingTimers()` since the scheduler creates new timers during processing. However, this introduces a risk: if there's a bug in the scheduler that causes it to continuously schedule work, tests will hang indefinitely.
Consider adding a timeout to these tests (e.g., `jest.setTimeout(5000)`) to catch infinite scheduling bugs early.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
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.
we tested yielding processing to the main thread periodically and it worked well
reducing variability of INP on pages using that mode
let's abstract the task queue processing approach and use it in a few more places where we might be running over large arrays and cause a long task