Open
Conversation
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughImplements project-type-aware daemon initialization supporting FastStore, Fresh, and Hono projects. Introduces Node.js-based worker isolation capability, refines file system metadata inference and ignore patterns, adds normalized since-timestamp handling to sync/SSE endpoints, and conditionally configures workers based on detected project type. Changes
Sequence DiagramsequenceDiagram
participant App as App Startup
participant Detect as detectProjectType()
participant CreateDeps as createDeps(type)
participant FastStore as FastStoreWorker
participant Worker as Worker Route
App->>Detect: Detect project type from package.json
Detect-->>App: Returns "faststore" | "fresh" | "hono"
App->>CreateDeps: Initialize with ProjectType
CreateDeps-->>App: Middleware configured
alt PROJECT_TYPE == "faststore"
App->>FastStore: createFastStoreWorkerOptions()
FastStore->>FastStore: Detect package manager & Node version
FastStore-->>App: WorkerOptions configured
App->>Worker: Register worker route with options
Worker-->>App: Route established
else PROJECT_TYPE == "fresh"
App->>App: Skip custom worker, use defaults
end
App-->>App: Initialization complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/fast-store.code-workspace">
<violation number="1" location="runtime/fast-store.code-workspace:7">
P2: This committed workspace file includes a machine-specific external folder path, which makes the workspace non-portable for other developers. Keep shared `.code-workspace` files repo-relative only, or remove local-only folders.</violation>
</file>
<file name="daemon/worker.ts">
<violation number="1" location="daemon/worker.ts:49">
P2: `Deno.Command` instances don’t expose a public `command` property, so `commandName` will always be `undefined` and the Node.js branch won’t activate. This means Node-based projects will still use `DenoRun` instead of `NodeRun`. Pass the command name (or package manager) alongside the `Deno.Command` in `WorkerOptions`, or change the API to accept a richer command descriptor you can inspect.</violation>
</file>
<file name="runtime/mod.ts">
<violation number="1" location="runtime/mod.ts:82">
P2: Remove the unconditional `console.log` from runtime initialization to avoid leaking configuration details and noisy production logs.</violation>
</file>
<file name="daemon/fs/api.ts">
<violation number="1" location="daemon/fs/api.ts:20">
P1: The early return makes the rest of inferMetadata unreachable, so block metadata is never inferred. Remove the unconditional return so the try/catch executes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| import { grep, type GrepResult } from "./grep.ts"; | ||
|
|
||
| const inferMetadata = async (filepath: string): Promise<Metadata | null> => { | ||
| return { kind: "file" }; |
There was a problem hiding this comment.
P1: The early return makes the rest of inferMetadata unreachable, so block metadata is never inferred. Remove the unconditional return so the try/catch executes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/fs/api.ts, line 20:
<comment>The early return makes the rest of inferMetadata unreachable, so block metadata is never inferred. Remove the unconditional return so the try/catch executes.</comment>
<file context>
@@ -17,11 +17,18 @@ import {
import { grep, type GrepResult } from "./grep.ts";
const inferMetadata = async (filepath: string): Promise<Metadata | null> => {
+ return { kind: "file" };
+
try {
</file context>
| "path": ".." | ||
| }, | ||
| { | ||
| "path": "../../faststore-demoaccount20" |
There was a problem hiding this comment.
P2: This committed workspace file includes a machine-specific external folder path, which makes the workspace non-portable for other developers. Keep shared .code-workspace files repo-relative only, or remove local-only folders.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/fast-store.code-workspace, line 7:
<comment>This committed workspace file includes a machine-specific external folder path, which makes the workspace non-portable for other developers. Keep shared `.code-workspace` files repo-relative only, or remove local-only folders.</comment>
<file context>
@@ -0,0 +1,11 @@
+ "path": ".."
+ },
+ {
+ "path": "../../faststore-demoaccount20"
+ }
+ ],
</file context>
|
|
||
| const createIsolate = (options: WorkerOptions): Isolate => { | ||
| // Detect worker type based on command | ||
| const commandName = (options.command as any).command; |
There was a problem hiding this comment.
P2: Deno.Command instances don’t expose a public command property, so commandName will always be undefined and the Node.js branch won’t activate. This means Node-based projects will still use DenoRun instead of NodeRun. Pass the command name (or package manager) alongside the Deno.Command in WorkerOptions, or change the API to accept a richer command descriptor you can inspect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/worker.ts, line 49:
<comment>`Deno.Command` instances don’t expose a public `command` property, so `commandName` will always be `undefined` and the Node.js branch won’t activate. This means Node-based projects will still use `DenoRun` instead of `NodeRun`. Pass the command name (or package manager) alongside the `Deno.Command` in `WorkerOptions`, or change the API to accept a richer command descriptor you can inspect.</comment>
<file context>
@@ -31,17 +33,30 @@ export const start = (): WorkerStatusEvent => ({
+const createIsolate = (options: WorkerOptions): Isolate => {
+ // Detect worker type based on command
+ const commandName = (options.command as any).command;
+
+ // Check if it's a Node.js package manager command
</file context>
| static async init<TAppManifest extends AppManifest = AppManifest>( | ||
| opts?: DecoOptions<TAppManifest>, | ||
| ): Promise<Deco<TAppManifest>> { | ||
| console.log("Deco.init", opts) |
There was a problem hiding this comment.
P2: Remove the unconditional console.log from runtime initialization to avoid leaking configuration details and noisy production logs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/mod.ts, line 82:
<comment>Remove the unconditional `console.log` from runtime initialization to avoid leaking configuration details and noisy production logs.</comment>
<file context>
@@ -79,6 +79,7 @@ export class Deco<TAppManifest extends AppManifest = AppManifest> {
static async init<TAppManifest extends AppManifest = AppManifest>(
opts?: DecoOptions<TAppManifest>,
): Promise<Deco<TAppManifest>> {
+ console.log("Deco.init", opts)
const site = opts?.site ?? siteNameFromEnv() ?? randomSiteName();
const decofile = opts?.decofile ?? await getProvider();
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
runtime/fast-store.code-workspace (1)
2-8: Avoid committing workspace roots that depend on sibling checkouts.Line 7 points to
../../faststore-demoaccount20, which is likely local-repo specific and creates noisy missing-folder warnings for contributors without that layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/fast-store.code-workspace` around lines 2 - 8, The workspace file currently lists a sibling checkout as a folder entry ("folders" array containing the object with "path": "../../faststore-demoaccount20"), which causes missing-folder warnings for other contributors; remove that sibling path entry or replace it with a neutral/example placeholder (e.g., "path": "demoaccount20-example" or comment) so the workspace only references repo-relative folders that exist for all contributors and does not rely on local sibling checkouts.daemon/sse/api.ts (1)
22-22: Gate/watchquery logging behind verbosity.Line 22 logs on every SSE connection; this can flood logs under normal admin usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daemon/sse/api.ts` at line 22, The unconditional console.log call that prints "[SSE /watch] Query params:" with sinceParam, since, normalizedSince should be gated behind a verbosity/ debug check to avoid flooding logs; update the code in the /watch handler to replace the direct console.log with a conditional or logger.debug call (e.g., if (process.env.LOG_LEVEL === 'debug' || processLogger.isDebugEnabled()) processLogger.debug(...)) or similar, referencing the existing variables sinceParam, since, normalizedSince and the console.log invocation so only verbose/debug logging emits those values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daemon/fs/api.ts`:
- Around line 161-168: The per-file console.log calls (e.g.,
console.log("entry", entry) and the metadata/mtime logs produced around the
inferMetadata(entry.path) and mtimeFor(entry.path) calls) must be gated behind a
verbose/debug flag instead of being unconditionally emitted; update these spots
to either use a logger.debug(...) or wrap them in a conditional like if
(process.env.VERBOSE) { ... } (or the project's logger.isDebug()/logger.debug)
so the logs only run when verbose mode is enabled; apply the same change to the
other occurrences that log per-file metadata/mtime as mentioned in the review.
- Around line 19-21: The function inferMetadata currently short-circuits by
immediately returning { kind: "file" }, skipping the real inference logic;
update inferMetadata to remove that unconditional return and re-enable the
file-parsing/block-detection path: read the file at filepath, run the existing
parsing/inference routines that detect blocks and produce Metadata (preserving
block-aware metadata propagation), handle I/O or parse errors (return null on
unrecoverable errors), and ensure the returned value conforms to the Metadata
type; use the inferMetadata function name to locate the stub and wire it back
into the calling flow.
In `@daemon/main.ts`:
- Around line 124-134: detectProjectType currently only inspects
packageJson.dependencies for "@faststore/cli" so projects that list the CLI in
devDependencies or peerDependencies are misdetected; update the detection in
detectProjectType to read packageJson (using packageJsonPath) and check
packageJson.dependencies, packageJson.devDependencies and
packageJson.peerDependencies for "@faststore/cli" (and any equivalent FastStore
package names you consider necessary) and return "faststore" if found, otherwise
fall back to the existing "fresh" default.
In `@daemon/worker.ts`:
- Around line 47-53: The code currently reads an internal property
(options.command as any).command inside createIsolate to detect package
managers; stop relying on Deno.Command internals and instead add explicit
runtime metadata to WorkerOptions (e.g., a runtime: 'node' | 'deno' |
'package-manager' or a packageManager flag). Update createIsolate to branch on
options.runtime (or options.packageManager) rather than options.command, and
construct the NodeRun isolate with new NodeRun({ command: options.command, port:
options.port }) when the runtime indicates Node/package-manager; remove usage of
(options.command as any).command entirely.
In `@daemon/workers/nodeRun.ts`:
- Around line 43-49: The cleanup currently always returns this.port to portPool
even when the port was provided externally via CommandIsolate; modify the
NodeRun constructor to set a boolean flag (e.g., this.portAllocated) to true
only when you call portPool.get() in the non-command branch and false when
isCmdIsolate(options) supplies the port, then in the shutdown/cleanup method
(where portPool.free(this.port) is called) guard that call with if
(this.portAllocated) to only free ports you actually allocated from portPool.
- Around line 10-12: PORT_WAIT_TIMEOUT currently uses PORT_WAIT_TIMEOUT_STR
directly which can produce NaN; update the initializer for PORT_WAIT_TIMEOUT to
coerce and validate the input (e.g., const parsed =
Number(PORT_WAIT_TIMEOUT_STR)) and only use parsed if Number.isFinite(parsed)
and parsed > 0, otherwise fall back to the default 120_000; make this change
where PORT_WAIT_TIMEOUT is defined and consider emitting a warning via the
existing logger if the env value is invalid.
- Around line 119-127: fetch increments this.inflightRequests but returns early
for WebSocket paths via proxyWebSocket(url, request) without ever decrementing;
update the WebSocket branch so that inflightRequests is decremented when the
socket closes/errors—either by modifying proxyWebSocket to accept a completion
callback or wrapping its returned promise/connection to call
this.inflightRequests-- on close/error, ensuring the decrement happens exactly
once and also on failure so the graceful shutdown that checks inflightRequests
(used by the shutdown logic) can succeed.
In `@engine/manifest/manifestGen.ts`:
- Line 83: The computed blockDir (const blockDir = join(dir, blk.type ===
"sections" ? "src/components/sections" : blk.type)) forces the new FastStore
path and drops the legacy "sections" path; change the resolution to try the new
path first and fall back to the legacy "sections" path when the new path does
not exist: compute the candidate path for blk.type === "sections" as join(dir,
"src/components/sections"), check existence (e.g., fs.existsSync or
fs.promises.stat), and if missing use join(dir, "sections") instead (keep the
same behavior for non-"sections" types), updating the blockDir assignment and
any callers that rely on it (reference symbols: blockDir, blk.type, join).
In `@runtime/mod.ts`:
- Line 82: Remove the unguarded console.log("Deco.init", opts) that prints the
full init options (opts) to avoid leaking sensitive data; either delete that
call or wrap it behind a debug flag (e.g., if (DEBUG) console.log(...)) or
sanitize the object (only log non-sensitive fields) so only safe fields are
emitted; update the init/Deco initialization code that references opts to use
the guarded logging path and ensure bindings or credential-like fields are
omitted from any logs.
---
Nitpick comments:
In `@daemon/sse/api.ts`:
- Line 22: The unconditional console.log call that prints "[SSE /watch] Query
params:" with sinceParam, since, normalizedSince should be gated behind a
verbosity/ debug check to avoid flooding logs; update the code in the /watch
handler to replace the direct console.log with a conditional or logger.debug
call (e.g., if (process.env.LOG_LEVEL === 'debug' ||
processLogger.isDebugEnabled()) processLogger.debug(...)) or similar,
referencing the existing variables sinceParam, since, normalizedSince and the
console.log invocation so only verbose/debug logging emits those values.
In `@runtime/fast-store.code-workspace`:
- Around line 2-8: The workspace file currently lists a sibling checkout as a
folder entry ("folders" array containing the object with "path":
"../../faststore-demoaccount20"), which causes missing-folder warnings for other
contributors; remove that sibling path entry or replace it with a
neutral/example placeholder (e.g., "path": "demoaccount20-example" or comment)
so the workspace only references repo-relative folders that exist for all
contributors and does not rely on local sibling checkouts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
daemon/fs/api.tsdaemon/main.tsdaemon/sse/api.tsdaemon/worker.tsdaemon/workers/nodeRun.tsengine/manifest/manifestGen.tsruntime/fast-store.code-workspaceruntime/mod.ts
Comment on lines
19
to
+21
| const inferMetadata = async (filepath: string): Promise<Metadata | null> => { | ||
| return { kind: "file" }; | ||
|
|
There was a problem hiding this comment.
inferMetadata is currently short-circuited and no longer infers blocks.
Line 20 unconditionally returns { kind: "file" }, making the parsing/inference logic unreachable and breaking block-aware metadata propagation.
Proposed fix
const inferMetadata = async (filepath: string): Promise<Metadata | null> => {
- return { kind: "file" };
-
try {📝 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 inferMetadata = async (filepath: string): Promise<Metadata | null> => { | |
| return { kind: "file" }; | |
| const inferMetadata = async (filepath: string): Promise<Metadata | null> => { | |
| try { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/fs/api.ts` around lines 19 - 21, The function inferMetadata currently
short-circuits by immediately returning { kind: "file" }, skipping the real
inference logic; update inferMetadata to remove that unconditional return and
re-enable the file-parsing/block-detection path: read the file at filepath, run
the existing parsing/inference routines that detect blocks and produce Metadata
(preserving block-aware metadata propagation), handle I/O or parse errors
(return null on unrecoverable errors), and ensure the returned value conforms to
the Metadata type; use the inferMetadata function name to locate the stub and
wire it back into the calling flow.
Comment on lines
+161
to
+168
| console.log("entry", entry) | ||
|
|
||
| const [metadata, mtime] = await Promise.all([ | ||
| inferMetadata(entry.path), | ||
| mtimeFor(entry.path), | ||
| ]); | ||
| console.log("metadata", metadata); | ||
| console.log("mtime", mtime) |
There was a problem hiding this comment.
Remove or gate hot-loop debug logs in filesystem scan.
Lines 161/167/168 log per file and will heavily impact large repos; this should be behind VERBOSE (or debug level) only.
Proposed fix (verbose-gated logs)
- console.log("entry", entry)
+ if (VERBOSE) console.log("[watchFS.start] entry", entry.path);
@@
- console.log("metadata", metadata);
- console.log("mtime", mtime)
+ if (VERBOSE) {
+ console.log("[watchFS.start] metadata/mtime", { path: entry.path, metadata, mtime });
+ }
@@
- console.log(`[watchFS.start] File walk complete! Processed: ${processedCount}, Sent: ${fileCount}, Skipped: ${skippedCount}`);
- console.log("[watchFS.start] Sending fs-snapshot event...");
+ if (VERBOSE) {
+ console.log(`[watchFS.start] File walk complete! Processed: ${processedCount}, Sent: ${fileCount}, Skipped: ${skippedCount}`);
+ console.log("[watchFS.start] Sending fs-snapshot event...");
+ }
@@
- console.log("[watchFS.start] ✅ fs-snapshot event sent successfully");
+ if (VERBOSE) console.log("[watchFS.start] ✅ fs-snapshot event sent successfully");Also applies to: 191-193, 199-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/fs/api.ts` around lines 161 - 168, The per-file console.log calls
(e.g., console.log("entry", entry) and the metadata/mtime logs produced around
the inferMetadata(entry.path) and mtimeFor(entry.path) calls) must be gated
behind a verbose/debug flag instead of being unconditionally emitted; update
these spots to either use a logger.debug(...) or wrap them in a conditional like
if (process.env.VERBOSE) { ... } (or the project's
logger.isDebug()/logger.debug) so the logs only run when verbose mode is
enabled; apply the same change to the other occurrences that log per-file
metadata/mtime as mentioned in the review.
Comment on lines
+124
to
+134
| const detectProjectType = async (): Promise<ProjectType> => { | ||
| try { | ||
| const packageJsonPath = join(Deno.cwd(), "package.json"); | ||
| if (await exists(packageJsonPath)) { | ||
| const packageJson = JSON.parse(await Deno.readTextFile(packageJsonPath)); | ||
| if (packageJson.dependencies?.["@faststore/cli"]) { | ||
| return "faststore"; | ||
| } | ||
| // Could add more Node.js framework detection here (e.g., Hono) | ||
| } | ||
| return "fresh"; // Default to Fresh for Deno projects |
There was a problem hiding this comment.
Project-type detection is too narrow for FastStore installs.
Line 129 only checks dependencies["@faststore/cli"]. If FastStore CLI is in devDependencies (common), the project is misdetected as Fresh and the FastStore worker path is skipped.
Proposed fix
const detectProjectType = async (): Promise<ProjectType> => {
try {
const packageJsonPath = join(Deno.cwd(), "package.json");
if (await exists(packageJsonPath)) {
const packageJson = JSON.parse(await Deno.readTextFile(packageJsonPath));
- if (packageJson.dependencies?.["@faststore/cli"]) {
+ const deps = {
+ ...packageJson.dependencies,
+ ...packageJson.devDependencies,
+ };
+ if (deps?.["@faststore/cli"]) {
return "faststore";
}📝 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 detectProjectType = async (): Promise<ProjectType> => { | |
| try { | |
| const packageJsonPath = join(Deno.cwd(), "package.json"); | |
| if (await exists(packageJsonPath)) { | |
| const packageJson = JSON.parse(await Deno.readTextFile(packageJsonPath)); | |
| if (packageJson.dependencies?.["@faststore/cli"]) { | |
| return "faststore"; | |
| } | |
| // Could add more Node.js framework detection here (e.g., Hono) | |
| } | |
| return "fresh"; // Default to Fresh for Deno projects | |
| const detectProjectType = async (): Promise<ProjectType> => { | |
| try { | |
| const packageJsonPath = join(Deno.cwd(), "package.json"); | |
| if (await exists(packageJsonPath)) { | |
| const packageJson = JSON.parse(await Deno.readTextFile(packageJsonPath)); | |
| const deps = { | |
| ...packageJson.dependencies, | |
| ...packageJson.devDependencies, | |
| }; | |
| if (deps?.["@faststore/cli"]) { | |
| return "faststore"; | |
| } | |
| // Could add more Node.js framework detection here (e.g., Hono) | |
| } | |
| return "fresh"; // Default to Fresh for Deno projects |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/main.ts` around lines 124 - 134, detectProjectType currently only
inspects packageJson.dependencies for "@faststore/cli" so projects that list the
CLI in devDependencies or peerDependencies are misdetected; update the detection
in detectProjectType to read packageJson (using packageJsonPath) and check
packageJson.dependencies, packageJson.devDependencies and
packageJson.peerDependencies for "@faststore/cli" (and any equivalent FastStore
package names you consider necessary) and return "faststore" if found, otherwise
fall back to the existing "fresh" default.
Comment on lines
+47
to
+53
| const createIsolate = (options: WorkerOptions): Isolate => { | ||
| // Detect worker type based on command | ||
| const commandName = (options.command as any).command; | ||
|
|
||
| // Check if it's a Node.js package manager command | ||
| if (commandName === "npm" || commandName === "yarn" || commandName === "pnpm") { | ||
| return new NodeRun({ command: options.command, port: options.port }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "worker.ts" | head -20Repository: deco-cx/deco
Length of output: 74
🌐 Web query:
Deno.Command public API properties and methods documentation
💡 Result:
Deno.Command (class)
Constructor
new Deno.Command(command: string | URL, options?: Deno.CommandOptions)[1]
Methods
output(): Promise<Deno.CommandOutput>— runs the command, waits for exit, buffers output (throws ifstdin: "piped"is set). Ifstdout/stderraren’t"piped", reading those fields on the returned output throwsTypeError. [1]outputSync(): Deno.CommandOutput— sync version ofoutput()with the samestdin/stdout/stderrcaveats. [1]spawn(): Deno.ChildProcess— spawns a streamable subprocess. [1]
Notes / behavior
Commandis a builder: eachspawn()oroutput()call starts a new subprocess. [1]- If any stdio option isn’t
"piped", accessing the corresponding field on the spawned child (or theCommandOutput) throwsTypeError. Also, whenstdin: "piped", you must manually close the writable stream. [1]
Related public types used by Deno.Command
Deno.CommandOptions (interface)
Properties include: args, cwd, clearEnv, env, uid, gid, signal (AbortSignal), stdin, stdout, stderr, windowsRawArguments, detached. [2]
Deno.CommandOutput (interface)
Returned from output() / outputSync(); extends CommandStatus and includes:
stdout: Uint8Arraystderr: Uint8Array[3]
Deno.ChildProcess (class) (returned by spawn())
Properties: pid, status: Promise<CommandStatus>, stdin: WritableStream, stdout, stderr [4]
Methods: kill(signo?), output(), ref(), unref(), [Symbol.asyncDispose]() [4]
Sources: Deno API reference pages for Deno.Command [1], Deno.CommandOptions [2], Deno.CommandOutput [3], Deno.ChildProcess [4].
🏁 Script executed:
# Once we find the file, let's read the relevant lines
git ls-files | grep -E "daemon/worker\.ts|worker\.ts"Repository: deco-cx/deco
Length of output: 72
🏁 Script executed:
cat -n daemon/worker.ts | sed -n '40,60p'Repository: deco-cx/deco
Length of output: 832
🏁 Script executed:
# Also check the full WorkerOptions interface and createIsolate function
cat -n daemon/worker.ts | head -100Repository: deco-cx/deco
Length of output: 3380
Use explicit runtime metadata instead of relying on internal Deno.Command properties.
Line 49 accesses (options.command as any).command, which is not a stable public contract. Deno.Command does not expose the original executable name through its public API, making this detection fragile and prone to silent failure if the internal structure changes. This can cause Node.js package manager commands to be routed to the wrong isolate.
Pass explicit runtime information via WorkerOptions instead:
Proposed refactor
export interface WorkerOptions {
persist: () => void;
command: Deno.Command;
port: number;
+ runtime?: "deno" | "node";
}
const createIsolate = (options: WorkerOptions): Isolate => {
- // Detect worker type based on command
- const commandName = (options.command as any).command;
-
- // Check if it's a Node.js package manager command
- if (commandName === "npm" || commandName === "yarn" || commandName === "pnpm") {
+ if (options.runtime === "node") {
return new NodeRun({ command: options.command, port: options.port });
}
-
- // Default to DenoRun for Deno projects
return new DenoRun({ command: options.command, port: options.port });
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/worker.ts` around lines 47 - 53, The code currently reads an internal
property (options.command as any).command inside createIsolate to detect package
managers; stop relying on Deno.Command internals and instead add explicit
runtime metadata to WorkerOptions (e.g., a runtime: 'node' | 'deno' |
'package-manager' or a packageManager flag). Update createIsolate to branch on
options.runtime (or options.packageManager) rather than options.command, and
construct the NodeRun isolate with new NodeRun({ command: options.command, port:
options.port }) when the runtime indicates Node/package-manager; remove usage of
(options.command as any).command entirely.
Comment on lines
+10
to
+12
| const PORT_WAIT_TIMEOUT = PORT_WAIT_TIMEOUT_STR | ||
| ? +PORT_WAIT_TIMEOUT_STR | ||
| : 120_000; // 2 minutes for Node.js workers (FastStore/Next.js can take longer to start) |
There was a problem hiding this comment.
Validate PORT_TIMEOUT before using it as a timeout.
If PORT_TIMEOUT is non-numeric, PORT_WAIT_TIMEOUT becomes NaN, which can make readiness waits behave incorrectly.
Proposed fix
const PORT_WAIT_TIMEOUT_STR = Deno.env.get("PORT_TIMEOUT");
-const PORT_WAIT_TIMEOUT = PORT_WAIT_TIMEOUT_STR
- ? +PORT_WAIT_TIMEOUT_STR
- : 120_000; // 2 minutes for Node.js workers (FastStore/Next.js can take longer to start)
+const parsedPortTimeout = PORT_WAIT_TIMEOUT_STR
+ ? Number(PORT_WAIT_TIMEOUT_STR)
+ : undefined;
+const PORT_WAIT_TIMEOUT = Number.isFinite(parsedPortTimeout) &&
+ (parsedPortTimeout as number) > 0
+ ? (parsedPortTimeout as number)
+ : 120_000; // 2 minutes for Node.js workers (FastStore/Next.js can take longer to start)📝 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 PORT_WAIT_TIMEOUT = PORT_WAIT_TIMEOUT_STR | |
| ? +PORT_WAIT_TIMEOUT_STR | |
| : 120_000; // 2 minutes for Node.js workers (FastStore/Next.js can take longer to start) | |
| const PORT_WAIT_TIMEOUT_STR = Deno.env.get("PORT_TIMEOUT"); | |
| const parsedPortTimeout = PORT_WAIT_TIMEOUT_STR | |
| ? Number(PORT_WAIT_TIMEOUT_STR) | |
| : undefined; | |
| const PORT_WAIT_TIMEOUT = Number.isFinite(parsedPortTimeout) && | |
| (parsedPortTimeout as number) > 0 | |
| ? (parsedPortTimeout as number) | |
| : 120_000; // 2 minutes for Node.js workers (FastStore/Next.js can take longer to start) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/workers/nodeRun.ts` around lines 10 - 12, PORT_WAIT_TIMEOUT currently
uses PORT_WAIT_TIMEOUT_STR directly which can produce NaN; update the
initializer for PORT_WAIT_TIMEOUT to coerce and validate the input (e.g., const
parsed = Number(PORT_WAIT_TIMEOUT_STR)) and only use parsed if
Number.isFinite(parsed) and parsed > 0, otherwise fall back to the default
120_000; make this change where PORT_WAIT_TIMEOUT is defined and consider
emitting a warning via the existing logger if the env value is invalid.
Comment on lines
+43
to
+49
| constructor(options: NodeIsolateOptions | CommandIsolate) { | ||
| if (isCmdIsolate(options)) { | ||
| this.port = options.port; | ||
| this.command = options.command; | ||
| } else { | ||
| this.port = portPool.get(); | ||
| const packageManager = options.packageManager ?? "npm"; |
There was a problem hiding this comment.
Do not free fixed externally-provided ports from portPool.
When NodeRun is created from CommandIsolate (Line 44), the port may not come from portPool. Line 115 currently frees it unconditionally.
Proposed fix
export class NodeRun implements Isolate {
+ private ownsPort = false;
@@
constructor(options: NodeIsolateOptions | CommandIsolate) {
if (isCmdIsolate(options)) {
this.port = options.port;
this.command = options.command;
} else {
+ this.ownsPort = true;
this.port = portPool.get();
@@
- portPool.free(this.port);
+ if (this.ownsPort) {
+ portPool.free(this.port);
+ }Also applies to: 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/workers/nodeRun.ts` around lines 43 - 49, The cleanup currently always
returns this.port to portPool even when the port was provided externally via
CommandIsolate; modify the NodeRun constructor to set a boolean flag (e.g.,
this.portAllocated) to true only when you call portPool.get() in the non-command
branch and false when isCmdIsolate(options) supplies the port, then in the
shutdown/cleanup method (where portPool.free(this.port) is called) guard that
call with if (this.portAllocated) to only free ports you actually allocated from
portPool.
Comment on lines
+119
to
+127
| fetch(request: Request): Promise<Response> { | ||
| this.inflightRequests++; | ||
| const { pathname, search } = new URL(request.url); | ||
| const url = new URL("." + pathname, this.proxyUrl); | ||
| url.search = search; | ||
|
|
||
| if (request.headers.get("upgrade") === "websocket") { | ||
| return proxyWebSocket(url, request); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the nodeRun.ts file
find . -name "nodeRun.ts" -type fRepository: deco-cx/deco
Length of output: 83
🏁 Script executed:
# Once located, let's read the file to see the actual code at the mentioned lines
cat -n daemon/workers/nodeRun.ts | head -250Repository: deco-cx/deco
Length of output: 8318
🌐 Web query:
Deno WebSocket constructor URL scheme requirements http https ws wss
💡 Result:
In Deno, WebSocket follows the Web standard URL rules:
- The constructor URL must use one of these schemes:
ws:,wss:,http:, orhttps:. Other schemes throw aSyntaxError. URL fragments (#...) are not allowed (alsoSyntaxError). [1][2] - If you pass
http:/https:, user agents treat it as a WebSocket connection and effectively map it tows:/wss:(e.g.,https:→wss:). [1][2] - In Deno specifically, outbound WebSocket connections also require the
--allow-netpermission (or equivalent permission model grant). [3]
Sources:
[1] MDN WebSocket() constructor URL requirements: allowed schemes + no fragments
[2] WHATWG WebSockets Standard: allowed schemes (ws/wss/http/https) + fragments rejected
[3] Deno WebSocket API docs (permission note and usage)
WebSocket proxy missing inflight request decrement on socket close.
The inflightRequests counter is incremented at line 120 but never decremented for WebSocket connections. While regular HTTP requests use .finally() at line 138 to clean up, the WebSocket branch (lines 125–126) returns immediately without tracking socket lifecycle. When sockets eventually close (lines 216–224), inflightRequests remains elevated, breaking the graceful shutdown mechanism at line 102–109 that waits for all inflight requests to reach zero.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/workers/nodeRun.ts` around lines 119 - 127, fetch increments
this.inflightRequests but returns early for WebSocket paths via
proxyWebSocket(url, request) without ever decrementing; update the WebSocket
branch so that inflightRequests is decremented when the socket
closes/errors—either by modifying proxyWebSocket to accept a completion callback
or wrapping its returned promise/connection to call this.inflightRequests-- on
close/error, ensuring the decrement happens exactly once and also on failure so
the graceful shutdown that checks inflightRequests (used by the shutdown logic)
can succeed.
| for (const blk of blocks()) { | ||
| let totalBlocks = 0; | ||
| const blockDir = join(dir, blk.type); | ||
| const blockDir = join(dir, blk.type === "sections" ? "src/components/sections" : blk.type); |
There was a problem hiding this comment.
Keep sections path resolution backward-compatible.
Line 83 hardcodes src/components/sections and drops the legacy sections path, which can remove section entries from manifests in non-FastStore layouts.
Proposed fix (fallback to legacy path when needed)
- const blockDir = join(dir, blk.type === "sections" ? "src/components/sections" : blk.type);
+ const blockDir = blk.type === "sections"
+ ? (await exists(join(dir, "src/components/sections"))
+ ? join(dir, "src/components/sections")
+ : join(dir, "sections"))
+ : join(dir, blk.type);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/manifest/manifestGen.ts` at line 83, The computed blockDir (const
blockDir = join(dir, blk.type === "sections" ? "src/components/sections" :
blk.type)) forces the new FastStore path and drops the legacy "sections" path;
change the resolution to try the new path first and fall back to the legacy
"sections" path when the new path does not exist: compute the candidate path for
blk.type === "sections" as join(dir, "src/components/sections"), check existence
(e.g., fs.existsSync or fs.promises.stat), and if missing use join(dir,
"sections") instead (keep the same behavior for non-"sections" types), updating
the blockDir assignment and any callers that rely on it (reference symbols:
blockDir, blk.type, join).
| static async init<TAppManifest extends AppManifest = AppManifest>( | ||
| opts?: DecoOptions<TAppManifest>, | ||
| ): Promise<Deco<TAppManifest>> { | ||
| console.log("Deco.init", opts) |
There was a problem hiding this comment.
Avoid logging full init options in production path
Line 82 logs the entire opts object, which can leak sensitive configuration (e.g., data within bindings) and adds noisy startup logs. Please remove it or gate/sanitize it behind an explicit debug flag.
Proposed fix
- console.log("Deco.init", opts)
+ // Intentionally avoid logging full opts to prevent leaking runtime config.
+ // If needed, add a debug-only, sanitized log here.📝 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
| console.log("Deco.init", opts) | |
| // Intentionally avoid logging full opts to prevent leaking runtime config. | |
| // If needed, add a debug-only, sanitized log here. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/mod.ts` at line 82, Remove the unguarded console.log("Deco.init",
opts) that prints the full init options (opts) to avoid leaking sensitive data;
either delete that call or wrap it behind a debug flag (e.g., if (DEBUG)
console.log(...)) or sanitize the object (only log non-sensitive fields) so only
safe fields are emitted; update the init/Deco initialization code that
references opts to use the guarded logging path and ensure bindings or
credential-like fields are omitted from any logs.
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 cubic
Adds FastStore support by auto-detecting FastStore projects and running their Node dev server (npm/yarn/pnpm run dev) on port 3000. Also improves file watching/SSE stability and updates manifest handling for sections.
Written for commit f846297. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores