Skip to content

fix(#3852): jump-to-question scrolls to response start instead of user message#3934

Closed
rodboev wants to merge 2 commits into
nesquena:masterfrom
rodboev:pr/jump-to-response
Closed

fix(#3852): jump-to-question scrolls to response start instead of user message#3934
rodboev wants to merge 2 commits into
nesquena:masterfrom
rodboev:pr/jump-to-response

Conversation

@rodboev

@rodboev rodboev commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

  • jumpToTurnQuestion() at ui.js:502 scrolls to document.getElementById(_userMessageDomId(questionRawIdx)), which is the user message row — so the button puts the question into view, not the answer.
  • The correct target is the first assistant segment of the turn. Assistant segments are div.assistant-segment elements identified by dataset.msgIdx, not id attributes; they must be resolved via container.querySelector('[data-msg-idx="..."]').
  • The render loop already builds questionRawIdxByAssistantRawIdx (assistant rawIdx → question rawIdx). Inverting it gives assistantRawIdxByQuestionRawIdx (question rawIdx → first assistant rawIdx for that turn), which lets us look up the segment to scroll to.
  • _questionJumpButtonHtml() embeds the onclick call; it needs to embed the assistant rawIdx as a second argument so jumpToTurnQuestion() can use it at click time without a DOM query.

What Changed

  • static/ui.js: in _questionJumpButtonHtml(), accept assistantRawIdx as a second parameter and embed it in the onclick; in jumpToTurnQuestion(), resolve and prefer the assistant segment via querySelector('[data-msg-idx]') with block:'start', falling back to the user row; after the questionRawIdxByAssistantRawIdx build loop in renderMessages(), add a reverse map assistantRawIdxByQuestionRawIdx; pass the current rawIdx as the second argument at the _questionJumpButtonHtml() call site.

Why It Matters

Clicking "↑ to question" on an assistant response now positions the viewport at the start of that response rather than at the user message above it, making the navigation button actually useful for finding where a long response begins.

Verification

$env:PYTHONUTF8 = '1'
..\hermes-agent\venv\Scripts\python.exe -m pytest tests/test_sessions.py -v --timeout=60
..\hermes-agent\venv\Scripts\python.exe -m pytest tests/ -v --timeout=60

Manual: open a session with multiple long assistant responses, click "↑ to question" on one — the viewport should scroll to the top of that assistant response block, not to the user message.

Risks / Follow-ups

  • The reverse map only captures the first assistant rawIdx per question, which is correct for block:'start' (scroll to the beginning of the turn). If a question produces many assistant segments (long tool-chain), all buttons correctly point to the same first segment.
  • The fallback to user-row scroll is preserved for cases where assistantRawIdx is not available (e.g., a button rendered before this change, though that would not persist across page reload).

Model Used

Claude Opus 4.6 via Claude Code CLI

@greptile-apps

greptile-apps Bot commented Jun 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes the "↑ to question" navigation button so it scrolls to the start of the assistant response block rather than to the user message above it. It achieves this by building a reverse map (assistantRawIdxByQuestionRawIdx) from the existing forward map and threading the resolved assistant rawIdx through the button's onclick to jumpToTurnQuestion.

  • static/ui.js: _questionJumpButtonHtml and jumpToTurnQuestion each gain an assistantRawIdx parameter; the scroll function now prefers querySelector('[data-msg-idx]') on the assistant segment with block:'start', falling back to the existing user-row scroll. A reverse map built after the forward-map loop ensures all buttons on a multi-segment turn point to the first assistant segment, with a ??rawIdx safety fallback.
  • static/i18n.js: All 13 locale entries for jump_to_question / jump_to_question_label are updated to describe the new "jump to response start" behavior.
  • tests/test_issue2246_question_jump.py: Existing signature-match assertions are updated and a new test verifies the reverse-map construction and call-site expression."

Confidence Score: 5/5

Safe to merge — the change is well-scoped to the jump button, preserves the existing fallback path, and all locale entries are consistently updated.

The reverse-map logic is correct: Map insertion order guarantees the first assistant segment per question turn is captured, the call site uses it with a safe ??rawIdx fallback, and the DOM attribute data-msg-idx is confirmed to be set on every assistant segment. The only open item is a missing CHANGELOG entry.

No files require special attention beyond the missing CHANGELOG entry.

Important Files Changed

Filename Overview
static/ui.js Adds assistantRawIdx parameter to jump button and scroll function; builds a reverse map to find the first assistant segment per question turn; querySelector('[data-msg-idx]') correctly targets the right DOM element. Logic is sound — the reverse map is properly used at the call site with a safe ??rawIdx fallback.
static/i18n.js Updates jump_to_question and jump_to_question_label strings across all 13 supported locales to reflect the new "scroll to response start" behavior. All locale entries have been updated consistently.
tests/test_issue2246_question_jump.py Updates existing signature-match assertions to the new two-parameter signatures and adds a new test test_multi_segment_turn_jumps_to_first_assistant_segment that verifies the reverse map presence and the correct call site expression.

Sequence Diagram

sequenceDiagram
    participant User
    participant Button as ↑ to response button
    participant jumpToTurnQuestion
    participant DOM

    User->>Button: click
    Button->>jumpToTurnQuestion: jumpToTurnQuestion(questionRawIdx, assistantRawIdx)
    jumpToTurnQuestion->>DOM: "querySelector('[data-msg-idx="assistantRawIdx"]')"
    alt assistant segment found
        DOM-->>jumpToTurnQuestion: seg element
        jumpToTurnQuestion->>DOM: "seg.scrollIntoView({block:'start', behavior:'smooth'})"
    else segment not in DOM (windowed)
        jumpToTurnQuestion->>DOM: expand render window via renderMessages()
        jumpToTurnQuestion->>DOM: requestAnimationFrame → retry querySelector
    else no assistantRawIdx (fallback)
        jumpToTurnQuestion->>DOM: getElementById(userMessageDomId)
        DOM-->>jumpToTurnQuestion: user row
        jumpToTurnQuestion->>DOM: row.scrollIntoView + _highlightQuestionRow
    end
Loading

Reviews (2): Last reviewed commit: "fix(#3852): update question-jump pins, w..." | Re-trigger Greptile

Comment thread static/ui.js
Comment thread static/ui.js
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

Read the diff at this PR against static/ui.js on origin/master, plus the row/segment construction in renderMessages() at static/ui.js:8326-8348. The core approach is sound and resolves the exact gap I flagged on #3852 (assistant segments have no stable id, only dataset.msgIdx). Threading the assistant rawIdx into the onclick and resolving it with container.querySelector('[data-msg-idx="..."]') is the right move — and it's unambiguous because rawIdx is the index into S.messages, so an assistant rawIdx never collides with a user row's data-msg-idx.

One concrete problem, though: the reverse map you added is dead code, and the actual behavior diverges from your stated intent.

Code reference

The reverse map at static/ui.js:8169-8172 (PR head):

const assistantRawIdxByQuestionRawIdx=new Map();
for(const [aIdx,qIdx] of questionRawIdxByAssistantRawIdx){
  if(!assistantRawIdxByQuestionRawIdx.has(qIdx)) assistantRawIdxByQuestionRawIdx.set(qIdx,aIdx);
}

But the call site at static/ui.js:8325-8327 never reads it — it passes the current segment's own rawIdx:

const _qJumpTarget=(!isUser&&!m._live)?questionRawIdxByAssistantRawIdx.get(rawIdx):undefined;
const questionJumpBtn = (_qJumpTarget!==undefined&&_qJumpTarget!==null)
  ? _questionJumpButtonHtml(_qJumpTarget, rawIdx)   // <- rawIdx, not the reverse-map lookup

grep -c assistantRawIdxByQuestionRawIdx over the PR head returns 2 (the definition and the populate loop) — it's never consumed.

Diagnosis

This matters for multi-segment turns. A button is rendered on every non-live assistant segment (_qJumpTarget is set for each), so a turn like reasoning → tool-card → final answer renders several jump buttons. With the current code each button scrolls to its own segment's start (the segment whose footer you clicked). That contradicts your Risks/Follow-ups note:

If a question produces many assistant segments (long tool-chain), all buttons correctly point to the same first segment.

They don't — the reverse map that would make them all point to the turn's first segment is computed and discarded.

Two clean resolutions, pick one:

  1. Honor the documented "first segment of the turn" intent — wire the map in at the call site:
    const _qAssistantTarget = assistantRawIdxByQuestionRawIdx.get(_qJumpTarget);
    ... _questionJumpButtonHtml(_qJumpTarget, _qAssistantTarget)
  2. Keep current per-segment behavior (scroll to the response block whose button you clicked — arguably the better UX) and delete the dead map at lines 8169-8172.

Either is fine, but right now the code does (2) while shipping the unused machinery for (1), which will confuse the next reader.

Test plan

The diff is JS-only; the tests/test_sessions.py run in your verification block doesn't exercise this path. Worth a manual check on a turn with an expanded reasoning block + tool card + final answer: click the jump button on the final answer footer and confirm where it lands (final-answer top vs turn top) — that single case distinguishes the two behaviors above and tells you which one to lock in.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Absorbed and shipped in v0.51.352 (Release LP, deployed live) via the batched release #3948 — rebased onto fresh master with attribution, content verified, full-suite (8570) + Codex + Opus gated. A folded-Worklog hidden-segment edge the gate caught was fixed inline (visible-target guard + regression test) before merge. Closes #3852.

Thanks @rodboev! 🙏

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.

2 participants