feat: adds Slack App construct#1304
Conversation
|
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? |
|
Fixed both AI comments. |
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. |
1de8529 to
dcf681d
Compare
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:
TypeScript compiles cleanly after the rebase. ObservationsTwo minor things worth being aware of in the codegen:
|
dcf681d to
504af91
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Affected Components
Notes for the Reviewer
Adds a new
SlackAppAlertChannelstandalone alert channel construct, following the same pattern asSlackAlertChannel.SlackAppAlertChannelconstruct withslackChannels: string[]config, synthesizing as typeSLACK_APPSlackAppAlertChannelCodegenfor code generation from imported resourcesAlertChannelCodegentype union and codegen map