Conversation
… owners in E2E test suite
- Introduced detailed documentation for middleware functions including `authenticate`, `optionalAuthenticate`, `authorize`, and others, outlining their roles and associated routes. - Added a reference for BullMQ workers such as `media-processing`, `notification-delivery`, and `email-delivery`, detailing job payloads, processing steps, and failure handling. - Documented cron jobs like `listingExpiry`, `expiryWarning`, and `hardDeleteCleanup`, including their schedules, actions, and API impacts.
…hensive updates across multiple API docs
…ect plan, and service contracts
…contactRevealGate middleware
… comments in multiple files
- Removed unnecessary whitespace and comments in multiple files for cleaner code. - Consolidated error handling in the rate limiter and media processor. - Improved token extraction logic in the authentication middleware. - Enhanced photo upload service to handle display order more effectively. - Updated listing service to exclude processing photos from queries. - Refined validation schemas for listing creation and updates to ensure better data integrity. - Added side effect handling in the verification event worker for better notification and email processing. - Adjusted Redis client handling in the rate limiter to simplify connection closure logic.
POST /api/v1/students/:userId/photo — upload profile photo
DELETE /api/v1/students/:userId/photo — remove profile photo
Same two endpoints under /api/v1/pg-owners/:userId/photo
Storage path: profiles/{userId}/{userId}.webp (single canonical file per user, makes replace/purge trivial)
Reuses the existing upload multer middleware and storageService directly (no BullMQ queue — profile photos are small, synchronous sharp processing is fine)
On upload: compress → upload to storage → update profile_photo_url column → delete old blob if one existed
On delete: null out column → delete blob
…iple services and controllers
- Implemented `getRentIndex` controller to fetch rent index data based on city, locality, and room type. - Created `rentIndex.service.js` for querying the rent index table. - Added routes for rent index in `rentIndex.js` and integrated it into the main router. - Developed a cron job to refresh rent index data nightly based on recent rent observations. - Introduced roommate controller and service for managing roommate profiles, blocking/unblocking users, and fetching roommate feed. - Added validation schemas for rent index and roommate operations. - Enhanced listing service to include rent index data in listing responses.
…ent deviation calculation
…nformation on backend endpoints and frontend coverage issues
…on to report routes
…iables and security notes
…files - Cleaned up code by removing excessive blank lines and comments in various route, service, and validator files. - Improved readability and maintainability of the codebase by streamlining formatting. - Ensured consistent style across files, enhancing overall code quality.
…h-scale Fix compatibility ranking and saved-search scalability
Contributor
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
Rent index data and APIs migrations/006: Proper rent index.sql, src/cron/rentIndexRefresh.js, src/services/rentIndex.service.js, src/controllers/rentIndex.controller.js, src/routes/rentIndex.js, src/services/listing.service.js |
Adds rent_observations and rent_index, cron to recompute percentiles, public GET rent-index endpoint, and enriches listings with rent-index/deviation fields. |
Saved searches and alerts migrations/004savedsearches.SQL, migrations/010_saved_search_cap.sql, src/services/savedSearch.service.js, src/controllers/savedSearch.controller.js, src/routes/savedSearch.js, src/cron/savedSearchAlert.js, src/workers/notificationQueue.js |
Implements saved_searches table and enum, DB trigger to cap active saved searches, CRUD service/controllers/routes, alert cron that bulk-enqueues notifications, and bulk enqueue helper. |
Roommate feed and blocks migrations/005: Roommate matching support.sql, migrations/007_fix_roommate_constraints.sql, src/services/roommate.service.js, src/controllers/roommate.controller.js, src/routes/roommate.js, src/db/utils/roommateCompatibility.js, migrations/009_idx_listings_posted_by_status_city.sql |
Adds roommate opt-in fields and partial index, roommate_blocks table with constraints, feed service with compatibility scoring, block/unblock endpoints, and index tuning for city filters. |
Profile photos migrations/003:profile_photo_url_pg_owner_profiles.sql, src/services/profilePhoto.service.js, src/controllers/profilePhoto.controller.js, src/routes/student.js, src/routes/pgOwner.js, src/middleware/upload.js |
Adds profile photo upload/delete services for students and PG owners, image compression/upload flow, atomic DB updates capturing prior URL, controllers and route wiring, and upload validation. |
Listings enhancements src/services/listing.service.js, src/services/listingRenewal.service.js, src/controllers/listingRenewal.controller.js, src/controllers/listingAnalytics.controller.js, src/routes/listing.js, src/services/listingAnalytics.service.js |
Adds listing renewal service, analytics endpoint/service, compatibility cursoring and score in search, enum casting safety, photo placeholder exclusions, and cover/ordering refinements. |
Auth, cookies, CORS, optional auth src/middleware/authenticate.js, src/controllers/auth.controller.js, src/app.js, src/middleware/optionalAuthenticate.js |
Exports cookie option constants with prod-aware SameSite/secure, adjusts auth response shape (mobile vs browser), sets trust proxy and dynamic CORS origin callback, and adds optionalAuthenticate middleware. |
Email via Brevo API src/config/env.js, src/services/email.service.js, docs/deployment/* |
Adds brevo-api provider option, validates BREVO_API_KEY, and implements REST-based Brevo sending paths for OTP/verification emails. |
Notifications and verification outbox migrations/002_verification_event_outbox.sql, src/workers/notificationWorker.js, src/controllers/notification.controller.js, src/routes/notification.js |
Adds verification-event outbox table and trigger, extends notification templates for verification/saved-search alerts, consolidates notification controllers, and wires worker templates. |
Photos for listings src/services/photo.service.js, src/controllers/photo.controller.js, src/validators/photo.validators.js |
Enforces per-listing photo cap, excludes processing: placeholders from selection/counts, robust transactional enqueue/delete/cover/reorder semantics, and owner bypass option for listing photo retrieval. |
DB/infra, workers, CI, docs migrations/001_initial_schema.sql, src/db/client.js, src/workers/bullConnection.js, src/workers/notificationQueue.js, src/workers/emailQueue.js, .github/workflows/ci.yml, docs/Deployment.md, docs/deployment/*, .gitignore |
Adds DB extensions, makes DB pool max configurable, BullMQ Redis connection parsing, bulk enqueue helper, CI workflow, major deployment doc updates (Render/Neon/Upstash/Brevo), and .gitignore tweaks. |
🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
- sumit1642/roomies-backend#39 — Overlaps on listing compatibility/cursoring, saved-search alert cron, and saved-search cap migration.
- sumit1642/roomies-backend#7 — Touches verification controller/service flow and related handlers.
A rabbit taps deploy at three,
Cron-jobs hum like busy bees.
Roommates match and messages ping,
Saved searches wake to news they bring.
Rent-index charts the city’s tune—
Photos crisp, approvals soon. 🐇✨
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
tier0
sumit1642
added
documentation
![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: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/Deployment.md (2)
319-335:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove environment-specific identifiers from committed deployment docs.
Line 319, Line 335, Line 341, Line 394, and Line 744 include concrete infrastructure identifiers/email values. Keep only placeholders to avoid leaking operational details and personal identifiers in-repo.
Proposed doc-safe replacement pattern
-REDIS_URL=rediss://default:REDACTED@upward-mule-75729.upstash.io:6379 +REDIS_URL=rediss://default:YOUR_REDIS_PASSWORD@YOUR_UPSTASH_ENDPOINT:6379 -BREVO_SMTP_FROM=sumity1642@gmail.com +BREVO_SMTP_FROM=your-verified-sender@example.com -GOOGLE_CLIENT_ID=535680244018-qbhv8r4iufvlh4qrlcro2g1n4et5uvh2.apps.googleusercontent.com +GOOGLE_CLIENT_ID=your-google-client-id.apps.googleusercontent.comAlso applies to: 341-343, 394-395, 744-748
🤖 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 `@docs/Deployment.md` around lines 319 - 335, Replace all concrete infrastructure identifiers and personal data in the environment examples with neutral placeholders: redact values for REDIS_URL, JWT_SECRET, JWT_REFRESH_SECRET, AZURE_STORAGE_CONNECTION_STRING, AZURE_STORAGE_CONTAINER, BREVO_API_KEY, and BREVO_SMTP_FROM (and any other env vars shown) so they read like placeholder templates (e.g., REDIS_URL=rediss://default:REDACTED@<HOST>:6379, AZURE_STORAGE_CONNECTION_STRING=<AZURE_CONNECTION_STRING>, AZURE_STORAGE_CONTAINER=<CONTAINER_NAME>, BREVO_API_KEY=<BREVO_API_KEY>, BREVO_SMTP_FROM=<CONTACT_EMAIL>, JWT_SECRET=<JWT_SECRET>, etc.); ensure no real account names, hostnames, keys, or personal emails remain and add brief guidance that users must replace placeholders with their own credentials before deployment.
121-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnify Brevo transport guidance (SMTP vs API).
Line 121-Line 123 says SMTP, while Line 332-Line 335 and Line 402-Line 404 configure
EMAIL_PROVIDER=brevo-api. Keep one transport path throughout the guide to prevent deployment misconfiguration.Also applies to: 332-335, 402-404
🤖 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 `@docs/Deployment.md` around lines 121 - 123, The docs currently list Brevo as SMTP ("Brevo SMTP — free 300 emails/day" and "smtp-relay.brevo.com:587") while other sections set EMAIL_PROVIDER=brevo-api; unify to a single transport across the guide by choosing either SMTP or API and updating all occurrences accordingly (replace the SMTP headings/host info if you standardize on brevo-api, or change EMAIL_PROVIDER lines to brevo-smtp if you standardize on SMTP), and ensure any example env keys and example config snippets (the lines referencing smtp-relay.brevo.com:587 and EMAIL_PROVIDER=brevo-api) are consistent and mention the chosen transport explicitly.
🤖 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 @.github/workflows/ci.yml:
- Around line 3-11: Add a concurrency policy to the GitHub Actions workflow so
superseded runs are canceled; modify the top-level workflow (where "on:" is
declared) to include a "concurrency" block (or add it under the "check" job)
that sets a stable group name (e.g. using expressions like ${{ github.workflow
}}-${{ github.ref }} or ${{ github.ref_name }}) and "cancel-in-progress: true"
so repeated pushes to the "tier0" branch will cancel previous runs for the same
group.
- Around line 15-21: Replace the floating action tags with pinned commit SHAs
for both actions used (replace "actions/checkout@v4" and "actions/setup-node@v4"
with their respective full commit SHA refs) and enable persist-credentials:
false on the checkout step (refer to the checkout step block using
actions/checkout). Add a top-level concurrency key to the workflow to cancel
superseded runs, and add a minimal permissions map (e.g., read-only for contents
and workflows only as required) to the workflow YAML to enforce least privilege
(refer to the workflow root where concurrency and permissions belong). Ensure
the actions/setup-node step still supplies node-version and cache after
replacing the ref.
- Around line 14-15: The checkout step uses actions/checkout@v4 without
disabling credential persistence; update the checkout step that references
actions/checkout@v4 to include the persist-credentials: false option under that
step so the GITHUB_TOKEN is not written to local git config (i.e., add
persist-credentials: false to the same step that has uses: actions/checkout@v4).
In @.gitignore:
- Around line 4-12: Add a broad ignore for all .env variants to restore coverage
for files like .env.test by adding a rule such as ".env.*" to the .gitignore,
and add explicit exceptions for any template files you want committed (e.g. add
"! .env.example" and/or "!.env.template") so templates stay tracked while all
other .env.* files are ignored; update the existing entries that mention
".env.*.local" to the broader ".env.*" and ensure the exception lines are
present.
In `@docs/deployment/tier0.md`:
- Around line 9-12: The blockquote in the docs contains a stray empty quoted
line between the two quoted lines ("Live API base URL" and "Live health check")
causing markdownlint MD028; remove the empty blockquote line or make both lines
part of one continuous blockquote so the two lines are consecutively quoted
(locate the block containing "Live API base URL:
`https://roomies-api.onrender.com/api/v1`" and "Live health check:
`https://roomies-api.onrender.com/api/v1/health`" and fix the spacing).
- Around line 388-390: The fenced code block containing the Redis URL example
(`rediss://default:YOUR_UPSTASH_PASSWORD@YOUR-ENDPOINT.upstash.io:6379`) is
missing a language tag and triggers MD040; update the opening fence from ``` to
a fenced block with an explicit language (for example ```text or ```bash) so the
block is explicitly tagged and linter-compliant.
In `@migrations/006`: Proper rent index.sql:
- Around line 76-80: The UPDATE branch currently requires OLD.status <>
'active', which prevents active→active renewals from creating listing_renewed
observations; in the TG_OP = 'UPDATE' condition(s) referencing "NEW.status =
'active' AND OLD.status <> 'active' AND NEW.deleted_at IS NULL" remove the "AND
OLD.status <> 'active'" clause (apply the same change to the other matching
block) so any UPDATE that results in NEW.status = 'active' and NEW.deleted_at IS
NULL will insert the renewal observation.
In `@migrations/007_fix_roommate_constraints.sql`:
- Around line 28-38: The migration currently drops and recreates roommate_blocks
which destroys existing data; instead remove the DROP TABLE and implement a
non-destructive migration: if the table does not exist CREATE TABLE
roommate_blocks (...), but if it does exist use ALTER TABLE to ensure the two
columns blocker_id and blocked_id are UUID NOT NULL and have REFERENCES
users(user_id) ON DELETE CASCADE, add the composite PRIMARY KEY (blocker_id,
blocked_id) only if absent, and add the CONSTRAINT chk_no_self_block CHECK
(blocker_id <> blocked_id) only if absent; reference the roommate_blocks table,
the chk_no_self_block constraint, and the composite primary key in your ALTER
statements to apply the changes safely without deleting data.
In `@migrations/009_idx_listings_posted_by_status_city.sql`:
- Around line 11-13: The index idx_listings_posted_by_status_city on listings
currently keys the raw city column but the query uses LOWER(l.city) so the index
won’t be used; change the index to index the expression LOWER(city) (i.e.,
create an expression index on LOWER(city) together with posted_by and status and
keep the WHERE deleted_at IS NULL) so the planner can use it for queries that
filter with LOWER(city).
In `@src/app.js`:
- Around line 32-44: The origin callback in the CORS middleware (the origin:
(origin, callback) => { ... } function) should not call callback(new Error(...))
for disallowed origins because that surfaces as a 500 via your errorHandler;
instead return a graceful CORS denial by calling callback(null, false) (and
optionally log the blocked origin) so the CORS middleware rejects the request
without triggering the global error handler. Locate the origin callback in
src/app.js and replace the final callback(new Error(...)) branch with
callback(null, false) (or equivalent non-throwing refusal) while keeping the
existing allow checks intact.
In `@src/config/env.js`:
- Around line 221-227: The code only rejects BREVO_API_KEY values that start
with "xsmtpsib-" but allows any other invalid non-empty string; update the
validation around parsed.data.BREVO_API_KEY to enforce the expected prefix
"xkeysib-" (e.g., if BREVO_API_KEY is present and does not startWith("xkeysib-")
then log a clear error including the bad value and call process.exit(1)). Modify
the existing conditional that currently checks
parsed.data.BREVO_API_KEY?.startsWith("xsmtpsib-") to instead check for the
presence and correct prefix, update the error message to explain the required
"xkeysib-" prefix, and keep using process.exit(1) for failure.
In `@src/controllers/auth.controller.js`:
- Line 30: The response currently returns full token pairs (the tokens variable
and its accessToken/refreshToken fields) in JSON (e.g., res.status(...).json({
status: "success", data: tokens })); remove tokens from the JSON body and
instead return a sanitized payload (e.g., { status: "success", data: { userId,
tokenType: "bearer" } } or just { status: "success" }) while keeping the tokens
only in HttpOnly/secure cookies; update every occurrence that returns tokens
(references: the tokens variable and any accessToken/refreshToken properties in
the controller methods where res.json(...) is used) so the cookies remain the
sole transport for credentials unless an explicit API option requests
token-in-body behavior.
In `@src/cron/hardDeleteCleanup.js`:
- Around line 347-348: The current log call that logs { storageErr,
profile_photo_url: url } in the hardDeleteCleanup cron can leak sensitive query
tokens if a signed URL is stored; change the logging to redact the URL by
parsing the `url` value (e.g., new URL(url).pathname or extract the object key)
and log only the pathname/object key (no query string) alongside `storageErr`
and a clear message like "cron:hardDeleteCleanup — failed to delete profile
photo blob"; update the code around the existing log invocation so it constructs
a `redactedProfilePhoto` (or similar) and logs that instead of the full `url`.
In `@src/cron/savedSearchAlert.js`:
- Around line 9-16: Validate and clamp SEARCH_BATCH_SIZE and
NOTIFICATION_CHUNK_SIZE to safe positive integers on startup: parse env vars for
SAVED_SEARCH_ALERT_BATCH_SIZE and SAVED_SEARCH_ALERT_NOTIFICATION_CHUNK_SIZE,
treat non-numeric or <=0 values as defaults (e.g., 1 minimum) and optionally
enforce an upper bound, then use those sanitized constants (SEARCH_BATCH_SIZE,
NOTIFICATION_CHUNK_SIZE) in the rest of the module so the chunk(items, size)
loop and any batch queries cannot run with negative or zero sizes.
In `@src/db/client.js`:
- Around line 7-11: DB_POOL_MAX is taken from Number(process.env.DB_POOL_MAX)
and passed straight into new Pool({ max }) which allows negative or non-integer
values; validate and normalize it first: parse the env as an integer (e.g., use
parseInt/Number), ensure it's a finite integer >= 1 (use Number.isInteger and
clamp/floor as needed), and if invalid fall back to the default 10; then pass
that sanitized value into Pool (reference DB_POOL_MAX and the Pool creation in
this file and replace the raw value with the validated/sanitized variable).
In `@src/db/migrate.js`:
- Line 58: The filter currently only matches lowercase extensions (files =
entries.filter((f) => f.endsWith(".sql")).sort()), so files like .SQL are
skipped; update the filter used when building files (the entries.filter call
that assigns to files) to perform a case-insensitive extension check (e.g.,
convert f to lower/upper before endsWith or use a case-insensitive regex) so
.sql, .SQL, .Sql, etc. are included, then keep the .sort() behavior unchanged.
In `@src/db/utils/roommateCompatibility.js`:
- Around line 52-57: The SQL uses WHERE user_id = ANY($1::uuid[]) but the
parameters pass multiple positional values ([requestingUserId,
...candidateIds]), so bind a single array as $1 instead of multiple params:
construct an idsArray = [requestingUserId, ...candidateIds] and pass it as the
lone parameter (e.g., [idsArray]) to the query that contains `SELECT user_id,
COUNT(*)::int AS pref_count ... WHERE user_id = ANY($1::uuid[])`; update the
call site in src/db/utils/roommateCompatibility.js so $1 receives the uuid[]
array.
In `@src/middleware/requireAdmin.js`:
- Around line 9-13: The requireAdmin middleware currently treats a missing
authenticated user as a server error; change the AppError status code when
req.user is falsy from 500 to 401 (unauthenticated) so the call next(new
AppError("authenticate middleware must run before requireAdmin", 401)) correctly
signals an auth failure; leave the isEmailVerified check and its 403 response
as-is.
In `@src/routes/verification.js`:
- Line 18: The route currently accepts a client-controlled :userId
(verificationRouter.post("/:userId/submit", ... , vc.submitDocument)), which
allows IDOR; change the route to a self-submit endpoint without a path param
(e.g. "/submit") and update vc.submitDocument to derive ownership from the
authenticated principal (use req.user.userId instead of req.params.userId),
remove any code paths that read params.userId, and adjust any callers/tests to
the new route signature so only the authenticated user's ID is used for
submission.
In `@src/services/listingRenewal.service.js`:
- Around line 16-18: The UPDATE resets expires_at from NOW(), which shortens
active listings with a future expiry; change the expires_at assignment in the
UPDATE in listingRenewal.service.js to extend from the later of the current
expires_at and now (e.g., use GREATEST with a fallback for nulls such as
COALESCE(expires_at, NOW())) before adding the renewal interval so active
listings keep remaining time when renewed.
In `@src/services/profilePhoto.service.js`:
- Around line 39-55: The code currently reads oldUrl before uploading, which can
orphan blobs under concurrent requests; instead, perform the upload
(compressAndUpload) and then run the UPDATE query with RETURNING old_url (or
profile_photo_url) and use that returned value to delete the previous blob —
update the logic in the functions using oldUrl (refer to the pre-read variable
oldUrl and the DB update calls that follow) to delete the returned old_url from
the UPDATE response; apply the same pattern to the PG owner upload paths
mentioned (the other blocks at 64-72, 115-132, 140-148) so deletion always
operates on the DB-returned previous URL rather than the earlier pre-read value.
In `@src/services/rentIndex.service.js`:
- Line 11: The normalization of locality (variable normLocality) should treat
whitespace-only strings as null; update the logic where normLocality is assigned
(and the duplicate assignment around the other occurrence) to first trim
locality and then set normLocality = trimmed.length ? trimmed.toLowerCase() :
null so that values like " " become null instead of an empty string, ensuring
consistent API output.
In `@src/services/roommate.service.js`:
- Around line 212-223: In blockUser, remove the explicit await
client.query("ROLLBACK") calls before throwing on expected validation failures
(e.g., "User not found" and MAX_BLOCKS_PER_USER checks) so the catch block can
centrally handle rollback; locate the checks around the SELECT COUNT(*)::int AS
cnt query and the earlier user existence check and delete the pre-throw
ROLLBACKs (also apply the same change to the similar error path at the later
block around lines 234-239) to avoid double-rollback attempts—ensure the catch
still performs a single rollback if a transaction was started.
In `@src/validators/rentIndex.validators.js`:
- Around line 7-8: The city and locality string validators currently accept
whitespace-only values; update the Zod validators so inputs are trimmed and
empty-only strings are rejected: for city replace z.string({ error: "city is
required" }).min(1, { error: "city is required" }).max(100) with a trimmed
validator (e.g., z.string().trim().min(1, { error: "city is required"
}).max(100)) and for locality apply the same trim+non-empty rule while keeping
it optional (e.g., z.string().trim().min(1).max(100).optional()) so undefined is
allowed but whitespace-only values are rejected.
In `@src/validators/roommate.validators.js`:
- Around line 28-33: blockTargetParamsSchema currently allows userId and
targetUserId to be equal; add a schema-level refinement on
blockTargetParamsSchema (or on params) to reject requests where params.userId
=== params.targetUserId. Locate blockTargetParamsSchema and its params object
(userId, targetUserId) and add a .refine(...) that checks the two IDs differ,
returning a clear error message like "Cannot block yourself" when they match so
the validation fails before hitting DB constraints.
In `@src/workers/mediaProcessor.js`:
- Around line 101-127: When a job is permanently failed (the branch gated by
job.attemptsMade >= (job.opts.attempts ?? 1)), also remove any uploaded staging
file referenced by job.data.stagingPath to avoid disk accumulation: read
stagingPath from job.data (alongside photoId and listingId), and if present call
fs.promises.unlink (or fs.rm) to delete it, catching and logging ENOENT as a
non-error and any other errors with logger.error; perform this cleanup
regardless of whether the DB UPDATE in the existing try block succeeds (i.e.,
run after the DB attempt or in a finally-like path), and log success with
logger.warn (use photoId/listingId/stagingPath in logs) and failures with
logger.error so the file removal is observable.
In `@src/workers/notificationQueue.js`:
- Around line 30-33: The function enqueueNotificationsBulk currently accesses
payloads.length before validating payloads; add an upfront shape check at the
start of enqueueNotificationsBulk to guard against undefined/non-array inputs
(e.g., if (!Array.isArray(payloads) || payloads.length === 0) return 0) so the
code never reads .length on invalid values; keep this check before the try block
and preserve existing behavior for empty arrays.
---
Outside diff comments:
In `@docs/Deployment.md`:
- Around line 319-335: Replace all concrete infrastructure identifiers and
personal data in the environment examples with neutral placeholders: redact
values for REDIS_URL, JWT_SECRET, JWT_REFRESH_SECRET,
AZURE_STORAGE_CONNECTION_STRING, AZURE_STORAGE_CONTAINER, BREVO_API_KEY, and
BREVO_SMTP_FROM (and any other env vars shown) so they read like placeholder
templates (e.g., REDIS_URL=rediss://default:REDACTED@<HOST>:6379,
AZURE_STORAGE_CONNECTION_STRING=<AZURE_CONNECTION_STRING>,
AZURE_STORAGE_CONTAINER=<CONTAINER_NAME>, BREVO_API_KEY=<BREVO_API_KEY>,
BREVO_SMTP_FROM=<CONTACT_EMAIL>, JWT_SECRET=<JWT_SECRET>, etc.); ensure no real
account names, hostnames, keys, or personal emails remain and add brief guidance
that users must replace placeholders with their own credentials before
deployment.
- Around line 121-123: The docs currently list Brevo as SMTP ("Brevo SMTP — free
300 emails/day" and "smtp-relay.brevo.com:587") while other sections set
EMAIL_PROVIDER=brevo-api; unify to a single transport across the guide by
choosing either SMTP or API and updating all occurrences accordingly (replace
the SMTP headings/host info if you standardize on brevo-api, or change
EMAIL_PROVIDER lines to brevo-smtp if you standardize on SMTP), and ensure any
example env keys and example config snippets (the lines referencing
smtp-relay.brevo.com:587 and EMAIL_PROVIDER=brevo-api) are consistent and
mention the chosen transport explicitly.
🪄 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
Run ID: 8ad2a6d7-c20e-42a0-b43b-799dd8ac47f7
📒 Files selected for processing (148)
.github/workflows/ci.yml.gitignoreRoomies API — Full E2E Test Suite.postman_collection.jsonRoomies API — Postman Testing Master P.mddocs/API.mddocs/Deployment.mddocs/ImplementationPlan.mddocs/README.mddocs/TechStack.mddocs/api/auth.mddocs/api/connections.mddocs/api/conventions.mddocs/api/health.mddocs/api/interests.mddocs/api/listings.api.mddocs/api/notifications.mddocs/api/preferences.mddocs/api/profiles-and-contact.mddocs/api/properties.mddocs/api/ratings-and-reports.mddocs/deployment/tier0.mddocs/deployment/tier2.mdmigrations/001_initial_schema.sqlmigrations/002_verification_event_outbox.sqlmigrations/003:profile_photo_url_pg_owner_profiles.sqlmigrations/004savedsearches.SQLmigrations/005: Roommate matching support.sqlmigrations/006: Proper rent index.sqlmigrations/007_fix_roommate_constraints.sqlmigrations/008_fix_rent_index_redundant_index.sqlmigrations/009_idx_listings_posted_by_status_city.sqlmigrations/010_saved_search_cap.sqlpackage.jsonsrc/app.jssrc/cache/client.jssrc/config/constants.jssrc/config/env.jssrc/config/preferences.jssrc/controllers/auth.controller.jssrc/controllers/connection.controller.jssrc/controllers/interest.controller.jssrc/controllers/listing.controller.jssrc/controllers/listingAnalytics.controller.jssrc/controllers/listingRenewal.controller.jssrc/controllers/notification.controller.jssrc/controllers/pgOwner.controller.jssrc/controllers/photo.controller.jssrc/controllers/preferences.controller.jssrc/controllers/profilePhoto.controller.jssrc/controllers/property.controller.jssrc/controllers/rating.controller.jssrc/controllers/rentIndex.controller.jssrc/controllers/report.controller.jssrc/controllers/roommate.controller.jssrc/controllers/savedSearch.controller.jssrc/controllers/student.controller.jssrc/controllers/verification.controller.jssrc/cron/expiryWarning.jssrc/cron/hardDeleteCleanup.jssrc/cron/listingExpiry.jssrc/cron/rentIndexRefresh.jssrc/cron/savedSearchAlert.jssrc/db/client.jssrc/db/migrate.jssrc/db/seeds/amenities.jssrc/db/utils/auth.jssrc/db/utils/compatibility.jssrc/db/utils/institutions.jssrc/db/utils/pgOwner.jssrc/db/utils/roommateCompatibility.jssrc/db/utils/spatial.jssrc/logger/index.jssrc/middleware/authenticate.jssrc/middleware/authorize.jssrc/middleware/contactRevealGate.jssrc/middleware/errorHandler.jssrc/middleware/guestListingGate.jssrc/middleware/optionalAuthenticate.jssrc/middleware/rateLimiter.jssrc/middleware/requireAdmin.jssrc/middleware/upload.jssrc/middleware/validate.jssrc/routes/amenities.jssrc/routes/auth.jssrc/routes/connection.jssrc/routes/health.jssrc/routes/index.jssrc/routes/interest.jssrc/routes/listing.jssrc/routes/notification.jssrc/routes/pgOwner.jssrc/routes/preferences.jssrc/routes/property.jssrc/routes/rating.jssrc/routes/rentIndex.jssrc/routes/report.jssrc/routes/roommate.jssrc/routes/savedSearch.jssrc/routes/student.jssrc/routes/testUtils.jssrc/routes/verification.jssrc/server.jssrc/services/auth.service.jssrc/services/connection.service.jssrc/services/email.service.jssrc/services/interest.service.jssrc/services/listing.service.jssrc/services/listingAnalytics.service.jssrc/services/listingRenewal.service.jssrc/services/notification.service.jssrc/services/pgOwner.service.jssrc/services/photo.service.jssrc/services/preferences.service.jssrc/services/profilePhoto.service.jssrc/services/property.service.jssrc/services/rating.service.jssrc/services/rentIndex.service.jssrc/services/report.service.jssrc/services/roommate.service.jssrc/services/savedSearch.service.jssrc/services/student.service.jssrc/services/verification.service.jssrc/storage/adapters/azureBlob.jssrc/validators/auth.validators.jssrc/validators/connection.validators.jssrc/validators/interest.validators.jssrc/validators/listing.validators.jssrc/validators/notification.validators.jssrc/validators/pagination.validators.jssrc/validators/pgOwner.validators.jssrc/validators/photo.validators.jssrc/validators/preferences.validators.jssrc/validators/property.validators.jssrc/validators/rating.validators.jssrc/validators/rentIndex.validators.jssrc/validators/report.validators.jssrc/validators/roommate.validators.jssrc/validators/savedSearch.validators.jssrc/validators/student.validators.jssrc/validators/verification.validators.jssrc/workers/bullConnection.jssrc/workers/emailQueue.jssrc/workers/emailWorker.jssrc/workers/mediaProcessor.jssrc/workers/notificationQueue.jssrc/workers/notificationWorker.jssrc/workers/queue.jssrc/workers/verificationEventWorker.js
💤 Files with no reviewable changes (61)
- src/logger/index.js
- docs/api/interests.md
- docs/api/conventions.md
- docs/api/preferences.md
- docs/api/listings.api.md
- docs/api/profiles-and-contact.md
- docs/api/connections.md
- src/validators/rating.validators.js
- src/routes/property.js
- docs/README.md
- src/controllers/notification.controller.js
- docs/api/auth.md
- docs/api/health.md
- src/validators/student.validators.js
- src/services/preferences.service.js
- docs/api/properties.md
- src/routes/preferences.js
- docs/API.md
- docs/TechStack.md
- docs/api/notifications.md
- src/controllers/connection.controller.js
- src/validators/report.validators.js
- src/middleware/guestListingGate.js
- src/controllers/preferences.controller.js
- src/validators/connection.validators.js
- src/cache/client.js
- docs/api/ratings-and-reports.md
- src/validators/property.validators.js
- src/controllers/verification.controller.js
- src/middleware/validate.js
- src/services/student.service.js
- src/middleware/authorize.js
- src/routes/auth.js
- src/validators/verification.validators.js
- src/validators/auth.validators.js
- src/routes/health.js
- src/controllers/student.controller.js
- src/validators/preferences.validators.js
- src/validators/interest.validators.js
- src/validators/photo.validators.js
- Roomies API — Full E2E Test Suite.postman_collection.json
- src/storage/adapters/azureBlob.js
- src/routes/notification.js
- src/validators/notification.validators.js
- docs/ImplementationPlan.md
- src/middleware/optionalAuthenticate.js
- src/controllers/pgOwner.controller.js
- src/services/verification.service.js
- src/controllers/listing.controller.js
- src/controllers/property.controller.js
- src/middleware/errorHandler.js
- src/workers/bullConnection.js
- src/controllers/rating.controller.js
- src/cron/listingExpiry.js
- src/routes/connection.js
- src/routes/interest.js
- src/services/property.service.js
- src/routes/rating.js
- src/services/rating.service.js
- src/workers/emailWorker.js
- src/workers/emailQueue.js
Comment on lines
+212
to
+223
| await client.query("ROLLBACK"); | ||
| throw new AppError("User not found", 404); | ||
| } | ||
|
|
||
| // Soft cap — re-checked inside the transaction to prevent TOCTOU race. | ||
| const { rows: countRows } = await client.query( | ||
| `SELECT COUNT(*)::int AS cnt FROM roommate_blocks WHERE blocker_id = $1`, | ||
| [requestingUserId], | ||
| ); | ||
| if (countRows[0].cnt >= MAX_BLOCKS_PER_USER) { | ||
| await client.query("ROLLBACK"); | ||
| throw new AppError(`You can block at most ${MAX_BLOCKS_PER_USER} users`, 422); |
Contributor
There was a problem hiding this comment.
Avoid double rollback in blockUser error paths.
blockUser executes ROLLBACK before throwing on expected checks, then the catch executes ROLLBACK again. This can produce false rollback-failed logs and obscure real failures. Let the catch own rollback, or gate rollback on transaction state.
Suggested fix
- if (!targetRows.length) {
- await client.query("ROLLBACK");
- throw new AppError("User not found", 404);
- }
+ if (!targetRows.length) {
+ throw new AppError("User not found", 404);
+ }
...
- if (countRows[0].cnt >= MAX_BLOCKS_PER_USER) {
- await client.query("ROLLBACK");
- throw new AppError(`You can block at most ${MAX_BLOCKS_PER_USER} users`, 422);
- }
+ if (countRows[0].cnt >= MAX_BLOCKS_PER_USER) {
+ throw new AppError(`You can block at most ${MAX_BLOCKS_PER_USER} users`, 422);
+ }Also applies to: 234-239
🤖 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 `@src/services/roommate.service.js` around lines 212 - 223, In blockUser,
remove the explicit await client.query("ROLLBACK") calls before throwing on
expected validation failures (e.g., "User not found" and MAX_BLOCKS_PER_USER
checks) so the catch block can centrally handle rollback; locate the checks
around the SELECT COUNT(*)::int AS cnt query and the earlier user existence
check and delete the pre-throw ROLLBACKs (also apply the same change to the
similar error path at the later block around lines 234-239) to avoid
double-rollback attempts—ensure the catch still performs a single rollback if a
transaction was started.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…, response formatting, and code clarity
- Update rent observation trigger to capture renewals correctly. - Revise roommate_blocks constraints for safer data handling. - Create expression index for city filtering in roommate feed.
Fix Tier 0 data integrity, indexing, and service reliability
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
19-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin GitHub Actions to immutable SHAs instead of floating tags.
Line 19 and Line 24 still use mutable tags (
@v4), which weakens workflow integrity guarantees. Pin both actions to full commit SHAs.#!/bin/bash set -euo pipefail FILE=".github/workflows/ci.yml" echo "== Current action refs ==" rg -n 'uses:\s*actions/(checkout|setup-node)@' "$FILE" echo echo "== Pinned SHA refs (should match both actions after fix) ==" rg -nP 'uses:\s*actions/(checkout|setup-node)@[0-9a-f]{40}' "$FILE" || true🤖 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 @.github/workflows/ci.yml around lines 19 - 24, Replace the mutable action refs using actions/checkout@v4 and actions/setup-node@v4 with their corresponding immutable full 40-character commit SHAs: locate the two "uses: actions/checkout" and "uses: actions/setup-node" entries and substitute the tag (e.g., `@v4`) with the exact commit SHA for each action release so both actions are pinned to specific immutable SHAs.
🤖 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.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 19-24: Replace the mutable action refs using actions/checkout@v4
and actions/setup-node@v4 with their corresponding immutable full 40-character
commit SHAs: locate the two "uses: actions/checkout" and "uses:
actions/setup-node" entries and substitute the tag (e.g., `@v4`) with the exact
commit SHA for each action release so both actions are pinned to specific
immutable SHAs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc5dd37b-9519-4ad2-bd49-ccb7552d1f57
📒 Files selected for processing (19)
.github/workflows/ci.ymlmigrations/006: Proper rent index.sqlmigrations/007_fix_roommate_constraints.sqlmigrations/009_idx_listings_posted_by_status_city.sqlsrc/app.jssrc/controllers/auth.controller.jssrc/controllers/verification.controller.jssrc/cron/savedSearchAlert.jssrc/db/client.jssrc/db/utils/roommateCompatibility.jssrc/middleware/requireAdmin.jssrc/routes/verification.jssrc/services/listingRenewal.service.jssrc/services/profilePhoto.service.jssrc/services/rentIndex.service.jssrc/validators/rentIndex.validators.jssrc/validators/roommate.validators.jssrc/workers/mediaProcessor.jssrc/workers/notificationQueue.js
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
Impact
Validation
tier0againstmain: 51 commits ahead, 0 behind.npm test; local run did not start becausenode_modules/.bin/jestis missing in this checkout.Notes
Summary by CodeRabbit
New Features
Infrastructure
Documentation