Skip to content

Limit batch interface to 100 items in UI and 10,000 via API #1213

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

Merged
merged 5 commits into from
Jun 3, 2025

Conversation

BenjaminCharmes
Copy link
Contributor

@BenjaminCharmes BenjaminCharmes commented May 22, 2025

Closes #1070

window.alert() will be replaced with new composant from #1212

Copy link

codecov bot commented May 22, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.73%. Comparing base (57652b8) to head (911510e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pydatalab/src/pydatalab/routes/v0_1/items.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1213      +/-   ##
==========================================
- Coverage   71.73%   71.73%   -0.01%     
==========================================
  Files          66       66              
  Lines        4479     4482       +3     
==========================================
+ Hits         3213     3215       +2     
- Misses       1266     1267       +1     
Files with missing lines Coverage Δ
pydatalab/src/pydatalab/config.py 82.26% <100.00%> (+0.12%) ⬆️
pydatalab/src/pydatalab/routes/v0_1/items.py 81.76% <50.00%> (-0.19%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

cypress bot commented May 22, 2025

datalab    Run #3412

Run Properties:  status check passed Passed #3412  •  git commit e1c1abcefa ℹ️: Merge fdfc74a23c229d34ae2da757c0704453d94d5ca9 into 57652b86cafd4fadf350817d2241...
Project datalab
Branch Review bc/limit-batch
Run status status check passed Passed #3412
Run duration 08m 00s
Commit git commit e1c1abcefa ℹ️: Merge fdfc74a23c229d34ae2da757c0704453d94d5ca9 into 57652b86cafd4fadf350817d2241...
Committer Ben Charmes
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 504
View all changes introduced in this branch ↗︎

@BenjaminCharmes BenjaminCharmes marked this pull request as ready for review May 22, 2025 13:30
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Thanks for having a go at this @BenjaminCharmes!

I think the overall problem is more that once you type in 1000, the UI makes 1000 rows which is mega slow, so we need the validation to happen before that step (apologies if this code does indeed do that, I can't test locally right now). I think due to the way we're binding the value from the input, it ignores the already set max, so it might just be a case of preventing the rows from being rendered until the number in the box is valid.

@BenjaminCharmes
Copy link
Contributor Author

Thanks for having a go at this @BenjaminCharmes!

I think the overall problem is more that once you type in 1000, the UI makes 1000 rows which is mega slow, so we need the validation to happen before that step (apologies if this code does indeed do that, I can't test locally right now). I think due to the way we're binding the value from the input, it ignores the already set max, so it might just be a case of preventing the rows from being rendered until the number in the box is valid.

Yes, that's right — with this PR, it works as expected: the UI doesn't allow rendering more than 100 rows. So even if you type in a higher number like 1000, it won't actually render more than 100 rows, display an error message and disable Submit button.

@ml-evs ml-evs added the on-hold label May 23, 2025
@ml-evs ml-evs removed the on-hold label Jun 3, 2025
@ml-evs ml-evs self-requested a review June 3, 2025 09:49
@ml-evs
Copy link
Member

ml-evs commented Jun 3, 2025

pre-commit.ci autofix

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Works well, thanks @BenjaminCharmes!

@ml-evs ml-evs changed the title Limit batch interface to 100 items Limit batch interface to 100 items in UI and 10,000 via API Jun 3, 2025
@ml-evs ml-evs added webapp For issues/PRs pertaining to the web interface usability labels Jun 3, 2025
@ml-evs ml-evs enabled auto-merge (squash) June 3, 2025 21:23
@ml-evs ml-evs merged commit 112f5c6 into main Jun 3, 2025
17 checks passed
@ml-evs ml-evs deleted the bc/limit-batch branch June 3, 2025 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usability webapp For issues/PRs pertaining to the web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit batch interface to reasonable number (100?)
2 participants