chore(sidebar): handle session updates #116
Conversation
WalkthroughThis update introduces a centralized authentication store ( Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant authStore
participant authClient
Component->>authStore: fetchSession()
authStore->>authClient: getSession()
authClient-->>authStore: { session, user }
authStore-->>Component: Updates reactive session/user state
sequenceDiagram
participant User
participant Website
participant Browser
participant Extension
User->>Website: Sign out
Website->>Browser: Redirect to /?fetchSession=true
Browser->>Extension: If fetchSession param, call window.grinta.fetchSession()
Extension->>authStore: fetchSession()
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/website/src/components/account-management.svelte (1)
40-49:⚠️ Potential issueAdd safety checks and error handling.
The code calls
window.grinta.fetchSession()without verifying that thegrintaobject and itsfetchSessionmethod exist, which could cause runtime errors.Add proper safety checks and error handling:
onMount(() => { if (typeof window === "undefined") return; const searchParams = new URLSearchParams(window.location.search); const fetchSession = searchParams.get("fetchSession"); if (fetchSession) { + if (!window.grinta?.fetchSession) { + console.warn('window.grinta.fetchSession is not available'); + return; + } setTimeout(() => { + try { window.grinta.fetchSession(); + } catch (error) { + console.error('Failed to fetch session:', error); + } }, 1000); } });
🧹 Nitpick comments (5)
apps/extension/src/lib/store/auth.svelte.ts (1)
5-15: Consider adding loading state management.The store lacks loading state management, which could lead to race conditions if
fetchSession()is called multiple times before completion. Users may also benefit from knowing when session fetching is in progress.Consider adding a loading state:
export class AuthStore { session = $state<Session>(); user = $state<User>(); + loading = $state<boolean>(false); async fetchSession() { + if (this.loading) return; // Prevent concurrent calls + this.loading = true; try { const { data } = await authClient.getSession(); if (!data) return; this.session = data.session; this.user = data.user; } catch (error) { console.error('Failed to fetch session:', error); this.session = undefined; this.user = undefined; + } finally { + this.loading = false; } } }apps/website/src/components/account-management.svelte (1)
45-47: Consider explaining the 1000ms delay.The 1000ms timeout delay appears arbitrary. Consider adding a comment explaining why this delay is necessary, or making it configurable if the timing is critical for the session refresh flow.
Add a comment explaining the delay:
setTimeout(() => { window.grinta.fetchSession(); -}, 1000); +}, 1000); // Allow time for sign-out to complete before fetching new sessionapps/extension/src/app.svelte (1)
47-50: Excellent improvement: Targeted session refresh over full reload.Replacing
window.location.reload()withauthStore.fetchSession()is a much better approach as it only refreshes the session data without disrupting the entire application state. However, consider adding error handling around thefetchSession()calls.Consider adding error handling:
const unsub = onMessage("grinta_fetchSession", () => { - authStore.fetchSession(); + authStore.fetchSession().catch(console.error); }); -authStore.fetchSession(); +authStore.fetchSession().catch(console.error);apps/extension/src/pages/home.svelte (1)
251-251: Good initialization pattern with room for error handling.Adding
authStore.fetchSession()inonMountensures proper session initialization. Consider adding error handling for robustness.Consider adding error handling:
-authStore.fetchSession(); +authStore.fetchSession().catch(console.error);apps/extension/src/pages/chats.svelte (1)
282-284: Good session initialization with error handling opportunity.The
onMounthook withauthStore.fetchSession()ensures proper session initialization when the component mounts. This is consistent with the pattern used in other components.Consider adding error handling for consistency:
onMount(() => { - authStore.fetchSession(); + authStore.fetchSession().catch(console.error); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/extension/package.json(1 hunks)apps/extension/src/app.svelte(2 hunks)apps/extension/src/lib/components/space-essentials.svelte(0 hunks)apps/extension/src/lib/components/space-picker.svelte(0 hunks)apps/extension/src/lib/store/auth.svelte.ts(1 hunks)apps/extension/src/pages/chats.svelte(2 hunks)apps/extension/src/pages/home.svelte(3 hunks)apps/extension/src/pages/settings.svelte(2 hunks)apps/website/src/components/account-management.svelte(1 hunks)apps/website/src/components/download-grinta.svelte(2 hunks)
💤 Files with no reviewable changes (2)
- apps/extension/src/lib/components/space-essentials.svelte
- apps/extension/src/lib/components/space-picker.svelte
🔇 Additional comments (10)
apps/extension/package.json (1)
5-5: Verify the version downgrade is intentional.The version was changed from
"1.0.0"to"0.1.0", which is a significant downgrade. This could impact extension updates for existing users and may cause confusion.Please confirm this version change is intentional and explain the reasoning. If this is a restructuring of the versioning scheme, consider documenting this change in a changelog or migration guide.
apps/extension/src/pages/settings.svelte (2)
2-2: LGTM: Clean import for version display.Good approach to import the package.json directly to display the current version dynamically.
106-133: LGTM: Well-implemented About section.The About section is well-implemented with:
- Dynamic version display using the imported package.json
- Proper security attributes on external links (
target="_blank"andrel="noopener noreferrer")- Clean UI structure consistent with the rest of the component
apps/website/src/components/account-management.svelte (1)
17-17: LGTM: Clean integration with session fetching.Good approach to use a query parameter to trigger session refresh after sign-out.
apps/extension/src/app.svelte (1)
21-21: LGTM: Good import addition for centralized auth store.The import of
authStorealigns with the migration to centralized authentication state management.apps/extension/src/pages/home.svelte (2)
24-24: LGTM: Consistent migration to centralized auth store.The import change from
authClienttoauthStorealigns with the architectural refactor.
316-331: Excellent migration to direct store access.The change from derived
uservariable to directauthStore.useraccess simplifies the code while maintaining the same conditional rendering logic. The user avatar URL construction and conditional rendering work correctly.apps/website/src/components/download-grinta.svelte (1)
3-3: LGTM: Proper lifecycle import for new functionality.The
onMountimport is necessary for the new session fetching logic.apps/extension/src/pages/chats.svelte (2)
24-25: LGTM: Consistent authentication migration pattern.The addition of
onMountand migration fromauthClienttoauthStorefollows the same pattern established in other components, ensuring consistency across the extension.
287-287: Perfect migration to direct store access.The change from a derived
uservariable to directauthStore.useraccess in the conditional rendering simplifies the code while maintaining the same functionality. The sign-in modal will correctly show when the user is not authenticated.
| async fetchSession() { | ||
| const { data } = await authClient.getSession(); | ||
| if (!data) return; | ||
| this.session = data.session; | ||
| this.user = data.user; | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent unhandled exceptions.
The fetchSession method lacks error handling. If authClient.getSession() throws an exception, it will propagate unhandled, potentially crashing the application.
Add proper error handling:
async fetchSession() {
+ try {
const { data } = await authClient.getSession();
if (!data) return;
this.session = data.session;
this.user = data.user;
+ } catch (error) {
+ console.error('Failed to fetch session:', error);
+ // Optionally reset session state on error
+ this.session = undefined;
+ this.user = undefined;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fetchSession() { | |
| const { data } = await authClient.getSession(); | |
| if (!data) return; | |
| this.session = data.session; | |
| this.user = data.user; | |
| } | |
| async fetchSession() { | |
| try { | |
| const { data } = await authClient.getSession(); | |
| if (!data) return; | |
| this.session = data.session; | |
| this.user = data.user; | |
| } catch (error) { | |
| console.error('Failed to fetch session:', error); | |
| // Optionally reset session state on error | |
| this.session = undefined; | |
| this.user = undefined; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/extension/src/lib/store/auth.svelte.ts around lines 9 to 14, the
fetchSession method does not handle errors from the async call to
authClient.getSession(), which can cause unhandled exceptions. Wrap the await
call in a try-catch block to catch any exceptions, and handle errors
appropriately, such as logging the error or setting an error state, to prevent
the application from crashing.
| onMount(() => { | ||
| if (typeof window === "undefined") return; | ||
| const searchParams = new URLSearchParams(window.location.search); | ||
| const fetchSession = searchParams.get("fetchSession"); | ||
| if (fetchSession) { | ||
| setTimeout(() => { | ||
| window.grinta.fetchSession(); | ||
| }, 1000); | ||
| } | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add type safety and error handling for robustness.
The conditional session fetching based on URL parameters is a good approach for cross-component coordination. However, there are a few concerns that should be addressed:
Apply this diff to improve type safety and error handling:
onMount(() => {
if (typeof window === "undefined") return;
const searchParams = new URLSearchParams(window.location.search);
const fetchSession = searchParams.get("fetchSession");
if (fetchSession) {
8000
setTimeout(() => {
- window.grinta.fetchSession();
+ if (window.grinta?.fetchSession) {
+ window.grinta.fetchSession().catch(console.error);
+ }
}, 1000);
}
});The improvements include:
- Type safety: Check if
window.grinta.fetchSessionexists before calling - Error handling: Add
.catch()to handle potential promise rejections - Optional chaining: Use
?.for safer property access
To verify the window.grinta interface is properly typed, run:
🏁 Script executed:
#!/bin/bash
# Description: Check for window.grinta type definitions
# Expected: Find type declarations for the grinta global object
rg -A 5 -B 5 "grinta.*fetchSession|window\.grinta" --type ts --type jsLength of output: 1706
Ensure robust, type-safe invocation of window.grinta.fetchSession
The extension injects a grinta.fetchSession async function, but without it present on every page and without a global type declaration, you risk runtime errors and TS compilation failures. Please apply the following fixes:
• In apps/website/src/components/download-grinta.svelte, guard and handle errors:
onMount(() => {
if (typeof window === "undefined") return;
const searchParams = new URLSearchParams(window.location.search);
const fetchSession = searchParams.get("fetchSession");
if (fetchSession) {
setTimeout(() => {
- window.grinta.fetchSession();
+ // Only call if the extension has injected grinta.fetchSession
+ if (window.grinta?.fetchSession) {
+ window.grinta.fetchSession().catch(err => {
+ console.error("grinta.fetchSession failed:", err);
+ });
+ }
}, 1000);
}
});• Declare the global grinta interface so TS knows about it. Create apps/website/src/global.d.ts (or another .d.ts included by your tsconfig) with:
declare interface Window {
grinta?: {
/** Injected by the browser extension */
fetchSession: () => Promise<void>;
};
}After adding that file, ensure your tsconfig.json includes src/global.d.ts under "include".
These changes cover optional chaining, promise‐rejection handling, and compile-time type safety.
🤖 Prompt for AI Agents
In apps/website/src/components/download-grinta.svelte around lines 13 to 22,
improve type safety and error handling by first checking if
window.grinta?.fetchSession exists before calling it, then invoke it with
optional chaining and add a .catch() block to handle any promise rejections.
Additionally, create a global type declaration file apps/website/src/global.d.ts
declaring the Window interface with an optional grinta object containing
fetchSession as a Promise-returning function, and ensure this file is included
in tsconfig.json to enable compile-time type safety.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes