Skip to content

New options page to add domain regexes to a whitelist#5

Merged
pqn merged 10 commits into
Exafunction:mainfrom
li-xin-yi:main
May 6, 2023
Merged

New options page to add domain regexes to a whitelist#5
pqn merged 10 commits into
Exafunction:mainfrom
li-xin-yi:main

Conversation

@li-xin-yi

Copy link
Copy Markdown
Contributor

I really appreciate this project and look forward to a new version that I can use on more websites more flexibly. I attempted to modify a few lines of code as below:

  1. Refactor the options page by React, and support users to edit their own domain lists on which Codeium is allowed to complete code. The default list is the following 11 regular expressions, extracted from script.ts
https:\/\/colab.research\.google\.com\/.*
https:\/\/(.*\.)?stackblitz\.com\/.*
https:\/\/(.*\.)?deepnote\.com\/.*
https:\/\/(.*\.)?(databricks\.com|azuredatabricks\.net)\/.*
https:\/\/(.*\.)?quadratichq\.com\/.*
https?:\/\/(.*\.)?jsfiddle\.net(\/.*)?
https:\/\/(.*\.)?codepen\.io(\/.*)?
https:\/\/(.*\.)?codeshare\.io(\/.*)?
https:\/\/console\.paperspace\.com\/.*\/notebook\/.*
https?:\/\/www\.codewars\.com(\/.*)?
https:\/\/(.*\.)?github\.com(\/.*)?

image

  1. Test the feature on some web pages with the online editor, e.g., leetcode.com, codepod.io, and some web apps I deployed on local env with URL starting by localhost. It works when I write a correct regex for them. And also in reverse, removing some default regexes above can disable the code completion of Codeium on those websites.
  2. Though it extends the application scenarios for Codeium further, it may not work on many websites even if they apply Monaco or CodeMirror as their online editors (for example, online vs code/github.dev, codesandbox). The rules are too rough to support them well for now.

@pqn pqn requested a review from khou22 April 27, 2023 19:59
@pqn

pqn commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

Though it extends the application scenarios for Codeium further, it may not work on many websites even if they apply Monaco or CodeMirror as their online editors (for example, online vs code/github.dev, codesandbox). The rules are too rough to support them well for now.

We have an internal issue tracking github.dev -- this would require a modification of our vscode extension to support the web environment.

I've previously looked at CodeSandbox. I think it doesn't work for a similar reason as described in #4 (comment)

@pqn pqn left a comment

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.

Really appreciate the PR! I just left a few comments.

Comment thread src/popup.ts

document.getElementById('go-to-options')?.addEventListener('click', () => {
chrome.runtime.openOptionsPage();
chrome.tabs.create({ url: 'chrome://extensions/?options=' + chrome.runtime.id });

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.

What's the reason for this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the options page is large enough to take up one whole tab, so use this link to go to the extension detail page and pop up the options window.

Comment thread src/script.ts Outdated
if (host.test(window.location.href)) {
// the url matches the whitelist
addMonacoInject();
addCodeMirrorInject();

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.

Is it possible that both addCodeMirrorInject and addCodeMirror5Inject will run on the same site?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I tried on Code Share and it gave completion twice. Thanks for pointing the bug out.

Comment thread src/component/Options.tsx Outdated
Comment thread src/component/Options.tsx
@li-xin-yi

Copy link
Copy Markdown
Contributor Author

@pqn

Thank you so much for reviewing this PR and valuable suggestions. I pushed some new commits to complete:

  1. Change the font of domain inputs to "monospace"
  2. Validate the domain inputs, add toast notification on success/error when clicking on the save button
  3. Fix the duplicated injection issue

Can you take a look and give more feedback?

@li-xin-yi li-xin-yi requested a review from pqn April 30, 2023 05:07

@pqn pqn left a comment

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.

Thanks, left some additional comments.

Comment thread src/component/Options.tsx Outdated
useEffect(() => {
(async () => {
const whitelist = await getStorageItem('whitelist');
setText(whitelist.join('\n'));

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.

Prefer calling this allowlist instead of whitelist everywhere.

Comment thread static/options.html Outdated
<script src="options.js"></script>
</body>
</html>
</html> No newline at end of file

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.

Merge/rebase on main and some additional CI checks will run. I think they might complain about missing \n here.

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.

Looks like we still need the merge/rebase (CI is still failing)

Comment thread src/component/Options.tsx Outdated
@@ -0,0 +1,183 @@
import React, { createRef, useEffect, useRef, useState } from 'react';

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.

image

Visually this part seems a bit crowded. Maybe we can do without the icon and the redundant "Domain Regexes" text.

Comment thread src/component/Options.tsx Outdated
textTransform: 'none',
}}
>
Save <SaveAltIcon />

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.

It's ambiguous from the UI if the Save button will also save the token field or not.

Comment thread src/script.ts Outdated
if (pattern.pattern.test(window.location.href)) {
let injectCodeMirror = false;

const addCodeMirrorInject = () =>

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.

Let's call this one addCodeMirror5GlobalInject and the other one addCodeMirror5LocalInject, since both only work on CodeMirror 5.

@pqn pqn left a comment

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.

I forgot to mention earlier, but let's have a Reset button for the regex field to restore the default settings.

Comment thread src/component/Options.tsx Outdated
/>
</Typography>
<Typography variant="body2">
Domains to allow auto-completion, double-click to edit, use regex format

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.

Suggested change
Domains to allow auto-completion, double-click to edit, use regex format
Domains to allow auto-completion. Use one regex per line.

Comment thread src/component/Options.tsx Outdated
<Button
variant="text"
onClick={() => {
const lst = text.split('\n').map((x) => x.trim());

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.

Filter empty strings (so we can tolerate newlines as spacing).

@li-xin-yi

Copy link
Copy Markdown
Contributor Author

@pqn Thanks for your comment. I updated the code as you suggested. Now, the new UI looks like:

image

I'm uncertain whether it should be "allow list" or "allowed list" (since I'm not a native English speaker...)

Comment thread src/component/Options.tsx Outdated

return (
<>
<Typography variant="h6">Allow List</Typography>

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.

Suggested change
<Typography variant="h6">Allow List</Typography>
<Typography variant="h6">Allowlist</Typography>

(Similar changes elsewhere.)

Comment thread src/component/Options.tsx Outdated
textTransform: 'none',
}}
>
Save the Allow List

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.

Suggested change
Save the Allow List
Save Allowlist

@pqn

pqn commented May 4, 2023

Copy link
Copy Markdown
Contributor

Looks like the rebase or merge had some issue and the commits are showing up in the PR history.

@li-xin-yi

Copy link
Copy Markdown
Contributor Author

Update: add the reset button and remove the exit button.

@pqn pqn left a comment

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.

One small change I missed, otherwise this looks good to merge!

Comment thread src/script.ts Outdated
}

export const OMonacoSite = {
UNSPECIFIED: 0,

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.

Sorry, to be more specific, let's keep UNSPECIFIED = 0 and add CUSTOM as a new value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I recovered it.

@pqn pqn merged commit a0e7f30 into Exafunction:main May 6, 2023
@pqn

pqn commented May 6, 2023

Copy link
Copy Markdown
Contributor

Thanks! 🎉

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.

2 participants