Feat/onboarding#314
Open
KushagraJaiswar02 wants to merge 19 commits into
Open
Conversation
KushagraJaiswar02
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
Contributor
|
Warning Review limit reached
More reviews will be available in 53 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (64)
📝 WalkthroughWalkthroughIntroduces a server-persisted 3-step developer onboarding system (project → collection → first API call), encrypted secret key storage with a single-use reveal endpoint, capability-based plan enforcement middleware replacing ChangesOnboarding, Secret Keys, and Capability Gating
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ProtectedRoute
participant OnboardingWizard as Onboarding.jsx
participant DashboardAPI as dashboard-api
participant PublicAPI as public-api
participant Developer as Developer (MongoDB)
Browser->>ProtectedRoute: navigate /onboarding/project
ProtectedRoute->>ProtectedRoute: check onboarding.steps.projectCreated
ProtectedRoute-->>Browser: render OnboardingWizard step 1
Browser->>OnboardingWizard: submit project name
OnboardingWizard->>DashboardAPI: POST /api/projects
DashboardAPI->>Developer: save {<br/>secretKey: hash,<br/>secretKeyEncrypted: encrypt(raw)<br/>}
DashboardAPI-->>OnboardingWizard: { project, apiKeysLocked }
OnboardingWizard->>DashboardAPI: GET /api/auth/me (refreshUser)
OnboardingWizard-->>Browser: navigate /onboarding/collection
Browser->>OnboardingWizard: submit collection schema
OnboardingWizard->>DashboardAPI: POST /api/projects/:id/collections
DashboardAPI->>Developer: markDeveloperOnboardingStep(collectionCreated)
OnboardingWizard-->>Browser: navigate /onboarding/api
Browser->>OnboardingWizard: (api step loads)
OnboardingWizard->>DashboardAPI: POST /api/projects/:id/api-key (×2)
alt not verified
OnboardingWizard-->>Browser: show OTP modal
Browser->>DashboardAPI: POST /api/auth/verify-otp
DashboardAPI-->>OnboardingWizard: verified
end
Browser->>OnboardingWizard: click Test API
OnboardingWizard->>PublicAPI: GET /api/data/:collection (publishableKey)
PublicAPI->>Developer: markDeveloperActivated → firstApiCall step
PublicAPI->>Developer: create PlatformEvent(first_api_call)
OnboardingWizard->>DashboardAPI: PATCH /api/user/onboarding
OnboardingWizard-->>Browser: navigate /dashboard
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 16
🧹 Nitpick comments (5)
apps/dashboard-api/src/routes/webhooks.js (1)
29-29: 💤 Low valueDELETE route missing
checkWebhookGatemiddleware, inconsistent with other webhook mutation routes.The POST, PATCH, and test routes all include
checkWebhookGateaftercheckDeveloperCapability, but the DELETE route omits it. While allowing deletion when webhooks are disabled by plan might be intentional (users should be able to clean up), this inconsistency could be confusing.If intentional, consider adding a comment. Otherwise:
-router.delete("/:projectId/webhooks/:webhookId", authMiddleware, attachDeveloper, checkDeveloperCapability("manageWebhooks"), deleteWebhook); +router.delete("/:projectId/webhooks/:webhookId", authMiddleware, attachDeveloper, checkDeveloperCapability("manageWebhooks"), checkWebhookGate, deleteWebhook);🤖 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/dashboard-api/src/routes/webhooks.js` at line 29, The DELETE route for webhooks (deleteWebhook) is missing the checkWebhookGate middleware that is present in the POST and PATCH webhook mutation routes, creating an inconsistency. Add the checkWebhookGate middleware to the deleteWebhook route in the same position as the other routes (after checkDeveloperCapability), or if allowing deletion when webhooks are disabled by plan is intentional, add a clarifying comment above the route explaining why this route is deliberately excluded from the gate check.apps/public-api/src/__tests__/api_usage.test.js (1)
166-171: ⚡ Quick winTighten the first-api event assertion with an exact call count.
Adding
toHaveBeenCalledTimes(1)here helps catch duplicate-event regressions.🤖 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/public-api/src/__tests__/api_usage.test.js` around lines 166 - 171, The mockPlatformEventCreate assertion lacks an exact call count verification, which could miss regressions where the event is emitted multiple times. Add `.toHaveBeenCalledTimes(1)` to the mockPlatformEventCreate assertion chain to enforce that the platform event is created exactly once during the first API call.apps/dashboard-api/src/__tests__/routes.user.test.js (1)
3-5: ⚡ Quick winMake the auth middleware mock populate
req.user.These are authenticated-route tests; a pass-through middleware can hide regressions where handlers depend on
req.userand would fail in production.Suggested patch
jest.mock('../middlewares/authMiddleware', () => - jest.fn((_req, _res, next) => next()) + jest.fn((req, _res, next) => { + req.user = { _id: 'dev_1', email: 'dev@example.com' }; + next(); + }) );🤖 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/dashboard-api/src/__tests__/routes.user.test.js` around lines 3 - 5, The authMiddleware mock in the test file is currently a simple pass-through that only calls next() without populating req.user on the request object. Since these are authenticated-route tests, the mock should simulate real authentication behavior by setting req.user to a mock user object before calling next(). Modify the jest.mock callback function for authMiddleware to assign a user object to the req parameter before invoking next(), ensuring that route handlers depending on req.user are properly tested and any production regressions would be caught.apps/dashboard-api/src/__tests__/auth.controller.test.js (1)
215-217: ⚡ Quick winRemove transient debug logging from the register happy-path test.
The conditional
console.logblock adds noisy CI output and should be removed now that behavior is asserted explicitly.Suggested patch
- if (next.mock.calls.length > 0) { - console.log("NEXT CALLED WITH:", next.mock.calls[0][0]); - }🤖 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/dashboard-api/src/__tests__/auth.controller.test.js` around lines 215 - 217, Remove the debug logging code from the register happy-path test. Delete the entire conditional if block that checks `next.mock.calls.length > 0` and contains the `console.log("NEXT CALLED WITH:", ...)` statement, as this transient debug code adds unnecessary noise to CI output and the test behavior is already explicitly asserted elsewhere.apps/public-api/src/middlewares/verifyApiKey.js (1)
38-43: 💤 Low valueMinor: Redundant
$orcondition for secret keys.For secret keys (
sk_live_), both conditions in the$orarray resolve to{ secretKey: hashedApi }, making the$orunnecessary. This is functionally correct but slightly inefficient.Suggested optimization
if (!project) { - project = await Project.findOne({ - $or: [ - { [keyField]: isSecret ? hashedApi : apiKey }, - { [keyField]: hashedApi } - ] - }) + const query = isSecret + ? { [keyField]: hashedApi } + : { $or: [{ [keyField]: apiKey }, { [keyField]: hashedApi }] }; + project = await Project.findOne(query)Also applies to: 68-69
🤖 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/public-api/src/middlewares/verifyApiKey.js` around lines 38 - 43, The `$or` query in the Project.findOne call at the secret key lookup location contains a redundant condition - when isSecret is true, both conditions in the $or array resolve to { secretKey: hashedApi }, making the $or unnecessary. Optimize this by checking isSecret first: if true, directly query with { [keyField]: hashedApi } without the $or wrapper; if false, keep the existing $or logic with both apiKey and hashedApi conditions. Apply this same optimization at the other location where this pattern appears (the additional site mentioned in the review).
🤖 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/consumer/src/index.js`:
- Around line 21-23: The port assignment logic (lines 21-23) does not validate
that the port number falls within the valid port range of 1-65535. Add
validation after the Number() conversions to check that the resulting port value
is within this valid range. If the port is outside the valid range, either use a
safe default value or throw an appropriate error to prevent runtime failures
when the server attempts to bind.
In `@apps/dashboard-api/src/controllers/auth.controller.js`:
- Around line 481-491: Remove raw OTP value logging from all email error
fallback paths to prevent production credential leakage. In the sendOtp error
handler around lines 481-491, remove the otp variable from the console.log
statement that logs "OTP code for ${email} is: ${otp}" - log only a generic
message without the actual OTP. Apply the identical fix to the forgot-password
error handler around lines 537-547 to ensure OTP values are never exposed in
logs regardless of error conditions or environment flags.
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 2928-2961: The revealSecretKey function's error responses are
missing the required data field per API guidelines. All error responses in the
function (the 404 for project not found, 403 for unverified account, 400 for
already revealed key, 404 for decryption failure, and the 500 catch block error)
must include an empty data object field alongside success and message to conform
to the standard API response format of { success: bool, data: {}, message: "" }.
Add data: {} to each error response JSON object to match the successful response
format already present in the endpoint.
In `@apps/dashboard-api/src/middlewares/planEnforcement.js`:
- Around line 41-86: The checkDeveloperCapability middleware in
planEnforcement.js does not handle the manageWebhooks capability that is called
from webhook routes, causing all unverified users to fail with a generic 403
error on the final fallback. You need to either add explicit handling for the
manageWebhooks capability inside the function (similar to the existing
createProject and createCollection handlers) if unverified users should have
limited webhook permissions, or remove the redundant
checkDeveloperCapability("manageWebhooks") call from webhook routes if the
existing checkWebhookGate middleware already enforces the required verification.
Determine which approach aligns with your webhook access policy and implement
accordingly.
In `@apps/public-api/src/__tests__/api_usage.test.js`:
- Around line 39-43: The module-scoped variable `finishCallback` is not being
reset in the `beforeEach` hook, causing stale callback listeners from previous
tests to persist and interfere with subsequent tests. Add a line in the
`beforeEach` function to reset `finishCallback` to its initial state (likely
null or undefined) alongside the other mock resets like jest.clearAllMocks() and
mockRedisSet.mockResolvedValue calls, ensuring each test starts with a clean
callback state.
In `@apps/public-api/src/middlewares/api_usage.js`:
- Around line 87-92: The markDeveloperActivated function returns { activated:
true } on every subsequent call (not just the first), causing duplicate
first_api_call events to be created in the PlatformEvent collection. Fix this by
implementing idempotency: either add a database uniqueness constraint on
(developerId, event) in the PlatformEvent collection to prevent duplicates, or
modify the markDeveloperActivated function logic to check if the event already
exists before creating a new PlatformEvent record, ensuring it only creates the
event once per developer.
In `@apps/web-dashboard/src/components/ProtectedRoute.jsx`:
- Around line 5-8: The ProtectedRoute component has two issues: the
allowUnverified prop defaults to true, which makes verification optional rather
than required by default, and the component does not consume the
allowIncompleteOnboarding and onboardingOnly props that are being passed to it
from App.jsx. Fix this by changing the allowUnverified default value from true
to false to make verification required by default, add allowIncompleteOnboarding
and onboardingOnly to the component's destructuring parameters, and implement
the logic in the verification/routing section (lines 32-70) to properly check
these conditions and gate access accordingly based on whether users are verified
and have completed onboarding as specified by the passed props.
In `@apps/web-dashboard/src/pages/Onboarding.jsx`:
- Around line 112-122: The fetchKeys function in Onboarding.jsx is being called
from multiple locations (lines 136-141, 149-157, and 289-294), and after OTP
verification completes at line 289, the useEffect dependency at line 149-157
also triggers fetchKeys, causing redundant API calls that create unnecessary
write operations on credentials. Add a guard condition in the fetchKeys function
(lines 112-122) to prevent it from making API calls if keys have already been
successfully fetched, or add a flag to track whether a fetch is already in
progress and skip subsequent calls. Alternatively, restructure the callsites
(specifically the effect at lines 149-157 and the verification handler at lines
289-294) to ensure fetchKeys is only invoked once after the verification flow
completes, removing the duplicate trigger from the effect.
- Around line 93-106: The onboarding flow currently expects unwrapped API
responses by accessing properties directly on res.data (e.g., res.data._id,
res.data.collections), but the documented coding standard requires wrapped
responses in the format { success: bool, data: {}, message: "" }. Update all API
response handling in Onboarding.jsx at the following locations to expect wrapped
responses: lines 93-106 where GET /api/projects is called and
res.data.collections is accessed, lines 170-175 where POST /api/projects is
called and res.data._id is accessed, and lines 308-312 where GET
/api/projects/:projectId is called and res.data.collections is accessed. Change
these to access res.data.data instead of res.data when retrieving project and
collection properties. Alternatively, if these endpoints are intentionally
exempt from the standard wrapper format, confirm this decision and document the
exception.
In `@apps/web-dashboard/src/pages/ProjectDetails.jsx`:
- Around line 92-120: The revealKeys async function within the useEffect is
regenerating live API keys using the /api-key endpoint based on a URL parameter,
which is insecure and non-idempotent. Replace the two POST calls to
/api/projects/{projectId}/api-key with calls to the proper /reveal-secret-key
endpoint that is designed for safely revealing keys in a one-time manner, and
adjust the response parsing accordingly to match the structure returned by that
endpoint instead of the key generation endpoint.
In `@apps/web-dashboard/src/pages/Storage.jsx`:
- Around line 197-200: The success toast "Storage bucket initialized!" in the
onClick handler is being shown immediately when the button is clicked, before
any actual storage operation completes. This is misleading because users can
cancel the file picker and still see the success message. Remove the
toast.success call from the onClick handler in the button that triggers
fileInputRef.current.click(), and instead ensure the success toast only appears
after the file upload or storage operation actually completes successfully
(likely in the file input change handler or upload completion callback).
In `@apps/web-dashboard/src/utils/quota.js`:
- Line 8: The return statement that calculates the percentage using Math.min
currently only clamps the value to a maximum of 100, but allows negative values
to be returned when numericUsed is negative. This causes negative CSS widths in
progress bars. Fix this by adding a lower bound clamp using Math.max to ensure
the percentage is never less than 0, while keeping the existing upper bound
clamp of 100. Wrap the Math.min expression inside a Math.max call that
establishes 0 as the minimum value.
In `@packages/common/src/utils/emailService.js`:
- Line 80: The AppError exceptions being thrown for email provider failures are
using status code 400 (client error), but these are upstream infrastructure
failures that should return 5xx (server error) status codes instead. Change the
status code from 400 to 502 (Bad Gateway) in the AppError constructor calls at
all affected locations in the emailService.js file: line 80, line 157, line 251,
line 302, and line 379. This ensures that failures from the external email
provider are correctly classified as server-side issues rather than client input
errors, allowing proper retry logic and error handling downstream.
- Around line 8-11: Remove the console.log statements that print the
RESEND_API_KEY and RESEND_API_KEY_2 environment variables directly to logs, as
this exposes sensitive credentials. Delete lines 10 and 11 that log
process.env.RESEND_API_KEY and process.env.RESEND_API_KEY_2, or replace them
with masked versions that only log partial key information (e.g., first 4 and
last 4 characters) if debugging is necessary.
In `@scratch/list_developers.js`:
- Around line 7-15: The list function does not guarantee that the database
connection is closed if an error occurs during the Developer.find({}) operation
or when processing the results. Wrap the database operations (the find call and
forEach loop) in a try-finally block, moving the mongoose.connection.close()
call to the finally block to ensure it executes regardless of whether an error
is thrown. This same pattern should be applied to any other similar database
query functions in the file that have the same vulnerability.
In `@scratch/test_signup.js`:
- Around line 25-34: This scratch script contains multiple sites leaking
sensitive data in logs. At scratch/test_signup.js lines 25-34, the cookie
function logs raw cookie values (which may contain session tokens) and the json
function logs full response data (which may contain PII/tokens). Redact these
values before logging by extracting and masking sensitive fields like tokens,
sessions, or passwords, or replace actual values with placeholders like
[REDACTED]. At scratch/list_developers.js lines 10-13, raw email addresses are
being logged directly. Replace direct email output with masked email addresses
(e.g., user***`@example.com`) or log only aggregate counts of developers instead
of individual identifiable information.
---
Nitpick comments:
In `@apps/dashboard-api/src/__tests__/auth.controller.test.js`:
- Around line 215-217: Remove the debug logging code from the register
happy-path test. Delete the entire conditional if block that checks
`next.mock.calls.length > 0` and contains the `console.log("NEXT CALLED WITH:",
...)` statement, as this transient debug code adds unnecessary noise to CI
output and the test behavior is already explicitly asserted elsewhere.
In `@apps/dashboard-api/src/__tests__/routes.user.test.js`:
- Around line 3-5: The authMiddleware mock in the test file is currently a
simple pass-through that only calls next() without populating req.user on the
request object. Since these are authenticated-route tests, the mock should
simulate real authentication behavior by setting req.user to a mock user object
before calling next(). Modify the jest.mock callback function for authMiddleware
to assign a user object to the req parameter before invoking next(), ensuring
that route handlers depending on req.user are properly tested and any production
regressions would be caught.
In `@apps/dashboard-api/src/routes/webhooks.js`:
- Line 29: The DELETE route for webhooks (deleteWebhook) is missing the
checkWebhookGate middleware that is present in the POST and PATCH webhook
mutation routes, creating an inconsistency. Add the checkWebhookGate middleware
to the deleteWebhook route in the same position as the other routes (after
checkDeveloperCapability), or if allowing deletion when webhooks are disabled by
plan is intentional, add a clarifying comment above the route explaining why
this route is deliberately excluded from the gate check.
In `@apps/public-api/src/__tests__/api_usage.test.js`:
- Around line 166-171: The mockPlatformEventCreate assertion lacks an exact call
count verification, which could miss regressions where the event is emitted
multiple times. Add `.toHaveBeenCalledTimes(1)` to the mockPlatformEventCreate
assertion chain to enforce that the platform event is created exactly once
during the first API call.
In `@apps/public-api/src/middlewares/verifyApiKey.js`:
- Around line 38-43: The `$or` query in the Project.findOne call at the secret
key lookup location contains a redundant condition - when isSecret is true, both
conditions in the $or array resolve to { secretKey: hashedApi }, making the $or
unnecessary. Optimize this by checking isSecret first: if true, directly query
with { [keyField]: hashedApi } without the $or wrapper; if false, keep the
existing $or logic with both apiKey and hashedApi conditions. Apply this same
optimization at the other location where this pattern appears (the additional
site mentioned in the review).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f2c1b43-d2a8-4ef6-b917-1405fd25c57d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (62)
apps/consumer/src/index.jsapps/dashboard-api/src/__tests__/auth.controller.test.jsapps/dashboard-api/src/__tests__/onboarding.common.test.jsapps/dashboard-api/src/__tests__/planEnforcement.capabilities.test.jsapps/dashboard-api/src/__tests__/routes.projects.storage.test.jsapps/dashboard-api/src/__tests__/routes.user.test.jsapps/dashboard-api/src/app.jsapps/dashboard-api/src/controllers/admin.metrics.controller.jsapps/dashboard-api/src/controllers/analytics.controller.jsapps/dashboard-api/src/controllers/auth.controller.jsapps/dashboard-api/src/controllers/project.controller.jsapps/dashboard-api/src/middlewares/planEnforcement.jsapps/dashboard-api/src/routes/projects.jsapps/dashboard-api/src/routes/user.jsapps/dashboard-api/src/routes/webhooks.jsapps/public-api/src/__tests__/api_usage.test.jsapps/public-api/src/middlewares/api_usage.jsapps/public-api/src/middlewares/verifyApiKey.jsapps/web-dashboard/src/App.jsxapps/web-dashboard/src/components/Dashboard/UsageProgressBar.jsxapps/web-dashboard/src/components/Layout/Sidebar.jsxapps/web-dashboard/src/components/Onboarding/OnboardingChecklist.jsxapps/web-dashboard/src/components/ProtectedRoute.jsxapps/web-dashboard/src/components/ThemeToggle.jsxapps/web-dashboard/src/constants/onboarding.jsapps/web-dashboard/src/context/AuthContext.jsxapps/web-dashboard/src/context/OnboardingContext.jsxapps/web-dashboard/src/pages/AdminMetrics.jsxapps/web-dashboard/src/pages/Analytics.jsxapps/web-dashboard/src/pages/Auth.jsxapps/web-dashboard/src/pages/CreateCollection.jsxapps/web-dashboard/src/pages/CreateProject.jsxapps/web-dashboard/src/pages/Dashboard.jsxapps/web-dashboard/src/pages/Database.jsxapps/web-dashboard/src/pages/ForgotPassword.jsxapps/web-dashboard/src/pages/Login.jsxapps/web-dashboard/src/pages/MailPlatform.jsxapps/web-dashboard/src/pages/Onboarding.jsxapps/web-dashboard/src/pages/OtpVerification.jsxapps/web-dashboard/src/pages/ProjectDetails.jsxapps/web-dashboard/src/pages/RequestPro.jsxapps/web-dashboard/src/pages/Settings.jsxapps/web-dashboard/src/pages/Signup.jsxapps/web-dashboard/src/pages/Storage.jsxapps/web-dashboard/src/pages/Webhooks.jsxapps/web-dashboard/src/utils/api.jsapps/web-dashboard/src/utils/authRedirect.jsapps/web-dashboard/src/utils/quota.jspackages/common/src/index.jspackages/common/src/models/Developer.jspackages/common/src/models/PlatformEvent.jspackages/common/src/models/Project.jspackages/common/src/utils/AppError.jspackages/common/src/utils/emailService.jspackages/common/src/utils/input.validation.jspackages/common/src/utils/onboarding.jsscratch/check_common.jsscratch/list_developers.jsscratch/test_login_schema.jsscratch/test_signup.jsscratch/test_zod.jssdks/urbackend-react/tests/context.test.tsx
💤 Files with no reviewable changes (1)
- apps/web-dashboard/src/pages/CreateCollection.jsx
Comment on lines
+41
to
+86
| exports.checkDeveloperCapability = function(capability) { | ||
| return async function(req, res, next) { | ||
| const { Project, AppError, sanitizeObjectId } = require('@urbackend/common'); | ||
| try { | ||
| if (isAdminRequest(req)) { | ||
| return next(); | ||
| } | ||
|
|
||
| const isVerified = !!(req.developer?.isVerified ?? req.user?.isVerified); | ||
|
|
||
| if (isVerified) { | ||
| return next(); | ||
| } | ||
|
|
||
| if (capability === 'createProject') { | ||
| const currentCount = await Project.countDocuments({ owner: req.user._id }); | ||
| if (currentCount >= UNVERIFIED_PROJECT_LIMIT) { | ||
| return next(new AppError(403, 'Verify your email to create additional projects.')); | ||
| } | ||
| req.projectLimit = Math.min(req.projectLimit ?? UNVERIFIED_PROJECT_LIMIT, UNVERIFIED_PROJECT_LIMIT); | ||
| return next(); | ||
| } | ||
|
|
||
| if (capability === 'createCollection') { | ||
| const cleanProjectId = sanitizeObjectId(req.body.projectId || req.params.projectId); | ||
| if (!cleanProjectId) return next(new AppError(400, 'Invalid or missing projectId')); | ||
|
|
||
| const project = await Project.findOne({ _id: cleanProjectId, owner: req.user._id }) | ||
| .select('collections') | ||
| .lean(); | ||
| if (!project) return next(new AppError(404, 'Project not found')); | ||
|
|
||
| if ((project.collections || []).length >= UNVERIFIED_COLLECTION_LIMIT) { | ||
| return next(new AppError(403, 'Verify your email to create more than 3 collections.')); | ||
| } | ||
|
|
||
| req.collectionLimit = Math.min(req.collectionLimit ?? UNVERIFIED_COLLECTION_LIMIT, UNVERIFIED_COLLECTION_LIMIT); | ||
| return next(); | ||
| } | ||
|
|
||
| return next(new AppError(403, 'Email verification is required for this action.')); | ||
| } catch (err) { | ||
| next(err); | ||
| } | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing handler for manageWebhooks capability causes unverified users to be blocked unconditionally.
The checkDeveloperCapability middleware only handles createProject and createCollection capabilities. However, webhooks.js routes use checkDeveloperCapability("manageWebhooks"). For unverified users, this falls through to line 81, returning a generic 403 error regardless of any limit checks.
If the intent is to block all unverified users from webhooks, checkWebhookGate (lines 163-171) already does this. This makes checkDeveloperCapability("manageWebhooks") redundant. If the intent is to allow unverified users limited webhook access, you need to add a handler.
🛠️ Suggested fix: Either remove the redundant capability check from webhook routes, or add explicit handling
Option A: Add explicit handling if webhooks should have unverified limits
return next();
}
+ if (capability === 'manageWebhooks') {
+ // Webhooks require verification - delegate to checkWebhookGate
+ return next();
+ }
+
return next(new AppError(403, 'Email verification is required for this action.'));Option B: Document that unhandled capabilities require verification by design
+ // Any unrecognized capability requires email verification
return next(new AppError(403, 'Email verification is required for this action.'));🤖 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/dashboard-api/src/middlewares/planEnforcement.js` around lines 41 - 86,
The checkDeveloperCapability middleware in planEnforcement.js does not handle
the manageWebhooks capability that is called from webhook routes, causing all
unverified users to fail with a generic 403 error on the final fallback. You
need to either add explicit handling for the manageWebhooks capability inside
the function (similar to the existing createProject and createCollection
handlers) if unverified users should have limited webhook permissions, or remove
the redundant checkDeveloperCapability("manageWebhooks") call from webhook
routes if the existing checkWebhookGate middleware already enforces the required
verification. Determine which approach aligns with your webhook access policy
and implement accordingly.
Collaborator
|
@KushagraJaiswar02 resolve all coderabbit issues then merge |
Member
|
Why there are soo many conflicts?? @coderabbitai |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 314 --repo geturbackend/urBackend --json title,baseRefName,headRefName,mergeable,mergeStateStatus,conflicts,state,updatedAtLength of output: 1060 🏁 Script executed: #!/bin/bash
gh pr view 314 --repo geturbackend/urBackend --json title,baseRefName,headRefName,mergeable,mergeStateStatus,state,updatedAt,commitsLength of output: 1596
To resolve, # Option 1: Merge main into the feature branch
git checkout feat/onboarding
git fetch origin
git merge origin/main
# Resolve conflicts, then commit
# Option 2: Rebase onto main (cleaner history)
git checkout feat/onboarding
git fetch origin
git rebase origin/main
# Resolve conflicts at each step, then git rebase --continueAfter resolving all conflicts, push the branch and the PR will become mergeable. It's also worth resolving the CodeRabbit review comments (as Nitin mentioned) before merging. (◕ᴗ◕✿)🐇 |
…e black/green theme
…rve collection name
…stApiCall in DB on success
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/dashboard-api/src/controllers/project.controller.js (2)
2943-2968:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake secret-key reveal atomically single-use.
Two concurrent requests can both read
secretKeyRevealed === falsebefore either save completes, so the “single-use” key can be revealed twice. Claim the reveal with an atomicfindOneAndUpdatepredicate before decrypting/returning the key.Proposed fix
- const project = await Project.findOne({ - _id: req.params.projectId, - owner: req.user._id, - }).select("+secretKeyEncrypted secretKeyRevealed"); + const project = await Project.findOneAndUpdate( + { + _id: req.params.projectId, + owner: req.user._id, + secretKeyRevealed: { $ne: true }, + "secretKeyEncrypted.encrypted": { $exists: true, $ne: null }, + }, + { $set: { secretKeyRevealed: true } }, + { new: false }, + ).select("+secretKeyEncrypted secretKeyRevealed"); if (!project) { - return res.status(404).json({ success: false, data: {}, message: "Project not found." }); - } - - if (!req.user.isVerified) { - return res.status(403).json({ success: false, data: {}, message: "Account verification required to reveal secret key." }); - } - - if (project.secretKeyRevealed) { - return res.status(400).json({ success: false, data: {}, message: "Secret key has already been revealed once." }); - } - - if (!project.secretKeyEncrypted || !project.secretKeyEncrypted.encrypted) { - return res.status(404).json({ success: false, data: {}, message: "Secret key cannot be decrypted (it may have been rolled or cleared)." }); + return res.status(404).json({ success: false, data: {}, message: "Secret key is not available for reveal." }); } const decryptedKey = decrypt(project.secretKeyEncrypted); project.secretKeyRevealed = 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 `@apps/dashboard-api/src/controllers/project.controller.js` around lines 2943 - 2968, The secret key reveal endpoint has a race condition where two concurrent requests can both read secretKeyRevealed as false before either completes the save operation. Replace the separate Project.findOne and project.save calls with a single atomic findOneAndUpdate operation that includes a predicate checking secretKeyRevealed is false. Use the predicate in the query filter to ensure the update only succeeds if secretKeyRevealed is still false at the time of update, making the reveal operation truly single-use by preventing the race condition that allows concurrent requests to both succeed.
474-480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the intended 403 instead of converting
AppErrorto 500.The new
AppError(403)is caught by the localcatch, which always sends a 500 with{ error }. Return a structured 403 here or pass the error to Express error middleware.As per coding guidelines, “All API endpoints must return response format:
{ success: bool, data: {}, message: "" }. Use AppError class for errors.”Proposed fix
if (!getProjectRole(projectObj, req.user._id)) { - throw new AppError(403, "Access denied."); + return res.status(403).json({ success: false, data: {}, message: "Access denied." }); } res.json(sanitizeProjectResponse(projectObj)); } catch (err) { - res.status(500).json({ error: err.message }); + res.status(500).json({ success: false, data: {}, message: "Failed to fetch project." }); }🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 474 - 480, The catch block in the getProject controller function catches all errors including the AppError(403) thrown for access denial, but then returns a generic 500 status with `{ error: err.message }` format, which both masks the intended 403 status and violates the API response format guidelines. Modify the catch block to check if the error is an instance of AppError and if so, return the error's statusCode with the standardized response format `{ success: false, data: {}, message: err.message }`, otherwise return a generic 500 error with the same format. Alternatively, remove the local try-catch entirely and let the error propagate to your Express error handling middleware which should already handle AppError instances properly.Source: Coding guidelines
packages/common/src/utils/emailService.js (1)
367-373:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFormat the export email sender consistently.
Line 368 wraps
EMAIL_FROMdirectly, so a configured value likeTeam <team@example.com>becomes"urBackend" <Team <team@example.com>>. ReuseformatFromAddressand the shared reply-to value here.Proposed fix
const { data, error } = await resend.emails.send({ - from: `"urBackend" <${process.env.EMAIL_FROM || 'onboarding@resend.dev'}>`, + from: formatFromAddress(process.env.EMAIL_FROM || 'onboarding@resend.dev'), to: to, subject: subject, text: textBody, html: htmlContent, - replyTo: 'urbackend@apps.bitbros.in', + replyTo: replyToAddress, });🤖 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 `@packages/common/src/utils/emailService.js` around lines 367 - 373, The from address in the resend.emails.send call within the email export function is formatting the sender address inconsistently by wrapping EMAIL_FROM directly, which causes double wrapping if the configured value is already formatted as "Team <team@example.com>". Replace the hardcoded from field logic with the existing formatFromAddress function to ensure consistent formatting. Additionally, extract the hardcoded reply-to value 'urbackend@apps.bitbros.in' to a shared constant and reuse it here instead of duplicating the email address across the codebase.apps/web-dashboard/src/pages/Dashboard.jsx (1)
138-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNo-project early return hides pending invitations and blocks invite acceptance.
When
projects.length === 0, the component returns before the invitation banner renders. Users with only pending invites cannot accept/decline from dashboard.Suggested fix
- if (!isLoading && projects.length === 0) { + if (!isLoading && projects.length === 0 && invitations.length === 0) {Also applies to: 238-289
🤖 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/web-dashboard/src/pages/Dashboard.jsx` around lines 138 - 196, The early return condition in Dashboard.jsx that checks only if projects.length === 0 prevents the invitation banner from rendering, which blocks users with pending invitations from accepting or declining them. Modify the conditional logic to also check if there are pending invitations available before returning the empty state view. If the user has pending invitations but no projects, they should see the invitations UI instead of just the welcome message. Apply the same fix to the related code block at lines 238-289 that has the same issue.apps/public-api/src/middlewares/verifyApiKey.js (1)
43-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPublishable-key cache fast-path is effectively disabled after DB lookup.
The cache-key decision relies on
project[keyField], butpublishableKeyis not selected in Line 44-57. That makes the raw publishable-key branch unreachable here and adds an avoidable cache miss on subsequent requests.Suggested minimal fix
project = await Project.findOne(queryCondition) .select(` name owner resources collections databaseLimit databaseUsed storageLimit storageUsed jwtSecret allowedDomains isAuthEnabled siteUrl + publishableKey `) .populate('owner', 'isVerified') .lean(); @@ - const cacheKey = isSecret ? hashedApi : (project[keyField] === apiKey ? apiKey : hashedApi); + const cacheKey = isSecret ? hashedApi : (project.publishableKey === apiKey ? apiKey : hashedApi); + delete project.publishableKey; await setProjectByApiKeyCache(cacheKey, project);🤖 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/public-api/src/middlewares/verifyApiKey.js` around lines 43 - 66, The publishableKey field is not included in the Project.findOne().select() statement (lines 44-57), which means when the code later tries to check project[keyField] === apiKey to decide the cache key, the publishableKey will be undefined and the condition will always evaluate to false, causing the cache to always use hashedApi. Add publishableKey to the list of selected fields in the .select() method to ensure the fast-path cache key logic in the cacheKey assignment works correctly.apps/web-dashboard/src/pages/Webhooks.jsx (1)
68-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
projectpayload parsing is inconsistent with the expected API envelope.This page unwraps
webhooksbut notproject. Ifprojectis wrapped,isViewerandcollectionsderivation become incorrect, affecting permissions and form behavior.Suggested fix
- setProject(projRes.data); + setProject(projRes.data?.data || projRes.data || null);As per coding guidelines, "All API endpoints must return response format:
{ success: bool, data: {}, message: "" }."🤖 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/web-dashboard/src/pages/Webhooks.jsx` around lines 68 - 69, The project payload is being set inconsistently compared to the webhooks payload in the Webhooks.jsx page. The setWebhooks call correctly unwraps the nested structure with webhooksRes.data.data, but setProject only uses projRes.data without unwrapping to the actual data property. Since API endpoints follow the standard envelope format with { success, data, message }, change setProject(projRes.data) to setProject(projRes.data.data) to match the API response structure and ensure derived values like isViewer and collections are correctly computed from the actual project object.Source: Coding guidelines
apps/web-dashboard/src/pages/MailPlatform.jsx (1)
332-340:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winViewer gating is inconsistent in the logs empty state CTA.
Configure Mailis still shown to viewers here, while other settings-mutating entry points were correctly hidden. This creates a mixed permission UX.Suggested fix
- <button onClick={() => navigate(`/project/${projectId}/settings?tab=mail`)} className="btn btn-primary" style={{ fontSize: '0.9rem', height: '40px', padding: '0 24px', fontWeight: 600 }}> - Configure Mail - </button> + {!isViewer && ( + <button onClick={() => navigate(`/project/${projectId}/settings?tab=mail`)} className="btn btn-primary" style={{ fontSize: '0.9rem', height: '40px', padding: '0 24px', fontWeight: 600 }}> + Configure Mail + </button> + )}🤖 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/web-dashboard/src/pages/MailPlatform.jsx` around lines 332 - 340, The Configure Mail button in the empty state of the MailPlatform component needs viewer gating to prevent viewers from accessing settings they cannot modify. Wrap the button element (the one with onClick={() => navigate(`/project/${projectId}/settings?tab=mail`)}) in a conditional check that verifies the user is not a viewer before rendering it, using the same permission check pattern applied to other settings-mutating entry points elsewhere in the component.
🤖 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/dashboard-api/src/__tests__/onboarding.common.test.js`:
- Around line 74-85: The test assertions for the forced completion scenario are
insufficient to validate the actual behavior. The current assertions using
expect(onboarding).toBeTruthy() and expect(developer.save).toHaveBeenCalled() do
not verify that the completed flag is actually set to true in the returned
onboarding object. Add explicit assertions that check onboarding.completed is
true and optionally verify that the developer object was updated with completed:
true before the save method was called to ensure the completion state is
properly persisted and returned.
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 3098-3104: The response message in the project invitation endpoint
incorrectly claims "Invitation sent to ${invitee.email}" when the email sending
functionality remains commented out as TODO. Update the success response message
to accurately reflect what actually happened: the invitation record was created
but not sent via email. Change the message text to something like "Invitation
created for ${invitee.email}" or "Invitation generated for ${invitee.email}" to
avoid misleading the API consumer about email delivery status.
- Around line 347-353: The markDeveloperOnboardingStep function does not support
the _reset parameter, and the current code attempts to use it to reset
collectionCreated and firstApiCall steps after project creation. This
unsupported parameter causes these steps to be marked as true prematurely,
advancing onboarding before a collection or API call actually exists. Remove the
Promise.all block that contains the two markDeveloperOnboardingStep calls with
_reset: true, keeping only the initial markDeveloperOnboardingStep call for the
projectCreated step.
- Around line 2971-2972: In the catch block at lines 2971-2972, the raw
err.message is being exposed directly to the client response, which can leak
sensitive information about database or crypto operations. Replace this by
logging the full error server-side using your logger (with all error details for
debugging), and then return a generic, safe error message to the client in the
res.status(500).json response instead of err.message, such as "An error occurred
processing your request" or similar generic message.
In `@apps/dashboard-api/src/routes/webhooks.js`:
- Line 18: The verifyEmail middleware is executing before attachDeveloper and
checkWebhookGate on the webhook routes (lines 18, 27, 30, 36), which prevents
the capability gate from applying onboarding allowances to unverified
developers. Remove the verifyEmail middleware from all four webhook route
definitions in the router.post calls, and integrate the email verification logic
into the checkWebhookGate middleware so it can make access decisions that
respect capability gates and onboarding rules.
In `@apps/web-dashboard/src/pages/ProjectDetails.jsx`:
- Around line 100-107: After the successful API call to reveal the secret key in
the code block that handles `/api/projects/${projectId}/reveal-secret-key`, you
need to synchronize the project state by updating `project.secretKeyRevealed` to
true. Currently, only `setNewKey` is being called after the reveal, but the
project object's `secretKeyRevealed` property is not being updated, which causes
the UI to show "Reveal Once" on subsequent clicks. Update the project state or
context (using setProject or equivalent) to set `secretKeyRevealed` to true
immediately after the API call succeeds and before or after `setNewKey` is
called.
In `@apps/web-dashboard/src/pages/Storage.jsx`:
- Around line 23-29: The setProject and setFiles state updates are directly
accessing projRes.data and filesRes.data without properly handling the
standardized API response envelope format that includes success, data, and
message fields. Update the Promise.all block to properly extract data from the
standard response structure (the actual data payload is likely nested one level
deeper within the response object) and validate the success field before setting
state, ensuring the code respects the API response contract and prevents
breaking changes when the envelope structure is applied.
In `@packages/common/src/utils/onboarding.js`:
- Around line 141-164: The code is incorrectly auto-synthesizing prerequisite
steps and allowing the payload to directly control the completed flag, bypassing
the intended server progression logic. Remove the special case handling for the
firstApiCall step that auto-sets missing prerequisites like projectCreated and
collectionCreated (the block that iterates through
ONBOARDING_STEP_PREREQUISITES), and also remove or refactor the conditional
logic that sets developer.onboarding.completed based on payload.completed ===
true. Instead, only persist the steps explicitly requested in the update and let
the server compute the completion state based on all available steps without
direct payload influence.
In `@scratch/list_developers.js`:
- Around line 8-21: The connectDB() call is outside the try block, so if it
fails, the finally block containing mongoose.connection.close() will never
execute, leaving the connection unclosed. Move the connectDB() call inside the
try block before the Developer.find() invocation to ensure the finally block
always runs and properly closes the connection via mongoose.connection.close(),
regardless of whether connectDB() or any subsequent operation fails.
---
Outside diff comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 2943-2968: The secret key reveal endpoint has a race condition
where two concurrent requests can both read secretKeyRevealed as false before
either completes the save operation. Replace the separate Project.findOne and
project.save calls with a single atomic findOneAndUpdate operation that includes
a predicate checking secretKeyRevealed is false. Use the predicate in the query
filter to ensure the update only succeeds if secretKeyRevealed is still false at
the time of update, making the reveal operation truly single-use by preventing
the race condition that allows concurrent requests to both succeed.
- Around line 474-480: The catch block in the getProject controller function
catches all errors including the AppError(403) thrown for access denial, but
then returns a generic 500 status with `{ error: err.message }` format, which
both masks the intended 403 status and violates the API response format
guidelines. Modify the catch block to check if the error is an instance of
AppError and if so, return the error's statusCode with the standardized response
format `{ success: false, data: {}, message: err.message }`, otherwise return a
generic 500 error with the same format. Alternatively, remove the local
try-catch entirely and let the error propagate to your Express error handling
middleware which should already handle AppError instances properly.
In `@apps/public-api/src/middlewares/verifyApiKey.js`:
- Around line 43-66: The publishableKey field is not included in the
Project.findOne().select() statement (lines 44-57), which means when the code
later tries to check project[keyField] === apiKey to decide the cache key, the
publishableKey will be undefined and the condition will always evaluate to
false, causing the cache to always use hashedApi. Add publishableKey to the list
of selected fields in the .select() method to ensure the fast-path cache key
logic in the cacheKey assignment works correctly.
In `@apps/web-dashboard/src/pages/Dashboard.jsx`:
- Around line 138-196: The early return condition in Dashboard.jsx that checks
only if projects.length === 0 prevents the invitation banner from rendering,
which blocks users with pending invitations from accepting or declining them.
Modify the conditional logic to also check if there are pending invitations
available before returning the empty state view. If the user has pending
invitations but no projects, they should see the invitations UI instead of just
the welcome message. Apply the same fix to the related code block at lines
238-289 that has the same issue.
In `@apps/web-dashboard/src/pages/MailPlatform.jsx`:
- Around line 332-340: The Configure Mail button in the empty state of the
MailPlatform component needs viewer gating to prevent viewers from accessing
settings they cannot modify. Wrap the button element (the one with onClick={()
=> navigate(`/project/${projectId}/settings?tab=mail`)}) in a conditional check
that verifies the user is not a viewer before rendering it, using the same
permission check pattern applied to other settings-mutating entry points
elsewhere in the component.
In `@apps/web-dashboard/src/pages/Webhooks.jsx`:
- Around line 68-69: The project payload is being set inconsistently compared to
the webhooks payload in the Webhooks.jsx page. The setWebhooks call correctly
unwraps the nested structure with webhooksRes.data.data, but setProject only
uses projRes.data without unwrapping to the actual data property. Since API
endpoints follow the standard envelope format with { success, data, message },
change setProject(projRes.data) to setProject(projRes.data.data) to match the
API response structure and ensure derived values like isViewer and collections
are correctly computed from the actual project object.
In `@packages/common/src/utils/emailService.js`:
- Around line 367-373: The from address in the resend.emails.send call within
the email export function is formatting the sender address inconsistently by
wrapping EMAIL_FROM directly, which causes double wrapping if the configured
value is already formatted as "Team <team@example.com>". Replace the hardcoded
from field logic with the existing formatFromAddress function to ensure
consistent formatting. Additionally, extract the hardcoded reply-to value
'urbackend@apps.bitbros.in' to a shared constant and reuse it here instead of
duplicating the email address across the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37b7b24b-68d7-4535-b460-be7eead0fe81
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
apps/consumer/src/index.jsapps/dashboard-api/src/__tests__/auth.controller.test.jsapps/dashboard-api/src/__tests__/onboarding.common.test.jsapps/dashboard-api/src/__tests__/routes.projects.storage.test.jsapps/dashboard-api/src/__tests__/routes.user.test.jsapps/dashboard-api/src/app.jsapps/dashboard-api/src/controllers/analytics.controller.jsapps/dashboard-api/src/controllers/auth.controller.jsapps/dashboard-api/src/controllers/project.controller.jsapps/dashboard-api/src/middlewares/planEnforcement.jsapps/dashboard-api/src/routes/projects.jsapps/dashboard-api/src/routes/webhooks.jsapps/public-api/src/__tests__/api_usage.test.jsapps/public-api/src/middlewares/api_usage.jsapps/public-api/src/middlewares/verifyApiKey.jsapps/web-dashboard/src/App.jsxapps/web-dashboard/src/components/Layout/Sidebar.jsxapps/web-dashboard/src/components/ProtectedRoute.jsxapps/web-dashboard/src/index.cssapps/web-dashboard/src/pages/Auth.jsxapps/web-dashboard/src/pages/CreateCollection.jsxapps/web-dashboard/src/pages/Dashboard.jsxapps/web-dashboard/src/pages/Database.jsxapps/web-dashboard/src/pages/Login.jsxapps/web-dashboard/src/pages/MailPlatform.jsxapps/web-dashboard/src/pages/Onboarding.jsxapps/web-dashboard/src/pages/ProjectDetails.jsxapps/web-dashboard/src/pages/Signup.jsxapps/web-dashboard/src/pages/Storage.jsxapps/web-dashboard/src/pages/Webhooks.jsxapps/web-dashboard/src/utils/api.jsapps/web-dashboard/src/utils/quota.jspackages/common/src/index.jspackages/common/src/models/Project.jspackages/common/src/utils/emailService.jspackages/common/src/utils/onboarding.jspr_comments.jsonscratch/list_developers.jsscratch/test_signup.jssdks/urbackend-react/tests/context.test.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- apps/dashboard-api/src/tests/routes.projects.storage.test.js
- apps/web-dashboard/src/pages/Auth.jsx
- apps/consumer/src/index.js
- apps/web-dashboard/src/pages/Login.jsx
- apps/web-dashboard/src/utils/quota.js
- apps/web-dashboard/src/pages/Database.jsx
- apps/dashboard-api/src/tests/routes.user.test.js
- apps/public-api/src/tests/api_usage.test.js
- scratch/test_signup.js
- packages/common/src/models/Project.js
- packages/common/src/index.js
- apps/public-api/src/middlewares/api_usage.js
- apps/web-dashboard/src/pages/CreateCollection.jsx
- apps/dashboard-api/src/app.js
- apps/dashboard-api/src/tests/auth.controller.test.js
- apps/web-dashboard/src/utils/api.js
- apps/web-dashboard/src/components/Layout/Sidebar.jsx
- apps/dashboard-api/src/controllers/auth.controller.js
- apps/web-dashboard/src/App.jsx
- apps/dashboard-api/src/middlewares/planEnforcement.js
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/dashboard-api/src/controllers/project.controller.js (2)
2943-2968:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake secret-key reveal atomically single-use.
Two concurrent requests can both read
secretKeyRevealed === falsebefore either save completes, so the “single-use” key can be revealed twice. Claim the reveal with an atomicfindOneAndUpdatepredicate before decrypting/returning the key.Proposed fix
- const project = await Project.findOne({ - _id: req.params.projectId, - owner: req.user._id, - }).select("+secretKeyEncrypted secretKeyRevealed"); + const project = await Project.findOneAndUpdate( + { + _id: req.params.projectId, + owner: req.user._id, + secretKeyRevealed: { $ne: true }, + "secretKeyEncrypted.encrypted": { $exists: true, $ne: null }, + }, + { $set: { secretKeyRevealed: true } }, + { new: false }, + ).select("+secretKeyEncrypted secretKeyRevealed"); if (!project) { - return res.status(404).json({ success: false, data: {}, message: "Project not found." }); - } - - if (!req.user.isVerified) { - return res.status(403).json({ success: false, data: {}, message: "Account verification required to reveal secret key." }); - } - - if (project.secretKeyRevealed) { - return res.status(400).json({ success: false, data: {}, message: "Secret key has already been revealed once." }); - } - - if (!project.secretKeyEncrypted || !project.secretKeyEncrypted.encrypted) { - return res.status(404).json({ success: false, data: {}, message: "Secret key cannot be decrypted (it may have been rolled or cleared)." }); + return res.status(404).json({ success: false, data: {}, message: "Secret key is not available for reveal." }); } const decryptedKey = decrypt(project.secretKeyEncrypted); project.secretKeyRevealed = 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 `@apps/dashboard-api/src/controllers/project.controller.js` around lines 2943 - 2968, The secret key reveal endpoint has a race condition where two concurrent requests can both read secretKeyRevealed as false before either completes the save operation. Replace the separate Project.findOne and project.save calls with a single atomic findOneAndUpdate operation that includes a predicate checking secretKeyRevealed is false. Use the predicate in the query filter to ensure the update only succeeds if secretKeyRevealed is still false at the time of update, making the reveal operation truly single-use by preventing the race condition that allows concurrent requests to both succeed.
474-480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the intended 403 instead of converting
AppErrorto 500.The new
AppError(403)is caught by the localcatch, which always sends a 500 with{ error }. Return a structured 403 here or pass the error to Express error middleware.As per coding guidelines, “All API endpoints must return response format:
{ success: bool, data: {}, message: "" }. Use AppError class for errors.”Proposed fix
if (!getProjectRole(projectObj, req.user._id)) { - throw new AppError(403, "Access denied."); + return res.status(403).json({ success: false, data: {}, message: "Access denied." }); } res.json(sanitizeProjectResponse(projectObj)); } catch (err) { - res.status(500).json({ error: err.message }); + res.status(500).json({ success: false, data: {}, message: "Failed to fetch project." }); }🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 474 - 480, The catch block in the getProject controller function catches all errors including the AppError(403) thrown for access denial, but then returns a generic 500 status with `{ error: err.message }` format, which both masks the intended 403 status and violates the API response format guidelines. Modify the catch block to check if the error is an instance of AppError and if so, return the error's statusCode with the standardized response format `{ success: false, data: {}, message: err.message }`, otherwise return a generic 500 error with the same format. Alternatively, remove the local try-catch entirely and let the error propagate to your Express error handling middleware which should already handle AppError instances properly.Source: Coding guidelines
packages/common/src/utils/emailService.js (1)
367-373:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFormat the export email sender consistently.
Line 368 wraps
EMAIL_FROMdirectly, so a configured value likeTeam <team@example.com>becomes"urBackend" <Team <team@example.com>>. ReuseformatFromAddressand the shared reply-to value here.Proposed fix
const { data, error } = await resend.emails.send({ - from: `"urBackend" <${process.env.EMAIL_FROM || 'onboarding@resend.dev'}>`, + from: formatFromAddress(process.env.EMAIL_FROM || 'onboarding@resend.dev'), to: to, subject: subject, text: textBody, html: htmlContent, - replyTo: 'urbackend@apps.bitbros.in', + replyTo: replyToAddress, });🤖 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 `@packages/common/src/utils/emailService.js` around lines 367 - 373, The from address in the resend.emails.send call within the email export function is formatting the sender address inconsistently by wrapping EMAIL_FROM directly, which causes double wrapping if the configured value is already formatted as "Team <team@example.com>". Replace the hardcoded from field logic with the existing formatFromAddress function to ensure consistent formatting. Additionally, extract the hardcoded reply-to value 'urbackend@apps.bitbros.in' to a shared constant and reuse it here instead of duplicating the email address across the codebase.apps/web-dashboard/src/pages/Dashboard.jsx (1)
138-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNo-project early return hides pending invitations and blocks invite acceptance.
When
projects.length === 0, the component returns before the invitation banner renders. Users with only pending invites cannot accept/decline from dashboard.Suggested fix
- if (!isLoading && projects.length === 0) { + if (!isLoading && projects.length === 0 && invitations.length === 0) {Also applies to: 238-289
🤖 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/web-dashboard/src/pages/Dashboard.jsx` around lines 138 - 196, The early return condition in Dashboard.jsx that checks only if projects.length === 0 prevents the invitation banner from rendering, which blocks users with pending invitations from accepting or declining them. Modify the conditional logic to also check if there are pending invitations available before returning the empty state view. If the user has pending invitations but no projects, they should see the invitations UI instead of just the welcome message. Apply the same fix to the related code block at lines 238-289 that has the same issue.apps/public-api/src/middlewares/verifyApiKey.js (1)
43-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPublishable-key cache fast-path is effectively disabled after DB lookup.
The cache-key decision relies on
project[keyField], butpublishableKeyis not selected in Line 44-57. That makes the raw publishable-key branch unreachable here and adds an avoidable cache miss on subsequent requests.Suggested minimal fix
project = await Project.findOne(queryCondition) .select(` name owner resources collections databaseLimit databaseUsed storageLimit storageUsed jwtSecret allowedDomains isAuthEnabled siteUrl + publishableKey `) .populate('owner', 'isVerified') .lean(); @@ - const cacheKey = isSecret ? hashedApi : (project[keyField] === apiKey ? apiKey : hashedApi); + const cacheKey = isSecret ? hashedApi : (project.publishableKey === apiKey ? apiKey : hashedApi); + delete project.publishableKey; await setProjectByApiKeyCache(cacheKey, project);🤖 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/public-api/src/middlewares/verifyApiKey.js` around lines 43 - 66, The publishableKey field is not included in the Project.findOne().select() statement (lines 44-57), which means when the code later tries to check project[keyField] === apiKey to decide the cache key, the publishableKey will be undefined and the condition will always evaluate to false, causing the cache to always use hashedApi. Add publishableKey to the list of selected fields in the .select() method to ensure the fast-path cache key logic in the cacheKey assignment works correctly.apps/web-dashboard/src/pages/Webhooks.jsx (1)
68-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
projectpayload parsing is inconsistent with the expected API envelope.This page unwraps
webhooksbut notproject. Ifprojectis wrapped,isViewerandcollectionsderivation become incorrect, affecting permissions and form behavior.Suggested fix
- setProject(projRes.data); + setProject(projRes.data?.data || projRes.data || null);As per coding guidelines, "All API endpoints must return response format:
{ success: bool, data: {}, message: "" }."🤖 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/web-dashboard/src/pages/Webhooks.jsx` around lines 68 - 69, The project payload is being set inconsistently compared to the webhooks payload in the Webhooks.jsx page. The setWebhooks call correctly unwraps the nested structure with webhooksRes.data.data, but setProject only uses projRes.data without unwrapping to the actual data property. Since API endpoints follow the standard envelope format with { success, data, message }, change setProject(projRes.data) to setProject(projRes.data.data) to match the API response structure and ensure derived values like isViewer and collections are correctly computed from the actual project object.Source: Coding guidelines
apps/web-dashboard/src/pages/MailPlatform.jsx (1)
332-340:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winViewer gating is inconsistent in the logs empty state CTA.
Configure Mailis still shown to viewers here, while other settings-mutating entry points were correctly hidden. This creates a mixed permission UX.Suggested fix
- <button onClick={() => navigate(`/project/${projectId}/settings?tab=mail`)} className="btn btn-primary" style={{ fontSize: '0.9rem', height: '40px', padding: '0 24px', fontWeight: 600 }}> - Configure Mail - </button> + {!isViewer && ( + <button onClick={() => navigate(`/project/${projectId}/settings?tab=mail`)} className="btn btn-primary" style={{ fontSize: '0.9rem', height: '40px', padding: '0 24px', fontWeight: 600 }}> + Configure Mail + </button> + )}🤖 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/web-dashboard/src/pages/MailPlatform.jsx` around lines 332 - 340, The Configure Mail button in the empty state of the MailPlatform component needs viewer gating to prevent viewers from accessing settings they cannot modify. Wrap the button element (the one with onClick={() => navigate(`/project/${projectId}/settings?tab=mail`)}) in a conditional check that verifies the user is not a viewer before rendering it, using the same permission check pattern applied to other settings-mutating entry points elsewhere in the component.
🤖 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/dashboard-api/src/__tests__/onboarding.common.test.js`:
- Around line 74-85: The test assertions for the forced completion scenario are
insufficient to validate the actual behavior. The current assertions using
expect(onboarding).toBeTruthy() and expect(developer.save).toHaveBeenCalled() do
not verify that the completed flag is actually set to true in the returned
onboarding object. Add explicit assertions that check onboarding.completed is
true and optionally verify that the developer object was updated with completed:
true before the save method was called to ensure the completion state is
properly persisted and returned.
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 3098-3104: The response message in the project invitation endpoint
incorrectly claims "Invitation sent to ${invitee.email}" when the email sending
functionality remains commented out as TODO. Update the success response message
to accurately reflect what actually happened: the invitation record was created
but not sent via email. Change the message text to something like "Invitation
created for ${invitee.email}" or "Invitation generated for ${invitee.email}" to
avoid misleading the API consumer about email delivery status.
- Around line 347-353: The markDeveloperOnboardingStep function does not support
the _reset parameter, and the current code attempts to use it to reset
collectionCreated and firstApiCall steps after project creation. This
unsupported parameter causes these steps to be marked as true prematurely,
advancing onboarding before a collection or API call actually exists. Remove the
Promise.all block that contains the two markDeveloperOnboardingStep calls with
_reset: true, keeping only the initial markDeveloperOnboardingStep call for the
projectCreated step.
- Around line 2971-2972: In the catch block at lines 2971-2972, the raw
err.message is being exposed directly to the client response, which can leak
sensitive information about database or crypto operations. Replace this by
logging the full error server-side using your logger (with all error details for
debugging), and then return a generic, safe error message to the client in the
res.status(500).json response instead of err.message, such as "An error occurred
processing your request" or similar generic message.
In `@apps/dashboard-api/src/routes/webhooks.js`:
- Line 18: The verifyEmail middleware is executing before attachDeveloper and
checkWebhookGate on the webhook routes (lines 18, 27, 30, 36), which prevents
the capability gate from applying onboarding allowances to unverified
developers. Remove the verifyEmail middleware from all four webhook route
definitions in the router.post calls, and integrate the email verification logic
into the checkWebhookGate middleware so it can make access decisions that
respect capability gates and onboarding rules.
In `@apps/web-dashboard/src/pages/ProjectDetails.jsx`:
- Around line 100-107: After the successful API call to reveal the secret key in
the code block that handles `/api/projects/${projectId}/reveal-secret-key`, you
need to synchronize the project state by updating `project.secretKeyRevealed` to
true. Currently, only `setNewKey` is being called after the reveal, but the
project object's `secretKeyRevealed` property is not being updated, which causes
the UI to show "Reveal Once" on subsequent clicks. Update the project state or
context (using setProject or equivalent) to set `secretKeyRevealed` to true
immediately after the API call succeeds and before or after `setNewKey` is
called.
In `@apps/web-dashboard/src/pages/Storage.jsx`:
- Around line 23-29: The setProject and setFiles state updates are directly
accessing projRes.data and filesRes.data without properly handling the
standardized API response envelope format that includes success, data, and
message fields. Update the Promise.all block to properly extract data from the
standard response structure (the actual data payload is likely nested one level
deeper within the response object) and validate the success field before setting
state, ensuring the code respects the API response contract and prevents
breaking changes when the envelope structure is applied.
In `@packages/common/src/utils/onboarding.js`:
- Around line 141-164: The code is incorrectly auto-synthesizing prerequisite
steps and allowing the payload to directly control the completed flag, bypassing
the intended server progression logic. Remove the special case handling for the
firstApiCall step that auto-sets missing prerequisites like projectCreated and
collectionCreated (the block that iterates through
ONBOARDING_STEP_PREREQUISITES), and also remove or refactor the conditional
logic that sets developer.onboarding.completed based on payload.completed ===
true. Instead, only persist the steps explicitly requested in the update and let
the server compute the completion state based on all available steps without
direct payload influence.
In `@scratch/list_developers.js`:
- Around line 8-21: The connectDB() call is outside the try block, so if it
fails, the finally block containing mongoose.connection.close() will never
execute, leaving the connection unclosed. Move the connectDB() call inside the
try block before the Developer.find() invocation to ensure the finally block
always runs and properly closes the connection via mongoose.connection.close(),
regardless of whether connectDB() or any subsequent operation fails.
---
Outside diff comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 2943-2968: The secret key reveal endpoint has a race condition
where two concurrent requests can both read secretKeyRevealed as false before
either completes the save operation. Replace the separate Project.findOne and
project.save calls with a single atomic findOneAndUpdate operation that includes
a predicate checking secretKeyRevealed is false. Use the predicate in the query
filter to ensure the update only succeeds if secretKeyRevealed is still false at
the time of update, making the reveal operation truly single-use by preventing
the race condition that allows concurrent requests to both succeed.
- Around line 474-480: The catch block in the getProject controller function
catches all errors including the AppError(403) thrown for access denial, but
then returns a generic 500 status with `{ error: err.message }` format, which
both masks the intended 403 status and violates the API response format
guidelines. Modify the catch block to check if the error is an instance of
AppError and if so, return the error's statusCode with the standardized response
format `{ success: false, data: {}, message: err.message }`, otherwise return a
generic 500 error with the same format. Alternatively, remove the local
try-catch entirely and let the error propagate to your Express error handling
middleware which should already handle AppError instances properly.
In `@apps/public-api/src/middlewares/verifyApiKey.js`:
- Around line 43-66: The publishableKey field is not included in the
Project.findOne().select() statement (lines 44-57), which means when the code
later tries to check project[keyField] === apiKey to decide the cache key, the
publishableKey will be undefined and the condition will always evaluate to
false, causing the cache to always use hashedApi. Add publishableKey to the list
of selected fields in the .select() method to ensure the fast-path cache key
logic in the cacheKey assignment works correctly.
In `@apps/web-dashboard/src/pages/Dashboard.jsx`:
- Around line 138-196: The early return condition in Dashboard.jsx that checks
only if projects.length === 0 prevents the invitation banner from rendering,
which blocks users with pending invitations from accepting or declining them.
Modify the conditional logic to also check if there are pending invitations
available before returning the empty state view. If the user has pending
invitations but no projects, they should see the invitations UI instead of just
the welcome message. Apply the same fix to the related code block at lines
238-289 that has the same issue.
In `@apps/web-dashboard/src/pages/MailPlatform.jsx`:
- Around line 332-340: The Configure Mail button in the empty state of the
MailPlatform component needs viewer gating to prevent viewers from accessing
settings they cannot modify. Wrap the button element (the one with onClick={()
=> navigate(`/project/${projectId}/settings?tab=mail`)}) in a conditional check
that verifies the user is not a viewer before rendering it, using the same
permission check pattern applied to other settings-mutating entry points
elsewhere in the component.
In `@apps/web-dashboard/src/pages/Webhooks.jsx`:
- Around line 68-69: The project payload is being set inconsistently compared to
the webhooks payload in the Webhooks.jsx page. The setWebhooks call correctly
unwraps the nested structure with webhooksRes.data.data, but setProject only
uses projRes.data without unwrapping to the actual data property. Since API
endpoints follow the standard envelope format with { success, data, message },
change setProject(projRes.data) to setProject(projRes.data.data) to match the
API response structure and ensure derived values like isViewer and collections
are correctly computed from the actual project object.
In `@packages/common/src/utils/emailService.js`:
- Around line 367-373: The from address in the resend.emails.send call within
the email export function is formatting the sender address inconsistently by
wrapping EMAIL_FROM directly, which causes double wrapping if the configured
value is already formatted as "Team <team@example.com>". Replace the hardcoded
from field logic with the existing formatFromAddress function to ensure
consistent formatting. Additionally, extract the hardcoded reply-to value
'urbackend@apps.bitbros.in' to a shared constant and reuse it here instead of
duplicating the email address across the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37b7b24b-68d7-4535-b460-be7eead0fe81
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
apps/consumer/src/index.jsapps/dashboard-api/src/__tests__/auth.controller.test.jsapps/dashboard-api/src/__tests__/onboarding.common.test.jsapps/dashboard-api/src/__tests__/routes.projects.storage.test.jsapps/dashboard-api/src/__tests__/routes.user.test.jsapps/dashboard-api/src/app.jsapps/dashboard-api/src/controllers/analytics.controller.jsapps/dashboard-api/src/controllers/auth.controller.jsapps/dashboard-api/src/controllers/project.controller.jsapps/dashboard-api/src/middlewares/planEnforcement.jsapps/dashboard-api/src/routes/projects.jsapps/dashboard-api/src/routes/webhooks.jsapps/public-api/src/__tests__/api_usage.test.jsapps/public-api/src/middlewares/api_usage.jsapps/public-api/src/middlewares/verifyApiKey.jsapps/web-dashboard/src/App.jsxapps/web-dashboard/src/components/Layout/Sidebar.jsxapps/web-dashboard/src/components/ProtectedRoute.jsxapps/web-dashboard/src/index.cssapps/web-dashboard/src/pages/Auth.jsxapps/web-dashboard/src/pages/CreateCollection.jsxapps/web-dashboard/src/pages/Dashboard.jsxapps/web-dashboard/src/pages/Database.jsxapps/web-dashboard/src/pages/Login.jsxapps/web-dashboard/src/pages/MailPlatform.jsxapps/web-dashboard/src/pages/Onboarding.jsxapps/web-dashboard/src/pages/ProjectDetails.jsxapps/web-dashboard/src/pages/Signup.jsxapps/web-dashboard/src/pages/Storage.jsxapps/web-dashboard/src/pages/Webhooks.jsxapps/web-dashboard/src/utils/api.jsapps/web-dashboard/src/utils/quota.jspackages/common/src/index.jspackages/common/src/models/Project.jspackages/common/src/utils/emailService.jspackages/common/src/utils/onboarding.jspr_comments.jsonscratch/list_developers.jsscratch/test_signup.jssdks/urbackend-react/tests/context.test.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- apps/dashboard-api/src/tests/routes.projects.storage.test.js
- apps/web-dashboard/src/pages/Auth.jsx
- apps/consumer/src/index.js
- apps/web-dashboard/src/pages/Login.jsx
- apps/web-dashboard/src/utils/quota.js
- apps/web-dashboard/src/pages/Database.jsx
- apps/dashboard-api/src/tests/routes.user.test.js
- apps/public-api/src/tests/api_usage.test.js
- scratch/test_signup.js
- packages/common/src/models/Project.js
- packages/common/src/index.js
- apps/public-api/src/middlewares/api_usage.js
- apps/web-dashboard/src/pages/CreateCollection.jsx
- apps/dashboard-api/src/app.js
- apps/dashboard-api/src/tests/auth.controller.test.js
- apps/web-dashboard/src/utils/api.js
- apps/web-dashboard/src/components/Layout/Sidebar.jsx
- apps/dashboard-api/src/controllers/auth.controller.js
- apps/web-dashboard/src/App.jsx
- apps/dashboard-api/src/middlewares/planEnforcement.js
🛑 Comments failed to post (9)
apps/dashboard-api/src/__tests__/onboarding.common.test.js (1)
74-85:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen forced-completion assertions to catch real contract regressions.
On Line 83 and Line 84, the assertions are too weak: this test can pass even when
completedis not actually reflected in the resulting onboarding state. Please assert the completion flag explicitly (persisted and returned) so this flow is truly validated.Suggested test assertion update
- expect(onboarding).toBeTruthy(); - expect(developer.save).toHaveBeenCalled(); + expect(developer.save).toHaveBeenCalledTimes(1); + expect(developer.onboarding.completed).toBe(true); + expect(onboarding.completed).toBe(true);📝 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.test('allows forced completion (skip flow)', async () => { const developer = makeDeveloper({ steps: { projectCreated: true } }); Developer.findById.mockReturnValue({ select: jest.fn().mockResolvedValue(developer) }); // completed:true is now allowed even if steps are incomplete — user chose to skip const onboarding = await updateDeveloperOnboarding('dev_1', { completed: true, }); expect(developer.save).toHaveBeenCalledTimes(1); expect(developer.onboarding.completed).toBe(true); expect(onboarding.completed).toBe(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 `@apps/dashboard-api/src/__tests__/onboarding.common.test.js` around lines 74 - 85, The test assertions for the forced completion scenario are insufficient to validate the actual behavior. The current assertions using expect(onboarding).toBeTruthy() and expect(developer.save).toHaveBeenCalled() do not verify that the completed flag is actually set to true in the returned onboarding object. Add explicit assertions that check onboarding.completed is true and optionally verify that the developer object was updated with completed: true before the save method was called to ensure the completion state is properly persisted and returned.apps/dashboard-api/src/controllers/project.controller.js (3)
347-353:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the unsupported onboarding “reset” calls.
markDeveloperOnboardingStepdoes not implement_reset; these calls attempt to setcollectionCreatedandfirstApiCalltotrue. After project creation this can advance—or even complete—onboarding before a collection or API call exists.Proposed fix
markDeveloperOnboardingStep(req.user._id, 'projectCreated', { projectId: newProject._id }) - .then(() => { - // Also reset subsequent steps since this is a new project - return Promise.all([ - markDeveloperOnboardingStep(req.user._id, 'collectionCreated', { _reset: true }), - markDeveloperOnboardingStep(req.user._id, 'firstApiCall', { _reset: true }) - ]); - }) .catch((err) => { console.error('[onboarding] Failed to mark projectCreated:', err.message); });🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 347 - 353, The markDeveloperOnboardingStep function does not support the _reset parameter, and the current code attempts to use it to reset collectionCreated and firstApiCall steps after project creation. This unsupported parameter causes these steps to be marked as true prematurely, advancing onboarding before a collection or API call actually exists. Remove the Promise.all block that contains the two markDeveloperOnboardingStep calls with _reset: true, keeping only the initial markDeveloperOnboardingStep call for the projectCreated step.
2971-2972:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not expose raw reveal failures to clients.
err.messagecan leak crypto or database internals. Log the error server-side and return a generic structured response.As per coding guidelines, “Never use raw throw statements or expose MongoDB errors to clients.”
Proposed fix
} catch (err) { - res.status(500).json({ success: false, data: {}, message: err.message }); + console.error("Reveal secret key failed:", err); + res.status(500).json({ success: false, data: {}, message: "Failed to reveal secret key." }); }📝 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.} catch (err) { console.error("Reveal secret key failed:", err); res.status(500).json({ success: false, data: {}, message: "Failed to reveal secret key." }); }🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 2971 - 2972, In the catch block at lines 2971-2972, the raw err.message is being exposed directly to the client response, which can leak sensitive information about database or crypto operations. Replace this by logging the full error server-side using your logger (with all error details for debugging), and then return a generic, safe error message to the client in the res.status(500).json response instead of err.message, such as "An error occurred processing your request" or similar generic message.Source: Coding guidelines
3098-3104:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not claim the invitation was sent while email delivery is still TODO.
The endpoint only creates an
Invitation; no notification is sent, but the response saysInvitation sent. Invitees may never discover the pending invite unless another surfaced flow exists.Do you want me to generate the invite email implementation or open an issue to track it?
🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 3098 - 3104, The response message in the project invitation endpoint incorrectly claims "Invitation sent to ${invitee.email}" when the email sending functionality remains commented out as TODO. Update the success response message to accurately reflect what actually happened: the invitation record was created but not sent via email. Change the message text to something like "Invitation created for ${invitee.email}" or "Invitation generated for ${invitee.email}" to avoid misleading the API consumer about email delivery status.apps/dashboard-api/src/routes/webhooks.js (1)
18-18:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet the capability gate own webhook access decisions.
verifyEmailruns beforeattachDeveloper/checkWebhookGate, so unverified developers are rejected before the new capability-aware gate can apply onboarding allowances. RemoveverifyEmailhere or fold the verification rule intocheckWebhookGate.Proposed fix
-router.post("/:projectId/webhooks", authMiddleware, verifyEmail, authorizeProject('admin'), attachDeveloper, checkWebhookGate, createWebhook); +router.post("/:projectId/webhooks", authMiddleware, authorizeProject('admin'), attachDeveloper, checkWebhookGate, createWebhook); @@ -router.patch("/:projectId/webhooks/:webhookId", authMiddleware, verifyEmail, authorizeProject('admin'), attachDeveloper, checkWebhookGate, updateWebhook); +router.patch("/:projectId/webhooks/:webhookId", authMiddleware, authorizeProject('admin'), attachDeveloper, checkWebhookGate, updateWebhook); @@ -router.delete("/:projectId/webhooks/:webhookId", authMiddleware, verifyEmail, authorizeProject('admin'), attachDeveloper, checkWebhookGate, deleteWebhook); +router.delete("/:projectId/webhooks/:webhookId", authMiddleware, authorizeProject('admin'), attachDeveloper, checkWebhookGate, deleteWebhook); @@ -router.post("/:projectId/webhooks/:webhookId/test", authMiddleware, verifyEmail, authorizeProject('admin'), attachDeveloper, checkWebhookGate, testWebhook); +router.post("/:projectId/webhooks/:webhookId/test", authMiddleware, authorizeProject('admin'), attachDeveloper, checkWebhookGate, testWebhook);Also applies to: 27-27, 30-30, 36-36
🤖 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/dashboard-api/src/routes/webhooks.js` at line 18, The verifyEmail middleware is executing before attachDeveloper and checkWebhookGate on the webhook routes (lines 18, 27, 30, 36), which prevents the capability gate from applying onboarding allowances to unverified developers. Remove the verifyEmail middleware from all four webhook route definitions in the router.post calls, and integrate the email verification logic into the checkWebhookGate middleware so it can make access decisions that respect capability gates and onboarding rules.apps/web-dashboard/src/pages/ProjectDetails.jsx (1)
100-107:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSync
project.secretKeyRevealedafter URL-triggered reveal.This path reveals the secret key but doesn’t update
project.secretKeyRevealed, so the sidebar can still show “Reveal Once” after success and then fail on the next click.Suggested fix
const secretRes = await api.post(`/api/projects/${projectId}/reveal-secret-key`); + setProject((prev) => ({ ...prev, secretKeyRevealed: true })); setNewKey({ type: 'API', keys: [ { label: 'Publishable Key', value: project.publishableKey }, { label: 'Secret Key', value: secretRes.data.data.secretKey } ] });📝 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.const secretRes = await api.post(`/api/projects/${projectId}/reveal-secret-key`); setProject((prev) => ({ ...prev, secretKeyRevealed: true })); setNewKey({ type: 'API', keys: [ { label: 'Publishable Key', value: project.publishableKey }, { label: 'Secret Key', value: secretRes.data.data.secretKey } ] });🤖 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/web-dashboard/src/pages/ProjectDetails.jsx` around lines 100 - 107, After the successful API call to reveal the secret key in the code block that handles `/api/projects/${projectId}/reveal-secret-key`, you need to synchronize the project state by updating `project.secretKeyRevealed` to true. Currently, only `setNewKey` is being called after the reveal, but the project object's `secretKeyRevealed` property is not being updated, which causes the UI to show "Reveal Once" on subsequent clicks. Update the project state or context (using setProject or equivalent) to set `secretKeyRevealed` to true immediately after the API call succeeds and before or after `setNewKey` is called.apps/web-dashboard/src/pages/Storage.jsx (1)
23-29:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winProject/files parsing is brittle against the API response contract.
Directly assigning
projRes.data/filesRes.datamakes this page dependent on unwrapped payloads and can break role gating + file list handling when endpoints return the standard envelope.Suggested fix
- setProject(projRes.data); - setFiles(filesRes.data); + setProject(projRes.data?.data || projRes.data || null); + setFiles(filesRes.data?.data || filesRes.data || []);As per coding guidelines, "All API endpoints must return response format:
{ success: bool, data: {}, message: "" }."🤖 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/web-dashboard/src/pages/Storage.jsx` around lines 23 - 29, The setProject and setFiles state updates are directly accessing projRes.data and filesRes.data without properly handling the standardized API response envelope format that includes success, data, and message fields. Update the Promise.all block to properly extract data from the standard response structure (the actual data payload is likely nested one level deeper within the response object) and validate the success field before setting state, ensuring the code respects the API response contract and prevents breaking changes when the envelope structure is applied.Source: Coding guidelines
packages/common/src/utils/onboarding.js (1)
141-164:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not synthesize prerequisite steps or
completedfrom the update payload.
updateDeveloperOnboarding({ steps: { firstApiCall: true } })now auto-setsprojectCreated/collectionCreated, andpayload.completed === truecan persist completion even when steps are incomplete. This bypasses the 3-step server progression and can return a normalized object that disagrees with the persistedcompletedflag.Proposed fix
const trueSteps = getRequestedTrueSteps(payload.steps); for (const step of trueSteps) { if (!normalizeOnboarding(developer.onboarding).steps[step]) { - if (step === 'firstApiCall') { - // For firstApiCall, auto-satisfy missing prerequisites. - // If collectionCreated was never persisted (skip/race condition), - // we still allow the API test to mark onboarding done. - const prerequisites = ONBOARDING_STEP_PREREQUISITES[step] || []; - for (const prereq of prerequisites) { - if (!normalizeOnboarding(developer.onboarding).steps[prereq]) { - developer.set(ONBOARDING_STEP_FIELDS[prereq], true); - } - } - } else { - assertCanSetStep(developer.onboarding, step); - } + assertCanSetStep(developer.onboarding, step); developer.set(ONBOARDING_STEP_FIELDS[step], true); } } const onboarding = normalizeOnboarding(developer.onboarding); - if (payload.completed === true) { - developer.onboarding.completed = true; - } else { - developer.onboarding.completed = onboarding.completed; + if (payload.completed === true && !onboarding.completed) { + const err = new Error('Complete all onboarding steps before marking onboarding completed.'); + err.statusCode = 409; + throw err; } + developer.onboarding.completed = onboarding.completed;🤖 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 `@packages/common/src/utils/onboarding.js` around lines 141 - 164, The code is incorrectly auto-synthesizing prerequisite steps and allowing the payload to directly control the completed flag, bypassing the intended server progression logic. Remove the special case handling for the firstApiCall step that auto-sets missing prerequisites like projectCreated and collectionCreated (the block that iterates through ONBOARDING_STEP_PREREQUISITES), and also remove or refactor the conditional logic that sets developer.onboarding.completed based on payload.completed === true. Instead, only persist the steps explicitly requested in the update and let the server compute the completion state based on all available steps without direct payload influence.scratch/list_developers.js (1)
8-21:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove
connectDB()insidetryso cleanup always runs.If
connectDB()rejects, execution never reachesfinally, so connection cleanup is skipped on that path.Suggested fix
async function list() { - await connectDB(); try { + await connectDB(); const developers = await Developer.find({}); console.log("All developers in DB:"); developers.forEach(d => { let maskedEmail = d.email; if (maskedEmail && maskedEmail.includes('@')) {🤖 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 `@scratch/list_developers.js` around lines 8 - 21, The connectDB() call is outside the try block, so if it fails, the finally block containing mongoose.connection.close() will never execute, leaving the connection unclosed. Move the connectDB() call inside the try block before the Developer.find() invocation to ensure the finally block always runs and properly closes the connection via mongoose.connection.close(), regardless of whether connectDB() or any subsequent operation fails.
… from user-controlled sources' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
yash-pouranik
temporarily deployed
to
feat/onboarding - urBackend-frankfrut PR #314
Render
Destroyed
…function' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Member
|
@coderabbitai |
Contributor
|
✅ Action performedFull review finished. |
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
4 participants
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.
🚀 Pull Request Description
Fixes # (issue number)
🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Optional)
✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit
first_api_call(with legacy alias handling) and improved verification-gate behavior.