Skip to content

Limit mobile event history to prevent freezes#3220

Open
RobertEmprechtinger wants to merge 1 commit into
Hmbown:mainfrom
RobertEmprechtinger:fix-mobile-event-limit
Open

Limit mobile event history to prevent freezes#3220
RobertEmprechtinger wants to merge 1 commit into
Hmbown:mainfrom
RobertEmprechtinger:fix-mobile-event-limit

Conversation

@RobertEmprechtinger

Copy link
Copy Markdown

Fixes a browser freeze observed with codewhale serve --mobile on long code-generation threads. The mobile page now keeps only the latest 100 visible events in the DOM, and the thread events SSE endpoint accepts replay_limit so the mobile page does not replay an entire old thread into the browser. Tested locally with serve --mobile on a long generation thread; the mobile browser stayed responsive after the change.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@github-actions

Copy link
Copy Markdown

Thanks @RobertEmprechtinger for taking the time to contribute.

This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a replay_limit parameter to the thread events API and updates the mobile TUI client to limit the number of visible events to 100, pruning older events from the DOM to improve performance. The reviewer feedback is highly constructive and suggests: 1) restricting the server-side replay_limit pruning to initial requests (where since_seq is 0 or unset) to avoid gaps in active streams, and 2) implementing sequence number tracking (lastSeq) in the client to filter out duplicate events that may be replayed during automatic EventSource reconnections.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +2405 to +2409
if let Some(limit) = query.replay_limit
&& backlog.len() > limit
{
backlog = backlog.split_off(backlog.len() - limit);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Applying replay_limit when since_seq is greater than 0 can cause the client to miss events between since_seq and the start of the pruned backlog, leading to state inconsistency or gaps in the event stream. It is safer to only apply the limit when starting a new stream (i.e., since_seq is None or 0).

Suggested change
if let Some(limit) = query.replay_limit
&& backlog.len() > limit
{
backlog = backlog.split_off(backlog.len() - limit);
}
if let Some(limit) = query.replay_limit
&& query.since_seq.unwrap_or(0) == 0
&& backlog.len() > limit
{
backlog = backlog.split_off(backlog.len() - limit);
}

Comment on lines 265 to 267
const state = {
threadId: "",
activeTurnId: "",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Initialize lastSeq in the state object to track the sequence number of the last processed event. This will be used to prevent duplicate events from being appended to the DOM if the EventSource automatically reconnects.

Suggested change
const state = {
threadId: "",
activeTurnId: "",
const state = {
threadId: "",
activeTurnId: "",
lastSeq: 0,

Comment on lines 466 to +469
$("events").innerHTML = "";
if (state.source) state.source.close();
const qs = "?since_seq=0" + (token() ? "&token=" + encodeURIComponent(token()) : "");
const qs = "?since_seq=0&replay_limit=" + MAX_VISIBLE_EVENTS +
(token() ? "&token=" + encodeURIComponent(token()) : "");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Reset state.lastSeq to 0 when selecting a new thread to ensure we correctly accept the replayed backlog events for the new thread.

Suggested change
$("events").innerHTML = "";
if (state.source) state.source.close();
const qs = "?since_seq=0" + (token() ? "&token=" + encodeURIComponent(token()) : "");
const qs = "?since_seq=0&replay_limit=" + MAX_VISIBLE_EVENTS +
(token() ? "&token=" + encodeURIComponent(token()) : "");
$("events").innerHTML = "";
state.lastSeq = 0;
if (state.source) state.source.close();
const qs = "?since_seq=0&replay_limit=" + MAX_VISIBLE_EVENTS +
(token() ? "&token=" + encodeURIComponent(token()) : "");

Comment on lines 418 to +422
$("events").appendChild(item);
pruneVisibleEvents();
state.eventCount += 1;
$("event-count").textContent = state.eventCount + " events";
const visible = Math.min(state.eventCount, MAX_VISIBLE_EVENTS);
$("event-count").textContent = visible + " of " + state.eventCount + " events";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add a check to ignore duplicate events based on their sequence number (seq). Since EventSource automatically reconnects with since_seq=0 on network hiccups, the server will replay the backlog, which would otherwise result in duplicate events being appended to the chat history.

Suggested change
$("events").appendChild(item);
pruneVisibleEvents();
state.eventCount += 1;
$("event-count").textContent = state.eventCount + " events";
const visible = Math.min(state.eventCount, MAX_VISIBLE_EVENTS);
$("event-count").textContent = visible + " of " + state.eventCount + " events";
if (data && data.seq) {
if (data.seq <= state.lastSeq) return;
state.lastSeq = data.seq;
}
$("events").appendChild(item);
pruneVisibleEvents();
state.eventCount += 1;
const visible = Math.min(state.eventCount, MAX_VISIBLE_EVENTS);
$("event-count").textContent = visible + " of " + state.eventCount + " events";

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant