client quality/docs (#66) - #95
Closed
Alimedhat000 wants to merge 34 commits into
Closed
Conversation
* docs: client quality initiative design spec * docs: client quality initiative implementation plan
* ci: trigger workflows for stacked client-quality branches * chore(client): wire vitest browser test runner * chore(client): remove empty orphaned test file * chore(client): scaffold components with autodocs and jsdoc stubs * ci: install playwright chromium before running client tests * ci: serialize root tests and disable client story file parallelism
* refactor(client): folder-per-component for layouts * refactor(client): kebab-case auth context files * refactor(client): normalize ui directory casing and dissolve Form/Input * refactor(client): fix DashboardMain casing, relocate shared NewDocumentFormBody * refactor(client): kebab-case hook filenames * refactor(client): kebab-case DocumentMain subtree and util filenames
* chore(client): enforce kebab-case filenames, drop stale shared ignores * chore(client): add jsdoc lint rule (warn)
* refactor(client): remove dead interceptor, split join-request api from hook * feat(client): expose error state from data hooks * docs(client): backfill JSDoc and add missing autodocs tags * chore(client): enforce jsdoc rule (error) with corrected export contexts * fix(server): clear collaborator/request rows before test document cleanup * fix(client): correct stale e2e API port fallback to 5001 * refactor(client): injectable provider factory in useCollab * docs(client): enforce JSDoc description/param quality via lint require-description on components/hooks/features kills empty stubs; require-param (destructured members exempt) covers hooks and feature helper files where params have no Props interface to document. Member- level prop docs stay a review convention. Policy recorded in AGENTS.md. * docs(client): backfill prop-level JSDoc across components/hooks/features Every custom component prop gets a one-line member doc (defaults noted when implementation-established); inherited React/Radix props left undocumented; hooks and feature helpers get full @PARAM tags.
* test(client): global router/auth/modal story decorators; drop redundant per-story routers * test(client): stories for spinner, seo head, layouts and auth/create forms with play coverage * test(client): stories for new-document modal and card dropdown with portal-aware plays * test(client): status bar and toolbar stories with real codemirror views * test(client): editor cluster stories; prewarm optimizeDeps for story tests * test(client): deflake modal form validation assertion (existence over visibility) * ci: prebundle react-dom/client for storybook tests * fix(client): editor view reached toolbar/status as null; harden story assertions markdown-editor held the CodeMirror view in a ref but passed it as a prop during render — refs don't re-render, so MarkdownToolbar/MarkdownStatusBar received null on mount and stayed inert until an unrelated toggle. Hold the view in state instead. Story fixes so green means verified: - DocumentMain stories dial a real (never-connecting) websocket; inject a seeded offline provider via the new createProvider prop and assert the seeded content actually renders; add missing DocumentData fields - editor Editable asserts live status bar (3 lines, non-zero length) - Head stories assert document.title/meta instead of mount-only - per-story fn() mocks: shared module-scope mocks made .not.toHaveBeenCalled() order-dependent under filtered reruns - register form: accessible-name button query, DOM error assertion - toolbar: destroy stale unattached views; null-view story asserts no-op
* ci: drop temporary client-quality branch triggers * docs: adr 0001 defer auth token consolidation
…ain) (#74) * Security: exact-ID doc resolution + WS authentication (#45, #24) (#59) * fix: resolve documents by exact ID instead of unordered prefix match * feat: authenticate collaboration websocket and enforce permissions * fix(server): restrict collaborators listing to owners and stop leaking credentials (#58) * fix(server): restrict collaborators listing to owners and stop leaking credentials Closes #43 * fix(server): allow editors to list collaborators on shared documents The owner-only restriction from #43 broke the editor UX: legitimate edit collaborators could no longer see who they share a document with. Relax listing to owners and edit-permission collaborators while keeping the sanitized select (no credentials) and denying viewers and unrelated users with 403. Part of #61 (RBAC follow-up). * feat(client): show collaborator roster to editors and view-only badge - Collaborators dropdown now renders for owners and edit collaborators; remove buttons stay owner-only (server enforces this too) - View-only collaborators get a 'View only' badge in the toolbar slot, explaining why editing is disabled, instead of silently missing controls - Drop the redundant isCollaborator skip-guard in useCollaborators — fetch whenever mounted with a docId; mount gating lives in the toolbar Part of #61 (RBAC follow-up). * feat(client): inject collaborators into dropdown stories via optional prop Stories previously passed a 'collaborators' arg the component ignored (it fetches via useCollaborators(docId), needing a live API). Add an optional collaborators override prop so Storybook can demo the roster rendering added for owners/editors, including owner-only remove buttons.
* chore: ignore local git worktrees directory * fix(server): remove leftover debug console.log calls Closes #36
* fix(server): remove leftover debug console.log calls Drops two debug console.log statements in document.controller.ts (share-token decoding and collaborator removal) plus the now-unused chalk import and a commented-out log in getClientInfo.ts. Both spots already emit structured winston logs. Closes #36 * feat(server): rate-limit auth endpoints Adds express-rate-limit (10 requests / 15 min / IP) on register, login and refresh to protect against brute force. closes #37 * fix(server): force-enable limiter in rate-limit tests The shared authLimiter skips outside production and vitest runs with NODE_ENV=test, so the limiter was disabled inside its own tests and CI saw 200s instead of 429s. Override skip via the factory in the tests.
* chore: ignore local git worktrees directory * fix(server): cascade-delete collaborators and join requests Documents with Collaborator or CollaborationRequest rows could not be deleted: both FKs were RESTRICT, so DELETE /api/document/:id returned 500 for any shared doc. Cascade from Document like YjsDocumentState already does. Regression test covers the previously untested path. closes #44
* chore: ignore local git worktrees directory * fix(client): copy the selected permission's share link, not a stale one The effect is now the single fetch trigger on permission change, fetches are last-request-wins, and Copy is disabled while a link loads. Covered by an e2e test that copies during a slowed share-link request. Fixes #48 * fix(client): invalidate previous share link whenever a new fetch starts A failed refetch left the old permission's link in state: Copy re-enabled with the select showing the new mode, reintroducing the #48 symptom via the error path. shareLink is now cleared at fetch start, so after a failure (or mid-load) nothing stale is displayed or copyable.
* fix(collab): accept email when adding collaborators
Client posted {email} while the server expected {userId}, so the
add-collaborator flow 500'd on Prisma errors; the UI was commented
out hiding the breakage.
- POST /:id/collaborators now takes {email, permission?} (zod
validated), resolves the user server-side
- 404 unknown email, 409 duplicate, 400 malformed body
- restore add-by-email form (owner only), controlled dropdown state
fixes chevron stuck rotated via global window.open
- addCollaborator hook returns success so input clears only on success
- drop debug console.log leftovers
Closes #50
* fix(server): map P2002 race on duplicate collaborator insert to 409
Two concurrent adds can both pass the findUnique pre-check; the
loser hit the raw unique-constraint error and surfaced as 500.
Wrap the create and translate PrismaClientKnownRequestError P2002
into ConflictError.
* fix(server): normalize email case at validation boundaries
Closes #82
* fix(server): make Yjs sole writer of Document.content REST update/create could overwrite the Yjs-derived content mirror, silently discarding collaborative edits (last-writer-wins). Strip content from both endpoints; also write snapshot + mirror atomically in dbPersistence.store so they cannot drift on partial failure. Closes #47 * fix(client): stop sending content in document save payload
Document routes accepted unvalidated bodies and params: malformed ids surfaced Prisma errors as 500s, non-boolean flags reached the DB layer, and title/permission values were never length- or enum-checked. Auth schemas also lagged client-side bounds (server allowed 6-char passwords while the client requires 8). Add body/param/query schemas wired through the existing validate() middleware for every /document route, replace the manual share-link permission check with a query enum schema, and harden register/login/ addCollaborator bounds (email <=254, username <=50 + charset, password 8..128, fullName <=100). Mirror those bounds in the client (zod schemas + maxLength attributes) so users never hit a 400. Closes #52. Related: #37 (rate limiting), #83 (enumeration hardening).
Registration now returns a single generic conflict message regardless of
which field collided, and login returns an identical 401 body with a
non-empty generic message for both unknown-user and bad-password cases
(previously the body serialized to {}). A dummy bcrypt compare equalizes
response timing when the user does not exist.
Logout previously required a valid access token, so an expired session could not log out (the exact moment logout matters). Reuse the validateRefreshToken middleware, which already populates req.user for the controller to revoke the stored token.
The refresh endpoint set an httpOnly accessToken cookie that no client ever reads; the access token already travels in the response body.
SameSite=None + Secure is only valid for cross-site HTTPS deployments; in dev the client and API are same-site over plain http, where Secure cookies get dropped and None requires TLS. Use Lax/insecure outside production, None/Secure in production.
Add a response interceptor that refreshes the access token once and replays the failed request. Concurrent 401s share one in-flight refresh; a request whose token was already rotated by a concurrent request replays directly instead of triggering a second round-trip. When the refresh itself fails, the stored token is cleared and listeners are notified so the auth provider can reset state. Also add a node-env vitest unit project so shared client lib code has a test runner (the existing vitest setup only ran Storybook tests).
Bootstrap now restores the session via the shared single-flight refresh and simply marks the user signed out when it fails — it no longer calls the authenticated logout endpoint (which always 401'd and threw an unhandled rejection). Remove the wasLoggedOut localStorage flag that kept logging users out after a logout->login cycle, swallow logout errors when the session is already dead server-side, and reset auth state when the interceptor reports an unrecoverable session expiry. Add e2e coverage for logout->re-login->reload session restore and for not calling logout during an unauthenticated bootstrap; serialize auth specs since real logins overwrite the user's single stored refresh token.
clearCookie must mirror the cookie's Secure/SameSite/Path/HttpOnly attributes, otherwise browsers retain the prod SameSite=None; Secure cookie after logout and subsequent refresh still succeeds.
Verifies that logout clears both refreshToken and legacy accessToken cookies with the same Path/HttpOnly/SameSite attributes used on set (otherwise prod SameSite=None; Secure cookies survive logout and refresh still succeeds), and that replaying the old cookie after logout is rejected. Would have failed before 4f8cdf5.
Concurrent 401s sharing one refreshPromise notified listeners per waiter (3×) and could replay with Bearer null after clearAccessToken. Move clear+notify into refreshPromise rejection (once) and guard the current !== _tokenUsed branch to throw when current is null instead of asserting non-null.
The synced-scroll toggle suppressed mirrored-pane echoes with a flag reset in a fresh requestAnimationFrame. Chromium dispatches the mirrored pane's scroll event before that frame, but Firefox delivers it after the reset: the echo then passes the guard and the next genuine update is swallowed, so the preview trails and snaps back continuously while scrolling. Replace the timing flag with position-based echo suppression (an event at the last-written offset is an echo) and batch mirror writes to one per animation frame. Handlers are now stable callbacks, so listeners stop resubscribing on every render. Covered by interaction stories: baseline mirroring both directions plus a deterministic late-echo regression test.
Chromium coalesces same-frame scroll events, so no existing story failed on the old guard when two updates landed in one frame. Deliver the first update, then add a second within the same frame (Firefox's per-write delivery): the old code swallows the second and the mirror settles stale; the new batched mirror converges on the latest position.
useCollaborators only reset its error inside the initial fetch effect, so a failed add or remove left 'Failed to add/remove collaborator' showing indefinitely — even after a successful retry or closing/reopening the dropdown. Reset the error at the start of each action, mirroring the fetch effect. Adds a browser-mode vitest project for hook-level tests (stories can't exercise API-dependent logic) with coverage for error clearing on successful retries.
Replace bare await act(async () => {}) flushes with a polling waitFor
that waits for loading to settle. Empty act flushes can miss the
initial fetch's microtask in CI timing.
unit (node) matched src/**/*.test.{ts,tsx} and browser-unit matched
src/**/__tests__/*.test.{ts,tsx}, so files under __tests__ ran in both
projects: use-collaborators.test needs document (fails in node) and
api.test imports node:http (fails in browser). Scope browser-unit to
src/hooks/__tests__/** and exclude that path from unit so each file runs
once in its intended environment.
Fixes failing Lint, Type Check and Test run 33129323962 on develop
(4× document is not defined + 1× node:http externalized).
- Add Session model (id, userId, jti, refreshToken, expiresAt) with migration 20260828010125_add_session; allows multiple concurrent refresh tokens per user instead of single User.refreshToken column that logged out other devices on each login. - Refresh rotation (TDD): login creates Session with jti + 24h expiry; POST /auth/refresh verifies jti, rotates to new jti/token, sets env-aware cookie (lax/secure), invalidates old token and keeps legacy User.refreshToken in sync. Old token replay → 401. New token works. Covers #51.1 stolen-token lifetime + reuse surface (reuse now just 401; full revoke-all on reuse can be added once migrated). - Multi-device (TDD): second login creates second Session without overwriting first; logout deletes only presented Session, leaving other device intact. Fixes single-column overwrite. - isActive enforcement (TDD): login rejects deactivated (401 generic), refresh rejects deactivated before rotation, authenticate middleware now async and checks isActive via prisma so deactivated JWTs are rejected within 15m. Closes #51.2. - Cookie flags: refresh rotation sets sameSite/secure per NODE_ENV (already done for login/logout in #92); logout clears only presented Session and both cookies with matching Path/SameSite/HttpOnly. Tests: 3 new suites (rotation, isActive, multi-device) — 6 tests — all green with existing 77 (83 total). Test setup now truncates sessions table.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
docs: client quality initiative design spec
docs: client quality initiative implementation plan