-
Notifications
You must be signed in to change notification settings - Fork 895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: manual topology manipulation #3326
Conversation
Signed-off-by: 35C4n0r <[email protected]>
@35C4n0r is attempting to deploy a commit to the KeepHQ Team on Vercel. A member of the Team first needs to authorize it. |
Linting in a few mins. |
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any migrations needed? if you have service topology map exists, do I need to change from int to str or smth? did you check ti?
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3326 +/- ##
==========================================
+ Coverage 30.92% 31.22% +0.29%
==========================================
Files 70 70
Lines 8184 8234 +50
==========================================
+ Hits 2531 2571 +40
- Misses 5653 5663 +10 β View full report in Codecov by Sentry. |
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@35C4n0r can we add unit tests and perhaps an e2e test to all the functionality you added? also - you mentioned import/export, is it in this PR?
Signed-off-by: 35C4n0r <[email protected]>
Signed-off-by: 35C4n0r <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I didn't review in depth but I played with it and the experience was great. I fixed a small issue not related with this PR and pushed it.
@talboren: I'll review the changes in this PR. β Actions performedReview triggered.
|
1 similar comment
@talboren: I'll review the changes in this PR. β Actions performedReview triggered.
|
π WalkthroughWalkthroughThis change set updates multiple parts of the topology module. The modifications include updates to TypeScript interfaces and hooks in the UI to support string-based identifiers and manual flags, along with refinements in component behavior. New endpoints and DTOs have been added to the API for creating, updating, deleting, importing, and exporting topology services and their dependencies. In addition, a new side panel component is introduced for adding or editing nodes, and corresponding tests have been updated to validate these behaviors. Changes
Sequence Diagram(s)Add/Edit Node Side Panel FlowsequenceDiagram
participant U as User
participant SP as AddEditNodeSidePanel
participant API as Topology API
participant DB as Database
U->>SP: Open side panel & fill in details
SP->>SP: Validate form input
SP->>API: Submit create/update request
API->>DB: Update topology service record
DB-->>API: Return confirmation
API-->>SP: Pass success response
SP->>U: Notify success and close panel
Topology Import Flow via TopologyMapsequenceDiagram
participant U as User
participant TM as TopologyMap UI
participant API as Topology API Route
participant DB as Database
U->>TM: Upload YAML file
TM->>API: Send file for import
API->>DB: Parse file and update topology data
DB-->>API: Return import result
API-->>TM: Forward confirmation
TM->>U: Display import success message
Poem
β¨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
π§Ή Nitpick comments (28)
keep-ui/components/SidePanel.tsx (2)
21-21
: Consider adjusting z-index hierarchy.The Dialog wrapper (z-50) has a higher z-index than its Panel content (z-30). This could potentially cause stacking context issues. Consider making them consistent:
- <Dialog as="div" className="relative z-50" onClose={onClose}> + <Dialog as="div" className="relative z-[100]" onClose={onClose}> - className={`fixed right-0 inset-y-0 ${panelWidth} bg-white z-30 flex flex-col p-6`} + className={`fixed right-0 inset-y-0 ${panelWidth} bg-white z-[100] flex flex-col p-6`}Using explicit numbers (like 100) instead of Tailwind's default scale makes the z-index relationship clearer and less likely to conflict with other components.
Also applies to: 46-46
20-21
: Add aria-label for better accessibility.While the dialog is semantic and the overlay is properly hidden, adding an aria-label would improve screen reader experience.
- <Dialog as="div" className="relative z-50" onClose={onClose}> + <Dialog + as="div" + className="relative z-50" + onClose={onClose} + aria-label="Side panel" + >keep-ui/app/(keep)/topology/model/models.ts (1)
35-36
: Provide documentation for thetopologyMutator
usage.The new interface
TopologyServiceWithMutator
includestopologyMutator
, which suggests a direct way to revalidate or refresh data. Ensure thereβs a clear explanation or examples of how to use this mutator to avoid confusion among team members.keep-ui/app/(keep)/topology/ui/map/manage-selection.tsx (3)
46-49
: Encourage descriptive naming for UI states.
isSidePanelOpen
andserviceToEdit
are clear, but you might consider more descriptive naming or JSDoc comments if these states are manipulated in multiple places for clarity.
80-119
: Refactor the early returns for clarity.The updated
updateSelectedServicesAndEdges
function reads well, but consider grouping thereturn
statements together or adding short comments for each condition, e.g., βAvoid dropping selection in modal,β βEdges selected β skip service selection,β etc. This improves maintainability.
294-310
: Consider broader edit edge features.
renderEditEdgeToolBar
nicely surfaces the delete or edit actions. Eventually, you might expand to other edge attributesβensuring the UI design remains consistent and future-proof.keep/api/routes/topology.py (4)
295-314
: Add clearer documentation for the manual service creation.The
create_service
endpoint logic is straightforward. Consider adding a docstring or inline comment on howis_manual
is enforced under the hood (e.g., the service model always setsis_manual
to True).π§° Tools
πͺ Ruff (0.8.2)
298-300: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
299-299: Do not perform function call
IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
301-301: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
311-313: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
342-369
: Partial or bulk deletion clarifications.Deleting multiple services requires careful error handling if some are manual and some are not. If partial successes are possible, consider returning partial results or an aggregated error message rather than entirely failing.
π§° Tools
πͺ Ruff (0.8.2)
345-347: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
346-346: Do not perform function call
IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
348-348: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
360-363: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
365-365: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
367-369: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
456-497
: Remove unnecessary explicit boolean conversions inexport_topology_yaml
.Line 478 sets
services_dict["is_manual"] = True if services_dict["is_manual"] is True else False
. Replace with a direct cast or assignment, e.g.:- services_dict["is_manual"] = True if services_dict["is_manual"] is True else False + services_dict["is_manual"] = bool(services_dict["is_manual"])π§° Tools
πͺ Ruff (0.8.2)
462-464: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
463-463: Do not perform function call
IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
465-465: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
478-478: Remove unnecessary
True if ... else False
Remove unnecessary
True if ... else False
(SIM210)
499-527
: Ensure robust YAML import error handling.
import_topology_yaml
properly raises 400 onYAMLError
. For more advanced usage, consider validating the structure or mandatory fields prior to callingimport_to_db
. This can proactively handle incomplete data.π§° Tools
πͺ Ruff (0.8.2)
505-507: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
506-506: Do not perform function call
IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
508-508: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
521-521: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
524-526: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
keep-ui/app/(keep)/topology/ui/map/topology-map.tsx (9)
27-33
: Heroicons and Tremor UI imports are appropriate.
Imports align well with the UI needs. Keep an eye on bundle size if more icons are added.
85-89
:MenuItem
interface is clear and concise.
Consider adding JSDoc comments if you foresee expansions or complex usage.
114-114
:mutate: mutateApplications
usage is straightforward.
No immediate concerns. Just verify if we need separate error handling for application mutations.
185-207
:menuItems
for import/export is a neat addition.
Might consider a loading indicator for longer export operations.
242-267
:onConnect
callback is well handled, but watch for partial failures.
If partial dependencies fail, consider offering a retry or tracking each failure individually.
275-308
:onReconnect
logic is solid but somewhat lengthy.
Consider breaking out error-handling steps into helper functions for clarity, especially reversion logic.
312-321
:getEdgeIdBySourceTarget
is a convenient utility.
No immediate red flags; just ensure caching if performance becomes a problem.
560-561
: AutocompleteonSelect
usage.
Ensure any asynchronous calls (if added later) handle loading states.
596-603
: Hidden file input usage.
Standard technique. Just be mindful of accessibility.keep/topologies/topologies_service.py (4)
156-167
:get_all_topology_data
calls the updatedget_topology_services
.
Integration is cohesive; ensure it handlesinclude_empty_deps
as intended.
431-440
:get_service_by_id
utility ensures data retrieval.
Consider raising an exception if the service is missing. Currently returningNone
might cause silent failures.
441-446
:get_dependency_by_id
fetch logic.
Similarly, might add error handling or throwDependencyNotFoundException
here if not found.
447-468
:create_service
withis_manual=True
.
Implementation is fine, but consider removingsession.close()
if you need to chain further operations in the same request.keep-ui/app/(keep)/topology/topology-client.tsx (1)
59-74
: Improve accessibility of the pull topology action.While the UI looks cleaner with the Icon component, consider adding an ARIA label for better accessibility.
<Icon icon={ArrowPathIcon} className="h-4 w-4 mr-2.5" onClick={handlePullTopology} tooltip="Pull latest topology" + aria-label="Pull latest topology" />
keep-ui/app/(keep)/topology/ui/map/service-node.tsx (1)
161-174
: Consider using a constant for the opacity value.The handle opacity control looks good, but consider extracting the opacity value to a constant for better maintainability.
+const MANUAL_HANDLE_OPACITY = '!opacity-100'; <Handle type="source" - className={clsx(data.is_manual === true && "!opacity-100")} + className={clsx(data.is_manual === true && MANUAL_HANDLE_OPACITY)} position={Position.Right} id="right" /> <Handle type="target" - className={clsx(data.is_manual === true && "!opacity-100")} + className={clsx(data.is_manual === true && MANUAL_HANDLE_OPACITY)} position={Position.Left} id="left" />tests/e2e_tests/test_topology.py (1)
14-15
: Consider using headless mode in CI/CD.Setting
PLAYWRIGHT_HEADLESS
tofalse
might be useful for local debugging but could cause issues in CI/CD environments.Consider making this configurable:
-os.environ["PLAYWRIGHT_HEADLESS"] = "false" +os.environ["PLAYWRIGHT_HEADLESS"] = os.getenv("PLAYWRIGHT_HEADLESS", "false")keep-ui/app/(keep)/topology/ui/map/AddEditNodeSidePanel.tsx (1)
105-107
: Enhance form validation.The current validation only checks for non-empty strings. Consider adding more comprehensive validation:
- Input sanitization
- Length limits
- Format validation for emails, URLs, etc.
const handleSaveValidation = () => { - return formData.display_name.length > 0 && formData.service.length > 0; + const isValidService = formData.service.length > 0 && formData.service.length <= 100; + const isValidDisplayName = formData.display_name.length > 0 && formData.display_name.length <= 100; + const isValidEmail = !formData.email || /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(formData.email); + return isValidService && isValidDisplayName && isValidEmail; };keep/api/models/db/topology.py (1)
44-44
: Document the purpose ofis_manual
field.The new
is_manual
field lacks documentation explaining its purpose and implications.Add docstrings to explain the field:
class TopologyService(SQLModel, table=True): + """ + Represents a service in the topology. + + Attributes: + is_manual: Indicates if the service was manually created rather than + automatically discovered. Defaults to False. + """ # ... other fields ... is_manual: Optional[bool] = FalseAlso applies to: 119-119
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (15)
keep-ui/app/(keep)/topology/model/models.ts
(2 hunks)keep-ui/app/(keep)/topology/model/useTopologyApplications.ts
(1 hunks)keep-ui/app/(keep)/topology/topology-client.tsx
(2 hunks)keep-ui/app/(keep)/topology/ui/map/AddEditNodeSidePanel.tsx
(1 hunks)keep-ui/app/(keep)/topology/ui/map/getNodesAndEdgesFromTopologyData.ts
(3 hunks)keep-ui/app/(keep)/topology/ui/map/manage-selection.tsx
(5 hunks)keep-ui/app/(keep)/topology/ui/map/service-node.tsx
(1 hunks)keep-ui/app/(keep)/topology/ui/map/topology-map.tsx
(13 hunks)keep-ui/components/SidePanel.tsx
(1 hunks)keep/api/models/db/migrations/versions/2025-01-26-15-25_8176d7153747.py
(1 hunks)keep/api/models/db/topology.py
(8 hunks)keep/api/routes/topology.py
(4 hunks)keep/topologies/topologies_service.py
(7 hunks)tests/e2e_tests/test_topology.py
(1 hunks)tests/test_enrichments.py
(1 hunks)
π§° Additional context used
πͺ Ruff (0.8.2)
tests/e2e_tests/test_topology.py
7-7: tests.e2e_tests.utils.trigger_alert
imported but unused
Remove unused import
(F401)
8-8: tests.e2e_tests.utils.assert_scope_text_count
imported but unused
Remove unused import
(F401)
9-9: tests.e2e_tests.utils.open_connected_provider
imported but unused
Remove unused import
(F401)
10-10: tests.e2e_tests.utils.delete_provider
imported but unused
Remove unused import
(F401)
11-11: tests.e2e_tests.utils.assert_connected_provider_count
imported but unused
Remove unused import
(F401)
keep/api/routes/topology.py
298-300: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
299-299: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
301-301: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
311-313: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
319-321: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
320-320: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
322-322: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
330-333: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
335-335: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
337-339: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
345-347: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
346-346: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
348-348: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
360-363: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
365-365: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
367-369: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
375-377: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
376-376: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
378-378: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
387-390: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
392-394: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
400-402: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
401-401: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
403-403: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
412-412: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
414-417: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
419-421: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
429-431: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
430-430: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
432-432: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
444-444: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
446-449: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
451-453: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
462-464: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
463-463: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
465-465: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
478-478: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
505-507: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
506-506: Do not perform function call IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
508-508: Do not perform function call Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
521-521: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
524-526: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
π Additional comments (52)
keep-ui/components/SidePanel.tsx (1)
4-10
: LGTM! Well-structured props interface.The props interface is well-defined with appropriate TypeScript types and optional styling parameters.
keep-ui/app/(keep)/topology/model/models.ts (3)
6-6
: Ensure consistent handling of string-based IDs and defaults foris_manual
.Changing IDs from numeric to string can create unexpected issues if other parts of the code still expect numeric values or perform numeric operations. Verify that all references to these IDs (e.g., sorting, filtering, or arithmetic) are updated accordingly. Also consider providing a default value for
is_manual
on creation if its initialization is unclear.Also applies to: 13-13, 32-32
41-41
: Double-check usage of the newServiceNodeType
.Refactoring to use
TopologyServiceWithMutator
withinServiceNodeType
is logical. Verify that all references in your node rendering logic handle the additionaltopologyMutator
property properly and that no older references toTopologyService
remain.
46-46
: Confirm minimal service references.Changing the
TopologyServiceMinimal
id
to a string must also align with the rest of the code that handles minimal servicesβmake sure to update or remove numeric-based assumptions.keep-ui/app/(keep)/topology/ui/map/manage-selection.tsx (8)
3-4
: All new imports look appropriate.
No immediate issues are apparent. These imports align with the new or updated interfaces and hooks.Also applies to: 16-19, 21-23, 25-26
28-36
: Validate prop usage and naming coherence.The new
ManageSelection
component props (topologyMutator
,getServiceById
) are well-defined. Verify that call sites pass the correct references and handle potential null returns fromgetServiceById
.
41-44
: Use more explicit selection type checks.Storing selected services as
TopologyServiceWithMutator[]
is logical. However, if any node lacks atopologyMutator
, ensure you handle it gracefully rather than expecting all nodes to be of this type.
51-62
: Handle potential concurrency withselectedServices
.The
handleServicesDelete
function uses the currentselectedServices
. IfselectedServices
can change between initial render and deletion, consider closure issues. Also add error handling for partial deletions if the API partially succeeds.
63-64
: Edge-based dependency editing logic looks correct.The
isDependencyEditable
flag is a neat approach. Just verify your assumptions aboutselectedEdges[0]
are safeβe.g., thatselectedEdges
is never empty whenever this effect runs with length = 1.Also applies to: 66-78
166-182
: Validate user input foreditEdgeProtocol
.Relying on
prompt
to gather protocol might allow invalid or empty strings. Add minimal validation or fallback logic to avoid partial updates or empty protocol settings.
214-291
: Good approach to combining application creation and service management.The form logic and conditionals (e.g., only show βDelete Serviceβ if all selected services are manual) is clear. Just ensure you have test coverage around these conditionsβespecially partial manual selections or changes after data revalidation.
312-316
: Return conditional checks.Returning
null
when no services/application/edges are selected is standard, but clarify in the code or docstring that selection must exist for the UI to be rendered.Also applies to: 329-331
keep/api/routes/topology.py (1)
372-394
: Dependency endpoints appear logically consistent.These create/update/delete routes for dependencies all follow a similar pattern of validating manual vs pulled services. The approach is consistent. As with services, consider re-raising from the underlying exceptions to preserve stack traces.
Also applies to: 397-422, 424-454
π§° Tools
πͺ Ruff (0.8.2)
375-377: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
376-376: Do not perform function call
IdentityManagerFactory.get_auth_verifier
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
378-378: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
387-390: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
392-394: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
keep-ui/app/(keep)/topology/ui/map/topology-map.tsx (19)
3-3
: ImportingElementType
appears fine.
No issues found here.
23-24
: Imports foraddEdge
andreconnectEdge
look good.
These utility methods from React Flow assist with dynamic connections.
62-67
: Additional imports for topology functionalities.
The new importsβsuch asEdgeBase
,Connection
,toast
, anduseApi
βappear consistent with the changes below.
103-108
: New usage ofuseTopology
hook.
The structure for handlingtopologyData
,isLoading
, etc., is standard. Ensure errors are surfaced clearly.
133-134
: Side panel state management is correct.
Toggling a side panel with a dedicated boolean state is a good approach.
160-183
: File upload process looks functional but consider security measures.
When accepting user-provided files:
- Validate file size limits.
- Verify that server-side checks are in place (e.g., scanning, MIME checks).
232-239
:getServiceById
function is straightforward.
Ensure upstream handles the null case if no service is found.
271-274
:onReconnectStart
sets a flag correctly.
No issues found.
323-349
:onReconnectEnd
finalizes reconnection or deletes edges if undone.
Implementation is thorough; consider re-checking concurrency if multiple reconnection attempts happen quickly.
550-557
: Overall layout container adjustments.
This redesign gives more space and structure. Looks good.
562-575
:MultiSelect
for applications is a clean approach.
Matches the rest of the UI. The data-driven approach is clear.
579-594
: DropdownMenu for import/export.
Implementation is consistent with theMenuItem
pattern. Looks good.
604-616
: Conditionally rendering βFull topology mapβ link.
Logic is appropriate. No concerns.
617-646
: ReactFlow layout plusManageSelection
.
Looks consistent. Good job adding relevant node/edge callbacks.
647-656
:<Background>
and<Controls>
usage.
No issues. Improves user navigation.
657-664
: Empty state overlay logic.
Helpful for first-time users. Good clarity.
666-666
: Fragment closure.
No changes required.
669-670
: Container closure.
Matches intended structure.
671-677
:AddEditNodeSidePanel
usage.
Implementation is consistent. The newly introduced side panel integration is neat.keep/topologies/topologies_service.py (14)
7-7
: Added imports forand_
,or_
, andexists
.
This seems necessary for refined filtering and existence checks.
20-25
: Importing additional DTOs aligns with new CRUD endpoint usage.
Consistent with the new manual flags and dependency features.
51-53
:ServiceNotManualException
introduced.
Meaningful name; it clarifies the domain constraint.
55-57
:DependencyNotFoundException
introduced.
Throwable for referencing missing dependencies. Looks good.
102-113
:validate_non_manual_exists
function.
Essential to prevent changes on non-manual services. Check performance if used in loops with large sets.
117-124
:get_topology_services
replaces old logic.
Return type nowlist[TopologyService]
, ensuring direct usage. Good improvement.
469-490
:create_services
for multiple additions.
Transaction usage is correct. Just ensure large-scale insert performance is tested if many services are created.
492-523
:update_service
checksis_manual
before updates.
Logic is valid. Consider returning a status or partial update info if needed.
525-571
:delete_services
with non-manual assertion.
Robust approach, but verify potential large list performance. A single transaction is used, which is good.
573-599
:create_dependency
respectsenforce_manual
.
Helpful domain constraint. Implementation is consistent.
601-636
:create_dependencies
for bulk creation.
Similar performance concern if the list is very large. The approach is correct.
638-676
:update_dependency
with manual checks.
Logic parallelsupdate_service
. Good consistency.
678-707
:delete_dependency
also ensures manual services.
Matches the existing pattern. Overall consistent.
709-746
:import_to_db
orchestrates creating services, apps, dependencies together.
Good use of transactions. Make sure partial imports rollback correctly.keep/api/models/db/migrations/versions/2025-01-26-15-25_8176d7153747.py (1)
1-30
: Migration addingis_manual
column.
Straightforward approach. Make sure to test upgrade/downgrade in staging.keep-ui/app/(keep)/topology/ui/map/getNodesAndEdgesFromTopologyData.ts (2)
33-40
: LGTM! Node data enrichment looks good.The node data is properly enriched with:
- Incidents count for monitoring
- Topology mutator for state management
58-69
:β Verification inconclusive
Verify edge ID uniqueness.
The edge creation logic now uses string IDs. Ensure that
dependency.id
is unique across all dependencies to prevent edge ID collisions.
π Script executed:
#!/bin/bash # Search for dependency ID assignments ast-grep --pattern 'dependency.id = $_'Length of output: 41
Action Required: Confirm
dependency.id
UniquenessThe code in
keep-ui/app/(keep)/topology/ui/map/getNodesAndEdgesFromTopologyData.ts
(lines 58β69) usesdependency.id.toString()
as the edge ID. Our automated search did not reveal any explicit assignment (or re-assignment) todependency.id
, so we couldnβt verify programmatically that these IDs are globally unique. Please manually confirm that the source data guarantees uniqueness across all dependencies to prevent edge ID collisions.
- File:
keep-ui/app/(keep)/topology/ui/map/getNodesAndEdgesFromTopologyData.ts
- Location: Lines 58β69 (snippet provided in the review)
- Next Steps: Verify via the data model or upstream logic that each
dependency.id
is unique across the entire topology data.keep-ui/app/(keep)/topology/topology-client.tsx (1)
3-10
: LGTM! Clean import organization.The imports are well-organized, grouping related Tremor components together.
keep-ui/app/(keep)/topology/model/useTopologyApplications.ts (1)
163-163
: LGTM! Exposing mutate function.Exposing the
mutate
function allows components to manually update the cache, which is essential for the new topology mutation features.keep/api/models/db/topology.py (1)
130-132
:β Verification inconclusive
Ensure consistent ID type conversion.
The conversion of IDs from int to str should be handled consistently across all DTOs.
Run this script to verify consistent ID handling:
Also applies to: 201-202
π Script executed:
#!/bin/bash # Description: Check for consistent ID type handling across DTOs # Search for ID field declarations rg -A 1 'id: .*(?:int|str)' --type py # Search for ID conversions in from_orm methods rg -A 5 'from_orm.*\bid=.*(?:int|str)' --type pyLength of output: 67220
Action: Verify Consistent ID Type Conversion Across DTOs
It appears that while the topology DTO now defines the ID as an optional string (as seen in lines 130β132 of keep/api/models/db/topology.py), our codebase still shows several instances where IDs are declared as integers. This discrepancy could lead to issues if ID conversions (from int to str) are not uniformly applied across all DTOs.
- In keep/api/models/db/topology.py the snippet:
contrasts with other parts of the repository where IDs remain as ints.id: Optional[str] = None serviceId: str serviceName: str
- No explicit conversion logic (e.g., within
from_orm
methods) for IDs was found in our search, indicating that the conversion approach might be inconsistent.- The same concern applies to the lines referenced at 201β202, which likely follow a similar pattern.
Please verify that the conversion of IDs from int to str is handled consistently across all DTOsβeither by enforcing a uniform type in model definitions or by ensuring that any necessary conversion is correctly applied (for example, within
from_orm
methods). If this discrepancy is intentional, consider adding documentation to clarify the conversion strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Closes #2904
Closes #3328
π Description
β Checks
βΉ Additional Information