fix: Feature flag user cache by sean-brydon · Pull Request #10359 · calcom/cal.diy

sean-brydon · 2023-07-25T08:53:25Z

Adds feature flags to cache after its ran once. We very rarley change this and it isnt needed to be fetched each time

vercel · 2023-07-25T08:53:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
api	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 25, 2023 9:58pm
cal-demo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 25, 2023 9:58pm
dev	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 25, 2023 9:58pm
ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 25, 2023 9:58pm

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)			Jul 25, 2023 9:58pm
qa	⬜️ Ignored (Inspect)			Jul 25, 2023 9:58pm

github-actions · 2023-07-25T08:53:38Z

Thank you for following the naming conventions! 🙏

sean-brydon · 2023-07-25T08:53:58Z

  } = props;
  const utils = trpc.useContext();
-  const mutation = trpc.viewer.features.toggle.useMutation({
+  const mutation = trpc.viewer.admin.toggleFeatureFlag.useMutation({


Moved to admin so we can set the cache control on the createNextAPIHandler to true as a result of isPublic=true

sean-brydon · 2023-07-25T08:54:09Z

-  toggle: authedAdminProcedure
-    .input(z.object({ slug: z.string(), enabled: z.boolean() }))
-    .mutation(({ ctx, input }) => {
-      const { prisma, user } = ctx;
-      const { slug, enabled } = input;
-      return prisma.feature.update({
-        where: { slug },
-        data: { enabled, updatedBy: user.id },
-      });
-    }),


Moved to admin as its a better fit there IMHO

alwaysmeticulous · 2023-07-25T08:59:12Z

🤖 Meticulous spotted visual differences in 26 of 271 screens tested: view and approve differences detected.

_{Last updated for commit 5951658. This comment will update as new commits are pushed.}

github-actions · 2023-07-25T09:02:33Z

📦 Next.js Bundle Analysis for @calcom/web

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

This PR introduced no changes to the JavaScript bundle! 🙌

deploysentinel · 2023-07-25T09:03:20Z

Current Playwright Test Results Summary

✅ 105 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 07/25/2023 09:59:40pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 5951658

Started: 07/25/2023 09:58:02pm UTC

⚠️ Flakes

📄 packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Popup Tests should be able to reschedule Retry 1 • Initial Attempt	1.55% (3) 3 / 193 runs failed over last 7 days	97.93% (189) 189 / 193 runs flaked over last 7 days

View Detailed Build Results

hariombalhara · 2023-07-25T10:20:31Z

          // Revalidation time here should be 1 second, per https://github.com/calcom/cal.com/pull/6823#issuecomment-1423215321
          "slots.getSchedule": `no-cache`, // FIXME
          cityTimezones: `max-age=${ONE_DAY_IN_SECONDS}, stale-while-revalidate`,
+          map: `max-age=${ONE_DAY_IN_SECONDS}, stale-while-revalidate`, // "map" - Feature Flag Map


This approach of mapping cacheRules can give unexpected results as it isn't checking for full path. So, there can be features.map and example.map. This cache header would be applied to both I guess?

This isn't related to the PR, so it's non blocking but should be fixed I guess.

Oh i guess youre right -> We could rename this route to something more specific so this doesnt happen?

Also, this is going to consider the response fresh for a day. We might not be able to disable/enable a feature for our users within a good amount of time which is an important thing IMHO.

Also, I suspect a weird issue with this
We might end up in a scenario like this

Say, a feature F1 was disabled and thus it's cached across all our users' browsers

We disable Feature F1, now, backend code knows immediately that F1 is disabled but frontend won't realize it and it might keep showing F1 in UI and taking action on that feature would be rejected by server(as it knows feature is disabled).

I think the approach to solve the multiple requests for flag might be to configure react-query in such a way that the request goes only once from one pageview. Only, on hard refresh we send a new request.

Above scenario can still happen in here, but user can always refresh the page

Using stale while revalidate will prevent stale data for more than a few seconds

Pushing a fix for the namespace issue

hariombalhara

Left some comments

zomars · 2023-07-25T17:30:37Z

 import { createNextApiHandler } from "@calcom/trpc/server/createNextApiHandler";

-export default createNextApiHandler(featureFlagRouter);
+export default createNextApiHandler(featureFlagRouter, true, "features");


We can now optionally namespace api handlers to distinguish them @sean-brydon @hariombalhara

zomars

Tested and addressed @hariombalhara concerns 🙏🏽

Changes implemented

Co-authored-by: zomars <zomars@me.com>

Feature flag user cache

330982a

zomars added the core area: core, team members only label Jul 25, 2023

sean-brydon commented Jul 25, 2023

View reviewed changes

Comment thread packages/trpc/server/createNextApiHandler.ts Outdated

Update packages/trpc/server/createNextApiHandler.ts

1504715

vercel Bot deployed to Preview – ui July 25, 2023 09:00 View deployment

sean-brydon requested a review from a team July 25, 2023 09:00

keithwillcode previously approved these changes Jul 25, 2023

View reviewed changes

sean-brydon added the performance area: performance, page load, slow, slow endpoints, loading screen, unresponsive label Jul 25, 2023

vercel Bot had a problem deploying to Preview – cal-demo July 25, 2023 09:02 Failure

vercel Bot deployed to Preview – api July 25, 2023 09:03 View deployment

vercel Bot deployed to Preview – dev July 25, 2023 09:04 View deployment

hariombalhara reviewed Jul 25, 2023

View reviewed changes

hariombalhara previously requested changes Jul 25, 2023

View reviewed changes

fix: namespace cached enpoints

a9f2b83

zomars dismissed keithwillcode’s stale review via a9f2b83 July 25, 2023 17:29

zomars reviewed Jul 25, 2023

View reviewed changes

zomars approved these changes Jul 25, 2023

View reviewed changes

zomars requested review from hariombalhara and keithwillcode July 25, 2023 17:31

zomars enabled auto-merge (squash) July 25, 2023 17:31

vercel Bot deployed to Preview – ui July 25, 2023 17:33 View deployment

vercel Bot deployed to Preview – api July 25, 2023 17:34 View deployment

vercel Bot deployed to Preview – dev July 25, 2023 17:34 View deployment

vercel Bot deployed to Preview – cal-demo July 25, 2023 17:35 View deployment

keithwillcode approved these changes Jul 25, 2023

View reviewed changes

Merge branch 'main' into fix/feature-flag-cache

8f914df

vercel Bot deployed to Preview – ui July 25, 2023 17:48 View deployment

vercel Bot deployed to Preview – api July 25, 2023 17:50 View deployment

vercel Bot deployed to Preview – dev July 25, 2023 17:50 View deployment

vercel Bot deployed to Preview – cal-demo July 25, 2023 17:53 View deployment

Merge branch 'main' into fix/feature-flag-cache

5951658

vercel Bot deployed to Preview – ui July 25, 2023 21:54 View deployment

vercel Bot deployed to Preview – cal-demo July 25, 2023 21:56 View deployment

vercel Bot deployed to Preview – api July 25, 2023 21:56 View deployment

vercel Bot deployed to Preview – dev July 25, 2023 21:58 View deployment

zomars merged commit ad3bdbb into main Jul 25, 2023

zomars deleted the fix/feature-flag-cache branch July 25, 2023 22:00

joeauyeung pushed a commit that referenced this pull request Jul 26, 2023

fix: Feature flag user cache (#10359)

ad31716

Co-authored-by: zomars <zomars@me.com>

sean-brydon added a commit that referenced this pull request Jul 31, 2023

fix: Feature flag user cache (#10359)

39f61d1

Co-authored-by: zomars <zomars@me.com>

Conversation

sean-brydon commented Jul 25, 2023

Uh oh!

vercel Bot commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alwaysmeticulous Bot commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

deploysentinel Bot commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hariombalhara Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hariombalhara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zomars left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vercel Bot commented Jul 25, 2023 •

edited

Loading

github-actions Bot commented Jul 25, 2023 •

edited

Loading

alwaysmeticulous Bot commented Jul 25, 2023 •

edited

Loading

github-actions Bot commented Jul 25, 2023 •

edited

Loading

deploysentinel Bot commented Jul 25, 2023 •

edited

Loading

hariombalhara Jul 25, 2023 •

edited

Loading