DX - Refactor some of the sidebar frame function names to clarify behavior, and fix sidebar-sub-frame console error #8475

BLoe · 2024-05-17T21:00:11Z

What does this PR do?

Closes Target error (benign?) in console for sidebar #8471
Closely related to the discussion here with Federico & Todd about sidebar frames: #7228: Add support for sidePanel in the MV3 build #7354 (comment)
Wraps the call to webext-messenger getConnectedTarget() in a try-catch to avoid console errors when it's called in a sub-frame in the sidebar
Eventually we should implement communication between the top sidebar frame and the sub-frames so that they can also identify the currently connected browser tab to the sidebar

Checklist

Add jest or playwright tests and/or storybook stories
Designate a primary reviewer

…d to sub-frames

codecov · 2024-05-17T21:10:21Z

Codecov Report

Attention: Patch coverage is 40.74074% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 73.41%. Comparing base (6d95047) to head (41148d3).
Report is 80 commits behind head on main.

Files	Patch %	Lines
src/contrib/automationanywhere/aaFrameProtocol.ts	0.00%	7 Missing ⚠️
src/sidebar/connectedTarget.tsx	55.55%	4 Missing ⚠️
src/errors/contextInvalidated.ts	50.00%	1 Missing ⚠️
src/sidebar/ConnectedSidebar.tsx	0.00%	1 Missing ⚠️
src/tinyPages/RestrictedUrlPopupApp.tsx	50.00%	1 Missing ⚠️
src/utils/expectContext.ts	50.00%	1 Missing ⚠️
src/utils/sidePanelUtils.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8475      +/-   ##
==========================================
- Coverage   73.47%   73.41%   -0.07%     
==========================================
  Files        1334     1352      +18     
  Lines       41259    41579     +320     
  Branches     7686     7803     +117     
==========================================
+ Hits        30316    30526     +210     
- Misses      10943    11053     +110

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-17T21:17:04Z

Playwright test results - MV2

42 passed
2 flaky
12 skipped

Details

Open report ↗︎
56 tests across 19 suites
13 minutes, 11 seconds
858f14c

Flaky tests

edgeSetup › auth.setup.ts › authenticate
chromeSetup › auth.setup.ts › authenticate

Skipped tests

chrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration
chrome › tests/extensionConsoleActivation.spec.ts › can activate a mod via url
edge › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration
edge › tests/extensionConsoleActivation.spec.ts › can activate a mod via url
chrome › tests/regressions/sidebarLinks.spec.ts › #8206: clicking links from the sidebar doesn't crash browser
edge › tests/regressions/sidebarLinks.spec.ts › #8206: clicking links from the sidebar doesn't crash browser
chrome › tests/runtime/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
edge › tests/runtime/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
chrome › tests/runtime/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
chrome › tests/runtime/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation
edge › tests/runtime/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
edge › tests/runtime/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation

github-actions · 2024-05-17T21:19:44Z

Playwright test results

55 passed
1 flaky

Details

Open report ↗︎
56 tests across 19 suites
16 minutes, 31 seconds
41148d3

Flaky tests

edge › tests/pageEditor/saveMod.spec.ts › can save a standalone trigger mod

twschiller · 2024-05-19T20:28:44Z

src/contrib/automationanywhere/aaFrameProtocol.ts

+    // in this specific case we don't want to be running the CoPilot Data
+    // initialization on a nested frame anyway, so we'll just eat the error and
+    // warn to console for now.
+    const connectedTarget = await getConnectedTarget();


Just check before the call that this is coming from the top-level sidebar frame?

From a Dev Ex perspective, I'd at least recommend clarifying in the warning message that the warning is expected for sidebar subframes

I think this can also run in a modal, right? What check exactly are you talking about?

src/contrib/automationanywhere/aaFrameProtocol.ts

twschiller

Conditionally approved - see comment on the scope of the try-catch and/or checking for top-frame status before making the call

github-actions · 2024-05-19T20:37:43Z

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.

…avior, and fix sidebar-sub-frame console error (#8475) * debug changes * make the code a bit more clear, and try-catch around the error related to sub-frames * only wrap the getConnectedTarget call in try catch * fix types * fix var --------- Co-authored-by: Ben Loe <ben@pixiebrix.com> Co-authored-by: Todd Schiller <todd.schiller@gmail.com>

Ben Loe added 3 commits May 17, 2024 11:55

debug changes

2aeaaf8

make the code a bit more clear, and try-catch around the error relate…

d771d0f

…d to sub-frames

Merge branch 'main' into dev/guard-against-empty-frame-aa

858f14c

BLoe requested review from twschiller and grahamlangford May 17, 2024 21:00

BLoe self-assigned this May 17, 2024

twschiller reviewed May 19, 2024

View reviewed changes

src/contrib/automationanywhere/aaFrameProtocol.ts Show resolved Hide resolved

twschiller self-requested a review May 19, 2024 20:34

twschiller approved these changes May 19, 2024

View reviewed changes

twschiller added this to the 2.0.0 milestone May 19, 2024

twschiller added the developer experience label May 19, 2024

Merge branch 'main' into dev/guard-against-empty-frame-aa

60138f8

vercel bot deployed to Preview May 21, 2024 00:16 View deployment

only wrap the getConnectedTarget call in try catch

d8d02d6

vercel bot deployed to Preview May 21, 2024 13:52 View deployment

Ben Loe added 2 commits May 21, 2024 10:16

fix types

fdf3e4d

fix var

41148d3

vercel bot deployed to Preview May 21, 2024 14:18 View deployment

BLoe merged commit b7f72b1 into main May 21, 2024
17 checks passed

BLoe deleted the dev/guard-against-empty-frame-aa branch May 21, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DX - Refactor some of the sidebar frame function names to clarify behavior, and fix sidebar-sub-frame console error #8475

DX - Refactor some of the sidebar frame function names to clarify behavior, and fix sidebar-sub-frame console error #8475

BLoe commented May 17, 2024

codecov bot commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024 •

edited

twschiller May 19, 2024

BLoe May 21, 2024

twschiller left a comment

github-actions bot commented May 19, 2024

DX - Refactor some of the sidebar frame function names to clarify behavior, and fix sidebar-sub-frame console error #8475

DX - Refactor some of the sidebar frame function names to clarify behavior, and fix sidebar-sub-frame console error #8475

Conversation

BLoe commented May 17, 2024

What does this PR do?

Checklist

codecov bot commented May 17, 2024 • edited

Codecov Report

github-actions bot commented May 17, 2024

Playwright test results - MV2

Details

github-actions bot commented May 17, 2024 • edited

Playwright test results

Details

twschiller May 19, 2024

Choose a reason for hiding this comment

BLoe May 21, 2024

Choose a reason for hiding this comment

twschiller left a comment

Choose a reason for hiding this comment

github-actions bot commented May 19, 2024

codecov bot commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited