feat(client-presence): Presence emitsworkspaceActivatedwhen a new workspace is received
#23917
Conversation
github-actions
bot
added
area: framework
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/framework/presence/src/presenceDatastoreManager.ts:90
- [nitpick] The function name 'isPresenceWorkspaceAddress' is misleading. Consider renaming it to 'hasColonInAddress' or something similar.
function isPresenceWorkspaceAddress(
packages/framework/presence/src/presenceDatastoreManager.ts:427
- [nitpick] The variable name 'internalWorkspaceTypes' could be more descriptive. Consider renaming it to 'workspaceTypeMappings' or something similar.
const internalWorkspaceTypes = {
| // Invalid public address (must be `${string}:${string}`) | ||
| "s:testStateWorkspace": statesWorkspaceUpdate, |
There was a problem hiding this comment.
"s:testStateWorkspace" sure looks like it satisfies ${string}:${string}.
| @@ -336,6 +339,62 @@ describe("Presence", () => { | |||
| // Verify | |||
| assert.strictEqual(listener.called, false); | |||
| }); | |||
|
|
|||
| it("with workspace that has an invalid internal address does NOT emit 'workspaceActivated'", () => { | |||
There was a problem hiding this comment.
I would say "unrecognized" rather than "invalid".
| const internalWorkspaceTypes = { | ||
| s: "States", | ||
| n: "Notifications", | ||
| } as const; | ||
|
|
||
| // Convert prefix to internal workspace type | ||
| const internalWorkspaceType = | ||
| internalWorkspaceTypes[prefix as keyof typeof internalWorkspaceTypes] ?? | ||
| "Unknown"; |
There was a problem hiding this comment.
Warning:
askeyword used.
This does not produce the correct type because prefix is coerced to keyof. TypeScript thinks that "Unknown" is unreachable.
Since you are going from an unknown string, internalWorkspaceTypes should be a read-only string record (Readonly<Record<string, ...>>). You can also define it outside of the class; so that it is only initialized once.
| function isPresenceWorkspaceAddress( | ||
| workspaceAddress: string, | ||
| ): workspaceAddress is PresenceWorkspaceAddress { | ||
| return workspaceAddress.includes(":"); | ||
| } | ||
|
|
There was a problem hiding this comment.
We can get rid of this by enhancing the regex
| @@ -403,7 +411,36 @@ export class PresenceDatastoreManagerImpl implements PresenceDatastoreManager { | |||
| if (workspaceDatastore === undefined) { | |||
| workspaceDatastore = this.datastore[workspaceAddress] = {}; | |||
| if (!workspaceAddress.startsWith("system:")) { | |||
| // TODO: Emit workspaceActivated event for PresenceEvents | |||
| // Separate internal type prefix from public workspace address | |||
| const match = workspaceAddress.match(/^([a-z]):(.*)/); | |||
There was a problem hiding this comment.
We should use a better match to simplify the code.
Then, because we have noUncheckedIndexAccess enabled and it is not good with array, we can do a little type fix-up based on the regex.
| const match = workspaceAddress.match(/^([a-z]):(.*)/); | |
| const match = workspaceAddress.match(/^([^:]):([^:]+:.+)$/) as null | [string,string,PresenceWorkspaceAddress]; |
(Optionally, define a regex outside of class right below InternalWorkspaceAddress.
const regexInternalWorkspaceAddress = /^([^:]):([^:]+:.+)$/;)
| @@ -403,7 +411,36 @@ export class PresenceDatastoreManagerImpl implements PresenceDatastoreManager { | |||
| if (workspaceDatastore === undefined) { | |||
| workspaceDatastore = this.datastore[workspaceAddress] = {}; | |||
| if (!workspaceAddress.startsWith("system:")) { | |||
There was a problem hiding this comment.
This if is no longer needed.
There was a problem hiding this comment.
There are some test cases that need added:
- Notifications should only happen one time per workspace address. If another instance of that address comes thru, even if, there isn't a registered workspace, then we shouldn't see the event again.
- There is nothing for the key scenario (and the implementation doesn't support it), need to make sure a new registration can get updates. Best case is for Notifications, should be able to see one is activated, register within that event, and then see the notifications that have arrived. Might want to do all or at least these register and listen cases in the events test file.
Description
Follow up to work item AB#29541 and change #23809
When Presence receives an unregistered workspace in a datastore update message, we emit a new 'workspaceActivated' event.
Added two more tests as well for invalid workspace addresses (both public and internal).
Fixes AB#29939