fix state manager #50

davixcky · 2025-04-24T01:18:49Z

refactor state
add debug panel
fix tracestate
context storage and debug panel under settings

This is break currently, but with this refactor i expect to fix multiple issues related to stata manager including
#44 and one that was reported today Error updating dynamic rules: Error: Rule with id 1 does not have a unique ID.

I think that refactor should improve the speed a little bit cause it will prevent some duplicate calls that are triggered on the service worker

…sion into fix-state-manager

davixcky · 2025-04-24T23:03:35Z

Update:
The refactor is working now, it has been apply to all the page and seems easy to extend now.

Sync React state with browser
The headers calculation is done on the react side and NOT service worker
Service worker still injects the header
All actions/updates are being set through storage context

cc: @foxish

foxish · 2025-04-28T22:20:38Z

I just loaded this change up locally by building from dist/. And I am not able to turn it on:

Also, as soon as I load it, I get an error:

(()=>{"use strict";var e;!function(e){e.routingKey="routingKey",e.signadotUrls="signadotUrls",e.enabled="enabled",e.traceparentHeader="traceparentHeader",e.headers="headers",e.debugMode="debugMode",e.apiUrl="apiUrl",e.previewUrl="previewUrl",e.dashboardUrl="dashboardUrl"}(e||(e={}));const t=chrome.declarativeNetRequest.ResourceType,r=[t.MAIN_FRAME,t.SUB_FRAME,t.STYLESHEET,t.SCRIPT,t.IMAGE,t.FONT,t.OBJECT,t.XMLHTTPREQUEST,t.PING,t.CSP_REPORT,t.MEDIA,t.WEBSOCKET,t.OTHER];function a(){return t=this,a=void 0,o=function*(){const t=yield(i=[e.routingKey,e.enabled,e.headers,e.traceparentHeader],a=void 0,n=void 0,o=void 0,c=function*(){return yield chrome.storage.local.get(i)},new(o||(o=Promise))((function(e,t){function r(e){try{d(c.next(e))}catch(e){t(e)}}function i(e){try{d(c.throw(e))}catch(e){t(e)}}function d(t){var a;t.done?e(t.value):(a=t.value,a instanceof o?a:new o((function(e){e(a)}))).then(r,i)}d((c=c.apply(a,n||[])).next())})));var a,n,o,c,i;const{routingKey:d,enabled:l,headers:s,traceparentHeader:u}=t;if(!(s.length>0&&l&&d)){try{const e=yield chrome.declarativeNetRequest.getDynamicRules();yield chrome.declarativeNetRequest.updateDynamicRules({removeRuleIds:e.map((e=>e.id))}),console.log("Dynamic rules cleared.")}catch(e){console.error("Error clearing dynamic rules: ",e)}return}const h=(e=>{let t=1;return e.map((([e,a])=>({id:t++,priority:1,action:{type:chrome.declarativeNetRequest.RuleActionType.MODIFY_HEADERS,requestHeaders:[{header:e,operation:chrome.declarativeNetRequest.HeaderOperation.SET,value:a}]},condition:{urlFilter:"*",excludedRequestDomains:["preview.signadot.com","preview.staging.signadot.com","localhost.signadot.com"],resourceTypes:r}})))})(JSON.parse(s)),m=yield chrome.declarativeNetRequest.getDynamicRules();yield chrome.declarativeNetRequest.updateDynamicRules({removeRuleIds:m.map((e=>e.id)),addRules:h}),console.log("Dynamic rules updated successfully.")},new((n=void 0)||(n=Promise))((function(e,r){function c(e){try{d(o.next(e))}catch(e){r(e)}}function i(e){try{d(o.throw(e))}catch(e){r(e)}}function d(t){var r;t.done?e(t.value):(r=t.value,r instanceof n?r:new n((function(e){e(r)}))).then(c,i)}d((o=o.apply(t,a||[])).next())}));var t,a,n,o}chrome.runtime.onInstalled.addListener(a),chrome.runtime.onStartup.addListener(a),chrome.storage.onChanged.addListener(((t,r)=>{console.log({changes:t,areaName:r}),"local"===r&&(t[e.headers]||t[e.routingKey]||t[e.enabled]||t[e.traceparentHeader])&&a()}))})();

Am I missing something / doing something wrong?

foxish

I see the intent behind the refactor and the separation of the storage this way makes sense, but I had several comments along the way.

foxish · 2025-04-28T22:23:29Z

src/components/ListRouteEntries/ListRouteEntries.tsx

  useEffect(() => {
    refetch();
-  }, [apiUrl]);
+  }, [settings.signadotUrls.apiUrl]);


Maybe a separate follow-up issue, but I'd expect this to be refetched if the org name changes as well (someone logs into different org)

src/components/ListRouteEntries/ListRouteEntries.tsx

src/components/Frame/Frame.tsx

foxish · 2025-04-28T22:28:52Z

src/service-worker.ts


-        await chrome.storage.local.set({[StorageKey.InjectedHeaders]: headerKeys});
+  const { routingKey, enabled, headers, traceparentHeader } = values;
+  const injectHeaders = headers.length > 0 && enabled && routingKey;


Given the default headers, we don't ever expect headers.length be 0 right?

Yes, we expect in the case of not being enabled in the react side, we set to empty array.

foxish · 2025-04-28T22:35:32Z

src/contexts/StorageContext/StorageContext.tsx

+
+                // Signadot URLs
+                if (storedValues[StorageBrowserKeys.signadotUrls] !== undefined) {
+                    try {


Can we do this without the try...catch? If this is our code, can't we check for whether it's present using if..else rather than throwing and catching exceptions?

JSON.parse could trigger an error if it's not a valid JSON or it's malformed. In any case i think it's a good practice to in case of error, set the default. Cause this could be user prune if they updated by hand, or for some reason value is corrupted. Or if between version and version we decide to change the schema

Okay. fair, but let's catch specific exceptions with JSON parsing using instanceof - that will at least ensure we don't hide errors that are unrelated and behave in surprising ways.

foxish · 2025-04-28T22:36:36Z

src/components/Settings/Settings.tsx

-    const [previewUrl, setPreviewUrl] = useState<string>(DEFAULT_PREVIEW_URL);
-    const [dashboardUrl, setDashboardUrl] = useState<string>(DEFAULT_DASHBOARD_URL);
-    const [traceparentHeader, setTraceparentHeader] = useState<string>(DEFAULT_TRACEPARENT_HEADER);
+    const [temporaryValues, setTemporaryValues] = useState<{


What is a temporaryValue is and what purpose does this serve?

Maybe unsavedValues? This is working as temporary values that are yet not saved

unsavedValues sgtm

src/components/PinnedRouteGroup/PinnedRouteGroup.tsx

src/contexts/StorageContext/types.ts

src/contexts/StorageContext/utils.ts

foxish · 2025-04-29T15:25:19Z

src/components/Settings/Settings.tsx


+        setTraceparent(temporaryValues.traceparentHeaderEnabled, temporaryValues.traceparentHeader);
+
+        // chrome.storage.local.set({


Please add a code comment explaining why this is commented out

I accidentally applyed the prettier to all the files. So there are bunch of changes, but primarly you should focus on - Route - ProtectedRoute - AuthContext - Frame - Layout This fixes some inconsistences on the unauthenticated state, blocking the access in some cases, where now it's being fixed thanks to the RouteView context

davixcky · 2025-04-29T18:55:50Z

@foxish major and minor fixes are been applied, any issue that is not state should be tackled separelty. Things that remains is to test traceparent is being updated correctly and injected when it should. Similar to the others headers to be injected only when

feature is enabled
selected routing key
user is authenticated

Tested on a fresh installation

foxish

LGTM, it got messy and too large, in future, please plan ahead so that the complicated changes can be merged separately and the stylistic stuff can happen later.

This change is not correct - but can be addressed in follow-up PR - the search should come up on top and also when nothing is selected, it's cutting off the dropdown.

davixcky · 2025-04-29T19:39:12Z

That issue is being addressed in #51

davixcky and others added 9 commits April 18, 2025 11:16

refactor state

9a20f29

add debug panel

3524f8c

fix tracestate

abdeb8d

context storage and debug panel under settings

0dad837

Delete src/components/Devtools.tsx

182dc47

update to use new context

cd4d307

calculate header and update browser state

961defd

fix storage and context and make service worker aware

282e726

Merge branch 'fix-state-manager' of github.com:signadot/browser-exten…

d2b4d14

…sion into fix-state-manager

davixcky marked this pull request as ready for review April 24, 2025 23:00

foxish reviewed Apr 28, 2025

View reviewed changes

foxish reviewed Apr 29, 2025

View reviewed changes

davixcky and others added 3 commits April 29, 2025 13:41

fix format and rename from basic to default header

110832d

rename from temporaryValues to unsavedValues

3f321df

foxish approved these changes Apr 29, 2025

View reviewed changes

davixcky merged commit 9f920ed into main Apr 29, 2025


		setTraceparent(temporaryValues.traceparentHeaderEnabled, temporaryValues.traceparentHeader);

		// chrome.storage.local.set({

fix state manager #50

fix state manager #50

Uh oh!

Conversation

davixcky commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davixcky commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxish commented Apr 28, 2025

Uh oh!

foxish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davixcky commented Apr 29, 2025

Uh oh!

foxish left a comment

Choose a reason for hiding this comment

Uh oh!

davixcky commented Apr 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davixcky commented Apr 24, 2025 •

edited

Loading

davixcky commented Apr 24, 2025 •

edited

Loading