🐛 Fix new labeler config steps #156
Conversation
|
Your Render PR Server URL is https://ozone-staging-pr-156.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cqmijt2j1k6c73a7c8eg. |
| @@ -241,6 +243,19 @@ function IdentityConfigurationFlow({ | |||
| placeholder="Confirmation Code" | |||
| className="block w-full mt-2" | |||
| value={token} | |||
| autoFocus | |||
There was a problem hiding this comment.
These are unrelated UX improvements.
| @@ -206,6 +207,7 @@ function Form( | |||
| const deactivatedAt = getDeactivatedAt( | |||
| repo ? { repo } : record ? { record } : {}, | |||
| ) | |||
| const canManageChat = checkPermission('canManageChat') | |||
There was a problem hiding this comment.
Also unrelated but this hides the show messaging data block from UI for external labelers.
| @@ -97,7 +97,7 @@ export const ModerationLabel = ({ | |||
| const labelVal = toLabelVal(label, recordAuthorDid) | |||
| const isSelfLabeled = isSelfLabel(labelVal) | |||
| const labelDefFromService = | |||
| labelerServiceDef?.policies.definitionById[label.val] | |||
| labelerServiceDef?.policies.definitionById?.[label.val] | |||
There was a problem hiding this comment.
This can crash a page if a plain label is added when the policy definition does not exist. So, this is more of a safety fallback.
There was a problem hiding this comment.
The type indicates that definitionById should be present— I think this points to an implementation issue around here:
ozone/components/common/labels/useLabelerDefinition.ts
Lines 21 to 31 in 1d93086
| client.reconfigure({ skipRecord: skip }) | ||
| client | ||
| .reconfigure({ skipRecord: skip }) | ||
| .then(() => client.refetchServerConfig()) |
There was a problem hiding this comment.
I wouldn't expect the server config to be updated from the labeler record configuration flow—is it possible we don't need it?
There was a problem hiding this comment.
it doesn't update but this is the issue i was seeing #156 (comment) so after the configuration is complete, we are fetching the server config for real here.
lib/client.ts
Outdated
| // If the labeler needs further configuration, let's generate a mock serverConfig | ||
| // and let the user move forward with configuration steps | ||
| const needsConfiguration = config.needs.key || config.needs.service | ||
| const serverConfig = await (needsConfiguration | ||
| ? parseServerConfig({}) | ||
| : this._getServerConfig(agent, config.did)) |
There was a problem hiding this comment.
I'm not sure I understand how this part works. Ideally IMO the server config and the labeler config could be managed independently from each other, and we wouldn't need to avoid loading the real server config. But maybe there's an overlap that I'm overlooking.
There was a problem hiding this comment.
requests to the server config seems to fail until the labeler config is available. the error was something like record not found or something along that line and since it doesn't make a lot of sense to fetch this until the labeler config is available, delaying the fetch makes sense to me.
There was a problem hiding this comment.
I can't think of a reason that "record not found" would come from fetching the server config. I have a hunch that error may have been a red herring and we may not need this change, but I'm going to do some investigation to find out.
If we can avoid it I think that would be ideal, as it is a bit difficult to reason about when we're in the state of using a mock server config and what guarantees we leave that state. These configs seem independent to me at a glance, and we might be able to untangle them if they are indeed tangled. Taking a look!
There was a problem hiding this comment.
(And if we're not sure whether we interpreted the "record not found" error properly, perhaps there's still an issue out there!)
There was a problem hiding this comment.
Ohhhhkay, I get what's going on now. They are tangled together a little bit because we need to use service proxying to fetch the server config. And server proxying is not going to work until the service is reflected in the labeler's DID document. So we need to wait until config.needs.service is false before we can make authed requests to ozone.
I made some small tweaks in fc7093c, let me know what you think!
This PR ensures that initial labeler set up steps can continue on without getting blocked by attempts to fetch server config.