Skip to content

chore: improve contributor experience#73

Merged
paresh011 merged 3 commits intomainfrom
chore/contributor-experience
Apr 20, 2026
Merged

chore: improve contributor experience#73
paresh011 merged 3 commits intomainfrom
chore/contributor-experience

Conversation

@aruneshwisdm
Copy link
Copy Markdown
Collaborator

@aruneshwisdm aruneshwisdm commented Apr 18, 2026

Summary

Fixes several issues a new contributor would hit when trying to set up and contribute to the project:

  • Fix env setup instructionspnpm dev runs Next.js from apps/web/, so .env.local must live there. Both README and CONTRIBUTING.md now point to apps/web/.env.example → apps/web/.env.local.
  • Add husky pre-commit hookpackage.json configures husky + lint-staged but .husky/pre-commit didn't exist. Contributors would push unlinted code and fail CI. Uses idiomatic Husky v9 single-line lint-staged.
  • Add CI badge to README — No way to see if main is passing at a glance.
  • Add specs reference — CONTRIBUTING.md references specs/PRODUCT_SPEC.md but README didn't mention specs exist. Added pointer in Contributing section.
  • Add docs/README.md index — 25+ docs with no table of contents. Now categorized into Getting Started, Architecture, API Reference, Testing, and Internal Planning sections.
  • Bind Mailpit to localhost — Matched Postgres security posture by binding Mailpit ports to 127.0.0.1.
  • Remove stale Redis references — Redis was intentionally removed (app uses Upstash). Cleaned up leftover references from docker-compose, .env.example, Stack section, and CONTRIBUTING.md prerequisites.

Test plan

  • docker-compose up -d starts Postgres and Mailpit (no Redis)
  • cp apps/web/.env.example apps/web/.env.local && pnpm dev works for manual setup
  • Pre-commit hook runs lint-staged on commit
  • CI badge renders correctly in README
  • docs/README.md links all resolve to existing files

🤖 Generated with Claude Code

- Add Redis service to docker-compose.yml (was listed as required but missing)
- Add REDIS_URL to .env.example
- Add husky pre-commit hook for lint-staged
- Add CI status badge to README
- Fix manual install to use .env.local instead of .env
- Add specs reference to Contributing section in README
- Add docs/README.md as categorized index for documentation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@paresh011 paresh011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Code Review — PR #73

Reviewer: paresh011 | Verdict: Request Changes


CRITICAL (Must Fix Before Merge)

1. Redis was intentionally removed — this PR re-adds it

Commit 150fd1a (March 10, 2026) by the same author explicitly removed Redis from docker-compose with message:

"Redis was unused — sessions use JWT via NextAuth and no caching layer is currently active. Removes redis service, REDIS_URL env vars, and associated volume/healthcheck config."

The app uses Upstash Redis (cloud-based @upstash/redis), configured via UPSTASH_REDIS_REST_URLnot the local REDIS_URL being added here. BullMQ is not a dependency anywhere in the project. The docker-compose comment "for BullMQ job queues" is factually incorrect.

Impact: Adds unnecessary service, increases resource consumption, contradicts a documented architectural decision, and confuses contributors about actual architecture.

Fix: Remove the Redis service, REDIS_URL from .env.example, and redis_data volume. If Redis is genuinely needed in the future, it should be added alongside the actual code that uses it.

2. Docker quickstart is broken — no web service exists

README Docker quickstart says:

docker-compose exec web pnpm db:migrate

But docker-compose.yml has no web service — only postgres and mailpit. This command will fail with service "web" not found. The web service only exists in docker-compose.simple.yml and docker-compose.prod.yml.

Impact: A first-time contributor following the Docker path hits an error immediately. A PR titled "improve contributor experience" should not leave a broken quickstart in place.

Fix: Either add a note that the base docker-compose.yml is for supporting services only (use pnpm dev locally), or reference docker-compose.simple.yml for the full containerized flow.


IMPORTANT (Should Fix)

3. .husky/pre-commit — NVM loading is redundant and incomplete

Husky v9's own wrapper (.husky/_/h) already:

  • Line 11-12: Loads ~/.config/husky/init.sh before running hooks (handles nvm/fnm)
  • Line 16: Adds node_modules/.bin to PATH

The NVM setup in the hook is:

  • Redundant — Husky v9 already handles this via init.sh
  • Incomplete — comment says "nvm/fnm/pnpm" but only handles nvm
  • Not idiomatic — Husky v9 docs recommend keeping hooks minimal

Recommended hook:

pnpm exec lint-staged

4. npx should be pnpm exec

Project enforces pnpm ("packageManager": "pnpm@9.15.0"). Using npx:

  • Is inconsistent with project tooling
  • May fail on systems without npm installed alongside pnpm
  • Bypasses pnpm's resolution strategy

5. Mailpit ports exposed to all interfaces (pre-existing)

# Postgres & Redis: bound to localhost ✅
- '127.0.0.1:5432:5432'
- '127.0.0.1:6379:6379'

# Mailpit: exposed to 0.0.0.0 ❌
- '8025:8025'
- '1025:1025'

Pre-existing issue, but since this PR touches docker-compose.yml, it should fix Mailpit to use 127.0.0.1 binding as well.

6. .env path inconsistency across docs

File Instruction Path Level
README (Docker) cp .env.example .env root
README (Manual, this PR) cp .env.example .env.local root
CONTRIBUTING.md cp apps/web/.env.example apps/web/.env.local apps/web

These are different files with different contents. A contributor following README vs CONTRIBUTING.md will get different env setups. Should be aligned.


SUGGESTIONS (Non-blocking)

  • Redis persistence volume (redis_data:/data) is unnecessary for a dev-only setup — if Redis is kept, ephemeral is fine
  • README formatting changes (list markers, table alignment) mixed with functional changes makes the diff harder to review — consider separate commits for cosmetic vs functional changes
  • docs/README.md — well done, all 25 links verified, descriptions accurate, categorization sensible 👍
  • CI badge — verified, .github/workflows/ci.yml exists

What's Good

  • The docs/README.md index is genuinely valuable — 25 docs with no TOC was a real problem
  • CI badge addition is a good DX improvement
  • .env.local fix for manual install is correct (Next.js convention)
  • Specs reference in README contributing section is helpful
  • The PR description is well-written with a clear test plan

Summary

The intent is good but the execution has issues. The biggest concern is re-adding Redis that was intentionally removed 5 weeks ago without any new code that uses it. I'd suggest splitting this into two PRs:

  1. Docs improvements (docs/README.md, CI badge, specs reference, env.local fix) — ready to merge
  2. Infrastructure changes (Redis, husky hook) — needs rework

- Remove Redis service, REDIS_URL, and redis_data volume (Redis was
  intentionally removed in 150fd1a; app uses Upstash, not local Redis)
- Fix Docker quickstart: clarify docker-compose runs supporting services
  only, not the web app
- Simplify .husky/pre-commit to just `lint-staged` (available via
  node_modules/.bin which Husky v9 adds to PATH)
- Bind Mailpit ports to 127.0.0.1 to match Postgres security posture
- Align .env path instructions: both README and CONTRIBUTING.md now use
  `cp .env.example .env.local` from root
- Remove Redis from Stack section and CONTRIBUTING.md prerequisites

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aruneshwisdm aruneshwisdm requested a review from paresh011 April 20, 2026 09:24
Copy link
Copy Markdown
Collaborator

@paresh011 paresh011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Review: PR #73 (Post-Fix Commit)

Thanks for the quick turnaround on the feedback, Arunesh! Most of the previous findings are resolved nicely. One new issue came up during re-review that's worth addressing before merge.


Previous Findings — All Resolved

# Finding Status
1 Redis re-added to docker-compose Resolved — service, env var, and volume removed
2 Broken Docker quickstart (exec web) Resolved — rewritten with clear "Docker for Services" flow
3 Husky NVM loading redundant Resolved — hook is now a clean single-line lint-staged
4 npx instead of pnpm exec Resolved — bare lint-staged works via Husky v9 PATH setup
5 Mailpit ports exposed to 0.0.0.0 Resolved — now bound to 127.0.0.1
6 .env path inconsistency Partially resolved — see new finding below

Great work on the Husky hook especially — single-line lint-staged is exactly the idiomatic Husky v9 approach.


Critical

.env path change breaks contributor setup

The updated CONTRIBUTING.md now instructs:

cp .env.example .env.local

This copies the root .env.example to root .env.local. However, Next.js never reads this file. Here's why:

  • pnpm dev → runs turbo dev → runs next dev inside apps/web/
  • Next.js loads .env.local from its CWD, which is apps/web/
  • The working env file on disk is apps/web/.env.local (confirmed)
  • Root .env.local does not exist and is not referenced anywhere

The previous CONTRIBUTING.md instruction was correct:

cp apps/web/.env.example apps/web/.env.local

A new contributor following the updated docs would create a root .env.local that Next.js ignores, resulting in missing DATABASE_URL, NEXTAUTH_SECRET, etc. The app would fail to start.

Suggested fix: Revert the env path in both README and CONTRIBUTING.md back to apps/web/.env.example → apps/web/.env.local. The README Quick Start sections should also reflect this path.


Minor

apps/web/.env.example still references Redis

Lines 14–15 still say:

# Redis (required for BullMQ job queue)
REDIS_URL=redis://localhost:6379

Since this PR removes Redis from docker-compose, Stack section, and CONTRIBUTING.md prerequisites, it would be good to clean this up too for consistency. Not a blocker — could be a follow-up if preferred.


Suggestions

PR description could use a refresh

The summary and test plan still reference Redis additions (e.g., "Add Redis to docker-compose.yml", "docker-compose up -d starts Redis alongside Postgres and Mailpit"). Updating these to match the current state of the PR would help future readers.


What Looks Good

  • Docker quickstart is much clearer now with the "Docker for Services" framing
  • Mailpit security posture matches Postgres (localhost-only binding)
  • Husky hook is minimal and correct
  • docs/README.md index remains a solid addition — all 25 links verified
  • CI badge, specs reference, and Redis removal from Stack section are all clean
  • CONTRIBUTING.md formatting improvements are consistent

Looking forward to the env path fix — once that's addressed, this should be good to go.

Next.js runs from apps/web/ via Turborepo, so .env.local must live in
apps/web/, not the repo root. Reverts env instructions in README and
CONTRIBUTING.md to the correct path. Also removes leftover Redis
reference from apps/web/.env.example.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aruneshwisdm aruneshwisdm requested a review from paresh011 April 20, 2026 10:34
Copy link
Copy Markdown
Collaborator

@paresh011 paresh011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final Review: PR #73 (3 Commits)

All 8 findings from previous two review rounds have been addressed. Nice work on the iterative fixes.


Previous Findings — All Resolved

# Finding Round Status
1 Redis re-added to docker-compose R1 Resolved — removed entirely
2 Broken Docker quickstart (exec web) R1 Resolved — rewritten with pnpm dev flow
3 Husky NVM loading redundant R1 Resolved — single line lint-staged
4 npx vs pnpm exec R1 Resolved — bare lint-staged via Husky PATH
5 Mailpit ports exposed to 0.0.0.0 R1 Resolved — 127.0.0.1 binding
6 .env path inconsistency R1 Resolved — both docs use apps/web/ path
7 .env path breaks contributor setup R2 Resolved — reverted to correct apps/web/.env.local
8 apps/web/.env.example stale Redis ref R2 Resolved — Redis lines removed

File-by-File Summary

  • .env.example (root) — Header updated to clarify "used by Docker" and directs developers to apps/web/.env.example. Eliminates confusion between the two env files.
  • .husky/pre-commit — Single line lint-staged. Husky v9 wrapper handles PATH and init.sh loading. Clean and idiomatic.
  • CONTRIBUTING.md — Redis removed from prerequisites and required vars. Env path correctly points to apps/web/.env.local. Formatting improvements throughout.
  • README.md — CI badge added. Docker quickstart rewritten with clear "Docker for Services" framing. Manual section updated. Redis + BullMQ removed from Stack. Specs reference added.
  • apps/web/.env.example — Stale Redis comment and REDIS_URL removed.
  • docker-compose.yml — Mailpit ports bound to 127.0.0.1, matching Postgres security posture. No Redis service.
  • docs/README.md — All 25 documentation links verified. Categorization is accurate and well-organized.

Minor Notes (Non-blocking, for a follow-up)

A few stale Redis references remain outside this PR's scope:

File Line Reference
.env.test.example 16 REDIS_URL="redis://localhost:6379/1"
CLAUDE.md 19 Cache/Queue: Redis + BullMQ in tech stack table
CLAUDE.md 213 REDIS_URL - Redis connection string in env vars

These can be cleaned up in a quick follow-up commit.

Also, the PR description and test plan still reference Redis additions from the original commit — worth updating to reflect the final state for future readers.


Verdict

Approving. The PR delivers meaningful contributor DX improvements: clearer setup instructions, correct env paths, security-hardened port bindings, a well-organized docs index, and a clean Husky hook. Good collaboration across the review rounds.

@paresh011 paresh011 merged commit 32e3278 into main Apr 20, 2026
4 checks passed
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.

2 participants