Nijil / GRWT-5269 / Integrate product_id to product-config #70
Conversation
Reviewer's Guide by SourceryThis pull request focuses on integrating the Sequence diagram for fetching product configurationsequenceDiagram
participant Component
participant useProductConfig
participant getProductConfig
participant API
Component->>useProductConfig: fetchProductConfig(product_id, instrument_id)
useProductConfig->>getProductConfig: await getProductConfig(instrument_id, product_id, account_uuid)
getProductConfig->>API: GET /v1/market/products/config?instrument_id&product_id&account_uuid
API-->>getProductConfig: ProductConfigResponse
getProductConfig-->>useProductConfig: ProductConfigResponse
useProductConfig-->>Component: Update UI with config
Updated class diagram for ProductConfigRequestclassDiagram
class ProductConfigRequest {
-instrument_id: string
-product_id: string
-account_uuid: string
}
note for ProductConfigRequest "Added product_id and account_uuid attributes"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nijil-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- The getProductConfig function in
src/services/api/rest/product-config/service.tsnow requiresproduct_idandaccount_uuid- ensure all call sites are updated accordingly.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| ); | ||
| return response.data; | ||
| const { instrument_id, product_id } = params; |
There was a problem hiding this comment.
suggestion (bug_risk): Pass a proper account identifier instead of an empty string.
Currently, account_uuid is hardcoded as an empty string. Make sure to replace it with the actual account identifier when available to avoid unexpected API behavior.
Suggested implementation:
params: ProductConfigRequest, accountUUID: string
): Promise<ProductConfigResponse> => { account_uuid: accountUUID,Make sure to update all the callers of this function so that you pass the proper account identifier.
| isInitialRender.current = true; | ||
| return () => { | ||
| isInitialRender.current = false; | ||
| export const DurationController: React.FC<DurationControllerProps> = ({ onClose }) => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting state management into a custom hook and splitting UI rendering into sub-components to improve readability and maintainability of the component.
Consider extracting parts of the local state and view logic into dedicated hooks or sub-components. This can reduce nested callbacks and improve readability without changing functionality. For example:
-
Extract duration state management into a custom hook:
Move all duration-related state and handlers into a hook (e.g.,useDurationController) that returns necessary values and methods.// hooks/useDurationController.ts import { useState, useMemo } from "react"; import { getDefaultDuration } from "@/utils/duration"; export function useDurationController(initialDuration: string) { const [localDuration, setLocalDuration] = useState(initialDuration); const [value, type] = localDuration.split(" "); const [selectedTabType, setSelectedTabType] = useState(type); const [selectedValues, setSelectedValues] = useState({ [type]: type === "hours" ? value : parseInt(value, 10), }); const selectedValue = selectedValues[selectedTabType]; const updateSelectedValue = (newVal: number | string) => { setSelectedValues(prev => ({ ...prev, [selectedTabType]: newVal })); const newDuration = `${newVal} ${selectedTabType}`; setLocalDuration(newDuration); return newDuration; }; return { localDuration, selectedTabType, selectedValue, setSelectedTabType, updateSelectedValue, setLocalDuration, selectedValues, }; }
-
Split UI rendering into sub-components:
Separate mobile and landscape views or even the tab list from the duration value list. For example:// components/DurationContent.tsx import React from "react"; import { TabList } from "@/components/ui/tab-list"; import { BottomSheetHeader } from "@/components/ui/bottom-sheet-header"; import { HoursDurationValue } from "./HoursDurationValue"; import { DurationValueList } from "./DurationValueList"; export const DurationContent: React.FC<{ isLandscape: boolean; availableDurationTypes: any; selectedTabType: string; selectedValue: string | number | undefined; onSelectTab: (type: string) => void; onValueSelect: (value: number | string) => void; onValueClick: (value: number | string) => void; }> = ({ isLandscape, availableDurationTypes, selectedTabType, selectedValue, onSelectTab, onValueSelect, onValueClick, }) => ( <div className={isLandscape ? "flex" : ""}> {!isLandscape && <BottomSheetHeader title="Duration" />} <TabList tabs={availableDurationTypes} selectedValue={selectedTabType} onSelect={onSelectTab} variant={isLandscape ? "vertical" : "chip"} /> <div className={`flex-1 relative bg-white ${isLandscape ? "px-2" : "px-8"}`}> {selectedTabType === "hours" ? ( <HoursDurationValue selectedValue={selectedValue?.toString() || ""} onValueSelect={onValueSelect} onValueClick={onValueClick} /> ) : ( <DurationValueList key={selectedTabType} selectedValue={selectedValue} durationType={selectedTabType} onValueSelect={onValueSelect} onValueClick={onValueClick} /> )} </div> </div> );
-
Integrate refactored hooks and sub-components into your main component:
// DurationController.tsx import React, { useEffect, useRef, useMemo } from "react"; import { DesktopTradeFieldCard } from "@/components/ui/desktop-trade-field-card"; import { PrimaryButton } from "@/components/ui/primary-button"; import { useTradeStore } from "@/stores/tradeStore"; import { useBottomSheetStore } from "@/stores/bottomSheetStore"; import { useOrientationStore } from "@/stores/orientationStore"; import { useDebounce } from "@/hooks/useDebounce"; import { getAvailableDurationTypes } from "@/adapters/duration-config-adapter"; import { DurationContent } from "./components/DurationContent"; import { useDurationController } from "./hooks/useDurationController"; export const DurationController: React.FC<{ onClose?: () => void }> = ({ onClose }) => { const { productConfig: config, duration, setDuration } = useTradeStore(); const { isLandscape } = useOrientationStore(); const { setBottomSheet } = useBottomSheetStore(); const isInitialRender = useRef(true); useEffect(() => { isInitialRender.current = true; return () => { isInitialRender.current = false; }; }, []); const availableDurationTypes = useMemo( () => getAvailableDurationTypes(config, /* your DURATION_TYPES constant */), [config] ); const { localDuration, selectedTabType, selectedValue, setSelectedTabType, updateSelectedValue, } = useDurationController(duration); useDebounce(localDuration, (value) => { if (isLandscape) setDuration(value); }, 300); const handleValueSelect = (value: number | string) => { updateSelectedValue(value); }; const handleValueClick = (value: number | string) => { const newDuration = updateSelectedValue(value); setDuration(newDuration); if (isLandscape) onClose?.(); }; const handleSave = () => { setDuration(localDuration); if (isLandscape) onClose?.(); else setBottomSheet(false); }; const content = ( <> <DurationContent isLandscape={isLandscape} availableDurationTypes={availableDurationTypes} selectedTabType={selectedTabType} selectedValue={selectedValue} onSelectTab={setSelectedTabType} onValueSelect={handleValueSelect} onValueClick={handleValueClick} /> {!isLandscape && ( <div className="w-full p-3"> <PrimaryButton className="rounded-3xl" onClick={handleSave}> Save </PrimaryButton> </div> )} </> ); return isLandscape ? ( <DesktopTradeFieldCard className="p-0"> <div className="w-[368px]">{content}</div> </DesktopTradeFieldCard> ) : ( <div className="flex flex-col h-full">{content}</div> ); };
These steps help isolate state logic and UI rendering, reducing component complexity and making the code easier to maintain.
| }; | ||
| const handleHoursSelect = (newHours: number) => { | ||
| lastValidHours.current = newHours; | ||
| if (isInitialRender.current) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting common helper functions for hours and minutes updates to reduce code duplication and improve readability, differentiating select vs. click in one place.
You’re handling very similar logic twice for hours (and likewise for minutes). Consider extracting a common helper for hours updates so you only handle the differences (select vs. click) in one place.
For example, you can refactor the hours handlers as follows:
const updateHours = (
newHours: number,
callback: (value: string) => void
) => {
lastValidHours.current = newHours;
if (!isInitialRender.current) {
lastValidMinutes.current = 0;
callback(`${newHours}:00`);
} else {
callback(`${newHours}:${lastValidMinutes.current}`);
}
scrollToZeroMinutes();
};
const handleHoursSelect = (newHours: number) =>
updateHours(newHours, onValueSelect);
const handleHoursClick = (newHours: number) =>
onValueClick && updateHours(newHours, onValueClick);Similarly, if the minutes handlers are similar, you could extract a helper as well:
const updateMinutes = (
newMinutes: number,
callback: (value: string) => void
) => {
lastValidMinutes.current = newMinutes;
callback(`${lastValidHours.current}:${newMinutes}`);
};
const handleMinutesSelect = (newMinutes: number) =>
updateMinutes(newMinutes, onValueSelect);
const handleMinutesClick = (newMinutes: number) =>
onValueClick && updateMinutes(newMinutes, onValueClick);This reduces duplication while keeping functionality intact.
| const finalResult: DurationRangesResponse = {} as DurationRangesResponse; | ||
|
|
||
| // Add each duration type only if it has a valid range | ||
| if (result.ticks) finalResult.ticks = result.ticks; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (result.ticks) finalResult.ticks = result.ticks; | |
| if (result.ticks) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
|
||
| // Add each duration type only if it has a valid range | ||
| if (result.ticks) finalResult.ticks = result.ticks; | ||
| if (result.seconds) finalResult.seconds = result.seconds; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (result.seconds) finalResult.seconds = result.seconds; | |
| if (result.seconds) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| }, 100); | ||
| useEffect(() => { | ||
| const container = containerRef.current; | ||
| if (!container) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!container) return; | |
| if (!container) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| const range = durationConfig.ranges[type]; | ||
| if (!range) return []; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!range) return []; | |
| if (!range) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| @@ -141,8 +131,8 @@ export function generateDurationValues( | |||
| * @returns Special case key (e.g., '24h') or empty string | |||
| */ | |||
| export function getSpecialCaseKey(hour?: number): string { | |||
| if (hour === undefined) return ""; | |||
| return SPECIAL_HOUR_CASES[hour]?.key || ""; | |||
| if (hour === undefined) return ""; | |||
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (hour === undefined) return ""; | |
| if (hour === undefined) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| return value >= range.min && value <= range.max; | ||
| export function isValidDuration(type: keyof DurationRangesResponse, value: number): boolean { | ||
| const range = durationConfig.ranges[type]; | ||
| if (!range) return false; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!range) return false; | |
| if (!range) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| if (!range) return 0; | ||
| return type === "minutes" ? 1 : range.min; | ||
| const range = durationConfig.ranges[type]; | ||
| if (!range) return 0; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!range) return 0; | |
| if (!range) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've reviewed the changes that replace product_type with product_id in the product configuration API. While the implementation is mostly complete, there are critical issues that need to be addressed before merging:
- TradeFormController.tsx still uses
trade_typeinstead ofproduct_idwhen calling fetchProductConfig, which will cause runtime errors - The
account_uuidparameter is required in the API but passed as an empty string - it should use the client store's selectedAccountId - Make sure all components using this hook have been updated to use the new parameter names
Please address these issues to ensure a smooth transition to the new API parameters.
Feel free to provide me with any instructions or feedback related to this review or other repository tasks, which I'll take into account for future work.
| const config = await getProductConfig({ | ||
| instrument_id, | ||
| product_id, | ||
| account_uuid: "", // TODO - get the actual account_uuid |
There was a problem hiding this comment.
The account_uuid parameter is currently passed as an empty string with a TODO comment. You should use the selectedAccountId from the client store instead:
| account_uuid: "", // TODO - get the actual account_uuid | |
| account_uuid: useClientStore.getState().selectedAccountId, // Get account ID from client store |
| setConfigError(null); | ||
|
|
||
| // Create a cache key | ||
| const cacheKey = `${product_id}_${instrument_id}`; |
There was a problem hiding this comment.
The cache key is now created using product_id instead of product_type. This is correct, but there's an issue in TradeFormController.tsx (line 74) which still calls this function with trade_type instead of product_id. This will cause runtime errors when the PR is merged.
| params: { | ||
| instrument_id, | ||
| product_id, | ||
| account_uuid: "", |
There was a problem hiding this comment.
The account_uuid parameter is being passed as an empty string. You should use useClientStore.getState().selectedAccountId to get the actual account UUID from the client store.
| const defaultStake = adaptDefaultStake(config); | ||
| setStake(defaultStake); | ||
| setAllowEquals(config.data.defaults.allow_equals); | ||
| }, |
There was a problem hiding this comment.
The function signature has been updated to use product_id instead of product_type, but TradeFormController.tsx (line 74) still calls this function with trade_type instead of product_id. This will cause runtime errors when the PR is merged.
| instrument_id: string; | ||
| instrument_id: string; | ||
| product_id: string; | ||
| account_uuid: string; |
There was a problem hiding this comment.
The account_uuid parameter has been added as required in the ProductConfigRequest interface, but it's being passed as an empty string in the implementation. You should use useClientStore.getState().selectedAccountId to populate this value.
|
|
||
| // Create a cache key | ||
| const cacheKey = `${product_id}_${instrument_id}`; | ||
|
|
There was a problem hiding this comment.
This PR introduces a breaking change by changing product_type to product_id in the API interface. You need to update TradeFormController.tsx which still uses trade_type instead of product_id when calling this function. Also verify that the tradeTypeConfigs object keys (like 'rise_fall') match valid product_id values.
Summary by Sourcery
Integrate product_id to product-config API calls, and refactor duration selection.
Enhancements: