Server/feat/academic year context#30
Conversation
- Add AcademicYearContextService to manage active academic year retrieval. - Create AcademicYearContextModule for dependency injection. - Update seeders to accommodate new academic year structure. - Refactor imports and adjust class and exam seeders for consistency.
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAcademic-year updates now validate editable fields, integrate academic-year context, seed multiple years, and adjust related class, exam, module, import, and seeder paths. ChangesAcademic year flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/database/seeders/seed-exams.ts (1)
17-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove dead
academicYearfetch and guard check.
academicYearis fetched (lines 17–19) and used in the guard at line 21, but bothacademicYearIdassignments are commented out (lines 37, 47). The guard!academicYearwill skip exam seeding when no current academic year exists — even though theExamschema no longer requires that relationship. Remove the fetch and simplify the guard to only checkteachingAllocations.🧹 Proposed fix to remove dead code
const teachingAllocations = await prismaService.teachingAllocation.findMany(); - const academicYear = await prismaService.academicYear.findFirst({ - where: { isCurrent: true }, - }); - if (teachingAllocations.length === 0 || !academicYear) { + if (teachingAllocations.length === 0) { console.log( - 'No teaching assignments or current academic year found. Skipping exam seeding.', + 'No teaching assignments found. Skipping exam seeding.', ); return; }Also applies to: 37-37, 47-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/database/seeders/seed-exams.ts` around lines 17 - 21, Remove the unused academicYear lookup from the seeder and simplify the guard in the seeding function to return only when teachingAllocations is empty. Keep the commented academicYearId assignments removed or unchanged, and ensure no remaining references to academicYear remain.
🧹 Nitpick comments (1)
server/src/modules/classes/classes.service.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
AcademicYearContextServiceis injected but unused; academic-year logic is commented out.The import path was updated (Line 7) and the service is injected (Line 20), but every usage is commented out (Lines 26–28, 44–46, 58–59, 63). This leaves a dead dependency and large blocks of commented-out code across
createandfindAll.If academic-year scoping is deferred, add a
// TODOwith context and remove the commented blocks. If it's ready to wire in, uncomment and implement the logic. Leaving commented code in production paths degrades readability and invites stale assumptions.Also applies to: 17-20, 26-28, 44-46, 58-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/modules/classes/classes.service.ts` at line 7, Remove the unused AcademicYearContextService import and injection, then delete the commented-out academic-year logic in create and findAll. If scoping is intentionally deferred, replace it with a concise TODO documenting the required future behavior; otherwise, implement and enable the logic consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/modules/academic-years/academic-years.service.ts`:
- Around line 116-127: Normalize the academic-year name in the update method to
match create behavior. In the Prisma update data within the academic-year
service’s update() method, trim updateAcademicYearDto.name before persisting it,
while preserving the existing handling of startDate and endDate.
- Line 3: Remove the unused AcademicYearContextService import and injection from
AcademicYearsService so it remains singleton-scoped; alternatively, use the
service in a request-aware method and add a corresponding mock provider in
academic-years.service.spec.ts.
---
Outside diff comments:
In `@server/src/database/seeders/seed-exams.ts`:
- Around line 17-21: Remove the unused academicYear lookup from the seeder and
simplify the guard in the seeding function to return only when
teachingAllocations is empty. Keep the commented academicYearId assignments
removed or unchanged, and ensure no remaining references to academicYear remain.
---
Nitpick comments:
In `@server/src/modules/classes/classes.service.ts`:
- Line 7: Remove the unused AcademicYearContextService import and injection,
then delete the commented-out academic-year logic in create and findAll. If
scoping is intentionally deferred, replace it with a concise TODO documenting
the required future behavior; otherwise, implement and enable the logic
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9ee0485-09f2-4cac-b29b-89a13f0de64d
📒 Files selected for processing (12)
server/package.jsonserver/src/app.module.tsserver/src/common/academic-year-context/academic-year-context.module.tsserver/src/common/academic-year-context/academic-year-context.service.spec.tsserver/src/common/academic-year-context/academic-year-context.service.tsserver/src/database/seeders/seed-academic-years.tsserver/src/database/seeders/seed-classes.tsserver/src/database/seeders/seed-exams.tsserver/src/modules/academic-years/academic-years.service.tsserver/src/modules/academic-years/dto/create-academic-year.dto.tsserver/src/modules/academic-years/dto/update-academic-year.dto.tsserver/src/modules/classes/classes.service.ts
| @@ -1,5 +1,6 @@ | |||
| import { Injectable, NotFoundException } from '@nestjs/common'; | |||
| import { Prisma } from '../../../prisma/generated/prisma/client.js'; | |||
| import { AcademicYearContextService } from '../../common/academic-year-context/academic-year-context.service.js'; | |||
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the test file mocks AcademicYearContextService
rg -n 'AcademicYearContextService' server/src/modules/academic-years/academic-years.service.spec.tsRepository: thureinhtet99/student-os
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== service outline ==\n'
ast-grep outline server/src/modules/academic-years/academic-years.service.ts --view expanded
printf '\n== service file ==\n'
cat -n server/src/modules/academic-years/academic-years.service.ts | sed -n '1,220p'
printf '\n== context service outline ==\n'
ast-grep outline server/src/common/academic-year-context/academic-year-context.service.ts --view expanded
printf '\n== context service file ==\n'
cat -n server/src/common/academic-year-context/academic-year-context.service.ts | sed -n '1,220p'
printf '\n== spec file ==\n'
cat -n server/src/modules/academic-years/academic-years.service.spec.ts | sed -n '1,260p'Repository: thureinhtet99/student-os
Length of output: 12270
🌐 Web query:
NestJS request-scoped provider injected into singleton service makes consumer request-scoped
💡 Result:
Yes, in NestJS, injecting a request-scoped provider into a singleton service causes the singleton service to become request-scoped as well [1][2][3]. This behavior is known as scope bubbling [1][2][4]. When a provider is marked as request-scoped (using Scope.REQUEST), NestJS must create a new instance of that provider for every incoming request [1][2][3]. Because a singleton service is expected to be shared across the entire application lifetime, NestJS cannot safely inject a volatile, request-specific instance into it [2][3]. To maintain the integrity of the dependency injection container, NestJS automatically promotes the singleton service to request scope [1][2]. This promotion behavior has several implications: 1. Scope Bubbling: The promotion propagates up the dependency graph [1][2][3]. Any controller or service that depends on that promoted singleton will also be forced into request scope [1][2]. 2. Performance Impact: Because the promoted services are now instantiated per request rather than being cached as singletons, this can increase the overhead of handling requests, potentially slowing down application performance [1][2][4]. 3. Background Contexts: If a service that has been promoted to request scope is later injected into a context where no request exists (such as a background worker, cron job, or application bootstrap), the application may fail to resolve the dependency because there is no request-specific DI container available [3]. It is generally recommended to use the default singleton scope whenever possible and only use request scope when absolutely necessary [1][4]. For cases involving request-specific metadata (like user context), developers often use AsyncLocalStorage or pass request metadata explicitly through method arguments to avoid forcing services into request scope [3].
Citations:
- 1: https://stackoverflow.com/questions/53679456/nestjs-how-to-register-transient-and-per-web-request-providers
- 2: https://nestjs-ninja.com/blog/2026-07-30-nestjs-injection-scopes-default-request-transient/
- 3: https://ofeng.org/posts/nestjs-request-scoped-providers-background-workers/
- 4: https://www.stanza.dev/courses/nestjs-fundamentals/advanced-dependency-injection/nestjs-fundamentals-injection-scopes
Remove the unused AcademicYearContextService injection, or use it in a request-aware method.
It only promotes AcademicYearsService to request scope, adding per-request DI overhead with no benefit. If it stays, server/src/modules/academic-years/academic-years.service.spec.ts also needs a mock provider for AcademicYearContextService.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/academic-years/academic-years.service.ts` at line 3,
Remove the unused AcademicYearContextService import and injection from
AcademicYearsService so it remains singleton-scoped; alternatively, use the
service in a request-aware method and add a corresponding mock provider in
academic-years.service.spec.ts.
| return await this.prisma.academicYear.update({ | ||
| where: { id }, | ||
| data: { | ||
| name: updateAcademicYearDto.name, | ||
| startDate: updateAcademicYearDto.startDate | ||
| ? new Date(updateAcademicYearDto.startDate) | ||
| : undefined, | ||
| endDate: updateAcademicYearDto.endDate | ||
| ? new Date(updateAcademicYearDto.endDate) | ||
| : undefined, | ||
| isCurrent: updateAcademicYearDto.isCurrent, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'server/src/modules/academic-years/*' 'server/src/modules/*academic*year*/*' | sed 's#^`#-` #'
printf '\n== Service outline ==\n'
ast-grep outline server/src/modules/academic-years/academic-years.service.ts --view expanded || true
printf '\n== DTO candidates ==\n'
fd -a '.*dto.*' server/src/modules/academic-years server/src/modules | sed 's#^`#-` #'
printf '\n== Search for academic year DTOs and decorators ==\n'
rg -n "class .*AcademicYear|PartialType|IsNotEmpty|IsOptional|trim\\(|new Date\\(" server/src/modules/academic-years server/src/modules -g '*.ts' -A 3 -B 3 || trueRepository: thureinhtet99/student-os
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== update() method ==\n'
sed -n '105,130p' server/src/modules/academic-years/academic-years.service.ts
printf '\n== academic-years service tests around update ==\n'
rg -n "update\\(|trim|name|startDate|endDate" server/src/modules/academic-years/academic-years.service.spec.ts -A 4 -B 4 || trueRepository: thureinhtet99/student-os
Length of output: 1020
Trim name in update() too. create() normalizes the academic-year label, but this path writes raw input, so whitespace-padded updates can still create duplicate-looking values and trip the unique constraint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/academic-years/academic-years.service.ts` around lines 116
- 127, Normalize the academic-year name in the update method to match create
behavior. In the Prisma update data within the academic-year service’s update()
method, trim updateAcademicYearDto.name before persisting it, while preserving
the existing handling of startDate and endDate.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes