Skip to content

fix(share): allow explicit short links for small plans - #1475

Open
FNDEVVE wants to merge 3 commits into
backnotprop:mainfrom
FNDEVVE:fix/small-plan-short-links
Open

fix(share): allow explicit short links for small plans#1475
FNDEVVE wants to merge 3 commits into
backnotprop:mainfrom
FNDEVVE:fix/small-plan-short-links

Conversation

@FNDEVVE

@FNDEVVE FNDEVVE commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Fixes #1427.

Summary

Offer Create short link for every shareable small plan, not only after a previous link was invalidated. This resolves the issue's open UI choice without adding invalidation-history state.

The production change removes only the size-based action gate in ExportModal; large-URL warnings remain size-gated. Existing sharing capabilities, payload/encryption, immutable-paste lifecycle, and generation callback are unchanged. Creation still requires the explicit button click—no automatic uploads.

Verification

  • Rendered modal + real sharing-hook lifecycle regressions: 6 passed (DOM_TESTS=1, Bun 1.3.14). Cover fresh-small eligibility, stale incoming link invalidation, explicit replacement, decrypting current annotations, original-paste immutability, and disabled/unsupported capabilities.
  • Full bun test with repository-pinned Bun 1.3.14, disposable HOME, and local macOS environment (CI unset): 4,302 passed, 907 skipped, 0 failed. An earlier concurrent run hit the unrelated Pi stdio timeout; that test passed independently and the complete suite then passed.
  • bun run typecheck, git diff --check, and review → hook production builds: passed.
  • Actual browser smoke with local paste storage: create a link for a 95-byte full URL, open that encrypted snapshot, add a comment, verify the old link disappears with zero automatic uploads, then explicitly create a distinct replacement. Decryption contains the new comment; original ciphertext is byte-identical. No paste was uploaded to the public service.
  • Independent Standards and Spec reviews completed.

No pending PR addressing this issue was found in the open-PR inventory or issue timeline before starting; the inventory was rechecked before publication.

CI correction (3cbe5712)

Resolved the import-order bug behind #1464: storage now resolves the active data directory for every plan/history read and write instead of retaining an early test's temporary override. Added a regression for changing that override after import; no sleeps, retries, or weakened assertions. The ordered OpenCode→annotate reproduction fails four tests before this fix and passes after it on both macOS and Linux. Unprivileged Linux verification: 83 pass, 0 fail. Latest local full suite: 4,303 pass, 907 skip, 0 fail; typecheck passes.

Fixes backnotprop#1427. Make short-link creation available for every shareable small plan while preserving explicit uploads, immutable pastes, and current-content encryption.
@FNDEVVE

FNDEVVE commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

This was generated by AI during triage.

Correction to my earlier rerun advice: the failure is now reproduced and fixed in 3cbe5712. It is deterministic import-order contamination, not a permission/timing issue requiring a maintainer rerun.

packages/shared/storage.ts captured PLANNOTATOR_DATA_DIR at import time. An earlier OpenCode lifecycle test imported storage under a temporary override; after that test restored the environment, storage still wrote to the old directory while annotate fixtures used the current one. This explains all four failures in #1464, including the bypassed failure fixtures and missing submission records.

Minimal reproduction, confirmed on both macOS and Linux with Bun 1.3.14:

bun test apps/opencode-plugin/plan-review-lifecycle.test.ts packages/server/annotate.test.ts

Before: the exact four CI failures. The correction resolves all seven plan/history path constructions at use time. A regression changes data-directory overrides after import and checks isolated writes/read visibility; it fails against the original code. No polling, sleeps, skipped tests, or weakened assertions were added.

Verification: unprivileged Linux ordered three-file suite 83 pass, 0 fail; this branch's local full suite 4,303 pass, 907 skip, 0 fail, plus typecheck. Local full-suite environment uses a disposable HOME and the pinned Bun 1.3.14; upstream Actions verifies the Linux CI environment. Independent Standards and Spec reviews found no issues.

Final upstream verification: all checks on the corrected head are green (publication-only jobs are intentionally skipped on PRs). Test workflow and Release build/smoke workflow both passed, including Linux/macOS/Windows binary smoke checks. No maintainer rerun is needed.

Resolve plan and history paths from the active data-directory configuration instead of capturing a temporary test override at import time. This reproduces and fixes the four order-dependent annotate CI failures reported in backnotprop#1464; keep both successful persistence and failed-write draft recovery assertions intact.
@backnotprop

Copy link
Copy Markdown
Owner

Reviewed in depth - the feature itself passes: privacy contract holds (upload only ever fires from the button's onClick, no effect/retry path; PLANNOTATOR_SHARE=disabled suppresses the whole tab so the button can never render), revert-verification confirms the new tests genuinely guard the feature, and the stacked storage.ts change is byte-identical to #1473's.

One blocker before merge: the new test "offers an explicit replacement for an edited small incoming encrypted snapshot" is flaky - 6 failures in 10 runs of the exact CI invocation (DOM_TESTS=1 bun test --isolate). The race: the first waitFor(isLoadingShared === false && Boolean(shareUrl)) can release while shareUrl still holds the pre-hydration empty-document URL, and the post-edit waitFor(shareUrl !== hydratedShareUrl) then releases when the hydrated URL commits, before the edited URL exists - so the decompress at the end finds "Original document" instead of "Edited document".

Suggested fix: wait on content rather than URL inequality - poll until decompress(shareUrl) yields the expected markdown, both before capturing hydratedShareUrl and after the edit. The pre-existing "preserves an incoming short URL through hydration" test has the same latent shape and would be worth fixing in passing.

Two optional cosmetic notes, non-blocking: the small-plan button renders inside the amber warning container with no warning text (a neutral container might fit better when !urlIsLarge), and an empty session renders a silent no-op button (practically unreachable).

Happy to merge as soon as the flake is fixed - everything else is done.

@FNDEVVE

FNDEVVE commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 615e5f6e:

  1. Flake fix: added waitForSharedPayload to poll until decompress(shareUrl) yields the expected payload before capturing hydratedShareUrl and after the edit in offers an explicit replacement for an edited small incoming encrypted snapshot. Also updated preserves an incoming short URL through hydration, then invalidates immutable snapshots after material edits to wait on the hydrated and 2-annotation payloads rather than URL inequality.
    • Local verification: 20/20 runs passed under DOM_TESTS=1 bun test --isolate.
  2. Cosmetic styling: updated ExportModal so the short link button container uses a neutral style (bg-muted/50 border-border) when !urlIsLarge, reserving the amber container for URL length warnings.
  3. CI verification: all upstream checks green on commit 615e5f6e (Test suite, Release build, binary smoke on macOS/Linux/Windows, guides.show budgets and pinned manifest).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow minting a replacement short link for small shared plans

2 participants