[FOEPD-3391] Implement ONNX model weight download, IndexedDB caching#7199
Open
[FOEPD-3391] Implement ONNX model weight download, IndexedDB caching#7199
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds an IndexedDB-backed model weights cache and a resilient fetch helper with retries, timeout, and streaming progress reporting ( Sequence Diagram(s)sequenceDiagram
participant Client
participant BAP as BrowserAnnotationProvider
participant Worker
participant Cache as modelCache
participant IDB as IndexedDB
participant Net as Network
Client->>BAP: initialize(onProgress,onWarning)
BAP->>BAP: store callbacks
BAP->>Worker: postMessage(loadModel request)
Worker->>Cache: loadModelWeights(url, onProgress, onWarning)
Cache->>IDB: openDB()
IDB-->>Cache: db handle or error
alt DB open & cache hit
Cache->>IDB: idbGet(key)
IDB-->>Cache: ArrayBuffer
Cache->>Worker: return ArrayBuffer (invoke onProgress(size,size))
else DB open failure or cache miss
Cache->>Worker: postWarning(message) if DB open/read error
Cache->>Net: fetchWithProgress(url) (retries, timeout)
Net-->>Cache: stream chunks / final response
Cache->>Worker: invoke onProgress(loaded,total) per chunk
Cache->>IDB: idbPut(key, buffer) (warn on failure)
Cache-->>Worker: return ArrayBuffer
end
Worker->>BAP: postNotification(progress/warning)
BAP->>Client: call onProgress/onWarning callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
yrahal
changed the title
Add modelCache that downloads encoder and decoder weights with streaming progress reporting and retry logic, then caches the weights in IndexedDB. On subsequent visits, weights are directly served from cache. Worker now uses loadModelWeights instead of bare fetch, and forwards progress/warning notifications to the main thread via typed postMessage helpers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/BrowserAnnotationProvider.ts`:
- Around line 26-28: The implementation of BrowserAnnotationProvider.initialize
accepts optional onProgress and onWarning callbacks but the AnnotationProvider
interface's initialize(): Promise<void> has no parameters, causing a type-safety
gap; either (1) update the AnnotationProvider interface in types.ts to declare
initialize(onProgress?: DownloadProgressCallback, onWarning?: WarningCallback):
Promise<void> (import/declare the DownloadProgressCallback and WarningCallback
types there) so the public contract matches
BrowserAnnotationProvider.initialize, or (2) remove the extra parameters from
BrowserAnnotationProvider.initialize (and any callers that rely on them) so its
signature conforms to the existing interface; target the initialize method in
BrowserAnnotationProvider and the AnnotationProvider interface in types.ts when
making the change.
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Around line 70-75: The code treats a missing Content-Length as total = 0 which
causes progress callbacks to receive (loaded, 0) and can lead to
division-by-zero in consumers; change the logic so a missing/unknown
content-length is represented as undefined (or null) and only invoke
onProgress-based percentage calculations when total is a positive number.
Concretely: replace Number(response.headers.get("content-length")) || 0 with
logic that sets total = Number(...) only if the header exists and >0 (otherwise
total = undefined), keep the existing fast-path return when !onProgress ||
!response.body, and in the streaming loop (the code using response.body and
onProgress) guard calls that compute percentages so they run only if total is a
positive number (otherwise either call onProgress with loaded and undefined or
skip calling it entirely).
- Around line 116-147: The loadModelWeights function can leak an open
IDBDatabase handle if openDB() succeeds but fetchWithProgress(url, onProgress)
throws; update loadModelWeights so any opened db is always closed: after setting
db = await openDB(), ensure subsequent operations (cache read,
fetchWithProgress, idbPut) are wrapped so db.close() is invoked in a finally
block (or otherwise guaranteed) when db is non-null; specifically adjust the
control flow around openDB(), idbGet, fetchWithProgress, and idbPut to call
db.close() in a finally section so no handles remain open on errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4c9e5c7-1727-4d03-a087-fddbeaca7ce5
📒 Files selected for processing (5)
app/packages/annotation/src/providers/BrowserAnnotationProvider.tsapp/packages/annotation/src/providers/modelCache.test.tsapp/packages/annotation/src/providers/modelCache.tsapp/packages/annotation/src/providers/types.tsapp/packages/annotation/src/providers/worker.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Line 104: The code currently does `throw lastError;` which can be null when
MAX_RETRIES <= 0; update the retry/failure handling in the modelCache.ts routine
that uses `lastError` and `MAX_RETRIES` so it always throws a proper Error
object—e.g., if `lastError` is falsy create and throw a new Error with a clear
message (or wrap the original `lastError`), ensuring the throwing site (the
variable `lastError` and the retry loop around `MAX_RETRIES`) never forwards a
null/undefined value to `throw`.
- Around line 123-132: The function leaks the IndexedDB handle on cache hits
because the early return after idbGet(url) bypasses the later finally that
closes the DB; refactor the function so the entire body that uses the local db
(the openDB() call, idbGet(), onProgress/onWarning usage and the subsequent
download logic) is wrapped in a single try...finally and ensure the finally
always closes the db (the same db variable returned from openDB) so cached hits
also trigger closure; locate openDB and idbGet usage in modelCache.ts and
move/merge the existing cleanup logic into that single finally block to
guarantee db closure on every exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5a90d96-fc3b-40c4-807b-543f59f0b602
📒 Files selected for processing (1)
app/packages/annotation/src/providers/modelCache.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/packages/annotation/src/providers/modelCache.ts (1)
124-133:⚠️ Potential issue | 🔴 CriticalDB handle leak on cache hit remains unaddressed.
When
cachedis truthy, the function returns at line 129, bypassing thefinallyblock at lines 147-148 (which is scoped to the second try block). Every cache hit leaks an IDBDatabase handle.Wrap the entire function body in a single try-finally to ensure
db?.close()runs on all exit paths.🐛 Proposed fix: single try-finally wrapping entire body
export async function loadModelWeights( url: string, onProgress?: (loaded: number, total: number) => void, onWarning?: (message: string) => void ): Promise<ArrayBuffer> { let db: IDBDatabase | null = null; - try { - db = await openDB(); - const cached = await idbGet(db, url); - if (cached) { - onProgress?.(cached.byteLength, cached.byteLength); - return cached; - } - } catch (err) { - onWarning?.(`IndexedDB cache read failed, downloading instead: ${err}`); - } - try { - const buffer = await fetchWithProgress(url, onProgress); + try { + db = await openDB(); + const cached = await idbGet(db, url); + if (cached) { + onProgress?.(cached.byteLength, cached.byteLength); + return cached; + } + } catch (err) { + onWarning?.(`IndexedDB cache read failed, downloading instead: ${err}`); + } + const buffer = await fetchWithProgress(url, onProgress); if (db) { try { await idbPut(db, url, buffer); } catch (err) { onWarning?.(`IndexedDB cache write failed: ${err}`); } } - return buffer; } finally { db?.close(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/annotation/src/providers/modelCache.ts` around lines 124 - 133, The function leaks the IDBDatabase handle when a cache hit returns early because the current finally that calls db?.close() only wraps the later try block; refactor by wrapping the entire function body (including the initial openDB()/idbGet/cache-return block that references db, idbGet, onProgress, onWarning) in a single try...finally so that db?.close() is executed on all exit paths; ensure you keep the existing logic and error handling (openDB(), idbGet(), onProgress/onWarning) but move the finalizer that calls db?.close() into the single finally that encloses those operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/modelCache.test.ts`:
- Around line 5-41: Wrap the mock DB's no-op close in a spy and expose it from
createMockIDB so tests can assert the handle was closed: replace mockDB.close
with a function that calls a spy (e.g., closeSpy) and return closeSpy on the
object returned by createMockIDB; keep mockIndexedDB.open returning the mockDB
as before. Then add the test that stubs global indexedDB with idb.mockIndexedDB,
seeds idb.store with TEST_BUFFER at TEST_URL, calls loadModelWeights(TEST_URL),
and asserts idb.closeSpy was called. Ensure you reference createMockIDB,
mockIndexedDB, mockDB.close (now hooked to closeSpy), and loadModelWeights in
the change.
---
Duplicate comments:
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Around line 124-133: The function leaks the IDBDatabase handle when a cache
hit returns early because the current finally that calls db?.close() only wraps
the later try block; refactor by wrapping the entire function body (including
the initial openDB()/idbGet/cache-return block that references db, idbGet,
onProgress, onWarning) in a single try...finally so that db?.close() is executed
on all exit paths; ensure you keep the existing logic and error handling
(openDB(), idbGet(), onProgress/onWarning) but move the finalizer that calls
db?.close() into the single finally that encloses those operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41c22335-be6c-4aa0-b872-3816cc7759b8
📒 Files selected for processing (2)
app/packages/annotation/src/providers/modelCache.test.tsapp/packages/annotation/src/providers/modelCache.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/packages/annotation/src/providers/modelCache.ts (1)
105-105:⚠️ Potential issue | 🟡 MinorDefensively avoid throwing a nullable value.
throw lastErrorcan thrownullif retry count is ever configured to0. Use a fallbackError.🛡️ Suggested fix
- throw lastError; + throw lastError ?? new Error(`Failed to fetch ${url} after ${MAX_RETRIES} attempts`);#!/bin/bash # Verify retry count and throw site. rg -n -C3 'MAX_RETRIES|throw lastError' app/packages/annotation/src/providers/modelCache.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/annotation/src/providers/modelCache.ts` at line 105, The throw site uses `throw lastError`, which can be null when retries are configured to 0; change the throw to use a non-null fallback (e.g., `throw lastError ?? new Error("Model cache operation failed")`) so a valid Error is always thrown; update the code that manages `lastError` and the `MAX_RETRIES`/retry loop in modelCache.ts (referencing the `lastError` variable and the retry logic around `MAX_RETRIES`) to ensure the thrown value is never nullable and preserves the original error when present.app/packages/annotation/src/providers/modelCache.test.ts (1)
17-40: 🧹 Nitpick | 🔵 TrivialAssert DB handle cleanup explicitly in tests.
mockDB.closeis a no-op, so cleanup regressions can pass unnoticed. Expose acloseSpyand assert it on cache-hit path.🧪 Suggested enhancement
function createMockIDB() { const store = new Map<string, ArrayBuffer>(); + const closeSpy = vi.fn(); @@ const mockDB = { @@ - close: () => {}, + close: closeSpy, }; @@ - return { store, mockIndexedDB }; + return { store, mockIndexedDB, closeSpy }; }it("Returns cached buffer without fetching on cache hit", async () => { - store.set(TEST_URL, TEST_BUFFER); + const idb = createMockIDB(); + vi.stubGlobal("indexedDB", idb.mockIndexedDB); + idb.store.set(TEST_URL, TEST_BUFFER); const result = await loadModelWeights(TEST_URL); expect(result.byteLength).toBe(64); expect(global.fetch).not.toHaveBeenCalled(); + expect(idb.closeSpy).toHaveBeenCalled(); });Also applies to: 88-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/annotation/src/providers/modelCache.test.ts` around lines 17 - 40, The mocked DB's close method is a no-op so tests don't detect cleanup regressions; replace that no-op with a spy (e.g., closeSpy) on the mockDB (the object returned by the open() request in the mockIndexedDB) and return or expose the spy alongside store/mockIndexedDB from the helper used in modelCache.test.ts; then update the cache-hit test to explicitly assert closeSpy was called (on the cache-hit path) to ensure DB handles are closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Around line 50-57: The retry loop in modelCache.ts (the for loop using
attempt, MAX_RETRIES and RETRY_BACKOFF_MS) calls fetch(url) with no per-attempt
timeout which can hang indefinitely; modify the attempt body to create an
AbortController per attempt, pass its signal to fetch (fetch(url, { signal })),
start a timer that calls controller.abort() after a configured
PER_ATTEMPT_TIMEOUT_MS, await the fetch, and always clear the timeout (and
handle AbortError in the existing catch) so each attempt is bounded and the
retry loop can continue or fail cleanly.
---
Duplicate comments:
In `@app/packages/annotation/src/providers/modelCache.test.ts`:
- Around line 17-40: The mocked DB's close method is a no-op so tests don't
detect cleanup regressions; replace that no-op with a spy (e.g., closeSpy) on
the mockDB (the object returned by the open() request in the mockIndexedDB) and
return or expose the spy alongside store/mockIndexedDB from the helper used in
modelCache.test.ts; then update the cache-hit test to explicitly assert closeSpy
was called (on the cache-hit path) to ensure DB handles are closed.
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Line 105: The throw site uses `throw lastError`, which can be null when
retries are configured to 0; change the throw to use a non-null fallback (e.g.,
`throw lastError ?? new Error("Model cache operation failed")`) so a valid Error
is always thrown; update the code that manages `lastError` and the
`MAX_RETRIES`/retry loop in modelCache.ts (referencing the `lastError` variable
and the retry logic around `MAX_RETRIES`) to ensure the thrown value is never
nullable and preserves the original error when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74d9ad79-6de3-4e65-967e-df8e7c93755d
📒 Files selected for processing (2)
app/packages/annotation/src/providers/modelCache.test.tsapp/packages/annotation/src/providers/modelCache.ts
…n case we ever have a 0 max tries...
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/modelCache.test.ts`:
- Around line 189-198: Add a unit test that simulates a failing IndexedDB read
path so loadModelWeights falls back to fetch and emits the proper warning: stub
global indexedDB to return a successful open but supply a
transaction/objectStore.get that triggers onerror (with an error message like
"Read failed"), call loadModelWeights(TEST_URL, undefined, onWarning) and assert
the returned buffer byteLength is 64 and that onWarning was called with a
message containing "IndexedDB cache read failed"; reference the loadModelWeights
function and the idbGet/read behavior in your mock (reuse or adapt createMockIDB
and the open/transaction/objectStore.get mocks) and ensure closeSpy is not
called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30a4ee8f-cd6d-4041-b5c1-371ff228cdf1
📒 Files selected for processing (2)
app/packages/annotation/src/providers/modelCache.test.tsapp/packages/annotation/src/providers/modelCache.ts
lanzhenw
reviewed
Mar 19, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Around line 60-65: The current error handling treats all 4xx responses as
non-retryable; update the conditional around response.status in the retry loop
(the block that sets lastError using response.status/response.statusText and
references response and url) so that 408 (Request Timeout) and 429 (Too Many
Requests) are treated as transient and retried: only break for 4xx statuses that
are not 408 or 429 (e.g., change the condition to break if response.status >=
400 && response.status < 500 && response.status !== 408 && response.status !==
429), otherwise continue to allow retries for 408/429. Ensure lastError is still
set before continuing so the final throw preserves the last failure.
- Around line 75-96: The current streaming code buffers every chunk into
chunks[] then allocates a second full Uint8Array (result) causing ~2x memory
use; change the logic to preallocate a single Uint8Array of size total (use the
existing total variable) and write each incoming chunk directly into that buffer
as you read from response.body.getReader(), updating an offset and calling
onProgress(loaded, total) as before, then return the preallocated buffer.buffer;
remove the temporary chunks array and the second copy to eliminate double
buffering (references: response.body.getReader(), reader, chunks, loaded,
result, offset, onProgress).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ff9ab1d-7876-40ec-a5a6-c406312f5717
📒 Files selected for processing (1)
app/packages/annotation/src/providers/modelCache.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/packages/annotation/src/providers/modelCache.ts (1)
85-87:⚠️ Potential issue | 🟡 MinorPotential
RangeErrorif server sends more data thanContent-Length.If the server's actual response body exceeds the declared
content-length,result.set(value, loaded)will throw when writing past the pre-allocated buffer boundary. This is an edge case (misbehaving server), but defensive handling prevents cryptic errors.🛡️ Proposed defensive fix
for (;;) { const { done, value } = await reader.read(); if (done) break; + if (loaded + value.byteLength > result.byteLength) { + // Server sent more data than Content-Length claimed; expand buffer + const expanded = new Uint8Array(loaded + value.byteLength); + expanded.set(result.subarray(0, loaded)); + result = expanded; + } result.set(value, loaded); loaded += value.byteLength; onProgress(loaded, total); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/annotation/src/providers/modelCache.ts` around lines 85 - 87, The code using result.set(value, loaded) can throw a RangeError if the server sends more bytes than the declared total; update the read loop in modelCache.ts to defensively check if loaded + value.byteLength > total before calling result.set, and handle that case by either writing only the slice that fits (e.g., value.subarray(0, total - loaded)) and then breaking/finishing, or by logging/throwing a clear error; ensure you still call onProgress with the clamped final loaded value and avoid writing past the pre-allocated buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/modelCache.test.ts`:
- Around line 185-210: Add a test that verifies loadModelWeights aborts a
stalled fetch via AbortController and retries: mock global.fetch so the first
call returns a never-resolving promise and the second call resolves to
mockFetchResponse(TEST_BUFFER), use vi.useFakeTimers(), call
loadModelWeights(TEST_URL) to get the promise, advance timers past
FETCH_TIMEOUT_MS (e.g. await vi.advanceTimersByTimeAsync(FETCH_TIMEOUT_MS + 1))
to trigger the AbortController timeout, then advance timers to process retry
backoff, await the promise and assert the final buffer, that global.fetch was
called twice, and any relevant cleanup spies (e.g. closeSpy) were invoked;
reference loadModelWeights and FETCH_TIMEOUT_MS to locate the implementation.
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Line 90: The code currently returns result.buffer which may contain trailing
uninitialized bytes if the stream closed early; in the function that reads
weights (referencing result.buffer, loaded and total), validate that loaded ===
total and if not either throw an error or return a trimmed buffer (e.g., slice
result.buffer to loaded) to avoid silent corruption of ONNX weights; update the
return path to use the trimmed buffer when loaded < total and ensure any error
path logs/throws a clear error indicating the truncation.
---
Duplicate comments:
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Around line 85-87: The code using result.set(value, loaded) can throw a
RangeError if the server sends more bytes than the declared total; update the
read loop in modelCache.ts to defensively check if loaded + value.byteLength >
total before calling result.set, and handle that case by either writing only the
slice that fits (e.g., value.subarray(0, total - loaded)) and then
breaking/finishing, or by logging/throwing a clear error; ensure you still call
onProgress with the clamped final loaded value and avoid writing past the
pre-allocated buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed6eaf91-6d7e-412a-9d5f-9bc976d92771
📒 Files selected for processing (2)
app/packages/annotation/src/providers/modelCache.test.tsapp/packages/annotation/src/providers/modelCache.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/packages/annotation/src/providers/modelCache.ts (1)
90-90:⚠️ Potential issue | 🟠 MajorThrow on truncated streams instead of returning partial weights.
Returning
result.slice(0, loaded).bufferturns a short read into a successful fetch, andloadModelWeights()will then persist that incomplete file in IndexedDB. A transient truncation becomes a sticky corrupted cache entry for later loads. Let this path throw so the retry loop can recover instead.💡 Proposed fix
- return loaded === total ? result.buffer : result.slice(0, loaded).buffer; + if (loaded !== total) { + throw new Error( + `Truncated response while fetching ${url}: received ${loaded} of ${total} bytes` + ); + } + return result.buffer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/annotation/src/providers/modelCache.ts` at line 90, The code in modelCache.ts currently returns a partial buffer when a stream is truncated (returning result.slice(0, loaded).buffer) which causes corrupted cache entries; change the logic in the function that reads weights (the loadModelWeights / buffer-assembly code using variables result, loaded, and total) to throw an Error when loaded !== total instead of returning a sliced buffer—include context in the error message (e.g., expected total vs loaded bytes and model id) so the retry loop can detect and recover from truncated reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/providers/modelCache.test.ts`:
- Around line 10-45: The mock writes persist even when a readwrite transaction
is supposed to abort; change the mock so mockObjectStore.put does not write
directly to the shared `store` but instead stages the value on the transaction,
and only commit staged writes to `store` inside the transaction's oncomplete
setter (in mockDB.transaction). Ensure when `shouldFailPut` causes tx.onerror to
fire the staged writes are discarded so aborted transactions do not modify
`store`; update references in mockObjectStore.put, mockDB.transaction
(transaction, objectStore, set oncomplete) and any staging container so tests
with failOn === "put" reflect real IndexedDB semantics.
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Around line 55-57: Replace the fixed 60s deadline with an idle timeout that is
re-armed after receiving headers and after each successful chunk read: create a
timeout handle (instead of a one-off setTimeout) tied to the existing
AbortController (controller), call clearTimeout and reset the timeout after
response.headers are processed and again inside the streaming loop after each
chunk is appended, and only call controller.abort() when the idle timer fires.
Also ensure the stream handler throws an error (and does not return/allow
caching) when the stream ends prematurely by checking if loaded < total and
rejecting/throwing instead of returning a partial buffer so truncated downloads
are not treated as successful.
---
Duplicate comments:
In `@app/packages/annotation/src/providers/modelCache.ts`:
- Line 90: The code in modelCache.ts currently returns a partial buffer when a
stream is truncated (returning result.slice(0, loaded).buffer) which causes
corrupted cache entries; change the logic in the function that reads weights
(the loadModelWeights / buffer-assembly code using variables result, loaded, and
total) to throw an Error when loaded !== total instead of returning a sliced
buffer—include context in the error message (e.g., expected total vs loaded
bytes and model id) so the retry loop can detect and recover from truncated
reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a8ac14f-4585-4d76-8203-9fcf70ce882f
📒 Files selected for processing (2)
app/packages/annotation/src/providers/modelCache.test.tsapp/packages/annotation/src/providers/modelCache.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Add modelCache that downloads encoder and decoder weights with streaming progress reporting and retry logic, then caches the weights in IndexedDB. On subsequent visits, weights are directly served from cache.
Worker now uses loadModelWeights instead of bare fetch, and forwards progress/warning notifications to the main thread via typed postMessage helpers.
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Tests