-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: fix JCEF browser issues #7291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7da13d7
to
2487fb7
Compare
73bb1d0
to
5c3f7eb
Compare
💡 To request a new review, comment |
AI Code ReviewAI review completed but failed to parse output No specific line comments generated. 💡 To request a new detailed review, comment |
I tested all the features (actions, focusing, chat functionalities) and it seems to be OK, except for one action that doesn’t recognize the command |
I'm switching it back to draft because we need to discuss OSR (let's wait with merge). |
✅ I've enabled OSR by default and removed the option. I also made sure there are no outdated comments in the readme/contribute docs. |
5c3f7eb
to
5cc8f29
Compare
💡 To request a new review, comment |
Solves CON-2469 Solves CON-2979 Off-screen rendering is now enabled by default. This is motivated by the fact that most users will benefit from it, but the minority with issues can disable it in the Continue settings. I believe this is the correct strategy, because disabling it causes a flickering window for the majority. Note: I tried to mask it by preloading the HTML with a background color and by changing the background color of the browser component. That worked, but only reduced the flashing time and didn't completely eliminate the problem. Enabling Off-screen rendering seems to be the only robust solution. After decoupling the browser from the toolbar and moving it to a dedicated service, there is now full control over when and what is initialized. The setting is now applied correctly. I've noticed that most of the actions require a working, active Continue toolbar. I've created the `ContinueToolbarAction` abstraction to remove duplicated logic. I've also removed `actions.utils`. * Fix bug in `CoreMessenger.handleMessage` (some pass-through messages had message type used as message ID) * Add `browser` package and moved browser-related classes there, including `CustomHandlerFactory` (package `factories` is now removed). `CustomHandlerFactory` implementation is untouched. * Add `project.getBrowser()` extension method (this is mostly for readability). * Some loggers now use a shorter identifier (simple class name) without the full package, because our package name is unusually long, and it was hard to read the logs. * Remove commented-out `TerminalActivityTrackingService`. * Create `JcefErrorPanel` and put `jcef_error.html` there.
@exigow I think the test failure here is no longer flake. Might be a test that we can just remove though, lmk your thoughts. |
@Patrick-Erichsen, you're right, it was failing test (compilation error caused by changes in our browser code). 😅 I think it's better to remove it. The test was fine, but almost everything was mocked there and the test was heavily dependent on the handler's code (+ everything around). It made some sense, but ideally there should be as few mocks as possible and it should be tested in a more black-box manner. If you think we should keep it, let me know - I'll try to fix it. |
Interesting error on CI (it's from e2e test I suppose):
Test is passing locally. 😮💨 I'm investigating this right now. |
48ee225
to
2b085c1
Compare
🟢 (fix: 2b085c1) |
Solves CON-2469
Solves CON-2979
Solves #5504
Changes
Off-screen rendering is the new default
Off-screen rendering is now enabled by default. This is motivated by the fact that most users will benefit from it,
but the minority with issues can disable it in the Continue settings. I believe this is the correct strategy,
because disabling it causes a flickering window for the majority.
Note: I tried to mask it by preloading the HTML with a background color and by changing the background color of the
browser component. That worked, but only reduced the flashing time and didn't completely eliminate the problem.
Enabling Off-screen rendering seems to be the only robust solution.
Fix JS_QUERY_POOL_SIZE errors
After decoupling the browser from the toolbar and moving it to a dedicated service, there is now full control over
when and what is initialized. The setting is now applied correctly.
Rework actions
I've noticed that most of the actions require a working, active Continue toolbar.
I've created the
ContinueToolbarAction
abstraction to remove duplicated logic. I've also removedactions.utils
.Minor changes
CoreMessenger.handleMessage
(some pass-through messages had message type used as message ID)browser
package and moved browser-related classes there, includingCustomHandlerFactory
(packagefactories
is now removed).CustomHandlerFactory
implementation is untouched.project.getBrowser()
extension method (this is mostly for readability).unusually long, and it was hard to read the logs.
TerminalActivityTrackingService
.JcefErrorPanel
and putjcef_error.html
there.