feat: complete Slack integration UI and models - #396
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Slack OAuth installation, workspace channel linking, project notification settings, asynchronous Slack posting, and frontend controls for configuring and disconnecting Slack integrations. ChangesSlack Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
apps/web/src/components/integrations/IntegrationsSection.tsx (1)
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
IconSlackis duplicated verbatim inapps/web/src/pages/instance-admin/InstanceAdminIntegrationsPage.tsx(lines 17-21). Consider extractingIconSlack/IconGitHubinto a shared provider-icon module so paths and sizing stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/integrations/IntegrationsSection.tsx` around lines 24 - 28, IconSlack is duplicated across IntegrationsSection and InstanceAdminIntegrationsPage, risking inconsistent provider icons. Extract IconSlack and IconGitHub into a shared provider-icon module, then update both consumers to import and reuse those shared components while preserving their current paths, sizing, and accessibility attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/internal/service/issue.go`:
- Around line 649-651: Update the issue-creation flow around IssueService so it
no longer calls NotificationService directly via s.notify.IssueCreated. Persist
a created-event or outbox record through the store, preferably within the issue
transaction, and leave notification delivery to the process that consumes that
record while preserving the services-call-stores boundary.
In `@apps/api/internal/service/notification.go`:
- Around line 525-530: Update the Slack publish flow around PublishSlackPost to
handle its returned error instead of discarding it. On failure, emit a warning
containing the relevant issue, project, and channel identifiers; if an existing
outbox or retry mechanism is available, persist the notification for retry as
well.
- Around line 516-530: Keep decrypted Slack credentials out of queued payloads
by passing only the integration/link identifier and message data. In
apps/api/internal/service/notification.go lines 516-530, remove Token from the
SlackPostPayload and include the identifier needed for resolution; update
apps/api/internal/queue/queue.go lines 56-62 to replace Token with that
identifier; in apps/api/internal/queue/consumer.go lines 163-192, resolve the
credential immediately before invoking Slack; and in apps/api/cmd/api/main.go
lines 129-130, inject the resolver/store into the Slack handler instead of
passing slack.PostMessage directly.
In `@apps/api/internal/slack/notification.go`:
- Around line 9-20: Update BuildSlackMessage to entity-escape dynamic mrkdwn
values—issueRef, title, actor, and action—before interpolating them into text
and block text, using the repository’s existing Slack escaping utility if
available. Preserve the intentional link markup around link and the existing
blocks return type.
In `@apps/web/src/components/integrations/SlackChannelSettingsModal.tsx`:
- Around line 78-84: Update the channel-linking flow around selectedChannelId
and slackLinkProjectChannel to use the existing translation mechanism for the
missing-channel error, return after setting that error, and avoid persisting a
placeholder channel name. Require a matching channel from channels before
calling slackLinkProjectChannel, using its actual name and preserving the
existing setError handling.
- Around line 193-238: Update handleSave so that when creating a new Slack
channel link, it follows the successful link request with
slackUpdateProjectChannel using the current eventCreated, eventStateChanged, and
eventCommented values. Ensure the modal only closes after both operations
succeed, while preserving the existing behavior for already-linked channels.
In `@apps/web/src/pages/instance-admin/InstanceAdminIntegrationsPage.tsx`:
- Around line 89-99: Update the integrations page sectioning around the Slack
entry and the existing source-control heading so Slack is not presented as a
source-control provider. Either categorize entries into separate Source control
and Communications sections using IntegrationsSection, or replace the heading
with a provider-agnostic label while preserving the existing integration rows.
In `@apps/web/src/services/integrationService.ts`:
- Around line 217-231: The slackGetProjectChannel method currently only
normalizes a 404 response, but the backend represents an unlinked channel with a
successful response whose data is null. Update the response handling in
slackGetProjectChannel to return null when the successful data value is null,
while preserving the existing 404 handling and rethrowing other errors.
---
Nitpick comments:
In `@apps/web/src/components/integrations/IntegrationsSection.tsx`:
- Around line 24-28: IconSlack is duplicated across IntegrationsSection and
InstanceAdminIntegrationsPage, risking inconsistent provider icons. Extract
IconSlack and IconGitHub into a shared provider-icon module, then update both
consumers to import and reuse those shared components while preserving their
current paths, sizing, and accessibility attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 225327a1-43ae-4d65-945a-0c973cff3920
📒 Files selected for processing (22)
apps/api/cmd/api/main.goapps/api/internal/handler/integration.goapps/api/internal/model/notification.goapps/api/internal/model/slack.goapps/api/internal/oauth/slack.goapps/api/internal/queue/consumer.goapps/api/internal/queue/queue.goapps/api/internal/router/router.goapps/api/internal/service/integration.goapps/api/internal/service/issue.goapps/api/internal/service/notification.goapps/api/internal/slack/client.goapps/api/internal/slack/notification.goapps/api/internal/store/integration.goapps/api/internal/store/slack.goapps/api/migrations/000013_slack_integration.down.sqlapps/api/migrations/000013_slack_integration.up.sqlapps/web/src/api/types.tsapps/web/src/components/integrations/IntegrationsSection.tsxapps/web/src/components/integrations/SlackChannelSettingsModal.tsxapps/web/src/pages/instance-admin/InstanceAdminIntegrationsPage.tsxapps/web/src/services/integrationService.ts
| if s.notify != nil { | ||
| s.notify.IssueCreated(ctx, issue, userID) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Preserve the service/store boundary.
IssueService now invokes NotificationService directly. Persist a created-event/outbox row through a store (ideally in the issue transaction), then let notification delivery process that row.
As per coding guidelines, apps/api/**/*.go: “handlers call services, services call stores; handlers must never touch GORM directly, and stores must never call services.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/internal/service/issue.go` around lines 649 - 651, Update the
issue-creation flow around IssueService so it no longer calls
NotificationService directly via s.notify.IssueCreated. Persist a created-event
or outbox record through the store, preferably within the issue transaction, and
leave notification delivery to the process that consumes that record while
preserving the services-call-stores boundary.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/internal/queue/consumer.go`:
- Line 204: Update the message-posting flow around poster to derive a
per-message context with a finite timeout before invoking poster, and pass that
bounded context instead of the long-lived consumer context. Ensure the timeout
is always cancelled and retain the existing error handling and
message-processing behavior.
In `@apps/web/src/components/integrations/SlackChannelSettingsModal.tsx`:
- Around line 89-102: Update the save flow in SlackChannelSettingsModal so
onSaved is called immediately with the successful result from
slackLinkProjectChannel before invoking slackUpdateProjectChannel. Keep the
event-settings update afterward, and only call onSaved again with the updated
channel when that update succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63d09e98-30bf-4fec-9284-0bd3bc8885fb
📒 Files selected for processing (10)
apps/api/cmd/api/main.goapps/api/internal/queue/consumer.goapps/api/internal/queue/queue.goapps/api/internal/service/notification.goapps/api/internal/slack/notification.goapps/web/src/components/icons/IntegrationIcons.tsxapps/web/src/components/integrations/IntegrationsSection.tsxapps/web/src/components/integrations/SlackChannelSettingsModal.tsxapps/web/src/pages/instance-admin/InstanceAdminIntegrationsPage.tsxapps/web/src/services/integrationService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/pages/instance-admin/InstanceAdminIntegrationsPage.tsx
1 participant
Summary
Linked issues
Closes #
Type of change
feat:) — user-visible new capabilityfix:) — corrects broken behaviorrefactor:) — no behavior change, internal onlyperf:) — measurable improvementdocs:) — README / CLAUDE.md / planning docs onlytest:) — adds or corrects testschore:) — deps, tooling, CI, formattingstyle:) — visual / theming polish onlySurface
apps/api/)apps/web/)apps/api/migrations/)What changed
Why this approach
Database / migrations
apps/api/migrations/NNNNNN_<name>.up.sqlAND matching.down.sqldatabase.RunMigrationson startupBreaking changes
Test plan
npm run validate(root) — typecheck + lint + prettier + go vet + go test[ ] Manual smoke test of the affected flow:
Screenshots / recordings (UI changes)
Rollout notes
AI assistance
____— and AI-assisted commits include aCo-Authored-By:trailerChecklist
--no-verifybypass)internal/config/config.goand documented above.envvalues committedSummary by CodeRabbit