Skip to content

fix(ui): keep loading below pending chat message - #5041

Open
colaforniaw wants to merge 1 commit into
apache:mainfrom
colaforniaw:fix/chat-tail-loading-position
Open

fix(ui): keep loading below pending chat message#5041
colaforniaw wants to merge 1 commit into
apache:mainfrom
colaforniaw:fix/chat-tail-loading-position

Conversation

@colaforniaw

@colaforniaw colaforniaw commented Sep 8, 2026

Copy link
Copy Markdown

Summary

When a new chat message was visible as a transient row but had not yet been persisted into turns, the running status could attach to the previous completed turn.

image

This change keeps the running status below the pending message until the new turn is materialized. When a live turn identity is available and its turn is already materialized, the status remains inside that turn. It also avoids claiming the last loaded turn while newer history is unavailable. In that fallback, the transcript boundary is no longer marked as the active row until a materialized live turn is available.

Added regression coverage for:

  • A transient new message while the previous turn is settled.
  • The bottom fallback before a live turn identity is available.
  • An identified live turn that is already materialized.
  • An identified live turn that is not yet materialized.
  • A transcript with newer history still unloaded.

Verification

  • git diff --check passed.
  • npm run dev passed the libraries, preload, filesystem worker, and main builds, then started Vite and Electron successfully.
  • npm --workspace @maka/ui run typecheck passed before this rebase.
  • npm --workspace @maka/ui run build passed before this rebase; after rebasing, the current worktree is blocked by pre-existing UI type errors in unrelated files.
  • node --test packages/ui/dist/__tests__/chat-view-tail-claim.test.js passed: 5/5 after rebuilding with the current source (the workspace build still reports unrelated baseline type errors).
  • The same regression suite failed when the old last-loaded-turn fallback was restored: 3/4 failures.
  • npm run lint passed: 3,419 files checked.
  • npm run format:check passed: 2,020 files checked.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex analyzed the race condition, implemented the UI fix, and added regression tests.

Checklist

  • Tests cover the change and fail without it

  • Lint, format, typecheck and the affected suites pass locally

    The affected source files pass Biome and git diff --check; the post-rebase UI build is blocked by unrelated baseline type errors.

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@colaforniaw
colaforniaw force-pushed the fix/chat-tail-loading-position branch from d43829c to 5dad8b8 Compare September 8, 2026 15:48
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 8, 2026
@colaforniaw
colaforniaw force-pushed the fix/chat-tail-loading-position branch 4 times, most recently from 48c1a4a to ca9769f Compare September 8, 2026 17:07
@colaforniaw
colaforniaw marked this pull request as ready for review September 8, 2026 17:09
@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 8, 2026

@hqhq1025 hqhq1025 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.

One P2 remains on this head. The change correctly stops attaching an unidentified running state to the previous settled turn, but it also moves queued follow-ups above the active loading row. Local validation passed: build:test, UI 424/424, the focused UI/Desktop handoff set 27/27, UI typecheck, changed-file Biome, and diff checks. GitHub currently exposes only the green label check for this head; no hosted test check is present. The branch cleanly merges with current main (9cb5cc93c).

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread packages/ui/src/chat-view.tsx Outdated
// Only an identified, materialized live turn may own the inline loading UI.
// When the host has not exposed a turn yet, keep the status at the transcript
// boundary instead of guessing that the last loaded turn is still running.
const tailTurnId = liveInFlight && turns.some((turn) => turn.turnId === props.liveTurn!.turnId)

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.

[P2] Keep queued follow-ups below the current response status

When runningStatus is true before a live turn is identified or materialized, this makes tailTurnId undefined. The unchanged rendering below then emits every non-inline transient row before the fallback loading section, including messages whose documented placement is next_turn ("below" the active turn). A render with one current_turn and one next_turn message produced current=5108, next=7732, loading=10156, so the queued follow-up appears above the status for the response that is still running. Please split the fallback ordering so the current pending message remains above loading while next_turn rows remain below it, and add that mixed-placement regression.

@colaforniaw
colaforniaw force-pushed the fix/chat-tail-loading-position branch 4 times, most recently from 6a61c7b to 1756376 Compare September 9, 2026 03:11

@hqhq1025 hqhq1025 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.

The previous loading-placement P2 is fixed on this head: the mixed transient path now renders current_turn → loading → next_turn, and the new regression exercises that ordering.

One P2 remains because the newly added test file fails the repository's mandatory ASF header audit. Local validation otherwise passed: build:test, UI 425/425, Desktop 2,361/2,361, focused handoff/queue coverage 42/42, UI typecheck, full lint and format checks, renderer architecture 101/101, changed-file Biome, and git diff --check. npm run check:asf-headers fails with this new file as the sole violation. No hosted checks are currently reported for this head; GitHub reports it mergeable, and it cleanly merges with current main (8d5c4612c46b19270f00fe7aea33c39dff23dbe5).

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@@ -0,0 +1,117 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more

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.

[P2] Replace this with the repository's canonical ASF header rendering. On this exact head, npm run check:asf-headers exits 1 and identifies this newly added file as the only violation because its line wrapping differs from the current canonical form. That required source-policy gate cannot pass until the header is reconciled; copy the canonical block from a current source file or run the repository's header writer, then rerun the audit.

@colaforniaw
colaforniaw force-pushed the fix/chat-tail-loading-position branch from 1756376 to 04f6c6a Compare September 9, 2026 03:45

@hqhq1025 hqhq1025 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.

Re-reviewed the rewritten head 04f6c6af98aa88f14627b30f3c4d9635eb2279fc. I found no remaining P0-P3 correctness issue.

The previous mixed transient-row regression remains fixed: when the live turn has not yet been identified, the pending current_turn row renders before the fallback loading row, while queued next_turn rows render after it. Once a materialized live turn is identified, loading remains inside that turn. The added regression coverage exercises both mixed and fallback placements. The prior ASF header finding is also resolved with the repository's canonical header.

Validation on this exact head:

  • npm run build:test
  • UI tests: 425/425 passed
  • Focused tail-claim tests: 5/5 passed
  • Full workspace typecheck and lint
  • Renderer architecture checks: 101/101 passed
  • ASF header check, changed-file Biome check, and git diff --check
  • Clean merge-tree against current main (8d5c4612c46b19270f00fe7aea33c39dff23dbe5)

There are currently no hosted checks attached to this head. I did not run a browser/Electron visual smoke test; the ordering contract was verified through the production React component's SSR tests.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@colaforniaw
colaforniaw force-pushed the fix/chat-tail-loading-position branch from 04f6c6a to c907a43 Compare September 10, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants