feat(api-service): improve managed-agent MCP setup gate UX fixes NV-7906#11350
Conversation
Show typing before the setup gate and before OAuth replay, nudge on gated follow-ups, and use platform-aware setup-complete card copy. Co-authored-by: Cursor <cursoragent@cursor.com>
|
PR author is not in the allowed authors list. |
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
📝 WalkthroughWalkthroughThis PR adds typing indicator support to the managed-agent setup flow. A new ChangesTyping indicator integration for managed-agent setup flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/app/agents/usecases/managed-agent-setup/complete-managed-agent-setup.usecase.ts (1)
273-275: ⚡ Quick winDeduplicate the typing-gate condition to prevent card/typing drift.
willShowTypingBeforeReplay(Lines 274-275) and the guard insideshowTypingBeforeSetupReplay(Line 340) are logical negations of the same predicate. They drive two coupled decisions —showProcessingHint: !willShowTypingBeforeReplayon the resolved card and whether typing actually fires. If one is edited without the other, the card copy can contradict the real behavior (e.g. celebration copy shown but no typing). Extract a single predicate and reuse it in both places.♻️ Proposed refactor
+ private shouldShowTypingBeforeReplay(conversation: ConversationEntity, config: ResolvedAgentConfig): boolean { + const platformThreadId = conversation.channels?.[0]?.platformThreadId; + + return Boolean( + config.acknowledgeOnReceived && PLATFORMS_WITH_TYPING_INDICATOR.has(config.platform) && platformThreadId + ); + }- const platformThreadId = conversation.channels?.[0]?.platformThreadId; - const willShowTypingBeforeReplay = - config.acknowledgeOnReceived && PLATFORMS_WITH_TYPING_INDICATOR.has(config.platform) && !!platformThreadId; + const willShowTypingBeforeReplay = this.shouldShowTypingBeforeReplay(conversation, config);private async showTypingBeforeSetupReplay( conversation: ConversationEntity, agent: Pick<AgentEntity, '_id'>, config: ResolvedAgentConfig ): Promise<void> { const platformThreadId = conversation.channels?.[0]?.platformThreadId; - if (!config.acknowledgeOnReceived || !PLATFORMS_WITH_TYPING_INDICATOR.has(config.platform) || !platformThreadId) { + if (!this.shouldShowTypingBeforeReplay(conversation, config) || !platformThreadId) { return; }Also applies to: 338-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/usecases/managed-agent-setup/complete-managed-agent-setup.usecase.ts` around lines 273 - 275, The two places use inverted copies of the same predicate causing potential drift; extract a single boolean predicate (e.g. shouldShowTypingBeforeReplay) computed once from the conversation/config (replace current willShowTypingBeforeReplay) and reuse it in both the card resolution (set showProcessingHint: !shouldShowTypingBeforeReplay) and the typing guard used by showTypingBeforeSetupReplay so both decisions come from the same value (update references to willShowTypingBeforeReplay and the guard in showTypingBeforeSetupReplay to use shouldShowTypingBeforeReplay).apps/api/src/app/agents/services/agent-inbound-handler.service.spec.ts (1)
298-351: ⚡ Quick winAssert the typing-before-gate ordering the test name promises.
The test is titled "should show typing before managed-agent setup gate" but only verifies both
thread.startTypingandsetupInboundwere each called once — it would still pass if the order were reversed. Add an explicit ordering assertion to actually pin down the behavior this PR changes.💚 Suggested ordering assertion
expect(thread.startTyping.calledOnceWith('Thinking...')).to.equal(true); expect(setupInbound.calledOnce).to.equal(true); + sinon.assert.callOrder(thread.startTyping, setupInbound); expect(managedAgentService.dispatch.called).to.equal(false);Separately, this case re-builds the entire handler inline rather than using
makeHandler, which now already exposesmanagedAgentService/agentRepository/subscriberRepository. ExtendingmakeHandlerwithsubscriberResolver/subscriberRepositoryoverrides and reusing it here would cut the duplication — optional, given focused-test stub conventions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/services/agent-inbound-handler.service.spec.ts` around lines 298 - 351, The test title promises startTyping happens before the managed-agent setup gate but currently only asserts both were called; add an explicit ordering check (e.g., use sinon.assert.callOrder or equivalent) to assert thread.startTyping was invoked before handleManagedAgentSetupInbound.execute (setupInbound) when AgentInboundHandler.handle runs; optionally refactor to reuse makeHandler by passing overrides for subscriberResolver/subscriberRepository/managedAgentService/agentRepository to remove the inline handler construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts`:
- Around line 391-409: The awaited call to thread.startTyping('Thinking...') can
throw and currently aborts subsequent setup/parking logic; make typing non-fatal
by catching errors instead of allowing the rejection to propagate: replace the
direct awaited call in agent-inbound-handler.service with a resilient form
(e.g., call thread.startTyping('Thinking...').catch(toDeliveryError) or wrap the
await in try/catch), and on error log a warning and call captureAgentWarning
with component 'agent-inbound-handler' and operation 'start-typing' so failures
are recorded but the code continues to the managed-agent parking/setup steps
(keep the existing acknowledge fallback branch behavior for first messages
intact).
---
Nitpick comments:
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.spec.ts`:
- Around line 298-351: The test title promises startTyping happens before the
managed-agent setup gate but currently only asserts both were called; add an
explicit ordering check (e.g., use sinon.assert.callOrder or equivalent) to
assert thread.startTyping was invoked before
handleManagedAgentSetupInbound.execute (setupInbound) when
AgentInboundHandler.handle runs; optionally refactor to reuse makeHandler by
passing overrides for
subscriberResolver/subscriberRepository/managedAgentService/agentRepository to
remove the inline handler construction.
In
`@apps/api/src/app/agents/usecases/managed-agent-setup/complete-managed-agent-setup.usecase.ts`:
- Around line 273-275: The two places use inverted copies of the same predicate
causing potential drift; extract a single boolean predicate (e.g.
shouldShowTypingBeforeReplay) computed once from the conversation/config
(replace current willShowTypingBeforeReplay) and reuse it in both the card
resolution (set showProcessingHint: !shouldShowTypingBeforeReplay) and the
typing guard used by showTypingBeforeSetupReplay so both decisions come from the
same value (update references to willShowTypingBeforeReplay and the guard in
showTypingBeforeSetupReplay to use shouldShowTypingBeforeReplay).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1439cb23-254d-4a6c-a806-534b3241566e
📒 Files selected for processing (7)
apps/api/src/app/agents/services/agent-inbound-handler.service.spec.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/managed-agent-setup/complete-managed-agent-setup.usecase.tsapps/api/src/app/agents/usecases/managed-agent-setup/handle-managed-agent-setup-inbound.usecase.tsapps/api/src/app/agents/usecases/managed-agent-setup/setup-card.builder.tsapps/api/src/app/agents/usecases/managed-agent-setup/setup-card.helpers.ts
| if (config.acknowledgeOnReceived) { | ||
| const supportsTyping = PLATFORMS_WITH_TYPING_INDICATOR.has(config.platform); | ||
|
|
||
| if (supportsTyping) { | ||
| await thread.startTyping('Thinking...'); | ||
| } else if (isFirstMessage && message.id) { | ||
| thread | ||
| .createSentMessageFromMessage(message) | ||
| .addReaction(ACKNOWLEDGE_FALLBACK_EMOJI) | ||
| .catch((err) => { | ||
| this.logger.warn(err, `[agent:${agentId}] Failed to add ack reaction to first message`); | ||
| captureAgentWarning(err, { | ||
| component: 'agent-inbound-handler', | ||
| operation: 'add-ack-reaction', | ||
| agentId, | ||
| }); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard the typing call so a typing failure can't skip the setup gate.
await thread.startTyping('Thinking...') is awaited with no error handling, and it now runs before the managed-agent parking logic (Line 413). If startTyping rejects (delivery/network error), the rejection propagates out of handle, so the message is never parked and the setup card is never shown — the opposite of the UX this PR is trying to deliver. Note the fallback reaction branch already swallows its errors via .catch(...), and ChatSdkService.startTypingInConversation routes failures through .catch(toDeliveryError); the typing branch here should be equally resilient.
🛡️ Proposed fix to make typing non-fatal
if (supportsTyping) {
- await thread.startTyping('Thinking...');
+ await thread.startTyping('Thinking...').catch((err) => {
+ this.logger.warn(err, `[agent:${agentId}] Failed to start typing indicator`);
+ captureAgentWarning(err, {
+ component: 'agent-inbound-handler',
+ operation: 'start-typing',
+ agentId,
+ });
+ });
} else if (isFirstMessage && message.id) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (config.acknowledgeOnReceived) { | |
| const supportsTyping = PLATFORMS_WITH_TYPING_INDICATOR.has(config.platform); | |
| if (supportsTyping) { | |
| await thread.startTyping('Thinking...'); | |
| } else if (isFirstMessage && message.id) { | |
| thread | |
| .createSentMessageFromMessage(message) | |
| .addReaction(ACKNOWLEDGE_FALLBACK_EMOJI) | |
| .catch((err) => { | |
| this.logger.warn(err, `[agent:${agentId}] Failed to add ack reaction to first message`); | |
| captureAgentWarning(err, { | |
| component: 'agent-inbound-handler', | |
| operation: 'add-ack-reaction', | |
| agentId, | |
| }); | |
| }); | |
| } | |
| } | |
| if (config.acknowledgeOnReceived) { | |
| const supportsTyping = PLATFORMS_WITH_TYPING_INDICATOR.has(config.platform); | |
| if (supportsTyping) { | |
| await thread.startTyping('Thinking...').catch((err) => { | |
| this.logger.warn(err, `[agent:${agentId}] Failed to start typing indicator`); | |
| captureAgentWarning(err, { | |
| component: 'agent-inbound-handler', | |
| operation: 'start-typing', | |
| agentId, | |
| }); | |
| }); | |
| } else if (isFirstMessage && message.id) { | |
| thread | |
| .createSentMessageFromMessage(message) | |
| .addReaction(ACKNOWLEDGE_FALLBACK_EMOJI) | |
| .catch((err) => { | |
| this.logger.warn(err, `[agent:${agentId}] Failed to add ack reaction to first message`); | |
| captureAgentWarning(err, { | |
| component: 'agent-inbound-handler', | |
| operation: 'add-ack-reaction', | |
| agentId, | |
| }); | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts` around
lines 391 - 409, The awaited call to thread.startTyping('Thinking...') can throw
and currently aborts subsequent setup/parking logic; make typing non-fatal by
catching errors instead of allowing the rejection to propagate: replace the
direct awaited call in agent-inbound-handler.service with a resilient form
(e.g., call thread.startTyping('Thinking...').catch(toDeliveryError) or wrap the
await in try/catch), and on error log a warning and call captureAgentWarning
with component 'agent-inbound-handler' and operation 'start-typing' so failures
are recorded but the code continues to the managed-agent parking/setup steps
(keep the existing acknowledge fallback branch behavior for first messages
intact).
ChmaraX
deleted the
nv-7906-managed-agent-mcp-setup-gate-typing-ack-and-setup-complete
branch
1 participant
Summary
Linear
https://linear.app/novu/issue/NV-7906/managed-agent-mcp-setup-gate-typing-ack-and-setup-complete-copy
Test plan
acknowledgeOnReceived: typing + setup card, no Anthropic dispatchYou're all set!) + typing + replay of parked messageMade with Cursor
What changed
The PR improves the managed-agent MCP OAuth setup flow UX by showing typing indicators and acknowledgment before the setup gate (on supported platforms like Slack/Teams/Telegram), providing immediate feedback while OAuth is pending. It adds platform-aware setup card copy that celebrates completion on platforms with typing indicators, or includes automatic replay hints on Email/WhatsApp or when acknowledge-on-receipt is disabled. Follow-up messages while gated now trigger nudge prompts and card refreshes. Stale setup cards are resolved with celebration-only copy.
Affected areas
startTypingInConversationhelper to ChatSdkService for triggering typing indicators, updated managed-agent setup usecases to conditionally show typing before replay, introduced platform-aware setup card builders withshowProcessingHintparameter to control completion copy, and added nudge markdown messages for gated interactions.Key technical decisions
acknowledgeOnReceivedbehavior earlier in the inbound flow (after managed-runtime detection) so it executes before the setup gate, ensuring immediate platform feedback.showProcessingHintparameter to setup card builders to conditionally display processing hints vs. celebration-only copy based on whether typing will be shown.startTypingInConversationhelper in ChatSdkService abstracts typing indicator logic with platform-aware fallbacks and graceful error handling.Testing
New unit test added to cover managed-agent inbound gating with
acknowledgeOnReceivedenabled, verifying typing is initiated and setup gate executes before dispatch.