-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: basic scaffolding for api monitoring & page level components added #7097
base: main
Are you sure you want to change the base?
Conversation
Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗 |
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. |
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. Reviewed everything up to f0b0889 in 2 minutes and 28 seconds
More details
- Looked at
265
lines of code in13
files - Skipped
1
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/container/AppLayout/index.tsx:427
- Draft comment:
Avoid inline styles; consider extracting the dynamic margin styling to a CSS class or styled component. - 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/Explorer.tsx:7
- Draft comment:
Typo: 'MONITROING' should be 'MONITORING'. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/AppLayout/index.tsx:427
- Draft comment:
Avoid inline styles for margin; move this styling to a CSS class or styled component to 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.
4. frontend/src/pages/ApiMonitoring/ApiMonitoringPage.styles.scss:15
- Draft comment:
Consider using predefined design tokens for border colors instead of hardcoded CSS variable values if available. - 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 additional rules specifically mention "Use design tokens or predefined color constants instead of hardcoding color values to maintain consistency". However, the code is already using CSS variables (var(--bg-slate-400) and var(--bg-vanilla-300)) which are a form of design tokens, not hardcoded color values. The comment seems to misunderstand that CSS variables can be valid design tokens.
Maybe there are specific design token constants defined elsewhere in the project that should be used instead of these CSS variables. Without seeing the full design system, I can't be certain.
Even without seeing the full design system, CSS variables are a valid form of design tokens and are clearly being used systematically here for theming (with light/dark mode support).
Delete the comment because the code is already using a valid form of design tokens (CSS variables) and is not hardcoding color values.
Workflow ID: wflow_XX1483GhtcQz7qKi
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.
function Explorer(): JSX.Element { | ||
return ( | ||
<Sentry.ErrorBoundary fallback={<ErrorBoundaryFallback />}> | ||
EXPLORER : API MONITROING LANDING PAGE |
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.
Typo: "MONITROING" should be "MONITORING".
EXPLORER : API MONITROING LANDING PAGE | |
EXPLORER : API MONITORING LANDING PAGE |
Summary
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Adds API Monitoring feature with new page, route, and UI components, updating routes, permissions, and sidebar.
ApiMonitoring
component topageComponents.ts
androutes.ts
for API monitoring feature./api-monitoring/explorer
added toroutes.ts
andconstants/routes.ts
.AppLayout
to includeisApiMonitoringView()
for layout adjustments.ApiMonitoringPage
andExplorer
components inApiMonitoringPage.tsx
andExplorer.tsx
.ApiMonitoringPage.styles.scss
.ApiMonitoring
to sidebar inmenuItems.tsx
withBinoculars
icon.routePermission
inutils/permission/index.ts
to includeAPI_MONITORING
for all roles.lucide-react
version inpackage.json
.This description was created by
for f0b0889. It will automatically update as commits are pushed.