Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ describe('NotificationSettingsByType', () => {
'continuous-profiling-billing',
'seer-billing',
'logs-billing',
'expose-category-trace-metric-byte',
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.

Positive test missing assertion for Metrics (Bytes) visibility

Low Severity

The positive test case adds expose-category-trace-metric-byte to the features list but never asserts that Metrics (Bytes) is visible. Every other feature-gated category (profile hours, seer budget, logs, active contributors) has a corresponding getByText assertion in this test. The negative test at line 530 also passes vacuously because isBilledCategory is false for TRACE_METRIC_BYTE in DATA_CATEGORY_INFO, meaning the field quotaTraceMetricBytes is never generated by CATEGORY_QUOTA_FIELDS — so the feature flag gating is never actually exercised by either test.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c3f80d6. Configure here.

'seer-user-billing-launch',
],
});
Expand Down Expand Up @@ -501,6 +502,7 @@ describe('NotificationSettingsByType', () => {
// No continuous-profiling-billing feature
// No seer-billing feature
// No logs-billing feature
// No expose-category-trace-metric-byte feature
],
});
renderComponent({
Expand All @@ -525,6 +527,7 @@ describe('NotificationSettingsByType', () => {
expect(screen.queryByText('Transactions')).not.toBeInTheDocument();
expect(screen.queryByText('Seer Budget')).not.toBeInTheDocument();
expect(screen.queryByText('Logs')).not.toBeInTheDocument();
expect(screen.queryByText('Metrics (Bytes)')).not.toBeInTheDocument();
expect(screen.queryByText('Active Contributors')).not.toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,10 @@ export function NotificationSettingsByType({notificationType}: Props) {
organization.features?.includes('logs-billing')
);

const hasTraceMetricsBilling = organizations.some(organization =>
organization.features?.includes('expose-category-trace-metric-byte')
);

const hasSeerUserBilling = organizations.some(organization =>
organization.features?.includes('seer-user-billing-launch')
);
Expand Down Expand Up @@ -282,6 +286,9 @@ export function NotificationSettingsByType({notificationType}: Props) {
if (field.name.startsWith('quotaLogBytes') && !includeLogs) {
return false;
}
if (field.name.startsWith('quotaTraceMetricBytes') && !hasTraceMetricsBilling) {
return false;
}
Comment on lines +289 to +291
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 feature flag gate for quotaTraceMetricBytes is dead code because the field is never generated due to its data category having isBilledCategory: false.
Severity: MEDIUM

Suggested Fix

To make the feature functional, change isBilledCategory to true for the TRACE_METRIC_BYTE data category in constants/index.tsx. Additionally, add a test case that asserts the 'Metrics (Bytes)' option is visible when the feature flag is enabled.

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/views/settings/account/notifications/notificationSettingsByType.tsx#L289-L291

Potential issue: The feature flag gate intended to control the visibility of the
'Metrics (Bytes)' notification setting will never execute. The field it checks for,
`quotaTraceMetricBytes`, is never generated because the field generation logic filters
for data categories with `isBilledCategory: true`. However, the `TRACE_METRIC_BYTE`
category is configured with `isBilledCategory: false`, causing it to be filtered out
before the feature flag check is ever reached. As a result, the feature is
non-functional, and the setting will not appear for users even when the flag is enabled.
This is corroborated by a missing assertion in the test suite for the positive case
where the flag is enabled.

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

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.

will be added in a separate PR i'm gating it first

if (field.name.startsWith('quotaSeerUsers') && !hasSeerUserBilling) {
return false;
}
Expand Down
Loading