refactor(web): break stores/auth and api/client cycle via leaf handler registry#2082
Conversation
Dependency ReviewThe following issues were found:
License Issues.github/workflows/cla.yml
.github/workflows/dev-release.yml
.github/workflows/lighthouse.yml
.github/workflows/refresh-test-durations.yml
.github/workflows/zizmor.yml
OpenSSF Scorecard
Scanned Files
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (30)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)!(src/synthorg/persistence/**)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
web/src/**/*.{js,jsx,ts,tsx,mts}📄 CodeRabbit inference engine (web/CLAUDE.md)
Files:
web/src/**/*.{ts,tsx,mts}📄 CodeRabbit inference engine (web/CLAUDE.md)
Files:
web/src/stores/**/*.ts📄 CodeRabbit inference engine (web/CLAUDE.md)
Files:
web/src/{api/endpoints,stores}/**/*.ts📄 CodeRabbit inference engine (web/CLAUDE.md)
Files:
web/src/{stores,**/*.test.{ts,tsx}}📄 CodeRabbit inference engine (web/CLAUDE.md)
Files:
🔇 Additional comments (30)
WalkthroughThis PR eliminates a real circular dependency between Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a long-standing circular dependency in the web architecture by implementing a registry pattern for unauthorized access handling. By introducing a lightweight leaf module to mediate communication between the API client and the authentication store, the change eliminates the need for dynamic imports and temporary linting exemptions, resulting in a cleaner and more robust module graph. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the 401 unauthorized handling to resolve a circular dependency between the API client and the auth store. It introduces a new unauthorized-handler leaf module to decouple the components and adds a regression test to ensure the circular dependency is not reintroduced. Review feedback suggests restoring a defensive fallback for 401 responses to prevent them from being silently ignored if the handler is not yet registered, and recommends enhancing the handler registry to support multiple subscribers for better extensibility.
| // We only need to sync the Zustand auth state. Routed through the | ||
| // leaf `unauthorized-handler` module so the client has no static | ||
| // dependency on the auth store. | ||
| notifyUnauthorized() |
There was a problem hiding this comment.
The removal of the fallback redirect logic (previously in the .catch block) introduces a risk where 401 responses might be silently ignored if the apiClient is used in a context where the authStore has not been initialized (e.g., standalone tools, specific test environments, or early bootstrap phases). While the main application wires this at module init, the previous implementation provided a defensive safety net. Consider adding a default behavior or a warning in the notifyUnauthorized call if no handler is present to ensure session expiry is always handled visibly.
| export function setUnauthorizedHandler(handler: UnauthorizedHandler | null): void { | ||
| current = handler | ||
| } |
There was a problem hiding this comment.
The current implementation uses a single-slot registry (let current). In a larger application, this pattern is susceptible to 'singleton collision' where multiple modules might attempt to register different handlers, with the last one silently overwriting previous ones. While currently only authStore uses this, using a Set of handlers or an event-emitter pattern would be more robust and extensible for future requirements (e.g., logging 401s in a separate observability module).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2082 +/- ##
==========================================
+ Coverage 85.04% 87.11% +2.06%
==========================================
Files 2251 2251
Lines 130269 130269
Branches 10748 0 -10748
==========================================
+ Hits 110792 113482 +2690
- Misses 16759 16772 +13
+ Partials 2718 15 -2703 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- zizmor: replace orphaned checkout SHA 31a45a7 with 2592118 (origin/main HEAD) across 25 workflow files; the prior SHA was not reachable from any branch, failing impostor-commit on PR runs - gemini(unauthorized-handler): convert single-slot registry to Set-based multi-subscriber; setUnauthorizedHandler now returns an unsubscribe closure so HMR and observability subscribers can co-exist without silent overwrites - gemini(unauthorized-handler): log warning when notifyUnauthorized fires with no subscribers (early bootstrap, standalone tools, test envs) so 401s never silently disappear
Aureliolo
force-pushed
the
refactor/stores-auth-api-client-circular-dep
branch
from
215da43 to
f7955e4
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
Bot
added
prio:medium
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Rebased on origin/main to pick up PR #2082's workflow SHA refresh, then folded fixes for the six failing checks (zizmor, lychee, 3x test flake symptoms, CI Pass) plus the three gemini-code-assist inline comments. lychee.yml: bump local action SHA 31a45a7 → 2592118 to match the rest of the workflows on main (the old SHA is no longer reachable in the repo, which is why zizmor's impostor-commit check fired across every workflow); rename version: to lycheeVersion: (the input name in v2 of lycheeverse/lychee-action — the old name was silently ignored, so the action ran default v0.23.0 instead of the pinned v0.24.2). renovate.json (gemini #1): \s+ → \s* so the binary-tool matcher catches unindented shell variables (LYCHEE_VERSION sits at column 0, not after YAML indent). Also add lycheeVersion: alternation to the action-input matcher so renovate keeps tracking the lychee bump path after the rename above. scripts/install_cli_tools.sh (gemini #2): handle GOPATH multi-entry strings — take the first colon/semicolon-separated entry, since go install writes to the first entry's bin/ and the raw joined string breaks mkdir -p. scripts/install_cli_tools.sh (gemini #3): switch tmpdir cleanup trap from RETURN to EXIT so set -e failures (failed curl / sha256sum / tar) still trigger cleanup; expand ${tmpdir} at trap-install time since the local goes out of scope before EXIT fires. lychee.toml: exclude dash.cloudflare.com (always 403 to unauthenticated probes — referenced from docs/guides/fork-setup.md as a navigation pointer) and docs.sigstore.dev (intermittent timeouts under the strict 10s ceiling — referenced from docs/guides/deployment.md). Test failures (Test Unit shard 4, Test E2E, Test Conformance SQLite) are pre-existing pytest_sessionstart cross-worker flakes — the os.abort() in tests/conftest.py is the documented forensic mechanism for capturing stacks when the FileLock races. The same flake hit main's prior CI run at 22:29 and self-healed on the next run at 23:51; re-pushing should re-trigger and likely clear them. CI Pass is the aggregator; it will go green once the above clear.
Rebased on origin/main to pick up PR #2082's workflow SHA refresh, then folded fixes for the six failing checks (zizmor, lychee, 3x test flake symptoms, CI Pass) plus the three gemini-code-assist inline comments. lychee.yml: bump local action SHA 31a45a7 → 2592118 to match the rest of the workflows on main (the old SHA is no longer reachable in the repo, which is why zizmor's impostor-commit check fired across every workflow); rename version: to lycheeVersion: (the input name in v2 of lycheeverse/lychee-action — the old name was silently ignored, so the action ran default v0.23.0 instead of the pinned v0.24.2). renovate.json (gemini #1): \s+ → \s* so the binary-tool matcher catches unindented shell variables (LYCHEE_VERSION sits at column 0, not after YAML indent). Also add lycheeVersion: alternation to the action-input matcher so renovate keeps tracking the lychee bump path after the rename above. scripts/install_cli_tools.sh (gemini #2): handle GOPATH multi-entry strings — take the first colon/semicolon-separated entry, since go install writes to the first entry's bin/ and the raw joined string breaks mkdir -p. scripts/install_cli_tools.sh (gemini #3): switch tmpdir cleanup trap from RETURN to EXIT so set -e failures (failed curl / sha256sum / tar) still trigger cleanup; expand ${tmpdir} at trap-install time since the local goes out of scope before EXIT fires. lychee.toml: exclude dash.cloudflare.com (always 403 to unauthenticated probes — referenced from docs/guides/fork-setup.md as a navigation pointer) and docs.sigstore.dev (intermittent timeouts under the strict 10s ceiling — referenced from docs/guides/deployment.md). Test failures (Test Unit shard 4, Test E2E, Test Conformance SQLite) are pre-existing pytest_sessionstart cross-worker flakes — the os.abort() in tests/conftest.py is the documented forensic mechanism for capturing stacks when the FileLock races. The same flake hit main's prior CI run at 22:29 and self-healed on the next run at 23:51; re-pushing should re-trigger and likely clear them. CI Pass is the aggregator; it will go green once the above clear.
Rebased on origin/main to pick up PR #2082's workflow SHA refresh, then folded fixes for the six failing checks (zizmor, lychee, 3x test flake symptoms, CI Pass) plus the three gemini-code-assist inline comments. lychee.yml: bump local action SHA 31a45a7 → 2592118 to match the rest of the workflows on main (the old SHA is no longer reachable in the repo, which is why zizmor's impostor-commit check fired across every workflow); rename version: to lycheeVersion: (the input name in v2 of lycheeverse/lychee-action — the old name was silently ignored, so the action ran default v0.23.0 instead of the pinned v0.24.2). renovate.json (gemini #1): \s+ → \s* so the binary-tool matcher catches unindented shell variables (LYCHEE_VERSION sits at column 0, not after YAML indent). Also add lycheeVersion: alternation to the action-input matcher so renovate keeps tracking the lychee bump path after the rename above. scripts/install_cli_tools.sh (gemini #2): handle GOPATH multi-entry strings — take the first colon/semicolon-separated entry, since go install writes to the first entry's bin/ and the raw joined string breaks mkdir -p. scripts/install_cli_tools.sh (gemini #3): switch tmpdir cleanup trap from RETURN to EXIT so set -e failures (failed curl / sha256sum / tar) still trigger cleanup; expand ${tmpdir} at trap-install time since the local goes out of scope before EXIT fires. lychee.toml: exclude dash.cloudflare.com (always 403 to unauthenticated probes — referenced from docs/guides/fork-setup.md as a navigation pointer) and docs.sigstore.dev (intermittent timeouts under the strict 10s ceiling — referenced from docs/guides/deployment.md). Test failures (Test Unit shard 4, Test E2E, Test Conformance SQLite) are pre-existing pytest_sessionstart cross-worker flakes — the os.abort() in tests/conftest.py is the documented forensic mechanism for capturing stacks when the FileLock races. The same flake hit main's prior CI run at 22:29 and self-healed on the next run at 23:51; re-pushing should re-trigger and likely clear them. CI Pass is the aggregator; it will go green once the above clear.
1 participant
Summary
Breaks the real static cycle
stores/auth.ts -> api/endpoints/auth.ts -> api/client.ts -> (dyn) stores/auth.tsand drops thedpdm --skip-importsexemption that PR #2078 carried as a temporary patch (tracked in ADR-0006 "Exemption ledger").What changed
web/src/api/unauthorized-handler.tswithsetUnauthorizedHandler/notifyUnauthorized/_resetUnauthorizedHandlerForTests. Depends on nothing under@/.web/src/api/client.ts401 branch switched from dynamicimport('@/stores/auth').then(...).catch(...)to a staticnotifyUnauthorized()call. The.catchfallback is removed; with static imports the handler is wired before any HTTP call can land a 401 (viamain.tsx->App.tsx-> router -> guards ->stores/auth).web/src/stores/auth.tsregisters the closure at module init:setUnauthorizedHandler(() => useAuthStore.getState().handleUnauthorized()).web/package.jsonlint:circularsimplified todpdm --no-warning --no-tree --exit-code circular:1 src/main.tsx(no--skip-imports)..github/workflows/ci.ymldashboard-lintjob now runsnpm run lint:circular.Test plan
npm --prefix web run type-check: clean.npm --prefix web run lint: 0 warnings.npm --prefix web run lint:circular: exits 0 with no cycles (no--skip-imports).web/src/__tests__/api/no-circular-deps.test.ts: assertslint:circularhas no--skip-importsand that dpdm reports zero cycles. Invokes dpdm vianode node_modules/dpdm/lib/bin/dpdm.js(cross-platform, noshell: truedeprecation).web/src/__tests__/api/client.test.ts:329-372) covers the rewired path end-to-end.Review coverage
Reduced pre-PR review covering only the agents whose scope overlaps this change:
frontend-reviewer+code-reviewer. The two HIGH/CRITICAL flags raised (module-init race condition, closure memory leak) were verified false positives against the actual static-ESM import graph and Zustand's singleton store lifetime; no action items remained.Closes #2072