Skip to content

Peer review: a few small issues + what's working well - #4

Open
kmpat339 wants to merge 8 commits into
swbehan:mainfrom
kmpat339:peer-review-notes
Open

Peer review: a few small issues + what's working well#4
kmpat339 wants to merge 8 commits into
swbehan:mainfrom
kmpat339:peer-review-notes

Conversation

@kmpat339

Copy link
Copy Markdown

Negatives: Added a series of comments highlighting these issues

  1. Typo + inconsistent message in middleware/auth.js: Line 15's comment says "remebers" instead of "remembers," and the 401 message on line 21 has a trailing space ("Not authenticated ") that doesn't match the one on line 8.

  2. duesTier stored as the string "null" instead of actual null in db/users-db.js (4 places). groupId right above it correctly uses real null — this inconsistency means a strict === null check anywhere would silently fail. routes/events.js line 24 even has a comment (// "null"/undefined → 0) working around it.

  3. Leftover project name in db/config.js / package.json: DB_NAME and the package name are still "group-sync", not ClubSync.

  4. README AI Use Disclosure formatting: -Julian reads like a signature but is placed before his section instead of after — compare to - Sean further down, which correctly follows his section.

  5. Misspelled folder name: desgin/ should be design/.

  6. Missing fallback message in GroupWidget.jsx: on a failed leave-club request, setError falls back to a default message (data.message ?? "Could not leave the club.") but the showToast call right below it doesn't, so a bad/empty JSON response would show a toast with undefined text.

Positives

  1. Clean, centralized role-based access control in middleware/auth.jsrequireRole(minRole) returning a reusable Express middleware via closure is a nice pattern that keeps route files free of repeated permission checks.
  2. checkEligibility in routes/events.js returns a clear, specific reason on failure (e.g. "This event requires the gold tier") instead of a generic error — matches what the README says was a real usability-study finding, and it shows.
  3. Consistent, scoped multi-club data access — routes check req.user.groupId up front so a user never accidentally sees another club's events/members, which is easy to get wrong in a multi-tenant app like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant