-
Notifications
You must be signed in to change notification settings - Fork 94
Web: Improve trace viewer load times and loading animation #270
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
Changes from all commits
9230c4a
82ef142
7aaf7e3
ecee45b
2e587e7
2c82672
da6aa82
668c518
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@workflow/cli": patch | ||
| --- | ||
|
|
||
| Fix --noBrowser option help documentation |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@workflow/web-shared": patch | ||
| "@workflow/web": patch | ||
| --- | ||
|
|
||
| Improve trace viewer load times and loading animation | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,13 +92,22 @@ export const cliFlags = { | |
| helpGroup: 'Output', | ||
| helpLabel: '-w, --web', | ||
| }), | ||
| webPort: Flags.integer({ | ||
| description: 'Port to use when launching the web UI (default: 3456)', | ||
| required: false, | ||
| default: 3456, | ||
| helpGroup: 'Output', | ||
| helpLabel: '--webPort', | ||
| helpValue: 'WEB_PORT', | ||
| env: 'WORKFLOW_WEB_PORT', | ||
| }), | ||
| noBrowser: Flags.boolean({ | ||
| description: 'Disable automatic browser opening when launching web UI', | ||
| required: false, | ||
| default: false, | ||
| env: 'WORKFLOW_DISABLE_BROWSER_OPEN', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if all the cli related env vars should be prefixed with WORKFLOW_CLI_ (so we have consistency here with expectations)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added to my TODO list |
||
| helpGroup: 'Output', | ||
| helpLabel: '--no-browser', | ||
| helpLabel: '--noBrowser', | ||
| }), | ||
| sort: Flags.string({ | ||
| description: 'sort order for list commands', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,8 @@ import { | |
| } from './workflow-server-actions'; | ||
|
|
||
| const MAX_ITEMS = 1000; | ||
| const LIVE_POLL_LIMIT = 5; | ||
| const LIVE_POLL_LIMIT = 10; | ||
| const LIVE_STEP_UPDATE_INTERVAL_MS = 2000; | ||
| const LIVE_UPDATE_INTERVAL_MS = 5000; | ||
|
|
||
| /** | ||
|
|
@@ -660,6 +661,7 @@ export function useWorkflowTraceViewerData( | |
| const [hooks, setHooks] = useState<Hook[]>([]); | ||
| const [events, setEvents] = useState<Event[]>([]); | ||
| const [loading, setLoading] = useState(true); | ||
| const [auxiliaryDataLoading, setAuxiliaryDataLoading] = useState(false); | ||
| const [error, setError] = useState<Error | null>(null); | ||
|
|
||
| const [stepsCursor, setStepsCursor] = useState<string | undefined>(); | ||
|
|
@@ -677,29 +679,33 @@ export function useWorkflowTraceViewerData( | |
|
|
||
| isFetchingRef.current = true; | ||
| setLoading(true); | ||
| setAuxiliaryDataLoading(true); | ||
| setError(null); | ||
|
|
||
| const promises = [ | ||
| fetchRun(env, runId).then((result) => { | ||
| // The run is the most visible part - so we can start showing UI | ||
| // as soon as we have the run | ||
| setLoading(false); | ||
| setRun(unwrapServerActionResult(result)); | ||
| }), | ||
| fetchAllSteps(env, runId).then((result) => { | ||
| setSteps(result.data); | ||
| setStepsCursor(result.cursor); | ||
| }), | ||
| fetchAllHooks(env, runId).then((result) => { | ||
| setHooks(result.data); | ||
| setHooksCursor(result.cursor); | ||
| }), | ||
| fetchAllEvents(env, runId).then((result) => { | ||
| setEvents(result.data); | ||
| setEventsCursor(result.cursor); | ||
| }), | ||
| ]; | ||
|
|
||
| try { | ||
| // Fetch run | ||
| const runServerResult = await fetchRun(env, runId); | ||
| const runData = unwrapServerActionResult(runServerResult); | ||
| setRun(runData); | ||
|
|
||
| // TODO: Do these in parallel | ||
| // Fetch steps exhaustively | ||
| const stepsResult = await fetchAllSteps(env, runId); | ||
| setSteps(stepsResult.data); | ||
| setStepsCursor(stepsResult.cursor); | ||
|
|
||
| // Fetch hooks exhaustively | ||
| const hooksResult = await fetchAllHooks(env, runId); | ||
| setHooks(hooksResult.data); | ||
| setHooksCursor(hooksResult.cursor); | ||
|
|
||
| // Fetch events exhaustively | ||
| const eventsResult = await fetchAllEvents(env, runId); | ||
| setEvents(eventsResult.data); | ||
| setEventsCursor(eventsResult.cursor); | ||
| await Promise.all(promises); | ||
| } catch (err) { | ||
| const error = | ||
| err instanceof WorkflowAPIError | ||
|
|
@@ -711,6 +717,7 @@ export function useWorkflowTraceViewerData( | |
| setError(error); | ||
| } finally { | ||
| setLoading(false); | ||
| setAuxiliaryDataLoading(false); | ||
| isFetchingRef.current = false; | ||
| setInitialLoadCompleted(true); | ||
| } | ||
|
|
@@ -808,27 +815,31 @@ export function useWorkflowTraceViewerData( | |
| }, [env, runId, eventsCursor, mergeEvents]); | ||
|
|
||
| // Update function for live polling | ||
| const update = useCallback(async (): Promise<{ foundNewItems: boolean }> => { | ||
| if (isFetchingRef.current || !initialLoadCompleted) { | ||
| return { foundNewItems: false }; | ||
| } | ||
| const update = useCallback( | ||
| async (stepsOnly: boolean = false): Promise<{ foundNewItems: boolean }> => { | ||
| if (isFetchingRef.current || !initialLoadCompleted) { | ||
| return { foundNewItems: false }; | ||
| } | ||
|
|
||
| let foundNewItems = false; | ||
| let foundNewItems = false; | ||
|
|
||
| try { | ||
| const [_, stepsUpdated, hooksUpdated, eventsUpdated] = await Promise.all([ | ||
| pollRun(), | ||
| pollSteps(), | ||
| pollHooks(), | ||
| pollEvents(), | ||
| ]); | ||
| foundNewItems = stepsUpdated || hooksUpdated || eventsUpdated; | ||
| } catch (err) { | ||
| console.error('Update error:', err); | ||
| } | ||
| try { | ||
| const [_, stepsUpdated, hooksUpdated, eventsUpdated] = | ||
| await Promise.all([ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all this data fetching logic and reactivity is making me think of RxJS 🥲
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this needs an overhaul eventually |
||
| stepsOnly ? Promise.resolve(false) : pollRun(), | ||
| pollSteps(), | ||
| stepsOnly ? Promise.resolve(false) : pollHooks(), | ||
| stepsOnly ? Promise.resolve(false) : pollEvents(), | ||
| ]); | ||
| foundNewItems = stepsUpdated || hooksUpdated || eventsUpdated; | ||
| } catch (err) { | ||
| console.error('Update error:', err); | ||
| } | ||
|
|
||
| return { foundNewItems }; | ||
| }, [pollSteps, pollHooks, pollEvents, initialLoadCompleted, pollRun]); | ||
| return { foundNewItems }; | ||
| }, | ||
| [pollSteps, pollHooks, pollEvents, initialLoadCompleted, pollRun] | ||
| ); | ||
|
|
||
| // Initial load | ||
| useEffect(() => { | ||
|
|
@@ -844,8 +855,14 @@ export function useWorkflowTraceViewerData( | |
| const interval = setInterval(() => { | ||
| update(); | ||
| }, LIVE_UPDATE_INTERVAL_MS); | ||
|
|
||
| return () => clearInterval(interval); | ||
| const stepInterval = setInterval(() => { | ||
| update(true); | ||
| }, LIVE_STEP_UPDATE_INTERVAL_MS); | ||
|
|
||
| return () => { | ||
| clearInterval(interval); | ||
| clearInterval(stepInterval); | ||
| }; | ||
| }, [live, initialLoadCompleted, update, run?.completedAt]); | ||
|
|
||
| return { | ||
|
|
@@ -854,6 +871,7 @@ export function useWorkflowTraceViewerData( | |
| hooks, | ||
| events, | ||
| loading, | ||
| auxiliaryDataLoading, | ||
| error, | ||
| update, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import { cn } from '../../lib/utils'; | ||
|
|
||
| function Skeleton({ | ||
| className, | ||
| ...props | ||
| }: React.HTMLAttributes<HTMLDivElement>) { | ||
| return ( | ||
| <div | ||
| className={cn('animate-pulse rounded-md bg-muted', className)} | ||
| {...props} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| export { Skeleton }; |
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.
Should we have a new group for web?
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.
Added to my TODO list