fix(webapp): remove deleted accounts from Loops marketing emails (#3010)#3014
fix(webapp): remove deleted accounts from Loops marketing emails (#3010)#3014JosephDoUrden wants to merge 3 commits intotriggerdotdev:mainfrom
Conversation
…erdotdev#3010) When a user deletes their last organization, remove their contact from Loops to prevent future marketing emails. - Idempotent: safe to call multiple times - Non-blocking: Loops failures do not block org deletion - Conditional: only runs when user has no remaining organizations
|
WalkthroughConverts LoopsClient to an exported class with an injectable logger and adds a new method Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (8)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/services/**/*.server.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/webapp/test/loopsClient.test.ts`:
- Around line 6-32: The test defines a local copy of LoopsClient (including
deleteContact) which diverges from production; export the real LoopsClient as a
named export from apps/webapp/app/services/loops.server.ts (while keeping the
existing singleton loopsClient export), remove the locally-declared LoopsClient
from apps/webapp/test/loopsClient.test.ts, and update that test to import {
LoopsClient } from the server module and instantiate it with a test API key; if
LoopsClient relies on a logger, either add an optional logger parameter to the
LoopsClient constructor (so tests can pass a no-op or test logger) or allow the
real logger to run in tests — ensure deleteContact in the imported class is used
so production regressions are caught (also apply the same exported-class pattern
for related code such as deleteOrganization.server.ts).
🧹 Nitpick comments (3)
apps/webapp/app/services/deleteOrganization.server.ts (2)
103-106: Add.catch()to the fire-and-forget promise to guard against future regressions.While
deleteContactcurrently catches all errors internally, an unguarded floating promise is fragile — any future refactor that lets an exception escape will produce an unhandled rejection. A one-liner.catch()makes this defensive.Proposed fix
if (user) { // Fire and forget - don't block deletion on Loops API - loopsClient?.deleteContact({ email: user.email }); + loopsClient?.deleteContact({ email: user.email }).catch(() => {}); }
6-6: SingletonloopsClientimport couples this service toenv.server.ts, making it harder to test.Per the coding guidelines, services in
apps/webapp/app/services/should separate testable code from configuration (singleton) files, and test files should not importenv.server.tsdirectly or indirectly. Consider accepting an optionalloopsClientparameter (e.g., via the constructor or method options) so the dependency can be injected in tests.This is the same pattern already recommended for other services (e.g.,
realtimeClient.server.tsvsrealtimeClientGlobal.server.ts).As per coding guidelines,
apps/webapp/app/services/**/*.server.{ts,tsx}: "Separate testable services from configuration files; follow the pattern ofrealtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp".Also applies to: 86-107
apps/webapp/test/loopsClient.test.ts (1)
34-45: Mockingglobal.fetch— consider alternatives.The coding guidelines state tests should avoid mocks. For external HTTP APIs where testcontainers don't apply, a lightweight HTTP interceptor like msw (Mock Service Worker) would be more aligned with the spirit of the guideline since it intercepts at the network level rather than replacing globals. That said, this is a pragmatic choice for a simple HTTP call and lower priority than fixing the duplicated class issue above.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/app/services/loops.server.tsapps/webapp/test/loopsClient.test.ts
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/test/loopsClient.test.tsapps/webapp/app/services/loops.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/test/loopsClient.test.tsapps/webapp/app/services/loops.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/test/loopsClient.test.tsapps/webapp/app/services/loops.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/app/services/loops.server.ts
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/app/services/loops.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/test/loopsClient.test.tsapps/webapp/app/services/loops.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/test/loopsClient.test.tsapps/webapp/app/services/loops.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/test/loopsClient.test.tsapps/webapp/app/services/loops.server.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/loopsClient.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Test files should only import classes and functions from
app/**/*.tsfiles and should not importenv.server.tsdirectly or indirectly; pass configuration through options insteadFor testable code in the webapp, never import env.server.ts in test files - pass configuration as options instead
Files:
apps/webapp/test/loopsClient.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
**/*.test.{ts,tsx,js,jsx}: Use vitest exclusively for testing and never mock anything - use testcontainers instead
Place test files next to source files with naming pattern: source file (e.g.,MyService.ts) →MyService.test.ts
Files:
apps/webapp/test/loopsClient.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers helpers (
redisTest,postgresTest,containerTest) from@internal/testcontainersfor Redis/PostgreSQL testing instead of mocks
Files:
apps/webapp/test/loopsClient.test.ts
🧬 Code graph analysis (1)
apps/webapp/app/services/deleteOrganization.server.ts (1)
apps/webapp/app/services/loops.server.ts (1)
loopsClient(111-111)
🔇 Additional comments (1)
apps/webapp/app/services/loops.server.ts (1)
25-52: Clean implementation ofdeleteContact.The error handling, 404-as-success treatment, and
encodeURIComponentfor the email are all correct. The method integrates well with the existing patterns in the class.One minor consideration: the fire-and-forget caller in
deleteOrganization.server.tsmeans unhandled promise rejections won't surface, but sincedeleteContactalready wraps everything in try/catch and returnsboolean, there's no risk of an unhandled rejection escaping.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…erdotdev#3010) When a user deletes their last organization, remove their contact from Loops to prevent future marketing emails. - Add deleteContact() using the correct Loops v1 endpoint (POST /api/v1/contacts/delete) - Extract LoopsClient into a testable class (separate env singleton) - Wire Loops deletion into the organization deletion flow - Only trigger Loops removal when the user has no remaining organizations
Summary
Changes
Test plan
Edge cases handled
Fixes #3010 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/services/loops.server.ts`:
- Around line 46-58: The code currently returns false on any non-OK fetch
response, which treats HTTP 404 (contact not found) as failure; modify the
delete flow so that after the fetch you check response.status === 404 and, in
that case, log an info like "Loops contact already deleted" with the email and
return true; otherwise if !response.ok continue to log the error and return
false as before, then parse response.json() and keep the existing
responseBody.message "Contact not found." branch for 200 responses; update
related tests to assert that a 404 response yields a successful result.
🧹 Nitpick comments (2)
apps/webapp/app/services/loopsGlobal.server.ts (1)
4-4: Consider wrapping insingleton()for HMR safety.Other global instances in the webapp (e.g.,
telemetryintelemetry.server.tsat line 225) use thesingleton()utility to survive Remix hot-module reloading in development. Without it,loopsClientwill be re-instantiated on every HMR cycle. Not a functional issue sinceLoopsClientis stateless, but it would be consistent with the existing codebase patterns.♻️ Optional: wrap with singleton
import { env } from "~/env.server"; import { LoopsClient } from "./loops.server"; +import { singleton } from "~/utils/singleton"; -export const loopsClient = env.LOOPS_API_KEY ? new LoopsClient(env.LOOPS_API_KEY) : null; +export const loopsClient = env.LOOPS_API_KEY + ? singleton("loopsClient", () => new LoopsClient(env.LOOPS_API_KEY!)) + : null;apps/webapp/app/services/loops.server.ts (1)
51-51: Consider using zod for response validation.The type assertion
as { success: boolean; message?: string }works but doesn't provide runtime safety if the API response shape changes. The existing#sendEvent(line 113) usesas any, so this is already an improvement in type specificity. Per coding guidelines, zod is preferred for validation in the webapp — but this is a minor consistency point given the existing pattern.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webapp/app/services/deleteOrganization.server.tsapps/webapp/app/services/loops.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/telemetry.server.tsapps/webapp/test/loopsClient.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/services/deleteOrganization.server.ts
- apps/webapp/test/loopsClient.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.tsapps/webapp/app/services/loops.server.ts
🧠 Learnings (2)
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/services/**/*.server.{ts,tsx} : Separate testable services from configuration files; follow the pattern of `realtimeClient.server.ts` (testable service) and `realtimeClientGlobal.server.ts` (configuration) in the webapp
Applied to files:
apps/webapp/app/services/telemetry.server.tsapps/webapp/app/services/loopsGlobal.server.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `trigger.dev/core` in the webapp, use subpath exports from the package.json instead of importing from the root path
Applied to files:
apps/webapp/app/services/telemetry.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/services/loopsGlobal.server.ts (1)
apps/webapp/app/services/loops.server.ts (1)
LoopsClient(5-128)
apps/webapp/app/services/loops.server.ts (1)
packages/core/src/logger.ts (1)
Logger(19-139)
🔇 Additional comments (3)
apps/webapp/app/services/telemetry.server.ts (1)
8-8: LGTM!Import correctly updated to point to the new singleton configuration module, consistent with the project's pattern of separating testable services (
loops.server.ts) from their configuration/singleton files (loopsGlobal.server.ts). Based on learnings: "Separate testable services from configuration files; follow the pattern ofrealtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp."apps/webapp/app/services/loops.server.ts (2)
1-13: Clean refactor to an injectable, testable class.The
Loggertype alias, injectable constructor, and separation from the singleton instantiation all follow best practices and align with the project's service/configuration split pattern.
73-127: LGTM — logger updates are consistent.Mechanical updates to use
this.#loggerinstead of the previous module-level logger. No logic changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
When deleting a Loops contact, a 404 response indicates the contact doesn’t exist, which is already the desired end state. Handle this explicitly by returning true instead of falling through to the generic error path. - Treat HTTP 404 from Loops delete endpoint as success - Add test coverage for the 404 case
|
@JosephDoUrden I think a better solution for this is to add the "Delete account" button to the User profile. We currently do this manually when contacted by users but haven't built the automated flow yet (it involves quite a few steps if they have active projects). With your solution, you could delete an org and add a new one and then you wouldn't get the emails anymore. I think it needs to be tied to users. |
2 participants
Summary
deleteContact()toLoopsClientfor removing users from LoopsTest plan
deleteContact()(6 cases)Loops deleting contactappears in logsEdge cases handled
loopsClientis null, operation becomes a no-opFixes #3010