Drop some comments and logging #8073
Conversation
| @@ -18,8 +18,6 @@ | |||
| import React, { Suspense } from "react"; | |||
| import { type DocumentViewProps } from "./DocumentViewProps"; | |||
|
|
|||
| // Dynamic import because documentView has a transitive dependency of react-shadow-root which assumed a proper | |||
| // `window` variable is present on module load. This isn't available on header generation | |||
There was a problem hiding this comment.
Comment related to "headers" generation. We don't have such restrictions anymore
| @@ -130,7 +130,6 @@ let modActivationPanelEntry: ModActivationPanelEntry | null = null; | |||
| * Attach the sidebar to the page if it's not already attached. Then re-renders all panels. | |||
| */ | |||
| export async function showSidebar(): Promise<void> { | |||
| console.debug("sidebarController:showSidebar"); | |||
There was a problem hiding this comment.
These logs are super old, we should try to remove old logs on parts of the extension that are stable and are no longer monitoring.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8073 +/- ##
=======================================
Coverage 73.19% 73.20%
=======================================
Files 1310 1310
Lines 40737 40729 -8
Branches 7567 7567
=======================================
- Hits 29818 29816 -2
+ Misses 10919 10913 -6 ☔ View full report in Codecov by Sentry. |
| const data = (await runPipeline(body, { | ||
| key: "body", | ||
| counter: 0, | ||
| })) as JsonObject; |
There was a problem hiding this comment.
Async-await code is easier to follow, and shorter if we account for the eslint-disable comments.
Here for example it makes the unknown as JsonObject assertion visible.
There was a problem hiding this comment.
Should we consider making runPipeline accept a generic instead of casting?
There was a problem hiding this comment.
That would be OK for now (e.g., TReturn). This type is actually wrong, I think the correct type is JsonValue. It just doesn't matter in practice because we pass it around as-is
The future change toRunPipeline would be to make the type TReturn extends JsonValue
|
Adding @fungairino as reviewer/FYI because he's working on Playwright test infrastructure for the sidebar |
| @@ -82,7 +82,7 @@ const PanelContent: React.FC = () => { | |||
| return () => { | |||
| navigationEvent.remove(onNavigation); | |||
| }; | |||
| }, [navigationEvent, onNavigation]); | |||
| }, [onNavigation]); | |||
There was a problem hiding this comment.
The exhaustive-deps rule was warning on this line. navigationEvent never changes because it's "global"
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
|
cc @fungairino -- Playwright failure |
|
Looks like e2e tests caught a regression in this PR. The admin console does not open automatically after the extension is done installing (we expect it to open in a new tab to automatically link the extension). I'm going to open a separate PR to add a timeout to the await page call that we make in the e2e setup code so it's more clear what it fails on. |
fregante
force-pushed
the
F/clean/comments
branch
from
2e4b563 to
3d2b2cd
Compare
|
Got it, sidebarDomControllerLite was imported by the MV3 background, which lacks a |
What does this PR do?
prefer-await-to-then-related eslint commentsChecklist