Add PR Review Comments Dashboard to RHS sidebar#971
Add PR Review Comments Dashboard to RHS sidebar#971nickmisasi wants to merge 2 commits intomasterfrom
Conversation
Implement a drill-down detail view in the RHS that shows a PR's review comments grouped by file with diff snippets, enabling replying, reacting, resolving threads, and assigning selected feedback to AI agents — all without leaving Mattermost. Backend: - Add GraphQL query/mutation structs for review threads with pagination - Add executeMutation method to GraphQL client - Add 6 new API endpoints: pr-review-threads, pr-review-comment-reply, pr-review-comment-reaction, pr-resolve-thread, pr-create-comment, and plugin-settings/ai-agents - Add AI Agents configuration (Name:@mention pairs) with Claude/Cursor defaults Frontend: - Add Redux infrastructure: action types, reducers, selectors, API client methods, and action creators for review thread management - Add drill-down navigation from PR list items to detail view - Add PRReviewDetail container with file-grouped thread display - Add DiffHunkDisplay with syntax-highlighted diff rendering - Add ReviewComment with author info, timestamps, and reaction bar - Add ReplyBox for inline comment replies - Add ResolveThread toggle for thread resolution via GraphQL - Add AIAssignBar for selecting comments and assigning to AI agents - Add ReactionBar with optimistic toggle for 6 emoji reactions https://claude.ai/code/session_01DpvWCC92Q1UUuM1AA52THr
nickmisasi
added
the
Setup Cloud Test Server
|
Creating a Plugin SpinWick test server |
🟡 Please give this pull request extra attention during review.This pull request introduces two risky client-side issues: a prototype pollution DoS in webapp/src/selectors.ts where unsanitized thread.path can be a reserved property (e.g., proto) and cause a crash when .push is called, and a Trojan Source vulnerability in webapp/src/components/.../diff_hunk_display.tsx where diff hunks with Unicode bidirectional control characters are rendered unsanitized, allowing maliciously reordered code to be displayed to reviewers. Both issues come from trusting external PR file paths and raw diff content and should be mitigated by validating/sanitizing keys and stripping or highlighting Bidi characters.
🟡 Prototype Pollution (DoS) in
|
| Vulnerability | Prototype Pollution (DoS) |
|---|---|
| Description | The getThreadsGroupedByFile selector in webapp/src/selectors.ts uses the unsanitized thread.path as an object key on a plain JavaScript object. Since thread.path is sourced from file paths in a GitHub repository, an attacker who can create files in that repository (e.g., through a PR or direct commit) can name a file with reserved property names like __proto__, constructor, or toString. When the selector processes a review thread on such a file, it will access the corresponding property on Object.prototype. The subsequent call to .push(thread) on this prototype property will throw a TypeError because these properties are not arrays and do not have a push method, resulting in a client-side Denial of Service (DoS) and crashing the PR review sidebar. |
mattermost-plugin-github/webapp/src/selectors.ts
Lines 89 to 92 in d5f6519
🟡 Deceptive Code Display (Trojan Source) in webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsx (1e03608f)
| Vulnerability | Deceptive Code Display (Trojan Source) |
|---|---|
| Description | The DiffHunkDisplay component in the Mattermost GitHub plugin renders code diffs directly from the GitHub API without sanitizing or highlighting Unicode Bidirectional (Bidi) control characters (e.g., U+202E, U+2067). This lack of sanitization allows for 'Trojan Source' attacks, where an attacker can craft a pull request containing malicious code that appears benign in the review UI by using Bidi characters to reorder the displayed text. Because the backend Go code also passes the raw diffHunk data to the frontend without any filtering, a reviewer might approve a malicious change that looks safe in the Mattermost plugin sidebar. |
All finding details can be found in the DryRun Security Dashboard.
|
Plugin Spinwick PR #971 🎉 Test server created! Access here: https://github-pr-971-3771b.test.mattermost.cloud
The test server was created successfully, but there was an issue installing or enabling the plugin automatically:
You can manually install the plugin:
Future commits will still attempt to automatically update the plugin. Installation ID: Credentials: Posted securely in this Mattermost channel - Look for PR #971 |
- Fix gofmt struct alignment in ReviewCommentResponse - Fix G115 integer overflow: add bounds check before int->int32 conversion - Fix modernize: use WaitGroup.Go instead of Add+go func+Done pattern - Fix modernize: use strings.SplitSeq instead of strings.Split for ranging - Fix S1016: use type conversion AIAgentResponse(a) instead of struct literal https://claude.ai/code/session_01DpvWCC92Q1UUuM1AA52THr
|
New commit detected. SpinWick will update the plugin if a new artifact is available. |
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
nickmisasi
added
the
Do Not Merge
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces GitHub PR review thread management with AI agent assignment capabilities. It adds backend API endpoints to fetch and manipulate review threads, GraphQL integration for thread data retrieval, Redux state management for PR context and thread data, and a detailed review sidebar component with thread visualization, comment interactions, and AI agent selection. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant React as React Components
participant Redux as Redux Store
participant Client as API Client
participant Backend as Backend API
participant GraphQL as GraphQL API
User->>React: Click PR in GithubItems
React->>Redux: dispatch selectPR(prData)
Redux->>Redux: Store selectedPR
React->>React: Render PRReviewDetail
React->>Redux: dispatch getPRReviewThreads()
Redux->>Client: client.getPRReviewThreads()
Client->>Backend: GET /pr-review-threads
Backend->>GraphQL: Execute GetReviewThreads query
GraphQL-->>Backend: Threads, comments, reactions
Backend-->>Client: PRReviewThreadsData
Client-->>Redux: Response
Redux->>Redux: Store threads & loading state
Redux-->>React: Update selectedPR, threads, aiAgents
React->>React: Render FileGroup & ReviewThread list
User->>React: Click reaction on comment
React->>Redux: dispatch toggleReaction()
Redux->>Client: client.toggleReaction()
Client->>Backend: POST /pr-review-comment-reaction
Backend->>GraphQL: GitHub API mutation
GraphQL-->>Backend: Updated reaction state
Backend-->>React: ToggleReactionResponse
React->>React: Update reaction display
User->>React: Select comments & click Assign
React->>Redux: dispatch postAIAssignment()
Redux->>Client: client.createPRComment()
Client->>Backend: POST /pr-create-comment
Backend->>GraphQL: GitHub API create comment
GraphQL-->>Backend: Created comment
Backend-->>React: Success response
React->>React: Clear selections, refresh threads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters 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: 9
🧹 Nitpick comments (6)
webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsx (1)
25-32: Prefer theme-aware colours instead of fixed RGBA/hex values.Hardcoded diff colours and text colours can be hard to read in some themes. Consider deriving these from theme tokens to keep contrast consistent.
Also applies to: 68-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsx` around lines 25 - 32, The styling currently returns hardcoded color objects for diff lines (the branches checking line.startsWith('+'), line.startsWith('-') and the default) — update these to use theme tokens from your styling system (e.g., Material-UI/theme, styled-components theme, or CSS variables) instead of literal 'rgba(...)' or '#555'; locate the function in diff_hunk_display.tsx that produces the inline style for each line (the returns shown) and replace them with values derived from the active theme (e.g., theme.palette.success.light, theme.palette.error.light, theme.palette.action.hover or a dedicated token for diff additions/deletions and text color) so colours adapt to light/dark themes and maintain contrast; ensure you import/use the theme context (useTheme or ThemeProvider tokens) and fall back to accessible defaults if the theme token is missing.server/plugin/configuration.go (1)
311-315: De-duplicate configured agents by mention before appending.Right now duplicates are allowed, which can produce repeated agent entries and duplicate React keys in the agent selector.
♻️ Proposed refinement
func (c *Configuration) ParseAIAgents() []AIAgent { defaults := []AIAgent{ {Name: "Claude", Mention: "@claude", IsDefault: true}, {Name: "Cursor", Mention: "@cursor", IsDefault: true}, } @@ - agents := make([]AIAgent, 0, len(defaults)) - agents = append(agents, defaults...) + agents := make([]AIAgent, 0, len(defaults)) + agents = append(agents, defaults...) + seen := map[string]struct{}{} + for _, d := range defaults { + seen[d.Mention] = struct{}{} + } @@ - agents = append(agents, AIAgent{ + if _, exists := seen[mention]; exists { + continue + } + seen[mention] = struct{}{} + agents = append(agents, AIAgent{ Name: name, Mention: mention, IsDefault: false, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/configuration.go` around lines 311 - 315, The agents slice is being appended with AIAgent entries without deduplicating by the Mention field, causing duplicate agents and React key collisions; update the code that builds agents (where agents = append(agents, AIAgent{ Name: name, Mention: mention, IsDefault: false })) to skip appending if that mention already exists — e.g., maintain a local seenMentions map[string]bool, check seenMentions[mention] before append, and mark seenMentions[mention]=true when you append so each AIAgent (by Mention) is only added once.webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail_header.tsx (1)
29-53: Localize header copy instead of hardcoding English strings.The visible labels are currently embedded literals; moving them to the i18n path will keep this view consistent in non-English workspaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail_header.tsx` around lines 29 - 53, Replace hardcoded English UI strings in pr_review_detail_header.tsx with i18n keys and calls so the header is localizable: import and use the project's translation helper (e.g., t from react-i18next or your i18n.translate) and change literals like '\u2190 Back', the label concatenation "{summary.approved + ' approved'}", "{summary.changes_requested + ' changes requested'}", and "{summary.unresolved_threads + ' unresolved threads'}" to t('prReview.header.back'), t('prReview.header.approved', {count: summary.approved}), t('prReview.header.changesRequested', {count: summary.changes_requested}), and t('prReview.header.unresolvedThreads', {count: summary.unresolved_threads}) (or your app's interpolation syntax); then add those keys to the i18n resource files under the appropriate namespace so translations are available.webapp/src/components/sidebar_right/pr_review_detail/reply_box.tsx (1)
29-43: Missing error handling for failed submissions.If
onSubmitthrows or rejects, the input text is cleared and the textarea collapses (lines 38-39), leaving the user with no feedback about the failure and losing their typed reply. Consider adding a catch block to preserve the text and display an error state.♻️ Proposed fix to handle submission errors
const handleSubmit = useCallback(async () => { const body = text.trim(); if (!body || submitting) { return; } setSubmitting(true); try { await onSubmit(body); setText(''); setExpanded(false); + } catch (error) { + // Keep text so user can retry; optionally show error feedback + console.error('Failed to submit reply:', error); } finally { setSubmitting(false); } }, [text, submitting, onSubmit]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/sidebar_right/pr_review_detail/reply_box.tsx` around lines 29 - 43, The submit handler clears the textarea and collapses the box even when onSubmit fails; update handleSubmit (in reply_box.tsx) to wrap the await onSubmit(body) in a try/catch so that on rejection you do NOT call setText('') or setExpanded(false), instead set an error state (e.g., setSubmitError / setError) and keep the text/expansion so the user can retry; keep the finally block to setSubmitting(false) and ensure the new error state is included/cleared appropriately when the user edits or retries.server/plugin/api.go (2)
2214-2217: Direct struct conversion may be fragile if underlying type changes.The type conversion
AIAgentResponse(a)works because both types have identical fields in the same order. Consider explicit field assignment for clarity and resilience to future changes.♻️ Suggested explicit conversion
agentList := make([]AIAgentResponse, 0, len(agents)) for _, a := range agents { - agentList = append(agentList, AIAgentResponse(a)) + agentList = append(agentList, AIAgentResponse{ + Name: a.Name, + Mention: a.Mention, + IsDefault: a.IsDefault, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/api.go` around lines 2214 - 2217, The loop that builds agentList uses a direct conversion AIAgentResponse(a), which is brittle; replace it with explicit field-wise construction of AIAgentResponse inside the loop (iterate over agents and append AIAgentResponse{<map each field of AIAgentResponse to the corresponding field on a>}) so future changes to struct ordering or types won't silently break; ensure you map every exported field from the source agent to the corresponding AIAgentResponse field in the agentList construction.
2032-2055: Consider pagination efficiency for high-reaction comments.The current implementation fetches all reactions across all pages to find the current user's reaction. For comments with many reactions, this could result in multiple API calls. Consider breaking out of the pagination loop early once the user's reaction is found.
♻️ Suggested optimization
// List existing reactions to check if current user already reacted var allReactions []*github.Reaction + var existingReactionID int64 opt := &github.ListOptions{PerPage: 100} + found := false for { var reactions []*github.Reaction var resp *github.Response var err error if cErr := p.useGitHubClient(c.GHInfo, func(info *GitHubUserInfo, token *oauth2.Token) error { reactions, resp, err = githubClient.Reactions.ListPullRequestCommentReactions(c.Ctx, req.Owner, req.Repo, req.CommentID, opt) if err != nil { return err } return nil }); cErr != nil { c.Log.WithError(cErr).Errorf("Failed to list reactions") p.writeAPIError(w, &APIErrorResponse{Message: "failed to list reactions", StatusCode: http.StatusInternalServerError}) return } - allReactions = append(allReactions, reactions...) + for _, reaction := range reactions { + if reaction.GetUser().GetLogin() == currentUserLogin && reaction.GetContent() == req.Reaction { + existingReactionID = reaction.GetID() + found = true + break + } + } + if found { + break + } if resp.NextPage == 0 { break } opt.Page = resp.NextPage } - - // Check if current user already reacted with this content - var existingReactionID int64 - for _, reaction := range allReactions { - if reaction.GetUser().GetLogin() == currentUserLogin && reaction.GetContent() == req.Reaction { - existingReactionID = reaction.GetID() - break - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/api.go` around lines 2032 - 2055, The pagination loop collects allReactions by calling githubClient.Reactions.ListPullRequestCommentReactions repeatedly; update this loop to stop paging early when the current user's reaction is found: after each successful ListPullRequestCommentReactions call (inside the loop after reactions are returned), scan the reactions slice for a reaction whose User.Login or User.ID matches the current user (use c.GHInfo.Login or ID), append the reactions to allReactions as before, and if a match is found set a flag and break out of the pagination loop (avoid further pages and API calls). Keep existing error handling around p.useGitHubClient and github call intact, but add the user-match check and early break to avoid unnecessary requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin/graphql/review_threads_query.go`:
- Around line 52-85: The package-level mutable GraphQL payload structs
reviewThreadsQuery, resolveThreadMutation, and unresolveThreadMutation are
shared across requests and cause data races when passed to
executeQuery/executeMutation; fix by making those structs local per-request:
remove or stop using the package-level vars and instead allocate new instances
of the same struct types inside the functions that call
executeQuery/executeMutation (or deep-copy before/after use), updating callers
to pass the local instance and read results from it so concurrent handlers no
longer share mutable state.
In `@webapp/src/components/sidebar_right/github_items.tsx`:
- Around line 285-287: The PR card container currently intercepts clicks from
nested anchor tags and lacks keyboard accessibility; update the container (where
style.container, isPR, props.onSelectPR and handlePRClick are used) to: 1) add
keyboard semantics (role="button" and tabIndex={0}) and an onKeyDown handler
that triggers the same action as handlePRClick for Enter and Space; and 2)
prevent hijacking nested anchor links by guarding the click/keyboard handler to
return early when the event originated from or is inside an anchor (check
event.target or walk up via event.target.closest('a') or respect
event.defaultPrevented) so nested <a> elements (e.g., userName and title/number
links) keep their default navigation. Ensure props.onSelectPR is still called
only when appropriate.
In `@webapp/src/components/sidebar_right/pr_review_detail/ai_assign_bar.tsx`:
- Line 4: The selectedAgent state initialized with useState(defaultMention) can
stay empty if agents load after initial render; update selectedAgent whenever
agents prop changes by adding a useEffect that computes the appropriate default
(same logic as defaultMention) and calls setSelectedAgent, and ensure any
handlers like onAssign or assign callback use the latest selectedAgent (refer to
selectedAgent, setSelectedAgent, defaultMention, and agents in
ai_assign_bar.tsx) so assignments no longer silently do nothing when agents
arrive late.
In `@webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsx`:
- Around line 49-63: The two JSX <button> elements that toggle expansion (the
ones with onClick={() => setExpanded(true)} and onClick={() =>
setExpanded(false)}) must have an explicit type='button' to avoid submitting a
surrounding form; update both buttons in diff_hunk_display.tsx to include
type='button' while keeping their existing onClick and styles
(styles.expandButton) intact.
- Around line 38-47: The current JSX inside the <pre> in diff_hunk_display.tsx
uses <div> elements for each line (visibleLines.map), which is invalid as <pre>
must contain phrasing content; replace those inner <div> tags with <span>
elements and preserve block layout by merging display:'block' into the existing
style (e.g., style={{...styles.line, ...getLineStyle(line), display: 'block'}})
while keeping the key, visibleLines iteration, getLineStyle, and styles.* usages
unchanged.
In `@webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsx`:
- Around line 66-77: The handleAIAssign callback clears the selection
immediately because postAIAssignment is not awaited; wrap the call to
actions.postAIAssignment in an async function (or make handleAIAssign async),
await it inside a try/catch, and only call setSelectedCommentIds(new Set())
after a successful await; in the catch block surface the error to the user
(e.g., via an existing actions.notifyError or setError state) so failures do not
silently drop the selection. Use the function names handleAIAssign,
postAIAssignment, setSelectedCommentIds, and actions to locate and update the
code.
- Around line 157-163: The loadingSpinner style uses animation 'spin' but the
`@keyframes` spin is missing; add the keyframes animation named "spin" (to rotate
360deg) and ensure it's loaded globally so the component's animation runs.
Define the `@keyframes` spin in a CSS/SCSS file or in your global
stylesheet/plugin initialization and import it into the app, or inject it into
the global styles used by the PRReviewDetail component so the loadingSpinner
animation is recognized at runtime.
In `@webapp/src/components/sidebar_right/pr_review_detail/reaction_bar.tsx`:
- Around line 52-72: The handler handleToggle currently reads
localReactions[content] from a stale closure; change it to perform the
optimistic update using the functional updater of setLocalReactions (compute
current = prev[content] inside the updater) and remove direct reads of
localReactions from the closure and from the dependency array; to support proper
revert on error, capture the previous snapshot into a ref (e.g.,
prevReactionsRef) immediately before the optimistic update and, on catch,
restore that snapshot via setLocalReactions(prev => prevReactionsRef.current) —
update references to handleToggle, setLocalReactions, localReactions, and
onToggleReaction accordingly.
In `@webapp/src/reducers/index.ts`:
- Around line 264-270: prReviewThreadsLoading currently only responds to
ActionTypes.PR_REVIEW_THREADS_LOADING and can stay true after the selected PR is
cleared; update the reducer function prReviewThreadsLoading to also handle the
action type that clears the selected PR (e.g., ActionTypes.CLEAR_SELECTED_PR)
and return false in that case so the loading flag resets when the PR is cleared.
---
Nitpick comments:
In `@server/plugin/api.go`:
- Around line 2214-2217: The loop that builds agentList uses a direct conversion
AIAgentResponse(a), which is brittle; replace it with explicit field-wise
construction of AIAgentResponse inside the loop (iterate over agents and append
AIAgentResponse{<map each field of AIAgentResponse to the corresponding field on
a>}) so future changes to struct ordering or types won't silently break; ensure
you map every exported field from the source agent to the corresponding
AIAgentResponse field in the agentList construction.
- Around line 2032-2055: The pagination loop collects allReactions by calling
githubClient.Reactions.ListPullRequestCommentReactions repeatedly; update this
loop to stop paging early when the current user's reaction is found: after each
successful ListPullRequestCommentReactions call (inside the loop after reactions
are returned), scan the reactions slice for a reaction whose User.Login or
User.ID matches the current user (use c.GHInfo.Login or ID), append the
reactions to allReactions as before, and if a match is found set a flag and
break out of the pagination loop (avoid further pages and API calls). Keep
existing error handling around p.useGitHubClient and github call intact, but add
the user-match check and early break to avoid unnecessary requests.
In `@server/plugin/configuration.go`:
- Around line 311-315: The agents slice is being appended with AIAgent entries
without deduplicating by the Mention field, causing duplicate agents and React
key collisions; update the code that builds agents (where agents =
append(agents, AIAgent{ Name: name, Mention: mention, IsDefault: false })) to
skip appending if that mention already exists — e.g., maintain a local
seenMentions map[string]bool, check seenMentions[mention] before append, and
mark seenMentions[mention]=true when you append so each AIAgent (by Mention) is
only added once.
In `@webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsx`:
- Around line 25-32: The styling currently returns hardcoded color objects for
diff lines (the branches checking line.startsWith('+'), line.startsWith('-') and
the default) — update these to use theme tokens from your styling system (e.g.,
Material-UI/theme, styled-components theme, or CSS variables) instead of literal
'rgba(...)' or '#555'; locate the function in diff_hunk_display.tsx that
produces the inline style for each line (the returns shown) and replace them
with values derived from the active theme (e.g., theme.palette.success.light,
theme.palette.error.light, theme.palette.action.hover or a dedicated token for
diff additions/deletions and text color) so colours adapt to light/dark themes
and maintain contrast; ensure you import/use the theme context (useTheme or
ThemeProvider tokens) and fall back to accessible defaults if the theme token is
missing.
In
`@webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail_header.tsx`:
- Around line 29-53: Replace hardcoded English UI strings in
pr_review_detail_header.tsx with i18n keys and calls so the header is
localizable: import and use the project's translation helper (e.g., t from
react-i18next or your i18n.translate) and change literals like '\u2190 Back',
the label concatenation "{summary.approved + ' approved'}",
"{summary.changes_requested + ' changes requested'}", and
"{summary.unresolved_threads + ' unresolved threads'}" to
t('prReview.header.back'), t('prReview.header.approved', {count:
summary.approved}), t('prReview.header.changesRequested', {count:
summary.changes_requested}), and t('prReview.header.unresolvedThreads', {count:
summary.unresolved_threads}) (or your app's interpolation syntax); then add
those keys to the i18n resource files under the appropriate namespace so
translations are available.
In `@webapp/src/components/sidebar_right/pr_review_detail/reply_box.tsx`:
- Around line 29-43: The submit handler clears the textarea and collapses the
box even when onSubmit fails; update handleSubmit (in reply_box.tsx) to wrap the
await onSubmit(body) in a try/catch so that on rejection you do NOT call
setText('') or setExpanded(false), instead set an error state (e.g.,
setSubmitError / setError) and keep the text/expansion so the user can retry;
keep the finally block to setSubmitting(false) and ensure the new error state is
included/cleared appropriately when the user edits or retries.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
plugin.jsonserver/plugin/api.goserver/plugin/configuration.goserver/plugin/graphql/client.goserver/plugin/graphql/review_threads.goserver/plugin/graphql/review_threads_query.gowebapp/src/action_types/index.tswebapp/src/actions/index.tswebapp/src/client/client.tswebapp/src/components/sidebar_right/github_items.tsxwebapp/src/components/sidebar_right/index.jsxwebapp/src/components/sidebar_right/pr_review_detail/ai_assign_bar.tsxwebapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsxwebapp/src/components/sidebar_right/pr_review_detail/file_group.tsxwebapp/src/components/sidebar_right/pr_review_detail/index.tsxwebapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsxwebapp/src/components/sidebar_right/pr_review_detail/pr_review_detail_header.tsxwebapp/src/components/sidebar_right/pr_review_detail/reaction_bar.tsxwebapp/src/components/sidebar_right/pr_review_detail/reply_box.tsxwebapp/src/components/sidebar_right/pr_review_detail/review_comment.tsxwebapp/src/components/sidebar_right/pr_review_detail/review_thread.tsxwebapp/src/components/sidebar_right/sidebar_right.jsxwebapp/src/constants/index.jswebapp/src/reducers/index.tswebapp/src/selectors.tswebapp/src/types/github_types.ts
| var reviewThreadsQuery struct { | ||
| Repository struct { | ||
| PullRequest struct { | ||
| Reviews struct { | ||
| Nodes []reviewSummaryNode | ||
| } `graphql:"reviews(first:100)"` | ||
| ReviewThreads struct { | ||
| Nodes []reviewThreadNode | ||
| PageInfo struct { | ||
| EndCursor githubv4.String | ||
| HasNextPage bool | ||
| } | ||
| } `graphql:"reviewThreads(first:50, after:$threadsCursor)"` | ||
| } `graphql:"pullRequest(number:$prNumber)"` | ||
| } `graphql:"repository(owner:$owner, name:$name)"` | ||
| } | ||
|
|
||
| var resolveThreadMutation struct { | ||
| ResolveReviewThread struct { | ||
| Thread struct { | ||
| ID githubv4.String | ||
| IsResolved githubv4.Boolean | ||
| } | ||
| } `graphql:"resolveReviewThread(input:$input)"` | ||
| } | ||
|
|
||
| var unresolveThreadMutation struct { | ||
| UnresolveReviewThread struct { | ||
| Thread struct { | ||
| ID githubv4.String | ||
| IsResolved githubv4.Boolean | ||
| } | ||
| } `graphql:"unresolveReviewThread(input:$input)"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how the global query/mutation structs are used
rg -n -C3 '\b(reviewThreadsQuery|resolveThreadMutation|unresolveThreadMutation)\b' server/plugin/graphql/Repository: mattermost/mattermost-plugin-github
Length of output: 3891
🏁 Script executed:
#!/bin/bash
# Check for address-of operations on these variables
rg -n -C3 '&reviewThreadsQuery|&resolveThreadMutation|&unresolveThreadMutation' server/plugin/graphql/Repository: mattermost/mattermost-plugin-github
Length of output: 1587
🏁 Script executed:
#!/bin/bash
# Look for Query/Mutate execution patterns
rg -n -C5 'client\.Query|client\.Mutate|\bQuery\s*\(|\bMutate\s*\(' server/plugin/graphql/review_threads_query.goRepository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check if this file is called in concurrent contexts
rg -n -C5 'goroutine|go ' server/plugin/graphql/review_threads_query.goRepository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Find the executeQuery and executeMutation implementations
rg -n -B2 -A10 'func.*executeQuery|func.*executeMutation' server/plugin/graphql/Repository: mattermost/mattermost-plugin-github
Length of output: 1374
🏁 Script executed:
#!/bin/bash
# Check the full review_threads.go to understand the calling context
head -100 server/plugin/graphql/review_threads.goRepository: mattermost/mattermost-plugin-github
Length of output: 3503
🏁 Script executed:
#!/bin/bash
# Look for how the review threads functions are called (potential concurrent callers)
rg -n 'FetchReviewThreads|ResolveReviewThread|UnresolveReviewThread' server/plugin/ --type goRepository: mattermost/mattermost-plugin-github
Length of output: 1075
🏁 Script executed:
#!/bin/bash
# Verify these are public methods that could be called from concurrent HTTP handlers
grep -n "^func.*GetReviewThreads\|^func.*ResolveReviewThread" server/plugin/graphql/review_threads.goRepository: mattermost/mattermost-plugin-github
Length of output: 311
🏁 Script executed:
#!/bin/bash
# Check the api.go call site context to confirm HTTP handler concurrency
sed -n '2135,2150p' server/plugin/api.goRepository: mattermost/mattermost-plugin-github
Length of output: 780
Avoid shared mutable GraphQL payload structs across concurrent requests.
Lines 52, 69, and 78 introduce package-level mutable containers (reviewThreadsQuery, resolveThreadMutation, unresolveThreadMutation). These are passed by reference to executeQuery and executeMutation, which populate them via the underlying GraphQL client. When these public methods are called concurrently from HTTP handlers, the shared globals can race—requests overwrite each other's data in the same struct fields. Create per-request local instances instead, or deep-copy the results before reuse.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/plugin/graphql/review_threads_query.go` around lines 52 - 85, The
package-level mutable GraphQL payload structs reviewThreadsQuery,
resolveThreadMutation, and unresolveThreadMutation are shared across requests
and cause data races when passed to executeQuery/executeMutation; fix by making
those structs local per-request: remove or stop using the package-level vars and
instead allocate new instances of the same struct types inside the functions
that call executeQuery/executeMutation (or deep-copy before/after use), updating
callers to pass the local instance and read results from it so concurrent
handlers no longer share mutable state.
| style={{...style.container, ...(isPR && props.onSelectPR ? {cursor: 'pointer'} : {})}} | ||
| {...(isPR && props.onSelectPR ? {onClick: handlePRClick} : {})} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the file
find . -type f -name "github_items.tsx" | head -20Repository: mattermost/mattermost-plugin-github
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Read the file to understand the structure around lines 285-287
cat -n webapp/src/components/sidebar_right/github_items.tsx | sed -n '270,310p'Repository: mattermost/mattermost-plugin-github
Length of output: 1845
🏁 Script executed:
#!/bin/bash
# Get the full handlePRClick function implementation
cat -n webapp/src/components/sidebar_right/github_items.tsx | sed -n '230,280p'Repository: mattermost/mattermost-plugin-github
Length of output: 2235
🏁 Script executed:
#!/bin/bash
# Check for nested elements within the div (look for anchor/button children)
cat -n webapp/src/components/sidebar_right/github_items.tsx | sed -n '285,350p'Repository: mattermost/mattermost-plugin-github
Length of output: 3000
🏁 Script executed:
#!/bin/bash
# Search for handlePRClick function definition
rg "const handlePRClick|function handlePRClick" -A 10 webapp/src/components/sidebar_right/github_items.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 595
🏁 Script executed:
#!/bin/bash
# Check the entire component structure to see dependencies and props
cat -n webapp/src/components/sidebar_right/github_items.tsx | sed -n '1,50p'Repository: mattermost/mattermost-plugin-github
Length of output: 2646
🏁 Script executed:
#!/bin/bash
# Look for all interactive elements in the rendered content
rg "<a|<button|<input" webapp/src/components/sidebar_right/github_items.tsx -nRepository: mattermost/mattermost-plugin-github
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Get a broader view of the component structure to see all nested content
cat -n webapp/src/components/sidebar_right/github_items.tsx | sed -n '200,330p'Repository: mattermost/mattermost-plugin-github
Length of output: 5953
🏁 Script executed:
#!/bin/bash
# Check the milestone variable and reviews variable definitions
cat -n webapp/src/components/sidebar_right/github_items.tsx | sed -n '50,200p'Repository: mattermost/mattermost-plugin-github
Length of output: 6323
🏁 Script executed:
#!/bin/bash
# Check if there are any other event handlers that might interfere
rg "onClick|onKeyDown" webapp/src/components/sidebar_right/github_items.tsx -nRepository: mattermost/mattermost-plugin-github
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Verify the component doesn't have any stopPropagation calls on nested elements
rg "stopPropagation|preventDefault" webapp/src/components/sidebar_right/github_items.tsx -nRepository: mattermost/mattermost-plugin-github
Length of output: 61
Add keyboard support and prevent nested link click hijacking on PR cards.
The container <div> at line 283 intercepts all clicks from nested anchors (userName link at line 309, and title/number links at lines 113 and 125), preventing their default navigation. Additionally, the div is mouse-only and lacks keyboard semantics, making it inaccessible to keyboard-only users.
Proposed fix
- const handlePRClick = () => {
+ const handlePRClick = (e?: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) => {
+ const target = e?.target as HTMLElement | null;
+ if (target?.closest('a, button')) {
+ return;
+ }
if (isPR && props.onSelectPR && repoName) {
const parts = repoName.split('/');
if (parts.length === 2) {
@@
<div
key={item.id}
style={{...style.container, ...(isPR && props.onSelectPR ? {cursor: 'pointer'} : {})}}
- {...(isPR && props.onSelectPR ? {onClick: handlePRClick} : {})}
+ {...(isPR && props.onSelectPR ? {
+ role: 'button',
+ tabIndex: 0,
+ onClick: handlePRClick,
+ onKeyDown: (e: React.KeyboardEvent<HTMLElement>) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ handlePRClick(e);
+ }
+ },
+ } : {})}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/sidebar_right/github_items.tsx` around lines 285 - 287,
The PR card container currently intercepts clicks from nested anchor tags and
lacks keyboard accessibility; update the container (where style.container, isPR,
props.onSelectPR and handlePRClick are used) to: 1) add keyboard semantics
(role="button" and tabIndex={0}) and an onKeyDown handler that triggers the same
action as handlePRClick for Enter and Space; and 2) prevent hijacking nested
anchor links by guarding the click/keyboard handler to return early when the
event originated from or is inside an anchor (check event.target or walk up via
event.target.closest('a') or respect event.defaultPrevented) so nested <a>
elements (e.g., userName and title/number links) keep their default navigation.
Ensure props.onSelectPR is still called only when appropriate.
| // Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. | ||
| // See LICENSE.txt for license information. | ||
|
|
||
| import React, {useState, useCallback} from 'react'; |
There was a problem hiding this comment.
Sync selectedAgent when agents updates after initial render.
useState(defaultMention) only runs once. If agents load later, the selected value can remain empty and assignment silently does nothing.
🐛 Proposed fix
-import React, {useState, useCallback} from 'react';
+import React, {useState, useCallback, useEffect} from 'react';
@@
- const defaultMention = agents.length > 0 ? (agents.find((a) => a.is_default)?.mention || agents[0].mention) : '';
- const [selectedAgent, setSelectedAgent] = useState(defaultMention);
+ const [selectedAgent, setSelectedAgent] = useState('');
+
+ useEffect(() => {
+ if (agents.length === 0) {
+ setSelectedAgent('');
+ return;
+ }
+
+ const nextDefault = agents.find((a) => a.is_default)?.mention || agents[0].mention;
+ if (!selectedAgent || !agents.some((a) => a.mention === selectedAgent)) {
+ setSelectedAgent(nextDefault);
+ }
+ }, [agents, selectedAgent]);Also applies to: 19-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/sidebar_right/pr_review_detail/ai_assign_bar.tsx` at
line 4, The selectedAgent state initialized with useState(defaultMention) can
stay empty if agents load after initial render; update selectedAgent whenever
agents prop changes by adding a useEffect that computes the appropriate default
(same logic as defaultMention) and calls setSelectedAgent, and ensure any
handlers like onAssign or assign callback use the latest selectedAgent (refer to
selectedAgent, setSelectedAgent, defaultMention, and agents in
ai_assign_bar.tsx) so assignments no longer silently do nothing when agents
arrive late.
| <pre style={styles.pre}> | ||
| {visibleLines.map((line, idx) => ( | ||
| <div | ||
| key={idx} | ||
| style={{...styles.line, ...getLineStyle(line)}} | ||
| > | ||
| {line} | ||
| </div> | ||
| ))} | ||
| </pre> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "diff_hunk_display.tsx" -t fRepository: mattermost/mattermost-plugin-github
Length of output: 153
🏁 Script executed:
cat -n webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 3584
🌐 Web query:
HTML5 specification div inside pre tag validity
💡 Result:
In the HTML5/WHATWG spec, a <pre> element’s content model is “phrasing content”. (html.spec.whatwg.org)
A <div> is flow content (not phrasing content), so a <div> is not a valid child of <pre> (the markup is non‑conforming). (developer.mozilla.org)
If you need indentation/grouping inside a <pre>, use phrasing elements (e.g., <span>, <code>) and CSS, or apply white-space: pre; to a container instead of using <pre>. (developer.mozilla.org)
Citations:
- 1: https://html.spec.whatwg.org/dev/grouping-content.html?utm_source=openai
- 2: https://developer.mozilla.org/docs/Web/HTML/Reference/Elements/div?utm_source=openai
- 3: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/pre?utm_source=openai
Replace <div> with <span> inside <pre> element.
The <pre> element's content model is restricted to phrasing content (per HTML5 spec). The <div> elements at lines 40-45 are flow content and therefore invalid children of <pre>, which can cause rendering inconsistencies. Use <span style={{display: 'block'}}> instead to maintain valid markup while preserving block-level visual presentation.
Proposed fix
- <pre style={styles.pre}>
+ <pre style={styles.pre}>
{visibleLines.map((line, idx) => (
- <div
+ <span
key={idx}
- style={{...styles.line, ...getLineStyle(line)}}
+ style={{...styles.line, ...getLineStyle(line), display: 'block'}}
>
{line}
- </div>
+ </span>
))}
</pre>📝 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.
| <pre style={styles.pre}> | |
| {visibleLines.map((line, idx) => ( | |
| <div | |
| key={idx} | |
| style={{...styles.line, ...getLineStyle(line)}} | |
| > | |
| {line} | |
| </div> | |
| ))} | |
| </pre> | |
| <pre style={styles.pre}> | |
| {visibleLines.map((line, idx) => ( | |
| <span | |
| key={idx} | |
| style={{...styles.line, ...getLineStyle(line), display: 'block'}} | |
| > | |
| {line} | |
| </span> | |
| ))} | |
| </pre> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsx`
around lines 38 - 47, The current JSX inside the <pre> in diff_hunk_display.tsx
uses <div> elements for each line (visibleLines.map), which is invalid as <pre>
must contain phrasing content; replace those inner <div> tags with <span>
elements and preserve block layout by merging display:'block' into the existing
style (e.g., style={{...styles.line, ...getLineStyle(line), display: 'block'}})
while keeping the key, visibleLines iteration, getLineStyle, and styles.* usages
unchanged.
| <button | ||
| style={styles.expandButton} | ||
| onClick={() => setExpanded(true)} | ||
| > | ||
| {'Show more (' + (lines.length - MAX_VISIBLE_LINES) + ' more lines)'} | ||
| </button> | ||
| )} | ||
| {isLong && expanded && ( | ||
| <button | ||
| style={styles.expandButton} | ||
| onClick={() => setExpanded(false)} | ||
| > | ||
| {'Show less'} | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
Set button type explicitly.
Line 49 and Line 57 should set type='button' to prevent unintended submit behaviour when rendered inside forms.
Proposed fix
<button
+ type='button'
style={styles.expandButton}
onClick={() => setExpanded(true)}
>
@@
<button
+ type='button'
style={styles.expandButton}
onClick={() => setExpanded(false)}
>📝 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.
| <button | |
| style={styles.expandButton} | |
| onClick={() => setExpanded(true)} | |
| > | |
| {'Show more (' + (lines.length - MAX_VISIBLE_LINES) + ' more lines)'} | |
| </button> | |
| )} | |
| {isLong && expanded && ( | |
| <button | |
| style={styles.expandButton} | |
| onClick={() => setExpanded(false)} | |
| > | |
| {'Show less'} | |
| </button> | |
| )} | |
| <button | |
| type='button' | |
| style={styles.expandButton} | |
| onClick={() => setExpanded(true)} | |
| > | |
| {'Show more (' + (lines.length - MAX_VISIBLE_LINES) + ' more lines)'} | |
| </button> | |
| )} | |
| {isLong && expanded && ( | |
| <button | |
| type='button' | |
| style={styles.expandButton} | |
| onClick={() => setExpanded(false)} | |
| > | |
| {'Show less'} | |
| </button> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/sidebar_right/pr_review_detail/diff_hunk_display.tsx`
around lines 49 - 63, The two JSX <button> elements that toggle expansion (the
ones with onClick={() => setExpanded(true)} and onClick={() =>
setExpanded(false)}) must have an explicit type='button' to avoid submitting a
surrounding form; update both buttons in diff_hunk_display.tsx to include
type='button' while keeping their existing onClick and styles
(styles.expandButton) intact.
| const handleAIAssign = useCallback((agentMention: string) => { | ||
| if (selectedCommentIds.size === 0 || !selectedPR) { | ||
| return; | ||
| } | ||
|
|
||
| // Build a comment body mentioning the agent and referencing selected comment IDs | ||
| const commentRefs = Array.from(selectedCommentIds).join(', '); | ||
| const body = `${agentMention} Please review the following comment threads: ${commentRefs}`; | ||
|
|
||
| actions.postAIAssignment(selectedPR.owner, selectedPR.repo, selectedPR.number, body); | ||
| setSelectedCommentIds(new Set()); | ||
| }, [selectedCommentIds, actions, selectedPR]); |
There was a problem hiding this comment.
Handle errors from postAIAssignment before clearing selection.
The postAIAssignment call on line 75 is not awaited, and the selection is cleared immediately on line 76 regardless of success or failure. If the assignment fails, the user loses their selection with no feedback.
🛡️ Proposed fix to await and handle errors
const handleAIAssign = useCallback((agentMention: string) => {
+ const handleAIAssign = useCallback(async (agentMention: string) => {
if (selectedCommentIds.size === 0 || !selectedPR) {
return;
}
// Build a comment body mentioning the agent and referencing selected comment IDs
const commentRefs = Array.from(selectedCommentIds).join(', ');
const body = `${agentMention} Please review the following comment threads: ${commentRefs}`;
- actions.postAIAssignment(selectedPR.owner, selectedPR.repo, selectedPR.number, body);
- setSelectedCommentIds(new Set());
+ try {
+ await actions.postAIAssignment(selectedPR.owner, selectedPR.repo, selectedPR.number, body);
+ setSelectedCommentIds(new Set());
+ } catch (error) {
+ console.error('Failed to post AI assignment:', error);
+ // Keep selection so user can retry
+ }
}, [selectedCommentIds, actions, selectedPR]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsx`
around lines 66 - 77, The handleAIAssign callback clears the selection
immediately because postAIAssignment is not awaited; wrap the call to
actions.postAIAssignment in an async function (or make handleAIAssign async),
await it inside a try/catch, and only call setSelectedCommentIds(new Set())
after a successful await; in the catch block surface the error to the user
(e.g., via an existing actions.notifyError or setError state) so failures do not
silently drop the selection. Use the function names handleAIAssign,
postAIAssignment, setSelectedCommentIds, and actions to locate and update the
code.
| loadingSpinner: { | ||
| width: '24px', | ||
| height: '24px', | ||
| border: '3px solid rgba(0, 0, 0, 0.1)', | ||
| borderRadius: '50%', | ||
| animation: 'spin 1s linear infinite', | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '@keyframes\s+spin' webapp/Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Also search more broadly for spin animation
rg -n 'spin' webapp/src/components/sidebar_right/pr_review_detail/ -A 2 -B 2Repository: mattermost/mattermost-plugin-github
Length of output: 616
🏁 Script executed:
# Check the full file to see imports and context
wc -l webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 156
🏁 Script executed:
# Check for global CSS files that might contain the animation
fd -e css -e scss -e less webapp/src/ | head -20Repository: mattermost/mattermost-plugin-github
Length of output: 428
🏁 Script executed:
# Find CSS files in webapp
fd -e css -e scss -e less . webapp/src/Repository: mattermost/mattermost-plugin-github
Length of output: 125
🏁 Script executed:
# Read the tsx file to see imports and check if styles are imported
head -50 webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 2042
🏁 Script executed:
# Check for any global CSS or style imports
rg -n 'import.*css|import.*style|@import' webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Search for spin animation more broadly (not just `@keyframes`)
rg -n 'spin' webapp/src/ | grep -i 'keyframe\|animation\|css' | head -20Repository: mattermost/mattermost-plugin-github
Length of output: 202
🏁 Script executed:
# Read more of the file to see the complete styles object
sed -n '150,172p' webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 636
🏁 Script executed:
# Check for global CSS or style files at the root
find webapp/ -maxdepth 2 -name '*.css' -o -name '*.scss' -o -name 'global*' -o -name 'index.css'Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Check if there's a CSS-in-JS solution or if `@keyframes` is defined dynamically
rg -n '@keyframes' webapp/ --type css --type scssRepository: mattermost/mattermost-plugin-github
Length of output: 107
🏁 Script executed:
# Check the entire styles object to understand context
sed -n '140,172p' webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 876
🏁 Script executed:
# Check if the loadingSpinner style is actually used anywhere
rg -n 'loadingSpinner' webapp/src/components/sidebar_right/pr_review_detail/Repository: mattermost/mattermost-plugin-github
Length of output: 354
🏁 Script executed:
# Search for any keyframes definition anywhere in the project
rg -n '@keyframes' webapp/ -g '!node_modules'Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Check main app files for global styles
find webapp/src -maxdepth 1 -type f \( -name '*.css' -o -name '*.tsx' -o -name '*.ts' \)Repository: mattermost/mattermost-plugin-github
Length of output: 130
🏁 Script executed:
# Check if there's any HTML file or global style injection
find webapp/src -maxdepth 2 -name 'index.html' -o -name 'app.tsx' -o -name 'app.ts'Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Check the public folder for global styles
find webapp/public -type f -name '*.css' 2>/dev/nullRepository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Look at the manifest or entry point files
cat webapp/src/manifest.test.ts | head -30Repository: mattermost/mattermost-plugin-github
Length of output: 410
🏁 Script executed:
# Search for any global style definitions or CSS injection
rg -n 'global|GlobalStyle|injectGlobal' webapp/src/Repository: mattermost/mattermost-plugin-github
Length of output: 161
🏁 Script executed:
# Check the index.js file to see if any CSS is being injected
cat webapp/src/index.js | head -150Repository: mattermost/mattermost-plugin-github
Length of output: 5428
🏁 Script executed:
# Check for any style tag or CSS injection in the plugin class
rg -n 'style\|css\|keyframes' webapp/src/index.js -A 2 -B 2Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Look at package.json to see what CSS-in-JS or styling libraries are used
cat webapp/package.json 2>/dev/null || cat package.json 2>/dev/null | grep -A 20 '"dependencies"'Repository: mattermost/mattermost-plugin-github
Length of output: 4341
🏁 Script executed:
# Check if there's a global CSS file being injected or referenced anywhere
rg -n 'link rel.*css|<style|importCSS|injectCSS' webapp/src/ -g '!node_modules'Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Look more carefully at how the component applies styles
sed -n '100,120p' webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 1139
🏁 Script executed:
# Check if there's any CSS file in the sidebar_right component folder
find webapp/src/components/sidebar_right -name '*.css'Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Check the entire component folder structure
ls -la webapp/src/components/sidebar_right/Repository: mattermost/mattermost-plugin-github
Length of output: 460
🏁 Script executed:
# Check if there's any global setup file that might define animations
find webapp/src -name 'setup*' -o -name 'global*' -o -name 'index.*' | grep -v node_modulesRepository: mattermost/mattermost-plugin-github
Length of output: 1077
🏁 Script executed:
# Check the LinkTooltip component CSS since we found tooltip.css
cat webapp/src/components/link_tooltip/tooltip.cssRepository: mattermost/mattermost-plugin-github
Length of output: 2806
🏁 Script executed:
# Check the entire sidebar_right folder structure
ls -la webapp/src/components/sidebar_right/pr_review_detail/Repository: mattermost/mattermost-plugin-github
Length of output: 862
🏁 Script executed:
# Look for any emotion/CSS-in-JS global style definitions
rg -n 'Global|injectGlobal|css`|styled' webapp/src/ | head -20Repository: mattermost/mattermost-plugin-github
Length of output: 1922
🏁 Script executed:
# Check if the Mattermost main app provides global animations
# Look at what mattermost-redux or mattermost-webapp might provide
rg -n 'animation.*spin|keyframes.*spin' webapp/node_modules/ 2>/dev/null | head -5Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Check the index.tsx export to understand how the component is used
cat webapp/src/components/sidebar_right/pr_review_detail/index.tsxRepository: mattermost/mattermost-plugin-github
Length of output: 1463
🏁 Script executed:
# Check if there's any CSS animation defined outside of webapp folder
find . -maxdepth 2 -name '*.css' -o -name '*.scss' | grep -v node_modules | xargs grep -l 'keyframes\|spin' 2>/dev/nullRepository: mattermost/mattermost-plugin-github
Length of output: 63
🏁 Script executed:
# Check if mattermost-redux or mattermost-webapp provides spin animation
find webapp/node_modules/mattermost-redux -name '*.css' 2>/dev/null | xargs grep -l 'spin' 2>/dev/null | head -5Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Check if there are any global styles being injected by Mattermost
rg -n 'spin' webapp/node_modules/@mattermost 2>/dev/null | head -10Repository: mattermost/mattermost-plugin-github
Length of output: 61
🏁 Script executed:
# Look for any animation definitions in the entire project (not just webapp)
find . -name '*.css' -o -name '*.scss' | grep -v node_modules | xargs grep -i '@keyframes.*spin\|animation.*spin' 2>/dev/nullRepository: mattermost/mattermost-plugin-github
Length of output: 63
CSS animation spin is not defined.
The loadingSpinner style at line 162 references animation: 'spin 1s linear infinite', but no @keyframes spin definition exists anywhere in the codebase. The spinner element will render with the correct styling but won't animate.
Add the missing keyframes definition:
`@keyframes` spin {
to {
transform: rotate(360deg);
}
}Then either define it in a CSS file and import it, or inject it globally via the plugin initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/sidebar_right/pr_review_detail/pr_review_detail.tsx`
around lines 157 - 163, The loadingSpinner style uses animation 'spin' but the
`@keyframes` spin is missing; add the keyframes animation named "spin" (to rotate
360deg) and ensure it's loaded globally so the component's animation runs.
Define the `@keyframes` spin in a CSS/SCSS file or in your global
stylesheet/plugin initialization and import it into the app, or inject it into
the global styles used by the PRReviewDetail component so the loadingSpinner
animation is recognized at runtime.
| const handleToggle = useCallback(async (content: string) => { | ||
| const current = localReactions[content]; | ||
| const newReacted = !current.reacted; | ||
| const newCount = newReacted ? current.count + 1 : Math.max(0, current.count - 1); | ||
|
|
||
| // Optimistic update | ||
| setLocalReactions((prev) => ({ | ||
| ...prev, | ||
| [content]: {count: newCount, reacted: newReacted}, | ||
| })); | ||
|
|
||
| try { | ||
| await onToggleReaction(content); | ||
| } catch { | ||
| // Revert on error | ||
| setLocalReactions((prev) => ({ | ||
| ...prev, | ||
| [content]: {count: current.count, reacted: current.reacted}, | ||
| })); | ||
| } | ||
| }, [localReactions, onToggleReaction]); |
There was a problem hiding this comment.
Avoid stale-closure reads in optimistic reaction toggles.
Using localReactions[content] from closure can drift under quick repeated clicks. Compute from the functional updater’s prev snapshot instead.
🛠️ Suggested adjustment
- const handleToggle = useCallback(async (content: string) => {
- const current = localReactions[content];
- const newReacted = !current.reacted;
- const newCount = newReacted ? current.count + 1 : Math.max(0, current.count - 1);
+ const handleToggle = useCallback(async (content: string) => {
+ let previous = {count: 0, reacted: false};
+ let next = {count: 0, reacted: false};
- // Optimistic update
setLocalReactions((prev) => ({
...prev,
- [content]: {count: newCount, reacted: newReacted},
+ [content]: (() => {
+ const current = prev[content] || {count: 0, reacted: false};
+ previous = current;
+ const reacted = !current.reacted;
+ next = {count: reacted ? current.count + 1 : Math.max(0, current.count - 1), reacted};
+ return next;
+ })(),
}));
try {
await onToggleReaction(content);
} catch {
- // Revert on error
setLocalReactions((prev) => ({
...prev,
- [content]: {count: current.count, reacted: current.reacted},
+ [content]: previous,
}));
}
- }, [localReactions, onToggleReaction]);
+ }, [onToggleReaction]);📝 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.
| const handleToggle = useCallback(async (content: string) => { | |
| const current = localReactions[content]; | |
| const newReacted = !current.reacted; | |
| const newCount = newReacted ? current.count + 1 : Math.max(0, current.count - 1); | |
| // Optimistic update | |
| setLocalReactions((prev) => ({ | |
| ...prev, | |
| [content]: {count: newCount, reacted: newReacted}, | |
| })); | |
| try { | |
| await onToggleReaction(content); | |
| } catch { | |
| // Revert on error | |
| setLocalReactions((prev) => ({ | |
| ...prev, | |
| [content]: {count: current.count, reacted: current.reacted}, | |
| })); | |
| } | |
| }, [localReactions, onToggleReaction]); | |
| const handleToggle = useCallback(async (content: string) => { | |
| let previousState = {count: 0, reacted: false}; | |
| // Optimistic update with state capture | |
| setLocalReactions((prev) => { | |
| previousState = prev[content] || {count: 0, reacted: false}; | |
| const reacted = !previousState.reacted; | |
| return { | |
| ...prev, | |
| [content]: { | |
| count: reacted ? previousState.count + 1 : Math.max(0, previousState.count - 1), | |
| reacted | |
| }, | |
| }; | |
| }); | |
| try { | |
| await onToggleReaction(content); | |
| } catch { | |
| // Revert on error | |
| setLocalReactions((prev) => ({ | |
| ...prev, | |
| [content]: previousState, | |
| })); | |
| } | |
| }, [onToggleReaction]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/sidebar_right/pr_review_detail/reaction_bar.tsx` around
lines 52 - 72, The handler handleToggle currently reads localReactions[content]
from a stale closure; change it to perform the optimistic update using the
functional updater of setLocalReactions (compute current = prev[content] inside
the updater) and remove direct reads of localReactions from the closure and from
the dependency array; to support proper revert on error, capture the previous
snapshot into a ref (e.g., prevReactionsRef) immediately before the optimistic
update and, on catch, restore that snapshot via setLocalReactions(prev =>
prevReactionsRef.current) — update references to handleToggle,
setLocalReactions, localReactions, and onToggleReaction accordingly.
| function prReviewThreadsLoading(state = false, action: {type: string, data: boolean}) { | ||
| switch (action.type) { | ||
| case ActionTypes.PR_REVIEW_THREADS_LOADING: | ||
| return action.data; | ||
| default: | ||
| return state; | ||
| } |
There was a problem hiding this comment.
Reset review-thread loading state on PR clear.
When clearing the selected PR, prReviewThreadsLoading should also return to false to avoid stale loading indicators.
💡 Proposed fix
function prReviewThreadsLoading(state = false, action: {type: string, data: boolean}) {
switch (action.type) {
case ActionTypes.PR_REVIEW_THREADS_LOADING:
return action.data;
+ case ActionTypes.CLEAR_SELECTED_PR:
+ return false;
default:
return state;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/reducers/index.ts` around lines 264 - 270, prReviewThreadsLoading
currently only responds to ActionTypes.PR_REVIEW_THREADS_LOADING and can stay
true after the selected PR is cleared; update the reducer function
prReviewThreadsLoading to also handle the action type that clears the selected
PR (e.g., ActionTypes.CLEAR_SELECTED_PR) and return false in that case so the
loading flag resets when the PR is cleared.
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive PR review comments dashboard to the right-hand sidebar (RHS) in Mattermost, enabling users to view, reply to, react to, resolve, and assign review comments to AI agents without leaving Mattermost. The implementation includes both backend GraphQL infrastructure for fetching review threads and frontend React components for displaying and interacting with review comments.
Changes:
- Added backend GraphQL queries/mutations for review threads with pagination support, plus 6 new API endpoints for comment operations and AI agent configuration
- Implemented frontend Redux state management and React components for drill-down PR review detail view with file-grouped threads, diff snippets, inline replies, emoji reactions, and AI agent assignment
- Added AI Agents configuration feature with Claude and Cursor as default agents
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/src/types/github_types.ts | Added TypeScript interfaces for review threads, comments, reactions, AI agents, and PR selection data |
| webapp/src/selectors.ts | Added selectors for PR review state and threads grouped by file |
| webapp/src/reducers/index.ts | Added reducers for selected PR, review threads, loading state, and AI agents |
| webapp/src/constants/index.js | Added REACTIONS constant array for emoji reactions |
| webapp/src/components/sidebar_right/sidebar_right.jsx | Integrated PRReviewDetail view with conditional rendering based on selectedPR |
| webapp/src/components/sidebar_right/pr_review_detail/*.tsx | Implemented 8 new React components for review detail UI including header, file groups, threads, comments, replies, reactions, diff display, and AI assignment |
| webapp/src/components/sidebar_right/index.jsx | Connected new actions and selectors for PR review functionality |
| webapp/src/components/sidebar_right/github_items.tsx | Added PR drill-down navigation with onClick handler |
| webapp/src/client/client.ts | Added 6 new client methods for review threads API endpoints |
| webapp/src/actions/index.ts | Implemented 6 new action creators for PR review operations |
| webapp/src/action_types/index.ts | Added 6 new action type constants |
| server/plugin/graphql/review_threads_query.go | Defined GraphQL query structures for review threads, comments, and resolution mutations |
| server/plugin/graphql/review_threads.go | Implemented GetReviewThreads and ResolveReviewThread methods with pagination |
| server/plugin/graphql/client.go | Added executeMutation method for GraphQL mutations |
| server/plugin/configuration.go | Added AIAgent type and ParseAIAgents configuration method |
| server/plugin/api.go | Implemented 6 new HTTP handlers for PR review endpoints |
| plugin.json | Added AIAgents configuration setting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| thread := ReviewThread{ | ||
| ID: string(threadNode.ID), | ||
| IsResolved: bool(threadNode.IsResolved), | ||
| ResolvedByLogin: string(threadNode.ResolvedBy.Login), |
There was a problem hiding this comment.
When a review thread is not resolved, the ResolvedBy field will be null in the GraphQL response, but converting threadNode.ResolvedBy.Login to a string will result in an empty string rather than handling the null case properly. This could cause issues in the frontend which expects resolved_by to be either an object with a login field or null. Consider checking if ResolvedBy is null before accessing its Login field, or adjust the type handling to properly represent null values.
| thread := ReviewThread{ | |
| ID: string(threadNode.ID), | |
| IsResolved: bool(threadNode.IsResolved), | |
| ResolvedByLogin: string(threadNode.ResolvedBy.Login), | |
| resolvedByLogin := "" | |
| if threadNode.ResolvedBy != nil { | |
| resolvedByLogin = string(threadNode.ResolvedBy.Login) | |
| } | |
| thread := ReviewThread{ | |
| ID: string(threadNode.ID), | |
| IsResolved: bool(threadNode.IsResolved), | |
| ResolvedByLogin: resolvedByLogin, |
|
|
||
| var reviewThreadsQuery struct { | ||
| Repository struct { | ||
| PullRequest struct { |
There was a problem hiding this comment.
Reviews are limited to the first 100 entries without pagination support. For PRs with more than 100 reviews, subsequent reviews will not be counted in the approved/changes_requested summary. Consider implementing pagination for reviews or increasing the limit and documenting this constraint.
| PullRequest struct { | |
| PullRequest struct { | |
| // NOTE: GitHub GraphQL limits the maximum page size to 100 items. | |
| // This query intentionally requests only the first 100 reviews. | |
| // Pull requests with more than 100 reviews will have subsequent reviews | |
| // ignored by any summary logic that processes this slice. Supporting | |
| // more than 100 reviews requires adding pagination (e.g., an "after" | |
| // cursor and corresponding PageInfo) and updating the calling code. |
| actions.getPRReviewThreads(selectedPR.owner, selectedPR.repo, selectedPR.number); | ||
| actions.getAIAgents(); | ||
| } | ||
| }, [selectedPR?.owner, selectedPR?.repo, selectedPR?.number]); // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
The actions object is missing from the dependencies array despite being used in the useEffect. While this is suppressed with an eslint-disable comment, it can lead to stale closure issues if the actions change. Consider including actions in the dependency array, or if actions are stable (memoized via bindActionCreators), document why it's safe to omit them.
| }, [selectedPR?.owner, selectedPR?.repo, selectedPR?.number]); // eslint-disable-line react-hooks/exhaustive-deps | |
| }, [selectedPR?.owner, selectedPR?.repo, selectedPR?.number, actions]); |
| id: string; | ||
| database_id: number; | ||
| body: string; | ||
| author: {login: string; avatar_url: string}; |
There was a problem hiding this comment.
The frontend type expects an author object with both login and avatar_url fields, but the backend API only returns author_login as a string (see server/plugin/api.go line 1873). This mismatch will cause the avatar display in review comments to fail silently when the component tries to access comment.author.avatar_url. Either update the backend to fetch and return avatar URLs (e.g., by querying the GitHub API or constructing the URL from the login), or change the frontend type to match what the backend actually provides.
| author: {login: string; avatar_url: string}; | |
| author_login: string; |
| path: string; | ||
| line: number; | ||
| start_line: number; | ||
| reactions: {content: string; count: number; reacted: boolean}[]; |
There was a problem hiding this comment.
The frontend expects a reacted boolean field in reactions to indicate if the current user has reacted, but the GraphQL query only fetches ReactionGroups which provide aggregated counts without per-user information. This will cause the reaction buttons to not properly show the current user's reaction state. The GraphQL query should include ViewerHasReacted information for each reaction group, or the backend should query reactions separately to determine if the current user has reacted to each comment.
| reactions: {content: string; count: number; reacted: boolean}[]; | |
| reactions: {content: string; count: number; reacted?: boolean}[]; |
|
|
||
| const handleResolveToggle = useCallback(async () => { | ||
| const action = isResolved ? 'unresolve' : 'resolve'; | ||
| return resolveThread(thread.id, action); |
There was a problem hiding this comment.
Similar to the reply functionality, if resolving/unresolving a thread fails, there's no error feedback to the user. The button click happens but the UI state doesn't change (because the data isn't refetched), leaving the user confused. Add error handling and user feedback for failed resolve/unresolve operations.
| return resolveThread(thread.id, action); | |
| try { | |
| await resolveThread(thread.id, action); | |
| } catch (error) { | |
| // Provide basic feedback so the user isn't left confused when the UI doesn't change. | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to resolve/unresolve review thread:', error); | |
| window.alert('Failed to update the thread resolution state. Please try again.'); | |
| } |
| const defaultMention = agents.length > 0 ? (agents.find((a) => a.is_default)?.mention || agents[0].mention) : ''; | ||
| const [selectedAgent, setSelectedAgent] = useState(defaultMention); |
There was a problem hiding this comment.
The selectedAgent state is initialized with defaultMention only once when the component mounts. If the agents list changes (e.g., from empty to populated after loading), the selectedAgent won't update to reflect the new default. Consider using useEffect to update selectedAgent when agents changes, or move the default selection logic into the render to always compute the current default.
| if err != nil { | ||
| p.writeAPIError(w, &APIErrorResponse{Message: "Invalid param 'number'.", StatusCode: http.StatusBadRequest}) | ||
| return | ||
| } |
There was a problem hiding this comment.
The parameter validation allows a zero or negative PR number, which would lead to invalid GitHub API calls. Add validation to ensure the number is positive (greater than 0) before proceeding with the GraphQL query.
| } | |
| } | |
| if number <= 0 { | |
| p.writeAPIError(w, &APIErrorResponse{Message: "Invalid param 'number'. Must be greater than 0.", StatusCode: http.StatusBadRequest}) | |
| return | |
| } |
| <div | ||
| style={{ | ||
| ...styles.resolvedCollapsed, | ||
| backgroundColor: changeOpacity(theme.centerChannelColor, 0.03), | ||
| borderLeft: `3px solid ${changeOpacity(theme.centerChannelColor, 0.2)}`, | ||
| }} | ||
| onClick={() => setExpandedResolved(true)} | ||
| > | ||
| <div style={{display: 'flex', alignItems: 'center', gap: '8px'}}> | ||
| <input | ||
| type='checkbox' | ||
| checked={isSelected} | ||
| onChange={(e) => { | ||
| e.stopPropagation(); | ||
| handleCheckboxChange(); | ||
| }} | ||
| onClick={(e) => e.stopPropagation()} | ||
| style={styles.checkbox} | ||
| /> | ||
| <span style={{...styles.resolvedLabel, color: changeOpacity(theme.centerChannelColor, 0.5)}}> | ||
| {'Resolved thread'} | ||
| {thread.resolved_by ? ` by ${thread.resolved_by.login}` : ''} | ||
| {' - '} | ||
| {firstComment?.body ? firstComment.body.substring(0, 80) + (firstComment.body.length > 80 ? '...' : '') : ''} | ||
| </span> | ||
| </div> | ||
| <span style={{...styles.expandHint, color: changeOpacity(theme.centerChannelColor, 0.4)}}> | ||
| {'Click to expand'} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
The clickable collapsed thread container (line 67-96) lacks keyboard accessibility. Users navigating with keyboard cannot expand resolved threads. Add appropriate ARIA attributes (e.g., role="button", tabIndex="0") and keyboard event handlers (onKeyDown for Enter/Space keys) to make this interactive element accessible.
| const handleSubmit = useCallback(async () => { | ||
| const body = text.trim(); | ||
| if (!body || submitting) { | ||
| return; | ||
| } | ||
|
|
||
| setSubmitting(true); | ||
| try { | ||
| await onSubmit(body); | ||
| setText(''); | ||
| setExpanded(false); | ||
| } finally { | ||
| setSubmitting(false); | ||
| } | ||
| }, [text, submitting, onSubmit]); |
There was a problem hiding this comment.
If the reply submission fails (e.g., network error, API error), the error is silently swallowed and the user receives no feedback. The text area collapses and clears as if the submission succeeded. Add error handling to display an error message to the user when submission fails, and keep the text in the textarea so they can retry.
Implement a drill-down detail view in the RHS that shows a PR's review
comments grouped by file with diff snippets, enabling replying, reacting,
resolving threads, and assigning selected feedback to AI agents — all
without leaving Mattermost.
Backend:
pr-review-comment-reaction, pr-resolve-thread, pr-create-comment,
and plugin-settings/ai-agents
Frontend:
methods, and action creators for review thread management
Summary by CodeRabbit