refactor#13
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds extensive CLI project-scaffolding (config renderers, dependency sync, framework/project/workspace renderers), refactors DB model/entity runtime into modular helpers and a strong-typed static-model API, improves ESLint runner resilience, and tweaks SvelteKit legacy hook migration and related tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/db/src/model/defineModel.ts (1)
161-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't make generated-model unique ID validation a one-time check.
resolvedAtDefinitionis sampled once here. If the model is declared before the generated schema registry is loaded,validateUniqueIdConfig()never runs again, so baduniqueIds/newUniqueIdconfig survives until a later write path fails with a generic column error. Please validate from the lazyuniqueIdConfigpath as soon asresolveTable()succeeds.Suggested fix
const resolveUniqueId = (): UniqueIdRuntimeConfig<GeneratedSchemaTable<TName>> | null => { - return resolveUniqueIdConfig( + const config = resolveUniqueIdConfig( options.traits, resolvePrimaryKey(), options.uniqueIds, options.newUniqueId, ) + validateUniqueIdConfig(resolveTable(), inferredName, config) + return config }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/defineModel.ts` around lines 161 - 196, The current validation uses the one-time snapshot resolvedAtDefinition and can miss late-loaded generated tables; change it to run validateUniqueIdConfig when the lazy resolver actually resolves the table: whenever resolveTable() returns a table (i.e., not undefined/null) call validateUniqueIdConfig with that resolved table, inferredName and the runtime result of resolveUniqueId(), rather than only checking resolvedAtDefinition; specifically update the logic around resolvedAtDefinition/resolveTable/resolveUniqueId to invoke validateUniqueIdConfig after resolveTable() succeeds so bad uniqueIds/newUniqueId are caught as soon as the generated schema is available.packages/cli/tests/cli.test.ts (1)
9318-9325:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore the pre-materialization check for pending models.
expect.any(Array)no longer proves thatserver/models/Course.tsstays out of the registry beforeserver/db/schema.generated.tsis materialized, so an eager-registration regression would still pass.🧪 Make the intent explicit
await withFakeBun(async () => { await cliInternals.runProjectPrepare(projectRoot) }) await withFakeBun(async () => { - await expect(loadGeneratedProjectRegistry(projectRoot)).resolves.toMatchObject({ - models: expect.any(Array), - }) + const initialRegistry = await loadGeneratedProjectRegistry(projectRoot) + expect(initialRegistry?.models ?? []).not.toContain('server/models/Course.ts') })🤖 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 9318 - 9325, Restore the explicit pre-materialization assertion: after the first withFakeBun + cliInternals.runProjectPrepare call, loadGeneratedProjectRegistry(projectRoot) should assert that the registry's models array exists but does NOT include the pending model file (e.g., "server/models/Course.ts" or a model named "Course")—replace the generic expect.any(Array) check with an assertion like expect(models).toEqual(expect.not.arrayContaining([expect.objectContaining({ file: expect.stringContaining('server/models/Course.ts') })])) (or equivalent) so the test verifies the Course model is absent before server/db/schema.generated.ts is materialized.
🧹 Nitpick comments (1)
packages/cli/tests/cli.test.ts (1)
1270-1277: ⚡ Quick winReuse
ESBUILD_PACKAGE_VERSIONin this assertion.This hard-codes a second copy of the scaffolded
esbuildrange even though production already sources it frompackages/cli/src/metadata.ts. The next version bump will drift unless both places are updated.♻️ Suggested cleanup
-import { HOLO_PACKAGE_VERSION } from '../src/metadata' +import { ESBUILD_PACKAGE_VERSION, HOLO_PACKAGE_VERSION } from '../src/metadata' ... - })).toContain('"esbuild": "^0.27.4"') + })).toContain(`"esbuild": "${ESBUILD_PACKAGE_VERSION}"`)🤖 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 1270 - 1277, The test hard-codes the esbuild version string; update the assertion in the test (the call to projectInternals.renderScaffoldPackageJson) to reference the shared ESBUILD_PACKAGE_VERSION constant instead of the literal '"esbuild": "^0.27.4"'; import ESBUILD_PACKAGE_VERSION from the metadata module (where it’s defined) and assert that the rendered scaffold contains the package entry built from that constant so the test stays in sync with packages/cli/src/metadata.ts and the renderScaffoldPackageJson behavior.
🤖 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/project/scaffold/dependencies.ts`:
- Around line 702-739: The loops over requestedPackages and
AUTH_SOCIAL_PROVIDER_PACKAGE_NAMES currently only remove disabled packages from
devDependencies, leaving stale runtime entries in dependencies; update the
disabled (enabled === false) branches inside the requestedPackages loop and the
AUTH_SOCIAL_PROVIDER_PACKAGE_NAMES loop so they also delete packageName from
dependencies when present (check typeof dependencies[packageName] !==
'undefined' and delete it, set changed = true), ensuring both dependencies and
devDependencies are pruned for disabled features in the code in dependencies.ts.
In `@packages/cli/src/project/scaffold/framework.ts`:
- Around line 576-590: The stopStaleNextDevServer function currently
unconditionally sends SIGTERM to the reported PID; change it to first verify the
PID belongs to the Next server instance for this project before killing it:
query the process details (e.g., read /proc/<pid>/cwd and /proc/<pid>/cmdline on
Linux or use ps -p <pid> -o args= / -o cwd= on other platforms) and compare the
process cwd or command-line to this project's root/path or an expected Next dev
marker; only call process.kill(pid, 'SIGTERM') if the cwd/args match the current
project, otherwise return false (or surface a contention error) and apply the
same ownership check to the analogous block referenced in
stopStaleNextDevServer's other occurrence.
- Around line 177-183: renderNextPage currently interpolates raw projectName
into JSX which can break markup or enable injection; escape special HTML
characters before embedding (convert & to &, < to <, > to >, { to
{, } to }, and quotes if needed) and use that escaped string in the
template. Update renderNextPage to call a small util (e.g., escapeHtml) that
performs these replacements and return the escaped value in the template, and
apply the same fix to the other page/template helper that generates markup
around lines 347-353 so all templates use the escapeHtml utility.
- Around line 610-616: Handlers for SIGINT/SIGTERM are anonymous and call
forwardSignal, which re-enters those handlers when the parent sends the signal
to itself; fix by making the handlers named functions and removing the
SIGINT/SIGTERM listeners (or remove both listeners) before calling forwardSignal
to avoid re-entry. Update the process.on('SIGINT', ...) and
process.on('SIGTERM', ...) blocks (and the duplicate blocks around forwardSignal
occurrences) to use named functions (e.g., onSigint, onSigterm), call
process.removeListener('SIGINT', onSigint) and process.removeListener('SIGTERM',
onSigterm) or process.removeAllListeners for that signal inside each handler,
then call forwardSignal('SIGINT'/'SIGTERM').
In `@packages/cli/src/project/scaffold/project-renderers.ts`:
- Around line 545-577: upsertEnvContents currently treats comment/blank lines
and repeated additions as missing because existingKeys only tracks keys from
existingContents; fix by normalizing and de-duplicating the additions before
computing missingLines: for each entry in additions, skip empty or comment lines
(e.g. lines starting with '#'), extract the key using parseEnvKey (like
existingKeys does), trim it, and maintain a local Set (e.g. additionsKeys) to
ignore duplicate keys within the same additions array; finally compute
missingLines as those normalized, unique addition lines whose key is not in
existingKeys so comments and duplicate assignments are not repeatedly appended.
In `@packages/cli/src/project/scaffold/workspace-renderers.ts`:
- Around line 4-20: The returned default ignore array in workspace-renderers.ts
(used by scaffoldProject()) is missing storage runtime files; update the
returned array (the ignore list) to include storage paths such as "storage",
"storage/database.sqlite" and "storage/framework" (or more specific subfolders
like "storage/framework/sessions" and "storage/framework/cache") so
scaffoldProject() does not accidentally commit runtime DB/cache/session files;
locate the array literal returned in workspace-renderers.ts and append these
entries to the list.
In `@packages/cli/tests/cli.test.ts`:
- Around line 1415-1417: The test currently only checks stdout for 'dev' which
can pass even if the Next runner fails; update the assertions after calling
runNodeScript(nextRoot, join(nextRoot, '.holo-js/framework/run.mjs'), ['dev'])
(and optionally the 'build' call) to also assert the process exited successfully
by checking the returned object's status/exitCode is 0 (e.g.,
expect(result.status).toBe(0) or expect(result.exitCode).toBe(0)) so the test
verifies the runner completed without error; locate the runNodeScript calls and
add the status/exitCode assertions for the dev (and build) runs.
In `@packages/db/src/model/entityRuntime.ts`:
- Around line 51-58: The equality check currently only compares entry counts and
then recurses, which treats objects with different keys (e.g., {a:undefined} vs
{b:undefined}) as equal; update the comparison in the function in
entityRuntime.ts to first compare the sets of keys (use Object.keys or
leftEntries.map(k=>k[0]) and rightEntries.map...) and ensure every key from left
exists in right (and vice versa) before calling valuesAreEqual for each
corresponding value; modify the branch that uses leftEntries/rightEntries so it
verifies key equality (exact same keys) and then iterates
valuesAreEqual(left[key], right[key]) for each key.
In `@scripts/run-eslint.mjs`:
- Around line 35-45: The retry flag retriedAfterCacheReset is currently shared
across all iterations of the groups loop, so only the first group gets a
cache-reset retry; move the retry scope inside each group iteration (e.g.,
declare retriedAfterCacheReset at the top of the for (const targets of groups)
block) or track retries per-group so that when run(targets) throws and
shouldResetCache(error) is true you call clearCache(cacheLocation) and retry
run(targets) for that particular group using the existing shouldResetCache(),
clearCache(), and run() calls.
In `@scripts/run-generated-eslint.mjs`:
- Around line 12-22: The retry budget is currently global via
retriedAfterCacheReset, so move the retry tracking to be per-group (e.g.,
declare retriedAfterCacheReset inside the for (const group of groups) loop or
use a Map keyed by group) so each group can independently perform one
cache-reset retry; keep the same logic around shouldResetCache(error),
clearCache(cacheLocation) and the second await run(group) but ensure
retriedAfterCacheReset is initialized fresh for each group and updated only for
that group to prevent the global one-time limit.
---
Outside diff comments:
In `@packages/cli/tests/cli.test.ts`:
- Around line 9318-9325: Restore the explicit pre-materialization assertion:
after the first withFakeBun + cliInternals.runProjectPrepare call,
loadGeneratedProjectRegistry(projectRoot) should assert that the registry's
models array exists but does NOT include the pending model file (e.g.,
"server/models/Course.ts" or a model named "Course")—replace the generic
expect.any(Array) check with an assertion like
expect(models).toEqual(expect.not.arrayContaining([expect.objectContaining({
file: expect.stringContaining('server/models/Course.ts') })])) (or equivalent)
so the test verifies the Course model is absent before
server/db/schema.generated.ts is materialized.
In `@packages/db/src/model/defineModel.ts`:
- Around line 161-196: The current validation uses the one-time snapshot
resolvedAtDefinition and can miss late-loaded generated tables; change it to run
validateUniqueIdConfig when the lazy resolver actually resolves the table:
whenever resolveTable() returns a table (i.e., not undefined/null) call
validateUniqueIdConfig with that resolved table, inferredName and the runtime
result of resolveUniqueId(), rather than only checking resolvedAtDefinition;
specifically update the logic around
resolvedAtDefinition/resolveTable/resolveUniqueId to invoke
validateUniqueIdConfig after resolveTable() succeeds so bad
uniqueIds/newUniqueId are caught as soon as the generated schema is available.
---
Nitpick comments:
In `@packages/cli/tests/cli.test.ts`:
- Around line 1270-1277: The test hard-codes the esbuild version string; update
the assertion in the test (the call to
projectInternals.renderScaffoldPackageJson) to reference the shared
ESBUILD_PACKAGE_VERSION constant instead of the literal '"esbuild": "^0.27.4"';
import ESBUILD_PACKAGE_VERSION from the metadata module (where it’s defined) and
assert that the rendered scaffold contains the package entry built from that
constant so the test stays in sync with packages/cli/src/metadata.ts and the
renderScaffoldPackageJson behavior.
🪄 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: 543e1cfd-f3cc-44ae-8156-deb22f5f99bb
📒 Files selected for processing (17)
packages/cli/src/project/registry.tspackages/cli/src/project/scaffold.tspackages/cli/src/project/scaffold/config-renderers.tspackages/cli/src/project/scaffold/dependencies.tspackages/cli/src/project/scaffold/framework.tspackages/cli/src/project/scaffold/project-renderers.tspackages/cli/src/project/scaffold/types.tspackages/cli/src/project/scaffold/workspace-renderers.tspackages/cli/tests/cli.test.tspackages/db/src/model/Entity.tspackages/db/src/model/ModelRepository.tspackages/db/src/model/defineModel.tspackages/db/src/model/defineModelHelpers.tspackages/db/src/model/entityRuntime.tspackages/db/src/model/staticModelApi.tsscripts/run-eslint.mjsscripts/run-generated-eslint.mjs
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/cli/tests/cli.test.ts (1)
69-69: ⚡ Quick winUse
ESBUILD_PACKAGE_VERSIONconsistently across this test file.Nice move introducing
ESBUILD_PACKAGE_VERSION, but a couple of assertions still hardcode'^0.27.4'(for example Line 1538 and Line 1588), which can drift.♻️ Suggested follow-up diff
- expect(packageJson.dependencies?.esbuild).toBe('^0.27.4') + expect(packageJson.dependencies?.esbuild).toBe(ESBUILD_PACKAGE_VERSION) ... - expect(packageJson.dependencies?.esbuild).toBe('^0.27.4') + expect(packageJson.dependencies?.esbuild).toBe(ESBUILD_PACKAGE_VERSION)Also applies to: 1277-1277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/cli.test.ts` at line 69, Replace all remaining hardcoded esbuild version assertions ('^0.27.4') in the test file with the imported ESBUILD_PACKAGE_VERSION constant so the test uses ESBUILD_PACKAGE_VERSION consistently; search the file for the literal '^0.27.4' (e.g., in assertions that expect the esbuild dependency) and update those assertions to reference ESBUILD_PACKAGE_VERSION, ensuring the existing import of ESBUILD_PACKAGE_VERSION, HOLO_PACKAGE_VERSION remains used.
🤖 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/project/scaffold/dependencies.ts`:
- Around line 247-248: The current logic uses cachePackageInstalled (computed
from dependencies and devDependencies) as an input-only requirement, preventing
removal of `@holo-js/cache` when config/cache.* is deleted; change the logic so
the presence of the package is considered desired only when the cache config
exists — compute a cacheManaged (or cacheDesired) boolean from the repository
config (e.g., existence of config/cache.*) and use that to decide adding vs
removing, while still reading cachePackageInstalled from
dependencies/devDependencies; update any sync/remove code that references
cachePackageInstalled (and the related block around the existing cache
install/remove handling) to remove `@holo-js/cache` when cacheDesired is false
even if cachePackageInstalled is true.
- Around line 641-672: The code only adds packages for the detected framework
via requestedPackages but does not remove stale framework-specific packages from
dependencies/devDependencies; update the loop logic so that you define a
complete set of all framework-specific package names (e.g.,
'@holo-js/flux-react', '@holo-js/adapter-next', '@holo-js/flux-vue',
'@holo-js/adapter-nuxt', '@holo-js/flux-svelte', '@holo-js/adapter-sveltekit')
and then iterate that full set: for each package, if it is not in
requestedPackages remove it from dependencies and devDependencies (setting
changed = true), and if it is in requestedPackages ensure it is set to
nextVersion in dependencies and removed from devDependencies (as current code
does); keep using the existing variables requestedPackages, dependencies,
devDependencies, nextVersion, changed, and writePackageJsonDependencyState/
packageJsonPath/parsed so stale framework adapters/flux packages are pruned when
the framework changes.
In `@packages/cli/src/project/scaffold/framework.ts`:
- Around line 469-476: The SvelteKit scaffold currently creates the health
endpoint at 'src/routes/api/holo/+server.ts' which yields /api/holo, but
Nuxt/Next produce /api/holo/health; update the Svelte scaffold entry to create
'src/routes/api/holo/health/+server.ts' (i.e. change the path used when calling
renderSvelteHealthRoute) so the public health-check URL is consistent across
frameworks, and verify renderSvelteHealthRoute still returns the correct handler
for that new path.
- Around line 772-776: The scaffold sets dependencies['vue-router'] = '^5.0.4'
when options.framework === 'nuxt', which is incompatible with Nuxt 3; change the
assignment in that block (the dependencies['vue-router'] line) to a Nuxt
3–compatible version (e.g. '^4.1.6' or '^4.x') so new Nuxt scaffolds install
correctly.
In `@packages/db/src/model/defineModel.ts`:
- Line 161: The code currently uses resolveGeneratedModelTableSafely which
suppresses SchemaError from resolveGeneratedModelTable, allowing defineModel to
register a model without its generated table; change this so the SchemaError
fails fast: remove or alter the suppression in resolveGeneratedModelTableSafely
(or stop calling it) so that resolveGeneratedModelTable's SchemaError propagates
up to defineModel when a generated table is missing, causing defineModel to
throw immediately (so references to definition.table cannot be created on models
without backing tables); ensure no catch blocks quietly swallow SchemaError
(rethrow if caught) and update all call sites (including the other occurrences
around the 909–917 region) to preserve fail-fast behavior.
In `@packages/db/src/model/entityRuntime.ts`:
- Around line 137-147: Wrap the synchronous call to
repo.resolveRelationProperty(entity, key) inside loadRelation with a try/catch
so any synchronous throw (e.g., HydrationError triggered by preventLazyLoading)
is converted into a rejected Promise; specifically, update the loadRelation
function used by relationMethod.then / .catch / .finally to try { return
Promise.resolve(repo.resolveRelationProperty(entity, key)) } catch (err) {
return Promise.reject(err) } so callers always receive a promise rejection
instead of an uncaught throw.
In `@scripts/run-eslint.mjs`:
- Around line 122-135: The process currently listens for child.on('exit') to
decide resolve/reject using the accumulated stderr, but exit can fire before
stderr is fully drained causing truncated messages and breaking
shouldResetCache() checks; change the listener to child.on('close') (which fires
after all stdio streams are closed) and keep the same resolve/reject logic using
the existing stderr variable and the same error message construction so retries
based on shouldResetCache() will see the complete stderr content.
In `@scripts/run-generated-eslint.mjs`:
- Around line 102-115: The failure handling is using child.on('exit', ...) which
can fire before trailing stderr is flushed, causing shouldResetCache() to miss
error patterns; change the listener to child.on('close', ...) (or add a close
handler and move the resolve/reject logic there) so you wait for all stdio to be
closed before rejecting; keep accumulating stderr in child.stderr.on('data',
...) and when the child emits 'close' use the same code path to resolve on
code===0 or reject(new Error(stderr || `Generated ESLint failed for:
${targets[0]}`)) so shouldResetCache() sees complete stderr output.
---
Nitpick comments:
In `@packages/cli/tests/cli.test.ts`:
- Line 69: Replace all remaining hardcoded esbuild version assertions
('^0.27.4') in the test file with the imported ESBUILD_PACKAGE_VERSION constant
so the test uses ESBUILD_PACKAGE_VERSION consistently; search the file for the
literal '^0.27.4' (e.g., in assertions that expect the esbuild dependency) and
update those assertions to reference ESBUILD_PACKAGE_VERSION, ensuring the
existing import of ESBUILD_PACKAGE_VERSION, HOLO_PACKAGE_VERSION remains used.
🪄 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: f3e78ec3-147f-42a5-9513-debddde8f7ee
📒 Files selected for processing (10)
packages/cli/src/project/scaffold/dependencies.tspackages/cli/src/project/scaffold/framework.tspackages/cli/src/project/scaffold/project-renderers.tspackages/cli/src/project/scaffold/workspace-renderers.tspackages/cli/tests/broadcast.test.tspackages/cli/tests/cli.test.tspackages/db/src/model/defineModel.tspackages/db/src/model/entityRuntime.tsscripts/run-eslint.mjsscripts/run-generated-eslint.mjs
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/project/scaffold/workspace-renderers.ts
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/db/src/model/defineModel.ts (1)
211-213: 💤 Low valueConditional is now always true after fail-fast fix.
Since
resolveGeneratedModelTable(tableName)at line 161 now throws if the table doesn't exist,resolvedAtDefinitionwill always be truthy when execution reaches line 211. Theif (resolvedAtDefinition)check is redundant.Consider simplifying:
Suggested simplification
- if (resolvedAtDefinition) { - validateUniqueIdConfig(resolvedAtDefinition, inferredName, resolveUniqueId()) - } + validateUniqueIdConfig(resolvedAtDefinition, inferredName, resolveUniqueId())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/defineModel.ts` around lines 211 - 213, The check "if (resolvedAtDefinition)" is redundant because resolveGeneratedModelTable(tableName) now throws for missing tables, so always call validateUniqueIdConfig; remove the surrounding conditional and invoke validateUniqueIdConfig(resolvedAtDefinition, inferredName, resolveUniqueId()) directly after resolvedAtDefinition is obtained, keeping the same parameters and preserving error behavior from resolveGeneratedModelTable.
🤖 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/blog-nuxt/server/models/User.ts`:
- Line 6: The User model's fillable array currently includes the trust-sensitive
field email_verified_at; remove 'email_verified_at' from the fillable array in
the User model (the fillable property in models/User.ts or the User class) so it
cannot be mass-assigned, and instead ensure verification logic updates this
field explicitly (e.g., via a dedicated method like verifyEmail or within the
auth controller) so only trusted code paths can set email_verified_at.
In `@apps/blog-sveltekit/server/models/User.ts`:
- Line 6: The fillable array in User.ts currently includes 'email_verified_at',
allowing callers to mass-assign verification state; remove 'email_verified_at'
from the fillable list and ensure verification is set only via a dedicated
method (e.g., User.verifyEmail() or a controller/service that sets
email_verified_at) or by marking it as guarded/hidden so create/update payloads
cannot set it directly; update any tests or factories that relied on
mass-assignment to use the dedicated setter instead.
In `@apps/Next_test_app/.holo-js/framework/run.mjs`:
- Around line 149-167: The current stopStaleNextDevServer function
unconditionally sends SIGTERM when isOwnedNextDevServer returns true, which can
kill a legitimately running dev server; change the logic so it does not
auto-terminate live servers: update stopStaleNextDevServer to accept an optional
force flag (or check lock age) and only call process.kill(pid, 'SIGTERM') when
force===true or when the lock/ownership metadata indicates the owner is stale
(e.g., lock mtime older than a configurable threshold); otherwise return false
and let the caller present a prompt or take explicit action. Reference:
stopStaleNextDevServer and isOwnedNextDevServer — add the force parameter and/or
stale-age check and gate the process.kill call behind that condition.
In `@packages/cli/src/project/scaffold/dependencies.ts`:
- Around line 497-510: The focused upsert helpers (e.g.,
upsertEventsPackageDependency) are missing the companion packages modeled by
syncManagedDriverDependencies (events should also add `@holo-js/queue`; auth
should also add `@holo-js/security`), causing a transient mismatch with the full
resolver; update upsertEventsPackageDependency to also set
dependencies['@holo-js/queue'] = nextVersion and delete any
devDependencies['@holo-js/queue'] (using the same nextVersion computed from
HOLO_PACKAGE_VERSION) before calling writePackageJsonDependencyState, and make
the analogous change in the auth helper (the upsertAuthPackageDependency
function) to add dependencies['@holo-js/security'] = nextVersion and remove it
from devDependencies so the focused installers produce the same final
package.json as the full sync.
In `@packages/cli/src/project/scaffold/framework.ts`:
- Around line 120-139: Update the dependency mappings so the scaffolded
package.json matches the canonical mapping in dependencies.ts: when
optionalPackages.includes('events') add '@holo-js/queue' (the events companion)
instead of '@holo-js/events', and when optionalPackages.includes('auth') add
'@holo-js/security' (the auth companion) along with the existing session entry;
modify the branches that set dependencies['@holo-js/events'] and
dependencies['@holo-js/auth'] to use dependencies['@holo-js/queue'] and
dependencies['@holo-js/security'] respectively so the initial scaffold contains
the companion packages up front.
- Around line 185-189: The lint command strings in scaffoldProject (the ternary
that sets the lint property based on options.framework) include a non-existent
"tests" folder which causes ESLint to error on fresh projects; update the three
branches (nuxt, next, default) in the lint assignment to either remove "tests"
from the paths or append "--no-error-on-unmatched-pattern" to each command
(e.g., change '... tests --fix --no-warn-ignored' to '... --fix
--no-warn-ignored' or '... tests --fix --no-warn-ignored
--no-error-on-unmatched-pattern') so the generated npm run lint will not fail
for new apps.
In `@packages/cli/tests/cli.test.ts`:
- Around line 9334-9339: The negative assertion is checking for an object with a
"file" property but registry.models is an array of registered path strings, so
the test is ineffective; update the assertion to check the actual element shape
by asserting registry.models does not contain the string
'server/models/Course.ts' (i.e. replace the expect.objectContaining({ file: ...
}) pattern with a direct string match against registry.models) so the test
correctly fails if that path is present; locate the assertion referencing
registry.models and adjust it accordingly.
---
Nitpick comments:
In `@packages/db/src/model/defineModel.ts`:
- Around line 211-213: The check "if (resolvedAtDefinition)" is redundant
because resolveGeneratedModelTable(tableName) now throws for missing tables, so
always call validateUniqueIdConfig; remove the surrounding conditional and
invoke validateUniqueIdConfig(resolvedAtDefinition, inferredName,
resolveUniqueId()) directly after resolvedAtDefinition is obtained, keeping the
same parameters and preserving error behavior from resolveGeneratedModelTable.
🪄 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: e80021bd-69f8-4343-bacc-50a451ef32bd
📒 Files selected for processing (22)
apps/Next_test_app/.holo-js/framework/run.mjsapps/blog-next/server/models/Category.tsapps/blog-next/server/models/Post.tsapps/blog-next/server/models/Tag.tsapps/blog-next/server/models/User.tsapps/blog-nuxt/server/models/Category.tsapps/blog-nuxt/server/models/Post.tsapps/blog-nuxt/server/models/Tag.tsapps/blog-nuxt/server/models/User.tsapps/blog-sveltekit/server/models/Category.tsapps/blog-sveltekit/server/models/Post.tsapps/blog-sveltekit/server/models/Tag.tsapps/blog-sveltekit/server/models/User.tspackages/cli/src/project/scaffold/dependencies.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/src/project/scaffold/framework.tspackages/cli/tests/cli.test.tspackages/db/src/model/defineModel.tspackages/db/src/model/entityRuntime.tspackages/db/tests/model-core.test.tsscripts/run-eslint.mjsscripts/run-generated-eslint.mjs
✅ Files skipped from review due to trivial changes (1)
- packages/db/src/model/entityRuntime.ts
| import { defineModel } from '@holo-js/db' | ||
|
|
||
| export default defineModel('users', { | ||
| fillable: ['name', 'email', 'password', 'avatar', 'email_verified_at'], |
There was a problem hiding this comment.
Do not keep email_verified_at mass-assignable.
Line 6 permits direct write access to a trust-sensitive field (email_verified_at), which weakens verification guarantees in typical auth flows.
Suggested fix
export default defineModel('users', {
- fillable: ['name', 'email', 'password', 'avatar', 'email_verified_at'],
+ fillable: ['name', 'email', 'password', 'avatar'],
hidden: ['password'],
})📝 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
| fillable: ['name', 'email', 'password', 'avatar', 'email_verified_at'], | |
| export default defineModel('users', { | |
| fillable: ['name', 'email', 'password', 'avatar'], | |
| hidden: ['password'], | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blog-nuxt/server/models/User.ts` at line 6, The User model's fillable
array currently includes the trust-sensitive field email_verified_at; remove
'email_verified_at' from the fillable array in the User model (the fillable
property in models/User.ts or the User class) so it cannot be mass-assigned, and
instead ensure verification logic updates this field explicitly (e.g., via a
dedicated method like verifyEmail or within the auth controller) so only trusted
code paths can set email_verified_at.
| import { defineModel } from '@holo-js/db' | ||
|
|
||
| export default defineModel('users', { | ||
| fillable: ['name', 'email', 'password', 'avatar', 'email_verified_at'], |
There was a problem hiding this comment.
Avoid mass-assigning email_verified_at in the base user model.
Line 6 allows callers to set verification state through normal create/update payloads, which can bypass email verification flows.
Suggested fix
export default defineModel('users', {
- fillable: ['name', 'email', 'password', 'avatar', 'email_verified_at'],
+ fillable: ['name', 'email', 'password', 'avatar'],
hidden: ['password'],
})📝 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
| fillable: ['name', 'email', 'password', 'avatar', 'email_verified_at'], | |
| export default defineModel('users', { | |
| fillable: ['name', 'email', 'password', 'avatar'], | |
| hidden: ['password'], | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blog-sveltekit/server/models/User.ts` at line 6, The fillable array in
User.ts currently includes 'email_verified_at', allowing callers to mass-assign
verification state; remove 'email_verified_at' from the fillable list and ensure
verification is set only via a dedicated method (e.g., User.verifyEmail() or a
controller/service that sets email_verified_at) or by marking it as
guarded/hidden so create/update payloads cannot set it directly; update any
tests or factories that relied on mass-assignment to use the dedicated setter
instead.
Comment on lines
+149
to
+167
| async function stopStaleNextDevServer(pid, reportedDir) { | ||
| if (!Number.isInteger(pid) || pid <= 0 || pid === process.pid) { | ||
| return false | ||
| } | ||
|
|
||
| if (!isOwnedNextDevServer(pid, reportedDir)) { | ||
| 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) |
There was a problem hiding this comment.
Don’t auto-terminate a live Next dev server during conflict recovery
At Line 159, this kills the reported PID whenever ownership checks pass. That can terminate a legitimately running dev server (same project, different terminal), not just a stale lock owner.
Suggested safer recovery behavior
async function stopStaleNextDevServer(pid, reportedDir) {
if (!Number.isInteger(pid) || pid <= 0 || pid === process.pid) {
return false
}
if (!isOwnedNextDevServer(pid, reportedDir)) {
return false
}
try {
- process.kill(pid, 'SIGTERM')
+ // Treat as stale only when the reported PID no longer exists.
+ process.kill(pid, 0)
} catch (error) {
if (error && typeof error === 'object' && 'code' in error && error.code === 'ESRCH') {
return true
}
return false
}
- return waitForProcessExit(pid)
+ // Live process found: avoid killing an active dev server automatically.
+ return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/Next_test_app/.holo-js/framework/run.mjs` around lines 149 - 167, The
current stopStaleNextDevServer function unconditionally sends SIGTERM when
isOwnedNextDevServer returns true, which can kill a legitimately running dev
server; change the logic so it does not auto-terminate live servers: update
stopStaleNextDevServer to accept an optional force flag (or check lock age) and
only call process.kill(pid, 'SIGTERM') when force===true or when the
lock/ownership metadata indicates the owner is stale (e.g., lock mtime older
than a configurable threshold); otherwise return false and let the caller
present a prompt or take explicit action. Reference: stopStaleNextDevServer and
isOwnedNextDevServer — add the force parameter and/or stale-age check and gate
the process.kill call behind that condition.
Comment on lines
+497
to
+510
| async function upsertEventsPackageDependency(projectRoot: string): Promise<boolean> { | ||
| const { packageJsonPath, parsed, dependencies, devDependencies } = await readPackageJsonDependencyState(projectRoot) | ||
| const nextVersion = `^${HOLO_PACKAGE_VERSION}` | ||
| const currentVersion = dependencies['@holo-js/events'] | ||
| const currentDevVersion = devDependencies['@holo-js/events'] | ||
|
|
||
| if (currentVersion === nextVersion && typeof currentDevVersion === 'undefined') { | ||
| return false | ||
| } | ||
|
|
||
| dependencies['@holo-js/events'] = nextVersion | ||
| delete devDependencies['@holo-js/events'] | ||
|
|
||
| await writePackageJsonDependencyState(packageJsonPath, parsed, dependencies, devDependencies) |
There was a problem hiding this comment.
Keep the focused auth/events installers aligned with the full resolver.
syncManagedDriverDependencies() in this file already models events -> @holo-js/queue and `auth -> `@holo-js/security, but these narrower upsert helpers omit those companion packages. That leaves install flows with a package.json the full sync would immediately “fix” afterward.
Suggested fix
async function upsertEventsPackageDependency(projectRoot: string): Promise<boolean> {
const { packageJsonPath, parsed, dependencies, devDependencies } = await readPackageJsonDependencyState(projectRoot)
const nextVersion = `^${HOLO_PACKAGE_VERSION}`
const currentVersion = dependencies['@holo-js/events']
+ const currentQueueVersion = dependencies['@holo-js/queue']
const currentDevVersion = devDependencies['@holo-js/events']
+ const currentDevQueueVersion = devDependencies['@holo-js/queue']
- if (currentVersion === nextVersion && typeof currentDevVersion === 'undefined') {
+ if (
+ currentVersion === nextVersion
+ && currentQueueVersion === nextVersion
+ && typeof currentDevVersion === 'undefined'
+ && typeof currentDevQueueVersion === 'undefined'
+ ) {
return false
}
dependencies['@holo-js/events'] = nextVersion
+ dependencies['@holo-js/queue'] = nextVersion
delete devDependencies['@holo-js/events']
+ delete devDependencies['@holo-js/queue']
await writePackageJsonDependencyState(packageJsonPath, parsed, dependencies, devDependencies)
return true
}
@@
const requestedPackages = {
'@holo-js/auth': true,
'@holo-js/session': true,
+ '@holo-js/security': true,
'@holo-js/auth-social': socialEnabled,
'@holo-js/auth-workos': features.workos === true,
'@holo-js/auth-clerk': features.clerk === true,
} as constAlso applies to: 718-724
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/project/scaffold/dependencies.ts` around lines 497 - 510,
The focused upsert helpers (e.g., upsertEventsPackageDependency) are missing the
companion packages modeled by syncManagedDriverDependencies (events should also
add `@holo-js/queue`; auth should also add `@holo-js/security`), causing a transient
mismatch with the full resolver; update upsertEventsPackageDependency to also
set dependencies['@holo-js/queue'] = nextVersion and delete any
devDependencies['@holo-js/queue'] (using the same nextVersion computed from
HOLO_PACKAGE_VERSION) before calling writePackageJsonDependencyState, and make
the analogous change in the auth helper (the upsertAuthPackageDependency
function) to add dependencies['@holo-js/security'] = nextVersion and remove it
from devDependencies so the focused installers produce the same final
package.json as the full sync.
Comment on lines
+120
to
+139
| if (optionalPackages.includes('events')) { | ||
| dependencies['@holo-js/events'] = `^${HOLO_PACKAGE_VERSION}` | ||
| } | ||
|
|
||
| if (optionalPackages.includes('queue')) { | ||
| dependencies['@holo-js/queue'] = `^${HOLO_PACKAGE_VERSION}` | ||
| } | ||
|
|
||
| if (optionalPackages.includes('validation')) { | ||
| dependencies['@holo-js/validation'] = `^${HOLO_PACKAGE_VERSION}` | ||
| } | ||
|
|
||
| if (optionalPackages.includes('forms')) { | ||
| dependencies['@holo-js/forms'] = `^${HOLO_PACKAGE_VERSION}` | ||
| } | ||
|
|
||
| if (optionalPackages.includes('auth')) { | ||
| dependencies['@holo-js/auth'] = `^${HOLO_PACKAGE_VERSION}` | ||
| dependencies['@holo-js/session'] = `^${HOLO_PACKAGE_VERSION}` | ||
| } |
There was a problem hiding this comment.
Scaffold the auth/events companion packages up front.
The initial package.json here disagrees with packages/cli/src/project/scaffold/dependencies.ts: auth implies @holo-js/security there, and events imply @holo-js/queue. Fresh scaffolds will start from an incomplete dependency set until some later sync step runs.
Suggested fix
if (optionalPackages.includes('events')) {
dependencies['@holo-js/events'] = `^${HOLO_PACKAGE_VERSION}`
+ dependencies['@holo-js/queue'] = `^${HOLO_PACKAGE_VERSION}`
}
@@
if (optionalPackages.includes('auth')) {
dependencies['@holo-js/auth'] = `^${HOLO_PACKAGE_VERSION}`
dependencies['@holo-js/session'] = `^${HOLO_PACKAGE_VERSION}`
+ dependencies['@holo-js/security'] = `^${HOLO_PACKAGE_VERSION}`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/project/scaffold/framework.ts` around lines 120 - 139,
Update the dependency mappings so the scaffolded package.json matches the
canonical mapping in dependencies.ts: when optionalPackages.includes('events')
add '@holo-js/queue' (the events companion) instead of '@holo-js/events', and
when optionalPackages.includes('auth') add '@holo-js/security' (the auth
companion) along with the existing session entry; modify the branches that set
dependencies['@holo-js/events'] and dependencies['@holo-js/auth'] to use
dependencies['@holo-js/queue'] and dependencies['@holo-js/security']
respectively so the initial scaffold contains the companion packages up front.
Comment on lines
+185
to
+189
| lint: options.framework === 'nuxt' | ||
| ? 'npx eslint app.vue config server tests --fix --no-warn-ignored' | ||
| : options.framework === 'next' | ||
| ? 'npx eslint app config server tests --fix --no-warn-ignored' | ||
| : 'npx eslint src config server tests --fix --no-warn-ignored', |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does ESLint CLI fail when a positional file or directory argument does not exist unless --no-error-on-unmatched-pattern is used?
💡 Result:
Yes—when you pass a glob/directory pattern that matches no files, ESLint’s CLI fails (non-zero exit) by default; using --no-error-on-unmatched-pattern suppresses that fatal “no files matching…” error. However, this is specifically about unmatched patterns (glob patterns and --ext/search behavior). It’s not the same as “a positional file/directory argument does not exist” in every case. Evidence: 1) ESLint documents that --no-error-on-unmatched-pattern “prevents errors when a quoted glob pattern is unmatched,” but “does not prevent errors when your shell can’t match a glob.” 2) ESLint issue discussion around the default behavior states that since ESLint v5, it throws a fatal error if a file pattern returns no files, and then an option was added to opt out. The “no files matching … were found” message is the one suppressed by --no-error-on-unmatched-pattern. 3) There are also reports about non-existent paths: older versions at times silently ignored missing files, but the CLI now (as reflected by the v5 behavior discussed in the issues) treats missing/non-matching targets as an error unless you opt out with the flag. So the direct answer to your question is: ESLint CLI does fail on “unmatched patterns” (including directory glob patterns that resolve to nothing) unless --no-error-on-unmatched-pattern is used; the flag is explicitly meant to prevent that specific failure mode.
Citations:
- 1: https://eslint.org/docs/latest/use/command-line-interface
- 2: Add an option to not throw an error on an unmatched pattern eslint/eslint#10587
- 3: https://hi.eslint.org/docs/latest/use/command-line-interface
- 4: eslint silently ignores missing files eslint/eslint#7390
- 5: Unable to run ESLint when folder missing (glob error) eslint/eslint#10528
- 6: eslint/eslint@1713d07
🏁 Script executed:
# First, find the scaffoldProject function to understand what it creates
rg -A 30 'function scaffoldProject|export.*scaffoldProject' packages/cli/src/project/scaffold/Repository: cobraprojects/holo-js
Length of output: 3157
🏁 Script executed:
# Also examine the framework.ts file around the lint commands
cat -n packages/cli/src/project/scaffold/framework.ts | head -200 | tail -50Repository: cobraprojects/holo-js
Length of output: 2339
🏁 Script executed:
# Search for where tests/ might be created in the scaffold
rg 'tests' packages/cli/src/project/scaffold/ -A 2 -B 2Repository: cobraprojects/holo-js
Length of output: 847
🏁 Script executed:
# Search for writeFile and mkdir calls in scaffoldProject to see what directories are created
rg 'writeFile|mkdir|createDir' packages/cli/src/project/scaffold/ -B 2 -A 2 | head -100Repository: cobraprojects/holo-js
Length of output: 9754
🏁 Script executed:
# Let's look at the full scaffoldProject function to see all files and directories it creates
cat -n packages/cli/src/project/scaffold/framework.ts | sed -n '200,400p'Repository: cobraprojects/holo-js
Length of output: 8506
🏁 Script executed:
# Search for tests directory creation anywhere in the scaffold directory
rg -r 'tests' packages/cli/src/project/scaffold/ --type ts | grep -i 'mkdir\|write\|create\|dir'Repository: cobraprojects/holo-js
Length of output: 47
Remove tests from the lint commands or add --no-error-on-unmatched-pattern.
scaffoldProject() never creates a tests/ directory, but all three lint commands include it. On ESLint v5+, this causes npm run lint to fail on fresh apps unless --no-error-on-unmatched-pattern is added.
Suggested fix
lint: options.framework === 'nuxt'
- ? 'npx eslint app.vue config server tests --fix --no-warn-ignored'
+ ? 'npx eslint app.vue config server tests --fix --no-warn-ignored --no-error-on-unmatched-pattern'
: options.framework === 'next'
- ? 'npx eslint app config server tests --fix --no-warn-ignored'
- : 'npx eslint src config server tests --fix --no-warn-ignored',
+ ? 'npx eslint app config server tests --fix --no-warn-ignored --no-error-on-unmatched-pattern'
+ : 'npx eslint src config server tests --fix --no-warn-ignored --no-error-on-unmatched-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
| lint: options.framework === 'nuxt' | |
| ? 'npx eslint app.vue config server tests --fix --no-warn-ignored' | |
| : options.framework === 'next' | |
| ? 'npx eslint app config server tests --fix --no-warn-ignored' | |
| : 'npx eslint src config server tests --fix --no-warn-ignored', | |
| lint: options.framework === 'nuxt' | |
| ? 'npx eslint app.vue config server tests --fix --no-warn-ignored --no-error-on-unmatched-pattern' | |
| : options.framework === 'next' | |
| ? 'npx eslint app config server tests --fix --no-warn-ignored --no-error-on-unmatched-pattern' | |
| : 'npx eslint src config server tests --fix --no-warn-ignored --no-error-on-unmatched-pattern', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/project/scaffold/framework.ts` around lines 185 - 189, The
lint command strings in scaffoldProject (the ternary that sets the lint property
based on options.framework) include a non-existent "tests" folder which causes
ESLint to error on fresh projects; update the three branches (nuxt, next,
default) in the lint assignment to either remove "tests" from the paths or
append "--no-error-on-unmatched-pattern" to each command (e.g., change '...
tests --fix --no-warn-ignored' to '... --fix --no-warn-ignored' or '... tests
--fix --no-warn-ignored --no-error-on-unmatched-pattern') so the generated npm
run lint will not fail for new apps.
Comment on lines
+9334
to
+9339
| expect(Array.isArray(registry.models)).toBe(true) | ||
| expect(registry.models).toEqual(expect.not.arrayContaining([ | ||
| expect.objectContaining({ | ||
| file: expect.stringContaining('server/models/Course.ts'), | ||
| }), | ||
| ])) |
There was a problem hiding this comment.
Assert against the actual registry.models element shape.
This negative check is currently ineffective. Later in this file, models is treated as an array of registered path strings, so objectContaining({ file: ... }) will still pass even if server/models/Course.ts is present.
♻️ Proposed fix
- expect(registry.models).toEqual(expect.not.arrayContaining([
- expect.objectContaining({
- file: expect.stringContaining('server/models/Course.ts'),
- }),
- ]))
+ expect(registry.models).not.toContain('server/models/Course.ts')📝 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
| expect(Array.isArray(registry.models)).toBe(true) | |
| expect(registry.models).toEqual(expect.not.arrayContaining([ | |
| expect.objectContaining({ | |
| file: expect.stringContaining('server/models/Course.ts'), | |
| }), | |
| ])) | |
| expect(Array.isArray(registry.models)).toBe(true) | |
| expect(registry.models).not.toContain('server/models/Course.ts') |
🤖 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 9334 - 9339, The negative
assertion is checking for an object with a "file" property but registry.models
is an array of registered path strings, so the test is ineffective; update the
assertion to check the actual element shape by asserting registry.models does
not contain the string 'server/models/Course.ts' (i.e. replace the
expect.objectContaining({ file: ... }) pattern with a direct string match
against registry.models) so the test correctly fails if that path is present;
locate the assertion referencing registry.models and adjust it accordingly.
This was referenced May 5, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit