add default labels visibility in dataset app config + selective labels rendering optimization #5356
Conversation
|
Warning Rate limit exceeded@sashankaryal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces extensive modifications across multiple modules. The changes include updates to configuration files, component imports, and state management. Enhancements in overlay processing, asynchronous label rendering, and modal selective rendering are implemented. New types, selectors, and GraphQL fields for label visibility have been added, while test suites and e2e cases have been updated. Several functions now include refined parameter lists and cleanup logic, improving resource management. Overall, the modifications focus on improved modularity and control flow across lookers, overlays, workers, and UI components. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as AsyncLabelsRenderingManager
participant Queue as JobQueue
participant Worker as FreeWorker
participant System as WorkerResponse Handler
Client->>Queue: enqueueLabelPaintingJob(sample, labels)
Queue->>Worker: assignJob(sample, labels)
Worker->>Worker: Process label painting job
Worker->>Queue: Send processed job result
Queue->>Client: Resolve job and update sample state
Client->>System: Trigger overlay updater
sequenceDiagram
participant Modal as Modal Component
participant Hook as useImageModalSelectiveRendering
participant Detector as useDetectNewActiveLabelFields
participant Looker as Looker Instance
Modal->>Hook: Call useImageModalSelectiveRendering(id, looker)
Hook->>Detector: Retrieve new active label fields
Detector->>Hook: Return new fields (or null)
alt New fields detected
Hook->>Looker: Invoke refreshSample(newFields) to update sample
else No new fields
Hook->>Looker: Maintain current sample state
end
Suggested labels
Suggested reviewers
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
|
sashankaryal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
app/packages/relay/src/fragments/datasetAppConfigFragment.ts (1)
8-11: Add type definitions for include/exclude fieldsWhile the structure is logical for visibility filtering, consider adding type definitions for the
includeandexcludefields to ensure type safety and better IDE support.defaultVisibilityLabels { - include - exclude + include: [String!] + exclude: [String!] }app/packages/looker/src/worker/shared.ts (1)
3-3: Add JSDoc comments for the render status typeConsider adding documentation to explain the purpose and meaning of each render status state for better maintainability.
+/** + * Represents the rendering lifecycle of a dense label + * @type {null} Initial state before rendering begins + * @type {"painting"} Label is currently being rendered + * @type {"painted"} Label has been rendered successfully + * @type {"decoded"} Label data has been decoded but not yet rendered + */ export type DenseLabelRenderStatus = null | "painting" | "painted" | "decoded";app/packages/looker/src/worker/resolve-color.ts (1)
33-42: Enhance promise error handlingThe promise creation lacks error handling. Consider adding error handling and timeout mechanism.
- seedPromises[key] = new Promise((resolve) => { + seedPromises[key] = new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + delete seedRequests[key]; + delete seedPromises[key]; + reject(new Error('Color resolution timeout')); + }, 5000); seedRequests[key] = resolve; postMessage({ method: "requestColor", key, seed, pool, }); + return () => clearTimeout(timeout); });app/packages/core/src/components/Sidebar/useOnSidebarSelectionChange.ts (2)
23-60: Optimize effect performance and add cleanupThe effect has several optimization opportunities:
- The Set operations could be memoized
- Missing cleanup function could lead to memory leaks
- Consider using
useMemoforcurrentActiveLabelFields+ const currentActiveLabelFields = useMemo( + () => new Set(activeLabelFieldsValue), + [activeLabelFieldsValue] + ); useEffect(() => { const thisPageActiveFields = sidebarTracker.get(gridPageValue); - const currentActiveLabelFields = new Set(activeLabelFieldsValue); if (currentActiveLabelFields.size === 0) { return; } // ... rest of the effect + return () => { + // Cleanup any subscriptions or pending state updates + }; }, [activeLabelFieldsValue, sidebarTracker]);
15-17: Consider using useLayoutEffect for ref updatesUpdating refs in render phase could lead to inconsistencies. Consider moving the ref update to useLayoutEffect.
const gridPageValueRef = useRef(gridPageValue); - gridPageValueRef.current = gridPageValue; + useLayoutEffect(() => { + gridPageValueRef.current = gridPageValue; + }, [gridPageValue]);app/packages/utilities/src/schema.ts (1)
70-92: Add JSDoc documentation for the enhanced function signature.The function has been enhanced with new parameters, but lacks documentation explaining their purpose and usage.
Add this documentation above the function:
+/** + * Returns fields with specified embedded document type(s). + * @param schema The schema to search through + * @param embeddedDocType Single or array of embedded document types to match + * @param shouldRecurse Whether to recursively search nested fields (default: true) + * @returns Array of matching fields + */ export function getFieldsWithEmbeddedDocType(app/packages/core/src/components/Grid/useRefreshers.ts (1)
100-109: Consider debouncing the refresh operation.Refreshing all lookers on every label toggle could impact performance if there are many lookers. Consider implementing debouncing to prevent rapid consecutive refreshes.
+import { debounce } from 'lodash'; + const gridLabelsToggleTracker = useOnSidebarSelectionChange({ modal: false }); +const debouncedRefresh = useMemo( + () => + debounce(() => { + lookerStoreRef.current?.forEach((looker) => { + looker.refreshSample(); + }); + }, 250), + [] +); useEffect(() => { - lookerStoreRef.current?.forEach((looker) => { - looker.refreshSample(); - }); + debouncedRefresh(); + return () => { + debouncedRefresh.cancel(); + }; }, [gridLabelsToggleTracker]);app/packages/looker/src/overlays/base.ts (1)
77-77: Add JSDoc documentation for the cleanup method.The new
setTargetsToNullparameter needs documentation to explain its purpose and when it should be set to true/false.Add this documentation above the method:
+/** + * Cleans up resources used by the overlay. + * @param setTargetsToNull When true, sets target references to null for garbage collection + */ cleanup?(setTargetsToNull: boolean): void;app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
134-135: Consider using proper type definition instead of type assertion.The type assertion to
DenseLabelRenderStatussuggests that the label object's type definition might need improvement.- label.renderStatus = "decoded" as DenseLabelRenderStatus; + label.renderStatus = "decoded";And update the label parameter type:
- label: Record<string, any>, + label: Record<string, any> & { renderStatus?: DenseLabelRenderStatus },app/packages/looker/src/overlays/detection.ts (1)
264-269: LGTM! Consider adding JSDoc.The implementation is correct and maintains backward compatibility. Consider adding JSDoc to document the purpose of the
setTargetsToNullparameter.+/** + * Cleans up resources associated with the detection overlay. + * @param setTargetsToNull - When true, sets the label mask to null after cleanup + */ public cleanup(setTargetsToNull = false): void {app/packages/looker/src/worker/painter.ts (1)
33-34: LGTM! Consider using an enum for render status.The implementation of render status tracking is consistent across all methods. For better type safety and maintainability, consider using an enum for the render status values.
enum RenderStatus { PAINTING = "painting", PAINTED = "painted", DECODED = "decoded", NONE = null }Also applies to: 146-147, 162-163, 193-194, 274-275, 407-408
app/packages/state/src/recoil/selectors.ts (1)
145-152: LGTM! Consider handling potential undefined values.The selector implementation is clean and type-safe. Consider handling the case where
datasetAppConfigmight be undefined.export const defaultVisibilityLabels = selector<State.DefaultVisibilityLabelsConfig>({ key: "defaultVisibilityLabels", get: ({ get }) => { - return get(datasetAppConfig) + const config = get(datasetAppConfig); + return config .defaultVisibilityLabels as State.DefaultVisibilityLabelsConfig | null; }, });app/packages/looker/src/worker/index.ts (1)
Line range hint
310-323: Consider documenting the 3D labels exception.The code correctly processes all 3D labels regardless of active paths, but this behavior should be documented.
- // we process all 3d labels regardless of active paths + // We process all 3D labels regardless of active paths because + // 3D label rendering has different optimization constraints + // and dependencies compared to 2D labels. process3DLabels(schema, sample);fiftyone/core/odm/dataset.py (1)
518-519: Documentation needs clarification.The docstring for
default_visibility_labelsshould specify the expected dictionary structure (include/exclude lists) to match the GraphQL schema.- default_visibility_labels (None): config with include and exclude lists - of labels to render by default in the App + default_visibility_labels (None): a dictionary containing 'include' and 'exclude' + lists of label field names to control their default visibility in the App. + Example:: + { + "include": ["detections", "classifications"], + "exclude": ["segmentations"] + }app/schema.graphql (1)
332-335: LGTM! Consider adding descriptions.The
FieldVisibilityConfigtype is well-structured. Consider adding descriptions to document the purpose of each field.type FieldVisibilityConfig { + """ + List of label field names to show by default + """ include: [String!] + """ + List of label field names to hide by default + """ exclude: [String!] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/datasetAppConfigFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (25)
.gitignore(1 hunks)app/packages/core/src/components/Dataset.tsx(1 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(3 hunks)app/packages/core/src/components/Modal/Modal.tsx(2 hunks)app/packages/core/src/components/Sidebar/useOnSidebarSelectionChange.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(7 hunks)app/packages/looker/src/overlays/base.ts(1 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(1 hunks)app/packages/looker/src/overlays/segmentation.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/index.ts(11 hunks)app/packages/looker/src/worker/painter.ts(8 hunks)app/packages/looker/src/worker/resolve-color.ts(1 hunks)app/packages/looker/src/worker/shared.ts(1 hunks)app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts(0 hunks)app/packages/relay/src/fragments/datasetAppConfigFragment.ts(1 hunks)app/packages/state/src/recoil/schema.ts(4 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)app/packages/state/src/recoil/sidebar.ts(2 hunks)app/packages/state/src/recoil/types.ts(2 hunks)app/packages/utilities/src/schema.ts(3 hunks)app/schema.graphql(2 hunks)fiftyone/core/odm/dataset.py(2 hunks)fiftyone/server/query.py(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- app/packages/core/src/components/Dataset.tsx
🧰 Additional context used
📓 Path-based instructions (19)
app/packages/looker/src/worker/shared.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/state/src/recoil/selectors.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/overlays/base.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/relay/src/fragments/datasetAppConfigFragment.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/state/src/recoil/types.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/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/overlays/heatmap.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/overlays/detection.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/core/src/components/Modal/Modal.tsx (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/state/src/recoil/sidebar.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/core/src/components/Sidebar/useOnSidebarSelectionChange.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/core/src/components/Grid/useRefreshers.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/state/src/recoil/schema.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/disk-overlay-decoder.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/painter.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/index.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/resolve-color.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/lookers/abstract.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/utilities/src/schema.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.
🪛 Biome (1.9.4)
app/packages/state/src/recoil/schema.ts
[error] 372-381: This code is unreachable
... because either this statement, ...
... this statement, ...
... this statement, ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (23)
app/packages/utilities/src/schema.ts (2)
1-12: LGTM! Well-structured constants declaration.The constants are well-organized, properly namespaced, and follow TypeScript naming conventions.
116-124: Document the rationale for non-recursive search.The function explicitly sets
shouldRecursetofalsewhen callinggetFieldsWithEmbeddedDocType. Please document why recursive search is not needed for dense labels.Let's verify if dense labels can appear in nested fields:
app/packages/core/src/components/Grid/useRefreshers.ts (2)
68-81: LGTM! Proper cleanup implementation.The subscription cleanup follows React best practices by properly unsubscribing when the component unmounts.
97-99: LGTM! Proper use of useRef for mutable references.The implementation correctly uses
useRefto maintain a reference to the looker store without triggering re-renders.app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
2-8: LGTM! Clean import organization.Unused imports have been removed and new imports are properly organized.
app/packages/state/src/recoil/types.ts (2)
24-27: LGTM! Well-structured type definition.The
DefaultVisibilityLabelsConfigtype is well-defined with clear optional properties for including and excluding labels.
127-127: LGTM! Interface extension is appropriate.The addition of
defaultVisibilityLabelstoDatasetAppConfigaligns with the PR objectives for configuring default label visibility.app/packages/looker/src/overlays/heatmap.ts (1)
209-214: LGTM! Cleanup method enhancement.The cleanup method now provides more control over target cleanup through the optional parameter, maintaining backward compatibility with the default
falsevalue.app/packages/looker/src/overlays/segmentation.ts (1)
264-269: LGTM! Consistent cleanup implementation.The cleanup method follows the same pattern as the heatmap overlay, providing consistent behavior across overlay types.
app/packages/core/src/components/Modal/Modal.tsx (1)
182-186: Consider race condition mitigation.While the implementation correctly refreshes the sample on sidebar selection changes, be aware of the potential race condition mentioned in the PR objectives when recoloring samples too quickly.
Consider implementing a debounce mechanism to prevent rapid consecutive refreshes:
+import { debounce } from 'lodash'; + const modalLabelsToggleTracker = useOnSidebarSelectionChange({ modal: true }); useEffect(() => { - activeLookerRef.current?.refreshSample(); + const debouncedRefresh = debounce(() => { + activeLookerRef.current?.refreshSample(); + }, 100); + debouncedRefresh(); + return () => debouncedRefresh.cancel(); }, [modalLabelsToggleTracker]);✅ Verification successful
Race condition mitigation is warranted
The review comment is valid. The
refreshSampleoperation involves async sample loading and overlay processing, which could be resource-intensive when triggered rapidly by sidebar selection changes. The suggested debounce implementation aligns with existing patterns in the codebase and provides proper cleanup.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for refreshSample implementation ast-grep --pattern 'refreshSample() { $$$ }' # Look for existing debounce usage rg "debounce" -A 2 # Search for similar useEffect patterns with refresh rg "useEffect.*refresh" -A 2Length of output: 27337
Script:
#!/bin/bash # Search for refreshSample method with context rg "refreshSample" -A 5 -B 5 # Look for Looker class implementation ast-grep --pattern 'class $_ { $$$ refreshSample($$$) { $$$ } $$$ }'Length of output: 2627
app/packages/looker/src/worker/index.ts (4)
Line range hint
52-61: Documentation accurately reflects the implementation.The updated documentation clearly explains the processing steps and aligns well with the implementation.
174-191: LGTM! Consistent handling of active paths.The implementation correctly applies the same active paths check for painting operations, maintaining consistency with the decoding phase.
Line range hint
213-217: Good use of render status for optimization.The bitmap collection is properly gated by the render status check, ensuring only fully painted labels are processed.
96-121: Verify the impact of skipping inactive paths.The conditional processing of labels based on
activePathsis a good optimization. However, we should verify that skipping inactive paths doesn't cause issues with label dependencies.✅ Verification successful
Skipping inactive paths is safe
The conditional processing of labels based on
activePathsis a safe optimization. Each label maintains its own independent rendering state, and there are no cross-label dependencies that could be affected by async processing of inactive paths.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential label dependencies across paths ast-grep --pattern 'label.renderStatus = $status'Length of output: 51
Script:
#!/bin/bash # Search for label-related patterns rg -A 5 "label\.renderStatus" --type ts # Look for label processing and dependencies ast-grep --pattern 'function $name($_) { $$$ label.renderStatus $$$ }' # Find async label processing rg -A 10 "async.*label" --type tsLength of output: 6460
fiftyone/server/query.py (2)
62-66: LGTM! Well-structured GraphQL type definition.The
FieldVisibilityConfigclass is properly defined with optional fields and appropriate type annotations for GraphQL integration.
219-219: LGTM! Clean integration of visibility configuration.The
default_visibility_labelsfield is properly added toDatasetAppConfigwith the correct type annotation.app/packages/looker/src/lookers/abstract.ts (5)
49-50: LGTM! Good concurrency control.The
UPDATING_SAMPLES_IDSSet effectively prevents concurrent updates of the same sample.
523-530: LGTM! Robust sample update handling.The implementation effectively handles concurrent sample updates by:
- Checking if a sample is already being updated
- Cleaning up overlays
- Queueing a microtask for retry
534-536: LGTM! Clean refresh implementation.The
refreshSamplemethod provides a convenient way to refresh the current sample.
719-722: LGTM! Enhanced cleanup flexibility.The
cleanOverlaysmethod now supports optional target nullification through thesetTargetsToNullparameter.
316-319: Address the TODO comment for mouse overlay detection.The commented code for mouse overlay detection needs to be fixed or removed.
Run the following script to check for any existing implementations or related code:
✅ Verification successful
Uncomment the mouse overlay detection code
The commented implementation is valid and consistent with the codebase's overlay detection patterns. The TODO comment appears to be outdated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for mouse overlay detection implementations # Test: Look for similar implementations of mouse overlay detection rg -A 5 "mouseIsOnOverlay|containsPoint"Length of output: 9329
app/packages/state/src/recoil/sidebar.ts (2)
75-76: LGTM! Clear type definitions.The types
GridPageNumberandSidebarEntriesSetare well-defined and serve clear purposes.
912-918: LGTM! Well-structured state management.The
labelsToggleTrackeratom family effectively manages label toggle states across grid pages using appropriate types and a sensible default value.
|
If the default is not set, does that mean previously rendered things will suddenly not be visible unless someone explicitly sets it? Considering the multi-user case, Should this be set at the dataset level for all users or per dataset per user? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/packages/looker/src/lookers/abstract.ts (1)
721-725: Enhance clarity of thesetTargetsToNullparameterThe parameter name
setTargetsToNullin thecleanOverlaysmethod might be unclear to other developers. Consider renaming it to something more descriptive, such asdisposeOverlaysorresetOverlayTargets, to better convey its purpose and improve code readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/packages/core/src/components/Sidebar/useOnSidebarSelectionChange.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/lookers/frame-reader.ts(3 hunks)app/packages/looker/src/lookers/video.ts(1 hunks)app/packages/looker/src/worker/index.ts(18 hunks)app/packages/looker/src/worker/resolve-color.ts(1 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/packages/state/src/recoil/selectors.ts
- app/packages/core/src/components/Sidebar/useOnSidebarSelectionChange.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/looker/src/lookers/video.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/lookers/frame-reader.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/resolve-color.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/lookers/abstract.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/index.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.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/resolve-color.ts
[error] 15-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: lint / eslint
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (6)
app/packages/looker/src/lookers/frame-reader.ts (1)
35-35: LGTM! Clean implementation of activePaths.The addition of
activePathsto the interface and its usage insetStreamis well-structured and aligns with the PR's objective of selective label rendering.Also applies to: 92-92, 135-135
app/packages/looker/src/lookers/video.ts (1)
260-260: LGTM! Proper integration of activePaths.The addition of
activePathsfrom state options is consistent with the frame-reader implementation.app/packages/looker/src/worker/index.ts (4)
Line range hint
52-60: LGTM! Well-documented label processing implementation.The comment clearly explains the processing steps and the recursive nature of the implementation.
310-310: Verify the decision to process all 3D labels.The code processes all 3D labels regardless of activePaths. Please confirm if this is intentional and document the reasoning.
96-121: LGTM! Clean implementation of selective label processing.The conditional processing of dense labels based on activePaths is well-implemented, with proper handling of the render status.
175-191: LGTM! Consistent implementation of painter factory integration.The activePaths check is consistently applied before invoking the painter factory.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/state/src/recoil/schema.ts (1)
325-378: Well-structured implementation of label visibility logic.The selector efficiently handles different visibility configurations using Set operations. However, the Set operations could be optimized further.
Consider this optimization for the Set operations:
- const includeList = new Set(defaultVisibleLabelsConfig.include); - const excludeList = new Set(defaultVisibleLabelsConfig.exclude); - // resolved = set(include) - set(exclude) - const resolved = new Set( - [...includeList].filter((x) => !excludeList.has(x)) - ); - return allLabels.filter((label) => resolved.has(label)); + // Pre-filter allLabels against include list first + const excludeSet = new Set(defaultVisibleLabelsConfig.exclude); + return allLabels + .filter((label) => defaultVisibleLabelsConfig.include.includes(label)) + .filter((label) => !excludeSet.has(label));This optimization:
- Reduces memory usage by creating only one Set
- Avoids unnecessary Set creation for the include list
- Maintains the same functionality with better performance
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/state/src/recoil/schema.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/state/src/recoil/schema.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.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (2)
app/packages/state/src/recoil/schema.ts (2)
26-26: LGTM! Clean import organization.The new imports are well-organized and properly grouped with related imports.
Also applies to: 35-35
573-573: LGTM! Proper fallback to default visible labels.The selector correctly uses
defaultVisibleLabelsas a fallback when no active fields are stored, which aligns with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (1)
9-9: Consider memory management for gridActivePathsLUTThe global Map instance could potentially grow unbounded as new Looker IDs are added. Consider implementing a cleanup mechanism or using a size-limited LRU cache.
-export const gridActivePathsLUT = new Map<LookerId, CachedLabels>(); +import LRU from "lru-cache"; +export const gridActivePathsLUT = new LRU<LookerId, CachedLabels>({ + max: 1000, // Adjust based on expected concurrent Looker instances +});app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts (1)
44-48: Consider optimizing Set creationThe spread operator with Set creation could be optimized for performance.
- new Set([...(thisLookerActiveFields ?? []), ...currentActiveLabelFields]) + new Set(function*() { + if (thisLookerActiveFields) yield* thisLookerActiveFields; + yield* currentActiveLabelFields; + }())app/packages/core/src/components/Grid/useSelect.ts (1)
38-41: Enhance comment clarityThe current comment could be more descriptive about why this refresh is necessary.
- // rerender looker if active fields have changed and have never been rendered before + // Trigger a looker refresh when newly activated label fields are detected, + // ensuring they are properly rendered in the viewportapp/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts (2)
17-18: Fix typo in commentThere's a typo in the comment.
- // lut shoiuldn't be modified + // lut shouldn't be modified
4-96: LGTM! Comprehensive test coverageThe test suite is well-structured and covers important edge cases:
- Empty fields
- New looker scenarios
- Subset of fields
- Repeated calls
Consider adding a test case for concurrent modifications to ensure thread safety.
app/packages/core/src/components/Grid/Grid.tsx (2)
Line range hint
179-179: Remove debug console.log statementThe console.log statement should be removed before merging to production.
- console.log(">>>painting ", `${prefix ?? ""}${field}`);
50-57: Add dependencies array to useRecoilCallbackThe useRecoilCallback hook should include a dependencies array as the second argument to prevent unnecessary recreations of the callback.
const getCurrentActiveLabelFields = useRecoilCallback( ({ snapshot }) => () => { return snapshot .getLoadable(fos.activeLabelFields({ modal: false })) .getValue(); } - ); + , []);app/packages/looker/src/worker/index.ts (1)
Line range hint
773-783: Improve error handling in worker message postingThe catch block could be more robust by logging the error before retrying.
} catch (error) { // rarely we'll get a DataCloneError // if one of the buffers is detached and we didn't catch it // try again without transferring the buffers (copying them) if (error.name === "DataCloneError") { + console.warn("DataCloneError encountered, retrying without buffer transfer", error); labelsWorker.postMessage(workerArgs); } else { + console.error("Unexpected error in worker message posting", error); throw error; } }app/packages/looker/src/lookers/abstract.ts (1)
536-543: Improve the TODO comment with more contextThe TODO comment should provide more context about the issue and potential solutions.
- // todo: sometimes instance in spotlight?.updateItems() is defined but has no ref to sample - // this crashes the app. this is a bug and should be fixed + /** + * TODO: Fix crash in spotlight?.updateItems() + * Issue: Sometimes the instance exists but has no reference to sample + * Potential fixes: + * 1. Add null checks in updateItems + * 2. Ensure sample is always set before calling updateItems + * 3. Add error boundary to handle the crash gracefully + */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/packages/core/src/components/Grid/Grid.tsx(4 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(2 hunks)app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/core/src/components/Modal/Modal.tsx(2 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts(1 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts(1 hunks)app/packages/core/src/components/Sidebar/useShouldReloadSample.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/worker/index.ts(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/components/Grid/useRefreshers.ts
🧰 Additional context used
📓 Path-based instructions (8)
app/packages/core/src/components/Grid/useSelect.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/core/src/components/Sidebar/useShouldReloadSample.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/lookers/abstract.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/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.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/index.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/core/src/components/Sidebar/syncAndCheckRefreshNeeded.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/core/src/components/Modal/Modal.tsx (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/core/src/components/Grid/Grid.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
🔇 Additional comments (6)
app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (1)
11-30: LGTM! Well-structured React hookThe hook follows React best practices:
- Proper use of useCallback for memoization
- Dependency array is correctly specified
- Clear type definitions
app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts (1)
3-11: LGTM! Excellent documentationThe JSDoc comments are clear, comprehensive, and properly document all parameters and return values.
app/packages/core/src/components/Grid/Grid.tsx (1)
74-74: Well-implemented active paths trackingThe implementation properly manages the lifecycle of active paths:
- Cleans up paths when destroying Spotlight instances
- Initializes paths when rendering new instances
- Safely checks for existence before initialization
Also applies to: 116-126
app/packages/core/src/components/Modal/Modal.tsx (1)
181-185: Well-implemented modal refresh on sidebar changesThe implementation properly handles sidebar selection changes:
- Uses a dedicated tracker for modal context
- Refreshes the sample when selections change
- Correctly scopes the effect with proper dependency
app/packages/looker/src/worker/index.ts (1)
96-121: Excellent optimization of label processingThe implementation efficiently processes only the active labels:
- Checks if the field is in activePaths before processing
- Sets renderStatus to null for inactive labels
- Prevents unnecessary processing and memory usage
app/packages/looker/src/lookers/abstract.ts (1)
522-532: Well-implemented concurrent update handlingThe implementation properly prevents concurrent updates of the same sample:
- Uses a shared Set to track updating samples
- Queues updates using queueMicrotask
- Properly cleans up state after updates
sashankaryal
force-pushed
the
feat/app-config-default-labels-visibility
branch
from
dd60641 to
4630ad4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/packages/core/src/components/Grid/Grid.tsx (1)
115-126: Consider memoizing the active label fields initialization.While the implementation is correct, consider memoizing the currentActiveLabelFields value to prevent unnecessary recomputation during frequent renders.
+ const memoizedActiveLabelFields = useMemo( + () => getCurrentActiveLabelFields(), + [getCurrentActiveLabelFields] + ); // initialize active paths tracker - const currentActiveLabelFields = getCurrentActiveLabelFields(); if ( - currentActiveLabelFields && + memoizedActiveLabelFields && !gridActivePathsLUT.has(id.description) ) { gridActivePathsLUT.set( id.description, - new Set(currentActiveLabelFields) + new Set(memoizedActiveLabelFields) ); }app/packages/utilities/src/schema.ts (2)
70-92: Consider adding type guards for the embeddedDocType parameter.The function now accepts both string and string[] types, but there's no type guard to ensure type safety at runtime.
export function getFieldsWithEmbeddedDocType( schema: Schema, - embeddedDocType: string | string[], + embeddedDocType: string | readonly string[], shouldRecurse = true ): Field[] { const result: Field[] = []; + const types = Array.isArray(embeddedDocType) ? embeddedDocType : [embeddedDocType]; function recurse(schema: Schema) { for (const field of Object.values(schema ?? {})) { - if (Array.isArray(embeddedDocType)) { - if (embeddedDocType.includes(field.embeddedDocType)) { - result.push(field); - } - } else if (field.embeddedDocType === embeddedDocType) { + if (types.includes(field.embeddedDocType)) { result.push(field); } if (field.fields && shouldRecurse) { recurse(field.fields); } } } recurse(schema); return result; }
116-124: Add JSDoc comments for the new function.The function's purpose and return value should be documented for better maintainability.
+/** + * Returns an array of field names that represent dense labels in the schema. + * Dense labels are fields with embedded document types defined in DENSE_LABEL_EMBEDDED_DOC_TYPES. + * @param schema - The schema to search for dense labels + * @returns An array of field names representing dense labels + */ export function getDenseLabelNames(schema: Schema): string[] { const denseLabels = getFieldsWithEmbeddedDocType( schema, DENSE_LABEL_EMBEDDED_DOC_TYPES, false ); return denseLabels.map((label) => label.name); }app/packages/state/src/recoil/schema.ts (1)
325-378: Optimize Set operations for better performance.The implementation is logically correct but could be optimized when handling include/exclude lists.
// is in both include and exclude -const includeList = new Set(defaultVisibleLabelsConfig.include); -const excludeList = new Set(defaultVisibleLabelsConfig.exclude); -// resolved = set(include) - set(exclude) -const resolved = new Set( - [...includeList].filter((x) => !excludeList.has(x)) -); -return allLabels.filter((label) => resolved.has(label)); +const excludeSet = new Set(defaultVisibleLabelsConfig.exclude); +return allLabels.filter( + (label) => + defaultVisibleLabelsConfig.include.includes(label) && !excludeSet.has(label) +);app/packages/looker/src/worker/resolve-color.ts (1)
19-20: Consider using Map for better type safety.The use of plain objects for storing requests and promises could benefit from using Map for better type safety and built-in key management.
- const requests = {}; - const promises = {}; + const requests = new Map<number, Record<string | number, (color: string) => void>>(); + const promises = new Map<number, Record<string | number, Promise<string>>>();app/packages/looker/src/worker/index.ts (1)
311-311: Document 3D labels processing behavior.Add a comment explaining why 3D labels are processed regardless of activePaths status.
- // we process all 3d labels regardless of active paths + // 3D labels are always processed regardless of active paths because they are + // essential for the scene geometry and cannot be selectively renderedapp/packages/looker/src/worker/disk-overlay-decoder.ts (1)
134-135: Consider improving type safety and error handling.The current implementation has a few areas for improvement:
- Type casting could be replaced with proper type definition
- Consider adding error handling for the race condition mentioned in the PR objectives
Apply this diff to improve type safety:
- label.renderStatus = "decoded" as DenseLabelRenderStatus; + label.renderStatus = "decoded"; + type LabelWithRenderStatus = typeof label & { renderStatus: DenseLabelRenderStatus };Also, consider adding a check to prevent race conditions:
+ if (label.renderStatus === "painting") { + // Skip updating if already being processed + return; + } label.renderStatus = "decoded" as DenseLabelRenderStatus;app/packages/looker/src/lookers/abstract.ts (1)
49-50: Add documentation for UPDATING_SAMPLES_IDS.While the module-scoped Set is intentional (as per retrieved learning), adding documentation would help future maintainers understand its purpose and scope.
+/** + * A shared Set to track sample IDs that are currently being updated across all instances + * of AbstractLooker. This is intentionally module-scoped to prevent concurrent updates + * of the same sample across different instances. + */ const UPDATING_SAMPLES_IDS = new Set();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/datasetAppConfigFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (30)
.gitignore(1 hunks)app/packages/core/src/components/Dataset.tsx(1 hunks)app/packages/core/src/components/Grid/Grid.tsx(4 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(2 hunks)app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/core/src/components/Modal/Modal.tsx(2 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts(1 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts(1 hunks)app/packages/core/src/components/Sidebar/useShouldReloadSample.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/lookers/frame-reader.ts(3 hunks)app/packages/looker/src/lookers/video.ts(1 hunks)app/packages/looker/src/overlays/base.ts(1 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(1 hunks)app/packages/looker/src/overlays/segmentation.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/index.ts(18 hunks)app/packages/looker/src/worker/painter.ts(8 hunks)app/packages/looker/src/worker/resolve-color.ts(1 hunks)app/packages/looker/src/worker/shared.ts(1 hunks)app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts(0 hunks)app/packages/relay/src/fragments/datasetAppConfigFragment.ts(1 hunks)app/packages/state/src/recoil/schema.ts(4 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)app/packages/state/src/recoil/types.ts(2 hunks)app/packages/utilities/src/schema.ts(3 hunks)app/schema.graphql(2 hunks)fiftyone/core/odm/dataset.py(2 hunks)fiftyone/server/query.py(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- .gitignore
- app/packages/looker/src/worker/shared.ts
- app/packages/core/src/components/Dataset.tsx
- app/packages/relay/src/fragments/datasetAppConfigFragment.ts
- app/packages/state/src/recoil/selectors.ts
- app/packages/core/src/components/Grid/useSelect.ts
- app/schema.graphql
- app/packages/looker/src/overlays/heatmap.ts
- app/packages/looker/src/overlays/detection.ts
- app/packages/state/src/recoil/types.ts
- fiftyone/server/query.py
- fiftyone/core/odm/dataset.py
- app/packages/looker/src/overlays/base.ts
- app/packages/looker/src/overlays/segmentation.ts
- app/packages/looker/src/worker/painter.ts
- app/packages/core/src/components/Grid/useRefreshers.ts
- app/packages/looker/src/lookers/video.ts
- app/packages/core/src/components/Sidebar/useShouldReloadSample.ts
- app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts
- app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts
🧰 Additional context used
📓 Path-based instructions (9)
app/packages/looker/src/lookers/frame-reader.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/core/src/components/Modal/Modal.tsx (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/core/src/components/Grid/Grid.tsx (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/state/src/recoil/schema.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/resolve-color.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/disk-overlay-decoder.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/lookers/abstract.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/index.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/utilities/src/schema.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.
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (18)
app/packages/core/src/components/Grid/Grid.tsx (3)
13-13: LGTM! Import statements are properly organized.The new imports are correctly added and align with the implementation requirements.
Also applies to: 16-16
50-57: LGTM! Well-implemented Recoil callback.The function correctly uses useRecoilCallback to access the Recoil state and follows React/Recoil best practices.
74-74: LGTM! Proper cleanup implementation.The addition of gridActivePathsLUT cleanup in the destroy callback prevents potential memory leaks.
app/packages/utilities/src/schema.ts (1)
1-12: LGTM! Well-structured constants for embedded document types.The constants are clearly named and organized, making the code more maintainable.
app/packages/state/src/recoil/schema.ts (1)
573-573: LGTM! Proper fallback to default visible labels.The modification correctly uses the new
defaultVisibleLabelsselector as a fallback when no active fields are set.app/packages/looker/src/worker/resolve-color.ts (2)
15-17: LGTM! Good cache configuration.The LRU cache is well-configured with a reasonable max size limit of 1000 entries, which helps prevent memory issues.
31-56: LGTM! Robust promise handling.The promise resolution logic is well-implemented with proper error handling and cache management.
app/packages/looker/src/lookers/frame-reader.ts (2)
35-35: LGTM! Clear interface extension.The addition of activePaths to AcquireReaderOptions is well-documented and non-breaking.
92-92: LGTM! Consistent parameter propagation.The activePaths parameter is correctly propagated through setStream and included in the worker message.
Also applies to: 135-135
app/packages/looker/src/worker/index.ts (1)
96-121: LGTM! Well-structured conditional processing.The label processing logic correctly handles activePaths with proper null checks and conditional rendering.
app/packages/looker/src/worker/disk-overlay-decoder.ts (2)
1-8: LGTM! Clean import statements.The removal of unused imports and addition of DenseLabelRenderStatus improves code maintainability.
Line range hint
20-138: Verify error handling in multi-user scenarios.Based on the PR comments from kaixi-wang about multi-user implications, we should verify the error handling and state management in concurrent scenarios.
Let's check for potential race conditions in the codebase:
✅ Verification successful
Error handling and concurrency implementation verified
The implementation shows proper error isolation and state management for multi-user scenarios. Failed operations gracefully exit without corrupting state, and the worker-based architecture naturally handles concurrent access through message passing and atomic state transitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for concurrent access patterns and error handling # Search for places where renderStatus is modified rg "renderStatus.*=.*" -A 5 # Search for error handling patterns around label processing rg "catch.*\{" -A 5 "app/packages/looker" # Look for other worker files that might interact with this one fd "worker" --type f "app/packages/looker"Length of output: 9630
app/packages/core/src/components/Modal/Modal.tsx (2)
181-182: LGTM!The
modalLabelsToggleTrackeris well-named and correctly initialized with modal-specific context.
183-186: LGTM!The effect is well-implemented with proper null checks and dependency tracking, ensuring samples are refreshed when label visibility changes.
app/packages/looker/src/lookers/abstract.ts (4)
522-532: LGTM!The enhanced
updateSamplemethod effectively prevents concurrent updates of the same sample through:
- Proper ID handling with null coalescing
- Clean overlay cleanup and retry mechanism using queueMicrotask
536-544: Address the TODO comment regarding spotlight?.updateItems() bug.The comment indicates a potential crash when the instance is defined but has no reference to the sample. This should be investigated and fixed.
Would you like me to create a GitHub issue to track this bug? The issue would help track the investigation and fix for the spotlight?.updateItems() crash scenario.
726-729: LGTM!The enhanced
cleanOverlaysmethod provides flexible cleanup behavior through the optionalsetTargetsToNullparameter while maintaining safe cleanup calls.
751-752: LGTM!The cleanup of sample ID from UPDATING_SAMPLES_IDS is properly implemented with consistent ID field handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/packages/core/src/components/Modal/use-looker.ts (1)
59-67: Consider debouncing the refresh operation.While the implementation is correct, rapid changes to
lookerOptions.activePathscould trigger multiple refreshes in quick succession. This might be related to the race condition mentioned in the PR objectives regarding sample recoloring.Consider adding debounce:
useEffect(() => { if (!looker) { return; } + const debouncedRefresh = debounce(() => { if (shouldRefresh(id)) { looker?.refreshSample(); } + }, 150); + + debouncedRefresh(); + return () => debouncedRefresh.cancel(); - if (shouldRefresh(id)) { - looker?.refreshSample(); - } }, [id, lookerOptions.activePaths, looker]);app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (3)
6-10: Consider enhancing type safety.While the types are correctly defined, they could be more specific:
-export type LookerId = string; +export type LookerId = string & { readonly __brand: unique symbol };This would prevent accidental assignment of any string as a LookerId.
12-28: Document the hook's behavior and return value.The implementation is solid, but missing JSDoc documentation would help other developers understand the hook's purpose and usage.
Add documentation:
+/** + * Hook to manage and track changes in label visibility for samples + * @param {Object} params - Hook parameters + * @param {boolean} params.modal - Whether the hook is used in modal context + * @returns {Function} shouldRefresh - Function to check if a sample needs refreshing + */ export const useShouldReloadSampleOnActiveFieldsChange = ({ modal, }: { modal: boolean; }) => {
33-41: Consider adding error handling for cleanup.The cleanup logic is correct but could benefit from error handling to ensure robustness.
useEffect(() => { return () => { + try { if (modal) { modalActivePathsLUT.clear(); } else { gridActivePathsLUT.clear(); } + } catch (error) { + console.error('Failed to clear lookup table:', error); + } }; }, []);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/core/src/components/Modal/use-looker.ts(2 hunks)app/packages/core/src/components/Sidebar/useShouldReloadSample.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/components/Sidebar/useShouldReloadSample.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/core/src/components/Modal/use-looker.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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint / eslint
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: build / changes
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (1)
app/packages/core/src/components/Modal/use-looker.ts (1)
55-57: LGTM! Clear configuration of reload hook.The hook configuration with
modal: truecorrectly identifies this as a modal context, aligning with the PR's objective of selective label rendering.
sashankaryal
force-pushed
the
feat/app-config-default-labels-visibility
branch
2 times, most recently
from
65def39 to
9eeac97
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
app/packages/app/src/components/Analytics.tsx (1)
46-54: LGTM! Well-documented test environment handling.The conditional check effectively disables analytics during Playwright tests, with clear comments explaining this exceptional case. The approach is pragmatic for improving test performance.
However, consider adding a type definition for the
IS_PLAYWRIGHTwindow property to improve type safety.+// Add to a global.d.ts file +interface Window { + IS_PLAYWRIGHT?: boolean; +}e2e-pw/src/oss/fixtures/loader.ts (1)
130-135: LGTM! Clean implementation of test environment flag.The initialization script is properly placed to ensure early setup of the IS_PLAYWRIGHT flag.
Consider adding a type definition instead of using @ts-ignore:
+// Add to a global.d.ts file +interface Window { + IS_PLAYWRIGHT?: boolean; +}app/packages/core/src/components/QueryPerformanceToast.tsx (1)
80-88: LGTM, but consider adding type safety for the global variable.The conditional check effectively prevents test delays. However, the
window.IS_PLAYWRIGHTproperty should be properly typed.Add type declaration for the global property:
declare global { interface Window { IS_PLAYWRIGHT?: boolean; } }app/packages/state/src/recoil/schema.ts (2)
325-378: Well-structured implementation with room for performance optimization.The selector efficiently handles different visibility configurations using Set operations. However, we can optimize the array spread operations.
Consider this optimization:
- const denseLabels = [...denseLabelsSamples, ...denseLabelsFrames]; - const allLabels = [...allSampleLabels, ...allFrameLabels]; + const denseLabels = denseLabelsSamples.concat(denseLabelsFrames); + const allLabels = allSampleLabels.concat(allFrameLabels);
325-378: Enhance code documentation for better maintainability.While the logic is clear, adding JSDoc comments would improve maintainability.
Consider adding documentation:
+/** + * Selector that computes the list of visible labels based on configuration. + * + * @returns {string[]} Array of visible label paths + * + * Configuration cases: + * 1. No config: Hide dense labels + * 2. Include only: Show only included labels + * 3. Exclude only: Show all except excluded + * 4. Both: Show (included - excluded) + */ export const defaultVisibleLabels = selector({app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (1)
12-44: Add TypeScript return type annotation to the hook.The hook implementation follows React best practices, but could benefit from explicit TypeScript return type annotation for better type safety and documentation.
-export const useShouldReloadSampleOnActiveFieldsChange = ({ +export const useShouldReloadSampleOnActiveFieldsChange = ({ modal, }: { modal: boolean; -}) => { +}): (id: string) => boolean => {app/packages/looker/src/worker/index.ts (2)
Line range hint
52-71: Enhance documentation for the activePaths parameter.While the function steps are well documented, consider adding a description of the
activePathsparameter and its role in the processing logic.
96-121: Refactor duplicated active paths check into a helper function.The active paths check logic is duplicated. Consider extracting it into a helper function to improve maintainability and reduce code duplication.
+const isFieldActive = (field: string, prefix: string | null, activePaths: string[]): boolean => { + return activePaths.length && activePaths.includes(`${prefix ?? ""}${field}`); +}; // Then use it in both places: -if ( - activePaths.length && - activePaths.includes(`${prefix ?? ""}${field}`) -) { +if (isFieldActive(field, prefix, activePaths)) {Also applies to: 174-191
app/packages/looker/src/lookers/abstract.ts (4)
49-50: Add documentation for the module-scoped Set.While the module-scoped Set is intentional (as confirmed by retrieved learning), consider adding documentation to explain its purpose in preventing concurrent updates across different instances.
+/** + * A shared Set to track sample IDs that are currently being updated across all instances + * of AbstractLooker. This is intentionally module-scoped to prevent concurrent updates + * of the same sample across different instances, avoiding potential race conditions. + */ const UPDATING_SAMPLES_IDS = new Set();
522-532: Add error handling for sample updates.Consider adding error handling and a maximum retry count to prevent infinite retry loops in case of persistent issues.
+private maxRetries = 3; +private retryCount = new Map<string, number>(); updateSample(sample: Sample) { const id = sample.id ?? sample._id; if (UPDATING_SAMPLES_IDS.has(id)) { + const currentRetries = this.retryCount.get(id) ?? 0; + if (currentRetries >= this.maxRetries) { + console.error(`Max retries reached for sample ${id}`); + this.retryCount.delete(id); + return; + } + this.retryCount.set(id, currentRetries + 1); UPDATING_SAMPLES_IDS.delete(id); this.cleanOverlays(true); queueMicrotask(() => this.updateSample(sample)); return; } + this.retryCount.delete(id); UPDATING_SAMPLES_IDS.add(id);
536-543: Address the TODO comment regarding potential crashes.The TODO comment indicates a bug where the instance in spotlight?.updateItems() might have an undefined sample reference, leading to crashes.
Would you like me to help investigate this issue and propose a solution? I can help create a GitHub issue to track this bug.
726-729: Add JSDoc for the setTargetsToNull parameter.Consider adding documentation to explain the purpose and impact of the
setTargetsToNullparameter.+/** + * Cleans up overlays and optionally nullifies their targets. + * @param setTargetsToNull When true, overlay targets will be set to null during cleanup + */ protected cleanOverlays(setTargetsToNull = false) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/datasetAppConfigFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (36)
.gitignore(1 hunks)app/packages/app/src/components/Analytics.tsx(1 hunks)app/packages/core/src/components/Dataset.tsx(1 hunks)app/packages/core/src/components/Grid/Grid.tsx(4 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(2 hunks)app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/core/src/components/Modal/use-looker.ts(2 hunks)app/packages/core/src/components/QueryPerformanceToast.tsx(1 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts(1 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts(1 hunks)app/packages/core/src/components/Sidebar/useShouldReloadSample.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/lookers/frame-reader.ts(3 hunks)app/packages/looker/src/lookers/video.ts(1 hunks)app/packages/looker/src/overlays/base.ts(1 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(1 hunks)app/packages/looker/src/overlays/segmentation.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/index.ts(18 hunks)app/packages/looker/src/worker/painter.ts(8 hunks)app/packages/looker/src/worker/resolve-color.ts(1 hunks)app/packages/looker/src/worker/shared.ts(1 hunks)app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts(0 hunks)app/packages/relay/src/fragments/datasetAppConfigFragment.ts(1 hunks)app/packages/state/src/recoil/schema.ts(4 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)app/packages/state/src/recoil/types.ts(2 hunks)app/packages/utilities/src/schema.ts(3 hunks)app/schema.graphql(2 hunks)e2e-pw/src/oss/fixtures/loader.ts(1 hunks)e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts(1 hunks)e2e-pw/src/oss/specs/sidebar/sidebar.spec.ts(4 hunks)e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts(1 hunks)fiftyone/core/odm/dataset.py(2 hunks)fiftyone/server/query.py(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- app/packages/core/src/components/Dataset.tsx
- app/packages/state/src/recoil/selectors.ts
- .gitignore
- app/packages/looker/src/overlays/base.ts
- app/packages/core/src/components/Modal/use-looker.ts
- app/packages/relay/src/fragments/datasetAppConfigFragment.ts
- app/packages/core/src/components/Grid/useSelect.ts
- app/packages/looker/src/lookers/frame-reader.ts
- app/packages/looker/src/worker/shared.ts
- app/packages/looker/src/lookers/video.ts
- app/packages/looker/src/overlays/segmentation.ts
- fiftyone/server/query.py
- app/packages/core/src/components/Grid/Grid.tsx
- app/packages/state/src/recoil/types.ts
- fiftyone/core/odm/dataset.py
- app/packages/looker/src/worker/painter.ts
- app/packages/looker/src/worker/resolve-color.ts
- app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts
- app/packages/looker/src/worker/disk-overlay-decoder.ts
- app/packages/looker/src/overlays/detection.ts
- app/packages/looker/src/overlays/heatmap.ts
- app/packages/core/src/components/Grid/useRefreshers.ts
- app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts
🧰 Additional context used
📓 Path-based instructions (11)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.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/app/src/components/Analytics.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
e2e-pw/src/oss/fixtures/loader.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/core/src/components/QueryPerformanceToast.tsx (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/state/src/recoil/schema.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.
e2e-pw/src/oss/specs/smoke-tests/tagger.spec.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/lookers/abstract.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/index.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/core/src/components/Sidebar/useShouldReloadSample.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.
e2e-pw/src/oss/specs/sidebar/sidebar.spec.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/utilities/src/schema.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.
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (14)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts (1)
93-95: LGTM! Proper test setup for default label visibility.The configuration correctly demonstrates the new default visibility labels feature, properly including both "ground_truth" and "prediction" labels.
e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (1)
37-40: LGTM! Proper test setup for label visibility.The changes ensure a consistent test state by explicitly setting visibility for required labels.
e2e-pw/src/oss/specs/sidebar/sidebar.spec.ts (1)
36-37: LGTM! Consistent updates across test cases.The changes consistently update all test cases to show only ground truth labels, aligning with the PR objectives for selective label rendering.
Also applies to: 92-93, 150-151, 208-209
app/packages/utilities/src/schema.ts (3)
1-12: LGTM! Well-organized constants for label types.The constants are clearly defined and logically grouped into the
DENSE_LABEL_EMBEDDED_DOC_TYPESarray.
70-92: LGTM! Enhanced function with improved flexibility.The
getFieldsWithEmbeddedDocTypefunction now supports:
- Array of embedded doc types
- Optional recursion control
116-124: LGTM! Clean utility function implementation.The
getDenseLabelNamesfunction is focused and effectively reuses existing functionality.app/schema.graphql (2)
333-336: LGTM! Well-structured visibility configuration type.The
FieldVisibilityConfigtype clearly defines the include/exclude pattern with non-nullable arrays.
250-250: LGTM! Clean integration with DatasetAppConfig.The
defaultVisibilityLabelsfield is properly added to theDatasetAppConfigtype.app/packages/state/src/recoil/schema.ts (2)
26-26: LGTM! Clean import additions.The new imports are well-organized and properly aligned with the feature requirements.
Also applies to: 35-35
573-573: LGTM! Clean integration of default visibility.The fallback to
defaultVisibleLabelsis properly implemented and aligns with the feature requirements.app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (3)
1-5: LGTM!The imports are well-organized and include all necessary dependencies for the hook's functionality.
6-8: LGTM!The type definitions are clear, descriptive, and appropriately typed.
9-10: LGTM!The lookup tables are well-structured with appropriate types and clear naming conventions.
app/packages/looker/src/worker/index.ts (1)
33-33: LGTM!Good refactoring to move color resolution logic into a separate module, improving code organization and maintainability.
Also applies to: 45-45
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/packages/state/src/labelsVisibility.ts (3)
27-27: Remove console.log statement.Production code should not contain console.log statements.
- console.log("all", all);
19-57: Optimize label filtering using Set operations.The current implementation uses array operations which have O(n²) complexity. Using Set operations would improve performance.
- const allLabels = [...allSampleLabels, ...allFrameLabels]; + const allLabelsSet = new Set([...allSampleLabels, ...allFrameLabels]); + const denseLabelsSet = new Set(denseLabels); if ( !defaultVisibleLabelsConfig?.include && !defaultVisibleLabelsConfig?.exclude ) { - const all = allLabels.filter((label) => !denseLabels.includes(label)); - return all; + return [...allLabelsSet].filter((label) => !denseLabelsSet.has(label)); } if ( defaultVisibleLabelsConfig?.include && !defaultVisibleLabelsConfig?.exclude ) { - return allLabels.filter((label) => - defaultVisibleLabelsConfig.include.includes(label) - ); + const includeSet = new Set(defaultVisibleLabelsConfig.include); + return [...allLabelsSet].filter((label) => includeSet.has(label)); } if ( !defaultVisibleLabelsConfig?.include && defaultVisibleLabelsConfig?.exclude ) { - return allLabels.filter( - (label) => !defaultVisibleLabelsConfig.exclude.includes(label) - ); + const excludeSet = new Set(defaultVisibleLabelsConfig.exclude); + return [...allLabelsSet].filter((label) => !excludeSet.has(label)); }
3-12: Add JSDoc documentation.Add comprehensive documentation to explain the function's purpose, parameters, and return value.
+/** + * Computes the list of labels that should be visible based on the provided schemas and configuration. + * @param sampleSchema - The schema for sample labels + * @param frameSchema - The schema for frame labels + * @param allSampleLabels - Array of all sample label names + * @param allFrameLabels - Array of all frame label names + * @param defaultVisibleLabelsConfig - Optional configuration for label visibility + * @param defaultVisibleLabelsConfig.include - Optional array of labels to include + * @param defaultVisibleLabelsConfig.exclude - Optional array of labels to exclude + * @returns Array of label names that should be visible + */ export const computeDefaultVisibleLabels = (app/packages/state/src/labelsVisibility.test.ts (1)
40-54: Enhance test coverage with edge cases.While the current test cases cover the main scenarios, consider adding tests for:
- Empty arrays in include/exclude configs
- Non-existent labels in include/exclude configs
- Overlapping labels in include/exclude configs
- Empty input label arrays
Here's an example test case for empty configs:
it("handles empty include/exclude arrays", () => { const allSampleLabels = ["detection", "classification"]; const allFrameLabels = ["frames.detection"]; const result = computeDefaultVisibleLabels( sampleSchemaMock, frameSchemaMock, allSampleLabels, allFrameLabels, { include: [], exclude: [] } ); expect(result).toEqual([]); });Also applies to: 56-69, 71-84, 86-102
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/state/src/labelsVisibility.test.ts(1 hunks)app/packages/state/src/labelsVisibility.ts(1 hunks)app/packages/state/src/recoil/schema.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/state/src/labelsVisibility.test.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/state/src/labelsVisibility.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/state/src/recoil/schema.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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (1)
app/packages/state/src/recoil/schema.ts (1)
325-344: Clean implementation of label visibility logic.The selector efficiently handles different visibility configurations and integrates well with the existing state management pattern.
Also applies to: 539-539
sashankaryal
force-pushed
the
feat/app-config-default-labels-visibility
branch
from
560c340 to
fc65aff
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
app/packages/core/src/components/Modal/use-looker.ts (1)
55-67: Consider debouncing the sample refresh calls.The effect could trigger multiple rapid refreshes when
activePathschanges frequently, potentially causing performance issues or race conditions. Consider implementing debouncing to coalesce multiple refresh calls.Here's a suggested implementation using debounce:
+import { debounce } from "lodash"; + const shouldRefresh = useShouldReloadSampleOnActiveFieldsChange({ modal: true, }); +const debouncedRefresh = React.useCallback( + debounce((looker: L) => { + looker.refreshSample(); + }, 250), + [] +); + useEffect(() => { if (!looker) { return; } if (shouldRefresh(id)) { - looker?.refreshSample(); + debouncedRefresh(looker); } + + return () => { + debouncedRefresh.cancel(); + }; }, [id, lookerOptions.activePaths, looker]);This change will:
- Prevent rapid consecutive refreshes
- Improve performance by reducing unnecessary refreshes
- Help mitigate potential race conditions
app/packages/state/src/labelsVisibility.ts (2)
27-27: Remove debug console.log statement.The console.log statement should be removed from production code.
- console.log("all", all); + return all;
51-56: Consider using Set operations for better performance.When both include and exclude are defined, you're using Set operations efficiently. Consider using the same approach for the default case to maintain consistency and potentially improve performance with large label lists.
- const resolved = new Set([...includeList].filter((x) => !excludeList.has(x))); + const resolved = new Set( + [...includeList].filter((x) => !excludeList.has(x)) + );app/packages/state/src/labelsVisibility.test.ts (1)
40-103: Add test cases for edge cases and error conditions.While the current test coverage is good, consider adding these test cases:
- Empty arrays for
allSampleLabelsandallFrameLabels- Empty arrays for
includeandexclude- Invalid input validation (e.g., undefined schemas)
Example additional test cases:
it("handles empty label arrays", () => { const result = computeDefaultVisibleLabels( sampleSchemaMock, frameSchemaMock, [], [], undefined ); expect(result).toEqual([]); }); it("handles empty include/exclude arrays", () => { const result = computeDefaultVisibleLabels( sampleSchemaMock, frameSchemaMock, ["detection"], [], { include: [], exclude: [] } ); expect(result).toEqual([]); });app/packages/looker/src/worker/resolve-color.ts (2)
15-17: Consider adding cache eviction callback.The LRU cache is well-configured with a reasonable size limit, but consider adding a
disposecallback to clean up any resources when entries are evicted.const cache = new LRUCache<number, Record<string | number, string>>({ max: MAX_COLOR_CACHE_SIZE, + dispose: (key, value) => { + // Clean up any resources associated with the evicted entry + }, });
19-20: Consider using Map for type safety.Using plain objects for storing requests and promises could lead to prototype pollution. Consider using
Mapfor better type safety.- const requests = {}; - const promises = {}; + const requests = new Map(); + const promises = new Map();app/packages/looker/src/processOverlays.ts (1)
36-45: Consider consolidating overlay validation logic.The validation logic for Segmentation and Heatmap overlays is duplicated. Consider extracting it into a shared validation function.
+const validateOverlayData = (overlay, requiredField, pathField) => { + return overlay[pathField] && !overlay[requiredField]; +}; -if ( - overlay instanceof SegmentationOverlay && - overlay.label.mask_path && - !overlay.label.mask -) { +if (overlay instanceof SegmentationOverlay && + validateOverlayData(overlay.label, 'mask', 'mask_path')) { continue; } -if ( - overlay instanceof HeatmapOverlay && - overlay.label.map_path && - !overlay.label.map -) { +if (overlay instanceof HeatmapOverlay && + validateOverlayData(overlay.label, 'map', 'map_path')) { continue; }Also applies to: 47-53
app/packages/looker/src/worker/index.ts (1)
96-121: Consider adding early return for inactive paths.The nested conditions could be simplified using an early return pattern.
-if ( - activePaths.length && - activePaths.includes(`${prefix ?? ""}${field}`) -) { +const isActivePath = activePaths.length && activePaths.includes(`${prefix ?? ""}${field}`); +if (!isActivePath) { + label.renderStatus = null; + continue; +} maskPathDecodingPromises.push( decodeOverlayOnDisk( // ... existing code ) ); if (cls in DeserializerFactory) { DeserializerFactory[cls](label, maskTargetsBuffers); label.renderStatus = "decoded"; } -} else { - label.renderStatus = null; -}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/datasetAppConfigFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (36)
.gitignore(1 hunks)app/packages/core/src/components/Dataset.tsx(1 hunks)app/packages/core/src/components/Grid/Grid.tsx(4 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(2 hunks)app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/core/src/components/Modal/use-looker.ts(2 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts(1 hunks)app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts(1 hunks)app/packages/core/src/components/Sidebar/useShouldReloadSample.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/lookers/frame-reader.ts(3 hunks)app/packages/looker/src/lookers/video.ts(1 hunks)app/packages/looker/src/overlays/base.ts(1 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(1 hunks)app/packages/looker/src/overlays/segmentation.ts(1 hunks)app/packages/looker/src/processOverlays.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/index.ts(18 hunks)app/packages/looker/src/worker/painter.ts(8 hunks)app/packages/looker/src/worker/resolve-color.ts(1 hunks)app/packages/looker/src/worker/shared.ts(1 hunks)app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts(0 hunks)app/packages/relay/src/fragments/datasetAppConfigFragment.ts(1 hunks)app/packages/state/src/labelsVisibility.test.ts(1 hunks)app/packages/state/src/labelsVisibility.ts(1 hunks)app/packages/state/src/recoil/schema.ts(3 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)app/packages/state/src/recoil/types.ts(2 hunks)app/packages/utilities/src/schema.ts(3 hunks)app/schema.graphql(2 hunks)e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts(1 hunks)e2e-pw/src/oss/specs/sidebar/sidebar.spec.ts(4 hunks)e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts(1 hunks)fiftyone/core/odm/dataset.py(2 hunks)fiftyone/server/query.py(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/tests/indexed-png-decoder.test.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- app/packages/looker/src/worker/shared.ts
- app/packages/state/src/recoil/selectors.ts
- .gitignore
- app/packages/core/src/components/Dataset.tsx
- app/schema.graphql
- app/packages/relay/src/fragments/datasetAppConfigFragment.ts
- app/packages/core/src/components/Grid/useSelect.ts
- e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts
- app/packages/looker/src/overlays/segmentation.ts
- app/packages/looker/src/overlays/heatmap.ts
- app/packages/looker/src/lookers/frame-reader.ts
- app/packages/state/src/recoil/types.ts
- app/packages/looker/src/overlays/detection.ts
- app/packages/looker/src/worker/painter.ts
- app/packages/core/src/components/Grid/Grid.tsx
- app/packages/looker/src/lookers/video.ts
- e2e-pw/src/oss/specs/sidebar/sidebar.spec.ts
- fiftyone/server/query.py
- app/packages/core/src/components/Grid/useRefreshers.ts
- e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts
- app/packages/looker/src/overlays/base.ts
- app/packages/looker/src/worker/disk-overlay-decoder.ts
- app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts
- app/packages/core/src/components/Sidebar/useShouldReloadSample.ts
- app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts
- fiftyone/core/odm/dataset.py
🧰 Additional context used
📓 Path-based instructions (9)
app/packages/state/src/labelsVisibility.test.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/core/src/components/Modal/use-looker.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/state/src/recoil/schema.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/processOverlays.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/resolve-color.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/lookers/abstract.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/state/src/labelsVisibility.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/utilities/src/schema.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/index.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.
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (13)
app/packages/core/src/components/Modal/use-looker.ts (1)
6-6: LGTM!Clean import statement following the project's import conventions.
app/packages/utilities/src/schema.ts (3)
1-12: LGTM! Well-structured constants and exports.The constants are properly defined and exported, following TypeScript best practices.
68-95: LGTM! Clean function modification.The changes to
getFieldsWithEmbeddedDocTypemaintain backward compatibility while adding new functionality. The code is well-structured and handles both string and array inputs efficiently.
116-124: LGTM! Clean utility function implementation.The
getDenseLabelNamesfunction follows single responsibility principle and effectively uses the modifiedgetFieldsWithEmbeddedDocTypefunction.app/packages/state/src/recoil/schema.ts (2)
325-344: LGTM! Clean Recoil selector implementation.The
defaultVisibleLabelsselector is well-implemented:
- Proper memoization through Recoil's selector
- Clean integration with existing selectors
- Follows Recoil best practices
533-548: LGTM! Clean integration with existing functionality.The changes to
activeFieldsselector cleanly integrate the new default visibility functionality while maintaining existing behavior.app/packages/looker/src/worker/resolve-color.ts (1)
5-9: LGTM! Well-defined interface.The
ResolveColorinterface clearly defines the contract for color resolution with appropriate types.app/packages/looker/src/processOverlays.ts (2)
34-35: LGTM! Good validation.The field validation ensures that only valid overlays are processed.
106-126: LGTM! Well-structured filter function.The filter function effectively validates overlays and follows the same pattern as the main processing logic.
app/packages/looker/src/worker/index.ts (2)
33-45: LGTM! Clean color resolution integration.The color resolution is well-integrated with the worker system.
396-398: LGTM! Proper type extension.The
FrameChunkResponseinterface is correctly extended with theactivePathsproperty.app/packages/looker/src/lookers/abstract.ts (2)
49-50: LGTM! Well-documented shared state.The module-scoped
UPDATING_SAMPLES_IDSset is intentionally shared to prevent concurrent updates across instances.
726-730: LGTM! Clean overlay cleanup.The
cleanOverlaysmethod properly handles the newsetTargetsToNullparameter.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (1)
12-44: Consider adding error boundaries for Map operations.While the implementation is solid, Map operations could potentially throw if the lookerId is invalid.
const getNewFields = useCallback( (id: string) => { + try { return syncAndGetNewLabels( id, modal ? modalActivePathsLUT : gridActivePathsLUT, new Set(activeLabelFieldsValue) ); + } catch (error) { + console.error('Error syncing label fields:', error); + return null; + } }, [activeLabelFieldsValue] );app/packages/core/src/components/Sidebar/syncAndGetNewLabels.ts (1)
12-48: LGTM! Well-implemented synchronization logic.The function is well-documented, has proper error handling, and follows best practices for performance optimization with early returns.
Consider adding debug logging for easier troubleshooting:
export const syncAndGetNewLabels = ( lookerId: string, lut: Map<LookerId, CachedLabels>, currentActiveLabelFields: Set<string> ): string[] | null => { + const debug = (msg: string, ...args: any[]) => console.debug(`[syncAndGetNewLabels] ${msg}`, ...args); + if (currentActiveLabelFields.size === 0) { + debug('No active label fields'); return null; }app/packages/core/src/components/Sidebar/syncAndGetNewLabels.test.ts (1)
4-87: LGTM! Comprehensive test coverage.The test suite is well-structured and covers all important scenarios including edge cases.
Consider adding tests for error cases:
+ it("handles invalid looker IDs gracefully", () => { + const id = ""; + const currentActiveLabelFields = new Set(["field1"]); + + const result = syncAndGetNewLabels(id, lut, currentActiveLabelFields); + expect(result).not.toBeUndefined(); + });app/packages/core/src/components/Modal/use-looker.ts (1)
55-69: Consider memoizing getNewFields callback.The effect implementation is correct, but the
getNewFieldscallback could be memoized to prevent unnecessary re-renders.- const getNewFields = useShouldReloadSampleOnActiveFieldsChange({ + const getNewFields = React.useMemo( + () => useShouldReloadSampleOnActiveFieldsChange({ modal: true, - }); + }), + [] + );app/packages/looker/src/worker/async-labels-rendering-manager.ts (1)
29-38: Consider a more efficient strategy for large queuesThis loop removes jobs while iterating over
jobQueue, which may become inefficient for large queues. You could optimize this logic by using a dedicated queue structure (e.g., a linked list or a lightweight FIFO queue) to reduce time complexity for removals at arbitrary positions.app/packages/looker/src/worker/async-labels-rendering-manager.test.ts (2)
23-40: Potential test flakiness from fixed timeoutsThe
setTimeout(..., 10)usage could lead to test flakiness in slower environments. Consider using mock timers or other deterministic synchronization methods to ensure consistent results.
111-129: Error scenario coverageThe test clarifies that worker errors bubble up as promises being rejected. You may consider adding an assertion to ensure partial updates do not remain in a corrupted state, but otherwise this coverage looks solid.
app/packages/looker/src/worker/index.ts (3)
33-46: Centralize color resolution initialization for clarityUsing
[requestColor, resolveColor] = colorResolve();is concise. Consider adding a short inline doc to clarify how color resolution is injected and used, especially since it’s central to label rendering.
51-74: Fix typographical error in comment“precendence” should be “precedence”. Additionally, consider clarifying how the
renderStatusoverridesactivePathsto make the logic explicit.- // it means this label was processed before and we should re-render it - // it takes precendence over activePaths + // It means this label was processed before and we should re-render it + // It takes precedence over activePaths
602-603: New property in SetStreamAdding
activePathsto the stream configuration is an effective way to control label processing. Ensure the upstream usage sets a default (e.g., an empty array) when not explicitly provided.app/packages/looker/src/lookers/abstract.ts (1)
110-169: Consider removing the type cast in the constructor.The type cast
this as unknown as Lookersmight be unnecessary. Consider extending the class fromLookersor updating theAsyncLabelsRenderingManagerconstructor to acceptAbstractLooker.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/core/src/components/Modal/use-looker.ts(2 hunks)app/packages/core/src/components/Sidebar/syncAndGetNewLabels.test.ts(1 hunks)app/packages/core/src/components/Sidebar/syncAndGetNewLabels.ts(1 hunks)app/packages/core/src/components/Sidebar/useShouldReloadSample.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(7 hunks)app/packages/looker/src/worker/async-labels-rendering-manager.test.ts(1 hunks)app/packages/looker/src/worker/async-labels-rendering-manager.ts(1 hunks)app/packages/looker/src/worker/index.ts(18 hunks)app/packages/looker/src/worker/painter.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/packages/core/src/components/Grid/useSelect.ts
- app/packages/looker/src/worker/painter.ts
🧰 Additional context used
📓 Path-based instructions (8)
app/packages/core/src/components/Modal/use-looker.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/core/src/components/Sidebar/useShouldReloadSample.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/index.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/async-labels-rendering-manager.test.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/lookers/abstract.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/core/src/components/Sidebar/syncAndGetNewLabels.test.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/async-labels-rendering-manager.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/core/src/components/Sidebar/syncAndGetNewLabels.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.
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (12)
app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (1)
6-10: LGTM! Well-defined types and clear exports.The type definitions are clear and the Map declarations follow TypeScript best practices.
app/packages/core/src/components/Modal/use-looker.ts (1)
1-6: LGTM! Clean import organization.The imports are well-organized and the new hook import is properly placed.
app/packages/looker/src/worker/async-labels-rendering-manager.ts (4)
49-65: Ensure active job removal logic is robustInside the
whileloop, you remove a job fromjobQueueand break. This is correct for immediate processing, but be mindful of potential edge cases where multiple jobs might share the same sample reference. A robust approach would confirm that subsequent jobs referencing the same sample are appropriately updated or merged.
103-107: Validate keys filtering logicWhen filtering out keys that are not in
job.labels, confirm that no vital metadata is accidentally removed from the sample object (e.g., metadata necessary for error handling or logging).
137-150: Check concurrency around shared referencesThe code pushes new jobs into the global queue and updates shared structures (
pendingJobs,processingSamples). Although this runs in a single thread in most workers, confirm that external race conditions or re-entrant calls (e.g., from message event handlers) won’t lead to inconsistent states.
172-178: Good testing utility for state resetThe
_internal_resetForTestsmethod is a clean choice to ensure consistent state between tests. No issues observed here.app/packages/looker/src/worker/async-labels-rendering-manager.test.ts (2)
86-99: Meaningful assertion on merged sampleThe test assertion thoroughly checks that the original field (“b”) remains intact while “a” and “c” are transformed. This is a clear demonstration of the merge behavior; nice work!
101-109: Good coverage of processing state checksVerifying
isProcessing()across asynchronous transitions is important. Overall logic appears correct.app/packages/looker/src/worker/index.ts (2)
95-96: Include usage checks for new parametersThe new
schemaandactivePathsparameters add complexity to downstream calls. Confirm that all call sites provide valid data, especially ifactivePathscan be empty or undefined.
205-212: Confirm painting logic syncs with decoding phase
shouldProcessLabelis invoked again before painting. Ensure that if a label was not decoded (e.g.,renderStatus = null), you do not attempt to paint it prematurely.app/packages/looker/src/lookers/abstract.ts (2)
48-51: LGTM!The module-scoped
UPDATING_SAMPLES_IDSset is intentionally shared across instances to prevent concurrent updates of the same sample.
758-761: LGTM!The
setTargetsToNullparameter is well-typed and properly used in the cleanup method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/looker/src/lookers/abstract.ts (2)
48-51: LGTM! Consider adding documentation for UPDATING_SAMPLES_IDS.The module-scoped Set is intentionally used to prevent concurrent updates of the same sample across different instances of AbstractLooker.
Add this documentation:
+/** + * A shared set to track sample IDs that are currently being updated across all instances + * of AbstractLooker. This is intentionally module-scoped to prevent concurrent updates + * of the same sample across different instances, avoiding potential race conditions + * when the same sample is being processed by multiple lookers. + */ const UPDATING_SAMPLES_IDS = new Set();
532-542: Add timeout and error handling for concurrent updates.While the concurrent update prevention is good, consider these improvements:
- Add a timeout to prevent deadlock if an error occurs
- Add error handling in the microtask
Apply this diff:
const id = sample.id ?? sample._id; +const UPDATE_TIMEOUT = 5000; // 5 seconds if (UPDATING_SAMPLES_IDS.has(id)) { UPDATING_SAMPLES_IDS.delete(id); this.cleanOverlays(true); - queueMicrotask(() => this.updateSample(sample)); + const timeoutId = setTimeout(() => { + UPDATING_SAMPLES_IDS.delete(id); + }, UPDATE_TIMEOUT); + queueMicrotask(() => { + try { + clearTimeout(timeoutId); + this.updateSample(sample); + } catch (error) { + UPDATING_SAMPLES_IDS.delete(id); + this.updater({ error }); + } + }); return; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/looker/src/lookers/abstract.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/lookers/abstract.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.
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: lint / eslint
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (3)
app/packages/looker/src/lookers/abstract.ts (3)
110-113: LGTM! The instance properties and initialization look good.The state property is correctly marked as internal, and the asyncLabelsRenderingManager is properly initialized in the constructor.
Also applies to: 167-169
759-762: LGTM! The cleanOverlays method changes look good.The addition of the
setTargetsToNullparameter provides better control over the cleanup behavior.
546-576: 🛠️ Refactor suggestionAdd comprehensive error handling and simplify the promise chain.
The method could be improved with better error handling and a simpler promise chain.
Apply this diff:
refreshSample(renderLabels: string[] | null) { - // todo: sometimes instance in spotlight?.updateItems() is defined but has no ref to sample - // this crashes the app. this is a bug and should be fixed if (!this.sample) { + console.warn('Attempted to refresh undefined sample'); return; } if (!renderLabels?.length) { - this.updateSample(this.sample); + try { + this.updateSample(this.sample); + } catch (error) { + this.updater({ error }); + } + return; } - this.asyncLabelsRenderingManager - .enqueueLabelPaintingJob({ + return this.asyncLabelsRenderingManager + .enqueueLabelPaintingJob({ sample: this.sample, labels: renderLabels, }) .then(({ sample, coloring }) => { this.sample = sample; this.state.options.coloring = coloring; this.loadOverlays(sample); - // to run looker reconciliation this.updater({ overlaysPrepared: true, }); }) .catch((error) => { this.updater({ error }); + throw error; // Re-throw to allow caller to handle the error }); }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/packages/core/src/components/Grid/useSelect.ts (2)
38-69: Add missing dependency to useEffect and consider refactoring for maintainability.The label rendering logic implementation looks solid, but there are a few improvements to consider:
- The
getNewFieldsfunction is missing from the useEffect dependency array on line 77.- The complex logic could be extracted into separate functions for better maintainability.
Consider refactoring the code like this:
+ const handleNewFields = (instance, newFields, overlays) => { + const thisInstanceOverlays = overlays.filter( + (o) => o.field && newFields.includes(o.field) + ); + + thisInstanceOverlays?.forEach((o) => { + if (o.label) { + o.label.renderStatus = "pending"; + } + }); + + if (thisInstanceOverlays?.length > 0) { + instance.refreshSample(newFields); + } + }; + + const handlePendingLabels = (instance, overlays) => { + const pending = overlays.filter( + (o) => o.field && o.label && o.label.renderStatus === "pending" + ); + + if (pending?.length > 0) { + const rerenderFields = pending.map((o) => o.field!); + instance.refreshSample(rerenderFields); + } + }; const newFieldsIfAny = getNewFields(id.description); const overlays = instance.getSampleOverlays() ?? []; if (newFieldsIfAny) { - const thisInstanceOverlays = overlays.filter( - (o) => o.field && newFieldsIfAny.includes(o.field) - ); - - thisInstanceOverlays?.forEach((o) => { - if (o.label) { - o.label.renderStatus = "pending"; - } - }); - - if (thisInstanceOverlays?.length > 0) { - instance.refreshSample(newFieldsIfAny); - } + handleNewFields(instance, newFieldsIfAny, overlays); } else { - const pending = overlays.filter( - (o) => o.field && o.label && o.label.renderStatus === "pending" - ); - - if (pending?.length > 0) { - const rerenderFields = pending.map((o) => o.field!); - instance.refreshSample(rerenderFields); - } + handlePendingLabels(instance, overlays); }Also update the dependency array:
- }, [deferred, getFontSize, options, selected, spotlight, store]); + }, [deferred, getFontSize, getNewFields, options, selected, spotlight, store]);
50-52: Document the potential race condition.The comment explains the "pending" status, but it would be helpful to document the known race condition mentioned in the PR objectives.
Add a note about the race condition:
// "pending" means we're marking this label for rendering / painting // even if it's interrupted, say by unchecking sidebar + // Note: There's a known race condition that may occur if recoloring happens too quicklyapp/packages/looker/src/lookers/abstract.ts (2)
51-51: LGTM! Consider adding documentation.The module-scoped set is intentionally designed to prevent concurrent updates of the same sample across different instances.
Add this documentation to clarify the intention:
+/** + * A shared set to track sample IDs that are currently being updated across all instances + * of AbstractLooker. This is intentionally module-scoped to prevent concurrent updates + * of the same sample across different instances, avoiding potential race conditions. + */ const UPDATING_SAMPLES_IDS = new Set();
199-201: Add JSDoc documentation.Consider adding documentation to clarify the purpose and return type of this method.
Add this documentation:
+/** + * Returns the current sample overlays. + * @returns {Overlay<State>[]} Array of overlays for the current sample + */ getSampleOverlays(): Overlay<State>[] { return this.sampleOverlays; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/looker/src/lookers/abstract.ts(9 hunks)app/packages/looker/src/overlays/base.ts(3 hunks)app/packages/looker/src/worker/index.ts(18 hunks)app/packages/looker/src/worker/shared.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/packages/looker/src/worker/shared.ts
- app/packages/looker/src/overlays/base.ts
- app/packages/looker/src/worker/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/components/Grid/useSelect.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/lookers/abstract.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.
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (5)
app/packages/core/src/components/Grid/useSelect.ts (1)
6-6: LGTM! Clean hook initialization.The import and hook initialization follow React best practices and align with the PR objectives for selective label rendering.
Also applies to: 16-18
app/packages/looker/src/lookers/abstract.ts (4)
110-111: Verify the necessity of making state public.While the
@internaltag indicates this is for internal use, making it public could lead to unintended external modifications. Consider keeping it protected unless there's a specific need for public access.Could you explain why this property needs to be public? If it's only needed by specific internal modules, consider using TypeScript's module system to control access instead.
778-781: LGTM! Clean implementation of the cleanup flag.The addition of the
setTargetsToNullparameter provides better control over the cleanup process.
536-561: LGTM! Robust implementation of concurrent update prevention.The changes effectively prevent concurrent updates of the same sample while adding proper error handling and deadlock prevention.
565-595:⚠️ Potential issueAdd error handling and improve promise chain.
The method has several issues that need to be addressed:
- The TODO comment indicates a known crash bug
- Missing error handling when
renderLabelsis empty- Promise chain could be improved
Apply this diff to fix the issues:
refreshSample(renderLabels: string[] | null = null) { - // todo: sometimes instance in spotlight?.updateItems() is defined but has no ref to sample - // this crashes the app. this is a bug and should be fixed if (!this.sample) { + console.warn('Attempted to refresh undefined sample'); return; } if (!renderLabels?.length) { - this.updateSample(this.sample); + try { + this.updateSample(this.sample); + } catch (error) { + this.updater({ error }); + } return; } - this.asyncLabelsRenderingManager + return this.asyncLabelsRenderingManager .enqueueLabelPaintingJob({ sample: this.sample, labels: renderLabels, }) .then(({ sample, coloring }) => { this.sample = sample; this.state.options.coloring = coloring; this.loadOverlays(sample); - // to run looker reconciliation this.updater({ overlaysPrepared: true, }); }) .catch((error) => { this.updater({ error }); + throw error; // Re-throw to allow caller to handle the error }); }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
app/packages/core/src/components/Modal/ImaVidLooker.tsx (3)
308-318: Replace polling with event-based updatesUsing setInterval to poll for frame count is inefficient and can cause unnecessary re-renders.
Consider implementing an event-based system:
- let intervalId = setInterval(() => { - const totalFrameCount = - imaVidLookerRef.current.frameStoreController.totalFrameCount; - if (totalFrameCount) { - setTotalFrameCount(totalFrameCount); - clearInterval(intervalId); - } - }, 10); - - return () => clearInterval(intervalId); + const handleFrameCountUpdate = (count: number) => { + setTotalFrameCount(count); + }; + + imaVidLookerRef.current.frameStoreController.on('frameCountUpdate', handleFrameCountUpdate); + + return () => { + imaVidLookerRef.current.frameStoreController.off('frameCountUpdate', handleFrameCountUpdate); + };
230-241: Fix potential memory leak in readyWhen hookThe interval in readyWhen might not be cleared if the component unmounts before totalFrameCount is resolved.
const readyWhen = useCallback(async () => { return new Promise<void>((resolve) => { - let intervalId; + let intervalId: NodeJS.Timeout; intervalId = setInterval(() => { if (totalFrameCountRef.current) { clearInterval(intervalId); resolve(); } }, 10); + + // Ensure cleanup after 5 seconds to prevent infinite polling + setTimeout(() => { + clearInterval(intervalId); + resolve(); + }, 5000); }); }, []);
217-219:🧩 Analysis chain
Address TODO comments
There are multiple TODO comments indicating incomplete implementation of frame count updates.
Please verify if these TODOs are still relevant and if they should be tracked as issues:
🏁 Script executed:
#!/bin/bash # Search for related frame count implementation in the codebase rg -A 5 "frameCount|totalFrames" --type tsLength of output: 37429
Frame count update mechanism in ImaVidLooker is still incomplete
- The TODO remains in place in
ImaVidLooker.tsxindicating that the mechanism for updating the frame count asynchronously is not fully implemented.- Unlike other parts of the codebase (e.g., in VideoLooker and various looker modules) where total frame counts are calculated and passed consistently, the ImaVidLooker component still expects an event-based update that has not been wired up.
- This discrepancy confirms that the TODO is still relevant and should be tracked as a known issue for follow-up.
🧹 Nitpick comments (1)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (1)
17-21: Add type safety fornewFieldsIfAnyThe
newFieldsIfAnyvariable lacks type information, which could lead to type-related issues.- const newFieldsIfAny = getNewFields(id); + const newFieldsIfAny: string[] | undefined = getNewFields(id); if (newFieldsIfAny) { - looker?.refreshSample(newFieldsIfAny); + looker.refreshSample(newFieldsIfAny); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/core/src/components/Modal/ImaVidLooker.tsx(5 hunks)app/packages/core/src/components/Modal/use-looker.ts(2 hunks)app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(1 hunks)app/packages/state/src/hooks/useLookerStore.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/components/Modal/use-looker.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/hooks/useLookerStore.tsapp/packages/core/src/components/Modal/use-modal-selective-rendering.tsapp/packages/core/src/components/Modal/ImaVidLooker.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (2)
app/packages/state/src/hooks/useLookerStore.ts (1)
1-11: LGTM! Clean type extension.The addition of
ImaVidLookerto theLookerstype union is well-structured and maintains type safety.app/packages/core/src/components/Modal/ImaVidLooker.tsx (1)
320-320: LGTM! Good integration of selective rendering.The integration of
useModalSelectiveRenderingis clean and follows the component's lifecycle management pattern.
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts
Outdated
Show resolved
Hide resolved
| // to prevent deadlock, we'll remove the id from the set after a timeout | ||
| const timeoutId = setTimeout(() => { | ||
| UPDATING_SAMPLES_IDS.delete(id); | ||
| }, updateTimeoutMs); |
There was a problem hiding this comment.
I don't see how this has an effect, noting that id is deleted on 542, as well.
Can UPDATING_SAMPLES_IDS be replaced with state internal to Looker? Like isBatching? Introducing a global store feels somewhat inflexible
There was a problem hiding this comment.
Can UPDATING_SAMPLES_IDS be replace with state internal to Looker?
Yes. lookers and samples have 1:1 relationship, so makes sense 100%. Will attempt in subsequent follow-up PR when I tackle data clone bug, which is related
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (1)
13-23:⚠️ Potential issueAdd missing dependency to useEffect
The
getNewFieldsfunction is used inside useEffect but not included in the dependencies array.- }, [id, lookerOptions.activePaths, looker]); + }, [id, lookerOptions.activePaths, looker, getNewFields]);
🧹 Nitpick comments (4)
app/packages/core/src/components/Modal/ImaVidLooker.tsx (1)
305-318: Replace polling with event-based approachThe current implementation uses polling to check for frame count, which is not optimal. Consider implementing an event-based system as noted in the TODO comments.
- useEffect(() => { - let intervalId = setInterval(() => { - const totalFrameCount = - imaVidLookerRef.current.frameStoreController.totalFrameCount; - if (totalFrameCount) { - setTotalFrameCount(totalFrameCount); - clearInterval(intervalId); - } - }, 10); - - return () => clearInterval(intervalId); - }, [looker]); + useEffect(() => { + const handleFrameCountUpdate = (count: number) => { + setTotalFrameCount(count); + }; + + imaVidLookerRef.current.frameStoreController.on('frameCountUpdate', handleFrameCountUpdate); + + return () => { + imaVidLookerRef.current.frameStoreController.off('frameCountUpdate', handleFrameCountUpdate); + }; + }, [looker]);app/packages/looker/src/lookers/imavid/index.ts (1)
208-213: Add type safety for frameNumber parameter.The frameNumber parameter's type is not specified, and the null check could be more explicit.
- const sampleId = - this.frameStoreController.store.frameIndex.get(frameNumber); - - if (!sampleId) { - return; - } + const sampleId = typeof frameNumber === 'number' + ? this.frameStoreController.store.frameIndex.get(frameNumber) + : null; + + if (sampleId === null || sampleId === undefined) { + console.warn(`No sample found for frame number: ${frameNumber}`); + return; + }app/packages/looker/src/overlays/detection.ts (2)
19-21: Add JSDoc comments for mask-related properties.The mask and mask_path properties lack documentation explaining their purpose and relationship.
export interface DetectionLabel extends RegularLabel { + /** Bitmap mask data for the detection */ mask?: LabelMask; + /** Path to the mask resource */ mask_path?: string;
46-50: Optimize containsPoint check for performance.The mask check could be optimized by avoiding unnecessary property access.
- if ((this.label.mask || this.label.mask_path) && this.label.mask?.data) { + const { mask, mask_path } = this.label; + if ((mask?.data || mask_path) && mask?.data) { return CONTAINS.NONE; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/core/src/components/Modal/ImaVidLooker.tsx(5 hunks)app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(8 hunks)app/packages/looker/src/lookers/imavid/index.ts(1 hunks)app/packages/looker/src/overlays/detection.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/lookers/abstract.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/use-modal-selective-rendering.tsapp/packages/looker/src/lookers/imavid/index.tsapp/packages/looker/src/overlays/detection.tsapp/packages/core/src/components/Modal/ImaVidLooker.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (3)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (1)
26-58: Well-implemented subscription pattern with proper cleanup!The hook follows React best practices:
- Proper subscription cleanup in useEffect
- Correct dependency array
- Efficient frame update handling
app/packages/core/src/components/Modal/ImaVidLooker.tsx (2)
61-66: Improved type safety with direct ImaVidLooker typingThe removal of type casting through unknown and use of direct ImaVidLooker type improves type safety and code clarity.
Also applies to: 77-77
320-320: Good integration of selective rendering hook!The hook is properly integrated at the component level, enhancing the modal's rendering performance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/looker/src/overlays/segmentation.ts (1)
268-273: Add JSDoc documentation for the cleanup method.While the implementation is correct, please add JSDoc documentation explaining the purpose of the
setTargetsToNullparameter and when it should be used.Add this documentation above the method:
+/** + * Cleans up resources associated with this overlay. + * @param setTargetsToNull - When true, clears the targets array in addition to closing the bitmap. + * Set to false to retain targets for potential reuse. + */ public cleanup(setTargetsToNull = false): void {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(1 hunks)app/packages/looker/src/lookers/imavid/index.ts(2 hunks)app/packages/looker/src/overlays/detection.ts(3 hunks)app/packages/looker/src/overlays/heatmap.ts(4 hunks)app/packages/looker/src/overlays/segmentation.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/packages/looker/src/overlays/heatmap.ts
- app/packages/looker/src/overlays/detection.ts
- app/packages/core/src/components/Modal/use-modal-selective-rendering.ts
- app/packages/looker/src/lookers/imavid/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{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/overlays/segmentation.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (3)
app/packages/looker/src/overlays/segmentation.ts (3)
49-49: LGTM! Improved null safety check in constructor.The enhanced null check using optional chaining prevents potential runtime errors by ensuring both mask and its data exist.
65-67: LGTM! Added defensive null check in containsPoint.Early return with
CONTAINS.NONEwhen mask data is missing prevents potential runtime errors.
84-84: LGTM! Enhanced bitmap validation in draw method.The improved check ensures bitmap is valid by verifying its width property, preventing attempts to draw invalid bitmaps.
| ? instance.pluckedOverlays | ||
| : instance.sampleOverlays; |
There was a problem hiding this comment.
I don't think exposing these is necessary. refreshSample could capture the overlay filtering logic happening below
| export type AsyncLabelsRenderingJob = { | ||
| sample: Sample; | ||
| labels: string[]; | ||
| lookerRef: Lookers; |
There was a problem hiding this comment.
I think this ref could be replaced with a function that returns the required options. This would remove the need to expose the internal state
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/core/src/components/Modal/ModalLooker.tsx(2 hunks)app/packages/core/src/components/Modal/VideoLooker.tsx(2 hunks)app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/VideoLooker.tsxapp/packages/core/src/components/Modal/ModalLooker.tsxapp/packages/core/src/components/Modal/use-modal-selective-rendering.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (4)
app/packages/core/src/components/Modal/ModalLooker.tsx (1)
9-9: LGTM! The selective rendering implementation looks good.The changes correctly implement the selective rendering optimization by:
- Adding the necessary hook import
- Extracting the looker from useLooker
- Using the hook with proper dependencies
Also applies to: 31-33
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (2)
16-26: Add missing dependency to useEffectThe
getNewFieldsfunction is used inside useEffect but not included in the dependencies array.- }, [id, lookerOptions.activePaths, looker]); + }, [id, lookerOptions.activePaths, looker, getNewFields]);
81-83: Consider implementing granular refreshing for video lookerThe TODO comment indicates missing functionality for granular refreshing in video lookers. This could impact performance.
Would you like me to help create an issue to track the implementation of granular refreshing for video lookers?
app/packages/core/src/components/Modal/VideoLooker.tsx (1)
12-12: LGTM! The video modal selective rendering is properly integrated.The hook is imported and used correctly with the required dependencies.
Also applies to: 26-26
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (1)
16-26: 🛠️ Refactor suggestionAdd missing dependency and remove unnecessary optional chaining.
The hook has two issues:
- Missing dependency in useEffect (getNewFields)
- Unnecessary optional chaining on looker.refreshSample since looker is already null-checked
- looker?.refreshSample(newFieldsIfAny); + looker.refreshSample(newFieldsIfAny); } - }, [id, lookerOptions.activePaths, looker]); + }, [id, lookerOptions.activePaths, looker, getNewFields]);
🧹 Nitpick comments (3)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (1)
6-9: Consider making the looker parameter non-nullable.Since the hook requires a valid looker to function, consider making it non-nullable to improve type safety.
- looker: Lookers + looker: NonNullable<Lookers>app/packages/core/src/components/Sidebar/useDetectNewActiveLabelFields.ts (1)
63-71: Consider using a single cleanup effect.The component has two separate effects for cleanup: one for dataset changes (lines 31-37) and one for unmounting. These could be combined into a single effect to reduce code duplication.
useEffect(() => { + // reset when dataset changes or component unmounts if (modal) { modalActivePathsLUT.clear(); } else { gridActivePathsLUT.clear(); } + return () => { + if (modal) { + modalActivePathsLUT.clear(); + } else { + gridActivePathsLUT.clear(); + } + }; - }, [datasetNameValue]); - - useEffect(() => { - return () => { - if (modal) { - modalActivePathsLUT.clear(); - } else { - gridActivePathsLUT.clear(); - } - }; - }, [modal]); + }, [datasetNameValue, modal]);app/packages/looker/src/lookers/imavid/index.ts (1)
32-36: Add a comment explaining the thumbnail check.The early return based on thumbnail configuration could use a more descriptive comment explaining why this check is necessary.
- // we have other mechanism for the modal + // Skip frame number subscription for modal views as they handle updates differently if (!this.state.config.thumbnail) { return; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(1 hunks)app/packages/core/src/components/Sidebar/useDetectNewActiveLabelFields.ts(1 hunks)app/packages/looker/src/lookers/imavid/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/components/Grid/useSelect.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Sidebar/useDetectNewActiveLabelFields.tsapp/packages/looker/src/lookers/imavid/index.tsapp/packages/core/src/components/Modal/use-modal-selective-rendering.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (6)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (2)
1-5: LGTM! Clean import organization.Imports are well-organized, following the convention of external dependencies first, followed by internal modules.
41-65:⚠️ Potential issueAdd safety checks and missing dependencies.
The useEffect has several issues:
- Missing null check on looker before subscription
- Potential race condition in subscription callback
- Missing dependency lookerOptions.activePaths
useEffect(() => { + if (!looker) { + return; + } + const unsub = looker.subscribeToState( "currentFrameNumber", (currentFrameNumber: number) => { - if (!looker.thisFrameSample?.sample) { + if (!looker?.thisFrameSample?.sample) { return; } const thisFrameId = `${id}-${currentFrameNumber}`; const newFieldsIfAny = getNewFields(thisFrameId); if (newFieldsIfAny) { looker.refreshSample(newFieldsIfAny, currentFrameNumber); } else { looker.refreshOverlaysToCurrentFrame(); } } ); return () => { unsub(); }; - }, [getNewFields, looker]); + }, [getNewFields, looker, lookerOptions.activePaths]);Likely invalid or redundant comment.
app/packages/core/src/components/Sidebar/useDetectNewActiveLabelFields.ts (2)
6-10: LGTM! Well-defined types and clear exports.The type definitions are clear and the lookup tables are properly typed with generics.
39-48: Addmodalto useCallback dependencies.The
getNewFieldscallback uses themodalparameter but doesn't include it in the dependencies array. This could lead to stale closures.const getNewFields = useCallback( (id: string) => { return syncAndGetNewLabels( id, modal ? modalActivePathsLUT : gridActivePathsLUT, new Set(activeLabelFieldsValue) ); }, - [activeLabelFieldsValue] + [activeLabelFieldsValue, modal] );app/packages/looker/src/lookers/imavid/index.ts (2)
227-231: Convert TODO comment into proper error handling.The TODO comment indicates a potential crash scenario. This should be properly handled with error boundaries or defensive programming.
- // todo: sometimes instance in spotlight?.updateItems() is defined but has no ref to sample - // this crashes the app. this is a bug and should be fixed + if (!this.sample) { + console.warn('Invalid sample reference detected in refreshSample'); + return; + }
252-276:⚠️ Potential issueImplement proper error handling and race condition prevention.
The async label painting job could lead to race conditions if multiple refreshes are triggered in quick succession. Additionally, error handling could be more informative.
+ let isLatestRequest = true; + const requestId = Date.now(); + this.latestRequestId = requestId; + this.asyncLabelsRenderingManager .enqueueLabelPaintingJob({ sample: sample as Sample, labels: renderLabels, }) .then(({ sample, coloring }) => { + if (!isLatestRequest || this.latestRequestId !== requestId) { + console.debug('Skipping stale label painting result'); + return; + } if (sampleIdFromFramesStore) { this.frameStoreController.store.updateSample( sampleIdFromFramesStore, sample ); } else { this.sample = sample; } this.state.options.coloring = coloring; this.loadOverlays(sample); // to run looker reconciliation this.updater({ overlaysPrepared: true, }); }) .catch((error) => { - this.updater({ error }); + console.error('Label painting failed:', error); + this.updater({ + error: new Error(`Failed to paint labels: ${error.message}`), + overlaysPrepared: false + }); });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
app/packages/looker/src/lookers/imavid/index.ts (1)
246-297:⚠️ Potential issueImprove error handling and prevent race conditions.
The function has several issues:
- The TODO comment indicates a potential crash that needs to be fixed.
- No race condition prevention for concurrent label painting jobs.
- Minimal error handling in the promise chain.
The TODO comment about potential crashes has been previously addressed. Additionally, apply this diff to improve error handling and prevent race conditions:
refreshSample(renderLabels: string[] | null = null, frameNumber?: number) { if (!this.sample) { + console.warn('Attempted to refresh undefined sample'); return; } if (!renderLabels?.length) { - this.updateSample(this.sample); + try { + this.updateSample(this.sample); + } catch (error) { + console.error('Failed to update sample:', error); + this.updater({ error }); + } return; } + let isLatestRequest = true; + const requestId = Date.now(); + this.latestRequestId = requestId; + const sampleIdFromFramesStore = this.frameStoreController.store.frameIndex.get(frameNumber); let sample: Sample; // if sampleIdFromFramesStore is not found, it means we're in grid thumbnail view if (sampleIdFromFramesStore) { const { image: _cachedImage, ...sampleWithoutImage } = this.frameStoreController.store.samples.get(sampleIdFromFramesStore); sample = sampleWithoutImage.sample; } else { sample = this.sample; } this.asyncLabelsRenderingManager .enqueueLabelPaintingJob({ sample: sample as Sample, labels: renderLabels, }) .then(({ sample, coloring }) => { + if (!isLatestRequest || this.latestRequestId !== requestId) { + console.debug('Skipping stale label painting result'); + return; + } if (sampleIdFromFramesStore) { this.frameStoreController.store.updateSample( sampleIdFromFramesStore, sample ); } else { this.sample = sample; } this.state.options.coloring = coloring; - this.loadOverlays(sample); + try { + this.loadOverlays(sample); + } catch (error) { + console.error('Failed to load overlays:', error); + this.updater({ error }); + return; + } // to run looker reconciliation this.updater({ overlaysPrepared: true, }); }) .catch((error) => { - this.updater({ error }); + console.error('Label painting failed:', error); + this.updater({ + error: new Error(`Failed to paint labels: ${error.message}`), + overlaysPrepared: false + }); }); }app/packages/looker/src/lookers/abstract.ts (2)
534-568:⚠️ Potential issueImprove error handling and timeout management.
The function has several areas for improvement:
- The TODO comment indicates a potential crash that needs to be fixed.
- The timeout duration is hardcoded.
- The error handling in the microtask could be more informative.
The TODO comment about potential crashes has been previously addressed. Additionally, apply this diff to improve error handling and timeout management:
+const SAMPLE_UPDATE_TIMEOUT_MS = 10000; + updateSample(sample: Sample) { if (!this.sample) { + console.warn('Attempted to update undefined sample'); return; } const id = sample.id ?? sample._id; - const updateTimeoutMs = 10000; if (UPDATING_SAMPLES_IDS.has(id)) { UPDATING_SAMPLES_IDS.delete(id); this.cleanOverlays(true); // to prevent deadlock, we'll remove the id from the set after a timeout const timeoutId = setTimeout(() => { + console.warn(`Sample update timed out after ${SAMPLE_UPDATE_TIMEOUT_MS}ms`); UPDATING_SAMPLES_IDS.delete(id); - }, updateTimeoutMs); + }, SAMPLE_UPDATE_TIMEOUT_MS); queueMicrotask(() => { try { this.updateSample(sample); clearTimeout(timeoutId); } catch (e) { UPDATING_SAMPLES_IDS.delete(id); - this.updater({ error: e }); + const error = e instanceof Error ? e : new Error('Failed to update sample'); + console.error('Sample update failed:', error); + this.updater({ + error, + overlaysPrepared: false + }); } }); return; } UPDATING_SAMPLES_IDS.add(id); - this.loadSample(sample, retrieveArrayBuffers(this.sampleOverlays)); + try { + this.loadSample(sample, retrieveArrayBuffers(this.sampleOverlays)); + } catch (error) { + UPDATING_SAMPLES_IDS.delete(id); + console.error('Failed to load sample:', error); + this.updater({ + error: new Error(`Failed to load sample: ${error.message}`), + overlaysPrepared: false + }); + } }
570-600:⚠️ Potential issueImprove error handling and prevent race conditions.
The function has several issues:
- The TODO comment indicates a potential crash that needs to be fixed.
- No race condition prevention for concurrent label painting jobs.
- Minimal error handling in the promise chain.
The TODO comment about potential crashes has been previously addressed. Additionally, apply this diff to improve error handling and prevent race conditions:
refreshSample(renderLabels: string[] | null = null) { if (!this.sample) { + console.warn('Attempted to refresh undefined sample'); return; } if (!renderLabels?.length) { - this.updateSample(this.sample); + try { + this.updateSample(this.sample); + } catch (error) { + console.error('Failed to update sample:', error); + this.updater({ error }); + } return; } + let isLatestRequest = true; + const requestId = Date.now(); + this.latestRequestId = requestId; + this.asyncLabelsRenderingManager .enqueueLabelPaintingJob({ sample: this.sample, labels: renderLabels, }) .then(({ sample, coloring }) => { + if (!isLatestRequest || this.latestRequestId !== requestId) { + console.debug('Skipping stale label painting result'); + return; + } this.sample = sample; this.state.options.coloring = coloring; - this.loadOverlays(sample); + try { + this.loadOverlays(sample); + } catch (error) { + console.error('Failed to load overlays:', error); + this.updater({ error }); + return; + } // to run looker reconciliation this.updater({ overlaysPrepared: true, }); }) .catch((error) => { - this.updater({ error }); + console.error('Label painting failed:', error); + this.updater({ + error: new Error(`Failed to paint labels: ${error.message}`), + overlaysPrepared: false + }); }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/looker/src/lookers/abstract.ts(8 hunks)app/packages/looker/src/lookers/imavid/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{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/lookers/imavid/index.tsapp/packages/looker/src/lookers/abstract.ts
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
🪛 Biome (1.9.4)
app/packages/looker/src/lookers/imavid/index.ts
[error] 237-237: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (2)
app/packages/looker/src/lookers/abstract.ts (2)
783-787: LGTM!The function correctly handles the new
setTargetsToNullparameter and safely calls cleanup on each overlay.
809-809: LGTM!The function correctly cleans up the
UPDATING_SAMPLES_IDSset after loading the sample.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/packages/core/src/components/Grid/useSelect.ts (1)
48-89: Consider adding debounce to prevent potential race conditions.While the code correctly handles the rendering status updates, rapid changes to the active fields could lead to race conditions. Consider adding debounce to the refresh operations.
Apply this diff to add debounce:
+import { debounce } from "lodash"; export default function useSelect( getFontSize: () => number, options: ReturnType<typeof fos.useLookerOptions>, store: LRUCache<string, fos.Lookers>, spotlight?: Spotlight<number, fos.Sample> ) { const { init, deferred } = fos.useDeferrer(); + const debouncedRefresh = debounce( + (instance: fos.Lookers, fields: string[]) => { + instance.refreshSample(fields); + }, + 300 + ); // ... rest of the code ... - instance.refreshSample(newFieldsIfAny); + debouncedRefresh(instance, newFieldsIfAny); // ... rest of the code ... - instance.refreshSample(rerenderFields); + debouncedRefresh(instance, rerenderFields);app/packages/looker/src/worker/index.ts (3)
57-81: Consider caching the path check result.The
shouldProcessLabelfunction is well-structured, but the path check could be optimized for repeated calls.const shouldProcessLabel = ({ field, label, activePaths, prefix, }: { prefix: string; field: string; label: any; activePaths: string[]; }) => { + const path = `${prefix ?? ""}${field}`; const currentLabelRenderStatus = label?.renderStatus; if ( currentLabelRenderStatus === RENDER_STATUS_PAINTED || currentLabelRenderStatus === RENDER_STATUS_DECODED || currentLabelRenderStatus === RENDER_STATUS_PENDING ) { return true; } - return activePaths?.includes(`${prefix ?? ""}${field}`); + return activePaths?.includes(path); };
351-352: Document the rationale for processing all 3D labels.The comment indicates that 3D labels are processed regardless of active paths. Consider documenting why this exception exists to help future maintainers understand the design decision.
- // we process all 3d labels regardless of active paths + // Process all 3D labels regardless of active paths because they require special handling + // for proper rendering in the 3D viewer and don't benefit from selective rendering
254-256: Consider using the RENDER_STATUS constant.For consistency with the rest of the codebase, use the
RENDER_STATUS_PAINTEDconstant instead of the string literal.- if (label?.renderStatus !== "painted") { + if (label?.renderStatus !== RENDER_STATUS_PAINTED) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/core/src/components/Grid/useSelect.ts(3 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/index.ts(18 hunks)app/packages/looker/src/worker/painter.ts(8 hunks)app/packages/looker/src/worker/shared.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/packages/looker/src/worker/disk-overlay-decoder.ts
- app/packages/looker/src/worker/painter.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{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/shared.tsapp/packages/core/src/components/Grid/useSelect.tsapp/packages/looker/src/worker/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (7)
app/packages/looker/src/worker/shared.ts (2)
3-6: Consider using shorter constant names for better readability.The constants could be more concise by removing the
RENDER_STATUS_prefix, as suggested in a previous review.Apply this diff to make the constants more concise:
-export const RENDER_STATUS_PENDING = "pending"; -export const RENDER_STATUS_PAINTING = "painting"; -export const RENDER_STATUS_PAINTED = "painted"; -export const RENDER_STATUS_DECODED = "decoded"; +export const PENDING = "pending"; +export const PAINTING = "painting"; +export const PAINTED = "painted"; +export const DECODED = "decoded";
8-13: LGTM!The type definition is well-structured and follows TypeScript best practices by using
typeofto reference the constants.app/packages/core/src/components/Grid/useSelect.ts (3)
1-1: LGTM!The imports are well-organized and properly scoped.
Also applies to: 7-8
18-20: LGTM!The hook is properly initialized with the correct configuration.
35-40: LGTM!The code properly handles different looker types and extracts overlays accordingly.
app/packages/looker/src/worker/index.ts (2)
33-51: LGTM! Good refactoring of color resolution logic.The extraction of color resolution logic into a separate module improves code organization and maintainability.
408-427: Consider handling Promise rejection.The Promise.all() call could fail if any of the bitmap promises reject. Consider adding error handling to ensure graceful degradation.
- Promise.all(imageBitmapPromises).then((bitmaps) => { + Promise.all(imageBitmapPromises).then((bitmaps) => { const flatBitmaps = bitmaps.flat() ?? []; const flatMaskTargetsBuffers = maskTargetsBuffers.flat() ?? []; const transferables = [...flatBitmaps, ...flatMaskTargetsBuffers]; postMessage( { method: "processSample", sample, coloring, uuid, customizeColorSetting, colorscale, labelTagColors, selectedLabelTags, }, // @ts-ignore transferables ); + }).catch((error) => { + postMessage({ + error: { + cls: error.constructor.name, + message: error.message, + }, + }); });
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/packages/looker/src/overlays/index.ts (1)
56-56: Fix typo inPOINTS_FROM_FOkey name.The key
Poylinesshould bePolylinesto match the corresponding key inFROM_FO.- Poylines: (label) => getPolylinePoints(label?.polylines ?? []), + Polylines: (label) => getPolylinePoints(label?.polylines ?? []),app/packages/looker/src/lookers/utils.ts (1)
13-60: Consider adding error handling for DataCloneError.The code mentions potential DataCloneError when switching colors too fast, but doesn't explicitly handle it.
export const retrieveTransferables = <State extends BaseState>( overlays?: Overlay<State>[] -) => { +): Transferable[] => { const transferables: Transferable[] = []; + try { for (const overlay of overlays ?? []) { // ... existing code ... } return transferables; + } catch (error) { + if (error instanceof DOMException && error.name === 'DataCloneError') { + console.warn('Buffer transfer failed, likely due to rapid color changes'); + return []; + } + throw error; + } };
🧹 Nitpick comments (4)
app/packages/looker/src/overlays/index.ts (2)
87-136: Consider enhancing type safety and documentation.While the implementation is solid, consider these improvements for better maintainability:
- Add JSDoc comments describing the function's purpose and parameters
- Consider making
depthparameter more type-safe by using a literal union type- Add return type documentation explaining the structure of classifications and overlays
+/** + * Accumulates overlays and classifications from a data object based on schema. + * @param data - The data object containing label information + * @param schema - The schema defining the structure of the data + * @param prefix - Optional path prefix for nested fields + * @param depth - Maximum recursion depth for nested documents (default: 1) + * @returns Object containing accumulated classifications and overlays + */ -export const accumulateOverlays = <State extends BaseState>( +export const accumulateOverlays = <State extends BaseState, Depth extends 0 | 1>( data: { [key: string]: any; }, schema: Schema, prefix = [], - depth = 1 + depth: Depth = 1 as Depth ): { classifications: Labels<TemporalDetectionLabel | ClassificationLabel>; overlays: Overlay<State>[]; } => {
81-85: Consider using TypeScript'sas constfor immutable sets.Using
as constwould provide better type safety for these constant sets.-const TAGS = new Set(LABEL_TAGS_CLASSES); +const TAGS = new Set(LABEL_TAGS_CLASSES) as const; -const EMBEDDED_FIELDS = Object.freeze( - new Set([EMBEDDED_DOCUMENT_FIELD, DYNAMIC_EMBEDDED_DOCUMENT_FIELD]) -); +const EMBEDDED_FIELDS = new Set([ + EMBEDDED_DOCUMENT_FIELD, + DYNAMIC_EMBEDDED_DOCUMENT_FIELD +]) as const;app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (1)
37-56: Consider memoizing the coloring hash map.The coloring hash generation could be optimized by memoizing the WeakMap lookups to prevent unnecessary recalculations.
+ const memoizedColoringHash = useMemo(() => { + if (!coloringIds.current.has(lookerOptions.coloring)) { + coloringIds.current.set( + lookerOptions.coloring, + `id_${coloringIdCounter.current++}` + ); + } + return coloringIds.current.get(lookerOptions.coloring); + }, [lookerOptions.coloring]); + - const getColoringHash = useCallback(() => { - if (!coloringIds.current.has(lookerOptions.coloring)) { - coloringIds.current.set( - lookerOptions.coloring, - `id_${coloringIdCounter.current++}` - ); - } - - return coloringIds.current.get(lookerOptions.coloring); - }, [lookerOptions.coloring]);app/packages/looker/src/lookers/utils.ts (1)
54-56: Consider adding bitmap null check.While the code checks for bitmap dimensions, an explicit null check would provide better safety.
- if (bitmap?.width || bitmap?.height) { + if (bitmap && (bitmap.width || bitmap.height)) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(1 hunks)app/packages/looker/src/elements/common/error.ts(1 hunks)app/packages/looker/src/lookers/abstract.ts(8 hunks)app/packages/looker/src/lookers/imavid/index.ts(3 hunks)app/packages/looker/src/lookers/utils.test.ts(3 hunks)app/packages/looker/src/lookers/utils.ts(2 hunks)app/packages/looker/src/overlays/index.ts(1 hunks)app/packages/looker/src/worker/async-labels-rendering-manager.ts(1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- app/packages/looker/src/elements/common/error.ts
- app/packages/looker/src/worker/async-labels-rendering-manager.ts
- app/packages/looker/src/lookers/imavid/index.ts
- app/packages/looker/src/lookers/abstract.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{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/overlays/index.tsapp/packages/looker/src/elements/common/error.tsapp/packages/looker/src/lookers/utils.test.tsapp/packages/looker/src/lookers/utils.tsapp/packages/looker/src/lookers/imavid/index.tsapp/packages/core/src/components/Modal/use-modal-selective-rendering.tsapp/packages/looker/src/lookers/abstract.tsapp/packages/looker/src/worker/async-labels-rendering-manager.ts
📓 Learnings (1)
app/packages/looker/src/lookers/abstract.ts (1)
Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.
🪛 Biome (1.9.4)
app/packages/looker/src/lookers/imavid/index.ts
[error] 240-240: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (7)
app/packages/looker/src/overlays/index.ts (1)
87-87: LGTM! MakingaccumulateOverlayspublic aligns with PR objectives.The change to export this function supports the new selective label rendering feature by allowing external modules to accumulate overlays based on visibility settings.
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (3)
1-5: LGTM! Clean imports with proper type declarations.All necessary imports are present and properly organized.
24-24: Remove unnecessary optional chaining.The
lookeris already null-checked at line 17, making the optional chaining onrefreshSampleredundant.- looker?.refreshSample(newFieldsIfAny); + looker.refreshSample(newFieldsIfAny);
103-106: Remove unnecessary optional chaining and track TODO.The hook has two issues:
- Unnecessary optional chaining on looker.refreshSample since looker is already null-checked
- Missing functionality for granular refreshing
Would you like me to create an issue to track the implementation of granular refreshing for the video looker?
app/packages/looker/src/lookers/utils.test.ts (1)
10-10: LGTM! Function rename consistently applied.The test cases have been correctly updated to use the new function name
retrieveTransferableswhile maintaining the same test coverage and assertions.Also applies to: 29-29, 33-33, 37-37, 43-43, 47-47, 59-59, 65-65
app/packages/looker/src/lookers/utils.ts (2)
13-18: LGTM! Type changes align with transferable objects.The function signature and return type have been correctly updated to handle
Transferable[]instead ofArrayBuffer[], which better represents the actual data being transferred.
35-52: Verify browser compatibility for buffer.detached API.The code includes a new browser API check for
buffer.detached. While there's a fallback mechanism, we should verify browser support to ensure wide compatibility.✅ Verification successful
🌐 Web query:
What browsers support ArrayBuffer.prototype.detached?💡 Result:
The `ArrayBuffer.prototype.detached` property is supported in modern browsers as of early 2025: **Current browser support:** - **Firefox**: Supported since v118[7] - **Chrome/Chromium**: Supported since v123[7] - **Safari**: Supported since v17.4[7] - **Edge**: Supported since v123[7] - **Opera**: Supported since v109[7] **Unsupported browsers:** - Internet Explorer (all versions)[7] - Legacy mobile browsers not updated since 2023 This property was standardized in ECMAScript 2024[2][4] and allows checking if an ArrayBuffer has been transferred/detached via methods like `structuredClone()` or `transfer()`[2][4]. Detached buffers become inaccessible and throw errors when accessed[1][2]. Implementation details vary slightly between browsers: - Firefox changed behavior to throw errors on detached buffer access[1] - Chrome/Safari implementations align with updated WebIDL standards[1] - All modern engines now return `true` for `.detached` after transfers[2][4] Citations: - 1: https://github.com/tc39/ecma262/issues/678 - 2: https://2ality.com/2024/06/array-buffers-es2024.html - 3: https://tc39.es/ecma262/multipage/structured-data.html - 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer?retiredLocale=pl - 5: https://caniuse.com - 6: https://github.com/es-shims/ArrayBuffer.prototype.detached - 7: https://caniuse.com/mdn-javascript_builtins_arraybuffer_detached
Browser Compatibility Verified for
buffer.detachedChecksThe web query confirms that the
ArrayBuffer.prototype.detachedproperty is supported in all major modern browsers—Firefox (v118+), Chrome/Chromium (v123+), Safari (v17.4+), Edge (v123+), and Opera (v109+). Legacy browsers like Internet Explorer remain unsupported, but the fallback in your code (checking forbuffer.byteLength) appropriately covers that case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (4)
6-9: Add return type annotation for better type safety.The hook's return type should be explicitly defined for better TypeScript type safety.
-export const useImageModalSelectiveRendering = ( +export const useImageModalSelectiveRendering = ( id: string, looker: Lookers -) => { +): void => {
16-26: Remove unnecessary optional chaining.The looker is already null-checked at the start of the effect, making the optional chaining redundant.
if (newFieldsIfAny) { - looker?.refreshSample(newFieldsIfAny); + looker.refreshSample(newFieldsIfAny); }
29-32: Add return type annotation for better type safety.The hook's return type should be explicitly defined for better TypeScript type safety.
-export const useImavidModalSelectiveRendering = ( +export const useImavidModalSelectiveRendering = ( id: string, looker: ImaVidLooker -) => { +): void => {
76-79: Add return type annotation for better type safety.The hook's return type should be explicitly defined for better TypeScript type safety.
-export const useVideoModalSelectiveRendering = ( +export const useVideoModalSelectiveRendering = ( id: string, looker: VideoLooker -) => { +): void => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(1 hunks)app/packages/looker/src/lookers/imavid/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{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/lookers/imavid/index.tsapp/packages/core/src/components/Modal/use-modal-selective-rendering.ts
🪛 Biome (1.9.4)
app/packages/looker/src/lookers/imavid/index.ts
[error] 244-244: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (6)
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (3)
1-5: LGTM! Clean and well-organized imports.The imports follow a good organization pattern, separating external dependencies from internal modules.
94-97: Remove unnecessary optional chaining and track TODO.The hook has two issues:
- Unnecessary optional chaining on looker.refreshSample since looker is already null-checked
- Missing functionality for granular refreshing
- looker?.refreshSample(); + looker.refreshSample();Would you like me to create an issue to track the implementation of granular refreshing for the video looker?
49-73:⚠️ Potential issueAdd null check and protect against looker changes.
The effect needs a null check for looker and should clean up properly if looker changes.
useEffect(() => { + if (!looker) { + return; + } + const unsub = looker.subscribeToState( "currentFrameNumber", (currentFrameNumber: number) => { - if (!looker.thisFrameSample?.sample) { + if (!looker?.thisFrameSample?.sample) { return; } const thisFrameId = `${id}-${currentFrameNumber}-${getColoringHash()}`; const newFieldsIfAny = getNewFields(thisFrameId); if (newFieldsIfAny) { looker.refreshSample(newFieldsIfAny, currentFrameNumber); } else { looker.refreshOverlaysToCurrentFrame(); } } ); return () => { unsub(); }; }, [getNewFields, getColoringHash, looker]);Likely invalid or redundant comment.
app/packages/looker/src/lookers/imavid/index.ts (3)
231-234: LGTM!The thumbnail-specific logic is well-placed and correctly handles the modal case through
useImavidModalSelectiveRendering.
240-251:⚠️ Potential issueAdd error handling and improve type safety.
The function has potential runtime issues:
- Optional chaining on line 244 could lead to TypeError if
getSampleAtFramereturns undefined- No error handling for
loadOverlayscallApply this diff to improve error handling and type safety:
refreshOverlaysToCurrentFrame() { + try { + const frameSample = this.frameStoreController.store.getSampleAtFrame( + this.frameNumber + ); + + if (!frameSample) { + console.debug('No sample found for current frame, using current sample'); + this.loadOverlays(this.sample); + return; + } + let { image: _cachedImage, ...thisFrameSample } = - this.frameStoreController.store.getSampleAtFrame( - this.frameNumber - )?.sample; + frameSample.sample; if (!thisFrameSample) { thisFrameSample = this.sample; } this.loadOverlays(thisFrameSample); + } catch (error) { + console.error('Failed to load overlays:', error); + this.updater({ error }); + } }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 244-244: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
34-71: 🛠️ Refactor suggestionImprove error handling and prevent race conditions in frame number subscription.
The frame number subscription callback is complex and lacks proper error handling. Consider:
- Race conditions when multiple frame updates occur rapidly
- Potential errors when accessing
this.thisFrameSample?.sampleApply this diff to improve the implementation:
init() { if (!this.state.config.thumbnail) { return; } + let isProcessing = false; + let latestFrameNumber: number | null = null; + this.unsubscribe = this.subscribeToState( "currentFrameNumber", (currentFrameNumber: number) => { + if (isProcessing) { + latestFrameNumber = currentFrameNumber; + return; + } + isProcessing = true; + try { const thisFrameId = `${ this.uuid }-${currentFrameNumber}-${this.state.options.coloring.targets.join( "-" )}`; if ( gridActivePathsLUT.has(thisFrameId) && this.thisFrameSample?.sample ) { this.refreshOverlaysToCurrentFrame(); } else { const newFieldsIfAny = syncAndGetNewLabels( thisFrameId, gridActivePathsLUT, new Set(this.options.activePaths) ); if (newFieldsIfAny && currentFrameNumber > 0) { this.refreshSample(newFieldsIfAny, currentFrameNumber); } else { this.refreshSample(); } } + } catch (error) { + console.error('Frame update failed:', error); + this.updater({ error }); + } finally { + isProcessing = false; + if (latestFrameNumber !== null && latestFrameNumber !== currentFrameNumber) { + const nextFrameNumber = latestFrameNumber; + latestFrameNumber = null; + this.subscribeToState("currentFrameNumber", nextFrameNumber); + } + } } ); }Likely invalid or redundant comment.
What changes are proposed in this pull request?
This PR adds a new dataset app config for configuring default visibility of labels. If
app_config.default_visibility_labelsis not set, all labels are not rendered by default.Also adds an optimization of not rendering labels if it's unchecked in the sidebar.
Notes to the reviewer
The most relevant files on the app side are:
-
useSelect.ts-
labelsVisibility.ts-
useDetectNewActiveLabelFields.ts-
lookers/abstract.ts-
worker/index.ts-
worker/async-labels-rendering-manager.test.tsKnown Issues:
postMessage). This doesn't crash the app, but might lead to confusion. I think it's safe to fix it in upcoming PRs.How is this patch tested? If it is not, please explain why.
Unit tests where applicable. E2E tests.
For local smoke testing, use following script:
Place a debugger in
painter.tsforSegmentationsand observe it doesn't get called. Scroll through the grid and check/uncheck fields to see different behavior.Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
ImaVidLookerclass to enhance user interaction with video data.