fix: Align flip should both work #599
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #599 +/- ##
==========================================
+ Coverage 96.40% 96.42% +0.01%
==========================================
Files 17 17
Lines 947 950 +3
Branches 279 279
==========================================
+ Hits 913 916 +3
Misses 34 34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
工作总结src/hooks/useAlign.ts 和 src/util.ts 中的对齐逻辑进行了内部重构。reversePoints 函数改为返回 Points 元组而非字符串，引入 flatPoints 辅助函数进行最终转换。nextAlignInfo.points 的赋值改为通过临时 nextPoints 数组进行，使用元组形式处理多种翻转情况（bt、tb、rl、lr），最终转换回字符串形式。isPointsEq 中添加 getVal 辅助函数以安全处理数组索引访问。 变更内容
预估代码审查工作量🎯 3 (中等) | ⏱️ ~25-35 分钟
建议审查人
诗文献礼
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)tests/align.test.tsx (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @zombieJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue in the alignment utility where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a bug where sequential horizontal and vertical alignment flips would not compose correctly. The fix involves refactoring reversePoints to return a Points array instead of a string, and introducing a nextPoints variable to track the state of the points through multiple flips. This is a clean and effective solution. A new test case has been added to validate the fix for combined flips. I've included one suggestion to improve the type safety of the reversePoints function.
| function reversePoints(points: Points, index: number): Points { | ||
| const reverseMap = { | ||
| t: 'b', | ||
| b: 't', | ||
| l: 'r', | ||
| r: 'l', | ||
| }; | ||
|
|
||
| return points | ||
| .map((point, i) => { | ||
| if (i === index) { | ||
| return reverseMap[point] || 'c'; | ||
| } | ||
| return point; | ||
| }) | ||
| .join(''); | ||
| const clone = [...points] as Points; | ||
| clone[index] = reverseMap[points[index]] || 'c'; | ||
| return clone; | ||
| } |
There was a problem hiding this comment.
While this implementation is more efficient and correctly enables chained flips, it relies on a type assertion (as Points) and the structure of reverseMap to maintain type safety, which might not be immediately obvious. The assignment clone[index] = reverseMap[points[index]] || 'c' is not strictly type-safe because TypeScript can't guarantee that (for example) a AlignPointLeftRight value isn't assigned to clone[0].
Consider a slightly more verbose but type-safe implementation that makes the logic clearer and safer by separating the vertical and horizontal reversal logic. This avoids potential issues if Points or the reversal logic were to change in the future.
| function reversePoints(points: Points, index: number): Points { | |
| const reverseMap = { | |
| t: 'b', | |
| b: 't', | |
| l: 'r', | |
| r: 'l', | |
| }; | |
| return points | |
| .map((point, i) => { | |
| if (i === index) { | |
| return reverseMap[point] || 'c'; | |
| } | |
| return point; | |
| }) | |
| .join(''); | |
| const clone = [...points] as Points; | |
| clone[index] = reverseMap[points[index]] || 'c'; | |
| return clone; | |
| } | |
| function reversePoints(points: Points, index: number): Points { | |
| const clone: Points = [...points]; | |
| if (index === 0) { | |
| const verticalMap: Record<AlignPointTopBottom, AlignPointTopBottom> = { t: 'b', b: 't', c: 'c' }; | |
| clone[0] = verticalMap[clone[0]]; | |
| } else { | |
| const horizontalMap: Record<AlignPointLeftRight, AlignPointLeftRight> = { l: 'r', r: 'l', c: 'c' }; | |
| clone[1] = horizontalMap[clone[1]]; | |
| } | |
| return clone; | |
| } |
Summary by CodeRabbit
发版说明
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.