Repository navigation
refactor: remove game phaser fixture - #30
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR migrates the default game workspace from Estimated code review effort: 4 (Complex) | ~60 minutes ChangesStarter workspace migration
Starter Runtime Platform Installer
Sequence Diagram(s)sequenceDiagram
participant Main as main.ts
participant Detector as detectRuntime
participant Installer as installStarterPlatform
participant Adapter as Adapter Module (Capacitor/AIT/Devvit/Browser)
Main->>Detector: detectRuntime()
Detector-->>Main: RuntimeConfig
Main->>Installer: await installStarterPlatform(runtimeConfig)
Installer->>Installer: switch(runtime.target)
alt android/ios
Installer->>Adapter: dynamic import Capacitor adapter
else ait
Installer->>Adapter: dynamic import AIT adapter (optional fallback bridge)
else reddit
Installer->>Installer: shouldUseDevvitSandbox(runtime)
Installer->>Adapter: dynamic import Devvit adapter (optional sandbox fallback)
else default
Installer->>Adapter: dynamic import browser adapter
end
Adapter-->>Installer: PlatformGateway
Installer-->>Main: TargetConfiguredGateway
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release-ait.yml (1)
33-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the missing target validation steps in
release-ait
.github/workflows/release-ait.ymlshould runvalidate:target-config,validate:effective-config, andvalidate:targetsbefore the smoke steps;validate:publicis a separate check, and the other release workflows already include these validations.🤖 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 @.github/workflows/release-ait.yml around lines 33 - 45, The release-ait workflow is missing the target validation checks before the smoke steps. Update the `release-ait` job to run `validate:target-config`, `validate:effective-config`, and `validate:targets` in the same validation block as the existing checks, and keep `validate:public` as a separate step; use the existing workflow step sequence in `release-ait.yml` to place them before `smoke:target-config`, `smoke:effective-config`, and `smoke:adapter-effective-config`.
🧹 Nitpick comments (1)
tools/smoke/verify-target-artifacts.ts (1)
544-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider importing
platformTargetsFilePathfromtools/target/platform-targets.tsinstead of duplicating.This file defines its own
platformTargetsFilePath()andplatformTargetsFileEnvconstant, which duplicate the exported versions intools/target/platform-targets.ts. Both now default tompgd.targets.json, but maintaining two copies risks future drift. The exported function accepts an optional path parameter and defaults identically when called without arguments, so it can be used as a drop-in replacement.♻️ Proposed refactor: import shared function
+import { platformTargetsFilePath, platformTargetsFileEnv } from '../target/platform-targets';Then remove the local definitions:
-const platformTargetsFileEnv = 'MPGD_PLATFORM_TARGETS_FILE'; - -function platformTargetsFilePath(): string { - return resolve(process.env[platformTargetsFileEnv] ?? 'mpgd.targets.json'); -}🤖 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 `@tools/smoke/verify-target-artifacts.ts` around lines 544 - 546, The local platform targets path logic in verify-target-artifacts duplicates the shared implementation from platform-targets.ts, so replace the local platformTargetsFilePath() and platformTargetsFileEnv usage with the exported platformTargetsFilePath from tools/target/platform-targets.ts. Update the callers in this module to use the shared function as a drop-in defaulting behavior, and remove the duplicated local constant/function so there is a single source of truth.
🤖 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.
Outside diff comments:
In @.github/workflows/release-ait.yml:
- Around line 33-45: The release-ait workflow is missing the target validation
checks before the smoke steps. Update the `release-ait` job to run
`validate:target-config`, `validate:effective-config`, and `validate:targets` in
the same validation block as the existing checks, and keep `validate:public` as
a separate step; use the existing workflow step sequence in `release-ait.yml` to
place them before `smoke:target-config`, `smoke:effective-config`, and
`smoke:adapter-effective-config`.
---
Nitpick comments:
In `@tools/smoke/verify-target-artifacts.ts`:
- Around line 544-546: The local platform targets path logic in
verify-target-artifacts duplicates the shared implementation from
platform-targets.ts, so replace the local platformTargetsFilePath() and
platformTargetsFileEnv usage with the exported platformTargetsFilePath from
tools/target/platform-targets.ts. Update the callers in this module to use the
shared function as a drop-in defaulting behavior, and remove the duplicated
local constant/function so there is a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 971e4a27-9369-4032-9aa5-8ba45685b946
⛔ Files ignored due to path filters (2)
apps/game-phaser/public/assets/ui/orb.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (56)
.github/workflows/build-target.yml.github/workflows/ci.yml.github/workflows/release-ait.yml.github/workflows/release-android.yml.github/workflows/release-devvit.yml.github/workflows/release-ios.yml.github/workflows/release-microsoft-store.yml.gitignoreCONTRIBUTING.mdREADME.mdapps/game-phaser/index.htmlapps/game-phaser/package.jsonapps/game-phaser/public/manifest.webmanifestapps/game-phaser/src/game/assets/manifest.tsapps/game-phaser/src/game/content/stageConfig.tsapps/game-phaser/src/game/input/actions.tsapps/game-phaser/src/game/input/installGameplayInput.tsapps/game-phaser/src/main.tsapps/game-phaser/src/platform/demoGameServices.tsapps/game-phaser/src/platform/demoState.tsapps/game-phaser/src/platform/installPlatform.tsapps/game-phaser/src/platform/platformEvents.tsapps/game-phaser/src/platform/runtimeDetector.test.tsapps/game-phaser/src/platform/runtimeDetector.tsapps/game-phaser/src/runtime/assetManifest.tsapps/game-phaser/src/runtime/createGame.tsapps/game-phaser/src/runtime/sceneRegistry.tsapps/game-phaser/src/scenes/BootScene.tsapps/game-phaser/src/scenes/GameScene.tsapps/game-phaser/src/scenes/LobbyScene.tsapps/game-phaser/src/scenes/PreloadScene.tsapps/game-phaser/src/scenes/ResultScene.tsapps/game-phaser/src/styles.cssapps/game-phaser/src/validation/boundaryGuards.tsapps/game-phaser/src/vite-env.d.tsapps/game-phaser/tsconfig.bundle.jsonapps/game-phaser/tsconfig.bundle.prod.jsonapps/game-phaser/tsconfig.jsonapps/game-phaser/vite.config.tsapps/target-devvit/README.mdapps/target-devvit/package.jsonapps/target-web-preview/package.jsondocs/GAME_DEVELOPMENT.mddocs/ROADMAP_CHECKLIST.mdexamples/phaser-starter/mpgd.targets.jsonpackage.jsontools/graph/requests/sdk-demo-loop.jsontools/graph/requests/starter-loop.jsontools/graph/requests/target-config-runtime.jsontools/smoke/build-target-artifacts.tstools/smoke/verify-target-artifacts.tstools/target/platform-targets.tstools/validate-game-assets.tstools/validate-public-readiness.tstools/validate-target-config.tstsconfig.graph.json
💤 Files with no reviewable changes (32)
- apps/game-phaser/src/scenes/GameScene.ts
- apps/game-phaser/tsconfig.bundle.prod.json
- apps/game-phaser/src/platform/runtimeDetector.test.ts
- apps/game-phaser/src/platform/demoState.ts
- apps/game-phaser/vite.config.ts
- apps/game-phaser/src/vite-env.d.ts
- apps/game-phaser/src/scenes/BootScene.ts
- apps/game-phaser/src/validation/boundaryGuards.ts
- apps/game-phaser/tsconfig.bundle.json
- apps/game-phaser/src/platform/demoGameServices.ts
- tools/validate-public-readiness.ts
- apps/game-phaser/index.html
- apps/game-phaser/src/platform/platformEvents.ts
- apps/game-phaser/src/runtime/createGame.ts
- apps/game-phaser/src/platform/installPlatform.ts
- apps/game-phaser/tsconfig.json
- apps/game-phaser/src/platform/runtimeDetector.ts
- apps/game-phaser/src/game/input/installGameplayInput.ts
- apps/game-phaser/src/styles.css
- apps/game-phaser/src/runtime/assetManifest.ts
- apps/game-phaser/src/game/content/stageConfig.ts
- apps/game-phaser/src/runtime/sceneRegistry.ts
- apps/game-phaser/src/scenes/PreloadScene.ts
- apps/game-phaser/package.json
- apps/game-phaser/src/scenes/LobbyScene.ts
- apps/game-phaser/src/game/assets/manifest.ts
- tools/graph/requests/sdk-demo-loop.json
- apps/game-phaser/src/scenes/ResultScene.ts
- apps/game-phaser/src/game/input/actions.ts
- apps/game-phaser/src/main.ts
- tools/validate-game-assets.ts
- apps/game-phaser/public/manifest.webmanifest
![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: 13b6681b07
ℹ️ 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".
| "create:game": "node tools/run-ttsx.mjs --mpgd-cli packages/cli/src/bin.ts game create", | ||
| "build:target": "pnpm i18n:build && node tools/run-ttsx.mjs tools/target/build-target.ts", | ||
| "build:target": "pnpm i18n:build && MPGD_PLATFORM_TARGETS_FILE=examples/phaser-starter/mpgd.targets.json node tools/run-ttsx.mjs tools/target/build-target.ts", | ||
| "build:web": "pnpm build:target web-preview production", |
There was a problem hiding this comment.
When a generated game runs mpgd target build ... --targets-file ..., packages/cli prepares .mpgd.targets.generated.json and passes it via MPGD_PLATFORM_TARGETS_FILE into pnpm build:target; this inline assignment replaces that value with examples/phaser-starter/mpgd.targets.json, so the build ignores the caller's game and writes artifacts for the in-repo starter. Use a defaulting mechanism that does not clobber an existing MPGD_PLATFORM_TARGETS_FILE (and mirror it for the smoke scripts) so external target builds keep using their own target file.
Useful? React with 👍 / 👎.
|
|
||
| function platformTargetsFilePath(): string { | ||
| return resolve(process.env[platformTargetsFileEnv] ?? 'platform.targets.json'); | ||
| return resolve(process.env[platformTargetsFileEnv] ?? 'mpgd.targets.json'); |
There was a problem hiding this comment.
pnpm smoke:targets:build runs tools/smoke/build-target-artifacts.ts, which invokes verifyTargetArtifacts() in the same process without setting MPGD_PLATFORM_TARGETS_FILE; after this change the verifier falls back to a root mpgd.targets.json, but the commit removed the root target file and only added examples/phaser-starter/mpgd.targets.json. That means this smoke command fails after the builds complete with a missing targets file, so pass the starter targets file into this script or set the default to the example path for repo-local smoke builds.
Useful? React with 👍 / 👎.
| "kind": "capacitor-android", | ||
| "gameApp": "apps/game-phaser", | ||
| "shellApp": "apps/mobile-capacitor", | ||
| "gameApp": ".", |
There was a problem hiding this comment.
Starting with the android entry, the root/release target builds now bundle examples/phaser-starter for non-web targets, but that app's installStarterPlatform() always constructs createBrowserPlatformGateway() and derives the web-preview config target. As a result android, ios, AIT, and Reddit artifacts no longer instantiate their Capacitor/AIT/Devvit gateways or target runtime even though APP_TARGET is set, so platform-specific identity, storage, commerce, ads, and leaderboard paths are absent from those builds.
Useful? React with 👍 / 👎.
| "gameApp": "apps/game-phaser", | ||
| "wrapperApp": "apps/target-devvit", | ||
| "gameApp": ".", | ||
| "wrapperApp": "../../apps/target-devvit", |
There was a problem hiding this comment.
For the Reddit target, build-target.ts writes the release manifest artifact as ${wrapperAppConfigPath}/dist; with this wrapper path relative to examples/phaser-starter, the manifest records ../../apps/target-devvit/dist. The target smoke verifier then resolves it outside the targets-file base and rejects it via assertPathInsideTargetBase, so pnpm build:devvit && pnpm smoke:target reddit in CI/release workflows fails even when the dist files exist.
Useful? React with 👍 / 👎.
| "microsoft-store": { | ||
| "kind": "web", | ||
| "gameApp": "apps/game-phaser", | ||
| "gameApp": ".", |
There was a problem hiding this comment.
When the Microsoft Store target now builds examples/phaser-starter, the copied Vite output has no manifest.webmanifest and its index.html has no manifest link, whereas verifyMicrosoftStorePwaManifest() requires both for pnpm smoke:target microsoft-store. This makes the CI/release Store smoke fail after pnpm build:microsoft-store; either keep a Store-ready manifest in the starter or point this target at an app that provides one.
Useful? React with 👍 / 👎.
|
Patched the CI/review follow-ups:
Local verification after the patch:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== index.html ==="
fd index.html examples/phaser-starter --exec cat {}
echo "=== manifest.webmanifest ==="
fd manifest.webmanifest examples/phaser-starter --exec cat {}
echo "=== release-ait.yml ==="
cat .github/workflows/release-ait.yml
echo "=== verify-target-artifacts.ts ==="
cat tools/smoke/verify-target-artifacts.tsLength of output: 24671
Kicking off a full review now. ✅ Action performedReview finished.
|
|
Follow-up CI patch pushed:
Local verification:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== verify-target-artifacts.ts ==="
cat tools/smoke/verify-target-artifacts.ts
echo "=== target-devvit package.json ==="
fd package.json apps/target-devvit --exec cat {}
echo "=== mpgd.targets.json reddit/devvit-web targets ==="
cat examples/phaser-starter/mpgd.targets.jsonLength of output: 25830
This looks correct and properly scoped to only the ✅ Action performedReview finished.
|
|
Resolved the remaining Codex review P1 follow-ups:
Local verification after this patch:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== package.json scripts (relevant) ==="
cat package.json | jq '.scripts' 2>/dev/null | grep -i -E "MPGD_PLATFORM_TARGETS_FILE|validate:target-config|validate:effective-config|validate:targets|build:target|smoke:target|build:effective-config" -A2 -B2 || true
echo "=== search for MPGD_PLATFORM_TARGETS_FILE usage in package.json ==="
grep -n "MPGD_PLATFORM_TARGETS_FILE" package.json
echo "=== examples/phaser-starter structure ==="
fd . examples/phaser-starter --max-depth 3
echo "=== runtime detection in starter ==="
fd -e ts . examples/phaser-starter/src --exec grep -l "APP_TARGET\|MPGD_CONFIG_TARGET" {} \;Length of output: 8993 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== runtimeDetector.ts ==="
cat examples/phaser-starter/src/platform/runtimeDetector.ts
echo "=== installStarterPlatform.ts ==="
cat examples/phaser-starter/src/platform/installStarterPlatform.ts
echo "=== env.d.ts ==="
cat examples/phaser-starter/src/env.d.ts
echo "=== package.json ==="
cat examples/phaser-starter/package.json
echo "=== vite.config.ts ==="
cat examples/phaser-starter/vite.config.tsLength of output: 7305 Everything checks out.
This all looks correctly scoped and consistent with the rest of the target-config flow. Kicking off a full review now. ✅ Action performedReview finished.
|
|
Follow-up for the latest CI failure:
Local verification:
|
|
Final local OCR rerun on
Local validation after the patch:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== installStarterPlatform.ts ==="
cat examples/phaser-starter/src/platform/installStarterPlatform.ts
echo "=== verify-target-artifacts.ts ==="
cat tools/smoke/verify-target-artifacts.ts
echo "=== commit 9104e79 check ==="
git log --oneline -5Length of output: 27031
Both changes look correct and well-scoped. Kicking off a full review now. ✅ Action performedReview finished.
|
Summary
apps/game-phasersample app, its rootplatform.targets.jsonfixture, and the old demo graph/assets validation wiring.examples/phaser-starter/mpgd.targets.jsonand update target tooling defaults, smoke checks, and graph presets accordingly.OCR review
902f475core/tooling commit: OCR reviewed successfully with 0 comments.9e0b52fCI/workflow commit: OCR initially found the missed iOS artifact path; fixed and reran OCR with 0 comments.13b6681docs commit: OCR preview reported only unsupported Markdown files, so no LLM review target.Validation
pnpm checkpnpm graph:preflightpnpm build:webpnpm smoke:target web-previewpnpm smoke:effective-configpnpm smoke:adapter-effective-configgit diff --checkrg -n "apps/game-phaser|game-phaser|validate:game-assets|graph:demo|sdk-demo-loop|platform\.targets\.json|13\.0\.0-rc\.2" -g '!node_modules' -g '!dist' -g '!artifacts' -g '!release-output'(no matches)Release notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation