Skip to content

fix: Memory leak with cal embed modal box#17016

Merged
CarinaWolli merged 4 commits into
calcom:mainfrom
nusr:fix-memory-leak
Oct 14, 2024
Merged

fix: Memory leak with cal embed modal box#17016
CarinaWolli merged 4 commits into
calcom:mainfrom
nusr:fix-memory-leak

Conversation

@nusr
Copy link
Copy Markdown

@nusr nusr commented Oct 9, 2024

What does this PR do?

dup

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

@vercel
Copy link
Copy Markdown

vercel Bot commented Oct 9, 2024

@nusr is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app Bot added the community Created by Linear-GitHub Sync label Oct 9, 2024
@graphite-app graphite-app Bot requested a review from a team October 9, 2024 07:43
@github-actions github-actions Bot added embed area: embed, widget, react embed 🐛 bug Something isn't working labels Oct 9, 2024
@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Oct 9, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (10/09/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (10/09/24)

1 label was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (10/12/24)

1 label was added to this PR based on Keith Williams's automation.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 12, 2024

E2E results are ready!

@CarinaWolli CarinaWolli merged commit c140b01 into calcom:main Oct 14, 2024
@clicktodev
Copy link
Copy Markdown
Contributor

clicktodev commented Oct 15, 2024

I don't know is this is the proper fix because now the embed doesn't unmount when closed thus it doesn't refetch when closed and reopened.

e.g.

  1. user 1 opens modal
  2. user 1 selects date and time
  3. user 1 closes modal
  4. user 2 books the the same date
  5. user 1 opens modal again after a while
  6. modal doesn't fetch available dates
  7. user 1 tries to book the same date again
  8. user 1 gets "Could not book the meeting" error

i believe the correct approach is to unmount the component when closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working community Created by Linear-GitHub Sync embed area: embed, widget, react embed ready-for-e2e

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak with cal embed modal box

4 participants