Conversation
shreeyadc
changed the title
Yarik-Popov
left a comment
Yarik-Popov
left a comment
There was a problem hiding this comment.
Overall good. Some minor changes are needed for the backend. I will review the frontend after work today
|
The frontend build is failing |
|
Congrats on finishing the backend portion if the gs onboarding, if you want you can dm me your email so i can add you to the notion |
Yarik-Popov
left a comment
Yarik-Popov
left a comment
There was a problem hiding this comment.
Good step in the right direction. In the command_input.tsx file you need to fetch the MainCommands instead of Commands.
For fixing the build error, just remove all the new files you created and see the other comments in the files that were changes that aren't in the frontend/src/display or frontend/src/input directories. For running the frontend, you need to change into the frontend directory before running npm ci and npm run dev
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1" | ||
| "react-dom": "^18.3.1", | ||
| "react-scripts": "^3.0.1" |
There was a problem hiding this comment.
The reason why the frontend build pipeline is failing due to you add something to the frontend/package.json but not running npm i in that directory
There was a problem hiding this comment.
This file isnt needed. See my comment on the frontend/package.json
There was a problem hiding this comment.
This file isnt needed. See my comment on the frontend/package.json
| await deleteCommand(id); // Call deleteCommand function to delete the command on the backend | ||
| // Remove the deleted command from the local state | ||
| setCommands(prevCommands => prevCommands.filter(command => command.id !== id)); |
There was a problem hiding this comment.
Use the data that was returned instead of filtering out on the frontend
| run: | | ||
| cd frontend | ||
| npm ci | ||
| npm ci --legacy-peer-deps |
There was a problem hiding this comment.
This command is generally not recommended as it can causes issues
| const [commands, setCommands] = useState<CommandResponse[]>([]); | ||
| const [selectedCommand, setSelectedCommand] = useState<string>(""); | ||
| const [params, setParams] = useState<string[]>([]); | ||
| const [error, setError] = useState<string | null>(null); // Error state for feedback |
| useEffect(() => { | ||
| const fetchCommands = async () => { | ||
| try { | ||
| const data = await getCommands(); |
There was a problem hiding this comment.
You should be fetching the main commands here and you don't need to fetch the regular commands in this component. Use the getMainCommands endpoint function
| export const createCommand = async (commandType: string, params: string): Promise<void> => { | ||
| try { | ||
| await axios.post(`${API_URL}/commands/`, { command_type: commandType, params }); | ||
| } catch (error) { | ||
| console.error("Error creating command:", error); | ||
| throw error; | ||
| } | ||
| }; No newline at end of file |
2 participants
Purpose
Completed the GS on-boarding task. Include a screenshot of the front-end of the application.
New Changes
Backend:
Frontend:
Testing
Explain tests that you ran to verify code functionality.
Any functions that can be unit-tested should include a unit test in the PR. Otherwise, explain why it cannot be unit-tested.
Backend:
pytestto check that all backend tests passed (13/13).Frontend:
Outstanding Changes