Skip to content

None send warnings to vscode bridge for analytics - #1882

Open
ccallcottstevens wants to merge 5 commits into
mainfrom
NONE-send-warnings-to-vscode-bridge-for-analytics
Open

None send warnings to vscode bridge for analytics#1882
ccallcottstevens wants to merge 5 commits into
mainfrom
NONE-send-warnings-to-vscode-bridge-for-analytics

Conversation

@ccallcottstevens

@ccallcottstevens ccallcottstevens commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

What Is This Change?

I want to capture how often in the frontend users are hitting the rate limit warning so I am sending this metric through to Jira.

How Has This Been Tested?

Basic checks:

  • npm run lint
  • npm run test

Advanced checks:

  • If Atlassian employee & Bitbucket changes: did you test with DC in mind? See Instructions

Recommendations:

  • Update the CHANGELOG if making a user facing change

Rovo Dev code review: Rovo Dev is reviewing this pull request…
Refresh the page in a few minutes to see the results.

Comment thread src/rovo-dev/rovoDevChatProvider.ts Outdated
Comment on lines +1209 to +1210
const haystack = `${response.title ?? ''} ${response.message ?? ''}`.toLowerCase();
return haystack.includes('rate limit');

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.

🔥 Code Bugs

Concatenating title and message with a space before searching can produce false positives when the phrase "rate limit" spans both fields (e.g. title="current rate", message="limit hit"); check each field independently instead: response.title?.toLowerCase().includes('rate limit') || response.message?.toLowerCase().includes('rate limit').

Details

📖 Explanation: The space-joined concatenation means a title ending in "rate" and a message beginning with "limit" would incorrectly match "rate limit".

Suggested change
const haystack = `${response.title ?? ''} ${response.message ?? ''}`.toLowerCase();
return haystack.includes('rate limit');
// Check title and message independently to avoid false positives when "rate" and "limit"
// are split across the two fields (e.g. title="current rate", message="limit hit").
return (response.title ?? '').toLowerCase().includes('rate limit') ||
(response.message ?? '').toLowerCase().includes('rate limit');

Uses AI. Verify results. Give Feedback


private isRateLimitWarning(response: { title?: string; message?: string }): boolean {
return (
!!response.title?.toLowerCase().includes('rate limit') ||

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.

⚠️ Maintainability - Best Practices

Consider extracting the string 'rate limit' into a constant to avoid magic strings and improve maintainability.

Details

📖 Explanation: Using a named constant makes the code more maintainable and reduces the risk of typos when the same string is used in multiple places.

Uses AI. Verify results. Give Feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant