-
Notifications
You must be signed in to change notification settings - Fork 2.9k
luci-base: mark background poll XHRs with X-Ubus-No-Touch header #8916
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -514,6 +514,14 @@ | |
|
|
||
| const requestQueue = []; | ||
|
|
||
| /* Requests made within BACKGROUND_THRESHOLD ms of a user gesture are | ||
| * foreground (session-touching); all others are background. | ||
| */ | ||
| const BACKGROUND_THRESHOLD = 5000; | ||
| for (const type of ['click', 'submit', 'change', 'input', 'keydown', 'mousedown', 'touchstart', 'wheel']) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the PR description enumerates this list as "common user-interaction event types ( Generated by Claude Code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, updated the PR description to include |
||
| document.addEventListener(type, () => { Request.lastUserInteraction = Date.now() }, { capture: true, passive: true }); | ||
| } | ||
|
|
||
| /** | ||
| * Check whether a Request.options object is eligible to be queued for RPC | ||
| * batching. | ||
|
|
@@ -570,6 +578,8 @@ | |
| reqopt.content[i] = batch[i][0].content; | ||
| } | ||
|
|
||
| reqopt.background = batch.every(e => e[0].background); | ||
|
|
||
| requestQueue.length = 0; | ||
|
|
||
| Request.request(rpcBaseURL, reqopt).then(reply => { | ||
|
|
@@ -605,6 +615,8 @@ | |
|
|
||
| interceptors: [], | ||
|
|
||
| lastUserInteraction: 0, | ||
|
|
||
| /** | ||
| * Turn the given relative URL into an absolute URL if necessary. | ||
| * | ||
|
|
@@ -665,6 +677,11 @@ | |
| * @property {Object<string, string>} [header] | ||
| * Specifies HTTP headers to set for the request. | ||
| * | ||
| * @property {boolean} [background] | ||
| * Whether the request is a background poll that should not reset the | ||
| * server-side session idle timer. Determined automatically from | ||
| * recent user interaction; set explicitly to override. | ||
| * | ||
| * @property {function()} [progress] | ||
| * An optional request callback function which receives ProgressEvent | ||
| * instances as sole argument during the HTTP request transfer. | ||
|
|
@@ -689,6 +706,9 @@ | |
| * The resulting HTTP response. | ||
| */ | ||
| request(target, options) { | ||
| const background = options?.background ?? | ||
| (Date.now() - Request.lastUserInteraction) > BACKGROUND_THRESHOLD; | ||
|
|
||
| return Promise.resolve(target).then(url => { | ||
| const state = { xhr: new XMLHttpRequest(), url: this.expandURL(url), start: Date.now() }; | ||
| const opt = Object.assign({}, options, state); | ||
|
|
@@ -698,6 +718,9 @@ | |
|
|
||
| return new Promise((resolveFn, rejectFn) => { | ||
| opt.xhr.onreadystatechange = callback.bind(opt, resolveFn, rejectFn); | ||
|
|
||
| opt.background = background; | ||
|
|
||
| opt.method = String(opt.method ?? 'GET').toUpperCase(); | ||
|
|
||
| if ('query' in opt) { | ||
|
|
@@ -787,6 +810,9 @@ | |
| contenttype = opt.headers[header]; | ||
| } | ||
|
|
||
| if (opt.background && new URL(opt.url, location.href).origin == location.origin) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the same-origin restriction is new in this revision and is not mentioned in the commit message. The message says only "Fix this by setting an X-Ubus-No-Touch: 1 request header on XHRs that are not driven by a user gesture", which reads as unconditional; the PR body does document the CORS-preflight rationale, but the commit message is what survives into the tree. Worth one line, e.g. "The header is restricted to same-origin targets so cross-origin XHRs (attendedsysupgrade, repository key fetches) are not turned into preflighted requests." Generated by Claude Code |
||
| opt.xhr.setRequestHeader('X-Ubus-No-Touch', '1'); | ||
|
|
||
| if ('progress' in opt && 'upload' in opt.xhr) | ||
| opt.xhr.upload.addEventListener('progress', opt.progress); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the PR description still documents the superseded mechanism. It describes a
Request.backgroundboolean that capture-phase listeners clear "for the synchronous duration of the handler" with "a microtask [that] resets it totrueimmediately after", and claims the flag "is guaranteed to befalseonly while a user event handler is executing". None of that exists any more — this revision records a timestamp and compares it against a 5 s threshold, which is precisely what makes the async-continuation case (ui.changes.apply,uci.save) work. The commit message is accurate; the PR body is what carries over into the merge discussion, so it is worth syncing before merge.Generated by Claude Code