You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Astro-Han (via GitHub)" <gi...@apache.org> on 2026/09/10 16:39:36 UTC
[PR] refactor(desktop): give the Renderer the transcript window [maka]
Astro-Han opened a new pull request, #5170:
URL: https://github.com/apache/maka/pull/5170
## Summary
Main owned the presentation window — a resident range with reading anchors, navigation versions, overlay settlement across pages and eviction bookkeeping — while the Renderer kept its own scroll state on top. Two owners of one window meant every reader gesture met Main's range accounting, and the reading position existed in six representations that all had to agree.
Main now keeps a tail cache and answers page requests pass-through. `loadBefore` / `loadAfter` return a page without touching the cache, `loadAround` and `loadLatest` return a reset snapshot, and catch-up evicts whole Turns from the oldest edge while always keeping the newest Turn. The Renderer owns the window: it extends it by pixel bandwidth, trims it with `retain`, and re-opens the edge it trimmed as a gap.
This resolves the three contracts raised on #5163:
1. **Completing a visible Turn while unpinned.** Main used to broadcast `completedOverlayMessageIds`, so a window retired an overlay whether or not it installed the durable row replacing it. A window off the tail declines tail growth — so it deleted the overlay and dropped the body in the same batch, and the Turn vanished from the reader's view. The catch-up broadcast also filtered its rows through Main's own tail residency, so a row installed and immediately evicted under budget reached no window at all. Retirement is now the window's own inference (installing a durable row retires the overlay it settles) and the broadcast carries every row the catch-up read.
2. **Host paging cursors vs. Renderer gaps.** `hasOlder` / `hasNewer` are transport-level cursors on a Host answer. The Renderer combines them with its own coverage: an edge it trimmed to meet its budget becomes a gap again on the spot, so paging reaching an end never implies nothing exists outside the window.
3. **Scope of the staleness mechanism.** `navigationVersion` invalidates navigation only. Session id, replica generation and Host epoch remain independent identity checks on every batch. Invalidation also covers late success, errors, replay and fragmented delivery: an in-flight page for a replaced window has its error swallowed rather than surfaced as the current window's failure, and a new version clears partially received fragments so a late fragment cannot splice into the new window.
The reading position is now one representation: the scroll authority publishes the Turn crossing the top of the scrollport, the prompt rail derives its tick from that instead of running its own IntersectionObserver / MutationObserver / rAF loop, and the controller keeps a bookmark to re-anchor after a replica generation change.
Supersedes #5147, which patched these symptoms at the old window authority.
Closes #5163
## Behavior change
- Opening a Session prefetches history until two viewports sit above the reader.
- Tail growth marks read while any consumer is open, where it previously waited for the reader to have nothing newer.
## Verification
Regression for the vanishing Turn (`transcript-overlay-settlement.test.ts`, "a window parked off the tail keeps the Turn it is reading when that Turn completes") fails without the fix, on the reader-visible body:
```text
AssertionError: the completed body replaces the overlay instead of both disappearing
actual: [ 'B partial' ]
expected: [ 'B partial and completed answer' ]
```
```text
apps/desktop — tsc --noEmit on tsconfig.{main,renderer,preload,storybook}.json
0 errors
apps/desktop — node --test dist/main/__tests__/{transcript-*,desktop-transcript-*,
runtime-host-session-observer,workhub-coordination-transcript-preload,
app-shell-session-ui-state,runtime-host-session-execution-ipc-main}.test.js
174 pass / 0 fail
packages/ui — node --test dist/__tests__/{use-chat-scroll,prompt-anchor-rail,
transcript-scroll-authority,prompt-rail-reading-position}.test.js
34 pass / 0 fail
apps/desktop — playwright e2e/transcript-scroll-cost.spec.ts e2e/partial-history-notice.spec.ts
4 passed (28.3s)
root — npm run format && npm run lint
clean
```
Not run: the full repository suite, and the cold-restart upward-scroll displacement trace proposed on #5163 — the existing `transcript-scroll-cost` suite asserts structural cost and reachability, not frame-by-frame reader displacement. That trace is tracked as remaining work below.
## Remaining work
- [ ] Cold-restart continuous-upward-scroll trace over the 120-Turn and 640-Turn fixtures, recording first visible `turnId + offsetPx`, page requests, resets, mounted Turns and long tasks per gesture, asserting no displacement at page boundaries. The renderer-replacement variant asserts exactly one live transcript consumer first, so #5136 cannot contaminate it.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Code — implementation, test authoring and verification across Main, preload and Renderer, under human review.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(desktop): give the Renderer the transcript window [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3981561561
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -351,43 +332,55 @@ export class DesktopTranscriptRangeStore {
this.#batchChanged = false;
}
+ /**
+ * A batch answering a command is current only under the version that issued
+ * it. A reset for a new replica generation is always current: Main replaced
+ * the transcript underneath every window. Broadcasts carry no version.
+ */
accepts(batch: DesktopTranscriptBatchPayload): boolean {
- // A stale reset must be rejected before it can clear the current range.
- if ((batch.navigationVersion ?? 0) !== this.#navigationVersion) return false;
if (this.#retiredGenerations.has(batch.generation)) return false;
- return batch.reset || (
- batch.sessionId === this.#sourceSessionId &&
+ if (batch.reset) {
+ return batch.navigationVersion === undefined ||
+ batch.navigationVersion === this.#navigationVersion ||
+ batch.generation !== this.#liveGeneration;
Review Comment:
**Blocking — admit the whole reconnect snapshot, not only its reset batch.**
After `loadAround` or `loadLatest`, the Renderer expects navigation N > 0. [Registry recovery](https://github.com/apache/maka/blob/bba473c27d9cc87c9cde51d0f68f68727ccd22e7/apps/desktop/src/main/runtime-host-session-observation-registry.ts#L546-L560) reopens the consumer, whose [navigation version starts at 0](https://github.com/apache/maka/blob/bba473c27d9cc87c9cde51d0f68f68727ccd22e7/apps/desktop/src/main/runtime-host-session-observer.ts#L299-L304). This exception admits the new generation's first batch, but only that batch has `reset=true`; lines 347–348 reject every continuation carrying version 0. Preload still ACKs those batches, including the final `ready` batch.
A direct probe against this head — initialize the store, `expectNavigation(1)`, then feed an encoded new-generation/version-0 snapshot containing a 300 KiB message — leaves `range().ready === false` and the replacement message incomplete. The existing navigation-race tests pass because they do not exercise this combination.
Give the entire recovery snapshot one consistent generation-scoped admission rule, while keeping navigation filtering for command answers. Removing Main's navigation replay should not require restoring Main's presentation authority.
<details><summary>中文</summary>
执行过导航后，重连的新 consumer 从版本 0 开始；新 generation 仅首批因 reset 特例获准，续批及 ready 均被旧导航版本过滤，但仍被 ACK。300 KiB 快照定向复现停在 ready=false。恢复快照整条流应使用一致的 generation 接收规则，命令回复仍保留导航版本过滤。
</details>
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -48,40 +54,52 @@ export function createDesktopTranscriptRangeController(
let closed = false;
let openController = new AbortController();
let handle = open(openController.signal);
- type Navigation = DesktopTranscriptNavigation & {
- readonly kind: 'before' | 'after' | 'around' | 'latest' | 'anchor';
- readonly sequence: number | null;
- readonly maxBytes?: number;
- };
- let navigation: Navigation = {
- navigationVersion: 0, intent: 'followTail', kind: 'latest', sequence: null,
- };
+ let navigationVersion = 0;
+ const extending: { older?: Promise<void>; newer?: Promise<void> } = {};
const current = async () => {
if (closed) throw new Error('Desktop transcript range is closed');
return handle;
};
- const dispatch = async (command: Navigation) => {
+ const command = async (
+ replace: boolean,
+ run: (value: DesktopTranscriptHandle, navigation: DesktopTranscriptNavigation) => Promise<void>,
+ ) => {
+ if (replace) {
+ navigationVersion += 1;
+ // Invalidate before awaiting an open handle or any in-flight page.
+ store.expectNavigation(navigationVersion);
+ }
+ const version = navigationVersion;
const opening = handle;
- const isCurrent = () => !closed && navigation === command && opening === handle;
+ const isCurrent = () => !closed && version === navigationVersion && opening === handle;
try {
const value = await current();
if (!isCurrent()) return;
- if (command.kind === 'before') {
- await value.loadBefore(command.sequence, command.maxBytes, command);
- } else if (command.kind === 'after') {
- await value.loadAfter(command.sequence, command.maxBytes, command);
- } else {
- await value.loadAround(command.sequence, command.maxBytes, command);
- }
+ await run(value, { navigationVersion: version });
} catch (error) {
if (isCurrent()) throw error;
}
};
- const navigate = (command: Omit<Navigation, 'navigationVersion'>) => {
- navigation = { ...command, navigationVersion: navigation.navigationVersion + 1 };
- // Invalidate before awaiting an open handle or any previous page request.
- store.expectNavigation(navigation.navigationVersion);
- return dispatch(navigation);
+ const extend = (edge: 'older' | 'newer', maxBytes: number): Promise<void> => {
+ const pending = extending[edge];
+ if (pending) return pending;
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return Promise.resolve();
+ }
+ if (edge === 'older' ? !range.hasOlder : !range.hasNewer) return Promise.resolve();
+ const anchor = edge === 'older' ? range.oldestSequence : range.newestSequence;
+ const task = command(false, (value, navigation) =>
+ edge === 'older'
+ ? value.loadBefore(anchor, maxBytes, navigation)
+ : value.loadAfter(anchor, maxBytes, navigation),
Review Comment:
**Blocking — reject extensions that no longer join the retained window.**
An extension captures this edge anchor, but `store.retain()` can remove that edge without invalidating the request. Its answer still carries the current navigation version, so `accept()` installs it unconditionally as a command answer.
A delayed-read probe against the production controller/store starts with rows 10..20, begins `loadBefore(10)`, retains 18..20 while the read is outstanding, then returns row 9. The resulting window is **[9, 18, 19, 20]**: rows 10..17 are missing internally, but the model only exposes outer-edge gaps. Subsequent `loadBefore` anchors at 9 and `loadAfter` at 20, so neither fills the missing interval. This is reachable when a reader moves far enough to trim an edge before its network page returns; a small returned Turn need not trigger another trim.
At admission, require an extension to remain connected to the current window, or invalidate the affected in-flight extension when trimming its edge. Keep the single contiguous-window model instead of introducing internal-gap bookkeeping.
<details><summary>中文</summary>
扩页以旧边缘为锚点，retain 裁掉该边缘却不使请求失效。定向复现：10..20 开始向前读，期间保留 18..20，旧页 9 返回后得到 [9,18,19,20]。10..17 成为模型无法表达、两端分页无法补回的内部缺口。应在回复接收时验证仍连接当前窗口，或裁剪相应边缘时让在途扩页失效，无须增加多段窗口模型。
</details>
##########
packages/ui/src/use-chat-scroll.ts:
##########
@@ -171,55 +175,83 @@ export function useChatScroll(input: {
};
}, [authority, input.scrollRef, input.sessionId]);
+ // The window is a band of pixels around the reader: fetch when less than two
+ // screens remain in a direction that has history, drop what lies more than
+ // six screens away and keep four. Both are re-evaluated whenever content or
+ // the reader moves, so the tail prefetches its history and a reader who
+ // stops mid-transcript never pins more than a bounded slice in memory.
+ const bandCheck = useRef<(() => void) | undefined>(undefined);
useEffect(() => {
const root = input.scrollRef.current;
if (!root) return;
+ const inFlight = { up: false, down: false };
const canLoad = (direction: 'up' | 'down'): boolean => direction === 'up'
? input.hasOlderHistory === true && canLoadEarlier
: input.hasNewerHistory === true && canLoadLater;
- // Asking twice is the loader's problem, not this one's: it refuses a
- // request while one is in flight, and asking for history the reader
- // already has is idempotent anyway.
const requestHistory = (direction: 'up' | 'down'): void => {
- activation.current = { sessionId: input.sessionId };
- commandTarget.current = null;
- // Moving input already released following. Only an immovable edge can
- // still be pinned; do not cancel the input operation that requested data.
- if (authority.getSnapshot().pinned) authority.releasePin();
- // A wheel at either edge moves nothing, so no scroll event refreshes the
- // anchor and the restore effect would load around an evicted Turn.
- reportReadingAnchor.current?.();
- const anchorTurnId = direction === 'up'
- ? firstVisibleTurnId(root)
- : lastVisibleTurnId(root);
+ if (inFlight[direction]) return;
// The browser anchors the reader against everything that lands above
- // them, with one exception: it declines while the scroller sits at zero,
- // which is exactly where a wheel asks for history. One pixel is the whole
- // fix — measured in Chromium, an insert of 501px above the reader moves
- // `scrollTop` by 501 at an offset of 1 and by 0 at an offset of 0.
- if (direction === 'up' && root.scrollTop < 1) root.scrollTop = 1;
+ // them, with one exception: it declines while the scroller sits at zero.
+ // One pixel is the whole fix — measured in Chromium, an insert of 501px
+ // above the reader moves `scrollTop` by 501 at an offset of 1 and by 0
+ // at an offset of 0.
+ if (direction === 'up' && !authority.getSnapshot().pinned && root.scrollTop < 1) {
+ root.scrollTop = 1;
+ }
const load = direction === 'up' ? loadEarlierRef.current : loadLaterRef.current;
- void Promise.resolve(load?.(anchorTurnId)).catch(() => undefined);
+ inFlight[direction] = true;
+ void Promise.resolve(load?.()).catch(() => undefined).finally(() => {
+ inFlight[direction] = false;
+ check();
Review Comment:
**Important — do not immediately retry an unchanged failed prefetch.**
The `finally` clears the in-flight flag and calls `check()` even after failure. If the geometry and history flag are unchanged, `check()` immediately issues the same request again. Desktop's `loadHistory` also catches errors, reports them, and resolves normally, so checking rejection alone would not fix the production path.
With the existing two-Turn fixture and a rejecting loader, a single mount issued **20 consecutive requests without input or range changes**; the probe deliberately left request 20 pending to stop the loop. A persistent Host/IPC read failure therefore causes unbounded requests and error reporting.
The messages-change effect already rechecks the band after successful data installation. Remove the unconditional completion-driven retry, or require actual progress before continuing; failures should await a fresh input, range change, or recovery trigger.
<details><summary>中文</summary>
失败后 finally 立即 check，相同边缘和 history flag 又触发相同请求。单次挂载定向复现连续请求 20 次，测试主动悬停第 20 次才停止。Desktop 会捕获错误并正常 resolve，因此只判断 reject 不够。成功的数据变化已有重新检查入口；失败应等待新输入、范围变化或恢复事件。
</details>
##########
packages/ui/src/use-chat-scroll.ts:
##########
@@ -171,55 +175,83 @@ export function useChatScroll(input: {
};
}, [authority, input.scrollRef, input.sessionId]);
+ // The window is a band of pixels around the reader: fetch when less than two
+ // screens remain in a direction that has history, drop what lies more than
+ // six screens away and keep four. Both are re-evaluated whenever content or
+ // the reader moves, so the tail prefetches its history and a reader who
+ // stops mid-transcript never pins more than a bounded slice in memory.
+ const bandCheck = useRef<(() => void) | undefined>(undefined);
useEffect(() => {
const root = input.scrollRef.current;
if (!root) return;
+ const inFlight = { up: false, down: false };
const canLoad = (direction: 'up' | 'down'): boolean => direction === 'up'
? input.hasOlderHistory === true && canLoadEarlier
: input.hasNewerHistory === true && canLoadLater;
- // Asking twice is the loader's problem, not this one's: it refuses a
- // request while one is in flight, and asking for history the reader
- // already has is idempotent anyway.
const requestHistory = (direction: 'up' | 'down'): void => {
- activation.current = { sessionId: input.sessionId };
- commandTarget.current = null;
- // Moving input already released following. Only an immovable edge can
- // still be pinned; do not cancel the input operation that requested data.
- if (authority.getSnapshot().pinned) authority.releasePin();
- // A wheel at either edge moves nothing, so no scroll event refreshes the
- // anchor and the restore effect would load around an evicted Turn.
- reportReadingAnchor.current?.();
- const anchorTurnId = direction === 'up'
- ? firstVisibleTurnId(root)
- : lastVisibleTurnId(root);
+ if (inFlight[direction]) return;
// The browser anchors the reader against everything that lands above
- // them, with one exception: it declines while the scroller sits at zero,
- // which is exactly where a wheel asks for history. One pixel is the whole
- // fix — measured in Chromium, an insert of 501px above the reader moves
- // `scrollTop` by 501 at an offset of 1 and by 0 at an offset of 0.
- if (direction === 'up' && root.scrollTop < 1) root.scrollTop = 1;
+ // them, with one exception: it declines while the scroller sits at zero.
+ // One pixel is the whole fix — measured in Chromium, an insert of 501px
+ // above the reader moves `scrollTop` by 501 at an offset of 1 and by 0
+ // at an offset of 0.
+ if (direction === 'up' && !authority.getSnapshot().pinned && root.scrollTop < 1) {
+ root.scrollTop = 1;
+ }
const load = direction === 'up' ? loadEarlierRef.current : loadLaterRef.current;
- void Promise.resolve(load?.(anchorTurnId)).catch(() => undefined);
+ inFlight[direction] = true;
+ void Promise.resolve(load?.()).catch(() => undefined).finally(() => {
+ inFlight[direction] = false;
+ check();
+ });
+ };
+ const check = (): void => {
+ if (!root.isConnected || bandCheck.current !== check) return;
+ const screen = Math.max(320, root.clientHeight);
+ const above = root.scrollTop;
+ const below = root.scrollHeight - root.clientHeight - root.scrollTop;
+ if (canLoad('up') && above < screen * 2) requestHistory('up');
+ if (canLoad('down') && below < screen * 2) requestHistory('down');
Review Comment:
**Blocking — automatic prefetch clears an outstanding search before its target arrives.**
A global search can open another Session with a target outside its initial tail. If that bootstrap is short (e.g. two 600px Turns in a 600px viewport, with older history), the initial band check automatically requests an older page while the target's remote `loadAround` is still pending. This goes through [`loadHistory`](https://github.com/apache/maka/blob/bba473c27d9cc87c9cde51d0f68f68727ccd22e7/apps/desktop/src/renderer/features/conversation/controller/transcript-reading-position-controller.tsx#L130-L140), whose `cancel()` unconditionally clears the current search target and cancels restoration. When the remote target finally mounts, the reveal command has already disappeared.
This does not require same-frame React timing or another reader gesture. The new mounted-target guard in this commit does not cover it: prefetch runs before that guard, and this target is not mounted yet.
Background filling must not consume explicit navigation intent. Keep it from cancelling an outstanding search/restoration, or defer that filling until the explicit navigation settles. A focused regression should delay the searched-for page, let the short-tail prefetch run, then deliver the target and assert it is actually revealed.
<details><summary>中文</summary>
跨 Session 搜索时，目标页仍在远端加载，短 tail bootstrap 会先自动补页；这经 loadHistory → cancel 清除尚未处理的搜索目标。目标稍后挂载也不再定位，无需假设同一帧内的渲染顺序。新 guard 在预取之后，而且只保护已挂载目标，未封住此路径。后台补页不能消费显式导航意图。
</details>
##########
apps/desktop/e2e/transcript-scroll-cost.spec.ts:
##########
@@ -125,6 +167,100 @@ async function observe(page: Page): Promise<void> {
});
}
+/**
+ * Watch every frame for a change in the mounted range, and measure what that
+ * change did to the reader.
+ *
+ * A Turn the reader can still see is at `top` in the viewport and at
+ * `top + scrollTop` in the document. Between two frames with no input, its
+ * document position must not move, so `Δtop + ΔscrollTop` is zero — whatever
+ * the Renderer installed above it, the browser's scroll anchoring absorbed. A
+ * page that displaces the reader breaks that sum by however tall the rows it
+ * added or dropped were.
+ *
+ * Sampled per frame rather than per gesture: the frame that installs a page is
+ * the only one where the reader can be lost, and a per-gesture reading would
+ * subtract the reader's own scrolling back out and see nothing.
+ */
+async function observeDisplacement(page: Page): Promise<void> {
+ await page.evaluate((scrollerSelector) => {
+ const scroller = document.querySelector(scrollerSelector);
+ if (!scroller) throw new Error('the chat scroll container is missing');
+ const read = () => {
+ const tops = new Map<string, number>();
+ for (const turn of document.querySelectorAll<HTMLElement>('[data-turn-id]')) {
+ const turnId = turn.dataset.turnId;
+ if (turnId) tops.set(turnId, turn.getBoundingClientRect().top);
+ }
+ return { scrollTop: scroller.scrollTop, tops, key: [...tops.keys()].join(',') };
+ };
+ const state: { boundaries: unknown[]; peakMounted: number; stop(): void } = {
+ boundaries: [],
+ peakMounted: 0,
+ stop: () => { running = false; },
+ };
+ let running = true;
+ let previous = read();
+ // The last frame before the range started changing. Held across a run of
+ // changing frames so the measurement spans settled state to settled state:
+ // scroll anchoring corrects after layout, so a reading taken inside the
+ // change would report a correction that never reached the screen.
+ let settled: ReturnType<typeof read> | null = null;
+ let peakMounted = 0;
+ const tick = (): void => {
+ if (!running) return;
+ const current = read();
+ peakMounted = Math.max(peakMounted, current.tops.size);
+ state.peakMounted = peakMounted;
+ if (current.key !== previous.key) {
+ if (!settled) settled = previous;
+ } else if (settled) {
+ const before = settled;
+ settled = null;
+ const scrolled = current.scrollTop - before.scrollTop;
+ let carried = 0;
+ let worstPx = 0;
+ let worstTurnId: string | null = null;
+ for (const [turnId, top] of current.tops) {
+ const wasAt = before.tops.get(turnId);
+ if (wasAt === undefined) continue;
+ carried += 1;
+ const displaced = Math.abs(top - wasAt + scrolled);
Review Comment:
**Important — this oracle measures document movement, not reader displacement.**
`top - wasAt + scrolled` is the change in the Turn's document coordinate. Scroll anchoring is meant to preserve its **viewport** coordinate while its document coordinate changes.
For example, prepend 100px above a carried Turn with no reader input:
- Correct anchoring: viewport top is unchanged and `scrollTop` increases by 100 → this reports **100px**.
- Broken anchoring: viewport top increases by 100 and `scrollTop` is unchanged → this also reports **100px**.
Conversely, an unintended 100px scroll-offset jump gives `Δtop=-100`, `ΔscrollTop=100`, so the metric reports **0px** when that Turn is compared. Thus the asserted threshold cannot establish the no-displacement behavior promised by this new test.
Compare the surviving reading anchor's viewport offset over a boundary without reader input, or compensate only for known reader input rather than the total scroll-offset change. Calibrate the oracle with a correctly anchored prepend and a deliberately displaced case before treating this as evidence for #5163.
<details><summary>中文</summary>
该公式算的是文档坐标变化，不是读者看到的位移。上方插入 100px，正确锚定（top 不变、scrollTop +100）与失效锚定（top +100、scrollTop 不变）都会报 100；反而纯 scrollTop 跳变会被抵消为 0。应测阅读锚点的视口偏移，排除或单独扣除读者输入，并先校准正确／错误锚定两种结果。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(desktop): give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5623068899
Thanks @M4n5ter — all five are fixed in aedc843, and each thread has the regression that fails without its fix. Rather than patch them one at a time I looked for what they share, and they collapse into two.
**The window had five writers and one of them could be invalidated.** Command answers, replica replacements, tail broadcasts, band trims and automatic fills all write the window, but the only invalidation mechanism was a *command* version. So a replacement was misfiltered as a stale command answer (#1), and a trim bypassed invalidation entirely, letting an extension splice rows onto an edge that no longer existed (#3) — your `[9, 18, 19, 20]`, a hole the contiguous-window model cannot even name.
The rule that covers all five writers is what each read assumed. An extension assumes one edge, so any replacement of that edge refuses it. A replacement assumes nothing about the edges, because it discards them, so only a newer navigation makes it stale. Batches that name no epoch answer nothing and apply to whatever the window holds. Promoting the counter this way removed a special case rather than adding one — the old `batch.generation !== this.#liveGeneration` branch is gone.
**Automatic filling had borrowed the reader's navigation channel.** `loadHistory` cancels restoration and clears the search target, which is right for a reader who decided where to be and wrong for a band that decided nothing (#2), and it swallows errors so failure could not be told from success (#4). It has its own command now, and it reports whether the window moved: a read that moved nothing — failed, or refused as stale — is not reissued until the reader or the range moves. That second half turned out to matter more than expected, because refusing an answer is exactly a read that moved nothing: without it, fixing #3 turned `partial-history-notice.spec.ts` from 3.4s into a 1.3m fetch loop.
On your oracle comment (#5) — you were right, and that formula had already misled me once before you got here. Details and the limits of what the probe can prove are in that thread.
Verified: 2426 `apps/desktop` tests, 410 `@maka/ui` tests, all four tsconfigs, Biome format and lint, and `partial-history-notice.spec.ts` + `transcript-scroll-cost.spec.ts` at 3 repeats each (12/12). `partial-history-notice.spec.ts` was the P0 flake you flagged: it is stable now, and its two remaining failure points both turned out to be real product behaviour rather than test fragility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5628398280
@Totoro-qaq Hi, the probe files would be indeed very useful! It would be great if you could share it on this PR discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3985069067
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -344,50 +329,84 @@ export class DesktopTranscriptRangeStore {
this.#expectedSessionId = sessionId;
}
- expectNavigation(navigationVersion: number): void {
- if (navigationVersion <= this.#navigationVersion) return;
- this.#navigationVersion = navigationVersion;
+ windowEpoch(): number {
+ return this.#windowEpoch;
+ }
+
+ navigationEpoch(): number {
+ return this.#navigationEpoch;
+ }
+
+ /**
+ * Mints the epoch for a window about to be replaced wholesale by a navigation,
+ * and drops the partially received records of the window being left behind.
+ */
+ replaceWindow(): number {
+ this.#navigationEpoch = this.#mintWindow();
+ return this.#navigationEpoch;
+ }
+
+ #mintWindow(): number {
+ this.#windowEpoch += 1;
this.#pending.clear();
this.#batchChanged = false;
+ return this.#windowEpoch;
}
+ /**
+ * A read is answerable only while what it assumed still holds, and the two
+ * kinds of read assume different things.
+ *
+ * An extension splices rows onto one edge, so it assumes that edge: any
+ * replacement of the window — navigating away, or the band trimming the edge
+ * out — leaves its answer unable to reach what is left, and no edge cursor
+ * can name the hole it would open. A replacement assumes nothing about the
+ * edges, because it discards them; only a newer navigation makes it stale.
+ *
+ * Batches that name no epoch answer nothing: tail broadcasts apply to
+ * whatever the window holds, and a snapshot replacing the transcript
+ * underneath every window is not this window's answer to refuse.
+ */
accepts(batch: DesktopTranscriptBatchPayload): boolean {
- // A stale reset must be rejected before it can clear the current range.
- if ((batch.navigationVersion ?? 0) !== this.#navigationVersion) return false;
if (this.#retiredGenerations.has(batch.generation)) return false;
- return batch.reset || (
- batch.sessionId === this.#sourceSessionId &&
+ if (batch.reset) {
+ return batch.windowEpoch === undefined || batch.windowEpoch === this.#navigationEpoch;
+ }
+ if (batch.windowEpoch !== undefined && batch.windowEpoch !== this.#windowEpoch) return false;
Review Comment:
Both paths fixed in 684c34c3f, with your two probes as regression tests.
1. `#reset` now takes the window's identity back to the epoch the navigation was issued under — a replacement discards the edges those trims were protecting — so its continuation fragments splice on. Minting moved to a counter kept apart from that identity, so a number still names one window forever and a page in flight under the trimmed epoch stays refusable.
2. Main only advances the consumer on a window-replacing command; an extension naming a newer epoch is admitted without moving it, and a read stays current while its epoch is at least the standing replacement's.
Both tests fail against the pre-fix build and pass after.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #5170:
URL: https://github.com/apache/maka/pull/5170
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3984975080
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -344,50 +329,84 @@ export class DesktopTranscriptRangeStore {
this.#expectedSessionId = sessionId;
}
- expectNavigation(navigationVersion: number): void {
- if (navigationVersion <= this.#navigationVersion) return;
- this.#navigationVersion = navigationVersion;
+ windowEpoch(): number {
+ return this.#windowEpoch;
+ }
+
+ navigationEpoch(): number {
+ return this.#navigationEpoch;
+ }
+
+ /**
+ * Mints the epoch for a window about to be replaced wholesale by a navigation,
+ * and drops the partially received records of the window being left behind.
+ */
+ replaceWindow(): number {
+ this.#navigationEpoch = this.#mintWindow();
+ return this.#navigationEpoch;
+ }
+
+ #mintWindow(): number {
+ this.#windowEpoch += 1;
this.#pending.clear();
this.#batchChanged = false;
+ return this.#windowEpoch;
}
+ /**
+ * A read is answerable only while what it assumed still holds, and the two
+ * kinds of read assume different things.
+ *
+ * An extension splices rows onto one edge, so it assumes that edge: any
+ * replacement of the window — navigating away, or the band trimming the edge
+ * out — leaves its answer unable to reach what is left, and no edge cursor
+ * can name the hole it would open. A replacement assumes nothing about the
+ * edges, because it discards them; only a newer navigation makes it stale.
+ *
+ * Batches that name no epoch answer nothing: tail broadcasts apply to
+ * whatever the window holds, and a snapshot replacing the transcript
+ * underneath every window is not this window's answer to refuse.
+ */
accepts(batch: DesktopTranscriptBatchPayload): boolean {
- // A stale reset must be rejected before it can clear the current range.
- if ((batch.navigationVersion ?? 0) !== this.#navigationVersion) return false;
if (this.#retiredGenerations.has(batch.generation)) return false;
- return batch.reset || (
- batch.sessionId === this.#sourceSessionId &&
+ if (batch.reset) {
+ return batch.windowEpoch === undefined || batch.windowEpoch === this.#navigationEpoch;
+ }
+ if (batch.windowEpoch !== undefined && batch.windowEpoch !== this.#windowEpoch) return false;
Review Comment:
**Keep a replacement's navigation lifetime intact through the entire response path.**
The new contract correctly says that trimming an edge must invalidate extensions, not an outstanding `loadAround` replacement. That distinction currently stops at the first reset batch:
1. Start navigation at epoch 1, then `retain()` trims the old window and advances `windowEpoch` to 2 while `navigationEpoch` remains 1. An epoch-1 replacement's reset passes line 373, but its continuation batches fail line 375. `#reset()` does not reconcile this. A 300 KiB replacement probe leaves `range().ready === false` after every batch has been delivered.
2. If background filling sends epoch 2 before the epoch-1 navigation returns, [Main advances its single consumer epoch](https://github.com/apache/maka/blob/327d96a89b43a8b77297eadecdefb3afe4f91c20/apps/desktop/src/main/runtime-host-session-observer.ts#L421-L432). The outstanding replacement then fails [the same equality check used for extensions](https://github.com/apache/maka/blob/327d96a89b43a8b77297eadecdefb3afe4f91c20/apps/desktop/src/main/runtime-host-session-observer.ts#L448-L461) and is silently discarded, even for a one-batch result. A probe using the actual observer starts with rows 1..3, delays `loadAround(0)`, retains row 3, then starts the background older read: after both promises resolve the window contains only rows 2 and 3, never the searched-for row 0.
This remains reachable while a search target is not yet mounted; the mounted-target trim guard does not cover that waiting period. The separate prefetch channel avoids clearing the search target explicitly, but cannot prevent Main from invalidating its read.
Please preserve the replacement/extension lifetime distinction consistently in admission, queued delivery, and every response fragment (or serialize background maintenance behind the pending replacement). Fixing only this reset branch would leave the Main path broken; re-admitting every old extension would restore the internal-gap bug. These are two manifestations of one incomplete read-lifetime contract, not a request for a more general window model.
<details><summary>中文</summary>
新契约允许导航跨过旧窗口裁剪，但目前仅首批 reset 遵守：
1. 导航 epoch=1，retain 后 windowEpoch=2、navigationEpoch=1；300 KiB 回复首批被接收，续批被拒绝，最终 ready=false。
2. 裁剪后的后台补页把 Main 的单一 consumer epoch 推到 2，在途 epoch=1 导航因此被静默取消。实际 observer 复现最终只有行 2、3，没有目标行 0。
未挂载的搜索目标不受 mounted-target guard 保护。需要让导航／扩页的不同生命周期贯穿接收、排队发送和所有分片，或让维护等待当前替换完成；不能只修 Renderer 首批，也不能重新放行所有旧扩页。这两个复现属于同一个未完成的契约。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(desktop): give the Renderer the transcript window [maka]
Posted by "Totoro-qaq (via GitHub)" <gi...@apache.org>.
Totoro-qaq commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5623741649
@Astro-Han Nice direction overall, and the extension/replacement split in aedc843 reads much clearer. I ran a few probes against it (the real observer, replica and Renderer store, with a fake Host), and three sequences still seem to leave the window somewhere the contiguous model can't recover from. I may be missing a guard, so please read these as questions.
A fill during a pending jump can splice the old edge onto the new window. replaceWindow() mints the epoch when loadAround is called, but range() still returns the old window until the reset lands, so an extend() in that gap pairs the old anchor with the new epoch. The replica's #enqueue then answers the jump first and the fill right after it. Probe: tail 18..20, jump to 5, fill before the reset lands, result [5, 6, 17] with newest 17, so 7..16 can't be paged in. This looks like the search ordering from the earlier review, now that the fill no longer cancels the jump.
A trim plus a fill during a pending jump makes Main drop the jump. retain() mints only a window epoch, but Main keeps a single consumer.windowEpoch, so the fill's higher epoch makes the in-flight loadAround fail isCurrent(). Probe: same setup, retain(19, 20) and then a fill. Both commands resolve without an error and the window stays at [18, 19, 20].
A Turn completing at the tail lands in a window parked far from it. loadAround's snapshot carries the live overlay, so a jump during a run shows [5, 6] plus overlay 21. When 21 completes it is inserted as durable, giving [5, 6, 21] with newest 21, and 7..20 are inside the range with no edge left to fetch them. The new settlement test checks Turn B's text but not its neighbours.
In case it's useful, a couple of directions I could imagine, though you'll know better what fits: hold extensions until the outstanding navigation's reset commits (or mint a window epoch when it does) and keep navigation and window epochs separate in Main; for a parked window, update the overlay in place and move it into the durable range only once the window reaches it.
Happy to share the probe files if that would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "orangeCatDeveloper (via GitHub)" <gi...@apache.org>.
orangeCatDeveloper commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5629495599
A suggestion on the overlay handling. I don't think this is a missing guard — a durable row shouldn't go on the axis at all while the window is parked.
`#acceptFragment` computes `installDurable = answersCommand || !this.#hasNewer` (L448), then at L662 admits the row anyway when `overlaid` is true. With the window parked far from the tail that installs it at its own sequence and drags `newestSequence` with it — Totoro-qaq's third probe, still red on 15894f8: `durable` ends up `[5, 6, 21]`, and 7–20 sit inside `[oldest, newest]` with no edge left to reach them.
The retirement rule is right; it's the *install* half that shouldn't run here. The body is still useful — it's the settled text of a message this window is showing, so it can refresh the overlay record in place without ever taking a sequence. Element does exactly this on remote echo (same object mutated; `pendingEventList` stays outside the timeline until `!canPaginate(FORWARDS)`), and Codex keeps the in-flight message in `active_cell` rather than `transcript_cells`.
```diff
const overlaid = this.#overlay.has(message.id);
if (!installDurable && !overlaid) return false;
const sequence = pending.identity as number;
+ if (!installDurable) {
+ // Settled text for a message this window is showing, but the window
+ // does not reach the row's place on the axis. Refresh the overlay in
+ // place; installing it would open a hole no edge cursor can name.
+ const settled = this.#overlay.get(message.id)!;
+ this.#overlay.set(message.id, { message, encoded: projected, order: settled.order });
+ this.#declinedSequence = Math.max(this.#declinedSequence ?? sequence, sequence);
+ return true;
+ }
const existing = this.#durable.get(sequence);
```
`#overlayOrder` doesn't move and `#createSnapshot` already appends overlay after durable, so the reader keeps the message with its final text. Once the window reaches the tail, `installDurable` is true and the existing install-and-retire path runs unchanged — still what `a window parked off the tail keeps the Turn it is reading when that Turn completes` asserts.
Two loose ends: `#declinedSequence` clears in `#reset()` (a replacement window inherits nothing), and a `loadAfter` answer shouldn't set `hasNewer = false` while it sits above `newestSequence`, or the edge closes over the row that was just declined — Zulip does the same with `_expected_max_message_id`. I'd avoid deriving `hasNewer` from `durableThrough` instead: the watermark can sit beyond the last visible row (the RuntimeEvent gap the replica comments mention), so it would stick true and spin.
Probe 3 from the thread works as the regression as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3987453668
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -48,93 +56,132 @@ export function createDesktopTranscriptRangeController(
let closed = false;
let openController = new AbortController();
let handle = open(openController.signal);
- type Navigation = DesktopTranscriptNavigation & {
- readonly kind: 'before' | 'after' | 'around' | 'latest' | 'anchor';
- readonly sequence: number | null;
- readonly maxBytes?: number;
- };
- let navigation: Navigation = {
- navigationVersion: 0, intent: 'followTail', kind: 'latest', sequence: null,
- };
+ /**
+ * The navigation the window currently sits on. Main uses it only to cancel
+ * work a newer navigation has made pointless, so an extension issued while a
+ * navigation is still in flight names that navigation: it is the one Main is
+ * already reading for.
+ */
+ let lastNavigation = 0;
+ const extending: {
+ older?: { anchor: number | null; task: Promise<void> };
+ newer?: { anchor: number | null; task: Promise<void> };
+ } = {};
+ /** The window a read was issued against; the same window answers the same way. */
+ const spent: { older?: object; newer?: object } = {};
const current = async () => {
if (closed) throw new Error('Desktop transcript range is closed');
return handle;
};
- const dispatch = async (command: Navigation) => {
+ const command = async (
+ replace: boolean,
+ run: (value: DesktopTranscriptHandle, navigation: DesktopTranscriptNavigation) => Promise<void>,
+ ) => {
+ // Mint before awaiting an open handle or any in-flight page.
+ const navigation = replace ? store.navigate() : store.pendingNavigation() ?? lastNavigation;
+ if (replace) lastNavigation = navigation;
const opening = handle;
- const isCurrent = () => !closed && navigation === command && opening === handle;
+ const isCurrent = () => !closed && opening === handle &&
+ (!replace || store.pendingNavigation() === navigation);
try {
const value = await current();
if (!isCurrent()) return;
- if (command.kind === 'before') {
- await value.loadBefore(command.sequence, command.maxBytes, command);
- } else if (command.kind === 'after') {
- await value.loadAfter(command.sequence, command.maxBytes, command);
- } else {
- await value.loadAround(command.sequence, command.maxBytes, command);
- }
+ await run(value, { navigation });
} catch (error) {
if (isCurrent()) throw error;
}
};
- const navigate = (command: Omit<Navigation, 'navigationVersion'>) => {
- navigation = { ...command, navigationVersion: navigation.navigationVersion + 1 };
- // Invalidate before awaiting an open handle or any previous page request.
- store.expectNavigation(navigation.navigationVersion);
- return dispatch(navigation);
+ /** The row a read at this edge would anchor on, or undefined with no edge to read. */
+ const edgeAt = (edge: 'older' | 'newer'): { anchor: number | null } | undefined => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return undefined;
+ }
+ if (edge === 'older' ? !range.hasOlder : !range.hasNewer) return undefined;
+ return { anchor: edge === 'older' ? range.oldestSequence : range.newestSequence };
+ };
+ const extend = async (edge: 'older' | 'newer', maxBytes: number): Promise<boolean> => {
+ const at = edgeAt(edge);
+ if (!at) return false;
+ // Sharing a read only holds while the edge it was anchored on does.
+ const pending = extending[edge];
+ if (pending && pending.anchor === at.anchor) {
+ await pending.task;
+ return false;
+ }
+ // A read issued against this exact window answers it the same way again.
+ // Every commit mints a fresh snapshot object, so a window that moved at all
+ // — by a page, a trim, a navigation or tail growth — is worth asking again.
+ const window = store.snapshot();
+ if (spent[edge] === window) return false;
+ spent[edge] = window;
+ const task = command(false, (value, navigation) =>
+ edge === 'older'
+ ? value.loadBefore(at.anchor, maxBytes, navigation)
+ : value.loadAfter(at.anchor, maxBytes, navigation),
+ ).finally(() => {
+ if (extending[edge]?.task === task) extending[edge] = undefined;
+ });
+ extending[edge] = { anchor: at.anchor, task };
+ await task;
+ return true;
+ };
+ /**
+ * Main marks the Session read from these and from nothing else, so the window
+ * reports every watermark it actually reaches, once. A window with newer
+ * history beyond it reports nothing: the reader is parked off the tail.
+ */
+ let acknowledged: number | undefined;
+ const acknowledgeTail = () => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return;
+ }
+ // A cached window's watermark is a fact about the local cache, not about
+ // the live replica an acknowledgement moves.
+ if (
+ !range.ready || range.hasNewer || range.durableThrough === null ||
+ range.generation.startsWith('cached:')
+ ) return;
+ const through = range.durableThrough;
+ if (acknowledged === through) return;
Review Comment:
Retracting this finding after tracing the persistence boundary. `hostEpoch` changes on Host process replacement, but the same Session ledger, durable sequences, and `hasUnread` / `lastReadMessageId` survive that replacement. Therefore suppressing a duplicate acknowledgement for the same tail N does not leave a newly unread Turn unread; the scenario I described would require the same Session ID to be replaced in place by a different ledger that reuses N, and I found no reachable path that does that. This is not a valuable blocker. 抱歉，这条意见的“可达且有用户影响”前提不成立，撤回。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "orangeCatDeveloper (via GitHub)" <gi...@apache.org>.
orangeCatDeveloper commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5632081588
Not a correctness review — the three blocking findings above are the ones that matter, and I have nothing to add to them. This is only a pass for dead weight, checked against `b239a9b39`. The PR nets about -985 lines of code and **+177 lines of comment**, and nearly every comment item below is a line this PR adds.
### 1. An orphaned doc comment that is now wrong
`apps/desktop/src/main/__tests__/desktop-transcript-range-store.test.ts:1281`
```ts
/** The one-byte read `loadAround` uses to ask whether `sequence` has anything
* older than it: only the presence of a fragment answers, not its content. */
function syntheticLargeTranscript(): Array<{ identity: number; message: StoredMessage }> {
```
Pre-PR this sat above `olderProbePage` (`fa0ff028e:1690`). That function is gone and the comment re-attached itself to a helper that builds a synthetic transcript and has nothing to do with probe reads. Of everything here this is the only item that actively misinforms a reader.
### 2. `DesktopTranscriptNavigation` is a wrapper around a single number
`apps/desktop/src/preload/transcript-contract.ts:27`
```ts
export interface DesktopTranscriptNavigation {
readonly navigation: number;
}
```
It used to carry four fields. Now every producer wraps and every consumer unwraps — `preload.ts:2587` is literally `navigation: navigation.navigation,`. The same file contradicts it at `:52` and `:83`, where `navigation` is declared bare as `number`. Replacing it with `number` in the four `DesktopTranscriptHandle` signatures (`:105-108`) drops the interface, two imports, and the wrapping at each call site.
### 3. `transcript-parked-completion-probe.test.ts` — new file, and its one assertion is already proven
Its only assertion is `durableEntries() == [5, 6]`: a non-contiguous durable upsert is dropped. `desktop-transcript-range-store.test.ts:1098-1118` already proves that over both `coversFrom` shapes. The one new ingredient here is the live overlay row (`21`) in the `loadAround` snapshot — and the overlay outcome is never asserted. Either drop the file or add the overlay assertion that would justify it.
### 4. `ChatMessageSurface` re-declares and re-passes four inherited props
`apps/desktop/src/renderer/chat-message-surface.tsx:92-95`, destructured at `:128-131`, re-passed at `:248-251`.
`ChatMessageSurfaceProps extends Omit<ComponentProps<typeof ChatView>, …>` and none of `hasOlderHistory` / `hasNewerHistory` / `onPrefetchHistory` / `onRetainWindow` is in the `Omit` list, so all four already arrive in `...chatViewRest`, which `:237` spreads onto `<ChatView>`. Before this PR the surface did real adapter work here; what survived the refactor is plain forwarding.
One caveat: `:94-95` declare the two callbacks required where `ChatView` has them optional. AppShell passes both, so removing them changes no behavior, but it does relax the type.
### 5. Two doc comments stacked on one property
`packages/ui/src/use-chat-scroll.ts:59-64` — both blocks are `+` lines in this diff, so this looks like a merge leftover.
```ts
/**
* Fills the window because the reader is running out of it — never because
* they asked to go somewhere. It must not consume an outstanding navigation
* intent, and it must reject when the read fails so this hook can tell a
* filled window from a failed one.
*/
/** Fills an edge the reader is approaching. Safe to call on every frame that wants it. */
onPrefetchHistory?(edge: 'older' | 'newer'): Promise<boolean>;
```
### Nits
- `apps/desktop/e2e/partial-history-notice.spec.ts` asserts against `.maka-transcript-gap-row`, `.maka-transcript-history-controls` and `[data-transcript-gap]`. No product code emits any of the three — `git grep` outside tests/e2e/stories finds nothing.
- `transcript-navigation-race.test.ts` and `transcript-pending-jump-probe.test.ts` each hand-roll `deferred<T>()`, while `@maka/core/test-only/async-primitives` exports it and a dozen tests in the same directory import it. (A few older files do the same, so this is a pre-existing habit, not something this PR started.)
- `runtime-host-session-observation-registry.ts:612` — `#bindTranscriptTarget` is `return this.#bindTarget(target);` and `#bindTarget` is already generic, so the wrapper adds no typing for its two call sites.
- `transcript-scroll-cost.spec.ts` — `peakMounted` is threaded through 10 sites to feed one `expect(peakMounted).toBeGreaterThan(0)`.
- One fact stated twice, in prose that has to be kept in sync: "sequence numbers advance by a stride, so adjacency cannot be read off them" at `transcript-contract.ts:42` and `desktop-transcript-replica.ts:79`. "Fills the window at an edge the reader is approaching" appears three times: `chat-view.tsx:273`, `use-chat-scroll.ts:64`, `transcript-reading-position-controller.tsx:127`. The contract is the one place worth stating each in.
- Several new comments pin numbers that nothing re-verifies: `use-chat-scroll.ts:181-185` restates `screen * 2 / 6 / 4` from 35 lines below; `:196-200` records a measurement ("an insert of 501px moves `scrollTop` by 501 at an offset of 1"); `transcript-scroll-cost.spec.ts:44-66` duplicates the 700px viewport and an 18px row height from the test bodies, and `:331-340` notes what a loaded CI runner measured.
- Typo: `packages/ui/src/__tests__/use-chat-scroll.test.tsx:440` — "and and an unmounted bookmark".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3987754561
##########
apps/desktop/src/renderer/features/conversation/controller/transcript-reading-position-controller.tsx:
##########
@@ -180,10 +178,48 @@ export function TranscriptReadingPositionController(props: {
: undefined,
controller: props.rangeController.current,
isCurrent,
- isLiveTurn: (sessionId, turnId) => props.sessionUi.liveTurnBySessionRef.current[sessionId]?.turnId === turnId,
setReadingAnchor: props.sessionUi.setTranscriptReadingAnchor,
onRestoreUnavailable: props.sessionUi.setTranscriptRestoreUnavailable,
onError: props.onRestoreError,
}), [props.sessionId, props.profileId, props.messages, props.searchTarget?.nonce]);
+ useEffect(() => {
+ const controller = props.rangeController.current;
+ const { sessionId } = props;
+ const range = currentTranscriptRange(controller, sessionId);
+ if (!controller || !sessionId || !range?.generation || !range.hostEpoch) return;
+ if (range.generation.startsWith('cached:')) return;
+ const previous = lastLiveGeneration.current;
+ lastLiveGeneration.current = { sessionId, generation: range.generation, hostEpoch: range.hostEpoch };
+ if (!previous || previous.sessionId !== sessionId || previous.generation === range.generation) return;
+ const anchor = props.sessionUi.transcriptReadingAnchorBySessionRef.current[sessionId];
+ if (!anchor || controller.store.sequenceForTurn(anchor.turnId) !== null) return;
+ const navigate = (sequence: number) => {
+ void controller.loadAround(sequence).catch((error) => {
+ reportNavigationError(error, sessionId, controller);
+ });
+ };
+ if (previous.hostEpoch === range.hostEpoch) {
+ if (anchor.sequence !== undefined) navigate(anchor.sequence);
+ return;
+ }
+ // Sequences only name the same rows within one Host epoch, so a bookmark
+ // carried across one has to be found again by Turn. The landmark index in
+ // hand still names the epoch that is gone, hence the refresh first.
+ if (landmarkSessionId !== sessionId) return;
+ const { turnId } = anchor;
+ let disposed = false;
+ void props.listTurnLandmarks(sessionId).then((snapshot) => {
+ if (disposed || !isCurrent(sessionId, controller)) return;
+ props.setTurnIndex({ sessionId, throughSequence: snapshot.throughSequence, turns: snapshot.landmarks });
+ // A reader who has gone somewhere else since owns the position now.
+ if (props.sessionUi.transcriptReadingAnchorBySessionRef.current[sessionId]?.turnId !== turnId) return;
+ const landmark = snapshot.landmarks.find((turn) => turn.turnId === turnId);
+ // A Turn the new epoch does not name leaves the reader where the reset put them.
+ if (!landmark) return;
+ props.sessionUi.setTranscriptReadingAnchor(sessionId, { turnId, sequence: landmark.sequence });
+ navigate(landmark.sequence);
+ }, () => undefined);
Review Comment:
**Blocking — keep cross-epoch re-anchoring retryable when the landmark read fails.**
This rejection is reachable on the exact Host-recovery path this commit fixes, but it is silently consumed after line 192 has already advanced `lastLiveGeneration` to the replacement generation. Any later render for the same generation returns at line 193, so neither a subsequent message update nor the separate landmark-index refresh can retry the bookmark lookup. A single transient `sessions:listTurnLandmarks` failure therefore leaves a reader who was parked in history at the reset tail for the rest of that Host generation, with no error and no unavailable indication.
Keep a pending re-anchor keyed by Session/generation until the lookup succeeds or definitively reports that the Turn is absent, and retry it from the refreshed index or a later effect pass. A focused regression can reject the first landmark read, then make it succeed and rerender/update the index; the controller should eventually call `loadAround` for the bookmarked Turn rather than treating the epoch as handled.
<details><summary>中文</summary>这次查询失败发生在本提交要修复的 Host 恢复路径上，但失败被静默吞掉前，`lastLiveGeneration` 已经推进到新 generation。之后同一 generation 的任何 render 都会提前返回，独立的 landmark index 刷新也不会再触发定位。因此一次瞬时 RPC 失败会让历史区读者在整个 Host generation 内永久停在 tail，既不重试也不报错。应保留按 Session/generation 标识的 pending re-anchor，直到查询成功或明确确认 Turn 不存在。</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3985267911
##########
apps/desktop/src/main/runtime-host-session-observer.ts:
##########
@@ -337,90 +354,157 @@ export class RuntimeHostSessionObserver {
request: DesktopTranscriptRangeRequest,
targetId?: number,
): Promise<void> {
- await this.#runTranscriptRangeOperation(request, targetId, (replica, token) =>
- replica.loadBefore(
+ await this.#runTranscriptRangeOperation(request, targetId, false, async (replica, isCurrent) => {
+ const page = await replica.loadBefore(
request.anchorSequence,
requireTranscriptRangeBytes(request.maxBytes),
- token,
- ),
- );
+ isCurrent,
+ );
+ return page && { batches: encodeDesktopTranscriptPage(this.#pageIdentity(replica, request), page), bytes: page.durable };
+ });
}
- async loadTranscriptAround(
+ async loadTranscriptAfter(
request: DesktopTranscriptRangeRequest,
targetId?: number,
): Promise<void> {
- await this.#runTranscriptRangeOperation(request, targetId, (replica, token) => {
- if (request.intent === 'followTail') {
- return replica.followLatest(requireTranscriptRangeBytes(request.maxBytes), token);
- }
- if (request.readingTurnId !== undefined) {
- return replica.readAt(request.anchorSequence, token, request.readingTurnId);
- }
- if (request.anchorSequence === null) {
- throw new Error('Desktop transcript around request requires an anchor');
- }
- if (request.preserveRange) return replica.readAt(request.anchorSequence, token);
- return replica.loadAround(
+ await this.#runTranscriptRangeOperation(request, targetId, false, async (replica, isCurrent) => {
+ const page = await replica.loadAfter(
request.anchorSequence,
requireTranscriptRangeBytes(request.maxBytes),
- token,
+ isCurrent,
);
+ return page && { batches: encodeDesktopTranscriptPage(this.#pageIdentity(replica, request), page), bytes: page.durable };
});
}
- async loadTranscriptAfter(
+ async loadTranscriptAround(
request: DesktopTranscriptRangeRequest,
targetId?: number,
): Promise<void> {
- await this.#runTranscriptRangeOperation(request, targetId, (replica, token) =>
- replica.loadAfter(
- request.anchorSequence,
+ if (request.anchorSequence === null) {
+ throw new Error('Desktop transcript around request requires an anchor');
+ }
+ const sequence = request.anchorSequence;
+ await this.#runTranscriptRangeOperation(request, targetId, true, async (replica, isCurrent) => {
+ const snapshot = await replica.loadAround(
+ sequence,
requireTranscriptRangeBytes(request.maxBytes),
- token,
- ),
- );
+ isCurrent,
+ );
+ return snapshot && {
+ batches: encodeDesktopTranscriptSnapshot({ ...snapshot, windowEpoch: request.windowEpoch }),
+ bytes: [...snapshot.durable, ...snapshot.overlay.map((message) => ({ message }))],
+ };
+ });
}
- async #runTranscriptRangeOperation(
+ async loadTranscriptLatest(
request: DesktopTranscriptRangeRequest,
- targetId: number | undefined,
- operation: (replica: DesktopTranscriptReplica, token: number) => Promise<void>,
+ targetId?: number,
): Promise<void> {
+ const { state, consumer } = this.#admitTranscriptNavigation(request, targetId, true);
+ if (!consumer) return;
+ consumer.resetRequested = true;
+ consumer.resetWindowEpoch = request.windowEpoch;
+ await this.#scheduleTranscriptDelivery(state, consumer);
+ this.#touchReplica(state);
+ }
+
+ #pageIdentity(replica: DesktopTranscriptReplica, request: DesktopTranscriptRangeRequest) {
+ return {
+ sessionId: replica.sessionId,
+ generation: replica.generation,
+ hostEpoch: replica.hostEpoch,
+ windowEpoch: request.windowEpoch,
+ };
+ }
+
+ /**
+ * The Renderer mints a version for every window it can no longer splice onto
+ * — navigating away, and the band trimming an edge out. Only the first of
+ * those abandons a read already in flight: a replacement discards the edges,
+ * so a trim under it leaves it perfectly answerable. So the consumer's
+ * version is what the last replacement named, an extension that names a newer
+ * one is admitted without moving it, and only a version older than the
+ * standing replacement belongs to a window the Renderer has left.
+ */
+ #admitTranscriptNavigation(
+ request: DesktopTranscriptRangeRequest,
+ targetId: number | undefined,
+ replaces: boolean,
+ ): { state: ObservedSessionState; replica: DesktopTranscriptReplica; consumer?: TranscriptConsumer } {
const { state, replica, consumer } = this.#requireTranscriptConsumer(request, targetId);
- const version = request.navigationVersion ?? consumer.navigationVersion;
+ const version = request.windowEpoch;
if (!Number.isSafeInteger(version) || version < 0) throw new Error('Invalid transcript navigation version');
- if (version < consumer.navigationVersion) return;
- if (request.intent !== undefined && request.intent !== 'history' && request.intent !== 'followTail') {
- throw new Error('Invalid transcript navigation intent');
+ if (version < consumer.windowEpoch) return { state, replica };
+ if (replaces) consumer.windowEpoch = version;
+ if (version > consumer.newestWindowEpoch) consumer.newestWindowEpoch = version;
+ this.#dropSupersededTranscriptPages(consumer);
+ return { state, replica, consumer };
+ }
+
+ /**
+ * Releases the delivery budget held by queued answers the window can no longer
+ * take. An extension is spliceable only onto the window it named, so any newer
+ * one strands it; a replacement installs its own window, so only a newer
+ * replacement does.
+ */
+ #dropSupersededTranscriptPages(consumer: TranscriptConsumer): void {
+ for (let index = consumer.pendingPages.length - 1; index >= 0; index -= 1) {
+ const page = consumer.pendingPages[index]!;
+ if (this.#deliversTranscriptPage(consumer, page)) continue;
+ consumer.pendingPages.splice(index, 1);
+ this.#adjustTranscriptDeliveryBytes(consumer, -page.encodedBytes);
}
- consumer.navigationVersion = version;
- consumer.navigationRequest = request;
- consumer.navigationPending = true;
- consumer.resetRequested = true;
- this.#clearPendingTranscriptChange(consumer);
- // Admission invalidates in-flight pages immediately, before the replica's
- // operation queue can run the newer command.
- const token = replica.setNavigation(request.intent ?? 'history');
+ }
+
+ #deliversTranscriptPage(consumer: TranscriptConsumer, page: PendingTranscriptPage): boolean {
+ return page.windowEpoch >= (page.replaces ? consumer.windowEpoch : consumer.newestWindowEpoch);
Review Comment:
**Do not use the highest observed epoch as the active edge after a replacement installs an older epoch.**
This now drops valid reads made *after* the navigation has successfully completed:
1. `loadAround` starts at epoch 1. A trim and background fill name epoch 2, so Main records `newestWindowEpoch = 2`.
2. The epoch-1 replacement completes. The new Renderer `#reset()` intentionally restores `store.windowEpoch()` to **1**, and its full snapshot is now received correctly.
3. A fresh `controller.loadAfter()` therefore legitimately names epoch **1**. Main admits and reads it, but this predicate compares its answer against the still-**2** high-water mark and discards it. Nothing ever lowers `newestWindowEpoch` when the replacement installs its window.
I extended the supported navigation → trim → background fill sequence with one next-page request using the actual controller/observer/store. The 300 KiB replacement reaches `ready=true` with message 0, but `loadAfter()` resolves leaving only **[message-0]**, instead of **[message-0, message-1]**; `hasNewer` remains true. Repeating the same read is also refused until another epoch-changing action occurs.
Delivery eligibility needs to reflect the window installed by the successful replacement, not simply the maximum epoch ever observed. Keep rejecting/releasing answers anchored on the abandoned trim window, while allowing fresh reads of the newly installed window. The existing new regression stops after asserting that navigation delivered; extending it through the next page would catch this.
<details><summary>中文</summary>
导航 epoch=1，期间裁剪／补页使 Main 的 newestWindowEpoch=2。导航成功后 Renderer 按新逻辑恢复当前窗口 epoch=1，但 Main 的高水位仍是 2。此后新发出的合法 loadAfter(epoch=1) 虽被读取，却在这里丢弃。
实际串联复现中，300 KiB 导航回复完整到达；随后加载下一页仍只有 [message-0]，缺少 message-1，hasNewer 还为 true。应根据成功替换后真正安装的窗口判断交付资格，同时继续释放旧裁剪窗口的过期答案，不能把历史最大 epoch 等同于当前边缘。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3987789760
##########
apps/desktop/src/renderer/features/conversation/controller/transcript-reading-position-controller.tsx:
##########
@@ -180,10 +178,48 @@ export function TranscriptReadingPositionController(props: {
: undefined,
controller: props.rangeController.current,
isCurrent,
- isLiveTurn: (sessionId, turnId) => props.sessionUi.liveTurnBySessionRef.current[sessionId]?.turnId === turnId,
setReadingAnchor: props.sessionUi.setTranscriptReadingAnchor,
onRestoreUnavailable: props.sessionUi.setTranscriptRestoreUnavailable,
onError: props.onRestoreError,
}), [props.sessionId, props.profileId, props.messages, props.searchTarget?.nonce]);
+ useEffect(() => {
+ const controller = props.rangeController.current;
+ const { sessionId } = props;
+ const range = currentTranscriptRange(controller, sessionId);
+ if (!controller || !sessionId || !range?.generation || !range.hostEpoch) return;
+ if (range.generation.startsWith('cached:')) return;
+ const previous = lastLiveGeneration.current;
+ lastLiveGeneration.current = { sessionId, generation: range.generation, hostEpoch: range.hostEpoch };
+ if (!previous || previous.sessionId !== sessionId || previous.generation === range.generation) return;
+ const anchor = props.sessionUi.transcriptReadingAnchorBySessionRef.current[sessionId];
+ if (!anchor || controller.store.sequenceForTurn(anchor.turnId) !== null) return;
+ const navigate = (sequence: number) => {
+ void controller.loadAround(sequence).catch((error) => {
+ reportNavigationError(error, sessionId, controller);
+ });
+ };
+ if (previous.hostEpoch === range.hostEpoch) {
+ if (anchor.sequence !== undefined) navigate(anchor.sequence);
+ return;
+ }
+ // Sequences only name the same rows within one Host epoch, so a bookmark
+ // carried across one has to be found again by Turn. The landmark index in
+ // hand still names the epoch that is gone, hence the refresh first.
+ if (landmarkSessionId !== sessionId) return;
+ const { turnId } = anchor;
+ let disposed = false;
+ void props.listTurnLandmarks(sessionId).then((snapshot) => {
+ if (disposed || !isCurrent(sessionId, controller)) return;
+ props.setTurnIndex({ sessionId, throughSequence: snapshot.throughSequence, turns: snapshot.landmarks });
+ // A reader who has gone somewhere else since owns the position now.
+ if (props.sessionUi.transcriptReadingAnchorBySessionRef.current[sessionId]?.turnId !== turnId) return;
+ const landmark = snapshot.landmarks.find((turn) => turn.turnId === turnId);
+ // A Turn the new epoch does not name leaves the reader where the reset put them.
+ if (!landmark) return;
+ props.sessionUi.setTranscriptReadingAnchor(sessionId, { turnId, sequence: landmark.sequence });
+ navigate(landmark.sequence);
+ }, () => undefined);
Review Comment:
Retracting this finding after tracing the production IPC policy. The code would indeed abandon the re-anchor if an arbitrary landmark Promise rejected, but the recovery-time transient failures I cited (`connection_lost` and Host draining) do not reach this branch: `sessions:listTurnLandmarks` is registered through `handleReconnectableRead`, whose router waits for the replacement handler and retries the query. I did not find a concrete, recoverable production failure left over that makes this a valuable blocker; an injected generic rejection alone is insufficient evidence. 撤回：恢复期可重试错误已在 Main 层处理，这条评论没有建立真实可达且有独立用户影响的剩余场景。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3985312019
##########
apps/desktop/src/main/runtime-host-session-observer.ts:
##########
@@ -337,90 +354,157 @@ export class RuntimeHostSessionObserver {
request: DesktopTranscriptRangeRequest,
targetId?: number,
): Promise<void> {
- await this.#runTranscriptRangeOperation(request, targetId, (replica, token) =>
- replica.loadBefore(
+ await this.#runTranscriptRangeOperation(request, targetId, false, async (replica, isCurrent) => {
+ const page = await replica.loadBefore(
request.anchorSequence,
requireTranscriptRangeBytes(request.maxBytes),
- token,
- ),
- );
+ isCurrent,
+ );
+ return page && { batches: encodeDesktopTranscriptPage(this.#pageIdentity(replica, request), page), bytes: page.durable };
+ });
}
- async loadTranscriptAround(
+ async loadTranscriptAfter(
request: DesktopTranscriptRangeRequest,
targetId?: number,
): Promise<void> {
- await this.#runTranscriptRangeOperation(request, targetId, (replica, token) => {
- if (request.intent === 'followTail') {
- return replica.followLatest(requireTranscriptRangeBytes(request.maxBytes), token);
- }
- if (request.readingTurnId !== undefined) {
- return replica.readAt(request.anchorSequence, token, request.readingTurnId);
- }
- if (request.anchorSequence === null) {
- throw new Error('Desktop transcript around request requires an anchor');
- }
- if (request.preserveRange) return replica.readAt(request.anchorSequence, token);
- return replica.loadAround(
+ await this.#runTranscriptRangeOperation(request, targetId, false, async (replica, isCurrent) => {
+ const page = await replica.loadAfter(
request.anchorSequence,
requireTranscriptRangeBytes(request.maxBytes),
- token,
+ isCurrent,
);
+ return page && { batches: encodeDesktopTranscriptPage(this.#pageIdentity(replica, request), page), bytes: page.durable };
});
}
- async loadTranscriptAfter(
+ async loadTranscriptAround(
request: DesktopTranscriptRangeRequest,
targetId?: number,
): Promise<void> {
- await this.#runTranscriptRangeOperation(request, targetId, (replica, token) =>
- replica.loadAfter(
- request.anchorSequence,
+ if (request.anchorSequence === null) {
+ throw new Error('Desktop transcript around request requires an anchor');
+ }
+ const sequence = request.anchorSequence;
+ await this.#runTranscriptRangeOperation(request, targetId, true, async (replica, isCurrent) => {
+ const snapshot = await replica.loadAround(
+ sequence,
requireTranscriptRangeBytes(request.maxBytes),
- token,
- ),
- );
+ isCurrent,
+ );
+ return snapshot && {
+ batches: encodeDesktopTranscriptSnapshot({ ...snapshot, windowEpoch: request.windowEpoch }),
+ bytes: [...snapshot.durable, ...snapshot.overlay.map((message) => ({ message }))],
+ };
+ });
}
- async #runTranscriptRangeOperation(
+ async loadTranscriptLatest(
request: DesktopTranscriptRangeRequest,
- targetId: number | undefined,
- operation: (replica: DesktopTranscriptReplica, token: number) => Promise<void>,
+ targetId?: number,
): Promise<void> {
+ const { state, consumer } = this.#admitTranscriptNavigation(request, targetId, true);
+ if (!consumer) return;
+ consumer.resetRequested = true;
+ consumer.resetWindowEpoch = request.windowEpoch;
+ await this.#scheduleTranscriptDelivery(state, consumer);
+ this.#touchReplica(state);
+ }
+
+ #pageIdentity(replica: DesktopTranscriptReplica, request: DesktopTranscriptRangeRequest) {
+ return {
+ sessionId: replica.sessionId,
+ generation: replica.generation,
+ hostEpoch: replica.hostEpoch,
+ windowEpoch: request.windowEpoch,
+ };
+ }
+
+ /**
+ * The Renderer mints a version for every window it can no longer splice onto
+ * — navigating away, and the band trimming an edge out. Only the first of
+ * those abandons a read already in flight: a replacement discards the edges,
+ * so a trim under it leaves it perfectly answerable. So the consumer's
+ * version is what the last replacement named, an extension that names a newer
+ * one is admitted without moving it, and only a version older than the
+ * standing replacement belongs to a window the Renderer has left.
+ */
+ #admitTranscriptNavigation(
+ request: DesktopTranscriptRangeRequest,
+ targetId: number | undefined,
+ replaces: boolean,
+ ): { state: ObservedSessionState; replica: DesktopTranscriptReplica; consumer?: TranscriptConsumer } {
const { state, replica, consumer } = this.#requireTranscriptConsumer(request, targetId);
- const version = request.navigationVersion ?? consumer.navigationVersion;
+ const version = request.windowEpoch;
if (!Number.isSafeInteger(version) || version < 0) throw new Error('Invalid transcript navigation version');
- if (version < consumer.navigationVersion) return;
- if (request.intent !== undefined && request.intent !== 'history' && request.intent !== 'followTail') {
- throw new Error('Invalid transcript navigation intent');
+ if (version < consumer.windowEpoch) return { state, replica };
+ if (replaces) consumer.windowEpoch = version;
+ if (version > consumer.newestWindowEpoch) consumer.newestWindowEpoch = version;
+ this.#dropSupersededTranscriptPages(consumer);
+ return { state, replica, consumer };
+ }
+
+ /**
+ * Releases the delivery budget held by queued answers the window can no longer
+ * take. An extension is spliceable only onto the window it named, so any newer
+ * one strands it; a replacement installs its own window, so only a newer
+ * replacement does.
+ */
+ #dropSupersededTranscriptPages(consumer: TranscriptConsumer): void {
+ for (let index = consumer.pendingPages.length - 1; index >= 0; index -= 1) {
+ const page = consumer.pendingPages[index]!;
+ if (this.#deliversTranscriptPage(consumer, page)) continue;
+ consumer.pendingPages.splice(index, 1);
+ this.#adjustTranscriptDeliveryBytes(consumer, -page.encodedBytes);
}
- consumer.navigationVersion = version;
- consumer.navigationRequest = request;
- consumer.navigationPending = true;
- consumer.resetRequested = true;
- this.#clearPendingTranscriptChange(consumer);
- // Admission invalidates in-flight pages immediately, before the replica's
- // operation queue can run the newer command.
- const token = replica.setNavigation(request.intent ?? 'history');
+ }
+
+ #deliversTranscriptPage(consumer: TranscriptConsumer, page: PendingTranscriptPage): boolean {
+ return page.windowEpoch >= (page.replaces ? consumer.windowEpoch : consumer.newestWindowEpoch);
Review Comment:
Correct, and fixed in c99ac2c73 — by removing the predicate rather than repairing it. `newestWindowEpoch` was Main trying to derive which window the Renderer currently holds, and that is not derivable from the epochs Main sees: the Renderer's identity was not monotone, so a monotone high-water mark had to be wrong somewhere. Main is back to one number, the standing replacement, which is the only thing that abandons a read; the Renderer refuses what it cannot splice, which it is the sole authority on.
The rollback that made it wrong is gone too. The window now has two states — settled, and navigating between a replacement being asked for and fully installed — and the identity only ever moves forward. A replacement's fragments are admitted because they name the navigation, not because the identity went back to meet them; installing one names the window afresh, which refuses every read anchored on an edge it discarded. Your sequence now ends with the fill admitted and `[message-0, message-1]`.
The same review turned up a second hole on the way in: `replaceWindow()` claims the window synchronously, but until the reset lands `range()` still describes the window being left, so background filling anchored on the old edge and spliced onto the new one. Filling is now refused while navigating. Both paths have regression tests.
On the 120px E2E failure — measurement, and fixed in 693dc10c8. 120 is exactly one wheel tick, on every boundary, on a runner where this machine reports none. The probe inferred "the reader's hand is off the wheel" from a 250ms quiet window, but the gesture and the rAF that reads it land in the same frame in an unspecified order. The test now tells the probe instead of the probe guessing.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(desktop): give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3981975896
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -351,43 +332,55 @@ export class DesktopTranscriptRangeStore {
this.#batchChanged = false;
}
+ /**
+ * A batch answering a command is current only under the version that issued
+ * it. A reset for a new replica generation is always current: Main replaced
+ * the transcript underneath every window. Broadcasts carry no version.
+ */
accepts(batch: DesktopTranscriptBatchPayload): boolean {
- // A stale reset must be rejected before it can clear the current range.
- if ((batch.navigationVersion ?? 0) !== this.#navigationVersion) return false;
if (this.#retiredGenerations.has(batch.generation)) return false;
- return batch.reset || (
- batch.sessionId === this.#sourceSessionId &&
+ if (batch.reset) {
+ return batch.navigationVersion === undefined ||
+ batch.navigationVersion === this.#navigationVersion ||
+ batch.generation !== this.#liveGeneration;
Review Comment:
Fixed in aedc843, and thank you — this one pointed at the wrong abstraction rather than at a missing branch.
The version was a *command* version, so it could only invalidate command answers. A replacement snapshot answers nothing this window asked for, and defaulting its absent version to 0 made it look like an answer to a navigation that never happened. The two kinds of read are now separated by what they assume: a replacement discards the edges, so only a newer navigation makes it stale; an extension splices onto one edge, so any replacement of that edge refuses it.
```ts
accepts(batch) {
if (this.#retiredGenerations.has(batch.generation)) return false;
if (batch.reset) {
return batch.windowEpoch === undefined || batch.windowEpoch === this.#navigationEpoch;
}
if (batch.windowEpoch !== undefined && batch.windowEpoch !== this.#windowEpoch) return false;
...
}
```
Regression: `a replica replacement is admitted whole, however far the window has navigated` in `transcript-navigation-race.test.ts` — it navigates the window past epoch 0, feeds an encoded new-generation snapshot carrying a 300 KiB message and no epoch, and asserts `range().ready === true` with both rows installed. On the old predicate it fails exactly as you described: the continuation batches are refused and the range never becomes ready.
##########
packages/ui/src/use-chat-scroll.ts:
##########
@@ -171,55 +175,83 @@ export function useChatScroll(input: {
};
}, [authority, input.scrollRef, input.sessionId]);
+ // The window is a band of pixels around the reader: fetch when less than two
+ // screens remain in a direction that has history, drop what lies more than
+ // six screens away and keep four. Both are re-evaluated whenever content or
+ // the reader moves, so the tail prefetches its history and a reader who
+ // stops mid-transcript never pins more than a bounded slice in memory.
+ const bandCheck = useRef<(() => void) | undefined>(undefined);
useEffect(() => {
const root = input.scrollRef.current;
if (!root) return;
+ const inFlight = { up: false, down: false };
const canLoad = (direction: 'up' | 'down'): boolean => direction === 'up'
? input.hasOlderHistory === true && canLoadEarlier
: input.hasNewerHistory === true && canLoadLater;
- // Asking twice is the loader's problem, not this one's: it refuses a
- // request while one is in flight, and asking for history the reader
- // already has is idempotent anyway.
const requestHistory = (direction: 'up' | 'down'): void => {
- activation.current = { sessionId: input.sessionId };
- commandTarget.current = null;
- // Moving input already released following. Only an immovable edge can
- // still be pinned; do not cancel the input operation that requested data.
- if (authority.getSnapshot().pinned) authority.releasePin();
- // A wheel at either edge moves nothing, so no scroll event refreshes the
- // anchor and the restore effect would load around an evicted Turn.
- reportReadingAnchor.current?.();
- const anchorTurnId = direction === 'up'
- ? firstVisibleTurnId(root)
- : lastVisibleTurnId(root);
+ if (inFlight[direction]) return;
// The browser anchors the reader against everything that lands above
- // them, with one exception: it declines while the scroller sits at zero,
- // which is exactly where a wheel asks for history. One pixel is the whole
- // fix — measured in Chromium, an insert of 501px above the reader moves
- // `scrollTop` by 501 at an offset of 1 and by 0 at an offset of 0.
- if (direction === 'up' && root.scrollTop < 1) root.scrollTop = 1;
+ // them, with one exception: it declines while the scroller sits at zero.
+ // One pixel is the whole fix — measured in Chromium, an insert of 501px
+ // above the reader moves `scrollTop` by 501 at an offset of 1 and by 0
+ // at an offset of 0.
+ if (direction === 'up' && !authority.getSnapshot().pinned && root.scrollTop < 1) {
+ root.scrollTop = 1;
+ }
const load = direction === 'up' ? loadEarlierRef.current : loadLaterRef.current;
- void Promise.resolve(load?.(anchorTurnId)).catch(() => undefined);
+ inFlight[direction] = true;
+ void Promise.resolve(load?.()).catch(() => undefined).finally(() => {
+ inFlight[direction] = false;
+ check();
+ });
+ };
+ const check = (): void => {
+ if (!root.isConnected || bandCheck.current !== check) return;
+ const screen = Math.max(320, root.clientHeight);
+ const above = root.scrollTop;
+ const below = root.scrollHeight - root.clientHeight - root.scrollTop;
+ if (canLoad('up') && above < screen * 2) requestHistory('up');
+ if (canLoad('down') && below < screen * 2) requestHistory('down');
Review Comment:
Fixed in aedc843. Filling an edge and navigating to one are different intents, and the fill had borrowed the reader's channel; `loadHistory`'s `cancel()` is right for a reader who decided where to be and wrong for a band that decided nothing.
The fill now has its own command. It does not cancel restoration or clear the search target, and unlike `loadHistory` it does not swallow the error either — see the reply on your Important comment, which the same change resolves.
Regression: `filling an edge leaves an outstanding jump and its pending state alone` in `transcript-reading-position-controller.test.ts` — an outstanding search target plus a failing `loadBefore`, asserting `clearSearchTarget` is never called and no history-pending state is set. Pointing `prefetchHistory` back at `loadHistory` fails it.
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -48,40 +54,52 @@ export function createDesktopTranscriptRangeController(
let closed = false;
let openController = new AbortController();
let handle = open(openController.signal);
- type Navigation = DesktopTranscriptNavigation & {
- readonly kind: 'before' | 'after' | 'around' | 'latest' | 'anchor';
- readonly sequence: number | null;
- readonly maxBytes?: number;
- };
- let navigation: Navigation = {
- navigationVersion: 0, intent: 'followTail', kind: 'latest', sequence: null,
- };
+ let navigationVersion = 0;
+ const extending: { older?: Promise<void>; newer?: Promise<void> } = {};
const current = async () => {
if (closed) throw new Error('Desktop transcript range is closed');
return handle;
};
- const dispatch = async (command: Navigation) => {
+ const command = async (
+ replace: boolean,
+ run: (value: DesktopTranscriptHandle, navigation: DesktopTranscriptNavigation) => Promise<void>,
+ ) => {
+ if (replace) {
+ navigationVersion += 1;
+ // Invalidate before awaiting an open handle or any in-flight page.
+ store.expectNavigation(navigationVersion);
+ }
+ const version = navigationVersion;
const opening = handle;
- const isCurrent = () => !closed && navigation === command && opening === handle;
+ const isCurrent = () => !closed && version === navigationVersion && opening === handle;
try {
const value = await current();
if (!isCurrent()) return;
- if (command.kind === 'before') {
- await value.loadBefore(command.sequence, command.maxBytes, command);
- } else if (command.kind === 'after') {
- await value.loadAfter(command.sequence, command.maxBytes, command);
- } else {
- await value.loadAround(command.sequence, command.maxBytes, command);
- }
+ await run(value, { navigationVersion: version });
} catch (error) {
if (isCurrent()) throw error;
}
};
- const navigate = (command: Omit<Navigation, 'navigationVersion'>) => {
- navigation = { ...command, navigationVersion: navigation.navigationVersion + 1 };
- // Invalidate before awaiting an open handle or any previous page request.
- store.expectNavigation(navigation.navigationVersion);
- return dispatch(navigation);
+ const extend = (edge: 'older' | 'newer', maxBytes: number): Promise<void> => {
+ const pending = extending[edge];
+ if (pending) return pending;
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return Promise.resolve();
+ }
+ if (edge === 'older' ? !range.hasOlder : !range.hasNewer) return Promise.resolve();
+ const anchor = edge === 'older' ? range.oldestSequence : range.newestSequence;
+ const task = command(false, (value, navigation) =>
+ edge === 'older'
+ ? value.loadBefore(anchor, maxBytes, navigation)
+ : value.loadAfter(anchor, maxBytes, navigation),
Review Comment:
Fixed in aedc843. Your `[9, 18, 19, 20]` is the sharpest statement of what the model cannot represent: the window is one contiguous interval plus two edge cursors, so a hole in the middle is not merely wrong, it is unnameable — no later page can be asked for it.
`retain()` now mints a window epoch when it drops rows, which is what refuses an extension anchored on the edge it just dropped. It does *not* mint a navigation epoch, so a `loadAround` still in flight is unaffected: a replacement discards the edges anyway, and invalidating it here cost the reader their jump (caught by `partial-history-notice.spec.ts` before it reached you).
Regression: `a page anchored on an edge the band has since dropped is refused` in `transcript-navigation-race.test.ts` — rows 1..3, capture the epoch, `retain(3, 3)`, then feed the encoded older page carrying row 0 under the captured epoch. Every batch is refused, the window stays `[3]`, and `hasOlder` stays true so the gap is still reachable. Dropping the mint from `retain()` fails it.
One thing the fix surfaced, worth recording here: refusing the answer means the band asks again, and it used to ask again immediately from its own callback, so a refusal turned into an unbounded fetch loop — `partial-history-notice.spec.ts` went from 3.4s to a 1.3m hang. That is fixed by the same change as your Important comment: the fill reports whether the window moved, and a read that moved nothing is not reissued until the reader or the range moves.
##########
packages/ui/src/use-chat-scroll.ts:
##########
@@ -171,55 +175,83 @@ export function useChatScroll(input: {
};
}, [authority, input.scrollRef, input.sessionId]);
+ // The window is a band of pixels around the reader: fetch when less than two
+ // screens remain in a direction that has history, drop what lies more than
+ // six screens away and keep four. Both are re-evaluated whenever content or
+ // the reader moves, so the tail prefetches its history and a reader who
+ // stops mid-transcript never pins more than a bounded slice in memory.
+ const bandCheck = useRef<(() => void) | undefined>(undefined);
useEffect(() => {
const root = input.scrollRef.current;
if (!root) return;
+ const inFlight = { up: false, down: false };
const canLoad = (direction: 'up' | 'down'): boolean => direction === 'up'
? input.hasOlderHistory === true && canLoadEarlier
: input.hasNewerHistory === true && canLoadLater;
- // Asking twice is the loader's problem, not this one's: it refuses a
- // request while one is in flight, and asking for history the reader
- // already has is idempotent anyway.
const requestHistory = (direction: 'up' | 'down'): void => {
- activation.current = { sessionId: input.sessionId };
- commandTarget.current = null;
- // Moving input already released following. Only an immovable edge can
- // still be pinned; do not cancel the input operation that requested data.
- if (authority.getSnapshot().pinned) authority.releasePin();
- // A wheel at either edge moves nothing, so no scroll event refreshes the
- // anchor and the restore effect would load around an evicted Turn.
- reportReadingAnchor.current?.();
- const anchorTurnId = direction === 'up'
- ? firstVisibleTurnId(root)
- : lastVisibleTurnId(root);
+ if (inFlight[direction]) return;
// The browser anchors the reader against everything that lands above
- // them, with one exception: it declines while the scroller sits at zero,
- // which is exactly where a wheel asks for history. One pixel is the whole
- // fix — measured in Chromium, an insert of 501px above the reader moves
- // `scrollTop` by 501 at an offset of 1 and by 0 at an offset of 0.
- if (direction === 'up' && root.scrollTop < 1) root.scrollTop = 1;
+ // them, with one exception: it declines while the scroller sits at zero.
+ // One pixel is the whole fix — measured in Chromium, an insert of 501px
+ // above the reader moves `scrollTop` by 501 at an offset of 1 and by 0
+ // at an offset of 0.
+ if (direction === 'up' && !authority.getSnapshot().pinned && root.scrollTop < 1) {
+ root.scrollTop = 1;
+ }
const load = direction === 'up' ? loadEarlierRef.current : loadLaterRef.current;
- void Promise.resolve(load?.(anchorTurnId)).catch(() => undefined);
+ inFlight[direction] = true;
+ void Promise.resolve(load?.()).catch(() => undefined).finally(() => {
+ inFlight[direction] = false;
+ check();
Review Comment:
Fixed in aedc843, and you were right that checking rejection alone would not fix the production path — it also would not fix the case that actually bit: a read *refused* as stale resolves successfully, moves nothing, and asked again forever. `partial-history-notice.spec.ts` went from 3.4s to a 1.3m hang on that.
So the signal is not success versus failure but whether the window moved. The fill answers that, and the band re-checks only when it did:
```ts
(moved) => {
inFlight[direction] = false;
if (moved !== false) check();
},
() => { inFlight[direction] = false; },
```
The re-check has to stay for the case it exists for: the render caused by the landed rows runs while the direction still counts as in flight, so chaining pages depends on it.
Two regressions in `use-chat-scroll.test.tsx`: `a fill that moved nothing is not reissued until the reader moves again` (resolves `false`) and `a failed fill is not reissued until the reader moves again` (rejects). Restoring the unconditional `check()` makes the first hang and the second fail.
##########
apps/desktop/e2e/transcript-scroll-cost.spec.ts:
##########
@@ -125,6 +167,100 @@ async function observe(page: Page): Promise<void> {
});
}
+/**
+ * Watch every frame for a change in the mounted range, and measure what that
+ * change did to the reader.
+ *
+ * A Turn the reader can still see is at `top` in the viewport and at
+ * `top + scrollTop` in the document. Between two frames with no input, its
+ * document position must not move, so `Δtop + ΔscrollTop` is zero — whatever
+ * the Renderer installed above it, the browser's scroll anchoring absorbed. A
+ * page that displaces the reader breaks that sum by however tall the rows it
+ * added or dropped were.
+ *
+ * Sampled per frame rather than per gesture: the frame that installs a page is
+ * the only one where the reader can be lost, and a per-gesture reading would
+ * subtract the reader's own scrolling back out and see nothing.
+ */
+async function observeDisplacement(page: Page): Promise<void> {
+ await page.evaluate((scrollerSelector) => {
+ const scroller = document.querySelector(scrollerSelector);
+ if (!scroller) throw new Error('the chat scroll container is missing');
+ const read = () => {
+ const tops = new Map<string, number>();
+ for (const turn of document.querySelectorAll<HTMLElement>('[data-turn-id]')) {
+ const turnId = turn.dataset.turnId;
+ if (turnId) tops.set(turnId, turn.getBoundingClientRect().top);
+ }
+ return { scrollTop: scroller.scrollTop, tops, key: [...tops.keys()].join(',') };
+ };
+ const state: { boundaries: unknown[]; peakMounted: number; stop(): void } = {
+ boundaries: [],
+ peakMounted: 0,
+ stop: () => { running = false; },
+ };
+ let running = true;
+ let previous = read();
+ // The last frame before the range started changing. Held across a run of
+ // changing frames so the measurement spans settled state to settled state:
+ // scroll anchoring corrects after layout, so a reading taken inside the
+ // change would report a correction that never reached the screen.
+ let settled: ReturnType<typeof read> | null = null;
+ let peakMounted = 0;
+ const tick = (): void => {
+ if (!running) return;
+ const current = read();
+ peakMounted = Math.max(peakMounted, current.tops.size);
+ state.peakMounted = peakMounted;
+ if (current.key !== previous.key) {
+ if (!settled) settled = previous;
+ } else if (settled) {
+ const before = settled;
+ settled = null;
+ const scrolled = current.scrollTop - before.scrollTop;
+ let carried = 0;
+ let worstPx = 0;
+ let worstTurnId: string | null = null;
+ for (const [turnId, top] of current.tops) {
+ const wasAt = before.tops.get(turnId);
+ if (wasAt === undefined) continue;
+ carried += 1;
+ const displaced = Math.abs(top - wasAt + scrolled);
Review Comment:
You are right, and the formula had already misled me before you got here: `top - wasAt + scrolled` reports every correctly anchored page as a displacement the size of the page, which is where an earlier "~1800px systematic displacement" reading of mine came from. The data disproved it — `worstPx ≈ scrolledPx` throughout, i.e. the viewport coordinate never moved.
The probe now reads `|Δtop|` only, settled-to-settled and only across quiet frames (250ms with no wheel input), so a reader-driven scroll is never counted as displacement.
One limit I want to state rather than paper over: a range boundary installs a page *and* drops the far side of the band, and both settle inside the same quiet frame, so what is measurable is their sum. That sum is 18px here on a 700px viewport, unchanged before and after this work, so the bound is `BOUNDARY_DISPLACEMENT_MAX_PX = 40` — a fraction of a Turn, where losing the reader costs a Turn or more. I first tried to split paged boundaries from trimmed ones and the split does not survive contact with the probe: every recorded boundary had already trimmed by the time it settled.
I also dropped this test's `peakMounted <= MOUNTED_TURNS_MAX` assertion. The band is a pixel budget; at this fixture's Turn height 40 Turns is under four screens, so under sustained paging the count reaches 99 while the pixel budget is respected. The two tests above it already hold the window to that budget under their own gestures, and inventing a second proxy here would only have been a number I tuned until it passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5631276687
@orangeCatDeveloper Thank you for tracing this through the fragment path and for the Element / Codex comparison — it helped. You read the install half right, and the code you were reading is gone — the admission decision moved out of fragment decoding and up to the whole answer, so there is no longer a per-row `installDurable` / `overlaid` pair to disagree with each other.
The rule the final design took instead of the in-place refresh: the overlay is a fact about the tail, not a member of the window. It lives on `TranscriptTail` beside the watermark, and `#createSnapshot` appends it only while the window still reaches the tail. A window parked off the tail therefore shows no overlay at all, which is why there is nothing left to refresh in place — and a tail answer it cannot join installs nothing, because `#install` returns `undefined` unless the answer's `coversFrom` is still this window's `through`. When the Turn completes, the durable range stays exactly what the band trimmed it to; reading forward from the window's own edge brings the completed body back. Your probe 3 is the regression for the first half (`transcript-parked-completion-probe.test.ts`), and `transcript-overlay-settlement.test.ts` covers the round trip: overlay hidden on trim, two catch-ups splice nothing, the completed body returns through the edge.
On the `durableThrough` warning — the premise is right, and in fact stronger than you put it. `throughSequence` is `ordinal * STRIDE + STRIDE - 1` (`session-transcript-reader.ts`), while a row sits at `ordinal * STRIDE + offset`, so the watermark is essentially always above the last row, not only across a RuntimeEvent gap. It sticks only if you compare the watermark against the last row's sequence. The store does not: `window.through` is itself a watermark — whatever `durableThrough` the last installed answer carried — and a page read returns the replica's own `#durableThrough` unchanged. So `hasNewer` is watermark-vs-watermark, and when the reader pages forward to the real tail the two are equal and it goes false, including for an answer that returns zero rows. The one case where `through` is a row identity is mid-catch-up, and that publish is always followed by one whose `coversFrom` is that same identity, so a tail-joined window moves off it in the same pass.
Two things guard the spin independently of all that: a read is refused against a window it has already been issued against (`spent[edge]` compared to the snapshot object, which is only re-minted when the window actually moved), and the fill reports whether it issued a read rather than looping on the result.
Your second loose end did survive, in a different shape: a page read before the tail grew must not close the newer edge, which is the `regressed` check in the newer-extend branch. Same instinct as `_expected_max_message_id`. And the Element/Codex reading was right — the design ends up where you pointed, just one layer up: the overlay is structurally outside the window rather than filtered out of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5633014942
Thanks for the pass — all five items plus the nits are addressed.
- Item 1 (the orphaned doc comment above `syntheticLargeTranscript`) — deleted in `4ee559739`.
- Item 2 (the one-field `DesktopTranscriptNavigation` wrapper) — removed in `4bce9ecb4`; the four handle signatures take the number.
- Item 3 (the parked-completion probe asserted nothing about the overlay) — `4ee559739`; it now asserts the overlay retires and row 21 arrives once, at the end, when the reader pages forward.
- Item 4 (props `ChatMessageSurface` re-declared only to forward) — dropped in `4bce9ecb4`; the two callbacks stay required through `Required<Pick<…>>`.
- Item 5 (two doc comments stacked on `onPrefetchHistory`) — both gone in `4ee559739`; the type says it.
- Nits — all taken in `4ee559739`: the dead e2e selectors, the hand-rolled `deferred`, `#bindTranscriptTarget`, `peakMounted`, the twice-stated prose, the pinned numbers, the typo.
Two deliberate keeps, both in `4ee559739`:
- The `ports.ts` sentence: it is the WorkHub port's own contract, and it carries "resolves to whether a read was issued", which the signature does not say.
- The `BOUNDARY_DISPLACEMENT_MAX_PX` rationale, minus the measured 18px you flagged as unverified: without the rationale the next editor raises the threshold instead of investigating.
On item 3, the new overlay assertion was ablated both ways — it fails when the `coversFrom` check is dropped and when the overlay settle is — so the probe now fails for the reason it exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3987094302
##########
packages/ui/src/use-chat-scroll.ts:
##########
@@ -171,55 +178,86 @@ export function useChatScroll(input: {
};
}, [authority, input.scrollRef, input.sessionId]);
+ // The window is a band of pixels around the reader: fetch when less than two
+ // screens remain in a direction that has history, drop what lies more than
+ // six screens away and keep four. Both are re-evaluated whenever content or
+ // the reader moves, so the tail prefetches its history and a reader who
+ // stops mid-transcript never pins more than a bounded slice in memory.
+ const bandCheck = useRef<(() => void) | undefined>(undefined);
useEffect(() => {
const root = input.scrollRef.current;
if (!root) return;
+ const inFlight = { up: false, down: false };
const canLoad = (direction: 'up' | 'down'): boolean => direction === 'up'
- ? input.hasOlderHistory === true && canLoadEarlier
- : input.hasNewerHistory === true && canLoadLater;
- // Asking twice is the loader's problem, not this one's: it refuses a
- // request while one is in flight, and asking for history the reader
- // already has is idempotent anyway.
+ ? input.hasOlderHistory === true && canPrefetch
+ : input.hasNewerHistory === true && canPrefetch;
const requestHistory = (direction: 'up' | 'down'): void => {
- activation.current = { sessionId: input.sessionId };
- commandTarget.current = null;
- // Moving input already released following. Only an immovable edge can
- // still be pinned; do not cancel the input operation that requested data.
- if (authority.getSnapshot().pinned) authority.releasePin();
- // A wheel at either edge moves nothing, so no scroll event refreshes the
- // anchor and the restore effect would load around an evicted Turn.
- reportReadingAnchor.current?.();
- const anchorTurnId = direction === 'up'
- ? firstVisibleTurnId(root)
- : lastVisibleTurnId(root);
+ if (inFlight[direction]) return;
// The browser anchors the reader against everything that lands above
- // them, with one exception: it declines while the scroller sits at zero,
- // which is exactly where a wheel asks for history. One pixel is the whole
- // fix — measured in Chromium, an insert of 501px above the reader moves
- // `scrollTop` by 501 at an offset of 1 and by 0 at an offset of 0.
- if (direction === 'up' && root.scrollTop < 1) root.scrollTop = 1;
- const load = direction === 'up' ? loadEarlierRef.current : loadLaterRef.current;
- void Promise.resolve(load?.(anchorTurnId)).catch(() => undefined);
+ // them, with one exception: it declines while the scroller sits at zero.
+ // One pixel is the whole fix — measured in Chromium, an insert of 501px
+ // above the reader moves `scrollTop` by 501 at an offset of 1 and by 0
+ // at an offset of 0.
+ if (direction === 'up' && !authority.getSnapshot().pinned && root.scrollTop < 1) {
+ root.scrollTop = 1;
+ }
+ inFlight[direction] = true;
+ void Promise.resolve(prefetchRef.current?.(direction === 'up' ? 'older' : 'newer'))
+ .then(
+ // Chaining pages needs a re-check here, because the render that the
+ // landed rows caused ran while this direction still counted as in
+ // flight. A prefetch that issued no read has nothing to chain from.
+ (issued) => { inFlight[direction] = false; if (issued) check(); },
+ () => { inFlight[direction] = false; },
+ );
};
- /** Close enough to the requested edge that the reader is about to reach it. */
- const nearEdge = (direction: 'up' | 'down'): boolean =>
- (direction === 'up'
- ? root.scrollTop
- : root.scrollHeight - root.clientHeight - root.scrollTop)
- <= Math.max(640, root.clientHeight * 2);
- // Nearness alone does not mean the reader wants history — on a transcript
- // shorter than about three viewports the tail is inside this band too, so
- // following it would ask on every write, and content landing above would
- // ask again on every anchoring correction until there was no history left.
- // Which movements were the reader's is not re-derived here; the authority
- // watches the scroller and says so.
- const stopWatchingReader = authority.subscribeToReaderScroll((direction) => {
- if (canLoad(direction) && nearEdge(direction)) requestHistory(direction);
- });
- return stopWatchingReader;
- }, [authority, input.hasOlderHistory, input.hasNewerHistory, canLoadEarlier, canLoadLater,
+ const check = (): void => {
+ if (!root.isConnected || bandCheck.current !== check) return;
+ const screen = Math.max(320, root.clientHeight);
+ const above = root.scrollTop;
+ const below = root.scrollHeight - root.clientHeight - root.scrollTop;
+ if (canLoad('up') && above < screen * 2) requestHistory('up');
+ if (canLoad('down') && below < screen * 2) requestHistory('down');
+ if (above <= screen * 6 && below <= screen * 6) return;
+ const rect = root.getBoundingClientRect();
+ const turns = [...root.querySelectorAll<HTMLElement>('[data-turn-id]')];
+ // A mounted Turn that a pending command is about to reveal must survive
+ // this pass: a page installs it several screens from the reader, so the
+ // band would trim it before the frame that scrolls to it ever runs, and
+ // the command would never land. A target that is not mounted cannot be
+ // trimmed anyway, and waiting for it would let the window grow unbounded.
+ const pending = commandTarget.current !== null
+ && handledTarget.current !== commandTarget.current
+ ? commandTargetTurnId.current
+ : undefined;
+ if (pending && turns.some((turn) => turn.dataset.turnId === pending)) return;
+ const kept = turns.filter((turn) => {
+ const box = turn.getBoundingClientRect();
+ return box.bottom >= rect.top - screen * 4 && box.top <= rect.bottom + screen * 4;
+ });
+ const first = kept[0]?.dataset.turnId;
+ const last = kept.at(-1)?.dataset.turnId;
+ if (!first || !last || kept.length === turns.length) return;
+ retainRef.current?.({ firstTurnId: first, lastTurnId: last });
+ };
+ bandCheck.current = check;
+ // Both phases matter: a gesture at an edge moves nothing and so reports
+ // only `input`, and that is exactly where the next page is wanted.
+ const stopWatchingReader = authority.subscribeToReaderScroll(() => check());
Review Comment:
**Important — re-evaluate the pixel band when the viewport changes size.**
The new budget is defined in `root.clientHeight`, but `check()` is only driven by reader input/scroll and message identity changes. The authority's `ResizeObserver` publishes geometry through `authority.subscribe()`, not `subscribeToReaderScroll()`, so resizing the Desktop window with stable messages never reaches this callback. For example, a parked window with 1,000 px above it is outside the two-screen prefetch threshold at 400 px; enlarging the viewport to 800 px should immediately fill, but no request occurs until the next reader gesture. Shrinking has the inverse problem: content now beyond six screens remains retained until another gesture/change.
That makes the first interaction after maximize/restore pay the paging/trim work this refactor is intended to move ahead of the reader, and the retained range is temporarily not the promised viewport-relative band. Subscribe the band check to geometry publication (with the existing in-flight guards), or observe the root size directly, and cover both grow-to-prefetch and shrink-to-retain without synthetic reader input.
<details><summary>中文</summary>
新预算以 `clientHeight` 为单位，但 band check 只监听读者输入/滚动和消息变化。authority 的 `ResizeObserver` 只通过 `authority.subscribe()` 发布几何变化，不会触发这里的 `subscribeToReaderScroll()`；因此最大化/还原窗口后，预取与裁剪仍按旧尺寸滞留，直到下一次读者手势才执行。应让 band check 订阅几何变化或直接观察 root 尺寸，并覆盖放大触发预取、缩小触发裁剪。
</details>
##########
apps/desktop/src/main/runtime-host-session-observer.ts:
##########
@@ -1159,7 +1238,7 @@ export class RuntimeHostSessionObserver {
this.#broadcast(state.sessionId, event);
}
this.#sendTranscriptChange(state, replica, change);
- if (!change.hasNewer && change.durableUpserts.length > 0) {
+ if (change.durableUpserts.length > 0) {
this.#markTranscriptRead(state, replica);
Review Comment:
**Blocking — do not mark an unseen tail as read.**
A live transcript consumer only proves that the Session is open; it does not prove that the Renderer window reaches the tail. In the parked-history case this PR explicitly handles, `#sendTranscriptChange` can deliver only the new watermark because `coversFrom` does not join the retained window, yet this unconditional call advances the read marker to `latestDurableVisibleMessageId()`. The reader never received or saw that durable row, so a new/completed Turn can lose its unread state while they are still reading older history.
The removed `!change.hasNewer` condition used Main's old window authority, so it should not be restored as-is. But the replacement needs the same semantic fact from the new owner — e.g. the one-bit tail-follow subscription described in #5163, or an acknowledgement that the Renderer actually installed/reached that tail — rather than using consumer existence as visibility. Add a regression with a consumer parked off-tail, advance the durable transcript, and assert the marker does not move until the reader returns to latest.
<details><summary>中文</summary>
consumer 存在只说明 Session 已打开，不说明 Renderer 窗口覆盖尾部。读者停在历史区时，新 durable 行因 `coversFrom` 无法连接而不会进入窗口，但这里仍把 read marker 推到最新消息，导致读者没看到的 Turn 被标为已读。旧 `!change.hasNewer` 依赖 Main 的旧窗口 authority，不应原样恢复；新 owner 需要提供 #5163 里描述的 tail-follow 位，或确认窗口实际安装/到达该尾部后再标记。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5630069570
@Totoro-qaq Both probe files are adopted as-is in `32c19749c` (`transcript-pending-jump-probe.test.ts`, `transcript-parked-completion-probe.test.ts`); the only edits are the API renames (`replaceWindow()` → `navigate()`, `windowEpoch()` → `pendingNavigation()`). All three pass, and the two jump probes fail again if the older-edge anchor check is removed, so they are doing real work.
On the third one — you were right to leave the intended behaviour open, because the settlement test was asserting the hole. The rule is now: the overlay is a fact about the tail, not a member of the window, so a window that no longer reaches the tail does not show it; when the Turn completes, the durable range stays whatever the band trimmed it to (`[5, 6]` in your probe); reading forward from the window's own edge brings the completed body back. `transcript-overlay-settlement.test.ts` is rewritten to that contract.
The underlying change: every answer now carries the anchor it was read from, and the window installs it only where that anchor is still its edge. The PR body's "Scope of the staleness mechanism" describes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5633014116
Thanks for the two retractions — a follow-up audit of every acknowledgement path reached the same conclusions on both. The Renderer-side dedupe is still keyed by the sequence alone; the one case it can swallow is a Host replacement whose tail lands on the same number, which leaves that Session's unread state as it was until the next message, so it is recorded and left as is. The landmark read goes through `handleReconnectableRead`, as you found, so nothing reaches the abandoned branch. (Main's own per-consumer dedupe, `acknowledgedThrough`, was removed in the simplification pass — it mirrored the Renderer's and the `through < durableThrough` gate is what carries the guarantee.) MicroGery's finding was real and is fixed in `0a881c46c`: tail acknowledgement is now opt-in, taken only by the recovering controller that every visible reader uses, with the regression `delegation feedback does not advance the target Session read marker`. The branch also merged origin/main now that #45
62 has landed, which required giving that change's teardown fakes the transcript window contract (`93e72a633`) — without `loadTranscriptLatest` / `acknowledgeTranscriptTail` the source lookup threw and the suite hung. That throw exposed a real leak, fixed in `1c1c3ff1e`: `openTranscript` rejected a source missing the window contract outside its cleanup path, so the registration and its `ready` promise stayed behind. HEAD is green on typecheck, architecture, lint/format, `apps/desktop` (2491 pass), `packages/ui` (418 pass) and the storybook smoke; Playwright is left to CI. A re-review would be welcome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3988209026
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -48,93 +57,132 @@ export function createDesktopTranscriptRangeController(
let closed = false;
let openController = new AbortController();
let handle = open(openController.signal);
- type Navigation = DesktopTranscriptNavigation & {
- readonly kind: 'before' | 'after' | 'around' | 'latest' | 'anchor';
- readonly sequence: number | null;
- readonly maxBytes?: number;
- };
- let navigation: Navigation = {
- navigationVersion: 0, intent: 'followTail', kind: 'latest', sequence: null,
- };
+ /**
+ * The navigation the window currently sits on. Main uses it only to cancel
+ * work a newer navigation has made pointless, so an extension issued while a
+ * navigation is still in flight names that navigation: it is the one Main is
+ * already reading for.
+ */
+ let lastNavigation = 0;
+ const extending: {
+ older?: { anchor: number | null; task: Promise<void> };
+ newer?: { anchor: number | null; task: Promise<void> };
+ } = {};
+ /** The window a read was issued against; the same window answers the same way. */
+ const spent: { older?: object; newer?: object } = {};
const current = async () => {
if (closed) throw new Error('Desktop transcript range is closed');
return handle;
};
- const dispatch = async (command: Navigation) => {
+ const command = async (
+ replace: boolean,
+ run: (value: DesktopTranscriptHandle, navigation: DesktopTranscriptNavigation) => Promise<void>,
+ ) => {
+ // Mint before awaiting an open handle or any in-flight page.
+ const navigation = replace ? store.navigate() : store.pendingNavigation() ?? lastNavigation;
+ if (replace) lastNavigation = navigation;
const opening = handle;
- const isCurrent = () => !closed && navigation === command && opening === handle;
+ const isCurrent = () => !closed && opening === handle &&
+ (!replace || store.pendingNavigation() === navigation);
try {
const value = await current();
if (!isCurrent()) return;
- if (command.kind === 'before') {
- await value.loadBefore(command.sequence, command.maxBytes, command);
- } else if (command.kind === 'after') {
- await value.loadAfter(command.sequence, command.maxBytes, command);
- } else {
- await value.loadAround(command.sequence, command.maxBytes, command);
- }
+ await run(value, { navigation });
} catch (error) {
if (isCurrent()) throw error;
}
};
- const navigate = (command: Omit<Navigation, 'navigationVersion'>) => {
- navigation = { ...command, navigationVersion: navigation.navigationVersion + 1 };
- // Invalidate before awaiting an open handle or any previous page request.
- store.expectNavigation(navigation.navigationVersion);
- return dispatch(navigation);
+ /** The row a read at this edge would anchor on, or undefined with no edge to read. */
+ const edgeAt = (edge: 'older' | 'newer'): { anchor: number | null } | undefined => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return undefined;
+ }
+ if (edge === 'older' ? !range.hasOlder : !range.hasNewer) return undefined;
+ return { anchor: edge === 'older' ? range.oldestSequence : range.newestSequence };
};
+ const extend = async (edge: 'older' | 'newer', maxBytes: number): Promise<boolean> => {
+ const at = edgeAt(edge);
+ if (!at) return false;
+ // Sharing a read only holds while the edge it was anchored on does.
+ const pending = extending[edge];
+ if (pending && pending.anchor === at.anchor) {
+ await pending.task;
+ return false;
+ }
+ // A read issued against this exact window answers it the same way again.
+ // Every commit mints a fresh snapshot object, so a window that moved at all
+ // — by a page, a trim, a navigation or tail growth — is worth asking again.
+ const window = store.snapshot();
+ if (spent[edge] === window) return false;
+ spent[edge] = window;
+ const task = command(false, (value, navigation) =>
+ edge === 'older'
+ ? value.loadBefore(at.anchor, maxBytes, navigation)
+ : value.loadAfter(at.anchor, maxBytes, navigation),
+ ).finally(() => {
+ if (extending[edge]?.task === task) extending[edge] = undefined;
+ });
+ extending[edge] = { anchor: at.anchor, task };
+ await task;
+ return true;
+ };
+ /**
+ * Main marks the Session read from these and from nothing else, so the window
+ * reports every watermark it actually reaches, once. A window with newer
+ * history beyond it reports nothing: the reader is parked off the tail.
+ */
+ let acknowledged: number | undefined;
+ const acknowledgeTail = () => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return;
+ }
+ // A cached window's watermark is a fact about the local cache, not about
+ // the live replica an acknowledgement moves.
+ if (
+ !range.ready || range.hasNewer || range.durableThrough === null ||
+ range.generation.startsWith('cached:')
+ ) return;
+ const through = range.durableThrough;
+ if (acknowledged === through) return;
+ acknowledged = through;
+ void (async () => {
+ try {
+ await (await current()).acknowledgeTail(through);
Review Comment:
Good catch — fixed in `0a881c46c`. Tail acknowledgement is no longer a property of the generic controller: `createDesktopTranscriptRangeController` stays silent unless the caller passes `acknowledgesTail`, and the only production caller that does is `createRecoveringDesktopTranscriptRangeController`, which every visible transcript reader (the app-shell conversation and the live WorkHub transcript) uses and no projection does — so `readDelegatedTurnResult()`, on the plain controller, can never move the target Session read marker. The projection never even needed to reach the tail: Main's first reset is `replica.snapshot()` with `hasNewer: false`, so it was the very first `ready()` that acknowledged. I enumerated every opener of a transcript consumer — four in total, two visible readers and two projections, the other projection (`session-message-settlement`) opening the raw handle and never having had a controller at all — so no other projection can acknowledge either. The r
egression is `delegation feedback does not advance the target Session read marker` in `workhub-coordination-transcript-preload.test.ts`: it drives `delegationFeedback` against a handle that records `acknowledgeTail` calls, with durable rows from a later Turn beyond the delegated one, and asserts the preview still comes back while nothing is acknowledged. Reverting the fix makes it fail with `actual: [2]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3987409998
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -48,93 +56,132 @@ export function createDesktopTranscriptRangeController(
let closed = false;
let openController = new AbortController();
let handle = open(openController.signal);
- type Navigation = DesktopTranscriptNavigation & {
- readonly kind: 'before' | 'after' | 'around' | 'latest' | 'anchor';
- readonly sequence: number | null;
- readonly maxBytes?: number;
- };
- let navigation: Navigation = {
- navigationVersion: 0, intent: 'followTail', kind: 'latest', sequence: null,
- };
+ /**
+ * The navigation the window currently sits on. Main uses it only to cancel
+ * work a newer navigation has made pointless, so an extension issued while a
+ * navigation is still in flight names that navigation: it is the one Main is
+ * already reading for.
+ */
+ let lastNavigation = 0;
+ const extending: {
+ older?: { anchor: number | null; task: Promise<void> };
+ newer?: { anchor: number | null; task: Promise<void> };
+ } = {};
+ /** The window a read was issued against; the same window answers the same way. */
+ const spent: { older?: object; newer?: object } = {};
const current = async () => {
if (closed) throw new Error('Desktop transcript range is closed');
return handle;
};
- const dispatch = async (command: Navigation) => {
+ const command = async (
+ replace: boolean,
+ run: (value: DesktopTranscriptHandle, navigation: DesktopTranscriptNavigation) => Promise<void>,
+ ) => {
+ // Mint before awaiting an open handle or any in-flight page.
+ const navigation = replace ? store.navigate() : store.pendingNavigation() ?? lastNavigation;
+ if (replace) lastNavigation = navigation;
const opening = handle;
- const isCurrent = () => !closed && navigation === command && opening === handle;
+ const isCurrent = () => !closed && opening === handle &&
+ (!replace || store.pendingNavigation() === navigation);
try {
const value = await current();
if (!isCurrent()) return;
- if (command.kind === 'before') {
- await value.loadBefore(command.sequence, command.maxBytes, command);
- } else if (command.kind === 'after') {
- await value.loadAfter(command.sequence, command.maxBytes, command);
- } else {
- await value.loadAround(command.sequence, command.maxBytes, command);
- }
+ await run(value, { navigation });
} catch (error) {
if (isCurrent()) throw error;
}
};
- const navigate = (command: Omit<Navigation, 'navigationVersion'>) => {
- navigation = { ...command, navigationVersion: navigation.navigationVersion + 1 };
- // Invalidate before awaiting an open handle or any previous page request.
- store.expectNavigation(navigation.navigationVersion);
- return dispatch(navigation);
+ /** The row a read at this edge would anchor on, or undefined with no edge to read. */
+ const edgeAt = (edge: 'older' | 'newer'): { anchor: number | null } | undefined => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return undefined;
+ }
+ if (edge === 'older' ? !range.hasOlder : !range.hasNewer) return undefined;
+ return { anchor: edge === 'older' ? range.oldestSequence : range.newestSequence };
+ };
+ const extend = async (edge: 'older' | 'newer', maxBytes: number): Promise<boolean> => {
+ const at = edgeAt(edge);
+ if (!at) return false;
+ // Sharing a read only holds while the edge it was anchored on does.
+ const pending = extending[edge];
+ if (pending && pending.anchor === at.anchor) {
+ await pending.task;
+ return false;
+ }
+ // A read issued against this exact window answers it the same way again.
+ // Every commit mints a fresh snapshot object, so a window that moved at all
+ // — by a page, a trim, a navigation or tail growth — is worth asking again.
+ const window = store.snapshot();
+ if (spent[edge] === window) return false;
+ spent[edge] = window;
+ const task = command(false, (value, navigation) =>
+ edge === 'older'
+ ? value.loadBefore(at.anchor, maxBytes, navigation)
+ : value.loadAfter(at.anchor, maxBytes, navigation),
+ ).finally(() => {
+ if (extending[edge]?.task === task) extending[edge] = undefined;
+ });
+ extending[edge] = { anchor: at.anchor, task };
+ await task;
+ return true;
+ };
+ /**
+ * Main marks the Session read from these and from nothing else, so the window
+ * reports every watermark it actually reaches, once. A window with newer
+ * history beyond it reports nothing: the reader is parked off the tail.
+ */
+ let acknowledged: number | undefined;
+ const acknowledgeTail = () => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return;
+ }
+ // A cached window's watermark is a fact about the local cache, not about
+ // the live replica an acknowledgement moves.
+ if (
+ !range.ready || range.hasNewer || range.durableThrough === null ||
+ range.generation.startsWith('cached:')
+ ) return;
+ const through = range.durableThrough;
+ if (acknowledged === through) return;
Review Comment:
**Blocking — scope tail-ack deduplication to the Host identity, not just the sequence number.**
This value survives registry source recovery, while the preload listener adopts the replacement batch's new `hostEpoch` without recreating this controller. Sequences are explicitly meaningful only within one Host epoch, but a replacement window at the tail with the same numeric `durableThrough` is suppressed here. The new Main consumer therefore never receives an acknowledgement for that tail. This is reachable when a Host replacement/reset reuses sequence N (with potentially different rows, which the new epoch permits); the store commits the replacement, `range.hostEpoch` changes, but `acknowledged === through` returns before the acknowledgement carrying that new epoch is sent.
Deduplicate at least by `{ hostEpoch, through }` (and make the failure rollback compare the same identity), then extend the recovery regression: acknowledge Host A at N, restore the consumer from Host B whose tail is also N, and assert Host B receives its own acknowledgement/read-marker update.
<details><summary>中文</summary>这里的去重状态会跨 registry source recovery 保留，而 sequence 只在同一 `hostEpoch` 内有意义。Host B 的尾部数值恰好仍为 N 时，会被 Host A 已确认的 N 错误拦截，新的 Main consumer 收不到任何确认。应至少按 `{hostEpoch, through}` 去重，并覆盖 A@N → Host 替换 → B@N 的恢复用例。</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3987564939
##########
apps/desktop/src/main/runtime-host-session-observer.ts:
##########
@@ -1159,7 +1238,7 @@ export class RuntimeHostSessionObserver {
this.#broadcast(state.sessionId, event);
}
this.#sendTranscriptChange(state, replica, change);
- if (!change.hasNewer && change.durableUpserts.length > 0) {
+ if (change.durableUpserts.length > 0) {
this.#markTranscriptRead(state, replica);
Review Comment:
Fixed in `bf3b56ffe`.
You are right that consumer existence is not visibility, and right that the old `!change.hasNewer` cannot come back: it was Main reading its own window, and Main no longer has one. So the owner reports the fact instead. The Renderer window acknowledges each watermark it actually reaches — `acknowledgeTail(through)`, sent once per new watermark, only when `hasNewer` is false, never while parked and never for a cached generation. Main records it as `consumer.acknowledgedThrough` and moves the read marker only when that value has reached the replica's own watermark. Marking at open and marking on delivery are both gone; a Session opened at the tail still marks read, but through the acknowledgement, not because it was opened.
The tempting cheaper version — let Main mark eagerly when a change's `coversFrom` meets `acknowledgedThrough` — is what I rejected. It looks like a proof that the window can join the tail, and it is not: a trim landing between the acknowledgement and the change moves the window's edge, the window refuses the change on arrival, and Main has already marked a row the reader never received. The acknowledgement is the only place that knows both facts at once.
Regressions:
- `moves the read marker only as far as the Renderer window reports reaching` (`runtime-host-session-observer.test.ts`) — a consumer parked off the tail watches the durable transcript advance and the marker stays put; acknowledging the current watermark moves it.
- `reports each tail the window reaches once, and none while it is parked` (`desktop-transcript-range-store.test.ts`) — the controller sends one acknowledgement per watermark and nothing at all while `hasNewer`.
The more useful part of this finding is the question behind it, and I want to record what it turned up. This was not a missing branch, it was an incomplete migration: I deleted Main's window authority and left one of its consumers reading a fact that no longer had a source. That is a class of bug, not an instance, so I enumerated every other consumer of the window facts Main used to own and checked whether each one still has a source. Four had lost theirs, and are fixed on the branch:
- `5ca912757` — `loadLatest` replayed Main's tail cache, which global reclaim can trim to empty; it now reads the tail back from the Host before answering, so return-to-latest cannot install a blank window with `hasNewer` false.
- `368ac3d38` — the reading position survives a Host epoch change again, resolved through the Turn landmark index (sequences name the same rows only within one epoch); and a read in flight across that change is raised as superseded rather than as a load error the following reset resolves by itself.
- `b239a9b39` — the WorkHub transcript gets the pixel band (`prefetchHistory` / `retain` on its port, the "older" button removed), so its window is bounded again and a queued follow-up returns the window to the tail instead of waiting for a row it can never join.
- `16191d20f` — the band re-check on viewport resize, from your Important comment below.
Two audited items are deliberately not fixed: an unanswered edge read poisoning that edge, which I could not find a reachable path to with a consumer open, and the rail jump's destination hold, which the browser's own anchoring plus the page-install displacement probe now cover.
##########
packages/ui/src/use-chat-scroll.ts:
##########
@@ -171,55 +178,86 @@ export function useChatScroll(input: {
};
}, [authority, input.scrollRef, input.sessionId]);
+ // The window is a band of pixels around the reader: fetch when less than two
+ // screens remain in a direction that has history, drop what lies more than
+ // six screens away and keep four. Both are re-evaluated whenever content or
+ // the reader moves, so the tail prefetches its history and a reader who
+ // stops mid-transcript never pins more than a bounded slice in memory.
+ const bandCheck = useRef<(() => void) | undefined>(undefined);
useEffect(() => {
const root = input.scrollRef.current;
if (!root) return;
+ const inFlight = { up: false, down: false };
const canLoad = (direction: 'up' | 'down'): boolean => direction === 'up'
- ? input.hasOlderHistory === true && canLoadEarlier
- : input.hasNewerHistory === true && canLoadLater;
- // Asking twice is the loader's problem, not this one's: it refuses a
- // request while one is in flight, and asking for history the reader
- // already has is idempotent anyway.
+ ? input.hasOlderHistory === true && canPrefetch
+ : input.hasNewerHistory === true && canPrefetch;
const requestHistory = (direction: 'up' | 'down'): void => {
- activation.current = { sessionId: input.sessionId };
- commandTarget.current = null;
- // Moving input already released following. Only an immovable edge can
- // still be pinned; do not cancel the input operation that requested data.
- if (authority.getSnapshot().pinned) authority.releasePin();
- // A wheel at either edge moves nothing, so no scroll event refreshes the
- // anchor and the restore effect would load around an evicted Turn.
- reportReadingAnchor.current?.();
- const anchorTurnId = direction === 'up'
- ? firstVisibleTurnId(root)
- : lastVisibleTurnId(root);
+ if (inFlight[direction]) return;
// The browser anchors the reader against everything that lands above
- // them, with one exception: it declines while the scroller sits at zero,
- // which is exactly where a wheel asks for history. One pixel is the whole
- // fix — measured in Chromium, an insert of 501px above the reader moves
- // `scrollTop` by 501 at an offset of 1 and by 0 at an offset of 0.
- if (direction === 'up' && root.scrollTop < 1) root.scrollTop = 1;
- const load = direction === 'up' ? loadEarlierRef.current : loadLaterRef.current;
- void Promise.resolve(load?.(anchorTurnId)).catch(() => undefined);
+ // them, with one exception: it declines while the scroller sits at zero.
+ // One pixel is the whole fix — measured in Chromium, an insert of 501px
+ // above the reader moves `scrollTop` by 501 at an offset of 1 and by 0
+ // at an offset of 0.
+ if (direction === 'up' && !authority.getSnapshot().pinned && root.scrollTop < 1) {
+ root.scrollTop = 1;
+ }
+ inFlight[direction] = true;
+ void Promise.resolve(prefetchRef.current?.(direction === 'up' ? 'older' : 'newer'))
+ .then(
+ // Chaining pages needs a re-check here, because the render that the
+ // landed rows caused ran while this direction still counted as in
+ // flight. A prefetch that issued no read has nothing to chain from.
+ (issued) => { inFlight[direction] = false; if (issued) check(); },
+ () => { inFlight[direction] = false; },
+ );
};
- /** Close enough to the requested edge that the reader is about to reach it. */
- const nearEdge = (direction: 'up' | 'down'): boolean =>
- (direction === 'up'
- ? root.scrollTop
- : root.scrollHeight - root.clientHeight - root.scrollTop)
- <= Math.max(640, root.clientHeight * 2);
- // Nearness alone does not mean the reader wants history — on a transcript
- // shorter than about three viewports the tail is inside this band too, so
- // following it would ask on every write, and content landing above would
- // ask again on every anchoring correction until there was no history left.
- // Which movements were the reader's is not re-derived here; the authority
- // watches the scroller and says so.
- const stopWatchingReader = authority.subscribeToReaderScroll((direction) => {
- if (canLoad(direction) && nearEdge(direction)) requestHistory(direction);
- });
- return stopWatchingReader;
- }, [authority, input.hasOlderHistory, input.hasNewerHistory, canLoadEarlier, canLoadLater,
+ const check = (): void => {
+ if (!root.isConnected || bandCheck.current !== check) return;
+ const screen = Math.max(320, root.clientHeight);
+ const above = root.scrollTop;
+ const below = root.scrollHeight - root.clientHeight - root.scrollTop;
+ if (canLoad('up') && above < screen * 2) requestHistory('up');
+ if (canLoad('down') && below < screen * 2) requestHistory('down');
+ if (above <= screen * 6 && below <= screen * 6) return;
+ const rect = root.getBoundingClientRect();
+ const turns = [...root.querySelectorAll<HTMLElement>('[data-turn-id]')];
+ // A mounted Turn that a pending command is about to reveal must survive
+ // this pass: a page installs it several screens from the reader, so the
+ // band would trim it before the frame that scrolls to it ever runs, and
+ // the command would never land. A target that is not mounted cannot be
+ // trimmed anyway, and waiting for it would let the window grow unbounded.
+ const pending = commandTarget.current !== null
+ && handledTarget.current !== commandTarget.current
+ ? commandTargetTurnId.current
+ : undefined;
+ if (pending && turns.some((turn) => turn.dataset.turnId === pending)) return;
+ const kept = turns.filter((turn) => {
+ const box = turn.getBoundingClientRect();
+ return box.bottom >= rect.top - screen * 4 && box.top <= rect.bottom + screen * 4;
+ });
+ const first = kept[0]?.dataset.turnId;
+ const last = kept.at(-1)?.dataset.turnId;
+ if (!first || !last || kept.length === turns.length) return;
+ retainRef.current?.({ firstTurnId: first, lastTurnId: last });
+ };
+ bandCheck.current = check;
+ // Both phases matter: a gesture at an edge moves nothing and so reports
+ // only `input`, and that is exactly where the next page is wanted.
+ const stopWatchingReader = authority.subscribeToReaderScroll(() => check());
Review Comment:
Fixed in `16191d20f`.
The band observes the root's size directly with its own `ResizeObserver`, rather than subscribing to `authority.subscribe()`. The authority publishes only when its snapshot changes, and the case in question changes nothing it publishes: a resize that keeps the pin and the reading Turn is a new band with the same snapshot, so the callback would never fire. Observing the element whose height defines the budget is the shorter path to the fact, and it reuses the existing in-flight guards unchanged.
Regressions in `use-chat-scroll.test.tsx`, both driven by a resize alone with no synthetic reader input:
- `a viewport that grows fills the band it just widened, without a reader gesture` — growing the scroller issues exactly one older fill.
- `a viewport that shrinks trims what it just pushed beyond the band` — shrinking retains the band around the reader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "MicroGery (via GitHub)" <gi...@apache.org>.
MicroGery commented on code in PR #5170:
URL: https://github.com/apache/maka/pull/5170#discussion_r3987797405
##########
apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:
##########
@@ -48,93 +57,132 @@ export function createDesktopTranscriptRangeController(
let closed = false;
let openController = new AbortController();
let handle = open(openController.signal);
- type Navigation = DesktopTranscriptNavigation & {
- readonly kind: 'before' | 'after' | 'around' | 'latest' | 'anchor';
- readonly sequence: number | null;
- readonly maxBytes?: number;
- };
- let navigation: Navigation = {
- navigationVersion: 0, intent: 'followTail', kind: 'latest', sequence: null,
- };
+ /**
+ * The navigation the window currently sits on. Main uses it only to cancel
+ * work a newer navigation has made pointless, so an extension issued while a
+ * navigation is still in flight names that navigation: it is the one Main is
+ * already reading for.
+ */
+ let lastNavigation = 0;
+ const extending: {
+ older?: { anchor: number | null; task: Promise<void> };
+ newer?: { anchor: number | null; task: Promise<void> };
+ } = {};
+ /** The window a read was issued against; the same window answers the same way. */
+ const spent: { older?: object; newer?: object } = {};
const current = async () => {
if (closed) throw new Error('Desktop transcript range is closed');
return handle;
};
- const dispatch = async (command: Navigation) => {
+ const command = async (
+ replace: boolean,
+ run: (value: DesktopTranscriptHandle, navigation: DesktopTranscriptNavigation) => Promise<void>,
+ ) => {
+ // Mint before awaiting an open handle or any in-flight page.
+ const navigation = replace ? store.navigate() : store.pendingNavigation() ?? lastNavigation;
+ if (replace) lastNavigation = navigation;
const opening = handle;
- const isCurrent = () => !closed && navigation === command && opening === handle;
+ const isCurrent = () => !closed && opening === handle &&
+ (!replace || store.pendingNavigation() === navigation);
try {
const value = await current();
if (!isCurrent()) return;
- if (command.kind === 'before') {
- await value.loadBefore(command.sequence, command.maxBytes, command);
- } else if (command.kind === 'after') {
- await value.loadAfter(command.sequence, command.maxBytes, command);
- } else {
- await value.loadAround(command.sequence, command.maxBytes, command);
- }
+ await run(value, { navigation });
} catch (error) {
if (isCurrent()) throw error;
}
};
- const navigate = (command: Omit<Navigation, 'navigationVersion'>) => {
- navigation = { ...command, navigationVersion: navigation.navigationVersion + 1 };
- // Invalidate before awaiting an open handle or any previous page request.
- store.expectNavigation(navigation.navigationVersion);
- return dispatch(navigation);
+ /** The row a read at this edge would anchor on, or undefined with no edge to read. */
+ const edgeAt = (edge: 'older' | 'newer'): { anchor: number | null } | undefined => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return undefined;
+ }
+ if (edge === 'older' ? !range.hasOlder : !range.hasNewer) return undefined;
+ return { anchor: edge === 'older' ? range.oldestSequence : range.newestSequence };
};
+ const extend = async (edge: 'older' | 'newer', maxBytes: number): Promise<boolean> => {
+ const at = edgeAt(edge);
+ if (!at) return false;
+ // Sharing a read only holds while the edge it was anchored on does.
+ const pending = extending[edge];
+ if (pending && pending.anchor === at.anchor) {
+ await pending.task;
+ return false;
+ }
+ // A read issued against this exact window answers it the same way again.
+ // Every commit mints a fresh snapshot object, so a window that moved at all
+ // — by a page, a trim, a navigation or tail growth — is worth asking again.
+ const window = store.snapshot();
+ if (spent[edge] === window) return false;
+ spent[edge] = window;
+ const task = command(false, (value, navigation) =>
+ edge === 'older'
+ ? value.loadBefore(at.anchor, maxBytes, navigation)
+ : value.loadAfter(at.anchor, maxBytes, navigation),
+ ).finally(() => {
+ if (extending[edge]?.task === task) extending[edge] = undefined;
+ });
+ extending[edge] = { anchor: at.anchor, task };
+ await task;
+ return true;
+ };
+ /**
+ * Main marks the Session read from these and from nothing else, so the window
+ * reports every watermark it actually reaches, once. A window with newer
+ * history beyond it reports nothing: the reader is parked off the tail.
+ */
+ let acknowledged: number | undefined;
+ const acknowledgeTail = () => {
+ let range: DesktopTranscriptRangeState;
+ try {
+ range = store.range();
+ } catch {
+ return;
+ }
+ // A cached window's watermark is a fact about the local cache, not about
+ // the live replica an acknowledgement moves.
+ if (
+ !range.ready || range.hasNewer || range.durableThrough === null ||
+ range.generation.startsWith('cached:')
+ ) return;
+ const through = range.durableThrough;
+ if (acknowledged === through) return;
+ acknowledged = through;
+ void (async () => {
+ try {
+ await (await current()).acknowledgeTail(through);
Review Comment:
Could tail acknowledgement be opt-in for a visible transcript reader? This generic controller is also used by `readDelegatedTurnResult()` in `create-workhub-services.ts`, which opens a transient consumer only to compute a bounded WorkHub result preview; it never renders that transcript. That consumer can reach the tail here and cause Main to advance the target Session read marker. If the delegated Turn has later durable rows, the helper can return only the target preview while acknowledging the later tail and clearing its unread state. Please disable acknowledgement for this projection, or require explicit visible-reader opt-in, with a regression that `delegationFeedback` does not advance the target Session read marker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop)!: give the Renderer the transcript window [maka]
Posted by "Totoro-qaq (via GitHub)" <gi...@apache.org>.
Totoro-qaq commented on PR #5170:
URL: https://github.com/apache/maka/pull/5170#issuecomment-5628720501
@Astro-Han Thanks, here they are. I re-ran them on 15894f8: the first two now pass, and your new regression tests look like they cover the same sequences. The third still reproduces.
Both files follow the layout of `transcript-navigation-race.test.ts`, so they should drop straight into `apps/desktop/src/main/__tests__/`.
On 15894f8:
```text
not ok 1 - a Turn completing at the tail does not splice into a window parked far from it
+ actual - expected
+ 21
ok 2 - a fill issued while a jump is pending does not splice the old edge onto the new window
ok 3 - a trim and a fill while a jump is pending do not make Main drop the jump
```
On aedc843 the two jump tests failed as described earlier: the window ended at `[5, 6, 17]`, and the jump target never arrived.
For the third one I wasn't sure what the intended behaviour is, since the settlement test wants the completed body to stay visible. So the assertion only checks that the durable range stays `[5, 6]`; it doesn't prescribe how the Turn remains on screen.
<details><summary><code>transcript-pending-jump-probe.test.ts</code> (jump + fill, jump + trim + fill)</summary>
```ts
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import assert from 'node:assert/strict';
import test from 'node:test';
import type { StoredMessage } from '@maka/core/session';
import { SESSION_CONTINUITY_SCHEMA_VERSION, type SessionTranscriptPage } from '@maka/runtime-host/protocol';
import type { DesktopTranscriptBatch, DesktopTranscriptRangeRequest } from '../../preload/transcript-contract.js';
import { DesktopTranscriptRangeStore } from '../../renderer/platform/desktop/desktop-transcript-range-store.js';
import { RuntimeHostSessionObserver } from '../runtime-host-session-observer.js';
import { runtimeHostSessionFixture } from './runtime-host-session-test-fixture.js';
// Drives the real observer, replica and Renderer store with a fake Host whose
// jump read (loadAround) is held until a fill has been queued behind it.
const PAGE_BYTES = 128 * 1024;
const THROUGH = 20;
async function harness() {
const store = new DesktopTranscriptRangeStore(JSON.stringify(['host-1', 'session-1']));
const eventsClosed = deferred<void>();
const aroundEntered = deferred<void>();
const releaseAround = deferred<void>();
const bootstrap = page();
const decoded = new Map<SessionTranscriptPage, { messages: Array<ReturnType<typeof record>>; nextCursor: string | null }>([
[bootstrap, { messages: [record(18), record(19), record(20)], nextCursor: 'older' }],
]);
const observer = new RuntimeHostSessionObserver({
client: { openSession: async () => runtimeHostSessionFixture({
snapshot: continuitySnapshot(), transcript: Promise.resolve([]),
events: { async *[Symbol.asyncIterator]() { await eventsClosed.promise; } },
transcriptBootstrap: { throughSequence: THROUGH, overlayMessageCount: 0, durable: bootstrap, overlay: { ...bootstrap, source: 'overlay' } },
loadTranscriptOverlay: async () => [],
decodeTranscriptPage: async (candidate) => decoded.get(candidate)!,
loadTranscriptPage: async (request) => {
const candidate = page();
if (request.direction === 'newer') {
aroundEntered.resolve();
await releaseAround.promise;
decoded.set(candidate, { messages: [record(5), record(6)], nextCursor: 'newer' });
} else if (request.maxBytes === 1) {
decoded.set(candidate, { messages: [record(4)], nextCursor: 'older' });
} else {
decoded.set(candidate, { messages: [record(request.anchorSequence! - 1)], nextCursor: 'older' });
}
return candidate;
},
async close() { eventsClosed.resolve(); },
}) },
emitSessionsChanged() {},
});
const ack = (batch: DesktopTranscriptBatch) => observer.acknowledgeTranscript('consumer-1', batch.generation, batch.deliverySequence, 1);
await observer.openTranscript('session-1', 'consumer-1', {
id: 1, once() {}, off() {},
send(_channel, batch) { store.accept(batch); queueMicrotask(() => ack(batch)); },
});
const request = (windowEpoch: number, anchorSequence: number): DesktopTranscriptRangeRequest => ({
consumerId: 'consumer-1', sessionId: 'session-1', hostEpoch: 'host-1', anchorSequence, maxBytes: PAGE_BYTES, windowEpoch,
});
const sequences = () => store.durableEntries().map(({ sequence }) => sequence);
return { store, observer, request, sequences, aroundEntered, releaseAround };
}
test('a fill issued while a jump is pending does not splice the old edge onto the new window', async () => {
const h = await harness();
try {
assert.deepEqual(h.sequences(), [18, 19, 20]);
const navigation = h.store.replaceWindow();
const jump = h.observer.loadTranscriptAround(h.request(navigation, 5), 1);
await h.aroundEntered.promise;
// Anchored on the window still on screen (18), stamped with the jump's epoch.
const fill = h.observer.loadTranscriptBefore(h.request(h.store.windowEpoch(), h.store.range().oldestSequence!), 1);
h.releaseAround.resolve();
await Promise.all([jump, fill]);
assert.deepEqual(h.sequences(), [5, 6]);
} finally {
await h.observer.close();
}
});
test('a trim and a fill while a jump is pending do not make Main drop the jump', async () => {
const h = await harness();
try {
const navigation = h.store.replaceWindow();
const jump = h.observer.loadTranscriptAround(h.request(navigation, 5), 1);
await h.aroundEntered.promise;
assert.equal(h.store.retain(19, 20), true);
const fill = h.observer.loadTranscriptBefore(h.request(h.store.windowEpoch(), h.store.range().oldestSequence!), 1);
h.releaseAround.resolve();
await Promise.all([jump, fill]);
assert.ok(h.sequences().includes(5), `the jump target never arrived: ${JSON.stringify(h.sequences())}`);
} finally {
await h.observer.close();
}
});
function record(identity: number) {
const message: StoredMessage = { type: 'assistant', id: `message-${identity}`, turnId: `turn-${identity}`, ts: 1, text: String(identity), modelId: 'test' };
return { identity, message };
}
function page(): SessionTranscriptPage {
return { kind: 'page', sessionId: 'session-1', source: 'durable', direction: 'older', throughSequence: THROUGH,
rawBytes: 1, fragments: [], rangeBoundarySequence: null, protectedTurnSequence: null, nextCursor: null };
}
function continuitySnapshot() {
return { schemaVersion: SESSION_CONTINUITY_SCHEMA_VERSION,
session: { sessionId: 'session-1', metadataRevision: 1, status: 'running' as const, createdAt: 1, isArchived: false },
projectionRevision: 1, rootTurn: null, goal: null,
queue: { hostEpoch: 'host-1', queueRevision: 0, steering: [], followup: [] }, interactions: { pending: [] } };
}
function deferred<T>() {
let resolve!: (value: T | PromiseLike<T>) => void;
const promise = new Promise<T>((complete) => { resolve = complete; });
return { promise, resolve };
}
```
</details>
<details><summary><code>transcript-parked-completion-probe.test.ts</code> (a completion landing in a parked window)</summary>
```ts
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import assert from 'node:assert/strict';
import test from 'node:test';
import type { StoredMessage } from '@maka/core/session';
import { DesktopTranscriptRangeStore } from '../../renderer/platform/desktop/desktop-transcript-range-store.js';
import { encodeDesktopTranscriptChange, encodeDesktopTranscriptSnapshot } from '../desktop-transcript-ipc.js';
const identity = { sessionId: 'session-1', hostEpoch: 'host-1', generation: 'generation-1' };
const message = (sequence: number, text = String(sequence)): StoredMessage =>
({ type: 'assistant', id: `message-${sequence}`, turnId: `turn-${sequence}`, ts: 1, text, modelId: 'test' });
test('a Turn completing at the tail does not splice into a window parked far from it', () => {
const store = new DesktopTranscriptRangeStore(JSON.stringify(['host-1', 'session-1']));
// A jump to 5 during a run: loadAround's snapshot carries the live overlay (21).
const navigation = store.replaceWindow();
for (const batch of encodeDesktopTranscriptSnapshot({
...identity, windowEpoch: navigation, durableThrough: 20,
durable: [{ sequence: 5, message: message(5) }, { sequence: 6, message: message(6) }],
overlay: [message(21, 'partial')], hasOlder: true, hasNewer: true,
})) store.accept(batch);
// 21 completes; the tail broadcast carries its durable row.
for (const batch of encodeDesktopTranscriptChange(identity, {
durableThrough: 21, durableUpserts: [{ sequence: 21, message: message(21, 'partial and completed') }],
})) store.accept(batch);
// 7..20 are not in the window, so 21 cannot join its durable range contiguously.
assert.deepEqual(store.durableEntries().map(({ sequence }) => sequence), [5, 6]);
});
```
</details>
_The probes were written with Claude Code; I ran them on both heads._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org