Skip to content

fix: remove the duplicate team-sessions/create registration — Closes #203 - #208

Merged
harsharajkumar-273 merged 1 commit into
harsharajkumar-273:mainfrom
SakethSumanBathini:fix/duplicate-team-route-issue-203
Aug 9, 2026
Merged

fix: remove the duplicate team-sessions/create registration — Closes #203#208
harsharajkumar-273 merged 1 commit into
harsharajkumar-273:mainfrom
SakethSumanBathini:fix/duplicate-team-route-issue-203

Conversation

@SakethSumanBathini

@SakethSumanBathini SakethSumanBathini commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Closes #203

The issue got one thing wrong, and it changes the fix

I wrote that the two implementations were "currently equivalent". They are not, and the difference matters:

system.routes.ts team.controller.ts
body guard req.body || {} req.body
400 message "Valid repository details are required" "...owner, name, and fullName is required"
error handling none try/catch → 500

express.json() leaves req.body undefined when the Content-Type is not JSON. So deleting the system handler as the issue proposed, without porting its guard, does this:

const { repo } = req.body        -> TypeError: Cannot destructure property 'repo' of 'body'
const { repo } = req.body || {}  -> repo = undefined, then a clean 400

A 400 becomes a 500 on a malformed request — a regression introduced by a change whose whole purpose was removing dead code.

What this does

Ports the guard into the controller first, with a comment recording why it is there, so it does not get stripped back out as noise:

// `|| {}` rather than a bare destructure. express.json() leaves req.body
// undefined when the Content-Type is not JSON, and destructuring undefined
// throws a TypeError before the validation below can return a clean 400.
const { repo } = req.body || {};

Then removes the unreachable registration at system.routes.ts:94-116.

The controller keeps its two advantages — the more specific 400 message and the try/catch that turns a store failure into a 500 rather than an unhandled rejection.

Why the controller is the one to keep

team.routes.ts plus team.controller.ts is where the name says the logic lives, and it leaves system.routes.ts for genuinely system-level concerns. team.routes.ts also registers GET /team-sessions/:code, which is not duplicated and does work — so the file was never dead, only half-reachable.

One consequence worth noting

isValidTeamRepo became an unused import in system.routes.ts once the handler went, and is removed. teamSessionStore and normalizeTeamSessionCode are still used there by the GET handler and stay.

Verified

  • POST /team-sessions/create still resolves — now only through team.routes.ts
  • the destructure behaviour above, for both forms
  • npx tsc --noEmit: 1 error, unchanged from main — the pre-existing PrismaClient resolution failure
  • backend/dev.db untouched

Follow-up

The issue suggested checking for other duplicate registrations. I have not done that here — it wants a systematic pass over every router's mount order rather than a grep, and it does not belong in the same change as this fix. Worth its own issue if you want it.

Summary by CodeRabbit

  • Bug Fixes

    • Invalid or non-JSON team-session creation requests now return a clear 400 response instead of causing an error.
  • Changes

    • Removed the authenticated endpoint for creating team sessions.
    • Team-session joining remains available.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 21331a78-fe41-4a7e-8717-84d62f412a53

📥 Commits

Reviewing files that changed from the base of the PR and between d934ce8 and 8ac385f.

📒 Files selected for processing (2)
  • backend/src/controllers/team.controller.ts
  • backend/src/routes/system.routes.ts
💤 Files with no reviewable changes (1)
  • backend/src/routes/system.routes.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: test
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-07-24T17:54:49.155Z
Learnt from: SakethSumanBathini
Repo: harsharajkumar-273/Proofdesk PR: 112
File: backend/src/controllers/import.controller.ts:0-0
Timestamp: 2026-07-24T17:54:49.155Z
Learning: For Express controllers that run long-running work (e.g., PDF imports), handle client disconnects using `res.on('close')` and treat the connection as aborted only when `res.writableFinished === false`. Avoid using `req.on('close')` for this purpose, since it can trigger during normal request-body consumption even when the response has not completed.

Applied to files:

  • backend/src/controllers/team.controller.ts
🔇 Additional comments (1)
backend/src/controllers/team.controller.ts (1)

5-11: LGTM!


📝 Walkthrough

Walkthrough

The team session controller now handles requests without a body. The duplicate authenticated creation route is removed from the system router, leaving team-session joining available.

Changes

Team session route cleanup

Layer / File(s) Summary
Controller request validation
backend/src/controllers/team.controller.ts
createTeamSession defaults an absent request body before extracting repo, allowing validation to return a 400 response.
Duplicate route removal
backend/src/routes/system.routes.ts
The system router no longer registers team-session creation or imports isValidTeamRepo.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the primary change: removing the duplicate team-sessions/create registration.
Linked Issues check ✅ Passed The changes remove the duplicate system route, retain the team route, and preserve controller validation and error responses required by issue #203.
Out of Scope Changes check ✅ Passed All changes directly support issue #203 and the stated objectives; no unrelated code changes are shown.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@harsharajkumar-273 harsharajkumar-273 added ELUSOC Required Tracking VETERAN Advanced (50 pts) labels Aug 9, 2026
@harsharajkumar-273
harsharajkumar-273 merged commit 7825a05 into harsharajkumar-273:main Aug 9, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ELUSOC Required Tracking VETERAN Advanced (50 pts)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] /team-sessions/create is registered twice — team.controller.ts's implementation is dead code

2 participants