fix: offer "Load Anyway" memory override for all model types (image parity)#498
Merged
Conversation
…or any model type The "Load Anyway" memory override was wired only in the text-model load path (loadModelWithOverride / useChatModelActions). The shared failure surface that every non-text modality reports through (reportModelFailure -> ModelFailureCard) had no override affordance at all - so image/tts/stt/embedding could only ever show "Retry", never force past the budget. Make the override a property of the shared surface instead of the text path: - ModelFailure gains `overridable` + `onLoadAnyway`. - reportModelFailure derives `overridable` ONCE from the typed OverridableMemoryError discriminant (never re-sniffed from the message) and only keeps onLoadAnyway when the cause is genuinely overridable and the failure is a hard error (not a warning). - ModelFailureCard renders a "Load Anyway" action next to Retry when overridable. Any modality that reports an overridable gate now gets the same override, uniformly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…"Load Anyway"
_ensureImageModelLoaded caught the typed OverridableMemoryError and immediately
downgraded it to a plain string (`error?.message`) before reporting the failure.
That destroyed the `overridable` discriminant one layer too early, so the shared
failure surface could never know the gate was overridable - image models on a
memory-tight device (e.g. an 8GB S23 FE) got a dead-end "Free memory & Retry"
that re-ran into the same wall, with no way to force the load past the budget.
Route the image path onto the same override seam the text path uses:
- _fail carries the original thrown error as `opts.cause` (not just its message).
- When the cause is an OverridableMemoryError, _fail builds an onLoadAnyway that
re-runs the last request with { override: true }; reportModelFailure drops it
for any non-overridable cause, so other failures are unaffected.
- generateImage / _ensureImageModelLoaded thread `override` down to
loadImageModel({ override: true }) (which already evicts + forces the load).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…re surface
Fails-before / passes-after guards for the override parity fix:
- modelFailureHandler: overridable is set from the TYPED error; a non-overridable
error (and a soft warning with a memory cause) drops onLoadAnyway - the false
branches, so a caller can't offer override where it doesn't apply.
- ModelFailureCard: the "Load Anyway" button renders only when overridable AND a
handler is present (incl. a non-image type), and pressing it runs the handler.
- imageGenerationFlow (integration): an OverridableMemoryError on image load now
surfaces overridable + onLoadAnyway, and invoking it re-runs the load with
{ override: true }; a plain load error offers no override. Drives the REAL
imageGenerationService + REAL failure store (only native boundaries mocked), so
deleting the fix fails the test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds image-model integrity and override handling, expands audio stop/playback coverage, adds Pro MCP transport and UI tests, and updates repository coverage and submodule configuration. ChangesLoad Anyway Override Flow
Audio streaming and playback controls
Pro MCP services and UI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoEnable "Load Anyway" override on shared model failure card for all model types
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
33 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/ModelFailureCard.tsx (1)
41-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting a shared action-button helper.
The Retry and Load Anyway blocks are structurally identical (AnimatedPressable + Icon + Text, dismiss-then-callback). A small parameterized sub-render (icon name, color, label, handler, testID) would remove the duplication and make adding a third action later straightforward.
As per coding guidelines, "extend close matches with props rather than forking, and only build new when nothing fits."
♻️ Sketch of a shared action renderer
+function ActionButton({ icon, color, label, onPress, accessibilityLabel, testID, styles }: { + icon: string; color: string; label: string; onPress: () => void; + accessibilityLabel: string; testID: string; styles: ReturnType<typeof createStyles>; +}) { + return ( + <AnimatedPressable onPress={onPress} style={styles.actionButton} accessibilityLabel={accessibilityLabel} testID={testID}> + <Icon name={icon} size={12} color={color} style={styles.actionIcon} /> + <Text style={[styles.actionText, { color }]}>{label}</Text> + </AnimatedPressable> + ); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ModelFailureCard.tsx` around lines 41 - 65, The Retry and Load Anyway action blocks in ModelFailureCard are duplicated, so extract a small shared action renderer/helper to handle the common AnimatedPressable, Icon, and Text layout plus dismiss-then-callback behavior. Parameterize it with the action’s icon name, color, label, handler, and testID, and then use that helper for both failure.onRetry and failure.onLoadAnyway so adding future actions stays simple.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/ModelFailureCard.tsx`:
- Around line 41-65: The Retry and Load Anyway action blocks in ModelFailureCard
are duplicated, so extract a small shared action renderer/helper to handle the
common AnimatedPressable, Icon, and Text layout plus dismiss-then-callback
behavior. Parameterize it with the action’s icon name, color, label, handler,
and testID, and then use that helper for both failure.onRetry and
failure.onLoadAnyway so adding future actions stays simple.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19f6f709-8060-47ee-be34-debb476aeba4
📒 Files selected for processing (7)
__tests__/integration/generation/imageGenerationFlow.test.ts__tests__/unit/components/ModelFailureCard.test.tsx__tests__/unit/services/modelFailureHandler.test.tssrc/components/ModelFailureCard.tsxsrc/services/imageGenerationService.tssrc/services/modelFailureHandler.tssrc/stores/modelFailureStore.ts
… overridable
Addresses PR review (qodo): after "Load Anyway" retried with { override: true }, a
residency survival-floor refusal still threw OverridableMemoryError, so the failure
card kept re-offering "Load Anyway" as a repeated no-op (the image path re-runs the
same request, so it could loop).
Decide overridability at the owning service, once: checkImageModelCanLoad now returns
overridable:false when the refusal persists under override (the copy also says "even
after freeing other models"). The text loadTextModel throw is aligned to the same
invariant — a first refusal is overridable, a refusal under override is a plain
non-overridable Error — so the rule is single-sourced across both modalities.
Tests: unit (checkImageModelCanLoad flips overridable with the override flag, incl. the
false branch) + integration (pressing Load Anyway, then the forced retry failing as a
hard limit, leaves the card with no onLoadAnyway — the user-facing outcome).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e (compliance) Addresses PR review (qodo): a UI change must reference the design specification. Adds a header note pointing to DESIGN_PHILOSOPHY / DESIGN_PHILOSOPHY_SYSTEM / VISUAL_HIERARCHY_STANDARD and restating the token/weight/vector-icon rules the actions row follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oad Anyway Addresses PR review (CodeRabbit): the two action blocks were structurally identical (AnimatedPressable + Icon + Text, dismiss-then-callback). Extract one parameterized ActionButton (icon/color/label/testID/onPress) so they can't drift and a third action is a one-liner - per the reuse-before-building guideline. Behavior + testIDs unchanged; card tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai Addressed the nitpick in f8310e5 — extracted a shared |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git fetch origin f8310e5a --depth=1 2>&1 | tail -5
git show f8310e5a --stat 2>&1 | head -20
git show f8310e5a -- src/components/ModelFailureCard.tsx 2>&1 | head -200Length of output: 5076
This is exactly the kind of consolidation I was hoping for — nicely done. Thanks for addressing it. |
1 similar comment
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git fetch origin f8310e5a --depth=1 2>&1 | tail -5
git show f8310e5a --stat 2>&1 | head -20
git show f8310e5a -- src/components/ModelFailureCard.tsx 2>&1 | head -200Length of output: 5076
This is exactly the kind of consolidation I was hoping for — nicely done. Thanks for addressing it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/services/activeModelService/index.ts (1)
166-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a text-model equivalent of the image overridable test.
Only the image path (
checkImageModelCanLoad) has dedicated unit tests for the first-refusal-vs-override-retry Error-type branching. The text path's equivalent branch (Lines 166-171) currently has no matching coverage in the supplied files.Want me to draft a
doLoadTextModelLockedoverride test mirroringactiveModelService.imageOverridable.test.ts?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/activeModelService/index.ts` around lines 166 - 171, Add unit coverage for the override-vs-first-refusal branching in doLoadTextModelLocked, matching the existing image-path behavior tested for checkImageModelCanLoad. Create a text-model test that verifies the initial insufficient-memory refusal throws OverridableMemoryError, then a retry with opts.override set throws a plain Error from the same room.fits failure path. Mirror the structure and assertions used in activeModelService.imageOverridable.test.ts so the text path is covered with the same first-refusal/override semantics.__tests__/unit/services/activeModelService.imageOverridable.test.ts (1)
39-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add an
override: true+fits: truecase.Only
override: falseis exercised for the success path; adding anoverride: true+fits: truecase would confirm overriding never blocks a load that already fits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/services/activeModelService.imageOverridable.test.ts` around lines 39 - 43, The success path in activeModelService.imageOverridable tests only covers check({ override: false }) with makeRoomFor returning fits: true, so add a complementary case in activeModelService.imageOverridable.test.ts that calls check({ override: true }) and asserts canLoad stays true when the gate already fits. Reuse the existing check and makeRoomFor mocks in the same describe block so the new test verifies override does not block a load that already fits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/activeModelService/index.ts`:
- Around line 166-171: The first-refusal message in activeModelService’s memory
check is using the retry wording for both branches. Update the `!room.fits` path
so the non-override case throws the shorter initial refusal copy, and keep the
existing “even after freeing other models” text only for the `opts?.override`
branch in the `OverridableMemoryError`/`Error` split.
---
Nitpick comments:
In `@__tests__/unit/services/activeModelService.imageOverridable.test.ts`:
- Around line 39-43: The success path in activeModelService.imageOverridable
tests only covers check({ override: false }) with makeRoomFor returning fits:
true, so add a complementary case in activeModelService.imageOverridable.test.ts
that calls check({ override: true }) and asserts canLoad stays true when the
gate already fits. Reuse the existing check and makeRoomFor mocks in the same
describe block so the new test verifies override does not block a load that
already fits.
In `@src/services/activeModelService/index.ts`:
- Around line 166-171: Add unit coverage for the override-vs-first-refusal
branching in doLoadTextModelLocked, matching the existing image-path behavior
tested for checkImageModelCanLoad. Create a text-model test that verifies the
initial insufficient-memory refusal throws OverridableMemoryError, then a retry
with opts.override set throws a plain Error from the same room.fits failure
path. Mirror the structure and assertions used in
activeModelService.imageOverridable.test.ts so the text path is covered with the
same first-refusal/override semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bae4d47d-b5b4-4932-85c4-0385361b27f0
📒 Files selected for processing (4)
__tests__/integration/generation/imageGenerationFlow.test.ts__tests__/unit/services/activeModelService.imageOverridable.test.tssrc/components/ModelFailureCard.tsxsrc/services/activeModelService/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ModelFailureCard.tsx
- tests/integration/generation/imageGenerationFlow.test.ts
…dge, stop control Tests for the pro streaming-TTS stop fix (off-grid-ai/mobile-pro#18), which live here so they can import both core and pro: - streamingSpeech unit: stopStreamingSpeechForTurn suppresses re-engagement for the turn; isStreaming projects true/false; a new turn re-enables; and stopping mid-segment never advances to another segment or releases the engine (fails-before/passes-after of the on-device wedge). - streamingStateMachine integration: the user-stop [TTS-SM] trace is a clean abort with no "engine wedged → release" and release() never called. - PlaybackControls (RNTL): STOP glyph for a fileless synth (streaming/re-synth/preparing), pause/play for file-backed, always-controllable, and the usePlaybackState.isThisSynth derivation matrix (streaming / re-synth / paused / file-backed / other-message / idle). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ro#18) Pauses on a streaming/fileless-synth TTS now STOP cleanly instead of wedging + releasing the Kokoro engine (which bricked all later playback). See off-grid-ai/mobile-pro#18. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/unit/audio/streamingSpeech.test.ts (1)
163-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTimeout override and hung mock implementation aren't restored after the test.
_setSpeakTimeoutForTest(20)andmockEngine.speak.mockImplementation(() => new Promise(() => {}))mutate shared module/mock state without anyafterEach/cleanup. If further tests are appended after this suite in the same file, they'd inherit the 20ms timeout and the never-resolving speak mock, causing confusing intermittent failures.♻️ Suggested cleanup
it('stopping mid-segment does NOT advance to another segment or release the engine (no wedge)', async () => { _setSpeakTimeoutForTest(20); // fail fast instead of the real 15s // Engine can't complete (as if paused) — speak hangs, then times out. mockEngine.speak.mockImplementation(() => new Promise<void>(() => { /* never settles */ })); ... expect(isStreamingSpeechActive()).toBe(false); + _setSpeakTimeoutForTest(15000); // restore default to avoid leaking into later tests });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/audio/streamingSpeech.test.ts` around lines 163 - 165, The test in streamingSpeech.test.ts mutates shared state by overriding the speak timeout via _setSpeakTimeoutForTest and replacing mockEngine.speak with a never-resolving implementation, but never restores either setting. Update the affected test block around the speak timeout and mockEngine.speak setup to use cleanup (for example in afterEach or a local restore pattern) so the original timeout and mock behavior are reset after this test, preventing later tests in the same suite from inheriting the 20ms timeout or hung mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/unit/audio/streamingSpeech.test.ts`:
- Around line 163-165: The test in streamingSpeech.test.ts mutates shared state
by overriding the speak timeout via _setSpeakTimeoutForTest and replacing
mockEngine.speak with a never-resolving implementation, but never restores
either setting. Update the affected test block around the speak timeout and
mockEngine.speak setup to use cleanup (for example in afterEach or a local
restore pattern) so the original timeout and mock behavior are reset after this
test, preventing later tests in the same suite from inheriting the 20ms timeout
or hung mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ede74ba-ce04-483f-94d9-7c8ab83d120b
📒 Files selected for processing (4)
__tests__/integration/audio/streamingStateMachine.test.ts__tests__/rntl/components/PlaybackControls.test.tsx__tests__/unit/audio/streamingSpeech.test.tspro
…" (text path)
Addresses PR review (CodeRabbit): the text loadTextModel refusal used the retry
wording ("even after freeing other models") for BOTH branches, so the FIRST refusal
read like a post-eviction message. Differentiate to match the image path: the initial
(overridable) refusal is the shorter copy; only the override-retry (hard-limit) refusal
keeps "even after freeing other models".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai addressed in 18e36eb — the text |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git fetch origin 18e36eb5 --depth=1 2>&1 | tail -5
git show 18e36eb5 --stat 2>&1 | head -20
git show 18e36eb5 -- src/services/activeModelService/index.ts 2>&1 | head -150Length of output: 3002
This now matches the image path's messaging semantics. Nice catch and clean fix. (๑˃̵ᴗ˂̵)و |
…ilure card The flat "Model unloaded to free memory — tap to continue" bar was rendered ABOVE the rounded ModelFailureCard, so a flat full-width bar capped a rounded card — looks wrong. Swap the order in ChatMessageArea: the rounded failure card sits higher, the flat snackbar drops directly above the composer. Pure render-order change, no logic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gship Snapdragons Observed live on SM7250 (OnePlus Nord): app recommends a QNN model then the native process crashes "may not support this model's backend". Root cause: classifySmNumber's 'min' catch-all marks every Snapdragon NPU-capable, so getQualcommImageRec recommends qnn for SoCs that can't run it. Deferred (needs a curated SoC allowlist + device rounds), tracked with evidence + fix direction rather than a rushed capability change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…for broken models On-device (OnePlus Nord) an image model failed to load with a MISLEADING "your device (SM7250) may not support this model's backend" error. Root cause was NOT the backend: the downloaded mnn model was missing pos_emb.bin + clip_v2.mnn.weight — a partial unzip that was still marked `_ready` because nothing validated the extraction. The native server then died (code 1) and the generic fallback message blamed the SoC. A — never mark a partial extraction `_ready`: - New src/utils/imageModelIntegrity.ts: backend-agnostic completeness rule (required base files pos_emb.bin/token_emb.bin/tokenizer.json + primary unet + MNN split-weight pairing: every *.mnn must have its non-zero *.mnn.weight — this catches the dropped 156MB clip_v2.mnn.weight). Pure `checkImageModelFiles` + IO `validateImageModelDir` + `ensureImageExtractionComplete` (re-unzip once, else throw). - Wired into the primary zip download finalize and the resume path (whose old `validateModelDir` only checked "dir non-empty", so it passed a 9-of-11-file model). B — honest failure for an already-broken model: - New ImageModelIncompleteError (typed, NOT overridable). activeModelService.loadImageModel validates the model dir before the native load and throws it, so the failure card says "re-download" instead of the misleading "Load Anyway"/SoC message. Missing files can't be fixed by forcing the load, so Load Anyway is correctly withheld. Verified on device: restoring the two files let the GPU model load + generate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… optimization" On the first run the progress card read "GPU optimization in progress... (6/8)" even as real diffusion steps advanced — it looked like generation hadn't started. Once steps are advancing it IS generating: show "Generating image (N/steps)" with a one-time "(optimizing GPU, one-time)" aside on the first run, instead of leading with "optimization". The initial pre-step first-run notice (~120s one-time) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nerating label - imageModelIntegrity unit: reproduces the exact on-device bug (pos_emb.bin + clip_v2.mnn.weight dropped → incomplete), split-weight pairing, zero-byte-as-missing, qnn unet.bin, coreml non-empty-only. - modelFailureHandler: an ImageModelIncompleteError is NOT overridable (no Load Anyway → re-download), vs the memory-gate OverridableMemoryError which IS (the OP repro). - imageGenerationFlow: first-run mid-step label is "Generating image (6/20)" with the one-time optimize aside, never "GPU optimization in progress". - gaps: log the first-run inline-preview native-emission behaviour to revisit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lete seam (DRY) Self-review follow-up: resume had its own assertResumeExtractionComplete (validate→throw) duplicating ensureImageExtractionComplete from the integrity module — and inconsistently (no re-unzip retry). Route both resume unzip sites through the single shared seam so the completeness gate is defined once and resume also gets the re-unzip-once resilience. Also moved a stray mid-file logger import to the top. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…a garbage resolution On a non-NPU device (GPU/mnn path) a full SD1.5 model is a hard trade the user must steer, and the old UI let them fall into both traps: 8 steps → muddy; 512x512 → ~18 min on a mid-tier GPU; 128x128 → 43s but GARBAGE (SD1.5 is incoherent below ~256 — it's not a "smaller image", it's broken). - New utils/imageGenAdvice: pure, data-driven rule (branch on backend as data) — advises only on the mnn path: raise steps (<20), lower size (>256, for speed), or raise size (<256, which is garbage not smaller). NPU/CoreML get no advice (fast + fixed-shape). - New ImageGenAdviceCard in the image settings: reads settings live, renders the tips as the user moves the Steps/Size sliders. Design tokens + Feather icons + brand voice. - Image Size slider floor raised 128 → 256 (128/192 only ever produced garbage for these models); description states the 256-fast / 512-detailed-but-slow trade. - Generation floors width/height to 256 (Math.max) so a STALE persisted sub-256 setting can't reach the pipeline — the user never sees garbage, only "reduced size". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…floor - imageGenAdvice unit: mnn-only; raiseSteps (<20), lowerSize (>256), raiseSize (<256 = garbage, the 128 case), quiet at the 256/20 sweet spot, no false raiseSize at width 0. - ImageGenAdviceCard (RNTL): hidden off-mnn + at sweet spot; shows the right tip per setting, driving the real store + real rule. - imageGenerationFlow: a stale 128 setting is floored to 256 before the native call; a valid 512 passes through unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new extraction-completeness + load-time integrity checks run real RNFS validation. Four pre-existing suites (imageDownloadActions/resume, imageDownloadRecovery, activeModelService) mock RNFS/services and never lay a complete model dir on disk, so the new gate threw ImageModelIncompleteError where they expected success. Those suites cover download/residency ORCHESTRATION, not the completeness rule (which has its own dedicated tests in imageModelIntegrity.test + imageGenerationFlow), so mock the integrity boundary to "complete" in each. Full suite green (330 suites, 6960 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…new modules - jest.config: collectCoverageFrom now includes pro/** (measured, gated on proExists); pro carved into its own aggregate threshold group (its authoritative gate is the pro repo CI — core only runs pro-dependent suites, ~18%); new standalone modules (imageModelIntegrity, imageGenAdvice, modelLoadErrors, ImageGenAdviceCard) held to 100%. - Move ImageGenAdviceCard out of the settings modal INTO the chat (ChatMessageArea, beside ModelFailureCard) so a user hitting slow/garbled generations actually sees the tips; add a session dismiss. Discoverability fix. - Tests to 100% on the new modules: imageModelIntegrity IO wrappers (RNFS/unzip boundaries only), ImageModelIncompleteError + guard branches, advice-card dismiss + nullish, and a B false-branch wiring test (loadImageModel REFUSES an incomplete model — asserts the outcome: throws + native load never happens). Stub the new barrel export in the two chat-screen suites' component mocks. - CLAUDE.md: document the coverage direction (measure pro; new code 100%). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…c (18% → 60%) Phase 1 of the pro-coverage push (release blocked until pro is robust). 11 real, minimal-mock test suites (286 cases) driving the actual pro logic — mocks ONLY at genuine boundaries (network transport, OAuth/secure-storage, native audio-api, calendar), asserting observable OUTCOMES/state and both branches (deleting the impl fails them): - mcp: schemaTrim (pure, 0 mocks), mcpClient (real JSON-RPC over a scripted XHR fake), mcpService (real stores + service, McpClient/oauth boundaries stubbed as dumb data), oauthClient/pkce/tokenStore, mcpStore (real zustand), adapters, McpToolExtension.extra. - audio: EngineRegistry (DI stub engines implementing the interface — no native), audioFilePlayer (AudioContext boundary only). demo: demoMode. Config: - jest: collectCoverageFrom now includes pro/** (excl index/d.ts/tests); anchored the testPathIgnorePatterns '/pro/' → '<rootDir>/pro/' so it ignores only the submodule's own suite, NOT this repo's __tests__/pro/** (the bare pattern swallowed both). pro carved into its own coverage group, floor ratcheted 15→55/45/45/55 (measured ~60/50/52/61). - tsconfig: add @offgrid/pro/* path so tsc resolves the alias (jest/metro already did). EmailCalendarExtension: the agent's file was a redundant duplicate of the existing suite (already 93%/100%) and leaked a jest.spyOn(Linking) into the next worker file — removed it. Full suite green: 342 suites, 7264 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase 2 of the pro-coverage push. 8 real, minimal-mock suites driving the actual engine + audio logic — the ONLY mocked boundary is the native runtime each engine wraps (react-native-executorch / llama.rn / react-native-audio-api / RNFS / background-download), stubbed as dumb recorders. The real state machines, event emitters, size-weighted progress math, session-abort, and error cascades all run; assertions check phase transitions, emitted event payloads, and thrown errors — both branches — never "was called" alone: - engines: Qwen3TTSEngine (100% stmts), OuteTTSEngine, KokoroEngine.extra (uncovered branches). - audio logic: ttsPlayback.extra (file path + pause/resume/stop routing via the REAL playback machine), ttsStore.extra (store actions + residency canEvict veto), ttsEngineSubscription.extra, OnDeviceEngineEmitter, useAudioProgressCaption. Verified: full suite green (349 suites / 7434), no cross-suite pollution (spies restored, no leaked globals), tsc clean, eslint clean. Mutation-checked (e.g. breaking Qwen3's completeness ratio fails its tests). pro floor ratcheted 55→70/58/60/72 (measured 74/61/64/76). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o fields; pro bump Phase 3 of the pro-coverage push — the release-blocking robustness pass is done: pro is now 90% stmts / 83% branch / 85% func / 91% lines (from 18% at the start). 13 real RNTL suites rendering the ACTUAL pro UI against the REAL stores (useMcpStore / useTTSStore driven by setState), asserting what the user sees + the resulting store STATE after presses — never "a fn was called". Mocks only genuine shims (vector-icons, Animated/Sheet wrappers, theme, navigation). Both branches of each conditional render + handler covered; no cross-suite pollution (spies restored, stores reset in afterEach): - MCP: McpServerModal (100%), McpToolsScreen, McpGuideScreen, McpToolPickerSheet (100%), McpSettingsSection. - audio UI: EngineBridge, AudioModeLayout, MessageAudioMode, TTSButton, TTSSection, VoicePickerPopover, QuickSettingsAudioRow, AudioEmptyState. Type gaps the new tests exposed (pre-existing; pro UI was never tsc-checked from core): - core Message was missing audioPath / waveformData / audioDurationSeconds — genuinely read + set for audio-mode messages (voice notes). Added them (all optional, additive). - pro estimateClipDuration transcript param widened to optional (fixed on the pro branch; submodule bumped 103cd2ee → 9035ec50). pro coverage floor ratcheted 70→88/80/82/89. Full suite green: 362 suites / 7577 tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r flakiness The pre-push --findRelatedTests run fans hundreds of Message-importing suites across all workers; under that contention RNTL's 1s default waitFor timeout starves and a couple async-hook suites (e.g. useVoiceDownloadItems) flake with a spurious timeout despite passing in isolation (1.6-2.9s). Raise the global asyncUtilTimeout to 5s — load-tolerant, still well under the 10s jest testTimeout, no assertion changed. Removes the class of load flakiness surfaced when src/types entered the changed-file set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full review please — 10+ new commits since the last pass (pro test-coverage 18%→90%, Message type fields, jest/tsconfig coverage config, asyncUtilTimeout flake fix). |
|
✅ Action performedFull review finished. |
…e audio comment Review follow-ups (SOLID/DRY pass): - Import SWEET_SPOT_SIZE from imageGenAdvice into the size-slider min + the generation Math.max floor instead of re-hardcoding 256 in three places, so the guard value and the advice rule share one source of truth. - Clarify the Message audio-fields comment: audioDurationSeconds on Message is distinct from the same-named field on MediaAttachment (not a duplicate — different interfaces). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
__tests__/unit/audio/streamingSpeech.test.ts (1)
162-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReal-timer wait after short timeout risks flakiness under CI load.
_setSpeakTimeoutForTest(20)followed by a realsetTimeout(..., 40)gives only a 20ms margin for the timeout to fire and the drain to settle. Under CI contention this window can be too tight, causing an intermittent failure wheremockEngine.speakhasn't yet been called a second time or the timeout hasn't fired.Consider using Jest fake timers (
jest.useFakeTimers()+jest.advanceTimersByTime) for deterministic timing instead of a wall-clock sleep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/audio/streamingSpeech.test.ts` around lines 162 - 181, The streamingSpeech timeout test is using a real wall-clock sleep after `_setSpeakTimeoutForTest(20)`, which makes the timing-dependent assertion flaky under CI load. Update the `stopping mid-segment does NOT advance to another segment or release the engine (no wedge)` test to use Jest fake timers instead of `setTimeout`, and advance time deterministically so the hung `mockEngine.speak`, timeout handling, and `stopStreamingSpeechForTurn` behavior are exercised without relying on a narrow real-time window.__tests__/pro/audio/engines/OuteTTSEngine.test.ts (1)
279-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTautological assertion doesn't verify audio resource cleanup.
expect(mockAudioApi.closed).toBeGreaterThanOrEqual(0)is always true (a non-negative counter starting at 0), so it never actually confirms whether the nativeAudioContextgets closed onrelease(). This leaves the audio resource-cleanup path effectively unasserted in this test.♻️ Proposed fix: assert an exact expected value
- expect(mockAudioApi.closed).toBeGreaterThanOrEqual(0); + // If no AudioContext is created until the first speak(), assert it stays untouched: + expect(mockAudioApi.closed).toBe(0); + // OR, if release() is expected to close an existing context, assert it was closed once: + // expect(mockAudioApi.closed).toBe(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/pro/audio/engines/OuteTTSEngine.test.ts` around lines 279 - 291, The test for OuteTTSEngine.release is currently using a tautological check on mockAudioApi.closed, so it never proves the AudioContext was actually closed. Update the assertion in the release path test to verify the exact expected closed count after calling e.release(), using the existing OuteTTSEngine, release, and mockAudioApi symbols to confirm native audio cleanup really happened.src/services/activeModelService/index.ts (1)
259-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared memory-refusal helper.
The text path (lines 165-168) and this image path now duplicate the same override/non-override message-construction shape (
opts?.override ? "...even after freeing..." : "...short copy..."). This exact duplication is what caused the wording-drift bug flagged in the earlier review round. Consolidating into one small helper (parameterized by subject text and throw-vs-return usage) would keep both paths in sync automatically the next time either message needs to change.♻️ Example shared helper
+function memoryRefusalMessage(subject: string, override: boolean | undefined): string { + return override + ? `Not enough memory to load ${subject}, even after freeing other models. Close other apps or choose a smaller model.` + : `Not enough memory to load ${subject}. Close other apps or choose a smaller model.`; +}Then both
doLoadTextModelLockedandcheckImageModelCanLoadbuild their message viamemoryRefusalMessage(...)instead of inlining the ternary independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/activeModelService/index.ts` around lines 259 - 270, Extract the duplicated override/non-override memory refusal wording into a shared helper so the text and image paths stay in sync. Add a small utility (for example, a memoryRefusalMessage-style helper) and use it from both doLoadTextModelLocked and checkImageModelCanLoad instead of inlining the same ternary in each place. Keep the helper parameterized by the model/subject text and whether the caller should throw or return an error so both paths produce identical messaging.src/services/imageGenerationService.ts (1)
161-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
onRetryandonLoadAnywayclosures.Both closures do the same thing — guard on
this._lastParamsand re-invokethis.generateImage(this._lastParams, ...)— differing only in the opts payload. Could be factored into a smallretryLastParams(opts?)helper for a bit less repetition, but it's not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/imageGenerationService.ts` around lines 161 - 186, The `_fail` method in `imageGenerationService` repeats the same “retry last params” logic in both `onRetry` and `onLoadAnyway`. Extract that shared `this._lastParams` guard and `this.generateImage(...)` re-invocation into a small local helper inside `_fail`, then have `onRetry` and `onLoadAnyway` call it with their respective options so the behavior stays identical but duplication is removed.src/types/index.ts (1)
232-238: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the stale parenthetical
audioDurationSecondsis already defined onMediaAttachment, so this note reads like a duplicate-field warning inMessage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/index.ts` around lines 232 - 238, Remove the stale parenthetical note from the Message audio-mode field comment so it no longer implies audioDurationSeconds is a duplicate or separately declared field. Update the documentation around audioPath, waveformData, and audioDurationSeconds in the Message type to describe the audio-mode payload clearly, and keep the comment aligned with the existing MediaAttachment definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/pro/audio/ui/TTSButton.test.tsx`:
- Around line 142-157: The TTSButton press test is triggering an async store
update outside React’s testing boundary, which can make the assertion run before
all microtasks finish. Update the TTSButton test case to wrap the
`fireEvent.press` call in the same `act(async () => { ... })` pattern used
elsewhere in `TTSButton.test.tsx`, using the `handlePress`/`speak()` path as the
target behavior so the test waits for the full transition before asserting store
state.
In `@__tests__/pro/ui/McpSettingsSection.test.tsx`:
- Around line 29-48: The test setup in McpSettingsSection.test.tsx still
evaluates mods!.section and mods!.store even when describe.skip is used, so move
the ProToolsSettingsSection/useMcpStore destructuring behind an early if (!mods)
return guard or into the test body. Also, the type-only import aliases for
SectionModule and StoreModule force resolution of pro modules during tsc checks,
so add this test path to the exclusion list or provide a stub type declaration
for the pro package in open-core CI.
In `@__tests__/pro/ui/McpToolPickerSheet.test.tsx`:
- Around line 16-31: The theme mock in McpToolPickerSheet.test.tsx closes over a
non-mock-prefixed COLORS binding, which will break Jest transform. Rename COLORS
to mockColors or move the object directly inside the `@offgrid/core/theme`
jest.mock factory, and make sure useTheme returns that mock-prefixed value. Keep
the Feather icon shim unchanged; the fix is specifically in the mock data
referenced by useTheme.
In `@__tests__/pro/ui/McpToolsScreen.test.tsx`:
- Around line 43-58: The test setup in McpToolsScreen.test.tsx still uses a
direct relative type import for ScreenModule, which makes tsc resolve the
private pro/ module during public builds even though load() is wrapped in
try/catch. Change the type source so it does not reference the private submodule
path directly, or move this suite to the same pro-only type path pattern used by
the other private tests, keeping the existing load()/mods/describe.skip
structure intact.
In `@src/components/GenerationSettingsModal/ImageQualitySliders.tsx`:
- Around line 45-47: The ImageQualitySliders slider is receiving persisted
imageWidth values that can fall below the allowed minimum, so normalize or clamp
imageWidth before it is passed into the slider. Update the settings flow that
feeds ImageQualitySliders so stale values are coerced into the 256–512 range
before rendering, using the imageWidth field and its parent settings
loader/initializer to keep the slider handle within min={256}.
In `@src/utils/imageModelIntegrity.ts`:
- Around line 47-76: The completeness check in checkImageModelFiles only
validates MNN .weight companions, so missing graph files like clip_v2.mnn,
vae_decoder.mnn, and vae_encoder.mnn can still slip through. Update
checkImageModelFiles to explicitly require the MNN graph files as part of the
BASE_REQUIRED/requirePresent validation, alongside the existing split-weight
pairing logic, so both the graph and its matching .weight file are enforced.
---
Nitpick comments:
In `@__tests__/pro/audio/engines/OuteTTSEngine.test.ts`:
- Around line 279-291: The test for OuteTTSEngine.release is currently using a
tautological check on mockAudioApi.closed, so it never proves the AudioContext
was actually closed. Update the assertion in the release path test to verify the
exact expected closed count after calling e.release(), using the existing
OuteTTSEngine, release, and mockAudioApi symbols to confirm native audio cleanup
really happened.
In `@__tests__/unit/audio/streamingSpeech.test.ts`:
- Around line 162-181: The streamingSpeech timeout test is using a real
wall-clock sleep after `_setSpeakTimeoutForTest(20)`, which makes the
timing-dependent assertion flaky under CI load. Update the `stopping mid-segment
does NOT advance to another segment or release the engine (no wedge)` test to
use Jest fake timers instead of `setTimeout`, and advance time deterministically
so the hung `mockEngine.speak`, timeout handling, and
`stopStreamingSpeechForTurn` behavior are exercised without relying on a narrow
real-time window.
In `@src/services/activeModelService/index.ts`:
- Around line 259-270: Extract the duplicated override/non-override memory
refusal wording into a shared helper so the text and image paths stay in sync.
Add a small utility (for example, a memoryRefusalMessage-style helper) and use
it from both doLoadTextModelLocked and checkImageModelCanLoad instead of
inlining the same ternary in each place. Keep the helper parameterized by the
model/subject text and whether the caller should throw or return an error so
both paths produce identical messaging.
In `@src/services/imageGenerationService.ts`:
- Around line 161-186: The `_fail` method in `imageGenerationService` repeats
the same “retry last params” logic in both `onRetry` and `onLoadAnyway`. Extract
that shared `this._lastParams` guard and `this.generateImage(...)` re-invocation
into a small local helper inside `_fail`, then have `onRetry` and `onLoadAnyway`
call it with their respective options so the behavior stays identical but
duplication is removed.
In `@src/types/index.ts`:
- Around line 232-238: Remove the stale parenthetical note from the Message
audio-mode field comment so it no longer implies audioDurationSeconds is a
duplicate or separately declared field. Update the documentation around
audioPath, waveformData, and audioDurationSeconds in the Message type to
describe the audio-mode payload clearly, and keep the comment aligned with the
existing MediaAttachment definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33f77333-df34-41a1-b8e7-25b34fd0b1aa
📒 Files selected for processing (70)
CLAUDE.md__tests__/integration/audio/streamingStateMachine.test.ts__tests__/integration/generation/imageGenerationFlow.test.ts__tests__/integration/models/activeModelService.test.ts__tests__/integration/models/imageDownloadRecovery.test.ts__tests__/pro/audio/EngineRegistry.test.ts__tests__/pro/audio/OnDeviceEngineEmitter.test.ts__tests__/pro/audio/audioFilePlayer.test.ts__tests__/pro/audio/engines/KokoroEngine.extra.test.ts__tests__/pro/audio/engines/OuteTTSEngine.test.ts__tests__/pro/audio/engines/Qwen3TTSEngine.test.ts__tests__/pro/audio/ttsEngineSubscription.extra.test.ts__tests__/pro/audio/ttsPlayback.extra.test.ts__tests__/pro/audio/ttsStore.extra.test.ts__tests__/pro/audio/ui/AudioEmptyState.test.tsx__tests__/pro/audio/ui/AudioModeLayout.test.tsx__tests__/pro/audio/ui/EngineBridge.test.tsx__tests__/pro/audio/ui/MessageAudioMode.test.tsx__tests__/pro/audio/ui/QuickSettingsAudioRow.test.tsx__tests__/pro/audio/ui/TTSButton.test.tsx__tests__/pro/audio/ui/TTSSection.test.tsx__tests__/pro/audio/ui/VoicePickerPopover.test.tsx__tests__/pro/audio/useAudioProgressCaption.test.ts__tests__/pro/demo/demoMode.test.ts__tests__/pro/mcp/McpToolExtension.extra.test.ts__tests__/pro/mcp/adapters.test.ts__tests__/pro/mcp/mcpClient.test.ts__tests__/pro/mcp/mcpService.test.ts__tests__/pro/mcp/mcpStore.test.ts__tests__/pro/mcp/oauthClient.test.ts__tests__/pro/mcp/schemaTrim.test.ts__tests__/pro/ui/McpGuideScreen.test.tsx__tests__/pro/ui/McpServerModal.test.tsx__tests__/pro/ui/McpSettingsSection.test.tsx__tests__/pro/ui/McpToolPickerSheet.test.tsx__tests__/pro/ui/McpToolsScreen.test.tsx__tests__/rntl/components/ImageGenAdviceCard.test.tsx__tests__/rntl/components/PlaybackControls.test.tsx__tests__/rntl/onboarding/ChatScreenSpotlight.test.tsx__tests__/rntl/screens/ChatScreen.test.tsx__tests__/unit/audio/streamingSpeech.test.ts__tests__/unit/components/ModelFailureCard.test.tsx__tests__/unit/screens/ModelsScreen/imageDownloadActions.test.ts__tests__/unit/screens/ModelsScreen/imageDownloadResume.test.ts__tests__/unit/services/activeModelService.imageOverridable.test.ts__tests__/unit/services/modelFailureHandler.test.ts__tests__/unit/services/modelLoadErrors.test.ts__tests__/unit/utils/imageGenAdvice.test.ts__tests__/unit/utils/imageModelIntegrity.io.test.ts__tests__/unit/utils/imageModelIntegrity.test.tsdocs/GAPS_BACKLOG.mdjest.config.jsjest.setup.tsprosrc/components/GenerationSettingsModal/ImageQualitySliders.tsxsrc/components/ImageGenAdviceCard.tsxsrc/components/ModelFailureCard.tsxsrc/components/index.tssrc/screens/ChatScreen/ChatMessageArea.tsxsrc/screens/ModelsScreen/imageDownloadActions.tssrc/screens/ModelsScreen/imageDownloadResume.tssrc/services/activeModelService/index.tssrc/services/imageGenerationService.tssrc/services/modelFailureHandler.tssrc/services/modelLoadErrors.tssrc/stores/modelFailureStore.tssrc/types/index.tssrc/utils/imageGenAdvice.tssrc/utils/imageModelIntegrity.tstsconfig.json
Comment on lines
+29
to
+48
| type SectionModule = typeof import('@offgrid/pro/ui/McpSettingsSection'); | ||
| type StoreModule = typeof import('@offgrid/pro/mcp/mcpStore'); | ||
|
|
||
| function load(): { section: SectionModule; store: StoreModule } | null { | ||
| try { | ||
| return { | ||
| section: require(['..', '..', '..', 'pro', 'ui', 'McpSettingsSection'].join('/')), | ||
| store: require(['..', '..', '..', 'pro', 'mcp', 'mcpStore'].join('/')), | ||
| }; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| const mods = load(); | ||
| const maybe = mods ? describe : describe.skip; | ||
|
|
||
| maybe('ProToolsSettingsSection', () => { | ||
| const { ProToolsSettingsSection } = mods!.section; | ||
| const { useMcpStore } = mods!.store; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat .gitmodules 2>/dev/null
fd -t d '^pro$' -d 1
rg -n '"pro"|pro/\*\*|exclude' tsconfig.jsonRepository: off-grid-ai/off-grid-ai-mobile
Length of output: 417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## __tests__/pro/ui/McpSettingsSection.test.tsx\n'
cat -n __tests__/pro/ui/McpSettingsSection.test.tsx | sed -n '1,120p'
printf '\n## tsconfig.json\n'
cat -n tsconfig.json | sed -n '1,120p'
printf '\n## jest config files\n'
fd -a 'jest.config.*|jest.*config.*|package.json' .
printf '\n## package.json excerpt\n'
cat -n package.json | sed -n '1,220p'Repository: off-grid-ai/off-grid-ai-mobile
Length of output: 12663
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## jest.config.js\n'
cat -n jest.config.js | sed -n '1,240p'
printf '\n## search for __tests__/pro or pro path exclusions\n'
rg -n "__tests__/pro|pro/\*\*|`@offgrid/pro`|describe\.skip" -S . --glob '!pro/**'Repository: off-grid-ai/off-grid-ai-mobile
Length of output: 21176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## __tests__/unit/tools/mcpPresets.test.ts\n'
cat -n __tests__/unit/tools/mcpPresets.test.ts | sed -n '1,120p'
printf '\n## __tests__/pro/ui/McpGuideScreen.test.tsx\n'
cat -n __tests__/pro/ui/McpGuideScreen.test.tsx | sed -n '1,120p'Repository: off-grid-ai/off-grid-ai-mobile
Length of output: 10372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## declarations and stubs for `@offgrid/pro`\n'
rg -n "declare module '`@offgrid/pro`|`@offgrid/pro`" src __tests__ *.d.ts *.ts *.tsx --glob '!pro/**' --glob '!node_modules/**'Repository: off-grid-ai/off-grid-ai-mobile
Length of output: 11393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## tsconfig files\n'
fd -a 'tsconfig*.json' .
printf '\n## search for pro-dependent files excluded from tsc\n'
rg -n "__tests__/pro/|proDependentTestPaths|exclude" tsconfig*.json .github package.json jest.config.js -SRepository: off-grid-ai/off-grid-ai-mobile
Length of output: 767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## github workflows\n'
fd -a '.github/workflows' .
printf '\n## search for tsc, pro checkout, and submodule handling in CI\n'
rg -n "tsc --noEmit|noEmit|submodule|checkout.*submodules|pro/package.json|proExists|git submodule|offgrid-pro" .github . -S --glob '!pro/**' --glob '!node_modules/**'Repository: off-grid-ai/off-grid-ai-mobile
Length of output: 563
Guard the pro-only setup before the skipped describe
describe.skipstill runs the callback, somods!.section/mods!.storewill throw whenpro/is missing. Move the destructuring behindif (!mods) return;or into each test.typeof import('@offgrid/pro/...')still forces module resolution undertsc --noEmit;tsconfig.jsonexcludespro/**but not__tests__/pro/**, so this file also needs exclusion or a type stub in open-core CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/pro/ui/McpSettingsSection.test.tsx` around lines 29 - 48, The test
setup in McpSettingsSection.test.tsx still evaluates mods!.section and
mods!.store even when describe.skip is used, so move the
ProToolsSettingsSection/useMcpStore destructuring behind an early if (!mods)
return guard or into the test body. Also, the type-only import aliases for
SectionModule and StoreModule force resolution of pro modules during tsc checks,
so add this test path to the exclusion list or provide a stub type declaration
for the pro package in open-core CI.
…ix theme consts Addresses the CodeRabbit review pass: - (Major) checkImageModelFiles only enforced the .weight side of split MNN assets, so a partial extraction that dropped a .mnn GRAPH itself (clip_v2.mnn / vae_decoder.mnn) but kept its .weight passed the completeness gate. Now explicitly require the clip graph (clip_v2.mnn OR clip.mnn) + vae_decoder.mnn for mnn — the graphs the native server always loads. vae_encoder stays optional (native adds --vae_encoder only if present). +5 tests (dropped-clip-graph, base-clip accepted, dropped-vae_decoder, vae_encoder-optional); module still 100% coverage. - (Convention) rename the theme `COLORS` const → `mockColors` in the 3 UI test files that reference it inside a jest.mock factory. It passed (lazy access) but relying on a non-mock-prefixed binding in a hoisted factory is fragile; align with the repo convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ias in test Final CodeRabbit review nits: - ImageQualitySliders: clamp the displayed value to >= SWEET_SPOT_SIZE so a stale persisted imageWidth (<256) doesn't render below the slider's own min. Matches the generation floor. - TTSButton test: wrap the async speak() press in act() so store updates settle cleanly. - McpToolsScreen test: type imports use the @offgrid/pro/* alias (consistent with siblings; the skippable runtime require stays relative). McpSettingsSection already guards via mods!/describe.skip — no change needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressed the review pass (5187e11, plus earlier fixes):
Full suite green (362 suites); tsc + lint clean. |
This was referenced Jul 9, 2026
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
1 participant
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
An image-generation reliability + UX pass, triggered by an r/off_grid_ai report ("Failed to load image model, not enough memory" on a Galaxy S23 FE with no "Load Anyway" option) and extended through live on-device debugging on a OnePlus Nord (Snapdragon 765G, GPU-only). Ships several independent fixes; the TTS piece rides as a
prosubmodule bump (off-grid-ai/mobile-pro#18).Everything is fails-before/passes-after tested. Full suite green: 330 suites, 6960 tests. Verified on-device throughout.
What's in here
1. Load Anyway for image models (the original report)
OverridableMemoryErrorinstead of stringifying it, so the shared failure card offers Load Anyway (retry with{ override: true }) — parity with text.6b3e7981,94c554ff,9b6a6bcef13323ad,18e36eb5ActionButtonextracted (Retry / Load Anyway) — DRY.f8310e5a; design-spec reference added5a0b18f52. Image model extraction integrity (the real S23-class bug)
pos_emb.bin+clip_v2.mnn.weight, yet it was marked_ready; the native server then died and reported a misleading "your device may not support this backend" (the SoC was blamed for a missing-file bug).imageModelIntegrityutil: backend-agnostic completeness rule (base files + primary unet +*.mnn⇒*.mnn.weightpairing). Wired into the primary download finalize + resume (both via one sharedensureImageExtractionCompleteseam), so a partial extraction is never marked ready.ef960ed2,49439e9cImageModelIncompleteError→ UI says "re-download" (not "Load Anyway" — missing files can't be forced).ef960ed23. Image-gen UX / quality (GPU-only devices)
988c5e7e3ea60c123ea60c12,8a26aaff4. TTS streaming stop (pro submodule bump — off-grid-ai/mobile-pro#18)
141835cb,ddacfad65. Layout + logged gaps
4736866c560207d0On-device verification (OnePlus Nord, SM7250, GPU-only)
Merge order
Merge off-grid-ai/mobile-pro#18 first, re-bump this PR's
propointer to pro-main's merge commit, then merge this. Merge commits only (no squash).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes