Skip to content
Open
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ Example:
theme = "graphite" # graphite, midnight, paper, ember
mode = "auto" # auto, split, stack
vcs = "git" # git, jj
watch = false
exclude_untracked = false
line_numbers = true
wrap_lines = false
Expand Down
46 changes: 46 additions & 0 deletions src/core/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,52 @@ describe("config resolution", () => {
expect(fallbackResolved.input.options.excludeUntracked).toBe(false);
});

test("resolves watch from config unless CLI explicitly enables it", () => {
const cases: Array<{
config: string;
cliOptions: Partial<CliInput["options"]>;
expected: boolean;
}> = [
{
config: "watch = true\n",
cliOptions: {},
expected: true,
},
{
config: "watch = false\n",
cliOptions: {},
expected: false,
},
{
config: "",
cliOptions: {},
expected: false,
},
{
config: "watch = false\n",
cliOptions: { watch: true },
expected: true,
},
];

for (const entry of cases) {
const home = createTempDir("hunk-config-home-");
mkdirSync(join(home, ".config", "hunk"), { recursive: true });
writeFileSync(join(home, ".config", "hunk", "config.toml"), entry.config);

const resolved = resolveConfiguredCliInput(
{
kind: "vcs",
staged: false,
options: entry.cliOptions,
},
{ cwd: createTempDir("hunk-config-cwd-"), env: { HOME: home } },
);

expect(resolved.input.options.watch).toBe(entry.expected);
}
Comment on lines +192 to +207
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Loop-based parametric test hides later failures on early assertion error

The for loop means that if the first expect throws, Bun reports only one failure and skips the remaining three cases. Using test.each (or Bun's describe.each) would make each cell an independent test so all failing cases surface together — the same pattern would also be more consistent with how the other parametric tests in this project are typically structured.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/config.test.ts
Line: 192-207

Comment:
**Loop-based parametric test hides later failures on early assertion error**

The `for` loop means that if the first `expect` throws, Bun reports only one failure and skips the remaining three cases. Using `test.each` (or Bun's `describe.each`) would make each cell an independent test so all failing cases surface together — the same pattern would also be more consistent with how the other parametric tests in this project are typically structured.

How can I resolve this? If you propose a fix, please make it concise.

});
Comment on lines +164 to +208
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing test case: TOML watch=true with CLI watch=false

The resolution table covers four of the five meaningful cells but omits the case { config: "watch = true\n", cliOptions: { watch: false }, expected: false } — CLI explicitly suppressing a config-enabled watch. If CommonOptions.watch can legitimately be false rather than undefined (e.g. a future --no-watch flag or a programmatic caller that sets it explicitly), the final override expression input.options.watch ?? resolvedOptions.watch ?? false would return false correctly, but this is untested. Adding the case would lock in that behavior and make the intent of ?? vs && clear for future readers.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/config.test.ts
Line: 164-208

Comment:
**Missing test case: TOML watch=true with CLI watch=false**

The resolution table covers four of the five meaningful cells but omits the case `{ config: "watch = true\n", cliOptions: { watch: false }, expected: false }` — CLI explicitly suppressing a config-enabled watch. If `CommonOptions.watch` can legitimately be `false` rather than `undefined` (e.g. a future `--no-watch` flag or a programmatic caller that sets it explicitly), the final override expression `input.options.watch ?? resolvedOptions.watch ?? false` would return `false` correctly, but this is untested. Adding the case would lock in that behavior and make the intent of `??` vs `&&` clear for future readers.

How can I resolve this? If you propose a fix, please make it concise.


test("defaults to git VCS mode and accepts jj from config", () => {
const home = createTempDir("hunk-config-home-");
mkdirSync(join(home, ".config", "hunk"), { recursive: true });
Expand Down
3 changes: 2 additions & 1 deletion src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function readConfigPreferences(source: Record<string, unknown>): CommonOptions {
mode: normalizeLayoutMode(source.mode),
vcs: normalizeVcsMode(source.vcs),
theme: normalizeString(source.theme),
watch: normalizeBoolean(source.watch),
excludeUntracked: normalizeBoolean(source.exclude_untracked),
lineNumbers: normalizeBoolean(source.line_numbers),
wrapLines: normalizeBoolean(source.wrap_lines),
Expand Down Expand Up @@ -186,7 +187,7 @@ export function resolveConfiguredCliInput(
...resolvedOptions,
agentContext: input.options.agentContext,
pager: input.options.pager ?? false,
watch: input.options.watch ?? false,
watch: input.options.watch ?? resolvedOptions.watch ?? false,
excludeUntracked: resolvedOptions.excludeUntracked ?? false,
vcs: resolvedOptions.vcs ?? "git",
mode: resolvedOptions.mode ?? DEFAULT_VIEW_PREFERENCES.mode,
Expand Down