From 6d562130c69543d6625368929f130cdc3ef86c13 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Fri, 21 Aug 2026 10:44:59 -0400 Subject: [PATCH 1/3] Show how much longer a rate limited batch has to run A throttled row said only that it was waiting, so a batch that was working correctly but slowly was indistinguishable from one that had hung. On a wiki allowing 8 uploads a minute, 100 files take over twelve minutes, and that is the right outcome rather than a fault -- but only if the user is told. The figure is not a measurement. The gate hands out one slot per interval and already knows when the next is due, so this is arithmetic on a schedule the client is itself enforcing: the wait left on the current backoff, plus one interval for every file queued behind it. Nothing is sampled, so nothing needs smoothing. Shown only while the wiki is actually throttling, because that is the only time a schedule exists. Before the first refusal there is nothing to estimate, and guessing from the advertised hits would assume a fresh budget, which is the one thing the API does not report. Whole minutes, rounded up, and "about". The estimate excludes the transfer itself and can grow if the wiki refuses again, so a ticking countdown would claim a precision it does not have. Rounding is also what keeps the live region quiet: the text changes at most once a minute rather than on every refresh. Verified against a live wiki -- a 128 second batch produced two distinct strings, not 128. The count comes from the batch limit rather than the upload queue. A file being retried has left the queue and not yet rejoined it, so the queue undercounts by one per retrying file at exactly the moment a refusal is being handled. Co-Authored-By: Claude Opus 5 (1M context) --- extension.json | 3 + i18n/en.json | 2 + i18n/qqq.json | 2 + release-notes.md | 1 + res/ext.SimpleBatchUpload/batchLimit.js | 5 ++ .../ext.SimpleBatchUpload.css | 5 ++ .../ext.SimpleBatchUpload.js | 49 +++++++++++++- res/ext.SimpleBatchUpload/rateLimitGate.js | 31 ++++++++- res/ext.SimpleBatchUpload/remainingTime.js | 57 +++++++++++++++++ .../ext.SimpleBatchUpload/batchLimit.test.js | 22 +++++++ .../rateLimitGate.test.js | 64 +++++++++++++++++++ .../remainingTime.test.js | 43 +++++++++++++ 12 files changed, 282 insertions(+), 2 deletions(-) create mode 100644 res/ext.SimpleBatchUpload/remainingTime.js create mode 100644 tests/vitest/ext.SimpleBatchUpload/remainingTime.test.js diff --git a/extension.json b/extension.json index af7f7a5..d83b748 100644 --- a/extension.json +++ b/extension.json @@ -65,6 +65,7 @@ "ext.SimpleBatchUpload/batchLimit.js", "ext.SimpleBatchUpload/rateLimitGate.js", "ext.SimpleBatchUpload/rateLimits.js", + "ext.SimpleBatchUpload/remainingTime.js", "ext.SimpleBatchUpload/renamePattern.js", "ext.SimpleBatchUpload/resultRow.js", "ext.SimpleBatchUpload/uploadQueue.js", @@ -82,6 +83,8 @@ ], "messages": [ "simplebatchupload-error-rename-pattern", + "simplebatchupload-estimate-minutes", + "simplebatchupload-estimate-under-a-minute", "simplebatchupload-max-files-reached", "simplebatchupload-rename-label", "simplebatchupload-result-error", diff --git a/i18n/en.json b/i18n/en.json index acdcb73..ae26375 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -10,6 +10,8 @@ "simplebatchupload-comment": "Uploaded with [[mw:Special:MyLanguage/Extension:SimpleBatchUpload|SimpleBatchUpload]]", "simplebatchupload-desc": "Allows for simple batch uploading of of files", "simplebatchupload-error-rename-pattern": "The +rename pattern is not a valid regular expression.", + "simplebatchupload-estimate-minutes": "About $1 {{PLURAL:$1|minute|minutes}} left at this wiki's upload rate limit.", + "simplebatchupload-estimate-under-a-minute": "Less than a minute left at this wiki's upload rate limit.", "simplebatchupload-max-files-reached": "Added $1 of $2 selected {{PLURAL:$2|file|files}}. No more can be added until the current uploads finish.", "simplebatchupload-name": "SimpleBatchUpload", "simplebatchupload-rename-label": "$1 → $2", diff --git a/i18n/qqq.json b/i18n/qqq.json index dd67552..df1a286 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -10,6 +10,8 @@ "simplebatchupload-comment": "Comment saved with the upload. Do not translate '[[mw:Special:MyLanguage/Extension:SimpleBatchUpload|SimpleBatchUpload]]'", "simplebatchupload-desc": "{{desc|name=SimpleBatchUpload|url=https://www.mediawiki.org/wiki/Extension:SimpleBatchUpload}}", "simplebatchupload-error-rename-pattern": "Reason shown when the +rename directive in the upload description is not a valid regular expression. Do not translate '+rename'. Used as $1 of {{msg-mw|simplebatchupload-result-error}}.", + "simplebatchupload-estimate-minutes": "Shown above the result list while the wiki is rate limiting the batch. Parameters:\n* $1 - a whole number of minutes, rounded up", + "simplebatchupload-estimate-under-a-minute": "Shown above the result list while the wiki is rate limiting the batch and less than a minute of waiting is left. Used instead of {{msg-mw|simplebatchupload-estimate-minutes}} so that no countdown in seconds is shown.", "simplebatchupload-max-files-reached": "Shown once in the result list when a file selection does not fit in the current batch. Parameters:\n* $1 - the number of files that were added\n* $2 - the number of files that were selected", "simplebatchupload-name": "{{Notranslate}} The name of the extesion as shown on special page 'Version'", "simplebatchupload-rename-label": "Label of a result row for a file that is renamed while uploading. Parameters:\n* $1 - the name of the selected file\n* $2 - the name it is uploaded under", diff --git a/release-notes.md b/release-notes.md index f70fbce..13fde26 100644 --- a/release-notes.md +++ b/release-notes.md @@ -8,6 +8,7 @@ * Changed uploading to pace itself to the rate limit the wiki advertises, once the wiki has refused an upload * Batches that fit inside the limit are unaffected and still upload at full speed * Batches on wikis with a long limit window now retry for much longer before giving up +* Added an estimate of how much longer a batch has left while the wiki is rate limiting it * Fixed files reported as uploaded when the wiki did not store them * Fixed an invalid `+rename` pattern cancelling the rest of the batch with no error shown * Fixed the result list losing uploads that were still running when more files were selected diff --git a/res/ext.SimpleBatchUpload/batchLimit.js b/res/ext.SimpleBatchUpload/batchLimit.js index b4bcb2c..ecc168f 100644 --- a/res/ext.SimpleBatchUpload/batchLimit.js +++ b/res/ext.SimpleBatchUpload/batchLimit.js @@ -50,6 +50,11 @@ function createBatchLimit( limit ) { admit: admit, release: release, remaining: () => Math.max( 0, limit - active ), + // Files admitted but not yet finished. Counted here rather than from + // the upload queue because a file being retried has left the queue and + // not yet rejoined it, so the queue undercounts exactly when a rate + // limit refusal is being handled. + active: () => active, limit: () => limit }; } diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css index 49a20cc..9bcb83b 100644 --- a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css +++ b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css @@ -42,3 +42,8 @@ ul.fileupload-results li.ful-error { ul.fileupload-results li.ful-notice { background-color: #fafad2; } + +ul.fileupload-results li.ful-estimate { + padding: 0.2em 1em; + font-style: italic; +} diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js index db0e995..5fbeb1c 100644 --- a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js +++ b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js @@ -18,6 +18,7 @@ const { createUploadQueue } = require( './uploadQueue.js' ); const { createUploadRunner } = require( './uploadRunner.js' ); const { createResultRow, pruneFinishedRows } = require( './resultRow.js' ); const { filePageUrl } = require( './uploadResult.js' ); +const { estimateRemainingMs, describeRemaining } = require( './remainingTime.js' ); // The rate limit is per user, so one gate and one queue serve every widget on // the page. blueimp's own limit is set to the same number as a backstop. @@ -44,6 +45,46 @@ $( () => { mw.config.get( 'wgUserGroups' ) ) ); + const resultLists = []; + + /** + * Shows how much longer the wiki's rate limit will hold the batch up. + * + * Refreshed only where its inputs change -- a file admitted, a file + * finished, an upload refused -- never on a timer. Because the text is + * whole minutes, it changes at most once a minute, which is what keeps the + * live region from announcing on every refresh. + */ + function refreshEstimate() { + const text = describeRemaining( + estimateRemainingMs( batchLimit.active(), gate.schedule() ) + ); + + resultLists.forEach( ( results ) => { + let row = results.querySelector( 'li.ful-estimate' ); + + if ( !text ) { + if ( row ) { + row.remove(); + } + + return; + } + + if ( !row ) { + row = document.createElement( 'li' ); + row.className = 'ful-estimate'; + row.setAttribute( 'role', 'status' ); + results.insertBefore( row, results.firstChild ); + } + + // Guarded, so an unchanged figure is not re-announced. + if ( row.textContent !== text ) { + row.textContent = text; + } + } ); + } + function appendNotice( results, text ) { const notice = document.createElement( 'li' ); notice.className = 'ful-notice'; @@ -96,7 +137,10 @@ $( () => { const outcome = await runner.run( () => data.submit(), - () => row.showWaiting(), + () => { + row.showWaiting(); + refreshEstimate(); + }, async () => { api.badToken( 'csrf' ); data.formData.token = await api.getToken( 'csrf' ); @@ -112,11 +156,13 @@ $( () => { } } finally { batchLimit.release(); + refreshEstimate(); } } function initContainer( container ) { const results = container.querySelector( 'ul.fileupload-results' ); + resultLists.push( results ); // blueimp calls add() once per file and hands every file of one // selection the same originalFiles array, which is how a new selection @@ -157,6 +203,7 @@ $( () => { } admitted += 1; + refreshEstimate(); startUpload( this, container, results, data ); }, diff --git a/res/ext.SimpleBatchUpload/rateLimitGate.js b/res/ext.SimpleBatchUpload/rateLimitGate.js index 35cd288..1f62a0b 100644 --- a/res/ext.SimpleBatchUpload/rateLimitGate.js +++ b/res/ext.SimpleBatchUpload/rateLimitGate.js @@ -89,13 +89,41 @@ function createRateLimitGate( options ) { * selection on the page for as long as the tab stayed open, including * batches small enough to fit comfortably. */ + /** + * @return {boolean} True once a full window has passed with nothing refused + */ + function refusalsAreStale() { + return pacing && now() - lastRefusalAt >= capMs; + } + function forgetStaleRefusals() { - if ( pacing && now() - lastRefusalAt >= capMs ) { + if ( refusalsAreStale() ) { pacing = false; nextReleaseAt = 0; } } + /** + * The schedule the gate is currently enforcing. + * + * Everything here is already computed to decide when to release the next + * upload; exposing it lets the UI say how much longer the batch has rather + * than only that it is waiting. + * + * @return {?{waitMs: number, intervalMs: number}} Null when nothing is + * being paced, so there is no wait to report + */ + function schedule() { + if ( halted || !pacing || refusalsAreStale() ) { + return null; + } + + return { + waitMs: Math.max( 0, Math.max( openAt, nextReleaseAt ) - now() ), + intervalMs: intervalMs + }; + } + /** * Adopts the limit the wiki advertises. * @@ -186,6 +214,7 @@ function createRateLimitGate( options ) { return { useLimit: useLimit, + schedule: schedule, wait: wait, noteRateLimited: noteRateLimited, noteProgress: noteProgress, diff --git a/res/ext.SimpleBatchUpload/remainingTime.js b/res/ext.SimpleBatchUpload/remainingTime.js new file mode 100644 index 0000000..b01d042 --- /dev/null +++ b/res/ext.SimpleBatchUpload/remainingTime.js @@ -0,0 +1,57 @@ +'use strict'; + +/** + * How much longer a rate limited batch has to run. + * + * This is not a measurement. The gate hands out one upload slot per interval + * and already knows when the next one is due, so the figure is arithmetic on a + * schedule the client is itself enforcing. That is why there is no smoothing + * here: there is no noisy throughput to smooth. + * + * It excludes the time the files themselves take to transfer, which is additive + * and matters for large ones, and it can grow if the wiki refuses again. Hence + * "about", and hence whole minutes rather than a countdown. + */ + +const MS_PER_MINUTE = 60000; + +/** + * @param {number} pending Files admitted to the batch that have not finished + * @param {?Object} schedule From the gate: { waitMs, intervalMs }, or null when + * nothing is paced + * @return {?number} Milliseconds, or null when there is nothing to estimate + */ +function estimateRemainingMs( pending, schedule ) { + if ( !schedule || pending < 1 ) { + return null; + } + + return schedule.waitMs + ( pending - 1 ) * schedule.intervalMs; +} + +/** + * @param {?number} ms From estimateRemainingMs() + * @return {?string} Message text, or null when there is nothing to say + */ +function describeRemaining( ms ) { + if ( ms === null || ms === undefined ) { + return null; + } + + if ( ms < MS_PER_MINUTE ) { + return mw.msg( 'simplebatchupload-estimate-under-a-minute' ); + } + + // Rounded up, so it never reads as no time left while files remain. Whole + // minutes are also what keeps the live region quiet: the text changes once + // a minute rather than on every refresh. + return mw.msg( + 'simplebatchupload-estimate-minutes', + Math.ceil( ms / MS_PER_MINUTE ) + ); +} + +module.exports = { + estimateRemainingMs: estimateRemainingMs, + describeRemaining: describeRemaining +}; diff --git a/tests/vitest/ext.SimpleBatchUpload/batchLimit.test.js b/tests/vitest/ext.SimpleBatchUpload/batchLimit.test.js index a0f7098..db73d8e 100644 --- a/tests/vitest/ext.SimpleBatchUpload/batchLimit.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/batchLimit.test.js @@ -46,3 +46,25 @@ describe( 'createBatchLimit', () => { expect( batch.admit() ).toBe( true ); } ); } ); + +describe( 'counting what is still in the batch', () => { + it( 'reports how many files are still queued or in flight', () => { + const batch = createBatchLimit( 10 ); + + batch.admit(); + batch.admit(); + batch.admit(); + batch.release(); + + expect( batch.active() ).toBe( 2 ); + } ); + + it( 'reports nothing left once every file has finished', () => { + const batch = createBatchLimit( 10 ); + + batch.admit(); + batch.release(); + + expect( batch.active() ).toBe( 0 ); + } ); +} ); diff --git a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js index d60f9d8..1131b62 100644 --- a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js @@ -263,3 +263,67 @@ describe( 'pacing stops once the budget has demonstrably refilled', () => { expect( clock.now() - before ).toBeGreaterThan( 0 ); } ); } ); + +describe( 'reporting the schedule it is enforcing', () => { + const EIGHT_PER_MINUTE = { intervalMs: 7500, windowMs: 60000 }; + + function limitedGate( clock, maxRetries ) { + return createRateLimitGate( { + now: clock.now, + sleep: clock.sleep, + limit: EIGHT_PER_MINUTE, + maxRetries: maxRetries + } ); + } + + it( 'offers no schedule before the wiki has refused an upload', () => { + expect( limitedGate( createFakeClock() ).schedule() ).toBeNull(); + } ); + + it( 'offers no schedule for a user the wiki does not limit', () => { + const clock = createFakeClock(); + const gate = createRateLimitGate( { now: clock.now, sleep: clock.sleep } ); + + gate.noteRateLimited(); + + expect( gate.schedule() ).toBeNull(); + } ); + + it( 'reports the wait left on the current backoff and the spacing behind it', () => { + const clock = createFakeClock(); + const gate = limitedGate( clock ); + + gate.noteRateLimited(); + + expect( gate.schedule() ).toEqual( { waitMs: 2000, intervalMs: 7500 } ); + } ); + + it( 'counts the wait down as time passes', () => { + const clock = createFakeClock(); + const gate = limitedGate( clock ); + + gate.noteRateLimited(); + clock.advance( 1500 ); + + expect( gate.schedule().waitMs ).toBe( 500 ); + } ); + + it( 'offers no schedule once a full window has passed without a refusal', () => { + const clock = createFakeClock(); + const gate = limitedGate( clock ); + + gate.noteRateLimited(); + clock.advance( 120000 ); + + expect( gate.schedule() ).toBeNull(); + } ); + + it( 'offers no schedule once the batch has been given up on', () => { + const clock = createFakeClock(); + const gate = limitedGate( clock, 0 ); + + gate.noteRateLimited(); + + expect( gate.schedule() ).toBeNull(); + } ); +} ); diff --git a/tests/vitest/ext.SimpleBatchUpload/remainingTime.test.js b/tests/vitest/ext.SimpleBatchUpload/remainingTime.test.js new file mode 100644 index 0000000..079ec05 --- /dev/null +++ b/tests/vitest/ext.SimpleBatchUpload/remainingTime.test.js @@ -0,0 +1,43 @@ +const { + estimateRemainingMs, + describeRemaining +} = require( '../../../res/ext.SimpleBatchUpload/remainingTime.js' ); + +const PACED = { waitMs: 10000, intervalMs: 5000 }; + +describe( 'estimateRemainingMs', () => { + it( 'counts only the current wait when one file is left', () => { + expect( estimateRemainingMs( 1, PACED ) ).toBe( 10000 ); + } ); + + it( 'adds one interval for every file queued behind the next release', () => { + expect( estimateRemainingMs( 4, PACED ) ).toBe( 10000 + 3 * 5000 ); + } ); + + it( 'says nothing while the wiki has refused nothing', () => { + // No schedule means no pacing, so there is no wait to describe. + expect( estimateRemainingMs( 20, null ) ).toBeNull(); + } ); + + it( 'says nothing once no files are left', () => { + expect( estimateRemainingMs( 0, PACED ) ).toBeNull(); + } ); +} ); + +describe( 'describeRemaining', () => { + it( 'rounds up to whole minutes rather than inventing precision', () => { + expect( describeRemaining( 61000 ) ).toBe( 'simplebatchupload-estimate-minutes(2)' ); + } ); + + it( 'says less than a minute rather than counting seconds down', () => { + expect( describeRemaining( 4000 ) ).toBe( 'simplebatchupload-estimate-under-a-minute' ); + } ); + + it( 'never reads as no time left while files remain', () => { + expect( describeRemaining( 60000 ) ).toBe( 'simplebatchupload-estimate-minutes(1)' ); + } ); + + it( 'says nothing when there is nothing to estimate', () => { + expect( describeRemaining( null ) ).toBeNull(); + } ); +} ); From 8cdea9354c369b4153b0672f7f103eccc2a9f54a Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Fri, 21 Aug 2026 10:49:35 -0400 Subject: [PATCH 2/3] Trim the comments on the estimate Several explained the mechanism the code already shows, or repeated a rationale that belonged in one place. What remains is the four notes that stop someone making a plausible wrong change: why the count does not come from the upload queue, why there is no smoothing, why the figure is rounded up to whole minutes, and why the DOM write is guarded. Co-Authored-By: Claude Opus 5 (1M context) --- res/ext.SimpleBatchUpload/batchLimit.js | 6 ++---- .../ext.SimpleBatchUpload.js | 5 +---- res/ext.SimpleBatchUpload/rateLimitGate.js | 7 +------ res/ext.SimpleBatchUpload/remainingTime.js | 16 +++++----------- 4 files changed, 9 insertions(+), 25 deletions(-) diff --git a/res/ext.SimpleBatchUpload/batchLimit.js b/res/ext.SimpleBatchUpload/batchLimit.js index ecc168f..35ef0e8 100644 --- a/res/ext.SimpleBatchUpload/batchLimit.js +++ b/res/ext.SimpleBatchUpload/batchLimit.js @@ -50,10 +50,8 @@ function createBatchLimit( limit ) { admit: admit, release: release, remaining: () => Math.max( 0, limit - active ), - // Files admitted but not yet finished. Counted here rather than from - // the upload queue because a file being retried has left the queue and - // not yet rejoined it, so the queue undercounts exactly when a rate - // limit refusal is being handled. + // Admitted but not finished. Not queue.running() + queue.waiting(): a + // file being retried has left the queue and not yet rejoined it. active: () => active, limit: () => limit }; diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js index 5fbeb1c..6d6135c 100644 --- a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js +++ b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js @@ -50,10 +50,7 @@ $( () => { /** * Shows how much longer the wiki's rate limit will hold the batch up. * - * Refreshed only where its inputs change -- a file admitted, a file - * finished, an upload refused -- never on a timer. Because the text is - * whole minutes, it changes at most once a minute, which is what keeps the - * live region from announcing on every refresh. + * Refreshed where its inputs change, never on a timer. */ function refreshEstimate() { const text = describeRemaining( diff --git a/res/ext.SimpleBatchUpload/rateLimitGate.js b/res/ext.SimpleBatchUpload/rateLimitGate.js index 1f62a0b..ddb838b 100644 --- a/res/ext.SimpleBatchUpload/rateLimitGate.js +++ b/res/ext.SimpleBatchUpload/rateLimitGate.js @@ -106,12 +106,7 @@ function createRateLimitGate( options ) { /** * The schedule the gate is currently enforcing. * - * Everything here is already computed to decide when to release the next - * upload; exposing it lets the UI say how much longer the batch has rather - * than only that it is waiting. - * - * @return {?{waitMs: number, intervalMs: number}} Null when nothing is - * being paced, so there is no wait to report + * @return {?{waitMs: number, intervalMs: number}} Null when nothing is paced */ function schedule() { if ( halted || !pacing || refusalsAreStale() ) { diff --git a/res/ext.SimpleBatchUpload/remainingTime.js b/res/ext.SimpleBatchUpload/remainingTime.js index b01d042..4e1d075 100644 --- a/res/ext.SimpleBatchUpload/remainingTime.js +++ b/res/ext.SimpleBatchUpload/remainingTime.js @@ -3,14 +3,9 @@ /** * How much longer a rate limited batch has to run. * - * This is not a measurement. The gate hands out one upload slot per interval - * and already knows when the next one is due, so the figure is arithmetic on a - * schedule the client is itself enforcing. That is why there is no smoothing - * here: there is no noisy throughput to smooth. - * - * It excludes the time the files themselves take to transfer, which is additive - * and matters for large ones, and it can grow if the wiki refuses again. Hence - * "about", and hence whole minutes rather than a countdown. + * Arithmetic on the schedule the gate already enforces, not a measurement of + * throughput, so there is nothing here to smooth. It excludes transfer time and + * can grow if the wiki refuses again, hence "about" and whole minutes. */ const MS_PER_MINUTE = 60000; @@ -42,9 +37,8 @@ function describeRemaining( ms ) { return mw.msg( 'simplebatchupload-estimate-under-a-minute' ); } - // Rounded up, so it never reads as no time left while files remain. Whole - // minutes are also what keeps the live region quiet: the text changes once - // a minute rather than on every refresh. + // Rounded up so it never reads as no time left, and whole minutes keep the + // live region quiet: the text changes once a minute, not on every refresh. return mw.msg( 'simplebatchupload-estimate-minutes', Math.ceil( ms / MS_PER_MINUTE ) From 98193d861a2ef4226b9a95c08b1771b447d1665b Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Fri, 21 Aug 2026 11:07:40 -0400 Subject: [PATCH 3/3] Cover the estimate's riskiest paths, and keep the row a list item Three tests looked like they covered the new code and did not. Verified by mutation: deleting the halted guard from schedule(), dropping nextReleaseAt from the wait it reports, and adding ful-estimate to the pruned classes all left the suite green. The nextReleaseAt one mattered most. Every schedule test read the gate before any file had passed it, so the term was only ever exercised on the openAt side -- yet once the first backoff expires it is the only thing holding the next file up, which is where a paced batch spends nearly all its life. Under that mutation the readout would have said "less than a minute" for an entire batch with nothing to catch it. The create, update and remove logic moves to estimateRow.js so it can be tested under jsdom the way resultRow.js already is. It was the riskiest part of the change and the only part with no tests, purely because of where it lived. While extracting it: role="status" was on the
  • , which replaces its listitem role, so the list announced one fewer item than it had. The live region is now a span inside the row, which keeps both semantics. Also stops claiming a wait for a single file that is released immediately, moves a docblock the earlier split left on the wrong function, and drops a padding declaration the rule above already sets. Co-Authored-By: Claude Opus 5 (1M context) --- extension.json | 1 + res/ext.SimpleBatchUpload/estimateRow.js | 46 ++++++++++++ .../ext.SimpleBatchUpload.css | 1 - .../ext.SimpleBatchUpload.js | 30 ++------ res/ext.SimpleBatchUpload/rateLimitGate.js | 14 ++-- res/ext.SimpleBatchUpload/remainingTime.js | 9 ++- res/ext.SimpleBatchUpload/resultRow.js | 3 + .../ext.SimpleBatchUpload/estimateRow.test.js | 73 +++++++++++++++++++ .../rateLimitGate.test.js | 19 ++++- .../ext.SimpleBatchUpload/resultRow.test.js | 9 +++ 10 files changed, 170 insertions(+), 35 deletions(-) create mode 100644 res/ext.SimpleBatchUpload/estimateRow.js create mode 100644 tests/vitest/ext.SimpleBatchUpload/estimateRow.test.js diff --git a/extension.json b/extension.json index d83b748..2b42952 100644 --- a/extension.json +++ b/extension.json @@ -65,6 +65,7 @@ "ext.SimpleBatchUpload/batchLimit.js", "ext.SimpleBatchUpload/rateLimitGate.js", "ext.SimpleBatchUpload/rateLimits.js", + "ext.SimpleBatchUpload/estimateRow.js", "ext.SimpleBatchUpload/remainingTime.js", "ext.SimpleBatchUpload/renamePattern.js", "ext.SimpleBatchUpload/resultRow.js", diff --git a/res/ext.SimpleBatchUpload/estimateRow.js b/res/ext.SimpleBatchUpload/estimateRow.js new file mode 100644 index 0000000..315d514 --- /dev/null +++ b/res/ext.SimpleBatchUpload/estimateRow.js @@ -0,0 +1,46 @@ +'use strict'; + +/** + * The one row that says how much longer the wiki's rate limit will hold the + * batch up. Kept above the file rows, and taken away when there is nothing to + * say. + */ + +/** + * @param {HTMLElement} list The ul.fileupload-results element + * @param {?string} text Null to take the row away + */ +function showEstimate( list, text ) { + let row = list.querySelector( 'li.ful-estimate' ); + + if ( !text ) { + if ( row ) { + row.remove(); + } + + return; + } + + if ( !row ) { + row = document.createElement( 'li' ); + row.className = 'ful-estimate'; + + // The live region is a child rather than the
  • itself: role="status" + // on the
  • would replace its listitem role, and the list would + // announce one fewer item than it has. + const region = document.createElement( 'span' ); + region.setAttribute( 'role', 'status' ); + row.appendChild( region ); + + list.insertBefore( row, list.firstChild ); + } + + const announced = row.firstChild; + + // Rewriting an unchanged live region announces it again. + if ( announced.textContent !== text ) { + announced.textContent = text; + } +} + +module.exports = { showEstimate: showEstimate }; diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css index 9bcb83b..0d67b35 100644 --- a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css +++ b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.css @@ -44,6 +44,5 @@ ul.fileupload-results li.ful-notice { } ul.fileupload-results li.ful-estimate { - padding: 0.2em 1em; font-style: italic; } diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js index 6d6135c..c4a2564 100644 --- a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js +++ b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js @@ -19,6 +19,7 @@ const { createUploadRunner } = require( './uploadRunner.js' ); const { createResultRow, pruneFinishedRows } = require( './resultRow.js' ); const { filePageUrl } = require( './uploadResult.js' ); const { estimateRemainingMs, describeRemaining } = require( './remainingTime.js' ); +const { showEstimate } = require( './estimateRow.js' ); // The rate limit is per user, so one gate and one queue serve every widget on // the page. blueimp's own limit is set to the same number as a backstop. @@ -50,36 +51,17 @@ $( () => { /** * Shows how much longer the wiki's rate limit will hold the batch up. * - * Refreshed where its inputs change, never on a timer. + * Refreshed where its inputs change -- a file admitted, an upload refused, + * a file finished -- and never on a timer. */ function refreshEstimate() { const text = describeRemaining( estimateRemainingMs( batchLimit.active(), gate.schedule() ) ); - resultLists.forEach( ( results ) => { - let row = results.querySelector( 'li.ful-estimate' ); - - if ( !text ) { - if ( row ) { - row.remove(); - } - - return; - } - - if ( !row ) { - row = document.createElement( 'li' ); - row.className = 'ful-estimate'; - row.setAttribute( 'role', 'status' ); - results.insertBefore( row, results.firstChild ); - } - - // Guarded, so an unchanged figure is not re-announced. - if ( row.textContent !== text ) { - row.textContent = text; - } - } ); + // Gate, queue and batch limit are page-wide, so every widget shows the + // same figure. + resultLists.forEach( ( results ) => showEstimate( results, text ) ); } function appendNotice( results, text ) { diff --git a/res/ext.SimpleBatchUpload/rateLimitGate.js b/res/ext.SimpleBatchUpload/rateLimitGate.js index ddb838b..7477d10 100644 --- a/res/ext.SimpleBatchUpload/rateLimitGate.js +++ b/res/ext.SimpleBatchUpload/rateLimitGate.js @@ -81,6 +81,13 @@ function createRateLimitGate( options ) { let nextReleaseAt = 0; let lastRefusalAt = 0; + /** + * @return {boolean} True once a full window has passed with nothing refused + */ + function refusalsAreStale() { + return pacing && now() - lastRefusalAt >= capMs; + } + /** * Stops pacing once the wiki has gone a full window without refusing * anything, because by then whatever was exhausted has refilled. @@ -89,13 +96,6 @@ function createRateLimitGate( options ) { * selection on the page for as long as the tab stayed open, including * batches small enough to fit comfortably. */ - /** - * @return {boolean} True once a full window has passed with nothing refused - */ - function refusalsAreStale() { - return pacing && now() - lastRefusalAt >= capMs; - } - function forgetStaleRefusals() { if ( refusalsAreStale() ) { pacing = false; diff --git a/res/ext.SimpleBatchUpload/remainingTime.js b/res/ext.SimpleBatchUpload/remainingTime.js index 4e1d075..797d4dc 100644 --- a/res/ext.SimpleBatchUpload/remainingTime.js +++ b/res/ext.SimpleBatchUpload/remainingTime.js @@ -21,7 +21,11 @@ function estimateRemainingMs( pending, schedule ) { return null; } - return schedule.waitMs + ( pending - 1 ) * schedule.intervalMs; + const remaining = schedule.waitMs + ( pending - 1 ) * schedule.intervalMs; + + // A single file released immediately is not being held up by anything, so + // there is nothing to announce. + return remaining > 0 ? remaining : null; } /** @@ -37,7 +41,8 @@ function describeRemaining( ms ) { return mw.msg( 'simplebatchupload-estimate-under-a-minute' ); } - // Rounded up so it never reads as no time left, and whole minutes keep the + // Rounded up because the figure excludes transfer time and so is already an + // underestimate; ceil keeps it an upper bound. Whole minutes also keep the // live region quiet: the text changes once a minute, not on every refresh. return mw.msg( 'simplebatchupload-estimate-minutes', diff --git a/res/ext.SimpleBatchUpload/resultRow.js b/res/ext.SimpleBatchUpload/resultRow.js index 0380d2d..324360d 100644 --- a/res/ext.SimpleBatchUpload/resultRow.js +++ b/res/ext.SimpleBatchUpload/resultRow.js @@ -169,6 +169,9 @@ function createResultRow( sourceName, targetName ) { // A row is finished once it carries one of these. Anything else is still // queued, waiting on the rate limit, or uploading. +// +// ful-estimate is deliberately absent: it is not an upload row, and it has to +// survive a new selection started while an earlier batch is still going. const FINISHED_ROW_CLASSES = [ 'ful-success', 'ful-error', 'ful-notice' ]; /** diff --git a/tests/vitest/ext.SimpleBatchUpload/estimateRow.test.js b/tests/vitest/ext.SimpleBatchUpload/estimateRow.test.js new file mode 100644 index 0000000..25c9e0e --- /dev/null +++ b/tests/vitest/ext.SimpleBatchUpload/estimateRow.test.js @@ -0,0 +1,73 @@ +const { showEstimate } = require( '../../../res/ext.SimpleBatchUpload/estimateRow.js' ); + +function list() { + return document.createElement( 'ul' ); +} + +function estimateIn( ul ) { + return ul.querySelector( 'li.ful-estimate' ); +} + +describe( 'showEstimate', () => { + it( 'shows the text above the rows', () => { + const ul = list(); + ul.appendChild( document.createElement( 'li' ) ); + + showEstimate( ul, 'About 3 minutes left.' ); + + expect( estimateIn( ul ).textContent ).toBe( 'About 3 minutes left.' ); + expect( ul.firstChild.className ).toBe( 'ful-estimate' ); + } ); + + it( 'keeps the row a list item and announces from inside it', () => { + const ul = list(); + + showEstimate( ul, 'About 3 minutes left.' ); + + // role="status" on the
  • itself would replace its listitem role and + // the list would announce one fewer item than it has. + expect( estimateIn( ul ).getAttribute( 'role' ) ).toBeNull(); + expect( estimateIn( ul ).querySelector( '[role="status"]' ) ).not.toBeNull(); + } ); + + it( 'updates the text in place rather than replacing the row', () => { + const ul = list(); + + showEstimate( ul, 'About 3 minutes left.' ); + const first = estimateIn( ul ); + showEstimate( ul, 'About 2 minutes left.' ); + + expect( estimateIn( ul ) ).toBe( first ); + expect( estimateIn( ul ).textContent ).toBe( 'About 2 minutes left.' ); + } ); + + it( 'leaves the text untouched when it has not changed', () => { + const ul = list(); + + showEstimate( ul, 'About 3 minutes left.' ); + const announced = estimateIn( ul ).querySelector( '[role="status"]' ); + const before = announced.textContent; + showEstimate( ul, 'About 3 minutes left.' ); + + // Rewriting an unchanged live region announces it again. + expect( estimateIn( ul ).querySelector( '[role="status"]' ) ).toBe( announced ); + expect( announced.textContent ).toBe( before ); + } ); + + it( 'takes the row away once there is nothing left to say', () => { + const ul = list(); + + showEstimate( ul, 'About 3 minutes left.' ); + showEstimate( ul, null ); + + expect( estimateIn( ul ) ).toBeNull(); + } ); + + it( 'does nothing when there was never anything to say', () => { + const ul = list(); + + showEstimate( ul, null ); + + expect( ul.children.length ).toBe( 0 ); + } ); +} ); diff --git a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js index 1131b62..c5ef6a3 100644 --- a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js @@ -320,10 +320,27 @@ describe( 'reporting the schedule it is enforcing', () => { it( 'offers no schedule once the batch has been given up on', () => { const clock = createFakeClock(); - const gate = limitedGate( clock, 0 ); + const gate = limitedGate( clock, 1 ); + gate.noteRateLimited(); + expect( gate.schedule() ).not.toBeNull(); + + // Past openAt, or the second refusal is dismissed as the same overrun. + clock.advance( 2000 ); gate.noteRateLimited(); expect( gate.schedule() ).toBeNull(); } ); + + it( 'reports the spacing to the next slot once the backoff has expired', async () => { + const clock = createFakeClock(); + const gate = limitedGate( clock ); + + gate.noteRateLimited(); + await gate.wait(); + + // openAt is now in the past; the spacing behind the slot just claimed + // is the only thing left holding the next file up. + expect( gate.schedule() ).toEqual( { waitMs: 7500, intervalMs: 7500 } ); + } ); } ); diff --git a/tests/vitest/ext.SimpleBatchUpload/resultRow.test.js b/tests/vitest/ext.SimpleBatchUpload/resultRow.test.js index afdced4..8736d5d 100644 --- a/tests/vitest/ext.SimpleBatchUpload/resultRow.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/resultRow.test.js @@ -106,6 +106,15 @@ describe( 'pruneFinishedRows', () => { expect( list.children.length ).toBe( 0 ); } ); + it( 'keeps the rate limit estimate, which is not an upload row', () => { + const list = listWith( [ 'ful-estimate', 'ful-success' ] ); + + pruneFinishedRows( list ); + + expect( list.children.length ).toBe( 1 ); + expect( list.children[ 0 ].className ).toBe( 'ful-estimate' ); + } ); + it( 'keeps rows whose upload is still running', () => { const list = listWith( [ 'ful-success', '', 'ful-error' ] );