fix(ups): convert estimatedRuntime from minutes to seconds#1822
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUPS runtime calculation changed to parse Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts (1)
24-27: LGTM! Conversion logic correctly fixes the unit mismatch.The conversion from minutes to seconds is mathematically correct, and the default value of '60' minutes appropriately maintains the 3600-second (1-hour) fallback. The comment provides valuable context about the external apcupsd format.
Optional: Consider defensive handling for invalid input.
If
upsData.TIMELEFTis an empty string or non-numeric,parseFloatreturnsNaN, resulting inestimatedRuntime: NaN. The||operator only catchesnull/undefined, not invalid strings. While the old code had the same limitation and apcupsd should provide valid data in normal operation, you could add a fallback:// Convert TIMELEFT from minutes (apcupsd format) to seconds -estimatedRuntime: Math.round( - parseFloat(upsData.TIMELEFT || '60') * 60 -), +estimatedRuntime: Math.round( + (parseFloat(upsData.TIMELEFT || '60') || 60) * 60 +),This ensures that any
NaNfromparseFloatfalls back to 60 minutes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts
🧠 Learnings (1)
📚 Learning: 2025-11-24T17:51:37.915Z
Learnt from: CR
Repo: unraid/api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:51:37.915Z
Learning: Applies to api/**/*.{ts,tsx} : cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Applied to files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts
|
@jandrop Thanks so much for your contribution! For consistency, and to avoid breaking changes with consumers, why don't we just change the comment on this to tell users that the value is returned in minutes (matching apcupsd)? We'd love to get this in if you change to that approach, your contribution is much appreciated! |
Hi If the project prefers to keep returning minutes for compatibility reasons, that's totally fine. Feel free to reject/close this PR — I honestly have no idea where the documentation for this part is located, so it's definitely easier if someone from the team updates it directly to avoid future confusion. Thanks anyway! |
|
Just in case this ends up in someone's google search, for future reference, the graphql schema documentation lives within the code, inside In this case, the incorrect documentation is coming from |
|
Good news! We decided to go proceed based on NUT standards as you've suggested. If the workflows pass, we can merge it in. |
I'm glad to hear that. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the estimatedRuntime field in the UPS battery data to correctly return values in seconds instead of minutes, aligning with the GraphQL schema documentation that specifies the unit should be seconds.
Key Changes:
- Converted
TIMELEFTvalue from apcupsd (minutes) to seconds by multiplying by 60 - Changed parsing from
parseInttoparseFloatwithMath.roundto handle decimal minute values - Updated the default fallback value from
'3600'to'60'(maintaining 3600 seconds as the default)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts (1)
21-29: Fixture TIMELEFT now matches apcupsd minutes; consider adding an assertion on estimatedRuntimeThe change to
TIMELEFT: '60'with the minutes-format comment looks correct and consistent with the resolver’s new “minutes → seconds” conversion.However, this spec still doesn’t assert anything about
estimatedRuntime, so a regression like the original bug (returning minutes instead of seconds) would not be caught here. Consider extending theupsDevicestest to also verify the computed runtime, for example by checking thatresult[0].estimatedRuntimeequals the expected seconds value derived from thisTIMELEFTinput.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-01-27T14:57:46.617Z
Learnt from: elibosley
Repo: unraid/api PR: 1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.617Z
Learning: The values in `web/_data/serverState.ts` are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.
Applied to files:
api/src/unraid-api/graph/resolvers/ups/ups.resolver.spec.ts
The TIMELEFT value from apcupsd is returned in minutes, but the GraphQL schema documentation states it should be in seconds. This fix converts the value by multiplying by 60 to match the documented API contract. Fixes unraid#1821
elibosley
force-pushed
the
fix/ups-estimated-runtime-unit
branch
from
e87a93a to
b605030
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1822 +/- ##
=======================================
Coverage 52.04% 52.04%
=======================================
Files 876 876
Lines 50509 50510 +1
Branches 5023 5023
=======================================
+ Hits 26285 26286 +1
Misses 24149 24149
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@elibosley could a cloudflare perms/secrets access issue ( |
Summary
Fixes the
estimatedRuntimefield in the UPS GraphQL query to return values in seconds as documented, instead of minutes.Problem
The
TIMELEFTvalue fromapcupsdis returned in minutes (e.g.,6.0for 6 minutes), but the GraphQL schema documentation states:Currently, the API returns
6(minutes) instead of360(seconds).Solution
Convert the
TIMELEFTvalue from minutes to seconds by multiplying by 60:Testing
upsDevicesbefore the fix →estimatedRuntime: 6(incorrect - minutes)upsDevicesafter the fix →estimatedRuntime: 360(correct - seconds)Tested on Unraid server with APC UPS connected via apcupsd.
Related Issues
Fixes #1821
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.