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

Some cells/rows don't load properly in large spreadsheets #320

Open
okennedy opened this issue Sep 20, 2024 · 1 comment
Open

Some cells/rows don't load properly in large spreadsheets #320

okennedy opened this issue Sep 20, 2024 · 1 comment
Assignees
Labels
bug Something isn't working layer-ui An issue involving the UI layer
Milestone

Comments

@okennedy
Copy link
Contributor

Describe the bug
Certain rows and/or cells don't load properly in a spreadsheet (instead of cell contents, you get a spinner).

To Reproduce
The bug is transient. On a sufficiently large dataset, it seems to happen to a random set of cells each time.

  1. Open a spreadsheet
  2. Click inside the spreadsheet to activate editing mode.
  3. Scroll through the spreadsheet
  4. Sometimes, certain cells will continue to have spinners

Expected behavior
All cells load properly

@okennedy okennedy added bug Something isn't working layer-ui An issue involving the UI layer labels Sep 20, 2024
@okennedy okennedy added this to the Version 2.1 milestone Sep 20, 2024
@okennedy okennedy self-assigned this Sep 20, 2024
@okennedy
Copy link
Contributor Author

Hypothesis 99.999% confirmed: The problem appears to result from the backend pushing messages to the frontend on a cell-by-cell basis. On a sufficiently large (wide/tall) dataset, the outgoing message queue fills up, and the cells are dropped.

76a65bc added a tempoorary workaround, bumping the spreadsheet queue up to a sufficiently large size that this is unlikely to happen during normal scrolling. By my rough estimates, we should be able to handle a few hundred columns without the problem surfacing again.

That said, the correct way to solve this is to modify the backend to group messages into rows. The protocol supports row-level notification, but SingleRowExecutor operates at the granularity of individual cells. The specific code path of interest involves:

  • The user scrolls to a new region of the spreadsheet
  • The frontend subscribes to rows in the region.
  • The backend socket relays the subscription request, which eventually makes its way into SingleRowExecutor.subscribe
  • SingleRowExecutor.subscribe calls SingleRowExecutor.recompute for each individual cell.
  • SingleRowExecutor.recompute takes responsibility for notifying the frontend of 'changes' (i.e., the value of cells in the newly subscribed rows)

I'm not entirely sure what the best way to fix this is, but what I'm leaning towards right now is to have subscribe take responsibility for notifying the frontend. e.g., add a notify parameter to recompute (to disable notification) and have subscribe send a full row update once the cell values have been computed.

okennedy added a commit that referenced this issue Sep 22, 2024
- Increase the spreadsheet socket outgoing message buffer size.  100k slots should be enough to keep up with most scrolling through the spreadsheet.

The *correct* way to fix this, is to change SpreadsheetExecutor/SingleRowExecutor to support notifying on full-row updates, rather than on a cell-by-cell basis (which it does right now).  This shouldn't be too hard, since SingleRowExecutor operates at the granularity of rows anyway... but I want to have the time to do it right (which is not the case right now OK).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working layer-ui An issue involving the UI layer
Projects
None yet
Development

No branches or pull requests

1 participant