feat: parametric ebios radar #1379
Conversation
…ticality-radar' into parametric_ebios_radar
WalkthroughThe pull request introduces enhancements to the application's global settings, focusing on adding configurable parameters for an Ebios RM radar feature. Changes span across backend and frontend files, introducing new settings keys, view methods, localization entries, and form components. The modifications allow dynamic configuration of radar parameters like maximum radius and zone radii, with updates to serializers, views, schemas, and user interface components to support these new settings. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (2)
🪧 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: 2
🧹 Nitpick comments (10)
backend/global_settings/serializers.py (1)
7-10: Ensure validation consistency for new keys.These newly added keys effectively expand the scope of global settings. Consider adding extra validation or type checks (e.g., integer vs. float) within the serializer or model to guarantee data integrity.
frontend/src/lib/components/Forms/NumberField.svelte (1)
61-61: Expose user-friendly increments.Using the
stepattribute is a great way to guide numeric input. If the defaultstepdoesn’t match expected usage patterns, consider clarifying the increments in help text or adding min/max constraints for more robust validation.frontend/src/lib/components/Forms/ModelForm/GeneralSettingForm.svelte (2)
15-30: Well-structured grouping for security objectives.Wrapping the security objective field in an
AccordionItemclarifies that these fields belong to a distinct settings category, enhancing clarity for users.
31-69: Consistent numeric fields for eBios radar parameters.The new
<NumberField>entries for all radar parameters follow a coherent naming pattern. Consider providing user-facing help text or tooltips if these values significantly affect UI behaviors (e.g., chart scaling) to reduce confusion about their usage and valid ranges.backend/global_settings/views.py (2)
62-68: Centralize default settings configuration.Storing default radar values here is convenient. However, moving them into a dedicated constants module or settings file can improve maintainability and make them more discoverable for future updates.
70-78: Partial merge ensures preservation of existing entries.Merging existing settings with defaults avoids overwriting user values. This is a solid approach. Consider logging or notifying administrators when default values are injected for transparency.
frontend/src/lib/components/Chart/EcosystemRadarChart.svelte (2)
16-19: Add defensive default values or checks for new exported variables.
These new properties (max,greenZoneRadius,yellowZoneRadius,redZoneRadius) rely on$page.data.settings. If the backend doesn’t populate them or if they're out-of-range (e.g., negative), the chart might display incorrectly.export let max = $page.data.settings.ebios_radar_max; export let greenZoneRadius = $page.data.settings.ebios_radar_green_zone_radius; export let yellowZoneRadius = $page.data.settings.ebios_radar_yellow_zone_radius; export let redZoneRadius = $page.data.settings.ebios_radar_red_zone_radius; + // Possible fallback assignment: + if (!max) max = 6; + if (!greenZoneRadius) greenZoneRadius = 1; + // etc.
220-223: Adjust or validate the yellow zone color and radius.
- The color
#F8EA47might have low contrast on certain backgrounds.- Similar range-bounding concerns as with
redZoneRadius.frontend/src/lib/utils/schemas.ts (1)
293-297: Consider constraints or default values for newly added numeric properties.
Whilez.number()ensures these fields are numeric, adding.min(0)or.default()might help avoid invalid or missing values.- ebios_radar_max: z.number(), + ebios_radar_max: z.number().min(1).default(6),frontend/messages/en.json (1)
1131-1136: Maintain consistent capitalization and styling for UI labels.
“Max Radius” uses Title Case, whereas the zone radius labels use standard capitalization. For a more uniform look, consider a consistent approach (e.g., “Max radius,” “Green zone radius,” etc.)."maxRadius": "Max radius",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/global_settings/serializers.py(1 hunks)backend/global_settings/views.py(2 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Chart/EcosystemRadarChart.svelte(6 hunks)frontend/src/lib/components/Forms/ModelForm/GeneralSettingForm.svelte(1 hunks)frontend/src/lib/components/Forms/NumberField.svelte(2 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)
🔇 Additional comments (7)
frontend/src/lib/components/Forms/NumberField.svelte (1)
9-9: Confirm step positivity and numeric type.The
stepprop defaults to1. Ensure that you validate or document that negative or zero values are handled appropriately (or disallowed) in relevant use cases.frontend/src/lib/components/Forms/ModelForm/GeneralSettingForm.svelte (2)
3-3: Import alignment looks good.Adding
NumberFieldsupports numeric input for new radar parameters. Looks correct and consistent with your existing form approach.
7-7: Accordion usage is a strong design choice.Using
@skeletonlabs/skeleton’sAccordionfor grouping settings improves usability and organization.frontend/src/lib/components/Chart/EcosystemRadarChart.svelte (3)
131-131: Guard against invalidmaxvalues.
Ifmaxis 0 or negative, the chart may break. Consider bounding or validating the value before assigning it to theradiusAxis.
203-203: EnsureredZoneRadiusmatchesmax.
IfredZoneRadiusis equal to or greater thanmax, the visualization might be misleading. You could ensure it remains strictly below or within a logical range.
237-240: Consistent usage for the green zone styling.
This implementation follows the same logic as red and yellow zones. Looks coherent as-is.frontend/messages/fr.json (1)
1131-1136: Translations and naming look good.
The newly added Ebios radar labels align well with the rest of the French localization. No issues detected.
| @action(detail=True, name="Get ebios rm radar parameters") | ||
| def ebios_radar_parameters(self, request): | ||
| ebios_rm_parameters = { | ||
| "ebios_radar_max": self.get_object().value.get("ebios_radar_max"), | ||
| "ebios_radar_green_zone_radius": self.get_object().value.get( | ||
| "ebios_radar_green_zone_radius" | ||
| ), | ||
| "ebios_radar_yellow_zone_radius": self.get_object().value.get( | ||
| "ebios_radar_yellow_zone_radius" | ||
| ), | ||
| "ebios_radar_red_zone_radius": self.get_object().value.get( | ||
| "ebios_radar_red_zone_radius" | ||
| ), | ||
| } | ||
| return Response(ebios_rm_parameters) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Endpoint clarity for eBios radar parameters.
This action method clearly fetches only the relevant radar settings. Consider adding default fallback values (e.g., get("ebios_radar_max", 0)) or explicit error handling for missing keys, ensuring robust responses in every scenario.
| @@ -2,6 +2,7 @@ | |||
| import { onMount } from 'svelte'; | |||
| import { safeTranslate } from '$lib/utils/i18n'; | |||
| import * as m from '$paraglide/messages.js'; | |||
| import { page } from '$app/stores'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Add fallback values for all radar chart settings
Direct access to $page.data.settings properties could cause runtime errors. Add optional chaining and fallback values for all radar chart configuration values:
- export let max = $page.data.settings.ebios_radar_max;
- export let greenZoneRadius = $page.data.settings.ebios_radar_green_zone_radius;
- export let yellowZoneRadius = $page.data.settings.ebios_radar_yellow_zone_radius;
- export let redZoneRadius = $page.data.settings.ebios_radar_red_zone_radius;
+ export let max = $page?.data?.settings?.ebios_radar_max ?? 6;
+ export let greenZoneRadius = $page?.data?.settings?.ebios_radar_green_zone_radius ?? 2;
+ export let yellowZoneRadius = $page?.data?.settings?.ebios_radar_yellow_zone_radius ?? 4;
+ export let redZoneRadius = $page?.data?.settings?.ebios_radar_red_zone_radius ?? 6;🔗 Analysis chain
Consider reactivity and fallback handling for $page.data
If $page.data.settings is undefined or the keys are missing, accessing them directly may cause runtime errors. You might add optional chaining or default values.
- import { page } from '$app/stores';
+ import { page } from '$app/stores';
// Example fallback usage:
+ export let max = $page?.data?.settings?.ebios_radar_max ?? 6;🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# View the full component code
cat frontend/src/lib/components/Chart/EcosystemRadarChart.svelte
# Find usage patterns of $page.data
ast-grep --pattern '$page.data'
Length of output: 7200
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/src/lib/components/Chart/EcosystemRadarChart.svelte (2)
197-203: Extract color constants for better maintainabilityConsider extracting the zone colors into constants at the top of the file for better maintainability and reusability.
+const ZONE_COLORS = { + RED: '#E73E51', + YELLOW: '#F8EA47', + GREEN: '#00ADA8' +}; // ... later in the code ... lineStyle: { - color: '#E73E51', + color: ZONE_COLORS.RED, width: 5 }Also applies to: 214-222, 230-238
257-257: Improve clarity of center dot positioningThe small offset (0.001) in the center dot position appears to be a workaround. Consider extracting this magic number into a named constant to improve code clarity.
+const CENTER_DOT_OFFSET = 0.001; // ... later in the code ... -data: [[max - 0.001, 0]], +data: [[max - CENTER_DOT_OFFSET, 0]],Also applies to: 274-274
backend/ebios_rm/helpers.py (3)
50-51: Handle missing 'general' settings gracefully.
GlobalSettings.objects.get(name="general")will fail if the "general" settings entry is not found. Consider adding error handling or a fallback to ensure system resilience when the "general" config is missing.Example adjustment:
try: - max_val = GlobalSettings.objects.get(name="general").value.get("ebios_radar_max", 6) + global_settings = GlobalSettings.objects.get(name="general") + max_val = global_settings.value.get("ebios_radar_max", 6) except GlobalSettings.DoesNotExist: + # fallback or logging + max_val = 6
60-61: Unify and clarify criticality capping logic.Capping criticalities at
max_val - 1 + 0.25might be confusing to maintain. Consider aligning the cap tomax_valor otherwise clarifying the rationale in comments for future maintainers.Refactor suggestion to centralize capping:
- c_criticality = ( - math.floor(sh.current_criticality * 100) / 100.0 - if sh.current_criticality <= max_val - else max_val - 1 + 0.25 - ) + def cap_criticality(value, max_val): + return math.floor(value * 100) / 100.0 if value <= max_val else max_val - 1 + 0.25 + ... + c_criticality = cap_criticality(sh.current_criticality, max_val)
80-81: Remove code duplication for residual criticality.
r_criticalityapplies the same capping logic asc_criticality. Extract a helper function to avoid repetitive code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/ebios_rm/helpers.py(5 hunks)frontend/src/lib/components/Chart/EcosystemRadarChart.svelte(6 hunks)frontend/src/lib/components/Forms/ModelForm/GeneralSettingForm.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
frontend/src/lib/components/Chart/EcosystemRadarChart.svelte (2)
16-19: Add fallback values for radar chart settingsDirect access to
$page.data.settingsproperties could cause runtime errors.Apply this diff to add optional chaining and fallback values:
-export let max = $page.data.settings.ebios_radar_max; -export let greenZoneRadius = $page.data.settings.ebios_radar_green_zone_radius; -export let yellowZoneRadius = $page.data.settings.ebios_radar_yellow_zone_radius; -export let redZoneRadius = $page.data.settings.ebios_radar_red_zone_radius; +export let max = $page?.data?.settings?.ebios_radar_max ?? 6; +export let greenZoneRadius = $page?.data?.settings?.ebios_radar_green_zone_radius ?? 2; +export let yellowZoneRadius = $page?.data?.settings?.ebios_radar_yellow_zone_radius ?? 4; +export let redZoneRadius = $page?.data?.settings?.ebios_radar_red_zone_radius ?? 6;
131-131: LGTM!The radiusAxis max value correctly uses the configurable parameter.
frontend/src/lib/components/Forms/ModelForm/GeneralSettingForm.svelte (2)
3-7: LGTM! Clean import additions.The new imports for NumberField and Skeleton UI components are properly organized and typed.
15-30: LGTM! Well-structured asset section.Good use of Accordion for better organization. The Select component is properly configured with all necessary bindings and internationalization.
backend/ebios_rm/helpers.py (1)
4-4: Consider potential import dependencies.If the
GlobalSettingsmodel references Ebios modules, there's a risk of circular imports. Otherwise, this import is acceptable for retrieving configuration values.Run this script to ensure no cyclic dependencies have been introduced:
✅ Verification successful
Import dependency is safe and correctly structured
The
GlobalSettingsimport is used appropriately for configuration purposes, and there are no circular dependencies. The dependency is one-way (ebios_rm → global_settings) which is the correct pattern for configuration modules.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for cyclical imports referencing 'ebios_rm/helpers.py' or 'global_settings/models.py' rg "import ebios_rm.helpers" -A 5 rg "import global_settings.models" -A 5Length of output: 77
Script:
#!/bin/bash # Search for any imports or references between the two modules echo "=== Checking ebios_rm directory for global_settings references ===" rg "from.*global_settings|import.*global_settings" backend/ebios_rm/ || true echo -e "\n=== Checking global_settings directory for ebios_rm references ===" rg "from.*ebios_rm|import.*ebios_rm" backend/global_settings/ || true echo -e "\n=== Checking GlobalSettings class usage in ebios_rm ===" rg "GlobalSettings" backend/ebios_rm/ || true echo -e "\n=== Checking for any ebios_rm references in global_settings ===" rg "ebios_rm" backend/global_settings/ || trueLength of output: 1196
| <AccordionItem open> | ||
| <svelte:fragment slot="summary" | ||
| ><i class="fa-solid fa-gopuram mr-2"></i>{m.ebiosRadarParameters()}</svelte:fragment | ||
| > | ||
| <svelte:fragment slot="content"> | ||
| <NumberField | ||
| {form} | ||
| field="ebios_radar_max" | ||
| label={m.maxRadius()} | ||
| min={6} | ||
| max={16} | ||
| step={0.1} | ||
| cacheLock={cacheLocks['ebios_radar_max']} | ||
| bind:cachedValue={formDataCache['ebios_radar_max']} | ||
| /> | ||
| <NumberField | ||
| {form} | ||
| field="ebios_radar_green_zone_radius" | ||
| label={m.greenZoneRadius()} | ||
| step={0.1} | ||
| cacheLock={cacheLocks['ebios_radar_green_zone_radius']} | ||
| bind:cachedValue={formDataCache['ebios_radar_green_zone_radius']} | ||
| /> | ||
| <NumberField | ||
| {form} | ||
| field="ebios_radar_yellow_zone_radius" | ||
| label={m.yellowZoneRadius()} | ||
| step={0.1} | ||
| cacheLock={cacheLocks['ebios_radar_yellow_zone_radius']} | ||
| bind:cachedValue={formDataCache['ebios_radar_yellow_zone_radius']} | ||
| /> | ||
| <NumberField | ||
| {form} | ||
| field="ebios_radar_red_zone_radius" | ||
| label={m.redZoneRadius()} | ||
| step={0.1} | ||
| cacheLock={cacheLocks['ebios_radar_red_zone_radius']} | ||
| bind:cachedValue={formDataCache['ebios_radar_red_zone_radius']} | ||
| /> | ||
| </svelte:fragment> | ||
| </AccordionItem> |
There was a problem hiding this comment.
Add validation constraints for radar zone parameters.
The radar zone configuration needs additional constraints to prevent invalid configurations:
- Missing min/max constraints on zone radius fields
- No validation to ensure zone radii follow a logical order (green > yellow > red)
- No validation against the max radius
- Missing help text to explain these parameters
Apply these improvements:
<NumberField
{form}
field="ebios_radar_green_zone_radius"
label={m.greenZoneRadius()}
+ min={0}
+ max={form.data.ebios_radar_max}
step={0.1}
+ helpText={m.greenZoneRadiusHelpText()}
cacheLock={cacheLocks['ebios_radar_green_zone_radius']}
bind:cachedValue={formDataCache['ebios_radar_green_zone_radius']}
/>
<NumberField
{form}
field="ebios_radar_yellow_zone_radius"
label={m.yellowZoneRadius()}
+ min={0}
+ max={form.data.ebios_radar_green_zone_radius}
step={0.1}
+ helpText={m.yellowZoneRadiusHelpText()}
cacheLock={cacheLocks['ebios_radar_yellow_zone_radius']}
bind:cachedValue={formDataCache['ebios_radar_yellow_zone_radius']}
/>
<NumberField
{form}
field="ebios_radar_red_zone_radius"
label={m.redZoneRadius()}
+ min={0}
+ max={form.data.ebios_radar_yellow_zone_radius}
step={0.1}
+ helpText={m.redZoneRadiusHelpText()}
cacheLock={cacheLocks['ebios_radar_red_zone_radius']}
bind:cachedValue={formDataCache['ebios_radar_red_zone_radius']}
/>Additionally, consider adding reactive validation to ensure the radii maintain their logical order:
<script lang="ts">
$: if (form.data.ebios_radar_green_zone_radius <= form.data.ebios_radar_yellow_zone_radius ||
form.data.ebios_radar_yellow_zone_radius <= form.data.ebios_radar_red_zone_radius) {
form.errors.ebios_radar_green_zone_radius = m.invalidRadiiOrder();
}
</script>
Summary by CodeRabbit
New Features
Improvements
Localization
Bug Fixes