-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: api monitoring domain details #7308
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
Conversation
|
Sahil seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Generated with ❤️ by ellipsis.dev |
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.
👍 Looks good to me! Reviewed everything up to a13d1c8 in 3 minutes and 39 seconds
More details
- Looked at
4761lines of code in32files - Skipped
1files when reviewing. - Skipped posting
20drafted comments based on config settings.
1. frontend/src/AppLayout/index.tsx:45
- Draft comment:
Avoid inline styles in the Drawer component. Instead of using an inline style for background and overscrollBehavior (line 46-47), use a CSS class or styled component to ensure design token 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.
2. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx:162
- Draft comment:
Avoid inline style in the Select component. Refactor the style={{ width: '100%' }} into a CSS class or use a design token. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/EndPointsDropDown.tsx:28
- Draft comment:
Avoid inline style on the Select component. Instead of style={{ width: 120 }}, use a CSS class or design token for consistency. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:912
- Draft comment:
The component passes inline style (via selectStyle imported) to the Select component. Consider moving any static style definitions into a stylesheet or a styled component to adhere to design consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:21
- Draft comment:
Avoid calling setCurrentRenderCount inside useMemo; use a separate useEffect for state updates to keep the memo pure. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/EndPointMetrics.tsx:80
- Draft comment:
Fix the error rate calculation by wrapping the nullish fallback in parentheses: use ((metricsData?.errorRate ?? 0) * 100). - 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/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.tsx:18
- Draft comment:
Avoid 'any' type for the domainData prop; define a proper interface for type safety. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/package.json:7
- Draft comment:
Typographical consistency: The script key is 'i18n:generate-hash', which suggests a file name should be 'i18n-generate-hash.js', but the command calls './i18-generate-hash.js'. Please confirm if this is intentional or update to maintain consistent 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.
9. frontend/src/AppRoutes/pageComponents.ts:98
- Draft comment:
Typographical error: The exported constant 'ListAllALertsPage' appears to have a typo. Consider renaming it to 'ListAllAlertsPage' 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.
10. frontend/src/AppRoutes/routes.ts:25
- Draft comment:
It appears that 'ListAllALertsPage' might be a typographical error. Consider renaming it to 'ListAllAlertsPage' to maintain consistency and clarity. - 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/components/QuickFilters/types.ts:15
- Draft comment:
Typographical error: The enum name 'SpecficFilterOperations' appears to be misspelled. It would be clearer to rename it to 'SpecificFilterOperations' to correct the error. - 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/constants/routes.ts:42
- Draft comment:
Typo: Consider renaming 'UN_AUTHORIZED' to 'UNAUTHORIZED' for consistency and clarity. - 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/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:810
- Draft comment:
It appears that the CSS property 'text-wrap' is used in the '.raw-log-content' class (line 810). 'text-wrap' is not a standard CSS property; consider using 'word-wrap' or 'overflow-wrap' for better browser support. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment is technically correct thattext-wrapis not a standard CSS property. However, the code already uses the recommendedword-wrap: break-wordproperty. Thetext-wrap: inheritline appears redundant but not harmful. The comment doesn't add much value since the proper property is already being used.
The comment correctly identifies a non-standard CSS property, but fails to notice that the recommended solution is already implemented.
While technically accurate abouttext-wrap, the comment is not useful since the code already uses the properword-wrapproperty.
Delete the comment since it points out an issue that is already properly handled withword-wrap: break-word.
14. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:989
- Draft comment:
There is an inconsistency in the naming convention: the class '.end-point-details-zero-state-wrapper' (line 989) uses a hyphen between 'end' and 'point', whereas similar classes in this file (e.g., '.endpoint-details-container' and '.endpoint-details-card') do not. Consider renaming for consistency. - Reason this comment was not posted:
Marked as duplicate.
15. frontend/src/container/ApiMonitoring/utils.tsx:70
- Draft comment:
Typo: The variable name 'minutedDiff' (line 70) should be renamed to 'minutesDiff' for clarity. This is a minor typographical error. - Reason this comment was not posted:
Marked as duplicate.
16. frontend/src/container/ApiMonitoring/utils.tsx:600
- Draft comment:
Typo in comment: 'endpoint dependant services' (line 600) should be changed to 'endpoint dependent services' for consistency with common spelling. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment is about a spelling mistake in a code comment. While technically correct, the review rules state that comments should focus on actual code changes that require action. Spelling fixes in comments are not critical to code functionality. Additionally, since this is a new file, the author can choose their preferred spelling during initial creation.
The spelling correction would improve consistency and professionalism in the codebase. Documentation quality is important for maintainability.
While documentation quality matters, the review rules explicitly state to focus on actionable code changes and not make purely informative comments. This is a minor spelling issue that doesn't impact functionality.
The comment should be deleted as it violates the rule about making purely informative comments that don't require code changes.
17. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:388
- Draft comment:
Typo: In the comment 'this is for adding subsequent comma seperated values', 'seperated' should be corrected to 'separated'. - 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.
18. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:807
- Draft comment:
Typo: In the comment 'keep the tags in sycn with current query', 'sycn' should be corrected to 'sync'. - 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.
19. frontend/src/container/SideNav/menuItems.tsx:92
- Draft comment:
Typo in comment: change 'read for release' to 'ready for release'. - 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.
20. frontend/src/container/TopNav/DateTimeSelectionV2/config.ts:97
- Draft comment:
Typographical error: In the RelativeDurationOptions array (line 97), the label 'Last 6 hour' should be corrected to 'Last 6 hours' for proper pluralization. - 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_mxIZ2APVB9v4m30K
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
❌ Changes requested. Incremental review on 73c4890 in 2 minutes and 12 seconds
More details
- Looked at
282lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/EndPointDetailsZeroState.tsx:27
- Draft comment:
The component is well-structured. Ensure that any further styling aligns with the external stylesheet instead of inline styles. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
2. frontend/src/assets/CustomIcons/EndPointsZeroStateIcon.tsx:16
- Draft comment:
Avoid hardcoding hex color values in SVG paths. Use design tokens or predefined color constants to maintain design consistency. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:1010
- Draft comment:
In the .end-point-details-zero-state-content-wrapper block, the property 'justify-content: left;' appears to be a typographical error. 'left' is not a valid value for justify-content. Consider changing it to 'flex-start' (or another valid flex alignment value) to ensure proper layout. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_lWjJO0wM17cb2aoQ
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/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss
Outdated
Show resolved
Hide resolved
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.
👍 Looks good to me! Incremental review on 51bad0d in 1 minute and 32 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/ExpandedRow.tsx:35
- Draft comment:
Passing an empty array as a second parameter to getEndPointsColumnsConfig may have unintended side effects if the columns config depends on external values. Double-check if this parameter should be dynamic, and update the dependency array accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment suggests double-checking if the parameter should be dynamic, which violates the rule against asking the PR author to confirm their intention or to double-check things. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
2. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/ExpandedRow.tsx:35
- Draft comment:
Ensure that the new second parameter (empty array) passed to getEndPointsColumnsConfig is intentional. If this parameter is expected to be dynamic or have significance later, consider adding it to the useMemo dependency array or documenting its purpose. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment violates several rules: 1) It asks to "ensure" something is intentional, which we explicitly forbid 2) It's speculative about future changes 3) The empty array is clearly hardcoded and not dynamic, so there's no need for it to be in useMemo dependencies 4) The comment doesn't point out a clear issue that needs fixing.
Maybe the empty array parameter is important for the function's behavior and we should understand its purpose better? Maybe there's a bug here we're missing?
Even if the parameter is important, the comment doesn't help - it just asks for verification without identifying any specific issue. If there was a bug, the comment should state it directly.
Delete this comment as it violates our rules by asking for verification without pointing out a specific issue, and makes speculative suggestions about future changes.
Workflow ID: wflow_ziwP1l4GAITdaIzu
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ahmadshaheer
left a comment
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.
added a couple of comments. can you please check them?
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/ExpandedRow.tsx
Outdated
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.tsx
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.tsx
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/EndPointDetails.tsx
Outdated
Show resolved
Hide resolved
...nd/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/EndPointMetrics.tsx
Show resolved
Hide resolved
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.
❌ Changes requested. Incremental review on e4c9e2a in 1 minute and 42 seconds
More details
- Looked at
346lines of code in7files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:35
- Draft comment:
Avoid calling setState (setCurrentRenderCount) inside useMemo. Move this state update to a useEffect to prevent side effects in memoized computations. - 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/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:35
- Draft comment:
Avoid side effects (state updates) inside useMemo. Update 'currentRenderCount' in a useEffect instead. - 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/ApiMonitoring/Explorer/Domains/DomainDetails/EndPointDetails.tsx:114
- Draft comment:
Remove the debugging console.log in the onChange callback. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:79
- Draft comment:
Avoid using inline styles for dynamic widths. Consider using a CSS class or styled-component with design tokens. - Reason this comment was not posted:
Comment was on unchanged code.
5. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:221
- Draft comment:
Typo alert: The CSS variable is written as '--Vanilla-400' here, while elsewhere similar variables use lowercase (e.g., '--vanilla-400' or '--text-vanilla-400'). Please check if this is a typo and 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.
6. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:759
- Draft comment:
Typo alert: The class names '.end-point-details-zero-state-wrapper', '.end-point-details-zero-state-content', etc., use a hyphen between 'end' and 'point'. This is inconsistent with similar class names used elsewhere (e.g., '.endpoint-details-card', '.endpoint-details-container'). Please review and standardize the naming convention. - 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/ApiMonitoring/utils.tsx:73
- Draft comment:
Typo: Consider renaming the variable 'minutedDiff' to 'minutesDiff' for clarity. - 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_Nwaoh4HaHVycTYs7
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/container/ApiMonitoring/Explorer/Domains/DomainDetails/EndPointDetails.tsx
Outdated
Show resolved
Hide resolved
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.
👍 Looks good to me! Incremental review on 9bb7167 in 2 minutes and 7 seconds
More details
- Looked at
88lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/EndPointDetails.tsx:51
- Draft comment:
Good useMemo refactoring to isolate endpoint filters from global query effects. Consider adding a null/undefined check for 'currentQuery' (and its 'builder') to avoid potential runtime errors if the constant ever becomes undefined. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Since currentQuery comes from a constant import initialQueriesMap, it should be guaranteed to exist and have the expected shape. The code is not fetching this data or getting it from an uncertain source. Adding null checks would just add unnecessary defensive programming. The comment seems overly cautious without strong evidence that these checks are needed.
I could be wrong about the initialQueriesMap constant - perhaps it could be modified at runtime or have undefined values. Also, defensive programming is sometimes good practice even with seemingly guaranteed values.
While defensive programming has its place, adding null checks for values from constant imports would be excessive and make the code more verbose without clear benefit. If initialQueriesMap needs runtime validation, that should be handled where it's defined.
The comment should be deleted as it suggests adding unnecessary null checks for values coming from constant imports without strong evidence that such checks are needed.
2. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/MetricOverTimeGraph.tsx:86
- Draft comment:
The refactoring moves the 'graph-container' div (with ref) to wrap renderCardContent outside. Confirm that this change aligns with the intended ResizeObserver monitoring, and that no unintended CSS/layout issues occur. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment asks the PR author to confirm their intention and check for unintended issues, which violates the rules. It doesn't provide a specific code suggestion or point out a clear issue.
3. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/EndPointDetails.tsx:51
- Draft comment:
Ensure that 'currentQuery' is defined before destructuring its builder. A null/undefined check could prevent runtime errors if the constant changes in the future. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/MetricOverTimeGraph.tsx:86
- Draft comment:
Moving the ref to the 'graph-container' div within the Card changes which element useResizeObserver observes. Confirm that this container is the correct one for dimension measurements. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is asking for confirmation rather than pointing out a clear issue. It's speculative - "confirm that this is correct" rather than "this is wrong because X". The code appears to be measuring dimensions for the graph container, and both the old and new locations would provide valid measurements. There's no clear evidence that this change is problematic.
The ref movement could potentially affect the graph's sizing behavior in ways that aren't immediately apparent from the code. The useResizeObserver hook's behavior might be important for the graph's responsiveness.
While the sizing behavior could be affected, the comment doesn't provide evidence of an actual problem. It's merely asking for confirmation, which violates our rules about not asking authors to verify their intentions.
The comment should be deleted because it's asking for confirmation rather than pointing out a clear issue, and there's no strong evidence that the ref movement is problematic.
Workflow ID: wflow_RQs6pRRoPSxpy8MJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
❌ Changes requested. Incremental review on 1aa1ae8 in 2 minutes and 41 seconds
More details
- Looked at
314lines of code in6files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx:162
- Draft comment:
Good use of optional chaining to prevent runtime errors when accessing nested properties. Verify that the API always returns an array at result[0] before using index access. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:117
- Draft comment:
Added padding-right for .error-rate seems fine; ensure this aligns with design tokens for consistent spacing. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/EndPointDetails.tsx:157
- Draft comment:
Passing down endPointName prop to MetricOverTimeGraph components is clear. Ensure the new prop is type‐checked and its usage consistent in child components. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/MetricOverTimeGraph.tsx:42
- Draft comment:
Extracting the endpoint from endPointName using extractPortAndEndpoint is a neat addition. Consider removing the console.log (line 1924) once debugging is complete to avoid accidental exposure in production. - Reason this comment was not posted:
Comment was on unchanged code.
5. frontend/src/container/ApiMonitoring/utils.tsx:493
- Draft comment:
In getEndPointsColumnsConfig, using extractPortAndEndpoint to obtain the endpoint value looks correct. Ensure that the hardcoded color (Color.BG_SLATE_100) is consistent with design tokens and not overridden in style classes. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
6. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx:188
- Draft comment:
Avoid using inline styles (e.g. style={{ width: '100%' }}) in the Select component; instead, define a CSS class in your stylesheet to maintain consistency and easier theming. - 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/ApiMonitoring/utils.tsx:1924
- Draft comment:
Remove the console.log from getFormattedChartData. Debug logging like 'console.log("uncaught result", result)' should be removed in production to avoid unnecessary log clutter. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/container/ApiMonitoring/utils.tsx:98
- Draft comment:
Consider renaming the exported variables (e.g. 'columnsConfig' and 'formatDataForTable') to more descriptive names. The inline comments suggest a future refactor; updating these names will improve code clarity. - 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/ApiMonitoring/utils.tsx:188
- Draft comment:
The utils.tsx file is quite large and encompasses various unrelated functions. Consider refactoring it into smaller, more focused modules to enhance maintainability and readability. - 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/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:222
- Draft comment:
It appears that the CSS variable is used with a capital 'V' (e.g.,var(--Vanilla-400, #c0c1c3)) in the .group-by-label block, which is inconsistent with the rest of the file that uses lowercase (e.g.,var(--text-vanilla-400)). Please review and ensure consistent casing for CSS variable names for clarity. - 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/ApiMonitoring/Explorer/Domains/DomainDetails/components/MetricOverTimeGraph.tsx:27
- Draft comment:
Consider renaming the parameter 'endPointName' to 'endpointName' for better consistency with typical naming conventions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the suggestion would improve naming consistency, this is a relatively minor stylistic change. The code works perfectly fine either way. The comment doesn't point out a bug or significant code quality issue. The rules state we should not make purely informative comments or obvious/unimportant suggestions.
The inconsistent casing could potentially cause confusion for other developers. It's also a new parameter being added, so now would be the best time to fix the naming.
While the timing is good, this is still a very minor stylistic issue that doesn't impact functionality or maintainability in any significant way. The rules emphasize focusing on more substantial issues.
Delete the comment as it suggests a minor stylistic change that doesn't meaningfully impact code quality or functionality.
12. frontend/src/container/ApiMonitoring/utils.tsx:74
- Draft comment:
Typographical note: The variable name 'minutedDiff' is a bit awkward. It might be clearer to rename it to 'minutesDiff' to better indicate that it represents the difference in minutes. - 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/ApiMonitoring/utils.tsx:654
- Draft comment:
Typographical note: In the comment for the sixth query payload, 'endpoint dependant services' is used. It would be more consistent and clear to use 'dependent' (as in the function names) instead of 'dependant'. - 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_g0LqrR6yvlWxJCOD
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.
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.
👍 Looks good to me! Incremental review on 1adc967 in 1 minute and 55 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/utils.tsx:1921
- Draft comment:
Remove debugging log before production. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/container/ApiMonitoring/utils.tsx:98
- Draft comment:
Placeholder naming: Several exports (e.g. 'columnsConfig', 'hardcodedAttributeKeys', 'formatDataForTable') are preceded by a note '// Rename this to a proper name'. Please rename them to more descriptive names before finalizing. - 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/ApiMonitoring/utils.tsx:91
- Draft comment:
Typo in variable name: 'minutedDiff' should likely be 'minutesDiff' for clarity. - 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/ApiMonitoring/utils.tsx:1910
- Draft comment:
Potential division by zero: In getFormattedDependentServicesData, 'totalCount' may be 0, causing division errors when computing percentages. - 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/ApiMonitoring/utils.tsx:278
- Draft comment:
Consider refactoring the duplicated, deeply nested query payload objects (in getEndPointsQueryPayload and getEndPointDetailsQueryPayload) to reduce maintenance overhead. - 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/ApiMonitoring/utils.tsx:423
- Draft comment:
Static query ID reused: The same hard-coded query ID ('315b15fa-ff0c-442f-89f8-2bf4fb1af2f2') appears in multiple payloads. Consider extracting it to a constant or generating it dynamically if needed. - 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/ApiMonitoring/utils.tsx:1922
- Draft comment:
Performance note: Heavy use of cloneDeep (e.g. in getFormattedChartData and groupStatusCodes) may impact performance on large objects. Verify if a shallow copy would suffice. - Reason this comment was not posted:
Comment was on unchanged code.
8. frontend/src/container/ApiMonitoring/utils.tsx:495
- Draft comment:
Inline render functions: Inline arrow functions in table column render methods (e.g. for displaying icons) are re-created on each render. Consider memoizing these callbacks if performance becomes an issue. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. frontend/src/container/ApiMonitoring/utils.tsx:214
- Draft comment:
Repeated literal strings: Keys like 'net.peer.name' and 'http.url' are used repeatedly. Consider extracting them as constants to reduce typos and ease future refactoring. - 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/ApiMonitoring/utils.tsx:91
- Draft comment:
In the function getLastUsedRelativeTime, the variable 'minutedDiff' appears to be a typo. Consider renaming it to 'minutesDiff' for clarity. - 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_TyBVoHxIWsJjoW19
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
👍 Looks good to me! Incremental review on eee9777 in 2 minutes and 59 seconds
More details
- Looked at
27lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/utils.tsx:575
- Draft comment:
Using a type assertion (as string) here can be risky if the value is not actually a string (it might be a number from the response). Consider using a safe conversion (e.g., String(endpoint.data['http.url'] || '')) to avoid potential runtime issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The suggestion has merit since type assertions can be unsafe if the value is not actually of the asserted type. Using String() would be safer as it handles non-string values gracefully. However, looking at the interface definition and usage, we can see that http.url would always be a string in practice since it represents a URL. The type system is just being overly broad in the interface definition. The type assertion here is actually documenting the developer's knowledge about the data shape.
The comment identifies a real type safety issue, but may be overly cautious given the domain knowledge that http.url will always be a string. The suggested fix could make the code more verbose without adding real safety.
While type safety is important, in this case the type assertion is actually appropriate documentation of the expected data shape. Using String() would obscure the fact that we expect this to always be a string.
The comment should be deleted. While it makes a valid point about type safety in general, in this specific case the type assertion is appropriate documentation and the suggested change would not meaningfully improve safety.
2. frontend/src/container/ApiMonitoring/utils.tsx:579
- Draft comment:
Similarly, for setting endpointName, using a safe conversion may be preferable over a simple type assertion to mitigate possible type mismatches. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The type assertion is actually fine here because we already know from the interface that endpoint.data['http.url'] is of type string | number | undefined, the || '' fallback handles the undefined case safely, and type assertions are a valid TypeScript pattern when you know more about the type than the compiler. String() conversion wouldn't add any real safety benefits here.
Could there be edge cases where the value is a number and the type assertion causes issues? Should we be more defensive in our type handling?
Even if the value was a number, both the type assertion and String() would handle it similarly - the || '' fallback ensures we always get a string either way. The current code is already handling edge cases appropriately.
The comment should be deleted. The current type assertion approach is valid and safe, and the suggested change would not provide any meaningful benefits.
3. frontend/src/container/ApiMonitoring/utils.tsx:581
- Draft comment:
Using the || operator for callCount will replace valid 0 values with '-', consider using the nullish coalescing operator (??) to check for null/undefined. - Reason this comment was not posted:
Comment was on unchanged code.
4. frontend/src/container/ApiMonitoring/utils.tsx:98
- Draft comment:
Remove placeholder comments like 'Rename this to a proper name' and update export names to be more descriptive. - 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/ApiMonitoring/utils.tsx:1915
- Draft comment:
Guard against division by zero in percentage calculation in getFormattedDependentServicesData. - 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/ApiMonitoring/utils.tsx:1923
- Draft comment:
Consider refactoring this large file into smaller modules grouped by functionality for better 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.
7. frontend/src/container/ApiMonitoring/utils.tsx:74
- Draft comment:
Typo: Consider renaming 'minutedDiff' to 'minutesDiff' for clarity. - 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_6eYVsbX0VxI8mBnq
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ahmadshaheer
left a comment
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! thanks for making the changes 👍
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx
Show resolved
Hide resolved
.../src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx
Outdated
Show resolved
Hide resolved
.../src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx
Show resolved
Hide resolved
.../src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx
Show resolved
Hide resolved
frontend/src/container/ApiMonitoring/Explorer/Explorer.styles.scss
Outdated
Show resolved
Hide resolved
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.
👍 Looks good to me! Incremental review on 5cba885 in 1 minute and 14 seconds
More details
- Looked at
88lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:32
- Draft comment:
Avoid calling setState (setCurrentRenderCount) inside useMemo. This side-effect should be relocated to a useEffect to prevent unexpected re-renders. - 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/ApiMonitoring/Explorer/Domains/DomainDetails/AllEndPoints.tsx:187
- Draft comment:
Good job removing the inline style from the Select component; ensure the width is consistently applied via CSS classes (as in the SCSS). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:32
- Draft comment:
Avoid performing side effects inside useMemo. Move the call to setCurrentRenderCount (lines 28–34) into a useEffect to prevent unintended re-renders. - 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/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:76
- Draft comment:
Avoid using inline styles for dynamic properties. Consider using CSS classes or CSS variables (perhaps via a styled component) to apply the dynamic width for the progress bar. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/DependentServices.tsx:87
- Draft comment:
For better accessibility, consider using a semantic element instead of a clickablefor the 'Show more' trigger. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/DomainDetails.styles.scss:222
- Draft comment:
Consider changing 'var(--Vanilla-400, #c0c1c3)' to 'var(--vanilla-400, #c0c1c3)' for consistency in variable 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.
Workflow ID: wflow_LIaTENKQfZwRNeFZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
❌ Changes requested. Incremental review on 4b00ec1 in 1 minute and 34 seconds
More details
- Looked at
318lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/EndPointDetailsZeroState.tsx:16
- Draft comment:
The changes look clean. Consider ensuring that the SVG asset /Icons/no-data.svg is managed consistently (e.g. imported as a module or using a design token) to maintain design consistency across the project. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/EndPointDetailsZeroState.tsx:16
- Draft comment:
Consider ensuring that the static image path ('/Icons/no-data.svg') is maintained consistently with design assets. It might be beneficial to use a central asset constant or design token for asset paths to avoid hardcoded strings. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. frontend/src/container/ApiMonitoring/Explorer/Domains/DomainDetails/components/ErrorState.tsx:21
- Draft comment:
You can simplify the onClick handler by directly passing 'refetch' (i.e. onClick={refetch}) instead of wrapping it in an arrow function. - 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_tV8cDE3PnZ5vR24T
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.
Summary
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Introduces API monitoring feature with new components, utilities, styles, and configurations for exploring and managing API data.
ApiMonitoringcomponent inpages/ApiMonitoringfor API monitoring domain.Explorercomponent incontainer/ApiMonitoring/Explorerfor exploring API data.DomainListandDomainDetailscomponents for displaying and interacting with API domains.StatusCodeTableandEndPointDetailscomponents for detailed API endpoint data.utils.tsxfor formatting API data and constructing query payloads.getEndPointsQueryPayloadandgetEndPointDetailsQueryPayloadfor API data queries.Explorer.styles.scssandApiMonitoringPage.styles.scssfor new components.routes.tsandmenuItems.tsxto include API Monitoring in navigation.utils/permission/index.ts.QueryBuilderSearchV2to support hardcoded attribute keys for API monitoring.lucide-reactversion inpackage.json.This description was created by
for 4b00ec1. It will automatically update as commits are pushed.