Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds many new utilities, expanded type definitions, widened public APIs (notably variadic array partition, PInstance promise-collection with concurrency), reworked string template, throttle/debounce signature changes, numerous docs and tests, CI coverage additions, and small config/license updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PInstance
participant pLimit
participant Worker
Client->>PInstance: p(items?, { concurrency? })
Client->>PInstance: PInstance.add(...) / .map(fn) / .filter(fn)
Client->>PInstance: await PInstance.promise
PInstance->>pLimit: schedule tasks with concurrency limit
pLimit->>Worker: execute task
Worker->>PInstance: return resolved value
PInstance->>Client: resolve array of results
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Tip For best results, initiate chat on the files or code changes.
Here are some things I can do:
Just let me know what you need! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/vendor.ts (1)
50-76: Same JSDoc@parammismatch asthrottle.Apply the same fix approach chosen for
throttleto maintain consistency across both wrapper functions.Option 1: Use named parameters (preferred)
-export function debounce<T extends (...args: any[]) => any>(...args: any[]): ReturnWithCancel<T> { - // `@ts-expect-error` - return _debounce(...args) +export function debounce<T extends (...args: any[]) => any>( + delay: number, + callback: T, + options?: { atBegin?: boolean } +): ReturnWithCancel<T> { + return _debounce(delay, callback, options) as ReturnWithCancel<T> }src/types.ts (1)
121-142: GuardDeepMergerecursion to prevent incorrect type merging of primitives, arrays, and functions.The current type definition recursively merges all types without guards, mismatching the runtime behavior. For overlapping keys where both properties are primitives or arrays, the type should override with the second type, not attempt recursion. The runtime
deepMergefunction correctly prevents this viaisMergableObjectchecks, but theDeepMerge<F, S>type lacks equivalent guards. For example,DeepMerge<{a: string}, {a: number}>should yield{a: number}, but the current type incorrectly recurses on primitive types.The proposed fix adds proper guards to only recurse when both overlapping properties extend
Record<string, any>, aligning the type signature with the runtime implementation that overrides primitives, non-mergable objects, and arrays instead of attempting to merge them.src/math.ts (1)
65-67: Guard against zero-length ranges inremap.
inMax === inMinwill produceInfinity/NaN. Consider throwing or returning a safe default.🛠️ Proposed guard
export function remap(n: number, inMin: number, inMax: number, outMin: number, outMax: number) { + if (inMax === inMin) + return outMin const interpolation = (n - inMin) / (inMax - inMin) return lerp(outMin, outMax, interpolation) }src/string.ts (1)
91-94: Preserve falsy values in object-based templates.
vars[key] || …treats0,false, and''as missing. Use a presence check (or??) so valid falsy values are kept.🐛 Proposed fix
- return str.replace(/\{(\w+)\}/g, (_, key) => vars[key] || ((typeof fallback === 'function' ? fallback(key) : fallback) ?? key)) + return str.replace(/\{(\w+)\}/g, (_, key) => { + if (Object.prototype.hasOwnProperty.call(vars, key)) + return vars[key] + const fb = typeof fallback === 'function' ? fallback(key) : fallback + return (fb ?? key) + })src/p.ts (1)
83-94:map/filterignore items added viaadd().Both methods iterate only
this.items, so items queued withadd()are skipped. Consider combiningitemsandpromises(same as thepromisegetter).🐛 Proposed fix
- return new PInstance( - Array.from(this.items) + const items = [...Array.from(this.items), ...Array.from(this.promises)] + return new PInstance( + items .map(async (i, idx) => { const v = await i if ((v as any) === VOID) return VOID as unknown as U return fn(v, idx) }), this.options, )- return new PInstance( - Array.from(this.items) + const items = [...Array.from(this.items), ...Array.from(this.promises)] + return new PInstance( + items .map(async (i, idx) => { const v = await i const r = await fn(v, idx) if (!r) return VOID as unknown as T return v }), this.options, )Also applies to: 106-118
🤖 Fix all issues with AI agents
In `@src/array.ts`:
- Around line 223-231: The JSDoc for the range function mismatches the overloads
(the first overload takes only stop), triggering jsdoc/check-param-names; fix by
adding overload-specific JSDoc blocks or by renaming the JSDoc params to align
with the overload signatures. Locate the range overloads and either (a) add a
separate JSDoc block immediately above each overload signature that documents
the exact parameters for that overload (e.g., one doc for range(stop: number)
and another for range(start: number, stop?: number, step?: number)), or (b)
unify the JSDoc param names used for all overloads (for example use
startOrStop/step or make stop optional) so the documented param names match the
actual overload parameter names in the range function.
In `@src/function.ts`:
- Around line 61-64: The example using tap in the chain is confusing—update the
snippet around someArray.map(...).filter(tap(...)) to use a clearer variable
name (e.g., val instead of arr), correct the comment to indicate the log runs
during the filter step (e.g., "During filter:"), and ensure the example
demonstrates intended behavior: either show tap used for side-effects
(map(...).tap/forEach style) or, if showing it inside filter, make the predicate
explicit so tap doesn't inadvertently become the filter predicate; reference the
chain functions someArray, map, filter and the tap helper when making the
change.
In `@src/object.ts`:
- Around line 268-271: objectOmit fails to omit numeric keys because
Object.entries yields string keys while the keys array may contain numbers; fix
by normalizing the keys array to strings (e.g. build a Set from
keys.map(String)) at the start of objectOmit and use set.has(key) in the filter
instead of keys.includes, preserving the omitUndefined check and returning the
same Omit<O, T> type.
In `@src/p.ts`:
- Line 30: The field promises is a Set which silently deduplicates entries and
causes add() to lose duplicate values; change promises from Set<T | Promise<T>>
to an Array<T | Promise<T>> and update all usages (the add method, any
iterator/forEach logic, and any clear/flush logic currently interacting with
promises) to push items and iterate by index or for-of so duplicates are
preserved; ensure methods that previously relied on Set semantics (like deletion
or checking existence) are adjusted or replaced (e.g., use array.includes or
filter) and update any Promise handling logic that consumed promises to use the
new array shape.
- Around line 173-178: Add a Biome lint suppression above the then method to
silence the noThenProperty rule for the intentional thenable implementation:
place a comment like "biome-ignore lint/noThenProperty: intentional thenable for
PInstance extending Promise" immediately above the then(...) declaration in
class PInstance so the linter knows this is deliberate.
In `@src/string.ts`:
- Around line 61-70: Add separate JSDoc blocks above each overload of the
template function: one JSDoc for the object-based overload (export function
template(str: string, object: Record<string | number, any>, fallback?: string |
((key: string) => string)): string) documenting `@param` str, `@param` object,
`@param` fallback and return, and another JSDoc for the index-based overload
(export function template(str: string, ...args: (string | number | bigint |
undefined | null)[]): string) documenting `@param` str, `@param` args and return;
include the example snippets shown in the review in the object-based JSDoc to
demonstrate fallback behavior and keep the main descriptive comment above the
implementation unchanged.
In `@src/types.ts`:
- Around line 111-119: MergeInsertions currently treats functions and arrays as
plain objects, stripping call signatures and array/tuple structure; update
MergeInsertions to first detect and preserve function types and array/tuple
types before the object branch. Implement conditionals so MergeInsertions
returns T unchanged for Function, maps arrays/tuples via a recursive element
rule (e.g., T extends readonly (infer U)[] ? MergeInsertions<U>[] or
tuple-mapping) and only then falls back to the object mapped type; reference the
MergeInsertions type (and DeepMerge usages) when applying the change.
In `@src/vendor.ts`:
- Around line 23-48: The JSDoc param names (delay, callback, options) don't
match the function's rest-parameter signature; change the throttle function
signature from throttle<T>(...args: any[]) to a typed, named-parameter signature
(e.g., throttle<T extends (...args:any[])=>any>(delay: number, callback: T,
options?: /* options type */) : ReturnWithCancel<T>) and forward to
_throttle(delay, callback, options), ensuring the return type remains
ReturnWithCancel<T> and updating any related types; alternatively (if you prefer
minimal change) update the JSDoc `@param` entries to describe the rest parameter
signature and remove the mismatched names — but prefer the named-parameter
version for type safety and consistency with the documented params.
🧹 Nitpick comments (1)
src/equal.ts (1)
23-46: Consider documenting or handling non‑plain objects.
getTypeNameyields'date','map','set', etc., which currently fall back to reference equality. Consider adding explicit handling for these types or clarifying the limitation in docs to avoid surprises.
| * @template T - The type of items in the collection | ||
| */ | ||
| class PInstance<T = any> extends Promise<Awaited<T>[]> { | ||
| private promises = new Set<T | Promise<T>>() |
There was a problem hiding this comment.
add() de-dupes items because of Set.
Using Set drops duplicates (e.g., adding the same value twice results in one entry), which differs from Array semantics and can lose data. Consider an array unless deduplication is explicitly intended and documented.
🐛 Proposed fix (preserve duplicates)
- private promises = new Set<T | Promise<T>>()
+ private promises: Array<T | Promise<T>> = []
add(...args: (T | Promise<T>)[]) {
args.forEach((i) => {
- this.promises.add(i)
+ this.promises.push(i)
})
}
clear() {
- this.promises.clear()
+ this.promises.length = 0
}Also applies to: 66-69, 160-162
🤖 Prompt for AI Agents
In `@src/p.ts` at line 30, The field promises is a Set which silently deduplicates
entries and causes add() to lose duplicate values; change promises from Set<T |
Promise<T>> to an Array<T | Promise<T>> and update all usages (the add method,
any iterator/forEach logic, and any clear/flush logic currently interacting with
promises) to push items and iterate by index or for-of so duplicates are
preserved; ensure methods that previously relied on Set semantics (like deletion
or checking existence) are adjusted or replaced (e.g., use array.includes or
filter) and update any Promise handling logic that consumed promises to use the
new array shape.
| then<TResult1 = Awaited<T>[], TResult2 = never>( | ||
| onfulfilled?: ((value: Awaited<T>[]) => TResult1 | PromiseLike<TResult1>) | null, | ||
| onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null, | ||
| ) { | ||
| return this.promise.then(onfulfilled, onrejected) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
cat -n src/p.ts | sed -n '170,180p'Repository: Oluwasetemi/utils
Length of output: 178
🏁 Script executed:
# Look for Biome configuration files
fd -e biome -e json | grep -i biomeRepository: Oluwasetemi/utils
Length of output: 71
🏁 Script executed:
# Check for biome.json or similar config
git ls-files | grep -i biomeRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Try to list files in the repository
git ls-files src/p.tsRepository: Oluwasetemi/utils
Length of output: 69
🏁 Script executed:
# Try to read the biome.json configuration
cat biome.jsonRepository: Oluwasetemi/utils
Length of output: 1835
🏁 Script executed:
# Try to read the p.ts file with a different approach
head -n 180 src/p.ts | tail -n 20Repository: Oluwasetemi/utils
Length of output: 821
🏁 Script executed:
# Read more context to find the class name
head -n 200 src/p.ts | tail -n 100 | grep -B 30 "then<TResult1"Repository: Oluwasetemi/utils
Length of output: 1092
🏁 Script executed:
# Search for biome-ignore comments to verify the syntax
rg "biome-ignore" src/Repository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Get a broader view of the file structure around the method
sed -n '150,180p' src/p.tsRepository: Oluwasetemi/utils
Length of output: 1035
🏁 Script executed:
# Find the class definition
rg "^class|^export class" src/p.ts -A 2Repository: Oluwasetemi/utils
Length of output: 165
🏁 Script executed:
# Look for the beginning of the file to understand the class structure
head -n 100 src/p.tsRepository: Oluwasetemi/utils
Length of output: 2630
🏁 Script executed:
# Search for any existing biome-ignore comments in the entire codebase
rg "biome-ignore" --type ts --type tsxRepository: Oluwasetemi/utils
Length of output: 88
🏁 Script executed:
# Search for biome-ignore comments with correct syntax
rg "biome-ignore" src/Repository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Check if the suspicious rule noThenProperty is explicitly configured
rg "noThenProperty" src/ biome.jsonRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Get more of the file to see the full class definition and understand context
wc -l src/p.tsRepository: Oluwasetemi/utils
Length of output: 73
🏁 Script executed:
# Read the full relevant section around the then method
sed -n '165,185p' src/p.tsRepository: Oluwasetemi/utils
Length of output: 988
🏁 Script executed:
# Check the Biome version from the schema URL in biome.json
grep schema biome.jsonRepository: Oluwasetemi/utils
Length of output: 122
🏁 Script executed:
# Verify the catch method or any other thenable-related methods
rg "catch|Promise\.prototype\." src/p.tsRepository: Oluwasetemi/utils
Length of output: 145
🏁 Script executed:
# Check if there's a catch method as well to confirm thenable pattern
sed -n '185,195p' src/p.tsRepository: Oluwasetemi/utils
Length of output: 395
🏁 Script executed:
# Verify the exact Biome rule documentation by checking if there's any comment about it
rg "noThenProperty\|noProperty" src/Repository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Check package.json for Biome version
cat package.json | grep -A 2 -B 2 biomeRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Look for the finally method to confirm the full thenable pattern
rg "finally" src/p.ts -A 3Repository: Oluwasetemi/utils
Length of output: 132
🏁 Script executed:
# Check the exact biome.json configuration for suspicious rules
sed -n '/suspicious/,/}/p' biome.jsonRepository: Oluwasetemi/utils
Length of output: 296
🏁 Script executed:
# Search web documentation about Biome noThenProperty rule to understand the exact rule path
# Let's also check if there's any ESLint config that might inform us
fd -e eslintrc -e eslintignoreRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Verify the exact Biome rule name by checking Biome documentation references
# Let's look for any comments or documentation in the codebase
rg "then.*Promise\|thenable" src/p.tsRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Check if there are any existing lint suppressions in the file for reference
rg "^[\s]*//.*@" src/p.ts | head -5Repository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Final verification: Check the complete biome.json to see if there's any override for noThenProperty anywhere
cat biome.json | jq '.linter.rules'Repository: Oluwasetemi/utils
Length of output: 1274
🏁 Script executed:
# Search for any documentation or comments about the specific Biome rule syntax
# Let's verify by checking what Biome version documentation says about this rule
cat package.json | grep -i versionRepository: Oluwasetemi/utils
Length of output: 82
🏁 Script executed:
# Check if there's a .biomerc or other config file we missed
fd "biome" -t fRepository: Oluwasetemi/utils
Length of output: 71
🏁 Script executed:
# Double-check by looking at the suspicious rules list to confirm noThenProperty isn't mentioned
rg "noThenProperty" .Repository: Oluwasetemi/utils
Length of output: 43
Add Biome lint suppression for intentional thenable behavior.
The then method implements the thenable pattern for PInstance, which is intentional since the class extends Promise. Suppress the noThenProperty lint error:
Fix
+ // biome-ignore lint/suspicious/noThenProperty: PInstance is intentionally thenable
then<TResult1 = Awaited<T>[], TResult2 = never>(
onfulfilled?: ((value: Awaited<T>[]) => TResult1 | PromiseLike<TResult1>) | null,
onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null,
) {📝 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.
| then<TResult1 = Awaited<T>[], TResult2 = never>( | |
| onfulfilled?: ((value: Awaited<T>[]) => TResult1 | PromiseLike<TResult1>) | null, | |
| onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null, | |
| ) { | |
| return this.promise.then(onfulfilled, onrejected) | |
| } | |
| // biome-ignore lint/suspicious/noThenProperty: PInstance is intentionally thenable | |
| then<TResult1 = Awaited<T>[], TResult2 = never>( | |
| onfulfilled?: ((value: Awaited<T>[]) => TResult1 | PromiseLike<TResult1>) | null, | |
| onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null, | |
| ) { | |
| return this.promise.then(onfulfilled, onrejected) | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 173-173: Do not add then to a class.
(lint/suspicious/noThenProperty)
🤖 Prompt for AI Agents
In `@src/p.ts` around lines 173 - 178, Add a Biome lint suppression above the then
method to silence the noThenProperty rule for the intentional thenable
implementation: place a comment like "biome-ignore lint/noThenProperty:
intentional thenable for PInstance extending Promise" immediately above the
then(...) declaration in class PInstance so the linter knows this is deliberate.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/array.ts (2)
61-89: Fix JSDoc param name to match overload signature.
The first overload usesf1, but the JSDoc documentsfilters, which breaks doc tooling/linting. Align the param name and mention additional filters in prose.🔧 Proposed fix
- * `@param` filters - Filter functions to determine partitions + * `@param` f1 - First filter function (additional filters can be passed as extra arguments)
223-269: Guard against negative or zero step to avoid infinite loops.
Ifstepis negative whilestart < stop, the loop never terminates. Either enforce a positive step or support descending ranges.🛠️ Suggested fix
const arr: number[] = [] let current = start - while (current < stop) { - arr.push(current) - current += step || 1 - } + if (step === 0) + throw new Error('step must not be 0') + + if (step > 0) { + while (current < stop) { + arr.push(current) + current += step + } + } + else { + while (current > stop) { + arr.push(current) + current += step + } + }src/string.ts (1)
109-112: Fix falsy object values being treated as missing.
vars[key] || ...causes0,'',falseto fall back incorrectly. Check key existence instead.🐛 Proposed fix
- const vars = firstArg as Record<string, any> - return str.replace(/\{(\w+)\}/g, (_, key) => vars[key] || ((typeof fallback === 'function' ? fallback(key) : fallback) ?? key)) + const vars = firstArg as Record<string, any> + return str.replace(/\{(\w+)\}/g, (_, key) => { + if (Object.prototype.hasOwnProperty.call(vars, key)) + return vars[key] + const fb = typeof fallback === 'function' ? fallback(key) : fallback + return fb ?? key + })
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 43-45: The devDependencies entries in package.json are not
alphabetically sorted causing jsonc/sort-keys lint failures; reorder the keys so
that "@setemiojo/eslint-config" appears before "@vitest/coverage-v8" (maintain
exact package names and versions for "@antfu/ni", "@setemiojo/eslint-config",
and "@vitest/coverage-v8") to satisfy the linter and unblock CI.
In `@src/base.test.ts`:
- Around line 1-53: The test imports the global-shadowing function name toString
which triggers lint rule lint/suspicious/noShadowRestrictedNames; rename the
import in the import statement from toString to baseToString and update every
call site in this file (all uses of toString(...) in the describe('toString',
...) block) to call baseToString(...) instead so the global is not shadowed and
the tests still exercise the same function.
In `@src/is.test.ts`:
- Around line 45-48: The test triggers prefer-arrow-callback because of an
anonymous function expression; replace the inline anonymous function used in the
expect(isFunction(function () {})) call with a named function declaration (e.g.,
function nonArrow() {}) and pass that function by reference to isFunction so the
test still verifies non-arrow functions without violating the lint rule; update
the spec in the it block that calls isFunction to use the named function
reference and keep the other arrow/async checks unchanged.
In `@src/time.test.ts`:
- Around line 191-203: The tests use locale-specific strings ('PM' and '14')
which fail in non-English/Non-Latin locales; update the two formatTime tests to
be locale-agnostic by either calling formatTime with an explicit locale argument
(e.g., 'en-US' for 12h, 'en-GB' for 24h) if formatTime supports it, or by
parsing the formatted output for Unicode digits: extract the first numeric group
from formatted (use a Unicode-aware regex like /\p{Nd}+/u), convert to a number,
and assert that for the 12h test hour % 12 === 2 (or equals 2 when treating
12-hour clocks) and for the 24h test the parsed hour === 14; reference
formatTime and Temporal.PlainTime.from in the updated expectations.
In `@src/vendor.test.ts`:
- Around line 9-12: Tests that call vi.useFakeTimers() do not restore real
timers, causing timer state leakage; update the afterEach blocks that currently
call vi.restoreAllMocks() to also call vi.useRealTimers() (e.g., modify the
afterEach containing vi.restoreAllMocks() and the other afterEach around the
same area) so each test resets both mocks and timers; ensure the calls run in
every afterEach (keep vi.restoreAllMocks() and append vi.useRealTimers()) and do
not change the test order.
🧹 Nitpick comments (1)
src/time.test.ts (1)
528-571: Guard time‑zone tests against unsupported IANA zones.
Hard‑coded"America/New_York"/"America/Los_Angeles"can fail in environments with limited ICU data. Prefer selecting fromgetAvailableTimeZones()with a safe fallback like"UTC"to avoid flaky CI.🔧 Suggested portability tweak
-describe('convertToTimeZone', () => { +describe('convertToTimeZone', () => { + const [tz1, tz2] = getAvailableTimeZones() + const sourceTz = tz1 ?? 'UTC' + const targetTz = tz2 ?? sourceTz + it('should convert datetime to timezone', () => { const dt = Temporal.PlainDateTime.from('2023-01-15T10:00:00') - const result = convertToTimeZone(dt, 'America/New_York') + const result = convertToTimeZone(dt, targetTz) expect(result instanceof Temporal.ZonedDateTime).toBe(true) }) }) describe('convertFromTimeZone', () => { it('should convert zoned datetime to different timezone', () => { - const zdt = Temporal.ZonedDateTime.from('2023-01-15T10:00:00[America/Los_Angeles]') - const result = convertFromTimeZone(zdt, 'America/New_York') - expect(result.timeZoneId).toBe('America/New_York') + const zdt = Temporal.ZonedDateTime.from(`2023-01-15T10:00:00[${sourceTz}]`) + const result = convertFromTimeZone(zdt, targetTz) + expect(result.timeZoneId).toBe(targetTz) }) }) describe('nowInTimeZone', () => { it('should return current datetime in timezone', () => { - const result = nowInTimeZone('America/New_York') + const result = nowInTimeZone(targetTz) expect(result instanceof Temporal.ZonedDateTime).toBe(true) }) }) describe('todayInTimeZone', () => { it('should return today in timezone', () => { - const result = todayInTimeZone('America/New_York') + const result = todayInTimeZone(targetTz) expect(result instanceof Temporal.PlainDate).toBe(true) }) })
| import { describe, expect, it } from 'vitest' | ||
| import { assert, getTypeName, noop, toString } from './base' | ||
|
|
||
| describe('assert', () => { | ||
| it('should not throw when condition is true', () => { | ||
| expect(() => assert(true, 'should not throw')).not.toThrow() | ||
| }) | ||
|
|
||
| it('should throw when condition is false', () => { | ||
| expect(() => assert(false, 'error message')).toThrow('error message') | ||
| }) | ||
|
|
||
| it('should throw with the provided message', () => { | ||
| expect(() => assert(false, 'custom error')).toThrow('custom error') | ||
| }) | ||
| }) | ||
|
|
||
| describe('toString', () => { | ||
| it('should return [object Object] for plain objects', () => { | ||
| expect(toString({})).toBe('[object Object]') | ||
| }) | ||
|
|
||
| it('should return [object Array] for arrays', () => { | ||
| expect(toString([])).toBe('[object Array]') | ||
| }) | ||
|
|
||
| it('should return [object Null] for null', () => { | ||
| expect(toString(null)).toBe('[object Null]') | ||
| }) | ||
|
|
||
| it('should return [object Undefined] for undefined', () => { | ||
| expect(toString(undefined)).toBe('[object Undefined]') | ||
| }) | ||
|
|
||
| it('should return [object Number] for numbers', () => { | ||
| expect(toString(42)).toBe('[object Number]') | ||
| }) | ||
|
|
||
| it('should return [object String] for strings', () => { | ||
| expect(toString('hello')).toBe('[object String]') | ||
| }) | ||
|
|
||
| it('should return [object Function] for functions', () => { | ||
| expect(toString(() => {})).toBe('[object Function]') | ||
| }) | ||
|
|
||
| it('should return [object Date] for dates', () => { | ||
| expect(toString(new Date())).toBe('[object Date]') | ||
| }) | ||
|
|
||
| it('should return [object RegExp] for regular expressions', () => { | ||
| expect(toString(/test/)).toBe('[object RegExp]') | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and inspect its content
cd /tmp && find . -name "src" -type d 2>/dev/null | head -5
# Then check repository structure
git ls-files | grep -E "(base\.test|biome|\.biomeignore|biome\.json)" | head -20Repository: Oluwasetemi/utils
Length of output: 129
🏁 Script executed:
# Find and read the test file mentioned
git ls-files | grep "base.test.ts"Repository: Oluwasetemi/utils
Length of output: 77
🏁 Script executed:
# Check for biome config and lint the file
git ls-files | grep -E "biome\.json|\.biomeignore" && \
cat $(git ls-files | grep "biome\.json") 2>/dev/nullRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Read the base.test.ts file
if [ -f "src/base.test.ts" ]; then
cat -n "src/base.test.ts" | head -60
fiRepository: Oluwasetemi/utils
Length of output: 2235
🏁 Script executed:
# Check if biome is available and run lint on the file
if command -v biome &> /dev/null; then
biome lint src/base.test.ts 2>&1
else
echo "Biome not found in PATH"
fiRepository: Oluwasetemi/utils
Length of output: 778
🏁 Script executed:
# Count occurrences of toString in the test file
if [ -f "src/base.test.ts" ]; then
echo "=== Total occurrences of 'toString' in file ===" && \
grep -o "toString" "src/base.test.ts" | wc -l && \
echo "=== Lines with toString calls ===" && \
grep -n "toString" "src/base.test.ts"
fiRepository: Oluwasetemi/utils
Length of output: 740
Biome flags shadowing the global toString property—alias the import and update all call sites.
The lint error lint/suspicious/noShadowRestrictedNames must be resolved. Rename the import to baseToString and update all 9 function calls throughout the test file.
🐛 Proposed fix
-import { assert, getTypeName, noop, toString } from './base'
+import { assert, getTypeName, noop, toString as baseToString } from './base'
@@
- expect(toString({})).toBe('[object Object]')
+ expect(baseToString({})).toBe('[object Object]')
@@
- expect(toString([])).toBe('[object Array]')
+ expect(baseToString([])).toBe('[object Array]')
@@
- expect(toString(null)).toBe('[object Null]')
+ expect(baseToString(null)).toBe('[object Null]')
@@
- expect(toString(undefined)).toBe('[object Undefined]')
+ expect(baseToString(undefined)).toBe('[object Undefined]')
@@
- expect(toString(42)).toBe('[object Number]')
+ expect(baseToString(42)).toBe('[object Number]')
@@
- expect(toString('hello')).toBe('[object String]')
+ expect(baseToString('hello')).toBe('[object String]')
@@
- expect(toString(() => {})).toBe('[object Function]')
+ expect(baseToString(() => {})).toBe('[object Function]')
@@
- expect(toString(new Date())).toBe('[object Date]')
+ expect(baseToString(new Date())).toBe('[object Date]')
@@
- expect(toString(/test/)).toBe('[object RegExp]')
+ expect(baseToString(/test/)).toBe('[object RegExp]')📝 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.
| import { describe, expect, it } from 'vitest' | |
| import { assert, getTypeName, noop, toString } from './base' | |
| describe('assert', () => { | |
| it('should not throw when condition is true', () => { | |
| expect(() => assert(true, 'should not throw')).not.toThrow() | |
| }) | |
| it('should throw when condition is false', () => { | |
| expect(() => assert(false, 'error message')).toThrow('error message') | |
| }) | |
| it('should throw with the provided message', () => { | |
| expect(() => assert(false, 'custom error')).toThrow('custom error') | |
| }) | |
| }) | |
| describe('toString', () => { | |
| it('should return [object Object] for plain objects', () => { | |
| expect(toString({})).toBe('[object Object]') | |
| }) | |
| it('should return [object Array] for arrays', () => { | |
| expect(toString([])).toBe('[object Array]') | |
| }) | |
| it('should return [object Null] for null', () => { | |
| expect(toString(null)).toBe('[object Null]') | |
| }) | |
| it('should return [object Undefined] for undefined', () => { | |
| expect(toString(undefined)).toBe('[object Undefined]') | |
| }) | |
| it('should return [object Number] for numbers', () => { | |
| expect(toString(42)).toBe('[object Number]') | |
| }) | |
| it('should return [object String] for strings', () => { | |
| expect(toString('hello')).toBe('[object String]') | |
| }) | |
| it('should return [object Function] for functions', () => { | |
| expect(toString(() => {})).toBe('[object Function]') | |
| }) | |
| it('should return [object Date] for dates', () => { | |
| expect(toString(new Date())).toBe('[object Date]') | |
| }) | |
| it('should return [object RegExp] for regular expressions', () => { | |
| expect(toString(/test/)).toBe('[object RegExp]') | |
| }) | |
| import { assert, getTypeName, noop, toString as baseToString } from './base' | |
| describe('assert', () => { | |
| it('should not throw when condition is true', () => { | |
| expect(() => assert(true, 'should not throw')).not.toThrow() | |
| }) | |
| it('should throw when condition is false', () => { | |
| expect(() => assert(false, 'error message')).toThrow('error message') | |
| }) | |
| it('should throw with the provided message', () => { | |
| expect(() => assert(false, 'custom error')).toThrow('custom error') | |
| }) | |
| }) | |
| describe('toString', () => { | |
| it('should return [object Object] for plain objects', () => { | |
| expect(baseToString({})).toBe('[object Object]') | |
| }) | |
| it('should return [object Array] for arrays', () => { | |
| expect(baseToString([])).toBe('[object Array]') | |
| }) | |
| it('should return [object Null] for null', () => { | |
| expect(baseToString(null)).toBe('[object Null]') | |
| }) | |
| it('should return [object Undefined] for undefined', () => { | |
| expect(baseToString(undefined)).toBe('[object Undefined]') | |
| }) | |
| it('should return [object Number] for numbers', () => { | |
| expect(baseToString(42)).toBe('[object Number]') | |
| }) | |
| it('should return [object String] for strings', () => { | |
| expect(baseToString('hello')).toBe('[object String]') | |
| }) | |
| it('should return [object Function] for functions', () => { | |
| expect(baseToString(() => {})).toBe('[object Function]') | |
| }) | |
| it('should return [object Date] for dates', () => { | |
| expect(baseToString(new Date())).toBe('[object Date]') | |
| }) | |
| it('should return [object RegExp] for regular expressions', () => { | |
| expect(baseToString(/test/)).toBe('[object RegExp]') | |
| }) | |
| }) |
🧰 Tools
🪛 Biome (2.1.2)
[error] 2-2: Do not shadow the global "toString" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In `@src/base.test.ts` around lines 1 - 53, The test imports the global-shadowing
function name toString which triggers lint rule
lint/suspicious/noShadowRestrictedNames; rename the import in the import
statement from toString to baseToString and update every call site in this file
(all uses of toString(...) in the describe('toString', ...) block) to call
baseToString(...) instead so the global is not shadowed and the tests still
exercise the same function.
There was a problem hiding this comment.
@coderabbitai, I am using eslint why are you using biome lint rules to evaluate my project
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for ESLint and Biome configuration files
echo "=== Checking for linting configuration files ==="
git ls-files | grep -E "(eslint|biome)" | head -20
echo -e "\n=== Checking package.json for linting dependencies ==="
if [ -f "package.json" ]; then
cat package.json | jq -r '.devDependencies, .dependencies' | grep -E "(eslint|biome)" || echo "No eslint or biome found in dependencies"
fi
echo -e "\n=== Looking for ESLint config files ==="
fd -t f "eslint" --max-depth 2
echo -e "\n=== Looking for Biome config files ==="
fd -t f "biome" --max-depth 2
echo -e "\n=== Checking for .eslintrc* files ==="
ls -la | grep eslintrc || echo "No .eslintrc files found"
echo -e "\n=== Checking root directory files ==="
ls -la | grep -E "\.(json|js|ts|yml|yaml)$" | head -20
Length of output: 787
🏁 Script executed:
#!/bin/bash
# Check the ESLint configuration
echo "=== ESLint configuration ==="
cat eslint.config.mjs
Length of output: 247
@Oluwasetemi, you're absolutely right, and I apologize for the error. I can confirm that your repository is configured with ESLint (using @setemiojo/eslint-config version 5.4.3 with the flat config format in eslint.config.mjs), and there are no Biome configuration files present.
I incorrectly used Biome lint rules to evaluate the code when I should have used ESLint. This was a mistake on my part.
Would you like me to re-check the src/base.test.ts file using ESLint to see if there are any actual linting issues according to your ESLint configuration? The toString import shadowing concern may or may not be flagged by your ESLint setup depending on which rules are enabled in your config.
The repository Oluwasetemi/utils uses ESLint (not Biome) for linting, configured via eslint.config.mjs with @setemiojo/eslint-config.
| it('should return true for functions', () => { | ||
| expect(isFunction(() => {})).toBe(true) | ||
| expect(isFunction(function () {})).toBe(true) | ||
| expect(isFunction(async () => {})).toBe(true) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f "src/is.test.ts" ]; then
echo "=== File found ==="
wc -l src/is.test.ts
echo ""
echo "=== Lines 40-55 ==="
sed -n '40,55p' src/is.test.ts
else
echo "File not found at src/is.test.ts"
# Try to find it
find . -name "is.test.ts" -type f 2>/dev/null
fiRepository: Oluwasetemi/utils
Length of output: 178
🏁 Script executed:
# Look for ESLint configuration
echo "=== ESLint config files ==="
find . -maxdepth 2 -name ".eslintrc*" -o -name "eslint.config.*" 2>/dev/null | head -20Repository: Oluwasetemi/utils
Length of output: 108
🏁 Script executed:
# Check for package.json eslintConfig or similar
echo "=== Checking package.json for ESLint config ==="
if [ -f "package.json" ]; then
cat package.json | grep -A 20 "eslintConfig\|eslint"
fiRepository: Oluwasetemi/utils
Length of output: 837
🏁 Script executed:
# Read the specific test file section
echo "=== src/is.test.ts lines 40-55 ==="
sed -n '40,55p' src/is.test.tsRepository: Oluwasetemi/utils
Length of output: 516
🏁 Script executed:
# Read the ESLint config to understand the rules
echo "=== ESLint configuration ==="
cat eslint.config.mjsRepository: Oluwasetemi/utils
Length of output: 308
Fix prefer-arrow-callback lint failure at line 47.
The anonymous function expression triggers the prefer-arrow-callback rule. Convert it to a named function declaration and pass by reference, which is an exception to the rule while preserving the test for non-arrow functions.
Proposed fix
it('should return true for functions', () => {
expect(isFunction(() => {})).toBe(true)
- expect(isFunction(function () {})).toBe(true)
+ function regularFn() {}
+ expect(isFunction(regularFn)).toBe(true)
expect(isFunction(async () => {})).toBe(true)
})📝 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('should return true for functions', () => { | |
| expect(isFunction(() => {})).toBe(true) | |
| expect(isFunction(function () {})).toBe(true) | |
| expect(isFunction(async () => {})).toBe(true) | |
| it('should return true for functions', () => { | |
| expect(isFunction(() => {})).toBe(true) | |
| function regularFn() {} | |
| expect(isFunction(regularFn)).toBe(true) | |
| expect(isFunction(async () => {})).toBe(true) | |
| }) |
🧰 Tools
🪛 ESLint
[error] 47-47: Unexpected function expression.
(prefer-arrow-callback)
🪛 GitHub Check: lint
[failure] 47-47:
Unexpected function expression
🤖 Prompt for AI Agents
In `@src/is.test.ts` around lines 45 - 48, The test triggers prefer-arrow-callback
because of an anonymous function expression; replace the inline anonymous
function used in the expect(isFunction(function () {})) call with a named
function declaration (e.g., function nonArrow() {}) and pass that function by
reference to isFunction so the test still verifies non-arrow functions without
violating the lint rule; update the spec in the it block that calls isFunction
to use the named function reference and keep the other arrow/async checks
unchanged.
| describe('formatTime', () => { | ||
| it('should format time in 12h format', () => { | ||
| const time = Temporal.PlainTime.from('14:30:00') | ||
| const formatted = formatTime(time, '12h') | ||
| expect(formatted).toContain('PM') | ||
| }) | ||
|
|
||
| it('should format time in 24h format', () => { | ||
| const time = Temporal.PlainTime.from('14:30:00') | ||
| const formatted = formatTime(time, '24h') | ||
| expect(formatted).toContain('14') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the formatTime function definition
rg -t ts 'function formatTime|export.*formatTime' --max-count=5Repository: Oluwasetemi/utils
Length of output: 171
🏁 Script executed:
# Check the test file structure
cat -n src/time.test.ts | sed -n '185,210p'Repository: Oluwasetemi/utils
Length of output: 1038
🏁 Script executed:
# Look for the time module/file that exports formatTime
fd -t ts 'time\.' | head -20Repository: Oluwasetemi/utils
Length of output: 230
🏁 Script executed:
# Read the formatTime implementation
cat -n src/time.ts | grep -A 20 'function formatTime'Repository: Oluwasetemi/utils
Length of output: 1105
🏁 Script executed:
# Look at the entire time.ts file to understand formatTime better
wc -l src/time.tsRepository: Oluwasetemi/utils
Length of output: 77
🏁 Script executed:
# Check if formatTime is used elsewhere in tests or if there are other time formatting tests
rg -A 3 'formatTime' src/time.test.tsRepository: Oluwasetemi/utils
Length of output: 468
🏁 Script executed:
# Check what Temporal.PlainTime.toLocaleString returns for different locales
# to confirm the locale-sensitivity issue
python3 << 'EOF'
# Simulate what toLocaleString would do with Intl API
# In English locale: 14:30 with 12h format = "2:30 PM"
# In different locales, the "PM" part changes significantly
examples = {
"en-US": "2:30 PM", # English: "PM"
"es-ES": "14:30", # Spanish might differ
"fr-FR": "14:30", # French would use 24h or different format
"ar-SA": "٢:٣٠ م", # Arabic uses different numerals and indicators
"de-DE": "14:30", # German typically uses 24h by default
}
print("Locale-dependent formatting examples for 14:30:")
for locale, output in examples.items():
print(f" {locale}: {output}")
print("\nKey insight: The test assertion 'expect(formatted).toContain(\"PM\")' ")
print("will FAIL in non-English locales where PM is expressed differently or not at all.")
EOFRepository: Oluwasetemi/utils
Length of output: 338
🏁 Script executed:
# Check if there's any test setup that configures locale
rg -B 5 'describe.*formatTime' src/time.test.ts | head -15Repository: Oluwasetemi/utils
Length of output: 194
🏁 Script executed:
# Check if there are any locale setup or environment variables in the test file
rg -i 'locale|intl|environment' src/time.test.ts | head -10Repository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Check package.json or test setup files for locale configuration
find . -maxdepth 2 -name 'jest.config.*' -o -name 'vitest.config.*' -o -name 'setup.*' | head -5Repository: Oluwasetemi/utils
Length of output: 79
🏁 Script executed:
# Verify the actual Temporal behavior with toLocaleString
python3 << 'EOF'
import json
# According to ECMAScript Intl spec, toLocaleString behavior:
# When hour12 is true and locale is "en-US": uses AM/PM notation
# When hour12 is true and locale is non-English: behavior varies
# - Some locales ignore hour12 and use 24h by default
# - Some use different AM/PM notation (e.g., "a.m."/"p.m.")
# - Some may use locale-specific time indicators
test_cases = {
"en-US with hour12=true": {
"input": "14:30",
"expected": "2:30 PM",
"contains_PM": True
},
"de-DE with hour12=true": {
"input": "14:30",
"expected": "14:30", # German ignores hour12 in many implementations
"contains_PM": False
},
"ja-JP with hour12=true": {
"input": "14:30",
"expected": "午後2:30", # Japanese uses different indicator
"contains_PM": False
},
"ar-SA with hour12=true": {
"input": "14:30",
"expected": "٢:٣٠ م", # Arabic numerals and notation
"contains_PM": False
}
}
print("Risk assessment for 'expect(formatted).toContain(\"PM\")':")
print(json.dumps(test_cases, indent=2, ensure_ascii=False))
print("\nConclusion: Test is locale-fragile and will fail in non-English environments.")
EOFRepository: Oluwasetemi/utils
Length of output: 630
🏁 Script executed:
# Check if there's a test that already uses different approaches for locale-aware testing
rg 'toLocaleString|Intl\.' src/time.test.tsRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Check vitest configuration for locale settings
cat vitest.config.tsRepository: Oluwasetemi/utils
Length of output: 542
🏁 Script executed:
# Check if there's any global setup file that configures locale
rg -l 'beforeAll|beforeEach|setup' src/time.test.tsRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Verify what the actual runtime behavior is - check if tests pass in CI
git log --oneline vitest.config.ts 2>/dev/null | head -5 || echo "No git history available"Repository: Oluwasetemi/utils
Length of output: 103
🏁 Script executed:
# Check if there are any CI/GitHub Actions workflows that set locale
find . -path '*/.github/workflows/*.yml' -o -path '*/.github/workflows/*.yaml' | head -3Repository: Oluwasetemi/utils
Length of output: 197
🏁 Script executed:
# Verify the actual test output to see if it's currently passing or failing
rg -A 1 'should format time in' src/time.test.tsRepository: Oluwasetemi/utils
Length of output: 267
🏁 Script executed:
# Double-check the actual formatting behavior described in Intl spec
python3 << 'EOF'
print("Intl.DateTimeFormat behavior with hour12 option:")
print("\nWith hour12: true (12h format):")
print(" en-US: '2:30 PM' ← contains 'PM'")
print(" de-DE: '14:30' ← ignores hour12, uses 24h")
print(" fr-FR: '14:30' ← typically uses 24h")
print(" ja-JP: '午後2:30' ← contains PM equivalent in Japanese")
print(" ar-SA: '٢:٣٠ م' ← uses Arabic numerals + notation")
print("\nWith hour12: false (24h format):")
print(" en-US: '14:30' ← contains '14' (Latin numerals)")
print(" ar-SA: '١٤:٣٠' ← uses Arabic numerals, NOT Latin '14'")
print("\n✓ Test assertions ARE locale-fragile and unreliable")
EOFRepository: Oluwasetemi/utils
Length of output: 589
🏁 Script executed:
# Check the CI workflow to see if locale is explicitly set
cat .github/workflows/ci.ymlRepository: Oluwasetemi/utils
Length of output: 1647
Replace locale-specific assertions with locale-agnostic format validation.
The 'PM' and '14' expectations fail in non-English locales or those with non-Latin numerals (e.g., Arabic uses ٢:٣٠ م instead of 2:30 PM, and ١٤:٣٠ instead of 14:30). Either explicitly set the test locale or verify format behavior without hardcoding locale-specific strings.
Suggested fix
describe('formatTime', () => {
- it('should format time in 12h format', () => {
- const time = Temporal.PlainTime.from('14:30:00')
- const formatted = formatTime(time, '12h')
- expect(formatted).toContain('PM')
- })
-
- it('should format time in 24h format', () => {
- const time = Temporal.PlainTime.from('14:30:00')
- const formatted = formatTime(time, '24h')
- expect(formatted).toContain('14')
- })
+ it('should format time in 12h vs 24h formats', () => {
+ const time = Temporal.PlainTime.from('14:30:00')
+ const formatted12 = formatTime(time, '12h')
+ const formatted24 = formatTime(time, '24h')
+ expect(typeof formatted12).toBe('string')
+ expect(typeof formatted24).toBe('string')
+ expect(formatted12).not.toBe(formatted24)
+ })
})📝 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.
| describe('formatTime', () => { | |
| it('should format time in 12h format', () => { | |
| const time = Temporal.PlainTime.from('14:30:00') | |
| const formatted = formatTime(time, '12h') | |
| expect(formatted).toContain('PM') | |
| }) | |
| it('should format time in 24h format', () => { | |
| const time = Temporal.PlainTime.from('14:30:00') | |
| const formatted = formatTime(time, '24h') | |
| expect(formatted).toContain('14') | |
| }) | |
| }) | |
| describe('formatTime', () => { | |
| it('should format time in 12h vs 24h formats', () => { | |
| const time = Temporal.PlainTime.from('14:30:00') | |
| const formatted12 = formatTime(time, '12h') | |
| const formatted24 = formatTime(time, '24h') | |
| expect(typeof formatted12).toBe('string') | |
| expect(typeof formatted24).toBe('string') | |
| expect(formatted12).not.toBe(formatted24) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In `@src/time.test.ts` around lines 191 - 203, The tests use locale-specific
strings ('PM' and '14') which fail in non-English/Non-Latin locales; update the
two formatTime tests to be locale-agnostic by either calling formatTime with an
explicit locale argument (e.g., 'en-US' for 12h, 'en-GB' for 24h) if formatTime
supports it, or by parsing the formatted output for Unicode digits: extract the
first numeric group from formatted (use a Unicode-aware regex like /\p{Nd}+/u),
convert to a number, and assert that for the 12h test hour % 12 === 2 (or equals
2 when treating 12-hour clocks) and for the 24h test the parsed hour === 14;
reference formatTime and Temporal.PlainTime.from in the updated expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/array.ts`:
- Around line 351-353: The sample function produces a biased selection because
it uses Math.round(Math.random() * (arr.length - 1)); change the index
calculation to use Math.floor(Math.random() * arr.length) to get a uniform
distribution across all indices; update the sample<T>(arr, quantity)
implementation to compute each index with Math.floor(Math.random() * arr.length)
(and optionally guard for empty arrays by returning [] or throwing if arr.length
=== 0) so selection is unbiased.
🧹 Nitpick comments (1)
src/array.ts (1)
396-396: Consider simplifying the loop structure for clarity.The loop
for (let i = array.length; i--; i >= 0)works correctly, buti >= 0in the update position is never used (the decrement happens in the condition). This unconventional pattern may confuse readers.♻️ Suggested simplification
- for (let i = array.length; i--; i >= 0) { + for (let i = array.length - 1; i >= 0; i--) {
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/array.ts (1)
276-293: Guard against negativestepto prevent infinite loops.If callers pass a negative
stepwithstart < stop(e.g.,range(0, 10, -1)), the loop never terminates becausecurrentdecreases while the condition stays true. Please validatestepand/or handle descending ranges explicitly.🛠️ Proposed fix
export function range(...args: any): number[] { let start: number, stop: number, step: number @@ else { ([start, stop, step = 1] = args) } + if (step === 0) + return [] + const arr: number[] = [] let current = start - while (current < stop) { + const increasing = step > 0 + while (increasing ? current < stop : current > stop) { arr.push(current) - current += step || 1 + current += step } return arr }
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/array.ts (1)
214-247: Fix overload return type to reflect actual implementation.The second overload of
atdeclares return typeTbut the implementation returnsT | undefinedfor out-of-range indices. This contradicts both the documentation example (at([1, 2, 3], 5) // undefined) and the runtime behavior (lines 240-241 and 246).🔧 Proposed fix
-export function at<T>(array: readonly T[], index: number): T +export function at<T>(array: readonly T[], index: number): T | undefined export function at<T>(array: readonly T[] | [], index: number): T | undefined { const len = array.lengthNote: The
lastfunction (line 184) callsatand has the same overload issue.
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 29-33: The package.json key order is failing jsonc/sort-keys
linting because "engines" appears before "files"; reorder the keys so "files"
comes before "engines" (i.e., move the "files": ["dist"] entry above the
"engines": {"node": ">=20"} entry) to satisfy the linter and resolve the CI
failure.
In `@src/array.ts`:
- Around line 162-185: The overloads for last are unsafe because readonly T[]
can be empty but the overload returns T; change the overloads to (1) add a
non-empty tuple overload export function last<T>(array: readonly [T, ...T[]]): T
and (2) widen the general array overload to export function last<T>(array:
readonly T[]): T | undefined, leaving the implementation export function
last<T>(array: readonly T[]): T | undefined { return at(array, -1) } unchanged
so callers with non-empty tuples get T while general arrays get T | undefined.
| /** | ||
| * Get last item | ||
| * Get the last item of an empty array. | ||
| * | ||
| * @category Array | ||
| * @param array - An empty array | ||
| */ | ||
| export function last(array: readonly []): undefined | ||
| /** | ||
| * Get the last item of an array. | ||
| * | ||
| * @category Array | ||
| * @param array - The array to get the last item from | ||
| * @returns The last item | ||
| * @example | ||
| * ```ts | ||
| * last([1, 2, 3]) // 3 | ||
| * last(['a', 'b']) // 'b' | ||
| * last([]) // undefined | ||
| * ``` | ||
| */ | ||
| export function last<T>(array: readonly T[]): T | ||
| export function last<T>(array: readonly T[]): T | undefined { | ||
| return at(array, -1) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/array.ts | sed -n '162,185p'Repository: Oluwasetemi/utils
Length of output: 768
🏁 Script executed:
# Find the `at` function to understand its behavior
rg -A10 "^export function at" src/array.ts | head -30Repository: Oluwasetemi/utils
Length of output: 668
🏁 Script executed:
# Search for call sites of the `last` function
rg -n "\.last\(" src/ --type=ts
rg -n "last\(" src/ --type=ts | grep -v "export function last" | grep -v "@" | head -20Repository: Oluwasetemi/utils
Length of output: 187
🏁 Script executed:
# Check if there are any type tests or unit tests that might reveal expectations
fd -e test.ts -e spec.ts src/ | xargs grep -l "last" | head -5Repository: Oluwasetemi/utils
Length of output: 382
🏁 Script executed:
# Try to find test files with correct fd syntax
fd -e 'test.ts' . | head -10
fd -e 'spec.ts' . | head -10Repository: Oluwasetemi/utils
Length of output: 238
🏁 Script executed:
# Search more thoroughly for last function usages (excluding comments and overloads)
rg "last\(" src/ --type=ts -B1 -A1 | grep -v "export function" | grep -v "^--$" | head -30Repository: Oluwasetemi/utils
Length of output: 253
🏁 Script executed:
# Check for any tests in the repository
find . -name "*.test.ts" -o -name "*.spec.ts" 2>/dev/null | head -10Repository: Oluwasetemi/utils
Length of output: 262
🏁 Script executed:
# Check the array test file for `last` function tests
grep -n "last" src/array.test.ts -B2 -A5Repository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Also check if there's any TypeScript strict mode or other type checking config
find . -maxdepth 2 -name "tsconfig.json" -o -name "vitest.config.ts" -o -name "jest.config.ts" | head -5
cat tsconfig.json 2>/dev/null | head -30Repository: Oluwasetemi/utils
Length of output: 367
🏁 Script executed:
# Check the array test file directly
cat src/array.test.ts | head -100Repository: Oluwasetemi/utils
Length of output: 1985
🏁 Script executed:
# Search for any test that includes "last"
rg "last" src/array.test.ts -iRepository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Search for `last` in the entire src directory to see if it's used internally
rg "last\(" src/ --type=ts | grep -v "test.ts" | grep -v "export function last"Repository: Oluwasetemi/utils
Length of output: 175
🏁 Script executed:
# Check if `last` is exported from the main index
rg "last" src/index.ts -B1 -A1Repository: Oluwasetemi/utils
Length of output: 43
🏁 Script executed:
# Verify the actual function signatures more carefully
cat -n src/array.ts | sed -n '155,190p'Repository: Oluwasetemi/utils
Length of output: 1108
Add non-empty tuple overload and widen readonly T[] return type.
The second overload on line 182 promises T for readonly T[] arrays, but this type includes empty arrays that actually return undefined (as shown in the examples). With strict null checking enabled, this violates type safety.
Proposed fix
export function last(array: readonly []): undefined
-export function last<T>(array: readonly T[]): T
+export function last<T>(array: readonly [T, ...T[]]): T
+export function last<T>(array: readonly T[]): T | undefined
export function last<T>(array: readonly T[]): T | undefined {
return at(array, -1)
}🤖 Prompt for AI Agents
In `@src/array.ts` around lines 162 - 185, The overloads for last are unsafe
because readonly T[] can be empty but the overload returns T; change the
overloads to (1) add a non-empty tuple overload export function last<T>(array:
readonly [T, ...T[]]): T and (2) widen the general array overload to export
function last<T>(array: readonly T[]): T | undefined, leaving the implementation
export function last<T>(array: readonly T[]): T | undefined { return at(array,
-1) } unchanged so callers with non-empty tuples get T while general arrays get
T | undefined.
2 participants
ObjectOmitfunction to the objectSummary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.