Conversation
…s from tsconfig Migrations dae11b7e812a and 70d75bc52042 tried to drop app_states/events/ sessions/user_states unconditionally; fresh staging DBs don't have these ADK-managed tables so all three migrations now check existence first. Exclude the tests/ directory from Next.js tsconfig so the production Docker build doesn't fail on test-only type errors (e.g. framer-motion mock's children typed as unknown). Vitest uses its own resolution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix: make ADK table drops conditional in all migrations, exclude test…
|
🤖 Hi @stenwire, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This pull request introduces valuable improvements to the database migration scripts and the frontend TypeScript configuration. The changes enhance the robustness of database migrations by making table drops conditional and improve the build process by excluding test files from the TypeScript compilation.
🔍 General Feedback
- Positive Highlight: The conditional logic added to the Alembic migrations is a great practice. It prevents migration scripts from failing when run against a database schema that doesn't contain the tables to be dropped, making the deployment process more resilient.
- Good Practice: Excluding the
testsdirectory intsconfig.jsonis a correct and standard configuration for Next.js/TypeScript projects, ensuring that test files are not unnecessarily included in production builds.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies database migrations to conditionally drop tables by inspecting the connection, and updates the frontend TypeScript configuration to exclude the tests directory. The feedback points out that redundant table drops across sequential migrations are an anti-pattern and suggests either removing them or using Alembic's built-in if_exists=True parameter. Additionally, it is recommended not to exclude the tests directory from tsconfig.json to preserve IDE type-checking and autocompletion support for test files.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| conn = op.get_bind() | ||
| existing = sa.inspect(conn).get_table_names() | ||
| for tbl in ('events', 'sessions', 'user_states', 'app_states'): | ||
| if tbl in existing: | ||
| op.drop_table(tbl) |
There was a problem hiding this comment.
If these two migrations are sequential in the migration chain, once the tables are dropped in the first migration, they will not exist when the second one runs. Instead of making the drops conditional in both files, the best practice is to keep the drop_table commands in the migration where they are first introduced and completely remove them from the subsequent migration.
If conditional drops are indeed necessary, you can use the built-in if_exists=True parameter supported in Alembic 1.13.3+ instead of manually inspecting the connection.
| conn = op.get_bind() | |
| existing = sa.inspect(conn).get_table_names() | |
| for tbl in ('events', 'sessions', 'user_states', 'app_states'): | |
| if tbl in existing: | |
| op.drop_table(tbl) | |
| for tbl in ('events', 'sessions', 'user_states', 'app_states'): | |
| op.drop_table(tbl, if_exists=True) |
| conn = op.get_bind() | ||
| existing = sa.inspect(conn).get_table_names() | ||
| for tbl in ('user_states', 'app_states', 'events', 'sessions'): | ||
| if tbl in existing: | ||
| op.drop_table(tbl) |
There was a problem hiding this comment.
If this migration runs after 70d75bc52042_transactions_table.py, these tables will have already been dropped. Having redundant drop_table commands across multiple sequential migrations is an anti-pattern (often caused by running autogenerate before applying previous migrations locally).
To keep the migration history clean, these redundant drop commands should be removed entirely from this migration.
| "**/*.mts" | ||
| ], | ||
| "exclude": ["node_modules"] | ||
| "exclude": ["node_modules", "tests"] |
There was a problem hiding this comment.
Excluding the tests directory from the main tsconfig.json disables TypeScript type-checking, autocompletion, and path alias resolution for all test files in IDEs/editors.
If the goal is to exclude tests from the production build output, it is better to keep them in tsconfig.json to preserve IDE support, and instead use a separate configuration (e.g., tsconfig.build.json) for the production build, or rely on Next.js/Vite which automatically ignores test files during the build process.
| "exclude": ["node_modules", "tests"] | |
| "exclude": ["node_modules"] |
There was a problem hiding this comment.
📋 Security Analysis Summary
No security vulnerabilities were identified in this pull request. The changes primarily consist of database migration scripts and a configuration file update, which do not introduce any security risks.
🔍 General Feedback
- The database migration scripts are well-structured and use the Alembic API correctly.
- The changes are small and focused, which is good for maintainability.
1 participant
Description
Type of Change
Checklist
make lint-be/make lint-fe)make test-be/make test-fe)Screenshots / Demo
Related Issues