fix logger and redeem admin review findings
This commit is contained in:
119
docs/reviews/2026-04-20-code-review-report.md
Normal file
119
docs/reviews/2026-04-20-code-review-report.md
Normal file
@@ -0,0 +1,119 @@
|
||||
# 2026-04-20 Code Review Report
|
||||
|
||||
## Scope
|
||||
|
||||
本次 review 以代码实现和真实验证结果为主,不以停用业务说明或文档状态作为主判断依据。重点覆盖:
|
||||
|
||||
- 后端 Go 代码实现与测试执行结果
|
||||
- 前端 Vue/TypeScript 实现与质量门禁结果
|
||||
- 高风险管理操作的真实代码路径
|
||||
|
||||
## Validation Summary
|
||||
|
||||
已执行的验证:
|
||||
|
||||
- `backend`: `go test ./...`
|
||||
- `backend`: 定向复现 `internal/pkg/logger` 相关测试
|
||||
- `frontend`: `npm run test:run`
|
||||
- `frontend`: `npm run typecheck`
|
||||
- `frontend`: `npm run lint:check`
|
||||
|
||||
验证结论:
|
||||
|
||||
- 后端除 `internal/pkg/logger` 外,其余项目包测试通过
|
||||
- 前端 `vitest` 59 个测试文件、354 个测试全部通过
|
||||
- 前端 `typecheck` 失败
|
||||
- 前端 `lint:check` 失败
|
||||
|
||||
## Findings
|
||||
|
||||
### 1. High: Windows 下 logger `Sync()` 在标准输出被 pipe 接管时会卡死
|
||||
|
||||
证据:
|
||||
|
||||
- [logger.go#L199](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L199)
|
||||
- [logger.go#L274](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L274)
|
||||
- [logger.go#L275](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L275)
|
||||
- [logger.go#L404](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L404)
|
||||
- [logger_test.go#L12](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L12)
|
||||
- [logger_test.go#L60](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L60)
|
||||
- [logger_test.go#L132](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L132)
|
||||
- [logger_test.go#L169](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L169)
|
||||
|
||||
实际验证中,`go test ./...` 在 `internal/pkg/logger` 失败,`TestInit_DualOutput` 超时 10 分钟,阻塞栈停在 `FlushFileBuffers -> zap Sync -> logger.Sync()`。这说明当前实现对 Windows pipe 场景不安全,不只是测试代码问题。
|
||||
|
||||
### 2. High: “删除全部未使用兑换码”只删除前 1000 条
|
||||
|
||||
证据:
|
||||
|
||||
- [RedeemView.vue#L737](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L737)
|
||||
- [RedeemView.vue#L740](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L740)
|
||||
- [RedeemView.vue#L749](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L749)
|
||||
- [RedeemView.vue#L750](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L750)
|
||||
|
||||
当前实现固定只拉取第 1 页、每页 1000 条未使用兑换码,然后直接批量删除并提示成功。当未使用兑换码总数大于 1000 时,剩余数据会被静默遗漏。
|
||||
|
||||
### 3. Medium-High: 兑换码生成非事务化,叠加幂等重试后存在“部分成功再放大”风险
|
||||
|
||||
证据:
|
||||
|
||||
- [admin_service.go#L2056](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2056)
|
||||
- [admin_service.go#L2072](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2072)
|
||||
- [admin_service.go#L2092](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2092)
|
||||
- [redeem_handler.go#L103](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L103)
|
||||
- [idempotency.go#L406](/D:/project/sub2api-merge/backend/internal/service/idempotency.go#L406)
|
||||
- [idempotency.go#L410](/D:/project/sub2api-merge/backend/internal/service/idempotency.go#L410)
|
||||
|
||||
服务层逐条 `Create`,中间任一点失败都会提前返回;幂等层则把执行错误记为 `failed_retryable`。如果请求已经成功插入部分兑换码,再按同一个 key 重试,存在超量生成的可能。
|
||||
|
||||
### 4. Medium: 兑换码批量删除会吞掉单条失败,并整体返回成功
|
||||
|
||||
证据:
|
||||
|
||||
- [admin_service.go#L2104](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2104)
|
||||
- [admin_service.go#L2107](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2107)
|
||||
- [admin_service.go#L2111](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2111)
|
||||
- [redeem_handler.go#L243](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L243)
|
||||
- [redeem_handler.go#L252](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L252)
|
||||
- [redeem_handler.go#L253](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L253)
|
||||
- [admin_service_delete_test.go#L534](/D:/project/sub2api-merge/backend/internal/service/admin_service_delete_test.go#L534)
|
||||
- [admin_service_delete_test.go#L543](/D:/project/sub2api-merge/backend/internal/service/admin_service_delete_test.go#L543)
|
||||
|
||||
当前实现只累计成功删除数,失败项不会中断,也不会返回失败明细。更关键的是,测试已经把这种语义固化成预期行为,因此这是实现策略问题,不是单纯的漏测。
|
||||
|
||||
### 5. Medium: 前端测试全绿,但静态质量门禁仍然损坏
|
||||
|
||||
证据:
|
||||
|
||||
- [SoraAdminView.vue#L243](/D:/project/sub2api-merge/frontend/src/views/admin/SoraAdminView.vue#L243)
|
||||
- [SoraAdminView.vue#L259](/D:/project/sub2api-merge/frontend/src/views/admin/SoraAdminView.vue#L259)
|
||||
- [Icon.vue#L16](/D:/project/sub2api-merge/frontend/src/components/icons/Icon.vue#L16)
|
||||
- [Icon.vue#L17](/D:/project/sub2api-merge/frontend/src/components/icons/Icon.vue#L17)
|
||||
- [AppSidebar.spec.ts#L24](/D:/project/sub2api-merge/frontend/src/components/layout/__tests__/AppSidebar.spec.ts#L24)
|
||||
- [package.json#L8](/D:/project/sub2api-merge/frontend/package.json#L8)
|
||||
- [package.json#L11](/D:/project/sub2api-merge/frontend/package.json#L11)
|
||||
- [package.json#L12](/D:/project/sub2api-merge/frontend/package.json#L12)
|
||||
- [package.json#L14](/D:/project/sub2api-merge/frontend/package.json#L14)
|
||||
|
||||
`vitest` 全绿并不代表前端处于可交付状态。当前 `typecheck` 和 `lint:check` 都失败,说明测试集没有覆盖编译期约束与静态门禁。
|
||||
|
||||
### 6. Low: 仍有部分管理接口是占位实现
|
||||
|
||||
证据:
|
||||
|
||||
- [admin_service.go#L765](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L765)
|
||||
- [redeem_handler.go#L284](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L284)
|
||||
- [proxy_handler.go#L262](/D:/project/sub2api-merge/backend/internal/handler/admin/proxy_handler.go#L262)
|
||||
- [dashboard_handler.go#L180](/D:/project/sub2api-merge/backend/internal/handler/admin/dashboard_handler.go#L180)
|
||||
|
||||
这批接口目前主要是 mock/占位返回。它们不是这次最关键的问题,但如果继续对外暴露,建议明确收敛策略。
|
||||
|
||||
## Overall Assessment
|
||||
|
||||
本次代码库的主要问题集中在三类:
|
||||
|
||||
- Windows 环境下的日志稳定性
|
||||
- 管理端兑换码批量操作的语义正确性
|
||||
- 前端“测试通过但质量门禁失败”的交付断层
|
||||
|
||||
其中,日志 `Sync()` 卡死和兑换码批量操作缺陷建议优先处理。
|
||||
106
docs/reviews/2026-04-20-code-review-task-list.md
Normal file
106
docs/reviews/2026-04-20-code-review-task-list.md
Normal file
@@ -0,0 +1,106 @@
|
||||
# 2026-04-20 Code Review Task List
|
||||
|
||||
## P0
|
||||
|
||||
- 修复 Windows 下 `logger.Sync()` 对 `stdout/stderr` pipe 的阻塞问题
|
||||
- 为 `internal/pkg/logger` 补充 Windows 兼容回归测试,确保 `go test ./...` 可稳定通过
|
||||
- 重新执行后端全量测试,确认 `internal/pkg/logger` 修复后无新增失败
|
||||
|
||||
## P1
|
||||
|
||||
- 修复管理端“删除全部未使用兑换码”只处理前 1000 条的问题
|
||||
- 明确该功能应采用分页循环删除、后端专用批量接口,还是服务端单点清理命令
|
||||
- 为该流程补充前端测试,覆盖“超过 1000 条未使用兑换码”场景
|
||||
|
||||
- 修复 `GenerateRedeemCodes` 的非事务性问题
|
||||
- 明确幂等语义: 部分成功后应回滚、应返回已生成结果,还是应禁止重放生成
|
||||
- 为兑换码生成补充失败注入测试,覆盖“部分创建成功 + 重试”路径
|
||||
|
||||
- 调整 `BatchDeleteRedeemCodes` 返回语义
|
||||
- 明确接口是否应该失败即整体失败,或返回 `deleted` / `failed_ids` / `errors`
|
||||
- 同步更新 handler、前端提示文案和现有测试断言
|
||||
|
||||
## P2
|
||||
|
||||
- 修复前端 `typecheck` 失败
|
||||
- 处理 [SoraAdminView.vue](/D:/project/sub2api-merge/frontend/src/views/admin/SoraAdminView.vue) 中无效的 `Icon` 名称,或将停用页面排除出编译链
|
||||
|
||||
- 修复前端 `lint:check` 失败
|
||||
- 处理 [AppSidebar.spec.ts](/D:/project/sub2api-merge/frontend/src/components/layout/__tests__/AppSidebar.spec.ts) 中的 `no-regex-spaces` 问题
|
||||
- 清理当前 lint warning,避免门禁长期失真
|
||||
|
||||
## P3
|
||||
|
||||
- 盘点仍保留 mock 返回的管理接口
|
||||
- 对每个接口确定策略: 实现、废弃、隐藏入口,或从路由移除
|
||||
- 为仍需保留的接口补充契约测试,避免占位实现长期留在主分支
|
||||
|
||||
## Recommended Execution Order
|
||||
|
||||
1. 先修复 `internal/pkg/logger`,恢复后端全量测试可信度
|
||||
2. 再修复兑换码两个批量操作问题,避免管理端误操作
|
||||
3. 然后恢复前端 `typecheck` 和 `lint`
|
||||
4. 最后清理低优先级占位接口
|
||||
|
||||
## Assignable Work Packages
|
||||
|
||||
### Backend
|
||||
|
||||
- `BE-LOGGER-001`
|
||||
目标: 修复 Windows 下 `logger.Sync()` 对 pipe `stdout/stderr` 的阻塞
|
||||
交付物: `logger.go` 修复、回归测试、`go test ./internal/pkg/logger` 通过
|
||||
验收标准: `TestInit_DualOutput` 和 `TestInit_CallerShouldPointToCallsite` 不再超时
|
||||
|
||||
- `BE-REDEEM-001`
|
||||
目标: 将兑换码生成从“逐条创建 + 部分成功可重试”改为可证明安全的语义
|
||||
交付物: 服务层实现调整、失败注入测试、幂等路径验证
|
||||
验收标准: 不存在“部分成功后同 key 重试超量生成”的路径
|
||||
|
||||
- `BE-REDEEM-002`
|
||||
目标: 重构批量删除返回语义,明确成功、失败和跳过项
|
||||
交付物: 服务层返回结构、handler 响应调整、测试同步
|
||||
验收标准: 部分失败时 API 不再伪装为“完全成功”
|
||||
|
||||
### Frontend
|
||||
|
||||
- `FE-REDEEM-001`
|
||||
目标: 修复“删除全部未使用兑换码”只删前 1000 条的问题
|
||||
交付物: `RedeemView.vue` 实现调整、视图测试
|
||||
验收标准: 超过 1000 条未使用兑换码时仍能完整删除或明确失败
|
||||
|
||||
- `FE-REDEEM-002`
|
||||
目标: 对齐新的批量删除 API 语义和用户提示
|
||||
交付物: API 类型、页面提示、异常和部分成功文案
|
||||
验收标准: 用户能看到准确的删除结果
|
||||
|
||||
- `FE-QUALITY-001`
|
||||
目标: 恢复 `typecheck`
|
||||
交付物: 修复无效 `Icon` 名称或将停用页移出编译链
|
||||
验收标准: `npm run typecheck` 通过
|
||||
|
||||
- `FE-QUALITY-002`
|
||||
目标: 恢复 `lint:check`
|
||||
交付物: 修复 `AppSidebar.spec.ts` 正则问题并清理 warning
|
||||
验收标准: `npm run lint:check` 通过
|
||||
|
||||
### Testing
|
||||
|
||||
- `TEST-BE-001`
|
||||
目标: 为 logger Windows 行为建立稳定回归测试
|
||||
交付物: 针对 `Sync()`、stdout/stderr pipe、caller 输出的测试
|
||||
验收标准: 可在 Windows 环境稳定执行,不依赖 10 分钟超时
|
||||
|
||||
- `TEST-BE-002`
|
||||
目标: 为兑换码批量生成/删除建立失败路径覆盖
|
||||
交付物: 服务层和 handler 层测试
|
||||
验收标准: 覆盖部分成功、失败回滚、结果透出
|
||||
|
||||
- `TEST-FE-001`
|
||||
目标: 为 `RedeemView` 补足批量删除流程测试
|
||||
交付物: 视图级测试,覆盖分页循环和部分失败提示
|
||||
验收标准: 批量删除流程不再依赖人工回归
|
||||
|
||||
- `TEST-FULL-001`
|
||||
目标: 恢复全链路质量门禁
|
||||
交付物: 后端 `go test ./...`、前端 `npm run test:run`、`npm run typecheck`、`npm run lint:check`
|
||||
验收标准: 四项全部通过
|
||||
Reference in New Issue
Block a user