fix user facing api#11
Merged
Merged
Conversation
|
Warning Rate limit exceeded
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 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds framework-specific prepare/sync hooks and SvelteKit-generated hooks; extends DB model APIs with relation callables and JSON-returning query helpers plus recursive model serialization; introduces a SvelteKit transport and public storage response handler; tightens CLI discovery/watch and scaffold outputs (VSCode, schema wiring); updates workspace scripts/deps and many tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant FS as FileSystem/Discovery
participant PM as PackageManager
participant Framework as FrameworkProcess
CLI->>FS: read .holo-js/framework/project.json
alt framework == "nuxt"
CLI->>PM: spawn detected PM with ["npx","nuxi","prepare"] (inherit stdio)
else framework == "sveltekit"
CLI->>PM: spawn detected PM with ["bun","x","svelte-kit","sync"] (inherit stdio)
end
PM->>Framework: run prepare/sync command
Framework-->>PM: exit (0/!=0)
PM-->>CLI: forward exit code (reject on non-zero)
CLI->>FS: refresh discovery (exclude `.holo-js/generated`, include generated schema path)
CLI->>PM: sync managed dependencies
CLI->>FS: write generated artifacts (.holo-js/generated/..., server/db/schema.generated.ts)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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 41 minutes and 35 seconds.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: 16
🧹 Nitpick comments (5)
packages/cli/src/templates.ts (1)
84-105: Remove unusedgeneratedSchemaImportPathparameter.The
generatedSchemaImportPathparameter is declared but never used in the function body. Removing it requires updating callers inpackages/cli/src/generators.ts(line 231) and multiple test cases inpackages/cli/tests/cli.test.ts(lines 8629–8656, 9304–9307).♻️ Proposed changes
export function renderModelTemplate(options: { tableName: string - generatedSchemaImportPath: string observerImportPath?: string observerClassName?: string }): string {Update callers to remove the
generatedSchemaImportPathproperty from the options object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/templates.ts` around lines 84 - 105, The renderModelTemplate function signature includes an unused parameter generatedSchemaImportPath; remove this parameter from the options type and function signature in renderModelTemplate and update all callers that pass an options object (notably the call site in generators where renderModelTemplate({...}) is invoked and the unit tests that construct the options) to stop providing generatedSchemaImportPath; also update any test fixtures/expectations that relied on the old signature and run tests to ensure no remaining references to generatedSchemaImportPath remain.packages/adapter-sveltekit/tests/adapter.type.test.ts (1)
7-7: Add a published-package smoke test for./transporttoo.This only exercises
../src/transport, so it will still pass ifdist/transport.d.tsor thepackage.jsonsubpath export is wrong. Since this PR adds a user-facing entrypoint, I'd add a NodeNext check that imports@holo-js/adapter-sveltekit/transport, mirroring the existing client assertion.Also applies to: 65-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/adapter-sveltekit/tests/adapter.type.test.ts` at line 7, Add a published-package smoke test that imports the transport entrypoint from the packaged subpath and asserts types just like the existing client check: in tests/adapter.type.test.ts add an import using the package subpath (e.g. import type { SerializedSvelteKitData } from '@holo-js/adapter-sveltekit/transport') and mirror the existing assertion/assertType call that currently targets ../src/transport so the NodeNext packaged export is exercised; also replicate this change for the other assertion blocks referenced around lines 65-101.packages/cli/src/project/registry.ts (2)
771-786: Redundantnuxtbranch returns the same value as the default.The
if (manifest.framework === 'nuxt')branch returns'../../tsconfig.json', which is identical to the fallback on line 785. Either the Nuxt-specific behavior isn't implemented yet, or this branch can be removed.🔧 Suggested simplification (if no Nuxt-specific behavior is needed)
async function resolveGeneratedTsconfigBase(projectRoot: string): Promise<string> { const frameworkProjectPath = resolve(projectRoot, '.holo-js/framework/project.json') try { const content = await readFile(frameworkProjectPath, 'utf8') const manifest = JSON.parse(content) as { framework?: string } - if (manifest.framework === 'nuxt') { - // For existing Nuxt projects, use root tsconfig.json which extends .nuxt/tsconfig.json - return '../../tsconfig.json' - } + // Future: add framework-specific tsconfig base resolution here if needed } catch { // Not a framework project } return '../../tsconfig.json' }🤖 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 771 - 786, The resolveGeneratedTsconfigBase function contains a redundant manifest.framework === 'nuxt' branch returning the same '../../tsconfig.json' as the fallback; remove the unnecessary conditional (the if block checking manifest.framework === 'nuxt') or replace it with the actual Nuxt-specific path logic if intended, ensuring the function still reads framework/project.json and returns the correct tsconfig path when a Nuxt framework is detected.
268-280: Regex pattern may not match all valid config formats.The regex
/(kit:\s*\{[^\n]*\n)/requires a newline afterkit: {. Single-line configs likekit: {}or configs with no trailing newline won't be patched. The function handles this gracefully by returningundefined, but users with non-standard formatting may need to manually add the hooks override.This is acceptable for an automated migration, but consider documenting this limitation.
🤖 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 268 - 280, The current regex in patchSvelteConfigWithHooksOverride (/(kit:\s*\{[^\n]*\n)/) misses single-line or no-newline Svelte kit blocks; update the matcher to handle `kit: {}` on a single line or blocks without a trailing newline and insert SVELTE_HOOKS_OVERRIDE_BLOCK appropriately. Specifically, change the pattern used in contents.replace to match either an opening brace with the rest of the line (with or without newline) or a closing brace on the same line (e.g., match `kit:\s*\{[^\n]*\n?` or `kit:\s*\{[^}]*\}`), then inject SVELTE_HOOKS_OVERRIDE_BLOCK after the matched opening (or before the closing brace) so single-line `kit: {}` and files without trailing newline are patched; keep returning undefined when no replacement occurred. Ensure the change targets patchSvelteConfigWithHooksOverride and still respects existing svelteConfigHasHooksOverride guard.packages/db/src/model/ModelQueryBuilder.ts (1)
993-1002: Consider centralizing paginator JSON return types to reduce cast drift.The repeated inline
as { data, meta/... }shapes are easy to desync. A shared type alias per paginator JSON shape would make future changes safer.♻️ Optional refactor sketch
+type PaginateJsonResult<TTable extends TableDefinition, TLoaded> = { + data: readonly SerializedEntityWithLoaded<TTable, TLoaded>[] + meta: PaginationMeta +} + +type SimplePaginateJsonResult<TTable extends TableDefinition, TLoaded> = { + data: readonly SerializedEntityWithLoaded<TTable, TLoaded>[] + meta: SimplePaginationMeta +} + +type CursorPaginateJsonResult<TTable extends TableDefinition, TLoaded> = { + data: readonly SerializedEntityWithLoaded<TTable, TLoaded>[] + perPage: number + cursorName: string + nextCursor: string | null + prevCursor: string | null +}Also applies to: 1031-1040, 1064-1082
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/ModelQueryBuilder.ts` around lines 993 - 1002, Create a shared paginator JSON type alias and replace the repeated inline casts in paginateJson and the other similar methods (e.g., the methods around lines 1031-1040 and 1064-1082) with that alias to avoid drift; define something like PaginatorJSON<TData, TMeta> = { data: readonly TData[], meta: TMeta } (using the existing SerializedEntityWithLoaded<TTable, TLoaded> and PaginationMeta generics) either exported at the top of ModelQueryBuilder.ts or next to the paginate/paginateJson methods, then change returns like (await this.paginate(...)).toJSON() as PaginatorJSON<SerializedEntityWithLoaded<TTable, TLoaded>, PaginationMeta> so all paginator JSON shapes share one canonical type.
🤖 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 153-211: Both runNuxtPrepare and runSvelteKitSync currently spawn
tool binaries directly (spawn('nuxi', ...) and spawn('bun', ['x',
'svelte-kit','sync'], ...)) which breaks when the project uses npm/pnpm/yarn;
update each function to call resolveProjectPackageManager(projectRoot) and build
the command/args according to the returned manager: for npm use ['exec', '--',
'nuxi', 'prepare'], for pnpm use ['exec', 'nuxi', 'prepare'], for yarn use
['run', 'nuxi', 'prepare'], and for bun use ['x', 'nuxi', 'prepare'] (and
similarly for svelte-kit sync: npm ['exec','--','svelte-kit','sync'], pnpm
['exec','svelte-kit','sync'], yarn ['run','svelte-kit','sync'], bun
['x','svelte-kit','sync']); then spawn the package manager binary (manager
string) with those args, preserving cwd and stdio and existing close/error
handling.
In `@packages/cli/src/metadata.ts`:
- Line 4: Restore the ESBUILD_PACKAGE_VERSION constant to the prior range used
in scaffolding so projects aren't downgraded: change the export
ESBUILD_PACKAGE_VERSION back to '^0.27.4' (it is referenced by scaffold and
dependency-sync logic in project/scaffold.ts), ensuring any consumers that read
ESBUILD_PACKAGE_VERSION continue to receive the intended newer esbuild range.
- Line 15: Replace the non-pinned Nuxt entry in the metadata object (the "nuxt"
property in packages/cli/src/metadata.ts) with a tested semver range instead of
'latest'; update the value to a specific range that matches the scaffolded
companion dependencies (for example a Nuxt 3-compatible range) so installs are
reproducible—edit the "nuxt" property in the exported metadata object to a
pinned version range like '^3.x.x' (or the exact tested range your CI uses) and
run a quick install/test to confirm compatibility.
In `@packages/cli/src/project/scaffold.ts`:
- Around line 2837-2847: The VSCode settings generator renderVSCodeSettings is
adding a Vue-specific key ('vue.server.hybridMode') for both 'nuxt' and
'sveltekit'; update renderVSCodeSettings so it only includes
'vue.server.hybridMode': true when options.framework === 'nuxt' and omit that
key for 'sveltekit', building the settings object conditionally (keep
'typescript.tsdk' and 'typescript.enablePromptUseWorkspaceTsdk' for both) before
JSON.stringify so SvelteKit projects don't receive Vue-specific settings.
- Around line 3384-3412: The current child close handler uses process.exit(code
?? 0) which treats signal-terminated children (code === null) as success; change
the fallback to a failure exit so signal termination isn't masked by replacing
that call with process.exit(code ?? 1). Update the close handler referenced as
child.on('close', (code) => { ... }) so that when code is null (signal
termination) the process exits with 1; keep the existing forwardedSignal and
forwardSignal logic unchanged.
In `@packages/cli/src/runtime.ts`:
- Around line 398-399: Replace the fragile URL construction that uses new
URL(...) with pathToFileURL for Windows-safe file URL conversion: after calling
writeGeneratedSchemaArtifact(manager, payload.generatedSchemaOutputPath), pass
pathToFileURL(payload.generatedSchemaOutputPath).href into
preloadGeneratedSchema so that preloadGeneratedSchema receives a
platform-correct file URL; update the call that currently uses new
URL(payload.generatedSchemaOutputPath, 'file://').href to use
pathToFileURL(payload.generatedSchemaOutputPath).href and keep the same manager
and payload.generatedSchemaOutputPath references.
In `@packages/cli/tests/cli.test.ts`:
- Around line 9184-9185: The test currently asserts
isDiscoveryRelevantPath('server/db/schema.generated.ts', project) is false, but
this file must be treated as a discovery trigger; update the expectation(s) in
packages/cli/tests/cli.test.ts so that calls to isDiscoveryRelevantPath for
'server/db/schema.generated.ts' (and the duplicate assertions around the other
occurrence) assert true instead of false, ensuring the discovery logic treats a
materialized generated-schema file as relevant; confirm you're editing the
expectations that call isDiscoveryRelevantPath with that exact path string.
- Around line 9338-9339: The test currently only checks that schema.generated.ts
still contains defineGeneratedTable; update it to also assert the generated
model registry was updated to re-register the Course model after prepare: read
the generated registry file (the generated models registry created by the
prepare step) and add an expect(...).toContain(...) asserting the Course model
is present (for example the import or registry entry referencing Course or
server/models/Course.ts / the registry function name used to register generated
models), using the same readFile/join pattern and expect(...).toContain(...) so
the test verifies the registry update, not just the schema file.
In `@packages/core/src/portable/holo.ts`:
- Around line 55-63: The catch block in packages/core/src/portable/holo.ts
currently suppresses any module-not-found style errors via a broad regex;
instead narrow suppression to only when the missing target is the generated
schema file itself: in the catch handler (the try/catch around dynamic import of
the schema) inspect the error's properties (e.g., error.code and error?.message
or error?.stack or error?.requireStack/ error?.meta) and extract the failing
module specifier, then compare that specifier to the expected schema file path
(the same target passed to the import in this module) and only return silently
if they match; otherwise rethrow the error—mirror the more-specific check used
in storageRuntime.ts to decide suppression.
In `@packages/db/src/model/defineModel.ts`:
- Around line 1154-1155: Change the saveMany signature to accept eagerly-loaded
entities like other methods: widen the parameter from readonly Entity<TTable,
TRelations>[] to readonly EntityWithLoaded<TTable, TRelations, unknown>[] so
calls like Model.saveMany(await Model.with('profile').get()) type-check; update
the declaration for saveMany (the method that calls
this.getRepository().saveMany(entities)) to use EntityWithLoaded<TTable,
TRelations, unknown>[] following the same pattern used by chunk() and
chunkById().
In `@packages/db/src/model/Entity.ts`:
- Around line 414-472: The dispatcher omits morph relation helpers so
morphOne/morphMany calls fall through; update the relation-kind checks to
include 'morphOne' with the same helper set as 'hasOne'/'hasOneThrough' and
'morphMany' with the same helper set as 'hasMany'/'hasManyThrough'. Concretely,
extend the first conditional that returns create/save to also check
relationDef.kind === 'morphOne', and extend the second conditional that returns
create/createMany/save/saveMany to also check relationDef.kind === 'morphMany'
(reusing createRelated, createManyRelated, saveRelated, saveManyRelated and
casting to RelationMethodsOf<TRelations[K]>).
- Around line 403-411: The belongsTo relation helper currently only updates the
in-memory cache via this.setRelation(name, ...), so change the
associate/dissociate implementations returned in the belongsTo branch to call
the repository-level association logic instead of setRelation: invoke the
entity's repository associate(...) and dissociate(...) paths (the same functions
used elsewhere to persist foreign-key changes) passing the current entity,
relation name, and target (or null) so the FK is updated and persisted; update
the returned object in the belongsTo branch of Entity (where relationDef.kind
=== 'belongsTo' and RelationMethodsOf<TRelations[K]> is constructed) to route
through those repository associate/dissociate methods rather than
this.setRelation.
In `@packages/db/src/model/serialize.ts`:
- Around line 5-13: The SerializeModels conditional type incorrectly matches
Date via the SerializableModel branch (because Date has toJSON), so change the
type logic to test for Date before testing for SerializableModel (or otherwise
guard the SerializableModel branch to exclude Date) so that Date maps to Date
rather than ReturnType<T['toJSON']>; update all similar conditional types (e.g.,
other SerializeModels occurrences) to perform the Date check prior to or
excluding Date from the SerializableModel branch, referencing the
SerializeModels type and the SerializableModel/toJSON relationship when making
the fix.
In `@packages/db/src/model/types.ts`:
- Around line 327-349: RelationMethodsOf currently erases related-model types by
using unknown and incorrectly infers TPivot from
BelongsToManyRelationDefinition; update RelationMethodsOf to infer the related
model type (e.g., change patterns like BelongsToRelationDefinition to
BelongsToRelationDefinition<infer TRelated> and use TRelated in the returned
method types instead of unknown) and stop extracting TPivot from the relation
def for many-to-many relations — use a pivot-attributes default such as
Record<string, unknown> (or the existing pivot-attributes shape expected by
BelongsToManyRelationMethods) for BelongsToManyRelationDefinition,
MorphToManyRelationDefinition and MorphedByManyRelationDefinition so
attach/attributes are typed correctly while leaving other branches
(HasOneRelationDefinition, HasManyRelationDefinition,
HasOneThroughRelationDefinition, HasManyThroughRelationDefinition,
MorphOneRelationDefinition, MorphManyRelationDefinition) to infer their TRelated
similarly.
In `@packages/db/tests/model-relations.test.ts`:
- Around line 929-997: The test registers generated tables with
registerGeneratedTables(...) but only calls clearGeneratedTables() at the end,
so if an assertion throws the generated-table registry will leak; wrap the test
body that uses the generated tables (everything after
registerGeneratedTables(...) up to the final clearGeneratedTables()) in a
try/finally and move the clearGeneratedTables() call into the finally block so
clearGeneratedTables() always runs; reference the test function,
registerGeneratedTables, and clearGeneratedTables symbols when making this
change.
---
Nitpick comments:
In `@packages/adapter-sveltekit/tests/adapter.type.test.ts`:
- Line 7: Add a published-package smoke test that imports the transport
entrypoint from the packaged subpath and asserts types just like the existing
client check: in tests/adapter.type.test.ts add an import using the package
subpath (e.g. import type { SerializedSvelteKitData } from
'@holo-js/adapter-sveltekit/transport') and mirror the existing
assertion/assertType call that currently targets ../src/transport so the
NodeNext packaged export is exercised; also replicate this change for the other
assertion blocks referenced around lines 65-101.
In `@packages/cli/src/project/registry.ts`:
- Around line 771-786: The resolveGeneratedTsconfigBase function contains a
redundant manifest.framework === 'nuxt' branch returning the same
'../../tsconfig.json' as the fallback; remove the unnecessary conditional (the
if block checking manifest.framework === 'nuxt') or replace it with the actual
Nuxt-specific path logic if intended, ensuring the function still reads
framework/project.json and returns the correct tsconfig path when a Nuxt
framework is detected.
- Around line 268-280: The current regex in patchSvelteConfigWithHooksOverride
(/(kit:\s*\{[^\n]*\n)/) misses single-line or no-newline Svelte kit blocks;
update the matcher to handle `kit: {}` on a single line or blocks without a
trailing newline and insert SVELTE_HOOKS_OVERRIDE_BLOCK appropriately.
Specifically, change the pattern used in contents.replace to match either an
opening brace with the rest of the line (with or without newline) or a closing
brace on the same line (e.g., match `kit:\s*\{[^\n]*\n?` or `kit:\s*\{[^}]*\}`),
then inject SVELTE_HOOKS_OVERRIDE_BLOCK after the matched opening (or before the
closing brace) so single-line `kit: {}` and files without trailing newline are
patched; keep returning undefined when no replacement occurred. Ensure the
change targets patchSvelteConfigWithHooksOverride and still respects existing
svelteConfigHasHooksOverride guard.
In `@packages/cli/src/templates.ts`:
- Around line 84-105: The renderModelTemplate function signature includes an
unused parameter generatedSchemaImportPath; remove this parameter from the
options type and function signature in renderModelTemplate and update all
callers that pass an options object (notably the call site in generators where
renderModelTemplate({...}) is invoked and the unit tests that construct the
options) to stop providing generatedSchemaImportPath; also update any test
fixtures/expectations that relied on the old signature and run tests to ensure
no remaining references to generatedSchemaImportPath remain.
In `@packages/db/src/model/ModelQueryBuilder.ts`:
- Around line 993-1002: Create a shared paginator JSON type alias and replace
the repeated inline casts in paginateJson and the other similar methods (e.g.,
the methods around lines 1031-1040 and 1064-1082) with that alias to avoid
drift; define something like PaginatorJSON<TData, TMeta> = { data: readonly
TData[], meta: TMeta } (using the existing SerializedEntityWithLoaded<TTable,
TLoaded> and PaginationMeta generics) either exported at the top of
ModelQueryBuilder.ts or next to the paginate/paginateJson methods, then change
returns like (await this.paginate(...)).toJSON() as
PaginatorJSON<SerializedEntityWithLoaded<TTable, TLoaded>, PaginationMeta> so
all paginator JSON shapes share one canonical type.
🪄 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: 83f4f11e-dd18-49db-9b85-dd706c94ff17
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
package.jsonpackages/adapter-nuxt/src/module.tspackages/adapter-nuxt/tests/module.test.tspackages/adapter-sveltekit/package.jsonpackages/adapter-sveltekit/src/index.tspackages/adapter-sveltekit/src/transport.tspackages/adapter-sveltekit/tests/adapter.test.tspackages/adapter-sveltekit/tests/adapter.type.test.tspackages/adapter-sveltekit/tsup.config.tspackages/auth/tests/package.test.tspackages/cache-db/tests/package.type.test.tspackages/cli/src/dev.tspackages/cli/src/metadata.tspackages/cli/src/project.tspackages/cli/src/project/registry.tspackages/cli/src/project/scaffold.tspackages/cli/src/project/shared.tspackages/cli/src/runtime.tspackages/cli/src/templates.tspackages/cli/tests/broadcast-registry.test.tspackages/cli/tests/cli.test.tspackages/config/src/env.tspackages/config/tests/config.test.tspackages/core/src/index.tspackages/core/src/portable/holo.tspackages/db/src/index.tspackages/db/src/model/Entity.tspackages/db/src/model/ModelQueryBuilder.tspackages/db/src/model/ModelRepository.tspackages/db/src/model/collection.tspackages/db/src/model/defineModel.tspackages/db/src/model/index.tspackages/db/src/model/serialize.tspackages/db/src/model/types.tspackages/db/src/schema/generated.tspackages/db/tests/eager-load-validation.test.tspackages/db/tests/model-core.test.tspackages/db/tests/model-relations.test.tspackages/db/tests/type-system.test.tspackages/storage/src/index.tspackages/storage/src/publicStorage.ts
Comment on lines
+153
to
211
| async function runNuxtPrepare(projectRoot: string): Promise<void> { | ||
| const frameworkProjectPath = resolve(projectRoot, '.holo-js/framework/project.json') | ||
| try { | ||
| const content = await readFile(frameworkProjectPath, 'utf8') | ||
| const manifest = JSON.parse(content) as { framework?: string } | ||
|
|
||
| if (manifest.framework !== 'nuxt') { | ||
| return | ||
| } | ||
| } catch { | ||
| return | ||
| } | ||
|
|
||
| const { spawn } = await import('node:child_process') | ||
| await new Promise((resolve, reject) => { | ||
| const child = spawn('nuxi', ['prepare'], { | ||
| cwd: projectRoot, | ||
| stdio: 'inherit', | ||
| }) | ||
| child.on('close', code => { | ||
| if (code === 0) { | ||
| resolve(undefined) | ||
| } else { | ||
| reject(new Error(`nuxi prepare exited with ${code}`)) | ||
| } | ||
| }) | ||
| child.on('error', reject) | ||
| }) | ||
| } | ||
|
|
||
| async function runSvelteKitSync(projectRoot: string): Promise<void> { | ||
| const frameworkProjectPath = resolve(projectRoot, '.holo-js/framework/project.json') | ||
| try { | ||
| const content = await readFile(frameworkProjectPath, 'utf8') | ||
| const manifest = JSON.parse(content) as { framework?: string } | ||
|
|
||
| if (manifest.framework !== 'sveltekit') { | ||
| return | ||
| } | ||
| } catch { | ||
| return | ||
| } | ||
|
|
||
| const { spawn } = await import('node:child_process') | ||
| await new Promise((resolve, reject) => { | ||
| const child = spawn('bun', ['x', 'svelte-kit', 'sync'], { | ||
| cwd: projectRoot, | ||
| stdio: 'inherit', | ||
| }) | ||
| child.on('close', code => { | ||
| if (code === 0) { | ||
| resolve(undefined) | ||
| } else { | ||
| reject(new Error(`svelte-kit sync exited with ${code}`)) | ||
| } | ||
| }) | ||
| child.on('error', reject) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Confirm multi-package-manager support exists in dev flow.
rg -nP 'SupportedScaffoldPackageManager|resolveProjectPackageManager' packages/cli/src/dev.ts -C2
# 2) Confirm framework prepare/sync currently use hardcoded command invocations.
rg -nP 'runNuxtPrepare|runSvelteKitSync|spawn\\(.*nuxi|spawn\\(.*svelte-kit|bun.*svelte-kit' packages/cli/src/dev.ts -C3
# 3) Inspect test expectations around package-manager behavior.
rg -nP 'packageManager|bun|npm|pnpm|yarn|runProjectPrepare' packages/cli/tests/cli.test.ts -C3Repository: cobraprojects/holo-js
Length of output: 49123
🏁 Script executed:
# Read the actual dev.ts file around the specified lines
wc -l packages/cli/src/dev.ts
# Get lines around 153-211 to verify the snippet
sed -n '150,215p' packages/cli/src/dev.ts | cat -nRepository: cobraprojects/holo-js
Length of output: 2276
🏁 Script executed:
# Check if resolveProjectPackageManager is used for any framework/build commands
rg -n 'resolveProjectPackageManager' packages/cli/src/dev.ts
# Look for how other tools are spawned (build tools, etc)
rg -n "spawn\(" packages/cli/src/dev.ts -A3
# Check if there are any tests specifically for runNuxtPrepare or runSvelteKitSync
rg -n 'runNuxtPrepare|runSvelteKitSync' packages/cli/tests/Repository: cobraprojects/holo-js
Length of output: 890
🏁 Script executed:
# Understand the context: are these functions actually called during dev?
rg -n 'runNuxtPrepare\|runSvelteKitSync' packages/cli/src/dev.ts -B5 -A2Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check where these functions are called
rg -n 'runNuxtPrepare|runSvelteKitSync' packages/cli/src/
# Check if tests exist for these functions
fd test packages/cli/tests/ -type f -name '*.test.ts' | head -5Repository: cobraprojects/holo-js
Length of output: 712
🏁 Script executed:
# Check the actual invocation patterns by looking at resolvePackageManagerCommand
sed -n '70,90p' packages/cli/src/dev.ts | cat -n
sed -n '78,100p' packages/cli/src/dev.ts | cat -nRepository: cobraprojects/holo-js
Length of output: 1765
🏁 Script executed:
# Verify if this is a real issue: check if projects using npm/pnpm can actually run nuxi prepare
# Look at test setup for nuxt projects
rg -n 'nuxt.*prepare\|runNuxtPrepare' packages/cli/tests/cli.test.ts -B3 -A3Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check context around where these functions are called (lines 141-142, 148-149)
sed -n '130,160p' packages/cli/src/dev.ts | cat -nRepository: cobraprojects/holo-js
Length of output: 1391
🏁 Script executed:
# Look for tests that exercise these framework functions
rg -n 'framework.*project.json|nuxt|sveltekit' packages/cli/tests/cli.test.ts | head -20Repository: cobraprojects/holo-js
Length of output: 846
🏁 Script executed:
# Check if these binaries (nuxi, svelte-kit) would be available via npm/pnpm exec
rg -n 'nuxi\|svelte-kit' packages/cli/tests/cli.test.ts -B5 -A5 | head -40Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check if nuxi and svelte-kit are actually installed as dev dependencies in generated projects
rg -n '"nuxi"|"svelte-kit"' packages/cli/src/ -A1 -B1 | head -30Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Look at how the test projects are set up - are these tools available?
rg -n 'writeFrameworkBinary|nuxi|svelte-kit' packages/cli/tests/cli.test.ts -B3 -A3 | head -50Repository: cobraprojects/holo-js
Length of output: 2393
🏁 Script executed:
# Check what the actual scaffolded package.json includes for framework dependencies
rg -n 'renderScaffoldPackageJson\|renderFrameworkPackageJson' packages/cli/src/ -A20 | head -60Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check what dependencies are actually scaffolded for nuxt/sveltekit projects
rg -n 'nuxi|@nuxt|svelte-kit' packages/cli/src/generators/ -B2 -A2 | head -50Repository: cobraprojects/holo-js
Length of output: 134
🏁 Script executed:
# Look at the framework runner to understand the execution context
find packages/cli/src -name '*framework*' -o -name '*runner*' | head -10Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check if nuxi/svelte-kit would be in node_modules for actual projects
rg -n '"nuxi"|"@nuxt|"svelte-kit"' packages/cli/src/ -B1 -A1 | grep -E '(dep|import|require)' | head -20Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Most important: test if runNuxtPrepare/runSvelteKitSync are actually tested with real package managers
rg -n 'runNuxtPrepare|runSvelteKitSync|runProjectPrepare' packages/cli/tests/cli.test.ts -B5 -A5 | grep -E 'packageManager|npm|pnpm|yarn|bun' | head -20Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Find where project generation happens
find packages/cli/src -type f -name '*.ts' | xargs grep -l 'renderScaffoldPackageJson\|framework.*dependencies' | head -5Repository: cobraprojects/holo-js
Length of output: 130
🏁 Script executed:
# Check the project scaffolding to see what dependencies are added for frameworks
rg -n 'nuxi|svelte-kit|@nuxt' packages/cli/src -B2 -A2 | head -60Repository: cobraprojects/holo-js
Length of output: 3643
🏁 Script executed:
# Look at test cases that use npm or pnpm - are they able to run nuxi/svelte-kit?
sed -n '4480,4510p' packages/cli/tests/cli.test.ts | cat -nRepository: cobraprojects/holo-js
Length of output: 1448
🏁 Script executed:
# Critical: check if writeFrameworkBinary is needed because these aren't in package.json
rg -n 'writeFrameworkBinary' packages/cli/tests/cli.test.ts -B10 | head -40Repository: cobraprojects/holo-js
Length of output: 1892
🏁 Script executed:
# Critical: Check if nuxi and svelte-kit are in the actual project dependencies
sed -n '3300,3360p' packages/cli/src/project/scaffold.ts | cat -nRepository: cobraprojects/holo-js
Length of output: 2841
🏁 Script executed:
# Look at the package.json template to see what dependencies are included
rg -n 'renderScaffoldPackageJson\|dependencies.*:' packages/cli/src/project/scaffold.ts -A30 | head -80Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check if nuxi/svelte-kit are listed as dependencies or only accessible via npx
rg -n '"nuxi"|"svelte-kit"|"@nuxt' packages/cli/src/project/scaffold.ts | head -20Repository: cobraprojects/holo-js
Length of output: 47
🏁 Script executed:
# Check the full scaffolded package.json to see if nuxi/svelte-kit are dependencies
rg -n 'function renderScaffoldPackageJson' packages/cli/src/project/scaffold.ts -A100 | head -120Repository: cobraprojects/holo-js
Length of output: 4371
🏁 Script executed:
# Look at the actual generated package.json scripts
sed -n '3540,3560p' packages/cli/src/project/scaffold.ts | cat -nRepository: cobraprojects/holo-js
Length of output: 1092
🏁 Script executed:
# Final verification: check if these tools would be available in PATH or need package manager
# Look for how the framework runner actually finds and runs the binaries
sed -n '3308,3380p' packages/cli/src/project/scaffold.ts | tail -40Repository: cobraprojects/holo-js
Length of output: 1416
🏁 Script executed:
# Check if there are any integration tests with npm/pnpm that run nuxi/svelte-kit
rg -n 'packageManager.*npm|packageManager.*pnpm' packages/cli/tests/cli.test.ts -A20 | grep -E 'nuxi|svelte-kit|runProjectPrepare' -B2 -A2 | head -40Repository: cobraprojects/holo-js
Length of output: 47
Framework sync commands must respect the project's package manager.
spawn('nuxi', ...) on line 168 and spawn('bun', ['x', 'svelte-kit', 'sync'], ...) on line 198 will fail for npm/pnpm/yarn projects. These binaries are not in the system PATH; they must be invoked through the project's package manager.
Call resolveProjectPackageManager() and construct the appropriate invocation:
- npm:
npm exec -- nuxi prepare - pnpm:
pnpm exec nuxi prepare - yarn:
yarn run nuxi prepare - bun:
bun x nuxi prepare
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/dev.ts` around lines 153 - 211, Both runNuxtPrepare and
runSvelteKitSync currently spawn tool binaries directly (spawn('nuxi', ...) and
spawn('bun', ['x', 'svelte-kit','sync'], ...)) which breaks when the project
uses npm/pnpm/yarn; update each function to call
resolveProjectPackageManager(projectRoot) and build the command/args according
to the returned manager: for npm use ['exec', '--', 'nuxi', 'prepare'], for pnpm
use ['exec', 'nuxi', 'prepare'], for yarn use ['run', 'nuxi', 'prepare'], and
for bun use ['x', 'nuxi', 'prepare'] (and similarly for svelte-kit sync: npm
['exec','--','svelte-kit','sync'], pnpm ['exec','svelte-kit','sync'], yarn
['run','svelte-kit','sync'], bun ['x','svelte-kit','sync']); then spawn the
package manager binary (manager string) with those args, preserving cwd and
stdio and existing close/error handling.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/db/src/model/defineModel.ts (1)
906-920:⚠️ Potential issue | 🟠 MajorAdd the array overload to the public
with()API.This implementation makes
Model.with(['posts'])work at runtime, butStaticModelApistill only exposes the variadic and map overloads. TypeScript callers of the new user-facing API will still get a type error.Suggested overload
with<TPaths extends readonly ModelRelationPath<TRelations>[]>(...relations: TPaths): ModelQueryBuilder<TTable, TRelations, ResolveEagerLoads<TRelations, TPaths>> + with<TPaths extends readonly ModelRelationPath<TRelations>[]>(relations: TPaths): ModelQueryBuilder<TTable, TRelations, ResolveEagerLoads<TRelations, TPaths>> with<TPath extends ModelRelationPath<TRelations>>(relation: TPath, constraint: RelationConstraintCallback): ModelQueryBuilder<TTable, TRelations, ResolveEagerLoads<TRelations, readonly [TPath]>> with<TMap extends Readonly<Partial<Record<ModelRelationPath<TRelations>, RelationConstraintCallback>>>>(relations: TMap): ModelQueryBuilder<TTable, TRelations>🤖 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 906 - 920, The public API is missing the array overload for with(), so although Model.with(['posts']) works at runtime the TypeScript types (StaticModelApi) don't allow it; add a new overload signature that accepts readonly ModelRelationPath<TRelations>[] to the public with() declaration(s) (e.g., StaticModelApi.with and any corresponding interface/type exports) so the variadic/map overloads remain and the array form is typed; update any exported type declarations that declare with to include the array overload matching the runtime implementation.packages/db/src/model/Entity.ts (1)
694-701:⚠️ Potential issue | 🟠 Major
load()andloadMissing()use an unsound type pattern that erases core entity members when relation names collide.
Omit<this, keyof ResolveEagerLoads<...>>removes keys from the entire entity type, including core methods and properties. However, at runtime, relation accessors skip existing keys with theif (key in this) continuecheck, so those members are not actually replaced. This creates a type/runtime mismatch—if a relation name collides with an existing method (e.g., agetrelation), the type claims that method is gone while the runtime still has it.The codebase already has a better pattern in
EntityWithLoaded<TTable, TRelations, TLoaded>, which usesOmit<ModelRelationMethods<TRelations>, Extract<keyof TLoaded, string>>to omit only from the relation method accessors, not from the entire entity. Adopt that pattern instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/Entity.ts` around lines 694 - 701, The load/loadMissing return types currently use Omit<this, keyof ResolveEagerLoads<...>> which erases core entity members; change the signature to mirror EntityWithLoaded by omitting only relation accessor methods instead of the whole entity: replace Omit<this, keyof ResolveEagerLoads<TRelations, TPaths>> with Omit<ModelRelationMethods<TRelations>, Extract<keyof ResolveEagerLoads<TRelations, TPaths>, string>> (or reuse EntityWithLoaded<TTable, TRelations, ResolveEagerLoads<TRelations, TPaths>> composition) and keep the ResolveEagerLoads/SerializeLoaded merges so that core methods on the entity remain typed while loaded relation accessors are added.packages/db/src/model/types.ts (1)
560-566:⚠️ Potential issue | 🟠 MajorNested eager-loaded entities won't properly serialize in
toJSON().
Entity<TTable, TRelations>is defined asModelBase<TTable, TRelations> & ModelRelationMethods<TRelations>(line 1066), butEntityWithLoadedis defined asModelBase<TTable, TRelations> & Omit<ModelRelationMethods<TRelations>, Extract<keyof TLoaded, string>> & TLoaded(lines 560-566). WhenSerializeLoadedValuechecksTValue extends Entity<...>(line 537), nested eager-loaded entities inTLoadedwill not match because they lack the fullModelRelationMethodsshape. This causes them to fall through to the else branch and leak entity types instead of serializing toModelRecordobjects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/types.ts` around lines 560 - 566, EntityWithLoaded currently omits ModelRelationMethods on keys overridden by TLoaded so nested eager-loaded entities in TLoaded no longer match the Entity shape used by SerializeLoadedValue; update EntityWithLoaded (the type alias named EntityWithLoaded) so that TLoaded values retain the full Entity shape (i.e., include ModelRelationMethods<TRelations> or align with the Entity<TTable, TRelations> definition) instead of removing those methods, ensuring SerializeLoadedValue can detect nested Entity types and serialize them to ModelRecord as intended; adjust the type composition to mirror Entity (ModelBase + ModelRelationMethods) plus TLoaded rather than omitting ModelRelationMethods from TLoaded.packages/cli/src/dev.ts (1)
342-356:⚠️ Potential issue | 🟠 MajorAdd the generated-schema directory to the non-recursive watch roots.
isDiscoveryRelevantPath()now treatsproject.config.paths.generatedSchemaas relevant, but this root set never watches that file’s parent directory. On non-recursivefs.watchplatforms, edits toserver/db/schema.generated.tswill still be missed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/dev.ts` around lines 342 - 356, The roots array currently used for non-recursive watches misses the generated schema directory referenced by isDiscoveryRelevantPath(); update the roots list (the const roots in dev.ts) to include the parent directory of project.config.paths.generatedSchema (i.e., resolve(projectRoot, path.dirname(project.config.paths.generatedSchema)) or equivalent) so edits to server/db/schema.generated.ts are watched on non-recursive fs.watch platforms.
♻️ Duplicate comments (3)
packages/db/src/model/serialize.ts (1)
20-31:⚠️ Potential issue | 🟠 MajorDate serialization still takes the wrong runtime branch (type/runtime mismatch).
At Line 21,
isSerializableModel(value)runs before the Date guard, soDatevalues hittoJSON()and become strings. That contradicts theSerializeModelstype at Line 6 (Date -> Date) and makes the Date check at Line 29 unreachable for actualDateinstances.Suggested fix
export function serializeModels<TValue>(value: TValue): SerializeModels<TValue> { + if (value instanceof Date) { + return value as SerializeModels<TValue> + } + if (isSerializableModel(value)) { return value.toJSON() as SerializeModels<TValue> } if (Array.isArray(value)) { return value.map(item => serializeModels(item)) as SerializeModels<TValue> } - if (value instanceof Date || value === null || typeof value !== 'object') { + if (value === null || typeof value !== 'object') { return value as SerializeModels<TValue> }In JavaScript/TypeScript, does `Date` implement `toJSON()`, and does placing a generic `toJSON` guard before `instanceof Date` cause Date values to serialize as strings?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/serialize.ts` around lines 20 - 31, serializeModels currently calls isSerializableModel(value) before checking for Date, so native Date instances (which implement toJSON) are treated as serializable models and become strings; reorder the runtime guards so the Date/primitive/null check (the "value instanceof Date || value === null || typeof value !== 'object'") runs before the isSerializableModel branch, or alternatively update isSerializableModel to explicitly exclude Date objects; adjust serializeModels to ensure Date values follow the Date branch (preserving Date -> Date in SerializeModels) and arrays/models still serialize correctly.packages/db/src/model/Entity.ts (1)
403-419:⚠️ Potential issue | 🟠 MajorFail closed for
belongsTohelpers instead of mutating only the cache.If
associateRelation/dissociateRelationare unavailable, this only updates the loaded relation and never touches the foreign key, so the entity can look associated in memory but persist nothing. Reusethis.associate()/this.dissociate()here.Suggested fix
return { associate: (related: unknown) => { - if (typeof repo.associateRelation === 'function') { - repo.associateRelation(this, name, related as Entity<TableDefinition> | null) - } else { - this.setRelation(name, related) - } + this.associate(name, related as Entity<TableDefinition> | null) }, dissociate: () => { - if (typeof repo.dissociateRelation === 'function') { - repo.dissociateRelation(this, name) - } else { - this.setRelation(name, null) - } + this.dissociate(name) }, } as unknown as RelationMethodsOf<TRelations[K]>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/Entity.ts` around lines 403 - 419, The belongsTo helper currently mutates only the loaded relation via this.setRelation when repo.associateRelation/dissociateRelation are absent, leaving foreign keys unchanged; update the associate and dissociate closures in the relationDef.kind === 'belongsTo' block to call this.associate(name, related) and this.dissociate(name) (or the instance methods used to update FK and persist) instead of this.setRelation(name, ...) so the FK is updated consistently when repo-level helpers are missing; keep using repo.associateRelation/repo.dissociateRelation when those functions exist.packages/db/src/model/defineModel.ts (1)
476-492:⚠️ Potential issue | 🟠 MajorKeep
uniqueIdConfiglazy for deferred generated models.When the generated table is unresolved,
uniqueIdConfigis computed once fromoptions.primaryKey ?? 'id'and never recomputed afterregisterGeneratedTables(). Models whose generated primary key or unique-id columns differ will keep stale config and skipvalidateUniqueIdConfig().🤖 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 476 - 492, uniqueIdConfig is computed eagerly when resolvedAtDefinition is false, causing stale config for deferred generated tables; change it to compute lazily so it uses the resolved primary key after generated tables are registered. Specifically, when resolvedAtDefinition is false, defer calling resolveUniqueIdConfig until after resolution (e.g., by capturing a thunk that calls resolveUniqueIdConfig with resolvePrimaryKey()), and then invoke that thunk before calling validateUniqueIdConfig; update the logic around uniqueIdConfig, resolvedAtDefinition, resolvePrimaryKey(), resolveUniqueIdConfig(...), registerGeneratedTables(), and validateUniqueIdConfig(...) so the real primary key/unique-id columns are used once the table is resolved.
🤖 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/registry.ts`:
- Around line 352-362: The current catch in syncManagedFrameworkArtifacts
swallows errors from ensureSvelteManagedHooks; refactor so only the framework
manifest read/parse is suppressed and any errors from ensureSvelteManagedHooks
surface. Concretely, isolate the readFile + JSON.parse of the
'.holo-js/framework/project.json' into its own try/catch (or validate manifest
before calling) and only ignore errors from that block (read/parse failures);
after successfully obtaining manifest.framework, call
ensureSvelteManagedHooks(projectRoot) outside that catch so write/patch failures
are not swallowed.
- Around line 320-330: The code currently always deletes legacyHooksUserPath and
legacyHooksServerUserPath even when no migration was performed; fix by only
unlinking those legacy files when the migration condition matched and the write
was attempted — i.e., move or wrap the unlinkIfPresent(legacyHooksUserPath) call
inside the same if block that checks legacyUserContents && (!hooksContents ||
isManagedPrepareArtifact(hooksContents)) and do the same for
legacyHooksServerUserPath inside the if that checks legacyServerUserContents &&
(!hooksServerContents || isManagedPrepareArtifact(hooksServerContents)); use the
same symbols (legacyUserContents, hooksContents, isManagedPrepareArtifact,
writeFileIfChanged, unlinkIfPresent, legacyHooksUserPath,
legacyServerUserContents, hooksServerContents, legacyHooksServerUserPath) so
legacy files are only removed when migration actually ran.
In `@packages/cli/tests/cli.test.ts`:
- Around line 9448-9449: Add assertions in the test after creating src/hooks.ts
and src/hooks.server.ts to ensure the generated Holo hooks files actually import
and re-export the user hooks: read the generated files matching
.holo-js/generated/hooks*.ts (the same files already inspected) and assert they
contain an import statement that references '../../src/hooks' and that they
reference/forward the exported names reroute and handleFetch (e.g., strings
"import" + "from '../../src/hooks'" and the identifiers "reroute" and
"handleFetch"); update the assertions near where writeProjectFile is used (and
the similar block at 9471-9478) to check those import and forward-export
occurrences so a missing import/export will fail the test.
In `@packages/db/src/model/Entity.ts`:
- Around line 396-400: The code in relation() currently reads relation metadata
directly from repo.definition.relations[name], which misses
dynamically-registered relations; instead, resolve relation metadata via the
repository API used elsewhere (e.g. use repo.getRelationNames() to verify
presence and call the repository method that returns/resolves a relation
definition—such as a getRelation/getRelationDefinition or the
resolveRelationUsing-backed resolver) so relation() uses the same resolution
path as initializeModelProperties(); replace the direct access to
repo.definition.relations[name] with a call to the repository’s
relation-resolution method and fall back to throwing the same error only if that
call returns nothing.
In `@packages/db/src/model/types.ts`:
- Around line 626-629: The collection factory type is losing model-specific
relations because DefineModelOptions.collection and ModelDefinition.collection
instantiate ModelCollectionFactory with only TTable; update those usages to pass
the TRelations generic so the concrete relation map is preserved. Locate the
DefineModelOptions.collection and ModelDefinition.collection members and change
their types from ModelCollectionFactory<TTable> to
ModelCollectionFactory<TTable, TRelations> (ensuring the surrounding generics
include TRelations) so TItem remains relation-aware throughout.
- Around line 327-350: The conditional type RelationMethodsOf does not handle
MorphOneOfManyRelationDefinition, causing those relations to resolve to never;
update the RelationMethodsOf<T> conditional (the type alias RelationMethodsOf)
to include a branch for MorphOneOfManyRelationDefinition<infer TRelated> that
maps to HasOneRelationMethods<Entity<ModelDefinitionTable<TRelated>>> (same as
other one-to-one relation cases like MorphOneRelationDefinition and
HasOneRelationDefinition) so relation<MorphOneOfMany...> gets the correct to-one
method surface.
---
Outside diff comments:
In `@packages/cli/src/dev.ts`:
- Around line 342-356: The roots array currently used for non-recursive watches
misses the generated schema directory referenced by isDiscoveryRelevantPath();
update the roots list (the const roots in dev.ts) to include the parent
directory of project.config.paths.generatedSchema (i.e., resolve(projectRoot,
path.dirname(project.config.paths.generatedSchema)) or equivalent) so edits to
server/db/schema.generated.ts are watched on non-recursive fs.watch platforms.
In `@packages/db/src/model/defineModel.ts`:
- Around line 906-920: The public API is missing the array overload for with(),
so although Model.with(['posts']) works at runtime the TypeScript types
(StaticModelApi) don't allow it; add a new overload signature that accepts
readonly ModelRelationPath<TRelations>[] to the public with() declaration(s)
(e.g., StaticModelApi.with and any corresponding interface/type exports) so the
variadic/map overloads remain and the array form is typed; update any exported
type declarations that declare with to include the array overload matching the
runtime implementation.
In `@packages/db/src/model/Entity.ts`:
- Around line 694-701: The load/loadMissing return types currently use
Omit<this, keyof ResolveEagerLoads<...>> which erases core entity members;
change the signature to mirror EntityWithLoaded by omitting only relation
accessor methods instead of the whole entity: replace Omit<this, keyof
ResolveEagerLoads<TRelations, TPaths>> with
Omit<ModelRelationMethods<TRelations>, Extract<keyof
ResolveEagerLoads<TRelations, TPaths>, string>> (or reuse
EntityWithLoaded<TTable, TRelations, ResolveEagerLoads<TRelations, TPaths>>
composition) and keep the ResolveEagerLoads/SerializeLoaded merges so that core
methods on the entity remain typed while loaded relation accessors are added.
In `@packages/db/src/model/types.ts`:
- Around line 560-566: EntityWithLoaded currently omits ModelRelationMethods on
keys overridden by TLoaded so nested eager-loaded entities in TLoaded no longer
match the Entity shape used by SerializeLoadedValue; update EntityWithLoaded
(the type alias named EntityWithLoaded) so that TLoaded values retain the full
Entity shape (i.e., include ModelRelationMethods<TRelations> or align with the
Entity<TTable, TRelations> definition) instead of removing those methods,
ensuring SerializeLoadedValue can detect nested Entity types and serialize them
to ModelRecord as intended; adjust the type composition to mirror Entity
(ModelBase + ModelRelationMethods) plus TLoaded rather than omitting
ModelRelationMethods from TLoaded.
---
Duplicate comments:
In `@packages/db/src/model/defineModel.ts`:
- Around line 476-492: uniqueIdConfig is computed eagerly when
resolvedAtDefinition is false, causing stale config for deferred generated
tables; change it to compute lazily so it uses the resolved primary key after
generated tables are registered. Specifically, when resolvedAtDefinition is
false, defer calling resolveUniqueIdConfig until after resolution (e.g., by
capturing a thunk that calls resolveUniqueIdConfig with resolvePrimaryKey()),
and then invoke that thunk before calling validateUniqueIdConfig; update the
logic around uniqueIdConfig, resolvedAtDefinition, resolvePrimaryKey(),
resolveUniqueIdConfig(...), registerGeneratedTables(), and
validateUniqueIdConfig(...) so the real primary key/unique-id columns are used
once the table is resolved.
In `@packages/db/src/model/Entity.ts`:
- Around line 403-419: The belongsTo helper currently mutates only the loaded
relation via this.setRelation when repo.associateRelation/dissociateRelation are
absent, leaving foreign keys unchanged; update the associate and dissociate
closures in the relationDef.kind === 'belongsTo' block to call
this.associate(name, related) and this.dissociate(name) (or the instance methods
used to update FK and persist) instead of this.setRelation(name, ...) so the FK
is updated consistently when repo-level helpers are missing; keep using
repo.associateRelation/repo.dissociateRelation when those functions exist.
In `@packages/db/src/model/serialize.ts`:
- Around line 20-31: serializeModels currently calls isSerializableModel(value)
before checking for Date, so native Date instances (which implement toJSON) are
treated as serializable models and become strings; reorder the runtime guards so
the Date/primitive/null check (the "value instanceof Date || value === null ||
typeof value !== 'object'") runs before the isSerializableModel branch, or
alternatively update isSerializableModel to explicitly exclude Date objects;
adjust serializeModels to ensure Date values follow the Date branch (preserving
Date -> Date in SerializeModels) and arrays/models still serialize correctly.
🪄 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: 5c7d8982-1836-40f5-99c1-03b5cb21fa42
📒 Files selected for processing (13)
packages/cli/src/dev.tspackages/cli/src/metadata.tspackages/cli/src/project/registry.tspackages/cli/src/project/scaffold.tspackages/cli/src/runtime.tspackages/cli/src/templates.tspackages/cli/tests/cli.test.tspackages/core/src/portable/holo.tspackages/db/src/model/Entity.tspackages/db/src/model/defineModel.tspackages/db/src/model/serialize.tspackages/db/src/model/types.tspackages/db/tests/model-relations.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/src/templates.ts
- packages/cli/src/metadata.ts
- packages/core/src/portable/holo.ts
Comment on lines
+320
to
+330
| // If the old .user.ts files exist, migrate their content to the standard hook paths | ||
| // and delete the legacy files. | ||
| if (legacyUserContents && (!hooksContents || isManagedPrepareArtifact(hooksContents))) { | ||
| await writeFileIfChanged(hooksPath, legacyUserContents) | ||
| } | ||
| await unlinkIfPresent(legacyHooksUserPath) | ||
|
|
||
| if (legacyServerUserContents && (!hooksServerContents || isManagedPrepareArtifact(hooksServerContents))) { | ||
| await writeFileIfChanged(hooksServerPath, legacyServerUserContents) | ||
| } | ||
| await unlinkIfPresent(legacyHooksServerUserPath) |
There was a problem hiding this comment.
Don't delete legacy hook files unless the migration actually happened.
When src/hooks.ts or src/hooks.server.ts already contains user-owned code, the copy is skipped, but Line 325 and Line 330 still remove the legacy .user.ts files. That can silently drop hook implementations during prepare.
🤖 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 320 - 330, The code
currently always deletes legacyHooksUserPath and legacyHooksServerUserPath even
when no migration was performed; fix by only unlinking those legacy files when
the migration condition matched and the write was attempted — i.e., move or wrap
the unlinkIfPresent(legacyHooksUserPath) call inside the same if block that
checks legacyUserContents && (!hooksContents ||
isManagedPrepareArtifact(hooksContents)) and do the same for
legacyHooksServerUserPath inside the if that checks legacyServerUserContents &&
(!hooksServerContents || isManagedPrepareArtifact(hooksServerContents)); use the
same symbols (legacyUserContents, hooksContents, isManagedPrepareArtifact,
writeFileIfChanged, unlinkIfPresent, legacyHooksUserPath,
legacyServerUserContents, hooksServerContents, legacyHooksServerUserPath) so
legacy files are only removed when migration actually ran.
Comment on lines
626
to
+629
| export type ModelCollectionFactory< | ||
| TTable extends TableDefinition = TableDefinition, | ||
| TRelations extends RelationMap = RelationMap, | ||
| > = (items: readonly Entity<TTable, TRelations>[]) => ModelCollection<TTable, TRelations> | ||
| > = <TItem extends Entity<TTable, TRelations>>(items: readonly TItem[]) => ModelCollection<TTable, TRelations, TItem> |
There was a problem hiding this comment.
Thread TRelations through the public collection option as well.
This alias now carries relation-aware TItem, but DefineModelOptions.collection and ModelDefinition.collection below still instantiate it as ModelCollectionFactory<TTable>. That resets TRelations to RelationMap, so model-specific collection callbacks still lose the concrete relation surface this change is trying to preserve.
Suggested diff
- collection?: ModelCollectionFactory<TTable>
+ collection?: ModelCollectionFactory<TTable, TRelations>- readonly collection?: ModelCollectionFactory<TTable>
+ readonly collection?: ModelCollectionFactory<TTable, TRelations>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/model/types.ts` around lines 626 - 629, The collection
factory type is losing model-specific relations because
DefineModelOptions.collection and ModelDefinition.collection instantiate
ModelCollectionFactory with only TTable; update those usages to pass the
TRelations generic so the concrete relation map is preserved. Locate the
DefineModelOptions.collection and ModelDefinition.collection members and change
their types from ModelCollectionFactory<TTable> to
ModelCollectionFactory<TTable, TRelations> (ensuring the surrounding generics
include TRelations) so TItem remains relation-aware throughout.
…lback 'id' primary key for deferred tables) to a lazy getter via Object.defineProperties, so the resolved primary key is used after generated tables are registered. Array overload for with(): Added a with<TPaths>(relations: TPaths) overload to StaticModelApi accepting readonly ModelRelationPath<TRelations>[] so Model.with(['posts']) is properly typed alongside the existing variadic and map overloads. Findings verified but not changed: DefineModelOptions.collection/ModelDefinition.collection losing TRelations: Adding TRelations to ModelCollectionFactory caused cascading variance errors across the codebase. The TRelations generic is invariant in many positions, making this change unsafe without a broader refactor. EntityWithLoaded / load/loadMissing return types: The current types are correct — Omit<this, keyof ResolveEagerLoads<...>> properly replaces lazy relation accessors with loaded values while preserving core entity methods. Legacy file deletion in ensureSvelteManagedHooks: The unconditional deletion is intentional — legacy .user.ts files are always cleaned up regardless of whether migration ran, as confirmed by the existing test expectations.
There was a problem hiding this comment.
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 (1)
packages/db/src/model/Entity.ts (1)
1007-1032:⚠️ Potential issue | 🟠 MajorDon't replace unloaded relation values with a callable wrapper.
For unloaded relations, this getter now returns a function object instead of the value from
repo.resolveRelationProperty(...). That changes plain property reads from[] | undefined | Promiseinto a truthy function, so user-facing code likepost.comments ?? [],Array.isArray(post.comments), orif (post.author)no longer behaves the same way until the relation is loaded. Sincerelation(name)already exposes the mutation helpers, the property getter should keep returning the relation value contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/model/Entity.ts` around lines 1007 - 1032, The getter currently returns a callable wrapper (relationMethod) for unloaded relations, which makes a property read evaluate to a truthy function instead of the relation value; change it to return the actual relation value or Promise produced by repo.resolveRelationProperty instead of a function. Specifically, in the property getter (referencing hasRelation, getRelation, relation(key), relationMethod, loadRelation and repo.resolveRelationProperty) remove the function wrapper (relationMethod) and its custom then/catch/finally assignments and instead return the result of loadRelation() (or the Promise it returns) so unloaded relations preserve the original value/Promise/undefined contract while still allowing relation(key) to provide mutation helpers.
♻️ Duplicate comments (2)
packages/cli/src/project/registry.ts (1)
322-330:⚠️ Potential issue | 🟠 MajorOnly delete legacy hook files when migration actually runs.
Line 325 and Line 330 still unlink legacy files unconditionally, even when migration conditions are false. This can delete user-owned legacy hook files without copying them.
Suggested fix
if (legacyUserContents && (!hooksContents || isManagedPrepareArtifact(hooksContents))) { await writeFileIfChanged(hooksPath, legacyUserContents) + await unlinkIfPresent(legacyHooksUserPath) } - await unlinkIfPresent(legacyHooksUserPath) if (legacyServerUserContents && (!hooksServerContents || isManagedPrepareArtifact(hooksServerContents))) { await writeFileIfChanged(hooksServerPath, legacyServerUserContents) + await unlinkIfPresent(legacyHooksServerUserPath) } - await unlinkIfPresent(legacyHooksServerUserPath)🤖 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 322 - 330, The unlinkIfPresent calls currently run unconditionally and may delete user-owned legacy hook files even when no migration occurred; change the logic so that unlinkIfPresent(legacyHooksUserPath) is only called when the preceding migration block actually ran (i.e., inside the same if that checks legacyUserContents && (!hooksContents || isManagedPrepareArtifact(hooksContents)) after writeFileIfChanged), and likewise call unlinkIfPresent(legacyHooksServerUserPath) only inside the if that handles legacyServerUserContents && (!hooksServerContents || isManagedPrepareArtifact(hooksServerContents)); reference the existing symbols legacyUserContents, hooksContents, isManagedPrepareArtifact, writeFileIfChanged, unlinkIfPresent, legacyHooksUserPath, legacyServerUserContents, hooksServerContents, and legacyHooksServerUserPath to locate and move the unlink calls.packages/db/src/model/defineModel.ts (1)
478-490:⚠️ Potential issue | 🟠 MajorValidate
uniqueIdConfigafter deferred generated tables resolve.This still only validates the config when the table is already present in the registry. If
defineModel('users')runs before generated tables are registered, invalidprimaryKey/uniqueIdsoptions stay unchecked forever.🔧 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 } if (resolvedAtDefinition) { - const eagerConfig = resolveUniqueId() - validateUniqueIdConfig(resolvedAtDefinition, inferredName, eagerConfig) + resolveUniqueId() }
🤖 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/registry.ts`:
- Around line 264-289: The current svelteConfigHasHooksOverride only looks for
the literal '.holo-js/generated/hooks' so user-defined kit.files.hooks still get
double-patched; update svelteConfigHasHooksOverride to detect any existing
kit.files.hooks override (not just the holo path) by searching the config for a
nested pattern like /kit\s*:\s*\{[\s\S]*?files\s*:\s*\{[\s\S]*?hooks\s*:/ or
similar regex that matches any hooks property within kit.files, and keep the
existing string check for the holo path as well; ensure
patchSvelteConfigWithHooksOverride continues to call
svelteConfigHasHooksOverride and only mutates when that function returns false,
and reference SVELTE_HOOKS_OVERRIDE_BLOCK when inserting the block.
In `@packages/cli/tests/cli.test.ts`:
- Around line 2115-2116: The test currently only asserts the presence of
"defineModel" and can pass even if the configured generated-schema path is wrong
or missing; update the test that calls installAuthIntoProject() to assert the
exact configured generated-schema import path (the configured value passed to or
referenced by installAuthIntoProject, e.g., "server/db/schema.generated.ts" or
the test's configured path variable) is present in the produced file contents in
addition to the defineModel import, so the assertion verifies the side-effect
import of the configured generated-schema path rather than only the generic
defineModel symbol.
- Around line 9251-9258: The test triggers the watcher and immediately asserts
prepare call counts, which is flaky because the startup prepare may not have
completed; after installing the watcher (where watchCallback is captured) wait
until prepare has been called at least once (e.g., poll until
prepare.mock.calls.length >= 1 or use an awaitable helper) before invoking
watchCallback('change', ...) and then assert the expected call count (or
reset/clear the mock before triggering the change and then assert
toHaveBeenCalledTimes(1)); reference the watchCallback and prepare mocks in the
test to locate where to add this wait or mock reset.
In `@packages/db/src/model/defineModel.ts`:
- Around line 1053-1060: The StaticModelApi type declares chunk, chunkById, and
chunkByIdDesc returning/accepting arrays of Entity<TTable, TRelations>, but the
runtime implementations (chunk, chunkById, chunkByIdDesc) use
EntityWithLoaded<TTable, TRelations, unknown> to preserve eager-loaded
relations; update the StaticModelApi method signatures (chunk, chunkById,
chunkByIdDesc) to use EntityWithLoaded<TTable, TRelations, unknown>[] for the
callback row parameter (and any return types) so callers of
Model.with(...).chunk*() retain loaded relation types and the interface matches
the implementation.
In `@packages/db/src/model/Entity.ts`:
- Around line 444-475: The discriminator check in the relation helper uses
relationDef.kind === 'belongsToMany' (plus 'morphToMany'/'morphedByMany') but
the repository classifies the same family as 'manyToMany', causing many-to-many
methods (attach/detach/sync/toggle/updateExistingPivot/create/save) to be
omitted; update the condition in the block that returns
RelationMethodsOf<TRelations[K]> so it accepts both aliases (e.g.,
relationDef.kind === 'manyToMany' || relationDef.kind === 'belongsToMany' ||
relationDef.kind === 'morphToMany' || relationDef.kind === 'morphedByMany') or
normalize the discriminator earlier to a single canonical value matching
ModelRepository (the latter via the same normalization function used in
ModelRepository), ensuring
attach/detach/sync/toggle/updateExistingPivot/create/save are exposed for the
many-to-many relation kinds.
---
Outside diff comments:
In `@packages/db/src/model/Entity.ts`:
- Around line 1007-1032: The getter currently returns a callable wrapper
(relationMethod) for unloaded relations, which makes a property read evaluate to
a truthy function instead of the relation value; change it to return the actual
relation value or Promise produced by repo.resolveRelationProperty instead of a
function. Specifically, in the property getter (referencing hasRelation,
getRelation, relation(key), relationMethod, loadRelation and
repo.resolveRelationProperty) remove the function wrapper (relationMethod) and
its custom then/catch/finally assignments and instead return the result of
loadRelation() (or the Promise it returns) so unloaded relations preserve the
original value/Promise/undefined contract while still allowing relation(key) to
provide mutation helpers.
---
Duplicate comments:
In `@packages/cli/src/project/registry.ts`:
- Around line 322-330: The unlinkIfPresent calls currently run unconditionally
and may delete user-owned legacy hook files even when no migration occurred;
change the logic so that unlinkIfPresent(legacyHooksUserPath) is only called
when the preceding migration block actually ran (i.e., inside the same if that
checks legacyUserContents && (!hooksContents ||
isManagedPrepareArtifact(hooksContents)) after writeFileIfChanged), and likewise
call unlinkIfPresent(legacyHooksServerUserPath) only inside the if that handles
legacyServerUserContents && (!hooksServerContents ||
isManagedPrepareArtifact(hooksServerContents)); reference the existing symbols
legacyUserContents, hooksContents, isManagedPrepareArtifact, writeFileIfChanged,
unlinkIfPresent, legacyHooksUserPath, legacyServerUserContents,
hooksServerContents, and legacyHooksServerUserPath to locate and move the unlink
calls.
🪄 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: 72010c71-fb2b-4d3b-9a06-1b1acb000be3
📒 Files selected for processing (7)
packages/cli/src/dev.tspackages/cli/src/project/registry.tspackages/cli/tests/cli.test.tspackages/db/src/model/Entity.tspackages/db/src/model/defineModel.tspackages/db/src/model/serialize.tspackages/db/src/model/types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/dev.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/model/serialize.ts
- packages/db/src/model/types.ts
Comment on lines
+2115
to
2116
| "import { defineModel } from '@holo-js/db'", | ||
| ) |
There was a problem hiding this comment.
Keep the configured generated-schema path under test.
This test is supposed to validate custom project-path wiring, but the new assertion only checks for the generic defineModel import. It will still pass if installAuthIntoProject() hardcodes server/db/schema.generated.ts or drops the schema side-effect import entirely.
Suggested assertion tightening
- expect(await readFile(join(projectRoot, 'app/models/User.ts'), 'utf8')).toContain(
- "import { defineModel } from '@holo-js/db'",
- )
+ const userModel = await readFile(join(projectRoot, 'app/models/User.ts'), 'utf8')
+ expect(userModel).toContain("import '../db/schema.generated'")
+ expect(userModel).toContain("import { defineModel } from '@holo-js/db'")🤖 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 2115 - 2116, The test currently
only asserts the presence of "defineModel" and can pass even if the configured
generated-schema path is wrong or missing; update the test that calls
installAuthIntoProject() to assert the exact configured generated-schema import
path (the configured value passed to or referenced by installAuthIntoProject,
e.g., "server/db/schema.generated.ts" or the test's configured path variable) is
present in the produced file contents in addition to the defineModel import, so
the assertion verifies the side-effect import of the configured generated-schema
path rather than only the generic defineModel symbol.
Comment on lines
+9251
to
+9258
| while (!watchCallback || child.listenerCount('close') === 0) { | ||
| await new Promise(resolve => setTimeout(resolve, 5)) | ||
| } | ||
|
|
||
| watchCallback('change', '.holo-js/generated/registry.json') | ||
| await new Promise(resolve => setTimeout(resolve, 25)) | ||
| expect(prepare).toHaveBeenCalledTimes(1) | ||
|
|
There was a problem hiding this comment.
Wait for the initial prepare() before asserting call counts.
This new watch test starts asserting prepare counts as soon as the watcher is installed, but it never waits for the startup prepare to run. That makes toHaveBeenCalledTimes(1) timing-dependent and flaky.
Suggested fix
- while (!watchCallback || child.listenerCount('close') === 0) {
+ while (!watchCallback || prepare.mock.calls.length === 0 || child.listenerCount('close') === 0) {
await new Promise(resolve => setTimeout(resolve, 5))
}📝 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
| while (!watchCallback || child.listenerCount('close') === 0) { | |
| await new Promise(resolve => setTimeout(resolve, 5)) | |
| } | |
| watchCallback('change', '.holo-js/generated/registry.json') | |
| await new Promise(resolve => setTimeout(resolve, 25)) | |
| expect(prepare).toHaveBeenCalledTimes(1) | |
| while (!watchCallback || prepare.mock.calls.length === 0 || child.listenerCount('close') === 0) { | |
| await new Promise(resolve => setTimeout(resolve, 5)) | |
| } | |
| watchCallback('change', '.holo-js/generated/registry.json') | |
| await new Promise(resolve => setTimeout(resolve, 25)) | |
| expect(prepare).toHaveBeenCalledTimes(1) |
🤖 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 9251 - 9258, The test triggers
the watcher and immediately asserts prepare call counts, which is flaky because
the startup prepare may not have completed; after installing the watcher (where
watchCallback is captured) wait until prepare has been called at least once
(e.g., poll until prepare.mock.calls.length >= 1 or use an awaitable helper)
before invoking watchCallback('change', ...) and then assert the expected call
count (or reset/clear the mock before triggering the change and then assert
toHaveBeenCalledTimes(1)); reference the watchCallback and prepare mocks in the
test to locate where to add this wait or mock reset.
Comment on lines
+444
to
+475
| if ( | ||
| relationDef.kind === 'belongsToMany' || | ||
| relationDef.kind === 'morphToMany' || | ||
| relationDef.kind === 'morphedByMany' | ||
| ) { | ||
| return { | ||
| attach: async (ids: unknown | readonly unknown[], attributes?: Record<string, unknown>) => { | ||
| await this.attach(name, ids, attributes ?? {}) | ||
| }, | ||
| detach: async (ids?: unknown | readonly unknown[]) => { | ||
| return this.detach(name, ids) | ||
| }, | ||
| sync: async (ids: unknown) => { | ||
| return this.sync(name, ids) | ||
| }, | ||
| toggle: async (ids: unknown) => { | ||
| return this.toggle(name, ids) | ||
| }, | ||
| updateExistingPivot: async (id: unknown, attributes: Record<string, unknown>) => { | ||
| return this.updateExistingPivot(name, id, attributes) | ||
| }, | ||
| create: async (values: Record<string, unknown>) => { | ||
| return this.createRelated(name, values) | ||
| }, | ||
| save: async (related: unknown) => { | ||
| return this.saveRelated(name, related as Entity<TableDefinition>) | ||
| }, | ||
| } as unknown as RelationMethodsOf<TRelations[K]> | ||
| } | ||
|
|
||
| return {} as unknown as RelationMethodsOf<TRelations[K]> | ||
| } |
There was a problem hiding this comment.
Align the many-to-many kind check with the repository runtime.
This branch keys off 'belongsToMany', but packages/db/src/model/ModelRepository.ts:2205-2230 still classifies the same relation family under 'manyToMany'. If those discriminants differ at runtime, normal many-to-many relations will skip this helper block and fall through to {}, so attach/detach/sync never get exposed. Please normalize the discriminator or accept both aliases here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/model/Entity.ts` around lines 444 - 475, The discriminator
check in the relation helper uses relationDef.kind === 'belongsToMany' (plus
'morphToMany'/'morphedByMany') but the repository classifies the same family as
'manyToMany', causing many-to-many methods
(attach/detach/sync/toggle/updateExistingPivot/create/save) to be omitted;
update the condition in the block that returns RelationMethodsOf<TRelations[K]>
so it accepts both aliases (e.g., relationDef.kind === 'manyToMany' ||
relationDef.kind === 'belongsToMany' || relationDef.kind === 'morphToMany' ||
relationDef.kind === 'morphedByMany') or normalize the discriminator earlier to
a single canonical value matching ModelRepository (the latter via the same
normalization function used in ModelRepository), ensuring
attach/detach/sync/toggle/updateExistingPivot/create/save are exposed for the
many-to-many relation kinds.
cli.test.ts
~2115-2116 — Test missing generated-schema assertion ❌ Not a real issue. The renderAuthUserModel function receives a _generatedSchemaImportPath parameter but intentionally doesn't use it (prefixed with _). The generated User model template doesn't include a schema import, so there's nothing to assert.
cli.test.ts
~9251-9258 — Watcher test flaky prepare assertion ❌ Not flaky. The test polls while (!watchCallback || child.listenerCount('close') === 0) which ensures the initial prepare has completed before proceeding. The startup prepare runs synchronously inside runProjectDevServer before the watcher is set up.
Entity.ts
~444-475 — Missing manyToMany kind discriminator ❌ Not a real issue. There is no manyToMany kind anywhere in the codebase. The relation kinds are belongsToMany, morphToMany, and morphedByMany, all of which are already handled.
Entity.ts
~1007-1032 — Property getter returns callable wrapper ❌ Intentional design. The getter returns a function that is also thenable (.then/.catch/.finally). This enables the dual-use API: entity.posts() calls the relation method, while await entity.posts lazy-loads. Removing the wrapper would break this contract.
registry.ts
~322-330 — Legacy file deletion unconditional ❌ Correct behavior. Legacy .user.ts files are always cleaned up regardless of whether migration ran. They're artifacts from an old naming convention. The existing test at line 9489-9492 confirms this expectation by asserting the files are deleted even when the standard hook files already have user content.
This was referenced Apr 29, 2026
Merged
Merged
Merged
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
New Features
Bug Fixes
Refactor