Open
Conversation
🦋 Changeset detectedLatest commit: 554251b The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 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.
|
|
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:
📝 WalkthroughWalkthroughAdds an experimental Slonik plugin package and demo, integrates plugin-driven expression mapping into the ESLint rule pipeline, improves sourcemap-aware error mapping/reporting, switches plugin resolution to synchronous loading, and expands tests, docs, and build configs. Changes
Sequence Diagram(s)sequenceDiagram
participant ESLintRule as "ESLint Rule"
participant ExprMapper as "Expression Mapper\n(mapTemplateLiteralToQueryText)"
participant SlonikPlugin as "Slonik Plugin"
participant Checker as "TypeChecker / DB (Zod/PG)"
ESLintRule->>ExprMapper: mapTemplateLiteralToQueryText(template, plugins, sourceCode)
activate ExprMapper
loop each template expression
ExprMapper->>SlonikPlugin: resolvePluginExpression(expr, tsNode, tsType, precedingSQL)
activate SlonikPlugin
alt plugin returns SQL string
SlonikPlugin-->>ExprMapper: SQL fragment with $N placeholders
else plugin returns false
SlonikPlugin-->>ExprMapper: signal to skip validation
else no plugin result
SlonikPlugin-->>ExprMapper: undefined (continue default mapping)
end
deactivate SlonikPlugin
end
ExprMapper-->>ESLintRule: final queryText + sourcemap
deactivate ExprMapper
ESLintRule->>Checker: run type-check / zod validation (enforceType: fix|suggest)
Checker-->>ESLintRule: errors, fixes or suggestions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts (1)
17-19: Escape embedded quotes in generated identifier text.
return \"${argument.value}"`;can produce invalid SQL when the literal contains"`. Escaping makes this fixture safer and more representative.Proposed tweak
- if (argument?.type === "Literal" && typeof argument.value === "string") { - return `"${argument.value}"`; + if (argument?.type === "Literal" && typeof argument.value === "string") { + return `"${argument.value.replaceAll('"', '""')}"`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts` around lines 17 - 19, The current literal-handling branch returns `"${argument.value}"` which can produce invalid SQL if the string contains double quotes; update the branch that checks `argument?.type === "Literal" && typeof argument.value === "string"` to escape embedded double quotes in `argument.value` before wrapping it in quotes (e.g., replace `"` with `\"` or use a safe serializer such as JSON.stringify(argument.value)) so the generated identifier text is valid and safe.packages/plugin-utils/src/resolve.ts (1)
74-87:resolveOneis now effectively synchronous; consider simplifying.The
asyncmethod now usesloadModuleSyncand no longer performs any asynchronous operations. Consider either:
- Removing the async wrapper if callers can be updated
- Adding a comment explaining why the async signature is retained (e.g., backward compatibility)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/src/resolve.ts` around lines 74 - 87, The resolveOne method is marked async but performs only synchronous work (getCacheKey, pluginCache.get/set, loadModuleSync, extractPlugin); remove the async wrapper and change the signature to synchronous (drop async and return Promise/Sync type as appropriate) or, if you must keep async for compatibility, add a concise comment above resolveOne stating it intentionally remains async for backward compatibility; update any callers if you choose to make it synchronous. Ensure to reference and adjust usages of resolveOne, loadModuleSync, getCacheKey, extractPlugin, and pluginCache to match the new synchronous signature or retain the async wrapper with the explanatory comment.packages/plugins/slonik/package.json (1)
39-41: Wide peer dependency range may need testing coverage.The peer dependency specifies
slonik >=38.0.0, but the dev dependency uses^48.13.2. This means the plugin claims compatibility with slonik versions 38-47 that aren't being tested.Consider either narrowing the peer dependency range to versions you've actually tested, or documenting known compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/slonik/package.json` around lines 39 - 41, The peer dependency in package.json currently lists "slonik": ">=38.0.0" while devDependencies only test "^48.13.2", which claims unsupported ranges; update package.json to either narrow "peerDependencies.slonik" to the tested range (e.g., "^48.13.2" or a compatible semver range you validated) or add testing coverage/documentation for older 38–47 versions; ensure you modify the package.json "peerDependencies" entry and, if choosing to support older versions, add CI matrix entries and update README/compatibility notes referencing "slonik" to reflect the validated versions.demos/plugin-slonik/src/index.ts (1)
5-5: Empty connection string may cause runtime issues.The pool is created with
"postgres://"which has no host, port, or database specified. While this demo may never be executed (only linted), if someone tries to run it, it will fail.Consider using a placeholder that's more obviously incomplete or adding a comment.
♻️ Suggested improvement
-const pool = await createPool("postgres://", { driverFactory: createPgDriverFactory() }); +// Demo file - not meant to be executed. Configure a real connection string if running. +const pool = await createPool("postgres://user:pass@localhost:5432/db", { driverFactory: createPgDriverFactory() });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/plugin-slonik/src/index.ts` at line 5, The demo uses an empty connection string ("postgres://") when constructing the pool via createPool with createPgDriverFactory (variable pool), which will fail at runtime; replace the empty URI with a clear placeholder (e.g. include host/port/db or a comment) or add an inline comment next to the createPool call clarifying it’s intentionally non-functional and must be replaced before running so readers won’t attempt to run with an invalid connection string.packages/plugins/slonik/src/plugin.ts (3)
91-98:sql.literalValuefallback returns empty string literal.When
sql.literalValue(...)is called with a non-string literal argument (e.g., a variable), the function returns''(empty string literal). This may produce unexpected SQL behavior.Consider returning
falseto skip the query when the literal value cannot be statically determined, similar to howsql.unnesthandles dynamic cases.♻️ Suggested fix
if (isCallTo(node, "literalValue")) { const arg = node.arguments[0]; if (arg?.type === "Literal" && typeof arg.value === "string") return `'${arg.value}'`; - return "''"; + return false; // Cannot statically determine literal value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/slonik/src/plugin.ts` around lines 91 - 98, The fallback for sql.literalValue currently returns the empty string literal "''" which can produce bad SQL when the argument isn't a static string; in the isCallTo(node, "literalValue") branch (in package plugins/slonik/src/plugin.ts) change the non-string/default return to false (matching how sql.unnest handles dynamic cases) so callers skip or treat the value as dynamic instead of injecting an empty literal; ensure isCallTo(..., "join") behavior remains unchanged.
181-196: Fragment variable lookup only searches immediate scope.The
findVariableInitfunction walks up to find the nearestProgramorBlockStatement, then searches for variable declarations. This may miss variables declared in:
- Parent function scopes
- Module-level declarations when inside nested blocks
- Destructured variables
For the common case of
const where = sql.fragment\...`; sql.unsafe`... ${where}``, this works, but more complex patterns may fail silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/slonik/src/plugin.ts` around lines 181 - 196, findVariableInit only inspects declarations in the first Program or BlockStatement it finds, missing variables in enclosing function/module scopes and destructured declarators; update findVariableInit to walk ancestors up to the Program node and at each ancestor that can contain statements (Program, BlockStatement, FunctionDeclaration/FunctionExpression/ArrowFunction) examine all statement lists (body, body.body or function.body) for VariableDeclaration nodes, and for each VariableDeclarator handle id types Identifier, ObjectPattern and ArrayPattern (recursively) to match the target id.name and return decl.init when found; keep the function signature (findVariableInit(id: TSESTree.Identifier)) and only return undefined if no matching declarator/init is found after checking all ancestor scopes.
144-150: Module detection relies on path pattern matching that may fail across different environments.The
isSymbolFromModulefunction checks ifsourceFile.fileNamecontains/${moduleName}/using string matching. This has potential issues:
- On Windows, TypeScript returns backslash-separated paths, so the forward-slash pattern won't match
- Symlinked packages may resolve to unexpected paths depending on TypeScript's resolution strategy
Consider using path normalization (e.g.,
path.normalize()orpath.sep) or a more robust comparison approach like checking resolved module paths directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/slonik/src/plugin.ts` around lines 144 - 150, The module detection in isSymbolFromModule currently checks sourceFile.fileName.includes(`/${moduleName}/`), which fails on Windows and symlinked paths; update isSymbolFromModule to normalize paths and perform a robust comparison: import path, call path.normalize(sourceFile.fileName) (or split by path.sep) and then check for a segment equal to moduleName or use path.join(path.sep, moduleName, path.sep) with path.sep, and also fallback to comparing path.basename or resolved module paths if declarations provide them; update the loop that iterates resolved.declarations and replace the string literal `/${moduleName}/` check with the normalized/segmented comparison so it works across platforms and symlinked resolutions.packages/eslint-plugin/src/utils/ts-pg.utils.ts (1)
193-221: Last-plugin-wins semantics may be surprising.The
resolvePluginExpressionfunction iterates through all plugins and keeps overwritingresultwhenever a plugin returns a non-undefinedvalue. This means the last plugin to return a value "wins", which differs from typical "first match wins" plugin patterns.Consider documenting this behavior or implementing early return when a plugin definitively handles an expression.
♻️ Alternative: first-plugin-wins semantics
function resolvePluginExpression(params: { expression: TSESTree.Expression; checker: ts.TypeChecker; plugins: SafeQLPlugin[]; precedingSQL: string; tsNode: ts.Node; tsType: ts.Type; }): string | false | undefined { - let result: string | false | undefined; - for (const plugin of params.plugins) { const pluginResult = plugin.onExpression?.({ node: params.expression, context: { precedingSQL: params.precedingSQL, checker: params.checker, tsNode: params.tsNode, tsType: params.tsType, tsTypeText: params.checker.typeToString(params.tsType), }, }); if (pluginResult !== undefined) { - result = pluginResult; + return pluginResult; // First plugin to handle wins } } - return result; + return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/utils/ts-pg.utils.ts` around lines 193 - 221, The current resolvePluginExpression iterates all plugins and lets the last non-undefined pluginResult win; change it to first-plugin-wins by returning immediately when a plugin's onExpression returns a non-undefined value (i.e., inside resolvePluginExpression, after calling plugin.onExpression for each plugin, if pluginResult !== undefined then return pluginResult). Make sure to reference resolvePluginExpression and plugin.onExpression in the change and preserve semantics for explicit false (still return false immediately).packages/plugins/slonik/src/plugin.test.ts (1)
150-156: Consider simplifying the assertion branching.The current logic checks for
"skipped" in c.outputfirst, then checks ifc.output.sql.includes("/* skipped */"). However, when"skipped" in c.outputis true,c.outputis{ skipped: true }and accessing.sqlwould be invalid. The current order prevents this, but the condition at line 152 could be clearer.♻️ Suggested refactor for clarity
// ASSERT - if ("skipped" in c.output) { + if ("skipped" in c.output && c.output.skipped === true) { expect(result).toEqual({ skipped: true }); - } else if (c.output.sql.includes("/* skipped */")) { + } else if ("sql" in c.output && c.output.sql.includes("/* skipped */")) { expect(result).toMatchObject({ sql: expect.stringContaining("/* skipped */") }); } else { expect(result).toEqual(c.output); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/slonik/src/plugin.test.ts` around lines 150 - 156, The branching in the test should explicitly guard access to c.output.sql; update the second condition to check that c.output has an sql property before calling includes (e.g. use "if ('sql' in c.output && c.output.sql.includes('/* skipped */'))" or use optional chaining c.output.sql?.includes(...)) so the assertions for result (checking { skipped: true }, sql containing "/* skipped */", or full equality to c.output) remain correct and clear when running the test in plugin.test.ts where c.output and result are compared.
🤖 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/plugin-utils/package.json`:
- Around line 40-41: The package.json adds "tsx" as a runtime dependency but the
build config's externals list (in build.config.ts) doesn't include "tsx", so the
bundler will package it; update the externals array (the externals variable or
export where `@typescript-eslint/parser`, `@typescript-eslint/utils`, and typescript
are listed) to include "tsx" so it is treated as an external dependency and not
bundled into the dist output.
In `@packages/plugin-utils/src/resolve.ts`:
- Around line 93-97: The tsx.require call in the isLocalPath branch is using
pluginPath as both module ID and parent filename which is incorrect; update the
call in resolve.ts where you create tsx with
createRequire(import.meta.url)(`tsx/cjs/api`) so that you call
tsx.require(pluginPath, import.meta.url) instead of tsx.require(pluginPath,
pluginPath) to provide the correct ESM resolution context (use import.meta.url
as the second argument).
In `@packages/plugins/slonik/package.json`:
- Around line 2-4: Remove the "private": true field from the package.json for
the `@ts-safeql/plugin-slonik` package so it can be published; locate the
package.json entry containing "name": "@ts-safeql/plugin-slonik" and delete the
"private": true property, ensuring the version and other fields (version,
exports, types) remain unchanged so the package can be released via the monorepo
changesets workflow.
---
Nitpick comments:
In `@demos/plugin-slonik/src/index.ts`:
- Line 5: The demo uses an empty connection string ("postgres://") when
constructing the pool via createPool with createPgDriverFactory (variable pool),
which will fail at runtime; replace the empty URI with a clear placeholder (e.g.
include host/port/db or a comment) or add an inline comment next to the
createPool call clarifying it’s intentionally non-functional and must be
replaced before running so readers won’t attempt to run with an invalid
connection string.
In `@packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts`:
- Around line 17-19: The current literal-handling branch returns
`"${argument.value}"` which can produce invalid SQL if the string contains
double quotes; update the branch that checks `argument?.type === "Literal" &&
typeof argument.value === "string"` to escape embedded double quotes in
`argument.value` before wrapping it in quotes (e.g., replace `"` with `\"` or
use a safe serializer such as JSON.stringify(argument.value)) so the generated
identifier text is valid and safe.
In `@packages/eslint-plugin/src/utils/ts-pg.utils.ts`:
- Around line 193-221: The current resolvePluginExpression iterates all plugins
and lets the last non-undefined pluginResult win; change it to first-plugin-wins
by returning immediately when a plugin's onExpression returns a non-undefined
value (i.e., inside resolvePluginExpression, after calling plugin.onExpression
for each plugin, if pluginResult !== undefined then return pluginResult). Make
sure to reference resolvePluginExpression and plugin.onExpression in the change
and preserve semantics for explicit false (still return false immediately).
In `@packages/plugin-utils/src/resolve.ts`:
- Around line 74-87: The resolveOne method is marked async but performs only
synchronous work (getCacheKey, pluginCache.get/set, loadModuleSync,
extractPlugin); remove the async wrapper and change the signature to synchronous
(drop async and return Promise/Sync type as appropriate) or, if you must keep
async for compatibility, add a concise comment above resolveOne stating it
intentionally remains async for backward compatibility; update any callers if
you choose to make it synchronous. Ensure to reference and adjust usages of
resolveOne, loadModuleSync, getCacheKey, extractPlugin, and pluginCache to match
the new synchronous signature or retain the async wrapper with the explanatory
comment.
In `@packages/plugins/slonik/package.json`:
- Around line 39-41: The peer dependency in package.json currently lists
"slonik": ">=38.0.0" while devDependencies only test "^48.13.2", which claims
unsupported ranges; update package.json to either narrow
"peerDependencies.slonik" to the tested range (e.g., "^48.13.2" or a compatible
semver range you validated) or add testing coverage/documentation for older
38–47 versions; ensure you modify the package.json "peerDependencies" entry and,
if choosing to support older versions, add CI matrix entries and update
README/compatibility notes referencing "slonik" to reflect the validated
versions.
In `@packages/plugins/slonik/src/plugin.test.ts`:
- Around line 150-156: The branching in the test should explicitly guard access
to c.output.sql; update the second condition to check that c.output has an sql
property before calling includes (e.g. use "if ('sql' in c.output &&
c.output.sql.includes('/* skipped */'))" or use optional chaining
c.output.sql?.includes(...)) so the assertions for result (checking { skipped:
true }, sql containing "/* skipped */", or full equality to c.output) remain
correct and clear when running the test in plugin.test.ts where c.output and
result are compared.
In `@packages/plugins/slonik/src/plugin.ts`:
- Around line 91-98: The fallback for sql.literalValue currently returns the
empty string literal "''" which can produce bad SQL when the argument isn't a
static string; in the isCallTo(node, "literalValue") branch (in package
plugins/slonik/src/plugin.ts) change the non-string/default return to false
(matching how sql.unnest handles dynamic cases) so callers skip or treat the
value as dynamic instead of injecting an empty literal; ensure isCallTo(...,
"join") behavior remains unchanged.
- Around line 181-196: findVariableInit only inspects declarations in the first
Program or BlockStatement it finds, missing variables in enclosing
function/module scopes and destructured declarators; update findVariableInit to
walk ancestors up to the Program node and at each ancestor that can contain
statements (Program, BlockStatement,
FunctionDeclaration/FunctionExpression/ArrowFunction) examine all statement
lists (body, body.body or function.body) for VariableDeclaration nodes, and for
each VariableDeclarator handle id types Identifier, ObjectPattern and
ArrayPattern (recursively) to match the target id.name and return decl.init when
found; keep the function signature (findVariableInit(id: TSESTree.Identifier))
and only return undefined if no matching declarator/init is found after checking
all ancestor scopes.
- Around line 144-150: The module detection in isSymbolFromModule currently
checks sourceFile.fileName.includes(`/${moduleName}/`), which fails on Windows
and symlinked paths; update isSymbolFromModule to normalize paths and perform a
robust comparison: import path, call path.normalize(sourceFile.fileName) (or
split by path.sep) and then check for a segment equal to moduleName or use
path.join(path.sep, moduleName, path.sep) with path.sep, and also fallback to
comparing path.basename or resolved module paths if declarations provide them;
update the loop that iterates resolved.declarations and replace the string
literal `/${moduleName}/` check with the normalized/segmented comparison so it
works across platforms and symlinked resolutions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7d9b820-bb1b-41a0-bc3b-79d65eb893bc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
demos/plugin-slonik/eslint.config.jsdemos/plugin-slonik/package.jsondemos/plugin-slonik/src/index.tsdemos/plugin-slonik/tsconfig.jsondocs/compatibility/postgres.js.mddocs/compatibility/slonik.mdpackages/eslint-plugin/package.jsonpackages/eslint-plugin/src/rules/check-sql.rule.tspackages/eslint-plugin/src/rules/check-sql.test.tspackages/eslint-plugin/src/rules/check-sql.utils.tspackages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.tspackages/eslint-plugin/src/utils/ts-pg.utils.tspackages/plugin-utils/package.jsonpackages/plugin-utils/src/resolve.tspackages/plugins/slonik/build.config.tspackages/plugins/slonik/package.jsonpackages/plugins/slonik/src/index.tspackages/plugins/slonik/src/plugin.integration.test.tspackages/plugins/slonik/src/plugin.test.tspackages/plugins/slonik/src/plugin.tspackages/plugins/slonik/src/ts-fixture/file.tspackages/plugins/slonik/src/ts-fixture/tsconfig.jsonpackages/plugins/slonik/tsconfig.jsonpackages/zod-annotator/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts (1)
36-50: Consider simplifying the type guard.The
isCallfunction is defensively checking properties onunknown, but in theonExpressioncontext,nodeis already typed asTSESTree.Expression. A more concise implementation could leverage this:♻️ Simplified implementation using Expression type
-function isCall(node: unknown, name: string): node is TSESTree.CallExpression { - return ( - typeof node === "object" && - node !== null && - "type" in node && - node.type === "CallExpression" && - "callee" in node && - typeof node.callee === "object" && - node.callee !== null && - "type" in node.callee && - node.callee.type === "Identifier" && - "name" in node.callee && - node.callee.name === name - ); +function isCall(node: TSESTree.Expression, name: string): node is TSESTree.CallExpression & { callee: TSESTree.Identifier } { + return ( + node.type === "CallExpression" && + node.callee.type === "Identifier" && + node.callee.name === name + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts` around lines 36 - 50, The isCall type-guard is overly defensive for callers that already pass a TSESTree.Expression (e.g., onExpression); change the isCall signature to accept node: TSESTree.Expression (or narrow inside callers) and simplify the check to assert node.type === "CallExpression" && node.callee.type === "Identifier" && node.callee.name === name, returning the appropriate type predicate (node is TSESTree.CallExpression) so callers like onExpression can rely on the simpler guard.
🤖 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/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts`:
- Around line 15-20: The ident() handling currently returns the raw string
interpolation for argument.value which breaks if the value contains double
quotes; update the logic in the isCall(node, "ident") branch to escape embedded
double quotes by replacing each " with "" before wrapping the result in double
quotes so the returned value is a valid SQL identifier (locate the isCall(node,
"ident") block and the use of argument.value and change the string construction
to perform the quote-doubling escape).
---
Nitpick comments:
In `@packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts`:
- Around line 36-50: The isCall type-guard is overly defensive for callers that
already pass a TSESTree.Expression (e.g., onExpression); change the isCall
signature to accept node: TSESTree.Expression (or narrow inside callers) and
simplify the check to assert node.type === "CallExpression" && node.callee.type
=== "Identifier" && node.callee.name === name, returning the appropriate type
predicate (node is TSESTree.CallExpression) so callers like onExpression can
rely on the simpler guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f192108-e3d2-4c92-b789-0f8764ef7b5b
📒 Files selected for processing (1)
packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts
Newbie012
force-pushed
the
feat-slonik-plugin
branch
3 times, most recently
from
b99d787 to
a74de57
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/plugins/slonik/src/plugin.ts (2)
99-100: Clarify behavior whensql.arrayhas a fragment type argument.When the second argument is not a string literal (e.g.,
sql.fragment\int[]`), the code returns"$N"(untyped placeholder). The test at line 103-104 inplugin.test.ts` confirms this is intentional, but consider adding a comment explaining why fragment types can't be resolved statically.📝 Suggested clarification
case "array": + // String literal type → typed array cast; fragment type → untyped placeholder (can't resolve statically) return node.arguments[1]?.type === "Literal" ? `$N::${node.arguments[1].value}[]` : "$N";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/slonik/src/plugin.ts` around lines 99 - 100, Add a short explanatory comment inside the switch case for "array" (around the code that inspects node.arguments[1]) stating that when the second argument is not a Literal (e.g., a sql.fragment like `sql.fragment\`int[]\``) we cannot resolve the SQL type statically at compile time, so we intentionally fall back to the untyped placeholder "$N"; reference the "array" case, node.arguments[1], and the related test in plugin.test.ts to make the rationale clear for future readers.
159-175: Fragment detection heuristic is reasonable but may have edge cases.The
isFragmentTokenTypefunction uses two approaches:
- Type text matching (
FragmentSqlToken,SqlFragmentToken)- Duck typing (has
sql,values,typebut notparser)This covers common cases but could potentially match non-Slonik types with similar shapes. Consider documenting this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/slonik/src/plugin.ts` around lines 159 - 175, isFragmentTokenType currently identifies fragment tokens by matching tsTypeText for FragmentSqlToken/SqlFragmentToken and by duck-typing the tsType for properties "sql", "values", "type" and absence of "parser", which can produce false positives for similarly-shaped non-Slonik types; update the code by adding a clear explanatory comment above the isFragmentTokenType function that documents this heuristic, lists the checked symbols (FragmentSqlToken, SqlFragmentToken) and property-based checks ("sql", "values", "type", not "parser"), and explicitly states the potential edge-case limitation and that future changes should tighten detection if needed (e.g., using fully-qualified type checks or additional distinctive markers).packages/plugin-utils/src/resolve.ts (1)
62-84:resolveOneandresolveOneSyncare now identical — consider consolidating.Both methods perform the same synchronous logic after the change.
resolveOnecould simply delegate toresolveOneSync, or one of them could be removed entirely.♻️ Suggested consolidation
- private resolveOne(descriptor: PluginDescriptor, projectDir: string): SafeQLPlugin { - const key = this.getCacheKey(descriptor); - const cached = this.pluginCache.get(key); - if (cached) return cached; - - const mod = this.loadModuleSync(descriptor.package, projectDir); - - const plugin = this.extractPlugin(descriptor, mod); - this.pluginCache.set(key, plugin); - return plugin; - } + private resolveOne(descriptor: PluginDescriptor, projectDir: string): SafeQLPlugin { + return this.resolveOneSync(descriptor, projectDir); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/src/resolve.ts` around lines 62 - 84, Both resolveOne and resolveOneSync contain identical synchronous logic; consolidate by having one delegate to the other to avoid duplication. Modify the resolveOne(descriptor, projectDir) implementation to simply return this.resolveOneSync(descriptor, projectDir) (or vice versa), keeping getCacheKey, loadModuleSync, extractPlugin, and pluginCache usage unchanged, and remove the duplicated body so only one implementation contains the actual logic.demos/plugin-slonik/src/index.ts (2)
5-5: Consider using a placeholder that clearly indicates this is a demo.The empty connection string
"postgres://"may cause confusion or runtime errors if someone tries to execute this file. Consider using a more explicit placeholder with a comment.📝 Suggested improvement
-const pool = await createPool("postgres://", { driverFactory: createPgDriverFactory() }); +// Demo file — connection URL should be replaced with a real database for execution +const pool = await createPool("postgres://user:password@localhost:5432/db", { driverFactory: createPgDriverFactory() });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/plugin-slonik/src/index.ts` at line 5, The connection string passed to createPool (const pool = await createPool("postgres://", { driverFactory: createPgDriverFactory() });) is an ambiguous empty URL; replace it with a clear demo placeholder (e.g., "postgres://user:pass@localhost:5432/dbname" or "postgres://<USER>:<PASS>@<HOST>:<PORT>/<DB>") and add a short inline comment indicating it must be replaced in real usage so users don’t run the demo with an invalid URI.
201-206: Thesectionandexamplehelper functions are no-ops.These functions execute their callbacks but don't provide any reporting or grouping. If this is intentional for static analysis purposes only (ESLint checking), consider adding a comment to clarify.
📝 Suggested clarification
+// These helpers exist solely to organize code for ESLint/SafeQL static analysis. +// They execute their callbacks immediately without additional runtime behavior. function section(_: string, fn: () => void) { fn(); } function example(_: string, fn: () => void) { fn(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/plugin-slonik/src/index.ts` around lines 201 - 206, The helpers section and example are currently no-ops (they simply call the callbacks) which is confusing; either make them meaningful or explicitly document their no-op intent: update the functions section and example to either implement simple grouping/reporting (e.g., call console.group/console.groupEnd or forward to your test/reporting utility around fn()) or add a clear explanatory comment/JSDoc above each function stating they intentionally exist only for static analysis/ESLint/test-structure and must not perform runtime behavior. Ensure you reference the exact function names section and example when making the change so reviewers can see the intended behavior.
🤖 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/eslint-plugin/src/utils/ts-pg.utils.ts`:
- Around line 56-68: The sourcemap entry pushed into sourcemaps uses
original.end: expression.range[1] - quasi.range[0] but the extracted text
includes the trailing '}' (sourceCode.text.slice uses expression.range[1] + 1),
so update the original.end calculation to expression.range[1] - quasi.range[0] +
1 to match the actual slice; locate the sourcemaps.push call that builds the
original object (references to sourcemaps, expression.range, quasi.range, and
the text slice) and adjust original.end accordingly so the sourcemap spans the
same characters as the extracted text.
---
Nitpick comments:
In `@demos/plugin-slonik/src/index.ts`:
- Line 5: The connection string passed to createPool (const pool = await
createPool("postgres://", { driverFactory: createPgDriverFactory() });) is an
ambiguous empty URL; replace it with a clear demo placeholder (e.g.,
"postgres://user:pass@localhost:5432/dbname" or
"postgres://<USER>:<PASS>@<HOST>:<PORT>/<DB>") and add a short inline comment
indicating it must be replaced in real usage so users don’t run the demo with an
invalid URI.
- Around line 201-206: The helpers section and example are currently no-ops
(they simply call the callbacks) which is confusing; either make them meaningful
or explicitly document their no-op intent: update the functions section and
example to either implement simple grouping/reporting (e.g., call
console.group/console.groupEnd or forward to your test/reporting utility around
fn()) or add a clear explanatory comment/JSDoc above each function stating they
intentionally exist only for static analysis/ESLint/test-structure and must not
perform runtime behavior. Ensure you reference the exact function names section
and example when making the change so reviewers can see the intended behavior.
In `@packages/plugin-utils/src/resolve.ts`:
- Around line 62-84: Both resolveOne and resolveOneSync contain identical
synchronous logic; consolidate by having one delegate to the other to avoid
duplication. Modify the resolveOne(descriptor, projectDir) implementation to
simply return this.resolveOneSync(descriptor, projectDir) (or vice versa),
keeping getCacheKey, loadModuleSync, extractPlugin, and pluginCache usage
unchanged, and remove the duplicated body so only one implementation contains
the actual logic.
In `@packages/plugins/slonik/src/plugin.ts`:
- Around line 99-100: Add a short explanatory comment inside the switch case for
"array" (around the code that inspects node.arguments[1]) stating that when the
second argument is not a Literal (e.g., a sql.fragment like
`sql.fragment\`int[]\``) we cannot resolve the SQL type statically at compile
time, so we intentionally fall back to the untyped placeholder "$N"; reference
the "array" case, node.arguments[1], and the related test in plugin.test.ts to
make the rationale clear for future readers.
- Around line 159-175: isFragmentTokenType currently identifies fragment tokens
by matching tsTypeText for FragmentSqlToken/SqlFragmentToken and by duck-typing
the tsType for properties "sql", "values", "type" and absence of "parser", which
can produce false positives for similarly-shaped non-Slonik types; update the
code by adding a clear explanatory comment above the isFragmentTokenType
function that documents this heuristic, lists the checked symbols
(FragmentSqlToken, SqlFragmentToken) and property-based checks ("sql", "values",
"type", not "parser"), and explicitly states the potential edge-case limitation
and that future changes should tighten detection if needed (e.g., using
fully-qualified type checks or additional distinctive markers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dc389cf-89fb-48fb-8584-a5391e77315b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.changeset/slonik-plugin-public.mddemos/plugin-slonik/eslint.config.jsdemos/plugin-slonik/package.jsondemos/plugin-slonik/src/index.tsdemos/plugin-slonik/tsconfig.jsondocs/compatibility/postgres.js.mddocs/compatibility/slonik.mdpackages/eslint-plugin/package.jsonpackages/eslint-plugin/src/rules/check-sql.rule.tspackages/eslint-plugin/src/rules/check-sql.test.tspackages/eslint-plugin/src/rules/check-sql.utils.tspackages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.tspackages/eslint-plugin/src/utils/ts-pg.utils.tspackages/plugin-utils/build.config.tspackages/plugin-utils/package.jsonpackages/plugin-utils/src/resolve.tspackages/plugins/slonik/build.config.tspackages/plugins/slonik/package.jsonpackages/plugins/slonik/src/index.tspackages/plugins/slonik/src/plugin.integration.test.tspackages/plugins/slonik/src/plugin.test.tspackages/plugins/slonik/src/plugin.tspackages/plugins/slonik/src/ts-fixture/file.tspackages/plugins/slonik/src/ts-fixture/tsconfig.jsonpackages/plugins/slonik/tsconfig.jsonpackages/zod-annotator/package.json
✅ Files skipped from review due to trivial changes (15)
- packages/plugins/slonik/src/index.ts
- demos/plugin-slonik/tsconfig.json
- .changeset/slonik-plugin-public.md
- packages/plugin-utils/package.json
- packages/zod-annotator/package.json
- packages/plugins/slonik/src/ts-fixture/file.ts
- packages/plugins/slonik/tsconfig.json
- packages/plugin-utils/build.config.ts
- packages/eslint-plugin/package.json
- docs/compatibility/postgres.js.md
- packages/plugins/slonik/build.config.ts
- packages/plugins/slonik/src/ts-fixture/tsconfig.json
- demos/plugin-slonik/package.json
- packages/plugins/slonik/package.json
- docs/compatibility/slonik.md
🚧 Files skipped from review as they are similar to previous changes (5)
- demos/plugin-slonik/eslint.config.js
- packages/eslint-plugin/src/rules/check-sql.rule.ts
- packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts
- packages/plugins/slonik/src/plugin.integration.test.ts
- packages/eslint-plugin/src/rules/check-sql.utils.ts
Greptile SummaryThis PR introduces the
Confidence Score: 4/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Newbie012
force-pushed
the
feat-slonik-plugin
branch
from
a74de57 to
ee7d695
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/eslint-plugin/src/utils/ts-pg.utils.ts`:
- Around line 52-53: The current global replace on pluginExpression (producing
generatedText using pluginExpression.replace(/\$N/g, ...)) wrongly rewrites
literal SQL like sql.literalValue("$N"); change this to a token-aware
replacement that only renumbers real plugin placeholders: either (a) switch to a
sentinel that cannot occur in emitted SQL (e.g., a unique token inserted by the
plugin) and replace that sentinel, or (b) parse/scan pluginExpression for bare
$N tokens that are not inside string literals (use a simple tokenizer or regex
that tracks quote boundaries) and only replace those occurrences with
`$${++$idx}`. Update the logic around pluginExpression/generateText and keep
$idx increment behavior the same so only true plugin placeholders are renumbered
(ensure sql.literalValue("$N") remains untouched).
In `@packages/plugins/slonik/src/plugin.ts`:
- Around line 223-250: resolveIdentifierFragmentSql() and resolveFragmentSql()
can recurse indefinitely on alias cycles (e.g., const a = b; const b = a;); add
a visited Set<ts.Node | string> passed as an extra parameter to both functions,
mark each node before descending, and check the set to immediately return
false/undefined when a node is already visited; update all calls to
resolveFragmentSql(...) and resolveIdentifierFragmentSql(...) to pass the same
visited set so cycles are detected and recursion stops.
- Around line 59-61: translateCallExpression is currently dispatching on method
names alone and can rewrite non-Slonik helpers into tokens; to fix, update the
onExpression call sites (including the block in onExpression and the similar
block covering lines ~87–114) to pass context.tsNode and context.checker into
translateCallExpression, and inside translateCallExpression perform an early
guard using isSlonikMethod(tsNode, checker, node) (or equivalent) to ensure the
target object is a Slonik symbol before switching on method names; also mirror
the same Slonik symbol check used by resolveFragmentFactoryCall so only true
Slonik methods (e.g., jsonb, identifier, array) are translated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ec97956-2eff-4d44-9c26-7fbc7500c39b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/slonik-plugin-public.mddemos/plugin-slonik/eslint.config.jsdemos/plugin-slonik/package.jsondemos/plugin-slonik/src/index.tsdemos/plugin-slonik/tsconfig.jsondocs/compatibility/postgres.js.mddocs/compatibility/slonik.mdpackages/eslint-plugin/package.jsonpackages/eslint-plugin/src/rules/check-sql.rule.tspackages/eslint-plugin/src/rules/check-sql.test.tspackages/eslint-plugin/src/rules/check-sql.utils.tspackages/eslint-plugin/src/rules/ts-fixture/skip-target-plugin.tspackages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.tspackages/eslint-plugin/src/utils/ts-pg.utils.tspackages/plugin-utils/build.config.tspackages/plugin-utils/package.jsonpackages/plugin-utils/src/resolve.tspackages/plugins/slonik/build.config.tspackages/plugins/slonik/package.jsonpackages/plugins/slonik/src/index.tspackages/plugins/slonik/src/plugin.integration.test.tspackages/plugins/slonik/src/plugin.test.tspackages/plugins/slonik/src/plugin.tspackages/plugins/slonik/src/ts-fixture/file.tspackages/plugins/slonik/src/ts-fixture/tsconfig.jsonpackages/plugins/slonik/tsconfig.jsonpackages/zod-annotator/package.json
✅ Files skipped from review due to trivial changes (18)
- demos/plugin-slonik/tsconfig.json
- packages/zod-annotator/package.json
- packages/plugins/slonik/src/ts-fixture/tsconfig.json
- docs/compatibility/postgres.js.md
- packages/plugins/slonik/src/ts-fixture/file.ts
- packages/plugin-utils/package.json
- packages/plugins/slonik/src/index.ts
- packages/eslint-plugin/package.json
- .changeset/slonik-plugin-public.md
- packages/plugin-utils/build.config.ts
- packages/plugins/slonik/build.config.ts
- demos/plugin-slonik/package.json
- demos/plugin-slonik/eslint.config.js
- packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts
- packages/plugins/slonik/src/plugin.test.ts
- packages/plugins/slonik/tsconfig.json
- packages/plugins/slonik/package.json
- packages/plugins/slonik/src/plugin.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/plugin-utils/src/resolve.ts
- docs/compatibility/slonik.md
- demos/plugin-slonik/src/index.ts
- packages/eslint-plugin/src/rules/check-sql.rule.ts
Newbie012
force-pushed
the
feat-slonik-plugin
branch
from
ee7d695 to
aa7f14c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/plugin-utils/src/resolve.ts (1)
62-68:⚠️ Potential issue | 🟠 MajorInclude the resolved local path in the cache key.
Local plugins are loaded relative to
projectDir, but the cache key is still built from the rawdescriptor.packagestring. In a monorepo, two packages that both use./safeql-plugin.tswith the same config can reuse the first loaded module instead of resolving against their own package root. Because the ESLint rule keeps a module-scopedPluginManager, this leak survives across files in the same lint run.🧩 One way to fix it
- private getCacheKey(descriptor: PluginDescriptor): string { - return `${descriptor.package}:${stableStringify(descriptor.config ?? {})}`; + private getCacheKey(descriptor: PluginDescriptor, projectDir?: string): string { + const packageKey = + projectDir !== undefined && isLocalPath(descriptor.package) + ? path.resolve(projectDir, descriptor.package) + : descriptor.package; + return `${packageKey}:${stableStringify(descriptor.config ?? {})}`; }- const key = this.getCacheKey(descriptor); + const key = this.getCacheKey(descriptor, projectDir);Apply the same normalization anywhere else that deletes from this cache.
Also applies to: 78-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/src/resolve.ts` around lines 62 - 68, The cache key for plugins must include the resolved local path so locally referenced plugins loaded relative to different projectDir values don’t collide; in resolveOneSync (and similarly resolveOneAsync / any delete-from-cache sites), call the resolver that computes the actual module path (the same logic used by loadModuleSync or loadModuleAsync) before constructing/looking up the cache key, and include that resolved absolute path (or a normalized form of it) alongside descriptor.package/config when calling getCacheKey or building the key; also update any cache deletion code to use the same normalized resolved path so add/remove use identical keys (refer to resolveOneSync, loadModuleSync and other resolveOne* methods and the places that delete from pluginCache).packages/eslint-plugin/src/rules/check-sql.rule.ts (1)
157-172:⚠️ Potential issue | 🟠 MajorDon’t short-circuit every configured target once
onTarget()matches.This branch reports immediately for each configured target whenever a plugin returns a
TargetMatch. With multipletargets, the same query gets checked more than once, and transforms/field transforms from non-matching targets can leak into the diagnostic.🧭 One way to keep plugin context without duplicating reports
- if (params.pluginCtx.targetMatch !== undefined) { - const target: ConnectionTarget = { - ...params.target, - skipTypeAnnotations: - params.pluginCtx.targetMatch.skipTypeAnnotations ?? params.target.skipTypeAnnotations, - }; - return reportCheck({ - context: params.context, - tag: params.tag, - connection: params.connection, - target, - projectDir: params.projectDir, - baseNode: params.tag.tag, - typeParameter: params.tag.typeArguments, - pluginCtx: params.pluginCtx, - }); - } + const target: ConnectionTarget = + params.pluginCtx.targetMatch !== undefined + ? { + ...params.target, + skipTypeAnnotations: + params.pluginCtx.targetMatch.skipTypeAnnotations ?? params.target.skipTypeAnnotations, + } + : params.target; - if ("tag" in params.target) { - return checkConnectionByTagExpression({ ...params, target: params.target }); + if ("tag" in target) { + return checkConnectionByTagExpression({ ...params, target }); } - if ("wrapper" in params.target) { - return checkConnectionByWrapperExpression({ ...params, target: params.target }); + if ("wrapper" in target) { + return checkConnectionByWrapperExpression({ ...params, target }); }Keep the synthetic no-target path in
check(...)for plugin-only connections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/check-sql.rule.ts` around lines 157 - 172, The current branch returns early when params.pluginCtx.targetMatch is set, causing reportCheck to run immediately and duplicate checks across configured targets; instead, do not return—merge the matched target info into the plugin context or create a synthetic "no-target" ConnectionTarget and let the existing check(...) flow handle reporting once. Concretely: in the block that currently builds target from params.target and params.pluginCtx.targetMatch, update params.pluginCtx (or a local pluginCtx copy) to include the resolved targetMatch/skipTypeAnnotations and then call check(...) (or fall through to the existing check path) rather than calling and returning reportCheck directly; keep reportCheck only where the original no-target/reporting logic expects it so transforms from non-matching targets cannot leak into diagnostics (refer to params.pluginCtx.targetMatch, ConnectionTarget construction, reportCheck, and the check(...) flow).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/plugin-slonik/src/index.ts`:
- Around line 103-106: The schema for the "typed array" example is wrong: the
query SELECT ${sql.array([1, 2, 3], "int4")} AS a returns an array but the
validator uses z.number().nullable(). Update the schema in the example("typed
array") call to expect an array of numbers (e.g., change z.object({ a:
z.number().nullable() }) to z.object({ a: z.array(z.number()).nullable() }) or
z.object({ a: z.array(z.number()) }) as appropriate), leaving the pool.one,
sql.type, and sql.array(...) usage unchanged.
- Line 5: The current usage of pool (const pool = await createPool(...)) fires
queries with pool.one()/pool.query() inside section() and example() without
awaiting their async results, causing unhandled rejections and leaving the pool
open; update the section() and example() functions to accept and await async
callbacks (i.e., call await callback()), change all top-level
section()/example() invocations to be awaited via top-level await or an (async
() => { ... })() IIFE, and ensure you call await pool.end() in a finally block
after all examples complete so the connection pool is closed cleanly.
In `@packages/plugins/slonik/src/plugin.ts`:
- Around line 245-266: The bug is that the shared Set named visited is mutated
and reused across independent fragment resolutions (e.g., resolveFragmentSql and
resolveIdentifierFragmentSql), causing false cyclic detection when the same
fragment is referenced more than once; fix it by scoping the cycle-guard Set to
each independent resolution path: when calling resolveIdentifierFragmentSql or
recursing via resolveFragmentSql, either create a fresh Set for that top-level
resolution or push/pop markers (clone or copy the Set) before descending and
restore it afterwards so each descent has its own local visited context; update
calls in resolveFragmentSql, resolveIdentifierFragmentSql, and the code paths
handling VariableDeclaration/BindingElement initializers to pass or derive a
local visited instance instead of mutating a shared one.
- Around line 103-107: The translation currently accepts partially static token
arguments (in the "array" and "unnest" cases and in
translateIdentifierCall/translateUnnestCall), which can produce SQL that Slonik
will not execute; change these helpers to fail closed by returning false
whenever any required argument segment is not a static string or the exact
expected literal type — e.g., in the "array" case only accept when
node.arguments[1] is a Literal with a string type name and otherwise return
false, in the "unnest" case only accept when every type in the provided list is
a Literal string before calling translateUnnestCall, and in
translateIdentifierCall ensure every identifier path segment is a static string
(return false if any segment is dynamic); apply the same strict checks to the
other affected ranges (lines ~121-141 and ~419-425) so partially static
arguments are rejected rather than rewritten.
- Around line 206-207: joinIdentifierPath currently wraps each part in double
quotes but doesn't escape embedded double quotes, producing invalid SQL for
inputs like foo"bar; update joinIdentifierPath (function
joinIdentifierPath(parts: readonly string[])) to escape any " inside each part
by replacing " with "" (SQL standard doubling) before wrapping with surrounding
quotes, preserving the existing behavior of returning false for empty arrays.
---
Outside diff comments:
In `@packages/eslint-plugin/src/rules/check-sql.rule.ts`:
- Around line 157-172: The current branch returns early when
params.pluginCtx.targetMatch is set, causing reportCheck to run immediately and
duplicate checks across configured targets; instead, do not return—merge the
matched target info into the plugin context or create a synthetic "no-target"
ConnectionTarget and let the existing check(...) flow handle reporting once.
Concretely: in the block that currently builds target from params.target and
params.pluginCtx.targetMatch, update params.pluginCtx (or a local pluginCtx
copy) to include the resolved targetMatch/skipTypeAnnotations and then call
check(...) (or fall through to the existing check path) rather than calling and
returning reportCheck directly; keep reportCheck only where the original
no-target/reporting logic expects it so transforms from non-matching targets
cannot leak into diagnostics (refer to params.pluginCtx.targetMatch,
ConnectionTarget construction, reportCheck, and the check(...) flow).
In `@packages/plugin-utils/src/resolve.ts`:
- Around line 62-68: The cache key for plugins must include the resolved local
path so locally referenced plugins loaded relative to different projectDir
values don’t collide; in resolveOneSync (and similarly resolveOneAsync / any
delete-from-cache sites), call the resolver that computes the actual module path
(the same logic used by loadModuleSync or loadModuleAsync) before
constructing/looking up the cache key, and include that resolved absolute path
(or a normalized form of it) alongside descriptor.package/config when calling
getCacheKey or building the key; also update any cache deletion code to use the
same normalized resolved path so add/remove use identical keys (refer to
resolveOneSync, loadModuleSync and other resolveOne* methods and the places that
delete from pluginCache).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adcbd1a5-a926-4616-99a0-9a2d2a77ac34
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.changeset/slonik-plugin-public.mddemos/plugin-slonik/eslint.config.jsdemos/plugin-slonik/package.jsondemos/plugin-slonik/src/index.tsdemos/plugin-slonik/tsconfig.jsondocs/compatibility/postgres.js.mddocs/compatibility/slonik.mdpackages/eslint-plugin/package.jsonpackages/eslint-plugin/src/rules/check-sql.rule.tspackages/eslint-plugin/src/rules/check-sql.test.tspackages/eslint-plugin/src/rules/check-sql.utils.tspackages/eslint-plugin/src/rules/ts-fixture/skip-target-plugin.tspackages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.tspackages/eslint-plugin/src/utils/ts-pg.utils.test.tspackages/eslint-plugin/src/utils/ts-pg.utils.tspackages/plugin-utils/build.config.tspackages/plugin-utils/package.jsonpackages/plugin-utils/src/resolve.tspackages/plugins/slonik/build.config.tspackages/plugins/slonik/package.jsonpackages/plugins/slonik/src/index.tspackages/plugins/slonik/src/plugin.integration.test.tspackages/plugins/slonik/src/plugin.test.tspackages/plugins/slonik/src/plugin.tspackages/plugins/slonik/src/ts-fixture/file.tspackages/plugins/slonik/src/ts-fixture/tsconfig.jsonpackages/plugins/slonik/tsconfig.jsonpackages/zod-annotator/package.json
✅ Files skipped from review due to trivial changes (17)
- docs/compatibility/postgres.js.md
- demos/plugin-slonik/tsconfig.json
- packages/plugins/slonik/src/ts-fixture/file.ts
- packages/plugin-utils/package.json
- packages/plugins/slonik/src/index.ts
- packages/plugin-utils/build.config.ts
- packages/plugins/slonik/tsconfig.json
- packages/zod-annotator/package.json
- packages/plugins/slonik/src/ts-fixture/tsconfig.json
- demos/plugin-slonik/eslint.config.js
- packages/plugins/slonik/build.config.ts
- demos/plugin-slonik/package.json
- .changeset/slonik-plugin-public.md
- packages/plugins/slonik/package.json
- packages/plugins/slonik/src/plugin.integration.test.ts
- packages/eslint-plugin/package.json
- packages/eslint-plugin/src/rules/check-sql.utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/eslint-plugin/src/rules/ts-fixture/skip-target-plugin.ts
- packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts
Newbie012
force-pushed
the
feat-slonik-plugin
branch
from
aa7f14c to
554251b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/eslint-plugin/src/utils/ts-pg.utils.ts (1)
216-228: Consider PostgreSQL backslash-escape edge case in quoted literals.The
findQuotedLiteralEndfunction handles SQL-standard doubled quotes ('',"") but not C-style backslash escapes (\'). Whenstandard_conforming_stringsis off (rare in modern PostgreSQL) or in E-prefixed strings (E'...'), a\'would prematurely end scanning.This is a minor edge case since most codebases use standard-conforming strings, but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/utils/ts-pg.utils.ts` around lines 216 - 228, The current quoted-literal scanning misses C-style backslash-escaped quotes (e.g., E'\\'') and will stop early; update the finder used by the callers (findQuotedLiteralEnd) to treat a backslash before the quote character as an escape by skipping the backslash and the escaped character instead of terminating on the quote, i.e., when scanning inside findQuotedLiteralEnd handle '\' followed by quoteChar (and '\\') as part of the literal, and ensure callers (the text[cursor] checks) still call findQuotedLiteralEnd for both single and double quotes so backslash-escaping is respected for E-prefixed/non-standard strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/eslint-plugin/src/utils/ts-pg.utils.ts`:
- Around line 216-228: The current quoted-literal scanning misses C-style
backslash-escaped quotes (e.g., E'\\'') and will stop early; update the finder
used by the callers (findQuotedLiteralEnd) to treat a backslash before the quote
character as an escape by skipping the backslash and the escaped character
instead of terminating on the quote, i.e., when scanning inside
findQuotedLiteralEnd handle '\' followed by quoteChar (and '\\') as part of the
literal, and ensure callers (the text[cursor] checks) still call
findQuotedLiteralEnd for both single and double quotes so backslash-escaping is
respected for E-prefixed/non-standard strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f76bf11-4c2f-4643-8fee-62deb1273ab5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.changeset/slonik-plugin-public.mddemos/plugin-slonik/eslint.config.jsdemos/plugin-slonik/package.jsondemos/plugin-slonik/src/index.tsdemos/plugin-slonik/tsconfig.jsondocs/compatibility/postgres.js.mddocs/compatibility/slonik.mdpackages/eslint-plugin/package.jsonpackages/eslint-plugin/src/rules/check-sql.rule.tspackages/eslint-plugin/src/rules/check-sql.test.tspackages/eslint-plugin/src/rules/check-sql.utils.tspackages/eslint-plugin/src/rules/ts-fixture/skip-target-plugin.tspackages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.tspackages/eslint-plugin/src/utils/ts-pg.utils.test.tspackages/eslint-plugin/src/utils/ts-pg.utils.tspackages/plugin-utils/build.config.tspackages/plugin-utils/package.jsonpackages/plugin-utils/src/resolve.tspackages/plugins/slonik/build.config.tspackages/plugins/slonik/package.jsonpackages/plugins/slonik/src/index.tspackages/plugins/slonik/src/plugin.integration.test.tspackages/plugins/slonik/src/plugin.test.tspackages/plugins/slonik/src/plugin.tspackages/plugins/slonik/src/ts-fixture/file.tspackages/plugins/slonik/src/ts-fixture/tsconfig.jsonpackages/plugins/slonik/tsconfig.jsonpackages/zod-annotator/package.json
✅ Files skipped from review due to trivial changes (17)
- packages/plugins/slonik/src/ts-fixture/file.ts
- demos/plugin-slonik/tsconfig.json
- packages/plugins/slonik/tsconfig.json
- packages/plugin-utils/package.json
- packages/eslint-plugin/package.json
- packages/plugins/slonik/src/index.ts
- packages/zod-annotator/package.json
- docs/compatibility/postgres.js.md
- packages/plugin-utils/build.config.ts
- packages/plugins/slonik/src/ts-fixture/tsconfig.json
- .changeset/slonik-plugin-public.md
- demos/plugin-slonik/eslint.config.js
- packages/plugins/slonik/build.config.ts
- demos/plugin-slonik/package.json
- docs/compatibility/slonik.md
- packages/plugins/slonik/package.json
- packages/plugin-utils/src/resolve.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/eslint-plugin/src/utils/ts-pg.utils.test.ts
- packages/eslint-plugin/src/rules/ts-fixture/skip-target-plugin.ts
- packages/eslint-plugin/src/rules/ts-fixture/sourcemap-plugin.ts
- packages/eslint-plugin/src/rules/check-sql.rule.ts
- packages/eslint-plugin/src/rules/check-sql.test.ts
- packages/plugins/slonik/src/plugin.integration.test.ts
- packages/eslint-plugin/src/rules/check-sql.utils.ts
- packages/plugins/slonik/src/plugin.test.ts
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.
closes #295
closes #286
closes #285
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores