feat(webllm): per-bullet "Suggest a rewrite" pilot — WebGPU-only, Qwen2-1.5B - #39
Conversation
bc9bb2c to
19be3f7
Compare
There was a problem hiding this comment.
PR Review: feat(webllm) per-bullet "Suggest a rewrite" pilot
Updated — reclassified the hardcoded-color and component-placement findings from Suggestion to Blocking. Both are enforced rules in CLAUDE.md (the styling "What NOT to do" list and the 3-tier component architecture), not stylistic preferences.
Summary
Clean, well-tested WebLLM pilot. Capability detection, lazy engine load, the pure rewrite function, and telemetry all match issue #3's spec. Verified locally on the branch: 207 tests pass (13 new), typecheck clean, build code-splits @mlc-ai/web-llm into its own ~6 MB chunk (index-CereMTgS.js) with the entry chunk barely moving, and no posthog string in dist/ when VITE_POSTHOG_KEY is unset. Three blocking items to address before merge: a broken retry path and two CLAUDE.md rule violations on the new component.
Spec Alignment (issue #3)
| Requirement | Status | Notes |
|---|---|---|
| WebGPU detect, cache, 3-state | ✅ Implemented | capability.ts, cached promise, telemetry fires once |
Pinned Qwen2-1.5B MODEL_ID |
✅ Implemented | single const, no inline string |
| Lazy load on click, own chunk | ✅ Verified | index-CereMTgS.js 6 MB; entry +5 KB |
| Hide CTA (not grey) w/o WebGPU | ✅ Implemented | returns null; header documents the stance |
| 4 snake_case events, env-gated | ✅ Verified | no posthog in dist without key |
| Tests pass | ✅ Verified | 207 (6 capability + 7 rewrite-bullet new) |
Highlights
- Narrow
WebLlmEnginecontract so tests stub without importing the 6 MB lib — right call, keeps the type graph and test deps light. BULLET_REWRITE_SYSTEM_PROMPTas single source of truth, paraphrased in the user-facing "What's happening?" disclosure. Nice honesty.- Capability + telemetry caching via module-level promise/flags is correct and well-commented.
Key Findings
1. [Blocking] Failed engine load caches a rejected promise — "Try again" can never recover. web-llm.ts:41. cached is assigned the async IIFE with no .catch. If CreateMLCEngine throws (OOM, network drop), cached holds a rejected promise for the page lifetime; every later click hits if (cached) return cached and re-rejects instantly. The UI's "Try again" button (the error branch of labelFor) is therefore non-functional — only a page reload recovers. Reset the cache on failure so retry can re-attempt. This path is also untested (web-llm.ts has no test file).
2. [Blocking] Hardcoded raw Tailwind palette in feature code. RewriteButton.tsx (L95, L119, L186, L193). border-neutral-300 bg-white text-red-700 border-emerald-200 bg-emerald-50 etc. CLAUDE.md lists this under "What NOT to do → ❌ Hardcoded colors" for feature components — enforced, not advisory. The token system already ships feedback-success-{bg,border,text} and feedback-error-* purpose-built for the green-result and red-error boxes here, and they auto-adapt to dark mode via @media (prefers-color-scheme) in styles.css — so the manual dark: pairs are redundant parallel theming. Chip/FeedbackControl/ContactCard are the pattern to follow.
3. [Blocking] RewriteButton placed in the wrong architecture tier. It's wired to bullet domain data, the CLAUDE.md definition of a Feature component, so it belongs in src/components/features/ next to the siblings Result.tsx already imports (ScoreRing, VerdictHeader, ContactCard, FeedbackControl). The 3-tier architecture is enforced, and the PR description's justification — "the ui/ + features/ tiers don't exist yet on main" — is factually wrong; features/ is right there. Move to features/RewriteButton.tsx.
4. [Suggestion] Raw <button> vs the <Button> primitive. CLAUDE.md says use the primitive — but none exists yet (only Chip in ui/). Keeping the raw element is fine for this PR; flagging so inline button styles don't proliferate before a Button primitive lands. Pairs with finding 2.
5. [Nit] Entry chunk +5.01 KB (559.46 → 564.47), a hair over the PR's "≤5 KB" claim. Trivial.
6. [Nit] webllm_first_rewrite fires even on empty output (rewrite-bullet.ts) when the model returns null content and postProcess yields "". Also copied never resets to false. Both minor.
Verdict
Action: REQUEST_CHANGES
Rationale: One blocking correctness bug (advertised retry can't recover) plus two blocking CLAUDE.md rule violations on the new component (hardcoded palette, wrong tier). All three are cheap to fix and avoid baking drift into the first AI-capability component. Spec coverage, tests, bundle split, and telemetry gating are all in great shape.
| downloadStartedFired = true; | ||
| trackWebllmDownloadStarted(); | ||
| } | ||
| cached = (async () => { |
There was a problem hiding this comment.
[Blocking]: If the engine load rejects (OOM, dropped network), cached permanently holds a rejected promise — every later click returns it via if (cached) return cached and re-rejects instantly. The "Try again" button (error branch of labelFor) then can't recover; only a reload does. Reset the cache on failure:
cached = (async () => { ... })();
cached.catch(() => { cached = null; }); // allow a real retryThis path is untested too — worth a web-llm.ts test for the reject-then-retry case.
| type="button" | ||
| onClick={onClick} | ||
| disabled={busy} | ||
| className="self-start rounded-md border border-neutral-300 bg-white px-2 py-1 text-[11px] font-medium text-neutral-700 hover:border-neutral-400 hover:bg-neutral-50 disabled:cursor-not-allowed disabled:opacity-60 dark:border-neutral-700 dark:bg-neutral-900 dark:text-neutral-200 dark:hover:bg-neutral-800" |
There was a problem hiding this comment.
[Blocking]: Hardcoded raw Tailwind palette here (border-neutral-300 bg-white text-neutral-700 …, plus text-red-700 L119 and border-emerald-200 bg-emerald-50 L186/193). CLAUDE.md lists this under "What NOT to do → ❌ Hardcoded colors" for feature code — it's an enforced rule, not a preference. Use semantic tokens: error → feedback-error-*, result box → feedback-success-{bg,border,text}, all already defined and auto-adapting to dark via @media (prefers-color-scheme) in styles.css (so the dark: pairs become dead weight). Chip/FeedbackControl/ContactCard are the pattern.
Relatedly (non-blocking): the raw <button className> trips CLAUDE.md's "use the <Button> primitive" rule — but no Button primitive exists yet (only Chip). Fine to keep the raw element for this PR; flagging so we don't multiply inline button styling before a primitive lands.
| import { VerdictHeader } from "./features/VerdictHeader.tsx"; | ||
| import { ContactCard } from "./features/ContactCard.tsx"; | ||
| import { FeedbackControl } from "./features/FeedbackControl.tsx"; | ||
| import { RewriteButton } from "./RewriteButton"; |
There was a problem hiding this comment.
[Blocking]: RewriteButton is wired to bullet domain data — a Feature component by CLAUDE.md's 3-tier definition — so it belongs in src/components/features/, alongside the siblings imported just above (ScoreRing, VerdictHeader, ContactCard, FeedbackControl). This is the enforced architecture, and the PR description's justification ("the ui/ + features/ tiers don't exist yet on main") is factually wrong — features/ is right here and you're importing from it. Move to features/RewriteButton.tsx. (At 199 LOC it's also right at the ~200 split threshold — worth keeping an eye on.)
| if (!firstRewriteFired) { | ||
| firstRewriteFired = true; | ||
| trackWebllmFirstRewrite(); | ||
| } |
There was a problem hiding this comment.
[Nit]: trackWebllmFirstRewrite() fires before checking whether cleaned is non-empty, so a null-content / empty rewrite still counts as a "first rewrite". Consider gating the event on a non-empty result.
Blocking findings from s-annam's review: - web-llm.ts: clear the `cached` slot when `CreateMLCEngine` rejects so the "Try again" branch can re-attempt. Previously a failed load cached a rejected promise for the page lifetime and every later click re-rejected instantly. `downloadStartedFired` deliberately stays true so a retry doesn't double-fire `webllm_download_started` for the same logical attempt. Adds `web-llm.test.ts` mocking `@mlc-ai/web-llm` via `vi.mock` to cover the reject-then-retry, concurrent-call dedupe, model-id pin, and progress-forward paths. - features/RewriteButton.tsx: moved from `src/components/` to `src/components/features/` — Feature tier per CLAUDE.md since it consumes bullet domain data. Replaced raw Tailwind palette (`neutral-300`, `emerald-50`, `red-700`, etc.) and parallel `dark:` variants with semantic tokens (`border-border-light`, `bg-feedback-success-bg`, `text-feedback-error-text`, …). The token CSS auto-adapts via `prefers-color-scheme`, so the dark overrides were dead weight. Nit also fixed: - rewrite-bullet.ts: gate `trackWebllmFirstRewrite()` on a non-empty `cleaned` output so a null/empty model response isn't counted as a funnel step. Test coverage added for both the no-fire and fire-once cases via a hoisted `vi.mock` of the analytics module. Result.tsx's import path updated to `./features/RewriteButton.tsx`. Verification: typecheck clean; 213/213 tests pass (was 207 — +4 web-llm + 2 rewrite-bullet telemetry); `npm run build` clean; WebLLM still in its own ~6 MB chunk; `posthog` still absent from `dist/` when `VITE_POSTHOG_KEY` is unset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Blocking findings from s-annam's review: - web-llm.ts: clear the `cached` slot when `CreateMLCEngine` rejects so the "Try again" branch can re-attempt. Previously a failed load cached a rejected promise for the page lifetime and every later click re-rejected instantly. `downloadStartedFired` deliberately stays true so a retry doesn't double-fire `webllm_download_started` for the same logical attempt. Adds `web-llm.test.ts` mocking `@mlc-ai/web-llm` via `vi.mock` to cover the reject-then-retry, concurrent-call dedupe, model-id pin, and progress-forward paths. - features/RewriteButton.tsx: moved from `src/components/` to `src/components/features/` — Feature tier per CLAUDE.md since it consumes bullet domain data. Replaced raw Tailwind palette (`neutral-300`, `emerald-50`, `red-700`, etc.) and parallel `dark:` variants with semantic tokens (`border-border-light`, `bg-feedback-success-bg`, `text-feedback-error-text`, …). The token CSS auto-adapts via `prefers-color-scheme`, so the dark overrides were dead weight. Nit also fixed: - rewrite-bullet.ts: gate `trackWebllmFirstRewrite()` on a non-empty `cleaned` output so a null/empty model response isn't counted as a funnel step. Test coverage added for both the no-fire and fire-once cases via a hoisted `vi.mock` of the analytics module. Result.tsx's import path updated to `./features/RewriteButton.tsx`. Verification: typecheck clean; 213/213 tests pass (was 207 — +4 web-llm + 2 rewrite-bullet telemetry); `npm run build` clean; WebLLM still in its own ~6 MB chunk; `posthog` still absent from `dist/` when `VITE_POSTHOG_KEY` is unset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
325040c to
a70a642
Compare
|
Thanks for the careful review @s-annam — all three blocking items + the empty-output nit are addressed. Rebased onto latest 1. Engine retry path —
2. Hardcoded palette — every raw color in 3. Component tier — 4. Empty-output telemetry — One nit I left as-is: Rebase noteMain's new table-row BulletRow drops Verification
Ready for re-review whenever you have a minute. |
s-annam
left a comment
There was a problem hiding this comment.
PR Review: feat(webllm) per-bullet "Suggest a rewrite" pilot — re-review
Follow-up commit a70a642 resolves all three blocking items from the prior review, plus both nits. Verified locally on the PR head.
Prior blocking items — all fixed
| Item | Status |
|---|---|
#1 Rejected-promise cache made "Try again" a no-op (web-llm.ts) |
✅ pending.catch(() => { if (cached === pending) cached = null }) — identity-guarded so a slow failure can't null a later success's cache. New web-llm.test.ts covers reject-then-retry (re-invokes CreateMLCEngine, resolves). |
| #2 Hardcoded Tailwind palette | ✅ All semantic tokens now (feedback-success-{bg,border,text}, feedback-error-text, surface/content/border-*). No raw palette, no redundant dark: pairs. |
| #3 Wrong architecture tier | ✅ Moved to src/components/features/RewriteButton.tsx; Result.tsx imports ./features/RewriteButton.tsx. No dangling old-path refs. |
Nit: webllm_first_rewrite fired on empty output |
✅ Gated on cleaned.length > 0; test asserts no-fire on null content. |
Nit: copied never reset |
✅ setCopied(false) at click start. |
Local verification (PR head a70a642)
npm test→ 213 passed (19 webllm: 6 capability + 9 rewrite-bullet + 4 web-llm)npm run typecheck→ cleannpm run build→@mlc-ai/web-llmcode-split into its own ~6 MB chunk (index-CereMTgS.js); entry chunk 566 kB- No
posthogstring indist/withVITE_POSTHOG_KEYunset → env-gating + DCE hold
Highlights
- Telemetry one-shot flags survive a retry by design (
downloadStartedFiredstays true) so a single logical attempt doesn't double-firewebllm_download_started— and that reasoning is in the doc comment. Good. - Narrow
WebLlmEnginecontract keeps the 6 MB lib out of the test graph; concurrent-click sharing of one engine/one download is tested.
Remaining — non-blocking nits only
- [Nit]
LoadingPanelprogress bar has norole="progressbar"/aria-valuenow— screen-reader users get no load progress. - [Nit] Error
<p>isn't announced (role="alert"). - [Nit] Component is 197 LOC — right at the ~200 split threshold; worth watching as the rewrite UX grows.
Verdict
Action: APPROVE
Rationale: Zero blocking. The retry-recovery bug is fixed and now covered by a regression test, palette is token-clean, the component sits in the correct tier, and telemetry gating is sane. Tests green, typecheck clean, bundle split confirmed. Nits are optional polish.
| case "error": | ||
| return "Try again"; | ||
| default: | ||
| return "Suggest a rewrite"; |
There was a problem hiding this comment.
[Nit]: This progress bar conveys state purely visually. Consider role="progressbar" with aria-valuenow={pct} / aria-valuemin={0} / aria-valuemax={100} on the track div so screen-reader users get the ~1-minute download progress, not silence.
| )} | ||
|
|
||
| {status.kind === "rewriting" && ( | ||
| <p className="text-[11px] text-content-muted">Rewriting…</p> |
There was a problem hiding this comment.
[Nit]: The error message isn't announced to assistive tech. role="alert" (or aria-live="polite") on this <p> would surface a failed load to SR users without a visual cue.
In-browser AI rewrite using Qwen2-1.5B via WebLLM. WebGPU-only — the CTA is hidden on Firefox / iOS Safari rather than degrading silently. Weights lazy-load on click (never on page render); engine + capability are cached for the page lifetime. Architecture: src/lib/webllm/ owns capability detection, the pinned MODEL_ID, the engine loader, and a pure rewriteBulletWithLlm that takes the engine as a parameter so tests can stub it. RewriteButton drops in next to existing check pills in Result.tsx::BulletRow. The @mlc-ai/web-llm dynamic import keeps the library in its own ~5.7MB chunk; the entry chunk is unchanged. Telemetry: four snake_case events (capability_detected, download_started, loaded, first_rewrite) routed through the existing env-gated track() — no-op when VITE_POSTHOG_KEY is unset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Blocking findings from s-annam's review: - web-llm.ts: clear the `cached` slot when `CreateMLCEngine` rejects so the "Try again" branch can re-attempt. Previously a failed load cached a rejected promise for the page lifetime and every later click re-rejected instantly. `downloadStartedFired` deliberately stays true so a retry doesn't double-fire `webllm_download_started` for the same logical attempt. Adds `web-llm.test.ts` mocking `@mlc-ai/web-llm` via `vi.mock` to cover the reject-then-retry, concurrent-call dedupe, model-id pin, and progress-forward paths. - features/RewriteButton.tsx: moved from `src/components/` to `src/components/features/` — Feature tier per CLAUDE.md since it consumes bullet domain data. Replaced raw Tailwind palette (`neutral-300`, `emerald-50`, `red-700`, etc.) and parallel `dark:` variants with semantic tokens (`border-border-light`, `bg-feedback-success-bg`, `text-feedback-error-text`, …). The token CSS auto-adapts via `prefers-color-scheme`, so the dark overrides were dead weight. Nit also fixed: - rewrite-bullet.ts: gate `trackWebllmFirstRewrite()` on a non-empty `cleaned` output so a null/empty model response isn't counted as a funnel step. Test coverage added for both the no-fire and fire-once cases via a hoisted `vi.mock` of the analytics module. Result.tsx's import path updated to `./features/RewriteButton.tsx`. Verification: typecheck clean; 213/213 tests pass (was 207 — +4 web-llm + 2 rewrite-bullet telemetry); `npm run build` clean; WebLLM still in its own ~6 MB chunk; `posthog` still absent from `dist/` when `VITE_POSTHOG_KEY` is unset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address re-review nits: progress bar gets role="progressbar" + aria-valuenow/min/max so screen readers announce model-download progress; error message gets role="alert" so it's announced live. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a70a642 to
7377882
Compare
s-annam
left a comment
There was a problem hiding this comment.
Re-approve after rebase onto main + a11y nits (progressbar/alert roles). Conflicts resolved, verify CI green, 243 tests pass.
…n2-1.5B (#39) Per-bullet "Suggest a rewrite" pilot: WebGPU-only, Qwen2-1.5B, lazy-loaded ~6 MB web-llm chunk. Engine retry path, semantic tokens, features/ tier, telemetry gating, and load/error a11y roles. Rebased onto main; verify CI green; 243 tests pass. Resolves #3 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n2-1.5B (#39) Per-bullet "Suggest a rewrite" pilot: WebGPU-only, Qwen2-1.5B, lazy-loaded ~6 MB web-llm chunk. Engine retry path, semantic tokens, features/ tier, telemetry gating, and load/error a11y roles. Rebased onto main; verify CI green; 243 tests pass. Resolves #3 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n2-1.5B (#39) Per-bullet "Suggest a rewrite" pilot: WebGPU-only, Qwen2-1.5B, lazy-loaded ~6 MB web-llm chunk. Engine retry path, semantic tokens, features/ tier, telemetry gating, and load/error a11y roles. Rebased onto main; verify CI green; 243 tests pass. Resolves #3 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
First in-browser AI capability: a per-bullet "Suggest a rewrite" CTA that runs Qwen2-1.5B-Instruct via WebLLM on the user's GPU. Bytes never leave the browser. Resolves #3.
Library —
src/lib/webllm/types.ts— narrowWebLlmEnginecontract (only the surfacerewrite-bulletconsumes) so tests stub without importing the real lib.capability.ts—detectWebGpu()returns"available" | "no-webgpu" | "unsupported-os", cached as a module-level promise so the discriminator fires exactly once per page.web-llm.ts— pinnedMODEL_ID = "Qwen2-1.5B-Instruct-q4f16_1-MLC".loadEngine()dynamic-imports@mlc-ai/web-llmand caches the engine for the page lifetime — concurrent clicks across bullets share one download.rewrite-bullet.ts— purerewriteBulletWithLlm(bullet, engine). Single-source-of-truthBULLET_REWRITE_SYSTEM_PROMPT; post-processing strips a"Rewritten:"prefix, wrapping quotes, and trims to the first non-empty line.UI —
src/components/RewriteButton.tsxdetectWebGpu(); if not"available", the component returnsnull. Hidden, not greyed. Header comment documents the stance so a future contributor doesn't "improve" it with a fallback banner.Result.tsx::BulletRownext to the existing check pills via<RewriteButton bullet={bullet.text} />.Telemetry —
src/lib/analytics.tssnake_caseevents through the existing env-gatedtrack()helper:webllm_capability_detected(payload{capability}),webllm_download_started,webllm_loaded,webllm_first_rewrite. Each fires at most once per page.VITE_POSTHOG_KEYis unset — verified noposthogstring in built assets.Bundle — the
@mlc-ai/web-llmdynamic import keeps the library in its own ~5.7 MB chunk (dist/assets/index-CereMTgS.js); the entry chunk holds only the tiny dynamic-import reference. Per AC, main chunk does not grow >5 KB.Out of scope (explicit per issue)
Test plan
npm run typecheckcleannpm run test— 194 tests pass (13 new insrc/lib/webllm/: 6 capability + 7 rewrite-bullet)npm run buildclean;@mlc-ai/web-llmlives in its own chunk;grep -r posthog dist/assets/returns nothing withVITE_POSTHOG_KEYunsetVITE_POSTHOG_KEYset)Notes for review
WebLlmEngineinterface intentionally duplicates the shape ofMLCEngineInterfacerather than re-exporting it, so the type graph for non-WebLLM call-sites stays light and tests don't pull the library.src/components/RewriteButton.tsx) matching the existingDropZone.tsx/PdfPreview.tsx/Result.tsx— theui/+features/tiers from CLAUDE.md don't exist yet onmain. Happy to move once a primitive folder lands.