Repository navigation
feat: let a capability package provide a whole Game mode - #4528
Conversation
github-actions
Bot
added
feature
📝 WalkthroughWalkthroughThis PR adds capability-package Game experiences with setup selection, package-owned surfaces, declarative Classic-system replacement, prompt-context contributions, resource writes, and post-activation asset rescanning. ChangesGame Experiences
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant NewGameExperienceChooser
participant GameRoute
participant GameSurface
participant ExperienceSurface
Player->>NewGameExperienceChooser: Select experience and configure setup
NewGameExperienceChooser->>GameRoute: Create game with experience metadata
GameRoute-->>NewGameExperienceChooser: Return created chat ID
GameSurface->>GameRoute: Load persisted experience ID
GameSurface->>ExperienceSurface: Mount surface with game state and UI seams
ExperienceSurface-->>GameSurface: Provide backgrounds, avatars, chrome, and choices
Possibly related PRs
Suggested reviewers: 🚥 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.
Actionable comments posted: 8
🤖 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/client/src/components/game/GameSurface.tsx`:
- Around line 3416-3451: Reset the experience seam state when the experience
scope changes: clear experienceAvatars, experienceChrome, and
experienceBackgroundUrl in the existing runtime reset logic keyed by
sceneRuntimeScopeKey and experienceSurfaceId. Update the relevant effects near
the activeChatId/sceneRuntimeScopeKey resets, ensuring stale avatar, background,
and providesPlayerInput declarations cannot persist across experience switches.
- Around line 6459-6467: Update pushExperienceBackground so
experienceBackgroundUrl is assigned only after validating the value as a
permitted game-asset path: resolve it through scopedAssetMap and accept
compatible game-assets entries, or validate package-owned paths with an explicit
scheme/hostname allowlist. Reject unsupported or untrusted values instead of
falling back to arbitrary value strings before CrossfadeBackground.
In `@packages/client/src/components/game/NewGameExperienceChooser.tsx`:
- Around line 224-252: Update the setup panel motion.div in
NewGameExperienceChooser to include role="dialog", aria-modal="true", and
aria-labelledby referencing the panel title. Add Escape-key handling that calls
onCancelSetup only when launching is false, while preserving the existing
backdrop and close-button behavior.
In `@packages/server/src/app.ts`:
- Around line 213-216: Update the post-activation asset rescan error handling
around buildAssetManifest to call the shared logger.warn instead of
app.log.warn, preserving the existing error object and warning message.
In `@packages/server/src/routes/game.routes.ts`:
- Around line 1687-1690: Update the game setup schema around gameExperienceId
and experienceConfig: require gameExperienceId to be a non-empty manifest-style
identifier using the existing package/manifest ID validation convention, and
constrain experienceConfig with explicit limits on key count, key size, and
value/payload size before persistence. Preserve both fields as optional and keep
the config opaque to the host.
In
`@packages/server/src/services/capability-packages/capability-prompt-context.service.ts`:
- Around line 95-98: Update the catch block in the capability prompt-context
contributor flow to pass the caught error object as the first argument to
logger.warn, preserving its stack and cause. Keep the Pino format string and
packageId as the subsequent context argument, and remove the String(error)
conversion.
- Around line 78-101: Update collectCapabilityPromptContext to invoke all
contributors concurrently and await them through Promise.allSettled, preserving
contributorsByPackage registration order when assembling blocks and provides.
Wrap each contribute(request) promise with a per-contributor timeout, and treat
timed-out or rejected contributors as non-fatal by logging the contributor
identifier and continuing without its context. Ensure the overall collection
completes within the timeout bound rather than being blocked by a contributor
that never settles.
In `@packages/shared/src/types/capability-runtime.ts`:
- Around line 99-105: Replace the open string index signature in
CapabilityLorebookEntryInput with an explicitly named nested extension bag for
forward-compatible fields. Preserve name, content, and keys as validated
top-level properties so misspelled fields produce compile-time errors, and
update any affected consumers to read additional fields through the new bag.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03f135c9-cc1c-4002-8550-56f439140688
📒 Files selected for processing (17)
CHANGELOG.mddocs/development/optional-agent-packages.mdpackages/client/src/components/game/GameNarration.tsxpackages/client/src/components/game/GameSetupWizard.tsxpackages/client/src/components/game/GameSurface.tsxpackages/client/src/components/game/NewGameExperienceChooser.tsxpackages/client/src/localization/locales/en.jsonpackages/server/src/app.tspackages/server/src/routes/game.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/services/capability-packages/capability-module-runtime.service.tspackages/server/src/services/capability-packages/capability-prompt-context.service.tspackages/server/src/services/capability-packages/capability-resources.service.tspackages/server/src/services/game/gm-prompts.tspackages/shared/src/schemas/capability-package.schema.tspackages/shared/src/types/capability-runtime.tspackages/shared/src/types/game.ts
Reset the experience seam state when the game scope changes. experienceAvatars, experienceChrome and experienceBackgroundUrl are component state on a component that survives chat switches, so a switch between two experience games carried the previous game's background, avatars and providesPlayerInput declaration into the next one -- the last of which could leave the player with no way to send a turn. Add dialog semantics to the experience setup panel: role, aria-modal and aria-labelledby matching what GameSetupWizard already sets, plus Escape to dismiss while a launch is not pending. Constrain gameExperienceId to the manifest id shape, since it is matched against one to mount the surface, and bound the opaque experienceConfig so it cannot grow into a payload every later write of the setup config has to carry. Give each prompt-context contributor a deadline. A throw was already non-fatal, but a promise that never settled would hang the turn, which the documented contract said could not happen. Pass the error object first to Pino so the stack survives. Trim the comments substantially and drop the bracketed tags left over from development. The remaining ones explain why a seam exists or what breaks without it, rather than restating the code.
|
Thanks for the thorough pass. Pushed a commit addressing five of the eight comments; notes on the AppliedSeam state now resets when the game scope changes. This was a real bug and the manual testing missed Dialog semantics on the experience setup panel. Added
Pino gets the error object first. Fixed — that one was a straightforward miss against the contributing Per-contributor deadline. You were right that the I kept the calls sequential rather than moving to Verified by running the collector: a contributor that never settles is dropped at the deadline without Not appliedRestricting That said, I have no objection to adding a resolve-under- The open index signature on
Also in this commitI cut the comments back substantially, which should address the redundant-comments flag on the walkthrough.
@coderabbitai full review |
|
🐇🔍 ✏️ Learnings added
✅ Action performedFull review 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 (4)
packages/server/src/services/capability-packages/capability-prompt-context.service.ts (1)
84-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun contributors concurrently to bound total latency.
Each contributor now has a 2-second deadline, so a single hung contributor can no longer block the turn indefinitely. The
for...ofloop stillawaits contributors one at a time, so total added latency for N registered contributors stays additive (up to N × 2000ms in the worst case) instead of being bounded by the slowest contributor.Collect all contributions with
Promise.alland process the resolved array afterward.Promise.allpreserves array order regardless of completion order, so the documented "contributors run in registration order" guarantee for block composition still holds.♻️ Proposed refactor for concurrent contributor collection
export async function collectCapabilityPromptContext( request: CapabilityPromptContextRequest, ): Promise<CapabilityPromptContextResult> { if (contributorsByPackage.size === 0) return { blocks: [], provides: {} }; - const blocks: string[] = []; - const provides: CapabilityProvidedGameSystems = {}; - for (const [packageId, contribute] of contributorsByPackage) { - try { - const contribution = await withDeadline(contribute(request), packageId); - if (contribution === null || contribution === undefined) continue; - const text = typeof contribution === "string" ? contribution : contribution.text; - if (typeof text === "string" && text.trim().length > 0) blocks.push(text.trim()); - if (typeof contribution === "object" && contribution.provides?.inventory === true) { - provides.inventory = true; - } - } catch (error) { - logger.warn(error, "[capability] prompt-context contributor failed for %s", packageId); - } - } - return { blocks, provides }; + const settled = await Promise.all( + [...contributorsByPackage].map(async ([packageId, contribute]) => { + try { + return await withDeadline(contribute(request), packageId); + } catch (error) { + logger.warn(error, "[capability] prompt-context contributor failed for %s", packageId); + return null; + } + }), + ); + const blocks: string[] = []; + const provides: CapabilityProvidedGameSystems = {}; + for (const contribution of settled) { + if (contribution === null || contribution === undefined) continue; + const text = typeof contribution === "string" ? contribution : contribution.text; + if (typeof text === "string" && text.trim().length > 0) blocks.push(text.trim()); + if (typeof contribution === "object" && contribution.provides?.inventory === true) { + provides.inventory = true; + } + } + return { blocks, provides }; }🤖 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/server/src/services/capability-packages/capability-prompt-context.service.ts` around lines 84 - 107, Update collectCapabilityPromptContext to invoke all contributors concurrently by mapping contributorsByPackage entries to promises and awaiting them with Promise.all, while retaining each contribution’s withDeadline and non-fatal error handling. Process the resolved results afterward in registration order so blocks remain deterministically composed and provides aggregation is unchanged.packages/shared/src/schemas/capability-package.schema.ts (1)
46-64: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate
supportedCapabilityApifor the new capability surface.
supportedCapabilityApistill reports1.7, but this release addsgame-surface,prompt-contextconsumption, and optional resource write methods. A schemaVersion-2 package cannot declare the new capability API withoutgetCapabilityApiCompatibilityIssuefailing for a host that supports it, or it must under-declare and skip the guard. Bump the minor version to the next supported release.🤖 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/shared/src/schemas/capability-package.schema.ts` around lines 46 - 64, Update the supportedCapabilityApi version from 1.7 to the next minor release compatible with the added game-surface, prompt-context, and optional resource write capabilities. Locate the supportedCapabilityApi declaration and preserve the existing schema-version compatibility logic.packages/client/src/components/game/NewGameExperienceChooser.tsx (1)
199-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the existing localization key for the fallback title.
Line 226 falls back to the hardcoded string
"New Game". Every other string in this panel useslocalizeUi.GameSetupWizard.tsxuseslocalizeUi("navigation.chatSidebar.new.game")for the identical fallback title in its own dialog header. Use the same key here for consistency and to keep the string translatable.🌐 Proposed fix for the localization gap
- <h3 id="game-experience-setup-title" className={NEUTRAL_PANEL_TITLE}> - {activeExperience?.manifest.name ?? "New Game"} - </h3> + <h3 id="game-experience-setup-title" className={NEUTRAL_PANEL_TITLE}> + {activeExperience?.manifest.name ?? localizeUi("navigation.chatSidebar.new.game")} + </h3>The Escape handling and dialog semantics (
role="dialog",aria-modal,aria-labelledby) that a past review requested are correctly implemented here. Based on learnings,packages/client/src/localization/locales/en.jsonis the authoritative source for UI localization keys, andnavigation.chatSidebar.new.gameis already the established key for this exact string.🤖 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/client/src/components/game/NewGameExperienceChooser.tsx` around lines 199 - 254, Update the fallback title in the dialog header rendered by the active experience branch to use localizeUi("navigation.chatSidebar.new.game") instead of the hardcoded "New Game", matching GameSetupWizard.tsx while preserving the existing activeExperience manifest name when available.Source: Learnings
packages/client/src/components/game/GameSurface.tsx (1)
2161-2184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict the cold-cache fallback for
experienceOwnsGame.
installedCapabilityPackagesexposes React Query state, butexperienceOwnsGameusesgameExperienceId !== nullfor the whole installed-package query lifecycle. If the owning package later becomes inactive/missing after loading,experienceOwnsGamestays true whileexperienceSurfaceActivestaysfalse, permanently suppressing the built-in tutorial and session-panel help. Track a dedicated “installed-packages still loading” flag so the broader check only bridges the initial fetch.🤖 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/client/src/components/game/GameSurface.tsx` around lines 2161 - 2184, Update the experienceOwnsGame logic near useInstalledCapabilityPackages so the gameExperienceId fallback applies only while the installed-capability-packages query is still loading, using its React Query loading state. Once loading completes, rely exclusively on experienceSurfaceActive so inactive or missing packages no longer suppress built-in help.
🤖 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 `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 2161-2184: Update the experienceOwnsGame logic near
useInstalledCapabilityPackages so the gameExperienceId fallback applies only
while the installed-capability-packages query is still loading, using its React
Query loading state. Once loading completes, rely exclusively on
experienceSurfaceActive so inactive or missing packages no longer suppress
built-in help.
In `@packages/client/src/components/game/NewGameExperienceChooser.tsx`:
- Around line 199-254: Update the fallback title in the dialog header rendered
by the active experience branch to use
localizeUi("navigation.chatSidebar.new.game") instead of the hardcoded "New
Game", matching GameSetupWizard.tsx while preserving the existing
activeExperience manifest name when available.
In
`@packages/server/src/services/capability-packages/capability-prompt-context.service.ts`:
- Around line 84-107: Update collectCapabilityPromptContext to invoke all
contributors concurrently by mapping contributorsByPackage entries to promises
and awaiting them with Promise.all, while retaining each contribution’s
withDeadline and non-fatal error handling. Process the resolved results
afterward in registration order so blocks remain deterministically composed and
provides aggregation is unchanged.
In `@packages/shared/src/schemas/capability-package.schema.ts`:
- Around line 46-64: Update the supportedCapabilityApi version from 1.7 to the
next minor release compatible with the added game-surface, prompt-context, and
optional resource write capabilities. Locate the supportedCapabilityApi
declaration and preserve the existing schema-version compatibility logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f1cff59-e2c2-4176-a4b2-3f9cf2f266f4
📒 Files selected for processing (14)
packages/client/src/components/game/GameNarration.tsxpackages/client/src/components/game/GameSetupWizard.tsxpackages/client/src/components/game/GameSurface.tsxpackages/client/src/components/game/NewGameExperienceChooser.tsxpackages/server/src/app.tspackages/server/src/routes/game.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/services/capability-packages/capability-module-runtime.service.tspackages/server/src/services/capability-packages/capability-prompt-context.service.tspackages/server/src/services/capability-packages/capability-resources.service.tspackages/server/src/services/game/gm-prompts.tspackages/shared/src/schemas/capability-package.schema.tspackages/shared/src/types/capability-runtime.tspackages/shared/src/types/game.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/development/optional-agent-packages.md`:
- Line 24: Update the prompt-context description to state that contributions are
added to the system prompt of each generated Game turn, not every turn type. In
the same description, state that contributors are logged and skipped when they
throw or fail to settle within the two-second deadline, while preserving the
existing behavior for contributors that return nothing.
In `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 10277-10284: Update the setup flow around NewGameExperienceChooser
and its experience-owned gameSetup.mutateAsync handling so JSON-repairable
failures are forwarded to handleJsonRepairError, allowing GameJsonRepairModal to
render for experience setups. Ensure useGameSetup does not stop the repair
request at its info-toast handling path, while preserving the existing
classicSetup behavior.
- Around line 11725-11737: Update the choicesSlot ternary in GameSurface so
experienceSurfaceActive takes precedence over the activeChoices && narrationDone
Classic branch, keeping the ExperienceChoiceSlot anchor mounted whenever the
experience surface is active. Alternatively, add and propagate a providesChoices
declaration alongside providesPlayerInput in ExperienceChromeDeclaration, then
suppress Classic GameChoiceCards when that declaration is enabled; ensure
experience-owned menus retain a stable portal target even when shared narration
emits choices.
In `@packages/client/src/components/game/NewGameExperienceChooser.tsx`:
- Around line 24-32: Move the game-surface package eligibility helper from
GameSurface.tsx into the capability-package helpers module and export it for
reuse. Update NewGameExperienceChooser to use that shared helper with
useInstalledCapabilityPackages, removing the local InstalledPkg type and any
cast; preserve filtering for active packages whose manifest contributions
include the "game-surface" slot.
In
`@packages/server/src/services/capability-packages/capability-prompt-context.service.ts`:
- Around line 61-107: The optional improvement is to unref the timeout created
in withDeadline so it cannot keep the Node.js event loop alive during shutdown.
Update the timer setup in withDeadline while preserving its existing rejection
and finally-based cleanup behavior.
In `@packages/server/src/services/game/gm-prompts.ts`:
- Around line 629-630: Update the persona-stats inventory tracking flow to honor
experienceProvidedSystems.inventory (or the existing experienceOwnsInventory
helper) when an installed experience owns inventory. Skip the inventory prompt
tracking and playerStats.inventory updates in that case, while preserving
current behavior when inventory remains built-in.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efa462a5-55c9-4a95-a78f-b8df430d6c26
📒 Files selected for processing (17)
CHANGELOG.mddocs/development/optional-agent-packages.mdpackages/client/src/components/game/GameNarration.tsxpackages/client/src/components/game/GameSetupWizard.tsxpackages/client/src/components/game/GameSurface.tsxpackages/client/src/components/game/NewGameExperienceChooser.tsxpackages/client/src/localization/locales/en.jsonpackages/server/src/app.tspackages/server/src/routes/game.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/services/capability-packages/capability-module-runtime.service.tspackages/server/src/services/capability-packages/capability-prompt-context.service.tspackages/server/src/services/capability-packages/capability-resources.service.tspackages/server/src/services/game/gm-prompts.tspackages/shared/src/schemas/capability-package.schema.tspackages/shared/src/types/capability-runtime.tspackages/shared/src/types/game.ts
experienceOwnsGame stayed true for the whole life of a game created with an experience, because gameExperienceId lives in the chat metadata and never clears. If the package was later uninstalled the surface stopped mounting but the flag did not, so the game lost the experience UI and the built-in help button and tutorial with it. The broad reading now applies only while the installed-packages query is still in flight, which is the cold-cache flash it was written for; once the query settles the flag follows the surface. Use the existing navigation.chatSidebar.new.game key for the setup panel title instead of a hardcoded English fallback, matching GameSetupWizard. Document the exported capability input types, the contributor type and the new deadline helper.
Add providesChoices to the chrome declaration. The choices slot gave the Classic branch priority, so a turn whose narration emitted choices rendered the built-in cards over an experience that has its own menu, and unmounted the in-flow anchor that menu is portaled into. Avoiding that was left to the package's prompt discipline, which the host should not depend on. Declaring it is opt-in: an experience that says nothing keeps the Classic cards exactly as before. Route setup failures through the JSON repair modal on the experience path. The modal only mounted inside the built-in wizard, so a malformed opening response was repairable in one setup path and a dead end in the other. It now mounts outside the chooser, and a launch failure is offered to the host before it is rethrown. Share the game-experience eligibility filter between GameSurface and the chooser instead of spelling it out twice, which also drops a local type and its cast. Unref the contributor deadline so a pending guard cannot hold the event loop open, and correct the prompt-context paragraph in the package docs: it applies to Game turns, and a contributor that exceeds its deadline is skipped the same way one that throws is.
Found while playtesting the experience surface: choices offered by one game were still on screen in the next one, and answerable there, against a GM that never asked them. An active QTE leaks the same way, and worse, since it carries a timer that then fires in the new game. Neither is caused by this PR. Every site that clears them is driven by a player action or by the session concluding, and the effect keyed on the scope resets about twenty other pieces of turn state without touching these two. Switching between two ordinary games has always done this. It surfaces here because an experience draws its own menu, so the stale cards render beside it and read as the new mode double-listing its options. Fixed in the effect that already owns this reset rather than filed separately, since resetting the experience seam state there while leaving these two leaking would be arbitrary. This does change Classic behaviour, which the PR description now says explicitly.
|
Second round. Three more commits: the four remaining items from the first pass, everything from the full From the full review
JSON repair on the experience path. Confirmed and fixed. The modal only mounted inside the built-in The Found while playtesting — pre-existing, fixed hereChoices offered by one game were still on screen in the next one and answerable there, against a GM that This is not caused by this PR. Every site that clears either one is driven by a player action or by the I fixed it in the effect that already owns that reset rather than filing it separately, since resetting Still open for youThe capability API version. Still at 1.7, still deliberately not bumped. The index signature on Running contributors concurrently. Still sequential. They compose one prompt, so block order has to be The client-side inventory parser. Left as is, and I checked the consequence rather than assuming it. Validation
Beyond that I exercised the parts that types cannot cover: the contributor deadline against the built @coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata ==='
gh pr view 4528 --repo Pasta-Devs/Marinara-Engine --json baseRefName,headRefName,commits,files --jq '{baseRefName, headRefName, commitCount:(.commits|length), files:[.files[].path]}'
echo '=== Changed files and diff stat ==='
BASE="$(git merge-base "origin/$(gh pr view 4528 --repo Pasta-Devs/Marinara-Engine --json baseRefName --jq .baseRefName)" HEAD)"
git diff --stat "$BASE"...HEAD
git diff --name-only "$BASE"...HEAD
echo '=== Outlines for changed TypeScript/TSX files ==='
git diff --name-only "$BASE"...HEAD \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
done
echo '=== Current diff ==='
git diff --find-renames --find-copies --unified=80 "$BASE"...HEADLength of output: 2301 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Local refs ==='
git branch -a
git remote -v
echo '=== PR patch ==='
gh pr diff 4528 --repo Pasta-Devs/Marinara-Engine --patch
echo '=== Changed-file outlines ==='
for file in \
packages/client/src/components/game/GameSurface.tsx \
packages/client/src/components/game/NewGameExperienceChooser.tsx \
packages/client/src/hooks/use-capability-packages.ts \
packages/server/src/routes/generate.routes.ts \
packages/server/src/services/capability-packages/capability-prompt-context.service.ts \
packages/server/src/services/capability-packages/capability-module-runtime.service.ts \
packages/server/src/services/game/gm-prompts.ts \
packages/shared/src/schemas/capability-package.schema.ts \
packages/shared/src/types/capability-runtime.ts \
packages/server/src/routes/game.routes.ts; do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
doneLength of output: 167171 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Prompt-context implementation and registrations ==='
sed -n '1,150p' packages/server/src/services/capability-packages/capability-prompt-context.service.ts
rg -n -C 5 'registerPromptContext|collectCapabilityPromptContext|CapabilityPromptContextContributor' \
packages/server packages/shared
echo '=== QTE and choice state: initialization, timer, cleanup, scope reset ==='
rg -n -C 8 'activeQte|queuedQte|setActiveChoices|setActiveQte|setQueuedQte|sceneRuntimeScopeKey' \
packages/client/src/components/game/GameSurface.tsx
echo '=== Experience setup launch and JSON repair handler ==='
sed -n '1,310p' packages/client/src/components/game/NewGameExperienceChooser.tsx
rg -n -C 10 'handleJsonRepairError|jsonRepairRequest|GameJsonRepairModal|useGameSetup' \
packages/client/src/components/game/GameSurface.tsx packages/client/src/hooks packages/client/src/lib
echo '=== Experience package query behavior ==='
sed -n '1,90p' packages/client/src/hooks/use-capability-packages.ts
rg -n -C 5 'useInstalledCapabilityPackages|capabilityPackageKeys\.installed|installed\(' \
packages/client/src
echo '=== Capability runtime public API and manifest compatibility ==='
sed -n '35,195p' packages/server/src/services/capability-packages/capability-module-runtime.service.ts
sed -n '1,155p' packages/shared/src/schemas/capability-package.schema.ts
sed -n '1,145p' packages/shared/src/types/capability-runtime.tsLength of output: 136299 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== CapabilityElement behavior and client-module loader ==='
ast-grep outline packages/client/src/components/capabilities/CapabilityElement.tsx --items all
sed -n '1,300p' packages/client/src/components/capabilities/CapabilityElement.tsx
sed -n '150,315p' packages/client/src/hooks/use-capability-packages.ts
echo '=== Capability package readiness and entrypoint validation ==='
rg -n -C 8 'entrypoints\.client|clientEntrypoint|runtimeBlockReason|markRuntimeStatus|isInstalledCapabilityReady' \
packages/server/src/services/capability-packages packages/shared/src
echo '=== Prompt-context tests and deadline tests ==='
fd -t f . packages/server | grep -E '(test|spec)' | while IFS= read -r file; do
rg -n -C 4 'collectCapabilityPromptContext|registerCapabilityPromptContext|prompt-context|CONTRIBUTOR_TIMEOUT' "$file" && echo "--- $file ---"
done || true
echo '=== Existing tests for game-surface selection ==='
fd -t f . packages/client packages/shared | grep -E '(test|spec)' | while IFS= read -r file; do
rg -n -C 4 'game-surface|selectGameExperiencePackages|NewGameExperienceChooser' "$file" && echo "--- $file ---"
done || trueLength of output: 43393
✅ Action performedFull review 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)
packages/shared/src/types/capability-runtime.ts (1)
116-125: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope capability resource writes to package-owned records.
updatePersona,updateLorebook,bulkCreateLorebookEntries, andremoveLorebookEntryaccept raw IDs and pass them directly to storage inpackages/server/src/services/capability-packages/capability-resources.service.ts. A privileged package can update or delete unrelated user-created data. Enforce ownership for these writes; if any packages are intentionally trusted with global write access, document the privilege and add boundary tests.🤖 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/shared/src/types/capability-runtime.ts` around lines 116 - 125, Scope the write methods in CapabilityResourcesService—updatePersona, updateLorebook, bulkCreateLorebookEntries, and removeLorebookEntry—to package-owned records before invoking storage, rejecting or ignoring unrelated user-owned IDs. Reuse the existing package ownership or authorization mechanism where available, and document plus test any intentionally trusted package with global write access.
🤖 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 `@packages/shared/src/types/capability-runtime.ts`:
- Around line 116-125: Scope the write methods in
CapabilityResourcesService—updatePersona, updateLorebook,
bulkCreateLorebookEntries, and removeLorebookEntry—to package-owned records
before invoking storage, rejecting or ignoring unrelated user-owned IDs. Reuse
the existing package ownership or authorization mechanism where available, and
document plus test any intentionally trusted package with global write access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d9d0b1c-0cbb-4e10-89f7-4274f91e9a22
📒 Files selected for processing (6)
docs/development/optional-agent-packages.mdpackages/client/src/components/game/GameSurface.tsxpackages/client/src/components/game/NewGameExperienceChooser.tsxpackages/client/src/hooks/use-capability-packages.tspackages/server/src/services/capability-packages/capability-prompt-context.service.tspackages/shared/src/types/capability-runtime.ts
…t entrypoint The deadline only raced a native Promise. A contributor returning any other thenable fell through to Promise.resolve, which ADOPTS it, so one that never settled slipped past the guard and hung the turn -- the exact failure the deadline exists to prevent. Anything that can defer now races it; only a primitive is handed straight back, so the common synchronous return still costs no timer. Require entrypoints.client on a manifest declaring the game-surface slot. The module loader skips a package without one, so it could be offered in the setup wizard and then render neither its setup view nor its surface. Rejected at manifest validation, with a matching check where the chooser and GameSurface resolve experiences, for anything installed before the rule existed.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/shared/src/schemas/capability-package.schema.ts (1)
39-64: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire
entrypoints.clientwhen a package declaresgame-surface.
contributions.slotsaccepts"game-surface"independently ofentrypoints.client, and.extend()is applied to the base schema before the exported discriminated union. Add the cross-field check to the exportedcapabilityPackageManifestSchemaunion so installed packages with a game surface cannot pass manifest validation without a client entry point. Also ensure client-side experience selection rejects or hides installed packages that declaregame-surfacewithoutentrypoints.client.🤖 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/shared/src/schemas/capability-package.schema.ts` around lines 39 - 64, Require an `entrypoints.client` value whenever `contributions.slots` includes `"game-surface"` by adding the cross-field validation to the exported `capabilityPackageManifestSchema` discriminated union, after the base schema extension. Update client-side experience selection to reject or hide installed manifests declaring `"game-surface"` without `entrypoints.client`, using the existing selection logic and symbols.packages/server/src/routes/game.routes.ts (1)
1676-1703: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the classic game fields optional for experience-driven creation.
createGameSchemastill usesgameSetupConfigSchema, which requiresgenre,setting,tone,difficulty,gmMode, andpartyCharacterIds;NewGameExperienceChooseronly stampsgameExperienceId, so packages that do not populate those fields will fail at/game/create. Relax or provide defaults for those fields whengameExperienceIdis present.🤖 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/server/src/routes/game.routes.ts` around lines 1676 - 1703, The gameSetupConfigSchema requires classic game fields even for experience-driven creation. Update createGameSchema and its gameSetupConfigSchema integration so genre, setting, tone, difficulty, gmMode, and partyCharacterIds are optional or receive appropriate defaults when gameExperienceId is present, while preserving required validation for classic creation; packages/client/src/components/game/NewGameExperienceChooser.tsx requires no direct change because it only supplies gameExperienceId.packages/client/src/components/game/GameSurface.tsx (1)
11669-11681: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate the mobile HUD widget slot behind
experienceSurfaceActive.The desktop HUD widget rail excludes
experienceSurfaceActive(Line 12247), butmobileWidgetSlotdoes not. It rendersMobileWidgetPanelwheneverhudWidgets.length > 0, with no check for an active experience surface. The compact "MobileChoiceStage" widget rails (Lines 11699-11719) have the same gap.
hudWidgetscan stay populated for an experience-owned game, because[widget:]tags from the GM model are applied unconditionally (Line 4717-4723). As a result, a mobile player can see the built-in HUD widgets overlapping the package's own HUD, even though the package is supposed to fully replace it. Desktop players do not see this because the rail already excludesexperienceSurfaceActive.Add the same
!experienceSurfaceActivecondition tomobileWidgetSlotand to the compact widget rails.🐛 Proposed fix
const mobileWidgetSlot = - !combatUiActive && hudWidgets.length > 0 && !(compactHudWidgets && choicesVisible) ? ( + !combatUiActive && + !experienceSurfaceActive && + hudWidgets.length > 0 && + !(compactHudWidgets && choicesVisible) ? (🤖 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/client/src/components/game/GameSurface.tsx` around lines 11669 - 11681, Add !experienceSurfaceActive to the mobileWidgetSlot visibility condition and to both compact MobileChoiceStage widget rail conditions. Preserve the existing combat, widget-count, and choicesVisible checks while ensuring no built-in mobile HUD widgets render when an experience surface is active.
🤖 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/client/src/components/game/NewGameExperienceChooser.tsx`:
- Around line 85-96: Normalize setupConfig to a non-null object before accessing
promptPresetId in the onLaunch flow, rather than relying on the direct cast to {
promptPresetId?: string }. Preserve the existing gameExperienceId merge and pass
promptPresetId from the normalized object, ensuring null or other non-object
setupConfig values do not cause a property-access throw.
In `@packages/client/src/hooks/use-capability-packages.ts`:
- Around line 52-63: Update selectGameExperiencePackages to require a client
entry point in addition to active status and the game-surface slot. Check
manifest.entrypoints.client in the existing filter so only packages that
ExperienceOwnsGame can mount are returned.
In
`@packages/server/src/services/capability-packages/capability-prompt-context.service.ts`:
- Around line 71-84: Update withDeadline to detect promise-like values via a
callable .then property instead of instanceof Promise, so non-native thenables
are normalized and included in the timeout race. Preserve immediate resolution
for ordinary non-thenable values, and add a regression test covering a
non-native thenable that never settles and must reject after
CONTRIBUTOR_TIMEOUT_MS.
---
Outside diff comments:
In `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 11669-11681: Add !experienceSurfaceActive to the mobileWidgetSlot
visibility condition and to both compact MobileChoiceStage widget rail
conditions. Preserve the existing combat, widget-count, and choicesVisible
checks while ensuring no built-in mobile HUD widgets render when an experience
surface is active.
In `@packages/server/src/routes/game.routes.ts`:
- Around line 1676-1703: The gameSetupConfigSchema requires classic game fields
even for experience-driven creation. Update createGameSchema and its
gameSetupConfigSchema integration so genre, setting, tone, difficulty, gmMode,
and partyCharacterIds are optional or receive appropriate defaults when
gameExperienceId is present, while preserving required validation for classic
creation; packages/client/src/components/game/NewGameExperienceChooser.tsx
requires no direct change because it only supplies gameExperienceId.
In `@packages/shared/src/schemas/capability-package.schema.ts`:
- Around line 39-64: Require an `entrypoints.client` value whenever
`contributions.slots` includes `"game-surface"` by adding the cross-field
validation to the exported `capabilityPackageManifestSchema` discriminated
union, after the base schema extension. Update client-side experience selection
to reject or hide installed manifests declaring `"game-surface"` without
`entrypoints.client`, using the existing selection logic and symbols.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fabb6466-a3a7-41cb-93e2-388fa208f58b
📒 Files selected for processing (18)
CHANGELOG.mddocs/development/optional-agent-packages.mdpackages/client/src/components/game/GameNarration.tsxpackages/client/src/components/game/GameSetupWizard.tsxpackages/client/src/components/game/GameSurface.tsxpackages/client/src/components/game/NewGameExperienceChooser.tsxpackages/client/src/hooks/use-capability-packages.tspackages/client/src/localization/locales/en.jsonpackages/server/src/app.tspackages/server/src/routes/game.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/services/capability-packages/capability-module-runtime.service.tspackages/server/src/services/capability-packages/capability-prompt-context.service.tspackages/server/src/services/capability-packages/capability-resources.service.tspackages/server/src/services/game/gm-prompts.tspackages/shared/src/schemas/capability-package.schema.tspackages/shared/src/types/capability-runtime.tspackages/shared/src/types/game.ts
…nfig defensively The desktop widget rail was gated on the experience surface but the mobile slot and the compact rails beside the choice stage were not, so a package that draws its own HUD could still have the built-in widgets rendered over it on a narrow viewport. All three render paths now share the same condition. Read the setup config the package returns defensively as well: it is package-supplied, so a null or non-object value threw on property access here instead of failing validation with a message the player can act on.
|
Both rounds addressed. Five of the seven findings are fixed; notes on the other two below. The deadline now covers thenables. This was the worst of the batch and worth saying plainly: the guard Verified against the built service, since types cannot catch this: a thenable that never settles is dropped A game-surface package must declare a client entrypoint. Correct — the module loader skips a package The built-in HUD widgets are now hidden on mobile too. A real gap on my side: I gated the desktop rail The package's setup config is read defensively. Also fair — it is package-supplied, so a null or Not appliedRelaxing the classic fields in An experience runs its own setup wizard and returns a complete Scoping the resource writes to package-owned records. The underlying concern is real, so it is worth A package's There is also no ownership model to reuse: nothing associates a persona or a lorebook with the package that So packages are broadly privileged by design today, and that is an engine-wide property rather than
@coderabbitai the last two items are deliberate and explained above — please take them into account rather |
|
Tip For best results, initiate chat on the files or code changes.
The thenable deadline and the I accept the two deliberate decisions for this PR:
I will not re-raise either item in this PR. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/client/src/components/game/NewGameExperienceChooser.tsx (2)
58-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFall back when the selected experience is no longer eligible.
activeIdcan outliveexperiences. If the selected package becomes inactive or uninstalled,activeExperiencebecomesnull, but the!activeIdguard still selects package mode and mountsCapabilityElementwith the stale package ID.Derive a validated selected ID from
experiencesand use it for rendering andgameExperienceId, or clearactiveIdwhen the package disappears.Proposed stale-selection guard
+ const selectedExperienceId = + activeId && experiences.some((experience) => experience.id === activeId) ? activeId : null; - const activeExperience = experiences.find((e) => e.id === activeId) ?? null; + const activeExperience = experiences.find((e) => e.id === selectedExperienceId) ?? null;Use
selectedExperienceIdin the Classic/package branch,CapabilityElement, and launch payload.🤖 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/client/src/components/game/NewGameExperienceChooser.tsx` around lines 58 - 60, Derive a validated selected experience ID from `experiences` by retaining `activeId` only when it matches an eligible package; otherwise fall back to the non-package mode. Update the Classic/package rendering condition, `CapabilityElement` props, and launch payload to use this validated ID instead of the stale `activeId`, while preserving `activeExperience` lookup behavior.
117-120: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRefresh resource caches when setup fails.
These invalidations run only after
gameSetup.mutateAsyncsucceeds. A package can write personas or lorebooks before callingonLaunch; if opening generation fails, the catch block rethrows before these calls run. The JSON repair path can then complete while the persona cache still contains old data.Move the invalidations into a
finallythat covers the full launch. Await them without masking the original setup error.🤖 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/client/src/components/game/NewGameExperienceChooser.tsx` around lines 117 - 120, Update the launch flow around gameSetup.mutateAsync and the existing persona/lorebook invalidateQueries calls so both cache invalidations run in a finally covering the full launch, including setup and generation failures. Await the invalidation promises while preserving and rethrowing the original setup error if one occurs.
♻️ Duplicate comments (1)
packages/client/src/components/game/GameSurface.tsx (1)
6435-6439: 🔒 Security & Privacy | 🟡 MinorReject raw package background values.
The previous review flagged this fallback, and it is still present. When the asset-map lookup fails, the package-controlled value reaches
displayedBackground,DirectionEngine.backgroundUrl, and the experience underlay unchanged.This permits browser-side arbitrary resource loading and weakens the CSS URL trust boundary. It is not server-side SSRF, but it still exposes users to unvalidated package-controlled URLs. Resolve only manifest-backed assets or enforce a strict allowlist. Do not pass the raw fallback.
🤖 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/client/src/components/game/GameSurface.tsx` around lines 6435 - 6439, Update pushExperienceBackground so package-provided background values are used only when they resolve through scopedAssetMap to a manifest-backed path; remove the raw value fallback and set the background to null for unresolved entries. Preserve null handling and ensure displayedBackground, DirectionEngine.backgroundUrl, and the experience underlay receive no unvalidated package-controlled URL.
🤖 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/client/src/hooks/use-capability-packages.ts`:
- Around line 61-68: Update selectGameExperiencePackages to require
isInstalledCapabilityReady(pkg) in addition to the existing active status,
game-surface contribution, and client entrypoint checks, matching the
client-module loader’s eligibility criteria.
In `@packages/shared/src/schemas/capability-package.schema.ts`:
- Line 146: Update the manifest validation condition around the client
entrypoint check to reject missing or whitespace-only
`manifest.entrypoints.client` values when the `game-surface` slot is declared.
Trim the value before validating it while preserving acceptance of non-blank
client module identifiers.
---
Outside diff comments:
In `@packages/client/src/components/game/NewGameExperienceChooser.tsx`:
- Around line 58-60: Derive a validated selected experience ID from
`experiences` by retaining `activeId` only when it matches an eligible package;
otherwise fall back to the non-package mode. Update the Classic/package
rendering condition, `CapabilityElement` props, and launch payload to use this
validated ID instead of the stale `activeId`, while preserving
`activeExperience` lookup behavior.
- Around line 117-120: Update the launch flow around gameSetup.mutateAsync and
the existing persona/lorebook invalidateQueries calls so both cache
invalidations run in a finally covering the full launch, including setup and
generation failures. Await the invalidation promises while preserving and
rethrowing the original setup error if one occurs.
---
Duplicate comments:
In `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 6435-6439: Update pushExperienceBackground so package-provided
background values are used only when they resolve through scopedAssetMap to a
manifest-backed path; remove the raw value fallback and set the background to
null for unresolved entries. Preserve null handling and ensure
displayedBackground, DirectionEngine.backgroundUrl, and the experience underlay
receive no unvalidated package-controlled URL.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a537434a-da1e-4f2e-8195-efc07c41d884
📒 Files selected for processing (5)
packages/client/src/components/game/GameSurface.tsxpackages/client/src/components/game/NewGameExperienceChooser.tsxpackages/client/src/hooks/use-capability-packages.tspackages/server/src/services/capability-packages/capability-prompt-context.service.tspackages/shared/src/schemas/capability-package.schema.ts
…emembered id Two findings from the last pass, both in the setup chooser. The selected id outlived the package it named: uninstalling or deactivating an experience while its setup panel is open left activeId set, so the panel still mounted a capability element for a package that was no longer there. Rendering and the id stamped onto the game now come from the resolved experience, so an unresolvable selection falls back to the built-in wizard. Move the persona and lorebook cache invalidations into a finally. A package can write both before it ever calls the host launcher, so a failed opening generation left those records in storage with nothing on the client aware of them.
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)
packages/client/src/components/game/NewGameExperienceChooser.tsx (1)
96-124: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInvalidate resource queries when game creation fails.
If
createGame.mutateAsyncrejects on Line 96, execution never enters thetryblock on Line 105. Persona and lorebook writes made beforeonLaunchthen remain stale in the client cache.Wrap game creation in the outer
try/finally. KeeponSetupErrorscoped to the opening-generation request.Proposed fix
- const res = await createGame.mutateAsync({ - name: gameName, - setupConfig: { ...cfg, gameExperienceId: selectedId } as unknown, - preferences: "", - chatId: activeChatId, - connectionId, - promptPresetId, - } as Parameters<typeof createGame.mutateAsync>[0]); - const chatId = res.sessionChat.id; try { - await gameSetup.mutateAsync({ - chatId, + const res = await createGame.mutateAsync({ + name: gameName, + setupConfig: { ...cfg, gameExperienceId: selectedId } as unknown, + preferences: "", + chatId: activeChatId, connectionId, - preferences: "", - promptPresetId: promptPresetId ?? null, + promptPresetId, } as Parameters<typeof gameSetup.mutateAsync>[0]); - } catch (error) { - onSetupError(error); - throw error; + const chatId = res.sessionChat.id; + try { + await gameSetup.mutateAsync({ + chatId, + connectionId, + preferences: "", + promptPresetId: promptPresetId ?? null, + } as Parameters<typeof gameSetup.mutateAsync>[0]); + } catch (error) { + onSetupError(error); + throw error; + } + return chatId; } finally { queryClient.invalidateQueries({ queryKey: characterKeys.personas }); queryClient.invalidateQueries({ queryKey: lorebookKeys.all }); } - return chatId;🤖 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/client/src/components/game/NewGameExperienceChooser.tsx` around lines 96 - 124, Wrap the createGame.mutateAsync call and subsequent setup flow in an outer try/finally so persona and lorebook query invalidation always runs when game creation or setup fails. Keep onSetupError confined to the gameSetup.mutateAsync catch block, while preserving rethrow behavior and the existing resource query keys.
🤖 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 `@packages/client/src/components/game/NewGameExperienceChooser.tsx`:
- Around line 96-124: Wrap the createGame.mutateAsync call and subsequent setup
flow in an outer try/finally so persona and lorebook query invalidation always
runs when game creation or setup fails. Keep onSetupError confined to the
gameSetup.mutateAsync catch block, while preserving rethrow behavior and the
existing resource query keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ddf8d53d-2ab1-4e0b-886f-44c005b49bdb
📒 Files selected for processing (1)
packages/client/src/components/game/NewGameExperienceChooser.tsx
…s too The finally only wrapped the opening generation, so a rejection from game creation skipped it entirely. A package can write the player persona and a lorebook before it ever calls the host launcher, which means either step failing left those records in storage with nothing on the client aware of them. The block now spans both, with the repair routing still scoped to generation. Document the named seam helpers while here.
It was mine, not the review's, and it was wrong. Game creation moves the active chat to the new session, so keying the chooser on it remounted the component in the middle of the launch it was driving, clearing the selection and unmounting the setup mid-flight. That is worse than the stale selection it was meant to prevent, which needs the player to switch chats during setup and is already handled by resolving the id rather than remembering it.
The seam discarded the whole update when the speaker map was empty, so an experience that has nothing to say about its speakers could not give the player's own dialogue a face. Either half now counts as a real push.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/client/src/components/game/GameSurface.tsx (4)
3475-3480: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve a player-only avatar when no persona is selected.
effectivePersonaInfoinjectsplayerAvatarUrlonly whenpersonaInfoexists. This component explicitly supports chats without a selected persona and creates apersona:defaultplayer. In that case, the retained player-only package update is not passed toGameNarration.Provide a default player avatar entry or pass the fallback through the no-persona player contract.
🤖 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/client/src/components/game/GameSurface.tsx` around lines 3475 - 3480, Update effectivePersonaInfo and the no-persona player contract used by GameNarration so playerAvatarUrl is preserved when personaInfo is undefined. Provide the fallback as the default player avatar entry or pass it through the existing no-persona path, while retaining the current persona avatar override behavior.
2164-2184: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefer experience-sensitive work while package resolution is pending.
experienceOwnsGamecovers the cold-cache state, but later gates still useexperienceSurfaceActive. WhileuseInstalledCapabilityPackagesis pending, Classic map, party, widget, choice, and input UI can render before the package surface mounts.sceneAnalysisEnabledcan also be false for an experience whosechatMeta.enableAgentsis false.If a new assistant turn is processed during this window,
processSceneReftakes the inline path and setslastProcessedMsgRef. Resolving the package later does not process that turn again.Defer scene processing until experience resolution settles. Use the pending ownership state for Classic UI suppression, then restore Classic fallback only when the query settles without the selected package.
🤖 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/client/src/components/game/GameSurface.tsx` around lines 2164 - 2184, Defer experience-sensitive scene processing until installed-package resolution settles, using experienceOwnsGame rather than experienceSurfaceActive for Classic map, party, widget, choice, and input UI suppression. Update processSceneRef so pending experience resolution does not take the inline path or set lastProcessedMsgRef, ensuring the turn is processed after the package mounts. Also prevent scene processing when the selected experience has scene analysis disabled by chatMeta.enableAgents, and restore Classic fallback only once the query settles without the selected package.
8336-8340: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftApply inventory and combat declarations to every Classic path.
These lines remove only the narration open/start callbacks.
GameCombatUIstill receivesonOpenInventory={() => setInventoryOpen(true)}at Line 11864, and thecombatUiActivebranch at Line 11806 still mounts Classic combat after a GM encounter or state tag.A package that declares
providesInventoryorprovidesCombatcan therefore still open or display the corresponding Classic system. Gate the Classic overlays, combat branch, queued encounter/generation flow, and every Classic callback when the declaration is true. Clear an already-open Classic inventory when ownership becomes true.🤖 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/client/src/components/game/GameSurface.tsx` around lines 8336 - 8340, Apply activeExperienceChrome.providesInventory and providesCombat consistently across every Classic path in GameSurface, not only classicInventoryOpener and classicCombatStarter. Gate GameCombatUI callbacks, the combatUiActive branch, Classic overlays, and queued encounter or generation flows when the corresponding declaration is true, while preserving custom-provided behavior. When inventory ownership becomes true, close any already-open Classic inventory.
6443-6447: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve pushed experience backgrounds lazily.
Store the raw pushed value from
setExperienceBackgroundTag, then deriveexperienceBackgroundUrlfrom the latestscopedAssetMapbefore it is used bydisplayedBackground. A later manifest refresh can add the asset afterpushExperienceBackgroundhas already captured the unresolved raw value.🤖 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/client/src/components/game/GameSurface.tsx` around lines 6443 - 6447, Update pushExperienceBackground and the surrounding GameSurface state so setExperienceBackgroundTag stores the raw pushed value instead of resolving it immediately. Derive experienceBackgroundUrl from that raw value and the latest scopedAssetMap before displayedBackground consumes it, preserving direct asset paths and null handling while allowing later manifest refreshes to resolve previously unknown tags.
🤖 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/client/src/components/game/GameSurface.tsx`:
- Around line 3405-3412: Update setExperienceSpeakerAvatars to validate
speakerAvatars as an iterable map-like collection whose entries are iterable
[string, { url: string }] pairs before storing it. Reject malformed collections
or entries, including objects that only expose size, by storing null; preserve
acceptance of valid playerAvatarUrl-only pushes.
---
Outside diff comments:
In `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 3475-3480: Update effectivePersonaInfo and the no-persona player
contract used by GameNarration so playerAvatarUrl is preserved when personaInfo
is undefined. Provide the fallback as the default player avatar entry or pass it
through the existing no-persona path, while retaining the current persona avatar
override behavior.
- Around line 2164-2184: Defer experience-sensitive scene processing until
installed-package resolution settles, using experienceOwnsGame rather than
experienceSurfaceActive for Classic map, party, widget, choice, and input UI
suppression. Update processSceneRef so pending experience resolution does not
take the inline path or set lastProcessedMsgRef, ensuring the turn is processed
after the package mounts. Also prevent scene processing when the selected
experience has scene analysis disabled by chatMeta.enableAgents, and restore
Classic fallback only once the query settles without the selected package.
- Around line 8336-8340: Apply activeExperienceChrome.providesInventory and
providesCombat consistently across every Classic path in GameSurface, not only
classicInventoryOpener and classicCombatStarter. Gate GameCombatUI callbacks,
the combatUiActive branch, Classic overlays, and queued encounter or generation
flows when the corresponding declaration is true, while preserving
custom-provided behavior. When inventory ownership becomes true, close any
already-open Classic inventory.
- Around line 6443-6447: Update pushExperienceBackground and the surrounding
GameSurface state so setExperienceBackgroundTag stores the raw pushed value
instead of resolving it immediately. Derive experienceBackgroundUrl from that
raw value and the latest scopedAssetMap before displayedBackground consumes it,
preserving direct asset paths and null handling while allowing later manifest
refreshes to resolve previously unknown tags.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a8984ad-84e9-4dad-8410-b1ca51fc65b0
📒 Files selected for processing (1)
packages/client/src/components/game/GameSurface.tsx
…size The seam is iterated during render, and the guard only checked `.size` -- any object carrying that property passed and then threw a TypeError mid-render, taking the surface down with it. The stored value is now normalized: a real Map or an empty one, so the consumer cannot be handed something it cannot iterate. Normalizing rather than tightening the condition also keeps a valid playerAvatarUrl when the map alongside it is malformed, which a condition change alone would have dropped.
|
@coderabbitai the actionable one is fixed. Here is where I land on the four outside-diff items, so they do Speaker-avatar map — fixed, and normalized rather than just re-checked. The seam is iterated during Player-only avatar with no persona — leaving as is. Using Lazy background resolution — leaving as is. A package pushes a ready asset path because it knows its own Inventory and combat declarations on every Classic path — this one I think is a real point rather than a |
|
Tip For best results, initiate chat on the files or code changes.
I accept the remaining decisions for this PR:
I will not re-raise these items in this PR. 🐇📝 ✏️ Learnings added
You are interacting with an AI system. |
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)
packages/client/src/components/game/GameSurface.tsx (1)
2180-2185: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftGate all Classic chrome with
experienceOwnsGamewhile package discovery is pending.While the installed-packages query is pending,
experienceOwnsGameis true butexperienceSurfaceActiveis false. The map and party header at Line 11519, mobile widget tray at Line 11693, choice UI at Line 11713, and desktop widget rails at Line 12271 test onlyexperienceSurfaceActive. Classic UI can flash and accept input before the experience resolves.Keep
experienceSurfaceActivefor mounting the capability. UseexperienceOwnsGamefor Classic UI suppression until the query settles.🤖 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/client/src/components/game/GameSurface.tsx` around lines 2180 - 2185, Update the Classic UI suppression checks for the map and party header, mobile widget tray, choice UI, and desktop widget rails to use experienceOwnsGame instead of experienceSurfaceActive. Keep experienceSurfaceActive unchanged for mounting the capability, while ensuring all Classic chrome is gated by experienceOwnsGame during package discovery.
🤖 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 `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 2180-2185: Update the Classic UI suppression checks for the map
and party header, mobile widget tray, choice UI, and desktop widget rails to use
experienceOwnsGame instead of experienceSurfaceActive. Keep
experienceSurfaceActive unchanged for mounting the capability, while ensuring
all Classic chrome is gated by experienceOwnsGame during package discovery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87340cfb-15fd-4cb6-8118-32fcccc6649e
📒 Files selected for processing (1)
packages/client/src/components/game/GameSurface.tsx
The map, party bar and the three widget rails tested experienceSurfaceActive, which only turns true once the installed-packages query resolves. On a cold cache that left the built-in HUD on screen, and clickable, for as long as package discovery took, before the surface replaced it. They now test experienceOwnsGame, which the chat already answers from its metadata. The split is now consistent: ownership gates chrome that must not appear for such a game, and the surface flag gates everything that needs the package to actually be mounted -- the seams it pushes into and the two mounts themselves. An ordinary game sets neither, so it is unaffected.
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)
packages/client/src/components/game/GameSurface.tsx (1)
6480-6507: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear Classic choices before an experience sends a turn.
Line 6488 exposes
sendMessagedirectly to the experience. This bypasseshandleSendGameTurn, which clearsactiveChoices. If the experience submits a choice and later removesprovidesChoices, the prior turn's choices can render as Classic choice cards after narration completes.Pass a wrapper that clears
activeChoicesbefore it callssendMessage.Proposed fix
+ const sendExperienceMessage = useCallback( + (...args: Parameters<typeof sendMessage>) => { + setActiveChoices(null); + return sendMessage(...args); + }, + [sendMessage], + ); + const experienceSurfaceProps = useMemo( () => (!experienceSurfaceActive ? undefined : { chatId: activeChatId, - sendMessage, + sendMessage: sendExperienceMessage, setExperienceBackgroundTag: pushExperienceBackground,🤖 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/client/src/components/game/GameSurface.tsx` around lines 6480 - 6507, Update the experienceSurfaceProps construction to pass a wrapper for sendMessage that clears activeChoices before delegating to the existing sendMessage function. Keep the wrapper’s arguments and return behavior aligned with sendMessage, and include any referenced state or callback in the useMemo dependency list so Classic choices are cleared for every experience-submitted turn.
🤖 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 `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 6480-6507: Update the experienceSurfaceProps construction to pass
a wrapper for sendMessage that clears activeChoices before delegating to the
existing sendMessage function. Keep the wrapper’s arguments and return behavior
aligned with sendMessage, and include any referenced state or callback in the
useMemo dependency list so Classic choices are cleared for every
experience-submitted turn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b8feba0-f3aa-4a6a-9abf-9b856a9142dd
📒 Files selected for processing (1)
packages/client/src/components/game/GameSurface.tsx
The surface received sendMessage directly, so a turn it drove skipped the cleanup the built-in send performs. Choices, a QTE or a queued encounter offered by the previous turn survived into the next one and could render after its narration finished. It now goes through the same clearPendingInteractiveCommands the built-in send uses, rather than clearing only the choices, so a turn driven by the experience leaves exactly the state a typed one does.
|
@SpicyMarinara I thinkg that I defeated the rabbit, it was... a learning experience. |
SpicyMarinara
left a comment
SpicyMarinara
left a comment
There was a problem hiding this comment.
Maintainer review complete on 95b6592. CodeRabbit has no unresolved threads; focused client/server/shared TypeScript, client ESLint, localization, version consistency, and capability lifecycle validation pass. UI uses theme accent tokens and responsive touch/focus states.
SpicyMarinara
left a comment
SpicyMarinara
left a comment
There was a problem hiding this comment.
Reconfirming maintainer approval on 6fd713b after the clean staging sync; the reviewed capability and UI changes are unchanged.
Important
Contributions target
staging. OnlySpicyMarinaramay promote this repository'sstagingbranch or a same-repositoryhotfix/*branch tomain.Outside and first-time contributors also require an approving review from
SpicyMarinara.Linked issue
Closes #4526
Why this change
Marinara can already be extended a lot — a package can add a panel, a chat surface, a settings
section. What it cannot do is provide a Game mode of its own: one that draws its own HUD, menus and
combat, keeps its own state, and replaces the built-in systems it implements. Everything a package can
contribute today sits next to Game mode rather than being one.
This adds that seam, plus the three smaller pieces of host support it turned out to need. All of it is
generic: the engine never names a package, an experience DECLARES what it replaces, and anything left
undeclared stays built-in. A game without an experience behaves exactly as it does today.
It's four separable pieces, described below so they can be reviewed one at a time.
What changed
1. Re-scan the asset manifest after packages activate —
packages/server/src/app.ts(+12)Unrelated to game modes, and useful to any package: a package can install its own art during
activate(), which runs after the boot-time asset scan. Until now that art stayed invisible toeverything that reads the manifest — scene analysis, the asset pickers — until the next restart.
This re-runs the same idempotent scan the upload routes already re-run.
Guarded with
try/catch+app.log.warnon purpose: this call runs over files a package has justwritten, and
scanDiruses barereaddirSync/statSync. A stale manifest costs that package its artuntil the next scan; failing to boot costs the user everything.
2. A per-turn
prompt-contexthook — 3 files + 1 newserver/services/capability-packages/capability-prompt-context.service.tsserver/services/capability-packages/capability-module-runtime.service.ts(+17)server/routes/generate.routes.ts(+23)server/services/game/gm-prompts.ts(+14/−3)The
prompt-contextpermission already existed in the manifest enum but had no consumer, sodeclaring it did nothing. This gives it one: a package that owns live state can append a block to the
system prompt of each turn, so the model narrates in sync with what the player is looking at instead of
guessing.
Registration is gated on the permission. Contributors are keyed by package id, so re-activation replaces
rather than stacks. A contributor that returns nothing is skipped and one that throws is logged and
skipped — a package can never break a turn.
A contribution may also declare which built-in systems it replaces (
provides). Today that's{ inventory?: boolean }: an experience with its own item system takes[inventory: …]and thePLAYER INVENTORY:block out of the GM format reminder. This has to be declarative rather than a line inthe package's own prompt, because the format reminder is injected as the LAST user message — a package
can't out-argue it from the system prompt. Anything not declared stays built-in.
3. Resource writes — 2 files (+97)
shared/types/capability-runtime.ts(+55)server/services/capability-packages/capability-resources.service.ts(+42)The resource facade exposed reads only. A package's setup flow needs to find-or-create the player persona
and its lorebook, so this adds six write methods as thin passthroughs to engine storage.
They are optional members of
CapabilityResourceHost, so adding them is not a breaking type changefor any other implementation, and a host that only wants to expose reads doesn't have to stub six
throwers. The input types are declared narrowly enough to hand straight to storage (a closed
CapabilityLorebookCategoryunion, a required entryname), so nothing needs a cast — a wrong value is acompile error in the package instead of a silent rejection at write time.
4. The game-mode experience seam — 7 files
shared/schemas/capability-package.schema.ts(+16) — thegame-surfaceslot and itscontributions.gameSurface.surfaceClassoptionshared/types/game.ts(+9) andserver/routes/game.routes.ts(+6) —gameExperienceId+experienceConfigon the setup config, copied to the chat metadata on createclient/components/game/GameSurface.tsx(+323/−36) — the mount and every seamclient/components/game/GameSetupWizard.tsx(+10/−1) — an optionalexperiencesSlotclient/components/game/NewGameExperienceChooser.tsx— the Experiences blockclient/components/game/GameNarration.tsx(+27/−6) — five inert theming hooksclient/src/localization/locales/en.json(+4)The experience is chosen when the game is created, from an Experiences block in the setup wizard's
first step, and belongs to that game for its lifetime. It is deliberately not a per-chat toggle: an
experience owns the whole run — its own inventory, combat, calendar and state — so switching one on
midway through an ordinary game, or off midway through its own, leaves a half-built run either way.
Which package it is comes from the manifest (it declares the
game-surfaceslot), so nothing here istied to a specific package id.
grepfor any package name across these files returns zero.The surface mounts twice: the main layer above the narration for HUD and menus, and a presentation-only
underlay in the background band for what must render behind the dialogue box, like a standing
sprite. Same
view, distinguished by constant props, so no new value was needed in theCapabilityElementview union.The seams are all declarative, and each is gated host-side so a stale value can never leak into an
ordinary game: scene background, speaker avatars, the live
activeSpeaker, an in-flow choice slot abovethe narration box, the host's sprite-scale setting, the resolved scene background handed to the underlay,
and a chrome declaration (
providesInventory/providesCombat/providesPlayerInput) by which anexperience says which built-in chrome it replaces. The Inventory and Start-combat buttons are hidden by
simply not receiving their callbacks — withholding beats hiding, since there's no dead button and no way
to reach the built-in flow underneath.
The five hooks in
GameNarrationare class names and onedata-attribute, with no logic. Nothing in theengine styles them, so Classic renders identically; an experience needs them because that chrome renders
outside its own element and is otherwise unreachable from its stylesheet.
The first-game tutorial is suppressed for an experience — it spotlights built-in chrome that isn't on
screen. Suppressed, not consumed: the persisted "never show again" flag is left untouched, so a player
who runs an experience first still gets the tour on their first ordinary game.
5. Clear pending choices and QTEs on a scope change —
packages/client/.../GameSurface.tsx(+3)Pre-existing, and the only change here that touches ordinary games. Choices offered by one game stayed on
screen in the next one and were answerable there; an active QTE leaks the same way and carries a timer
that then fires in the new game. Every site clearing either one is driven by a player action or by the
session concluding, and the effect keyed on the scope resets about twenty other pieces of turn state
without touching these two.
It is fixed here rather than separately because it lives in the same effect this PR was asked to reset the
experience seam state in, and resetting ours while leaving these two leaking would be arbitrary. Say the
word and I will split it out.
Validation
pnpm checkpasses locallyCONTRIBUTING.mdManual verification notes
Ran against
staging, with a package that provides an experience installed:The experience path. The surface mounts, HUD and menus render over the narration, and the standing
sprite draws behind the dialogue box (the underlay mount). Speaker avatars resolve for a cast that has
no character cards. The experience's choice menu lands above the narration box rather than floating over
it. The built-in Inventory and Start-combat buttons are absent, and the text input disappears while a menu
is on screen and comes back when there is nothing to pick — the chrome declaration is live, not one-shot.
Switching between modes. Moved back and forth between an experience game and an ordinary one
repeatedly. No state leaks in either direction: no stuck background, no leftover avatars, no chrome from
one showing up in the other.
Ordinary Game mode. Checked on a fresh ordinary game: the setup wizard is unchanged apart from the
added block, and the Inventory / Start-combat buttons, text input, map, party bar, HUD widgets and
per-scene background and music all behave as before. The first-game tutorial still fires on the first
ordinary game even after playing an experience first, which is the point of suppressing it rather than
consuming the flag.
One deliberate change to ordinary games, described under part 5 below: pending choices and QTEs are now
cleared when the game scope changes.
Empty state. With no experience installed, the block renders its "none downloaded yet" state with an
affordance to open the catalog, and the built-in wizard is untouched.
Part 1 — the manifest is rebuilt after activation and indexes the package's art (114 entries for the
installed package) on the first restart after installing, with no second restart needed.
Part 2 — verified in a real game in both modes: an ordinary game still gets the
[inventory: …]command, and the experience — which declares
provides: { inventory: true }— does not.Part 3 — creating a game through the experience's setup creates the player persona and its lorebook
with the expected fields.
Docs and release impact
docs-i18nbranch updated to match, or a[docs-i18n]follow-up issue opened (see CONTRIBUTING.md § Translated documentation)docs/development/optional-agent-packages.md— extended "Package model" to cover full Game-modesurfaces, per-turn prompt contributions, and the resource writes. No version heading, as suggested.
Nothing under
docs/proper changed, so nodocs-i18nwork is needed. No version bump.UI evidence (if applicable)
Before:
After:
Summary by CodeRabbit