Repository navigation
Perf/telemetry optimization - #177
Merged
Merged
Conversation
- Caches global settings and app version on mount in App.tsx to avoid thousands of redundant GET calls on event arrivals.
- Intercepts and dispatches client-side telemetry events locally to Google Analytics without a roundtrip feedback loop.
- Added a broadcast bypass option { broadcast: false } to trackTelemetry for client-originated events.
- Suppressed noisy heartbeat logs in SharedEventSource to clean the browser console.
- Excluded telemetry POST and health GET calls from server request logging to clean the backend console.
- Successfully verified by passing all 57 backend tests and all 35 E2E Playwright tests.
…s to optimize CPU
Contributor
📝 WalkthroughWalkthroughThis PR refactors telemetry broadcasting and optimizes background polling across the application. The backend now supports selective event broadcasting, the frontend centralizes telemetry processing with cached state, and multiple polling mechanisms reduce unnecessary activity when the browser tab is hidden. ChangesTelemetry Broadcast Control and Background Polling Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aviatam
approved these changes
May 22, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/hooks/useFileWatcherEvents.ts (1)
46-53: ⚡ Quick winAdd visibility-resume catch-up refresh.
On Line 48 and Line 53, skipping hidden-tab polling is good, but returning to the tab can leave stale data for up to 120s. Add a
visibilitychangehandler to trigger one immediate refresh when the tab becomes visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useFileWatcherEvents.ts` around lines 46 - 53, The polling block skips work when document.hidden which can leave data stale on resume; add a document.addEventListener('visibilitychange', ...) inside useFileWatcherEvents to immediately call appApi.getProjectWorkflows(activeProject.id).then(setWorkflows).catch(()=>{}) and appApi.listArtifacts(activeProject.id).then(setArtifacts).catch(()=>{}) when the tab becomes visible (check document.visibilityState === 'visible' or !document.hidden), and ensure you remove that listener in the hook cleanup (same place you clearInterval) so you don't leak listeners; use the same handlers used in the interval (setWorkflows/setArtifacts) and keep error handling consistent.src/hooks/useWorkspaceInit.ts (1)
75-80: ⚡ Quick winTrigger fallback refresh when tab becomes visible.
On Line 76, this can defer recovery refresh up to 5 minutes after tab restore. Consider a one-shot
visibilitychangerefresh whendocument.hiddenbecomesfalse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useWorkspaceInit.ts` around lines 75 - 80, The periodic interval in useWorkspaceInit skips refreshFallback while document.hidden and can delay recovery up to 5 minutes; add a one-shot visibilitychange handler that, when document.visibilityState becomes "visible" (or document.hidden === false), checks appApi.isServerOnline() and calls refreshFallback().catch(() => {}) immediately, then removes itself; register this listener alongside the existing interval and ensure it is cleaned up when the hook unmounts (clearInterval and removeEventListener) so the one-shot runs only once per visibility transition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/App.tsx`:
- Around line 323-329: The handler handleSettingsChanged is trusting every
productos:settings-changed event and overwrites activeSettings even for
project-level updates; change it to only update activeSettings when the event
payload explicitly indicates global settings (e.g., check
customEvent.detail.scope === 'global' or a dedicated flag like
customEvent.detail.isGlobal === true or absence of a projectId), otherwise
ignore or log and return; update the conditional inside handleSettingsChanged to
guard on that property before assigning activeSettings and emitting telemetry,
and reference runtimeApi.saveProjectSettings as the source that emits
project-level payloads so you can differentiate project vs global payloads.
In `@src/components/workflow/WorkflowList.tsx`:
- Around line 53-63: The listener promise can resolve after cleanup leaving the
handler attached; introduce a boolean flag (e.g., disposed) in the component
scope, set disposed = true in the cleanup, and when the
appApi.listen<any>('workflow-progress', ...) promise resolves check the flag: if
disposed call the returned unlisten function immediately (u()), otherwise assign
it to the existing unlisten variable; ensure the same disposed check is used
before assigning/using unlisten in the return cleanup and keep pollRuns()
invocation unchanged.
---
Nitpick comments:
In `@src/hooks/useFileWatcherEvents.ts`:
- Around line 46-53: The polling block skips work when document.hidden which can
leave data stale on resume; add a document.addEventListener('visibilitychange',
...) inside useFileWatcherEvents to immediately call
appApi.getProjectWorkflows(activeProject.id).then(setWorkflows).catch(()=>{})
and appApi.listArtifacts(activeProject.id).then(setArtifacts).catch(()=>{}) when
the tab becomes visible (check document.visibilityState === 'visible' or
!document.hidden), and ensure you remove that listener in the hook cleanup (same
place you clearInterval) so you don't leak listeners; use the same handlers used
in the interval (setWorkflows/setArtifacts) and keep error handling consistent.
In `@src/hooks/useWorkspaceInit.ts`:
- Around line 75-80: The periodic interval in useWorkspaceInit skips
refreshFallback while document.hidden and can delay recovery up to 5 minutes;
add a one-shot visibilitychange handler that, when document.visibilityState
becomes "visible" (or document.hidden === false), checks appApi.isServerOnline()
and calls refreshFallback().catch(() => {}) immediately, then removes itself;
register this listener alongside the existing interval and ensure it is cleaned
up when the hook unmounts (clearInterval and removeEventListener) so the
one-shot runs only once per visibility transition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5db05990-84d9-4151-b340-28ee54ed2b7f
📒 Files selected for processing (8)
node-backend/lib/telemetry/index.mjsnode-backend/server.mjssrc/App.tsxsrc/api/runtime.tssrc/api/server.tssrc/components/workflow/WorkflowList.tsxsrc/hooks/useFileWatcherEvents.tssrc/hooks/useWorkspaceInit.ts
Comment on lines
+323
to
+329
| const handleSettingsChanged = (e: Event) => { | ||
| const customEvent = e as CustomEvent; | ||
| if (customEvent.detail) { | ||
| console.log('[Telemetry] Active settings updated:', customEvent.detail); | ||
| activeSettings = customEvent.detail; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard cache updates to global settings payload only.
Line 323-Line 329 currently trusts every productos:settings-changed payload as global settings. runtimeApi.saveProjectSettings dispatches the same event name with project settings, which can overwrite activeSettings and cause telemetry to be sent even after a global opt-out.
Proposed fix
- const handleSettingsChanged = (e: Event) => {
- const customEvent = e as CustomEvent;
- if (customEvent.detail) {
- console.log('[Telemetry] Active settings updated:', customEvent.detail);
- activeSettings = customEvent.detail;
- }
- };
+ const handleSettingsChanged = (e: Event) => {
+ const customEvent = e as CustomEvent;
+ const detail = customEvent.detail;
+ if (detail && typeof detail === 'object' && 'telemetry' in detail) {
+ activeSettings = detail;
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleSettingsChanged = (e: Event) => { | |
| const customEvent = e as CustomEvent; | |
| if (customEvent.detail) { | |
| console.log('[Telemetry] Active settings updated:', customEvent.detail); | |
| activeSettings = customEvent.detail; | |
| } | |
| }; | |
| const handleSettingsChanged = (e: Event) => { | |
| const customEvent = e as CustomEvent; | |
| const detail = customEvent.detail; | |
| if (detail && typeof detail === 'object' && 'telemetry' in detail) { | |
| activeSettings = detail; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/App.tsx` around lines 323 - 329, The handler handleSettingsChanged is
trusting every productos:settings-changed event and overwrites activeSettings
even for project-level updates; change it to only update activeSettings when the
event payload explicitly indicates global settings (e.g., check
customEvent.detail.scope === 'global' or a dedicated flag like
customEvent.detail.isGlobal === true or absence of a projectId), otherwise
ignore or log and return; update the conditional inside handleSettingsChanged to
guard on that property before assigning activeSettings and emitting telemetry,
and reference runtimeApi.saveProjectSettings as the source that emits
project-level payloads so you can differentiate project vs global payloads.
Comment on lines
+53
to
+63
| let unlisten: (() => void) | undefined; | ||
| appApi.listen<any>('workflow-progress', () => { | ||
| pollRuns(); | ||
| }).then(u => { | ||
| unlisten = u; | ||
| }).catch(() => {}); | ||
|
|
||
| return () => { | ||
| clearInterval(interval); | ||
| if (unlisten) unlisten(); | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle async listener teardown race in cleanup.
On Line 53 and Line 60, unmount can happen before the listen promise resolves, leaving the listener attached. Add a disposed flag and unlisten immediately if registration finishes after cleanup.
Proposed fix
- let unlisten: (() => void) | undefined;
+ let unlisten: (() => void) | undefined;
+ let disposed = false;
appApi.listen<any>('workflow-progress', () => {
pollRuns();
}).then(u => {
- unlisten = u;
+ if (disposed) {
+ u();
+ } else {
+ unlisten = u;
+ }
}).catch(() => {});
return () => {
+ disposed = true;
clearInterval(interval);
if (unlisten) unlisten();
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/workflow/WorkflowList.tsx` around lines 53 - 63, The listener
promise can resolve after cleanup leaving the handler attached; introduce a
boolean flag (e.g., disposed) in the component scope, set disposed = true in the
cleanup, and when the appApi.listen<any>('workflow-progress', ...) promise
resolves check the flag: if disposed call the returned unlisten function
immediately (u()), otherwise assign it to the existing unlisten variable; ensure
the same disposed check is used before assigning/using unlisten in the return
cleanup and keep pollRuns() invocation unchanged.
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
2 participants
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.
Summary by CodeRabbit
Release Notes