Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion .contextbridge/rules/testing-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ globs: ["packages/**/*.test.ts", "packages/**/*.test.tsx", "packages/**/*TestIds
const env = environment.build({ HOME: tmp });
```

- **Export component test IDs as a colocated object from the component file.** Each React component that exposes test selectors declares its own \`xxxTestIds\` object next to the component definition and exports it. Tests import this object directly, never using string literals for test IDs. The naming pattern is \`\<componentName>TestIds\`.
- **Prefer `toMatchObject` for structured payload assertions.** When a test verifies several fields on the same object or nested payload, use one `expect(value).toMatchObject({ ... })` instead of a run of field-by-field assertions. Keep separate assertions for orthogonal behavior, clearer failure messages, or values that need a specialized matcher.

- **Use the shared deferred-promise helper; do not hand-roll it.** Tests that need manual promise resolution should import `createDeferred` from `@contextbridge/shared/testHelpers`. Do not recreate local `Deferred` types or `new Promise` wrappers in individual test files.
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.

Trying to guide the agent to quit doing this


- **Export component test IDs as a colocated object from the component file.** Each React component that exposes test selectors declares its own `xxxTestIds` object next to the component definition and exports it. Tests import this object directly, never using string literals for test IDs. The naming pattern is `<componentName>TestIds`.

**Good:**

Expand Down
114 changes: 108 additions & 6 deletions packages/annotation/src/App.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { AnnotationPayload } from '@contextbridge/shared/annotationSchema';
import type { AnnotationPayload, AnnotationSubmission } from '@contextbridge/shared/annotationSchema';
import { DOCS_URL, GITHUB_DISCUSSIONS_URL, GITHUB_REPO_URL, SLACK_COMMUNITY_URL } from '@contextbridge/shared/links';
import { createDeferred } from '@contextbridge/shared/testHelpers';
import type { UpdateNotice } from '@contextbridge/shared/updateNoticeSchema';
import { headerTestIds } from '@contextbridge/ui/components/Header';
import { act, cleanup, fireEvent, screen, waitFor } from '@testing-library/react';
Expand All @@ -10,7 +11,7 @@ import { annotationPopoverTestIds } from './AnnotationPopover.tsx';
import { appTestIds } from './App.tsx';
import { globalCommentComposerTestIds } from './GlobalCommentComposer.tsx';
import { submitBarTestIds } from './SubmitBar.tsx';
import { drag, renderApp } from './testHelpers/index.tsx';
import { drag, pressSubmitShortcut, renderApp } from './testHelpers/index.tsx';
import { updateNoticeCardTestIds } from './UpdateNoticeCard.tsx';

describe('App', () => {
Expand Down Expand Up @@ -91,10 +92,15 @@ describe('App', () => {
});

const submission = submitAnnotation.mock.calls[0]?.[0];
expect(submission?.status).toBe('changes_requested');
expect(submission?.threads).toHaveLength(1);
expect(submission?.threads[0]?.subject.kind).toBe('global');
expect(submission?.threads[0]?.messages[0]?.body).toBe('Please spell out rollback steps');
expect(submission).toMatchObject({
status: 'changes_requested',
threads: [
{
subject: { kind: 'global' },
messages: [{ body: 'Please spell out rollback steps' }],
},
],
});
expect(screen.getByTestId(submitBarTestIds.countdown)).toHaveTextContent('This window will close in 3 seconds.');

act(() => timers.advance());
Expand All @@ -103,6 +109,102 @@ describe('App', () => {
expect(timers.closeWindow).toHaveBeenCalledTimes(1);
});

it('submits the review with Cmd+Enter from the global comment textarea', async () => {
const user = userEvent.setup();
const { submitAnnotation } = renderApp({ initialPayload: { contentKind: 'plan', content: '# Ship it' } });
const textarea = screen.getByTestId(globalCommentComposerTestIds.textarea);

await user.type(textarea, 'Please spell out rollback steps');
pressSubmitShortcut(textarea, 'meta');

await waitFor(() => {
expect(submitAnnotation).toHaveBeenCalledTimes(1);
});

const submission = submitAnnotation.mock.calls[0]?.[0];
expect(submission).toMatchObject({
status: 'changes_requested',
threads: [
{
subject: { kind: 'global' },
messages: [{ body: 'Please spell out rollback steps' }],
},
],
});
});

it('submits the review with Ctrl+Enter from the global comment textarea', async () => {
const user = userEvent.setup();
const { submitAnnotation } = renderApp({ initialPayload: { contentKind: 'plan', content: '# Ship it' } });
const textarea = screen.getByTestId(globalCommentComposerTestIds.textarea);

await user.type(textarea, 'Needs a migration plan');
pressSubmitShortcut(textarea, 'ctrl');

await waitFor(() => {
expect(submitAnnotation).toHaveBeenCalledTimes(1);
});

const submission = submitAnnotation.mock.calls[0]?.[0];
expect(submission).toMatchObject({
status: 'changes_requested',
threads: [
{
subject: { kind: 'global' },
messages: [{ body: 'Needs a migration plan' }],
},
],
});
});

it('does not submit again from the global shortcut while submitting', async () => {
const user = userEvent.setup();
const pendingSubmission = createDeferred<void>();
const submitAnnotation = vi
.fn<(submission: AnnotationSubmission) => Promise<void>>()
.mockReturnValue(pendingSubmission.promise);
renderApp({ initialPayload: { contentKind: 'plan', content: '# Ship it' } }, { submitAnnotation });
const textarea = screen.getByTestId(globalCommentComposerTestIds.textarea);

await user.type(textarea, 'Needs a migration plan');
pressSubmitShortcut(textarea, 'meta');

await waitFor(() => {
expect(screen.getByTestId(submitBarTestIds.button)).toBeDisabled();
});
pressSubmitShortcut(textarea, 'meta');

expect(submitAnnotation).toHaveBeenCalledTimes(1);

pendingSubmission.resolve();
await screen.findByTestId(submitBarTestIds.countdown);
});

it('does not submit again from the global shortcut after submitted', async () => {
const user = userEvent.setup();
const { submitAnnotation } = renderApp({ initialPayload: { contentKind: 'plan', content: '# Ship it' } });
const textarea = screen.getByTestId(globalCommentComposerTestIds.textarea);

await user.type(textarea, 'Looks good');
pressSubmitShortcut(textarea, 'meta');

await screen.findByTestId(submitBarTestIds.countdown);
pressSubmitShortcut(textarea, 'meta');

expect(submitAnnotation).toHaveBeenCalledTimes(1);
});

it('keeps plain Enter as textarea input in the global comment composer', async () => {
const user = userEvent.setup();
const { submitAnnotation } = renderApp({ initialPayload: { contentKind: 'plan', content: '# Ship it' } });
const textarea = screen.getByTestId(globalCommentComposerTestIds.textarea);

await user.type(textarea, 'Line one{enter}Line two');

expect(textarea).toHaveValue('Line one\nLine two');
expect(submitAnnotation).not.toHaveBeenCalled();
});

it('opens an annotation draft from a clicked target and submits it', async () => {
const user = userEvent.setup();
const { submitAnnotation } = renderApp({
Expand Down
2 changes: 1 addition & 1 deletion packages/annotation/src/CommentsSidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function CommentsSidebar({
</div>

<div className="flex shrink-0 flex-col gap-3 border-t border-border pt-4">
<GlobalCommentComposer globalComment={globalComment} submitted={submission.submitted} />
<GlobalCommentComposer globalComment={globalComment} submission={submission} />
<SubmitBar source={source} submission={submission} />
</div>
</section>
Expand Down
16 changes: 14 additions & 2 deletions packages/annotation/src/GlobalCommentComposer.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { Textarea } from '@contextbridge/ui/components/ui/textarea';
import type { SubmissionState } from './SubmitBar.tsx';
import { useSubmitOnCmdEnter } from './useSubmitOnCmdEnter.ts';

export const globalCommentComposerTestIds = {
textarea: 'plan-review-global-comment-textarea',
Expand All @@ -11,16 +13,26 @@ export interface GlobalCommentState {

export interface GlobalCommentComposerProps {
globalComment: GlobalCommentState;
submitted: boolean;
submission: Pick<SubmissionState, 'submit' | 'submitting' | 'submitted'>;
}

export function GlobalCommentComposer({ globalComment, submitted }: GlobalCommentComposerProps) {
export function GlobalCommentComposer({ globalComment, submission }: GlobalCommentComposerProps) {
const { submitted, submitting, submit } = submission;
const handleKeyDown = useSubmitOnCmdEnter(() => {
if (submitted || submitting) {
return;
}

void submit();
});

return (
<Textarea
className="min-h-24 resize-none rounded-md border bg-background text-sm leading-6 transition focus-visible:border-chart-3/40 focus-visible:ring-chart-3/10"
disabled={submitted}
data-testid={globalCommentComposerTestIds.textarea}
onChange={(event) => globalComment.setBody(event.target.value)}
onKeyDown={handleKeyDown}
placeholder="Add overall feedback…"
value={globalComment.body}
/>
Expand Down
9 changes: 9 additions & 0 deletions packages/annotation/src/testHelpers/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ export function renderApp(
return { result, ...fake };
}

export type SubmitShortcutModifier = 'meta' | 'ctrl';

export function pressSubmitShortcut(element: Element, modifier: SubmitShortcutModifier): void {
fireEvent.keyDown(element, {
key: 'Enter',
metaKey: modifier === 'meta',
ctrlKey: modifier === 'ctrl',
});
}
type DragArgs = {
target: Text;
from: number;
Expand Down
3 changes: 2 additions & 1 deletion packages/cli/src/annotation/runAnnotation.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import type { AnnotationSubmission } from '@contextbridge/shared/annotationSchema';
import { createDeferred } from '@contextbridge/shared/testHelpers';
import { describe, expect, it } from 'bun:test';
import { annotationArgs } from '#src/testFactories.ts';
import { createAnnotationDependencies, createDeferred, createStubContext } from '#src/testHelpers/index.ts';
import { createAnnotationDependencies, createStubContext } from '#src/testHelpers/index.ts';
import { runAnnotation } from './runAnnotation.ts';

describe('runAnnotation', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/commands/open.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ import { tmpdir } from 'node:os';
import { join, resolve } from 'node:path';
import type { AnnotationSubmission } from '@contextbridge/shared/annotationSchema';
import { annotationSubmission } from '@contextbridge/shared/testFactories';
import { createDeferred } from '@contextbridge/shared/testHelpers';
import { afterEach, beforeEach, describe, expect, it } from 'bun:test';
import { CommanderError } from 'commander';
import { formatAgentResponse } from '#src/formatters/annotation/markdown.ts';
import { DOCUMENT_TEMPLATES } from '#src/formatters/document/templates.ts';
import {
createAnnotationDependencies,
createDeferred,
createStubContext,
readErrorLogs,
readWarnLogs,
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/commands/plan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ import { tmpdir } from 'node:os';
import { join } from 'node:path';
import type { AnnotationSubmission } from '@contextbridge/shared/annotationSchema';
import { annotationSubmission } from '@contextbridge/shared/testFactories';
import { createDeferred } from '@contextbridge/shared/testHelpers';
import { afterEach, beforeEach, describe, expect, it } from 'bun:test';
import { CommanderError } from 'commander';
import { formatAgentResponse } from '#src/formatters/annotation/markdown.ts';
import { PLAN_TEMPLATES } from '#src/formatters/plan/templates.ts';
import {
createAnnotationDependencies,
createDeferred,
createStubContext,
readErrorLogs,
readLogs,
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/testHelpers/annotationFakes.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { AnnotationPayload, AnnotationSubmission } from '@contextbridge/shared/annotationSchema';
import { annotationSubmission } from '@contextbridge/shared/testFactories';
import { createDeferred } from '@contextbridge/shared/testHelpers';
import type { AnnotationDependencies } from '#src/annotation/runAnnotation.ts';
import { createDeferred } from './createDeferred.ts';

export interface TrackedAnnotationDependencies extends AnnotationDependencies {
/** Payloads passed to startReviewServer in invocation order. */
Expand Down
1 change: 0 additions & 1 deletion packages/cli/src/testHelpers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,4 @@ export { MemoryStream } from './MemoryStream.ts';
export { type TestContext, createStubContext } from './createStubContext.ts';
export { parseStdoutJson } from './parseStdoutJson.ts';
export { createAnnotationDependencies, type TrackedAnnotationDependencies } from './annotationFakes.ts';
export { createDeferred, type Deferred } from './createDeferred.ts';
export { type LogRecord, readErrorLogs, readLogs, readWarnLogs } from './readLogs.ts';
1 change: 1 addition & 0 deletions packages/shared/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"./links": "./src/links.ts",
"./annotationSchema": "./src/annotationSchema.ts",
"./testFactories": "./src/testFactories.ts",
"./testHelpers": "./src/testHelpers.ts",
"./time": "./src/time.ts",
"./typeGuards": "./src/typeGuards.ts",
"./types": "./src/types.ts",
Expand Down
21 changes: 21 additions & 0 deletions packages/shared/src/testHelpers.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { describe, expect, it } from 'bun:test';
import { createDeferred } from './testHelpers.ts';

describe('createDeferred', () => {
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.

Hoisting this

it('exposes promise controls to resolve later', () => {
const deferred = createDeferred<string>();

deferred.resolve('done');

expect(deferred.promise).resolves.toBe('done');
});

it('exposes promise controls to reject later', () => {
const deferred = createDeferred<string>();
const error = new Error('failed');

deferred.reject(error);

expect(deferred.promise).rejects.toThrow(error);
});
});