Skip to content
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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sawhil
Copy link
Member

@sawhil sawhil commented Feb 12, 2025

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.

  • Behavior:
    • Adds ApiMonitoring component to pageComponents.ts and routes.ts for API monitoring feature.
    • New route /api-monitoring/explorer added to routes.ts and constants/routes.ts.
    • Updates AppLayout to include isApiMonitoringView() for layout adjustments.
  • UI Components:
    • Adds ApiMonitoringPage and Explorer components in ApiMonitoringPage.tsx and Explorer.tsx.
    • Styles for API Monitoring page added in ApiMonitoringPage.styles.scss.
    • Adds ApiMonitoring to sidebar in menuItems.tsx with Binoculars icon.
  • Permissions:
    • Updates routePermission in utils/permission/index.ts to include API_MONITORING for all roles.
  • Dependencies:
    • Updates lucide-react version in package.json.

This description was created by Ellipsis for f0b0889. It will automatically update as commits are pushed.

@sawhil sawhil requested a review from YounixM as a code owner February 12, 2025 10:57
Copy link

welcome bot commented Feb 12, 2025

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

@CLAassistant
Copy link

CLAassistant commented Feb 12, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ sawhil
❌ Sahil


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.

@github-actions github-actions bot added docs required enhancement New feature or request labels Feb 12, 2025
@sawhil sawhil marked this pull request as draft February 12, 2025 10:59
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 13 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
Copy link
Contributor

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".

Suggested change
EXPLORER : API MONITROING LANDING PAGE
EXPLORER : API MONITORING LANDING PAGE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants