merging breaks storyboards on staging, wait till 2.3.5 rdy [Feature] Extract Storyboards into an installable agent#4121
merging breaks storyboards on staging, wait till 2.3.5 rdy [Feature] Extract Storyboards into an installable agent#4121thetopham wants to merge 11 commits into
Conversation
📝 WalkthroughWalkthroughThis PR extracts Game Storyboard configuration and prompt ownership into a host-managed ChangesStoryboard agent extraction
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GameSurface
participant StoryboardAgentSettings
participant GameRoutes
participant ImageProvider
participant VideoProvider
GameSurface->>StoryboardAgentSettings: normalize active agent settings
GameSurface->>GameRoutes: request storyboard generation
GameRoutes->>StoryboardAgentSettings: apply settings to generation metadata
GameRoutes->>ImageProvider: generate storyboard images
GameRoutes->>VideoProvider: generate storyboard videos
GameRoutes-->>GameSurface: return storyboard assets
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
feature
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
the
refactor
thetopham
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
scripts/regressions/roleplay-streaming.regression.ts (1)
90-96: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the cleanup assertion to the missed-transition branch.
The broad
[\s\S]*?chain can pass when the dispatch, cache invalidation, andclearPerChatStatecall occur in separate branches. Match the exactifblock—or use a structural parser—so this regression proves the cleanup path itself performs all three operations.🤖 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 `@scripts/regressions/roleplay-streaming.regression.ts` around lines 90 - 96, The cleanup assertion around generationCleanupSource is too permissive and can combine operations from separate branches. Scope the regex or structural parsing to the missed-transition if block containing the roleplay/game condition and !spatialCapabilityRefreshDispatched, then assert that this same block dispatches the hierarchical-maps spatial_context_refresh event, invalidates the exact spatialContextKeys.detail query with refetchType "active", and calls clearPerChatState.packages/client/src/components/game/GameSurface.tsx (2)
2129-2141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe constant exists now; use it.
This PR promotes
STORYBOARD_AGENT_IDinto the shared barrel (packages/shared/src/features/agents/storyboard-agent-settings.tsLine 3), and sibling call sites such asStoryboardChatSettingsPanel.tsxalready consume it. Here the literal"storyboard"is hand-copied three times. A single import removes the possibility of a divergent typo surviving compilation.⚗️ Substitution
- (chatMeta.activeAgentIds as string[]).includes("storyboard"); + (chatMeta.activeAgentIds as string[]).includes(STORYBOARD_AGENT_ID); const { data: agentConfigs } = useAgentConfigs(storyboardAgentActive); const storyboardAgentConfig = useMemo( - () => agentConfigs?.find((config) => config.type === "storyboard") ?? null, + () => agentConfigs?.find((config) => config.type === STORYBOARD_AGENT_ID) ?? null, [agentConfigs], ); const storyboardAgentSettings = useMemo( - () => normalizeStoryboardAgentSettings(mergeBuiltInAgentSettings("storyboard", storyboardAgentConfig?.settings)), + () => + normalizeStoryboardAgentSettings( + mergeBuiltInAgentSettings(STORYBOARD_AGENT_ID, storyboardAgentConfig?.settings), + ), [storyboardAgentConfig?.settings], );🤖 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 2129 - 2141, Import and use the shared STORYBOARD_AGENT_ID constant throughout the storyboard agent logic in GameSurface, replacing each hard-coded "storyboard" value in the active-agent check, agent-config lookup, and mergeBuiltInAgentSettings call. Preserve the existing behavior and dependencies.
10238-10250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep related storyboard state updates out of the size updater.
In
handleStoryboardViewerSizeChange,setStoryboardViewerWidthandsetStoryboardViewerPositionshould not be invoked from insidesetStoryboardViewerSize((size) => ...); updater callbacks must return the next state value and not perform queued state updates. Move these setters into the handler body and use functional updates forsize/positionif they depend on current values.🤖 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 10238 - 10250, Refactor handleStoryboardViewerSizeChange so the setStoryboardViewerSize updater only computes and returns the next size, without invoking other setters. Move storyboard viewer width and position updates into the handler body, using functional updates for size-dependent calculations and clamping position against the computed next width.packages/client/src/components/agents/StoryboardAgentSettingsPanel.tsx (1)
292-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo vials of the same reagent, mixed by hand instead of measured from the same source.
The keyframe (
1–6) and duration (1–15) bounds here are hand-typed, while the siblingStoryboardChatSettingsPanel.tsximports the equivalentGAME_STORYBOARD_KEYFRAME_COUNT_MIN/MAXandGAME_STORYBOARD_ANIMATION_DURATION_SECONDS_MIN/MAXconstants. Should those shared bounds ever be recalibrated, this agent-level editor would quietly drift out of alignment with the chat-level override panel.♻️ Proposed refactor
+import { + GAME_STORYBOARD_ANIMATION_DURATION_SECONDS_MAX, + GAME_STORYBOARD_ANIMATION_DURATION_SECONDS_MIN, + GAME_STORYBOARD_KEYFRAME_COUNT_MAX, + GAME_STORYBOARD_KEYFRAME_COUNT_MIN, +} from "`@marinara-engine/shared`"; ... - min={1} - max={6} + min={GAME_STORYBOARD_KEYFRAME_COUNT_MIN} + max={GAME_STORYBOARD_KEYFRAME_COUNT_MAX} value={settings.keyframeCount} - onChange={(event) => update({ keyframeCount: Math.min(6, Math.max(1, Number(event.target.value) || 1)) })} + onChange={(event) => + update({ + keyframeCount: Math.min( + GAME_STORYBOARD_KEYFRAME_COUNT_MAX, + Math.max(GAME_STORYBOARD_KEYFRAME_COUNT_MIN, Number(event.target.value) || 1), + ), + }) + } ... - min={1} - max={15} + min={GAME_STORYBOARD_ANIMATION_DURATION_SECONDS_MIN} + max={GAME_STORYBOARD_ANIMATION_DURATION_SECONDS_MAX}🤖 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/agents/StoryboardAgentSettingsPanel.tsx` around lines 292 - 314, Update the StoryboardAgentSettingsPanel inputs and onChange clamping to reuse the shared GAME_STORYBOARD_KEYFRAME_COUNT_MIN/MAX and GAME_STORYBOARD_ANIMATION_DURATION_SECONDS_MIN/MAX constants, matching StoryboardChatSettingsPanel.tsx. Remove the duplicated numeric bounds while preserving the existing validation behavior.packages/client/src/localization/locales/en.json (1)
1220-1253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove now-experimental corpses before they pollute the locale inventory.
ui.agents.storyboard.promptConnectionDescriptionis defined but is not referenced by anylocalizeUi()consumer outsidepackages/client/src/localization/locales/en.json; remove the orphaned entry or wire it to a Storyboard prompt/connection panel if it is needed.🤖 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/localization/locales/en.json` around lines 1220 - 1253, Remove the unused ui.agents.storyboard.promptConnectionDescription entry from the English locale inventory, unless the Storyboard prompt/connection panel is updated to reference it through localizeUi().
🤖 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/agents/AgentEditor.tsx`:
- Around line 1415-1427: Move the setLocalStoryboardSettings call out of the
setLocalPromptTemplates updater in handleAddPromptTemplate, while still
appending the newly created option ID when isStoryboardAgent is true. Preserve
the option creation and list update, and follow handleRemovePromptTemplate’s
pattern so the updater remains pure.
In `@packages/client/src/components/game/game-storyboard-ui.ts`:
- Around line 48-86: Update buildStoryboardVisibleNarration to derive each
segment’s source index using segment.sourceSegmentIndex ?? index, and use that
value for segmentDeletes and segmentEdits lookups. Keep the existing filtering
and narration formatting behavior unchanged, matching
buildStoryboardSectionsFromMessage’s indexing scheme.
In `@packages/client/src/components/game/GameStoryboardViewer.tsx`:
- Around line 132-144: Localize every user-facing fallback in
GameStoryboardViewer: pass the storyboard image alt fallback and the “Storyboard
turn” and “Generating keyframes...” labels through localizeUi with
interpolation. Replace the raw frame.status.replace("_", " ") display with a
status-enum-to-localized-label mapping, preserving the existing status values
while ensuring no underscore-form or other untranslated string reaches the UI.
- Around line 223-230: Update the resize handle in GameStoryboardViewer and the
surrounding GameSurface flow so its focusable control supports keyboard
resizing: thread an onResizeByKeyboard(delta: number) callback from GameSurface,
reuse clampStoryboardViewerWidth for arrow-key width adjustments, and assign an
appropriate interactive role while preserving the existing pointer resize
behavior.
In `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 3754-3759: Remove the constant
`gameStoryboardAnimationDurationConfigured` and simplify all downstream
ternaries that use it, since the normalized fallback always produces a finite
duration. Preserve the intended behavior by either always sending the resolved
duration or, if unset values must defer to the provider, compute the
configuration check from `chatMeta.gameStoryboardAnimationDurationSeconds` alone
and retain the corresponding unset branch.
In `@packages/client/src/components/game/GameVolumeMixer.tsx`:
- Around line 49-55: Update the rows definition in GameVolumeMixer to pass each
label through localizeUi using dedicated localization keys for Master, Music,
Sound Effects, TTS, and Ambient. Follow the existing localizeUi usage and
key-naming pattern in the surrounding component, while preserving the current
row structure and labels’ meanings.
In `@packages/client/src/hooks/use-generate.ts`:
- Line 1327: Make the spatial reconciliation invalidation fire-and-forget
instead of awaiting it, matching the existing spatial_transition_committed
handler. Update the finally-block logic around
spatialCapabilityRefreshDispatched and the related occurrences at the SSE
handler and other shared path so qc.invalidateQueries does not block stillOwner
cleanup or streaming-state teardown; preserve the existing query key and
invalidation conditions.
In `@packages/server/src/services/image/game-storyboard-image-prompt.ts`:
- Around line 25-33: Update the selectedTemplate resolution in the storyboard
image prompt loader to fall back to the default storyboard template or first
available option when selectedId is stale or absent from options, matching the
shared selector behavior. Keep the existing error throw only when no usable
template exists at all.
In `@packages/shared/src/features/agents/storyboard-agent-settings.ts`:
- Around line 51-55: Update normalizeBoundedInteger to return fallback when
value is null, undefined, or an empty string before converting it with Number;
preserve the existing finite-number validation, truncation, and min/max clamping
for all other inputs, matching normalizeGameStoryboardKeyframeCount behavior.
---
Nitpick comments:
In `@packages/client/src/components/agents/StoryboardAgentSettingsPanel.tsx`:
- Around line 292-314: Update the StoryboardAgentSettingsPanel inputs and
onChange clamping to reuse the shared GAME_STORYBOARD_KEYFRAME_COUNT_MIN/MAX and
GAME_STORYBOARD_ANIMATION_DURATION_SECONDS_MIN/MAX constants, matching
StoryboardChatSettingsPanel.tsx. Remove the duplicated numeric bounds while
preserving the existing validation behavior.
In `@packages/client/src/components/game/GameSurface.tsx`:
- Around line 2129-2141: Import and use the shared STORYBOARD_AGENT_ID constant
throughout the storyboard agent logic in GameSurface, replacing each hard-coded
"storyboard" value in the active-agent check, agent-config lookup, and
mergeBuiltInAgentSettings call. Preserve the existing behavior and dependencies.
- Around line 10238-10250: Refactor handleStoryboardViewerSizeChange so the
setStoryboardViewerSize updater only computes and returns the next size, without
invoking other setters. Move storyboard viewer width and position updates into
the handler body, using functional updates for size-dependent calculations and
clamping position against the computed next width.
In `@packages/client/src/localization/locales/en.json`:
- Around line 1220-1253: Remove the unused
ui.agents.storyboard.promptConnectionDescription entry from the English locale
inventory, unless the Storyboard prompt/connection panel is updated to reference
it through localizeUi().
In `@scripts/regressions/roleplay-streaming.regression.ts`:
- Around line 90-96: The cleanup assertion around generationCleanupSource is too
permissive and can combine operations from separate branches. Scope the regex or
structural parsing to the missed-transition if block containing the
roleplay/game condition and !spatialCapabilityRefreshDispatched, then assert
that this same block dispatches the hierarchical-maps spatial_context_refresh
event, invalidates the exact spatialContextKeys.detail query with refetchType
"active", and calls clearPerChatState.
🪄 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 Plus
Run ID: 4f1931e4-833f-44be-bd39-bd78f83b7509
📒 Files selected for processing (39)
.github/plans/storyboard-agent-extraction.mde2e/core-flows.e2e.tspackages/client/src/components/agents/AgentEditor.tsxpackages/client/src/components/agents/StoryboardAgentSettingsPanel.tsxpackages/client/src/components/chat/AgentSettingsControls.tsxpackages/client/src/components/chat/ChatSettingsDrawer.tsxpackages/client/src/components/chat/ChatSetupWizard.tsxpackages/client/src/components/chat/StoryboardChatSettingsPanel.tsxpackages/client/src/components/game/GameSetupWizard.tsxpackages/client/src/components/game/GameStoryboardViewer.tsxpackages/client/src/components/game/GameSurface.tsxpackages/client/src/components/game/GameVolumeMixer.tsxpackages/client/src/components/game/game-storyboard-ui.tspackages/client/src/components/panels/SettingsPanel.tsxpackages/client/src/hooks/use-generate.tspackages/client/src/localization/locales/en.jsonpackages/server/src/routes/game.routes.tspackages/server/src/routes/generate/retry-agents-route.tspackages/server/src/services/agents/custom-agent-repositories.service.tspackages/server/src/services/capability-packages/package-manager.service.tspackages/server/src/services/game/game-asset-generation.tspackages/server/src/services/game/storyboard-agent-settings.tspackages/server/src/services/image/game-storyboard-image-prompt.tspackages/server/src/services/prompt-overrides/index.tspackages/server/src/services/prompt-overrides/registry.tspackages/server/src/services/prompt-overrides/registry/game-assets.tspackages/server/src/services/video/game-video-prompt.tspackages/shared/src/constants/chat-mode-capabilities.tspackages/shared/src/constants/game-storyboard-image-prompts.tspackages/shared/src/constants/game-storyboard-prompts.tspackages/shared/src/features/agents/agent-manifest.types.tspackages/shared/src/features/agents/agent-registry.tspackages/shared/src/features/agents/storyboard-agent-settings.tspackages/shared/src/index.tspackages/shared/src/schemas/capability-package.schema.tspackages/shared/src/types/agent.tsscripts/regressions/capability-package-lifecycle.regression.tsscripts/regressions/prompt.regression.tsscripts/regressions/roleplay-streaming.regression.ts
💤 Files with no reviewable changes (3)
- packages/client/src/components/panels/SettingsPanel.tsx
- packages/server/src/services/prompt-overrides/index.ts
- packages/server/src/services/prompt-overrides/registry.ts
| const handleAddPromptTemplate = useCallback(() => { | ||
| setLocalPromptTemplates((options) => [...options, createBlankPromptOption(options)]); | ||
| setLocalPromptTemplates((options) => { | ||
| const option = createBlankPromptOption(options); | ||
| if (isStoryboardAgent) { | ||
| setLocalStoryboardSettings((settings) => ({ | ||
| ...settings, | ||
| illustrationPlannerTemplateIds: [...settings.illustrationPlannerTemplateIds, option.id], | ||
| })); | ||
| } | ||
| return [...options, option]; | ||
| }); | ||
| markDirty(); | ||
| }, [markDirty]); | ||
| }, [isStoryboardAgent, markDirty]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A side effect hiding inside a "pure" vessel — most improper.
setLocalStoryboardSettings is summoned inside the updater callback given to setLocalPromptTemplates. React's contract requires such callbacks be pure — no side effects — precisely because it may invoke them more than once (StrictMode's double-invocation experiment exists to expose exactly this). Should that happen, the newly minted option.id could be appended into illustrationPlannerTemplateIds without a corresponding entry surviving in localPromptTemplates, leaving a dangling reference (later pruned by normalizeStoryboardAgentSettings, but incorrect in the interim). Compare with handleRemovePromptTemplate immediately below, which performs the equivalent second setState call outside its updater — the correct specimen to imitate.
🧪 Proposed fix — extract the side effect from the updater
const handleAddPromptTemplate = useCallback(() => {
- setLocalPromptTemplates((options) => {
- const option = createBlankPromptOption(options);
- if (isStoryboardAgent) {
- setLocalStoryboardSettings((settings) => ({
- ...settings,
- illustrationPlannerTemplateIds: [...settings.illustrationPlannerTemplateIds, option.id],
- }));
- }
- return [...options, option];
- });
- markDirty();
- }, [isStoryboardAgent, markDirty]);
+ const option = createBlankPromptOption(localPromptTemplates);
+ setLocalPromptTemplates((options) => [...options, option]);
+ if (isStoryboardAgent) {
+ setLocalStoryboardSettings((settings) => ({
+ ...settings,
+ illustrationPlannerTemplateIds: [...settings.illustrationPlannerTemplateIds, option.id],
+ }));
+ }
+ markDirty();
+ }, [isStoryboardAgent, localPromptTemplates, markDirty]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleAddPromptTemplate = useCallback(() => { | |
| setLocalPromptTemplates((options) => [...options, createBlankPromptOption(options)]); | |
| setLocalPromptTemplates((options) => { | |
| const option = createBlankPromptOption(options); | |
| if (isStoryboardAgent) { | |
| setLocalStoryboardSettings((settings) => ({ | |
| ...settings, | |
| illustrationPlannerTemplateIds: [...settings.illustrationPlannerTemplateIds, option.id], | |
| })); | |
| } | |
| return [...options, option]; | |
| }); | |
| markDirty(); | |
| }, [markDirty]); | |
| }, [isStoryboardAgent, markDirty]); | |
| const handleAddPromptTemplate = useCallback(() => { | |
| const option = createBlankPromptOption(localPromptTemplates); | |
| setLocalPromptTemplates((options) => [...options, option]); | |
| if (isStoryboardAgent) { | |
| setLocalStoryboardSettings((settings) => ({ | |
| ...settings, | |
| illustrationPlannerTemplateIds: [...settings.illustrationPlannerTemplateIds, option.id], | |
| })); | |
| } | |
| markDirty(); | |
| }, [isStoryboardAgent, localPromptTemplates, markDirty]); |
🤖 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/agents/AgentEditor.tsx` around lines 1415 -
1427, Move the setLocalStoryboardSettings call out of the
setLocalPromptTemplates updater in handleAddPromptTemplate, while still
appending the newly created option ID when isStoryboardAgent is true. Preserve
the option creation and list update, and follow handleRemovePromptTemplate’s
pattern so the updater remains pure.
| export function buildStoryboardVisibleNarration( | ||
| message: Message | null, | ||
| segmentEdits: Map<string, GameSegmentEdit>, | ||
| segmentDeletes: Set<string>, | ||
| ): string { | ||
| if (!message?.id || !message.content) return ""; | ||
| const visibleText: string[] = []; | ||
| const segments = parseNarrationSegments(message, EMPTY_GAME_SPEAKER_COLORS); | ||
| for (let index = 0; index < segments.length; index++) { | ||
| const segment = segments[index]!; | ||
| if (segmentDeletes.has(`${message.id}:${index}`)) continue; | ||
| if (segment.partyType === "side" || segment.partyType === "extra") continue; | ||
| const text = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${index}`)); | ||
| if (text) visibleText.push(text); | ||
| } | ||
| return visibleText.join("\n").trim(); | ||
| } | ||
|
|
||
| export function buildStoryboardSectionsFromMessage( | ||
| message: Message | null, | ||
| segmentEdits: Map<string, GameSegmentEdit>, | ||
| segmentDeletes: Set<string>, | ||
| ): GameStoryboardSourceSection[] { | ||
| if (!message?.id || !message.content) return []; | ||
| return parseNarrationSegments(message, EMPTY_GAME_SPEAKER_COLORS) | ||
| .map((segment, fallbackIndex): GameStoryboardSourceSection | null => { | ||
| const sourceIndex = segment.sourceSegmentIndex ?? fallbackIndex; | ||
| if (segmentDeletes.has(`${message.id}:${sourceIndex}`)) return null; | ||
| const content = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${sourceIndex}`)); | ||
| if (!content) return null; | ||
| return { | ||
| index: sourceIndex, | ||
| kind: segment.type, | ||
| speaker: segment.speaker?.trim() || null, | ||
| content: content.slice(0, 6000), | ||
| }; | ||
| }) | ||
| .filter((section): section is GameStoryboardSourceSection => Boolean(section)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Two functions, two indexing schemes — an inelegant asymmetry in my laboratory.
buildStoryboardVisibleNarration keys edits/deletes by the loop position (index), while buildStoryboardSectionsFromMessage keys by segment.sourceSegmentIndex ?? fallbackIndex. Whenever sourceSegmentIndex diverges from array position — the very reason that field exists — the narration text applies edits and deletions to the wrong segments while the section payload gets them right. Align both on the source index.
🔬 Proposed correction
const segments = parseNarrationSegments(message, EMPTY_GAME_SPEAKER_COLORS);
for (let index = 0; index < segments.length; index++) {
const segment = segments[index]!;
- if (segmentDeletes.has(`${message.id}:${index}`)) continue;
+ const sourceIndex = segment.sourceSegmentIndex ?? index;
+ if (segmentDeletes.has(`${message.id}:${sourceIndex}`)) continue;
if (segment.partyType === "side" || segment.partyType === "extra") continue;
- const text = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${index}`));
+ const text = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${sourceIndex}`));
if (text) visibleText.push(text);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function buildStoryboardVisibleNarration( | |
| message: Message | null, | |
| segmentEdits: Map<string, GameSegmentEdit>, | |
| segmentDeletes: Set<string>, | |
| ): string { | |
| if (!message?.id || !message.content) return ""; | |
| const visibleText: string[] = []; | |
| const segments = parseNarrationSegments(message, EMPTY_GAME_SPEAKER_COLORS); | |
| for (let index = 0; index < segments.length; index++) { | |
| const segment = segments[index]!; | |
| if (segmentDeletes.has(`${message.id}:${index}`)) continue; | |
| if (segment.partyType === "side" || segment.partyType === "extra") continue; | |
| const text = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${index}`)); | |
| if (text) visibleText.push(text); | |
| } | |
| return visibleText.join("\n").trim(); | |
| } | |
| export function buildStoryboardSectionsFromMessage( | |
| message: Message | null, | |
| segmentEdits: Map<string, GameSegmentEdit>, | |
| segmentDeletes: Set<string>, | |
| ): GameStoryboardSourceSection[] { | |
| if (!message?.id || !message.content) return []; | |
| return parseNarrationSegments(message, EMPTY_GAME_SPEAKER_COLORS) | |
| .map((segment, fallbackIndex): GameStoryboardSourceSection | null => { | |
| const sourceIndex = segment.sourceSegmentIndex ?? fallbackIndex; | |
| if (segmentDeletes.has(`${message.id}:${sourceIndex}`)) return null; | |
| const content = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${sourceIndex}`)); | |
| if (!content) return null; | |
| return { | |
| index: sourceIndex, | |
| kind: segment.type, | |
| speaker: segment.speaker?.trim() || null, | |
| content: content.slice(0, 6000), | |
| }; | |
| }) | |
| .filter((section): section is GameStoryboardSourceSection => Boolean(section)); | |
| } | |
| export function buildStoryboardVisibleNarration( | |
| message: Message | null, | |
| segmentEdits: Map<string, GameSegmentEdit>, | |
| segmentDeletes: Set<string>, | |
| ): string { | |
| if (!message?.id || !message.content) return ""; | |
| const visibleText: string[] = []; | |
| const segments = parseNarrationSegments(message, EMPTY_GAME_SPEAKER_COLORS); | |
| for (let index = 0; index < segments.length; index++) { | |
| const segment = segments[index]!; | |
| const sourceIndex = segment.sourceSegmentIndex ?? index; | |
| if (segmentDeletes.has(`${message.id}:${sourceIndex}`)) continue; | |
| if (segment.partyType === "side" || segment.partyType === "extra") continue; | |
| const text = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${sourceIndex}`)); | |
| if (text) visibleText.push(text); | |
| } | |
| return visibleText.join("\n").trim(); | |
| } | |
| export function buildStoryboardSectionsFromMessage( | |
| message: Message | null, | |
| segmentEdits: Map<string, GameSegmentEdit>, | |
| segmentDeletes: Set<string>, | |
| ): GameStoryboardSourceSection[] { | |
| if (!message?.id || !message.content) return []; | |
| return parseNarrationSegments(message, EMPTY_GAME_SPEAKER_COLORS) | |
| .map((segment, fallbackIndex): GameStoryboardSourceSection | null => { | |
| const sourceIndex = segment.sourceSegmentIndex ?? fallbackIndex; | |
| if (segmentDeletes.has(`${message.id}:${sourceIndex}`)) return null; | |
| const content = formatNarrationSegmentForContext(segment, segmentEdits.get(`${message.id}:${sourceIndex}`)); | |
| if (!content) return null; | |
| return { | |
| index: sourceIndex, | |
| kind: segment.type, | |
| speaker: segment.speaker?.trim() || null, | |
| content: content.slice(0, 6000), | |
| }; | |
| }) | |
| .filter((section): section is GameStoryboardSourceSection => Boolean(section)); | |
| } |
🤖 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/game-storyboard-ui.ts` around lines 48 -
86, Update buildStoryboardVisibleNarration to derive each segment’s source index
using segment.sourceSegmentIndex ?? index, and use that value for segmentDeletes
and segmentEdits lookups. Keep the existing filtering and narration formatting
behavior unchanged, matching buildStoryboardSectionsFromMessage’s indexing
scheme.
| ) : frame?.image ? ( | ||
| <img | ||
| src={frame.image.url} | ||
| alt={frame.title || `Storyboard keyframe ${frame.index + 1}`} | ||
| className="aspect-video w-full bg-black object-cover" | ||
| draggable={false} | ||
| /> | ||
| ) : ( | ||
| <div className="flex aspect-video w-full items-center justify-center gap-2 bg-black/45 text-xs text-white/55"> | ||
| {generating ? <Loader2 size={14} className="animate-spin" /> : null} | ||
| {frame ? frame.status.replace("_", " ") : localizeUi("ui.game.gamesurfacecomponent.creatingStoryboard")} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Untranslated strings smuggled in among the localized ones.
alt={frame.title || \Storyboard keyframe ${frame.index + 1}`}and the rawframe.status.replace("_", " ")bypasslocalizeUi, as do the "Storyboard turn"and"Generating keyframes..."fallbacks further down (Lines 149, 206). Every neighboring label in this component goes through the localization pipeline; these few escaped the procedure. Route them throughlocalizeUi` with interpolation, and map the status enum to localized labels rather than exposing the underscore form to the subject.
🤖 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/GameStoryboardViewer.tsx` around lines
132 - 144, Localize every user-facing fallback in GameStoryboardViewer: pass the
storyboard image alt fallback and the “Storyboard turn” and “Generating
keyframes...” labels through localizeUi with interpolation. Replace the raw
frame.status.replace("_", " ") display with a status-enum-to-localized-label
mapping, preserving the existing status values while ensuring no underscore-form
or other untranslated string reaches the UI.
| <div | ||
| className="absolute -bottom-2 -right-2 z-20 flex h-7 w-7 cursor-nwse-resize items-center justify-center rounded-lg border border-[var(--marinara-chat-chrome-button-border)] bg-[var(--marinara-chat-chrome-button-bg)] text-[var(--marinara-chat-chrome-button-text)] shadow-lg transition-all duration-150 hover:border-[var(--marinara-chat-chrome-button-border-hover)] hover:bg-[var(--marinara-chat-chrome-button-bg-hover)] hover:text-[var(--marinara-chat-chrome-button-text-hover)] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[var(--marinara-chat-chrome-focus-ring)] active:scale-95" | ||
| aria-label={localizeUi("ui.game.gamesurfacecomponent.resizeStoryboardViewer")} | ||
| tabIndex={0} | ||
| {...resizeHandlers} | ||
| > | ||
| <span className="h-2.5 w-2.5 rounded-br-sm border-b-2 border-r-2 border-current" /> | ||
| </div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
A focusable element that responds to nothing is a cruel experiment.
The resize handle carries tabIndex={0} and an aria-label, so keyboard users can focus it — yet it exposes only pointer handlers. The subject reaches the node and finds no exit. Either grant it a role="slider"/role="button" with arrow-key width adjustment, or remove tabIndex so it stops advertising an interaction it cannot honor.
🧪 Minimal remedy — keyboard resize
<div
className="absolute -bottom-2 -right-2 z-20 flex h-7 w-7 cursor-nwse-resize items-center justify-center rounded-lg border border-[var(--marinara-chat-chrome-button-border)] bg-[var(--marinara-chat-chrome-button-bg)] text-[var(--marinara-chat-chrome-button-text)] shadow-lg transition-all duration-150 hover:border-[var(--marinara-chat-chrome-button-border-hover)] hover:bg-[var(--marinara-chat-chrome-button-bg-hover)] hover:text-[var(--marinara-chat-chrome-button-text-hover)] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[var(--marinara-chat-chrome-focus-ring)] active:scale-95"
aria-label={localizeUi("ui.game.gamesurfacecomponent.resizeStoryboardViewer")}
+ role="slider"
+ aria-valuenow={width}
+ onKeyDown={(event) => {
+ if (event.key !== "ArrowLeft" && event.key !== "ArrowRight") return;
+ event.preventDefault();
+ onResizeByKeyboard(event.key === "ArrowRight" ? 32 : -32);
+ }}
tabIndex={0}
{...resizeHandlers}
>This requires threading a small onResizeByKeyboard(delta: number) prop from GameSurface.tsx, where clampStoryboardViewerWidth already lives.
🤖 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/GameStoryboardViewer.tsx` around lines
223 - 230, Update the resize handle in GameStoryboardViewer and the surrounding
GameSurface flow so its focusable control supports keyboard resizing: thread an
onResizeByKeyboard(delta: number) callback from GameSurface, reuse
clampStoryboardViewerWidth for arrow-key width adjustments, and assign an
appropriate interactive role while preserving the existing pointer resize
behavior.
| const gameStoryboardAnimationDurationConfigured = hasGameStoryboardAnimationDuration( | ||
| chatMeta.gameStoryboardAnimationDurationSeconds, | ||
| chatMeta.gameStoryboardAnimationDurationSeconds ?? storyboardAgentSettings.animationDurationSeconds, | ||
| ); | ||
| const gameStoryboardAnimationDurationSeconds = normalizeGameStoryboardAnimationDuration( | ||
| chatMeta.gameStoryboardAnimationDurationSeconds, | ||
| chatMeta.gameStoryboardAnimationDurationSeconds ?? storyboardAgentSettings.animationDurationSeconds, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
gameStoryboardAnimationDurationConfigured is now a constant true. Observe.
storyboardAgentSettings.animationDurationSeconds is produced by normalizeStoryboardAgentSettings, which always returns a finite number (default 6). Therefore chatMeta.gameStoryboardAnimationDurationSeconds ?? settings.animationDurationSeconds can never be null/undefined/"", and hasGameStoryboardAnimationDuration can never return false. Every downstream ternary guarded by this flag — Lines 5757-5760, 5881-5883, 5897-5900 — has collapsed into its first branch, and durationSeconds: undefined is now unreachable.
If sending the agent default explicitly is the intended behavior, delete the flag and the dead ternaries. If "unset means let the provider decide" was meant to survive, the check must be applied to chatMeta.gameStoryboardAnimationDurationSeconds alone.
🧫 Latter interpretation
const gameStoryboardAnimationDurationConfigured = hasGameStoryboardAnimationDuration(
- chatMeta.gameStoryboardAnimationDurationSeconds ?? storyboardAgentSettings.animationDurationSeconds,
+ chatMeta.gameStoryboardAnimationDurationSeconds,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const gameStoryboardAnimationDurationConfigured = hasGameStoryboardAnimationDuration( | |
| chatMeta.gameStoryboardAnimationDurationSeconds, | |
| chatMeta.gameStoryboardAnimationDurationSeconds ?? storyboardAgentSettings.animationDurationSeconds, | |
| ); | |
| const gameStoryboardAnimationDurationSeconds = normalizeGameStoryboardAnimationDuration( | |
| chatMeta.gameStoryboardAnimationDurationSeconds, | |
| chatMeta.gameStoryboardAnimationDurationSeconds ?? storyboardAgentSettings.animationDurationSeconds, | |
| ); | |
| const gameStoryboardAnimationDurationConfigured = hasGameStoryboardAnimationDuration( | |
| chatMeta.gameStoryboardAnimationDurationSeconds, | |
| ); | |
| const gameStoryboardAnimationDurationSeconds = normalizeGameStoryboardAnimationDuration( | |
| chatMeta.gameStoryboardAnimationDurationSeconds ?? storyboardAgentSettings.animationDurationSeconds, | |
| ); |
🤖 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 3754 -
3759, Remove the constant `gameStoryboardAnimationDurationConfigured` and
simplify all downstream ternaries that use it, since the normalized fallback
always produces a finite duration. Preserve the intended behavior by either
always sending the resolved duration or, if unset values must defer to the
provider, compute the configuration check from
`chatMeta.gameStoryboardAnimationDurationSeconds` alone and retain the
corresponding unset branch.
| const rows = [ | ||
| { id: "master", label: "Master", value: masterVolume, onChange: onMasterVolumeChange }, | ||
| { id: "music", label: "Music", value: musicVolume, onChange: onMusicVolumeChange }, | ||
| { id: "sfx", label: "Sound Effects", value: sfxVolume, onChange: onSfxVolumeChange }, | ||
| { id: "tts", label: "TTS", value: ttsVolume, onChange: onTtsVolumeChange }, | ||
| { id: "ambient", label: "Ambient", value: ambientVolume, onChange: onAmbientVolumeChange }, | ||
| ]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ku ku ku... a most curious oversight, isn't it? Every other label in this specimen — the panel title, the mute tooltip, the close button — has been properly translated via localizeUi. Yet these five row labels, "Master," "Music," "Sound Effects," "TTS," "Ambient," remain frozen in raw English, untouched by the localization apparatus. A small inconsistency, but one that will surely irritate our non-English-speaking test subjects.
Route these labels through localizeUi with dedicated keys, matching the pattern already demonstrated mere lines below.
🧪 Proposed remedy
const rows = [
- { id: "master", label: "Master", value: masterVolume, onChange: onMasterVolumeChange },
- { id: "music", label: "Music", value: musicVolume, onChange: onMusicVolumeChange },
- { id: "sfx", label: "Sound Effects", value: sfxVolume, onChange: onSfxVolumeChange },
- { id: "tts", label: "TTS", value: ttsVolume, onChange: onTtsVolumeChange },
- { id: "ambient", label: "Ambient", value: ambientVolume, onChange: onAmbientVolumeChange },
+ { id: "master", label: localizeUi("game.toolbar.volume.master"), value: masterVolume, onChange: onMasterVolumeChange },
+ { id: "music", label: localizeUi("game.toolbar.volume.music"), value: musicVolume, onChange: onMusicVolumeChange },
+ { id: "sfx", label: localizeUi("game.toolbar.volume.sfx"), value: sfxVolume, onChange: onSfxVolumeChange },
+ { id: "tts", label: localizeUi("game.toolbar.volume.tts"), value: ttsVolume, onChange: onTtsVolumeChange },
+ { id: "ambient", label: localizeUi("game.toolbar.volume.ambient"), value: ambientVolume, onChange: onAmbientVolumeChange },
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rows = [ | |
| { id: "master", label: "Master", value: masterVolume, onChange: onMasterVolumeChange }, | |
| { id: "music", label: "Music", value: musicVolume, onChange: onMusicVolumeChange }, | |
| { id: "sfx", label: "Sound Effects", value: sfxVolume, onChange: onSfxVolumeChange }, | |
| { id: "tts", label: "TTS", value: ttsVolume, onChange: onTtsVolumeChange }, | |
| { id: "ambient", label: "Ambient", value: ambientVolume, onChange: onAmbientVolumeChange }, | |
| ]; | |
| const rows = [ | |
| { id: "master", label: localizeUi("game.toolbar.volume.master"), value: masterVolume, onChange: onMasterVolumeChange }, | |
| { id: "music", label: localizeUi("game.toolbar.volume.music"), value: musicVolume, onChange: onMusicVolumeChange }, | |
| { id: "sfx", label: localizeUi("game.toolbar.volume.sfx"), value: sfxVolume, onChange: onSfxVolumeChange }, | |
| { id: "tts", label: localizeUi("game.toolbar.volume.tts"), value: ttsVolume, onChange: onTtsVolumeChange }, | |
| { id: "ambient", label: localizeUi("game.toolbar.volume.ambient"), value: ambientVolume, onChange: onAmbientVolumeChange }, | |
| ]; |
🤖 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/GameVolumeMixer.tsx` around lines 49 -
55, Update the rows definition in GameVolumeMixer to pass each label through
localizeUi using dedicated localization keys for Master, Music, Sound Effects,
TTS, and Ambient. Follow the existing localizeUi usage and key-naming pattern in
the surrounding component, while preserving the current row structure and
labels’ meanings.
| let illustrationSettled = false; | ||
| let passiveStreamRecovered = false; | ||
| let spatialTransitionCommitted = false; | ||
| let spatialCapabilityRefreshDispatched = false; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
An unconditional network toll levied on every Roleplay and Game turn — most unbecoming of an efficient apparatus.
The new reconciliation safety net in the finally block awaits qc.invalidateQueries(...) for every Roleplay/Game generation whose spatial refresh wasn't already dispatched — with no gate on whether Hierarchical Maps (or any spatial-context capability) is even installed for that chat, only on chat mode. When the spatialContextKeys.detail query is actively observed, refetchType: "active" triggers a real network refetch, and this await sits directly in the path before stillOwner's cleanup (setStreaming(false), buffer clearing, etc.) executes — meaning every single turn in an eligible chat now pays a network round-trip before the streaming indicator can clear, purely for a background cache-freshness reconciliation that doesn't need to block anything downstream. The sibling handler for the actual spatial_transition_committed SSE event (lines 1608-1617) already does the right thing by firing its equivalent invalidation with void — this new block should follow the same fire-and-forget pattern.
⚗️ Proposed fix — stop blocking cleanup on this reconciliation
if (
(chatModeForGeneration === "roleplay" || chatModeForGeneration === "game") &&
!spatialCapabilityRefreshDispatched
) {
// Narrated Maps transitions commit near the end of the server stream.
// Reconcile both client caches when the transition SSE was missed.
dispatchCapabilityClientEvent({
packageId: "hierarchical-maps",
type: "spatial_context_refresh",
chatId: params.chatId,
data: null,
});
- await qc.invalidateQueries({
+ void qc.invalidateQueries({
queryKey: spatialContextKeys.detail(params.chatId),
exact: true,
refetchType: "active",
});
}Also applies to: 1608-1617, 2796-2815
🤖 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/hooks/use-generate.ts` at line 1327, Make the spatial
reconciliation invalidation fire-and-forget instead of awaiting it, matching the
existing spatial_transition_committed handler. Update the finally-block logic
around spatialCapabilityRefreshDispatched and the related occurrences at the SSE
handler and other shared path so qc.invalidateQueries does not block stillOwner
cleanup or streaming-state teardown; preserve the existing query key and
invalidation conditions.
| const options = normalizeAgentPromptTemplateOptions(args.customTemplates).slice(0, 20); | ||
| const selectedId = readTrimmedString(args.templateId); | ||
| const selectedTemplate = selectedId ? options.find((template) => template.id === selectedId) : undefined; | ||
|
|
||
| if (selectedTemplate) return renderSelectedStoryboardImagePrompt(selectedTemplate, args.ctx); | ||
| const selectedTemplate = selectedId | ||
| ? options.find((template) => template.id === selectedId) | ||
| : (options.find((template) => template.id === GAME_STORYBOARD_IMAGE_PROMPT_TEMPLATE_ID) ?? options[0]); | ||
|
|
||
| if (args.promptOverridesStorage) { | ||
| const storedOverride = await loadStoredSceneIllustrationOverride(args.promptOverridesStorage, args.ctx); | ||
| if (storedOverride) return storedOverride; | ||
| if (!selectedTemplate) { | ||
| throw new Error("The Storyboard Agent does not provide a usable image formatter prompt."); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether a stale chat-level storyboard image template id can outlive the agent's template list.
rg -nP -C4 'gameStoryboardImagePromptTemplateId' --type=ts
echo '--- projection of the template list ---'
rg -nP -C6 'gameStoryboardImagePromptTemplates' --type=ts
echo '--- does any writer clear the id when templates change? ---'
rg -nP -C3 'gameStoryboardImagePromptTemplateId\s*:\s*(null|undefined|"")' --type=ts --type=tsxRepository: Pasta-Devs/Marinara-Engine
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tracked TS files (subset) ---'
git ls-files '*.ts' | rg '(storyboard-agent-settings|game-asset-generation|game-storyboard-image-prompt)' || true
echo '--- file sizes ---'
for f in \
packages/server/src/services/image/game-storyboard-image-prompt.ts \
packages/server/src/services/game/game-asset-generation.ts \
packages/server/src/services/game/storyboard-agent-settings.ts \
packages/shared/src/features/agents/storyboard-agent-settings.ts
do
if [ -f "$f" ]; then
wc -l "$f"
else
echo "MISSING $f"
fi
done
echo '--- relevant snippets ---'
for f in \
packages/server/src/services/image/game-storyboard-image-prompt.ts \
packages/server/src/services/game/game-asset-generation.ts \
packages/shared/src/features/agents/storyboard-agent-settings.ts
do
if [ -f "$f" ]; then
echo "===== $f ====="
sed -n '1,140p' "$f" | cat -n
fi
doneRepository: Pasta-Devs/Marinara-Engine
Length of output: 15624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- server storyboard-agent-settings.ts snippet ---'
sed -n '1,120p' packages/server/src/services/game/storyboard-agent-settings.ts | cat -n
echo '--- caller area around storyboard image prompt invocation ---'
sed -n '1020,1110p' packages/server/src/services/game/game-asset-generation.ts | cat -n
echo '--- all references in tracked ts/tsx files ---'
rg -n -C3 'gameStoryboardImagePromptTemplateId|gameStoryboardImagePromptTemplates|loadGameStoryboardImagePrompt|storyboardImagePromptTemplateId|storyboardImagePromptTemplates' --glob '*.{ts,tsx}' .
echo '--- behavioral probe for selection logic vs stale id ---'
node - <<'JS'
function readTrimmedString(value) {
return typeof value === "string" && value.trim() ? value.trim() : null;
}
function current(selectedId, ids) {
const selectedTemplate = selectedId
? ids.find((id) => id === selectedId)
: (ids.find((id) => id === "default") ?? ids[0]);
return selectedTemplate ?? null;
}
function proposed(selectedId, ids) {
const selectedTemplate =
(selectedId ? ids.find((id) => id === selectedId) : undefined) ??
ids.find((id) => id === "default") ??
ids[0];
return selectedTemplate ?? null;
}
for (const [selectedId, ids, description] of [
["stale", ["default"], "stale id, stale id remains"],
["stale", [], "stale id, no templates"],
[null, ["default"], "no selected id, default exists"],
]) {
console.log(`${description}: current=${current(selectedId, ids)} proposed=${proposed(selectedId, ids)}`);
}
JSRepository: Pasta-Devs/Marinara-Engine
Length of output: 30538
Do not let stale storyboard template IDs abort scene illustration
The server loader accepts a chat-level storyboardImagePromptTemplateId while refreshing the template list from Agent Settings. If that id is missing from args.customTemplates, the strict lookup hits undefined and throws, killing storyboard illustration generation. Mirror the shared selector’s behavior and fall back to the default/first option; keep the throw only for the truly empty-template case.
🧬 Corrective procedure
const options = normalizeAgentPromptTemplateOptions(args.customTemplates).slice(0, 20);
const selectedId = readTrimmedString(args.templateId);
- const selectedTemplate = selectedId
- ? options.find((template) => template.id === selectedId)
- : (options.find((template) => template.id === GAME_STORYBOARD_IMAGE_PROMPT_TEMPLATE_ID) ?? options[0]);
+ const selectedTemplate =
+ (selectedId ? options.find((template) => template.id === selectedId) : undefined) ??
+ options.find((template) => template.id === GAME_STORYBOARD_IMAGE_PROMPT_TEMPLATE_ID) ??
+ options[0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const options = normalizeAgentPromptTemplateOptions(args.customTemplates).slice(0, 20); | |
| const selectedId = readTrimmedString(args.templateId); | |
| const selectedTemplate = selectedId ? options.find((template) => template.id === selectedId) : undefined; | |
| if (selectedTemplate) return renderSelectedStoryboardImagePrompt(selectedTemplate, args.ctx); | |
| const selectedTemplate = selectedId | |
| ? options.find((template) => template.id === selectedId) | |
| : (options.find((template) => template.id === GAME_STORYBOARD_IMAGE_PROMPT_TEMPLATE_ID) ?? options[0]); | |
| if (args.promptOverridesStorage) { | |
| const storedOverride = await loadStoredSceneIllustrationOverride(args.promptOverridesStorage, args.ctx); | |
| if (storedOverride) return storedOverride; | |
| if (!selectedTemplate) { | |
| throw new Error("The Storyboard Agent does not provide a usable image formatter prompt."); | |
| } | |
| const options = normalizeAgentPromptTemplateOptions(args.customTemplates).slice(0, 20); | |
| const selectedId = readTrimmedString(args.templateId); | |
| const selectedTemplate = | |
| (selectedId ? options.find((template) => template.id === selectedId) : undefined) ?? | |
| options.find((template) => template.id === GAME_STORYBOARD_IMAGE_PROMPT_TEMPLATE_ID) ?? | |
| options[0]; | |
| if (!selectedTemplate) { | |
| throw new Error("The Storyboard Agent does not provide a usable image formatter prompt."); | |
| } |
🤖 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/image/game-storyboard-image-prompt.ts` around
lines 25 - 33, Update the selectedTemplate resolution in the storyboard image
prompt loader to fall back to the default storyboard template or first available
option when selectedId is stale or absent from options, matching the shared
selector behavior. Keep the existing error throw only when no usable template
exists at all.
| function normalizeBoundedInteger(value: unknown, fallback: number, min: number, max: number): number { | ||
| const parsed = typeof value === "number" ? value : Number(value); | ||
| if (!Number.isFinite(parsed)) return fallback; | ||
| return Math.min(max, Math.max(min, Math.trunc(parsed))); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
null/"" collapse to min, not to fallback.
Number(null) and Number("") both evaluate to 0, which is finite, so the guard passes and the clamp yields min — keyframeCount becomes 1 instead of 3, animationDurationSeconds becomes 1 instead of 6. Note that the client-side twin, normalizeGameStoryboardKeyframeCount in packages/client/src/components/game/game-storyboard-ui.ts, explicitly guards value == null || value === "" and returns the default. Two normalizers for the same field disagreeing is precisely the sort of asymmetry that produces bug reports one cannot reproduce.
🔬 Proposed correction
function normalizeBoundedInteger(value: unknown, fallback: number, min: number, max: number): number {
+ if (value == null || value === "") return fallback;
const parsed = typeof value === "number" ? value : Number(value);
if (!Number.isFinite(parsed)) return fallback;
return Math.min(max, Math.max(min, Math.trunc(parsed)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function normalizeBoundedInteger(value: unknown, fallback: number, min: number, max: number): number { | |
| const parsed = typeof value === "number" ? value : Number(value); | |
| if (!Number.isFinite(parsed)) return fallback; | |
| return Math.min(max, Math.max(min, Math.trunc(parsed))); | |
| } | |
| function normalizeBoundedInteger(value: unknown, fallback: number, min: number, max: number): number { | |
| if (value == null || value === "") return fallback; | |
| const parsed = typeof value === "number" ? value : Number(value); | |
| if (!Number.isFinite(parsed)) return fallback; | |
| return Math.min(max, Math.max(min, Math.trunc(parsed))); | |
| } |
🤖 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/features/agents/storyboard-agent-settings.ts` around
lines 51 - 55, Update normalizeBoundedInteger to return fallback when value is
null, undefined, or an empty string before converting it with Number; preserve
the existing finite-number validation, truncation, and min/max clamping for all
other inputs, matching normalizeGameStoryboardKeyframeCount behavior.
1 participant
Linked issue
Closes #4113
Related package PR: Pasta-Devs/Marinara-Agents#108
This branch now includes canonical staging through
66fd75e2b, including the official #4112 merge, so Storyboard's Insert location path and the updated Hierarchical Maps Agent Editor remain intact.Why this change
What changed
GameSurface.tsx,ChatSettingsDrawer.tsx, and Storyboard settings split into lazy modules so no source module crosses Babel's 500 kB warning threshold.Validation
pnpm checkpasses locallyCONTRIBUTING.mdVerification notes
Automated and local runtime evidence:
pnpm lintpasses for shared, server, and client.pnpm localization:checkpasses.pnpm regression:promptpasses, including package ownership, merged connection/reference precedence, planner selection, and manual Game Illustrator controls.pnpm version:checkpasses for 2.3.5.pnpm buildpasses at Engine head2297fc723.ChatSettingsDrawer.tsxis 499,079 bytes and does not trigger Babel's 500 kB source warning.2.3.5+2297fc7238f7; Storyboard 1.0.0 and Hierarchical Maps 1.2.0 are bothactiveandready.pnpm checkstill stops at the known repository guard,Impeccable context loader did not return valid JSON.Direct lint, localization, regression, browser, version, and production-build validation are listed separately.Manual browser verification still required:
Docs and release impact
docs-i18nbranch updated to match, or a[docs-i18n]follow-up issue opened (see CONTRIBUTING.md § Translated documentation)UI evidence (if applicable)
No screenshots committed. Manual browser evidence remains listed above.
Summary by CodeRabbit