Add self-hosted env & docker updates; Convex migration scanner/report; improve webhook delivery and tests#52
Merged
weroperking merged 3 commits intomainfrom Mar 30, 2026
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThe PR establishes a self-hosted deployment template, switches Inngest from Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant Inngest
participant Database
participant HTTP["HTTP Endpoint"]
Note over Client,HTTP: Old Flow (attempt-based)
Client->>Server: Trigger webhook event
Server->>Inngest: send("betterbase/webhook.deliver", {attempt: 1})
Inngest->>HTTP: POST delivery (attempt=1)
alt HTTP succeeds
HTTP-->>Inngest: 200 OK
Inngest->>Database: INSERT row with attempt=1
else HTTP fails
HTTP-->>Inngest: Error
Inngest->>Inngest: Retry with attempt=2
Inngest->>HTTP: POST delivery (attempt=2)
end
Note over Client,HTTP: New Flow (payload hash + advisory lock)
Client->>Server: Trigger webhook event
Server->>Inngest: send("betterbase/webhook.deliver", {eventType, payload})
Inngest->>HTTP: POST delivery
alt HTTP succeeds
HTTP-->>Inngest: 200 OK + response
Inngest->>Database: pg_advisory_xact_lock(hash)
Database->>Database: COALESCE(MAX(attempt_count),0)+1
Database-->>Inngest: INSERT with status=success
else HTTP fails
HTTP-->>Inngest: Error
Inngest->>Database: pg_advisory_xact_lock(hash)
Database->>Database: COALESCE(MAX(attempt_count),0)+1
Database-->>Inngest: INSERT with status=failed
Inngest->>Inngest: Retry (Inngest handles backoff)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/docker-setup.md (1)
42-63:⚠️ Potential issue | 🟠 MajorSelf-hosted bootstrap/env guidance is inconsistent and can misconfigure deployments
Line 47 still points to
.env.exampleinstead of.env.self-hosted.example, and the “Required Environment Variables” block omitsBETTERBASE_PUBLIC_URLandCORS_ORIGINS. This conflicts with the self-hosted contract and can produce incorrect external URL/CORS behavior.Proposed doc fix
# 2. Configure environment -cp .env.example .env +cp .env.self-hosted.example .env # Edit .env with your values @@ # Generate a secure auth secret BETTERBASE_JWT_SECRET=$(openssl rand -base64 32) +BETTERBASE_PUBLIC_URL=https://your-domain.com +CORS_ORIGINS=https://your-frontend.com # Generate Inngest keys (optional for development) INNGEST_SIGNING_KEY=$(openssl rand -hex 32) INNGEST_EVENT_KEY=$(openssl rand -hex 16)As per coding guidelines, “Self-hosted deployment docs must use
cp .env.self-hosted.example .env… Ensure required envs match the new names … Verify CORS viaCORS_ORIGINSand useBETTERBASE_PUBLIC_URLfor external URL references.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docker-setup.md` around lines 42 - 63, Update the self-hosted setup step to copy from .env.self-hosted.example instead of .env.example and update the "Required Environment Variables" block to include BETTERBASE_PUBLIC_URL and CORS_ORIGINS so the docs match the self-hosted contract; specifically change the cp command shown in the shell snippet to use .env.self-hosted.example and add entries describing BETTERBASE_PUBLIC_URL (external base URL) and CORS_ORIGINS (comma-separated allowed origins) alongside the existing BETTERBASE_JWT_SECRET and INNGEST keys so external URL/CORS behavior is correctly configured.packages/server/src/lib/inngest.ts (1)
268-273:⚠️ Potential issue | 🟡 Minor
deliveryResultaccessed without null guard despite| undefinedtype.At line 157-163,
deliveryResultis typed as{ ... } | undefined. Although control flow ensures it's defined here (we rethrow in the catch), TypeScript's narrowing may not recognize this.🛡️ Proposed fix
return { success: true, webhookId, - httpStatus: deliveryResult.httpStatus, - durationMs: deliveryResult.durationMs, + httpStatus: deliveryResult!.httpStatus, + durationMs: deliveryResult!.durationMs, };Or initialize
deliveryResultto a sentinel and remove| undefinedfrom the type if you prefer avoiding!.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/lib/inngest.ts` around lines 268 - 273, The return block accesses deliveryResult which is typed as possibly undefined; update the code around deliveryResult (the variable named deliveryResult and the function that builds/returns the object) to guarantee non-null before use—either add a null-check (if (!deliveryResult) throw or return an error) or initialize deliveryResult to a sentinel default so its type is not undefined (or use a single non-null assertion in the return if you prefer, e.g. deliveryResult!), ensuring the properties deliveryResult.httpStatus and deliveryResult.durationMs are only accessed when deliveryResult is proven defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BetterBase_Competitive_Plan.md`:
- Around line 78-92: MD022 violations occur because each week heading (e.g.,
"### Week 1", "### Week 2", "### Week 3", "### Week 4") is immediately followed
by a list without a blank line; update the markdown so there is one blank line
after each "### Week X" heading before its list to satisfy MD022 (ensure the
headings "### Week 1"/"### Week 2"/"### Week 3"/"### Week 4" are followed by an
empty line).
In `@packages/cli/src/commands/migrate/from-convex.ts`:
- Around line 48-50: The input-path validation currently logs and returns on
failure (the existsSync/statSync branch that calls logger.error and returns),
which yields a zero exit status; change this to propagate a failure by throwing
an Error (or otherwise returning a rejected promise) with a clear message so the
CLI exits non‑zero. Locate the validation in the from-convex command handler
(the block using existsSync and statSync and calling logger.error) and replace
the plain return with throw new Error(`Input path is not a directory:
${inputPath}`) (or reject) so scripts/CI receive a failing status.
In `@packages/core/test/branching.test.ts`:
- Around line 400-414: Tests incorrectly assert that listPreviewDatabases,
previewDatabaseExists, and teardownPreviewDatabase should reject; these are
success-path APIs and should be asserted to resolve deterministically. Update
the three tests referencing dbBranching.listPreviewDatabases,
dbBranching.previewDatabaseExists, and dbBranching.teardownPreviewDatabase to
assert resolution (for example use await expect(...).resolves.toBeDefined() or
await expect(...).resolves.toBeTruthy() or simply await the call and assert
expected return) instead of using rejects.toThrow so tests don't fail when
Postgres is reachable.
In `@packages/server/src/lib/inngest.ts`:
- Around line 230-241: There is a TOCTOU race between getNextDeliveryAttempt and
the subsequent INSERT: replace the separate read-then-write with an atomic
operation by acquiring a per-webhook/event/payload advisory lock
(pg_advisory_xact_lock) or performing the attempt_count computation and insert
in a single DB transaction; specifically, in the block inside
step.run("log-failed-delivery") remove the call to getNextDeliveryAttempt and
instead run one transaction that first issues
pg_advisory_xact_lock(hashtext(concat(webhookId,eventType,JSON.stringify(payload))))
and then INSERT INTO betterbase_meta.webhook_deliveries (...) SELECT
$1,$2,$3,'failed',$4,NOW(),COALESCE(MAX(attempt_count),0)+1 FROM
betterbase_meta.webhook_deliveries WHERE webhook_id=$1 AND event_type=$2 AND
payload=$3, using the same pool.query call (and the same advisory-lock+insert
pattern for the success path referenced around step.run for the success case).
Ensure identifiers: step.run("log-failed-delivery"), getNextDeliveryAttempt, and
the INSERT into betterbase_meta.webhook_deliveries are updated accordingly.
- Around line 129-137: The Events type and dispatcher payload include an unused
attempt field; remove this dead field instead of recalculating it. Update the
Events type to drop attempt, remove any attempt: 1 from the dispatcher payload,
and remove attempt from the destructuring in the webhook handler (the block that
currently extracts webhookId, webhookName, url, secret, eventType, tableName,
payload). Also search for other uses of attempt in this module and related types
and tests and delete them so getNextDeliveryAttempt() remains the single source
of truth for attemptCount.
- Around line 28-44: The query in getNextDeliveryAttempt is causing full scans
because only (webhook_id, created_at DESC) is indexed; add a composite index
that covers (webhook_id, event_type, payload) or (better) add a payload_hash
(SHA-256 of the canonical JSON string) column, index (webhook_id, event_type,
payload_hash) and change queries to filter on payload_hash (and verify payload
equality when needed) to avoid large JSONB indexes. To prevent TOCTOU on
attempt_count in getNextDeliveryAttempt, perform the read-and-insert inside a
transaction with proper serialization (use SERIALIZABLE isolation or a SELECT
... FOR UPDATE on a per-webhook/event/payload lock row or use a unique
constraint on (webhook_id,event_type,payload_hash,attempt_count) and an
UPSERT/retry loop) so concurrent callers cannot obtain the same next_attempt;
update getNextDeliveryAttempt (and the caller that inserts deliveries) to use
the chosen approach.
---
Outside diff comments:
In `@docs/docker-setup.md`:
- Around line 42-63: Update the self-hosted setup step to copy from
.env.self-hosted.example instead of .env.example and update the "Required
Environment Variables" block to include BETTERBASE_PUBLIC_URL and CORS_ORIGINS
so the docs match the self-hosted contract; specifically change the cp command
shown in the shell snippet to use .env.self-hosted.example and add entries
describing BETTERBASE_PUBLIC_URL (external base URL) and CORS_ORIGINS
(comma-separated allowed origins) alongside the existing BETTERBASE_JWT_SECRET
and INNGEST keys so external URL/CORS behavior is correctly configured.
In `@packages/server/src/lib/inngest.ts`:
- Around line 268-273: The return block accesses deliveryResult which is typed
as possibly undefined; update the code around deliveryResult (the variable named
deliveryResult and the function that builds/returns the object) to guarantee
non-null before use—either add a null-check (if (!deliveryResult) throw or
return an error) or initialize deliveryResult to a sentinel default so its type
is not undefined (or use a single non-null assertion in the return if you
prefer, e.g. deliveryResult!), ensuring the properties deliveryResult.httpStatus
and deliveryResult.durationMs are only accessed when deliveryResult is proven
defined.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c44fa2f-7dd3-4324-85cd-e7d11939b869
📒 Files selected for processing (13)
.env.self-hosted.example.gitignoreBetterBase_Competitive_Plan.mddocker-compose.production.ymldocker-compose.self-hosted.ymldocs/docker-setup.mddocs/guides/deployment.mddocs/guides/production-checklist.mdpackages/cli/src/commands/migrate/from-convex.tspackages/cli/test/migrate-from-convex.test.tspackages/client/test/auth.test.tspackages/core/test/branching.test.tspackages/server/src/lib/inngest.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description
.env.self-hosted.exampleand update.gitignoreto allow it to be checked into the repo.docker-compose.production.ymlanddocker-compose.self-hosted.ymlto pin MinIO images, changeinngestto run in production mode (inngest start), add healthchecks for Inngest, and require/rename server env vars toBETTERBASE_JWT_SECRET,BETTERBASE_PUBLIC_URL, andCORS_ORIGINS.docs/*) and deployment guides to reference the new env names and the Inngest production command.bb migrate from-convexcommand: add file/directory safety checks, convert schema and functions, scan for compatibility issues (blockers/warnings), and emit JSON/Markdown migration reports; include a compatibility ruleset and file-level conversion summaries.packages/cli/test/migrate-from-convex.test.ts) and small test fixes/adjustments in other test files.packages/server/src/lib/inngest.tsby addinggetNextDeliveryAttempt, restructuring HTTP send into a retried step, logging failed and successful deliveries intobetterbase_meta.webhook_deliverieswith attempt counts, and better timeout/error handling.Testing
bun testtargetingpackages/cli, and the newmigrate-from-convextests passed.packages/coreandpackages/client) after small test updates, and tests passed locally.Codex Task
Summary by CodeRabbit
New Features
Documentation
Chores