feat(react-native): put registry refresh on the models namespace - #720
feat(react-native): put registry refresh on the models namespace#720ayaangazali wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe React Native models API adds ChangesReact Native model refresh
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This adds the React Native models.refresh API by forwarding to existing registry-refresh behavior, with no actionable merge-blocking risk remaining after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description gives a detailed explanation of the problem, implementation, and verification results. However, it does not use the repository template and omits the required Type of Change, platform-specific testing, labels, checklist, and screenshots sections. Resolution Update the description to include the template sections. Mark the change as a new feature, record applicable React Native iOS and Android testing, add the React Native SDK label, complete the code and review checklist, and state whether screenshots are not applicable. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Correction to one line in the description. I wrote that React Native is the only SDK still missing this; that is not quite right, and I would rather say so than have you find it. Checking the remaining bindings after opening this: Electron has it as The claim this PR actually rests on is unchanged: RN is the only SDK where the capability is fully wired natively and unreachable from the public API. |
2f14237 to
c3fd3bf
Compare
|
Rebased onto Still reproduces on One nuance I confirmed since opening this, in case it looks like an inconsistency: Web deliberately does not have Happy to rework it if you would rather this verb not spread further. |
c3fd3bf to
8bda9f8
Compare
RunanywhereAI#605 aligned the public API across all eight SDKs and gave Swift `models.refresh(rescanLocal:includeRemoteCatalog:pruneOrphans:)`. Kotlin and Flutter were missed and RunanywhereAI#712 fixed those two. React Native is the one left. The whole native path already exists on RN: the Nitro spec declares `refreshModelRegistry`, the C++ HybridObject encodes the three flags into a ModelRegistryRefreshRequest, and `RunAnywhere+ModelRegistry.ts` wraps it with the same defaults Swift uses. Nothing surfaces it. `index.ts` exports only the `RunAnywhere` object, that object exposes namespaces, and `models` has no refresh, so there is no public way to reconcile the registry on RN. `models.list()` reads the registry as it stands and never rescans, so an artifact that changed on disk out from under the SDK stays wrong until the process restarts. Forwards to the existing extension function rather than reimplementing it, so the non-throwing best-effort behaviour and the defaults stay in one place.
8bda9f8 to
f08f731
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
What is wrong
React Native has no public way to refresh the model registry.
#605aligned the public API across all eight SDKs and gave Swiftmodels.refresh(rescanLocal:includeRemoteCatalog:pruneOrphans:)(ModelsNamespace.swift:389). Kotlin and Flutter were missed;#712closed those two yesterday. RN is the one left.Evidence
Every piece of the native path is already there on RN:
specs/RunAnywhereCore.nitro.ts:274declaresrefreshModelRegistry(includeRemoteCatalog, rescanLocal, pruneOrphans)cpp/HybridRunAnywhereCore+Registry.cpp:443encodes those three flags into aModelRegistryRefreshRequest(fields 1/2/3, matchingidl/model_types.proto:411) and runs the same discovery pre-pass Swift doesPublic/Extensions/Models/RunAnywhere+ModelRegistry.ts:1046wraps it with the same defaults Swift uses, and its own comment says "Matches Swift"Nothing surfaces it.
src/index.ts:13exports only theRunAnywhereobject, that object is namespaces plusinitialize/reset/capabilities, andmodels(Public/Api/Models.ts:194) has norefresh.storage(Public/Api/Platform.ts:25) has no refresh either, which is where the Web SDK put its equivalent.So
refreshModelRegistryis reachable from inside the package and from nowhere else.This is not a documented omission:
capabilities()lists the things RN deliberately does not ship (agents,wakeword,realtime) with a reason attached to each. Registry refresh is not among them.The practical effect is that
models.list()reads the registry as it stands and never rescans, so an artifact that changed on disk out from under the SDK stays wrong for the life of the process.What this does
Adds
models.refresh(options?), forwarding to the existing extension function. Forwarding rather than reimplementing keeps the non-throwing best-effort behaviour and the three defaults in one place, and matches how the rest of the namespace delegates.Placed after
state()to match Swift's ordering.Verification
Ran on this branch:
tsc --noEmitfor@runanywhere/core: cleantsc -p tsconfig.test.json --noEmit: cleaneslint src/Public/Api/Models.ts --max-warnings 0: cleanjestfor@runanywhere/core: 75 passed, 15 suites, 0 failedThe two typechecks needed the full generated proto tree, so I ran
idl/codegen/generate_ts.sh,generate_ts_convenience.py,generate_defaults_pool.pyandgenerate_streams.shfirst. None of that output is committed here;git statusshows only the one file.Also compiled a scratch caller against the public entry point to confirm the verb is actually reachable and the option shapes are right:
No test added: this is a one-line delegation to a function that already has its behaviour defined elsewhere, and a test here would assert that a forwarder forwards.
I checked the C++ flag ordering against the proto field numbers while I was in there, in case the two adjacent booleans had been swapped across the bridge. They line up correctly.
Summary by CodeRabbit