Implement Mattermost plugin for Cursor Background Agents - #1
Conversation
Complete v1 implementation mirroring Cursor's Slack integration for Mattermost. Built across 6 phases with full code review and QA validation. Server (Go): - Cursor API client with retry, Basic Auth, all 9 endpoints - Message parser for @cursor mentions (repo, branch, model, options) - MessageHasBeenPosted hook for mention-driven agent launching - HA-safe background poller via cluster.Schedule - Thread follow-up detection and Cursor API forwarding - Thread context enrichment via Agents plugin bridge client - 7 slash commands: launch, list, status, cancel, settings, models, help - Interactive settings dialog for channel/user defaults - GitHub webhook receiver with HMAC-SHA256 verification - PR lifecycle notifications (merged, closed, reviewed) - Admin health endpoint with rich diagnostics - Toggleable debug logging for API troubleshooting - Configurable system prompt for Cursor agents Webapp (React/TypeScript): - Redux store with WebSocket-driven real-time updates - RHS agent dashboard with list and detail views - App Bar icon with RHS toggle - Post dropdown menu actions (follow-up, cancel, view details) - Mattermost CSS variables throughout (dark mode compatible) Documentation: - CLAUDE.md files for progressive context disclosure - 8 skills in .claude/commands/ for common development tasks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR converts the starter template into a Cursor Background Agents plugin: adds a Cursor API client and types, KV-backed store and indices, GitHub webhook handling, slash command and mention-driven agent flows, REST API and RHS UI with WebSocket events, background polling, extensive tests, and comprehensive documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Mattermost
participant Plugin
participant Parser
participant CursorAPI
participant KVStore
participant Bot
User->>Mattermost: Post message with `@cursor` ...
Mattermost->>Plugin: MessageHasBeenPosted
Plugin->>Parser: Parse(message, botMention)
Parser-->>Plugin: ParsedMention
Plugin->>Plugin: Resolve defaults & enrich prompt
Plugin->>CursorAPI: LaunchAgent(request)
CursorAPI-->>Plugin: Agent{ID, status=CREATING}
Plugin->>KVStore: SaveAgent(record)
KVStore-->>Plugin: OK
Plugin->>Bot: Post agent link in thread
Bot-->>Mattermost: Thread message posted
Plugin->>Mattermost: Publish WebSocket event (agent_created)
sequenceDiagram
participant Poller
participant KVStore
participant CursorAPI
participant Plugin
participant Bot
participant WebSocket
loop periodic poll
Poller->>KVStore: ListActiveAgents()
KVStore-->>Poller: [AgentRecords]
Poller->>CursorAPI: GetAgent(agent_id)
CursorAPI-->>Poller: Agent{status, summary, pr_url}
Poller->>Plugin: handleAgentFinished(...) (or other)
Plugin->>KVStore: SaveAgent(updated)
KVStore-->>Plugin: OK
Plugin->>Bot: Post completion in thread
Plugin->>WebSocket: Publish agent_status_changed
end
sequenceDiagram
participant GitHub
participant Plugin
participant KVStore
participant CursorAPI
participant Bot
participant Mattermost
GitHub->>Plugin: POST webhook (pull_request/review)
Plugin->>Plugin: verifyWebhookSignature()
Plugin->>KVStore: GetAgentByPRURL or GetAgentByBranch
KVStore-->>Plugin: AgentRecord
Plugin->>CursorAPI: GetAgent(id)
CursorAPI-->>Plugin: Agent{status,summary}
Plugin->>KVStore: SaveAgent(updated)
Plugin->>Bot: Post notification to thread
Plugin->>Mattermost: Swap reactions on trigger post
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In @.claude/commands/debug-cursor-api.md:
- Around line 175-219: Replace literal basic-auth placeholders (-u
"YOUR_API_KEY:") with an environment-variable pattern to avoid secret-scanning;
export a variable (e.g., CURSOR_API_KEY) and use curl -u "$CURSOR_API_KEY:" in
every example (Test API Key, List Agents, Get Agent Status, Launch Agent, List
Models, Stop Agent) so update all occurrences of the string YOUR_API_KEY and the
-u argument in the markdown examples to reference the env var instead.
In `@server/api.go`:
- Around line 368-377: The code currently proceeds to mark agents STOPPED even
when the remote Cursor client is unavailable; update the guard around
p.getCursorClient() so that if cursorClient == nil you log a clear error via
p.API.LogError (include "agentID" and context), send an appropriate http error
(e.g. http.StatusBadGateway) and return early instead of continuing to update
local state; ensure the only place that calls cursorClient.StopAgent(ctx,
agentID) remains inside the non-nil branch and that any subsequent local state
update (where you set the agent to STOPPED) is not executed unless StopAgent
succeeded.
In `@server/command/command_test.go`:
- Around line 834-853: There are two conflicting implementations of
sanitizeBranchName (one used in command.go and one in handlers.go) with
different truncation limits and prefix behavior; consolidate them into a single
canonical function sanitizeBranchName that takes an optional maxLen (or a
constant) and a boolean flag for whether to include the "cursor/" prefix, then
replace both existing copies with calls to this single implementation (or export
a wrapper like sanitizeBranchNameWithPrefix) so truncation limit and prefixing
are consistent across usages (ensure tests are updated to use the chosen max
length and prefix semantics and remove the duplicate implementation).
In `@server/command/command.go`:
- Around line 218-227: The SaveAgent call in the /cursor command path
(h.deps.Store.SaveAgent with kvstore.AgentRecord) is missing Prompt, Model,
TargetBranch, CreatedAt and UpdatedAt, causing incomplete records; update the
AgentRecord construction used when saving (the call around agent, botPost, args,
repo, branch) to include Prompt (agent.Prompt or args-provided prompt), Model
(agent.Model), TargetBranch (branch or agent.TargetBranch), and set
CreatedAt/UpdatedAt to the current timestamp (matching the pattern used in
handlers.go where those fields are populated) so command-launched agents persist
the same fields as the `@mention` path.
In `@server/cursor/client.go`:
- Around line 112-244: The doRequest function currently retries all HTTP methods
on 429/5xx which can cause duplicate non-idempotent POST operations (affecting
LaunchAgent, AddFollowup, StopAgent); change the retry logic in doRequest to
only perform automatic retries for idempotent methods (e.g., GET, DELETE, HEAD,
OPTIONS, and optionally PUT) by checking the request method before treating
429/5xx as retryable, and for non-idempotent methods (POST, PATCH) return the
API error immediately instead of continuing the loop; update the retry-related
log messages to reflect the method-based decision and add/adjust tests for
LaunchAgent/AddFollowup/StopAgent to ensure POSTs are not retried by doRequest.
In `@server/handlers.go`:
- Around line 695-706: sanitizeBranchName can return "cursor/" when the slug is
empty (prompt was only non-alphanumerics), which is invalid; update the
sanitizeBranchName function to detect when the post-processed slug is empty and
replace it with a safe fallback (for example "untitled" or a short unique token
like "branch-<timestamp>" or a short hash of the original prompt) before
prefixing with "cursor/"; ensure the fallback is alphanumeric/hyphen-only and
still trimmed/validated so the function always returns a valid branch name.
In `@server/plugin.go`:
- Around line 168-176: The command handler (p.commandHandler) is initialized
with a snapshot of p.cursorClient via
command.NewHandler(command.Dependencies{...}) and therefore keeps a stale client
after setCursorClient/OnConfigurationChange updates p.cursorClient; reinitialize
p.commandHandler whenever the cursor client is changed (e.g., in
OnConfigurationChange after calling setCursorClient()) by recreating it with the
updated p.cursorClient, or refactor the handler to call back to p.cursorClient
dynamically instead of storing the client at construction (refer to
p.commandHandler, command.NewHandler, command.Dependencies, setCursorClient, and
OnConfigurationChange).
In `@server/webhook.go`:
- Around line 252-257: Don't write non-Cursor strings into AgentRecord.Status;
instead add a separate field on AgentRecord (e.g., PRState or PRLifecycle) and
set that to "MERGED" or "PR_CLOSED" from event.PullRequest.Merged, and then set
AgentRecord.Status to a valid Cursor terminal value (use your existing
cursor-normalization helper or map to the closest Cursor enum) so UI rendering,
status→emoji mapping and IsTerminal checks continue to work; update code
references that read PR lifecycle to use the new PRState field.
- Around line 153-183: The code marks deliveries processed unconditionally which
can drop events if handlers fail; change the flow so
MarkDeliveryProcessed(deliveryID) is called only after a successful handler
response: either (a) modify handlers handlePingEvent, handlePullRequestEvent,
handlePullRequestReviewEvent to return an error or status code and call
MarkDeliveryProcessed only on success, or (b) wrap http.ResponseWriter with a
small recorder to capture the final status code after calling the handler and
call MarkDeliveryProcessed only when the recorded status is 2xx; also keep using
HasDeliveryBeenProcessed to skip duplicates before handling.
In `@webapp/src/components/common/styles.css`:
- Around line 16-197: The file uses legacy rgba() notation in several CSS
classes (e.g., .cursor-status-finished, .cursor-status-stopped,
.cursor-rhs-empty, .cursor-rhs-empty-hint, .cursor-agent-card,
.cursor-agent-card:hover, .cursor-agent-card-time, .cursor-agent-card-prompt,
.cursor-agent-detail-label, .cursor-agent-detail-followup,
.cursor-agent-detail-cancel, .cursor-textarea::placeholder); replace each
rgba(var(--center-channel-color-rgb), alpha) with the modern slash syntax
rgb(var(--center-channel-color-rgb) / alpha) (and similarly for other rgba uses
like rgba(var(--center-channel-color-rgb), 0.04) →
rgb(var(--center-channel-color-rgb) / 0.04) and
rgba(var(--center-channel-color-rgb), 0.56) →
rgb(var(--center-channel-color-rgb) / 0.56)) so the styles conform to the
color-function-notation:"modern" rule while keeping the same variables and alpha
values.
🟡 Minor comments (12)
CLAUDE.md-22-47 (1)
22-47:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced block to satisfy MD040.
🔧 Minimal markdownlint fix
-``` +```text server/ plugin.go # Plugin struct, OnActivate, ServeHTTP, ExecuteCommand configuration.go # Config struct, OnConfigurationChange, validation handlers.go # MessageHasBeenPosted, agent launch flow, follow-ups, thread enrichment api.go # HTTP router (gorilla/mux), REST endpoints, middleware poller.go # Background agent status polling via cluster.Schedule dialog.go # Settings dialog submission handler webhook.go # GitHub webhook receiver (HMAC verification, PR events) command/command.go # /cursor slash command handler (list, status, cancel, settings, models, help) cursor/client.go # Cursor API HTTP client (interface-based) cursor/types.go # Cursor API request/response types parser/parser.go # `@cursor` mention parser (repo, branch, model, prompt extraction) store/kvstore/ # KV store interface + implementation webapp/src/ index.tsx # Plugin registration (reducer, RHS, app bar, post actions, WS handlers) reducer.ts # Redux reducer for plugin state actions.ts # Action creators (sync + thunks) selectors.ts # Redux selectors types.ts # TypeScript interfaces client.ts # REST client for plugin API websocket.ts # WebSocket event handler registration components/rhs/ # RHS panel components (RHSPanel, AgentList, AgentCard, AgentDetail) components/common/ # StatusBadge, styles.cssserver/parser/parser.go-3-6 (1)
3-6:⚠️ Potential issue | 🟡 MinorInvalid
autoprvalues silently disable AutoPR. Consider only settingAutoPRwhen the value parses cleanly so typos don’t override defaults.🔧 Suggested change
import ( "regexp" + "strconv" "strings" ) @@ case "autopr": - b := strings.EqualFold(value, "true") - result.AutoPR = &b + if b, err := strconv.ParseBool(value); err == nil { + result.AutoPR = &b + } } }Also applies to: 147-159
server/parser/CLAUDE.md-22-49 (1)
22-49:⚠️ Potential issue | 🟡 MinorAdd language tags to fenced examples. This satisfies markdownlint and keeps rendering consistent.
✏️ Example change (apply similarly to other plain-text fences)
-``` +```text `@cursor` in backend-api, fix the auth issue -> Repository: "backend-api" `@cursor` in org/repo, fix the auth issue -> Repository: "org/repo" `@cursor` with opus, fix the login bug -> Model: "opus" `@cursor` in org/repo, with opus, fix it -> Repository + Model -``` +```.claude/commands/modify-agent-prompt.md-7-31 (1)
7-31:⚠️ Potential issue | 🟡 MinorAdd a language tag to the pipeline code fence. This satisfies markdownlint and improves rendering consistency.
✏️ Suggested change
-``` +```text User Message | v Parser (server/parser/parser.go) @@ Cursor API LaunchAgent Request |-- prompt.text = wrapped prompt |-- prompt.images = extracted thread images -``` +```server/dialog.go-16-34 (1)
16-34:⚠️ Potential issue | 🟡 MinorAdd authentication check against
Mattermost-User-IDheader to match request.UserId for defense-in-depth. The endpoint is already protected byMattermostAuthorizationRequiredmiddleware that validates theMattermost-User-IDheader. However, the handler should additionally verify that the authenticated user matches the request'sUserIdto prevent any potential confusion between the authenticated identity and the request payload. While the current check againstrequest.Stateprovides protection (since State is server-controlled), comparing against the authenticated user from the header is a security best practice.Suggested check
func (p *Plugin) handleSettingsDialogSubmission(w http.ResponseWriter, r *http.Request) { + authUserID := r.Header.Get("Mattermost-User-ID") + if authUserID == "" { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + var request model.SubmitDialogRequest if err := json.NewDecoder(r.Body).Decode(&request); err != nil { http.Error(w, "invalid request", http.StatusBadRequest) return } parts := strings.SplitN(request.State, "|", 2) if len(parts) != 2 { http.Error(w, "invalid state", http.StatusBadRequest) return } channelID := parts[0] userID := parts[1] - if request.UserId != userID { + if request.UserId != userID || request.UserId != authUserID { http.Error(w, "unauthorized", http.StatusForbidden) return }webapp/src/components/rhs/AgentDetail.tsx-96-105 (1)
96-105:⚠️ Potential issue | 🟡 MinorUse a semantic label for the follow-up textarea.
This improves screen reader association and form accessibility.
♿ Suggested accessibility tweak
- {agent.status === 'RUNNING' && ( + {agent.status === 'RUNNING' && ( <div className='cursor-agent-detail-followup'> - <div className='cursor-agent-detail-label'>{'Send Follow-up'}</div> + <label + className='cursor-agent-detail-label' + htmlFor='cursor-followup-textarea' + > + {'Send Follow-up'} + </label> <textarea + id='cursor-followup-textarea' className='cursor-textarea' placeholder='Enter follow-up instructions...' value={followupText} onChange={(e) => setFollowupText(e.target.value)} rows={3} />webapp/src/components/common/StatusBadge.tsx-17-19 (1)
17-19:⚠️ Potential issue | 🟡 MinorAdd accessible label for the status indicator.
The status badge is color-only, particularly in the agent card view where status text isn't adjacent. Add anaria-labelto ensure screen reader users can identify the agent status.♿ Suggested accessibility fix
- return <span className={`cursor-status-badge ${className}`} title={status}/>; + return ( + <span + className={`cursor-status-badge ${className}`} + title={status} + aria-label={`Agent status: ${status}`} + role='img' + /> + );plugin.json-57-91 (1)
57-91:⚠️ Potential issue | 🟡 MinorChange settings_schema default values from strings to JSON literals.
Mattermost settings_schema expects defaults to match the native JSON type:boolfields require boolean literals (true/false), andnumberfields require numeric literals. String defaults may not apply correctly. Official Mattermost plugins (GitHub, Calls) use JSON literals for these types.🔧 Required changes
{ "key": "AutoCreatePR", "display_name": "Auto-Create Pull Requests", "type": "bool", "help_text": "When enabled, agents automatically create pull requests on completion. Users can override per-message with autopr=false.", - "default": "true" + "default": true }, { "key": "PollIntervalSeconds", "display_name": "Status Poll Interval (seconds)", "type": "number", "help_text": "How often the plugin checks for agent status changes. Minimum 10 seconds. Lower values increase API usage.", - "default": "30", + "default": 30, "placeholder": "30" }, { "key": "EnableDebugLogging", "display_name": "Enable Debug Logging", "type": "bool", "help_text": "When enabled, the plugin logs detailed debug information including API request/response bodies. Disable in production to reduce log noise.", - "default": "false" + "default": false }webapp/src/reducer.ts-21-26 (1)
21-26:⚠️ Potential issue | 🟡 MinorClear stale selection after full agent refresh.
When the agents map is rebuilt, a previously selected ID might no longer exist, which can leave selectors with
undefineddata. Consider nulling the selection if it’s not present in the new map.🔧 Suggested fix
case AGENTS_RECEIVED: { const agents: Record<string, Agent> = {}; for (const agent of action.data) { agents[agent.id] = agent; } - return {...state, agents}; + const selectedAgentId = + state.selectedAgentId && agents[state.selectedAgentId] ? state.selectedAgentId : null; + return {...state, agents, selectedAgentId}; }webapp/src/components/rhs/AgentCard.tsx-28-66 (1)
28-66:⚠️ Potential issue | 🟡 MinorAdd Space-key activation for keyboard accessibility.
With
role="button", keyboard users expect both Space and Enter to activate the button. Currently only Enter is handled, violating WAI-ARIA Button Pattern requirements.♿ Suggested fix
onKeyDown={(e) => { - if (e.key === 'Enter') { - onClick(); - } + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick(); + } }}.claude/commands/add-webhook-event.md-97-104 (1)
97-104:⚠️ Potential issue | 🟡 MinorExample code references undefined field.
The handler example accesses
event.PullRequeston line 99, but theCheckRunEventstruct defined in Step 2 (lines 38-48) doesn't include aPullRequestfield. This would cause a compile error if developers follow the example literally.For
check_runevents, the PR information is typically nested undercheck_run.pull_requests(an array). Either update the struct definition to include the relevant fields, or adjust the lookup strategy in the example.Suggested fix for the struct definition
type CheckRunEvent struct { Action string `json:"action"` CheckRun struct { Name string `json:"name"` Status string `json:"status"` Conclusion string `json:"conclusion"` HTMLURL string `json:"html_url"` + PullRequests []struct { + URL string `json:"url"` + } `json:"pull_requests"` } `json:"check_run"` Repository ghRepository `json:"repository"` Sender ghSender `json:"sender"` }Then update the lookup to iterate over
event.CheckRun.PullRequestsor look up by branch instead.server/handlers.go-353-366 (1)
353-366:⚠️ Potential issue | 🟡 MinorRemove the eyes reaction when the follow-up text is empty.
Right now
sendFollowUpreturns early and leaves the “eyes” reaction in place, which reads like an acknowledged follow-up even though nothing was sent.🧹 Suggested fix
followUpText = strings.TrimSpace(followUpText) if followUpText == "" { + p.removeReaction(post.Id, "eyes") return }
🧹 Nitpick comments (10)
.claude/commands/add-rhs-feature.md (1)
7-23: Consider referencingSTYLE_GUIDE.mdfor UI consistency.Based on learnings: "Reference `STYLE_GUIDE.md` for colors, typography, spacing, component patterns, and motion guidelines when building new UI components."📌 Suggested addition
Key files: - `webapp/src/index.tsx` -- Plugin registration (reducer, RHS, app bar, post actions, websocket) - `webapp/src/reducer.ts` -- Redux reducer - `webapp/src/actions.ts` -- Action type constants, action interfaces, sync/async action creators - `webapp/src/selectors.ts` -- Redux selectors - `webapp/src/types.ts` -- TypeScript interfaces - `webapp/src/client.ts` -- HTTP client for plugin API calls - `webapp/src/websocket.ts` -- WebSocket event handler registration - `webapp/src/components/rhs/` -- RHS panel components - `webapp/src/components/common/` -- Shared components and CSS + - `STYLE_GUIDE.md` -- Colors, typography, spacing, component patterns, motion guidelineswebapp/src/components/rhs/RHSPanel.tsx (1)
11-19: Create a typed dispatch hook to eliminateas anycasting. Define auseAppDispatchhook that properly types the dispatch to accept thunks, removing the need for type assertions throughout the codebase. This would improve type safety across RHSPanel and other components that dispatch async actions.webapp/src/index.tsx (1)
13-14: Consider using more specific types for action references.The
objecttype forrhsToggleActionandrhsShowActionis very permissive. Based on theregisterRightHandSidebarComponentreturn type in the type definitions, these could be typed more specifically, thoughobjectis acceptable given the opaque nature of Redux actions.server/cursor/client_test.go (1)
350-364: Potential test resource leak on assertion failure.The test server handler uses
select {}which blocks forever. If the test assertions fail before the context cancellation propagates, the goroutine will leak until the test process exits. This is generally acceptable for tests but worth noting.Consider adding a timeout to the context or the select to make debugging easier if this test becomes flaky:
select { case <-r.Context().Done(): return case <-time.After(5 * time.Second): t.Error("handler was not cancelled") }server/command/command_test.go (1)
19-193: Duplicate mock implementations across test files.The
mockCursorClientandmockKVStoreimplementations are duplicated fromserver/handlers_test.go. Consider extracting these into a sharedtestutilormockspackage to reduce duplication and ensure consistency.Suggested approach
Create a shared test utilities package:
server/ testutil/ mocks.go // Contains mockCursorClient, mockKVStoreThen import in both test files:
import "github.com/mattermost/mattermost-plugin-cursor/server/testutil"This ensures mock implementations stay in sync and reduces maintenance burden.
server/store/kvstore/store.go (3)
71-85: Silent failures in index updates could cause data inconsistency.The secondary index updates for user, PR URL, and branch all ignore errors with
_, _. While this prevents index failures from blocking the primary save, it could lead to data inconsistency where lookups by these indices fail to find records that exist.Consider at minimum logging these errors for debugging:
Suggested improvement
// Maintain a per-user agent index for GetAgentsByUser. if record.UserID != "" { key := prefixUserAgentIdx + record.UserID + ":" + record.CursorAgentID - _, _ = s.client.KV.Set(key, record.CursorAgentID) + if _, err := s.client.KV.Set(key, record.CursorAgentID); err != nil { + // Log but don't fail - index is secondary + _ = err // TODO: Add logging when logger is available + } }Alternatively, if index consistency is critical, consider returning a multi-error that captures all failures.
107-126: ListKeys has a hardcoded limit of 1000.The
GetAgentsByUserandListActiveAgentsfunctions use a hardcoded limit of 1000 keys. For deployments with many agents, this could silently truncate results.Consider either:
- Documenting this limitation
- Implementing pagination
- Making the limit configurable
197-204: GetChannelSettings returns empty struct when not found.When the channel settings key doesn't exist,
GetChannelSettingsreturns an emptyChannelSettings{}struct rather thannil. This differs fromGetAgentwhich returnsnilwhen not found.This inconsistency could cause confusion - callers might check
settings.DefaultRepository == ""to detect "not found" vs "found but empty". Consider aligning the behavior withGetAgent:Suggested fix for consistency
func (s *store) GetChannelSettings(channelID string) (*ChannelSettings, error) { var settings ChannelSettings err := s.client.KV.Get(prefixChannel+channelID, &settings) if err != nil { return nil, errors.Wrap(err, "failed to get channel settings") } + // Return nil if not found (all fields are zero values) + if settings.DefaultRepository == "" && settings.DefaultBranch == "" { + return nil, nil + } return &settings, nil }Apply the same pattern to
GetUserSettings.server/handlers.go (1)
544-613: Consider caching display names per thread to reduce API calls.
getDisplayNamecan be invoked for every post; a simple per-thread cache avoids repeatedGetUserlookups for the same author.♻️ Optional caching tweak
func (p *Plugin) formatThread(postList *model.PostList) (string, []cursor.Image) { order := postList.Order posts := postList.Posts + displayNameCache := map[string]string{} // Sort by CreateAt for chronological order. sort.Slice(order, func(i, j int) bool { return posts[order[i]].CreateAt < posts[order[j]].CreateAt }) @@ threadPost := posts[postID] // Get display name for the author. - displayName := p.getDisplayName(threadPost.UserId) + displayName, ok := displayNameCache[threadPost.UserId] + if !ok { + displayName = p.getDisplayName(threadPost.UserId) + displayNameCache[threadPost.UserId] = displayName + } sb.WriteString(fmt.Sprintf("[%s]: %s\n", displayName, threadPost.Message))server/api.go (1)
204-222: Remove unusedRootPostIDor populate it if needed.
AgentResponseincludesRootPostID, but it's never set in API responses and the webapp doesn't use it—it only appears in type definitions and test initialization where it's set to empty string. Either remove the field or populate it witha.PostID/record.PostIDif intended for future use.
- Fix import ordering across all files (import/order) - Replace raw <a target="_blank"> with ExternalLink component - Fix operator-linebreak, dot-notation, quote-props, naming-convention - Remove unused _dispatch parameters from thunks - Fix jsx-max-props-per-line in StatusBadge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@webapp/src/actions.ts`:
- Around line 120-129: The websocketAgentStatusChange action creator currently
uses parseInt(data.updated_at, 10) which can return NaN for invalid timestamp
strings; update websocketAgentStatusChange to validate the parsed value (e.g.,
const ts = parseInt(...); if (Number.isNaN(ts)) use a safe fallback like
Date.now() or 0) and assign that validated/fallback value to updated_at, and
apply the same validation/fallback pattern to the other similar handlers
referenced in the file (the other action creators around lines 131-149) so
invalid timestamps never enter Redux state.
In `@webapp/src/components/rhs/AgentCard.tsx`:
- Around line 42-46: The onKeyDown handler in AgentCard only triggers on Enter,
so update the onKeyDown in the AgentCard component to also detect Space (handle
e.key === ' ' and legacy values like 'Spacebar' or 'Space') and call onClick()
when Space is pressed; also call e.preventDefault() when handling Space to stop
page scrolling and ensure the element with role='button' is fully keyboard
accessible.
🧹 Nitpick comments (3)
webapp/src/actions.ts (1)
108-116: Consider dispatching state update after successful cancellation.
cancelAgentdoesn't dispatch any action on success, so the UI won't reflect the cancellation until the next WebSocket event or manual refresh. Consider optimistically updating the agent status or refreshing the agent data after the API call succeeds.♻️ Proposed enhancement to update state on success
export function cancelAgent(agentId: string) { - return async () => { + return async (dispatch: (action: PluginAction) => void) => { try { await Client.cancelAgent(agentId); + // Refresh agent to get updated status + const agent = await Client.getAgent(agentId); + dispatch({type: AGENT_RECEIVED, data: agent}); } catch (error) { console.error('Failed to cancel agent:', error); // eslint-disable-line no-console } }; }webapp/src/index.tsx (2)
14-15:rhsToggleActionis stored but never used.The
rhsToggleActionfield is assigned but never referenced elsewhere in the file. OnlyrhsShowActionis used (in the "View Agent Details" action). Consider removing the unused field if it's not planned for future use.🧹 Proposed cleanup
export default class Plugin { - private rhsToggleAction: object | null = null; private rhsShowAction: object | null = null;And update the destructuring:
- const {toggleRHSPlugin, showRHSPlugin} = registry.registerRightHandSidebarComponent( + const {showRHSPlugin} = registry.registerRightHandSidebarComponent( RHSPanel, 'Cursor Agents', ); - this.rhsToggleAction = toggleRHSPlugin; this.rhsShowAction = showRHSPlugin;
29-35: Note:toggleRHSPluginis used in App Bar registration.Looking more closely,
toggleRHSPluginis used in the App Bar callback (line 32), so whilerhsToggleActionfield is unused, thetoggleRHSPluginvariable itself is needed. The field storage is indeed unnecessary.
- Fix registerPostDropdownMenuAction type signature to match runtime API - Cast reducer and selectAgent dispatches for Redux 5 UnknownAction compat - All tsc --noEmit, ESLint, tests, and build pass Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webapp/src/types/mattermost-webapp/index.d.ts`:
- Around line 527-537: The declared signature for registerPostDropdownMenuAction
incorrectly types the filter as (postId: string) => boolean; change both
overloads so the filter parameter is (post: Post) => boolean to match the real
API and SDK docs, keeping action as (postId: string) => void; update the type
reference to use the existing Post type (import or fully-qualified name used
elsewhere in this d.ts) so consumers receive the actual Post object instead of a
postId.
🧹 Nitpick comments (3)
webapp/src/index.tsx (3)
14-15: Remove unusedrhsToggleActionfield.
rhsToggleActionis assigned at line 26 but never used. OnlyrhsShowActionis referenced (line 113). Consider removing the unused field to reduce confusion.🧹 Suggested cleanup
export default class Plugin { - private rhsToggleAction: object | null = null; private rhsShowAction: object | null = null;And at line 26:
- const {toggleRHSPlugin, showRHSPlugin} = registry.registerRightHandSidebarComponent( + const {showRHSPlugin} = registry.registerRightHandSidebarComponent( RHSPanel, 'Cursor Agents', ); - this.rhsToggleAction = toggleRHSPlugin; this.rhsShowAction = showRHSPlugin;
54-76: Consider user feedback when agent ID is missing.If the user provides follow-up text but
agentIdis not found (lines 62-64), the action silently does nothing. Consider showing an error notification to the user in this edge case.
52-123: Consider extracting repeated state access into a helper.The pattern
state.entities?.posts?.posts?.[postId]appears six times throughoutregisterPostActions. Extracting this to a small helper function would reduce duplication and centralize the state shape dependency.♻️ Optional helper extraction
// At module level or as a private method const getPost = (state: any, postId: string) => state.entities?.posts?.posts?.[postId]; // Usage in actions: const post = getPost(store.getState(), postId);
- Check all error returns from Body.Close, Write, Encode, Decode - Fix gofmt formatting in configuration.go, plugin.go, handlers_test.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dernize) - Check all remaining unchecked error returns (Encode, Close, Write) - Fix gofmt formatting in api_test.go, plugin.go, poller.go - Lowercase error strings per staticcheck ST1005 - Remove unnecessary fmt.Sprintf per staticcheck S1039 - Add nolint:gosec for test webhook secret constant - Use strings.Cut and strings.SplitSeq per modernize linter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Accepted fixes: - CR-1: Replace literal API key in curl examples with env var pattern - CR-2: Return HTTP 502 when cancelling agent with nil Cursor client - CR-3+6: Consolidate duplicate sanitizeBranchName, add empty slug fallback - CR-4: Populate missing fields in /cursor command agent save - CR-7: Use CursorClientFn getter to avoid stale client reference - CR-8: Only mark webhook delivery processed after handler succeeds - CR-10: Fix deprecated word-break: break-word -> overflow-wrap: anywhere - CR-11: Add parseTimestamp helper with NaN fallback for WebSocket events - CR-12: Add Space key handling for WCAG keyboard accessibility Rejected with PR replies: - CR-5: POST retries are intentional (idempotent operations, no retry keys) - CR-9: MERGED/PR_CLOSED statuses are intentional (extended lifecycle tracking) - CR-10 (partial): rgba() with CSS vars is incompatible with modern slash syntax - CR-13: filter param receives postId at runtime, not Post object Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Complete v1 implementation of the Mattermost plugin for Cursor Background Agents, mirroring the Cursor Slack integration feature-for-feature.
Built across 6 phases, each with dedicated planning, implementation, code review, and remediation. QA-validated end-to-end on a live Mattermost server.
Test plan
go test ./server/...— 5 packages)npm test— 14 tests, 3 suites)go vetclean/cursor help,/cursor list,/cursor settingsverified working🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests