Skip to content

fix: show single success emoji in completion notifications#70

Merged
adamrtalbot merged 2 commits into
mainfrom
fix/66-single-success-emoji
Jun 30, 2026
Merged

fix: show single success emoji in completion notifications#70
adamrtalbot merged 2 commits into
mainfrom
fix/66-single-success-emoji

Conversation

@adamrtalbot

Copy link
Copy Markdown
Collaborator

Summary

  • Remove the duplicate ✅ from the default Status field on pipeline success notifications
  • Keep the checkmark in the message header (✅ *Pipeline completed successfully*) so success is still visually indicated once in the message body
  • Applies to both the default complete message and map-based custom messages that include the status field

Closes #66

Test plan

  • ./gradlew test --tests "nextflow.slack.SlackMessageBuilderTest.should build workflow complete message"
  • ./gradlew test --tests "nextflow.slack.SlackMessageBuilderTest.should use map-based custom complete message with selective fields"
  • ./gradlew test --tests "nextflow.slack.SlackMessageBuilderTest.should use custom complete message template"

Made with Cursor

Remove duplicate checkmark from the default Status field on pipeline
success; keep the emoji in the message header only.

Closes #66

Generated by Codex

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Code Review

Fix looks correct and focused. The duplicate ✅ issue is real — the header already carries it.

One concern: inconsistency in getStatusEmoji

The method now returns mixed emoji coverage:

  • started🚀 Running
  • completedSuccess ← no emoji
  • failed❌ Failed

This is used in map-based custom messages where the caller supplies their own header (e.g. 🎉 *Analysis finished!*), so those messages will show a plain Success with no visual indicator while started/failed still get emojis. Consider either renaming the method (it is misleadingly called getStatusEmoji) or keeping a neutral emoji for completed in that path so all three states are visually consistent when there is no default header.

Tests

Existing tests pass, but they use .contains('Success') which matched both before and after this change — they don't guard against the emoji being re-added. A simple !it.text.contains('✅') assertion would make the intent explicit. Not a blocker.

Docs / Examples

No changes needed for a fix this small.

Status field no longer repeats header emojis for success, failure, or
running states in default and map-based custom messages.

Generated by Codex

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Code Review

This PR removes the duplicate status emoji from the Status field in Slack notification blocks, keeping the emoji only in the message header. The rename from getStatusEmojigetStatusText is clean and complete — no orphaned call sites remain.


Finding: Custom messages with omitted text lose all emoji in the header

File: src/main/groovy/nextflow/slack/SlackMessageBuilder.groovy ~line 474

In buildCustomMessage, the header is set to customConfig.text ?: getDefaultMessageText(status). The fallback getDefaultMessageText returns bare strings with no emoji (e.g. '*Pipeline completed successfully*'), while the standard non-custom path hard-codes them (e.g. '✅ *Pipeline completed successfully*').

Before this PR: a custom message with no text key got emoji in the Status field — ugly but present.
After this PR: emoji disappears entirely (header has none, status field has none).

The new tests at line 924–967 all supply explicit emoji in text ('🚀 *Starting*', '✅ *Done*', '❌ *Boom*'), so the fallback path is untested and this regression is undetected.

Fix: Either add emoji to getDefaultMessageText returns, or add a test asserting the fallback-text path still surfaces a visible status indicator.


Tests

The three new tests are well-structured and correctly cover the intended behavior. The statusFieldValue helper is safe — the positive == 'Success' assertion guards against false-negatives before the negative emoji check.

Documentation / Examples

No README or example config updates accompany the behavior change. If the project documents what the Status field looks like (screenshots, example configs), those should be updated.

@adamrtalbot adamrtalbot merged commit 5d59206 into main Jun 30, 2026
6 checks passed
@adamrtalbot adamrtalbot deleted the fix/66-single-success-emoji branch June 30, 2026 15:22
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.

Feature: By default, only show one emoji on pipeline success

1 participant