fix(electron): use the shared error-category table instead of a second one - #779
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe Electron error bindings now re-export ChangesError category mapping
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The shared error-category mapping is now used consistently by Electron, with no unresolved merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
This one is not mine, so per the rule I am replying with evidence rather than pushing a fix. The failing step is "Swift distribution repo (runanywhere-swift) is cut at this release", i.e. It fails identically on a clean checkout of the default branch with none of my changes: The timing is what decides it, not the diff.
The repo already documents this as expected.
and For completeness, my diff is a single TypeScript file, Clearing it is the maintainer step the gate prints: cut the |
|
Also restructured the description onto Filled in truthfully rather than ticking boxes:
Not something to fix inside this PR, and I am not going to open an issue for it unprompted. Flagging it here since it is the direct cause of two of the unticked sections above, and say the word if you would like a separate docs PR adding the Electron rows. |
dae49b8 to
854971b
Compare
|
Update: this clears itself now, and I have rebased to trigger it. When I looked at this the first time, So the red on this PR was a stale result: that Rebased onto |
|
Still reproduces on today's I parsed the range rules out of both functions in source order and compared them across codes 1..999. The local copy has 9 rules, the shared one has 21, and they disagree on 350 codes: Those are not hypothetical ranges. Cross-referencing So an Electron caller branching on Branch merges clean into current |
…d one errors.ts declared its own categoryForCode, a port of the narrower commons range table. proto-ts already exports the canonical one and names Electron as a consumer: "Canonical ErrorCode / rac_result_t -> ErrorCategory range table for every TypeScript SDK (Web, React Native, Electron) ... Do not invent a second table in an SDK." The two disagree on 560 codes in 0..1100. Electron collapsed the auth (330-349), archive (350-369), calibration (370-379), module (400-499), platform (500-599) and backend (600-699) bands to INTERNAL, so the same error carried a different category on Electron than on Web and React Native. Import the shared table and re-export it. This is the same rule the file already applies one level up to the enums themselves: re-export, never re-declare.
854971b to
ecc6149
Compare
|
Still reproduces on
Electron's That is not cosmetic, because those codes exist. Comparing the two tables band by band, Electron mis-categorizes every defined code from 330 up: So the same failure reported through Electron and through Web gets a different Worth noting what this does not change: the local table is fallback-only, consulted when the wire supplies no category. Commons does supply one on the paths that produce it, and those are unaffected. This matters for the paths where Electron constructs an Merges clean, CI green. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
That would be great, thanks for flagging it -- the label set really doesn't have an Electron entry anywhere yet (only ios-sdk/kotlin-sdk/web-sdk/flutter-sdk/rn-sdk/core exist today). A separate docs PR adding Electron to the template's platform matrix and the label list would be welcome. Reviewed with help from Claude Code and Codex. |
That makes sense -- thanks for laying out the evidence. We read both range tables side by side and confirm the same 28-code divergence across 330-699; this is a real behavior fix, not cosmetic dedup, and it's why we're merging on that basis. Treating this as addressed. Reviewed with help from Claude Code and Codex. |
Confirmed on our end too -- Reviewed with help from Claude Code and Codex. |
sanchitmonga22
left a comment
There was a problem hiding this comment.
Thanks a lot for this, @ayaangazali! Electron's fallback categories for 28 real error codes (keychain -> AUTH, checksum -> IO, module/backend -> COMPONENT, adapter -> CONFIGURATION, and more) now match Web and React Native instead of collapsing to INTERNAL, and the duplicate table is gone.
Checked: CodeRabbit reviewed the latest commit · CI green · built and linted locally merged into main (electron, TS-only change) · two independent code reviews.
Follow-ups, not blocking: #880 (Electron's two native error paths, and the other SDKs, still disagree with each other and with this table for codes 330-699) -- you're welcome to pick it up if you're interested.
Merging now -- really appreciate the contribution, and thanks for the patient, well-evidenced follow-ups while this waited for a look!
Reviewed with help from Claude Code and Codex.
Description
What is wrong
bindings/proto-ts/src/convenience/errors_category.tsopens by naming its own scope:bindings/electron/src/errors.ts:70invents a second table. Web and React Native bothimport { categoryForCode } from '@runanywhere/proto-ts/convenience/errors_category'; Electron declares its own, described as a port of the commons range table.The same file already argues against this one level up, about the enums:
The category table then does the collapsing instead.
The divergence
Evaluating both tables over codes 0..1100, they disagree on 560 of them:
So an Electron consumer branching on
e.categorysees INTERNAL where the identical code gives AUTH or CONFIGURATION on Web and React Native.This is not the commons table being wrong, and the shared file says so explicitly:
Electron's copy is that deferred under-mapping, re-introduced in the SDK layer.
It is reachable: the local table is the fallback for when the wire supplies no category, and proto3 leaves an unset enum at 0, which
fromSerializedSdkErrortreats as absent.What this changes
Import and re-export the shared
categoryForCode, delete the local copy. Six insertions, twenty-eight deletions, no new code.SDKException's two call sites are untouched.Verification
Ran the codegen (
generate_ts.sh,generate_ts_convenience.py,generate_defaults_pool.py,generate_streams.sh), builtproto-ts, then built and tested the Electron package:Both existing assertions in
test/unit/errors.test.tsstill pass unchanged, includingcategoryForCode maps ranges like the canonical table. That is not luck: all eight codes it pins (0, 100, 110, 130, 182, 259, 380, 804) fall in bands where the two tables already agreed, which is exactly why the divergence went unnoticed.The behaviour change, from the built package:
Summary by CodeRabbit
Type of Change
Testing
bindings/electronhas nolintscript;npm run build(tsc, 0 errors) is the equivalent gate and passedtest/unit/errors.test.tsalready pins this function and passes unchanged; perAGENTS.mdI do not add tests unless askedRan locally in
bindings/electron, after the four IDL codegen steps and aproto-tsbuild:Platform-Specific Testing
None of the template's platform sections apply: this changes
bindings/electron, which the matrix does not cover (Electron is also missing from the Labels list below).Labels
None of the listed labels matches. This is
bindings/electron, and there is no Electron SDK label in the repo today (ios-sdk,kotlin-sdk,web-sdk,flutter-sdk,rn-sdk,core). Closest by blast radius is none of them, since no other binding is touched. Happy to relabel if you have a preference.Checklist
Screenshots
Not applicable, no UI surface.