refactor(repo): enforce file structure conventions and upgrade dependencies#39
Merged
zrosenbauer merged 8 commits intomainfrom Mar 17, 2026
Merged
refactor(repo): enforce file structure conventions and upgrade dependencies#39zrosenbauer merged 8 commits intomainfrom
zrosenbauer merged 8 commits intomainfrom
Conversation
…encies - Apply file structure pattern across all packages: exports at top, private helpers below a `// --- Private ---` separator - Add full JSDoc (with @param, @returns) on all functions; @Private tag on non-exported helpers - Colocate test files alongside source files - Update .claude/rules/typescript.md with file structure conventions - Upgrade all dependencies to latest versions Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: ff18dd9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMonorepo-wide file-structure refactor (exports-first, private helpers section, JSDoc enforcement), moves tests alongside sources and updates Vitest/tsconfigs, bumps dependencies, adds CLI commands (setup, dump) and build-check APIs, converts templates to lazy built-ins and changes registry API, and performs broad UI/OpenAPI and sidebar reorganizations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Co-Authored-By: Claude <noreply@anthropic.com>
…grade - Exclude **/*.test.ts from tsconfig.json in all packages to prevent test files from being included in declaration generation - Fix stale import paths in colocated test files (../../src/ → ./) - Migrate command definitions from `args` to `options` for @kidd-cli/core 0.7.0 - Replace removed ctx.output.raw with process.stdout.write in dump command Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
packages/core/src/sync/workspace.ts (1)
195-211:⚠️ Potential issue | 🟠 MajorPrevent invalid or colliding synthesized category links.
At Line 196, generated links are only checked against
existingLinks, so collisions between newly synthesized categories can still slip through. Also, ifslugify(...)returns an empty string, this creates/, which can conflict with root routing.💡 Suggested fix
export function synthesizeWorkspaceSections(config: ZpressConfig): Section[] { const existingLinks = collectAllLinks(config.sections) + const usedLinks = new Set(existingLinks) @@ - const categoryEntries = categories.map((category): Section | null => { - const link = category.link ?? `/${slugify(String(category.title))}` - if (existingLinks.has(link)) { + const categoryEntries = categories.map((category): Section | null => { + const generated = slugify(String(category.title)) + const link = category.link ?? (generated.length > 0 ? `/${generated}` : null) + if (link === null || usedLinks.has(link)) { return null } + usedLinks.add(link) return { title: category.title, link,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/workspace.ts` around lines 195 - 211, The synthesized category link generation in the categories.map block (using category.link, slugify(String(category.title)), existingLinks, and workspaceToSection) only checks collisions against existingLinks and doesn't handle empty slugs, so multiple new categories can collide or produce "/" for an empty slug; fix by computing a safeSlug (fallback to a non-empty value derived from title or an index when slugify returns empty), maintain a local Set (e.g., newLinks) while iterating categories to ensure each generated link is unique (on collision append a deterministic suffix like `-1`, `-2` or use the category index) and check against both existingLinks and newLinks before accepting a link, and only include the category when a unique non-root link has been produced.packages/core/src/sync/openapi.ts (1)
127-132: 🛠️ Refactor suggestion | 🟠 MajorConvert helper signatures with 2+ params to object params.
Helpers on Line 132, Line 280, Line 317, and Line 350 still use positional parameters with 2+ args. This conflicts with the repository TS function-signature rule.
As per coding guidelines: "Use object parameters for functions with 2+ parameters and include explicit return types".
♻️ Example refactor pattern (apply similarly to other helpers)
-function buildSidebarItems( - title: string, - prefix: string, - tagGroups: readonly TagGroup[], - sidebarLayout: 'method-path' | 'title' -): readonly SidebarItem[] { +function buildSidebarItems({ + title, + prefix, + tagGroups, + sidebarLayout, +}: { + readonly title: string + readonly prefix: string + readonly tagGroups: readonly TagGroup[] + readonly sidebarLayout: 'method-path' | 'title' +}): readonly SidebarItem[] { const tagItems: readonly SidebarItem[] = tagGroups.map((group) => ({ text: capitalize(group.tag), @@ - const sidebarItems = buildSidebarItems(title, prefix, tagGroups, sidebarLayout) + const sidebarItems = buildSidebarItems({ title, prefix, tagGroups, sidebarLayout })Also applies to: 275-280, 312-317, 350-355, 388-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/openapi.ts` around lines 127 - 132, Convert any helper functions used by syncOpenAPI that currently accept two or more positional parameters into single object-parameter signatures and add explicit return types; update all their call sites inside syncOpenAPI accordingly. Specifically, find the helper functions declared/used within the syncOpenAPI implementation (the helpers referenced in the review around the later parts of the file) and change signatures from fn(a: A, b: B, ...) to fn({ a, b }: { a: A; b: B; ... }): ReturnType, then adjust every invocation to pass an object (e.g., helper({ a, b })). Ensure TypeScript types (OpenAPIConfig, SyncContext, SingleSyncResult) are preserved and exported types updated if needed.packages/ui/src/head/read.ts (1)
73-92:⚠️ Potential issue | 🟠 MajorAvoid reporting all read failures as "missing asset".
readAssetcatches every exception and labels it asmissing, which can mask permission/path/IO failures and slow down debugging.🛠️ Proposed fix
interface AssetError { readonly _tag: 'AssetError' - readonly type: 'missing' + readonly type: 'missing' | 'read-failed' readonly message: string readonly path: string } @@ -function readAsset(relativePath: string): AssetResult { +function readAsset(relativePath: string): AssetResult { const fullPath = resolveAsset(relativePath) try { @@ - } catch { + } catch (cause) { + const nodeError = cause as NodeJS.ErrnoException + const isMissing = nodeError?.code === 'ENOENT' const error: AssetError = { _tag: 'AssetError', - type: 'missing', - message: `Missing head asset: ${relativePath} — run "pnpm build" in packages/ui first`, + type: isMissing ? 'missing' : 'read-failed', + message: isMissing + ? `Missing head asset: ${relativePath} — run "pnpm build" in packages/ui first` + : `Failed to read head asset: ${relativePath} (${nodeError?.message ?? 'unknown error'})`, path: fullPath, } return [error, null] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/head/read.ts` around lines 73 - 92, The readAsset function currently treats every exception as a missing asset; modify readAsset to catch the thrown error (capture it as e) after calling resolveAsset/readFileSync, detect whether it's actually a missing-file error (e.g., e.code === 'ENOENT') and set AssetError.type = 'missing' only in that case, otherwise set a more appropriate type like 'io' or 'permission' and include the original error details (e.message and e.code) in the AssetError.message and/or path fields so permission/path/IO errors are surfaced instead of being masked; keep the returned tuple shape [AssetError|null, content|null] unchanged and reference readAsset, resolveAsset, and readFileSync locations when applying the change.packages/core/src/sync/planning.ts (1)
126-133:⚠️ Potential issue | 🟡 MinorUse object parameters for 2-argument helpers.
generatePlanningIndexandnaturalComparestill take positional args; this violates the TypeScript guideline for functions with 2+ parameters.♻️ Suggested refactor
- const indexPage: PageData = { - content: () => generatePlanningIndex(files, planningDir), + const indexPage: PageData = { + content: () => generatePlanningIndex({ files, planningDir }), outputPath: linkToOutputPath(PLANNING_PREFIX), frontmatter: PLANNING_FRONTMATTER, } +interface GeneratePlanningIndexParams { + readonly files: readonly string[] + readonly planningDir: string +} + async function generatePlanningIndex( - files: readonly string[], - planningDir: string + { files, planningDir }: GeneratePlanningIndexParams ): Promise<string> { @@ - .toSorted((a, b) => naturalCompare(a.slug, b.slug)) + .toSorted((a, b) => naturalCompare({ a: a.slug, b: b.slug })) @@ - .toSorted(([a], [b]) => naturalCompare(a, b)) + .toSorted(([a], [b]) => naturalCompare({ a, b })) @@ - .toSorted((a, b) => naturalCompare(a.slug, b.slug)), + .toSorted((a, b) => naturalCompare({ a: a.slug, b: b.slug })), @@ -function naturalCompare(a: string, b: string): number { +function naturalCompare({ a, b }: { readonly a: string; readonly b: string }): number {As per coding guidelines
**/*.{ts,tsx}: “Use object parameters for functions with 2+ parameters and include explicit return types”.Also applies to: 188-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/planning.ts` around lines 126 - 133, The functions generatePlanningIndex and naturalCompare currently use positional parameters; change both to accept a single object parameter each (e.g., generatePlanningIndex({ files, planningDir }): Promise<string> and naturalCompare({ a, b }: { a: string; b: string }): number) with explicit return types, update all internal references to use the destructured names, and update every call site to pass an object instead of positional args; ensure any exports/types/tests are updated accordingly and that the functions' behavior and return types remain the same.packages/core/src/sync/manifest.ts (1)
58-60:⚠️ Potential issue | 🔴 CriticalFix unsafe path boundary check in
pruneEmptyDirsThe current boundary check using
dir.startsWith(stopAt)is vulnerable to prefix collisions (e.g.,/home/outvs/home/outside) and does not prevent escape sequences from..in manifest paths. This allows the recursive pruning to delete files outside the intendedoutDir, risking data loss.Replace with
path.relative()validation to safely detect boundary violations:Suggested fix
async function pruneEmptyDirs(dir: string, stopAt: string): Promise<void> { - if (dir === stopAt || !dir.startsWith(stopAt)) { + const rel = path.relative(stopAt, dir) + if (rel === '' || rel.startsWith('..') || path.isAbsolute(rel)) { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/manifest.ts` around lines 58 - 60, The recursive removal in pruneEmptyDirs is using dir.startsWith(stopAt), which is unsafe; update pruneEmptyDirs to resolve and normalize both stopAt and dir (using path.resolve) and then compute path.relative(stopAtResolved, dirResolved) and only allow recursion/deletion when the relative path does not start with '..' and is not an absolute path (e.g., relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative))). Also ensure callers (e.g., where abs = path.resolve(outDir, oldPath) and pruneEmptyDirs(path.dirname(abs), outDir) is invoked) pass resolved/normalized outDir to pruneEmptyDirs so that boundary checks are correct; if the check fails, bail out without deleting and surface an error or skip pruning.packages/ui/src/config.ts (1)
159-159:⚠️ Potential issue | 🟠 MajorHandle malformed generated JSON gracefully.
JSON.parsecan throw and fail config generation if a generated file is corrupt. Use a guarded parse and returnfallbackwith a warning.💡 Suggested fix
- return JSON.parse(readFileSync(p, 'utf8')) as T + try { + return JSON.parse(readFileSync(p, 'utf8')) as T + } catch { + process.stderr.write(`[zpress] Invalid generated JSON: ${name} — run "zpress sync" first\n`) + return fallback + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/config.ts` at line 159, The direct JSON.parse(readFileSync(p, 'utf8')) can throw on malformed files; wrap the file read + parse in a try/catch inside the function that returns a T (the code using readFileSync and JSON.parse, referencing variables p and fallback), and on parse error log a warning (e.g., console.warn or the project logger) including the file path and error, then return fallback as a safe fallback value; ensure the returned value still has the T type and do not rethrow so config generation continues.packages/ui/src/theme/components/openapi/spec-utils.ts (1)
81-111:⚠️ Potential issue | 🟠 MajorRead from
examplesas well asexample.
extractBodyExample()only checksmediaType.example. Specs that put samples under anexamplesmap or on the schema fall back tonull, so the request-body preview and generated code snippets drop valid payloads.🔧 Minimal fix
- const { example } = mediaType as { readonly example?: unknown } - return match(example) - .with(P.nonNullable, (ex) => ex) - .otherwise(() => null) + const typedMediaType = mediaType as { + readonly example?: unknown + readonly examples?: Record<string, { readonly value?: unknown }> + readonly schema?: Record<string, unknown> + } + const firstNamedExample = Object.values(typedMediaType.examples ?? {})[0]?.value + return typedMediaType.example ?? firstNamedExample ?? typedMediaType.schema?.example ?? null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/spec-utils.ts` around lines 81 - 111, extractBodyExample currently only reads mediaType.example and returns null if that's absent; update it to also check mediaType.examples (take the first example value from the examples map) and then fallback to mediaType.schema?.example (or schema examples if present) before returning null. In the function extractBodyExample, when you destructure mediaType, look for an examples property (iterate Object.values or Object.entries and extract the example payload from the first Example Object) and if none found check mediaType.schema?.example (or schema.examples) and return that value if present; otherwise keep returning null.packages/ui/src/theme/components/openapi/response-list.tsx (1)
55-78:⚠️ Potential issue | 🟡 MinorPrefer the first media type that actually has a schema.
extractSchema()returns the firstcontententry’sschemaornull. If the first media type omits a schema but a later one includes it, the UI incorrectly renders “No response body”.🔧 Minimal fix
- const entries = Object.entries(c) - return match(entries) - .with( - P.when((e): e is [string, Record<string, unknown>][] => e.length > 0), - (e) => { - const [[, mediaType]] = e - return (mediaType['schema'] ?? null) as Record<string, unknown> | null - } - ) - .otherwise(() => null) + return ( + Object.values(c).find((mediaType) => mediaType['schema'] !== undefined)?.['schema'] ?? + null + ) as Record<string, unknown> | null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/response-list.tsx` around lines 55 - 78, The extractSchema function currently takes the very first content entry's schema which can be missing; change extractSchema to iterate the content entries (e.g., via Object.values/Object.entries on the content object inside extractSchema) and return the first non-null mediaType['schema'] found (casting to Record<string, unknown> | null), returning null only if none of the media types contain a schema; update the match logic around content (and the inner entries handling) to check each mediaType rather than assuming the first entry has a schema.packages/ui/src/theme/components/openapi/schema-viewer.tsx (1)
201-229:⚠️ Potential issue | 🟡 MinorExpose the expand/collapse state on the custom schema toggle.
The button changes visibility but never reports its state. Add
aria-expanded={expanded}and, ideally, anaria-controlslink to the indented content so screen-reader users can tell whether the branch is open.♿ Minimal fix
- <button type="button" className="zp-oas-schema__trigger" onClick={toggle}> + <button + type="button" + className="zp-oas-schema__trigger" + aria-expanded={expanded} + onClick={toggle} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/schema-viewer.tsx` around lines 201 - 229, The schema toggle button currently changes the UI but doesn't expose its state to assistive tech; update the button in the SchemaViewer component to include aria-expanded={expanded} and aria-controls pointing to the indented content, and give the indented content a stable id (e.g., compute contentId using the component's unique symbols like name and depth such as const contentId = `zp-oas-schema-${name}-${depth}`) then set id={contentId} on the <div className="zp-oas-schema__indent"> and aria-controls={contentId} on the <button> so screen readers can know which region is toggled; keep using the existing expanded and toggle variables and ensure aria-expanded receives the boolean expanded value.packages/ui/src/theme/components/openapi/code-sample.tsx (1)
134-157:⚠️ Potential issue | 🟠 MajorFix Python code generation for payloads with boolean and null values.
The current implementation uses
JSON.stringify()to inject payloads directly into generated Python code. JSON literalstrue,false, andnullare not valid Python identifiers, causing the generated examples to fail immediately upon execution.Use
json.loads()to parse the JSON string into a Python dictionary at runtime:Recommended fix
const payloadLines = match(hasBody) - .with(true, () => [`payload = ${JSON.stringify(bodyExample, null, 4)}`, '']) + .with(true, () => [`payload = json.loads(${JSON.stringify(JSON.stringify(bodyExample))})`, '']) .otherwise(() => []) return [ + 'import json', 'import requests',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/code-sample.tsx` around lines 134 - 157, The generated Python snippet in generatePython incorrectly inlines JSON via JSON.stringify (payloadLines and jsonArg) which produces JS-style literals (true/false/null) invalid in Python; change generation to import json, build payload as payload = json.loads('<escaped JSON string>') (use JSON.stringify on the JS side to create the string and ensure it's properly escaped for inclusion in single/double quotes), remove inlined Python literal output, and keep json argument as ',\n json=payload' so requests.<method> uses the runtime-parsed payload; update generatePython (and related payloadLines/jsonArg construction) to produce these lines and ensure buildUrl/extractBodyExample/isBodyMethod usage remains the same.packages/ui/src/theme/components/openapi/overview.tsx (1)
232-234:⚠️ Potential issue | 🟡 MinorFix singular/plural operation label.
Line 233 always renders
"operations", which produces"1 operations"for single-entry tags.💡 Proposed fix
- <span className="zp-oas-tag-group__count"> - {`${String(tag.operationCount)} operations`} - </span> + <span className="zp-oas-tag-group__count"> + {`${String(tag.operationCount)} ${ + tag.operationCount === 1 ? 'operation' : 'operations' + }`} + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/overview.tsx` around lines 232 - 234, The tag count label always uses "operations" causing "1 operations" for single-entry tags; update the render in the component that outputs the element with className "zp-oas-tag-group__count" (where tag.operationCount is used) to pluralize correctly by emitting "operation" when tag.operationCount === 1 and "operations" otherwise (e.g., build the string from tag.operationCount plus a conditional suffix) so the label reads "1 operation" or "N operations" appropriately.packages/ui/src/theme/components/openapi/markdown.ts (1)
326-333:⚠️ Potential issue | 🟠 MajorUse request-body content type instead of hardcoded JSON in cURL output.
Line 327 always emits
Content-Type: application/jsonfor body methods, which can produce incorrect examples for specs using other media types (e.g., form-data, XML).🛠️ Proposed fix
const normalizedMethod = method.toLowerCase() const url = `${baseUrl}${urlPath}` const upper = normalizedMethod.toUpperCase() + const content = requestBody?.['content'] as Record<string, unknown> | undefined + const contentType = Object.keys(content ?? {})[0] const headerPart = match(isBodyMethod(normalizedMethod)) - .with(true, () => " \\\n -H 'Content-Type: application/json'") + .with( + true, + () => (contentType !== undefined ? ` \\\n -H ${shellQuote(`Content-Type: ${contentType}`)}` : '') + ) .otherwise(() => '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/markdown.ts` around lines 326 - 333, The cURL builder currently hardcodes "Content-Type: application/json" in headerPart and always JSON.stringifies the example in bodyPart; instead, read the request media type from requestBody.content (e.g., pick the first key) and use that media type for the header (replace the hardcoded string in headerPart), and conditionally serialize the bodyExample in bodyPart based on that media type: if the media type is JSON-like (contains "json") stringify and shellQuote as before, if it's form-data build -F parts or for other raw media types use the raw example string (shellQuote as appropriate). Update the logic around isBodyMethod(normalizedMethod), requestBody, extractBodyExample, headerPart and bodyPart to use the derived mediaType and handle form-data vs JSON vs raw payloads accordingly.packages/config/src/errors.ts (1)
43-49: 🛠️ Refactor suggestion | 🟠 MajorUse an object parameter for
configErrorto match project TS conventions.This exported function currently takes two positional args, which violates the repository rule for 2+ parameters. Refactoring here is the root fix; downstream callsites (e.g., in
packages/config/src/loader.ts, Lines 77, 82, 90) should be updated accordingly.♻️ Proposed refactor
-export function configError(type: ConfigErrorType, message: string): ConfigError { +export function configError(params: { + readonly type: ConfigErrorType + readonly message: string +}): ConfigError { return { _tag: 'ConfigError', - type, - message, + type: params.type, + message: params.message, } }As per coding guidelines:
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters and include explicit return types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/errors.ts` around lines 43 - 49, The function configError currently accepts two positional parameters (type: ConfigErrorType, message: string); change its signature to accept a single object parameter (e.g., { type, message }: { type: ConfigErrorType; message: string }) while preserving the explicit return type ConfigError, and update all downstream callsites that invoke configError with two args (notably the usages in loader.ts) to pass a single object argument with matching property names; keep the returned shape (_tag, type, message) unchanged.packages/core/src/sync/sidebar/landing.test.ts (2)
5-9:⚠️ Potential issue | 🔴 CriticalUse the string-path
vi.mockoverload.The dynamic
import(...)syntax requires anasyncfactory withimportOriginalparameter in Vitest 4.1.0. The current synchronous factory causes a TS2769 type mismatch that blocks the build duringpnpm typecheck.Proposed fix
-vi.mock(import('node:fs/promises'), () => ({ +vi.mock('node:fs/promises', () => ({ default: { readFile: vi.fn().mockResolvedValue('---\n---\nSome description paragraph'), }, }))Also apply this same fix to
packages/core/src/sync/text.test.tsandpackages/core/src/sync/sidebar/sidebar.test.ts, which use the identical pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/sidebar/landing.test.ts` around lines 5 - 9, Replace the dynamic import overload in the vi.mock call with the string-path overload so the factory can stay synchronous: change vi.mock(import('node:fs/promises'), () => ({ default: { readFile: vi.fn().mockResolvedValue(...) } })) to vi.mock('node:fs/promises', () => ({ default: { readFile: vi.fn().mockResolvedValue(...) } })); apply the same change in the tests that mock readFile in packages/core/src/sync/text.test.ts and packages/core/src/sync/sidebar/sidebar.test.ts so the vi.mock invocation uses the module string ID instead of import(...) (keep the readFile mock implementation intact).
3-12:⚠️ Potential issue | 🔴 CriticalFix the module-under-test import path.
The test file is now colocated with
landing.ts, but the import path still uses../../src/sync/sidebar/landing, which resolves to the wrong location due to the extrasrc/segment. Use a relative import to the colocated source file.Fix
const { buildWorkspaceCardJsx, generateLandingContent } = - await import('../../src/sync/sidebar/landing') + await import('./landing')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/sidebar/landing.test.ts` around lines 3 - 12, The test imports the module-under-test from the wrong path ("../../src/sync/sidebar/landing") causing resolution issues; update the dynamic import to the colocated source file (use a relative import like './landing') so the destructured symbols buildWorkspaceCardJsx and generateLandingContent are imported from the local landing.ts module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/refactor-file-structure.md:
- Around line 22-35: Update the testing rule to match the new colocated test
convention: find the rule text that reads "Place test files in `test/` directory
within the package (e.g., `packages/core/test/config.test.ts`)" and replace it
with a short guidance that tests should be colocated with their source under src
(for example, "Place test files alongside their source in the package's src
directory (e.g., packages/core/src/config.test.ts)"). Also update any examples
and the rationale paragraph in the same document to reference the colocated
layout and remove references to the old `test/` directory so the rule is
consistent with the changeset.
In @.claude/rules/typescript.md:
- Around line 40-53: Update the rule scope and wording so the "File Structure"
guidance applies to all TypeScript sources (glob **/*.{ts,tsx}) instead of only
packages/**/*.ts, and broaden Step 3 to cover all exported declarations
(exported functions, constants, types, interfaces, enums, and React components)
with full JSDoc; also mention TSX files where relevant and ensure private
helpers requirement remains for non-exported members. Locate the "File
Structure" section and change the target glob, adjust the numbered list item 3
text from "Exported functions" to "Exported declarations (functions, consts,
types, interfaces, enums, components)" and add a short note that the rule
applies to .ts and .tsx files across the repo.
In @.vscode/settings.json:
- Line 22: Remove or disable the workspace-level permission-skip setting by
deleting the "claudeCode.allowDangerouslySkipPermissions" entry (or setting it
to false) from the committed .vscode/settings.json and move any intentional
opt-in to a local, uncommitted user setting; update references to
"claudeCode.allowDangerouslySkipPermissions" so only developer-local settings
control this behavior and ensure the shared settings file no longer enables
dangerous permission skipping.
In `@packages/core/src/banner/index.ts`:
- Line 173: The function writeAsset currently accepts two positional params
(asset: GeneratedAsset, publicDir: string); change its signature to a single
object parameter (e.g. writeAsset({ asset, publicDir }: { asset: GeneratedAsset;
publicDir: string }): Promise<AssetResult<string>>) and keep the explicit return
type. Inside the function, replace direct parameter references with the
destructured names, and update every call site to pass an object { asset,
publicDir } instead of two arguments. Ensure type imports (GeneratedAsset,
AssetResult) still match and run type checks to catch any missed call-sites.
In `@packages/core/src/banner/svg-banner.ts`:
- Around line 56-72: Trim params.tagline and treat an all-whitespace string as
absent: inside the match on params.tagline (the P.string branch), call .trim()
on the matched tagline and if the trimmed result is empty return the same shape
as the otherwise branch (markup: '', separatorY: art.artEndY + SEPARATOR_GAP);
otherwise use the trimmed text when calling buildTagline (keep using centerX,
taglineY and separatorY calculations and leave
CLI_SECTION_HEIGHT/SEPARATOR_GAP/TAGLINE_GAP logic unchanged). This ensures
blank or whitespace-only taglines do not produce an empty <text> node or extra
gap.
In `@packages/core/src/sync/resolve/index.ts`:
- Around line 51-53: Replace the current banner-style private section header in
resolve/index.ts with the exact convention marker; find the existing block
comment around the private helpers (currently the banner starting with "//
---------------------------------------------------------------------------"
near the top of the private section) and change it to the exact line "// ---
Private ---" so the file follows the repository's private-section convention.
In `@packages/core/src/sync/sidebar/index.ts`:
- Around line 70-72: The current leaf-handling branch in the sidebar builder
returns a non-clickable item ({ text: entry.title }) when entry.link is
null/undefined, but the reviewer wants truly skipped leaves; change the branch
inside the function handling each entry (the code checking entry.link === null
|| entry.link === undefined) to return undefined instead of { text: entry.title
}, and then update generateSidebar() to filter out falsy/undefined entries (e.g.
after mapping entries, run .filter(Boolean) or equivalent) so broken leaves are
removed entirely; look for the symbols entry, generateSidebar, and the
leaf-processing block to apply these two coordinated fixes.
In `@packages/core/src/sync/sidebar/sidebar.test.ts`:
- Line 3: The test imports the sidebar inject module via a broken relative path
'../../src/sync/sidebar/inject' which resolves to a non-existent location after
the test was moved; update the import in
packages/core/src/sync/sidebar/sidebar.test.ts so the dynamic import/reference
points to the colocated module (replace '../../src/sync/sidebar/inject' with
'./inject') to correctly load the inject module used by the tests.
In `@packages/templates/src/built-in.ts`:
- Around line 1-2: Module currently performs eager filesystem I/O at import time
(using readFileSync and join), causing import-time side effects; change to
lazy-loading by removing any top-level readFileSync calls and instead provide
factory/accessor functions (e.g., getTemplate(name) or createTemplatesLoader())
that perform readFileSync(join(import.meta.dirname, ...)) only when invoked;
update any usages referenced around the noted locations (lines 14-15 and 78-79)
to call the new loader/accessor instead of relying on module-initialized
template values.
In `@packages/ui/src/config.ts`:
- Line 303: The concatenation using .filter(Boolean).join('') can produce
invalid JS if fragments like colorModeJs, themeAttrJs, VSCODE_DETECT_JS, or
LOADER_DOTS_JS lack terminal semicolons; update the return to join fragments
with a safe separator (e.g., ';' or '\n') or ensure each fragment ends with a
semicolon before joining so the assembled inline script is always valid
JavaScript; modify the return expression that references colorModeJs,
themeAttrJs, VSCODE_DETECT_JS, and LOADER_DOTS_JS accordingly.
- Around line 40-42: The three helper functions use positional args and must be
converted to object-parameter style: update the function signatures for
loadGenerated, resolveColorMode, and resolveSidebarLinks to accept a single
options object (e.g., { dir, filename, fallback } for loadGenerated; { req,
defaultMode } or similar names for resolveColorMode; { links, config } for
resolveSidebarLinks) and keep their explicit return types; then update every
call site (the three loadGenerated calls, the resolveColorMode call, and the two
resolveSidebarLinks calls) to pass a single object with matching property names
instead of positional arguments and adjust any destructuring inside the
functions to use those property names. Ensure names are clear and consistent
with existing parameter semantics so behavior remains unchanged.
In `@packages/ui/src/theme/components/openapi/code-sample.tsx`:
- Around line 96-106: The buildUrl helper currently just concatenates baseUrl
and urlPath; change it to accept a single object parameter (e.g., { baseUrl,
urlPath, samples? }) and return a string, then normalize/trim slashes so no
duplicate or missing slashes occur, resolve templated path parameters (e.g.,
replace `{id}` tokens using provided sample values or fallback placeholders),
and append any sample query parameters in a normalized querystring; update the
function signature (buildUrl) and all call sites to the new object parameter
pattern to follow the TS convention and ensure generated snippets produce valid,
runnable URLs.
In `@packages/ui/src/theme/components/sidebar/sidebar-links.tsx`:
- Around line 51-55: renderIcon currently ignores the color field when icon is
passed as an object; update the function (renderIcon and the pattern that
matches SidebarLinkItem['icon']) to pass the object's color into the Icon
component (e.g., use i.color when matching { id: P.string }) so object-mode
icons preserve their configured color while keeping the P.string branch
unchanged.
In `@packages/ui/src/theme/components/theme-provider.tsx`:
- Around line 161-168: The lookup in resolveColorPairs uses COLOR_VAR_MAP[key]
which can pick up inherited prototype properties (e.g., "constructor") and may
return non-array values causing .map to throw; fix by guarding the lookup—use a
safe hasOwnProperty check (e.g.,
Object.prototype.hasOwnProperty.call(COLOR_VAR_MAP, key)) and verify the
retrieved vars is an actual array (Array.isArray(vars)) before calling vars.map,
otherwise return []; keep the rest of resolveColorPairs logic intact.
- Around line 221-229: The parseColors function currently trusts JSON.parse(raw)
as a Record<string,string> which can yield null or non-object and later cause
Object.keys(colors) to throw; update parseColors to validate the parsed value
from JSON.parse(raw) (in function parseColors) ensuring it is a non-null plain
object and that each value is a string (coerce/filter values to strings if
needed) and return {} for any invalid shape so callers (e.g., where
Object.keys(colors) is used) always receive a safe Record<string,string>.
---
Outside diff comments:
In `@packages/config/src/errors.ts`:
- Around line 43-49: The function configError currently accepts two positional
parameters (type: ConfigErrorType, message: string); change its signature to
accept a single object parameter (e.g., { type, message }: { type:
ConfigErrorType; message: string }) while preserving the explicit return type
ConfigError, and update all downstream callsites that invoke configError with
two args (notably the usages in loader.ts) to pass a single object argument with
matching property names; keep the returned shape (_tag, type, message)
unchanged.
In `@packages/core/src/sync/manifest.ts`:
- Around line 58-60: The recursive removal in pruneEmptyDirs is using
dir.startsWith(stopAt), which is unsafe; update pruneEmptyDirs to resolve and
normalize both stopAt and dir (using path.resolve) and then compute
path.relative(stopAtResolved, dirResolved) and only allow recursion/deletion
when the relative path does not start with '..' and is not an absolute path
(e.g., relative === '' || (!relative.startsWith('..') &&
!path.isAbsolute(relative))). Also ensure callers (e.g., where abs =
path.resolve(outDir, oldPath) and pruneEmptyDirs(path.dirname(abs), outDir) is
invoked) pass resolved/normalized outDir to pruneEmptyDirs so that boundary
checks are correct; if the check fails, bail out without deleting and surface an
error or skip pruning.
In `@packages/core/src/sync/openapi.ts`:
- Around line 127-132: Convert any helper functions used by syncOpenAPI that
currently accept two or more positional parameters into single object-parameter
signatures and add explicit return types; update all their call sites inside
syncOpenAPI accordingly. Specifically, find the helper functions declared/used
within the syncOpenAPI implementation (the helpers referenced in the review
around the later parts of the file) and change signatures from fn(a: A, b: B,
...) to fn({ a, b }: { a: A; b: B; ... }): ReturnType, then adjust every
invocation to pass an object (e.g., helper({ a, b })). Ensure TypeScript types
(OpenAPIConfig, SyncContext, SingleSyncResult) are preserved and exported types
updated if needed.
In `@packages/core/src/sync/planning.ts`:
- Around line 126-133: The functions generatePlanningIndex and naturalCompare
currently use positional parameters; change both to accept a single object
parameter each (e.g., generatePlanningIndex({ files, planningDir }):
Promise<string> and naturalCompare({ a, b }: { a: string; b: string }): number)
with explicit return types, update all internal references to use the
destructured names, and update every call site to pass an object instead of
positional args; ensure any exports/types/tests are updated accordingly and that
the functions' behavior and return types remain the same.
In `@packages/core/src/sync/sidebar/landing.test.ts`:
- Around line 5-9: Replace the dynamic import overload in the vi.mock call with
the string-path overload so the factory can stay synchronous: change
vi.mock(import('node:fs/promises'), () => ({ default: { readFile:
vi.fn().mockResolvedValue(...) } })) to vi.mock('node:fs/promises', () => ({
default: { readFile: vi.fn().mockResolvedValue(...) } })); apply the same change
in the tests that mock readFile in packages/core/src/sync/text.test.ts and
packages/core/src/sync/sidebar/sidebar.test.ts so the vi.mock invocation uses
the module string ID instead of import(...) (keep the readFile mock
implementation intact).
- Around line 3-12: The test imports the module-under-test from the wrong path
("../../src/sync/sidebar/landing") causing resolution issues; update the dynamic
import to the colocated source file (use a relative import like './landing') so
the destructured symbols buildWorkspaceCardJsx and generateLandingContent are
imported from the local landing.ts module.
In `@packages/core/src/sync/workspace.ts`:
- Around line 195-211: The synthesized category link generation in the
categories.map block (using category.link, slugify(String(category.title)),
existingLinks, and workspaceToSection) only checks collisions against
existingLinks and doesn't handle empty slugs, so multiple new categories can
collide or produce "/" for an empty slug; fix by computing a safeSlug (fallback
to a non-empty value derived from title or an index when slugify returns empty),
maintain a local Set (e.g., newLinks) while iterating categories to ensure each
generated link is unique (on collision append a deterministic suffix like `-1`,
`-2` or use the category index) and check against both existingLinks and
newLinks before accepting a link, and only include the category when a unique
non-root link has been produced.
In `@packages/ui/src/config.ts`:
- Line 159: The direct JSON.parse(readFileSync(p, 'utf8')) can throw on
malformed files; wrap the file read + parse in a try/catch inside the function
that returns a T (the code using readFileSync and JSON.parse, referencing
variables p and fallback), and on parse error log a warning (e.g., console.warn
or the project logger) including the file path and error, then return fallback
as a safe fallback value; ensure the returned value still has the T type and do
not rethrow so config generation continues.
In `@packages/ui/src/head/read.ts`:
- Around line 73-92: The readAsset function currently treats every exception as
a missing asset; modify readAsset to catch the thrown error (capture it as e)
after calling resolveAsset/readFileSync, detect whether it's actually a
missing-file error (e.g., e.code === 'ENOENT') and set AssetError.type =
'missing' only in that case, otherwise set a more appropriate type like 'io' or
'permission' and include the original error details (e.message and e.code) in
the AssetError.message and/or path fields so permission/path/IO errors are
surfaced instead of being masked; keep the returned tuple shape
[AssetError|null, content|null] unchanged and reference readAsset, resolveAsset,
and readFileSync locations when applying the change.
In `@packages/ui/src/theme/components/openapi/code-sample.tsx`:
- Around line 134-157: The generated Python snippet in generatePython
incorrectly inlines JSON via JSON.stringify (payloadLines and jsonArg) which
produces JS-style literals (true/false/null) invalid in Python; change
generation to import json, build payload as payload = json.loads('<escaped JSON
string>') (use JSON.stringify on the JS side to create the string and ensure
it's properly escaped for inclusion in single/double quotes), remove inlined
Python literal output, and keep json argument as ',\n json=payload' so
requests.<method> uses the runtime-parsed payload; update generatePython (and
related payloadLines/jsonArg construction) to produce these lines and ensure
buildUrl/extractBodyExample/isBodyMethod usage remains the same.
In `@packages/ui/src/theme/components/openapi/markdown.ts`:
- Around line 326-333: The cURL builder currently hardcodes "Content-Type:
application/json" in headerPart and always JSON.stringifies the example in
bodyPart; instead, read the request media type from requestBody.content (e.g.,
pick the first key) and use that media type for the header (replace the
hardcoded string in headerPart), and conditionally serialize the bodyExample in
bodyPart based on that media type: if the media type is JSON-like (contains
"json") stringify and shellQuote as before, if it's form-data build -F parts or
for other raw media types use the raw example string (shellQuote as
appropriate). Update the logic around isBodyMethod(normalizedMethod),
requestBody, extractBodyExample, headerPart and bodyPart to use the derived
mediaType and handle form-data vs JSON vs raw payloads accordingly.
In `@packages/ui/src/theme/components/openapi/overview.tsx`:
- Around line 232-234: The tag count label always uses "operations" causing "1
operations" for single-entry tags; update the render in the component that
outputs the element with className "zp-oas-tag-group__count" (where
tag.operationCount is used) to pluralize correctly by emitting "operation" when
tag.operationCount === 1 and "operations" otherwise (e.g., build the string from
tag.operationCount plus a conditional suffix) so the label reads "1 operation"
or "N operations" appropriately.
In `@packages/ui/src/theme/components/openapi/response-list.tsx`:
- Around line 55-78: The extractSchema function currently takes the very first
content entry's schema which can be missing; change extractSchema to iterate the
content entries (e.g., via Object.values/Object.entries on the content object
inside extractSchema) and return the first non-null mediaType['schema'] found
(casting to Record<string, unknown> | null), returning null only if none of the
media types contain a schema; update the match logic around content (and the
inner entries handling) to check each mediaType rather than assuming the first
entry has a schema.
In `@packages/ui/src/theme/components/openapi/schema-viewer.tsx`:
- Around line 201-229: The schema toggle button currently changes the UI but
doesn't expose its state to assistive tech; update the button in the
SchemaViewer component to include aria-expanded={expanded} and aria-controls
pointing to the indented content, and give the indented content a stable id
(e.g., compute contentId using the component's unique symbols like name and
depth such as const contentId = `zp-oas-schema-${name}-${depth}`) then set
id={contentId} on the <div className="zp-oas-schema__indent"> and
aria-controls={contentId} on the <button> so screen readers can know which
region is toggled; keep using the existing expanded and toggle variables and
ensure aria-expanded receives the boolean expanded value.
In `@packages/ui/src/theme/components/openapi/spec-utils.ts`:
- Around line 81-111: extractBodyExample currently only reads mediaType.example
and returns null if that's absent; update it to also check mediaType.examples
(take the first example value from the examples map) and then fallback to
mediaType.schema?.example (or schema examples if present) before returning null.
In the function extractBodyExample, when you destructure mediaType, look for an
examples property (iterate Object.values or Object.entries and extract the
example payload from the first Example Object) and if none found check
mediaType.schema?.example (or schema.examples) and return that value if present;
otherwise keep returning null.
---
Nitpick comments:
In `@packages/cli/src/commands/dump.ts`:
- Around line 8-15: The DumpEntry interface has all properties marked readonly
except items; update the interface definition (DumpEntry) to mark the items
property as readonly (e.g., readonly items?: DumpEntry[]) so the array reference
is immutable like the other fields and consistent with the immutability pattern
used across the interface.
- Around line 29-34: The code uses configErr.errors.map purely for logging side
effects which unnecessarily allocates an array; replace the map call with
forEach so you iterate without creating a new array (i.e., change
configErr.errors.map(...) to configErr.errors.forEach(...) and remove the return
inside the callback), keeping the same logging call to ctx.logger.error and
preserving the path construction (const path = err.path.join('.')).
In `@packages/cli/src/commands/setup.ts`:
- Line 39: Remove the redundant directory creation by deleting the await
fsPromises.mkdir(paths.publicDir, { recursive: true }) call in the setup flow;
rely on generateAssets (from `@zpress/core`) which already performs
fs.mkdir(params.publicDir, { recursive: true }) internally, so keep the call to
generateAssets and any related error handling but remove the explicit
fsPromises.mkdir invocation to avoid duplicate work.
- Around line 98-104: Refactor execSilent to accept a single object parameter
instead of three positional args: change its signature from execSilent(file:
string, args: readonly string[], cwd: string) to execSilent({ file, args, cwd }:
{ file: string; args: readonly string[]; cwd: string }), keep the same return
type and body, then update all call sites (notably extractGitRepoName) to call
execSilent({ file: ..., args: ..., cwd: ... }) so parameter names are explicit
and the function complies with the 2+ parameter guideline; ensure TypeScript
types are updated accordingly and run/adjust any local tests or usages.
- Around line 45-48: The handler currently calls process.exit(1) after logging
asset generation failure; instead, modify the error path to return or throw the
error so the framework can handle the non-zero exit (avoid terminating the
process). Locate the block checking assetErr (the ctx.logger.error(`Asset
generation failed: ${assetErr.message}`) followed by process.exit(1)) and
replace the process.exit(1) call with either throwing assetErr (or a new Error
with context) or returning a rejected Promise/value per the handler signature so
tests can observe the failure without killing the test process.
In `@packages/cli/src/lib/check.ts`:
- Around line 132-137: Replace the non-idiomatic use of
configResult.errors.map(...) (which performs only side-effects and returns null)
with configResult.errors.forEach(...); remove the unnecessary return null and
the oxlint-disable comment around this block so the iteration is clear and
side-effects are explicit, keeping the logger.message calls intact (look for the
configResult.errors.map usage and the logger.message invocation in check.ts).
In `@packages/cli/src/lib/watcher.ts`:
- Around line 34-43: The createWatcher function currently uses three positional
params (initialConfig, paths, onConfigReload) which violates the rule requiring
object parameters for functions with 2+ args; change the function signature to
accept a single object parameter (e.g., { initialConfig, paths, onConfigReload
}: { initialConfig: ZpressConfig; paths: Paths; onConfigReload?: (newConfig:
ZpressConfig) => Promise<void> }) while keeping the explicit return type
WatcherHandle, update any internal references inside createWatcher to
destructure from that object, and update all call sites to pass an object with
those named properties.
- Around line 170-172: Replace the existing private-section comment block that
reads the long dashed header and "Private" (the multi-line marker starting with
"// ---------------------------------------------------------------------------"
and "// Private") with the canonical single-line marker "// --- Private ---" so
the section header matches the PR convention; locate the header in watcher.ts
where the private section is demarcated and swap it for the exact string "// ---
Private ---".
In `@packages/core/src/sync/copy.ts`:
- Around line 93-98: Refactor the helper that currently accepts positional
parameters (raw, page, ctx) to use a single object parameter (e.g., { raw, page,
ctx }) and add an explicit return type; update its JSDoc to document the object
shape and return type, and update all call sites accordingly; do the same for
the other helper referenced around the 119-123 region that uses multiple
positional args, ensuring both functions' signatures, JSDoc, and callers are
changed to use named object parameters and explicit TypeScript return types.
In `@packages/core/src/sync/index.ts`:
- Around line 382-387: The concatPage function unnecessarily clones the pages
array when page is undefined; update concatPage to return the original pages
reference in that branch instead of spreading into a new array. Specifically, in
the function concatPage(pages: readonly PageData[], page: PageData | undefined)
return pages directly when page is falsy (ensuring the return type stays
PageData[] via an appropriate cast or by adjusting the parameter/return types)
and only create a new array when appending with [...pages, page].
- Line 295: The helper function copySeeded (async function copySeeded(src:
string, dest: string): Promise<void>) and the other helper functions referenced
(the helpers declared at the other noted spots) currently use positional
parameters with 2+ args; change each to accept a single object parameter (e.g.,
copySeeded({ src, dest }: { src: string; dest: string }): Promise<void>) and
keep the explicit return type; update all internal references/call sites to pass
a single object and apply the same object-parameter refactor to the other helper
functions at the other noted locations so they follow the repository TS
function-shape rule.
In `@packages/core/src/sync/manifest.ts`:
- Around line 47-51: Change the multi-argument functions cleanStaleFiles and
pruneEmptyDirs to accept a single object parameter (e.g., { outDir, oldManifest,
newManifest } and { dir, rootDir } respectively) and keep/declare explicit
return types (Promise<number> and Promise<void> or appropriate). Update the
function signatures in packages/core/src/sync/manifest.ts, replace positional
parameter references inside the functions with the object properties, and update
all call sites to pass an object instead of positional args; also export/update
any related types/interfaces if needed to keep public API consistent.
In `@packages/core/src/sync/openapi.ts`:
- Around line 19-31: The HTTP_METHODS constant is duplicated; centralize it into
a single shared export and import it where needed instead of redefining it.
Create a single exported constant (e.g., export const HTTP_METHODS: readonly
string[] = [...]) in a shared OpenAPI utilities module and replace the local
definitions in both the openapi.ts and spec-utils.ts usages by importing that
shared HTTP_METHODS, preserving the readonly string[] type and all existing
values so compilation and behavior remain unchanged.
In `@packages/core/src/sync/planning.ts`:
- Around line 82-84: Replace the current private-section separator block ("//
---------------------------------------------------------------------------\n//
Private\n//
---------------------------------------------------------------------------")
with the repository-standard single-line separator "// --- Private ---" so the
file's private section header matches the canonical pattern used elsewhere
(update in packages/core/src/sync/planning.ts where the Private section header
appears).
In `@packages/core/src/sync/resolve/sort.ts`:
- Around line 60-74: The IIFE usage in sectionFirst is unnecessary and verbose;
simplify by replacing the IIFE blocks that compute aIsSection and bIsSection
with direct boolean checks on a.items and b.items (e.g., compute aIsSection and
bIsSection from the expression "items exist and length > 0" and coerce to the
same numeric values used now) and keep the same return expression (aIsSection -
bIsSection) so the sorting behavior of sectionFirst remains unchanged.
- Around line 83-103: The toResolvedPage function uses IIFEs and match for
simple null checks; replace those with optional chaining and nullish coalescing:
get source via entry.page?.source, frontmatter via entry.page?.frontmatter ??
{}, and normalize link via entry.link ?? '' (or entry.link ?? '' using optional
chaining if needed). Update the function body (toResolvedPage, referencing
ResolvedEntry/ResolvedPage and entry.page/entry.link) to remove the IIFEs and
match call and return the same shape with the simplified expressions.
In `@packages/core/src/sync/sidebar/inject.ts`:
- Around line 28-33: The function injectLandingPages currently uses four
positional parameters (entries, configSections, workspaces, colorIndex) which is
brittle; change its signature to accept a single object parameter (e.g., params:
{ entries: readonly ResolvedEntry[]; configSections: readonly Section[];
workspaces: readonly Workspace[]; colorIndex?: { value: number } }) and keep the
explicit return type void, update the function body to destructure those fields,
and update all callers to pass a single object argument; add or export a named
type/interface for the param shape if helpful to keep call sites and tests typed
and maintainable.
In `@packages/core/src/sync/workspace.ts`:
- Around line 324-329: The helper functions (e.g., buildWorkspaceSection and the
other multi-argument helpers referenced at lines 399 and 511) use positional
parameters; convert each to accept a single object parameter (e.g., { heading,
description, items, scopePrefix }) and add explicit return types to their
signatures so callers are self-descriptive and conform to the TS rule; update
all call sites to pass a named-object argument and preserve original behavior
and types for items (readonly Workspace[]) and returned string values.
- Around line 232-234: Replace the current private-section delimiter that reads
"// ---------------------------------------------------------------------------
// Private //
---------------------------------------------------------------------------"
with the agreed convention "// --- Private ---" so the file's private section
header matches the PR objective and project grep conventions; update the
delimiter in packages/core/src/sync/workspace.ts where the existing "Private"
separator is defined (around the private section header).
In `@packages/ui/src/head/read.ts`:
- Around line 20-43: Both readCss and readJs duplicate identical logic for
calling readAsset and handling errors; introduce a small private helper (e.g.,
readPublicAsset or readAssetOrEmpty) that accepts relativePath and the asset
type/name if desired, calls readAsset(relativePath), logs errors to
process.stderr with the same `[zpress] ${error.message}\n` format, and returns
'' on error or the content on success; then update readCss and readJs to
delegate to this helper, keeping the public signatures unchanged and preserving
existing behavior.
In `@packages/ui/src/theme/components/home/feature.tsx`:
- Around line 32-34: Replace the current private section separator comment
string "//
---------------------------------------------------------------------------"
that precedes the "Private" section in feature.tsx with the project-standard
separator "// --- Private ---" so the file matches the documented convention;
locate the block containing the "Private" header comment and swap the long
dashed line for the shorter standard form.
In `@packages/ui/src/theme/components/home/workspaces.tsx`:
- Around line 31-33: Replace the non-standard private-section banner line that
currently reads something like "//
---------------------------------------------------------------------------"
with the canonical marker "// --- Private ---" in the workspaces.tsx private
section; locate the existing header comment in the file (the block labeled
"Private") and update it to exactly "// --- Private ---" so it matches the
project-wide convention.
In `@packages/ui/src/theme/components/openapi/markdown.ts`:
- Around line 126-131: Convert buildOperationHeader to accept a single object
parameter and add an explicit return type: change signature to something like
function buildOperationHeader(args: { method: string; urlPath: string;
deprecated: boolean }): string, destructure inside the function, and preserve
behavior (uppercase method, backticked urlPath, deprecated tag). Apply the same
object-parameter + explicit return type refactor to the other private helpers in
this file that currently use 2+ positional parameters (the additional helpers
flagged in the review), updating their callers accordingly to pass a single
object.
In `@packages/ui/src/theme/components/shared/tech-icon-table.tsx`:
- Around line 15-31: Move the non-exported style constants (tableStyle,
iconCellStyle, iconWrapperStyle) below the exported TechIconTable component to
match the repo convention: keep exports at top, then add a "// --- Private ---"
separator and annotate each constant with an `@private` JSDoc comment. Ensure
TechIconTable remains exported at the top of the file and only the three style
constants are relocated/marked as private.
In `@packages/ui/src/theme/components/theme-provider.tsx`:
- Line 178: The helper applyColorOverrides currently uses two positional args
(html, colors) which breaks the repo convention; change its signature to accept
a single object parameter (e.g., { html, colors }: { html: HTMLElement; colors:
Record<string,string> }) and keep the explicit return type void, then update
every call site that invokes applyColorOverrides(html, colors) to pass an object
instead (applyColorOverrides({ html, colors })); ensure TypeScript types match
and update imports/usage in any files referencing applyColorOverrides.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/sync/text.test.ts (1)
1-10: Consider moving test file tosync/resolve/for true colocation.The test file is at
sync/text.test.tsbut tests code fromsync/resolve/text.ts. As per coding guidelines, test files should be created alongside source files. Moving this topackages/core/src/sync/resolve/text.test.tswould align with the colocation convention.This is optional given the PR scope already accomplishes the major relocation from the top-level
test/directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/text.test.ts` around lines 1 - 10, Move the test so it sits alongside the module that exports deriveText and kebabToTitle (the resolve/text module) instead of the current sync directory; update the test file location and any relative imports accordingly so it imports the same './resolve/text' module and the mocked 'node:fs/promises' in that new location, and verify the vi.mock and awaited imports still resolve correctly and the tests run.
🤖 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/ui/tsconfig.json`:
- Line 9: Update the tsconfig.json "exclude" array to also drop JSX test files
by adding the pattern "**/*.test.tsx"; locate the "exclude" property in the
package's tsconfig.json (currently containing "node_modules", "dist",
"**/*.test.ts") and append "**/*.test.tsx" so test files with .tsx are
consistently excluded.
---
Nitpick comments:
In `@packages/core/src/sync/text.test.ts`:
- Around line 1-10: Move the test so it sits alongside the module that exports
deriveText and kebabToTitle (the resolve/text module) instead of the current
sync directory; update the test file location and any relative imports
accordingly so it imports the same './resolve/text' module and the mocked
'node:fs/promises' in that new location, and verify the vi.mock and awaited
imports still resolve correctly and the tests run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5f71642-5f42-49df-a907-6ac429362520
📒 Files selected for processing (18)
packages/cli/src/commands/build.tspackages/cli/src/commands/dev.tspackages/cli/src/commands/draft.tspackages/cli/src/commands/dump.tspackages/cli/src/commands/serve.tspackages/cli/src/commands/sync.tspackages/cli/src/lib/check.test.tspackages/cli/tsconfig.jsonpackages/config/schemas/schema.jsonpackages/config/tsconfig.jsonpackages/core/src/sync/sidebar/landing.test.tspackages/core/src/sync/sidebar/sidebar.test.tspackages/core/src/sync/text.test.tspackages/core/tsconfig.jsonpackages/templates/tsconfig.jsonpackages/theme/tsconfig.jsonpackages/ui/src/css.test.tspackages/ui/tsconfig.json
✅ Files skipped from review due to trivial changes (2)
- packages/theme/tsconfig.json
- packages/config/schemas/schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/sync/sidebar/landing.test.ts
- Update testing.md to reflect colocated test convention (src/ not test/) - Extend typescript.md rule paths to include .tsx files - Disable claudeCode.allowDangerouslySkipPermissions in shared settings - Refactor writeAsset to object parameter signature - Treat blank taglines as absent in SVG banner composer - Fix broken sidebar leaf skipping to return undefined instead of text-only item - Defer template loading via lazy getBuiltInTemplates() factory - Convert loadGenerated, resolveColorMode, resolveSidebarLinks to object params - Insert semicolon separators when joining inline JS fragments - Normalize URLs in code-sample buildUrl with slash handling - Apply icon color in sidebar-links object icon mode - Harden COLOR_VAR_MAP lookup with Array.isArray guard - Validate parsed JSON shape in parseColors before returning - Add .test.tsx to UI tsconfig exclude pattern Co-Authored-By: Claude <noreply@anthropic.com>
All test files were moved from packages/*/test/ to packages/*/src/ but the vitest include patterns still pointed to test/**/*.test.ts. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/banner/index.ts (1)
107-124:⚠️ Potential issue | 🟠 MajorPropagate generation/write failures instead of returning silent success.
At Line 111 and Line 122, errors are dropped and the reducer continues. This makes
generateAssetsreturn[null, written]even when asset generation or disk writes fail.♻️ Proposed fix
- const written = await generators.reduce<Promise<readonly string[]>>( + const [reducedErr, written] = await generators.reduce<Promise<AssetResult<readonly string[]>>>( async (accPromise, generate) => { - const acc = await accPromise + const [accErr, acc] = await accPromise + if (accErr) { + return [accErr, null] + } const [err, asset] = generate() if (err) { - return acc + return [err, null] } const filePath = path.resolve(params.publicDir, asset.filename) const shouldWrite = await shouldGenerate(filePath) if (!shouldWrite) { - return acc + return [null, acc] } const [writeErr, filename] = await writeAsset({ asset, publicDir: params.publicDir }) if (writeErr) { - return acc + return [writeErr, null] } - return [...acc, filename] + return [null, [...acc, filename]] }, - Promise.resolve([]) + Promise.resolve([null, []] as const) ) - return [null, written] + if (reducedErr) { + return [reducedErr, null] + } + return [null, written]As per coding guidelines: "Use Result tuples for error handling instead of throw statements".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/banner/index.ts` around lines 107 - 124, The reducer in generators.reduce inside generateAssets currently swallows errors from generate() and writeAsset(), returning silent success; change it to propagate Result tuples: when generate() yields an error, short-circuit and return a rejecting Result tuple (e.g., [err, []]) up the promise chain instead of continuing, and likewise when writeAsset(...) yields writeErr return that error tuple immediately; keep using shouldGenerate(...) as-is but ensure any failure from shouldGenerate or async calls is converted to and returned as the Result tuple so generateAssets returns the first error (or aggregated error) with no silent success.packages/core/src/banner/svg-banner.ts (1)
191-194:⚠️ Potential issue | 🟠 MajorEscape FIGlet rows before embedding into SVG text nodes.
On Line 193, raw
linecontent is injected into SVG markup. This should be XML-escaped to prevent malformed SVG and markup injection when titles contain XML-sensitive characters.🔧 Proposed fix
const textLines = params.lines .map((line, i) => { + const escapedLine = escapeXml(line) const y = params.startY + i * ART_LINE_HEIGHT - return ` <text class="text brand" font-size="${ART_FONT_SIZE}" y="${y}" xml:space="preserve">${line}</text>` + return ` <text class="text brand" font-size="${ART_FONT_SIZE}" y="${y}" xml:space="preserve">${escapedLine}</text>` }) .join('\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/banner/svg-banner.ts` around lines 191 - 194, The SVG text nodes currently interpolate raw FIGlet row strings into the template (in the .map callback that computes y using params.startY and ART_LINE_HEIGHT and sets font-size using ART_FONT_SIZE), which can produce malformed SVG or allow injection; fix this by XML-escaping the FIGlet row content before embedding (escape &, <, >, " and ')—add or reuse an escapeXml helper (e.g., escapeXml(string): string) and replace the `${line}` interpolation with the escaped value in the text node generation so all rows are safely encoded.
♻️ Duplicate comments (1)
packages/ui/src/theme/components/openapi/code-sample.tsx (1)
96-111:⚠️ Potential issue | 🟠 MajorResolve templated/path query parameters inside
buildUrl(still unresolved).
buildUrlonly concatenates normalized base/path, so placeholders like{id}and query samples are not materialized. Since every generator depends on this helper, snippets can be non-runnable for common OpenAPI operations.🔧 Suggested direction
function buildUrl(input: { readonly baseUrl: string readonly path: string + readonly parameters?: readonly Record<string, unknown>[] }): string { const base = input.baseUrl.replace(/\/+$/, '') - const urlPath = input.path.startsWith('/') ? input.path : `/${input.path}` - return `${base}${urlPath}` + const normalizedPath = input.path.startsWith('/') ? input.path : `/${input.path}` + + const parameters = input.parameters ?? [] + const resolvedPath = parameters + .filter((p) => p.in === 'path') + .reduce((acc, p) => { + const name = String(p.name ?? '') + const raw = p.example ?? p.default ?? `{${name}}` + return name ? acc.replaceAll(`{${name}}`, encodeURIComponent(String(raw))) : acc + }, normalizedPath) + + const url = new URL(resolvedPath, `${base}/`) + parameters + .filter((p) => p.in === 'query') + .forEach((p) => { + const name = String(p.name ?? '') + const raw = p.example ?? p.default + if (name && raw !== undefined) url.searchParams.set(name, String(raw)) + }) + + return url.toString() }Then pass
parametersat call sites (generateCurl,generatePython,generateJavascript,generateGo,generateRuby,generateJava).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/code-sample.tsx` around lines 96 - 111, The buildUrl function currently only joins baseUrl and path; update buildUrl to accept parameter values (e.g., an additional parameters object) and resolve path template placeholders like {id} and fill query parameters from parameter definitions, URL-encoding values and omitting undefined ones before returning the full URL; then update all callers (generateCurl, generatePython, generateJavascript, generateGo, generateRuby, generateJava) to pass the operation parameters into buildUrl so generated snippets include concrete path and query values.
🧹 Nitpick comments (7)
packages/ui/src/theme/components/sidebar/sidebar-links.tsx (2)
68-70: Make external-link detection case-insensitive.URL schemes are case-insensitive;
HTTP://...currently won’t receive_blank/rel.♻️ Proposed change
-function externalProps(link: string): { target?: string; rel?: string } { - return match(link.startsWith('http://') || link.startsWith('https://')) +function externalProps(link: string): { target?: string; rel?: string } { + return match(/^https?:\/\//i.test(link)) .with(true, () => ({ target: '_blank' as const, rel: 'noopener noreferrer' })) .otherwise(() => ({})) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/sidebar/sidebar-links.tsx` around lines 68 - 70, The external-link detection uses link.startsWith('http://') / 'https://' which is case-sensitive; update the check in the match expression to compare a lowercased copy of the URL (e.g., const normalized = link.toLowerCase()) or use a case-insensitive test (e.g., /^https?:\/\//i) so that schemes like "HTTP://" are recognized and the returned attributes (target: '_blank', rel: 'noopener noreferrer') are applied correctly; update the logic around the match(...) that inspects the link variable to use the normalized check and keep the existing returned object shape.
32-34: Use a collision-resistant key for mapped sidebar items.
key={item.link}can collide when multiple items share the same URL, which can cause unstable reconciliation.♻️ Proposed change
- {props.items.map((item) => ( - <SidebarLinkEntry key={item.link} item={item} /> + {props.items.map((item) => ( + <SidebarLinkEntry key={`${item.link}:${item.text}`} item={item} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/sidebar/sidebar-links.tsx` around lines 32 - 34, The mapped sidebar list uses key={item.link} which can collide; update the mapping in the props.items.map(...) that renders <SidebarLinkEntry key=... item={item} /> to use a collision-resistant key such as a stable unique identifier (e.g., item.id) or, if no unique id exists, compose a stable key like `${item.link}-${index}` (use the map index only as a fallback) so SidebarLinkEntry receives a unique, stable key for each item.packages/core/src/sync/sidebar/index.ts (1)
66-79: KeepbuildSidebarEntrypure; move logging to caller edge.Line 77 introduces side effects inside a private transformation helper. Prefer returning
undefinedonly, and log ingenerateSidebarwhere entries are orchestrated.As per coding guidelines, "Treat every function as a value and push side effects to the edges of the application."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/sidebar/index.ts` around lines 66 - 79, Remove the logging side-effect from the transformation helper buildSidebarEntry: have it simply return undefined when a leaf entry has no link (keep the existing check for entry.link === null || undefined but do not call log.error). Move the log.error call into the caller that orchestrates entries (generateSidebar) so generateSidebar inspects buildSidebarEntry's return value and logs a descriptive error (including entry.title) when buildSidebarEntry returns undefined for a leaf without a link; keep all other return behavior unchanged.packages/core/src/banner/svg-banner.ts (2)
100-105: Remove unusedheightfromBannerLayout.
heightis always returned as0and never consumed. Keeping it in the public shape ofBannerLayoutis misleading.🔧 Proposed cleanup
interface BannerLayout { readonly width: number - readonly height: number readonly artSection: string readonly artEndY: number } @@ - return { width, height: 0, artSection, artEndY } + return { width, artSection, artEndY } @@ - return { width, height: 0, artSection, artEndY } + return { width, artSection, artEndY }Also applies to: 336-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/banner/svg-banner.ts` around lines 100 - 105, Remove the unused height property from the BannerLayout interface and update all places that construct or consume BannerLayout to no longer set or read height; specifically, edit the BannerLayout declaration to only include width, artSection, and artEndY, then update any functions or return values that currently include a height field (e.g., the routines that create/return BannerLayout objects) to stop populating it and update any callers that destructure or reference .height to use the remaining fields (width, artSection, artEndY) or compute height locally where actually needed.
319-329: Derive FIGlet height from actual rendered rows.Line 328 uses
FIGLET_ROWS, which can drift fromrenderFigletText(...)output and shift downstream layout. Usefiglet.lines.lengthto keep spacing correct if renderer output changes.🔧 Proposed refactor
- const artEndY = artStartY + (FIGLET_ROWS - 1) * ART_LINE_HEIGHT + const artEndY = artStartY + (figlet.lines.length - 1) * ART_LINE_HEIGHT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/banner/svg-banner.ts` around lines 319 - 329, The layout currently computes artEndY using the constant FIGLET_ROWS which can drift from actual figlet output; update the calculation to derive height from the rendered figlet output by using figlet.lines.length (from renderFigletText(params.title)) instead of FIGLET_ROWS so artEndY and any spacing derived from number of rows reflect the real output; update any other nearby uses that rely on FIGLET_ROWS for the figlet block (e.g., artEndY, ART line computations) to reference figlet.lines.length to avoid layout shifts.packages/ui/src/theme/components/theme-provider.tsx (2)
52-52: KeepuseIsomorphicLayoutEffectwith the rest of the private implementation details.Line 52 still leaves a private binding above the public export, so this file does not fully match the exports-first/private-block layout the refactor is standardizing. Moving it below the separator would keep the structure consistent.
Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/theme-provider.tsx` at line 52, The private binding useIsomorphicLayoutEffect (created via getIsomorphicEffect()) should be moved out of the public exports area and placed with the other private implementation details below the separator; locate the declaration of useIsomorphicLayoutEffect and the call to getIsomorphicEffect() and move that line into the private block (and do the same for the similar private binding(s) around the other occurrence at the bottom of the file), so all public exports appear before any private helper bindings.
178-184: SwitchapplyColorOverridesto an object parameter.This is the only two-argument helper in the private block, so it breaks the object-parameter convention this PR is otherwise applying.
♻️ Suggested refactor
-function applyColorOverrides(html: HTMLElement, colors: Record<string, string>): void { +function applyColorOverrides({ + html, + colors, +}: { + readonly html: HTMLElement + readonly colors: Readonly<Record<string, string>> +}): void { const pairs = resolveColorPairs(colors) // oxlint-disable-next-line unicorn/no-array-for-each -- DOM side effect; for-loops also banned pairs.forEach(([cssVar, value]) => { html.style.setProperty(cssVar, value) }) }Call sites at Lines 106, 113, 122, and 125 would then pass
{ html, colors }/{ html, colors: darkColors }.As per coding guidelines, "Use object parameters for functions with 2+ parameters and include explicit return types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/theme-provider.tsx` around lines 178 - 184, The helper applyColorOverrides currently takes two positional args; change its signature to accept a single object parameter and explicit return type: function applyColorOverrides({ html, colors }: { html: HTMLElement; colors: Record<string, string> }): void { ... } (keep body logic the same). Then update every call site that passed (html, colors) or (html, darkColors) to instead call applyColorOverrides({ html, colors }) or applyColorOverrides({ html, colors: darkColors }). Ensure types import/aliases remain valid and there are no remaining positional calls.
🤖 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/core/src/sync/sidebar/index.ts`:
- Around line 76-84: The current leaf-building block only skips entry.link when
it is null/undefined, allowing empty strings or whitespace to produce invalid
links; update the check around entry.link in the code that returns { text:
entry.title, link: entry.link } to treat blank or whitespace-only strings as
missing by trimming the value (e.g., const link = entry.link?.toString() and if
(!link || link.trim() === '') return undefined) and use the trimmed/validated
link when constructing the returned object so blank links are skipped.
In `@packages/ui/src/config.ts`:
- Around line 149-161: The loadGenerated function currently calls JSON.parse and
casts to T directly, which can throw on truncated JSON and permits invalid
shapes; wrap the read/parse in a try/catch and return params.fallback on parse
errors, then validate the parsed object with a runtime decoder/validator before
casting (e.g., dispatch validation based on params.name or change loadGenerated
to accept a validator callback), and on validation failure log the issue to
stderr and return params.fallback; follow the project's Result-tuple pattern for
error handling instead of letting errors propagate (use a Result-like return or
internal try/catch that yields fallback).
In `@packages/ui/src/theme/components/openapi/code-sample.tsx`:
- Around line 100-102: Update the stale JSDoc for the function buildUrl to
describe the single parameter named input (an object) and its properties instead
of separate baseUrl/urlPath params; specifically document input.baseUrl and
input.urlPath (or whatever property names buildUrl actually reads) and the
return value (Full URL string), so the docblock matches the function signature
and prevents future confusion when editing buildUrl.
In `@packages/ui/src/theme/components/theme-provider.tsx`:
- Around line 259-263: The cleanup in clearDotsInterval currently only clears
numeric timer handles; widen the guard to handle both browser numeric handles
and Node/jsdom Timeout objects by checking that (globalThis as Record<string,
unknown>).__zpDotsInterval is not null/undefined and then calling clearInterval
on it (casting to unknown/any if needed), and finally delete the
__zpDotsInterval property; update the function clearDotsInterval to reference
__zpDotsInterval and clear it for both number and object timer handles returned
by setInterval.
---
Outside diff comments:
In `@packages/core/src/banner/index.ts`:
- Around line 107-124: The reducer in generators.reduce inside generateAssets
currently swallows errors from generate() and writeAsset(), returning silent
success; change it to propagate Result tuples: when generate() yields an error,
short-circuit and return a rejecting Result tuple (e.g., [err, []]) up the
promise chain instead of continuing, and likewise when writeAsset(...) yields
writeErr return that error tuple immediately; keep using shouldGenerate(...)
as-is but ensure any failure from shouldGenerate or async calls is converted to
and returned as the Result tuple so generateAssets returns the first error (or
aggregated error) with no silent success.
In `@packages/core/src/banner/svg-banner.ts`:
- Around line 191-194: The SVG text nodes currently interpolate raw FIGlet row
strings into the template (in the .map callback that computes y using
params.startY and ART_LINE_HEIGHT and sets font-size using ART_FONT_SIZE), which
can produce malformed SVG or allow injection; fix this by XML-escaping the
FIGlet row content before embedding (escape &, <, >, " and ')—add or reuse an
escapeXml helper (e.g., escapeXml(string): string) and replace the `${line}`
interpolation with the escaped value in the text node generation so all rows are
safely encoded.
---
Duplicate comments:
In `@packages/ui/src/theme/components/openapi/code-sample.tsx`:
- Around line 96-111: The buildUrl function currently only joins baseUrl and
path; update buildUrl to accept parameter values (e.g., an additional parameters
object) and resolve path template placeholders like {id} and fill query
parameters from parameter definitions, URL-encoding values and omitting
undefined ones before returning the full URL; then update all callers
(generateCurl, generatePython, generateJavascript, generateGo, generateRuby,
generateJava) to pass the operation parameters into buildUrl so generated
snippets include concrete path and query values.
---
Nitpick comments:
In `@packages/core/src/banner/svg-banner.ts`:
- Around line 100-105: Remove the unused height property from the BannerLayout
interface and update all places that construct or consume BannerLayout to no
longer set or read height; specifically, edit the BannerLayout declaration to
only include width, artSection, and artEndY, then update any functions or return
values that currently include a height field (e.g., the routines that
create/return BannerLayout objects) to stop populating it and update any callers
that destructure or reference .height to use the remaining fields (width,
artSection, artEndY) or compute height locally where actually needed.
- Around line 319-329: The layout currently computes artEndY using the constant
FIGLET_ROWS which can drift from actual figlet output; update the calculation to
derive height from the rendered figlet output by using figlet.lines.length (from
renderFigletText(params.title)) instead of FIGLET_ROWS so artEndY and any
spacing derived from number of rows reflect the real output; update any other
nearby uses that rely on FIGLET_ROWS for the figlet block (e.g., artEndY, ART
line computations) to reference figlet.lines.length to avoid layout shifts.
In `@packages/core/src/sync/sidebar/index.ts`:
- Around line 66-79: Remove the logging side-effect from the transformation
helper buildSidebarEntry: have it simply return undefined when a leaf entry has
no link (keep the existing check for entry.link === null || undefined but do not
call log.error). Move the log.error call into the caller that orchestrates
entries (generateSidebar) so generateSidebar inspects buildSidebarEntry's return
value and logs a descriptive error (including entry.title) when
buildSidebarEntry returns undefined for a leaf without a link; keep all other
return behavior unchanged.
In `@packages/ui/src/theme/components/sidebar/sidebar-links.tsx`:
- Around line 68-70: The external-link detection uses link.startsWith('http://')
/ 'https://' which is case-sensitive; update the check in the match expression
to compare a lowercased copy of the URL (e.g., const normalized =
link.toLowerCase()) or use a case-insensitive test (e.g., /^https?:\/\//i) so
that schemes like "HTTP://" are recognized and the returned attributes (target:
'_blank', rel: 'noopener noreferrer') are applied correctly; update the logic
around the match(...) that inspects the link variable to use the normalized
check and keep the existing returned object shape.
- Around line 32-34: The mapped sidebar list uses key={item.link} which can
collide; update the mapping in the props.items.map(...) that renders
<SidebarLinkEntry key=... item={item} /> to use a collision-resistant key such
as a stable unique identifier (e.g., item.id) or, if no unique id exists,
compose a stable key like `${item.link}-${index}` (use the map index only as a
fallback) so SidebarLinkEntry receives a unique, stable key for each item.
In `@packages/ui/src/theme/components/theme-provider.tsx`:
- Line 52: The private binding useIsomorphicLayoutEffect (created via
getIsomorphicEffect()) should be moved out of the public exports area and placed
with the other private implementation details below the separator; locate the
declaration of useIsomorphicLayoutEffect and the call to getIsomorphicEffect()
and move that line into the private block (and do the same for the similar
private binding(s) around the other occurrence at the bottom of the file), so
all public exports appear before any private helper bindings.
- Around line 178-184: The helper applyColorOverrides currently takes two
positional args; change its signature to accept a single object parameter and
explicit return type: function applyColorOverrides({ html, colors }: { html:
HTMLElement; colors: Record<string, string> }): void { ... } (keep body logic
the same). Then update every call site that passed (html, colors) or (html,
darkColors) to instead call applyColorOverrides({ html, colors }) or
applyColorOverrides({ html, colors: darkColors }). Ensure types import/aliases
remain valid and there are no remaining positional calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08f30a19-e541-4339-a274-b0d341f46af6
📒 Files selected for processing (15)
.claude/rules/testing.md.claude/rules/typescript.md.vscode/settings.jsonpackages/core/src/banner/index.tspackages/core/src/banner/svg-banner.tspackages/core/src/sync/sidebar/index.tspackages/templates/src/built-in.test.tspackages/templates/src/built-in.tspackages/templates/src/index.tspackages/templates/src/registry.tspackages/ui/src/config.tspackages/ui/src/theme/components/openapi/code-sample.tsxpackages/ui/src/theme/components/sidebar/sidebar-links.tsxpackages/ui/src/theme/components/theme-provider.tsxpackages/ui/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- .vscode/settings.json
- packages/templates/src/built-in.ts
- packages/templates/src/built-in.test.ts
- Treat blank/whitespace links as missing in sidebar leaf entries - Catch JSON.parse failures in loadGenerated and return fallback - Update buildUrl JSDoc to match object parameter signature - Broaden clearDotsInterval to handle non-numeric timer handles Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/core/src/sync/sidebar/index.ts (1)
67-84:⚠️ Potential issue | 🟡 MinorUse the normalized link value when emitting sidebar items.
Line 76 validates with
trim(), but Line 83 and the section path still emit the rawentry.link, so whitespace-wrapped links can pass through unchanged.🐛 Proposed fix
function buildSidebarEntry(entry: ResolvedEntry): SidebarItem | undefined { + const link = entry.link?.trim() + if (entry.items && entry.items.length > 0) { return { text: entry.title, items: generateSidebar(entry.items), ...maybeCollapsed(entry.collapsible), - ...maybeLink(entry.link), + ...maybeLink(link), } } - if (!entry.link || entry.link.trim().length === 0) { + if (!link) { log.error(`[zpress] Leaf entry "${entry.title}" has no link — skipping`) return undefined } return { text: entry.title, - link: entry.link, + link, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sync/sidebar/index.ts` around lines 67 - 84, The code validates entry.link with trim() but returns the raw value; fix by normalizing the link once and reusing it: compute a normalizedLink = entry.link.trim() (handle undefined/null) and use normalizedLink for the validation check and when returning the leaf object and when calling maybeLink so emitted sidebar items never contain leading/trailing whitespace; update references in this block around generateSidebar, maybeCollapsed, and maybeLink to use normalizedLink instead of entry.link.packages/ui/src/theme/components/openapi/code-sample.tsx (1)
103-110:⚠️ Potential issue | 🟠 Major
buildUrlstill leaves templated/sample parameters unresolved across all generators.Line 103-110 only normalizes and concatenates
baseUrl+path; it still ignoresprops.parameters, so URLs like/users/{id}remain unresolved and query samples are never appended.🔧 Directional fix at the root cause
function buildUrl(input: { readonly baseUrl: string readonly path: string + readonly parameters?: readonly Record<string, unknown>[] }): string { const base = input.baseUrl.replace(/\/+$/, '') - const urlPath = input.path.startsWith('/') ? input.path : `/${input.path}` - return `${base}${urlPath}` + const urlPath = match(input.path.startsWith('/')) + .with(true, () => input.path) + .otherwise(() => `/${input.path}`) + + const params = input.parameters ?? [] + const resolvedPath = params.reduce((acc, p) => { + const location = p['in'] + const name = p['name'] + const sample = p['example'] + if (location === 'path' && typeof name === 'string' && sample !== undefined) { + return acc.replaceAll(`{${name}}`, encodeURIComponent(String(sample))) + } + return acc + }, urlPath) + + const query = new URLSearchParams( + params.flatMap((p) => { + const location = p['in'] + const name = p['name'] + const sample = p['example'] + return location === 'query' && typeof name === 'string' && sample !== undefined + ? [[name, String(sample)]] + : [] + }) + ).toString() + + return `${base}${resolvedPath}${query.length > 0 ? `?${query}` : ''}` }Also pass
parameters: props.parametersin eachbuildUrl(...)callsite (Line 120, 139, 172, 203, 267, 312).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/openapi/code-sample.tsx` around lines 103 - 110, The buildUrl function currently only normalizes baseUrl and path and ignores request parameters, leaving templated path params like /users/{id} unresolved and omitting query samples; update buildUrl to accept a parameters argument (e.g., add parameter: readonly parameters?: ParameterType[]) and implement logic to: 1) replace templated path variables in input.path by finding parameters with in === 'path' and substituting their example or default values, and 2) append a query string built from parameters with in === 'query' using their example/default values (URL-encode values and skip undefined). Then update every callsite to pass props.parameters into buildUrl (the comment-listed callsites referencing buildUrl at lines 120, 139, 172, 203, 267, 312) so calls use buildUrl({ baseUrl, path, parameters: props.parameters }). Ensure function signature and callers use the same ParameterType shape used elsewhere in the component.packages/ui/src/config.ts (1)
149-166:⚠️ Potential issue | 🟠 MajorDon't trust parseable generated JSON without a shape check.
Catching
JSON.parsefailures helps, but Line 162 still accepts any parseable payload asT. A stale or mismatched.generated/sidebar.json,nav.json, orworkspaces.jsonwill now flow intothemeConfigand fail later instead of falling back here. Please validate the decoded value before returning it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/config.ts` around lines 149 - 166, The loadGenerated function currently returns any parseable JSON as T (using params.contentDir, params.name, params.fallback) which allows stale/mismatched generated files to slip through; update loadGenerated to validate the parsed value's shape before returning by adding a runtime check (either accept a validator/predicate argument or perform a structural check based on params.name) and if validation fails, log an error to stderr and return params.fallback instead of casting and returning the parsed value. Ensure the validation is applied inside the try block after JSON.parse and uses clear identifying messages including params.name when logging.
🧹 Nitpick comments (1)
packages/ui/src/theme/components/theme-provider.tsx (1)
171-184: Consider using an object parameter for consistency with coding guidelines.
applyColorOverrideshas two parameters. The coding guidelines recommend object parameters for functions with 2+ parameters. Since this is a private helper and the current signature is clear, this is optional.♻️ Optional refactor
-function applyColorOverrides(html: HTMLElement, colors: Record<string, string>): void { +function applyColorOverrides(params: { + html: HTMLElement + colors: Record<string, string> +}): void { + const { html, colors } = params const pairs = resolveColorPairs(colors)Would also require updating call sites at lines 106, 113, 122, and 125.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/theme/components/theme-provider.tsx` around lines 171 - 184, Refactor applyColorOverrides to accept a single object parameter (e.g. { html, colors }) instead of two positional params to follow the object-parameter guideline: update the function signature for applyColorOverrides and adjust the implementation to read html and colors from the options object (still calling resolveColorPairs and invoking html.style.setProperty for each pair), then update all call sites that pass (html, colors) to pass the new object shape instead.
🤖 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/ui/src/config.ts`:
- Around line 160-162: The oxlint suppression comment is placed above the try
block instead of the actual filesystem call causing the linter to still flag
security/detect-non-literal-fs-filename; move the comment `//
oxlint-disable-next-line security/detect-non-literal-fs-filename -- safe:
derived from known output directory` so it directly precedes the readFileSync
call (the call to readFileSync(p, 'utf8') inside the JSON.parse in the function
that returns `JSON.parse(readFileSync(p, 'utf8')) as T`), ensuring the linter
suppression applies to the non-literal filename usage of `p`.
In `@packages/ui/src/theme/components/openapi/code-sample.tsx`:
- Line 108: The ternary expression assigning urlPath should be replaced with an
explicit conditional to satisfy eslint(no-ternary): initialize urlPath from
input.path and then, using an if check on input.path.startsWith('/'), prepend a
'/' when missing; update the code that references urlPath accordingly. Target
the urlPath assignment near the declaration that currently uses
input.path.startsWith('/') ? input.path : `/${input.path}` and replace it with a
clear if-based mutation so the logic remains identical without a ternary.
---
Duplicate comments:
In `@packages/core/src/sync/sidebar/index.ts`:
- Around line 67-84: The code validates entry.link with trim() but returns the
raw value; fix by normalizing the link once and reusing it: compute a
normalizedLink = entry.link.trim() (handle undefined/null) and use
normalizedLink for the validation check and when returning the leaf object and
when calling maybeLink so emitted sidebar items never contain leading/trailing
whitespace; update references in this block around generateSidebar,
maybeCollapsed, and maybeLink to use normalizedLink instead of entry.link.
In `@packages/ui/src/config.ts`:
- Around line 149-166: The loadGenerated function currently returns any
parseable JSON as T (using params.contentDir, params.name, params.fallback)
which allows stale/mismatched generated files to slip through; update
loadGenerated to validate the parsed value's shape before returning by adding a
runtime check (either accept a validator/predicate argument or perform a
structural check based on params.name) and if validation fails, log an error to
stderr and return params.fallback instead of casting and returning the parsed
value. Ensure the validation is applied inside the try block after JSON.parse
and uses clear identifying messages including params.name when logging.
In `@packages/ui/src/theme/components/openapi/code-sample.tsx`:
- Around line 103-110: The buildUrl function currently only normalizes baseUrl
and path and ignores request parameters, leaving templated path params like
/users/{id} unresolved and omitting query samples; update buildUrl to accept a
parameters argument (e.g., add parameter: readonly parameters?: ParameterType[])
and implement logic to: 1) replace templated path variables in input.path by
finding parameters with in === 'path' and substituting their example or default
values, and 2) append a query string built from parameters with in === 'query'
using their example/default values (URL-encode values and skip undefined). Then
update every callsite to pass props.parameters into buildUrl (the comment-listed
callsites referencing buildUrl at lines 120, 139, 172, 203, 267, 312) so calls
use buildUrl({ baseUrl, path, parameters: props.parameters }). Ensure function
signature and callers use the same ParameterType shape used elsewhere in the
component.
---
Nitpick comments:
In `@packages/ui/src/theme/components/theme-provider.tsx`:
- Around line 171-184: Refactor applyColorOverrides to accept a single object
parameter (e.g. { html, colors }) instead of two positional params to follow the
object-parameter guideline: update the function signature for
applyColorOverrides and adjust the implementation to read html and colors from
the options object (still calling resolveColorPairs and invoking
html.style.setProperty for each pair), then update all call sites that pass
(html, colors) to pass the new object shape instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 397d6080-18cd-4406-9981-7d269b1064c1
📒 Files selected for processing (4)
packages/core/src/sync/sidebar/index.tspackages/ui/src/config.tspackages/ui/src/theme/components/openapi/code-sample.tsxpackages/ui/src/theme/components/theme-provider.tsx
- Replace ternaries with if/else in code-sample, svg-banner, registry - Restructure built-in.ts to use closure-based cache instead of let - Fix utf-8 → utf8 encoding identifier - Fix oxlint-disable comment placement for non-literal fs args Co-Authored-By: Claude <noreply@anthropic.com>
- Broaden file structure rule to cover all exported declarations (not just functions) - Widen clearDotsInterval to handle both browser and Node timer handles - Clean up globalThis property after clearing interval Co-Authored-By: Claude <noreply@anthropic.com>
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
// --- Private ---separator@param,@returns) on all functions;@privatetag on non-exported helperstest/→src/).claude/rules/typescript.mdwith file structure conventionsChanges
File Structure (all packages)
// --- Private ---separator comment@param/@returnstags@privateJSDoc tag on all non-exported helpersTest Colocation
packages/cli/test/→packages/cli/src/lib/packages/config/test/→packages/config/src/packages/core/test/→packages/core/src/packages/templates/test/→packages/templates/src/packages/theme/test/→packages/theme/src/packages/ui/test/→packages/ui/src/Dependency Upgrades
oxlint1.55.0 → 1.56.0oxfmt0.40.0 → 0.41.0@kidd-cli/core0.4.0 → 0.7.0c124.0.0-beta.3 → 4.0.0-beta.4laufen1.1.0 → 1.2.1@iconify-json/material-icon-theme1.2.55 → 1.2.56@iconify-json/simple-icons1.2.73 → 1.2.74Test plan
pnpm checkpasses (typecheck + lint + format)pnpm buildsucceedsSummary by CodeRabbit
New Features
Improvements