MM-67455: DM connected users when mentioned in confluence#229
Merged
nang2049 merged 6 commits intoJun 4, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds mention-DM notifications: extract account IDs from Cloud ADF and Server XHTML, per-user opt settings in KV, SendMentionDMs service, REST client surface for mention resolution, webhook/poller integration to dispatch DMs, slash commands to control preferences, and supporting docs/config updates. ChangesMention Notification System
Sequence Diagram(s)sequenceDiagram
participant ConfluenceWebhook
participant ConfluenceClient
participant MentionExtraction
participant SendMentionDMs
participant MattermostDM
ConfluenceWebhook->>ConfluenceClient: Fetch page/comment with expanded storage
ConfluenceClient->>MentionExtraction: Extract mention account IDs
MentionExtraction-->>SendMentionDMs: []accountID
SendMentionDMs->>SendMentionDMs: De-duplicate & filter actor
SendMentionDMs->>SendMentionDMs: Check KV opt-in
SendMentionDMs->>MattermostDM: Create DM post for enabled recipients
MattermostDM-->>SendMentionDMs: success/error
sequenceDiagram
participant ForgePoller
participant ForgeQueue
participant ADFParsing
participant SendMentionDMs
participant MattermostDM
ForgePoller->>ForgeQueue: Drain queued events
ForgeQueue-->>ForgePoller: events (with enriched ADF body)
ForgePoller->>ADFParsing: Extract mention account IDs from ADF
ADFParsing-->>SendMentionDMs: []accountID
SendMentionDMs->>MattermostDM: Create DM posts for enabled recipients
MattermostDM-->>SendMentionDMs: success/error
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ns-for-connected-user
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/service/mentions_test.go (1)
68-69: ⚡ Quick winAssert exact DM count to prevent duplicate-send regressions.
AssertCalledis too weak here; use an exact call count so duplicate DM bugs fail the test.Proposed fix
- api.AssertCalled(t, "CreatePost", mock.Anything) + api.AssertNumberOfCalls(t, "CreatePost", 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/mentions_test.go` around lines 68 - 69, Replace the weak api.AssertCalled(t, "CreatePost", mock.Anything) with an exact-call assertion to prevent duplicate DM regressions: call api.AssertNumberOfCalls(t, "CreatePost", 1) (or the equivalent AssertNumberOfCalls method on the mock) so the test fails if CreatePost is invoked more than once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/command.go`:
- Around line 63-76: The handlers map on ConfluenceCommandHandler is missing the
"edit" entry advertised elsewhere; add an "edit" key to the handlers map and
point it to the existing edit handler function (e.g., editSubscription or
executeEdit — use whatever function implements the edit flow), or implement that
handler if it doesn't exist, then register it as "edit": editSubscription (or
"edit": executeEdit) so the /confluence edit command routes to the edit flow
instead of falling through to invalidCommand.
In `@server/confluence_server.go`:
- Around line 288-292: GetMentionAccountIDsWithAPIToken currently returns nil
error for non-200 responses because it only returns the err from
MakeHTTPCallWithAPIToken, which is nil for 403/404; change it to treat any
statusCode != http.StatusOK as an error by returning a constructed error (e.g.
fmt.Errorf("failed to fetch content %s: status %d", contentID, statusCode)) so
callers don’t silently proceed; update the call sites that use
GetMentionAccountIDsWithAPIToken (the API-token path around the other
mention-fetching logic) to handle and propagate this error instead of treating a
nil error as success.
In `@server/forge_poller.go`:
- Line 196: The reconnect-notice dedupe check in the KVGet → CreatePost → KVSet
sequence is not atomic and can race when dispatchMentionDMs is invoked
concurrently; serialize that critical section by adding per-actor
synchronization (e.g., a per-actor mutex or a single map of locks) and acquire
the lock around the check/send/set sequence inside the function that currently
does KVGet, CreatePost, KVSet so only one goroutine for a given actor can
perform the read→create→write path at a time; ensure the lock is released after
KVSet (or on error) and reference dispatchMentionDMs and the
KVGet/CreatePost/KVSet calls when updating the code.
In `@server/service/mentions.go`:
- Around line 36-65: The loop over p.AccountIDs can send duplicate DMs when the
same accountID appears multiple times; add a local seen set
(map[string]struct{}) at the start of the dispatch to skip duplicates: before
resolving store.GetMattermostUserIDFromConfluenceID for an accountID, check and
mark it in seen so each accountID is processed at most once per invocation; keep
the existing checks (skip when accountID == p.ActorAccountID, when mmUserIDPtr
nil/empty, when !IsMentionNotificationEnabled, and when
GetDirectChannel/CreatePost fail) and use buildMentionMessage/preserving the
same usage of config.Mattermost.GetDirectChannel and CreatePost.
---
Nitpick comments:
In `@server/service/mentions_test.go`:
- Around line 68-69: Replace the weak api.AssertCalled(t, "CreatePost",
mock.Anything) with an exact-call assertion to prevent duplicate DM regressions:
call api.AssertNumberOfCalls(t, "CreatePost", 1) (or the equivalent
AssertNumberOfCalls method on the mock) so the test fails if CreatePost is
invoked more than once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6235fd94-b6f0-4c07-8f67-c9175058113b
📒 Files selected for processing (15)
server/client.goserver/client_cloud.goserver/client_server.goserver/command.goserver/confluence_server.goserver/forge_poller.goserver/instance_cloud.goserver/serializer/confluence_forge.goserver/service/adf.goserver/service/adf_test.goserver/service/mention_settings.goserver/service/mentions.goserver/service/mentions_test.goserver/service/storage_xhtml.goserver/service/storage_xhtml_test.go
Comment on lines
63
to
76
| var ConfluenceCommandHandler = Handler{ | ||
| handlers: map[string]HandlerFunc{ | ||
| "list": listChannelSubscription, | ||
| "unsubscribe": deleteSubscription, | ||
| "install": showInstallEditionPrompt, | ||
| "install/cloud": showInstallCloudHelp, | ||
| "install/server": showInstallServerHelp, | ||
| "connect": executeConnect, | ||
| "disconnect": executeDisconnect, | ||
| "help": confluenceHelpCommand, | ||
| "list": listChannelSubscription, | ||
| "unsubscribe": deleteSubscription, | ||
| "install": showInstallEditionPrompt, | ||
| "install/cloud": showInstallCloudHelp, | ||
| "install/server": showInstallServerHelp, | ||
| "connect": executeConnect, | ||
| "disconnect": executeDisconnect, | ||
| "help": confluenceHelpCommand, | ||
| "notifications": executeNotificationsStatus, | ||
| "notifications/on": executeNotificationsOn, | ||
| "notifications/off": executeNotificationsOff, | ||
| }, |
There was a problem hiding this comment.
Register the edit subcommand in the router.
Line 40 and the autocomplete entries below now advertise /confluence edit, but this map still has no "edit" entry, so that command will fall through to invalidCommand instead of opening the edit flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/command.go` around lines 63 - 76, The handlers map on
ConfluenceCommandHandler is missing the "edit" entry advertised elsewhere; add
an "edit" key to the handlers map and point it to the existing edit handler
function (e.g., editSubscription or executeEdit — use whatever function
implements the edit flow), or implement that handler if it doesn't exist, then
register it as "edit": editSubscription (or "edit": executeEdit) so the
/confluence edit command routes to the edit flow instead of falling through to
invalidCommand.
Contributor
Author
There was a problem hiding this comment.
Confirmed valid but pre-existing on master, I can flag as a follow up rather than expanding scope here.
There was a problem hiding this comment.
@nang2049, understood — keeping this PR focused makes sense. Would you like me to open a GitHub issue to track the missing "edit" handler registration in ConfluenceCommandHandler so it doesn't get lost?
avasconcelos114
left a comment
avasconcelos114
left a comment
Member
There was a problem hiding this comment.
Awesome improvement! I bet this will come in handy to a lot of people
Just leaving a suggestion for nesting the notification slash command for future-proofing and consistency with other plugins
| if e.Content == nil { | ||
| return "" | ||
| } | ||
| return e.Content.Extensions.Location |
Member
There was a problem hiding this comment.
Is there any chance we end up trying to read a nil value since both Extensions and Location have omitempty marked on them? (not entirely familiar with the shape of Forge event bodies and what defaults it may send so not sure if this is a valid concern or not)
| "* `/confluence list` - List all subscriptions for the current channel.\n" + | ||
| "* `/confluence edit \"<name>\"` - Edit the subscription settings associated with the given subscription name.\n" | ||
| "* `/confluence edit \"<name>\"` - Edit the subscription settings associated with the given subscription name.\n" + | ||
| "* `/confluence notifications [on|off]` - Show or change whether you get a direct message when you are @-mentioned in Confluence.\n" |
Member
There was a problem hiding this comment.
Wondering if it would be good to follow GitHub's pattern and nest this under a settings command family in case we want to add other types of personalised features
Contributor
Author
|
@avasconcelos114 ready for review |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/serializer/confluence_forge.go (1)
55-61: 💤 Low valueClarify/remove unused
ForgeExtensions.Location
server/serializer/confluence_forge.godefinesForgeExtensions.Location, but there are no Go references toExtensions.Locationbeyond the inline comment stating it’s"footer"for comment-related payloads.- No nil-deref risk:
Locationis a non-pointer string withomitempty(missing fields deserialize to"").- Keep this field only if it’s required to mirror the Forge event payload; otherwise remove it or add a stronger rationale for why it must remain.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/serializer/confluence_forge.go` around lines 55 - 61, The ForgeExtensions.Location field in type ForgeExtensions (used by the parent struct field Extensions ForgeExtensions) appears unused; either remove the Location string field to avoid unnecessary struct noise or add a clear comment explaining it’s required to mirror the incoming Forge event payload (and keep json:"location,omitempty") so future readers know it’s intentionally preserved; update any related tests or JSON examples accordingly and ensure the parent struct still serializes/deserializes correctly after the change.forge/src/index.ts (1)
119-121: 💤 Low valueUpdate the stale
registercomment.The comment still states registration is a one-shot that is "refused" once
mm.registeredis set, but the handler now allows overwriting via{"force": true}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forge/src/index.ts` around lines 119 - 121, Update the stale comment for register to reflect that registration is no longer strictly one-shot: mention that while mm.registered indicates prior registration, the handler accepts an overwrite when the request includes {"force": true}; reference the register handler and mm.registered flag and note the new behavior and how to re-register (standard flow vs forced overwrite) so the comment matches the current implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forge/src/index.ts`:
- Around line 30-38: The storage.set call in enqueue can fail for values >240
KiB and currently rethrows (dropping the event); modify the enqueue flow around
enrichWithBody and storage.set to detect serialized size before write and fall
back to a trimmed payload: after calling enrichWithBody(evt) compute the byte
size of the object you intend to store (e.g., Buffer.byteLength(JSON.stringify({
event: enriched, context, enqueuedAt: Date.now() }))), and if it exceeds
~240*1024 bytes, remove or replace enriched.content.body with a lightweight
marker/summary or move only metadata (e.g., title, url, contentSnippet) and
optionally set a flag like bodyStripped=true so the consumer can fetch the full
body lazily; then attempt storage.set with the trimmed object and only throw if
that fails for non-size reasons. Ensure you reference and update the same
variables used in this block (enrichWithBody, enriched, storage.set, key,
context) so the retry/fallback uses the trimmed payload and preserves delivery.
In `@server/flow.go`:
- Around line 785-790: stepCloudForgeBridge currently renders
[{{.ForgeInstallURL}}]({{.ForgeInstallURL}}) unconditionally which produces a
broken link when GetForgeInstallURL() returns an empty string (ForgeInstallURL
not validated by Configuration.IsValid()). Fix by adding a guard or fallback: in
stepCloudForgeBridge (or the template data preparation) check
GetForgeInstallURL() / c.ForgeInstallURL and if empty render a non-link fallback
text like "Install the bridge on your site (installation URL not available)" or
omit the markdown link entirely; alternatively add validation in
Configuration.IsValid() to ensure ForgeInstallURL is set for Cloud installs so
the template never receives an empty value. Ensure references to
GetForgeInstallURL(), stepCloudForgeBridge, Configuration.IsValid(), and the
ForgeInstallURL field are updated accordingly.
---
Nitpick comments:
In `@forge/src/index.ts`:
- Around line 119-121: Update the stale comment for register to reflect that
registration is no longer strictly one-shot: mention that while mm.registered
indicates prior registration, the handler accepts an overwrite when the request
includes {"force": true}; reference the register handler and mm.registered flag
and note the new behavior and how to re-register (standard flow vs forced
overwrite) so the comment matches the current implementation.
In `@server/serializer/confluence_forge.go`:
- Around line 55-61: The ForgeExtensions.Location field in type ForgeExtensions
(used by the parent struct field Extensions ForgeExtensions) appears unused;
either remove the Location string field to avoid unnecessary struct noise or add
a clear comment explaining it’s required to mirror the incoming Forge event
payload (and keep json:"location,omitempty") so future readers know it’s
intentionally preserved; update any related tests or JSON examples accordingly
and ensure the parent struct still serializes/deserializes correctly after the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a451414-5e75-4d6d-8483-4ce345b45d09
📒 Files selected for processing (14)
forge/README.mdforge/manifest.ymlforge/src/index.tsplugin.jsonserver/client_cloud.goserver/command.goserver/confluence_server.goserver/flow.goserver/forge_poller.goserver/serializer/confluence_forge.goserver/service/mentions.goserver/service/mentions_test.goserver/service/notification_test.goserver/store/store.go
✅ Files skipped from review due to trivial changes (3)
- forge/manifest.yml
- plugin.json
- forge/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- server/service/mentions_test.go
- server/service/mentions.go
- server/confluence_server.go
Comment on lines
+785
to
+790
| text := "##### :white_check_mark: Install the Forge bridge for event delivery\n\n" + | ||
| "Event subscriptions on Confluence Cloud run through a small Forge app we publish.\n\n" + | ||
| "1. Install the bridge on your site: [{{.ForgeInstallURL}}]({{.ForgeInstallURL}}).\n" + | ||
| "2. After installation, open the install log (or run `forge webtrigger` against your tenant) and copy the **drain URL** and **register URL**.\n" + | ||
| "3. Click **Register bridge** below and paste both URLs. The plugin will register itself with the bridge and start polling for events.\n\n" + | ||
| ":lock: The shared secret never leaves the plugin — it is sent directly from the server to your bridge's register endpoint." |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect GetForgeInstallURL to confirm it can never be empty.
ast-grep --pattern 'func ($_ $_) GetForgeInstallURL() $_ { $$$ }'
rg -nP -C3 'GetForgeInstallURL' --type=goRepository: mattermost/mattermost-plugin-confluence
Length of output: 1206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ForgeInstallURL" --type=goRepository: mattermost/mattermost-plugin-confluence
Length of output: 632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ForgeInstallURL" --type=goRepository: mattermost/mattermost-plugin-confluence
Length of output: 632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ForgeInstallURL" --type=goRepository: mattermost/mattermost-plugin-confluence
Length of output: 632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all config/UI/default references to ForgeInstallURL (including non-Go files)
rg -n --hidden --glob '!**/node_modules/**' -S "forgeinstallurl|ForgeInstallURL" .
# Inspect the GetForgeInstallURL implementation and surrounding config wiring
sed -n '1,140p' server/config/main.go
sed -n '140,260p' server/config/main.go
# Inspect the flow state/template wiring around ForgeInstallURL usage
sed -n '130,260p' server/flow.go
sed -n '740,830p' server/flow.goRepository: mattermost/mattermost-plugin-confluence
Length of output: 11110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where forgeinstallurl / ForgeInstallURL is referenced besides the getter/flow text.
rg -n --hidden --glob '!**/node_modules/**' -S "forgeinstallurl|ForgeInstallURL" .
# Inspect plugin.json around the ForgeInstallURL config definition
sed -n '1,120p' plugin.json
sed -n '120,220p' plugin.json
# Inspect any config pages / handlers that might populate default values
ls -la server/config
rg -n --hidden --glob '!**/node_modules/**' "ForgeDrainURL|ForgeSharedSecret|forgedrainurl|forgesharedsecret" server/config server -SRepository: mattermost/mattermost-plugin-confluence
Length of output: 5911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any template usage for ForgeInstallURL
rg -n "{{\\.ForgeInstallURL}}" -S server
# Inspect the rest of server/flow.go around the Forge bridge registration step for any validations
sed -n '820,930p' server/flow.go
# Look for any explicit config validation for ForgeInstallURL
rg -n "ForgeInstallURL" server/config server/flow.go server/forge_poller.go server/plugin.go -SRepository: mattermost/mattermost-plugin-confluence
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find template usage for ForgeInstallURL (literal)
rg -n -F "{{.ForgeInstallURL}}" server || true
rg -n "ForgeInstallURL" server/config server/flow.go server/plugin.go server/forge_poller.go || true
# Show the full cloud-forge-bridge and submitCloudOAuthConfig sections
sed -n '650,860p' server/flow.goRepository: mattermost/mattermost-plugin-confluence
Length of output: 8716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find template usage for ForgeInstallURL (literal)
rg -n -F "{{.ForgeInstallURL}}" server || true
rg -n "ForgeInstallURL" server/config server/flow.go server/plugin.go server/forge_poller.go || true
# Show the full cloud-forge-bridge and submitCloudOAuthConfig sections
sed -n '650,860p' server/flow.goRepository: mattermost/mattermost-plugin-confluence
Length of output: 8716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Where IsValid is used
rg -n "IsValid\\(\\) error" -S server || true
rg -n "\\.IsValid\\(\\)" -S . || true
# Inspect server/config/main.go and plugin startup around config loading/validation
rg -n "ProcessConfiguration\\(|Sanitize\\(|GetConfig\\(" -S server || true
# Inspect server/plugin.go which likely loads/validates config
sed -n '1,220p' server/plugin.go
sed -n '220,520p' server/plugin.goRepository: mattermost/mattermost-plugin-confluence
Length of output: 9579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Where IsValid is used
rg -n "IsValid\\(\\) error" -S server || true
rg -n "\\.IsValid\\(\\)" -S . || true
# Inspect server/config/main.go and plugin startup around config loading/validation
rg -n "ProcessConfiguration\\(|Sanitize\\(|GetConfig\\(" -S server || true
# Inspect server/plugin.go which likely loads/validates config
sed -n '1,220p' server/plugin.go
sed -n '220,520p' server/plugin.goRepository: mattermost/mattermost-plugin-confluence
Length of output: 9579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect config validation logic
sed -n '1,120p' server/config/main.go
# Confirm there are no other assignments/defaults for ForgeInstallURL
rg -n "ForgeInstallURL\\s*=" server || true
rg -n "\"forgeinstallurl\"|ForgeInstallURL" server/config server || trueRepository: mattermost/mattermost-plugin-confluence
Length of output: 3727
Prevent broken “Forge Bridge Install URL” link in the Cloud wizard.
stepCloudForgeBridge renders [{{.ForgeInstallURL}}]({{.ForgeInstallURL}}) unconditionally. GetForgeInstallURL() only TrimSpaces c.ForgeInstallURL, and Configuration.IsValid() does not validate ForgeInstallURL, so it can be empty for Cloud installs—producing a broken/empty link while still prompting the user to install. Restore a fallback/guard (or validate ForgeInstallURL for Cloud setup) to avoid this.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/flow.go` around lines 785 - 790, stepCloudForgeBridge currently
renders [{{.ForgeInstallURL}}]({{.ForgeInstallURL}}) unconditionally which
produces a broken link when GetForgeInstallURL() returns an empty string
(ForgeInstallURL not validated by Configuration.IsValid()). Fix by adding a
guard or fallback: in stepCloudForgeBridge (or the template data preparation)
check GetForgeInstallURL() / c.ForgeInstallURL and if empty render a non-link
fallback text like "Install the bridge on your site (installation URL not
available)" or omit the markdown link entirely; alternatively add validation in
Configuration.IsValid() to ensure ForgeInstallURL is set for Cloud installs so
the template never receives an empty value. Ensure references to
GetForgeInstallURL(), stepCloudForgeBridge, Configuration.IsValid(), and the
ForgeInstallURL field are updated accordingly.
Contributor
Author
There was a problem hiding this comment.
Operator workflow assumes the System Console level Forge Bridge Install URL is populated before the install-cloud wizard is run since the field is secret: true and surfaced in System Console > Plugins > Confluence. If it's empty the wizard renders an empty markdown link
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
added 2 commits
June 3, 2026 17:40
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
nang2049
added a commit
that referenced
this pull request
Jun 12, 2026
…ridge POC (#228) * MM-68853: Confluence Cloud install path via OAuth 2.0 (3LO) + Forge bridge POC * MM-68853: Confluence Cloud install path via OAuth 2.0 (3LO) + Forge bridge POC * coderabbit comments * MM-68853: deliver Forge events to subscribed channels end-to-end * coderabbit feedbadck * PR feedback * MM-67455: DM connected users when mentioned in confluence (#229) * MM-67455: DM connected users when @-mentioned in Confluence * PR feedback * Linter * coderabbitai feedback * README.md --------- Co-authored-by: Nevyana Angelova <nevyangelova@192.168.100.47> * Update README.md * fix broen link * Send a DM notifying admins when the secret was regenerated and access was lost * fix spam * fix forge docs * switch app to production --------- Co-authored-by: Nevyana Angelova <nevyangelova@192.168.100.47>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on top of #228
When a Mattermost user is mentioned in a Confluence page or comment the plugin now sends them a direct message from the bot linking back to the page. Works on both Confluence Cloud via the Forge bridge from MM-68853 and Confluence Server.
Cloud fetches ADF body via
GET /wiki/api/v2/{pages,footer-comments,inline-comments}/{id}?body-format=atlas_doc_formatand wait for{type: "mention", attrs: {id}}nodes.DC fetches storage-format body via
GET /rest/api/content/{id}?expand=body.storageand extracts user ID from<ri:user ri:account-id="…"/>and legacy<ri:user ri:userkey="…"/>.New slash subcommand:
/confluence notifications on|offSetting is stored in KV per Mattermost user
Existing slash command UX fixes from thread in Mattermost https://hub.mattermost.com/pde/pl/tfzomf7r33dffn5artkq645wno install hidden from non-admins
Added granular scopes
read:page:confluenceandread:comment:confluenceto the OAuth consent ininstance_cloud.go. The v2 endpoints we now call don't accept the legacyread:confluence-content.allscope.Existing connected users have tokens without these scopes so their DMs will not fire until they reconnect. To handle this
the Cloud client returns
ErrCloudInsufficientScopeon 403. When the dispatcher sees it it DMs the user telling them to reconnect.When the user isn't a connected Mattermost user the existing webhook handler already falls back to
AdminAPITokenfor channel notifications. We now do the same for this mention dispatch. This dose not work on Cloud today. It would require shipping a Forge admin scoped token and I will open a ticket to follow up.Manual QA on Cloud:
/confluence notifications off: no DMManual QA on DC:
AdminAPIToken: recipient gets DMTicket Link
https://mattermost.atlassian.net/browse/MM-67455
Change Impact: 🔴 High
Reasoning: This PR introduces cross-cutting mention-based DM notifications that touch authentication (new OAuth scopes), event ingestion (Forge bridge), HTTP clients for Cloud/DC, parsing logic (ADF/XHTML), command UX, KV persistence, and DM dispatch—affecting core notification flows and connection resolution across multiple modules.
Regression Risk: High — changes modify connection storage semantics (reverse mapping skip for admin), add asynchronous DM dispatch paths, and introduce multiple mention-extraction implementations and OAuth-scope-dependent behaviors (ErrCloudInsufficientScope) that can break existing notification delivery or require user re-authentication; these touch widely-used flows and have numerous integration points.
** QA Recommendation:** Do not skip manual QA. Recommended tests: (1) Cloud ADF mention extraction (pages, footer comments, inline comments) including actor self-mentions and deduplication; (2) DC storage-format extraction including legacy ri:user keys; (3) Forge enrichment and drain behavior with storage limits and failure modes; (4) DM dispatch end-to-end with per-user notifications enabled/disabled and AdminAPIToken fallback on DC (and confirm Cloud fallback behavior); (5) behavior when Cloud tokens lack new scopes and the reconnect prompt flow; (6) regression tests for existing notification flows and StoreConnection reverse-mapping change; (7) slash-command UX and settings persistence; (8) concurrent/async dispatch error handling and logging. Prioritize integration tests and staged rollout to catch cross-layer regressions.
Generated by CodeRabbitAI