Last activity date display in project page - #259
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds nullable project activity timestamps, backfills existing activity, exposes the timestamp in project queries, and updates it transactionally during translated-verse upserts and chapter-assignment changes. ChangesProject activity tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TranslatedVersesService
participant ChapterAssignmentsService
participant Transaction
participant ProjectsService
participant Repository
TranslatedVersesService->>Transaction: Begin transaction
TranslatedVersesService->>Repository: Upsert translated verse
Repository-->>TranslatedVersesService: Return upsert result
TranslatedVersesService->>ProjectsService: Touch project activity
ProjectsService->>Transaction: Update lastActivityAt
ChapterAssignmentsService->>Transaction: Persist assignment and history
ChapterAssignmentsService->>ProjectsService: Touch project activity
ProjectsService->>Transaction: Update lastActivityAt
Transaction-->>TranslatedVersesService: Commit transaction
Transaction-->>ChapterAssignmentsService: Commit transaction
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/scripts/backfill-project-last-activity.ts`:
- Around line 20-23: Update the SQL query around the project activity
aggregation to pre-aggregate chapter assignments and translated verses by
project_unit_id before joining them to project_units. Combine those per-unit
aggregates afterward, preserving the existing project-level MAX behavior while
avoiding the many-to-many join in the main query.
- Around line 10-26: Update the projects UPDATE statement so last_activity_at is
assigned only when activity.max_ts is newer than the existing
p.last_activity_at. Preserve newer timestamps set by touchLastActivity while
retaining the current source timestamp calculation and epoch filter.
In `@src/domains/projects/projects.service.ts`:
- Around line 40-50: Update the failed lookup branch in upsertTranslatedVerse
after repo.getProjectIdByUnitId returns unsuccessfully so it propagates the
resolution error instead of returning normally, ensuring the transaction caller
rolls back and does not commit without updating lastActivityAt.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7446b473-81fc-4ae4-bd13-c39a1b728576
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
src/db/migrations/0015_add_project_last_activity.sqlsrc/db/migrations/meta/0015_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/db/scripts/backfill-project-last-activity.tssrc/domains/projects/projects.query-builder.tssrc/domains/projects/projects.repository.tssrc/domains/projects/projects.service.tssrc/domains/translated-verses/translated-verses.repository.tssrc/domains/translated-verses/translated-verses.service.ts
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
Validated CodeRabbit's findings against the current branch (ft/last-activity). All three are confirmed as real issues:
1. projects.service.ts:42 — Failed activity resolution is silently swallowed
touchProjectActivity logs and does a bare return when repo.getProjectIdByUnitId fails, instead of throwing. Its caller in translated-verses.service.ts (upsertTranslatedVerse) awaits it but ignores the (void) return value, so the enclosing db.transaction still resolves ok and commits — the translated verse is saved but last_activity_at silently never updates, with only a log line as evidence.
Fix: propagate the resolution error out of touchProjectActivity (or have it return a Result) so the transaction rolls back / surfaces the failure.
2. backfill-project-last-activity.ts:21 — Row-explosion join before aggregation
The query LEFT JOINs chapter_assignments and translated_verses directly on project_unit_id with no per-table pre-aggregation:
FROM project_units pu
LEFT JOIN chapter_assignments ca ON ca.project_unit_id = pu.id
LEFT JOIN translated_verses tv ON tv.project_unit_id = pu.id
GROUP BY pu.project_idFor a unit with M chapter assignments and N translated verses, this produces M×N intermediate rows before the GROUP BY collapses them — potentially tens of thousands of rows per book-length unit, multiplied across every project unit in the table.
Fix: aggregate chapter_assignments and translated_verses by project_unit_id independently, then combine the two aggregates.
3. backfill-project-last-activity.ts:12 — Unconditional overwrite can regress a newer timestamp on rerun
SET last_activity_at = activity.max_ts always replaces the column. touchLastActivity sets last_activity_at = new Date() independent of the source tables' updated_at. If a contributing chapter_assignments/translated_verses row is later deleted (or its updated_at becomes non-representative), a rerun recomputes a lower GREATEST(...) and overwrites a legitimately newer value — a real risk given the script is explicitly documented as "safe to re-run."
Fix: only update when the new max is greater than the existing last_activity_at (CodeRabbit's suggested CASE guard is correct).
Requesting changes on items 1 and 3 (correctness); item 2 is a performance concern worth addressing before this runs against production-scale data.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domains/chapter-assignments/chapter-assignments.service.ts (1)
201-215: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefer AI work until the owner transaction commits.
updateChapterAssignmentqueues AI work as soon asexec(externalTx)returns, but the owner transaction has not committed yet. Users can roll back that transaction, so current project assignment APIs can enqueue AI suggestions for changes that did not persist. Return the AI trigger state to the owner, or write the work through an outbox processed only after commit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/chapter-assignments.service.ts` around lines 201 - 215, Update updateChapterAssignment so AI work is not queued immediately when exec(externalTx) completes before the owner transaction commits. Return the AI trigger state to the caller for external transactions, or persist it through an outbox that is processed only after commit; retain the existing db.transaction behavior for internally owned transactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/domains/chapter-assignments/chapter-assignments.service.ts`:
- Around line 201-215: Update updateChapterAssignment so AI work is not queued
immediately when exec(externalTx) completes before the owner transaction
commits. Return the AI trigger state to the caller for external transactions, or
persist it through an outbox that is processed only after commit; retain the
existing db.transaction behavior for internally owned transactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5827f30-4a04-446b-babb-a1fc1715f528
📒 Files selected for processing (3)
src/db/scripts/backfill-project-last-activity.tssrc/domains/chapter-assignments/chapter-assignments.service.tssrc/domains/projects/projects.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/db/scripts/backfill-project-last-activity.ts
- src/domains/projects/projects.service.ts
39d5be5 to
cba98f2
Compare
2 participants
Last activity date display in project page
Summary by CodeRabbit