Skip to content
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

feat(mobile): add estimated duration display for entry items #2956

Merged
merged 2 commits into from
Mar 5, 2025

Conversation

kovsu
Copy link
Contributor

@kovsu kovsu commented Mar 4, 2025

@follow-reviewer-bot
Copy link

Thank you for your contribution. We will review it promptly.

Copy link

vercel bot commented Mar 4, 2025

@kovsu is attempting to deploy a commit to the RSS3 Team on Vercel.

A member of the Team first needs to authorize it.

@follow-reviewer-bot
Copy link

Suggested PR Title:

feat(entry): display estimated time based on duration in seconds

Change Summary:
Added duration and estimated time display to EntryNormalItem.

Code Review:

Code Review

Issues Requiring Change Requests

  1. Function Name Consistency and Purpose

    • File: apps/mobile/src/modules/entry-list/templates/EntryNormalItem.tsx
    • Lines: 231, formatEstimatedMins
    • Issue: The function formatEstimatedMins is a formatting utility. However, its naming does not clearly convey its purpose in relation to its output. It formats time into a string with months, days, hours, and minutes, implying broader functionality than just minutes. This misalignment can lead to misunderstanding among developers.
    • Suggested Fix: Rename the function to something more descriptive like formatDuration or formatTimeFromMinutes to accurately reflect its purpose.
  2. Edge Case Handling for Zero Values

    • File: apps/mobile/src/modules/entry-list/templates/EntryNormalItem.tsx
    • Lines: 231–250, formatEstimatedMins
    • Issue: The function does not handle the case where all calculated values (e.g., months, days, hours, minutes) are 0. For example, if estimatedMins is 0, it returns "0 mins", which might not be user-friendly or meaningful.
    • Suggested Fix: Add a special case to handle 0 values more gracefully, such as returning "less than 1 minute" or "0 minutes" depending on the desired UX.
  3. Hardcoded Time Conversion Factors

    • File: apps/mobile/src/modules/entry-list/templates/EntryNormalItem.tsx
    • Lines: 233–235, formatEstimatedMins
    • Issue: The time conversion factors for months, days, hours, and minutes are hardcoded. While this is technically correct, it reduces flexibility and can lead to incorrect results if time units are ever redefined (e.g., accounting for leap years or varying month durations).
    • Suggested Fix: Use constants or a utilities module to define these values (MINUTES_IN_HOUR, HOURS_IN_DAY, etc.). This approach improves readability and maintainability.
  4. Conditional Logic Complexity

    • File: apps/mobile/src/modules/entry-list/templates/EntryNormalItem.tsx
    • Lines: 239–248, formatEstimatedMins
    • Issue: The conditionals in the formatEstimatedMins function are somewhat repetitive and verbose. For example, the same logic is repeated for each time unit without abstraction.
    • Suggested Fix: Refactor the function to reduce repetition. Consider iterating over a list of time units with corresponding labels and thresholds (e.g., { unit: 'month', value: minutesInMonth }) to generate the formatted result programmatically.
  5. Potential Rendering Edge Case

    • File: apps/mobile/src/modules/entry-list/templates/EntryNormalItem.tsx
    • Lines: 86–92
    • Issue: The estimatedMins && (...) check does not account for the scenario where estimatedMins is 0. In such cases, estimatedMins evaluates to false, and the component will omit rendering any duration label even though formatEstimatedMins can handle 0. This might lead to inconsistent UI behavior.
    • Suggested Fix: Update the rendering logic to explicitly check for estimatedMins !== undefined (or != null) to ensure 0 is handled correctly.

By addressing these issues, the code's readability, maintainability, and robustness can be improved while avoiding potential edge cases in functionality.

@Innei Innei merged commit 6588b3e into RSSNext:dev Mar 5, 2025
3 of 7 checks passed
@follow-reviewer-bot
Copy link

Thank you @kovsu for your contribution! 🎉

Your pull request has been merged and we really appreciate your help in making this project better. We hope to see more contributions from you in the future! 💪

@kovsu kovsu deleted the mobile/display-audio-times branch March 5, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants