feat(dashboard): re run activity item #7459
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| <CardContent className="overflow-hidden rounded-lg bg-neutral-50 p-2"> | ||
| <div className="flex min-w-0 items-center gap-2"> | ||
| <div className="min-w-0 flex-1 overflow-hidden"> | ||
| <span className="text-foreground-400 block truncate text-xs">{getStatusMessage(job)}</span> | ||
| </div> | ||
| <Badge variant="lighter" color="gray" size="sm" className="shrink-0 whitespace-nowrap"> |
There was a problem hiding this comment.
Fixed clipping of the date badge on long text
scopsy
changed the title
|
|
||
| export function ActivityPanel({ | ||
| activityId, | ||
| transactionId: initialTransactionId, | ||
| onActivitySelect, | ||
| headerClassName, | ||
| overviewHeaderClassName, | ||
| }: ActivityPanelProps) { | ||
| const queryClient = useQueryClient(); | ||
| const [shouldRefetch, setShouldRefetch] = useState(true); | ||
| const track = useTelemetry(); | ||
| const { isLoadingTransaction, setTransactionId } = useActivityByTransaction({ | ||
| transactionId: initialTransactionId, | ||
| onActivityFound: onActivitySelect, | ||
| }); | ||
|
|
||
| const { activity, isPending, error } = useActivityPolling({ | ||
| activityId, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Moved the logic of fetching and refetching by transaction id to the activity panel to allow initializing by either activityId or transaction id
There was a problem hiding this comment.
Can you please elaborate on why do we need to do both? The transaction is identified by transactionID, so I think we can just use that.
There was a problem hiding this comment.
When vieweing an item from the activity feed, we want to view a specific activityId, transacitonId is used in cases of a re-trigger, and also from the test workflow sidebar. As a trigger can potentially generate multiple activity ids, when testing we assume it generates one and just take the first.
There was a problem hiding this comment.
I see. Let's continue here. My suggestion is to work with the notification list (activity feed) page as the main resource and the selected notification (details of a run).
If not mistaken, the transactionId for the rerun should be available in the selected notification resource.
| isLoading?: boolean; | ||
| } | ||
| >(({ children, icon: Icon, ...rest }, forwardedRef) => { | ||
| >(({ children, icon: Icon, isLoading, disabled, ...rest }, forwardedRef) => { | ||
| return ( | ||
| <CompactButtonRoot ref={forwardedRef} {...rest}> | ||
| <CompactButtonIcon as={Icon} /> | ||
| <CompactButtonRoot ref={forwardedRef} disabled={isLoading || disabled} {...rest}> | ||
| <CompactButtonIcon as={isLoading ? RiLoader4Line : Icon} className={isLoading ? 'loading' : ''} /> | ||
| </CompactButtonRoot> |
There was a problem hiding this comment.
Add's a nice loading state
|
|
||
| export function ActivityPanel({ | ||
| activityId, | ||
| transactionId: initialTransactionId, | ||
| onActivitySelect, | ||
| headerClassName, | ||
| overviewHeaderClassName, | ||
| }: ActivityPanelProps) { | ||
| const queryClient = useQueryClient(); | ||
| const [shouldRefetch, setShouldRefetch] = useState(true); | ||
| const track = useTelemetry(); | ||
| const { isLoadingTransaction, setTransactionId } = useActivityByTransaction({ | ||
| transactionId: initialTransactionId, | ||
| onActivityFound: onActivitySelect, | ||
| }); | ||
|
|
||
| const { activity, isPending, error } = useActivityPolling({ | ||
| activityId, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Can you please elaborate on why do we need to do both? The transaction is identified by transactionID, so I think we can just use that.
| }); | ||
| }, [activity, queryClient, currentEnvironment, activityId]); | ||
|
|
||
| return { |
There was a problem hiding this comment.
Nit picky: return all the results from useFetchActivity.
| onActivityFound: (activityId: string) => void; | ||
| } | ||
|
|
||
| export function useActivityByTransaction({ |
There was a problem hiding this comment.
This hook is needed to correct a misalignment in the dashboard's Activity Feed. The current notification item, ? activityItemFeed=..., is already present in the address bar.
So, it's unclear if we rerun based on transactionID or Notification ID.
My suggestion is to use the notificationId for now. We shouldn't need the useState; we should get it from the URL. That would eliminate the need for this hook.
Having said that, I suggest changing the URL schema to
https://dashboard-v2.novu.co/env/:env_id/activity-feed/:notificationId to map with the API powering this screen.
There was a problem hiding this comment.
Sorry, but I do not entirely follow your suggestion. The re run uses the current notification entity, and performs a trigger using the same payload and recipient properties as the previous trigger.
useActivityByTransactionId is just a refactor I did that encapsulates the fetch activities given a specific transactionId, this happens in 2 cases:
- We perform a trigger in the test workflow sidebar (Workflow editor), the API returns a transactionId, and we need to obtain the notification Id by searching the notification feed based on the transaction id and select the first activity item.
- When we re-run an activity item, the API returns us a transactionId, which we again need to convert to a notificationId/activityId to display the full details.
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer