Classes are duplicated

[nemvince]

After clearing previous imports, lots of cohorts still remain that are unused. We should debug why this happens, consider how we could clean them up

[coderabbitai]

Coding Plan

Summary

• Fix the core issue by extending the existing delete handler to actually delete cohorts that become orphaned, rather than just nullifying user references
• Provide a one-time cleanup mechanism via an admin endpoint for existing orphaned cohorts already accumulated in the database
• Leverage existing orphan-identification logic already present in the delete handler
• No changes needed to the import flow since the issue stems from deletion behavior, not import behavior

Implementation Steps

Phase 1: Modify Timetable Delete Handler to Remove Orphaned Cohorts

This phase addresses the root cause by ensuring that when a timetable is deleted, any cohorts that become orphaned (no longer linked to any timetable) are also deleted.

Task 1: Delete Orphaned Cohorts in Delete Handler

The delete handler already identifies orphaned cohorts and nullifies user references. Extend this logic to actually delete the cohort rows.

• Update apps/chronos/src/routes/timetable/index.ts in the deleteTimetable handler
• After the existing logic that nullifies user.cohortId for affected users, add a DELETE operation for the identified orphaned cohort IDs
• The deletion should occur within the same transaction that deletes the timetable
• Remove or update the "Cohorts survive" comment to reflect the new behavior
• Ensure the delete happens BEFORE the timetable deletion so the orphan identification query still works (MTM rows exist until timetable is deleted)

Task 2: Update Preview-Delete Route

The preview-delete endpoint shows the impact of deletion before confirming. Update it to reflect that orphaned cohorts will now be deleted.

• Update the preview-delete handler in apps/chronos/src/routes/timetable/index.ts
• Ensure the response communicates that orphaned cohorts will be deleted (not just that users will be unlinked)
• This is informational only and uses the same orphan-identification query

🤖 Prompt for AI agents

Modify the timetable delete handler in
`apps/chronos/src/routes/timetable/index.ts` to actually delete cohort rows that
become orphaned upon timetable deletion, rather than leaving them in the
database.

- In the `deleteTimetable` handler, locate the existing logic that identifies
orphaned cohorts and nullifies `user.cohortId` references
- After nullifying user references (within the same transaction), add a DELETE
operation targeting the identified orphaned cohort IDs
- Ensure this cohort deletion occurs **before** the timetable deletion step, so
the MTM rows still exist when the orphan identification query runs
- Remove or update the existing `"Cohorts survive"` comment to reflect the new
behavior
- In the preview-delete handler (also in
`apps/chronos/src/routes/timetable/index.ts`), update the response to
communicate that orphaned cohorts will be deleted (not just that users will be
unlinked); this handler uses the same orphan-identification query and is
informational only

Phase 2: Clean Up Existing Orphaned Cohorts

Existing orphaned cohorts (from previously deleted timetables) are not linked to any current timetable and won't be caught by the delete handler. This phase provides a mechanism to clean them up.

Task 1: Create Orphan Cleanup Utility Function

Create a reusable function to identify and delete all cohorts that have no MTM links to any timetable.

• Add a new utility function in apps/chronos/src/utils/timetable/ (e.g., in imports.ts or a new cleanup.ts file)
• Query for cohorts where cohort.id does NOT exist in cohort_timetable_mtm.cohortId
• For each orphan: nullify user.cohortId references, then delete the cohort
• Return summary of deleted cohorts for logging/response purposes

Task 2: Expose Admin Cleanup Endpoint

Expose the cleanup function as an admin-only endpoint for manual invocation.

• Add a new route (e.g., DELETE /timetables/orphaned-cohorts or POST /timetables/cleanup-cohorts) in apps/chronos/src/routes/timetable/
• Register the route in apps/chronos/src/routes/timetable/_router.ts
• Protect with admin authentication (follow existing admin route patterns)
• Return the count/list of deleted cohorts in the response

🤖 Prompt for AI agents

Create a reusable orphan cleanup utility and expose it as an admin-only endpoint
to handle cohorts that were orphaned by previously deleted timetables.

- Add a new utility function in `apps/chronos/src/utils/timetable/` (either in
`imports.ts` or a new `cleanup.ts` file) that:
- Queries for all cohorts where `cohort.id` does NOT exist in
`cohort_timetable_mtm.cohortId`
- For each identified orphan: nullifies `user.cohortId` references, then deletes
the cohort row
  - Returns a summary of deleted cohorts for logging/response purposes
- Add a new route (e.g., `DELETE /timetables/orphaned-cohorts` or `POST
/timetables/cleanup-cohorts`) in `apps/chronos/src/routes/timetable/` that calls
this utility function
- Register the new route in `apps/chronos/src/routes/timetable/_router.ts`
- Protect the endpoint with admin authentication, following existing admin route
patterns in the codebase
- Return the count/list of deleted cohorts in the response

Research

The backend (apps/chronos) uses Drizzle ORM with PostgreSQL. Cohorts are linked to timetables via a many-to-many join table (cohort_timetable_mtm). When a timetable is deleted, the MTM rows cascade-delete but cohort rows survive due to onDelete: 'set null' on the direct FK. The delete handler in apps/chronos/src/routes/timetable/index.ts already identifies cohorts that will become orphaned and handles user notifications, but deliberately does not delete the cohort rows themselves (code comment: "Cohorts survive"). The user and group tables have cohortId FKs with onDelete: 'set null', so deleting cohorts is safe from a referential integrity standpoint.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Modify the timetable delete handler in
`apps/chronos/src/routes/timetable/index.ts` to actually delete cohort rows that
become orphaned upon timetable deletion, rather than leaving them in the
database.

- In the `deleteTimetable` handler, locate the existing logic that identifies
orphaned cohorts and nullifies `user.cohortId` references
- After nullifying user references (within the same transaction), add a DELETE
operation targeting the identified orphaned cohort IDs
- Ensure this cohort deletion occurs **before** the timetable deletion step, so
the MTM rows still exist when the orphan identification query runs
- Remove or update the existing `"Cohorts survive"` comment to reflect the new
behavior
- In the preview-delete handler (also in
`apps/chronos/src/routes/timetable/index.ts`), update the response to
communicate that orphaned cohorts will be deleted (not just that users will be
unlinked); this handler uses the same orphan-identification query and is
informational only
===============================================================================

Task: 2

Create a reusable orphan cleanup utility and expose it as an admin-only endpoint
to handle cohorts that were orphaned by previously deleted timetables.

- Add a new utility function in `apps/chronos/src/utils/timetable/` (either in
`imports.ts` or a new `cleanup.ts` file) that:
- Queries for all cohorts where `cohort.id` does NOT exist in
`cohort_timetable_mtm.cohortId`
- For each identified orphan: nullifies `user.cohortId` references, then deletes
the cohort row
  - Returns a summary of deleted cohorts for logging/response purposes
- Add a new route (e.g., `DELETE /timetables/orphaned-cohorts` or `POST
/timetables/cleanup-cohorts`) in `apps/chronos/src/routes/timetable/` that calls
this utility function
- Register the new route in `apps/chronos/src/routes/timetable/_router.ts`
- Protect the endpoint with admin authentication, following existing admin route
patterns in the codebase
- Return the count/list of deleted cohorts in the response

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[nemvince]

@coderabbitai we should try to move towards having a single set of cohorts used for all timetables, completely decoupling them. How feasible is this?

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find schema/model definitions for cohort, timetable, and MTM
fd -e ts -e js -i 'schema' | head -20
echo "---"
rg -n 'cohort' --type ts -l | head -30

Length of output: 1814

---

🏁 Script executed:

#!/bin/bash
# Look at cohort schema definition
rg -n 'cohort' --type ts -C 3 -l | xargs grep -l 'pgTable\|defineTable\|model\|schema' 2>/dev/null | head -10

Length of output: 676

---

🏁 Script executed:

#!/bin/bash
# Find the DB schema files
fd -e ts 'schema' --full-path | head -20
echo "==="
fd -e ts 'cohort' | head -20

Length of output: 657

---

🏁 Script executed:

#!/bin/bash
cat apps/chronos/src/database/schema/timetable.ts

Length of output: 6845

---

🏁 Script executed:

#!/bin/bash
cat apps/chronos/src/routes/timetable/cohort.ts

Length of output: 2394

---

🏁 Script executed:

#!/bin/bash
cat apps/chronos/src/utils/timetable/imports.ts

Length of output: 28026

---

🏁 Script executed:

#!/bin/bash
cat apps/chronos/src/routes/timetable/index.ts

Length of output: 15208

---

---

@nemvince Great direction — and good news: the codebase is already ~80% of the way there. Here's the full analysis:

---

Feasibility Assessment: Global Cohort Pool

What the codebase already does correctly

The import logic in upsertCohort (apps/chronos/src/utils/timetable/imports.ts) already matches cohorts by name and reuses them across timetables:

const [existing] = await tx
  .select()
  .from(cohortSchema)
  .where(eq(cohortSchema.name, attrs.name))  // ← already global by name
  .limit(1);

if (existing) {
  cohortId = existing.id;
} else {
  // insert new cohort
}

await tx.insert(cohortTimetableMtm).values({ cohortId, timetableId }).onConflictDoNothing();

So cohorts are already semantically shared — the same "9A" cohort row is reused when a new timetable is imported. The MTM row is just metadata saying "this cohort appears in this timetable".

user.cohortId is also already a direct FK to cohort, not scoped to any timetable, meaning user assignments survive timetable changes naturally.

---

What needs to change

graph LR
    subgraph Current ["Current (partially coupled)"]
        C1[cohort\n+ timetableId FK ❌] --> MTM[cohort_timetable_mtm]
        MTM --> T[timetable]
    end
    subgraph Target ["Target (fully decoupled)"]
        C2[cohort\nglobal, no timetableId] -.->|optional metadata| MTM2[cohort_timetable_mtm]
        L[lesson\n+ timetableId] --> C2
    end

Loading

Change	Effort	Notes
Drop cohort.timetableId column	Low	It's vestigial — the MTM table is what's actually queried everywhere. Needs a DB migration.
Decide fate of cohort_timetable_mtm	Design decision	See below
Simplify deleteTimetable handler	Low	The entire orphan-detection block goes away — cohorts are never deleted with a timetable
getCohortsForTimetable endpoint	Low	Either return all cohorts or keep MTM-filtered endpoint as a convenience
cohortGroup table	No change needed	Groups already have their own timetableId with onDelete: 'cascade', so they're rebuilt per import

---

The key design decision: keep or drop cohort_timetable_mtm?

Option A — Keep MTM as metadata:

• Cohorts are global, but the MTM table records which timetables a cohort has appeared in
• getCohortsForTimetable still works as a filtered view (useful for admin UI)
• Minimal migration complexity

Option B — Drop MTM entirely:

• Cohorts are fully decoupled — no timetable association at all
• Simpler schema, but you lose the ability to say "show me which classes exist in timetable X"
• The lesson→cohort link (lesson_cohort_mtm) already handles "which cohorts have lessons in timetable X" indirectly, so this may be sufficient

Option A is the lower-risk path and most compatible with the current admin UI.

---

Effect on the original issue (#200)

This architectural change eliminates the issue entirely — there are no orphaned cohorts to clean up if cohorts are never owned by a timetable. The Phase 2 cleanup endpoint becomes unnecessary, and Phase 1 simplifies to just removing the orphan-detection logic from the delete handler.

---

Rough implementation order

1. DB migration: drop cohort.timetableId column
2. Simplify deleteTimetable — remove orphan block
3. Update upsertCohort — remove timetableId from insert (already not setting it in most paths)
4. Optionally update getCohortsForTimetable to return all cohorts (or keep MTM join as a filter)
5. Update the deleteTimetableResponseSchema description (remove "Cohorts survive" as a special note — it's now just the default behaviour)

Want me to replan the implementation around this approach instead?