Conversation
dev-sweetr-dev
bot
added
the
medium
WalkthroughAdds a Docker Build & Publish GitHub Actions workflow; introduces GET /health; standardizes and adds environment variable defaults; adds runtime guards for Stripe and Slack, and a Stripe client guard; gates BullBoard, LogTail, and Sentry initialization on config; updates docs for LogTail. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/env.ts">
<violation number="1" location="apps/api/src/env.ts:38">
P1: Avoid defaulting webhook/API secrets to an empty string. This makes missing secrets silently pass validation and can run production with an empty signing key. Require these values (or limit empty defaults to dev via devDefault).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/bull-mq/bull-board.router.ts (1)
24-25:⚠️ Potential issue | 🟠 MajorPre-existing bug:
windowMs: 60is 60 milliseconds, not 15 minutes.The comment says "15 minutes" but the value
60is 60 ms. The rate limiter effectively allows 200 requests per 60 ms — i.e., no real throttling at all.🐛 Proposed fix
- windowMs: 60, // 15 minutes + windowMs: 15 * 60 * 1000, // 15 minutes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/bull-mq/bull-board.router.ts` around lines 24 - 25, The rateLimiter configuration in the rateLimit(...) call incorrectly sets windowMs to 60 (milliseconds) while the comment says 15 minutes; update windowMs in the rateLimit call used in bull-board.router.ts (the rateLimit(...) invocation) to 15 minutes (900000 ms or use 15 * 60 * 1000) and adjust the inline comment to match so the limiter enforces 15 minutes correctly.apps/api/src/express.ts (1)
14-26:⚠️ Potential issue | 🟠 MajorRemoving
trust proxybreaks per-client rate limiting when the container sits behind a reverse proxy.If the app is behind a proxy/load balancer (common with Nginx, Traefik, Cloudflare, etc.), the IP address seen by Express will be the load balancer's IP, making the rate limiter effectively a global one and potentially blocking all requests once the limit is reached. Since this image is intended for self-hosted Docker deployments, users are likely to deploy it behind a reverse proxy.
Restoring the setting (or making it configurable via env var) is safer:
🛡️ Proposed fix
export const expressApp = express(); + +// Required when running behind a reverse proxy (Nginx, Traefik, etc.) +expressApp.set("trust proxy", 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/express.ts` around lines 14 - 26, The app removes Express's proxy trust which causes per-client rate limiting to see the proxy IP; update expressApp to restore trust by calling expressApp.set('trust proxy', true) or make it configurable via an env var (e.g., process.env.TRUST_PROXY) and apply that value (true/false or numeric) before middlewares are mounted so rate limiting and req.ip reflect the real client IP; locate where expressApp is declared and configured (expressApp variable) and add the set call early in the setup.
🧹 Nitpick comments (1)
.github/workflows/docker-publish.yml (1)
51-60: The validated image is discarded;publishrebuilds independently.Both
validateandpublishbuild from the Dockerfile. The GHA layer cache mitigates the cost, but the published artifact is not byte-for-byte proven to be the validated one. A common alternative is to push the test image to a staging registry (or usedocker buildx imagetools) so thepublishjob retags and pushes what was already validated, rather than rebuilding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-publish.yml around lines 51 - 60, The current "Build image" step uses docker/build-push-action@v6 with push: false and tags the image as ${{ env.IMAGE }}:test, so the validated image is never pushed and the later publish job rebuilds; change the build step to push the validated artifact to a staging registry (set push: true and tag as a stable staging tag like :validated or :staging) or use docker buildx imagetools to create an index and push it, then update the publish job to retag/pull that staged image and push it to the final registry rather than rebuilding; locate the "Build image" step, its uses entry, and the tag ${ { env.IMAGE } }:test plus the publish job that currently rebuilds, and make the push-and-retag flow so publish operates on the exact validated image.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/env.ts`:
- Around line 71-75: The LOG_DRAIN env var's description is incorrect (it
currently repeats the GitHub webhook secret text); update the desc for the
LOG_DRAIN str() entry to a correct, concise description such as "Where to send
logs (e.g., 'logtail' or 'console')" so it matches the choices and default in
the LOG_DRAIN definition and ensure the change is made in the LOG_DRAIN constant
in env.ts.
In `@apps/api/src/express.ts`:
- Line 26: The CORS config using use(cors({ origin: env.FRONTEND_URL,
credentials: true, methods: ["*"] })) must be changed because the cors package
and credentialed requests do not accept "*" for methods; replace the methods
array with an explicit list: ["GET", "HEAD", "PUT", "PATCH", "POST", "DELETE",
"OPTIONS"] so preflight checks succeed for credentialed requests while keeping
origin: env.FRONTEND_URL and credentials: true unchanged.
---
Outside diff comments:
In `@apps/api/src/bull-mq/bull-board.router.ts`:
- Around line 24-25: The rateLimiter configuration in the rateLimit(...) call
incorrectly sets windowMs to 60 (milliseconds) while the comment says 15
minutes; update windowMs in the rateLimit call used in bull-board.router.ts (the
rateLimit(...) invocation) to 15 minutes (900000 ms or use 15 * 60 * 1000) and
adjust the inline comment to match so the limiter enforces 15 minutes correctly.
In `@apps/api/src/express.ts`:
- Around line 14-26: The app removes Express's proxy trust which causes
per-client rate limiting to see the proxy IP; update expressApp to restore trust
by calling expressApp.set('trust proxy', true) or make it configurable via an
env var (e.g., process.env.TRUST_PROXY) and apply that value (true/false or
numeric) before middlewares are mounted so rate limiting and req.ip reflect the
real client IP; locate where expressApp is declared and configured (expressApp
variable) and add the set call early in the setup.
---
Nitpick comments:
In @.github/workflows/docker-publish.yml:
- Around line 51-60: The current "Build image" step uses
docker/build-push-action@v6 with push: false and tags the image as ${{ env.IMAGE
}}:test, so the validated image is never pushed and the later publish job
rebuilds; change the build step to push the validated artifact to a staging
registry (set push: true and tag as a stable staging tag like :validated or
:staging) or use docker buildx imagetools to create an index and push it, then
update the publish job to retag/pull that staged image and push it to the final
registry rather than rebuilding; locate the "Build image" step, its uses entry,
and the tag ${ { env.IMAGE } }:test plus the publish job that currently
rebuilds, and make the push-and-retag flow so publish operates on the exact
validated image.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/env.ts`:
- Around line 41-64: Add presence checks for credentials before creating SDK
instances: in getStripeClient() verify env.STRIPE_API_KEY is non-empty and
return null/undefined (or throw a clear, documented error) instead of
instantiating Stripe with an empty key, and update callers to guard against a
null client; in getInstallUrl() verify env.SLACK_CLIENT_ID (the clientId used by
getInstallUrl()/getIntegration()) is non-empty before building the OAuth URL and
return a disabled/no-op response when missing. Follow the same guard pattern
used in sentry.ts, logger.ts, and bull-board.router.ts so integrations are
gracefully disabled when STRIPE_API_KEY or SLACK_CLIENT_ID are empty.
| STRIPE_API_KEY: str({ | ||
| desc: "The secret API Key for Stripe access", | ||
| allowEmpty: true, | ||
| devDefault: "", | ||
| default: "", | ||
| }), | ||
| STRIPE_WEBHOOK_SECRET: str({ | ||
| desc: "The secret string used to sign Stripe webhooks", | ||
| allowEmpty: true, | ||
| devDefault: "", | ||
| default: "", | ||
| }), | ||
| SLACK_CLIENT_ID: str({ | ||
| desc: "The public Slack Client ID", | ||
| allowEmpty: true, | ||
| devDefault: "", | ||
| default: "", | ||
| }), | ||
| SLACK_CLIENT_SECRET: str({ | ||
| desc: "The secret API Key for Slack access", | ||
| allowEmpty: true, | ||
| devDefault: "", | ||
| default: "", | ||
| }), | ||
| SLACK_WEBHOOK_SECRET: str({ | ||
| desc: "The secret string used to sign Slack webhooks", | ||
| allowEmpty: true, | ||
| devDefault: "", | ||
| default: "", | ||
| }), | ||
| SLACK_INSTALL_NOTIFICATION_WEBHOOK_URL: str({ | ||
| desc: "The URL to the Slack install notification webhook", | ||
| allowEmpty: true, | ||
| devDefault: "", | ||
| default: "", | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Stripe client/SDK initialization
rg -nP --type=ts "stripe|STRIPE" --max-count=200 | head -100Repository: sweetr-dev/sweetr.dev
Length of output: 3276
🏁 Script executed:
# Search for Slack client/SDK initialization
rg -nP --type=ts "slack|SLACK" --max-count=200 | head -100Repository: sweetr-dev/sweetr.dev
Length of output: 11198
🏁 Script executed:
cat -n apps/api/src/lib/stripe.tsRepository: sweetr-dev/sweetr.dev
Length of output: 407
🏁 Script executed:
cat -n apps/api/src/app/integrations/slack/services/slack-integration.service.ts | head -120Repository: sweetr-dev/sweetr.dev
Length of output: 4003
🏁 Script executed:
cat -n apps/api/src/config/slack.config.tsRepository: sweetr-dev/sweetr.dev
Length of output: 542
🏁 Script executed:
cat -n apps/api/src/app/billing/stripe.router.tsRepository: sweetr-dev/sweetr.dev
Length of output: 2179
🏁 Script executed:
# Check if stripe client is instantiated conditionally anywhere
rg -n "getStripeClient" --type=ts -B 3 -A 3Repository: sweetr-dev/sweetr.dev
Length of output: 6037
🏁 Script executed:
# Find getSlackClient definition
rg -n "export.*getSlackClient" --type=ts -A 10 apps/api/src/app/integrations/slack/services/slack-client.service.tsRepository: sweetr-dev/sweetr.dev
Length of output: 459
🏁 Script executed:
# Check where getInstallUrl is called and if it has guards
rg -n "getInstallUrl" --type=ts -B 5 -A 5Repository: sweetr-dev/sweetr.dev
Length of output: 3428
🏁 Script executed:
# Check if Stripe is gated anywhere or if billing is feature-flagged
rg -n "STRIPE_API_KEY|getStripeClient" --type=ts -B 5 -A 5 | grep -E "(if|guard|feature|enabled|check)" -B 3 -A 3Repository: sweetr-dev/sweetr.dev
Length of output: 1896
Add guards to Stripe and Slack initialization code before instantiating SDKs with potentially empty credentials.
Stripe's getStripeClient() currently instantiates the SDK unconditionally (apps/api/src/lib/stripe.ts:9), and all routes call it without checking if STRIPE_API_KEY is configured. Similarly, Slack's getInstallUrl() (slack-integration.service.ts:118) builds an OAuth URL without verifying clientId is non-empty, even though getIntegration() has such a guard. With both integrations now defaulting to empty strings in env.ts, these SDKs will fail at runtime with cryptic errors instead of gracefully disabling. Add checks like the ones in sentry.ts, logger.ts, and bull-board.router.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/env.ts` around lines 41 - 64, Add presence checks for
credentials before creating SDK instances: in getStripeClient() verify
env.STRIPE_API_KEY is non-empty and return null/undefined (or throw a clear,
documented error) instead of instantiating Stripe with an empty key, and update
callers to guard against a null client; in getInstallUrl() verify
env.SLACK_CLIENT_ID (the clientId used by getInstallUrl()/getIntegration()) is
non-empty before building the OAuth URL and return a disabled/no-op response
when missing. Follow the same guard pattern used in sentry.ts, logger.ts, and
bull-board.router.ts so integrations are gracefully disabled when STRIPE_API_KEY
or SLACK_CLIENT_ID are empty.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| publish: | ||
| needs: validate | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: Login to Docker Hub | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| username: ${{ secrets.DOCKERHUB_USERNAME }} | ||
| password: ${{ secrets.DOCKERHUB_TOKEN }} | ||
|
|
||
| - name: Build and push | ||
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: . | ||
| file: ./apps/api/Dockerfile | ||
| push: true | ||
| tags: | | ||
| ${{ env.IMAGE }}:${{ inputs.version }} | ||
| ${{ env.IMAGE }}:latest | ||
| cache-from: type=gha | ||
| cache-to: type=gha,mode=max |
There was a problem hiding this comment.
Publish job rebuilds instead of reusing validated image. The validate job builds and tests the image, but the publish job builds a completely new image from source. Even with GHA cache, a cache miss or non-deterministic build step could mean the published image differs from the one validated. Consider saving the validated image as an artifact (e.g., via docker save / docker load or a shared registry tag) and pushing that instead.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/api/src/app/integrations/slack/services/slack-integration.service.ts (1)
119-121: PreferIntegrationExceptionover bareErrorfor consistency.Every other thrown error in this file uses
IntegrationException(lines 26 and 74). Throwing a rawErrorhere means any middleware or GraphQL error handler that specifically catches/formatsIntegrationExceptioninstances will treat this differently — potentially leaking a raw stack trace or bypassing structured error formatting.♻️ Proposed fix
- if (!config.slack.clientId) { - throw new Error("SLACK_CLIENT_ID is not set"); - } + if (!config.slack.clientId) { + throw new IntegrationException("Slack is not configured for this instance.", { + extra: { reason: "SLACK_CLIENT_ID is not set" }, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/integrations/slack/services/slack-integration.service.ts` around lines 119 - 121, Replace the raw Error thrown when config.slack.clientId is missing with the project's IntegrationException so error handling stays consistent; locate the check that references config.slack.clientId in slack-integration.service.ts and throw new IntegrationException with a clear message (matching other uses at lines where IntegrationException is used) instead of new Error, ensuring imports include IntegrationException if not already present.apps/api/src/app/billing/stripe.router.ts (1)
51-53: Redundant guard and duplicated error message.Two minor issues:
The
STRIPE_API_KEYcheck here is redundant. Unlike the webhook handler, the checkout handler has no innertry/catch, sogetStripeClient()(called transitively viacreateStripeCheckoutSessionon line 69) would already throw and propagate cleanly throughcatchErrorswithout being swallowed.The error message
"STRIPE_API_KEY is not set"now lives in three places:stripe.tsline 8,stripe.router.tsline 20, and here on line 52. If the message or logic changes, all three must stay in sync.Consider removing lines 51–53 and relying on
getStripeClient()'s own guard, which is sufficient for routes without an innertry/catch.♻️ Suggested cleanup
catchErrors(async (req, res) => { - if (!env.STRIPE_API_KEY) { - throw new Error("STRIPE_API_KEY is not set"); - } - const schema = z.object({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/billing/stripe.router.ts` around lines 51 - 53, Remove the redundant STRIPE_API_KEY guard in the checkout route: delete the if-check that throws "STRIPE_API_KEY is not set" and rely on getStripeClient() (used transitively by createStripeCheckoutSession) to perform the environment check and throw; this avoids duplicating the error message across stripe.ts, stripe.router.ts, and the checkout handler and keeps error handling centralized so catchErrors will propagate the original error from getStripeClient().apps/api/src/lib/stripe.ts (1)
7-11: Move the cache-hit fast path before the guard.The
STRIPE_API_KEYguard (lines 7–9) fires on every call, including all the cache-hit returns on line 11. Since env vars are stable for the process lifetime, the check is redundant once the client has already been constructed. Reordering puts the cheapest exit first:♻️ Suggested reorder
export const getStripeClient = (): Stripe => { + if (stripeClient) return stripeClient; + if (!env.STRIPE_API_KEY) { throw new Error("STRIPE_API_KEY is not set"); } - if (stripeClient) return stripeClient; - stripeClient = new Stripe(env.STRIPE_API_KEY); return stripeClient; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/stripe.ts` around lines 7 - 11, The current guard throws if env.STRIPE_API_KEY is missing before checking the cached stripeClient, which forces the env check on every call; move the cache-hit fast path so the `if (stripeClient) return stripeClient;` executes before inspecting `env.STRIPE_API_KEY`. Update the initialization flow in the factory/initializer function that references `stripeClient` and `env.STRIPE_API_KEY` so you only validate the env var when you actually need to create a new client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/app/billing/stripe.router.ts`:
- Around line 51-53: Remove the redundant STRIPE_API_KEY guard in the checkout
route: delete the if-check that throws "STRIPE_API_KEY is not set" and rely on
getStripeClient() (used transitively by createStripeCheckoutSession) to perform
the environment check and throw; this avoids duplicating the error message
across stripe.ts, stripe.router.ts, and the checkout handler and keeps error
handling centralized so catchErrors will propagate the original error from
getStripeClient().
In `@apps/api/src/app/integrations/slack/services/slack-integration.service.ts`:
- Around line 119-121: Replace the raw Error thrown when config.slack.clientId
is missing with the project's IntegrationException so error handling stays
consistent; locate the check that references config.slack.clientId in
slack-integration.service.ts and throw new IntegrationException with a clear
message (matching other uses at lines where IntegrationException is used)
instead of new Error, ensuring imports include IntegrationException if not
already present.
In `@apps/api/src/lib/stripe.ts`:
- Around line 7-11: The current guard throws if env.STRIPE_API_KEY is missing
before checking the cached stripeClient, which forces the env check on every
call; move the cache-hit fast path so the `if (stripeClient) return
stripeClient;` executes before inspecting `env.STRIPE_API_KEY`. Update the
initialization flow in the factory/initializer function that references
`stripeClient` and `env.STRIPE_API_KEY` so you only validate the env var when
you actually need to create a new client.
2 participants
Greptile Summary
This PR adds Docker publishing workflow and improves self-hosted deployment support by making optional integrations truly optional. The changes transform environment variable validation from
allowEmpty: truetodefault: "", allowing the API to start without Stripe, Slack, Sentry, or LogTail credentials. The workflow validates builds via health checks before publishing to Docker Hub.Key changes:
default: ""instead ofallowEmpty: true, enabling graceful degradation!isAppSelfHosted())Confidence Score: 4/5
APP_MODEenv var (already flagged in previous comments), which could cause the container to default to an unexpected mode during testing..github/workflows/docker-publish.ymlneedsAPP_MODE=self-hostedadded to validation stepImportant Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Docker Workflow Triggered] --> B[Validate Job] B --> C[Build Docker Image] C --> D[Start Container with Test Env Vars] D --> E[Health Check: /health endpoint] E -->|Success| F[Publish Job] E -->|Failure| G[Show Logs & Fail] F --> H[Build & Push to Docker Hub] H --> I[Tag: version + latest] J[App Startup] --> K{APP_MODE check} K -->|self-hosted| L[Skip Stripe Router] K -->|saas| M[Load Stripe Router] L --> N[Load Health, GitHub, Slack, BullBoard] M --> N N --> O[Optional Services Check] O -->|SENTRY_DSN present| P[Init Sentry] O -->|LOG_DRAIN=logtail & token| Q[Init LogTail] O -->|BULLBOARD creds present| R[Init BullBoard]Last reviewed commit: 1c00e5a