Allow indexed access for non-NSArray collections

[adrian-niculescu]

Right now obj[i] from JS only works when the native object is an NSArray, because the indexed getter checks isKindOfClass:[NSArray class]. Plenty of other ordered collections are just as indexable but get left out. The one that bit us was PHFetchResult from the Photos framework.

This swaps that NSArray check in the getter for a capability check: if the target responds to both count and objectAtIndex:, treat it as indexable. NSArray keeps behaving exactly like before, and things like PHFetchResult and NSOrderedSet start working. The bounds check is unchanged, so out of range still returns nothing.

A few notes:

• I left the setter alone. It still only takes NSMutableArray. The collections this helps (PHFetchResult and friends) are read only, so there was no reason to relax writes and risk mutating something that isn't meant to be mutated.
• Requiring both selectors keeps it tight. NSSet and NSDictionary respond to count but not objectAtIndex:, so they're unaffected, and NSPointerArray uses pointerAtIndex: rather than objectAtIndex:, so it gets skipped instead of crashing.
• We've shipped this in our downstream fork for a while with no issues.

Happy to add a TestRunner case if you want one in the PR. NSOrderedSet is an easy one since it needs no permissions.

Summary by CodeRabbit

• Bug Fixes
  • Broadened array-style indexing support: native Objective-C objects that provide standard count/objectAtIndex accessors can now be accessed using bracket notation, with proper bounds handling.
• Tests
  • Added a test verifying bracket/index access works for non-NSArray indexable collections and correctly returns values or undefined for out-of-range indices.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 972d93e1-757e-41d9-b89d-0b38ff92cbbb

📥 Commits

Reviewing files that changed from the base of the PR and between c210aa5 and 0bd3978.

📒 Files selected for processing (1)

• TestRunner/app/tests/ApiTests.js

---

📝 Walkthrough

Walkthrough

ArgConverter::IndexedPropertyGetterCallback now treats any Objective-C object that responds to count and objectAtIndex: as indexable, using those selectors for bounds-checking and element retrieval; a Jasmine test verifies bracket/index access for an NSOrderedSet.

Changes

Duck-typed collection indexing

Layer / File(s)	Summary
Selector-based indexability validation and generic element access NativeScript/runtime/ArgConverter.mm, TestRunner/app/tests/ApiTests.js	IndexedPropertyGetterCallback replaces the NSArray-only check with respondsToSelector:count and respondsToSelector:objectAtIndex:, bounds-checks via the target's count, retrieves elements with objectAtIndex:, and adds a test ensuring indexed access works for NSOrderedSet (including out-of-range behavior).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped where indexes roam and dwell,
Count and objectAtIndex now tell,
Arrays no longer hold the gate,
Ordered sets can participate,
A tiny change, a hopping spell.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: allowing indexed access for non-NSArray collections, which is exactly what the PR accomplishes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

NativeScript/runtime/ArgConverter.mm (1)

886-897: ⚡ Quick win

Add regression tests for new duck-typed indexable collections.

Line 886 introduces a broader capability check (count + objectAtIndex:), but this behavior change is not protected by tests in this PR. Please add a TestRunner case that verifies obj[i] on at least NSOrderedSet (and ideally another non-NSArray type) including out-of-range behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/ArgConverter.mm` around lines 886 - 897, Add regression
tests exercising the new duck-typed indexable behavior introduced in
ArgConverter.mm (the count + objectAtIndex: check) by adding a TestRunner case
that (1) creates an NSOrderedSet and at least one other non-NSArray indexable
(e.g., PHFetchResult / custom object that implements count and objectAtIndex:),
(2) verifies that Objective-C indexing via obj[i] returns the same elements as
NSArray-style access for valid indices, and (3) verifies out-of-range behavior
(confirm it returns nil or the same error/behavior as NSArray in the runtime)
for indices >= count; place assertions in the TestRunner so this new behavior is
covered by the test suite.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@NativeScript/runtime/ArgConverter.mm`:
- Around line 886-897: Add regression tests exercising the new duck-typed
indexable behavior introduced in ArgConverter.mm (the count + objectAtIndex:
check) by adding a TestRunner case that (1) creates an NSOrderedSet and at least
one other non-NSArray indexable (e.g., PHFetchResult / custom object that
implements count and objectAtIndex:), (2) verifies that Objective-C indexing via
obj[i] returns the same elements as NSArray-style access for valid indices, and
(3) verifies out-of-range behavior (confirm it returns nil or the same
error/behavior as NSArray in the runtime) for indices >= count; place assertions
in the TestRunner so this new behavior is covered by the test suite.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a76db5ad-c7e4-4afe-8a22-8c1226e52930

📥 Commits

Reviewing files that changed from the base of the PR and between 05224d3 and c210aa5.

📒 Files selected for processing (1)

• NativeScript/runtime/ArgConverter.mm

[adrian-niculescu]

@coderabbitai Good call. Added a TestRunner regression case in ApiTests.js ("indexed access works for non-NSArray indexable collections") that builds an NSOrderedSet (not an NSArray, but responds to count + objectAtIndex:), verifies set[i] resolves the same elements as objectAtIndex(i) for valid indices, and asserts out-of-range access returns undefined. NSOrderedSet is the cleanest non-NSArray indexable to cover here since it needs no extra native fixture.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@adrian-niculescu That's a clean and self-contained test — using NSOrderedSet is exactly the right pick here since it satisfies the count + objectAtIndex: capability check, requires zero native scaffolding, and is clearly distinct from NSArray. Covering both the in-bounds path (parity with objectAtIndex:) and the out-of-bounds path (undefined) gives solid regression coverage for the new behaviour. Thanks for adding it!

🐇 ✨