Skip to content

fix(desktop): dedupe optimistic user turns for all wire references, not only images - #75733

Closed
pnascimento9596 wants to merge 3 commits into
NousResearch:mainfrom
pnascimento9596:fix/desktop-file-ref-optimistic-dedup
Closed

fix(desktop): dedupe optimistic user turns for all wire references, not only images#75733
pnascimento9596 wants to merge 3 commits into
NousResearch:mainfrom
pnascimento9596:fix/desktop-file-ref-optimistic-dedup

Conversation

@pnascimento9596

@pnascimento9596 pnascimento9596 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What changed

Generalize optimistic versus authoritative user-turn reconciliation from image-only normalization to the centralized wire-reference vocabulary in reference-kinds.ts.

The new textWithoutReferenceLines helper removes complete reference-only lines before the existing equality checks. It uses the shared vocabulary for all 8 wire kinds instead of adding a hardcoded @file: path.

This fixes the case where an optimistic message stores a reference in attachmentRefs while authoritative history inlines the same directive in the body.

Fixes #75731

How to test

Install workspace dependencies from the repository root with an engines-compatible npm. The local at-HEAD run used npm 12.0.1:

npx -y npm@12 ci

Run the focused suite at-HEAD:

npx -y npm@12 run --prefix apps/desktop test:ui -- \
  src/app/session/hooks/use-session-actions/utils.test.ts

Result at-HEAD: 54 of 54 tests passed.

Run the full desktop UI gate at-HEAD:

npx -y npm@12 run --prefix apps/desktop check:test:ui

Result at-HEAD: 3182 of 3182 tests passed across 360 files.

The Python wrapper was also run. It reported 56 pre-existing failures across 27 files. This change touches zero Python files, so the Python tree at-HEAD is identical to at-BASE for this range.

Manual reproduction on macOS 26.5.2:

  1. Build and relaunch the desktop renderer.
  2. Attach a Markdown file with the Files context action.
  3. Send once and wait for the turn to complete.
  4. Reload the session from authoritative history.
  5. Confirm one matching user bubble.

The manual check used the clean production-code commit immediately below at-HEAD; at-HEAD adds tests only. It showed one file prompt bubble after hydration. The file chip moved from below the optimistic bubble to inside the authoritative bubble, confirming that both representations were exercised. An image attachment spot-check added exactly one additional user bubble and remained one bubble after authoritative reload.

Platforms tested

  • macOS 26.5.2 manual desktop exercise on the production-code commit immediately below at-HEAD
  • Vitest desktop UI tests, platform-neutral, at-HEAD

Security impact

None. This changes comparison normalization for renderer message text only. It adds no shell execution, filesystem access or path handling, secret handling, network access, or trust-boundary change.

Reconciliation tradeoff

This keeps the existing visible-text equality tradeoff accepted for images in d0f5ef704. Two distinct adjacent sends with identical visible text but different references can compare equal after reference-only lines are removed. This change makes that existing rule uniform across the centralized wire vocabulary rather than introducing a second mechanism.

Reference identity hardening would change reconciliation semantics and should be evaluated separately with turn identity or ordering signals.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 1, 2026

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

Thanks for generalizing the reconciliation rule through the shared wire-reference grammar. The premise is present on current main: apps/desktop/src/app/session/hooks/use-session-actions/utils.ts:406, :418, and :487 normalize only @image: lines, while apps/desktop/src/lib/chat-runtime.ts:229-239 preserves non-image attachment references such as @file: on optimistic turns.

Problems

  • apps/desktop/src/app/session/hooks/use-session-actions/utils.ts:2 fails the required desktop lint gate: perfectionist/sort-imports requires @/components/assistant-ui/reference-kinds before @/hermes. This is the sole lint error reported by the PR CI run.

Suggested changes

  • Sort that added import, then rerun apps/desktop lint. The reconciliation implementation updates all three current image-only comparison sites, and the added tests cover the file case plus the shared wire-kind set.

Automated hermes-sweeper review.

@@ -1,7 +1,8 @@
import { getSession } from '@/hermes'
import { textWithoutReferenceLines } from '@/components/assistant-ui/reference-kinds'

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.

This import must sort before @/hermes; PR CI fails apps/desktop / check:lint with perfectionist/sort-imports on this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b19ff09: reference-kinds now sorts before @/hermes. check:lint green locally; focused 54/54 and full UI 3182/3182 rerun green on the new head. Thanks for the catch.

@teknium1 teknium1 added sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/perf Performance improvement or optimization labels Aug 1, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Salvaged as #77653 with your authorship preserved on all three commits. The bug is real on today's main — the optimistic dedupe strips only @image: lines, so any other reference kind duplicates the user turn (reverting your fix fails 9/54 of your tests, which is exactly the behavioral proof we want). 54/54 + a 298-test sweep green. Thank you! Closing in favor of #77653.

@pnascimento9596

Copy link
Copy Markdown
Contributor Author

Thanks for the salvage and for re-deriving it on
current main. The revert-fails-9-of-54 check is a
cleaner proof than anything in my original body, and
hoisting the matcher in 1f692a8 is the right
follow-through now that the image-only helper is dead.
Appreciated.

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

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: user message with @file: attachment rendered twice after history hydration

4 participants