-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: move notifications out of activity log space #35233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: move notifications out of activity log space #35233
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR moves notification functionality out of the activity log space into a dedicated endpoint. The main changes include:
- Creating a new
/my_notifications
API endpoint - Moving
ServerTimingsGathered
toposthog.api.utils
- Updating API routes in frontend and tests
- Moving and adapting notification-related tests
- Implementing efficient notification handling with deduplication logic
This architectural change prepares the system for future notifications that aren't tied to activity logs, making the codebase more modular and logically organized.
Confidence score: 4/5
- The PR is safe to merge as it's a well-structured internal API change with comprehensive test coverage
- The score is 4 because while the changes are well-tested and logical, the lack of redirects for old endpoints could cause temporary disruption to internal services
- Files needing attention:
posthog/api/test/test_my_notifications.py
: The API endpoint for bookmarking notifications hasn't moved with the rest of the notifications codefrontend/src/scenes/session-recordings/apm/performance-event-utils.test.ts
: Test URLs need to be updated for consistency
10 files reviewed, 3 comments
Edit PR Review Bot Settings | Greptile
Size Change: 0 B Total Size: 2.62 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves the notifications API from the activity log namespace to its own dedicated endpoint to prepare for adding notifications that aren't based on activity log items. The change makes the API structure more logical and reduces confusion when notifications include non-activity-log content.
Key changes:
- Moved notifications API from
/activity_log/important_changes
to/my_notifications
- Created new dedicated
MyNotificationsViewSet
and related logic - Updated frontend components to use the new API endpoint
- Migrated all notification-related tests to the new structure
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
posthog/api/my_notifications.py |
New dedicated API viewset for notifications with bookmark functionality |
posthog/api/activity_log.py |
Removed notification-related methods and imports |
posthog/api/test/test_my_notifications.py |
New comprehensive test suite for notifications API |
posthog/api/test/test_activity_log.py |
Removed notification tests, cleaned up imports |
posthog/settings/web.py |
Updated API route pattern for notifications |
posthog/api/__init__.py |
Registered new notifications router |
frontend/src/layout/navigation-3000/sidepanel/panels/activity/sidePanelNotificationsLogic.tsx |
New frontend logic for notifications |
frontend/src/layout/navigation-3000/sidepanel/panels/activity/sidePanelActivityLogic.tsx |
Removed notification logic from activity panel |
frontend/src/layout/navigation-3000/sidepanel/panels/activity/SidePanelActivity.tsx |
Updated to use separate notifications logic |
frontend/src/mocks/handlers.ts |
Updated mock endpoint |
frontend/src/scenes/session-recordings/apm/performance-event-utils.test.ts |
Updated test URL |
frontend/src/scenes/session-recordings/apm/__snapshots__/performance-event-utils.test.ts.snap |
Updated snapshot |
ee/session_recordings/session_summary/summarize_session.py |
Updated import path |
Comments suppressed due to low confidence (2)
posthog/api/test/test_my_notifications.py:273
- The test name suggests it marks notifications as read, but the implementation marks them as unread. Consider renaming to 'test_reading_notifications_marks_them_unread' or updating the logic to match the name.
]
posthog/api/test/test_activity_log.py:73
- The test name 'test_reading_notifications_marks_them_unread' appears to be incorrect - reading notifications should typically mark them as read, not unread. This test was removed in the refactor, but the naming inconsistency suggests there may have been confusion about the expected behavior.
def _create_and_edit_things(self):
📸 UI snapshots have been updated18 snapshot changes in total. 0 added, 18 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated18 snapshot changes in total. 0 added, 18 modified, 0 deleted:
Triggered by this commit. |
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
we're going to add some notifications that aren't based on activity log items
so let's move the notifications API out of the activity log view set
that way it isn't surprising when it loads things that aren't in the activity log
(i've not set up redirects because it's an internal API 🙈)
tested by checking the API still works in the UI