Feat/activity log backend - #367
Conversation
Signed-off-by: Payalrvs0310@gmail.com <Payalrvs0310@gmail.com>
PR Context Summary
Suggested issue links
Use |
|
Warning Review limit reached
Next review available in: 20 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a local append-only activity log with SQLAlchemy persistence, scoped listing and deletion APIs, configurable retention, event recording across media, vault, settings, and image processing flows, comprehensive tests, and feature documentation. ChangesActivity Log
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EventSource
participant record_activity
participant ActivityDatabase
EventSource->>record_activity: record completed state change
record_activity->>ActivityDatabase: insert and commit Activity
ActivityDatabase-->>record_activity: commit result
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
ApprovabilityVerdict: Needs human review Unable to check for correctness in 1121670. This PR introduces a new activity logging feature with database schema changes, new API endpoints, and integration across multiple existing routers and workers. All 16 changed files are owned by a different developer (Abhash-Chakraborty), warranting review by the designated code owner. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/src/find_api/routers/activity.py`:
- Around line 42-111: Move the database query construction and execution from
list_activity, clear_activity, and purge_expired_activity into
services/activity_log.py, reusing or adding service functions for scoped
filtering, ordering, pagination, counting, and bulk deletion. Keep the router
handlers as thin HTTP adapters that validate inputs, call the activity-log
service, and return the existing response shapes and messages.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7eeb9362-cd78-4af7-bba7-b91a7b4cbdc3
📒 Files selected for processing (15)
backend/alembic/versions/20260715_activity_log.pybackend/src/find_api/core/config.pybackend/src/find_api/core/database.pybackend/src/find_api/core/dependencies.pybackend/src/find_api/main.pybackend/src/find_api/models/__init__.pybackend/src/find_api/models/activity.pybackend/src/find_api/routers/activity.pybackend/src/find_api/routers/config.pybackend/src/find_api/routers/gallery.pybackend/src/find_api/routers/vault.pybackend/src/find_api/services/activity_log.pybackend/src/find_api/workers/jobs.pybackend/tests/test_activity.pydocs/guides/features.md
Signed-off-by: Payalrvs0310@gmail.com <Payalrvs0310@gmail.com>
|
@macroscope-app review Please review this PR against its linked issue, local-first privacy rules, and the current Find repo instructions. |
|
This is excellent work. The things I'd normally have to ask for are already here: One thing to fix before I merge:
One note, not a change request: in Fix the lock attribution and I'll merge this. |
Signed-off-by: Payalrvs0310@gmail.com <Payalrvs0310@gmail.com>
|
Hey @Abhash-Chakraborty, Branch also picked up #377/#379 from your side along the way, let me know if you'd rather I do anything about that, otherwise this is ready on the vault fix. |
Attributing the lock to the acting user via get_required_user broke the route once an admin exists. The client sends the *vault* session token in the Authorization header, and get_required_user reads that same header: get_current_user cannot resolve a vault token to a user, so shared mode took the 401 branch and the vault never locked server-side. Resolve the actor from the find_session login cookie instead, using the non-raising get_current_user. The frontend already sends the cookie (withCredentials), so real sessions still get attribution, and a cookie-less API client locks successfully with an unattributed row. The existing lock test only ran in local mode, where get_required_user returns None, so nothing caught this. Added shared-mode coverage for both the status code and the attribution. Claude-Session: https://claude.ai/code/session_01UBTPUoE8ZKveSPEuhAWkVT Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Pushed a fix to your branch rather than sending you round again for it — the lock attribution was right in principle but it 401s in shared mode, and that's my fault for asking for it without flagging the header collision.
The actor now comes from the Nothing else outstanding from me. Once CI is green this is going in. On #377/#379 riding along: leave them, they're mine and they'll disappear when this squashes. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/test_vault.py (1)
532-552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the cookie-less attribution branch.
This verifies that locking succeeds, but not that its
vault/lockedactivity row is unattributed. Query the latest locked row and assertuser_id is Noneto protect the intended cookie-less behavior.🤖 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 `@backend/tests/test_vault.py` around lines 532 - 552, Update test_lock_succeeds_with_vault_token_in_header to query the latest vault/locked activity row after the lock request and assert its user_id is None, preserving the existing response assertions.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@backend/tests/test_vault.py`:
- Around line 532-552: Update test_lock_succeeds_with_vault_token_in_header to
query the latest vault/locked activity row after the lock request and assert its
user_id is None, preserving the existing response assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e80f3780-97c0-4f70-ac67-beca42058914
📒 Files selected for processing (2)
backend/src/find_api/routers/vault.pybackend/tests/test_vault.py
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Approved. The vault-lock regression is fixed and covered, everything else here was already in good shape — best-effort recording that can't roll back real work, SET NULL on both FKs, a hand-picked payload allow-list, and scope_activity_query mirroring the media scoping. Marked ready-to-merge.
Also cleared the needs linked issue label: you did link #354, the triage bot just doesn't recognise "Part of" as a link. Using it deliberately so #354 stays open for the UI half was the right call — that's on my tooling, not you.
Every pip-authored PR against /backend has been dead on arrival. The pip
ecosystem edits pyproject.toml and stops there -- it does not know uv.lock
exists. backend-check runs `uv sync --locked`, so each PR failed with:
error: The lockfile at `uv.lock` needs to be updated, but `--locked`
was provided.
That killed #387, #388, #389, #390 and #391 in the same batch, and it is
not a new problem: the backend pins have been frozen behind it long enough
that pgvector is still on 0.2.4 (current 0.5.0) and minio on 7.2.3
(current 7.2.20). The major/minor split added in #384 could not help,
because the failure has nothing to do with which versions were proposed.
The uv ecosystem updates the manifest and the lockfile in the same commit,
which is the only shape that can pass this repo's CI. The torch/torchvision
ignore and the group split carry over unchanged.
Also fixes the linked-issue check in pr-context-triage, which only counted
closing keywords. A PR implementing half of a tracking issue has to write
"Part of #N" instead of "Closes #N" or merging it auto-closes work that is
still open -- so the rule punished contributors for splitting work
correctly, and mislabelled #367 as `needs linked issue` when it linked #354
on purpose. Non-closing links now count, matched off explicit keywords
rather than any bare #N so a passing mention of an unrelated PR does not
qualify. The Macroscope trigger comment reports both kinds too, since a
"Part of" issue still holds the acceptance criteria the review is judged on.
Claude-Session: https://claude.ai/code/session_01UBTPUoE8ZKveSPEuhAWkVT
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Approved — migration present, reads owner-scoped via scope_activity_query, payload contract documented and privacy-safe.
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Approved — migration present, reads owner-scoped via scope_activity_query, payload contract documented and privacy-safe.
Two review findings. ACTIVITY_RETENTION_DAYS accepted negative values. 0 is a documented "keep forever" switch, but -1 has no meaning and silently disabled cleanup rather than failing, so an environment typo looked like it worked. Now validated as non-negative, along with TRASH_RETENTION_DAYS, which had the same gap. list_activity/clear_activity/purge_expired_activity built and ran their SQLAlchemy filtering, ordering, pagination and bulk deletes inline in the router. services/activity_log.py already existed for record_activity and is the natural home for the rest, per the thin-router guideline. The endpoints now do request/response handling only; owner scoping moves with the queries so it cannot be forgotten at one call site.
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Approved — the retention validator and router/service split are fixed in code; migration, owner scoping, and privacy-safe payload contract all check out.
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Approved — retention validator and router/service split fixed in code; migration, owner scoping and payload contract all verified.
2c17c82
into
Abhash-Chakraborty:canary
Abhash-Chakraborty#367 landed as a squash commit, so the backend half of the activity log arrives in canary as new content rather than as this branch's own commits. That turns the three files this branch carried from Abhash-Chakraborty#367 into add/add conflicts: routers/activity.py, services/activity_log.py, and tests/test_activity.py. Resolved by taking canary wholesale for backend/ and docs/, since everything this branch held there was an older copy of Abhash-Chakraborty#367's work -- including the pre-review router, before the query logic moved into services/activity_log.py. The diff against canary is now frontend only.
Frontend half of the local activity log, over the /api/activity endpoints from #367. - /activity route: day-grouped feed, category filters (upload/media/vault/ settings), load-more pagination, and a Clear control. - lib/activity.ts holds pure describeActivity/activityTone/groupActivityByDay, unit-tested without rendering. - lib/api.ts gains getActivity/clearActivity/purgeActivity. - Nav link added under Utilities. - Retention purge fires on page view, mirroring how Trash auto-purges on open. States and accessibility: skeleton with role=status/aria-busy, role=alert with a retry on the error path, empty state, aria-pressed filter chips, sr-only fieldset legend, aria-hidden icons. All colours resolve through the CSS custom properties, so light and dark both stay readable. Pagination compensates for offset drift: the feed is newest-first and append-only, so rows recorded mid-pagination would otherwise make Load more re-serve visible rows and skip the same number of older ones. The next offset is pushed past the drift between the live total and the first page's snapshot, and the flatten step drops ids already rendered so page seams cannot duplicate a row. The retention purge invalidates the activity query once it has actually deleted something, so it can no longer lose the race against the initial load. Fixes #354 Co-authored-by: payalrvs3 <Payalrvs0310@gmail.com>
Summary
Adds the backend half of the local activity log: an append-only
Activitymodel, a best-effort recorder wired into upload, archive/trash/restore, vault lock/unlock/restore, and settings-change flows, and a paginated/ filterable read + retention API. UI route is a follow-up PR.Part of #354 - this covers the data model, recording, and API; the timeline UI and accessibility pass are tracked separately in that issue, so I've used "Part of" rather than "Fixes" to avoid auto-closing it.
Type of change
Release impact
What changed
Activitymodel + migration (single head, fulldowngrade()).record_activity()- best-effort writer, never raises, wired at upload completion/failure, archive/trash/restore, vault lock/unlock/restore, and settings changes.GET/POST /activity,/activity/clear,/activity/purge- scoped via newscope_activity_query(mirrorsscope_media_query).ACTIVITY_RETENTION_DAYS(default 90), mirrorsTRASH_RETENTION_DAYS.How to test
cd backend && uv run ruff check . && uv run ruff format --check . && uv run pytest tests/ -v
→ 545 passed, 6 skipped, 0 regressions (19 new tests in test_activity.py).
Checklist
canaryunless it is the maintainer promotion PRGSSoC'26 checklist
Summary by CodeRabbit