Repository navigation
Features - #133
Merged
Merged
Features#133
Conversation
- added awaiting and error handling + logging in the sendEmail method of the email service - removed the sendEmail method of the `EmailVerificationsService`. This responsibility is offloaded to the `EmailService`. Removed the corresponding dto and type in the `email-verifications.dto` - in the AuthService, used directly the EmailService to send emails in the `createEmailUpdate` method and injected the EmailModule in the AuthModule - extracted the email logo url from the urls body in a constant, same for the support email - placed the bucket used to serve this logo image under a custom domain instead of a public development url
- made the extension check for the type of error received from the server during the fetch of initial data or a periodic data synchronization, and when it is undefined, logs the user out by deleting the token. This can happen if the `JWT_SECRET` on the server side has been rotated and no longer recognizes the jwt sent by the extension - made the data of the error typesafe using a trpc utility `isTRPCClientError`
- added the `isTRPCClientError` function in the trpc file - simplified the logic of the retry function of the query client in the dashboard - simplified the logic of error handling in the error-boundary of the dashboard (trpc errors), redirect to /login if the error received is `UNAUTHORIZED`, and the boundary is reset not only when the order of the before and after dates is changed but also when the period changes
- removed unused constants from the constants of the`@repo/common` package - scoped some of those constants to the api only
- removed the `ALLOWED_CLIENTS` constants and added a function `getAllowedClients` to retrieve them depending of the NODE_ENV - added the `prettier-plugin-sh` to format bash scripts and Dockerfiles
- added a format command to each app/package - added a persistent turbo format command - turbo format is the part of the husky pre-commit hook
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request implements comprehensive code formatting infrastructure across the monorepo, consolidates distributed error message constants into app-specific locations, refactors email service error handling with Resend response validation, and updates auth origin validation from static to dynamic configuration. Shell script formatting support is added, and tRPC error handling is enhanced with type guards throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Auth Service)
participant EmailService as EmailService
participant Resend as Resend API
participant Handler as Error Handler
Client->>EmailService: sendEmail(SendEmailDtoType)
EmailService->>Resend: POST /emails/send (via Resend SDK)
alt Success Response
Resend-->>EmailService: { data: { id, ... } }
EmailService->>EmailService: Await & capture result
EmailService-->>Client: return result.data
else Error Response
Resend-->>EmailService: { error: { message, name, statusCode } }
EmailService->>EmailService: Await & capture result
EmailService->>EmailService: log(error.message, error.name, error.statusCode)
EmailService->>Handler: throw TRPCError(INTERNAL_SERVER_ERROR)
Handler-->>Client: Error with SUPPORT_EMAIL reference
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 32 minutes and 18 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/auth/auth.service.ts (1)
307-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle partial-success flow when notice email fails.
createEmailUpdatecan return an error after the verification was already created/sent (step 1 succeeds, notice send at Line 313 fails). That makes the operation externally fail while state has advanced.Use explicit handling: either treat the notice as best-effort (log and continue) or rollback the created verification before rethrowing.
Suggested direction (best-effort notice variant)
- await this.emailService.sendEmail({ - type: "notice email update", - email: existingUser.email, - }); + try { + await this.emailService.sendEmail({ + type: "notice email update", + email: existingUser.email, + }); + } catch { + // Keep primary flow successful; optionally add structured warning log. + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.service.ts` around lines 307 - 316, The current flow creates a verification via createEmailVerification and then sends a notice using emailService.sendEmail; if the notice send fails the operation bubbles up while the verification was already created. Change this to a best-effort notice: call emailService.sendEmail inside a try/catch, and on error log the failure (include context like verificationToken, message, existingUser.email and the caught error) but do not rethrow so the verification creation is preserved; ensure the surrounding function (the method containing the calls) still returns the created verification details (message, verificationToken).
🧹 Nitpick comments (1)
apps/api/src/common/utils/get-allowed-clients.ts (1)
12-20: ⚡ Quick winPrefer explicit production ports over generated ranges.
The computed
DASHBOARD_PRODUCTION_PORT + irange broadens trusted origins implicitly. Since this list is used for credentialed CORS, explicit ports are safer and easier to audit.Suggested change
-const ALLOWED_CLIENTS_IN_PRODUCTION = Array.from( - { length: 6 }, - (_, i) => DASHBOARD_PRODUCTION_PORT + i, -) - .flatMap((port) => [`http://localhost:${port}`, `http://127.0.0.1:${port}`]) - .concat([ - `http://localhost:${DASHBOARD_PREVIEW_PORT}`, - `http://127.0.0.1:${DASHBOARD_PREVIEW_PORT}`, - ]); +const ALLOWED_CLIENT_PORTS_IN_PRODUCTION = [ + DASHBOARD_PRODUCTION_PORT, + DASHBOARD_PREVIEW_PORT, +]; + +const ALLOWED_CLIENTS_IN_PRODUCTION = ALLOWED_CLIENT_PORTS_IN_PRODUCTION.flatMap( + (port) => [`http://localhost:${port}`, `http://127.0.0.1:${port}`], +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/common/utils/get-allowed-clients.ts` around lines 12 - 20, Replace the implicit range generation in ALLOWED_CLIENTS_IN_PRODUCTION with an explicit, auditable list of production ports: remove the Array.from(...) + flatMap logic that generates DASHBOARD_PRODUCTION_PORT + i and instead enumerate the specific production ports you intend to allow (each producing http://localhost:PORT and http://127.0.0.1:PORT) and keep the existing entries for DASHBOARD_PREVIEW_PORT; update the ALLOWED_CLIENTS_IN_PRODUCTION constant so it only contains those explicit origin strings derived from the specific ports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/pre-commit:
- Around line 4-6: Replace direct calls to the local turbo binary with npx to
avoid PATH issues: update the commands "turbo format" and "turbo lint" in the
pre-commit hook to use "npx turbo format" and "npx turbo lint" respectively
(leave "git add -u" unchanged) so the hook resolves the local node_modules
binary reliably in environments like VSCode or nvm.
In `@apps/dashboard/src/components/errors/error-boundary.tsx`:
- Around line 55-63: The effect currently calls resetErrorBoundary() on mount
whenever period !== "Custom Range", which can cause immediate retry/thrashing;
change the effect in the component that uses useEffect, customRange.start,
customRange.end, period and resetErrorBoundary to skip the initial mount by
tracking previous values (e.g., a ref like isMountedRef or prevPeriod/prevRange)
and only call resetErrorBoundary when the user actually changes period or date
range compared to the previous values (not on first render); implement the ref
check at the top of the effect and update the prev values after deciding to
reset.
In `@apps/dashboard/src/features/auth/components/login-form.tsx`:
- Around line 54-57: The login form currently routes errors by checking
substrings in errorMessage, which misses AuthService.signIn's "User not found"
text and sends unknown-email to root; update the error handling in the sign-in
flow (where errorMessage is processed and form.setError is called) to inspect
the tRPC error code instead of plain text: map error.code === "NOT_FOUND" to set
form.setError("email", ...) and error.code === "UNAUTHORIZED" to set
form.setError("password", ...), falling back to form.setError("root", ...) for
other cases so inline field feedback works regardless of error wording.
In `@apps/vscode-extension/src/utils/errors/handle-invalid-token-error.ts`:
- Around line 11-13: The current branch only treats error.data?.code ===
"UNAUTHORIZED" as an invalid-token case and skips deletions when code is
undefined; update the condition in handle-invalid-token-error (the block that
calls deleteToken() and setLogoutContextAndStatusBar()) to also trigger when
error.data?.code is undefined (e.g., error.data?.code === "UNAUTHORIZED" ||
error.data?.code === undefined or !("code" in error.data)), so
rotated-JWT-secret or missing-code cases will delete the token and set logout
context/status bar as intended.
In `@apps/vscode-extension/src/utils/fetch-initial-data.ts`:
- Around line 34-38: The catch path currently calls
handleInvalidTokenError(error) without awaiting it, causing race conditions
because handleInvalidTokenError is async and mutates state via deleteToken() and
setLogoutContextAndStatusBar(); update the code in fetch-initial-data.ts so that
inside the isTRPCClientError(error) branch you await
handleInvalidTokenError(error) (i.e., change the call to await
handleInvalidTokenError(error)) and ensure the enclosing function is async so
the await is valid, preserving existing logError(...) and error handling.
---
Outside diff comments:
In `@apps/api/src/auth/auth.service.ts`:
- Around line 307-316: The current flow creates a verification via
createEmailVerification and then sends a notice using emailService.sendEmail; if
the notice send fails the operation bubbles up while the verification was
already created. Change this to a best-effort notice: call
emailService.sendEmail inside a try/catch, and on error log the failure (include
context like verificationToken, message, existingUser.email and the caught
error) but do not rethrow so the verification creation is preserved; ensure the
surrounding function (the method containing the calls) still returns the created
verification details (message, verificationToken).
---
Nitpick comments:
In `@apps/api/src/common/utils/get-allowed-clients.ts`:
- Around line 12-20: Replace the implicit range generation in
ALLOWED_CLIENTS_IN_PRODUCTION with an explicit, auditable list of production
ports: remove the Array.from(...) + flatMap logic that generates
DASHBOARD_PRODUCTION_PORT + i and instead enumerate the specific production
ports you intend to allow (each producing http://localhost:PORT and
http://127.0.0.1:PORT) and keep the existing entries for DASHBOARD_PREVIEW_PORT;
update the ALLOWED_CLIENTS_IN_PRODUCTION constant so it only contains those
explicit origin strings derived from the specific ports.
🪄 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: bc2a03de-c095-4851-9510-d9762f423e17
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (59)
.husky/pre-commitapps/api/.prettierignoreapps/api/.prettierrcapps/api/Dockerfileapps/api/docker-entrypoint.shapps/api/package.jsonapps/api/src/analytics/dto/common.tsapps/api/src/analytics/routers/analytics.router.tsapps/api/src/auth/auth.guard.tsapps/api/src/auth/auth.module.tsapps/api/src/auth/auth.service.tsapps/api/src/auth/utils/validate-state-query-param.tsapps/api/src/common/constants.tsapps/api/src/common/utils/get-allowed-clients.tsapps/api/src/drizzle/schema/daily-data.tsapps/api/src/drizzle/schema/files.tsapps/api/src/drizzle/schema/password-resets.tsapps/api/src/email-verifications/email-verifications.dto.tsapps/api/src/email-verifications/email-verifications.service.tsapps/api/src/email/constants.tsapps/api/src/email/email.service.spec.tsapps/api/src/email/email.service.tsapps/api/src/email/utils/get-email-update-email-body.tsapps/api/src/email/utils/get-email-update-notice-email-body.tsapps/api/src/email/utils/get-onboarding-email-body.tsapps/api/src/email/utils/get-password-reset-email-body.tsapps/api/src/extension/extension.service.spec.tsapps/api/src/main.tsapps/api/src/password-resets/password-resets.service.spec.tsapps/api/src/trpc/trpc.service.tsapps/dashboard/package.jsonapps/dashboard/src/app/app.tsxapps/dashboard/src/components/errors/error-boundary.tsxapps/dashboard/src/features/auth/components/login-form.tsxapps/dashboard/src/utils/trpc.tsapps/vscode-extension/.vscode-test.tsapps/vscode-extension/eslint.config.tsapps/vscode-extension/package.jsonapps/vscode-extension/src/test/extension.test.tsapps/vscode-extension/src/utils/errors/handle-invalid-token-error.tsapps/vscode-extension/src/utils/fetch-initial-data.tsapps/vscode-extension/src/utils/periodic-sync-data.tsapps/vscode-extension/src/utils/trpc/client.tsapps/vscode-extension/vsc-extension-quickstart.mdapps/web/.prettierrcapps/web/Dockerfileapps/web/package.jsonpackage.jsonpackages/common/.prettierignorepackages/common/package.jsonpackages/common/src/constants.tspackages/common/src/convert-to-iso-date.tspackages/trpc/.prettierignorepackages/trpc/package.jsonpackages/ui/package.jsonpackages/ui/src/components/ui/collapsible.tsxpackages/ui/src/components/ui/form.tsxpackages/ui/src/lib/utils.tsturbo.json
💤 Files with no reviewable changes (3)
- packages/common/src/convert-to-iso-date.ts
- apps/api/src/email-verifications/email-verifications.dto.ts
- packages/common/src/constants.ts
- awaited the handleInvalidTokenError function in the `fetchInitialData` and `periodicSyncData` functions
- bumped the extension version to `v0.0.64`
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
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.
Commits
feat: email service
EmailVerificationsService. This responsibility is offloaded to theEmailService. Removed the corresponding dto and type in theemail-verifications.dtocreateEmailUpdatemethod and injected the EmailModule in the AuthModulefix: invalid token error
JWT_SECRETon the server side has been rotated and no longer recognizes the jwt sent by the extensionisTRPCClientErrorrefactor: trpc error handling and boundaries
isTRPCClientErrorfunction in the trpc fileUNAUTHORIZED, and the boundary is reset not only when the order of the before and after dates is changed but also when the period changesfix: constants
@repo/commonpackagechore: allowed clients and prettier sh plugin
ALLOWED_CLIENTSconstants and added a functiongetAllowedClientsto retrieve them depending of the NODE_ENVprettier-plugin-shto format bash scripts and Dockerfileschore: prettier and format
Summary by CodeRabbit
Bug Fixes
Chores