feat(data-warehouse): log view deletions and name views in activity feed#61023
Merged
Conversation
Add a `deleted` activity-log entry when a data warehouse saved query is deleted, capturing the view name before the soft-delete path scrambles it. Update the saved query activity describers to include the view name across all activity types (created, updated, sync triggered/cancelled, materialization enabled/disabled, sync frequency reset, deleted), falling back to "a view" when the name is unavailable. Generated-By: PostHog Code Task-Id: 131bbc95-0109-471d-bd45-c9104b823faa
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/scenes/data-warehouse/saved_queries/activityDescriptions.tsx:65-69
When `logItem.detail?.name` is absent, `viewName` resolves to `<i>a view</i>`, making both of these read "created view *a view*" — the duplicated noun is grammatically awkward. Dropping the static word "view" from these two phrases (matching the pattern used for sync/materialization messages) fixes it.
```suggestion
if (logItem.activity === 'created') {
return {
description: <SentenceList listParts={[<>created {viewName}</>]} prefix={user} />,
}
}
```
### Issue 2 of 2
frontend/src/scenes/data-warehouse/saved_queries/activityDescriptions.tsx:136-140
Same issue for the `deleted` branch — with the fallback it renders "deleted view *a view*". Removing the static word keeps it consistent with the suggested fix for `created`.
```suggestion
if (logItem.activity === 'deleted') {
return {
description: <SentenceList listParts={[<>deleted {viewName}</>]} prefix={user} />,
}
}
```
Reviews (1): Last reviewed commit: "feat(data-warehouse): log view deletions..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
65
to
69
| if (logItem.activity === 'created') { | ||
| return { | ||
| description: <SentenceList listParts={[<>created a new view</>]} prefix={user} />, | ||
| description: <SentenceList listParts={[<>created view {viewName}</>]} prefix={user} />, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
logItem.detail?.name is absent, viewName resolves to <i>a view</i>, making both of these read "created view a view" — the duplicated noun is grammatically awkward. Dropping the static word "view" from these two phrases (matching the pattern used for sync/materialization messages) fixes it.
Suggested change
| if (logItem.activity === 'created') { | |
| return { | |
| description: <SentenceList listParts={[<>created a new view</>]} prefix={user} />, | |
| description: <SentenceList listParts={[<>created view {viewName}</>]} prefix={user} />, | |
| } | |
| } | |
| if (logItem.activity === 'created') { | |
| return { | |
| description: <SentenceList listParts={[<>created {viewName}</>]} prefix={user} />, | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/data-warehouse/saved_queries/activityDescriptions.tsx
Line: 65-69
Comment:
When `logItem.detail?.name` is absent, `viewName` resolves to `<i>a view</i>`, making both of these read "created view *a view*" — the duplicated noun is grammatically awkward. Dropping the static word "view" from these two phrases (matching the pattern used for sync/materialization messages) fixes it.
```suggestion
if (logItem.activity === 'created') {
return {
description: <SentenceList listParts={[<>created {viewName}</>]} prefix={user} />,
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+136
to
+140
| if (logItem.activity === 'deleted') { | ||
| return { | ||
| description: <SentenceList listParts={[<>deleted view {viewName}</>]} prefix={user} />, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
deleted branch — with the fallback it renders "deleted view a view". Removing the static word keeps it consistent with the suggested fix for created.
Suggested change
| if (logItem.activity === 'deleted') { | |
| return { | |
| description: <SentenceList listParts={[<>deleted view {viewName}</>]} prefix={user} />, | |
| } | |
| } | |
| if (logItem.activity === 'deleted') { | |
| return { | |
| description: <SentenceList listParts={[<>deleted {viewName}</>]} prefix={user} />, | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/data-warehouse/saved_queries/activityDescriptions.tsx
Line: 136-140
Comment:
Same issue for the `deleted` branch — with the fallback it renders "deleted view *a view*". Removing the static word keeps it consistent with the suggested fix for `created`.
```suggestion
if (logItem.activity === 'deleted') {
return {
description: <SentenceList listParts={[<>deleted {viewName}</>]} prefix={user} />,
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
|
Size Change: 0 B Total Size: 80.9 MB ℹ️ View Unchanged
|
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
…vity When a view name is unavailable the describer falls back to "a view", so "created view a view" / "deleted view a view" read awkwardly. Drop the static "view" word from the created and deleted branches to match the sync/materialization phrasing. Generated-By: PostHog Code Task-Id: 131bbc95-0109-471d-bd45-c9104b823faa
Generated-By: PostHog Code Task-Id: 131bbc95-0109-471d-bd45-c9104b823faa
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No showstoppers. The backend deletion logging follows the established pattern in the viewset, the third argument to defaultDescriber is valid per its signature (resource?: string | JSX.Element), tests cover the new behavior, and the bot's inline comments are from an older commit that don't apply to the current diff.
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 free
to 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.
Problem
The data warehouse saved query activity feed had two gaps:
Changes
deletedactivity entry in the saved query viewset's destroy path. The view name is captured beforedelete_saved_queryruns, because the soft-delete path scramblesname(it gets prefixed withPOSTHOG_DELETED_and the real name moves todeleted_name).deletedbranch and threaded the view name into thedefaultDescriberfallback.How did you test this code?
I'm an agent. I added an automated assertion to
test_deleteverifying that adeletedactivity log with the correct view name is recorded after deletion. I was not able to run the Python test suite in this environment because the dev environment (flox/venv) was not bootstrapped, so the test has not been executed locally — CI will run it. The frontend change was checked against thedefaultDescribersignature, which already accepts the optionalresourceargument being passed.Automatic notifications
🤖 Agent context
Authored with Claude Code (Opus 4.8). The non-obvious decision was capturing
instance.namebefore callingdelete_saved_query: the soft-delete moves the real name todeleted_nameand overwritesnamewith aPOSTHOG_DELETED_placeholder, so logging after deletion would record the placeholder. The activity describer reuses the existingdefaultDescriberresource fallback rather than duplicating the rendering logic.