feat: comprehensive core tasks for March 2026 release#36
Merged
weroperking merged 24 commits intomainfrom Mar 21, 2026
Merged
Conversation
- Add new auth-providers command for managing authentication providers - Enhance existing auth command with additional features - Add tests for auth and login commands
- Improve dev server with better function lifecycle management - Add RLS test command for row-level security testing - Add branch commands tests
- Implement new migrate command for database migrations - Add migrate-schema.sql with schema definitions - Add migrate-utils for migration helpers - Add tests for migrate utilities
- Improve webhook command with better event handling - Add webhook command tests
- Enhance CLI main index with improved command registration - Update logger with additional utility functions - Update CLI package.json with new dependencies - Update logger tests
- Improve webhook dispatcher with better event processing - Add support for multiple webhook handlers - Enhance error handling and retry logic
- Enhance webhook startup initialization - Add schema.sql for webhook configurations - Update webhook index exports
- Implement S3 adapter for cloud storage operations - Add comprehensive storage type definitions - Support for multiple storage backends
- Add storage module index with exports - Implement image transformer for on-the-fly image processing - Add support for image optimization and resizing
- Improve GraphQL resolvers with better query handling - Enhance GraphQL server with additional features - Add support for complex data types
- Add schema generator for dynamic GraphQL schemas - Implement realtime-bridge for real-time GraphQL subscriptions - Enhance GraphQL index exports
- Implement local-runtime for local function execution - Enhance functions index with additional exports - Support for local development and testing
- Implement request-logger for HTTP request logging - Add comprehensive logger module - Enhance middleware index with new exports
- Improve auto-rest with better CRUD operations - Enhance migration system with improved schema handling - Update migration tests
- Enhance branching module with improved storage handling - Add configuration schema updates - Improve postgres provider implementations
- Enhance core index with additional exports - Add new realtime module for real-time functionality
- Improve client realtime module with better event handling - Add support for real-time subscriptions - Enhance connection management
- Enhance test project with improved realtime support - Add webhooks route - Add hello function example - Update routes configuration
- Add tests for function commands - Add graphql type map tests - Add storage command tests
- Add tests for auto-rest functions - Add chain code maps tests - Add functions runtime tests - Add image transformer tests - Add logger functions tests
- Add middleware functions tests - Add realtime channel manager tests - Add vector search tests - Add webhook functions tests
- Add new-features-docs with updated documentation - Remove deprecated issues.md and core task issues 2.md
- Update bun.lock with new dependency versions - Update packages/core/package.json with new dependencies
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix postgres import in rls-test.ts - Fix quote formatting in client realtime.ts - Fix Handler type in local-runtime.ts - Fix WebhookDbClient adapter in test-project - Simplify webhook routes to avoid type errors - Add test summary script with pass/fail/skip counts
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
scripts/test-summary.ts (1)
1-58: Duplicated test summary logic withpackage.jsontest script.This script duplicates the test summary parsing logic found in the root
package.jsontest script (same regex patterns, same output format). Consider either:
- Using this script from
package.json:"test": "bun scripts/test-summary.ts"- Removing this script if the inline shell approach in
package.jsonis preferred🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-summary.ts` around lines 1 - 58, This file duplicates the test-summary parsing/printing logic (the Bun spawn and regex parsing using proc, fullOutput, passMatch/failMatch/testsMatch and the final exit handling), so either remove this standalone script and call the same logic from package.json (e.g. replace the inline shell summary with "bun scripts/test-summary.ts") or consolidate into a single shared module used by both locations; update package.json to point to the script if keeping it, or delete the script and remove the duplicate regex/summary code from package.json if preferring the inline approach, ensuring proc exit handling and regex patterns (/\d+ pass/g, /\d+ fail/g, /Ran (\d+) tests?/g) remain consistent.apps/test-project/src/routes/webhooks.ts (1)
21-25: Unused variabledeliveryId.The
deliveryIdparameter is extracted but never used before returning the 404 response.🔧 Suggested fix
webhooksRoute.get("/deliveries/:deliveryId", async (c) => { - const deliveryId = c.req.param("deliveryId"); + const _deliveryId = c.req.param("deliveryId"); return c.json({ error: "Delivery not found" }, 404); });Alternatively, remove the extraction entirely until the implementation is ready, or prefix with underscore to indicate intentionally unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/test-project/src/routes/webhooks.ts` around lines 21 - 25, The route handler webhooksRoute.get("/deliveries/:deliveryId", async (c) => { ... }) extracts deliveryId but never uses it; either remove the unused extraction or rename it to _deliveryId to indicate intentional non‑use, or implement the lookup using deliveryId (e.g., fetch delivery by id and return it or 404) and then return the appropriate c.json response — update the handler body in the webhooksRoute.get callback accordingly.apps/test-project/src/index.ts (2)
14-19: Stub adapter silently discards webhook delivery logs.The
dbAdapteralways returns empty rows without actually persisting anything. While this is intentional for the stub phase, consider adding a TODO or logging when deliveries would be logged.💡 Suggestion for visibility
// Create an adapter to make drizzle SQLite compatible with WebhookDbClient interface +// TODO: Implement actual persistence once migration is applied const dbAdapter: WebhookDbClient = { async execute(_args: { sql: string; args: unknown[] }) { + // Silently drop webhook delivery logs until migration is applied return { rows: [] }; }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/test-project/src/index.ts` around lines 14 - 19, The stub dbAdapter (object named dbAdapter implementing WebhookDbClient) currently returns empty rows and silently discards webhook delivery logs; update the execute method to at minimum log a clear warning (including the passed _args.sql/_args.args or a summary) and add a TODO comment indicating persistence should be implemented, while preserving the method signature and return shape ({ rows: [] }) so callers continue to work; reference dbAdapter.execute and WebhookDbClient when making the change.
116-118: Type assertion onprocess.envmay propagate undefined values.
process.envhas typeNodeJS.ProcessEnvwhere values arestring | undefined. Casting toRecord<string, string>masks the potential for undefined values, which could cause issues in functions expecting all env vars to be defined.♻️ Safer approach
- const functionsRuntime = await initializeFunctionsRuntime( - ".", - process.env as Record<string, string>, - ); + // Filter to only defined env vars + const envVars = Object.fromEntries( + Object.entries(process.env).filter((entry): entry is [string, string] => entry[1] !== undefined) + ); + const functionsRuntime = await initializeFunctionsRuntime(".", envVars);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/test-project/src/index.ts` around lines 116 - 118, The call is force-casting process.env to Record<string,string>, which hides undefined values; update the call site around initializeFunctionsRuntime and process.env to pass a correctly-typed object (either change initializeFunctionsRuntime to accept Record<string,string|undefined> or construct a sanitized env object by iterating process.env and including only keys with defined string values or supplying explicit defaults or validation that throws on missing values) so callers and callee no longer rely on an unsafe type assertion.packages/core/src/functions/local-runtime.ts (2)
80-113: Consider validating function name at load boundary as defense-in-depth.While the middleware should validate input,
loadFunctiondirectly constructs filesystem paths fromname. Adding validation here provides defense-in-depth against internal misuse.🛡️ Suggested addition
async loadFunction(name: string): Promise<LoadedFunction> { + if (!/^[a-zA-Z0-9_-]+$/.test(name)) { + throw new Error(`Invalid function name: ${name}`); + } const functionPath = path.join(this.functionsDir, name, "index.ts");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/functions/local-runtime.ts` around lines 80 - 113, Validate the incoming name inside loadFunction before constructing functionPath: reject names containing path separators or traversal patterns (e.g., '/', '\', '..') and enforce a safe whitelist regex such as /^[A-Za-z0-9_-]+$/; if validation fails, throw a clear Error (e.g., "Invalid function name") so functionsDir + name cannot be abused, and keep the rest of loadFunction (statSync, import, caching) unchanged.
171-182: File watcher callback should handle edge cases.The watcher callback splits on
path.sepbut doesn't account for:
- Files directly in
functionsDir(not in subdirectories)- Platform differences where
filenamemay use different separators♻️ More robust extraction
this.watcher = watch(this.functionsDir, { recursive: true }, (eventType, filename) => { if (filename && filename.endsWith(".ts")) { - // Extract function name from path (first segment) - const parts = filename.split(path.sep); - const functionName = parts[0]; + // Extract function name from path (first directory segment) + // Normalize to handle mixed separators on Windows + const normalized = filename.replace(/\\/g, "/"); + const parts = normalized.split("/"); + const functionName = parts.length > 1 ? parts[0] : null; - if (functionName && functionName !== "functions") { + if (functionName) { logger.info({ msg: `File changed, invalidating cache`, file: filename }); this.functions.delete(functionName); } } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/functions/local-runtime.ts` around lines 171 - 182, The watcher callback can mis-extract the function name for files at the functionsDir root and on platforms with different path separators; update the callback in the watch handler to (1) normalize the incoming filename separators (e.g. filename.replace(/[/\\]/g, path.sep)), (2) compute the path relative to this.functionsDir using path.relative(this.functionsDir, path.resolve(this.functionsDir, normalizedFilename)) to ensure we get a consistent relative path, (3) split that relative path on path.sep and pick the first non-empty segment (fall back to path.basename if necessary) as the functionName, and (4) keep the existing guard (functionName && functionName !== "functions") before calling this.functions.delete(functionName) so root-level files and cross-platform separators are handled safely.
🤖 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/test-project/src/index.ts`:
- Line 9: Remove the unused import of db from the top of the file: locate the
import statement "import { db } from \"./db\";" and delete it (or remove db from
the import list if other imports are present) so the module no longer imports an
unused symbol named db.
In `@package.json`:
- Line 46: Update the "test" npm script in package.json (the "test" script
string) to avoid GNU-specific grep -P usage: replace all grep -oP '\\d+'
patterns with grep -oE '[0-9]+' and change instances like '\\d+ pass' to '[0-9]+
pass', '\\d+ fail' to '[0-9]+ fail', '\\d+ skip' to '[0-9]+ skip', and 'Ran \\d+
tests?' to 'Ran [0-9]+ tests?' so macOS/BSD grep (using -oE) works
cross-platform.
- Line 46: The "test" npm script in package.json currently chains commands with
semicolons so test failures are lost; update the "test" script to capture the
exit code from the test runner (the bunx turbo run test invocation) into a
variable (or use &&/|| logic) before running the summary commands and then exit
with that captured code so CI sees failures, and also replace GNU-only grep -oP
usages with POSIX-friendly grep -oE in the summary pipeline to improve
portability; locate the "test" script string in package.json to implement these
changes.
In `@packages/client/src/realtime.ts`:
- Line 342: The JSDoc closing token is on the same line as the method
declaration for off(eventType: string, callback: (data: unknown) => void): void;
move the closing comment delimiter (*/) to its own line immediately above the
method signature to separate the JSDoc block from the function declaration and
restore consistent formatting in the off method in
packages/client/src/realtime.ts.
- Around line 306-322: The onPresence and onBroadcast methods currently register
anonymous listeners with this.on and never remove them; change each to create a
named handler (e.g., presenceHandler / broadcastHandler) that casts the incoming
data to PresenceEvent/BroadcastEvent, register it via this.on("presence",
presenceHandler) / this.on("broadcast", broadcastHandler), and return an
unsubscribe function that calls this.off("presence", presenceHandler) /
this.off("broadcast", broadcastHandler). Ensure the handler still checks
event.channel === channelName before invoking the callback so behavior is
unchanged.
- Around line 271-283: The channel subscription code can drop subscribe messages
because channel().subscribe() calls connect() then immediately this.send(),
which will be ignored if the socket is still CONNECTING; fix by tracking channel
subscriptions and re-sending them on reconnect like table subscriptions. Modify
channel(channelName).subscribe / unsubscribe to add/remove channelName (and
options if needed) to a tracked map or Set (e.g., trackedChannelSubscriptions)
and call this.sendSubscribeAll-style logic from the socket onopen handler to
iterate and re-send all tracked channel subscribe messages; also ensure
immediate subscribe attempts send if socket.readyState === WebSocket.OPEN,
otherwise rely on the tracked set so the onopen replay will deliver the
subscribe. Ensure unsubscribe removes from the tracked set and sends an
unsubscribe if socket is open.
In `@packages/core/src/functions/local-runtime.ts`:
- Around line 259-262: The function initializeFunctionsRuntime currently
defaults envVars to process.env which can leak secrets; change the signature and
callers so envVars is required (remove the default) or accept an explicit
allowlist parameter and filter process.env before use; update references to
initializeFunctionsRuntime and any call sites to pass a controlled
Record<string,string> (or an allowlist array) and implement the filtering logic
inside initializeFunctionsRuntime (or a helper like filterEnvForFunctions) to
only include allowed keys.
- Around line 147-158: The catch block currently returns error.message to
clients which can leak internals; update the catch in localRuntime (around
func.handler invocation) to keep logger.error({ msg: ..., function: name, error
}) for full diagnostics but replace the response body with a generic
production-safe message (e.g. "Internal Server Error" or "An internal error
occurred") and avoid including error.message or stack; optionally gate returning
the actual error text behind a debug/dev flag (NODE_ENV !== 'production') if you
need verboseness in development.
In `@scripts/test-summary.ts`:
- Around line 55-58: The closing separator console.log call is skipped when
tests fail because process.exit(1) is called before it; modify the control flow
around totalFail and proc.exitCode so the separator is printed regardless of
failure — e.g., call console.log("━━━━━━━━...") before invoking process.exit(1)
or ensure a single exit path prints the separator then exits; update the logic
that checks totalFail and proc.exitCode (the block that currently calls
process.exit) so console.log always runs prior to process.exit.
- Around line 3-11: The current code awaits proc.exited before consuming
proc.stdout/proc.stderr, which can deadlock if the child fills the pipe buffer;
change to consume streams while the process runs (or set Bun.spawn(..., { stdio:
"inherit" }) if you don't need to parse output). Specifically, when calling
Bun.spawn(...) (symbol: proc) either 1) start streaming proc.stdout and
proc.stderr to process.stdout/process.stderr using async iteration or stream
piping immediately after spawn and only await proc.exited after those consumers
are started, or 2) pass the option stdio: "inherit" to Bun.spawn to forward
output directly; ensure any parsing logic adapts if you choose inherit.
---
Nitpick comments:
In `@apps/test-project/src/index.ts`:
- Around line 14-19: The stub dbAdapter (object named dbAdapter implementing
WebhookDbClient) currently returns empty rows and silently discards webhook
delivery logs; update the execute method to at minimum log a clear warning
(including the passed _args.sql/_args.args or a summary) and add a TODO comment
indicating persistence should be implemented, while preserving the method
signature and return shape ({ rows: [] }) so callers continue to work; reference
dbAdapter.execute and WebhookDbClient when making the change.
- Around line 116-118: The call is force-casting process.env to
Record<string,string>, which hides undefined values; update the call site around
initializeFunctionsRuntime and process.env to pass a correctly-typed object
(either change initializeFunctionsRuntime to accept
Record<string,string|undefined> or construct a sanitized env object by iterating
process.env and including only keys with defined string values or supplying
explicit defaults or validation that throws on missing values) so callers and
callee no longer rely on an unsafe type assertion.
In `@apps/test-project/src/routes/webhooks.ts`:
- Around line 21-25: The route handler
webhooksRoute.get("/deliveries/:deliveryId", async (c) => { ... }) extracts
deliveryId but never uses it; either remove the unused extraction or rename it
to _deliveryId to indicate intentional non‑use, or implement the lookup using
deliveryId (e.g., fetch delivery by id and return it or 404) and then return the
appropriate c.json response — update the handler body in the webhooksRoute.get
callback accordingly.
In `@packages/core/src/functions/local-runtime.ts`:
- Around line 80-113: Validate the incoming name inside loadFunction before
constructing functionPath: reject names containing path separators or traversal
patterns (e.g., '/', '\', '..') and enforce a safe whitelist regex such as
/^[A-Za-z0-9_-]+$/; if validation fails, throw a clear Error (e.g., "Invalid
function name") so functionsDir + name cannot be abused, and keep the rest of
loadFunction (statSync, import, caching) unchanged.
- Around line 171-182: The watcher callback can mis-extract the function name
for files at the functionsDir root and on platforms with different path
separators; update the callback in the watch handler to (1) normalize the
incoming filename separators (e.g. filename.replace(/[/\\]/g, path.sep)), (2)
compute the path relative to this.functionsDir using
path.relative(this.functionsDir, path.resolve(this.functionsDir,
normalizedFilename)) to ensure we get a consistent relative path, (3) split that
relative path on path.sep and pick the first non-empty segment (fall back to
path.basename if necessary) as the functionName, and (4) keep the existing guard
(functionName && functionName !== "functions") before calling
this.functions.delete(functionName) so root-level files and cross-platform
separators are handled safely.
In `@scripts/test-summary.ts`:
- Around line 1-58: This file duplicates the test-summary parsing/printing logic
(the Bun spawn and regex parsing using proc, fullOutput,
passMatch/failMatch/testsMatch and the final exit handling), so either remove
this standalone script and call the same logic from package.json (e.g. replace
the inline shell summary with "bun scripts/test-summary.ts") or consolidate into
a single shared module used by both locations; update package.json to point to
the script if keeping it, or delete the script and remove the duplicate
regex/summary code from package.json if preferring the inline approach, ensuring
proc exit handling and regex patterns (/\d+ pass/g, /\d+ fail/g, /Ran (\d+)
tests?/g) remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 150cca6a-2035-4419-8353-7397815df3b8
📒 Files selected for processing (7)
apps/test-project/src/index.tsapps/test-project/src/routes/webhooks.tspackage.jsonpackages/cli/src/commands/rls-test.tspackages/client/src/realtime.tspackages/core/src/functions/local-runtime.tsscripts/test-summary.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/commands/rls-test.ts
| import { upgradeWebSocket, websocket } from "hono/bun"; | ||
| import config from "../betterbase.config"; | ||
| import { auth } from "./auth"; | ||
| import { db } from "./db"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if db is actually used in this file
rg -n '\bdb\b' apps/test-project/src/index.ts | grep -v "import.*db" | grep -v "dbAdapter" | grep -v "dbEventEmitter"Repository: weroperking/Betterbase
Length of output: 48
🏁 Script executed:
cat -n apps/test-project/src/index.tsRepository: weroperking/Betterbase
Length of output: 5788
Remove the unused db import.
The db import on line 9 is not used anywhere in this file and should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/test-project/src/index.ts` at line 9, Remove the unused import of db
from the top of the file: locate the import statement "import { db } from
\"./db\";" and delete it (or remove db from the import list if other imports are
present) so the module no longer imports an unused symbol named db.
| "files": [".", "!node_modules", "!.git"], | ||
| "scripts": { | ||
| "test": "bunx turbo run test", | ||
| "test": "bunx turbo run test 2>&1 | tee /tmp/test.log; echo ''; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; echo '📋 TEST SUMMARY'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; grep -oP '\\d+ pass' /tmp/test.log | awk '{sum+=$1} END {print \"✅ Passed: \" sum}'; grep -oP '\\d+ fail' /tmp/test.log | awk '{sum+=$1} END {print \"❌ Failed: \" sum}'; grep -oP '\\d+ skip' /tmp/test.log | awk '{sum+=$1} END {if (sum>0) print \"⏭️ Skipped: \" sum}'; grep -oP 'Ran \\d+ tests?' /tmp/test.log | grep -oP '\\d+' | awk '{sum+=$1} END {print \"📝 Total Tests: \" sum}'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'", |
There was a problem hiding this comment.
grep -oP is not portable to macOS.
The -P flag (Perl regex) is GNU grep-specific and unavailable on macOS's BSD grep. Developers on macOS will see grep: invalid option -- P. Use -oE (extended regex) with [0-9]+ instead of \d+ for cross-platform compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 46, Update the "test" npm script in package.json (the
"test" script string) to avoid GNU-specific grep -P usage: replace all grep -oP
'\\d+' patterns with grep -oE '[0-9]+' and change instances like '\\d+ pass' to
'[0-9]+ pass', '\\d+ fail' to '[0-9]+ fail', '\\d+ skip' to '[0-9]+ skip', and
'Ran \\d+ tests?' to 'Ran [0-9]+ tests?' so macOS/BSD grep (using -oE) works
cross-platform.
Test failures are not propagated to the exit code.
The script uses semicolons to chain commands, so the exit code is always from the last echo command (0), regardless of whether tests passed. This means CI pipelines won't detect test failures.
🔧 Proposed fix: capture and propagate exit code
-"test": "bunx turbo run test 2>&1 | tee /tmp/test.log; echo ''; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; echo '📋 TEST SUMMARY'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; grep -oP '\\d+ pass' /tmp/test.log | awk '{sum+=$1} END {print \"✅ Passed: \" sum}'; grep -oP '\\d+ fail' /tmp/test.log | awk '{sum+=$1} END {print \"❌ Failed: \" sum}'; grep -oP '\\d+ skip' /tmp/test.log | awk '{sum+=$1} END {if (sum>0) print \"⏭️ Skipped: \" sum}'; grep -oP 'Ran \\d+ tests?' /tmp/test.log | grep -oP '\\d+' | awk '{sum+=$1} END {print \"📝 Total Tests: \" sum}'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'",
+"test": "bunx turbo run test 2>&1 | tee /tmp/test.log; EXIT_CODE=${PIPESTATUS[0]}; echo ''; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; echo '📋 TEST SUMMARY'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; grep -oE '[0-9]+ pass' /tmp/test.log | awk '{sum+=$1} END {print \"✅ Passed: \" sum}'; grep -oE '[0-9]+ fail' /tmp/test.log | awk '{sum+=$1} END {print \"❌ Failed: \" sum}'; grep -oE '[0-9]+ skip' /tmp/test.log | awk '{sum+=$1} END {if (sum>0) print \"⏭️ Skipped: \" sum}'; grep -oE 'Ran [0-9]+ tests?' /tmp/test.log | grep -oE '[0-9]+' | awk '{sum+=$1} END {print \"📝 Total Tests: \" sum}'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; exit $EXIT_CODE",Note: This also switches from grep -oP (GNU-only) to grep -oE (POSIX extended regex) for better portability.
📝 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
| "test": "bunx turbo run test 2>&1 | tee /tmp/test.log; echo ''; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; echo '📋 TEST SUMMARY'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; grep -oP '\\d+ pass' /tmp/test.log | awk '{sum+=$1} END {print \"✅ Passed: \" sum}'; grep -oP '\\d+ fail' /tmp/test.log | awk '{sum+=$1} END {print \"❌ Failed: \" sum}'; grep -oP '\\d+ skip' /tmp/test.log | awk '{sum+=$1} END {if (sum>0) print \"⏭️ Skipped: \" sum}'; grep -oP 'Ran \\d+ tests?' /tmp/test.log | grep -oP '\\d+' | awk '{sum+=$1} END {print \"📝 Total Tests: \" sum}'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'", | |
| "test": "bunx turbo run test 2>&1 | tee /tmp/test.log; EXIT_CODE=${PIPESTATUS[0]}; echo ''; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; echo '📋 TEST SUMMARY'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; grep -oE '[0-9]+ pass' /tmp/test.log | awk '{sum+=$1} END {print \"✅ Passed: \" sum}'; grep -oE '[0-9]+ fail' /tmp/test.log | awk '{sum+=$1} END {print \"❌ Failed: \" sum}'; grep -oE '[0-9]+ skip' /tmp/test.log | awk '{sum+=$1} END {if (sum>0) print \"⏭️ Skipped: \" sum}'; grep -oE 'Ran [0-9]+ tests?' /tmp/test.log | grep -oE '[0-9]+' | awk '{sum+=$1} END {print \"📝 Total Tests: \" sum}'; echo '━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'; exit $EXIT_CODE", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 46, The "test" npm script in package.json currently
chains commands with semicolons so test failures are lost; update the "test"
script to capture the exit code from the test runner (the bunx turbo run test
invocation) into a variable (or use &&/|| logic) before running the summary
commands and then exit with that captured code so CI sees failures, and also
replace GNU-only grep -oP usages with POSIX-friendly grep -oE in the summary
pipeline to improve portability; locate the "test" script string in package.json
to implement these changes.
Comment on lines
+271
to
+283
| channel(channelName: string) { | ||
| // Ensure connection is established | ||
| if (!this.disabled) { | ||
| this.connect(); | ||
| } | ||
|
|
||
| return { | ||
| subscribe: (options?: ChannelSubscribeOptions) => { | ||
| this.send({ | ||
| type: "subscribe", | ||
| channel: channelName, | ||
| payload: options, | ||
| }); |
There was a problem hiding this comment.
Race condition: subscribe message may be silently dropped.
When channel().subscribe() is called, connect() initiates an async WebSocket connection. The immediate send() call on line 279 checks readyState === WebSocket.OPEN, but the socket is likely still CONNECTING. The subscribe message is silently dropped.
Unlike table subscriptions which are re-sent via sendSubscribeAll() in the onopen handler, channel subscriptions have no recovery mechanism. Users will believe they're subscribed but won't receive messages.
Consider tracking channel subscriptions and re-sending them on reconnect, similar to how table subscriptions work.
Proposed approach
export class RealtimeClient {
private ws: WebSocket | null = null;
private subscriptions = new Map<string, Map<string, SubscriberEntry>>();
+ private channelSubscriptions = new Map<string, ChannelSubscribeOptions | undefined>();
private reconnectTimeout: ReturnType<typeof setTimeout> | null = null;Then in onopen, after re-sending table subscriptions:
for (const table of this.subscriptions.keys()) {
this.sendSubscribeAll(table);
}
+
+ for (const [channelName, options] of this.channelSubscriptions) {
+ this.send({ type: "subscribe", channel: channelName, payload: options });
+ }And track/untrack in subscribe/unsubscribe:
subscribe: (options?: ChannelSubscribeOptions) => {
+ this.channelSubscriptions.set(channelName, options);
this.send({
type: "subscribe",
channel: channelName,
payload: options,
}); unsubscribe: () => {
+ this.channelSubscriptions.delete(channelName);
this.send({ type: "unsubscribe", channel: channelName });
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/realtime.ts` around lines 271 - 283, The channel
subscription code can drop subscribe messages because channel().subscribe()
calls connect() then immediately this.send(), which will be ignored if the
socket is still CONNECTING; fix by tracking channel subscriptions and re-sending
them on reconnect like table subscriptions. Modify
channel(channelName).subscribe / unsubscribe to add/remove channelName (and
options if needed) to a tracked map or Set (e.g., trackedChannelSubscriptions)
and call this.sendSubscribeAll-style logic from the socket onopen handler to
iterate and re-send all tracked channel subscribe messages; also ensure
immediate subscribe attempts send if socket.readyState === WebSocket.OPEN,
otherwise rely on the tracked set so the onopen replay will deliver the
subscribe. Ensure unsubscribe removes from the tracked set and sends an
unsubscribe if socket is open.
Comment on lines
+306
to
+322
| onPresence: (callback: (event: PresenceEvent) => void) => { | ||
| this.on("presence", (data) => { | ||
| const event = data as PresenceEvent; | ||
| if (event.channel === channelName) { | ||
| callback(event); | ||
| } | ||
| }); | ||
| }, | ||
|
|
||
| onBroadcast: (callback: (event: string, data: unknown) => void) => { | ||
| this.on("broadcast", (data) => { | ||
| const event = data as BroadcastEvent; | ||
| if (event.channel === channelName) { | ||
| callback(event.event, event.payload); | ||
| } | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Memory leak: onPresence and onBroadcast handlers cannot be removed.
These methods call this.on() to register handlers but don't return a cleanup function. Handlers accumulate across the lifecycle and continue firing even after unsubscribe() is called. Additionally, calling onPresence/onBroadcast multiple times adds duplicate handlers.
Return an unsubscribe function to allow cleanup.
Proposed fix
- onPresence: (callback: (event: PresenceEvent) => void) => {
- this.on("presence", (data) => {
- const event = data as PresenceEvent;
- if (event.channel === channelName) {
- callback(event);
- }
- });
- },
+ onPresence: (callback: (event: PresenceEvent) => void) => {
+ const handler = (data: unknown) => {
+ const event = data as PresenceEvent;
+ if (event.channel === channelName) {
+ callback(event);
+ }
+ };
+ this.on("presence", handler);
+ return () => this.off("presence", handler);
+ },
- onBroadcast: (callback: (event: string, data: unknown) => void) => {
- this.on("broadcast", (data) => {
- const event = data as BroadcastEvent;
- if (event.channel === channelName) {
- callback(event.event, event.payload);
- }
- });
- },
+ onBroadcast: (callback: (event: string, data: unknown) => void) => {
+ const handler = (data: unknown) => {
+ const event = data as BroadcastEvent;
+ if (event.channel === channelName) {
+ callback(event.event, event.payload);
+ }
+ };
+ this.on("broadcast", handler);
+ return () => this.off("broadcast", handler);
+ },📝 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
| onPresence: (callback: (event: PresenceEvent) => void) => { | |
| this.on("presence", (data) => { | |
| const event = data as PresenceEvent; | |
| if (event.channel === channelName) { | |
| callback(event); | |
| } | |
| }); | |
| }, | |
| onBroadcast: (callback: (event: string, data: unknown) => void) => { | |
| this.on("broadcast", (data) => { | |
| const event = data as BroadcastEvent; | |
| if (event.channel === channelName) { | |
| callback(event.event, event.payload); | |
| } | |
| }); | |
| }, | |
| onPresence: (callback: (event: PresenceEvent) => void) => { | |
| const handler = (data: unknown) => { | |
| const event = data as PresenceEvent; | |
| if (event.channel === channelName) { | |
| callback(event); | |
| } | |
| }; | |
| this.on("presence", handler); | |
| return () => this.off("presence", handler); | |
| }, | |
| onBroadcast: (callback: (event: string, data: unknown) => void) => { | |
| const handler = (data: unknown) => { | |
| const event = data as BroadcastEvent; | |
| if (event.channel === channelName) { | |
| callback(event.event, event.payload); | |
| } | |
| }; | |
| this.on("broadcast", handler); | |
| return () => this.off("broadcast", handler); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/realtime.ts` around lines 306 - 322, The onPresence and
onBroadcast methods currently register anonymous listeners with this.on and
never remove them; change each to create a named handler (e.g., presenceHandler
/ broadcastHandler) that casts the incoming data to
PresenceEvent/BroadcastEvent, register it via this.on("presence",
presenceHandler) / this.on("broadcast", broadcastHandler), and return an
unsubscribe function that calls this.off("presence", presenceHandler) /
this.off("broadcast", broadcastHandler). Ensure the handler still checks
event.channel === channelName before invoking the callback so behavior is
unchanged.
|
|
||
| /** | ||
| * Remove an event handler | ||
| */ off(eventType: string, callback: (data: unknown) => void): void { |
There was a problem hiding this comment.
Formatting: JSDoc closing and function signature on same line.
The closing */ and function declaration should be on separate lines for readability and consistency.
Proposed fix
- */ off(eventType: string, callback: (data: unknown) => void): void {
+ */
+ off(eventType: string, callback: (data: unknown) => void): void {📝 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
| */ off(eventType: string, callback: (data: unknown) => void): void { | |
| */ | |
| off(eventType: string, callback: (data: unknown) => void): void { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/realtime.ts` at line 342, The JSDoc closing token is on
the same line as the method declaration for off(eventType: string, callback:
(data: unknown) => void): void; move the closing comment delimiter (*/) to its
own line immediately above the method signature to separate the JSDoc block from
the function declaration and restore consistent formatting in the off method in
packages/client/src/realtime.ts.
Comment on lines
+147
to
+158
| try { | ||
| return await func.handler(ctx); | ||
| } catch (error) { | ||
| logger.error({ msg: `Function execution error`, function: name, error }); | ||
| return new Response( | ||
| JSON.stringify({ | ||
| error: "Internal Server Error", | ||
| message: error instanceof Error ? error.message : String(error), | ||
| }), | ||
| { status: 500, headers: { "Content-Type": "application/json" } }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handler errors expose internal message to clients.
The error response includes error.message which may leak internal details (stack traces, file paths, etc.) in the response body.
🔧 Suggested fix for production safety
} catch (error) {
logger.error({ msg: `Function execution error`, function: name, error });
+ const isDev = process.env.NODE_ENV === "development";
return new Response(
JSON.stringify({
error: "Internal Server Error",
- message: error instanceof Error ? error.message : String(error),
+ ...(isDev && { message: error instanceof Error ? error.message : String(error) }),
}),
{ status: 500, headers: { "Content-Type": "application/json" } },
);
}📝 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
| try { | |
| return await func.handler(ctx); | |
| } catch (error) { | |
| logger.error({ msg: `Function execution error`, function: name, error }); | |
| return new Response( | |
| JSON.stringify({ | |
| error: "Internal Server Error", | |
| message: error instanceof Error ? error.message : String(error), | |
| }), | |
| { status: 500, headers: { "Content-Type": "application/json" } }, | |
| ); | |
| } | |
| try { | |
| return await func.handler(ctx); | |
| } catch (error) { | |
| logger.error({ msg: `Function execution error`, function: name, error }); | |
| const isDev = process.env.NODE_ENV === "development"; | |
| return new Response( | |
| JSON.stringify({ | |
| error: "Internal Server Error", | |
| ...(isDev && { message: error instanceof Error ? error.message : String(error) }), | |
| }), | |
| { status: 500, headers: { "Content-Type": "application/json" } }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/functions/local-runtime.ts` around lines 147 - 158, The
catch block currently returns error.message to clients which can leak internals;
update the catch in localRuntime (around func.handler invocation) to keep
logger.error({ msg: ..., function: name, error }) for full diagnostics but
replace the response body with a generic production-safe message (e.g. "Internal
Server Error" or "An internal error occurred") and avoid including error.message
or stack; optionally gate returning the actual error text behind a debug/dev
flag (NODE_ENV !== 'production') if you need verboseness in development.
Comment on lines
+259
to
+262
| export async function initializeFunctionsRuntime( | ||
| projectRoot: string, | ||
| envVars: Record<string, string> = process.env as Record<string, string>, | ||
| ): Promise<LocalFunctionsRuntime | null> { |
There was a problem hiding this comment.
Default envVars exposes entire process.env to functions.
Defaulting to process.env may inadvertently pass sensitive environment variables (secrets, API keys, internal configs) to user-defined functions.
🔧 Suggested fix - require explicit env allowlist
export async function initializeFunctionsRuntime(
projectRoot: string,
- envVars: Record<string, string> = process.env as Record<string, string>,
+ envVars: Record<string, string> = {},
): Promise<LocalFunctionsRuntime | null> {Consider requiring callers to explicitly pass a filtered subset of environment variables, or implement an allowlist pattern.
📝 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
| export async function initializeFunctionsRuntime( | |
| projectRoot: string, | |
| envVars: Record<string, string> = process.env as Record<string, string>, | |
| ): Promise<LocalFunctionsRuntime | null> { | |
| export async function initializeFunctionsRuntime( | |
| projectRoot: string, | |
| envVars: Record<string, string> = {}, | |
| ): Promise<LocalFunctionsRuntime | null> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/functions/local-runtime.ts` around lines 259 - 262, The
function initializeFunctionsRuntime currently defaults envVars to process.env
which can leak secrets; change the signature and callers so envVars is required
(remove the default) or accept an explicit allowlist parameter and filter
process.env before use; update references to initializeFunctionsRuntime and any
call sites to pass a controlled Record<string,string> (or an allowlist array)
and implement the filtering logic inside initializeFunctionsRuntime (or a helper
like filterEnvForFunctions) to only include allowed keys.
Comment on lines
+3
to
+11
| const proc = Bun.spawn(["bunx", "turbo", "run", "test"]); | ||
|
|
||
| await proc.exited; | ||
|
|
||
| const output = await new Response(proc.stdout).text(); | ||
| const errorOutput = await new Response(proc.stderr).text(); | ||
|
|
||
| process.stdout.write(output); | ||
| process.stderr.write(errorOutput); |
There was a problem hiding this comment.
Potential deadlock: awaiting process exit before consuming stdout/stderr.
Bun.spawn defaults to piped stdio. If the child produces more output than the pipe buffer (typically 64KB), it will block waiting for the parent to read. Since the parent awaits proc.exited before reading, this creates a deadlock when output is large.
Either stream output while the process runs, or use stdio: "inherit" to forward directly (though you'd lose the ability to parse output).
🔧 Proposed fix: stream output incrementally
-const proc = Bun.spawn(["bunx", "turbo", "run", "test"]);
+const proc = Bun.spawn(["bunx", "turbo", "run", "test"], {
+ stdout: "pipe",
+ stderr: "pipe",
+});
-await proc.exited;
+// Read streams concurrently while process runs to avoid deadlock
+const [output, errorOutput] = await Promise.all([
+ new Response(proc.stdout).text(),
+ new Response(proc.stderr).text(),
+]);
-const output = await new Response(proc.stdout).text();
-const errorOutput = await new Response(proc.stderr).text();
+await proc.exited;
process.stdout.write(output);
process.stderr.write(errorOutput);📝 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
| const proc = Bun.spawn(["bunx", "turbo", "run", "test"]); | |
| await proc.exited; | |
| const output = await new Response(proc.stdout).text(); | |
| const errorOutput = await new Response(proc.stderr).text(); | |
| process.stdout.write(output); | |
| process.stderr.write(errorOutput); | |
| const proc = Bun.spawn(["bunx", "turbo", "run", "test"], { | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| // Read streams concurrently while process runs to avoid deadlock | |
| const [output, errorOutput] = await Promise.all([ | |
| new Response(proc.stdout).text(), | |
| new Response(proc.stderr).text(), | |
| ]); | |
| await proc.exited; | |
| process.stdout.write(output); | |
| process.stderr.write(errorOutput); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-summary.ts` around lines 3 - 11, The current code awaits
proc.exited before consuming proc.stdout/proc.stderr, which can deadlock if the
child fills the pipe buffer; change to consume streams while the process runs
(or set Bun.spawn(..., { stdio: "inherit" }) if you don't need to parse output).
Specifically, when calling Bun.spawn(...) (symbol: proc) either 1) start
streaming proc.stdout and proc.stderr to process.stdout/process.stderr using
async iteration or stream piping immediately after spawn and only await
proc.exited after those consumers are started, or 2) pass the option stdio:
"inherit" to Bun.spawn to forward output directly; ensure any parsing logic
adapts if you choose inherit.
Comment on lines
+55
to
+58
| if (totalFail > 0 || proc.exitCode !== 0) { | ||
| process.exit(1); | ||
| } | ||
| console.log("━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"); |
There was a problem hiding this comment.
Closing separator not printed on failure.
When tests fail, process.exit(1) on line 56 prevents the closing separator on line 58 from printing. This results in inconsistent output formatting between success and failure cases.
🛠️ Proposed fix
+console.log("━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━");
+
if (totalFail > 0 || proc.exitCode !== 0) {
process.exit(1);
}
-console.log("━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━");📝 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
| if (totalFail > 0 || proc.exitCode !== 0) { | |
| process.exit(1); | |
| } | |
| console.log("━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"); | |
| console.log("━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"); | |
| if (totalFail > 0 || proc.exitCode !== 0) { | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-summary.ts` around lines 55 - 58, The closing separator
console.log call is skipped when tests fail because process.exit(1) is called
before it; modify the control flow around totalFail and proc.exitCode so the
separator is printed regardless of failure — e.g., call
console.log("━━━━━━━━...") before invoking process.exit(1) or ensure a single
exit path prints the separator then exits; update the logic that checks
totalFail and proc.exitCode (the block that currently calls process.exit) so
console.log always runs prior to process.exit.
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
Commits (23 total)
This PR consolidates 23 categorized commits covering:
Type of Change
Pre-flight Checklist
Summary by CodeRabbit
New Features
Documentation
Chores