Skip to content

fix(mobile): polish pull request review UX papercuts #6343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iscekic wants to merge 1 commit into
base: main
Choose a base branch
from
+559 −268
Open

fix(mobile): polish pull request review UX papercuts #6343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 20 additions & 108 deletions apps/mobile/src/app/(app)/agent-chat/[session-id].mounted.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@ import {
} from '@kilocode/cloud-agent-sdk';
import { kiloId, stubTextPart, stubUserMessage } from '@kilocode/cloud-agent-sdk/test-helpers';

import { i18n } from '@/i18n';
import { sessionResumeUrl } from '@kilocode/app-shared/universal-links';
import { AgentSessionProvider, useSessionManager } from '@/components/agents/session-provider';
import { SessionCopyLinkAction } from '@/components/agents/session-copy-link-action';
import { UserWebConnectionProvider } from '@/components/agents/user-web-connection-provider';
import { useSessionDetailRename } from '@/components/agents/use-session-detail-rename';
import { QueryError } from '@/components/query-error';
import { ScreenHeader } from '@/components/screen-header';
import { Button } from '@/components/ui/button';
import { i18n } from '@/i18n';
import { clearActiveToken, setActiveToken, setSignOutTeardownActive } from '@/lib/auth/token-owner';
import { bumpAuthEpoch, currentAuthEpoch } from '@/lib/auth/auth-epoch';
import { setSignOutActive } from '@/lib/auth/sign-out-state';
Expand Down Expand Up @@ -671,104 +669,32 @@ describe('SessionDetailScreen valid session-id', () => {
expect(propOf(findByType(renderer.root, 'SessionDetailContent')[0], 'resumeAt')).toBeNull();
});

it('keeps the route anchor on the loading header Copy-link action', async () => {
// Owner request item 4 moved the Copy link action off the conversation header
// and into the context details sheet. The loading header therefore reserves
// the loaded header's context pill only: it carries no copy control, because
// the sheet — the copy affordance's home — mounts with SessionDetailContent
// below. Rendering one here would resurrect the control the request removed
// and shift the pill at the loading -> loaded swap.
it('reserves the context pill without a copy control on the loading header', async () => {
useLocalSearchParamsMock.mockReturnValue({ 'session-id': 'sess-1', at: 'msg_42' });
queryState.data = null;
queryState.isPending = true;
const renderer = await mountRoute();

// The transcript has not loaded, so the skeleton header is mounted...
expect(findByType(renderer.root, 'SessionSkeletonMessages')).toHaveLength(1);
expect(findByType(renderer.root, 'SessionDetailContent')).toHaveLength(0);
// ...and its Copy-link action must copy the position the route already
// holds, not an anchor-less session link.
const copyActions = renderer.root.findByType(ScreenHeader).findAllByType(SessionCopyLinkAction);
expect(copyActions).toHaveLength(1);
expect(propOf(copyActions[0], 'anchorMessageId')).toBe('msg_42');
});
});

// The session header's Copy-link action copies the same universal link the OS
// handoff advertises, anchored at the position the transcript is showing.
describe('SessionDetailScreen copy link action', () => {
beforeEach(() => {
clipboardSetStringAsync.mockReset();
hapticsSelection.mockReset();
toastSuccess.mockReset();
toastError.mockReset();
});

async function mountCopyAction(anchorMessageId: string | null) {
const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined };
await act(async () => {
ref.current = TestRenderer.create(
createElement(SessionCopyLinkAction, { sessionId: 'sess-1', anchorMessageId })
);
await Promise.resolve();
});
if (!ref.current) {
throw new Error('copy action did not render');
}
const renderer = ref.current;
onTestFinished(() => {
act(() => {
renderer.unmount();
});
});
return renderer;
}

function copyControl(renderer: TestRenderer.ReactTestRenderer) {
return renderer.root.findByProps({ accessibilityLabel: i18n.t('common.copyLink') });
}

it('copies the resume URL of the shown position and confirms it', async () => {
clipboardSetStringAsync.mockResolvedValue(true);
const renderer = await mountCopyAction('msg_42');

await act(async () => {
pressControl(copyControl(renderer));
await Promise.resolve();
});

expect(clipboardSetStringAsync).toHaveBeenCalledWith(
sessionResumeUrl({ sessionId: 'sess-1', anchorMessageId: 'msg_42' })
);
expect(toastSuccess).toHaveBeenCalledWith(i18n.t('agentChat.chatLink.linkCopied'), {
// Longer than the Sonner default: on Android the system clipboard preview
// covers the bottom-center toast region for its whole default life.
duration: expect.any(Number),
});
expect(hapticsSelection).toHaveBeenCalledTimes(1);
});

it('surfaces a retryable failure when the clipboard rejects', async () => {
clipboardSetStringAsync.mockRejectedValueOnce(new Error('clipboard unavailable'));
const renderer = await mountCopyAction('msg_42');

await act(async () => {
pressControl(copyControl(renderer));
await Promise.resolve();
});

expect(toastError).toHaveBeenCalledWith(i18n.t('agentChat.chatLink.couldNotCopyLink'), {
action: { label: i18n.t('common.tryAgain'), onClick: expect.any(Function) },
});
expect(toastSuccess).not.toHaveBeenCalled();
});

it('copies the session link without a position when the position is unknown', async () => {
clipboardSetStringAsync.mockResolvedValue(true);
const renderer = await mountCopyAction(null);

await act(async () => {
pressControl(copyControl(renderer));
await Promise.resolve();
});

expect(clipboardSetStringAsync).toHaveBeenCalledWith(
sessionResumeUrl({ sessionId: 'sess-1', anchorMessageId: null })
);
const header = renderer.root.findByType(ScreenHeader);
const metrics = findByType(header, 'SessionContextMetrics');
expect(metrics).toHaveLength(1);
expect(propOf(metrics[0], 'loading')).toBe(true);
// No `onPress`: the context sheet, which owns both copy rows, is not
// mounted until SessionDetailContent takes over.
expect(propOf(metrics[0], 'onPress')).toBeUndefined();
expect(
findByType(header, 'Pressable').filter(
node => propOf(node, 'accessibilityLabel') === i18n.t('common.copyLink')
)
).toHaveLength(0);
});
});

Expand Down Expand Up @@ -1426,20 +1352,6 @@ describe.each([
expect(findByType(renderer.root, 'SessionSkeletonMessages')).toHaveLength(1);
expect(findByType(renderer.root, 'SessionDetailContent')).toHaveLength(0);
expect(transcriptText(renderer, 'RootText')).toBe('');
// The loading header reserves the loaded header's Copy-link action, so the
// 44pt control appearing at the swap cannot narrow and re-wrap the title.
const loadingHeader = renderer.root.findByType(ScreenHeader);
const loadingCopyActions = loadingHeader.findAllByType(SessionCopyLinkAction);
expect(loadingCopyActions).toHaveLength(1);
const loadingCopyAction = loadingCopyActions[0];
if (!loadingCopyAction) {
throw new Error('loading header did not render the copy-link action');
}
expect(propOf(loadingCopyAction, 'sessionId')).toBe('sess-1');
expect(propOf(loadingCopyAction, 'anchorMessageId')).toBeNull();
const loadingCopyPressable = loadingCopyAction.findAllByType('Pressable');
expect(loadingCopyPressable).toHaveLength(1);
expect(propOf(loadingCopyPressable[0], 'className')).toContain('h-11 w-11');
await act(async () => {
identity.resolve({ id: 'user-B' });
await vi.advanceTimersByTimeAsync(0);
Expand Down
13 changes: 6 additions & 7 deletions apps/mobile/src/app/(app)/agent-chat/[session-id].tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
SessionSkeletonMessages,
} from '@/components/agents/session-detail-skeleton';
import { SessionContextMetrics } from '@/components/agents/session-context-metrics';
import { SessionCopyLinkAction } from '@/components/agents/session-copy-link-action';
import { AgentSessionProvider } from '@/components/agents/session-provider';
import { useIdentityConfirmation } from '@/components/agents/user-web-connection-provider';
import { buildTerminalErrorCopyText } from '@/components/agents/session-terminal-error';
Expand Down Expand Up @@ -154,11 +153,12 @@ export default function SessionDetailScreen() {
) {
// The composer placeholder holds its own height: nothing may shift when
// the query resolves. Route title hints are not bound to an account.
// The right cluster reserves the loaded header's Copy-link action too, so
// the 44pt control appearing at the swap cannot narrow and re-wrap the
// title. The route already holds the `?at=` anchor, so copying the link
// while the transcript loads keeps the same position the loaded header
// falls back to; with no usable anchor it copies the session-top link.
// The loading header reserves the loaded header's context pill (the loaded
// right cluster is that pill plus an optional PR badge) so the swap cannot
// re-wrap the title. Copying the session link belongs to the context
// details sheet, which mounts with SessionDetailContent below, so this
// header deliberately renders no copy control while the session is
// unresolved.
return (
<View className="flex-1 bg-background">
<ScreenHeader
Expand All @@ -173,7 +173,6 @@ export default function SessionDetailScreen() {
hasMessages={false}
loading
/>
<SessionCopyLinkAction sessionId={sessionId} anchorMessageId={resumeAt} />
</View>
}
/>
Expand Down
99 changes: 99 additions & 0 deletions apps/mobile/src/components/agents/session-context-sheet.mounted.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { type ComponentProps, createElement, type ReactElement } from 'react';
import type * as ReactI18next from 'react-i18next';
import { act, TestRenderer } from '@/test/renderer';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { sessionResumeUrl } from '@kilocode/app-shared/universal-links';

import { Text } from '@/components/ui/text';
import { i18n } from '@/i18n';
Expand All @@ -17,6 +18,8 @@ const holder = vi.hoisted(() => ({
isPending: false,
copied: [] as string[],
copyResult: true as boolean | Promise<boolean>,
copiedLinks: [] as { sessionId: string; anchorMessageId: string | null }[],
linkCopyResult: true as boolean | Promise<boolean>,
}));

vi.mock('@tanstack/react-query', () => ({
Expand All @@ -33,6 +36,11 @@ vi.mock('./session-row-actions', () => ({
holder.copied.push(id);
return holder.copyResult;
},
copySessionLink: async (sessionId: string, anchorMessageId: string | null) => {
await Promise.resolve();
holder.copiedLinks.push({ sessionId, anchorMessageId });
return holder.linkCopyResult;
},
}));
vi.mock('react-i18next', async importOriginal => {
const actual = await importOriginal<typeof ReactI18next>();
Expand Down Expand Up @@ -101,6 +109,7 @@ function renderSheet(
visible: true,
info: INFO,
sessionId: 'ses-123',
anchorMessageId: null,
sessionTitle: 'Greeting',
activeSessionType: null,
ownerConnectionId: null,
Expand Down Expand Up @@ -280,6 +289,7 @@ function sheetElement(
visible: true,
info: INFO,
sessionId: 'ses-123',
anchorMessageId: null,
sessionTitle: 'Greeting',
activeSessionType: null,
ownerConnectionId: null,
Expand Down Expand Up @@ -340,6 +350,8 @@ beforeEach(() => {
holder.isPending = false;
holder.copied = [];
holder.copyResult = true;
holder.copiedLinks = [];
holder.linkCopyResult = true;
});

describe('SessionContextSheet session id and running on', () => {
Expand Down Expand Up @@ -508,6 +520,93 @@ describe('SessionContextSheet session id and running on', () => {
});
});

describe('SessionContextSheet copy link row', () => {
it('shows the resume URL as the row value and copies it from the call to action', async () => {
const renderer = await mountSheet();
const expected = sessionResumeUrl({ sessionId: 'ses-123', anchorMessageId: null });
expect(textValues(renderer)).toContain(i18n.t('common.copyLink'));
expect(textValues(renderer)).toContain(expected);

await act(async () => {
pressByTestID(renderer, 'session-context-sheet-copy-link');
await Promise.resolve();
});

expect(holder.copiedLinks).toEqual([{ sessionId: 'ses-123', anchorMessageId: null }]);
const values = textValues(renderer);
expect(values).toContain(i18n.t('agentChat.chatLink.linkCopied'));
// The row keeps its call-to-action name beside the outcome, so the sheet
// still names the row after the copy completes.
expect(values).toContain(i18n.t('common.copyLink'));
await unmount(renderer);
});

it('copies the row value anchored at the position the sheet was given', async () => {
const renderer = await mountSheet({ anchorMessageId: 'msg-42' });
const expected = sessionResumeUrl({ sessionId: 'ses-123', anchorMessageId: 'msg-42' });
expect(textValues(renderer)).toContain(expected);

await act(async () => {
pressByTestID(renderer, 'session-context-sheet-copy-link');
await Promise.resolve();
});

expect(holder.copiedLinks).toEqual([{ sessionId: 'ses-123', anchorMessageId: 'msg-42' }]);
await unmount(renderer);
});

it('shows the could-not-copy outcome and retries from the same row', async () => {
holder.linkCopyResult = false;
const renderer = await mountSheet();
await act(async () => {
pressByTestID(renderer, 'session-context-sheet-copy-link');
await Promise.resolve();
});
const values = textValues(renderer);
expect(values).toContain(i18n.t('agentChat.chatLink.couldNotCopyLink'));
expect(values).not.toContain(i18n.t('agentChat.chatLink.linkCopied'));
expect(values).toContain(i18n.t('common.copyLink'));

holder.linkCopyResult = true;
await act(async () => {
pressByTestID(renderer, 'session-context-sheet-copy-link');
await Promise.resolve();
});
expect(holder.copiedLinks).toHaveLength(2);
expect(textValues(renderer)).toContain(i18n.t('agentChat.chatLink.linkCopied'));
expect(textValues(renderer)).not.toContain(i18n.t('agentChat.chatLink.couldNotCopyLink'));
await unmount(renderer);
});

it('resets the link feedback to the call to action when the sheet closes', async () => {
const renderer = await mountSheet();
await act(async () => {
pressByTestID(renderer, 'session-context-sheet-copy-link');
await Promise.resolve();
});
expect(textValues(renderer)).toContain(i18n.t('agentChat.chatLink.linkCopied'));
await act(async () => {
renderer.update(sheetElement({ visible: false }));
await Promise.resolve();
});
expect(textValues(renderer)).toContain(i18n.t('common.copyLink'));
expect(textValues(renderer)).not.toContain(i18n.t('agentChat.chatLink.linkCopied'));
await unmount(renderer);
});

it('keeps the link feedback independent of the session id feedback', async () => {
const renderer = await mountSheet();
await act(async () => {
pressByTestID(renderer, 'session-context-sheet-copy-link');
await Promise.resolve();
});
const values = textValues(renderer);
expect(values).toContain(i18n.t('agentChat.chatLink.linkCopied'));
expect(values).not.toContain(i18n.t('agents.sessionRow.idCopied'));
await unmount(renderer);
});
});

describe('SessionContextSheet connection row', () => {
it.each([
{ display: 'connected' as const, copy: 'common.connected' },
Expand Down
Loading
Loading