Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor StatefulAuthProvider for static initialization and improved session management #1822

Merged
merged 40 commits into from
Dec 12, 2024

Conversation

pastuxso
Copy link
Contributor

@pastuxso pastuxso commented Nov 25, 2024

This refactor centralizes session management, reduces duplication, and improves maintainability across the extension.

  • Centralized session change notifications using a new SessionChangeHandler implemented with RxJS and a singleton pattern.
  • Refactored StatefulAuthProvider to follow a singleton pattern, simplifying its lifecycle and reducing dependencies.
  • Added functionality to retrieve sessions from secrets storage, avoiding potential infinite loops when checking sessions inside a session change listener.
  • Removed Kernel and RunmeUriHandler dependencies from StatefulAuthProvider for better modularity.
  • Introduced a new GraphQL client instantiator to encapsulate implementation details like authentication token configuration.
  • Ensured Webview refreshes correctly on login/logout events.

@pastuxso pastuxso force-pushed the cris/stateful-auth-provider branch from 6956ac7 to a22b762 Compare November 27, 2024 18:01
@pastuxso pastuxso marked this pull request as ready for review December 6, 2024 02:44
@pastuxso
Copy link
Contributor Author

pastuxso commented Dec 6, 2024

Hi @sourishkrout,

I had to rollback the reactive approach in cloud.ts (d2040b1). Unfortunately, the subscription isn't being triggered, so the panel fails to refresh its content as intended.

I've recorded a video demonstrating the steps to test the desired functionality. Your expertise would be greatly appreciated to resolve this issue. Let me know if you need any further details!

Thanks! 🙌

login-logout.mp4

@pastuxso pastuxso requested a review from sourishkrout December 6, 2024 02:51
@sourishkrout
Copy link
Member

@pastuxso When I open the Runme side panel, I'll immediately get prompted to log in. That's not right. It should display the "Sign in or create account" button first.

@pastuxso
Copy link
Contributor Author

pastuxso commented Dec 9, 2024

Just to confirm, is it happening for Stateful or Runme ext?

@sourishkrout
Copy link
Member

Just to confirm, is it happening for Stateful or Runme ext?

I've only tested Runme so far.

src/extension/utils.ts Outdated Show resolved Hide resolved
@pastuxso
Copy link
Contributor Author

pastuxso commented Dec 10, 2024

@pastuxso When I open the Runme side panel, I'll immediately get prompted to log in. That's not right. It should display the "Sign in or create account" button first.

@sourishkrout, I’ve tried to replicate it for Runme, but I couldn’t. The only way to replicate is within Stateful Extension. I wonder if a specific setting might be causing this issue. Would you mind sending me a screenshot of your panel so I can take a closer look?

@sourishkrout
Copy link
Member

@pastuxso When I open the Runme side panel, I'll immediately get prompted to log in. That's not right. It should display the "Sign in or create account" button first.

@sourishkrout, I’ve tried to replicate it for Runme, but I couldn’t. The only way to replicate is within Stateful Extension. I wonder if a specific setting might be causing this issue. Would you mind sending me a screenshot of your panel so I can take a closer look?

I believe this below is the issue. It forces the creation of a session inside the call stack without 'true' here. That being said, I wonder if the client flag should be an object and explicit. Optional arguments are easy to overlook.

diff --git a/src/extension/panels/cloud.ts b/src/extension/panels/cloud.ts
index 4c917942..6eec159b 100644
--- a/src/extension/panels/cloud.ts
+++ b/src/extension/panels/cloud.ts
@@ -59,7 +59,7 @@ export default class CloudPanel extends TanglePanel {
 
     log.trace(`${this.identifier} webview resolving`)
 
-    const html = await this.getHydratedHtml()
+    const html = await this.getHydratedHtml(true)
     webviewView.webview.html = html
     webviewView.webview.options = {
       ...webviewOptions,

Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

✅ LGTM

@sourishkrout sourishkrout merged commit 1a55850 into main Dec 12, 2024
1 check passed
@sourishkrout sourishkrout deleted the cris/stateful-auth-provider branch December 12, 2024 21:23
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.

2 participants