Conversation
kadyvillicana
commented
Mar 2, 2026
- Add AppVersionModel across both NDB and PostgreSQL backends to store minimum supported and latest app version info as a singleton entity
- Extend the inituser API endpoint to return app_version data (min_supported_version, latest_version, optional update_message), enabling the Explo mobile client to prompt or force users to update
- Fix Werkzeug reloader causing spurious "port in use" errors during local development
There was a problem hiding this comment.
Pull request overview
Adds app version metadata plumbing so the mobile client can prompt/force upgrades, and tweaks local dev startup to avoid Werkzeug reloader “port in use” noise.
Changes:
- Introduces an
AppVersionModelconcept for both NDB and PostgreSQL backends (singleton “app_version”). - Extends
/inituserto returnapp_versiondata (min_supported_version,latest_version, optionalupdate_message). - Avoids duplicate local port checks when Werkzeug’s reloader spawns a child process.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/skrafldb_pg.py |
Adds PostgreSQL facade AppVersionModel.get_versions() that reads from the PG repository layer. |
src/skrafldb_ndb.py |
Adds NDB AppVersionModel entity keyed by "app_version". |
src/api.py |
Extends /inituser response with app_version payload. |
src/main.py |
Skips port availability check in Werkzeug reloader child process. |
src/db/protocols.py |
Adds AppVersionRepositoryProtocol and DatabaseBackendProtocol.app_versions. |
src/db/postgresql/repositories.py |
Adds AppVersionRepository.get_versions() implementation. |
src/db/postgresql/models.py |
Adds AppVersion ORM model mapped to app_versions table. |
src/db/postgresql/backend.py |
Wires app_versions repository into the PostgreSQL backend. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class AppVersionRepositoryProtocol(Protocol): | ||
| """Protocol for app version repository operations.""" | ||
|
|
||
| def get_versions(self) -> Optional[Any]: |
There was a problem hiding this comment.
AppVersionRepositoryProtocol.get_versions() returns Optional[Any], which defeats static type checking and is inconsistent with the other repositories that return explicit *EntityProtocol types. Consider adding an AppVersionEntityProtocol (with at least key_id, min_supported_version, latest_version, update_message) and updating this protocol to return Optional[AppVersionEntityProtocol] so both backends can type-check cleanly.
| class AppVersionRepositoryProtocol(Protocol): | |
| """Protocol for app version repository operations.""" | |
| def get_versions(self) -> Optional[Any]: | |
| class AppVersionEntityProtocol(Protocol): | |
| """Protocol describing an app version entity.""" | |
| key_id: str | |
| min_supported_version: str | |
| latest_version: str | |
| update_message: str | |
| class AppVersionRepositoryProtocol(Protocol): | |
| """Protocol for app version repository operations.""" | |
| def get_versions(self) -> Optional[AppVersionEntityProtocol]: |
| @property | ||
| def app_versions(self) -> AppVersionRepositoryProtocol: | ||
| """Access the AppVersion repository.""" | ||
| ... |
There was a problem hiding this comment.
DatabaseBackendProtocol now exposes an app_versions repository, but the NDB backend (src/db/ndb/backend.py) does not currently implement this property. If CI runs pyright/mypy, this will break protocol conformance for the NDB backend and any code typed as DatabaseBackendProtocol. Add an AppVersionRepository to the NDB backend (even if it’s just a thin wrapper around skrafldb_ndb.AppVersionModel.get_by_id("app_version")) and expose it via an app_versions property.
| if av.update_message: | ||
| app_version["update_message"] = av.update_message | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The new app-version lookup swallows all exceptions with except Exception: pass, which can hide real misconfigurations (e.g., missing table/permissions) and make the force-update feature silently ineffective. Consider at least logging the exception (possibly at debug/info) and/or narrowing the exception types you intentionally ignore.
| pass | |
| logging.getLogger(__name__).warning( | |
| "Failed to fetch app version requirements in inituser_api", | |
| exc_info=True, | |
| ) |
| return jsonify( | ||
| ok=True, | ||
| userprefs=uf.as_dict(), | ||
| userstats=us, | ||
| firebase_token=token, | ||
| app_version=app_version, | ||
| ) |
There was a problem hiding this comment.
/inituser now returns an app_version field, but the existing API e2e coverage appears to only assert ok and the presence of userprefs/userstats. Please extend the /inituser tests to validate the new field behavior (e.g., app_version is null when no singleton exists, and contains min_supported_version/latest_version/update_message when configured).
| class AppVersion(Base): | ||
| """App version requirements - mirrors NDB AppVersionModel.""" | ||
|
|
||
| __tablename__ = "app_versions" | ||
|
|
||
| # Singleton row, always "app_version" | ||
| id: Mapped[str] = mapped_column(String(64), primary_key=True) | ||
|
|
||
| # Minimum version the app must have to function | ||
| min_supported_version: Mapped[str] = mapped_column(String(20), nullable=False) | ||
|
|
||
| # Latest available version of the app | ||
| latest_version: Mapped[str] = mapped_column(String(20), nullable=False) | ||
|
|
||
| # Optional custom message to display in the update prompt | ||
| update_message: Mapped[Optional[str]] = mapped_column(Text, nullable=True) | ||
|
|
||
| def __repr__(self) -> str: | ||
| return ( | ||
| f"<AppVersion(min={self.min_supported_version!r}, " | ||
| f"latest={self.latest_version!r})>" | ||
| ) |
There was a problem hiding this comment.
AppVersion is intended to “mirror” the NDB model, but unlike the other PostgreSQL ORM models in this file it doesn’t expose a key_id property. Adding key_id (typically returning self.id) would keep it consistent with the other entity models and makes it easier to type it as an EntityProtocol later.
| class AppVersion(Base): | ||
| """App version requirements - mirrors NDB AppVersionModel.""" | ||
|
|
||
| __tablename__ = "app_versions" | ||
|
|
||
| # Singleton row, always "app_version" | ||
| id: Mapped[str] = mapped_column(String(64), primary_key=True) | ||
|
|
||
| # Minimum version the app must have to function | ||
| min_supported_version: Mapped[str] = mapped_column(String(20), nullable=False) | ||
|
|
||
| # Latest available version of the app | ||
| latest_version: Mapped[str] = mapped_column(String(20), nullable=False) | ||
|
|
||
| # Optional custom message to display in the update prompt | ||
| update_message: Mapped[Optional[str]] = mapped_column(Text, nullable=True) | ||
|
|
There was a problem hiding this comment.
This PR introduces a new PostgreSQL table (app_versions) but there’s no corresponding migration/DDL or documented bootstrap step to ensure the table (and the singleton row with id app_version) exists in deployed environments. Without that, the feature will quietly do nothing (or error) on PostgreSQL. Please ensure the production schema management path creates this table and seeds/updates the singleton row.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client, | ||
| create_user, | ||
| login_user, | ||
| u1, |
There was a problem hiding this comment.
Unused imports: client, create_user, and u1 are imported from utils but not referenced in this module (pytest will still discover the fixtures because importing utils loads the module). Removing them avoids lint noise and keeps the test focused.
| client, | |
| create_user, | |
| login_user, | |
| u1, | |
| login_user, |
There was a problem hiding this comment.
In pytest, client, create_user, and u1 are fixtures, and they must be explicitly imported into the test module for pytest to make them available as function parameters. If you remove those imports, pytest won't find the fixtures and the tests will fail.
login_user is a regular helper function (not a fixture), and it's called directly in the test bodies, so it's clearly used.