refactor(app): resolve projects within team context on nested routes#757
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
|
Warning Review limit reached
More reviews will be available in 18 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNested project API handlers were refactored to resolve team and project context together via a new ChangesTeam-scoped project context alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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 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 |
HugoRCD
changed the title
HugoRCD
force-pushed
the
refactor/team-project-context
branch
from
May 31, 2026 18:51
57b510e to
c33782d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/[groupId]/index.put.ts (1)
16-21: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDe-escalation: the PUT update is already project-scoped via the preflight check (minor defense-in-depth improvement only).
getGroupForProject(groupId, project.id)filters by bothvariableGroups.idandvariableGroups.projectId, throwing 404 if the group doesn’t belong to the route project.updateGroupupdates onlyname,description, andposition, andUpdateVariableGroupInputhas noprojectId, so this handler can’t reassign a group to another project even though the UPDATE predicate uses onlyid.- Optional: for tighter atomicity/consistency, add a
projectIdconstraint toupdateGroup’s.where(...)(or passproject.idinto the update input) so the mutation is constrained in a single statement.
[Optional location:apps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/[groupId]/index.put.tslines 16-21;apps/shelve/server/services/variable-groups.tsgetGroupForProject+updateGroup]🤖 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 `@apps/shelve/server/api/teams/`[slug]/projects/[projectId]/variable-groups/[groupId]/index.put.ts around lines 16 - 21, getGroupForProject is already verifying variableGroups.id and variableGroups.projectId, but updateGroup currently updates by id only; to add defense-in-depth, change VariableGroupsService.updateGroup to constrain the update by projectId as well (e.g., add projectId to the .where predicate) or accept a projectId parameter and include it in the update input; update the PUT handler in index.put.ts to pass project.id into updateGroup if you opt for the parameter approach so the mutation is atomic and cannot reassign a group across projects.apps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/[variableId]/index.put.ts (1)
21-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
groupIdagainst the resolved project before updating.This handler now resolves
projectfrom the nested route, but it still forwards any positivegroupIdtoupdateVariablewithout checking that the group belongs toproject.id. That leaves a cross-project assignment path open if a foreign group ID is supplied.variables/group.patch.tsalready added the right guard for the bulk path; this route needs the same check.Suggested fix
export default eventHandler(async (event) => { const { team, project } = await requireUserTeamProject(event, { minRole: TeamRole.ADMIN }) const { variableId } = await getValidatedRouterParams(event, variableIdParamsSchema.parse) const body = await readValidatedBody(event, updateVariableSchema.parse) const existing = await db.query.variables.findFirst({ where: and( eq(schema.variables.id, variableId), eq(schema.variables.projectId, project.id), ), }) if (!existing) throw createError({ statusCode: 404, statusMessage: 'Variable not found' }) + if (body.groupId !== undefined && body.groupId !== null) { + await new VariableGroupsService().getGroupForProject(body.groupId, project.id) + } + const environmentIds = [...new Set(body.values.map(v => v.environmentId))] await assertPushAllowedForEnvironmentIds(environmentIds, team.id, project.syncPolicy) await new VariablesService(event).updateVariable({ id: variableId,🤖 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 `@apps/shelve/server/api/teams/`[slug]/projects/[projectId]/variables/[variableId]/index.put.ts around lines 21 - 43, The handler forwards any positive groupId from the request into VariablesService.updateVariable without ensuring that the group belongs to the resolved project, allowing cross-project assignment; before calling new VariablesService(event).updateVariable(...) (after fetching project and existing variable and validating environment IDs) verify that if body.groupId is a truthy/positive ID it exists and has projectId === project.id (use the same table/query pattern as the bulk guard in variables/group.patch.ts, e.g. a variables_groups or groups query with projectId) and throw an appropriate error (404/400) if not, otherwise proceed to call updateVariable.
🤖 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
`@apps/shelve/server/api/teams/`[slug]/projects/[projectId]/variable-groups/[groupId]/index.delete.ts:
- Around line 8-9: The handler currently does a separate read via
VariableGroupsService.getGroupForProject(groupId, project.id) then calls
VariableGroupsService.deleteGroup(groupId), which makes the delete non-atomic
and allows a delete by id only; change VariableGroupsService.deleteGroup to
accept both groupId and projectId (e.g., deleteGroup(groupId, projectId)) and
implement the DB delete to include WHERE id = ? AND projectId = ? so the write
is scoped to the project atomically, then update the handler in index.delete.ts
to call deleteGroup(groupId, project.id) directly (remove the separate
getGroupForProject call).
In `@apps/shelve/server/services/projects.ts`:
- Around line 26-31: The pre-update lookup in updateProject() currently resolves
existingProject by id only; change it to call getProjectForTeam(input.id,
input.teamId) so the initial fetch is scoped to the caller's team, then perform
uniqueness checks (e.g., name uniqueness) against input.teamId as well; finally
keep the scoped UPDATE that uses eq(schema.projects.id, input.id) and
eq(schema.projects.teamId, input.teamId) and preserve the 404 throw when the
scoped update returns no row.
In `@apps/shelve/server/services/variables.ts`:
- Around line 220-235: The deleteVariablesInProject function currently performs
the DELETE then checks deleted.length, causing possible partial deletes; instead
first verify all requested variableIds exist for the given project (e.g., query
schema.variables for ids where projectId = projectId and id IN variableIds and
compare found count/ids to variableIds), throw the 404 if any are missing, and
only then execute the DELETE and call clearCache('Variables', projectId);
alternatively wrap the existence-check + delete in a transaction to ensure
atomicity.
- Around line 197-208: The bulkAssignGroup function can silently perform a
partial update when some variableIds are missing or belong to another project;
to fix, first validate that the set of variableIds all belong to the given
project (e.g., query schema.variables where id IN variableIds and projectId =
projectId and compare counts or returned ids), and if the counts/ids don’t match
throw an error (abort) instead of performing the update; then perform the update
(optionally inside a transaction) and only call clearCache('Variables',
projectId) after a successful, validated update.
In `@apps/shelve/server/utils/auth.ts`:
- Around line 5-6: The top-level import of projectIdParamsSchema in auth.ts
causes a load-time dependency on '~~/server/db/zod' that breaks tests; move the
schema import inside the requireUserTeamProject function (use a dynamic import
or require call there) so the module is only loaded at runtime when that
function runs, leaving ProjectsService imported at top-level, and update any
references to projectIdParamsSchema accordingly.
In `@apps/shelve/test/unit/team-project-context.test.ts`:
- Around line 19-25: The test should assert that the mocked getProject was
called with the expected project id to make the interaction explicit: capture
the spy returned by vi.spyOn(service, 'getProject') and after calling
service.getProjectForTeam(projectOnA.id, 1) add an expectation that the spy was
called with projectOnA.id (and optionally called once). Reference the
ProjectsService class, the getProject spy, the getProjectForTeam call, and the
projectOnA.id value when adding the assertion.
- Around line 27-35: The test currently stubs ProjectsService.getProject but
doesn't assert it was called; add an assertion that the spy on
service.getProject was invoked with the expected project id (projectOnB.id) when
calling service.getProjectForTeam(projectOnB.id, 1), and optionally assert it
was called exactly once to make the interaction explicit; reference the
ProjectsService class, the getProject spy, the getProjectForTeam method, and the
projectOnB fixture when adding this check.
---
Outside diff comments:
In
`@apps/shelve/server/api/teams/`[slug]/projects/[projectId]/variable-groups/[groupId]/index.put.ts:
- Around line 16-21: getGroupForProject is already verifying variableGroups.id
and variableGroups.projectId, but updateGroup currently updates by id only; to
add defense-in-depth, change VariableGroupsService.updateGroup to constrain the
update by projectId as well (e.g., add projectId to the .where predicate) or
accept a projectId parameter and include it in the update input; update the PUT
handler in index.put.ts to pass project.id into updateGroup if you opt for the
parameter approach so the mutation is atomic and cannot reassign a group across
projects.
In
`@apps/shelve/server/api/teams/`[slug]/projects/[projectId]/variables/[variableId]/index.put.ts:
- Around line 21-43: The handler forwards any positive groupId from the request
into VariablesService.updateVariable without ensuring that the group belongs to
the resolved project, allowing cross-project assignment; before calling new
VariablesService(event).updateVariable(...) (after fetching project and existing
variable and validating environment IDs) verify that if body.groupId is a
truthy/positive ID it exists and has projectId === project.id (use the same
table/query pattern as the bulk guard in variables/group.patch.ts, e.g. a
variables_groups or groups query with projectId) and throw an appropriate error
(404/400) if not, otherwise proceed to call updateVariable.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 018e6e3d-6cc5-46cd-82e3-b20e3953d88f
📒 Files selected for processing (20)
.changeset/team-project-context.mdapps/shelve/server/api/teams/[slug]/projects/[projectId]/index.delete.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/index.get.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/index.put.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/[groupId]/index.delete.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/[groupId]/index.put.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/index.get.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/index.post.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/[variableId]/index.delete.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/[variableId]/index.put.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/env/[envId].get.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/group.patch.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/index.delete.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/index.get.tsapps/shelve/server/api/teams/[slug]/projects/[projectId]/variables/index.post.tsapps/shelve/server/services/projects.tsapps/shelve/server/services/variable-groups.tsapps/shelve/server/services/variables.tsapps/shelve/server/utils/auth.tsapps/shelve/test/unit/team-project-context.test.ts
Comment on lines
+8
to
9
| await new VariableGroupsService().getGroupForProject(groupId, project.id) | ||
| await new VariableGroupsService().deleteGroup(groupId) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 '\b(getGroupForProject|deleteGroup)\s*\(' \
apps/shelve/server/services/variable-groups.ts \
"apps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups"Repository: HugoRCD/shelve
Length of output: 3421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' apps/shelve/server/services/variable-groups.ts | nl -ba | sed -n '1,120p'Repository: HugoRCD/shelve
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba apps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/[groupId]/index.delete.ts | sed -n '1,120p'Repository: HugoRCD/shelve
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== variable-groups.ts: getGroupForProject + deleteGroup ==="
cat -n apps/shelve/server/services/variable-groups.ts | sed -n '1,120p'
echo "=== variable-groups.ts: deleteGroup (tail section) ==="
cat -n apps/shelve/server/services/variable-groups.ts | sed -n '40,100p'
echo "=== index.delete.ts handler ==="
cat -n "apps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/[groupId]/index.delete.ts" | sed -n '1,120p'Repository: HugoRCD/shelve
Length of output: 4459
Make deleteGroup enforce projectId in the delete query (atomic scope)
VariableGroupsService.deleteGroup(id: number) deletes solely by variableGroups.id (no projectId), while the handler deletes via bare groupId after a separate getGroupForProject(...) read. That two-step, non-atomic flow weakens the project-scoped write contract.
File: apps/shelve/server/api/teams/[slug]/projects/[projectId]/variable-groups/[groupId]/index.delete.ts (lines 8-9)
await new VariableGroupsService().getGroupForProject(groupId, project.id)
await new VariableGroupsService().deleteGroup(groupId)Prefer a project-scoped delete API (e.g. deleteGroup(groupId, project.id) with WHERE id = ... AND projectId = ...) and call it directly.
🤖 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
`@apps/shelve/server/api/teams/`[slug]/projects/[projectId]/variable-groups/[groupId]/index.delete.ts
around lines 8 - 9, The handler currently does a separate read via
VariableGroupsService.getGroupForProject(groupId, project.id) then calls
VariableGroupsService.deleteGroup(groupId), which makes the delete non-atomic
and allows a delete by id only; change VariableGroupsService.deleteGroup to
accept both groupId and projectId (e.g., deleteGroup(groupId, projectId)) and
implement the DB delete to include WHERE id = ? AND projectId = ? so the write
is scoped to the project atomically, then update the handler in index.delete.ts
to call deleteGroup(groupId, project.id) directly (remove the separate
getGroupForProject call).
Comment on lines
+26
to
+31
| .where(and( | ||
| eq(schema.projects.id, input.id), | ||
| eq(schema.projects.teamId, input.teamId), | ||
| )) | ||
| .returning() | ||
| if (!updatedProject) throw createError({ statusCode: 422, message: 'Failed to update project' }) | ||
| if (!updatedProject) throw createError({ statusCode: 404, message: `Project not found with id ${input.id}` }) |
Contributor
There was a problem hiding this comment.
Scope the pre-update lookup to the caller's team.
updateProject() still resolves existingProject by bare id before this scoped UPDATE, so a cross-team caller can reach validation against another team's project before the final 404. Resolve the project with getProjectForTeam(input.id, input.teamId) first and validate uniqueness against input.teamId.
Suggested fix
async updateProject(input: ProjectUpdateInput): Promise<Project> {
- const existingProject = await this.getProject(input.id)
- if (!existingProject) throw createError({ statusCode: 404, message: `Project not found with id ${input.id}` })
+ const existingProject = await this.getProjectForTeam(input.id, input.teamId)
if (existingProject.name !== input.name)
- await this.validateProjectName(input.name, existingProject.teamId, input.id)
+ await this.validateProjectName(input.name, input.teamId, input.id)
const [updatedProject] = await db.update(schema.projects)🤖 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 `@apps/shelve/server/services/projects.ts` around lines 26 - 31, The pre-update
lookup in updateProject() currently resolves existingProject by id only; change
it to call getProjectForTeam(input.id, input.teamId) so the initial fetch is
scoped to the caller's team, then perform uniqueness checks (e.g., name
uniqueness) against input.teamId as well; finally keep the scoped UPDATE that
uses eq(schema.projects.id, input.id) and eq(schema.projects.teamId,
input.teamId) and preserve the 404 throw when the scoped update returns no row.
Comment on lines
+197
to
208
| async bulkAssignGroup(projectId: number, variableIds: number[], groupId: number | null): Promise<void> { | ||
| if (!variableIds.length) return | ||
|
|
||
| await db.update(schema.variables) | ||
| .set({ groupId }) | ||
| .where(inArray(schema.variables.id, variableIds)) | ||
| .where(and( | ||
| eq(schema.variables.projectId, projectId), | ||
| inArray(schema.variables.id, variableIds), | ||
| )) | ||
|
|
||
| const first = await db.query.variables.findFirst({ | ||
| where: eq(schema.variables.id, variableIds[0]!), | ||
| }) | ||
| if (first) await clearCache('Variables', first.projectId) | ||
| await clearCache('Variables', projectId) | ||
| } |
Contributor
There was a problem hiding this comment.
Reject partial bulk group assignments.
This now prevents cross-project writes, but it still returns success when part of variableIds is missing or belongs to another project. The batch ends up partially applied with no signal to the caller.
Suggested fix
async bulkAssignGroup(projectId: number, variableIds: number[], groupId: number | null): Promise<void> {
if (!variableIds.length) return
- await db.update(schema.variables)
+ const updated = await db.update(schema.variables)
.set({ groupId })
.where(and(
eq(schema.variables.projectId, projectId),
inArray(schema.variables.id, variableIds),
))
+ .returning({ id: schema.variables.id })
+
+ if (updated.length !== variableIds.length) {
+ throw createError({ statusCode: 404, statusMessage: 'One or more variables not found' })
+ }
await clearCache('Variables', projectId)
}🤖 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 `@apps/shelve/server/services/variables.ts` around lines 197 - 208, The
bulkAssignGroup function can silently perform a partial update when some
variableIds are missing or belong to another project; to fix, first validate
that the set of variableIds all belong to the given project (e.g., query
schema.variables where id IN variableIds and projectId = projectId and compare
counts or returned ids), and if the counts/ids don’t match throw an error
(abort) instead of performing the update; then perform the update (optionally
inside a transaction) and only call clearCache('Variables', projectId) after a
successful, validated update.
Comment on lines
+220
to
+235
| async deleteVariablesInProject(projectId: number, variableIds: number[]): Promise<void> { | ||
| if (!variableIds.length) return | ||
|
|
||
| const deleted = await db.delete(schema.variables) | ||
| .where(and( | ||
| eq(schema.variables.projectId, projectId), | ||
| inArray(schema.variables.id, variableIds), | ||
| )) | ||
| .returning() | ||
|
|
||
| if (deleted.length !== variableIds.length) { | ||
| throw createError({ statusCode: 404, statusMessage: 'One or more variables not found' }) | ||
| } | ||
|
|
||
| await clearCache('Variables', projectId) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't delete a subset before throwing 404.
This method validates the row count after the DELETE, so a mixed list can permanently remove the matching variables and still return an error. That turns a bad request into partial data loss.
Suggested fix
async deleteVariablesInProject(projectId: number, variableIds: number[]): Promise<void> {
if (!variableIds.length) return
- const deleted = await db.delete(schema.variables)
- .where(and(
- eq(schema.variables.projectId, projectId),
- inArray(schema.variables.id, variableIds),
- ))
- .returning()
-
- if (deleted.length !== variableIds.length) {
- throw createError({ statusCode: 404, statusMessage: 'One or more variables not found' })
- }
+ await db.transaction(async (tx) => {
+ const existing = await tx.query.variables.findMany({
+ columns: { id: true },
+ where: and(
+ eq(schema.variables.projectId, projectId),
+ inArray(schema.variables.id, variableIds),
+ ),
+ })
+
+ if (existing.length !== variableIds.length) {
+ throw createError({ statusCode: 404, statusMessage: 'One or more variables not found' })
+ }
+
+ await tx.delete(schema.variables)
+ .where(and(
+ eq(schema.variables.projectId, projectId),
+ inArray(schema.variables.id, variableIds),
+ ))
+ })
await clearCache('Variables', projectId)
}🤖 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 `@apps/shelve/server/services/variables.ts` around lines 220 - 235, The
deleteVariablesInProject function currently performs the DELETE then checks
deleted.length, causing possible partial deletes; instead first verify all
requested variableIds exist for the given project (e.g., query schema.variables
for ids where projectId = projectId and id IN variableIds and compare found
count/ids to variableIds), throw the 404 if any are missing, and only then
execute the DELETE and call clearCache('Variables', projectId); alternatively
wrap the existence-check + delete in a transaction to ensure atomicity.
Comment on lines
+19
to
+25
| test('returns the project when it belongs to the team', async () => { | ||
| const { ProjectsService } = await import('../../server/services/projects') | ||
| const service = new ProjectsService() | ||
| vi.spyOn(service, 'getProject').mockResolvedValue(projectOnA) | ||
|
|
||
| await expect(service.getProjectForTeam(projectOnA.id, 1)).resolves.toEqual(projectOnA) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider verifying the spy call arguments.
The test mocks getProject but doesn't verify it was called with the expected projectId. Adding an assertion would make the test more explicit about the interaction.
✨ Optional enhancement
await expect(service.getProjectForTeam(projectOnA.id, 1)).resolves.toEqual(projectOnA)
+expect(service.getProject).toHaveBeenCalledWith(projectOnA.id)🤖 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 `@apps/shelve/test/unit/team-project-context.test.ts` around lines 19 - 25, The
test should assert that the mocked getProject was called with the expected
project id to make the interaction explicit: capture the spy returned by
vi.spyOn(service, 'getProject') and after calling
service.getProjectForTeam(projectOnA.id, 1) add an expectation that the spy was
called with projectOnA.id (and optionally called once). Reference the
ProjectsService class, the getProject spy, the getProjectForTeam call, and the
projectOnA.id value when adding the assertion.
Comment on lines
+27
to
+35
| test('returns 404 when the project belongs to another team', async () => { | ||
| const { ProjectsService } = await import('../../server/services/projects') | ||
| const service = new ProjectsService() | ||
| vi.spyOn(service, 'getProject').mockResolvedValue(projectOnB) | ||
|
|
||
| await expect(service.getProjectForTeam(projectOnB.id, 1)).rejects.toMatchObject({ | ||
| statusCode: 404, | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider verifying the spy call arguments.
Similar to the first test, adding an assertion for the spy call would make the test interaction more explicit.
✨ Optional enhancement
await expect(service.getProjectForTeam(projectOnB.id, 1)).rejects.toMatchObject({
statusCode: 404,
})
+expect(service.getProject).toHaveBeenCalledWith(projectOnB.id)🤖 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 `@apps/shelve/test/unit/team-project-context.test.ts` around lines 27 - 35, The
test currently stubs ProjectsService.getProject but doesn't assert it was
called; add an assertion that the spy on service.getProject was invoked with the
expected project id (projectOnB.id) when calling
service.getProjectForTeam(projectOnB.id, 1), and optionally assert it was called
exactly once to make the interaction explicit; reference the ProjectsService
class, the getProject spy, the getProjectForTeam method, and the projectOnB
fixture when adding this check.
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
Test plan
pnpm test test/unit/team-project-context.test.ts(apps/shelve)Summary by CodeRabbit
Release Notes
Bug Fixes
Chores