Conversation
📝 WalkthroughWalkthroughThis PR performs a domain migration from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/repositories/src/repos/UsdRepository/UsdRepositoryCow.ts (1)
108-108: Fix minor typo in inline comment.At Line 108,
See Seeandtokens islook accidental. Small cleanup will improve readability.✏️ Suggested wording
-// If tokens is not found, return null. See See https://api.cow.finance/mainnet/api/v1/token/this-is-not-a-token/native_price +// If token is not found, return null. See https://api.cow.finance/mainnet/api/v1/token/this-is-not-a-token/native_price🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/repositories/src/repos/UsdRepository/UsdRepositoryCow.ts` at line 108, Fix the inline comment in UsdRepositoryCow.ts inside the error handling comment: remove the duplicated "See" and correct "tokens is" to "token is" so the comment reads like "If token is not found, return null. See https://api.cow.finance/..."—update the comment near the token lookup logic in the UsdRepositoryCow class/function accordingly.libs/repositories/src/datasources/cowApi.ts (1)
3-4: Normalize the base URL before path concatenation.Good domain migration. One small hardening: if
COW_API_BASE_URLends with/, Line 17 can produce a double slash.💡 Suggested tweak
-const COW_API_BASE_URL = - process.env.COW_API_BASE_URL || 'https://api.cow.finance'; +const COW_API_BASE_URL = ( + process.env.COW_API_BASE_URL || 'https://api.cow.finance' +).replace(/\/+$/, '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/repositories/src/datasources/cowApi.ts` around lines 3 - 4, Normalize COW_API_BASE_URL to remove any trailing slash before concatenating paths: update the COW_API_BASE_URL initialization (the constant named COW_API_BASE_URL) to trim a trailing '/' (e.g., using .replace(/\/+$/, '') or new URL(base).origin) so subsequent concatenations that build endpoints (where you currently append paths to COW_API_BASE_URL) cannot produce a double slash; ensure all places that build URLs from COW_API_BASE_URL use the normalized value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/repositories/src/datasources/coingecko.test.ts`:
- Line 4: The test restores COINGECKO_API_KEY by assigning
process.env.COINGECKO_API_KEY = originalApiKey which can set the literal string
"undefined"; instead, update the teardown/restore logic around the
originalApiKey variable so that if originalApiKey is undefined you delete
process.env.COINGECKO_API_KEY, otherwise set process.env.COINGECKO_API_KEY =
originalApiKey; locate the restoration code referencing originalApiKey and
process.env.COINGECKO_API_KEY in the test file and replace the direct assignment
with this conditional delete-or-assign behavior.
---
Nitpick comments:
In `@libs/repositories/src/datasources/cowApi.ts`:
- Around line 3-4: Normalize COW_API_BASE_URL to remove any trailing slash
before concatenating paths: update the COW_API_BASE_URL initialization (the
constant named COW_API_BASE_URL) to trim a trailing '/' (e.g., using
.replace(/\/+$/, '') or new URL(base).origin) so subsequent concatenations that
build endpoints (where you currently append paths to COW_API_BASE_URL) cannot
produce a double slash; ensure all places that build URLs from COW_API_BASE_URL
use the normalized value.
In `@libs/repositories/src/repos/UsdRepository/UsdRepositoryCow.ts`:
- Line 108: Fix the inline comment in UsdRepositoryCow.ts inside the error
handling comment: remove the duplicated "See" and correct "tokens is" to "token
is" so the comment reads like "If token is not found, return null. See
https://api.cow.finance/..."—update the comment near the token lookup logic in
the UsdRepositoryCow class/function accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e36b0e06-5066-41be-89f4-2f6a28e10dab
⛔ Files ignored due to path filters (2)
libs/repositories/src/gen/cow/cow-api-types.tsis excluded by!**/gen/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
apps/api/src/app/routes/proxies/socket/index.tsapps/notification-producer/src/sendPush.test.tsapps/twap/src/app/utils/getApiBaseUrl.tslibs/repositories/src/datasources/coingecko.test.tslibs/repositories/src/datasources/cowApi.tslibs/repositories/src/repos/UsdRepository/UsdRepositoryCow.spec.tslibs/repositories/src/repos/UsdRepository/UsdRepositoryCow.tslibs/shared/src/utils/format.tspackage.json
| import { getCoingeckoProClient } from './coingecko'; | ||
|
|
||
| describe('getCoingeckoProClient', () => { | ||
| const originalApiKey = process.env.COINGECKO_API_KEY; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, what happens when assigning process.env.MY_VAR = undefined? Does it delete the variable or set it to the string "undefined"?
💡 Result:
Assigning process.env.MY_VAR = undefined sets the value to the string "undefined", not deleting the variable. To delete it, use delete process.env.MY_VAR. This is the documented behavior in the official Node.js documentation (v25.9.0), which states: "Assigning a property on process.env will implicitly convert the value to a string... env.test = undefined; console.log(env.test); // => 'undefined'". This behavior is deprecated, and future versions may throw an error for non-string/number/boolean values, but currently, it converts undefined to "undefined". Confirmed in Node.js GitHub issues #48875 and #9248, where developers note that assignment always stringifies the value, and delete is required to unset.
Citations:
- 1: Setting
undefinedtoprocess.env.*sets a"undefined"string value nodejs/node#48875 - 2: https://nodejs.org/docs/latest/api/process.html
- 3: https://nodejs.org/api/process.html
- 4: Allow assignment of undefined to process.env variables nodejs/node#9248
🏁 Script executed:
cat -n libs/repositories/src/datasources/coingecko.test.ts | head -50Repository: cowprotocol/bff
Length of output: 1781
Avoid restoring env vars via direct undefined assignment.
At lines 9 and 13, restoring with direct assignment sets "undefined" (string) when originalApiKey is absent, polluting the environment for other tests. In Node.js, assigning undefined to process.env.* coerces to the string "undefined" rather than deleting it; use delete instead, as shown on line 34.
Suggested fix
describe('getCoingeckoProClient', () => {
const originalApiKey = process.env.COINGECKO_API_KEY;
+
+ const restoreApiKey = () => {
+ if (originalApiKey === undefined) {
+ delete process.env.COINGECKO_API_KEY;
+ } else {
+ process.env.COINGECKO_API_KEY = originalApiKey;
+ }
+ };
beforeEach(() => {
jest.resetModules();
jest.clearAllMocks();
- process.env.COINGECKO_API_KEY = originalApiKey;
+ restoreApiKey();
});
afterAll(() => {
- process.env.COINGECKO_API_KEY = originalApiKey;
+ restoreApiKey();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/repositories/src/datasources/coingecko.test.ts` at line 4, The test
restores COINGECKO_API_KEY by assigning process.env.COINGECKO_API_KEY =
originalApiKey which can set the literal string "undefined"; instead, update the
teardown/restore logic around the originalApiKey variable so that if
originalApiKey is undefined you delete process.env.COINGECKO_API_KEY, otherwise
set process.env.COINGECKO_API_KEY = originalApiKey; locate the restoration code
referencing originalApiKey and process.env.COINGECKO_API_KEY in the test file
and replace the direct assignment with this conditional delete-or-assign
behavior.
|
Superseded by #213 |
1 participant
Summary
Bump to latest cow-sdk which points to the new .finance domain
Summary by CodeRabbit