Skip to content

fix: parse provided payloads before replacing templated variables#449

Merged
zimeg merged 8 commits into
slackapi:mainfrom
zimeg:fix-variable-parsing
Jun 10, 2025
Merged

fix: parse provided payloads before replacing templated variables#449
zimeg merged 8 commits into
slackapi:mainfrom
zimeg:fix-variable-parsing

Conversation

@zimeg

@zimeg zimeg commented May 31, 2025

Copy link
Copy Markdown
Member

Summary

This PR parses the provided payload before replacing templated variables to avoid erroneous parsings from perhaps variables with linebreaks - fixes #427.

Requirements

@zimeg zimeg added this to the 2.2 milestone May 31, 2025
@zimeg zimeg self-assigned this May 31, 2025
@zimeg zimeg added bug Something isn't working semver:patch labels May 31, 2025
@codecov

codecov Bot commented May 31, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (6e4ac77) to head (3a520d6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #449   +/-   ##
=======================================
  Coverage   99.85%   99.86%           
=======================================
  Files           7        7           
  Lines         709      722   +13     
=======================================
+ Hits          708      721   +13     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zimeg

zimeg commented May 31, 2025

Copy link
Copy Markdown
Member Author

📣 A proof of concept for this fix is available in the following commits:

https://github.com/zimeg/slack-sandbox/tree/f2af0c9944c3802ffce73e270332c3d02c80a012

🔍 Test this as curious with a multline commit too perhaps:

- name: Run the release candidate
  uses: zimeg/slack-github-action@91855720aaee2e23bf81dae3512204a371ca1609 # v2.1.1-rc.1
  env:
    CHANNEL_ID: ${{ secrets.SLACK_CHANNEL_ID }}
  with:
    errors: true
    method: files.uploadV2
    token: ${{ secrets.SLACK_BOT_TOKEN }}
    payload-file-path: ./uploads.json  # Example below!
    payload-templated: true
{
  "channel_id": "${{ env.CHANNEL_ID }}",
  "initial_comment": "🚀 *New build*\n*Commit message:*\n${{ github.payload.head_commit.message }}\n",
  "file": "./README.md",
  "filename": "docs-${{ github.sha }}.md"
}
$ git commit
feat: attempt the odd after pushing this multiline commit

<3

@zimeg

zimeg commented May 31, 2025

Copy link
Copy Markdown
Member Author

📣 This link performs the same run but with debug logs, for the kind reviewer:

@WilliamBergamin WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet 🚀 this looks like a great fix!

Left a few non blocking comments

Comment thread src/content.js
Comment on lines +153 to +154
* @param {unknown} input - The initial value of the content.
* @returns {unknown} Content with templatized variables replaced.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet 💯 I like the simplified arguments, makes it easier to test 🚀

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WilliamBergamin Haha I started writing all possible types but also found that testing the actual behavior is more useful here 😉

Comment thread src/content.js
*/
const out = {};
for (const [k, v] of Object.entries(input)) {
out[k] = this.templatize(v);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid use of recursion 🥇

Comment thread src/content.js
Comment thread test/content.spec.js
@@ -257,14 +295,61 @@ describe("content", () => {
mocks.fs.readFileSync

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are pretty sweet 💯 should we also add a case with array options?

But this might be out of scope for this project because these elements mainly appear in interactive components like static multi-selects

Example

[
	{
		"type": "section",
		"block_id": "section678",
		"text": {
			"type": "mrkdwn",
			"text": "Pick items from the list"
		},
		"accessory": {
			"action_id": "text1234",
			"type": "multi_static_select",
			"placeholder": {
				"type": "plain_text",
				"text": "Select items"
			},
			"options": [
				{
					"text": {
						"type": "plain_text",
						"text": "*this is plain_text text*"
					},
					"value": "value-0"
				},
				{
					"text": {
						"type": "plain_text",
						"text": "*this is plain_text text*"
					},
					"value": "value-2"
				}
			]
		}
	}
]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WilliamBergamin This is a fascinating suggestion! 🧠 ✨

Right now we have a blocks array but that's at the top-level and doesn't promise we recurse as expected 🤓

I hadn't thought about sending interactive blocks from a GitHub Action but I'm now so curious about how this might connect with the same app listening for events in the workspace otherwise!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A similar example is added in 5c48fcf to confirm the templated replacement behavior 🚀

zimeg and others added 2 commits June 2, 2025 13:28
Co-authored-by: William Bergamin <william.bergamin.coen@gmail.com>

@WilliamBergamin WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work on the array support 💯

@zimeg

zimeg commented Jun 10, 2025

Copy link
Copy Markdown
Member Author

@WilliamBergamin @mwbrooks Thanks both for the most helpful reviews! I'll merge this now to close the related issue and will look into release related changes next 🚢 💨

@zimeg zimeg merged commit a3c435b into slackapi:main Jun 10, 2025
5 checks passed
@zimeg zimeg deleted the fix-variable-parsing branch June 10, 2025 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working semver:patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Templated payload variables are replaced before the payload is parsed

3 participants