fix(integrations): prevent duplicate workspace linkage across orgs#4386
Merged
Conversation
Two different Superset orgs could OAuth to the same Linear/Slack workspace, leaving the webhook handler to pick a winner non- deterministically via `findFirst` — so each webhook landed in a random org. Affected 153 orgs and 167 connection rows in prod (worst case: 13 active orgs claiming one Linear workspace). - Schema: partial unique `(provider, external_org_id) WHERE disconnected_at IS NULL` on `integration_connections`. - Migration: backfills duplicate groups by keeping the most recently updated active row per `(provider, external_org_id)` and soft-disconnecting the rest with reason `duplicate_resolved`, then adds the constraint in the same file. - OAuth callbacks (Linear, Slack): pre-check for active linkage in another org and short-circuit with `?error=workspace_already_linked &owner=<connector email>`; also catch `23505` for the race window. - Webhook + Slack event handlers: filter `isNull(disconnectedAt)` and `orderBy desc(updatedAt)` so even if a stray duplicate slips in, routing is deterministic. - Error UI: surfaces the connector's email so the user knows who to ping to disconnect; toast call deferred via `setTimeout(0)` so it fires after Sonner's `<Toaster />` subscribes on hydration.
Contributor
📝 WalkthroughWalkthroughThis PR enforces one-to-one workspace-to-organization linking for Linear and Slack integrations by adding a partial unique database index, detecting conflicts proactively in OAuth callbacks, filtering event handlers to active connections, shipping email notification tooling for duplicate-resolved disconnects, and documenting a Linear teams/task identity refactor. ChangesWorkspace Uniqueness Enforcement
|Notifications: Email Template & Script| Linear Team Entity & Task Identity Refactoring Plan
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Brings back plans/20260501-linear-team-entity.md (deleted incidentally in #3893). Updated to drop the OAuth refresh workstream (already shipped via #4002), collapse the three-table teams design into a single teams entity with the counter on the row, and switch disconnect/reconnect to soft-delete so identifiers survive Linear disconnect/reconnect cycles.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes a data-integrity bug where multiple Superset orgs could simultaneously hold active OAuth connections to the same Linear or Slack workspace, causing non-deterministic webhook routing. It introduces a partial unique index
Confidence Score: 3/5The migration and schema changes are sound, but the Slack callback has a missing field reset that will silently break event delivery for any workspace that reconnects after a prior disconnection. The Slack UPSERT never resets disconnectedAt or disconnectReason to null on re-connect, and every Slack event handler added in this same PR now filters isNull(disconnectedAt). An org that disconnects and reconnects Slack will have their tokens refreshed but will receive no events — a silent regression that could go unnoticed until users complain. The fix is a two-line addition, but the gap is real and affects the main integration delivery path. apps/api/src/app/api/integrations/slack/callback/route.ts — the onConflictDoUpdate set block needs disconnectedAt and disconnectReason reset fields before this is safe to merge.
|
| Filename | Overview |
|---|---|
| apps/api/src/app/api/integrations/slack/callback/route.ts | Adds pre-check and 23505 catch for workspace collision, but the onConflictDoUpdate set omits disconnectedAt/disconnectReason reset — reconnected-after-disconnect Slack orgs will be invisible to all event handlers post-PR. |
| apps/api/src/app/api/integrations/linear/callback/route.ts | Adds conflict pre-check and race-safety 23505 catch; UPSERT correctly resets disconnectedAt/disconnectReason. Owner email passed in redirect URL is a minor PII concern. |
| packages/db/drizzle/0048_integration_connections_active_unique.sql | Correctly backfills duplicates using ROW_NUMBER keeping the most-recent row, then creates the partial unique index. NULL external_org_id rows handled safely by PostgreSQL NULL-distinct semantics. |
| packages/db/src/schema/schema.ts | Adds uniqueIndex for (provider, externalOrgId) WHERE disconnectedAt IS NULL; schema definition matches the migration SQL cleanly. |
| apps/api/src/app/api/integrations/linear/webhook/route.ts | Adds isNull(disconnectedAt) filter and desc(updatedAt) ordering to findFirst; deterministic routing is now enforced. |
| apps/api/src/app/api/integrations/slack/events/process-app-home-opened/process-app-home-opened.ts | Adds isNull/desc filters to findFirst, consistent with other Slack event handlers. |
| apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts | Adds isNull(disconnectedAt) and orderBy desc(updatedAt) to connection lookup; no issues. |
| apps/api/src/app/api/integrations/slack/events/process-entity-details/process-entity-details.ts | Adds isNull/desc ordering filters; no issues. |
| apps/api/src/app/api/integrations/slack/events/process-link-shared/process-link-shared.ts | Adds isNull/desc ordering filters; no issues. |
| apps/api/src/app/api/integrations/slack/events/process-mention/process-mention.ts | Adds isNull/desc ordering filters; no issues. |
| apps/api/src/app/api/integrations/slack/link/route.ts | Adds isNull/desc ordering filters to connection lookup; no issues. |
| apps/web/src/app/(dashboard-legacy)/integrations/linear/components/ErrorHandler/ErrorHandler.tsx | Handles workspace_already_linked with owner email display; setTimeout(0) correctly defers toast after Sonner hydration; cleanup via clearTimeout is correct. |
| apps/web/src/app/(dashboard-legacy)/integrations/slack/components/ErrorHandler/ErrorHandler.tsx | Same pattern as Linear ErrorHandler; correctly handles workspace_already_linked with and without owner email. |
Sequence Diagram
sequenceDiagram
participant User
participant OAuthProvider as Linear/Slack
participant Callback as API Callback
participant DB
User->>OAuthProvider: Initiate OAuth
OAuthProvider->>Callback: "GET /callback?code=..."
Callback->>DB: Verify membership
DB-->>Callback: OK
Callback->>OAuthProvider: Exchange code for token
OAuthProvider-->>Callback: access_token + team/org ID
Callback->>DB: "SELECT active connection for (provider, externalOrgId) WHERE disconnectedAt IS NULL AND orgId != currentOrg"
alt Conflict found
DB-->>Callback: "{email: owner@example.com}"
Callback-->>User: "Redirect to ?error=workspace_already_linked&owner=email"
else No conflict
DB-->>Callback: null
Callback->>DB: INSERT ... ON CONFLICT (orgId, provider) DO UPDATE SET tokens...
alt Race: 23505 unique violation
DB-->>Callback: ERROR 23505
Callback-->>User: "Redirect to ?error=workspace_already_linked"
else Success
DB-->>Callback: OK
Callback-->>User: Redirect to /integrations/linear or /integrations/slack
end
end
Comments Outside Diff (3)
-
apps/api/src/app/api/integrations/slack/callback/route.ts, line 113-126 (link)The Slack
onConflictDoUpdatenever resetsdisconnectedAtordisconnectReasontonull. Before this PR that was harmless, but every event handler in this PR now filtersisNull(disconnectedAt)— so an org that reconnects after a prior disconnection will have its tokens updated yet still be treated as disconnected, making every Slack event silently no-op for them. The Linear UPSERT already includes these two fields in itsset; Slack needs the same.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/api/src/app/api/integrations/slack/callback/route.ts Line: 113-126 Comment: The Slack `onConflictDoUpdate` never resets `disconnectedAt` or `disconnectReason` to `null`. Before this PR that was harmless, but every event handler in this PR now filters `isNull(disconnectedAt)` — so an org that reconnects after a prior disconnection will have its tokens updated yet still be treated as disconnected, making every Slack event silently no-op for them. The Linear UPSERT already includes these two fields in its `set`; Slack needs the same. How can I resolve this? If you propose a fix, please make it concise.
-
apps/api/src/app/api/integrations/slack/callback/route.ts, line 141-151 (link)Misleading error log for non-unique-violation DB failures
The
23505guard returns early, but every other exception — including genuine database errors, network timeouts, or ORM panics — falls through toconsole.error("[slack/callback] Token exchange failed:", error). That label belongs to the Slack API portion of the try block, not to DB failures, making production triage harder. Consider logging a more generic "unexpected error during Slack callback" or restructure the UPSERT into a nested try-catch (as was done in the Linear callback) so the Slack API error message stays accurate.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/api/src/app/api/integrations/slack/callback/route.ts Line: 141-151 Comment: **Misleading error log for non-unique-violation DB failures** The `23505` guard returns early, but every other exception — including genuine database errors, network timeouts, or ORM panics — falls through to `console.error("[slack/callback] Token exchange failed:", error)`. That label belongs to the Slack API portion of the try block, not to DB failures, making production triage harder. Consider logging a more generic "unexpected error during Slack callback" or restructure the UPSERT into a nested try-catch (as was done in the Linear callback) so the Slack API error message stays accurate. How can I resolve this? If you propose a fix, please make it concise.
-
apps/api/src/app/api/integrations/linear/callback/route.ts, line 63-108 (link)Owner email exposed in URL query parameter
The connector's email is appended verbatim to the redirect URL (
?owner=alice@example.com) and survives in browser history, server access logs, andRefererheaders if the user navigates elsewhere from the error page.ErrorHandlerreads it back fromsearchParamsbeforewindow.history.replaceStateclears the URL, but the window between redirect arrival and theuseEffectcleanup means the email is briefly visible in the address bar and already in history. Passing a short-lived token or session-scoped value that the frontend exchanges server-side would avoid persisting PII in the URL. Same pattern is present in the Slack callback.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/api/src/app/api/integrations/linear/callback/route.ts Line: 63-108 Comment: **Owner email exposed in URL query parameter** The connector's email is appended verbatim to the redirect URL (`?owner=alice@example.com`) and survives in browser history, server access logs, and `Referer` headers if the user navigates elsewhere from the error page. `ErrorHandler` reads it back from `searchParams` before `window.history.replaceState` clears the URL, but the window between redirect arrival and the `useEffect` cleanup means the email is briefly visible in the address bar and already in history. Passing a short-lived token or session-scoped value that the frontend exchanges server-side would avoid persisting PII in the URL. Same pattern is present in the Slack callback. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/api/src/app/api/integrations/slack/callback/route.ts:113-126
The Slack `onConflictDoUpdate` never resets `disconnectedAt` or `disconnectReason` to `null`. Before this PR that was harmless, but every event handler in this PR now filters `isNull(disconnectedAt)` — so an org that reconnects after a prior disconnection will have its tokens updated yet still be treated as disconnected, making every Slack event silently no-op for them. The Linear UPSERT already includes these two fields in its `set`; Slack needs the same.
```suggestion
.onConflictDoUpdate({
target: [
integrationConnections.organizationId,
integrationConnections.provider,
],
set: {
accessToken: tokenData.access_token,
externalOrgId: tokenData.team.id,
externalOrgName: tokenData.team.name,
connectedByUserId: userId,
config,
disconnectedAt: null,
disconnectReason: null,
updatedAt: new Date(),
},
});
```
### Issue 2 of 3
apps/api/src/app/api/integrations/slack/callback/route.ts:141-151
**Misleading error log for non-unique-violation DB failures**
The `23505` guard returns early, but every other exception — including genuine database errors, network timeouts, or ORM panics — falls through to `console.error("[slack/callback] Token exchange failed:", error)`. That label belongs to the Slack API portion of the try block, not to DB failures, making production triage harder. Consider logging a more generic "unexpected error during Slack callback" or restructure the UPSERT into a nested try-catch (as was done in the Linear callback) so the Slack API error message stays accurate.
### Issue 3 of 3
apps/api/src/app/api/integrations/linear/callback/route.ts:63-108
**Owner email exposed in URL query parameter**
The connector's email is appended verbatim to the redirect URL (`?owner=alice@example.com`) and survives in browser history, server access logs, and `Referer` headers if the user navigates elsewhere from the error page. `ErrorHandler` reads it back from `searchParams` before `window.history.replaceState` clears the URL, but the window between redirect arrival and the `useEffect` cleanup means the email is briefly visible in the address bar and already in history. Passing a short-lived token or session-scoped value that the frontend exchanges server-side would avoid persisting PII in the URL. Same pattern is present in the Slack callback.
Reviews (1): Last reviewed commit: "docs(plans): restore linear teams + per-..." | Re-trigger Greptile
![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: 5
🤖 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/api/src/app/api/integrations/linear/callback/route.ts`:
- Around line 92-93: The redirect currently appends a cross-tenant connector
email (selected via select({ email: users.email }) on integrationConnections)
into the URL query params, which leaks PII; instead stop including the email in
the redirect URL and return a generic conflict response or an opaque
backend-issued token. Locate the logic in route.ts where the redirect URL or
query params are built after the integrationConnections lookup (search for
select({ email: users.email }) and the redirect/response construction), remove
the email from the query string, and either (a) replace it with a
non-identifying static conflict message (e.g., ?status=conflict) or (b) generate
a one-time opaque token stored server-side (in DB or cache) that maps to the
conflict details and return that token in the redirect so the frontend can fetch
non-PII details.
In `@apps/api/src/app/api/integrations/slack/callback/route.ts`:
- Around line 90-92: The upsert update branch for reconnecting integrations is
not clearing the disconnected flag fields, so although lookups require
isNull(integrationConnections.disconnectedAt) a reconnect can remain logically
disconnected; in the upsert/update code path (the block that updates an existing
integration connection record—referencing integrationConnections and the
upsert/update call around lines 118-125) set disconnectedAt = null and
disconnectReason = null (or remove the disconnect reason) when reactivating,
ensuring the update clears those columns and any related status fields so the
connection is treated as active for subsequent lookups and webhook/event
routing.
- Around line 83-84: The redirect URL is currently leaking user email via query
params (built from the users.email selected from integrationConnections); remove
the PII by not appending owner=users.email to the redirect (or replace it with a
non-PII identifier/state token and persist any mapping server-side). Locate the
code that selects email (.select({ email: users.email }) /
integrationConnections) and the subsequent redirect/res.redirect construction
(the block that appends owner=... on lines ~98-99), stop adding the email to the
URL, and instead return a generic status (e.g., success=true or a short opaque
state id) or a server-stored mapping for later retrieval.
In
`@apps/api/src/app/api/integrations/slack/events/process-entity-details/process-entity-details.ts`:
- Around line 41-42: The current query's orderBy uses only
integrationConnections.updatedAt which can produce nondeterministic results when
timestamps tie; update the orderBy passed to the findFirst (or the query
building code that uses orderBy) to include a stable secondary tiebreaker such
as id: 'desc' (e.g., add integrationConnections.id DESC or the primary id DESC)
after integrationConnections.updatedAt so selection becomes deterministic;
locate the orderBy usage around orderBy: desc(integrationConnections.updatedAt)
in process-entity-details.ts and augment it with the secondary sort key.
In `@plans/20260501-linear-team-entity.md`:
- Line 180: Add a language tag to the unlabeled fenced code block containing the
example starting with `input = "SUPER-103" or UUID` to satisfy markdownlint
MD040; edit the fence in the `plans/20260501-linear-team-entity.md` block that
opens before that example (the triple-backtick fence around the sample) and
change it to a labeled fence such as ```text or ```ts so the code block is
explicitly annotated.
🪄 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: 1507e22b-e945-46e9-aa0a-1ca308f04e8a
📒 Files selected for processing (16)
apps/api/src/app/api/integrations/linear/callback/route.tsapps/api/src/app/api/integrations/linear/webhook/route.tsapps/api/src/app/api/integrations/slack/callback/route.tsapps/api/src/app/api/integrations/slack/events/process-app-home-opened/process-app-home-opened.tsapps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.tsapps/api/src/app/api/integrations/slack/events/process-entity-details/process-entity-details.tsapps/api/src/app/api/integrations/slack/events/process-link-shared/process-link-shared.tsapps/api/src/app/api/integrations/slack/events/process-mention/process-mention.tsapps/api/src/app/api/integrations/slack/link/route.tsapps/web/src/app/(dashboard-legacy)/integrations/linear/components/ErrorHandler/ErrorHandler.tsxapps/web/src/app/(dashboard-legacy)/integrations/slack/components/ErrorHandler/ErrorHandler.tsxpackages/db/drizzle/0048_integration_connections_active_unique.sqlpackages/db/drizzle/meta/0048_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/schema.tsplans/20260501-linear-team-entity.md
Comment on lines
+92
to
+93
| .select({ email: users.email }) | ||
| .from(integrationConnections) |
Contributor
There was a problem hiding this comment.
Avoid leaking another org’s connector email in redirect query params.
Line 107 includes a cross-tenant email in the URL. That exposes PII through URL logs/history/referrers. Return a generic conflict message (or a backend-issued opaque token), not raw email.
Suggested patch
- return Response.redirect(
- `${env.NEXT_PUBLIC_WEB_URL}/integrations/linear?error=workspace_already_linked&owner=${encodeURIComponent(conflict.email)}`,
- );
+ return Response.redirect(
+ `${env.NEXT_PUBLIC_WEB_URL}/integrations/linear?error=workspace_already_linked`,
+ );Also applies to: 107-108
🤖 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/api/src/app/api/integrations/linear/callback/route.ts` around lines 92 -
93, The redirect currently appends a cross-tenant connector email (selected via
select({ email: users.email }) on integrationConnections) into the URL query
params, which leaks PII; instead stop including the email in the redirect URL
and return a generic conflict response or an opaque backend-issued token. Locate
the logic in route.ts where the redirect URL or query params are built after the
integrationConnections lookup (search for select({ email: users.email }) and the
redirect/response construction), remove the email from the query string, and
either (a) replace it with a non-identifying static conflict message (e.g.,
?status=conflict) or (b) generate a one-time opaque token stored server-side (in
DB or cache) that maps to the conflict details and return that token in the
redirect so the frontend can fetch non-PII details.
Comment on lines
+83
to
+84
| .select({ email: users.email }) | ||
| .from(integrationConnections) |
Contributor
There was a problem hiding this comment.
Do not place connector email in the redirect URL.
Line 98 leaks cross-org user email via query params (owner=...), which can persist in logs/history/referrers. Prefer a generic message without PII in the URL.
Suggested patch
- return Response.redirect(
- `${env.NEXT_PUBLIC_WEB_URL}/integrations/slack?error=workspace_already_linked&owner=${encodeURIComponent(conflict.email)}`,
- );
+ return Response.redirect(
+ `${env.NEXT_PUBLIC_WEB_URL}/integrations/slack?error=workspace_already_linked`,
+ );Also applies to: 98-99
🤖 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/api/src/app/api/integrations/slack/callback/route.ts` around lines 83 -
84, The redirect URL is currently leaking user email via query params (built
from the users.email selected from integrationConnections); remove the PII by
not appending owner=users.email to the redirect (or replace it with a non-PII
identifier/state token and persist any mapping server-side). Locate the code
that selects email (.select({ email: users.email }) / integrationConnections)
and the subsequent redirect/res.redirect construction (the block that appends
owner=... on lines ~98-99), stop adding the email to the URL, and instead return
a generic status (e.g., success=true or a short opaque state id) or a
server-stored mapping for later retrieval.
Adds the email template and a runnable script for notifying the 92 users whose Linear/Slack connections were soft-disconnected by the 0048 migration. Bullets out each affected (org, provider, workspace, new-owner-email) tuple so the recipient can see exactly what changed and reach out to the current owner if they want to take over. Script supports --dry-run, --test[=email], and --send. Requires NEXT_PUBLIC_MARKETING_URL=https://superset.sh on the command line so the logo/social icons in the email body resolve from prod rather than the localhost URL baked into root .env.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/email/scripts/notify-disconnected-integrations.ts (1)
94-103: ⚡ Quick winAdd runtime validation for the provider field.
On line 100, the code assumes
r.provideris either"linear"or"slack"and maps them to capitalized display names. If the database contains an unexpected provider value (due to future additions or data corruption), the ternary will silently fall back to"Slack"for any non-"linear"value, which could confuse recipients.♻️ Proposed validation to make provider mapping explicit
return rows.map((r) => ({ recipientEmail: r.recipientEmail, recipientName: r.recipientName, connection: { orgName: r.orgName, workspaceName: r.workspaceName ?? "(unnamed workspace)", - provider: r.provider === "linear" ? "Linear" : "Slack", + provider: r.provider === "linear" + ? "Linear" + : r.provider === "slack" + ? "Slack" + : (() => { throw new Error(`Unknown provider: ${r.provider}`); })(), winnerEmail: r.winnerEmail, }, }));Or use a mapping object for clarity:
+const PROVIDER_DISPLAY: Record<string, "Linear" | "Slack"> = { + linear: "Linear", + slack: "Slack", +}; + return rows.map((r) => ({ recipientEmail: r.recipientEmail, recipientName: r.recipientName, connection: { orgName: r.orgName, workspaceName: r.workspaceName ?? "(unnamed workspace)", - provider: r.provider === "linear" ? "Linear" : "Slack", + provider: PROVIDER_DISPLAY[r.provider] ?? (() => { + throw new Error(`Unknown provider: ${r.provider}`); + })(), winnerEmail: r.winnerEmail, }, }));🤖 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/email/scripts/notify-disconnected-integrations.ts` around lines 94 - 103, The provider mapping in the rows.map return assumes r.provider is "linear" or "slack" and currently uses a ternary that treats any non-"linear" value as "Slack"; update the mapping in the notify-disconnected-integrations.ts transformation (the rows.map callback that builds connection.provider) to validate r.provider at runtime using an explicit mapping object or switch (e.g., { linear: "Linear", slack: "Slack" }) and fall back to a safe default like "Unknown" or the raw provider value when r.provider is not recognized; ensure the new logic is used where connection.provider is set so unexpected or corrupted DB values do not silently show as "Slack".packages/email/src/emails/integration-disconnected.tsx (1)
76-76: 💤 Low valueConsider extracting the hardcoded integration URL to a shared constant.
The URL
https://app.superset.sh/integrationsis hardcoded here. If the integration route changes or if you need environment-specific URLs (staging/prod), this will require updates across multiple files.♻️ Suggested refactor to use a shared constant
Define a constant in a shared config/constants file:
// e.g., packages/email/src/constants.ts export const INTEGRATIONS_URL = "https://app.superset.sh/integrations";Then import and use it:
+import { INTEGRATIONS_URL } from "../constants"; + // ... <Section className="mt-6 mb-6"> - <Button href="https://app.superset.sh/integrations"> + <Button href={INTEGRATIONS_URL}> Open Integrations </Button> </Section>🤖 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/email/src/emails/integration-disconnected.tsx` at line 76, Extract the hardcoded integration URL used in integration-disconnected.tsx (the Button href "https://app.superset.sh/integrations") into a shared constant (e.g., INTEGRATIONS_URL) exported from a central constants/config file (for example packages/email/src/constants.ts), then import that constant into integration-disconnected.tsx and replace the literal in the Button href with INTEGRATIONS_URL; ensure the constant export name matches the import and consider making the constant driven by an environment variable if you need staging/prod overrides.
🤖 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.
Nitpick comments:
In `@packages/email/scripts/notify-disconnected-integrations.ts`:
- Around line 94-103: The provider mapping in the rows.map return assumes
r.provider is "linear" or "slack" and currently uses a ternary that treats any
non-"linear" value as "Slack"; update the mapping in the
notify-disconnected-integrations.ts transformation (the rows.map callback that
builds connection.provider) to validate r.provider at runtime using an explicit
mapping object or switch (e.g., { linear: "Linear", slack: "Slack" }) and fall
back to a safe default like "Unknown" or the raw provider value when r.provider
is not recognized; ensure the new logic is used where connection.provider is set
so unexpected or corrupted DB values do not silently show as "Slack".
In `@packages/email/src/emails/integration-disconnected.tsx`:
- Line 76: Extract the hardcoded integration URL used in
integration-disconnected.tsx (the Button href
"https://app.superset.sh/integrations") into a shared constant (e.g.,
INTEGRATIONS_URL) exported from a central constants/config file (for example
packages/email/src/constants.ts), then import that constant into
integration-disconnected.tsx and replace the literal in the Button href with
INTEGRATIONS_URL; ensure the constant export name matches the import and
consider making the constant driven by an environment variable if you need
staging/prod overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 509639c2-dafb-4cea-8915-8253271c9d9b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/email/package.jsonpackages/email/scripts/notify-disconnected-integrations.tspackages/email/src/emails/integration-disconnected.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/email/scripts/notify-disconnected-integrations.ts">
<violation number="1" location="packages/email/scripts/notify-disconnected-integrations.ts:100">
P2: Provider mapping is non-exhaustive and mislabels any non-Linear provider as Slack.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| connection: { | ||
| orgName: r.orgName, | ||
| workspaceName: r.workspaceName ?? "(unnamed workspace)", | ||
| provider: r.provider === "linear" ? "Linear" : "Slack", |
Contributor
There was a problem hiding this comment.
P2: Provider mapping is non-exhaustive and mislabels any non-Linear provider as Slack.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/email/scripts/notify-disconnected-integrations.ts, line 100:
<comment>Provider mapping is non-exhaustive and mislabels any non-Linear provider as Slack.</comment>
<file context>
@@ -0,0 +1,216 @@
+ connection: {
+ orgName: r.orgName,
+ workspaceName: r.workspaceName ?? "(unnamed workspace)",
+ provider: r.provider === "linear" ? "Linear" : "Slack",
+ winnerEmail: r.winnerEmail,
+ },
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
- Slack callback UPSERT now clears disconnectedAt + disconnectReason on reconnect, matching the Linear callback. Without this, a Slack reconnect after a soft-disconnect would leave the row hidden from the new isNull(disconnectedAt) filter in event handlers and webhooks would silently no-op. - Webhook + 6 Slack lookup sites now order by (updatedAt desc, id desc) so findFirst is deterministic on equal timestamps. Belt-and-suspenders; the partial unique constraint makes ties impossible in practice. - Tag the resolver pseudo-code fence in the slug plan as `text` for markdownlint.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plans/20260501-linear-team-entity.md`:
- Around line 112-113: Decide on one canonical behavior for default-team
creation (either "Every org gets a default team" or "lazy creation only when
tasks exist"), then update all occurrences of both phrases ("Every org gets a
default team" and "lazy creation avoids creating empty teams for orgs without
tasks") so the doc consistently states the chosen behavior; ensure the
migration/TS script plan, the section describing team lifecycle, and any
examples all reflect the same policy and remove the contradictory sentence(s).
- Around line 132-135: The CTE's non-deterministic team_id subquery uses "SELECT
... LIMIT 1" and should be replaced with an explicit deterministic reference to
the org's default team; update the numbered CTE (and any references to team_id)
to select or join the teams row that is explicitly marked as the default (e.g.,
join teams tm ON tm.organization_id = t.organization_id AND tm.is_default =
TRUE, or use the organizations.default_team_id if available) so the selection of
team_id in the numbered CTE is deterministic across reruns.
🪄 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: 34f10c21-3e96-49c3-a753-b923c2086670
📒 Files selected for processing (9)
apps/api/src/app/api/integrations/linear/webhook/route.tsapps/api/src/app/api/integrations/slack/callback/route.tsapps/api/src/app/api/integrations/slack/events/process-app-home-opened/process-app-home-opened.tsapps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.tsapps/api/src/app/api/integrations/slack/events/process-entity-details/process-entity-details.tsapps/api/src/app/api/integrations/slack/events/process-link-shared/process-link-shared.tsapps/api/src/app/api/integrations/slack/events/process-mention/process-mention.tsapps/api/src/app/api/integrations/slack/link/route.tsplans/20260501-linear-team-entity.md
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/api/src/app/api/integrations/slack/events/process-mention/process-mention.ts
- apps/api/src/app/api/integrations/linear/webhook/route.ts
- apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts
- apps/api/src/app/api/integrations/slack/events/process-link-shared/process-link-shared.ts
- apps/api/src/app/api/integrations/slack/link/route.ts
- apps/api/src/app/api/integrations/slack/callback/route.ts
- apps/api/src/app/api/integrations/slack/events/process-app-home-opened/process-app-home-opened.ts
- apps/api/src/app/api/integrations/slack/events/process-entity-details/process-entity-details.ts
Comment on lines
+112
to
+113
| Single Drizzle migration + one deploy-time TS script. Every org gets a default team; every task flattens into that team's number space. | ||
|
|
Contributor
There was a problem hiding this comment.
Resolve the default-team creation contradiction.
The doc currently says “Every org gets a default team” but later says lazy creation avoids creating empty teams for orgs without tasks. Pick one behavior and make both sections consistent to prevent divergent implementation.
Also applies to: 266-267
🤖 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 `@plans/20260501-linear-team-entity.md` around lines 112 - 113, Decide on one
canonical behavior for default-team creation (either "Every org gets a default
team" or "lazy creation only when tasks exist"), then update all occurrences of
both phrases ("Every org gets a default team" and "lazy creation avoids creating
empty teams for orgs without tasks") so the doc consistently states the chosen
behavior; ensure the migration/TS script plan, the section describing team
lifecycle, and any examples all reflect the same policy and remove the
contradictory sentence(s).
Comment on lines
+132
to
+135
| WITH numbered AS ( | ||
| SELECT t.id, | ||
| (SELECT id FROM teams tm WHERE tm.organization_id = t.organization_id LIMIT 1) AS team_id, | ||
| ROW_NUMBER() OVER (PARTITION BY t.organization_id ORDER BY t.created_at, t.id) AS num |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l plans/20260501-linear-team-entity.mdRepository: superset-sh/superset
Length of output: 105
🏁 Script executed:
sed -n '120,150p' plans/20260501-linear-team-entity.md | cat -nRepository: superset-sh/superset
Length of output: 1744
🏁 Script executed:
sed -n '100,120p' plans/20260501-linear-team-entity.md | cat -nRepository: superset-sh/superset
Length of output: 817
🏁 Script executed:
# Check for mentions of "default team" or "step 2" context
rg -n "default.*team|step 2" plans/20260501-linear-team-entity.md -A 2 -B 2Repository: superset-sh/superset
Length of output: 3018
🏁 Script executed:
# Check lines around 112 and 266 mentioned in the scratchpad
sed -n '110,115p' plans/20260501-linear-team-entity.md | cat -n
sed -n '264,270p' plans/20260501-linear-team-entity.md | cat -nRepository: superset-sh/superset
Length of output: 722
Make backfill team selection more explicit: use a deterministic reference instead of LIMIT 1.
The SELECT ... LIMIT 1 approach is non-deterministic: if a rerun or partial migration creates multiple team rows before step 4 executes, the result is undefined. Although step 2 guarantees exactly one default team per org, joining explicitly through that team makes the intent clearer and protects against future refactors.
Suggested doc patch
+-- 4. Set tasks.team_id and tasks.number — flatten everything into the org's default team.
+-- Join to the default team created in step 2 (deterministic, not LIMIT 1):
-WITH numbered AS (
+WITH default_team AS (
+ SELECT organization_id, id AS team_id
+ FROM teams
+ WHERE created_during_migration = true
+), numbered AS (
SELECT t.id,
- (SELECT id FROM teams tm WHERE tm.organization_id = t.organization_id LIMIT 1) AS team_id,
+ dt.team_id,
ROW_NUMBER() OVER (PARTITION BY t.organization_id ORDER BY t.created_at, t.id) AS num
FROM tasks t
+ JOIN default_team dt ON dt.organization_id = t.organization_id
)📝 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.
Suggested change
| WITH numbered AS ( | |
| SELECT t.id, | |
| (SELECT id FROM teams tm WHERE tm.organization_id = t.organization_id LIMIT 1) AS team_id, | |
| ROW_NUMBER() OVER (PARTITION BY t.organization_id ORDER BY t.created_at, t.id) AS num | |
| -- 4. Set tasks.team_id and tasks.number — flatten everything into the org's default team. | |
| -- Join to the default team created in step 2 (deterministic, not LIMIT 1): | |
| WITH default_team AS ( | |
| SELECT organization_id, id AS team_id | |
| FROM teams | |
| WHERE created_during_migration = true | |
| ), numbered AS ( | |
| SELECT t.id, | |
| dt.team_id, | |
| ROW_NUMBER() OVER (PARTITION BY t.organization_id ORDER BY t.created_at, t.id) AS num | |
| FROM tasks t | |
| JOIN default_team dt ON dt.organization_id = t.organization_id | |
| ) |
🤖 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 `@plans/20260501-linear-team-entity.md` around lines 132 - 135, The CTE's
non-deterministic team_id subquery uses "SELECT ... LIMIT 1" and should be
replaced with an explicit deterministic reference to the org's default team;
update the numbered CTE (and any references to team_id) to select or join the
teams row that is explicitly marked as the default (e.g., join teams tm ON
tm.organization_id = t.organization_id AND tm.is_default = TRUE, or use the
organizations.default_team_id if available) so the selection of team_id in the
numbered CTE is deterministic across reruns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two different Superset orgs could OAuth to the same Linear/Slack workspace because nothing stopped them at the schema or callback layer. The webhook handler then picked a winner non-deterministically via
findFirst, so each webhook landed in a random org. In prod this affected 153 orgs / 167 connection rows (worst case: 13 active orgs all claiming the 당근 Linear workspace).This is also the underlying reason "Linear → Superset doesn't sync" surfaced for the Superset org — Kiet's stale org row was winning the routing roulette ~50% of the time. The migration in this PR resolves that incidentally.
Changes
unique(provider, external_org_id) WHERE disconnected_at IS NULLonintegration_connections.0048_integration_connections_active_unique): single file thatupdated_atactive row per(provider, external_org_id), soft-disconnects the rest (disconnect_reason = 'duplicate_resolved'), then?error=workspace_already_linked&owner=<connector_email>. Catch23505on the UPSERT as a race-safety fallback.isNull(disconnectedAt)andorderBy desc(updatedAt)on thefindFirstso even a stray duplicate routes deterministically (belt-and-suspenders alongside the constraint).ErrorHandlerfor Linear + Slack): renders the connector's email so the user knows who to ping. Toast call deferred viasetTimeout(0)so it fires after Sonner's<Toaster />subscribes on hydration (otherwise it was dropped on first render and only appeared after the next re-mount).Test plan
integration_connections_provider_external_org_active_unique(matches the catch-path constant).Out of scope / follow-ups
plans/20260501-linear-team-entity.md.Summary by cubic
Prevents multiple orgs from linking the same Linear/Slack workspace and makes webhook/event routing deterministic. Adds a one‑shot email script to notify users whose connections were auto‑disconnected.
Bug Fixes
?error=workspace_already_linked(owner email only on the pre‑check path).disconnectedAtanddisconnectReasonin the callback UPSERT.updatedAt,id(desc) for deterministic selection.Migration
(provider, external_org_id)and soft‑disconnect the rest with reasonduplicate_resolved.unique(provider, external_org_id) WHERE disconnected_at IS NULL.--dry-run,--test[=email], and--send.Written for commit bba2046. Summary will update on new commits.
Summary by CodeRabbit