[WEB-2316] fix: Style Calculation performance issue in virtualization

[rahulramesha]

This PR mainly tries to fix the Style calculation performance issue of RenderIfVisibleHOC component. This Includes,

• Read style calculation only for Kanban
• If reading style calculation then do it only on idle time
• Add place holder for List layout in RenderIfVisibleHOC
• other minor housekeeping changes

Summary by CodeRabbit

Release Notes

• New Features

  • Introduced a new prop, shouldRecordHeights, for better control over height recording in the RenderIfVisible component.
  • Added a loading placeholder component, ListLoaderItemRow, to enhance user feedback during content loading.

• Improvements

  • Enhanced rendering logic for mobile devices in the IssueBlockRoot component.
  • Improved styling flexibility for placeholder elements in the SpreadsheetIssueRow.

• Bug Fixes

  • Streamlined issue handling by removing unnecessary checks for empty issue maps in the IssueStore.

[coderabbitai]

Walkthrough

The pull request introduces several enhancements across multiple components, primarily focusing on the RenderIfVisible component. A new prop, shouldRecordHeights, is added to control height recording behavior. Modifications are made to improve rendering logic, particularly for mobile platforms, and to enhance user experience with loading states. Additionally, the IssueStore class is streamlined by removing checks for empty maps in various methods, allowing for more direct operations on issues.

Changes

File Path	Change Summary
web/core/components/core/render-if-visible-HOC.tsx	Added shouldRecordHeights prop to control height recording; adjusted rendering logic for placeholderChildren.
web/core/components/issues/issue-layouts/list/block-root.tsx	Integrated usePlatformOS hook for mobile detection; conditionally set shouldRecordHeights based on mobile status; added ListLoaderItemRow for loading indication. Removed defaultHeight.
web/core/components/issues/issue-layouts/spreadsheet/issue-row.tsx	Adjusted placeholder styling; removed defaultHeight; set shouldRecordHeights to false.
web/core/components/ui/loader/layouts/list-layout-loader.tsx	Introduced new props (shouldAnimate, renderForPlaceHolder, defaultPropertyCount) for customization of loading items. Updated rendering logic for placeholders.
web/core/store/issue/issue.store.ts	Removed checks for empty issuesMap in several methods, simplifying logic for issue operations.

Possibly related PRs

• [WEB-2316] chore: Kanban group virtualization #5565: Modifies the RenderIfVisible component, directly related to the shouldRecordHeights prop and rendering logic.
• [WEB 2418] Fix minor UI inconsistencies #5568: Changes to the KanBan component that also utilize the RenderIfVisible component, indicating shared functionality.
• [WEB-2480] fix: project settings layout #5638: Addresses layout issues in the ProjectSettingLayout, relating to rendering and visibility logic.

Suggested labels

🧹chore

Suggested reviewers

• SatishGandham
• rahulramesha
• sriramveeraghanta

Poem

🐰 In the meadow where we play,
Changes hop in bright array.
Heights recorded, loading shown,
Mobile friendly, skills have grown.
With each tweak, our joy will bloom,
Hopping high, we chase the moon! 🌙

---

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3a9f01b and 9deb77e.

Files selected for processing (5)

• web/core/components/core/render-if-visible-HOC.tsx (3 hunks)
• web/core/components/issues/issue-layouts/list/block-root.tsx (3 hunks)
• web/core/components/issues/issue-layouts/spreadsheet/issue-row.tsx (1 hunks)
• web/core/components/ui/loader/layouts/list-layout-loader.tsx (1 hunks)
• web/core/store/issue/issue.store.ts (4 hunks)

Additional comments not posted (19)

web/core/components/core/render-if-visible-HOC.tsx (5)

14-14: LGTM!

The new prop shouldRecordHeights is a clear and useful addition to control whether the component should record heights. The default value of true maintains the existing behavior.

---

27-28: LGTM!

The destructuring of the new prop shouldRecordHeights with a default value is consistent with the other props. The comment for placeholderChildren is a helpful addition for clarity.

---

70-73: LGTM!

The updated useEffect hook correctly checks the new prop shouldRecordHeights before recording heights. Wrapping the height recording logic in requestIdleCallback is a good practice to perform the work during idle time and avoid blocking the main thread.

---

75-75: LGTM!

Adding shouldRecordHeights as a dependency to the useEffect hook is necessary to ensure that the effect is re-run when this prop changes. This allows the effect to respond correctly to changes in the prop value.

---

78-78: LGTM!

The conditional style object allows for flexible rendering based on the component's visibility and the presence of placeholderChildren. When the component is not visible and no placeholder is provided, the style sets the height to the recorded placeholderHeight and width to "100%". This is a good approach to handle the different rendering scenarios.

web/core/components/ui/loader/layouts/list-layout-loader.tsx (5)

6-9: LGTM!

The addition of the new optional props shouldAnimate, renderForPlaceHolder, and defaultPropertyCount enhances the ListLoaderItemRow component's customizability and flexibility. These props allow for better control over the component's visual behavior and placeholder rendering, aligning with the PR objective of improving the component's functionality.

---

12-15: LGTM!

Updating the class names to utilize the cn utility for conditional class application improves the readability and maintainability of the code. The conditional class application allows for dynamic styling based on the provided shouldAnimate and renderForPlaceHolder props, enhancing the component's visual behavior and aligning with the PR objective.

Also applies to: 19-22, 25-28, 37-40, 44-47

---

32-32: LGTM!

Modifying the logic for rendering placeholder items to use the defaultPropertyCount prop instead of a fixed array length enhances the component's configurability. This change allows for more flexible usage of the component by enabling dynamic control over the number of placeholder items rendered, aligning with the PR objective of improving the component's functionality.

---

Line range hint 58-75: No impact on usage.

The changes made to the ListLoaderItemRow component do not affect its usage within the ListSection component. The ListSection component continues to render a specific number of ListLoaderItemRow instances based on the itemCount prop, and the modifications to the ListLoaderItemRow component's props and behavior do not impact this usage.

---

Line range hint 77-83: No changes required.

The modifications made to the ListLoaderItemRow component do not directly affect the ListLayoutLoader component. The ListLayoutLoader component's structure and behavior remain unchanged, and it continues to render multiple ListSection components with different itemCount values. No changes are required in the ListLayoutLoader component as a result of the updates to the ListLoaderItemRow component.

web/core/store/issue/issue.store.ts (4)

80-80: Verify the impact of removing the isEmpty(this.issuesMap) check.

The removal of the isEmpty(this.issuesMap) check allows the updateIssue method to proceed even when this.issuesMap is empty, as long as a valid issueId is provided. This change may lead to different behavior in scenarios where this.issuesMap is empty but a valid issueId is provided.

Please ensure that this is the intended behavior and consider the potential impact on the application's functionality.

---

95-95: Verify the impact of removing the isEmpty(this.issuesMap) check.

Similar to the updateIssue method, the removal of the isEmpty(this.issuesMap) check in the removeIssue method allows it to proceed even when this.issuesMap is empty, as long as a valid issueId is provided. This change may lead to different behavior in scenarios where this.issuesMap is empty but a valid issueId is provided.

Please ensure that this is the intended behavior and consider the potential impact on the application's functionality.

---

108-108: Verify the impact of removing the isEmpty(this.issuesMap) check.

The removal of the isEmpty(this.issuesMap) check in the getIssueById method allows it to proceed even when this.issuesMap is empty, as long as a valid issueId is provided. This change may lead to different behavior in scenarios where this.issuesMap is empty but a valid issueId is provided.

Please ensure that this is the intended behavior and consider the potential impact on the application's functionality.

---

119-119: LGTM!

The removal of the isEmpty(issueIds) check in the getIssuesByIds method is redundant since the length check serves the same purpose. This change can be considered a code cleanup or optimization and does not have a significant impact on the method's behavior.

web/core/components/issues/issue-layouts/list/block-root.tsx (3)

16-16: LGTM!

The import statement for the ListLoaderItemRow component is correctly added.

---

20-20: LGTM!

The import statement for the usePlatformOS hook is correctly added.

---

Line range hint 71-139: Excellent work on enhancing the user experience and optimizing for mobile devices!

The changes in this code segment are well-implemented and align with the PR objectives:

• The usePlatformOS hook is correctly used to determine if the platform is mobile.
• The ListLoaderItemRow component is appropriately used as a placeholder for the loading state, improving the user experience.
• The shouldRecordHeights prop in the RenderIfVisible component is conditionally set based on the mobile status, optimizing the rendering behavior for mobile devices.

Great job on these enhancements!

web/core/components/issues/issue-layouts/spreadsheet/issue-row.tsx (2)

81-85: LGTM!

The changes to the placeholder element styling in the RenderIfVisible component look good. Removing the defaultHeight prop and adding an inline style attribute with a calculated height value provides more flexibility and ensures a consistent height for the placeholder.

---

92-92: Verify the impact of setting shouldRecordHeights to false.

Please ensure that setting the shouldRecordHeights prop to false does not introduce any unintended layout or performance issues. Review the behavior of the RenderIfVisible component and its children to confirm that the change aligns with the expected functionality.

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  -- I pushed a fix in commit <commit_id>, please review it.
  -- Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  -- @coderabbitai generate unit testing code for this file.
  -- @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  -- @coderabbitai generate interesting stats about this repository and render them as a table.
  -- @coderabbitai read src/utils.ts and generate unit testing code.
  -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  -- @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.