-
Notifications
You must be signed in to change notification settings - Fork 491
perf: Improve audit detail view loading time #2672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughConditional rendering was added so child RecursiveTreeViewItem components render only when their parent node’s id is present in expandedNodes. Previously, nested items rendered unconditionally under childrenSlot. No changes to events or bindings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TreeViewItem as RecursiveTreeViewItem (Parent)
participant State as expandedNodes
participant Child as RecursiveTreeViewItem (Child)
User->>TreeViewItem: Toggle expand/collapse
TreeViewItem->>State: Update expandedNodes with node.id
Note over TreeViewItem,State: expandedNodes.includes(node.id) determines rendering
alt Parent expanded
TreeViewItem->>Child: Render children (mount)
Child-->>TreeViewItem: Child subtree renders
else Parent collapsed
TreeViewItem--X Child: Do not render children (unmounted/hidden)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/lib/components/TreeView/RecursiveTreeViewItem.svelte (1)
52-52
: Consider usingArray.from()
for array initialization.
Array(nodes.length).fill([])
creates multiple references to the same empty array object. While Svelte's binding mechanism replaces each element correctly, usingArray.from()
is more explicit and avoids potential confusion:-let childrenNodes: TreeViewItem[][] = $state(Array(nodes.length).fill([])); +let childrenNodes: TreeViewItem[][] = $state(Array.from({ length: nodes.length }, () => []));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/lib/components/TreeView/RecursiveTreeViewItem.svelte
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
🔇 Additional comments (1)
frontend/src/lib/components/TreeView/RecursiveTreeViewItem.svelte (1)
194-211
: LGTM! Effective performance optimization.Wrapping the recursive child rendering in
{#if expandedNodes.includes(node.id)}
prevents unnecessary component instantiation and DOM creation for collapsed nodes. This is especially beneficial for large tree structures and directly addresses the PR objective of improving loading time.The approach is sound because:
- State (
expandedNodes
,checkedNodes
,indeterminateNodes
) is bound from the parent, so it persists across collapse/expand cycles- The condition matches the
open
prop on line 165, ensuring consistency- Event forwarding (lines 202-209) correctly propagates interactions from nested nodes
To ensure robustness, please verify:
- Deeply nested trees render correctly when multiple levels are expanded
- Node state (checked, indeterminate) is preserved after collapsing and re-expanding
- The performance improvement is measurable in the audit detail view with large datasets
For a CJIS framework based Audit, the Audit detail view loading time passes from around 7 seconds to 1.3 seconds.