Skip to content

Make ADK table drops conditional in migrations and update tsconfig #73

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.

Sign up for GitHub

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

Merged
stenwire merged 2 commits into from
Jul 4, 2026
Merged

Make ADK table drops conditional in migrations and update tsconfig #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eacf43b
fix: make ADK table drops conditional in all migrations, exclude test…
stenwire Jul 4, 2026
46ecf47
Merge pull request #72 from stenwire/root-dir-dockerfile
stenwire Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions backend/alembic/versions/70d75bc52042_transactions_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@
def upgrade() -> None:
"""Upgrade schema."""
# ### commands auto generated by Alembic - please adjust! ###
op.drop_table('events')
op.drop_table('sessions')
op.drop_table('user_states')
op.drop_table('app_states')
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)
Comment on lines +24 to +28

@gemini-code-assist gemini-code-assist Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

with op.batch_alter_table('payment_transactions', schema=None) as batch_op:
batch_op.add_column(sa.Column('transaction_metadata', sa.JSON(), nullable=True))
batch_op.drop_column('metadata')
Expand Down
9 changes: 5 additions & 4 deletions backend/alembic/versions/dae11b7e812a_add_webhook_payload_field.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@
def upgrade() -> None:
"""Upgrade schema."""
# ### commands auto generated by Alembic - please adjust! ###
op.drop_table('user_states')
op.drop_table('app_states')
op.drop_table('events')
op.drop_table('sessions')
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)
Comment on lines +24 to +28

@gemini-code-assist gemini-code-assist Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

with op.batch_alter_table('payment_transactions', schema=None) as batch_op:
batch_op.add_column(sa.Column('raw_webhook_payload', sa.JSON(), nullable=True))

Expand Down
2 changes: 1 addition & 1 deletion frontend/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,5 @@
".next/dev/types/**/*.ts",
"**/*.mts"
],
"exclude": ["node_modules"]
"exclude": ["node_modules", "tests"]

@gemini-code-assist gemini-code-assist Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
"exclude": ["node_modules", "tests"]
"exclude": ["node_modules"]

}
Loading