Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1446 +/- ##
==========================================
- Coverage 90.01% 89.71% -0.31%
==========================================
Files 67 68 +1
Lines 4769 4880 +111
Branches 885 888 +3
==========================================
+ Hits 4293 4378 +85
- Misses 458 484 +26
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
re-vlad
left a comment
There was a problem hiding this comment.
lets couple of non-blocking suggestions
| { | ||
| tabName: 'Steps', | ||
| tabIcon: TimelineIcon, | ||
| tabContent: <StepsTimeline steps={push.steps} />, |
There was a problem hiding this comment.
Can we pass steps={push.steps ?? []} into StepsTimeline so the component never receives undefined
There was a problem hiding this comment.
Also noticed that StepsTimeline already has an expandStepId prop that auto-expands a specific accordion, but it's never passed from PushDetails. If we calculate the first failing step, we can auto-expand it when the user opens the Steps tab:
const firstErrorStep = (push.steps ?? []).find((step) => step.error);
tabContent: <StepsTimeline steps={push.steps ?? []} expandStepId={firstErrorStep?.id} />
This way errors are immediately visible without the user having to manually click the failing step.
There was a problem hiding this comment.
@fabiovincenzi auto-opening the steps with errors is a nice idea, but feels strange passing firstErrorStep in when StepsTimeline has all the information to do itself?
| if (isError) throw new Error(message || 'Something went wrong ...'); | ||
| if (!push) return <div>No push data found</div>; | ||
|
|
||
| const errorCount = push.steps.filter((step) => step.error).length; |
There was a problem hiding this comment.
Small non-blocking suggestion: if push.steps is ever undefined (e.g. old API shape or malformed response), this throws. As for me, optional is more appropriate, something like this: const errorCount = push.steps?.filter((step) => step.error).length ?? 0
| </TableRow> | ||
| </TableHead> | ||
| <TableBody> | ||
| {commitData.map((c) => ( |
There was a problem hiding this comment.
When commitData is empty, the table renders only headers with no body rows. Can you add an explicit empty state (e.g. a single row with colSpan and "No commits" or a short message)
Summary
It can be hard to understand why a push was rejected. This PR adds a view of each step involved in a push, making it easier to identify failures. It also fixes #1387 and prevents the UI from crashing when a push fails before a diff is generated.
We use "steps" instead of actions in the UI, since “actions” was confusing for users.
Solution
CustomTabsto support an optional badge.