Skip to content

Commit ee4920b

Browse files
committed
fix(feedback): bug bash fixes — wizard exit, dir error, tilde paths
Three issues from bug bash: - 2.5: Esc/Ctrl+C in the wizard didn't terminate the process. The onExit callback unmounted the screen but Ink's stdin raw-mode listeners kept Node alive. Fix: await render()'s waitUntilExit() and call process.exit(0) once the wizard unmounts. Matches invoke/command.tsx's pattern. - 3.2: Pointing --screenshot at a directory ("/tmp") produced "Screenshot must be one of: .png, .jpg, .jpeg" because we extension- checked first. Fix: stat the path first; surface a directory-specific error before the extension check. - 3.6: Quoted paths starting with "~/" (e.g. ~/Desktop/foo.png) hit ENOENT because Node's fs APIs don't expand tildes — only the shell does, and quoting suppresses that. Fix: expand a leading "~" or "~/..." to os.homedir() before stat/read. Added unit tests for both new validator branches.
1 parent 1f387d7 commit ee4920b

4 files changed

Lines changed: 73 additions & 6 deletions

File tree

src/cli/commands/feedback/__tests__/command.test.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,11 @@ vi.mock('../../../tui/screens/feedback', () => ({
2424
vi.mock('ink', () => ({
2525
render: (...args: unknown[]) => {
2626
mockRender(...args);
27-
return { clear: vi.fn(), unmount: vi.fn() };
27+
return {
28+
clear: vi.fn(),
29+
unmount: vi.fn(),
30+
waitUntilExit: () => Promise.resolve(),
31+
};
2832
},
2933
Text: 'Text',
3034
Box: 'Box',
@@ -146,11 +150,14 @@ describe('registerFeedback', () => {
146150
expect(mockHandleFeedback).not.toHaveBeenCalled();
147151
});
148152

149-
it('hands off to the TUI when no message argument is provided', async () => {
150-
await program.parseAsync(['feedback'], { from: 'user' });
153+
it('hands off to the TUI when no message argument is provided, then exits cleanly', async () => {
154+
await expect(program.parseAsync(['feedback'], { from: 'user' })).rejects.toThrow('process.exit');
151155

152156
expect(mockRequireTTY).toHaveBeenCalled();
153157
expect(mockRender).toHaveBeenCalled();
154158
expect(mockHandleFeedback).not.toHaveBeenCalled();
159+
// After the wizard unmounts we must terminate the Node process; otherwise
160+
// Ink's stdin raw-mode listeners keep the process alive.
161+
expect(mockExit).toHaveBeenCalledWith(0);
155162
});
156163
});

src/cli/commands/feedback/command.tsx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ export const registerFeedback = (program: Command) => {
2424
return;
2525
}
2626
requireTTY();
27-
const { clear, unmount } = render(
27+
const { clear, unmount, waitUntilExit } = render(
2828
<FeedbackScreen
2929
initialScreenshot={options.screenshot}
3030
onExit={() => {
@@ -33,7 +33,10 @@ export const registerFeedback = (program: Command) => {
3333
}}
3434
/>
3535
);
36-
return;
36+
// Wait for the wizard to unmount, then exit. Without this Node sticks
37+
// around because of Ink's stdin raw-mode listeners.
38+
await waitUntilExit();
39+
process.exit(0);
3740
}
3841

3942
const has_screenshot = !!options.screenshot;

src/cli/operations/feedback/__tests__/submit-feedback.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,33 @@ describe('submitFeedback', () => {
147147
expect(fetchMock).not.toHaveBeenCalled();
148148
});
149149

150+
it('rejects a directory with a directory-specific error, not the extension error', async () => {
151+
const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'feedback-test-'));
152+
await expect(submitFeedback({ message: 'msg', screenshot: { path: tmp } })).rejects.toThrow(
153+
/is a directory, not a file/
154+
);
155+
expect(fetchMock).not.toHaveBeenCalled();
156+
await fs.rm(tmp, { recursive: true, force: true });
157+
});
158+
159+
it('expands a leading tilde so quoted paths like "~/file.png" resolve', async () => {
160+
// Drop a real file in $HOME so tilde-expansion has somewhere to land
161+
const fileName = `feedback-tilde-${Date.now()}.png`;
162+
const realPath = path.join(os.homedir(), fileName);
163+
await fs.writeFile(realPath, Buffer.from([0x89, 0x50, 0x4e, 0x47]));
164+
165+
fetchMock
166+
.mockResolvedValueOnce(new Response('https://s3.example/key?sig=x', { status: 200 }))
167+
.mockResolvedValueOnce(emptyOk(200))
168+
.mockResolvedValueOnce(jsonResponse(successResponse));
169+
170+
const result = await submitFeedback({ message: 'tilde', screenshot: { path: `~/${fileName}` } });
171+
expect(result.id).toBe('submission-123');
172+
expect(fetchMock).toHaveBeenCalledTimes(3);
173+
174+
await fs.rm(realPath, { force: true });
175+
});
176+
150177
it('maps Aperture 412 responses to a missing-headers error', async () => {
151178
fetchMock.mockResolvedValueOnce(new Response('missing headers', { status: 412 }));
152179

src/cli/operations/feedback/submit-feedback.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
import type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types';
1212
import { createHash } from 'node:crypto';
1313
import * as fs from 'node:fs/promises';
14+
import * as os from 'node:os';
1415
import * as path from 'node:path';
1516

1617
export class FeedbackValidationError extends Error {
@@ -73,7 +74,36 @@ interface LoadedScreenshot {
7374
size: number;
7475
}
7576

76-
async function loadAndValidateScreenshot(filePath: string): Promise<LoadedScreenshot> {
77+
/**
78+
* Expand a leading `~` or `~/...` to the user's home directory. Node's fs APIs
79+
* don't expand tildes — the shell normally does — so users who quote a path
80+
* like `"~/Desktop/foo.png"` to preserve spaces hit ENOENT. This handles that.
81+
*/
82+
function expandTilde(filePath: string): string {
83+
if (filePath === '~') return os.homedir();
84+
if (filePath.startsWith('~/')) return path.join(os.homedir(), filePath.slice(2));
85+
return filePath;
86+
}
87+
88+
async function loadAndValidateScreenshot(rawFilePath: string): Promise<LoadedScreenshot> {
89+
const filePath = expandTilde(rawFilePath);
90+
91+
// Stat the path first so we can give a precise error for directories or
92+
// missing files, rather than letting the extension check mask them.
93+
let stat: Awaited<ReturnType<typeof fs.stat>>;
94+
try {
95+
stat = await fs.stat(filePath);
96+
} catch (err) {
97+
const reason = err instanceof Error ? err.message : String(err);
98+
throw new FeedbackValidationError(`Could not read screenshot at ${filePath}: ${reason}`);
99+
}
100+
if (stat.isDirectory()) {
101+
throw new FeedbackValidationError(`Screenshot path is a directory, not a file: ${filePath}`);
102+
}
103+
if (!stat.isFile()) {
104+
throw new FeedbackValidationError(`Screenshot path is not a regular file: ${filePath}`);
105+
}
106+
77107
const ext = path.extname(filePath).toLowerCase();
78108
if (!ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number])) {
79109
throw new FeedbackValidationError(`Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(', ')}.`);

0 commit comments

Comments
 (0)