-
Notifications
You must be signed in to change notification settings - Fork 1
Fix commit message loading fallback #360
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
e9bcc4f
1c4d43e
ae96288
ba9692f
1b849c3
3a23185
0e0e981
f7e0e36
b77776e
ee56e78
e374326
c5feb84
ac2eb1f
0bb8fbf
6689d2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| import type { SemanticChangeMap } from "@/ipc/types"; | ||
| import { useViewModel } from "@/stores/view-model"; | ||
| import { useWidgetStore } from "@/stores/widget-store"; | ||
| import { act, fireEvent, render, screen } from "@testing-library/react"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| const { mockGenerateCommitMessage, mockHandleCommit } = vi.hoisted(() => ({ | ||
| mockGenerateCommitMessage: vi.fn< | ||
| () => Promise< | ||
| | { status: "pending" } | ||
| | { status: "ready"; message: string } | ||
| | { status: "error" } | ||
| > | ||
| >(), | ||
| mockHandleCommit: vi.fn<(args: { message: string }) => Promise<boolean>>(), | ||
| })); | ||
|
|
||
| vi.mock("@/hooks/use-summary", () => ({ | ||
| useSummary: () => ({ | ||
| generateCommitMessage: mockGenerateCommitMessage, | ||
| }), | ||
| })); | ||
|
|
||
| vi.mock("@/hooks/use-git-operations", () => ({ | ||
| useGitOperations: () => ({ | ||
| handleCommit: mockHandleCommit, | ||
| }), | ||
| })); | ||
|
|
||
| vi.mock("@/components/widget/summaries/markdown-description", () => ({ | ||
| MarkdownDescription: ({ text }: { text: string }) => ( | ||
| <div data-testid="commit-body">{text}</div> | ||
| ), | ||
| })); | ||
|
|
||
| import { MergeSection } from "./merge-section"; | ||
|
|
||
| const changeMapFor = ( | ||
| filename: string, | ||
| hash = "hash-1", | ||
| ): SemanticChangeMap => ({ | ||
| groups: [], | ||
| singles: [ | ||
| { | ||
| id: 1, | ||
| hash, | ||
| filename, | ||
| diff: "", | ||
| lineCount: 8, | ||
| createdAt: 1, | ||
| ownSummaryId: null, | ||
| title: "Update shell packages", | ||
| description: "Adds a package to the nix-darwin configuration.", | ||
| }, | ||
| ], | ||
| unsummarizedHashes: [], | ||
| }); | ||
|
|
||
| const flushPromises = async () => { | ||
| await act(async () => { | ||
| await Promise.resolve(); | ||
| }); | ||
| }; | ||
|
|
||
| const commitForm = () => | ||
| screen.getByRole("button", { name: /commit/i }).closest("form")!; | ||
|
|
||
| describe("<MergeSection>", () => { | ||
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| mockGenerateCommitMessage.mockReset(); | ||
| mockGenerateCommitMessage.mockResolvedValue({ status: "pending" }); | ||
| mockHandleCommit.mockReset(); | ||
|
|
||
| const store = useWidgetStore.getState(); | ||
| store.setCommitMessageSuggestion(null); | ||
| store.setProcessing(false); | ||
| store.setEvolvePrompt(""); | ||
| useViewModel.setState({ changeMap: changeMapFor("flake.nix") }); | ||
| mockHandleCommit.mockResolvedValue(true); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| }); | ||
|
|
||
| it("renders loading and fallback states from the draft hook", async () => { | ||
| render(<MergeSection />); | ||
| await flushPromises(); | ||
|
|
||
| const input = screen.getByRole("textbox", { name: "" }); | ||
| const button = screen.getByRole("button", { name: /commit/i }); | ||
|
|
||
| expect(input).toHaveAttribute("placeholder", "Loading..."); | ||
| expect(input).toHaveValue(""); | ||
| expect(button).toBeDisabled(); | ||
|
|
||
| act(() => { | ||
| vi.advanceTimersByTime(10_000); | ||
| }); | ||
|
|
||
| expect(input).toHaveValue("chore(nix): update flake.nix"); | ||
| expect(button).toBeEnabled(); | ||
| expect(screen.getByText(/still generating/i)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("renders generated subject and body returned by the draft hook", async () => { | ||
| mockGenerateCommitMessage.mockResolvedValueOnce({ | ||
| status: "ready", | ||
| message: "feat(nix): generated subject\n\nGenerated body.", | ||
| }); | ||
|
|
||
| render(<MergeSection />); | ||
| await flushPromises(); | ||
|
|
||
| expect(screen.getByRole("textbox", { name: "" })).toHaveValue( | ||
| "feat(nix): generated subject", | ||
| ); | ||
| expect(screen.getByTestId("commit-body")).toHaveTextContent("Generated body."); | ||
| }); | ||
|
|
||
| it("renders error fallback copy when lookup fails", async () => { | ||
| mockGenerateCommitMessage.mockResolvedValueOnce({ status: "error" }); | ||
|
|
||
| render(<MergeSection />); | ||
| await flushPromises(); | ||
|
|
||
| expect(screen.getByRole("textbox", { name: "" })).toHaveValue( | ||
| "chore(nix): update flake.nix", | ||
| ); | ||
| expect(screen.getByText(/suggestion unavailable/i)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("submits the current subject and body, then clears the draft after success", async () => { | ||
| mockGenerateCommitMessage.mockResolvedValueOnce({ | ||
| status: "ready", | ||
| message: "feat(nix): add shell package\n\nUpdates package list.", | ||
| }); | ||
| useWidgetStore.getState().setEvolvePrompt("Add a package"); | ||
|
|
||
| render(<MergeSection />); | ||
| await flushPromises(); | ||
|
|
||
| const input = screen.getByRole("textbox", { name: "" }); | ||
| expect(input).toHaveValue("feat(nix): add shell package"); | ||
|
|
||
| fireEvent.submit(commitForm()); | ||
| await flushPromises(); | ||
|
|
||
| expect(mockHandleCommit).toHaveBeenCalledWith({ | ||
| message: "feat(nix): add shell package\n\nUpdates package list.", | ||
| }); | ||
| expect(useWidgetStore.getState().commitMessageSuggestion).toBeNull(); | ||
| expect(useWidgetStore.getState().evolvePrompt).toBe(""); | ||
| expect(input).toHaveValue(""); | ||
| expect(screen.queryByTestId("commit-body")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("preserves the edited draft after a failed commit", async () => { | ||
| mockHandleCommit.mockResolvedValueOnce(false); | ||
| mockGenerateCommitMessage.mockResolvedValueOnce({ | ||
| status: "ready", | ||
| message: "feat(nix): add shell package\n\nUpdates package list.", | ||
| }); | ||
|
|
||
| render(<MergeSection />); | ||
| await flushPromises(); | ||
|
|
||
| const input = screen.getByRole("textbox", { name: "" }); | ||
| fireEvent.change(input, { target: { value: "custom subject" } }); | ||
| fireEvent.submit(commitForm()); | ||
| await flushPromises(); | ||
|
|
||
| expect(mockHandleCommit).toHaveBeenCalledWith({ | ||
| message: "custom subject\n\nUpdates package list.", | ||
| }); | ||
| expect(input).toHaveValue("custom subject"); | ||
| expect(screen.getByTestId("commit-body")).toHaveTextContent("Updates package list."); | ||
| }); | ||
|
|
||
| it("wires user edits through to submission without a generated body that arrives later", async () => { | ||
| let resolveMessage: ( | ||
| value: { status: "ready"; message: string }, | ||
| ) => void = () => {}; | ||
| mockGenerateCommitMessage.mockReturnValueOnce( | ||
| new Promise((resolve) => { | ||
| resolveMessage = resolve; | ||
| }), | ||
| ); | ||
|
|
||
| render(<MergeSection />); | ||
| await flushPromises(); | ||
|
|
||
| const input = screen.getByRole("textbox", { name: "" }); | ||
| fireEvent.change(input, { target: { value: "custom commit subject" } }); | ||
|
|
||
| await act(async () => { | ||
| resolveMessage({ | ||
| status: "ready", | ||
| message: "feat(nix): generated subject\n\nGenerated body.", | ||
| }); | ||
| }); | ||
|
|
||
| fireEvent.submit(commitForm()); | ||
| await flushPromises(); | ||
|
|
||
| expect(mockHandleCommit).toHaveBeenCalledWith({ | ||
| message: "custom commit subject", | ||
| }); | ||
| expect(screen.queryByTestId("commit-body")).not.toBeInTheDocument(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,39 +3,46 @@ | |
| import { Button } from "@/components/ui/button"; | ||
| import { Input } from "@/components/ui/input"; | ||
| import { MarkdownDescription } from "@/components/widget/summaries/markdown-description"; | ||
| import { commitMessageBody } from "@/components/widget/summaries/markdown-utils"; | ||
| import { useCommitMessageDraft } from "@/components/widget/layout/use-commit-message-draft"; | ||
| import { useGitOperations } from "@/hooks/use-git-operations"; | ||
| import { useSummary } from "@/hooks/use-summary"; | ||
| import { useViewModel } from "@/stores/view-model"; | ||
| import { useWidgetStore } from "@/stores/widget-store"; | ||
| import { GitMerge, Loader2 } from "lucide-react"; | ||
| import { useEffect } from "react"; | ||
|
|
||
| export function MergeSection() { | ||
| const isProcessing = useWidgetStore((s) => s.isProcessing); | ||
| const processingAction = useWidgetStore((s) => s.processingAction); | ||
| const commitMessageSuggestion = useWidgetStore((s) => s.commitMessageSuggestion); | ||
| const changeMap = useViewModel((s) => s.changeMap); | ||
|
|
||
| const { handleCommit } = useGitOperations(); | ||
| const { generateCommitMessage } = useSummary(); | ||
|
|
||
| useEffect(() => { | ||
| generateCommitMessage(); | ||
| }, [generateCommitMessage, changeMap]); | ||
| const { | ||
| body: commitBody, | ||
| reset: resetCommitMessageDraft, | ||
| setSubject: setCommitSubject, | ||
| status, | ||
| subject: commitSubject, | ||
| } = useCommitMessageDraft(changeMap); | ||
|
|
||
| async function handleSubmit(e: React.FormEvent<HTMLFormElement>) { | ||
| e.preventDefault(); | ||
| const subject = | ||
| new FormData(e.currentTarget).get("commitMsg")?.toString() ?? ""; | ||
| const body = commitMessageBody(commitMessageSuggestion ?? ""); | ||
| const message = body ? `${subject}\n\n${body}` : subject; | ||
| await handleCommit({ message }); | ||
| useWidgetStore.getState().setEvolvePrompt(""); | ||
| const subject = commitSubject.trim(); | ||
| if (!subject) { | ||
| return; | ||
| } | ||
|
|
||
| const message = commitBody ? `${subject}\n\n${commitBody}` : subject; | ||
| const didCommit = await handleCommit({ message }); | ||
| if (!didCommit) { | ||
| return; | ||
| } | ||
|
|
||
| const store = useWidgetStore.getState(); | ||
| store.setCommitMessageSuggestion(null); | ||
| store.setEvolvePrompt(""); | ||
| resetCommitMessageDraft(); | ||
|
Comment on lines
+40
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| const commitSubject = (commitMessageSuggestion ?? "").split(/\r?\n/)[0] ?? ""; | ||
| const commitBody = commitMessageBody(commitMessageSuggestion ?? ""); | ||
| const canCommit = !isProcessing && commitSubject.trim().length > 0; | ||
|
|
||
| return ( | ||
| <div className="flex flex-col"> | ||
|
|
@@ -49,21 +56,34 @@ export function MergeSection() { | |
| <form className="pt-4" onSubmit={handleSubmit}> | ||
| <div className="mb-4"> | ||
| <Input | ||
| key={commitMessageSuggestion} | ||
| className="border-border bg-background mb-2" | ||
| defaultValue={commitSubject || commitMessageSuggestion || ''} | ||
| disabled={isProcessing} | ||
| name="commitMsg" | ||
| placeholder="Loading..." | ||
| onChange={(event) => { | ||
| setCommitSubject(event.target.value); | ||
| }} | ||
| placeholder={status === "loading" ? "Loading..." : undefined} | ||
| value={commitSubject} | ||
| disabled={isProcessing} | ||
| /> | ||
| {status === "fallback" && ( | ||
| <p className="mb-2 text-muted-foreground text-xs"> | ||
| Still generating a better suggestion. This fallback will update if | ||
| one arrives. | ||
| </p> | ||
| )} | ||
| {status === "error" && ( | ||
| <p className="mb-2 text-muted-foreground text-xs"> | ||
| Suggestion unavailable. You can commit with this fallback or edit it. | ||
| </p> | ||
| )} | ||
| {commitBody && ( | ||
| <MarkdownDescription modalTitle={commitSubject} text={commitBody} /> | ||
| )} | ||
| </div> | ||
|
|
||
| <Button | ||
| className="bg-slate-200 hover:bg-slate-300 text-slate-800" | ||
| disabled={isProcessing} | ||
| disabled={!canCommit} | ||
| type="submit" | ||
| > | ||
| {processingAction === "merge" ? ( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is impossible for me to understand for me the correctness of this component now, there are 3 different
useStateplus 8 differentuseRef, massiveuseEffectwith all kinds of asynchrnous ping-pong, and no comments. Even Claude is struggling to follow and tell me what it's doing, and only the tests are difficult to understand.I think this requires extracting the logic parts into a hook, for example, a
useCommitMessageDraft(changeMap)hook returning{ subject, body, status, setSubject, reset }that makes all the commit message dance testable and specifiable in separately from the UI. It should also be documented.