Refactor database conditionals for readability#13
Conversation
WalkthroughConsolidates DB/npm-script generation into engine-driven mappings with an isLocal flag; introduces a typed top-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as Generator CLI
participant GenPkg as generatePackageJson
participant GenDocker as generateDockerContainer
participant GenDB as generateDBBlock
participant GenImport as generateImportsBlock
CLI->>GenPkg: provide config (engine, host, orm, flags)
alt isLocal && engine supports docker
GenPkg-->>CLI: emit unified scripts (db:up, postdb:up, db:down, db:reset, db:<engine>)
else sqlite/remote/unsupported
GenPkg-->>CLI: emit sqlite-specific or no-local scripts
end
CLI->>GenDocker: request container templates
GenDocker-->>CLI: return templates {image, port, env, volumePath, optional command}
CLI->>GenDB: request DB snippet (orm, engine, host)
alt orm == drizzle
GenDB-->>CLI: emit drizzle connection (createPool for mysql/mariadb or SQL/driver expr)
else
GenDB-->>CLI: emit single-line `const db = ...` or empty
end
CLI->>GenImport: request imports (orm, engine, host, isDrizzleDialect)
GenImport-->>CLI: assemble imports via ormImports / ormDatabaseImports / noOrmImports maps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (3)src/generators/project/generateImportsBlock.ts (1)
src/generators/db/generateDockerContainer.ts (1)
src/generators/configurations/generatePackageJson.ts (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/generators/db/generateDockerContainer.ts (1)
4-88: Avoid emitting an emptycommand:in docker-compose for most enginesRight now
commandis set to''for all engines except CockroachDB, but the compose template always emits:command: ${command}For those engines this becomes
command:with an empty value, which can override the image’s default CMD and potentially prevent the DB from starting correctly. It’s safer to omit thecommandline entirely unless an override is required.You can fix this by making the command line conditional, e.g.:
-const templates = { - cockroachdb: { - command: 'start-single-node --insecure', +const templates = { + cockroachdb: { + command: 'start-single-node --insecure', @@ -const { image, port, env, volumePath, command } = templates[databaseEngine]; +const { image, port, env, volumePath, command } = templates[databaseEngine]; +const commandLine = command ? `\n command: ${command}` : ''; @@ - ports: - - "${port}" - command: ${command} - volumes: + ports: + - "${port}"${commandLine} + volumes: - db_data:${volumePath}
🧹 Nitpick comments (3)
src/generators/configurations/generatePackageJson.ts (3)
32-54: Consider adding password environment variables for MySQL/MariaDB client commands.The
clientCmdformysqlandmariadblacks theMYSQL_PWD=userpasswordenvironment variable present ininitTemplates.cli. When users runbun db:mysqlorbun db:mariadb, they'll be prompted for a password, which may be unexpected since the docker container has a preset password.Apply this diff to add the password environment variable:
mariadb: { - clientCmd: 'mariadb -h127.0.0.1 -u user database', + clientCmd: 'MYSQL_PWD=userpassword mariadb -h127.0.0.1 -u user database', waitCmd: initTemplates.mariadb.wait },mysql: { - clientCmd: 'mysql -h127.0.0.1 -u user database', + clientCmd: 'MYSQL_PWD=userpassword mysql -h127.0.0.1 -u user database', waitCmd: initTemplates.mysql.wait },
202-202: Add runtime type guard for safer dbScripts access.While the current logic ensures only engines present in
dbScriptsreach line 202, there's no type-level constraint. If a new database engine is added to the type system in the future without updatingdbScripts, this will cause a runtime error.Apply this diff to add a type guard:
} else { + if (!(databaseEngine in dbScripts)) { + throw new Error( + `Internal error: Database engine "${databaseEngine}" not supported in dbScripts` + ); + } const config = dbScripts[databaseEngine];
210-211: Consider adding-itflags for interactive database client scripts.The generated
db:<engine>scripts (e.g.,db:mysql,db:postgresql) are likely intended for interactive shell access, but thedocker compose execcommand doesn't include-itflags for interactive terminal support. Users may experience issues with terminal input/output.Apply this diff to add interactive flags:
scripts[`db:${databaseEngine}`] = - `${dockerPrefix} exec db bash -lc '${config.clientCmd}'`; + `${dockerPrefix} exec -it db bash -lc '${config.clientCmd}'`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/generators/configurations/generatePackageJson.ts(3 hunks)src/generators/db/dockerInitTemplates.ts(1 hunks)src/generators/db/generateDockerContainer.ts(3 hunks)src/generators/db/generateDrizzleSchema.ts(3 hunks)src/generators/db/handlerTemplates.ts(1 hunks)src/generators/project/generateDBBlock.ts(1 hunks)src/generators/project/generateImportsBlock.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-22T04:55:44.009Z
Learnt from: bnziv
Repo: absolutejs/create-absolutejs PR: 11
File: src/generators/configurations/generateEnv.ts:17-17
Timestamp: 2025-11-22T04:55:44.009Z
Learning: The mssql npm package supports Windows-style connection string format (key=value pairs separated by semicolons) in addition to URI format. Connection strings like "Server=localhost,1433;Database=master;User Id=sa;Password=SApassword1;Encrypt=true;TrustServerCertificate=true" are valid for the mssql package.
Applied to files:
src/generators/db/dockerInitTemplates.ts
🧬 Code graph analysis (2)
src/generators/project/generateImportsBlock.ts (2)
src/typeGuards.ts (1)
isDrizzleDialect(27-30)src/generators/db/scaffoldDatabase.ts (1)
ScaffoldDatabaseProps(27-107)
src/generators/configurations/generatePackageJson.ts (3)
src/generators/db/dockerInitTemplates.ts (1)
initTemplates(112-141)src/generators/db/scaffoldDocker.ts (1)
ScaffoldDockerProps(20-56)src/generators/db/scaffoldDatabase.ts (1)
ScaffoldDatabaseProps(27-107)
🔇 Additional comments (4)
src/generators/db/handlerTemplates.ts (1)
327-334: MariaDB drizzle driver configuration looks consistentThe new
mariadb:drizzle:localentry matches the existing MySQL drizzle configuration (dbType, imports, and query operations) and should work correctly withdrizzle-orm/mysql2+ MariaDB.src/generators/db/generateDrizzleSchema.ts (1)
16-23: MariaDB dialect wiring is consistent with existing MySQL/singlestore behaviorThe new
mariadbentry inDIALECTSmirrors the MySQL setup (mysql-core builders), and the updatedintBuilder/uidColumnconditions correctly treat MariaDB like MySQL/Singlestore for autoincrement integer PKs. This keeps schema generation consistent across these dialects.Also applies to: 72-77, 111-121
src/generators/db/dockerInitTemplates.ts (1)
127-127: LGTM! Improved string readability.The change from escaped single quotes to a double-quoted string with embedded single quotes improves readability without changing functionality.
src/generators/configurations/generatePackageJson.ts (1)
219-232: LGTM! Database-specific dependency logic is correct.The conditional addition of
mysql2for MySQL/MariaDB with Drizzle andmssqlpackages for MS SQL Server is implemented correctly and maintains consistency with the previous logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/generators/project/generateImportsBlock.ts (1)
130-130: Past critical issue resolved.The missing closing backtick in the PostgreSQL drizzle import string has been fixed. The template literal is now properly terminated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/generators/project/generateDBBlock.ts(1 hunks)src/generators/project/generateImportsBlock.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/generators/project/generateDBBlock.ts (4)
src/generators/db/generateDrizzleSchema.ts (1)
GenerateSchemaProps(66-163)src/generators/db/generateHandlers.ts (1)
GenerateDBHandlersProps(9-28)src/generators/db/scaffoldDatabase.ts (1)
ScaffoldDatabaseProps(27-107)src/generators/configurations/generateEnv.ts (1)
GenerateEnvProps(23-44)
src/generators/project/generateImportsBlock.ts (3)
src/typeGuards.ts (1)
isDrizzleDialect(27-30)src/generators/db/generateDrizzleSchema.ts (1)
GenerateSchemaProps(66-163)src/generators/db/scaffoldDatabase.ts (1)
ScaffoldDatabaseProps(27-107)
🔇 Additional comments (1)
src/generators/project/generateImportsBlock.ts (1)
102-202: LGTM: Well-structured refactor with clear separation of concerns.The introduction of structured import maps (
ormImports,ormDatabaseImports,noOrmImports) significantly improves maintainability over the previous ad-hoc approach. The conditional assembly logic (lines 185-202) correctly:
- Pushes base ORM imports for drizzle
- Adds remote connector/dialect imports when
isRemoteHost- Includes database-specific imports when
isDrizzleDialect(databaseEngine)is true- Falls back to
noOrmImportsfor no-ORM configurationsThe alignment between these imports and the usage in
generateDBBlock.tsappears correct for all database engine and host combinations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/generators/configurations/generatePackageJson.ts (1)
190-233: DB scripts guard looks sane, but indexing intodbScriptscould be saferThe new guard around DB script generation:
if ( (!databaseHost || databaseHost === 'none') && databaseEngine !== undefined && databaseEngine !== 'none' && databaseEngine !== 'mongodb' && databaseEngine !== 'singlestore' && databaseEngine !== 'gel' ) { … }nicely confines docker‑based scripts to local, SQL‑style engines and keeps sqlite in an explicit branch, which improves readability.
Inside the
elsebranch you rely on:const config = dbScripts[databaseEngine];This is correct for the current union (cockroachdb, mariadb, mssql, mysql, postgresql) plus sqlite handled above, but it depends on
CreateConfiguration['databaseEngine']staying in sync withkeyof typeof dbScriptsand the exclusion list. A future engine added to the union but not todbScripts/exclusions will yieldconfigasundefinedat runtime.Consider tightening this a bit by either:
- Typing
databaseEngineaskeyof typeof dbScripts | 'sqlite' | 'mongodb' | 'singlestore' | 'gel' | 'none', or- Adding a small runtime guard:
const config = dbScripts[databaseEngine as keyof typeof dbScripts]; if (!config) { throw new Error(`Unsupported databaseEngine for db scripts: ${databaseEngine}`); }to fail fast rather than producing a cryptic error if things drift.
Otherwise, the consolidated scripts (
db:up,postdb:up,db:down,db:reset,db:<engine>, plus pre/post dev hooks) and the conditional mysql2/mssql dependencies look consistent and easier to follow than the engine‑specific branches they replace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/generators/configurations/generatePackageJson.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/generators/configurations/generatePackageJson.ts (1)
src/generators/db/dockerInitTemplates.ts (1)
initTemplates(112-141)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/generators/project/generateDBBlock.ts (1)
77-77: Minor: expr retrieved but unused for mysql/mariadb in Drizzle path.Line 77 retrieves
exprfromconnectionMap[databaseEngine][hostKey], which formysql.noneormariadb.noneevaluates tonew SQL(getEnv("DATABASE_URL"))(from lines 14, 23). However, the mysql/mariadb block (lines 80-90) ignores thisexprand hardcodescreatePool(getEnv("DATABASE_URL"))on line 87. Theexprvariable is only used in the fallback block (lines 92-95) for other database engines.Consider refactoring to avoid the unused retrieval by moving line 77 inside the final else block, or document why different connection types are needed for Drizzle mysql/mariadb vs non-Drizzle.
Apply this diff to defer expr retrieval until it's actually needed:
if (!drizzleDialectSet.has(databaseEngine)) return ''; - const expr = engineGroup[hostKey]?.expr ?? remoteDrizzleInit[hostKey]; - if (!expr) return ''; - if (databaseEngine === 'mysql' || databaseEngine === 'mariadb') { const mode = databaseHost === 'planetscale' && databaseEngine === 'mysql' ? 'planetscale' : 'default'; return ` const pool = createPool(getEnv("DATABASE_URL")) const db = drizzle(pool, { schema, mode: '${mode}' }) `; } + const expr = engineGroup[hostKey]?.expr ?? remoteDrizzleInit[hostKey]; + if (!expr) return ''; + return ` const pool = ${expr} const db = drizzle(pool, { schema }) `;Also applies to: 80-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/generators/db/dockerInitTemplates.ts(1 hunks)src/generators/project/generateDBBlock.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-22T04:55:44.021Z
Learnt from: bnziv
Repo: absolutejs/create-absolutejs PR: 11
File: src/generators/configurations/generateEnv.ts:17-17
Timestamp: 2025-11-22T04:55:44.021Z
Learning: The mssql npm package supports Windows-style connection string format (key=value pairs separated by semicolons) in addition to URI format. Connection strings like "Server=localhost,1433;Database=master;User Id=sa;Password=SApassword1;Encrypt=true;TrustServerCertificate=true" are valid for the mssql package.
Applied to files:
src/generators/db/dockerInitTemplates.ts
🔇 Additional comments (3)
src/generators/db/dockerInitTemplates.ts (1)
127-127: LGTM! Quote style improvement for readability.The change from escaped single quotes to double quotes improves readability while maintaining identical functionality in the shell command context.
src/generators/project/generateDBBlock.ts (2)
14-14: Good fix! connectionMap now aligns with non-Drizzle usage.Updating these entries to
new SQL(getEnv("DATABASE_URL"))resolves the inconsistency flagged in the previous review where connectionMap entries were effectively dead code due to the hardcoded branch.Based on past review comment.
Also applies to: 23-23
72-72: Good simplification! Hardcoded branch removed as suggested.The removal of the hardcoded special case for mysql/mariadb
databaseHost === 'none'makes the code flow through connectionMap consistently for all non-Drizzle cases, addressing the previous review feedback.Based on past review comment.
bnziv
force-pushed
the
fix/code-cleanup
branch
from
43f91f5 to
299593f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/generators/project/generateDBBlock.ts (1)
16-18: MongoDB connection expression appears malformed
connectionMap.mongodb.nonecurrently uses:none: { expr: 'new MongoClient(getEnv("DATABASE_URL") })' }The extra
}/)will produce invalid generated code. This should be corrected to a balanced call, e.g.:- none: { expr: 'new MongoClient(getEnv("DATABASE_URL") })' } + none: { expr: 'new MongoClient(getEnv("DATABASE_URL"))' }
🧹 Nitpick comments (3)
src/generators/db/handlerTemplates.ts (1)
327-334: MariaDB drizzle config mirrors MySQL drizzle; consider deduping shared configThe new
'mariadb:drizzle:local'entry correctly reuses the MySql2Database +mysqlDrizzleQueryOperationssetup and looks functionally sound. To reduce duplication, you could factor the shared importLines/dbType/queries into a small helper used by both the MySQL and MariaDB drizzle keys.src/generators/project/generateDBBlock.ts (1)
69-72: Drizzle MySQL/MariaDB branch now hardcodes mysql2 pool; confirm remote Planetscale intentFor
orm === 'drizzle'anddatabaseEnginein{ mysql, mariadb }, the block always does:const pool = createPool(getEnv("DATABASE_URL")) const db = drizzle(pool, { schema, mode: /* 'default' | 'planetscale' */ })ignoring the
exprfromconnectionMap/remoteDrizzleInit. This means:
- Remote MySQL+Planetscale drizzle no longer uses the Planetscale-specific client expression from
connectionMap.- The runtime now always depends on the mysql2-based path for drizzle MySQL/MariaDB.
Please double-check this is intentional and aligned with the imports/dependency setup (in particular with how
generateImportsBlock.tsandcreatePackageJson.tshandle Planetscale andmysql2). Based on library_context, this is a behavior change from the previous engine-driven approach.Also applies to: 80-89
src/generators/project/generateImportsBlock.ts (1)
185-199: Centralized drizzle import maps look good; minor redundancy for remote MySQLThe new
ormImports/ormDatabaseImports+isDrizzleDialectusage makes the drizzle import path much easier to follow. One side effect is that for drizzle + remote MySQL (e.g. Planetscale), you may now import both the mysql2 drizzle adapter and the remote connector/dialect pair even thoughgenerateDBBlockuses only the mysql2 path. This is harmless but slightly noisy; if you want to tighten things up, you could either drop the unused remote imports for that combo or switchgenerateDBBlockback to using the remote connector expression.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/generators/configurations/generatePackageJson.ts(3 hunks)src/generators/db/dockerInitTemplates.ts(1 hunks)src/generators/db/generateDockerContainer.ts(3 hunks)src/generators/db/generateDrizzleSchema.ts(3 hunks)src/generators/db/handlerTemplates.ts(1 hunks)src/generators/project/generateDBBlock.ts(4 hunks)src/generators/project/generateImportsBlock.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/generators/db/dockerInitTemplates.ts
- src/generators/db/generateDockerContainer.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/generators/project/generateImportsBlock.ts (1)
src/typeGuards.ts (1)
isDrizzleDialect(27-30)
src/generators/configurations/generatePackageJson.ts (1)
src/generators/db/dockerInitTemplates.ts (1)
initTemplates(112-141)
🔇 Additional comments (2)
src/generators/db/generateDrizzleSchema.ts (1)
16-23: MariaDB dialect wiring is consistent with MySQL/SingleStoreAdding
DIALECTS.mariadband including'mariadb'in theintBuilderanduidColumnbranches makes MariaDB behave like MySQL/SingleStore (int builder + autoincrement PK), which matches the rest of the schema code. This looks correct and consistent.Also applies to: 73-76, 111-115
src/generators/project/generateImportsBlock.ts (1)
143-183: SingleStore non‑ORM path lacks imports, but proposed package name appears incorrectThe core issue is valid:
noOrmImports.singlestoreis an empty array, butgenerateDBBlock.tsline 33 generates code callingcreateClient({ url: getEnv("DATABASE_URL") })for the non-ORM SingleStore path. This will cause a runtime error when those symbols are undefined.However, the proposed fix references
@singlestore/db-client, which does not exist as a real npm package. Web search confirms the official SingleStore Node.js packages use names like@singlestore/client. Additionally,handlerTemplates.tsline 391 also references the same non-existent package, suggesting a broader codebase inconsistency.Before applying the suggested fix, verify:
- The correct SingleStore package name and export (e.g., is it
SingleStoreClientfrom@singlestore/client, or does the codebase use mysql2 for SingleStore compatibility?)- Whether
handlerTemplates.tsline 391 has the same issue and should be updated together- What the actual generated code should import for SingleStore non-ORM connections
bnziv
force-pushed
the
fix/code-cleanup
branch
from
299593f to
cdf5b75
Compare
| (!databaseHost || databaseHost === 'none') | ||
| ) { | ||
| const { wait } = initTemplates.mssql; | ||
| if (isLocal && databaseEngine === 'mssql') { |
There was a problem hiding this comment.
if this does not us drizzle doesnt that mean we can use the bun version of mssql
| volumePath: '/var/opt/mssql' | ||
| }, | ||
| mysql: { | ||
| command: '', |
There was a problem hiding this comment.
if the command is not necessary for most of them make it an optional property instead of giving it empty string
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/generators/db/generateDockerContainer.ts (2)
3-14: Nice centralization of DB templates with a strongly typed mapDefining
DatabaseTemplateand usingRecord<Exclude<DatabaseEngine, 'none' | 'sqlite' | undefined>, DatabaseTemplate>makes the per-engine config much clearer and helps ensure all real engines have entries. The sharedvolumePathand optionalcommandfields also remove a lot of duplication. No issues here.If you ever want even stronger guarantees, you could later replace
Record<...>with a discriminated union of exact engine keys to get exhaustiveness when adding engines, but what you have is already solid for readability.Also applies to: 16-16, 22-22, 32-32, 43-43, 53-53, 62-62, 73-73, 83-83, 91-91, 93-93
95-105: Runtime guard could be encoded in the type for stricter callers (optional)Given this function throws for
undefined | 'none' | 'sqlite', its effective domain is alreadyExclude<DatabaseEngine, 'none' | 'sqlite' | undefined>. To push that invariant to callers and reduce the need for the runtime check, you could expose a narrower parameter type or a small helper that returns a validated engine and use that inside.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/generators/db/generateDockerContainer.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/generators/db/generateDockerContainer.ts (1)
src/types.ts (1)
DatabaseEngine(45-47)
bnziv
force-pushed
the
fix/code-cleanup
branch
from
e46ba0b to
151f098
Compare
bnziv
force-pushed
the
fix/code-cleanup
branch
from
151f098 to
0678b8d
Compare
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.