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 15 minutes and 42 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 (3)
WalkthroughThis pull request introduces a comprehensive set of changes across multiple packages in the FiftyOne application, focusing on enhancing label visibility, improving sample rendering, and refining state management. The modifications span various components, including schema definitions, worker processes, and UI interactions. Key improvements include adding default visibility label configurations, introducing more flexible overlay management, and implementing advanced caching mechanisms for label processing. Changes
Sequence DiagramsequenceDiagram
participant User
participant Grid
participant Sidebar
participant Looker
participant State
User->>Sidebar: Select Active Labels
Sidebar->>State: Update Active Fields
State->>Grid: Trigger Refresh
Grid->>Looker: Request Sample Reload
Looker->>Looker: Process Sample with Active Fields
Possibly related PRs
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
from
9e59b90 to
d8ecb44
Compare
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.
Note: we store per-looker list of cached labels. Space complexity: O(n), where n = number of labels. Number of lookers cached itself is bounded (currently to 510).
Todo:
Known Issues:
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
Release Notes
New Features
Improvements
Bug Fixes
Performance
Development