fix(CL router): cl path dismatch#1905
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This pull request addresses a path mismatch issue in the code review feature where the frontend receives long paths from the API but the CL directory tree uses short paths. The fix modifies the cl_files_list router endpoint to return short paths instead of concatenating the CL base path with file paths.
Changes:
- Removes unused
std::path::PathBufimport - Simplifies the path handling in
cl_files_listendpoint by returning short paths directly instead of joining withcl.path
| .map(|m| { | ||
| let mut item: ClFilesRes = m.into(); | ||
| item.path = cl_base.join(item.path).to_string_lossy().to_string(); | ||
| let item: ClFilesRes = m.into(); |
There was a problem hiding this comment.
The PR description states "this interface is not being used" (目前这个接口没有被使用), but the endpoint is actually being used in the frontend code. The useGetClFilesList hook is imported and used in moon/apps/web/components/DiffView/comment/CommentForm.tsx to retrieve commit SHA for code review comments.
Before making this breaking change, verify that returning short paths instead of long paths won't break the path matching logic in CommentForm.tsx line 46: file.path.endsWith(filePath). The frontend may need to be updated to handle this change.
| .map(|m| { | ||
| let mut item: ClFilesRes = m.into(); | ||
| item.path = cl_base.join(item.path).to_string_lossy().to_string(); | ||
| let item: ClFilesRes = m.into(); |
There was a problem hiding this comment.
There's an inconsistency in the codebase: ceres/src/build_trigger/changes_calculator.rs lines 47-56 still joins the CL path with file paths when creating ClFilesRes objects (using cl_base.join(item.path)), while this PR removes the same logic from the API router. This creates an inconsistency where different code paths return different path formats for the same data structure.
Consider whether the build_changes function in changes_calculator.rs should also be updated to match this new behavior, or if there's a reason for the different path handling in different contexts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afa3201eb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| let res = cl_diff_files | ||
| .into_iter() | ||
| .map(|m| { | ||
| let mut item: ClFilesRes = m.into(); | ||
| item.path = cl_base.join(item.path).to_string_lossy().to_string(); | ||
| let item: ClFilesRes = m.into(); | ||
| item |
There was a problem hiding this comment.
Preserve CL base in files-list paths
The files-list response now returns raw diff paths without prefixing them with the CL base path. Downstream consumers (e.g., scorpio/src/daemon/antares.rs relative_path_for_mount) normalize each entry as an absolute path and require it to start with the mount path; if the CL base prefix is missing, relative_path_for_mount returns None and the file is skipped, which results in missing CL layer contents for non-root mounts. This regression appears when mount_path is anything other than / and the CL is scoped to a subdirectory, because the returned paths no longer include that subdirectory.
Useful? React with 👍 / 👎.
问题
code review 进行评论的时候需要获取到 commit sha。目前前端能从接口获取到的是长路径,但是cl目录树这里是短路径
路径匹配不太友好 。
解决方案
cl_files_listrouter 返回短路径。目前这个接口没有被使用,不用担心代码改动影响其他逻辑