Conversation
Greptile SummaryThis PR implements the long-requested ergonomic default for bare Key changes:
Confidence Score: 5/5Safe to merge — all remaining findings are minor style/consistency suggestions that do not affect runtime behaviour. The logic is correct across all four bare-invocation paths and is well-covered by new tests. Both flagged items are P2: a redundant branch that compiles and runs identically, and a dep-injection inconsistency that doesn't affect any current test or production path. No correctness, security, or data-integrity issues were found. src/core/startup.ts — minor redundant branch and one direct call to Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[argv] --> B[parseCli]
B --> C{kind}
C -- "--help / -h / help" --> D[kind: help]
C -- "no command" --> E[kind: bare]
C -- "diff / show / patch / ..." --> F[CliInput]
C -- pager --> G[kind: pager]
D --> H[StartupPlan: help]
F --> N[resolveRuntime -> resolveConfig -> loadBootstrap]
E --> I{stdinIsTTY?}
I -- yes --> J["appCliInput = git, staged:false"]
I -- no --> K[appCliInput = pager]
J --> N
G --> K
K --> L[readStdin]
L --> M{stdinText?}
M -- "empty + bare" --> H
M -- not patch --> O[StartupPlan: plain-text-pager]
M -- patch --> P["appCliInput = patch, pager:true"]
P --> N
N --> Q[StartupPlan: app]
Reviews (1): Last reviewed commit: "Default bare hunk to diff startup" | Re-trigger Greptile |
| } else if (parsedCliInput.kind === "pager") { | ||
| appCliInput = parsedCliInput; | ||
| } else { | ||
| appCliInput = parsedCliInput; | ||
| } |
There was a problem hiding this comment.
Redundant
else if and else branches
Both the else if (parsedCliInput.kind === "pager") and the final else branch do exactly the same thing (appCliInput = parsedCliInput). Since help, mcp-serve, session, and bare have all been handled above, the only remaining shapes are CliInput | PagerCommandInput, making the distinction between the two branches meaningless.
| } else if (parsedCliInput.kind === "pager") { | |
| appCliInput = parsedCliInput; | |
| } else { | |
| appCliInput = parsedCliInput; | |
| } | |
| } else { | |
| appCliInput = parsedCliInput; | |
| } |
| if (stdinText.length === 0 && bareInvocation) { | ||
| return { | ||
| kind: "help", | ||
| text: renderCliHelp(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
renderCliHelp bypasses dependency injection
Every other side-effectful operation in prepareStartupPlan is routed through an injectable dep (e.g. parseCliImpl, readStdinText). Here renderCliHelp() is called directly, so tests that inject a custom parseCliImpl returning { kind: "bare" } cannot control the help text produced by the bare + empty-stdin path. This is fine for the current test suite (the real renderCliHelp works fine), but it breaks the otherwise-consistent injection pattern. Consider adding an optional renderCliHelpImpl dep, or accepting a pre-rendered helpText string alongside the bare input.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
hunkto the interactivediffflow in TTY sessionsCloses #116.
Validation
bun run typecheckbun test test/cli.test.tsbun test test/startup.test.tsbun test test/help-output.test.tsbun run src/main.tsx -- --helpbun test(Bun crashed with a segmentation fault mid-suite; focused tests above passed)