Skip to content

chore(flags): retire phoenix-optimistic-edits — inline the optimistic edit path#3714

Merged
usirin merged 1 commit into
mainfrom
umut/3667-retire-optimistic-edits-flag
Jul 20, 2026
Merged

chore(flags): retire phoenix-optimistic-edits — inline the optimistic edit path#3714
usirin merged 1 commit into
mainfrom
umut/3667-retire-optimistic-edits-flag

Conversation

@usirin

@usirin usirin commented Jul 20, 2026

Copy link
Copy Markdown
Member

What

Retires the phoenix-optimistic-edits flag. It served on@100% in production (optimistic in-place content edits, #1675, epic #1637, released), so the useFlag gate and its flag-off fallback were dead weight. This rips both and keeps only the served-on behavior, then deletes the flag's IaC declaration (ADR 0136).

Changes

  • Gate + fallback ripped. useFlag(PHOENIX_OPTIMISTIC_EDITS, false) removed from the three call sites — apps/web/src/pages/PanoPostDetail.tsx (post.edit + CommentEditComposer) and apps/web/src/components/sozluk/DefinitionCard.tsx. Each now passes optimistic unconditionally instead of the ...(optimistic ? {optimistic} : {}) spread-away.
  • Builders un-gated. postEditOptimistic / bodyEditOptimistic in apps/web/src/fate/optimisticEdit.ts drop their enabled parameter and the | undefined return — the partial is always built.
  • IaC declaration deleted. The apps/web/src/flags/keys.ts const + its DECLARED_FLAGS entry; the OPTIMISTIC_EDITS_FLAG config, optimisticEditsFlag factory, import and re-export in apps/web/worker/features/flagship/resources.ts; the import + stack wiring in apps/web/alchemy.run.ts.
  • Invariant test deleted. apps/web/worker/features/flagship/optimistic-edits.invariant.test.ts pinned the retired flag's default-off IaC record, so it goes with the declaration. apps/web/src/fate/optimisticEdit.test.ts drops its two flag-gate cases and keeps the payload-shape coverage.
  • Stale prose reworded in .patterns/fate-mutations-client.md and .glossary/TERMS.md.

No Sentry/flag-tag fixture was repointed — none referenced this flag, so no retirement-set flag was touched.

Verification

  • git grep confirms zero surviving references to the flag string/const/factory in tracked source.
  • pnpm typecheck — 33/33 tasks pass.
  • pnpm lint (biome) — clean over the 8 changed files.
  • pnpm vitest run --project unit src/flags worker/features/flagship — 40 files, 202 tests pass, including every sibling *.invariant.test.ts.
  • optimisticEdit.test.ts — 3/3 pass.

Closes #3667

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

🚀 Preview deployed

  • web — Stage pr-3714 torn down.

@usirin
usirin force-pushed the umut/3667-retire-optimistic-edits-flag branch from eadfeac to fb3413e Compare July 20, 2026 02:42
@usirin

usirin commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

Repair: rebased onto origin/main (was mergeable_state=dirty).

New head: fb3413e04994a3ea3d331f7e656ac907af73ad62 (was eadfeac9). Retirement intent unchanged — still exactly the phoenix-optimistic-edits removal.

Conflicts resolved by union-of-removals. Five files conflicted against the sibling flag retirements that landed on main (pano-optimistic-*, pano-feed-edge-cache, phoenix-mod-queue, pano-base-feed). Each was reset to its origin/main version and only this PR's phoenix-optimistic-edits removals re-applied: .glossary/TERMS.md, apps/web/alchemy.run.ts, apps/web/src/flags/keys.ts, apps/web/src/pages/PanoPostDetail.tsx, apps/web/worker/features/flagship/resources.ts. No already-retired flag was revived (verified per-flag by diffing the grep hit-set against origin/main; the only delta is the intended phoenix-optimistic-edits drop).

One rebase-induced extra removal worth naming: in PanoPostDetail.tsx, main had already dropped the PANO_OPTIMISTIC_POST_DELETE useFlag call, so removing the last two useFlag sites leaves useFlag/flags/keys fully unused in that file — both imports are now deleted.

Verification

  • git grep for phoenix-optimistic-edits / phoenix_optimistic_edits / PHOENIX_OPTIMISTIC_EDITS / optimisticEditsFlag / OPTIMISTIC_EDITS_FLAG — zero hits in tracked source.
  • pnpm typecheck --force — 33/33 green, zero cache replays.
  • pnpm lint — clean.
  • apps/web unit project, src/fate/optimisticEdit.test.ts — 3/3 pass.

Reviewer note — flag-default/CI exposure (see #3720). OPTIMISTIC_EDITS_FLAG.defaultVariation was "off", and the three e2e specs that exercise these edits — tests/e2e/13-sozluk-edit-delete.spec.ts, 16-pano-edit-delete-post.spec.ts, 19-pano-edit-delete-comment.spec.ts — carry no phoenix_flag_overrides dev-override cookie (unlike the pano-base-feed retirement, where the cookie made this a non-issue). So CI has been asserting the flag-OFF (wait-for-server) world, and this PR makes the optimistic path unconditional there.

Assessed as low risk, but worth watching: every assertion in those specs is a timeout-bounded presence/absence check (toBeVisible({timeout: …}) on the edited text, toHaveCount(0) on the original), and the optimistic path only makes those transitions happen sooner, never later. No assertion depends on the edited text being absent before the round-trip. Flagging rather than pre-emptively rewriting fixtures, since there is no observed failure yet.

Acceptance criteria re-check against #3667 (the original coder could not read the issue body — GitHub was 503ing; it is readable now): all four AC met — gate/useFlag consumers and the dead fallback removed at all three call sites (PanoPostDetail post.edit + CommentEditComposer, sozluk DefinitionCard); the flagship/resources.ts IaC declaration and the flags/keys.ts key constant plus its DECLARED_FLAGS entry deleted, grep-clean under apps/web/; optimistic-edits.invariant.test.ts deleted and the optimisticEdit unit test's gate cases dropped with the behavior tests still green; typecheck and lint clean. No gaps found.

… edit path (#3667)

The flag served on@100% in production (optimistic in-place content edits,
weight. Rip both and keep only the served-on behavior (ADR 0136).

- `useFlag(PHOENIX_OPTIMISTIC_EDITS)` removed from the three call sites
  (`PanoPostDetail` post.edit + CommentEditComposer, sozluk `DefinitionCard`);
  each now passes `optimistic` unconditionally instead of spreading it away.
- `postEditOptimistic` / `bodyEditOptimistic` drop their `enabled` parameter and
  the `| undefined` return — the partial is always built.
- IaC declaration deleted: the `flags/keys.ts` const + `DECLARED_FLAGS` entry,
  the `flagship/resources.ts` config/factory/import/re-export, and the
  `alchemy.run.ts` import + stack wiring.
- `optimistic-edits.invariant.test.ts` deleted (it pinned the retired flag's
  default-off IaC record); the `optimisticEdit` unit test drops its gate cases.
- Stale prose reworded in `.patterns/fate-mutations-client.md` and
  `.glossary/TERMS.md`.

Closes #3667
@usirin
usirin force-pushed the umut/3667-retire-optimistic-edits-flag branch from fb3413e to 5207d3c Compare July 20, 2026 06:27
@usirin

usirin commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

review-code: PASS @ 5207d3c

Fresh verdict at the post-rebase head (ADR 0058 — any earlier verdict is stale). Verified read-only off a per-run ref; the merged tree (git merge-tree vs main@0753b877) is byte-identical to the PR head tree (a944bf8d) and main is an ancestor — a clean fast-forward, no merge drift.

AC (#3667 + ADR 0136)

  • Gate/useFlag consumers + dead fallback removed at all three call sites. PanoPostDetail.tsx (post.edit in PostContentInner, and CommentEditComposer) and sozluk/DefinitionCard.tsx. Each dropped its useFlag(PHOENIX_OPTIMISTIC_EDITS, false) read and the ...(optimistic ? {optimistic} : {}) spread-away fallback, now a direct optimistic: <builder>(…). DefinitionCard correctly retains its useFlag import for the still-live PHOENIX_OPTIMISTIC_DEFINITION_DELETE (feat(fate): optimistic definition.delete — instant term-page drop (per reconciliation ADR) #1681) — not a leftover; PanoPostDetail drops both imports since no other flag is read there.
  • Builders dropped enabled ENTIRELY — the retired state is unrepresentable. postEditOptimistic / bodyEditOptimistic in apps/web/src/fate/optimisticEdit.ts removed the leading enabled: boolean param and narrowed the return from T | undefined to T. This is the strong form the AC asked for: no caller can disable the optimistic path, and there is no undefined arm left to spread away — enforced by the type, not by a hardcoded true. The injectable now clock is preserved.
  • IaC deleted. flags/keys.ts: the PHOENIX_OPTIMISTIC_EDITS const + its DECLARED_FLAGS entry. flagship/resources.ts: the OPTIMISTIC_EDITS_FLAG record, the optimisticEditsFlag factory, the key import, and the re-export. alchemy.run.ts: the import + the yield* optimisticEditsFlag(flagship.appId) wiring + its comment.
  • Tests. optimistic-edits.invariant.test.ts deleted (27 lines). optimisticEdit.test.ts dropped both flag-off cases and the on/off framing, keeping the payload-shape + injected-clock assertions; result?.updatedAt tightened to direct access, correct under the now non-optional return.
  • Grep-clean. ZERO occurrences of phoenix-optimistic-edits, PHOENIX_OPTIMISTIC_EDITS, optimisticEditsFlag, OPTIMISTIC_EDITS_FLAG anywhere in the merged tree (not just under apps/web/).
  • typecheck / lintlint / format / typecheck green at this head.

Check 1 — union-of-removals vs the nine already-retired flags

No retired flag is resurrected as live wiring: keys.ts, flagship/resources.ts and alchemy.run.ts in the merged tree contain zero of the nine (#3658#3666). The only optimistic keys still declared are phoenix-optimistic-definition-add/-delete, which are active dark-ship flags (#3668/#3669, still open), not retired.

Residual textual mentions do survive, but all are prose/history/fixtures that pre-exist on main — ADR bodies (0088, 0134, 0145, 0173), .glossary/TERMS.md, .patterns/feature-flags-targeting.md, skill docs, packages/audit-stage/**, and reachability-guard unit-test fixtures. Diffing main → head, the only added line naming any of the nine is the pre-existing TERMS.md "optimistic slice" row, edited solely to drop this PR's own key. This PR therefore introduces zero new occurrences and removes nothing it shouldn't.

Follow-up, not a blocker on this PR: that TERMS.md row still lists pano-optimistic-submit and pano-optimistic-comment-add/-delete as live per-slice dark-ship flags, though #3659#3661 retired them. Pre-existing staleness inherited from those siblings; worth a separate doc-sync issue.

Check 3 — cross-reference durability

optimistic-definition-add.invariant.test.ts:6 repointed from the deleted optimistic-edits.invariant.test.ts to member-mute.invariant.test.ts. Target exists in the merged tree. member-mute is genuinely not in the milestone-34 retirement set (the set is #3658#3674; member-mute appears nowhere in it) and is still serving-off — MEMBER_MUTE is defaultValue: false with removal trigger "retire once on at 100% and stable" (#3112, epic #2035), so it cannot graduate in this batch. Pointer is durable. ZERO surviving optimistic-edits.invariant references repo-wide.

Check 4 — rebase hygiene

The export {…} collapse in flagship/resources.ts is a pure biome formatting consequence, not a dropped export. The four surviving names — PANO_DRAFT_SAVE, PHOENIX_BILDIRIM, PHOENIX_KARMA_GATES, PHOENIX_OPTIMISTIC_DEFINITION_ADD — are byte-identical to main; only PHOENIX_OPTIMISTIC_EDITS was removed, and 5→4 names fits the 100-col limit so biome reflows to one line. That accounts for the -42 vs -35.

Check 5 — the #3720 risk, adjudicated with CI evidence: RETIRED

The premise is real: OPTIMISTIC_EDITS_FLAG.defaultVariation was "off", and I confirmed none of the three specs sets a phoenix_flag_overrides dev-override cookie — so CI had been asserting the flag-OFF world, while this PR makes the optimistic path unconditional. Empirically that path is compatible. All six tests pass at this head (job 88286058601):

✓ 52 13-sozluk-edit-delete.spec.ts:29  author can edit and delete their own definition (8.0s)
✓ 53 13-sozluk-edit-delete.spec.ts:98  non-author does not see edit/delete buttons … (7.7s)
✓ 57 16-pano-edit-delete-post.spec.ts:22  author can edit and delete their own post (6.7s)
✓ 58 16-pano-edit-delete-post.spec.ts:91  non-author does not see edit/delete buttons … (8.7s)
✓ 61 19-pano-edit-delete-comment.spec.ts:24  author can edit a comment, delete a parent → [silindi], delete a leaf → removed (34.8s)
✓ 62 19-pano-edit-delete-comment.spec.ts:135 non-author does not see edit/delete buttons … (11.1s)

No fixture update is needed. Consistent with the prior pre-rebase observation, now re-confirmed at 5207d3ca.

Check 6 — CI diagnosis

42 check runs at head: 40 success, 2 skipped, 0 failure. Skips are correct no-op scopes (lint workflow YAML, packages unit tests — no matching changes). Required server-side all green: scan changed files for leaks, validate skill frontmatter, ci-required. e2e (reads + authed + flows, blocking) = success, 80 passed / 2 flaky / 0 failed. The 2 flaky (06-sozluk-home Base-UI portal re-render, addressed by ec92f9e; 22-pano-live two-client) both retried green and are not among the three edit specs — unrelated to this change. None of the known-infra classes (D1-429, paths-filter, crabbox 503, CodeQL init) even fired this run.

Behavior is byte-for-byte what serves:on@100% already delivers in prod. Not §CP — no changed path is CODEOWNERS-owned.

@usirin

usirin commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

review-doc: PASS @ 5207d3c

Fresh verdict at the post-rebase head (ADR 0058). Doc surface in this diff: .patterns/fate-mutations-client.md (+4/-5). (.glossary/TERMS.md classifies as has-code under §CLASS and is gated in the review-code verdict; noted here for completeness since it is prose in practice.)

.patterns/fate-mutations-client.md

  • Accurate to the shipped code. The worked example now reads optimistic: bodyEditOptimistic(body) with the {body, updatedAt: <now>} comment — matching the real post-retirement signature in apps/web/src/fate/optimisticEdit.ts exactly. The stale const optimistic = bodyEditOptimistic(optimisticEdits, body); // undefined when the flag is off line and the ...(optimistic ? {optimistic} : {}) spread are both gone, as is the "returns undefined when the gate is off, so the call site spreads it away under exactOptionalPropertyTypes" lead-in. No surviving claim that a gate exists.
  • History preserved, not erased. The closing line reframes to "Shipped dark behind a per-slice flag (feat(fate): optimistic in-place edits — post.edit, comment.edit, definition.edit #1675, epic Extend fate optimistic updates to content mutations (add/edit/delete), not just votes/toggles #1637), released, and the flag retired (ADR 0136) — the optimistic path is now unconditional." Correct doc-surface split per CLAUDE.md: .patterns/ states how the code is shaped now, with a pointer to the why rather than a re-derivation.
  • The blockquote is correspondingly fixed — "Votes and the in-place edits are both fully optimistic" replaces the flag-conditional phrasing, and the intra-doc anchor #optimistic-edits is retained and still resolves.
  • Links are repo-relative (../apps/web/src/fate/optimisticEdit.ts), standard markdown, no wikilinks, no placeholders. No home/local/absolute/sibling-repo paths introduced.
  • check docs have no dead internal links and check CLAUDE.md path pointers resolve are green at this head.

Docs match the code the PR actually ships. Not §CP.

@usirin

usirin commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

review-design: PASS @ 5207d3c

Fresh verdict at the post-rebase head (ADR 0058). Dispatched because class-probe resolves this diff as has-uiapps/web/src/pages/PanoPostDetail.tsx and apps/web/src/components/sozluk/DefinitionCard.tsx are on the rendered frontend surface (UI_RE = ^apps/web/src/, re-resolved live from ship-it/SKILL.md on main).

Rendered-surface delta: none against production

No JSX structure, no component swap, no CSS/class change, no design-token touch, no route change, no copy change. The Turkish user-facing strings ("tanım güncellenemedi", "başlık güncellenemedi", "yorum güncellenemedi", the EditedIndicator "düzenlendi") are all untouched — TR product copy preserved per the language rule.

The only user-visible property is edit latency, and it is unchanged relative to production: phoenix-optimistic-edits served on@100% in prod, so the optimistic in-place render on post.edit / comment.edit / definition.edit is already what users see today. This PR removes the ability to serve the off variant, not the rendered behavior. The "düzenlendi" indicator continues to be driven by the fresh updatedAt from the same builders, reconciled against the server live.update({changed:[…]}) frame on the same fields of the same entity — so no divergence and no flash was introduced.

Four-pillars / design-law

Nothing in this diff generates or restyles UI, so no pillar surface is engaged — no role-token annotation, component-selection, or per-pillar prohibition applies. check every component CSS file consumes the design-token seam and property-based a11y over the ui/ primitives are green at this head.

Empirical confirmation

The three edit-path e2e specs — which drive these exact surfaces with no flag-override cookie, i.e. the previously-flag-OFF world — all pass at this head (6/6, job 88286058601), including the reply-aware comment-delete [silindi] tombstone rendering and the non-author "no edit/delete buttons" authorization surface. The rendered edit/delete affordances behave correctly with the path unconditional.

Not §CP.

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.

Retire phoenix-optimistic-edits flag — rip FlagGate + fallback + IaC declaration (ADR 0136)

1 participant