Feedback changes from PR 40 and 41

[joeljoby02]

Implemented the feedback from pull request #40 and #41 which included code refactoring and rewriting tests to match the update to the codebase. Created a new branch for the changes since the pull requests were already merged.

[thehabes]

This looks good and can move forward. I approve, but please consider addressing the major issue found in the static review. It is up to you whether or not to address the minor issue.

Static Review Comments

Branch: feedback_40-41
Review Date: 2026-03-09
Reviewer: Pair Static Review - Claude & @thehabes

Claude and Bryan make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.

Category	Issues Found
🔴 Critical	0
🟠 Major	1
🟡 Minor	1
🔵 Suggestions	0

Critical Issues 🔴

None.

Major Issues 🟠

Issue 1: Missing break statements in new switch/case (index.js)

File: index.js:56-63
Category: Logic Error / Consistency

Problem:
The new server.on('error', ...) handler in start() uses a switch statement without break after each case. The EACCES case falls through into EADDRINUSE. While process.exit(1) terminates the process before the fall-through can execute, this is the same class of bug that this PR fixes in rest.js. Applying the fix in one file but introducing the same pattern in another is inconsistent and fragile — if process.exit(1) is ever replaced with logging or a recoverable action, the fall-through will silently manifest as a bug.

Current Code:

switch (error.code) {
  case 'EACCES':
    console.error(`Port ${p} requires elevated privileges`)
    process.exit(1)
  case 'EADDRINUSE':
    console.error(`Port ${p} is already in use`)
    process.exit(1)
  default:
    throw error
}

Suggested Fix:

switch (error.code) {
  case 'EACCES':
    console.error(`Port ${p} requires elevated privileges`)
    process.exit(1)
    break
  case 'EADDRINUSE':
    console.error(`Port ${p} is already in use`)
    process.exit(1)
    break
  default:
    throw error
}

How to Verify:
Static analysis only. The break after process.exit() is unreachable but serves as a defensive guard and keeps the pattern consistent with the rest.js fix in the same PR.

---

Minor Issues 🟡

Issue 1: Residual comments explaining removed dotenv calls

Files: routes/__tests__/patch.test.js:2, routes/__tests__/set.test.js:2, routes/__tests__/unset.test.js:2
Category: Code Hygiene

Problem:
Three test files retain the comment // dotenv.config() is no longer needed; config module handles environment loading where the dotenv import/call was removed. These comments explain code that no longer exists and will be meaningless to future contributors who never saw the old version. The update.test.js file correctly removed the lines without leaving a comment — follow that approach in the others.

Current Code (example from patch.test.js):

import { jest } from "@jest/globals"
// dotenv.config() is no longer needed; config module handles environment loading
// Only real way to test an express route is to mount it and call it so that we can use the req, res, next.

Suggested Fix:

import { jest } from "@jest/globals"
// Only real way to test an express route is to mount it and call it so that we can use the req, res, next.

Apply the same removal in set.test.js and unset.test.js.

---

Suggestions 🔵

None.

If there are significant code changes in response to this review please test those changes. Run the application manually and test or perform internal application tests when applicable.