Normalize boolish persisted config flags#17
Conversation
📝 WalkthroughWalkthroughThis PR standardizes boolean-flag interpretation using a shared ChangesBoolean Configuration Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
• Findings
The patch looks internally consistent: it normalizes legacy truthy string values and booleans on the connection/default paths it touches, and the preset default UI now matches the storage shape it already writes. Validation
Residual risk
|
Promansis
changed the title
|
• Findings
The patch looks internally consistent: it normalizes legacy truthy string values and booleans on the connection/default paths it touches, and the preset default UI now matches the storage shape it already writes. Validation
Residual risk
|
|
The bug fix here is valid: persisted config flags can come back as string-ish values like The issue is the implementation shape. This PR copies the same local helper into many files: function isStoredBooleanTrue(value: unknown): boolean {
return value === true || value === "true" || value === "1";
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/presets/components/PresetsPanel.tsx`:
- Line 281: The badge logic only checks preset.isDefault while useDefaultPreset
uses both isDefault and default; update the badge determination in PresetsPanel
(the const isDefault assignment) to consider both fields consistently (e.g., use
the same OR/NULL-coalescing logic used in useDefaultPreset) by replacing the
single-field check with a combined check that passes the resulting value into
boolish so presets with default: true also render the default badge.
🪄 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: 0b8e5dae-2c37-4e93-a5f6-79d248d40c6c
📒 Files selected for processing (12)
src/engine/generation/context.tssrc/engine/modes/chat/core/summaries/auto-summary.service.tssrc/engine/modes/chat/schedules/schedule.service.tssrc/engine/modes/roleplay/scene/scene-service.tssrc/features/chats/components/ChatSettingsDrawer.tsxsrc/features/chats/components/QuickConnectionSwitcher.tsxsrc/features/chats/components/QuickSwitcherMobile.tsxsrc/features/chats/hooks/use-chats.tssrc/features/connections/components/ConnectionsPanel.tsxsrc/features/presets/components/PresetsPanel.tsxsrc/features/presets/hooks/use-presets.tsupdates/people/promansis.md
| const sectionCount = getSectionCount(preset); | ||
| const wrapFormat = (preset.wrapFormat ?? "xml") as string; | ||
| const isDefault = preset.isDefault === "true"; | ||
| const isDefault = boolish(preset.isDefault, false); |
There was a problem hiding this comment.
Ah, a fascinating divergence in experimental protocol! The badge rendering examines only one specimen field.
The default-badge logic checks solely preset.isDefault, yet your useDefaultPreset hook in use-presets.ts (lines 129-130) examines both isDefault and default fields with OR logic. Should a preset possess default: true whilst isDefault remains falsy, the hook shall identify it as the default preset—yet the badge will fail to materialize!
This creates observable inconsistency: a preset may be selected as default but display no badge, contradicting the stated research objective: "normalize prompt preset default reads so boolean defaults show the default badge and are found by default preset lookup."
⚗️ Proposed remedy to synchronize badge logic with lookup logic
- const isDefault = boolish(preset.isDefault, false);
+ const isDefault = boolish(preset.isDefault, false) || boolish((preset as any).default, false);📝 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 isDefault = boolish(preset.isDefault, false); | |
| const isDefault = boolish(preset.isDefault, false) || boolish((preset as any).default, false); |
🤖 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 `@src/features/presets/components/PresetsPanel.tsx` at line 281, The badge
logic only checks preset.isDefault while useDefaultPreset uses both isDefault
and default; update the badge determination in PresetsPanel (the const isDefault
assignment) to consider both fields consistently (e.g., use the same
OR/NULL-coalescing logic used in useDefaultPreset) by replacing the single-field
check with a combined check that passes the resulting value into boolish so
presets with default: true also render the default badge.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/presets/components/PresetsPanel.tsx`:
- Around line 281-283: Replace the undefined isStoredBooleanTrue usage in
PresetsPanel with the imported boolish helper: instead of calling
isStoredBooleanTrue(preset.isDefault ?? ...), call boolish(preset.isDefault ||
(preset as PresetRow & { default?: unknown }).default) so both preset.isDefault
and preset.default are evaluated (using OR to let an explicit false fall through
to the other field) and avoid duplicating engine logic; remove the
isStoredBooleanTrue reference and use boolish(preset.isDefault || (preset as
PresetRow & { default?: unknown }).default) in the PresetsPanel rendering logic.
🪄 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: ea922550-9041-42e4-9333-2e284228cb78
📒 Files selected for processing (1)
src/features/presets/components/PresetsPanel.tsx
| const isDefault = isStoredBooleanTrue( | ||
| preset.isDefault ?? (preset as PresetRow & { default?: unknown }).default, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the undefined function and locate the boolish helper.
# Check if isStoredBooleanTrue is defined anywhere in the file
echo "=== Checking for isStoredBooleanTrue definition in PresetsPanel ==="
rg -n "function isStoredBooleanTrue|const isStoredBooleanTrue" src/features/presets/components/PresetsPanel.tsx
# Locate the boolish helper implementation
echo "=== Locating boolish helper implementation ==="
rg -n -A 5 "export.*function boolish|export.*const boolish" src/engine/generation/runtime-records.ts
# Check for other files using isStoredBooleanTrue (potential duplication)
echo "=== Searching for isStoredBooleanTrue across the codebase ==="
rg -l "function isStoredBooleanTrue|const isStoredBooleanTrue" src/Repository: Pasta-Devs/Marinara-Engine-Refactor
Length of output: 568
🏁 Script executed:
# Check the import statement at line 16
head -20 src/features/presets/components/PresetsPanel.tsx | tail -10Repository: Pasta-Devs/Marinara-Engine-Refactor
Length of output: 546
🏁 Script executed:
# Read lines 275-290 to see the exact code at lines 281-283 with context
sed -n '275,290p' src/features/presets/components/PresetsPanel.tsxRepository: Pasta-Devs/Marinara-Engine-Refactor
Length of output: 914
🏁 Script executed:
# Verify boolish is properly accessible and check its full signature
sed -n '45,60p' src/engine/generation/runtime-records.tsRepository: Pasta-Devs/Marinara-Engine-Refactor
Length of output: 684
Fix critical runtime error: isStoredBooleanTrue is undefined. This function does not exist anywhere in the codebase and will cause a ReferenceError when the component renders. The boolish helper is already imported from the engine at line 16 but sits unused—employ it directly to satisfy both the runtime requirement and the coding guideline demanding that React components not duplicate engine rules but instead call feature APIs or engine use cases through proper adapters.
The current nullish coalescing approach (preset.isDefault ?? preset.default) only evaluates the first non-nullish value, potentially missing the case where preset.isDefault is explicitly false but preset.default is true. Implement OR-based logic to evaluate both fields consistently:
Corrected implementation
- const isDefault = isStoredBooleanTrue(
- preset.isDefault ?? (preset as PresetRow & { default?: unknown }).default,
- );
+ const isDefault = boolish(preset.isDefault) || boolish((preset as PresetRow & { default?: unknown }).default);🤖 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 `@src/features/presets/components/PresetsPanel.tsx` around lines 281 - 283,
Replace the undefined isStoredBooleanTrue usage in PresetsPanel with the
imported boolish helper: instead of calling isStoredBooleanTrue(preset.isDefault
?? ...), call boolish(preset.isDefault || (preset as PresetRow & { default?:
unknown }).default) so both preset.isDefault and preset.default are evaluated
(using OR to let an explicit false fall through to the other field) and avoid
duplicating engine logic; remove the isStoredBooleanTrue reference and use
boolish(preset.isDefault || (preset as PresetRow & { default?: unknown
}).default) in the PresetsPanel rendering logic.
2 participants
Summary
Root Cause
Some persisted configuration readers only treated string
"true"as enabled, while current writers and engine contracts use booleantrue. That made UI state and resolver state diverge for random-pool membership and prompt preset defaults.Validation
pnpm typecheckgit diff --checkuseForRandom,isDefault, anddefaultequality checks insrc/featuresandsrc/engine.Summary by CodeRabbit