Skip to content

systemd: guard against currentIdentifiers being undefined #22038

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

Merged
merged 2 commits into from
May 27, 2025

Conversation

jelly
Copy link
Member

@jelly jelly commented May 22, 2025

The initial state of currentIdentifiers is undefined, while unlikely to be triggered as prependEntries() is called after some promises are done.

Adding a link to the logs page from cockpit-podman triggered and clicking it in our tests triggered:

TypeError: Cannot read properties of undefined (reading 'includes')"]

Not sure if this is a fix for this podman issue but it smells like it!

@jelly jelly requested a review from mvollmer May 22, 2025 07:42
@@ -168,7 +168,7 @@ export class JournalBox extends React.Component {
const serviceTag = entries[i].SYSLOG_IDENTIFIER;
this.renderer.prepend(entries[i]);
// Only update if the service is not yet known
if (serviceTag && !this.props.currentIdentifiers.includes(serviceTag))
if (serviceTag && this.props.currentIdentifiers && !this.props.currentIdentifiers.includes(serviceTag))
Copy link
Member

Choose a reason for hiding this comment

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

This looks fishy. Without knowing the context, I would expect a falsy currentIdentifiers to be treated like the empty list. I.e.:

!(this.props.currentIdentifiers || []).includes(serviceTag)

Copy link
Member

Choose a reason for hiding this comment

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

But it's probably better to initialize this.props.currentIdentifiers to [] explicitly, in the parent component.

@jelly jelly force-pushed the oops-teardown-debug branch from e54d97e to 29b9bfb Compare May 22, 2025 09:06
@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label May 22, 2025
@jelly jelly force-pushed the oops-teardown-debug branch 2 times, most recently from d55ece2 to 57fe620 Compare May 26, 2025 11:18
jelly added 2 commits May 27, 2025 10:51
The initial state of `currentIdentifiers` is undefined, while unlikely
to be triggered as prependEntries() is called after some promises are
done.

Adding a link to the logs page from cockpit-podman triggered and
clicking it in our tests triggered:

```
TypeError: Cannot read properties of undefined (reading 'includes')"]
```
This issue has been uncovered by adding a link to the logs page from
cockpit-podman. Once clicking on it the initial state is undefined and
the ServiceDetails page raises an exception:

> error: TypeError: Cannot read properties of undefined (reading 'includes')
> error: The above error occurred in the <ServiceDetails> component:

    at ServiceDetails (http://127.0.0.2:9091/cockpit/@localhost/system/services.js:38578:7)
@jelly jelly force-pushed the oops-teardown-debug branch from 3ded2b1 to 20bbfb4 Compare May 27, 2025 08:52
@jelly jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label May 27, 2025
@jelly jelly force-pushed the oops-teardown-debug branch from 20bbfb4 to 6fbe36d Compare May 27, 2025 08:53
@jelly jelly requested a review from mvollmer May 27, 2025 08:53
Copy link
Member

@mvollmer mvollmer left a comment

Choose a reason for hiding this comment

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

Thanks!

@jelly jelly merged commit ac0558c into cockpit-project:main May 27, 2025
87 of 88 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.

2 participants