fix(profiler-ui): rehydrate report on mount so split panes keep the chart#1321
Open
dylan-savage wants to merge 1 commit into
Open
fix(profiler-ui): rehydrate report on mount so split panes keep the chart#1321dylan-savage wants to merge 1 commit into
dylan-savage wants to merge 1 commit into
Conversation
…hart Report data lived only in ProfilerView local state, populated solely by the live Stop flow. Splitting a tab mounts fresh instances with empty state, so both panes showed 'No profiling data available' while the status line still read 'report available' (status is server-backed). Add a mount-time effect that refetches the existing report when has_report is true but local data is empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dylan-savage
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
Contributor
📝 WalkthroughWalkthrough
ChangesServer Report Rehydration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete. |
![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: 1
🤖 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 `@apps/profiler-ui/src/views/ProfilerView.tsx`:
- Around line 559-563: The guard condition at line 562 that checks if
treeData?.tree exists doesn't account for target switches. When a new target is
selected, the existing treeData may be stale data from the previous target,
causing the effect to return early without rehydrating the new target's data.
Modify the guard condition to also verify that the existing treeData corresponds
to the current target, not just check if treeData exists. This ensures that when
switching to a new target, the effect will properly proceed to rehydrate that
target's report instead of skipping it due to stale data from the previous
target.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa3a7382-64be-47c2-ac55-1550532415c1
📒 Files selected for processing (1)
apps/profiler-ui/src/views/ProfilerView.tsx
Comment on lines
+559
to
+563
| if (!isConnected) { rehydratedTargetRef.current = undefined; return; } | ||
| if (status?.active === true) return; // a live session owns the data | ||
| if (!status?.has_report) return; // nothing on the server to rehydrate | ||
| if (treeData?.tree != null) return; // already have data locally | ||
| if (rehydratedTargetRef.current === target) return; // already attempted for this target |
Contributor
There was a problem hiding this comment.
Rehydration guard can skip fetch for a newly selected target.
At Line 562, treeData is treated as globally valid, but target switches can leave stale local tree data from the previous target (Line 678 only changes target). That causes this effect to return early and not rehydrate the new target’s report.
💡 Proposed fix
@@
- <select
+ <select
style={styles.select}
value={target || ''}
- onChange={(e) => setTarget(e.target.value || null)}
+ onChange={(e) => {
+ const nextTarget = e.target.value || null;
+ setTarget(nextTarget);
+ // Clear local report state so rehydration can fetch for the new target
+ setReport('');
+ setTreeData(null);
+ setOriginalRoot(null);
+ setVizRoot(null);
+ setCallStack([]);
+ rehydratedTargetRef.current = undefined;
+ }}
disabled={isActive}
>🤖 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 `@apps/profiler-ui/src/views/ProfilerView.tsx` around lines 559 - 563, The
guard condition at line 562 that checks if treeData?.tree exists doesn't account
for target switches. When a new target is selected, the existing treeData may be
stale data from the previous target, causing the effect to return early without
rehydrating the new target's data. Modify the guard condition to also verify
that the existing treeData corresponds to the current target, not just check if
treeData exists. This ensures that when switching to a new target, the effect
will properly proceed to rehydrate that target's report instead of skipping it
due to stale data from the previous target.
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
1 participant
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.
Problem
Splitting a Profiler tab made both panes show "No profiling data available" even though the status line still read "Idle — report available".
Root cause
Report data (
treeData/vizRoot) lived only inProfilerViewlocal state and was populated solely by the live Stop flow. Splitting (splitGroupWithDocument) mounts freshProfilerViewinstances with empty state. Meanwhilestatusis server-backed (polled), sohas_reportstayed true — hence the divergence.Fix
Add a mount-time effect that calls the existing
fetchReport()once (guarded per-target) when connected, not actively profiling,status.has_reportis true, but localtreeDatais empty. This makes report data server-rehydratable like status, so freshly-mounted split panes reload the existing report.Fixes both the sunburst chart and the stats table (same
treeDatagate).Verification
🤖 Generated with Claude Code
Summary by CodeRabbit