Feature: implement Slack integration and notification pipeline - #395
Feature: implement Slack integration and notification pipeline#395Amit-max-ui wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughAdds Slack app settings support to the API and a new instance-admin Slack integration flow, including credential management, OAuth redirect display, integration listing, routing, breadcrumbs, and Vite startup configuration. ChangesSlack integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant SlackPage
participant SettingsAPI
participant SettingsStore
Admin->>SlackPage: Open Slack integration
SlackPage->>SettingsAPI: Load slack_app and auth config
SettingsAPI-->>SlackPage: Return settings and redirect base
Admin->>SlackPage: Submit credentials
SlackPage->>SettingsAPI: Update slack_app
SettingsAPI->>SettingsStore: Encrypt and save non-empty secrets
SettingsStore-->>SettingsAPI: Return updated settings
SettingsAPI-->>SlackPage: Return saved settings
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 4
🤖 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/handler/instance.go`:
- Line 287: Update the Slack secret registration near the "slack_app"
configuration so decrypted client_secret and signing_secret are excluded from
settings GET and PATCH responses. Preserve only the corresponding
client_secret_set and signing_secret_set flags in the API response, consistent
with the UI contract that plaintext secrets are never returned.
- Around line 523-557: Move the Slack-specific merge and secret-encryption logic
from the handler branch around the key "slack_app" into an instance-settings
service method, including the h.Settings.Get access. Update the handler to
delegate the Slack settings update to that service and use its returned value,
preserving default merging, client_id handling, and secret flags.
In `@apps/web/src/pages/instance-admin/InstanceAdminIntegrationsPage.tsx`:
- Around line 105-114: Update the integrations renderer in
InstanceAdminIntegrationsPage to group or filter providers by their category,
preserving the existing source-control section and adding a messaging section
for entries such as the Slack provider. Ensure the Slack integration is rendered
under messaging rather than beneath the static Source control heading, using the
existing category field.
- Around line 80-81: Update the Slack settings lookup in the page’s
settings-loading flow to read the backend’s `slack_app` section key instead of
`settings.slack`, while preserving the existing fallback to an empty object and
the subsequent setSlack call.
🪄 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: 22f82c41-dbe3-4cfa-92bf-0c440524ca3e
📒 Files selected for processing (8)
apps/api/internal/handler/instance.goapps/web/package.jsonapps/web/src/api/types.tsapps/web/src/components/layout/InstanceAdminLayout.tsxapps/web/src/pages/instance-admin/InstanceAdminIntegrationSlackPage.tsxapps/web/src/pages/instance-admin/InstanceAdminIntegrationsPage.tsxapps/web/src/pages/instance-admin/index.tsapps/web/src/routes/index.tsx
| "ai": {"api_key"}, | ||
| "image": {"unsplash_access_key"}, | ||
| "github_app": {"private_key", "client_secret", "webhook_secret"}, | ||
| "slack_app": {"client_secret", "signing_secret"}, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not serialize decrypted Slack secrets.
Registering these fields makes both settings GET and PATCH responses return plaintext Slack credentials via decryptSectionSecretsInternal, despite the UI contract that secrets are never echoed. Redact secret fields from API responses and return only their *_set flags.
🤖 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/handler/instance.go` at line 287, Update the Slack secret
registration near the "slack_app" configuration so decrypted client_secret and
signing_secret are excluded from settings GET and PATCH responses. Preserve only
the corresponding client_secret_set and signing_secret_set flags in the API
response, consistent with the UI contract that plaintext secrets are never
returned.
| if key == "slack_app" { | ||
| existing, _ := h.Settings.Get(c.Request.Context(), "slack_app") | ||
| merged := model.JSONMap{} | ||
|
|
||
| if existing != nil { | ||
| for k, v := range existing.Value { | ||
| merged[k] = v | ||
| } | ||
| } else { | ||
| for k, v := range defaultSettingValue("slack_app") { | ||
| merged[k] = v | ||
| } | ||
| } | ||
|
|
||
| /* plain feilds */ | ||
| for _, feild := range []string{"client_id"} { | ||
| if v, ok := req.Value[feild]; ok { | ||
| merged[feild] = v | ||
| } | ||
| } | ||
|
|
||
| /* Secret fields (Encrypt & Set flag) */ | ||
| setSecret := func(feild, setKey string) { | ||
| if v, ok := req.Value[feild]; ok { | ||
| if s, ok := v.(string); ok && s != "" { | ||
| merged[feild] = crypto.EncryptOrPlain(s) | ||
| merged[setKey] = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| setSecret("client_secret", "client_secret_set") | ||
| setSecret("signing_secret", "signing_secret_set") | ||
| value = merged | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep Slack settings updates in the service layer.
This new handler branch calls h.Settings.Get directly. Move Slack merge/encryption logic behind an instance-settings service so the handler delegates and the service owns store access.
As per path instructions, apps/api/**/*.go requires “handlers call services, services call stores.”
🤖 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/handler/instance.go` around lines 523 - 557, Move the
Slack-specific merge and secret-encryption logic from the handler branch around
the key "slack_app" into an instance-settings service method, including the
h.Settings.Get access. Update the handler to delegate the Slack settings update
to that service and use its returned value, preserving default merging,
client_id handling, and secret flags.
Source: Path instructions
| const sl = (settings.slack || {}) as InstanceSlackAppSection; | ||
| setSlack(sl); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Read the API’s slack_app section key.
The backend returns slack_app, but this reads settings.slack; the list therefore always receives {} and shows Slack as “Not configured” after credentials are saved.
-const sl = (settings.slack || {}) as InstanceSlackAppSection;
+const sl = (settings.slack_app || {}) as InstanceSlackAppSection;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sl = (settings.slack || {}) as InstanceSlackAppSection; | |
| setSlack(sl); | |
| const sl = (settings.slack_app || {}) as InstanceSlackAppSection; | |
| setSlack(sl); |
🤖 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/pages/instance-admin/InstanceAdminIntegrationsPage.tsx` around
lines 80 - 81, Update the Slack settings lookup in the page’s settings-loading
flow to read the backend’s `slack_app` section key instead of `settings.slack`,
while preserving the existing fallback to an empty object and the subsequent
setSlack call.
| category: 'source-control', | ||
| }, | ||
| { | ||
| id: 'slack', | ||
| name: 'Slack', | ||
| desc: 'Post Devlane notifications (assigned, state changed, commented, mentioned) to a Slack channel per project.', | ||
| Icon: IconSlack, | ||
| editPath: '/instance-admin/integrations/slack', | ||
| configured: isSlackConfigured(slack), | ||
| category: 'messaging', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render Slack under a messaging section.
category: 'messaging' is unused: the renderer maps every provider beneath the static “Source control” heading. Filter/group by category and add a messaging section so Slack is not mislabeled.
🤖 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/pages/instance-admin/InstanceAdminIntegrationsPage.tsx` around
lines 105 - 114, Update the integrations renderer in
InstanceAdminIntegrationsPage to group or filter providers by their category,
preserving the existing source-control section and adding a messaging section
for entries such as the Slack provider. Ensure the Slack integration is rendered
under messaging rather than beneath the static Source control heading, using the
existing category field.
|
@Amit-max-ui Please take a look at coderabbit's comments, Thank you for your contributions |
|
@martian56 Alright, I will check these codeRabbit comments, resolve them, and create a PR fixing those. |
You can push the fixes to this PR as well, instead of creating a new PR. Thanks |
2 participants
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
New Features
Bug Fixes