aichat: code formatting #21342

nullhook · 2023-12-12T15:25:09Z

this PR adds formatted code UI rendering. it uses a lightweight component where syntax highlighted is limited for selected languages only.

production size:

790.62 KB  chat_ui.bundle.js // without highlighter
792.41 KB chat_ui.bundle.js // with lazy loaded highlighter
910.92 KB  chat_ui.bundle.js // with highlighter

dev size:

5.4 MB  chat_ui.bundle.js // without highlighter
5.7 MB  chat_ui.bundle.js // with highlighter

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

brave-builds · 2023-12-12T16:31:04Z

A Storybook has been deployed to preview UI for the latest push

petemill · 2023-12-13T22:37:57Z

components/ai_chat/resources/page/components/conversation_list/index.tsx

+                <div
+                  className={styles.message}
+                >
+                  <div dangerouslySetInnerHTML={{ __html: escapeHTMLPolicy.createHTML(turn.text


Can we get around the need to use innerHTML. One option would be to use your regex here to build an array of matches and then render everything in a loop

regular text block in between a match

code block text wrapped with special element or class

And for each regular block you could use the same pattern for any inline code block matches.

ok, changed. i'm building the string into parts and wrapping relevant html around it. this works, nicely.

however, i'm curious how expensive using regex is here

petemill · 2023-12-14T01:17:23Z

components/resources/ai_chat_prompts.grdp

@@ -53,6 +53,8 @@ The current time and date is <ph name="DATE">$1</ph>
 Your name is Leo, a helpful, respectful and honest AI assistant created by the company Brave. You will be replying to a user of the Brave browser. Always respond in a neutral tone. Be polite and courteous. Answer concisely in no more than 50-80 words.

 Please ensure that your responses are socially unbiased and positive in nature. If a question does not make any sense, or is not factually coherent, explain why instead of answering something not correct. If you don't know the answer to a question, please don't share false information.
+
+If you are asked to write code or json, then use this format: ```language```


How about something like "Use unicode symbols for formatting where appropriate. Use backticks (`) to wrap inline coding-related words and triple backticks (```) to wrap blocks of code or data." cc @LorenzoMinto

brave-builds · 2023-12-18T18:58:28Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-12-19T20:57:21Z

A Storybook has been deployed to preview UI for the latest push

mkarolin

strings++

socket-security · 2023-12-20T00:17:31Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@types/react-syntax-highlighter	15.5.11	None	`+0`	195 kB	types
react-syntax-highlighter	15.5.0	filesystem	`+20`	9.48 MB	simmerer

diracdeltas · 2023-12-20T00:20:43Z

@brave/devops-team why was audit-deps not automatically tagged on this PR?

wknapik · 2023-12-20T00:27:08Z

@diracdeltas the label is added automatically based on the files touched by the PR. The files modified here didn't match any of the existing patterns.

brave-builds · 2023-12-20T00:47:01Z

A Storybook has been deployed to preview UI for the latest push

diracdeltas · 2023-12-20T03:50:05Z

@diracdeltas the label is added automatically based on the files touched by the PR. The files modified here didn't match any of the existing patterns.

i don't understand, since this PR does modify package.json and package-lock.json

wknapik · 2023-12-20T12:13:57Z

@diracdeltas the label is added automatically based on the files touched by the PR. The files modified here didn't match any of the existing patterns.

i don't understand, since this PR does modify package.json and package-lock.json

Hmm, I thought the commit that touched package*.json happened after you asked the question, but actually it looks like it happened 7min before.

I think we need to set up this github action differently. It currently doesn't trigger on all the relevant events. I'll look into it.

wknapik · 2023-12-20T17:10:28Z

@diracdeltas should be fixed

brave-builds · 2023-12-28T03:12:14Z

A Storybook has been deployed to preview UI for the latest push

diracdeltas · 2023-12-28T19:26:11Z

Since this adds new deps I'm tagging @bcaller to do sec review here when he's back.

bcaller · 2024-01-05T03:21:28Z

components/ai_chat/resources/page/components/conversation_list/index.tsx

+function buildMessage(str: string) {
+  const parts: any = []
+
+  for (const match of str.matchAll(/```([^\n`]+)?\n?([\s\S]*?)```|`(.*?)`|([^`]+)/gs)) {


Note that this will start/end fenced code blocks even when the backticks don't start at the beginning of a line (not necessarily an issue). Overall, the regex seems OK.

Also note that LLMs are happy writing broken Markdown. We had some issues with Brave Search CodeLLM.

bcaller · 2024-01-05T03:56:35Z

Since this adds new deps I'm tagging @bcaller to do sec review here when he's back.

No issue with adding the dependency https://github.com/react-syntax-highlighter/react-syntax-highlighter

browser/ui/webui/ai_chat/ai_chat_ui.cc

brave-builds · 2024-01-12T17:19:23Z

A Storybook has been deployed to preview UI for the latest push

petemill · 2024-01-16T08:12:04Z

components/ai_chat/resources/page/components/conversation_list/index.tsx

+function buildMessage(str: string) {
+  const parts: any = []
+
+  for (const match of str.matchAll(/```([^\n`]+)?\n?([\s\S]*?)```|`(.*?)`|([^`]+)/gs)) {


Is it more optimal to not create this regex every time the function is run? i.e. put it at the module level?

petemill · 2024-01-16T08:12:06Z

components/ai_chat/resources/page/components/conversation_list/index.tsx

+                <div
+                  className={styles.message}
+                >
+                  {buildMessage(turn.text)}


With the amount of time this gets re-rendered, should we memoize the result of buildMessage, dependent on turn.text?

brave-builds · 2024-01-16T20:01:35Z

A Storybook has been deployed to preview UI for the latest push

petemill

If you fix the minor issues that are left, then this looks good to go from me!

petemill · 2024-01-17T06:43:19Z

components/ai_chat/resources/page/components/conversation_list/index.tsx

+  text: string
+}
+
+function FormattedTextRenderer(props: FormattedTextProps): JSX.Element {


nit: Well, now that this is so simple, you could more simply just wrap this function in React.memo

petemill · 2024-01-17T06:44:32Z

components/ai_chat/resources/page/components/conversation_list/index.tsx

 const SUGGESTION_STATUS_SHOW_BUTTON: mojom.SuggestionGenerationStatus[] = [
  mojom.SuggestionGenerationStatus.CanGenerate,
  mojom.SuggestionGenerationStatus.IsGenerating
 ]

+const regexp = /```([^\n`]+)?\n?([\s\S]*?)```|`(.*?)`|([^`]+)/gs


this name is just the type - how about something more meaningful, like codeFormatRegexp?

Also could you add a comment explaining what the regex is attempting to achieve - something about splitting the string in to groups by backtick sequences (if I'm correct about that!)

renamed and added comment

petemill · 2024-01-17T06:47:22Z

package.json

@@ -90,6 +90,7 @@
    "@types/react": "17.0.2",
    "@types/react-dom": "17.0.18",
    "@types/react-redux": "7.1.25",
+    "@types/react-syntax-highlighter": "^15.5.11",


needs to be exact version

petemill · 2024-01-17T06:47:50Z

package.json

@@ -190,6 +191,7 @@
    "react-json-view-lite": "^0.9.5",
    "react-router": "^5.2.1",
    "react-router-dom": "^5.3.0",
+    "react-syntax-highlighter": "^15.5.0",


needs to be exact version. I know some others here aren't, but it should be.

petemill · 2024-01-17T06:51:25Z

SonarCloud is wondering about this

Make sure the regex used here, which is vulnerable to super-linear runtime due to backtracking, cannot lead to denial of service.

petemill · 2024-01-17T06:51:42Z

I restarted android

brave-builds · 2024-01-17T17:14:45Z

A Storybook has been deployed to preview UI for the latest push

bcaller · 2024-01-18T01:45:04Z

SonarCloud is wondering about this

Make sure the regex used here, which is vulnerable to super-linear runtime due to backtracking, cannot lead to denial of service.

The section should be approximately quadratic:

([^\n`]+)?\n?([\s\S]*?)```

because a long string of 'A's which doesn't end in three backticks will match both the first [^\n`]+ and the [\s\S]* (and the \n? in the middle).

However, causing any slow down would require the AI to produce such string with a length > 10000 I think and then it would only be client-side and for a second or two. Double the length would quadruple the time taken to process but I'd hope the AI rarely goes that crazy.

stephendonner · 2024-01-19T01:35:30Z

Verified `PASSED` using

Brave | 1.64.3 Chromium: 121.0.6167.57 (Official Build) nightly (64-bit)
-- | --
Revision | 0ecd8a5470d06ce96921cb7585ecbc0f9878f3cf
OS | Windows 10 Version 22H2 (Build 19045.3930)

Steps:

installed 1.64.3
launched Brave using --env-leo=staging --env-ai-chat.bsg=dev --env-ai-chat-premium.bsg=dev --enable-logging=stderr --v=2
loaded cnn.com (not necessary)
clicked Summarize this page
asked a slight variation of "write me a "Hello, world!" program using/in {language}`

Confirmed output was language-appropriate, in a code block, and accompanied by a `copy code` button

`Python`

Sample code:

print("Hello, world!")

`C++`

Sample code:

#include <iostream>

int main() {
  std::cout << "Hello, world!" << std::endl;
  return 0;
}

`.NET`

`JavaScript`

* aichat: input is growable (#21124) * aichat: scroll is interruptable (#21235) * aichat: model maker text shouldnt look like a link (#21220) * aichat: code formatting (#21342) * make claude output formatted code (#21599)

* Merge pull request #21134 from brave/cr121 Upgrade from Chromium 120 to Chromium 121. * Remove the assert for patch_ffmpeg.py (#21184) * Merge pull request #21539 from brave/ffmpeg-dynamic-alloc Use dynamic allocation for ffmpeg fft tables on Windows. * Merge pull request #21585 from brave/issues/35318 Remove dynamic allocation of ffmpeg ff_tx tables. * Disables a flaky upstream browser test. * Merge pull request #21584 from brave/fix_new_tab_button_plus_misaligned Fixed new tab button's plus icon is mis-aligned with horizontal tab * Merge pull request #21600 from brave/121.0.6167.75_master Upgrade from Chromium 121.0.6167.57 to Chromium 121.0.6167.75. * Merge pull request #21628 from brave/maxk-disable-reading-mode Hides `Open in Reading Mode` context menu item. * [Uplift 1.62.x] AI chat issues cr121 1.62.x (#21629) * aichat: input is growable (#21124) * aichat: scroll is interruptable (#21235) * aichat: model maker text shouldnt look like a link (#21220) * aichat: code formatting (#21342) * make claude output formatted code (#21599) --------- Co-authored-by: Mikhail <matuchin@brave.com> Co-authored-by: Aleksey Khoroshilov <5928869+goodov@users.noreply.github.com> Co-authored-by: Simon Hong <shong@brave.com> Co-authored-by: taher <8665427+nullhook@users.noreply.github.com>

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Dec 12, 2023

github-actions bot assigned nullhook Dec 12, 2023

petemill reviewed Dec 13, 2023

View reviewed changes

petemill reviewed Dec 14, 2023

View reviewed changes

nullhook force-pushed the ai-chat-code-formatting branch from 0f932c5 to 3c0c7a9 Compare December 18, 2023 17:51

nullhook force-pushed the ai-chat-code-formatting branch from 3c0c7a9 to 746e06a Compare December 19, 2023 20:20

nullhook marked this pull request as ready for review December 19, 2023 20:21

nullhook requested a review from a team as a code owner December 19, 2023 20:21

nullhook requested a review from petemill December 19, 2023 20:21

nullhook changed the title ~~aichat: code layout~~ aichat: code formatting Dec 19, 2023

mkarolin approved these changes Dec 19, 2023

View reviewed changes

nullhook requested a review from a team as a code owner December 20, 2023 00:13

diracdeltas added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Dec 20, 2023

thypon added the needs-security-review label Dec 20, 2023

diracdeltas requested a review from thypon December 20, 2023 03:50

wknapik mentioned this pull request Dec 20, 2023

Fixed automated labeling #21433

Merged

25 tasks

wknapik closed this in #21433 Dec 20, 2023

wknapik reopened this Dec 20, 2023

github-actions bot assigned wknapik Dec 20, 2023

wknapik removed their assignment Dec 20, 2023

diracdeltas requested a review from bcaller December 28, 2023 19:25

bcaller reviewed Jan 5, 2024

View reviewed changes

nullhook force-pushed the ai-chat-code-formatting branch from 1fc5a0d to cfbf6ce Compare January 12, 2024 00:32

nullhook requested a review from petemill January 12, 2024 00:42

diracdeltas reviewed Jan 12, 2024

View reviewed changes

browser/ui/webui/ai_chat/ai_chat_ui.cc Show resolved Hide resolved

nullhook force-pushed the ai-chat-code-formatting branch from fe0a1e9 to 1caa09b Compare January 12, 2024 16:32

petemill reviewed Jan 16, 2024

View reviewed changes

nullhook force-pushed the ai-chat-code-formatting branch from c6dab91 to 9655031 Compare January 16, 2024 19:12

petemill approved these changes Jan 17, 2024

View reviewed changes

aichat: code layout

c7b4215

nullhook force-pushed the ai-chat-code-formatting branch from 9655031 to c7b4215 Compare January 17, 2024 16:35

nullhook merged commit b5f4ad0 into master Jan 17, 2024
18 checks passed

nullhook deleted the ai-chat-code-formatting branch January 17, 2024 17:38

github-actions bot added this to the 1.63.x - Nightly milestone Jan 17, 2024

petemill mentioned this pull request Jan 17, 2024

Mismatch in package-lock.json with react-syntax-highlighter brave/brave-browser#35390

Closed

nullhook added a commit that referenced this pull request Jan 18, 2024

aichat: code formatting (#21342)

598cd00

nullhook mentioned this pull request Jan 18, 2024

[Uplift 1.62.x] AI chat issues cr121 1.62.x #21629

Merged

kjozwiak mentioned this pull request Jan 20, 2024

Upgrade from Chromium 120 to Chromium 121 (1.62.x). #21593

Merged

24 tasks

aichat: code formatting #21342

aichat: code formatting #21342

Conversation

nullhook commented Dec 12, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Dec 12, 2023

petemill Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Dec 18, 2023

brave-builds commented Dec 19, 2023

mkarolin left a comment

Choose a reason for hiding this comment

socket-security bot commented Dec 20, 2023

diracdeltas commented Dec 20, 2023

wknapik commented Dec 20, 2023

brave-builds commented Dec 20, 2023

diracdeltas commented Dec 20, 2023

wknapik commented Dec 20, 2023

wknapik commented Dec 20, 2023

brave-builds commented Dec 28, 2023

diracdeltas commented Dec 28, 2023

bcaller Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

bcaller commented Jan 5, 2024

brave-builds commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Jan 16, 2024

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Jan 17, 2024

petemill commented Jan 17, 2024

brave-builds commented Jan 17, 2024

bcaller commented Jan 18, 2024 • edited Loading

stephendonner commented Jan 19, 2024 • edited Loading

Verified PASSED using

Steps:

Confirmed output was language-appropriate, in a code block, and accompanied by a copy code button

Python

C++

.NET

JavaScript

nullhook commented Dec 12, 2023 •

edited

Loading

petemill Dec 13, 2023 •

edited

Loading

bcaller Jan 5, 2024 •

edited

Loading

bcaller commented Jan 18, 2024 •

edited

Loading

stephendonner commented Jan 19, 2024 •

edited

Loading

Verified `PASSED` using

Confirmed output was language-appropriate, in a code block, and accompanied by a `copy code` button

`Python`

`C++`

`.NET`

`JavaScript`