Skip to content

Add banner to handle errors for Containers page instead of window.alert()#8897

Merged
mook-as merged 3 commits intorancher-sandbox:mainfrom
tylercritchlow:better-handling-of-errors-on-containers-page
Jul 10, 2025
Merged

Add banner to handle errors for Containers page instead of window.alert()#8897
mook-as merged 3 commits intorancher-sandbox:mainfrom
tylercritchlow:better-handling-of-errors-on-containers-page

Conversation

@tylercritchlow
Copy link
Copy Markdown
Contributor

#8893

This changes displaying error messages with window.alert() to using the banner component in the Containers page.

similar to how it is done for the volumes page implementation here: #8856

…rt()

Signed-off-by: Tyler Critchlow <tylerdarincritchlow@gmail.com>
@tylercritchlow
Copy link
Copy Markdown
Contributor Author

Added comments to explain what I am doing while cleaning the error message, I can remove them if wanted

@tylercritchlow tylercritchlow marked this pull request as ready for review July 8, 2025 04:08
@mook-as mook-as linked an issue Jul 8, 2025 that may be closed by this pull request
mook-as
mook-as previously approved these changes Jul 8, 2025
Copy link
Copy Markdown
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Basically fine; just a question about parsing out messages from msg= lines.

// Extract message from fatal/error format: time="..." level=fatal msg="actual message"
const msgMatch = rawMessage.match(/msg="([^"]+)"/);
if (msgMatch) {
return msgMatch[1].replace(/\\n/g, ' '); // Replace \n with spaces
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

\n doesn't actually matter because HTML doesn't display new lines, but it doesn't hurt either (except readability).


if (typeof rawMessage === 'string') {
// Extract message from fatal/error format: time="..." level=fatal msg="actual message"
const msgMatch = rawMessage.match(/msg="([^"]+)"/);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this need to worry about escaped quotes (i.e. the error message itself contains a quote and got escaped)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes it does, will push up change

Comment on lines +7 to +9
>
{{ error }}
</banner>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is equivalent to:

Suggested change
>
{{ error }}
</banner>
v-text="error" />

But there's no harm leaving it as-is either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry! Turns out this is not equivalent; it was breaking the <banner> rendering. Reverting this change fixed it.

Signed-off-by: Tyler Critchlow <tylerdarincritchlow@gmail.com>
Copy link
Copy Markdown
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

The code looks fine; I'm trying to figure out why the banner doesn't like me (the changes look like it should work). At least it's definitely safe from XSS via error messages :)

Image

Copy link
Copy Markdown
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Please revert the v-text= change; sorry about that. It broke <banner> rendering.

Signed-off-by: Tyler Critchlow <tylerdarincritchlow@gmail.com>
@tylercritchlow tylercritchlow requested a review from mook-as July 10, 2025 01:26
Copy link
Copy Markdown
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Seems to work.

@mook-as mook-as enabled auto-merge July 10, 2025 17:27
@mook-as mook-as merged commit ed586c5 into rancher-sandbox:main Jul 10, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI: Containers: Avoid use of window.alert

2 participants