merge dev to main #29
Conversation
* refactor: get rid of ds config provider from generated schema * update
WalkthroughThis update refactors the way database dialect configuration is supplied to the ZenStackClient, making the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant ZenStackClient
participant Database
User->>App: Start application
App->>ZenStackClient: Instantiate with schema and { dialectConfig }
ZenStackClient->>Database: Connect using provided dialectConfig
App->>ZenStackClient: Perform queries/mutations
ZenStackClient->>Database: Execute operations via dialect
Database-->>ZenStackClient: Return results
ZenStackClient-->>App: Return results
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/create-zenstack/src/index.ts (1)
113-118: Wrong sub-command for non-npm package managers
yarn install <pkg> --dev,pnpm install <pkg>etc. are invalid; they should beadd.
Switching toaddwhen the agent is notnpmavoids a hard failure.-function installPackage(pkg: { name: string; dev: boolean }) { - runCommand( - `${agent} install ${pkg.name} ${pkg.dev ? saveDev : ''}`, - `Installing "${pkg.name}"` - ); +function installPackage(pkg: { name: string; dev: boolean }) { + const verb = agent === 'npm' ? 'install' : 'add'; + runCommand( + `${agent} ${verb} ${pkg.name} ${pkg.dev ? saveDev : ''}`, + `Installing "${pkg.name}"` + ); }
♻️ Duplicate comments (2)
packages/runtime/test/client-api/default-values.test.ts (1)
67-69: Same DB-handle-cleanup concern as other testsThe test builds a new
better-sqlite3connection that is never closed. Consider applying theafterEachcleanup pattern suggested inquery-lifecycle.test.tsto prevent descriptor leaks.packages/runtime/test/client-api/name-mapping.test.ts (1)
90-92: Same resource-cleanup concern as aboveMirror the fix in the second test case to avoid open handles.
🧹 Nitpick comments (20)
TODO.md (1)
69-69: Normalize list indentation
The newly added task uses 8-space indentation, whereas other checklist items in this subsection are indented by 4 spaces.
Apply this diff to align with the existing style:- - [ ] Implement changesets + - [ ] Implement changesetspackages/runtime/test/typing/verify-typing.ts (1)
3-8: Use an in-memory DB to avoid stray artefactsPersisting
./zenstack/test.dbwill leave a file on disk after the type-check run and may cause CI parallelisation conflicts. An in-memory database is usually sufficient for typing tests and keeps the repo clean.-import SQLite from 'better-sqlite3'; +import SQLite from 'better-sqlite3'; ... - database: new SQLite('./zenstack/test.db'), + database: new SQLite(':memory:'),packages/runtime/test/policy/utils.ts (1)
9-15: Drop the double type assertions – they mask real typing issues
createTestClientis already generic; the extraas anyonschemaand the cast on the options object are unnecessary and weaken type safety.- return createTestClient( - schema as any, - { - ...options, - plugins: [new PolicyPlugin()], - } as CreateTestClientOptions<SchemaDef> - ); + return createTestClient(schema, { + ...options, + plugins: [new PolicyPlugin()], + });packages/runtime/test/plugin/mutation-hooks.test.ts (2)
7-7: Remove duplicate space in suite title-describe('Entity lifecycle tests', () => { +describe('Entity lifecycle tests', () => {
11-15: Consider closing the SQLite handle after the suite
better-sqlite3keeps the connection open untildb.close()is called.
Add anafterAllto close it and release resources:let _client: ClientContract<typeof schema>; +let _db: SQLite.Database; beforeEach(async () => { - _client = new ZenStackClient(schema, { - dialectConfig: { - database: new SQLite(':memory:'), - }, - }); + _db = new SQLite(':memory:'); + _client = new ZenStackClient(schema, { dialectConfig: { database: _db } }); await _client.$pushSchema(); }); + +afterAll(() => _db.close());packages/cli/src/actions/templates.ts (1)
34-38: Path mismatch between Prisma URL and runtime DBThe datasource URL in the generated schema points to
file:./dev.db, but the sample code opens./zenstack/dev.db.
To prevent confusion (two separate files depending on the working dir) align the paths:- database: new SQLite('./zenstack/dev.db'), + database: new SQLite('./dev.db'),(or change the datasource URL to
file:./zenstack/dev.db).Please verify which location you want.
packages/runtime/test/plugin/query-lifecycle.test.ts (1)
10-14: Dispose the in-memory SQLite handle to avoid leaked FDs across testsEach
beforeEachcreates a freshbetter-sqlite3instance but it’s never closed, so file descriptors accumulate when the suite grows.
A minimal fix is to retain theDatabaseobject and calldb.close()in anafterEach.let _client: ClientContract<typeof schema>; +let _db: SQLite.Database; beforeEach(async () => { - _client = new ZenStackClient(schema, { - dialectConfig: { - database: new SQLite(':memory:'), - }, - }); + _db = new SQLite(':memory:'); + _client = new ZenStackClient(schema, { + dialectConfig: { database: _db }, + }); await _client.$pushSchema(); }); +afterEach(() => { + _db.close(); +});samples/blog/README.md (1)
25-28: Nit: make the DB path OS-portableUsing a hard-coded
'./zenstack/dev.db'is fine, butpath.joinavoids subtle issues with differing CWDs or separators:-import SQLite from 'better-sqlite3'; -const db = ZenStackClient(schema, { - dialectConfig: { database: new SQLite('./zenstack/dev.db') }, -}); +import path from 'path'; +import SQLite from 'better-sqlite3'; +const db = ZenStackClient(schema, { + dialectConfig: { + database: new SQLite(path.join(__dirname, 'zenstack', 'dev.db')), + }, +});samples/blog/main.ts (1)
2-2: Portability & cleanup improvements
path.join(see README note) avoids path issues whenmain.tsis executed from a different CWD.- Consider storing the
SQLite.Databaseinstance in a variable and callingclose()beforeprocess.exit, especially in long-running demos.-import SQLite from 'better-sqlite3'; +import path from 'path'; +import SQLite from 'better-sqlite3'; - dialectConfig: { - database: new SQLite('./zenstack/dev.db'), - }, + dialectConfig: { + database: new SQLite( + path.join(__dirname, 'zenstack', 'dev.db') + ), + },Also applies to: 7-9
README.md (4)
51-52: Great call-out to the sample – consider linking to the runnable entry fileThe added note is helpful. A quick link to
samples/blog/main.ts(or equivalent) would make navigation even smoother for readers skimming the README.
141-145: Code snippet could use a concrete driver exampleNew users may not realise they need to pass an actual driver instance, e.g.
import SQLite from 'better-sqlite3'; const client = new ZenStackClient(schema, { dialectConfig: { database: new SQLite('db.sqlite') }, });Explicitly showing one realistic config avoids “what goes inside
{ ... }?” questions.
214-215: Minor formatting: stray leading spacesThere’s an extra indentation before
postCountmaking the fenced code block mis-align. Shaving those two spaces keeps the example tidy.
375-376: Replace absolute path interpolation with a relative one
'${outputPath}/schema'in the migration guide may expand to an absolute path (/Users/alex/dev/…) which won’t compile in published code. Using'./schema'(or leaving the path unresolved) avoids confusion.-import { schema } from '${outputPath}/schema'; +import { schema } from './schema';packages/runtime/test/client-api/name-mapping.test.ts (1)
58-60: Keep a handle to the SQLite instance so it can be closed
better-sqlite3keeps the DB open untildb.close()is called. In long test runs this leaks file descriptors.-const client = new ZenStackClient(schema, { - dialectConfig: { database: new SQLite(':memory:') }, -}); +const db = new SQLite(':memory:'); +const client = new ZenStackClient(schema, { + dialectConfig: { database: db }, +}); +afterAll(() => db.close());packages/cli/src/actions/generate.ts (1)
43-45: Sample import path may output an absolute filesystem pathWhen
outputPathis absolute, the snippet inside the green message turns intoimport { schema } from '/tmp/foo/schema', which breaks copy-paste usage. Prefer a relative path in the example:-import { schema } from '${outputPath}/schema'; +import { schema } from './schema';packages/runtime/test/plugin/kysely-on-query.test.ts (1)
17-19: Close the in-memory SQLite DB after each suiteJust like the other tests, hold a reference to
new SQLite(':memory:')and close it in anafterEachhook to keep the test runner clean.-let _client: ClientContract<typeof schema>; +let _client: ClientContract<typeof schema>; +let _db: SQLite.Database; -beforeEach(async () => { - _client = new ZenStackClient(schema, { - dialectConfig: { database: new SQLite(':memory:') }, - }); +beforeEach(async () => { + _db = new SQLite(':memory:'); + _client = new ZenStackClient(schema, { + dialectConfig: { database: _db }, + }); }); +afterEach(() => _db.close());packages/runtime/src/client/client-impl.ts (1)
59-64: Redundant nullish-coalescing –optionscan no longer beundefined
optionsis now mandatory, so the fallback?? {}is dead code.- this.$options = options ?? ({} as ClientOptions<Schema>); + this.$options = options;packages/runtime/test/utils.ts (3)
21-25: Heavy casting hides type gaps
as unknown as ClientOptions<Schema>circumvents the new mandatorydialectConfigat compile-time. Prefer building a proper object and letting the compiler ensure safety.
98-104:pluginsremoved from options before being typedAfter destructuring,
_optionsis asserted asClientOptionsbut is still missingdialectConfig(added later).
Until the assignment below, the object violates the type – remove the premature cast.- const _options: ClientOptions<Schema> = { - ...rest, - } as ClientOptions<Schema>; + const _options = { + ...rest, + } as Partial<ClientOptions<Schema>>;
136-151:providerdefaulting to SQLite is implicitWhen
provideris omitted the else-branch assumes SQLite.
Explicitly defaulting improves readability and prevents silent mis-config.- if (options?.provider === 'postgresql') { + const provider = options?.provider ?? 'sqlite'; + if (provider === 'postgresql') { _options.dialectConfig = { @@ - } else { + } else if (provider === 'sqlite') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
README.md(4 hunks)TODO.md(1 hunks)packages/cli/src/actions/generate.ts(1 hunks)packages/cli/src/actions/templates.ts(1 hunks)packages/cli/test/ts-schema-gen.test.ts(0 hunks)packages/create-zenstack/src/index.ts(1 hunks)packages/runtime/package.json(0 hunks)packages/runtime/src/client/client-impl.ts(4 hunks)packages/runtime/src/client/options.ts(1 hunks)packages/runtime/src/utils/pg-utils.ts(0 hunks)packages/runtime/src/utils/sqlite-utils.ts(0 hunks)packages/runtime/test/client-api/default-values.test.ts(1 hunks)packages/runtime/test/client-api/name-mapping.test.ts(2 hunks)packages/runtime/test/plugin/kysely-on-query.test.ts(2 hunks)packages/runtime/test/plugin/mutation-hooks.test.ts(1 hunks)packages/runtime/test/plugin/query-lifecycle.test.ts(2 hunks)packages/runtime/test/policy/utils.ts(1 hunks)packages/runtime/test/test-schema.ts(0 hunks)packages/runtime/test/typing/schema.ts(1 hunks)packages/runtime/test/typing/verify-typing.ts(1 hunks)packages/runtime/test/utils.ts(5 hunks)packages/runtime/tsup.config.ts(0 hunks)packages/sdk/src/schema/schema.ts(0 hunks)packages/sdk/src/ts-schema-generator.ts(0 hunks)samples/blog/README.md(1 hunks)samples/blog/main.ts(1 hunks)samples/blog/zenstack/schema.ts(1 hunks)samples/blog/zenstack/schema.zmodel(1 hunks)
💤 Files with no reviewable changes (8)
- packages/cli/test/ts-schema-gen.test.ts
- packages/runtime/tsup.config.ts
- packages/sdk/src/schema/schema.ts
- packages/runtime/package.json
- packages/runtime/src/utils/pg-utils.ts
- packages/runtime/test/test-schema.ts
- packages/sdk/src/ts-schema-generator.ts
- packages/runtime/src/utils/sqlite-utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/runtime/test/plugin/query-lifecycle.test.ts (2)
packages/runtime/src/client/client-impl.ts (1)
ZenStackClient(41-47)packages/runtime/test/test-schema.ts (1)
schema(7-328)
packages/runtime/test/typing/verify-typing.ts (2)
packages/runtime/src/client/client-impl.ts (1)
ZenStackClient(41-47)packages/runtime/test/typing/schema.ts (1)
schema(11-417)
🪛 markdownlint-cli2 (0.17.2)
TODO.md
69-69: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (6)
samples/blog/zenstack/schema.zmodel (1)
12-20: Formatting-only change looks goodPure whitespace/indentation tweak, no semantic impact on the schema.
packages/runtime/src/client/options.ts (1)
46-51: MakingdialectConfigmandatory is a breaking change – scan for missing argumentsGreat to see the option becoming explicit, but any call‐sites that still rely on the old optional behaviour will now fail at compile/run time.
Run the quick grep below; expect zero results. Any hit indicates residual-code that needs updating.
#!/bin/bash # list ZenStackClient constructions that don't mention "dialectConfig" rg -n --no-heading $'ZenStackClient\\([^,]*\\)$' -g '!**/node_modules/**'samples/blog/zenstack/schema.ts (1)
8-10: Explicit provider looks good – remember to always passdialectConfigat client construction timeRemoving the
dialectConfigProviderand keeping onlytype: "sqlite"matches the new API requirement.
Just double-check that every place constructing aZenStackClientfor this schema now supplies a proper{ dialectConfig }option, otherwise runtime connection errors will surface.packages/runtime/test/plugin/kysely-on-query.test.ts (1)
27-32: Updated interceptor signature looks correctThe destructured
{ query, proceed }signature aligns with the new API – no issues spotted.packages/create-zenstack/src/index.ts (1)
14-24: To confirm howagentExecis used and whether it needs to call the binary runner (e.g.pnpm exec,yarn dlx,bunx), let’s inspect the surrounding code:#!/bin/bash # Locate the file under review file=$(find . -type f -path "*packages/create-zenstack/src/index.ts") echo "File: $file" # Show where agentExec is set echo "----- agentExec declaration -----" rg -n "agentExec" -A3 -B3 "$file" # Show how agentExec is actually executed (execa/spawn/etc.) echo "----- Usage of agentExec / execa / spawn -----" rg -n -e "agentExec" -e "execa" -e "spawn" -e "execSync" -A3 -B3 "$file"packages/runtime/test/typing/schema.ts (1)
6-10: Generated file – skippedThis file is generated; no manual changes required.
| private makePostgresKyselyDialect(): PostgresDialect { | ||
| const { dialectConfigProvider } = this.schema.provider; | ||
| const mergedConfig = { | ||
| ...dialectConfigProvider(), | ||
| ...this.options?.dialectConfig, | ||
| } as PostgresDialectConfig; | ||
| return new PostgresDialect(mergedConfig); | ||
| return new PostgresDialect( | ||
| this.options.dialectConfig as PostgresDialectConfig | ||
| ); | ||
| } | ||
|
|
||
| private makeSqliteKyselyDialect(): SqliteDialect { | ||
| const { dialectConfigProvider } = this.schema.provider; | ||
| const mergedConfig = { | ||
| ...dialectConfigProvider(), | ||
| ...this.options?.dialectConfig, | ||
| } as SqliteDialectConfig; | ||
| return new SqliteDialect(mergedConfig); | ||
| return new SqliteDialect( | ||
| this.options.dialectConfig as SqliteDialectConfig | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No runtime guard that dialectConfig matches provider
If a caller passes a PostgreSQL dialectConfig while schema.provider.type is 'sqlite' (or vice-versa) the dialect will be built with an incompatible config and fail later.
Add an assertion to fail fast:
private makePostgresKyselyDialect(): PostgresDialect {
- return new PostgresDialect(
- this.options.dialectConfig as PostgresDialectConfig
- );
+ if (this.schema.provider.type !== 'postgresql') {
+ throw new Error('PostgreSQL dialect requested for non-postgres provider');
+ }
+ return new PostgresDialect(
+ this.options.dialectConfig as PostgresDialectConfig
+ );
}(Same check for SQLite.)
📝 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.
| private makePostgresKyselyDialect(): PostgresDialect { | |
| const { dialectConfigProvider } = this.schema.provider; | |
| const mergedConfig = { | |
| ...dialectConfigProvider(), | |
| ...this.options?.dialectConfig, | |
| } as PostgresDialectConfig; | |
| return new PostgresDialect(mergedConfig); | |
| return new PostgresDialect( | |
| this.options.dialectConfig as PostgresDialectConfig | |
| ); | |
| } | |
| private makeSqliteKyselyDialect(): SqliteDialect { | |
| const { dialectConfigProvider } = this.schema.provider; | |
| const mergedConfig = { | |
| ...dialectConfigProvider(), | |
| ...this.options?.dialectConfig, | |
| } as SqliteDialectConfig; | |
| return new SqliteDialect(mergedConfig); | |
| return new SqliteDialect( | |
| this.options.dialectConfig as SqliteDialectConfig | |
| ); | |
| private makePostgresKyselyDialect(): PostgresDialect { | |
| if (this.schema.provider.type !== 'postgresql') { | |
| throw new Error('PostgreSQL dialect requested for non-postgres provider'); | |
| } | |
| return new PostgresDialect( | |
| this.options.dialectConfig as PostgresDialectConfig | |
| ); | |
| } | |
| private makeSqliteKyselyDialect(): SqliteDialect { | |
| return new SqliteDialect( | |
| this.options.dialectConfig as SqliteDialectConfig | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/runtime/src/client/client-impl.ts around lines 142 to 151, add
runtime assertions to verify that the dialectConfig matches the expected
provider type before constructing the dialect. For the PostgreSQL dialect,
assert that the provider type is 'postgres', and for the SQLite dialect, assert
that it is 'sqlite'. This will ensure that incompatible configs are caught early
and prevent later failures.
| const content = fs.readFileSync(prismaSchemaPath, 'utf-8'); | ||
| const found = content.match(/^\s*url\s*=(\s|")*([^"]+)(\s|")*$/m); | ||
| if (!found) { | ||
| throw new Error('No url found in prisma schema'); | ||
| } | ||
| const dbPath = found[2]!; | ||
| // convert 'file:./dev.db' to './dev.db' | ||
| const r = path.join( | ||
| path.dirname(prismaSchemaPath), | ||
| dbPath.replace(/^file:/, '') | ||
| ); | ||
| return r; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Fragile regex for extracting DB path from Prisma schema
The current pattern breaks when:
• the url is wrapped in env()
• the value is single-quoted
• there are additional attributes on the line.
Consider using Prisma’s own parser or a stricter block parser instead of a single-line regex.
Fragile regex for extracting DB path from Prisma schema
The current pattern breaks when:
• the url is wrapped in env()
• the value is single-quoted
• there are additional attributes on the line.
Consider using Prisma’s own parser or a stricter block parser instead of a single-line regex.
🤖 Prompt for AI Agents
In packages/runtime/test/utils.ts around lines 169 to 180, the regex used to
extract the database path from the Prisma schema is fragile and fails with env()
usage, single quotes, or extra attributes. Replace the regex approach with
Prisma's official schema parser or a more robust block parser to accurately
extract the url value, handling different quoting styles and ignoring additional
attributes on the line.
Summary by CodeRabbit
signUpprocedure from sample code and schema.