Skip to content

fix(settings): persist model allowlist as JSON array#518

Merged
cita-777 merged 1 commit intocita-777:mainfrom
kevinye13:fix/settings-allowlist-json-array
May 8, 2026
Merged

fix(settings): persist model allowlist as JSON array#518
cita-777 merged 1 commit intocita-777:mainfrom
kevinye13:fix/settings-allowlist-json-array

Conversation

@kevinye13
Copy link
Copy Markdown
Contributor

@kevinye13 kevinye13 commented Apr 27, 2026

Summary

  • Fix double JSON encoding when persisting global_allowed_models.
  • Apply the same fix to global_blocked_brands, which uses the same settings persistence path.
  • Add hydration compatibility for legacy double-encoded list settings so existing bad values are parsed back into arrays on restart.
  • Add regression coverage for both persistence and restart hydration behavior.

Problem

Related to #301, which introduced the global model allowlist feature. This PR fixes a persistence encoding issue in that feature's runtime settings save path.

Runtime settings are persisted through upsertSetting, which already serializes values with JSON.stringify.

The model allowlist save path pre-serialized arrays before passing them to upsertSetting, so the stored value became a JSON string containing an array instead of a JSON array. After export/import or restart hydration, this could be interpreted as a malformed model name list.

Example bad persisted value:

"[\"model-alpha\",\"model-beta\"]"

Expected persisted value:

["model-alpha","model-beta"]

Changes

  • Pass normalized arrays directly to upsertSetting for:
    • global_allowed_models
    • global_blocked_brands
  • Extend runtime settings hydration so legacy double-encoded array strings are parsed as arrays before falling back to comma-separated string handling.
  • Add focused tests that verify:
    • runtime settings persistence stores JSON arrays, not double-encoded strings
    • legacy double-encoded model allowlist values hydrate back into normalized arrays

Validation

npm test -- src/server/routes/api/settings.events.test.ts src/server/runtimeSettingsHydration.test.ts

Result:

Test Files  2 passed
Tests       39 passed

Also verified with a local Docker deployment:

  • rebuilt the image from this commit
  • saved and reloaded runtime settings through the API
  • confirmed the container SQLite settings.value stores global_allowed_models as a JSON array
  • restarted the container and confirmed /api/settings/runtime still returns an array

Risk

Low. The change is limited to runtime settings persistence and hydration for list values. Existing valid JSON arrays continue to work, and legacy double-encoded arrays are now accepted for compatibility.

Summary by CodeRabbit

  • Bug Fixes

    • Improved normalization of global blocked brands and allowed models settings by removing duplicates and trimming whitespace entries.
    • Enhanced support for legacy runtime settings formats.
  • Tests

    • Added test coverage for runtime settings normalization and validation.

@github-actions github-actions Bot added area: server Server-side API and backend changes size: S 50 to 199 lines changed labels Apr 27, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 481ffe8d-847c-4e0f-a066-77b85dd8bbe6

📥 Commits

Reviewing files that changed from the base of the PR and between 262651a and a53c628.

📒 Files selected for processing (4)
  • src/server/routes/api/settings.events.test.ts
  • src/server/routes/api/settings.ts
  • src/server/runtimeSettingsHydration.test.ts
  • src/server/runtimeSettingsHydration.ts

📝 Walkthrough

Walkthrough

This PR refines the runtime settings system for global model and brand lists by updating persistence to store arrays directly, enhancing the hydration parser to handle JSON-encoded values and legacy double-encoded formats, and adding corresponding test coverage for normalization and database persistence behavior.

Changes

Cohort / File(s) Summary
Test Coverage
src/server/routes/api/settings.events.test.ts, src/server/runtimeSettingsHydration.test.ts
Added tests for list normalization with duplicate/whitespace handling, settings API updates, and legacy double-encoded JSON parsing in hydration.
Runtime Settings Hydration
src/server/runtimeSettingsHydration.ts
Enhanced toStringList to parse JSON-encoded array inputs, extract and trim entries, then fall back to comma-splitting for non-JSON strings.
Settings API Persistence
src/server/routes/api/settings.ts
Updated upsertSetting payloads for global_blocked_brands and global_allowed_models to persist arrays directly instead of JSON-stringified values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

area: server, size: M

Poem

🐰 Arrays now rest in proper form,
No JSON strings to confuse the norm,
We trim the whitespace, deduplicate with care,
Legacy formats? We parse them fair,
Settings hydrate clean and spare! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: fixing persistence of model allowlist as JSON array instead of double-encoded strings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cita-777 cita-777 merged commit 508fa71 into cita-777:main May 8, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: server Server-side API and backend changes size: S 50 to 199 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants