[AGE-4014] fix(frontend): Show changed-path affordance for provider/connection edits in config drawer - #5627
Conversation
dosubot
Bot
added
the
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe model harness tracks draft changes to ChangesProvider credentials tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ge-shows-a-dirty-badge-but-no-changed
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09d42cc0-b2ea-4d3d-93b2-12d171e0b640
📒 Files selected for processing (2)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsx
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f601b57be6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Railway Preview Environment
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14754a8b0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… atomically, trim comment Addresses review: a provider-only change (llm.provider without llm.connection) now gets the same changed indicator and Restore as a connection change. The group Restore also reverts llm.model alongside provider/connection, since writeModel can move all three together and a partial revert reattaches a stale connection slug to the new model (rejected by the backend). Also trims a comment that grew past the one-line convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsx
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ardaerzin
left a comment
ardaerzin
left a comment
There was a problem hiding this comment.
Read the diff plus ChangedPathsContext, FocusPathsContext, sectionChanges.ts and ProviderCredentialsSectionView. Three things — the first one blocks.
| // `writeModel` can move `llm.model` atomically with `llm.provider`/`llm.connection.slug` (a | ||
| // catalog pick clears the old custom slug) — revert all three together or the group's Restore | ||
| // reattaches a stale slug to the new model, a combination the backend rejects. | ||
| const revertCredentials = useCallback(() => { |
There was a problem hiding this comment.
This restores only llm.model.
Each of revertConnection / revertProviderField / revertModel is onChange(revertPathsTo(config, committed, paths)) (AgentTemplateControl.tsx:497), closed over the same render-time config. Calling them in sequence is three full-config writes in one handler — last write wins, so the reverted provider and connection slug are overwritten by the model-only revert. That leaves the new provider/slug with the old model: the exact combination the comment above says the backend rejects.
Needs to be one revert() call with the union of paths, e.g. a multi-prefix useRevertUnder, or revert([...pathsUnder("llm.connection"), "llm.provider", "llm.model"]).
revertPermissions a few lines up has the same shape and the same latent bug.
| disabled={disabled} | ||
| > | ||
| {mh.providerCredentialsInline} | ||
| {credentialsPathChanged ? ( |
There was a problem hiding this comment.
This drops the inline model picker from the connect-key pane.
providerCredentialsSection is the non-bare variant: it never receives modelControl, and it renders railDetail rather than topRail (ProviderCredentialsSectionView.tsx:612-653). So this branch loses the model select and swaps in the 190px side rail that the comment on topRail describes as leaving the key form cramped in a narrow column.
That hits the common path, not an edge case: the usual way into the needs-key state is a catalog model pick, which moves llm.provider, so credentialsPathChanged is true. Threading bare + modelControl into the changed-aware render would keep the picker and the compact rail alongside the new badge/indicator/revert.
| // catalog model switch can move on its own — track both, or a provider-only change gets a "Connect | ||
| // key" badge with no changed indicator or Restore to go with it. | ||
| const connectionChanged = useHasChangedUnder("llm.connection") | ||
| const providerFieldChanged = useHasChangedUnder("llm.provider") |
There was a problem hiding this comment.
credentialsInFocus (line 468) wasn't widened along with credentialsChanged — it still checks llm.connection only.
In the non-key "what changed" branch, a provider-only diff therefore leaves harnessKindInFocus, modelInFocus and credentialsInFocus all false, and the expanded section renders nothing but the "Detailed configuration" link. That's the #5544 symptom again, in the branch this PR doesn't touch — and it's the same provider-moves-on-its-own case the comment right here describes.
2 participants
Summary
Fixes #5544. In the agent config's Model & harness section, changing the provider/connection without committing shows the "Draft" state and the "Connect key" badge on the collapsed header, but expanding it showed no changed-path affordance at all — no dotted underline, no committed-value popover, no Restore.
Root cause: that section has three inline render branches, in precedence order — needs-key pane, changed-diff view, plain drawer row. When a provider/connection key is missing, the needs-key pane always wins, and it renders the bare
ProviderCredentialsSectionvariant, which is headerless by design and is never mounted under aChangedPathsProvider. So even when a real diff exists, the component reading it silently sees "nothing changed."Changes
When the needs-key pane is showing AND the edit that triggered it touched
llm.connection.*/llm.providerspecifically (not some other model-harness field likeharness.kind), it now renders through the sameChangedPathsProvider-wrapped path the rest of the panel's diff views use, and swaps to the non-bareProviderCredentialsSection— the variant that already has the "Connect key" badge, a changed-path indicator dot, and a "Revert this group?" action, just previously never assembled together for this specific pane.A change to an unrelated model-harness field keeps the original plain pane (with the inline model picker) unchanged — this only widens the branch that was structurally incapable of showing a diff.
Testing
Verified locally
tsc --noEmitclean on@agenta/entity-uiand onweb/oss(which consumes this component).eslintclean on both touched files.Added or updated tests
N/A — no existing unit tests cover
AgentTemplateControl/useModelHarness/ProviderCredentialsSection.QA follow-up
Demo
Screen.Recording.2026-08-01.at.4.06.36.PM.mov
Checklist
Contributor Resources