Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91a1612a0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if table == "reading_activities" and row is not None and schema.model_validate(row.payload) != parsed: | ||
| raise ValidationError("Activity and period history are immutable; append a correction") |
There was a problem hiding this comment.
When a goal_periods row already exists, this immutability check is skipped, and the later validation only fixes goal_id and start_date. A PowerSync PUT/PATCH can therefore rewrite the period's target, end date, metric, or rule history, corrupting the retained history used by clients to reproject past progress. Include existing period rows in the immutable-payload comparison.
Useful? React with 👍 / 👎.
| if definition.time_period != "custom": | ||
| definition.start_date, definition.end_date = calendar_period(definition, now) |
There was a problem hiding this comment.
For every non-custom goal this overwrites the validated request dates with the calendar period containing now, even when is_recurring is false. For example, creating a one-time yearly goal for 2027 during 2026 persists and returns the 2026 period instead, silently activating the goal in the wrong interval. Calendar realignment should not replace the explicit dates of a non-recurring goal.
Useful? React with 👍 / 👎.
| changes = request.model_dump(exclude_unset=True, exclude={"end_date"}) | ||
| if changes.get("is_archived"): | ||
| changes["is_active"] = False | ||
| updated = definition.model_copy(update=changes) |
There was a problem hiding this comment.
The request schema accepts explicit nulls for fields such as target_value, is_active, and is_archived, and exclude_unset=True retains those nulls here. model_copy then installs them without validation, after which constructing GoalRule raises an uncaught Pydantic validation error and the PATCH returns HTTP 500. These null values should either be rejected at request validation or omitted before building the rule.
Useful? React with 👍 / 👎.
| title=title, | ||
| reading_time_minutes=values["seconds"] // 60, | ||
| pages_read=int(values["pages"]), | ||
| sessions_count=sum(entry.book_id == book_id for entry in counted), |
There was a problem hiding this comment.
When a reader session is represented by multiple activity chunks sharing one session_id, the sessions endpoint groups those chunks and the total statistics count the identifier once, but this per-book field counts every chunk. The same response can consequently report one total session and multiple sessions for its only book; count distinct session_id or id values per book instead.
Useful? React with 👍 / 👎.
| sessions_count=len({entry.session_id or entry.id for entry in counted}), | ||
| average_session_minutes=total["seconds"] / 60 / len(counted) if counted else 0, |
There was a problem hiding this comment.
For sessions split across multiple activity rows with the same session_id, sessions_count correctly deduplicates the identifier but the average divides total reading time by the number of activity rows. A single 20-minute session uploaded as two chunks is therefore reported as one session with a 10-minute average. Use the same distinct-session count as the denominator.
Useful? React with 👍 / 👎.
Summary
Replace example goal/session responses with owned persisted goals and an immutable reading-activity ledger. Add calendar-aware projections for books, time, pages, and reading days; merge concurrent rules and activity, retain deletion history, and expose tracking schema version 1 through sync settings.
Testing
Compatibility and migration
Apply additive Alembic revision
b5c6d7e8f901, update PostgreSQL publication/grants with the revised bootstrap, and deploy PowerSync streams before releasing the integrated client. Existing metric strings remain;reading_daysandtracking_schema_version: 1are additive. Older servers retain client-local tracking through staging.Notes
Target
development; versions remain unchanged. Coordinate with reader PR #5 and client #40. Seedocs-tracking.mdfor deployment order. Real multi-device PowerSync transport is an opt-in validation lane and has not been exercised by these isolated database tests.Workspace integration #13 pins all three component revisions and documents release order.