Merged
Conversation
…g the .github/workflow github actions , fixing some cli commands issues , and adding a test for client.test.ts , fixing the user.ts -> base templates src routes
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughEstablishes comprehensive test infrastructure and CI/CD pipeline for the monorepo. Adds GitHub Actions workflow for parallel test, lint, and typecheck jobs; extends workspace to include templates; adds test scripts to packages and templates; creates test suites for CLI, client SDK, core, and shared modules; removes obsolete documentation; exports internal migration utilities; and improves CLI robustness for auth setup and schema scanning. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/migrate.ts (1)
316-320:⚠️ Potential issue | 🟠 MajorLine comment termination condition is unreachable.
Line 318 checks
ch === "", which never occurs in this loop. After--, parser stays in line-comment mode and may skip splitting subsequent statements.Suggested fix
if (inLineComment) { current += ch; - if (ch === "") { + if (ch === "\n" || ch === "\r") { inLineComment = false; } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/migrate.ts` around lines 316 - 320, The termination check inside the line-comment handling is wrong: change the unreachable check for ch === "" to detect a newline character (e.g., ch === '\n' or include '\r' as needed) so the parser exits line-comment mode at end-of-line; update the block around inLineComment, current, and ch to set inLineComment = false when a newline is seen and handle appending or flushing current appropriately so subsequent statements are parsed.
🧹 Nitpick comments (8)
templates/base/test/health.test.ts (2)
1-1: Remove unused importafterAll.The
afterAllhook is imported but never used in this test file.♻️ Proposed fix
-import { describe, expect, test, beforeAll, afterAll } from "bun:test"; +import { describe, expect, test, beforeAll } from "bun:test";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/base/test/health.test.ts` at line 1, The import list in the test file includes an unused symbol `afterAll`; remove `afterAll` from the import statement (keep only `describe, expect, test, beforeAll`) so the file no longer imports an unused hook and lints cleanly; update the import line that currently references `afterAll` accordingly.
13-21: Test assumes live database connectivity.This test expects
data.databaseto be"connected", implying a real database connection must succeed during test execution. Consider:
- Mocking the database layer for isolated unit tests
- Adding test setup documentation for required environment variables
- Marking this as an integration test that runs separately
If this is intentionally an integration test requiring a real database, consider adding a comment to clarify that expectation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/base/test/health.test.ts` around lines 13 - 21, The test "GET /health returns 200 with healthy status" (in health.test.ts) assumes a live DB by asserting data.database === "connected"; either decouple the unit test from external DB or document the integration requirement: update the test to mock the database health-check (replace the real DB call used by app.request("/health") with a stubbed/mocked service that returns "connected") or mark the test as an integration test and add setup docs/environment variable instructions for a real DB; alternatively, change the assertion to accept both "connected" and a mockable sentinel and add a comment clarifying the chosen approach so CI maintainers know whether a real DB is required.packages/core/package.json (1)
30-30: Note: Zod version inconsistency across monorepo.This package uses
zod@^3.23.8whiletemplates/base/package.jsonuseszod@^4.0.0. Zod 4 has breaking API changes (unified error customization, record schema changes, etc.). Consider aligning versions to avoid confusion when sharing schemas or utilities across packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/package.json` at line 30, The repo has inconsistent Zod versions: packages referencing "zod": "^3.23.8" while other packages use "^4.0.0", which can cause runtime and type incompatibilities when sharing schemas; update the dependency entry currently set to "zod": "^3.23.8" to match the monorepo standard (e.g., "^4.0.0") or otherwise harmonize all package.json entries to a single Zod major version, then run dependency install and adjust any code using Zod APIs (look for usages of Zod schemas and error customization) to the chosen major version.packages/core/test/webhooks.test.ts (1)
11-24: Consider asserting hex format, not just length, for signatures.Line 13 and Line 23 only validate length. A regex assertion ensures the signer returns valid SHA-256 hex output.
Example assertion
expect(signature.length).toBe(64); +expect(signature).toMatch(/^[0-9a-f]{64}$/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/webhooks.test.ts` around lines 11 - 24, The tests for signPayload in webhooks.test.ts only check signature length; update both tests (the string- and object-payload cases referencing signPayload) to also assert the signature matches a hex SHA-256 pattern (e.g., 64 hex characters) using a regex check so the output is validated as hex, not just length.packages/shared/test/shared.test.ts (1)
98-100: Version test is brittle due to hardcoded release value.Line 99 will fail on routine version bumps even when behavior is correct. Prefer validating semantic version format (or deriving expected value from package metadata).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/test/shared.test.ts` around lines 98 - 100, The test "exports version string" is brittle because it asserts BETTERBASE_VERSION equals a hardcoded "0.1.0"; change it to validate the format instead by asserting BETTERBASE_VERSION matches a semantic-version pattern (e.g. a semver regex) or derive the expected value from package metadata (import package.json's version) and compare that to BETTERBASE_VERSION; update the test referencing BETTERBASE_VERSION to use the semver match or the package.json-derived value so routine version bumps do not break the test.packages/cli/test/generate-crud.test.ts (1)
24-77: Consider reusing shared fixtures to reduce scaffold drift.This local
MULTI_TABLE_SCHEMA+scaffoldProjectsetup overlaps withpackages/cli/test/fixtures.ts. Reusing the shared helper (with per-test overrides) would lower maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/test/generate-crud.test.ts` around lines 24 - 77, The test defines a local MULTI_TABLE_SCHEMA and scaffoldProject function that duplicate fixtures in packages/cli/test/fixtures.ts; replace the local definitions by importing and reusing the shared fixture utilities (e.g., the exported scaffoldProject or createTestProject helper) from fixtures.ts, then apply per-test overrides for the schema (write or pass MULTI_TABLE_SCHEMA as an argument) and for pre-created files (realtime.ts, routes/index.ts, node_modules stub) so you avoid duplicated setup logic; locate and update the test to call the shared helper instead of the local scaffoldProject and only perform minimal, test-specific writes.packages/cli/test/migrate.test.ts (1)
4-51: Add comment-handling cases forsplitStatements.Current suite validates quotes/semicolons well, but doesn’t lock behavior for
--and/* ... */comments, which are part of the parser state machine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/test/migrate.test.ts` around lines 4 - 51, Add tests to cover SQL comment handling in splitStatements: include cases for single-line comments starting with -- (e.g., a line with -- containing semicolons should not produce extra statements and comment-only input returns []), block comments /* ... */ that contain semicolons (ensure those semicolons are ignored and do not split statements), and mixed inputs where comments appear inline after statements (verify trailing comments are stripped and statements still parsed correctly). Target the splitStatements tests in migrate.test.ts and name tests clearly like "ignores semicolons inside -- comments" and "ignores semicolons inside /* */ block comments" to lock expected behavior. Ensure assertions check resulting statement count and that content does not include comment text.packages/cli/src/utils/scanner.ts (1)
215-241: Use per-property match state instead of globalindexes.length.Line 240 checks global collection size; once any prior index is found, later
.on(...)chains break early regardless of current property match outcome. Use a localfoundIndexflag for this property.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils/scanner.ts` around lines 215 - 241, The .on() chain handling uses the global indexes.length to decide whether to break, causing earlier-found indexes to short-circuit later properties; change this to use a per-property boolean (e.g., foundIndex) initialized false at the start of the if (callName === "on") block, set foundIndex = true when you push into indexes (inside the innerCallName === "index" || "uniqueIndex" branch), and replace the if (indexes.length > 0) break; check with if (foundIndex) break; so the loop only breaks for the current property when its own index was found; keep the rest of the inner traversal (inner, getCallName, property.name logic) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 18: The workflow uses bun-version: latest which conflicts with
package.json's packageManager: "bun@1.3.9" and can cause flaky CI; update all
occurrences of the bun-version key in the CI workflow jobs (the three
bun-version entries currently set to "latest") to pin the version to 1.3.9 to
match package.json so the toolchain is consistent across jobs.
In `@betterbase_test_suite_v3.md`:
- Line 43: The fenced code block at the monorepo tree example is missing a
language tag (triggering MD040); update the opening fence (the triple backticks
that begin the code block) to include a language identifier such as "text"
(i.e., change ``` to ```text) so the block is explicitly labeled and the
markdown linter warning is resolved.
In `@packages/cli/src/commands/auth.ts`:
- Around line 215-243: The import-insertion logic is brittle (string equality)
and can leave auth undefined before adding the mount; update the routine that
manipulates src/index.ts so it robustly ensures the auth import exists before
adding the handler mount: parse the file text to detect any existing import of
"auth" (allowing single/double quotes and differing whitespace) or use an
AST/ts-morph approach to insert import { auth } from "./auth" if missing, then
only proceed to insert the app.on("/api/auth/**", ...) mount after confirming
the import was added/present; update the logic around
current/indexPath/readFileSync/writeFileSync/registerRoutes/auth to bail or log
an error if a safe import insertion cannot be performed so auth.handler(...) is
never emitted without the import.
In `@packages/cli/src/utils/scanner.ts`:
- Around line 253-258: The code passes indexRoot.body (typed Block | Expression)
into unwrapExpression which expects a ts.Expression; update the guard in the
arrow/function expression branch to check the body kind before calling
unwrapExpression: after confirming ts.isArrowFunction(indexRoot) ||
ts.isFunctionExpression(indexRoot), get const body = indexRoot.body and if
(!ts.isBlock(body)) { const unwrappedBody = unwrapExpression(body); if
(ts.isObjectLiteralExpression(unwrappedBody)) collectFromObject(unwrappedBody);
} so you avoid calling unwrapExpression on a Block and keep existing behavior
for expression-bodied arrows.
In `@packages/cli/test/auth-command.test.ts`:
- Around line 75-147: Multiple tests repeatedly call runAuthSetupCommand (e.g.,
in the cases checking files and content), causing repeated package installs;
change the test suite to run runAuthSetupCommand(tmpDir, "sqlite") once in a
beforeAll for all sqlite-related tests (and similarly run once for "pg" tests if
present) and then remove the duplicate calls inside each test so tests only
assert on generated files/content; locate runAuthSetupCommand usages and replace
per-test invocations with a shared beforeAll setup (keeping individual tests as
simple asserts like reading files and checking content).
In `@packages/cli/test/edge-cases.test.ts`:
- Around line 93-122: Tests incorrectly call new RouteScanner(p).scan() passing
the path to the constructor and no args to scan; update each test to instantiate
RouteScanner with no args and call scan with the path (i.e., new
RouteScanner().scan(p)) so the RouteScanner class and its scan(routesDir:
string) signature are used correctly in tests referencing RouteScanner and its
scan method.
In `@packages/cli/test/init.test.ts`:
- Around line 18-31: Tests are only asserting the options object and never
exercise runInitCommand, so update the tests to call runInitCommand with
realistic InitCommandOptions and verify side effects: create a temporary
directory (use os.tmpdir or jest's tmpdir fixture), pass { projectName:
"test-project", targetDir: tmpDir } to runInitCommand, mock or stub filesystem
interactions (fs-extra or mock-fs) or run against the temp dir, assert expected
outputs (created files/directories or returned result) and clean up; reference
the runInitCommand function and InitCommandOptions type to locate where to
invoke the command and assert behavior.
In `@packages/client/test/query-builder.test.ts`:
- Around line 85-91: The test named ".in() sends JSON-encoded array" only
asserts the presence of "id_in" in the URL but must also assert the actual
encoded value; update the test that builds the request (the call to
makeClient/mockFetch and the chain client.from("users").in("id",
["a","b","c"]).execute()) to parse the request URL (new URL(url) and
URLSearchParams) or inspect the fetchImpl.mock.calls[0] to extract the query
param value for "id_in" and assert it equals the JSON-encoded array (e.g.,
'["a","b","c"]' or its encoded form), ensuring the test checks both the param
name and its exact encoded value.
- Around line 9-18: The helper mockFetch currently hardcodes "error: null",
which prevents testing real error payload handling; change mockFetch to accept a
response body (or separate successData and errorData parameters) instead of
always wrapping users and error:null, and return
Response(JSON.stringify(responseBody), ...) so tests can pass error payloads
(e.g., { users: null, error: "msg" }) to exercise server error paths; update
tests that call mockFetch to provide the appropriate responseBody or error
string.
In `@templates/base/test/crud.test.ts`:
- Around line 8-23: The test creates the users table in beforeAll but doesn't
clear existing rows, so tests that expect an empty table can fail; update the
beforeAll (the block that imports db and calls db.run) to ensure a deterministic
empty state by executing a cleanup statement after table creation (e.g., run
"DELETE FROM users" or "TRUNCATE" equivalent) and/or use "BEGIN/COMMIT" to
ensure the operation completes before tests run; target the beforeAll function
and the db.run usage that creates the users table and add a subsequent db.run to
remove existing rows so the table is empty for assertions.
---
Outside diff comments:
In `@packages/cli/src/commands/migrate.ts`:
- Around line 316-320: The termination check inside the line-comment handling is
wrong: change the unreachable check for ch === "" to detect a newline character
(e.g., ch === '\n' or include '\r' as needed) so the parser exits line-comment
mode at end-of-line; update the block around inLineComment, current, and ch to
set inLineComment = false when a newline is seen and handle appending or
flushing current appropriately so subsequent statements are parsed.
---
Nitpick comments:
In `@packages/cli/src/utils/scanner.ts`:
- Around line 215-241: The .on() chain handling uses the global indexes.length
to decide whether to break, causing earlier-found indexes to short-circuit later
properties; change this to use a per-property boolean (e.g., foundIndex)
initialized false at the start of the if (callName === "on") block, set
foundIndex = true when you push into indexes (inside the innerCallName ===
"index" || "uniqueIndex" branch), and replace the if (indexes.length > 0) break;
check with if (foundIndex) break; so the loop only breaks for the current
property when its own index was found; keep the rest of the inner traversal
(inner, getCallName, property.name logic) unchanged.
In `@packages/cli/test/generate-crud.test.ts`:
- Around line 24-77: The test defines a local MULTI_TABLE_SCHEMA and
scaffoldProject function that duplicate fixtures in
packages/cli/test/fixtures.ts; replace the local definitions by importing and
reusing the shared fixture utilities (e.g., the exported scaffoldProject or
createTestProject helper) from fixtures.ts, then apply per-test overrides for
the schema (write or pass MULTI_TABLE_SCHEMA as an argument) and for pre-created
files (realtime.ts, routes/index.ts, node_modules stub) so you avoid duplicated
setup logic; locate and update the test to call the shared helper instead of the
local scaffoldProject and only perform minimal, test-specific writes.
In `@packages/cli/test/migrate.test.ts`:
- Around line 4-51: Add tests to cover SQL comment handling in splitStatements:
include cases for single-line comments starting with -- (e.g., a line with --
containing semicolons should not produce extra statements and comment-only input
returns []), block comments /* ... */ that contain semicolons (ensure those
semicolons are ignored and do not split statements), and mixed inputs where
comments appear inline after statements (verify trailing comments are stripped
and statements still parsed correctly). Target the splitStatements tests in
migrate.test.ts and name tests clearly like "ignores semicolons inside --
comments" and "ignores semicolons inside /* */ block comments" to lock expected
behavior. Ensure assertions check resulting statement count and that content
does not include comment text.
In `@packages/core/package.json`:
- Line 30: The repo has inconsistent Zod versions: packages referencing "zod":
"^3.23.8" while other packages use "^4.0.0", which can cause runtime and type
incompatibilities when sharing schemas; update the dependency entry currently
set to "zod": "^3.23.8" to match the monorepo standard (e.g., "^4.0.0") or
otherwise harmonize all package.json entries to a single Zod major version, then
run dependency install and adjust any code using Zod APIs (look for usages of
Zod schemas and error customization) to the chosen major version.
In `@packages/core/test/webhooks.test.ts`:
- Around line 11-24: The tests for signPayload in webhooks.test.ts only check
signature length; update both tests (the string- and object-payload cases
referencing signPayload) to also assert the signature matches a hex SHA-256
pattern (e.g., 64 hex characters) using a regex check so the output is validated
as hex, not just length.
In `@packages/shared/test/shared.test.ts`:
- Around line 98-100: The test "exports version string" is brittle because it
asserts BETTERBASE_VERSION equals a hardcoded "0.1.0"; change it to validate the
format instead by asserting BETTERBASE_VERSION matches a semantic-version
pattern (e.g. a semver regex) or derive the expected value from package metadata
(import package.json's version) and compare that to BETTERBASE_VERSION; update
the test referencing BETTERBASE_VERSION to use the semver match or the
package.json-derived value so routine version bumps do not break the test.
In `@templates/base/test/health.test.ts`:
- Line 1: The import list in the test file includes an unused symbol `afterAll`;
remove `afterAll` from the import statement (keep only `describe, expect, test,
beforeAll`) so the file no longer imports an unused hook and lints cleanly;
update the import line that currently references `afterAll` accordingly.
- Around line 13-21: The test "GET /health returns 200 with healthy status" (in
health.test.ts) assumes a live DB by asserting data.database === "connected";
either decouple the unit test from external DB or document the integration
requirement: update the test to mock the database health-check (replace the real
DB call used by app.request("/health") with a stubbed/mocked service that
returns "connected") or mark the test as an integration test and add setup
docs/environment variable instructions for a real DB; alternatively, change the
assertion to accept both "connected" and a mockable sentinel and add a comment
clarifying the chosen approach so CI maintainers know whether a real DB is
required.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.github/workflows/ci.yml.kilocode/mcp.jsonbetterbase_auth_refactor.mdbetterbase_blueprint_v3.mdbetterbase_test_suite_v3.mdpackage.jsonpackages/cli/src/commands/auth.tspackages/cli/src/commands/migrate.tspackages/cli/src/utils/scanner.tspackages/cli/test/auth-command.test.tspackages/cli/test/edge-cases.test.tspackages/cli/test/fixtures.tspackages/cli/test/generate-crud.test.tspackages/cli/test/init.test.tspackages/cli/test/migrate.test.tspackages/cli/test/smoke.test.tspackages/client/test/client.test.tspackages/client/test/edge-cases.test.tspackages/client/test/errors.test.tspackages/client/test/query-builder.test.tspackages/client/test/realtime.test.tspackages/core/package.jsonpackages/core/test/config.test.tspackages/core/test/webhooks.test.tspackages/shared/package.jsonpackages/shared/test/shared.test.tstemplates/base/package.jsontemplates/base/src/routes/users.tstemplates/base/test/crud.test.tstemplates/base/test/health.test.tstemplates/base/tsconfig.jsonturbo.json
💤 Files with no reviewable changes (3)
- betterbase_blueprint_v3.md
- betterbase_auth_refactor.md
- .kilocode/mcp.json
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Bun version alignment between repository metadata and CI workflow
echo "=== Checking packageManager in package.json ==="
rg -n '"packageManager"\s*:\s*"bun@' package.json
echo "=== Checking bun-version in CI workflow ==="
rg -n 'bun-version:' .github/workflows/ci.yml
echo "=== Full context for each bun-version occurrence ==="
rg -B2 -A2 'bun-version:' .github/workflows/ci.ymlRepository: weroperking/Betterbase
Length of output: 695
Pin Bun version in CI to match package.json instead of latest.
Currently, the CI workflow uses bun-version: latest while package.json specifies "packageManager": "bun@1.3.9". Using latest can cause flaky or sudden breakage. Update all three jobs to align with the pinned toolchain.
Proposed change
- bun-version: latest
+ bun-version: 1.3.9Apply to all three jobs (lines 18, 39, 57).
📝 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.
Suggested change
| bun-version: latest | |
| bun-version: 1.3.9 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 18, The workflow uses bun-version: latest
which conflicts with package.json's packageManager: "bun@1.3.9" and can cause
flaky CI; update all occurrences of the bun-version key in the CI workflow jobs
(the three bun-version entries currently set to "latest") to pin the version to
1.3.9 to match package.json so the toolchain is consistent across jobs.
|
|
||
| ## MONOREPO STRUCTURE (the ground truth — verified from `tree -I node_modules`) | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block.
Line 43 opens a fenced code block without a language, which triggers MD040.
📝 Proposed fix
-```
+```text
betterbase/ ← monorepo root
├── package.json
...
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@betterbase_test_suite_v3.md` at line 43, The fenced code block at the
monorepo tree example is missing a language tag (triggering MD040); update the
opening fence (the triple backticks that begin the code block) to include a
language identifier such as "text" (i.e., change ``` to ```text) so the block is
explicitly labeled and the markdown linter warning is resolved.
Comment on lines
215
to
243
| if (!current.includes('import { auth } from "./auth"')) { | ||
| const insertAfter = 'import { registerRoutes } from "./routes";'; | ||
| const importLine = '\nimport { auth } from "./auth";'; | ||
| const updated = current.replace(insertAfter, insertAfter + importLine); | ||
| // Try with semicolon first, then without | ||
| let insertAfter = 'import { registerRoutes } from "./routes";'; | ||
| let importLine = '\nimport { auth } from "./auth";'; | ||
| let updated = current.replace(insertAfter, insertAfter + importLine); | ||
|
|
||
| if (updated === current) { | ||
| insertAfter = 'import { registerRoutes } from "./routes"'; | ||
| updated = current.replace(insertAfter, insertAfter + importLine); | ||
| } | ||
|
|
||
| writeFileSync(indexPath, updated); | ||
| } | ||
|
|
||
| // Add the auth handler mount if not present | ||
| const updatedWithMount = readFileSync(indexPath, "utf-8"); | ||
| if (!updatedWithMount.includes("/api/auth/**")) { | ||
| const insertAfter = "registerRoutes(app);"; | ||
| const mountCode = `\n\napp.on(["POST", "GET"], "/api/auth/**", (c) => {\n return auth.handler(c.req.raw)\n})`; | ||
| const final = updatedWithMount.replace(insertAfter, insertAfter + mountCode); | ||
| // Try with semicolon first, then without | ||
| let insertAfter = "registerRoutes(app);"; | ||
| let mountCode = `\n\napp.on(["POST", "GET"], "/api/auth/**", (c) => {\n return auth.handler(c.req.raw)\n})`; | ||
| let final = updatedWithMount.replace(insertAfter, insertAfter + mountCode); | ||
|
|
||
| if (final === updatedWithMount) { | ||
| insertAfter = "registerRoutes(app)"; | ||
| final = updatedWithMount.replace(insertAfter, insertAfter + mountCode); | ||
| } | ||
|
|
||
| writeFileSync(indexPath, final); | ||
| logger.info("Updated src/index.ts with BetterAuth handler mount"); |
There was a problem hiding this comment.
Import patching is too brittle and can leave auth undefined.
Line 217/Line 222 rely on exact string matches. For valid src/index.ts styles (e.g., single quotes/formatting variation), import insertion can fail silently, while Line 233–Line 240 may still insert the mount. That yields auth.handler(...) without an auth import.
🔧 Proposed hardening
- if (!current.includes('import { auth } from "./auth"')) {
- // Try with semicolon first, then without
- let insertAfter = 'import { registerRoutes } from "./routes";';
- let importLine = '\nimport { auth } from "./auth";';
- let updated = current.replace(insertAfter, insertAfter + importLine);
-
- if (updated === current) {
- insertAfter = 'import { registerRoutes } from "./routes"';
- updated = current.replace(insertAfter, insertAfter + importLine);
- }
-
- writeFileSync(indexPath, updated);
- }
+ const hasAuthImport = /import\s*\{\s*auth\s*\}\s*from\s*["']\.\/auth["'];?/.test(current);
+ if (!hasAuthImport) {
+ const importAnchor = /(^\s*import\s*\{\s*registerRoutes\s*\}\s*from\s*["']\.\/routes["'];?\s*$)/m;
+ const updated = current.replace(importAnchor, `$1\nimport { auth } from "./auth";`);
+ if (updated === current) {
+ logger.warn('Could not auto-insert auth import in src/index.ts. Please add `import { auth } from "./auth"` manually.');
+ } else {
+ writeFileSync(indexPath, updated);
+ }
+ }
@@
- let insertAfter = "registerRoutes(app);";
let mountCode = `\n\napp.on(["POST", "GET"], "/api/auth/**", (c) => {\n return auth.handler(c.req.raw)\n})`;
- let final = updatedWithMount.replace(insertAfter, insertAfter + mountCode);
-
- if (final === updatedWithMount) {
- insertAfter = "registerRoutes(app)";
- final = updatedWithMount.replace(insertAfter, insertAfter + mountCode);
- }
-
- writeFileSync(indexPath, final);
+ const mountAnchor = /registerRoutes\s*\(\s*app\s*\)\s*;?/;
+ const final = updatedWithMount.replace(mountAnchor, (m) => `${m}${mountCode}`);
+ if (final === updatedWithMount) {
+ logger.warn("Could not auto-insert BetterAuth mount in src/index.ts. Please add it manually.");
+ return;
+ }
+ writeFileSync(indexPath, final);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/auth.ts` around lines 215 - 243, The
import-insertion logic is brittle (string equality) and can leave auth undefined
before adding the mount; update the routine that manipulates src/index.ts so it
robustly ensures the auth import exists before adding the handler mount: parse
the file text to detect any existing import of "auth" (allowing single/double
quotes and differing whitespace) or use an AST/ts-morph approach to insert
import { auth } from "./auth" if missing, then only proceed to insert the
app.on("/api/auth/**", ...) mount after confirming the import was added/present;
update the logic around
current/indexPath/readFileSync/writeFileSync/registerRoutes/auth to bail or log
an error if a safe import insertion cannot be performed so auth.handler(...) is
never emitted without the import.
Comment on lines
+75
to
+147
| test("creates src/auth/index.ts", async () => { | ||
| // Increase timeout for first test - bun add better-auth takes ~30s on first run | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| expect(existsSync(join(tmpDir, "src/auth/index.ts"))).toBe(true); | ||
| }, 60000); | ||
|
|
||
| test("creates src/auth/types.ts", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| expect(existsSync(join(tmpDir, "src/auth/types.ts"))).toBe(true); | ||
| }); | ||
|
|
||
| test("creates src/db/auth-schema.ts", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| expect(existsSync(join(tmpDir, "src/db/auth-schema.ts"))).toBe(true); | ||
| }); | ||
|
|
||
| test("creates src/middleware/auth.ts", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| expect(existsSync(join(tmpDir, "src/middleware/auth.ts"))).toBe(true); | ||
| }); | ||
|
|
||
| test("middleware contains requireAuth export", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| const content = await readFile(join(tmpDir, "src/middleware/auth.ts"), "utf-8"); | ||
| expect(content).toContain("requireAuth"); | ||
| }); | ||
|
|
||
| test("middleware contains optionalAuth export", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| const content = await readFile(join(tmpDir, "src/middleware/auth.ts"), "utf-8"); | ||
| expect(content).toContain("optionalAuth"); | ||
| }); | ||
|
|
||
| test("auth-schema.ts contains user and session tables for sqlite", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| const schema = await readFile(join(tmpDir, "src/db/auth-schema.ts"), "utf-8"); | ||
| expect(schema).toContain("sqliteTable"); | ||
| expect(schema).toContain("user"); | ||
| expect(schema).toContain("session"); | ||
| }); | ||
|
|
||
| test("auth-schema.ts uses pgTable for pg provider", async () => { | ||
| await runAuthSetupCommand(tmpDir, "pg"); | ||
| const schema = await readFile(join(tmpDir, "src/db/auth-schema.ts"), "utf-8"); | ||
| expect(schema).toContain("pgTable"); | ||
| }); | ||
|
|
||
| test("auth/index.ts references the correct provider and betterAuth", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| const content = await readFile(join(tmpDir, "src/auth/index.ts"), "utf-8"); | ||
| expect(content).toContain("sqlite"); | ||
| expect(content).toContain("betterAuth"); | ||
| }); | ||
|
|
||
| test("adds AUTH_SECRET to .env.example", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| const env = await readFile(join(tmpDir, ".env.example"), "utf-8"); | ||
| expect(env).toContain("AUTH_SECRET"); | ||
| }); | ||
|
|
||
| test("mounts auth handler in src/index.ts", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| const index = await readFile(join(tmpDir, "src/index.ts"), "utf-8"); | ||
| expect(index).toContain("/api/auth/**"); | ||
| }); | ||
|
|
||
| test("is idempotent — running twice does not duplicate auth handler mount", async () => { | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| await runAuthSetupCommand(tmpDir, "sqlite"); | ||
| const index = await readFile(join(tmpDir, "src/index.ts"), "utf-8"); | ||
| const matches = index.match(/\/api\/auth\/\*\*/g) || []; | ||
| expect(matches.length).toBe(1); | ||
| }); |
There was a problem hiding this comment.
Tests repeatedly trigger package installation, making CI flaky and slow.
Line 77 and most subsequent tests call runAuthSetupCommand independently, which reruns bun add better-auth. This introduces avoidable network dependency and large runtime variance in a unit-test suite.
⚡ Suggested restructure (run setup once per provider)
describe("runAuthSetupCommand", () => {
- let tmpDir: string;
-
- beforeEach(async () => {
- tmpDir = await mkdtemp(join(tmpdir(), "bb-auth-"));
- await scaffoldProject(tmpDir);
- });
-
- afterEach(async () => {
- await rm(tmpDir, { recursive: true, force: true });
- });
+ describe("sqlite provider", () => {
+ let tmpDir: string;
+ beforeAll(async () => {
+ tmpDir = await mkdtemp(join(tmpdir(), "bb-auth-"));
+ await scaffoldProject(tmpDir);
+ await runAuthSetupCommand(tmpDir, "sqlite");
+ }, 60000);
+ afterAll(async () => {
+ await rm(tmpDir, { recursive: true, force: true });
+ });
- test("creates src/auth/index.ts", async () => {
- await runAuthSetupCommand(tmpDir, "sqlite");
- expect(existsSync(join(tmpDir, "src/auth/index.ts"))).toBe(true);
- }, 60000);
+ test("creates src/auth/index.ts", async () => {
+ expect(existsSync(join(tmpDir, "src/auth/index.ts"))).toBe(true);
+ });
+ // ...other sqlite assertions without re-running command
+ });
+
+ describe("pg provider", () => {
+ let tmpDir: string;
+ beforeAll(async () => {
+ tmpDir = await mkdtemp(join(tmpdir(), "bb-auth-pg-"));
+ await scaffoldProject(tmpDir);
+ await runAuthSetupCommand(tmpDir, "pg");
+ }, 60000);
+ afterAll(async () => {
+ await rm(tmpDir, { recursive: true, force: true });
+ });
+ });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/test/auth-command.test.ts` around lines 75 - 147, Multiple tests
repeatedly call runAuthSetupCommand (e.g., in the cases checking files and
content), causing repeated package installs; change the test suite to run
runAuthSetupCommand(tmpDir, "sqlite") once in a beforeAll for all sqlite-related
tests (and similarly run once for "pg" tests if present) and then remove the
duplicate calls inside each test so tests only assert on generated
files/content; locate runAuthSetupCommand usages and replace per-test
invocations with a shared beforeAll setup (keeping individual tests as simple
asserts like reading files and checking content).
Comment on lines
+18
to
+31
| test("creates project with project name", async () => { | ||
| const options: InitCommandOptions = { | ||
| projectName: "test-project", | ||
| }; | ||
|
|
||
| // This will fail because the command tries to create files in the current directory | ||
| // We're just testing that the options are accepted | ||
| expect(options.projectName).toBe("test-project"); | ||
| }); | ||
|
|
||
| test("InitCommandOptions type is correct", () => { | ||
| const options: InitCommandOptions = {}; | ||
| expect(options).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Tests are placeholders and do not exercise runInitCommand.
Both tests pass without executing command behavior, so regressions in init logic won’t be caught. Line 5 imports runInitCommand, but it is never called.
💡 Suggested test direction
test("creates project with project name", async () => {
- const options: InitCommandOptions = {
- projectName: "test-project",
- };
-
- // This will fail because the command tries to create files in the current directory
- // We're just testing that the options are accepted
- expect(options.projectName).toBe("test-project");
+ const cwd = process.cwd();
+ try {
+ process.chdir(tempDir);
+ await runInitCommand({ projectName: "test-project" });
+ expect(existsSync(join(tempDir, "test-project"))).toBe(true);
+ } finally {
+ process.chdir(cwd);
+ }
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/test/init.test.ts` around lines 18 - 31, Tests are only
asserting the options object and never exercise runInitCommand, so update the
tests to call runInitCommand with realistic InitCommandOptions and verify side
effects: create a temporary directory (use os.tmpdir or jest's tmpdir fixture),
pass { projectName: "test-project", targetDir: tmpDir } to runInitCommand, mock
or stub filesystem interactions (fs-extra or mock-fs) or run against the temp
dir, assert expected outputs (created files/directories or returned result) and
clean up; reference the runInitCommand function and InitCommandOptions type to
locate where to invoke the command and assert behavior.
Comment on lines
+9
to
+18
| function mockFetch(responseData: unknown, status = 200) { | ||
| return mock(() => | ||
| Promise.resolve( | ||
| new Response(JSON.stringify({ users: responseData, error: null }), { | ||
| status, | ||
| headers: { "Content-Type": "application/json" }, | ||
| }), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
mockFetch hardcodes error: null, weakening error-path coverage.
Line 12 always sets error: null, so failure tests don’t validate server-provided error payload handling realistically.
🔧 Suggested helper refinement
-function mockFetch(responseData: unknown, status = 200) {
+function mockFetch(body: unknown, status = 200) {
return mock(() =>
Promise.resolve(
- new Response(JSON.stringify({ users: responseData, error: null }), {
+ new Response(JSON.stringify(body), {
status,
headers: { "Content-Type": "application/json" },
}),
),
);
}Then pass success/error bodies explicitly per test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/test/query-builder.test.ts` around lines 9 - 18, The helper
mockFetch currently hardcodes "error: null", which prevents testing real error
payload handling; change mockFetch to accept a response body (or separate
successData and errorData parameters) instead of always wrapping users and
error:null, and return Response(JSON.stringify(responseBody), ...) so tests can
pass error payloads (e.g., { users: null, error: "msg" }) to exercise server
error paths; update tests that call mockFetch to provide the appropriate
responseBody or error string.
Comment on lines
+85
to
+91
| test(".in() sends JSON-encoded array", async () => { | ||
| const fetchImpl = mockFetch([]); | ||
| const client = makeClient(fetchImpl); | ||
| await client.from("users").in("id", ["a", "b", "c"]).execute(); | ||
| const [url] = fetchImpl.mock.calls[0] as [string]; | ||
| expect(url).toContain("id_in"); | ||
| }); |
There was a problem hiding this comment.
.in() test name and assertion are misaligned.
The test says it verifies JSON-encoded array transport, but it only checks that id_in exists in the URL. Please assert the encoded value as well (or parse URLSearchParams and assert exact param value).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/test/query-builder.test.ts` around lines 85 - 91, The test
named ".in() sends JSON-encoded array" only asserts the presence of "id_in" in
the URL but must also assert the actual encoded value; update the test that
builds the request (the call to makeClient/mockFetch and the chain
client.from("users").in("id", ["a","b","c"]).execute()) to parse the request URL
(new URL(url) and URLSearchParams) or inspect the fetchImpl.mock.calls[0] to
extract the query param value for "id_in" and assert it equals the JSON-encoded
array (e.g., '["a","b","c"]' or its encoded form), ensuring the test checks both
the param name and its exact encoded value.
Comment on lines
+8
to
+23
| beforeAll(async () => { | ||
| // Import db AFTER app modules load — this is the exact same | ||
| // db instance the route handlers will use at runtime. | ||
| // We run CREATE TABLE IF NOT EXISTS on it so the schema exists | ||
| // before any test hits the GET /api/users endpoint. | ||
| const { db } = await import("../src/db"); | ||
|
|
||
| db.run(` | ||
| CREATE TABLE IF NOT EXISTS users ( | ||
| id TEXT PRIMARY KEY, | ||
| name TEXT NOT NULL, | ||
| email TEXT NOT NULL UNIQUE, | ||
| created_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| updated_at INTEGER NOT NULL DEFAULT (unixepoch()) | ||
| ) | ||
| `); |
There was a problem hiding this comment.
Stabilize DB state before assertions that expect empty data.
Line 15 creates the table, but the suite never clears users. If the underlying DB persists between runs, Line 35 can fail nondeterministically.
Suggested fix
beforeAll(async () => {
@@
db.run(`
CREATE TABLE IF NOT EXISTS users (
@@
)
`);
+ db.run(`DELETE FROM users`);
app = new Hono();
registerRoutes(app);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base/test/crud.test.ts` around lines 8 - 23, The test creates the
users table in beforeAll but doesn't clear existing rows, so tests that expect
an empty table can fail; update the beforeAll (the block that imports db and
calls db.run) to ensure a deterministic empty state by executing a cleanup
statement after table creation (e.g., run "DELETE FROM users" or "TRUNCATE"
equivalent) and/or use "BEGIN/COMMIT" to ensure the operation completes before
tests run; target the beforeAll function and the db.run usage that creates the
users table and add a subsequent db.run to remove existing rows so the table is
empty for assertions.
## Lint Fixes (@betterbase/client) - **[](packages/client/src/client.ts:46)** - Replaced with to fix the Biome lint rule - **[](packages/client/test/realtime.test.ts)** - Fixed formatting (method chains on separate lines) - **[](packages/client/test/errors.test.ts)** - Fixed import sorting (alphabetical order) - **[](packages/client/test/query-builder.test.ts)** - Fixed Promise.resolve formatting - **[](packages/client/test/edge-cases.test.ts)** - Fixed format issues ## TypeScript Fixes (@betterbase/cli) - **[](packages/cli/src/utils/scanner.ts:255)** - Fixed AST type error by adding guard check for before calling - **[](packages/cli/test/edge-cases.test.ts)** - Fixed incorrect API usage: takes no constructor arguments, and takes a directory path ## Additional Fixes (discovered during verification) - **[](packages/client/src/client.ts)** - Changed type from to - **[](templates/base/src/routes/storage.ts)** - Fixed multiple TypeScript errors (S3Config, String object, Buffer/Uint8Array) ## Verification Results - • Packages in scope: @betterbase/cli, @betterbase/client, @betterbase/core, @betterbase/shared, betterbase-base-template • Running lint in 5 packages • Remote caching disabled @betterbase/client:lint: cache hit, replaying logs 55d3b0dc710279c9 @betterbase/client:lint: $ biome check src test @betterbase/client:lint: Checked 14 files in 81ms. No fixes applied. Tasks: 1 successful, 1 total Cached: 1 cached, 1 total Time: 73ms >>> FULL TURBO ✅ PASSED (exit code 0) - • Packages in scope: //, @betterbase/cli, @betterbase/client, @betterbase/core, @betterbase/shared, betterbase-base-template • Running typecheck in 6 packages • Remote caching disabled @betterbase/client:typecheck: cache hit, replaying logs c9f80e4288e6ae22 @betterbase/client:typecheck: $ tsc --noEmit --project tsconfig.json @betterbase/shared:typecheck: cache hit, replaying logs 65460088672fafbb @betterbase/shared:typecheck: $ tsc --noEmit @betterbase/cli:typecheck: cache hit, replaying logs fd14307b1a6591f3 @betterbase/cli:typecheck: $ tsc -p tsconfig.json --noEmit betterbase-base-template:typecheck: cache hit, replaying logs 8ff0bbf2f4a93d46 betterbase-base-template:typecheck: $ tsc --noEmit @betterbase/core:typecheck: cache hit, replaying logs 656eadacec63901c @betterbase/core:typecheck: $ tsc --noEmit Tasks: 5 successful, 5 total Cached: 5 cached, 5 total Time: 62ms >>> FULL TURBO ✅ PASSED (exit code 0) - All 5 packages pass
…erbase into feat/test-suite
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.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores