-
Notifications
You must be signed in to change notification settings - Fork 105
[HUD] Display on hud table the autorevert decisions #7304
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
Conversation
jeanschmidt
commented
Oct 6, 2025
- Highlight hud autorevert decisions on main hud table with a different background
- Add a banner on hover for autoreverted commits on main hud table
- Special background for signals that are flagged to autorevert, making them easier to identify
- Adds a banner on commit page about the autorevert and the identified signals
Signed-off-by: Jean Schmidt <[email protected]>
…nschmidt/hud_revert_info
Signed-off-by: Jean Schmidt <[email protected]>
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Signed-off-by: Jean Schmidt <[email protected]>
FYI: test error is unrelated to the change, it is a library that changed behaviour and I am not managing this in this PR |
torchci/pages/hud/[repoOwner]/[repoName]/[branch]/[[...page]].tsx
Outdated
Show resolved
Hide resolved
Note it wasn't a accidental changes, just gave up on fixing the test for the newer version of the library. End up updating anyway.
Yeah, ok, good point, extracted the logic and reused for both the grouped and expanded views, I wasn't going to support the expanded views, but I guess this is a good point that support it.
changed to orange |
} | ||
> | ||
{job.name} | ||
{isAutorevert && " ⚠️ (triggered autorevert)"} |
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.
I don't understand where this is shown
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.
this is on our tooltip
href={job.htmlUrl} | ||
target="_blank" | ||
rel="noreferrer" | ||
style={{ |
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.
nit: ideally this should be css classes instead of inline styles
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.
this one was hard to make it work :(
Signed-off-by: Jean Schmidt <[email protected]>
OK, it should be functionally correct now.
|
// Since we don't have job IDs in the table, we can't create direct job links | ||
job_url: undefined, | ||
// Try to create a HUD URL using the signal key as a filter | ||
hud_url: signalKey |
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.
this won't work, signal key is not a substring of HUD job name
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.
it's still overcomplicated and:
pointing to the jobs and a link to them;
won't work
but let's fix and improve in the follow up prs, as it provides immediate utility and I'd like it in sooner rather than later.
Thank you for reviewing and providing improvements. Lets make sure we're keeping track of this so we're not end up forgetting about this. |