feat: merge open and draft interfaces#1958
Conversation
|
Claude finished @zyd123-cmd's task in 1m 44s —— View job Code Review
Summary: This PR simplifies the CL list frontend by removing the dual-fetch logic (separate calls for
|
| Area | Status |
|---|---|
| Code simplification | ✅ Good — logic significantly simplified |
| Bug: draft description missing | |
| Dead code cleanup | useDraftClList.ts not deleted |
| Type safety | any usage |
| Breaking type change | PostApiCreateEntryData |
| Tests | ℹ️ No tests added/modified |
The core logic change is sound. The main concerns are the missing draft case in getDescription (which would silently hide descriptions for draft CLs on the open tab) and verifying nothing is broken by the PostApiCreateEntryData type change.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR consolidates the frontend CL (Change List) fetching logic by merging the separate open and draft API calls into a single endpoint. The backend now returns both open and draft CLs when status='open' is requested, eliminating the need for dual API calls and complex race condition handling on the frontend.
Changes:
- Updated TypeScript generated types to include new API response fields (from_hash, repo_path, CreateEntryResult, EditCLMode, skip_build)
- Simplified ClView component by removing useDraftClList hook and the complex dual-fetch logic with race condition handling
- Changed API response type for PostApiCreateEntryData from CommonResultString to CommonResultCreateEntryResult
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| moon/packages/types/generated.ts | Added new fields to CompleteResponse and CommonResultCompleteResponse (from_hash, repo_path); introduced CreateEntryResult type and updated PostApiCreateEntryData response type; added EditCLMode and skip_build support for CreateEntryInfo; updated trigger type documentation |
| moon/apps/web/components/ClView/index.tsx | Removed useDraftClList hook import and usage; simplified fetchClListData to make single API call instead of dual open+draft calls; removed complex finalize() logic that handled race conditions between two parallel requests; updated useEffect dependency array |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [page, pageSize, status]) |
There was a problem hiding this comment.
The useEffect dependency array is missing fetchClListData which is used inside the effect. While you've disabled the exhaustive-deps lint rule, this creates a potential issue: if fetchClList changes (which is included in fetchClListData's dependencies), the effect won't re-run but fetchClListData will capture a stale closure of fetchClList.
Either:
- Add fetchClListData to the dependency array (recommended), or
- If you intentionally want to avoid re-running when fetchClList changes, document why with a comment explaining the reasoning
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [page, pageSize, status]) | |
| }, [fetchClListData]) |
| import { Pagination } from '../Issues/Pagenation' | ||
| import { clIdAtom } from '../Issues/utils/store' | ||
| import { useDraftClList } from './hook/useDraftClList' | ||
|
|
There was a problem hiding this comment.
The import statement for useDraftClList has been removed, but the hook file itself (moon/apps/web/components/ClView/hook/useDraftClList.ts) likely still exists in the codebase. Since this hook is no longer used anywhere (verified via search), the file should be deleted to avoid confusion and maintain a clean codebase.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
前端修改-只调用open状态的接口 后端会一起返回open和draft的cl数据