Skip to content
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

[TableListView] Fix letters are skipped when typing fast #194009

Merged
merged 4 commits into from
Sep 30, 2024

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Sep 25, 2024

Summary

Fix #193879 - I noticed this buggy behaviour before but haven't looked into it closer. But React@18 highligted major problems with how state works in that component that must be fixed before the upgrade.
I think the largest issue that is hard to fix or workaround is that rebuidling a query from url string is async:

const updateQueryFromURL = async (text: string = '') => {
const updatedQuery = await buildQueryFromText(text);
dispatch({
type: 'onSearchQueryChange',
data: {
query: updatedQuery,
text,
},
});
};

And this doesn't work nicely with controlled input

This PR fixes #193879 by "degrading" the url syncing behavior to only picking up the initial state from the URL. I wasn't sure how to refactor nicely the current logic so that it doesn't lag, so decided to simplify the functionality assuming that the lost syncing is not very usefull anyway.

@Dosant Dosant changed the title only set initial url [TableListView] Fix letters are skipped when typing fast Sep 25, 2024
@Dosant Dosant added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Component:TableListView labels Sep 26, 2024
@Dosant Dosant marked this pull request as ready for review September 26, 2024 08:30
@Dosant Dosant requested a review from a team as a code owner September 26, 2024 08:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@Dosant Dosant requested a review from sebelga September 26, 2024 08:30
updateFilterFromURL(urlState.filter);
}, [urlState, buildQueryFromText, urlStateEnabled]);
updateQueryFromURL(initialUrlState.s);
updateSortFromURL(initialUrlState.sort);
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I re-read the code, I am surprise we call sometimes twice dispatch onTableChange. But I guess that's a tiny optimisation

Copy link
Contributor Author

@Dosant Dosant Sep 26, 2024

Choose a reason for hiding this comment

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

it could be a good optimizaiton now, but would be automatically batched with react@18
I'd not touch to not break for now

@@ -849,12 +848,10 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
});
}

if (data.page || !urlStateEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to remove the condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So now I want to always call dispatch directly, because it is the source of state now even when url is enabled.

previosly it was called only if there is a page change or if url state was disabled

@Dosant Dosant requested a review from sebelga September 26, 2024 14:44
@Dosant
Copy link
Contributor Author

Dosant commented Sep 30, 2024

@elasticmachine merge upstream

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally and works as expected 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
dashboard 473.7KB 473.5KB -116.0B
eventAnnotationListing 226.8KB 226.7KB -116.0B
filesManagement 120.5KB 120.4KB -115.0B
graph 413.1KB 413.0KB -116.0B
maps 3.0MB 3.0MB -115.0B
visualizations 315.7KB 315.6KB -115.0B
total -693.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Dosant Dosant merged commit 23effbe into elastic:main Sep 30, 2024
20 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11104190598

@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 30, 2024
…) (#194407)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[TableListView] Fix letters are skipped when typing fast
(#194009)](#194009)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Anton
Dosov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-30T10:24:09Z","message":"[TableListView]
Fix letters are skipped when typing fast
(#194009)","sha":"23effbedd4f88219b71088b34d8e9221527e7a91","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor","Component:TableListView"],"title":"[TableListView]
Fix letters are skipped when typing
fast","number":194009,"url":"https://github.com/elastic/kibana/pull/194009","mergeCommit":{"message":"[TableListView]
Fix letters are skipped when typing fast
(#194009)","sha":"23effbedd4f88219b71088b34d8e9221527e7a91"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194009","number":194009,"mergeCommit":{"message":"[TableListView]
Fix letters are skipped when typing fast
(#194009)","sha":"23effbedd4f88219b71088b34d8e9221527e7a91"}}]}]
BACKPORT-->

Co-authored-by: Anton Dosov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Component:TableListView React@18 release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TableListView] Letters are skipped when typing fast
5 participants