fix: one module answers to ./tenantService (#1049) - #1054
Conversation
`src/services/tenantService.js` and `src/services/tenantService.ts` both satisfied the extensionless specifier `./tenantService`. Vite resolves `.js` ahead of `.ts`, so `historicalDataService` — which calls `getTenantScopedDbName()` at module scope — resolved to the REST client, got `undefined`, and threw `TypeError: getTenantScopedDbName is not a function` before executing any of its own code. That took down every importer, and `npm run check:shadowing` (a blocking CI step, added for Aditya8369#990) failed on main. Folds the workspace REST client into `tenantService.ts` and deletes the `.js` file, so a single module exports both the scoping helpers and the API calls. While moving the client: - storage reads go through one guarded helper; the previous `localStorage.getItem('token')` throws outright in a Firefox private window - the Authorization header is omitted when no token is stored, rather than sending the literal `Bearer null` - path segments are encoded, so an id containing `/` cannot re-point the request - error bodies that are not JSON (a proxy's HTML 502) fall back to the status instead of throwing a SyntaxError over the original failure Also routes `TenantContext`'s two remaining bare `localStorage.getItem` calls through a guarded read. One of them runs inside a `useEffect` where nothing catches it, which is what broke the Aditya8369#843 unavailable-storage test. Fixes Aditya8369#1049
|
@MOHITKOURAV01 is attempting to deploy a commit to the Aditya Mahajan's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thank You for Your Contribution! 🎉Hi @MOHITKOURAV01, Thank you for opening this Pull Request and contributing to our project. We truly appreciate your efforts.
The maintainer @Aditya8369 will review your PR shortly! Happy Contributing! 🚀 |
|
@MOHITKOURAV01 conflicts |
…ce-shadowing # Conflicts: # src/services/tenantService.js
|
Merged The conflict was Resolved by keeping the delete. The consolidated After the merge: versus 17 failed / 1584 passed on |
…ce-shadowing # Conflicts: # src/services/tenantService.js # src/services/tenantService.ts
…arded read Aditya8369#1054 is open against this same file and independently added a guarded raw reader for the API-driven paths. Same fix, different name. Using its name and its comment verbatim means git's three-way merge sees both branches making the identical change rather than two competing ones, so two of the four conflict hunks between these branches disappear. No behaviour change. The two that remain are the real differences this PR is for -- the ref-based fetchTenants and the deleted mount effect -- and they are noted on the PR with the resolution to take.
Heads-up: overlaps with #1137 → #1142#1142 changes this same file. Both are mergeable into Short version: this PR's Either order works. Nothing needed on this PR. |
2 participants
Fixes #1049
The problem
src/services/tenantService.js(added with the workspace management work, #1031) andsrc/services/tenantService.ts(the scoping helpers, #759) both answer to../services/tenantService. Vite's defaultresolve.extensionsputs.jsahead of.ts, so the specifier landed on the REST client — which exports none of the scoping helpers.historicalDataService.jscalls them at module scope:so the import threw
TypeError: getTenantScopedDbName is not a functionbefore a single statement of that module ran, taking every importer down with it.npm run check:shadowing— a blocking step in CI'slintjob — was failing on main, so every open PR was red for a reason its author did not cause.The fix
The two halves are not really separate concerns: both answer "which organisation is this request for", one for local storage and one for the API. So they are now one module.
tenantService.tsgains the four REST functions (typed against the existingsrc/types/tenant.ts), andtenantService.jsis deleted.Nothing at any call site changed —
TenantContextandWorkspaceSettingsimport the same names from the same specifier as before.Picked up while moving the client
The four functions each did
localStorage.getItem('token')inline. That call throws in a Firefox private window and in a browser with site data blocked, so it is now a single guarded read. Three consequences worth naming:Bearer null. Interpolating a missing token producedAuthorization: Bearer null, which asks the server to reject a request an anonymous caller might have been allowed to make. The header is now omitted when there is no token.${tenantId}and${memberId}went into the URL raw; an id containing/or?re-points the request at a different route rather than 404ing.await response.json()on a failure assumed JSON. A proxy's HTML 502 threw aSyntaxErrorthat replaced the real error. It now falls back to the status.TenantContext.jsxalso had two barelocalStorage.getItemcalls left over, bypassing thereadStoredTenant()helper right above them. One is inside auseEffectwhere nothing catches it — that is what broke the #843 unavailable-storage test.Verification
Test files that were failing on
getTenantScopedDbName is not a functionand now pass:New cover:
src/services/tenantService.client.test.js— 12 tests. The assertion that matters most is the import at the top: both halves have to be reachable from one specifier. While the files shadowed each other, exactly one group was, and which one depended on the resolver.src/services/historicalDataService.tenantScoping.test.js— 5 tests, subject is the import itself plus the scoped database name. When every test in a directory fails at collection it reads as "the suite is broken" rather than "this one import is wrong", so it is worth having one test that says which.npx tsc --noEmitreports no errors for either changed file.Note for a follow-up
src/tests/services/tenantService.test.tsnever runs.vite.config.jshasinclude: ["src/**/*.{test,spec}.{js,jsx}"], which does not match.ts, so that file has been collected zero times since it was added — part of why this collision went unnoticed. Widening the glob is a separate change with its own fallout, so the new tests here are.js.