Conversation
There was a problem hiding this comment.
Code Review
This pull request adds iOS support via Capacitor, implementing Fetch and XHR interceptors to handle API requests on the capacitor: scheme and enhancing mobile touch interactions for notes and checkboxes. The review feedback focuses on refining the XMLHttpRequest polyfill for better specification compliance and efficiency, specifically regarding state transitions, event lifecycle, lazy decoding, and header formatting.
| open(method: string, url: string | URL, async?: boolean, user?: string | null, password?: string | null) { | ||
| const urlStr = typeof url === "string" ? url : url.href; | ||
| const abs = new URL(urlStr, location.href); | ||
| this._ti_method = method; | ||
| this._ti_url = abs.href; | ||
| this._ti_intercept = abs.origin === location.origin && isLocalApiRequest(abs); | ||
| this._ti_headers = {}; | ||
| if (!this._ti_intercept) { | ||
| return super.open(method, url as string, async ?? true, user ?? null, password ?? null); | ||
| } | ||
| } |
There was a problem hiding this comment.
When intercepting the XHR open call, the readyState should be transitioned to 1 (OPENED) and a readystatechange event should be dispatched to remain consistent with the standard XMLHttpRequest state machine. Many libraries (like jQuery) rely on these state transitions to manage their internal request lifecycle.
open(method: string, url: string | URL, async?: boolean, user?: string | null, password?: string | null) {
const urlStr = typeof url === "string" ? url : url.href;
const abs = new URL(urlStr, location.href);
this._ti_method = method;
this._ti_url = abs.href;
this._ti_intercept = abs.origin === location.origin && isLocalApiRequest(abs);
this._ti_headers = {};
if (this._ti_intercept) {
Object.defineProperty(this, "readyState", { value: 1, configurable: true });
this.dispatchEvent(new Event("readystatechange"));
return;
}
return super.open(method, url as string, async ?? true, user ?? null, password ?? null);
}| send(body?: Document | XMLHttpRequestBodyInit | null) { | ||
| if (!this._ti_intercept) return super.send(body as any); |
There was a problem hiding this comment.
The loadstart event should be dispatched when send() is called, as per the XMLHttpRequest specification. This is important for progress tracking in many client-side libraries.
| send(body?: Document | XMLHttpRequestBodyInit | null) { | |
| if (!this._ti_intercept) return super.send(body as any); | |
| send(body?: Document | XMLHttpRequestBodyInit | null) { | |
| if (!this._ti_intercept) return super.send(body as any); | |
| this.dispatchEvent(new ProgressEvent("loadstart")); |
| const buffer = await resp.arrayBuffer(); | ||
| const text = new TextDecoder().decode(buffer); | ||
|
|
||
| let parsedResponse: unknown = text; | ||
| if (this._ti_responseType === "json") { | ||
| try { parsedResponse = JSON.parse(text); } catch { parsedResponse = null; } | ||
| } else if (this._ti_responseType === "arraybuffer") { | ||
| parsedResponse = buffer; | ||
| } else if (this._ti_responseType === "blob") { | ||
| parsedResponse = new Blob([buffer], { type: resp.headers.get("content-type") ?? "" }); | ||
| } |
There was a problem hiding this comment.
Decoding the entire response buffer as text unconditionally is inefficient, especially for large binary responses (like images or attachments) where responseType is set to 'blob' or 'arraybuffer'. This can lead to significant CPU usage and potential memory issues. It's better to decode the text lazily or only when the responseType requires it (e.g., 'json', 'text', or default).
| const buffer = await resp.arrayBuffer(); | |
| const text = new TextDecoder().decode(buffer); | |
| let parsedResponse: unknown = text; | |
| if (this._ti_responseType === "json") { | |
| try { parsedResponse = JSON.parse(text); } catch { parsedResponse = null; } | |
| } else if (this._ti_responseType === "arraybuffer") { | |
| parsedResponse = buffer; | |
| } else if (this._ti_responseType === "blob") { | |
| parsedResponse = new Blob([buffer], { type: resp.headers.get("content-type") ?? "" }); | |
| } | |
| const buffer = await resp.arrayBuffer(); | |
| let textCache: string | undefined; | |
| const getText = () => { | |
| if (textCache === undefined) textCache = new TextDecoder().decode(buffer); | |
| return textCache; | |
| }; | |
| let parsedResponse: unknown = null; | |
| if (this._ti_responseType === "json") { | |
| try { parsedResponse = JSON.parse(getText()); } catch { parsedResponse = null; } | |
| } else if (this._ti_responseType === "arraybuffer") { | |
| parsedResponse = buffer; | |
| } else if (this._ti_responseType === "blob") { | |
| parsedResponse = new Blob([buffer], { type: resp.headers.get("content-type") ?? "" }); | |
| } else if (this._ti_responseType === "text" || this._ti_responseType === "") { | |
| parsedResponse = getText(); | |
| } |
| const headerLines = [...resp.headers.entries()] | ||
| .map(([k, v]) => `${k}: ${v}`).join("\r\n"); |
There was a problem hiding this comment.
The getAllResponseHeaders implementation is not fully compliant with the XMLHttpRequest specification. According to the spec, each header line (including the last one) must be terminated by a CRLF (\r\n). The current implementation using join("\r\n") will omit the CRLF on the final line.
| const headerLines = [...resp.headers.entries()] | |
| .map(([k, v]) => `${k}: ${v}`).join("\r\n"); | |
| const headerLines = [...resp.headers.entries()] | |
| .map(([k, v]) => `${k}: ${v}\r\n`).join(""); |
| Object.defineProperty(this, "responseText", { | ||
| get: () => { | ||
| if (this._ti_responseType && this._ti_responseType !== "text") { | ||
| throw new DOMException( | ||
| "responseText is only available when responseType is '' or 'text'", | ||
| "InvalidStateError" | ||
| ); | ||
| } | ||
| return text; | ||
| }, | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
The responseText getter should use the lazy decoding logic to avoid redundant work and ensure consistency with the response property.
| Object.defineProperty(this, "responseText", { | |
| get: () => { | |
| if (this._ti_responseType && this._ti_responseType !== "text") { | |
| throw new DOMException( | |
| "responseText is only available when responseType is '' or 'text'", | |
| "InvalidStateError" | |
| ); | |
| } | |
| return text; | |
| }, | |
| configurable: true, | |
| }); | |
| Object.defineProperty(this, "responseText", { | |
| get: () => { | |
| if (this._ti_responseType && this._ti_responseType !== "text") { | |
| throw new DOMException( | |
| "responseText is only available when responseType is '' or 'text'", | |
| "InvalidStateError" | |
| ); | |
| } | |
| return getText(); | |
| }, | |
| configurable: true, | |
| }); |
|
🖥️ App preview is ready! 🔗 Preview URL: https://pr-9539.trilium-app.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
No description provided.