Conversation
🦋 Changeset detectedLatest commit: 9825155 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
dependencies
|
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:
WalkthroughThis PR splits Babel transformers into dedicated JS/JSX packages, refactors the config/loader/worker/catalogue/message subsystems (sync config resolution, transformer-aware buckets), updates the Babel/unplugin integration to use transformers, and adds Next.js and TanStack Start examples plus related tooling and docs changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Commands
participant Config as Config (resolveConfig)
participant Bucket as BucketWorker
participant Extract as BucketExtractWorker
participant Compile as BucketCompileWorker
participant Transformer as Transformer (JS/JSX)
participant Formatter as Formatter (PO)
participant FS as File System
CLI->>Config: resolveConfig(name)
Config-->>CLI: Configuration
CLI->>Bucket: buildAll()/scanAll()/compileAll()
Bucket->>Extract: scanAll()
Extract->>FS: glob include patterns
FS-->>Extract: file list
loop per file
Extract->>FS: readFile(path)
FS-->>Extract: content
Extract->>Transformer: extract(content, id)
Transformer-->>Extract: Message[]
end
Extract->>Extract: mergeExtractedMessages()
Extract->>Formatter: stringify(messages, {locale})
Formatter-->>FS: content
Extract->>FS: writeCatalogueMessages(path, content)
Bucket->>Compile: compileAll()
Compile->>FS: readCatalogueMessages(locale)
FS-->>Compile: messages
Compile->>Compile: hydrateTranslations(messages)
Compile->>FS: writeRuntimeTranslations(locale,json)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/config/src/commands/extract.ts (1)
15-21:⚠️ Potential issue | 🟡 MinorVerify Promise.allSettled or add cancellation to prevent partial writes.
Promise.all fails fast on the first bucket rejection, leaving sibling buckets'
scanAll/writeAlloperations running in the background. More critically,writeAll()writes locale files in a loop—ifwriteCatalogueMessagesfails after writing locales 0–2 of 5, those 2 locales remain on disk and only that bucket's remaining work unwinds, leaving inconsistent output.Either use
Promise.allSettledwith aggregated error handling to ensure all buckets complete before handling rejections, or propagate anAbortSignalinto workers so all can cancel on first failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/commands/extract.ts` around lines 15 - 21, The current parallel launch using config.buckets.map and awaiting Promise.all lets one bucket rejection abort the aggregate while sibling BucketExtractWorker instances continue running and may partially write files (scanAll / writeAll / writeCatalogueMessages). Fix by either (A) replacing Promise.all(tasks) with Promise.allSettled(tasks) and then aggregate and throw a combined error if any task.status === "rejected", ensuring all buckets finish before error handling, or (B) add AbortSignal support to BucketExtractWorker (accept a signal in the constructor and make scanAll/writeAll respect it) then create an AbortController, pass controller.signal into each new BucketExtractWorker, and on the first caught rejection call controller.abort() and await Promise.allSettled to let workers stop cleanly; choose one approach and update the calling code and BucketExtractWorker methods (scanAll/writeAll/writeCatalogueMessages) accordingly.
🟡 Minor comments (12)
packages/transform-jsx/src/generator.ts-14-19 (1)
14-19:⚠️ Potential issue | 🟡 MinorAdd validation for JSX identifier constraints in attribute names.
The empty-string and invalid-character concerns on line 17 are valid edge cases:
- Object property keys like
{ "": value }pass an empty string as identifier, which coerces+""to0(notNaN), creating attribute_0instead of being rejected- Object property keys like
{ "foo-bar": value }or{ "foo bar": value }pass invalid JSX identifier characters;t.jsxIdentifier()accepts them at construction time but fails only at code generation/parse timeWhilst JSX attribute names (from
getAttributeNameAsString) are constrained by Babel, object property names extracted viagetPropertyNameAsStringin the upstream parsers are not. Consider validating or sanitising identifiers here to fail fast on invalid input, or confirm that the upstreamMessage.identifiercontract already guarantees valid JavaScript identifiers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transform-jsx/src/generator.ts` around lines 14 - 19, The attributes construction currently uses t.jsxIdentifier with strings from id and children keys without validating JSX identifier rules; update the mapping that builds attributes (the attributes array and the children.map callback) to validate each attribute name (including id and each key k) before calling t.jsxIdentifier—reject or throw a clear error for empty strings or names containing invalid JSX identifier characters (e.g. spaces, dashes, starting with digits) so invalid inputs fail fast; alternatively, if sanitisation is chosen, transform invalid names to a guaranteed-safe form (e.g. prefix with "_" and replace invalid chars) consistently and document that behavior so t.jsxAttribute/t.jsxIdentifier never receive invalid names.packages/config/src/commands/extract.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRemove unnecessary
awaitfrom synchronousresolveConfigcall.The
resolveConfigfunction in~/features/loader/resolve.jsis synchronous (noasynckeyword), but line 11 in extract.ts awaits it:const config = await resolveConfig('saykit');. Although awaiting a non-promise value is valid, it's misleading. Remove theawaitto match the actual function signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/commands/extract.ts` at line 2, The call to resolveConfig in extract.ts is using await even though resolveConfig is synchronous; update the code in the extract command (the line that currently does const config = await resolveConfig('saykit');) to remove the unnecessary await so it reads const config = resolveConfig('saykit'); and keep the existing import of resolveConfig from '~/features/loader/resolve.js'.website/content/getting-started/installation.mdx-21-26 (1)
21-26:⚠️ Potential issue | 🟡 MinorTautological description for
@saykit/babel-plugin."Babel plugin for SayKit" restates the package name without adding information. Consider mentioning what it does (e.g. that it delegates to
@saykit/transform-js/@saykit/transform-jsxto transformsaycalls/tagged templates at build time). Same applies to the@saykit/reactline above — "i18n hooks and components" is quite vague compared to the previous wording that mentioned<Say>and client/server hooks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/content/getting-started/installation.mdx` around lines 21 - 26, Update the tautological package descriptions to explain what each package actually does: change the `@saykit/babel-plugin` line to describe that it delegates build-time transformation of say calls and tagged templates to `@saykit/transform-js` and `@saykit/transform-jsx`, and expand the `@saykit/react` description to mention the provided <Say> component and the client/server hooks (e.g., useSay/useServerSay) rather than just "i18n hooks and components" so readers understand the concrete API and build-time responsibilities.packages/transform-js/src/parser.test.ts-442-448 (1)
442-448:⚠️ Potential issue | 🟡 MinorWeak assertion:
toBeInstanceOf(Object)matches virtually anything.Other tests (Lines 385, 401) correctly assert
CompositeMessage. Tighten this one for parity and to actually exercise the type:✏️ Proposed fix
expect(result).not.toBeNull(); - expect(result).toBeInstanceOf(Object); // CompositeMessage + expect(result).toBeInstanceOf(CompositeMessage); if (result) { expect(result.children).toHaveLength(3);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transform-js/src/parser.test.ts` around lines 442 - 448, The test currently uses a weak assertion expect(result).toBeInstanceOf(Object); — replace that with a specific assertion for CompositeMessage so the test actually verifies the parsed type: assert expect(result).toBeInstanceOf(CompositeMessage) (keeping the existing checks for children, ArgumentMessage and AUTO_INCREMENT_IDENTIFIER) so the block that references result, CompositeMessage, ArgumentMessage and AUTO_INCREMENT_IDENTIFIER exercises the precise type rather than a generic Object.examples/tanstack-start/src/routes/__root.tsx-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorAdd a
langattribute to<html>.Since this example demonstrates SayKit i18n, the root document should set
langon<html>(ideally reflecting the active locale from the route) for accessibility and SEO. A static fallback is better than nothing.Proposed change
- <html> + <html lang="en">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-start/src/routes/__root.tsx` at line 34, The root document's <html> element is missing a lang attribute; update the render in routes/__root.tsx to set <html lang={locale || 'en'}> (or similar) so it reflects the active locale from the route with a static fallback. Retrieve the active locale from the route loader data or SayKit i18n hook (e.g., useLoaderData().locale or your i18n getActiveLocale()/useTranslation() value) and use that value when rendering the <html> tag in the root document component.packages/config/src/features/catalogue/extractor.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorAvoid mutating global
Error.stackTraceLimiton every extraction.Setting
Error.stackTraceLimit = Infinityis a process-wide side effect invoked for each file processed. This can bloat memory for unrelated errors thrown elsewhere in the host process (CLI, bundler dev servers) and is also redundant to set per call. If truly needed for transformer debug traces, set it once at module init or guard behind a debug flag; otherwise remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/catalogue/extractor.ts` at line 9, Remove the per-call mutation of Error.stackTraceLimit in the extractor module: do not set Error.stackTraceLimit = Infinity inside the extraction code path. Either delete that line entirely or move it to module initialization (top-level once) or guard it behind a debug flag so it only runs once (e.g., process.env.DEBUG_TRANSFORM). Target the statement referencing Error.stackTraceLimit in the extractor module and implement one of these options to avoid a process-wide side effect on every extraction.packages/config/src/features/catalogue/extractor.ts-6-7 (1)
6-7:⚠️ Potential issue | 🟡 MinorSilently swallowing read errors can mask real failures.
Treating every
readFilefailure (EACCES, EISDIR, etc.) as "empty file" means extraction will silently skip files the user expected to be processed. Consider logging or only swallowingENOENT.Proposed fix
- const content = await readFile(path, 'utf8').catch(() => ''); - if (!content) return []; + let content: string; + try { + content = await readFile(path, 'utf8'); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + return []; + } + if (!content) return [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/catalogue/extractor.ts` around lines 6 - 7, The current readFile call that assigns to content and swallows all errors (const content = await readFile(path, 'utf8').catch(() => '')) should be changed to only treat missing files as empty and surface or log other failures: replace the blanket .catch with a try/catch (or inspect the caught error) so that if error.code === 'ENOENT' you return '' and continue, but for other errors (EACCES, EISDIR, etc.) either rethrow the error or log it via the module's logger before throwing; update the logic around the content variable and the early return (if (!content) return []) accordingly so only ENOENT leads to skipping and other read failures are not silently swallowed.packages/transform-js/src/generator.ts-14-17 (1)
14-17:⚠️ Potential issue | 🟡 MinorReplace
t.identifier(ident)witht.stringLiteral(ident)for dynamic property keys.The
identvalues fromgenerateChildExpressionsinclude numeric strings (e.g., "0", "1", "2") which are not valid JavaScript identifiers. Usingt.identifier()with numeric strings produces invalid Babel AST nodes. Uset.stringLiteral(ident)instead, which correctly handles all string values including numeric ones. The existing test patterns (e.g.,t.stringLiteral('1'),t.stringLiteral('one')) confirm this approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transform-js/src/generator.ts` around lines 14 - 17, The object property keys created in the properties expression use t.identifier(ident), but ident values from generateChildExpressions can be numeric strings (e.g., "0", "1") which produce invalid AST nodes; replace t.identifier(ident) with t.stringLiteral(ident) in the t.objectProperty calls inside the properties construction (the const properties = t.objectExpression([...]) that spreads children.map(([ident, expr]) => t.objectProperty(...))) so dynamic keys are always emitted as string literals.packages/config/src/features/loader/resolve.ts-6-12 (1)
6-12:⚠️ Potential issue | 🟡 Minor
unwrapNamedConfighardcodes'saykit'while the caller suppliesname.
resolveConfigtakes anameparameter and uses it to discover<name>.config.*, butunwrapNamedConfigonly unwrapsconfig.saykit. If anyone callsresolveConfig('foo')the named-config branch is silently ignored forconfig.foowrappers, which is inconsistent with the file lookup. Either threadnamethrough or document that named wrappers are fixed tosaykit.Proposed fix
-function unwrapNamedConfig(config: object) { - if (!('saykit' in config) || !config.saykit) return config; - return config.saykit; -} +function unwrapNamedConfig(config: object, name: string) { + if (!(name in config) || !(config as Record<string, unknown>)[name]) return config; + return (config as Record<string, unknown>)[name] as object; +} @@ - config = unwrapNamedConfig(config); + config = unwrapNamedConfig(config, name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/loader/resolve.ts` around lines 6 - 12, unwrapNamedConfig currently checks for the literal 'saykit' key while resolveConfig accepts a name parameter, causing wrappers like config.foo to be ignored; modify unwrapNamedConfig to accept the name (e.g., unwrapNamedConfig(config: object, name: string)) or otherwise look up the dynamic key (config[name]) and return that when present, then update resolveConfig to call unwrapNamedConfig(fileContent, name) so the unwrapping matches the discovered config file name.packages/config/src/features/workers/extract-worker.ts-23-23 (1)
23-23:⚠️ Potential issue | 🟡 MinorMinor: log string typo.
Found N messages(s)should beFound N message(s).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/workers/extract-worker.ts` at line 23, Update the log message in extract-worker.ts where this.logger.step is called with messages.length and relativePath: change the string from "Found ${messages.length} messages(s) in ${relativePath}" to "Found ${messages.length} message(s) in ${relativePath)" so the pluralization reads "message(s)"; ensure the template uses the same variables (messages.length and relativePath) and preserves surrounding formatting in the this.logger.step call.examples/nextjs/package.json-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorRemove or adjust
"sideEffects": falsefrom this Next.js example package.Whilst this is a private example (not published), marking application packages as side-effect-free can interfere with bundler behaviour and isn't recommended for Next.js apps. The codebase already has a side-effect import (
'server-only'), which could be problematic ifsideEffectsis strictly enforced. For an example application, consider removing this flag entirely, or if retained, narrow it to an allow-list (e.g.["*.css", "*.scss"]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/nextjs/package.json` at line 5, The package.json currently sets "sideEffects": false which can break Next.js bundling due to known side-effectful imports like the 'server-only' module; remove the "sideEffects" key entirely from this example package.json or replace it with a narrow allow-list (for example ["*.css","*.scss"]) so that known legitimate side-effect assets are preserved while avoiding a global false that could tree-shake required modules.packages/config/src/shapes.ts-14-23 (1)
14-23:⚠️ Potential issue | 🟡 MinorConsider adding format validation to the
extensionfield in theFormatterschemaThe
extensionfield now accepts any string viaz.string(). However, downstream code inpackages/config/src/features/catalogue/path.tsassumes the extension starts with a dot (.slice(1)removes the first character) when constructing file paths. Without validation, invalid inputs like"json","json.", or an empty string could silently create broken file paths. A minimal regex validator (e.g.,/^\.[a-zA-Z0-9]+$/) would catch these typos at config-load time instead of at file I/O.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/shapes.ts` around lines 14 - 23, The Formatter schema's extension currently accepts any string; update the extension validator in the Formatter z.object to enforce it starts with a dot and contains alphanumeric chars (e.g. use z.string().regex(/^\.[a-zA-Z0-9]+$/) with a clear error message) so downstream code that uses extension.slice(1) (see Formatter type and usage in catalogue path logic) cannot receive values like "json" or "" that would break path construction.
🧹 Nitpick comments (22)
packages/config/src/features/messages/identifier.ts (1)
11-25: Readability: add braces to nestedif/forblocksThe chained
if+ body-lessif+forwithout braces is easy to misread (and brittle if a maintainer later adds a statement). Consider using braces and/or an early-return style; behaviour is the same but intent becomes clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/messages/identifier.ts` around lines 11 - 25, The nested body-less conditionals in assignSequenceIdentifiers make the control flow hard to read and fragile; update assignSequenceIdentifiers to use explicit braces (or an early-return pattern) for each if and for block so each conditional and loop has a clear, block-scoped body—specifically wrap the checks that use Message instanceof ArgumentMessage | ElementMessage | ChoiceMessage and the AUTO_INCREMENT_IDENTIFIER assignment, the loop over message.children when Message instanceof CompositeMessage | ElementMessage, and the loop over message.branches for ChoiceMessage in explicit { } blocks to preserve existing behavior while improving readability and safety.website/content/getting-started/introduction.mdx (1)
29-33: Commented-out JSX left in docsIf the "Framework Agnostic" card is being intentionally dropped, prefer removing the block entirely rather than leaving commented-out JSX in published docs. If it is temporary, a short
{/* TODO: ... */}note explaining why/when it'll return would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/content/getting-started/introduction.mdx` around lines 29 - 33, Remove the commented-out JSX block for the "Framework Agnostic" Card (the Card component instance and Puzzle icon usage) from the MDX file; if the omission is temporary, replace the commented JSX with a brief JSX comment like {/* TODO: re-add Framework Agnostic card when X is implemented */} so maintainers know why it was removed—locate the block that contains the Card component and the Puzzle icon and either delete it or swap it for the single-line TODO comment.packages/plugin-babel/tsdown.config.ts (1)
4-4: Remove redundant entry from glob pattern
src/*.tsalready matchessrc/index.ts, making the explicit entry unnecessary. Simplify to just['src/*.ts']to avoid duplication.Proposed simplification
- entry: ['src/index.ts', 'src/*.ts'], + entry: ['src/*.ts'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-babel/tsdown.config.ts` at line 4, The entry array in tsdown.config.ts currently lists both 'src/index.ts' and the glob 'src/*.ts', causing duplication; update the entry property (entry) to only include the glob 'src/*.ts' so the explicit 'src/index.ts' is removed and the build/input list is simplified.packages/transform-jsx/tsconfig.json (1)
1-1: Consider mirroring~/*alias for consistency.Sibling packages (
packages/config,packages/integration-carbon,packages/integration-react) all addcompilerOptions.paths: { "~/*": ["./src/*"] }in this PR. Ifpackages/transform-jsx/srcdoesn't rely on the alias this is fine, but dropping it asymmetrically is easy to trip over later. Happy to ignore if intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transform-jsx/tsconfig.json` at line 1, The package's tsconfig.json currently only extends "../../tsconfig.base.json" and should add the same "~/*" path alias used by sibling packages; update tsconfig.json to include a compilerOptions.paths entry: "compilerOptions": { "paths": { "~/*": ["./src/*"] } } (keeping the existing extends), so imports can use the "~/*" alias consistently with packages like packages/config and packages/integration-react.examples/tanstack-start/.gitignore (1)
3-3: Optional: narrow the generated-locale pattern.
src/locales/*/*.jsonwill ignore any JSON placed directly under a locale directory. If hand-written catalogs or schema files ever land there, they'd silently be untracked. If the generator outputs a known filename (e.g.messages.json), pinning the pattern (src/locales/*/messages.json) removes the ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-start/.gitignore` at line 3, Replace the broad generated-locale ignore pattern so handwritten JSON files aren't unintentionally ignored: change the pattern "src/locales/*/*.json" to the specific generated filename (for example "src/locales/*/messages.json") in .gitignore, or add that specific pattern alongside the existing one if you need to preserve the broader ignore for other generated files.packages/config/src/features/messages/hash.test.ts (1)
19-19: Consider using.not.toBe()for clearer intent.
expect(withContext === withoutContext).toBeFalsy()works but obscures the assertion and loses Vitest's diff output on failure.expect(withContext).not.toBe(withoutContext)is more idiomatic and produces better failure messages. Same applies to lines 27–28 (use.toBe(withoutContext)).♻️ Proposed refactor
- expect(withContext === withoutContext).toBeFalsy(); + expect(withContext).not.toBe(withoutContext);- expect(withUndefinedContext === withoutContext).toBeTruthy(); - expect(withEmptyStringContext === withoutContext).toBeTruthy(); + expect(withUndefinedContext).toBe(withoutContext); + expect(withEmptyStringContext).toBe(withoutContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/messages/hash.test.ts` at line 19, Replace the boolean equality assertions that hide Vitest diffs with direct object comparisons: change expect(withContext === withoutContext).toBeFalsy() to expect(withContext).not.toBe(withoutContext), and similarly update the other assertions referenced (the ones that currently use a boolean equality like expect(... === ...).toBeTruthy()/toBeFalsy()) to use expect(left).toBe(right) or expect(left).not.toBe(right) as appropriate so Vitest can show proper diff output; locate these in the test that uses the withContext and withoutContext variables in hash.test.ts and update those expect calls accordingly.packages/config/src/commands/build.ts (1)
13-13: Remove redundantawaiton synchronousresolveConfig()calls.The
resolveConfig()function is synchronous (returnsConfigurationdirectly, not aPromise), making theawaitkeywords in build.ts (line 13), compile.ts (line 11), and extract.ts (line 11) redundant. While harmless, they're misleading to readers. Remove theawaitkeywords from these three call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/commands/build.ts` at line 13, The call sites are using an unnecessary await on the synchronous resolveConfig() function; remove the await so you assign the returned Configuration directly (e.g., change "const config = await resolveConfig('saykit');" to "const config = resolveConfig('saykit');") in build.ts and do the analogous removal in the compile.ts and extract.ts calls that invoke resolveConfig(); no other behavior changes are required.packages/config/src/features/catalogue/path.ts (1)
7-12: Guard against non-dot-prefixed extensions or document the invariant.The code at line 11 assumes
extensionalways starts with a dot (e.g..po→po), stripping it with.slice(1). Currently, the sole formatter (@saykit/format-po) does provide.po, but the Formatter schema (shapes.ts:15) allows any string without enforcing the dot prefix. If a future formatter providespoinstead,.slice(1)silently producesowith no error.Either guard the call or document that formatters must provide dot-prefixed extensions:
Optional guard
- extension = bucket.formatter.extension, -) { + extension = bucket.formatter.extension, +): string { + const ext = extension.startsWith('.') ? extension.slice(1) : extension; const outputMessageTemplate = bucket.output .replaceAll('{locale}', locale) - .replaceAll('{extension}', extension.slice(1)); + .replaceAll('{extension}', ext); return resolve(outputMessageTemplate); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/catalogue/path.ts` around lines 7 - 12, The code assumes bucket.formatter.extension has a leading dot and uses extension.slice(1), which breaks for values like "po"; update the replacement to normalize the extension first: compute a safe value from extension (e.g., if extension startsWith('.') use extension.slice(1) else use extension unchanged) and use that safe value when building outputMessageTemplate (referencing bucket.formatter.extension, the local variable extension, and outputMessageTemplate/resolve); alternatively add a runtime validation that throws if extension does not start with '.' so callers must comply—pick one approach and apply it where the replacement is performed.examples/tanstack-start/src/i18n.ts (1)
3-9: Refactor locale loading to avoid blocking both locales on module init.The top-level
await import(...)forces both locale JSON files to load synchronously before the module finishes evaluating. In TanStack Start's SSR environment, this blocks the server from rendering the route until both locales resolve—even though the parameterised route (/{-$locale}) only needs the active locale at runtime. Consider lazy-loading the locale catalogs inside the route handler based on the matched locale, rather than awaiting both at module initialisation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-start/src/i18n.ts` around lines 3 - 9, The current top-level creation of the Say instance (const say) uses await import(...) for both locales which blocks module initialization; change to lazy-load locale catalogs at runtime instead: remove the top-level await imports from the messages object in the Say constructor and instead export a loader function (e.g., loadLocale or getMessages) that dynamically imports the needed './locales/{locale}/messages.json' inside your route handler for the matched locale and then initializes or updates Say with that single locale's messages (or pass messages into Say.create/instance method) so only the active locale is loaded during request handling.examples/nextjs/src/proxy.ts (1)
5-6: Locale list duplicated fromsaykit.config.ts.This duplication is flagged by the existing TODO and is fine for an example, but it's worth noting that the two lists must be kept in sync manually. A thin shared module (even within the example) would prevent drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/nextjs/src/proxy.ts` around lines 5 - 6, The SOURCE_LOCALE and LOCALES arrays are duplicated; create a thin shared module (e.g., export default or named exports like SOURCE_LOCALE and LOCALES from a new module) that holds the canonical locale constants, update examples/nextjs/src/proxy.ts to import SOURCE_LOCALE and LOCALES from that shared module instead of defining them locally, and update saykit.config.ts to import the same symbols from the shared module so both files reference the single source of truth (ensure names match the existing symbols SOURCE_LOCALE and LOCALES).packages/config/src/features/catalogue/storage.ts (1)
11-12: Catch is broad and may mask real I/O errors.
readFile(...).catch(() => '')treats every failure — permissions, EISDIR, transient errors — as “empty file”, which can silently hide bugs during extract/compile. Consider narrowing toENOENT:Proposed refinement
- const content = await readFile(path, 'utf8').catch(() => ''); - if (!content) return []; + const content = await readFile(path, 'utf8').catch((err) => { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return ''; + throw err; + }); + if (!content) return [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/catalogue/storage.ts` around lines 11 - 12, The current readFile call in storage.ts silences all errors by doing readFile(path, 'utf8').catch(() => '') which can hide real I/O problems; change this to catch only a missing-file error (ENOENT) and rethrow other errors — e.g., wrap the await readFile(path, 'utf8') in a try/catch, return '' / [] when (err as NodeJS.ErrnoException).code === 'ENOENT', and rethrow/log otherwise so that other permission or I/O errors are not masked (refer to the readFile usage, the content variable, and the subsequent if (!content) return [] logic).packages/plugin-babel/src/index.ts (1)
6-6: Address the TODO or open a tracking issue.The
transformSourcehelper is generic enough that unplugin/webpack/vite integrations will want it too — if it stays here it will be duplicated when those adapters land.Happy to move it into a shared module (e.g.
@saykit/config/features/transform) and open an issue to track — want me to draft that?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-babel/src/index.ts` at line 6, The TODO asks to avoid duplicating the generic transformSource helper; move the transformSource function out of packages/plugin-babel/src/index.ts into a shared module (suggested name: `@saykit/config/features/transform`) and update imports in plugin-babel to import transformSource from that shared module, or if you prefer not to move now open a short tracking issue referenced in the repo’s issue tracker that records “Move transformSource to shared feature module” and link it in this TODO; ensure the exported symbol name remains transformSource so unplugin/webpack/vite adapters can consume it without changes.packages/config/src/features/loader/resolve.ts (1)
11-26: Consider memoising the resolved config.Now that
resolveConfigis synchronous and called directly from the Babel plugin’sProgramvisitor (seepackages/plugin-babel/src/index.ts), it will re-runexistsSync/readFileSync/safeParseon every transformed file. Since config is effectively immutable during a build, memoising by(name, projectDir)would remove the repeated disk hit without changing semantics for callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/loader/resolve.ts` around lines 11 - 26, resolveConfig currently re-reads and re-parses the config on every call; memoise results by caching per (name, projectDir) to avoid repeated disk/io and parsing. Add a module-level Map cache keyed by e.g. `${name}::${projectDir}` (use process.cwd() or accept a second optional projectDir param) and return the cached parsed result at the top of resolveConfig; only run findConfigFile, configLoaders load, unwrapNamedConfig and Configuration.safeParse when there is no cache entry, then store result.data in the cache before returning. Ensure the cache key references the same project dir used by findConfigFile (process.cwd() or provided arg) and keep behavior and error throwing unchanged.packages/plugin-babel/package.json (1)
50-50: Remove the deprecated@types/babel__parserstub package.
@types/babel__parseris a deprecated stub —@babel/parserships its own TypeScript definitions as of recent versions, making the stub unnecessary. The npm package page for@types/babel__parserexplicitly indicates it is no longer needed.Proposed fix
- "devDependencies": { - "@types/babel__parser": "^7.1.5" - } + "devDependencies": {}(or remove the
devDependenciesblock entirely if it becomes empty)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-babel/package.json` at line 50, Remove the deprecated devDependency entry "@types/babel__parser" from package.json because `@babel/parser` now ships its own TypeScript types; open packages/plugin-babel/package.json, delete the "@types/babel__parser" line from the devDependencies object (or remove the entire devDependencies block if it becomes empty), and run npm/yarn install to update lockfiles.packages/config/src/features/loader/module.ts (3)
45-46:jsandtsare identical — consider collapsing.Both loaders delegate to
transpileAndRequirewith identical behaviour. Either keep two exports for future divergence (document the intent) or export a singletranspileAndRequireand reference it directly inconfigLoaders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/loader/module.ts` around lines 45 - 46, The js and ts loaders both just call transpileAndRequire; replace the duplicate exports by exporting transpileAndRequire as the single loader and update any usages (e.g. in configLoaders) to reference transpileAndRequire directly, or if you want to keep the two names for future divergence, make js and ts simple aliases to transpileAndRequire and add a brief comment documenting intent; ensure symbols js, ts, transpileAndRequire and configLoaders are updated consistently.
40-42: UsermSync(..., { force: true })and drop the existence check.The
ts.sys.fileExistscheck followed byrmSynchas a minor TOCTOU window and also requireststo have been successfully required. UsingrmSync(outputPath, { force: true })is race-free and handles the "file never created" case (e.g.writeFileSyncthrew) without an existence probe.♻️ Proposed change
- } finally { - if (ts.sys.fileExists(outputPath)) rmSync(outputPath); - } + } finally { + rmSync(outputPath, { force: true }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/loader/module.ts` around lines 40 - 42, The finally block currently checks ts.sys.fileExists(outputPath) before calling rmSync, which creates a TOCTOU window and depends on ts being available; replace that pattern by calling rmSync(outputPath, { force: true }) unconditionally to atomically handle missing files and avoid the existence probe—update the finally block that references ts.sys.fileExists, rmSync, and outputPath accordingly.
10-10: Write temp file toos.tmpdir()rather than alongside the user's config.Placing
${path}.${process.pid}.saykit.config.cjsnext to the user's source file has two drawbacks:
- It dirties the project directory (even transiently) and can trip the recursive
watchDebounced('.', …)used byBucketBuildWorker.watch(), producing spurious fs events on every config load.- If the process is killed (SIGKILL) between write and
finally, the stale artefact is left in the user's repo.Prefer
path.join(os.tmpdir(), ...)with a unique name (e.g.crypto.randomUUID()plus PID) so temp artefacts are scoped to the OS temp dir and resistant to PID collisions across concurrent saykit invocations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/loader/module.ts` at line 10, The current temp file name assigned to outputPath (const outputPath = `${path}.${process.pid}.saykit.config.cjs`) writes next to the user's config; change it to write into the OS temp directory using path.join(os.tmpdir(), ...) and a unique filename (e.g. include crypto.randomUUID() and process.pid) to avoid repo pollution and collisions; update module.ts to import os (and crypto if not present) and construct outputPath using path.join(os.tmpdir(), `${crypto.randomUUID()}.${process.pid}.saykit.config.cjs`) while preserving existing cleanup/finally logic that deletes outputPath.packages/config/src/features/workers/build-worker.ts (1)
26-40: Consider guarding the watch loop against transient errors.An exception from
extract.updatePath(...)orcompile.compileAll()will propagate out of thefor awaitloop and terminate watch mode entirely. For a better developer experience, consider wrapping the body in a try/catch so a single bad save (e.g. syntax error mid-edit) logs and continues rather than killing the watcher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/workers/build-worker.ts` around lines 26 - 40, The watch loop currently lets exceptions from extract.updatePath or compile.compileAll escape and kill the watcher; wrap the loop body inside watch() (the for await over watchDebounced) in a try/catch so any thrown error from this.extract.updatePath(...) or this.compile.compileAll() is caught, logged via this.logger.error with contextual info (include normalisePathForLogs(filePath) and the error), and then continue to the next event instead of rethrowing; keep existing logging for successful changes and only suppress the exception propagation so the watcher stays running.packages/config/src/features/runtime/translations.ts (1)
14-20: Drop the underscore prefix onbucket— the parameter is actually used.The
_prefix conventionally signals an unused parameter, but_bucketis forwarded intoapplyFallbackTranslationson line 20. Consider renaming it back tobucketto match the intent and the downstream parameter name.♻️ Proposed rename
- _bucket: Bucket, + bucket: Bucket, locale: string, messages: Message[], ) { if (cache.has(locale)) return cache.get(locale)!; - const translations = await applyFallbackTranslations(cache, config, _bucket, locale, messages); + const translations = await applyFallbackTranslations(cache, config, bucket, locale, messages);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/runtime/translations.ts` around lines 14 - 20, The parameter named `_bucket` is actually used (it's passed to applyFallbackTranslations), so rename `_bucket` to `bucket` in the function signature and update all references in that function (including the call to applyFallbackTranslations) to use `bucket`; ensure any related parameter destructuring, type annotations (Bucket), and local usages that referenced `_bucket` are updated to `bucket` so names remain consistent with downstream code.packages/transform-js/src/index.ts (1)
40-84: Minor:extractandtransformmutate the parsed AST.On lines 46 and 72,
path.node.leadingComments = path.node.leadingComments ?? []mutates each visited expression node.parseExpressionpresumably expects a non-undefinedleadingCommentsarray; if that's the only reason for this assignment, consider doing the??dereference insideparseExpressionto keep the AST immutable and make this transformer safely re-usable on an already-parsed program.Not blocking — just flagging since the AST coming out of
createProgramwill look subtly different from a vanilla@babel/parserAST.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transform-js/src/index.ts` around lines 40 - 84, Both extract and transform currently mutate the AST by assigning path.node.leadingComments = path.node.leadingComments ?? []; instead move the defensive handling into parseExpression so callers (extract and transform) no longer change nodes: revert/remove the assignments in the traverse callbacks in extract and transform, and update parseExpression to treat undefined leadingComments as an empty array (e.g., read node.leadingComments ?? [] inside parseExpression) so the AST returned from createProgram remains unchanged while parseExpression still works when leadingComments is missing.packages/transform-js/src/parser.ts (1)
150-156: Drop the misleadingas stringcast.
getExpressionAsKeyreturnsstring | typeof AUTO_INCREMENT_IDENTIFIER, andArgumentMessage.identifieralready accepts that union (perpackages/config/src/features/messages/types.ts), so theas stringcast on line 152 is both unnecessary and actively lies about the runtime value when the expression is neither anIdentifiernor aJSXIdentifier(wherekeywill beAUTO_INCREMENT_IDENTIFIER).♻️ Proposed cleanup
- } else if (fallback) { - const key = getExpressionAsKey(expression); - return new ArgumentMessage(key as string, expression); - } else { + } else if (fallback) { + return new ArgumentMessage(getExpressionAsKey(expression), expression); + } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transform-js/src/parser.ts` around lines 150 - 156, The code casts the result of getExpressionAsKey(expression) to string before constructing new ArgumentMessage, which is misleading because getExpressionAsKey returns string | typeof AUTO_INCREMENT_IDENTIFIER and ArgumentMessage.identifier already accepts that union; remove the unnecessary "as string" cast and pass key directly to new ArgumentMessage(key, expression) so the runtime value (including AUTO_INCREMENT_IDENTIFIER) is preserved; update any affected type annotations if the TypeScript compiler flags anything, but primarily change the call site in the block that constructs ArgumentMessage to use the uncast key variable.packages/config/src/features/messages/types.ts (1)
1-72: Decoupling from@babel/typesLGTM; one small refactor worth considering.Swapping
t.Expressionforanyonexpression/accessoris the right call now that transformers live outside@saykit/config, and theAUTO_INCREMENT_IDENTIFIERunion is propagated consistently acrossArgumentMessage,ElementMessage,ChoiceMessage, and per-branchidentifier.Optional nit: centralising
toHashStringwiththis instanceof CompositeMessage(line 11) works but makesBaseaware of a subclass. A cleaner pattern would be a protected/virtualgetHashContext()method thatCompositeMessageoverrides to returndescriptor.context, lettingBasestay agnostic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/messages/types.ts` around lines 1 - 72, The Base class's toHashString currently checks "this instanceof CompositeMessage" making Base depend on a subclass; refactor by adding a protected method like getHashContext(): string | undefined on Base (default returns undefined) and override it in CompositeMessage to return this.descriptor.context, then change Base.toHashString to call this.getHashContext() instead of doing the instanceof check; update signatures for Base, CompositeMessage, and toHashString accordingly (refer to Base.toHashString and CompositeMessage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40170f56-bb85-443d-a3b9-e6adce95ec6d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (137)
.changeset/config.json.changeset/silent-moose-wish.md.oxfmtrc.jsonexamples/carbon/.dev.vars.exampleexamples/carbon/.gitignoreexamples/carbon/package.jsonexamples/carbon/saykit.config.tsexamples/carbon/src/commands/about.tsexamples/carbon/src/commands/maths.tsexamples/carbon/src/commands/ping.tsexamples/carbon/src/commands/roll.tsexamples/carbon/src/entry.tsexamples/carbon/src/i18n.tsexamples/carbon/src/index.tsexamples/carbon/src/locales/en/messages.poexamples/carbon/src/locales/fr/messages.poexamples/carbon/tsconfig.jsonexamples/carbon/tsdown.config.tsexamples/carbon/wrangler.tomlexamples/nextjs-babel/saykit.config.tsexamples/nextjs/.babelrcexamples/nextjs/.gitignoreexamples/nextjs/next-env.d.tsexamples/nextjs/package.jsonexamples/nextjs/saykit.config.tsexamples/nextjs/src/app/[locale]/client-component.tsxexamples/nextjs/src/app/[locale]/layout.tsxexamples/nextjs/src/app/[locale]/page.tsxexamples/nextjs/src/app/[locale]/server-component.tsxexamples/nextjs/src/i18n.tsexamples/nextjs/src/locales/en/messages.poexamples/nextjs/src/locales/fr/messages.poexamples/nextjs/src/proxy.tsexamples/nextjs/tsconfig.jsonexamples/tanstack-start/.gitignoreexamples/tanstack-start/package.jsonexamples/tanstack-start/saykit.config.tsexamples/tanstack-start/src/i18n.tsexamples/tanstack-start/src/locales/en/messages.poexamples/tanstack-start/src/locales/fr/messages.poexamples/tanstack-start/src/routeTree.gen.tsexamples/tanstack-start/src/router.tsxexamples/tanstack-start/src/routes/__root.tsxexamples/tanstack-start/src/routes/{-$locale}/index.tsxexamples/tanstack-start/src/routes/{-$locale}/route.tsxexamples/tanstack-start/tsconfig.jsonexamples/tanstack-start/vite.config.tspackage.jsonpackages/config/package.jsonpackages/config/src/commands/build.tspackages/config/src/commands/compile.tspackages/config/src/commands/extract.tspackages/config/src/define.tspackages/config/src/features/catalogue/extractor.tspackages/config/src/features/catalogue/merge.tspackages/config/src/features/catalogue/path.tspackages/config/src/features/catalogue/storage.tspackages/config/src/features/extract.tspackages/config/src/features/loader/explorer.tspackages/config/src/features/loader/files.tspackages/config/src/features/loader/index.tspackages/config/src/features/loader/loaders.tspackages/config/src/features/loader/module.tspackages/config/src/features/loader/resolve.tspackages/config/src/features/messages/convert.test.tspackages/config/src/features/messages/convert.tspackages/config/src/features/messages/hash.test.tspackages/config/src/features/messages/hash.tspackages/config/src/features/messages/identifier.tspackages/config/src/features/messages/index.tspackages/config/src/features/messages/types.tspackages/config/src/features/runtime/translations.tspackages/config/src/features/worker.tspackages/config/src/features/workers/build-worker.tspackages/config/src/features/workers/compile-worker.tspackages/config/src/features/workers/extract-worker.tspackages/config/src/features/workers/index.tspackages/config/src/features/workers/shared.tspackages/config/src/index.tspackages/config/src/shapes.tspackages/config/tsconfig.jsonpackages/config/tsdown.config.tspackages/config/vitest.config.tspackages/format-po/src/formatter.tspackages/integration-carbon/package.jsonpackages/integration-carbon/tsconfig.jsonpackages/integration-react/package.jsonpackages/integration-react/src/components/renderer.tspackages/integration-react/tsconfig.jsonpackages/plugin-babel/package.jsonpackages/plugin-babel/src/core/context.tspackages/plugin-babel/src/core/index.tspackages/plugin-babel/src/core/messages/convert.test.tspackages/plugin-babel/src/core/program.test.tspackages/plugin-babel/src/core/program.tspackages/plugin-babel/src/core/visitor.test.tspackages/plugin-babel/src/core/visitor.tspackages/plugin-babel/src/features/jsx/generator.tspackages/plugin-babel/src/index.tspackages/plugin-babel/src/play.tspackages/plugin-babel/tsdown.config.tspackages/plugin-unplugin/package.jsonpackages/plugin-unplugin/src/bun.tspackages/plugin-unplugin/src/esbuild.tspackages/plugin-unplugin/src/farm.tspackages/plugin-unplugin/src/index.tspackages/plugin-unplugin/src/rolldown.tspackages/plugin-unplugin/src/rollup.tspackages/plugin-unplugin/src/rspack.tspackages/plugin-unplugin/src/unloader.tspackages/plugin-unplugin/src/vite.tspackages/plugin-unplugin/src/webpack.tspackages/plugin-unplugin/tsconfig.jsonpackages/plugin-unplugin/tsdown.config.tspackages/transform-js/package.jsonpackages/transform-js/src/generator.tspackages/transform-js/src/index.tspackages/transform-js/src/parser.test.tspackages/transform-js/src/parser.tspackages/transform-js/tsconfig.jsonpackages/transform-js/tsdown.config.tspackages/transform-js/vitest.config.tspackages/transform-jsx/package.jsonpackages/transform-jsx/src/generator.tspackages/transform-jsx/src/index.tspackages/transform-jsx/src/parser.test.tspackages/transform-jsx/src/parser.tspackages/transform-jsx/tsconfig.jsonpackages/transform-jsx/tsdown.config.tspackages/transform-jsx/vitest.config.tspackages/unplugin/src/index.tstsconfig.base.jsonwebsite/content/core-concepts/configuration.mdxwebsite/content/core-concepts/runtime.mdxwebsite/content/getting-started/installation.mdxwebsite/content/getting-started/introduction.mdxwebsite/package.json
💤 Files with no reviewable changes (16)
- packages/config/vitest.config.ts
- packages/config/src/define.ts
- packages/unplugin/src/index.ts
- packages/plugin-babel/src/features/jsx/generator.ts
- packages/config/src/features/loader/explorer.ts
- packages/plugin-babel/src/core/index.ts
- packages/plugin-babel/src/core/messages/convert.test.ts
- packages/plugin-babel/src/core/context.ts
- packages/plugin-babel/src/core/program.test.ts
- packages/config/src/features/extract.ts
- packages/plugin-babel/src/core/visitor.test.ts
- packages/plugin-babel/src/core/program.ts
- packages/plugin-babel/src/core/visitor.ts
- examples/nextjs-babel/saykit.config.ts
- packages/config/src/features/loader/loaders.ts
- packages/config/src/features/worker.ts
…sequence assignment
…streamline code and improve readability
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/plugin-babel/src/index.ts (1)
21-25:⚠️ Potential issue | 🔴 CriticalStill unresolved:
path.replaceWith(ast.program)onProgramis unsupported and will throw.This mirrors the earlier critical review comment, which does not appear to have been addressed.
@babel/traversedoes not support replacing aProgrampath with anotherProgramnode — the root has noparentPath, andreplaceWithinternally delegates to operations that require one. See babel/babel#1436. Mutatepath.node.body/path.node.directivesdirectly instead:🐛 Proposed fix
if (transformed !== code) { const ast = parse(transformed, state.file.opts.parserOpts as any); - path.replaceWith(ast.program); - path.skip(); + path.node.body = ast.program.body; + path.node.directives = ast.program.directives; + path.skip(); }Secondary concerns that remain from the earlier review:
state.file.opts.parserOptsis@babel/core's shape (ParserOptionsmerged with plugin arrays as strings/tuples), not@babel/parser's publicParserOptions. Theas anyhides a real type mismatch, and if the original Babel config didn't enable the JSX/TS plugin that the transformer emitted,parse()will throw mid-build.- Transforming
state.file.code(the pristine source) discards any AST edits made by earlier plugins in the pipeline. If downstream users combine@saykit/plugin-babelwith other Babel plugins, those edits are silently lost. Either generate source frompathfirst, or document/enforce this plugin's ordering requirement.`@babel/traverse` NodePath replaceWith Program root node supported 2026🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-babel/src/index.ts` around lines 21 - 25, Replace the unsupported Program-level replaceWith: instead of calling path.replaceWith(ast.program), mutate the existing Program node by assigning path.node.body = ast.program.body and path.node.directives = ast.program.directives (and copy any other top-level properties you need) so the root keeps its parent context; when re-parsing, avoid using state.file.code (which discards prior plugin edits) — generate source from the current path (e.g., use `@babel/generator` to get generated = generate(path.node).code) and parse that generated code with proper parser options (do not use state.file.opts.parserOpts as any; ensure the parser plugins like jsx/ts are enabled or map the `@babel/core` ParserOptions to `@babel/parser` options) before applying the body/directives swap.
🧹 Nitpick comments (2)
packages/plugin-babel/src/index.ts (2)
6-7: Consider resolving config lazily to avoid import-time throws.
resolveConfig()throws when the config file is missing or invalid. Calling it at module scope means any consumer that imports@saykit/plugin-babel(IDE tooling, test harnesses, downstream plugins) will throw before the plugin factory is ever invoked — which is significantly more hostile than throwing on first transform. Also, Babel’scwdoption can differ fromprocess.cwd(), yetresolveConfig()is evaluated before the factory receives any Babel API.Consider resolving inside the factory (or memoising on first
Programvisit) so the throw surfaces through Babel’s plugin-loading error channel and honours Babel’s configuredcwd:♻️ Proposed refactor
-export default function (): PluginObj { - const config = resolveConfig(); - - return { +export default function (): PluginObj { + let config: ReturnType<typeof resolveConfig> | undefined; + return { name: 'saykit', visitor: { Program(path, state) { + config ??= resolveConfig(); const id_ = state.file.opts.filename;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-babel/src/index.ts` around lines 6 - 7, The module currently calls resolveConfig() at import time inside the default exported function signature, which can throw before Babel invokes the plugin and ignores Babel's cwd; move the call to resolveConfig() out of module scope into the plugin factory so it runs lazily (either at the start of the exported default function body or deferred until the first Program visitor invocation) and memoize the resolved value so subsequent transforms reuse it; ensure the code uses the Babel API/options (e.g., the received cwd or plugin options) when calling resolveConfig() so errors surface through Babel's plugin-loading/transform channel rather than at import time.
14-14: Nit:node_modulessubstring check can over-match.
id_.includes('node_modules')will skip any absolute path that happens to contain the substring anywhere (e.g.,/home/user/my-node_modules_backup/..., or a monorepo package path withnode_modulesin its name). Matching path segments is safer:- if (!id_ || id_.includes('node_modules')) return; + if (!id_ || /[\\/]node_modules[\\/]/.test(id_)) return;Also worth aligning with
@saykit/plugin-unpluginif it uses the same guard, so the two plugins behave identically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-babel/src/index.ts` at line 14, The current guard uses id_.includes('node_modules') which can false-positive on paths that merely contain that substring; change the check in the file's loader (the branch where id_ is checked) to match node_modules as a path segment instead (e.g., use a path-aware test such as splitting id_ by path separators or using a regex that checks for node_modules as a segment like (^|[/\\])node_modules([/\\]|$)); update the conditional that currently reads "if (!id_ || id_.includes('node_modules')) return;" to use this safer path-segment check for id_, and make the behavior consistent with the guard used in `@saykit/plugin-unplugin`.
🤖 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/config/src/shapes.ts`:
- Line 15: The extension field in shapes.ts was loosened to z.string(), which
allows values without a leading dot and empty strings causing double dots in
output ('.{extension}'); update the validation for the extension property to
require a leading dot and at least one character after it (or alternatively
enforce min length > 0 and forbid a leading dot depending on intended semantics)
by replacing the current z.string() with a stricter validator (e.g., a
regex-based z.string().regex(...) or a template-literal style validator) so that
the extension always yields a single leading dot when interpolated into output
paths.
---
Duplicate comments:
In `@packages/plugin-babel/src/index.ts`:
- Around line 21-25: Replace the unsupported Program-level replaceWith: instead
of calling path.replaceWith(ast.program), mutate the existing Program node by
assigning path.node.body = ast.program.body and path.node.directives =
ast.program.directives (and copy any other top-level properties you need) so the
root keeps its parent context; when re-parsing, avoid using state.file.code
(which discards prior plugin edits) — generate source from the current path
(e.g., use `@babel/generator` to get generated = generate(path.node).code) and
parse that generated code with proper parser options (do not use
state.file.opts.parserOpts as any; ensure the parser plugins like jsx/ts are
enabled or map the `@babel/core` ParserOptions to `@babel/parser` options) before
applying the body/directives swap.
---
Nitpick comments:
In `@packages/plugin-babel/src/index.ts`:
- Around line 6-7: The module currently calls resolveConfig() at import time
inside the default exported function signature, which can throw before Babel
invokes the plugin and ignores Babel's cwd; move the call to resolveConfig() out
of module scope into the plugin factory so it runs lazily (either at the start
of the exported default function body or deferred until the first Program
visitor invocation) and memoize the resolved value so subsequent transforms
reuse it; ensure the code uses the Babel API/options (e.g., the received cwd or
plugin options) when calling resolveConfig() so errors surface through Babel's
plugin-loading/transform channel rather than at import time.
- Line 14: The current guard uses id_.includes('node_modules') which can
false-positive on paths that merely contain that substring; change the check in
the file's loader (the branch where id_ is checked) to match node_modules as a
path segment instead (e.g., use a path-aware test such as splitting id_ by path
separators or using a regex that checks for node_modules as a segment like
(^|[/\\])node_modules([/\\]|$)); update the conditional that currently reads "if
(!id_ || id_.includes('node_modules')) return;" to use this safer path-segment
check for id_, and make the behavior consistent with the guard used in
`@saykit/plugin-unplugin`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 700eeebb-555e-4d08-82f8-b7530dc8c28a
📒 Files selected for processing (5)
examples/tanstack-start/vite.config.tspackages/config/src/features/messages/identifier.tspackages/config/src/shapes.tspackages/plugin-babel/src/index.tspackages/plugin-unplugin/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- examples/tanstack-start/vite.config.ts
- packages/plugin-unplugin/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/transform-js/src/parser.ts (1)
160-166: Consider renaminggetPropertyNameAsStringnow that it can return a symbol.The
...AsStringsuffix is no longer accurate — the function may returnAUTO_INCREMENT_IDENTIFIER(asymbol). A name likegetPropertyNameAsIdentifier(mirroringgetExpressionAsKey/theidentifierfield it feeds into) would better convey the actual contract and match the terminology used elsewhere in the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transform-js/src/parser.ts` around lines 160 - 166, Rename the function getPropertyNameAsString to getPropertyNameAsIdentifier (or similar) to reflect that it can return a symbol (AUTO_INCREMENT_IDENTIFIER) rather than strictly a string; update the function declaration and all call sites (notably where getExpressionAsKey consumes its result into the identifier field) so names are consistent and tests/type annotations adjusted accordingly to avoid misleading "...AsString" terminology.
🤖 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/transform-js/src/parser.ts`:
- Around line 148-156: The code is unsafely casting the result of
getExpressionAsKey to string before constructing an ArgumentMessage; remove the
"as string" cast so key retains its true type (string | typeof
AUTO_INCREMENT_IDENTIFIER) and pass that value directly to new
ArgumentMessage(expression) as used in the existing branch, ensuring you use the
variable from getExpressionAsKey (key) and that ArgumentMessage's constructor
(from packages/config/src/features/messages/types.ts) already accepts the union;
if the constructor signature was changed, adjust it to accept string | typeof
AUTO_INCREMENT_IDENTIFIER rather than forcing a string.
- Line 35: Update unsafe loc.filename accesses and unsound casts: replace direct
accesses like tagged.loc.filename with optional chaining and a safe default
(e.g. tagged.loc?.filename ?? '<unknown>') wherever source location strings are
built (the two places that construct filename:line strings), remove the `as
string` cast when passing key from getExpressionAsKey() into ArgumentMessage so
the symbol AUTO_INCREMENT_IDENTIFIER is preserved, and rename
getPropertyNameAsString to reflect it may return a symbol (e.g.
getPropertyNameAsStringOrSymbol) and update its call sites accordingly.
---
Nitpick comments:
In `@packages/transform-js/src/parser.ts`:
- Around line 160-166: Rename the function getPropertyNameAsString to
getPropertyNameAsIdentifier (or similar) to reflect that it can return a symbol
(AUTO_INCREMENT_IDENTIFIER) rather than strictly a string; update the function
declaration and all call sites (notably where getExpressionAsKey consumes its
result into the identifier field) so names are consistent and tests/type
annotations adjusted accordingly to avoid misleading "...AsString" terminology.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 742a472b-2e9f-4579-a337-af205c58db54
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
examples/nextjs/.babelrcexamples/nextjs/package.jsonpackages/config/src/commands/extract.tspackages/config/src/shapes.tspackages/plugin-babel/package.jsonpackages/transform-js/src/parser.tspackages/transform-jsx/src/parser.ts
✅ Files skipped from review due to trivial changes (1)
- examples/nextjs/.babelrc
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/config/src/commands/extract.ts
- packages/transform-jsx/src/parser.ts
- packages/config/src/shapes.ts
- packages/plugin-babel/package.json
|
@greptile one more review pls |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile will keep reasking for review until its 5/5 ig |
|
@greptile review one more time |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/config/src/shapes.ts (1)
38-49:⚠️ Potential issue | 🔴 CriticalUse the reducer accumulator when chaining transformers.
Line 47 still calls
t.transform(code, id), so earlier transformer output is discarded whenever more than one transformer matches. Feedpinto the next transformer instead.🐛 Proposed fix
- transform: (code: string, id: string) => - t.reduce((p, t) => (t.match(id) ? t.transform(code, id) : p), code), + transform: (code: string, id: string) => + t.reduce((p, t) => (t.match(id) ? t.transform(p, id) : p), code),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/shapes.ts` around lines 38 - 49, The reducer currently ignores the accumulated transformed code by calling t.transform(code, id) inside the reduce in the Transformer.transform chain; update the reducer in the transform block so the accumulator p is passed into each next transformer (call t.transform(p, id) instead of t.transform(code, id)) so earlier transformer output is fed into subsequent transformers and the reduce logic in the Transformer transform implementation works correctly.examples/nextjs/package.json (1)
15-15:⚠️ Potential issue | 🟠 MajorVerify
next@16.2.4is published before merging.Line 15 still pins the previously flagged version. The registry metadata I could inspect for
nextdid not show16.2.4in the listed releases, so this can break installs if npm still lacks that version: https://packages.ecosyste.ms/registries/npmjs.org/packages/next/versionsRun this read-only check against npm and pin to the latest published version if it fails:
#!/bin/bash # Description: Verify whether the pinned Next.js version exists on npm. set -euo pipefail npm view next@16.2.4 version npm view next dist-tags --json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/nextjs/package.json` at line 15, The package.json currently pins "next" to 16.2.4 but that version may not be published; run an npm registry check (e.g., npm view next@16.2.4 version and npm view next dist-tags) to confirm existence, and if it’s missing update the "next" entry in package.json (the dependency key "next") to a published version (either the latest dist-tag or a specific published semver) or change to a valid tag like "latest" before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/nextjs/package.json`:
- Line 15: The package.json currently pins "next" to 16.2.4 but that version may
not be published; run an npm registry check (e.g., npm view next@16.2.4 version
and npm view next dist-tags) to confirm existence, and if it’s missing update
the "next" entry in package.json (the dependency key "next") to a published
version (either the latest dist-tag or a specific published semver) or change to
a valid tag like "latest" before merging.
In `@packages/config/src/shapes.ts`:
- Around line 38-49: The reducer currently ignores the accumulated transformed
code by calling t.transform(code, id) inside the reduce in the
Transformer.transform chain; update the reducer in the transform block so the
accumulator p is passed into each next transformer (call t.transform(p, id)
instead of t.transform(code, id)) so earlier transformer output is fed into
subsequent transformers and the reduce logic in the Transformer transform
implementation works correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d1a21ab-3060-4cf2-8bc6-0ca67626ca52
📒 Files selected for processing (16)
.changeset/config.json.changeset/nine-zebras-greet.md.changeset/silent-moose-wish.md.github/labels.ymlexamples/nextjs/package.jsonpackages/config/src/features/catalogue/extractor.tspackages/config/src/features/loader/resolve.tspackages/config/src/features/workers/extract-worker.tspackages/config/src/shapes.tspackages/plugin-babel/src/index.tspackages/plugin-unplugin/src/index.tspackages/transform-js/src/index.tspackages/transform-js/src/parser.tspackages/transform-jsx/src/index.tswebsite/content/core-concepts/configuration.mdxwebsite/content/getting-started/installation.mdx
✅ Files skipped from review due to trivial changes (7)
- .github/labels.yml
- .changeset/nine-zebras-greet.md
- .changeset/silent-moose-wish.md
- packages/config/src/features/catalogue/extractor.ts
- website/content/getting-started/installation.mdx
- packages/plugin-unplugin/src/index.ts
- packages/plugin-babel/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/config.json
- website/content/core-concepts/configuration.mdx
- packages/config/src/features/workers/extract-worker.ts
- packages/transform-jsx/src/index.ts
- packages/transform-js/src/parser.ts
|
@greptile one more review pls, say 5/5 this time |
1 participant
Closes #9
Summary by CodeRabbit
New Features
Improvements
Breaking Changes
Documentation
Greptile Summary
This PR refactors the saykit transformer architecture by splitting the monolithic Babel plugin into two new standalone packages —
@saykit/transform-js(for.js/.tsfiles) and@saykit/transform-jsx(for.jsx/.tsxfiles) — and introduces a first-classTransformerconcept in the config schema alongside the existingFormatter. It also adds new worker classes (BucketBuildWorker,BucketExtractWorker,BucketCompileWorker) for cleaner orchestration of the build/extract/compile pipeline, adds example Next.js and TanStack Start apps, and renamescreateFormatter→createPoFormatterin the PO package.Key changes:
TransformerZod schema inshapes.ts; single transformers are wrapped into an array and composed into a singlematch/extract/transforminterfacetransform-jsandtransform-jsxeach providecreateProgram,extract, andtransformusing their own internal Babel traversalformat-poformatter API changed from async (Promise<>) to synchronousBucketExtractWorker,BucketCompileWorker, andBucketBuildWorkerextractarguments, broken transform chaining,cooked!null assertion, single-transformermatchbypass, stale fallback translations) appear to be resolved in this versionConfidence Score: 4/5
Safe to merge — all previously flagged critical bugs are resolved and no new critical issues were found in this revision.
All prior critical findings (reversed extract args, broken transform chain, null cooked assertion, single-transformer match bypass, wrong fallback locale messages) are addressed in this PR. The transform pipeline correctly uses the accumulator
pin the reduce,extract(content, path)arguments are in the correct order,cooked ?? rawfallback is applied, and the Zod composite transformer wrapper guards each individual transformer'smatchbefore delegating. TheapplyFallbackTranslationsnow correctly reads the fallback locale's catalogue from disk viareadCatalogueMessages. No new logic or security bugs were identified. Score is 4 rather than 5 because this is a large multi-package refactor with new example apps and non-trivial AST-level code paths that benefit from integration testing before full confidence.No files require special attention; all critical paths in
packages/config/src/shapes.ts,packages/config/src/features/catalogue/extractor.ts,packages/transform-js/src/, andpackages/transform-jsx/src/look correct.Reviews (7): Last reviewed commit: "Add labels for transform-js and transfor..." | Re-trigger Greptile