Skip to content
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: add link from live events view to explore view #29349

Merged
merged 13 commits into from
Mar 1, 2025

Conversation

frankh
Copy link
Contributor

@frankh frankh commented Feb 28, 2025

Problem

clicking on a live event will take you to the explore view filtered for that specific event (so you can check when its been ingested)

Changes

image

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

clicking on a live event will take you to the explore view filtered
for that specific event (so you can check when its been ingested)
@frankh frankh requested a review from Twixes February 28, 2025 11:40
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Added a feature that links live events to the explore view, allowing users to click on an event to see it filtered by UUID in the explore interface.

  • Added Link component in LiveEventsTable.tsx to make events clickable in the live events table
  • Implemented URL construction with encoded JSON query parameters to filter events by their UUID
  • Created search URL that directs to ActivityTab.ExploreEvents with a DataTableNode query configuration
  • Added proper imports for Link, ActivityTab, and PropertyOperator types needed for the implementation

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

propertiesViaUrl: true
}))}`
return (
<Link to={searchUrl}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should instead implement the "Copy link to event" pattern, because this inline link fundamentally doesn't work: at the point in time the user sees a live event come in, that event does not yet exist in ClickHouse for a minute or so (depending on ingestion lag), so this link is effectively a 404

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in DataTable: code

Copy link
Contributor Author

@frankh frankh Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, that's why i made it a search instead of direct to the event, so you can search and refresh until it comes in - maybe we just need a user friendly way of telling users that it might be delayed though

copy link to event has the same issue right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, in the end i went with your way 1) for consistency, and 2) because we already have a tooltip on the event name and adding a second tooltip didn't work - and i wanted the warning tooltip saying it might not show up

see screenshot!

@frankh frankh requested a review from Twixes February 28, 2025 14:47
Copy link
Contributor

github-actions bot commented Feb 28, 2025

Size Change: +2 B (0%)

Total Size: 9.73 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 9.73 MB +2 B (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small refactor and a fix to timestamp filtering in those event links – thanks for putting this out!

@Twixes Twixes enabled auto-merge (squash) February 28, 2025 23:50
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes merged commit e6cdfed into master Mar 1, 2025
95 checks passed
@Twixes Twixes deleted the frank/live-to-explore-link branch March 1, 2025 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants