Repository navigation
[codex] Add Microsoft Store PWA target - #17
Conversation
|
@codex review |
|
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 39 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 (2)
📝 WalkthroughWalkthroughThis PR adds ChangesMicrosoft Store target support
Estimated code review effort: 4 (Complex) | ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/target/build-target.ts (1)
225-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHelper implicitly depends on outer-scope
targetNameinstead of an explicit parameter.
appTargetForBuildtakestargetbut falls back to the enclosing function'stargetNamevariable rather than a value tied totargetitself. This works only because the single call site always passes the pair together; extracting/reusing this helper elsewhere would silently break.Consider passing the name explicitly:
♻️ Proposed refactor
-function appTargetForBuild(target: BuildTargetConfig): string { - return target.kind === 'web' ? 'browser' : targetName; +function appTargetForBuild(target: BuildTargetConfig, name: string): string { + return target.kind === 'web' ? 'browser' : name; }And update the call site to
appTargetForBuild(target, targetName).🤖 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/target/build-target.ts` around lines 225 - 228, The helper appTargetForBuild currently relies on the outer-scope targetName instead of using only its own inputs, which makes it fragile and non-reusable. Update appTargetForBuild to accept targetName as an explicit parameter, use that value for the non-web branch, and adjust its call site in the build-target flow to pass both target and targetName so the function is self-contained.
🤖 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 `@tools/target/build-target.ts`:
- Around line 225-228: The helper appTargetForBuild currently relies on the
outer-scope targetName instead of using only its own inputs, which makes it
fragile and non-reusable. Update appTargetForBuild to accept targetName as an
explicit parameter, use that value for the non-web branch, and adjust its call
site in the build-target flow to pass both target and targetName so the function
is self-contained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94e0a26d-5fcb-4754-8917-8786fa9b0226
📒 Files selected for processing (29)
.github/ISSUE_TEMPLATE/bug_report.yml.github/workflows/build-target.yml.github/workflows/ci.yml.github/workflows/release-microsoft-store.yml.sampo/changesets/microsoft-store-pwa-target.mdREADME.mddocs/GAME_DEVELOPMENT.mddocs/PUBLIC_RELEASE_CHECKLIST.mdexamples/phaser-starter/agent/brief.template.mdexamples/phaser-starter/agent/game.manifest.jsonpackage.jsonpackages/cli/src/index.tspackages/cli/templates/phaser-game/README.mdpackages/cli/templates/phaser-game/agent/acceptance.mdpackages/cli/templates/phaser-game/agent/brief.mdpackages/cli/templates/phaser-game/agent/game-manifest.jsonpackages/cli/templates/phaser-game/mpgd.targets.jsonpackages/cli/templates/phaser-game/package.jsonpackages/target-config/src/runtime.tspackages/target-config/targets.jsonplatform.targets.jsontools/smoke/adapter-effective-config.tstools/smoke/build-target-artifacts.tstools/smoke/effective-target-config.tstools/smoke/target-config-runtime.tstools/target/build-target.tstools/target/effective-config.tstools/validate-starter-workflow.tstools/validate-target-config.ts
![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: e29a60e1bc
ℹ️ 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".
| } | ||
|
|
||
| function appTargetForBuild(target: BuildTargetConfig): string { | ||
| return target.kind === 'web' ? 'browser' : targetName; |
There was a problem hiding this comment.
When pnpm build:microsoft-store reaches this branch, it sets APP_TARGET=browser; the game runtime then calls targetConfigKeyForPlatform(runtime.target) in apps/game-phaser/src/platform/installPlatform.ts and the generated template, which maps browser back to web-preview. As a result the Microsoft Store bundle's in-game effective config, release profile, and policy metadata are computed as web-preview even though the copied artifact contains microsoft-store, so any Store-specific gating such as requiresStoreReview is invisible to the running app.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fe8c7de. Web-kind builds now inject MPGD_CONFIG_TARGET separately from APP_TARGET, so Microsoft Store keeps configTarget: "microsoft-store" while still using the browser adapter/runtime target.
| "requiresAitReview": false | ||
| } | ||
| }, | ||
| "microsoft-store": { |
There was a problem hiding this comment.
Adding this matrix entry makes writeEffectiveTargetConfigs() validate microsoft-store on every run, even when only a requested target such as web-preview is being written. Existing generated games' mpgd.targets.json files do not define a microsoft-store platform target, so upgrading the kit/CLI and running mpgd target build web against those files fails during effective-config validation because sources.platformTargetKind is undefined; make this target opt-in/migrate missing target files or validate only requested targets so existing generated games can still build.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fe8c7de. Build preflight and effective config artifact generation now validate requested targets only, so legacy generated target files that do not yet define microsoft-store can still build web-preview/other requested targets. I also reproduced this with a temporary legacy target file missing microsoft-store.
|
Patched the new review feedback in fe8c7de:
Validation run:
@codex review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe8c7de56a
ℹ️ 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".
| const gameTemplateDir = path.resolve(packageRoot, 'templates/phaser-game'); | ||
| const cliVersion = readPackageVersion(packageRoot); | ||
| const defaultMatrixTargets = 'web,ait,reddit'; | ||
| const defaultMatrixTargets = 'web,microsoft-store,ait,reddit'; |
There was a problem hiding this comment.
When the published CLI is used from an existing generated game whose mpgd.targets.json predates the new microsoft-store entry, running mpgd target build-all or smoke-all without --targets now expands to microsoft-store and then invokes the kit scripts for a target that is absent from that file, failing as an unknown target. This bypasses the target-scoped validation fallback added for legacy target files; please derive the default from the configured targets or keep the implicit default to targets present in older templates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fdff0ac. Matrix defaults now derive from the configured target file: omitted --targets uses the recommended target order filtered by configured targets, while --targets all uses all configured targets. I reproduced this with a temporary legacy target file missing microsoft-store; mpgd target build-all selected web-preview, ait, and reddit without trying the absent Store target.
| "microsoft-store": { | ||
| "kind": "web", | ||
| "gameApp": "apps/game-phaser", | ||
| "adapter": "browser", | ||
| "output": "artifacts/microsoft-store" |
There was a problem hiding this comment.
When this new target is selected, build-target copies apps/game-phaser/dist directly to artifacts/microsoft-store, but a repo search found no linked web app manifest or manifest.webmanifest/manifest.json under apps/game-phaser. Microsoft’s PWA docs describe the web app manifest as the piece that makes a web app installable as a PWA (https://learn.microsoft.com/en-us/microsoft-edge/progressive-web-apps/how-to/), so PWABuilder’s Store readiness check won’t have the required app metadata/icons to package; please add and link a manifest for this artifact before treating it as a Microsoft Store PWA target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fdff0ac. The game app and generated Phaser template now ship a linked manifest.webmanifest plus app icon metadata, and smoke:target microsoft-store verifies the artifact has the manifest link, manifest metadata, and local icon file. I also checked Microsoft's PWA guidance while applying this.
|
Patched the latest Codex feedback in fdff0ac:
Validation run:
I also verified the Microsoft PWA manifest requirement against the official Edge PWA guidance. OCR connectivity passed, but the workspace review run produced no output for several minutes and was cancelled after local validation completed. @codex review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/game-phaser/src/platform/runtimeDetector.test.ts (1)
16-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for non-browser passthrough branch.
Coverage only exercises the
target === 'browser'branch ofnormalizeConfigTarget. Add a case for a non-browser target (e.g.'android') to confirmvaluefalls back totargetunchanged.✅ Suggested additional test
it('maps browser config to web-preview when no config target is injected', () => { expect(normalizeConfigTarget('', 'browser')).toBe('web-preview'); }); + + it('falls back to the runtime target for non-browser platforms', () => { + expect(normalizeConfigTarget('', 'android')).toBe('android'); + }); });🤖 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 `@apps/game-phaser/src/platform/runtimeDetector.test.ts` around lines 16 - 23, Add a test case in runtimeDetector.test.ts for the normalizeConfigTarget function that covers a non-browser target such as android. The current tests only verify the browser branch, so add a passthrough assertion confirming that when the target is not browser and no explicit config target is provided, normalizeConfigTarget returns the target unchanged.apps/game-phaser/src/platform/runtimeDetector.ts (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate mapping logic with
targetConfigKeyForPlatform.
normalizeConfigTarget's fallback branch (target === 'browser' ? 'web-preview' : target) reimplements the same rule already defined inpackages/target-config/src/runtime.ts(targetConfigKeyForPlatform). Consider reusing that function here instead of duplicating the mapping, so both stay in sync if the default target-key rule ever changes.♻️ Proposed refactor
+import { targetConfigKeyForPlatform } from '`@mpgd/target-config`'; + export function normalizeConfigTarget(value: string, target: PlatformTarget): string { - return value.length > 0 ? value : target === 'browser' ? 'web-preview' : target; + return value.length > 0 ? value : targetConfigKeyForPlatform(target); }🤖 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 `@apps/game-phaser/src/platform/runtimeDetector.ts` around lines 30 - 33, The fallback mapping in normalizeConfigTarget duplicates the browser-to-web-preview rule already defined by targetConfigKeyForPlatform. Update normalizeConfigTarget to reuse targetConfigKeyForPlatform from packages/target-config/src/runtime.ts for the empty-value fallback instead of hardcoding target === 'browser' ? 'web-preview' : target, so the runtimeDetector logic stays aligned with the shared target-key mapping.tools/validate-effective-target-config.ts (1)
17-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
readTargetFilterFromEnvhelper across two files.This helper is byte-for-byte identical to the one added in
tools/validate-target-config.ts(Lines 52-63 there). Consider extracting it into a shared module (e.g.tools/env.tsor similar) to avoid drift between the two copies.♻️ Proposed refactor
-function readTargetFilterFromEnv(name: string): readonly string[] | undefined { - const raw = process.env[name]; - - if (raw === undefined || raw.trim().length === 0) { - return undefined; - } - - return raw - .split(',') - .map((target) => target.trim()) - .filter((target) => target.length > 0); -} +export { readTargetFilterFromEnv } from './env'; // shared implementation🤖 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/validate-effective-target-config.ts` around lines 17 - 29, The readTargetFilterFromEnv helper is duplicated in tools/validate-effective-target-config.ts and tools/validate-target-config.ts, so move that logic into a shared utility module and have both scripts import it. Keep the existing behavior of trimming, splitting, and filtering env values, but centralize it under a single helper name so changes only need to be made once.
🤖 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 `@apps/game-phaser/src/platform/runtimeDetector.test.ts`:
- Around line 16-23: Add a test case in runtimeDetector.test.ts for the
normalizeConfigTarget function that covers a non-browser target such as android.
The current tests only verify the browser branch, so add a passthrough assertion
confirming that when the target is not browser and no explicit config target is
provided, normalizeConfigTarget returns the target unchanged.
In `@apps/game-phaser/src/platform/runtimeDetector.ts`:
- Around line 30-33: The fallback mapping in normalizeConfigTarget duplicates
the browser-to-web-preview rule already defined by targetConfigKeyForPlatform.
Update normalizeConfigTarget to reuse targetConfigKeyForPlatform from
packages/target-config/src/runtime.ts for the empty-value fallback instead of
hardcoding target === 'browser' ? 'web-preview' : target, so the runtimeDetector
logic stays aligned with the shared target-key mapping.
In `@tools/validate-effective-target-config.ts`:
- Around line 17-29: The readTargetFilterFromEnv helper is duplicated in
tools/validate-effective-target-config.ts and tools/validate-target-config.ts,
so move that logic into a shared utility module and have both scripts import it.
Keep the existing behavior of trimming, splitting, and filtering env values, but
centralize it under a single helper name so changes only need to be made once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9345250e-7bf4-4163-9c88-d343be4e2ac0
⛔ Files ignored due to path filters (1)
packages/cli/templates/phaser-game/public/icon.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
.sampo/changesets/microsoft-store-pwa-target.mdREADME.mdapps/game-phaser/index.htmlapps/game-phaser/public/manifest.webmanifestapps/game-phaser/src/platform/installPlatform.tsapps/game-phaser/src/platform/runtimeDetector.test.tsapps/game-phaser/src/platform/runtimeDetector.tsapps/game-phaser/src/vite-env.d.tsapps/game-phaser/vite.config.tspackages/cli/src/index.tspackages/cli/templates/phaser-game/README.mdpackages/cli/templates/phaser-game/index.htmlpackages/cli/templates/phaser-game/public/manifest.webmanifestpackages/cli/templates/phaser-game/src/env.d.tspackages/cli/templates/phaser-game/src/platform/installPlatform.tspackages/cli/templates/phaser-game/src/platform/runtimeDetector.tspackages/cli/templates/phaser-game/vite.config.tstools/smoke/verify-target-artifacts.tstools/target/build-target.tstools/target/effective-config.tstools/validate-effective-target-config.tstools/validate-target-config.ts
✅ Files skipped from review due to trivial changes (5)
- packages/cli/templates/phaser-game/index.html
- .sampo/changesets/microsoft-store-pwa-target.md
- apps/game-phaser/index.html
- packages/cli/templates/phaser-game/src/env.d.ts
- packages/cli/templates/phaser-game/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdff0ac4ad
ℹ️ 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".
| return linkTags.some( | ||
| (tag) => | ||
| /\brel=["']manifest["']/iu.test(tag) | ||
| && /\bhref=["']\/?manifest\.webmanifest["']/iu.test(tag), |
There was a problem hiding this comment.
For generated games, the starter Vite config uses base: './', so Vite can rewrite HTML asset references in the built artifact to href="./manifest.webmanifest". This check only accepts /manifest.webmanifest or manifest.webmanifest, which means mpgd target smoke microsoft-store can fail on a valid generated Microsoft Store artifact even though the manifest exists and is linked. Please allow the ./ form or parse and resolve the href instead of matching only these two spellings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ba2229c. The Microsoft Store smoke check now normalizes local web hrefs, so /manifest.webmanifest, manifest.webmanifest, and ./manifest.webmanifest all resolve to the expected artifact manifest link.
| assertString(icon.src, `Microsoft Store PWA manifest icon ${index} src`); | ||
| assertString(icon.sizes, `Microsoft Store PWA manifest icon ${index} sizes`); | ||
|
|
||
| const iconPath = resolve(dirname(manifestPath), icon.src); |
There was a problem hiding this comment.
When a game manifest uses a root-relative local icon such as /icon.svg, this resolves it as the filesystem path /icon.svg instead of as artifactPath/icon.svg, so the Microsoft Store smoke test fails the inside-artifact/file-exists checks even though the built PWA contains a valid local icon at the web root. Please treat web-root-relative icon URLs as relative to the artifact root before checking the file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ba2229c. Manifest icon paths now resolve as artifact web paths: root-relative icon URLs such as /icon.svg are checked under the artifact root, while relative paths still resolve from the manifest directory.
|
CI fix pushed in 4109ee5. Root cause: Fix: Validated locally:
@codex review |
|
✅ Action performedReview finished.
|
|
Addressed the latest Codex smoke feedback in ba2229c. Changes:
Validated locally:
@codex review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Hooray! 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
microsoft-storePWA release target that reuses the browserPlatformGatewayand writesartifacts/microsoft-store.microsoft-store-pwaeffective target metadata, smoke coverage, CLI generated-game target support, and Microsoft Store CI/release workflow hooks.Validation
pnpm validate:target-configpnpm validate:targetspnpm validate:effective-configpnpm validate:starter-workflowpnpm smoke:target-configpnpm smoke:effective-configpnpm smoke:adapter-effective-configMPGD_SKIP_BUILD_TARGET_PREFLIGHT=1 pnpm build:microsoft-storepnpm smoke:target microsoft-storepnpm checkpnpm testpnpm pack:packages:preparedocr review --audience agent --background ...and patched the single actionable assertion-message issueNotes
pnpm sampo:release:dry-runwas not runnable on this feature branch because Sampo release config only allowsmainfor release dry-runs.Summary by CodeRabbit
New Features
msstorealias).manifest.webmanifest).Bug Fixes
all/defaultcan follow the configured platform targets list.Documentation
manifest.webmanifest.