refactor!: rewrite ck-cli in TypeScript on Bun#15
Conversation
Full rewrite of the CLI from Go to TypeScript on the Bun runtime. The parser is byte-identical against the original Go binary's output across all fixtures (en, zh, other, rare, ric). Stack: - Bun + TypeScript strict mode (ES2022) - cac for arg parsing, ink for stderr-rendered status/progress UI - smol-toml for ~/.ck-cli.toml - native fetch for GraphQL with chunked uploads (20/chunk, 10 concurrent) - bun:test for tests, oxlint + oxfmt for lint/format - bun build --compile produces self-contained binaries for linux/darwin/windows x amd64/arm64 (skipping windows-arm64 to match the prior goreleaser config) Two intentional behavior corrections vs the Go binary: - parse --output http now actually uploads to GraphQL (the Go syncToServer had a TODO and only printed success) - parse with no --input + TTY stdin now exits with help instead of hanging on a stdin read Release pipeline (workflow YAML pushed separately): - release-please switched from release-type: go to release-type: node - Quill (Anchore) is retained for macOS sign + notarize; reuses the existing QUILL_* secrets unchanged - A Docker image is now built and pushed to GHCR on release Co-Authored-By: Claude <[email protected]>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @AnnatarHe's task —— View job Code Review Complete ✅I've conducted a comprehensive review of this Go → TypeScript rewrite. Here's my analysis: Review Tasks
🎯 Overall Assessment: EXCELLENTThis is a high-quality rewrite that maintains backward compatibility while modernizing the entire stack. The code is well-structured, thoroughly tested, and follows TypeScript best practices. ✅ StrengthsArchitecture & Code Quality
Parser Implementation
Test Coverage
Security & Configuration
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the ck-cli tool from Go to TypeScript using the Bun runtime, introducing a new project structure, build pipeline, and Ink-based UI. The core logic for parsing Kindle clippings and syncing them via GraphQL has been rewritten, with comprehensive tests added to ensure parity with the original implementation. Review feedback identifies potential issues with the brittleness of Chinese date parsing and the limitation of only extracting single-line highlights. Additionally, it is suggested to adopt a stream-based parsing approach to improve memory efficiency when handling large clipping files.
| let s = dateStr; | ||
| const ampm = s.includes("上午") ? "AM" : "PM"; | ||
| s = s.replace(cjkPattern, "-"); | ||
| s = s.replace(multiDashPattern, ""); | ||
| s = s.trim(); | ||
| s = `${s} ${ampm}`; | ||
| return parseShortDateAmpm(s); | ||
| } |
There was a problem hiding this comment.
The parseChineseDate logic might fail if there is no space between the weekday and the AM/PM indicator (or time). The current implementation relies on parseShortDateAmpm which splits by the first space. If the normalization process removes all spaces or if the input format varies slightly, spaceIdx will be -1 and an error will be thrown.
Additionally, the multiDashPattern replacement might be too aggressive. If the input is 2024年4月1日星期一 下午2:30:45, the cjkPattern replacement results in 2024-4-1--- --2:30:45. multiDashPattern (which is /-{2,}/g) will replace --- and -- with empty strings, resulting in 2024-4-1 2:30:45. This happens to work for the space-separated format, but it's brittle. A more robust approach would be to normalize to a standard format using a single regex or a more predictable replacement strategy.
| function parseGroup(group: string[], language: Language): ClippingItem | null { | ||
| if (group.length < 4) { | ||
| return null; | ||
| } | ||
|
|
||
| const titleLine = (group[0] ?? "").replaceAll(BOM, ""); | ||
| const title = parseTitle(titleLine); | ||
| if (title === "") { | ||
| return null; | ||
| } | ||
|
|
||
| const infoLine = group[1] ?? ""; | ||
| const info = parseInfo(infoLine, language); | ||
| if (info === null) { | ||
| return null; | ||
| } | ||
|
|
||
| const content = (group[3] ?? "").trim(); | ||
| if (content === "") { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| title, | ||
| content, | ||
| pageAt: info.location, | ||
| createdAt: info.createdAt, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The parseGroup function only extracts the first line of content (group[3]). Kindle clippings can contain multi-line highlights. While the PR description mentions byte-parity with the Go version, if the Go version also had this limitation, it might be worth documenting it as a known limitation or improving it here if parity allows for including all lines from index 3 onwards until the end of the group.
References
- Ensure code functionality handles edge cases like multi-line content in clippings.
| async function readInput(inputPath: string): Promise<string> { | ||
| if (inputPath === "") { | ||
| if (process.stdin.isTTY) { | ||
| throw new TTYError( | ||
| "No --input file provided and stdin is a TTY. Pipe data in or pass --input PATH.", | ||
| ); | ||
| } | ||
| return await Bun.stdin.text(); | ||
| } | ||
| return await readFile(inputPath, "utf8"); | ||
| } |
There was a problem hiding this comment.
Reading the entire input from Bun.stdin.text() or readFile into a string can lead to high memory usage for very large clipping files. Although Kindle clipping files are typically small, a more scalable approach would be to use a stream-based parser to process the file line by line or group by group.
2 participants
Summary
Full rewrite of
ck-clifrom Go to TypeScript on the Bun runtime.The parser produces byte-identical output to the existing Go binary across every fixture in
tests/fixtures/clippings_*.txt— verified bytests/fixtures.test.ts(en,zh,other,rare,ric, ~17K lines of oracle JSON).Stack
cacink(rendered to stderr only, stdout reserved for JSON)smol-tomlfetchbun:testoxlintoxfmtbun build --compile --target=bun-<os>-<arch>QUILL_*secrets reused)release-please(switchedrelease-typefromgo→node)Two intentional behavior changes vs. the Go binary
parse --output httpnow actually uploads to GraphQL. The GosyncToServerhad a TODO and just printed success without invoking the (fully-implemented) HTTP client.parsewith no--inputwhile stdin is a TTY now prints help and exits 1 instead of hanging on a stdin read.Everything else is preserved: command surface, flag names, config file format (
~/.ck-cli.toml), GraphQL mutation shape, chunk size (20), concurrency cap (10), 30s request timeout, RFC3339 date format,X-CLI <token>auth header.Layout
Verified locally
bun test— 29 tests pass (4 files, 52 assertions)bun run lint— 0 errorsbun run format:check— cleanbun run typecheck— 0 errorsbun run build:all -- --archive— all 5 targets compile, archive, and produce achecksums.txt./ck-cli parse --input tests/fixtures/clippings_*.txtis byte-identical to the Go binary's output on every fixture./ck-cli loginround-trip writes a TOML in the exact same[http]/[http.headers]shape that the Go binary writesBinary sizes (Bun embeds its runtime; expected to be ~20× larger than Go):
The GitHub App used by Claude Code in this environment lacks the
workflowspermission, so.github/workflows/ci.ymland.github/workflows/release.ymlcould not be updated by this branch. They still reference the Go toolchain onmasterand will fail CI until updated manually.Please apply the new workflow files below before merging (or grant the bot
workflowspermission and re-push).Replacement for
.github/workflows/ci.ymlReplacement for
.github/workflows/release.ymlTest plan
bun installbun test— confirm 29 passbun run build— confirm local binary works (./ck-cli --version)./ck-cli parse --input tests/fixtures/clippings_en.txt | diff - tests/fixtures/clippings_en.result.json(expect no output)workflowspermission)Generated by Claude Code