Fix timegraph misalignment when analysis running#1210
Merged
PatrickTasse merged 1 commit intoeclipse-cdt-cloud:masterfrom Jul 2, 2025
Merged
Fix timegraph misalignment when analysis running#1210PatrickTasse merged 1 commit intoeclipse-cdt-cloud:masterfrom
PatrickTasse merged 1 commit intoeclipse-cdt-cloud:masterfrom
Conversation
Contributor
|
I was able to replicate the margin bug and validate bug fix to be functional. |
Wait for table header to be rendered before setting top margin. Fixes eclipse-cdt-cloud#1207 Signed-off-by: Patrick Tasse <patrick.tasse@gmail.com>
aaanh
approved these changes
Jul 2, 2025
Contributor
aaanh
left a comment
aaanh
left a comment
There was a problem hiding this comment.
Check condition now depends on whether or not the headers and items exist and non-empty.
| } | ||
|
|
||
| private async setMarginTop() { | ||
| const header = await this.waitForHeader(); |
Contributor
There was a problem hiding this comment.
We should verify that waitForHeader is executed for a minimal number calls. If not then, it should be called in a useEffect hook with an empty dependency array or the state that we're looking for changes.
Example
useEffect(() => {}, [empty or headersState])
Comment on lines
411
to
418
| const getHeader = () => { | ||
| const header = this.treeRef.current?.querySelector('th'); | ||
| if (header) { | ||
| resolve(header); | ||
| } else { | ||
| requestAnimationFrame(getHeader); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Avoid nested function definition if possible, might cause mem leak
bhufmann
approved these changes
Jul 2, 2025
Contributor
bhufmann
left a comment
bhufmann
left a comment
There was a problem hiding this comment.
I confirm that the fix works. Thanks for investigating this.
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
3 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.
What it does
Wait for table header to be rendered before setting top margin.
Fixes #1207
How to test
Remove trace from trace viewer if it exists, to make sure analyses haven't run
Open a timegraph output with a slow analysis, or use breakpoint in analysis to simulate a delay
Wait for analysis to complete, or exit the breakpoint
Observe that timegraph has a top margin equal to tree header and rows are aligned with the tree
Follow-ups
N/A
Review checklist