Changes to integrate rate-limit headers for `Commands left` component UI #67

aasifkhan7 · 2024-10-10T04:18:18Z

No description provided.

…me is returned as response header

aasifkhan7 · 2024-10-11T17:14:07Z

Hi @lucifercr07 , could you please review the PR?
Summary of changes:

Integrated the x-ratelimit-remaining header to update the "Commands Left" value.
Test changes are pending.
Still investigating where to fetch the "Cleanup Time Left" value from Mono, so that's in progress.

lucifercr07

Please add tests.

lucifercr07 · 2024-10-12T09:25:47Z

apps/playground-web/services/webServices.ts

+      response.headers.forEach((value, key) => {
+          headers[key] = value;
+      });
+      headers['x-ratelimit-remaining'] = 101; 


Why're we hardcoding this number?

This was just a temporary change, I'll remove it. It looks like chrome blocks ceratin response headers to be read by the react app, so I had to hardcode this to test it out.

I've removed this change and done this corresponding change in playground-mono: https://github.com/DiceDB/playground-mono/pull/41/files

@lucifercr07

lucifercr07 · 2024-10-12T09:26:41Z

apps/playground-web/shared/utils/shellUtils.ts

+      setOutput((prevOutput) => [...prevOutput, newOutput, result?.body?.error]);
+    } 
+    const commandsLeft = result?.headers?.['x-ratelimit-remaining'];
+    const cleanupTimeLeft = 10;


Same as above

yes, I have to fix this. waiting on this issue DiceDB/playground-mono#32

aasifkhan7 · 2024-10-14T21:00:41Z

added test cases as well @lucifercr07

lucifercr07 · 2024-10-16T17:03:54Z

@aasifkhan7 please resolve build issues.

aasifkhan7 · 2024-10-16T17:54:30Z

resolved formatting issues @lucifercr07

lucifercr07

@aasifkhan7 shall we just keep changes for commandsLeft as part of this PR as discussed earlier? Since the PR for cleanup time left is still pending.

aasifkhan7 · 2024-10-16T18:05:29Z

Okay, let me do that @lucifercr07

aasifkhan7 · 2024-10-16T18:14:46Z

Done @lucifercr07

KaviiSuri

In general looks fine, but i'm a bit worried about introducing a lot of anys in the code, please avoid it and use zod to validate the payload immedately after we get the response.

I'm also not sure why we have both CLI and Shell, we should just have Shell. CLI is not used anymore and is the same thing.

KaviiSuri · 2024-10-16T18:14:37Z

apps/playground-web/services/webServices.ts


      // Parse the result as JSON
-      const result = await response.json();
-      return result;
+      const body = await response.json();


can we have a type defined here? preferrably using zod schema. Idea is to ensure and catch early if there is an issue with the response.

right now, we just have a lot of any which should be avoided

KaviiSuri · 2024-10-16T18:15:36Z

apps/playground-web/shared/utils/commonUtils.ts

+  result: any,
+  newOutput: string,
+  setOutput: any,
+  onCommandExecuted: any,


can we avoid the any here? very easy to make mistakes on this. Let me know if you need any help with the actual types

KaviiSuri · 2024-10-16T18:16:23Z

apps/playground-web/shared/utils/commonUtils.ts

+export const handleResult = (
+  result: any,
+  newOutput: string,
+  setOutput: any,
+  onCommandExecuted: any,
+) => {


when we have more than 3 arguments, let's convert it into an object, so that we can have named arguments. so instead of hanleResult(result, newOutput, setOutput, onComandExecuted) => hanleResult({ result, newOutput, setOutput, onComandExecuted })

aasifkhan7 · 2024-10-16T19:55:23Z

Done all the changes except removing the CLI @KaviiSuri . Please let me know if that also needs to be shipped with this PR as well.

aasifkhan7 · 2024-10-17T01:51:23Z

resolved build failures @KaviiSuri @lucifercr07 , please review now

lucifercr07 · 2024-10-17T06:53:28Z

@aasifkhan7 can you please fix the failing tests.

aasifkhan7 · 2024-10-17T08:34:59Z

@lucifercr07 fixed the test.

lucifercr07

LGTM

KaviiSuri · 2024-10-17T09:16:23Z

@aasifkhan7 could you also remove the CLI? looks good otherwise, will merge post that

aasifkhan7 · 2024-10-17T09:39:17Z

removed cli @KaviiSuri

…_commandsleft

…setup-ui-packages-and-tailwind-for-console * 'master' of github.com:DiceDB/playground-web: Fix docker compose file (DiceDB#77) integrate cleanup time from playground-mono (DiceDB#76) Changes to integrate rate-limit headers for `Commands left` component UI (DiceDB#67) Adding support for JSON.SET command (DiceDB#74) Unit tests for playground-web repository (DiceDB#71) fix: docker compose contexts to ensure docker compose runs

aasifkhan7 mentioned this pull request Oct 10, 2024

Integrate Ratelimiter Headers and Cron Cleanup Timer with Playground UI #66

Closed

1. commands left to be fixed dynamically. 2. assuming that cleanup ti…

268adb2

…me is returned as response header

aasifkhan7 force-pushed the integrate_ratelimit_commandsleft branch from bc60e1a to 268adb2 Compare October 11, 2024 17:06

fixed error handling in case of wrong commands

cae29cd

lucifercr07 requested changes Oct 12, 2024

View reviewed changes

Aasif Khan added 2 commits October 15, 2024 00:18

refactor: added the handleResult method in commonUtils.ts

bb205a8

remove the hardcoded headers

38582df

aasifkhan7 mentioned this pull request Oct 14, 2024

Integrate ratelimit commandsleft DiceDB/playground-mono#41

Merged

Aasif Khan added 2 commits October 15, 2024 02:26

added test case for rate-limit

d146475

remove unnecessary comment

4463636

fixed comment above test case

fdfa9ac

lucifercr07 changed the title ~~first draft of changes to integrate rate-limit~~ Changes to integrate rate-limit Oct 16, 2024

fix formatting issues

d5638fd

aasifkhan7 requested a review from lucifercr07 October 16, 2024 17:41

Aasif Khan added 2 commits October 16, 2024 23:14

fix formatting issues in src/lib/__tests__/api.test.ts

e2411a7

backmerge

2764ad7

lucifercr07 requested changes Oct 16, 2024

View reviewed changes

fix build error

10fe0a2

remove changes to integrate cleanup time

811e758

aasifkhan7 requested a review from lucifercr07 October 16, 2024 18:16

KaviiSuri requested changes Oct 16, 2024

View reviewed changes

Aasif Khan added 3 commits October 17, 2024 01:10

added validation of zod

6eb234d

refactor

dc2c628

handle error in every condition

75a60d1

aasifkhan7 requested a review from KaviiSuri October 16, 2024 19:55

Aasif Khan added 2 commits October 17, 2024 01:51

remove redundant nullable from ErrorSchema

8fce1d1

resolve build failures

947bf44

fixed tests

63a58f9

lucifercr07 approved these changes Oct 17, 2024

View reviewed changes

removed CLI

dbdc650

lucifercr07 changed the title ~~Changes to integrate rate-limit~~ Changes to integrate rate-limit headers for Commands left component UI Oct 17, 2024

Merge remote-tracking branch 'dicedb/master' into integrate_ratelimit…

cfb6843

…_commandsleft

lucifercr07 merged commit 02369c1 into DiceDB:master Oct 18, 2024
1 check passed

aasifkhan7 deleted the integrate_ratelimit_commandsleft branch November 26, 2024 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to integrate rate-limit headers for `Commands left` component UI #67

Changes to integrate rate-limit headers for `Commands left` component UI #67

aasifkhan7 commented Oct 10, 2024

aasifkhan7 commented Oct 11, 2024

lucifercr07 left a comment

lucifercr07 Oct 12, 2024

aasifkhan7 Oct 14, 2024

aasifkhan7 Oct 14, 2024

aasifkhan7 Oct 14, 2024

lucifercr07 Oct 12, 2024

aasifkhan7 Oct 14, 2024

aasifkhan7 commented Oct 14, 2024

lucifercr07 commented Oct 16, 2024

aasifkhan7 commented Oct 16, 2024

lucifercr07 left a comment •

edited

Loading

aasifkhan7 commented Oct 16, 2024

aasifkhan7 commented Oct 16, 2024

KaviiSuri left a comment

KaviiSuri Oct 16, 2024

KaviiSuri Oct 16, 2024

KaviiSuri Oct 16, 2024

aasifkhan7 commented Oct 16, 2024

aasifkhan7 commented Oct 17, 2024

lucifercr07 commented Oct 17, 2024

aasifkhan7 commented Oct 17, 2024

lucifercr07 left a comment

KaviiSuri commented Oct 17, 2024

aasifkhan7 commented Oct 17, 2024

Changes to integrate rate-limit headers for Commands left component UI #67

Changes to integrate rate-limit headers for Commands left component UI #67

Conversation

aasifkhan7 commented Oct 10, 2024

aasifkhan7 commented Oct 11, 2024

lucifercr07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aasifkhan7 commented Oct 14, 2024

lucifercr07 commented Oct 16, 2024

aasifkhan7 commented Oct 16, 2024

lucifercr07 left a comment • edited Loading

Choose a reason for hiding this comment

aasifkhan7 commented Oct 16, 2024

aasifkhan7 commented Oct 16, 2024

KaviiSuri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aasifkhan7 commented Oct 16, 2024

aasifkhan7 commented Oct 17, 2024

lucifercr07 commented Oct 17, 2024

aasifkhan7 commented Oct 17, 2024

lucifercr07 left a comment

Choose a reason for hiding this comment

KaviiSuri commented Oct 17, 2024

aasifkhan7 commented Oct 17, 2024

Changes to integrate rate-limit headers for `Commands left` component UI #67

Changes to integrate rate-limit headers for `Commands left` component UI #67

lucifercr07 left a comment •

edited

Loading