feat: add Adrapid plugin - #663
Conversation
📝 WalkthroughWalkthroughThe PR registers the You.com provider, maps its Jest imports to TypeScript sources, defers PostgreSQL availability checks until test execution, and makes the probe timeout test control delayed-read resolution explicitly. ChangesProvider and test updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment Warning |
Greptile SummaryThis PR registers the existing You.com plugin in the core provider vocabulary and adds Jest source-module mappings for its tests. It also includes unrelated core test changes that alter unavailable-Postgres behavior.
Confidence Score: 1/5This PR should not merge until the unrelated core test changes are removed or separated and the Postgres suite no longer reports unavailable-database runs as passing. The provider registration and Jest mappings are consistent, but the PR violates mandatory plugin scope confinement and changes enabled Postgres integration tests to succeed without executing assertions when the connection probe fails. Files Needing Attention: packages/corsair/tests/postgres-js-database.test.ts, packages/corsair/tests/probe.test.ts Important Files Changed
Reviews (1): Last reviewed commit: "feat: add Adrapid plugin" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const runner = liveDisabled ? it.skip : it; | ||
| return (name: string, fn: any, timeout?: number) => { | ||
| runner( | ||
| name, | ||
| async (...args: any[]) => { | ||
| if (!connectable) return; | ||
| return fn(...args); | ||
| }, | ||
| timeout, | ||
| ); |
There was a problem hiding this comment.
This plugin PR changes core database and probe tests outside the permitted packages/youcom/**, provider-registry, and lockfile footprint, mixing unrelated core test behavior into a plugin registration change.
Rule Used: A plugin PR must only modify files inside a single... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const runner = liveDisabled ? it.skip : it; | ||
| return (name: string, fn: any, timeout?: number) => { | ||
| runner( | ||
| name, |
There was a problem hiding this comment.
If SKIP_PG_TESTS is unset and Postgres is unreachable, the tests remain registered as active but this wrapper returns before running their assertions, causing Jest to report the integration suite as passing instead of exposing the unavailable database or a regression.
Plugin PR scorecard —
|
| Check | Status | Notes |
|---|---|---|
| R1 — Scope: plugin files only | ❌ | Out of scope: packages/corsair/tests/postgres-js-database.test.ts, packages/corsair/tests/probe.test.ts |
| R2 — Tests with assertions | ✅ | |
| R3 — Description complete | ✅ | |
| R3 — Linked issue / claim | No "Fixes #…" or claim link — add one if this PR has a claim or issue | |
| R4 — Demo video / recording | ❌ | Required in "Screenshots / Demos" before a maintainer reviews |
Rules: PLUGIN_PR_RULES.md · re-runs on every push
|
Hey @Vishuzz, thanks for the contribution! 🏴☠️ Before a maintainer reviews, please fix the items below — the review re-runs automatically on your next push. Must fix
Rule Used: A plugin PR must only modify files inside a single... (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
PR requirements (rules)
If anything remains after your next push, a bot commit will clean it up; a maintainer always does the final review and merge. |
![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
🤖 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/corsair/tests/postgres-js-database.test.ts`:
- Around line 161-173: Update the test gating around gated and beforeAll so
unavailable PostgreSQL does not make active tests pass without running their
bodies. Use a pre-registration skip mechanism when canConnect() is false, or
make beforeAll throw instead of setting connectable and returning; preserve
liveDisabled handling and ensure registered tests either are explicitly skipped
or fail.
- Around line 163-169: Update the test wrapper returned by the surrounding setup
so unavailable Postgres tests cannot return successfully when connectable is
false. When SKIP_PG_TESTS is unset and canConnect() fails, make beforeAll fail
or explicitly register the affected tests as skipped; preserve normal execution
when Postgres is available and the existing skip behavior when SKIP_PG_TESTS is
set.
🪄 Autofix
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 Plus
Run ID: b8bfddc5-00d0-4194-aede-7492a9c38a86
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/corsair/core/constants.tspackages/corsair/tests/postgres-js-database.test.tspackages/corsair/tests/probe.test.tspackages/youcom/jest.config.cjs
| const gated = () => { | ||
| const runner = liveDisabled ? it.skip : it; | ||
| return (name: string, fn: any, timeout?: number) => { | ||
| runner( | ||
| name, | ||
| async (...args: any[]) => { | ||
| if (!connectable) return; | ||
| return fn(...args); | ||
| }, | ||
| timeout, | ||
| ); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
file="packages/corsair/tests/postgres-js-database.test.ts"
printf '%s\n' '--- target sections ---'
sed -n '140,185p;785,830p' "$file"
printf '%s\n' '--- callback-style and wrapper usages ---'
rg -n -C 3 'gated|connectable|done\b|=>\s*\{[^}]*done|function\s*\([^)]*done' "$file"
printf '%s\n' '--- package and Jest configuration references ---'
rg -n -i -C 2 'jest|testTimeout|`@jest/globals`' package.json packages/corsair/package.json packages/corsair/jest.config.* jest.config.* 2>/dev/null || trueRepository: corsairdev/corsair
Length of output: 12462
🏁 Script executed:
#!/bin/bash
set -e
file="packages/corsair/tests/postgres-js-database.test.ts"
printf '%s\n' '--- connection probe and lifecycle ---'
sed -n '1,145p' "$file"
printf '%s\n' '--- static verifier for the gate behavior and callback-style usages ---'
python3 - <<'PY'
from pathlib import Path
import re
p = Path("packages/corsair/tests/postgres-js-database.test.ts")
s = p.read_text()
required = [
"const runner = liveDisabled ? it.skip : it;",
"async (...args: any[]) => {",
"if (!connectable) return;",
]
for fragment in required:
print(f"{fragment!r}: {fragment in s}")
before_all = re.search(
r"beforeAll\(async \(\) => \{(?P<body>.*?)\n\t\}, 30000\);",
s,
re.S,
)
if before_all:
body = before_all.group("body")
print("beforeAll assigns connectable:", "connectable = await canConnect();" in body)
print("beforeAll throws when canConnect is false:",
bool(re.search(r"if\s*\(\s*!connectable\s*\)\s*\{.*?\bthrow\b", body, re.S)))
print("beforeAll returns when canConnect is false:",
bool(re.search(r"if\s*\(\s*!connectable\s*\)\s*\breturn\b", body, re.S)))
# Find direct gated/runIf registrations whose callback parameter list names `done`.
for m in re.finditer(r"\b(?:gated\(\)|runIf)\s*\(\s*[\s\S]*?", s):
pass
print("callback-style `done` occurrences:", len(re.findall(r"\bdone\b", s)))
print("wrapper function length is zero by syntax:", "async (...args: any[]) =>" in s)
PYRepository: corsairdev/corsair
Length of output: 4666
🌐 Web query:
Jest 29 asynchronous tests callback done function.length async test returns undefined skipped test documentation
💡 Result:
In Jest, you should not combine the done callback with async/await in the same test [1][2]. When a test function is passed a done callback, Jest expects you to manually signal completion by invoking it [3][4]. If you additionally return a promise (which happens implicitly when using async), Jest will throw an error to prevent potential memory leaks and unpredictable behavior [3][5][1]. Key points regarding your query: 1. Combined Usage: You must choose either the done callback pattern or the promise-based (async/await) pattern [1][2]. If you mark a test function as async and also accept done as an argument, Jest will treat this as an invalid configuration [1]. 2. Handling "undefined" or Skipped Tests: If your test appears to be skipped or finishes prematurely returning "undefined," it is often because Jest completes the test as soon as the synchronous execution block ends, ignoring asynchronous operations that have not yet triggered [3][6]. - If using async, ensure you await your asynchronous operations or return the promise directly [7][4]. - If using done, ensure it is called explicitly [8][4]. If done is never called, the test will eventually fail with a timeout error rather than succeeding with an "undefined" state [3][8]. 3. Recommended Patterns: - For Promise-based code: Use async/await and return the result or await the promise [7][4]. - For Callback-based code: Accept the done argument and call it once your assertions are complete [3][4]. If an assertion fails, ensure you pass the error to done(error) so that Jest reports the failure correctly instead of just timing out [3][5]. If you are encountering a situation where a test seems to be ignored, verify that you are not accidentally triggering conditions that cause the runner to skip the test, and confirm that your test file is correctly matching your configuration patterns [2][9].
Citations:
- 1: Use async-await with done does not work since v27. jestjs/jest#11404
- 2: Test won't skip jestjs/jest#3603
- 3: https://jestjs.io/docs/29.7/asynchronous
- 4: https://github.com/jestjs/jest/blob/main/docs/TestingAsyncCode.md
- 5: https://jestjs.io/docs/asynchronous
- 6: https://stackoverflow.com/questions/70253271/jest-async-function-always-returns-undefined-never-resolves
- 7: https://jestjs.io/docs/tutorial-async
- 8: https://www.pluralsight.com/resources/blog/guides/test-asynchronous-code-jest
- 9: https://jestjs.io/docs/api
Fail when PostgreSQL is unavailable instead of returning from active tests.
When canConnect() returns false, beforeAll returns with connectable set to false. Each registered it test then returns before executing fn, so Jest reports it as passed without assertions. Use an explicit pre-registration skip gate or throw from beforeAll.
🤖 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/corsair/tests/postgres-js-database.test.ts` around lines 161 - 173,
Update the test gating around gated and beforeAll so unavailable PostgreSQL does
not make active tests pass without running their bodies. Use a pre-registration
skip mechanism when canConnect() is false, or make beforeAll throw instead of
setting connectable and returning; preserve liveDisabled handling and ensure
registered tests either are explicitly skipped or fail.
| return (name: string, fn: any, timeout?: number) => { | ||
| runner( | ||
| name, | ||
| async (...args: any[]) => { | ||
| if (!connectable) return; | ||
| return fn(...args); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'gated\(\)|runIf|done' packages/corsair/tests/postgres-js-database.test.tsRepository: corsairdev/corsair
Length of output: 7531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- outer setup and gated wrapper ---'
sed -n '1,185p' packages/corsair/tests/postgres-js-database.test.ts
printf '%s\n' '--- Slack setup and runIf wrapper ---'
sed -n '785,925p' packages/corsair/tests/postgres-js-database.test.ts
printf '%s\n' '--- callback-style signatures in this file ---'
rg -n -P '^\s*(?:gated|runIf|it|test)\([^;]*' packages/corsair/tests/postgres-js-database.test.ts | head -200
printf '%s\n' '--- Jest/package versions ---'
rg -n '"jest"|"`@jest/`"|jest-circus' package.json packages package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -80 || trueRepository: corsairdev/corsair
Length of output: 14992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const callbackStyle = (fn) => ({
length: fn.length,
source: String(fn),
receivesDone: fn.length > 0,
});
const wrapped = async (...args) => undefined;
const promiseTest = async () => undefined;
const doneTest = (done) => done();
console.log(JSON.stringify({
wrapped: callbackStyle(wrapped),
promiseTest: callbackStyle(promiseTest),
doneTest: callbackStyle(doneTest),
}, null, 2));
JSRepository: corsairdev/corsair
Length of output: 471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,180p' packages/corsair/tests/postgres-js-database.test.ts
sed -n '790,825p' packages/corsair/tests/postgres-js-database.test.ts
rg -n -P '^\s*(?:gated|runIf)\s*\(' packages/corsair/tests/postgres-js-database.test.ts
rg -n -P '\(\s*(?:done|next|callback|cb)\s*(?:[,)]|=>)' packages/corsair/tests/postgres-js-database.test.ts || true
rg -n '"jest"|"jest-circus"|`@jest/`' package.json packages/*/package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null | head -100 || trueRepository: corsairdev/corsair
Length of output: 9316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const wrapped = async (...args) => undefined;
const callbackTest = (done) => done();
console.log(`wrapped.length=${wrapped.length}`);
console.log(`callbackTest.length=${callbackTest.length}`);
JSRepository: corsairdev/corsair
Length of output: 195
Prevent unavailable Postgres tests from passing.
When SKIP_PG_TESTS is unset and canConnect() returns false, beforeAll returns with connectable set to false. The wrappers then return normally, so Jest reports active tests as passed without executing them. Make beforeAll fail when Postgres is unavailable, or register the tests as skipped.
🤖 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/corsair/tests/postgres-js-database.test.ts` around lines 163 - 169,
Update the test wrapper returned by the surrounding setup so unavailable
Postgres tests cannot return successfully when connectable is false. When
SKIP_PG_TESTS is unset and canConnect() fails, make beforeAll fail or explicitly
register the affected tests as skipped; preserve normal execution when Postgres
is available and the existing skip behavior when SKIP_PG_TESTS is set.
1 participant
Description
The
youcomplugin is already implemented underpackages/youcom/, but it was missing from the core provider registry.This PR registers
youcominpackages/corsair/core/constants.tsso it is recognized by the core provider system.Changes
youcomtoBaseProviders, alphabetically betweenxquikandyoutubeyoucom: 'You.com'toProviderDisplayNamesyoucomto theAllProvidersunion typepackages/youcom/jest.config.cjswith the required module mappingsVerification
pnpm run validate:plugins— passedpnpm typecheck— passedpnpm lint— passedpnpm --filter @corsair-dev/youcom test— 22/22 tests passedSummary by CodeRabbit
New Features
Bug Fixes
Tests