feat(ui): aggregate risk scenarios in risk matrix to avoid bloating #1407
Conversation
WalkthroughA new Svelte component Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
![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 (6)
frontend/src/lib/components/RiskMatrix/Cell.svelte (5)
7-8: Improve type safety for props.The
cellDataprop usesArray<any>which loses type safety. Consider creating an interface for the data items.+interface CellDataItem { + ref_id: string; + // add other required properties +} -export let cellData: Array<any> = []; +export let cellData: Array<CellDataItem> = [];
5-8: Add JSDoc comments for props.Add documentation for the props to improve maintainability and developer experience.
+/** The cell configuration containing level and color information */ export let cell; +/** Scale factor for bubble size calculation */ export let bubbleSizeScale: number = 0; +/** Array of data items to be displayed in the cell */ export let cellData: Array<any> = []; +/** Component to render individual data items */ export let dataItemComponent;
10-17: Add error handling for missing ref_id and improve readability.The popup target generation assumes all items have a ref_id property and the line is quite long.
let popupClick: PopupSettings; if (cellData.length) { + const targetId = cellData + .map(e => e.ref_id ?? 'unknown') + .join('-'); popupClick = { event: 'click', - target: 'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-'), + target: `popup-data-${targetId}`, placement: 'top' }; }
19-22: Extract magic numbers and add validation for bubble size calculation.The bubble size calculation uses magic numbers and lacks validation.
+const MIN_BUBBLE_SIZE = 7; +const MAX_SCALE_FACTOR = 9; +const SIZE_TO_REM_RATIO = 4; $: classesBubbleSize = (itemNumber: number) => { - itemNumber = (itemNumber / bubbleSizeScale) * 9 + 7; // rescaled to 16 + if (itemNumber < 0 || bubbleSizeScale < 0) { + console.warn('Invalid bubble size parameters'); + return ''; + } + const scaledSize = (itemNumber / bubbleSizeScale) * MAX_SCALE_FACTOR + MIN_BUBBLE_SIZE; - return `width: ${itemNumber / 4}rem; height: ${itemNumber / 4}rem`; + return `width: ${scaledSize / SIZE_TO_REM_RATIO}rem; height: ${scaledSize / SIZE_TO_REM_RATIO}rem`; };
54-62: Maintain consistency with main cell and improve accessibility.The fallback cell has inconsistent styling and lacks accessibility attributes.
<div class="flex flex-wrap items-center space-x-1 justify-center h-full [&>*]:pointer-events-none whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText( cell.level.hexcolor )}" style="background-color: {cell.level.hexcolor};" data-testid="cell" + role="cell" + aria-label="Empty risk matrix cell" > - <div class="mx-auto text-center">{cellData}</div> + <div class="mx-auto text-center" aria-hidden="true">{cellData}</div> </div>frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte (1)
44-49: Optimize array operations and add error handling.The calculation of
biggestCelldataLengthcould be optimized and needs error handling.let biggestCelldataLength: number; if (displayedData) { - biggestCelldataLength = displayedData - .flat(1) - .reduce((max, e) => (e.length > max.length ? e : max), []).length; + try { + const flatData = displayedData.flat(1); + if (flatData.length === 0) { + biggestCelldataLength = 0; + } else { + biggestCelldataLength = Math.max(...flatData.map(e => e.length)); + } + } catch (error) { + console.error('Error calculating cell data length:', error); + biggestCelldataLength = 0; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/RiskMatrix/Cell.svelte(1 hunks)frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: migrations-check (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte (1)
90-99: 🛠️ Refactor suggestionAdd key prop and make props passing more explicit.
The Cell component iteration needs a key prop and props passing could be more explicit.
{#if displayedData} <Cell + key={`cell-${i}-${j}`} {cell} cellData={displayedData[i][j]} bubbleSizeScale={biggestCelldataLength} {dataItemComponent} /> {:else} - <Cell {cell} {dataItemComponent} /> + <Cell + key={`cell-${i}-${j}`} + {cell} + cellData={[]} + bubbleSizeScale={0} + {dataItemComponent} + /> {/if}Likely invalid or redundant comment.
| {#if cellData.length && dataItemComponent} | ||
| <div | ||
| class="flex flex-wrap items-center space-x-1 justify-center h-full cursor-pointer whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText( | ||
| cell.level.hexcolor | ||
| )}" | ||
| style="background-color: {cell.level.hexcolor};" | ||
| data-testid="cell" | ||
| use:popup={popupClick} | ||
| > | ||
| <div | ||
| class="bg-surface-900/80 rounded-full flex justify-center items-center text-center text-white" | ||
| style="{classesBubbleSize(cellData.length)};" | ||
| > | ||
| {cellData.length} | ||
| </div> | ||
| <div | ||
| class="card bg-black text-gray-200 p-4 z-20" | ||
| data-popup={'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-')} | ||
| > | ||
| {#each cellData as item} | ||
| <svelte:component this={dataItemComponent} data={item} /> | ||
| {/each} | ||
| <div class="arrow bg-black" /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and overflow handling.
The cell needs accessibility improvements and better overflow handling.
<div
class="flex flex-wrap items-center space-x-1 justify-center h-full cursor-pointer whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText(
cell.level.hexcolor
)}"
style="background-color: {cell.level.hexcolor};"
data-testid="cell"
+ role="button"
+ aria-label="Click to view {cellData.length} risk scenarios"
+ tabindex="0"
use:popup={popupClick}
>
<div
class="bg-surface-900/80 rounded-full flex justify-center items-center text-center text-white"
style="{classesBubbleSize(cellData.length)};"
+ aria-hidden="true"
>
{cellData.length}
</div>
<div
- class="card bg-black text-gray-200 p-4 z-20"
+ class="card bg-black text-gray-200 p-4 z-20 max-h-[80vh] overflow-y-auto"
data-popup={'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-')}
+ role="dialog"
+ aria-label="Risk scenarios details"
>📝 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.
| {#if cellData.length && dataItemComponent} | |
| <div | |
| class="flex flex-wrap items-center space-x-1 justify-center h-full cursor-pointer whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText( | |
| cell.level.hexcolor | |
| )}" | |
| style="background-color: {cell.level.hexcolor};" | |
| data-testid="cell" | |
| use:popup={popupClick} | |
| > | |
| <div | |
| class="bg-surface-900/80 rounded-full flex justify-center items-center text-center text-white" | |
| style="{classesBubbleSize(cellData.length)};" | |
| > | |
| {cellData.length} | |
| </div> | |
| <div | |
| class="card bg-black text-gray-200 p-4 z-20" | |
| data-popup={'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-')} | |
| > | |
| {#each cellData as item} | |
| <svelte:component this={dataItemComponent} data={item} /> | |
| {/each} | |
| <div class="arrow bg-black" /> | |
| </div> | |
| </div> | |
| {#if cellData.length && dataItemComponent} | |
| <div | |
| class="flex flex-wrap items-center space-x-1 justify-center h-full cursor-pointer whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText( | |
| cell.level.hexcolor | |
| )}" | |
| style="background-color: {cell.level.hexcolor};" | |
| data-testid="cell" | |
| role="button" | |
| aria-label="Click to view {cellData.length} risk scenarios" | |
| tabindex="0" | |
| use:popup={popupClick} | |
| > | |
| <div | |
| class="bg-surface-900/80 rounded-full flex justify-center items-center text-center text-white" | |
| style="{classesBubbleSize(cellData.length)};" | |
| aria-hidden="true" | |
| > | |
| {cellData.length} | |
| </div> | |
| <div | |
| class="card bg-black text-gray-200 p-4 z-20 max-h-[80vh] overflow-y-auto" | |
| data-popup={'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-')} | |
| role="dialog" | |
| aria-label="Risk scenarios details" | |
| > | |
| {#each cellData as item} | |
| <svelte:component this={dataItemComponent} data={item} /> | |
| {/each} | |
| <div class="arrow bg-black" /> | |
| </div> | |
| </div> | |
| {/if} |
|
Bubbles are currently sized according to the cell presenting most risk scenarios. This should be changed to a fixed scale to avoid confusion. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/lib/components/RiskMatrix/Cell.svelte (1)
5-7: Add TypeScript interfaces and JSDoc comments for props.The props would benefit from stronger typing and documentation:
- Replace
Array<any>with a specific interface- Add JSDoc comments describing the purpose and shape of each prop
+interface CellData { + ref_id: string; + // Add other required properties +} + +/** The cell configuration containing level and color information */ export let cell; +/** Array of risk scenarios to be displayed in this cell */ -export let cellData: Array<any> = []; +export let cellData: CellData[] = []; +/** The component to render for each risk scenario */ export let dataItemComponent;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/RiskMatrix/Cell.svelte(1 hunks)frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: startup-functional-test (3.12)
- 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)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
frontend/src/lib/components/RiskMatrix/Cell.svelte (2)
41-82: Previous accessibility suggestions still need to be implemented.The accessibility improvements suggested in the previous review remain relevant.
10-18: Revise bubble size calculation as per PR comments.As noted in the PR comments, the current implementation sizes bubbles relative to the cell with most scenarios. Consider using a fixed scale instead to avoid user confusion.
Suggested approach:
- Define a minimum and maximum bubble size
- Use a linear scale between these sizes
- Cap the size at the maximum even for larger counts
-const bubbleSizeRanges = [ - { max: 3, value: 1.5 }, - { max: 5, value: 2 }, - { max: 8, value: 2.5 }, - { max: 13, value: 3 }, - { max: 21, value: 3.5 }, - { max: 34, value: 4 }, - { max: 55, value: 4.25 } -]; +const MIN_BUBBLE_SIZE = 1.5; +const MAX_BUBBLE_SIZE = 3; +const MAX_COUNT_FOR_SCALING = 20;frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte (1)
83-87: LGTM! Clean implementation of the Cell component.The changes effectively modularize the cell rendering logic by using the new Cell component.
| if (cellData.length) { | ||
| popupClick = { | ||
| event: 'click', | ||
| target: 'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-'), | ||
| placement: 'top' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve popup target generation and add validation.
The current implementation has potential issues:
- Long concatenated strings of ref_ids could exceed length limits
- No validation of ref_ids existence
if (cellData.length) {
+ const validRefIds = cellData
+ .filter(item => item.ref_id)
+ .map(item => item.ref_id);
+ if (validRefIds.length !== cellData.length) {
+ console.warn('Some items are missing ref_id');
+ }
popupClick = {
event: 'click',
- target: 'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-'),
+ target: `popup-${validRefIds.slice(0, 3).join('-')}`,
placement: 'top'
};
}📝 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.
| if (cellData.length) { | |
| popupClick = { | |
| event: 'click', | |
| target: 'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-'), | |
| placement: 'top' | |
| }; | |
| } | |
| if (cellData.length) { | |
| const validRefIds = cellData | |
| .filter(item => item.ref_id) | |
| .map(item => item.ref_id); | |
| if (validRefIds.length !== cellData.length) { | |
| console.warn('Some items are missing ref_id'); | |
| } | |
| popupClick = { | |
| event: 'click', | |
| target: `popup-${validRefIds.slice(0, 3).join('-')}`, | |
| placement: 'top' | |
| }; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/lib/components/RiskMatrix/Cell.svelte (2)
29-35: Improve bubble size calculation logic.The bubble size calculation function has the following issues:
- No default case for values exceeding the maximum range (55)
- Could be simplified using Array.find
-$: classesBubbleSize = (itemCount: number) => { - for (const range of bubbleSizeRanges) { - if (itemCount <= range.max) { - return `width: ${range.value}rem; height: ${range.value}rem`; - } - } -}; +$: classesBubbleSize = (itemCount: number) => { + const range = bubbleSizeRanges.find(range => itemCount <= range.max) + ?? bubbleSizeRanges[bubbleSizeRanges.length - 1]; + return `width: ${range.value}rem; height: ${range.value}rem`; +};
56-65: Fix popup arrow visibility.The popup arrow might be hidden due to
overflow-y-scrollon the parent div. Consider moving the arrow outside the scrollable area.-<div - class="card bg-surface-300 text-black p-4 z-20 max-h-56 overflow-y-scroll" - data-popup={'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-')} -> - {#each cellData as item} - <svelte:component this={dataItemComponent} data={item} /> - {/each} - <!-- not displayed because of overflow-y-scroll on parent --> - <div class="arrow bg-surface-300" /> -</div> +<div class="card bg-surface-300 text-black z-20 relative"> + <div class="p-4 max-h-56 overflow-y-auto"> + {#each cellData as item} + <svelte:component this={dataItemComponent} data={item} /> + {/each} + </div> + <div class="arrow bg-surface-300 absolute" /> +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/RiskMatrix/Cell.svelte(1 hunks)frontend/src/lib/components/RiskMatrix/RiskScenarioItem.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/lib/components/RiskMatrix/RiskScenarioItem.svelte
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
frontend/src/lib/components/RiskMatrix/Cell.svelte (2)
21-27: Improve popup target generation and add validation.The current implementation has potential issues:
- Long concatenated strings of ref_ids could exceed length limits
- No validation of ref_ids existence
41-66: Improve accessibility and overflow handling.The cell needs accessibility improvements and better overflow handling.
| export let cell; | ||
| export let cellData: Array<any> = []; | ||
| export let dataItemComponent; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript types for props.
The current prop declarations lack proper typing, which reduces type safety. Consider adding proper TypeScript interfaces and type validation.
+interface CellLevel {
+ hexcolor: string;
+}
+
+interface Cell {
+ level: CellLevel;
+}
+
+interface RiskScenario {
+ ref_id: string;
+ // Add other required properties
+}
+
+interface SvelteComponent {
+ // Add required component properties
+}
+
-export let cell;
-export let cellData: Array<any> = [];
-export let dataItemComponent;
+export let cell: Cell;
+export let cellData: RiskScenario[] = [];
+export let dataItemComponent: typeof SvelteComponent;Committable suggestion skipped: line range outside the PR's diff.
|
I've set the minimum risk scenario to 3 in a cell to dislay a bubble (was 2). So now, cells with one or two risk scenarios are displayed it as plain text |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/lib/components/RiskMatrix/Cell.svelte (1)
21-29: 🛠️ Refactor suggestionValidate concatenated popup target strings.
Concatenatingref_idarrays could get overly long or fail if items lackref_id. This issue was flagged in a previous review. Consider sanitizing or truncating.
🧹 Nitpick comments (1)
frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte (1)
24-24: Consider a threshold alignment for bubbles.
You setuseBubbles = true;here, but your stated intention in the PR was to display a bubble only if risk scenarios ≥ 3. If that’s still valid, consider passing an explicit threshold or adjusting the logic in theCell.sveltefile to ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/components/RiskMatrix/Cell.svelte(1 hunks)frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte(3 hunks)frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: functional-tests (3.12, chromium)
🔇 Additional comments (9)
frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte (2)
271-271: Confirm consistent usage of the bubble feature.
You’re passinguseBubblesas a prop, which is good for modularity. Verify that all risk matrix cells are intended to show bubbles in this particular risk assessment context.
282-282: Same bubble activation logic.
Similarly, ensure that the residual risk matrix’s cells should also display bubbles at the same threshold as the current risk matrix. This preserves a consistent user experience.frontend/src/lib/components/RiskMatrix/Cell.svelte (4)
5-9: Add stronger typing for props.
ExportingcellData: Array<any>reduces type safety. Previously, a similar suggestion was made to define detailed interfaces for these props (e.g., an interface forRiskScenario).
30-41: Efficient reactive styling.
The functionsclassesBubbleSizeandclassesCellTextare clear and straightforward. If performance becomes a concern, memoize these to avoid repeated computations.
43-69: Add basic accessibility enhancements.
To improve keyboard navigation and screen reader support, consider addingrole="button"andaria-labelattributes. This is functionally the same request as a prior accessibility suggestion.
70-85: Fallback behavior matches new bubble logic.
WhenuseBubblesis false or item count is under the threshold, you correctly show the fallback content. Just ensure you updatebubbleMinCount(currently 1) if you truly intend to hide bubbles for fewer than 3 risk scenarios.frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte (3)
3-3: Good modular design with Cell import.
Importing theCellcomponent keeps the matrix logic cleaner and more maintainable.
17-17: DefaultinguseBubblesto false.
This default ensures backward compatibility. Just confirm that all consuming code explicitly opts in to the new bubble behavior if desired.
84-87: Effective conditional cell rendering.
Using<Cell>with or withoutdisplayedDataclarifies the matrix logic. This separation keeps the codebase organized and flexible for future changes.
| const bubbleMinCount = 1; | ||
| const maxBubbleSize = 4.5; | ||
| const bubbleSizeRanges = [ | ||
| { max: 3, value: 1.5 }, | ||
| { max: 5, value: 2 }, | ||
| { max: 8, value: 2.5 }, | ||
| { max: 13, value: 3 }, | ||
| { max: 21, value: 3.5 }, | ||
| { max: 34, value: 4 } | ||
| ]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revisit bubble size thresholds.
Your code dynamically grows bubble sizes but might conflict with user feedback requesting a uniform/fixed scale. Also, bubbleMinCount is set to 1, but your PR notes a threshold of 3. Consider aligning this logic with the agreed bubble sizing policy.
-const bubbleMinCount = 1;
+const bubbleMinCount = 3; 📝 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.
| const bubbleMinCount = 1; | |
| const maxBubbleSize = 4.5; | |
| const bubbleSizeRanges = [ | |
| { max: 3, value: 1.5 }, | |
| { max: 5, value: 2 }, | |
| { max: 8, value: 2.5 }, | |
| { max: 13, value: 3 }, | |
| { max: 21, value: 3.5 }, | |
| { max: 34, value: 4 } | |
| ]; | |
| const bubbleMinCount = 3; | |
| const maxBubbleSize = 4.5; | |
| const bubbleSizeRanges = [ | |
| { max: 3, value: 1.5 }, | |
| { max: 5, value: 2 }, | |
| { max: 8, value: 2.5 }, | |
| { max: 13, value: 3 }, | |
| { max: 21, value: 3.5 }, | |
| { max: 34, value: 4 } | |
| ]; | |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/lib/components/RiskMatrix/Cell.svelte (1)
69-83: Improve plain display accessibility and error handling.The plain display needs accessibility improvements and better error handling.
<div class="flex flex-wrap flex-col items-center justify-center h-full [&>*]:pointer-events-none whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText( cell.level.hexcolor )}" style="background-color: {cell.level.hexcolor};" data-testid="cell" + role="region" + aria-label="Risk scenarios" > {#if dataItemComponent} - {#each cellData as item} + {#each cellData as item, i (item?.id)} <svelte:component this={dataItemComponent} data={item} /> {/each} {:else} - <div class="mx-auto text-center">{cellData}</div> + <div class="mx-auto text-center"> + {#if Array.isArray(cellData) && cellData.length} + {cellData} + {:else} + <span aria-label="No risk scenarios">-</span> + {/if} + </div> {/if} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/RiskMatrix/Cell.svelte(1 hunks)frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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: Analyze (python)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
frontend/src/lib/components/RiskMatrix/Cell.svelte (3)
5-9: Add proper TypeScript types for props.The props lack proper typing, which reduces type safety. This is particularly concerning for
cellData: Array<any>.
11-20:⚠️ Potential issueUpdate bubble configuration to match requirements.
The current implementation has two issues that conflict with the PR objectives:
bubbleMinCountis set to 1, but should be 3 per PR notes- Dynamic bubble sizing via ranges conflicts with the request for a fixed scale
Apply this diff to align with requirements:
-const bubbleMinCount = 1; -const maxBubbleSize = 4.5; -const bubbleSizeRanges = [ - { max: 3, value: 1.5 }, - { max: 5, value: 2 }, - { max: 8, value: 2.5 }, - { max: 13, value: 3 }, - { max: 21, value: 3.5 }, - { max: 34, value: 4 } -]; +const bubbleMinCount = 3; +const bubbleSize = 3; // Fixed size as requestedLikely invalid or redundant comment.
44-67: 🛠️ Refactor suggestionEnhance accessibility and user experience.
The bubble view needs accessibility and UX improvements:
- Missing keyboard navigation
- Scroll behavior could be improved
- Missing ARIA attributes
Apply these improvements:
{#if useBubbles && cellData.length >= bubbleMinCount && dataItemComponent} <div class="flex flex-wrap items-center space-x-1 justify-center h-full cursor-pointer whitespace-normal overflow-y-scroll hide-scrollbar group {classesCellText( cell.level.hexcolor )}" style="background-color: {cell.level.hexcolor};" data-testid="cell" + role="button" + tabindex="0" + on:keydown={(e) => e.key === 'Enter' && e.target.click()} + aria-label="{cellData.length} risk scenarios" use:popup={popupClick} > <div class="bg-surface-900/70 rounded-full flex justify-center items-center text-center text-white transition-colors group-hover:bg-surface-900/100 duration-300" - style="{classesBubbleSize(cellData.length)};" + style="{classesBubbleSize()};" + aria-hidden="true" > {cellData.length} </div> - <div class="card bg-surface-300 text-black z-20" data-popup={popupTarget}> - <div class="max-h-56 overflow-y-scroll p-4"> + <div + class="card bg-surface-300 text-black z-20" + data-popup={popupTarget} + role="dialog" + aria-label="Risk scenarios details" + > + <div class="max-h-56 overflow-y-auto p-4 focus:outline-none"> {#each cellData as item} <svelte:component this={dataItemComponent} data={item} /> {/each} <div class="arrow bg-surface-300" /> </div> </div> </div>Likely invalid or redundant comment.
| $: classesBubbleSize = (itemCount: number) => { | ||
| for (const range of bubbleSizeRanges) { | ||
| if (itemCount <= range.max) { | ||
| return `width: ${range.value}rem; height: ${range.value}rem`; | ||
| } | ||
| } | ||
| return `width: ${maxBubbleSize}rem; height: ${maxBubbleSize}rem`; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify bubble sizing logic for fixed scale.
Since we're moving to a fixed bubble size as per requirements, this reactive statement can be simplified.
-$: classesBubbleSize = (itemCount: number) => {
- for (const range of bubbleSizeRanges) {
- if (itemCount <= range.max) {
- return `width: ${range.value}rem; height: ${range.value}rem`;
- }
- }
- return `width: ${maxBubbleSize}rem; height: ${maxBubbleSize}rem`;
-};
+$: classesBubbleSize = () => `width: ${bubbleSize}rem; height: ${bubbleSize}rem`;Committable suggestion skipped: line range outside the PR's diff.
| let popupClick: PopupSettings; | ||
| if (cellData.length && popupTarget) { | ||
| popupClick = { | ||
| event: 'click', | ||
| target: popupTarget, | ||
| placement: 'top' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and accessibility for popup.
The popup configuration needs validation and accessibility improvements:
- No validation of popupTarget
- Missing ARIA attributes for accessibility
Apply these improvements:
let popupClick: PopupSettings;
-if (cellData.length && popupTarget) {
+if (cellData.length && typeof popupTarget === 'string' && popupTarget.trim()) {
popupClick = {
event: 'click',
target: popupTarget,
- placement: 'top'
+ placement: 'top',
+ // Add ARIA attributes
+ ariaLabel: `${cellData.length} risk scenarios`,
+ ariaExpanded: false
};
+} else if (cellData.length) {
+ console.warn('Invalid popupTarget provided for cell with data');
}Committable suggestion skipped: line range outside the PR's diff.
ab-smith
deleted the
CA-819-aggregate-risk-scenarios-in-risk-matrix-to-avoid-bloating
branch
|
relates to #1434 |
Only numbers of risks scenarios are now displayed in bubbles inside each cell.
You can access the list of risks scenarios from a cell by clicking on it, it will show up in a tooltip
Summary by CodeRabbit
New Features
Cellcomponent for the risk matrix.Improvements
RiskScenarioItem.useBubblesproperty to control bubble display in the risk matrix.