Cloud/fix cookie timing race #6398
Conversation
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/30/2025, 03:48:11 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/30/2025, 04:00:32 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.31 MB (baseline 3.31 MB) • ⚪ 0 BMain entry bundles and manifests
Graph Workspace — 718 kB (baseline 718 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Views & Navigation — 8.14 kB (baseline 8.14 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 294 kB (baseline 294 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
UI Components — 12.3 kB (baseline 12.3 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 10 kB (baseline 10 kB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.36 MB (baseline 5.36 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
christian-byrne
added
needs-backport
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| let inFlightCreateSession: Promise<void> | null = null |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Module-level variables (inFlightCreateSession, currentCreateController, logoutInProgress) create shared mutable state that could cause issues in environments where the module is imported multiple times
Context: This breaks the isolation principle and could lead to unexpected behavior if useSessionCookie is used across different components or contexts
Suggestion: Move these variables inside the composable factory or use a proper singleton pattern with explicit state management
| createSession, | ||
| deleteSession | ||
| } | ||
| const isAbortError = (error: unknown): boolean => { |
There was a problem hiding this comment.
[performance] low Priority
Issue: The isAbortError function could be more efficient with direct property access
Context: The current implementation uses nested checks and type assertions
Suggestion: Simplify to: return error instanceof Error && error.name === 'AbortError'
| `Failed to create session: ${errorData.message || response.statusText}` | ||
| ) | ||
| } | ||
| } catch (error: unknown) { |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Missing error logging for debugging session creation failures
Context: When session creation fails, the error is silently caught if it's an AbortError, but other errors only bubble up without any logging
Suggestion: Add console.debug or appropriate logging for both successful completions and errors to aid in troubleshooting authentication issues
| reject = rej | ||
| }) | ||
|
|
||
| // @ts-expect-error initialized via closure assignments above |
There was a problem hiding this comment.
[quality] low Priority
Issue: Test uses @ts-expect-error without providing a meaningful error message
Context: Line 25 suppresses TypeScript error for closure variable assignments without explaining why
Suggestion: Add descriptive comment: '@ts-expect-error - resolve and reject are assigned via closure in Promise constructor'
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: Cloud/fix cookie timing race (#6398)
Impact: 225 additions, 27 deletions across 2 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 2
- Low: 2
Category Breakdown
- Architecture: 1 issues
- Security: 0 issues
- Performance: 1 issues
- Code Quality: 2 issues
Key Findings
Architecture & Design
The PR successfully addresses a race condition in session management by introducing:
- Singleton Pattern: Uses VueUse's createSingletonPromise to ensure only one session creation request occurs at a time
- Request Cancellation: Implements proper AbortController usage to cancel in-flight requests during logout
- State Synchronization: Coordinates between create and delete operations with shared state variables
However, the module-level state variables (inFlightCreateSession, currentCreateController, logoutInProgress) could cause isolation issues if the module is imported multiple times.
Security Considerations
The authentication flow appears secure:
- Proper credential handling with credentials: include
- Auth headers from Firebase Auth Store are correctly applied
- Error messages don't leak sensitive information
- Session deletion properly cleans up state
Performance Impact
The implementation is generally efficient:
- Request deduplication prevents redundant API calls
- Proper cleanup of AbortController instances
- Minor optimization opportunity in error checking utility function
Integration Points
This change affects:
- Cloud authentication flow
- Session lifecycle management
- Firebase Auth Store integration
- Any components that call createSession/deleteSession
Positive Observations
- Comprehensive Testing: The test suite covers both primary use cases - deduplication and abort scenarios
- Proper Error Handling: Includes appropriate error boundaries and type checking
- Clean API: Maintains the same public interface while fixing the race condition
- Modern Patterns: Uses current best practices with AbortController and VueUse utilities
References
- Repository Architecture Guide: https://github.com/Comfy-Org/comfy-claude-prompt-library/blob/master/project-summaries-for-agents/ComfyUI_frontend/REPOSITORY_GUIDE.md
- VueUse Documentation: https://vueuse.org/shared/createSingletonPromise/
Next Steps
- Address module-level state isolation concern before merge
- Consider adding debug logging for authentication troubleshooting
- Minor performance optimization in error checking function
- Enhance @ts-expect-error comments in tests
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
Summary
Changes
Review Focus
Screenshots (if applicable)
┆Issue is synchronized with this Notion page by Unito