review: Add code review comments for TogglTrack Clone

client/src/App.css

@@ -4,3 +4,4 @@
 } */
 
 
+/* Don't leave empty files in the codebase */

client/src/App.test.js

@@ -1,3 +1,5 @@
+// This file should be in a separate tests directory.
+
 import { render, screen } from '@testing-library/react';
 import App from './App';
 

client/src/Components/Homepage/Footer.jsx

@@ -61,6 +61,7 @@ export default function Footer() {
         <Box>  
             <img  style={{width:"15%"}} src="https://i.postimg.cc/1RDLznkj/tt.png" alt="" /></Box>
         <SimpleGrid columns={{ base: 1, sm: 2, md:3,lg:6 }} spacing={8} >
+          {/* The repetition of these tag blocks below could be avoided and replaced with loops. */}
           <Stack align={'flex-start'} >
             <ListHeader>TOGGL GLOBAL</ListHeader>
             <Link  href={'#'} fontWeight={"normal"} textDecor={"none"}>Blog</Link>
@@ -112,7 +113,8 @@ export default function Footer() {
         </SimpleGrid>
       </Container>
       <Stack direction={'row'} marginBottom={"30px"} spacing={6} justifyContent={"center"}>
-            <SocialButton label={'Twitter'} href={'#'}>
+          {/* The repetition of tags below could be avoided and replaced with a loop. */}
+          <SocialButton label={'Twitter'} href={'#'}>
               <FaTwitter />
             </SocialButton>
             <SocialButton label={'facebook'} href={'#'}>

client/src/Components/Homepage/FooterSignup.jsx

@@ -1,3 +1,5 @@
+// Remove all of these commented lines.
+
 // import { EmailIcon } from '@chakra-ui/icons';
 // import {
 //   Flex,

client/src/Components/Homepage/secondHome.module.css

@@ -1,3 +1,6 @@
+/* Move the jsx and css files for the same component into its own folder
+like done for some other components. Follow one convention. */
+
 /* second div style start from here */
 .fixtop{
     width: 100%;

client/src/Components/Loging/Login.jsx

@@ -58,7 +58,9 @@ export default function Login() {
         .then((res) => res.json())
         .then((res) => {
           console.log(res)
-
+          //  Never string match the error message. Later if the backend changes the error text
+          //  this check will fail and the code will behave unexpectedly. Instead the status code
+          //  or some status enum should be checked for here.
             if(res.message === "required fields are email,password")
          {
           toast({
@@ -113,15 +115,18 @@ export default function Login() {
 
         })
 
-
+        // This is poor error handling. Logging the error isn't enough.
         .catch((err) => console.log(err))
     }
 
   return (
 
 <>
+
+{/* Please format code. It is all over the place. */}
 <Navbar/>
 <Box w={"full"}>
+  {/* Such urls should be put in some constants file */}
   <Box backgroundImage="url('https://public-assets.toggl.com/b/static/a848ad9070fcf959a459fa1e878d2abb/c0583/hero-laptops.jpg')"
   backgroundPosition="center"
   backgroundRepeat="no-repeat" bgSize="full"  h={'400px'} margin="auto"  zIndex={"-1"}>

client/src/Components/NavComponents/responsive Nav/navElements.jsx

@@ -1,3 +1,6 @@
+// Why does this file have a jsx extension when there is no jsx. This prevents the IDE
+// from properly highlighting the syntax and showing any suggestions.
+
 import styled from "styled-components";
 import { Link } from "react-router-dom";
 import {FaBars} from "react-icons/fa";

client/src/Components/TimerPage/SubNav.js

@@ -15,6 +15,11 @@ import {
 import { format } from "date-fns";
 import Rangetimer from "./Rangetimer";
 
+// A common issue that all the components in the codebase suffer from is that the styling is defined inline
+// instead of separating that in a dedicated scss or css file. Inline css is usually okay when the component
+// is tiny and the css rules are relatively self contained. For such a big component, inline css makes this
+// component practically non-reusable, degrades code readbility and clutters the layout and logic contained
+// in the component with its styling rules which have little role to play in what the component is about.
 const SubNav = () => {
   const[open,setOpen]= useState(false)
   const [date, setDate] = useState([

client/src/Components/TimerPage/api.js

@@ -1,3 +1,6 @@
+// Don't put utility javascript code along with the components. Have a separate
+// utils folder for that.
+
 import axios from 'axios'
 export const postdata = (send) => {
   console.log(send)

client/src/Components/prices/Apps.jsx

@@ -34,6 +34,7 @@ const Apps = () => {
         pb="50px"
       >
         <Box w={{ lg: "25%",md:"60%",base:"80%" }} >
+          {/* Passing styling properties as props is a very strange pattern. Never seen it in well written codebases. */}
           <Image m={{md:"0 0 0 80px",base:"0 0 0 -120px",lg:"70px 0 0 -10px"}} src="https://public-assets.toggl.com/b/static/2038847e62390356691df99de87fece9/368f9/icon-mobile.avif" />
           <Box ml={{ lg: "-60px",md:"-10px",base:"-280px" }} w={{ lg: "70%" }} mt="30px">
             <Text fontWeight={"bold"}>MOBILE APPS</Text>

client/src/Components/prices/Faqs.jsx

@@ -52,6 +52,10 @@ const Faqs = () => {
                   <AccordionIcon />
                 </AccordionButton>
               </h2>
+              {/* Such strings should be declared in one place in a separate class or js file.
+              For example, see - https://github.com/mozilla/addons-frontend/blob/master/src/amo/constants.js
+              This makes managing static text easier. It also becomes easier to add support l10n and i18n
+              so the app can render text for different languages (like rtl languages).               */}
               <AccordionPanel pb={4} fontSize={{ lg: "15.36px" ,base:"14px",md:"15" }}>
                 You will be charged a monthly fee for each member of your team.
                 For paid plans, this fee applies even if you have under 5 active

client/src/Components/prices/PlanComparison.jsx

@@ -109,6 +109,8 @@ const PlanComparison = () => {
                 <Td fontSize={{ lg: "20px" }} p={{ lg: "20px" }}>
                   Time Tracking
                 </Td>
+                {/* What is this for? I'm guessing this is for adding some column space.
+                If so, it is not the right way to do so. Please avoid such hacks. */}
                 <Td></Td>
                 <Td></Td>
                 <Td></Td>

client/src/Components/prices/PricesTop.jsx

@@ -149,6 +149,12 @@ const navigate =useNavigate()
             }}
             textAlign={"left"}
           >
+            {/* The following block could've been written in a much neater fashion by having a
+            containerwhich assembles each Flex > Box > BsCheckLg unit by looping over a prop
+            containingdata elements in an array. This is precisely a perfect example of anti-patterns
+            withReact components. Components are written for being reusable and helping maintain code
+            better. What is the point of using react and its component framework if y'all hardcode
+            the entire page like this? Same applies below as well */}
             <Flex gap={5} mt="9px">
               <Box color={"#E57CD8"} fontSize="20px">
                 <BsCheckLg />

client/src/Components/sample.jsx

@@ -1,3 +1,5 @@
+// What is this for? Please remove such spurious components which serve no purpose.
+
 import React from 'react'
 
 const sample = () => {

client/src/Components/sidebar/Sidebar.jsx

@@ -1,3 +1,53 @@
+// This file is just unreadable.
+// 1. Lots of unused imports
+// 2. Lots of commented code
+// 3. Absolutely 0 comments on what purpose the sidebar serves
+// 4. Tons of CSS mixed along with the component definition and contents
+//
+// Please write smaller and more well defined components which have a specific purpose.
+// Each component should have a few comments about what it does, a sample usage example if possible, etc.
+// A good component has a well defined purpose and usually it should be configurable so it can be used elsewhere too.
+// From the code below, it looks like Sidebar is a "container" which is a component that is composed of multiple smaller
+// independent components. I'm making this distinction because that helps to think about the UI in a different, more
+// compartmentalised way that makes organising, writing, reusing and maintaining code easier.
+//
+// Also, the current way of combining the entire JSX in a single return statement makes understanding the sub-components
+// extremely difficult. A better way to use JSX is to define different variables for different logical elements within
+// the component. For example (might not be strictly syntactically correct) -
+//
+// const Header = (props) => (
+//   <h1 className="heading">
+//     props.message
+//   </h1>
+// );
+//
+// const Body = (props) => (
+//   <div className="content">
+//     props.text
+//   </div>
+// );
+//
+// // ... Similarly Section and Footer are defined.
+//
+// const Page = (props) => {
+//   return (
+//     <div>
+//       <Header message=props.heading />
+//       <Body text=props.content />
+//         <Section>
+//           <OtherComponent />
+//           <AnotherComponent />
+//         </Section>
+//       <Footer />
+//     </div>
+//   );
+// }
+//
+// The example above makes reading and understanding what the component is doing is much easier.
+//
+// A good example of a cleanly written big component is -
+// https://github.com/mozilla/addons-frontend/tree/master/src/amo/components/AddonsCard.
+// It also separates its styling from the component and encapsulates all the points described above.
 import React,{ReactNode, useEffect, useState} from 'react'
 import {
 

client/src/Mainroutes/Navroutes.jsx

@@ -1,5 +1,7 @@
 import React from 'react'
 import { Routes,Route } from 'react-router-dom'
+// Remove all unused imports from all the files please. Importing something adds an overhead
+// while loading the application / component. Avoid this.
 import Footer from '../Components/Homepage/Footer'
 import Homepage from '../Components/Homepage/Homepage'
 import Navbar from "../Components/NavComponents/Navbar"
@@ -30,6 +32,7 @@ const Navroutes = () => {
          <Route path="/YourInfo" element={<YourInfo/>} />
          <Route path="/ToggleBook" element={<ToggleBook/>} />
          <Route path="/Prices" element={<Prices/>} />
+         {/* Have consistency in naming your routes. Follow the same convention. */}
          <Route path="/login" element={<Login/>} />
          <Route path='/signup' element={<Signupm/>}/>
          {/* <Route path="/timers" element={<Sidebarroutes/>} /> */}

server/backend server/config.js

@@ -1,6 +1,18 @@
 const mongoose = require("mongoose");
 require("dotenv").config();
 
+// Typically a database connection pool should be created and used throughout the application.
+// Having only a single connection limits the database operations that can be performed at once.
+// A connection pool is a resource which manages multiple database connections so that as needed
+// the APIs can acquire a connection, use it and return it back to the pool after use. The
+// advantage of a pool is that you can perform multiple independent database operations at once,
+// improving the performance of your application. Databases can support 100s of concurrent connections
+// so using only one connection also leads to wasteage of compute and memory resources of the
+// database server. Having only a single connection also means that in case of concurrent API
+// invocations only one API call can effectively make progress (the one which is using the database
+// connection at the moment). This means that the other API calls will have a much higher wait time
+// and thus higher than expected latency.
+// Mongoose supports connection pools. See - https://mongoosejs.com/docs/connections.html#connection_pools.
 const connection = mongoose.connect(process.env.mongo_url);
 module.exports = {
   connection,

server/backend server/controller/authController.js

@@ -5,16 +5,33 @@ var jwt = require("jsonwebtoken");
 
 const register = (req, res) => {
   if ( !req.body.email || !req.body.password) {
-    return res.send({message:"required fields are email,password"});
+        // Explicitly add status codes for error responses. Using a json object for the response body is
+        // good but not having the correct status code hurts. The statement should be -
+        // return res.status(400).send({message:"required fields are email,password"});
+        // Status code 400 is used for indicating bad requests, where the request was malformed or
+        // had missing parameters. Also a better way to write the same error message is -
+        // "Missing required fields - email, password"
+        return res.send({message:"required fields are email,password"});
   }
   const {  email, password } = req.body;
   bcrypt.hash(password, 8, async (err, hash) => {
     // Store hash in your password DB.
     if (err) {
-      return res.send("error in bcrypt password");
+        // This is not a good way to return error responses. APIs should conform to REST principles.
+        // All error response should be formatted in a certain way so that the client knows what to expect.
+        // A proper json object with error message and any other relevant information must be sent back
+        // with an appropriate status code. For internal errors status code 401 should be used.
+        // Read about status codes here - https://restfulapi.net/http-status-codes/ 
+        // It should be something like -
+        // res.status(500).send({ error: "Encrypting the password failed" });
+        // Also, try to maintain uniformity with responses in the application. Like above and below, both
+        // error responses are json objects but here it is a string.
+        return res.send("error in bcrypt password");
     }
     const user = new UserModel({ email, password: hash });
+    // What if this operation fails? Always handle for failures and add fallback mechanisms.
     await user.save();
+    // Use status code 201 here.
     return res.send({message:"Register Successfull"});
   });
 };
@@ -28,6 +45,9 @@ const login = async (req, res) => {
   try {
     const find_user = await UserModel.findOne({ email });
     if (!find_user) {
+      // Avoid words like 'Plz'. Also the real error is that the user was not found. So the
+      // error message should be something like - "User with email: ${email} not found".
+      // Should use status code 404. Similarly for all the error cases below.
       return res.send({message:"Plz enter registered details"});
     }
 

server/backend server/controller/taskController.js

@@ -1,7 +1,51 @@
 const TaskModel = require("../models/Task.model");
 
+// Each API should have a head comment which provides a good description about:
+// 1. what the API does
+// 2. what the request looks like
+// 3. what responses can be expected (success and failure cases both included)
+// post all the data 
 const createTask = async (req, res) => {
-  try {
+    // The very first statement in any API function should be to log the incoming request.
+    // In case of any failure downstream, knowing the incoming request helps a lot during
+    // debugging and understanding what could've caused the failure. Logging is extremely helpful
+    // and logging the right things in the right place with the right messages can be the
+    // difference between spending hours in finding the root error causes vs just looking at the
+    // logs and figuring it out in 2 minutes.
+    //
+    // Each API function should follow the following structure -
+    //
+    // const <_http_method_><_resource_> = async (req, res) => {
+    //   console.group('Received request from {req.user.id} to create task, with body: {req.body}.');
+    //   try {
+    //     console.log('Performing _operation_ with params: {_params_}.')
+    //     // perform API specific operations here
+    //     console.log('Successfully performed _operation_.');
+    //     res.status(_status_code_)
+    //   } catch (e) {
+    //     console.error('Could not perform _operation_.', e.message);
+    //     // add some retry logic here.
+    //     if (numRetries > retryLimit) {
+    //        res.status(500).send({ "error": "_relevant_error_message_goes_here_" });
+    //     }
+    //   }
+    //   console.groupEnd();
+    // });
+    // 
+
+    // Deserialisation can be done is a better way. Usually for serialisation and deserialisation
+    // static functions are defined in the model class itself. Something like this for example -
+    //
+    // In models/Task.model.js
+    //
+    // // This function deserialises the input json into a Task object.
+    // static from(task_json, user_json) {
+    //   return Object.assign(new Task(), task_json, user_json);
+    // }
+    //
+    // Its invocation would be something like -
+    // const task = Task.from(req.body, req.user);
+    try {
     const newTask = await TaskModel({
       title: req.body.title,
       id:req.body.id,
@@ -14,19 +58,36 @@ const createTask = async (req, res) => {
     console.log(err);
   }
 };
+
 const getAllTasks = async (req, res) => {
   try {
     const allTasks = await TaskModel.find();
     // if (!allTasks) return res.send({message:"No Task found"});
     return res.send(allTasks);
   } catch (err) {
+    // Errors should be logged at the error level, not the default level.
     console.log(err);
+    // Error message used below is not clear at all. It gives 0 idea about
+    // what happened at the server and whether the client should retry the
+    // request and hope for the correct response the next time. The error
+    // response, should contain a human readable / understandable message
+    // with the proper status code. In this case the status code would be
+    // 500. This statement should be something like -
+    // res.status(500).send({"error": _stringified_error_});
+    // Also, there should be some form of failure handling on the server
+    // end first, based on the error type. There should be some notion of
+    // whether the error was transient and would get fixed on retries.
+    // Error handling in APIs is very crucial for applications working at
+    // a large scale.
     res.send("sth went wrong");
   }
 };
 
 const editTasks = async (req,res) => {
     try{
+      // What is the '+' for? It serves no purpose. Avoid such statements.
+      // Nit: Also req.params.id can be used inline in the 2 places. That
+      // will avoid creating a new variable.
       const id= +req.params.id
       const allTasks = await TaskModel.findOne({id:id})
       const update_user = await TaskModel.findOneAndUpdate({id :id},
@@ -44,6 +105,24 @@ const editTasks = async (req,res) => {
 const deleteTasks = async (req,res) => {
     try{
       const id= +req.params.id
+      // Always log before and after database (or any IO operations for that matter).
+      // Certain IO operations might take long to run so it might also be nice to
+      // read the logs and see how long it takes on average for some operation to
+      // finish. It will give some idea about what the critical path of the API is.
+      // This is helpful in figuring out what operations in the API flow should be
+      // focused on for optimisations.
+      //
+      // IO operations also need to have proper error handling. Having a catch-all
+      // for all kinds of errors is not a good way to do error handling because errors
+      // from different operations are different and should be treated as such. Some
+      // errors would be transient like say transaction timeout error or connection
+      // error but something like duplicate primary key or some other column constraint
+      // error is not. Knowing the distinction between error types greatly helps in
+      // making the API code more resilient to failures and improves the success rate
+      // of the API calls. Large applications with way more complex APIs and a lot more
+      // moving parts cannot tolerate points of failures. They all have some or the other
+      // fallback mechanism. Think about such things while writing APIs. Read more
+      // about how to better handle errors.
       const data=await TaskModel.findOne({id:id})
       const del = await TaskModel.findOneAndDelete({id :id});
       res.send({message:"delted",data:del})