Improve humanize time accuracy #3976
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe pull request introduces a new utility function, Changes
Possibly related PRs
Suggested reviewers
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
packages/web/components/nomic/nomic-pending-transfers.tsxOops! Something went wrong! :( ESLint: 8.50.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 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: 1
🧹 Outside diff range and nitpick comments (6)
packages/web/utils/__tests__/date.spec.ts (2)
56-61: Add more edge cases for hours and minutes combinations.While the current test case for "2 hours and 30 minutes" is good, consider adding these important edge cases:
- 1 hour 1 minute (testing singular forms)
- 23 hours 59 minutes (testing near-day boundary)
- 2 hours 0 minutes (testing zero minutes handling)
{ name: "should return hours and minutes for less than a day", input: 2 * 60 * 60 + 30 * 60, // 2 hours and 30 minutes from now expected: [ { value: 2, unit: "hours" }, { value: 30, unit: "minutes" }, ], }, + { + name: "should handle singular forms correctly", + input: 1 * 60 * 60 + 1 * 60, // 1 hour and 1 minute from now + expected: [ + { value: 1, unit: "hour" }, + { value: 1, unit: "minute" }, + ], + }, + { + name: "should handle near-day boundary", + input: 23 * 60 * 60 + 59 * 60, // 23 hours and 59 minutes from now + expected: [ + { value: 23, unit: "hours" }, + { value: 59, unit: "minutes" }, + ], + }, + { + name: "should handle zero minutes", + input: 2 * 60 * 60, // 2 hours exactly + expected: [ + { value: 2, unit: "hours" }, + ], + },
70-73: Consider using a more meaningful duration for the formatted date test.The choice of 32 days seems arbitrary. Consider using a more meaningful duration that clearly demonstrates the transition point where we switch to date formatting (e.g., exactly 7 days or 30 days).
{ name: "should return formatted date for more than a day", - input: 32 * 24 * 60 * 60, // 32 days from now + input: 7 * 24 * 60 * 60, // 7 days from now (weekly boundary) expected: [ { value: dayjs().add(32, "days").format("MMM D, YYYY"), unit: "" }, ], },packages/web/utils/date.ts (2)
76-88: Consider including remaining hours for day calculationsFollowing the same principle of improved accuracy, consider including remaining hours when displaying days (e.g., "2 days and 23 hours") in a future enhancement.
95-106: Add input validation for empty humanizedTime arrayThe implementation is clean and provides good flexibility with the delimiter option. Consider adding validation for empty arrays to prevent potential issues.
export function displayHumanizedTime({ humanizedTime, t, delimitedBy = " and ", }: { humanizedTime: ReturnType<typeof humanizeTime>; t: (key: string) => string; delimitedBy?: string; }) { + if (!humanizedTime.length) { + return ""; + } return humanizedTime .map((time) => `${time.value} ${t(time.unitTranslationKey)}`) .join(delimitedBy); }packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx (1)
Line range hint
332-367: Consider refactoring session period handling for better maintainabilityThe switch statement for session periods could be simplified using a mapping object or utility function.
Consider this refactor:
- switch (transaction1CTParams.sessionPeriod.end) { - case "5min": - sessionPeriod = { - end: unixSecondsToNanoSeconds(dayjs().add(5, "minute").unix()), - }; - break; - case "10min": - sessionPeriod = { - end: unixSecondsToNanoSeconds(dayjs().add(10, "minute").unix()), - }; - break; - case "30min": - sessionPeriod = { - end: unixSecondsToNanoSeconds(dayjs().add(30, "minute").unix()), - }; - break; - case "1hour": - sessionPeriod = { - end: unixSecondsToNanoSeconds(dayjs().add(1, "hour").unix()), - }; - break; - case "3hours": - sessionPeriod = { - end: unixSecondsToNanoSeconds(dayjs().add(3, "hours").unix()), - }; - break; - case "12hours": - sessionPeriod = { - end: unixSecondsToNanoSeconds(dayjs().add(12, "hours").unix()), - }; - break; - default: - throw new Error( - `Unsupported time limit: ${transaction1CTParams.sessionPeriod.end}` - ); - } + const SESSION_PERIODS = { + "5min": { value: 5, unit: "minute" }, + "10min": { value: 10, unit: "minute" }, + "30min": { value: 30, unit: "minute" }, + "1hour": { value: 1, unit: "hour" }, + "3hours": { value: 3, unit: "hours" }, + "12hours": { value: 12, unit: "hours" }, + } as const; + + const period = SESSION_PERIODS[transaction1CTParams.sessionPeriod.end]; + if (!period) { + throw new Error( + `Unsupported time limit: ${transaction1CTParams.sessionPeriod.end}` + ); + } + + sessionPeriod = { + end: unixSecondsToNanoSeconds( + dayjs().add(period.value, period.unit).unix() + ), + };This approach:
- Reduces code duplication
- Makes it easier to add/modify periods
- Maintains type safety with
as constpackages/web/stores/transfer-history.tsx (1)
442-447: Consider tracking the direct DOM manipulation issueWhile the direct DOM manipulation is currently necessary due to react-toastify issues (as noted in the comments), it would be beneficial to track this technical debt for future improvement.
Would you like me to create a GitHub issue to track the potential migration away from direct DOM manipulation once react-toastify issues are resolved?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/web/components/bridge/deposit-address-screen.tsx(2 hunks)packages/web/components/one-click-trading/one-click-remaining-time.tsx(2 hunks)packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx(2 hunks)packages/web/stores/__tests__/transfer-history.spec.tsx(2 hunks)packages/web/stores/transfer-history.tsx(2 hunks)packages/web/utils/__tests__/date.spec.ts(1 hunks)packages/web/utils/date.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/web/stores/__tests__/transfer-history.spec.tsx
[error] 96-96: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (18)
packages/web/components/one-click-trading/one-click-remaining-time.tsx (2)
7-7: Clean implementation of the humanized time display changes!
The changes effectively integrate the new displayHumanizedTime utility while maintaining the component's core functionality and error handling.
Also applies to: 58-58
58-58: Verify the displayHumanizedTime implementation
Let's ensure the new utility correctly handles minutes as per the PR objective (e.g., "3 hours 59 minutes" case).
✅ Verification successful
Implementation correctly handles hours and minutes display
The displayHumanizedTime implementation, along with the underlying humanizeTime function, correctly handles the "3 hours 59 minutes" case and similar scenarios. The code specifically:
- Handles hours with minutes when time is less than 24 hours
- Correctly formats plural/singular units
- Joins time units with appropriate delimiter
- Updates every second to maintain accuracy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the displayHumanizedTime implementation and its test coverage
# Check the implementation
ast-grep --pattern 'function displayHumanizedTime' -A 20
# Check test coverage
rg -A 10 "describe.*displayHumanizedTime|test.*displayHumanizedTime|it.*displayHumanizedTime"
Length of output: 154
Script:
#!/bin/bash
# Let's try a broader search to locate the displayHumanizedTime implementation
# First, search for the file containing the implementation
rg -l "displayHumanizedTime"
# Then search for any exports or declarations of displayHumanizedTime
ast-grep --pattern 'export function displayHumanizedTime'
ast-grep --pattern 'export const displayHumanizedTime'
ast-grep --pattern 'const displayHumanizedTime'
# Also search for any test files containing displayHumanizedTime
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | xargs rg "displayHumanizedTime"
Length of output: 563
Script:
#!/bin/bash
# Let's examine the implementation in the utils/date.ts file
cat packages/web/utils/date.ts
# Also check the component using it to understand the context
cat packages/web/components/one-click-trading/one-click-remaining-time.tsx
Length of output: 4497
packages/web/utils/__tests__/date.spec.ts (2)
18-26: LGTM! Well-structured test runner implementation.
The new test runner implementation properly supports multiple time units and handles translations correctly. The iteration over expected units aligns well with the PR objective of improving time accuracy by supporting combinations like hours and minutes.
Line range hint 1-73: Verify the humanizeTime function implementation.
The test changes look good and align well with the PR objectives. Let's verify the actual implementation of the humanizeTime function to ensure it matches these test cases.
✅ Verification successful
Implementation matches test cases and handles time units correctly
The humanizeTime implementation aligns perfectly with the test cases and PR objectives:
- Correctly handles seconds (< 60s)
- Properly formats minutes (< 60m)
- Accurately represents hours and minutes together (< 24h)
- Returns formatted date for longer periods
- Supports singular/plural forms for all units
- Implements the new multi-unit output format, especially for hours and minutes combination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and analyze the humanizeTime function implementation
ast-grep --pattern 'export function humanizeTime($_) {
$$$
}'
Length of output: 65
Script:
#!/bin/bash
# Let's try to find the humanizeTime function with ripgrep first
rg "humanizeTime" -A 10 --type ts
Length of output: 14483
Script:
#!/bin/bash
# Let's get the full implementation of the humanizeTime function
rg -A 50 "export function humanizeTime" packages/web/utils/date.ts
Length of output: 1366
Script:
#!/bin/bash
# Let's get the complete implementation as the previous output was truncated
rg -A 100 "export function humanizeTime" packages/web/utils/date.ts
Length of output: 2661
packages/web/stores/__tests__/transfer-history.spec.tsx (2)
18-23: LGTM: Mock translations properly support the new time format
The added mock translations for minutes and hours are implemented consistently with the existing pattern.
97-113: Verify the expected minute value in the test
The test sets up 3 hours and 59 minutes but expects "58 minutes" in the output. If this is intentional (e.g., due to rounding or time passage simulation), please add a comment explaining the discrepancy. Otherwise, the expected value might need adjustment.
✅ Verification successful
Looking at the code, I can now generate a final response based on the implementation details found.
The test is correct, and the discrepancy is expected
The test sets up 3 hours and 59 minutes but expects "58 minutes" because the time difference is calculated dynamically between the estimated arrival time and the current time. Since there's a small processing delay between setting up the time and rendering the component, it's expected to see the minute value decrease by 1. This is a common pattern in time-based tests to account for processing time.
Key evidence:
- The component uses
dayjs().diff()to calculate the time difference in real-time - The time is updated every second through a setInterval in the component
- The humanizeTime function in
packages/web/utils/date.tscalculates the difference between the target date and the current time
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for time calculation logic in the component
ast-grep --pattern 'function PendingTransferCaption' -A 20
Length of output: 60
Script:
#!/bin/bash
# Let's try to find the PendingTransferCaption component using ripgrep
rg -A 20 "PendingTransferCaption" --type tsx
Length of output: 72
Script:
#!/bin/bash
# Let's try to find the PendingTransferCaption component without file type restriction
rg -A 20 "PendingTransferCaption"
# Also search for any files that might contain this component
fd "PendingTransferCaption"
Length of output: 12428
Script:
#!/bin/bash
# Let's look for the humanizeTime function that might be handling the time calculation
rg -A 20 "humanizeTime"
Length of output: 22056
packages/web/utils/date.ts (5)
9-9: LGTM: Return type change enables multiple time unit support
The change from returning a single object to an array enables the representation of multiple time units, which is essential for improving time accuracy.
13-25: LGTM: Seconds case properly handles edge cases
The implementation correctly handles:
- Non-negative values using Math.max
- Singular/plural unit translations
- Short/long unit format options
47-71: Great improvement: Hours now include remaining minutes
This change directly addresses the PR objective by including minutes in the hour display. The implementation:
- Correctly calculates remaining minutes using modulo
- Maintains consistent translation key patterns
- Improves accuracy of time representation
92-92: LGTM: Default case maintains existing behavior
The implementation correctly maintains the existing date formatting behavior while adapting to the new array return type.
30-42: Verify handling of edge cases near minute boundaries
The current implementation might show 0 minutes for durations between 0-59 seconds. Consider if this is the desired behavior or if rounding up would be more appropriate for user experience.
Let's verify the behavior:
packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx (2)
31-31: LGTM: Import addition aligns with PR objective
The addition of displayHumanizedTime import alongside humanizeTime supports the goal of improving time accuracy display.
241-241: LGTM: Enhanced time display in session activation toast
The implementation correctly uses displayHumanizedTime to show more accurate session duration in the toast notification.
Let's verify the toast displays correctly for various durations:
✅ Verification successful
LGTM: Time display implementation is consistent across the codebase
The implementation is correctly using displayHumanizedTime utility, which is consistently used across the codebase for displaying remaining time in various components including:
- One-click trading session notifications
- Bridge deposit address screen
- Transfer history
- One-click trading remaining time component
The toast implementation follows the same pattern as other time displays in the application, ensuring a uniform user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check toast message implementations across the codebase
# to ensure consistent usage of displayHumanizedTime
# Search for toast implementations with time display
rg -A 3 'displayToast.*captionElement.*displayHumanizedTime'
# Search for other potential places that might need similar updates
rg -A 3 'humanizeTime.*remaining'
Length of output: 98
Script:
#!/bin/bash
# Let's try a broader search to find toast implementations and time display patterns
# Search for displayHumanizedTime usage
rg -A 3 'displayHumanizedTime'
# Search for toast implementations with time-related content
rg -A 3 'displayToast.*time'
# Search for components using remaining time display
rg -A 3 'remaining.*time'
# Check the implementation of displayHumanizedTime utility
fd -e ts -e tsx displayHumanizedTime --exec cat {}
Length of output: 8295
packages/web/stores/transfer-history.tsx (2)
26-26: LGTM: Import changes align with PR objective
The addition of displayHumanizedTime while maintaining humanizeTime is appropriate as both functions serve distinct purposes in the time formatting logic.
443-446: LGTM: Time display improvements implemented correctly
The implementation properly uses displayHumanizedTime to show more accurate duration information, aligning with the PR objective to display both hours and minutes instead of just hours.
packages/web/components/bridge/deposit-address-screen.tsx (3)
31-31: LGTM!
The import statement follows the project's conventions and is correctly placed with other utility imports.
597-597: LGTM! Implementation aligns with PR objectives.
The change correctly utilizes the new displayHumanizedTime utility function while maintaining translation support, which should improve the accuracy of displayed time as intended.
597-597: Verify handling of edge cases.
Since this component displays expiration times for deposit addresses, please ensure the new time formatting handles edge cases appropriately:
- Very short remaining times (< 1 hour)
- Times close to expiration (e.g., 4 hours warning threshold)
✅ Verification successful
Time formatting handles edge cases appropriately
Based on the comprehensive test cases and implementation found:
- The
humanizeTimefunction properly handles all edge cases:- Single second/seconds (< 60s)
- Single minute/minutes (< 60m)
- Single hour/hours with minutes
- Dates beyond a day
- The implementation uses a real-time hook that updates the display continuously
- The formatting is internationalized through translation keys
- Zero and negative times are handled gracefully (Math.max prevents negative seconds)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for tests and other usages of displayHumanizedTime to verify edge case handling
# Find test files related to humanized time
echo "Searching for related test files..."
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . | rg -i "humanize.*time"
# Find other components using the utility
echo "Searching for other usages of displayHumanizedTime..."
rg "displayHumanizedTime" -A 3
# Look for time-related constants or thresholds
echo "Searching for time-related constants..."
rg "hour|minute|second" -A 3 "src|packages" | rg "const|let|var"
Length of output: 3906
Script:
#!/bin/bash
# Let's check the implementation of displayHumanizedTime and humanizeTime functions
echo "Checking time formatting implementation..."
rg -A 20 "^export function (displayHumanizedTime|humanizeTime)" packages/web/utils/date.ts
# Let's also check for any time-related constants or thresholds in the deposit address screen
echo "Checking deposit address screen for time thresholds..."
rg "(const|let|var).*(\d+\s*(hour|minute|second|ms)|remainingTime)" packages/web/components/bridge/deposit-address-screen.tsx
# Let's look for test cases specifically for date utils
echo "Checking for date utility tests..."
fd "date" -e test.ts -e test.tsx -e spec.ts -e spec.tsx packages/web
Length of output: 1626
Script:
#!/bin/bash
# Let's check the date utility tests to see how edge cases are handled
echo "Checking date utility test cases..."
cat packages/web/utils/__tests__/date.spec.ts
# Let's also check how the remaining time is calculated in the deposit screen
echo "Checking remaining time calculation..."
rg -B 5 -A 5 "humanizedRemainingTime" packages/web/components/bridge/deposit-address-screen.tsx
Length of output: 2903
Script:
#!/bin/bash
# Let's check the implementation of useHumanizedRemainingTime hook
echo "Checking useHumanizedRemainingTime implementation..."
rg -A 15 "useHumanizedRemainingTime" packages/web
# Let's also check for any warning thresholds or time-related constants
echo "Checking for time-related constants in the codebase..."
rg "(WARNING_THRESHOLD|EXPIRY_THRESHOLD|TIME_THRESHOLD)" packages/web
Length of output: 3939
| @@ -86,4 +92,24 @@ describe("PendingTransferCaption", () => { | |||
|
|
|||
| expect(screen.getByText(/About 5 seconds remaining/)).toBeInTheDocument(); | |||
| }); | |||
|
|
|||
| it.only("displays the hours and minutes", () => { | |||
There was a problem hiding this comment.
Remove .only to enable all tests
The .only modifier prevents other tests from running. This was likely used during development and should be removed.
Apply this diff:
- it.only("displays the hours and minutes", () => {
+ it("displays the hours and minutes", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.only("displays the hours and minutes", () => { | |
| it("displays the hours and minutes", () => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/web/components/nomic/nomic-pending-transfers.tsx (1)
Line range hint
89-89: Consider extracting magic numbers into named constants.There are several magic numbers in the code that would benefit from being extracted into named constants with clear documentation:
- The success threshold of 6 confirmations
- The refetch interval of 30000ms
- The estimation of 10 minutes per confirmation
Consider applying this refactor:
+ /** Number of confirmations required for a Bitcoin transaction to be considered successful */ + const BITCOIN_SUCCESS_THRESHOLD = 6; + /** Interval (in ms) to check for updates to pending transfers */ + const PENDING_TRANSFERS_REFETCH_INTERVAL = 30000; + /** Average time (in minutes) between Bitcoin block confirmations */ + const AVERAGE_BITCOIN_BLOCK_TIME = 10; - const successThreshold = 6; - const refetchInterval = 30000; + const successThreshold = BITCOIN_SUCCESS_THRESHOLD; + const refetchInterval = PENDING_TRANSFERS_REFETCH_INTERVAL; const humanizedEstimatedTime = humanizeTime( - dayjs().add((successThreshold - depositData.confirmations) * 10, "minutes") + dayjs().add( + (successThreshold - depositData.confirmations) * AVERAGE_BITCOIN_BLOCK_TIME, + "minutes" + ) );Also applies to: 282-285
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/web/components/nomic/nomic-pending-transfers.tsx(2 hunks)
🔇 Additional comments (2)
packages/web/components/nomic/nomic-pending-transfers.tsx (2)
23-23: LGTM: Import statement aligns with the new time display approach.
The addition of displayHumanizedTime import alongside humanizeTime supports the centralization of time formatting logic.
290-293: LGTM: Time display implementation improved.
The change from direct string interpolation to using displayHumanizedTime ensures consistent and accurate time formatting across the application. This aligns with the PR objective to improve time accuracy, particularly for cases where both hours and minutes need to be displayed.
Let's verify the consistent usage of this new function across the codebase:
✅ Verification successful
Consistent usage of displayHumanizedTime confirmed across the codebase
The verification shows that displayHumanizedTime is consistently used across all relevant components and no direct string interpolation with humanizeTime.value and unitTranslationKey was found. The function is properly imported and utilized in:
- Transfer history store
- One-click trading components and hooks
- Nomic pending transfers
- Bridge deposit address screen
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct usage of humanizeTime that might need updating
# Expected: No direct string interpolation with humanizeTime.value and unitTranslationKey
# Search for potential direct usage of humanizeTime
rg -A 2 'humanizeTime.*\{.*value.*unitTranslationKey'
# Search for adoption of displayHumanizedTime
rg 'displayHumanizedTime'
Length of output: 1422
What is the purpose of the change:
Currently, the “humanize time” function only displays the remaining hours, ignoring the minutes. This can be misleading, as a duration like 3 hours and 59 minutes is displayed simply as 3 hours.
Linear Task
https://linear.app/osmosis/issue/FE-1257/improve-humanize-time-accuracy
Testing and Verifying