Skip to content

Comments

fix: licenses in community edition & improve messaging#7456

Merged
therealpandey merged 6 commits intomainfrom
feat/signoz-versions
Apr 1, 2025
Merged

fix: licenses in community edition & improve messaging#7456
therealpandey merged 6 commits intomainfrom
feat/signoz-versions

Conversation

@YounixM
Copy link
Member

@YounixM YounixM commented Mar 27, 2025

Enhance platform to handle cloud, self-hosted, community, and enterprise user types with tailored routing, error handling, and feature access.

Closes #7449
Fixes #6826
Closes https://github.com/SigNoz/platform-pod/issues/595


Important

Enhance platform to handle different user types with tailored routing, error handling, and feature access, including updates to user type detection, error handling, and feature access control.

  • User Type Handling:
    • useGetTenantLicense updated to distinguish between isCloudUser, isEnterpriseSelfHostedUser, isCommunityUser, and isCommunityEnterpriseUser.
    • SideNav.tsx and SettingsPage updated to use new user type distinctions for feature access and UI adjustments.
  • Error Handling:
    • Added errors.TypeUnsupported handling in render.go.
    • New routes in http_handler.go for /api/v3/licenses and /api/v3/licenses/active with specific error responses.
  • Feature Access:
    • Conditional feature access in SideNav.tsx and SettingsPage based on user type.
    • useGetDeploymentsData now accepts an isEnabled parameter to conditionally enable queries.
  • Miscellaneous:
    • Removed Sentry and Posthog initialization from index.tsx.
    • Updated AppRoutes/index.tsx to include Sentry error boundary and conditional analytics initialization.

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

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 011ede7 in 2 minutes and 25 seconds

More details
  • Looked at 420 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 12 drafted comments based on config settings.
1. frontend/src/SideNav/SideNav.styles.scss:50
  • Draft comment:
    Avoid hardcoding color values like '#fff'; use design tokens or predefined color constants for better 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/SideNav/SideNav.styles.scss:212
  • Draft comment:
    Replace hardcoded background color '#0b0c0e' with a design token to maintain theming 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.
3. frontend/src/container/SideNav/SideNav.styles.scss:50
  • Draft comment:
    Avoid hardcoding colors like '#fff'. Use design tokens or predefined color constants for consistent 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.
4. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:956
  • Draft comment:
    Avoid inline styles (e.g., 'style={{ width: 90 }}'). Move static style definitions to external CSS or styled components to enhance 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.
5. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:490
  • Draft comment:
    The functions handling limit payload creation (e.g., handleAddLimit and handleUpdateLimit) are quite complex. Consider refactoring them into smaller helper functions to reduce cognitive complexity and improve testability.
  • 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/CustomDomainSettings/CustomDomainSettings.tsx:101
  • Draft comment:
    Consider clearing the setTimeout when the component unmounts to avoid potential memory leaks during polling.
  • 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/AppRoutes/index.tsx:267
  • Draft comment:
    Typographical note: In the comments above line 267, consider changing 'fails' to 'fail' (since it's referring to multiple calls) and 'indefinitive' to 'indefinite' to improve 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.
8. frontend/src/container/OnboardingV2Container/IngestionDetails/IngestionDetails.tsx:192
  • Draft comment:
    Typographical error: Consider changing 'multiple ingestions keys' to 'multiple ingestion keys' for grammatical 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/SideNav/SideNav.tsx:140
  • Draft comment:
    Typo: The variable name 'onboaringRoute' should be renamed to 'onboardingRoute' to fix the spelling 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.
10. frontend/src/container/SideNav/SideNav.tsx:183
  • Draft comment:
    Typo: The function name 'handleUserManagentMenuItemClick' contains a misspelling. Consider renaming it to 'handleUserManagementMenuItemClick'.
  • 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/SideNav/SideNav.tsx:372
  • Draft comment:
    Typo: In the tooltip text, the phrase 'in order enable Enterprise features' is missing the word 'to'. It should read 'in order to enable Enterprise features'.
  • 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%
    While this is a real grammatical error that makes the text slightly awkward to read, it's a very minor UI text issue. The rules state not to comment on pure frontend UI changes and to assume the author made UI changes correctly. This falls into that category - it's just UI text content.
    The text is technically incorrect and fixing it would improve readability. The rule about UI changes might not have been intended to cover actual errors in text content.
    The rules are quite clear about not commenting on UI changes. While this is a real error, it's a minor one that doesn't affect functionality, and the rules say to trust the author on UI decisions.
    Delete this comment as it violates the rule about not commenting on UI changes. While it points out a real grammatical error, UI text content falls under the UI changes that we should trust the author to handle.
12. frontend/src/pages/Settings/index.tsx:18
  • Draft comment:
    The property 'trialInfo?.workSpaceBlock' in line 18 uses an inconsistent casing ('workSpaceBlock'). Consider using 'workspaceBlock' (all lowercase) for consistency with typical naming conventions, unless this spelling was intentional.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_TBOdsK5ChbVjtZTN


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.

@YounixM YounixM force-pushed the feat/signoz-versions branch from 011ede7 to d67c22e Compare March 27, 2025 20:05
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 d67c22e in 3 minutes and 18 seconds

More details
  • Looked at 290 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. frontend/src/AppRoutes/index.tsx:269
  • Draft comment:
    Telemetry (Sentry and Posthog) initialization logic is now placed in a useEffect inside the App component. Consider extracting this logic into a dedicated helper module for clarity and reusability, which also helps keep the component lean.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. frontend/src/index.tsx:40
  • Draft comment:
    Sentry.ErrorBoundary was removed from index.tsx and is now wrapped around the App within AppRoutes. Ensure this intended change still covers errors at the highest level, as errors occurring before AppRoutes mounts might not be caught.
  • 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%
    This is a speculative comment asking to "ensure" something. It's asking the author to verify their intention and double-check things. We can't see the AppRoutes file to verify the new error boundary location. The comment violates multiple rules about not asking for verification or making speculative comments.
    The error handling coverage concern could be valid from an architectural perspective. Moving error boundaries could impact application stability.
    While the architectural concern may be valid, the comment violates our rules by being speculative and asking for verification. We also can't verify the new location without seeing AppRoutes.
    The comment should be deleted as it violates rules against asking for verification and making speculative comments. If there was a real error handling issue, it would be caught in testing.
3. frontend/src/AppRoutes/index.tsx:241
  • Draft comment:
    The condition for booting Intercom now checks for (isCloudUser || isEnterpriseSelfHostedUser). Confirm that this meets the intended requirements for messaging in both cloud and enterprise self-hosted environments.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. frontend/src/AppRoutes/index.tsx:264
  • Draft comment:
    Consider updating the analytics enabling condition to include enterprise self-hosted users. Currently, the check uses only 'isCloudUser', but later analytics (Sentry/posthog) are initialized for both cloud and enterprise self-hosted users. This could lead to missed analytics for enterprise self-hosted users.
  • 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/AppRoutes/index.tsx:327
  • Draft comment:
    Moving the Sentry.ErrorBoundary into AppRoutes might reduce error coverage because errors originating in higher-level providers (e.g., ThemeProvider, TimezoneProvider) in index.tsx will no longer be captured. Confirm if this change in error boundary placement is intentional.
  • 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%
  1. The comment suggests the ErrorBoundary should be higher up, but it's already at the highest level possible within this component. 2. The comment seems to misunderstand the component hierarchy. 3. Without seeing the previous code or parent components, we can't verify if this is actually a "move" or where it was before. 4. The current placement looks correct as it wraps all providers.
    I could be wrong about the previous placement of the ErrorBoundary - maybe it was in a parent component that would have caught more errors.
    Even without seeing the previous code, the current placement is logically sound as it wraps all providers in this component. If there are higher-level concerns, they should be handled at those levels.
    The comment should be deleted as it appears to be incorrect about the current ErrorBoundary placement and makes assumptions about previous code we can't verify.
6. frontend/src/AppRoutes/index.tsx:197
  • Draft comment:
    Before calling window.Intercom, consider checking if it is defined (e.g., typeof window.Intercom === 'function') to avoid potential runtime errors if Intercom is not available.
  • 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/AppRoutes/index.tsx:269
  • Draft comment:
    The Sentry initialization inside the useEffect may be re-triggered on changes to the license flags. Ensure that re-initializing Sentry (and calling Sentry.close in the else branch) does not cause unintended side effects or performance overhead.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to ensure that re-initializing Sentry does not cause unintended side effects or performance overhead. This falls under the rule of not asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
8. frontend/src/AppRoutes/index.tsx:316
  • Draft comment:
    Typo: In the comment, 'indefinitive loading' should probably be 'indefinite loading' (or 'infinite loading') to improve clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_SDbhrChsr33Be99g


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

@therealpandey therealpandey changed the title feat: handle messaging in the platform for cloud, self-hosted, commun… fix: fix licenses in community edition & improve messaging Mar 31, 2025
@therealpandey therealpandey changed the title fix: fix licenses in community edition & improve messaging fix: licenses in community edition & improve messaging Mar 31, 2025
@YounixM YounixM force-pushed the feat/signoz-versions branch from d67c22e to 464573f Compare April 1, 2025 15:08
@github-actions github-actions bot added the bug Something isn't working label Apr 1, 2025
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. Incremental review on 464573f in 2 minutes and 45 seconds

More details
  • Looked at 291 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. frontend/src/AppRoutes/index.tsx:237
  • Draft comment:
    Consider extracting the long conditional in the Intercom bootstrap into a well-named variable for improved readability.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
2. frontend/src/AppRoutes/index.tsx:271
  • Draft comment:
    Reinitializing Sentry in a useEffect may lead to multiple initializations; consider checking if Sentry is already initialized or moving global Sentry init outside conditional logic.
  • 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 concern is valid - useEffect will run multiple times if its dependencies change. However, the code handles this by properly cleaning up with Sentry.close() in the else branch. The initialization is intentionally tied to the user type changing. This pattern appears intentional and well-handled.
    The comment raises a real concern about multiple initializations. Perhaps there could be edge cases where rapid changes to isCloudUser/isEnterpriseSelfHostedUser could cause issues.
    The code properly handles cleanup and reinitialization through Sentry.close(). The pattern used here appears to be a valid way to conditionally initialize Sentry based on user type.
    The comment should be deleted. While it raises a valid concern, the code properly handles Sentry initialization/cleanup and the pattern appears intentional.
3. frontend/src/index.tsx:40
  • Draft comment:
    Removal of Sentry initialization and error boundary from index.tsx in favor of wrapping AppRoutes is acceptable; ensure that no uncaught errors occur before AppRoutes mounts.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
4. frontend/src/AppRoutes/index.tsx:46
  • Draft comment:
    Removed licensesFetchError from the context destructuring. Please add a clarifying comment on why this omission is intentional for community edition to avoid accidental regressions.
  • 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/AppRoutes/index.tsx:240
  • Draft comment:
    The additional check (isCloudUser || isEnterpriseSelfHostedUser) before booting Intercom is appropriate to restrict chat support to eligible editions.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the additional check is appropriate, which does not align with the rules provided.
6. frontend/src/AppRoutes/index.tsx:270
  • Draft comment:
    Telemetry (posthog and Sentry) is conditionally initialized based on tenant type. This is a good approach; consider refactoring this logic into a custom hook for clarity and reusability if it expands further.
  • 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 telemetry logic is contained in a single useEffect with clear purpose. While it could be moved to a hook, the current implementation is already fairly readable and self-contained. The logic is specific to this component and may not need reuse elsewhere. Moving it to a hook would add indirection without clear benefits.
    The suggestion could improve code organization if this telemetry logic needs to be reused elsewhere. The current implementation does handle multiple services which could make a case for abstraction.
    However, there's no evidence that this logic needs reuse, and the current implementation is already clean and focused. Creating a hook would add complexity without clear benefits.
    The comment should be removed as the suggested refactor would add unnecessary complexity without clear benefits. The current implementation is sufficiently clear and maintainable.
7. frontend/src/AppRoutes/index.tsx:328
  • Draft comment:
    Wrapping the app in Sentry.ErrorBoundary with a fallback component improves error resilience. Consider whether providers above this boundary also require error handling.
  • 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 comment raises a valid architectural concern about error handling coverage. The providers could potentially throw errors that would not be caught by the current error boundary. However, the comment is somewhat speculative ("consider whether...") and doesn't definitively point to a specific issue. It's more of a suggestion to think about something rather than identifying a clear problem.
    The comment could be seen as a good architectural suggestion for improving error handling. However, it violates the rule about not making speculative comments ("If X, then Y is an issue").
    While the architectural suggestion has merit, our rules explicitly state not to make speculative comments. We should only keep comments that point to definitive issues.
    Delete the comment because it is speculative and doesn't point to a definitive issue that needs to be fixed.
8. frontend/src/index.tsx:4
  • Draft comment:
    Sentry and posthog initialization code has been removed from index.tsx and moved into AppRoutes for conditional telemetry setup. Ensure that any errors occurring in providers above AppRoutes are adequately handled.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is informative and suggests ensuring error handling, which violates the rule against asking the PR author to ensure behavior is intended or tested. It doesn't provide a specific code suggestion or ask for a specific test to be written.
9. frontend/src/AppRoutes/index.tsx:290
  • Draft comment:
    Sentry is configured with tracesSampleRate set to 1.0. Confirm that capturing 100% of transactions in production is intended, as this may impact performance.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the author to confirm their intention regarding a specific configuration setting. It is not making a specific code suggestion or asking for a test to be written. Therefore, it violates the rule against asking the PR author to confirm their intention.
10. frontend/src/AppRoutes/index.tsx:318
  • Draft comment:
    Consider rephrasing the comment 'if all of the data is not set then return a spinner, this is required because there is some gap between loading states and data setting' to something clearer, for example: 'If the required data is not fully loaded, return a spinner due to a gap between loading states and data initialization.'
  • Reason this comment was not posted:
    Comment was on unchanged code.
11. frontend/src/AppRoutes/index.tsx:314
  • Draft comment:
    Consider changing the comment 'if the required calls fails then return a something went wrong error' to 'if the required calls fail, then return a "something went wrong" error' for improved 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_F7FYm0c702hNrWIZ


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
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 ddbc559 in 3 minutes and 26 seconds

More details
  • Looked at 31 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 17 drafted comments based on config settings.
1. pkg/query-service/app/http_handler.go:618
  • Draft comment:
    Licenses endpoints now return a 501 error via render.Error. This fixes the spinner issue in Community edition. Confirm that this ‘not implemented’ message is the desired behavior for community users.
  • 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 appears to be asking for confirmation about intended behavior, which violates the rule "Do NOT ask the PR author to confirm their intention". The comment is also speculative about fixing a spinner issue without strong evidence. The code change itself is clear - it adds two endpoints that return "not implemented" errors.
    Could the comment be highlighting a legitimate concern about the user experience with the spinner? Maybe this deserves discussion?
    While UX concerns are valid, the rules clearly state not to ask for confirmation of intent or make speculative comments. The code change itself is straightforward and doesn't require clarification.
    The comment should be deleted as it violates the rules against asking for confirmation and making speculative comments. The code change is clear and self-explanatory.
2. pkg/query-service/app/http_handler.go:280
  • Draft comment:
    This file contains a very large number of HTTP handler functions. Consider splitting the file by grouping related endpoints (e.g., authentication, integrations, logs, metrics) into separate packages/modules 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.
3. pkg/query-service/app/http_handler.go:2112
  • Draft comment:
    There is commented-out code in the loginUser handler (lines 2112-2115). If no longer needed, removing unused commented code will keep the codebase clean.
  • 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. pkg/query-service/app/http_handler.go:5190
  • Draft comment:
    The live tail logs implementations for V2 and V3 are very similar. Consolidate common logic to reduce duplication and simplify future changes.
  • 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. pkg/query-service/app/http_handler.go:2450
  • Draft comment:
    Error responses are constructed by marshaling error details into JSON. Ensure that no sensitive internal error details are leaked in production responses.
  • 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. pkg/query-service/app/http_handler.go:4650
  • Draft comment:
    The file is extremely long (over 5000 lines). Consider refactoring it into smaller, modular files for improved maintainability and easier navigation.
  • 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. pkg/query-service/app/http_handler.go:4956
  • Draft comment:
    Typo in variable name: 'errQuriesByName' should be 'errQueriesByName'.
  • 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. pkg/query-service/app/http_handler.go:4852
  • Draft comment:
    In getSpanKeysV3, the function returns on the first trace query found. If there are multiple trace queries, consider whether you need to aggregate span keys from all of them.
  • 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. pkg/query-service/app/http_handler.go:5009
  • Draft comment:
    Regex patterns are compiled inside sendQueryResultEvents on each call. Consider caching the compiled regex objects to improve performance.
  • 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. pkg/query-service/app/http_handler.go:5277
  • Draft comment:
    The handler overrides r.Body using a query parameter 'q'. This nonstandard approach may be confusing; ensure this behavior is intentional and well documented.
  • 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. pkg/query-service/app/http_handler.go:2149
  • Draft comment:
    Several endpoints use 'context.Background()' instead of the request's context (r.Context()). This bypasses any cancellation signals from the client. Consider using r.Context() to better support cancellations.
  • 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. pkg/query-service/app/http_handler.go:2577
  • Draft comment:
    The type assertion for extracting an int value using '*value.(*uint8)' may panic if the underlying type is not as expected. Consider using a safe type assertion with a check.
  • 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. pkg/query-service/app/http_handler.go:5149
  • Draft comment:
    For query progress updates, validate the existence of the 'q' query parameter before subscribing, to avoid unexpected behavior if it is missing.
  • 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.
14. pkg/http/render/render.go:46
  • Draft comment:
    Typo suggestion: Check if 'errors.Unwrapb(cause)' on line 46 is intended. It looks like it might be a typographical error; perhaps it should be 'errors.Unwrap(cause)'?
  • 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.
15. pkg/query-service/app/http_handler.go:1334
  • Draft comment:
    Typo: 'apiRrr' appears as a variable name in the testRule function. This is likely a typo and should be renamed to 'apiErr' 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.
16. pkg/query-service/app/http_handler.go:2995
  • Draft comment:
    Typo: The variable name 'errQuriesByName' is misspelled multiple times. It should be 'errQueriesByName'.
  • 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.
17. pkg/query-service/app/http_handler.go:4971
  • Draft comment:
    Typo in comment: 'experssion' should be corrected to 'expressions' to improve 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_qC2gnqfV19g7doGj


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 cf63057 in 1 minute and 24 seconds

More details
  • Looked at 34 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. pkg/query-service/app/server.go:431
  • Draft comment:
    Prefer using structured logging (e.g. zap) instead of fmt.Println for consistency in logs.
  • 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. pkg/query-service/app/server.go:460
  • Draft comment:
    When shutting down HTTP servers, consider using a context with timeout (e.g. context.WithTimeout) instead of context.Background() to ensure graceful shutdown and avoid potential hangs.
  • 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. pkg/query-service/app/server.go:278
  • Draft comment:
    The CORS options allow origins set as '*' in multiple places. Review if this is acceptable for your use case or if it should be restricted for security.
  • 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. pkg/query-service/app/server.go:431
  • Draft comment:
    Use zap logging instead of fmt.Println 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.
5. pkg/query-service/app/server.go:421
  • Draft comment:
    Consider securing or restricting the pprof endpoint in production.
  • 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. pkg/query-service/app/server.go:415
  • Draft comment:
    Consider buffering s.unavailableChannel to prevent potential blocking if no consumer is reading.
  • 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. pkg/query-service/app/server.go:460
  • Draft comment:
    Use a context with timeout for Shutdown calls to ensure graceful and timely server shutdown.
  • 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. pkg/query-service/app/http_handler.go:4955
  • Draft comment:
    There are several instances where the variable name 'errQuriesByName' (and similar variants like 'errQuriesByNameFetchLatency') is used. It appears to be a typo; the correct spelling should be 'errQueriesByName'. Please update these variable names for clarity and 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. pkg/query-service/app/server.go:314
  • Draft comment:
    Typo Alert: The error message in the getUserFromRequest function currently reads "orgId is missing in the claims". For consistency with the field 'OrgID', consider changing it to "OrgID is missing in the claims".
  • 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_gpm3OiCRv1NSCUV0


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

therealpandey
therealpandey previously approved these changes Apr 1, 2025
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. Incremental review on 1355092 in 1 minute and 59 seconds

More details
  • Looked at 16 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/container/SideNav/SideNav.tsx:325
  • Draft comment:
    The comment says 'EE cloud users' while the condition checks for isEnterpriseSelfHostedUser. Consider updating the comment (e.g., to 'EE self-hosted users') so that it accurately reflects the condition.
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/container/SideNav/SideNav.tsx:325
  • Draft comment:
    The comment mentions 'with an active license' but the condition only checks the user type (isEnterpriseSelfHostedUser) without validating if the license is active. If an active license is required, consider adding that check or updating the comment accordingly.
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/container/SideNav/SideNav.tsx:140
  • Draft comment:
    Typographical error: The variable name 'onboaringRoute' looks like it should be 'onboardingRoute'.
  • 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/SideNav/SideNav.tsx:183
  • Draft comment:
    Typographical error: The function name 'handleUserManagentMenuItemClick' is likely misspelled. It should probably be 'handleUserManagementMenuItemClick'.
  • 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/SideNav/SideNav.tsx:388
  • Draft comment:
    Typographical/grammatical issue: In the tooltip text, the phrase 'in order enable Enterprise features' should be 'in order to enable Enterprise features'.
  • 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_YWMjgf83WoiCFofV


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.

@therealpandey therealpandey enabled auto-merge (squash) April 1, 2025 19:42
@therealpandey therealpandey merged commit 597752a into main Apr 1, 2025
13 of 15 checks passed
@therealpandey therealpandey deleted the feat/signoz-versions branch April 1, 2025 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Frontend for Community version stuck on forever loading

3 participants