perf(bridge): optimize map/slice reflection bindings and fix arraybuffer safety#163
perf(bridge): optimize map/slice reflection bindings and fix arraybuffer safety#163repyh wants to merge 1 commit into
Conversation
…fer safety - Replaced v.MapKeys() with v.MapRange() in bindMap to eliminate slice allocations. - Replaced fmt.Sprint with strconv and Kind checks in bindMap for numeric keys to prevent boxing. - Added fast-path in bindSlice for []byte, using a native ToUint8Array conversion to prevent deep iteration and interface boxing. - Fixed a safety vulnerability in ToArrayBuffer by explicitly copying the underlying byte slice instead of letting sobek wrap shared Go memory directly. - Implemented ToUint8Array with a graceful fallback if the environment lacks the Uint8Array global constructor. - Corrected MapSharedBuffer to use sobek.IsNull and sobek.IsUndefined safely and gracefully fallback to avoid panics. Co-authored-by: repyh <63894915+repyh@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR hardens binary data conversion and improves reflection-based type binding in the bridge layer. ChangesBinary data and reflection optimizations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@bridge/core/reflection.go`:
- Around line 381-389: The map-key-to-string conversion currently switches on
key.Kind() (using key.String(), strconv.FormatInt/FormatUint) which bypasses any
fmt.Stringer implementations on defined types; before the Kind-based switch in
reflection.go where variable key is used (see key.Kind(), key.String(),
strconv.FormatInt/FormatUint, fmt.Sprint), check key.CanInterface() and if
key.Interface() implements fmt.Stringer call its String() and assign that to
keyStr, otherwise fall back to the existing Kind-based switch; this preserves
custom String() outputs for defined numeric and string key types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 118a3571-c648-4b3a-be4c-e1190307d672
📒 Files selected for processing (2)
bridge/core/arraybuffer.gobridge/core/reflection.go
| switch key.Kind() { | ||
| case reflect.String: | ||
| keyStr = key.String() | ||
| } else { | ||
| case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: | ||
| keyStr = strconv.FormatInt(key.Int(), 10) | ||
| case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: | ||
| keyStr = strconv.FormatUint(key.Uint(), 10) | ||
| default: | ||
| keyStr = fmt.Sprint(key.Interface()) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, for a defined type type K intthat implementsString() string, does fmt.Sprint(K(1))useString()output, and isreflect.TypeOf(K(1)).Kind()stillreflect.Int?
💡 Result:
Yes, for a defined type type K int that implements a String string method, fmt.Sprint(K(1)) will use the output of that String method [1][2][3]. The fmt package checks if an operand implements the Stringer interface (which requires a String string method) and invokes it when formatting values for default verbs like %v [1][4]. Regarding reflection, reflect.TypeOf(K(1)).Kind will return reflect.Int [5][6]. In Go, the Kind of a defined type is the same as the Kind of its underlying type [5][6]. While the Type (as returned by reflect.TypeOf) will be K, the Kind remains the underlying primitive category, which in this case is int [5][6].
Citations:
- 1: https://pkg.go.dev/fmt@latest
- 2: https://go.dev/src/fmt/doc.go?m=text
- 3: https://pkg.go.dev/fmt
- 4: https://github.com/golang/go/blob/de28555c0b33fcaa02779d55ea9289135280ae9f/src/fmt/print.go
- 5: https://stackoverflow.com/questions/73372033/assigning-to-type-definition-using-reflection-in-go
- 6: https://medium.com/kokster/go-reflection-creating-objects-from-types-part-ii-composite-types-69a0e8134f20
Preserve fmt.Stringer output when converting map keys
In bridge/core/reflection.go (lines 381–389), the Kind-based switch formats int/uint keys via strconv.FormatInt/FormatUint, which bypasses fmt.Stringer for defined numeric key types (e.g., type K int with String() string), changing JS object property key names vs the previous fmt.Sprint(key.Interface()). The same bypass occurs for defined string key types implementing fmt.Stringer due to the case reflect.String: key.String() path.
Fix by checking key.CanInterface() and whether key.Interface() implements fmt.Stringer before the Kind-based switch, and using s.String() when available.
🤖 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 `@bridge/core/reflection.go` around lines 381 - 389, The map-key-to-string
conversion currently switches on key.Kind() (using key.String(),
strconv.FormatInt/FormatUint) which bypasses any fmt.Stringer implementations on
defined types; before the Kind-based switch in reflection.go where variable key
is used (see key.Kind(), key.String(), strconv.FormatInt/FormatUint,
fmt.Sprint), check key.CanInterface() and if key.Interface() implements
fmt.Stringer call its String() and assign that to keyStr, otherwise fall back to
the existing Kind-based switch; this preserves custom String() outputs for
defined numeric and string key types.
1 participant
Optimized
bridge/core/reflection.gospecifically inbindSliceandbindMapby utilizing fast paths for byte slices andMapRangefor maps to reduce allocations and interface boxing. Also optimized and fixed safety issues inbridge/core/arraybuffer.go, ensuring byte slices are copied safely when passed to JS to prevent mutation of Go memory, implementedToUint8Array, and added safety checks against missing globals.PR created automatically by Jules for task 12774365595715365521 started by @repyh
Summary by CodeRabbit
Bug Fixes
Performance