-
Notifications
You must be signed in to change notification settings - Fork 25
feat(dashboard): Add task node run information to the sidebard #1950
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
base: master
Are you sure you want to change the base?
Conversation
ba464b1 to
ff94732
Compare
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 see how cool is to use this magic classes to process colors for each state, although there's something you are missing handling it in this way.
You won't be able to catch when a new status is added in build time. I would suggest to change this to a typed data and not use css
| export const NodeStatus = ({ status }: { status: LHStatus }) => { | ||
|
|
||
| export const NodeStatus = ({ status, type }: { status: LHStatus | TaskStatus; type?: string }) => { | ||
| const statusType = type === 'task' ? `task-status--${status.toUpperCase()}` : `node-status--${status.toLowerCase()}` |
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.
why some are lowercase and others uppercase?
This is where if you don't use a typed data structure, you won't catch when a new status is added during build time. Use typed data instead of classNames as strings.
| export const getAttemptOutput = (output: VariableValue | undefined): string => { | ||
| if (!output?.value?.value) return 'No Output' | ||
| return String(getVariableValue(output)) | ||
| } | ||
|
|
||
| export const getAttemptResult = ( | ||
| result: | ||
| | { $case: 'output'; value: VariableValue } | ||
| | { $case: 'error'; value: any } | ||
| | { $case: 'exception'; value: any } | ||
| | undefined | ||
| ): string => { | ||
| if (!result) return 'No Output' | ||
|
|
||
| if (result.$case === 'output') { | ||
| return getAttemptOutput(result.value) | ||
| } | ||
|
|
||
| // For error/exception, prefer a message property if present, otherwise fallback to JSON | ||
| const val = result.value | ||
| return (val && (val.message ?? val.msg ?? val.toString())) || JSON.stringify(val) || 'No Output' | ||
| } |
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.
What is this? you should use the types from the littlehorse-client library. Also avoid using any
| const { tenantId } = useWhoAmI() | ||
| const [nodeTask, setNodeTask] = useState<TaskRun>() | ||
| const [attemptIndex, setAttemptIndex] = useState(0) | ||
| useLayoutEffect(() => { |
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.
You shouldn't use useLayoutEffect this must be handled by SWR.
The task node information is available on the sidebard
