refactor: deduplicate shared logic across monorepo#1472
Conversation
…ient Move duplicate tRPC client code (react.tsx, server.tsx, query-client.ts) from apps/web and apps/admin into packages/trpc/src/client/ with factory functions. Each app now uses thin wrappers that pass env-specific config.
Move the org membership query into @superset/db/utils so both tRPC procedures and API route handlers share the same base query instead of duplicating the drizzle query inline across 7+ files.
Deduplicate image upload logic (mime validation, base64 decoding, size check, old image deletion, blob upload) from uploadAvatar and uploadLogo into a shared uploadImage utility in packages/trpc/src/lib/upload.ts.
…lution, response formatting) - Import taskPriorityValues from @superset/db/enums instead of redefining - Extract resolveTaskId and taskIdCondition for UUID/slug resolution - Extract formatMcpResponse helper for consistent response formatting - Extract isPriority type guard to shared utils
Deduplicate the identical disconnect logic from linear and slack routers into a shared disconnectIntegration function parameterized by provider.
Move the device online threshold constant (60s) from packages/trpc and packages/mcp into @superset/shared/constants as the single source of truth.
📝 WalkthroughWalkthroughThis PR refactors membership lookups and utility functions across the codebase. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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. 🎉 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 |
![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 `@packages/trpc/src/lib/upload.ts`:
- Around line 25-27: The base64 extraction logic using
fileData.split("base64,")[1] falls back to the full data URI when the payload is
empty, causing Buffer.from(fullDataUri, "base64") to produce garbage; update the
code that computes base64Data (the base64Data variable derived from fileData) to
explicitly detect an empty payload after "base64," and treat it as an error
(e.g., throw or return a validation error) instead of returning fileData, so
downstream Buffer.from(base64Data, "base64") only receives a non-empty base64
payload; ensure the new guard is applied before any size checks or Buffer.from
calls.
- Around line 38-49: Reorder the blob operations so the new upload is performed
before deleting the old image: call put(pathname, buffer, { access: "public",
contentType: mimeType }) first and only if it succeeds attempt del(existingUrl).
Wrap the del(existingUrl) call in a try/catch and log any deletion error with
context (include existingUrl and the error) instead of silently swallowing it;
do not delete the old blob until put has completed successfully, and if del
fails after a successful put, log the failure but do not revert the successful
put.
🧹 Nitpick comments (9)
apps/marketing/src/lib/content-utils.ts (2)
1-2: Unused constant_MONTH_LONG.The underscore prefix signals intentional non-use, but if it's truly not needed, consider removing it to avoid dead code.
36-42: Redundant truthy-value branch and inconsistent numeric handling.Lines 40-42 catch any truthy non-string/non-number/non-Date value (e.g., objects, booleans), producing potentially meaningless strings like
"[object Object]"or"true". Also, numeric values (e.g., Unix timestamps) are returned as raw strings viaString(value)rather than going throughtoDateInput, which could produce unexpected date representations downstream.Consider either:
- Removing the catch-all truthy branch (lines 40-42) since it overlaps with the string/number check and could mask bad data.
- Routing numbers through
toDateInputfor consistency with theDatebranch.Suggested simplification
if (typeof value === "string" || typeof value === "number") { - return String(value); - } - - if (value) { - return String(value); + return typeof value === "number" ? toDateInput(value) : value; }packages/trpc/src/router/integration/utils.ts (2)
8-11: Consider using object parameters forverifyOrgMembershipandverifyOrgAdmin.These functions take 2 positional string arguments (
userId,organizationId) which are easy to swap accidentally. The newdisconnectIntegrationcorrectly uses object parameters — consider aligning these older helpers for consistency.As per coding guidelines: "Use object parameters for functions with 2 or more parameters instead of positional arguments."
37-59:disconnectIntegrationsilently returnssuccess: falsewhen no connection exists.The callers (Linear/Slack disconnect mutations) return this result directly to the client. A missing connection during disconnect could indicate a bug or race condition. Consider throwing a
TRPCErrorwithNOT_FOUNDinstead, which would be more consistent withverifyOrgMembership's throw-on-missing pattern and the coding guideline on TRPCError codes.That said, if the intent is idempotent disconnects (no-op when already disconnected), the current approach is fine — just worth confirming the design intent.
packages/trpc/src/lib/upload.ts (1)
61-63:Math.random()for pathname generation is fine for uniqueness but could collide under high concurrency.Not a blocking issue, but
crypto.randomUUID()would be more robust and already imported (node:cryptois used elsewhere in this PR). Worth considering for a shared utility that may see wider use.packages/mcp/src/tools/tasks/utils.ts (1)
14-41: Consider reusingtaskIdConditioninsideresolveTaskIdto DRY the UUID-detection logic.Both
resolveTaskIdandtaskIdConditionindependently instantiatez.string().uuid()and branch on UUID vs slug.resolveTaskIdcould delegate totaskIdConditionto avoid duplicating this pattern.♻️ Suggested refactor
export async function resolveTaskId({ taskId, organizationId, }: { taskId: string; organizationId: string; }) { - const isUuid = z.string().uuid().safeParse(taskId).success; - const [task] = await db .select({ id: tasks.id }) .from(tasks) .where( and( - isUuid ? eq(tasks.id, taskId) : eq(tasks.slug, taskId), + taskIdCondition(taskId), eq(tasks.organizationId, organizationId), isNull(tasks.deletedAt), ), ) .limit(1); return task ?? null; }packages/mcp/src/tools/tasks/create-task/create-task.ts (1)
150-152:isPrioritycheck is redundant here — Zod already validates the value.
input.priorityis validated byz.enum(taskPriorityValues).default("none")intaskInputSchema, so it is always a validTaskPriorityby the time you reach this line. The guard adds no safety and could be removed for clarity.♻️ Simplified assignment
- const priority: TaskPriority = isPriority(input.priority) - ? input.priority - : "none"; + const priority: TaskPriority = input.priority;packages/mcp/src/tools/tasks/delete-task/delete-task.ts (1)
31-46: SequentialresolveTaskIdcalls create N+1 queries — consider batching if this becomes a concern.Each task ID triggers a separate DB query (up to 25). Given the max-25 cap and MCP tool context, this is acceptable, but a batch resolver could be a future optimization.
packages/mcp/src/tools/tasks/update-task/update-task.ts (1)
117-131: Sequential per-task updates are not transaction-wrapped — partial updates possible on failure.Unlike
create-task(which usesdbWs.transaction), this handler issues individualUPDATEstatements in a loop. If a mid-batch update fails, earlier updates are already committed. Consider wrapping the loop in adbWs.transactionfor atomicity, consistent with the create-task pattern.♻️ Wrap updates in a transaction
- const updatedTasks: { id: string; slug: string; title: string }[] = []; - - for (const { taskId, updateData } of resolvedUpdates) { - const [task] = await dbWs - .update(tasks) - .set(updateData) - .where(eq(tasks.id, taskId)) - .returning({ - id: tasks.id, - slug: tasks.slug, - title: tasks.title, - }); - - if (task) { - updatedTasks.push(task); - } - } + const updatedTasks = await dbWs.transaction(async (tx) => { + const results: { id: string; slug: string; title: string }[] = []; + for (const { taskId, updateData } of resolvedUpdates) { + const [task] = await tx + .update(tasks) + .set(updateData) + .where(eq(tasks.id, taskId)) + .returning({ + id: tasks.id, + slug: tasks.slug, + title: tasks.title, + }); + if (task) { + results.push(task); + } + } + return results; + });
| const base64Data = fileData.includes("base64,") | ||
| ? fileData.split("base64,")[1] || fileData | ||
| : fileData; |
There was a problem hiding this comment.
Edge case in base64 extraction when data after "base64," is empty.
If fileData is "data:image/png;base64," (no payload after the comma), split("base64,")[1] yields "", which is falsy, so the fallback returns the entire data URI string. Buffer.from(fullDataUri, "base64") will silently produce garbage bytes rather than failing. This would result in a corrupted upload that passes the size check.
Consider a stricter guard:
Proposed fix
const base64Data = fileData.includes("base64,")
- ? fileData.split("base64,")[1] || fileData
+ ? fileData.split("base64,")[1] ?? fileData
: fileData;
+
+ if (!base64Data) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Invalid file data: empty base64 payload",
+ });
+ }📝 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 base64Data = fileData.includes("base64,") | |
| ? fileData.split("base64,")[1] || fileData | |
| : fileData; | |
| const base64Data = fileData.includes("base64,") | |
| ? fileData.split("base64,")[1] ?? fileData | |
| : fileData; | |
| if (!base64Data) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid file data: empty base64 payload", | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@packages/trpc/src/lib/upload.ts` around lines 25 - 27, The base64 extraction
logic using fileData.split("base64,")[1] falls back to the full data URI when
the payload is empty, causing Buffer.from(fullDataUri, "base64") to produce
garbage; update the code that computes base64Data (the base64Data variable
derived from fileData) to explicitly detect an empty payload after "base64," and
treat it as an error (e.g., throw or return a validation error) instead of
returning fileData, so downstream Buffer.from(base64Data, "base64") only
receives a non-empty base64 payload; ensure the new guard is applied before any
size checks or Buffer.from calls.
| if (existingUrl) { | ||
| try { | ||
| await del(existingUrl); | ||
| } catch { | ||
| // Old image doesn't exist or isn't in blob storage | ||
| } | ||
| } | ||
|
|
||
| const blob = await put(pathname, buffer, { | ||
| access: "public", | ||
| contentType: mimeType, | ||
| }); |
There was a problem hiding this comment.
Old image is deleted before the new upload succeeds — data loss on upload failure.
If del(existingUrl) succeeds but put(pathname, ...) subsequently throws, the old image is gone and no new image was stored. Consider reordering: upload first, then delete the old blob only after the new one is confirmed.
Also, the catch block on Line 41 silently swallows the deletion error. Per coding guidelines, errors should at minimum be logged with context.
Proposed fix: reorder operations and log deletion errors
- if (existingUrl) {
- try {
- await del(existingUrl);
- } catch {
- // Old image doesn't exist or isn't in blob storage
- }
- }
-
const blob = await put(pathname, buffer, {
access: "public",
contentType: mimeType,
});
+ if (existingUrl) {
+ try {
+ await del(existingUrl);
+ } catch (error) {
+ console.error("[upload/uploadImage] Failed to delete old image:", error);
+ }
+ }
+
return blob.url;As per coding guidelines, "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly".
📝 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.
| if (existingUrl) { | |
| try { | |
| await del(existingUrl); | |
| } catch { | |
| // Old image doesn't exist or isn't in blob storage | |
| } | |
| } | |
| const blob = await put(pathname, buffer, { | |
| access: "public", | |
| contentType: mimeType, | |
| }); | |
| const blob = await put(pathname, buffer, { | |
| access: "public", | |
| contentType: mimeType, | |
| }); | |
| if (existingUrl) { | |
| try { | |
| await del(existingUrl); | |
| } catch (error) { | |
| console.error("[upload/uploadImage] Failed to delete old image:", error); | |
| } | |
| } | |
| return blob.url; |
🤖 Prompt for AI Agents
In `@packages/trpc/src/lib/upload.ts` around lines 38 - 49, Reorder the blob
operations so the new upload is performed before deleting the old image: call
put(pathname, buffer, { access: "public", contentType: mimeType }) first and
only if it succeeds attempt del(existingUrl). Wrap the del(existingUrl) call in
a try/catch and log any deletion error with context (include existingUrl and the
error) instead of silently swallowing it; do not delete the old blob until put
has completed successfully, and if del fails after a successful put, log the
failure but do not revert the successful put.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/trpc/package.json`:
- Around line 39-47: Remove "react" and "@tanstack/react-query" from
dependencies in package.json and add them under peerDependencies with the same
semver ranges (e.g., "react": "19.2.0", "@tanstack/react-query": "^5.90.19"),
while keeping them in devDependencies for local typechecking; this package
exports hooks (useTRPC, TRPCProvider, createTRPCReactProvider) so moving these
two to peerDependencies prevents duplicate React/React-Query instances and
broken hook/cache behavior.
| "@tanstack/react-query": "^5.90.19", | ||
| "@trpc/client": "^11.7.1", | ||
| "@trpc/server": "^11.7.1", | ||
| "@trpc/tanstack-react-query": "^11.7.1", | ||
| "@upstash/qstash": "^2.8.4", | ||
| "@vercel/blob": "^2.0.0", | ||
| "@vercel/kv": "^3.0.0", | ||
| "drizzle-orm": "0.45.1", | ||
| "react": "19.2.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, confirm the current state of packages/trpc/package.json
echo "=== Current state of packages/trpc/package.json (lines 35-55) ==="
sed -n '35,55p' packages/trpc/package.json
echo ""
echo "=== Full dependencies section of packages/trpc/package.json ==="
jq '.dependencies, .peerDependencies, .devDependencies | keys' packages/trpc/package.jsonRepository: superset-sh/superset
Length of output: 1186
🏁 Script executed:
#!/bin/bash
# Check how packages/trpc is consumed - find imports across apps
echo "=== Imports of `@trpc` in web app ==="
fd -t f '\.ts$' '\.tsx$' apps/web --max-depth 3 -x rg '@trpc' {} + 2>/dev/null | head -20
echo ""
echo "=== Imports of `@trpc` in admin app ==="
fd -t f '\.ts$' '\.tsx$' apps/admin --max-depth 3 -x rg '@trpc' {} + 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Compare how other shared packages declare react dependencies
echo "=== Checking react declaration in other packages/ ==="
fd package.json 'packages' --max-depth 2 --exec sh -c '
path="$1"
echo "--- $path ---"
if jq -e ".dependencies.react" "$path" >/dev/null 2>&1; then
echo "dependencies.react: $(jq -r ".dependencies.react" "$path")"
fi
if jq -e ".peerDependencies.react" "$path" >/dev/null 2>&1; then
echo "peerDependencies.react: $(jq -r ".peerDependencies.react" "$path")"
fi
if ! jq -e ".dependencies.react, .peerDependencies.react" "$path" >/dev/null 2>&1; then
echo "react: not declared"
fi
' _ {}Repository: superset-sh/superset
Length of output: 827
🏁 Script executed:
#!/bin/bash
# Check if web and admin apps have react in their dependencies
echo "=== apps/web/package.json react declaration ==="
jq '.dependencies.react, .peerDependencies.react' apps/web/package.json 2>/dev/null
echo ""
echo "=== apps/admin/package.json react declaration ==="
jq '.dependencies.react, .peerDependencies.react' apps/admin/package.json 2>/dev/nullRepository: superset-sh/superset
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Double-check if admin app might use `@trpc` indirectly or through a different import pattern
echo "=== Checking admin app directory structure ==="
ls -la apps/admin/src/ 2>/dev/null | head -20
echo ""
echo "=== Searching for 'trpc' (case-insensitive) in admin ==="
rg -i 'trpc' apps/admin/ --type ts --type tsx 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 704
🏁 Script executed:
#!/bin/bash
# Check what packages/trpc actually exports - is it frontend-only or mixed?
echo "=== packages/trpc exports and main entry ==="
head -30 packages/trpc/package.json | grep -A 2 '"exports"\|"main"'
echo ""
echo "=== List of files in packages/trpc/src ==="
find packages/trpc/src -type f -name '*.ts' -o -name '*.tsx' 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 994
🏁 Script executed:
#!/bin/bash
# Check what's in apps/admin/src/trpc directory
echo "=== Contents of apps/admin/src/trpc ==="
find apps/admin/src/trpc -type f 2>/dev/null | head -20
echo ""
echo "=== Sample from apps/admin/src/trpc/client.ts ==="
head -20 apps/admin/src/trpc/client.ts 2>/dev/null
echo ""
echo "=== How does admin app import from packages/trpc ==="
rg 'from.*trpc|import.*trpc' apps/admin/src/ --type ts --type tsx 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 269
🏁 Script executed:
#!/bin/bash
# Check the actual purpose - look at react.tsx exports
echo "=== packages/trpc/src/client/react.tsx first 50 lines ==="
head -50 packages/trpc/src/client/react.tsx
echo ""
echo "=== Check if react.tsx exports React hooks ==="
rg 'export.*useQuery|export.*useMutation|export.*Provider|export function' packages/trpc/src/client/react.tsx | head -15Repository: superset-sh/superset
Length of output: 1686
Move react and @tanstack/react-query to peerDependencies.
This package exports React hooks (useTRPC, TRPCProvider, createTRPCReactProvider) and is consumed by multiple apps (web, admin). Declaring react and @tanstack/react-query as direct dependencies risks bundling separate instances, causing hook errors and broken cache sharing. The monorepo already follows this pattern in packages/ui and packages/durable-session.
Move both to peerDependencies and keep in devDependencies for local typechecking:
Suggested diff
"dependencies": {
...
- "@tanstack/react-query": "^5.90.19",
"@trpc/client": "^11.7.1",
"@trpc/server": "^11.7.1",
"@trpc/tanstack-react-query": "^11.7.1",
...
- "react": "19.2.0",
...
},
+ "peerDependencies": {
+ "react": "^19.0.0",
+ "@tanstack/react-query": "^5.0.0"
+ },📝 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.
| "@tanstack/react-query": "^5.90.19", | |
| "@trpc/client": "^11.7.1", | |
| "@trpc/server": "^11.7.1", | |
| "@trpc/tanstack-react-query": "^11.7.1", | |
| "@upstash/qstash": "^2.8.4", | |
| "@vercel/blob": "^2.0.0", | |
| "@vercel/kv": "^3.0.0", | |
| "drizzle-orm": "0.45.1", | |
| "react": "19.2.0", | |
| "dependencies": { | |
| "@trpc/client": "^11.7.1", | |
| "@trpc/server": "^11.7.1", | |
| "@trpc/tanstack-react-query": "^11.7.1", | |
| "@upstash/qstash": "^2.8.4", | |
| "@vercel/blob": "^2.0.0", | |
| "@vercel/kv": "^3.0.0", | |
| "drizzle-orm": "0.45.1" | |
| }, | |
| "peerDependencies": { | |
| "react": "^19.0.0", | |
| "@tanstack/react-query": "^5.0.0" | |
| }, | |
| "devDependencies": { | |
| "react": "^19.0.0", | |
| "@tanstack/react-query": "^5.0.0" | |
| } |
🤖 Prompt for AI Agents
In `@packages/trpc/package.json` around lines 39 - 47, Remove "react" and
"@tanstack/react-query" from dependencies in package.json and add them under
peerDependencies with the same semver ranges (e.g., "react": "19.2.0",
"@tanstack/react-query": "^5.90.19"), while keeping them in devDependencies for
local typechecking; this package exports hooks (useTRPC, TRPCProvider,
createTRPCReactProvider) so moving these two to peerDependencies prevents
duplicate React/React-Query instances and broken hook/cache behavior.
This reverts commit 515abc9.
1 participant
Summary
Deduplicates shared logic across the monorepo:
findOrgMembershipinto@superset/db/utils, replacing 6 inline queries across tRPC routers and API routes.packages/trpc/src/lib/upload.ts.Test plan
bun run lint:fixpassesbunx sherifpassesSummary by CodeRabbit
New Features
Refactor