-
Notifications
You must be signed in to change notification settings - Fork 56
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
Changes to integrate rate-limit headers for Commands left
component UI
#67
Changes from 2 commits
268adb2
cae29cd
bb205a8
38582df
d146475
4463636
fdfa9ac
d5638fd
e2411a7
2764ad7
10fe0a2
811e758
6eb234d
dc2c628
75a60d1
8fce1d1
947bf44
63a58f9
dbdc650
cfb6843
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,19 +41,15 @@ export const WebService = { | |
try { | ||
const response = await fetch(`${PLAYGROUND_MONO_URL}${url}`, options); | ||
|
||
// If the response is not OK, check if the response contains error information | ||
if (!response.ok) { | ||
const errorResponse = await response.json(); | ||
if (errorResponse?.error) { | ||
throw errorResponse.error; | ||
} else { | ||
throw new Error(`HTTP error! Status: ${response.status}`); | ||
} | ||
} | ||
const headers = {}; | ||
response.headers.forEach((value, key) => { | ||
headers[key] = value; | ||
}); | ||
headers['x-ratelimit-remaining'] = 101; | ||
|
||
// Parse the result as JSON | ||
const result = await response.json(); | ||
return result; | ||
const body = await response.json(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return { headers: headers, body: body }; | ||
} catch (error) { | ||
if (error instanceof Error) { | ||
console.error(`Error with ${method} request: ${error.message}`); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,9 @@ | |
import { executeShellCommandOnServer } from '@/lib/api'; | ||
import { CommandHandler } from '@/types'; | ||
|
||
export const handleCommand = async ({ command, setOutput }: CommandHandler) => { | ||
export const handleCommand = async ({ command, setOutput, onCommandExecuted }: CommandHandler) => { | ||
const newOutput = `dice > ${command}`; | ||
let result: string; | ||
let result: any; | ||
|
||
const [cmd, ...args] = command.split(' '); | ||
|
||
|
@@ -14,7 +14,14 @@ | |
} | ||
try { | ||
result = await executeShellCommandOnServer(cmd, args); | ||
setOutput((prevOutput) => [...prevOutput, newOutput, result]); | ||
if (result?.body?.data) { | ||
setOutput((prevOutput) => [...prevOutput, newOutput, result?.body?.data]); | ||
} else if (result?.body?.error) { | ||
setOutput((prevOutput) => [...prevOutput, newOutput, result?.body?.error]); | ||
} | ||
const commandsLeft = result?.headers?.['x-ratelimit-remaining']; | ||
const cleanupTimeLeft = 10; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I have to fix this. waiting on this issue DiceDB/playground-mono#32 |
||
onCommandExecuted(commandsLeft, cleanupTimeLeft); | ||
} catch (error: unknown) { | ||
console.error('Error executing command:', error); | ||
result = 'Error executing command'; | ||
|
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.
Why're we hardcoding this number?
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.
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.
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've removed this change and done this corresponding change in playground-mono: https://github.com/DiceDB/playground-mono/pull/41/files
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.
@lucifercr07