fix: report campus selection set by security and update schema etc - #91
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 43 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds security item create/update validation and routes, wires the item detail page to persist edits, and updates campus/report-link handling plus related security navigation. ChangesSecurity item persistence
Campus report-link flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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
🧹 Nitpick comments (2)
backend/src/routes/items.ts (1)
616-620: 💤 Low valueConsider logging all updated fields in the audit trail.
The audit log currently includes
title,category, anddateFound, but notlocationFoundordescriptionInternal. For a complete audit trail, consider logging all fields that were updated, or at minimum addinglocationFoundanddescriptionInternalto thedetailsobject.📝 Suggested enhancement
details: { title, category, dateFound: dateFound.toISOString().slice(0, 10), + locationFound, + descriptionInternal, },🤖 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 `@backend/src/routes/items.ts` around lines 616 - 620, The audit trail details object in the items route is incomplete and only logs title, category, and dateFound fields. Add the missing locationFound and descriptionInternal fields to the details object to ensure the audit log captures all fields that were updated during the item update operation.backend/src/routes/reportLinks.ts (1)
234-266: ⚡ Quick winDocument the 400 validation error response.
The OpenAPI documentation should include the
400response that is returned whencampusIdcannot be determined (lines 284–296). This helps API consumers understand the complete contract.📝 Suggested documentation addition
* '404': * description: Campus not found + * '400': + * description: Validation error (e.g., campusId required) */🤖 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 `@backend/src/routes/reportLinks.ts` around lines 234 - 266, Add a 400 response definition to the OpenAPI documentation for the POST /api/report-links endpoint. In the responses section of the OpenAPI spec (after the existing 201, 401, 403, and 404 responses), include a 400 response with an appropriate description explaining that this error is returned when campusId cannot be determined (as referenced in lines 284-296 of the implementation). This documents the validation error case that API consumers need to handle.
🤖 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 `@backend/src/validators/items.ts`:
- Around line 12-16: The getTodayAtMidnight() function uses setHours(0, 0, 0, 0)
which sets midnight in the local timezone, but the dateFound transform at line
40 creates a Date at UTC midnight. This timezone mismatch causes incorrect
validation in non-UTC timezones. Fix getTodayAtMidnight() to use UTC midnight
instead by replacing the local timezone methods with UTC equivalents, such as
setUTCHours(0, 0, 0, 0) instead of setHours(0, 0, 0, 0), ensuring both the
transform and the refine comparison use the same UTC-based midnight.
In `@foundit-ui/app/security/items/`[itemId]/page.tsx:
- Line 18: The function `updateSecurityItem` is being imported but does not
exist in the module, causing a build failure. You need to implement and export
this function in the `@/lib/api/items` module. The function should accept an
`itemId` parameter (string) and a data object containing title, category,
dateFound, locationFound, and descriptionInternal fields. It should make a PATCH
request to the endpoint `/api/items/:itemId` and return a Promise that resolves
to a SecurityItemDetail object. Implement the function following the same
pattern as the existing `fetchSecurityItem` export in that module, then ensure
it is exported so the import statement can resolve successfully.
---
Nitpick comments:
In `@backend/src/routes/items.ts`:
- Around line 616-620: The audit trail details object in the items route is
incomplete and only logs title, category, and dateFound fields. Add the missing
locationFound and descriptionInternal fields to the details object to ensure the
audit log captures all fields that were updated during the item update
operation.
In `@backend/src/routes/reportLinks.ts`:
- Around line 234-266: Add a 400 response definition to the OpenAPI
documentation for the POST /api/report-links endpoint. In the responses section
of the OpenAPI spec (after the existing 201, 401, 403, and 404 responses),
include a 400 response with an appropriate description explaining that this
error is returned when campusId cannot be determined (as referenced in lines
284-296 of the implementation). This documents the validation error case that
API consumers need to handle.
🪄 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 Plus
Run ID: 7dee8f55-19b1-49f3-919d-e0b47b1d8e32
📒 Files selected for processing (5)
backend/src/routes/items.tsbackend/src/routes/reportLinks.tsbackend/src/validators/items.tsfoundit-ui/app/security/items/[itemId]/page.tsxfoundit-ui/app/security/qr/page.tsx
| function getTodayAtMidnight() { | ||
| const today = new Date(); | ||
| today.setHours(0, 0, 0, 0); | ||
| return today; | ||
| } |
There was a problem hiding this comment.
Timezone mismatch causes incorrect future-date validation.
getTodayAtMidnight() uses setHours(0, 0, 0, 0), which sets the time to midnight in the local timezone. However, the dateFound transform (line 40) creates a Date at UTC midnight: new Date(\${value}T00:00:00.000Z`)`. When the refine (line 41) compares these two values, it's comparing a UTC midnight Date against a local midnight Date.
Impact: In non-UTC timezones, this causes incorrect validation:
- Server in UTC+8: Local midnight is 8 hours before UTC midnight of the same calendar day. A valid "today" UTC date would be rejected as "future".
- Server in UTC-5: Local midnight is 5 hours after UTC midnight, allowing some actual future dates to pass validation.
🐛 Proposed fix: use UTC midnight
function getTodayAtMidnight() {
- const today = new Date();
- today.setHours(0, 0, 0, 0);
- return today;
+ const now = new Date();
+ return new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate()));
}📝 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.
| function getTodayAtMidnight() { | |
| const today = new Date(); | |
| today.setHours(0, 0, 0, 0); | |
| return today; | |
| } | |
| function getTodayAtMidnight() { | |
| const now = new Date(); | |
| return new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate())); | |
| } |
🤖 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 `@backend/src/validators/items.ts` around lines 12 - 16, The
getTodayAtMidnight() function uses setHours(0, 0, 0, 0) which sets midnight in
the local timezone, but the dateFound transform at line 40 creates a Date at UTC
midnight. This timezone mismatch causes incorrect validation in non-UTC
timezones. Fix getTodayAtMidnight() to use UTC midnight instead by replacing the
local timezone methods with UTC equivalents, such as setUTCHours(0, 0, 0, 0)
instead of setHours(0, 0, 0, 0), ensuring both the transform and the refine
comparison use the same UTC-based midnight.
| Textarea, | ||
| } from '@chakra-ui/react'; | ||
| import { fetchSecurityItem } from '@/lib/api/items'; | ||
| import { fetchSecurityItem, updateSecurityItem } from '@/lib/api/items'; |
There was a problem hiding this comment.
Missing updateSecurityItem export breaks the build.
The import references updateSecurityItem from @/lib/api/items, but the pipeline failure confirms this export does not exist:
Turbopack build failed: export updateSecurityItem doesn't exist in target module '`@/lib/api/items`'
The implementation of updateSecurityItem is missing from @/lib/api/items. This PR cannot be merged until the function is implemented and exported.
Expected implementation pattern
Based on the usage on lines 181-187, the function should accept itemId and a request body matching the backend's UpdateSecurityItemInput type:
export async function updateSecurityItem(
itemId: string,
data: {
title: string;
category: string;
dateFound: string; // YYYY-MM-DD format
locationFound: string | null;
descriptionInternal: string | null;
}
): Promise<SecurityItemDetail> {
// PATCH /api/items/:itemId implementation
// ...
}🧰 Tools
🪛 GitHub Actions: CI / 1_Frontend (Lint + Build).txt
[error] 18-18: Turbopack build failed: export 'updateSecurityItem' does not exist in '@/lib/api/items'. Import traces indicate this occurs in the page component for both Client Component Browser and Client Component SSR. Did you mean to import 'fetchSecurityItem'?
🪛 GitHub Actions: CI / Frontend (Lint + Build)
[error] 18-18: Turbopack build failed: export updateSecurityItem doesn't exist in target module '@/lib/api/items'. Import was: import { fetchSecurityItem, updateSecurityItem } from '@/lib/api/items'; Did you mean to import fetchSecurityItem?
🤖 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 `@foundit-ui/app/security/items/`[itemId]/page.tsx at line 18, The function
`updateSecurityItem` is being imported but does not exist in the module, causing
a build failure. You need to implement and export this function in the
`@/lib/api/items` module. The function should accept an `itemId` parameter
(string) and a data object containing title, category, dateFound, locationFound,
and descriptionInternal fields. It should make a PATCH request to the endpoint
`/api/items/:itemId` and return a Promise that resolves to a SecurityItemDetail
object. Implement the function following the same pattern as the existing
`fetchSecurityItem` export in that module, then ensure it is exported so the
import statement can resolve successfully.
Source: Pipeline failures
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/validators/items.ts (1)
37-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrim before enforcing non-empty constraints.
z.string().min(1).max(...).trim()lets whitespace-only input through and then trims it to"", so required fields liketitle,category,itemDescription, andlocationFoundcan end up empty. Swap to.trim().min(1)...in both item schemas.🤖 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 `@backend/src/validators/items.ts` around lines 37 - 38, The item validators are applying non-empty checks before trimming, which allows whitespace-only values to pass and then become empty strings. Update the relevant Zod fields in the item schemas in items validation so the `.trim()` call comes before `.min(1)` for `title`, `category`, `itemDescription`, and `locationFound`, using the existing schema definitions to locate and fix both item schemas 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 `@backend/src/routes/items.ts`:
- Around line 492-503: The item create/update flow in items routes currently
authorizes only by role and allows a security user to act on any submitted
campusId, which can cross campus boundaries. Update the relevant handlers in the
items route logic so security users are scoped to their authenticated campus_id
and can only create or patch items within that campus, while admin can continue
to use any campusId. Use the existing campus lookup and item update/create paths
to validate the requested campusId against the user context before proceeding,
and return an appropriate forbidden or not-found response when the campus does
not match.
- Around line 505-567: The audit write in the item creation flow is not atomic
because `writeAuditLog()` happens after `prisma.$transaction` has already
committed the item and images. Move the audit insertion into the same
transaction in `backend/src/routes/items.ts` (the create-item handler that uses
`tx.item.create`) so the item write and audit row succeed or fail together. If
that is not possible, make the post-commit `writeAuditLog()` failure non-fatal
and avoid returning an error for an already-created item. Apply the same fix to
the other item flow noted in the comment (the matching create/update handler
with the same `writeAuditLog` pattern).
In `@foundit-ui/components/Navbar.tsx`:
- Line 9: The navbar report link is now pointing to a route that does not exist,
so security users will hit a 404. Update the link target in Navbar to match the
actual report flow route used by the page at app/security/qr/page.tsx, or add
the missing /security/report-found page/redirect if that is the intended new
path. Keep the href generation and any related navigation labels in Navbar
aligned with the route names used by the report page component.
---
Outside diff comments:
In `@backend/src/validators/items.ts`:
- Around line 37-38: The item validators are applying non-empty checks before
trimming, which allows whitespace-only values to pass and then become empty
strings. Update the relevant Zod fields in the item schemas in items validation
so the `.trim()` call comes before `.min(1)` for `title`, `category`,
`itemDescription`, and `locationFound`, using the existing schema definitions to
locate and fix both item schemas 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 Plus
Run ID: 08e26ebc-c79b-42a7-a0c7-78f5882f3836
📒 Files selected for processing (5)
backend/src/routes/items.tsbackend/src/utils/itemHelpers.tsbackend/src/validators/items.tsbackend/src/validators/reportLinks.tsfoundit-ui/components/Navbar.tsx
| const campus = await prisma.campus.findUnique({ | ||
| where: { campusId }, | ||
| select: { campusId: true, retentionDays: true }, | ||
| }); | ||
|
|
||
| if (!campus) { | ||
| res.status(404).json({ | ||
| code: 'CAMPUS_NOT_FOUND', | ||
| message: 'Campus not found.', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce campus scope for security users.
These routes only check security/admin role. Since authenticated users carry campus_id, a security user can submit another campusId on create or patch any itemId from another campus.
Proposed fix
+ if (req.user!.role !== 'admin' && campusId !== req.user!.campus_id) {
+ res.status(403).json({
+ code: 'FORBIDDEN',
+ message: 'Cannot manage items for another campus.',
+ });
+ return;
+ }
+
const campus = await prisma.campus.findUnique({ if (!existing) {
res.status(404).json({
code: 'ITEM_NOT_FOUND',
message: 'Item not found.',
});
return;
}
+
+ if (
+ req.user!.role !== 'admin' &&
+ existing.campusId !== req.user!.campus_id
+ ) {
+ res.status(403).json({
+ code: 'FORBIDDEN',
+ message: 'Cannot manage items for another campus.',
+ });
+ return;
+ }Also applies to: 694-709
🤖 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 `@backend/src/routes/items.ts` around lines 492 - 503, The item create/update
flow in items routes currently authorizes only by role and allows a security
user to act on any submitted campusId, which can cross campus boundaries. Update
the relevant handlers in the items route logic so security users are scoped to
their authenticated campus_id and can only create or patch items within that
campus, while admin can continue to use any campusId. Use the existing campus
lookup and item update/create paths to validate the requested campusId against
the user context before proceeding, and return an appropriate forbidden or
not-found response when the campus does not match.
| const item = await prisma.$transaction(async (tx) => { | ||
| const created = await tx.item.create({ | ||
| data: { | ||
| campusId, | ||
| category, | ||
| title: itemTitleFromDescription(itemDescription, category), | ||
| descriptionInternal: buildDescriptionInternal(itemDescription), | ||
| locationFound, | ||
| dateFound, | ||
| status: ItemStatus.stored, | ||
| foundItemReportId: null, | ||
| registeredBy: req.user!.user_id, | ||
| retentionExpiryDate: computeRetentionExpiryDate( | ||
| dateFound, | ||
| campus.retentionDays | ||
| ), | ||
| }, | ||
| select: { | ||
| itemId: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (images.length > 0) { | ||
| await tx.itemImage.createMany({ | ||
| data: images.map((image) => ({ | ||
| itemId: created.itemId, | ||
| imageUrl: image.imageUrl, | ||
| uploadedBy: req.user!.user_id, | ||
| fileType: image.fileType, | ||
| fileSizeKb: image.fileSizeKb, | ||
| })), | ||
| }); | ||
| } | ||
|
|
||
| return created; | ||
| }); | ||
|
|
||
| const detail = await prisma.item.findUnique({ | ||
| where: { itemId: item.itemId }, | ||
| select: securityItemDetailSelect, | ||
| }); | ||
|
|
||
| if (!detail) { | ||
| res.status(500).json({ | ||
| code: 'INTERNAL_ERROR', | ||
| message: 'Item was created but could not be loaded.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| await writeAuditLog({ | ||
| actorId: req.user!.user_id, | ||
| action: 'item_created', | ||
| entityType: 'item', | ||
| entityId: detail.itemId, | ||
| details: { | ||
| title: detail.title, | ||
| category: detail.category, | ||
| campusId, | ||
| dateFound: dateFound.toISOString().slice(0, 10), | ||
| }, | ||
| ipAddress: req.ip, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep audit logging atomic with the item write.
writeAuditLog() runs after the item transaction/update has committed. If audit insertion fails, the API returns an error for an already-created item; clients may retry and create duplicates. Write the audit row in the same transaction, or make the post-commit audit failure explicitly non-fatal.
Also applies to: 731-762
🤖 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 `@backend/src/routes/items.ts` around lines 505 - 567, The audit write in the
item creation flow is not atomic because `writeAuditLog()` happens after
`prisma.$transaction` has already committed the item and images. Move the audit
insertion into the same transaction in `backend/src/routes/items.ts` (the
create-item handler that uses `tx.item.create`) so the item write and audit row
succeed or fail together. If that is not possible, make the post-commit
`writeAuditLog()` failure non-fatal and avoid returning an error for an
already-created item. Apply the same fix to the other item flow noted in the
comment (the matching create/update handler with the same `writeAuditLog`
pattern).
| * 'guest' — not logged in; no username, shows Login button. | ||
| * 'student' — authenticated student; shows Home, Found Items, My Claims + user dropdown. | ||
| * 'security' — authenticated security staff; shows Home, Items, Claims, QR/Link + user dropdown. | ||
| * 'security' — authenticated security staff; shows Home, Items, Claims, Report Found Item + user dropdown. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the navbar href aligned with the actual page route.
In the supplied UI layer, the report-link generator still lives at foundit-ui/app/security/qr/page.tsx, so this navbar change now points security users at a different URL. Unless the PR also adds a /security/report-found page or redirect, this entry will 404 and remove the main path into the flow.
Suggested fix
- { label: 'Report Found Item', href: '/security/report-found' },
+ { label: 'Report Found Item', href: '/security/qr' },Also applies to: 168-168
🤖 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 `@foundit-ui/components/Navbar.tsx` at line 9, The navbar report link is now
pointing to a route that does not exist, so security users will hit a 404.
Update the link target in Navbar to match the actual report flow route used by
the page at app/security/qr/page.tsx, or add the missing /security/report-found
page/redirect if that is the intended new path. Keep the href generation and any
related navigation labels in Navbar aligned with the route names used by the
report page component.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes