fix: add clear/reset option to Slack workspace default agent selector#2085
fix: add clear/reset option to Slack workspace default agent selector#2085amikofalvy wants to merge 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1b16a8c The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
🟠 1) index.tsx:144-161 Optimistic update not rolled back on API failure
Issue: The handleRemoveDefaultAgent handler performs an optimistic UI update (setDefaultAgent(null)) before the API call completes, but does not restore the previous state if the API call fails.
Why: When the API fails (network error, server error, auth failure), users see inconsistent state: the UI shows "Select a default agent..." while the server still has the previous agent configured. A page refresh reveals the discrepancy, causing user confusion about whether the removal actually happened. This is especially problematic for Slack integrations where the old default agent would continue handling messages.
Fix: Capture the previous state before the optimistic update and restore it in the catch block. See inline comment for a 1-click suggestion.
Refs:
Inline Comments:
- 🟠 Major:
index.tsx:144-161Optimistic update rollback (1-click fix available)
🟠 2) agents-docs/ Documentation doesn't mention the "Remove default agent" capability
Issue: The Slack configuration documentation at agents-docs/content/talk-to-your-agents/slack/configuration.mdx explains how to set a workspace default agent, but does not document the new ability to remove/clear the workspace default.
Why: Users reading the documentation won't know this capability exists. Workspace administrators who want to disable the fallback behavior (e.g., to require explicit channel configuration) may incorrectly believe they cannot undo a workspace default once set. The overview page mentions the behavior when no agent is configured, but the configuration page doesn't explain how to reach that state.
Fix: Update the "Set the default agent" section (around lines 26-31 of configuration.mdx) to mention the remove option. Suggested addition:
To remove the workspace default agent, open the dropdown and select **Remove default agent**.
This clears the workspace-level default — channels without a channel-specific agent will
prompt users to contact an admin.Refs:
💭 Consider (1) 💭
💭 1) workspace-default-section.tsx:91-97 Label terminology: "Remove" vs "Clear"
Issue: The label "Remove default agent" uses different terminology than the existing "Reset to workspace default" pattern in channel cells and bulk select. While "Remove" is semantically accurate (there's no parent level to reset to), "Clear default agent" might better align with common UX patterns for clearing selections.
Why: The RotateCcw icon implies "reset/undo" which is slightly mismatched with "Remove" semantics. However, this is a minor UX consideration and the current implementation is reasonable.
Fix: Consider renaming to "Clear default agent" for consistency with common "clear selection" patterns. Alternatively, keep "Remove" if the intent is to emphasize that the action disables the feature rather than resetting to a default value.
Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: This PR correctly implements the "Remove default agent" feature following established UI patterns (RotateCcw icon, CommandSeparator, conditional rendering). The main areas to address are: (1) adding rollback on API failure for the optimistic update, and (2) documenting the new capability in the Slack configuration docs. The code quality and pattern consistency are excellent otherwise.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
slack-api.ts:78-93 |
No unit tests for new API method | Follows existing pattern — no tests exist for other methods in this file. Low-risk thin wrapper around fetch. |
index.tsx |
No component tests for AgentConfigurationCard | Follows existing pattern — codebase relies on manual E2E testing for UI components. |
| Pattern observations | Multiple INFO findings about pattern consistency | These were positive observations confirming the PR follows established patterns, not issues to address. |
Reviewers (8)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
2 | 0 | 1 | 0 | 1 | 0 | 0 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-docs |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-frontend |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-errors |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 10 | 1 | 1 | 0 | 1 | 0 | 5 |
Note: The optimistic update rollback issue was flagged by 3 reviewers (pr-review-product, pr-review-frontend, pr-review-errors) and merged into a single inline comment + main finding.
| const handleRemoveDefaultAgent = async () => { | ||
| if (!teamId) return; | ||
|
|
||
| setDefaultAgent(null); | ||
| setDefaultOpen(false); | ||
| setSavingDefault(true); | ||
|
|
||
| try { | ||
| await slackApi.removeWorkspaceDefaultAgent(teamId); | ||
| installedWorkspaces.refetch(); | ||
| toast.success('Default agent removed'); | ||
| } catch (error) { | ||
| console.error('Failed to remove default agent:', error); | ||
| toast.error('Failed to remove default agent'); | ||
| } finally { | ||
| setSavingDefault(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟠 MAJOR: Optimistic update not rolled back on API failure
Issue: The handler sets setDefaultAgent(null) on line 147 before the API call, but if the removeWorkspaceDefaultAgent call fails, the previous defaultAgent value is lost and the UI remains in an inconsistent state.
Why: When the API fails (network error, server error, auth failure), users see the dropdown showing "Select a default agent..." with a "Failed to remove default agent" toast, but on page refresh, the original agent reappears. This creates confusion: the user may believe the removal partially succeeded, when in fact the server still has the old default agent configured.
Fix: Capture the previous state before the optimistic update and restore it on failure:
| const handleRemoveDefaultAgent = async () => { | |
| if (!teamId) return; | |
| setDefaultAgent(null); | |
| setDefaultOpen(false); | |
| setSavingDefault(true); | |
| try { | |
| await slackApi.removeWorkspaceDefaultAgent(teamId); | |
| installedWorkspaces.refetch(); | |
| toast.success('Default agent removed'); | |
| } catch (error) { | |
| console.error('Failed to remove default agent:', error); | |
| toast.error('Failed to remove default agent'); | |
| } finally { | |
| setSavingDefault(false); | |
| } | |
| }; | |
| const handleRemoveDefaultAgent = async () => { | |
| if (!teamId) return; | |
| const previousAgent = defaultAgent; | |
| setDefaultAgent(null); | |
| setDefaultOpen(false); | |
| setSavingDefault(true); | |
| try { | |
| await slackApi.removeWorkspaceDefaultAgent(teamId); | |
| installedWorkspaces.refetch(); | |
| toast.success('Default agent removed'); | |
| } catch (error) { | |
| console.error('Failed to remove default agent:', error); | |
| setDefaultAgent(previousAgent); | |
| toast.error('Failed to remove default agent'); | |
| } finally { | |
| setSavingDefault(false); | |
| } | |
| }; |
Refs:
- handleSetDefaultAgent has the same pattern (pre-existing, out of scope)
- React optimistic updates best practice
amikofalvy
force-pushed
the
fix/slack-workspace-default-agent-clear
branch
from
c5b685d to
d8c0b23
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Prior Feedback Status
✅ Both major issues from the previous review have been addressed in commit d8c0b23c3:
-
Optimistic update rollback — The
handleRemoveDefaultAgenthandler now properly capturespreviousAgentbefore the optimistic update and restores it in the catch block on API failure. -
Documentation — The Slack configuration docs (
configuration.mdx) now include a "Remove the default agent (optional)" step explaining the capability and its consequences.
The inline comment on index.tsx:144-192 from the previous review is now resolved.
💭 Consider (2) 💭
💭 1) workspace-default-section.tsx:98-104 Icon semantics: RotateCcw vs "Remove"
Issue: The RotateCcw icon is used for "Remove default agent" at the workspace level. This icon typically implies "reset/revert" which matches the channel-level "Reset to workspace default" pattern, but semantically differs for workspace-level removal where there's no parent to reset to.
Why: Users familiar with the channel pattern (RotateCcw = reset to parent) may have a brief moment of confusion when the same icon means "remove entirely" at the workspace level. This is a minor UX consideration.
Fix: Consider using a different icon (e.g., X, Trash2, or MinusCircle from lucide-react) to visually distinguish the destructive removal action from the reset-to-parent pattern. Alternatively, keep the current icon for visual consistency — both approaches are reasonable.
Refs:
💭 2) workspace-default-section.tsx:95-107 Consequence communication for admin action
Issue: Removing the workspace default affects all channels currently using the workspace default (shown as "Used by N channels" in the card description), but the UI doesn't communicate this scope of impact at the point of action.
Why: The documentation correctly notes "Channels without a channel-specific agent will no longer have a fallback," but this consequence isn't surfaced in the UI. However, this is an admin-only action (canEditWorkspaceDefault = isAdmin) where admins likely understand the hierarchical model.
Fix: Options to consider if desired: (1) Add brief helper text in the CommandItem, (2) Use a confirmation dialog if multiple channels are affected. Current behavior is acceptable for admin-only features — flagging for intentional product consideration.
Refs:
✅ APPROVE
Summary: This PR cleanly implements the "Remove default agent" feature following established UI patterns. The prior review feedback has been addressed, the implementation is consistent with existing code patterns, and the documentation is updated. The two Consider items above are minor UX suggestions for future iteration, not blocking issues. Great work on the follow-up commit addressing the optimistic rollback! 🎉
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
slack-api.ts:79-94 |
API uses PUT with empty body instead of DELETE | Follows existing server-side design — PUT endpoint handles both set and clear via optional defaultAgent field. Not an issue. |
workspace-default-section.tsx:95-108 |
Menu item placement | Positive observation — placement is consistent with channel-level and bulk patterns. Not an issue. |
index.tsx:173-192 |
Optimistic update rollback | Previously raised in prior review — now addressed in commit d8c0b23c3. |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
2 | 0 | 2 | 0 | 0 | 0 | 0 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 5 | 0 | 2 | 0 | 0 | 0 | 3 |
Note: The icon semantics issue was flagged by both pr-review-product and pr-review-consistency; merged into one Consider finding under pr-review-product.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Prior Feedback Status
✅ All feedback from previous reviews has been addressed:
-
Icon semantics (Consider item) — Addressed in commit
78418de73. Changed fromRotateCcwtoXicon, which semantically distinguishes the workspace-level "remove" action from the channel-level "reset to parent" pattern. This was the main Consider item from the prior review. -
Test coverage — The same commit adds comprehensive test coverage for the
PUT /workspaces/:teamId/settingsendpoint, including:- Setting a default agent (happy path)
- Clearing the default agent via empty body (happy path)
- Error handling when Nango persistence fails
The prior inline comment on index.tsx:144-192 about optimistic update rollback remains marked as ACTIVE in GitHub, but the code was addressed in an earlier commit (d8c0b23c3) — the handler now properly captures previousAgent before the optimistic update and restores it in the catch block.
Delta Review Summary
This re-review is scoped to the 1 commit since the last automated review:
| File | Change | Assessment |
|---|---|---|
workspace-default-section.tsx |
RotateCcw → X icon |
✅ Addresses prior Consider item |
routes.test.ts |
+82 lines of new tests | ✅ Comprehensive coverage |
No new issues identified in the delta.
✅ APPROVE
Summary: This commit cleanly addresses the prior review's Consider item (icon semantics) and adds valuable test coverage for the settings endpoint. The implementation is complete and follows established patterns. Ship it! 🚀
Discarded (0)
No findings were discarded.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Given the minimal delta (icon change + tests), only the tests reviewer was dispatched. The delta addressed prior feedback rather than introducing new functionality requiring full re-review.
amikofalvy
force-pushed
the
fix/slack-workspace-default-agent-clear
branch
from
78418de to
e0ab1f2
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Prior Feedback Status
✅ All feedback from previous reviews has been addressed:
| Issue | Status | Commit |
|---|---|---|
| Optimistic update rollback | ✅ Fixed | 009f9c38c |
| Documentation for remove capability | ✅ Added | 009f9c38c |
| Icon semantics (RotateCcw → X) | ✅ Addressed | e0ab1f29c |
| Test coverage for PUT settings | ✅ Added | e0ab1f29c |
The inline comment on index.tsx:144-192 from the initial review is now resolved — the handler properly captures previousAgent before the optimistic update and restores it in the catch block.
🧹 While You're Here (1)
🧹 1) workspaces.ts:323-326 Pre-existing: Silent failure when clearing workspace default agent
Issue: The else-branch in the PUT /workspaces/:teamId/settings handler doesn't check the return value of setWorkspaceDefaultAgentInNango(teamId, null). While the if-branch (line 309-313) correctly checks nangoSuccess and returns 500 on failure, the else-branch always returns { success: true } regardless of whether Nango persistence succeeded.
Why: This PR enables users to trigger the else-branch via the new "Remove default agent" UI. If Nango fails (network issue, workspace not found, API error), users will see a success toast while the old default agent remains configured. On page refresh, the agent reappears — causing user confusion.
Fix: Match the if-branch pattern:
} else {
const nangoSuccess = await setWorkspaceDefaultAgentInNango(teamId, null);
if (!nangoSuccess) {
logger.warn({ teamId }, 'Failed to clear workspace default agent in Nango');
return c.json({ success: false }, 500);
}
logger.info({ teamId }, 'Cleared workspace default agent');
}Refs:
- workspaces.ts:308-326 — if-branch checks return value, else-branch doesn't
- Introduced in PR #2033 (pre-existing, not from this PR)
✅ APPROVE
Summary: This PR cleanly implements the "Remove default agent" feature following established UI patterns (X icon, optimistic updates with rollback, CommandSeparator). All prior review feedback has been addressed across the 4 commits. The implementation is consistent with existing codebase patterns and includes proper documentation and test coverage. The one "While You're Here" item above is a pre-existing backend issue that this PR surfaces but doesn't introduce — it's optional to address alongside this work. Ship it! 🚀
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
routes.test.ts |
Missing auth test coverage for PUT settings | Pre-existing test pattern — auth middleware tests are shared infrastructure, not PR-specific scope |
routes.test.ts |
Cache clearing not verified in tests | Tests implementation details rather than observable behavior — the API contract is adequately covered |
slack-api.ts:90 |
Error message loses HTTP status context | Follows existing pattern in other API methods — not a regression |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
3 | 0 | 0 | 1 | 0 | 0 | 2 |
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
2 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 5 | 0 | 0 | 1 | 0 | 0 | 3 |
Note: The silent failure issue was flagged by both pr-review-tests and pr-review-errors; merged into one While You're Here finding (attributed to pr-review-tests as the source). The pr-review-errors version is counted as discarded to avoid double-counting.
The workspace default agent selector had no way to deselect/remove a previously chosen agent. Adds a "Remove default agent" option using the existing RotateCcw + CommandSeparator pattern from the channel agent cell. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Capture previous defaultAgent before optimistic update and restore it in the catch block so the UI stays consistent on API failure - Add "Remove the default agent" step to Slack configuration docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use X icon instead of RotateCcw for the workspace-level "Remove default agent" action to distinguish it from the channel-level "Reset to workspace default" pattern. Add integration tests for the PUT workspace settings endpoint covering set, clear, and Nango failure scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
amikofalvy
force-pushed
the
fix/slack-workspace-default-agent-clear
branch
from
e0ab1f2 to
1b16a8c
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Prior Feedback Status
✅ All feedback from previous reviews remains addressed:
| Issue | Status | Commit |
|---|---|---|
| Optimistic update rollback | ✅ Fixed | 1a3863d05 |
| Documentation for remove capability | ✅ Added | 1a3863d05 |
| Icon semantics (RotateCcw → X) | ✅ Addressed | 1b16a8c7f |
| Test coverage for PUT settings | ✅ Added | 1b16a8c7f |
Re-Review Summary
This is a re-review triggered on the same commit (1b16a8c7f) that received ✅ APPROVE in the previous review. The implementation is complete:
- Frontend:
handleRemoveDefaultAgentwith proper optimistic update rollback ✓ - API:
removeWorkspaceDefaultAgent()method using existing PUT endpoint ✓ - UI: X icon + CommandSeparator pattern, conditional rendering ✓
- Tests: 3 test cases covering set, clear, and error scenarios ✓
- Docs: "Remove the default agent (optional)" step added ✓
🧹 While You're Here (1)
Previously flagged — optional, not blocking:
🧹 1) workspaces.ts:323-326 Pre-existing: Silent failure when clearing workspace default agent
The else-branch doesn't check the return value of setWorkspaceDefaultAgentInNango(teamId, null). Consider matching the if-branch pattern for consistency. See previous review for full details.
✅ APPROVE
Summary: Implementation complete and follows established patterns. All prior review feedback has been addressed. The "While You're Here" item is a pre-existing backend issue (not blocking). Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 1 | 0 | 0 | 0 |
Note: No new reviewers dispatched — this is a re-review confirming prior approval on unchanged commit.
1 participant
Summary
RotateCcwicon +CommandSeparatorpattern already used in the channel agent cell and bulk select barChanges
slack-api.ts— AddedremoveWorkspaceDefaultAgent()API method (sends empty body to existing PUT settings endpoint, which already handles clearing)index.tsx— AddedhandleRemoveDefaultAgenthandler with optimistic UI update, error rollback, and toast notificationsworkspace-default-section.tsx— Added "Remove default agent"CommandItemwithRotateCcwicon, shown conditionally whendefaultAgentis setconfiguration.mdx— Added "Remove the default agent (optional)" step to Slack docsChangeset
@inkeep/agents-manage-ui— patchTest plan
Manual QA scenarios that resist automation. Updated as tests complete.
handleRemoveDefaultAgentcapturespreviousAgentbeforesetDefaultAgent(null)and restores it in the catch block on API failure · Verified via code review/work-apps/slack/workspaces/:teamId/settingswith empty body{}triggers else-branch callingsetWorkspaceDefaultAgentInNango(teamId, null), setting metadata to empty string and clearing cache · Verified via backend code tracegetWorkspaceDefaultAgentFromNangoreturnsnull, GET settings returns{ defaultAgent: undefined }, frontendfetchWorkspaceSettingsleavesdefaultAgentasnull· Verified via code trace{defaultAgent && canEdit && ...}guard correctly hides the grant-access switch when default agent is cleared · Verified via code reviewdefaultAgentis truthy ({defaultAgent && ...}) · Verified via code reviewRotateCcwicon +CommandGroup+CommandSeparatorpattern aschannel-agent-cell.tsxandbulk-select-agent-bar.tsx· Verified via greppnpm --filter @inkeep/agents-manage-ui typecheckpasses cleanly after rebase · Verified via shell🤖 Generated with Claude Code