chore: post-0.0.103 cleanup — SonarCloud reliability + promote-as-draft fix#560
chore: post-0.0.103 cleanup — SonarCloud reliability + promote-as-draft fix#560alichherawalla wants to merge 2 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.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe changes update background-download tests for string identifiers, log failed Android cleanup deletions, handle optional hooks, make download persistence ordering deterministic, and configure draft status for Play Store track promotion. ChangesDownload lifecycle handling
Optional hook handling
Play Store promotion configuration
Estimated code review effort: 3 (Moderate) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
🧹 Nitpick comments (2)
android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt (1)
166-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse existing constant for the log tag.
The module already defines
const val NAME = "DownloadManagerModule"in its companion object. Consider reusing this constant instead of a hardcoded string.♻️ Proposed refactor
- if (file.exists() && !file.delete()) Log.w("DownloadManagerModule", "Failed to delete cancelled download file: ${file.path}") + if (file.exists() && !file.delete()) Log.w(NAME, "Failed to delete cancelled download file: ${file.path}")🤖 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 `@android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt` at line 166, Update the warning log in the cancelled-download cleanup branch to use the existing companion-object constant NAME as its tag instead of the hardcoded "DownloadManagerModule" string, preserving the current message and behavior.__tests__/unit/services/modelManager.test.ts (1)
513-515: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deleting mockist assertions rather than repairing them.
As per coding guidelines, we should
delete mockist tests that mock application code or assert toHaveBeenCalled rather than repairing them.Consider removing these assertions or the test entirely instead of maintaining them.Also applies to: 541-541
🤖 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/modelManager.test.ts` around lines 513 - 515, Remove the mockist call-count assertions for onProgress, onComplete, and onError in the affected model manager test cases, including the corresponding assertion at the second referenced location; if those tests exist solely to verify mocked application interactions, delete the tests entirely rather than replacing the assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/unit/services/modelManager.test.ts`:
- Around line 513-515: Remove the mockist call-count assertions for onProgress,
onComplete, and onError in the affected model manager test cases, including the
corresponding assertion at the second referenced location; if those tests exist
solely to verify mocked application interactions, delete the tests entirely
rather than replacing the assertions.
In
`@android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt`:
- Line 166: Update the warning log in the cancelled-download cleanup branch to
use the existing companion-object constant NAME as its tag instead of the
hardcoded "DownloadManagerModule" string, preserving the current message and
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31046b67-f586-4d34-8a04-8dd383bac6d6
📒 Files selected for processing (7)
__tests__/unit/services/modelManager.test.tsandroid/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.ktandroid/app/src/main/java/ai/offgridmobile/download/WorkerDownload.ktfastlane/Fastfilesrc/screens/DownloadManagerScreen/useVoiceDownloadItems.tssrc/services/activeDownloadPersistence.tssrc/services/modelPreloader.ts
|
1 participant
Follow-up to the 0.0.103 release. Two concerns, both concrete and testable:
1. SonarCloud reliability (the quality gate that failed at merge —
new_reliability_rating=D)Resolves the core-repo new-code bugs:
.sort()given an explicit string comparator (deterministic signature).callHookreturnsR | undefined, so no Promise actually reached a boolean condition; rewritten asawait …?.catch()/await callHook(...)so SonarJSno-misused-promisesis satisfied. Behaviour-neutral.downloadIdis typedstring; the mock returned a number and assertions compared to42. Aligned everydownloadIdto'42'so runtime + type + assertions match.delete()— the ignoredBooleanreturn is now logged on a failed best-effort cleanup (DownloadManagerModule + WorkerDownload), matching the existing pattern in WorkerDownload.2. promote.sh lands Play production as a DRAFT
The android promote lane set
release_status: 'draft', but a track promotion readstrack_promote_release_status— which defaulted tocompleted, sopromote.shsent 0.0.103 straight to Play review/100% rollout instead of pausing as a draft. Addedtrack_promote_release_status: 'draft'.Deliberately NOT in this PR
pro/submodule → separate pro-repo PR.Gates: tsc / eslint / knip / depcruise clean; affected jest suites green (the one pre-push failure was a known heavy-memory flake — passes in isolation).
Summary by CodeRabbit