a2-nickborrello-nick-borrello #58
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements an expense tracker application as part of Assignment 2. The application transforms a basic Node.js starter template into a functional expense management system with create, read, and delete operations.
Key Changes:
- Refactored server to handle expense tracking with
/create,/update, and/deleteendpoints - Built a single-page application (SPA) with dynamic table rendering and inline editing
- Added server-side calculation of "days remaining" derived field based on expense due dates
- Updated styling with flexbox layout and custom color scheme
Reviewed changes
Copilot reviewed 6 out of 22 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| server.improved.js | Implements REST API endpoints for expense CRUD operations with derived field calculation |
| public/js/main.js | Frontend logic for expense management, table rendering, and server communication |
| public/index.html | HTML structure with form inputs and expense table |
| public/css/main.css | Styling with flexbox layout and custom theme |
| package.json | Updated metadata and mime dependency version |
| README.md | Project documentation replacing assignment instructions |
| node_modules/ | Added mime 2.6.0 dependency files |
Comments suppressed due to low confidence (1)
public/js/main.js:99
- Unused variable photo.
photo = document.querySelector("#expenseDate");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| appdata.push(dataString); | ||
| console.log("Added", appdata); | ||
| response.writeHead(200, "OK", { "Content-Type": "text/plain" }); | ||
| response.end("New contact posted"); |
There was a problem hiding this comment.
The error message says "New contact posted" but this is an expense tracker application, not a contact manager. The message should say "New expense posted" or similar.
| response.end("New contact posted"); | |
| response.end("New expense posted"); |
| event.preventDefault() | ||
| const addToTable = function (expense) { | ||
| let expenseList = document.querySelector("tbody"); | ||
| let data = JSON.parse(expense); |
There was a problem hiding this comment.
The function addToTable is parsing JSON that was already stringified by JSON.stringify() in the server. This double serialization is inefficient. Consider storing the parsed object directly in appdata instead of stringifying it, or adjust the client-side code to handle the data structure appropriately.
| let data = JSON.parse(expense); | |
| let data = expense; |
| row.appendChild(cell); | ||
| } | ||
| const cell = document.createElement("td"); | ||
| cell.innerHTML='<input type="button" value="Delete" onclick="deleteRow(this)"/>'; |
There was a problem hiding this comment.
The deleteRow function is exposed globally via onclick="deleteRow(this)" in the HTML. This is not ideal for security and maintainability. Consider using event delegation or addEventListener instead of inline onclick handlers.
| cell.innerHTML='<input type="button" value="Delete" onclick="deleteRow(this)"/>'; | |
| const deleteBtn = document.createElement("input"); | |
| deleteBtn.type = "button"; | |
| deleteBtn.value = "Delete"; | |
| deleteBtn.addEventListener("click", function() { | |
| deleteRow(deleteBtn); | |
| }); | |
| cell.appendChild(deleteBtn); |
| expenseName: name.value, | ||
| expenseNumber: number.value, | ||
| expenseDate: dueDate.value, | ||
| }, |
There was a problem hiding this comment.
Missing semicolon after the closing parenthesis of the request.on event handler.
| let dataIndex = 0; | ||
| request.on("data", function (data) { | ||
| dataIndex = data; | ||
| }) |
There was a problem hiding this comment.
Missing semicolon after the closing brace of the request.on('data') event handler.
| }) | |
| }); |
|
|
||
| th, tr, ts { | ||
| text-align: center; | ||
| border: |
There was a problem hiding this comment.
Incomplete CSS rule - the border property on line 40 has no value specified. This will cause a CSS syntax error.
| border: | |
| border: 1px solid #e1e1e1; |
| number = document.querySelector("#expenseNumber"), | ||
| photo = document.querySelector("#expenseDate"); |
There was a problem hiding this comment.
The variable photo is declared but never used. It appears this should be named dueDate instead of photo based on the context (line 99 shows #expenseDate is the due date input).
| number = document.querySelector("#expenseNumber"), | |
| photo = document.querySelector("#expenseDate"); | |
| number = document.querySelector("#expenseNumber"); |
| document.getElementById('expenseList').deleteRow(i-1); | ||
|
|
||
| // Delete the row from the server | ||
| const reponse = await fetch("/delete", { |
There was a problem hiding this comment.
Unused variable reponse.
| const reponse = await fetch("/delete", { | |
| await fetch("/delete", { |
| const name = document.querySelector("#expenseName"), | ||
| number = document.querySelector("#expenseNumber"), | ||
| photo = document.querySelector("#expenseDate"); |
There was a problem hiding this comment.
Unused variable name.
| const name = document.querySelector("#expenseName"), | |
| number = document.querySelector("#expenseNumber"), | |
| photo = document.querySelector("#expenseDate"); |
| } No newline at end of file | ||
| window.onload = function () { | ||
| const name = document.querySelector("#expenseName"), | ||
| number = document.querySelector("#expenseNumber"), |
There was a problem hiding this comment.
Unused variable number.
| number = document.querySelector("#expenseNumber"), |
1 participant
No description provided.