Skip to content

Scaffolder - Addition of Dismissable Error Banner #13522

Merged
freben merged 3 commits intobackstage:masterfrom
mufaddal7:scaffolder/error-handling
Sep 9, 2022
Merged

Scaffolder - Addition of Dismissable Error Banner #13522
freben merged 3 commits intobackstage:masterfrom
mufaddal7:scaffolder/error-handling

Conversation

@mufaddal7
Copy link
Contributor

Signed-off-by: mufaddal motiwala mufaddalmm.52@gmail.com

Hey, I just made a Pull Request!

This PR is our first step to enhance exception handling in the Scaffolder and show it back to the user.

Currently I have made a simple change and have managed to show the user an error pop up , whenever an error is getting logged in the completion phase using the existing TaskStream features.

image

Future

The future plan is to enhance exception handling by adding the Error field to TaskEventType , something like this -

export type TaskEventType = 'completion' | 'log' | 'error';

#12760

Signed-off-by: mufaddal motiwala <mufaddalmm.52@gmail.com>
@mufaddal7 mufaddal7 requested review from a team as code owners September 4, 2022 19:56
@github-actions github-actions bot added the area:scaffolder Everything and all things related to the scaffolder project area label Sep 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2022

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-scaffolder plugins/scaffolder patch v1.6.0-next.2

Signed-off-by: mufaddal motiwala <mufaddalmm.52@gmail.com>
};

export const TaskErrors = ({ taskStream }: TaskPageLinksProps) => {
return taskStream.error ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really work? Seems to me that the taskStream itself doesn't necessarily have its reference (instance) value change? Since the error is what actually "drives" redraws here, it seems to me that if this works then it's accidentally because the surrounding component had to redraw itself, right? Essentially I'm asking if there should be a singe error?: Error | undefined prop instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work, I checked it on fresh instances.

So I chose to take the entire TaskStream Prop as in the future we may be extracting errors from EventStream stepLogs , yeah for now we can only take a single prop, should I make that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change

return taskStream.error ? (
<Box>
<DismissableBanner
id={String(Math.random())}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will update the id on every repaint, which probably is not desirable. The render phase should be side-effect-free. Maybe you could have something like this (untested written out of my own head):

const id = useRef('');

useEffect(() => {
  id.current = String(Math.random());
}, [error]);

and then use id={id.current}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, thanks!

Signed-off-by: mufaddal motiwala <mufaddalmm.52@gmail.com>
@mufaddal7 mufaddal7 requested a review from freben September 9, 2022 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scaffolder Everything and all things related to the scaffolder project area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants