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

fix: raise valueError for pageSize > 200 #3472

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

Light2Dark
Copy link
Contributor

@Light2Dark Light2Dark commented Jan 16, 2025

📝 Summary

Fixes #3407 . This will raise an error for data_editor, dataframe and table when the page_size configured is > 200.
image

🔍 Description of Changes

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

Copy link

vercel bot commented Jan 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 11:54pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 11:54pm

Copy link

vercel bot commented Jan 16, 2025

@Light2Dark is attempting to deploy a commit to the marimo Team on Vercel.

A member of the Team first needs to authorize it.


def validate_page_size(page_size: int) -> None:
# We will need to support frontend row virtualization beyond this limit
if page_size > 200:
Copy link
Contributor

Choose a reason for hiding this comment

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

curios if this could be 500 or so? not sure where it starts causing issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with 50 cols & 500 rows, it gets a little jittery for me, but it's okay. I'm worried about older browsers / lack of cpu/ram users. The worst thing to do is cause a crash (and it's pretty hard to restart marimo, upon restart it tries to run again).

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for checking. agreed with your conclusion then.

@mscolnick mscolnick merged commit 7641d41 into marimo-team:main Jan 17, 2025
31 checks passed
Copy link

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.10.14-dev27

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.

webpage crashes with large mo.ui.table page_size
2 participants