-
Notifications
You must be signed in to change notification settings - Fork 30
fix: correct exports (tsdown changes) #756
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
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. WalkthroughUpdated tooling (c8 → nyc) and package exports to ESM (.mjs); refactored safeImport to a named export with JSON import support; removed the integrity/insist export and its re-exports; exposed/adjusted several time/translation utilities and added tests. Biome schema bumped. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant SafeImport as safeImport()
participant DynImport as Dynamic Import
participant Module
participant Fallback
Caller->>SafeImport: safeImport(path, fallback)
alt path ends with .json
SafeImport->>DynImport: import(path, { with: { type: 'json' } })
else
SafeImport->>DynImport: import(path)
end
alt import succeeds
DynImport-->>SafeImport: module
alt module has default
SafeImport->>Caller: return module.default
else
SafeImport->>Caller: return module
end
else import fails
SafeImport->>Fallback: log and return fallback
Fallback-->>Caller: fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)test/utilities/timedate.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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 |
Dependency ReviewThe following issues were found:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR focuses on improving test coverage and removing dead code. The main changes include migrating from c8 to nyc for coverage reporting, removing the unused tools/integrity.ts file, converting from default to named exports for safeImport, and adding new test cases to improve coverage of existing utility functions. However, there are critical bugs introduced in the safeImport function and package.json configuration.
Key Changes
- Migrated test coverage tool from c8 to nyc with updated configuration
- Removed dead code (
tools/integrity.tsand its exports) - Converted
safeImportfrom default export to named export - Added dependency injection to time-related functions (
weeklyReset,dailyReset) for better testability - Expanded test coverage for translation, time/date utilities, and safeImport functions
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Replaced c8 with nyc dependency; updated test scripts; changed exports to .mjs extensions (inconsistent with main field) |
| package-lock.json | Updated dependencies for nyc and removed c8-related packages |
| safeImport.ts | Changed from default to named export; removed conditional JSON import logic (breaking change) |
| exports.ts | Updated import to use named export syntax |
| tools/integrity.ts | Removed entire file containing unused insist function |
| tools/utilities.ts | Added istanbul ignore comment; removed integrity exports |
| tools/timeDate.ts | Added dependency injection for weeklyReset and dailyReset functions |
| test/utilities/timedate.spec.ts | Added tests for new time functions and edge cases |
| test/utilities/translation.spec.ts | Updated tests to remove unnecessary locale parameters and add type assertions |
| test/exports.spec.ts | Updated import syntax and added test for fallback behavior |
| tsdown.config.ts | Removed integrity.ts from build entries |
| biome.json | Updated schema version |
| .nyc.json | Added html reporter to coverage output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/utilities/timedate.spec.ts (2)
26-34: NewtimeDeltaToStringedge‑case tests match implementation (one tiny wording nit)
16ms →'0s'is consistent withMath.abs(millis / 1000)and flooring seconds.- Non‑number input correctly asserts the
TypeErrorthrown by the runtime guard.- Negative delta
-120000→'-2m 0s'matches the prefix +pieceIsSmollerbehavior.Only nit: the description string has a duplicated word.
- it('should should return seconds for timestamps under a minute', () => { + it('should return seconds for timestamps under a minute', () => {
43-71: Deterministic tests forfromNow/toNowand reset helpers are well‑designed
- Using a fixed
nowfunction forfromNow/toNowvalidates the sign and magnitude of the millisecond deltas exactly as implemented intools/timeDate.ts.weeklyReset(nowFunc)correctly asserts activation on the previous Monday and expiry on the next Monday at00:00:00.000Zgiven a Wednesdaynow.dailyReset(nowFunc)correctly asserts activation at the current day’s midnight and expiry at the following day’s midnight.If you want to go further later, consider adding boundary tests (e.g.,
nowon a Sunday or exactly at midnight) to lock in behavior around reset edges, but what’s here is already solid.tools/timeDate.ts (1)
82-95: InjectednowFuncpreserves behavior and improves testability; update JSDoc to matchThe refactor to accept an optional
nowFuncfor bothweeklyResetanddailyResetis clean:
- Default
() => new Date()keeps existing runtime behavior intact.- Tests can now inject a fixed
Datesource, which is exactly howtest/utilities/timedate.spec.tsuses these helpers.- Activation/expiry calculations remain unchanged relative to the original logic.
The only gap is that the JSDoc comments don’t mention the injectable
nowFuncparameter. Consider updating them for clarity:/** * Get a weekly reset timestamp + * @param {() => Date} [nowFunc] Optional function returning the current Date (useful for tests) */ export const weeklyReset = (nowFunc = () => new Date()): { activation: Date; expiry: Date } => { @@ /** * Get a daily reset timestamp + * @param {() => Date} [nowFunc] Optional function returning the current Date (useful for tests) */ export const dailyReset = (nowFunc = () => new Date()): { activation: Date; expiry: Date } => {Also applies to: 100-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.nyc.json(1 hunks)biome.json(1 hunks)exports.ts(1 hunks)package.json(4 hunks)safeImport.ts(1 hunks)test/exports.spec.ts(2 hunks)test/utilities/timedate.spec.ts(3 hunks)test/utilities/translation.spec.ts(5 hunks)tools/integrity.ts(0 hunks)tools/timeDate.ts(2 hunks)tools/utilities.ts(1 hunks)tsdown.config.ts(0 hunks)
💤 Files with no reviewable changes (2)
- tsdown.config.ts
- tools/integrity.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T16:31:39.910Z
Learnt from: TobiTenno
Repo: WFCD/warframe-worldstate-data PR: 579
File: test/languages.spec.js:5-5
Timestamp: 2024-11-19T16:31:39.910Z
Learning: In the 'warframe-worldstate-data' project, using `import ... with { type: 'json' }` syntax is compatible with Node.js 18 and is acceptable.
Applied to files:
exports.tstest/exports.spec.ts
🧬 Code graph analysis (2)
test/exports.spec.ts (1)
safeImport.ts (1)
safeImport(10-24)
test/utilities/translation.spec.ts (1)
tools/translation.ts (4)
toTitleCase(9-11)nodeEnemy(93-95)region(257-258)languageString(103-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Summary
🔇 Additional comments (16)
tools/utilities.ts (1)
1-4: LGTM! Appropriate coverage exclusion for re-export module.The
istanbul ignore filedirective is appropriate for this file since it only contains re-exports and has no testable logic of its own..nyc.json (1)
3-3: LGTM! Enhanced coverage reporting.Adding the HTML reporter provides better local coverage visualization alongside lcov and text reports.
exports.ts (1)
1-1: LGTM! Consistent with safeImport refactoring.The switch from default to named import aligns with the safeImport.ts changes and maintains consistency across the codebase.
test/exports.spec.ts (2)
6-6: LGTM! Consistent with safeImport refactoring.The switch to named import aligns with the safeImport.ts changes.
44-49: LGTM! Improved test coverage for fallback behavior.The new test case explicitly verifies that
safeImportreturns the provided fallback when a file doesn't exist, enhancing test coverage and making the expected behavior more explicit.test/utilities/translation.spec.ts (5)
39-41: LGTM! Enhanced test coverage for toTitleCase.The three new test cases cover additional input variations (lowercase-only, uppercase-only, and mixed-case), improving test coverage for edge cases.
75-81: LGTM! Simplified tests using default locale.The removal of the explicit locale parameter in the default locale section simplifies the tests while maintaining the same behavior (defaulting to 'en').
252-252: LGTM! Appropriate type casting for edge case testing.The type cast allows testing the
regionfunction's behavior when passed an invalid type (string instead of number), which is a valid edge case to verify.
353-353: LGTM! Appropriate type casting for undefined input test.The type cast enables testing the function's behavior with undefined input, which is a valid edge case to verify.
507-507: LGTM! Consistent edge case testing.The type cast maintains consistency with the default locale section's edge case testing approach.
package.json (3)
54-55: LGTM! Coverage tooling migration.The migration from c8 to nyc is appropriate and aligns with the PR objectives to improve test coverage infrastructure.
82-82: LGTM! Added nyc dependency.The nyc dependency is correctly added to devDependencies to support the new coverage tooling.
94-96: LGTM! Formatting consistency.The multi-line formatting of
publishConfigimproves readability without changing functionality.safeImport.ts (1)
10-24: LGTM! All safeImport usages verified to target JSON files.Verification confirms that all 36 production safeImport calls in exports.ts target JSON files (arcanes.json, archonShards.json, etc.). The
with: { type: 'json' }assertion is appropriate and necessary. Test cases intentionally use non-JSON files ('poop.js') to verify fallback behavior, which is valid. The project requires Node.js >=22, ensuring full compatibility with the JSON import assertion syntax.biome.json (1)
2-2: Schema URL is valid and accessible.The Biome schema version 2.3.6 exists and is properly published. Verification confirms:
- The schema URL returns HTTP 200 (accessible)
- Biome v2.3.6 is an official release
- The URL format matches Biome's documented schema pattern
- Version is compatible with devDependencies
test/utilities/timedate.spec.ts (1)
3-3: Import surface and type usage look correctBringing in
ContentTimestamp,fromNow,toNow,weeklyReset, anddailyResetfrom../../tools/timeDatematches the module’s exports and tightens typing in theparseDatetests. No issues here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/utilities/timedate.spec.ts (1)
26-34: Missing semicolon on line 34.The closing brace on line 34 is missing a semicolon, which is inconsistent with the other test cases in this file.
Apply this diff:
it('should handle negative time deltas', () => { timeDeltaToString(-120000).should.equal('-2m 0s'); - }) + });
Test logic is correct and improves coverage.
These new tests appropriately cover:
- Edge case where milliseconds round to zero seconds (line 26-28)
- Type validation for non-number inputs (line 29-31)
- Negative time delta formatting with prefix (line 32-34)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(4 hunks)safeImport.ts(1 hunks)test/utilities/timedate.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T16:31:39.910Z
Learnt from: TobiTenno
Repo: WFCD/warframe-worldstate-data PR: 579
File: test/languages.spec.js:5-5
Timestamp: 2024-11-19T16:31:39.910Z
Learning: In the 'warframe-worldstate-data' project, using `import ... with { type: 'json' }` syntax is compatible with Node.js 18 and is acceptable.
Applied to files:
safeImport.ts
🧬 Code graph analysis (1)
test/utilities/timedate.spec.ts (1)
tools/timeDate.ts (5)
timeDeltaToString(19-42)fromNow(50-52)toNow(60-62)weeklyReset(82-95)dailyReset(100-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
test/utilities/timedate.spec.ts (4)
3-3: LGTM! Import additions align with new test coverage.The new imports (
fromNow,toNow,weeklyReset,dailyReset) are all utilized in the expanded test suites below.
43-55: Excellent use of injectable clock for deterministic testing.The
fromNow()andtoNow()tests correctly verify the millisecond difference calculations using a fixednowfunction. The test values are accurate:
fromNow()returns-3600000(date is 1 hour in the past)toNow()returns3600000(1 hour has elapsed since date)
56-63: Weekly reset test logic is correct.The test properly verifies that for a Wednesday (Sept 15), the weekly reset window spans from the previous Monday (Sept 13 at 00:00 UTC) to the next Monday (Sept 20 at 00:00 UTC). The injectable
nowFuncensures deterministic behavior.
64-71: Daily reset test logic is correct.The test properly verifies that the daily reset window spans from the start of the current day (Sept 15 at 00:00 UTC) to the start of the next day (Sept 16 at 00:00 UTC). The injectable
nowFuncensures deterministic behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/utilities/timedate.spec.ts (1)
56-63: Consider adding weekly reset edge-case tests (Sunday, Monday, Saturday)The Wednesday case here is correct and matches the
weeklyResetlogic (previous Monday → next Monday). To harden coverage around thecurrentDay === 0 ? 1 : 8 - currentDaycalculation, you might also add cases for:
- Sunday (UTC day 0, special branch),
- Monday (reset day itself),
- Saturday (day before reset).
This reiterates a prior suggestion on the same code path but remains optional.
🧹 Nitpick comments (2)
safeImport.ts (1)
12-14: Consider usingendsWithfor more precise JSON detection.The current check
path.includes('.json')will match.jsonanywhere in the path, which could cause issues if.jsonappears in a directory name (e.g.,/path/.json.backup/module.ts). Usingpath.endsWith('.json')would be more precise.Apply this diff:
- const mod: T | { default: T } = path.includes('.json') + const mod: T | { default: T } = path.endsWith('.json') ? await import(path, { with: { type: 'json' } }) : await import(path);test/utilities/timedate.spec.ts (1)
26-34: Clarify test descriptions and units aroundtimeDeltaToStringThe behavior tested is correct, but the wording could be clearer:
Line 26–28:
timeDeltaToString(16)is testing sub‑second behavior (16ms → '0s'). The description says “under a minute”, which is technically true but a bit misleading for this specific case. Consider either:
- Changing the value to
16000and expecting'16s'to clearly exercise “under a minute but ≥ 1s`, or- Renaming the test to something like “should return
0sfor timestamps under a second”.Line 29–31: The test name “should test non-number inputs” is vague. Consider renaming to “should throw TypeError for non-number inputs” to describe behavior rather than the act of testing.
Line 32–34: Negative delta case and expectation
'-2m 0s'correctly reflect the implementation and add good coverage for the sign prefix.These are naming/semantics nits only; behavior looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
safeImport.ts(1 hunks)test/utilities/timedate.spec.ts(3 hunks)tsdown.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tsdown.config.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T16:31:39.910Z
Learnt from: TobiTenno
Repo: WFCD/warframe-worldstate-data PR: 579
File: test/languages.spec.js:5-5
Timestamp: 2024-11-19T16:31:39.910Z
Learning: In the 'warframe-worldstate-data' project, using `import ... with { type: 'json' }` syntax is compatible with Node.js 18 and is acceptable.
Applied to files:
safeImport.ts
🧬 Code graph analysis (1)
test/utilities/timedate.spec.ts (1)
tools/timeDate.ts (5)
timeDeltaToString(19-42)fromNow(50-52)toNow(60-62)weeklyReset(82-95)dailyReset(100-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
safeImport.ts (1)
15-15: LGTM! Falsy value handling corrected.The
'default' in modcheck properly handles modules with falsy default exports (e.g.,false,0,""), addressing the concerns raised in previous reviews. This ensures valid falsy defaults are returned correctly instead of falling back to the module namespace.test/utilities/timedate.spec.ts (3)
3-3: Updated imports are correct and fully usedAll newly imported utilities (
fromNow,toNow,weeklyReset,dailyReset) are exercised in this file; no unused or missing imports.
43-55:fromNow/toNowtests correctly cover the injected clock behaviorThe tests use a fixed
nowand a date one hour earlier to assert-3600000forfromNowand3600000fortoNow, matching the implementations with an injectednowfunction. This gives deterministic coverage for both directions of the delta.
64-71: Daily reset test matches the intended activation/expiry windowThe test correctly asserts that for a fixed timestamp,
dailyResetreturns activation at that day’s UTC midnight and expiry at the following day’s UTC midnight. This aligns with the implementation and gives good confidence in the injectednowFuncbehavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe3b601 to
b473bf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/utilities/timedate.spec.ts (1)
56-71: Reset function tests are correct; consider a few more weeklyReset edge casesThe new
weeklyResetanddailyResettests using injectednowFuncline up with the implementation:
- Weekly reset: Wednesday produces an activation of the previous Monday at 00:00 UTC and expiry at the following Monday at 00:00 UTC.
- Daily reset: activation at today’s midnight UTC and expiry at tomorrow’s.
For even stronger coverage of the weekly calculation, you could optionally add cases for Sunday, Monday, and Saturday to exercise the
currentDay === 0 ? 1 : 8 - currentDaybranch more fully. This was previously suggested in an earlier review, so feel free to skip if you’ve already decided current coverage is sufficient.
🧹 Nitpick comments (4)
.github/workflows/ci.yaml (1)
30-40: CI now runs tests without coverage upload—confirm this is intentionalSwitching the test job from
npm run test:covto plainnpm testmeans CI no longer runs the Coveralls reporting step. Coverage will still be produced locally bynyc, but it won’t be uploaded anywhere from this workflow. Please confirm this matches your desired coverage strategy, or consider invokingnpm run test:cov(or a dedicated coverage job) if you still want CI-driven reports..github/workflows/release.yaml (1)
30-40: Release workflow tests no longer run coverage/coverallsThe release workflow’s test job now runs
npm testinstead of the coverage+Coveralls script. If Coveralls (or any coverage gate) was previously part of your release criteria, this change effectively disables it. If that’s not intended, consider restoring a coverage-specific step (e.g.,npm run test:cov) or adding a separate coverage/reporting job.package.json (1)
47-56: NYC-based test/coverage scripts look coherent but CI doesn’t usetest:covUpdating
"test"tonyc mochaand"test:cov"to runnpm test && nyc report --reporter=text-lcov | npx coverallsis a sensible split between local coverage and CI/reporting, and the added nyc-related devDependencies match this setup. However, neither CI nor release workflows currently invokenpm run test:cov, so the Coveralls pipeline is effectively dead code. Consider either:
- Updating workflows to call
npm run test:covwhere you still want Coveralls, or- Dropping
test:cov(and possibly the Coveralls bits) if coverage upload is no longer part of your process.tools/timeDate.ts (1)
79-111: InjectablenowFuncin weeklyReset/dailyReset improves testability; consider updating docsAllowing
weeklyResetanddailyResetto accept anowFuncthat returns aDatemakes these utilities deterministic and easy to test, while the default() => new Date()keeps the runtime API unchanged. The UTC-based calculations for activation/expiry align with the new tests.The only minor follow-up is that the JSDoc for the default export (
TimeDateFunctions) still only documentstimeDeltaToString,fromNow, andtoNow, even thoughparseDate,dailyReset, andweeklyResetare also exported via the default object. Consider updating that comment so consumers see the full surface area.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/ci.yaml(1 hunks).github/workflows/release.yaml(1 hunks).nycrc.json(1 hunks)package.json(5 hunks)test/utilities/timedate.spec.ts(3 hunks)tools/timeDate.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .nycrc.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Summary
🔇 Additional comments (6)
package.json (2)
37-44: ESM-only export map and main are now consistentUsing
"type": "module"with all exports andmainpointing at.mjsindist/is internally consistent and matches an ESM-only distribution. This resolves the previous mismatch between ESM exports and a CJS-stylemainand should make module resolution more predictable for consumers.
64-91: NYC devDependencies and publishConfig provenance flagAdding
@istanbuljs/nyc-config-typescriptandnycas devDependencies, and turningpublishConfiginto a multi-line object with"provenance": true, all look fine and align with the new coverage tooling and supply-chain provenance best practices. No functional issues spotted here.test/utilities/timedate.spec.ts (3)
3-35: Additional timeDeltaToString tests correctly cover edge casesThe new tests for sub-minute deltas, non-number inputs, and negative deltas align with the implementation:
16ms ->'0s', type guarding withTypeError, and negative millis prefixed with-while formatting the absolute value. These improve coverage of both error paths and sign handling without changing behavior.
43-55: fromNow/toNow tests validate sign conventions around a fixed reference timeUsing a fixed
nowvalue and a date one hour earlier cleanly checks thatfromNowreturns a negative delta andtoNowreturns the corresponding positive delta. This matches the function definitions (d.getTime() - now()vsnow() - d.getTime()) and should catch regressions in their semantics.
72-96: pieceIsSmoller test accurately exercises day/hour/minute decompositionThe test for
pieceIsSmollerwalks through 90,061 seconds and validates that:
- 86,400 seconds →
'1d'with 3,661 seconds remaining,- 3,600 seconds →
'1h'with 61 seconds remaining,- 60 seconds →
'1m'with 1 second remaining,
and then appends the final seconds piece. This matches the helper’s contract and ensures the shared logic behindtimeDeltaToStringis directly covered.tools/timeDate.ts (1)
7-42: Exporting pieceIsSmoller and reusing it in timeDeltaToString is soundMaking
pieceIsSmollera named export while continuing to use it internally intimeDeltaToStringis a nice way to share the decomposition logic and enables focused unit tests. The helper’s behavior (only adding a label whenseconds >= ceiling) matches both existing and new tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🎉 This PR is included in version 3.1.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What did you fix?
n/a - more coverage, prune dead code
Reproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
Breaking Changes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.