fix: thread ProviderModelSelector's loading gate to the remaining provider pickers - #6106
Conversation
atomantic
left a comment
atomantic
left a comment
There was a problem hiding this comment.
✅ Approved
A clean, well-tested follow-up to #6087 that threads the providers-loaded gate and ProviderModelSelector's loading flag into the remaining COS and FableLoom provider pickers. The patch applies cleanly against main, all 14 covering test files pass (236 tests), every new prop defaults to a backward-compatible value, and the new self-committing apps read mirrors the already-shipped applyProviders pattern without introducing a new failure mode. No blocking findings; one non-blocking note on the shared wipe-on-transient-failure fallback.
Scope: Client-only: threads providersLoaded/loading from ChiefOfStaff through the Tasks/Agents/Workflow tabs, the task/agent modals, and five FableLoom ProviderModelSelector consumers.
💡 Non-blocking (1)
client/src/pages/ChiefOfStaff.jsx:249— apps fetch failure clears last-good list, same as providers
Test evidence
- ✅
NODE_ENV=test npx vitest run src/components/cos/tabs/AgentsTab.test.jsx src/components/cos/tabs/RelaunchAgentModal.test.jsx src/components/cos/tabs/schedule/AppOverrideRow.test.jsx src/components/cos/tabs/TaskItem.test.jsx src/components/cos/tabs/TasksTab.test.jsx src/components/cos/tabs/WorkflowTab.providers.test.jsx— 6 files, 106 tests passed - ✅
NODE_ENV=test npx vitest run src/components/cos/TaskAddForm.test.jsx src/components/fableloom/LoomEditorialAutomation.test.jsx src/components/fableloom/LoomEpisodeFeedback.test.jsx src/components/fableloom/LoomEpisodeOutlinePlanner.test.jsx src/components/fableloom/LoomSeriesPlan.test.jsx src/pages/FableLoom.test.jsx— 6 files, 59 tests passed - ✅
NODE_ENV=test npx vitest run src/pages/ChiefOfStaff.test.jsx src/components/ProviderModelSelector.test.jsx— 2 files, 71 tests passed (the ChiefOfStaff caller and the unchanged ProviderModelSelector contract) - ⏭️
npm test -w client— Full client workspace suite deliberately not run as review evidence: the sandbox denies network binds, GPU, and background services a minority of suites need, so a from-zero full run reports thousands of environment-driven failures unrelated to this patch; the 14 targeted files covering every patched component were run instead.
Notes
- The PR description carries a Claude Code attribution footer, which the repo convention says PR descriptions should not include; a maintainer may ask the author to drop it.
Claims verified against the code
- ProviderModelSelector's loading prop disables the provider, model, and effort selects and swaps the empty option for a Loading providers placeholder (client/src/components/ProviderModelSelector.jsx:212).
- providersLoaded is stamped true only inside applyProviders, the single provider-list write path, so the flag can never be true without the list present (client/src/pages/ChiefOfStaff.jsx:189).
- The new appsRead self-committing read is added to the settle barrier alongside healthRead and providersRead (client/src/pages/ChiefOfStaff.jsx:284).
- LoomSeriesPlan destructures the hook's loading flag (client/src/components/fableloom/LoomSeriesPlan.jsx:384) and threads it into ProviderModelSelector (client/src/components/fableloom/LoomSeriesPlan.jsx:457); the other four FableLoom consumers do the same with their respective loading variables.
- providersLoaded is passed from ChiefOfStaff into WorkflowTab, the tab that previously accepted the prop but was never fed it (client/src/pages/ChiefOfStaff.jsx:1247).
| .then(applyProviders); | ||
| // Same rationale as providersRead above: apps commits on its own settle | ||
| // instead of waiting on the slower siblings in secondaryRead. | ||
| const appsRead = api.getApps().catch(() => []).then(applyApps); |
There was a problem hiding this comment.
💡 Non-blocking — apps fetch failure clears last-good list, same as providers
The catch fallback on appsRead turns a transient fetch failure into an empty array, and applyApps (line 201) then overwrites the last-good apps state with an empty list instead of preserving it, so one failed poll blips the app pickers empty until the next successful fetch. This is not new: the providers path in applyProviders already has identical behavior and this PR mirrors it deliberately. The same function already preserves last-good for insights specifically to avoid this flicker, so a follow-up could make both the apps and providers self-commit reads keep the last-good list on a failed fetch instead of resetting to empty.
…vider pickers (atomantic#6087) apps and providers now settle on their own read in ChiefOfStaff.fetchData instead of waiting on the whole secondaryRead batch, and providersLoaded flows down to every picker that was still rendering a lone emptyProviderOption during the initial fetch: TaskAddForm (which also stopped clobbering a pinned provider mid-fetch), TaskItem/SortableTaskItem, RelaunchAgentModal, ResumeAgentModal, and the Workflow tab's per-app override pickers. The five FableLoom components on useProviderModels now pass its loading flag through as well. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3ab486d to
b39637d
Compare
2 participants
Summary
ChiefOfStaff.fetchDatanow settlesappson its own read (appsRead, mirroringprovidersRead) instead of holding it hostage insidesecondaryRead'sPromise.all, with asameJsonShapeguard to keep array identity stable on an unchanged poll.providersLoadednow flows fromChiefOfStaffdown throughTasksTab→TaskAddForm/TaskItem/SortableTaskItem,AgentsTab→RelaunchAgentModal/ResumeAgentModal, andWorkflowTab→AppOverridePanel→PerAppOverrideList(which already accepted the prop from the Schedule-tab fix, just wasn't fed by Workflow).TaskAddForm's provider-reset effect was clearing a legitimately pinned provider on every mount, becauseenabledProvidersis always empty mid-fetch — it's now gated onprovidersLoaded.ResumeAgentModalrenders its own<select>s rather thanProviderModelSelector; applied the same "Loading providers…" + disabled treatment inline instead of restructuring the modal (no dedicated test file existed for it, so I kept the change surgical).useProviderModelsconsumers named in the issue (LoomEpisodeFeedback,LoomSeriesPlan,LoomEditorialAutomation,FableLoom,LoomEpisodeOutlinePlanner) now pass the hook'sloadingflag intoProviderModelSelectoralongside thedisabledprop they already wired.Follow-up to the Schedule-tab loading-gate fix, per #6087.
Test plan
client/src/components/cos/tabs/AgentsTab.test.jsxclient/src/components/cos/tabs/RelaunchAgentModal.test.jsxclient/src/components/cos/tabs/schedule/AppOverrideRow.test.jsxclient/src/components/cos/tabs/TaskItem.test.jsxclient/src/components/cos/tabs/TasksTab.test.jsxclient/src/components/cos/tabs/WorkflowTab.providers.test.jsxclient/src/components/cos/TaskAddForm.test.jsxclient/src/components/fableloom/LoomEditorialAutomation.test.jsxclient/src/components/fableloom/LoomEpisodeFeedback.test.jsxclient/src/components/fableloom/LoomEpisodeOutlinePlanner.test.jsxclient/src/components/fableloom/LoomSeriesPlan.test.jsxclient/src/pages/FableLoom.test.jsxbiome lint --error-on-warningson every changed file🤖 Generated with Claude Code