release: onboarding reliability and post-0.0.103 fixes#571
Open
alichherawalla wants to merge 20 commits into
Open
release: onboarding reliability and post-0.0.103 fixes#571alichherawalla wants to merge 20 commits into
alichherawalla wants to merge 20 commits into
Conversation
SonarCloud's quality gate flagged new_reliability_rating=D on the release. Fix the core-repo bugs: - activeDownloadPersistence: give .sort() an explicit string comparator (deterministic signature). - useVoiceDownloadItems / modelPreloader: callHook returns R|undefined, so a Promise never actually reached a boolean condition — await the optional chain (?./await undefined) instead so no Promise sits in a conditional (SonarJS no-misused-promises). - modelManager.test: downloadId is typed string; the mock returned a number and assertions compared against 42 — align every downloadId to '42' so the assertion types match at runtime. - Android delete(): use the Boolean return — log a warning on a failed best-effort cleanup, matching the existing pattern in WorkerDownload (both DownloadManagerModule + WorkerDownload). (SonarCloud #2 McpServerModal is in the pro/ submodule — separate pro PR.)
…_release_status) The android promote lane set release_status: 'draft', but a track PROMOTION reads track_promote_release_status — which defaulted to 'completed'. So scripts/promote.sh sent 0.0.103 straight to Play review/100% rollout instead of parking as a draft for a human to confirm the rollout % (device 2026-07-16). Set track_promote_release_status: 'draft' so it actually pauses.
…fit) from the owned budgets
…w snug the fit is Browse used to HIDE any model whose quants all exceeded the balanced budget (fileExceedsBudget), so a model that's loadable via reclaim credit + Load Anyway just vanished. Now browse keeps every model with a quant that isn't 'wontFit' (past the aggressive ceiling) and tags it with a small device-fit chip: Easy / Fits / Tight (fitTier owner). Only genuinely-too-big models are hidden. - useTextModels: relax the browse hide-filter to noLoadableQuant (every quant 'wontFit'); attach each model's best-quant fitTier for the chip. Recommended stays filtered by deviceRecommendation (curated cap) and search/sort are untouched. - ModelCardContent: render the fit chip in the badges row (InfoBadgesRow extracted to module scope to stay under the complexity gate); emerald for easy/fits, muted for a snug 'tight'. - TextModelsTab detail 'Available Files': relax the same way so a 'tight' model opened from browse still lists a downloadable quant (isCompatible still flags snug ones for the Load-Anyway warning). Rendered tests: a tight model now shows in browse with its chip (red-verified: revert → hidden); the detail list offers loadable quants and hides only 'wontFit' (boundary pinned at 8GB×0.75 = exactly 6.0GB). Note: cosmetic chip colors/placement want an on-device eyeball. Image-tab parity + the Home picker chip are follow-ups (same owner).
The 'loadable = fitTier !== wontFit' rule was written inline in two callers (useTextModels browse filter + TextModelsTab detail list). Move it to one owned predicate memoryBudget.isLoadableOnDevice so 'what counts as loadable on this device' is defined once; both surfaces call it. fitTier still owns the tier value for the chip. No behaviour change (verified: fit tests green).
…CodeRabbit)
{model.paramCount && …} rendered a bare '0' text node outside <Text> when paramCount/minRamGB was 0,
which crashes RN. Coerce with !! so 0/undefined render nothing (a 0-value badge is meaningless anyway).
…log tag (CodeRabbit nitpick)
Convert all POSIX [ ... ] conditionals in scripts/uat.sh to [[ ... ]] (S7688) and redirect the error() message to stderr (S7677). All operands were already quoted, so the change is behavior-neutral for this release script. Verified with bash -n.
S5906: replace generic assertions with dedicated matchers per Sonar's exact suggestions (toHaveLength / toBeNull / toBeGreaterThan / toBeLessThanOrEqual) across 36 integration test files. Every asserted value is unchanged — tests assert exactly the same things. S5976: parameterize the 3 near-identical <think>-tag stripping tests in imageGenerationFlow.test.ts into a single it.each table (coverage identical). Pure test-quality nits; no production code touched.
…HF metadata calls Fresh installs froze on 'Analyzing your device...' whenever HuggingFace was slow or down (live now: HF/AWS outage). The init effect awaited fetchModelFiles() for every recommended model before setIsLoading(false), and the HF fetch had NO timeout, so the screen sat until the OS socket timeout (~75s). Two fixes: - Render the screen as soon as device analysis completes; load the file lists in the background (they only populate download cards, which can't download during an outage anyway). Device analysis is instant and local. - Bound every HF metadata request (search, file listing) with a 5s AbortController timeout. These are small JSON APIs, not the multi-GB downloads (separate native path); no answer in 5s means broken, not slow — fail fast to a retry state. This also stops the model-detail 'Available Files' screen spinning forever during the outage.
… ceiling The 'Your Device' card labelled 'Available Memory' rendered deviceInfo.availableMemory, which the memory-budget rework changed to os_proc_available_memory (the per-process allocatable ceiling before jetsam) - a small number that reads as wrong device memory (e.g. 4.57GB on a device with more RAM). That value is a budget input, not a device spec. Show total physical RAM: the number the user expects and the same one the model recommendations gate on (getTotalMemoryGB). Budget logic unchanged.
… load During an HF outage (or any offline/timeout), tapping a model showed one of two bad states: an infinite spinner (no timeout on the shipped build), or - once the 5s timeout lands - the misleading 'No compatible files found for this model', which blames the model when the fetch actually failed. Two changes: - getModelFiles now fails fast on a timed-out/aborted request instead of paying a second 5s on the siblings fallback (which also can't reach the network). The fallback stays for its real purpose: a non-ok tree endpoint on a repo whose file list the model page serves. - Track filesLoadError distinctly from 'loaded but empty'. On failure the detail screen shows an inline 'Couldn't load files. Check your connection.' with a Retry button (re-runs the fetch) instead of a transient modal + misleading empty text.
…t retry state Two rendered integration tests, both red-verified (revert the fix -> they fail for the right reason): - deviceCardShowsTotalRam: an 8GB device with a 4.57GB process ceiling shows '8.00 GB', never '4.57 GB'. Catches the availableMemory-vs-totalMemory regression. - modelFilesLoadErrorShowsRetry: a failed file-list fetch shows 'Couldn't load files' + Retry (not 'No compatible files found'); a successful retry renders the files.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughChangesThe pull request adds RAM-based model fit tiers and compact fit badges, introduces timeout-aware model-file loading with inline retry UI, makes onboarding device analysis non-blocking, updates total-RAM display, hardens download and release scripts, and modernizes numerous integration assertions and repository instructions. Model fit and onboarding
Reliability and maintenance
Repository guidance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/huggingface.ts (1)
30-88: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTimeout clears prematurely, leaving the body stream unprotected.
fetchWithTimeoutclears the timer as soon asfetchresolves (which happens when the HTTP headers are received). Consequently,await response.json()is executed after the timeout has been cleared. If the network connection drops or the server stalls while streaming the JSON body, the app will hang indefinitely onresponse.json(), bypassing the intended 5-second limit entirely.Refactor the timeout logic into a wrapper that keeps the
AbortSignalactive until the body is fully read.🛠️ Proposed fix using a holistic timeout wrapper
Replace
fetchWithTimeoutwithwithTimeout, and updatefetchJsonandgetModelFilesto wrap the entire parse operation:- private async fetchWithTimeout(url: string, timeoutMs = HF_REQUEST_TIMEOUT_MS): Promise<Response> { + private async withTimeout<T>(operation: (signal: AbortSignal) => Promise<T>, timeoutMs = HF_REQUEST_TIMEOUT_MS): Promise<T> { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), timeoutMs); try { - return await fetch(url, { headers: { Accept: 'application/json' }, signal: controller.signal }); + return await operation(controller.signal); } finally { clearTimeout(timer); } } private async fetchJson<T>(url: string): Promise<T> { - const response = await this.fetchWithTimeout(url); - if (!response.ok) throw new Error(`API error: ${response.status}`); - return response.json() as Promise<T>; + return this.withTimeout(async (signal) => { + const response = await fetch(url, { headers: { Accept: 'application/json' }, signal }); + if (!response.ok) throw new Error(`API error: ${response.status}`); + return response.json(); + }); } // ... async getModelFiles(modelId: string): Promise<ModelFile[]> { try { - const response = await this.fetchWithTimeout(`${this.apiUrl}/models/${modelId}/tree/main`); - if (!response.ok) return this.getModelFilesFromSiblings(modelId); - const files: Array<{ type: string; path: string; size?: number; lfs?: { size: number } }> = await response.json(); - const allGguf = files.filter(f => f.type === 'file' && f.path.endsWith('.gguf')); - const mmProjFiles = allGguf.filter(f => this.isMMProjFile(f.path)); - const modelFiles = allGguf.filter(f => !this.isMMProjFile(f.path)); - return modelFiles - .map(file => ({ - name: file.path, - size: file.lfs?.size || file.size || 0, - quantization: this.extractQuantization(file.path), - downloadUrl: this.getDownloadUrl(modelId, file.path), - mmProjFile: this.findMatchingMMProj(file.path, mmProjFiles, modelId), - })) - .sort((a, b) => a.size - b.size); + return await this.withTimeout(async (signal) => { + const response = await fetch(`${this.apiUrl}/models/${modelId}/tree/main`, { headers: { Accept: 'application/json' }, signal }); + if (!response.ok) throw new Error('API Error'); // Triggers the catch block to run the sibling fallback + const files: Array<{ type: string; path: string; size?: number; lfs?: { size: number } }> = await response.json(); + const allGguf = files.filter(f => f.type === 'file' && f.path.endsWith('.gguf')); + const mmProjFiles = allGguf.filter(f => this.isMMProjFile(f.path)); + const modelFiles = allGguf.filter(f => !this.isMMProjFile(f.path)); + return modelFiles + .map(file => ({ + name: file.path, + size: file.lfs?.size || file.size || 0, + quantization: this.extractQuantization(file.path), + downloadUrl: this.getDownloadUrl(modelId, file.path), + mmProjFile: this.findMatchingMMProj(file.path, mmProjFiles, modelId), + })) + .sort((a, b) => a.size - b.size); + }); } catch (e) { // A timed-out/aborted request means the network is down — don't pay a second 5s on the🤖 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/huggingface.ts` around lines 30 - 88, Refactor fetchWithTimeout into a withTimeout wrapper that keeps the AbortController active until the entire asynchronous operation completes, including response.json(). Update fetchJson and getModelFiles to wrap both fetching and body parsing in this timeout, while preserving existing non-OK handling, abort propagation, and siblings fallback behavior.src/screens/ModelsScreen/useTextModels.ts (1)
209-231: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRace condition in
handleSelectModelstate updates.State updates are applied unconditionally after the async file fetch. If a user quickly navigates between different models, concurrent fetch responses can resolve out-of-order. This TOCTOU (Time-of-Check to Time-of-Use) race condition will overwrite
modelFileswith stale data, causing the detail view to display and eventually download files belonging to a completely different model.Guard the state updates to ensure only the latest active selection modifies the state.
🔒️ Proposed fix to ignore stale responses
(Note: ensure
useRefis included in yourreactimports)+ const fetchIdRef = useRef(0); + const handleSelectModel = async (model: ModelInfo) => { + fetchIdRef.current += 1; + const currentFetchId = fetchIdRef.current; + setSelectedModel(model); setIsLoadingFiles(true); setFilesLoadError(false); // Curated entries under the offgrid/ namespace (e.g. the synthetic LiteRT // parent) ship with their files baked into the ModelInfo — skip the // HuggingFace fetch and use them as-is. Real HF models always go through // the fetch path even when factories/mocks pre-populate model.files. if (model.id.startsWith('offgrid/') && model.files && model.files.length > 0) { setModelFiles(model.files); setIsLoadingFiles(false); return; } try { const files = await huggingFaceService.getModelFiles(model.id); + if (fetchIdRef.current !== currentFetchId) return; setModelFiles(files); } catch { // Fetch failed (offline / HF unreachable / timeout). Surface an inline retry state rather // than a transient modal — the detail view reads filesLoadError and offers Retry. + if (fetchIdRef.current !== currentFetchId) return; setModelFiles([]); setFilesLoadError(true); } finally { + if (fetchIdRef.current === currentFetchId) { setIsLoadingFiles(false); + } } };🤖 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/screens/ModelsScreen/useTextModels.ts` around lines 209 - 231, Update handleSelectModel to track the latest selection with a useRef-based request or selection token, and verify that token before applying fetched files, errors, or loading-state updates. Ensure stale responses from prior model selections cannot modify modelFiles, filesLoadError, or loading state, while preserving the existing offgrid fast path and current behavior for the active selection.
🧹 Nitpick comments (1)
src/types/index.ts (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer top-level
import typeover inline imports.Using a top-level type import is more idiomatic and improves readability compared to an inline
import()type inside the interface definition.♻️ Proposed refactor
Add the type import at the top of the file:
+import type { FitTier } from '../services/memoryBudget';And update the field definition:
/** Device-fit tier of this model's BEST (most-fitting) quant, for the browse fit chip. Set by the * models VM from memoryBudget.fitTier; undefined until the file sizes are known. */ - fitTier?: import('../services/memoryBudget').FitTier; + fitTier?: FitTier;🤖 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 25 - 27, Replace the inline import type used by the fitTier property in the relevant interface with a top-level import type for FitTier from the memoryBudget service, then reference FitTier directly in the property declaration.
🤖 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__/unit/services/huggingface.test.ts`:
- Around line 596-604: Update the test around the custom global.fetch stub to
capture the original fetch implementation and restore it in a finally block,
ensuring restoration occurs whether the test passes, fails, or aborts.
Alternatively, replace the direct assignment with a jest.spyOn mock configured
with the existing behavior and ensure it is restored after the test.
In `@CLAUDE.md`:
- Around line 1-4: Update the references in batch9-diagnostics-debuglog.test.ts
and DEVICE_TEST_LOG.md that point to CLAUDE.md, replacing them with AGENTS.md so
debug-log and gate guidance directs contributors to the canonical source. Leave
unrelated repository instructions unchanged.
---
Outside diff comments:
In `@src/screens/ModelsScreen/useTextModels.ts`:
- Around line 209-231: Update handleSelectModel to track the latest selection
with a useRef-based request or selection token, and verify that token before
applying fetched files, errors, or loading-state updates. Ensure stale responses
from prior model selections cannot modify modelFiles, filesLoadError, or loading
state, while preserving the existing offgrid fast path and current behavior for
the active selection.
In `@src/services/huggingface.ts`:
- Around line 30-88: Refactor fetchWithTimeout into a withTimeout wrapper that
keeps the AbortController active until the entire asynchronous operation
completes, including response.json(). Update fetchJson and getModelFiles to wrap
both fetching and body parsing in this timeout, while preserving existing non-OK
handling, abort propagation, and siblings fallback behavior.
---
Nitpick comments:
In `@src/types/index.ts`:
- Around line 25-27: Replace the inline import type used by the fitTier property
in the relevant interface with a top-level import type for FitTier from the
memoryBudget service, then reference FitTier directly in the property
declaration.
🪄 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: 4b441aab-6882-473d-8878-0f40b74e98ce
📒 Files selected for processing (63)
AGENTS.mdCLAUDE.md__tests__/integration/audio/voiceModeImageJourney.rendered.happy.test.tsx__tests__/integration/audio/voiceModeResendEnhancedImage.rendered.redflow.test.tsx__tests__/integration/audio/voiceModeResendImageRoutes.rendered.redflow.test.tsx__tests__/integration/chat/voiceNoteChatModeEmptyTurn.redflow.test.ts__tests__/integration/downloads/downloadedCountBadge.rendered.happy.test.tsx__tests__/integration/downloads/iosImageStagingPurgedRedownloads.redflow.test.ts__tests__/integration/downloads/iosImageStagingPurgedRedownloads.rendered.redflow.test.tsx__tests__/integration/downloads/iosInterruptedNoFailedEntry.redflow.test.ts__tests__/integration/downloads/iosTextRetryReissues.rendered.redflow.test.tsx__tests__/integration/downloads/queuedDownloadsSurviveKill.redflow.test.ts__tests__/integration/downloads/textRetryReissuesOnIosLostDownloadId.rendered.redflow.test.tsx__tests__/integration/generation/enhancementNoThinking.rendered.redflow.test.tsx__tests__/integration/generation/enhancementStreamingProgress.rendered.redflow.test.tsx__tests__/integration/generation/generationFlow.test.ts__tests__/integration/generation/imageGenerationFlow.test.ts__tests__/integration/generation/queuedForceImagePreservesMode.rendered.redflow.test.tsx__tests__/integration/generation/queuedSendFeedback.test.ts__tests__/integration/generation/reloadRaceKeepsThinking.rendered.redflow.test.tsx__tests__/integration/generation/remoteParallelTools.rendered.happy.test.tsx__tests__/integration/generation/resendImageRoutes.rendered.redflow.test.tsx__tests__/integration/generation/resendImageRoutesLlama.rendered.redflow.test.tsx__tests__/integration/generation/resendImageTurnFallsBackToText.rendered.redflow.test.tsx__tests__/integration/happy/imageBackends.happy.test.tsx__tests__/integration/happy/imageIntentRouting.happy.test.tsx__tests__/integration/happy/imageLightbox.happy.test.tsx__tests__/integration/happy/imageModeToggle.happy.test.tsx__tests__/integration/happy/imageOomCard.happy.test.tsx__tests__/integration/happy/smartBudgeting.happy.test.tsx__tests__/integration/happy/speakMessage.happy.test.tsx__tests__/integration/happy/tools.happy.test.tsx__tests__/integration/image/imageTunablesReadFreshFromStore.redflow.test.ts__tests__/integration/models/activeModelService.test.ts__tests__/integration/models/browseFitChipShowsLoadableModels.rendered.test.tsx__tests__/integration/models/detailFilesFitHintUsesOwnedBudget.rendered.test.tsx__tests__/integration/models/modelFilesLoadErrorShowsRetry.rendered.test.tsx__tests__/integration/onboarding/deviceAnalysisDoesNotWaitForNetwork.rendered.test.tsx__tests__/integration/onboarding/deviceCardShowsTotalRam.rendered.test.tsx__tests__/integration/onboarding/spotlightFlowIntegration.test.ts__tests__/integration/rag/embeddingFlow.test.ts__tests__/integration/rag/ragFlow.test.ts__tests__/integration/stores/chatStoreIntegration.test.ts__tests__/rntl/screens/ModelDownloadScreen.test.tsx__tests__/unit/services/huggingface.test.ts__tests__/unit/services/modelManager.test.tsandroid/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.ktandroid/app/src/main/java/ai/offgridmobile/download/WorkerDownload.ktfastlane/Fastfilescripts/uat.shsrc/components/ModelCardContent.tsxsrc/screens/DownloadManagerScreen/useVoiceDownloadItems.tssrc/screens/ModelDownloadScreen.tsxsrc/screens/ModelsScreen/TextModelsTab.tsxsrc/screens/ModelsScreen/index.tsxsrc/screens/ModelsScreen/styles.tssrc/screens/ModelsScreen/useModelsScreen.tssrc/screens/ModelsScreen/useTextModels.tssrc/services/activeDownloadPersistence.tssrc/services/huggingface.tssrc/services/memoryBudget.tssrc/services/modelPreloader.tssrc/types/index.ts
Comment on lines
+596
to
+604
| global.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { | ||
| requestedUrls.push(String(input)); | ||
| requestSignal = init?.signal ?? undefined; | ||
| return new Promise<Response>((_resolve, reject) => { | ||
| init?.signal?.addEventListener('abort', () => { | ||
| reject(new DOMException('The operation was aborted.', 'AbortError')); | ||
| }); | ||
| }); | ||
| }) as typeof fetch; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if `fetch` is restored globally in an `afterEach` hook within this file.
ast-grep run --pattern $'afterEach(() => { $$$ })' __tests__/unit/services/huggingface.test.tsRepository: off-grid-ai/OGAM
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline __tests__/unit/services/huggingface.test.ts --view expanded || true
echo
echo '--- search for fetch mutations / teardown ---'
rg -n "global\.fetch|jest\.spyOn\(global,\s*'fetch'|afterEach|beforeEach|afterAll|restoreAllMocks|useRealTimers|useFakeTimers" __tests__/unit/services/huggingface.test.ts
echo
echo '--- relevant slices ---'
sed -n '540,660p' __tests__/unit/services/huggingface.test.tsRepository: off-grid-ai/OGAM
Length of output: 5022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- top of file ---'
sed -n '1,120p' __tests__/unit/services/huggingface.test.ts
echo
echo '--- surrounding describe blocks around the timeout test ---'
sed -n '520,660p' __tests__/unit/services/huggingface.test.ts
echo
echo '--- later fetch-using tests after timeout block ---'
sed -n '660,760p' __tests__/unit/services/huggingface.test.tsRepository: off-grid-ai/OGAM
Length of output: 9355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '320,380p' __tests__/unit/services/huggingface.test.tsRepository: off-grid-ai/OGAM
Length of output: 1838
Restore global.fetch in finally. This test leaves a never-resolving stub on global.fetch, and there’s no file-level teardown to reset it before later tests. Capture the original value and restore it here, or switch to jest.spyOn so it can be auto-restored.
🤖 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/huggingface.test.ts` around lines 596 - 604, Update
the test around the custom global.fetch stub to capture the original fetch
implementation and restore it in a finally block, ensuring restoration occurs
whether the test passes, fails, or aborts. Alternatively, replace the direct
assignment with a jest.spyOn mock configured with the existing behavior and
ensure it is restored after the test.
Comment on lines
+1
to
+4
| # Repository instructions | ||
|
|
||
| ## Repository Layout | ||
|
|
||
| **All Pro feature code lives in the `pro/` submodule (its own git repo, `@offgrid/pro`) - not in core.** When changing or adding a Pro feature (e.g. TTS/audio, MCP/tools, and other paid surfaces), edit files under `pro/` and commit/PR them in that repo. Core only wires Pro in through the slot/hook registries; it never imports Pro code directly. Pro changes are a separate branch + PR from core (see `pro/CLAUDE.md`). | ||
|
|
||
| ## Device Logs (how to see what's actually happening on the device) | ||
|
|
||
| **RN 0.83 moved JS `console.log` off the Metro terminal into React Native DevTools, and RN's console never reaches the iOS device syslog.** So `metro` stdout, `idevicesyslog`, and `npx react-native log-ios` (simulator-only) all capture NOTHING from a physical device. Do not waste time tailing Metro for app logs. | ||
|
|
||
| Instead, a **dev-only persistent file sink** (`src/utils/debugLogFile.ts`, wired in `App.tsx` behind `__DEV__`) mirrors every `logger.*` line - which is where ALL the state-machine traces go (`[TTS-SM]`, `[GEN-SM]`, `[MODEL-SM]`, `[DL-SM]`, `[ROUTE-SM]`, `[IMG-SM]`, `[MEM-SM]`, `[FAIL-SM]`) - into a file in the app container. Pull it over the cable to read the real trace: | ||
|
|
||
| The debug (Debug-config) build's bundle id is **`ai.offgridmobile.dev`** — Debug carries a | ||
| `.dev` suffix so it installs alongside the App Store / TestFlight build (`ai.offgridmobile`, | ||
| the Release config). The log sink is `__DEV__`-only, so you are almost always pulling from the | ||
| `.dev` container. Get the device UDID from `xcrun devicectl list devices` (it is per-device — do | ||
| not hardcode one): | ||
|
|
||
| ```sh | ||
| # Read the connected device's UDID from devicectl's JSON (parsing the human-readable table with | ||
| # awk is brittle — the last column is the device model, not the UDID). Or just paste the UDID. | ||
| xcrun devicectl list devices --json-output /tmp/devs.json >/dev/null 2>&1 | ||
| DEVICE=$(python3 -c "import json;ds=json.load(open('/tmp/devs.json'))['result']['devices'];print(next(d['hardwareProperties']['udid'] for d in ds if d.get('connectionProperties',{}).get('tunnelState')=='connected'))") | ||
| xcrun devicectl device copy from \ | ||
| --device "$DEVICE" \ | ||
| --domain-type appDataContainer --domain-identifier ai.offgridmobile.dev \ | ||
| --source Documents/offgrid-debug.log --destination /tmp/offgrid-debug.log | ||
| ``` | ||
|
|
||
| (A Release/TestFlight build uses `--domain-identifier ai.offgridmobile` and has no dev log sink.) | ||
|
|
||
| Then `grep`/read `/tmp/offgrid-debug.log`. The file appends a `===== session start … =====` marker on each launch and is size-capped (rotates, keeping the tail). The in-app **Debug Logs** screen (Settings → Debug Logs) shows the same lines live for quick visual checks. **When diagnosing a device issue, pull this file rather than guessing.** | ||
|
|
||
| ## Branch Policy | ||
|
|
||
| **Never push directly to `main`.** All changes must go through a pull request: | ||
|
|
||
| 0. Always create a branch specific to the change before committing: `feat/`, `fix/`, `docs/`, `chore/`, `test/`, etc. | ||
| 1. Push the branch and open a PR - never `git push origin main`. | ||
| 2. If you find yourself on `main`, create a branch first: `git checkout -b <branch-name>`. | ||
|
|
||
| **Merge strategy: ALWAYS a merge commit. NEVER squash (and never rebase-merge).** When merging a PR, use `gh pr merge --merge` (or the "Create a merge commit" button) so the full commit history is preserved on `main`. Do not squash under any circumstances - the small, meaningful per-concern commits are the record and must survive the merge. This applies to both the core repo and the `pro` submodule. | ||
|
|
||
| ### Commit early, commit often - never lose progress (agents especially) | ||
|
|
||
| **A long task is a chain of small, GREEN, committed steps - not one giant uncommitted diff.** Agents run against context/session limits; anything uncommitted is lost when the session ends. So: | ||
|
|
||
| - **Commit each cohesive step as soon as it is green** (typecheck + the relevant tests pass), with a real per-concern message. A refactor done in slices commits after each slice, not at the end. | ||
| - **Never leave a large uncommitted working tree across a risky/long operation.** If you are about to start something big, commit what already works first so there is a clean restore point. | ||
| - **Every commit is a safe restore point:** it must be behavior-neutral-or-better and pass the gates for the files it touches. Do not commit a knowingly-broken tree; if mid-refactor is unavoidably broken, finish to green before committing (or stash), never push broken. | ||
| - **Prefer many small commits over few large ones.** They survive a merge (we never squash), make review tractable, and mean a lost session costs one step, not the whole task. | ||
| - This is not optional polish - it is how work is not lost. Treat "a lot of uncommitted changes" as a bug to fix immediately by landing them as small commits. | ||
|
|
||
| ## Copy & Content Standards | ||
|
|
||
| **Any change to website copy, essays, docs text, UI strings, or marketing content must follow the brand voice guide:** | ||
|
|
||
| - Read `docs/brand_tone_voice.md` before writing or editing any copy. | ||
| - The full quality checklist is at the bottom of that file - run every item before committing content changes. | ||
|
|
||
| Key rules that are easy to miss: | ||
|
|
||
| | Rule | Wrong | Right | | ||
| |---|---|---| | ||
| | Proof-first | "fast" | "15-30 tok/s on flagship devices" | | ||
| | Privacy as mechanism | "we value your privacy" | "the model runs in your phone's RAM, nothing is sent anywhere" | | ||
| | No exclamation marks | "It works!" | "It works." | | ||
| | No em dashes | "private - always" | "private - always" | | ||
| | No forbidden words | revolutionary, seamlessly, empower, leverage, robust, comprehensive, crucial, pivotal, delve, tapestry, testament, underscore, foster, cultivate, showcase, enhance | use specific, plain words instead | | ||
| | No AI slop phrases | "serves as", "stands as", "represents a", "marks a turning point", "it is worth noting" | just say "is" | | ||
| | No structural clichés | "Not just X, but Y" / "It's not X, it's Y" | state the thing directly | | ||
| | No curly quotes | "private" | "private" | | ||
|
|
||
| The emotional arc for all content: **Recognition -> Return -> Freedom**. Name what's been happening, show what's being given back, hand over the capability without condition. | ||
|
|
||
| --- | ||
|
|
||
| ## Design Standards | ||
|
|
||
| **Any change that touches UI (screens, components, styles) must comply with the design system.** Inherit the shared Off Grid design philosophy from **`../brand/DESIGN_PHILOSOPHY.md`** (the source of truth - brutalist/terminal, Menlo mono, emerald accent, tokens in `@offgrid/design`). Platform specifics: **`docs/design/DESIGN_PHILOSOPHY_SYSTEM.md`** + **`docs/design/VISUAL_HIERARCHY_STANDARD.md`**. | ||
|
|
||
| - Read `docs/design/VISUAL_HIERARCHY_STANDARD.md` before writing or modifying any UI code. | ||
| - Check `docs/design/` for any other relevant design documents. | ||
| - Use `TYPOGRAPHY` tokens - never hardcode font sizes or weights. | ||
| - Use `COLORS` tokens - never hardcode color values. | ||
| - Use `SPACING` tokens - never hardcode margin/padding values. | ||
| - Weights must stay ≤ 400 (no bold). | ||
| - Never use emojis or emoticons in UI text - always use `react-native-vector-icons` instead. Feather is the default; MaterialIcons is allowed only when Feather lacks a suitable icon (e.g. `whatshot` for trending). | ||
| - Never use `lucide-react` or any other icon library - only `react-native-vector-icons`. | ||
| - Follow the 5-category text hierarchy: TITLE → BODY → SUBTITLE/DESCRIPTION → META. | ||
|
|
||
| ## Reuse Before Building | ||
|
|
||
| **Before writing any new component, style, hook, or service, search for an existing one and reuse it.** Building a parallel version of something that already exists creates visual and behavioural drift (e.g. a search box that looks different from every other search box). | ||
|
|
||
| - For UI: grep `src/components/` and the relevant screen folder for an existing component or shared style (e.g. `ModelCard`, `Card`, `Button`, shared `searchContainer`/`searchInput` styles) before creating your own. Two screens that show the same kind of thing must use the same component. | ||
| - For logic: check for an existing hook/service/store action (`grep -rn`) before adding a new one. | ||
| - If an existing component is close but not exact, extend it with a prop rather than forking a copy. | ||
| - Only build new when nothing fits - and say so in the PR description. | ||
|
|
||
| ## Architecture & Abstractions (SOLID) | ||
|
|
||
| **Design to abstractions, not concrete implementations.** When there are multiple interchangeable implementations of a thing (TTS engines, model backends, providers, storage), the rest of the app must depend on a single interface/service layer - never branch on a concrete type. | ||
|
|
||
| **Before every code edit, stop and ask three questions - out loud, in the response:** | ||
|
|
||
| 1. **Is there enough here to abstract?** Two or more concrete cases handled by the same caller (text vs vision vs image models, Slack vs Mail surfaces, kokoro vs piper TTS) means there's a seam. One case, used once, is not - don't abstract speculatively (YAGNI). | ||
| 2. **Can we apply SOLID here?** Mainly: does one thing own one responsibility (SRP), and do callers depend on an interface rather than the concretes (DSP)? A `kind === 'x'` / `instanceof` / per-type `switch` in a caller - *especially in the renderer* - is the tell that the decision belongs behind a service. | ||
| 3. **Are we actually using it?** A mapping or rule must be defined ONCE and reused. If the same kind→modality map, the same routing `if`, or the same capability check appears in two layers (e.g. main process AND renderer), that's duplication, not abstraction - collapse it to a single source of truth and have both sides call it. | ||
|
|
||
| If the answer to 1 is "no", say so and write the simple version. If "yes", build the seam before piling on the second concrete branch - retrofitting after drift is the expensive path. | ||
|
|
||
| - **No leaking implementation details upward.** UI and stores must not do `instanceof SpecificEngine`, check `engineId === 'kokoro'`, or branch on capabilities to decide *how* to do something. Push that decision behind the abstraction (the engine/provider implements it; or a service layer dispatches once). If you find yourself writing `if (engine X) … else …` in a component, the abstraction is wrong. | ||
| - **Single uniform entry point.** Prefer one polymorphic method (e.g. `engine.play(text, opts)`) that every implementation satisfies over several mechanism-specific methods (`speak` vs `playFromFile`) that callers must choose between. | ||
| - **Service layer between UI and implementations.** Implementations (engines/adapters) are swappable; a service abstracts them and exposes a normalized API + state. Adding a new implementation must require zero changes to UI/store. | ||
| - **Dependency Inversion / Liskov:** any implementation must be substitutable through the interface without callers knowing which one is active. Normalize gaps (e.g. an engine that can't report playback position) inside the service, not in the UI. | ||
| - Apply the rest of SOLID: single responsibility per module, open for extension (add an implementation) / closed for modification (don't touch callers), segregated interfaces (don't force implementations to stub methods they can't support - model that with the abstraction). | ||
| - **Think from first principles and keep a reference architecture in mind.** Before changing a subsystem, know its intended shape: what owns which state and resources, and how the pieces compose. Make changes consistent with that architecture. | ||
| - **Fix the seam - never patch around a missing abstraction.** When a subsystem has shared state or resources spread across multiple implementations (e.g. audio playback: the iOS AVAudioSession + AudioContext lifecycle + playback state across the streaming-TTS / file-player / PCM-replay paths), build/extend the *single owning service* and route everything through it. Do NOT add gates, guards, or flags in callers/UI/stores to compensate for the missing owner. Point-patches layered on shared mutable state cause cascading regressions - one fix silently breaks another path - and the subsystem becomes chaotic and flaky. If the owning abstraction doesn't exist yet, that's the work: create it, then migrate every path onto it with no bypass. | ||
| - **Migrations to an owning abstraction MUST be backward-compatible / behavior-neutral for existing paths.** When you route existing code through a new service, preserve its exact prior behavior - the refactor should be *additive* (it may fix a missing case), never change a behavior callers depended on. Example: the old TTS/recorder paths re-activated the iOS AVAudioSession on *every* call; making the new session owner "idempotent" silently dropped that re-activation and broke TTS. Verify each migrated path behaves exactly as before, then layer the fix on top. | ||
| - **Reactive stores are for UI projection - NOT for coordinating side-effects or owning resources.** Zustand/reactive state is the right tool for rendering; it is the wrong source of truth for imperative coordination (audio session/context, model loads, playback control, any hardware/resource). Most of the audio flakiness came from making imperative decisions (play vs block, which session category) by branching on a reactive store snapshot that several code paths write and desync. Follow a clear presentation separation (MVVM/MVP): the **Service/Model** owns the authoritative state machine + resources + side-effects; the reactive store is a **thin read-only projection** of that service; the **View** observes the projection and dispatches *intents* to the service. Never make an imperative decision (or fire a side-effect) by reading a reactive snapshot that multiple writers can mutate - that is the recipe for the desync/race bugs. | ||
| - **State and data MUST NOT live in the presentation layer.** A screen/component/hook (the View) holds NO authoritative state, NO business logic, and NO side-effecting data operations - it observes a service's projection and dispatches intents. Concretely: no retry/cancel/delete/finalize logic, no platform-branched mechanism, no store-mutation orchestration, no "compute the real value from several sources" in a screen or a `useXxxScreen`/`useXxxManager` hook. That logic belongs in the owning **service** (which carries the state machine + permanent logs). If a UI hook is doing the work instead of calling a service, that is the bug - move the work into the service and have the hook delegate. (This is why download retry/remove moved out of `useDownloadManager`/`retryHandlers` into `ModelDownloadService` + its providers.) | ||
|
|
||
| ## Platform Abstraction (no iOS-only / Android-only bugs) | ||
|
|
||
| **A platform-specific bug is the symptom of a leaked platform detail.** With the right abstraction every bug is catchable on both platforms at once - that is the goal. We are writing ONE common layer, not two parallel apps. | ||
|
|
||
| - **One typed TS contract per native capability; both Swift and Kotlin must satisfy it.** Downloads, audio session, model load, image gen, STT - each has a single interface the JS calls. A method that exists on one platform but not the other is a contract violation, not an acceptable difference. Make the missing method a *compile error* (the TS interface requires it), never a runtime `"only available on Android"` throw. | ||
| - **Never branch on `Platform.OS` to decide HOW to do something.** Branching to choose a *mechanism* (which download path, which retry strategy, which audio setup) is the missing-abstraction smell - push that decision into the native module / a service that dispatches once. Branching for a genuine presentation value (a keyboard event name, a style inset) is fine. | ||
| - **Genuine OS capability gaps are declared DATA, not silent divergence.** When one platform truly can't do something (iOS URLSession dies on app-kill while Android WorkManager survives; an engine can't cancel), model it as a capability flag on the object (like `DownloadCapabilities`), normalize the gap ONCE inside the service, and let the UI render from the flag. The gap is then testable - never an `if (ios)` scattered through callers. | ||
| - **Contract tests run against the abstraction, so they catch both platforms.** Test the common interface + the capability flags; a single test then guards iOS and Android together. If a test can only be written per-platform, the abstraction is wrong. | ||
| - **Native module contract parity is mandatory.** The Swift and Kotlin implementations of a module must expose the SAME method names, the SAME events (names + payloads), and the SAME semantics (persistence, cleanup, error cascading). Contract drift between Swift and Kotlin is the root cause of platform-only bugs - when you touch a native module on one platform, verify/mirror the other side against the shared TS contract. | ||
|
|
||
| ## Quality Gates run on PRE-PUSH (not pre-commit) | ||
|
|
||
| **Commits are intentionally ungated so red-first / work-in-progress tests can land as small commits.** | ||
| The full quality gate runs via Husky on `git push` (`.husky/pre-push`), scoped to the files pushed | ||
| since upstream: | ||
|
|
||
| | Pushed file type | Checks that run automatically (pre-push) | | ||
| |---|---| | ||
| | `.ts` / `.tsx` / `.js` / `.jsx` | eslint, `tsc --noEmit`, `jest --findRelatedTests`, `npm run depcruise` | | ||
| | `.swift` | `npm run test:ios` | | ||
| | `.kt` / `.kts` | `npm run test:android` | | ||
|
|
||
| **Requirements:** | ||
| - SwiftLint: `brew install swiftlint` (skipped with a warning if not installed) | ||
| - Android checks require the Gradle wrapper in `android/` | ||
|
|
||
| **Workflow implication (TDD / adversarial red-first):** write a failing test, commit it red (commit is | ||
| free), then drive it green; the branch must be green before `git push` (the gate blocks a red push). | ||
| Never bypass the push gate with `--no-verify`. `core.hooksPath` is `.husky/_` (husky v9); there is no | ||
| pre-commit hook by design. | ||
|
|
||
| ## Testing (lean — this is the whole doctrine) | ||
|
|
||
| **One rendered integration test per fix. Nothing more.** | ||
|
|
||
| - Mount the real screen, arrive via real gestures, assert what the user SEES. Fakes ONLY at the device boundary (`__tests__/harness/`); never mock our own code. | ||
| - **While iterating, run ONLY that test's file.** Do NOT run `--findRelatedTests` or the whole suite per fix — the full suite runs once at pre-push (the gate is the safety net). | ||
| - **No unit tests required. No coverage thresholds.** If a mockist test (mocks our own code, or asserts `toHaveBeenCalled`) fails, DELETE it — never repair it. | ||
| - "Show the red" (stash the fix, watch it fail) is optional: do it only for genuinely new behavior, skip it for a clear bug fix. | ||
| - Confirm a device fix against the log FIRST — pull only the live-session tail (from the last `===== session start =====`), never the whole file. | ||
|
|
||
| ## Push = Create PR + Address Review | ||
|
|
||
| When the user says "push" (or any equivalent like "ship it", "send it", "push this"), follow this full workflow: | ||
|
|
||
| ### Before pushing | ||
| 0. Write tests for any new or changed logic if they don't already exist. | ||
| 1. Run `npm run lint && npx tsc --noEmit && npm test` - fix any failures before continuing. | ||
| 2. Commit all staged changes with a descriptive message. | ||
| 3. Ensure you are NOT on `main`. If you are, create an appropriately named branch first: `git checkout -b feat/...` or `fix/...` or `chore/...` etc. | ||
|
|
||
| ### Pushing & PR | ||
| 4. Push the branch: `git push -u origin <branch>` | ||
| 5. If no PR exists for this branch, create one with `gh pr create`. **Do NOT include "Generated with Codex" or any AI attribution in PR descriptions.** | ||
| 6. If a PR already exists, update its description to reflect **all commits in the PR** (not just the latest push). Read the full commit history with `git log main..HEAD` and write a coherent description that summarises the entire change set - what it does, why, and how. | ||
|
|
||
| ### Review loop | ||
| 7. Wait for Gemini to review the PR (poll with `gh pr checks` and `gh api repos/{owner}/{repo}/pulls/{number}/reviews` until a review appears). | ||
| 8. Pull down review comments: `gh api repos/{owner}/{repo}/pulls/{number}/comments` and `gh api repos/{owner}/{repo}/pulls/{number}/reviews`. | ||
| 9. Address every review comment - fix the code, re-run quality gates (lint, tsc, test). | ||
| 10. Reply to **each** review comment individually using `gh api` (`/pulls/comments/{id}/replies`). Every comment gets its own reply - do not post a single summary comment. | ||
| 11. Push fixes, update the PR description again to stay coherent across all commits. | ||
| 12. Report what was changed in response to the review. | ||
|
|
||
| ## CI Review Loop | ||
|
|
||
| The repo has three automated reviewers on every PR. After pushing, loop until all are green: | ||
|
|
||
| | Reviewer | What it checks | How to address | | ||
| |---|---|---| | ||
| | **Gemini Bot** | Code quality, style, logic issues | Read comments via `gh api`, fix code or reply explaining why it's fine, then comment `/gemini review` to trigger a fresh pass | | ||
| | **Codecov** | Test coverage thresholds | Add missing tests, ensure new code is covered. Check the Codecov report for uncovered lines | | ||
| | **SonarCloud** | Security hotspots, code smells, duplications, bugs | Fix flagged issues - especially security hotspots and duplications. Resolve quality gate failures before merging | | ||
|
|
||
| **Workflow:** | ||
| 1. Push code → wait for all three reviewers to report | ||
| 2. Pull down Gemini comments, Codecov report, and SonarCloud findings | ||
| 3. Fix issues: code changes for Gemini/SonarCloud, add tests for Codecov | ||
| 4. Re-run local quality gates (`npm run lint && npm test && npx tsc --noEmit`) | ||
| 5. Push fixes, comment `/gemini review` on the PR to re-trigger Gemini | ||
| 6. Repeat until all three reviewers pass with no blocking issues | ||
|
|
||
| ## PR hygiene (lean) | ||
|
|
||
| - One concern per PR, small diff. Ship the one rendered test that would fail without the change. | ||
| - No Provit journey, no self-audit comment, no mandatory ceremony. Multi-agent fan-out is opt-in, only when asked. | ||
| Read and follow `AGENTS.md`. It is the canonical engineering and workflow standard for this | ||
| repository. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update downstream references after moving the guidance.
__tests__/hardening/batch9-diagnostics-debuglog.test.ts and docs/DEVICE_TEST_LOG.md still point to CLAUDE.md for debug-log and gate guidance, but that content now lives in AGENTS.md. Update those references so contributors are sent to the right source.
🤖 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 `@CLAUDE.md` around lines 1 - 4, Update the references in
batch9-diagnostics-debuglog.test.ts and DEVICE_TEST_LOG.md that point to
CLAUDE.md, replacing them with AGENTS.md so debug-log and gate guidance directs
contributors to the canonical source. Leave unrelated repository instructions
unchanged.
This was referenced Jul 16, 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
Consolidates the onboarding reliability fix and the recent post-0.0.103 work into one release candidate.
Easy,Fits,Tight) instead of hiding every model beyond the balanced target.AGENTS.mdthe canonical concise engineering contract: SOLID/DRY/separation of concerns, no mocks of Off Grid code, real screen/navigation/gesture integration tests, and fakes only at uncontrollable boundaries.CLAUDE.mdnow points to it.Type of Change
Screenshots / Screen Recordings
The UI regressions are covered by rendered integration tests. Manual screenshot capture remains for release QA.
Android
Analyzing your device...11.00 GB RAMiOS
Analyzing your device...Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Supersedes #560, #561, #568, and #569.
Additional Notes
Local verification:
BUILD SUCCESSFUL, 589 tasks).Summary by CodeRabbit
New Features
Bug Fixes
Documentation