feat(history): replace backends.json path hints with history-sources.json - #84
Conversation
…json Removes historyPathHints/cliHistoryPathHints from BackendDefinitionRecord and BackendSummary. Introduces a dedicated history-sources.json config file with read/write helpers and adds GET /api/history-sources + PATCH /api/history-sources/:provider routes. Updates the settings UI with a new History Sources section. Closes #77.
There was a problem hiding this comment.
Pull request overview
This PR splits history path-hint configuration out of backends.json into a dedicated history-sources.json, exposing it via new backend routes and a new Settings UI section so backend connection settings and history discovery settings are managed independently.
Changes:
- Remove
historyPathHints/cliHistoryPathHintsfrom backend config and backend summaries, and route history discovery hint lookups throughhistory-sources.json. - Add
GET /api/history-sourcesandPATCH /api/history-sources/:providerendpoints backed by a newbackend/src/history/sources-config.tsmodule. - Update the Settings UI + tests to edit history source paths per provider in a separate “History Sources” section.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/routes/settings.tsx | Adds History Sources section and card UI; removes history-hint editing from backend cards. |
| frontend/src/router.test.tsx | Updates router test fetch mocks/assertions for the new history-sources UI. |
| frontend/src/hooks/useBackendSettings.ts | Introduces useHistorySources() hook for loading/saving history-sources.json. |
| backend/src/routes/agents.ts | Adds /history-sources API routes alongside existing agent/backend routes. |
| backend/src/routes/agents.test.ts | Adds integration tests for history-sources routes and updates backend stubs. |
| backend/src/history/sources-config.ts | New config module for reading/writing/updating history-sources.json and providing hint lookups. |
| backend/src/history/sources-config.test.ts | Unit tests for the new history-sources config module. |
| backend/src/agents/types.ts | Adds HistorySourceConfig API shape; removes history hint fields from BackendSummary. |
| backend/src/agents/registry.ts | Switches session/history discovery to use getHistoryHintsForProvider() lookups instead of backend config fields. |
| backend/src/agents/registry.test.ts | Updates mocks/expectations for hint lookups now sourced from history-sources.json. |
| backend/src/agents/config.ts | Removes history hint fields from backend config records and updates legacy Copilot backend migration notes/logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Migrate old split Copilot backend records (copilot-cli-wsl, copilot-cli-host, | ||
| * copilot-vscode-host, copilot-vscode-wsl) into a single `copilot` backend. | ||
| * Any user-customized command/args from the CLI backends are preserved. | ||
| * All historyPathHints are merged into `historyPathHints` (VS Code roots). | ||
| * Paths that look like CLI session-state directories (absolute paths containing | ||
| * '.copilot' but not 'workspaceStorage') are also placed in `cliHistoryPathHints`. | ||
| * History path hints previously stored on these records are no longer part of | ||
| * `BackendDefinitionRecord` — they live in `history-sources.json` now. | ||
| * Non-Copilot backends are unchanged. | ||
| * Does not delete unknown custom user backends. | ||
| */ |
There was a problem hiding this comment.
readBackendConfig()/migrateLegacyCopilotBackends() no longer migrates legacy historyPathHints/cliHistoryPathHints out of existing backends.json. Because normalizeBackendRecord() now drops those fields and history-sources.json is initialized to empty defaults, upgrading from an older config will silently lose user-configured history roots. Consider extracting legacy hint fields from the raw backend config file (before normalization) and writing them into history-sources.json (only if that file is missing or still default), mapping gemini-cli→gemini etc., so existing installs keep their configured history paths.
| export function readHistorySourcesConfig(): HistorySourceRecord[] { | ||
| ensureHistorySourcesConfigExists() | ||
| const file = readHistorySourcesConfigFile() | ||
| const configured = file.sources | ||
|
|
||
| if (!configured || configured.length === 0) { | ||
| return DEFAULT_SOURCES | ||
| } | ||
|
|
||
| return configured.map(normalizeHistorySourceRecord) |
There was a problem hiding this comment.
readHistorySourcesConfig() returns only the configured sources entries and does not merge in defaults for missing providers. This means a partially-populated history-sources.json (or an older file after adding a new provider) will cause the API/UI to omit providers entirely. Consider merging DEFAULT_SOURCES with configured records by provider (configured values win) so the API always returns a stable, complete set of providers.
| export function readHistorySourcesConfig(): HistorySourceRecord[] { | |
| ensureHistorySourcesConfigExists() | |
| const file = readHistorySourcesConfigFile() | |
| const configured = file.sources | |
| if (!configured || configured.length === 0) { | |
| return DEFAULT_SOURCES | |
| } | |
| return configured.map(normalizeHistorySourceRecord) | |
| function mergeHistorySourcesWithDefaults(sources?: HistorySourceRecord[]): HistorySourceRecord[] { | |
| if (!sources || sources.length === 0) { | |
| return DEFAULT_SOURCES.map(normalizeHistorySourceRecord) | |
| } | |
| const configuredByProvider = new Map<HistoryProvider, HistorySourceRecord>( | |
| sources | |
| .map(normalizeHistorySourceRecord) | |
| .map((source) => [source.provider, source] as const) | |
| ) | |
| return DEFAULT_SOURCES.map((source) => configuredByProvider.get(source.provider) ?? normalizeHistorySourceRecord(source)) | |
| } | |
| export function readHistorySourcesConfig(): HistorySourceRecord[] { | |
| ensureHistorySourcesConfigExists() | |
| const file = readHistorySourcesConfigFile() | |
| return mergeHistorySourcesWithDefaults(file.sources) |
| // Only include cliPaths for copilot; include it (even empty) when present so | ||
| // the key is persisted. | ||
| if (record.provider === 'copilot' || record.cliPaths !== undefined) { | ||
| normalized.cliPaths = Array.isArray(record.cliPaths) | ||
| ? record.cliPaths.filter((p): p is string => typeof p === 'string') | ||
| : [] | ||
| } |
There was a problem hiding this comment.
normalizeHistorySourceRecord() currently includes cliPaths for non-copilot providers whenever the input record has cliPaths defined (record.provider === 'copilot' || record.cliPaths !== undefined). This contradicts the comment and the intended schema (“only meaningful for copilot”), and allows /api/history-sources/:provider to persist cliPaths for gemini/opencode. Consider restricting cliPaths normalization/persistence to record.provider === 'copilot' only (and ignoring cliPaths for other providers).
| } | ||
|
|
||
| const updated = (await response.json()) as HistorySourceConfig | ||
| setSources((current) => current.map((s) => (s.provider === provider ? updated : s))) |
There was a problem hiding this comment.
saveSource() updates local state with current.map(...), which will be a no-op if the provider isn’t already present in sources (e.g., if the backend starts returning additional providers or if the config file is missing an entry). Consider upserting: replace when found, otherwise append the returned record so the UI stays in sync with the backend response.
| setSources((current) => current.map((s) => (s.provider === provider ? updated : s))) | |
| setSources((current) => { | |
| const exists = current.some((s) => s.provider === provider) | |
| return exists | |
| ? current.map((s) => (s.provider === provider ? updated : s)) | |
| : [...current, updated] | |
| }) |
| const body = await c.req.json<{ | ||
| paths?: string[] | ||
| cliPaths?: string[] | ||
| }>() | ||
|
|
||
| try { |
There was a problem hiding this comment.
In PATCH /history-sources/:provider, await c.req.json() is executed outside the try/catch. If the client sends invalid JSON (or no body with a JSON Content-Type), this will throw and likely surface as a 500 instead of a 400. Consider wrapping the JSON parse in the existing try/catch (or handling parse errors explicitly) and returning a 400 with a clear error message.
| const body = await c.req.json<{ | |
| paths?: string[] | |
| cliPaths?: string[] | |
| }>() | |
| try { | |
| try { | |
| const body = await c.req.json<{ | |
| paths?: string[] | |
| cliPaths?: string[] | |
| }>() |
| describe('getHistoryHintsForProvider', () => { | ||
| it('returns empty arrays for unknown provider', () => { | ||
| readHistorySourcesConfig() // init defaults | ||
|
|
||
| const result = getHistoryHintsForProvider('gemini') | ||
| expect(result).toEqual({ historyPathHints: [], cliHistoryPathHints: [] }) | ||
| }) |
There was a problem hiding this comment.
Test name is misleading: it says “unknown provider” but calls getHistoryHintsForProvider('gemini'), which is a known provider in the type and defaults. Consider renaming the test to reflect the actual behavior being validated (e.g., “returns empty arrays when no paths are configured”).
- readHistorySourcesConfig: merge configured sources with DEFAULT_SOURCES by provider so partial files always return a complete provider set - normalizeHistorySourceRecord: restrict cliPaths to copilot provider only - PATCH /history-sources/:provider: move JSON parse inside try/catch so malformed request bodies return 400 instead of 500 - readBackendConfig: migrate legacy historyPathHints/cliHistoryPathHints from backends.json into history-sources.json on first upgrade; make BACKEND_CONFIG_PATH lazy (getBackendConfigPath()) to support test isolation - useHistorySources saveSource: upsert provider in local state rather than map-only so a new provider returned by the server is kept in sync - Rename misleading test 'returns empty arrays for unknown provider' to 'returns empty arrays when no paths are configured for a provider' - Add tests for mergeWithDefaults, cliPaths stripping, and legacy migration
Summary
historyPathHints/cliHistoryPathHintsfromBackendDefinitionRecordandBackendSummary; backends.json now only stores agent registry databackend/src/history/sources-config.tswith a dedicatedhistory-sources.jsonconfig file,GET /api/history-sourcesandPATCH /api/history-sources/:providerroutesCloses #77