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: celery overview page #6918

Merged
merged 17 commits into from
Jan 27, 2025
Merged

feat: celery overview page #6918

merged 17 commits into from
Jan 27, 2025

Conversation

SagarRajput-7
Copy link
Contributor

@SagarRajput-7 SagarRajput-7 commented Jan 24, 2025

Summary

Added Celery Overview Page

Related Issues / PR's

Screenshots

image
Screen.Recording.2025-01-27.at.10.19.04.AM.mov

Affected Areas and Manually Tested Areas


Important

Add Celery Overview page with new components, routes, and API integration for displaying Celery task data.

  • Behavior:
    • Adds CeleryOverview component in CeleryOverview.tsx to display Celery task overview.
    • Integrates getQueueOverview API in getQueueOverview.ts to fetch queue data.
    • Adds filtering options in CeleryOverviewConfigOptions.tsx using useCeleryFilterOptions.
  • Routing:
    • Adds route MESSAGING_QUEUES_CELERY_OVERVIEW in routes.ts and AppRoutes/pageComponents.ts.
    • Updates MessagingQueuesMainPage.tsx to include Celery Overview tab.
  • Styles:
    • Adds styles for Celery Overview components in CeleryOverview.styles.scss, CeleryOverviewConfigOptions.styles.scss, and CeleryOverviewTable.styles.scss.
  • Permissions:
    • Updates routePermission in utils/permission/index.ts to include MESSAGING_QUEUES_CELERY_OVERVIEW.

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

@github-actions github-actions bot added the enhancement New feature or request label Jan 24, 2025
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Base automatically changed from celery-task to main January 27, 2025 03:23
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@SagarRajput-7 SagarRajput-7 marked this pull request as ready for review January 27, 2025 05:10
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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 e48223c in 1 minute and 40 seconds

More details
  • Looked at 1051 lines of code in 21 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/AppRoutes/routes.ts:414
  • Draft comment:
    The component for the route MESSAGING_QUEUES_CELERY_OVERVIEW should be CeleryOverview instead of MessagingQueues.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The pattern in the code shows MessagingQueues being used as a container component for multiple messaging queue related routes. This is likely intentional where the component internally handles different views based on the route. The comment suggests breaking this pattern without strong evidence that it's incorrect. Additionally, there's no evidence that a CeleryOverview component even exists in the codebase.
    I could be wrong if there's a specific architectural decision that Celery-related views should use separate components, or if the MessagingQueues component isn't properly handling the Celery overview case.
    Without seeing strong evidence of either of those cases, and given the clear pattern of using MessagingQueues for other messaging queue routes including another Celery route, the current implementation appears intentional and correct.
    The comment should be deleted as it suggests breaking an established pattern without strong evidence that the current implementation is wrong.
2. frontend/src/components/CeleryOverview/CeleryOverviewConfigOptions/CeleryOverviewConfigOptions.styles.scss:21
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values for consistency. This is also applicable in other stylesheets like CeleryOverviewTable.styles.scss and CeleryOverview.styles.scss.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_3vEELARp8eRJjEQG


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.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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.

👍 Looks good to me! Incremental review on 60281c4 in 30 seconds

More details
  • Looked at 86 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/components/CeleryTask/CeleryTaskGraph/CeleryTaskGraphUtils.ts:536
  • Draft comment:
    The celeryTaskLatencyWidgetData function is duplicated. Consider removing the first instance to avoid confusion and maintain code clarity.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/components/CeleryTask/CeleryTaskGraph/CeleryTaskGraphGrid.tsx:69
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to the entire file.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_ex2EJVGgHMJbuS5v


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on ed45628 in 15 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/components/CeleryTask/CeleryTaskGraph/CeleryTaskGraphUtils.ts:572
  • Draft comment:
    The change to use '{{celery.task_name}}' for the legend improves clarity by dynamically displaying the task name. This is a good enhancement for the Celery Overview Page.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The change in the legend field is straightforward and aligns with the intent of the PR to improve the Celery Overview Page.

Workflow ID: wflow_L4yG67O8t4diCP32


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@SagarRajput-7 SagarRajput-7 merged commit bd0c4be into main Jan 27, 2025
16 of 18 checks passed
@SagarRajput-7 SagarRajput-7 deleted the celery-overview-page branch January 27, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs not required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants