Skip to content

feat: adds Slack App construct#1304

Merged
sorccu merged 5 commits into
mainfrom
tnolet/add-slack-app-construct
May 27, 2026
Merged

feat: adds Slack App construct#1304
sorccu merged 5 commits into
mainfrom
tnolet/add-slack-app-construct

Conversation

@tnolet
Copy link
Copy Markdown
Member

@tnolet tnolet commented May 20, 2026

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Adds a new SlackAppAlertChannel standalone alert channel construct, following the same pattern as SlackAlertChannel.

  • SlackAppAlertChannel construct with slackChannels: string[] config, synthesizing as type SLACK_APP
  • SlackAppAlertChannelCodegen for code generation from imported resources
  • Registered in AlertChannelCodegen type union and codegen map
  • AI context updated with fixture, example config, and reference docs

@tnolet tnolet requested a review from sorccu May 20, 2026 08:47
@thebiglabasky
Copy link
Copy Markdown
Contributor

Just ran a review agent real quick, but didn't yet take the time to review myself, so posting more as an FYI than as a conviction. Usually these findings are relevant. I'll review this afternoon.

AI-generated comments below:

Findings

Critical: packages/cli/src/constructs/slack-app-alert-channel-codegen.ts:12 uses SlackAlertChannel as the generated construct for SLACK_APP. The new class is SlackAppAlertChannel, but import/codegen instantiates the legacy Slack webhook construct while passing slackChannels. I confirmed the runtime effect: new SlackAlertChannel(..., { slackChannels: ['#ops'] }).synthesize() returns {"type":"SLACK","config":{}}, so JS imports silently become the wrong channel type, and TS imports fail because SlackAlertChannelProps requires url. This breaks import plan output for every Slack App alert channel. Change the codegen construct to SlackAppAlertChannel.

Important: packages/cli/src/constructs/slack-app-alert-channel.ts:4 makes slackChannels optional, so new SlackAppAlertChannel('x', {}) type-checks and synthesizes {"type":"SLACK_APP","config":{}}. The backend alert-channel schemas require at least one #channel or @handle, and the delivery path expects config.slackChannels.length. The construct should make slackChannels required at minimum, and ideally validate non-empty entries with the same #/@ prefix rule before deploy.

Verification

I checked the PR diff against the GitHub base/head SHAs and inspected the surrounding alert-channel/codegen patterns. I ran npm ci, npm run prepare:dist --workspace packages/cli, and npm exec --workspace packages/cli -- vitest --run src/constructs/tests/alert-channel.spec.ts; the build and targeted existing tests passed. I did not post any GitHub review comments.

@tnolet
Copy link
Copy Markdown
Member Author

tnolet commented May 20, 2026

Just ran a review agent real quick, but didn't yet take the time to review myself, so posting more as an FYI than as a conviction. Usually these findings are relevant. I'll review this afternoon.

AI-generated comments below:

Findings

Critical: packages/cli/src/constructs/slack-app-alert-channel-codegen.ts:12 uses SlackAlertChannel as the generated construct for SLACK_APP. The new class is SlackAppAlertChannel, but import/codegen instantiates the legacy Slack webhook construct while passing slackChannels. I confirmed the runtime effect: new SlackAlertChannel(..., { slackChannels: ['#ops'] }).synthesize() returns {"type":"SLACK","config":{}}, so JS imports silently become the wrong channel type, and TS imports fail because SlackAlertChannelProps requires url. This breaks import plan output for every Slack App alert channel. Change the codegen construct to SlackAppAlertChannel.

Important: packages/cli/src/constructs/slack-app-alert-channel.ts:4 makes slackChannels optional, so new SlackAppAlertChannel('x', {}) type-checks and synthesizes {"type":"SLACK_APP","config":{}}. The backend alert-channel schemas require at least one #channel or @handle, and the delivery path expects config.slackChannels.length. The construct should make slackChannels required at minimum, and ideally validate non-empty entries with the same #/@ prefix rule before deploy.

Verification

I checked the PR diff against the GitHub base/head SHAs and inspected the surrounding alert-channel/codegen patterns. I ran npm ci, npm run prepare:dist --workspace packages/cli, and npm exec --workspace packages/cli -- vitest --run src/constructs/tests/alert-channel.spec.ts; the build and targeted existing tests passed. I did not post any GitHub review comments.

@thebiglabasky why are you reviewing this?

@tnolet
Copy link
Copy Markdown
Member Author

tnolet commented May 20, 2026

Fixed both AI comments.

@thebiglabasky
Copy link
Copy Markdown
Contributor

@thebiglabasky why are you reviewing this?

I've worked a fair bit on CLI and recently did the constructs and codegen for agentic check, so thought the more the merrier. Happy to drop and do something else though 🤷🏻

@tnolet
Copy link
Copy Markdown
Member Author

tnolet commented May 20, 2026

@thebiglabasky why are you reviewing this?

I've worked a fair bit on CLI and recently did the constructs and codegen for agentic check, so thought the more the merrier. Happy to drop and do something else though 🤷🏻

ah, ok. Thank for the offer but there is no need to distract you with this.

@sorccu sorccu force-pushed the tnolet/add-slack-app-construct branch from 1de8529 to dcf681d Compare May 22, 2026 14:59
@sorccu
Copy link
Copy Markdown
Member

sorccu commented May 22, 2026

Rebased onto main (post-v8.0.0)

Rebased the 3 commits onto current main, which includes the v8.0.0 ESM migration. Changes made during rebase:

  • Resolved conflicts in alert-channel-codegen.ts and index.ts — took main's ESM imports, added Slack App entries with .js suffixes
  • Updated imports in both new files (slack-app-alert-channel.ts, slack-app-alert-channel-codegen.ts) to ESM style: .js suffixes on all local imports, index.js for barrel re-exports from ./internal/codegen and ../sourcegen

TypeScript compiles cleanly after the rebase.

Observations

Two minor things worth being aware of in the codegen:

  1. Fallback name when slackChannels is undefined: In prepare(), if slackChannels is undefined the fallback name becomes "slack-app-undefined". Unlikely in practice since the API should always provide channels for a SLACK_APP type, but worth noting.

  2. slackChannels?.concat().slice(-8) operates on array elements, not string characters — it takes the last 8 channel names from the array. This differs from the existing Slack codegen's url.slice(-4) which takes the last 4 characters of a string. The resulting fallback name may be quite long if the channel names are long.

@sorccu sorccu force-pushed the tnolet/add-slack-app-construct branch from dcf681d to 504af91 Compare May 27, 2026 05:58
Comment thread packages/cli/src/constructs/slack-app-alert-channel-codegen.ts Outdated
Comment thread packages/cli/src/constructs/slack-app-alert-channel-codegen.ts Outdated
Comment thread packages/cli/src/constructs/slack-app-alert-channel-codegen.ts Outdated
sorccu and others added 2 commits May 27, 2026 18:05
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sorccu sorccu merged commit 169e1f4 into main May 27, 2026
8 checks passed
@sorccu sorccu deleted the tnolet/add-slack-app-construct branch May 27, 2026 09:28
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.

3 participants