feat: filter by status/result in compliance assessment page #1517
Conversation
WalkthroughThe changes introduce enhanced visibility and filtering logic for tree view components. A recursive function ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as +page.svelte
participant TreeView as TreeViewItemContent.svelte
User->>Page: Click filter button (status/result)
Page->>Page: Run toggleStatus()/toggleResult() to update filters
Page->>Page: Update selectedStatus and selectedResults arrays
Page->>TreeView: Re-render tree view with updated filter state
TreeView->>TreeView: Evaluate node visibility using recursive function and filter criteria
TreeView->>User: Display filtered tree view
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
CodeRabbit Configuration File (
|
Mohamed-Hacene
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/lib/components/TreeView/RecursiveTreeViewItem.svelte (1)
134-139: Consider memoizing the recursive function for performance.The recursive function
areAllChildrenHiddenRecursivecould impact performance for large trees as it's called during rendering. Consider memoizing the results to avoid unnecessary recalculations.+import { memoize } from 'lodash'; -function areAllChildrenHiddenRecursive(node: TreeViewNode): boolean { +const areAllChildrenHiddenRecursive = memoize((node: TreeViewNode): boolean => { if (!node.children || node.children.length === 0) return false; return node.children.every( (child) => child.contentProps.hidden || areAllChildrenHiddenRecursive(child) ); -} +});frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (3)
109-116: Consider using a Set for better performance.For frequent inclusion checks in
selectedStatusandselectedResults, usingSetwould provide better performance than arrays.-let selectedStatus = ['done', 'to_do', 'in_progress', 'in_review']; -let selectedResults = [ - 'compliant', - 'non_compliant', - 'partially_compliant', - 'not_assessed', - 'not_applicable' -]; +let selectedStatus = new Set(['done', 'to_do', 'in_progress', 'in_review']); +let selectedResults = new Set([ + 'compliant', + 'non_compliant', + 'partially_compliant', + 'not_assessed', + 'not_applicable' +]);
136-139: Consider extracting the visibility logic to a separate function.The visibility condition is complex and would be more maintainable as a separate function.
+function isNodeHidden(node: Node, displayOnlyAssessableNodes: boolean): boolean { + const hasAssessableChildren = Object.keys(node.children || {}).length > 0; + return ( + !(!displayOnlyAssessableNodes || node.assessable || hasAssessableChildren) || + ((!selectedStatus.has(node.status) || !selectedResults.has(node.result)) && + node.assessable) + ); +} const hidden = - !(!$displayOnlyAssessableNodes || node.assessable || hasAssessableChildren) || - ((!selectedStatus.includes(node.status) || !selectedResults.includes(node.result)) && - node.assessable); + isNodeHidden(node, $displayOnlyAssessableNodes);
457-489: Consider extracting filter buttons to a separate component.The filter button implementation is duplicated for status and results. Consider extracting it to a reusable component.
<!-- FilterButtons.svelte --> <script lang="ts"> export let items: [string, string][]; export let selected: Set<string>; export let onToggle: (item: string) => void; </script> <div class="flex flex-wrap gap-2 text-xs bg-gray-100 border-2 p-1 rounded-md"> {#each items as [item, color]} <button type="button" on:click={() => onToggle(item)} class="px-2 py-1 rounded-md font-bold" style="background-color: {selected.has(item) ? color + '44' : 'grey'}; color: {selected.has(item) ? darkenColor(color, 0.3) : 'black'}; opacity: {selected.has(item) ? 1 : 0.5};" > {safeTranslate(item)} </button> {/each} </div>Usage:
-<div class="flex flex-wrap gap-2 ml-2 text-xs bg-gray-100 border-2 p-1 rounded-md"> - {#each Object.entries(complianceStatusColorMap) as [status, color]} - <button - type="button" - on:click={() => toggleStatus(status)} - class="px-2 py-1 rounded-md font-bold" - style="background-color: {selectedStatus.includes(status) - ? color + '44' - : 'grey'}; color: {selectedStatus.includes(status) - ? darkenColor(color, 0.3) - : 'black'}; opacity: {selectedStatus.includes(status) ? 1 : 0.5};" - > - {safeTranslate(status)} - </button> - {/each} -</div> +<FilterButtons + items={Object.entries(complianceStatusColorMap)} + selected={selectedStatus} + onToggle={toggleStatus} +/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/components/TreeView/RecursiveTreeViewItem.svelte(2 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte(4 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/TreeViewItemContent.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (3)
frontend/src/lib/components/TreeView/RecursiveTreeViewItem.svelte (1)
150-152: LGTM! Clean and readable class assignment.The class assignment logic is clear and effectively combines both direct and recursive hidden checks.
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/TreeViewItemContent.svelte (1)
159-166: LGTM! Clean status badge rendering logic.The status badge rendering logic is well-structured:
- Uses Object.entries for cleaner iteration
- Properly checks both existence of count and status selection
- Applies color with opacity for visual distinction
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (1)
117-131: LGTM! Clean and reusable toggle implementation.The toggle implementation is well-structured:
- Generic
toggleItemfunction for reusability- Type-specific wrapper functions for clarity
...d/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/TreeViewItemContent.svelte
Outdated
Show resolved
Hide resolved
Mohamed-Hacene
requested review from
ab-smith,
nas-tabchiche and
eric-intuitem
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (3)
109-116: Consider moving initial states to constants.The initial values for
selectedStatusandselectedResultscould be derived from the color map keys to ensure consistency and avoid duplication.-let selectedStatus = ['done', 'to_do', 'in_progress', 'in_review']; -let selectedResults = [ - 'compliant', - 'non_compliant', - 'partially_compliant', - 'not_assessed', - 'not_applicable' -]; +let selectedStatus = Object.keys(complianceStatusColorMap); +let selectedResults = Object.keys(complianceResultColorMap);
133-140: Simplify the node visibility logic.The visibility condition is complex and could be made more readable by breaking it down into smaller, named conditions.
function isNodeHidden(node: Node, displayOnlyAssessableNodes: boolean): boolean { const hasAssessableChildren = Object.keys(node.children || {}).length > 0; + const isVisibleByAssessableFilter = !displayOnlyAssessableNodes || node.assessable || hasAssessableChildren; + const isVisibleByStatusAndResult = !node.assessable || (selectedStatus.includes(node.status) && selectedResults.includes(node.result)); - return ( - !(!displayOnlyAssessableNodes || node.assessable || hasAssessableChildren) || - ((!selectedStatus.includes(node.status) || !selectedResults.includes(node.result)) && - node.assessable) - ); + return !isVisibleByAssessableFilter || !isVisibleByStatusAndResult; }
464-495: Extract a reusable filter button component.The filter button implementation is duplicated between status and result filters. Consider extracting it into a reusable component.
Create a new component
FilterButton.svelte:<script lang="ts"> export let item: string; export let color: string; export let isSelected: boolean; export let onToggle: (item: string) => void; </script> <button type="button" on:click={() => onToggle(item)} class="px-2 py-1 rounded-md font-bold" style="background-color: {isSelected ? color + '44' : 'grey'}; color: {isSelected ? darkenColor(color, 0.3) : 'black'}; opacity: {isSelected ? 1 : 0.5};" > {safeTranslate(item)} </button>Then simplify the usage:
<div class="flex flex-wrap gap-2 ml-2 text-xs bg-gray-100 border-2 p-1 rounded-md"> - {#each Object.entries(complianceStatusColorMap) as [status, color]} - <button - type="button" - on:click={() => toggleStatus(status)} - class="px-2 py-1 rounded-md font-bold" - style="background-color: {selectedStatus.includes(status) - ? color + '44' - : 'grey'}; color: {selectedStatus.includes(status) - ? darkenColor(color, 0.3) - : 'black'}; opacity: {selectedStatus.includes(status) ? 1 : 0.5};" - > - {safeTranslate(status)} - </button> - {/each} + {#each Object.entries(complianceStatusColorMap) as [status, color]} + <FilterButton + item={status} + color={color} + isSelected={selectedStatus.includes(status)} + onToggle={toggleStatus} + /> + {/each} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/messages/de.json(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte(4 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/TreeViewItemContent.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (10)
frontend/messages/nl.json (1)
870-871: Addition of Filter Key is Consistent
The new key"filterBy": "Filteren op"is correctly added and matches the naming convention used in other languages. This will enable the filtering feature consistently within the UI.frontend/messages/es.json (1)
870-871: Filter Key Addition is Correct
The inclusion of"filterBy": "Filtrar por"aligns well with the changes in the other localization files and supports the filtering functionality as intended.frontend/messages/de.json (1)
870-871: Add new localization key for filtering functionality.
The new key"filterBy": "Filtern nach"is correctly added and aligns with similar updates in other localization files. This change supports the new filtering feature on the compliance assessment page. Ensure that its usage in the UI matches the intended context.frontend/messages/en.json (2)
1172-1173: Clarify potential duplication for expiration date keys.
The addition of"expirationDate": "Expiration date"may overlap with the existing"expiryDate": "Expiry date". Please verify whether both keys are intended for separate contexts or if one should be deprecated for consistency.
1173-1173: New localization key for filtering functionality added.
The key"filterBy": "Filter by"is added consistently with other language files and supports the UI’s new filtering feature.frontend/messages/fr.json (1)
1163-1164: New Localization Key Added: "filterBy"The new key
"filterBy": "Filtrer par"is a clear and concise addition that aligns with similar changes in other localization files (e.g., the corresponding keys in the English, German, Spanish, etc.). This update supports the new filtering feature on the compliance assessment page as described in the PR objectives. While the key names are case-sensitive, ensure that similar keys across languages follow a consistent naming pattern.frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/TreeViewItemContent.svelte (2)
22-22: LGTM!The type annotation for
selectedStatusis correct and matches its usage in the template.
159-166: LGTM!The badge rendering logic is well-implemented:
- Uses Object.entries for cleaner iteration
- Correctly checks both resultCounts and selectedStatus
- Properly applies colors using template literals
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (2)
117-131: LGTM!The toggle functions are well-implemented:
- Clean and reusable logic
- Uses immutable array operations
- Properly typed parameters
524-524: LGTM!The key directive correctly includes filter states to trigger re-rendering when they change.
| "publicationDate": "Data publikacji", | ||
| "filterby": "Filtruj według" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Naming Consistency for Filter Key
The new entry for filtering is introduced as "filterby": "Filtruj według", but in the other localization files the key is "filterBy" (with a capital "B"). For consistency across the application, I recommend renaming it to "filterBy".
Proposed diff:
-"filterby": "Filtruj według"
+"filterBy": "Filtruj według"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "publicationDate": "Data publikacji", | |
| "filterby": "Filtruj według" | |
| "publicationDate": "Data publikacji", | |
| "filterBy": "Filtruj według" |
nas-tabchiche
deleted the
CA-860-Add-a-filter-on-compliance-status-for-compliance-assessment
branch
Summary by CodeRabbit
New Features
Refactor