feat(connector-slack): in-Slack UX pass — consent card, assistant affordances, results, errors#150
Conversation
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Fire-and-forget postStatus() on the persisted app.client — not the handler-scoped setStatus — so the "peek is thinking…" indicator survives the async reply chain. No-op on the /peek slash path (no thread). README adds the chat:write scope note. Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Adds `classifyError(err)` to connector-core/runtime.ts with 7 kind
branches (mcp-connection-lost, tool-error, llm-key-rejected,
llm-endpoint-error, not-recording, consent-timeout, max-turns) plus an
`unknown` default so a provider swap can never break error handling.
Rewrites the `runLoop` catch to call `adapter.postError?.(cid, classified)`
with postText fallback; removes the now-unused `ERROR_TEXT` const.
Wraps `mcp.connect()` in connector-slack/index.ts with a try/catch that
emits a clear stderr message and exits with code 1 before any Slack thread
exists.
Substring grounding: the brief's max-turns fixtures used 'maxturns'/'max
turns' which do not appear in SdkBrain's real thrown message ("SdkBrain
exceeded N tool-use turns"). Changed both the branch substring to
'tool-use turns' and the test fixture to the real string.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…rkdwn Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
@coderabbitai review |
Action performedReview triggered.
|
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesLegible errors and consent UX
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant RunLoop
participant classifyError
participant Adapter
RunLoop->>classifyError: err thrown during turn
classifyError-->>RunLoop: {kind, headline, hint}
alt adapter has postError
RunLoop->>Adapter: postError(conversationId, classified)
else
RunLoop->>Adapter: postText(conversationId, headline + hint)
end
sequenceDiagram
participant SlackAdapter
participant BlockKit
participant SlackAPI
SlackAdapter->>SlackAdapter: postStatus(conversationId, "peek is thinking…")
SlackAdapter->>SlackAPI: assistant.threads.setStatus
SlackAdapter->>BlockKit: errorBlock(headline, hint)
BlockKit-->>SlackAdapter: warning + context blocks
SlackAdapter->>SlackAPI: chat.postMessage(blocks, text=headline)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/connector-slack/src/slack-adapter.ts (1)
165-178: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
setTitlefailure silently drops the user's message.
await setTitle(m.text.slice(0, 75))is not wrapped in error handling. If the Slack API call fails (network, permissions), the promise rejects,emit()is never reached, and the user's message is lost. This is inconsistent withpostStatus, which swallows errors for the same reason — both are niceties that must never block the turn.Proposed fix: wrap setTitle in try/catch
if (!m.text || !m.channel) return; - // Thread title is handler-scoped and safe to set synchronously before emit. - await setTitle(m.text.slice(0, 75)); + // Thread title is a nicety; never block the message on it. + try { + await setTitle(m.text.slice(0, 75)); + } catch { + // Title is a nicety; a failure must never drop the user's message. + } const cid = m.thread_ts ?? m.ts; this.emit(cid, m.channel, cid, m.user ?? 'unknown', m.text);🤖 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 `@packages/connector-slack/src/slack-adapter.ts` around lines 165 - 178, The message flow in SlackAdapter’s userMessage handler can be blocked by a failing setTitle call, causing emit() to never run and the user’s message to be lost. Update the userMessage path in slack-adapter.ts to wrap await setTitle(...) in try/catch, matching the non-blocking behavior used by postStatus, and ensure any title update failure is swallowed so the code always continues to compute cid and call this.emit(...).
🧹 Nitpick comments (1)
packages/connector-core/src/runtime.test.ts (1)
466-510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the fallback text content.
The postText fallback test verifies
postTextis called with the right conversation ID but doesn't check the composed text (${headline}. ${hint}). Asserting the text contains the classified headline/hint would catch regressions in the fallback formatting.🧪 Suggested addition
await vi.waitFor(() => expect(adapter.texts).toHaveLength(1)); expect(adapter.texts[0]?.[0]).toBe('t1'); + expect(adapter.texts[0]?.[1]).toContain('Something went wrong reaching peek'); + expect(adapter.texts[0]?.[1]).toContain('Please try again'); });🤖 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 `@packages/connector-core/src/runtime.test.ts` around lines 466 - 510, The fallback branch in ConnectorRuntime’s error handling is only asserting that postText is called, but not that it uses the composed headline/hint message. Update the “falls back to postText when the adapter has no postError” test in runtime.test.ts to assert the posted text includes the classified error content (the `${headline}. ${hint}` formatting) so regressions in fallback formatting are caught. Use the existing ConnectorRuntime, FakeAdapter, and postText assertions to locate the test.
🤖 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 `@packages/connector-slack/src/blockkit.ts`:
- Around line 189-192: Mixed prose plus code is being routed to resultBlocks as
a code block whenever looksLikeCode sees any ``` marker, which causes nested
fencing in Slack. Update resultBlocks in blockkit.ts to detect fenced code
embedded inside surrounding prose and send those cases to textBlocks instead of
codeBlock, using the existing helpers looksLikeCode, codeBlock, and textBlocks
to keep standalone code unchanged.
---
Outside diff comments:
In `@packages/connector-slack/src/slack-adapter.ts`:
- Around line 165-178: The message flow in SlackAdapter’s userMessage handler
can be blocked by a failing setTitle call, causing emit() to never run and the
user’s message to be lost. Update the userMessage path in slack-adapter.ts to
wrap await setTitle(...) in try/catch, matching the non-blocking behavior used
by postStatus, and ensure any title update failure is swallowed so the code
always continues to compute cid and call this.emit(...).
---
Nitpick comments:
In `@packages/connector-core/src/runtime.test.ts`:
- Around line 466-510: The fallback branch in ConnectorRuntime’s error handling
is only asserting that postText is called, but not that it uses the composed
headline/hint message. Update the “falls back to postText when the adapter has
no postError” test in runtime.test.ts to assert the posted text includes the
classified error content (the `${headline}. ${hint}` formatting) so regressions
in fallback formatting are caught. Use the existing ConnectorRuntime,
FakeAdapter, and postText assertions to locate the test.
🪄 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 Plus
Run ID: 63bdec0b-2d79-4f21-85b8-c9517b70c2cd
📒 Files selected for processing (12)
.changeset/peek-connector-inslack-consent-text.mdpackages/connector-core/src/runtime.test.tspackages/connector-core/src/runtime.tspackages/connector-core/src/surface.tspackages/connector-slack/README.mdpackages/connector-slack/src/blockkit.test.tspackages/connector-slack/src/blockkit.tspackages/connector-slack/src/index.tspackages/connector-slack/src/slack-adapter.test.tspackages/connector-slack/src/slack-adapter.tspackages/peek-mcp/src/mcp/elicitation.test.tspackages/peek-mcp/src/mcp/elicitation.ts
…ssert fallback text (#150) - resultBlocks: already-fenced text routes to textBlocks (Slack mrkdwn renders ``` natively); only bare Playwright code gets wrapped by codeBlock once. Prevents nested-fence rendering bug for mixed prose+code LLM replies. - slack-adapter userMessage: wrap setTitle in try/catch so a title API failure never drops the user's message (emit always fires). - runtime.test: assert postText fallback contains the unknown-kind headline + hint substrings, not just the conversation id. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What
The first in-Slack product-UX pass for the peek Slack connector — how it
feels inside Slack, not the local setup flow. Four areas, no security-model,
consent-model, or daemon change.
1. Assistant affordances
"Show console errors", "What caused it?", "Make a Playwright repro") + a
greeting, and a thread title from the user's first message.
assistant.threads.setStatusat message receipt (the reply is async off theruntime turn-chain, so the Bolt handler-scoped helper would auto-clear too
early). Auto-clears when the reply posts. (Needs
chat:write— see theconnector-slack README.)
2. Human-readable, secret-masked consent card
buildElicitMessagenow emits a masked, verb-specific sentence(e.g. peek wants to Type "m•••m" into
#emailon your live browser.Approve?) instead of
run "type".consentCardrenders both consent paths: the delegated(default) path shows that masked sentence in a clean header/context/buttons
card; the suspend path shows a structured fields card.
typetext andrequest_user_inputprompt are always masked (first/last char, lengthnot leaked) — verified no plaintext can reach any block, incl. the raw-JSON
fallback (only unclassifiable non-Action payloads).
3. Tier-1 result formatting
code block. Tier-2 structured cards deliberately deferred (would break the
load-bearing brain-returns-text seam for little gain at alpha).
4. In-Slack error legibility
SurfaceAdapter.postError+errorBlock, and a defensive7-mode
classifyErrorin the runtime's turn-loop catch (mcp-down,llm-key-rejected, endpoint, not-recording, tool-error, consent-timeout,
max-turns,
unknownfallback) → actionable:warning:messages instead of ageneric catch-all. Plus an
mcp.connect()guard for a clean daemon failure.How it was built
Subagent-driven: 8 tasks, each TDD with a per-task spec+quality review, then a
whole-branch review (READY TO MERGE). The error-classifier substrings were
grounded against the real thrown messages — which caught the plan's
max-turnsstring being wrong (real:
…tool-use turns).Testing
Full local CI green:
pnpm build && typecheck && lint && test. peek-mcp 448,connector-core 113, connector-slack 44. Changeset:
@peekdev/mcppatch(user-visible consent text; connectors are private).
Spec + plan:
rrweb-stack-private/docs/{specs,plans}/2026-07-09-peek-connector-inslack-ux*.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation