-
Notifications
You must be signed in to change notification settings - Fork 48
Refactor Flow Editor auth handling: remove setAuthHeader and improve token renewal #3553
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
base: master
Are you sure you want to change the base?
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚀 Deployed on https://deploy-preview-3553--glific-frontend.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/floweditor/FlowEditor.tsx (1)
269-284: Call the health check immediately to avoid initial token expiry gaps.The polling interval starts only after 15 minutes, which means if the token expires within the first 15 minutes after opening the flow editor, the polling won't prevent authentication failures. Consider invoking
pollFlowConnection()immediately when the effect runs.Additionally, including
getFreeFlowin the dependency array may cause unnecessary effect re-runs if the query function reference changes. Since Apollo Client'suseLazyQuerytypically returns stable references, this might not be an issue in practice, but consider removing it from the dependencies if you encounter performance issues.Apply this diff to call the health check immediately:
useEffect(() => { if (flowId) { // health check to ensure the session is active const pollFlowConnection = () => { getFreeFlow({ variables: { id: flowId } }); }; + // Call immediately on mount + pollFlowConnection(); + // setting the polling interval to 15 minutes const POLLING_INTERVAL = 15 * 60 * 1000; const pollingInterval = setInterval(pollFlowConnection, POLLING_INTERVAL); return () => { clearInterval(pollingInterval); }; } -}, [flowId, getFreeFlow]); +}, [flowId]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/floweditor/FlowEditor.tsx(1 hunks)src/services/AuthService.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
src/services/AuthService.tsxsrc/components/floweditor/FlowEditor.tsx
🧬 Code graph analysis (1)
src/components/floweditor/FlowEditor.tsx (1)
src/mocks/Flow.tsx (1)
getFreeFlow(507-525)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
- GitHub Check: CI
- GitHub Check: build
🔇 Additional comments (2)
src/services/AuthService.tsx (2)
81-88: LGTM! The 1-minute pre-expiry buffer prevents token failures.The implementation correctly adds a 60-second buffer before the actual token expiry time, ensuring tokens are renewed proactively before they become invalid. This aligns well with the PR objectives to prevent failures in the flow editor.
73-93: setAuthHeaders removal verified
No remaining references tosetAuthHeadersacross.ts/.tsxfiles.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3553 +/- ##
==========================================
+ Coverage 82.99% 83.16% +0.17%
==========================================
Files 340 340
Lines 11241 11195 -46
Branches 2379 2368 -11
==========================================
- Hits 9329 9310 -19
+ Misses 1202 1177 -25
+ Partials 710 708 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Glific
|
||||||||||||||||||||||||||||||||||||||||||||||
| Project |
Glific
|
| Branch Review |
floweditor-auth-handling
|
| Run status |
|
| Run duration | 27m 55s |
| Commit |
|
| Committer | Akansha Sakhre |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
1
|
|
|
2
|
|
|
0
|
|
|
0
|
|
|
181
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/flow/FlowEditor.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Flow > should configure Flow |
Test Replay
Screenshots
|
|
chat/Chat.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Role - Staff - Chats > should send the message correctly |
Test Replay
Screenshots
|
|
contactBar/ContactBar.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Role - Staff - Contact bar > should start a flow |
Test Replay
Screenshots
|
|
| }; | ||
|
|
||
| // setting the polling interval to 15 minutes | ||
| const POLLING_INTERVAL = 15 * 60 * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to constants.ts
| if (flowId) { | ||
| // health check to ensure the session is active | ||
| const pollFlowConnection = () => { | ||
| getFreeFlow({ variables: { id: flowId } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about using getFreeFlow for polling here again? Maybe we can just add polling to the initial query and remove manual polling. Let's discuss.
|
progress so far:
Next steps
|
@akanshaaa19 isn't it ok for the token to expire with a 30+ min inactivity? As long as the "work" is saved automatically. What is our requirement on this from a user's perspective? |
Summary by CodeRabbit