Implement foundation of greedy algorithm client side#285
Open
mehdo14 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a matching router and a new company-greedy matching implementation; the constraint-summary component now delegates allocation generation to MatchingRouterService, and tests and models supporting the greedy strategy are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component<br/>ConstraintSummary
participant Router as Service<br/>MatchingRouter
participant Greedy as Service<br/>CompanyGreedy
participant Legacy as Service<br/>MatchingService
Component->>Router: getAllocations(students, projects, constraintWrappers, locks, courseIteration)
Router->>Router: getMatchingMode(input)
alt company-greedy
Router->>Greedy: getAllocations(mappedStudents, mappedProjects, constraintWrappers, locks, semester)
Greedy->>Greedy: Filter & shuffle eligible students
Greedy->>Greedy: Validate & apply locks
Greedy->>Greedy: Fill primary slots (perfect → qualified → general)
Greedy->>Greedy: Iterate to fill remaining slots (respect capacity & constraints)
Greedy-->>Router: Allocation[]
else ranked-project-preferences
Router->>Router: Build constraints (active wrappers)
Router->>Legacy: getAllocations(constraints)
Legacy-->>Router: Allocation[]
end
Router-->>Component: Allocation[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 3
🧹 Nitpick comments (6)
client/src/app/shared/matching/company-greedy/company-greedy.models.ts (1)
19-27: LGTM — models align with the existingAllocationshape.Minor note:
activeSemester: stringis required; the router call site passescourseIteration?.semesterNamewhich can be undefined. I've flagged the mismatch at the router. Consider makingactiveSemester?: stringhere to match the call-site intent and the service's own!input.activeSemesterfallback on line 38 ofcompany-greedy-matching.service.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/company-greedy/company-greedy.models.ts` around lines 19 - 27, The interface CompanyGreedyMatchingInput currently requires activeSemester but callers (router using courseIteration?.semesterName) may pass undefined; make activeSemester optional by changing its type to activeSemester?: string on CompanyGreedyMatchingInput so it matches the call site and the service's fallback logic (see company-greedy-matching.service.ts where input.activeSemester is checked). Ensure any downstream consumers handle the optional value consistently.client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts (3)
62-88: Lock handling looks correct — nit on the warning channel.
forEach((projectId, studentId) => ...)correctly destructures the Map<studentId, projectId> callback args, capacity and constraint violations both short-circuit, and already-assigned students are skipped. One small nit:console.warnleaks per-student IDs to the browser console and the greedy path silently drops violating locks, which diverges from the LP path (which surfaces a toast on failure). Consider surfacing a user-visible aggregate warning viaToastsServiceaftergetAllocations, so operators notice dropped locks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts` around lines 62 - 88, The per-student console.warns in applyLocks leak IDs to the console and silently drop violating locks; instead collect failed lock infos inside applyLocks (reference applyLocks, the locks Map and assignedStudentIds, and failure cases: missing entities and canAssignStudentToProject), return or attach an aggregate summary of dropped locks to the result of getAllocations, and surface that summary to the operator via ToastsService in the caller (reference getAllocations and ToastsService) so users see a single user-visible warning rather than per-student console warnings.
43-53: Non-deterministic allocations — consider an injectable RNG for reproducibility.
Math.random()makes results non-reproducible across runs, which complicates debugging and any future snapshot testing. The spec'performs a fair first round...'only asserts counts (2 vs 1), so it happens to be stable, but any future spec that asserts specific student IDs would flake.Optional: inject a seedable RNG (or accept a
rng?: () => numberon the input) so tests and reruns can be deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts` around lines 43 - 53, Replace the use of Math.random() inside shuffleStudents with an injectable or passed RNG so shuffling is deterministic in tests: modify CompanyGreedyMatchingService (or the module providing it) to accept a RNG function (e.g., via constructor injection of a RandomService or an optional rng: () => number parameter) and update shuffleStudents(students: CompanyGreedyStudent[]) to use that rng function instead of Math.random(); ensure a default fallback to Math.random() for production and update callers/tests to pass a seedable RNG when determinism is required.
106-134: Scalability concern for 1000+ projects — consider reducing the candidate-scan cost.For each pass of
fillRemainingCapacity, every project callsfindBestCandidate, which scans the fullstudentsarray up to 3 times and runscanAssignStudentToProject(includingsatisfiesActiveNationalityConstraints) for every student. The outerwhilemay run until no progress is made. Worst-case cost is roughly O(passes × projects × students × constraints × avg(studentIds length)).Cheap wins:
- Maintain a running
Set<string>of remaining (unassigned) students and iterate that set instead of the fullstudentsarray on each pass.- In
satisfiesActiveNationalityConstraints(lines 244-265), pre-compute the active nationality constraints for each project once pergetAllocationscall, and convertconstraintFunction.studentIdsto aSet<string>for O(1) membership — the currentprojectedStudentIds.filter(...includes...)is O(n·m) per check.Not blocking, but worth addressing before this is enabled on 1000+ project datasets as called out in the PR objectives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts` around lines 106 - 134, fillRemainingCapacity is repeatedly scanning the entire students array via assignBestCandidate/findBestCandidate and calling satisfiesActiveNationalityConstraints, causing O(projects×students×passes) work; change fillRemainingCapacity to track a running Set<string> of remainingUnassignedStudentIds and iterate that set instead of the full students array so each student is considered only while unassigned, and update the set when a student is allocated; additionally, in getAllocations precompute active nationality constraint data per project (used by satisfiesActiveNationalityConstraints) and convert constraintFunction.studentIds into a Set<string> for O(1) membership checks (replace any projectedStudentIds.filter(...includes...) usage), so satisfiesActiveNationalityConstraints and related constraint checks use the precomputed sets rather than repeated O(n·m) scans.client/src/app/shared/matching/matching-router.service.ts (1)
36-43: Unchecked type casts toCompanyGreedyStudent[]/CompanyGreedyProject[].
input.students as CompanyGreedyStudent[]/input.projects as CompanyGreedyProject[]are safe today only because all the greedy-specific fields are optional, but this defeats TypeScript's help if base types change. SincehasCompanyGreedyDataalready distinguishes shapes for auto-detection, consider either typingMatchingRouterInputwith the greedy variants directly on the greedy path (e.g., overload or split inputs) or adding a small mapper that only copies known greedy fields, so an accidental schema drift onStudent/Projectisn't silently absorbed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/matching-router.service.ts` around lines 36 - 43, The unchecked casts to CompanyGreedyStudent[]/CompanyGreedyProject[] in the path that calls companyGreedyMatchingService.getAllocations are unsafe; change the matching router to either (a) narrow the input type for the greedy branch (add an overload or split MatchingRouterInput into a greedy-specific variant used when hasCompanyGreedyData is true) so the compiler knows students/projects are the greedy shapes, or (b) add a lightweight mapper function (e.g., mapToCompanyGreedyStudent/mapToCompanyGreedyProject) that constructs new arrays containing only the greedy-specific fields before calling companyGreedyMatchingService.getAllocations; locate this logic around hasCompanyGreedyData and the call site to companyGreedyMatchingService.getAllocations and implement one of the two fixes so you remove the direct "as" casts.client/src/app/shared/matching/company-greedy/company-greedy-matching.service.spec.ts (1)
19-139: Consider adding coverage for a few untested branches.Nice coverage overall. A few gaps worth filling once a runner is wired up (per PR objectives):
- Lock rejection path: a lock that would violate a constraint (e.g., nationality upper bound) → expect the warning path and no assignment, verifying the student is still eligible for other projects.
- Lock + capacity interaction: two locks for the same project beyond
capacity→ second lock should be skipped.- The
registeredBefore=false+ matchingpreferredProjectTypescase vs aregisteredBefore=truestudent without type match — this pins down the tier ordering (see the related comment onisQualifiedMatch/isGeneralMatchin the service).- Empty
projectsinput → returns[]without error.Also optional: the ambient
declare const describe/beforeEach/it/expectblock at lines 8-17 will become redundant once@types/jasmine(or equivalent) is configured — worth removing then to avoid drift from the real matcher types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.spec.ts` around lines 19 - 139, Add unit tests exercising untested branches in CompanyGreedyMatchingService.getAllocations: (1) Lock rejection: create a lock Map that assigns a student to a project violating a constraint (use createNationalityConstraint and createStudent/createProject) and assert the student is not assigned to that project, a warning path is observed by asserting the student remains assignable to other projects, and the invalid lock is effectively ignored. (2) Lock + capacity overflow: set two locks for different students both targeting the same project whose capacity is 1 and assert only the first lock is applied and the second student is not assigned to that project. (3) Tier ordering (registeredBefore vs type match): create two students where one has registeredBefore=false but matches preferredProjectTypes and the other has registeredBefore=true but no type match, run getAllocations with the target project and assert the tie-breaker/order results in the expected student per the service’s isQualifiedMatch/isGeneralMatch logic. (4) Empty projects: call getAllocations with projects: [] and assert it returns [] without throwing. Use createInput/createStudent/createProject/constraint wrappers and locks Map to locate code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts`:
- Around line 173-211: The match-tiering logic currently demotes new students
who explicitly prefer the project type; update the predicate logic so that
perfect remains (matchesProjectTypeInterest AND registeredBefore) but qualified
becomes (canAssignStudentToProject AND (matchesProjectTypeInterest OR
registeredBefore)) and general becomes (canAssignStudentToProject AND NOT
(matchesProjectTypeInterest OR registeredBefore)); edit isQualifiedMatch and
isGeneralMatch accordingly while leaving isPerfectMatch and the helper
matchesProjectTypeInterest/canAssignStudentToProject usage intact.
In `@client/src/app/shared/matching/matching-router.service.ts`:
- Around line 69-77: The normalizeMatchingMode function currently only matches
exact-cased variants and will return null for inputs like "Company-Greedy",
"company_greedy", or values with surrounding whitespace; update
normalizeMatchingMode to robustly normalize inputs by trimming whitespace,
converting to lower case, and normalizing separators (e.g. convert underscores
to hyphens) before comparing, then map the normalized string to either
'company-greedy' or 'ranked-project-preferences' (or return null) so
CourseIterationWithMatchingMode['matchingMode'] values from prompts/config are
handled reliably.
- Around line 34-43: The activeSemester field on CompanyGreedyMatchingInput is
declared required but callers pass input.courseIteration?.semesterName (string |
undefined); update the model in company-greedy.models.ts to make activeSemester
optional (e.g., activeSemester?: string or string | undefined) so it matches the
caller and the guard in CompanyGreedyMatchingService.getAllocations; update any
type references to CompanyGreedyMatchingInput or
CompanyGreedyMatchingService.getAllocations signature if necessary to accept the
optional value.
---
Nitpick comments:
In
`@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.spec.ts`:
- Around line 19-139: Add unit tests exercising untested branches in
CompanyGreedyMatchingService.getAllocations: (1) Lock rejection: create a lock
Map that assigns a student to a project violating a constraint (use
createNationalityConstraint and createStudent/createProject) and assert the
student is not assigned to that project, a warning path is observed by asserting
the student remains assignable to other projects, and the invalid lock is
effectively ignored. (2) Lock + capacity overflow: set two locks for different
students both targeting the same project whose capacity is 1 and assert only the
first lock is applied and the second student is not assigned to that project.
(3) Tier ordering (registeredBefore vs type match): create two students where
one has registeredBefore=false but matches preferredProjectTypes and the other
has registeredBefore=true but no type match, run getAllocations with the target
project and assert the tie-breaker/order results in the expected student per the
service’s isQualifiedMatch/isGeneralMatch logic. (4) Empty projects: call
getAllocations with projects: [] and assert it returns [] without throwing. Use
createInput/createStudent/createProject/constraint wrappers and locks Map to
locate code paths.
In
`@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts`:
- Around line 62-88: The per-student console.warns in applyLocks leak IDs to the
console and silently drop violating locks; instead collect failed lock infos
inside applyLocks (reference applyLocks, the locks Map and assignedStudentIds,
and failure cases: missing entities and canAssignStudentToProject), return or
attach an aggregate summary of dropped locks to the result of getAllocations,
and surface that summary to the operator via ToastsService in the caller
(reference getAllocations and ToastsService) so users see a single user-visible
warning rather than per-student console warnings.
- Around line 43-53: Replace the use of Math.random() inside shuffleStudents
with an injectable or passed RNG so shuffling is deterministic in tests: modify
CompanyGreedyMatchingService (or the module providing it) to accept a RNG
function (e.g., via constructor injection of a RandomService or an optional rng:
() => number parameter) and update shuffleStudents(students:
CompanyGreedyStudent[]) to use that rng function instead of Math.random();
ensure a default fallback to Math.random() for production and update
callers/tests to pass a seedable RNG when determinism is required.
- Around line 106-134: fillRemainingCapacity is repeatedly scanning the entire
students array via assignBestCandidate/findBestCandidate and calling
satisfiesActiveNationalityConstraints, causing O(projects×students×passes) work;
change fillRemainingCapacity to track a running Set<string> of
remainingUnassignedStudentIds and iterate that set instead of the full students
array so each student is considered only while unassigned, and update the set
when a student is allocated; additionally, in getAllocations precompute active
nationality constraint data per project (used by
satisfiesActiveNationalityConstraints) and convert constraintFunction.studentIds
into a Set<string> for O(1) membership checks (replace any
projectedStudentIds.filter(...includes...) usage), so
satisfiesActiveNationalityConstraints and related constraint checks use the
precomputed sets rather than repeated O(n·m) scans.
In `@client/src/app/shared/matching/company-greedy/company-greedy.models.ts`:
- Around line 19-27: The interface CompanyGreedyMatchingInput currently requires
activeSemester but callers (router using courseIteration?.semesterName) may pass
undefined; make activeSemester optional by changing its type to activeSemester?:
string on CompanyGreedyMatchingInput so it matches the call site and the
service's fallback logic (see company-greedy-matching.service.ts where
input.activeSemester is checked). Ensure any downstream consumers handle the
optional value consistently.
In `@client/src/app/shared/matching/matching-router.service.ts`:
- Around line 36-43: The unchecked casts to
CompanyGreedyStudent[]/CompanyGreedyProject[] in the path that calls
companyGreedyMatchingService.getAllocations are unsafe; change the matching
router to either (a) narrow the input type for the greedy branch (add an
overload or split MatchingRouterInput into a greedy-specific variant used when
hasCompanyGreedyData is true) so the compiler knows students/projects are the
greedy shapes, or (b) add a lightweight mapper function (e.g.,
mapToCompanyGreedyStudent/mapToCompanyGreedyProject) that constructs new arrays
containing only the greedy-specific fields before calling
companyGreedyMatchingService.getAllocations; locate this logic around
hasCompanyGreedyData and the call site to
companyGreedyMatchingService.getAllocations and implement one of the two fixes
so you remove the direct "as" casts.
🪄 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: 27bce41a-b6ce-4b30-aa48-f2203619e450
📒 Files selected for processing (5)
client/src/app/components/constraint-summary-view/constraint-summary.component.tsclient/src/app/shared/matching/company-greedy/company-greedy-matching.service.spec.tsclient/src/app/shared/matching/company-greedy/company-greedy-matching.service.tsclient/src/app/shared/matching/company-greedy/company-greedy.models.tsclient/src/app/shared/matching/matching-router.service.ts
Comment on lines
+173
to
+211
| private isPerfectMatch( | ||
| student: CompanyGreedyStudent, | ||
| project: CompanyGreedyProject, | ||
| allocation: Allocation, | ||
| assignedStudentIds: Set<string>, | ||
| constraintWrappers: ConstraintWrapper[] | ||
| ): boolean { | ||
| return ( | ||
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | ||
| this.matchesProjectTypeInterest(student, project) && | ||
| student.registeredBefore === true | ||
| ); | ||
| } | ||
|
|
||
| private isQualifiedMatch( | ||
| student: CompanyGreedyStudent, | ||
| project: CompanyGreedyProject, | ||
| allocation: Allocation, | ||
| assignedStudentIds: Set<string>, | ||
| constraintWrappers: ConstraintWrapper[] | ||
| ): boolean { | ||
| return ( | ||
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | ||
| student.registeredBefore === true | ||
| ); | ||
| } | ||
|
|
||
| private isGeneralMatch( | ||
| student: CompanyGreedyStudent, | ||
| project: CompanyGreedyProject, | ||
| allocation: Allocation, | ||
| assignedStudentIds: Set<string>, | ||
| constraintWrappers: ConstraintWrapper[] | ||
| ): boolean { | ||
| return ( | ||
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | ||
| student.registeredBefore !== true | ||
| ); | ||
| } |
There was a problem hiding this comment.
Match tiering leaves a gap: new students who explicitly prefer this project type are demoted.
isPerfectMatchrequiresregisteredBefore === trueand project-type interest.isQualifiedMatchrequiresregisteredBefore === trueonly.isGeneralMatchrequiresregisteredBefore !== true.
Consequence: a new student whose preferredProjectTypes matches project.projectType falls into the general tier, ranked below a returning student with no project-type interest. That inverts the stated priority (project-type interest as the strongest signal) and likely contradicts intent for "company/project-type interests" matching described in the PR summary.
🛠️ Suggested refactor — promote project-type interest above returning-student status
private isPerfectMatch(
student: CompanyGreedyStudent,
project: CompanyGreedyProject,
allocation: Allocation,
assignedStudentIds: Set<string>,
constraintWrappers: ConstraintWrapper[]
): boolean {
return (
this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) &&
this.matchesProjectTypeInterest(student, project) &&
student.registeredBefore === true
);
}
private isQualifiedMatch(
student: CompanyGreedyStudent,
project: CompanyGreedyProject,
allocation: Allocation,
assignedStudentIds: Set<string>,
constraintWrappers: ConstraintWrapper[]
): boolean {
return (
this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) &&
- student.registeredBefore === true
+ (this.matchesProjectTypeInterest(student, project) || student.registeredBefore === true)
);
}This way: perfect = returning + type match, qualified = type match or returning, general = anyone else feasible.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private isPerfectMatch( | |
| student: CompanyGreedyStudent, | |
| project: CompanyGreedyProject, | |
| allocation: Allocation, | |
| assignedStudentIds: Set<string>, | |
| constraintWrappers: ConstraintWrapper[] | |
| ): boolean { | |
| return ( | |
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | |
| this.matchesProjectTypeInterest(student, project) && | |
| student.registeredBefore === true | |
| ); | |
| } | |
| private isQualifiedMatch( | |
| student: CompanyGreedyStudent, | |
| project: CompanyGreedyProject, | |
| allocation: Allocation, | |
| assignedStudentIds: Set<string>, | |
| constraintWrappers: ConstraintWrapper[] | |
| ): boolean { | |
| return ( | |
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | |
| student.registeredBefore === true | |
| ); | |
| } | |
| private isGeneralMatch( | |
| student: CompanyGreedyStudent, | |
| project: CompanyGreedyProject, | |
| allocation: Allocation, | |
| assignedStudentIds: Set<string>, | |
| constraintWrappers: ConstraintWrapper[] | |
| ): boolean { | |
| return ( | |
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | |
| student.registeredBefore !== true | |
| ); | |
| } | |
| private isPerfectMatch( | |
| student: CompanyGreedyStudent, | |
| project: CompanyGreedyProject, | |
| allocation: Allocation, | |
| assignedStudentIds: Set<string>, | |
| constraintWrappers: ConstraintWrapper[] | |
| ): boolean { | |
| return ( | |
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | |
| this.matchesProjectTypeInterest(student, project) && | |
| student.registeredBefore === true | |
| ); | |
| } | |
| private isQualifiedMatch( | |
| student: CompanyGreedyStudent, | |
| project: CompanyGreedyProject, | |
| allocation: Allocation, | |
| assignedStudentIds: Set<string>, | |
| constraintWrappers: ConstraintWrapper[] | |
| ): boolean { | |
| return ( | |
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | |
| (this.matchesProjectTypeInterest(student, project) || student.registeredBefore === true) | |
| ); | |
| } | |
| private isGeneralMatch( | |
| student: CompanyGreedyStudent, | |
| project: CompanyGreedyProject, | |
| allocation: Allocation, | |
| assignedStudentIds: Set<string>, | |
| constraintWrappers: ConstraintWrapper[] | |
| ): boolean { | |
| return ( | |
| this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers) && | |
| student.registeredBefore !== true | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts`
around lines 173 - 211, The match-tiering logic currently demotes new students
who explicitly prefer the project type; update the predicate logic so that
perfect remains (matchesProjectTypeInterest AND registeredBefore) but qualified
becomes (canAssignStudentToProject AND (matchesProjectTypeInterest OR
registeredBefore)) and general becomes (canAssignStudentToProject AND NOT
(matchesProjectTypeInterest OR registeredBefore)); edit isQualifiedMatch and
isGeneralMatch accordingly while leaving isPerfectMatch and the helper
matchesProjectTypeInterest/canAssignStudentToProject usage intact.
Comment on lines
+34
to
+43
| async getAllocations(input: MatchingRouterInput): Promise<Allocation[]> { | ||
| if (this.getMatchingMode(input) === 'company-greedy') { | ||
| return this.companyGreedyMatchingService.getAllocations({ | ||
| students: input.students as CompanyGreedyStudent[], | ||
| projects: input.projects as CompanyGreedyProject[], | ||
| constraintWrappers: input.constraintWrappers, | ||
| locks: input.locks, | ||
| activeSemester: input.courseIteration?.semesterName, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether CourseIteration.semesterName is optional, and check call sites of MatchingRouterService.
fd -e ts | xargs rg -nP --no-heading -C2 '\bsemesterName\b' | head -n 80
echo '---'
rg -nP --no-heading -C2 'interface\s+CourseIteration\b|class\s+CourseIteration\b' --type=ts | head -n 60
echo '---'
rg -nP --no-heading -C3 'matchingRouterService\.getAllocations|MatchingRouterService' --type=ts | head -n 80Repository: prompt-edu/tease
Length of output: 5401
🏁 Script executed:
#!/bin/bash
# Check CompanyGreedyMatchingInput definition and activeSemester field
rg -nP --no-heading -B5 -A10 'interface\s+CompanyGreedyMatchingInput|activeSemester' --type=ts | head -n 60
echo '---'
# Check the fallback logic in company-greedy-matching.service.ts around line 38
rg -nP --no-heading -B5 -A5 '!input\.activeSemester|activeSemester.*==' --type=ts | head -n 80Repository: prompt-edu/tease
Length of output: 7951
activeSemester is typed as required (string) but receives an optional value (string | undefined).
input.courseIteration?.semesterName is string | undefined (since CourseIteration.semesterName is optional), but CompanyGreedyMatchingInput.activeSemester is declared as required (string) in company-greedy.models.ts:24. This causes a type mismatch that will fail strict TypeScript compilation.
The service already handles this gracefully with a fallback on line 38 of company-greedy-matching.service.ts (!input.activeSemester ||...), so the cleanest fix is to make the field optional in the model:
🛠️ Proposed fix
// company-greedy.models.ts
export interface CompanyGreedyMatchingInput {
students: CompanyGreedyStudent[];
projects: CompanyGreedyProject[];
constraintWrappers: ConstraintWrapper[];
locks: StudentIdToProjectIdMapping;
- activeSemester: string;
+ activeSemester?: string;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/shared/matching/matching-router.service.ts` around lines 34 -
43, The activeSemester field on CompanyGreedyMatchingInput is declared required
but callers pass input.courseIteration?.semesterName (string | undefined);
update the model in company-greedy.models.ts to make activeSemester optional
(e.g., activeSemester?: string or string | undefined) so it matches the caller
and the guard in CompanyGreedyMatchingService.getAllocations; update any type
references to CompanyGreedyMatchingInput or
CompanyGreedyMatchingService.getAllocations signature if necessary to accept the
optional value.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/src/app/shared/matching/matching-router.service.ts (1)
84-102: Mapping helpers can be simplified — the explicit re-assignments are redundant.
{ ...student }/{ ...project }already copy every own enumerable runtime property, so re-reading each optional field via a cast toCompanyGreedyStudent/CompanyGreedyProjectjust surfaces the types, it doesn't add any runtime data (and converts "field absent" into "field: undefined", which is harmless given all consumer sites use optional chaining). You can collapse both helpers to a single cast and keep the type contract intact:♻️ Proposed refactor
- private mapToCompanyGreedyStudents(students: Student[]): CompanyGreedyStudent[] { - return students.map(student => ({ - ...student, - preferredProjectTypes: (student as CompanyGreedyStudent).preferredProjectTypes, - registeredBefore: (student as CompanyGreedyStudent).registeredBefore, - previousParticipation: (student as CompanyGreedyStudent).previousParticipation, - blacklisted: (student as CompanyGreedyStudent).blacklisted, - registeredIn: (student as CompanyGreedyStudent).registeredIn, - })); - } - - private mapToCompanyGreedyProjects(projects: Project[]): CompanyGreedyProject[] { - return projects.map(project => ({ - ...project, - projectType: (project as CompanyGreedyProject).projectType, - preferredStudyPrograms: (project as CompanyGreedyProject).preferredStudyPrograms, - capacity: (project as CompanyGreedyProject).capacity, - })); - } + private mapToCompanyGreedyStudents(students: Student[]): CompanyGreedyStudent[] { + return students.map(student => ({ ...(student as CompanyGreedyStudent) })); + } + + private mapToCompanyGreedyProjects(projects: Project[]): CompanyGreedyProject[] { + return projects.map(project => ({ ...(project as CompanyGreedyProject) })); + }Once PROMPT-side mapping lands (per the PR's "known limitations"), you may want to replace these with a real adapter rather than a structural cast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/matching-router.service.ts` around lines 84 - 102, The two mapping helpers mapToCompanyGreedyStudents and mapToCompanyGreedyProjects redundantly spread the object and then reassign optional fields (casting to CompanyGreedyStudent/CompanyGreedyProject) which is unnecessary and introduces extra undefined properties; simplify each by returning the input array cast to the desired type (e.g., return students as CompanyGreedyStudent[] and return projects as CompanyGreedyProject[]) or by mapping to identity with a straight cast per element to preserve the compile-time contract without reassigning each field.client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts (1)
153-171: Consider bucketing candidates once instead of three passes per project.
findBestCandidateinvokesstudents.find(...)three times per project per pass, and each tier predicate re-runscanAssignStudentToProject(capacity + study-program + nationality filtering) for every student it visits. With the 1000+ project / large-cohort scale called out in the PR summary andfillRemainingCapacitylooping until no new assignment happens, the total work grows quickly (roughly O(projects × students × passes × 3)).A single linear scan that buckets feasible candidates by tier would eliminate the redundant
canAssignStudentToProjectwork and return the first perfect/qualified/general hit in priority order:♻️ Proposed refactor
private findBestCandidate( project: CompanyGreedyProject, students: CompanyGreedyStudent[], allocation: Allocation, assignedStudentIds: Set<string>, constraintWrappers: ConstraintWrapper[] ): CompanyGreedyStudent | undefined { - return ( - students.find(student => - this.isPerfectMatch(student, project, allocation, assignedStudentIds, constraintWrappers) - ) || - students.find(student => - this.isQualifiedMatch(student, project, allocation, assignedStudentIds, constraintWrappers) - ) || - students.find(student => - this.isGeneralMatch(student, project, allocation, assignedStudentIds, constraintWrappers) - ) - ); + let qualified: CompanyGreedyStudent | undefined; + let general: CompanyGreedyStudent | undefined; + + for (const student of students) { + if (!this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, constraintWrappers)) { + continue; + } + const typeMatch = this.matchesProjectTypeInterest(student, project); + const returning = student.registeredBefore === true; + if (typeMatch && returning) { + return student; // perfect + } + if (!qualified && (typeMatch || returning)) { + qualified = student; + } else if (!general && !typeMatch && !returning) { + general = student; + } + } + return qualified ?? general; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts` around lines 153 - 171, findBestCandidate currently calls students.find three times causing repeated calls to canAssignStudentToProject; refactor findBestCandidate (and keep signature with CompanyGreedyProject, CompanyGreedyStudent[], Allocation, assignedStudentIds, constraintWrappers) to do a single linear scan over students, call canAssignStudentToProject once per student, classify each feasible student into three buckets (perfect via isPerfectMatch, qualified via isQualifiedMatch, general via isGeneralMatch) and then return the first student from the highest-priority non-empty bucket (perfect, then qualified, then general); ensure you still respect assignedStudentIds and constraintWrappers during the single pass and preserve existing early-return semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts`:
- Around line 62-88: The applyLocks function currently swallows invalid or
infeasible locks with console.warn; change it to collect and surface these
skipped locks (with distinct reasons for "ineligible student" vs "constraint
violation") instead of silently dropping them: update applyLocks (and its
signature if needed) to append objects like {studentId, projectId, reason} to a
new/sketch field on CompanyGreedyMatchingResult (or return a list) whenever a
lock is skipped, use the existing canAssignStudentToProject check to distinguish
feasibility violations from eligibility checks, and ensure callers of
CompanyGreedyMatchingInput/Allocation receive and propagate that skipped-lock
list so the UI can display or log precise failures rather than relying on
console.warn.
---
Nitpick comments:
In
`@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts`:
- Around line 153-171: findBestCandidate currently calls students.find three
times causing repeated calls to canAssignStudentToProject; refactor
findBestCandidate (and keep signature with CompanyGreedyProject,
CompanyGreedyStudent[], Allocation, assignedStudentIds, constraintWrappers) to
do a single linear scan over students, call canAssignStudentToProject once per
student, classify each feasible student into three buckets (perfect via
isPerfectMatch, qualified via isQualifiedMatch, general via isGeneralMatch) and
then return the first student from the highest-priority non-empty bucket
(perfect, then qualified, then general); ensure you still respect
assignedStudentIds and constraintWrappers during the single pass and preserve
existing early-return semantics.
In `@client/src/app/shared/matching/matching-router.service.ts`:
- Around line 84-102: The two mapping helpers mapToCompanyGreedyStudents and
mapToCompanyGreedyProjects redundantly spread the object and then reassign
optional fields (casting to CompanyGreedyStudent/CompanyGreedyProject) which is
unnecessary and introduces extra undefined properties; simplify each by
returning the input array cast to the desired type (e.g., return students as
CompanyGreedyStudent[] and return projects as CompanyGreedyProject[]) or by
mapping to identity with a straight cast per element to preserve the
compile-time contract without reassigning each field.
🪄 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: 77726a30-875a-41f9-899a-f96653e881a3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
client/src/app/shared/matching/company-greedy/company-greedy-matching.service.spec.tsclient/src/app/shared/matching/company-greedy/company-greedy-matching.service.tsclient/src/app/shared/matching/company-greedy/company-greedy.models.tsclient/src/app/shared/matching/matching-router.service.ts
✅ Files skipped from review due to trivial changes (1)
- client/src/app/shared/matching/company-greedy/company-greedy.models.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/app/shared/matching/company-greedy/company-greedy-matching.service.spec.ts
Comment on lines
+62
to
+88
| private applyLocks( | ||
| input: CompanyGreedyMatchingInput, | ||
| eligibleStudents: CompanyGreedyStudent[], | ||
| allocations: Allocation[], | ||
| assignedStudentIds: Set<string> | ||
| ): void { | ||
| input.locks.forEach((projectId, studentId) => { | ||
| const student = eligibleStudents.find(candidate => candidate.id === studentId); | ||
| const project = input.projects.find(candidate => candidate.id === projectId); | ||
| const allocation = allocations.find(candidate => candidate.projectId === projectId); | ||
|
|
||
| if (!student || !project || !allocation || assignedStudentIds.has(studentId)) { | ||
| console.warn(`Skipping invalid locked assignment for student "${studentId}" and project "${projectId}".`); | ||
| return; | ||
| } | ||
|
|
||
| if (!this.canAssignStudentToProject(student, project, allocation, assignedStudentIds, input.constraintWrappers)) { | ||
| console.warn( | ||
| `Skipping locked assignment that violates company matching constraints for student "${studentId}".` | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| allocation.students.push(student.id); | ||
| assignedStudentIds.add(student.id); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Locked assignments are silently dropped on violation.
When a lock refers to an ineligible student (blacklisted / wrong semester / already filtered) or violates capacity/study-program/nationality constraints, it is skipped with only a console.warn. Admin-configured locks are typically treated as hard overrides — operators running a 1000+ allocation pass via the UI are unlikely to notice a console message, and the student who was supposed to be locked may end up allocated elsewhere (or unassigned) without any surfaced feedback.
Consider surfacing these skipped locks back through CompanyGreedyMatchingResult (or throwing for violations, per product intent) so the caller can report them to the user. At minimum, the ineligible-student case (line 69 returning undefined) warrants distinguishing from a feasibility violation, since the two have different remediations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@client/src/app/shared/matching/company-greedy/company-greedy-matching.service.ts`
around lines 62 - 88, The applyLocks function currently swallows invalid or
infeasible locks with console.warn; change it to collect and surface these
skipped locks (with distinct reasons for "ineligible student" vs "constraint
violation") instead of silently dropping them: update applyLocks (and its
signature if needed) to append objects like {studentId, projectId, reason} to a
new/sketch field on CompanyGreedyMatchingResult (or return a list) whenever a
lock is skipped, use the existing canAssignStudentToProject check to distinguish
feasibility violations from eligibility checks, and ensure callers of
CompanyGreedyMatchingInput/Allocation receive and propagate that skipped-lock
list so the UI can display or log precise failures rather than relying on
console.warn.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces the foundation for a second matching workflow based on company/project type interests, primarily for 1000+ project matching. Companies are represented as extended Projects, students as extended Students, and the result remains the existing Allocation[ ] shape. All changes remain on the client side for now.
Changes
Current Behavior
Not Included
Summary by CodeRabbit
New Features
Tests