Derive the %Diff reference price from a less noisy metric #168
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
error next-auth@4.18.7: The engine "node" is incompatible with this module. Expected version "^12.19.0 || ^14.15.0 || ^16.13.0 || ^18.12.0". Got "22.9.0" WalkthroughThe changes in this pull request involve updates to several components within the market module, enhancing type safety and simplifying code structure. Key modifications include the introduction of a utility function, Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
components/Market/utils.ts (1)
60-62: Remove the unused 'isDistinctMedianIndex' functionAfter correcting the median calculation, the
isDistinctMedianIndexfunction is no longer needed.Apply this diff to remove the unused function:
-function isDistinctMedianIndex(idx: number): boolean { - return Math.trunc(idx) === idx; -}components/Market/MarketDataCenter/MarketDataCenter.tsx (1)
38-70: Consider refactoring duplicate average calculationsThe code for calculating averages of listings and sales is repetitive. Extracting this logic into a helper function can improve maintainability.
Example:
function calculateAverage(values: number[]): number { return Math.ceil(values.reduce((sum, val) => sum + val, 0) / values.length) || 0; } // Usage: const hqListingsAveragePpu = calculateAverage(hqListings.map((listing) => listing.pricePerUnit));components/Market/MarketRegion/MarketRegion.tsx (1)
57-89: Refactor repetitive average calculationsThe average calculations for listings and sales are duplicated. Consider creating a utility function to reduce redundancy.
Example:
function calculateAverage(values: number[]): number { return Math.ceil(values.reduce((sum, val) => sum + val, 0) / values.length) || 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
components/Market/MarketAverages/MarketAverages.tsx(1 hunks)components/Market/MarketDataCenter/MarketDataCenter.tsx(6 hunks)components/Market/MarketRegion/MarketRegion.tsx(6 hunks)components/Market/MarketWorld/MarketWorld.tsx(4 hunks)components/Market/utils.ts(1 hunks)types/universalis/MarketV2.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/Market/MarketAverages/MarketAverages.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/Market/utils.ts
[error] 3-6: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
🔇 Additional comments (12)
types/universalis/MarketV2.ts (1)
35-41: New properties in 'MarketV2' interface are appropriate
The additional properties enhance the data structure with detailed pricing and histogram information. The types are correctly defined.
components/Market/MarketWorld/MarketWorld.tsx (3)
14-15: Type updates and imports improve type safety
Updating the market property to type MarketV2 and importing calculateReferencePrice and Quality enhances type safety and consistency across the codebase.
Also applies to: 20-20
38-39: Correct implementation of reference price calculations
Calculating nqReferencePrice and hqReferencePrice using calculateReferencePrice ensures consistent pricing logic.
55-56: Passing reference prices to tables is appropriate
Using the calculated reference prices in ListingsTable and SalesTable maintains consistency in displaying pricing information.
Also applies to: 70-71
components/Market/MarketDataCenter/MarketDataCenter.tsx (4)
15-16: Updated types and imports enhance code quality
Changing the market type to MarketV2 and importing calculateReferencePrice and Quality improves type safety and aligns with the overall architecture.
Also applies to: 21-21
31-34: Filtering listings and sales logically
The filtering of listings and sales into HQ and NQ categories is clear and efficient.
73-75: Proper use of reference price calculations
Calculating nqReferencePrice and hqReferencePrice enhances the pricing logic with recent sales data.
94-95: Consistent usage of reference prices in tables
Passing the reference prices to ListingsTable and SalesTable ensures accurate %Diff calculations.
Also applies to: 108-109, 126-127, 139-140
components/Market/MarketRegion/MarketRegion.tsx (4)
15-16: Type enhancements improve robustness
Updating dcMarkets to type Record<string, MarketV2> and importing necessary utilities strengthens type safety.
Also applies to: 22-22
50-53: Effective filtering of listings and sales
Separating HQ and NQ listings and sales is efficiently handled.
92-94: Correct implementation of reference price calculations
Using calculateReferencePrice for nqReferencePrice and hqReferencePrice correctly integrates the new pricing logic.
113-114: Consistent application of reference prices
Passing the calculated reference prices to the tables ensures accurate and consistent data display across the application.
Also applies to: 129-130, 149-150, 164-165
Changes the %Diff reference price to use the median unit price of the last 7 days of sales. This will make it based on the actual selling price and less vulnerable to high-priced listings shifting the value to an unrealistic amount. This aims to make the %Diff more useful for sellers and purchasers by communicating something about what the useful selling prices are, rather than whatever the current arbitrary listing prices are.
karashiiro
force-pushed
the
feat/new-reference-prices
branch
from
8d1b9f6 to
88efd73
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/Market/utils.ts (1)
67-73: Consider adding a constant for milliseconds conversionWhile the implementation is correct, readability could be improved by extracting the milliseconds conversion factor.
const SECONDS_PER_DAY = 86400; +const MILLISECONDS_PER_SECOND = 1000; function withinDays(days: number) { const maxTimeDiff = days * SECONDS_PER_DAY; - const now = Date.now() / 1000; + const now = Date.now() / MILLISECONDS_PER_SECOND; return function (sale: Sale): boolean { return now - sale.timestamp <= maxTimeDiff; }; }components/Market/MarketRegion/MarketRegion.tsx (1)
92-93: Consider consolidating average price calculationsThe component now has two sets of average calculations:
- Reference prices using the new median-based calculation
- Arithmetic means for listings and sales
Consider using the reference prices consistently throughout the component to reduce complexity and potential confusion.
Also applies to: 113-114, 129-130, 149-150, 164-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
components/Market/MarketAverages/MarketAverages.tsx(1 hunks)components/Market/MarketDataCenter/MarketDataCenter.tsx(6 hunks)components/Market/MarketRegion/MarketRegion.tsx(6 hunks)components/Market/MarketWorld/MarketWorld.tsx(4 hunks)components/Market/utils.ts(1 hunks)types/universalis/MarketV2.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/Market/MarketAverages/MarketAverages.tsx
- types/universalis/MarketV2.ts
- components/Market/MarketDataCenter/MarketDataCenter.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/Market/utils.ts
[error] 3-6: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
🔇 Additional comments (7)
components/Market/utils.ts (5)
8-28: LGTM! Well-structured implementation of the new reference price calculation
The function effectively:
- Uses median of last 7 days of sales as requested in issue #1394
- Handles both single market and multiple markets through overloads
- Properly handles empty sales case
30-47: LGTM! Type-safe implementation of sales concatenation
The function effectively handles both single and multiple markets with proper type narrowing and null safety.
49-59: LGTM! Correct implementation of median calculation
The function correctly handles both odd and even-length arrays while maintaining immutability.
75-83: LGTM! Clean implementation of quality filter
The function follows functional programming patterns and has clear, simple logic.
3-6:
Convert const enum to regular enum for better compatibility
Using const enum can cause issues with bundlers and is incompatible with the 'isolatedModules' mode.
Apply this diff to fix the issue:
-export const enum Quality {
+export enum Quality {
NormalQuality,
HighQuality,
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 3-6: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
components/Market/MarketWorld/MarketWorld.tsx (1)
14-15: LGTM! Proper integration of new reference price calculation
The changes correctly:
- Improve type safety by using MarketV2 type
- Calculate reference prices for both quality types
- Update table components with the new reference prices
Also applies to: 20-20, 38-39, 55-56, 70-71
components/Market/MarketRegion/MarketRegion.tsx (1)
15-16: LGTM! Improved type safety with MarketV2
The changes properly type the market data structures.
Also applies to: 22-22
Changes the %Diff reference price to use the median unit price of the last 7 days of sales. This will make it based on the actual selling price and less vulnerable to high-priced listings shifting the value to an unrealistic amount.
This aims to make the %Diff more useful for sellers and purchasers by communicating something about what the useful selling prices are, rather than whatever the current arbitrary listing prices are.
Fixes Universalis-FFXIV/Universalis#1394.
Summary by CodeRabbit
Release Notes
New Features
Qualityenum for better categorization of market data.Bug Fixes
Documentation
MarketAveragescomponent.