Skip to content

[ML] Anomaly Explorer: Prevent crash on anomaly table filter #213075

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.

Sign up for GitHub

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
rbrtj merged 2 commits into from
Mar 6, 2025
Merged

[ML] Anomaly Explorer: Prevent crash on anomaly table filter #213075

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f9823b
initialize setCallback ref with a value
rbrtj Mar 4, 2025
999c405
Merge branch 'main' into ml-anomaly-explorer-crashes-after-applying-a…
rbrtj Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion x-pack/platform/packages/private/ml/url_state/src/url_state.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ export const useUrlStateService = <K extends Accessor, T>(
? state?.[optionsRef.current.pageKey]
: state;

const setCallback = useRef<typeof setState>();
const setCallback = useRef<typeof setState>(setState);
Copy link
Copy Markdown
Contributor

@peteharverson peteharverson Mar 4, 2025

Choose a reason for hiding this comment

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

This looks good, preventing the crashes which I have been seeing when changing the filter.

However looks like the URL is persisting the anomalies table page index when changing properties of the view, such as the selected job ID(s). I think we should reset the page index to 0 when changing the selected job(s). @rbrtj @darnautov what do you think? I'd still recommend getting this fix in, and then potentially doing the reset in a follow-up unless it's a simple change.

Copy link
Copy Markdown
Contributor Author

@rbrtj rbrtj Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It should definitely reset the page index to 0 when changing the selected jobs, in my opinion.
Also, without this fix, the page would crash if we select a page in the anomalies table that is not available for a different job (..and then we change the job).

Copy link
Copy Markdown
Contributor

@peteharverson peteharverson Mar 6, 2025

Choose a reason for hiding this comment

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

As discussed offline, let's fix the page index reset to 0 in a separate PR, and get this fix for the page crash in first.


useEffect(() => {
setCallback.current = setState;
Expand Down