fix(share): invalidate stale short links - #1425
Conversation
|
Reviewed the diagnosis and the fix. The root cause is real and I reproduced it independently rather than reading it off the description. On main, useSharing.ts:300-307 sets isSharedRef once isSharedSession flips true, and isSharedSession is never set back to false, so short-link invalidation is dead for the whole life of an imported session. Since the paste is immutable by design (apps/paste-service/core/handler.ts has no update or delete route), the displayed URL keeps pointing at the original ciphertext no matter what you annotate. That is exactly the loop @grncdr described in #798. To make sure the new test guards the bug and not just the incidental StrictMode change, I applied only the incomingShortUrl capture to main's hook and kept main's old guard. The test still fails at the "short URL cleared after adding an annotation" assertion, so it is pinning the real regression. Layer and compatibility both look right to me. Nothing outside packages/ui is touched, so the Bun and Pi mirror rule is not engaged. sharing.ts is untouched, which means SharePayload, toShareable/fromShareable and the deflate plus base64url plus #key= pipeline are all unchanged, so existing links in the wild keep restoring. Guide share links and the PLANNOTATOR_SHARE=disabled path are unaffected. No dependency, lockfile, endpoint or new URL changes. The postCount assertions are the best part of the test: they pin that invalidation never uploads and never mutates the original paste. Three things I would like before merge:
Two smaller notes. The StrictMode rationale in the comment at :154-157 is development-only behavior; apps/portal does wrap in StrictMode, but the deployed production build never double-invokes the mount effect. The capture is still correct and is what makes the test deterministic, so this is a comment wording thing only. And the incoming-hydration tag at :316-323 is consume-on-next-effect; it is safe today only because fromShareable and setGlobalAttachments always return fresh arrays so the hydration commit always changes the request context. One line of comment there would keep a future refactor from quietly swallowing an invalidation. What I ran: the two useSharing files under DOM_TESTS=1 (5 pass, 33 assertions), the new file eight times for flake, the full typecheck across all nine projects including the strict-consumer config, the portal build, and the related registered DOM files (45 pass). The full bun test run has 12 failures, all in review-workspace.test.ts and the Pi server test; I checked out main in the same worktree and the same ones fail there, so they are environmental (no sem or but binary present) and unrelated to this change. Recommendation: take with changes. Items 1 and 2 before merge; item 3 as a follow-up issue so #798 can be closed the way the reporter described it. AI-assisted (Claude) under maintainer direction. |
|
Addressed every item from the maintainer review in commit bcf8935:
There were no inline review threads in the PR API; this comment responds to the single posted maintainer review item-by-item. Typecheck and portal build pass. Fresh CI is running now. |
|
CI follow-up: registering the lifecycle file in the existing 83-file Bun DOM invocation exposed deterministic Linux cross-file global-state interference in unrelated theme/highlight tests (the code-block test received another test's Kanagawa palette; both #798 lifecycle tests passed). The same 83-file command passed locally, but GitHub reproduced the unrelated failures on a failed-job rerun. Commit d570d93 keeps the new regression mandatory in the same |
|
Final CI resolution (superseding the adjacent-invocation detail in my prior comment): commit Final evidence: the exact isolated 83-file workflow command passes locally (685 tests, 2,916 assertions), GitHub Test passes, Release test/build/package and all cross-platform smoke jobs pass, and security checks pass. The PR is open, mergeable, and was not merged. |
|
Follow-up to the review in #1425 (comment) Recovery is complete on commit
Verification on the final head:
Important scope note: this does not fully resolve #798. Small plans still cannot mint a replacement short link after invalidation because No merge was performed. |
Closes #798 for stale-snapshot correctness. Follow-up #1427 tracks the remaining small-plan short-link creation limitation described below.
What changed
Root cause
The previous shared-session guard skipped short-link invalidation for the lifetime of an imported session, so Export could continue presenting the original immutable paste after local edits.
Product limitation and follow-up
This PR prevents Export from presenting an incorrect short URL. A small markdown plan now falls back to its correct fresh full hash URL after invalidation, but
ExportModalstill only offers Create short link when the full URL exceeds 2048 characters or hash sharing is unavailable. Therefore the literal request in #798 to always send a new short link back is only partially resolved for small plans. #1427 tracks adding an explicit replacement-short-link action without expanding this focused lifecycle fix.Verification
DOM_TESTS=1 bun test packages/ui/hooks/useSharing.shortUrlLifecycle.test.tsx packages/ui/hooks/useSharing.contentRevision.test.tsx— 6 passed, 44 assertionsbun run --cwd packages/ui typecheckbun run --cwd apps/portal buildtest.ymlparsed successfully; lifecycle DOM test registered exactly oncegit diff --checkSecurity and compatibility
Hosted pastes remain immutable and ciphertext-only. This change performs no update or delete request. Existing incoming links and payload formats remain compatible; only the client-side presentation lifecycle changes.