-
Notifications
You must be signed in to change notification settings - Fork 578
Fix dev server hang by reducing log spam and event frequency #828
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
Changes from all commits
fc6c69f
90be17f
736e12d
0a6e7a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -88,9 +88,13 @@ const PORT_PATTERNS: Array<{ pattern: RegExp; description: string }> = [ | |||||||||
| }, | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| // Throttle output to prevent overwhelming WebSocket under heavy load | ||||||||||
| const OUTPUT_THROTTLE_MS = 4; // ~250fps max update rate for responsive feedback | ||||||||||
| const OUTPUT_BATCH_SIZE = 4096; // Smaller batches for lower latency | ||||||||||
| // Throttle output to prevent overwhelming WebSocket under heavy load. | ||||||||||
| // 100ms (~10fps) is sufficient for readable log streaming while keeping | ||||||||||
| // WebSocket traffic manageable. The previous 4ms rate (~250fps) generated | ||||||||||
| // up to 250 events/sec which caused progressive browser slowdown from | ||||||||||
| // accumulated console logs, JSON serialization overhead, and React re-renders. | ||||||||||
|
Comment on lines
+91
to
+95
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comments added to explain the throttling mechanism are very helpful for understanding the rationale behind the changes. However, the comment could be improved by explicitly mentioning the specific metrics or monitoring tools that can be used to observe the impact of the throttling on browser performance and server load. This would provide additional guidance for future maintenance and optimization. It's important to monitor the impact of this change on the user experience. If the throttling is too aggressive, it could lead to a noticeable degradation in the responsiveness of the dev server logs. |
||||||||||
| const OUTPUT_THROTTLE_MS = 100; // ~10fps max update rate | ||||||||||
| const OUTPUT_BATCH_SIZE = 8192; // Larger batches to compensate for lower frequency | ||||||||||
|
Comment on lines
+96
to
+97
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Making these values configurable would provide flexibility to adapt to different network conditions and browser capabilities.
Suggested change
|
||||||||||
|
|
||||||||||
| export interface DevServerInfo { | ||||||||||
| worktreePath: string; | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,8 +90,8 @@ describe('DevServerService Event Types', () => { | |
|
|
||
| // 2. Output & URL Detected | ||
| mockProcess.stdout.emit('data', Buffer.from('Local: http://localhost:5173/\n')); | ||
| // Throttled output needs a bit of time | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| // Throttled output needs a bit of time (OUTPUT_THROTTLE_MS is 100ms) | ||
| await new Promise((resolve) => setTimeout(resolve, 250)); | ||
|
Comment on lines
+93
to
+94
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The timeout in the test is increased from 100ms to 250ms to accommodate the new throttle rate. While this addresses the immediate issue, it's important to ensure that the test remains reliable and doesn't become flaky due to timing variations. Consider adding additional assertions or retries to make the test more robust. It's important to ensure that the test consistently passes and doesn't flake due to timing issues. |
||
| expect(emittedEvents['dev-server:output'].length).toBeGreaterThanOrEqual(1); | ||
| expect(emittedEvents['dev-server:url-detected'].length).toBe(1); | ||
| expect(emittedEvents['dev-server:url-detected'][0].url).toBe('http://localhost:5173/'); | ||
|
|
||
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.
The log level for the WebSocket not-open warning is changed from
infotowarn. While this change aligns with the severity of the message, it's important to ensure that such warnings are appropriately handled and monitored in a production environment. Consider adding metrics or alerts to track the frequency of these warnings to proactively address potential WebSocket connectivity issues.If the WebSocket is not opened, it might be a critical issue that needs to be addressed soon.