Conversation
Replace localStorage opt-in + DOM listener approach with a config-driven
developer-managed API: `autoRefresh: { whenActive: true }` + `sdk.markActive()`.
- `withAutoRefresh`: parse `autoRefresh` as `boolean | { whenActive }`;
expose `markActive()` on the SDK (no-op when not in whenActive mode);
remove DOM event listeners and localStorage check
- `types.ts`: `autoRefresh` now accepts `boolean | AutoRefreshConfig`
- `react-sdk` AuthProvider: update `autoRefresh` prop type to `AutoRefreshConfig`
- `react-sdk` types/useSdk: add `markActive` to `Sdk` type
- `react-sdk` example: add `ActivityTracker` component with click/keydown listeners
- READMEs: document new API
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
✅ Code review completed successfully |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
View your CI Pipeline Execution ↗ for commit 7b7be3a
☁️ Nx Cloud last updated this comment at |
![shuni-bot-dev[bot]](https://avatars.githubusercontent.com/u/97479186?s=60&v=4){kind=small}
| import { getLocalStorage } from '../helpers'; | ||
|
|
||
| // localStorage key for opt-in activity-based refresh | ||
| export const ACTIVITY_REFRESH_KEY = '__descope_activity_refresh'; |
There was a problem hiding this comment.
🟠 HIGH: Dead code - ACTIVITY_REFRESH_KEY is defined but never used in the codebase.
The code defines a localStorage key for "opt-in activity-based refresh", but this localStorage approach isn't actually used in the implementation. The whenActive flag is passed directly via the config object instead.
Recommendation: Remove this constant and the related isActivityRefreshEnabled() function (lines 20-27) if localStorage-based opt-in is not needed.
| }, ACTIVITY_DEBOUNCE_MS); | ||
| }; | ||
|
|
||
| const attachListeners = () => { |
There was a problem hiding this comment.
🟠 HIGH: attachListeners() and detachListeners() methods are never called.
The activity tracker creates these methods for automatic DOM event listening, but they're never invoked in withAutoRefresh/index.ts. This means:
- The automatic event listeners (mousemove, keydown, touchstart, scroll, click, pointerdown) are never attached
- Developers must manually call
sdk.markActive()for activity tracking to work - There's confusion between automatic vs manual activity tracking
Recommendation: Either:
- Remove these unused methods if manual
sdk.markActive()calls are the intended design - OR call
attachListeners()whenwhenActiveis enabled anddetachListeners()on cleanup
The current implementation creates dead code that suggests functionality that doesn't exist.
🐕 Shuni Code ReviewSummaryThis PR adds activity-based session refresh functionality, allowing developers to skip refresh calls for idle users. The feature is well-intentioned and addresses a real need for reducing unnecessary API calls. However, there are several code quality issues that should be addressed before merging, including dead code, missing test coverage, and a memory leak in event listener cleanup. Findings by Severity🔴 Critical Issues (0)No critical security vulnerabilities or data loss risks identified. 🟠 High Priority Issues (3)
🟡 Medium Priority Issues (2)
🟢 Low Priority Suggestions (2)
✅ Strengths
Security AssessmentNo security vulnerabilities identified. The implementation:
Code Quality AssessmentArchitecture: Generally follows existing patterns but has dead code that suggests incomplete refactoring. Maintainability: Good documentation but dead code reduces clarity. Performance: Debouncing strategy is appropriate, though unused listener methods suggest potential confusion. Overall RecommendationREQUEST CHANGES The feature implementation is solid, but code quality issues should be resolved before merging:
These are straightforward fixes that will significantly improve code maintainability. Next Steps
🤖 Automated review by Shuni (Claude Opus 4.6) |
Remove unused DOM listener code (ACTIVITY_EVENTS, ACTIVITY_REFRESH_KEY, isActivityRefreshEnabled, attachListeners, detachListeners, debounce logic) left over from the previous localStorage opt-in approach. Add JSDoc to createActivityTracker describing its state and flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
… var - Add 6 tests covering whenActive activity-based refresh: idle skip, catch-up refresh on markActive, no-op without prior skip, active user timer flow, and no-op when whenActive is not set - Uncomment ActivityTracker in react-sdk example, controlled by DESCOPE_ACTIVITY_TRACKING=true env var Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Related Issues
Fixes https://github.com/descope/etc/issues/13944
Description
User activity tracking (opt in)