Post new sign up reasons to slack, not plain #1995
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe environment schema was updated to make the Slack-related environment variables Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant SlackService
participant SlackAPI
User->>WebApp: Submit new organization form
WebApp->>SlackService: sendNewOrgMessage({orgName, whyUseUs, userEmail})
SlackService->>SlackAPI: Post formatted message to channel
SlackAPI-->>SlackService: Response (success/failure)
SlackService-->>WebApp: (void)
WebApp->>User: Redirect/response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/services/slack.server.ts (1)
13-40: Well-formatted Slack message with proper error handlingThe function constructs a well-structured Slack message using Block Kit format, which aligns with Slack's best practices for rich message formatting. Error handling is properly implemented to ensure failures don't propagate to the calling code.
Two minor suggestions:
- Consider returning a boolean or result object to indicate success/failure to the caller:
export async function sendNewOrgMessage({ orgName, whyUseUs, userEmail }: SendNewOrgMessageParams) { try { await slack.chat.postMessage({ // ... existing code ... }); + return true; } catch (error) { logger.error("Error sending data to Slack when creating an org:", { error }); + return false; } }
- Consider adding more structured error information in the log:
} catch (error) { - logger.error("Error sending data to Slack when creating an org:", { error }); + logger.error("Error sending data to Slack when creating an org:", { + error, + orgName, + userEmail + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.new/route.tsx(2 hunks)apps/webapp/app/services/slack.server.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/webapp/app/services/slack.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(723-723)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/webapp/app/env.server.ts (1)
718-719: Addition of required Slack environment variablesTwo new required environment variables are added for the Slack integration. This is the right approach for setting up external service credentials, ensuring they are properly validated at application startup.
Make sure to update your deployment environment(s) with these new variables, as the application will fail to start without them.
apps/webapp/app/routes/_app.orgs.new/route.tsx (2)
26-26: Clean import of new Slack serviceGood replacement of the previous notification system import with the new Slack service.
64-69: Improved notification handlingThe code has been simplified by delegating message construction and error handling to the Slack service. This makes the route handler cleaner and more focused on its primary responsibility.
The previous try-catch block for notification errors has been removed, as error handling is now managed within the Slack service. This is a good separation of concerns.
apps/webapp/app/services/slack.server.ts (2)
1-6: Well-structured service initializationThe Slack WebClient is properly initialized with the bot token from environment variables. This follows the recommended pattern for the Slack SDK.
7-12: Clear type definition for message parametersGood use of TypeScript to define the expected parameters for the message function. This provides clear documentation and type safety.
apps/webapp/app/env.server.ts
Outdated
| SLACK_BOT_TOKEN: z.string(), | ||
| SLACK_SIGNUP_REASON_CHANNEL_ID: z.string(), |
There was a problem hiding this comment.
@samejr these need to be optional because of self-hosters
|
When a new org signs up and completes the "What problem are you trying to solve?" the response is posted to Slack instead of Plain.
It looks like this:
Summary by CodeRabbit
New Features
Chores