fix(slack): preserve query params for POST requests - #591
Conversation
|
@lyydsheep is attempting to deploy a commit to the corsair Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryFixes Slack pagination and filtering on
Confidence Score: 5/5The PR appears safe to merge, with the intended POST query forwarding covered by a focused regression test. The changed client behavior fixes the existing Important Files Changed
Reviews (1): Last reviewed commit: "fix(slack): preserve query params for PO..." | Re-trigger Greptile |
📝 WalkthroughWalkthroughThe Slack client now forwards query parameters for all HTTP methods. Tests verify that POST requests preserve ChangesSlack query forwarding
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/slack/client.test.ts (1)
27-32: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the POST body in this regression test.
The fixture passes
typesinbody, but the assertion checks onlymethodandquery. A later change that drops or rewrites the POST body would still pass this test.Add the body assertion so the test verifies that
typesremains separate from the query parameters. This follows the PR objective to preservetypesand query parameters in the outbound request.Suggested assertion
expect.objectContaining({ method: 'POST', + body: { types: 'public_channel' }, query: { cursor: 'next-page', limit: 100 }, })🤖 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/slack/client.test.ts` around lines 27 - 32, Update the request assertion in the regression test around mockRequest to also verify the POST body contains the fixture’s types value, while keeping the existing method and query checks. Ensure the assertion confirms types remains in body and separate from query parameters.
🤖 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/slack/client.test.ts`:
- Around line 27-32: Update the request assertion in the regression test around
mockRequest to also verify the POST body contains the fixture’s types value,
while keeping the existing method and query checks. Ensure the assertion
confirms types remains in body and separate from query parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f02b2499-068d-4d1e-b15a-712ff4748080
📒 Files selected for processing (2)
packages/slack/client.test.tspackages/slack/client.ts
|
LGTM |
Fixes #590
Description
Forward query parameters for non-GET Slack requests so pagination fields such as
cursorandlimitreachconversations.list. The change keeps the shared request client behavior consistent and adds a mocked regression test.Screenshots / Demos
This is a headless API client; the regression is exercised by the mocked Jest test and the repository CI run: https://github.com/corsairdev/corsair/actions/runs/30911390778