Skip to content

chore: Update closed and opened templates to have compact look - #136

Merged
bbaarriiss merged 2 commits into
mainfrom
chore/more-compact-opened-and-closed-templates
Aug 14, 2026
Merged

chore: Update closed and opened templates to have compact look#136
bbaarriiss merged 2 commits into
mainfrom
chore/more-compact-opened-and-closed-templates

Conversation

@bbaarriiss

Copy link
Copy Markdown
Contributor

After we deployed the new notifications, we realized that the amount of emojis are eye-tiring and also the main identifier emojis are not catchy because of the crowded look of many emojis.

Below you will see the before/after comparison.

PR opened (OLD):
image

PR opened (NEW):
image

PR Merged (OLD):
image

PR Merged (NEW):
image

…ook with less repeating emojis. Only the main identifier emoji will stay.
@bbaarriiss bbaarriiss self-assigned this Aug 14, 2026
@bbaarriiss
bbaarriiss requested a review from a team as a code owner August 14, 2026 08:09
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review

Nice cleanup — the before/after screenshots make the case well, and collapsing the title into the PR link line is the right call. The change is small and low-risk. A few things I found while reading it.

Note: there's no CLAUDE.md in this repo, so I reviewed against .editorconfig and the conventions already established in src/main/resources/templates/en/.


1. pull_request.opened.template now ends with a dangling blank line (inconsistent with the closed template)

The blank line that used to separate the header from the detail block wasn't removed — it just ended up at the bottom of the file. Byte-level check:

pull_request.opened.template  ->  ... {{pullRequest.title}}\n\n   # trailing blank line
pull_request.closed.template  ->  ... {{pullRequest.title}}\n     # clean

Since Mustache emits the template body verbatim, the opened notification will carry a trailing empty line that the closed one doesn't. Both files are edited in this PR, so it's worth making them match — drop the final newline in pull_request.opened.template:4.

2. This changes user-facing output with no test covering it — and the golden files that look like they cover it are dead

src/test/fixtures/messages/*.txt looks exactly like a set of expected-output golden files, but:

  • grep -rn "fixtures" --include=*.kt src/ returns nothing — no Kotlin code reads them.
  • Their contents are a third format, older than both the pre- and post-PR templates. pull_request.opened.txt still says **[Eva22/Integrations]** Pull request **Modified MD** was opened by **Eva22**.
  • ApplicationTest mocks TemplateHandler outright (every { templateHandler.handleEvent(...) } returns DUMMY_TEMPLATE), so no test ever exercises real rendering.

So the only verification of this change is the manual screenshots in the description. That's acceptable for a two-line template tweak, but the stale fixtures are an active trap — the next person may well assume they're live and that a green build means the output is unchanged.

Worth a follow-up (not a blocker for this PR): a small TemplateHandlerTest that feeds each src/test/fixtures/events/*.json through TemplateHandler.handleEvent and asserts against the matching messages/*.txt, with those files regenerated. That fixture pairing is already sitting there by filename — it just needs wiring up. It would also have caught issue #1 above automatically.

3. {{pullRequest.title}} is HTML-escaped; sibling templates use {{{ }}} for the same field

This is pre-existing (the old 🔖 **Title:** line had it too), but you're touching the line, so it's cheap to fix now. Every other template in the directory uses triple braces for user-supplied titles:

issues.opened.template:1           Issue **{{{issue.title}}}** ...
issues.closed.template:1           Issue **{{{issue.title}}}** ...
issue_comment.created.template:1   on issue **{{{issue.title}}}**

...while both PR templates use {{pullRequest.title}}, which runs through mustache.java's HTML escaper (& -> &amp;, < -> &lt;, etc.).

Flagging with a caveat: whether this is actually visible depends on how Wire renders the message. If it parses markdown into HTML, the entities decode back and you'd never notice; if it renders to native attributed text, a PR titled fix: A & B shows up literally as fix: A &amp; B. I couldn't run it here to confirm which. Easy to check against a real PR title containing & — and if it does render wrong, {{{pullRequest.title}}} both fixes it and matches the rest of the directory.

4. Minor security note: PR titles are untrusted markdown, now rendered adjacent to a link

Anyone who can open a PR on a watched repo controls pullRequest.title, and it's now placed immediately after the PR link on the same line. A title like [click here](https://evil.example) renders as a live link in the Wire notification, and sitting right next to the real PR link it's more plausible as a spoof than it was on its own 🔖 **Title:** line.

To be clear, this is pre-existing and repo-wide, not introduced here — the triple-braced issue titles, comment bodies, and commit messages in the other templates have the same exposure, and note that HTML-escaping doesn't mitigate it either way since [, ], (, ) aren't escaped. Low severity given the trust model (you've presumably already decided to relay repo content into a channel). Not something this PR needs to solve; just worth being aware of if these notifications ever get pointed at public repos.

5. Follow-up: the two notification styles are now further apart

pull_request.opened / pull_request.closed use the emoji-header style; issues.*, push, commit_comment.created, issue_comment.created, and both pull_request_review* templates still use the older **[repo]** ... — style with the em-dash terminator. Entirely fine to defer — but if the compact style is the direction, the rest are a natural next PR so a channel doesn't show two visibly different formats.


Performance: no impact. DefaultMustacheFactory caches compiled templates internally, and this doesn't change the number or shape of tags.

Verdict: approve modulo #1 (the trailing newline — one-character fix). #2 and #3 are worth a look but shouldn't hold this up.

@bbaarriiss
bbaarriiss merged commit 545c536 into main Aug 14, 2026
5 checks passed
@bbaarriiss
bbaarriiss deleted the chore/more-compact-opened-and-closed-templates branch August 14, 2026 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants