fix(e2e/oauth): gate Redux store exposure and fix loopback listener timeout race#2670
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR centralizes an E2E restart flag, uses it to gate Redux store window exposure, fixes a race in the loopback OAuth listener timeout/registration flow, and strengthens related unit and E2E tests. ChangesE2E Gating and OAuth Timeout Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
@coderabbitai please review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch PR details
gh pr view 2670 --repo tinyhumansai/openhuman --json title,body,files,additions,deletions,commits
echo "=== PR DIFF ==="
gh pr diff 2670 --repo tinyhumansai/openhumanLength of output: 12517
Review:
|
| Scenario | Behaviour |
|---|---|
Timer fires before listen() resolves (unlisten === null) |
timedOut = true, promise rejected; .then(fn) immediately calls fn() without storing — no leak ✅ |
Timer fires after listen() resolves (unlisten !== null) |
unlisten() called; activeUnlisten cleared only when identity matches ✅ |
| Callback arrives before timeout | if (timedOut) return guards a double-resolve ✅ |
listen() registration throws |
if (timedOut) return in .catch() prevents a double-rejection ✅ |
One subtle but non-blocking observation: in the post-timeout .then(fn) branch, fn() is correctly called to avoid a leak, but activeUnlisten is not explicitly nulled. Because fn was never assigned to activeUnlisten in this branch, the identity check activeUnlisten === fn would always be false and nulling it would be meaningless. However, if activeUnlisten still holds a stale reference from a prior call at this point, that stale handle is not affected. This is a pre-existing condition rather than a regression introduced here.
app/src/store/index.ts — Store gating
The IS_DEV || E2E_RESTART_APP_AS_RELOAD guard is the right approach. The updated comment is accurate and clear.
Missing: unit tests for the race condition
This is the main concern. The checklist states "Tests added or updated (happy path + at least one failure / edge case)", but the test diff only refactors the existing E2E-build-hook assertions (swapping vi.stubEnv → vi.doMock). There are no new test cases covering the timedOut paths, specifically:
- Timer fires while
listen()is still pending → listener should not leak intoactiveUnlisten. - Timer fires after
listen()resolves →activeUnlistenshould benullafterward. - Callback arrives after timeout fires → promise must not resolve/reject a second time.
Without these, the race-fix coverage delta is likely below the 80 % gate enforced by .github/workflows/coverage.yml.
app/src/utils/__tests__/loopbackOauthListener.test.ts
The switch from vi.stubEnv + module reset to vi.doMock('../config', …) is the correct pattern here — since E2E_RESTART_APP_AS_RELOAD is a pre-evaluated constant, stubbing the env after the module graph is loaded has no effect. The mock-based approach is more robust.
One small observation: vi.doUnmock('../config') in the finally blocks removes the mock for future imports, but because vi.resetModules() is called at the top of each test before each dynamic import(), isolation between tests is maintained correctly.
## Related / linked issue
The PR description does not include a ## Related section or a Closes #NNN`` reference. Per the submission checklist, this should either reference a linked issue or be explicitly marked N/A.
Checklist items to address before merge
- Add unit tests for the three
timedOutrace scenarios described above to satisfy the 80 % diff-coverage requirement. - Add or mark N/A the
Closes#NNN`` /## Relatedchecklist item in the PR description.
…ate callback registration
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean PR, nice work.
What changed
| File | Area | Change |
|---|---|---|
app/src/store/index.ts |
Frontend | Gates window.__OPENHUMAN_STORE__ behind IS_DEV || E2E_RESTART_APP_AS_RELOAD — no longer exposed in production bundles |
app/src/utils/loopbackOauthListener.ts |
Frontend | Introduces timedOut guard to close the timeout-vs-listen-registration race: late listen() resolutions are torn down immediately, late callbacks are no-oped, activeUnlisten is only cleared when the reference matches |
app/src/utils/__tests__/loopbackOauthListener.test.ts |
Tests | Covers all three race scenarios (late listen resolution, late callback, stale activeUnlisten after timeout) and migrates E2E build hook tests from vi.stubEnv to vi.doMock to match the centralized config pattern |
Notes
- The
timedOutclosure neatly handles all orderings of timeout-vs-listen-resolution-vs-callback, and theactiveUnlisten === unlistenguard prevents a timed-out call from accidentally clearing a newer call's listener. Well-reasoned. - Config centralization (
E2E_RESTART_APP_AS_RELOADfromconfig.tsinstead of rawimport.meta.env) follows the project convention — good. - Test coverage is thorough for the edge cases being fixed.
No issues found. Marking for manual approval.
YellowSnnowmann
dismissed stale reviews from graycyrus and coderabbitai[bot]
via
ed546a4
2 participants
Summary
Problem
Solution
Added IS_DEV || E2E_RESTART_APP_AS_RELOAD gating around global store exposure so production bundles do not publish the Redux store handle.
Replaced direct env-flag checks with E2E_RESTART_APP_AS_RELOAD from config.ts to follow centralized config access conventions.
Introduced a timedOut guard in loopback OAuth listener setup:timeout marks state and rejects once;
callback path no-ops after timeout;
post-timeout listen().then(fn) path immediately calls fn() instead of storing it;
activeUnlisten is cleared only when the same handle is removed.
Tradeoff: slightly more listener lifecycle code, but deterministic cleanup and safer behavior under timing edge cases.
Submission Checklist
.github/workflows/coverage.yml. Run pnpm test:coverage and pnpm test:rust locally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change (or N/A: behaviour-only change)docs/RELEASE-MANUAL-SMOKE.md)Impact
Related
Summary by CodeRabbit
Bug Fixes
Chores