feat: Add resilience to provider API response handling#223
feat: Add resilience to provider API response handling#223
Conversation
- Add runtime validation for API responses in providers and config contexts
(isValidProviderListData, isValidConfig type guards replace unsafe casts)
- Guard provider.models with optional chaining (provider.models ?? {})
- Add ErrorBoundary around Settings page tab content with user-friendly
error fallback and retry button
- Handle race condition in connectProvider/completeOAuth by adding 500ms
delay between dispose() and refetchProviders()
Closes #220
There was a problem hiding this comment.
Pull request overview
This PR aims to make the frontend more resilient to backend API response shape changes by adding runtime validation/defensive access in provider + config contexts, and by adding an ErrorBoundary fallback on the Settings page to avoid blank-screen failures. It also introduces saved prompt scoping (global vs project) and associated UI/storage behavior.
Changes:
- Add runtime shape checks for provider list/auth responses and config responses; reduce unsafe casts and guard potentially-missing fields.
- Add a Settings-page
ErrorBoundarywith a user-facing fallback and “Try Again” button. - Introduce saved prompt scoping (global/project), including storage migration/dedup logic and UI changes across Settings/Session/Layout.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app-prefixable/src/context/providers.tsx | Adds runtime validation for provider/auth responses and a post-dispose delay before refetch. |
| app-prefixable/src/context/config.tsx | Adds minimal runtime validation for config responses (object/non-array). |
| app-prefixable/src/pages/settings.tsx | Wraps tab content in an ErrorBoundary; adds prompt scope selection and prompt list scope badges. |
| app-prefixable/src/context/saved-prompts.tsx | Introduces PromptScope, split global/project stores, migration tagging, and merged prompt view. |
| app-prefixable/src/pages/session.tsx | Adds prompt scope to picker grouping and to “save prompt” dialog behavior. |
| app-prefixable/src/pages/layout.tsx | Updates saved prompt dropdown to display scope badges and types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Something went wrong | ||
| </h3> | ||
| <p class="text-xs mt-1" style={{ color: "var(--text-weak)" }}> | ||
| {props.error.message || "An unexpected error occurred while rendering this section."} | ||
| </p> |
There was a problem hiding this comment.
SectionErrorFallback assumes the caught value is an Error and unconditionally reads .message. Solid's ErrorBoundary can receive non-Error throwables (e.g. strings, null), which would make the fallback itself throw when accessing .message. Consider accepting unknown and deriving a safe message via err instanceof Error ? err.message : String(err) (with a final fallback string).
There was a problem hiding this comment.
Addressed in 537a64a: fallback now handles unknown throwables safely via getThrowableMessage instead of assuming Error.message.
| <button | ||
| onClick={props.reset} | ||
| class="px-3 py-1.5 rounded-md text-xs font-medium transition-colors" |
There was a problem hiding this comment.
The "Try Again" action only calls reset(), which re-renders but does not trigger any provider/config refetch. If the error was caused by a failed fetch or invalid API response, this button is likely to do nothing. Consider wiring this to the relevant refresh/refetch functions (or placing section-specific error boundaries with fallbacks that can retry).
There was a problem hiding this comment.
Addressed in 537a64a: "Try Again" now runs providers.refetch() + config.refresh() via Promise.allSettled(...) before reset, so it actively retries data loading.
| function isValidProviderListData(data: unknown): data is ProviderListData { | ||
| if (!data || typeof data !== "object") return false | ||
| const d = data as Record<string, unknown> | ||
| return Array.isArray(d.all) && Array.isArray(d.connected) |
There was a problem hiding this comment.
isValidProviderListData only checks that all/connected are arrays, but the code later assumes each all entry is a non-null object with id and models. If all contains null/primitives due to an API change, data.all.map((provider) => ...) will still throw. Consider strengthening the guard (e.g., validate items are objects with id: string and models being an object) or filtering out invalid entries before mapping.
| function isValidProviderListData(data: unknown): data is ProviderListData { | |
| if (!data || typeof data !== "object") return false | |
| const d = data as Record<string, unknown> | |
| return Array.isArray(d.all) && Array.isArray(d.connected) | |
| function isValidProviderListEntry(provider: unknown): provider is { id: string; models: Record<string, unknown> } { | |
| if (!provider || typeof provider !== "object") return false | |
| const p = provider as Record<string, unknown> | |
| return typeof p.id === "string" && !!p.models && typeof p.models === "object" && !Array.isArray(p.models) | |
| } | |
| function isValidProviderListData(data: unknown): data is ProviderListData { | |
| if (!data || typeof data !== "object") return false | |
| const d = data as Record<string, unknown> | |
| return Array.isArray(d.all) && d.all.every(isValidProviderListEntry) && Array.isArray(d.connected) |
There was a problem hiding this comment.
Addressed in 537a64a: provider-list validation was strengthened with entry normalization/validation (including provider/model field checks) before mapping, so malformed entries no longer crash mapping.
| const data = res.data | ||
| if (!isValidProviderListData(data)) { | ||
| console.error("[Providers] Invalid provider list response shape:", data) | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
On invalid provider list responses, the resource returns undefined after logging. This prevents crashes but also makes the UI silently look like "no providers" and the new ErrorBoundary fallback won't be reached. Consider propagating an explicit error state (e.g., throw an Error from the resource or expose an error signal in the providers context) so Settings can present a user-visible message and retry action.
There was a problem hiding this comment.
Addressed in 537a64a/ef5930c: provider errors are now surfaced via explicit provider error state for Settings handling; consumers keep safe defaults rather than silently treating malformed payloads as empty success.
| // Dispose instance to reload provider state, then wait for the | ||
| // server to reinitialize before refetching the provider list. | ||
| await client.instance.dispose() | ||
| await new Promise((resolve) => setTimeout(resolve, 500)) | ||
| await refetchProviders() |
There was a problem hiding this comment.
Using a fixed 500ms delay after dispose() is brittle: on slower machines or heavier restarts the server may still be unavailable, and refetch will fail intermittently. Prefer waiting for a concrete readiness signal (e.g. the existing server.connected event pattern used in config.tsx) or retrying refetchProviders() with backoff until it succeeds or times out.
There was a problem hiding this comment.
Addressed in 537a64a: replaced fixed 500ms wait with bounded retry/backoff (refetchProvidersWithRetry) after dispose() in connect flow.
| // Dispose instance to reload provider state, then wait for the | ||
| // server to reinitialize before refetching the provider list. | ||
| await client.instance.dispose() | ||
| await new Promise((resolve) => setTimeout(resolve, 500)) | ||
| await refetchProviders() |
There was a problem hiding this comment.
Using a fixed 500ms delay after dispose() is brittle for the same reason as in connectProvider(). Prefer waiting for a readiness signal (server.connected) or adding retry/backoff around the provider list refetch to avoid intermittent failures.
There was a problem hiding this comment.
Addressed in 537a64a: complete OAuth path now also uses bounded retry/backoff (refetchProvidersWithRetry) instead of fixed sleep.
| function mergePrompts(global: SavedPrompt[], project: SavedPrompt[]): SavedPrompt[] { | ||
| const projectIds = new Set(project.map((p) => p.id)) | ||
| const dedupedGlobal = global.filter((p) => !projectIds.has(p.id)) | ||
| return [...dedupedGlobal, ...project].sort(sortNewest) | ||
| } |
There was a problem hiding this comment.
mergePrompts() always sorts by createdAt, which means reorder(ids) cannot affect the visible order (any reordering is immediately overridden by the sort in the memo). Either remove reorder from the context API if ordering is always "newest first", or change the merge logic so it preserves the persisted per-store ordering.
There was a problem hiding this comment.
Stale after scope cleanup in 4181c35. app-prefixable/src/context/saved-prompts.tsx is no longer part of this PR diff.
| const [editingPromptId, setEditingPromptId] = createSignal<string | null>(null) | ||
| const [promptTitle, setPromptTitle] = createSignal("") | ||
| const [promptText, setPromptText] = createSignal("") | ||
| const [promptScope, setPromptScope] = createSignal<PromptScope>("global") | ||
| const [promptToDelete, setPromptToDelete] = createSignal<string | null>(null) |
There was a problem hiding this comment.
This PR introduces prompt scoping (global/project), new UI controls, and a storage/migration strategy for saved prompts across multiple pages/contexts. That functionality isn't mentioned in the PR title/description or Issue #220 scope (provider/config API resilience). If intentional, please update the PR description/title (or split into a separate PR) so reviewers can evaluate the additional feature and its migration implications explicitly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (providerData.error) throw providerData.error | ||
| const disabled = cfg.global.disabled_providers ?? [] | ||
| return (providerData()?.all ?? []).filter((p) => !disabled.includes(p.id)) | ||
| }, | ||
| get connected() { | ||
| if (providerData.error) throw providerData.error |
There was a problem hiding this comment.
The providers/connected getters throw when providerData.error is set. Several pages (e.g. Session/Layout) access providers.providers and providers.connected without any surrounding ErrorBoundary, so a transient or shape-mismatch error from client.provider.list() will crash the whole route (returning to a blank page) instead of degrading gracefully. Prefer returning safe defaults (e.g. []) and exposing the error via providerError, or add an app-wide ErrorBoundary that covers all consumers of this context (not just Settings).
| if (providerData.error) throw providerData.error | |
| const disabled = cfg.global.disabled_providers ?? [] | |
| return (providerData()?.all ?? []).filter((p) => !disabled.includes(p.id)) | |
| }, | |
| get connected() { | |
| if (providerData.error) throw providerData.error | |
| const disabled = cfg.global.disabled_providers ?? [] | |
| return (providerData()?.all ?? []).filter((p) => !disabled.includes(p.id)) | |
| }, | |
| get connected() { |
There was a problem hiding this comment.
Addressed in ef5930c: removed throws from providers/connected getters to avoid route crashes in consumers without ErrorBoundary; safe defaults are preserved.
| const parsed = JSON.parse(existing) | ||
| if (Array.isArray(parsed)) { | ||
| let needsWrite = false | ||
| for (const p of parsed) { | ||
| if (!p.scope) { | ||
| p.scope = "project" | ||
| needsWrite = true | ||
| } | ||
| } | ||
| if (needsWrite) localStorage.setItem(pKey, JSON.stringify(parsed)) |
There was a problem hiding this comment.
migrateIfNeeded mutates each parsed entry via p.scope = ... without checking that p is actually a non-null object. If localStorage contains an array with non-object elements (corrupt/older data), assigning a property on a primitive will throw in module strict mode and abort the migration early. Guard with an object check (e.g. if (p && typeof p === "object" && !("scope" in p)) ...) before mutating.
There was a problem hiding this comment.
Stale after scope cleanup in 4181c35. saved-prompts.tsx is no longer in the current PR diff.
| if (props.onRetry) await props.onRetry() | ||
| props.reset() |
There was a problem hiding this comment.
In the ErrorBoundary fallback, the click handler awaits onRetry() before calling reset(). If onRetry throws/rejects, reset() will never run, leaving the section stuck in the error UI even if the next render would succeed. Consider wrapping the retry in try/finally (or catching errors) so reset() always executes when the user clicks “Try Again”.
| if (props.onRetry) await props.onRetry() | |
| props.reset() | |
| try { | |
| if (props.onRetry) await props.onRetry() | |
| } finally { | |
| props.reset() | |
| } |
There was a problem hiding this comment.
Addressed in ef5930c: retry handler now uses try/finally so reset() always executes even if retry fails.
| const res = await sdk.client.config.update({ config: patch }) | ||
| const data = res.data as Config | undefined | ||
| if (data) { | ||
| const data = res.data | ||
| if (data && !isValidConfig(data)) { | ||
| console.error("[Config] Unexpected project update response shape:", data) | ||
| return null | ||
| } |
There was a problem hiding this comment.
When config.updateProject / config.updateGlobal receive a non-null response that fails isValidConfig, they currently just console.error(...) and return null while leaving error() cleared. Call sites (e.g. Settings) only show UI feedback via config.error() / saveError(), so this becomes a silent save failure. Consider setting setError(...) (or throwing) on invalid response shapes so the user gets a visible error state.
There was a problem hiding this comment.
Addressed in ef5930c: invalid non-object project update responses now set a user-visible config error before returning null.
| const res = await sdk.client.global.config.update({ config: safePatch }) | ||
| const data = res.data as Config | undefined | ||
| if (data) { | ||
| const data = res.data | ||
| if (data && !isValidConfig(data)) { | ||
| console.error("[Config] Unexpected global update response shape:", data) | ||
| return null | ||
| } |
There was a problem hiding this comment.
Same as above for the global config save: if sdk.client.global.config.update(...) returns a non-object payload, the function returns null without setting error(), so the UI has no indication that the save failed. Setting an error message (or throwing) on invalid response shapes would make this failure mode user-visible.
There was a problem hiding this comment.
Addressed in ef5930c: invalid non-object global update responses now set a user-visible config error before returning null.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await client.instance.dispose() | ||
| await refetchProviders() | ||
| return true | ||
| return refetchProvidersWithRetry(providerID) |
There was a problem hiding this comment.
connectProvider returns the refetchProvidersWithRetry() promise without awaiting it. If refetchProvidersWithRetry rejects (e.g., the provider list fetch throws due to invalid response shape), that rejection will bypass this function’s try/catch and bubble up to callers even though connectProvider is typed/expected to resolve false on failure. Await the call (or catch inside refetchProvidersWithRetry) so errors are handled consistently and connectProvider reliably returns a boolean.
| return refetchProvidersWithRetry(providerID) | |
| return await refetchProvidersWithRetry(providerID) |
There was a problem hiding this comment.
Fixed in 9337314: connectProvider now uses return await refetchProvidersWithRetry(providerID) so rejections are handled by the surrounding try/catch and the function reliably returns a boolean.
| async function completeOAuth(providerID: string, methodIndex: number, code?: string): Promise<boolean> { | ||
| try { | ||
| await client.provider.oauth.callback({ | ||
| providerID, | ||
| method: methodIndex, | ||
| code, | ||
| }) | ||
| // Dispose instance to reload provider state, then refresh | ||
| // Dispose instance to reload provider state, then wait for the | ||
| // server to reinitialize and provider state to be observable. | ||
| await client.instance.dispose() | ||
| await refetchProviders() | ||
| return true | ||
| return refetchProvidersWithRetry(providerID) | ||
| } catch (e) { |
There was a problem hiding this comment.
Same issue as connectProvider: completeOAuth returns the refetchProvidersWithRetry() promise without awaiting it, so any rejection from the retry/refetch path will not be caught by this try/catch and can surface as an unhandled rejection. Await the retry call (or handle errors inside the retry loop) to ensure completeOAuth returns false on failure as intended.
There was a problem hiding this comment.
Fixed in 9337314 as well: completeOAuth now awaits refetchProvidersWithRetry(providerID) so failures are caught and the function returns false on error.
| const raw = localStorage.getItem(GLOBAL_KEY) | ||
| if (raw) { | ||
| const parsed = JSON.parse(raw) | ||
| if (Array.isArray(parsed)) { | ||
| let needsWrite = false | ||
| for (const p of parsed) { | ||
| if (!p.scope) { | ||
| p.scope = "global" | ||
| needsWrite = true | ||
| } | ||
| } | ||
| if (needsWrite) localStorage.setItem(GLOBAL_KEY, JSON.stringify(parsed)) |
There was a problem hiding this comment.
Same migration robustness issue for the global store backfill: the loop assumes each parsed entry is an object and sets p.scope without checking. Add a per-entry object check before accessing/mutating scope to avoid TypeErrors on corrupted localStorage data.
There was a problem hiding this comment.
This is stale from an earlier revision. app-prefixable/src/context/saved-prompts.tsx is no longer part of this PR diff after scope cleanup, so this migration-loop comment no longer applies to the current change set.
| const ok = await providers.completeOAuth(pending.providerID, pending.methodIndex, code) | ||
|
|
||
| setConnecting(false) | ||
|
|
||
| if (ok) { | ||
| setSuccess(`Connected to ${pending.providerName}!`) | ||
| setOauthPending(null) | ||
| setOauthCode("") | ||
| setSelectedProvider(null) | ||
| setProviderSearch("") | ||
| } else { | ||
| setError("Failed to complete authentication. Please try again.") | ||
| } | ||
| } | ||
|
|
||
| function cancelOAuth() { | ||
| setOauthPending(null) | ||
| setOauthCode("") | ||
| setCodeCopied(false) | ||
| setConnecting(false) | ||
| } | ||
|
|
||
| async function copyCode() { | ||
| const pending = oauthPending() | ||
| if (!pending?.code) return | ||
| try { | ||
| await navigator.clipboard.writeText(pending.code) | ||
| setCodeCopied(true) | ||
| setTimeout(() => setCodeCopied(false), 2000) | ||
| } catch (e) { | ||
| console.error("Failed to copy code:", e) | ||
| } | ||
| } | ||
|
|
||
| function getProviderDisplayName(id: string): string { | ||
| const provider = providers.providers.find((p) => p.id === id) | ||
| return provider?.name ?? id | ||
| } | ||
|
|
||
| async function confirmMcpDelete() { | ||
| const name = mcpToDelete() | ||
| if (!name) return | ||
| setMcpToDelete(null) | ||
| setMcpDeleting(name) | ||
| try { | ||
| await mcp.remove(name) | ||
| } catch (e) { | ||
| console.error("[Settings] Failed to remove MCP server:", e) | ||
| } finally { | ||
| setMcpDeleting(null) | ||
| } | ||
| } | ||
|
|
||
| function openAddPromptDialog() { | ||
| setEditingPromptId(null) | ||
| setPromptTitle("") | ||
| setPromptText("") | ||
| setPromptScope(savedPrompts.hasProject() ? "project" : "global") | ||
| setPromptDialogOpen(true) | ||
| } | ||
|
|
||
| function openEditPromptDialog(id: string) { | ||
| const prompt = savedPrompts.prompts().find((p) => p.id === id) | ||
| if (!prompt) return | ||
| setEditingPromptId(id) | ||
| setPromptTitle(prompt.title) | ||
| setPromptText(prompt.text) | ||
| setPromptScope(prompt.scope) | ||
| setPromptDialogOpen(true) | ||
| } | ||
|
|
||
| function savePromptDialog() { | ||
| const title = promptTitle().trim() | ||
| const text = promptText().trim() | ||
| if (!title || !text) return | ||
| const editing = editingPromptId() | ||
| if (editing) { | ||
| savedPrompts.update(editing, { title, text }) | ||
| const existing = savedPrompts.prompts().find((p) => p.id === editing) | ||
| if (existing && existing.scope !== promptScope()) { | ||
| // Scope changed — move across stores while preserving id/createdAt | ||
| savedPrompts.move(editing, promptScope()) | ||
| } | ||
| if (existing && (existing.title !== title || existing.text !== text)) { | ||
| savedPrompts.update(editing, { title, text }) | ||
| } | ||
| if (!existing) { | ||
| savedPrompts.add(title, text, promptScope()) | ||
| } | ||
| } else { | ||
| savedPrompts.add(title, text) | ||
| savedPrompts.add(title, text, promptScope()) | ||
| } | ||
| setPromptDialogOpen(false) | ||
| setEditingPromptId(null) | ||
| setPromptTitle("") | ||
| setPromptText("") | ||
| } | ||
|
|
||
| function confirmPromptDelete() { | ||
| const id = promptToDelete() | ||
| if (!id) return | ||
| savedPrompts.remove(id) | ||
| setPromptToDelete(null) | ||
| } | ||
|
|
||
| // Scope badge type for each tab | ||
| type ScopeBadge = "Global" | "Project" | "Global + Project" | null | ||
|
|
||
| const tabs = createMemo(() => { | ||
| const base: Array<{ id: string; label: string; icon: () => JSX.Element; scope: ScopeBadge }> = [ | ||
| { id: "providers", label: "Providers", icon: () => <Plug class="w-4 h-4" />, scope: "Global" }, | ||
| { id: "servers", label: "Servers", icon: () => <Globe class="w-4 h-4" />, scope: "Global" }, | ||
| { id: "git", label: "Git", icon: () => <GitBranch class="w-4 h-4" />, scope: "Global" }, | ||
| { id: "mcp", label: "MCP Servers", icon: () => <Server class="w-4 h-4" />, scope: "Global + Project" }, | ||
| { id: "prompts", label: "Prompts", icon: () => <BookmarkPlus class="w-4 h-4" />, scope: directory ? "Project" : null }, | ||
| { id: "prompts", label: "Prompts", icon: () => <BookmarkPlus class="w-4 h-4" />, scope: directory ? "Global + Project" : "Global" }, |
There was a problem hiding this comment.
This PR introduces a separate feature beyond provider/config resilience: saved prompts are now split into global vs project scope (new PromptScope, storage keys, UI changes, move/reorder semantics). The PR description/title/issue #220 summary doesn’t mention this behavioral change. Please either update the PR description (and linked issue scope) to include this feature and its migration implications, or move the prompts-scope work into a separate PR to keep the change focused.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type PromptScope = "global" | "project" | ||
|
|
||
| export interface SavedPrompt { | ||
| id: string | ||
| title: string | ||
| text: string | ||
| createdAt: number | ||
| scope: PromptScope | ||
| } |
There was a problem hiding this comment.
This PR introduces a saved-prompts scoping feature (PromptScope / global vs project storage) that is unrelated to the stated goal of provider/config API resilience in the PR title/description and issue #220. Please either update the PR description (and linked issue scope, if appropriate) to explicitly cover this behavior/migration, or split the prompt-scoping work into a separate PR to keep changes focused.
| {/* Content */} | ||
| <div class="flex-1 overflow-y-auto"> | ||
| <div class="max-w-2xl p-6 space-y-6"> | ||
| <ErrorBoundary fallback={(err, reset) => <SectionErrorFallback error={err} reset={reset} onRetry={retrySettingsSection} />}> |
There was a problem hiding this comment.
Wrapping the page in ErrorBoundary will only catch errors thrown during rendering; it will not automatically surface async resource failures (e.g., provider/config fetch errors stored in providers.providerError or config.error()). As-is, a failed provider list normalization can still present as an empty state with no explanation. Consider explicitly rendering these error states (or re-throwing them during render) so users see a meaningful message and the retry button is actionable.
| <ErrorBoundary fallback={(err, reset) => <SectionErrorFallback error={err} reset={reset} onRetry={retrySettingsSection} />}> | |
| <ErrorBoundary fallback={(err, reset) => <SectionErrorFallback error={err} reset={reset} onRetry={retrySettingsSection} />}> | |
| {(() => { | |
| const asyncSettingsError = providers.providerError ?? config.error() | |
| if (asyncSettingsError) throw asyncSettingsError | |
| return null | |
| })()} |
There was a problem hiding this comment.
Addressed in b8c2f35: Settings now derives async context errors from providers.providerError and config.error() and throws during render inside the existing ErrorBoundary, making fallback + retry actionable for async failures.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Saved prompts | ||
| const savedPrompts = useSavedPrompts() | ||
| const [promptDialogOpen, setPromptDialogOpen] = createSignal(false) | ||
| const [editingPromptId, setEditingPromptId] = createSignal<string | null>(null) | ||
| const [promptTitle, setPromptTitle] = createSignal("") | ||
| const [promptText, setPromptText] = createSignal("") | ||
| const [promptScope, setPromptScope] = createSignal<PromptScope>("global") |
There was a problem hiding this comment.
This change set introduces a new saved-prompts scoping feature (PromptScope global/project), updates Settings/Session/Layout UI to display & select scope, and overhauls saved-prompts storage/migration. This is a significant behavioral change but it is not mentioned in the PR title/description or in Issue #220 (which is about provider/config API resilience). Please either (1) update the PR description/title to explicitly cover the prompts-scoping feature and its migration semantics, or (2) split the prompts work into a separate PR to keep the resilience fix focused.
| const raw = localStorage.getItem(GLOBAL_KEY) | ||
| if (raw) { | ||
| const parsed = JSON.parse(raw) | ||
| if (Array.isArray(parsed)) { | ||
| let needsWrite = false | ||
| for (const p of parsed) { | ||
| if (!p.scope) { | ||
| p.scope = "global" | ||
| needsWrite = true | ||
| } | ||
| } | ||
| if (needsWrite) localStorage.setItem(GLOBAL_KEY, JSON.stringify(parsed)) | ||
| } |
There was a problem hiding this comment.
Same issue for the global-store backfill: the loop assumes each parsed entry is an object and writes p.scope. With corrupted localStorage data this can throw and prevent the scope backfill. Guard each element before accessing/mutating scope.
There was a problem hiding this comment.
Stale for the current diff. app-prefixable/src/context/saved-prompts.tsx is no longer part of this PR after scope cleanup, so this migration-loop concern is out of scope here.
| * (artefact of the old migration that copied everything). When duplicate | ||
| * IDs are found, keep the project-scoped copy and drop the global one from | ||
| * the merged view — the global store itself is left untouched. | ||
| */ | ||
| function mergePrompts(global: SavedPrompt[], project: SavedPrompt[]): SavedPrompt[] { | ||
| const projectIds = new Set(project.map((p) => p.id)) | ||
| const dedupedGlobal = global.filter((p) => !projectIds.has(p.id)) |
There was a problem hiding this comment.
mergePrompts hides duplicate IDs by dropping the global copy when a project copy exists, but it intentionally leaves the global store untouched. This can lead to confusing behavior: if a prompt was duplicated by the old migration (same id in both stores) and the user deletes the visible (project) copy, the hidden global duplicate will reappear in the merged view. Consider cleaning duplicates during migration (e.g., removing global entries whose ids exist in the project store) or updating remove/update to also apply to hidden duplicates so deleted/edited prompts don’t resurrect from the other store.
| * (artefact of the old migration that copied everything). When duplicate | |
| * IDs are found, keep the project-scoped copy and drop the global one from | |
| * the merged view — the global store itself is left untouched. | |
| */ | |
| function mergePrompts(global: SavedPrompt[], project: SavedPrompt[]): SavedPrompt[] { | |
| const projectIds = new Set(project.map((p) => p.id)) | |
| const dedupedGlobal = global.filter((p) => !projectIds.has(p.id)) | |
| * (artefact of the old migration that copied everything). When duplicate | |
| * IDs are found, keep the project-scoped copy and clean the hidden global | |
| * duplicate from storage so it cannot reappear later after edits/removals. | |
| */ | |
| function mergePrompts(global: SavedPrompt[], project: SavedPrompt[]): SavedPrompt[] { | |
| const projectIds = new Set(project.map((p) => p.id)) | |
| const dedupedGlobal = global.filter((p) => !projectIds.has(p.id)) | |
| if (dedupedGlobal.length !== global.length) { | |
| saveToStorage(GLOBAL_KEY, dedupedGlobal) | |
| } |
There was a problem hiding this comment.
Also stale/out of scope for this PR revision: saved-prompts.tsx changes were removed in 4181c35. We can track this duplicate-resurrection behavior separately in a dedicated prompts issue/PR if needed.
Summary
astype casts with proper type guards (isValidProviderListData,isValidConfig) that check expected fields before usedata.all.map()andprovider.modelswith optional chaining/nullish coalescing to prevent crashes on unexpected response shapesErrorBoundary(SolidJS built-in) around Settings page tab content with a user-friendly error fallback showing the error message and a "Try Again" buttonconnectProviderandcompleteOAuthbetweendispose()andrefetchProviders()by adding a 500ms delay for the server to reinitializeTesting
bun run buildinapp-prefixable/)Closes #220