chore: post-0.0.103 cleanup — SonarCloud reliability + promote-as-draft fix#560
Closed
alichherawalla wants to merge 3 commits into
Closed
chore: post-0.0.103 cleanup — SonarCloud reliability + promote-as-draft fix#560alichherawalla wants to merge 3 commits into
alichherawalla wants to merge 3 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
…log tag (CodeRabbit nitpick)
|
Collaborator
Author
|
Nitpicks addressed:
|
26 tasks
Collaborator
Author
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.
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