feat(paper-trading): add paper trading feature and related scripts#81
Open
ericosmic wants to merge 12 commits into
Open
feat(paper-trading): add paper trading feature and related scripts#81ericosmic wants to merge 12 commits into
ericosmic wants to merge 12 commits into
Conversation
Added a complete paper trading system, including account management and trade execution functionalities. Introduced multiple helper scripts for portfolio analysis, stock research, and automated trade execution. Configured dependency build options to optimize performance. - Added paper trading page and dashboard components - Implemented AI-driven automated simulated trading functionality - Added MongoDB data models for trade record management - Implemented automatic price fetching and caching mechanisms - Added portfolio analysis and risk monitoring features - Configured npm dependency build optimizations
Contributor
|
@ericosmic is attempting to deploy a commit to the ravixalgorithm's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete paper-trading feature: multi-account simulation, pending orders, AI-driven trade cycles, market-hours utilities, Finnhub filesystem caching, dashboard and UI components, and workspace/build configuration. ChangesPaper Trading System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
lib/actions/finnhub.actions.ts-12-12 (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a guaranteed-writable temp dir instead of
process.env.HOME.
HOMEis unset on Windows (falls back to/tmp, which isn't a valid path there) and is frequently read-only in serverless/containerized deploys where only the OS temp dir is writable. Deriving the cache dir fromos.tmpdir()avoids these failure modes.🛠️ Proposed fix
+import * as os from 'os'; ... -const FINNHUB_CACHE_DIR = path.join(process.env.HOME || '/tmp', '.hermes', 'cache', 'finnhub'); +const FINNHUB_CACHE_DIR = path.join(os.tmpdir(), '.hermes', 'cache', 'finnhub');🤖 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 `@lib/actions/finnhub.actions.ts` at line 12, Replace the current FINNHUB_CACHE_DIR construction which uses process.env.HOME with a guaranteed-writable temp directory by using os.tmpdir(); update the symbol FINNHUB_CACHE_DIR to join path.join(os.tmpdir(), '.hermes', 'cache', 'finnhub') (importing os if missing) so the cache lives under the OS temp directory instead of HOME._scripts/sector_analysis.js-52-56 (1)
52-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
q.dpcan beundefinedeven whenq.cis set →q.dp.toFixed(2)throws.The guard only checks
q.c. If Finnhub returns a quote withoutdp, Line 56'sq.dp.toFixed(2)throwsTypeError, aborting the whole loop. Defaultdpbefore use.🛠️ Proposed fix
if (q && q.c) { - const chg = q.dp >= 0 ? '+' : ''; - const emoji = q.dp > 2 ? '🔥' : q.dp > 0 ? '📈' : q.dp > -2 ? '📉' : '💀'; + const dp = typeof q.dp === 'number' ? q.dp : 0; + const chg = dp >= 0 ? '+' : ''; + const emoji = dp > 2 ? '🔥' : dp > 0 ? '📈' : dp > -2 ? '📉' : '💀'; const mv = pp?.marketCapitalization ? '$' + (pp.marketCapitalization / 1e9).toFixed(1) + 'B' : ''; - console.log(emoji + ' ' + s.sym.padEnd(6) + s.name.padEnd(22) + '$' + String(q.c).padStart(8) + ' ' + chg + q.dp.toFixed(2) + '% ' + mv); + console.log(emoji + ' ' + s.sym.padEnd(6) + s.name.padEnd(22) + '$' + String(q.c).padStart(8) + ' ' + chg + dp.toFixed(2) + '% ' + mv);🤖 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 `@_scripts/sector_analysis.js` around lines 52 - 56, The issue is that q.dp may be undefined causing q.dp.toFixed(2) to throw; fix by normalizing dp before use: create a local numeric dp (e.g. const dp = typeof q.dp === 'number' ? q.dp : 0) and then use dp for the emoji selection, chg calculation and formatting instead of q.dp; update references in the block that compute emoji, chg and the toFixed output to use this dp variable so the loop won't throw when dp is missing._scripts/check_positions.js-12-12 (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
process.env.HOMEis undefined on Windows.
path.join(process.env.HOME, ...)throwsTypeErrorwhenHOMEis unset (Windows usesUSERPROFILE). Preferos.homedir().🛠️ Proposed fix
+const os = require('os'); -const cacheDir = path.join(process.env.HOME, '.hermes', 'cache', 'finnhub'); +const cacheDir = path.join(os.homedir(), '.hermes', 'cache', 'finnhub');🤖 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 `@_scripts/check_positions.js` at line 12, Replace use of process.env.HOME with the cross-platform API os.homedir(): add a require/import for the os module (const os = require('os')) and change the cacheDir initialization (the const cacheDir = path.join(...)) to use os.homedir() instead of process.env.HOME so it works on Windows and when HOME is unset._scripts/execute_trades.js-10-10 (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
.envis read relative to the current working directory, not the script.Unlike the sibling scripts (which use
path.join(__dirname, '..', '.env')), this reads'.env'fromprocess.cwd(), so the token resolves to''unless the script is run from the repo root. Make the path script-relative for consistency.🛠️ Proposed fix
-const token = fs.readFileSync('.env', 'utf8').match(/NEXT_PUBLIC_FINNHUB_API_KEY=(\S+)/)?.[1] || ''; +const token = fs.readFileSync(path.join(__dirname, '..', '.env'), 'utf8').match(/NEXT_PUBLIC_FINNHUB_API_KEY=(\S+)/)?.[1] || '';🤖 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 `@_scripts/execute_trades.js` at line 10, The token is read from '.env' relative to process.cwd() which breaks when the script is executed outside the repo root; update the read to use a script-relative path by constructing the env file path with __dirname (e.g. path.join(__dirname, '..', '.env')) and pass that into fs.readFileSync where the token is defined (the const token declaration) so the script consistently finds the repo .env regardless of current working directory._scripts/check_positions.js-1-5 (1)
1-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix ESLint failures from CommonJS
require()in_scripts/*.js
@typescript-eslint/no-require-importsis enforced and fails lint on_scripts/check_positions.js(and the other_scripts/*.jsfiles) due torequire()usage;_scripts/isn’t ignored ineslint.config.mjs(it only ignoresnode_modules/**,.next/**,out/**,build/**,next-env.d.ts). Convert these scripts to ESMimportor exclude_scripts// disable@typescript-eslint/no-require-importsfor that folder.🤖 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 `@_scripts/check_positions.js` around lines 1 - 5, The CommonJS requires (e.g., const { MongoClient } = require('mongodb'), const undici = require('undici'), const fs = require('fs'), const path = require('path'), const crypto = require('crypto')) must be converted to ESM imports to satisfy `@typescript-eslint/no-require-imports` for files under _scripts; replace them with import statements like import { MongoClient } from 'mongodb'; import * as undici from 'undici' (or import specific named exports you use); import fs from 'fs'; import path from 'path'; import crypto from 'crypto'; and ensure the project treats these scripts as ESM (add "type":"module" to package.json or rename the scripts to .mjs) so Node will accept the import syntax.
🧹 Nitpick comments (6)
lib/actions/paper-trading.actions.ts (1)
153-157: ⚡ Quick winResolve the
no-explicit-anylint errors flagged across this file.ESLint reports
@typescript-eslint/no-explicit-anyas errors on the(account as any)casts here (and at Lines 179-188, 222, 225, 408, 460-462). Since the rule is at error level it will block CI. The casts stem fromfindById(...).lean()losing the document type — typing the lean result (e.g.PaperTradingAccount) removes most of them.🤖 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 `@lib/actions/paper-trading.actions.ts` around lines 153 - 157, The code is using (account as any) casts because Mongoose .lean() lost the type; fix by properly typing the lean result (e.g. call .lean<PaperTradingAccount | null>() or assign the result to a variable typed as PaperTradingAccount | null) when fetching the document in the functions that use account (look for usages of findById(...).lean() and the local variable account in lib/actions/paper-trading.actions.ts), then remove all (account as any) casts and access properties directly (tradingPeriod, startDate, endDate, customPeriodDays, name, and the other usages at the later locations referenced in the review). Ensure other occurrences (lines ~179-188, 222, 225, 408, 460-462) are similarly updated by replacing explicit any casts with the correct typed lean result or by narrowing the type before mutation.lib/actions/finnhub.actions.ts (1)
47-53: ⚡ Quick winSynchronous filesystem I/O blocks the event loop.
Inside a
'use server'action,fs.existsSync/readFileSync(here and at Lines 66-67, 148-149) andmkdirSync/writeFileSync(Lines 60-61) run synchronously and stall all concurrent requests handled by the same Node process. Consider switching tofs.promises(await fs.promises.readFile, etc.) since the surrounding functions are already async.🤖 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 `@lib/actions/finnhub.actions.ts` around lines 47 - 53, Replace all synchronous fs calls (fs.existsSync, fs.readFileSync, fs.mkdirSync, fs.writeFileSync) with their async Promise counterparts to avoid blocking the event loop: use await fs.promises.readFile to read cacheFile and JSON.parse the result, or await fs.promises.stat/access to check existence, and use await fs.promises.mkdir(cacheDir, { recursive: true }) and await fs.promises.writeFile for writes; update the try/catch around the cache logic that uses cacheFile and revalidateSeconds to await these operations and preserve existing error handling and return types, and make the equivalent changes for the other sync usages in this module (the blocks that currently call fs.existsSync/readFileSync and mkdirSync/writeFileSync)._scripts/check_positions.js (2)
8-10: ⚡ Quick winHardcoded proxy makes the script fail wherever no proxy runs on
127.0.0.1:7890.All requests are routed through a fixed local
ProxyAgent; on any machine without that proxy,fetchwill error out. Consider deriving the proxy from an env var (e.g.HTTPS_PROXY) and falling back to a direct dispatcher when unset.♻️ Suggested change
-const PROXY = 'http://127.0.0.1:7890'; -const dispatcher = new undici.ProxyAgent(PROXY); -const fetch = (url) => undici.fetch(url, { dispatcher }); +const PROXY = process.env.HTTPS_PROXY || process.env.HTTP_PROXY || ''; +const dispatcher = PROXY ? new undici.ProxyAgent(PROXY) : undefined; +const fetch = (url) => undici.fetch(url, dispatcher ? { dispatcher } : undefined);🤖 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 `@_scripts/check_positions.js` around lines 8 - 10, The script currently hardcodes PROXY and always creates dispatcher = new undici.ProxyAgent(PROXY) and a wrapped fetch that passes that dispatcher, which breaks on machines without a local proxy; change it to read the proxy URL from an environment variable (e.g. process.env.HTTPS_PROXY || process.env.https_proxy), only construct new undici.ProxyAgent(proxy) when that env var is set, and make the fetch wrapper (the fetch function defined in this file) conditionally include the dispatcher option (omit the dispatcher or pass undefined when no proxy is configured) so requests use a direct connection when no proxy is present; update references to PROXY, dispatcher, ProxyAgent and fetch accordingly.
15-26: ⚡ Quick win
getQuotedoesn't check the HTTP response and caches error payloads.If Finnhub returns a non-200 (e.g. rate limit / auth error),
r.json()yields an error object with noc, sopricesilently becomes0, and that bad payload is cached on disk for 60s. Validate the response before caching.♻️ Suggested guard
const r = await fetch('https://finnhub.io/api/v1/quote?symbol=' + sym + '&token=' + token); + if (!r.ok) throw new Error(`Finnhub ${r.status} for ${sym}`); const d = await r.json(); + if (typeof d.c !== 'number') throw new Error(`Invalid quote for ${sym}: ${JSON.stringify(d)}`); fs.writeFileSync(cf, JSON.stringify({ ts: Date.now(), data: d }));🤖 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 `@_scripts/check_positions.js` around lines 15 - 26, getQuote currently calls r.json() and caches the payload without checking HTTP status, so non-200 error bodies get written to cf; change getQuote to validate the fetch response (check r.ok or r.status) before writing the cache file (cf) or returning data: if the response is not ok, read the response body or text for diagnostics but do NOT write to the cache and either throw an error or return a clear error value; ensure you only JSON.parse/fs.writeFileSync the successful payload (d) and preserve existing timeout logic that uses Date.now() and token/cacheDir variables.components/paper-trading/AccountSwitcher.tsx (1)
14-21: 💤 Low valueUnused prop
onAccountListChangein component signature.The prop
onAccountListChangeis defined inPropsinterface but never used in the component. Either remove it from the interface or implement the callback where appropriate (e.g., after create/delete operations).🤖 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 `@components/paper-trading/AccountSwitcher.tsx` around lines 14 - 21, Props defines onAccountListChange but AccountSwitcher never uses it; remove onAccountListChange from the Props interface and the AccountSwitcher parameter list if you don't need the callback, or if you do need it, wire it up by adding the callback parameter to the function signature (AccountSwitcher) and invoking onAccountListChange() after account create/delete flows (where createAccount, deleteAccount or similar handlers are implemented) so the parent is notified; update any call sites that render <AccountSwitcher ... /> to pass the prop only if kept.lib/actions/ai-trading.actions.ts (1)
249-265: ⚖️ Poor tradeoffFragile price extraction via regex on localized message strings.
Parsing price from the success message using regex (
/\$([\d.]+)/g) is brittle. If the message format changes or contains other dollar amounts, extraction may fail or return wrong values. The executed trade record will haveprice: 0and incorrecttotal.Consider returning structured data from
buyStock/sellStock(e.g.,{ success, price, total }) rather than parsing natural language messages.🤖 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 `@lib/actions/ai-trading.actions.ts` around lines 249 - 265, The price extraction from result.message using regex in the buyStock/sellStock call sites is brittle; modify the buyStock and sellStock functions to return structured data (e.g., { success, price, total, message }) and update the usage in ai-trading.actions (where buyStock, sellStock are called and executed.push is built) to read price and total directly from the returned object (use result.price and result.total), removing the regex parsing; if you cannot change the underlying functions immediately, add a robust fallback: attempt to read result.price/result.total first, then only as a last resort parse result.message with a stricter pattern and handle parse failures by flagging an error instead of defaulting to 0.
🤖 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 `@_scripts/execute_trades.js`:
- Around line 70-89: This script currently unconditionally inserts each BUY
(inside the for loop using trades.insertOne) and then updates the account
balance with accounts.updateOne, causing duplicate buys and double-debits when
rerun; fix by generating a deterministic batchId (or orderBatchId) for this run,
add/require a unique index on the trades collection keyed by batchId (and
symbol/shares if needed), check for an existing marker via trades.findOne({
batchId }) before inserting, and if not present perform all inserts and the
balance update inside a single MongoDB transaction (use a session) so
trades.insertOne and accounts.updateOne (referencing USER_ID, totalCost and
acc._id) are atomic and idempotent.
In `@components/paper-trading/AccountSwitcher.tsx`:
- Around line 59-74: handleDelete has a race where you filter the stale accounts
array after awaiting loadAccounts(); change it to use the fresh list returned by
loadAccounts (or modify loadAccounts to return the new accounts) instead of
referencing the old accounts variable: after calling await loadAccounts(),
capture its returned array (from loadAccounts) and compute remaining =
returnedAccounts.filter(a => a._id !== accountId), then call
onAccountChange(remaining[0]._id) if any; reference functions/variables:
handleDelete, loadAccounts, deleteAccount, accounts, onAccountChange.
In `@components/paper-trading/AITradingPanel.tsx`:
- Around line 120-128: handleToggle is calling toggleAITrading with userId as
the first arg but the server action toggleAITrading(accountId, enabled, userId?)
expects accountId first; update the call inside handleToggle to pass the correct
accountId (e.g., accountId or config.accountId) as the first argument and pass
userId only as the optional third argument if required, then keep the rest of
the logic (setConfig and setMessage) unchanged so the correct AI config document
is updated.
- Around line 130-145: In AITradingPanel update the handleRun function to pass
the account ID to the server action: replace the incorrect userId argument with
the accountId (or the prop/state variable that holds the account identifier)
when calling runAITradeCycle; ensure the correct identifier name used in this
component is referenced so runAITradeCycle(accountId) can load the AI config and
portfolio correctly.
In `@database/models/paper-trading.model.ts`:
- Around line 95-111: The AIConfigSchema currently persists apiKey as plaintext
(field apiKey in AIConfigSchema), so change storage to avoid plaintext: either
replace apiKey with a secret reference (e.g., secretRef) that holds an opaque ID
to a secrets manager, or implement application-level encryption via a Mongoose
pre-save hook on AIConfigSchema that encrypts apiKey with a KMS-derived key and
decrypts only when explicitly needed; additionally ensure apiKey is excluded
from outgoing documents by adding schema toJSON/toObject transforms (or use
select: false) so apiKey is never returned to clients, and update any codepaths
that read/write apiKey to use the secretRef or explicit decrypt helper.
In `@lib/actions/ai-trading.actions.ts`:
- Around line 61-75: In AITradingPanel, the call uses toggleAITrading(userId,
newEnabled) but the function expects accountId as the first argument; update the
caller in the AITradingPanel component to pass the correct account identifier
(e.g., change toggleAITrading(userId, newEnabled) to toggleAITrading(accountId,
newEnabled)), ensuring the accountId variable is available/derived in that scope
and not accidentally using userId.
- Around line 118-126: The caller in AITradingPanel incorrectly passes userId to
runAITradeCycle (which expects an accountId) causing config lookup to fail;
locate the call to runAITradeCycle in AITradingPanel.tsx and change the argument
from userId to the correct accountId value (or derive accountId from the current
account object/props) and update any related typing if necessary so
runAITradeCycle(accountId: string) receives the actual accountId rather than the
userId.
- Around line 196-226: The undici LLM call in this block lacks a timeout and
uses a hardcoded fallback proxy; update the logic so that proxy usage is
conditional (only create ProxyAgent/dispatcher when process.env.HTTPS_PROXY is
set and do NOT default to 'http://127.0.0.1:7890') and add an abort timeout
around undici.fetch (use AbortController/abort signal and a clearable
setTimeout) to ensure the request is cancelled after a sensible timeout; update
references in this block (proxyUrl, ProxyAgent, dispatcher, undici.fetch, and
the try/catch) so dispatcher is omitted from the fetch options when no proxy is
configured and the fetch call is aborted on timeout to surface an error to the
existing catch.
In `@lib/actions/finnhub.actions.ts`:
- Around line 58-73: The current try around doFetch<T>(url, timeoutMs) also
performs fs.mkdirSync(FINNHUB_CACHE_DIR, { recursive: true }) and
fs.writeFileSync(cacheFile, ...), so any write error causes the catch to run and
rethrows even though the fetch succeeded; change this so the filesystem
operations are best-effort: after obtaining const data (from doFetch)
immediately return data to callers and/or move the mkdirSync/writeFileSync into
their own nested try/catch that swallows/logs errors (do not rethrow) so that
getCompanyProfile/getNews/searchStocks (and functions using FINNHUB_CACHE_DIR,
cacheFile, now) always return the fetched data even if cache writes fail.
- Around line 144-156: The stale-cache fallback never runs because token and url
are declared inside the try and thus may be referenced out of scope, and
fetchJSON is called with revalidateSeconds === 0 so it never writes a cache for
the fallback to read; fix getQuote by hoisting the Finnhub auth token and the
constructed url (the variables referenced in getQuote) into the outer scope
before the try/catch (so token/url are available in the fallback block and error
handlers) and call fetchJSON(url, SHORT_TTL) with a small non-zero
revalidateSeconds (e.g., 30) so fetchJSON will persist the quote to
FINNHUB_CACHE_DIR and allow the existing stale-cache lookup (which checks
FINNHUB_CACHE_DIR and url md5) to succeed; also ensure the inner catch does not
reference scoped vars and only swallows read errors, preserving the intended
fallback return behavior in getQuote.
- Around line 87-105: In doFetch, replace the require('undici') usage with a
dynamic import (await import('undici')) to match other modules, change the
actualFetch typing from a custom signature to typeof globalThis.fetch and remove
the init?: any, and avoid creating a new ProxyAgent on every call by
caching/reusing a single ProxyAgent/dispatcher per proxyUrl (or a module-scoped
map keyed by proxyUrl) so you can reuse it and call its close() when
appropriate; update the code paths that set actualFetch to use the cached
dispatcher (new undici.ProxyAgent only when no cached agent exists) and ensure
types reference ProxyAgent/dispatcher from the imported undici module.
In `@lib/actions/paper-trading.actions.ts`:
- Around line 343-382: The sell flow has a TOCTOU race: holdings are read via
db.collection('papertrades').aggregate and later a SELL insert (and
PaperAccount.updateOne) happens outside an atomic context; fix by doing the
holdings check, balance update (PaperAccount.updateOne) and trade insert
(db.collection('papertrades').insertOne) inside a single MongoDB
transaction/session so the holdings are re-read and validated within the same
transaction and concurrent sells cannot both pass. Concretely, start a client
session (session = client.startSession()), run session.withTransaction(async ()
=> { recompute holdings using
db.collection('papertrades').aggregate(...).toArray({ session }), if held <
shares throw; await PaperAccount.updateOne(..., { $inc: { balance: total } }, {
session }); await db.collection('papertrades').insertOne({...}, { session }) }),
and ensure all DB calls (aggregate, updateOne, insertOne) include the session
param and that you abort/handle errors from the transaction.
- Around line 275-298: The buyStock and sellStock flows currently use the native
driver (db.collection('papertrades').insertOne(...)) which bypasses Mongoose
schema validation and allows non-positive or non-integer shares; add an explicit
guard at the top of both buyStock and sellStock to validate shares is an integer
>= 1 (reject otherwise with a clear error like 'shares must be a positive
integer'), and ensure this check runs before any price/held/balance calculations
or the native insert; reference the buyStock and sellStock functions and the
Trade schema constraint (shares: { min: 1 }) when adding the validation.
- Around line 300-318: The balance check and separate decrement are racy;
replace the two-step check-then-update with an atomic conditional update on
PaperAccount (e.g., use updateOne or findOneAndUpdate with filter {_id:
accountId, balance: {$gte: total}} and $inc: {balance: -total}) and verify the
update result (modifiedCount or returned value) succeeded before calling
db.collection('papertrades').insertOne; if the atomic update failed, return the
insufficient-funds error; keep accountId, total, roundedPrice, symbol and the
trade insert logic intact but only run the insert after the successful
conditional update.
- Around line 205-243: The aggregation and P/L math treat totalCost as the cost
of remaining shares which is wrong after partial sells; update the aggregation
to separately sum buyShares, sellShares, buyCost (sum of BUY totals) and
sellProceeds (sum of SELL totals) instead of only totalCost/totalProceeds,
compute shares = buyShares - sellShares, compute costPerBuyShare = buyCost /
buyShares (guard buyShares > 0), derive remainingCost = costPerBuyShare *
shares, then set avgCost = remainingCost / shares, pl = marketValue -
remainingCost, and plPercent based on remainingCost; change references in the
mapping (pos.totalCost, pos.shares, avgCost, pl, plPercent) to use the new
fields (buyShares, sellShares, buyCost, remainingCost) so partial sells produce
correct unrealized P/L.
In `@pnpm-workspace.yaml`:
- Around line 1-6: Replace the placeholder strings under allowBuilds with
boolean values and remove the unsupported onlyBuiltDependencies setting: change
allowBuilds to a map like "allowBuilds:\n protobufjs: true\n sharp: false\n
unrs-resolver: false" (or set true/false per package policy you want) so each
package matcher is mapped to a boolean, delete the onlyBuiltDependencies line
entirely, and keep nodeLinker: hoisted as-is; update the allowBuilds entries for
protobufjs, sharp and unrs-resolver accordingly.
---
Minor comments:
In `@_scripts/check_positions.js`:
- Line 12: Replace use of process.env.HOME with the cross-platform API
os.homedir(): add a require/import for the os module (const os = require('os'))
and change the cacheDir initialization (the const cacheDir = path.join(...)) to
use os.homedir() instead of process.env.HOME so it works on Windows and when
HOME is unset.
- Around line 1-5: The CommonJS requires (e.g., const { MongoClient } =
require('mongodb'), const undici = require('undici'), const fs = require('fs'),
const path = require('path'), const crypto = require('crypto')) must be
converted to ESM imports to satisfy `@typescript-eslint/no-require-imports` for
files under _scripts; replace them with import statements like import {
MongoClient } from 'mongodb'; import * as undici from 'undici' (or import
specific named exports you use); import fs from 'fs'; import path from 'path';
import crypto from 'crypto'; and ensure the project treats these scripts as ESM
(add "type":"module" to package.json or rename the scripts to .mjs) so Node will
accept the import syntax.
In `@_scripts/execute_trades.js`:
- Line 10: The token is read from '.env' relative to process.cwd() which breaks
when the script is executed outside the repo root; update the read to use a
script-relative path by constructing the env file path with __dirname (e.g.
path.join(__dirname, '..', '.env')) and pass that into fs.readFileSync where the
token is defined (the const token declaration) so the script consistently finds
the repo .env regardless of current working directory.
In `@_scripts/sector_analysis.js`:
- Around line 52-56: The issue is that q.dp may be undefined causing
q.dp.toFixed(2) to throw; fix by normalizing dp before use: create a local
numeric dp (e.g. const dp = typeof q.dp === 'number' ? q.dp : 0) and then use dp
for the emoji selection, chg calculation and formatting instead of q.dp; update
references in the block that compute emoji, chg and the toFixed output to use
this dp variable so the loop won't throw when dp is missing.
In `@lib/actions/finnhub.actions.ts`:
- Line 12: Replace the current FINNHUB_CACHE_DIR construction which uses
process.env.HOME with a guaranteed-writable temp directory by using os.tmpdir();
update the symbol FINNHUB_CACHE_DIR to join path.join(os.tmpdir(), '.hermes',
'cache', 'finnhub') (importing os if missing) so the cache lives under the OS
temp directory instead of HOME.
---
Nitpick comments:
In `@_scripts/check_positions.js`:
- Around line 8-10: The script currently hardcodes PROXY and always creates
dispatcher = new undici.ProxyAgent(PROXY) and a wrapped fetch that passes that
dispatcher, which breaks on machines without a local proxy; change it to read
the proxy URL from an environment variable (e.g. process.env.HTTPS_PROXY ||
process.env.https_proxy), only construct new undici.ProxyAgent(proxy) when that
env var is set, and make the fetch wrapper (the fetch function defined in this
file) conditionally include the dispatcher option (omit the dispatcher or pass
undefined when no proxy is configured) so requests use a direct connection when
no proxy is present; update references to PROXY, dispatcher, ProxyAgent and
fetch accordingly.
- Around line 15-26: getQuote currently calls r.json() and caches the payload
without checking HTTP status, so non-200 error bodies get written to cf; change
getQuote to validate the fetch response (check r.ok or r.status) before writing
the cache file (cf) or returning data: if the response is not ok, read the
response body or text for diagnostics but do NOT write to the cache and either
throw an error or return a clear error value; ensure you only
JSON.parse/fs.writeFileSync the successful payload (d) and preserve existing
timeout logic that uses Date.now() and token/cacheDir variables.
In `@components/paper-trading/AccountSwitcher.tsx`:
- Around line 14-21: Props defines onAccountListChange but AccountSwitcher never
uses it; remove onAccountListChange from the Props interface and the
AccountSwitcher parameter list if you don't need the callback, or if you do need
it, wire it up by adding the callback parameter to the function signature
(AccountSwitcher) and invoking onAccountListChange() after account create/delete
flows (where createAccount, deleteAccount or similar handlers are implemented)
so the parent is notified; update any call sites that render <AccountSwitcher
... /> to pass the prop only if kept.
In `@lib/actions/ai-trading.actions.ts`:
- Around line 249-265: The price extraction from result.message using regex in
the buyStock/sellStock call sites is brittle; modify the buyStock and sellStock
functions to return structured data (e.g., { success, price, total, message })
and update the usage in ai-trading.actions (where buyStock, sellStock are called
and executed.push is built) to read price and total directly from the returned
object (use result.price and result.total), removing the regex parsing; if you
cannot change the underlying functions immediately, add a robust fallback:
attempt to read result.price/result.total first, then only as a last resort
parse result.message with a stricter pattern and handle parse failures by
flagging an error instead of defaulting to 0.
In `@lib/actions/finnhub.actions.ts`:
- Around line 47-53: Replace all synchronous fs calls (fs.existsSync,
fs.readFileSync, fs.mkdirSync, fs.writeFileSync) with their async Promise
counterparts to avoid blocking the event loop: use await fs.promises.readFile to
read cacheFile and JSON.parse the result, or await fs.promises.stat/access to
check existence, and use await fs.promises.mkdir(cacheDir, { recursive: true })
and await fs.promises.writeFile for writes; update the try/catch around the
cache logic that uses cacheFile and revalidateSeconds to await these operations
and preserve existing error handling and return types, and make the equivalent
changes for the other sync usages in this module (the blocks that currently call
fs.existsSync/readFileSync and mkdirSync/writeFileSync).
In `@lib/actions/paper-trading.actions.ts`:
- Around line 153-157: The code is using (account as any) casts because Mongoose
.lean() lost the type; fix by properly typing the lean result (e.g. call
.lean<PaperTradingAccount | null>() or assign the result to a variable typed as
PaperTradingAccount | null) when fetching the document in the functions that use
account (look for usages of findById(...).lean() and the local variable account
in lib/actions/paper-trading.actions.ts), then remove all (account as any) casts
and access properties directly (tradingPeriod, startDate, endDate,
customPeriodDays, name, and the other usages at the later locations referenced
in the review). Ensure other occurrences (lines ~179-188, 222, 225, 408,
460-462) are similarly updated by replacing explicit any casts with the correct
typed lean result or by narrowing the type before mutation.
🪄 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: 551973cd-a201-4470-ab5f-4daf86449dbc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.npmrc_scripts/check_positions.js_scripts/execute_trades.js_scripts/research_candidates.js_scripts/round2_trades.js_scripts/round3_trades.js_scripts/sector_analysis.jsapp/(root)/paper-trading/page.tsxcomponents/paper-trading/AITradingPanel.tsxcomponents/paper-trading/AccountSwitcher.tsxcomponents/paper-trading/PaperTradingDashboard.tsxdatabase/models/paper-trading.model.tslib/actions/ai-trading.actions.tslib/actions/finnhub.actions.tslib/actions/paper-trading.actions.tslib/constants.tspackage.jsonpnpm-workspace.yaml
Comment on lines
+70
to
+89
| for (const o of orders) { | ||
| console.log('BUY ' + o.shares + ' x ' + o.sym + ' @ $' + o.price.toFixed(2) + ' = $' + o.cost.toFixed(2)); | ||
| await trades.insertOne({ | ||
| userId: USER_ID, | ||
| symbol: o.sym, | ||
| company: o.company, | ||
| type: 'BUY', | ||
| shares: o.shares, | ||
| price: o.price, | ||
| total: o.cost, | ||
| timestamp: new Date(), | ||
| }); | ||
| } | ||
|
|
||
| // Update balance | ||
| const newBalance = +(acc.balance - totalCost).toFixed(2); | ||
| await accounts.updateOne( | ||
| { _id: acc._id }, | ||
| { $set: { balance: newBalance, updatedAt: new Date() } } | ||
| ); |
Contributor
There was a problem hiding this comment.
Trade execution is not idempotent — re-running double-buys and corrupts the balance.
Each run unconditionally inserts new BUY documents and subtracts totalCost from balance. Running the script twice (a likely occurrence for a manual script) silently duplicates positions and double-debits cash, with no way to detect or roll back. Consider a guard such as a deterministic order/batch id (unique index on papertrades) or a "round already executed" check before inserting, and ideally perform the inserts + balance update inside a single MongoDB transaction so a mid-run failure doesn't leave trades recorded without the balance updated (or vice versa).
🤖 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 `@_scripts/execute_trades.js` around lines 70 - 89, This script currently
unconditionally inserts each BUY (inside the for loop using trades.insertOne)
and then updates the account balance with accounts.updateOne, causing duplicate
buys and double-debits when rerun; fix by generating a deterministic batchId (or
orderBatchId) for this run, add/require a unique index on the trades collection
keyed by batchId (and symbol/shares if needed), check for an existing marker via
trades.findOne({ batchId }) before inserting, and if not present perform all
inserts and the balance update inside a single MongoDB transaction (use a
session) so trades.insertOne and accounts.updateOne (referencing USER_ID,
totalCost and acc._id) are atomic and idempotent.
Comment on lines
+95
to
+111
| const AIConfigSchema = new Schema<AITradingConfig>( | ||
| { | ||
| accountId: { type: String, required: true, unique: true, index: true }, | ||
| userId: { type: String, required: true, index: true }, | ||
| enabled: { type: Boolean, default: false }, | ||
| apiEndpoint: { type: String, default: 'https://api.openai.com/v1/chat/completions' }, | ||
| apiKey: { type: String, default: '' }, | ||
| model: { type: String, default: 'gpt-4o' }, | ||
| systemPrompt: { type: String, default: '' }, | ||
| strategy: { type: String, enum: ['aggressive', 'moderate', 'conservative', 'custom'], default: 'moderate' }, | ||
| maxPositionPct: { type: Number, default: 25, min: 1, max: 100 }, | ||
| stopLossPct: { type: Number, default: -10, max: 0 }, | ||
| tradingIntervalMin: { type: Number, default: 60, min: 5 }, | ||
| lastTradeAt: { type: Date, default: null }, | ||
| }, | ||
| { timestamps: true } | ||
| ); |
Contributor
There was a problem hiding this comment.
API keys are persisted in plaintext.
apiKey is stored as a raw String. Any DB dump, backup, or log of these documents leaks user-provided LLM credentials. Consider encrypting at rest (e.g. application-level envelope encryption) or storing only a reference to a secrets manager, and ensure this field is never returned to the client unredacted.
🤖 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 `@database/models/paper-trading.model.ts` around lines 95 - 111, The
AIConfigSchema currently persists apiKey as plaintext (field apiKey in
AIConfigSchema), so change storage to avoid plaintext: either replace apiKey
with a secret reference (e.g., secretRef) that holds an opaque ID to a secrets
manager, or implement application-level encryption via a Mongoose pre-save hook
on AIConfigSchema that encrypts apiKey with a KMS-derived key and decrypts only
when explicitly needed; additionally ensure apiKey is excluded from outgoing
documents by adding schema toJSON/toObject transforms (or use select: false) so
apiKey is never returned to clients, and update any codepaths that read/write
apiKey to use the secretRef or explicit decrypt helper.
Comment on lines
+300
to
+318
| if (total > account.balance) { | ||
| return { success: false, error: `余额不足。需要 $${total.toFixed(2)}，可用 $${account.balance.toFixed(2)}` }; | ||
| } | ||
|
|
||
| // Update balance | ||
| await PaperAccount.updateOne({ _id: accountId }, { $inc: { balance: -total } }); | ||
|
|
||
| // Record trade | ||
| await db.collection('papertrades').insertOne({ | ||
| accountId, | ||
| userId: userId || account.userId, | ||
| symbol: symbol.toUpperCase(), | ||
| company: company || symbol, | ||
| type: 'BUY', | ||
| shares, | ||
| price: roundedPrice, | ||
| total, | ||
| timestamp: new Date(), | ||
| }); |
Contributor
There was a problem hiding this comment.
Race condition: balance check and decrement are not atomic.
total > account.balance is evaluated against a snapshot read at Line 288, then the balance is decremented separately at Line 305. Two concurrent buys (very plausible with AI auto-trading firing trades) can both pass the check and overdraw the account into a negative balance. Make the debit conditional and atomic, and verify it applied before recording the trade.
🔒 Atomic conditional debit
- if (total > account.balance) {
- return { success: false, error: `余额不足。需要 $${total.toFixed(2)}，可用 $${account.balance.toFixed(2)}` };
- }
-
- // Update balance
- await PaperAccount.updateOne({ _id: accountId }, { $inc: { balance: -total } });
+ // Atomic check-and-debit to avoid overdraw under concurrency
+ const debit = await PaperAccount.updateOne(
+ { _id: accountId, balance: { $gte: total } },
+ { $inc: { balance: -total } }
+ );
+ if (debit.modifiedCount === 0) {
+ return { success: false, error: `余额不足。需要 $${total.toFixed(2)}，可用 $${account.balance.toFixed(2)}` };
+ }🤖 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 `@lib/actions/paper-trading.actions.ts` around lines 300 - 318, The balance
check and separate decrement are racy; replace the two-step check-then-update
with an atomic conditional update on PaperAccount (e.g., use updateOne or
findOneAndUpdate with filter {_id: accountId, balance: {$gte: total}} and $inc:
{balance: -total}) and verify the update result (modifiedCount or returned
value) succeeded before calling db.collection('papertrades').insertOne; if the
atomic update failed, return the insufficient-funds error; keep accountId,
total, roundedPrice, symbol and the trade insert logic intact but only run the
insert after the successful conditional update.
Comment on lines
+343
to
+382
| const holding = await db.collection('papertrades').aggregate([ | ||
| { $match: { accountId, symbol: symbol.toUpperCase() } }, | ||
| { | ||
| $group: { | ||
| _id: '$symbol', | ||
| shares: { $sum: { $cond: [{ $eq: ['$type', 'BUY'] }, '$shares', { $multiply: ['$shares', -1] }] } }, | ||
| company: { $first: '$company' }, | ||
| } | ||
| }, | ||
| ]).toArray(); | ||
|
|
||
| const held = holding[0]?.shares || 0; | ||
| if (held < shares) { | ||
| return { success: false, error: `持仓不足。持有 ${held} 股，尝试卖出 ${shares} 股` }; | ||
| } | ||
|
|
||
| // Get current price | ||
| const { getQuote } = await import('@/lib/actions/finnhub.actions'); | ||
| const quote = await getQuote(symbol); | ||
| const price = quote?.c || 0; | ||
| if (!price) return { success: false, error: `无法获取 ${symbol} 的实时价格` }; | ||
|
|
||
| const roundedPrice = Math.round(price * 100) / 100; | ||
| const total = Math.round(shares * roundedPrice * 100) / 100; | ||
|
|
||
| // Update balance | ||
| await PaperAccount.updateOne({ _id: accountId }, { $inc: { balance: total } }); | ||
|
|
||
| // Record trade | ||
| await db.collection('papertrades').insertOne({ | ||
| accountId, | ||
| userId: userId || account.userId, | ||
| symbol: symbol.toUpperCase(), | ||
| company: holding[0]?.company || symbol, | ||
| type: 'SELL', | ||
| shares, | ||
| price: roundedPrice, | ||
| total, | ||
| timestamp: new Date(), | ||
| }); |
Contributor
There was a problem hiding this comment.
Same TOCTOU exposure on sells — holdings check and trade insert are not atomic.
The held-shares aggregation (Lines 343-357) and the SELL insert (Line 372) are independent steps, so concurrent sells of the same symbol can both pass the held < shares guard and oversell the position, leaving net holdings negative. Once the shares validation above is in place, consider serializing per-account trades or re-checking net holdings inside a transaction so positions cannot go negative.
🤖 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 `@lib/actions/paper-trading.actions.ts` around lines 343 - 382, The sell flow
has a TOCTOU race: holdings are read via db.collection('papertrades').aggregate
and later a SELL insert (and PaperAccount.updateOne) happens outside an atomic
context; fix by doing the holdings check, balance update
(PaperAccount.updateOne) and trade insert
(db.collection('papertrades').insertOne) inside a single MongoDB
transaction/session so the holdings are re-read and validated within the same
transaction and concurrent sells cannot both pass. Concretely, start a client
session (session = client.startSession()), run session.withTransaction(async ()
=> { recompute holdings using
db.collection('papertrades').aggregate(...).toArray({ session }), if held <
shares throw; await PaperAccount.updateOne(..., { $inc: { balance: total } }, {
session }); await db.collection('papertrades').insertOne({...}, { session }) }),
and ensure all DB calls (aggregate, updateOne, insertOne) include the session
param and that you abort/handle errors from the transaction.
Comment on lines
+1
to
+6
| allowBuilds: | ||
| protobufjs: set this to true or false | ||
| sharp: set this to true or false | ||
| unrs-resolver: set this to true or false | ||
| nodeLinker: hoisted | ||
| onlyBuiltDependencies: protobufjs |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: inspect current YAML value types/shape.
python - <<'PY'
import yaml, pathlib
p = pathlib.Path("pnpm-workspace.yaml")
data = yaml.safe_load(p.read_text())
print("allowBuilds:", type(data.get("allowBuilds")).__name__, data.get("allowBuilds"))
print("onlyBuiltDependencies:", type(data.get("onlyBuiltDependencies")).__name__, data.get("onlyBuiltDependencies"))
PYRepository: Open-Dev-Society/OpenStock
Length of output: 254
🌐 Web query:
According to current pnpm documentation, what are the valid keys and value shapes for allowBuildsandonlyBuiltDependenciesinpnpm-workspace.yaml?
💡 Result:
In current pnpm documentation (v10.x/v11.x docs), pnpm-workspace.yaml supports: 1) allowBuilds Valid keys (how entries are written): - The keys are “package matchers” (package name patterns) such as esbuild or nx@21.6.4 || 21.6.5 and version-disjunction forms; you use matcher expressions as YAML map keys (example given: nx@21.6.4 || 21.6.5: true, and plain package names like core-js: false). [1][2] Valid value shape: - allowBuilds is a map: each matcher key maps to a boolean true (explicitly allow script execution) or false (explicitly disallow). [1] 2) onlyBuiltDependencies - The current pnpm documentation states that allowBuilds replaces onlyBuiltDependencies (and other related settings) and that those removed settings have been replaced by allowBuilds. Therefore onlyBuiltDependencies is no longer supported as a current/valid key in pnpm-workspace.yaml. [1] So, for “current pnpm documentation” validity: - Use allowBuilds with a YAML map from package matcher -> boolean. [1] - Do not use onlyBuiltDependencies; it has been replaced by allowBuilds in current docs. [1]
Citations:
Fix pnpm-workspace.yaml build policy schema (invalid allowBuilds values + unsupported onlyBuiltDependencies)
allowBuildscurrently uses placeholder strings ("set this to true or false") but pnpm expects a map of package matcher -> boolean (true/false).onlyBuiltDependenciesis no longer a supported workspace setting in current pnpm; remove it and encode those restrictions inallowBuilds.
🤖 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 `@pnpm-workspace.yaml` around lines 1 - 6, Replace the placeholder strings
under allowBuilds with boolean values and remove the unsupported
onlyBuiltDependencies setting: change allowBuilds to a map like "allowBuilds:\n
protobufjs: true\n sharp: false\n unrs-resolver: false" (or set true/false per
package policy you want) so each package matcher is mapped to a boolean, delete
the onlyBuiltDependencies line entirely, and keep nodeLinker: hoisted as-is;
update the allowBuilds entries for protobufjs, sharp and unrs-resolver
accordingly.
…ments P&L Calculation Fix - Fixed weighted average cost method in getPositions/getAccountOverview - Changed from totalCost/remainingShares to totalBuyCost/totalBuyShares * remainingShares - Fixes incorrect avgCost and P&L after partial sells US Market Hours System (lib/utils/market-hours.ts) - NYSE/NASDAQ regular hours: Mon-Fri 9:30AM-4PM ET - Auto DST detection (EDT/EST) - 2026 US holiday calendar (10 holidays) - getMarketStatus() returns open/closed/pre/post labels - All trades restricted: market orders auto-convert to MARKET_ON_OPEN when closed Pending Orders System - New MongoDB collection: pendingorders - Order types: LIMIT, STOP, MARKET_ON_OPEN - Balance/holdings validation before order creation - cron processes every 10min during market hours via auto-trading-monitor - UI: PendingOrdersList component with cancel support Dashboard UI Improvements - Buy form: Market/Limit/Stop toggle with inline price inputs - Sell panel slides out below the stock row (inline toggle) - Market status indicator in buy form and sell panel headers - Inline messages per operation section - Slide-down animation (animate-slideDown) AI Trading Fixes & Enhancements - Fixed bug: handleToggle/handleRun passed userId instead of accountId - Manual Run button disabled when market closed - Market Open/Closed indicator in header - Interval check: respects tradingIntervalMin from AI config - Manual Run passes force=true (bypasses interval check) Cron Updates - auto-trading-monitor: 4 fixed times -> */10 21-3 * * 1-5 - Pending order processing merged into auto-trading-monitor Cleanup - Removed 5 stale one-time scripts - Fixed process-pending-orders.js .env file path - Script outputs only when orders execute (watchdog pattern)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/paper-trading/AITradingPanel.tsx (1)
385-394:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent market-closed check on inner "Run AI Trade" button.
The header Run button (line 214) disables itself when the market is closed:
disabled={running || (marketStatus !== null && !marketStatus.isOpen)}But this expanded-panel button only checks
running, allowing users to trigger AI trading when the market is closed.Proposed fix
{config?.enabled && ( <button onClick={handleRun} - disabled={running} + disabled={running || (marketStatus !== null && !marketStatus.isOpen)} className="flex-1 bg-teal-600 hover:bg-teal-700 disabled:opacity-50 text-white py-2.5 rounded-lg text-sm font-medium transition-colors flex items-center justify-center gap-2" >🤖 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 `@components/paper-trading/AITradingPanel.tsx` around lines 385 - 394, The inner "Run AI Trade" button in AITradingPanel allows running when the market is closed because it only checks running; update its disabled condition to match the header button by combining running with the marketStatus check (use the same expression: running || (marketStatus !== null && !marketStatus.isOpen)) so the button that calls handleRun is disabled when the market is closed; locate the JSX for the expanded-panel button (the one rendering Play/Loader and using config?.enabled and onClick={handleRun}) and replace its disabled prop accordingly to ensure consistent behavior.
🧹 Nitpick comments (2)
lib/actions/pending-orders.actions.ts (1)
62-63: 💤 Low valueRedundant dynamic import of
connectToDatabase.
connectToDatabaseis already imported at the top of the file (line 3). This dynamic re-import is unnecessary.♻️ Remove redundant import
- const { connectToDatabase } = await import('`@/database/mongoose`'); - await connectToDatabase(); + // connectToDatabase already called via getPendingOrdersCollection()🤖 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 `@lib/actions/pending-orders.actions.ts` around lines 62 - 63, The code dynamically re-imports connectToDatabase even though it's already statically imported at the top of pending-orders.actions.ts; remove the redundant lines "const { connectToDatabase } = await import('`@/database/mongoose`');" and "await connectToDatabase();" and instead call the existing connectToDatabase() directly where needed (e.g., inside the function that currently performs the dynamic import) to avoid duplicate imports and unnecessary await/import overhead._scripts/process-pending-orders.js (1)
25-52: ⚖️ Poor tradeoffMarket hours logic is duplicated from
market-hours.ts.This script reimplements market hours checking with a simplified DST approximation (
month >= 3 && month <= 11) that differs from the main implementation. Changes to holiday lists or DST rules must be updated in both places.Consider importing the shared utility or extracting to a shared CommonJS module if this script needs to remain standalone.
🤖 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 `@_scripts/process-pending-orders.js` around lines 25 - 52, The isMarketOpen function duplicates market-hours logic and uses a simplified DST rule; replace this reimplementation by importing and using the centralized market hours utility (e.g., the exported function from market-hours.ts) or move the shared logic into a CommonJS module that both scripts require; update this file to call that shared function instead of the local isMarketOpen, remove the local holiday/DST code, and ensure the import/require references the exact exported symbol name from market-hours (or the new shared module) so DST rules and holiday lists remain single-source-of-truth.
🤖 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 `@_scripts/process-pending-orders.sh`:
- Around line 1-5: Replace the hardcoded absolute path used in the cd command in
_scripts/process-pending-orders.sh (currently "cd /Volumes/ks500/OpenStock")
with a portable lookup: compute the project root relative to the script location
(using the script's directory via dirname "$0") or fall back to an environment
variable like PROJECT_ROOT, then cd into that root before running exec node
_scripts/process-pending-orders.js so the script works across machines, CI and
production.
In `@app/globals.css`:
- Line 135: Keyframe name uses camelCase "slideDown" which violates kebab-case
linting; rename the keyframe to "slide-down" and update its reference in the CSS
variable --animate-slide-down (and the corresponding `@keyframes` declaration) so
both the animation variable and the `@keyframes` identifier match the new
kebab-case name (ensure you update any other occurrences of "slideDown" in the
file).
In `@components/paper-trading/PendingOrdersList.tsx`:
- Around line 36-42: The switch in function orderTypeLabel
(PendingOrdersList.tsx) can fall through and return undefined for unknown
PendingOrderType values; update orderTypeLabel to include a default case that
returns a safe descriptive string (e.g., `Unknown order type: ${o.orderType}` or
a localized "Unknown") so the function always returns a string and the Condition
column never renders empty, ensuring you reference the existing cases 'LIMIT',
'STOP', and 'MARKET_ON_OPEN' when adding the default.
In `@lib/actions/pending-orders.actions.ts`:
- Around line 275-292: The function processAllPendingOrders never increments the
failed counter; update the for-loop that calls attemptExecuteOrder(order._id) so
that when ok is false you increment failed, and also wrap the await
attemptExecuteOrder call in a try/catch so any thrown errors increment failed
and do not stop processing remaining orders; reference the failed variable, the
for (const order of orders) loop, and attemptExecuteOrder to locate where to add
the increment and error handling.
- Around line 195-269: attemptExecuteOrder suffers a TOCTOU race: it reads a
PENDING order then executes buyStock/sellStock allowing concurrent workers to
double-execute; replace the initial read+check with an atomic claim using
getPendingOrdersCollection().findOneAndUpdate({ _id: new
mongoose.Types.ObjectId(orderId), status: 'PENDING' }, { $set: { status:
'EXECUTING', claimedAt: new Date() } }, { returnDocument: 'after' }) and if that
returns null, return false; proceed using the returned document (use its _id) to
call buyStock/sellStock and then update to EXECUTED or FAILED (and include
failReason) — also ensure to revert or mark FAILED if execution throws,
referencing function attemptExecuteOrder and helpers buyStock/sellStock.
---
Outside diff comments:
In `@components/paper-trading/AITradingPanel.tsx`:
- Around line 385-394: The inner "Run AI Trade" button in AITradingPanel allows
running when the market is closed because it only checks running; update its
disabled condition to match the header button by combining running with the
marketStatus check (use the same expression: running || (marketStatus !== null
&& !marketStatus.isOpen)) so the button that calls handleRun is disabled when
the market is closed; locate the JSX for the expanded-panel button (the one
rendering Play/Loader and using config?.enabled and onClick={handleRun}) and
replace its disabled prop accordingly to ensure consistent behavior.
---
Nitpick comments:
In `@_scripts/process-pending-orders.js`:
- Around line 25-52: The isMarketOpen function duplicates market-hours logic and
uses a simplified DST rule; replace this reimplementation by importing and using
the centralized market hours utility (e.g., the exported function from
market-hours.ts) or move the shared logic into a CommonJS module that both
scripts require; update this file to call that shared function instead of the
local isMarketOpen, remove the local holiday/DST code, and ensure the
import/require references the exact exported symbol name from market-hours (or
the new shared module) so DST rules and holiday lists remain
single-source-of-truth.
In `@lib/actions/pending-orders.actions.ts`:
- Around line 62-63: The code dynamically re-imports connectToDatabase even
though it's already statically imported at the top of pending-orders.actions.ts;
remove the redundant lines "const { connectToDatabase } = await
import('`@/database/mongoose`');" and "await connectToDatabase();" and instead
call the existing connectToDatabase() directly where needed (e.g., inside the
function that currently performs the dynamic import) to avoid duplicate imports
and unnecessary await/import overhead.
🪄 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: 97a4c347-63d0-49d5-bded-86c661d41d9f
📒 Files selected for processing (11)
_scripts/check_positions.js_scripts/process-pending-orders.js_scripts/process-pending-orders.shapp/globals.csscomponents/paper-trading/AITradingPanel.tsxcomponents/paper-trading/PaperTradingDashboard.tsxcomponents/paper-trading/PendingOrdersList.tsxlib/actions/ai-trading.actions.tslib/actions/paper-trading.actions.tslib/actions/pending-orders.actions.tslib/utils/market-hours.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/actions/paper-trading.actions.ts
- Replace static 2026 holiday array with runtime calculation for any year - Gauss algorithm for Easter Sunday → Good Friday - Weekend observance rules (Sat→Fri, Sun→Mon) - Works automatically for 2026, 2027, and beyond - Also updated standalone process-pending-orders.js to match
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/market-hours.ts (1)
217-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWeekend
nextOpenmessage can be incorrect on Monday holidaysLine 218 always says
Monday 9:30 AM ET. That is wrong on long weekends where Monday is a market holiday (e.g., Presidents’ Day), where next open is Tuesday.Minimal safe fix
- return { isOpen: false, label: 'Market Closed — Weekend', nextOpen: 'Monday 9:30 AM ET' }; + return { isOpen: false, label: 'Market Closed — Weekend', nextOpen: 'Next trading day 9:30 AM ET' };🤖 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 `@lib/utils/market-hours.ts` around lines 217 - 219, The weekend branch currently returns a hard-coded 'Monday 9:30 AM ET' which fails on Monday market holidays; update the branch in lib/utils/market-hours.ts (where et.dayOfWeek is checked and the returned object includes nextOpen) to compute the next trading day instead of hard-coding Monday: iterate from the following day until a date that is not a weekend and not a market holiday (use the existing holiday check helper, e.g., isMarketHoliday or the holidays list), then format and return that weekday name with "9:30 AM ET" as nextOpen (keep isOpen: false and the label).
🤖 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 `@lib/utils/market-hours.ts`:
- Around line 17-23: nthWeekday is incorrectly adding an extra week when the
month's 1st is the target weekday because of the conditional "if (offset === 0
&& n > 1) offset = 7"; remove that conditional and let offset = (weekday - day +
7) % 7 stand as-is, keep the date calculation date = 1 + offset + (n - 1) * 7,
and optionally validate that n >= 1 to avoid invalid inputs; update the function
nthWeekday to eliminate the special-case offset adjustment so nth occurrences
are computed correctly.
---
Outside diff comments:
In `@lib/utils/market-hours.ts`:
- Around line 217-219: The weekend branch currently returns a hard-coded 'Monday
9:30 AM ET' which fails on Monday market holidays; update the branch in
lib/utils/market-hours.ts (where et.dayOfWeek is checked and the returned object
includes nextOpen) to compute the next trading day instead of hard-coding
Monday: iterate from the following day until a date that is not a weekend and
not a market holiday (use the existing holiday check helper, e.g.,
isMarketHoliday or the holidays list), then format and return that weekday name
with "9:30 AM ET" as nextOpen (keep isOpen: false and the label).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- fix nthWeekday bug when 1st of month matches target weekday - add 30s timeout to LLM API call in runAITradeCycle - fix failed counter not being incremented in processAllPendingOrders - add TOCTOU protection to attemptExecuteOrder (atomic claim via findOneAndUpdate) - add default case to PendingOrdersList order type switch - rename slideDown keyframe to kebab-case slide-down
- AccountSwitcher: stale React state after account deletion (used fresh API call instead of filtering stale accounts state) - finnhub.actions fetchJSON: cache write failure no longer discards successful data (wrapped in non-fatal try/catch) - finnhub.actions getQuote: token variable moved out of try block so stale cache fallback in catch block actually works (was ReferenceError) - paper-trading.actions buyStock/sellStock: added share count validation (reject non-integer, 0, or negative shares)
There was a problem hiding this comment.
Pull request overview
This PR introduces a full paper-trading feature set (accounts, simulated trades, pending orders, and an AI-assisted auto-trading panel), plus supporting utilities (market-hours detection, Finnhub fetching/caching) and a new navigation entry.
Changes:
- Added paper trading dashboard UI with multi-account support, manual trading, pending orders, and history/positions views.
- Implemented server-side paper trading actions + MongoDB models (accounts, trades, AI config) and a pending-order execution engine.
- Added market-hours utilities and enhanced Finnhub fetching with timeout/proxy support and a local file cache.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds pnpm workspace config (currently invalid/misplaced content). |
| package.json | Adds undici dependency for proxy-capable fetch usage. |
| lib/utils/market-hours.ts | Introduces US market-hours/holiday helpers used by trading logic/UI. |
| lib/constants.ts | Adds “Paper Trading” to navigation items. |
| lib/actions/pending-orders.actions.ts | Implements pending orders CRUD + execution processing (contains critical execution-state bugs). |
| lib/actions/paper-trading.actions.ts | Implements paper accounts, simulated buy/sell, overview, positions, history, reset/period logic. |
| lib/actions/finnhub.actions.ts | Adds file-based caching + timeout/proxy support for Finnhub calls. |
| lib/actions/ai-trading.actions.ts | Implements AI config CRUD and a trade-cycle runner that calls an LLM + executes simulated trades. |
| database/models/paper-trading.model.ts | Adds Mongoose schemas for paper accounts, trades, and AI trading config. |
| components/paper-trading/PendingOrdersList.tsx | UI to display/cancel pending orders. |
| components/paper-trading/PaperTradingDashboard.tsx | Main paper trading dashboard UI (buy/sell forms, holdings, pending orders, history, setup/reset). |
| components/paper-trading/AITradingPanel.tsx | UI for configuring and manually running AI trading. |
| components/paper-trading/AccountSwitcher.tsx | UI for switching/creating/deleting paper accounts. |
| app/globals.css | Adds slide-down keyframes / animation variable for UI transitions. |
| app/(root)/paper-trading/page.tsx | Adds the paper trading page and loads/creates the active account. |
| .npmrc | Configures onlyBuiltDependencies list. |
| .gitignore | Ignores /_scripts/ helper scripts directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+6
| allowBuilds: | ||
| protobufjs: set this to true or false | ||
| sharp: set this to true or false | ||
| unrs-resolver: set this to true or false | ||
| nodeLinker: hoisted | ||
| onlyBuiltDependencies: protobufjs |
Comment on lines
+199
to
+206
| // Atomically claim the order (prevents TOCTOU race when two processes try to execute the same order) | ||
| const order = await coll.findOneAndUpdate( | ||
| { _id: new mongoose.Types.ObjectId(orderId), status: 'PENDING' }, | ||
| { $set: { status: 'PROCESSING', processingStartedAt: new Date() } }, | ||
| { returnDocument: 'before' } | ||
| ); | ||
| if (!order) return false; // Already claimed or doesn't exist | ||
|
|
Comment on lines
+207
to
+214
| // Market must be open to execute | ||
| if (!isMarketOpen()) return false; | ||
|
|
||
| // Get current price | ||
| const { getQuote } = await import('@/lib/actions/finnhub.actions'); | ||
| const quote = await getQuote(order.symbol); | ||
| const currentPrice = quote?.c || 0; | ||
| if (!currentPrice) return false; |
| break; | ||
| } | ||
|
|
||
| if (!shouldExecute) return false; |
Comment on lines
+58
to
+66
| const coll = await getPendingOrdersCollection(); | ||
| const sym = symbol.toUpperCase(); | ||
|
|
||
| // ── Balance / Holdings Check ── | ||
| const { connectToDatabase } = await import('@/database/mongoose'); | ||
| await connectToDatabase(); | ||
| const db = mongoose.connection.db; | ||
| if (!db) return { success: false, error: 'Database connection failed' }; | ||
|
|
Comment on lines
+220
to
+224
| const proxyUrl = process.env.HTTPS_PROXY || 'http://127.0.0.1:7890'; | ||
| const { ProxyAgent } = await import('undici'); | ||
| const undici = await import('undici'); | ||
|
|
||
| const dispatcher = new ProxyAgent(proxyUrl); |
- Add scheduled AI trading execution through checkAITrading - Support API key configuration and connection testing - Enable automatic activation after successful API validation - Add DeepSeek reasoning model compatibility - Support proxy configuration for outbound network access - Add symbol-based filtering to trade history - Replace manual execution with background automation - Enhance error handling and user notifications
Author
|
Hi! It seems the Vercel check is blocked waiting for authorization. Could you please help review and trigger the deployment check? Thanks |
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
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.
Added a complete paper trading system, including account management and trade execution functionalities. Introduced multiple helper scripts for portfolio analysis, stock research, and automated trade execution. Configured dependency build options to optimize performance.
Summary by CodeRabbit
New Features
Style
Chores