From c33de1175b0c608498e9a4f4422cd29f42c7ccc4 Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:28:37 -0700 Subject: [PATCH 1/3] Render deterministic rule hits instantly, not on the LLM's clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rule pack in prose.ts already found "a apple" — articles, pluralAfterNumber, subjectVerbAgreement, uncountables are all there, and they already surface as real mechanics cards. But mechanicsFindings only ran inside runCheck, behind the same 4s checkpoint debounce as the model, and only reached the screen via the server's reply. A free, instant, offline-capable detection was being delivered on an LLM-shaped delay. The rule pack now runs on its own 250ms fuse and renders its findings with no network at all, as provisional cards. The mechanics submit follows; its reply is authoritative and clears them. If the reply never comes — offline, server down — the cards simply stay, which is the whole point of having rules that need no model. Provisional cards are keyed by wording rather than position, so one can't flicker into a duplicate of its own persisted twin while she types around it. resolveServerId maps a card to the row the API can act on, awaiting the in-flight submit, so accepting inside that window still records the keep and plants its word in the garden instead of being quietly dropped; null means there is no row and the edit has landed regardless. Findings she actions while provisional are remembered client-side, because the detector has no memory between runs. runCheck no longer re-submits what the fast pass already filed — it's the catch-up path for when that submit failed. The arrival chime keys rule-pack cards by wording too, so a finding doesn't chime once as provisional and again as persisted. Not done, deliberately: no distinct style for unconfirmed local hits. The rail renders both engines identically on purpose, and a provisional card now lives for one LAN round-trip. Claude-Session: https://claude.ai/code/session_016y6gyuHkQXPiEuW8RGQyua --- UX_REVIEW_2026-07-27.md | 49 +++++++++++++++++++++++++++++++++ web/src/App.tsx | 23 +++++++++++----- web/src/hooks/useCheckpoint.ts | Bin 8478 -> 14498 bytes 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/UX_REVIEW_2026-07-27.md b/UX_REVIEW_2026-07-27.md index 951dc57..238d9d3 100644 --- a/UX_REVIEW_2026-07-27.md +++ b/UX_REVIEW_2026-07-27.md @@ -166,6 +166,52 @@ in the status bar. Two independent levers, both worth doing: offline; during a full check, at least one card appears before the last chunk finishes; the status bar shows a running count. +### 3b — DONE (third session). The rules existed; the latency didn't. + +Scoped against the code as the handoff advised, and the handoff was right: +`prose.ts` already carries every rule this item asks for — `articles` +(a/an), `pluralAfterNumber`, `subjectVerbAgreement`, `uncountables` — and +they already surface as real cards via the `mechanics` family. Nothing to +write there. The gap was purely *when* they render: `mechanicsFindings` ran +only inside `runCheck`, behind the same 4000 ms checkpoint debounce as the +LLM, and only reached the screen via the server's reply. So a free, +instant, offline-capable detection was being delivered at network speed on +an LLM-shaped delay. + +**Implemented:** +- `useCheckpoint.ts` — the rule pack gets its own `FAST_MS = 250` fuse, + separate from the 4 s checkpoint. It renders its findings as + *provisional* suggestions with no network at all, then persists them; the + server's reply is authoritative and clears the provisional set. If the + reply never comes (offline, server down) the cards simply stay — which is + the point of a rule pack. +- Provisional cards carry a `local: ` id. The merge + matches on wording, not position, so a card can't flicker into a + duplicate of its own persisted twin while she types around it. +- `resolveServerId` maps a card to the row the API can act on, awaiting the + in-flight submit if she accepts inside that window — so an early accept + still records the keep and plants its word in the garden instead of being + silently dropped. Null means no row exists (offline); the edit has landed + regardless. +- Findings she actions while provisional are remembered client-side + (`actionedRef`), because the detector has no memory between runs. The + server already keeps the equivalent record for persisted rows. +- `runCheck` no longer re-submits mechanics for text the fast pass already + filed; it's now a catch-up path for when that submit failed. +- `App.tsx` — accept/dismiss go through `resolveServerId`; the arrival + chime keys rule-pack cards by wording so one finding doesn't chime twice + (once provisional, once persisted). + +**Deliberately not done:** no distinct "modest style" for unconfirmed local +hits. The rail renders LLM and rule-pack cards identically on purpose (see +the note on `Suggestion.source` in `client.ts`), and a provisional card now +lives for one LAN round-trip. Styling it differently would be a visible +regression against an existing decision, not polish. + +**Still open from item 3:** the incremental-surfacing half (per-chunk LLM +results) and the running count in the status bar — both belong with item 2's +chunking and item 8's status-bar summary. + ## 4. Rail scrolls away from the text With ~7 cards the rail is taller than the viewport; scrolling to reach @@ -289,6 +335,9 @@ if you're comparing against memory of the live site, that's why. reproducible) and item 5's original premise (re-scoped, much cheaper now). **Untouched:** items 2, 3, 4, 6, 7, 8. +*(Third session: item 3b done — see the subsection under item 3. Item 3's +incremental-surfacing half remains. Untouched: 2, 4, 6, 7, 8.)* + **Suggested next:** item 3b, the instant local rules layer — but it is **largely already built, in `main`**, and the item as written doesn't know that. Before writing any rules engine, read: diff --git a/web/src/App.tsx b/web/src/App.tsx index ff2b504..95606cf 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -1,7 +1,7 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { api, type DocSummary, type DocUpdate, type Document, type Suggestion, type Tag, type TagColor } from './api/client' import { useAutoSave } from './hooks/useAutoSave' -import { useCheckpoint } from './hooks/useCheckpoint' +import { findingKey, useCheckpoint } from './hooks/useCheckpoint' import { useSpellChecker } from './hooks/useSpellChecker' import { useTags } from './hooks/useTags' import { DocList } from './components/DocList/DocList' @@ -90,6 +90,7 @@ export default function App() { runVoice, runCollocation, removeSuggestion, + resolveServerId, } = useCheckpoint(currentDoc?.id ?? null) // Browser-side spell checker — loads the en-US dictionary once per session. const { checker: spellChecker, addWord } = useSpellChecker() @@ -350,17 +351,20 @@ export default function App() { // Accept applies the replacement in the editor (handled in EditorCore) and // marks the suggestion accepted; dismiss just rejects it. Both drop it locally. + // A rule-pack card can be accepted before its row exists — the edit has already + // landed either way, so a missing id just means there's nothing to file. const handleAccept = useCallback( async (s: Suggestion) => { removeSuggestion(s.id) setAcceptTick((n) => n + 1) try { - await api.acceptSuggestion(s.id) + const id = await resolveServerId(s) + if (id) await api.acceptSuggestion(id) } catch (err) { console.error('accept failed', err) } }, - [removeSuggestion], + [removeSuggestion, resolveServerId], ) // After restoring a version, swap the restored doc into the editor. Bumping @@ -398,12 +402,13 @@ export default function App() { async (s: Suggestion) => { removeSuggestion(s.id) try { - await api.dismissSuggestion(s.id) + const id = await resolveServerId(s) + if (id) await api.dismissSuggestion(id) } catch (err) { console.error('dismiss failed', err) } }, - [removeSuggestion], + [removeSuggestion, resolveServerId], ) // Play a soft sound when freshly-checked suggestions arrive — one per distinct @@ -411,10 +416,14 @@ export default function App() { // a pile-up. We track which ids we've already chimed for, and only chime for // recently-created suggestions so opening a doc with old pending advice stays // silent (the existing set was created in a past session). + // Rule-pack findings are chimed by their wording, not their id: the same fix + // appears first as a provisional card and then as its persisted row, and the + // writer should hear it once. const chimedRef = useRef>(new Set()) useEffect(() => { - const fresh = suggestions.filter((s) => !chimedRef.current.has(s.id)) - fresh.forEach((s) => chimedRef.current.add(s.id)) + const key = (s: Suggestion) => (s.source === 'local' ? `local:${findingKey(s)}` : s.id) + const fresh = suggestions.filter((s) => !chimedRef.current.has(key(s))) + fresh.forEach((s) => chimedRef.current.add(key(s))) const justMade = fresh.filter( (s) => Date.now() - new Date(s.created_at).getTime() < 12_000, ) diff --git a/web/src/hooks/useCheckpoint.ts b/web/src/hooks/useCheckpoint.ts index a0a8f1f501edd088290cc2539b22facad17aa61b..b4b578acd1688aa67bdab41c0bde0e5b3b7dc042 100644 GIT binary patch literal 14498 zcmb`O|85+|k;m_EJw;16gv*#*DNgbaZP}1587D-t4M~?E5DaU$+smEf>&;cS9l}px*T=7`ra7+`mnZt= z$!ZlBgMEA+H=TWW5m)x}Ex~fmYaB~&I^SD@7ZN2Ef zsoQ1UUZl$}cNZ72AL_0h*O<@m^Vt1GQ^&SkYjdzdUp}$ucxB52{r#``$eX(M{?)eY z~d5@c7AB-~H#eUqAWr`OEMy{O-Yn2Y2q=zaL($V_3!^ZZ~yXBgrCccTEge zY(6Zye&|En4QnJ0L&sO`N$9&!58)#6Rn5Qdx()pr&1tn|JnwexBA$hNRj976ns`rN z?wV!Twad6|>NbY!bt_6OwX*+As!6TMT*t-9Tot88G|hiZkCdW?s< zt6qAQSXH~!!!3(6H~eoA(^}iN++MZ)b=>yzq=auCy?o^g`1JR_|3{(e7F82gZWlzV zmxxghH)dbf?!u-T7VA(26#BOAQ7!i`s_imV7ga6Yq6dk4#hcJ!&y49+W_NZxVU2i;-L~)eA#bVLp}Cpg ziSH~V+<}sY{t$2QpV{h+Na?oqMU6Jj5O#|(K4Y_2O|@XhEfV_0@cuYF{NhdsZ~pzK z)jU7){@?Ular^r>=8uJri{MypW8b5>%DnT`yfeuYDqSw_b^lzr5?4)kji$EU?qY2k zTh|xshJVBTZB;jzc7b!p?G1`hgkHp@Tj7nl#n~I&0lP=(xHcoZ6|qE$iZ!a=RNG5M zgJi*VPjBLIIyu9vj}gW6UGp}+jB?AT%j3@^jm%oAzb(!*9@5T z3?hTj1v<~e8@mnFya~%~{Wk63T)I$J(Q?ZUtn!yCo$mUts%?Toixxs;h^I}xQMw_l z#;sW{s*)fBfoQ^+oI}@l6I^u005QZY^GuVy3zo-9wpAY{X+FjEe zn>#x?zAluATydBXss91MWHoIrem-4){lgct*FPL%a+L6bthu-q9&ZXuY0Nc7Q= zg7?Xrs@~UZI;C6b5Xc*EY@_ll1SH}qBR*X_EM6viXCPWsEPdT=FC}JDT!AsBxbLSd z`?HEmOJpCj=f9R;;#%dyvsKmfF{%31c9%&=g~T0-&%qMj8g`m;Kv}#J03}(3nZTf4 z5J%?YKr|q6TClyIXu%?+C`jb2G zNxjY&B|)<7xCA4x^nnz%)r!^Icxs4jX3WTLSy>M*yVw(QzF@yQC*_$p8^-Fm7Sgd&FPJG z6p1M*BN>*IR$wzo0Nb~`R}t1Y8}WWixkgP8t}9fktR^ynksHV;j+isy$dswke8siu zi&U!l>g-UkQTLmgq6BMLJ&QSR&Sp-IchqfJ&8YMkIPiRb6VVkozGv!A^9ngDc;P5*>QHCuh>20Vgwc2 zApwjQTjiR>4jnIzxHyc?!y{!ml8*6OS7k1}zWeq`0kiHLBq-~Gu(mVdG5Wy`P~tM) zU>AbMYb-p{*CaMBVC9&s?bXpS7~hG_RTSVN=oQANjwsq7Ys*=6yTY3kS?Armlp?OJ zm0}g)HCi=SNF2_134VWX%&F8>$}hF`4VhK-lTaG0-ffo*gC??)Y^!SQZ&Qb>2Ew%SHvaU*Z#arJ%^?i8*aJe}p zy`?$@=Ku(ZtIK>7cD#7<>cyXbeEj6uqd&j&=FRKh(!M$gzxmXD{Py?!_@Tg)5%mqf zVR-Cvf|oJG(efDCQ&1G*sTxB~Ml{GNQ?->~T9PJQGQKO-7?#=Y|7!=Tt(3E;t@zzw zQi47YHMXTz?}Ih8y=d5} zXQI%sqP7pe#*)8IbzstzZrDtI^g(Y7&_unYg~((w9oS3%t!#4AJPWtitRrSxtdO&&96!GrW?A z`2XP}{`IKEhg0JHCi#E~b3DhN3H=$J8p(6FKG63^4SZ>Vo_bwYk}7uepVUj$KDQ!ob~L@*m6~$U z(TS=+-zo<*>Qj?kvWcWwzkSc5xS(G>PCxg)HQsMRNImaQg7n3wF}kc0!-VP`r6Z&q z%qd&)<%54UmX)bf89B?fUiKf~?DcRb7QT?dk037{;dQu@~ZSPB}}+P4u)Jt*s^Z=M6Rhawzc<*HuLapx80b*ED=Fk z%mX~1;uz!yV{2uwAlP_TbkT*`IXuZLXlpdEHO=STj(5z$_n7dJdi?VhP+ zD3ADiaCiVmliS|{M4p`m>C)+bVD2SszcHKR^Kd|d=TvnU!@ATpF8c>cKe>;>m)bAt z97&$E?9|6wH5JXbu{`hsM&Yig6I+Y#))g4+jLGAmKm)-<@)SGH+1~79Bao6oX=VsV zOLVC%8qkcM^-1^*_4dJbUZ;&(H{A3cBG@yc^`2oSyqfHC2=6F^cDr>66q@(UD5!N;VB)J&2iTQ@0nq3}~*PZcN<`C-L8JXJ?ml}TL6mpSB0_S{F%N8dj7I@jLl zD^1}Fx*z&U&Jkg92z49f6;K%k73ZrLct9tdXP?tqm>DuPaM7Mu?rTVqOMtK8b*!<7 zjoP}nk(K3R=H9d8KBM4OK&#gX>PMah0CKNTxV7t|B4_w&*WYme`59dWI8;To1Cdv# zo{${Q<9L=|kdzG2A1^s>FFH&kkI1?C>?Aqcic~_@Eew{;Wvy?sh$Y=*J=?D>W=c)f zZKcB{Mi{(lA*v2nCFu(Aie`p7FH00$m=^1e$+6^8xh-7aTFwl%V1_r7;+C5}60uo* za`2DPF~c{RrXXX%h(PA6c@gQ3RlK5*@r&H}bKvr`HJ1E}O_UXb-j|h-zP>d+8 ztu=$oKe2X(LCoD?=60^}reN!O&&OfXkTJ`!mkT~1`Q|gp=@o9T{^RLO8VPUH0UH6Y z=_SjHwBu+l;!LsEQBU7tg^Br88z- z&@a;-yKU9nzM1g^6~+HUTPlRe8ynZM%@sHN@-3>O+%I818~ffQJ+HTlfefFI!N8by=CagAo}T>(iOlC?C7%WQ`kH+6WB3rZ4xBnnR(0VJu3fBh=d~8y3~Qyo}fVBBjR*-6rYYV?WC_vViW7I=j^4oq!eVrCOMbfMwNk_Vp`MD7n8j*n8-~=LJ9b4v4Er4 zW{0oaASOt$LgA^n%qO3dB1BUb!_F`(&Ah3HGnsBQiJwsU!H14sRP>hWv~%Cuv`Z?-o)Umhpk&nVa5~wWq4Q z$OjZ-@2(G=^FuE`P_8QqwG<0~@`*j>(fx{8DZaxPR`ugByY>|96B9* z+fpCv3#8SGdp>-Xed&3{jsdf`%x&j0-vH55c7k)ylFKD=DK48okX6c(p1V`7Ik;K? z^}#opa}?QC5pWkRJl*fMtB*EO`?8TnR$J0#aqhlHvC9@)#>UZ4GvQOdZ1CN3qyBo+ zTqaI;rhu^aYuP3s{EeW0}b!W^ByjLRLR(G=i8Q-(16Jk{QpP1uo&94ABK!I}vz z!TbeXFQmh4=AAiDuFPR8JX^|pO3zOs-Ti^*ccu-{*{96PP3hA|MAdEh3*I{gqUDubc>fUM9Q}u zxF#3#5fvw!YP1m^d8|NV*{LFv@3DsM>;m~C4UB0hcITeYdSl&ge9My(+AxS{ht^z zn)#Ue+0k+CKwF5I=ef^knv@U63jemdY}ItrxQ3{MeOqQWUoW>0b6QJ}+TCMdq?-ud zb~&59v(QcYS0L#EVOLxsw7yhmfB9u`PKP~ofSCh1rJPZ=oLEMF_$T>gHWa>~`X~LZ zjZFx^cfgtx#v7q@;(i|tmgtBm@E0KL#sXFT!T@wp_DbztuqN=wk>ZycRoyJ%=T`4tnT{FMuAj12l5tli_G zvSbnY>6pPZ&9P+H}W3vddTZ&|MexS0Voga9--k delta 824 zcmY*Xzi$&U6joXlln5d*6i7?pAyrBuE=iGK0cB`YA%>O#BnGMu?%hjb;j@G7YY0^c zVr5BXW(FqqgBc02vmq81CJ=uE67SqJftzgWd*82jZ=bx`oU9$y*N^PWbaLu zw3L20S6oPMubj>8-hv1%h89A>QQErU5dxDD^idmbEHDGMhXwFovk8?L!yc17G!^Ku zn2RCoV@GMsV9?bpj+ur&GsfLrE$__-HJZMXhS-Hh77Za}T66UDaRWBd*gAHIi8M)H zNn%#gvvSETX9BDq!jBiPXPja|?;gtaUMh4wn&^UDJRKm>HhUk#Q~( zvK%LrP%7(Qm9Ds4dD(reogZIqwLm69)ZlPB0bY521E3=Aql{2{e=`NreM*UV9Am_p zMT$UvGt96;a$FkAq=l+fu)V!g3&5zEgeN8Bm}2k_rw|6MAEksg(GpBgB_fg)3j;=J zr{x%-2LJWm_=hs2#tF`CZ->?fEnt3>^?BpCv+UV#v&y5uS5@@aJJSNIRLSz`TFrAh z25av5h0-cHdysUyv@%Wt)f4^<<%-SA?p^7|Quf1)Qc-R%x<5Db>1X(sKQjtoS0}RF z!|<{De(jh088G+tVln+vzqjBv)*rd?^=IP@wO1<;*);een7LX2VKz?N4d6iW&>uo2 rgbJDU7a`&;HIURUA)`cK03`a5u*awN30M(EW2_Evo(4vXg;C)z1qT&0 From 10e8aef86cc8ce5b4256b9eff1437a7389870f65 Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:46:12 -0700 Subject: [PATCH 2/3] Stop regenerating the world on every check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A card vanishing and coming back seconds later, with different words, was never about latency: every pass deleted its whole family and re-inserted it, so each round minted new row ids. The rail keys on suggestion.id, so a full remount was guaranteed — new id, new created_at (hence the re-fired chime), and a fresh explanation from a model that re-reasons every time it is asked. One unchanged mistake carried three different explanations in a single sitting. Passes now reconcile instead of replace. A re-proposed edit keeps its row: its id, its created_at, and the wording she has already read. And the grammar checkpoint stops asking about sentences nobody touched — the document is split into hashed sentences, checked_chunks records which ones a family has read, and only the difference is sent. When nothing changed it doesn't call the model at all, and doesn't spend its rate-limit slot on having done nothing. The tone is part of a sentence's identity: cached advice was written for the old register, so switching doc type re-reads every line. replaceMechanics reconciles too, which mattered more than expected — the rule pack fires 250 ms after a keystroke, so it was re-minting every local card's id several times a sentence. Only the grammar checkpoint is chunked. Voice is a property of the whole document, and the collocation coach is a button she pressed asking for a fresh read. No client change was needed; stable ids were the whole of it. Claude-Session: https://claude.ai/code/session_016y6gyuHkQXPiEuW8RGQyua --- UX_REVIEW_2026-07-27.md | 83 +++++++ internal/db/db.go | 26 +++ internal/suggestions/chunk.go | 145 ++++++++++++ internal/suggestions/chunk_test.go | 118 ++++++++++ internal/suggestions/handlers.go | 203 ++++++++++------- internal/suggestions/handlers_test.go | 29 ++- internal/suggestions/reconcile.go | 293 +++++++++++++++++++++++++ internal/suggestions/stability_test.go | 191 ++++++++++++++++ 8 files changed, 1009 insertions(+), 79 deletions(-) create mode 100644 internal/suggestions/chunk.go create mode 100644 internal/suggestions/chunk_test.go create mode 100644 internal/suggestions/reconcile.go create mode 100644 internal/suggestions/stability_test.go diff --git a/UX_REVIEW_2026-07-27.md b/UX_REVIEW_2026-07-27.md index 238d9d3..840c079 100644 --- a/UX_REVIEW_2026-07-27.md +++ b/UX_REVIEW_2026-07-27.md @@ -143,6 +143,81 @@ instability, doubles the pause after each accept, and burns qwen3.5 tokens. or position of any other card; re-check traffic after a one-sentence edit contains only that sentence's chunk; explanations are stable across rounds. +### 2 — DONE (fourth session). Server-side; the client needed nothing. + +Both halves shipped, and they turned out to be one idea. The root cause of +the vanish/reappear was structural: **every pass deleted its whole family +and re-inserted it**, so each round minted new row ids. The rail keys its +cards on `suggestion.id`, so a full remount was guaranteed — new id, new +`created_at` (hence the re-fired arrival chime), and a freshly-worded +explanation from a model that re-reasons every time it's asked. + +**Implemented:** + +- `chunk.go` — splits the document into sentences and hashes each. Newlines + always break; ASCII terminators need trailing whitespace (so `3.50` and + `Ms.` stay whole); `。！？` break outright, since Chinese runs sentences + together with no space and she writes both languages in one document. The + hash normalizes quotes and whitespace runs through the existing + `normalizeForDedup`, so the editor's constant quote rewriting and a + reflowed paragraph cost nothing. Identity is the hash, not the position — + insert a paragraph at the top and every sentence below keeps its cards. +- `reconcile.go` — passes now *reconcile* rather than replace. A row on a + sentence this pass didn't ask about is kept untouched; a row whose + sentence is gone is dropped; a row on a sentence that was re-read survives + only if the model proposed the same edit again, keeping its id, + `created_at` and its **original explanation**. Re-proposals are matched on + `(original, replacement)` normalized — not on type, so a re-labelled edit + keeps the label she's already reading. +- `checked_chunks` (migration `0014`) records which sentences a family has + read. The grammar checkpoint asks only about the difference. **When + nothing changed it doesn't call the model at all** — and doesn't consume + its rate-limit slot, so an idle check can't throttle the next real edit. +- The tone is folded into a sentence's hash, so switching doc type still + re-reads every line: the same sentence gets different advice as an + academic essay than as a journal entry, and cached advice was written for + the old register. +- `replaceMechanics` reconciles too. This mattered more than expected: the + rule pack fires 250 ms after a keystroke (item 3b), so it was re-minting + every local card's id several times a sentence. + +**Deliberately not done:** + +- Only the grammar checkpoint is chunked. Voice is a property of the + document as a whole — a sentence isn't inconsistent with itself — and the + collocation coach is a button she presses asking for a fresh read. Both + still read everything, but both now reconcile, so they keep their ids. +- No client change. With stable ids the existing code already does what the + item asked for: the rail keeps its card DOM, an expanded card survives a + re-check, and the chime (which keys on id) stops re-firing for advice she + is already reading. The one-card optimistic removal on accept was already + there. + +**Sentences the model can't be trusted to have read.** Two guards the plan +didn't anticipate, both found while writing the tests: a finding is +attributed to a sentence the model was *actually shown* before falling back +to the whole document (a short span like "the the" can occur twice, and +crediting the cached copy would drop it); and a cached row whose quoted span +no longer matches byte-for-byte is dropped *and* its sentence re-opened, +rather than caching advice the frontend can't anchor. + +**Verified on the running binary**, not just in tests — per the handoff's +own advice. Against a stand-in model server: three checks over a two- +sentence document, editing only the second. The model received exactly +`She goes to market yesterday.` and never saw the first sentence; the +untouched card kept its id and its first explanation across all three +rounds; the fixed sentence's card was dropped; the idle re-check made zero +model calls. Mechanics identity confirmed the same way (a finding kept its +row id while its span moved). + +Coverage: `chunk_test.go` (splitting, CJK, decimals, cosmetic churn) and +`stability_test.go` (untouched cards keep id + explanation, unchanged +document skips the model, deleted sentence drops its card, tone change +re-opens everything, mechanics rows keep identity). Two existing tests +changed contract deliberately — `TestFickleEditsSuppressed` and +`TestCollocationPassCoexists` both re-checked a document nobody had edited, +which is now a no-op; they edit the text between passes, as she always does. + ## 3. Perceived latency: mask the LLM round-trip Measured ~8–15 s from typing-stop to cards, with only a small "Checking…" @@ -338,6 +413,14 @@ reproducible) and item 5's original premise (re-scoped, much cheaper now). *(Third session: item 3b done — see the subsection under item 3. Item 3's incremental-surfacing half remains. Untouched: 2, 4, 6, 7, 8.)* +*(Fourth session: item 2 done — see the subsection under it. **Neither 3b +nor 2 is deployed yet**: both sit on unmerged topic branches +(`feat/instant-local-rules`, then `feat/stable-suggestions` stacked on it) +and `main` is still at `ba06d90`. Untouched: 4, 6, 7, 8. Item 3's +incremental-surfacing half is now cheap — the chunking it was waiting on +exists — but it needs streaming, which the current `/check` shape doesn't +do.)* + **Suggested next:** item 3b, the instant local rules layer — but it is **largely already built, in `main`**, and the item as written doesn't know that. Before writing any rules engine, read: diff --git a/internal/db/db.go b/internal/db/db.go index 8e91c31..b5ea7e1 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -497,6 +497,32 @@ CREATE INDEX idx_suggestions_resolved ON suggestions(status, resolved_at); stmt: ` ALTER TABLE suggestions ADD COLUMN source TEXT NOT NULL DEFAULT 'llm'; UPDATE suggestions SET source = 'local' WHERE type = 'mechanics'; +`, + }, + { + // Sentence-level identity, so a re-check stops regenerating the world. + // Every pass used to delete its whole family and re-insert it, which + // meant accepting one edit gave every other card a new id and a newly + // worded explanation — the rail visibly emptied and refilled, and the + // model was asked again about sentences nobody had touched. + // + // `chunk_hash` records which sentence a suggestion belongs to, and + // checked_chunks records which sentences a family has already read. A + // re-check then asks only about the difference and keeps the rest of + // the rows exactly as they are, id and wording included. + // + // Existing rows get '' — "sentence unknown", which reads as in-play, so + // they are simply reconciled on the next pass like any fresh finding. + name: "0014_suggestion_chunk_hash", + stmt: ` +ALTER TABLE suggestions ADD COLUMN chunk_hash TEXT NOT NULL DEFAULT ''; + +CREATE TABLE checked_chunks ( + doc_id TEXT NOT NULL REFERENCES documents(id) ON DELETE CASCADE, + family TEXT NOT NULL, + hash TEXT NOT NULL, + PRIMARY KEY (doc_id, family, hash) +); `, }, } diff --git a/internal/suggestions/chunk.go b/internal/suggestions/chunk.go new file mode 100644 index 0000000..611d9a2 --- /dev/null +++ b/internal/suggestions/chunk.go @@ -0,0 +1,145 @@ +package suggestions + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "unicode" +) + +// Chunking splits a document into sentence-sized units so a re-check can ask the +// model only about the sentences that actually changed. Accepting one edit used +// to re-run the whole document: every card vanished, came back with a new id and +// a freshly-worded explanation, and spans re-merged into different shapes. The +// sentences she didn't touch have nothing new to say about themselves, so their +// suggestions are simply kept (see reconcilePending). +// +// A chunk's identity is its hash, not its position — she inserts a paragraph at +// the top and every sentence below keeps its suggestions. + +// chunk is one sentence of the document, with the hash that identifies it. +type chunk struct { + text string + hash string +} + +// asciiTerminators end a sentence only when whitespace (or the end of the text) +// follows, so "3.5" and "Ms." don't split mid-word — a wrong split costs only a +// slightly smaller chunk, but a split inside a number would churn its hash on +// every keystroke around it. +const asciiTerminators = ".!?" + +// cjkTerminators end a sentence outright: Chinese runs sentences together with +// no space after 。, and she writes in both languages in one document. +const cjkTerminators = "。！？" + +// closers are swallowed into the sentence they close, so the quote mark travels +// with the sentence rather than opening the next one. +const closers = `)]}"'’”」』` + +// splitChunks divides text into sentences, dropping whitespace-only runs. +// Newlines always break a chunk, so a list or a line of dialogue is its own unit. +// +// `salt` distinguishes two *readings* of the same sentence. The grammar +// checkpoint's advice depends on the document's tone — the same line gets +// different notes as an academic essay than as a journal entry — so switching +// tone must re-open every sentence rather than serve back advice written for the +// old register. +func splitChunks(text, salt string) []chunk { + var out []chunk + runes := []rune(text) + start := 0 + add := func(end int) { + if s := string(runes[start:end]); strings.TrimSpace(s) != "" { + out = append(out, chunk{text: s, hash: hashChunk(s, salt)}) + } + start = end + } + + for i := 0; i < len(runes); i++ { + r := runes[i] + if r == '\n' { + add(i + 1) + continue + } + cjk := strings.ContainsRune(cjkTerminators, r) + if !cjk && !strings.ContainsRune(asciiTerminators, r) { + continue + } + // Swallow a run of terminators ("?!", "…") and any closing punctuation. + j := i + 1 + for j < len(runes) && (strings.ContainsRune(asciiTerminators+cjkTerminators+closers, runes[j])) { + j++ + } + if cjk || j >= len(runes) || unicode.IsSpace(runes[j]) { + add(j) + i = j - 1 + } + } + if start < len(runes) { + add(len(runes)) + } + return out +} + +// hashChunk identifies a sentence by its content under the same normalization +// the suppression logic uses: quote style and whitespace runs churn constantly +// (the editor rewrites quotes as she types, a paragraph reflows) and none of +// that changes what the sentence says, so none of it should cost a re-check. +func hashChunk(s, salt string) string { + sum := sha256.Sum256([]byte(salt + "\x00" + normalizeForDedup(s))) + return hex.EncodeToString(sum[:])[:16] +} + +// hashSet indexes chunks by hash — "is this sentence in the document?" +func hashSet(chunks []chunk) map[string]bool { + out := make(map[string]bool, len(chunks)) + for _, c := range chunks { + out[c.hash] = true + } + return out +} + +// changedChunks returns the chunks whose hash wasn't in the last checked set, +// in document order and deduplicated — a sentence repeated verbatim is one +// question, not two. +func changedChunks(chunks []chunk, checked map[string]bool) []chunk { + seen := make(map[string]bool, len(chunks)) + var out []chunk + for _, c := range chunks { + if checked[c.hash] || seen[c.hash] { + continue + } + seen[c.hash] = true + out = append(out, c) + } + return out +} + +// joinChunks renders a chunk set as the text to hand the model: one sentence per +// line, so two sentences pulled from opposite ends of the document don't read as +// one run-on. +func joinChunks(chunks []chunk) string { + parts := make([]string, 0, len(chunks)) + for _, c := range chunks { + parts = append(parts, strings.TrimSpace(c.text)) + } + return strings.Join(parts, "\n") +} + +// chunkFor names the sentence a suggestion belongs to: the first chunk whose +// text contains the flagged span. Returns "" when the span straddles a sentence +// boundary or the model paraphrased what it quoted — such a row is re-examined +// on every pass rather than cached, which is the safe direction. +func chunkFor(original string, chunks []chunk) string { + o := normalizeForDedup(original) + if o == "" { + return "" + } + for _, c := range chunks { + if strings.Contains(normalizeForDedup(c.text), o) { + return c.hash + } + } + return "" +} diff --git a/internal/suggestions/chunk_test.go b/internal/suggestions/chunk_test.go new file mode 100644 index 0000000..2faab5e --- /dev/null +++ b/internal/suggestions/chunk_test.go @@ -0,0 +1,118 @@ +package suggestions + +import ( + "strings" + "testing" +) + +func texts(chunks []chunk) []string { + out := make([]string, 0, len(chunks)) + for _, c := range chunks { + out = append(out, strings.TrimSpace(c.text)) + } + return out +} + +func TestSplitChunks(t *testing.T) { + cases := []struct { + name string + in string + want []string + }{ + { + name: "plain sentences", + in: "I has two apple. She go to market yesterday! Why?", + want: []string{"I has two apple.", "She go to market yesterday!", "Why?"}, + }, + { + // A decimal must not split, or the sentence's identity would churn + // while she types the number. + name: "decimals stay whole", + in: "It costs 3.50 today. Tomorrow, more.", + want: []string{"It costs 3.50 today.", "Tomorrow, more."}, + }, + { + name: "closing quote travels with its sentence", + in: `He said "early," and left. She stayed.`, + want: []string{`He said "early," and left.`, "She stayed."}, + }, + { + // Chinese runs sentences together with no space after 。 — she writes + // in both languages in one document. + name: "cjk terminators split without a space", + in: "我想说这句话。但是不知道用英语怎么说。", + want: []string{"我想说这句话。", "但是不知道用英语怎么说。"}, + }, + { + name: "newlines break chunks", + in: "A list item\nAnother item\n", + want: []string{"A list item", "Another item"}, + }, + { + name: "blank runs are dropped", + in: "\n\n \nOnly this.\n\n", + want: []string{"Only this."}, + }, + { + name: "trailing fragment is its own chunk", + in: "Done. Still writing", + want: []string{"Done.", "Still writing"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := texts(splitChunks(tc.in, "")) + if len(got) != len(tc.want) { + t.Fatalf("want %q, got %q", tc.want, got) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("chunk %d: want %q, got %q", i, tc.want[i], got[i]) + } + } + }) + } +} + +// A sentence's identity survives the churn that doesn't change what it says: +// the editor rewrites quotes as she types, and a paragraph reflows. +func TestChunkIdentityIgnoresCosmeticChurn(t *testing.T) { + a := splitChunks(`She said "hello" softly.`, "") + b := splitChunks("She said “hello” softly.", "") + if len(a) != 1 || len(b) != 1 { + t.Fatalf("want one chunk each, got %d and %d", len(a), len(b)) + } + if a[0].hash != b[0].hash { + t.Fatalf("quote/whitespace churn changed the sentence's identity") + } + if same := splitChunks(`She said "hello" softly.`, "academic"); same[0].hash == a[0].hash { + t.Fatalf("a different tone must be a different reading of the sentence") + } +} + +func TestChangedChunksAndLookup(t *testing.T) { + chunks := splitChunks("One thing. Another thing. One thing.", "") + if len(chunks) != 3 { + t.Fatalf("want 3 chunks, got %d", len(chunks)) + } + + // A repeated sentence is one question, not two. + if got := changedChunks(chunks, nil); len(got) != 2 { + t.Fatalf("want 2 distinct changed chunks, got %d", len(got)) + } + + checked := hashSet(chunks[:1]) + changed := changedChunks(chunks, checked) + if len(changed) != 1 || strings.TrimSpace(changed[0].text) != "Another thing." { + t.Fatalf("want only the unread sentence, got %q", texts(changed)) + } + + if chunkFor("Another", chunks) != chunks[1].hash { + t.Fatalf("span was attributed to the wrong sentence") + } + // A span the document doesn't contain has no sentence, so it is never cached. + if chunkFor("nowhere in here", chunks) != "" { + t.Fatalf("unanchorable span should have no chunk") + } +} diff --git a/internal/suggestions/handlers.go b/internal/suggestions/handlers.go index bf1dfaa..fb9a1cc 100644 --- a/internal/suggestions/handlers.go +++ b/internal/suggestions/handlers.go @@ -160,16 +160,21 @@ func (h *Handler) mechanics(w http.ResponseWriter, r *http.Request) { httputil.WriteJSON(w, http.StatusOK, out) } -// replaceMechanics swaps the document's pending offline rows for the supplied -// findings in one transaction, leaving the LLM families and actioned rows -// untouched. Findings the user already accepted or dismissed are suppressed (the -// detector has no memory between runs), and malformed spans are skipped. +// replaceMechanics brings the document's pending offline rows in line with the +// supplied findings in one transaction, leaving the LLM families and actioned +// rows untouched. Findings the user already accepted or dismissed are suppressed +// (the detector has no memory between runs), and malformed spans are skipped. // -// The DELETE is scoped by *source*, not by type: the rule pack owns both the -// mechanics family and its share of the collocation family, and every run is a -// full recompute of the document, so everything it wrote last time goes. Scoping -// by type instead would strand offline collocations the current text no longer -// warrants — the one row nobody would ever replace. +// A finding the detector still reports keeps its existing row — same id, same +// created_at — and only its offsets move. This pass fires 250 ms after a +// keystroke, so deleting and re-inserting the family would hand every card a new +// identity several times a sentence: the rail would remount, a card expanded for +// Ask Petal would collapse under her, and the arrival chime would re-fire. +// +// The scope is *source*, not type: the rule pack owns both the mechanics family +// and its share of the collocation family, and every run is a full recompute of +// the document. Scoping by type instead would strand offline collocations the +// current text no longer warrants — the one row nobody would ever replace. func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) error { tx, err := h.DB.Begin() if err != nil { @@ -177,18 +182,18 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er } defer tx.Rollback() - if _, err := tx.Exec( - `DELETE FROM suggestions WHERE doc_id = ? AND status = ? AND source = ?`, - docID, db.SuggestionStatusPending, db.SuggestionSourceLocal, - ); err != nil { + existing, err := loadPending(tx, docID, "source = '"+db.SuggestionSourceLocal+"'") + if err != nil { return err } + index := indexByEdit(existing) sup, err := buildSuppressor(tx, docID) if err != nil { return err } + kept := make(map[string]bool, len(existing)) for _, f := range findings { if f.From < 0 || f.To <= f.From || strings.TrimSpace(f.Original) == "" { continue // malformed span — the client re-anchors by string anyway @@ -196,16 +201,34 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er if sup.suppressed(f.Original, f.Replacement) { continue } + typ := localType(f.Type) + if row, ok := index.take(f.Original, f.Replacement, f.From); ok { + kept[row.id] = true + if err := reposition(tx, row, f.From, f.To, ""); err != nil { + return err + } + continue + } if _, err := tx.Exec( `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source) VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, docID, f.From, f.To, f.Original, f.Replacement, f.Explanation, - localType(f.Type), db.SuggestionSourceLocal, + typ, db.SuggestionSourceLocal, ); err != nil { return err } } + // Whatever the detector no longer reports, she has fixed. + for _, row := range existing { + if kept[row.id] { + continue + } + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, row.id); err != nil { + return err + } + } + return tx.Commit() } @@ -258,12 +281,65 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - // Nothing to analyze on an empty document — skip the LLM round-trip. + // Nothing to analyze on an empty document — skip the LLM round-trip. The + // family's rows go with the text they were about. if strings.TrimSpace(contentText) == "" { - httputil.WriteJSON(w, http.StatusOK, []db.Suggestion{}) + if err := h.reconcilePending(docID, contentText, nil, scope, nil, nil, false); err != nil { + httputil.ServerError(w, err) + return + } + out, err := h.fetchPending(userID, docID) + if err != nil { + httputil.ServerError(w, err) + return + } + httputil.WriteJSON(w, http.StatusOK, out) return } + // Decide what to ask about before spending anything: a chunked pass asks only + // about the sentences that changed since it last read the document, and when + // none did it doesn't call the model at all — nor consume its rate-limit slot, + // so the next real edit isn't throttled by a check that had nothing to do. + // + // Only a chunked pass consults that record, so only it needs the tone folded + // into a sentence's identity. + salt := "" + if scope.chunked { + salt = tone + } + chunks := splitChunks(contentText, salt) + askText, fresh := contentText, chunks + if scope.chunked { + checked, err := h.checkedChunks(docID, scope.family) + if err != nil { + httputil.ServerError(w, err) + return + } + changed := changedChunks(chunks, checked) + if len(changed) == 0 { + // Every sentence has already been read. Drop the rows whose sentence is + // gone, keep the rest exactly as they are, and answer immediately. + if err := h.reconcilePending(docID, contentText, nil, scope, chunks, nil, false); err != nil { + httputil.ServerError(w, err) + return + } + out, err := h.fetchPending(userID, docID) + if err != nil { + httputil.ServerError(w, err) + return + } + httputil.WriteJSON(w, http.StatusOK, out) + return + } + // When every sentence is new — a first pass, a paste, a tone switch — hand + // over the document verbatim so the model reads it with its paragraphing + // intact. Otherwise send just the delta, one sentence per line. + if len(changed) < len(hashSet(chunks)) { + askText, fresh = joinChunks(changed), changed + } + } + ok, _, slotAt := limiter.Allow(docID) if !ok { // Throttled: return the existing pending set unchanged rather than an @@ -277,7 +353,7 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - raw, err := run(r.Context(), h.Client, contentText, tone, llm.LangFor(pairLang)) + raw, err := run(r.Context(), h.Client, askText, tone, llm.LangFor(pairLang)) if err != nil { // Allow ran before the model call, so a failed pass would otherwise hold // the per-document slot for the full interval — stranding the frontend's @@ -287,7 +363,9 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - if err := h.replacePending(docID, contentText, raw, scope); err != nil { + // A whole-document pass re-read everything, so every one of its rows is up for + // re-proposal; a chunked pass only puts the sentences it asked about in play. + if err := h.reconcilePending(docID, contentText, raw, scope, chunks, fresh, !scope.chunked); err != nil { httputil.ServerError(w, err) return } @@ -308,78 +386,49 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R // inserts. The grammar checkpoint and voice pass each own a disjoint family, so // running one never disturbs the other's pending flags. type pendingScope struct { - deleteWhere string // extra WHERE clause scoping the DELETE to this family + deleteWhere string // extra WHERE clause scoping this pass to its own family forceType string // if set, every inserted row gets this type; else normalizeType + // family keys the sentences this pass has already read (see checked_chunks). + family string + // chunked passes re-read only the sentences that changed. True for the typing- + // cadence grammar checkpoint, which fires constantly and must feel still; + // false for the explicit whole-document passes, where she pressed a button + // asking for a fresh read of everything. + chunked bool } -// Every scope below is confined to source='llm'. The offline rule pack replaces -// its own rows wholesale on each edit (see replaceMechanics) and its findings +// Every scope below is confined to source='llm'. The offline rule pack owns its +// own rows and recomputes them on each edit (see replaceMechanics); its findings // must survive all three model passes — including the collocation coach, which // now shares the collocation family with it. var ( // grammarScope owns the grammar/phrasing/idiom/clarity flags — everything but // the other self-owned families (voice, collocation), which run on their own // cadence/pass and must survive a grammar checkpoint. Notably the offline pass - // writes its rows in the same /check request just before this DELETE runs, so - // the source clause is also what keeps them alive. - grammarScope = pendingScope{deleteWhere: "source = 'llm' AND type NOT IN ('voice','collocation')", forceType: ""} - // voiceScope owns the model's voice flags only. - voiceScope = pendingScope{deleteWhere: "source = 'llm' AND type = 'voice'", forceType: db.SuggestionTypeVoice} + // writes its rows in the same /check request just before this pass reconciles, + // so the source clause is also what keeps them alive. + grammarScope = pendingScope{ + deleteWhere: "source = 'llm' AND type NOT IN ('voice','collocation')", + family: "grammar", + chunked: true, + } + // voiceScope owns the model's voice flags only. Voice is a property of the + // document as a whole — a sentence isn't inconsistent with itself — so this + // pass always reads everything. + voiceScope = pendingScope{ + deleteWhere: "source = 'llm' AND type = 'voice'", + forceType: db.SuggestionTypeVoice, + family: "voice", + } // collocationScope owns the model's collocation flags only — the rule pack's // share of the same family is left standing. - collocationScope = pendingScope{deleteWhere: "source = 'llm' AND type = 'collocation'", forceType: db.SuggestionTypeCollocation} + collocationScope = pendingScope{ + deleteWhere: "source = 'llm' AND type = 'collocation'", + forceType: db.SuggestionTypeCollocation, + family: "collocation", + } ) -// replacePending swaps a document's pending suggestions within one family for a -// fresh batch in a single transaction. Accepted/rejected suggestions and the -// other family's pending rows are left untouched. -// -// Suggestions touching a sentence the user already settled are suppressed from -// the fresh batch (see suppressor): not just the identical edit re-proposed, but -// reversals and re-polishing of the model's own just-accepted output — the -// "fickle, keeps going back and forth on a few sentences" behavior. The model has -// no memory between passes, so without this it re-opens resolved sentences every -// checkpoint. -func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggestion, scope pendingScope) error { - tx, err := h.DB.Begin() - if err != nil { - return err - } - defer tx.Rollback() - - if _, err := tx.Exec( - `DELETE FROM suggestions WHERE doc_id = ? AND status = ? AND `+scope.deleteWhere, - docID, db.SuggestionStatusPending, - ); err != nil { - return err - } - - sup, err := buildSuppressor(tx, docID) - if err != nil { - return err - } - - for _, s := range raw { - if sup.suppressed(s.Original, s.Replacement) { - continue - } - typ := scope.forceType - if typ == "" { - typ = normalizeType(s.Type) - } - from, to := locate(contentText, s.Original) - if _, err := tx.Exec( - `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, - docID, from, to, s.Original, s.Replacement, s.Explanation, typ, db.SuggestionSourceLLM, - ); err != nil { - return err - } - } - - return tx.Commit() -} - // dedupQuoteReplacer folds every straight/curly single- and double-quote variant // (and backtick/acute accent) onto one canonical character. The editor and the // model both rewrite quotes between passes — a sentence accepted with "…" comes diff --git a/internal/suggestions/handlers_test.go b/internal/suggestions/handlers_test.go index ca2a289..badb3ab 100644 --- a/internal/suggestions/handlers_test.go +++ b/internal/suggestions/handlers_test.go @@ -20,10 +20,17 @@ import ( type stubClient struct { response string calls int + // The full prompt of the most recent call, so a test can assert which + // sentences a chunked pass actually asked about. + lastPrompt string } -func (s *stubClient) Complete(_ context.Context, _ llm.CompletionRequest) (string, error) { +func (s *stubClient) Complete(_ context.Context, req llm.CompletionRequest) (string, error) { s.calls++ + s.lastPrompt = "" + for _, m := range req.Messages { + s.lastPrompt += m.Content + "\n" + } return s.response, nil } @@ -64,6 +71,17 @@ func newTestServer(t *testing.T, client llm.LLMClient) (http.Handler, string, *H return authed, docID, h } +// setDocText rewrites the seeded document, standing in for the writer editing. +// The grammar checkpoint only asks the model about sentences that changed since +// it last read the document, so a test that wants a second real pass has to +// change something first — as she always has. +func setDocText(t *testing.T, h *Handler, docID, text string) { + t.Helper() + if _, err := h.DB.Exec(`UPDATE documents SET content_text = ? WHERE id = ?`, text, docID); err != nil { + t.Fatalf("update doc text: %v", err) + } +} + func do(t *testing.T, srv http.Handler, method, path, body string) *httptest.ResponseRecorder { t.Helper() var r *http.Request @@ -183,6 +201,7 @@ func TestFickleEditsSuppressed(t *testing.T) { ]}`} srv, docID, h := newTestServer(t, client) h.Limit = llm.NewRateLimiter(0) + setDocText(t, h, docID, `He left "early," because of the rain. The cat always have a calm face.`) rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") var got []db.Suggestion @@ -194,6 +213,10 @@ func TestFickleEditsSuppressed(t *testing.T) { do(t, srv, http.MethodPost, "/suggestions/"+s.ID+"/accept", "") } + // Both edits are now in the document, which is what re-opens those sentences + // for a second reading. + setDocText(t, h, docID, `He left "early," due to the rain. The cat always has a calm face.`) + // Reversal of the first accept (note the " → ' quote churn) and a re-polish of // the second accept must both be dropped; only the unrelated edit survives. client.response = `{"suggestions":[ @@ -314,7 +337,9 @@ func TestCollocationPassCoexists(t *testing.T) { t.Fatalf("collocation response should carry all three families, got %+v", got) } - // A grammar checkpoint must NOT wipe the voice or collocation flags. + // A grammar checkpoint must NOT wipe the voice or collocation flags. She fixes + // the flagged sentence, so its own grammar row goes and nothing replaces it. + setDocText(t, h, docID, "I have two apples.") client.response = `{"suggestions":[]}` rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") if rec.Code != http.StatusOK { diff --git a/internal/suggestions/reconcile.go b/internal/suggestions/reconcile.go new file mode 100644 index 0000000..6048bd8 --- /dev/null +++ b/internal/suggestions/reconcile.go @@ -0,0 +1,293 @@ +package suggestions + +import ( + "database/sql" + + "gitea.parodia.dev/drwily/petal/internal/db" + "gitea.parodia.dev/drwily/petal/internal/llm" +) + +// Reconciliation replaces the old "delete the family, insert the new batch" +// shape of every pass. A suggestion the pass proposes again is the *same* +// suggestion: it keeps its row, and therefore its id, its created_at and — most +// visibly — the explanation it was first given. The model re-words its reasoning +// every time it is asked, so re-inserting meant one unchanged mistake carried +// three different explanations in a single sitting. +// +// The id is what the frontend keys its cards on, so a stable id is also what +// keeps the rail from emptying and refilling, a card from collapsing mid-read, +// and the arrival chime from re-firing for advice she has already seen. + +// pendingRow is the part of an existing pending suggestion reconciliation cares +// about. +type pendingRow struct { + id string + original string + replacement string + chunkHash string + from int +} + +// loadPending reads the pending rows a pass owns. `where` is the pass's own +// scoping clause (by source, and for the model passes by family) — the same +// fragment that used to scope its DELETE. +func loadPending(tx *sql.Tx, docID, where string) ([]pendingRow, error) { + rows, err := tx.Query( + `SELECT id, original, replacement, chunk_hash, from_pos FROM suggestions + WHERE doc_id = ? AND status = ? AND `+where, + docID, db.SuggestionStatusPending, + ) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []pendingRow + for rows.Next() { + var r pendingRow + if err := rows.Scan(&r.id, &r.original, &r.replacement, &r.chunkHash, &r.from); err != nil { + return nil, err + } + out = append(out, r) + } + return out, rows.Err() +} + +// editKey identifies an edit by what it proposes, not where: "this exact change +// to this exact text". Normalized like the suppression comparisons, so the +// editor's quote rewriting and a reflowed paragraph don't read as a new edit. +func editKey(original, replacement string) string { + return normalizeForDedup(original) + "\x00" + normalizeForDedup(replacement) +} + +// editIndex matches freshly proposed edits against the rows already standing. +type editIndex struct { + rows []pendingRow + used []bool + byKey map[string][]int +} + +func indexByEdit(rows []pendingRow) *editIndex { + idx := &editIndex{rows: rows, used: make([]bool, len(rows)), byKey: map[string][]int{}} + for i, r := range rows { + k := editKey(r.original, r.replacement) + idx.byKey[k] = append(idx.byKey[k], i) + } + return idx +} + +// take claims the standing row for this edit, if there is one. When a document +// repeats the same mistake, `near` (the fresh span's start) picks the closest +// standing row, so two identical cards keep their own identities instead of +// trading them whenever the text between them grows. +func (i *editIndex) take(original, replacement string, near int) (pendingRow, bool) { + best, bestDist := -1, 0 + for _, n := range i.byKey[editKey(original, replacement)] { + if i.used[n] { + continue + } + d := i.rows[n].from - near + if d < 0 { + d = -d + } + if best < 0 || d < bestDist { + best, bestDist = n, d + } + } + if best < 0 { + return pendingRow{}, false + } + i.used[best] = true + return i.rows[best], true +} + +// reposition updates the advisory offsets (and the sentence a row belongs to) +// without touching anything the writer can see. The frontend re-anchors by +// string at render time, so these only matter for the local-vs-model span +// arbitration in dedupeSpans. +func reposition(tx *sql.Tx, row pendingRow, from, to int, chunkHash string) error { + if row.from == from && row.chunkHash == chunkHash { + return nil + } + _, err := tx.Exec( + `UPDATE suggestions SET from_pos = ?, to_pos = ?, chunk_hash = ? WHERE id = ?`, + from, to, chunkHash, row.id, + ) + return err +} + +// reconcilePending brings a model pass's family in line with what it just +// proposed, sentence by sentence: +// +// - A row on a sentence this pass didn't ask about is kept untouched — that +// is the whole point of chunking. Only its offsets are refreshed. +// - A row on a sentence that no longer exists in the document is dropped: she +// rewrote or deleted it. +// - A row on a sentence the pass *did* ask about survives only if the model +// proposed the same edit again, in which case it keeps its identity. +// +// `fresh` names the sentences the model was asked about (nil when it wasn't +// called at all). inPlayAll marks the whole-document passes — voice and the +// collocation coach — where every row is up for re-proposal because the model +// just re-read everything. +func (h *Handler) reconcilePending( + docID, contentText string, + raw []llm.RawSuggestion, + scope pendingScope, + chunks, fresh []chunk, + inPlayAll bool, +) error { + tx, err := h.DB.Begin() + if err != nil { + return err + } + defer tx.Rollback() + + existing, err := loadPending(tx, docID, scope.deleteWhere) + if err != nil { + return err + } + + present := hashSet(chunks) + asked := hashSet(fresh) + modelRan := inPlayAll || fresh != nil + + // Sentences to hand back to the model next time, because a row we were + // caching on them turned out to be unanchorable (see below). + reopen := map[string]bool{} + + var inPlay []pendingRow + for _, r := range existing { + switch { + // A row whose sentence we can't name is never cached — it is re-examined + // whenever the model speaks, and left alone when it doesn't. + case inPlayAll, r.chunkHash == "" && modelRan, asked[r.chunkHash]: + inPlay = append(inPlay, r) + case r.chunkHash != "" && !present[r.chunkHash]: + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + default: + // Untouched sentence: keep the card exactly as she last saw it. + from, to := locate(contentText, r.original) + if from < 0 { + // The sentence is unchanged in substance but the quoted span no + // longer matches byte for byte — a quote mark the editor rewrote + // inside it, say. The frontend anchors by that string, so this card + // can't be shown; drop it and let the sentence be read again rather + // than cache advice nobody can see. + reopen[r.chunkHash] = true + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + continue + } + if err := reposition(tx, r, from, to, r.chunkHash); err != nil { + return err + } + } + } + for h := range reopen { + delete(present, h) + } + + sup, err := buildSuppressor(tx, docID) + if err != nil { + return err + } + + index := indexByEdit(inPlay) + kept := make(map[string]bool, len(inPlay)) + for _, s := range raw { + if sup.suppressed(s.Original, s.Replacement) { + continue + } + from, to := locate(contentText, s.Original) + // Attribute the finding to a sentence the model was actually shown before + // falling back to the whole document: a short span ("the the") can occur in + // two sentences, and crediting it to the cached one would drop it as advice + // we already have. + hash := chunkFor(s.Original, fresh) + if hash == "" { + hash = chunkFor(s.Original, chunks) + } + // A sentence we didn't ask about already has whatever advice it deserves. + // The model can't normally quote one — it was only shown the delta — but if + // it wanders there anyway, the cached card stands rather than gaining a + // twin. + if !inPlayAll && hash != "" && present[hash] && !asked[hash] { + continue + } + if row, ok := index.take(s.Original, s.Replacement, from); ok { + kept[row.id] = true + if err := reposition(tx, row, from, to, hash); err != nil { + return err + } + continue + } + typ := scope.forceType + if typ == "" { + typ = normalizeType(s.Type) + } + if _, err := tx.Exec( + `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source, chunk_hash) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + docID, from, to, s.Original, s.Replacement, s.Explanation, typ, db.SuggestionSourceLLM, hash, + ); err != nil { + return err + } + } + + // Asked about and not proposed again: the model has changed its mind, or she + // has fixed it. + for _, r := range inPlay { + if kept[r.id] { + continue + } + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + } + + // Record the sentences this family has now read. Every sentence still in the + // document has been read by *some* pass: the ones just asked about now, the + // rest in an earlier round. + if scope.chunked { + if _, err := tx.Exec( + `DELETE FROM checked_chunks WHERE doc_id = ? AND family = ?`, docID, scope.family, + ); err != nil { + return err + } + for h := range present { + if _, err := tx.Exec( + `INSERT INTO checked_chunks (doc_id, family, hash) VALUES (?, ?, ?)`, + docID, scope.family, h, + ); err != nil { + return err + } + } + } + + return tx.Commit() +} + +// checkedChunks loads the sentences a family read on its last pass. +func (h *Handler) checkedChunks(docID, family string) (map[string]bool, error) { + rows, err := h.DB.Query( + `SELECT hash FROM checked_chunks WHERE doc_id = ? AND family = ?`, docID, family, + ) + if err != nil { + return nil, err + } + defer rows.Close() + + out := map[string]bool{} + for rows.Next() { + var hash string + if err := rows.Scan(&hash); err != nil { + return nil, err + } + out[hash] = true + } + return out, rows.Err() +} diff --git a/internal/suggestions/stability_test.go b/internal/suggestions/stability_test.go new file mode 100644 index 0000000..20ffa90 --- /dev/null +++ b/internal/suggestions/stability_test.go @@ -0,0 +1,191 @@ +package suggestions + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + "gitea.parodia.dev/drwily/petal/internal/db" + "gitea.parodia.dev/drwily/petal/internal/llm" +) + +// byOriginal indexes a pending set by the text each card flags. +func byOriginal(in []db.Suggestion) map[string]db.Suggestion { + out := map[string]db.Suggestion{} + for _, s := range in { + out[s.Original] = s + } + return out +} + +// TestUntouchedSentencesKeepTheirCards is the heart of the stability work: she +// edits one sentence, and the cards on every other sentence stay exactly as they +// were — same id (so the rail keeps the card instead of remounting it), same +// explanation (the model re-words its reasoning every time it is asked, and one +// unchanged mistake used to carry three different explanations in a sitting). +// The model is only asked about the sentence that changed. +func TestUntouchedSentencesKeepTheirCards(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has two apple","replacement":"I have two apples","explanation":"first wording","type":"grammar"}, + {"original":"She go to market","replacement":"She goes to market","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + setDocText(t, h, docID, "I has two apple. She go to market yesterday.") + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var first []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &first); err != nil { + t.Fatalf("decode: %v", err) + } + if len(first) != 2 { + t.Fatalf("first pass: want 2, got %d: %+v", len(first), first) + } + kept := byOriginal(first)["I has two apple"] + + // She fixes only the second sentence. The model, asked again, re-words its + // reasoning about the first — which it must never get the chance to do. + setDocText(t, h, docID, "I has two apple. She goes to market yesterday.") + client.response = `{"suggestions":[ + {"original":"I has two apple","replacement":"I have two apples","explanation":"REWORDED","type":"grammar"} + ]}` + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + + if strings.Contains(client.lastPrompt, "I has two apple") { + t.Fatalf("untouched sentence was sent to the model:\n%s", client.lastPrompt) + } + if !strings.Contains(client.lastPrompt, "She goes to market") { + t.Fatalf("edited sentence was not sent to the model:\n%s", client.lastPrompt) + } + + now := byOriginal(second)["I has two apple"] + if now.ID != kept.ID { + t.Fatalf("card was remounted: id %q became %q", kept.ID, now.ID) + } + if now.Explanation != "first wording" { + t.Fatalf("explanation drifted: %q", now.Explanation) + } + // The fixed sentence's card is gone, and the model's stray re-proposal for the + // cached sentence did not become a second card. + if len(second) != 1 { + t.Fatalf("want exactly one card left, got %d: %+v", len(second), second) + } +} + +// TestUnchangedDocumentSkipsTheModel proves a check with nothing new to read +// costs nothing: no model call, and every card left standing untouched. This is +// the doc-open and tone-less re-check path. +func TestUnchangedDocumentSkipsTheModel(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var first []db.Suggestion + _ = json.Unmarshal(rec.Body.Bytes(), &first) + if len(first) != 1 || client.calls != 1 { + t.Fatalf("first pass: %d cards, %d calls", len(first), client.calls) + } + + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + if client.calls != 1 { + t.Fatalf("re-checking an unedited document called the model %d times", client.calls) + } + if len(second) != 1 || second[0].ID != first[0].ID { + t.Fatalf("card did not survive an idle re-check: %+v", second) + } +} + +// TestDeletedSentenceDropsItsCard covers the other half of the skip path: she +// removes a flagged sentence outright, so nothing changed that the model could +// be asked about — but its card must still go. +func TestDeletedSentenceDropsItsCard(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + setDocText(t, h, docID, "") + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var got []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got) != 0 { + t.Fatalf("card outlived its sentence: %+v", got) + } +} + +// TestToneChangeReopensEverySentence: the checkpoint's advice is written for the +// document's tone, so switching from a journal to an academic essay has to +// re-read sentences that haven't changed a character. +func TestToneChangeReopensEverySentence(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + if _, err := h.DB.Exec(`UPDATE documents SET tone = 'academic' WHERE id = ?`, docID); err != nil { + t.Fatalf("set tone: %v", err) + } + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + + if client.calls != 2 { + t.Fatalf("tone change did not re-read the document: %d model calls", client.calls) + } +} + +// TestMechanicsFindingsKeepTheirRows: the rule pack re-runs 250 ms after every +// keystroke. A finding it still reports must keep its row, or the rail would +// remount several times a sentence — collapsing a card she has open, and +// re-firing the arrival chime for advice she is already reading. +func TestMechanicsFindingsKeepTheirRows(t *testing.T) { + srv, docID, _ := newTestServer(t, &stubClient{response: `{"suggestions":[]}`}) + body := `{"findings":[ + {"from":0,"to":5,"original":"I has","replacement":"I have","explanation":"agreement"}, + {"from":6,"to":15,"original":"two apple","replacement":"two apples","explanation":"plural"} + ]}` + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/mechanics", body) + var first []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &first); err != nil { + t.Fatalf("decode: %v", err) + } + if len(first) != 2 { + t.Fatalf("want 2 rows, got %d", len(first)) + } + + // She types elsewhere: same findings, shifted spans, one of them now fixed. + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/mechanics", `{"findings":[ + {"from":20,"to":25,"original":"I has","replacement":"I have","explanation":"agreement"} + ]}`) + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + if len(second) != 1 { + t.Fatalf("want 1 row, got %d: %+v", len(second), second) + } + if second[0].ID != byOriginal(first)["I has"].ID { + t.Fatalf("surviving finding was given a new identity: %+v", second[0]) + } + if second[0].FromPos != 20 { + t.Fatalf("span did not follow the text: %+v", second[0]) + } +} From de251ceae2380d62bb5d4f55bd7577ba13238d2e Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Mon, 27 Jul 2026 23:07:09 -0700 Subject: [PATCH 3/3] Make the suggestion rail's overhang reachable, and keep the text in view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The margin rail hangs off an absolutely-positioned column, so its cards add no layout height. On her live document that meant four 173px cards anchored inside 126px of text: a 714px stack over a page whose scrollHeight equalled its clientHeight. The lower cards weren't far from their sentence, they were off-screen with nothing to scroll. The rail now reports how far its resolved stack reaches and the wrapper takes that as a minimum height, so the space those cards occupy is real, scrollable page. minHeight never shrinks the column, so a rail that fits beside its text is unaffected. Scrolling into that space would have carried every sentence off the top, so the prose is pinned while the stack overhangs it. The offset is min(0, port - content): prose shorter than the viewport pins at the top, taller prose pins by its bottom edge, keeping the last lines visible — those are the ones the overhanging cards flag. The prose box has to stay at its natural height. Keeping the old h-full made it measure the wrapper this change had just grown, reporting the cards' height as the text's own, so the pin could never trip. Verified in a browser at the review's 1517x810, driven offline by the rule pack: 8 cards over 95px of prose gained 675px of scroll where there was none, the last card lands fully in view with the text still on screen, tall prose pins bottom-anchored without disturbing ordinary scrolling, and hover-linking still glows the right span. Claude-Session: https://claude.ai/code/session_016y6gyuHkQXPiEuW8RGQyua --- UX_REVIEW_2026-07-27.md | 74 ++++++++++++++++++++ web/src/App.tsx | 5 +- web/src/components/Editor/EditorCore.tsx | 71 ++++++++++++++++++- web/src/components/Editor/SuggestionRail.tsx | 10 +++ 4 files changed, 157 insertions(+), 3 deletions(-) diff --git a/UX_REVIEW_2026-07-27.md b/UX_REVIEW_2026-07-27.md index 840c079..29c1e10 100644 --- a/UX_REVIEW_2026-07-27.md +++ b/UX_REVIEW_2026-07-27.md @@ -302,6 +302,74 @@ takes over when the stack exceeds the viewport. **Acceptance:** with 10+ suggestions, the flagged text stays visible while scrolling the card list; hover-linking still highlights the right span. +### 4 — DONE (fifth session). The cards weren't distant; they were unreachable. + +Measured on the live build before touching anything, and the item understates +its own bug. Her open document: **four cards, 173 px each, all anchored inside +126 px of text** — the stack resolved to tops 4 / 189 / 374 / 559, so ~714 px +of cards beside four lines of prose. And because `.petal-rail` is +`position: absolute`, none of that counts as layout height: the page reported +`scrollHeight === clientHeight`, **no scroll container at all**. On the review's +810 px viewport the lower cards weren't merely severed from their sentence, +they were off-screen with no way to scroll to them. That is the real defect, +and it is why the item read as a scrolling problem. + +**Implemented:** + +- `SuggestionRail.tsx` — the stack reports how far it reaches (`onExtent`), + computed in the same pass that resolves the collision-avoided tops. +- `EditorCore.tsx` — the wrapper takes `minHeight: railExtent + 24`, so the + space the cards occupy becomes real, scrollable page. `minHeight` never + shrinks the column, so a rail that fits beside its text changes nothing. +- The prose moved into its own box, pinned with `position: sticky` while the + stack overhangs it, so scrolling down to reach the lower cards no longer + carries every sentence off the top. The offset is `min(0, port − content)`: + prose shorter than the viewport pins at the top; **taller prose pins by its + bottom edge**, so the last lines — the ones the overhanging cards flag — + stay visible rather than the first. +- The extent is cleared when the last card goes, or the window narrows past + the rail's threshold; otherwise the column keeps the height of a stack that + no longer exists. + +**A trap worth recording.** That prose box must be left at its natural height. +The first version kept the existing `h-full`, so it measured the wrapper — which +this change had just grown to the stack's height — and reported the cards' +height back as the text's own. `railExtent > contentH` was then never true and +the pin could never trip. It typechecked, looked right, and did nothing; only +measuring the running page caught it (`proseHeight: 1424` for a two-line +document). + +**Verified in a real browser at the review's own 1517×810**, driving the local +build with the rule pack from item 3b — which needs no model, so eight cards +appear offline in one paragraph. All three branches exercised: + +- *Overhang, short prose* — 8 cards, stack 1400 px, prose 95 px. Page gained + 675 px of scroll where it previously had none; scrolled to the end, the last + card sits fully in view (770–926) **and the prose is still on screen** (80–175). +- *Overhang, tall prose* — port 225 px, prose 347 px → `top: −146px`. Ordinary + scrolling is untouched (at `scrollTop` 200 the text moves normally with the + page); only at the overhang does it pin, bottom-anchored, last lines visible. +- *No overhang* — the port stays unscrollable and nothing moves. + +Hover-linking re-checked on the last card, the one this fix made reachable at +all: it glows the right span ("It make"), the span is on screen, the card lifts. + +**Known limit, not fixed.** The overlays anchored in wrapper coordinates (gloss +tip, selection bubble, word/misspell cards, confetti) rely on the invariant +noted at `recomputeRail` — "stable under scroll since text and wrapper scroll +together" — which the pin breaks. They are still placed correctly when opened, +because their coordinates come from live rects; they drift only if she scrolls +*while one is open* *and* the column is pinned, i.e. inside the overhang. Left +alone rather than papered over; if it ever bites, the fix is to close or +re-anchor them on scroll. + +**Deliberately not done:** no compaction of the cards. Making crowded cards +drop to a one-line form is the obvious way to shorten the stack, and it is +wrong here — the explanation *is* the teaching, and hiding it from an ESL +writer to save vertical space trades the product's purpose for tidiness. Ten +cards cannot sit beside four lines of text; the answer is to make the overhang +navigable, not to shrink what each card says. + ## 5. Mixed-language spans: offer translation, don't ignore **Status (follow-up session): premise partly wrong — re-scope before @@ -421,6 +489,12 @@ incremental-surfacing half is now cheap — the chunking it was waiting on exists — but it needs streaming, which the current `/check` shape doesn't do.)* +*(Fifth session: item 4 done — see the subsection under it. Still nothing +deployed: `main` remains at `ba06d90`, and 3b → 2 → 4 are now three stacked +topic branches. **Merging and deploying that stack is the obvious next move** +— three sessions of work she hasn't seen. Untouched: 6, 7, 8, item 3's +incremental half, and item 5's re-scoped Translate card type.)* + **Suggested next:** item 3b, the instant local rules layer — but it is **largely already built, in `main`**, and the item as written doesn't know that. Before writing any rules engine, read: diff --git a/web/src/App.tsx b/web/src/App.tsx index 95606cf..bc3fa11 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -518,7 +518,10 @@ export default function App() { <>
{/* Title, then the three chrome pills. Their labels are diff --git a/web/src/components/Editor/EditorCore.tsx b/web/src/components/Editor/EditorCore.tsx index 3e6fef5..0a5e317 100644 --- a/web/src/components/Editor/EditorCore.tsx +++ b/web/src/components/Editor/EditorCore.tsx @@ -15,7 +15,7 @@ import TableHeader from '@tiptap/extension-table-header' import TableCell from '@tiptap/extension-table-cell' import { FontSize } from './FontSize' import type { EditorView } from '@tiptap/pm/view' -import { useCallback, useEffect, useRef, useState } from 'react' +import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react' import { Toolbar } from '../Toolbar/Toolbar' import { SuggestionCard } from './SuggestionCard' import { SuggestionRail, type RailItem } from './SuggestionRail' @@ -34,6 +34,10 @@ import { speak, speechSupported } from '../../audio/speech' import type { SpellChecker } from '../../hooks/useSpellChecker' import { usePack } from '../../i18n' +// Breathing room left below the last suggestion card when the rail's stack is what +// defines the column's height, so the bottom card doesn't sit flush on the edge. +const RAIL_TAIL = 24 + export interface EditorChange { content: string // Tiptap JSON, stringified content_text: string // flattened plain text for the LLM @@ -229,6 +233,9 @@ export function EditorCore({ // own name, so it says "português" rather than "pt-PT". const pack = usePack() const wrapperRef = useRef(null) + // The text column itself, measured separately from its wrapper: the wrapper is + // grown to cover the card stack, so only this reports the height of the prose. + const contentRef = useRef(null) const [hover, setHover] = useState(null) // The open spelling popover (click a red-underlined word), or null. const [misspell, setMisspell] = useState(null) @@ -266,6 +273,15 @@ export function EditorCore({ // `activeId` is the suggestion currently emphasized (hovered text or card). const [railItems, setRailItems] = useState([]) const [railEnabled, setRailEnabled] = useState(false) + // How far the resolved card stack reaches below the wrapper's top, reported by + // the rail. Cards are absolutely positioned and so contribute no layout height: + // without this the column below the last line of text isn't scrollable and any + // card that lands there is unreachable, not merely far from its sentence. + const [railExtent, setRailExtent] = useState(0) + // Sticky offset for the text column, or null when it should sit in normal flow. + // Set only while the stack overhangs the text: scrolling down to reach the lower + // cards would otherwise carry every sentence off the top of the screen. + const [stickTop, setStickTop] = useState(null) const [railExpandedId, setRailExpandedId] = useState(null) const [activeId, setActiveId] = useState(null) // A stable handle to the latest recompute so the editor's onUpdate (captured @@ -444,6 +460,42 @@ export function EditorCore({ } }, [recomputeRail]) + // The rail only reports its extent while it's mounted, so clear it when the last + // card goes (accepted the lot, or the window narrowed past the rail's threshold) + // — otherwise the column keeps the height of a stack that no longer exists. + useEffect(() => { + if (!railEnabled || railItems.length === 0) setRailExtent(0) + }, [railEnabled, railItems.length]) + + // Decide whether the text column has to be pinned. The rail's cards hang off an + // absolutely-positioned column, so when several suggestions share one short + // paragraph the stack runs far past the last line of text. Growing the wrapper to + // `railExtent` makes that space scrollable (item 4: the lower cards were simply + // unreachable); pinning the prose inside it means scrolling down to read those + // cards keeps the sentences on screen instead of scrolling them away. + // + // The offset is `min(0, port - content)`: prose shorter than the viewport sticks + // at the top, taller prose sticks by its *bottom* edge, so its last lines — the + // ones the overhanging cards flag — stay visible rather than the first. + useLayoutEffect(() => { + const wrapper = wrapperRef.current + const content = contentRef.current + if (!wrapper || !content || !railEnabled || railExtent <= 0) { + setStickTop(null) + return + } + const contentH = content.offsetHeight + // Only pin when the stack actually overhangs the prose; a rail that fits + // beside its text needs nothing, and pinning it would be a change for free. + if (railExtent <= contentH) { + setStickTop(null) + return + } + const port = wrapper.closest('.petal-scrollport') + const portH = port ? port.clientHeight : window.innerHeight + setStickTop(Math.min(0, portH - contentH - RAIL_TAIL)) + }, [railEnabled, railExtent, railItems]) + // Emphasize the flagged text for the active suggestion, mirroring the rail // card ↔ text link both ways. Driven through the decoration plugin (not an // imperative DOM class) so it survives the repaints that fire on every edit. @@ -1082,6 +1134,10 @@ export function EditorCore({
0 ? { minHeight: railExtent + RAIL_TAIL } : undefined} onMouseOver={handleMouseOver} onMouseOut={handleMouseOut} onMouseMove={handleMouseMove} @@ -1093,7 +1149,17 @@ export function EditorCore({ onTouchMove={cancelLongPress} onTouchEnd={cancelLongPress} > - + {/* The prose sits in its own box so it can be measured (and pinned) + independently of the wrapper, which the rail may have grown. The box is + deliberately left at its natural height: sized to the wrapper it would + report the stack's height back as the text's own, and the pin below + could never trip. */} +
+ +
{findOpen && editor && setFindOpen(false)} />} {confetti && } {gloss && ( @@ -1165,6 +1231,7 @@ export function EditorCore({ onHover={setActiveId} onActivate={activateRailCard} onToggleExpand={toggleRailExpand} + onExtent={setRailExtent} /> )}
diff --git a/web/src/components/Editor/SuggestionRail.tsx b/web/src/components/Editor/SuggestionRail.tsx index 3295edd..1b970a5 100644 --- a/web/src/components/Editor/SuggestionRail.tsx +++ b/web/src/components/Editor/SuggestionRail.tsx @@ -28,6 +28,12 @@ interface Props { // A card's body was clicked — scroll its highlight into view and toggle expand. onActivate: (id: string) => void onToggleExpand: (id: string) => void + // How far down the resolved stack reaches (px below the wrapper's top). Cards + // are absolutely positioned, so they add nothing to layout height — a cluster of + // errors in one short paragraph can pile cards hundreds of px past the end of the + // text, with no scrollable space to reach them. The editor uses this to grow the + // column so every card can at least be scrolled to. + onExtent: (bottom: number) => void } // SuggestionRail is the right-margin "comment column": every outstanding @@ -44,6 +50,7 @@ export function SuggestionRail({ onHover, onActivate, onToggleExpand, + onExtent, }: Props) { // Measured resolved tops keyed by suggestion id (after collision avoidance). const [tops, setTops] = useState>({}) @@ -65,13 +72,16 @@ export function SuggestionRail({ const layoutKey = ordered.map((i) => `${i.suggestion.id}:${Math.round(i.anchorTop)}`).join('|') useLayoutEffect(() => { let cursor = -Infinity + let bottom = 0 const next: Record = {} for (const { suggestion, anchorTop } of ordered) { const h = cardRefs.current.get(suggestion.id)?.offsetHeight ?? 96 const top = Math.max(anchorTop, cursor) next[suggestion.id] = top cursor = top + h + CARD_GAP + bottom = top + h } + onExtent(bottom) setTops((prev) => { const ids = Object.keys(next) if (ids.length === Object.keys(prev).length && ids.every((id) => prev[id] === next[id])) return prev