fix: only notify registration when actually adding a new webhook

[fewerner]

---

PR Submission Checklist for internal contributors

• The PR Title

  • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
  • contains a reference JIRA issue number like SQPIT-764
  • answers the question: If merged, this PR will: ... ³

• The PR Description

  • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

---

What's new in this PR?

Issues

Briefly describe the issue you have solved or implemented with this pull request. If the PR contains multiple issues, use a bullet list.

Causes (Optional)

Briefly describe the causes behind the issues. This could be helpful to understand the adopted solutions behind some nasty bugs or complex issues.

Solutions

Briefly describe the solutions you have implemented for the issues explained above.

Dependencies (Optional)

If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.

Needs releases with:

• GitHub link to other pull request

Testing

Test Coverage (Optional)

• I have added automated test to this contribution

How to Test

Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.

Notes (Optional)

Specify here any other facts that you think are important for this issue.

Attachments (Optional)

Attachments like images, videos, etc. (drag and drop in the text box)

---

PR Post Submission Checklist for internal contributors (Optional)

• Wire's Github Workflow has automatically linked the PR to a JIRA issue

---

PR Post Merge Checklist for internal contributors

• If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

---

References

1. https://sparkbox.com/foundry/semantic_commit_messages
2. https://github.com/wireapp/.github#usage
3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

[claude]

PR Review: only notify registration when actually adding a new webhook

Thanks for tightening up the notification logic — distinguishing "newly subscribed" from "already subscribed" is the right call, and threading it through a typed RegistrationResult is clean. A few issues to address, one of which is blocking.

🔴 Blocking: missing comma — this will not compile. In EventsHandler.kt, the onSuccess argument is no longer separated from onFailure by a comma. The closing brace of the onSuccess lambda is immediately followed by onFailure = { exception -> ... } with no comma between them. Result.fold(onSuccess, onFailure) takes two arguments; without the comma this is a syntax error. (The original code had the trailing comma and it was dropped in this change.)

🔴 null leaks into the user-facing message. After the comma is fixed, fold now returns String?, so results is List<String?> and return results.joinToString(separator = "\n") renders a null element as the literal text "null". So when a user posts a link for an already-subscribed repo, the bot replies with null — the opposite of the intended "stay quiet" behavior. Drop the nulls, e.g. results.filterNotNull().joinToString(...).

🔴 Empty message gets sent when nothing is new. Closely related: if every repository in the message is already subscribed, the filtered list is empty and joinToString yields an empty string. onTextMessageReceived then calls sendMessage(text = ""), posting an empty Wire message. Guard against this — only send when the result is non-blank (if (message.isNotBlank()) sendMessage(...)).

🟡 Retry after a failed provisioning now produces no confirmation. sadd(conversationsKey, ...) runs before webhook provisioning. If ensureRepositoryWebhook throws on the first attempt, the conversation is already in the set but no webhook_id was stored. On the retry, newlySubscribed is now false (sadd returns 0), the webhook is created successfully this time, and the result is null → the user gets no confirmation despite a successful subscribe. Consider basing the notify decision on whether the webhook was actually (re)provisioned — or on webhook_id transitioning from absent to present — rather than solely on set membership.

🟡 Behavior change: webhook is no longer re-verified against GitHub. Previously ensureRepositoryWebhook was called on every registration, making it self-healing (findRepositoryWebhook then update-or-create). Now, if a webhook_id exists in Redis the GitHub call is skipped entirely. This is a nice latency/rate-limit win, but a webhook deleted on the GitHub side (or a stale/invalid stored id) will never be recreated. Worth a deliberate decision and maybe a comment noting the trade-off.

🟢 Minor. RegistrationResult.webhookId is no longer used by the caller. Fine to keep for future use, but flagging in case it is dead weight.

Tests. There is no coverage for GitHubWebhookManager or registerRepositories (only ApplicationTest.kt exists). The null/empty-message bugs above are exactly what a small unit test would have caught — e.g. registering an already-subscribed repo returns no notification, and a failed-then-retried provisioning still confirms. Mocking storage/gitHubAppClient (or fakeredis) would make this testable.

Summary: the intent and structure are good, but it will not compile as-is (missing comma), and once it does there are two user-visible regressions (the null reply and empty messages). I would resolve those three before merge.