From 0e1a5828313de7a66021e19aca8faa353cd7367a Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 7 May 2026 16:50:31 -0700 Subject: [PATCH] Revert "Add experimental newest-first issue thread" (#5460) This is actually bad. Glad it was under experiments. --- packages/shared/src/types/instance.ts | 1 - packages/shared/src/validators/instance.ts | 1 - .../instance-settings-routes.test.ts | 3 - server/src/services/instance-settings.ts | 2 - ui/src/components/IssueChatThread.test.tsx | 174 +----------- ui/src/components/IssueChatThread.tsx | 266 +++++++----------- ui/src/pages/InstanceExperimentalSettings.tsx | 20 -- ui/src/pages/IssueDetail.test.tsx | 44 --- ui/src/pages/IssueDetail.tsx | 10 - 9 files changed, 106 insertions(+), 415 deletions(-) diff --git a/packages/shared/src/types/instance.ts b/packages/shared/src/types/instance.ts index c9328ac0..ee6a6553 100644 --- a/packages/shared/src/types/instance.ts +++ b/packages/shared/src/types/instance.ts @@ -29,7 +29,6 @@ export interface InstanceGeneralSettings { export interface InstanceExperimentalSettings { enableEnvironments: boolean; enableIsolatedWorkspaces: boolean; - enableNewestFirstIssueThread: boolean; autoRestartDevServerWhenIdle: boolean; enableIssueGraphLivenessAutoRecovery: boolean; issueGraphLivenessAutoRecoveryLookbackHours: number; diff --git a/packages/shared/src/validators/instance.ts b/packages/shared/src/validators/instance.ts index 4c77448d..3415539a 100644 --- a/packages/shared/src/validators/instance.ts +++ b/packages/shared/src/validators/instance.ts @@ -38,7 +38,6 @@ export const patchInstanceGeneralSettingsSchema = instanceGeneralSettingsSchema. export const instanceExperimentalSettingsSchema = z.object({ enableEnvironments: z.boolean().default(false), enableIsolatedWorkspaces: z.boolean().default(false), - enableNewestFirstIssueThread: z.boolean().default(false), autoRestartDevServerWhenIdle: z.boolean().default(false), enableIssueGraphLivenessAutoRecovery: z.boolean().default(false), issueGraphLivenessAutoRecoveryLookbackHours: z diff --git a/server/src/__tests__/instance-settings-routes.test.ts b/server/src/__tests__/instance-settings-routes.test.ts index d09ef151..41e52190 100644 --- a/server/src/__tests__/instance-settings-routes.test.ts +++ b/server/src/__tests__/instance-settings-routes.test.ts @@ -64,7 +64,6 @@ describe("instance settings routes", () => { mockInstanceSettingsService.getExperimental.mockResolvedValue({ enableEnvironments: false, enableIsolatedWorkspaces: false, - enableNewestFirstIssueThread: false, autoRestartDevServerWhenIdle: false, enableIssueGraphLivenessAutoRecovery: true, issueGraphLivenessAutoRecoveryLookbackHours: 24, @@ -82,7 +81,6 @@ describe("instance settings routes", () => { experimental: { enableEnvironments: true, enableIsolatedWorkspaces: true, - enableNewestFirstIssueThread: false, autoRestartDevServerWhenIdle: false, enableIssueGraphLivenessAutoRecovery: true, issueGraphLivenessAutoRecoveryLookbackHours: 24, @@ -125,7 +123,6 @@ describe("instance settings routes", () => { expect(getRes.body).toEqual({ enableEnvironments: false, enableIsolatedWorkspaces: false, - enableNewestFirstIssueThread: false, autoRestartDevServerWhenIdle: false, enableIssueGraphLivenessAutoRecovery: true, issueGraphLivenessAutoRecoveryLookbackHours: 24, diff --git a/server/src/services/instance-settings.ts b/server/src/services/instance-settings.ts index 5806a1f6..c447a920 100644 --- a/server/src/services/instance-settings.ts +++ b/server/src/services/instance-settings.ts @@ -41,7 +41,6 @@ function normalizeExperimentalSettings(raw: unknown): InstanceExperimentalSettin return { enableEnvironments: parsed.data.enableEnvironments ?? false, enableIsolatedWorkspaces: parsed.data.enableIsolatedWorkspaces ?? false, - enableNewestFirstIssueThread: parsed.data.enableNewestFirstIssueThread ?? false, autoRestartDevServerWhenIdle: parsed.data.autoRestartDevServerWhenIdle ?? false, enableIssueGraphLivenessAutoRecovery: parsed.data.enableIssueGraphLivenessAutoRecovery ?? false, issueGraphLivenessAutoRecoveryLookbackHours: @@ -52,7 +51,6 @@ function normalizeExperimentalSettings(raw: unknown): InstanceExperimentalSettin return { enableEnvironments: false, enableIsolatedWorkspaces: false, - enableNewestFirstIssueThread: false, autoRestartDevServerWhenIdle: false, enableIssueGraphLivenessAutoRecovery: false, issueGraphLivenessAutoRecoveryLookbackHours: diff --git a/ui/src/components/IssueChatThread.test.tsx b/ui/src/components/IssueChatThread.test.tsx index 61d3a8ce..3918b39b 100644 --- a/ui/src/components/IssueChatThread.test.tsx +++ b/ui/src/components/IssueChatThread.test.tsx @@ -295,19 +295,16 @@ function createFileDragEvent(type: string, files: File[]) { describe("IssueChatThread", () => { let container: HTMLDivElement; - const originalDocumentElementScrollIntoView = document.documentElement.scrollIntoView; beforeEach(() => { container = document.createElement("div"); document.body.appendChild(container); window.scrollTo = vi.fn(); - document.documentElement.scrollIntoView = vi.fn() as unknown as typeof document.documentElement.scrollIntoView; localStorage.clear(); }); afterEach(() => { container.remove(); - document.documentElement.scrollIntoView = originalDocumentElementScrollIntoView; vi.useRealTimers(); appendMock.mockReset(); markdownEditorFocusMock.mockReset(); @@ -330,7 +327,6 @@ describe("IssueChatThread", () => { liveRuns={[]} onAdd={async () => {}} showComposer={false} - newestFirst enableLiveTranscriptPolling={false} /> , @@ -340,16 +336,6 @@ describe("IssueChatThread", () => { expect(container.textContent).toContain("Jump to latest"); expect(container.textContent).not.toContain("Chat ("); - const threadRoot = container.querySelector('[data-testid="thread-root"]'); - const jumpButton = Array.from(container.querySelectorAll("button")).find( - (button) => button.textContent === "Jump to latest", - ); - expect(threadRoot).not.toBeNull(); - expect(jumpButton).toBeDefined(); - expect( - threadRoot?.compareDocumentPosition(jumpButton!), - ).toBe(Node.DOCUMENT_POSITION_FOLLOWING); - const viewport = container.querySelector('[data-testid="thread-viewport"]') as HTMLDivElement | null; expect(viewport).not.toBeNull(); expect(viewport?.className).not.toContain("overflow-y-auto"); @@ -360,106 +346,6 @@ describe("IssueChatThread", () => { }); }); - it("defaults to oldest-first rendering and jump placement", () => { - const root = createRoot(container); - - act(() => { - root.render( - - {}} - showComposer={false} - enableLiveTranscriptPolling={false} - /> - , - ); - }); - - const rows = Array.from(container.querySelectorAll('[data-testid="issue-chat-message-row"]')); - expect(rows[0]?.textContent).toContain("Older comment"); - expect(rows[1]?.textContent).toContain("Newer comment"); - - const threadRoot = container.querySelector('[data-testid="thread-root"]'); - const jumpButton = Array.from(container.querySelectorAll("button")).find( - (button) => button.textContent === "Jump to latest", - ); - expect(threadRoot).not.toBeNull(); - expect(jumpButton).toBeDefined(); - expect( - threadRoot?.compareDocumentPosition(jumpButton!), - ).toBe(Node.DOCUMENT_POSITION_PRECEDING); - - act(() => { - root.unmount(); - }); - }); - - it("renders the jump control above the thread when newest-first mode is disabled", () => { - const root = createRoot(container); - - act(() => { - root.render( - - {}} - showComposer={false} - newestFirst={false} - enableLiveTranscriptPolling={false} - /> - , - ); - }); - - const threadRoot = container.querySelector('[data-testid="thread-root"]'); - const jumpButton = Array.from(container.querySelectorAll("button")).find( - (button) => button.textContent === "Jump to latest", - ); - expect(threadRoot).not.toBeNull(); - expect(jumpButton).toBeDefined(); - expect( - threadRoot?.compareDocumentPosition(jumpButton!), - ).toBe(Node.DOCUMENT_POSITION_PRECEDING); - - act(() => { - root.unmount(); - }); - }); - it("renders the composer in planning mode when the issue is in planning mode", () => { const root = createRoot(container); @@ -1073,7 +959,6 @@ describe("IssueChatThread", () => { agentMap={issueChatLongThreadAgentMap} currentUserId="user-board" onAdd={async () => {}} - newestFirst enableLiveTranscriptPolling={false} onRefreshLatestComments={async () => { setComments([olderComment, latestComment]); @@ -1110,7 +995,7 @@ describe("IssueChatThread", () => { }); }); - it("findLatestCommentMessageIndex prefers the first comment-anchored row when newest renders first", () => { + it("findLatestCommentMessageIndex prefers the last comment-anchored row (PAP-2672)", () => { const messages = [ { metadata: { custom: { anchorId: "comment-a" } } }, { metadata: { custom: { anchorId: "run-1" } } }, @@ -1118,7 +1003,7 @@ describe("IssueChatThread", () => { { metadata: { custom: { anchorId: "run-2" } } }, { metadata: { custom: { anchorId: "activity-3" } } }, ]; - expect(findLatestCommentMessageIndex(messages as never)).toBe(0); + expect(findLatestCommentMessageIndex(messages as never)).toBe(2); expect( findLatestCommentMessageIndex([ { metadata: { custom: { anchorId: "run-only" } } }, @@ -1127,17 +1012,6 @@ describe("IssueChatThread", () => { expect(findLatestCommentMessageIndex([] as never)).toBe(-1); }); - it("findLatestCommentMessageIndex prefers the last comment-anchored row when newest-first mode is disabled", () => { - const messages = [ - { metadata: { custom: { anchorId: "comment-a" } } }, - { metadata: { custom: { anchorId: "run-1" } } }, - { metadata: { custom: { anchorId: "comment-b" } } }, - { metadata: { custom: { anchorId: "run-2" } } }, - { metadata: { custom: { anchorId: "activity-3" } } }, - ]; - expect(findLatestCommentMessageIndex(messages as never, false)).toBe(2); - }); - it("keeps the direct render path for short threads under the virtualization threshold", () => { const root = createRoot(container); const directComments = issueChatLongThreadComments.slice(0, 12); @@ -1846,50 +1720,6 @@ describe("IssueChatThread", () => { }); }); - it("renders the comment timestamp above the comment body", () => { - vi.useFakeTimers(); - vi.setSystemTime(new Date("2026-04-08T12:00:00.000Z")); - const root = createRoot(container); - - act(() => { - root.render( - - {}} - showComposer={false} - newestFirst - enableLiveTranscriptPolling={false} - /> - , - ); - }); - - const text = container.textContent ?? ""; - const timestampIndex = text.indexOf("2d ago"); - expect(timestampIndex).toBeGreaterThanOrEqual(0); - expect(timestampIndex).toBeLessThan(text.indexOf("Agent summary")); - - act(() => { - root.unmount(); - }); - }); - it("shows deferred wake badge only for hold-deferred queued comments", () => { const root = createRoot(container); diff --git a/ui/src/components/IssueChatThread.tsx b/ui/src/components/IssueChatThread.tsx index 778e845d..36cf2eb7 100644 --- a/ui/src/components/IssueChatThread.tsx +++ b/ui/src/components/IssueChatThread.tsx @@ -138,7 +138,6 @@ import { IssueAssignedBacklogNotice } from "./IssueAssignedBacklogNotice"; interface IssueChatMessageContext { feedbackDataSharingPreference: FeedbackDataSharingPreference; feedbackTermsUrl: string | null; - newestFirst: boolean; agentMap?: Map; currentUserId?: string | null; userLabelMap?: ReadonlyMap | null; @@ -177,7 +176,6 @@ interface IssueChatMessageContext { const IssueChatCtx = createContext({ feedbackDataSharingPreference: "prompt", feedbackTermsUrl: null, - newestFirst: true, issueStatus: undefined, successfulRunHandoff: null, }); @@ -333,7 +331,6 @@ interface IssueChatThreadProps { onWorkModeChange?: (workMode: IssueWorkMode) => Promise | void; showComposer?: boolean; showJumpToLatest?: boolean; - newestFirst?: boolean; emptyMessage?: string; variant?: "full" | "embedded"; enableLiveTranscriptPolling?: boolean; @@ -533,6 +530,7 @@ function IssueChatFallbackThread({ const DRAFT_DEBOUNCE_MS = 800; const COMPOSER_FOCUS_SCROLL_PADDING_PX = 96; +const SUBMIT_SCROLL_RESERVE_VH = 0.4; type ComposerAttachmentItem = { id: string; @@ -621,33 +619,6 @@ function commentDateLabel(date: Date | string | undefined): string { return formatShortDate(date); } -function IssueChatTimestampLink({ - anchorId, - createdAt, - className, -}: { - anchorId?: string; - createdAt?: Date | string; - className?: string; -}) { - if (!createdAt) return null; - return ( - - - - {commentDateLabel(createdAt)} - - - - {formatDateTime(createdAt)} - - - ); -} - const IssueChatTextPart = memo(function IssueChatTextPart({ text, recessed }: { text: string; recessed?: boolean }) { const { onImageClick } = useContext(IssueChatCtx); if (isSuccessfulRunHandoffComment(text)) { @@ -1288,7 +1259,6 @@ function IssueChatUserMessage({ onCancelQueued, currentUserId, userProfileMap, - newestFirst, } = useContext(IssueChatCtx); const custom = message.metadata.custom as Record; const anchorId = typeof custom.anchorId === "string" ? custom.anchorId : undefined; @@ -1320,20 +1290,13 @@ function IssueChatUserMessage({ ); const messageBody = (
-
+
{resolvedAuthorName} {followUpRequested ? ( Follow-up ) : null} - {newestFirst ? ( - - ) : null}
- {!newestFirst ? ( - - ) : null} + + + + {message.createdAt ? commentDateLabel(message.createdAt) : ""} + + + + {message.createdAt ? formatDateTime(message.createdAt) : ""} + + ) : ( -
-
- {authorName} - {followUpRequested ? ( - - Follow-up - - ) : null} - {isRunning ? ( - - - Running - - ) : null} -
- {newestFirst ? ( - +
+ {authorName} + {followUpRequested ? ( + + Follow-up + + ) : null} + {isRunning ? ( + + + Running + ) : null}
)} @@ -1622,12 +1582,19 @@ function IssueChatAssistantMessage({ onVote={handleVote} /> ) : null} - {!newestFirst ? ( - - ) : null} + + + + {message.createdAt ? commentDateLabel(message.createdAt) : ""} + + + + {message.createdAt ? formatDateTime(message.createdAt) : ""} + +
) : null} + -
{messages.length === 0 ? (
- )) - )} + )) + )} {showComposer ? (
) : null} +
{showComposer ? (
) : null}
- {resolvedShowJumpToLatest && newestFirst ? ( -
- -
- ) : null} - {showComposer ? (
-
-
-
-

Enable Newest-First Issue Thread

-

- Show issue comments and messages with the newest activity first, move the jump control to the bottom of - the page, and surface plain comment timestamps in the header area. -

-
- - toggleMutation.mutate({ enableNewestFirstIssueThread: !enableNewestFirstIssueThread })} - disabled={toggleMutation.isPending} - aria-label="Toggle newest-first issue thread experimental setting" - /> -
-
-
diff --git a/ui/src/pages/IssueDetail.test.tsx b/ui/src/pages/IssueDetail.test.tsx index c3b5a582..331d71ca 100644 --- a/ui/src/pages/IssueDetail.test.tsx +++ b/ui/src/pages/IssueDetail.test.tsx @@ -59,7 +59,6 @@ const mockProjectsApi = vi.hoisted(() => ({ const mockInstanceSettingsApi = vi.hoisted(() => ({ getGeneral: vi.fn(), - getExperimental: vi.fn(), })); const mockNavigate = vi.hoisted(() => vi.fn()); @@ -193,7 +192,6 @@ vi.mock("../components/InlineEditor", () => ({ vi.mock("../components/IssueChatThread", () => ({ IssueChatThread: (props: { - newestFirst?: boolean; onWorkModeChange?: (workMode: string) => void; issueWorkMode?: string; onStopRun?: (runId: string) => Promise; @@ -806,9 +804,6 @@ describe("IssueDetail", () => { keyboardShortcuts: false, feedbackDataSharingPreference: "prompt", }); - mockInstanceSettingsApi.getExperimental.mockResolvedValue({ - enableNewestFirstIssueThread: false, - }); mockIssuesListRender.mockClear(); mockIssueChatThreadRender.mockClear(); }); @@ -844,45 +839,6 @@ describe("IssueDetail", () => { expect(consoleErrorSpy).not.toHaveBeenCalled(); }); - it("passes oldest-first thread mode when the experimental flag is disabled", async () => { - mockIssuesApi.get.mockResolvedValue(createIssue()); - - await act(async () => { - root.render( - - - , - ); - }); - - await flushReact(); - - expect(mockIssueChatThreadRender.mock.calls.at(-1)?.[0]).toMatchObject({ - newestFirst: false, - }); - }); - - it("passes newest-first thread mode when the experimental flag is enabled", async () => { - mockIssuesApi.get.mockResolvedValue(createIssue()); - mockInstanceSettingsApi.getExperimental.mockResolvedValue({ - enableNewestFirstIssueThread: true, - }); - - await act(async () => { - root.render( - - - , - ); - }); - - await flushReact(); - - expect(mockIssueChatThreadRender.mock.calls.at(-1)?.[0]).toMatchObject({ - newestFirst: true, - }); - }); - it("passes blocker attention to the issue detail header status icon", async () => { mockIssuesApi.get.mockResolvedValue(createIssue({ status: "blocked", diff --git a/ui/src/pages/IssueDetail.tsx b/ui/src/pages/IssueDetail.tsx index c859be20..3663b4c8 100644 --- a/ui/src/pages/IssueDetail.tsx +++ b/ui/src/pages/IssueDetail.tsx @@ -592,7 +592,6 @@ type IssueDetailChatTabProps = { issueId: string; companyId: string; projectId: string | null; - newestFirstIssueThreadEnabled: boolean; issueStatus: Issue["status"]; issueWorkMode: IssueWorkMode; executionRunId: string | null; @@ -656,7 +655,6 @@ const IssueDetailChatTab = memo(function IssueDetailChatTab({ issueId, companyId, projectId, - newestFirstIssueThreadEnabled, issueWorkMode, issueStatus, executionRunId, @@ -857,7 +855,6 @@ const IssueDetailChatTab = memo(function IssueDetailChatTab({ ) : null} 0 || resolvedHasActiveRun; - const { data: experimentalSettings } = useQuery({ - queryKey: queryKeys.instance.experimentalSettings, - queryFn: () => instanceSettingsApi.getExperimental(), - placeholderData: keepPreviousDataForSameQueryTail(issueId ?? "pending"), - }); - const newestFirstIssueThreadEnabled = experimentalSettings?.enableNewestFirstIssueThread === true; useEffect(() => { if (!hasLiveRuns && locallyQueuedCommentRunIds.size > 0) { setLocallyQueuedCommentRunIds(new Map()); @@ -3790,7 +3781,6 @@ export function IssueDetail() { issueId={issue.id} companyId={issue.companyId} projectId={issue.projectId ?? null} - newestFirstIssueThreadEnabled={newestFirstIssueThreadEnabled} issueStatus={issue.status} issueWorkMode={issue.workMode ?? "standard"} executionRunId={issue.executionRunId ?? null}