Handle inline and block comments for code generation [devtools/devtools-frontend : main]
Samiya Caur (Gerrit)
Samiya Caur voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ergün Erdoğmuş
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samiya Caur (Gerrit)
Samiya Caur added 1 comment![]( "Open in Gerrit")
Updated code to only work if the cursor is at the end of the block comment
Related details
- Ergün Erdoğmuş
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ergün Erdoğmuş (Gerrit)
Ergün Erdoğmuş added 4 comments![]( "Open in Gerrit")
I have refined the implementation a bit with Jetski and here is the diff, and I think I quite liked it: https://paste.googleplex.com/6326754597797888. The only issue might be that we're not able to stub out ES modules so we might want to make `AiCodeGenerationParser` a class with a static method `getCommentText` that we can stub out in `AiCodeGenerationProvider` tests.
Especially, testing this logic in isolation and keeping the implementation separate from the generation is quite cool.
I have added some comments for that state, please give it a try and let me know what you think!
===
We can add a dedicated unit test file `AiCodeGenerationParser.test.ts` for the new module. This should test edge cases (empty files, specific whitespace scenarios, unclosed comments) in isolation, while
`AiCodeGenerationProvider.test.ts` can focus on the higher-level integration (triggering the AI generation) once `AiCodeGenerationParser`s `getCommentText` resolves to a prompt.
function getCommentNodeAtCursor(state: CodeMirror.EditorState, cursorPosition: number): CodeMirror.SyntaxNode|I think... this file is becoming quite large. Let's move all the comment parsing logic, including this function and the regex patterns, into a separate module named `AiCodeGenerationParser.ts`.
Additionally, this node resolution is too strict. If a user types a space after a block comment (e.g. `/* ... */ |`), node.to will not match the cursor position. We can update the logic to be tolerant of trailing whitespace: if the strict check fails, look at the line up to the cursor, trim the end, and check if we are checking a block comment node at that 'effective' end.
#getCommentTextAtCursor(cursorPosition: number): string|undefined {Let's refactor this method to use the new `AiCodeGenerationParser`. Inside the parser, we can break this logic down into smaller, helper functions for readability:
- `resolveCommentNode(state, cursor)`
- `extractBlockComment(state, node)`
- `extractCommentText(state, node)`
- `fallbackCommentCheck(state, cursor)`
let isComment = false;Instead of duplicating the fallback logic here (returning a boolean) vs in `#getCommentTextAtCursor` (returning a string), using the new parser module here as well.
`getCommentText` should return undefined if it's not a comment, which we can use to determine isComment. This ensures the teaser only shows up if we can actually extract text.
Related details
- Samiya Caur
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samiya Caur (Gerrit)
Samiya Caur voted and added 4 comments![]( "Open in Gerrit")
Votes added by Samiya Caur
| Commit-Queue | +1 |
I have refined the implementation a bit with Jetski and here is the diff, and I think I quite liked it: https://paste.googleplex.com/6326754597797888. The only issue might be that we're not able to stub out ES modules so we might want to make `AiCodeGenerationParser` a class with a static method `getCommentText` that we can stub out in `AiCodeGenerationProvider` tests.
Especially, testing this logic in isolation and keeping the implementation separate from the generation is quite cool.
I have added some comments for that state, please give it a try and let me know what you think!
===
We can add a dedicated unit test file `AiCodeGenerationParser.test.ts` for the new module. This should test edge cases (empty files, specific whitespace scenarios, unclosed comments) in isolation, while
`AiCodeGenerationProvider.test.ts` can focus on the higher-level integration (triggering the AI generation) once `AiCodeGenerationParser`s `getCommentText` resolves to a prompt.
Thank you for reviewing! I think I have addressed all the comments and made the required changes.
function getCommentNodeAtCursor(state: CodeMirror.EditorState, cursorPosition: number): CodeMirror.SyntaxNode|I think... this file is becoming quite large. Let's move all the comment parsing logic, including this function and the regex patterns, into a separate module named `AiCodeGenerationParser.ts`.
Additionally, this node resolution is too strict. If a user types a space after a block comment (e.g. `/* ... */ |`), node.to will not match the cursor position. We can update the logic to be tolerant of trailing whitespace: if the strict check fails, look at the line up to the cursor, trim the end, and check if we are checking a block comment node at that 'effective' end.
Makes sense, updated the logic as per this.
#getCommentTextAtCursor(cursorPosition: number): string|undefined {Let's refactor this method to use the new `AiCodeGenerationParser`. Inside the parser, we can break this logic down into smaller, helper functions for readability:
- `resolveCommentNode(state, cursor)`
- `extractBlockComment(state, node)`
- `extractCommentText(state, node)`
- `fallbackCommentCheck(state, cursor)`
Done
Instead of duplicating the fallback logic here (returning a boolean) vs in `#getCommentTextAtCursor` (returning a string), using the new parser module here as well.
`getCommentText` should return undefined if it's not a comment, which we can use to determine isComment. This ensures the teaser only shows up if we can actually extract text.
- Ergün Erdoğmuş
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ergün Erdoğmuş (Gerrit)
Ergün Erdoğmuş added 6 comments![]( "Open in Gerrit")
describe('findLastNonWhitespacePos', () => {Since these methods are not in the public API of the module, I don't think we should test them specifically but rather we should test the public-facing API (`extractCommentText`) extensively to cover the cases where these helpers acted on. (e.g. it returns the correct comment text, when the cursor is at the end, after a tab...)
static findLastNonWhitespacePos(state: CodeMirror.EditorState, cursorPosition: number): number {Let's put the non public methods as a non static module methods and only mark the public methods as static methods (I expect only `extractCommentText` to be static, in this case)
const trimmedMatch = textBefore.match(/\S\s*$/);I think we can implement this a bit more performantly by using:
```
// Find the text before cursor
const textBeforeCursor = line.text.substring(0, cursorPosition - line.from);
// Calculate the effective cursor position for the last position of the comment text
const effectiveEnd = line.from + textBeforeCursor.trimEnd().length;
```
I'm a little afraid of running an expensive search on every keystroke once the code generation is enabled, it'd be great if you can take a look at the performance with throttling and see how it affects the writing experience.
let cleaned = rawText.replace(/^\/\*+\s*/, '').replace(/\s*\*+\/$/, '');(nit): Let's move these regex definitions out of line so that it's easier to read what we're matching. Something like:
```
const LINE_COMMENT_PATTERN = /^(?:\/\/|#)\s*/;
const BLOCK_COMMENT_START_PATTERN = /^\/\*+\s*/;
const BLOCK_COMMENT_END_PATTERN = /\s*\*+\/$/;
const BLOCK_COMMENT_LINE_PREFIX_PATTERN = /^\s*\*\s?/;
```
There might also be some little performance gains by creating the regex statically rather than dynamically here (though I also expect some optimizations on the V8 side for this).
return this.fallbackCommentCheck(state, cursorPosition);I don't expect this `catch` block to be hit often, do you see cases where it happens? I think we can simplify the implementation by removing the try-catch block and the fallback handling (or if you're convinced that `CodeMirror` operations might throw, we can keep it but return undefined for the erroneous case).
Related details
- Samiya Caur
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samiya Caur (Gerrit)
Samiya Caur voted and added 5 comments![]( "Open in Gerrit")
Votes added by Samiya Caur
| Auto-Submit | +1 |
| Commit-Queue | +1 |
5 comments
Since these methods are not in the public API of the module, I don't think we should test them specifically but rather we should test the public-facing API (`extractCommentText`) extensively to cover the cases where these helpers acted on. (e.g. it returns the correct comment text, when the cursor is at the end, after a tab...)
Done
static findLastNonWhitespacePos(state: CodeMirror.EditorState, cursorPosition: number): number {Let's put the non public methods as a non static module methods and only mark the public methods as static methods (I expect only `extractCommentText` to be static, in this case)
Done
I think we can implement this a bit more performantly by using:
```
// Find the text before cursor
const textBeforeCursor = line.text.substring(0, cursorPosition - line.from);
// Calculate the effective cursor position for the last position of the comment text
const effectiveEnd = line.from + textBeforeCursor.trimEnd().length;
```I'm a little afraid of running an expensive search on every keystroke once the code generation is enabled, it'd be great if you can take a look at the performance with throttling and see how it affects the writing experience.
Ah you are right, updated
let cleaned = rawText.replace(/^\/\*+\s*/, '').replace(/\s*\*+\/$/, '');(nit): Let's move these regex definitions out of line so that it's easier to read what we're matching. Something like:
```
const LINE_COMMENT_PATTERN = /^(?:\/\/|#)\s*/;
const BLOCK_COMMENT_START_PATTERN = /^\/\*+\s*/;
const BLOCK_COMMENT_END_PATTERN = /\s*\*+\/$/;
const BLOCK_COMMENT_LINE_PREFIX_PATTERN = /^\s*\*\s?/;
```There might also be some little performance gains by creating the regex statically rather than dynamically here (though I also expect some optimizations on the V8 side for this).
Done
return this.fallbackCommentCheck(state, cursorPosition);I don't expect this `catch` block to be hit often, do you see cases where it happens? I think we can simplify the implementation by removing the try-catch block and the fallback handling (or if you're convinced that `CodeMirror` operations might throw, we can keep it but return undefined for the erroneous case).
- Ergün Erdoğmuş
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ergün Erdoğmuş (Gerrit)
Ergün Erdoğmuş voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ergün Erdoğmuş
| Code-Review | +1 |
| Commit-Queue | +1 |
1 comment
yay, thank you for bearing with me!
Related details
- Samiya Caur
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samiya Caur (Gerrit)
Samiya Caur voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devtools-frontend LUCI CQ (Gerrit)
Devtools-frontend LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Handle inline and block comments for code generation
- M config/gni/devtools_grd_files.gni
- A front_end/ui/components/text_editor/AiCodeGenerationParser.test.ts
- A front_end/ui/components/text_editor/AiCodeGenerationParser.ts
- M front_end/ui/components/text_editor/AiCodeGenerationProvider.test.ts
- M front_end/ui/components/text_editor/AiCodeGenerationProvider.ts
- M front_end/ui/components/text_editor/BUILD.gn
- M front_end/ui/components/text_editor/text_editor.ts
Code-Review: +1 by Ergün Erdoğmuş
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |