Repository navigation
feat: add storage adapter conformance - #90
Conversation
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 48 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 (1)
📝 WalkthroughWalkthroughStorage persistence now follows shared fail-closed conformance rules across browser, AIT, Devvit, Verse8, Capacitor Android/iOS, and smoke-test targets. New quotas, structured errors, native backends, adapter tests, CI checks, documentation, and package exports cover the updated behavior. ChangesStorage Conformance
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
![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: 921e8407b4
ℹ️ 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".
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Preserve JSON null instead of loading it as missing
When a game saves value: null, saveStorage serializes it as the literal null; this line then sends bridge data null back, and the Devvit gateway interprets bridge null as a missing key rather than { value: null }. That violates the advertised JSON-serializable round-trip contract and can silently turn a nullable save slot into an absent one; wrap persisted values or reject top-level null so stored null is distinguishable from no stored value.
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tools/smoke/storage-adapter-conformance.ts (1)
90-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplication and unsafe casts in
createBridgeFixture.The four branches (android/ios/ait/reddit) repeat near-identical primary/isolated construction, each force-casting a minimal
{ request }stub viaas NativeBridge/as GamePlatformBridge/as DevvitBridge. This works today but hides any contract drift if those bridge interfaces grow beyondrequest(), and the duplication makes adding a new target tedious.♻️ Proposed refactor
- if (target === 'android' || target === 'ios') { - primary = createCapacitorPlatformGateway({ - target, - appVersion: '1.0.0', - buildId: 'storage-conformance', - bridge: provider.bridge('primary-account', true) as NativeBridge, - }).storage; - isolated = createCapacitorPlatformGateway({ - target, - appVersion: '1.0.0', - buildId: 'storage-conformance', - bridge: provider.bridge('isolated-account', false) as NativeBridge, - }).storage; - } else if (target === 'ait') { - ... - } else { - ... - } + const gatewayFactories = { + android: (bridge: unknown) => + createCapacitorPlatformGateway({ target, appVersion: '1.0.0', buildId: 'storage-conformance', bridge: bridge as NativeBridge }), + ios: (bridge: unknown) => + createCapacitorPlatformGateway({ target, appVersion: '1.0.0', buildId: 'storage-conformance', bridge: bridge as NativeBridge }), + ait: (bridge: unknown) => + createAitPlatformGateway({ appVersion: '1.0.0', buildId: 'storage-conformance', bridge: bridge as GamePlatformBridge }), + reddit: (bridge: unknown) => + createDevvitPlatformGateway({ appVersion: '1.0.0', buildId: 'storage-conformance', bridge: bridge as DevvitBridge }), + } as const; + primary = gatewayFactories[target](provider.bridge('primary-account', true)).storage; + isolated = gatewayFactories[target](provider.bridge('isolated-account', false)).storage;🤖 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/storage-adapter-conformance.ts` around lines 90 - 135, Refactor createBridgeFixture to use a target-to-platform factory or shared construction helper instead of repeating primary and isolated gateway creation for each target. Define the bridge provider and gateway configuration with the complete platform-specific bridge types so provider.bridge results are type-safe without force-casting to NativeBridge, GamePlatformBridge, or DevvitBridge. Preserve the existing target mappings, account identifiers, isolation flags, gateway metadata, and returned createFixture values.
🤖 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 `@adapters/verse8/src/index.test.ts`:
- Around line 96-106: Restore the original globalThis.localStorage value after
the “fails closed when local storage is unavailable” test completes. Capture the
existing property before Reflect.deleteProperty, then restore it in cleanup
regardless of assertion success, preserving the test’s simulated-unavailable
behavior without leaking global state.
In `@apps/target-devvit/src/server/index.ts`:
- Line 283: Remove the key value from both storage failure warnings, leaving
only the failure context and error details. Apply this in
apps/target-devvit/src/server/index.ts at lines 283-283 and 350-350, and mirror
the sanitized load and save warnings in
packages/cli/templates/phaser-game/apps/target-devvit/src/server/index.ts at
lines 285-285 and 352-352.
In
`@native-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/CapacitorGameServicesPlugin.java`:
- Around line 215-225: Update the save handler surrounding localStorage().save
in the CapacitorGameServicesPlugin to catch unexpected native exceptions in
addition to LocalJsonStorage.StorageException, and resolve the bridge call
through the existing errorResponse path rather than allowing initialization
failures to escape. Preserve the current success response and
StorageException-specific code and retryability fields.
In
`@native-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/LocalJsonStorage.swift`:
- Around line 92-94: The set method currently uses UserDefaults, which cannot
report persistence failures despite its throwing contract and
LocalJsonStorageError.saveFailed path. Update set and the corresponding storage
backend to use a write mechanism that surfaces errors, or remove the
throwing/save-failed contract consistently if failures are not supported.
In `@packages/platform/src/storage-conformance.ts`:
- Around line 205-211: Update the mutation probes around mutableLoadedValue so
the coins assignment and inventory push execute independently, each with its own
try/catch. Preserve the existing isolation read and immutable-value handling,
ensuring the inventory probe still runs when the progress mutation throws.
---
Nitpick comments:
In `@tools/smoke/storage-adapter-conformance.ts`:
- Around line 90-135: Refactor createBridgeFixture to use a target-to-platform
factory or shared construction helper instead of repeating primary and isolated
gateway creation for each target. Define the bridge provider and gateway
configuration with the complete platform-specific bridge types so
provider.bridge results are type-safe without force-casting to NativeBridge,
GamePlatformBridge, or DevvitBridge. Preserve the existing target mappings,
account identifiers, isolation flags, gateway metadata, and returned
createFixture values.
🪄 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: bd05cd35-da4f-49dd-868d-9153586ef4b2
📒 Files selected for processing (28)
.github/workflows/ci.yml.sampo/changesets/storage-adapter-conformance.mdadapters/browser/src/index.test.tsadapters/browser/src/index.tsadapters/devvit/src/index.test.tsadapters/devvit/src/index.tsadapters/verse8/src/index.test.tsadapters/verse8/src/index.tsapps/target-devvit/src/server/index.tsdocs/GAME_DEVELOPMENT.mdnative-plugins/capacitor-game-services/README.mdnative-plugins/capacitor-game-services/android/build.gradlenative-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/CapacitorGameServicesPlugin.javanative-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/LocalJsonStorage.javanative-plugins/capacitor-game-services/android/src/test/java/dev/mpgd/capacitor/LocalJsonStorageTest.javanative-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/CapacitorGameServicesPlugin.swiftnative-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/LocalJsonStorage.swiftnative-plugins/capacitor-game-services/ios/Tests/LocalJsonStorageConformance/main.swiftpackage.jsonpackages/cli/templates/phaser-game/README.mdpackages/cli/templates/phaser-game/apps/target-devvit/src/server/index.tspackages/platform/package.jsonpackages/platform/src/index.tspackages/platform/src/storage-conformance.test.tspackages/platform/src/storage-conformance.tspackages/platform/test/storage-conformance-dist-import.mjstools/smoke/platform-capability-conformance.tstools/smoke/storage-adapter-conformance.ts
|
Addressed the Codex stored-null finding in 5e8943b. Bridge-backed storage now returns an explicit |
|
@codex review @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: 5e8943bdaa
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
`@native-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/CapacitorGameServicesPlugin.swift`:
- Around line 11-13: Update the CapacitorGameServicesPlugin localStorage
initialization to migrate existing values from the previous UserDefaults backend
into FileLocalJsonStorageBackend on first access. Read each legacy value, write
it to file storage, and remove the corresponding UserDefaults entry only after
the file write succeeds; preserve normal file-backed access after migration.
🪄 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: 08088e9e-8e6d-4405-8b37-c416639a0358
📒 Files selected for processing (21)
.sampo/changesets/storage-adapter-conformance.mdadapters/ait/src/index.test.tsadapters/ait/src/index.tsadapters/capacitor/src/index.test.tsadapters/capacitor/src/index.tsadapters/devvit/src/index.test.tsadapters/devvit/src/index.tsadapters/verse8/src/index.test.tsapps/target-ait/src/aitBridge.tsapps/target-ait/src/bridgeTypes.tsapps/target-devvit/src/server/index.tsnative-plugins/capacitor-game-services/README.mdnative-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/CapacitorGameServicesPlugin.javanative-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/CapacitorGameServicesPlugin.swiftnative-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/LocalJsonStorage.swiftnative-plugins/capacitor-game-services/ios/Tests/LocalJsonStorageConformance/main.swiftpackages/bridge/src/bridge-protocol.test.tspackages/bridge/src/index.tspackages/cli/templates/phaser-game/apps/target-devvit/src/server/index.tspackages/platform/src/storage-conformance.tstools/smoke/storage-adapter-conformance.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- native-plugins/capacitor-game-services/README.md
- .sampo/changesets/storage-adapter-conformance.md
- adapters/verse8/src/index.test.ts
- native-plugins/capacitor-game-services/ios/Tests/LocalJsonStorageConformance/main.swift
- packages/cli/templates/phaser-game/apps/target-devvit/src/server/index.ts
- tools/smoke/storage-adapter-conformance.ts
- apps/target-devvit/src/server/index.ts
- native-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/CapacitorGameServicesPlugin.java
- adapters/devvit/src/index.ts
- packages/platform/src/storage-conformance.ts
- adapters/devvit/src/index.test.ts
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
f3a6807 to
eacbe20
Compare
|
@codex review @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: eacbe20053
ℹ️ 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".
|
@codex review @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: d26c2fc7e2
ℹ️ 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".
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
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 `@apps/target-ait/src/aitBridge.test.ts`:
- Around line 118-199: Wrap the installed bridge test sequence after
installAitBridge() in a try...finally block, and move
Reflect.deleteProperty(bridgeHost, '__GAME_PLATFORM_BRIDGE__') into the finally
block to guarantee global cleanup on assertion or await failures. Replace the
firstLoad.ok equality assertion and manual failure branch with
assert(firstLoad.ok), and use direct firstLoad.data access in subsequent
assertions instead of conditional ternaries.
🪄 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: ac79d7c1-9f45-46d8-a10e-00fa64937340
📒 Files selected for processing (39)
.github/workflows/ci.yml.sampo/changesets/storage-adapter-conformance.mdadapters/ait/src/index.test.tsadapters/ait/src/index.tsadapters/browser/src/index.test.tsadapters/browser/src/index.tsadapters/capacitor/src/index.test.tsadapters/capacitor/src/index.tsadapters/devvit/src/index.test.tsadapters/devvit/src/index.tsadapters/verse8/src/index.test.tsadapters/verse8/src/index.tsapps/target-ait/src/aitBridge.test.tsapps/target-ait/src/aitBridge.tsapps/target-ait/src/bridgeTypes.tsapps/target-devvit/src/server/index.tsdocs/GAME_DEVELOPMENT.mdnative-plugins/capacitor-game-services/README.mdnative-plugins/capacitor-game-services/android/build.gradlenative-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/CapacitorGameServicesPlugin.javanative-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/LocalJsonStorage.javanative-plugins/capacitor-game-services/android/src/test/java/dev/mpgd/capacitor/LocalJsonStorageTest.javanative-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/CapacitorGameServicesPlugin.swiftnative-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/LocalJsonStorage.swiftnative-plugins/capacitor-game-services/ios/Tests/LocalJsonStorageConformance/main.swiftpackage.jsonpackages/bridge/src/bridge-protocol.test.tspackages/bridge/src/cloudflare-pages.test.tspackages/bridge/src/cloudflare-pages.tspackages/bridge/src/index.tspackages/cli/templates/phaser-game/README.mdpackages/cli/templates/phaser-game/apps/target-devvit/src/server/index.tspackages/platform/package.jsonpackages/platform/src/index.tspackages/platform/src/storage-conformance.test.tspackages/platform/src/storage-conformance.tspackages/platform/test/storage-conformance-dist-import.mjstools/smoke/platform-capability-conformance.tstools/smoke/storage-adapter-conformance.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- adapters/verse8/src/index.test.ts
- native-plugins/capacitor-game-services/README.md
- packages/platform/package.json
- apps/target-ait/src/aitBridge.ts
- native-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/LocalJsonStorage.swift
- tools/smoke/platform-capability-conformance.ts
- packages/platform/test/storage-conformance-dist-import.mjs
- .sampo/changesets/storage-adapter-conformance.md
- native-plugins/capacitor-game-services/android/src/test/java/dev/mpgd/capacitor/LocalJsonStorageTest.java
- docs/GAME_DEVELOPMENT.md
- adapters/browser/src/index.test.ts
- .github/workflows/ci.yml
- adapters/capacitor/src/index.ts
- adapters/ait/src/index.ts
- adapters/verse8/src/index.ts
- packages/platform/src/storage-conformance.ts
- native-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/LocalJsonStorage.java
- adapters/browser/src/index.ts
- packages/platform/src/storage-conformance.test.ts
- native-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/CapacitorGameServicesPlugin.swift
- package.json
- packages/cli/templates/phaser-game/apps/target-devvit/src/server/index.ts
- tools/smoke/storage-adapter-conformance.ts
- adapters/devvit/src/index.ts
- native-plugins/capacitor-game-services/android/src/main/java/dev/mpgd/capacitor/CapacitorGameServicesPlugin.java
- adapters/devvit/src/index.test.ts
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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
Validation
pnpm checkpnpm test:preparedpnpm pack:packages:preparedSAMPO_RELEASE_BRANCH=main sampo release --dry-runpnpm build:webpnpm build:microsoft-storepnpm build:verse8pnpm build:devvitpnpm build:target ait sandboxAIT production packaging was also exercised and stopped at the existing external-console prerequisite:
ait.icon.externalUrlis intentionally absent from the repository. No personal or production console configuration is added.Release
Includes a focused Sampo changeset for the affected publishable package contracts.
Summary by CodeRabbit
storage-conformancepackage export.nullare distinguished reliably; load/save results use explicit “found” semantics.