Conversation
|
@keshav-005 is attempting to deploy a commit to the ravixalgorithm's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRefactors Finnhub integration with concrete TypeScript DTOs and a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/actions/finnhub.actions.ts`:
- Around line 44-53: The getExchangeLabel function currently uses the last
symbol segment after a '.' as a fallback which mislabels share-class tickers
(e.g., BRK.B) as exchange "B"; update getExchangeLabel to only accept suffix
fallbacks when the suffix matches a known exchange allowlist (e.g., TO, L, HK,
T, SS, SZ, AX, PA, etc.) or when suffix length >= 2, otherwise return 'US';
change references in getExchangeLabel (the symbol.split('.') logic and returned
suffix) to validate the suffix against that allowlist/length rule before
returning it.
🪄 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: 997004d2-a3b5-40f2-bf2a-5edc247d1921
📒 Files selected for processing (2)
components/SearchCommand.tsxlib/actions/finnhub.actions.ts
|
Good catch, I tightened the fallback so only known exchange suffixes or longer market suffixes are used, which avoids mislabeling share-class tickers like BRK.B as an exchange. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/actions/finnhub.actions.ts (1)
59-67: Allowlist is effectively bypassed by thesuffix.length >= 2fallback.Any symbol whose last
.-segment is 2+ characters will be treated as an exchange label, regardless of whether it's a real Finnhub exchange suffix. That weakens the allowlist you just added — e.g. a ticker likeFOO.BARwould renderBARas the exchange. Since the allowlist already enumerates the known Finnhub suffixes, consider dropping the length-based escape hatch and defaulting to'US'when the suffix isn't inFINNHUB_EXCHANGE_SUFFIXES.Proposed fix
- if (FINNHUB_EXCHANGE_SUFFIXES.has(suffix) || suffix.length >= 2) { - return suffix; - } - - return 'US'; + return FINNHUB_EXCHANGE_SUFFIXES.has(suffix) ? suffix : 'US';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/actions/finnhub.actions.ts` around lines 59 - 67, The allowlist is being bypassed by the fallback condition "suffix.length >= 2"; update the logic in the function that examines the parsed suffix (the code using the suffix variable and FINNHUB_EXCHANGE_SUFFIXES) to remove the "suffix.length >= 2" branch and instead only accept suffixes that exist in FINNHUB_EXCHANGE_SUFFIXES, returning 'US' for any suffix not present; ensure the function still returns 'US' when suffix is falsy and when it's not in FINNHUB_EXCHANGE_SUFFIXES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/actions/finnhub.actions.ts`:
- Around line 51-54: The guard in getExchangeLabel uses exchange?.trim() but
returns the raw exchange, which can include whitespace; update getExchangeLabel
to return exchange.trim() (not the untrimmed exchange) when the guard passes so
the label is consistent and free of leading/trailing whitespace, leaving the
fallback return (e.g., symbol) unchanged.
---
Nitpick comments:
In `@lib/actions/finnhub.actions.ts`:
- Around line 59-67: The allowlist is being bypassed by the fallback condition
"suffix.length >= 2"; update the logic in the function that examines the parsed
suffix (the code using the suffix variable and FINNHUB_EXCHANGE_SUFFIXES) to
remove the "suffix.length >= 2" branch and instead only accept suffixes that
exist in FINNHUB_EXCHANGE_SUFFIXES, returning 'US' for any suffix not present;
ensure the function still returns 'US' when suffix is falsy and when it's not in
FINNHUB_EXCHANGE_SUFFIXES.
🪄 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: f258abb8-e147-41e3-9937-9f06a44650b6
📒 Files selected for processing (1)
lib/actions/finnhub.actions.ts
|
Updated again: the helper now returns a trimmed explicit exchange value and only falls back to suffixes from the allowlist, defaulting to US otherwise. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/actions/finnhub.actions.ts (1)
25-27: Minor:__exchangeas a structural field onFinnhubSearchResultis a bit leaky.Using a
__-prefixed property to smuggle profile-derived exchange data through the candidate shape works, but it mixes Finnhub's wire type with an internal enrichment field and relies on convention to keep it out of downstream consumers. A cleaner alternative is a discriminated wrapper instead of extending the DTO:Optional refactor
-type SearchStockCandidate = FinnhubSearchResult & { - __exchange?: string; -}; +type SearchStockCandidate = { + raw: FinnhubSearchResult; + exchange?: string; +};…and adjust the two producers and the final
.mapaccordingly. Not a blocker — current code is correct given__exchangeis optional and only read in one place.Also applies to: 229-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/actions/finnhub.actions.ts` around lines 25 - 27, Replace the leaky __exchange structural field and the SearchStockCandidate alias by introducing a discriminated wrapper (e.g., a StockCandidate wrapper that contains the original FinnhubSearchResult DTO as a property plus an optional exchange field) and update all producers that create SearchStockCandidate and the consumer that reads __exchange (the final .map) to use the new wrapper shape instead of extending FinnhubSearchResult; keep the exchange optional and only read it from the wrapper when enriching results to preserve downstream DTO purity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/actions/finnhub.actions.ts`:
- Around line 25-27: Replace the leaky __exchange structural field and the
SearchStockCandidate alias by introducing a discriminated wrapper (e.g., a
StockCandidate wrapper that contains the original FinnhubSearchResult DTO as a
property plus an optional exchange field) and update all producers that create
SearchStockCandidate and the consumer that reads __exchange (the final .map) to
use the new wrapper shape instead of extending FinnhubSearchResult; keep the
exchange optional and only read it from the wrapper when enriching results to
preserve downstream DTO purity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 220d641a-de28-4407-b895-6f9ad6fd4ad3
📒 Files selected for processing (1)
lib/actions/finnhub.actions.ts
1 participant
Summary
Test plan
Fixes #55
Summary by CodeRabbit
Bug Fixes
Refactor