Skip to content

Conversation

@couvq
Copy link
Contributor

@couvq couvq commented Nov 11, 2025

Closes #242254
Closes #243566
Closes https://github.com/elastic/streams-program/issues/623
Closes https://github.com/elastic/observability-error-backlog/issues/318
Closes https://github.com/elastic/observability-error-backlog/issues/319

Description

This PR addresses missing input validation and error handling for the child/fork stream UIs and APIs, as well as a performance bug associated with the child stream input. Below is a summary of the cases this PR handles:

Test case Result
User tries creating empty child stream Help text is displayed letting the user know they can't create empty child streams, button is disabled
User tries creating child stream that is longer than 200 characters Help text is displayed letting the user know they can't create child streams that are longer than 200 characters, button is disabled
User tries creating child stream that contains uppercase characters An error is displayed letting the user know stream names cannot contain uppercase characters, button is disabled
User tries creating child stream that already exists An error is displayed letting the user know stream already exists, button is disabled
User tries creating child stream with a dot and the root child does not exist An error is displayed letting the user know they must create the root child first, button is disabled
User tries creating child stream with a dot and the root child exists An error is displayed letting the user know they must create the new stream from the root child with a link to the root child stream, button is disabled
User types in a valid stream name Button is enabled and the user is able to create the stream
(Performance) User types quickly in the input with data present in the data preview The input should be responsive with no lag

Note about what was causing the performance issue and the fix for it:

  • The onChange function passed to the <StreamNameFormRow /> component was being triggered on every key press. The onChange functions passed to the component updated contexts within the routing state machine and since the PreviewPanel component read the entire snapshot of state from the routing state machine, it caused the PreviewTable component to rerender along with all of its preview data, this caused the input to lag as React has to rerender the preview table along with the input and anything else that reads the routing state. I resolved this by having the component hook manage a local state, and debouncing what was passed to the onChange so that it would only get called after 300ms of inactivity. This way only the input is rerendered on key press, with the onChange causing downstream rerendering only after a period of inactivity. Now the input is snappy when typing and the client side validation happens quickly, even when data is rendered in the preview table.

Screen recording (works the same on manual and AI suggestion inputs)

Screen.Recording.2025-12-01.at.2.28.01.PM.mov

@couvq
Copy link
Contributor Author

couvq commented Nov 12, 2025

/ci

@couvq couvq force-pushed the 242254_child_stream_min_length branch from 780867a to b51e9ea Compare November 13, 2025 22:48
@couvq
Copy link
Contributor Author

couvq commented Nov 13, 2025

/ci

@couvq couvq force-pushed the 242254_child_stream_min_length branch from 6873ed5 to 5b37da3 Compare November 14, 2025 18:41
@couvq
Copy link
Contributor Author

couvq commented Nov 14, 2025

/ci

@couvq couvq marked this pull request as ready for review November 14, 2025 20:56
@couvq couvq requested review from a team as code owners November 14, 2025 20:56
@couvq couvq added release_note:fix backport:skip This PR does not require backporting Team:obs-onboarding Observability Onboarding Team v9.3.0 labels Nov 14, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-onboarding-team (Team:obs-onboarding)

@couvq couvq added Feature:Streams This is the label for the Streams Project backport:version Backport to applied version labels backport:skip This PR does not require backporting and removed backport:skip This PR does not require backporting backport:version Backport to applied version labels labels Nov 14, 2025
@couvq couvq force-pushed the 242254_child_stream_min_length branch 2 times, most recently from 6ca2248 to 9ebbf27 Compare November 21, 2025 01:03
@couvq couvq requested a review from a team as a code owner November 21, 2025 13:07
@gbamparop
Copy link
Contributor

Should the Save button on the processor be disabled when the form state is invalid?

@couvq
Copy link
Contributor Author

couvq commented Nov 24, 2025

Should the Save button on the processor be disabled when the form state is invalid?

@gbamparop I Asked the EUI team about this off thread (slack), I think it would make sense to disable the button here but want to make sure it aligns with any UX patterns they have. I couldn't find anything specific to this use case on the EUI doc site.

@mohamedhamed-ahmed
Copy link
Contributor

mohamedhamed-ahmed commented Nov 24, 2025

Should the Save button on the processor be disabled when the form state is invalid?

I think it makes sense to disable any save or update button when there is a validation error, otherwise we are making a backend call and relying on the backend validation to reject the request. Since we also disable it when there is a . in the name.

@mohamedhamed-ahmed
Copy link
Contributor

I remember we used to display an error message when it exceeds 200 chars, this no longer happens.

Screenshot 2025-11-24 at 20 42 27

@couvq
Copy link
Contributor Author

couvq commented Nov 24, 2025

I remember we used to display an error message when it exceeds 200 chars, this no longer happens.

Screenshot 2025-11-24 at 20 42 27

I think that is from the maxLength property that was set on the input

@mohamedhamed-ahmed
Copy link
Contributor

mohamedhamed-ahmed commented Nov 24, 2025

@couvq Sorry edited your comment instead of quoting it 😅.

I think that is from the maxLength property that was set on the input

We can probably rely on the custom validation we have to show the error message, otherwise the user won't know why no more typing is allowed.

@couvq
Copy link
Contributor Author

couvq commented Nov 24, 2025

@couvq Sorry edited your comment instead of quoting it 😅.

I think that is from the maxLength property that was set on the input

We can probably rely on the custom validation we have to show the error message, otherwise the user won't know why no more typing is allowed.

Cool that makes sense, I can remove that as part of this change as well 👍. Also the messages are shown as help messages in the current state. Should these be error messages instead?

@mohamedhamed-ahmed
Copy link
Contributor

Also the messages are shown as help messages in the current state. Should these be error messages instead?

I remember I had this discussion with Marco before and we decided on keeping the length messages as help ones not error. Lets keep it this way for now unless the Design team says otherwise

couvq added 25 commits December 3, 2025 09:11
…ourage reusability between manual child and ai suggested partitions
@couvq couvq force-pushed the 242254_child_stream_min_length branch from 66df4fe to 1db4d2b Compare December 3, 2025 14:59
@couvq
Copy link
Contributor Author

couvq commented Dec 3, 2025

/ci

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
streams 72 73 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
streamsApp 1.1MB 1.1MB +1.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
streams 9.2KB 9.3KB +47.0B
Unknown metric groups

API count

id before after diff
streams 75 76 +1

ESLint disabled line counts

id before after diff
streamsApp 18 19 +1

Total ESLint disabled count

id before after diff
streamsApp 28 29 +1

History

@couvq couvq marked this pull request as ready for review December 3, 2025 17:54
@couvq couvq requested a review from flash1293 December 3, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Streams This is the label for the Streams Project release_note:fix Team:obs-onboarding Observability Onboarding Team v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Streams] 🐞 AI suggestion edit mode allows '.' in stream name [Streams] Child Stream name should have minimum length

5 participants