feat: Funnel Details Page Base Structure

[ahmadshaheer]

Summary

Related Issues / PR's

close https://github.com/SigNoz/engineering-pod/issues/2245

Screenshots

2025-03-19.17-37-23.mov

Affected Areas and Manually Tested Areas

---

Important

Introduces a new funnel details page with components and hooks for managing funnel steps and integrates with existing funnel APIs.

• Funnel Details Page:
  • Introduces TracesFunnelDetails page with FunnelConfiguration and FunnelResults components.
  • Uses FunnelProvider for context management.
• Components:
  • Adds AddFunnelDescriptionModal, AddFunnelStepDetailsModal, DeleteFunnelStep, FunnelBreadcrumb, FunnelStep, FunnelStepPopover, InterStepConfig, StepsContent, StepsFooter, StepsHeader.
  • Adds EmptyFunnelResults, FunnelGraph, FunnelMetricsTable, FunnelResults, FunnelTable, FunnelTopTracesTable, OverallMetrics, StepsTransitionMetrics, StepsTransitionResults, TopSlowestTraces.
• Hooks:
  • Adds useFunnelConfiguration, useFunnelGraph, useFunnelMetrics, useFunnels, useHandleTraceFunnelsSearch, useHandleTraceFunnelsSort.
• API Integration:
  • Updates API interactions in useFunnels.tsx for CRUD operations on funnels.
  • Adds types for FunnelData, FunnelStepData, CreateFunnelPayload, CreateFunnelResponse.
• Miscellaneous:
  • Updates TracesModulePage and TracesModulePage/constants.tsx to include funnel routes.
  • Updates styles in TracesFunnels.styles.scss and other component-specific styles.

^{This description was created by for d1ed777. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 7eeea10 in 3 minutes and 29 seconds

More details

• Looked at 1129 lines of code in 34 files
• Skipped 0 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. frontend/src/container/AppLayout/index.tsx:331

• Draft comment:
  This function duplicates existing functionality. Consider using the existing isFunnelDetails function instead.

• function isFunnelDetails (constants.tsx)

• 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 there is some duplication, the functions are in different files and serve different components. The AppLayout component is a core layout component that needs its own routing logic. Moving routing logic between files could create unwanted dependencies. The duplication is minor (one line) and may be justified for separation of concerns.
  The comment correctly identifies code duplication. Having multiple functions that do the same thing could lead to maintenance issues.
  However, this appears to be an acceptable case of duplication since it maintains separation between layout and page-specific code. The duplication is minimal and the functions serve different components.
  The comment should be deleted. While technically correct about the duplication, the current implementation is justified for maintaining good component separation.

2. frontend/src/pages/TracesFunnelDetails/TracesFunnelDetails.tsx:14

• Draft comment:
  Remove debug logging (console.log) before merging.
• Reason this comment was not posted:
  Comment was on unchanged code.

3. frontend/src/pages/TracesFunnelDetails/components/FunnelResults/StepsTransitionResults.tsx:30

• Draft comment:
  Consider safely handling an empty steps array instead of defaulting to an empty string in state initialization.
• 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 current code already handles empty arrays safely. The optional chaining operator (?.) will return undefined if stepTransitions[0] is undefined, which then falls back to empty string. The suggested change is functionally equivalent but more verbose. This comment doesn't identify a real issue or suggest a meaningful improvement.
  Maybe there's a subtle edge case where the optional chaining behavior differs from an explicit length check? Maybe the more explicit version is more readable?
  The optional chaining operator is a standard TypeScript feature that clearly expresses the intent and handles null/undefined cases safely. The current code is both safe and idiomatic.
  Delete this comment as it suggests a change that provides no real benefit over the current implementation, which is already safe and idiomatic TypeScript.

4. frontend/src/pages/TracesFunnelDetails/components/FunnelConfiguration/StepsContent.tsx:34

• Draft comment:
  The step-add limit is hard-coded to 3 steps; ensure this limit meets all UX requirements or consider making it configurable.
• 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 appears to be a speculative comment asking to verify requirements rather than pointing out a clear issue. The 3-step limit could be an intentional UX decision. Without strong evidence that this limit is problematic or that it needs to be configurable, we shouldn't keep this comment. The comment starts with "ensure" which is a red flag according to our rules.
  The limit of 3 steps could genuinely be too restrictive for some use cases, and making it configurable might be a valid improvement.
  While that's possible, we don't have evidence that this is actually a problem. The rules specifically state not to make speculative comments or ask authors to verify requirements.
  Delete this comment as it's asking for requirement verification without clear evidence of an issue, which violates our review rules.

5. frontend/src/pages/TracesFunnelDetails/TracesFunnelDetails.tsx:14

• Draft comment:
  Remove the debug console.log (line 14) before production to avoid unnecessary logging.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. frontend/src/pages/TracesFunnelDetails/TracesFunnelDetails.tsx:10

• Draft comment:
  The PR description mentions an attribute suggestion API for the aggregation type, but no related logic is implemented in this page. Ensure that the UI integrates or clearly defers the API functionality if it’s in scope for this issue.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that the UI integrates or defers the API functionality, which is similar to asking them to confirm their intention or ensure behavior. This violates the rule against asking the author to ensure behavior is intended or tested.

7. frontend/src/pages/TracesModulePage/constants.tsx:22

• Draft comment:
  Using hooks (useLocation) inside an inline component definition here may hinder debugging and global search. Consider extracting this logic into a separate named component for better maintainability.
• Reason this comment was not posted:
  Marked as duplicate.

8. frontend/src/AppRoutes/pageComponents.ts:110

• Draft comment:
  Typographical error: The component name 'ListAllALertsPage' appears to contain a typo ('ALerts'). 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.

Workflow ID: wflow_QPhQeC635Um5QhUO

---

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.

[sawhil]

Minor comments added

[ellipsis-dev]

❌ Changes requested. Incremental review on 974e8ff in 2 minutes and 18 seconds

More details

• Looked at 36 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. frontend/src/pages/TracesModulePage/TracesModulePage.tsx:23

• Draft comment:
  Ensure tracesFunnel is defined as a function accepting pathname as an argument. The change from 'tracesFunnel' to 'tracesFunnel(pathname)' impacts its usage.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. frontend/src/pages/TracesFunnels/components/FunnelsList/FunnelsList.tsx:99

• Draft comment:
  Data is defined as a required prop in FunnelsListProps. Using optional chaining (data?.map) may hide issues if data is unexpectedly undefined. Consider removing '?' or updating the prop type.
• Reason this comment was not posted:
  Marked as duplicate.

3. frontend/src/pages/TracesModulePage/TracesModulePage.tsx:23

• Draft comment:
  Confirm that tracesFunnel is intended to be used as a function taking 'pathname' as a parameter. This change implies route generation relies on pathname.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_90LsCDac5KEQwpbV

---

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.

[ellipsis-dev]

❌ Changes requested. Incremental review on 7393790 in 2 minutes and 22 seconds

More details

• Looked at 104 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:95

• Draft comment:
  Avoid hardcoding color values in inline styles. Instead of using literal 'rgb(29 33 45)' in the backgroundImage, use a design token or color constant from the theme.
• 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/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:164

• Draft comment:
  Avoid hardcoding asset paths. Consider importing the funnel-add icon as a module or using a predefined asset constant rather than using a literal string.
• 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/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:89

• Draft comment:
  Multiple elements use inline styling (e.g. dynamic padding and background properties). Consider moving static styling to a CSS class and only compute dynamic values if necessary.
• 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/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:304

• Draft comment:
  Review the defaulting of the feature flag: the fallback to 'true' if the flag is not found may unintentionally enable funnel functionality. Confirm if this is the intended default behavior.
• 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 asks for confirmation of intended behavior, which violates our rule about not asking authors to confirm their intention. While the observation about the default behavior is correct, the way the comment is phrased makes it a question rather than an actionable suggestion. If there was a specific security or functionality concern, it should be stated directly.
  The default behavior could be a deliberate choice to ensure the feature is available by default. Maybe I'm being too strict about the "no questions" rule.
  Even if the default is intentional, asking for confirmation doesn't help. The comment should either point out a specific issue or be removed. Questions about intention don't lead to concrete improvements.
  Delete the comment. While it identifies a real pattern in the code, it's phrased as a question about intention rather than providing actionable feedback.

5. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:89

• Draft comment:
  Avoid inline styles in React components. Consider moving these dynamic style rules (e.g. padding and backgroundImage) to CSS classes or styled components for better maintainability 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.

6. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:290

• Draft comment:
  The PR description mentions an attribute suggestion API for aggregation types, but the diff only implements funnel details functionality. Please ensure that all requirements for issue 2245 are addressed.
• 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_MWWTnru47XRePNJK

---

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.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 352008b in 1 minute and 5 seconds

More details

• Looked at 12 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. frontend/src/pages/TracesFunnelDetails/components/FunnelConfiguration/FunnelStep.tsx:139

• Draft comment:
  Ensure removal of maxTagCount from QueryBuilderSearchV2 is intentional. Verify that its absence doesn’t affect tag rendering behavior in the filters.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

2. frontend/src/pages/TracesFunnelDetails/components/FunnelConfiguration/FunnelStep.tsx:140

• Draft comment:
  Removed 'maxTagCount' prop from QueryBuilderSearchV2. Confirm that this removal doesn’t adversely affect responsive tag display in the filters.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_PYj7URIQnDBBnPiC

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 74db095 in 2 minutes and 29 seconds

More details

• Looked at 18 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. frontend/src/hooks/TracesFunnels/useFunnelMetrics.ts:51

• Draft comment:
  Ensure that changing 'ms' to 'ns' for avg_duration is intentional and consistent with how durations are displayed elsewhere.
• 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 change in the code, which is not allowed according to the rules. It does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.

2. frontend/src/hooks/TracesFunnels/useFunnelMetrics.ts:55

• Draft comment:
  Verify that converting p99_latency to 'ns' instead of 'ms' aligns with user expectations and other metrics in the UI.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to verify their intention, which is against the rules. It does not provide a specific suggestion or point out a clear issue with the code.

3. frontend/src/hooks/TracesFunnels/useFunnelMetrics.ts:51

• Draft comment:
  Changed unit from 'ms' to 'ns' for avg. duration. Ensure the backend returns values in nanoseconds and the formatting logic in getYAxisFormattedValue is updated accordingly.
• 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 comment violates several rules. It's asking to "ensure" something, which is explicitly discouraged. It's speculative about potential backend issues. It's asking for verification rather than pointing out a clear problem. If there was an actual mismatch between backend and frontend, it would be obvious in testing.
  Maybe the unit change could cause serious display issues if not properly coordinated with backend changes? Maybe this is a critical validation step?
  If there was a real incompatibility, it would be immediately visible in testing. The author clearly made an intentional change to both duration fields, suggesting they know what they're doing.
  Delete the comment. It's asking for verification rather than pointing out a clear issue, and any real problems would be caught in testing.

4. frontend/src/hooks/TracesFunnels/useFunnelMetrics.ts:55

• Draft comment:
  Changed unit from 'ms' to 'ns' for P99 latency. Verify that the value and its display are consistent with the expected nanosecond precision.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to verify consistency between the value and its display, which violates the rule against asking the author to ensure behavior is intended or to double-check things. It doesn't provide a specific suggestion or point out a clear issue.

Workflow ID: wflow_EbLiO0vls9gT0dYS

---

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

[ellipsis-dev]

❌ Changes requested. Incremental review on 475a1dd in 3 minutes and 27 seconds

More details

• Looked at 27 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. frontend/src/pages/TracesFunnelDetails/components/FunnelConfiguration/StepsContent.tsx:69

• Draft comment:
  Consider extracting the hardcoded limit (3) into a named constant for better maintainability.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. frontend/src/pages/TracesFunnels/TracesFunnels.styles.scss:49

• Draft comment:
  Avoid hardcoding font-size; prefer using a design token variable instead for consistency.
• Reason this comment was not posted:
  Marked as duplicate.

3. frontend/src/pages/TracesFunnels/TracesFunnels.styles.scss:34

• Draft comment:
  The PR description mentions attribute suggestion API changes, but the diff only shows UI/styling modifications. Confirm that the intended changes are included.
• 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/TracesFunnelDetails/components/FunnelConfiguration/StepsContent.tsx:69

• Draft comment:
  The added comment noting 'For now we are only supporting 3 steps' is clear for a temporary limitation. However, the issue description mentions implementing an attribute suggestion API for aggregation types, which isn’t reflected in this diff. Please ensure the PR either includes that functionality or update the description accordingly.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to update the PR description or ensure the PR includes certain functionality. This violates the rule against asking the author to update the PR description or confirm intentions.

5. frontend/src/pages/TracesFunnelDetails/components/FunnelConfiguration/StepsContent.tsx:90

• Draft comment:
  Typo suggestion: The button label 'Add for new Step' seems off. Consider changing it to 'Add New Step' for better 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_ti5INGMrxlAmoI5W

---

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.

[ellipsis-dev]

❌ Changes requested. Incremental review on ebdafe5 in 3 minutes and 29 seconds

More details

• Looked at 185 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. frontend/src/constants/features.ts:28

• Draft comment:
  Ensure removal of TRACE_FUNNELS flag doesn’t break any dependencies. Verify that other parts of the code aren’t relying on this flag.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that the removal of a flag doesn't break dependencies and to verify that other parts of the code aren't relying on it. This falls under the rule of not asking the author to double-check or ensure behavior, which is not allowed.

2. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:86

• Draft comment:
  Avoid inline styles. Move the dynamic 'paddingLeft' and 'backgroundImage' styles 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.

3. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:215

• Draft comment:
  Inline styles in SpanDuration (e.g. left offset and color) should be moved to CSS 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.

4. frontend/src/pages/TraceDetailV2/index.tsx:68

• Draft comment:
  Consider using a feature flag for enabling Funnels instead of process.env.NODE_ENV check to allow more flexible control in production.
• 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 change appears intentional - they specifically removed the feature flag code and replaced it with NODE_ENV check. This suggests they want to restrict Funnels to development only. The comment is suggesting an alternative approach, but doesn't point out an actual problem. It's more of a suggestion/preference than a required change.
  The comment could be valid if there's a company-wide policy about feature flag usage that I'm unaware of. The development-only restriction might be temporary and feature flags would provide more flexibility.
  While feature flags provide more flexibility, the explicit removal of feature flag code suggests this was an intentional architectural decision to restrict this feature to development only. Without more context about why this change was made, we should respect the author's decision.
  Delete the comment as it's suggesting an alternative approach rather than pointing out a clear issue, and the current implementation appears to be an intentional choice.

5. frontend/src/pages/TracesModulePage/TracesModulePage.tsx:16

• Draft comment:
  Using process.env.NODE_ENV for route toggling similarly limits Funnel functionality to development environment; consider adopting a dedicated feature flag if production access is planned.
• 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 code actually removed a feature flag in favor of NODE_ENV. This seems like an intentional architectural decision to limit this feature to development only. The comment assumes future production access is planned, but we don't have evidence of that. The comment is speculative about future needs rather than addressing a current issue.
  The comment could be valid if there are plans to enable this in production later. Moving from feature flags to NODE_ENV could be seen as a regression in flexibility.
  Without clear evidence that production access is planned, this comment is speculative. The PR author intentionally removed the feature flag, suggesting they specifically want development-only access.
  Delete the comment as it makes assumptions about future requirements and questions an intentional architectural decision without strong evidence that it's incorrect.

6. frontend/src/constants/features.ts:26

• Draft comment:
  TRACE_FUNNELS was removed from the FeatureKeys enum. Confirm this removal is intentional and update any references to funnel functionality accordingly.
• 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 asking for confirmation of an intentional change and suggesting to update references. This violates two rules: 1) Do not ask authors to confirm their intention 2) Do not comment on things that would be caught by the build (removed enum values would cause compilation errors if still referenced). The author clearly meant to remove this flag.
  Maybe there's some critical business context around trace funnels that makes this removal particularly risky and worthy of extra attention?
  Even if trace funnels are important, the build system would catch any lingering references, making this comment unnecessary. Additionally, we should trust the author's intention to remove this flag.
  Delete this comment as it violates rules by asking for confirmation and raising concerns that would be caught by the build system anyway.

7. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:436

• Draft comment:
  The funnel modal is now rendered only when process.env.NODE_ENV is 'development'. Confirm that this gating is intentional for production builds or if a feature flag should be used 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.

8. frontend/src/pages/TraceDetailV2/index.tsx:68

• Draft comment:
  The Funnels tab is now included only if process.env.NODE_ENV==='development'. Ensure that this environment-based gating meets your requirements, or consider using a feature flag for more flexibility.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

9. frontend/src/pages/TracesModulePage/TracesModulePage.tsx:16

• Draft comment:
  The funnel route is now rendered only in development mode via process.env.NODE_ENV. If funnel features are expected in production eventually, a proper feature flag might be more appropriate.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_qb4jLJcRxlQD9QoY

---

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.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 796b682 in 1 minute and 58 seconds. Click for details.

• Reviewed 72 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/hooks/TracesFunnels/useFunnelConfiguration.tsx:146

• Draft comment:
  In the onError callback, ensure that resetting the steps with handleRestoreSteps(lastSavedStepsStateRef.current) accurately reverts to a consistent state. Verify that lastSavedStepsStateRef.current reliably holds the last valid state.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to verify the behavior of the code, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code. Therefore, it should be removed.

2. frontend/src/pages/TracesFunnels/FunnelContext.tsx:145

• Draft comment:
  The new handleRestoreSteps callback is correctly added to the context. Confirm that simply setting steps to the provided oldSteps is sufficient for all rollback scenarios.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to confirm their intention regarding the rollback scenarios. It violates the rule against asking the author to confirm their intention or to ensure behavior is intended.

3. frontend/src/pages/TracesFunnels/FunnelContext.tsx:206

• Draft comment:
  The PR title and description mention an attribute suggestion API for aggregation type, but no related changes appear in the diff. Ensure the PR fully addresses the issue as described.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that the PR fully addresses the issue as described in the title and description. This falls under the rule of not asking the PR author to update the PR description or ensure the behavior is intended.

4. frontend/src/hooks/TracesFunnels/useFunnelConfiguration.tsx:48

• Draft comment:
  Added handleRestoreSteps from useFunnelContext. Verify that its restoration logic properly resets the steps state on error.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

5. frontend/src/hooks/TracesFunnels/useFunnelConfiguration.tsx:147

• Draft comment:
  In the onError callback, steps are reverted using handleRestoreSteps. Consider adding error logging to aid debugging and ensure lastSavedStepsStateRef holds the correct state.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

6. frontend/src/pages/TracesFunnels/FunnelContext.tsx:145

• Draft comment:
  handleRestoreSteps is introduced as a useCallback that simply sets steps. Confirm that this basic restoration is sufficient for all state dependencies or if additional logic is needed.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

7. frontend/src/pages/TracesFunnels/FunnelContext.tsx:204

• Draft comment:
  Including handleRestoreSteps in the context provider's value ensures its availability to consumers. Verify that its inclusion in dependency arrays does not trigger unnecessary re-renders.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_AMoNV36jEK4MCZar

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[aks07]

Apart from the use of process.env (check if this is appropriate) being used in the components, the other comments are mostly suggestions that we could consider setting as standards.

[aks07]

Lets try to have small PRs going forward. This becomes really difficult to review.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 994093f in 2 minutes and 14 seconds. Click for details.

• Reviewed 279 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 8 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/components/CeleryOverview/CeleryOverviewConfigOptions/CeleryOverviewConfigOptions.tsx:42

• Draft comment:
  Good refactor: extracting selectValue and memoizing handleSelectChange improves clarity.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:149

• Draft comment:
  Toggling the funnel button display with process.env.NODE_ENV is clear; verify that the dev-only feature is intended.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. frontend/src/pages/TracesFunnelDetails/components/FunnelResults/utils.tsx:1

• Draft comment:
  Utility for table columns is nicely abstracted; ensure getYAxisFormattedValue and Link usage follow design tokens for colors if applicable.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:86

• Draft comment:
  Avoid inline styles where practical. Consider moving dynamic style definitions (e.g., padding, backgroundImage) to a CSS class or styled component while using design tokens for colors.
• 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/components/CeleryOverview/CeleryOverviewConfigOptions/CeleryOverviewConfigOptions.tsx:102

• Draft comment:
  Consider using default parameter values in the function signature instead of defaultProps for functional components.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

6. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:86

• Draft comment:
  Avoid using inline styles in React components. Move dynamic style properties (e.g. paddingLeft, backgroundImage with hardcoded color 'rgb(29 33 45)') to external CSS classes or styled components using design tokens/constants.
• 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/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:149

• Draft comment:
  The add-funnel button is now wrapped with a process.env.NODE_ENV check. Confirm that restricting this feature to the development environment is intentional since it won't appear in production.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

8. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:214

• Draft comment:
  Avoid inline styles in the SpanDuration component. Instead of applying dynamic styles directly using the style prop (e.g. left, width, backgroundColor), consider using external CSS classes or styled components.
• 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_kuOrCQBIrrL0ZKT9

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed d1ed777 in 2 minutes and 31 seconds. Click for details.

• Reviewed 70 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 11 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/components/QuickFilters/tests/__snapshots__/QuickFilters.test.tsx.snap:20

• Draft comment:
  Added aria-hidden attributes on SVG icons to improve accessibility. Ensure that these updates are consistently applied across similar components.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/__tests__/SpanDuration.test.tsx:44

• Draft comment:
  New jest.mock for 'uplot' has been added. The mock setup looks correct, ensuring 'paths' and default export are available for tests.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. frontend/src/pages/TraceDetail/__test__/TraceDetail.test.tsx:28

• Draft comment:
  The uplot mock here is consistent with other tests. Confirm consistency across tests; otherwise, the setup seems appropriate.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. frontend/src/components/QuickFilters/tests/__snapshots__/QuickFilters.test.tsx.snap:20

• Draft comment:
  Good: 'aria-hidden' attribute has been added to the svg element for improved accessibility.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

5. frontend/src/components/QuickFilters/tests/__snapshots__/QuickFilters.test.tsx.snap:348

• Draft comment:
  Good: 'aria-hidden' attribute added to the svg for improved accessibility.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

6. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/__tests__/SpanDuration.test.tsx:42

• Draft comment:
  Consider extracting the duplicated jest.mock('uplot') into a shared test utility to reduce redundancy.
• 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 suggests there's duplication, but I can't see any evidence of that in this file. Without seeing other test files, I can't verify if this mock is actually duplicated elsewhere. The comment is making an assumption about code we can't see. Even if there is duplication elsewhere, this would be a cross-file issue which we're supposed to ignore. What if there actually is widespread duplication of this mock across many test files that would make this refactoring valuable? What if this is a known pattern in the codebase? Even if there is duplication elsewhere, we're explicitly told to ignore cross-file issues and only focus on the current file. We also need strong evidence to keep comments, which we don't have here. Delete this comment. It's speculative about potential duplication we can't verify, and even if true, it would be a cross-file issue which we're told to ignore.

7. frontend/src/pages/TraceDetail/__test__/TraceDetail.test.tsx:49

• Draft comment:
  Remove the extraneous comma after the closing Route tag in MemoryRouter to avoid unintended text nodes.
• 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/pages/TraceDetail/__test__/TraceDetail.test.tsx:28

• Draft comment:
  Duplicate uplot mock configuration detected here. Consider centralizing this mock in a shared test setup to avoid redundancy.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

9. frontend/src/pages/TraceDetail/__test__/TraceDetail.test.tsx:43

• Draft comment:
  Typographical inconsistency: The test title 'should render tracedetail' (line 43) uses 'tracedetail' in lowercase. For clarity and consistency with the component name, consider changing it to 'TraceDetail'.
• Reason this comment was not posted:
  Comment was on unchanged code.

10. frontend/src/pages/TraceDetail/__test__/TraceDetail.test.tsx:161

• Draft comment:
  Typographical issue: The test name 'should be able to selected another span and see its detail' (line 161) appears grammatically off. Consider changing 'selected' to 'select' so it reads 'should be able to select another span and see its detail'.
• 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/pages/TraceDetail/__test__/TraceDetail.test.tsx:49

• Draft comment:
  Typographical detail: Several tests (e.g., at line 49, and similarly in other test cases) include an extraneous comma after the closing tag within the MemoryRouter. Consider removing these stray commas to keep the JSX 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.

Workflow ID: wflow_egKSKkUZLpMDMyNW

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}