diff --git a/server/src/__tests__/paperclip-skill-utils.test.ts b/server/src/__tests__/paperclip-skill-utils.test.ts index df10c1a3a2..420d8fddc6 100644 --- a/server/src/__tests__/paperclip-skill-utils.test.ts +++ b/server/src/__tests__/paperclip-skill-utils.test.ts @@ -68,10 +68,13 @@ describe("paperclip skill utils", () => { const skillPath = path.resolve(".agents/skills/create-issue-interaction-ui/SKILL.md"); const skillBody = await fs.readFile(skillPath, "utf8"); const normalizedSkillBody = skillBody.replace(/\s+/g, " "); + const normalizedLowerSkillBody = normalizedSkillBody.toLowerCase(); expect(skillBody).toContain("name: create-issue-interaction-ui"); - expect(skillBody).toContain("Developer/maintainer skill"); - expect(normalizedSkillBody).toContain("Do NOT install this on production Paperclip agents"); + expect(normalizedLowerSkillBody).toContain("developer/maintainer skill"); + expect(normalizedLowerSkillBody).toContain( + "not the operational agents that run inside a deployed paperclip company", + ); expect(skillBody).toContain("packages/shared/src/constants.ts"); expect(skillBody).toContain("server/src/services/issue-thread-interactions.ts"); expect(skillBody).toContain("ui/src/components/IssueThreadInteractionCard.tsx"); diff --git a/ui/src/components/IssueChatThread.test.tsx b/ui/src/components/IssueChatThread.test.tsx index 177c11c6f3..2c8a2d1306 100644 --- a/ui/src/components/IssueChatThread.test.tsx +++ b/ui/src/components/IssueChatThread.test.tsx @@ -794,6 +794,7 @@ describe("IssueChatThread", () => { onAdd={async () => {}} showComposer={false} showJumpToLatest={false} + autoScrollToHashOnInitialLoad enableLiveTranscriptPolling={false} transcriptsByRunId={issueChatLongThreadTranscriptsByRunId} hasOutputForRun={(runId) => issueChatLongThreadTranscriptsByRunId.has(runId)} @@ -891,7 +892,7 @@ describe("IssueChatThread", () => { requestAnimationFrameMock.mockRestore(); }); - it("scrolls loaded hash targets through the virtualized message index", () => { + it("scrolls loaded hash targets through the virtualized message index when initial hash scrolling is enabled", () => { const root = createRoot(container); const targetComment = issueChatLongThreadComments.at(-1); expect(targetComment).toBeDefined(); @@ -910,6 +911,7 @@ describe("IssueChatThread", () => { onAdd={async () => {}} showComposer={false} showJumpToLatest={false} + autoScrollToHashOnInitialLoad enableLiveTranscriptPolling={false} transcriptsByRunId={issueChatLongThreadTranscriptsByRunId} hasOutputForRun={(runId) => issueChatLongThreadTranscriptsByRunId.has(runId)} @@ -1071,8 +1073,8 @@ describe("IssueChatThread", () => { ) as HTMLButtonElement | undefined; expect(jump).toBeDefined(); - // Flush the on-load auto-scroll-to-latest (PAP-97) so this test measures - // only the jump-to-latest interaction, not the initial mount scroll. + // Flush pending mount timers so this test measures only the explicit + // jump-to-latest interaction. act(() => { vi.advanceTimersByTime(500); }); @@ -1101,10 +1103,9 @@ describe("IssueChatThread", () => { scrollHost.remove(); }); - // PAP-97: on first thread load we land on the latest comment instead of the - // top of the thread (board rev-2 feedback for PAP-95). No deep-link hash and - // no user interaction — the scroll must happen purely from mounting. - it("auto-scrolls to the latest comment on initial load (PAP-97)", () => { + // PAP-12003: initial page load must not jump to the latest comment. If a + // user wants the newest message, the explicit Jump to latest control owns it. + it("does not auto-scroll to the latest comment on initial load", () => { vi.useFakeTimers(); container.remove(); const scrollHost = document.createElement("main"); @@ -1142,15 +1143,13 @@ describe("IssueChatThread", () => { ); }); - // No jump click — let the mount auto-scroll's rAF + settle ticks run. + // No jump click: initial render should preserve the page position. act(() => { vi.advanceTimersByTime(500); }); - const scrolledToLatest = - elementScrollToMock.mock.calls.some(([arg]) => hasSmoothScrollBehavior(arg)) - || scrollIntoViewMock.mock.calls.length > 0; - expect(scrolledToLatest).toBe(true); + expect(elementScrollToMock).not.toHaveBeenCalled(); + expect(scrollIntoViewMock).not.toHaveBeenCalled(); Element.prototype.scrollIntoView = originalScrollIntoView; act(() => { @@ -1190,7 +1189,6 @@ describe("IssueChatThread", () => { agentMap={issueChatLongThreadAgentMap} currentUserId="user-board" onAdd={async () => {}} - autoScrollToLatestOnInitialLoad={false} enableLiveTranscriptPolling={false} transcriptsByRunId={issueChatLongThreadTranscriptsByRunId} hasOutputForRun={(runId) => issueChatLongThreadTranscriptsByRunId.has(runId)} @@ -1228,6 +1226,60 @@ describe("IssueChatThread", () => { vi.useRealTimers(); }); + it("can keep the page at the top on initial load even when the URL has a comment hash", () => { + vi.useFakeTimers(); + const originalScrollIntoView = Element.prototype.scrollIntoView; + const scrollIntoViewMock = vi.fn(); + Object.defineProperty(Element.prototype, "scrollIntoView", { + configurable: true, + value: scrollIntoViewMock, + }); + + const root = createRoot(container); + act(() => { + root.render( + + {}} + showComposer={false} + enableLiveTranscriptPolling={false} + /> + , + ); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(scrollIntoViewMock).not.toHaveBeenCalled(); + + Object.defineProperty(Element.prototype, "scrollIntoView", { + configurable: true, + value: originalScrollIntoView, + }); + act(() => { + root.unmount(); + }); + vi.useRealTimers(); + }); + // Regression for PAP-2672: when the merged feed ends with a non-comment row // (run/timeline/embedded output) we still want Jump to latest to land on the // last comment, not whichever activity row sorts last. diff --git a/ui/src/components/IssueChatThread.tsx b/ui/src/components/IssueChatThread.tsx index d5e233887d..d01a5762dd 100644 --- a/ui/src/components/IssueChatThread.tsx +++ b/ui/src/components/IssueChatThread.tsx @@ -481,6 +481,7 @@ interface IssueChatThreadProps { showComposer?: boolean; showJumpToLatest?: boolean; autoScrollToLatestOnInitialLoad?: boolean; + autoScrollToHashOnInitialLoad?: boolean; emptyMessage?: string; footer?: ReactNode; variant?: "full" | "embedded"; @@ -4216,7 +4217,8 @@ export function IssueChatThread({ composerHint = null, showComposer = true, showJumpToLatest, - autoScrollToLatestOnInitialLoad = true, + autoScrollToLatestOnInitialLoad = false, + autoScrollToHashOnInitialLoad = false, emptyMessage, footer, variant = "full", @@ -4246,6 +4248,7 @@ export function IssueChatThread({ }: IssueChatThreadProps) { const location = useLocation(); const lastScrolledHashRef = useRef(null); + const didInitialHashScrollDecisionRef = useRef(false); const virtualizedThreadRef = useRef(null); const bottomAnchorRef = useRef(null); const composerViewportAnchorRef = useRef(null); @@ -4511,19 +4514,23 @@ export function IssueChatThread({ useEffect(() => { const hash = location.hash || (typeof window !== "undefined" ? window.location.hash : ""); - if ( - !( - hash.startsWith("#comment-") - || hash.startsWith("#activity-") - || hash.startsWith("#run-") - || hash.startsWith("#interaction-") - ) - ) return; - if (messages.length === 0 || lastScrolledHashRef.current === hash) return; + const isThreadHash = hash.startsWith("#comment-") + || hash.startsWith("#activity-") + || hash.startsWith("#run-") + || hash.startsWith("#interaction-"); + if (messages.length === 0) return; + if (!isThreadHash) { + if (!didInitialHashScrollDecisionRef.current) { + didInitialHashScrollDecisionRef.current = true; + } + return; + } + if (lastScrolledHashRef.current === hash) return; const targetId = hash.slice(1); if (targetId.startsWith("comment-")) { const targetMessage = messages.find((message) => issueChatMessageAnchorId(message) === targetId); if (targetMessage && issueChatMessageIsDeleted(targetMessage)) { + didInitialHashScrollDecisionRef.current = true; lastScrolledHashRef.current = hash; if (typeof window !== "undefined") { window.history.replaceState(null, "", `${location.pathname}${location.search}`); @@ -4531,6 +4538,13 @@ export function IssueChatThread({ return; } } + if (!didInitialHashScrollDecisionRef.current) { + didInitialHashScrollDecisionRef.current = true; + if (!autoScrollToHashOnInitialLoad) { + lastScrolledHashRef.current = hash; + return; + } + } let cancelled = false; const attemptScroll = (finalAttempt = false) => { if (cancelled || lastScrolledHashRef.current === hash) return; @@ -4549,12 +4563,11 @@ export function IssueChatThread({ cancelAnimationFrame(frame); window.clearTimeout(timeout); }; - }, [location.hash, messageAnchorIndex, messages, useVirtualizedThread]); + }, [autoScrollToHashOnInitialLoad, location.hash, messageAnchorIndex, messages, useVirtualizedThread]); - // On first thread load, land on the latest comment instead of defaulting to - // the top (board rev-2 feedback for PAP-95). A deep-link hash takes - // precedence — the hash-scroll effect above owns that case. Runs once per - // mount, after messages first populate. + // Optional legacy behavior: callers may explicitly request landing on the + // latest comment. The shared default stays off so ordinary page loads keep + // the user's initial viewport stable. useEffect(() => { if (didInitialLatestScrollRef.current) return; if (!autoScrollToLatestOnInitialLoad) return;