fix(#510 audit): Load-Anyway everywhere, reclaim-aware gate, single-owner dedup + rendered tests#558
Merged
Merged
Conversation
…se scripts uat.sh (beta) and release.sh (production/local) now post the release notes to Slack via scripts/notify-slack-release.mjs after the GitHub release is cut, before the notes tempfile is removed. Fail-soft (|| true) so a chat post can never fail a shipped build. The webhook is read from the gitignored .env.keygen (the local secret store that already holds the RevenueCat/Resend/Keygen/Cloudflare tokens), mirroring the SLACK_WEBHOOK_URL repo secret the CI release workflows use. Extracted with a scoped sed for that one key, not a full source, so the other secrets in .env.keygen stay out of the script env. notify-slack-release.mjs no-ops when the webhook is unset, so this is a no-op on machines without it.
release.sh omitted CHANNEL_LABEL, so prod announcements rendered with no channel tag while uat.sh betas showed `beta`. Pass CHANNEL_LABEL=stable for parity — the notify script's documented values are beta | stable.
…STemporaryDirectory Root cause of the SDXL (Core ML) image-model download failing on iOS with "...coreml_apple_...xl-base-ios couldn't be opened because there is no such file": a completed single-file download was staged in NSTemporaryDirectory(), which iOS reaps across app relaunches and under memory pressure. The staged zip's path is persisted (localUri) and finalize (moveCompletedDownload -> unzip) can now run AFTER a relaunch (the queued-survival rework), by which point the OS has already deleted the temp file -> unzip opens a zip that no longer exists. A multi-GB image zip is the worst case: it commonly spans a backgrounding/ relaunch before its unzip finishes. Since CoreML is iOS-only, the shared download-infra change surfaced it only on iOS. Stage completed downloads (both the URLSession delegate hand-off copy and the handleSingleFileCompletion move) in a DURABLE Documents/.download-staging dir that survives relaunch + memory pressure, excluded from iCloud backup (dir flag is not inherited, so the staged file is excluded per-file too). Native change — device-verified separately (jest fakes the native module).
…ble on retry When an image zip download completed but its staged bytes are gone at finalize (iOS temp-purge on pre-durable-staging builds, or the user cleared storage), resumeZipDownload rethrew moveCompletedDownload's "no such file" and the row stuck at 'failed'. Retry re-ran the same finalize on the same dead download, so tapping Retry did nothing (retryImageDownload's tryResumeImageFinalization handles it and never falls through to the fresh-download path). Now, when neither a valid zip nor an extracted dir survives, resumeZipDownload re-downloads from scratch via proceedWithDownload (descriptor reconstructed from the entry's persisted metadata) instead of dead-ending. The user recovers by re-downloading rather than being stuck on a permanently-failed card. Red-flow test drives the real retry-finalize path over a faked native boundary where moveCompletedDownload rejects (staged source purged) and no artifact survives; asserts a fresh native download is issued + the row goes active again. Verified red (reverted fallback -> stuck 'failed', no new row) then green.
…uard on both platforms Addresses hygiene/tests self-audit on the retry-recovery fix: - DRY: extract imageDescriptorFromMetadata into a pure zero-IO module (imageDescriptor.ts), used by BOTH the iOS retry path (retryHandlers) and resume's re-download fallback, instead of two hand-built descriptors that could drift. - Copy: drop the em dash from the user-facing re-download failure message (brand guide). - Tests: resumeImageDownload is shared (iOS retry + Android app-open resume), so the red-flow now runs the purged-artifact recovery on BOTH ios and android with a device-faithful fixture each (CoreML zip / MNN zip). Red-verified on both (reverted fallback -> stuck 'failed', no new row).
…rd, assert recovery Mounts the real DownloadManagerScreen over the native+fs fakes, taps the actual failed-retry-button on the failed SDXL card, and asserts the card recovers (row no longer 'failed', Retry gone, a fresh native download in flight). Closes the /tests gap: the sibling .redflow.test.ts drives the service directly; this drives the real UI gesture through the full retry wiring. Pins Platform.OS=ios (the device bug) so imageProvider.retry takes the iOS re-download path, not Android's native-resume branch. Red-verified (reverted the reDownloadFromMetadata fallback -> card stuck 'failed'). registerCoreDownloadProviders() before mount, else the service silently refuses retry (no owning provider).
… fix/ios-download-durable-staging
…ny mode The pre-load gate (llmSafetyChecks.resolveSafeContext) threw a plain Error when weights exceed available RAM, so loadModelWithOverride fell to the dead-end OK-only alert with no Load Anyway (device: 12GB, Aggressive, ~6.7GB model refused with no override). Throw OverridableMemoryError instead — the pure typed discriminant loadModelWithOverride already keys the Load Anyway branch on. Any mode, guaranteed override. Red-flow test drives the real resolveSafeContext over an injected memory sensor; red on HEAD (plain Error → isOverridableMemoryError false → dead-end), green after.
…from a blocked whisper load
…es inline channel reasoning
…uses shared REASONING_DELIMITERS Defect 1: stripControlTokens now strips the XML-style <function=…>…</function> tool-call block (plus its unclosed-at-EOS tail) the tool-loop extractor recognizes, via new shared XML_TOOL_CALL_* marker sources in messageContent that BOTH the extractor (parseXmlStyleToolCall) and the stripper key on — so they cannot drift. Defect 2: deltaHasThinking probes inline content through the shared REASONING_DELIMITERS grammar instead of a hardcoded <think>, so a remote model emitting Gemma/Qwen channel reasoning inline (no reasoning_content field) is no longer misdetected as non-thinking.
…erForTranscription so a blocked load recovers The realtime path called whisperStore.loadModel() directly; a 'blocked' single-model memory refusal returns (does not throw), so execution fell through to startRealtimeTranscription → 'No Whisper model loaded' — a silent dead-end with no recovery. Inject the SAME free-generation-then-retry recovery the file path already uses (ensureWhisperForTranscription) into useWhisperTranscription, so both dictation modes share ONE recovery owner and a memory-blocked dictation recovers instead of dead-ending.
- stripControlTokens strips <function=…>…</function> via shared markers so the extractor (parseXmlStyleToolCall) and display stripper key on ONE grammar (DR7). - deltaHasThinking probes via shared REASONING_DELIMITERS, not a hardcoded <think>. Both red→green, test-first.
…th recovery Route realtime hold-to-talk through ensureWhisperForTranscription (free the generation model, retry the whisper load) — the SAME owner the Audio-mode file path already uses. A memory-blocked dictation now recovers instead of dead-ending with an error haptic + empty composer. One recovery path, no parallel branch. Red->green, test-first.
…ines.ts owner (DRY) isLiteRTActive/isUsingRemote and the two re-inlined activeServer+hasProvider+!localLoaded checks (callLLMWithRetry, selectEffectiveSchemas) now delegate to getActiveEngineService() and isRemoteTextModelActive() so 'which engine is active' lives in one place. Behavior-neutral in the loop's runtime context (readiness is gated before the loop); aligns with the sibling generationServiceHelpers.isLiteRTActive.
…nical isMMProjFile (red)
…al isMMProjFile (DRY) The local copy matched only 'mmproj' and missed 'projector'/'clip', so an orphaned projector sidecar with those names slipped past getParentRows and hydrated as a phantom standalone model. Delegates to src/services/mmproj.ts (the single source), still exported for restore.ts.
… the loose rule (defect 2a) it.failing evidence: findMatchingMMProj pairs a wrong-arch E4B projector to an E2B model at the same quant. NOT migrated to the strict pickMmProjForModel because that would regress the generic single-projector download path (bare mmproj-F16.gguf) the existing getModelFiles test relies on.
…-owner - generationToolLoop: isLiteRTActive/isUsingRemote route through the engines.ts owner (getActiveEngineService/isRemoteTextModelActive), no inline re-derivation. Auto-merged cleanly with the parsing fix (different functions); both verified. - downloadHydration.isMmProjFileName delegates to canonical isMMProjFile (now catches projector/clip names). - huggingface.findMatchingMMProj strict migration NOT shipped: would regress generic single-projector vision downloads to text-only. Documented as an it.failing + follow-up recommendation (own download-listing matcher in mmproj.ts). All test-first, red->green; 1739-test blast radius green.
…s a no-op (no Retry affordance)
…flow) Drives the real store speak() -> ttsPlayback.speakMessage -> initializeEngine -> modelResidencyManager over the device-memory harness. A resident peer sidecar whose native unload rejects makes the override force refuse (fits:false); asserts a tts failure lands in the real failure store, overridable, with an onLoadAnyway - the user-facing Load Anyway affordance, not a silent bail.
…dMultiplier so it matches the residency chip
…lDownloadService.retry)
Delete the inline Platform.OS retry state machine in TextModelsTab (a second live copy
of textProvider.retry's Android branch) and the iOS proceedDownload() no-op. Retry now
routes through modelDownloadService.retry(uniformDownloadId('text', modelKey)) — the same
owner the Download Manager screen uses — which owns the platform decision AND the
lost-downloadId re-issue case. The failed card no longer gates its Retry on a downloadId,
so a rehydrated app-killed iOS entry that lost its downloadId is retriable, not a no-op.
…leExceedsBudget verdict Guards the DRY collapse: the list filter must offer exactly the files fileExceedsBudget says fit. Proven red by drifting the inline formula (over-budget quant then leaks in).
…Speech (red-flow) Mounts the real AudioMessageBubble, taps Play, asserts the dispatched speech text equals prepareMessageForSpeech(transcript) for a markdown + control-token input. RED: the bubble uses stripMarkdownForSpeech only, leaving the <think> block in.
… fileExceedsBudget Replace the three hand-rolled 'size/GB < ramGB*modelBudgetFraction' copies (the file-card isCompatible badge + the Available-Files list filter in TextModelsTab, and the browse-list RAM filter in useTextModels) with the ONE owned device-budget primitive fileExceedsBudget, so no fit verdict can drift from the others (or from the download-warning/picker verdicts).
…nd when no image model is loaded
…nd resend Extract dispatchResolvedTurn as the single owner of the post-decision routing (image-model guard + text-model provisioning + image-fallback note). Both handleSendFn/dispatchGenerationFn and regenerateResponseFn now route through it, so resend can no longer fire the image pipeline unconditionally (it falls back to text like send when no image model is loaded) nor skip the text-provision path on an image-only device. The normal model-loaded case is behavior-identical.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
…' into fix/510-load-anyway-memory
|
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:
📝 WalkthroughWalkthroughThis PR adds extensive integration/unit test coverage for prior ChangesRelease
Estimated code review effort: 4 (Complex) | ~60 minutes Release infrastructure and packaging
Sequence Diagram(s)sequenceDiagram
participant User
participant GenerationService
participant dispatchResolvedTurn
participant ImagePipeline
User->>GenerationService: send turn `#2` with imageMode=force (queued)
GenerationService->>GenerationService: coalesce queue, preserve force imageMode
GenerationService->>dispatchResolvedTurn: drain queue with imageMode
dispatchResolvedTurn->>ImagePipeline: generate image
ImagePipeline-->>dispatchResolvedTurn: handled: true
sequenceDiagram
participant TTSStore
participant ModelResidencyManager
participant ModelFailureCard
TTSStore->>ModelResidencyManager: speak() triggers eviction
ModelResidencyManager-->>TTSStore: unload() rejects (memory refusal)
TTSStore->>ModelFailureCard: register TTS failure
ModelFailureCard-->>TTSStore: render "Load Anyway" affordance
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…s 'Off Grid AI') The Debug config carries the .dev bundle id and installs alongside the release app; give it a distinct home-screen name too. Point Info.plist CFBundleDisplayName at the per-config INFOPLIST_KEY_CFBundleDisplayName build setting (Debug='Off Grid AI Debug', Release='Off Grid AI') so the name is config-specific instead of a shared literal.
…-anyway, SDXL, debug name)
…ResolvedDispatch, XML_TOOL_CALL_FUNCTION_CLOSER, stripMarkdownForSpeech) CI 'architecture' job failed on knip (unused exports), not depcruise. The refactors left these exported but only used within their own file. De-export (keep the decls). No external importer (verified across src/pro/tests). knip + tsc clean.
…st pins deltaHasThinking The verifier's mutation (M10) survived: an always-true deltaHasThinking passed the positive-only test (inverting .some(includes) is trivially true over the multi- delimiter list). Add a PLAIN-content probe case (no channel opener, no reasoning field) asserting the toggle is ABSENT — now a broken/always-true deltaHasThinking fails the negative case. Verified: mutating the channel branch to always-true now KILLS the discriminator (was surviving). Payload shapes match the real wire capture (docs/wire-captures part4/part8: gemma emits '<|channel>thought' inline, no reasoning_content).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
__tests__/integration/memory/pickerRamMatchesResidencyChip.rendered.redflow.test.tsx (1)
126-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
currentModelPathprop.The
ModelSelectorModalcomponent no longer acceptscurrentModelPathas a prop (as documented in its source code: "Callers no longer pass it"). You can safely remove it from the test.♻️ Proposed refactor
- const picker = rtl.render(React.createElement(ModelSelectorModal, { - visible: true, onClose: () => {}, onSelectModel: () => {}, onUnloadModel: () => {}, isLoading: false, - currentModelPath: llmService.getLoadedModelPath(), - })); + const picker = rtl.render(React.createElement(ModelSelectorModal, { + visible: true, onClose: () => {}, onSelectModel: () => {}, onUnloadModel: () => {}, isLoading: 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 `@__tests__/integration/memory/pickerRamMatchesResidencyChip.rendered.redflow.test.tsx` around lines 126 - 129, Remove the unused currentModelPath prop and its llmService.getLoadedModelPath() value from the ModelSelectorModal invocation in the picker test, leaving the remaining props unchanged.src/components/ChatInput/Voice.ts (1)
50-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMemoize
ensureWhisperto prevent hook dependency churn.
ensureWhisperis passed intouseWhisperTranscription, which uses it as a dependency for its internalstartRecordingcallback. Since it is currently recreated on every render, it will causestartRecordingto be unnecessarily redefined on each render.Wrap it in
useCallbackto maintain a stable reference.♻️ Proposed refactor
- const ensureWhisper = (): Promise<boolean> => ensureWhisperForTranscription({ - isLoaded: () => whisperService.isModelLoaded(), - hasDownloadedModel: () => !!downloadedModelId, - loadWhisper: () => useWhisperStore.getState().loadModel(), - // keepSelection=true so routing reloads the right generation model after the - // transcript decides text-vs-image. - freeGenerationModels: () => activeModelService.unloadAllModels(true).then(() => {}), - }); + const ensureWhisper = useCallback((): Promise<boolean> => ensureWhisperForTranscription({ + isLoaded: () => whisperService.isModelLoaded(), + hasDownloadedModel: () => !!downloadedModelId, + loadWhisper: () => useWhisperStore.getState().loadModel(), + // keepSelection=true so routing reloads the right generation model after the + // transcript decides text-vs-image. + freeGenerationModels: () => activeModelService.unloadAllModels(true).then(() => {}), + }), [downloadedModelId]);🤖 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/ChatInput/Voice.ts` around lines 50 - 57, Wrap the ensureWhisper function passed to useWhisperTranscription in useCallback so its reference remains stable across renders. Include the referenced services and downloadedModelId in the dependency list, preserving the existing ensureWhisperForTranscription behavior and callbacks.__tests__/integration/audio/ttsSpeakMemoryRefusalLoadAnyway.redflow.test.ts (1)
98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the visible text to reflect what the user actually sees.
While checking for the test ID works, verifying the explicit visible text better aligns with the coding guidelines to "assert what the user sees" and standard
@testing-library/react-nativebest practices.Consider querying by the text output directly (as is done in the other integration test).
🛠️ Proposed refactor
- expect(view.queryByTestId('model-failure-load-anyway-tts')).not.toBeNull(); - expect(view.queryByTestId('model-failure-tts')).not.toBeNull(); // the tts failure card itself is on screen + expect(view.queryByText('Load Anyway')).not.toBeNull();🤖 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__/integration/audio/ttsSpeakMemoryRefusalLoadAnyway.redflow.test.ts` around lines 98 - 100, Update the ModelFailureCard assertions in the test to query for the explicit visible text rendered for the TTS failure card instead of relying on the model-failure-tts test ID; retain the existing load-anyway assertion unless the corresponding visible text is already established by the neighboring integration test.Source: Coding guidelines
__tests__/integration/memory/loadAnywayCardRendered.redflow.test.tsx (1)
61-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure the store subscription is deterministically cleaned up.
If the test fails on an assertion before
stopSync()is called, the subscription will leak into subsequent tests, potentially causing flaky cross-test contamination. Wrap the operations in atry/finallyblock to guarantee the cleanup always runs.🧹 Proposed refactor to ensure cleanup
// Real app wiring: App.tsx boots this so the settings toggle drives the residency manager. const stopSync = startLoadPolicySync(); - // GESTURE: turn on Aggressive via the real segmented control (the device was in Aggressive). - const toggle = h.rtl.render(React.createElement(ModelLoadingModeSelector, {})); - h.rtl.fireEvent.press(toggle.getByTestId('model-loading-mode-aggressive-button')); - await h.rtl.waitFor(() => { expect(require('../../../src/services/modelResidency').modelResidencyManager.getLoadPolicy()).toBe('aggressive'); }); - - // Precondition: no refusal surface yet. - expect(h.view!.queryByText('Load Anyway')).toBeNull(); - - // GESTURE: the real first-send lazy load → residency admits → resolveSafeContext refuses. - await h.tapSend('hello'); - - // TERMINAL ARTIFACT: the override alert offers "Load Anyway", AND its body carries resolveSafeContext's - // signature ("it needs ~") so this can only pass when THAT gate (the fix site) refuses — no false-green - // on the residency gate. RED on HEAD: plain Error → dead-end "Failed to load model", no "Load Anyway". - await h.rtl.waitFor(() => { - expect(h.view!.queryByText('Load Anyway')).not.toBeNull(); - }, { timeout: 8000 }); - expect(h.view!.queryByText(/it needs ~/)).not.toBeNull(); - expect(h.view!.queryByText(/Failed to load model/)).toBeNull(); - - stopSync(); + try { + // GESTURE: turn on Aggressive via the real segmented control (the device was in Aggressive). + const toggle = h.rtl.render(React.createElement(ModelLoadingModeSelector, {})); + h.rtl.fireEvent.press(toggle.getByTestId('model-loading-mode-aggressive-button')); + await h.rtl.waitFor(() => { expect(require('../../../src/services/modelResidency').modelResidencyManager.getLoadPolicy()).toBe('aggressive'); }); + + // Precondition: no refusal surface yet. + expect(h.view!.queryByText('Load Anyway')).toBeNull(); + + // GESTURE: the real first-send lazy load → residency admits → resolveSafeContext refuses. + await h.tapSend('hello'); + + // TERMINAL ARTIFACT: the override alert offers "Load Anyway", AND its body carries resolveSafeContext's + // signature ("it needs ~") so this can only pass when THAT gate (the fix site) refuses — no false-green + // on the residency gate. RED on HEAD: plain Error → dead-end "Failed to load model", no "Load Anyway". + await h.rtl.waitFor(() => { + expect(h.view!.queryByText('Load Anyway')).not.toBeNull(); + }, { timeout: 8000 }); + expect(h.view!.queryByText(/it needs ~/)).not.toBeNull(); + expect(h.view!.queryByText(/Failed to load model/)).toBeNull(); + } finally { + stopSync(); + } }, 30000);🤖 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__/integration/memory/loadAnywayCardRendered.redflow.test.tsx` around lines 61 - 83, Wrap the test operations after startLoadPolicySync() in a try/finally block so stopSync() is always invoked, including when an assertion or interaction fails. Keep the existing gesture, assertions, and timeout unchanged, and move cleanup into the finally block.
🤖 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 `@docs/GAPS_BACKLOG.md`:
- Around line 286-287: Update the log-inspection instruction in GAPS_BACKLOG.md
to use a precise extended regular expression matching complete state-machine
tags such as [IMG-SM] or [MEM-SM], rather than the overbroad [*-SM] character
class. Preserve the surrounding debugging steps and scope.
- Line 282: Replace each em dash in the specified Markdown copy with commas,
colons, or parentheses: docs/GAPS_BACKLOG.md lines 282, 310, and 331, and
docs/RELEASE_DEVICE_CHECKLIST_pr558.md lines 7, 25, and 41. Preserve the wording
and remove any curly quotes if present in these changed sections.
- Around line 293-306: Update the listed deferred entries in GAPS_BACKLOG.md to
reflect their implementation in PR `#558`: remove them or mark them resolved,
including relevant test and commit references where available. Ensure the
onboarding warning, mockist-test removal, queued imageMode preservation, strict
mmproj matching, and reclaim-aware pre-load gating no longer appear as
outstanding work.
- Around line 335-341: The STT hard-failure backlog entry must explicitly define
whether the Load Anyway requirement applies after the final retry fails. Update
the objective/checklist around resolveTranscription and reportModelFailure to
either require an onLoadAnyway card for terminal STT failures or clearly
document the terminal-failure carve-out while retaining recovery behavior.
In `@src/hooks/useWhisperTranscription.ts`:
- Around line 187-200: Add a startNonce ref alongside isCancelled in the hook,
increment it in stopRecording and clearResult to invalidate pending starts, and
capture its value in startRecording before awaiting ensureModelReady(). After
the await and before resetting cancellation state or activating the microphone,
abort if the captured nonce differs from the current nonce.
In `@src/screens/ChatScreen/useChatGenerationActions.ts`:
- Around line 489-508: The text-route setup in the image fallback currently
excludes image requests because of the !shouldGenerateImage condition. Update
the guard in the routing function containing shouldGenerateImage and
ensureTextModelForChat so missing text models trigger ensureTextModelForChat()
whenever execution falls through to the text route, including image requests
without an active image model.
---
Nitpick comments:
In `@__tests__/integration/audio/ttsSpeakMemoryRefusalLoadAnyway.redflow.test.ts`:
- Around line 98-100: Update the ModelFailureCard assertions in the test to
query for the explicit visible text rendered for the TTS failure card instead of
relying on the model-failure-tts test ID; retain the existing load-anyway
assertion unless the corresponding visible text is already established by the
neighboring integration test.
In `@__tests__/integration/memory/loadAnywayCardRendered.redflow.test.tsx`:
- Around line 61-83: Wrap the test operations after startLoadPolicySync() in a
try/finally block so stopSync() is always invoked, including when an assertion
or interaction fails. Keep the existing gesture, assertions, and timeout
unchanged, and move cleanup into the finally block.
In
`@__tests__/integration/memory/pickerRamMatchesResidencyChip.rendered.redflow.test.tsx`:
- Around line 126-129: Remove the unused currentModelPath prop and its
llmService.getLoadedModelPath() value from the ModelSelectorModal invocation in
the picker test, leaving the remaining props unchanged.
In `@src/components/ChatInput/Voice.ts`:
- Around line 50-57: Wrap the ensureWhisper function passed to
useWhisperTranscription in useCallback so its reference remains stable across
renders. Include the referenced services and downloadedModelId in the dependency
list, preserving the existing ensureWhisperForTranscription behavior and
callbacks.
🪄 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: 197e6058-3cce-452e-8671-8939e07b5934
📒 Files selected for processing (51)
__tests__/integration/audio/audioBubbleSpeechTextSingleSource.redflow.test.tsx__tests__/integration/audio/ttsSpeakMemoryRefusalLoadAnyway.redflow.test.ts__tests__/integration/audio/whisperRealtimeBlockedRecovers.redflow.test.tsx__tests__/integration/downloads/iosImageStagingPurgedRedownloads.redflow.test.ts__tests__/integration/downloads/iosImageStagingPurgedRedownloads.rendered.redflow.test.tsx__tests__/integration/downloads/orphanProjectorPhantomHydration.rendered.redflow.test.tsx__tests__/integration/downloads/textRetryReissuesOnIosLostDownloadId.rendered.redflow.test.tsx__tests__/integration/generation/queuedForceImagePreservesMode.rendered.redflow.test.tsx__tests__/integration/generation/remoteGemmaChannelThinkingToggle.rendered.test.tsx__tests__/integration/generation/resendImageTurnFallsBackToText.rendered.redflow.test.tsx__tests__/integration/generation/toolLoopEngineOwnerRouting.test.ts__tests__/integration/memory/curatedLiteRTOverBudgetWarning.rendered.redflow.test.tsx__tests__/integration/memory/loadAnywayCardRendered.redflow.test.tsx__tests__/integration/memory/pickerRamMatchesResidencyChip.rendered.redflow.test.tsx__tests__/integration/memory/textPreloadGateReclaimAware.rendered.redflow.test.tsx__tests__/integration/models/detailFilesFitHintUsesOwnedBudget.rendered.test.tsx__tests__/integration/models/mmProjHydrationClassify.test.ts__tests__/rntl/components/ModelSelectorModal.test.tsx__tests__/rntl/screens/ModelDownloadScreen.test.tsx__tests__/unit/services/huggingface.test.ts__tests__/unit/services/huggingfaceProjectorStrictness.test.ts__tests__/unit/stores/remoteModelCapabilities.test.ts__tests__/unit/utils/messageContent.test.tsdocs/GAPS_BACKLOG.mddocs/RELEASE_DEVICE_CHECKLIST_pr558.mdios/DownloadManagerModule.swiftios/OffgridMobile.xcodeproj/project.pbxprojios/OffgridMobile/Info.plistproscripts/release.shscripts/uat.shsrc/components/ChatInput/Voice.tssrc/components/ModelSelectorModal/TextTab.tsxsrc/hooks/useWhisperTranscription.tssrc/screens/ChatScreen/useChatGenerationActions.tssrc/screens/ChatScreen/useChatScreen.tssrc/screens/DownloadManagerScreen/retryHandlers.tssrc/screens/ModelDownloadScreen.tsxsrc/screens/ModelsScreen/TextModelsTab.tsxsrc/screens/ModelsScreen/imageDescriptor.tssrc/screens/ModelsScreen/imageDownloadResume.tssrc/screens/ModelsScreen/useTextModels.tssrc/services/downloadHydration.tssrc/services/generationService.tssrc/services/generationToolLoop.tssrc/services/huggingface.tssrc/services/llm.tssrc/services/llmSafetyChecks.tssrc/services/mmproj.tssrc/stores/remoteModelCapabilities.tssrc/utils/messageContent.ts
💤 Files with no reviewable changes (1)
- tests/rntl/components/ModelSelectorModal.test.tsx
|
|
||
| ## Cosmetic voice-mode label (deferred from the 0.0.103 device session) | ||
|
|
||
| **Verdict: instrument-and-revisit.** During the 0.0.103-beta device session, two fixes landed (Lean |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace em dashes in the changed Markdown copy.
Use commas, colons, or parentheses instead. As per coding guidelines, content must avoid em dashes and curly quotes.
docs/GAPS_BACKLOG.md#L282-L282: replace the em dash introducing the cosmetic-label clause.docs/GAPS_BACKLOG.md#L310-L310: replace the em dash in the device-finding heading.docs/GAPS_BACKLOG.md#L331-L331: replace the em dash in the partial-fixes heading.docs/RELEASE_DEVICE_CHECKLIST_pr558.md#L7-L7: replace the em dash in the HIGH-priority heading.docs/RELEASE_DEVICE_CHECKLIST_pr558.md#L25-L25: replace the em dash in the MEDIUM heading.docs/RELEASE_DEVICE_CHECKLIST_pr558.md#L41-L41: replace the em dash in the LOW heading.
📍 Affects 2 files
docs/GAPS_BACKLOG.md#L282-L282(this comment)docs/GAPS_BACKLOG.md#L310-L310docs/GAPS_BACKLOG.md#L331-L331docs/RELEASE_DEVICE_CHECKLIST_pr558.md#L7-L7docs/RELEASE_DEVICE_CHECKLIST_pr558.md#L25-L25docs/RELEASE_DEVICE_CHECKLIST_pr558.md#L41-L41
🤖 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 `@docs/GAPS_BACKLOG.md` at line 282, Replace each em dash in the specified
Markdown copy with commas, colons, or parentheses: docs/GAPS_BACKLOG.md lines
282, 310, and 331, and docs/RELEASE_DEVICE_CHECKLIST_pr558.md lines 7, 25, and
41. Preserve the wording and remove any curly quotes if present in these changed
sections.
Source: Coding guidelines
Comment on lines
+286
to
+287
| `offgrid-debug.log` from the `.dev` container, grep the `[*-SM]` traces while entering voice mode, read | ||
| the actual rendered label value, then fix in `pro/audio/` UI. NOT a release hazard. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a precise regex for state-machine traces.
[*-SM] is a character class, not a pattern for tags such as [IMG-SM] or [MEM-SM]; it can overmatch unrelated characters. Use a precise command, for example grep -E '\[[A-Z]+-SM\]'.
🤖 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 `@docs/GAPS_BACKLOG.md` around lines 286 - 287, Update the log-inspection
instruction in GAPS_BACKLOG.md to use a precise extended regular expression
matching complete state-machine tags such as [IMG-SM] or [MEM-SM], rather than
the overbroad [*-SM] character class. Preserve the surrounding debugging steps
and scope.
Comment on lines
+293
to
+306
| - **Onboarding litert download-warning unreachable** (`ModelDownloadScreen.tsx:299`): fix is code-ready | ||
| (route the over-budget-but-warnable card through the owned `curatedLiteRTDownloadWarning`) but blocked | ||
| by a mockist test `__tests__/rntl/screens/ModelDownloadScreen.test.tsx:607` that asserts the buggy | ||
| pre-filter. Per doctrine: update/delete that mockist test, then land the fix. | ||
| - **`ModelSelectorModal.test.tsx` is mockist** (jest.mocks our stores/services/hardware) — 44 tests over a | ||
| fake store. The RAM-parity fix is really proven by `pickerRamMatchesResidencyChip.rendered.redflow.test.tsx`; | ||
| replace this file with rendered coverage post-release. Source carries a harmless `s.settings?.` to keep it green. | ||
| - **Queued-message imageMode carry** (`useChatGenerationActions`/`generationService`/`useChatScreen`): a | ||
| force-image send that gets queued loses its force flag (re-decided at 'auto' on drain). Needs the | ||
| QueuedMessage interface + drain handler edited together — own PR. | ||
| - **huggingface.findMatchingMMProj strict migration**: keep the generic-single-projector case, refuse a | ||
| projector naming a DIFFERENT model (E4B for E2B). Own download-listing matcher in mmproj.ts. See the | ||
| it.failing at `huggingfaceProjectorStrictness.test.ts`. | ||
| - **Reclaim-aware pre-load gate**: in progress on its own branch (device-verify on 12GB Android before merge). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile these deferred items with PR #558.
The supplied PR objectives state that the onboarding warning, mockist-test removal, queued imageMode preservation, strict mmproj matching, and reclaim-aware pre-load gating are implemented in this PR. Leaving them marked as deferred makes the backlog and release status contradictory; remove them or mark them resolved with the relevant test/commit references.
🤖 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 `@docs/GAPS_BACKLOG.md` around lines 293 - 306, Update the listed deferred
entries in GAPS_BACKLOG.md to reflect their implementation in PR `#558`: remove
them or mark them resolved, including relevant test and commit references where
available. Ensure the onboarding warning, mockist-test removal, queued imageMode
preservation, strict mmproj matching, and reclaim-aware pre-load gating no
longer appear as outstanding work.
Comment on lines
+489
to
+508
| const shouldGenerateImage = kind === 'image'; | ||
| if (shouldGenerateImage && deps.activeImageModel) { | ||
| logger.log('[ROUTE-SM] dispatch → IMAGE pipeline'); | ||
| await handleImageGenerationFn(deps, { prompt: opts.text, conversationId: opts.conversationId, attachments: opts.attachments, skipUserMessage: opts.imageSkipsUserMessage }); | ||
| return { handled: true }; | ||
| } | ||
| logger.log(`[ROUTE-SM] dispatch → TEXT generation (shouldGenerateImage=${shouldGenerateImage})`); | ||
| // Text route, no text model selected (image-only device): load one / open selector. | ||
| if (!shouldGenerateImage && deps.hasTextModel === false && !deps.activeModelInfo?.isRemote) { | ||
| const ready = await deps.ensureTextModelForChat(); | ||
| if (!ready) { | ||
| opts.onTextModelUnavailable(); | ||
| return { handled: true }; | ||
| } | ||
| } | ||
| let messageText = appendAttachmentText(opts.text, opts.attachments); | ||
| if (shouldGenerateImage && !deps.activeImageModel) messageText = `[User wanted an image but no image model is loaded] ${messageText}`; | ||
| return { handled: false, messageText }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant range.
ast-grep outline src/screens/ChatScreen/useChatGenerationActions.ts --view expanded || true
echo '--- lines around dispatchResolvedTurn ---'
sed -n '440,535p' src/screens/ChatScreen/useChatGenerationActions.ts
echo '--- callers around dispatchGenerationFn / regenerateResponseFn ---'
sed -n '520,640p' src/screens/ChatScreen/useChatGenerationActions.ts
echo '--- search for related tests ---'
rg -n "resendImageTurnFallsBackToText|ensureTextModelForChat|hasTextModel|activeImageModel|shouldGenerateImage" src test __tests__ . || trueRepository: off-grid-ai/OGAM
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test file(s) if found by the search.
for f in $(rg -l "resendImageTurnFallsBackToText|ensureTextModelForChat|hasTextModel" src test __tests__ . 2>/dev/null | head -n 20); do
echo "--- FILE: $f ---"
wc -l "$f"
sed -n '1,260p' "$f"
doneRepository: off-grid-ai/OGAM
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fallback: inspect the file directly if the previous AST map is unavailable.
sed -n '440,640p' src/screens/ChatScreen/useChatGenerationActions.ts
echo '--- test file candidates mentioning the route ---'
rg -n "resendImageTurnFallsBackToText|regenerateResponseFn|dispatchGenerationFn|ensureTextModelForChat|onTextModelUnavailable" src test __tests__ .Repository: off-grid-ai/OGAM
Length of output: 35326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact test file around the fallback case, then inspect the surrounding block.
test_file=$(rg -l "resendImageTurnFallsBackToText" src test __tests__ . | head -n 1 || true)
if [ -n "${test_file:-}" ]; then
echo "--- FILE: $test_file ---"
rg -n -C 8 "resendImageTurnFallsBackToText|ensureTextModelForChat|hasTextModel|activeImageModel" "$test_file"
fiRepository: off-grid-ai/OGAM
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow structural check: does the text-provisioning guard depend only on shouldGenerateImage?
python3 - <<'PY'
from pathlib import Path
p = Path('src/screens/ChatScreen/useChatGenerationActions.ts')
text = p.read_text()
needle = "if (!shouldGenerateImage && deps.hasTextModel === false && !deps.activeModelInfo?.isRemote)"
print("guard_present:", needle in text)
print("count_shouldGenerateImage:", text.count("shouldGenerateImage"))
PYRepository: off-grid-ai/OGAM
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact surrounding lines around the alleged bug to inspect the control flow.
nl -ba src/screens/ChatScreen/useChatGenerationActions.ts | sed -n '486,510p'Repository: off-grid-ai/OGAM
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Inspecting target file and nearby tests'
nl -ba src/screens/ChatScreen/useChatGenerationActions.ts | sed -n '450,510p'
echo '---'
rg -n -C 6 "resendImageTurnFallsBackToText|ensureTextModelForChat|onTextModelUnavailable|hasTextModel|activeImageModel" src test __tests__ . || trueRepository: off-grid-ai/OGAM
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('__tests__/unit/hooks/useChatGenerationActions.test.ts')
lines = p.read_text().splitlines()
for start, end, label in [
(650, 710, 'image fallback / ensureTextModelForChat'),
(790, 840, 'dispatchGenerationFn text routing'),
(500, 545, 'regenerateResponseFn recordedKind=image'),
]:
print(f'--- {label} ({start}-{end}) ---')
for i in range(start, min(end, len(lines)) + 1):
print(f'{i:4d}: {lines[i-1]}')
PYRepository: off-grid-ai/OGAM
Length of output: 9896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, spans in {
'src/screens/ChatScreen/useChatScreen.ts': [(190, 240), (250, 270)],
'src/screens/ChatScreen/useChatGenerationActions.ts': [(489, 506), (582, 604)],
}.items():
print(f'=== {path} ===')
lines = Path(path).read_text().splitlines()
for start, end in spans:
print(f'--- {start}-{end} ---')
for i in range(start, min(end, len(lines)) + 1):
print(f'{i:4d}: {lines[i-1]}')
PYRepository: off-grid-ai/OGAM
Length of output: 8078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Quick deterministic check of the bug condition from the source itself:
python3 - <<'PY'
from pathlib import Path
text = Path('src/screens/ChatScreen/useChatGenerationActions.ts').read_text()
print("provision_guard_requires_not_image =", "!shouldGenerateImage && deps.hasTextModel === false" in text)
print("fallback_note_present =", "no image model is loaded" in text)
PYRepository: off-grid-ai/OGAM
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/screens/ChatScreen/useChatGenerationActions.ts')
lines = p.read_text().splitlines()
for start, end, label in [
(351, 455, 'startGenerationFn'),
(274, 350, 'generateWithCompactionRetry / resolveToolsAndPrompt'),
]:
print(f'--- {label} ({start}-{end}) ---')
for i in range(start, min(end, len(lines)) + 1):
print(f'{i:4d}: {lines[i-1]}')
PYRepository: off-grid-ai/OGAM
Length of output: 12932
Image fallback still skips text-model provisioning. When kind === 'image' but activeImageModel is missing, this seam falls through to the text route with shouldGenerateImage still true, so deps.hasTextModel === false never reaches ensureTextModelForChat(). Both send and resend then go straight into the text executor without opening the selector or stashing a pending turn. Drop the !shouldGenerateImage guard so the text-route setup runs whenever this function resolves to text.
🤖 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/ChatScreen/useChatGenerationActions.ts` around lines 489 - 508,
The text-route setup in the image fallback currently excludes image requests
because of the !shouldGenerateImage condition. Update the guard in the routing
function containing shouldGenerateImage and ensureTextModelForChat so missing
text models trigger ensureTextModelForChat() whenever execution falls through to
the text route, including image requests without an active image model.
…delRam owner (was a 1.8x copy) The picker's image RAM number was a local 1.8x estimate, divergent from every other image-RAM surface. Route it through hardwareService.estimateImageModelRam (single owner) so surfaces agree. Verdict (fileExceedsBudget, 'may not fit') unchanged. Text-half backend-aware unification (needs a settings dep on this HomeScreen picker) deferred as low/cosmetic — verdict is authoritative; logged.
…eImageModelRam owner (was a 1.8x copy)" This reverts commit bc33700.
The single-message test only hit the all[0] shortcut; the coalesce branch (all.length>1 → imageMode = all.some(force)) was untested (verifier M16 surviving mutant). Add a case that queues two sends (one forced) behind an in-flight turn and asserts the merged dispatch draws. Verified: dropping the .some(force) now KILLS this case (was surviving).
…ills the >=/> mutant The verifier's >=→> mutant survived (no test straddled exact equality). Add a BOUNDARY case on a 4GB device where 0.50 fraction = EXACTLY 2.0GB (integer bytes, the only tier where equality is reachable — 0.60×6GB=3.6GB isn't integer). A file exactly at 2.0GB must be hidden (>=), 1 byte under must show. Verified: >=→> now KILLS this case. M5a closed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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__/integration/models/detailFilesFitHintUsesOwnedBudget.rendered.test.tsx`:
- Around line 94-103: Replace the jest.doMock for huggingFaceService with a
network-boundary stub, such as intercepting fetch responses for the Hugging Face
API requests used by the test. Return equivalent model, file, and download data
through those responses while exercising the real huggingFaceService
implementation and preserving the existing assertions.
In `@docs/GAPS_BACKLOG.md`:
- Line 351: Update the headings in the `#510` audit section and the other
referenced headings to remove em dashes, replacing them with acceptable
punctuation or spacing while preserving the headings’ meaning.
🪄 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: 4c283a2a-5b82-4da4-b663-770011f663b7
📒 Files selected for processing (6)
__tests__/integration/generation/queuedForceImagePreservesMode.rendered.redflow.test.tsx__tests__/integration/generation/remoteGemmaChannelThinkingToggle.rendered.test.tsx__tests__/integration/models/detailFilesFitHintUsesOwnedBudget.rendered.test.tsxdocs/GAPS_BACKLOG.mdsrc/screens/ChatScreen/useChatGenerationActions.tssrc/utils/messageContent.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/generation/queuedForceImagePreservesMode.rendered.redflow.test.tsx
- tests/integration/generation/remoteGemmaChannelThinkingToggle.rendered.test.tsx
Comment on lines
+94
to
+103
| jest.doMock('../../../src/services/huggingface', () => ({ | ||
| huggingFaceService: { | ||
| searchModels: jest.fn(async () => [modelInfo]), | ||
| getModelFiles: jest.fn(async () => [underBudget, atBudget]), | ||
| getModelDetails: jest.fn(async () => modelInfo), | ||
| getDownloadUrl: (m: string, f: string, r = 'main') => `https://hf.co/${m}/resolve/${r}/${f}`, | ||
| formatModelSize: jest.fn(() => '2.0 GB'), | ||
| formatFileSize: jest.fn((b: number) => `${(b / GB).toFixed(2)} GB`), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Do not mock application code in integration tests.
As per coding guidelines, integration tests must "fake only device boundaries and never mock application code." By mocking huggingFaceService, this test bypasses the real application logic.
Please remove jest.doMock and instead stub the network boundary (e.g., by intercepting fetch to return the appropriate JSON responses for the Hugging Face API) so the actual huggingFaceService logic is fully exercised.
🤖 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__/integration/models/detailFilesFitHintUsesOwnedBudget.rendered.test.tsx`
around lines 94 - 103, Replace the jest.doMock for huggingFaceService with a
network-boundary stub, such as intercepting fetch responses for the Hugging Face
API requests used by the test. Return equivalent model, file, and download data
through those responses while exercising the real huggingFaceService
implementation and preserving the existing assertions.
Source: Coding guidelines
|
|
||
| --- | ||
|
|
||
| ## #510 audit — STT-terminal + embedding: VERIFIED WORKING-AS-DESIGNED (not bugs, do NOT "fix") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the em dashes from these headings.
As per coding guidelines, Markdown content must avoid em dashes.
Also applies to: 364-364, 371-371
🤖 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 `@docs/GAPS_BACKLOG.md` at line 351, Update the headings in the `#510` audit
section and the other referenced headings to remove em dashes, replacing them
with acceptable punctuation or spacing while preserving the headings’ meaning.
Source: Coding guidelines
…ip the cancel pill Two coupled defects in the hold-to-record mic (chat/asSendButton mode), root-caused from a [VoiceButton-SM] trace of press → cold-load → release: 1. Slide-to-cancel severed on cold load. The instant a press triggered a model load, deriveVoiceButtonState flipped to 'loading' and the component EARLY-RETURNED a bare spinner, UNMOUNTING the hold-to-record view and its PanResponder while the finger was still down. The gesture (hold, slide-to-cancel, release) had nothing left to act on, and no responderRelease/terminate reached a handler (the ghost-recording window). Fix: the loading/transcribing spinner may only REPLACE the button in audio mode (tap-to-toggle, no hold). Chat mode keeps ONE gesturable wrapper mounted across ready/loading/transcribing and swaps only the inner face (ChatButtonFace). The 'Slide to cancel' hint now also shows during the load, since the finger is down. 2. Cancel pill text clipped. styles.cancelHint was position:absolute with only left:-100 and NO width; as an absolute child of the ~44px button container it shrink-fit to that width and wrapped/clipped 'Slide to cancel'. Fix: explicit width, anchor by right (just left of the mic), single-line text, weight 400 (was 500, over the design ceiling). Adds a permanent [VoiceButton-SM] render-state log (the crux trace). Extracts AudioBusyFace + ChatButtonFace to module scope to hold the component under the complexity gate.
… starts no ghost (#558) startRecording now awaits ensureModelReady() (which can free the generation model and reload whisper — seconds on device). If the mic is released/cancelled during that await, the stop already ran, so proceeding into startRealtimeTranscription activated a session no stop event could reach — a ghost recording holding the mic + model resident forever (#558 CodeRabbit). Fix: a session-intent nonce captured before the await; stopRecording/clearResult bump it; the start aborts if it changed on resume. Paired with the VoiceRecordButton gesture-continuity fix, the release now actually reaches stopRecording during the load. Rendered red-flow test: mount ChatScreen, hold the whisper load open (device-boundary fake), release the mic mid-load, resolve the load — assert NO realtime session is active. Verified by trace: release lands → nonce bumped → 'Start superseded during model load — aborting'. Red-check confirmed the test guards the nonce specifically (neutralize the guard → session activates → red).
…shows during load) jsdom runs no real Yoga layout, so the pixel clip itself can't be measured — but the two rendered properties that caused vs cure it can: an explicit numeric width on the cancel pill (before: none → shrink-fit to the ~44px button → wrapped/clipped) and numberOfLines=1 on the label. Adds a voice-cancel-hint testID and asserts both, plus that the pill is present during a cold model load (the gesture-continuity fix keeps the hold cancellable while the model warms). Red-verified all three: remove width → red, remove numberOfLines → red, revert the load guard so chat-mode loading early-returns a bare spinner → pill absent → red.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/VoiceRecordButton/styles.ts`:
- Around line 176-191: Update the cancelHint and cancelHintText styles to
replace hardcoded spacing and typography values with the existing SPACING and
TYPOGRAPHY design tokens. Map right and paddingHorizontal/paddingVertical in
cancelHint, and fontSize in cancelHintText, while preserving the current layout
and text appearance.
🪄 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: 2e61254e-cc78-4a27-98a5-430dfb4579a7
📒 Files selected for processing (4)
__tests__/integration/audio/whisperStartSupersededNoGhost.redflow.test.tsxsrc/components/VoiceRecordButton/index.tsxsrc/components/VoiceRecordButton/styles.tssrc/hooks/useWhisperTranscription.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useWhisperTranscription.ts
Comment on lines
176
to
191
| cancelHint: { | ||
| position: 'absolute' as const, | ||
| left: -100, | ||
| right: 52, | ||
| width: 132, | ||
| paddingHorizontal: 12, | ||
| paddingVertical: 6, | ||
| backgroundColor: `${colors.primary}40`, | ||
| borderRadius: 12, | ||
| alignItems: 'center' as const, | ||
| }, | ||
| cancelHintText: { | ||
| color: colors.primary, | ||
| fontSize: 12, | ||
| fontWeight: '500' as const, | ||
| fontWeight: '400' as const, | ||
| textAlign: 'center' as const, | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace hardcoded typography and spacing values with design system tokens.
As per coding guidelines, UI changes must avoid hardcoded typography, margins, or padding, and instead use the provided TYPOGRAPHY and SPACING tokens. The newly added cancelHint and cancelHintText styles use hardcoded values (e.g., right: 52, paddingHorizontal: 12, paddingVertical: 6, fontSize: 12).
🤖 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/VoiceRecordButton/styles.ts` around lines 176 - 191, Update
the cancelHint and cancelHintText styles to replace hardcoded spacing and
typography values with the existing SPACING and TYPOGRAPHY design tokens. Map
right and paddingHorizontal/paddingVertical in cancelHint, and fontSize in
cancelHintText, while preserving the current layout and text appearance.
Source: Coding guidelines
…l, cold-load continuity, no ghost)
…ingle owner) Device 2026-07-15: with a voice model downloading, Home → Models → Speech showed the plain download icon with no progress, while the Transcription tab showed the live bar. The picker read ONLY whisperStore.downloadProgressById; the tab read the canonical downloadStore (+ whisper fallback). A download tracked canonically (started elsewhere / rehydrated after relaunch) was invisible to the picker — a DRY/SOLID break (the rule lived inline in two places and diverged). Fix: extract the ONE owner useSttDownloadState (pure deriveSttDownloadState + a thin hook merging the canonical tracker with the whisper-store fallback). Both surfaces consume it, so they can never disagree. Picker now renders the transferring % and a queued clock. Rendered test seeds the canonical store (native boundary output) and asserts the row; red-verified by neutralizing the canonical read.
Device 2026-07-15: while holding to record, the 'Slide to cancel' pill was barely visible and overlapped the mic/+ (the thumb sits on the mic). Two causes: - opacity was tied to slide distance (outputRange [1,0]) → ~invisible at rest, only appearing once you started sliding; now baseline 0.85, brightening to 1 as you approach the cancel zone. - it sat BESIDE the mic (right:52), landing under the thumb; now floats ABOVE the mic (bottom:54, right:0) with a solid surface fill + emerald border so it reads against the chat background. Visual polish — needs an on-device eyeball; the layout guard test (width + single-line + shown during load) still holds.
Replaces the floating corner 'Slide to cancel' pill (barely visible, overlapped the mic/thumb — device 2026-07-15) with the standard push-to-talk affordance, verified on Android + iOS: - While holding, the composer pill shows an inline hint: a recording dot + '‹ Slide to cancel' centred, with the mic to its right where the thumb is (ChatInput.RecordingHint). Always visible, never overlapping the mic. - The mic scales up on press (jumps to 1.4x + gentle breathe) so it's obvious the hold registered. - The corner pill (cancelHint/cancelHintText) is removed from VoiceRecordButton; the PanResponder (hold + slide-to-cancel + release) and the gesture-continuity-through-load fix are unchanged. Extracted RecordingHint + ComposerIconsRow to keep ChatInput's render under the line-count gate. VoiceRecordButton test now guards gesture continuity (mic stays mounted during a cold load) since the hint moved out of that component.
The useSttDownloadState refactor made TranscriptionModelsTab read the derivation from the single owner (real stores), so the two mockist tab tests that seeded mockWhisperState.downloadProgressById proved nothing (mock of our own store). Deleted per test doctrine; the fallback path (whisper-store progress with no canonical entry → downloading/queued) is now covered by real renders of the picker.
…/retriable cards Device 2026-07-15: on iOS, a hard app-kill drops the URLSession task, so an in-flight download had no native row on relaunch AND no in-memory entry (downloadStore isn't persisted) — strandInterrupted Entries had nothing to carry forward, so the download vanished entirely from the Download Manager. The 'never let it silently vanish' rule only ever covered same-process (background→foreground) interruptions, not a cold kill. Fix (shared JS, platform-neutral — no Android/WorkManager touch): persist the IN-FLIGHT set (running/processing, NOT queued — the queue owns its own persistence) on status-change only, never on byte-progress ticks (activeDownloadPersistence). hydrateDownloadStore loads it and strands any entry whose native row is gone as failed/retriable. Android's WorkManager row survives a kill and reappears in the native snapshot, so it's in hydratedKeys and never stranded — zero Android behaviour change. Tests: cold-kill strand (persisted in-flight + empty native → failed card), red-verified by neutralizing the persisted-prior load; plus a guard that a still-live native row is never falsely stranded (the Android-no-regression case).
…o retry loop)
Device 2026-07-15: a local model (Qwythos GGUF) emitted tool-call arguments with CURLY double quotes
— {“expression”:“7 * 7”} — which strict JSON.parse rejects. parseToolCall then fell back to args={},
so the calculator got an EMPTY call, schema validation failed, and the model retried the same call
in a loop until it happened to emit straight quotes ('worked eventually').
Fix: parseToolArguments tries strict JSON first (unchanged for the common case), then retries once
with curly double quotes normalized to straight — only the string DELIMITERS, never content. The real
expression now reaches the tool on the first try. Tested through the real generateWithToolsImpl path
(fake engine boundary emits the curly-quote tool_call), red-verified by reverting the normalization.
NOTE: the raw <tool_call>…</tool_call> / [Tool Result] text seen in the reply was the model QUOTING
its own calls inside its <think> reasoning (rendered as thinking text) — the display stripper already
handles real tool-call blocks (messageContent.ts). That's the model narrating, not a strip miss.
Device 2026-07-16: during 'Refining Image' the real preview thumbnail is shown, but the placeholder <Icon name="image"> stayed in the header and overlapped/duplicated the image. Gate it on !imagePreviewPath so it only shows in the pre-preview 'Generating Image' phase. Rendered test asserts the placeholder tracks imagePreviewPath; red-verified by removing the gate.
…sh gate CI's 'architecture' job runs knip as a hard 0-gate, but the local pre-push hook only ran dependency-cruiser — so unused-export failures slipped through to CI twice. Fix both: - de-export deriveSttDownloadState / serializeActiveDownloads / SttDownloadEntry (used only within their own module + exercised through the real render/hydration tests, never imported by symbol); - add 'npm run knip' to the pre-push architecture gate so this class of failure is caught locally.
|
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.
Closes the confirmed gaps from the deep audit of #510 (refactor/parse-once-boundary, 527 commits) — where several commit-message guarantees ("Load-Anyway-always", "unify reclaim across BOTH paths", "single owner / DRY") were not fully delivered by the code. Every fix is test-first, red-verified at the rendered layer where a user-visible surface exists.
Fixes (each with a rendered red-flow test unless noted)
Honesty
Summary by CodeRabbit