Skip to content

Conversation

@Yopi
Copy link
Contributor

@Yopi Yopi commented Oct 31, 2025

Backend:

  • Adds a hierarchical grouping of events (via parent_event_id).
  • Allows ingestion with a external_id as an idempotency key. Events with the same external_id are not reingested
  • Allows ingestion of a parent_id either as a Polar event.id or as a external_id. We do the lookup on our side to allow both sides.

Frontend:

  • Add display of nested events (in one level for now)
  • Add pagination of nested events
Screenshot 2025-10-31 at 12 05 38 Screenshot 2025-10-31 at 12 05 46 Screenshot 2025-10-31 at 12 05 51

@vercel
Copy link

vercel bot commented Oct 31, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
polar Ignored Ignored Preview Nov 3, 2025 2:24pm
polar-sandbox Ignored Ignored Preview Nov 3, 2025 2:24pm

@Yopi Yopi force-pushed the introduce-spans-of-events branch 2 times, most recently from 4a2bc40 to 7a93761 Compare October 31, 2025 14:55
@emilwidlund
Copy link
Member

Great work on this!

I wonder if we should rename external_event_id to idempotency_key instead? I wonder if that better conveys the purpose of it. But that's just my 2 cents :)

@pieterbeulque
Copy link
Contributor

@Yopi one thing I noticed during your demo was that with the pagination, the 6 children for a span became 10 children when you clicked "Show more", is that a bug in the pagination logic?

@Yopi
Copy link
Contributor Author

Yopi commented Nov 3, 2025

@Yopi one thing I noticed during your demo was that with the pagination, the 6 children for a span became 10 children when you clicked "Show more", is that a bug in the pagination logic?

Good catch! It's the 10 children that goes to 13 children, because we actually don't know how many there are. It's a bit complicated to get the full counts in the recursive function. We could try a bit more to do it in a good way, or we could work with the UX. The button shows up if the count is the same as the limit of children (https://github.com/polarsource/polar/pull/7609/files#diff-bd567ed3816662d20cecb8b5111fa9c68085ed30b08458815dc178d840a09ad5R44)

@pieterbeulque
Copy link
Contributor

Then what if you have exactly 10 child events? 🤔

@Yopi
Copy link
Contributor Author

Yopi commented Nov 3, 2025

Then what if you have exactly 10 child events? 🤔

Haha exactly. If we think it's decent enough to do this without real pagination we could select one more to be able to determine if more exists and pass that to the frontend.

@Yopi
Copy link
Contributor Author

Yopi commented Nov 3, 2025

I was reading our docs, and this type of pagination strays a bit from our promise on how we do pages (page, limit, max_count), but on the other hand I don't think it's feasible for us to get a max count for each depth. I pushed a change now that we can at least know if there are more children to load or not and maybe that's fine.

I can check how this looks and behaves in the SDK.

@Yopi Yopi force-pushed the introduce-spans-of-events branch 2 times, most recently from 08cab4a to ea44d30 Compare November 3, 2025 14:09
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

📦 Next.js Bundle Analysis for web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Yopi added 3 commits November 3, 2025 15:17
This will allow us to store events hierarchically and introduce a span-type structure where different
events are related to each other and can be bundled up.
@Yopi Yopi force-pushed the introduce-spans-of-events branch from ea44d30 to 457728e Compare November 3, 2025 14:17
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

📦 Next.js Bundle Analysis for web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@Yopi Yopi marked this pull request as ready for review November 3, 2025 14:25
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

📦 Next.js Bundle Analysis for web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link
Contributor

@pieterbeulque pieterbeulque left a comment

Choose a reason for hiding this comment

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

I like it! The API is backwards compatible in this shape, right? (i.e. everything new is optional?)

@pieterbeulque
Copy link
Contributor

pieterbeulque commented Nov 3, 2025

Actually, should external_id be unique only for the specified event name?

This would horribly break parent_event_id, sorry.

@Yopi
Copy link
Contributor Author

Yopi commented Nov 3, 2025

I like it! The API is backwards compatible in this shape, right? (i.e. everything new is optional?)

Precisely, everything new is optional!

@Yopi Yopi merged commit 41066c5 into main Nov 3, 2025
15 checks passed
@Yopi Yopi deleted the introduce-spans-of-events branch November 3, 2025 16:44
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.

4 participants