feat(settings): persist extension runtime settings - #637
Open
SantiagoDePolonia wants to merge 3 commits into
Open
feat(settings): persist extension runtime settings#637SantiagoDePolonia wants to merge 3 commits into
SantiagoDePolonia wants to merge 3 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds extension-defined runtime settings. It introduces registry support, SQL and MongoDB persistence, synchronized validation and updates, admin API routes, application wiring, and a dashboard settings interface. ChangesRuntime settings
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant AdminHandler
participant RuntimeSettingsService
participant RuntimeSettingsStore
Dashboard->>AdminHandler: Request runtime setting descriptors
AdminHandler->>RuntimeSettingsService: List settings
RuntimeSettingsService-->>AdminHandler: Ordered descriptors
AdminHandler-->>Dashboard: Settings response
Dashboard->>AdminHandler: Update setting key and value
AdminHandler->>RuntimeSettingsService: Validate and apply update
RuntimeSettingsService->>RuntimeSettingsStore: Persist value
RuntimeSettingsStore-->>RuntimeSettingsService: Save result
RuntimeSettingsService-->>AdminHandler: Updated descriptor
AdminHandler-->>Dashboard: Update response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Confidence Score: 4/5
 T-Rex LogsWhat T-Rex did
Comments Outside Diff (1)
Reviews (1): Last reviewed commit: "feat(settings): persist extension runtim..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+101
to
+104
| if err := setting.Apply(value); err != nil { | ||
| return ext.SettingDescriptor{}, fmt.Errorf("%w: %v", ErrInvalid, err) | ||
| } | ||
| if err := s.store.Set(ctx, key, setting.Descriptor().Value); err != nil { |
There was a problem hiding this comment.
Update invokes Apply only on the RuntimeSetting held by the process that received the admin request, then persists the selected value. Already-running peers sharing the same PostgreSQL or MongoDB backend do not reload the stored value and have no invalidation or notification path, so they continue serving the old live setting until restart. In a multi-instance deployment, requests can therefore receive different behavior depending on which instance handles them.
Contributor
Author
There was a problem hiding this comment.
Fixed in c6e06a9. Runtime settings now reconcile from the shared backend every two seconds on each instance, outside the request path. Reconciliation compares the stored and live values before Apply, so unchanged values do not reset compaction epochs. Added a two-service shared-SQLite convergence test and deduplicated repeated sync errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/admin/handler_runtime_settings_test.go`:
- Around line 60-99: Add test cases alongside TestRuntimeSettingsListAndUpdate
and TestRuntimeSettingManagedByEnvironmentIsReadOnly covering unknown-key PUTs
returning 404 with runtime_setting_not_found, invalid option values returning
400, and GET/PUT requests on a handler without WithRuntimeSettings returning the
feature-unavailable response. Reuse the existing Echo route setup and runtime
settings test helpers, and verify the setting remains unchanged where
applicable.
In `@internal/runtimesettings/service_test.go`:
- Around line 48-104: Add a test alongside TestServicePersistsAndRestoresSetting
that uses a stub Store whose Set method returns an error, then calls
Service.Update and verifies the setting’s value is restored to its previous
state via Apply. Assert the update returns the store error while preserving the
existing locked and persistence test coverage.
In `@internal/runtimesettings/service.go`:
- Around line 31-72: Reject non-locked settings with empty Options during
registration in Service.New, returning a startup error immediately after the
descriptor.Locked check; update ext/ext.go documentation for
SettingDescriptor/RuntimeSetting to state that Options must include every
accepted value and that empty Options makes an unlocked setting non-editable.
Apply the service.go change at lines 31-72 and the documentation change in
ext/ext.go at lines 84-113.
- Around line 87-111: Update the Service.Update method to add the same
nil-receiver guard used by List and Close before accessing s.mu or other fields.
Return the established nil-service error consistently, while preserving the
existing update behavior for non-nil receivers.
In `@web/dashboard/src/pages/settings/RuntimeSettings.svelte`:
- Around line 37-38: Update the select disabled-state logic in the settings UI
so every select is disabled whenever any save is in flight, not only when
savingKey matches that setting. Keep locked settings disabled as before, and
ensure the save guard in save remains consistent with this global in-flight
behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: cf009c00-ad9d-474f-8eb7-4cc190dd07f7
⛔ Files ignored due to path filters (4)
internal/admin/dashboard/static/dist/assets/index-6sHfhgfb.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-B-Rv4AUL.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-B_MQg975.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (16)
ext/ext.goext/registry.goext/registry_test.gointernal/admin/handler.gointernal/admin/handler_runtime_settings.gointernal/admin/handler_runtime_settings_test.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/app/app.gointernal/runtimesettings/service.gointernal/runtimesettings/service_test.gointernal/runtimesettings/store.gointernal/runtimesettings/store_mongodb.gointernal/runtimesettings/store_sql.goweb/dashboard/src/pages/settings/RuntimeSettings.svelteweb/dashboard/src/pages/settings/SettingsPage.svelte
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/runtimesettings/store_mongodb_test.go`:
- Around line 11-29: Extend TestMongoDBStoreRoundTrip with a nil-database call
to NewMongoDBStore and assert that it returns an error, covering the
constructor’s nil-database rejection while preserving the existing
mongotest-backed round-trip checks.
In `@internal/runtimesettings/sync.go`:
- Around line 39-71: Update Service.sync to continue iterating through all keys
after store.Get or setting.Apply fails, while recording each failure and
returning an aggregate error after the loop. Preserve the existing invalid-value
handling and successful application behavior, and include each failure’s setting
key and underlying error in the aggregate.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 004930c0-6783-4be5-8804-c045f4be5749
⛔ Files ignored due to path filters (2)
internal/admin/dashboard/static/dist/assets/index-D6HB9TkL.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (11)
ext/ext.gointernal/admin/handler_runtime_settings.gointernal/admin/handler_runtime_settings_test.gointernal/runtimesettings/service.gointernal/runtimesettings/service_test.gointernal/runtimesettings/store.gointernal/runtimesettings/store_mongodb.gointernal/runtimesettings/store_mongodb_test.gointernal/runtimesettings/store_sql.gointernal/runtimesettings/sync.goweb/dashboard/src/pages/settings/RuntimeSettings.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
GoModel Pro needs a deployment-wide prompt-compression level that can change at runtime without putting Pro-specific behavior into open core or browser-local storage.
Verification
go test ./...npm testnpm run checknpm run buildSummary by CodeRabbit