From 244ce99550f56b3d763489b69f26fda94f9e3b84 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 16:23:43 +0000 Subject: [PATCH] commit the provider list as soon as its own fetch settles, not after four slower reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CoS Schedule page showed a provider picker with a single "Default (active provider)" option for seconds after load, which reads as a broken control rather than a slow one. Two causes, both fixed here: `/providers` is a cache-only read that returns in milliseconds, but ChiefOfStaff bundled it into a five-way `Promise.all` and only committed the result after the slowest sibling settled — and `getCosActionableInsights` runs a server-side PM2/memory health check plus a cross-repo git scan. Providers now commit on their own settle, mirroring the `healthRead` split four lines above. `applyProviders` is the single write path (as `applyHealth` is for health), so the settle flag can't be skipped by a future writer, and a `sameJsonShape` guard keeps the array identity stable when the payload is unchanged — otherwise the earlier commit would cost a full-tree re-render on every 30s poll. An empty `providers` array is also ambiguous: "still fetching" and "none configured" both render a picker whose only choice is the caller's `emptyProviderOption`. `ProviderModelSelector` gains a `loading` prop that disables its selects and says "Loading providers…" instead — the same settle-gate it already applies to its own tool-use capability scan. Wired through the Schedule tab: the card's quick pins and all three pickers in the task config drawer, so no picker sits enabled-but-empty beside a disabled one. Remaining pickers (Tasks/Agents/Workflow tabs, and the ~15 that fetch through useProviderModels, which already returns a `loading` flag nobody reads) are tracked in #6087, along with the same head-of-line block on `getApps`. --- .../src/components/ProviderModelSelector.jsx | 22 +++++++++-- .../components/ProviderModelSelector.test.jsx | 29 +++++++++++++++ client/src/components/cos/AppProviderPin.jsx | 3 ++ .../src/components/cos/tabs/ScheduleTab.jsx | 4 +- .../cos/tabs/schedule/AppOverrideRow.jsx | 3 +- .../cos/tabs/schedule/AppTaskCard.jsx | 4 +- .../cos/tabs/schedule/AppTaskCard.test.jsx | 9 +++++ .../cos/tabs/schedule/AppTaskTypeSection.jsx | 3 +- .../tabs/schedule/GlobalConfigControls.jsx | 10 +++-- .../cos/tabs/schedule/PerAppOverrideList.jsx | 3 +- .../cos/tabs/schedule/PipelineStageConfig.jsx | 3 +- .../cos/tabs/schedule/TaskConfigDrawer.jsx | 4 ++ .../tabs/schedule/TaskModelQuickControls.jsx | 3 +- client/src/pages/ChiefOfStaff.jsx | 37 ++++++++++++++++--- 14 files changed, 115 insertions(+), 22 deletions(-) diff --git a/client/src/components/ProviderModelSelector.jsx b/client/src/components/ProviderModelSelector.jsx index d62c1cc714..f65314cae8 100644 --- a/client/src/components/ProviderModelSelector.jsx +++ b/client/src/components/ProviderModelSelector.jsx @@ -23,6 +23,14 @@ * @param {function} props.onModelChange - Called with model string * @param {string} [props.label] - Label text (default: "Provider") * @param {boolean} [props.disabled] - Disable both selectors + * @param {boolean} [props.loading] - The caller's provider list hasn't settled + * yet. An empty `providers` is ambiguous — "still fetching" and "none + * configured" both render a picker whose only choice is the + * `emptyProviderOption` ("Default (active provider)", "Inherit (…)"), which + * reads as a broken control rather than a slow one. Pass `true` while the + * fetch is in flight to disable the selects and say so instead. This is the + * same settle-gate the `annotateToolUse` scan below uses on its own fetch, + * applied to the list the caller owns. * @param {boolean} [props.modelDisabled] - Disable only the model selector (e.g. * when the selected provider has no models). Composes with `disabled`. * @param {boolean} [props.compact] - Hide labels for inline/toolbar use @@ -95,6 +103,7 @@ export default function ProviderModelSelector({ onModelChange, label = 'Provider', disabled = false, + loading = false, modelDisabled = false, compact = false, emptyProviderOption, @@ -200,12 +209,17 @@ export default function ProviderModelSelector({ id={providerSelectId} value={selectedProviderId} onChange={(e) => onProviderChange(e.target.value)} - disabled={disabled} + disabled={disabled || loading} title={compact ? label : undefined} aria-label={compact ? label : undefined} className={SELECT_CLASS} > - {emptyProviderOption != null && } + {/* Rendered even when the caller forces a selection: mid-fetch there + is nothing else to offer, and a genuinely empty select reads as the + same broken control. */} + {loading + ? + : emptyProviderOption != null && } {visibleProviders.map((p) => { const hardwareUnavailable = !isProviderHardwareCompatible(p); const policyDisallowed = Boolean(providerAllowed && !providerAllowed(p)); @@ -227,7 +241,7 @@ export default function ProviderModelSelector({ id={modelSelectId} value={selectedModel} onChange={(e) => handleModelChange(e.target.value)} - disabled={disabled || modelDisabled} + disabled={disabled || modelDisabled || loading} title={compact ? 'Model' : undefined} aria-label={compact ? 'Model' : undefined} className={SELECT_CLASS} @@ -271,7 +285,7 @@ export default function ProviderModelSelector({ model={effectiveModel} value={effort || ''} onChange={onEffortChange} - disabled={disabled} + disabled={disabled || loading} optionFilter={effortAllowed} className={SELECT_CLASS} /> diff --git a/client/src/components/ProviderModelSelector.test.jsx b/client/src/components/ProviderModelSelector.test.jsx index b3d6fd3e94..b7fa1e2e18 100644 --- a/client/src/components/ProviderModelSelector.test.jsx +++ b/client/src/components/ProviderModelSelector.test.jsx @@ -406,3 +406,32 @@ describe('ProviderModelSelector', () => { }); }); }); + +describe('ProviderModelSelector — provider list still loading', () => { + it('names the in-flight fetch instead of offering the empty sentinel as the only choice', () => { + // Mid-fetch `providers` is [], so "Default (active provider)" would be the + // select's only option — a slow control that reads as a broken one. + renderSelector({ providers: [], selectedProviderId: '', emptyProviderOption: 'Default (active provider)', loading: true }); + const provider = screen.getByLabelText('Provider'); + expect([...provider.options].map((o) => o.textContent)).toEqual(['Loading providers…']); + expect(provider.disabled).toBe(true); + }); + + it('says so even when the caller forces a selection, since there is nothing else to show', () => { + renderSelector({ providers: [], selectedProviderId: '', loading: true }); + expect(screen.getByRole('option', { name: 'Loading providers…' })).toBeTruthy(); + }); + + it('disables the model select too, so a pin cannot be retargeted against a list that has not arrived', () => { + renderSelector({ loading: true }); + expect(screen.getByLabelText('Model').disabled).toBe(true); + }); + + it('restores the caller\'s own sentinel once the list settles', () => { + renderSelector({ selectedProviderId: '', emptyProviderOption: 'Default (active provider)' }); + const provider = screen.getByLabelText('Provider'); + expect(screen.getByRole('option', { name: 'Default (active provider)' })).toBeTruthy(); + expect(screen.queryByRole('option', { name: 'Loading providers…' })).toBeNull(); + expect(provider.disabled).toBe(false); + }); +}); diff --git a/client/src/components/cos/AppProviderPin.jsx b/client/src/components/cos/AppProviderPin.jsx index 32ae1fcf72..8b0c8dc3e4 100644 --- a/client/src/components/cos/AppProviderPin.jsx +++ b/client/src/components/cos/AppProviderPin.jsx @@ -27,6 +27,7 @@ import { providerPinPatch } from './constants'; * @param {string} props.label Provider-select label (also its aria-label when compact). * @param {string} [props.inheritLabel] What a blank pin resolves to, e.g. `Inherit (claude-code)`. * @param {boolean} [props.disabled] + * @param {boolean} [props.loading] Provider list not settled yet — see ProviderModelSelector. * @param {boolean} [props.compact] Hide labels for inline/table use. * @param {'row'|'stacked'} [props.layout] */ @@ -38,6 +39,7 @@ export default function AppProviderPin({ label, inheritLabel = 'Use default provider', disabled = false, + loading = false, compact = false, layout = 'row' }) { @@ -67,6 +69,7 @@ export default function AppProviderPin({ emptyModelOption="Default model" alwaysShowModel disabled={disabled} + loading={loading} compact={compact} layout={layout} /> diff --git a/client/src/components/cos/tabs/ScheduleTab.jsx b/client/src/components/cos/tabs/ScheduleTab.jsx index 0796500f47..04a56ad54b 100644 --- a/client/src/components/cos/tabs/ScheduleTab.jsx +++ b/client/src/components/cos/tabs/ScheduleTab.jsx @@ -40,7 +40,7 @@ function mergeOnDemandRequest(schedule, request) { // passed down — same convention as TasksTab/AgentsTab — so this tab's provider/ // model pickers stay live without standing up a second independent poll of the // same data. -export default function ScheduleTab({ apps, providers, activeProviderId }) { +export default function ScheduleTab({ apps, providers, providersLoaded, activeProviderId }) { const [searchParams, setSearchParams] = useSearchParams(); const [schedule, setSchedule] = useState(null); const [loading, setLoading] = useState(true); @@ -185,6 +185,7 @@ export default function ScheduleTab({ apps, providers, activeProviderId }) { tasks={tasks} apps={apps} providers={providers} + providersLoaded={providersLoaded} activeProviderId={activeProviderId} onTrigger={handleTriggerAppImprovement} onUpdate={handleUpdateTask} @@ -208,6 +209,7 @@ export default function ScheduleTab({ apps, providers, activeProviderId }) { onUpdate={handleUpdateTask} onTrigger={handleTriggerAppImprovement} providers={providers} + providersLoaded={providersLoaded} activeProviderId={activeProviderId} apps={apps} onUpdateOverride={handleUpdateOverride} diff --git a/client/src/components/cos/tabs/schedule/AppOverrideRow.jsx b/client/src/components/cos/tabs/schedule/AppOverrideRow.jsx index 1b168aeb84..dd80aec733 100644 --- a/client/src/components/cos/tabs/schedule/AppOverrideRow.jsx +++ b/client/src/components/cos/tabs/schedule/AppOverrideRow.jsx @@ -8,7 +8,7 @@ import ToggleSwitch from '../../../ToggleSwitch'; import useFieldDraft from '../../../../hooks/useFieldDraft'; import { INTERVAL_LABELS, setMetadataOverride } from './scheduleConstants'; -const AppOverrideRow = memo(function AppOverrideRow({ app, taskType, globalIntervalType, globalTaskMetadata, managedAgentOptions, fileIssuesCapable, defaultFileIssues, doWorkRequiresWorktree, inheritedProviderText, providers, override, onUpdate }) { +const AppOverrideRow = memo(function AppOverrideRow({ app, taskType, globalIntervalType, globalTaskMetadata, managedAgentOptions, fileIssuesCapable, defaultFileIssues, doWorkRequiresWorktree, inheritedProviderText, providers, providersLoaded = true, override, onUpdate }) { const [updating, setUpdating] = useState(false); const [cronEditing, setCronEditing] = useState(false); const isEnabled = override?.enabled === true; @@ -202,6 +202,7 @@ const AppOverrideRow = memo(function AppOverrideRow({ app, taskType, globalInter
) : ( - + ))} {/* Footer actions */} diff --git a/client/src/components/cos/tabs/schedule/AppTaskCard.test.jsx b/client/src/components/cos/tabs/schedule/AppTaskCard.test.jsx index 28332f0cbf..06713f558e 100644 --- a/client/src/components/cos/tabs/schedule/AppTaskCard.test.jsx +++ b/client/src/components/cos/tabs/schedule/AppTaskCard.test.jsx @@ -219,6 +219,15 @@ describe('AppTaskCard', () => { expect(screen.queryByLabelText('Thinking effort')).toBeNull(); }); + it('says the provider list is still loading instead of offering a lone bare Default', () => { + // Proves the card actually threads the flag; the label/disable rule itself + // is ProviderModelSelector's (see its own suite). + renderCardWithPins({}, { providers: [], providersLoaded: false }); + const provider = screen.getByLabelText('Provider'); + expect(within(provider).getByRole('option', { name: 'Loading providers…' })).toBeTruthy(); + expect(provider.disabled).toBe(true); + }); + it('hides a disabled provider from the picker unless the task is pinned to it', () => { const withDisabled = [...providers, { id: 'retired', name: 'Retired CLI', enabled: false }]; renderCardWithPins({}, { providers: withDisabled }); diff --git a/client/src/components/cos/tabs/schedule/AppTaskTypeSection.jsx b/client/src/components/cos/tabs/schedule/AppTaskTypeSection.jsx index 69d299ae2d..fce71b6f9c 100644 --- a/client/src/components/cos/tabs/schedule/AppTaskTypeSection.jsx +++ b/client/src/components/cos/tabs/schedule/AppTaskTypeSection.jsx @@ -3,7 +3,7 @@ import { Search, X } from 'lucide-react'; import AppTaskCard from './AppTaskCard'; import { TASK_FILTERS, DEFAULT_FILTER_ID, taskSortKey } from './scheduleConstants'; -export default function AppTaskTypeSection({ tasks, apps, providers, activeProviderId, onTrigger, onUpdate, onSelectTask, improvementDisabled, filter, onFilterChange }) { +export default function AppTaskTypeSection({ tasks, apps, providers, providersLoaded, activeProviderId, onTrigger, onUpdate, onSelectTask, improvementDisabled, filter, onFilterChange }) { const [search, setSearch] = useState(''); const taskEntries = Object.entries(tasks || {}); @@ -90,6 +90,7 @@ export default function AppTaskTypeSection({ tasks, apps, providers, activeProvi config={config} apps={apps} providers={providers} + providersLoaded={providersLoaded} activeProviderId={activeProviderId} onTrigger={onTrigger} onUpdate={onUpdate} diff --git a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx index afaae9d64a..9ca93ca0bd 100644 --- a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx +++ b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx @@ -40,7 +40,7 @@ const REVIEW_CONFIG_KEYS = [ 'reviewerApplies', ]; -export default function GlobalConfigControls({ taskType, config, onUpdate, onTrigger, category: _category, providers, activeProviderId, apps, updating, setUpdating, allTaskTypes, improvementDisabled, dataInputCatalog }) { +export default function GlobalConfigControls({ taskType, config, onUpdate, onTrigger, category: _category, providers, providersLoaded = true, activeProviderId, apps, updating, setUpdating, allTaskTypes, improvementDisabled, dataInputCatalog }) { const reviewDefaults = useCodeReviewDefaults(); // Resolved model lists for the reviewer table's Model column (the picker itself // never fetches — see its `modelOptions` prop). @@ -389,10 +389,12 @@ export default function GlobalConfigControls({ taskType, config, onUpdate, onTri handleModelChange(e.target.value)} - disabled={updating} + disabled={updating || !providersLoaded} className="w-full bg-port-card border border-port-border rounded px-3 py-2 text-white text-sm" > {/* `availableModels` already carries a pin the provider no longer diff --git a/client/src/components/cos/tabs/schedule/PerAppOverrideList.jsx b/client/src/components/cos/tabs/schedule/PerAppOverrideList.jsx index b82d9e2bc5..35f1f73a60 100644 --- a/client/src/components/cos/tabs/schedule/PerAppOverrideList.jsx +++ b/client/src/components/cos/tabs/schedule/PerAppOverrideList.jsx @@ -2,7 +2,7 @@ import { useState } from 'react'; import { providerModelLabel } from '../../../../utils/providers'; import AppOverrideRow from './AppOverrideRow'; -export default function PerAppOverrideList({ taskType, config, apps, providers, onUpdateOverride, onBulkToggleOverride }) { +export default function PerAppOverrideList({ taskType, config, apps, providers, providersLoaded = true, onUpdateOverride, onBulkToggleOverride }) { const [bulkUpdating, setBulkUpdating] = useState(false); const activeApps = apps?.filter(app => !app.archived) || []; const appOverrides = config.appOverrides || {}; @@ -58,6 +58,7 @@ export default function PerAppOverrideList({ taskType, config, apps, providers, doWorkRequiresWorktree={config.doWorkRequiresWorktree} inheritedProviderText={inheritedProviderText} providers={providers} + providersLoaded={providersLoaded} override={appOverrides[app.id]} onUpdate={onUpdateOverride} /> diff --git a/client/src/components/cos/tabs/schedule/PipelineStageConfig.jsx b/client/src/components/cos/tabs/schedule/PipelineStageConfig.jsx index c234e2c48b..adbe63a50d 100644 --- a/client/src/components/cos/tabs/schedule/PipelineStageConfig.jsx +++ b/client/src/components/cos/tabs/schedule/PipelineStageConfig.jsx @@ -51,7 +51,7 @@ const actionsStageNote = (eligibleProviders) => { ].join(' '); }; -export default function PipelineStageConfig({ taskType, config, providers, onUpdate, updating, setUpdating }) { +export default function PipelineStageConfig({ taskType, config, providers, providersLoaded = true, onUpdate, updating, setUpdating }) { const stages = pipelineStages(config); const needsSecurityModelPolicy = taskType === 'pr-reviewer'; const { ollama, lmstudio, capabilitiesByBackend, loading: localModelsLoading } = useLocalModels({ @@ -203,6 +203,7 @@ export default function PipelineStageConfig({ taskType, config, providers, onUpd )} {!isSecurityStage && ( diff --git a/client/src/components/cos/tabs/schedule/TaskModelQuickControls.jsx b/client/src/components/cos/tabs/schedule/TaskModelQuickControls.jsx index 673fde41a8..39cea3fe21 100644 --- a/client/src/components/cos/tabs/schedule/TaskModelQuickControls.jsx +++ b/client/src/components/cos/tabs/schedule/TaskModelQuickControls.jsx @@ -7,7 +7,7 @@ import ProviderModelSelector from '../../../ProviderModelSelector'; // // `highlightToolUse` is on because a scheduled task IS an agent run: a task // pinned to a local model that can't call tools narrates instead of working. -export default function TaskModelQuickControls({ pins, providers, disabled = false }) { +export default function TaskModelQuickControls({ pins, providers, loading = false, disabled = false }) { const { providerId, model, effort, effectiveProviderId, defaultProviderLabel, availableModels, saving, changeProvider, changeModel, changeEffort, @@ -30,6 +30,7 @@ export default function TaskModelQuickControls({ pins, providers, disabled = fal alwaysShowModel compact highlightToolUse + loading={loading} disabled={disabled || saving} />
diff --git a/client/src/pages/ChiefOfStaff.jsx b/client/src/pages/ChiefOfStaff.jsx index 08ef8a6405..aa0d48150a 100644 --- a/client/src/pages/ChiefOfStaff.jsx +++ b/client/src/pages/ChiefOfStaff.jsx @@ -6,6 +6,7 @@ import { useAutoRefetch } from '../hooks/useAutoRefetch'; import { useValidTab } from '../hooks/useValidTab'; import * as api from '../services/api'; import { coalesce } from '../utils/coalesce'; +import { sameJsonShape } from '../lib/sameJsonShape'; import { Play, Pause, Square, Clock, CheckCircle, AlertCircle, Cpu, ChevronDown, ChevronUp, ChevronLeft, ChevronRight, Brain, PanelLeftClose, PanelLeftOpen } from 'lucide-react'; import toast from '../components/ui/Toast'; import BrailleSpinner from '../components/BrailleSpinner'; @@ -101,6 +102,9 @@ export default function ChiefOfStaff() { // Which provider an unpinned task actually runs on — the Schedule tab names it // on the "Default" option and resolves model/effort choices against it. const [activeProviderId, setActiveProviderId] = useState(null); + // `[]` is ambiguous — "not fetched yet" vs "fetched, none configured". The + // pickers below need the difference; see ProviderModelSelector's `loading`. + const [providersLoaded, setProvidersLoaded] = useState(false); const [apps, setApps] = useState([]); const [loading, setLoading] = useState(true); const [agentState, setAgentState] = useState('sleeping'); @@ -163,6 +167,19 @@ export default function ChiefOfStaff() { return resolved; }, []); + // The single write path for the provider list, mirroring `applyHealth` above: + // stamping the settle flag anywhere else could set `providers` without it. + // `sameJsonShape` keeps the array identity stable when the payload is + // unchanged — which it is on essentially every 30s poll — so the early commit + // that fixes first-paint latency doesn't cost a full-tree re-render each tick + // (`providers` is an unmemoized prop down through every schedule card). + const applyProviders = useCallback((data) => { + setProviders(prev => (sameJsonShape(prev, data.providers || []) ? prev : data.providers || [])); + setActiveProviderId(data.activeProvider || null); + setProvidersLoaded(true); + return data; + }, []); + // Derive agent state from system status const deriveAgentState = useCallback((statusData, agentsData, healthData) => { if (!statusData?.running) return 'sleeping'; @@ -196,9 +213,16 @@ export default function ChiefOfStaff() { applyHealth(data, { merge: true }); return data; }); + // `/providers` is a cache-only read that returns in milliseconds, but bundling + // it into the Promise.all below held it until the SLOWEST sibling settled — + // and `getCosActionableInsights` runs a server-side PM2/memory health check. + // That left the Schedule tab's provider pickers empty for seconds, rendering + // a lone "Default (active provider)" option that reads as broken. Same fix as + // `healthRead`: commit on its own settle. + const providersRead = api.getProviders() + .catch(() => ({ providers: [] })) + .then(applyProviders); const secondaryRead = Promise.all([ - healthRead, - api.getProviders().catch(() => ({ providers: [] })), api.getApps().catch(() => []), api.getCosLearningSummary().catch(() => null), // `silent: true` keeps transient poll blips quiet, matching the banner's @@ -230,7 +254,10 @@ export default function ChiefOfStaff() { const runningAgent = agentsData.find(a => a.status === 'running'); setActiveAgentMeta(runningAgent?.metadata || null); - const [, providersData, appsData, learningSummaryData, insightsData] = await secondaryRead; + const [appsData, learningSummaryData, insightsData] = await secondaryRead; + // Both self-committing reads are barriers, not values: `mergedHealth` below + // reads what `healthRead` wrote, so it must not run before they settle. + await Promise.all([healthRead, providersRead]); // `getCosHealth` above reads the *pre-check* persisted health, while the // getCosActionableInsights call in this same batch triggers a fresh server // health check (cos.runHealthCheck) that emits `cos:health:check` — the @@ -239,8 +266,6 @@ export default function ChiefOfStaff() { // failed); everything below derives from what it returned, never from the // raw read, so the bubble can't name an older issue than the tile shows. const mergedHealth = healthRef.current; - setProviders(providersData.providers || []); - setActiveProviderId(providersData.activeProvider || null); // Filter out PortOS Autofixer (it's part of PortOS project) setApps(appsData.filter(a => a.id !== 'portos-autofixer')); setLearningSummary(learningSummaryData); @@ -1178,7 +1203,7 @@ export default function ChiefOfStaff() { {activeTab === 'schedule' && (
}> - +
)}