Rb sept 15 slack integration#2200
Conversation
…into rb-sept-15-slack-integration
…into rb-sept-15-slack-integration
…into rb-sept-15-slack-integration
…into rb-sept-15-slack-integration
…into rb-sept-15-slack-integration
WalkthroughAdds end-to-end Slack integration: new env vars for encryption, client env mapping and permissions, React hook/UI and repository methods, server routes/controllers, Sequelize model/migration and utilities, encryption tools, Slack messaging service, and new server dependencies and queues. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client as Client (Settings/Slack)
participant Slack as Slack OAuth
participant Server as API (/api/slackWebhooks)
participant DB as DB
User->>Client: Click "Add to Slack"
Client->>Slack: Redirect with client_id & scopes
Slack-->>Client: Redirect back with code or error
Client->>Server: POST /slackWebhooks { code }
Server->>Slack: Exchange code for tokens
Slack-->>Server: Tokens + workspace/channel info
Server->>DB: Create SlackWebhook (encrypted fields)
DB-->>Server: Record created
Server-->>Client: 201 Created + integration data
Client-->>User: Show success and list integrations
note right of Server #DDDDDD: access_token/url encrypted at rest
sequenceDiagram
autonumber
actor User
participant Client as Client (SlackIntegrations)
participant API as API (/api/slackWebhooks/:id/send)
participant Svc as SlackNotificationService
participant Slack as Slack API
User->>Client: Click "Send Test"
Client->>API: POST :id/send { message }
API->>Svc: sendImmediateMessage(integration, message)
Svc->>Svc: Decrypt token
Svc->>Slack: chat.postMessage(channel, blocks)
Slack-->>Svc: OK (ts, channel)
Svc-->>API: success payload
API-->>Client: 200 OK
Client-->>User: Show success alert
alt Error
Slack-->>Svc: Error
Svc-->>API: Error thrown
API-->>Client: 4xx/5xx
Client-->>User: Show error alert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
rachanabasnet
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (25)
.env.dev (1)
33-35: Configure real encryption settings via secrets manager; don’t commit real valuesUse a concrete algorithm (e.g., aes-256-cbc) and source ENCRYPTION_PASSWORD from a secret manager in non-dev. Ensure these match Servers/tools/createSecureValue.ts expectations.
Please confirm:
- Algorithm used by createSecureValue.ts matches ENCRYPTION_ALGORITHM.
- Production uses a secret store (not committed .env) for ENCRYPTION_PASSWORD.
.env.prod (1)
35-37: Use a secret manager for production and plan key rotationDo not store real ENCRYPTION_PASSWORD in .env files. Store in a secret manager and document rotation plus re-encryption steps for persisted data.
Servers/index.ts (1)
70-72: Ensure parseOrigins handles string fallback; otherwise wrap as JSON arrayIf parseOrigins expects a JSON array, passing a plain string can misconfigure CORS. Safer fallback:
- const allowedOrigins = parseOrigins( - process.env.ALLOWED_ORIGINS || frontEndUrl, - ); + const allowedOrigins = parseOrigins( + process.env.ALLOWED_ORIGINS || JSON.stringify([frontEndUrl]), + );Servers/package.json (1)
20-20: Remove or defer unused queue/scheduling deps (bull, redis, ioredis, node-cron)Servers/services/slackNotificationService.ts imports Queue and redis but neither is referenced; package.json still lists bull, ioredis, redis, node-cron — remove the unused imports and uninstall or defer these runtime deps to reduce attack surface and cold-start time.
Location: Servers/services/slackNotificationService.ts
Servers/domain.layer/interfaces/i.slackWebhook.ts (2)
3-4: Align IV fields with actual usage (DB requires them; public interface likely shouldn’t expose them).
- In the migration, access_token_iv and url_iv are NOT NULL, but here they’re optional.
- If this interface is a public/server DTO (not the ORM model), consider removing IV fields entirely to avoid leaking cryptographic metadata. If it’s the persistence shape, mark them required.
Apply this diff if the interface is intended as a public/server DTO:
- access_token_iv?: string; + // iv is not exposed in public DTO access_token: string; @@ - url_iv?: string; + // iv is not exposed in public DTOAlso applies to: 12-13
13-13: Clarify comment for url.“URL of the slack workspace” is misleading; this is typically the incoming webhook URL or a Slack API resource URL. Clarify to avoid misuse.
Apply this diff:
- url: string; // URL of the slack workspace + url: string; // Incoming Webhook URL (or Slack API destination URL), not the workspace homepageClients/src/application/hooks/useSlackIntegrations.tsx (2)
35-41: Guard against null userId and remove non-null assertion.Avoid calling the API when userId is null and remove the unsafe userId! usage.
Apply this diff:
-const useSlackIntegrations = (userId: number | null) => { +const useSlackIntegrations = (userId: number | null) => { @@ - const fetchSlackIntegrations = async () => { + const fetchSlackIntegrations = async () => { + if (!userId) { + setSlackIntegrations([]); + setLoading(false); + return; + } try { - const controller = new AbortController(); - const signal = controller.signal; + const controller = new AbortController(); + const signal = controller.signal; setLoading(true); - const response = await getSlackIntegrations({ id: userId!, signal }); + const response = await getSlackIntegrations({ id: userId, signal }); @@ - useEffect(() => { - fetchSlackIntegrations(); - }, []); + useEffect(() => { + const controller = new AbortController(); + if (userId) { + // reuse same controller in fetch if you thread it through; otherwise keep this simple + fetchSlackIntegrations(); + } else { + setLoading(false); + } + return () => controller.abort(); + }, [userId]);Also applies to: 75-77
4-19: Avoid duplicating server types on the client.These types will drift. Prefer a client-facing DTO type or a generated type from the API schema.
Consider importing a shared API type (if available) or defining a minimal client DTO limited to what the UI needs.
Also applies to: 20-29
Clients/src/presentation/pages/SettingsPage/index.tsx (1)
31-35: Preserve deep-linking by updating the activeTab query param instead of deleting it.Deleting the param after tab change defeats deep-linking. Update it to the new tab.
Apply this diff:
- useEffect(() => { - if (activeSetting) { - setActiveTab(activeSetting); - } - }, [activeSetting]); + useEffect(() => { + if (activeSetting) setActiveTab(activeSetting); + }, [activeSetting]); @@ const handleTabChange = (_: React.SyntheticEvent, newValue: string) => { setActiveTab(newValue); - if (activeSetting) { - searchParams.delete("activeTab"); - setSearchParams(searchParams); - } + setSearchParams({ activeTab: newValue }); };Also applies to: 38-44
Servers/database/migrations/20250916121737-create-slack-webhook.js (2)
21-24: Use TEXT for potentially long fields (scope, configuration_url, url).Slack scopes and URLs can exceed 255 chars; Sequelize.STRING may truncate.
Apply this diff:
scope: { - type: Sequelize.STRING, + type: Sequelize.TEXT, allowNull: false, }, @@ configuration_url: { - type: Sequelize.STRING, + type: Sequelize.TEXT, allowNull: false, }, url: { - type: Sequelize.STRING, + type: Sequelize.TEXT, allowNull: false, },Also applies to: 51-58
80-83: Consider uniqueness and additional indexing.Optionally add a unique index on (team_id, channel_id) to prevent duplicate channel entries per workspace; and an index on user_id for fast lookups.
Servers/tools/createSecureValue.ts (1)
28-52: Prefer AEAD (e.g., AES-256-GCM) over CBC and include auth tag.CBC lacks integrity/authentication. Consider switching to AES-GCM and returning authTag to store alongside iv.
If you want, I can provide a drop-in GCM version and a migration plan to add auth_tag columns (access_token_auth_tag, url_auth_tag). Would you like that?
Also applies to: 57-86
Servers/services/slackNotificationService.ts (3)
1-11: Remove unused Bull/Redis artifacts.
Queue,redis,redisClient,notificationQueue, andREDIS_URLare unused. Drop them to reduce noise.Apply:
-import Queue from "bull"; -import redis from "redis"; import logger from "../utils/logger/fileLogger"; import { decryptText } from "../tools/createSecureValue"; import { WebClient } from "@slack/web-api"; import { ISlackWebhook } from "../domain.layer/interfaces/i.slackWebhook"; -let redisClient; -let notificationQueue: any; -const REDIS_URL = process.env.REDIS_URL || "redis://localhost:6379";
30-36: Prefer channel_id when posting messages.Slack recommends using channel IDs. Fallback to name only if ID missing.
- const channel = integration.channel; + const channel = (integration as any).channel_id || integration.channel; const msg = formatSlackMessage(message);
76-78: Use ISO timestamp for consistency.UTC ISO string is more consistent for logs and parsing.
- text: `📅 ${new Date().toLocaleString("en-US", { timeZone: "UTC" })} UTC`, + text: `📅 ${new Date().toISOString()}`,Clients/src/presentation/pages/SettingsPage/Slack/index.tsx (1)
55-56: Fix OAuth URL: remove empty user_scope and URL‑encode params.Avoid sending
user_scope=empty and ensure proper encoding forclient_id,scope, andredirect_uri.- const url = `${ENV_VARs.SLACK_URL}?client_id=${ENV_VARs.CLIENT_ID}&scope=${scopes}&user_scope=&redirect_uri=${window.location.origin}/setting/?activeTab=slack`; + const redirectUri = `${window.location.origin}/setting/?activeTab=slack`; + const url = `${ENV_VARs.SLACK_URL}?client_id=${encodeURIComponent( + ENV_VARs.CLIENT_ID, + )}&scope=${encodeURIComponent(scopes)}&redirect_uri=${encodeURIComponent( + redirectUri, + )}`;Clients/src/application/repository/slack.integration.repository.ts (1)
55-57: Standardize repository returns.Other methods return
response.data. AlignupdateSlackIntegrationfor consistency.- const response = await apiServices.patch(`/slackWebhooks/${id}`, body); - return response; + const response = await apiServices.patch(`/slackWebhooks/${id}`, body); + return response.data;Servers/utils/slackWebhook.utils.ts (2)
68-69: Fix return shape from INSERT.
sequelize.queryreturns[rows, metadata]. WithRETURNING *andmapToModel: true, use the first row.- return result[0]; + return (result[0] as SlackWebhookModel[])[0];
92-105: Handle empty updates.If no updatable fields are provided,
setClauseis empty and the SQL becomes invalid. Guard this case.const query = `UPDATE public.slack_webhooks SET ${setClause} WHERE id = :id RETURNING *;`; updateSlackWebhookData.id = id; - const result = await sequelize.query(query, { + if (!setClause) { + // No-op update: return current record + const existing = await SlackWebhookModel.findByPk(id, { transaction }); + return existing ?? null; + } + + const result = await sequelize.query(query, { replacements: updateSlackWebhookData, mapToModel: true, model: SlackWebhookModel, transaction, });Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx (1)
71-77: Handle non-success API responses.Consider an else path to notify failures when
msg.data.successis falsy.- if (msg.data.success) { + if (msg.data?.success) { showAlert( "success", "Success", "Test message sent successfully to the Slack channel.", ); + } else { + showAlert("error", "Error", "Slack API did not confirm message delivery."); }Servers/controllers/slackWebhook.ctrl.ts (4)
227-231: Remove non-existent title from logs.
slackWebhookDatafrom OAuth doesn't containtitle. Avoid logging undefined fields.- await logEvent( - "Create", - `slackWebhook created: ID ${newSlackWebhook.id}, title: ${slackWebhookData.title}`, - ); + await logEvent("Create", `slackWebhook created: ID ${newSlackWebhook.id}`);
241-244: Same here: avoid referencing undefined title in error log.- await logEvent( - "Error", - `slackWebhook creation failed: ${slackWebhookData.title}`, - ); + await logEvent("Error", "slackWebhook creation failed");
149-177: Propagate Slack OAuth error messages for better diagnostics.Don’t swallow the root cause.
} catch (error) { - throw new Error("Failed to validate Slack OAuth code"); + const msg = + (error as Error)?.message || "Failed to validate Slack OAuth code"; + throw new Error(`Failed to validate Slack OAuth code: ${msg}`); }
460-468: Return a response when Slack send doesn’t report success.Add a non-OK path to avoid hanging requests or false positives.
- if (slackMsgSent.success) { + if (slackMsgSent.success) { logStructured( "successful", `slackWebhook found: ID ${requestId}`, functionName, fileName, ); return res.status(200).json(STATUS_CODE[200](slackMsgSent)); - } + } else { + logger.warn("Slack send returned success=false", { id: requestId }); + return res + .status(502) + .json(STATUS_CODE[502]("Slack did not confirm message delivery")); + }Servers/domain.layer/models/slackNotification/slackWebhook.model.ts (1)
36-36: Type accuracy: use string for access_token.
anyhides errors and weakens guarantees.- access_token!: any; + access_token!: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Servers/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.env.dev(1 hunks).env.prod(1 hunks)Clients/env.vars.ts(1 hunks)Clients/src/application/constants/permissions.ts(1 hunks)Clients/src/application/hooks/useSlackIntegrations.tsx(1 hunks)Clients/src/application/repository/slack.integration.repository.ts(1 hunks)Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx(1 hunks)Clients/src/presentation/pages/SettingsPage/Slack/index.tsx(1 hunks)Clients/src/presentation/pages/SettingsPage/index.tsx(4 hunks)Servers/controllers/slackWebhook.ctrl.ts(1 hunks)Servers/database/db.ts(2 hunks)Servers/database/migrations/20250916121737-create-slack-webhook.js(1 hunks)Servers/domain.layer/interfaces/i.slackWebhook.ts(1 hunks)Servers/domain.layer/models/slackNotification/slackWebhook.model.ts(1 hunks)Servers/index.ts(4 hunks)Servers/package.json(2 hunks)Servers/routes/slackWebhook.route.ts(1 hunks)Servers/services/slackNotificationService.ts(1 hunks)Servers/tools/createSecureValue.ts(1 hunks)Servers/utils/slackWebhook.utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
Servers/routes/slackWebhook.route.ts (3)
Servers/controllers/slackWebhook.ctrl.ts (5)
getAllSlackWebhooks(24-80)getSlackWebhookById(82-147)createNewSlackWebhook(180-292)updateSlackWebhookById(294-436)sendSlackMessage(438-499)Servers/domain.layer/models/slackNotification/slackWebhook.model.ts (2)
createNewSlackWebhook(107-203)updateSlackWebhookById(471-487)Clients/src/application/repository/slack.integration.repository.ts (1)
sendSlackMessage(59-68)
Clients/src/presentation/pages/SettingsPage/Slack/index.tsx (5)
Clients/src/application/hooks/useAuth.ts (1)
useAuth(4-16)Clients/src/application/hooks/useSlackIntegrations.tsx (1)
SlackWebhook(20-29)Clients/env.vars.ts (1)
ENV_VARs(1-13)Clients/src/application/repository/slack.integration.repository.ts (1)
createSlackIntegration(39-46)Clients/src/presentation/pages/Home/1.0Home/style.ts (1)
vwhomeHeading(1-6)
Clients/src/application/repository/slack.integration.repository.ts (2)
Clients/src/infrastructure/api/networkServices.ts (1)
apiServices(72-218)Servers/controllers/slackWebhook.ctrl.ts (1)
sendSlackMessage(438-499)
Clients/src/application/hooks/useSlackIntegrations.tsx (2)
Servers/domain.layer/interfaces/i.slackWebhook.ts (1)
ISlackWebhook(1-17)Clients/src/application/repository/slack.integration.repository.ts (1)
getSlackIntegrations(3-21)
Servers/services/slackNotificationService.ts (2)
Servers/tools/createSecureValue.ts (1)
decryptText(88-88)Servers/domain.layer/interfaces/i.slackWebhook.ts (1)
ISlackWebhook(1-17)
Servers/controllers/slackWebhook.ctrl.ts (7)
Servers/utils/logger/fileLogger.ts (1)
logStructured(35-44)Servers/domain.layer/interfaces/i.slackWebhook.ts (1)
ISlackWebhook(1-17)Servers/utils/slackWebhook.utils.ts (4)
getSlackWebhookByIdAndChannelQuery(20-33)getAllSlackWebhooksQuery(6-18)createNewSlackWebhookQuery(35-69)updateSlackWebhookByIdQuery(71-116)Servers/domain.layer/exceptions/custom.exception.ts (3)
ValidationException(94-116)NotFoundException(121-143)BusinessLogicException(220-242)Servers/domain.layer/models/slackNotification/slackWebhook.model.ts (2)
createNewSlackWebhook(107-203)updateSlackWebhookById(471-487)Servers/utils/logger/dbLogger.ts (1)
logEvent(8-30)Servers/services/slackNotificationService.ts (1)
sendImmediateMessage(20-52)
Servers/utils/slackWebhook.utils.ts (2)
Servers/domain.layer/interfaces/i.slackWebhook.ts (1)
ISlackWebhook(1-17)Servers/database/db.ts (1)
sequelize(141-141)
Servers/domain.layer/models/slackNotification/slackWebhook.model.ts (3)
Servers/domain.layer/interfaces/i.slackWebhook.ts (1)
ISlackWebhook(1-17)Servers/domain.layer/exceptions/custom.exception.ts (2)
ValidationException(94-116)NotFoundException(121-143)Servers/domain.layer/validations/number.valid.ts (1)
numberValidation(10-36)
Servers/index.ts (1)
Servers/config/constants.js (2)
frontEndUrl(3-3)frontEndUrl(3-3)
Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx (5)
Clients/src/application/hooks/useSlackIntegrations.tsx (1)
SlackWebhook(20-29)Clients/src/application/repository/slack.integration.repository.ts (1)
sendSlackMessage(59-68)Clients/src/presentation/pages/Home/1.0Home/style.ts (1)
vwhomeHeading(1-6)Clients/src/presentation/tools/isoDateToString.ts (1)
formatDate(7-20)Clients/src/presentation/components/Cards/ProjectCard/style.ts (1)
viewProjectButtonStyle(42-48)
🪛 Biome (2.1.2)
Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx
[error] 28-29: Expected a parameter but instead found 'const'.
Expected a parameter here.
(parse)
[error] 29-29: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 29-29: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 32-32: Expected a statement but instead found ': SlackIntegrationsProps) =>'.
Expected a statement here.
(parse)
[error] 34-34: Expected an expression but instead found ')'.
Expected an expression here.
(parse)
🪛 GitHub Actions: Frontend Checks
Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx
[error] 29-29: TS1359: Identifier expected. 'const' is a reserved word that cannot be used here. Occurred during 'npm run build' (tsc -b).
🔇 Additional comments (6)
Servers/index.ts (1)
129-129: LGTM: Slack routes mountedMounting under /api/slackWebhooks is consistent with client repository calls.
Servers/database/db.ts (1)
65-65: Model registration LGTM — migration present; ensure it’s applied before useFound Servers/database/migrations/20250916121737-create-slack-webhook.js (creates "slack_webhooks", adds indexes, down drops table). Confirm this migration has been run in the target environment before using the model.
Clients/env.vars.ts (1)
11-12: Verify env vars and rename SLACK_URL to SLACK_AUTH_URL (not SLACK_REDIRECT_URI)
- Ensure VITE_CLIENT_ID and VITE_SLACK_URL are present in the client env and documented (Clients/env.vars.ts:11-12).
- SLACK_URL is the Slack OAuth authorize endpoint — rename to SLACK_AUTH_URL or SLACK_OAUTH_URL for clarity. Do NOT rename to SLACK_REDIRECT_URI; redirect_uri is built at runtime (Clients/src/presentation/pages/SettingsPage/Slack/index.tsx:55).
Likely an incorrect or invalid review comment.
Clients/src/presentation/pages/SettingsPage/index.tsx (1)
105-111: Slack tab integration looks good.Good addition of the Slack tab with role-based disabling and panel wiring.
Also applies to: 131-133
Clients/src/presentation/pages/SettingsPage/Slack/index.tsx (1)
55-56: Verify redirect_uri matches server config.Server uses
process.env.FRONTEND_URL/setting/?activeTab=slack. Ensure exact match (scheme/host/port/path) with the client’s URL to avoid OAuth failures.Servers/controllers/slackWebhook.ctrl.ts (1)
149-157: Confirm redirect_uri parity between client and server.Ensure
process.env.FRONTEND_URL/setting/?activeTab=slackexactly matches the client’s redirect URI.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx (7)
156-162: Sticky action cell needs a background and z-index.Prevents bleed-through when horizontally scrolling.
Apply this diff:
sx={{ ...singleTheme.tableStyles.primary.body.cell, textTransform: "none", position: "sticky", right: 0, + backgroundColor: theme.palette.background.paper, + zIndex: 1, }}
163-169: Add an accessible label to “Send Test” button.Improves SR usability and context per row.
Apply this diff:
<CustomizableButton variant="outlined" onClick={handleSlackTestClick(item.id)} size="small" text="Send Test" + aria-label={`Send Slack test to ${item.channel || item.teamName}`} sx={viewProjectButtonStyle} />
143-146: Guard against invalid dates and remove unnecessarytoString().
formatDatethrows on invalid input; provide a safe fallback.Apply this diff:
- {item.createdAt - ? formatDate(item.createdAt.toString()) - : ""} + {(() => { + try { + return item.createdAt ? formatDate(item.createdAt) : "-"; + } catch { + return "-"; + } + })()}
199-204: Avoid “Page 1 of 0” in empty state.Render a sensible label when count is 0.
Apply this diff:
- labelDisplayedRows={({ page, count }) => - `Page ${page + 1} of ${Math.max( - 0, - Math.ceil(count / rowsPerPage), - )}` - } + labelDisplayedRows={({ page, count }) => { + const totalPages = Math.ceil(count / rowsPerPage); + return count === 0 + ? "Page 0 of 0" + : `Page ${page + 1} of ${totalPages}`; + }}
195-197: PassActionsComponentdirectly (no wrapper).Removes an unnecessary render wrapper.
Apply this diff:
- ActionsComponent={(props) => ( - <TablePaginationActions {...props} /> - )} + ActionsComponent={TablePaginationActions}
1-1: Avoid importing MUI internals forTablePaginationActions.
@mui/material/TablePagination/TablePaginationActionsis internal and may break on upgrade. Prefer your own small actions component or rely on the default actions (and remove the prop).If you choose to remove the custom actions entirely, also delete this import.
-import TablePaginationActions from "@mui/material/TablePagination/TablePaginationActions";
119-122: Drop unnecessary optional chaining on array.map.
slice()always returns an array;?.mapis redundant.Apply this diff:
- ?.map((item) => ( + .map((item) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx (5)
Clients/src/application/hooks/useSlackIntegrations.tsx (1)
SlackWebhook(20-29)Clients/src/application/repository/slack.integration.repository.ts (1)
sendSlackMessage(59-68)Clients/src/presentation/pages/Home/1.0Home/style.ts (1)
vwhomeHeading(1-6)Clients/src/presentation/tools/isoDateToString.ts (1)
formatDate(7-20)Clients/src/presentation/components/Cards/ProjectCard/style.ts (1)
viewProjectButtonStyle(42-48)
🔇 Additional comments (2)
Clients/src/presentation/pages/SettingsPage/Slack/SlackIntegrations.tsx (2)
25-32: Props interface fixed — resolved the prior build blocker.The
showAlertsignature and interface closure are correct now.
34-37: Consolidated to a single component and includedshowAlert— looks good.The duplicate component issue is resolved; props align with the interface.
|
There's only a conflict here @rachanabasnet |
…into rb-sept-15-slack-integration
Thank You! I've resolved the conflict. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Clients/src/presentation/pages/SettingsPage/index.tsx (4)
39-42: Avoid mutatingsearchParamsin place; clone before delete.Prevents subtle issues due to shared mutable URLSearchParams.
- if (activeSetting) { - searchParams.delete("activeTab"); - setSearchParams(searchParams); - } + if (activeSetting) { + const next = new URLSearchParams(searchParams); + next.delete("activeTab"); + setSearchParams(next); + }
53-55: Remove redundant whitespace nodes around breadcrumbs.These literal spaces are unnecessary.
- {" "} - <PageBreadcrumbs />{" "} + <PageBreadcrumbs />
93-97: Use functional state update when toggling the Helper Drawer.Prevents stale-closure bugs.
- <HelperIcon - onClick={() => setIsHelperDrawerOpen(!isHelperDrawerOpen)} - size="small" - /> + <HelperIcon + onClick={() => setIsHelperDrawerOpen((open) => !open)} + size="small" + />
25-25: Harden Slack permission lookup with optional chainingClients/src/application/constants/permissions.ts defines slack.view; update the lookup to avoid future breakage.
- const isSlackTabDisabled = !allowedRoles.slack.view.includes(userRoleName); + const isSlackTabDisabled = !(allowedRoles.slack?.view?.includes(userRoleName) ?? false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Clients/src/presentation/pages/SettingsPage/index.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Clients/src/presentation/pages/SettingsPage/index.tsx (1)
Clients/src/presentation/pages/ProjectView/styles.ts (1)
tabContainerStyle(81-85)
🔇 Additional comments (1)
Clients/src/presentation/pages/SettingsPage/index.tsx (1)
89-98: LGTM on PageHeader and Slack tab wiring.Header composition and tab registration look consistent with existing patterns.
Also applies to: 131-137
MuhammadKhalilzadeh
left a comment
MuhammadKhalilzadeh
left a comment
There was a problem hiding this comment.
Thank you @rachanabasnet
2 participants
Slack Notification Integration
This PR includes
SlackIntegrations.webm
How to test Slack Integration in localhost (Slack doesn't allow http as its redirect URL, so the website must have https)
Clients/env.vars.tsFixes #1888