-
Notifications
You must be signed in to change notification settings - Fork 206
chore: add a posthog react playground that does nothing #2514
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset (and the release label) is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
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.
11 files reviewed, 1 comment
| useEffect(() => { | ||
| const newEvents = events.filter((e) => !eventsWithTimestamp.find((existing) => existing.uuid === e.uuid)) | ||
| if (newEvents.length > 0) { | ||
| setEventsWithTimestamp((prev) => [...prev, ...newEvents.map((e) => ({ ...e, capturedAt: Date.now() }))]) | ||
| } | ||
| }, [events, eventsWithTimestamp]) |
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.
logic: infinite loop: eventsWithTimestamp is in the dependency array but also updated in the effect body
| useEffect(() => { | |
| const newEvents = events.filter((e) => !eventsWithTimestamp.find((existing) => existing.uuid === e.uuid)) | |
| if (newEvents.length > 0) { | |
| setEventsWithTimestamp((prev) => [...prev, ...newEvents.map((e) => ({ ...e, capturedAt: Date.now() }))]) | |
| } | |
| }, [events, eventsWithTimestamp]) | |
| useEffect(() => { | |
| const newEvents = events.filter((e) => !eventsWithTimestamp.find((existing) => existing.uuid === e.uuid)) | |
| if (newEvents.length > 0) { | |
| setEventsWithTimestamp((prev) => [...prev, ...newEvents.map((e) => ({ ...e, capturedAt: Date.now() }))]) | |
| } | |
| }, [events]) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: playground/react-nextjs/app/EventDisplay.tsx
Line: 19:24
Comment:
**logic:** infinite loop: `eventsWithTimestamp` is in the dependency array but also updated in the effect body
```suggestion
useEffect(() => {
const newEvents = events.filter((e) => !eventsWithTimestamp.find((existing) => existing.uuid === e.uuid))
if (newEvents.length > 0) {
setEventsWithTimestamp((prev) => [...prev, ...newEvents.map((e) => ({ ...e, capturedAt: Date.now() }))])
}
}, [events])
```
How can I resolve this? If you propose a fix, please make it concise.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.
I think greptile is right on this, it looks like an infinite loop
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.
hmmm, it didn't have a problem in the UI. you'd hope react is clever than that 🫠
|
Size Change: 0 B Total Size: 4.86 MB ℹ️ View Unchanged
|
bf050dc to
7dd3ef8
Compare
| }, | ||
| "dependencies": { | ||
| "next": "14.2.33", | ||
| "posthog-js": "*", |
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.
if you import like this you will need to update the pnpm-workspace to use the pnpmfile inside playground folder. You can have a look at nextjs playground to see a example
Merge activity
|

#2504 had some weird unrelated surveys failure
so let's split it up and see what the what
importantly that let me see this playground was bootstrapped in the previous PR with different versions of things, let's avoid that!