Skip to content

fix: offer one-tap model recovery instead of dead-end load alert#406

Open
bhaveshpawar07 wants to merge 3 commits into
mainfrom
fix/play-broken-functionality-model-load
Open

fix: offer one-tap model recovery instead of dead-end load alert#406
bhaveshpawar07 wants to merge 3 commits into
mainfrom
fix/play-broken-functionality-model-load

Conversation

@bhaveshpawar07

Copy link
Copy Markdown
Collaborator

Why

Google Play removed the app under the Broken Functionality policy (cited version code 1780986915, the current main build).

From the reviewer screenshot (IN_APP_EXPERIENCE-9607.png): on a mid-RAM device (~7.3 GB → ~4.4 GB safe budget) the reviewer opened Select Model → Image with a text model already loaded. Every image model they tapped produced a "Failed to Load" modal whose only action was OK. The message said "unload the other model first" but gave no way to do that from the sheet. Tap model → blocked → OK → tap another → blocked again. To the automated reviewer the app loads but nothing works → flagged as unresponsive.

Root cause

ModelSelectorModal.handleSelectImageModel caught the RAM-guard error into showAlert('Failed to Load', message) with no action buttons, so CustomAlert defaulted to a single OK button — a dead end. The existing ≤4 GB auto-unload in checkImageModelCanLoad didn't apply to the reviewer's device.

Fix

When a load is blocked because other models are already loaded (the model fits on its own, but the total exceeds the RAM budget), offer a one-tap "Unload others & load" recovery that frees memory and then loads the requested model. When the model is too large even on its own, show a clear informational alert instead of a false "unload" suggestion.

  • types.ts — add resolvableByUnload to MemoryCheckResult.
  • memory.ts — set the flag only when the model fits alone but the total is over budget.
  • ModelSelectorModal/index.tsx — image path now checks memory up front; resolvable block → recovery action, otherwise → "Model too large" info alert.
  • useChatModelActions.ts — same recovery on both text-load entry points (initiateModelLoad, handleModelSelectFn); "Load Anyway" remains as the override only when unloading can't help.

Tests

  • Integration (activeModelService.test.ts): resolvable vs. too-large resolvableByUnload + messaging.
  • Component (ModelSelectorModal.test.tsx): recovery offered (not auto-loaded), unload-then-load on confirm, no recovery when too large.
  • Unit (useChatModelActions.test.ts): text-path recovery on both entry points.

Full suite: 182 suites, 5497 passed, 0 failed. tsc and eslint clean on changed files.

Notes

  • No versionCode bump in this PR — to be bumped at resubmission.
  • Backward compatible: existing "Load Anyway" behavior is preserved when a block is not resolvable by unloading.

Google Play removed the app under the Broken Functionality policy
(version code 1780986915). A reviewer on a mid-RAM device opened
Select Model -> Image with a text model already loaded and hit a
"Failed to Load" modal on every image model they tried. The modal's
only action was OK, and the message told them to "unload the other
model first" with no way to do so from that sheet - a dead end that
reads as an unresponsive app.

Root cause: ModelSelectorModal.handleSelectImageModel caught the RAM
guard error into showAlert() with no buttons, defaulting to a lone OK.

Fix: when a load is blocked because OTHER models are already loaded
(i.e. the model fits on its own but the total exceeds the RAM budget),
offer a one-tap "Unload others & load" recovery that frees memory then
loads the requested model. When the model is too large even on its own,
show a clear informational alert instead of a false "unload" promise.

- types: add resolvableByUnload to MemoryCheckResult
- memory: set the flag when the model fits alone but the total is over budget
- ModelSelectorModal: check memory up front; show recovery vs. info alert
- useChatModelActions: same recovery on both text-load entry points,
  keeping "Load Anyway" as the override only when unloading cannot help

Adds unit, integration, and component tests for both the resolvable
and too-large paths.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a "resolvableByUnload" flag to the memory check results, allowing the application to detect when a model loading block can be resolved by unloading other active models. The UI in "ModelSelectorModal" and "useChatModelActions" has been updated to offer a convenient "Unload others & load" recovery option when this flag is true. The review feedback correctly identifies two locations in "useChatModelActions.ts" where the promise chains involving "unloadAllModels()" lack error handling. Adding ".catch()" blocks to these chains is highly recommended to prevent the UI from getting stuck in a loading state if an unload operation fails.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +104 to +108
text: 'Unload others & load', style: 'default' as const, onPress: () => {
startLoad()
.then(() => activeModelService.unloadAllModels())
.then(() => doLoadTextModel(deps));
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The promise chain initiated by startLoad() does not handle potential rejections from unloadAllModels(). If unloadAllModels() rejects, the promise chain will break, and doLoadTextModel(deps) will never be called, leaving the UI stuck in a loading state. Adding a .catch() block to handle the rejection ensures that the error is logged and the loading state is properly cleaned up.

              text: 'Unload others & load', style: 'default' as const, onPress: () => {
                startLoad()
                  .then(() => activeModelService.unloadAllModels())
                  .catch(err => logger.error('Failed to unload models:', err))
                  .then(() => doLoadTextModel(deps));
              },
References
  1. When catching errors, log them instead of swallowing them to ensure failures are visible and to aid in debugging.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8787b69. Added .catch(err => logger.error('Failed to unload models before load:', err)) to the chain so an unloadAllModels() rejection is logged and the load still proceeds via doLoadTextModel(deps) rather than leaving the UI stuck. Added a regression test asserting the load proceeds when unload rejects.

Comment on lines +226 to +229
text: 'Unload others & load', style: 'default' as const, onPress: () => {
deps.setAlertState(hideAlert());
activeModelService.unloadAllModels().then(() => proceedWithModelLoadFn(deps, model));
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The promise chain initiated by unloadAllModels() does not handle potential rejections. If unloadAllModels() rejects, the promise chain will break, and proceedWithModelLoadFn(deps, model) will never be called, leaving the UI stuck in a loading state. Adding a .catch() block to handle the rejection ensures that the error is logged and the loading state is properly cleaned up.

            text: 'Unload others & load', style: 'default' as const, onPress: () => {
              deps.setAlertState(hideAlert());
              activeModelService.unloadAllModels()
                .catch(err => logger.error('Failed to unload models:', err))
                .then(() => proceedWithModelLoadFn(deps, model));
            },
References
  1. When catching errors, log them instead of swallowing them to ensure failures are visible and to aid in debugging.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8787b69. The unloadAllModels() chain now has .catch(err => logger.error('Failed to unload models before load:', err)) before .then(() => proceedWithModelLoadFn(deps, model)), so a rejection is logged and the load still proceeds. Added a regression test for the rejection path.

@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.86%. Comparing base (fbd22c7) to head (010d925).

Files with missing lines Patch % Lines
src/components/ModelSelectorModal/index.tsx 89.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   81.82%   81.86%   +0.03%     
==========================================
  Files         241      241              
  Lines       12835    12863      +28     
  Branches     3535     3540       +5     
==========================================
+ Hits        10502    10530      +28     
+ Misses       1403     1402       -1     
- Partials      930      931       +1     
Files with missing lines Coverage Δ
src/screens/ChatScreen/useChatModelActions.ts 80.48% <100.00%> (+1.13%) ⬆️
src/services/activeModelService/memory.ts 90.90% <100.00%> (+0.18%) ⬆️
src/services/activeModelService/types.ts 100.00% <ø> (ø)
src/components/ModelSelectorModal/index.tsx 82.17% <89.47%> (+3.60%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Addresses Gemini review on PR #406:
- Both text-path "Unload others & load" promise chains now .catch() a
  rejection from unloadAllModels(), log it, and still proceed to load so
  the UI is never left stuck in a loading state (the failure mode this PR
  set out to remove). The image path already wraps unload in try/catch.

Also resolves the SonarCloud "21% duplication on new code" gate failure
by extracting shared test helpers (mockResolvableBlock, alertButton,
flushRecoveryChain in the unit test; blockedImageStore, mockMemoryCheck,
renderImageTab in the component test).

Adds regression tests asserting the load still proceeds when
unloadAllModels rejects, on both text-load entry points.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@bhaveshpawar07

Copy link
Copy Markdown
Collaborator Author

/gemini review

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
6.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a resolvableByUnload flag to the memory check results, allowing the application to offer a one-tap "Unload others & load" recovery action when a model cannot be loaded due to other active models occupying memory. This prevents users from hitting a dead-end alert when trying to load a model that fits within the device's budget on its own. The changes are integrated into the model selector modal and chat model actions, accompanied by comprehensive integration and unit tests. The feedback suggests improving the user experience in handleModelSelectFn by displaying a loading indicator and waiting for a render frame before unloading models, preventing the UI from appearing frozen during the unloading process.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +227 to +232
text: 'Unload others & load', style: 'default' as const, onPress: () => {
deps.setAlertState(hideAlert());
activeModelService.unloadAllModels()
.catch(err => logger.error('Failed to unload models before load:', err))
.then(() => proceedWithModelLoadFn(deps, model));
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In handleModelSelectFn, when the user triggers the "Unload others & load" recovery action, unloadAllModels() is called immediately without showing a loading indicator first. Since unloading models from memory can take several seconds, the UI may appear frozen or unresponsive during this time.

To ensure a responsive user experience (consistent with initiateModelLoad), we should set the loading state and wait for a render frame before calling unloadAllModels().

            text: 'Unload others & load', style: 'default' as const, onPress: () => {
              deps.setAlertState(hideAlert());
              deps.setIsModelLoading(true);
              deps.setLoadingModel(model);
              deps.modelLoadStartTimeRef.current = Date.now();
              waitForRenderFrame()
                .then(() => activeModelService.unloadAllModels())
                .catch(err => logger.error('Failed to unload models before load:', err))
                .then(() => proceedWithModelLoadFn(deps, model));
            },

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 220a992. handleModelSelectFn now sets the loading state and waits for a render frame before calling unloadAllModels(), so the loading indicator is visible during the unload instead of the UI looking frozen — consistent with initiateModelLoad. Added an assertion that the indicator is shown before the unload begins.

Addresses Gemini review on PR #406: the "Unload others & load" recovery
in handleModelSelectFn called unloadAllModels() immediately with no
loading indicator, so the UI looked frozen during the multi-second
unload. Now sets the loading state and waits for a render frame before
unloading, consistent with initiateModelLoad. Asserts the indicator
shows before unload begins.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@bhaveshpawar07

Copy link
Copy Markdown
Collaborator Author

/gemini review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant