feat(webllm): model picker + consent gate + Dialog primitive - #143
Merged
Conversation
…64) Phase 2 of the M4 in-browser AI rewrite epic, closing #64. PR A shipped the backend (typed registry, keyed engine cache, model-dimensioned telemetry); this PR adds the user-facing picker and the consent gate required for the Restricted-Community models in that registry. Steps 5-6 of #64: - New Dialog primitive at src/design-system/primitives/Dialog.tsx, built on the native <dialog> element so focus trap, Esc, and ::backdrop come free. Controlled via the open prop. Re-exported via @design-system. - New ConsentDialog feature component on the Dialog primitive. Fires before any Restricted-Community model begins downloading. Per-model vendor link (licenseUrl), Accept/Decline buttons, decline defaults to initial focus per consent UX convention. Persistence handled by useModelSelection (per-licenseType in localStorage). - New ModelSelector feature component. Lists the 3 registry models with name, tier, downloadSizeMb, licenseType. Marks cached vs uncached rows by probing IndexedDB via hasModelInCache. Cached label "Downloaded · runs offline"; uncached label "Will download ~N GB (one-time)" — the spec's download-size warning that fires only on fresh downloads. Restricted-Community picks route through ConsentDialog. Inline download progress + a friendly-summary error surface with optional Technical details disclosure for the raw error. - New useModelSelection hook. Persists selection + per-licenseType consent to localStorage. Consent mirrored into React state so re-render freshness doesn't depend on incidental coupling. `storage` event listener wires up cross-tab sync (Tab B mirrors Tab A's pick). Pure I/O functions (readPersistedModelId, etc.) exported for testing without a React harness. Plus carrying PR A's TODO: - Cross-model loadEngine calls now serialize through a single in-flight `serialChain`. PR A documented the race as unreachable in its consumer scope and deferred the fix to PR B's picker; PR B introduces the picker, so the chain is in. Pinned by a test that verifies two same-microtask cross-model calls execute sequentially. - web-llm.ts cache shape split into loadedEngines (engines whose load finished — eviction candidates) and pendingByModelId (in-flight or chained — NOT eviction candidates, nothing to unload yet). This fixes a subtle pre-existing bug where evicting a pending entry would queue unload-on-resolve on a model the user just asked for. And closing #130 along the way: - New ModelLoadProgress shared component at src/design-system/shared/ModelLoadProgress.tsx replaces the two near-identical LoadingPanel functions previously hand-rolled in RewriteButton.tsx and SectionRewrite.tsx. Both consumers now import the shared one. Consumer wiring: - RewriteButton.tsx and SectionRewrite.tsx now read selectedModelId from useModelSelection (instead of hard-coding DEFAULT_MODEL_ID). - ReconstructedResume.tsx mounts ModelSelector at the top of the Experience section. Picker returns null when WebGPU is unavailable (same pattern as RewriteButton/SectionRewrite). Reviewer pass on the diff caught 2 blocking issues — both fixed: - Cache probe was racing with WebLLM's in-flight IndexedDB writes during a download (could flip a row to "Downloaded" mid-download). Probe now triggers on a lastCompletedAt timestamp bumped only after the load resolves successfully. - Consent freshness was depending on incidental re-renders. Now consentMap is a real React state, so recordConsent triggers the re-render that hasConsent's check needs. Plus reviewer-flag fixes: removed autoFocus from Accept (Decline is the safe consent default), inverted error fallback (friendly summary first, raw error in a disclosure), derived LICENSE_TYPES from MODEL_REGISTRY (no hardcoded list to maintain), and added console.warn on unload failure so OOM-after-switch bug reports have something to investigate. Gates: 597/597 tests deterministic across 3 consecutive full-suite runs, typecheck clean, build green. Two pre-merge gates from PR A carry forward (drafts in .claude/scratch/issue-64-prep/, posted by hand on the issue): - Version-pin verification (mechanical) - License vetting for Gemma 2 Terms of Use + Llama 3.2 Community License (human judgment; this is the more critical of the two now that the consent gate fires at runtime) Closes #64 Closes #130 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Vaishnavi1709
force-pushed
the
vk/model-picker-issue-64
branch
from
June 22, 2026 18:44
1e7a877 to
6405558
Compare
s-annam
reviewed
Jun 22, 2026
s-annam
left a comment
s-annam
left a comment
Contributor
There was a problem hiding this comment.
PR Review: model picker + consent gate + Dialog primitive
Summary
High-quality PR B of the M4 epic. Picker + consent gate + Dialog primitive + shared ModelLoadProgress (closes #130), and it lands PR A's deferred cross-model serialization in web-llm.ts. Tests thorough, docstrings excellent, reuse analysis honored. No blocking issues. One notable lazy-load regression + one primitive footgun worth addressing.
Gates (run on checked-out branch): npm run typecheck clean · npm run test 668/668 green.
Spec alignment (#64 steps 5-6)
| Requirement | Status |
|---|---|
| Picker UI mounted near rewrite surface | ✅ |
Per-licenseType consent persistence + cross-tab sync |
✅ |
| Consent gate fires before Restricted-Community load; decline reverts | ✅ |
| Cached vs fresh-download row labels | ✅ |
Cross-model loadEngine serialization (PR A TODO) |
✅ |
| Friendly-first error w/ raw detail disclosure | ✅ |
Highlights
serialChain+ splitloadedEngines/pendingByModelIdis correct and cleanly reasoned — eviction now happens at our turn under serialization, killing the concurrent-cross-model race. The chain never rejects (inner try/catch resolves/rejects the separateslot), so a failed load can't wedge the queue.useModelSelectionstorage I/O split into pure, registry-validated functions;safeGet/safeSetswallow sandboxed-storage throws. Strong coverage incl. cross-tab + deprecated-id fallback.- Genuine dedup of the two
LoadingPanelcopies into sharedModelLoadProgress.
Key findings (all non-blocking)
- [Suggestion - perf]
ModelSelectoreagerly imports@mlc-ai/web-llmon mount viaprobeCachedIds. The picker mounts for any WebGPU user viewing a parsed resume with bullets, so the multi-MB WebLLM chunk now downloads on resume render — before any rewrite intent. Defeats the project's lazy-load design (web-llm was previously fetched only on Rewrite click). See inline. - [Suggestion]
Dialogprimitive double-firesonCloseon programmatic close. See inline. - [Nit] Stale-engine micro-race in
loadEnginefast-path A. See inline. - [Nit]
ModelSelectordocstring mentions a "Try again" button that isn't rendered (retry = re-click the errored row).
Verdict
COMMENT — no blocking items; tests green. Recommend addressing #1 before merge (real shipped-bytes regression against the lazy-load design); #2 hardens the new primitive for reuse; #3/#4 are cleanup.
…ose, fast-path doc Resolves three review threads from s-annam on PR #143. - ModelSelector: defer `probeCachedIds` (which does `await import("@mlc-ai/web-llm")`) from a mount-time effect to first picker interaction (`onPointerEnter` / `onFocus` on the container, plus a defense-in-depth `setProbed(true)` in `onPick` for touch/keyboard paths). Previously, any WebGPU user viewing a parsed resume with bullets paid the multi-MB WebLLM chunk download before any rewrite intent, defeating the project's dynamic-import design. Until first interaction every row shows "Will download ~N GB" (degrades gracefully for returning users); after interaction the post-load `lastCompletedAt` re-probe keeps newly-cached rows flipping to "Downloaded · runs offline". - Dialog primitive: add an `isProgrammaticCloseRef` flag set true before the effect's own `dialog.close()`. A new `handleClose` swallows the redundant close event the native `<dialog>` emits in response, so consumers that toggle `open` and also do work in `onClose` (abort a request, free a resource) no longer see a spurious second invocation per user gesture. Current ConsentDialog unmounts rather than toggling, so the bug wasn't reachable — but the primitive shouldn't ship a reuse footgun. - web-llm.ts: doc-only. Note on the `loadEngine` fast-path A that a queued cross-model load can subsequently evict + `.unload()` the engine returned here. Unreachable with the current single-picker consumer (rows disabled mid-load), but a future multi-consumer caller would need a lock around `chat.completions.create()`. Gates: typecheck clean, 668/668 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
s-annam
reviewed
Jun 22, 2026
s-annam
left a comment
s-annam
left a comment
Contributor
There was a problem hiding this comment.
Reviewed on the checked-out branch — typecheck clean, 668/668 tests green, build per author. One critical functional bug plus supporting findings inline.
Headline (blocks merge): the picker selection never reaches the rewrite buttons in the same tab, so picking a model is a no-op for already-mounted RewriteButton / SectionRewrite instances. Detail on useModelSelection.ts.
s-annam
requested changes
Jun 22, 2026
s-annam
left a comment
s-annam
left a comment
Contributor
There was a problem hiding this comment.
Requesting changes on the critical bug: the picker selection never reaches the rewrite buttons in the same tab (useModelSelection is per-instance useState; the storage event does not fire in the writing tab). Picking a model is a no-op for already-mounted RewriteButton / SectionRewrite instances — they load DEFAULT_MODEL_ID. See the inline thread on src/hooks/useModelSelection.ts. Fix = shared store (useSyncExternalStore or context). The two 🟠 (stale eviction claim + untested stateful layer) and 🟡 (dialog unmount) also stand.
Resolves four review threads from s-annam on PR #143. 🔴 Critical (useModelSelection.ts): selection writes never propagated to already-mounted rewrite consumers in the same tab, because each call to the hook held isolated `useState` and the `storage` event only fires in OTHER tabs. Reduced rewrite buttons to silently using DEFAULT_MODEL_ID regardless of the picker. Moved state to a module-level store consumed via `useSyncExternalStore`; every write calls `notify()` synchronously so all subscribers re-render with the new snapshot. The cross-tab `storage` listener folds into the same notify path (registered lazily once). `_resetPersistedModelSelectionForTesting` resets store state too. 🟠 Stale eviction claim (web-llm.ts + rewrite-bullet.ts + rewrite-section.ts): the prior doc note claimed the fast-path-A eviction race was unreachable because picker rows are disabled mid-load — but RewriteButton and SectionRewrite are independent consumers, not gated by the picker's loading state, so a bullet rewrite can fast-path to engine A while the picker is downloading B. Added `acquireInference` / `releaseInference` counter and a `pendingUnload` Map; `evictAllExcept` parks `.unload()` for any model with positive inflight count and `releaseInference` drains the parked unload on the last release. Both rewrite call sites wrap `engine.chat.completions.create()` in `try/finally` so error paths still release. 🟠 Untested stateful layer (new useModelSelection.integration.test.tsx): the bug above lived in exactly the surface the existing pure-IO tests deliberately don't cover. Added a jsdom-env integration test that mounts two Probe consumers in one root and asserts a setSelectedModelId (and a recordConsent) from consumer A is observed by consumer B. Uses `react-dom/client` + React 19's `act` directly (no RTL). Required a jsdom devDep and extending vitest's include to `*.test.tsx`. 🟡 Dialog unmount-while-open (ModelSelector.tsx): consent dialog used to unmount via `consentModel && <…>` on accept/decline, skipping the Dialog primitive's `dialog.close()` effect and stranding focus. Split `consentModel` (data) from `consentOpen` (visibility): decline/accept sets `consentOpen=false`, then a rAF effect nulls out `consentModel` one frame later so the close effect has already restored focus by the time the React tree unmounts. Gates: typecheck clean, 670/670 tests green (+2 new integration), build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
s-annam
approved these changes
Jun 23, 2026
s-annam
left a comment
s-annam
left a comment
Contributor
There was a problem hiding this comment.
APPROVE — model picker + consent gate + Dialog primitive. Clean, well-documented PR; reuse analysis is solid and the 3-tier placement (primitive/shared/feature) is correct.
Gates: typecheck ✅, build ✅, tests green on CI Node 20 (671/673 — the 2 local fails I hit are Node 25's native experimental localStorage shadowing jsdom, not a code defect).
Verified the two riskiest spots — both correct:
Dialogdrives the native<dialog>imperatively viashowModal()/close()in an effect — so focus-trap,::backdrop, and Esc genuinely work (not the broken<dialog open>attribute path).ConsentDialogwiresonClose={onDecline}andDialog'sonCancelpreventDefaults → Esc maps to Decline (the safe consent default). Initial focus on Decline.
Non-blocking follow-ups (not required for merge):
- TOCTOU (narrow): between
await loadEngine()resolving on the fast path andacquireInference()firing insiderewriteBulletWithLlm, a concurrent picker switch canevictAllExcept→unload()the engine mid-use. Contained byserialChain+ the UI's retry path; worth a tracked issue if cross-model switching during an active rewrite becomes common. - A few silent
catch/void-promise spots (detectWebGpu, probe,safeSet) could log for diagnosability. ConsentDialogrendersmodel.licenseUrlas anhrefwith no scheme guard — registry is static source so near-zero risk, but anhttps://check is cheap defense.
None block. Nice work.
2 tasks
6 tasks
s-annam
added a commit
that referenced
this pull request
Jun 25, 2026
* feat(webllm): model picker + consent gate + Dialog primitive (PR B of #64) Phase 2 of the M4 in-browser AI rewrite epic, closing #64. PR A shipped the backend (typed registry, keyed engine cache, model-dimensioned telemetry); this PR adds the user-facing picker and the consent gate required for the Restricted-Community models in that registry. Steps 5-6 of #64: - New Dialog primitive at src/design-system/primitives/Dialog.tsx, built on the native <dialog> element so focus trap, Esc, and ::backdrop come free. Controlled via the open prop. Re-exported via @design-system. - New ConsentDialog feature component on the Dialog primitive. Fires before any Restricted-Community model begins downloading. Per-model vendor link (licenseUrl), Accept/Decline buttons, decline defaults to initial focus per consent UX convention. Persistence handled by useModelSelection (per-licenseType in localStorage). - New ModelSelector feature component. Lists the 3 registry models with name, tier, downloadSizeMb, licenseType. Marks cached vs uncached rows by probing IndexedDB via hasModelInCache. Cached label "Downloaded · runs offline"; uncached label "Will download ~N GB (one-time)" — the spec's download-size warning that fires only on fresh downloads. Restricted-Community picks route through ConsentDialog. Inline download progress + a friendly-summary error surface with optional Technical details disclosure for the raw error. - New useModelSelection hook. Persists selection + per-licenseType consent to localStorage. Consent mirrored into React state so re-render freshness doesn't depend on incidental coupling. `storage` event listener wires up cross-tab sync (Tab B mirrors Tab A's pick). Pure I/O functions (readPersistedModelId, etc.) exported for testing without a React harness. Plus carrying PR A's TODO: - Cross-model loadEngine calls now serialize through a single in-flight `serialChain`. PR A documented the race as unreachable in its consumer scope and deferred the fix to PR B's picker; PR B introduces the picker, so the chain is in. Pinned by a test that verifies two same-microtask cross-model calls execute sequentially. - web-llm.ts cache shape split into loadedEngines (engines whose load finished — eviction candidates) and pendingByModelId (in-flight or chained — NOT eviction candidates, nothing to unload yet). This fixes a subtle pre-existing bug where evicting a pending entry would queue unload-on-resolve on a model the user just asked for. And closing #130 along the way: - New ModelLoadProgress shared component at src/design-system/shared/ModelLoadProgress.tsx replaces the two near-identical LoadingPanel functions previously hand-rolled in RewriteButton.tsx and SectionRewrite.tsx. Both consumers now import the shared one. Consumer wiring: - RewriteButton.tsx and SectionRewrite.tsx now read selectedModelId from useModelSelection (instead of hard-coding DEFAULT_MODEL_ID). - ReconstructedResume.tsx mounts ModelSelector at the top of the Experience section. Picker returns null when WebGPU is unavailable (same pattern as RewriteButton/SectionRewrite). Reviewer pass on the diff caught 2 blocking issues — both fixed: - Cache probe was racing with WebLLM's in-flight IndexedDB writes during a download (could flip a row to "Downloaded" mid-download). Probe now triggers on a lastCompletedAt timestamp bumped only after the load resolves successfully. - Consent freshness was depending on incidental re-renders. Now consentMap is a real React state, so recordConsent triggers the re-render that hasConsent's check needs. Plus reviewer-flag fixes: removed autoFocus from Accept (Decline is the safe consent default), inverted error fallback (friendly summary first, raw error in a disclosure), derived LICENSE_TYPES from MODEL_REGISTRY (no hardcoded list to maintain), and added console.warn on unload failure so OOM-after-switch bug reports have something to investigate. Gates: 597/597 tests deterministic across 3 consecutive full-suite runs, typecheck clean, build green. Two pre-merge gates from PR A carry forward (drafts in .claude/scratch/issue-64-prep/, posted by hand on the issue): - Version-pin verification (mechanical) - License vetting for Gemma 2 Terms of Use + Llama 3.2 Community License (human judgment; this is the more critical of the two now that the consent gate fires at runtime) Closes #64 Closes #130 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(webllm): address review on PR #143 — lazy probe, dialog double-close, fast-path doc Resolves three review threads from s-annam on PR #143. - ModelSelector: defer `probeCachedIds` (which does `await import("@mlc-ai/web-llm")`) from a mount-time effect to first picker interaction (`onPointerEnter` / `onFocus` on the container, plus a defense-in-depth `setProbed(true)` in `onPick` for touch/keyboard paths). Previously, any WebGPU user viewing a parsed resume with bullets paid the multi-MB WebLLM chunk download before any rewrite intent, defeating the project's dynamic-import design. Until first interaction every row shows "Will download ~N GB" (degrades gracefully for returning users); after interaction the post-load `lastCompletedAt` re-probe keeps newly-cached rows flipping to "Downloaded · runs offline". - Dialog primitive: add an `isProgrammaticCloseRef` flag set true before the effect's own `dialog.close()`. A new `handleClose` swallows the redundant close event the native `<dialog>` emits in response, so consumers that toggle `open` and also do work in `onClose` (abort a request, free a resource) no longer see a spurious second invocation per user gesture. Current ConsentDialog unmounts rather than toggling, so the bug wasn't reachable — but the primitive shouldn't ship a reuse footgun. - web-llm.ts: doc-only. Note on the `loadEngine` fast-path A that a queued cross-model load can subsequently evict + `.unload()` the engine returned here. Unreachable with the current single-picker consumer (rows disabled mid-load), but a future multi-consumer caller would need a lock around `chat.completions.create()`. Gates: typecheck clean, 668/668 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(webllm): shared store + eviction guard + dialog mount on PR #143 Resolves four review threads from s-annam on PR #143. 🔴 Critical (useModelSelection.ts): selection writes never propagated to already-mounted rewrite consumers in the same tab, because each call to the hook held isolated `useState` and the `storage` event only fires in OTHER tabs. Reduced rewrite buttons to silently using DEFAULT_MODEL_ID regardless of the picker. Moved state to a module-level store consumed via `useSyncExternalStore`; every write calls `notify()` synchronously so all subscribers re-render with the new snapshot. The cross-tab `storage` listener folds into the same notify path (registered lazily once). `_resetPersistedModelSelectionForTesting` resets store state too. 🟠 Stale eviction claim (web-llm.ts + rewrite-bullet.ts + rewrite-section.ts): the prior doc note claimed the fast-path-A eviction race was unreachable because picker rows are disabled mid-load — but RewriteButton and SectionRewrite are independent consumers, not gated by the picker's loading state, so a bullet rewrite can fast-path to engine A while the picker is downloading B. Added `acquireInference` / `releaseInference` counter and a `pendingUnload` Map; `evictAllExcept` parks `.unload()` for any model with positive inflight count and `releaseInference` drains the parked unload on the last release. Both rewrite call sites wrap `engine.chat.completions.create()` in `try/finally` so error paths still release. 🟠 Untested stateful layer (new useModelSelection.integration.test.tsx): the bug above lived in exactly the surface the existing pure-IO tests deliberately don't cover. Added a jsdom-env integration test that mounts two Probe consumers in one root and asserts a setSelectedModelId (and a recordConsent) from consumer A is observed by consumer B. Uses `react-dom/client` + React 19's `act` directly (no RTL). Required a jsdom devDep and extending vitest's include to `*.test.tsx`. 🟡 Dialog unmount-while-open (ModelSelector.tsx): consent dialog used to unmount via `consentModel && <…>` on accept/decline, skipping the Dialog primitive's `dialog.close()` effect and stranding focus. Split `consentModel` (data) from `consentOpen` (visibility): decline/accept sets `consentOpen=false`, then a rAF effect nulls out `consentModel` one frame later so the close effect has already restored focus by the time the React tree unmounts. Gates: typecheck clean, 670/670 tests green (+2 new integration), build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Srinivas Annam <annam@annam.org>
s-annam
added a commit
that referenced
this pull request
Jun 28, 2026
* feat(webllm): model picker + consent gate + Dialog primitive (PR B of #64) Phase 2 of the M4 in-browser AI rewrite epic, closing #64. PR A shipped the backend (typed registry, keyed engine cache, model-dimensioned telemetry); this PR adds the user-facing picker and the consent gate required for the Restricted-Community models in that registry. Steps 5-6 of #64: - New Dialog primitive at src/design-system/primitives/Dialog.tsx, built on the native <dialog> element so focus trap, Esc, and ::backdrop come free. Controlled via the open prop. Re-exported via @design-system. - New ConsentDialog feature component on the Dialog primitive. Fires before any Restricted-Community model begins downloading. Per-model vendor link (licenseUrl), Accept/Decline buttons, decline defaults to initial focus per consent UX convention. Persistence handled by useModelSelection (per-licenseType in localStorage). - New ModelSelector feature component. Lists the 3 registry models with name, tier, downloadSizeMb, licenseType. Marks cached vs uncached rows by probing IndexedDB via hasModelInCache. Cached label "Downloaded · runs offline"; uncached label "Will download ~N GB (one-time)" — the spec's download-size warning that fires only on fresh downloads. Restricted-Community picks route through ConsentDialog. Inline download progress + a friendly-summary error surface with optional Technical details disclosure for the raw error. - New useModelSelection hook. Persists selection + per-licenseType consent to localStorage. Consent mirrored into React state so re-render freshness doesn't depend on incidental coupling. `storage` event listener wires up cross-tab sync (Tab B mirrors Tab A's pick). Pure I/O functions (readPersistedModelId, etc.) exported for testing without a React harness. Plus carrying PR A's TODO: - Cross-model loadEngine calls now serialize through a single in-flight `serialChain`. PR A documented the race as unreachable in its consumer scope and deferred the fix to PR B's picker; PR B introduces the picker, so the chain is in. Pinned by a test that verifies two same-microtask cross-model calls execute sequentially. - web-llm.ts cache shape split into loadedEngines (engines whose load finished — eviction candidates) and pendingByModelId (in-flight or chained — NOT eviction candidates, nothing to unload yet). This fixes a subtle pre-existing bug where evicting a pending entry would queue unload-on-resolve on a model the user just asked for. And closing #130 along the way: - New ModelLoadProgress shared component at src/design-system/shared/ModelLoadProgress.tsx replaces the two near-identical LoadingPanel functions previously hand-rolled in RewriteButton.tsx and SectionRewrite.tsx. Both consumers now import the shared one. Consumer wiring: - RewriteButton.tsx and SectionRewrite.tsx now read selectedModelId from useModelSelection (instead of hard-coding DEFAULT_MODEL_ID). - ReconstructedResume.tsx mounts ModelSelector at the top of the Experience section. Picker returns null when WebGPU is unavailable (same pattern as RewriteButton/SectionRewrite). Reviewer pass on the diff caught 2 blocking issues — both fixed: - Cache probe was racing with WebLLM's in-flight IndexedDB writes during a download (could flip a row to "Downloaded" mid-download). Probe now triggers on a lastCompletedAt timestamp bumped only after the load resolves successfully. - Consent freshness was depending on incidental re-renders. Now consentMap is a real React state, so recordConsent triggers the re-render that hasConsent's check needs. Plus reviewer-flag fixes: removed autoFocus from Accept (Decline is the safe consent default), inverted error fallback (friendly summary first, raw error in a disclosure), derived LICENSE_TYPES from MODEL_REGISTRY (no hardcoded list to maintain), and added console.warn on unload failure so OOM-after-switch bug reports have something to investigate. Gates: 597/597 tests deterministic across 3 consecutive full-suite runs, typecheck clean, build green. Two pre-merge gates from PR A carry forward (drafts in .claude/scratch/issue-64-prep/, posted by hand on the issue): - Version-pin verification (mechanical) - License vetting for Gemma 2 Terms of Use + Llama 3.2 Community License (human judgment; this is the more critical of the two now that the consent gate fires at runtime) Closes #64 Closes #130 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(webllm): address review on PR #143 — lazy probe, dialog double-close, fast-path doc Resolves three review threads from s-annam on PR #143. - ModelSelector: defer `probeCachedIds` (which does `await import("@mlc-ai/web-llm")`) from a mount-time effect to first picker interaction (`onPointerEnter` / `onFocus` on the container, plus a defense-in-depth `setProbed(true)` in `onPick` for touch/keyboard paths). Previously, any WebGPU user viewing a parsed resume with bullets paid the multi-MB WebLLM chunk download before any rewrite intent, defeating the project's dynamic-import design. Until first interaction every row shows "Will download ~N GB" (degrades gracefully for returning users); after interaction the post-load `lastCompletedAt` re-probe keeps newly-cached rows flipping to "Downloaded · runs offline". - Dialog primitive: add an `isProgrammaticCloseRef` flag set true before the effect's own `dialog.close()`. A new `handleClose` swallows the redundant close event the native `<dialog>` emits in response, so consumers that toggle `open` and also do work in `onClose` (abort a request, free a resource) no longer see a spurious second invocation per user gesture. Current ConsentDialog unmounts rather than toggling, so the bug wasn't reachable — but the primitive shouldn't ship a reuse footgun. - web-llm.ts: doc-only. Note on the `loadEngine` fast-path A that a queued cross-model load can subsequently evict + `.unload()` the engine returned here. Unreachable with the current single-picker consumer (rows disabled mid-load), but a future multi-consumer caller would need a lock around `chat.completions.create()`. Gates: typecheck clean, 668/668 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(webllm): shared store + eviction guard + dialog mount on PR #143 Resolves four review threads from s-annam on PR #143. 🔴 Critical (useModelSelection.ts): selection writes never propagated to already-mounted rewrite consumers in the same tab, because each call to the hook held isolated `useState` and the `storage` event only fires in OTHER tabs. Reduced rewrite buttons to silently using DEFAULT_MODEL_ID regardless of the picker. Moved state to a module-level store consumed via `useSyncExternalStore`; every write calls `notify()` synchronously so all subscribers re-render with the new snapshot. The cross-tab `storage` listener folds into the same notify path (registered lazily once). `_resetPersistedModelSelectionForTesting` resets store state too. 🟠 Stale eviction claim (web-llm.ts + rewrite-bullet.ts + rewrite-section.ts): the prior doc note claimed the fast-path-A eviction race was unreachable because picker rows are disabled mid-load — but RewriteButton and SectionRewrite are independent consumers, not gated by the picker's loading state, so a bullet rewrite can fast-path to engine A while the picker is downloading B. Added `acquireInference` / `releaseInference` counter and a `pendingUnload` Map; `evictAllExcept` parks `.unload()` for any model with positive inflight count and `releaseInference` drains the parked unload on the last release. Both rewrite call sites wrap `engine.chat.completions.create()` in `try/finally` so error paths still release. 🟠 Untested stateful layer (new useModelSelection.integration.test.tsx): the bug above lived in exactly the surface the existing pure-IO tests deliberately don't cover. Added a jsdom-env integration test that mounts two Probe consumers in one root and asserts a setSelectedModelId (and a recordConsent) from consumer A is observed by consumer B. Uses `react-dom/client` + React 19's `act` directly (no RTL). Required a jsdom devDep and extending vitest's include to `*.test.tsx`. 🟡 Dialog unmount-while-open (ModelSelector.tsx): consent dialog used to unmount via `consentModel && <…>` on accept/decline, skipping the Dialog primitive's `dialog.close()` effect and stranding focus. Split `consentModel` (data) from `consentOpen` (visibility): decline/accept sets `consentOpen=false`, then a rAF effect nulls out `consentModel` one frame later so the close effect has already restored focus by the time the React tree unmounts. Gates: typecheck clean, 670/670 tests green (+2 new integration), build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Srinivas Annam <annam@annam.org>
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.
Summary
PR B of the M4 in-browser AI rewrite epic, completing Phase 2 (#64). PR A (#129) shipped the backend — typed registry, keyed engine cache, model-dimensioned telemetry. This PR adds the user-facing picker (Steps 5–6 of the issue): a
Dialogprimitive, aConsentDialogfor Restricted-Community models, theModelSelectorfeature component mounted inReconstructedResume, and auseModelSelectionhook that persists selection + per-licenseTypeconsent tolocalStorage(with cross-tabstorage-event sync).Carries PR A's deferred TODO: cross-model
loadEnginecalls now serialize through a single in-flightserialChain, and the cache is split intoloadedEngines(eviction candidates) andpendingByModelId(in-flight, never evicted). Also closes #130 by replacing the two near-identical loading panels inRewriteButton/SectionRewritewith a sharedModelLoadProgresscomponent.Closes #64
Closes #130
Reuse analysis (per CLAUDE.md 3-tier rule)
Dialog(primitive,src/design-system/primitives/Dialog.tsx) — no existing modal primitive in@design-system; built on the native<dialog>element so focus trap, Esc, and::backdropcome free. Controlled via theopenprop. Re-exported via@design-system.ModelSelector(feature,src/components/features/ModelSelector.tsx) — no existing model-picker surface; the rewrite surfaces (RewriteButton,SectionRewrite) consume a model id but don't expose selection. Picker is mounted inline at the top of the Experience section inReconstructedResume, visible only in the rewrite context (returnsnullwhen WebGPU is unavailable, same pattern as the rewrite buttons).ConsentDialog(feature) — composed on the newDialogprimitive; not a parallel modal implementation.ModelLoadProgress(shared,src/design-system/shared/) — promoted from two hand-rolledLoadingPanelfunctions previously duplicated acrossRewriteButton.tsxandSectionRewrite.tsx. Both call sites now import the shared one.Reviewer-pass fixes already applied
lastCompletedAtbumped only after the load resolves successfully.consentMapis now real React state, sorecordConsenttriggers the re-render thathasConsentneeds.autoFocusfrom Accept (Decline is the safe consent default).<details>disclosure.LICENSE_TYPESderived fromMODEL_REGISTRY(no hardcoded list to maintain).console.warnon unload failure so OOM-after-switch bug reports have something to investigate.Pre-merge gates (carry over from PR A — handled by author on the issue)
@mlc-ai/web-llm(mechanical)..claude/scratch/issue-64-prep/.Test plan
npm run typecheckcleannpm run testgreen — 597/597 deterministic across 3 consecutive full-suite runsnpm run buildgreennpm run dev: pick each model; Restricted-Community picks fire the consent dialog; Decline reverts; cached vs uncached row labels correct; cross-tabstoragesync works