-
Notifications
You must be signed in to change notification settings - Fork 0
fix: improve litestream support #558
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
Co-authored-by: WillBooster (Codex CLI) <[email protected]>
Co-authored-by: WillBooster (Codex CLI) <[email protected]>
Co-authored-by: WillBooster (Codex CLI) <[email protected]>
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 enhances Litestream support by introducing a new command to list backups and extending the data retention period for disaster recovery. It also refines the 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
このプルリクエストは、Litestreamのサポートを改善するための修正ですね。新しい list-backups コマンドの追加、バックアップ保持期間の延長(8時間から72時間へ)、そして reset コマンドの改善が含まれています。全体的に、Litestreamを利用したデータベース管理がより堅牢になり、使いやすさが向上する良い変更だと思います。
1点、コードの可読性を高めるためのリファクタリング提案をさせていただきました。ご確認ください。
I am having trouble creating individual review comments. Click here to see my feedback.
packages/wb/src/scripts/prismaScripts.ts (50-52)
この複数行にわたる三項演算子は、テンプレートリテラル内で直接記述することで、より簡潔に表現できます。これにより、コードが1行にまとまり、可読性が向上します。
project.packageJson.dependencies?.blitz の評価ロジックは49行目と重複していますが、この変更だけでも可読性は十分に向上します。もし可能であれば、この評価を一度だけ行い、結果を定数に格納して再利用することもご検討ください。
const resetCommandForTest = String.raw`find ${project.packageJson.dependencies?.blitz ? 'db' : 'prisma'} \( -name "test.db*" -o -name "test.sqlite*" \) -delete`;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 Litestream support by adding a new command to list backups, refactoring the reset logic to include test database cleanup, and extending backup retention from 8 hours to 3 days. The changes enhance database backup management and maintenance capabilities for projects using Litestream with SQLite databases.
Key Changes:
- Added new
list-backupscommand to view Litestream backup history - Refactored the
resetmethod to clean up test database files after reset operations - Increased Litestream backup retention period from 8 hours to 72 hours (3 days)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/wb/src/scripts/prismaScripts.ts |
Added listBackups method and refactored reset method to include test database cleanup |
packages/wb/src/commands/prisma.ts |
Added listBackupsCommand, reordered commands alphabetically, and updated retention configuration to 72h with explanatory comment |
Comments suppressed due to low confidence (2)
packages/wb/src/scripts/prismaScripts.ts:48
- When
resetOptionsis an empty string after trimming, the command will have a trailing space:PRISMA migrate reset --force. While this typically works in shell commands, it would be cleaner to conditionally include the options only when they're non-empty:PRISMA migrate reset --force${resetOptions ? ' ' + resetOptions : ''}.
const baseReset = `PRISMA migrate reset --force ${resetOptions}`;
packages/wb/src/scripts/prismaScripts.ts:52
- The variable name
resetCommandForTestis misleading. This variable contains a command to delete test database files, not a reset command for tests. A clearer name would bedeleteTestDbCommandorcleanupTestDbCommandto better reflect its purpose.
const resetCommandForTest = project.packageJson.dependencies?.blitz
? String.raw`find db \( -name "test.db*" -o -name "test.sqlite*" \) -delete`
: String.raw`find prisma \( -name "test.db*" -o -name "test.sqlite*" \) -delete`;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Close #
Self Check
All checks have passedon this page.WIP:.yarn test) passed.yarn lint) passed.