feat(ux): Display library related objects #2826
Conversation
monsieurswag
changed the title
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a Django migration adjusting three ForeignKey fields, a LoadedLibrary.get_related_objects() method, a new backend Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant FrontendServer
participant BackendAPI
participant Database
User->>Browser: Open library detail
Browser->>FrontendServer: GET /loaded-libraries/{id}
FrontendServer->>BackendAPI: GET /api/loaded-libraries/{id}
FrontendServer->>BackendAPI: GET /api/loaded-libraries/{id}/related-objects
BackendAPI->>Database: LoadedLibrary.get_related_objects(): query related models
Database-->>BackendAPI: return aggregated related objects
BackendAPI-->>FrontendServer: related-objects JSON (count, list)
FrontendServer-->>Browser: page + relatedObjects data
Browser->>Browser: build relatedObjectsTable (getObjectHref)
Browser->>Browser: render ModelTable (hrefKey -> use rowMetaData.href)
User->>Browser: click related object row
Browser->>Browser: navigate to rowMetaData.href
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/models.py (1)
4350-4356: Same collision note for RiskAssessment.risk_matrix related_nameThis sets related_name="risk_assessments" on RiskMatrix, which collides with RiskMatrix.risk_assessments property. Align with the resolution proposed earlier (rename property or pick a different related_name and adjust annotate/prefetch here).
🧹 Nitpick comments (4)
backend/core/models.py (1)
905-973: Type annotation and duplication risk
- The return type hints don’t match actual objects returned (RiskScenario, RiskAssessment, AppliedControl, ComplianceAssessment, LoadedLibrary). Update the annotation or return model IDs only.
- Possible duplicates across buckets (e.g., same AppliedControl referenced by RA and CA). Consider deduplication.
Example dedupe and lighter payload building:
- related_objects = [] + related = [] for threat in threats: - related_objects.extend(threat.risk_scenarios.all()) + related.extend(threat.risk_scenarios.all()) for risk_matrix in risk_matrices: - related_objects.extend(risk_matrix.risk_assessments.all()) + related.extend(risk_matrix.risk_assessments.all()) for reference_control in risk_assessment_reference_controls: - related_objects.extend(reference_control.applied_controls.all()) + related.extend(reference_control.applied_controls.all()) for framework in frameworks: - related_objects.extend(framework.compliance_assessments.all()) + related.extend(framework.compliance_assessments.all()) for reference_control in compliance_assessment_reference_controls: - related_objects.extend(reference_control.applied_controls.all()) + related.extend(reference_control.applied_controls.all()) - related_objects.extend(libraries) - return related_objects + related.extend(libraries) + # Deduplicate by model+id + seen, unique = set(), [] + for obj in related: + key = (obj._meta.label_lower, obj.pk) + if key not in seen: + seen.add(key) + unique.append(obj) + return uniquefrontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+server.ts (2)
12-21: Guard against non‑JSON error bodiesBackend returns plain text for some 4xx paths. Calling relatedObjectsRes.json() in an error branch can throw. Use text() fallback or try/catch.
Example:
- if (!relatedObjectsRes.ok) - error(relatedObjectsRes.status as NumericRange<400, 599>, await relatedObjectsRes.json()); + if (!relatedObjectsRes.ok) { + const body = await relatedObjectsRes.text().catch(() => ''); + error(relatedObjectsRes.status as NumericRange<400, 599>, body || 'Error'); + }
23-28: Optional: avoid materializing full objects when only ids/names are neededSince the API only needs id/name/type, consider having the backend return that projection directly (as it already does), or in future, fetch only what’s necessary to keep payloads lean.
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte (1)
60-74: Simplify exhaustive switch with explicit return type.The switch statement exhaustively handles all cases in the
RelatedObject['type']union, making the finalreturn nullat line 73 unreachable. TypeScript's exhaustiveness checking can be leveraged to remove this dead code.Apply this diff:
- function getObjectHref(obj: RelatedObject): string | null { + function getObjectHref(obj: RelatedObject): string { switch (obj.type) { case 'RiskAssessment': return `/risk-assessments/${obj.id}`; case 'RiskScenario': return `/risk-scenarios/${obj.id}`; case 'AppliedControl': return `/applied-controls/${obj.id}`; case 'LoadedLibrary': return `/loaded-libraries/${obj.id}?loaded`; case 'ComplianceAssessment': return `/compliance-assessments/${obj.id}`; } - return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/core/migrations/0110_alter_appliedcontrol_reference_control_and_more.py(1 hunks)backend/core/models.py(5 hunks)backend/library/views.py(1 hunks)frontend/src/lib/components/ModelTable/ModelTable.svelte(3 hunks)frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte(3 hunks)frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+server.ts(1 hunks)frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/tree/+server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/tree/+server.ts (3)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+server.ts (1)
GET(6-34)frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/tree/+server.ts (1)
GET(6-19)frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/+server.ts (1)
GET(6-25)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+server.ts (2)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)backend/library/views.py (2)
content(194-200)content(457-463)
backend/library/views.py (1)
backend/core/models.py (2)
LoadedLibrary(856-1035)get_related_objects(905-972)
backend/core/models.py (2)
backend/library/views.py (1)
related_objects(486-503)backend/core/tests/fixtures.py (1)
applied_controls(58-69)
🪛 GitHub Actions: Backend Linters
backend/core/migrations/0110_alter_appliedcontrol_reference_control_and_more.py
[error] 1-1: Command 'ruff format --check .' failed: 1 file would be reformatted (core/migrations/0110_alter_appliedcontrol_reference_control_and_more.py). Run 'ruff format' to fix.
⏰ 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). (7)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/tree/+server.ts (1)
6-9: LGTMSignature cleanup is fine; behavior unchanged.
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts (1)
4-4: LGTMType‑only import is appropriate; no behavior change.
backend/core/migrations/0110_alter_appliedcontrol_reference_control_and_more.py (1)
1-29: Fix the incorrect review claims and verify pre-existing reverse accessor collisionThe formatting claim is incorrect:
ruff formatsucceeded (output: "1 file reformatted").The reverse accessor collision is real but pre-existing: RiskAssessment has two ForeignKeys both using
related_name="risk_assessments"(lines 4355 to RiskMatrix and 4369 to EbiosRMStudy), and RiskMatrix already defines a@property risk_assessments. This conflict exists in the model layer regardless of this migration. Verify whether this collision causes runtime issues in your environment and resolve by renaming one of the conflicting related_names or the property.frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte (1)
21-34: LGTM! Well-defined type interfaces.The
RelatedObjectandRelatedObjectResponseinterfaces are clearly structured with an exhaustive union type for object types. This provides good type safety for the related objects feature.
| RiskMatrix.objects.filter(library=self) | ||
| .annotate(risk_assessment_count=Count("risk_assessments")) | ||
| .filter(risk_assessment_count__gt=0) | ||
| .prefetch_related("risk_assessments") | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Blocking: related_name conflicts with existing property RiskMatrix.risk_assessments
You introduced related_name="risk_assessments" (see RiskAssessment.risk_matrix), but RiskMatrix already defines a property named risk_assessments. Django will raise a reverse accessor clash at startup; annotate/prefetch on "risk_assessments" will also be affected.
Two viable fixes:
- Prefer removing/renaming the RiskMatrix.risk_assessments property, and rely on the reverse manager, or
- Rename the related_name (and adjust usages here to match).
Example (property rename to avoid the clash):
- @property
- def risk_assessments(self) -> list:
- return RiskAssessment.objects.filter(risk_matrix=self)
+ # Avoid clashing with the reverse accessor name
+ @property
+ def risk_assessments_qs(self):
+ return RiskAssessment.objects.filter(risk_matrix=self)If you keep the related_name as "risk_assessments", update any call sites of the property accordingly.
🏁 Script executed:
#!/bin/bash
# Verify clashes and usages
rg -nP --type=py -C2 '\bclass\s+RiskMatrix\b.*?@property\s+def\s+risk_assessments\b' backend/core/models.py
rg -nP --type=py '\.risk_assessments\b(?!\()'Length of output: 419
</thinking>
I need to gather more information about the actual model definitions and the conflict. Let me search for the relevant code.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find RiskAssessment model and check for related_name on ForeignKey fields
rg -nP --type=py -B5 -A5 'class RiskAssessment' backend/core/models.py | head -50
# Look for ForeignKey pointing to RiskMatrix with related_name
rg -nP --type=py 'ForeignKey.*RiskMatrix.*related_name' backend/core/models.py
# Find RiskMatrix class and check for risk_assessments property
rg -nP --type=py -A30 'class RiskMatrix' backend/core/models.py | grep -A10 '@property' | head -30
Length of output: 672
🏁 Script executed:
#!/bin/bash
# More targeted: Find the exact @property definition
rg -nP --type=py '@property' backend/core/models.py | grep -A2 'risk_assessments'
# Get context around line 960 to understand what object risk_matrix is
sed -n '950,970p' backend/core/models.py
# Get context around line 3923
sed -n '3915,3930p' backend/core/models.pyLength of output: 1202
🏁 Script executed:
#!/bin/bash
# Find RiskAssessment model definition and ForeignKey fields
rg -nP --type=py -B2 -A20 'class RiskAssessment' backend/core/models.py
# Find RiskMatrix property definition
rg -nP --type=py -B2 -A8 'def risk_assessments' backend/core/models.py | head -40Length of output: 1907
🏁 Script executed:
#!/bin/bash
# Get context around the usage lines
sed -n '955,965p' backend/core/models.py
sed -n '3918,3928p' backend/core/models.py
sed -n '6770,6780p' backend/core/models.pyLength of output: 1026
🏁 Script executed:
#!/bin/bash
# Check if risk_matrix field in RiskAssessment has related_name specified
rg -nP --type=py 'risk_matrix\s*=\s*ForeignKey' backend/core/models.py -A2Length of output: 0
Blocking: related_name="risk_assessments" conflicts with RiskMatrix.risk_assessments property
The conflict is confirmed. RiskAssessment.risk_matrix has related_name="risk_assessments" (line 4355), but RiskMatrix defines a property with the same name (line 1560). Django will raise a reverse accessor clash error at startup.
This must be resolved by either:
- Removing or renaming the RiskMatrix.risk_assessments property (line 1560), or
- Renaming the related_name and updating usages
The code snippet at lines 918-922 relies on the related_name for .prefetch_related("risk_assessments"), so the related_name should be kept. Rename the property:
- @property
- def risk_assessments(self) -> list:
- return RiskAssessment.objects.filter(risk_matrix=self)
+ @property
+ def risk_assessments_list(self) -> list:
+ return RiskAssessment.objects.filter(risk_matrix=self)Update line 960 accordingly: risk_matrix.risk_assessments.all() → risk_matrix.risk_assessments_list.all() (or use the reverse manager directly).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/core/models.py around lines 918–922 there is a reverse accessor
clash: RiskAssessment.risk_matrix uses related_name="risk_assessments" (see
~line 4355) while RiskMatrix defines a property named risk_assessments (~line
1560); keep the related_name and rename the RiskMatrix property to avoid Django
startup errors (suggestion: rename to risk_assessments_list), update the
property name definition at ~line 1560 and change all its usages (for example
update the call at ~line 960 from risk_matrix.risk_assessments.all() to
risk_matrix.risk_assessments_list.all()), and run tests / grep the repo to
ensure no other references to the old property remain.
| compliance_assessment_reference_controls = ( | ||
| ReferenceControl.objects.filter(library=self) | ||
| .annotate( | ||
| compliance_assessment_count=Count( | ||
| "applied_controls__requirement_assessments__compliance_assessment" | ||
| ) | ||
| ) | ||
| .filter(compliance_assessment_count=0) | ||
| .prefetch_related("applied_controls") | ||
| ) |
There was a problem hiding this comment.
Bug: wrong filter operator (should be > 0, not == 0)
compliance_assessment_reference_controls filters compliance_assessment_count=0, which returns RCs NOT linked to any compliance assessment. Given the goal (display related objects), this should be compliance_assessment_count__gt=0.
Apply:
- .filter(compliance_assessment_count=0)
+ .filter(compliance_assessment_count__gt=0)📝 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.
| compliance_assessment_reference_controls = ( | |
| ReferenceControl.objects.filter(library=self) | |
| .annotate( | |
| compliance_assessment_count=Count( | |
| "applied_controls__requirement_assessments__compliance_assessment" | |
| ) | |
| ) | |
| .filter(compliance_assessment_count=0) | |
| .prefetch_related("applied_controls") | |
| ) | |
| compliance_assessment_reference_controls = ( | |
| ReferenceControl.objects.filter(library=self) | |
| .annotate( | |
| compliance_assessment_count=Count( | |
| "applied_controls__requirement_assessments__compliance_assessment" | |
| ) | |
| ) | |
| .filter(compliance_assessment_count__gt=0) | |
| .prefetch_related("applied_controls") | |
| ) |
🤖 Prompt for AI Agents
In backend/core/models.py around lines 942 to 951, the queryset filters
ReferenceControl objects with compliance_assessment_count=0 which returns
controls NOT linked to any compliance assessment; change the filter to
compliance_assessment_count__gt=0 so it returns only ReferenceControls that have
one or more linked compliance assessments. Update the .filter(...) call
accordingly (use __gt=0) and keep the annotate and prefetch_related as-is.
| related_name="applied_controls", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Reverse accessor renamed; update call sites using appliedcontrol_set
Changing related_name to "applied_controls" breaks any usages of reference_control.appliedcontrol_set. There’s at least one in this file:
- ReferenceControl.is_deletable uses self.appliedcontrol_set.exists()
Please update to self.applied_controls.exists().
🏁 Script executed:
#!/bin/bash
# Find broken reverse accessor usages
rg -nP --type=py '\bappliedcontrol_set\b'Length of output: 102
---
🏁 Script executed:
```shell
#!/bin/bash
# Get context around line 1511 to verify method and class membership
sed -n '1480,1530p' backend/core/models.py | head -60
Length of output: 1452
🏁 Script executed:
#!/bin/bash
# Also search for the is_deletable method to confirm its location and content
rg -nP --type=py -B5 -A15 'def is_deletable' backend/core/models.pyLength of output: 1942
🏁 Script executed:
#!/bin/bash
# Verify the related_name change at lines 3681-3682
sed -n '3675,3690p' backend/core/models.pyLength of output: 504
Update reverse accessor at line 1511
The change to related_name="applied_controls" requires updating the reverse accessor usage in ReferenceControl.is_deletable() at line 1511:
Change self.appliedcontrol_set.exists() to self.applied_controls.exists()
🤖 Prompt for AI Agents
In backend/core/models.py around lines 3681-3682 (and update
ReferenceControl.is_deletable at line 1511), the reverse accessor was changed to
related_name="applied_controls", so replace any usage of
self.appliedcontrol_set.exists() with self.applied_controls.exists() (and update
similar appliedcontrol_set references to applied_controls where present) to
match the new related_name.
| @action(detail=True, methods=["get"], url_path="related-objects") | ||
| def related_objects(self, request, pk): | ||
| try: | ||
| key = "urn" if pk.startswith("urn:") else "id" | ||
| lib = LoadedLibrary.objects.get(**{key: pk}) | ||
| except: | ||
| return Response("Library not found.", status=HTTP_404_NOT_FOUND) | ||
|
|
||
| related_objects = lib.get_related_objects() | ||
| serialized_related_objects = [ | ||
| {"type": type(obj).__name__, "id": obj.id, "name": obj.name} | ||
| for obj in related_objects | ||
| ] | ||
| return Response( | ||
| { | ||
| "count": len(serialized_related_objects), | ||
| "objects": serialized_related_objects, | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Return JSON errors and avoid bare except
The action returns plain strings on errors; frontend calls res.json(), which will throw. Return JSON consistently and catch specific exceptions.
Apply:
- except:
- return Response("Library not found.", status=HTTP_404_NOT_FOUND)
+ except LoadedLibrary.DoesNotExist:
+ return Response({"error": "libraryNotFound"}, status=HTTP_404_NOT_FOUND)Also consider enforcing a uniform error shape across content/tree endpoints for consistency.
🤖 Prompt for AI Agents
In backend/library/views.py around lines 485 to 504, the method uses a bare
except and returns plain string errors; change to catch the specific exception
(LoadedLibrary.DoesNotExist) and return a JSON response with a consistent error
shape (e.g. {"error": "Library not found."} or {"detail": "Library not found."})
and HTTP_404_NOT_FOUND status; ensure any other predictable failures are caught
explicitly (e.g. ValueError if parsing pk logic is added) and return the same
JSON error shape so this endpoint matches the content/tree endpoints' uniform
error format.
| baseEndpoint?: string; | ||
| detailQueryParameter?: string; | ||
| fields?: string[]; | ||
| hrefKey?: string | null; |
There was a problem hiding this comment.
Misleading prop name and type.
The hrefKey prop is typed as string | null and named as if it's a key to look up an href in metadata, but the implementation only checks its truthiness as a boolean flag (line 174). The actual href is always read from rowMetaData.href.
Consider renaming for clarity:
- hrefKey?: string | null;
+ useMetadataHref?: boolean;- hrefKey = null,
+ useMetadataHref = false,Then update the usage at line 174:
- if (hrefKey) {
+ if (useMetadataHref) {Also applies to: 127-127
🤖 Prompt for AI Agents
In frontend/src/lib/components/ModelTable/ModelTable.svelte around lines 78, 127
and 174, the prop named hrefKey is misleading and typed as string | null but
only used as a boolean flag; rename it to enableRowHref (or isHrefEnabled) and
change its type to boolean (or boolean | undefined), update all prop
references/prop export accordingly, and replace the truthiness checks at line
174 (and the usage at 127) to check the new boolean prop; alternatively, if you
intend to support a dynamic key, keep hrefKey as a string and change the
implementation at line 174 to read the href from rowMetaData[hrefKey] instead of
rowMetaData.href—pick one approach and make consistent changes to the prop
declaration and all usages.
| if (hrefKey) { | ||
| console.log(rowMetaData); | ||
| const href = rowMetaData.href; | ||
| if (href) goto(href); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Remove debug logging and improve error handling.
Two issues with this navigation logic:
- Line 175 contains a
console.logthat should be removed before merging to production. - When
hrefis falsy, the function returns silently without feedback to the user or fallback navigation.
Apply this diff:
if (hrefKey) {
- console.log(rowMetaData);
const href = rowMetaData.href;
- if (href) goto(href);
+ if (href) {
+ goto(href);
+ } else {
+ console.warn('Navigation href missing from row metadata', rowMetaData);
+ }
return;
}📝 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 (hrefKey) { | |
| console.log(rowMetaData); | |
| const href = rowMetaData.href; | |
| if (href) goto(href); | |
| return; | |
| } | |
| if (hrefKey) { | |
| const href = rowMetaData.href; | |
| if (href) { | |
| goto(href); | |
| } else { | |
| console.warn('Navigation href missing from row metadata', rowMetaData); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
In frontend/src/lib/components/ModelTable/ModelTable.svelte around lines 173 to
179, remove the debug console.log(rowMetaData) and replace the silent return
when href is falsy with a clear fallback: either navigate to a safe default
route (e.g., a model details list) or show user feedback (toast/notification)
indicating the link is unavailable; ensure the function neither logs debug info
nor returns silently but handles the missing href path explicitly.
| objects: RelatedObject[]; | ||
| } | ||
| const libraryRelatedObjects: RelatedObjectResponse = data?.library?.relatedObjects ?? []; |
There was a problem hiding this comment.
Fix type mismatch in default value.
The default value [] doesn't match the RelatedObjectResponse type, which expects an object with count and objects properties. This will cause runtime errors when the data is missing and the code tries to access libraryRelatedObjects.count (line 189) or libraryRelatedObjects.objects (lines 81-82).
Apply this diff:
- const libraryRelatedObjects: RelatedObjectResponse = data?.library?.relatedObjects ?? [];
+ const libraryRelatedObjects: RelatedObjectResponse = data?.library?.relatedObjects ?? { count: 0, objects: [] };📝 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 libraryRelatedObjects: RelatedObjectResponse = data?.library?.relatedObjects ?? []; | |
| const libraryRelatedObjects: RelatedObjectResponse = data?.library?.relatedObjects ?? { count: 0, objects: [] }; |
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte
around line 36, the default value for libraryRelatedObjects is an empty array
which mismatches the RelatedObjectResponse type; replace the default with an
object matching that type (e.g. { count: 0, objects: [] }) so accesses to .count
and .objects are safe and keep the declared type RelatedObjectResponse.
| const relatedObjectsTable: TableSource = { | ||
| head: { | ||
| type: 'type', | ||
| name: 'name' | ||
| }, | ||
| body: tableSourceMapper(libraryRelatedObjects.objects, ['type', 'name']), | ||
| meta: libraryRelatedObjects.objects.map((obj) => { | ||
| return { href: getObjectHref(obj) }; | ||
| }) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Integration logic is sound.
The relatedObjectsTable construction correctly maps related objects to table rows with href metadata, and the conditional rendering based on count is appropriate. Once the hrefKey prop in ModelTable is renamed (as suggested in the ModelTable review), update line 197 accordingly.
Verify that the href values generated by getObjectHref correctly navigate to existing routes in the application:
Also applies to: 189-205
🏁 Script executed:
#!/bin/bash
# Check that target routes exist for related object types
echo "Checking route files for related object types..."
# Check for risk-assessments route
fd -t f "\+page.svelte" frontend/src/routes | rg "risk-assessments/\[id"
# Check for risk-scenarios route
fd -t f "\+page.svelte" frontend/src/routes | rg "risk-scenarios/\[id"
# Check for applied-controls route
fd -t f "\+page.svelte" frontend/src/routes | rg "applied-controls/\[id"
# Check for loaded-libraries route
fd -t f "\+page.svelte" frontend/src/routes | rg "loaded-libraries/\[id"
# Check for compliance-assessments route
fd -t f "\+page.svelte" frontend/src/routes | rg "compliance-assessments/\[id"Length of output: 1004
The web search results don't contain information about this specific codebase's getObjectHref function. Let me search the codebase directly for this function.
🏁 Script executed:
#!/bin/bash
# Find getObjectHref function definition
echo "Searching for getObjectHref function..."
rg -n "getObjectHref" frontend/src
echo -e "\n\nSearching for function definition..."
rg -nA 20 "function getObjectHref|const getObjectHref|export.*getObjectHref" frontend/src
echo -e "\n\nSearching for 'applied-controls' references..."
rg -n "applied-controls" frontend/src --type ts --type js
echo -e "\n\nSearching for libraryRelatedObjects to understand what types it returns..."
rg -nA 5 "libraryRelatedObjects" frontend/src/routes/Length of output: 13364
I found a critical issue. Let me verify whether AppliedControl objects can actually appear in the libraryRelatedObjects response.
🏁 Script executed:
#!/bin/bash
# Find RelatedObject and RelatedObjectResponse type definitions
echo "Searching for RelatedObject and RelatedObjectResponse type definitions..."
rg -nA 10 "type RelatedObject|interface RelatedObject|type RelatedObjectResponse|interface RelatedObjectResponse" frontend/src
echo -e "\n\nChecking what types are actually returned from the API..."
rg -nB 5 -A 10 "relatedObjects" frontend/src/lib --type ts --type js | head -100Length of output: 2468
🏁 Script executed:
#!/bin/bash
# Verify applied-controls has NO detail page route
echo "Confirming absence of applied-controls detail route..."
find frontend/src/routes -path "*applied-controls*" -name "+page.svelte" | grep -v flash-mode
echo "If empty above, no detail page exists for applied-controls"Length of output: 113
Applied-controls detail route is missing and will cause 404 errors.
The RelatedObject type explicitly includes 'AppliedControl' as a valid type, and getObjectHref returns /applied-controls/${obj.id} for that case. However, no route exists at frontend/src/routes/(app)/(internal)/applied-controls/[id=uuid]/+page.svelte. When AppliedControl objects appear in related objects, users will encounter broken links.
Either remove 'AppliedControl' from the RelatedObject type if it shouldn't appear in related objects, or create the missing detail page route.
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte
around lines 76 to 85, links for RelatedObject may point to
/applied-controls/${obj.id} but the route
frontend/src/routes/(app)/(internal)/applied-controls/[id=uuid]/+page.svelte
does not exist, causing 404s; either remove 'AppliedControl' from the
RelatedObject union/type (and update any upstream code that supplies those
related objects) so getObjectHref will never generate that path, or add the
missing route file
frontend/src/routes/(app)/(internal)/applied-controls/[id=uuid]/+page.svelte
that accepts the id param and renders the AppliedControl detail (fetching data
by id and handling not-found), and ensure getObjectHref and any navigation use
the exact path and param name so links resolve correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/library/views.py (1)
489-490: Return JSON error response instead of plain string.The error response returns a plain string, which will cause the frontend's
.json()parsing to fail. This is consistent with the concern raised in previous reviews.Apply this diff to return a consistent JSON error response:
- if lib is None: - return Response("Library not found.", status=HTTP_404_NOT_FOUND) + if lib is None: + return Response({"error": "Library not found."}, status=HTTP_404_NOT_FOUND)
🧹 Nitpick comments (1)
backend/library/views.py (1)
493-496: Consider using DRF serializers for consistency.The manual serialization works but bypasses DRF's serialization benefits (validation, consistent formatting, field control). Consider creating a lightweight serializer for related objects.
Example approach:
class RelatedObjectSerializer(serializers.Serializer): type = serializers.SerializerMethodField() id = serializers.UUIDField() name = serializers.CharField() def get_type(self, obj): return type(obj).__name__Then use it in the view:
- serialized_related_objects = [ - {"type": type(obj).__name__, "id": obj.id, "name": obj.name} - for obj in related_objects - ] + serialized_related_objects = RelatedObjectSerializer(related_objects, many=True).data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/migrations/0110_alter_appliedcontrol_reference_control_and_more.py(1 hunks)backend/library/views.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/core/migrations/0110_alter_appliedcontrol_reference_control_and_more.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/library/views.py (1)
backend/core/models.py (2)
LoadedLibrary(856-1035)get_related_objects(905-972)
⏰ 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). (26)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/settings/sso.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/login.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/enterprise/settings/client.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/settings/general.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/mappings.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/ebios-rm.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/user-route.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/analytics.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/startup.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/login.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/incidents.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/nav.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/business-impact-analysis.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/common.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/settings/sso.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/libraries.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/mappings.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/ebios-rm.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/findings-assessments.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/compliance-assessments.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/settings/general.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/startup.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/analytics.test.ts)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
🔇 Additional comments (2)
backend/library/views.py (2)
485-486: LGTM!The action decorator and method signature are correctly configured.
492-492: Line number correction needed; verify filter semantics for compliance_assessment_reference_controls.The filter logic is at line 949 (not 948 as stated). The query filters
compliance_assessment_reference_controlswithcompliance_assessment_count=0, which differs from the frameworks query at line 938 that usescompliance_assessment_count__gt=0. These filters select opposite sets of data (reference controls without assessments vs. frameworks with assessments). Confirm whether this semantic difference is intentional or whether both should usecount__gt=0to be consistent.
| @action(detail=True, methods=["get"], url_path="related-objects") | ||
| def related_objects(self, request, pk): | ||
| key = "urn" if pk.startswith("urn:") else "id" | ||
| lib = LoadedLibrary.objects.filter(**{key: pk}).first() | ||
| if lib is None: | ||
| return Response("Library not found.", status=HTTP_404_NOT_FOUND) | ||
|
|
||
| related_objects = lib.get_related_objects() | ||
| serialized_related_objects = [ | ||
| {"type": type(obj).__name__, "id": obj.id, "name": obj.name} | ||
| for obj in related_objects | ||
| ] | ||
| return Response( | ||
| { | ||
| "count": len(serialized_related_objects), | ||
| "objects": serialized_related_objects, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Add permission check for security.
This endpoint lacks permission validation, unlike other similar methods in this class (e.g., retrieve() at line 420). Users without proper permissions could access library relationship data.
Add a permission check at the beginning of the method:
@action(detail=True, methods=["get"], url_path="related-objects")
def related_objects(self, request, pk):
+ if "view_loadedlibrary" not in request.user.permissions:
+ return Response(status=HTTP_403_FORBIDDEN)
key = "urn" if pk.startswith("urn:") else "id"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/library/views.py around lines 485 to 502, the related_objects action
is missing an object-level permission check; after you load the library (keep
the existing key/lookup and 404 return if lib is None), call
self.check_object_permissions(request, lib) before accessing or serializing
related objects so the method enforces the same permissions as retrieve() and
prevents unauthorized access.
Add standart permission
monsieurswag
force-pushed
the
feat/show_library_related_objects
branch
from
e967a3d to
e95effd
Compare
Summary by CodeRabbit
New Features
Bug Fixes / UX