Repository navigation
feat(ui): add contribution editor workflow - #20
Conversation
|
Vercel preview is ready: https://hivelore-66btoqc8w-kareemn1936s-projects.vercel.app Commit: 7b69208 |
|
@KareemN1936 this is your strongest PR so far, and it shows you took the earlier feedback on board: it wires to real, permission-enforced endpoints (I verified Most importantly you got the canon model right: no status select, kind is LORE/STORY only, and the sole state transition is Must fix: 1. 🔴 Data-loss bug — 2. 🔴 The Stories tab creates the wrong kind — 3. 🟠 Permission gate is still "is there a token". 4. 🔴 Route collision with your own #18 — needs resolving before either merges. #18 and #20 both rewrite 5. Mirror the 100 KB Nits: no draft loading, so Sequencing: this is a third PR stacked on #16 while #14 is still stalled — the chain is #14 → #16 → {#18, #20}, with #19 on a separate branch off #14. I'd rather you didn't open more stacked work until #14 lands; if you're blocked, say so and we'll unblock it from our side rather than have you build further on a moving base. |
|
@mcfarhat Pushed fixes in Addressed:
Validation passed: web typecheck, web lint, web build. |
|
@KareemN1936 re-reviewed at Fixed, and fixed properly1. Stale-draft snapshot on Submit. 3. Permission gate. I checked the reasoning, not just the presence of the preflight: 5. The 100 KB cap. Client constant matches the server's exactly, and the measurement matches byte-for-byte ( Double-submit is also properly guarded at both ends — client button plus a status-guarded The same bug class survives in two narrower formsA. Save Draft stays enabled during an in-flight submit. B. An emptied summary can never be cleared. ...(summary.trim() ? { summary: summary.trim() } : {}),The server only acts when a field is Partial2. Contribution kinds. Looking at the API, I think you may be stuck: Reported fixed, but not4. Route collision with #18. Renaming Whichever merges second still conflicts. Since #18 is now base-only for #21, the cleanest fix is to rebase this PR after #21 lands rather than trying to keep them independent. Should-fixPreflight failure is permanent. NitsThe Still open from earlier and not scoped out: reload still creates a second draft row rather than resuming — and you now call the exact One thing worth knowing
|
|
@mcfarhat Pushed Addressed the latest editor review follow-ups:
Verification:
I’ll rebase this after #21 lands to resolve the remaining stacked route/page overlap. |
|
@KareemN1936 re-reviewed at 1. Save disabled during in-flight submit — FIXED, and both halves are right. Minor leftover: the inputs stay editable during submit, so keystrokes typed after clicking Submit are displayed but never sent. Lost-edit UX, not corruption — a 2. Cleared summary sends I traced the whole round trip rather than just the client change: the PATCH schema accepts nullable, the handler spreads Small regression to tidy: both keys now go out on every save even when unchanged, so 3. Unsupported categories — PARTIAL, and your backend statement is correct. I verified it: The warning banner is the right immediate move and it satisfies what I actually asked for — no longer silent. It's partial only because the behaviour is unchanged: the category is still discarded and the draft is still LORE, it's just disclosed now. Two follow-ups: the banner doesn't name the rejected type even though 4. Route collision with #18 — still there, and you said as much. I didn't infer it from filenames; I ran the merge:
Not claimed, still openThe preflight is the one worth fixing next: Also unchanged: the Coverage
All three claims are substantively honest. Fix the preflight and tidy the leftovers, and this is ready once the rebase can happen. |
|
Addressed the latest review follow-ups in 61f7bf4:
Also opened the Trello follow-up card for typed contribution categories beyond LORE/STORY. Quality and Vercel checks are green. The remaining route-collision item is still waiting on the PR #21 dependency/rebase path. |
|
@KareemN1936 verified at Preflight now distinguishes transient from denied. The new Input locking covers all five controls — title, type select, target id, summary, and the rich-text editor. I checked the last one actually takes effect rather than just receiving a prop: One new gap, and it's the same class as the bug you just fixedA 401 isn't a 403, so it lands in Related, low: on a 502/504 from a proxy the body is HTML, so Two small things on the fixes themselvesThe banner prints the raw slug ( Input locking triggers on Still open
Sequencing has changed — worth knowing before you plan the rebaseI ran the trial merge rather than comparing filenames: the
That means the #20 rebase is further out than "wait for #21" — #18 has to be un-conflicted first. Might be worth resolving #18 next, since three PRs are queued behind it. |
|
@mcfarhat Pushed Addressed the latest PR #20 follow-ups:
Validation:
Leaving the remaining low-priority items scoped out for now: proposal:null fallback, audit-log noise, draft resume, and the route conflict/rebase sequencing with #18. |
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
@KareemN1936 verified b74ede77 — driven in a real browser against a stub API, not read. All three claimed items are genuinely fixed, and I could see them working:
- 401 → reauth.
/worlds/w401/contributerenders "Sign in again" withhref="/login?next=%2Fworlds%2Fw401%2Fcontribute". 403 still lands indeniedwith no retry, which is right. - 502 with an HTML body now reads "Contribution access could not be checked" instead of the old "not saved" mislabel.
- Reflected
?type=closed.?type=%3Cscript%3Ehackrenders no attacker text;?type=charactersrenders "Characters".
Typecheck, eslint, prettier and next build all clean.
But the 401 fix is half a fix, and the half that's missing is the half where a user loses work.
Blockers
A — the "Sign in" link you just added is a dead end
apps/web/src/app/login/page.tsx is, in full:
import Home from '@/app/page';
export default function LoginPage() {
return <Home />;
}It never reads searchParams, so your ?next= is discarded. And nothing clears the expired access token on a 401 — auth-session-provider.tsx:41-51 only clears when a refresh fails — so the provider still holds user. I loaded /login?next=%2Fworlds%2Fw401%2Fcontribute and it rendered "Signed in as @kareem / Refresh Session / Log Out".
So: idle fifteen minutes → reauth card → click "Sign in" → a page telling you you're already signed in, no form, no way back to the editor.
The better fix is already in the hook you import. refreshSession() uses the refresh token, whose TTL is 14 days against the access token's 900 seconds. On a 401 you can silently recover instead of asking the user to re-sign with Keychain. That single change also fixes B below.
B — the same 401 gap survives on the save path, where there's an actual draft to lose
I grepped all of apps/web: the only 401 branch anywhere is contribution-editor-form.tsx:150, in the preflight. So the write path is untouched. Verified in the browser: preflight passes on a fresh token, POST /contributions returns 401, and you get a red "Contribution was not saved / Authentication required." with the form still editable, no reauth path, no retry — and the draft exists only in memory.
That's strictly worse than the preflight case you just fixed. There, the form was empty. Here, an author has written lore for fifteen-plus minutes, and the only sign-in affordance available (site-header.tsx:30, /login, no next) throws the draft away.
Calling refreshSession() and retrying once covers both paths.
Should-fix
404 is still mapped to the transient state. /worlds/w404/contribute gives "Could not check contribution access / World not found." with a Retry that can never succeed. Same class as the 401 bug, one status along — a permanent error wearing a transient error's UI. Treat it as terminal.
The banner sentence is broken by the label swap. ?type=characters → "The requested Characters is not supported yet"; unknown → "The requested requested category is not supported yet", with the word duplicated. The prose at :303-305 still expects a bare slug.
The no-token denied branch (:118-122) is unreachable — the sibling effect at :107-111 calls router.replace('/login') first, and without next. So ?next= now exists in one of the two sign-in paths and is honoured in neither.
One to be aware of before you wire any auto-refresh: :361 unmounts the whole form subtree whenever status ≠ allowed. Any accessToken change re-enters checking, TipTap remounts with initialContent='' while the parent keeps the old content state, and onJsonChange doesn't fire on mount — so the editor would look empty with stale state behind it. Not reachable today, but it makes the refresh fix above unsafe unless the subtree survives.
Merge sequencing — unchanged, and slightly worse
I ran the trial merges rather than comparing filenames. pr20 ← pr18 still conflicts on contribute/page.tsx, and b74ede77 edited that file inside the conflicting hunk, so the conflict is now slightly larger than at 61f7bf4. And #18 is still CONFLICTING against its own base, so the rebase remains gated on fixing #18 first.
Coverage
apps/web has zero test files and no test script, so nothing here is covered — and ci.yml has no test step, so even the API suite can't gate a merge. For the record I ran the API suite: 99 pass / 8 fail, all pre-existing refreshSession-stub failures in contributions.test.ts, not yours.
Fix A and B — realistically one change, refreshSession() on 401 in both places — and this is done.
|
@mcfarhat Pushed Addressed the latest contribution editor blockers:
Validation:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
@KareemN1936 reviewed b971fe7 with two passes, both driving a real browser against a stub API with the same auth semantics as ours (rotating refresh cookie, reuse detection, rotation invalidating prior access tokens).
The happy path works, and you covered more than your comment claimed. runWithAuthRetry wraps create, update and submit — not just the preflight — and the retry is correctly bounded to one attempt: with a permanently-401ing POST and a healthy refresh, the stub saw create:2, refresh:2 then stopped cleanly. The /login dead end is genuinely gone: the provider now clears token and user, so it renders the real Keychain form instead of "Signed in as @kareem". 404 is terminal now, and the banner sentence is fixed.
First, a correction to my own last review. I predicted the failure mode as "TipTap remounts empty while the parent's content keeps the old document" — a silent saves-invisible-content hazard. That was wrong. Measured on a 4 KB draft: title and draft survive, and the parent's content is also reset to the empty document. So there's no state mismatch to find. I flag this because it matters for how you check it: looking for the mismatch I described and not finding one would suggest the area is fine. It isn't — the body is simply gone.
Blocker — this commit turns a recoverable error into destroyed work
Run against your parent commit b74ede7 with identical inputs:
b74ede7: editorPresent = true
editorText = "DELTA-UNSAVED-WORK-30-MINUTES-OF-WRITING"
alert = "Contribution was not saved / Authentication required."
b971fe7: EDITOR_REMOVED html:"DELTA-UNSAVED-WORK-30-MINUTES-OF-WRITING"
server persisted: []
available actions: "Sign in" (navigates away), "Back to World"
The chain: save 401 → runWithAuthRetry calls refreshSession() → it throws → auth-session-provider.tsx:46-49 clears accessToken on any refresh error → the effect re-runs → !accessToken sets reauth (:112-116) → :408 unmounts the whole subtree. The old build kept the text on screen so a copy-paste rescue existed. Now there's no Retry, and the only button navigates away.
And it fires on a transient blip with a perfectly valid session. Aborting the /auth/refresh request at the network layer gives "Contribution was not saved / Failed to fetch", editor unmounted, draft gone — then reloading works fine, proving the session was never invalid. refreshSession treats network errors, 429s and 500s as auth failures and clears the session.
Your :121-123 guard (currentStatus === 'allowed' ? currentStatus : 'checking') does work — but only on the success leg, which is exactly the case a reviewer would test. Every failure leg still unmounts.
Fix direction: once status has been allowed, render the reauth/error card above a still-mounted form rather than replacing it, and don't re-run the preflight just because the token changed — key the effect on worldId + permissionRetryKey and read the token from a ref. There's also no beforeunload guard anywhere, so the "Back to World" links discard silently too.
Blocker — the refresh has no ceiling, and it can loop
I checked: there is no attempt counter, no backoff and no cap anywhere around the refresh. If the access-check 401s for a reason a refresh can't fix (revoked sid, JWT secret skew during a rolling deploy, clock skew), the effect and the preflight feed each other, because a successful refresh changes accessToken, which re-runs the effect:
t≈3s refresh calls = 417 list calls = 827
t≈15s refresh calls = 1749 list calls = 3423
UI: "Checking contribution access..." forever
~115 rotations/second from one tab. The only brake in production is the server's 40/min cap on /auth/refresh — and since the limiter is Prisma-backed, every iteration is DB writes. When the cap trips, 429 isn't 401, so refreshSession throws, and the user is logged out. The key is per-IP, so shared-NAT users take collateral damage.
Blocker — two tabs revoke the whole session family
Two contribute tabs with a stale token, both saving:
refresh(rt-12) <- tab A
refresh(rt-12) <- tab B, 3ms later
refresh_ok rt-12 -> rt-14
REUSE_DETECTED_FAMILY_REVOKED family=fam-9
PERSISTED = []
Both tabs lose their drafts, nothing saves, and reloading doesn't recover — the family is dead and it needs a fresh Keychain sign-in. On the real server it's worse than the stub, because routes/auth.ts:309 also clears the browser cookie on the reuse path.
To be clear: the server is correct here. auth-sessions.ts:184-186 and the atomic updateMany claim are exactly right. The bug is that the client has no single-flight dedupe — runWithAuthRetry:244 and the effect's :145 both call refreshSession() freely. One shared in-flight promise in the provider fixes this, the loop above, and the stale-closure issue below, all at once.
Blocker — two rapid Submit clicks create two proposals
No double-submit protection at all. saveDraft (:249) has no isSaving guard, handleSave guards only isSubmitting, and a second click in the same task still reads isSubmitting === false because the disabled prop hasn't committed. Against a clean server:
| action | result |
|---|---|
| two rapid Save Draft | c1:DRAFT, c2:DRAFT — c1 orphaned |
| Save + Submit together | c1:DRAFT, c2:SUBMITTED |
| two rapid Submit Proposal | c1:SUBMITTED, c2:SUBMITTED — two duplicate contributions in the canon-voting queue |
That last one puts duplicates in front of voters. A ref-based guard at the top of saveDraft fixes all four.
The retry you added is mostly raced out by its own effect
Sequencing the authenticated GETs as [403, 200]: the retry inside the 401 catch received the 403, and the UI ignored it and rendered allowed. Because refreshSession() mutates provider state, the effect cleanup fires controller.abort() on the same controller the retry is using, so its result is dropped by the !controller.signal.aborted guards — recovery actually comes from the effect re-running on the new token. What the retry reliably contributes is a duplicate preflight GET per refresh and a silently swallowed 403. Also catch { } at :157-164 swallows everything and always reports "Your session expired", which is then overwritten by the re-run's message.
Should-fix
?next= is still inert — app/login/page.tsx is untouched, still return <Home />, and nothing in app/page.tsx reads useSearchParams or navigates after setSession. The recovery affordance the card advertises doesn't exist.
State machine: checking has no timeout and no escape (no AbortSignal.timeout on the preflight), so a hung API leaves it on screen indefinitely. denied is terminal with no Retry, and it's overloaded — a 404 sets it, so the card reads "Contribution access unavailable" over "World not found." reauth is terminal and over-triggered by any network/5xx/429.
Server error strings reach users verbatim — one tab displayed "Refresh token reuse detected.", a security-internal message, via error.body?.error.
The preflight still discards the draft it fetches (:131), so there's still no resume — and that's now the only thing that could recover what the unmount destroys. RichTextEditor's initialContent prop exists and is never passed by the form, and useEditor doesn't re-sync when it changes, so resume isn't wired even latently.
Clean, and worth saying
Server-side validation is solid — content is properly validated (100 KB cap, doc root, array children) rather than trusted, targetLoreEntryId is verified against the world, bodies are .strict(), and submit re-validates. I looked specifically for client data being trusted where the server should check, and found none.
Merge sequencing
Unchanged: contribute/page.tsx still conflicts whole-file with #18, and #18 is still CONFLICTING against its own base, so the rebase stays gated on fixing #18.
The single highest-value change is keeping the form mounted once it's been allowed. That alone converts three of these blockers from data loss into a visible banner.
|
@mcfarhat Pushed Addressed the latest auth-recovery blockers:
Validation:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Reviewed at c6642e2 (the merge commit; eec8785 is its second parent). Two independent passes, both driving the real Next 15 app in headless Chrome against a fault-injecting API — real typing into TipTap, real cookie jar, real BroadcastChannel, real Web Locks. Then every guard was deleted to confirm the failure reappears.
All five fixes hold. And the mutation testing turned up something worth telling you, because it means you're crediting the wrong mechanism in two places:
- Draft survival is defence in depth, and the guard you lead with isn't what does it. Removing only the sticky render guard at
:408→ draft still survived. Reverting only the effect (puttingaccessTokenback in the deps at:217, dropping thehasAllowedAccessRefshort-circuit at:127-129) → draft still survived. Reverting both → draft lost, original bug reproduced exactly. The load-bearing fix is the effect no longer re-running on token change. - Cross-tab: the Web Lock is what saves you, not the broadcast. Removing the lock (
auth-session-provider.tsx:82-86) → reuse detected, family revoked, a session cleared. Removing the broadcast (:91-94) → no behavioural change at all. The broadcast is a UX nicety; the lock plus the shared cookie jar is the actual protection. - The double-submit guard specifically requires a synchronous ref. Deleting
operationInFlightRef→ 2 contributions and 2 submits, reproducing the duplicate proposals. Replacing it with anisSaving/isSubmittingstate check → also 2 contributions. Good instinct; worth a comment in the code saying why state won't do.
Refresh storm: measured over 15s under persistent failure it's now 1–3 requests where it was 417 in 3s. Reverting the effect deps reproduces the storm at 41 refreshes in 8s and trips the 40/min cap, so that fix is doing real work. Double-submit: 2 clicks, 10 clicks, 30ms and 120ms spacing, Save+Submit racing — every case produced exactly 1 contribution.
Three things to fix, and they're all at the edges of the new mechanisms rather than in the old bug.
1. BLOCKER — one stalled tab freezes every other tab, permanently
navigator.locks.request(AUTH_REFRESH_LOCK, cb) is called with no signal, no timeout, no ifAvailable (auth-session-provider.tsx:82-86), and lib/api/client.ts:82 does a bare fetch with no timeout anywhere in the stack. A refresh that never settles holds the lock forever.
Reproduced: tab A loaded with /auth/refresh hanging → navigator.locks.query() shows held: ["hivelore-auth-refresh"]. Made the API healthy, opened tab B → tab B sat on "Checking contribution access…" for the entire observation window, and the API logged only 1 refresh call total, proving tab B never reached the network. It's blocked purely on the lock. Because isSessionLoading gates on that promise, the whole app is dead in tab B — and if that's the tab with the editor, the form never mounts, so you can't even start typing.
A crashed tab is fine (Web Locks release on context death). The failure mode is a live-but-stalled tab: server hang, load balancer holding the connection, mobile dead-air. This is new in this PR — the pre-fix code had no lock at all.
Fix: AbortSignal.timeout() on the lock request plus a fetch timeout, and fall through to an unlocked refresh on abort.
2. BLOCKER — a terminal refresh failure is classified as transient, so the user retries forever
Your classifier clears the session only on ApiError && status === 401 (:97-107). Reuse and invalid-token both return 401, so that part is right. But apps/api/src/routes/auth.ts:286-290 returns 403 "Request origin is not allowed." whenever the refresh cookie is used and Origin doesn't exactly equal env.CORS_ORIGIN — a single exact match, no list. That's terminal: retrying can never succeed.
Reproduced with contributions 401 + refresh 403 — three consecutive Save clicks, each POST /contributions → 401 → POST /auth/refresh → 403, each showing "Could not refresh your session. Your draft is still on this page; try again before leaving." Save stayed enabled, no sign-in link ever appeared, session never cleared.
This repo deploys to Vercel, so the realistic triggers are everyday: preview deployments on generated URLs, www vs apex, a domain alias, a proxy rewriting Origin, or a stale CORS_ORIGIN. Every one becomes a permanent "try again" loop instead of "sign in again". Treat 403 from /auth/refresh as terminal.
3. BLOCKER-ish — the draft is no longer wiped, but it's now stranded with no way out
Both passes hit this independently. After a real session death mid-writing: form stays mounted, title and body intact — the fix works. But canSave requires accessToken (:244-256), which the provider just nulled, so Save Draft and Submit Proposal are both permanently disabled. And permissionStatus is still 'allowed', so the reauth card carrying the ?next= sign-in link (:438-487) never renders. The only in-form action left is "Back to World", which navigates away and destroys the work.
So the UI instructs an action it doesn't offer. Your cross-tab work does provide a real rescue path — signing in in a new tab broadcasts session-refreshed, Save re-enables, and the draft saves — but nothing tells the user that's the move. Rendering the sign-in affordance in this state, with target="_blank", closes it almost for free.
Not blocking
- There's no actual refresh ceiling — no attempt counter, no backoff. The storm is gone because there's one refresh per trigger, but the remaining trigger is the user: 60 Retry clicks over 2.4s produced 81 refresh requests, 41 rate-limited, enough to trip the server cap and log them out. Roughly 20 clicks/minute does it, and mashing Retry is the natural response to an error banner.
beforeunloadnags after a successful save.:219-236computeshasUnsavedWorkfrom content presence, not dirtiness against the last saved snapshot, and nothing resets it after save. Verified: typed → saved successfully → reload → warning fires. Training users to click through the prompt makes the prompt worthless when it matters.- Client-side navigation still destroys the draft silently. Typed a draft, clicked the form's own "Back to World" link: no dialog (App Router
Linkdoesn't firebeforeunload), went back, form empty. Same for the header "Sign in" link — the exact link the error in #3 tells people to use. - Save+Submit in the same tick silently swallows the Submit. Verified: 1 contribution created, 0 submit calls, status stays DRAFT, no error shown — the guard
return nulls with no feedback, so the user believes they submitted. - A saved draft isn't resumable. After reload the saved draft isn't loaded back and the next save issues another create, so each page load orphans a duplicate.
listContributionsat:153is used purely as a permission probe and its results discarded. Combined with thebeforeunloadnag, this is a trap. - The broadcast applies an inbound session without checking it's the same user. If another account signs in elsewhere in the browser, this tab silently re-arms with the new identity and the next Save creates the draft under their account (the server sets
authorIdfrom the token). The cookie is per-browser so the replacement is inherent — but the form staying armed isn't. Compare user ids on inbound broadcast and freeze the form if they differ.
Two process items
On rebase, drop the 12 apps/api files — I compared the copies rather than assuming: yours add no exported symbols Perla's lack, the difference is an inlined WorldWithRelations type and a publicWorldInclude const. But two things there are uniquely yours and must survive: the two publishedAt: new Date() lines in prisma/seed.ts, and the migration 20260810133000_publish_initial_world_bibles.
Your commit fe6e038 deletes apps/web/src/lib/api/session.ts, which #21 still imports in four components — and git applies that delete with no conflict, so merging the two produces a guaranteed build break that shows nothing in the merge output. I've asked Kareem to migrate those importers on #21; flagging it here so whichever lands second doesn't get blamed for it.
Measured merge order across every combination is #22 → #21 → #20, so this one lands last — which is fine, since the three blockers above are all small and localised.
One ask for the next round: apps/web has no test script at all. This PR adds 928 lines of async auth-failure logic and every behaviour above had to be verified by hand in a browser. Even a couple of tests around the in-flight guard and the transient/terminal classifier would pay for themselves immediately.
|
Pushed Covered:
Verified locally:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Re-reviewed at a0930bc6. Real production bundle (next build + next start) driven in headless Chrome against a fault-injecting API, with c6642e2 built and served alongside as a baseline so I could prove the harness reproduces the original three before trusting it on the fixes.
Blockers 2 and 3 — properly closed
403 is now terminal, and it didn't over-correct. Save with write 401 → refresh 403, three clicks:
baseline c6642e2 : 3 refreshes, 3 create attempts, "try again" each time,
Save/Submit still enabled, no sign-in affordance <- the unwinnable loop
head a0930bc6 : click 1 = 1 refresh + 1 create; clicks 2 and 3 = ZERO requests
Author -> "Sign in required", recovery card shown, draft intact
And the important half — it didn't swing too far. refresh → 429, 500 and 503 each keep the session alive, Save and Submit stay enabled, one refresh per click, no recovery card, no sign-out. That's exactly right: 403 from /auth/refresh can only be the origin check (apps/api/src/routes/auth.ts:286-289), so treating it as terminal is correct, and everything else stays transient.
The stranded draft is fixed and the rescue works end to end. Baseline: the "sign in again" element is absent, Save disabled, draft on screen — the state that instructed an action it didn't offer. Head: element present and laid out (718×24, visible), linking to /login?next=%2Fworlds%2FW1%2Fcontribute with target=_blank. Signing in in another tab applied the broadcast in 106 ms, the card disappeared, Save re-enabled, and a subsequent save created exactly 1 contribution with the draft text byte-identical.
All five original fixes still hold — I re-verified rather than assumed. Draft survives four separate failure legs with title, summary and body intact; transient failures preserve the session; no storm (2 refreshes in a 15s window under a 401ing preflight); the Web Lock still prevents family revocation; and 10 rapid Submit clicks still produce exactly 1 contribution — with a rebuilt mutant swapping operationInFlightRef for isSaving || isSubmitting state producing 10 creates and 10 submits, so the synchronous ref is confirmed load-bearing.
Blocker 1 — half closed
The permanent freeze is genuinely gone, and that's real progress:
baseline : tab B stuck 40s, API logged 1 refresh call (never reached the network)
head : tab B clears in 6,857 ms -> usable editor (API healthy)
tab B clears in 17,641 ms -> "Sign in again" card (API dead)
But the fallback that's supposed to make lock contention harmless is dead code.
createTimeoutSignal prefers AbortSignal.timeout(ms) when available — which is every modern browser. That aborts with TimeoutError. isAbortError (auth-session-provider.tsx:48-50) tests error.name === 'AbortError'. It never matches, so return performRefresh() at :107 never executes and the catch rethrows.
I checked this rather than assumed, because the AbortController branch further down would produce AbortError — but that branch only runs on browsers without AbortSignal.timeout, i.e. effectively never:
AbortSignal.timeout(10) -> reason.name = "TimeoutError" isAbortError -> false
controller.abort() -> reason.name = "AbortError" isAbortError -> true (legacy path only)
Measured consequence, 2 of 2 runs, 10,178 ms each: with a squatter holding hivelore-auth-refresh, a healthy API and a valid session, the tab waits 10.2s, makes zero network refresh calls, and lands on "Sign in again." So the fix bounds the freeze by falsely signing out a user whose session was fine.
Please don't just widen the check, though — there's a sharp edge behind it. apps/api/src/lib/auth-sessions.ts:184-185 revokes the whole session family on refresh-token reuse, and right now the lock is the only thing preventing concurrent redemptions. If TimeoutError simply falls through to performRefresh(), a timed-out tab refreshes unlocked and concurrently with the lock holder — which is precisely the family-revocation bug fix #4 was written to prevent. The fallback needs to re-check latestSessionRef (or decline to redeem) rather than just stop throwing.
Related measurement worth having: the lock serializes, it doesn't dedupe. Two simultaneous tabs → 2 refresh calls (t=529ms, t=937ms, strictly non-overlapping so the second carries the rotated cookie); six simultaneous tabs → only 2, because later waiters receive the broadcast in time. The latestSessionRef short-circuit loses the race because the lock releases before postMessage fires (:114-120). Harmless today, but it means safety rests entirely on serialization — which is the same thing the fallback would undermine.
Second half of blocker 1: the timeout is only on /auth/refresh. lib/api/client.ts still has no default — it only forwards a caller-supplied signal (:74-76). Executed:
- preflight
GET /worlds/:id/contributionshangs → "Checking contribution access…" still spinning at 45,000 ms POSTcreate hangs after Save → button stuckaria-busy="true", disabled, no error banner, still stuck at 45,000 ms
My original wording was "no timeout anywhere in the stack", and that's still true for every call except refresh. A default timeout in request() covers all of it at once.
Non-blocking items — none were addressed
Not asking for them now, but so they're not lost:
- No attempt ceiling. 60 Retry clicks under
preflight 401 → refresh 429produced 120 requests, all rate-limited, Retry still enabled, no cooldown and no message. (Was 81/41 — same class, higher count.) beforeunloadnags after a successful save. Dispatched a cancelable event:defaultPreventedistruewhile dirty and stilltrueafter a successful save.- Client-side navigation destroys the draft — the in-app "Back to World"
<Link>(:660-665) navigates with no dialog and the draft is gone. - Save+Submit in one tick silently swallows the Submit — 1 create, 0 submits, badge stays DRAFT, and no message of any kind.
- The broadcast applies any inbound session. Posted a foreign session (
user-999/someoneelse/platformRole: ADMIN) from another same-origin tab and the editor's Author flipped@kareem → @someoneelsewith zero network calls.
Gates
typecheck 0, lint 0, next build 0, API tests 107/107 pass (up from develop's 89). All green.
apps/web still has no test script — its package.json scripts block is byte-identical to develop's and there are zero test files under apps/web/src. Every behaviour in this review had to be established by hand in a browser, twice now. That's the thing I'd most like to see before the internship ends: even a handful of tests around the in-flight guard, the terminal/transient classifier and the lock fallback would have caught the TimeoutError mismatch immediately.
Two fixes and this is done — the TimeoutError handling (with the revocation interaction thought through) and a default timeout in request().
|
Pushed Covered:
Validation:
|
|
Merged latest Conflict choices:
Validation after the merge:
|
e332416 to
7125b9a
Compare
|
Final pushed head is Remote checks are green now:
The PR is clean/mergeable against |
7125b9a to
0efbf64
Compare
|
Process correction: I reset this branch back to Reason: Kareem clarified that approved work should be merged by the reviewer/DR, not by me. I opened revert PR #24 to undo my #21 merge through the normal PR process. This PR is back to the contribution-editor/auth request timeout work only. |
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Re-reviewed at 798ce9b9. Real production build in headless Chrome against a fault-injecting API, with develop built alongside as a baseline.
Part B — the default request timeout: CLOSED
DEFAULT_REQUEST_TIMEOUT_MS = 30_000 in client.ts:13, applied inside request() so it covers every verb:
hung preflight GET : 45,000 unbounded -> 30,056 ms, then "Could not check contribution access" + Retry
hung POST save : 45,000 unbounded -> 30,041 ms, error banner, Save re-enabled, draft preserved
That's properly fixed. Two small things: the raw DOMException text "signal timed out" is rendered verbatim to users in both banners, and 30s is a long time to stare at a spinner in an editor — 10–15s would be kinder.
And the five original fixes all still hold, re-verified: draft survives every failure leg, transient failures keep the session with Save and Submit enabled, one refresh call in a 15s window, two tabs on the healthy path make one server refresh, and 10 rapid Submits still produce exactly one contribution — with the mutation (sync ref → isSaving state) giving 10/10/10, so the ref is confirmed load-bearing again.
🔴 The request timeout introduces whole-family session revocation. develop does not have this.
This is the one to fix, and it's more severe than anything this PR set out to solve. One tab. No lock contention.
1. refresh takes 12s; client aborts at 10s -> "Sign in again" at 10,910 ms
2. server completes the rotation anyway -> "ok"; browser never receives the rotated cookie
3. next page load replays the old token -> rotatedAt set -> RefreshTokenReuseError
-> revokeSessionFamily()
measured: load 2 settled 111 ms, server log ["ok","REUSE_FAMILY_REVOKED"],
reuseDetected=1, familyRevoked=true
develop, identical scenario: ["ok","ok"], reuseDetected=0, familyRevoked=false
I verified the mechanism in the source rather than taking the measurement on trust:
PR auth-session-provider.tsx:99 signal: createTimeoutSignal(AUTH_REFRESH_REQUEST_TIMEOUT_MS) // 10s
develop 0 occurrences of that constant
server auth-sessions.ts:184 if (session.rotatedAt || session.revokedAt) {
await revokeSessionFamily(...); throw RefreshTokenReuseError }
develop has no client-side abort, so it waits and receives the rotated cookie. Your 10s abort creates a window where the server has rotated but the client hasn't heard — and the server treats the next use of that token as theft.
The previous freeze was one tab stuck for 10.2s with a false sign-out. This is permanent server-side revocation of the whole family, across every tab and device, triggered by one slow refresh. Worth being clear that this constant predates your two newest commits — it just had never been measured.
Whatever you do about the timeout, the redemption itself needs to be atomic from the client's point of view: either don't abort the refresh request at all (accept the wait, as develop does), or make an aborted refresh not leave a rotated-but-unreceived token — e.g. have the server only mark rotatedAt once the response is acknowledged, or give the client a way to re-fetch the current token for its family rather than replaying the old one.
🟠 Part A — the fallback fires now, but your scenario is unchanged
isAbortError matches TimeoutError (:49-53), and I can see the fallback running from the timing — every lock-timeout path now settles ~750ms later, which is exactly AUTH_REFRESH_BROADCAST_WAIT_MS. But:
squatter on the lock + healthy API + valid session, 2/2 runs
previous round this round
settle time 10,178 ms 10,965 / 10,869 ms
refresh calls 0 0
outcome "Sign in again" "Sign in again"
usable editor no no
The fallback was changed from performRefresh() to waitForBroadcastedSession(), and on a cold mount latestSessionRef.current is null — only setSession populates it — so both branches at :106-118 fail and :120 throws. Net effect: 750ms more freeze, same false sign-out.
On the warm path it's arguably worse: it returns latestSessionRef.current where the access token equals the one that just 401'd, so the caller retries with the same stale token (measured createCalls=2, 0 refresh calls, save failed with the original error). It "recovers" to a token that cannot work.
To answer my own earlier question: it neither re-checks-then-redeems nor falls through — it declines to redeem entirely. That does keep the lock invariant safe (0 refresh calls, no reuse, no revocation under contention), so the caution was heeded. It just doesn't recover the user.
Also, :106-112 and :113-118 are logically identical — !== and === both return latestSessionRef.current. The whole thing reduces to if (ref) return ref.
A correction I owe you from last round
I told you the effect-dependency change was what saves the draft and the render guard wasn't. That was my sloppy compression of "either alone suffices". This round's mutation is cleaner: re-adding accessToken to the deps at :217 did not destroy the draft — the render guard at :408 covers it. So the honest split is the dep change is load-bearing for the refresh count, the render guard for draft survival. Both are doing work, on different problems. Sorry for the misleading steer.
Merge state
CONFLICTING with develop, 4 files — because develop has moved (#21 landed, was reverted, then restored):
auth-session-provider.tsx— add/add, 5 hunks.developindependently implemented the Web Lock, without timeouts and with a module-levelrefreshAuthSessionOnce. Genuine semantic overlap.contribute/page.tsx— whole-file, and it's a product question rather than a merge.developrendersLoreContributeClient; this PR rendersContributionEditorForm. Mohammad's ruling was to follow the spec: §8.4 is one editor with an entry-type selector and a Submit Proposal button, and Journey B has a character going through the vote. So the per-type fields and relationship picker should feed the contribution/proposal flow rather than creating a lore entry directly. That's the shape to resolve toward.worlds/[worldId]/page.tsxandworlds.test.ts— one hunk each.
Still unaddressed, still non-blocking
No attempt ceiling (60 Retry clicks → 60 requests), beforeunload firing after a successful save, in-app <Link> navigation destroying the draft with no dialog, Save+Submit in one tick silently swallowing the Submit, and the broadcast applying a session for a different user (posting one for "mallory" flipped the author from @kareem).
The ask I keep making
apps/web still has no test script — fourth round where every one of these behaviours had to be established by hand in a browser. All five original fixes and both new items are cheaply testable. Leen landed a CI build gate on hive-freelance today that goes red for all seven workspaces; worth a look at what she did, because this file is the strongest argument in the org for having one.
|
Changes requested on latest head The green gates hold locally:
Still blocking:
Non-blocking but worth tightening while you are in here: timeout errors currently surface raw text like I tried to submit this as a formal Request Changes review, but GitHub rejected it because the authenticated CLI user is the PR author. |
|
@KareemN1936 your refresh-abort finding is correct, and Mohammad has ruled on the fix: a grace window. Thank you for raising it — and for being straight about why it came through as a comment rather than a review. I verified the mechanism end to end rather than take it on trust: So the chain is exactly as you described: abort at 10s → the server may already have rotated → the client keeps the old token → the next refresh presents a rotated token → indistinguishable from replay → the whole session family is revoked. A slow mobile connection becomes a sign-out on every device. There's a second path at The reason this is a good catch rather than an obvious one: aborting a request is normally the defensive choice. It's unsafe here only because refresh-token redemption is single-use and stateful server-side, so abandoning it isn't free — and the server has no way to tell an abandoned redemption from a stolen token. The decision: grace window on rotationThe fix goes server-side, in
That keeps reuse detection meaningful while making an aborted or lost response survivable. Recording the successor id on the rotated row is the usual way to implement it. This is backend work and it isn't yours — I'm carding it separately rather than parking your PR behind it. What I'd like in #20Since the grace window won't exist yet when this merges, don't ship the hazard in the meantime: remove the abort from the refresh-token redemption path specifically ( If you'd rather keep a ceiling on that request, make it long enough that it can't fire before the server has realistically finished (and add a comment saying why the number exists), but removing it is simpler and strictly safer until the grace window lands. Your other two items from 11:00 stand: the cold-mount case in |
|
@KareemN1936 a heads-up on the conflict state, because it has moved since you reported it and you'd have no way of knowing that from your side. You flagged this yourself at 11:01 yesterday, naming GitHub reports Two things worth knowing from that: The conflict has shifted from source files to test files. Twenty-one commits behind is the more useful number. Rather than resolving conflicts against a snapshot, merge And the contribute-page conflict is still not yours to guess at. You said it needs a product-level resolution toward the contribution/proposal editor rather than a mechanical pick, and I agree — don't resolve that one by choosing a side. If it's still conflicting after you merge Nothing else changes on this PR. Your three items from yesterday stand, and the refresh-abort one has an answer now: Mohammad's decision is a grace window on rotation, implemented server-side, carded separately at https://trello.com/c/TUQMVcP2 so it isn't yours. The interim ask on this PR is unchanged — drop the abort from the refresh-redemption path specifically ( |
798ce9b to
82ee56d
Compare
|
Pushed the rebase/review follow-up in Covered:
Verified locally:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Changes requested at 82ee56d7 — two changes close this
First, the thing I warned you about: you heeded it. Zero files deleted. Verified four independent ways, all empty — --diff-filter=D, a scoped diff over components/lore/, a net-line-loss check, and a tree comparison. All five files under apps/web/src/components/lore/ are byte-identical to develop. I mutation-tested those checks (deleted a lore file, gutted another) and all four went red, so they're sensitive rather than vacuously passing.
The branch is also genuinely rebased — 0 behind, 12 ahead, git merge origin/develop says "Already up to date". Every gate is green on both sides: lint, typecheck, build, format, and 122/122 API tests matching develop exactly.
But the lore authoring UI is dead anyway, and no file-level check catches it
You didn't move the lore editor to /lore/edit — you overwrote contribute/page.tsx with yours. So LoreContributeClient now has zero importers, and LoreEntryForm is imported only by it. That's 686 lines of the lore feature preserved on disk and unreachable from any route. next build and eslint are both perfectly happy, which is exactly why it slipped past.
Same URL, both real production builds:
develop /worlds/W1/contribute?type=characters
-> "Create lore entity ... Entity title / Entity type /
Character, City/Kingdom, Faction, Quest, Historical Event, Story"
this PR same URL
-> "Contribution type not supported yet"
A warning if you spot-check this: curl /worlds/W1/lore/edit returns 200, not 404 — the dynamic [entryId] segment swallows the literal "edit" and renders the entry-detail page for a nonexistent id. The route genuinely does not exist (it's absent from the build route table), but a status-code check would tell you otherwise.
The four links are all still pointing at /contribute
I drove each one:
lore-entity-tabs.tsx:210"Add {category}" →?type=characters→ "Contribution type not supported yet"lore-entry-detail-client.tsx:187"Extend this lore" →?type=CHARACTER→ same walllore-entity-tabs.tsx:188"Edit" andlore-entry-detail-client.tsx:181"Suggest edit" →?entryId=X→ silently dropped
That last pair is the nastiest. Your page reads targetLoreEntryId, not entryId. Proven from the RSC payload: with ?targetLoreEntryId=abc123 the prop is bound; with ?entryId=abc123 it is absent. So "Edit this entry" opens a blank, unbound new contribution — no error, no warning, and the user's edit target is silently lost.
Also: ?type=stories-contributions, the real category id for stories, also hits the unsupported wall. Only the literal ?type=stories/story works, so the single story link that functions is the one on the world hub.
auth-session-provider.tsx — this is the strongest work in the PR
You genuinely reconciled it rather than picking a side. Develop's Web Lock, its module-level refreshAuthSessionOnce singleton and the BroadcastChannel all survive, with your lock timeout and non-terminal-error handling layered on top. Every removed line is accounted for and intentional. You also fixed the identical-branches bug I flagged at :106-118 — waitForBroadcastedSession now really compares against startingAccessToken instead of both legs returning the same ref. And isTerminalRefreshError correctly narrows session clearing to 401/403, so a transient blip no longer signs people out.
But the commit titled "avoid aborting auth refresh redemption" doesn't
You removed AUTH_REFRESH_REQUEST_TIMEOUT_MS and its dedicated 10-second abort. But DEFAULT_REQUEST_TIMEOUT_MS = 30_000 (apps/web/src/lib/api/client.ts:13), which you added earlier in this same PR, is applied unconditionally to every request at client.ts:113-116 — including POST /auth/refresh. The redemption is still aborted, just at 30s instead of 10s.
Measured against a stub that holds /auth/refresh for 32s, real production builds in headless Chrome:
this PR : refresh received t=12.974s
>>> CLIENT ABORTED at t=42.972s <<< (exactly 29.998s)
rotation COMMITTED
could NOT deliver rotated cookie (BrokenPipeError)
develop : refresh received t=7.325s
client still connected -> NO abort
rotation COMMITTED, cookie delivered
Isolating mutation: I rebuilt your branch with that one constant raised to 120_000 and changed nothing else — the abort disappeared and the cookie was delivered. So the constant is the sole cause, not Chrome and not my harness.
That's the same rotated-but-unreceived window from the 11 Aug review: the server sets rotatedAt, the client never receives the new cookie, the next use replays the old token, and auth-sessions.ts:184-186 throws RefreshTokenReuseError and calls revokeSessionFamily(). A real user gets signed out on every device. The window is narrower than before, but it's still a regression against develop.
The agreed grace window was never implemented — apps/api/src/lib/auth-sessions.ts is untouched; the only apps/api changes in this PR are a data migration and two lines in seed.ts. There's no "recent rotation + unused successor" check anywhere.
Genuine token reuse still revokes the family, which is correct and unchanged — that test passes.
Two smaller things
refreshAuthSession(options) gained a signal parameter that nothing passes — dead surface. And the lock timeout (10s) is now shorter than the request timeout (30s), so a waiting tab can fail spuriously while the holder is still legitimately refreshing; minor, since it no longer signs the user out.
Not yours, but worth a ticket: prisma migrate deploy can't bootstrap a fresh database on develop — 20260718154842_init fails with type "LoreType" already exists, created by 20260718131500_init_hivelore_schema. Two init migrations collide. I also checked whether your migration's timestamp ordering caused a problem by baselining a real Postgres at develop's state; it applies cleanly. Not a finding.
983 lines added, zero new tests — fifth round with that ask outstanding.
Two changes close this:
- Create
apps/web/src/app/worlds/[worldId]/lore/edit/page.tsxwith develop's 13-line page, and repoint the four links. That's the option-C item that didn't land. - Exempt
/auth/refreshfromDEFAULT_REQUEST_TIMEOUT_MS, or implement the server-side grace window.
The second is the one that can revoke a real user's session family across every device, so if you only do one, do that.
|
Pushed the two requested fixes in Done:
Verified locally:
The build route table now includes |
|
Follow-up: pushed |
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Approving. Reviewed at eefa356 against the code itself, not the diff summary.
What's good
- Refresh lock timeout + broadcast fallback.
navigator.locks.requestwith a 10s abort signal, falling back towaitForBroadcastedSession(), is the right shape. A tab that can't get the lock now waits for the winner to broadcast instead of hanging or duplicating the refresh. combineSignals(). Correctly handles the already-aborted case up front, unwraps the single-signal case instead of allocating, prefersAbortSignal.anywhere available, and degrades cleanly. Preserving the abortreasonthrough the fallback is a nice touch that's easy to miss.- Narrowing the initial-load
catchto 401/403. This is a real improvement: a transient network error no longer wipes the user's session. I specifically checked whether it could strand the app on a spinner — it can't,.finally()clearsisSessionLoadingunconditionally. error.body?.error ?? '<fallback>'throughout. Optional chaining plus a fallback on every path means a malformed error body can't surface asundefinedin the UI.- 100KB content limit is enforced on UTF-8 byte length rather than string length, which is the correct check.
One earlier concern, withdrawn
A review pass flagged that ApiError might not carry a usable body.error. That's wrong and I'm recording it so it doesn't resurface: ApiError is defined in apps/web/src/lib/api/errors.ts with status, statusText and body: ApiErrorBody | null, and ApiErrorBody.error?: string. Every call site here uses it correctly. No change needed.
Non-blocking nits
createTimeoutSignal()fallback —setTimeout(() => controller.abort(), ms)is never cleared, so on browsers withoutAbortSignal.timeouta timer survives for up to 30s after the request settles. Harmless, but returning a cleanup or usingAbortSignal.timeoutexclusively would tidy it.combineSignals()fallback — theabortlisteners are{ once: true }but never removed when the request completes normally, so the controller is retained until the caller's signal is collected. Same legacy-only path.
Neither is worth another round.
On tests
contribution-editor-form.tsx has no tests, and for a 673-line component doing session recovery, draft state and permission checks I'd normally push back. I'm not blocking on it here, because apps/web has no test infrastructure at all — no runner, no testing dependencies, zero test files — so this is a gap the project never set up, not something you skipped. Holding your PR to a bar no previous web PR has met wouldn't be fair.
I've raised it as its own card. When the harness lands, the three cases worth covering first are: draft save/load, a token refresh landing mid-save, and the 403 permission-denied path.
Nice work on the concurrency handling in particular — that's the part most people get wrong.
2 participants
No description provided.