feat: persist default History API provider, selectable in Admin UI#2837
Open
dirkwa wants to merge 7 commits into
Open
feat: persist default History API provider, selectable in Admin UI#2837dirkwa wants to merge 7 commits into
dirkwa wants to merge 7 commits into
Conversation
The default history provider was first-come-first-served: whichever plugin registered first (e.g. KIP) became the default, and the choice made via POST _providers/_default was lost on restart. Persist the selection in settings (historyApi.defaultProvider) and resolve the default dynamically: the configured provider whenever it is registered, falling back to the first registered provider when it is not (e.g. its plugin is disabled). This makes the default independent of plugin load order and stable across enable/disable. Add a History Provider card to Data -> Preferences in the Admin UI with a pull-down of the registered providers. closes SignalK#2835
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe History API now persists a configured default provider, falls back when unavailable, and reports provider availability. The admin UI adds provider selection, warnings, and sidebar badges, with tests covering registration, fallback, persistence, and notifications. ChangesHistory provider default selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant HistoryApiHttpRegistry
participant Settings
AdminUI->>HistoryApiHttpRegistry: Submit selected default provider
HistoryApiHttpRegistry->>Settings: Persist historyApi.defaultProvider
Settings-->>HistoryApiHttpRegistry: Return save result
HistoryApiHttpRegistry-->>AdminUI: Return effective and configured provider IDs
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
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 `@packages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsx`:
- Around line 25-49: Update the HistoryProviderSettings loading flow around
fetchProviders to track an error for both non-OK responses and caught fetch
failures, rather than silently returning or only logging. Render a danger Alert
when loading fails while providers remains unavailable, ensuring the card
displays actionable feedback instead of disappearing.
- Around line 95-119: Update the Default Provider Form.Group and Form.Select in
the provider selector block to use a matching shared controlId/id, associating
the label with the select for assistive technology while preserving the existing
selection behavior.
- Around line 61-64: Update the URL construction in registerHistoryApiProvider
to encode the provider id before interpolating it into the path, preserving the
intended endpoint for IDs containing slashes, fragments, query characters, or
spaces.
In `@src/api/history/index.ts`:
- Around line 177-191: Update the provider-selection flow around
configuredProviderId and saveConfiguredProvider so the requested ID is passed
into persistence, the write uses an immutable settings snapshot, and runtime
state is committed only from the successful callback. Keep failed writes from
changing the active provider, including under concurrent POSTs, and add a
regression test covering the write-failure case.
In `@test/history-api.ts`:
- Around line 239-259: Replace the any-based fixtures in provider, makeApp, and
defaultOf with HistoryApplication or a narrow test-app type that exposes the
required configuration and getHistoryApi contract. Make provider.getValues
return a valid ValuesResponse object directly rather than casting the name
string through unknown, while preserving the existing provider-selection
assertions.
🪄 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
Run ID: 9df07d2b-8687-433b-bee3-1c58f8c75209
📒 Files selected for processing (7)
packages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsxpackages/server-admin-ui/src/views/ServerConfig/PreferencesPage.tsxsrc/api/history/index.tssrc/api/history/openApi.tssrc/api/index.tssrc/config/config.tstest/history-api.ts
Comment on lines
+177
to
+191
| this.configuredProviderId = req.params.id | ||
| this.saveConfiguredProvider((err) => { | ||
| if (err) { | ||
| res.status(500).json({ | ||
| statusCode: 500, | ||
| state: 'FAILED', | ||
| message: `Failed to save settings: ${err.message}` | ||
| }) | ||
| } else { | ||
| res.status(200).json({ | ||
| statusCode: 200, | ||
| state: 'COMPLETED', | ||
| message: `Default provider set to ${req.params.id}.` | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Commit the provider selection only after persistence succeeds.
The code updates both configuredProviderId and app.config.settings before the write completes. On failure, the API returns 500 but the failed selection remains active in memory; concurrent POSTs can also leave runtime state different from disk.
Pass the requested ID into saveConfiguredProvider, write an immutable settings snapshot, and update runtime state only in the successful callback. Add a write-failure regression test.
Proposed fix
- this.configuredProviderId = req.params.id
- this.saveConfiguredProvider((err) => {
+ const providerId = req.params.id
+ this.saveConfiguredProvider(providerId, (err) => {
if (err) {
res.status(500).json({
statusCode: 500,
state: 'FAILED',
message: `Failed to save settings: ${err.message}`
})
} else {
+ this.configuredProviderId = providerId
+ this.app.config.settings.historyApi = {
+ ...this.app.config.settings.historyApi,
+ defaultProvider: providerId
+ }
res.status(200).json({
statusCode: 200,
state: 'COMPLETED',
- message: `Default provider set to ${req.params.id}.`
+ message: `Default provider set to ${providerId}.`
})
}
})- private saveConfiguredProvider(cb: (err?: Error) => void) {
- const settings = this.app.config.settings
- settings.historyApi = {
- ...settings.historyApi,
- defaultProvider: this.configuredProviderId
+ private saveConfiguredProvider(
+ providerId: string,
+ cb: (err?: Error) => void
+ ) {
+ const settings = {
+ ...this.app.config.settings,
+ historyApi: {
+ ...this.app.config.settings.historyApi,
+ defaultProvider: providerId
+ }
}
writeSettingsFile(this.app, settings, cb)
}Also applies to: 247-253
🤖 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/api/history/index.ts` around lines 177 - 191, Update the
provider-selection flow around configuredProviderId and saveConfiguredProvider
so the requested ID is passed into persistence, the write uses an immutable
settings snapshot, and runtime state is committed only from the successful
callback. Keep failed writes from changing the active provider, including under
concurrent POSTs, and add a regression test covering the write-failure case.
added 2 commits
July 15, 2026 12:20
Address review findings: associate the Default Provider label with the select via controlId, URL-encode the provider id in the POST path, and name the saved-message timeout constant.
Address review findings: a failed settings write no longer switches the active provider or leaves the in-memory settings mutated (with a regression test); the Preferences card shows an error instead of disappearing when loading providers fails; the provider-selection test fixtures are typed instead of any-based.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsx`:
- Around line 151-155: Replace the inline margin styles in
HistoryProviderSettings.tsx at lines 151-155, 164-168, and 172-176 with
className="mt-2 mb-0"; at lines 109-113, replace style={{ marginBottom: 0 }}
with className="mb-0".
- Around line 131-143: Add a hidden, disabled fallback option to the Form.Select
in the provider dropdown for the empty value used when providers.defaultId is
missing, while preserving the existing providers.ids options and selection
behavior.
- Around line 156-157: Update the unavailable-provider message in the
HistoryProviderSettings JSX so the opening quote, providers.configuredId, and
closing quote remain on one source line, preserving the intended spacing and
wording.
In `@src/api/history/index.ts`:
- Around line 246-264: Update saveConfiguredProvider to build an immutable
settings snapshot without mutating app.config.settings before writeSettingsFile
completes; assign the snapshot to app.config.settings only on success and
preserve the committed runtime state on failure. Verify callers do not depend on
the old settings object being mutated in place, and add a regression test
covering overlapping saves with one failure and one success, asserting settings
and configuredProviderId remain consistent.
🪄 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
Run ID: 9a2039ab-137f-4415-b2b3-539213e2e4df
📒 Files selected for processing (3)
packages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsxsrc/api/history/index.tstest/history-api.ts
Comment on lines
+156
to
+157
| The configured default provider "{providers.configuredId} | ||
| " is not currently available |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Keep quotes and provider ID on the same line to avoid spacing glitches.
Spanning text and HTML entities across multiple lines can cause JSX parsers to trim whitespace unexpectedly, resulting in missing spaces before or after the quotes. Keeping them on a single line guarantees predictable spacing and improves readability.
🛠️ Proposed fix
- The configured default provider "{providers.configuredId}
- " is not currently available
+ The configured default provider "{providers.configuredId}" is not currently available📝 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.
Suggested change
| The configured default provider "{providers.configuredId} | |
| " is not currently available | |
| The configured default provider "{providers.configuredId}" is not currently available |
🤖 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-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsx`
around lines 156 - 157, Update the unavailable-provider message in the
HistoryProviderSettings JSX so the opening quote, providers.configuredId, and
closing quote remain on one source line, preserving the intended spacing and
wording.
tkurki
reviewed
Jul 15, 2026
tkurki
left a comment
tkurki
left a comment
Member
There was a problem hiding this comment.
Nice!
Since this is about plugin configuration could we add this on the plugin config page instead? A bit tricky, but maybe at the top and show only when there are >1 providers?
If the configured provider is not available when needed we could send a warn notification.
Address review feedback: - Move the default provider selector from Data -> Preferences to the top of the plugin configuration page, shown only when there is more than one registered provider (or the configured one is unavailable). - Raise a warn notification (notifications.server.history .defaultProvider) the first time a request needs the default while the configured provider is unavailable; clear it when the provider registers again. Show a warning badge on Apps & Plugins -> Configuration in the sidebar for the same condition. - Persist the default via an immutable settings snapshot, committing in-memory state only after the write succeeds. - Add a placeholder option when no default is set.
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 `@packages/server-admin-ui/src/hooks/useHistoryProviderStatus.ts`:
- Around line 1-50: Extract the shared provider-fetching logic from
useHistoryProviderUnavailable and HistoryProviderSettings.tsx into a reusable
useHistoryProviders hook returning the provider IDs plus normalized
default/configured IDs, including null handling. Update both consumers to use
this hook instead of duplicating fetch/parse logic, and reuse the existing
HistoryProviders type rather than the local Record<string, unknown> declaration.
In `@src/api/history/index.ts`:
- Around line 254-274: Update saveConfiguredProvider so that after a successful
settings write commits configuredProviderId, it also calls
notifyConfiguredAvailable(). Keep this call inside the !err branch after the new
provider is committed, preserving the existing callback behavior and relying on
the helper’s no-op behavior when no warning is active.
🪄 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
Run ID: e72a0ea1-d0fa-48e3-a5b7-99f52aed5645
📒 Files selected for processing (6)
packages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/hooks/useHistoryProviderStatus.tspackages/server-admin-ui/src/views/Configuration/Configuration.tsxpackages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsxsrc/api/history/index.tstest/history-api.ts
Switching the default to a registered provider now clears an active unavailable-provider warning, so the stale warn state can no longer suppress a later warning for the new provider (with regression test). Extract the provider fetch shared by the settings card and the sidebar badge into a single useHistoryProviders hook.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
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 `@packages/server-admin-ui/src/hooks/useHistoryProviderStatus.ts`:
- Around line 4-8: Export PROVIDERS_PATH from useHistoryProviderStatus.ts as the
shared endpoint constant. In
packages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsx:14,
import and reuse PROVIDERS_PATH from the hook instead of declaring the literal
locally.
🪄 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
Run ID: 802936fe-01b1-478e-a79f-7c2c3c9ff4d6
📒 Files selected for processing (4)
packages/server-admin-ui/src/hooks/useHistoryProviderStatus.tspackages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsxsrc/api/history/index.tstest/history-api.ts
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
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 (2)
packages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsx (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the error response body instead of letting
anyflow through.
res.json()returnsPromise<any>;body.messageis accessed without a type. As per coding guidelines, strict type checking should avoidany-equivalent escape hatches — the sibling hook (useHistoryProviderStatus.ts) already types its JSON responses this way.♻️ Proposed fix
- const body = await res.json() + const body = (await res.json()) as { message?: string } setSaveError(body.message || `Save failed (HTTP ${res.status})`)🤖 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-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsx` around lines 38 - 39, Type the JSON result in the save-error handling flow around res.json() and body.message, following the response type pattern used by useHistoryProviderStatus.ts. Ensure the response type includes the optional message field so accessing body.message does not allow an implicit any.Source: Coding guidelines
test/history-api.ts (1)
455-509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared boilerplate between this test and the "clears a stale warning" test (Lines 392-453).
Both tests stub
config.writeSettingsFile, castmakeApp(...)the same way, wire uppostHandlers, and build an identicalresstub — only the stub's success/failure behavior and final assertions differ. A small shared factory would remove the duplication for future scenarios.♻️ Sketch of a shared helper
function withMockPostApp( writeSettingsFile: (id: string, cb: (err?: Error) => void) => void ) { const config = require('../dist/config/config') const orig = config.writeSettingsFile config.writeSettingsFile = (_app: unknown, settings: unknown, cb: (err?: Error) => void) => writeSettingsFile((settings as { historyApi?: { defaultProvider?: string } }).historyApi?.defaultProvider ?? '', cb) return { restore: () => (config.writeSettingsFile = orig) } }🤖 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 `@test/history-api.ts` around lines 455 - 509, Extract the duplicated setup shared by the “clears a stale warning” test and “does not change the active provider when persisting fails” test into a small helper near those tests. The helper should stub and restore config.writeSettingsFile, create and configure the makeApp instance, register POST handlers, and provide the shared response stub; keep each test’s distinct persistence outcome and assertions in the individual test bodies.
🤖 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/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsx`:
- Around line 38-39: Type the JSON result in the save-error handling flow around
res.json() and body.message, following the response type pattern used by
useHistoryProviderStatus.ts. Ensure the response type includes the optional
message field so accessing body.message does not allow an implicit any.
In `@test/history-api.ts`:
- Around line 455-509: Extract the duplicated setup shared by the “clears a
stale warning” test and “does not change the active provider when persisting
fails” test into a small helper near those tests. The helper should stub and
restore config.writeSettingsFile, create and configure the makeApp instance,
register POST handlers, and provide the shared response stub; keep each test’s
distinct persistence outcome and assertions in the individual test bodies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bee6a021-c919-46bc-9588-ab632f7fe2c2
📒 Files selected for processing (4)
packages/server-admin-ui/src/hooks/useHistoryProviderStatus.tspackages/server-admin-ui/src/views/ServerConfig/HistoryProviderSettings.tsxsrc/api/history/index.tstest/history-api.ts
Extract the writeSettingsFile stub, router-capturing app and POST helper shared by the default-provider route tests into withDefaultProviderRoute; type the save-error response body.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
Ready for human review. |
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
2 participants
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.
The default history provider is first-come-first-served: whichever plugin registers first (e.g. KIP) becomes the default, and a choice made via
POST /_providers/_defaultis lost on restart. A provider registering later (e.g. QuestDB) cannot be selected persistently.The selection is now persisted in server settings (
historyApi.defaultProvider) and resolved dynamically: the configured provider serves as default whenever it is registered, with the first registered provider as fallback when it is not (e.g. its plugin is disabled). This makes the default independent of plugin load order and stable across plugin enable/disable and server restarts.The Admin UI gets a History Provider card under Data → Preferences with a pull-down of the registered providers. A warning is shown when the configured provider is currently unavailable.
GET /_providers/_defaultadditionally reports the persisted choice asconfigured.closes #2835
Tested: added unit tests for order independence and fallback plus endpoint tests for set/persist. I also verified manually against a running server with two test history-provider plugins plus KIP: set the default via the UI, restarted, and the default survived load order; disabling the configured provider's plugin falls back to another provider with a warning shown in the UI.
Summary
This PR adds a persisted default History API provider configuration.
historyApi.defaultProviderand resolves the effective default dynamically: it uses the configured provider when available, otherwise it falls back to the first registered provider (independent of plugin load order).GET /_providers/_defaultto return both the effectiveidand the persistedconfiguredprovider id, and updatesPOST /_providers/_default/:idto persist the configured default via settings (returning an error and not changing in-memory state when settings persistence fails).useHistoryProviders/useHistoryProviderUnavailable.