Skip to content

feat: base setup for inspect metrics feature#7490

Merged
amlannandy merged 4 commits intomainfrom
feat/inspect-metrics/base-setup
Apr 1, 2025
Merged

feat: base setup for inspect metrics feature#7490
amlannandy merged 4 commits intomainfrom
feat/inspect-metrics/base-setup

Conversation

@amlannandy
Copy link
Copy Markdown
Contributor

@amlannandy amlannandy commented Mar 31, 2025

Summary

  • Setup the base setup for the inspect metrics modal so that other components can be picked in separate PRs individually
  • Add a feature flag on UI side local storage so that the inspect button only shows in metric details when FF is on

Related Issues / PR's

Closes #7486

Screenshots

NA

Affected Areas and Manually Tested Areas

Metrics Explorer


Important

Add inspect metrics feature with modal and feature flag control in Metrics Explorer.

  • Feature:
    • Adds Inspect modal in Inspect.tsx for inspecting metrics, wrapped in Sentry error boundary.
    • Feature flag INSPECT_FEATURE_FLAG_KEY in constants.ts controls visibility of inspect button.
    • isInspectEnabled() in utils.tsx checks feature flag in local storage.
  • UI Changes:
    • MetricDetails.tsx updated to include inspect button if feature flag is enabled.
    • Summary.tsx manages state for opening/closing Inspect modal.
  • Styles:
    • New styles in Inspect.styles.scss for inspect modal and button.
    • Updates in MetricDetails.styles.scss for inspect button.

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

@amlannandy amlannandy requested a review from YounixM as a code owner March 31, 2025 13:19
@github-actions github-actions Bot added docs required enhancement New feature or request labels Mar 31, 2025
Copy link
Copy Markdown
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 4cfa527 in 3 minutes and 10 seconds

More details
  • Looked at 256 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. frontend/src/container/MetricsExplorer/Inspect/Inspect.tsx:34
  • Draft comment:
    Avoid applying inline styles (e.g. background) on Drawer. Consider moving these styles to the SCSS file to adhere to best practices.
  • 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 aligns with the additional rule about avoiding inline styles in React components. However, this is a dynamic style that depends on isDarkMode state - moving it to SCSS would make it harder to implement the dark mode toggle. The overscrollBehavior is also a functional property that might be intentionally placed inline.
    The comment might be too rigid in applying the "no inline styles" rule without considering the dynamic nature of these particular styles. Sometimes inline styles are the most practical solution for dynamic values.
    While the general rule of avoiding inline styles is good, there are valid exceptions for dynamic styles that depend on component state or props.
    The comment should be deleted because these particular inline styles serve a specific purpose (dark mode toggling and scroll behavior) that would be more complicated to implement in SCSS.
2. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:124
  • Draft comment:
    Avoid using inline styling (e.g. background) on the Drawer. Move these styles to the SCSS file to maintain consistency with design tokens.
  • 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/MetricsExplorer/MetricDetails/MetricDetails.tsx:124
  • Draft comment:
    Avoid inline styling for the Drawer background. Use external styles (e.g., CSS classes) with design tokens to maintain design 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/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:56
  • Draft comment:
    Memoizing the feature flag with an empty dependency array means it won’t update if the flag in localStorage changes dynamically. Consider reading it on render or adding proper dependencies.
  • 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 technical point about reactivity. However, feature flags typically don't change during a user session - they usually require a page reload to take effect. The useMemo here is likely an optimization to avoid rechecking the flag on every render. The suggested change wouldn't hurt but also isn't clearly necessary.
    I might be making assumptions about how the feature flag system works. Some systems do support dynamic runtime updates to flags without page reloads.
    Even if the flag system supports dynamic updates, the performance impact of checking on every render vs memoizing is likely negligible, and the current code isn't clearly wrong.
    This comment raises a speculative issue without strong evidence that it's actually problematic. Per the rules, we should not keep speculative comments or those that ask authors to verify behavior.
5. frontend/src/container/MetricsExplorer/Inspect/utils.tsx:7
  • Draft comment:
    Accessing localStorage directly may cause issues in non-browser (SSR) environments. Consider adding a check (e.g., 'typeof window !== "undefined"') before using localStorage.
  • 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%
    This is a frontend-only utility in a React component directory. The code is specifically for browser feature flag checking. SSR issues would be caught during testing/build. The comment suggests a defensive coding practice but may not be critical for this specific use case.
    The comment raises a valid concern about SSR compatibility. Not handling this could cause runtime errors if the code runs server-side.
    Given this is in a frontend-only metrics explorer component and specifically for browser feature flags, SSR compatibility is likely already handled at a higher level or not relevant.
    Delete the comment. While technically correct, it's likely unnecessary given the frontend-specific context and would be caught in testing if it was an issue.
6. frontend/src/container/MetricsExplorer/Inspect/utils.tsx:4
  • Draft comment:
    In the comment on line 4, there is an extra space between 'in' and 'metrics explorer'. Consider removing one of the spaces for clarity.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
    This comment is about a minor formatting issue, which is not critical to the functionality or design of the code. It doesn't provide a significant improvement or suggestion related to the code's logic or structure.
7. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:107
  • Draft comment:
    Typographical issue: The inline comment reads "Show the based on the feature flag. Will remove before releasing the feature" which appears to be missing a word (e.g., 'button' or another descriptor). Please reword it for clarity (e.g., 'Show the button based on the feature flag. Will remove before releasing the feature').
  • 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 rules state we should not make purely informative comments. This comment is about improving the clarity of a temporary comment that will be removed before release anyway. It's not about any functional code changes or important code quality issues. The existing comment, while slightly unclear, is still understandable and doesn't affect the code's functionality.
    The comment does point out a legitimate grammatical issue. Clear documentation, even in temporary comments, could be considered important for maintainability.
    However, this is a temporary comment that will be removed before release, so improving its clarity provides very little value. The rules explicitly state not to make purely informative comments.
    Delete the comment as it's purely informative and addresses a temporary comment that will be removed anyway.

Workflow ID: wflow_Bymo1sinn5RNIBSQ


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.

Comment thread frontend/src/container/MetricsExplorer/Inspect/index.ts
Comment thread frontend/src/container/MetricsExplorer/Inspect/Inspect.tsx
Comment thread frontend/src/container/MetricsExplorer/Inspect/Inspect.styles.scss Outdated
Comment thread frontend/src/container/MetricsExplorer/Inspect/Inspect.styles.scss Outdated
Copy link
Copy Markdown
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 3ae412b in 54 seconds

More details
  • Looked at 31 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.styles.scss:24
  • Draft comment:
    Good use of design tokens for styling the inspect button. Ensure the component’s styling remains consistent with overall design patterns.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. frontend/src/container/MetricsExplorer/Summary/Summary.styles.scss:1
  • Draft comment:
    Removal of duplicate inspect button styles improves component encapsulation; ensure that the inspect button is only styled via its dedicated component.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.styles.scss:24
  • Draft comment:
    Good scoping of the .inspect-metric-button style inside the MetricDetails header. This keeps the style component-specific and avoids accidental global overrides.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. frontend/src/container/MetricsExplorer/Summary/Summary.styles.scss:302
  • Draft comment:
    Removal of the global .inspect-metric-button styles is appropriate since the button is now specific to MetricDetails. This cleanup prevents potential styling conflicts.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_XczO28S4CUCZAA25


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

Copy link
Copy Markdown
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 2f8a75e in 1 minute and 35 seconds

More details
  • Looked at 57 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. frontend/src/container/MetricsExplorer/Inspect/Inspect.tsx:34
  • Draft comment:
    Avoid inline styles on the Drawer. Move styles (e.g., overscrollBehavior and background) to external CSS 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.
2. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:125
  • Draft comment:
    Avoid inline styles on the Drawer; consider moving these to an external stylesheet.
  • 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/MetricsExplorer/Inspect/Inspect.tsx:34
  • Draft comment:
    Avoid inline styles on the Drawer. Consider moving dynamic properties (background and overscrollBehavior) to external CSS 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.
4. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:125
  • Draft comment:
    Avoid using inline styles for the Drawer (background and overscrollBehavior). Use external CSS (with CSS variables if needed) 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.
5. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:56
  • Draft comment:
    The useMemo for 'showInspectFeature' has an empty dependency array. If the feature flag in local storage might change during runtime, consider updating dependencies so the flag reflects dynamic changes.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
6. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:110
  • Draft comment:
    Good addition of the aria-label 'Inspect Metric' to the Button for improved accessibility.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
7. frontend/src/container/MetricsExplorer/Inspect/Inspect.tsx:23
  • Draft comment:
    Consistent renaming from 'inspect-metric-button' to 'inspect-metrics-button' improves clarity.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
8. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:107
  • Draft comment:
    There appears to be a typographical error in the comment. Instead of 'Show the based on the feature flag. Will remove before releasing the feature', consider clarifying the message (e.g., 'Show the button based on the feature flag. Will remove before releasing the feature').
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_SuVkKEiAIrzuxdkH


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

@amlannandy amlannandy force-pushed the feat/inspect-metrics/base-setup branch from 2f8a75e to c751778 Compare March 31, 2025 13:27
Comment thread frontend/src/container/MetricsExplorer/Inspect/utils.tsx Outdated
SagarRajput-7
SagarRajput-7 previously approved these changes Apr 1, 2025
@amlannandy amlannandy force-pushed the feat/inspect-metrics/base-setup branch from 552d9d5 to 24c8291 Compare April 1, 2025 11:42
Copy link
Copy Markdown
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 552d9d5 in 56 seconds

More details
  • Looked at 26 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. frontend/src/container/MetricsExplorer/Inspect/constants.ts:1
  • Draft comment:
    Good use of a constant for the feature flag key to avoid duplication.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. frontend/src/container/MetricsExplorer/Inspect/utils.tsx:9
  • Draft comment:
    Consider wrapping localStorage access in a try/catch block to handle exceptions in environments where localStorage may be inaccessible (e.g., server-side rendering or private mode).
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. frontend/src/container/MetricsExplorer/Inspect/constants.ts:1
  • Draft comment:
    Good use of a constant for the feature flag key. Consider adding a brief JSDoc comment for clarity if this constant might be used in multiple contexts.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. frontend/src/container/MetricsExplorer/Inspect/utils.tsx:6
  • Draft comment:
    There's an extra space in the comment ('in metrics explorer'). Please remove the extra space for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. frontend/src/container/MetricsExplorer/Inspect/utils.tsx:9
  • Draft comment:
    Consider adding a check to verify that 'localStorage' is available before accessing it, to avoid issues in non-browser contexts.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
6. frontend/src/container/MetricsExplorer/Inspect/utils.tsx:6
  • Draft comment:
    There's an extra space between 'in' and 'metrics' on line 6. Consider removing the extra space to maintain consistency.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_gXrtekqNWmfnqPH3


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

@amlannandy amlannandy enabled auto-merge (squash) April 1, 2025 11:43
@amlannandy amlannandy merged commit a4ed9e4 into main Apr 1, 2025
12 of 14 checks passed
@amlannandy amlannandy deleted the feat/inspect-metrics/base-setup branch April 1, 2025 11:47
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.

Base setup for inspect feature

2 participants