extend compatibility of array deserialization in lookers #5132
Conversation
WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/packages/looker/src/worker/deserializer.ts (3)
Line range hint
5-20: Add TypeScript type annotations for better type safetyThe method lacks type annotations for its parameters and return value. Consider adding proper TypeScript types to improve maintainability and catch potential errors at compile time.
- Detection: (label, buffers) => { + Detection: (label: { mask?: string | { $binary: { base64: string } } }, buffers: Array<ArrayBuffer>) => void {Also, consider adding runtime validation for the buffer size calculations to prevent potential memory issues.
Line range hint
5-67: Refactor duplicate deserialization logic into a shared utility functionThere's significant code duplication across Detection, Heatmap, and Segmentation methods. They share the same deserialization logic with only the property name being different ('mask' vs 'map').
Consider refactoring into a shared utility function:
interface BinaryData { $binary: { base64: string; }; } const deserializeField = ( label: Record<string, unknown>, fieldName: 'mask' | 'map', buffers: ArrayBuffer[] ): void => { let serializedData: string; const field = label?.[fieldName]; if (typeof field === "string") { serializedData = field; } else if (typeof (field as BinaryData)?.$binary?.base64 === "string") { serializedData = (field as BinaryData).$binary.base64; } if (serializedData) { const data = deserialize(serializedData); const [height, width] = data.shape; label[fieldName] = { data, image: new ArrayBuffer(width * height * 4), }; buffers.push(data.buffer); buffers.push(label[fieldName].image); } };Then use it in the factory methods:
const DeserializerFactory = { Detection: (label, buffers) => deserializeField(label, 'mask', buffers), Heatmap: (label, buffers) => deserializeField(label, 'map', buffers), Segmentation: (label, buffers) => deserializeField(label, 'mask', buffers), // ... rest of the factory methods };
Line range hint
12-20: Add error handling for deserialization and buffer allocationThe code lacks error handling for several critical operations:
- The
deserialize()call could fail- Shape dimensions could be invalid
- ArrayBuffer allocation could fail with large dimensions
Add proper error handling:
if (serializedMask) { try { const data = deserialize(serializedMask); const [height, width] = data.shape; // Validate dimensions to prevent overflow if (!Number.isFinite(height) || !Number.isFinite(width) || height <= 0 || width <= 0 || height > Number.MAX_SAFE_INTEGER || width > Number.MAX_SAFE_INTEGER) { throw new Error(`Invalid dimensions: ${width}x${height}`); } const imageSize = width * height * 4; if (imageSize > Number.MAX_SAFE_INTEGER) { throw new Error(`Image buffer size exceeds safe limits`); } label.mask = { data, image: new ArrayBuffer(imageSize), }; buffers.push(data.buffer); buffers.push(label.mask.image); } catch (error) { console.error('Failed to deserialize mask:', error); // Consider how to handle cleanup of any partially allocated buffers throw error; // or handle it according to your error handling strategy } }Also applies to: 37-46, 58-67
app/packages/looker/src/overlays/segmentation.ts (1)
276-279: LGTM! Good safety improvements.The additional null checks and bounds validation effectively prevent potential crashes when
this.targetsis undefined or when the index is out of bounds.Consider adding explicit return type.
For better type safety, consider adding an explicit return type to the method:
- private getTarget(state: Readonly<State>): number { + private getTarget(state: Readonly<State>): number | null {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/looker/src/overlays/segmentation.ts(1 hunks)app/packages/looker/src/worker/deserializer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/looker/src/overlays/segmentation.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/deserializer.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/looker/src/worker/deserializer.ts (1)
Line range hint 1-67: Verify the changes meet all PR objectives
The changes successfully extend the array deserialization compatibility by handling both string and MongoDB binary formats. However, let's verify the integration with the SegmentationOverlay changes mentioned in the PR objectives.
✅ Verification successful
Changes successfully extend array deserialization and integrate with SegmentationOverlay
The verification confirms that the changes in deserializer.ts properly support both string and MongoDB binary formats for mask data, and integrate correctly with the SegmentationOverlay component. Specifically:
- The
DeserializerFactory.Segmentationmethod properly deserializes mask data into a format compatible withSegmentationOverlay's requirements:- Creates a structured object with
dataandimageproperties - Maintains shape information (
height,width) - Properly handles buffer management for both the data and image buffers
- Creates a structured object with
The changes align with the PR objectives and maintain compatibility with the existing overlay visualization system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SegmentationOverlay usage of deserialized data
rg -t typescript "class SegmentationOverlay" -A 10 app/packages/looker/src/
Length of output: 112
Script:
#!/bin/bash
# Search for SegmentationOverlay without file type restriction
rg "class SegmentationOverlay" -A 10
# Also search for any files containing Segmentation in their name
fd "Segmentation"
# Search for imports or usage of deserializer
rg "deserializer|DeserializerFactory" -A 5
Length of output: 5873
Script:
#!/bin/bash
# Check how the SegmentationOverlay uses the deserialized mask data
rg -A 20 "private label: SegmentationLabel" app/packages/looker/src/overlays/segmentation.ts
# Look for the SegmentationLabel type definition
rg "type SegmentationLabel|interface SegmentationLabel"
# Check worker usage of segmentation data
rg "Segmentation.*mask" -A 5
Length of output: 20265
app/packages/looker/src/overlays/segmentation.ts (1)
276-279: Verify null handling in dependent code.
The change to return null for invalid cases affects callers of getTarget. While containsPoint handles this correctly, verify that all other usages properly handle the null case.
✅ Verification successful
Let me gather more context about how the return value is used in both files.
Let me check one more thing to see how the value is used in containsPoint.
Let me get one final piece of context about the containsPoint implementation.
Return type change is safe and properly handled
The change to return null for invalid cases in getTarget is safe because:
- In
containsPoint, the value is used in a boolean context where bothnulland0are falsy, maintaining the same behavior - In
getPointInfo, the value is used to get colors and labels, but this method is only called aftercontainsPointreturnsCONTAINS.CONTENT, ensuring a valid target exists
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of getTarget to verify null handling
ast-grep --pattern 'this.getTarget($$$)'
# Also check for direct property access on the result that might cause null errors
rg -A 2 'getTarget\(' app/packages/looker/src/overlays/
Length of output: 2001
Script:
#!/bin/bash
# Check the implementation in heatmap.ts to see how the value is used
ast-grep --pattern 'getPointInfo($$$) {
$$$
}'
# Get more context around the usage in containsPoint and getPointInfo
rg -B 2 -A 5 'const target = this.getTarget\(state\);' app/packages/looker/src/overlays/
Length of output: 1327
Script:
#!/bin/bash
# Get the containsPoint implementation to see how it handles the value
ast-grep --pattern 'containsPoint($$$) {
$$$
}'
# Also check if there are any other methods that might use getTarget
rg -l 'getTarget' app/packages/looker/src/overlays/
Length of output: 196
Script:
#!/bin/bash
# Get the full containsPoint implementation from both files
rg -B 2 -A 10 'containsPoint\(' app/packages/looker/src/overlays/segmentation.ts app/packages/looker/src/overlays/heatmap.ts
Length of output: 3768
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/looker/src/worker/deserializer.ts (2)
3-14: Consider using a more specific type than 'object'Good job extracting the common mask extraction logic into a helper function! This addresses the code duplication concern from the previous review.
Consider using a more specific type for the
labelparameter:- label: object, + label: Record<string, unknown>,
38-41: Consider further reducing duplication in buffer handlingWhile the mask extraction is now DRY, there's still duplication in the buffer handling logic across Detection, Heatmap, and Segmentation methods.
Consider extracting the common buffer handling logic:
const handleBuffers = ( serializedData: string, label: Record<string, unknown>, buffers: ArrayBuffer[], propertyName: 'mask' | 'map' ) => { const data = deserialize(serializedData); const [height, width] = data.shape; label[propertyName] = { data, image: new ArrayBuffer(width * height * 4), }; buffers.push(data.buffer); buffers.push(label[propertyName].image); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/worker/deserializer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/deserializer.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/looker/src/worker/deserializer.ts (2)
18-21: LGTM! Clean refactor using the new helper function
The implementation correctly uses the new helper while maintaining the existing buffer management pattern.
54-57: LGTM! Consistent with established pattern
What changes are proposed in this pull request?
this.targetscan be undefined inSegmentation, causing a crashHow is this patch tested? If it is not, please explain why.
local app
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes