[NoQA] Add post-edit formatting hooks to claude#81999
Conversation
|
CC @mountiny |
|
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bf60672e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "jq -r '.tool_input.file_path' | xargs npx prettier --write --ignore-unknown" |
There was a problem hiding this comment.
Preserve whitespace in edited file paths
The hook command pipes .tool_input.file_path into plain xargs, which splits on whitespace, so any edited path containing spaces is broken into multiple arguments and prettier exits non-zero. This is reproducible in this repo with files like docs/articles/expensify-classic/expenses/Export- Expenses-from-the-Expenses-Page.md, where the hook reports “No files matching the pattern” for split fragments; that makes post-edit formatting fail for valid files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@kacper-mikolajczak interesting find, is this valid?
There was a problem hiding this comment.
I've checked and that indeed could potentially fail - there is only this specific file accessible by prettier that triggers the edge case, nice catch Codex!
Fixed the bug by using command substitution wrapped with quotes.
Also, I feel like this is a typo in Export- Expenses-from-the-Expenses-Page.md name :D
Julesssss
left a comment
There was a problem hiding this comment.
Thanks for change. How fast does this work locally?
e771941
@Julesssss The timings were under 1 second regardless of tested file size. I skipped running linter because of the time it took to complete
|
|
Was that closed intentionally? I couldn't find changes on main. @roryabraham |
|
Updated version of the PR: #82050
|
|
Sounds good, will check out the pr |


Explanation of Change
Fixed Issues
$ #81969
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari