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

fix(insights): Don't reset new insight when recording opened #21424

Closed
wants to merge 3 commits into from

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Apr 9, 2024

Problem

When you opened a recording while making a new insight… well, you no longer had that insight:

Screen.Recording.2024-04-09.at.09.39.07.mov

Reported in ZEN-12286 (issue #1).

Changes

This was caused by an old branch in the code, which allowed easy resetting of the new insight by clicking the "New insight" navbar button again. However, this conflicted with the session recording ID hash param set by the player, which is why we also unintentionally reset the insight when merely opening a recording. Safer to remove this behavior.

@Twixes Twixes requested a review from a team April 9, 2024 07:42
@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 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Apr 9, 2024

Size Change: 0 B

Total Size: 845 kB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 845 kB

compressed-size-action

@Twixes
Copy link
Member Author

Twixes commented Apr 9, 2024

Hmm, there's a couple failing E2E tests, and they're very valid. This is tricky. Closing in favor of a different approach in #21449

@Twixes Twixes closed this Apr 9, 2024
@Twixes Twixes deleted the fix-new-insight-recording-reset branch April 18, 2024 21:54
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