Skip to content

fix scaffolding#12

Merged
cobraprojects merged 3 commits into
mainfrom
fix-user-facing-api
Apr 29, 2026
Merged

fix scaffolding#12
cobraprojects merged 3 commits into
mainfrom
fix-user-facing-api

Conversation

@cobraprojects
Copy link
Copy Markdown
Owner

@cobraprojects cobraprojects commented Apr 29, 2026

Summary by CodeRabbit

  • New Features

    • Added a Next.js config enhancer (withHolo) and exposed it for consumers.
  • Improvements

    • Dev environment now regenerates a framework runner script and better handles stale Next dev servers.
    • Next scaffolding emits a simplified TypeScript config; managed dependency list always includes core.
    • SvelteKit hook handling and legacy migration improved.
  • Database

    • Stronger typing for belongsTo and many-to-many relation APIs.
  • Tests

    • Expanded CLI and SvelteKit prepare tests; updated broadcast manifest type assertions.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

Warning

Rate limit exceeded

@cobraprojects has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 25 minutes and 50 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ec3f1c7a-af49-4757-8f5f-ad7306d64c51

📥 Commits

Reviewing files that changed from the base of the PR and between 63ccee7 and ad83d6c.

📒 Files selected for processing (4)
  • packages/adapter-next/src/config.ts
  • packages/cli/tests/cli.test.ts
  • packages/core/src/portable/holo.ts
  • packages/core/tsup.config.ts
📝 Walkthrough

Walkthrough

Adds framework-runner refresh steps to project prepare flow, implements Next.js dev-server stale-PID detection and restart, refines SvelteKit hooks override detection and legacy cleanup, moves generated broadcast manifest types to local declarations, exports renderFrameworkRunner, and widens DB relation generics; accompanying test updates and new Next/adapter-next additions included.

Changes

Cohort / File(s) Summary
CLI Prepare / Runner
packages/cli/src/dev.ts, packages/cli/src/project.ts
Adds refreshFrameworkRunner invocation to runProjectPrepare (before/after prepare steps) and re-exports renderFrameworkRunner.
CLI Scaffold & Runner Behavior
packages/cli/src/project/scaffold.ts
Framework runner: per-line stdout/stderr callback support, Next.js dev conflict recovery (buffer stderr, detect "Another next dev server", kill stale PID, restart once), ensures @holo-js/core remains a managed dependency, and tsconfig include update.
SvelteKit Hooks & Registry
packages/cli/src/project/registry.ts
Adds getSvelteConfigHooksOverrideState to classify managed/custom/none, adjusts patching to skip managed and error on custom overrides, updates single-line kit injection handling, and only removes legacy .user.ts hooks when migrated; generated broadcast manifest now emits local type declarations rather than importing from core.
CLI Tests
packages/cli/tests/cli.test.ts, packages/cli/tests/broadcast-registry.test.ts
Adds test emulation of stale Next dev server and assertions for restart behavior, updates dependency sync expectations to require @holo-js/core, tightens watch/prepare test timing, and changes broadcast-manifest test to expect local GeneratedBroadcastManifest type.
Core runtime schema loading
packages/core/src/portable/holo.ts
preloadGeneratedSchemaModule now verifies generated schema path with existsSync, uses importBundledRuntimeModule(...) for loading, and broadens error-message matching while only suppressing errors for the exact expected target.
Database typings
packages/db/src/model/ModelQueryBuilder.ts, packages/db/src/model/defineModel.ts
Adds TRelatedRelations extends RelationMap = RelationMap generic to whereBelongsTo/orWhereBelongsTo signatures and propagates it through helper typings.
Database tests
packages/db/tests/model-relations.test.ts
Adds test verifying many-to-many relation property can be both a callable and a lazy value resolver.
Adapter Next: config & exports
packages/adapter-next/src/config.ts, packages/adapter-next/src/index.ts, packages/adapter-next/package.json, packages/adapter-next/tsup.config.ts
Adds withHolo(nextConfig) helper, re-exports it, exposes new ./config package subpath, and includes src/config.ts in build entries.
Build / Post-build tweaks
packages/core/tsup.config.ts
Post-build pass rewrites bare Node built-in imports to node:<name> across .mjs outputs.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI (runProjectPrepare)
    participant Refresh as refreshFrameworkRunner
    participant Runner as Framework Runner (run.mjs)
    participant NextDev as Next.js dev child
    participant Stale as Stale next dev PID

    CLI->>Refresh: Read project.json & render run.mjs
    CLI->>Runner: Spawn framework runner
    Runner->>NextDev: Start `next dev` (child process)
    NextDev-->>Runner: Emit stderr ("Another next dev server...")
    Runner->>Runner: Buffer stderr lines (up to 200)
    Runner->>Runner: Detect stale PID X in message
    Runner->>Stale: Send SIGTERM to PID X
    Stale->>Stale: Exit
    Runner->>NextDev: Restart `next dev` (one retry)
    NextDev->>Runner: Start successfully / return code
    CLI->>Refresh: Re-run refreshFrameworkRunner after managed dep sync
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • fix user facing api #11: Overlaps CLI prepare and dev runner changes (touches packages/cli/src/dev.ts, project.ts), directly related to runner/prepare behavior.
  • Test coverage #10: Modifies project/registry.ts code-gen/registry logic, related to generated broadcast-manifest/type changes.
  • security package #3: Adds exports in packages/cli/src/project.ts, related to the newly re-exported renderFrameworkRunner.

Poem

🐰 I refreshed the runner, neat and spry,
I chased a stale PID from the sky,
Hooks patched with manners, types aligned,
Rewrites and tests all neatly timed,
A carrot for CI — hop, we fly! 🥕✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'fix scaffolding' is vague and generic, using non-descriptive language that fails to convey meaningful information about the actual changeset. Revise the title to be more specific about the primary changes, such as 'Refactor Next.js config scaffolding and add framework runner management' or 'Update framework runner generation and Next.js config generation'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-user-facing-api

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 25 minutes and 50 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/cli/src/project/registry.ts (1)

531-565: ⚠️ Potential issue | 🔴 Critical

Type discrepancies prevent proper type alignment with @holo-js/core interfaces.

The locally declared types do not match the original interfaces from @holo-js/core (packages/core/src/portable/registry.ts):

  • GeneratedBroadcastManifestChannel.type: Local includes 'public' | 'private' | 'presence', but the original only allows 'private' | 'presence'. This broader type could permit invalid values at runtime.
  • GeneratedBroadcastManifestChannel.member: Local declares readonly member?: unknown, but the original specifies readonly member?: Readonly<Record<string, unknown>>. The local version loses type specificity.

The satisfies GeneratedBroadcastManifest operator does not catch these mismatches because only GeneratedBroadcastManifest itself is properly aligned; the nested type definitions diverge from the original interfaces, undermining type safety.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/project/registry.ts` around lines 531 - 565, The locally
declared types diverge from the core interfaces; replace or align them with the
originals by either importing the types from `@holo-js/core` (preferred) or
updating the local declarations: change GeneratedBroadcastManifestChannel.type
to only allow 'private' | 'presence' (remove 'public'), change
GeneratedBroadcastManifestChannel.member to readonly Readonly<Record<string,
unknown>> (instead of unknown), and ensure GeneratedBroadcastManifest and
GeneratedBroadcastManifestEvent reflect the core shapes; update the export
broadcastManifest to use the imported/updated types so the satisfies
GeneratedBroadcastManifest check enforces correct nested types (refer to
GeneratedBroadcastManifestChannel, GeneratedBroadcastManifestEvent,
GeneratedBroadcastManifest, and broadcastManifest to locate the code).
packages/core/src/portable/holo.ts (1)

61-75: ⚠️ Potential issue | 🟡 Minor

Regex pattern mismatch: won't catch actual bundling errors.

The regex on line 64 expects Failed to load url but the actual error message thrown by bundleRuntimeModule (line 108 of runtimeModule.ts) is Failed to load ${entryPath}. (e.g., Failed to load src/schema.ts.). This means bundling errors for the expected target won't be suppressed—they'll always propagate because the pattern fails to match.

Either adjust the regex to match the actual error format or confirm that bundling failures should always be fatal.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/portable/holo.ts` around lines 61 - 75, The catch block
currently ignores bundling errors by testing error.message against a regex that
expects "Failed to load url" but bundleRuntimeModule (runtimeModule.ts) throws
"Failed to load <entryPath>.", so update the regex/test and the capture logic
around error (the code that builds failedTarget and compares to expectedTarget)
to also match the actual "Failed to load <entryPath>" format: broaden the test
to include plain "Failed to load" and add an alternative capture branch that
extracts the entryPath from messages like "Failed to load <path>." so
failedTarget can equal expectedTarget; modify the pattern used in the
failedTarget extraction (the call that .match(...).slice(1).find(...)) to
include a capture for that "Failed to load <entryPath>" form.
🧹 Nitpick comments (2)
packages/db/tests/model-relations.test.ts (1)

971-973: Strengthen the remaining-role assertion.

After Line 971, checking only length can still pass with the wrong remaining relation. Consider asserting the remaining role identity too.

Suggested test hardening
     const loadedRoles = await lazyUser.roles
     expect(loadedRoles).toHaveLength(1)
+    expect(loadedRoles[0]?.get('id')).toBe(101)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/db/tests/model-relations.test.ts` around lines 971 - 973, The test
currently only asserts loadedRoles length after awaiting lazyUser.roles;
strengthen it by also asserting the remaining role’s identity to ensure the
correct relation was kept. After obtaining const loadedRoles = await
lazyUser.roles, add an assertion that compares the remaining role to the
expected role (e.g., check loadedRoles[0].id or loadedRoles[0].name against the
known role variable or use expect(loadedRoles).toContainEqual(expectedRole)) so
the test fails if the wrong relation remains.
packages/cli/src/project/registry.ts (1)

285-309: Single-line kit config handling looks correct but may produce inconsistent indentation.

The logic correctly identifies single-line kit: { ... } patterns and expands them to multi-line. However, the resulting indentation may not perfectly match the surrounding code style (e.g., 4-space files: vs 2-space closing }). This is cosmetic and won't affect functionality.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/project/registry.ts` around lines 285 - 309, The single-line
expansion for kit: {...} can produce inconsistent indentation; update the
replacement to preserve the original indentation by detecting the leading
whitespace of the matched opening (use the captured opening or inspect the
match.index to derive leading spaces) and prefix bodyLines,
SVELTE_HOOKS_OVERRIDE_BLOCK and the closing line with that indentation instead
of hardcoded spaces; keep the existing logic for trimmedBody, suffix detection
and the symbols singleLineKitPattern, singleLineKitMatch,
SVELTE_HOOKS_OVERRIDE_BLOCK and replacement when constructing the final string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cli/src/dev.ts`:
- Around line 157-179: The try/catch in refreshFrameworkRunner hides errors from
writeTextFile (and renderFrameworkRunner); change the flow so only the
readFile/JSON.parse are guarded: wrap the readFile+JSON.parse (and manifest
validation) in a try/catch that returns on missing/invalid project.json, but
call writeTextFile(frameworkRunnerPath, renderFrameworkRunner(...)) outside that
catch so any write failures propagate (or are explicitly handled/logged). Refer
to refreshFrameworkRunner, frameworkProjectPath, manifest.framework,
renderFrameworkRunner, and writeTextFile when making the change.

In `@packages/cli/src/project/scaffold.ts`:
- Around line 3469-3480: The code currently pushes every stderr line into
stderrLines during the whole child process lifetime (see spawn(...), child,
pipeOutput, stderrLines), which can grow unbounded for long-lived dev servers;
limit collection to a small, bounded buffer (e.g., keep only the last N lines
via a ring buffer or Array.splice to maintain maxStderrLines) or stop appending
after a short startup window or after the conflict parser has run — update the
stderr pipe callback (the lambda passed to pipeOutput for child.stderr) to
perform bounded buffering and/or to set a flag to ignore further lines once
parsing is complete, so memory cannot grow unbounded.
- Around line 3482-3501: The current close handler only resolves the exit code
and loses the terminating signal, causing forwarded SIGINT/SIGTERM to be
converted into exit code 1; change the Promise around child to resolve an object
with both code and signal (e.g., resolve({ code, signal })), update subsequent
checks to use that object (replace references to `code` with `result.code` and
check `result.signal`), preserve signal exits by, when `result.signal` is
non-null, re-raising it for the current process (e.g., call
process.kill(process.pid, result.signal)) instead of calling process.exit(1),
and keep the existing restart-on-conflict flow (using `extractNextConflictPid`,
`stopStaleNextDevServer`, `restartedAfterConflict`, `child`, and `stderrLines`)
otherwise.
- Around line 3423-3437: stopStaleNextDevServer currently sends SIGTERM to any
PID reported by Next and treats a live PID as stale; change it to first verify
the PID is actually a defunct/stale Next dev server before killing it by (a)
using a non-destructive check (e.g., try process.kill(pid, '0') to confirm the
process exists, then probe the expected dev-server endpoint/port or inspect the
process command line to confirm it’s a Next dev process), (b) only call
process.kill(pid, 'SIGTERM') if those checks indicate the process is the Next
dev server and not responding, and (c) handle and return correctly on
ESRCH/EPERM as now; keep waitForProcessExit(pid) for post-terminate waiting and
update stopStaleNextDevServer to return false when the live process is confirmed
healthy instead of killing it.

In `@packages/cli/tests/cli.test.ts`:
- Around line 1420-1448: Wrap the test logic that uses the spawned process
(staleNextProcess) in a try/finally (or add an afterEach cleanup) that ensures
the interval process is terminated regardless of assertion failures: in the
finally block call staleNextProcess.kill() (or
process.kill(staleNextProcess.pid) with a try/catch that ignores ESRCH) and
await its exit (e.g., listen for the 'exit' event or use wait) to guarantee the
child is not left running; update the test surrounding runNodeScript(... 'dev')
/ expect(...) checks to use this cleanup so staleNextProcess is always cleaned
up.

---

Outside diff comments:
In `@packages/cli/src/project/registry.ts`:
- Around line 531-565: The locally declared types diverge from the core
interfaces; replace or align them with the originals by either importing the
types from `@holo-js/core` (preferred) or updating the local declarations: change
GeneratedBroadcastManifestChannel.type to only allow 'private' | 'presence'
(remove 'public'), change GeneratedBroadcastManifestChannel.member to readonly
Readonly<Record<string, unknown>> (instead of unknown), and ensure
GeneratedBroadcastManifest and GeneratedBroadcastManifestEvent reflect the core
shapes; update the export broadcastManifest to use the imported/updated types so
the satisfies GeneratedBroadcastManifest check enforces correct nested types
(refer to GeneratedBroadcastManifestChannel, GeneratedBroadcastManifestEvent,
GeneratedBroadcastManifest, and broadcastManifest to locate the code).

In `@packages/core/src/portable/holo.ts`:
- Around line 61-75: The catch block currently ignores bundling errors by
testing error.message against a regex that expects "Failed to load url" but
bundleRuntimeModule (runtimeModule.ts) throws "Failed to load <entryPath>.", so
update the regex/test and the capture logic around error (the code that builds
failedTarget and compares to expectedTarget) to also match the actual "Failed to
load <entryPath>" format: broaden the test to include plain "Failed to load" and
add an alternative capture branch that extracts the entryPath from messages like
"Failed to load <path>." so failedTarget can equal expectedTarget; modify the
pattern used in the failedTarget extraction (the call that
.match(...).slice(1).find(...)) to include a capture for that "Failed to load
<entryPath>" form.

---

Nitpick comments:
In `@packages/cli/src/project/registry.ts`:
- Around line 285-309: The single-line expansion for kit: {...} can produce
inconsistent indentation; update the replacement to preserve the original
indentation by detecting the leading whitespace of the matched opening (use the
captured opening or inspect the match.index to derive leading spaces) and prefix
bodyLines, SVELTE_HOOKS_OVERRIDE_BLOCK and the closing line with that
indentation instead of hardcoded spaces; keep the existing logic for
trimmedBody, suffix detection and the symbols singleLineKitPattern,
singleLineKitMatch, SVELTE_HOOKS_OVERRIDE_BLOCK and replacement when
constructing the final string.

In `@packages/db/tests/model-relations.test.ts`:
- Around line 971-973: The test currently only asserts loadedRoles length after
awaiting lazyUser.roles; strengthen it by also asserting the remaining role’s
identity to ensure the correct relation was kept. After obtaining const
loadedRoles = await lazyUser.roles, add an assertion that compares the remaining
role to the expected role (e.g., check loadedRoles[0].id or loadedRoles[0].name
against the known role variable or use
expect(loadedRoles).toContainEqual(expectedRole)) so the test fails if the wrong
relation remains.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aca5df80-e7ea-4daf-92c3-188e8797fc1b

📥 Commits

Reviewing files that changed from the base of the PR and between c6cd77f and 7284897.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • packages/cli/src/dev.ts
  • packages/cli/src/project.ts
  • packages/cli/src/project/registry.ts
  • packages/cli/src/project/scaffold.ts
  • packages/cli/tests/broadcast-registry.test.ts
  • packages/cli/tests/cli.test.ts
  • packages/core/src/portable/holo.ts
  • packages/db/src/model/ModelQueryBuilder.ts
  • packages/db/src/model/defineModel.ts
  • packages/db/tests/model-relations.test.ts

Comment thread packages/cli/src/dev.ts
Comment on lines +3423 to +3437
'async function stopStaleNextDevServer(pid) {',
' if (!Number.isInteger(pid) || pid <= 0 || pid === process.pid) {',
' return false',
' }',
'',
' try {',
' process.kill(pid, \'SIGTERM\')',
' } catch (error) {',
' if (error && typeof error === \'object\' && \'code\' in error && error.code === \'ESRCH\') {',
' return true',
' }',
' return false',
' }',
'',
' return waitForProcessExit(pid)',
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't treat any live Next PID as stale.

stopStaleNextDevServer() sends SIGTERM to any existing PID. The preceding Next.js error only tells us another dev server is running; it does not prove the process is stale. Starting a second holo dev will therefore kill the first healthy server instead of surfacing the conflict.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/project/scaffold.ts` around lines 3423 - 3437,
stopStaleNextDevServer currently sends SIGTERM to any PID reported by Next and
treats a live PID as stale; change it to first verify the PID is actually a
defunct/stale Next dev server before killing it by (a) using a non-destructive
check (e.g., try process.kill(pid, '0') to confirm the process exists, then
probe the expected dev-server endpoint/port or inspect the process command line
to confirm it’s a Next dev process), (b) only call process.kill(pid, 'SIGTERM')
if those checks indicate the process is the Next dev server and not responding,
and (c) handle and return correctly on ESRCH/EPERM as now; keep
waitForProcessExit(pid) for post-terminate waiting and update
stopStaleNextDevServer to return false when the live process is confirmed
healthy instead of killing it.

Comment thread packages/cli/src/project/scaffold.ts
Comment thread packages/cli/src/project/scaffold.ts Outdated
Comment thread packages/cli/tests/cli.test.ts
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
packages/cli/tests/cli.test.ts (1)

1449-1451: ⚠️ Potential issue | 🟠 Major

Await the stale child exit in the finally cleanup.

This still only sends a signal. If an assertion fails earlier, the spawned interval process can remain alive long enough to bleed into later tests, which is the same cleanup gap from the earlier review.

🛠️ Proposed fix
     const staleNextProcess = spawn('node', ['-e', 'setInterval(() => {}, 1000)'], {
       stdio: 'ignore',
     })
+    const staleNextProcessExit = new Promise<void>((resolve) => {
+      staleNextProcess.once('exit', () => resolve())
+    })
     try {
       const nextStatePath = join(nextRoot, '.next-runner-state')
       await writeFile(join(nextRoot, 'node_modules/.bin/next'), `#!/usr/bin/env node
 const { existsSync, readFileSync, writeFileSync } = require('node:fs')
@@
     expect(await readFile(nextStatePath, 'utf8')).toBe('2')
     await expect(async () => process.kill(staleNextProcess.pid!, 0)).rejects.toMatchObject({ code: 'ESRCH' })
     } finally {
-      try { process.kill(staleNextProcess.pid!, 'SIGKILL') } catch { /* already exited */ }
+      if (staleNextProcess.exitCode === null && staleNextProcess.signalCode === null) {
+        staleNextProcess.kill('SIGTERM')
+        await staleNextProcessExit
+      }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/tests/cli.test.ts` around lines 1449 - 1451, In the finally
cleanup replace the blind process.kill call with logic that sends SIGKILL to
staleNextProcess (using process.kill(staleNextProcess.pid, 'SIGKILL')) and then
awaits the child process to actually exit by listening for the 'exit' event on
the ChildProcess object (staleNextProcess.once('exit', ...)) with a short
timeout fallback; swallow errors if the process already exited, but ensure you
await the exit promise so the spawned interval process cannot leak into
subsequent tests.
🧹 Nitpick comments (2)
packages/core/tsup.config.ts (1)

25-31: Avoid rebuilding the same regex on each recursive call.

Line 26 recompiles the same pattern for every directory depth. Small cost, but easy to avoid by hoisting/passing the compiled regex once.

♻️ Proposed refactor
+const bareToNodePattern = buildBareToNodeRegex()
+
-async function restoreNodeProtocol(dir: string): Promise<void> {
-  const pattern = buildBareToNodeRegex()
+async function restoreNodeProtocol(
+  dir: string,
+  pattern: RegExp = bareToNodePattern,
+): Promise<void> {
   const entries = await readdir(dir, { withFileTypes: true })
   for (const entry of entries) {
     const fullPath = join(dir, entry.name)
     if (entry.isDirectory()) {
-      await restoreNodeProtocol(fullPath)
+      await restoreNodeProtocol(fullPath, pattern)
     } else if (entry.name.endsWith('.mjs')) {
       const content = await readFile(fullPath, 'utf8')
       const updated = content.replace(pattern, '$1node:$2$3')
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/tsup.config.ts` around lines 25 - 31, The function
restoreNodeProtocol currently calls buildBareToNodeRegex() on every recursive
invocation (pattern variable), which is unnecessary; instead compute the regex
once and pass it down: add a new parameter like pattern: RegExp (optional) to
restoreNodeProtocol or overload it, call buildBareToNodeRegex() once before the
initial call, and use that same RegExp in all recursive calls (reference
restoreNodeProtocol and buildBareToNodeRegex to locate the change), updating all
internal calls that previously reinitialized pattern to reuse the passed RegExp.
packages/adapter-next/src/config.ts (1)

55-60: Harden beforeFiles handling before spreading.

If beforeFiles is present but not an array, spread will throw. A small guard avoids crashing on malformed user returns.

Proposed hardening
       if (userResult && typeof userResult === 'object' && !Array.isArray(userResult)) {
         const shaped = userResult as { beforeFiles?: unknown[], afterFiles?: unknown[], fallback?: unknown[] }
+        const beforeFiles = Array.isArray(shaped.beforeFiles) ? shaped.beforeFiles : []
         return {
           ...shaped,
-          beforeFiles: [...(shaped.beforeFiles ?? []), holoRewrite],
+          beforeFiles: [...beforeFiles, holoRewrite],
         }
       }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/adapter-next/src/config.ts` around lines 55 - 60, The code currently
spreads shaped.beforeFiles directly which will throw if beforeFiles exists but
is not an array; update the return to coerce or guard before spreading by using
Array.isArray(shaped.beforeFiles) ? shaped.beforeFiles : [] so beforeFiles
becomes [...(Array.isArray(shaped.beforeFiles) ? shaped.beforeFiles : []),
holoRewrite]; do the same pattern for afterFiles and fallback if they are
handled similarly to avoid crashes when user returns malformed values (refer to
the shaped object and holoRewrite symbol to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/adapter-next/src/config.ts`:
- Around line 29-37: The returned async rewrites() currently invokes the
detached reference userRewrites?.() which can lose the original this binding;
change the call to invoke userRewrites with the current this (e.g., use
userRewrites.call(this, ...) or Reflect.apply(userRewrites, this, [])) so the
original context is preserved when delegating to the user-provided rewrites
function; update the async rewrites() implementation that references
userRewrites to call it with the preserved this binding.

In `@packages/core/src/portable/holo.ts`:
- Around line 64-69: The regex used to extract failed module paths from
error.message (inside packages/core/src/portable/holo.ts) stops at the first dot
because it uses "Failed to load ([^.]+)\.", causing failedTarget (the result of
the message.match(...).slice(1).find(...)) to be truncated and not equal
expectedTarget; change the pattern "Failed to load ([^.]+)\." to allow dots
(e.g. "Failed to load ([^ ]+)\.") so the full generated-schema path (including
dots and .ts/.js) is captured and the failedTarget === expectedTarget check
works.

---

Duplicate comments:
In `@packages/cli/tests/cli.test.ts`:
- Around line 1449-1451: In the finally cleanup replace the blind process.kill
call with logic that sends SIGKILL to staleNextProcess (using
process.kill(staleNextProcess.pid, 'SIGKILL')) and then awaits the child process
to actually exit by listening for the 'exit' event on the ChildProcess object
(staleNextProcess.once('exit', ...)) with a short timeout fallback; swallow
errors if the process already exited, but ensure you await the exit promise so
the spawned interval process cannot leak into subsequent tests.

---

Nitpick comments:
In `@packages/adapter-next/src/config.ts`:
- Around line 55-60: The code currently spreads shaped.beforeFiles directly
which will throw if beforeFiles exists but is not an array; update the return to
coerce or guard before spreading by using Array.isArray(shaped.beforeFiles) ?
shaped.beforeFiles : [] so beforeFiles becomes
[...(Array.isArray(shaped.beforeFiles) ? shaped.beforeFiles : []), holoRewrite];
do the same pattern for afterFiles and fallback if they are handled similarly to
avoid crashes when user returns malformed values (refer to the shaped object and
holoRewrite symbol to locate the change).

In `@packages/core/tsup.config.ts`:
- Around line 25-31: The function restoreNodeProtocol currently calls
buildBareToNodeRegex() on every recursive invocation (pattern variable), which
is unnecessary; instead compute the regex once and pass it down: add a new
parameter like pattern: RegExp (optional) to restoreNodeProtocol or overload it,
call buildBareToNodeRegex() once before the initial call, and use that same
RegExp in all recursive calls (reference restoreNodeProtocol and
buildBareToNodeRegex to locate the change), updating all internal calls that
previously reinitialized pattern to reuse the passed RegExp.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c04f1f78-5f07-4ac8-a70e-9cbc50d274cf

📥 Commits

Reviewing files that changed from the base of the PR and between 7284897 and 63ccee7.

📒 Files selected for processing (10)
  • packages/adapter-next/package.json
  • packages/adapter-next/src/config.ts
  • packages/adapter-next/src/index.ts
  • packages/adapter-next/tsup.config.ts
  • packages/cli/src/dev.ts
  • packages/cli/src/project/registry.ts
  • packages/cli/src/project/scaffold.ts
  • packages/cli/tests/cli.test.ts
  • packages/core/src/portable/holo.ts
  • packages/core/tsup.config.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/adapter-next/tsup.config.ts

Comment thread packages/adapter-next/src/config.ts
Comment thread packages/core/src/portable/holo.ts
@cobraprojects cobraprojects merged commit c7b4ab0 into main Apr 29, 2026
1 check passed
This was referenced Apr 30, 2026
@coderabbitai coderabbitai Bot mentioned this pull request May 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant