-
Notifications
You must be signed in to change notification settings - Fork 0
fix: improve prisma scripts #563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @exKAZUu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the reliability and robustness of Prisma-related utility scripts. The changes streamline database reset and deployment processes, particularly by ensuring consistent test database environments, preventing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
このプルリクエストは、Prisma関連のスクリプトを改善するものです。主な変更点は、テストデータベースのリセットを強制的に行う処理の追加、deploy-forceコマンドの簡略化、resetコマンドのリファクタリング、そしてSQLiteのアーティファクト削除処理の単純化です。
全体的に良い改善ですが、いくつかフィードバックがあります。process.envの変更による副作用の可能性と、冗長なコマンドの削除についてコメントしました。ご確認ください。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves Prisma database management scripts by refactoring the reset command logic, simplifying file cleanup operations, and adding automatic test database reset functionality.
Key Changes:
- Simplified the
resetmethod by removing test database cleanup logic (now handled separately) - Added proactive database removal in
deployForceto prevent migration reset failures - Introduced automatic test database reset after main database reset in the reset command handler
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/wb/src/scripts/prismaScripts.ts | Refactored deployForce to remove database before reset, simplified reset method logic by removing test database cleanup, and simplified removeSqliteArtifacts to use wildcard pattern |
| packages/wb/src/commands/prisma.ts | Added automatic test database reset logic when WB_ENV is not 'test' to ensure both production and test databases are reset together |
Comments suppressed due to low confidence (1)
packages/wb/src/scripts/prismaScripts.ts:122
- The use of
rm -Rf ${sqlitePath}*with a wildcard is potentially dangerous. IfsqlitePathis empty or evaluates to a short path, this could delete unintended files. Consider using more specific removal like the previous implementation that explicitly listed the files to remove (${sqlitePath},${sqlitePath}-shm,${sqlitePath}-wal,${sqlitePath}-litestream). Alternatively, add validation to ensure sqlitePath is not empty before executing the command.
return project.packageJson.dependencies?.blitz ? 'db/mount' : 'prisma/mount';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // `prisma migrate reset` sometimes fails if the existing database schema, so we remove it first. | ||
| // Don't skip "migrate deploy" because restored database may be older than the current schema. | ||
| return `PRISMA migrate reset --force --skip-seed && rm -Rf ${dirPath}/prod.sqlite3* | ||
| return `rm -Rf ${dirPath}/prod.sqlite3*; PRISMA migrate reset --force --skip-seed && rm -Rf ${dirPath}/prod.sqlite3* |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the security concern in the removeSqliteArtifacts method, using rm -Rf ${dirPath}/prod.sqlite3* with a wildcard could be dangerous if dirPath is empty or misconfigured. While getDatabaseDirPath returns fixed paths, consider adding validation or using more explicit file patterns to prevent accidental deletion of unintended files.
| // Force to reset test database | ||
| if (process.env.WB_ENV !== 'test') { | ||
| process.env.WB_ENV = 'test'; | ||
| for (const project of prepareForRunningCommand('WB_ENV=test prisma reset', await findPrismaProjects(argv))) { |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling await findPrismaProjects(argv) again inside the handler after already calling it at line 135 introduces redundant work. The function performs file system operations to find projects, which is unnecessary since the projects list is already available in allProjects. Consider reusing the allProjects variable instead of calling findPrismaProjects again.
| for (const project of prepareForRunningCommand('WB_ENV=test prisma reset', await findPrismaProjects(argv))) { | |
| for (const project of prepareForRunningCommand('WB_ENV=test prisma reset', allProjects)) { |
|
|
||
| deployForce(project: Project): string { | ||
| const dirPath = getDatabaseDirPath(project); | ||
| // `prisma migrate reset` sometimes fails if the existing database schema, so we remove it first. |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment has a grammatical error. It should read "if the existing database schema exists" or "with the existing database schema".
| // `prisma migrate reset` sometimes fails if the existing database schema, so we remove it first. | |
| // `prisma migrate reset` sometimes fails if the existing database schema exists, so we remove it first. |
| process.env.WB_ENV = 'test'; | ||
| for (const project of prepareForRunningCommand('WB_ENV=test prisma reset', await findPrismaProjects(argv))) { | ||
| await runWithSpawn(prismaScripts.reset(project, unknownOptions), project, argv); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutating process.env.WB_ENV at runtime in a command handler has potential side effects that could affect other parts of the application. This creates a stateful change that persists beyond this command execution. Consider using environment variable passing through the command execution instead of mutating the global process.env object, or document this side effect clearly if it's intentional.
| process.env.WB_ENV = 'test'; | |
| for (const project of prepareForRunningCommand('WB_ENV=test prisma reset', await findPrismaProjects(argv))) { | |
| await runWithSpawn(prismaScripts.reset(project, unknownOptions), project, argv); | |
| const testEnv = { ...process.env, WB_ENV: 'test' }; | |
| for (const project of prepareForRunningCommand('WB_ENV=test prisma reset', await findPrismaProjects(argv))) { | |
| await runWithSpawn(prismaScripts.reset(project, unknownOptions), project, argv, { env: testEnv }); |
Close #
Self Check
All checks have passedon this page.WIP:.yarn test) passed.yarn lint) passed.