Merged
Conversation
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 4a1b9fa in 2 minutes and 1 seconds
More details
- Looked at
225lines of code in11files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:429
- Draft comment:
Avoid using inline styles. The Drawer’s closeIcon prop uses an inline style (marginTop from Spacing), which can be moved to a CSS class or styled component for better consistency and easier maintenance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:200
- Draft comment:
There is a pattern of logging events (e.g. in handleTabChange, handleTimeChange, etc.) that is repeated across several components. Consider abstracting these logEvent calls into a custom hook or helper function to reduce duplication and improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/container/InfraMonitoringK8s/StatefulSetDetails/StatefulSetDetails.tsx:75
- Draft comment:
Inline style objects (for example, the style prop on the Drawer) are used directly. It’s preferable to use CSS classes or styled components for these styles to ensure consistency with design tokens and easier debugging. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/container/InfraMonitoringK8s/StatefulSetDetails/StatefulSetDetails.tsx:181
- Draft comment:
The logic for updating filter states (e.g. in handleChangeLogFilters and handleChangeTracesFilters) is repeated across similar components. Consider abstracting this logic into a reusable utility or custom hook to minimize duplication. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/container/InfraMonitoringK8s/Namespaces/NamespaceDetails/NamespaceDetails.tsx:341
- Draft comment:
The Drawer component uses inline styles for background and overscrollBehavior. To maintain design consistency and theming, consider moving these styles to a dedicated CSS class. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/src/container/InfraMonitoringK8s/Jobs/JobDetails/JobDetails.tsx:203
- Draft comment:
The useCallback hook for handleTimeChange is using an empty dependency array (with eslint disabled). Please verify that all necessary dependencies are included to avoid potential stale closures in future updates. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:181
- Draft comment:
There is a lot of duplicated logic for filtering and event logging across Kubernetes entity detail components. Consider refactoring the common filtering and logging logic into reusable functions to reduce duplication and improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. frontend/src/container/InfraMonitoringK8s/DaemonSets/DaemonSetDetails/DaemonSetDetails.tsx:107
- Draft comment:
Typo in the 'id' field of the namespace filter key: it uses 'k8s_daemonSet_name--string--resource--false' instead of something like 'k8s_namespace_name--string--resource--false'. Please update it for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. frontend/src/container/InfraMonitoringK8s/Deployments/DeploymentDetails/DeploymentDetails.tsx:109
- Draft comment:
Typographical error: In the initialFilters, the id for the namespace filter (using QUERY_KEYS.K8S_NAMESPACE_NAME) is incorrectly set to 'k8s_deployment_name--string--resource--false'. It should be updated to something like 'k8s_namespace_name--string--resource--false' to avoid confusion. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. frontend/src/container/InfraMonitoringK8s/Jobs/JobDetails/JobDetails.tsx:111
- Draft comment:
Typographical Error: In the second filter definition within 'initialFilters', the 'id' field for the key corresponding to QUERY_KEYS.K8S_NAMESPACE_NAME is incorrectly set to 'k8s_job_name--string--resource--false'. It should be changed to 'k8s_namespace_name--string--resource--false' for clarity and correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. frontend/src/container/InfraMonitoringK8s/Pods/PodDetails/PodDetails.tsx:116
- Draft comment:
Typographical Error: In the namespace filter object, theidproperty is set to 'k8s_pod_name--string--resource--false', which is identical to the pod name filter. It seems like a copy-paste error. Consider changing it to 'k8s_namespace_name--string--resource--false' for clarity and correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. frontend/src/container/InfraMonitoringK8s/Pods/PodDetails/PodDetails.tsx:622
- Draft comment:
Typographical Error: The prop passed to the PodEvents component is namedhandleChangeEventFilters, but the corresponding handler is defined ashandleChangeEventsFilters(note the extra 's'). For consistency and clarity, please align the naming. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:461
- Draft comment:
Consider updating the label 'Statefulset Name' to 'StatefulSet Name' for consistency with naming conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_l4Ut1avWjVhXRgSZ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx
Outdated
Show resolved
Hide resolved
amlannandy
previously approved these changes
Mar 24, 2025
frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityTraces/EntityTraces.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3bc1008 in 1 minute and 40 seconds
More details
- Looked at
24lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityTraces/EntityTraces.tsx:213
- Draft comment:
Removed inline style for pointer cursor. Ensure a CSS class is applied instead for visual feedback on clickable rows. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:202
- Draft comment:
Suggestion: Remove inline style is good practice. Ensure the clickable row still shows a pointer cursor by applying a CSS class in HostMetricTraces.styles.scss. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityTraces/EntityTraces.tsx:212
- Draft comment:
Suggestion: Instead of inline style, use a CSS class (defined in entityTraces.styles.scss) to set the pointer cursor for clickable rows. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_3wSqguNiWOosPMe7
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx
Show resolved
Hide resolved
amlannandy
approved these changes
Mar 24, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix https://github.com/SigNoz/engineering-pod/issues/2243
Important
Add logging for span row clicks and tab changes across various components to track user interactions.
logEventfor row click inHostMetricTraces.tsxandEntityTraces.tsxto logItemClickedevent.logEventfor tab change inHostMetricsDetails.tsxandClusterDetails.tsxto logTabChangedevent.categoryprop toClusterDetails.tsx,DaemonSetDetails.tsx, andDeploymentDetails.tsxfor logging purposes.categoryprop toJobDetails.tsx,NamespaceDetails.tsx, andNodeDetails.tsxfor logging purposes.categoryprop toPodDetails.tsxandStatefulSetDetails.tsxfor logging purposes.This description was created by
for 3bc1008. It will automatically update as commits are pushed.