Close the twelve mutations that still survived the suite - #118
Merged
Conversation
…aring resolveSlackTargets and resolveSlackContext each filter on organizationId, and resolveSlackContext also filters on provider. The suite seeded one workspace with one slack integration, so all three clauses could be deleted with 436 tests still green: a workspace could have been offered another workspace channel, and dispatched with another workspace bot token. Seed a second workspace with its own slack team and its own token, plus a workspace whose only integration is github, and record the authorization header the dispatch actually sends.
resolveCycle was the one resolver with no test, so it could return cycles[0] regardless of the reference and drop the "active" shortcut entirely with the suite still green. An agent asked to move an issue into Sprint 3 would have moved it into Sprint 1. Give engineering three sprints, design and operations one each, and close the operations sprint so the empty active case has something to land on.
The sweep is what closes a socket whose session went away without a control message ever announcing it, and it had no test. sweepSessions could return before doing anything, and liveSessionIds could stop filtering on the expiry, with the whole suite green either way: a signed out tab would have kept streaming. Run a hub with a short sweep interval and let it find an expired session and a deleted one, while a live session stays connected.
…leting getLabel, updateLabel and deleteLabel each scope their where clause to the caller workspace, and every label test ran inside a single workspace, so all three clauses could be deleted with 553 tests green. Any admin could have renamed or deleted a label belonging to a workspace they have never been a member of. Put a same named label in each of two workspaces and let each one reach for the other.
useToggleReaction was the one exported hook in use-comments with no test, so it could have posted to any url at all and 1045 tests stayed green. Assert the endpoint it asks, the emoji it sends, the optimistic add and remove, and the rollback behind the error toast. The slack webhook picks the bot token by slack team id, and every fixture workspace had exactly one integration, so the argument could be dropped without a failure. Give one workspace a second slack team and record the token the client was constructed with.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
imshashank has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughThe PR expands test coverage for Slack workspace scoping, comment reactions, label isolation, cycle resolution, and realtime session cleanup. ChangesSlack workspace routing
Comment reactions
Label workspace isolation
Cycle resolution
Realtime session sweeps
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/services/tests/slack/dispatch.test.ts (2)
32-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn
integrationIdonly when the fixture inserts a Slack integration.If
options.botTokenis undefined,seedWorkspaceinserts no Slack integration but still returnsintegrationId. The returned ID then points to no row. A later test that passes that ID toconnectSlackChannelfails on the foreign key, or worse, asserts against a phantom integration. Make the field optional so the type reflects reality.♻️ Proposed fix
interface Fixture { readonly organizationId: string; - readonly integrationId: string; + readonly integrationId: string | null; readonly userId: string; readonly teamA: string; readonly teamB: string; } @@ - const integrationId = `int_${suffix}`; + let integrationId: string | null = null; if (options.botToken !== undefined) { + integrationId = `int_${suffix}`; await tx.insert(integration).values({ id: integrationId,Note that this change requires the call sites that pass
fixture.integrationIdto narrow the value first.🤖 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/services/tests/slack/dispatch.test.ts` around lines 32 - 85, Update the Fixture type and seedWorkspace so integrationId is optional and is returned only when the Slack integration is inserted for a defined botToken. Narrow fixture.integrationId at every call site that passes it to connectSlackChannel or otherwise requires an existing integration, while preserving unconditional fixture fields.
480-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the two-workspace setup used by both dispatch tests.
Both tests seed
AcmeandGlobexwith the same options and connect one channel per workspace. A shared helper removes the repetition and keeps the assertions in focus.♻️ Proposed fix
+async function seedTwoWorkspaces( + tx: TestTransaction, +): Promise<{ readonly acme: Fixture; readonly globex: Fixture }> { + const acme = await seedWorkspace(tx, { + name: 'Acme', + slackTeamId: 'T-acme', + botToken: 'xoxb-acme', + }); + const globex = await seedWorkspace(tx, { + name: 'Globex', + slackTeamId: 'T-globex', + botToken: 'xoxb-globex', + }); + return { acme, globex }; +}🤖 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/services/tests/slack/dispatch.test.ts` around lines 480 - 562, Extract the repeated Acme/Globex workspace seeding and channel connections from both tests into a shared setup helper near the dispatch tests. Have the helper return the seeded workspace data needed by each test, while preserving the existing workspace-specific channel configuration and keeping each test’s dispatch inputs and assertions unchanged.
🤖 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/services/tests/slack/dispatch.test.ts`:
- Around line 32-85: Update the Fixture type and seedWorkspace so integrationId
is optional and is returned only when the Slack integration is inserted for a
defined botToken. Narrow fixture.integrationId at every call site that passes it
to connectSlackChannel or otherwise requires an existing integration, while
preserving unconditional fixture fields.
- Around line 480-562: Extract the repeated Acme/Globex workspace seeding and
channel connections from both tests into a shared setup helper near the dispatch
tests. Have the helper return the seeded workspace data needed by each test,
while preserving the existing workspace-specific channel configuration and
keeping each test’s dispatch inputs and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff21f87e-be1d-4659-8954-d70220937039
📒 Files selected for processing (6)
apps/web/tests/app/api/webhooks/slack/route.test.tsapps/web/tests/lib/query/use-comments.test.tsxpackages/core/tests/work/label-service.test.tspackages/mcp-server/tests/resolve.test.tspackages/realtime-server/tests/hub.test.tspackages/services/tests/slack/dispatch.test.ts
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.
Twelve guards that already exist in production code had nothing holding
them up. Each one could be deleted and the whole suite stayed green, so
the regression would have shipped without a red build anywhere.
Every test here was watched failing first: the guard it covers was
deliberately broken, the named tests went red, and the break was
reverted before the next one started. No production code changes.
Slack, in
packages/servicesThe suite seeded one workspace with one Slack integration, which can
prove team scoping and can never prove workspace scoping.
resolveSlackTargetsdropseq(slackChannelSync.organizationId, organizationId)resolveSlackContextdropseq(integration.organizationId, organizationId)resolveSlackContextdropseq(integration.provider, 'slack')A second workspace with its own Slack team and its own token now exists
in the fixture, so an issue on workspace A's Engineering team can be
shown never to resolve into workspace B's channel, and the dispatch
records the
authorizationheader it actually sent. A workspace whoseonly integration is GitHub resolves to no Slack context at all.
resolveCycle, inpackages/mcp-serverThe one resolver
tests/resolve.test.tsskipped. Replacing the wholematch with
cycles[0]passed the suite, and so did deleting theactiveshortcut. An agent told to file into Sprint 3 would have filedinto Sprint 1.
Engineering now has three sprints and only the first is running, design
and operations have one each, and operations has its sprint closed so
the empty active case lands somewhere.
cycles[0]kills 5 tests,dropping the
activebranch kills 1.The session sweep, in
packages/realtime-serversweepSessionsis the fail-closed sweep that closes a socket whosesession went away with no control message to announce it, and it had no
test at all. Returning early from it passed 68 tests, and so did
dropping
gt(session.expiresAt, now)fromliveSessionIds. A signedout tab would have kept streaming until the process restarted.
A hub with a short sweep interval now finds an expired session and a
deleted one, closes each with
session_revoked, and leaves a livesession subscribed across a dozen sweeps. The early return kills 2
tests, the expiry filter kills 1.
Labels, in
packages/coregetLabel,updateLabelanddeleteLabeleach scope their whereclause to the caller's workspace, and every label test ran inside one
workspace, so all three clauses could be deleted with 553 tests green.
Any admin could have renamed or deleted a label in a workspace they
have never belonged to.
A same named label now sits in each of two workspaces and each reaches
for the other. The three clauses kill 1, 2 and 1 test respectively.
useToggleReactionand the Slack webhook, inapps/webuseToggleReactionwas the only exported hook inuse-commentswithno test, so it could have posted anywhere and 1045 tests stayed green.
It now proves the endpoint it asks, the emoji it sends, the optimistic
add and the optimistic remove, and the rollback behind the error toast.
The webhook picks the bot token by Slack team id, and every fixture
workspace had exactly one integration, so the argument could be dropped
with nothing failing. One workspace now has two Slack teams, and the
recording client keeps the token it was constructed with.
Verification
bun run verifyexits 0 againstmainmerged in: lint, commentpolicy, byte policy, typecheck, and 2291 tests.