fix: decode Hive string items in backup payload & accept List[Any] in backend - #21
Merged
Conversation
- Created backend/ directory with FastAPI server. - Added MongoDB integration and Pydantic models. - Configured Railway deployment with railway.toml. - Added http dependency to Flutter app. - Implemented ApiService for reporting usage and backing up data. - Added exportAllData method to WorkoutProvider. - Integrated usage reporting in app initialization. - Added Settings screen with Backup functionality. Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
- Create backend/ directory with FastAPI and MongoDB logic. - Move requirements.txt to root for Railpack detection. - Configure railway.toml for Railpack deployment. - Implement ApiService in Flutter for reporting and backups. - Update WorkoutProvider to expose exportAllData. - Add SettingsScreen to UI for manual backups. - Fix Pydantic models to use default_factory. - Make API URL configurable via environment variable. Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
- Create backend/ directory with FastAPI and MongoDB logic. - Move requirements.txt to root for Railpack detection. - Add main.py to root for Railpack detection. - Remove railway.toml to use default Railpack configuration. - Implement ApiService in Flutter for reporting and backups. - Update WorkoutProvider to expose exportAllData. - Add SettingsScreen to UI for manual backups. - Fix Pydantic models to use default_factory. - Make API URL configurable via environment variable. - Update .gitignore to include Python and Flutter artifacts. Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
- Create backend/ directory with FastAPI and MongoDB logic. - Move requirements.txt to root for Railpack detection. - Add main.py to root for Railpack detection. - Remove railway.toml to use default Railpack configuration. - Implement ApiService in Flutter for reporting and backups. - Update WorkoutProvider to expose exportAllData. - Add SettingsScreen to UI for manual backups. - Fix Pydantic models to use default_factory. - Make API URL configurable via environment variable. - Update .gitignore to include Python and Flutter artifacts. - Add CORS middleware to backend for cross-origin requests. Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
… backend - Flutter: decode JSON-string list items to Maps before sending to /backup - Backend: BackupData model accepts List[Any] with parsed_backup() helper - Fixes 'Input should be a valid dictionary' validation error
Contributor
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds an analytics stack: MongoDB-backed FastAPI ingest/read API, Pydantic analytics models, a Streamlit dashboard, Flutter client analytics integration and backup UI, storage import/export/merge logic, platform helpers, expanded multi-ecosystem .gitignore, and pinned backend/dashboard dependencies. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workout-logger/lib/services/storage_service.dart (1)
278-289: 🧹 Nitpick | 🔵 Trivial
exportAllDataproduces double-encoded JSON — root cause of the backup validation error.Hive box values are already JSON strings. Wrapping them in a map and calling
jsonEncodeembeds escaped JSON strings inside the outer JSON (e.g.,"sessions": ["{\"id\":...}"]). TheApiService.backupDataworkaround decodes these, but consumers ofexportAllDatamust always remember to post-process the output.Consider decoding Hive values at the source so
exportAllDatareturns clean, single-encoded JSON. This would also simplifyimportData.♻️ Suggested refactor
`@override` Future<String> exportAllData() async { final data = { - 'sessions': _sessionsBox.values.toList(),- 'routines': _routinesBoxInstance.values.toList(),- 'targets': _targetsBoxInstance.values.toList(),- 'muscleGroups': _muscleGroupsBoxInstance.values.toList(),- 'customExercises': _customExercisesBoxInstance.values.toList(),+ 'sessions': _sessionsBox.values.map((v) => jsonDecode(v)).toList(),+ 'routines': _routinesBoxInstance.values.map((v) => jsonDecode(v)).toList(),+ 'targets': _targetsBoxInstance.values.map((v) => jsonDecode(v)).toList(),+ 'muscleGroups': _muscleGroupsBoxInstance.values.map((v) => jsonDecode(v)).toList(),+ 'customExercises': _customExercisesBoxInstance.values.map((v) => jsonDecode(v)).toList(), 'exportDate': DateTime.now().toIso8601String(), }; return jsonEncode(data); }Then update
importDatato skip the innerjsonDecodecalls, and remove the decode workaround inApiService.backupData.
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 32: Remove the duplicate gitignore entry for android/local.properties by
keeping a single occurrence and deleting the redundant line; locate the repeated
pattern "android/local.properties" in the .gitignore and remove one of the two
entries so the file contains only one entry for that path.
- Line 31: In .gitignore remove the duplicated pattern "android/gradlew.bat" so
it only appears once; locate both occurrences of the exact string and delete one
duplicate entry, leaving a single listing for android/gradlew.bat.
- Line 76: Replace the misspelled ignore pattern ".hypothesise" in the
.gitignore with the correct Hypothesis directory pattern ".hypothesis/" so the
Hypothesis cache directory will be properly ignored in future; locate the entry
".hypothesise" and change it to ".hypothesis/".
In `@backend/database.py`:
- Around line 6-10: client is left undefined when MONGODB_URI is falsy which
will raise NameError when other modules import client; fix by ensuring both
client and db are always defined (e.g., initialize client = None and db = None
before the if) or set client = None in the else branch where db is set to None;
update the AsyncIOMotorClient instantiation usage
(AsyncIOMotorClient(MONGODB_URI)) and any health-check/shutdown code that
expects a possibly-None client to handle None safely.
In `@backend/main.py`:
- Around line 45-46: Replace each "except Exception as e: raise
HTTPException(status_code=500, detail=str(e))" pattern in backend/main.py with
exception chaining and non-leaking client messages: log the original exception
server-side (e.g., using the module logger) and re-raise the HTTPException using
"raise HTTPException(status_code=500, detail='Internal server error') from e".
Apply this change to the handlers referenced (the blocks currently at the diff
locations and similar handlers around lines 62-63, 73-74, and 100-101) so the
original exception is chained via "from e" while the client receives a generic
message.
- Around line 106-219: The analytics routes (analytics_overview,
analytics_retention, analytics_events, analytics_users, analytics_user_detail)
are unauthenticated and expose PII; require and enforce
authentication/authorization (e.g., add a Depends(get_current_user) or
Depends(get_current_admin) parameter on those route handlers and check
scopes/roles so only authorized users (admins or resource owners) can access
them. For analytics_user_detail specifically enforce owner-or-admin access and
for all handlers redact or exclude PII fields (user_app_id,
device/platform/backup metadata, etc.) via projection in db.find/find_one and by
removing/masking IDs in returned payloads. Update any tests or docs to reflect
the auth requirement and ensure error handling returns 401/403 for
unauthenticated/unauthorized requests.
- Around line 12-18: The CORS config in app.add_middleware using CORSMiddleware
currently sets allow_origins=["*"] together with allow_credentials=True which is
invalid; update the middleware configuration in backend/main.py
(app.add_middleware with CORSMiddleware) to either set allow_credentials=False
or replace allow_origins=["*"] with a concrete list of allowed domains (e.g.,
["https://example.com"]) so that Access-Control-Allow-Origin and
Access-Control-Allow-Credentials are compliant; pick the temporary fix of
setting allow_credentials=False if you don't have a fixed origin list yet.
- Around line 34-46: The dict `doc` produced by stats.model_dump() is being
mutated by db.reports.update_one (upsert) which adds an "_id" and causes a
DuplicateKeyError when reused for db.report_log.insert_one; fix by preventing
reuse of the mutated object—either create a fresh copy for the upsert (e.g., use
dict(doc) or copy.deepcopy(doc) when calling db.reports.update_one) or
explicitly remove any "_id" (doc.pop("_id", None)) before calling
db.report_log.insert_one; locate usages of stats.model_dump(), the local
variable doc, db.reports.update_one, and db.report_log.insert_one to apply the
change.
- Around line 116-118: Replace the current heartbeat count with a distinct-user
count: instead of using db.heartbeats.count_documents({"timestamp": {"$gte":
day_ago}}) to compute dau, query db.heartbeats.distinct on the user identifier
(e.g., "user_id" or the actual field used in heartbeats) with the same timestamp
filter and set dau to the length of that distinct result; also ensure any
returned value or JSON key still labeled DAU returns this unique-user count. Use
the existing symbols dau, db.heartbeats, and day_ago to locate and update the
code.
In `@backend/models.py`:
- Around line 21-49: BackupData currently accepts unbounded lists (sessions,
routines, targets, muscleGroups, customExercises) which can cause memory
exhaustion; add explicit size/depth limits by applying Pydantic Field
constraints (e.g., max_length/max_items or equivalent) on those fields in the
BackupData class and implement a model_validator/field_validator to
enforce/validate those limits at runtime; also update parsed_backup/_parse_list
to check and reject or truncate overly large lists and to enforce a maximum JSON
nesting depth when calling json.loads so deeply nested payloads are denied or
handled safely.
- Line 2: Replace deprecated typing generics with built-in generics: change the
import line that currently imports typing.List and typing.Dict so it only
imports Any and Optional (e.g., "from typing import Any, Optional"), then update
all type hints that use List[...] and Dict[...] in this module to use list[...]
and dict[...] respectively (leave Optional and Any as-is). Ensure annotations
like List[Foo] -> list[Foo] and Dict[str, Any] -> dict[str, Any], and remove
List/Dict from the import list.
In `@dashboard/app.py`:
- Around line 22-30: Wrap the MongoDB connection in get_db() with a try/except
that checks for a missing st.secrets["mongo"]["uri"] and catches MongoClient
connection errors; on failure call st.error(...) with a friendly message
(including the error text) and return None (or handle by calling st.stop()) so
the dashboard doesn't crash, and update the code that uses db (the db variable)
to handle a None return value gracefully. Ensure the function name get_db, the
use of st.secrets["mongo"]["uri"], and the MongoClient construction are the
places you modify.
- Around line 279-286: The detail view is loading the entire backup document via
db.backups.find_one and rendering it with st.json(doc), which can OOM for large
backups; change the logic in the sel handling (the block referencing sel,
db.backups.find_one, st.metric, st.expander, st.json) to fetch only lightweight
fields (or only counts) by projecting out heavy arrays, and replace the
unconditional st.json(doc) with a safer UI: show metrics (Sessions, Routines,
Custom Exercises) from counts, display a truncated preview of arrays (e.g.,
first N items) inside the expander, and add a user-initiated "Load full backup"
action (button/confirmation) that only then fetches the full document or streams
it; also display a clear size warning before rendering large payloads.
- Around line 53-56: total_heartbeats_today currently uses
db.heartbeats.count_documents and so counts heartbeat rows rather than distinct
users; change it to count distinct user_app_id like WAU/MAU: replace the
db.heartbeats.count_documents({"timestamp": {"$gte": day_ago}}) call in the
total_heartbeats_today assignment with len(db.heartbeats.distinct("user_app_id",
{"timestamp": {"$gte": day_ago}}))) so DAU reflects unique users (and update any
UI metric card that displays total_heartbeats_today to use this new DAU value if
necessary).
In `@dashboard/requirements.txt`:
- Around line 1-5: The requirements file lists unpinned packages (streamlit,
pymongo[srv], dnspython, pandas, plotly); update dashboard/requirements.txt to
pin each dependency to a specific version or a compatible-release constraint
(for example use exact versions or ~= constraints) to ensure reproducible
builds—use the suggested February 2026 versions (streamlit 1.54.0, pymongo
4.16.0, dnspython 2.8.0, pandas 3.0.0, plotly 6.5.2) or choose compatible
constraints, and explicitly call out/verify any breaking changes for pandas in
your code before committing the pandas bump.
In `@requirements.txt`:
- Around line 1-5: Pin the backend dependencies in requirements.txt by replacing
the unpinned package names (fastapi, uvicorn, motor, dnspython, pydantic) with
exact versions from your tested environment (for example fastapi==X.Y.Z,
uvicorn==A.B.C, motor==M.N.O, dnspython==D.E.F, pydantic==P.Q.R); update each
entry to the specific version you validated and commit the change so builds are
reproducible.
In `@workout-logger/lib/main.dart`:
- Around line 97-109: The code directly instantiates ApiService instead of using
the app's DI; register ApiService in your composition root MultiProvider (e.g.,
Provider<ApiService>.value(value: ApiService())) and then replace the direct new
ApiService() in main.dart with a DI lookup (context.read<ApiService>() or
Provider.of<ApiService>(context, listen: false)) before calling sendHeartbeat(),
trackEvent('app_open') and reportUsage(stats) (the async block using
provider.getQuickStats()). Ensure you import Provider where needed and keep the
fire-and-forget behavior by calling the same methods on the injected ApiService
instance.
In `@workout-logger/lib/screens/settings_screen.dart`:
- Around line 47-51: The SnackBar currently displays raw exception details via
Text('Error: $e'); update the catch block (the conditional using mounted and the
call to ScaffoldMessenger.of(context).showSnackBar) to show a user-friendly
message (e.g., "Something went wrong") in the SnackBar with AppTheme.error while
sending the full exception details to a logger (debugPrint/developer.log or your
app logger) instead of rendering them to the UI; keep the mounted check and
ensure the logging call includes the caught exception and stack trace for
debugging.
- Line 26: The SettingsScreen currently instantiates ApiService inline (final
api = ApiService()), which couples the widget to a concrete implementation;
change SettingsScreen to accept an ApiService via dependency injection (either
add a required constructor parameter like ApiService api or pull it from the
widget tree using Provider/InheritedWidget) and replace the inline instantiation
where ApiService is referenced; update any callers to pass the dependency (or
ensure a Provider is added above in the tree) and adjust tests to supply a
mock/fake ApiService for isolation.
- Around line 26-28: The call to ApiService.trackEvent('backup_triggered') is
not awaited so any rejection can escape the surrounding try/catch; modify the
Settings screen to either await the call (await api.trackEvent(...)) before
calling api.backupData(...) or explicitly handle failures (e.g.,
api.trackEvent(...).catchError((_) => null)) so that errors from trackEvent are
captured and do not become unhandled—update the code around ApiService,
trackEvent, and backupData usage to use one of these approaches.
In `@workout-logger/lib/services/api_service.dart`:
- Around line 15-22: The factory constructor ApiService({http.Client? client})
mutates the shared singleton's _client (via _instance._client = client) which is
surprising; change the design so the factory never mutates singleton state and
instead add a separate `@visibleForTesting` static setter/constructor (e.g.,
ApiService.setTestClient or ApiService.createForTesting) that explicitly
replaces _client for tests, keep ApiService._internal() and the _instance
singleton intact, and ensure all production call sites use the parameterless
factory while tests call the new visibleForTesting method to inject a custom
http.Client.
- Around line 27-37: The getter userAppId assumes Hive.box<String>('settings')
is already open and can throw if called before StorageService.init(); fix by
defensively ensuring the settings box is open before accessing it: check
Hive.isBoxOpen('settings') and if not open call await
Hive.openBox<String>('settings') (or obtain the box via StorageService's
open/get helper) before reading/writing and updating _cachedAppId in the
userAppId getter.
- Around line 1-6: The direct import of dart:io (Platform) in api_service.dart
breaks web builds; replace it with a conditional-import platform helper: add
lib/services/platform_stub.dart (returns 'unknown') and
lib/services/platform_io.dart (uses dart:io Platform to return
'android'/'ios'/'unknown'), then in api_service.dart remove "import 'dart:io'
show Platform;" and instead import the helper via conditional imports and call
getPlatformName() wherever Platform or the kIsWeb runtime guard was used (update
the existing platform-detection getter/function in api_service.dart to delegate
to getPlatformName()); this keeps web builds working while preserving native
platform detection.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- .gitignore: remove duplicate entries, fix .hypothesise typo - backend/database.py: always define client and db (avoid NameError) - backend/main.py: add API key auth on analytics routes, fix CORS allow_credentials, exception chaining with logging, DAU as distinct users, prevent doc mutation via _id leak - backend/models.py: use built-in generics, add list size limits and JSON depth guard in parsed_backup - requirements.txt: pin all backend dependencies - dashboard/requirements.txt: pin all dashboard dependencies - dashboard/app.py: wrap get_db with error handling, fix DAU to distinct users, lazy-load backup detail to prevent OOM - api_service.dart: conditional import for web compat, @VisibleForTesting setTestClient, defensive Hive box open - main.dart: register ApiService in MultiProvider, use DI lookup - settings_screen.dart: use DI for ApiService, await trackEvent, sanitize error messages shown to users
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@backend/main.py`:
- Line 28: ADMIN_API_KEY is defaulting to an empty string which causes
require_admin to return 503 silently if the env var is unset; add a startup log
warning when ADMIN_API_KEY is falsy so misconfigured deployments are visible.
Modify where ADMIN_API_KEY is initialized (symbol ADMIN_API_KEY) to check if not
ADMIN_API_KEY and call the app logger (e.g., logger.warn/ warning) with a clear
message like "ADMIN_API_KEY not set — analytics endpoints will return 503" so
operators see the problem at startup; keep the fail-closed behavior in
require_admin unchanged.
- Around line 50-51: The endpoints (/report, /backup, /event, /heartbeat) are
unauthenticated; add a simple shared app-level secret check and basic
rate-limiting: implement an auth dependency that reads an X-API-Key (or
Authorization) header and compares it to a configured APP_API_KEY (env var) and
use it as a FastAPI Depends for report_usage (and the backup/event/heartbeat
handler functions) to reject requests with 401 if missing/invalid; additionally
add per-IP or per-user_app_id rate-limiting (e.g., a lightweight in-memory
counter or integrate a limiter like slowapi/limits) that returns 429 when
thresholds are exceeded and attach it to the same endpoints so bots cannot flood
the DB.
In `@backend/models.py`:
- Around line 67-76: _depth_guard_decode currently counts raw brace characters
and miscounts when braces appear inside JSON string values; change it to first
parse the JSON string with json.loads (raising the parser's errors as-is) and
then recursively walk the resulting Python object to compute nesting depth,
comparing against max_depth and raising ValueError if exceeded. Locate
_depth_guard_decode and replace the character-scan logic with a call to
json.loads(s) followed by a helper (e.g., _compute_depth or an inner recursive
function) that inspects dicts/lists to measure depth, using function names
_depth_guard_decode and the helper to find the code to modify.
- Around line 89-96: The HeartbeatPayload Pydantic model contains duplicated
field declarations for platform and timestamp (copy-paste), so remove the second
occurrences to avoid silent overrides; edit the HeartbeatPayload class to keep a
single platform: Optional[str] and a single timestamp: datetime =
Field(default_factory=_utcnow) definition (retaining the existing user_app_id
and app_version fields) and remove the duplicate lines.
- Around line 43-57: The code double-parses JSON strings in _parse_list by
calling _depth_guard_decode(item, max_depth=MAX_JSON_NESTING_DEPTH) and then
json.loads(item); instead parse once with json.loads(item) inside the try block,
then walk the resulting object with a new helper (e.g., _check_depth(obj,
max_depth, _current=0)) that recursively checks nesting against
MAX_JSON_NESTING_DEPTH, raising on overflow; replace the two-step depth scan +
parse with this single-pass approach in _parse_list and keep the existing except
(json.JSONDecodeError, TypeError, ValueError) branch to fall back to the
original string.
In `@dashboard/app.py`:
- Line 341: The assignment to full_doc[arr_key] uses list concatenation; change
it to iterable unpacking to satisfy Ruff RUF005 by replacing the concatenation
expression (arr[:5] + [f"... and {len(arr) - 5} more"]) with an unpacking form
that expands the first five elements then appends the summary element (use arr,
arr_key, full_doc and the same length calculation).
- Around line 288-298: The selectbox currently builds ids =
[b.get("user_app_id", "?") for b in backups] causing duplicate labels and
ambiguous lookups; change the selection options to include a unique identifier
from each backup (e.g., combine b.get("_id") or b.get("backup_received_at") with
b.get("user_app_id") into the selectbox value/label) and then use that unique
identifier when calling db.backups.find_one (replace the single-field query
{"user_app_id": sel} with a query that matches the chosen backup's unique field,
e.g., {"_id": chosen_id} or {"user_app_id": user_app_id, "backup_received_at":
timestamp}) so each detail lookup (the calls to db.backups.find_one) returns the
exact document rather than the first matching user_app_id.
- Around line 36-37: Replace the user-facing error that currently interpolates
the exception (the except (ConnectionFailure, ConfigurationError) as e block
that calls st.error(f"Failed to connect to MongoDB: {e}")) with a generic
message for dashboard users (e.g., "Failed to connect to MongoDB. Please contact
the administrator."), and send the full exception details to an internal log
instead (use logging.exception or your existing logger to record variable e and
stack trace) so diagnostics are preserved but sensitive connection details are
not exposed.
In `@requirements.txt`:
- Around line 1-5: Update the FastAPI pin in requirements.txt from
fastapi==0.115.0 to fastapi==0.128.8 (or at minimum >=0.116.2) so Starlette is
bumped to a non-vulnerable release (Starlette >=0.47.2; ideally >=0.49.1), and
add an explicit Starlette constraint (e.g., starlette>=0.52.1) to ensure the
resolved dependency includes the security fixes for
GHSA-2c2j-9gv5-cj73/CVE-2025-54121 and GHSA-f96h-pmfr-66vw/CVE-2024-47874 (and
address Range header DoS by targeting >=0.49.1/CVE-2025-62727); after updating
the pins, regenerate your lock/constraints (pip-compile/pip freeze) and run
tests to validate compatibility.
In `@workout-logger/lib/services/api_service.dart`:
- Around line 13-16: The default API URL in the static const _baseUrl using
String.fromEnvironment('API_URL', defaultValue: ...) should not point to the
production Railway endpoint; change the defaultValue to an empty string (or
localhost) and add a fast-fail in non-release builds when _baseUrl is
empty—update the initialization logic around _baseUrl (and any getter that
returns it) to throw or assert when API_URL is not provided in debug/staging so
accidental omission of --dart-define cannot silently hit production.
| # ────────────────────── auth ────────────────────── | ||
| ADMIN_API_KEY = os.environ.get("ADMIN_API_KEY", "") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty-string default for ADMIN_API_KEY disables all analytics endpoints if unset.
The require_admin dependency returns 503 when ADMIN_API_KEY is falsy, which is the correct fail-closed behavior. However, there's no startup warning or log when the key is missing, so a misconfigured deployment would silently lock out the dashboard with an opaque 503.
Consider logging a warning at startup:
ifnotADMIN_API_KEY:
logger.warning("ADMIN_API_KEY not set — analytics endpoints will return 503")🤖 Prompt for AI Agents
In `@backend/main.py` at line 28, ADMIN_API_KEY is defaulting to an empty string
which causes require_admin to return 503 silently if the env var is unset; add a
startup log warning when ADMIN_API_KEY is falsy so misconfigured deployments are
visible. Modify where ADMIN_API_KEY is initialized (symbol ADMIN_API_KEY) to
check if not ADMIN_API_KEY and call the app logger (e.g., logger.warn/ warning)
with a clear message like "ADMIN_API_KEY not set — analytics endpoints will
return 503" so operators see the problem at startup; keep the fail-closed
behavior in require_admin unchanged.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+43
to
+57
| @staticmethod | ||
| def _parse_list(items: list) -> list: | ||
| """Decode any JSON-string items; enforce nesting depth.""" | ||
| out: list = [] | ||
| for item in items: | ||
| if isinstance(item, str): | ||
| try: | ||
| # Custom decoder with depth guard | ||
| _depth_guard_decode(item, max_depth=MAX_JSON_NESTING_DEPTH) | ||
| out.append(json.loads(item)) | ||
| except (json.JSONDecodeError, TypeError, ValueError): | ||
| out.append(item) | ||
| else: | ||
| out.append(item) | ||
| return out |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Double-parse: depth guard scans the string, then json.loads parses it again.
_depth_guard_decode on Line 51 iterates the full string for depth, then json.loads on Line 52 parses it a second time. Consider combining both into a single pass — e.g., parse once with json.loads, then walk the resulting object to check depth.
♻️ Proposed refactor: single-pass depth check
`@staticmethod`
def _parse_list(items: list) -> list:
"""Decode any JSON-string items; enforce nesting depth."""
out: list = []
for item in items:
if isinstance(item, str):
try:
- # Custom decoder with depth guard- _depth_guard_decode(item, max_depth=MAX_JSON_NESTING_DEPTH)- out.append(json.loads(item))+ parsed = json.loads(item)+ _check_depth(parsed, max_depth=MAX_JSON_NESTING_DEPTH)+ out.append(parsed)
except (json.JSONDecodeError, TypeError, ValueError):
out.append(item)
else:
out.append(item)
return outWith a helper that walks the parsed structure:
def_check_depth(obj, max_depth: int, _current: int=0) ->None:
if_current>max_depth:
raiseValueError(f"JSON nesting exceeds maximum depth of {max_depth}")
ifisinstance(obj, dict):
forvinobj.values():
_check_depth(v, max_depth, _current+1)
elifisinstance(obj, list):
forvinobj:
_check_depth(v, max_depth, _current+1)This also fixes the false-positive bracket-in-strings issue.
🤖 Prompt for AI Agents
In `@backend/models.py` around lines 43 - 57, The code double-parses JSON strings
in _parse_list by calling _depth_guard_decode(item,
max_depth=MAX_JSON_NESTING_DEPTH) and then json.loads(item); instead parse once
with json.loads(item) inside the try block, then walk the resulting object with
a new helper (e.g., _check_depth(obj, max_depth, _current=0)) that recursively
checks nesting against MAX_JSON_NESTING_DEPTH, raising on overflow; replace the
two-step depth scan + parse with this single-pass approach in _parse_list and
keep the existing except (json.JSONDecodeError, TypeError, ValueError) branch to
fall back to the original string.
Comment on lines
+67
to
+76
| def _depth_guard_decode(s: str, max_depth: int = 20) -> None: | ||
| """Raise ValueError if the JSON string nests deeper than max_depth.""" | ||
| depth = 0 | ||
| for ch in s: | ||
| if ch in "{[": | ||
| depth += 1 | ||
| if depth > max_depth: | ||
| raise ValueError(f"JSON nesting exceeds maximum depth of {max_depth}") | ||
| elif ch in "}]": | ||
| depth -= 1 |
Contributor
There was a problem hiding this comment.
Depth guard doesn't account for brackets inside JSON string values.
_depth_guard_decode scans raw characters, so a value like "key": "text with { braces" increments the depth counter spuriously. This means the guard may reject valid payloads with bracket characters in string values — a false-positive risk. It won't cause false negatives (it over-counts, not under-counts), so the security posture is safe, but legitimate user data with brackets in text fields could fail to back up.
A proper fix would be to use json.loads with a custom decoder or an object_pairs_hook that tracks depth, or to walk the parsed object tree instead.
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 74-74: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@backend/models.py` around lines 67 - 76, _depth_guard_decode currently counts
raw brace characters and miscounts when braces appear inside JSON string values;
change it to first parse the JSON string with json.loads (raising the parser's
errors as-is) and then recursively walk the resulting Python object to compute
nesting depth, comparing against max_depth and raising ValueError if exceeded.
Locate _depth_guard_decode and replace the character-scan logic with a call to
json.loads(s) followed by a helper (e.g., _compute_depth or an inner recursive
function) that inspects dicts/lists to measure depth, using function names
_depth_guard_decode and the helper to find the code to modify.
Comment on lines
+89
to
+96
| class HeartbeatPayload(BaseModel): | ||
| """Minimal ping sent on every app open for DAU/MAU calculation.""" | ||
| user_app_id: str | ||
| app_version: Optional[str] = None | ||
| platform: Optional[str] = None | ||
| timestamp: datetime = Field(default_factory=_utcnow) | ||
| platform: Optional[str] = None | ||
| timestamp: datetime = Field(default_factory=_utcnow) |
Contributor
There was a problem hiding this comment.
Duplicate platform and timestamp fields — copy-paste error.
Lines 95-96 re-declare the same fields already defined on lines 93-94. Python silently uses the last definition, so the model happens to work, but this is clearly unintended and flagged by Ruff (PIE794). Remove the duplicates.
🐛 Proposed fix
class HeartbeatPayload(BaseModel):
"""Minimal ping sent on every app open for DAU/MAU calculation."""
user_app_id: str
app_version: Optional[str] = None
platform: Optional[str] = None
timestamp: datetime = Field(default_factory=_utcnow)
- platform: Optional[str] = None- timestamp: datetime = Field(default_factory=_utcnow)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| classHeartbeatPayload(BaseModel): | |
| """Minimal ping sent on every app open for DAU/MAU calculation.""" | |
| user_app_id: str | |
| app_version: Optional[str] =None | |
| platform: Optional[str] =None | |
| timestamp: datetime=Field(default_factory=_utcnow) | |
| platform: Optional[str] =None | |
| timestamp: datetime=Field(default_factory=_utcnow) | |
| classHeartbeatPayload(BaseModel): | |
| """Minimal ping sent on every app open for DAU/MAU calculation.""" | |
| user_app_id: str | |
| app_version: Optional[str] =None | |
| platform: Optional[str] =None | |
| timestamp: datetime=Field(default_factory=_utcnow) |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 95-95: Class field platform is defined multiple times
Remove duplicate field definition for platform
(PIE794)
[warning] 96-96: Class field timestamp is defined multiple times
Remove duplicate field definition for timestamp
(PIE794)
🤖 Prompt for AI Agents
In `@backend/models.py` around lines 89 - 96, The HeartbeatPayload Pydantic model
contains duplicated field declarations for platform and timestamp (copy-paste),
so remove the second occurrences to avoid silent overrides; edit the
HeartbeatPayload class to keep a single platform: Optional[str] and a single
timestamp: datetime = Field(default_factory=_utcnow) definition (retaining the
existing user_app_id and app_version fields) and remove the duplicate lines.
Comment on lines
+36
to
+37
| except (ConnectionFailure, ConfigurationError) as e: | ||
| st.error(f"Failed to connect to MongoDB: {e}") |
Contributor
There was a problem hiding this comment.
Avoid leaking connection details in user-facing error messages.
ConnectionFailure and ConfigurationError exceptions may include the MongoDB URI or internal server details. Exposing these via st.error() to dashboard users is a minor security concern.
Proposed fix
except (ConnectionFailure, ConfigurationError) as e:
- st.error(f"Failed to connect to MongoDB: {e}")+ st.error("Failed to connect to MongoDB. Check server logs for details.")+ import logging+ logging.exception("MongoDB connection failed")
return None🤖 Prompt for AI Agents
In `@dashboard/app.py` around lines 36 - 37, Replace the user-facing error that
currently interpolates the exception (the except (ConnectionFailure,
ConfigurationError) as e block that calls st.error(f"Failed to connect to
MongoDB: {e}")) with a generic message for dashboard users (e.g., "Failed to
connect to MongoDB. Please contact the administrator."), and send the full
exception details to an internal log instead (use logging.exception or your
existing logger to record variable e and stack trace) so diagnostics are
preserved but sensitive connection details are not exposed.
Comment on lines
+288
to
+298
| st.markdown("---") | ||
| st.subheader("Backup Detail") | ||
| ids = [b.get("user_app_id", "?") for b in backups] | ||
| sel = st.selectbox("Select user_app_id", ids, key="backup_user") | ||
| if sel: | ||
| # Fetch only lightweight metadata first (project out heavy arrays) | ||
| meta = db.backups.find_one( | ||
| {"user_app_id": sel}, | ||
| {"_id": 0, "sessions": 0, "routines": 0, | ||
| "targets": 0, "muscleGroups": 0, "customExercises": 0}, | ||
| ) |
Contributor
There was a problem hiding this comment.
Duplicate user_app_id entries in backup selectbox yield ambiguous detail lookups.
If a user has multiple backups, ids will contain duplicate user_app_id values. The st.selectbox will show indistinguishable entries, and all find_one({"user_app_id": sel}) calls (lines 294, 300, 316, 335) will always return the same (first) document regardless of which duplicate is selected.
Consider using a unique key per backup (e.g., include backup_received_at or the document _id) to disambiguate:
Sketch of a possible approach
- ids = [b.get("user_app_id", "?") for b in backups]- sel = st.selectbox("Select user_app_id", ids, key="backup_user")+ labels = [+ f"{b.get('user_app_id', '?')} @ {b.get('backup_received_at', 'unknown')}"+ for b in backups+ ]+ sel_idx = st.selectbox("Select backup", range(len(labels)),+ format_func=lambda i: labels[i], key="backup_user")+ sel_backup = backups[sel_idx]+ sel = sel_backup.get("user_app_id", "?")+ sel_time = sel_backup.get("backup_received_at")Then use both user_app_id and backup_received_at in detail queries to pinpoint the exact document.
🤖 Prompt for AI Agents
In `@dashboard/app.py` around lines 288 - 298, The selectbox currently builds ids
= [b.get("user_app_id", "?") for b in backups] causing duplicate labels and
ambiguous lookups; change the selection options to include a unique identifier
from each backup (e.g., combine b.get("_id") or b.get("backup_received_at") with
b.get("user_app_id") into the selectbox value/label) and then use that unique
identifier when calling db.backups.find_one (replace the single-field query
{"user_app_id": sel} with a query that matches the chosen backup's unique field,
e.g., {"_id": chosen_id} or {"user_app_id": user_app_id, "backup_received_at":
timestamp}) so each detail lookup (the calls to db.backups.find_one) returns the
exact document rather than the first matching user_app_id.
| for arr_key in ("sessions", "routines", "targets", "muscleGroups", "customExercises"): | ||
| arr = full_doc.get(arr_key, []) | ||
| if len(arr) > 5: | ||
| full_doc[arr_key] = arr[:5] + [f"... and {len(arr) - 5} more"] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer iterable unpacking over list concatenation (Ruff RUF005).
Proposed fix
- full_doc[arr_key] = arr[:5] + [f"... and {len(arr) - 5} more"]+ full_doc[arr_key] = [*arr[:5], f"... and {len(arr) - 5} more"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| full_doc[arr_key] =arr[:5]+ [f"... and {len(arr) -5} more"] | |
| full_doc[arr_key] =[*arr[:5], f"... and {len(arr) -5} more"] |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 341-341: Consider [*arr[:5], f"... and {len(arr) - 5} more"] instead of concatenation
Replace with [*arr[:5], f"... and {len(arr) - 5} more"]
(RUF005)
🤖 Prompt for AI Agents
In `@dashboard/app.py` at line 341, The assignment to full_doc[arr_key] uses list
concatenation; change it to iterable unpacking to satisfy Ruff RUF005 by
replacing the concatenation expression (arr[:5] + [f"... and {len(arr) - 5}
more"]) with an unpacking form that expands the first five elements then appends
the summary element (use arr, arr_key, full_doc and the same length
calculation).
Comment on lines
+1
to
+5
| fastapi==0.115.0 | ||
| uvicorn==0.30.6 | ||
| motor==3.6.0 | ||
| dnspython==2.7.0 | ||
| pydantic==2.9.2 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
fastapi 0.115.0 starlette vulnerability GHSA-2c2j-9gv5-cj73
💡 Result:
GHSA-2c2j-9gv5-cj73 (CVE-2025-54121) is a Starlette vulnerability: when handling multipart/form-data uploads with large files, Starlette can block the main/event-loop thread while rolling the spooled upload to disk, creating a denial-of-service condition. Affected versions are starlette < 0.47.2; it’s fixed in 0.47.2. [1]
Impact on FastAPI 0.115.0: FastAPI 0.115.x pins Starlette to older ranges (e.g., 0.115.2: >=0.37.2,<0.41.0; later 0.115.10: >=0.40.0,<0.47.0), which are entirely below 0.47.2, so environments following those pins will pull a vulnerable Starlette. [2]
Recommended remediation
- Upgrade FastAPI to a version that allows Starlette versions including the fix, e.g. FastAPI 0.116.2 (supports Starlette
>=0.40.0,<0.49.0), then ensure Starlette >= 0.47.2 is installed. [2] - Explicitly (after upgrading FastAPI) install a fixed Starlette, e.g.:
pip install "starlette>=0.47.2".
Verify
python -c "import fastapi,starlette; print(fastapi.__version__, starlette.__version__)"
Sources: [1] GitHub Advisory GHSA-2c2j-9gv5-cj73, [2] FastAPI release notes (Starlette version ranges).
🌐 Web query:
fastapi latest version 2026 starlette security patch
💡 Result:
- Latest FastAPI (as of Feb 11, 2026):0.128.8 (released Feb 11, 2026) on PyPI. [1]
- Latest Starlette (as of Jan 18, 2026):0.52.1 on PyPI. [2]
Starlette security patch you likely mean
- CVE-2025-62727 / GHSA-7f5h-v6xp-fcq8: DoS risk via crafted HTTP
Rangeheader leading to quadratic-time processing instarlette.responses.FileResponse(affects file-serving endpoints likeStaticFiles/FileResponse). [3] - Fixed in Starlette:0.49.1 (released Oct 28, 2025) — explicitly called out as a security vulnerability fix in Starlette release notes. [4]
Action: ensure your dependency set includes starlette>=0.49.1 (or just upgrade to Starlette 0.52.1 and FastAPI 0.128.8). [1][2][3][4]
🌐 Web query:
GHSA-f96h-pmfr-66vw multipart form DoS vulnerability
💡 Result:
GHSA-f96h-pmfr-66vw is a Starlette vulnerability (also affecting apps built on it, including many FastAPI apps) where handling multipart/form-data can be abused to cause a Denial of Service (DoS) via memory exhaustion. If a multipart part omits the filename parameter, Starlette treats it as a text field and buffers it without an effective size limit, allowing an attacker to send very large fields (or many requests in parallel) until the process slows, swaps, or is OOM-killed. [1][2]
- CVE:CVE-2024-47874 [1][3]
- Affected: Starlette versions before 0.40.0 (GitHub advisory lists “affected < 0.39.2”; fix is in 0.40.0). [1][2]
- Fixed:Upgrade to Starlette 0.40.0+. The fix adds a multipart part-size limit (
max_part_size) to the multipart parser. [1][3] - Example PoC pattern: a
multipart/form-dataPOST with a huge field lackingfilename(e.g.,curl ... -F 'big=</dev/urandom'). [1][2]
Recommended action: upgrade Starlette to >= 0.40.0 (and redeploy any services that bundle it), and consider adding/enforcing request/body limits at your ASGI server / reverse proxy as defense-in-depth (but the core issue is in Starlette’s buffering behavior). [1][2]
Sources: [1] GitHub Security Advisory, [2] OSV entry, [3] Tenable CVE page.
Upgrade FastAPI immediately due to known Starlette DoS vulnerabilities.fastapi==0.115.0 constrains Starlette to versions below 0.47.2, which are vulnerable to:
- GHSA-2c2j-9gv5-cj73 (CVE-2025-54121): DoS via multipart/form-data with large files
- GHSA-f96h-pmfr-66vw (CVE-2024-47874): DoS via unbuffered multipart fields
Upgrade to FastAPI 0.128.8 (latest, Feb 2026) which supports Starlette 0.52.1 (includes all security patches). Alternatively, minimum: FastAPI 0.116.2+ with Starlette >= 0.47.2. Additionally consider Starlette >= 0.49.1 to address CVE-2025-62727 (Range header DoS).
🧰 Tools
🪛 OSV Scanner (2.3.2)
[HIGH] 1-1: starlette 0.38.6: Starlette has possible denial-of-service vector when parsing large files in multipart forms
[HIGH] 1-1: starlette 0.38.6: Starlette Denial of service (DoS) via multipart/form-data
🤖 Prompt for AI Agents
In `@requirements.txt` around lines 1 - 5, Update the FastAPI pin in
requirements.txt from fastapi==0.115.0 to fastapi==0.128.8 (or at minimum
>=0.116.2) so Starlette is bumped to a non-vulnerable release (Starlette
>=0.47.2; ideally >=0.49.1), and add an explicit Starlette constraint (e.g.,
starlette>=0.52.1) to ensure the resolved dependency includes the security fixes
for GHSA-2c2j-9gv5-cj73/CVE-2025-54121 and GHSA-f96h-pmfr-66vw/CVE-2024-47874
(and address Range header DoS by targeting >=0.49.1/CVE-2025-62727); after
updating the pins, regenerate your lock/constraints (pip-compile/pip freeze) and
run tests to validate compatibility.
Comment on lines
+13
to
+16
| static const String _baseUrl = String.fromEnvironment( | ||
| 'API_URL', | ||
| defaultValue: 'https://workout-logger-production-1e93.up.railway.app', | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded production URL as default value.
The default API_URL bakes in a production Railway endpoint. If the --dart-define is accidentally omitted, debug/staging builds will silently hit production. Consider using an empty string or localhost default and failing fast when unconfigured in non-release builds.
🤖 Prompt for AI Agents
In `@workout-logger/lib/services/api_service.dart` around lines 13 - 16, The
default API URL in the static const _baseUrl using
String.fromEnvironment('API_URL', defaultValue: ...) should not point to the
production Railway endpoint; change the defaultValue to an empty string (or
localhost) and add a fast-fail in non-release builds when _baseUrl is
empty—update the initialization logic around _baseUrl (and any getter that
returns it) to throw or assert when API_URL is not provided in debug/staging so
accidental omission of --dart-define cannot silently hit production.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@workout-logger/lib/screens/settings_screen.dart`:
- Around line 196-206: The snackbar is showing total items from the backup
(itemCount/routineCount) instead of actual imported counts; change the import
flow so importData returns a summary with actualAddedSessions and
actualAddedRoutines (or similar), update the call site in settings_screen.dart
to use those returned counts rather than (data['sessions'] as List?)?.length and
(data['routines'] as List?)?.length, and update the SnackBar text to report the
actualAddedSessions and actualAddedRoutines values (keep the same SnackBar
creation and AppTheme.success usage).
- Around line 27-70: The code performs an unnecessary JSON encode/decode
round-trip inside _performBackup by calling exportAllData() (which returns a
JSON string), decoding it to a Map, and then passing that Map to
ApiService.backupData (which likely re-encodes it); fix by adding a new provider
API that returns structured data directly (e.g., exportAllDataAsMap() or change
exportAllData to return Map<String, dynamic>) and update _performBackup to call
that method and pass its Map directly to backupData, also update
WorkoutProvider.exportAllData and any callers accordingly to avoid the redundant
serialization round-trip.
In `@workout-logger/lib/services/api_service.dart`:
- Around line 66-70: All network POST calls using _client.post in
api_service.dart (the heartbeat call to Uri.parse('$_baseUrl/heartbeat') and the
other POST call sites around lines for /event, /usage and /backup) currently
have no timeout; wrap each _client.post(...) Future with a timeout (e.g.
.timeout(Duration(seconds: 10)) for heartbeat/event/usage and a longer timeout
for the /backup upload, e.g. .timeout(Duration(minutes: 2))), and catch
TimeoutException to log and return a sensible failure value instead of letting
the Future hang; update the specific call sites that call
_client.post(Uri.parse('$_baseUrl/heartbeat')), the event/usage POSTs, and the
backup POST to use these per-endpoint timeouts and exception handling.
- Around line 105-130: The reportUsage function currently inserts nulls into the
payload when keys like 'totalWorkouts', 'weeklyWorkouts', 'weeklyVolume', or
'exercisesThisWeek' are missing from the stats map; update reportUsage to
defensively validate and coerce those values before sending: check stats for
each required key (totalWorkouts, weeklyWorkouts, weeklyVolume,
exercisesThisWeek), apply sensible defaults or skip/omit fields when values are
null, and ensure numeric fields are cast to the expected types (int/double)
before building payload; if required fields are absent, log a warning via
debugPrint and avoid calling _client.post (or return early) to prevent sending
invalid data.
In `@workout-logger/lib/services/storage_service.dart`:
- Line 297: The map in StorageService that currently sets 'appVersion': '1.0.6'
uses a hardcoded magic string that will drift; replace this with a dynamic value
from package_info_plus (PackageInfo.fromPlatform()) or a build-time constant so
it stays in sync with pubspec.yaml. Update the code that builds the metadata map
in StorageService (locate the 'appVersion' key) to await
PackageInfo.fromPlatform() during initialization (or read an injected/build-time
APP_VERSION) and set 'appVersion' to
"${packageInfo.version}+${packageInfo.buildNumber}" (or corresponding constant)
instead of the literal '1.0.6'. Ensure you add the package_info_plus dependency
and handle async initialization where StorageService constructs or provides the
metadata.
- Around line 302-308: _decodeItem currently casts unexpected types and will
throw; update it to defensively handle nulls and non-Map/non-String values: if
item is null return an empty Map<String, dynamic>, if item is a Map return it,
if item is a String attempt jsonDecode and on decode error return an empty Map
(or a sentinel {}) instead of letting a TypeError propagate, and for any other
type coerce to a Map safely (e.g., treat as invalid and return {}). Make changes
inside the _decodeItem function so callers of _decodeItem (import routines) get
a safe Map result rather than the import aborting on TypeError.
- Around line 289-299: The export is collecting Hive-stored JSON strings and
then jsonEncode-ing them again, producing double-encoded values; in
exportAllData (the block that builds 'data' using _sessionsBox,
_routinesBoxInstance, _targetsBoxInstance, _muscleGroupsBoxInstance,
_customExercisesBoxInstance and settingsMap) decode each box value (e.g., map
values from box.values.toList() through jsonDecode or a small helper) so the
exported map contains real Maps/Lists not JSON strings before calling
jsonEncode(data); update the construction of 'sessions', 'routines', 'targets',
'muscleGroups', and 'customExercises' to decode entries and normalize types
(safely handle already-decoded values), which removes the need for the later
_decodeItem/double-decode logic in importData/backupData.
- Around line 314-384: The import logic iterates over top-level fields like
data['sessions'], data['routines'], data['targets'], data['muscleGroups'], and
data['customExercises'] without verifying they are lists, which can throw at
runtime; update the import method to guard each block with an explicit runtime
type check (e.g. data['sessions'] is List) before iterating and cast to
List<dynamic> when looping so _decodeItem(item) is only called for actual list
elements; keep the existing merge semantics (check existing via
getWorkoutSession/getRoutine/getTarget/getMuscleGroup or box.get and call
saveWorkoutSession/saveRoutine/saveTarget/saveCustomExercise or put on
_muscleGroupsBoxInstance) and leave the settings block as-is since it already
checks for Map.
In `@workout-logger/pubspec.yaml`:
- Around line 51-56: Update the outdated dependency versions in pubspec.yaml:
change file_picker from ^8.0.0 to ^10.3.10, change share_plus from ^9.0.0 to
^12.0.1, and bump path_provider from ^2.1.0 to ^2.1.5 while leaving http at
^1.2.1; ensure these package name entries (file_picker, share_plus,
path_provider) are edited accordingly and run pub get to verify compatibility.
Comment on lines
+27
to
+70
| Future<void> _performBackup() async { | ||
| setState(() => _isBackingUp = true); | ||
| try { | ||
| final provider = context.read<WorkoutProvider>(); | ||
| final jsonString = await provider.exportAllData(); | ||
| final data = jsonDecode(jsonString) as Map<String, dynamic>; | ||
| final api = context.read<ApiService>(); | ||
| await api.trackEvent('backup_triggered').catchError((_) => null); | ||
| final success = await api.backupData(data); | ||
| if (!mounted) return; | ||
| if (success) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Backup successful!'), | ||
| backgroundColor: AppTheme.success, | ||
| ), | ||
| ); | ||
| } else { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Backup failed. Please try again.'), | ||
| backgroundColor: AppTheme.error, | ||
| ), | ||
| ); | ||
| } | ||
| } catch (e, stackTrace) { | ||
| debugPrint('Backup error: $e\n$stackTrace'); | ||
| if (!mounted) return; | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Something went wrong. Please try again.'), | ||
| backgroundColor: AppTheme.error, | ||
| ), | ||
| ); | ||
| } finally { | ||
| if (mounted) { | ||
| setState(() => _isBackingUp = false); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant encode → decode round-trip in _performBackup.
exportAllData() returns a JSON string. Line 33 immediately decodes it back to a Map, only for backupData to re-encode it with jsonEncode. Consider adding an exportAllDataAsMap() method (or returning structured data directly) to avoid the needless serialization round-trip.
🤖 Prompt for AI Agents
In `@workout-logger/lib/screens/settings_screen.dart` around lines 27 - 70, The
code performs an unnecessary JSON encode/decode round-trip inside _performBackup
by calling exportAllData() (which returns a JSON string), decoding it to a Map,
and then passing that Map to ApiService.backupData (which likely re-encodes it);
fix by adding a new provider API that returns structured data directly (e.g.,
exportAllDataAsMap() or change exportAllData to return Map<String, dynamic>) and
update _performBackup to call that method and pass its Map directly to
backupData, also update WorkoutProvider.exportAllData and any callers
accordingly to avoid the redundant serialization round-trip.
Comment on lines
+196
to
+206
| final itemCount = (data['sessions'] as List?)?.length ?? 0; | ||
| final routineCount = (data['routines'] as List?)?.length ?? 0; | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text( | ||
| 'Import complete! Processed $itemCount sessions, $routineCount routines.', | ||
| ), | ||
| backgroundColor: AppTheme.success, | ||
| ), | ||
| ); |
Contributor
There was a problem hiding this comment.
Import summary reports total file counts, not actually-imported counts.
itemCount and routineCount reflect the number of items in the backup file, not how many were actually added (duplicates are skipped by the merge logic). On a re-import, this will show "Processed 50 sessions, 10 routines" even if 0 were new. Consider having importData return a summary of what was actually imported.
🤖 Prompt for AI Agents
In `@workout-logger/lib/screens/settings_screen.dart` around lines 196 - 206, The
snackbar is showing total items from the backup (itemCount/routineCount) instead
of actual imported counts; change the import flow so importData returns a
summary with actualAddedSessions and actualAddedRoutines (or similar), update
the call site in settings_screen.dart to use those returned counts rather than
(data['sessions'] as List?)?.length and (data['routines'] as List?)?.length, and
update the SnackBar text to report the actualAddedSessions and
actualAddedRoutines values (keep the same SnackBar creation and AppTheme.success
usage).
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+105
to
+130
| Future<void> reportUsage(Map<String, dynamic> stats) async { | ||
| try { | ||
| final id = await userAppId; | ||
| final payload = { | ||
| 'user_app_id': id, | ||
| 'total_workouts': stats['totalWorkouts'], | ||
| 'weekly_workouts': stats['weeklyWorkouts'], | ||
| 'weekly_volume': stats['weeklyVolume'], | ||
| 'exercises_this_week': stats['exercisesThisWeek'], | ||
| 'platform': _platform, | ||
| 'report_date': DateTime.now().toUtc().toIso8601String(), | ||
| }; | ||
| final response = await _client.post( | ||
| Uri.parse('$_baseUrl/report'), | ||
| headers: {'Content-Type': 'application/json'}, | ||
| body: jsonEncode(payload), | ||
| ); | ||
| if (response.statusCode != 200) { | ||
| debugPrint('Failed to report usage: ${response.body}'); | ||
| } | ||
| } catch (e) { | ||
| debugPrint('Error reporting usage: $e'); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
reportUsage silently sends null if stats map keys are missing.
Lines 110–113 access stats['totalWorkouts'], stats['weeklyWorkouts'], etc. without null checks. If the stats map is missing a key, null is sent in the payload, which may cause a backend validation error or insert corrupt data.
🛡️ Proposed defensive approach
final payload = {
'user_app_id': id,
- 'total_workouts': stats['totalWorkouts'],- 'weekly_workouts': stats['weeklyWorkouts'],- 'weekly_volume': stats['weeklyVolume'],- 'exercises_this_week': stats['exercisesThisWeek'],+ 'total_workouts': stats['totalWorkouts'] ?? 0,+ 'weekly_workouts': stats['weeklyWorkouts'] ?? 0,+ 'weekly_volume': stats['weeklyVolume'] ?? 0.0,+ 'exercises_this_week': stats['exercisesThisWeek'] ?? 0,
'platform': _platform,
'report_date': DateTime.now().toUtc().toIso8601String(),
};🤖 Prompt for AI Agents
In `@workout-logger/lib/services/api_service.dart` around lines 105 - 130, The
reportUsage function currently inserts nulls into the payload when keys like
'totalWorkouts', 'weeklyWorkouts', 'weeklyVolume', or 'exercisesThisWeek' are
missing from the stats map; update reportUsage to defensively validate and
coerce those values before sending: check stats for each required key
(totalWorkouts, weeklyWorkouts, weeklyVolume, exercisesThisWeek), apply sensible
defaults or skip/omit fields when values are null, and ensure numeric fields are
cast to the expected types (int/double) before building payload; if required
fields are absent, log a warning via debugPrint and avoid calling _client.post
(or return early) to prevent sending invalid data.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
workout-logger/lib/services/storage_service.dart (2)
317-374:⚠️ Potential issue | 🟡 MinorGuard each top-level collection before iterating it.
_decodeItem()normalizes each element, butdata['sessions'],data['routines'],data['targets'],data['muscleGroups'], anddata['customExercises']are still iterated asdynamic. A malformed backup like"sessions": {}or"sessions": 42will throw before_decodeItem()ever runs. Please fail fast with aFormatExceptionwhen a section is not aList.🛡️ Example pattern
- if (data['sessions'] != null) {- for (var item in data['sessions']) {+ final sessions = data['sessions'];+ if (sessions != null) {+ if (sessions is! List) {+ throw const FormatException('Expected "sessions" to be a list');+ }+ for (final item in sessions) { final map = _decodeItem(item); final session = WorkoutSession.fromJson(map); final existing = await getWorkoutSession(session.id); if (existing == null) { await saveWorkoutSession(session); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/storage_service.dart` around lines 317 - 374, Before iterating each top-level collection (sessions, routines, targets, muscleGroups, customExercises) validate that data['...'] is a List and throw a FormatException if not; update the blocks that call _decodeItem() (the sessions, routines, targets, muscleGroups, customExercises loops) to first check e.g. if (data['sessions'] is! List) throw FormatException('Invalid backup: "sessions" must be a List'); do the same for 'routines', 'targets', 'muscleGroups', and 'customExercises' so _decodeItem, WorkoutSession.fromJson, Routine.fromJson, Target.fromJson, MuscleGroup.fromJson, and Exercise.fromJson are only called on list elements and malformed backups fail fast.
297-297:⚠️ Potential issue | 🟡 MinorAvoid hardcoding the exported app version.
This metadata will drift from the real build version, so backups can advertise the wrong release during debugging or future import/migration handling. Please source it from the app’s runtime/build version instead.
Based on learnings, "Do not manually edit the
pubspec.yamlversion before merging tomain— the CI automatically bumps the patch version viadart scripts/bump_version.dart patch."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/storage_service.dart` at line 297, Replace the hardcoded 'appVersion': '1.0.6' metadata in storage_service.dart with the runtime/build version: read the app version from the platform/build info (e.g., PackageInfo.version via package_info_plus or your existing build constant) and assign that value to the 'appVersion' field when exporting backups; update any initialization code that constructs the metadata object (the location containing the 'appVersion' key) to asynchronously fetch PackageInfo.fromPlatform().version or use the build-time constant so the exported appVersion always reflects the real build rather than a hardcoded string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/lib/services/storage_service.dart`:
- Around line 282-286: Multiple for-loops in storage_service.dart declare their
loop variable with var even though they are never reassigned; change each loop
variable declaration to final (e.g., change "for (var key in
_settingsBoxInstance.keys)" to "for (final key in _settingsBoxInstance.keys)")
across all occurrences (including the loop that fills settingsMap and the other
13 loops mentioned) so local immutability is enforced; locate loops by searching
for for (...) var ... patterns and update the loop variable to final (references
to symbols like _settingsBoxInstance.keys and settingsMap can help find at least
one occurrence).
---
Duplicate comments:
In `@workout-logger/lib/services/storage_service.dart`:
- Around line 317-374: Before iterating each top-level collection (sessions,
routines, targets, muscleGroups, customExercises) validate that data['...'] is a
List and throw a FormatException if not; update the blocks that call
_decodeItem() (the sessions, routines, targets, muscleGroups, customExercises
loops) to first check e.g. if (data['sessions'] is! List) throw
FormatException('Invalid backup: "sessions" must be a List'); do the same for
'routines', 'targets', 'muscleGroups', and 'customExercises' so _decodeItem,
WorkoutSession.fromJson, Routine.fromJson, Target.fromJson,
MuscleGroup.fromJson, and Exercise.fromJson are only called on list elements and
malformed backups fail fast.
- Line 297: Replace the hardcoded 'appVersion': '1.0.6' metadata in
storage_service.dart with the runtime/build version: read the app version from
the platform/build info (e.g., PackageInfo.version via package_info_plus or your
existing build constant) and assign that value to the 'appVersion' field when
exporting backups; update any initialization code that constructs the metadata
object (the location containing the 'appVersion' key) to asynchronously fetch
PackageInfo.fromPlatform().version or use the build-time constant so the
exported appVersion always reflects the real build rather than a hardcoded
string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8a2abed-f149-42f0-880e-0cc917296c22
📒 Files selected for processing (1)
workout-logger/lib/services/storage_service.dart
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Mar 19, 2026
This was referenced Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores