Feat/postgresql sql optimization#158
Closed
seba3567 wants to merge 4 commits into
Closed
Conversation
📝 WalkthroughWalkthroughSe añaden tres nuevas skills al registro ( CambiosAutoskills: nuevas skills, detección PostgreSQL mejorada y combo svelte-shadcn
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (6)
packages/autoskills/skills-registry/postgres-best-practices/SKILL.md (1)
6-14: ⚖️ Poor tradeoffConsiderar expandir el contenido introductorio del SKILL.md.
El archivo es muy minimal, con solo una tabla de referencias. Para mejorar la experiencia del usuario y darle contexto inmediato sin necesidad de navegar a referencias externas, sería recomendable agregar al menos una breve descripción de cuándo se debe usar esta skill, casos de uso comunes, o un flujo de trabajo sugerido en las secciones iniciales.
Comparar con otros SKILL.md en el registro para verificar si este patrón es consistente con el resto.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/skills-registry/postgres-best-practices/SKILL.md` around lines 6 - 14, The SKILL.md file for Postgres Best Practices contains only a minimal references table without introductory context. Expand the content after the header to include: a brief description of when this skill should be used, common use cases, and a suggested workflow before the references table. Review other SKILL.md files in the skills-registry to ensure the added content follows a consistent structure and pattern with the rest of the registry, then apply that same pattern to the postgres-best-practices/SKILL.md file to maintain consistency across the documentation.packages/autoskills/skills-registry/sql-optimization/SKILL.md (2)
207-212: ⚡ Quick winAclarar la sintaxis de covering indexes específica para cada base de datos.
La línea 210 usa
INCLUDEque es sintaxis específica de SQL Server y no funcionará en PostgreSQL o MySQL. El comentario en línea 211 ("Other databases") es vago. Debería ser explícito:
- SQL Server: Use
INCLUDEpara covering indexes- PostgreSQL/MySQL: Use múltiples columnas en el índice (no hay
INCLUDE)Ejemplo mejorado:
-- SQL Server CREATE INDEX idx_orders_covering ON orders(customer_id, created_at) INCLUDE (total_amount, status); -- PostgreSQL / MySQL CREATE INDEX idx_orders_covering ON orders(customer_id, created_at, total_amount, status);Esto evita confusión a desarrolladores que usen PostgreSQL o MySQL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/skills-registry/sql-optimization/SKILL.md` around lines 207 - 212, The covering index example in the SKILL.md file uses SQL Server's INCLUDE syntax which is not compatible with PostgreSQL or MySQL, and the inline comment referencing "Other databases" is too vague. Replace the current single example with clear, database-specific examples that explicitly show the syntax differences: for SQL Server use the INCLUDE clause syntax, and for PostgreSQL and MySQL show the syntax using multiple columns in the index without the INCLUDE keyword. This will eliminate confusion for developers using different database systems.
224-249: ⚡ Quick winAgregar requisitos previos para las queries de monitoreo.
Las queries de ejemplo para MySQL, PostgreSQL y SQL Server son técnicamente correctas, pero asumen que las herramientas de monitoreo están habilitadas:
- MySQL: Requiere
slow_query_log = 1en my.cnf- PostgreSQL: Requiere
pg_stat_statementsenshared_preload_libraries(requiere reinicio)- SQL Server: Está habilitado por defecto
Sería útil agregar una nota sobre estos requisitos previos para que el desarrollador sepa si necesita configuración adicional antes de ejecutar estas queries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/skills-registry/sql-optimization/SKILL.md` around lines 224 - 249, The Query Performance Analysis section provides example SQL queries for MySQL, PostgreSQL, and SQL Server but does not document the prerequisite configuration required for these queries to function correctly. Add a prerequisites or requirements note before the code block examples that clearly states: for MySQL that slow_query_log must be enabled in my.cnf, for PostgreSQL that pg_stat_statements extension must be added to shared_preload_libraries and requires a database restart, and for SQL Server that monitoring is enabled by default. This helps developers understand what setup steps may be needed before executing these monitoring queries.packages/autoskills/skills-registry/postgres-best-practices/references/schema-design.md (1)
7-9: ⚡ Quick winAgregar contexto sobre trade-offs y excepciones a las recomendaciones.
Las recomendaciones de tipos de datos son técnicamente sólidas, pero carecen de contexto sobre:
- text vs varchar(n): Mencionar que varchar(n) sigue siendo apropiado cuando hay una restricción de dominio explícita (ej. códigos de país, códigos de estado).
- uuid: Aclarar los trade-offs (mayor tamaño de almacenamiento: 16 vs 8 bytes; peor rendimiento en índices comparado con secuenciales; pérdida de ordenamiento natural). Ayuda a que el desarrollador sepa cuándo usar uuid y cuándo preferir bigint.
- timestamptz: Buena recomendación; quizás mencionar que siempre debe usar timestamptz a menos que haya una razón muy específica para no hacerlo.
Esto haría la guía más equilibrada y ayudaría a evitar decisiones de diseño poco óptimas.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/skills-registry/postgres-best-practices/references/schema-design.md` around lines 7 - 9, Expand the three schema design recommendations in the schema-design.md file to include context about trade-offs and exceptions. For the text vs varchar(n) guideline, add examples of when varchar(n) remains appropriate such as domain-constrained values like country codes or state codes. For the uuid recommendation, add a dedicated section explaining the trade-offs including larger storage footprint (16 bytes vs 8 bytes for sequential integers), potential index performance degradation compared to sequential IDs, and loss of natural ordering, then clarify when developers should consider bigint as an alternative. For the timestamptz recommendation, reinforce it as the default choice while noting that exceptions exist only in rare specific cases. This will transform the guide from prescriptive rules into balanced guidance that helps developers make context-aware decisions.packages/autoskills/tests/detect.test.ts (1)
1385-1389: ⚡ Quick winAñade aserción negativa para validar la separación
postgresqlvspostgres-ruby.El caso nuevo verifica inclusión de
postgresql, pero no comprueba que no entrepostgres-rubyen el flujo npm.✅ Propuesta de test
it("detects PostgreSQL from pg npm package", () => { writePackageJson(tmp.path, { dependencies: { pg: "^8.0.0" } }); const { detected } = detectTechnologies(tmp.path); ok(detected.some((t) => t.id === "postgresql")); + ok(!detected.some((t) => t.id === "postgres-ruby")); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/tests/detect.test.ts` around lines 1385 - 1389, The test case "detects PostgreSQL from pg npm package" verifies that the postgresql technology is detected when the pg npm package is present, but it does not check that postgres-ruby (which is a Ruby-specific technology) is NOT detected in an npm context. Add a negative assertion after the existing assertion in this test to verify that detected does not include postgres-ruby, ensuring proper separation between the postgresql and postgres-ruby technology detection paths.packages/autoskills/skills-registry/index.json (1)
3-3: Los commit SHAs de los repositorios fuente son válidos.Los valores
commitShahan sido verificados y apuntan a commits válidos en los repositorios indicados (huntabyte/shadcn-svelte,neondatabase/postgres-skills,github/awesome-copilot).Sin embargo, los hashes
sha256ybundleHashpara las tres nuevas skills aún requieren validación. Sin acceso a los archivos de skill reales, no es posible confirmar que estos hashes coincidan con el contenido actual de los artefactos en los commits especificados.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/skills-registry/index.json` at line 3, The commit SHAs in the skills registry have been verified as valid, but the sha256 and bundleHash values for the three new skills require validation against the actual artifact contents. Locate the three new skill entries in the index.json file and verify that each sha256 and bundleHash value accurately matches the content of the corresponding artifacts in their specified commits (huntabyte/shadcn-svelte, neondatabase/postgres-skills, and github/awesome-copilot). Update these hash values to ensure they correctly represent the current artifact contents, which is critical for integrity verification when these skills are downloaded and used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/autoskills/skills-map.ts`:
- Around line 926-931: The PostgreSQL skill detection is too broad and causes
false positives in projects using other databases. Remove sequelize, typeorm,
and mikro-orm from the packages array in the detect object of the PostgreSQL
skill definition (around line 930). These are multi-database ORMs that support
MySQL and SQLite too, so they should not be used as signals for
PostgreSQL-specific skill installation. Keep only the PostgreSQL-specific
packages like pg, postgres, and pg-promise in the detection list.
In `@packages/autoskills/skills-registry/index.json`:
- Around line 13022-13121: The three new skills in
packages/autoskills/skills-registry/index.json have contradictory review
metadata where review.status is marked as "approved" but the review.summary
indicates the review was skipped with --no-review flag. For shadcn-svelte
(review field around line 13053-13059), change review.status from "approved" to
"skipped" and update review.summary from "review skipped (--no-review)" to
accurately reflect that no review was performed. Apply the same fix to
postgres-best-practices (review field around line 13081-13087) and
sql-optimization (review field around line 13107-13113). This ensures the review
metadata is truthful and consistent with downstream code that validates skills
based on review.status === "approved".
---
Nitpick comments:
In `@packages/autoskills/skills-registry/index.json`:
- Line 3: The commit SHAs in the skills registry have been verified as valid,
but the sha256 and bundleHash values for the three new skills require validation
against the actual artifact contents. Locate the three new skill entries in the
index.json file and verify that each sha256 and bundleHash value accurately
matches the content of the corresponding artifacts in their specified commits
(huntabyte/shadcn-svelte, neondatabase/postgres-skills, and
github/awesome-copilot). Update these hash values to ensure they correctly
represent the current artifact contents, which is critical for integrity
verification when these skills are downloaded and used.
In
`@packages/autoskills/skills-registry/postgres-best-practices/references/schema-design.md`:
- Around line 7-9: Expand the three schema design recommendations in the
schema-design.md file to include context about trade-offs and exceptions. For
the text vs varchar(n) guideline, add examples of when varchar(n) remains
appropriate such as domain-constrained values like country codes or state codes.
For the uuid recommendation, add a dedicated section explaining the trade-offs
including larger storage footprint (16 bytes vs 8 bytes for sequential
integers), potential index performance degradation compared to sequential IDs,
and loss of natural ordering, then clarify when developers should consider
bigint as an alternative. For the timestamptz recommendation, reinforce it as
the default choice while noting that exceptions exist only in rare specific
cases. This will transform the guide from prescriptive rules into balanced
guidance that helps developers make context-aware decisions.
In `@packages/autoskills/skills-registry/postgres-best-practices/SKILL.md`:
- Around line 6-14: The SKILL.md file for Postgres Best Practices contains only
a minimal references table without introductory context. Expand the content
after the header to include: a brief description of when this skill should be
used, common use cases, and a suggested workflow before the references table.
Review other SKILL.md files in the skills-registry to ensure the added content
follows a consistent structure and pattern with the rest of the registry, then
apply that same pattern to the postgres-best-practices/SKILL.md file to maintain
consistency across the documentation.
In `@packages/autoskills/skills-registry/sql-optimization/SKILL.md`:
- Around line 207-212: The covering index example in the SKILL.md file uses SQL
Server's INCLUDE syntax which is not compatible with PostgreSQL or MySQL, and
the inline comment referencing "Other databases" is too vague. Replace the
current single example with clear, database-specific examples that explicitly
show the syntax differences: for SQL Server use the INCLUDE clause syntax, and
for PostgreSQL and MySQL show the syntax using multiple columns in the index
without the INCLUDE keyword. This will eliminate confusion for developers using
different database systems.
- Around line 224-249: The Query Performance Analysis section provides example
SQL queries for MySQL, PostgreSQL, and SQL Server but does not document the
prerequisite configuration required for these queries to function correctly. Add
a prerequisites or requirements note before the code block examples that clearly
states: for MySQL that slow_query_log must be enabled in my.cnf, for PostgreSQL
that pg_stat_statements extension must be added to shared_preload_libraries and
requires a database restart, and for SQL Server that monitoring is enabled by
default. This helps developers understand what setup steps may be needed before
executing these monitoring queries.
In `@packages/autoskills/tests/detect.test.ts`:
- Around line 1385-1389: The test case "detects PostgreSQL from pg npm package"
verifies that the postgresql technology is detected when the pg npm package is
present, but it does not check that postgres-ruby (which is a Ruby-specific
technology) is NOT detected in an npm context. Add a negative assertion after
the existing assertion in this test to verify that detected does not include
postgres-ruby, ensuring proper separation between the postgresql and
postgres-ruby technology detection paths.
🪄 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 Plus
Run ID: a242f8ae-c2f1-496a-b051-2955b67726c7
⛔ Files ignored due to path filters (2)
packages/autoskills/skills-registry/shadcn-svelte/assets/shadcn-svelte-small.pngis excluded by!**/*.pngpackages/autoskills/skills-registry/shadcn-svelte/assets/shadcn-svelte.pngis excluded by!**/*.png
📒 Files selected for processing (18)
packages/autoskills/lib.tspackages/autoskills/scripts/sync-skills.mjspackages/autoskills/skills-map.tspackages/autoskills/skills-registry/index.jsonpackages/autoskills/skills-registry/postgres-best-practices/SKILL.mdpackages/autoskills/skills-registry/postgres-best-practices/references/schema-design.mdpackages/autoskills/skills-registry/shadcn-svelte/SKILL.mdpackages/autoskills/skills-registry/shadcn-svelte/agents/openai.ymlpackages/autoskills/skills-registry/shadcn-svelte/cli.mdpackages/autoskills/skills-registry/shadcn-svelte/customization.mdpackages/autoskills/skills-registry/shadcn-svelte/evals/evals.jsonpackages/autoskills/skills-registry/shadcn-svelte/rules/composition.mdpackages/autoskills/skills-registry/shadcn-svelte/rules/forms.mdpackages/autoskills/skills-registry/shadcn-svelte/rules/icons.mdpackages/autoskills/skills-registry/shadcn-svelte/rules/styling.mdpackages/autoskills/skills-registry/sql-optimization/SKILL.mdpackages/autoskills/tests/collect.test.tspackages/autoskills/tests/detect.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/autoskills/installer.ts`:
- Line 35: The new `"skipped"` status value added to the status type definition
is not being handled explicitly in the `securityCheckForEntry` security check
fallback logic, which currently uses binary checking (only checking for
`"flagged"` and treating all other cases as `"ok"`). Update the
`securityCheckForEntry` function to implement exhaustive mapping that explicitly
handles all three status values: `"approved"`, `"flagged"`, and `"skipped"`,
ensuring that skipped checks do not implicitly receive an `"ok"` security status
and instead are mapped to an appropriate security signal that reflects the lack
of security review.
🪄 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 Plus
Run ID: 018aa3f5-e4ac-4981-bca6-33c7007906fb
📒 Files selected for processing (3)
packages/autoskills/installer.tspackages/autoskills/skills-map.tspackages/autoskills/skills-registry/index.json
✅ Files skipped from review due to trivial changes (1)
- packages/autoskills/skills-registry/index.json
| bundleHash: string; | ||
| review: { | ||
| status: "approved" | "flagged"; | ||
| status: "approved" | "flagged" | "skipped"; |
There was a problem hiding this comment.
"skipped" queda clasificado implícitamente como "ok" en el fallback de seguridad.
Al añadir "skipped" en Line 35, el fallback de securityCheckForEntry (Lines 232-240) sigue siendo binario ("flagged" vs resto), por lo que una skill no revisada puede terminar con estado "ok" cuando falta securityCheck. Esto degrada la señal de seguridad.
Propuesta de ajuste (mapeo exhaustivo)
function securityCheckForEntry(skillName: string, entry: RegistryEntry): InstallSecurityCheck {
if (entry.securityCheck) {
return {
name: skillName,
status: entry.securityCheck.status,
summary: entry.securityCheck.summary,
findings: entry.securityCheck.findings,
};
}
+ const reviewStatus = entry.review?.status;
+ const status: InstallSecurityCheck["status"] =
+ reviewStatus === "flagged" || reviewStatus === "skipped" ? "warning" : "ok";
+
return {
name: skillName,
- status: entry.review?.status === "flagged" ? "warning" : "ok",
+ status,
summary:
entry.review?.summary ||
- (entry.review?.status === "flagged"
+ (reviewStatus === "flagged" || reviewStatus === "skipped"
? "The sync review found issues that should be checked."
: "The sync review did not find security issues."),
findings: entry.review?.flags || [],
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| status: "approved" | "flagged" | "skipped"; | |
| function securityCheckForEntry(skillName: string, entry: RegistryEntry): InstallSecurityCheck { | |
| if (entry.securityCheck) { | |
| return { | |
| name: skillName, | |
| status: entry.securityCheck.status, | |
| summary: entry.securityCheck.summary, | |
| findings: entry.securityCheck.findings, | |
| }; | |
| } | |
| const reviewStatus = entry.review?.status; | |
| const status: InstallSecurityCheck["status"] = | |
| reviewStatus === "flagged" || reviewStatus === "skipped" ? "warning" : "ok"; | |
| return { | |
| name: skillName, | |
| status, | |
| summary: | |
| entry.review?.summary || | |
| (reviewStatus === "flagged" || reviewStatus === "skipped" | |
| ? "The sync review found issues that should be checked." | |
| : "The sync review did not find security issues."), | |
| findings: entry.review?.flags || [], | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/autoskills/installer.ts` at line 35, The new `"skipped"` status
value added to the status type definition is not being handled explicitly in the
`securityCheckForEntry` security check fallback logic, which currently uses
binary checking (only checking for `"flagged"` and treating all other cases as
`"ok"`). Update the `securityCheckForEntry` function to implement exhaustive
mapping that explicitly handles all three status values: `"approved"`,
`"flagged"`, and `"skipped"`, ensuring that skipped checks do not implicitly
receive an `"ok"` security status and instead are mapped to an appropriate
security signal that reflects the lack of security review.
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.
What Changed
postgres-best-practicesde Neon Database para cubrir diseño de esquemas, índices y configuración de pools de conexión.sql-optimization(proveniente degithub/awesome-copilot) para guiar en la optimización universal de rendimiento, índices GIN y patrones de refactorización de consultas.postgresqlenskills-map.ts.postgres-rubycomo un ID de tecnología independiente (detectando la gemapgde Ruby) para evitar colisiones de configuración con los controladores de bases de datos de JavaScript/TypeScript (pg,sequelize,typeorm, etc.).skills-registry/index.json.Why This Change
Instrucciones de BD Explícitas: Los agentes de IA carecían de pautas precisas para optimizar bases de datos relacionales al trabajar en servicios backend. Esta adición garantiza que los asistentes escriban consultas seguras y configuren correctamente los pools de conexión.
postgres-ruby) asegura que la CLI detecte las dependencias correctas según el lenguaje del proyecto y no cargue skills redundantes.Summary by CodeRabbit
Summary by CodeRabbit
Notas de Lanzamiento