feat(ztd-cli): align starter directory taxonomy - #777
Merged
Hidden character warning
The head ref may contain hidden characters: "776-bfa-\u30c7\u30a3\u30ec\u30af\u30c8\u30ea\u69cb\u6210"
Conversation
Closed
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 35 seconds. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR restructures the starter scaffold's directory taxonomy from Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CLI as ztd init (packages/ztd-cli/src/commands/init.ts)
participant Templates as Template Bundle (packages/ztd-cli/templates)
participant FS as Project Filesystem
Dev->>CLI: run `ztd init` / scaffold (dry-run or emit)
CLI->>Templates: select template files (libraries, adapters, tests, READMEs)
Templates-->>CLI: provide template content & target mapping
CLI->>FS: emit files to new targets:
Note right of FS: `src/libraries/sql/*`<br/>`src/libraries/telemetry/*`<br/>`src/adapters/pg/*`<br/>`src/adapters/console/*`<br/>`src/features/.../queries/.../tests/...`
FS-->>Dev: scaffolded project with feature-first layout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/ztd-cli/templates/src/adapters/README.md (1)
1-7: LGTM! Clear adapter placement guidance.The documentation properly establishes
src/adapters/<tech>/as the location for technology-specific bindings and explicitly reservesdb/for DDL/migrations/schema assets.Optional: Consider hyphenating compound modifier
-- Put driver or sink specific code under `src/adapters/<tech>/`. +- Put driver-or-sink-specific code under `src/adapters/<tech>/`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/src/adapters/README.md` around lines 1 - 7, Update the compound modifier in the README: change the phrase "Put driver or sink specific code under `src/adapters/<tech>/`" to use correct hyphenation as "Put driver- or sink-specific code under `src/adapters/<tech>/`" so both modifiers are properly attached; edit the string in the README.md content near the top (look for the sentence starting with "Put driver or sink specific code") and replace it with the hyphenated version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guide/ztd-cli-describe-schema.md`:
- Around line 63-64: The docs list of generated files is missing the
`.ztd/generated/ztd-fixture-manifest.generated.ts` entry; update the file list
in the ztd-cli describe-schema documentation (the array including
".ztd/generated/ztd-row-map.generated.ts" and
".ztd/generated/ztd-layout.generated.ts") to include
".ztd/generated/ztd-fixture-manifest.generated.ts" so the documented
`ztd-config` output matches the actual command descriptor.
In `@packages/ztd-cli/README.md`:
- Around line 58-59: Clarify the adapter-boundary rule for
src/adapters/<tech>/*: state that the <tech> path is a boundary when it names a
single technology (e.g., src/adapters/pg/ is the adapter boundary), but when the
technology token is plural or represents a family (e.g., aws, cloud), treat
<tech> as a container for child boundaries and require explicit child folders
(e.g., src/adapters/aws/<child>/ where each <child> is a concrete adapter like
s3 or lambda); update the README sentence to explicitly permit both patterns and
give the two canonical examples (src/adapters/pg/ and src/adapters/aws/<child>/)
so consumers know when to create direct adapters versus nested adapter folders.
In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 2778-2782: The dry-run plan is out of sync with the actual writes
in runInitCommand(): ensure the sql README and adapters README that appear in
the plan are actually emitted (or remove them from the plan). Specifically, in
runInitCommand() fix the handling of sqlReadme so it is both registered in
absolutePaths and written via the same write/template flow used for other files
(or drop it from absolutePaths if not desired), add the adapters README
template/path/write step (e.g., create and write 'src/adapters/README.md' using
the adapters README template) and remove the duplicate 'src/libraries/README.md'
entry added in starter mode so absolutePaths/file template lists exactly mirror
the files written.
In `@packages/ztd-cli/templates/src/adapters/pg/sql-client.ts`:
- Around line 16-34: The PR added a new adapter function fromPg with runtime
behavior (unwrapping result.rows and throwing when values is a named object) but
no tests; add behavioral tests that call fromPg.query to verify (1) it returns
the rows array extracted from the underlying queryable (i.e., that
queryable.query resolves to { rows: [...] } and fromPg.query returns that inner
array) and (2) it throws the specific Error when passed a non-array values
object (named parameters). Target tests to exercise the exported fromPg function
and its query method (use a mock/stub queryable implementing query(text,
values?) => Promise<{ rows: [...] }>) and assert both successful row unwrapping
and the named-parameter rejection.
In `@packages/ztd-cli/tests/directoryFinding.docs.test.ts`:
- Around line 125-131: The test's expected scaffold bundle list in
directoryFinding.docs.test.ts is missing the new README for the sql library;
update the array of expected paths in the test to include
'packages/ztd-cli/templates/src/libraries/sql/README.md' so it matches what
buildInitDryRunPlan() advertises, then re-run and update any
snapshots/assertions that depend on that list.
---
Nitpick comments:
In `@packages/ztd-cli/templates/src/adapters/README.md`:
- Around line 1-7: Update the compound modifier in the README: change the phrase
"Put driver or sink specific code under `src/adapters/<tech>/`" to use correct
hyphenation as "Put driver- or sink-specific code under `src/adapters/<tech>/`"
so both modifiers are properly attached; edit the string in the README.md
content near the top (look for the sentence starting with "Put driver or sink
specific code") and replace it with the hyphenated version.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50d6b091-5303-46b2-a0a1-46e61135df91
📒 Files selected for processing (34)
.changeset/green-apples-drum.mdREADME.mddocs/guide/feature-index.mddocs/guide/multiple-db-clients-in-one-workflow.mddocs/guide/repository-telemetry-setup.mddocs/guide/sql-first-end-to-end-tutorial.mddocs/guide/ztd-cli-describe-schema.mddocs/recipes/sql-contract.mdpackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/templates/AGENTS.mdpackages/ztd-cli/templates/PROMPT_DOGFOOD.mdpackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/src/adapters/README.mdpackages/ztd-cli/templates/src/adapters/console/repositoryTelemetry.tspackages/ztd-cli/templates/src/adapters/pg/sql-client.tspackages/ztd-cli/templates/src/features/AGENTS.mdpackages/ztd-cli/templates/src/features/README.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/telemetry/AGENTS.mdpackages/ztd-cli/templates/src/libraries/README.mdpackages/ztd-cli/templates/src/libraries/sql/README.mdpackages/ztd-cli/templates/src/libraries/sql/sql-client.tspackages/ztd-cli/templates/src/libraries/telemetry/repositoryTelemetry.tspackages/ztd-cli/templates/src/libraries/telemetry/types.tspackages/ztd-cli/templates/src/repositories/AGENTS.mdpackages/ztd-cli/templates/tests/AGENTS.mdpackages/ztd-cli/templates/tests/support/testkit-client.webapi.tspackages/ztd-cli/templates/tests/support/ztd/README.mdpackages/ztd-cli/templates/tests/support/ztd/harness.tspackages/ztd-cli/templates/tests/support/ztd/verifier.tspackages/ztd-cli/tests/directoryFinding.docs.test.tspackages/ztd-cli/tests/init.command.test.tspackages/ztd-cli/tests/sqlFirstTutorial.docs.test.ts
Comment on lines
+16
to
+34
| export function fromPg( | ||
| queryable: { | ||
| query(text: string, values?: readonly unknown[]): Promise<{ rows: Record<string, unknown>[] }>; | ||
| } | ||
| ): SqlClient { | ||
| return { | ||
| query<T extends Record<string, unknown> = Record<string, unknown>>( | ||
| text: string, | ||
| values?: readonly unknown[] | Record<string, unknown> | ||
| ): Promise<T[]> { | ||
| if (values != null && !Array.isArray(values)) { | ||
| throw new Error('fromPg adapter does not support named parameter objects; use positional parameter arrays'); | ||
| } | ||
| return queryable | ||
| .query(text, values as readonly unknown[]) | ||
| .then((result) => result.rows as T[]); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add behavioral coverage for the new fromPg adapter path.
This introduces new runtime behavior (row unwrapping and named-parameter rejection), but no direct behavior test is included in the provided change set for this adapter surface.
As per coding guidelines: "Unless the request explicitly says not to, behavior changes must add or update tests in the same change".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ztd-cli/templates/src/adapters/pg/sql-client.ts` around lines 16 -
34, The PR added a new adapter function fromPg with runtime behavior (unwrapping
result.rows and throwing when values is a named object) but no tests; add
behavioral tests that call fromPg.query to verify (1) it returns the rows array
extracted from the underlying queryable (i.e., that queryable.query resolves to
{ rows: [...] } and fromPg.query returns that inner array) and (2) it throws the
specific Error when passed a non-array values object (named parameters). Target
tests to exercise the exported fromPg function and its query method (use a
mock/stub queryable implementing query(text, values?) => Promise<{ rows: [...]
}>) and assert both successful row unwrapping and the named-parameter rejection.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ztd-cli/src/commands/init.ts (1)
2796-2812:⚠️ Potential issue | 🟠 Major
--dry-runstill over-reports files for non-starter initLine 2808-Line 2812 are always included in the dry-run plan, but those files are only written in starter mode (Line 1225-Line 1262). This keeps dry-run output inaccurate for non-starter workflows.
🔧 Suggested fix (keep dry-run parity with actual writes)
const files = [ 'ztd.config.json', path.join(DEFAULT_ZTD_CONFIG.ddlDir, schemaFileName), path.join('src', 'features', 'README.md'), path.join(resolveSupportDir(DEFAULT_ZTD_CONFIG), 'global-setup.ts'), path.join(resolveSupportDir(DEFAULT_ZTD_CONFIG), 'setup-env.ts'), path.join(resolveGeneratedDir(DEFAULT_ZTD_CONFIG), 'ztd-row-map.generated.ts'), path.join(resolveGeneratedDir(DEFAULT_ZTD_CONFIG), 'ztd-layout.generated.ts'), path.join(resolveGeneratedDir(DEFAULT_ZTD_CONFIG), 'ztd-fixture-manifest.generated.ts'), 'README.md', '.env.example', '.gitignore', - 'src/libraries/README.md', - 'src/libraries/sql/README.md', 'src/libraries/sql/sql-client.ts', - 'src/adapters/README.md', 'src/adapters/pg/sql-client.ts', 'vitest.config.ts', 'tsconfig.json' ]; if (starter) { files.push( + path.join('src', 'libraries', 'README.md'), + path.join('src', 'libraries', 'sql', 'README.md'), + path.join('src', 'adapters', 'README.md'), STARTER_COMPOSE_FILE, path.join('src', 'features', '_shared', 'featureQueryExecutor.ts'), path.join('src', 'features', '_shared', 'loadSqlResource.ts'), path.join('src', 'features', 'smoke', 'README.md'), path.join('src', 'features', 'smoke', 'tests', 'README.md'), path.join('src', 'features', 'smoke', 'boundary.ts'), path.join('src', 'features', 'smoke', 'tests', 'smoke.boundary.test.ts'), path.join('src', 'features', 'smoke', 'tests', 'smoke.validation.test.ts'), path.join('src', 'features', 'smoke', 'tests', 'smoke.test.ts'), path.join('src', 'features', 'smoke', 'queries', 'smoke', 'tests', 'smoke.boundary.ztd.test.ts'), path.join('src', 'features', 'smoke', 'queries', 'smoke', 'boundary.ts'), path.join('src', 'features', 'smoke', 'queries', 'smoke', 'smoke.sql'), path.join('src', 'features', 'smoke', 'queries', 'smoke', 'tests', 'boundary-ztd-types.ts'), path.join('src', 'features', 'smoke', 'queries', 'smoke', 'tests', 'cases', 'basic.case.ts'), path.join('src', 'features', 'smoke', 'queries', 'smoke', 'tests', 'generated', 'analysis.json'), path.join('src', 'features', 'smoke', 'queries', 'smoke', 'tests', 'generated', 'TEST_PLAN.md'), path.join('tests', 'support', 'ztd', 'README.md'), path.join('tests', 'support', 'ztd', 'case-types.ts'), path.join('tests', 'support', 'ztd', 'verifier.ts'), path.join('tests', 'support', 'ztd', 'harness.ts'), path.join('src', 'libraries', 'telemetry', 'types.ts'), path.join('src', 'libraries', 'telemetry', 'repositoryTelemetry.ts'), path.join('src', 'adapters', 'console', 'repositoryTelemetry.ts'), path.join(resolveSupportDir(DEFAULT_ZTD_CONFIG), 'postgres-testkit.ts') ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/src/commands/init.ts` around lines 2796 - 2812, The dry-run currently lists starter-only files because the files array unconditionally includes entries (e.g., 'src/libraries/README.md', 'src/libraries/sql/README.md', 'src/libraries/sql/sql-client.ts', 'src/adapters/README.md', 'src/adapters/pg/sql-client.ts'); update the code that builds that files list in the init command to only push those starter-specific entries when starter mode is enabled (the same condition that triggers the actual starter writes in the initStarter()/starter branch), so the --dry-run output matches what will actually be written; reference the same starter flag/variable used around initStarter() and gate the inclusion of those file paths accordingly.
🧹 Nitpick comments (1)
packages/ztd-cli/README.md (1)
58-60: Split the ownership rule into bullets for scanability.This paragraph is accurate, but it packs too many rules into two long sentences. Converting it to a short bullet list will make the taxonomy easier to apply correctly.
As per coding guidelines "Keep human-facing docs scannable: prefer short headings, short paragraphs, short sentences, and strong structure".Proposed doc-only refactor
-Outside feature-owned boundaries, keep shared feature seams under `src/features/_shared/*`, driver-neutral contracts under `src/libraries/*`, driver- or sink-specific bindings under `src/adapters/<tech>/*`, shared verification seams under `tests/support/*`, and tool-managed assets under `.ztd/*`. -Treat `src/adapters/pg/` as the adapter boundary when `<tech>` names one concrete technology. If `<tech>` names a family or plural container such as `aws` or `cloud`, treat `src/adapters/<tech>/` as a parent container and put each concrete adapter in its own child boundary such as `src/adapters/aws/s3/` or `src/adapters/aws/lambda/`. -Reserve `db/` for DDL, migration, and schema assets; do not place runtime clients or adapters there. +Outside feature-owned boundaries: +- Keep shared feature seams under `src/features/_shared/*`. +- Keep driver-neutral contracts under `src/libraries/*`. +- Keep driver- or sink-specific bindings under `src/adapters/<tech>/*`. +- Keep shared verification seams under `tests/support/*`. +- Keep tool-managed assets under `.ztd/*`. + +Adapter boundary rule: +- If `<tech>` is one concrete technology, treat `src/adapters/<tech>/` as the adapter boundary (for example, `src/adapters/pg/`). +- If `<tech>` is a family/plural container (for example, `aws`, `cloud`), treat `src/adapters/<tech>/` as a parent and create child boundaries like `src/adapters/aws/s3/` and `src/adapters/aws/lambda/`. + +Reserve `db/` for DDL, migration, and schema assets only; do not place runtime clients or adapters there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/README.md` around lines 58 - 60, Split the long paragraph into a short bulleted list for scanability: create separate bullets for (1) shared feature seams under src/features/_shared/*, (2) driver-neutral contracts under src/libraries/*, (3) driver- or sink-specific bindings under src/adapters/<tech>/*, (4) shared verification seams under tests/support/*, (5) tool-managed assets under .ztd/*, (6) adapter-boundary rule treating src/adapters/pg/ as a concrete adapter vs. src/adapters/<tech>/ as a parent for families (with examples like src/adapters/aws/s3/ and src/adapters/aws/lambda/), and (7) reserving db/ for DDL/migrations/schema only (no runtime clients/adapters); keep each bullet short, one sentence, and retain the original path examples verbatim.
🤖 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/README.md`:
- Line 201: Update the README table row for the ztd feature tests scaffold
command to also mention that the command refreshes boundary type definitions by
updating tests/boundary-ztd-types.ts; specifically, modify the description for
`ztd feature tests scaffold --feature <feature-name>` to include "refresh
tests/boundary-ztd-types.ts" alongside the existing notes about regenerating
TEST_PLAN.md, analysis.json, the Vitest entrypoint, and preserving cases/.
---
Duplicate comments:
In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 2796-2812: The dry-run currently lists starter-only files because
the files array unconditionally includes entries (e.g.,
'src/libraries/README.md', 'src/libraries/sql/README.md',
'src/libraries/sql/sql-client.ts', 'src/adapters/README.md',
'src/adapters/pg/sql-client.ts'); update the code that builds that files list in
the init command to only push those starter-specific entries when starter mode
is enabled (the same condition that triggers the actual starter writes in the
initStarter()/starter branch), so the --dry-run output matches what will
actually be written; reference the same starter flag/variable used around
initStarter() and gate the inclusion of those file paths accordingly.
---
Nitpick comments:
In `@packages/ztd-cli/README.md`:
- Around line 58-60: Split the long paragraph into a short bulleted list for
scanability: create separate bullets for (1) shared feature seams under
src/features/_shared/*, (2) driver-neutral contracts under src/libraries/*, (3)
driver- or sink-specific bindings under src/adapters/<tech>/*, (4) shared
verification seams under tests/support/*, (5) tool-managed assets under .ztd/*,
(6) adapter-boundary rule treating src/adapters/pg/ as a concrete adapter vs.
src/adapters/<tech>/ as a parent for families (with examples like
src/adapters/aws/s3/ and src/adapters/aws/lambda/), and (7) reserving db/ for
DDL/migrations/schema only (no runtime clients/adapters); keep each bullet
short, one sentence, and retain the original path examples verbatim.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb602a14-c18a-49a0-bb8f-84e4dd03f4e3
📒 Files selected for processing (7)
docs/guide/ztd-cli-describe-schema.mdpackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/templates/src/adapters/README.mdpackages/ztd-cli/tests/directoryFinding.docs.test.tspackages/ztd-cli/tests/init.command.test.tspackages/ztd-cli/tests/sqlClientAdapterTemplate.unit.test.ts
✅ Files skipped from review due to trivial changes (3)
- docs/guide/ztd-cli-describe-schema.md
- packages/ztd-cli/templates/src/adapters/README.md
- packages/ztd-cli/tests/directoryFinding.docs.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ztd-cli/tests/init.command.test.ts
6 tasks
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
src/features,src/adapters, andsrc/librariesas the app-code roots; keepdb/for schema assets,tests/support/*for shared verification support, and.ztd/*for tool-managed workspace files.SqlClientand telemetry seams into the newlibraries/adapterslayout and update template guidance, README text, and tutorials to describe the same ownership model.@rawsql-ts/ztd-clias a minor release.Verification
pnpm --filter @rawsql-ts/ztd-cli test -- directoryFinding.docs.test.ts sqlFirstTutorial.docs.test.ts init.command.test.tsMerge Readiness
Tracking issue: #776
Scoped checks run:
pnpm --filter @rawsql-ts/ztd-cli test -- directoryFinding.docs.test.ts sqlFirstTutorial.docs.test.ts init.command.test.tsWhy full baseline is not required: the change scope is limited to
@rawsql-ts/ztd-cliscaffold/templates/docs, and the targeted scaffold/docs contract tests for those surfaces passed.CLI Surface Migration
No-migration rationale:
Upgrade note: newly scaffolded projects now place shared runtime contracts under
src/libraries/*, technology bindings undersrc/adapters/<tech>/*, shared verification support undertests/support/*, and keepdb/*reserved for DDL/migration/schema assets.Deprecation/removal plan or issue: none beyond #776.
Docs/help/examples updated: yes; updated
README.md,packages/ztd-cli/README.md, template README/AGENTS files, and the directory/telemetry/tutorial guides.Release/changeset wording:
.changeset/green-apples-drum.mdreleases@rawsql-ts/ztd-clias minor for the canonical directory taxonomy alignment.Scaffold Contract Proof
No-proof rationale:
Non-edit assertion: the starter guidance now consistently reserves
db/*for schema assets,tests/support/*for shared verification support,.ztd/*for tool-managed workspace files, and keeps feature-owned runtime seams undersrc/features/*,src/libraries/*, andsrc/adapters/*.Fail-fast input-contract proof:
init.command.test.tsverifies the emitted starter structure and guidance for the scaffolded project layout.Generated-output viability proof:
init.command.test.ts,directoryFinding.docs.test.ts, andsqlFirstTutorial.docs.test.tspassed with the updated paths and examples.Closes #776
Summary by CodeRabbit