Repository navigation
Support game-owned target release inputs - #36
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 19 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)
📝 WalkthroughWalkthroughThe PR adds game-owned catalog, ad-placement, version, and source-revision inputs; updates target builds and release manifests; enables ESM-compatible target-config output; broadens platform identifiers; and adds validation, smoke tests, and release changesets. ChangesRelease inputs and build compatibility
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as CLI target command
participant GameRoot as Game root
participant Catalog as Catalog and placement files
participant Git as Git repository
participant Build as Target build
CLI->>GameRoot: resolve target configuration directory
GameRoot->>Catalog: locate game-owned monetization files
GameRoot->>Git: resolve source revision when not overridden
CLI-->>Build: pass catalog, placements, version, and source SHA
Build->>Catalog: validate and embed effective target configuration
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (2)
packages/platform/src/index.test.ts (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding starter-literal
satisfieschecks for symmetry.The test verifies game-owned arbitrary strings satisfy the broadened types, which is the key new contract. Adding equivalent checks for starter literals (e.g.,
'COINS_100' satisfies LogicalProductId) would document that backward compatibility is intentional and guard against accidental narrowing in future refactors.♻️ Optional: add starter-literal checks
const gameOwnedProductId = 'SUDOKU_THEME_PACK' satisfies LogicalProductId; const gameOwnedAdPlacementId = 'SUDOKU_HINT_REWARDED' satisfies LogicalAdPlacementId; +const starterProductId = 'COINS_100' satisfies LogicalProductId; +const starterAdPlacementId = 'CONTINUE_AFTER_FAIL' satisfies LogicalAdPlacementId; void gameOwnedProductId; void gameOwnedAdPlacementId; +void starterProductId; +void starterAdPlacementId;🤖 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 `@packages/platform/src/index.test.ts` around lines 3 - 7, Add starter-literal compile-time checks alongside game-owned checks in the test, using literals such as 'COINS_100' satisfies LogicalProductId and the corresponding starter ad placement literal satisfies LogicalAdPlacementId; reference the existing gameOwnedProductId and gameOwnedAdPlacementId declarations for symmetry, and consume the new constants to avoid unused-variable errors.tools/smoke/cli-game-release-inputs.ts (1)
91-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the
spawnSynccall.Without a timeout, a hung CLI process will block the test indefinitely in CI. Adding
timeout(e.g., 30s) ensures the test fails fast on hangs.♻️ Proposed fix
{ cwd: process.cwd(), encoding: 'utf8', env, + timeout: 30_000, },🤖 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/cli-game-release-inputs.ts` around lines 91 - 110, Add a finite timeout, such as 30 seconds, to the options object passed to the spawnSync call in the CLI release-inputs test. Keep the existing cwd, encoding, and env settings unchanged so hung CLI processes terminate and the test fails promptly.
🤖 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 `@packages/platform/src/index.test.ts`:
- Around line 3-7: Add starter-literal compile-time checks alongside game-owned
checks in the test, using literals such as 'COINS_100' satisfies
LogicalProductId and the corresponding starter ad placement literal satisfies
LogicalAdPlacementId; reference the existing gameOwnedProductId and
gameOwnedAdPlacementId declarations for symmetry, and consume the new constants
to avoid unused-variable errors.
In `@tools/smoke/cli-game-release-inputs.ts`:
- Around line 91-110: Add a finite timeout, such as 30 seconds, to the options
object passed to the spawnSync call in the CLI release-inputs test. Keep the
existing cwd, encoding, and env settings unchanged so hung CLI processes
terminate and the test fails promptly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30acc61e-459c-49af-91b4-6b05226046c4
📒 Files selected for processing (26)
.sampo/changesets/bright-game-owned-ids.md.sampo/changesets/calm-target-config-esm.md.sampo/changesets/steady-cli-game-release-inputs.mdapps/target-devvit/vite.server.config.tsexamples/phaser-starter/src/env.d.tsexamples/phaser-starter/vite.config.tspackage.jsonpackages/cli/src/index.tspackages/cli/templates/phaser-game/apps/target-devvit/vite.server.config.tspackages/cli/templates/phaser-game/src/env.d.tspackages/cli/templates/phaser-game/vite.config.tspackages/platform/src/index.test.tspackages/platform/src/index.tspackages/target-config/src/effective.tspackages/target-config/src/index.tspackages/target-config/src/runtime.tspackages/target-config/src/viewport.tspackages/target-config/test/dist-import.mjstools/catalog-paths.tstools/smoke/cli-game-release-inputs.tstools/smoke/game-owned-catalog.tstools/smoke/release-manifest-merge.tstools/target/effective-config.tstools/target/generate-release-manifest.tstools/validate-ad-placements.tstools/validate-product-catalog.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: baeba20013
ℹ️ 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".
|
Addressed the current review feedback in 4a6a9f3:
Local validation after the patch:
@coderabbitai 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: 4a6a9f33fa
ℹ️ 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".
|
Addressed the latest Codex P2 feedback in d894aae:
Local validation after the patch:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d894aae633
ℹ️ 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".
|
Addressed the latest Codex P2 round in a76e703:
Local validation after the patch:
@coderabbitai 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: a76e703c9e
ℹ️ 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".
|
Fixed the CI-only failure from the previous push in a058802:
Local validation after the CI fix:
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
Addressed the latest Codex relative-path feedback in ca634b0:
Local validation after the patch:
@coderabbitai 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: ca634b03d3
ℹ️ 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".
|
Addressed the latest Codex target catalog env-base feedback in b6fbaa3:
Local validation after the patch:
@coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/target/monetization-catalog-env.ts (1)
11-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated contract with
tools/catalog-paths.ts.
readConfiguredEnvPath(Lines 34-37) and the "configured together" validation (Lines 11-15) reproducereadConfiguredPathand the exact error message already defined inmonetizationCatalogFilePaths(tools/catalog-paths.ts:17-37). Two sources of truth for the same env contract can silently diverge, and the message string is asserted verbatim bytools/smoke/game-owned-catalog.ts. Consider importing/reusing the shared helper and message so the paired-config contract stays in one place.🤖 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/monetization-catalog-env.ts` around lines 11 - 37, The monetization environment parsing duplicates the paired-file contract already centralized in monetizationCatalogFilePaths from tools/catalog-paths.ts. Refactor the relevant function to reuse the shared readConfiguredPath helper and paired-configuration validation/error message, removing readConfiguredEnvPath and the local duplicate check while preserving the existing resolved environment output.
🤖 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 `@tools/target/monetization-catalog-env.ts`:
- Around line 45-64: Update the candidate ordering in the path-resolution logic
around configuredFileExists so configBaseDir/fallbackBaseDir is checked before
INIT_CWD and PWD, ensuring game-owned monetization files take precedence. Apply
the same precedence to the fallback return expression, and add a smoke test
covering conflicting relative catalog and ad-placement files across bases.
---
Nitpick comments:
In `@tools/target/monetization-catalog-env.ts`:
- Around line 11-37: The monetization environment parsing duplicates the
paired-file contract already centralized in monetizationCatalogFilePaths from
tools/catalog-paths.ts. Refactor the relevant function to reuse the shared
readConfiguredPath helper and paired-configuration validation/error message,
removing readConfiguredEnvPath and the local duplicate check while preserving
the existing resolved environment output.
🪄 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: 7f68bfba-e3c6-4a3f-b9ae-eb58b0812265
📒 Files selected for processing (5)
examples/phaser-starter/vite.config.tspackages/cli/templates/phaser-game/vite.config.tstools/smoke/game-owned-catalog.tstools/target/build-target.tstools/target/monetization-catalog-env.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/templates/phaser-game/vite.config.ts
- examples/phaser-starter/vite.config.ts
- tools/smoke/game-owned-catalog.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6fbaa3de1
ℹ️ 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".
|
Addressed the latest CodeRabbit/Codex follow-up feedback in 3eff5e9:
Local validation after the patch:
@coderabbitai review |
|
✅ Action performedReview finished.
✅ Action performedReview finished.
|
|
Addressed the remaining CodeRabbit catalog-env parsing nitpick in bfdd43f:
Local validation after the patch:
@coderabbitai 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: bfdd43f93f
ℹ️ 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".
|
Addressed the latest Codex P2 review round in 45ddd9a:
Local validation after the patch:
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Breezy! 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". |
45ddd9a to
e8d26f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
examples/phaser-starter/vite.config.ts (1)
45-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatalog path helpers duplicated from
tools/catalog-paths.ts
readConfiguredPath(lines 66–68) is an exact duplicate oftools/catalog-paths.ts:45–48, and the paired-configuration enforcement increateCatalogAliases(lines 50–54) mirrorsconfiguredMonetizationCatalogFilePaths. The template atpackages/cli/templates/phaser-game/vite.config.tscarries the same copies. Consider importingreadConfiguredPathandconfiguredMonetizationCatalogFilePathsfromtools/catalog-paths.tsin the example starter to avoid drift. The template may need to remain self-contained, but the monorepo's own configs should use the shared helpers as the single source of truth.🤖 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 `@examples/phaser-starter/vite.config.ts` around lines 45 - 68, Replace the duplicated path normalization and paired-configuration logic in createCatalogAliases with the shared readConfiguredPath and configuredMonetizationCatalogFilePaths helpers from tools/catalog-paths.ts, then build the aliases from their returned paths while preserving existing behavior. Apply this shared-helper usage to the monorepo example starter; the self-contained Phaser template may retain local implementations.tools/smoke/game-owned-catalog.ts (1)
303-336: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd subprocess timeout to
runValidatorandassertValidatorFailurefor consistency.The CLI release-input smoke test (
cli-game-release-inputs.ts) includes a 30-second timeout on itsspawnSynccall, butrunValidatorandassertValidatorFailuredo not. A hung validator subprocess will stall the smoke test indefinitely until CI times out.♻️ Proposed fix
function runValidator(script: string): string { const result = spawnSync(process.execPath, ['tools/run-ttsx.mjs', script], { cwd: process.cwd(), encoding: 'utf8', env: process.env, + timeout: 30_000, }); if (result.error !== undefined) { throw result.error; } assert.equal(result.status, 0, result.stderr || result.stdout); return result.stdout; } function assertValidatorFailure( script: string, env: NodeJS.ProcessEnv, pattern: RegExp, ): void { const result = spawnSync(process.execPath, ['tools/run-ttsx.mjs', script], { cwd: process.cwd(), encoding: 'utf8', env, + timeout: 30_000, }); const output = result.stderr || result.stdout;🤖 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/game-owned-catalog.ts` around lines 303 - 336, Add a 30-second subprocess timeout to the spawnSync options in both runValidator and assertValidatorFailure, matching the timeout used by cli-game-release-inputs.ts, so hung validator processes terminate instead of blocking indefinitely.tools/smoke/release-manifest-merge.ts (1)
46-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd subprocess timeout and signal handling to
runManifest.The CLI release-input smoke test includes a 30-second timeout and explicit signal check on its
spawnSynccall, butrunManifesthas neither. A hung manifest subprocess will stall the test, and a signal kill produces a confusing "exited with status null" error message.♻️ Proposed fix
{ cwd: process.cwd(), encoding: 'utf8', env: { ...process.env, APP_VERSION: '1.0.0', BUILD_ID: 'manifest-merge-smoke', MPGD_PRODUCT_CATALOG_FILE: catalogFile, MPGD_AD_PLACEMENTS_FILE: placementsFile, MPGD_EFFECTIVE_TARGET_CONFIG_OUTPUT_DIR: effectiveConfigDir, MPGD_SOURCE_GIT_SHA: 'game-source-sha', }, + timeout: 30_000, }, ); if (result.error !== undefined) { throw result.error; } + if (result.signal !== null) { + throw new Error( + `Manifest subprocess was killed by signal ${result.signal}:\n${result.stderr || result.stdout || '(no output)'}`, + ); + } + assert.equal( result.status, 0, `Manifest subprocess exited with status ${String(result.status)}:\n${result.stderr || result.stdout || '(no output)'}`, );🤖 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/release-manifest-merge.ts` around lines 46 - 81, Update runManifest to pass a 30-second timeout to spawnSync and handle signal termination explicitly. After checking result.error, assert that result.signal is null, including the signal in the failure message, then retain the exit-status assertion for non-signaled processes.
🤖 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 `@examples/phaser-starter/vite.config.ts`:
- Around line 75-93: Update resolveCatalogBaseDir to prioritize the game-owned
fallback base directory, matching resolveConfiguredBaseDir, before checking
INIT_CWD, PWD, or process.cwd(). Normalize configured environment paths through
readConfiguredPath rather than accessing process.env.INIT_CWD and
process.env.PWD directly, while preserving the paired catalog existence checks
and final fallback behavior.
---
Nitpick comments:
In `@examples/phaser-starter/vite.config.ts`:
- Around line 45-68: Replace the duplicated path normalization and
paired-configuration logic in createCatalogAliases with the shared
readConfiguredPath and configuredMonetizationCatalogFilePaths helpers from
tools/catalog-paths.ts, then build the aliases from their returned paths while
preserving existing behavior. Apply this shared-helper usage to the monorepo
example starter; the self-contained Phaser template may retain local
implementations.
In `@tools/smoke/game-owned-catalog.ts`:
- Around line 303-336: Add a 30-second subprocess timeout to the spawnSync
options in both runValidator and assertValidatorFailure, matching the timeout
used by cli-game-release-inputs.ts, so hung validator processes terminate
instead of blocking indefinitely.
In `@tools/smoke/release-manifest-merge.ts`:
- Around line 46-81: Update runManifest to pass a 30-second timeout to spawnSync
and handle signal termination explicitly. After checking result.error, assert
that result.signal is null, including the signal in the failure message, then
retain the exit-status assertion for non-signaled processes.
🪄 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: 8aa622a0-b96e-4161-b53a-9be8a724d402
📒 Files selected for processing (29)
.sampo/changesets/bright-game-owned-ids.md.sampo/changesets/calm-target-config-esm.md.sampo/changesets/steady-cli-game-release-inputs.mdapps/target-devvit/vite.server.config.tsexamples/phaser-starter/src/env.d.tsexamples/phaser-starter/vite.config.tspackage.jsonpackages/cli/src/index.tspackages/cli/templates/phaser-game/apps/target-devvit/vite.server.config.tspackages/cli/templates/phaser-game/src/env.d.tspackages/cli/templates/phaser-game/vite.config.tspackages/platform/src/index.test.tspackages/platform/src/index.tspackages/target-config/src/effective.tspackages/target-config/src/index.tspackages/target-config/src/runtime.tspackages/target-config/src/viewport.tspackages/target-config/test/dist-import.mjstools/catalog-paths.tstools/smoke/cli-game-release-inputs.tstools/smoke/game-owned-catalog.tstools/smoke/release-manifest-merge.tstools/target/build-target.tstools/target/effective-config.tstools/target/generate-release-manifest.tstools/target/monetization-catalog-env.tstools/validate-ad-placements.tstools/validate-product-catalog.tstools/validate-public-readiness.ts
✅ Files skipped from review due to trivial changes (7)
- packages/target-config/src/viewport.ts
- examples/phaser-starter/src/env.d.ts
- .sampo/changesets/steady-cli-game-release-inputs.md
- .sampo/changesets/bright-game-owned-ids.md
- packages/platform/src/index.test.ts
- .sampo/changesets/calm-target-config-esm.md
- packages/cli/templates/phaser-game/src/env.d.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/target-config/test/dist-import.mjs
- packages/target-config/src/effective.ts
- packages/target-config/src/index.ts
- packages/cli/templates/phaser-game/apps/target-devvit/vite.server.config.ts
- packages/target-config/src/runtime.ts
- apps/target-devvit/vite.server.config.ts
- tools/validate-ad-placements.ts
- tools/target/effective-config.ts
- packages/cli/templates/phaser-game/vite.config.ts
- packages/platform/src/index.ts
- package.json
- tools/validate-product-catalog.ts
Summary
Validation
pnpm checkpnpm graph:preflightpnpm testpnpm build:web && pnpm smoke:target web-previewpnpm build:ait && pnpm smoke:target aitpnpm build:devvit && pnpm smoke:target redditRelease
npm/@mpgd/platform,npm/@mpgd/target-config, andnpm/@mpgd/cli.Summary by CodeRabbit
New Features
Bug Fixes
Validation