feat(dashboard): merge agent config modal into expandable sections#795
feat(dashboard): merge agent config modal into expandable sections#795
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactor that moves agent config editing from a modal into inline expandable sections. The approach simplifies state management by eliminating shared modal state and co-locating form state with each agent section. Two issues worth addressing around the "Saved" indicator and timer cleanup.
Code Issues
Should Fix
-
project-agent-configs.tsx ~line 155-156 —
setSaved(true)is called immediately inhandleSave()before the async mutation completes. If the mutation fails, the user sees "Saved" text for 2 seconds simultaneously with the error toast, which is confusing and misleading. The "Saved" indicator should only appear after confirmed persistence — either by havingonSaveConfigaccept a success callback, or by deriving the "just saved" state from the parent (e.g., a prop that flips afteronSuccessfires in the mutation). -
project-agent-configs.tsx ~line 156 — The
setTimeout(() => setSaved(false), 2000)timer is not cleaned up on unmount. If the user collapses the section or navigates away within 2 seconds of saving, React will warn about a state update on an unmounted component. Store the timeout ID in auseRefand clear it in auseEffectcleanup, or use auseEffectthat reacts to thesavedstate changing totrue.
Both issues also exist in the lifecycle triggers section (line ~564-566 area), but that code predates this PR. Worth fixing both together since the pattern is being replicated.
…p timers - setSaved(true) now fires only after the mutation's onSuccess callback, preventing "Saved" text showing when the mutation fails - Save success is signalled via a per-agent saveSuccessNonce counter that increments on each confirmed onSuccess, so repeated saves always trigger the useEffect in DefinitionAgentSection - setTimeout IDs are stored in useRef and cleared in useEffect cleanup to avoid state updates on unmounted components (fixes both agent section and lifecycle triggers section) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that consolidates the agent config modal into inline expandable sections. The approach is sound — eliminating the modal in favor of inline editing improves discoverability and reduces state management complexity. CI is green. Two issues worth addressing.
Code Issues
Should Fix
-
Saved indicator race condition (
project-agent-configs.tsx:118): After a successful save,onSuccessincrementssaveSuccessNonce(which triggerssetSaved(true)via useEffect) and simultaneously callsinvalidateQueries. When the refetch resolves shortly after, theconfigprop gets a new reference, which triggers the[config]sync useEffect that callssetSaved(false). The "Saved" indicator will flash briefly and then disappear when the query resolves — could be near-instant on fast networks. Consider either (a) skipping thesetSaved(false)in the config sync effect whensavedis already true, (b) using a ref to track "just saved" state that the config sync effect can check and clear, or (c) keying the nonce reset on a timeout rather than config changes. -
deleteMutation missing error handling (
project-agent-configs.tsx:445-448): ThedeleteMutationhas noonErrorcallback. If a delete fails, the error is silently swallowed — the user clicks Delete, confirms, and gets no feedback. The create/update mutations both havetoast.errorhandlers; the delete mutation should be consistent.
…or handling - Add `justSavedRef` to prevent the config sync effect from clearing the "Saved" indicator when `invalidateQueries` triggers a refetch after save - Set `justSavedRef.current = true` in the nonce effect before showing the indicator, cleared by the next config sync cycle - Add `onError` toast to `deleteMutation` for consistent error feedback alongside create/update mutations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that moves agent config form fields from a dialog modal into inline expandable sections. The change is well-scoped to a single file, CI passes, types check, and the implementation is sound.
Code Issues
Should Fix
-
project-agent-configs.tsx:639 — Minor race condition in
isSavingindicator:savingAgentTypeis a single string shared across all agent sections. If a user rapidly saves two agents (A then B),savingAgentTypeis overwritten to B. When A'sonSuccessfiressetSavingAgentType(null), this clears the saving state while B's mutation is still in-flight, causing B'sisSavingprop to briefly evaluate to false. Consider using aSet<string>instead to track saving state per agent type independently. -
project-agent-configs.tsx:245 —
maxIterationsnumber input is missingmin={1}(ormin={0}), whilemaxConcurrencyon line 260 correctly hasmin={1}. This means a user could enter negative iteration values. Addmin={1}for consistency.
Both are minor and don't block merge.
What's Good
- Removing the Dialog and 7 associated state variables (
dialogOpen,editing,agentType,model,maxIterations,agentEngine,maxConcurrency) eliminates the biome cognitive-complexity suppression, confirming the refactoring genuinely simplified the component. - The nonce-based "Saved" indicator with
justSavedRefis a thoughtful design that prevents flicker wheninvalidateQueriestriggers a refetch that would otherwise clear the saved state. - Proper error handling added via
toast.erroron all mutation paths (create, update, delete) — the old code was silently swallowing errors on create/update. - Timer cleanup on unmount prevents state updates on unmounted components.
- Mutations now accept explicit input parameters instead of capturing parent scope, making them more predictable and testable.
Summary
ProjectAgentConfigsDialog,Pencil,Trash2, etc.)DefinitionAgentSectionprops interface withonSaveConfig,isSaving, andenginesThe global settings modal (
AgentConfigFormDialogused byagent-configs-table.tsx) is untouched.Trello card: https://trello.com/c/MtgOurMH/315-lets-rethink-the-dashboard-ui-for-editing-project-level-agent-configs-right-now-we-have-both-modal-edit-agent-config-and-expanda
Test plan
npm test)🤖 Generated with Claude Code