Skip to content

ref(nav) use topbar slot for feedback button#112443

Merged
JonasBa merged 11 commits intomasterfrom
jb/nav/settingspageheader
Apr 9, 2026
Merged

ref(nav) use topbar slot for feedback button#112443
JonasBa merged 11 commits intomasterfrom
jb/nav/settingspageheader

Conversation

@JonasBa
Copy link
Copy Markdown
Member

@JonasBa JonasBa commented Apr 8, 2026

Migrates the use of feedback buttons inside page headers to the Slot inside TopBar. When components are rendered into our feedback slot, they replace the fallback Feedback button with the provided component or in this case, a customized feedback button that allows).

Users get an identical experience while developers get an entry point into being able to customize the slot. It is advisable to not set any size prop here and override the default button size provided by the SizeContext from our TopBar component and instead focus on the only replacing the button tags, text or other attributes.

Before
CleanShot 2026-04-08 at 03 17 12@2x

After
CleanShot 2026-04-08 at 03 17 22@2x

Fix DE-1069

JonasBa and others added 3 commits April 7, 2026 22:00
Tracks 33 sites where FeedbackButton needs to be wrapped in a
TopBar.Slot when the page frame feature flag is active, so the
top navigation bar can own button placement in that context.

Co-Authored-By: Claude <noreply@anthropic.com>
When the page-frame feature flag is active, FeedbackButton instances
at the top of pages are rendered into the TopBar via a slot instead of
inline, letting the top navigation bar own their placement.

Migrates 33 sites across alerts, explore, insights, profiling,
replays, releases, dashboards, issue details, trace views, preprod,
settings, and automations.

Co-Authored-By: Claude <noreply@anthropic.com>
… migrate subscriptionSettingsLayout

Inside TopBar.Slot, the TopBar owns button sizing, so the size prop
on FeedbackButton is redundant. Remove it from all 11 migrated files.

Also migrate subscriptionSettingsLayout.tsx to use TopBar.Slot when
the page-frame feature is active.

Co-Authored-By: Claude <noreply@anthropic.com>
@JonasBa JonasBa requested review from a team as code owners April 8, 2026 01:22
@linear-code
Copy link
Copy Markdown

linear-code bot commented Apr 8, 2026

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 8, 2026
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Size prop swapped to wrong conditional branch
    • Moved size='xs' prop from TopBar.Slot branch (line 188) to fallback branch (line 203) to match the correct pattern where TopBar.Slot inherits size from SizeContext and fallback preserves original size='xs' behavior.

Create PR

Or push these changes by commenting:

@cursor push db37bc00b0
Preview (db37bc00b0)
diff --git a/static/app/views/performance/newTraceDetails/traceSummary.tsx b/static/app/views/performance/newTraceDetails/traceSummary.tsx
--- a/static/app/views/performance/newTraceDetails/traceSummary.tsx
+++ b/static/app/views/performance/newTraceDetails/traceSummary.tsx
@@ -185,7 +185,6 @@
           {hasPageFrameFeature ? (
             <TopBar.Slot name="feedback">
               <FeedbackButton
-                size="xs"
                 feedbackOptions={{
                   messagePlaceholder: t(
                     'How can we make the trace summary better for you?'
@@ -201,6 +200,7 @@
             </TopBar.Slot>
           ) : (
             <FeedbackButton
+              size="xs"
               feedbackOptions={{
                 messagePlaceholder: t(
                   'How can we make the trace summary better for you?'

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

</SmallerHeaderContent>
<StyledHeaderActions>
<FeedbackButton />
{hasPageFrameFeature ? (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: When hasPageFrameFeature is true, the FeedbackButton is rendered as an icon-only button without an aria-label, making it inaccessible to screen reader users.
Severity: MEDIUM

Suggested Fix

In all 10 modified files, add an aria-label to the FeedbackButton component when it is rendered as an icon-only button. For example: <FeedbackButton aria-label={t('Give Feedback')}>.... This will provide an accessible name for screen readers, matching the pattern already used for the fallback button in topBar.tsx.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/profiling/continuousProfileHeader.tsx#L57

Potential issue: When the `page-frame` feature flag is enabled, `FeedbackButton`
components are rendered inside `TopBar.Slot` with `{null}` as children. This creates an
icon-only button that lacks an accessible name, as no `aria-label` or visually-hidden
text is provided. This violates WCAG accessibility standards because users with screen
readers will hear the element announced as just a "button" without any context about its
purpose. This accessibility regression is present in all 10 files modified by the pull
request.

Did we get this right? 👍 / 👎 to inform future reviews.

JonasBa and others added 2 commits April 8, 2026 03:31
…evel consts

Both branches of the hasPageFrameFeature ternary had identical
feedbackOptions objects. Extract them to a const before the component
so the props are only defined once.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment on lines +95 to +100
{hasPageFrameFeature ? (
<TopBar.Slot name="feedback">
<FeedbackButton>{null}</FeedbackButton>
</TopBar.Slot>
) : (
<FeedbackButton />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The FeedbackButton is rendered as an icon-only button without an aria-label, making it inaccessible to screen readers.
Severity: MEDIUM

Suggested Fix

Add aria-label={t('Give Feedback')} to all instances of FeedbackButton that are rendered inside TopBar.Slot. This will provide an accessible name for screen readers, consistent with the pattern used for the fallback button in topBar.tsx.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/profiling/profileHeader.tsx#L95-L100

Potential issue: The `FeedbackButton` component is rendered inside `TopBar.Slot` with
`children={null}`. This configuration creates an icon-only button. However, no
`aria-label` is provided as a prop. The underlying button component uses `aria-label` or
string `children` to create an accessible name. Since both are missing, the rendered
button has no accessible name, making it unusable for screen reader users. This
accessibility issue is present in all 10 files where the `FeedbackButton` was migrated
to use the `TopBar.Slot` pattern.

},
}}
/>
{hasPageFrameFeature ? (
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cursoragent return null in this case and omit the FeedbackButton from rendering when hasPageFrameFeature evaluates to true so that we don't end up with duplicates, a single button is enough

Copy link
Copy Markdown
Contributor

@cursor cursor bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! The fix has been successfully applied. The change I made:

Before:

{feedback && (
  <Flex justify="end" marginTop="xl">
    {hasPageFrameFeature ? (
      <TopBar.Slot name="feedback">
        <FeedbackButton feedbackOptions={traceSummaryFeedbackOptions}>
          {null}
        </FeedbackButton>
      </TopBar.Slot>
    ) : (
      <FeedbackButton feedbackOptions={traceSummaryFeedbackOptions} />
    )}
  </Flex>
)}

After:

{feedback && !hasPageFrameFeature && (
  <Flex justify="end" marginTop="xl">
    <FeedbackButton feedbackOptions={traceSummaryFeedbackOptions} />
  </Flex>
)}

Summary

I've successfully addressed the PR comment by modifying traceSummary.tsx to return null (not render the feedback button) when hasPageFrameFeature is true. This prevents duplicate feedback buttons from appearing in the TopBar.

The issue was that both TraceMetaDataHeader and TraceSummarySection were trying to populate the same "feedback" slot in the TopBar when the page frame feature was enabled. Now, when the feature is enabled, only the header's feedback button will be rendered, avoiding the duplication.

The change has been committed and pushed to the jb/nav/settingspageheader branch.

Open in Web Open in Cursor 

…void duplicate feedback buttons

Co-authored-by: Jonas <JonasBa@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@nsdeschenes nsdeschenes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question regarding the adding of null children, and one missed combination of feedback options 👀

<FeedbackButton />
{hasPageFrameFeature ? (
<TopBar.Slot name="feedback">
<FeedbackButton>{null}</FeedbackButton>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this null child needed, or can it just be a self-closing tag?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we need it. If you check <FeedbackButton, it checks for props.children === undefined and initializes a default label otherwise 😢

Comment on lines +157 to +180
feedbackOptions={{
messagePlaceholder: t(
'Please provide feedback on the issue Sentry detected.'
),
tags: {
['feedback.source']: feedbackSource,
},
}}
>
{null}
</FeedbackButton>
</TopBar.Slot>
) : (
<FeedbackButton
aria-label={t('Give feedback on the issue Sentry detected')}
size="xs"
feedbackOptions={{
messagePlaceholder: t(
'Please provide feedback on the issue Sentry detected.'
),
tags: {
['feedback.source']: feedbackSource,
},
}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Seems like the combining of feedback options got missed here 👀

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cursoragent assign these options to a const and spread them over each instance

Copy link
Copy Markdown
Contributor

@cursor cursor bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! The refactoring is complete. I've successfully extracted the duplicated feedback options to a const and now both instances of FeedbackButton spread the same feedbackOptions object.

Summary

I've addressed the PR comment by refactoring the issue details header to eliminate code duplication:

Changes made:

  1. Created a feedbackOptions const (lines 97-102) containing the messagePlaceholder and tags configuration
  2. Updated both FeedbackButton instances (lines 164 and 173) to use feedbackOptions={feedbackOptions} instead of duplicating the entire object

Before:

  • The feedback options were duplicated in two places (lines 157-164 and 173-180)

After:

  • A single feedbackOptions const is defined once and spread over both instances
  • Cleaner, more maintainable code with no duplication

The changes have been committed and pushed to the jb/nav/settingspageheader branch.

Open in Web Open in Cursor 

…uplication

Co-authored-by: Jonas <JonasBa@users.noreply.github.com>
Co-authored-by: Jonas <JonasBa@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 59185a6. Configure here.

…nd monitor buttons

Co-authored-by: Jonas <JonasBa@users.noreply.github.com>
@JonasBa JonasBa merged commit 36f46b0 into master Apr 9, 2026
65 checks passed
@JonasBa JonasBa deleted the jb/nav/settingspageheader branch April 9, 2026 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants