Repository navigation
feat: add Phaser asset manifests and target artifact doctor - #15
Conversation
|
Warning Review limit reached
Next review available in: 23 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 (7)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesRelease artifact copy and target doctor validation
Phaser assets package and starter template wiring
Estimated code review effort: 3 (Moderate) | ~30 minutes 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 |
![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: 40465c4e0e
ℹ️ 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".
| 'ios: cap sync completed; set MPGD_RUN_IOS_SIMULATOR_BUILD=1 for a simulator .app or MPGD_RUN_IOS_ARCHIVE=1 for an xcarchive.', | ||
| ); | ||
| releaseArtifact = 'release-output/ios/capacitor-sync'; | ||
| replaceDirectory(`${shellApp}/ios`, targetPath(releaseArtifact)); |
There was a problem hiding this comment.
When the default iOS sync variant copies only ${shellApp}/ios into release-output/ios/capacitor-sync, the copied Xcode project loses the relative package layout that cap sync generated in the shell. In this repo the synced apps/mobile-capacitor/ios/App/CapApp-SPM/Package.swift references ../../../node_modules/@mpgd/capacitor-game-services, which is valid under apps/mobile-capacitor/ios but resolves to release-output/ios/node_modules/... after this copy. Any consumer opening or building the manifest artifact from the game output will fail to resolve the local Swift package even though smoke verification can still find the embedded config.
Useful? React with 👍 / 👎.
imjlk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/cli/templates/phaser-game/pnpm-workspace.yaml`:
- Line 2: The pnpm workspace entry is pointing at a package file instead of the
template root directory. Update the pnpm-workspace.yaml template to use the root
directory entry for the workspace package rather than the package.json path, so
the workspace correctly treats this template root as a directory.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbb92c35-51cf-4948-b8b6-3fbb9c271b8a
⛔ Files ignored due to path filters (2)
packages/cli/templates/phaser-game/src/assets/marker.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.sampo/changesets/create-game-phaser-assets.md.sampo/changesets/phaser-assets-package.md.sampo/changesets/target-artifact-ownership.md.sampo/config.tomlpackages/cli/src/index.tspackages/cli/templates/phaser-game/README.mdpackages/cli/templates/phaser-game/agent/acceptance.mdpackages/cli/templates/phaser-game/agent/game-manifest.jsonpackages/cli/templates/phaser-game/apps/target-devvit/README.mdpackages/cli/templates/phaser-game/package.jsonpackages/cli/templates/phaser-game/pnpm-workspace.yamlpackages/cli/templates/phaser-game/src/assets/manifest.tspackages/cli/templates/phaser-game/src/scenes/BootScene.tspackages/cli/templates/phaser-game/src/scenes/PlayScene.tspackages/cli/templates/phaser-game/vite.config.tspackages/phaser-assets/package.jsonpackages/phaser-assets/src/index.tspackages/phaser-assets/test/index.test.tspackages/phaser-assets/tsconfig.jsontools/smoke/verify-target-artifacts.tstools/target/build-target.ts
✅ Files skipped from review due to trivial changes (10)
- packages/cli/templates/phaser-game/agent/acceptance.md
- packages/phaser-assets/tsconfig.json
- packages/phaser-assets/test/index.test.ts
- .sampo/changesets/phaser-assets-package.md
- .sampo/changesets/create-game-phaser-assets.md
- .sampo/changesets/target-artifact-ownership.md
- packages/cli/templates/phaser-game/vite.config.ts
- .sampo/config.toml
- packages/cli/templates/phaser-game/apps/target-devvit/README.md
- packages/cli/templates/phaser-game/README.md
|
@codex review Patched the outstanding review threads:
Validation run locally:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc7324c379
ℹ️ 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".
| { | ||
| kind: 'image', | ||
| key: starterAssetKeys.marker, | ||
| url: createPhaserAssetUrl('./marker.svg', import.meta.url), |
There was a problem hiding this comment.
In generated starters after pnpm build or any target build, this is the only reference to marker.svg, but it is just a string passed into createPhaserAssetUrl; Vite cannot see a call-site new URL('./marker.svg', import.meta.url) or ?url import, so it will not emit/copy the SVG into the production bundle. The helper then computes a URL relative to the JS chunk and Phaser loads a missing marker asset, breaking the new asset-manifest starter sample outside the dev server; use a Vite-visible URL/import here and pass that emitted URL into the manifest.
Useful? React with 👍 / 👎.
| } else { | ||
| console.warn('ait: package build skipped; release manifest points to wrapper webDir.'); | ||
| releaseArtifact = 'release-output/ait/wrapper-web'; | ||
| replaceDirectory(webDir, targetPath(releaseArtifact)); |
There was a problem hiding this comment.
When MPGD_AIT_PACKAGE_MODE=skip is set (the template's --ait-variant wrapper path), the wrapper Vite build has just produced wrapperApp/dist, but this copies webDir, which is only the game's public/game payload. The release manifest then points at release-output/ait/wrapper-web even though that directory lacks the Apps in Toss wrapper/bridge bundle, so the target smoke/doctor path validates a plain game web build rather than the wrapper artifact users are trying to inspect.
Useful? React with 👍 / 👎.
| for (const extraArtifact of extraRequiredArtifactsForTarget(targetConfig, artifactPath)) { | ||
| assertFileExists(extraArtifact, `${target} required artifact`); | ||
| assertPathExists(extraArtifact, `${target} required artifact`); |
There was a problem hiding this comment.
For devvit-web targets, extraRequiredArtifactsForTarget() still returns server/index.cjs, but this changed the check from file-only to assertPathExists, which also accepts directories. If a broken Devvit build leaves server/index.cjs/ as a directory or otherwise not a file, smoke:target, smoke-all, and the new doctor command now pass the server-entry validation even though the generated Devvit artifact cannot load the server module.
Useful? React with 👍 / 👎.
| if (!existsSync(packageFile)) { | ||
| return []; |
There was a problem hiding this comment.
For the default iOS sync artifact (release-output/ios/capacitor-sync), App/CapApp-SPM/Package.swift is what records the local Swift package dependencies that make the copied Xcode project portable. Returning an empty list when that file is missing means smoke:target ios and target doctor can still pass a copied sync directory that lacks the SPM manifest entirely, as long as the web effective-config file is present; this should only be optional for built .app/.xcarchive artifacts, not for the sync project copy.
Useful? React with 👍 / 👎.
|
@codex review Patched the new review feedback in
Validation run locally:
|
|
✅ Action performedReview finished.
|
|
@codex review Follow-up patch pushed in
Additional validation:
The earlier |
|
✅ Action performedReview finished.
|
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)
tools/smoke/verify-target-artifacts.ts (1)
60-62: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGuard
effectiveConfigPathwith the target-base check too.resolveArtifactPath()preserves absolute paths, so a manifest-controlledentry.effectiveConfig.pathcan still point outsideloadedPlatformTargets.baseDirand get read here.🤖 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 60 - 62, The artifact verification logic currently validates only artifactPath against the target base, but effectiveConfigPath can still be absolute and escape loadedPlatformTargets.baseDir. Update verify-target-artifacts.ts so the effectiveConfigPath used in the target check is also passed through assertPathInsideTargetBase before assertPathExists, using the same target-based guard pattern already applied to artifactPath; this should be done in the verification flow around resolveArtifactPath and effectiveConfigPath handling.
🤖 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 `@tools/smoke/verify-target-artifacts.ts`:
- Around line 60-62: The artifact verification logic currently validates only
artifactPath against the target base, but effectiveConfigPath can still be
absolute and escape loadedPlatformTargets.baseDir. Update
verify-target-artifacts.ts so the effectiveConfigPath used in the target check
is also passed through assertPathInsideTargetBase before assertPathExists, using
the same target-based guard pattern already applied to artifactPath; this should
be done in the verification flow around resolveArtifactPath and
effectiveConfigPath handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27f83510-e9cb-40ea-aa62-2a8c8fa95b4d
📒 Files selected for processing (6)
.sampo/changesets/create-game-phaser-assets.md.sampo/changesets/ios-sync-game-artifact.md.sampo/changesets/target-artifact-ownership.mdpackages/cli/templates/phaser-game/src/assets/manifest.tstools/smoke/verify-target-artifacts.tstools/target/build-target.ts
✅ Files skipped from review due to trivial changes (2)
- .sampo/changesets/target-artifact-ownership.md
- .sampo/changesets/create-game-phaser-assets.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .sampo/changesets/ios-sync-game-artifact.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 883ef2ef2b
ℹ️ 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".
| export function createPhaserAssetUrl(path: string, baseUrl: string | URL): string { | ||
| return new URL(path, baseUrl).href; |
There was a problem hiding this comment.
When callers follow the new asset guidance and pass createPhaserAssetUrl('./file', import.meta.url), Vite cannot statically analyze this new URL because the relative path is now a function parameter. Dev may work from /src, but production builds leave the runtime URL relative to the emitted JS chunk and do not emit/hash the asset, so manifest entries can 404 on built targets; require callers to pass a Vite-emitted ?url import or keep the static new URL('./file', import.meta.url) at the call site.
Useful? React with 👍 / 👎.
| releaseArtifact = 'release-output/ios/capacitor-sync'; | ||
| replaceDirectory(`${shellApp}/ios`, targetPath(releaseArtifact)); | ||
| copyIosSyncSwiftPackage(shellApp, '@mpgd/capacitor-game-services'); |
There was a problem hiding this comment.
In the iOS sync fallback, the release manifest points at release-output/ios/capacitor-sync, but copyIosSyncSwiftPackage writes the SwiftPM package to the sibling release-output/ios/node_modules path required by App/CapApp-SPM/Package.swift. Any handoff or automation that consumes entry.artifact as the artifact root gets a copied Xcode project with a broken local package reference; either make the manifest artifact the common release-output/ios root or copy/rewrite the Swift package dependency so it is contained in the advertised artifact.
Useful? React with 👍 / 👎.
|
@codex review Patched the latest review feedback in
Validation run locally:
|
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
@mpgd/phaser-assetshelpers for Phaser asset manifests and target-portable loader wiring@mpgd/create-gamerelease-output/ait/wrapper-webmpgd target doctorto verify existing target artifacts and reject manifest artifacts outside the game target config directory@mpgd/phaser-assetsin the Sampo fixed release group after local initial publishLocal publish
@mpgd/phaser-assets@0.3.2initial-published locally with maintainer npm auth loaded throughmisenpm pack @mpgd/phaser-assets@0.3.2works without auth@mpgd/phaser-assets: publicand dist-taglatest: 0.3.2npm view @mpgd/phaser-assets versionstill returned 404 immediately after publish, while pack/dist-tag/access endpoints were already visibleValidation
pnpm --dir packages/phaser-assets checkpnpm --dir packages/phaser-assets testpnpm --dir packages/cli checkpnpm validate:starter-workflow/tmp/mpgd-template-smokewith local kit pathpnpm install -wpnpm checkpnpm buildmpgd target build-all --targets web,ait --ait-variant wrappermpgd target smoke-all --targets web,aitmpgd target doctor --targets web,aitpnpm validate:publicpnpm checkpnpm testpnpm graph:preflightpnpm pack:packages(passed when run standalone; avoid parallel package builds in the same worktree)pnpm build:webpnpm build:aitpnpm smoke:target web-previewpnpm smoke:target aitOpen Code Review
ec8ee18reviewed; addressed exhaustiveness and binary asset dataType feedback, rerun clean except repo-supportedttsc fix/formatfalse positive8923efereviewed; 0 comments48c216areviewed; 0 comments0b73628reviewed; 0 commentsSummary by CodeRabbit
New Features
mpgd target doctorcheck to validate target artifacts and release paths.Bug Fixes
Documentation