Skip to content

refactor: unify download utils and temp directories#132

Open
yujiawei wants to merge 1 commit intomainfrom
refactor/unify-download-utils
Open

refactor: unify download utils and temp directories#132
yujiawei wants to merge 1 commit intomainfrom
refactor/unify-download-utils

Conversation

@yujiawei
Copy link
Copy Markdown
Collaborator

Summary

  • 三个临时目录 (/tmp/dmwork-upload, /tmp/dmwork-media, /tmp/dmwork-files) 合并为 /tmp/dmwork-temp/{upload,media,files}
  • 三个下载函数的公共流式下载逻辑抽取到 src/temp-utils.tsstreamDownloadToFile(),支持 backpressure、size limit、HEAD 预检
  • cleanup 加节流:cleanupTempDir() 10 分钟内不重复扫同一目录
  • 纯重构,所有现有 export 函数签名和行为保持不变
  • 204 个测试全部通过(原 194 + 新增 10)

变更文件

文件 变更
src/temp-utils.ts 新增 — 统一常量、cleanupTempDirstreamDownloadToFile
src/temp-utils.test.ts 新增 — 10 个测试覆盖目录常量、节流、下载
src/channel.ts 删除 UPLOAD_TEMP_DIRdownloadToTempFile 实现、cleanupOldUploadTempFiles,改用 temp-utils
src/inbound.ts 删除 MEDIA_TEMP_DIRTEMP_DIRcleanupMediaTempFilescleanupTempFiles,三个下载函数改为调用 streamDownloadToFile 的 wrapper
src/inbound.test.ts 更新路径断言 /tmp/dmwork-media//tmp/dmwork-temp/media/

Test plan

  • npm test — 204 tests passed (8 test files)
  • 验证 inbound 媒体下载仍写入 /tmp/dmwork-temp/media/
  • 验证 outbound 上传仍写入 /tmp/dmwork-temp/upload/
  • 验证文件下载仍写入 /tmp/dmwork-temp/files/

🤖 Generated with Claude Code

- Consolidate three temp directories (/tmp/dmwork-upload, /tmp/dmwork-media,
  /tmp/dmwork-files) into /tmp/dmwork-temp/{upload,media,files}
- Extract common stream download logic into src/temp-utils.ts with
  streamDownloadToFile() supporting backpressure, size limits, and HEAD pre-check
- Add throttled cleanup (cleanupTempDir) — skips directory scan if called
  within 10 minutes of last run
- Existing function signatures and behavior unchanged:
  - downloadToTempFile returns {tempPath, contentType}
  - downloadMediaToLocal returns string|undefined (graceful degradation)
  - downloadToTemp throws on error

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

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

Good refactoring — consolidating three duplicate download+cleanup patterns into a shared utility reduces maintenance surface and makes behavior consistent. A few things worth addressing:

  1. Dropped signal parameter (channel.ts): downloadToTempFile still accepts signal?: AbortSignal but the new implementation ignores it entirely. Callers relying on abort signal for cancellation will find it non-functional. Either wire it through StreamDownloadOptions or remove the parameter to avoid a misleading API.

  2. HEAD response unchecked (temp-utils.ts:75): The HEAD pre-check does not validate head.ok — a 404/500 HEAD response yields contentLength = 0 and silently passes the size gate. Consider only applying the size check when head.ok is true, so a broken HEAD falls through to the GET request naturally.

  3. Fragile error message routing (inbound.ts downloadMediaToLocal): The catch block routes log levels by checking msg.includes("HTTP ") and msg.includes("File too large"). If streamDownloadToFile error message wording changes, these branches break silently. Consider using typed error subclasses or an error code field for reliable branching.

Overall this is a solid DRY improvement — the shared streamDownloadToFile with backpressure and throttled cleanup is well-structured. 👍

Copy link
Copy Markdown
Collaborator Author

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

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

Coda review ✅ — temp-utils.ts 抽取合理,streamDownloadToFile 统一三处下载逻辑,cleanup 节流设计好。CI 204 测试全过。等 TestBot 验证后合并。

@Jerry-Xin
Copy link
Copy Markdown
Collaborator

Thanks for confirming the overall approach looks good! Before merging, could you clarify whether the three items from the review are being tracked?

  1. signal parameter: downloadToTempFile still exposes signal?: AbortSignal in its signature, but the underlying streamDownloadToFile doesn't wire it through. This is a silent API contract break for any caller passing an abort signal. Do you plan to either plumb it through or remove the parameter?

  2. HEAD pre-check on non-OK responses: If the HEAD request returns 404/500, contentLength falls to 0 and the size gate is bypassed silently. A quick if (head.ok) guard around the size check would prevent this.

  3. String-based error routing in downloadMediaToLocal: The msg.includes("HTTP ") / msg.includes("File too large") branching will break silently if error messages change. Typed errors or an error code field would make this robust.

None of these are blocking if you've considered them and have a reason to defer — just want to make sure they don't slip through. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants