fix(core): strip trailing periods from error URLs#28069
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where URLs included in error messages were incorrectly terminated with a trailing period, rendering them invalid. The changes implement a regex-based utility to detect and clean these URLs while ensuring that standard sentence-ending periods remain unaffected. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/S
|
|
You already have 7 pull requests open. Please work on getting existing PRs merged before opening more. |
There was a problem hiding this comment.
Code Review
This pull request introduces a utility function stripTrailingPeriodsFromUrls to remove trailing periods from URLs in error messages, and applies it within getErrorMessage. It also adds corresponding unit tests. The review feedback highlights a correctness issue with the regular expression when URLs are wrapped in parentheses or brackets, which can cause closing brackets to be swallowed or periods inside parentheses to be missed. It suggests a more robust regex and additional test cases to cover these edge cases.
| function stripTrailingPeriodsFromUrls( | ||
| message: string | null | undefined, | ||
| ): string { | ||
| return message?.replace(/(https?:\/\/\S+)\.(?=\s|$)/g, '$1') ?? ''; |
There was a problem hiding this comment.
The current regular expression /(https?:\/\/\S+)\.(?=\s|$)/g has a correctness issue when URLs are wrapped in parentheses or brackets, such as (https://google.com). or [https://google.com]..
Because \S+ is greedy and matches closing brackets/parentheses, it will swallow the closing parenthesis/bracket into the URL group $1 and strip the trailing period, resulting in "Go to (https://google.com)" (where the URL now incorrectly ends with a parenthesis). Conversely, for a URL like (https://google.com.) where the period is inside the parenthesis, the regex fails to match because the period is followed by ) (which is not whitespace or end of string).
We can make the regex more robust by:
- Excluding closing brackets/parentheses from the URL match group using
[^\s)\]}]+. - Allowing optional closing brackets/parentheses in the lookahead assertion:
(?=[)\]}]?(?:\s|$)).
This ensures that periods outside parentheses are preserved (without swallowing the parenthesis), and periods inside parentheses are correctly stripped.
| return message?.replace(/(https?:\/\/\S+)\.(?=\s|$)/g, '$1') ?? ''; | |
| return message?.replace(/(https?:\/\/[^\s)\]}]+)\.(?=[)\]}]?(?:\s|$))/g, '$1') ?? ''; |
| it('should remove a trailing period from URLs in error messages', () => { | ||
| const error = new Error('Migrate to https://antigravity.google.'); | ||
|
|
||
| expect(getErrorMessage(error)).toBe( | ||
| 'Migrate to https://antigravity.google', | ||
| ); | ||
| }); | ||
|
|
||
| it('should preserve periods that do not trail a URL', () => { | ||
| expect(getErrorMessage(new Error('Authentication failed.'))).toBe( | ||
| 'Authentication failed.', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Add comprehensive test cases to verify the behavior of URLs wrapped in parentheses or brackets (both with periods inside and outside the brackets/parentheses) to ensure the regex handles these edge cases correctly and doesn't swallow closing brackets.
it('should remove a trailing period from URLs in error messages', () => {
const error = new Error('Migrate to https://antigravity.google.');
expect(getErrorMessage(error)).toBe(
'Migrate to https://antigravity.google',
);
});
it('should handle URLs wrapped in parentheses or brackets correctly', () => {
expect(getErrorMessage(new Error('Go to (https://google.com.)'))).toBe(
'Go to (https://google.com)',
);
expect(getErrorMessage(new Error('Go to (https://google.com).'))).toBe(
'Go to (https://google.com).',
);
expect(getErrorMessage(new Error('Go to [https://google.com.]'))).toBe(
'Go to [https://google.com]',
);
expect(getErrorMessage(new Error('Go to [https://google.com].'))).toBe(
'Go to [https://google.com].',
);
});
it('should preserve periods that do not trail a URL', () => {
expect(getErrorMessage(new Error('Authentication failed.'))).toBe(
'Authentication failed.',
);
});
Closes #28052