-
Notifications
You must be signed in to change notification settings - Fork 0
fix: chart interval, short tp/sl labels, position count #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request addresses three distinct issues: updating chart time intervals, implementing proper TP/SL label display for short positions, and adding a position count display to the home screen tab.
Changes:
- Updated chart intervals from 1/3/5/15/30 minutes to 15m/30m/1h/1d/1w for better trading timeframes
- Implemented inverted sign logic for TP/SL labels on short positions (e.g., TP shows "-25%" for shorts vs "+25%" for longs)
- Added position count display in the Positions tab label that dynamically updates based on open positions
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/modules/PositionDetails/Overview/chart.tsx | Updated chart intervals to 15m/30m/1h/1d/1w timeframes |
| src/components/molecules/tp-sl-dialog.tsx | Added side parameter and implemented inverted TP/SL calculations and label signs for short positions using Match pattern |
| tests/components/tp-sl-dialog.test.tsx | Added comprehensive test coverage for short position TP/SL calculations and label rendering |
| src/components/modules/Order/Overview/utils.ts | Updated formatTPOrSLSettings to accept side parameter and invert signs for short positions |
| src/components/modules/Order/Overview/index.tsx | Passed side parameter to formatTPOrSLSettings function |
| src/components/modules/PositionDetails/Overview/Position/index.tsx | Passed position.side to TPOrSLDialog and getTPOrSLConfigurationFromPosition calls |
| src/components/modules/Home/index.tsx | Added PositionsTabLabel component that fetches and displays position count |
| package.json | Added @tanstack/router-cli dev dependency and generate-routes script |
| pnpm-lock.yaml | Updated lock file with new router-cli dependencies and router-core version bump |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/components/modules/Order/Overview/utils.ts:25
- The
formatTPOrSLSettingsfunction has been updated to accept asideparameter that changes the sign of the percentage labels (e.g., "TP +25%" vs "TP -25%" for short positions). However, there are no tests for this utility function. Given that other similar utility functions in the codebase have comprehensive test coverage (e.g., tp-sl-dialog.test.tsx, leverage-dialog.test.tsx), it would be beneficial to add tests that verify the correct formatting for both long and short positions, especially the sign inversion logic.
export function formatTPOrSLSettings(
settings: TPOrSLSettings,
side: "long" | "short" = "long",
): string {
const tp = Option.fromNullable(settings.takeProfit.percentage).pipe(
Option.filter((percentage) => percentage !== 0),
Option.map((percentage) =>
side === "short" ? `TP -${percentage}%` : `TP +${percentage}%`,
),
Option.getOrElse(() => "TP Off"),
);
const sl = Option.fromNullable(settings.stopLoss.percentage).pipe(
Option.filter((percentage) => percentage !== 0),
Option.map((percentage) =>
side === "short" ? `SL +${percentage}%` : `SL -${percentage}%`,
),
Option.getOrElse(() => "SL Off"),
);
return `${tp}, ${sl}`;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes: ENG-1211, ENG-1210, ENG-1212