perf(ztd-cli): reuse DDL analysis in ztd-config#672
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR extracts a shared DDL analysis (parsing + linting) returned as Changes
Sequence DiagramsequenceDiagram
participant CLI as ztd-config CLI
participant Analysis as DDL Analysis
participant Linting as Linting
participant Metadata as Table Metadata
participant Config as Config Persist
CLI->>Analysis: analyzeDdlSources(sources)
activate Analysis
Analysis->>Analysis: parse SQL & produce AST groups + diagnostics
Analysis-->>CLI: return analysis (create/alter/index + diagnostics)
deactivate Analysis
CLI->>Linting: validate using analysis.diagnostics
Linting-->>CLI: diagnostics result
CLI->>Metadata: buildTableMetadataFromCreateStatements(analysis.createStatements)
activate Metadata
Metadata->>Metadata: construct TableMetadata (dedupe & normalize)
Metadata-->>CLI: table metadata
deactivate Metadata
CLI->>Config: writeZtdProjectConfig(rootDir, overrides, baseConfig)
activate Config
Config->>Config: compare serialized base vs final config
alt configs match
Config-->>CLI: return false
else configs differ
Config->>Config: write file
Config-->>CLI: return true
end
deactivate Config
CLI->>CLI: emit telemetry only if configUpdated == true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
packages/testkit-core/tests/ddlLint.test.ts (1)
27-30: Consider assertingalterStatementsandindexStatementsfor complete coverage.The test only checks
createStatements.length. Adding assertions for the other statement arrays would verify the full analysis result:♻️ Optional enhancement
const analysis = analyzeDdlSources(sources); expect(analysis.createStatements).toHaveLength(1); + expect(analysis.indexStatements).toHaveLength(1); + expect(analysis.alterStatements).toHaveLength(1); expect(analysis.diagnostics).toEqual(lintDdlSources(sources)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/testkit-core/tests/ddlLint.test.ts` around lines 27 - 30, The test currently only asserts analysis.createStatements length and diagnostics equality; extend it to also assert analysis.alterStatements and analysis.indexStatements (e.g., toHaveLength(expected) and/or toEqual(expected arrays) depending on test data) to fully cover the analyzeDdlSources result and ensure consistency with lintDdlSources; reference the analyzeDdlSources return object (createStatements, alterStatements, indexStatements, diagnostics) and the lintDdlSources(sources) comparison when adding these assertions.packages/ztd-cli/src/commands/ztdConfigCommand.ts (1)
311-320: Minor: Watcher cleanup references wrong handler.The watcher registers
scheduleReloadIfDdlbut cleanup removesscheduleReload. This is pre-existing code (not part of this PR), but the mismatch means listeners aren't properly removed on shutdown.♻️ Suggested fix (outside PR scope)
- watcher.off('add', scheduleReload); - watcher.off('change', scheduleReload); - watcher.off('unlink', scheduleReload); + watcher.off('add', scheduleReloadIfDdl); + watcher.off('change', scheduleReloadIfDdl); + watcher.off('unlink', scheduleReloadIfDdl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/src/commands/ztdConfigCommand.ts` around lines 311 - 320, The watcher registers listeners using scheduleReloadIfDdl but the shutdown cleanup in stop() removes scheduleReload, so listeners are not removed; update the cleanup to call watcher.off('add', scheduleReloadIfDdl), watcher.off('change', scheduleReloadIfDdl), and watcher.off('unlink', scheduleReloadIfDdl) (replace references to scheduleReload with scheduleReloadIfDdl) inside the stop function to match the registered handlers and ensure proper teardown.
🤖 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/ztd-cli/src/utils/ztdProjectConfig.ts`:
- Around line 125-142: The comparison uses finalSerialized before the connection
merge, so changes to connection are ignored; to fix, call
mergeConnectionConfig(baseConfig.connection, overrides.connection) and apply its
result to finalConfig (or delete finalConfig.connection) immediately after
creating finalConfig from mergeProjectConfig (i.e., just after const finalConfig
= mergeProjectConfig(baseConfig, overrides)) and only then compute
baseSerialized and finalSerialized for the existingPath/no-op check; reference
mergeProjectConfig, mergeConnectionConfig, finalConfig, baseConfig, overrides,
resolveZtdConfigPath and writeFileSync to locate the code to reorder.
---
Nitpick comments:
In `@packages/testkit-core/tests/ddlLint.test.ts`:
- Around line 27-30: The test currently only asserts analysis.createStatements
length and diagnostics equality; extend it to also assert
analysis.alterStatements and analysis.indexStatements (e.g.,
toHaveLength(expected) and/or toEqual(expected arrays) depending on test data)
to fully cover the analyzeDdlSources result and ensure consistency with
lintDdlSources; reference the analyzeDdlSources return object (createStatements,
alterStatements, indexStatements, diagnostics) and the lintDdlSources(sources)
comparison when adding these assertions.
In `@packages/ztd-cli/src/commands/ztdConfigCommand.ts`:
- Around line 311-320: The watcher registers listeners using scheduleReloadIfDdl
but the shutdown cleanup in stop() removes scheduleReload, so listeners are not
removed; update the cleanup to call watcher.off('add', scheduleReloadIfDdl),
watcher.off('change', scheduleReloadIfDdl), and watcher.off('unlink',
scheduleReloadIfDdl) (replace references to scheduleReload with
scheduleReloadIfDdl) inside the stop function to match the registered handlers
and ensure proper teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cadef4a-a3a4-4a1f-aa33-e4d8880602e4
📒 Files selected for processing (9)
.changeset/soft-bears-breathe.mdpackages/testkit-core/src/fixtures/ddlLint.tspackages/testkit-core/src/index.tspackages/testkit-core/tests/ddlLint.test.tspackages/ztd-cli/src/commands/ztdConfig.tspackages/ztd-cli/src/commands/ztdConfigCommand.tspackages/ztd-cli/src/utils/ztdProjectConfig.tspackages/ztd-cli/tests/ztdConfigCommand.telemetry.unit.test.tspackages/ztd-cli/tests/ztdProjectConfig.unit.test.ts
Comment on lines
+125
to
+142
| const finalConfig = mergeProjectConfig(baseConfig, overrides); | ||
| const existingPath = resolveZtdConfigPath(rootDir); | ||
| const existingConfigPresent = existsSync(existingPath); | ||
| const baseSerialized = `${JSON.stringify(baseConfig, null, 2)}\n`; | ||
| const finalSerialized = `${JSON.stringify(finalConfig, null, 2)}\n`; | ||
| if (existingConfigPresent && baseSerialized === finalSerialized) { | ||
| return false; | ||
| } | ||
|
|
||
| const resolvedConnection = mergeConnectionConfig(baseConfig.connection, overrides.connection); | ||
| if (resolvedConnection) { | ||
| finalConfig.connection = resolvedConnection; | ||
| } else { | ||
| delete finalConfig.connection; | ||
| } | ||
|
|
||
| const serialized = `${JSON.stringify(finalConfig, null, 2)}\n`; | ||
| writeFileSync(resolveZtdConfigPath(rootDir), serialized, 'utf8'); | ||
| writeFileSync(existingPath, `${JSON.stringify(finalConfig, null, 2)}\n`, 'utf8'); | ||
| return true; |
There was a problem hiding this comment.
Connection config is merged after the no-op comparison, causing incorrect change detection.
The finalConfig used in finalSerialized (line 129) doesn't include the merged connection from lines 134-139. This means:
- If
baseConfig.connectiondiffers fromoverrides.connection, the serialized comparison won't detect the change - The function may return
false(no write) while the actual written content differs frombaseSerialized
Move the connection merge before the serialization/comparison:
🐛 Proposed fix
export function writeZtdProjectConfig(
rootDir: string,
overrides: Partial<ZtdProjectConfig> = {},
baseConfig: ZtdProjectConfig = loadZtdProjectConfig(rootDir)
): boolean {
- const finalConfig = mergeProjectConfig(baseConfig, overrides);
+ let finalConfig = mergeProjectConfig(baseConfig, overrides);
+
+ const resolvedConnection = mergeConnectionConfig(baseConfig.connection, overrides.connection);
+ if (resolvedConnection) {
+ finalConfig = { ...finalConfig, connection: resolvedConnection };
+ } else {
+ const { connection: _, ...rest } = finalConfig;
+ finalConfig = rest as ZtdProjectConfig;
+ }
+
const existingPath = resolveZtdConfigPath(rootDir);
const existingConfigPresent = existsSync(existingPath);
const baseSerialized = `${JSON.stringify(baseConfig, null, 2)}\n`;
const finalSerialized = `${JSON.stringify(finalConfig, null, 2)}\n`;
if (existingConfigPresent && baseSerialized === finalSerialized) {
return false;
}
- const resolvedConnection = mergeConnectionConfig(baseConfig.connection, overrides.connection);
- if (resolvedConnection) {
- finalConfig.connection = resolvedConnection;
- } else {
- delete finalConfig.connection;
- }
-
writeFileSync(existingPath, `${JSON.stringify(finalConfig, null, 2)}\n`, 'utf8');
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ztd-cli/src/utils/ztdProjectConfig.ts` around lines 125 - 142, The
comparison uses finalSerialized before the connection merge, so changes to
connection are ignored; to fix, call
mergeConnectionConfig(baseConfig.connection, overrides.connection) and apply its
result to finalConfig (or delete finalConfig.connection) immediately after
creating finalConfig from mergeProjectConfig (i.e., just after const finalConfig
= mergeProjectConfig(baseConfig, overrides)) and only then compute
baseSerialized and finalSerialized for the existingPath/no-op check; reference
mergeProjectConfig, mergeConnectionConfig, finalConfig, baseConfig, overrides,
resolveZtdConfigPath and writeFileSync to locate the code to reorder.
This was referenced Mar 26, 2026
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
ztd-confignow reuses shared DDL analysis for linting and table metadata generation, and it skips no-op config writes so telemetry matches the actual persistence path.What changed
ztd-config.ztd.config.jsonwrites are skipped when the effective config does not change, and telemetry / JSON output now reflect the actual write result.Why
ztd-confighot path.generate-ztd-configfrom about 175 seconds to about 89 seconds.generate-ztd-config; config writes are not the bottleneck.Verification
pnpm --filter @rawsql-ts/ztd-cli buildpnpm --filter @rawsql-ts/ztd-cli test -- ztdProjectConfig.unit.test.ts ztdConfigCommand.telemetry.unit.test.ts ztdConfig.unit.test.tspnpm --filter @rawsql-ts/testkit-core test -- ddlLint.test.tsgenerate-ztd-configavg89,254.501msgenerate-ztd-configavg174,966.294msNotes
@rawsql-ts/ztd-cliand@rawsql-ts/testkit-core.Summary by CodeRabbit
New Features
Chores
Tests