feat(mobile): add app update management#14
Open
firstedition0123 wants to merge 7 commits into
Open
Conversation
- Adds mobile/capacitor.config.ts with appId net.bawes.universe - Configures server.url to universe.bawes.net (no local build needed) - Adds mobile/package.json with cap sync/open scripts and Fastlane gem - Adds mobile/Gemfile for Fastlane (shared by Android + iOS issues) - Adds mobile/.gitignore excluding platforms and node_modules - Adds mobile/README.md with full setup and workflow docs - Adds .github/workflows/mobile-ci.yml for PR validation Closes BAWES-Universe#2
- Upgrade Capacitor from v6 (EOL) to v8.3.4 (latest stable) - Move @capacitor/push-notifications and @capacitor/splash-screen to dependencies (not devDependencies) so cap sync works in CI - Add mobile/tsconfig.json so tsc --noEmit runs with strict options - Remove || true from tsc step — failures now surface in CI output - Fix cap doctor || true comment to be clear it's intentionally non-blocking - Add .github/workflows/mobile-ci.yml to push.paths (was missing, caused workflow file changes on universe branch to not trigger the job) - Fix misleading Gemfile comment: fastlane-plugin-versioning is for build/version bumping, not App Store Connect API auth - Add mobile/package-lock.json (generated from package.json) so npm cache-dependency-path in CI resolves correctly
The tsc --noEmit step was failing because @capacitor/cli types are not
available until npm install runs against a real lockfile, and the stub
package-lock.json we committed cannot be resolved by npm ci.
Fix: convert capacitor.config.ts -> capacitor.config.js (plain CommonJS).
Capacitor 7+ supports and recommends .js configs for live-server setups.
This eliminates the @capacitor/cli type import entirely — the config is
just a JS object, which is what Capacitor reads at runtime anyway.
CI change: replace `npx tsc --noEmit` with `node -e "require('./capacitor.config.js')"`,
which validates the file parses and exports correctly without needing
TypeScript or the @capacitor/cli package installed.
Also: update tsconfig.json to remove the now-deleted .ts file from
include, keeping it for future use by Android/iOS platform scripts.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a backend GET /api/version endpoint, a Capacitor mobile scaffold and CI workflow, and frontend service-worker and native-update flows with Svelte stores and UI (blocking modal vs dismissible banner) wired to version checks. ChangesBackend Version API Endpoint
Mobile Project Scaffold
Frontend Service Worker and Native Update Management
Sequence Diagram(s)sequenceDiagram
participant Client as Client / WebView
participant Manager as NativeUpdateManager
participant Cap as Capacitor.App
participant BE as Backend (/api/version)
participant UI as UI Components
Client->>Manager: initNativeUpdateCheck()
par
Manager->>Cap: App.getInfo()
Manager->>BE: GET /api/version
and
Cap-->>Manager: {version}
BE-->>Manager: {minNativeVersion, latestNativeVersion, updateUrl}
end
Manager->>Manager: compareVersions()
alt version < minNativeVersion
Manager->>UI: nativeUpdateStore (blocking=true)
UI->>Client: Show blocking modal "Update required"
Client->>UI: Tap "Update"
UI->>Cap: Navigate to updateUrl
else version < latestNativeVersion
Manager->>UI: nativeUpdateStore (blocking=false)
UI->>Client: Show banner "New version available"
Client->>UI: Tap "Later" or "Update"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1b044759
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+95
to
+97
| const comparison = leftPart.localeCompare(rightPart); | ||
| if (comparison !== 0) { | ||
| return comparison > 0 ? 1 : -1; |
There was a problem hiding this comment.
compareVersions falls back to lexicographic comparison for non-numeric segments, which makes versions like 1.2.0-beta compare as newer than 1.2.0 (the extra beta segment is compared against default "0" and wins). In practice, Android versionName strings often include prerelease suffixes, so a prerelease build can incorrectly bypass the minNativeVersion block (or suppress the optional update banner) even though it should be treated as older than the final release.
Useful? React with 👍 / 👎.
Comment on lines
+25
to
+27
| {#if $nativeUpdateStore.updateUrl} | ||
| <button type="button" class="light" on:click={openUpdate}>Update app</button> | ||
| {/if} |
There was a problem hiding this comment.
The blocking update modal renders no actionable control when updateUrl is missing, because the only button is conditional and blocking mode has no dismiss path. Since the backend leaves MOBILE_IOS_UPDATE_URL optional, an iOS user below minNativeVersion can be hard-blocked on a dead-end screen with no way to open the store from inside the app.
Useful? React with 👍 / 👎.
Author
|
Follow-up after Codex review: addressed both actionable findings in e3b1c25.
Verification run locally:px vitest run tests/VersionController.test.ts --config vitest.config.ts from �ack/
px prettier --check on the changed update-management files
No secrets or personal data were added; changed files were scanned for obvious credential material. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
back/src/Enum/EnvironmentVariable.ts (1)
32-37: ⚡ Quick winConsider adding explicit type annotations for the mobile constants.
While TypeScript will correctly infer the types, adding explicit annotations would improve clarity, especially for
MOBILE_IOS_UPDATE_URLwhich can beundefined:export const MOBILE_IOS_UPDATE_URL: string | undefined = env.MOBILE_IOS_UPDATE_URL;This makes the optional nature immediately visible to consumers without needing to trace through the validation layer.
🤖 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 `@back/src/Enum/EnvironmentVariable.ts` around lines 32 - 37, The exported mobile constants lack explicit TypeScript type annotations; add explicit types to make optionality clear by annotating MOBILE_WEB_VERSION, MOBILE_MIN_NATIVE_VERSION, MOBILE_LATEST_NATIVE_VERSION, MOBILE_ANDROID_UPDATE_URL as string and annotate MOBILE_IOS_UPDATE_URL as string | undefined so consumers can see it may be undefined; update the declarations for MOBILE_WEB_VERSION, MOBILE_MIN_NATIVE_VERSION, MOBILE_LATEST_NATIVE_VERSION, MOBILE_ANDROID_UPDATE_URL and MOBILE_IOS_UPDATE_URL accordingly (preserve existing fallback logic).back/tests/VersionController.test.ts (2)
5-30: ⚡ Quick winExpand test coverage with additional scenarios.
The test only validates default environment values. Consider adding test cases for:
- Custom environment variables set – verify the endpoint correctly returns operator-configured versions and update URLs (especially
MOBILE_IOS_UPDATE_URL)- Version hierarchy – test when
MOBILE_LATEST_NATIVE_VERSION>MOBILE_MIN_NATIVE_VERSIONto ensure the payload reflects the distinctionThis would increase confidence that the endpoint correctly adapts to different deployment configurations.
🤖 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 `@back/tests/VersionController.test.ts` around lines 5 - 30, Add two new tests in VersionController.test.ts that instantiate VersionController and invoke the "/api/version" handler: (1) a test that sets process.env values (MOBILE_WEB_VERSION, MOBILE_MIN_NATIVE_VERSION, MOBILE_LATEST_NATIVE_VERSION, MOBILE_ANDROID_UPDATE_URL, MOBILE_IOS_UPDATE_URL) to non-default values and asserts the JSON response includes those exact values (referencing VersionController and the "/api/version" route), and (2) a test for version hierarchy where MOBILE_LATEST_NATIVE_VERSION is greater than MOBILE_MIN_NATIVE_VERSION verifying both fields are returned distinctly; ensure each test sets required process.env keys before creating VersionController and restores/clears them after to avoid cross-test pollution, and reuse the existing mock Express app pattern (routes Map, mocked res with status/json) used in the current test.
21-29: 💤 Low valueNote: Test expectation vs. actual HTTP response.
The assertion expects
ios: undefinedin the payload object, which correctly matches the function argument tores.json(). However, the actual HTTP response body will omit theiosfield entirely, sinceJSON.stringify()dropsundefinedvalues:{ "webVersion": "dev", "minNativeVersion": "1.0.0", "latestNativeVersion": "1.0.0", "updateUrl": { "android": "https://play.google.com/store/apps/details?id=net.bawes.universe" } }This is the correct API behavior for optional fields. Consider adding a comment clarifying this distinction if it might confuse future maintainers.
🤖 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 `@back/tests/VersionController.test.ts` around lines 21 - 29, The test currently asserts expect(json).toHaveBeenCalledWith({... updateUrl: { android: ..., ios: undefined }}), but JSON responses drop undefined fields so the real HTTP body omits the ios key; update the test in VersionController.test.ts to reflect that by removing the ios: undefined expectation (e.g., assert updateUrl only contains android or use expect(...).toHaveProperty('updateUrl.android') and expect(...).not.toHaveProperty('updateUrl.ios')), and add a short comment near the expect(json).toHaveBeenCalledWith line explaining that undefined properties are omitted by JSON.stringify so the ios field will be absent in the actual response.play/src/front/Network/NativeUpdateManager.ts (1)
77-117: ⚡ Quick winUse a well-known semver library instead of a custom comparison.
The custom
compareVersionsfunction doesn't handle semantic versioning prereleases correctly. For example, comparing"1.0-beta"to"1.0.0"would incorrectly conclude"1.0-beta" > "1.0.0"(line 110 lexicographically compares"beta"vs"0"), when semver specifies that prereleases are lower precedence than releases.While typical mobile app versions like
"1.2.0"will compare correctly, using a battle-tested semver library eliminates edge-case bugs and aligns with best practices. As per coding guidelines, prefer well-known libraries over rolling your own for common tasks like version comparison.📦 Consider using the `semver` package
Install the package:
npm install semver npm install --save-dev `@types/semver`Then replace the
compareVersionsfunction:+import * as semver from 'semver'; + export async function initNativeUpdateCheck(): Promise<void> { // ... - if (compareVersions(currentVersion, versionPayload.minNativeVersion) < 0) { + if (semver.lt(semver.coerce(currentVersion) ?? '0.0.0', semver.coerce(versionPayload.minNativeVersion) ?? '0.0.0')) { // ... - if (compareVersions(currentVersion, versionPayload.latestNativeVersion) < 0) { + if (semver.lt(semver.coerce(currentVersion) ?? '0.0.0', semver.coerce(versionPayload.latestNativeVersion) ?? '0.0.0')) { // ... - -function compareVersions(left: string, right: string): number { - // ... remove entire function -}Note:
semver.coerce()normalizes versions; usesemver.valid()+ direct comparison if strict semver format is guaranteed.🤖 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 `@play/src/front/Network/NativeUpdateManager.ts` around lines 77 - 117, The custom compareVersions function in NativeUpdateManager.ts should be replaced with a call to a battle-tested semver library: install semver (and `@types/semver`), import it, and implement compareVersions to use semver.compare when both inputs are valid semver strings; if not valid, attempt semver.coerce on each and compare the coerced results, and finally fall back to a simple lexical or numeric compare only if coercion fails—this preserves the existing function signature and corrects prerelease vs release ordering per semver rules.
🤖 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 @.github/workflows/mobile-ci.yml:
- Around line 39-41: The CI step named "Run cap doctor (info only)" currently
runs a floating Capacitor CLI via "npx --yes `@capacitor/cli`", making builds
non-reproducible; update that step to invoke a repo-pinned Capacitor CLI version
(for example use "npx `@capacitor/cli`@< pinned-version >" or install the exact
CLI version from package.json before running) so the job always runs a
deterministic CLI release—modify the command in that step accordingly and ensure
the pinned version matches the repository's lockfile/package.json.
In `@back/src/Enum/EnvironmentVariableValidator.ts`:
- Around line 18-27: The MOBILE_MIN_NATIVE_VERSION and
MOBILE_LATEST_NATIVE_VERSION zod schemas currently accept any string; add
semver-format validation (e.g., a semver-compatible regex or zod .refine) after
the .transform(emptyStringToUndefined) so only valid version strings like
"1.2.3" (optionally with pre-release/build) pass, and reject values like "abc";
apply the same change to both MOBILE_MIN_NATIVE_VERSION and
MOBILE_LATEST_NATIVE_VERSION, provide a clear validation error message (e.g.,
"must be a semantic version like X.Y.Z") so failures point to the bad env var,
and keep the emptyStringToUndefined behavior intact.
In `@mobile/capacitor.config.ts`:
- Around line 13-54: The Capacitor configuration is duplicated between
capacitor.config.ts and capacitor.config.js; keep a single source of truth by
consolidating the object exported as config (the CapacitorConfig constant and
its export default config) into one file and removing the other duplicate, or
make the .js import/export the canonical file and have capacitor.config.ts
re-export/import that file so only one object definition exists; update any
tooling or references that expect capacitor.config.js or capacitor.config.ts to
point to the canonical file and ensure the exported symbol remains export
default config and the shape (server, ios, android, plugins) is preserved.
In `@mobile/README.md`:
- Around line 9-10: The README currently points contributors to
capacitor.config.ts but the project and CI use capacitor.config.js as the
validated canonical config; update the README text to state that
capacitor.config.js is the canonical Capacitor config file (or vice versa if you
prefer TS), and add a short note instructing contributors to edit only that file
and keep the other in sync or autogenerated. Locate references to
capacitor.config.ts and capacitor.config.js in the README and adjust the wording
to explicitly name the single canonical file (capacitor.config.js), mention that
CI validates that file, and add a brief command or note about how to regenerate
or sync the other format if needed.
In `@mobile/tsconfig.json`:
- Around line 1-11: tsconfig.json currently has "include": [] and doesn't
inherit the shared baseline; change it to extend the mono-repo/base TS config
(add an "extends": "<path-to-base-tsconfig>") and replace the empty "include"
with the service-specific globs (e.g., ["src/**/*", "typings/**/*"]) so
type-checking runs for this mobile service; keep existing "compilerOptions"
overrides if needed and ensure the "extends" property is the first sibling in
the root object.
In `@play/src/front/Components/NativeUpdatePrompt.svelte`:
- Around line 21-48: The component uses hardcoded English strings; replace all
literals in NativeUpdatePrompt.svelte (the user-facing texts around
$nativeUpdateStore, and buttons that call openUpdate, reload, dismiss) with
typesafe-i18n lookups and add the corresponding keys to the nativeUpdate
translation module (e.g., modal.title, modal.message with placeholders
{currentVersion}/{requiredVersion}, modal.updateButton/modal.retryButton,
banner.message with {latestVersion}, banner.updateButton, banner.dismissButton);
import and use the typesafe-i18n translator (t or the project’s wrapper) in the
component to pass version placeholders when rendering the message and buttons so
all strings are translated in all languages.
In `@play/src/front/Components/ServiceWorkerUpdateBanner.svelte`:
- Around line 14-17: The banner's visible strings in
ServiceWorkerUpdateBanner.svelte (the <span> "Game updated - tap to reload" and
the two button labels "Reload" and "Later" used by the reload and dismiss
handlers) are hardcoded; replace them with typesafe-i18n translation keys (e.g.,
serviceWorker.update.title, serviceWorker.update.reload,
serviceWorker.update.later) and use the project's i18n helper to render those
keys in the component; then add corresponding entries under the same module file
in every language catalog in play/src/i18n/[language]/[module].ts so all
supported locales receive translations.
---
Nitpick comments:
In `@back/src/Enum/EnvironmentVariable.ts`:
- Around line 32-37: The exported mobile constants lack explicit TypeScript type
annotations; add explicit types to make optionality clear by annotating
MOBILE_WEB_VERSION, MOBILE_MIN_NATIVE_VERSION, MOBILE_LATEST_NATIVE_VERSION,
MOBILE_ANDROID_UPDATE_URL as string and annotate MOBILE_IOS_UPDATE_URL as string
| undefined so consumers can see it may be undefined; update the declarations
for MOBILE_WEB_VERSION, MOBILE_MIN_NATIVE_VERSION, MOBILE_LATEST_NATIVE_VERSION,
MOBILE_ANDROID_UPDATE_URL and MOBILE_IOS_UPDATE_URL accordingly (preserve
existing fallback logic).
In `@back/tests/VersionController.test.ts`:
- Around line 5-30: Add two new tests in VersionController.test.ts that
instantiate VersionController and invoke the "/api/version" handler: (1) a test
that sets process.env values (MOBILE_WEB_VERSION, MOBILE_MIN_NATIVE_VERSION,
MOBILE_LATEST_NATIVE_VERSION, MOBILE_ANDROID_UPDATE_URL, MOBILE_IOS_UPDATE_URL)
to non-default values and asserts the JSON response includes those exact values
(referencing VersionController and the "/api/version" route), and (2) a test for
version hierarchy where MOBILE_LATEST_NATIVE_VERSION is greater than
MOBILE_MIN_NATIVE_VERSION verifying both fields are returned distinctly; ensure
each test sets required process.env keys before creating VersionController and
restores/clears them after to avoid cross-test pollution, and reuse the existing
mock Express app pattern (routes Map, mocked res with status/json) used in the
current test.
- Around line 21-29: The test currently asserts
expect(json).toHaveBeenCalledWith({... updateUrl: { android: ..., ios: undefined
}}), but JSON responses drop undefined fields so the real HTTP body omits the
ios key; update the test in VersionController.test.ts to reflect that by
removing the ios: undefined expectation (e.g., assert updateUrl only contains
android or use expect(...).toHaveProperty('updateUrl.android') and
expect(...).not.toHaveProperty('updateUrl.ios')), and add a short comment near
the expect(json).toHaveBeenCalledWith line explaining that undefined properties
are omitted by JSON.stringify so the ios field will be absent in the actual
response.
In `@play/src/front/Network/NativeUpdateManager.ts`:
- Around line 77-117: The custom compareVersions function in
NativeUpdateManager.ts should be replaced with a call to a battle-tested semver
library: install semver (and `@types/semver`), import it, and implement
compareVersions to use semver.compare when both inputs are valid semver strings;
if not valid, attempt semver.coerce on each and compare the coerced results, and
finally fall back to a simple lexical or numeric compare only if coercion
fails—this preserves the existing function signature and corrects prerelease vs
release ordering per semver rules.
🪄 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
Run ID: 2972e11d-3ccd-4d01-9746-8df6416dd19e
⛔ Files ignored due to path filters (1)
mobile/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.github/workflows/mobile-ci.ymlback/src/App.tsback/src/Controller/VersionController.tsback/src/Enum/EnvironmentVariable.tsback/src/Enum/EnvironmentVariableValidator.tsback/src/Services/MobileVersionService.tsback/tests/VersionController.test.tsmobile/.gitignoremobile/Gemfilemobile/README.mdmobile/capacitor.config.jsmobile/capacitor.config.tsmobile/package.jsonmobile/tsconfig.jsonplay/public/service-worker-prod.jsplay/src/front/Components/App.svelteplay/src/front/Components/GameOverlay.svelteplay/src/front/Components/NativeUpdatePrompt.svelteplay/src/front/Components/ServiceWorkerUpdateBanner.svelteplay/src/front/Network/NativeUpdateManager.tsplay/src/front/Network/ServiceWorker.tsplay/src/front/Stores/NativeUpdateStore.tsplay/src/front/Stores/ServiceWorkerUpdateStore.ts
Comment on lines
+1
to
+11
| { | ||
| "compilerOptions": { | ||
| "target": "ES2020", | ||
| "module": "commonjs", | ||
| "esModuleInterop": true, | ||
| "strict": true, | ||
| "skipLibCheck": true, | ||
| "moduleResolution": "node" | ||
| }, | ||
| "include": [] | ||
| } |
There was a problem hiding this comment.
tsconfig.json is not inheriting base config and currently type-checks nothing.
Line 10 ("include": []) makes the config effectively inert, and there is no extends to align with the shared baseline.
Proposed fix
{
+ "extends": "<path-to-repo-base-tsconfig>",
"compilerOptions": {
"target": "ES2020",
"module": "commonjs",
"esModuleInterop": true,
"strict": true,
"skipLibCheck": true,
"moduleResolution": "node"
},
- "include": []
+ "include": ["./**/*.ts", "./**/*.tsx"]
}As per coding guidelines, "**/tsconfig.json: TypeScript configuration files (tsconfig.json) in each service must inherit from the appropriate base configuration and be service-specific".
🤖 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 `@mobile/tsconfig.json` around lines 1 - 11, tsconfig.json currently has
"include": [] and doesn't inherit the shared baseline; change it to extend the
mono-repo/base TS config (add an "extends": "<path-to-base-tsconfig>") and
replace the empty "include" with the service-specific globs (e.g., ["src/**/*",
"typings/**/*"]) so type-checking runs for this mobile service; keep existing
"compilerOptions" overrides if needed and ensure the "extends" property is the
first sibling in the root object.
Author
|
Follow-up pushed in d320195 addressing the latest CodeRabbit review items:
Verification run locally:
No secrets, personal data, live services, payment/KYC data, or wallet actions were used. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
mobile/tsconfig.json (1)
1-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd base-config inheritance in this service
tsconfig.Line 1 defines a standalone config with no
extends, which violates the repo TS-config policy and risks drift from shared compiler defaults.Proposed minimal fix
{ + "extends": "<relative-path-to-repo-base-tsconfig>", "compilerOptions": { "target": "ES2020", "module": "commonjs", "esModuleInterop": true, "strict": true, "skipLibCheck": true, "ignoreDeprecations": "6.0", "moduleResolution": "node" }, "include": ["*.ts", "src/**/*.ts", "typings/**/*.d.ts"] }As per coding guidelines, "
**/tsconfig.json: TypeScript configuration files (tsconfig.json) in each service must inherit from the appropriate base configuration and be service-specific".🤖 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 `@mobile/tsconfig.json` around lines 1 - 12, This tsconfig.json is standalone and must inherit the repo base; add an "extends" entry at the top of the JSON to point to the repository's base TypeScript config (so this service inherits shared defaults) while leaving the existing "compilerOptions" and "include" in place to override service-specific settings; ensure the extends value references the correct base config filename used in the repo (e.g., tsconfig.base.json or equivalent) and keep "moduleResolution", "strict", and other local overrides intact.
🧹 Nitpick comments (1)
back/tests/VersionController.test.ts (1)
58-76: 💤 Low valueConsider more explicit assertion for iOS property absence.
Line 75 uses
JSON.stringifyto verify theiosproperty is not present. While this works correctly (JSON.stringify omits undefined properties), a more direct assertion would improve readability:-expect(JSON.stringify(payload?.updateUrl)).not.toContain("ios"); +expect(payload?.updateUrl).not.toHaveProperty("ios");🤖 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 `@back/tests/VersionController.test.ts` around lines 58 - 76, The test currently checks for absence of the iOS URL by asserting JSON.stringify(payload?.updateUrl) does not contain "ios"; replace that with a direct property absence assertion: update the assertion to use the payload and updateUrl objects (e.g. expect(payload.updateUrl).not.toHaveProperty("ios") or expect(payload.updateUrl.ios).toBeUndefined()) in the VersionController.test.ts test where payload is derived and updateUrl is checked.
🤖 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 `@play/src/front/Network/NativeUpdateManager.ts`:
- Around line 64-72: The code only sets nativeUpdateStore when
compareVersions(...) < 0 and never clears it when the app is already up-to-date;
add an else branch after the compareVersions check to reset/clear
nativeUpdateStore so stale prompt state is removed on successful checks.
Specifically, beside the existing block that calls nativeUpdateStore.set({
blocking: false, currentVersion, requiredVersion:
versionPayload.minNativeVersion, latestVersion:
versionPayload.latestNativeVersion, updateUrl }), add an else that calls
nativeUpdateStore.set(...) to a neutral/cleared value (e.g., null or an
empty/default object such as { blocking: false, currentVersion, requiredVersion:
undefined, latestVersion: versionPayload.latestNativeVersion, updateUrl:
undefined }) so the store is explicitly reset when currentVersion >=
versionPayload.latestNativeVersion.
- Around line 78-87: compareVersions currently uses semver.coerce which strips
suffixes like "-hotfix" and makes "1.2.3-hotfix" compare equal to "1.2.3";
instead detect when semver.coerce produced a version but the original string
contains a suffix (e.g., contains '-' or '+' or non-numeric trailing chars) and
treat that coerced result as a prerelease by appending the original suffix (or a
stable prerelease token) to the coerced version before calling semver.compare;
update the logic around leftVersion/rightVersion in compareVersions to build
coercedLeftWithSuffix and coercedRightWithSuffix when needed so suffixed
versions sort older than the base stable version.
In `@play/src/i18n/en-US/refreshPrompt.ts`:
- Around line 5-25: Add the missing translation keys serviceWorkerUpdate and
nativeUpdate (including nested keys message/reload/later/dismissLabel and
modal.title/message/updateButton/retryButton and
banner.message/updateButton/laterButton/dismissLabel) to each supported locale
file for ar-SA, de-DE, dsb-DE, fr-FR, it-IT, ko-KR, nl-NL, and pt-BR; for each
locale, create the same object shape as in en-US/refreshPrompt.ts and provide
the appropriate translated strings (or placeholder translations) so the i18n
bundle includes these keys across all locales, then run the i18n validation/lint
to ensure no missing keys.
---
Duplicate comments:
In `@mobile/tsconfig.json`:
- Around line 1-12: This tsconfig.json is standalone and must inherit the repo
base; add an "extends" entry at the top of the JSON to point to the repository's
base TypeScript config (so this service inherits shared defaults) while leaving
the existing "compilerOptions" and "include" in place to override
service-specific settings; ensure the extends value references the correct base
config filename used in the repo (e.g., tsconfig.base.json or equivalent) and
keep "moduleResolution", "strict", and other local overrides intact.
---
Nitpick comments:
In `@back/tests/VersionController.test.ts`:
- Around line 58-76: The test currently checks for absence of the iOS URL by
asserting JSON.stringify(payload?.updateUrl) does not contain "ios"; replace
that with a direct property absence assertion: update the assertion to use the
payload and updateUrl objects (e.g.
expect(payload.updateUrl).not.toHaveProperty("ios") or
expect(payload.updateUrl.ios).toBeUndefined()) in the VersionController.test.ts
test where payload is derived and updateUrl is checked.
🪄 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
Run ID: 9791940e-0f28-4db0-b0c7-b842e95b8a47
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.github/workflows/mobile-ci.ymlback/src/Enum/EnvironmentVariable.tsback/src/Enum/EnvironmentVariableValidator.tsback/tests/VersionController.test.tsdocs/others/self-hosting/env-variables.mdmobile/README.mdmobile/capacitor.config.jsmobile/capacitor.config.tsmobile/tsconfig.jsonplay/package.jsonplay/src/front/Components/NativeUpdatePrompt.svelteplay/src/front/Components/ServiceWorkerUpdateBanner.svelteplay/src/front/Network/NativeUpdateManager.tsplay/src/i18n/en-US/refreshPrompt.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- back/src/Enum/EnvironmentVariable.ts
- play/src/front/Components/NativeUpdatePrompt.svelte
- .github/workflows/mobile-ci.yml
- play/src/front/Components/ServiceWorkerUpdateBanner.svelte
Comment on lines
+5
to
+25
| serviceWorkerUpdate: { | ||
| message: "Game updated - tap to reload", | ||
| reload: "Reload", | ||
| later: "Later", | ||
| dismissLabel: "Dismiss update prompt", | ||
| }, | ||
| nativeUpdate: { | ||
| modal: { | ||
| title: "Please update the app", | ||
| message: | ||
| "Version {currentVersion:string} is no longer supported. Update to {requiredVersion:string} or newer to continue.", | ||
| updateButton: "Update app", | ||
| retryButton: "Try again", | ||
| }, | ||
| banner: { | ||
| message: "New app version {latestVersion:string} is available.", | ||
| updateButton: "Update", | ||
| laterButton: "Later", | ||
| dismissLabel: "Dismiss native update prompt", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify refreshPrompt locale coverage for new keys
fd -p refreshPrompt.ts play/src/i18n | sort
rg -n --glob '**/refreshPrompt.ts' 'serviceWorkerUpdate|nativeUpdate' play/src/i18nRepository: BAWES-Universe/workadventure-universe
Length of output: 541
🏁 Script executed:
cat -n play/src/i18n/fr-FR/refreshPrompt.tsRepository: BAWES-Universe/workadventure-universe
Length of output: 367
🏁 Script executed:
cat -n play/src/i18n/de-DE/refreshPrompt.tsRepository: BAWES-Universe/workadventure-universe
Length of output: 370
Add missing translations for serviceWorkerUpdate and nativeUpdate to all supported locales.
The new keys at lines 5-25 exist only in en-US/refreshPrompt.ts. They must be translated in all supported language files: ar-SA, de-DE, dsb-DE, fr-FR, it-IT, ko-KR, nl-NL, and pt-BR per the i18n requirement in the coding guidelines.
🤖 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 `@play/src/i18n/en-US/refreshPrompt.ts` around lines 5 - 25, Add the missing
translation keys serviceWorkerUpdate and nativeUpdate (including nested keys
message/reload/later/dismissLabel and
modal.title/message/updateButton/retryButton and
banner.message/updateButton/laterButton/dismissLabel) to each supported locale
file for ar-SA, de-DE, dsb-DE, fr-FR, it-IT, ko-KR, nl-NL, and pt-BR; for each
locale, create the same object shape as in en-US/refreshPrompt.ts and provide
the appropriate translated strings (or placeholder translations) so the i18n
bundle includes these keys across all locales, then run the i18n validation/lint
to ensure no missing keys.
Author
|
Follow-up pushed in 3213ecd for the latest review:
I left mobile/tsconfig.json without an Verification run locally:
No secrets, personal data, live services, payment/KYC data, or wallet actions were used. |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Notes
Verification
Known local verification limitation
Closes #12
Summary by CodeRabbit
New Features
Tests
Documentation