#9154: mod variable session sync (2/3) #9155
Conversation
twschiller
changed the base branch from
main
to
feature/9137-variable-migration
twschiller
added
do not merge
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/9137-variable-migration #9155 +/- ##
===================================================================
- Coverage 74.83% 74.79% -0.05%
===================================================================
Files 1355 1355
Lines 41828 41882 +54
Branches 7804 7816 +12
===================================================================
+ Hits 31304 31324 +20
- Misses 10524 10558 +34 ☔ View full report in Codecov by Sentry. |
Playwright test results
Details
Failed testschrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod Flaky testsmsedge › tests/regressions/welcomeStarterBricks.spec.ts › #8740: can view the starter mods on the pixiebrix.com/welcome page Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
twschiller
changed the title
| browser.storage.onChanged.addListener(onSessionStorageChange); | ||
| } |
There was a problem hiding this comment.
Do we need to consider any cases when we should remove the listener?
There was a problem hiding this comment.
Oh, I see the note on future work in the PR description -
Delete a mod session's variables when the a mod is deactivated. That will require providing event contexts can listen to for when a mod is deactivated
I'd recommend to create a followup issue to do this work.
There was a problem hiding this comment.
I'd recommend to create a followup issue to do this work.
There will be one when the issue for the slice is created: https://www.notion.so/pixiebrix/Mod-variable-syncing-across-Page-Load-and-all-Frames-in-runtime-and-workshop-c533b56bc9ed4f6ea7aa8502439c632a?pvs=4#10143b21a25380f2a89ad449668fdaea
Also, we don't need to remove the listener. That comment is about actually deleting the data from session storage when the mod is deactivated
| const key = getSessionStorageKey(modId); | ||
| // Skip call if there are no synchronized variables | ||
| const value = await browser.storage.session.get(key); | ||
| // eslint-disable-next-line security/detect-object-injection -- key passed to .get |
There was a problem hiding this comment.
I'd like us to revisit removing this eslint rule at some point. I'm not sure there really is any practical place to consider this a security issue at the moment.
src/contentScript/stateController.ts
Outdated
| document.dispatchEvent( | ||
| new CustomEvent(STATE_CHANGE_JS_EVENT_TYPE, { | ||
| detail, | ||
| bubbles: true, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
This might be a good time to refactor creating the event detail and dispatching to a helper method.
There was a problem hiding this comment.
This might be a good time to refactor creating the event detail and dispatching to a helper method.
@grahamlangford @fungairino I'm not sure what you mean by the conversation - are you suggesting abstracting dispatch event and CustomEvent constructor? Or something else?
There was a problem hiding this comment.
Yes, I was suggesting abstracting creating the event payload and sending it in a helper method.
| modState.clear(); | ||
| framePrivateState.clear(); | ||
| frameModState.clear(); | ||
| modSyncPolicies.clear(); | ||
| } |
There was a problem hiding this comment.
This file is getting rather large... Maybe we can consider splitting up each of the types of state into their own separate modules
There was a problem hiding this comment.
Sure, can look at that in the next slice for tab-level storage https://www.notion.so/pixiebrix/Mod-variable-syncing-across-Page-Load-and-all-Frames-in-runtime-and-workshop-c533b56bc9ed4f6ea7aa8502439c632a?pvs=4#c005395654754d359fc21186ca84531a
|
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. |
twschiller
force-pushed
the
feature/9154-session-sync
branch
from
936f4c7
to
695cf09
Compare
twschiller
changed the title
Add state controller test 9154: add E2E runtime test
twschiller
force-pushed
the
feature/9154-session-sync
branch
from
6fd6e3e
to
006640c
Compare
What does this PR do?
Remaining Work
Demo
Future Work
For more information on our expectations for the PR process, see the
code review principles doc