refactor(api): new endpoints for slackbot#293
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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. 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: 6
🤖 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 `@packages/api/src/router/slack.test.ts`:
- Around line 177-205: Add a regression test that seeds two slackUsers rows with
the same slackId but different slackTeamId values and assert lookups are scoped
by both slackId and teamId: create fixtures inserting a user with slackId into
teamA and another with the same slackId into teamB, then call
client.slack.getUserBySlackId({ slackId, teamId: teamA }) and
client.slack.getUserBySlackId({ slackId, teamId: teamB }) and assert each
returns the correct userName/email for that team; also duplicate analogous
assertions for upsertUser, getOrCreateUser, and the role-related endpoints to
ensure those functions respect the composite (slackId, teamId) key rather than
slackId alone.
In `@packages/api/src/router/slack.ts`:
- Around line 44-61: The current read-modify-write on schema.slackSpaces using
space.settings then writing updatedSettings can race and lose concurrent
updates; instead perform the merge atomically in the database (or inside a DB
transaction with row lock) so updates to different keys don't clobber each
other: replace the in-memory merge of (space.settings as Record<string,
unknown>) + input.settings and the separate ctx.db.update call with a single
atomic DB update that merges JSON (e.g., settings = settings || :input.settings
using your DB/jsonb merge operator) or run the update inside a transaction
selecting the row FOR UPDATE before writing; operate on the same identifier used
now (eq(schema.slackSpaces.teamId, input.teamId)) so the change is applied
safely.
- Around line 167-184: getOrCreateSpace does a select-then-insert and races the
unique teamId constraint (schema.slackSpaces.teamId), so change the flow to
perform an upsert/conflict-handling insert via
ctx.db.insert(...).onConflict(...) (or the Drizzle equivalent) that either
returns the inserted row or does nothing on conflict, then re-select the
existing row; alternatively, wrap the insert in a try/catch that catches the
unique-constraint error and re-query using
ctx.db.select().from(schema.slackSpaces).where(eq(schema.slackSpaces.teamId,
input.teamId)) to return the existing space. Ensure you update the code paths
around getOrCreateSpace, ctx.db, and schema.slackSpaces to use the
conflict-handling approach and return a single canonical row.
- Around line 116-132: The Slack user lookups/updates currently query by slackId
alone and must be scoped by team; modify the select and update predicates that
use eq(schema.slackUsers.slackId, input.slackId) so they include the team
equality (e.g., AND eq(schema.slackUsers.slackTeamId, input.slackTeamId) or an
equivalent composite where) so both the initial select and the subsequent
ctx.db.update(...).where(...) use (slackId, slackTeamId) together; apply the
same change to the other occurrences that use schema.slackUsers.slackId alone
(the other select/update sites noted in the review).
- Around line 11-26: getSpace currently uses publicProcedure and returns the
full slack_spaces row (via ctx.db.select().from(schema.slackSpaces).where(...)),
which exposes secrets like botToken for a given teamId; either change the
procedure to apiKeyProcedure (replace publicProcedure with apiKeyProcedure on
getSpace) or modify the DB query to project only safe columns (select explicit
non-sensitive fields instead of the full row) before returning; ensure you
remove botToken, signingSecret, or any persisted secret fields from the returned
object and keep references to getSpace, publicProcedure, apiKeyProcedure,
schema.slackSpaces, botToken, and teamId so reviewers can find the change.
- Around line 1194-1215: The current read/merge/write for meta (using
ctx.db.select on schema.users.meta, merging into updateData.meta, then
ctx.db.update) is non-atomic and can clobber concurrent updates; change this to
perform the merge inside a transaction with row-level locking or a single
DB-side update. Specifically, wrap the operation that reads and writes
schema.users.meta in ctx.db.transaction and use a SELECT ... FOR UPDATE on
schema.users (where(eq(schema.users.id, slackUser.userId))) before merging, or
replace the in-memory merge with a DB JSON/JSONB merge expression in the
.update(...) so updateData.meta is computed by the DB (referencing
schema.users.meta) rather than reading it into the app; ensure you still apply
other updateData fields in the same transaction/update so the change to meta is
atomic with the rest of the update.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7cf407f6-5f0b-4835-b59f-c86738e76b9a
📒 Files selected for processing (4)
apps/api/src/app/docs/openapi.json/route.tspackages/api/src/index.tspackages/api/src/router/slack.test.tspackages/api/src/router/slack.ts
|
@taterhead247 curious what you think about the latest commit I made here. I got to thinking, I really don't see a need to have these routes generally available. So I've hidden them from the API docs, and put them under their own authorization which ONLY accepts the SLACKBOT_API_KEY (another env variable, I know you're not a fan of adding more of these). Open to suggestions though, I can easily roll this back. I do intend on having more public facing Slack routes, like a /slack/post-message where you can trigger Slack to post a message for regions on the Slackbot. That would use the normal |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/api/src/router/slack.test.ts (1)
84-99: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit coverage for
SLACKBOT_API_KEYauth path (and wrong-key rejection).The suite currently proves
SUPER_ADMIN_API_KEYworks and missing key fails, but doesn’t lock in the new Slackbot-key path. Add one positive case using"test-slackbot-api-key"and one negative case with an invalid key soslackBotProcedure’s dual-key contract is regression-protected.Also applies to: 176-205
🤖 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/api/src/router/slack.test.ts` around lines 84 - 99, Add two tests covering the SLACKBOT_API_KEY path for slack.getSpace: one positive using createTestClient("test-slackbot-api-key") that calls client.slack.getSpace({ teamId }) and asserts the response is not null, has teamId === teamId and settings.welcome_dm_template === "Welcome!"; and one negative using createTestClient("invalid-key") that calls client.slack.getSpace({ teamId }) and asserts the call is rejected/returns null (matching the existing failure behavior). Place these alongside the existing admin/missing-key tests (also mirror at the other block flagged around lines 176-205) so slackBotProcedure’s dual-key acceptance and wrong-key rejection are covered.packages/api/src/router/slack.ts (1)
97-149:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce
(slackId, slackTeamId)uniqueness to prevent duplicate Slack-user rows.
upsertUserandgetOrCreateUserstill do read-then-write without a DB uniqueness guarantee. With concurrent requests for the same Slack identity/team, both can insert, and subsequent.select()[0]paths become nondeterministic.Suggested direction
# 1) Add a DB unique constraint/index on (slack_id, slack_team_id) # (migration in packages/db) # 2) Then use conflict-aware writes in router handlers: - const [existing] = await ctx.db.select().from(schema.slackUsers).where(...) - if (existing) { await ctx.db.update(...).where(...); ... } else { await ctx.db.insert(...); ... } + const [row] = await ctx.db + .insert(schema.slackUsers) + .values({ + slackId: input.slackId, + slackTeamId: input.teamId, + userName: input.userName, + email: input.email ?? "", + userId: input.userId, + isAdmin: input.isAdmin, + isOwner: input.isOwner, + isBot: input.isBot, + }) + .onConflictDoUpdate({ + target: [schema.slackUsers.slackId, schema.slackUsers.slackTeamId], + set: { + userName: input.userName, + email: input.email ?? "", + userId: input.userId, + isAdmin: input.isAdmin, + isOwner: input.isOwner, + isBot: input.isBot, + }, + }) + .returning();Also applies to: 191-242
🤖 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/api/src/router/slack.ts` around lines 97 - 149, Add a unique constraint on the (slackId, slackTeamId) columns in the slackUsers table and change the read-then-write logic in upsertUser and getOrCreateUser to perform a safe upsert using that constraint: rely on the DB-native upsert/ON CONFLICT or a single INSERT ... RETURNING with ON CONFLICT DO UPDATE (targeting schema.slackUsers and the slackId + slackTeamId composite key) or run the select/insert inside a serializable transaction/for-update lock so concurrent requests cannot create duplicate rows; update code paths that currently use ctx.db.select().from(schema.slackUsers)... and ctx.db.insert(schema.slackUsers)... to the DB upsert API so outcomes are deterministic and duplicates cannot be created.
🤖 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/docs/openapi.json/route.ts`:
- Line 111: Typo in the public API policy text: update the policy string in
route.ts (the OpenAPI/public API policy description) by replacing the misspelled
word "restrcited" with the correct "restricted" so the sentence reads
"...regional edit access is highly restricted."; locate the policy text used to
build the OpenAPI doc (the public API policy description) and fix that string
literal.
- Line 57: The OpenAPI generator's path filter in route.ts currently checks
path[0] !== "slack" but routes are prefixed with API_PREFIX_V1 ("/v1"), so
update the filter used when building the spec (the filter callback referenced in
route.ts that receives { path }) to account for the "/v1" prefix (e.g., check
path[0] !== "slack" && path[1] !== "slack" or normalize/remove API_PREFIX_V1
before checking) so Slack endpoints are excluded; also correct the typo in the
OpenAPI info.description from "restrcited" to "restricted" so the user-facing
docs string is accurate.
---
Outside diff comments:
In `@packages/api/src/router/slack.test.ts`:
- Around line 84-99: Add two tests covering the SLACKBOT_API_KEY path for
slack.getSpace: one positive using createTestClient("test-slackbot-api-key")
that calls client.slack.getSpace({ teamId }) and asserts the response is not
null, has teamId === teamId and settings.welcome_dm_template === "Welcome!"; and
one negative using createTestClient("invalid-key") that calls
client.slack.getSpace({ teamId }) and asserts the call is rejected/returns null
(matching the existing failure behavior). Place these alongside the existing
admin/missing-key tests (also mirror at the other block flagged around lines
176-205) so slackBotProcedure’s dual-key acceptance and wrong-key rejection are
covered.
In `@packages/api/src/router/slack.ts`:
- Around line 97-149: Add a unique constraint on the (slackId, slackTeamId)
columns in the slackUsers table and change the read-then-write logic in
upsertUser and getOrCreateUser to perform a safe upsert using that constraint:
rely on the DB-native upsert/ON CONFLICT or a single INSERT ... RETURNING with
ON CONFLICT DO UPDATE (targeting schema.slackUsers and the slackId + slackTeamId
composite key) or run the select/insert inside a serializable
transaction/for-update lock so concurrent requests cannot create duplicate rows;
update code paths that currently use ctx.db.select().from(schema.slackUsers)...
and ctx.db.insert(schema.slackUsers)... to the DB upsert API so outcomes are
deterministic and duplicates cannot be created.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f569cdd-d024-492d-a414-6487fab97314
📒 Files selected for processing (8)
apps/api/__tests__/docs-routes.test.tsapps/api/src/app/docs/openapi.json/route.tspackages/api/src/router/slack.test.tspackages/api/src/router/slack.tspackages/api/src/shared.tspackages/api/vitest.config.tspackages/env/.env.local.examplepackages/env/src/index.ts
| }); | ||
|
|
||
| const spec = (await generator.generate(router, { | ||
| filter: ({ path }) => path[0] !== "slack", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section of the OpenAPI route file (around line 57 and 110-124)
FILE="apps/api/src/app/docs/openapi.json/route.ts"
if [ -f "$FILE" ]; then
echo "=== $FILE (lines 40-90) ==="
nl -ba "$FILE" | sed -n '40,90p'
echo
echo "=== $FILE (lines 95-150) ==="
nl -ba "$FILE" | sed -n '95,150p'
else
echo "File not found: $FILE"
fi
# Find where "openapi.json" is built/filters are applied (if any other occurrences exist)
rg -n "openapi\.json|filter:\s*\(\{\s*path\s*\}|\bslack\b" apps/api/src/app/docs/openapi.json/route.ts apps/api/src -S || true
# Look for mounting/version prefix logic (verify whether it's apps/api/src/index.ts or packages/api/src/index.ts)
# Search for "v1" and route mounting and also for "apps/api/src/index.ts" / "packages/api/src/index.ts".
rg -n "mount|app\.use|/v1|\\bv1\\b|createServer|listen|next|app\\.route|OpenAPI|openapi" -S apps/api/src packages/api/src 2>/dev/null || true
ls -la apps/api/src || true
ls -la packages/api/src 2>/dev/null || true
# If index.ts exists in either location, print key sections
for IDX in apps/api/src/index.ts packages/api/src/index.ts; do
if [ -f "$IDX" ]; then
echo
echo "=== $IDX (first 200 lines) ==="
nl -ba "$IDX" | sed -n '1,200p'
fi
doneRepository: F3-Nation/f3-nation
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/api/src/app/docs/openapi.json/route.ts"
echo "== exists? $FILE =="
ls -la "$FILE" 2>/dev/null || true
echo
if [ -f "$FILE" ]; then
echo "=== $FILE (lines 40-90) ==="
sed -n '40,90p' "$FILE" | cat -n
echo
echo "=== $FILE (lines 95-150) ==="
sed -n '95,150p' "$FILE" | cat -n
fi
echo
echo "== grep for slack + filter/path in that file =="
rg -n "filter:\s*\(\{\s*path\s*\}|slack|path\[0\]|path\[1\]" "$FILE" -S || true
echo
echo "== find other OpenAPI generation/filtering =="
rg -n "openapi\.json|OpenAPI|swagger|filter:\s*\(\{\s*path\s*\}" apps/api/src packages/api/src -S || true
echo
echo "== check for /v1 mounting (index/route mounting) =="
rg -n "(/v1\b|\\bv1\\b|app\.use|router\.use|next|express|createServer|mount)" apps/api/src packages/api/src -S || true
echo
echo "== show likely index.ts files (first 200 lines) =="
for IDX in apps/api/src/index.ts packages/api/src/index.ts; do
if [ -f "$IDX" ]; then
echo "---- $IDX ----"
sed -n '1,200p' "$IDX" | cat -n
fi
doneRepository: F3-Nation/f3-nation
Length of output: 15106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/api/src/app/docs/openapi.json/route.ts"
echo "wc -l: $(wc -l < "$FILE")"
echo
echo "=== filter line (around 45-70) with file line numbers ==="
cat -n "$FILE" | sed -n '45,70p'
echo
echo "=== search for typo word 'restr' in the file (with file line numbers) ==="
rg -n "restr" "$FILE" -S || true
# Print a bit around the match area
MATCH_LINE="$(rg -n "restr" "$FILE" -S | head -n1 | cut -d: -f1 || true)"
if [ -n "${MATCH_LINE:-}" ]; then
START=$((MATCH_LINE-10))
END=$((MATCH_LINE+10))
echo
echo "=== lines ${START}-${END} around first match ==="
cat -n "$FILE" | sed -n "${START},${END}p"
fiRepository: F3-Nation/f3-nation
Length of output: 3153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show imports + router variable usage in openapi route
FILE="apps/api/src/app/docs/openapi.json/route.ts"
echo "=== top of $FILE (1-80) ==="
sed -n '1,80p' "$FILE" | cat -n
echo
echo "=== find where router is imported/declared in $FILE ==="
rg -n "from .*router|import .*router|const router|generator\.generate\(" "$FILE" -S || true
# 2) Locate API_PREFIX_V1 definition
echo
echo "=== find API_PREFIX_V1 definition ==="
rg -n "API_PREFIX_V1" -S . || trueRepository: F3-Nation/f3-nation
Length of output: 4726
🌐 Web query:
@orpc/openapi OpenAPIGenerator filter ({ path }) signature path array includes prefix
💡 Result:
In the @orpc/openapi package, the OpenAPIGenerator filter option allows you to include or exclude procedures from the generated OpenAPI specification [1][2]. The filter function receives an object containing path as one of its arguments, which is a readonly string[] array representing the path to the procedure within your router [3][4]. You can check if the path includes a specific prefix by inspecting the elements of this array [1][2]. For example, if you want to exclude any procedures under an "internal" path, you can use the following approach [1][2]: const spec = await generator.generate(router, { filter: ({ path }) => path!== 'internal', }) In this example, path[0] === 'internal' checks if the first segment of the path is 'internal' [1][2]. Because path is an array of strings, you can use standard JavaScript array methods like includes() if you need to check for a prefix that might exist at any level of the path hierarchy, or simply index into the array if the prefix is always at a specific depth [1][2]. The filter function should return true to include the procedure or false to exclude it [1][3][2].
Citations:
- 1: https://middleapi-orpc.mintlify.app/api-reference/openapi
- 2: https://mintlify.wiki/middleapi/orpc/api-reference/openapi
- 3: https://github.com/middleapi/orpc/blob/main/packages/openapi/src/openapi-generator.ts
- 4: https://github.com/middleapi/orpc/blob/f4868a14/packages/openapi/src/openapi-generator.ts
Fix OpenAPI Slack filtering to handle the /v1 prefix (and correct docs typo).
apps/api/src/app/docs/openapi.json/route.tsbuilds the spec fromrouterimported from@acme/api, whosepackages/api/src/index.tsprefixes all routes withAPI_PREFIX_V1(/v1). That means Slack endpoints are typically underpath[1], sopath[0] !== "slack"won’t filter them out.- Fix the user-facing typo
restrcited→restrictedin the OpenAPIinfo.description.
Suggested fix
- filter: ({ path }) => path[0] !== "slack",
+ filter: ({ path }) => {
+ const first = path[0];
+ const second = path[1];
+ return !(first === "slack" || second === "slack");
+ },📝 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.
| filter: ({ path }) => path[0] !== "slack", | |
| filter: ({ path }) => { | |
| const first = path[0]; | |
| const second = path[1]; | |
| return !(first === "slack" || second === "slack"); | |
| }, |
🤖 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/docs/openapi.json/route.ts` at line 57, The OpenAPI
generator's path filter in route.ts currently checks path[0] !== "slack" but
routes are prefixed with API_PREFIX_V1 ("/v1"), so update the filter used when
building the spec (the filter callback referenced in route.ts that receives {
path }) to account for the "/v1" prefix (e.g., check path[0] !== "slack" &&
path[1] !== "slack" or normalize/remove API_PREFIX_V1 before checking) so Slack
endpoints are excluded; also correct the typo in the OpenAPI info.description
from "restrcited" to "restricted" so the user-facing docs string is accurate.
| - **Admin**: Same permissions as Editor, plus the ability to add/remove other Admins and Editors | ||
|
|
||
|
|
||
| As of February 1, 2026, regional admins can only create read-only API keys. If you want an API Key edit access to your region, you will need to contact an F3 Nation admin. Right now, regional edit access is highly restrcited. This is because the API is still new and there are almost certainly gaps in security - meaning that a region has the potential to mess up data for other regions. |
There was a problem hiding this comment.
Fix typo in public API policy text.
“restrcited” should be “restricted”.
Suggested fix
-Right now, regional edit access is highly restrcited.
+Right now, regional edit access is highly restricted.📝 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.
| As of February 1, 2026, regional admins can only create read-only API keys. If you want an API Key edit access to your region, you will need to contact an F3 Nation admin. Right now, regional edit access is highly restrcited. This is because the API is still new and there are almost certainly gaps in security - meaning that a region has the potential to mess up data for other regions. | |
| As of February 1, 2026, regional admins can only create read-only API keys. If you want an API Key edit access to your region, you will need to contact an F3 Nation admin. Right now, regional edit access is highly restricted. This is because the API is still new and there are almost certainly gaps in security - meaning that a region has the potential to mess up data for other regions. |
🤖 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/docs/openapi.json/route.ts` at line 111, Typo in the public
API policy text: update the policy string in route.ts (the OpenAPI/public API
policy description) by replacing the misspelled word "restrcited" with the
correct "restricted" so the sentence reads "...regional edit access is highly
restricted."; locate the policy text used to build the OpenAPI doc (the public
API policy description) and fix that string literal.
|
@evanpetzoldt a few standups ago we talked about you using the api for general stuff but still hitting the db directly for stuff that doesn't make sense to share. Is that an option? |
|
@taterhead247 yeah, that's still an option... there's just a bit of a blurry line on what general stuff is vs something very slackbot specific. For example, some of these are setting admins (using slack ids so the lookups are in the route), marking attendance, etc. We also won't be able to get away from the double data model headache as long as I'm hitting the db directly. |
|
There's no way for you to get in step with our data model while still hitting the db directly? I guess if you were in the mono repo you could? I get the fuzzy line. The env thing bothers me. Not just because it's an env. But because yours won't be the only use case and I don't live the idea of a dozen envs just to whitelist an api key. We really need to add scopes like is described in action #2 of #378. But I imagine that would take a while to implement. |
|
When I move Slackbot to the monorepo, I could move the data models in as well. Then at least when db updates are made to drizzle, we could make them simultaneously to the sqlalchemy models. Or I could just switch to good old fashioned SQL Well ok, maybe I'll leave this as a draft for now, there's no rush on getting it in. May focus on getting slackbot in next |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores