-
Notifications
You must be signed in to change notification settings - Fork 1
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
Audit/issues #23
base: main
Are you sure you want to change the base?
Audit/issues #23
Conversation
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.
❌ Changes requested. Reviewed everything up to 76dca74 in 1 minute and 28 seconds
More details
- Looked at
86
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. backend/index.ts:76
- Draft comment:
Typo in console log message: 'recieved' should be 'received'. This typo is present in multiple places. - Reason this comment was not posted:
Confidence changes required:50%
The console log message has a typo in 'recieved'. This typo is present in multiple places and should be corrected for clarity and professionalism.
2. backend/index.ts:77
- Draft comment:
Typo in console log message: 'recieved' should be 'received'. This typo is present in multiple places. - Reason this comment was not posted:
Confidence changes required:50%
The console log message has a typo in 'recieved'. This typo is present in multiple places and should be corrected for clarity and professionalism.
Workflow ID: wflow_y4YaxvfAJW3OuY4A
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
const DEPLOYED_SERVER_URL = 'https://algorithm-visualisations-express-production.up.railway.app' | ||
|
||
async function selectServerUrl() { | ||
const hasNetworkConnection = await axios({ |
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.
The network connectivity check is incorrect. hasNetworkConnection
will always be truthy if the request doesn't throw an error. Consider using a try-catch block to handle errors and determine connectivity.
} | ||
|
||
// It's not clear to me why this promise is getting rejected? | ||
const TEST_URL = selectServerUrl() |
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.
TEST_URL
is assigned a promise instead of the resolved value. Use await selectServerUrl()
to get the resolved value.
const TEST_URL = selectServerUrl() | ||
console.log('server', TEST_URL) | ||
|
||
const SERVER_URL = 'http://localhost:8080' |
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.
SERVER_URL
is hardcoded to 'http://localhost:8080'. Consider using the result from selectServerUrl()
to dynamically set the server URL based on connectivity.
Important
Improves logging and adjusts code comments for selection sort and server URL logic in backend and frontend.
index.ts
for selection sort and DFS routes to include request and response details.express.ts
to check server accessibility.selectionSort.ts
.SelectionSort.tsx
to referenceselectionSort
instead ofbubbleSort
.selectServerUrl
function inexpress.ts
to dynamically choose between deployed and local server URLs based on network connection.This description was created by for 76dca74. It will automatically update as commits are pushed.