Skip to content

feat!: safari extension client status#57

Merged
dwosk merged 5 commits intomainfrom
feat/safari-extension-client-status
Mar 18, 2025
Merged

feat!: safari extension client status#57
dwosk merged 5 commits intomainfrom
feat/safari-extension-client-status

Conversation

@JAHMCR
Copy link
Copy Markdown
Contributor

@JAHMCR JAHMCR commented Mar 13, 2025

Added a listener to handle when the client state changes to notify event subscribers accordingly.

@JAHMCR JAHMCR requested review from dabrad26 and dwosk March 13, 2025 18:47
@JAHMCR JAHMCR self-assigned this Mar 13, 2025
Copy link
Copy Markdown
Member

@dwosk dwosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JAHMCR did you see my comment on the ticket about adding the listener for isKeepAlive?

It should be a relatively minor change (i.e. don't think we need to export any new functions, etc.)

// Register in Safari context
document.addEventListener('isAppAlive', (event) => {
  console.info('Desktop app running', event.detail.running);
});

When the SDK gets an event, it should have logic to trigger the registered status listeners (i.e. listeners registered via registerStatusCallback) if the desktop app went from running -> not running or vice versa.

@JAHMCR
Copy link
Copy Markdown
Contributor Author

JAHMCR commented Mar 14, 2025

@JAHMCR did you see my comment on the ticket about adding the listener for isKeepAlive?

It should be a relatively minor change (i.e. don't think we need to export any new functions, etc.)

// Register in Safari context
document.addEventListener('isAppAlive', (event) => {
  console.info('Desktop app running', event.detail.running);
});

When the SDK gets an event, it should have logic to trigger the registered status listeners (i.e. listeners registered via registerStatusCallback) if the desktop app went from running -> not running or vice versa.

Yeah thanks, I will update the PR very soon

@JAHMCR JAHMCR force-pushed the feat/safari-extension-client-status branch from 3e1aaae to a986a44 Compare March 14, 2025 16:47
@JAHMCR JAHMCR requested a review from dwosk March 14, 2025 20:39
Comment thread src/helpers/client/safari-client.ts Outdated
Comment thread src/helpers/client/safari-client.ts
Comment thread src/models/aspera-sdk.model.ts Outdated
Comment thread src/models/aspera-sdk.model.ts
Comment thread src/models/aspera-sdk.model.ts Outdated
Comment thread src/models/aspera-sdk.model.ts
@JAHMCR JAHMCR requested a review from dwosk March 17, 2025 15:13
Copy link
Copy Markdown
Member

@dwosk dwosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JAHMCR. It seems to work fine for me. Had Chrome and Safari side by side and saw the callbacks were triggered the same when I started/stopped the app.

Copy link
Copy Markdown
Contributor

@dabrad26 dabrad26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe can change to !item for all the item === undefined stuff. Do we have a type for the any on events. Maybe better if we can define event

Comment thread src/helpers/client/safari-client.ts Outdated
*/
private listenResponseEvents() {
document.addEventListener('AsperaDesktop.Response', (event: CustomEvent<JSONRPCResponse>) => {
if (event.detail === undefined) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably just do !event.detail. Then we don’t need to explicit checking undefined as I assume we don’t want continue if null or empty string or anything.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@JAHMCR JAHMCR requested a review from dabrad26 March 18, 2025 22:31
@dwosk dwosk merged commit 266a649 into main Mar 18, 2025
3 of 4 checks passed
@dwosk dwosk deleted the feat/safari-extension-client-status branch March 18, 2025 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants