前端代码评审：把维护风险讲清楚，而不是挑语法
一条评审意见有没有用，很大程度取决于我能不能跟作者把「为什么要改」讲清楚。
如果我留的评论是「这里命名不太好」，作者大概率会礼貌地改一下变量名，然后我们都觉得完成了一次评审——可上线后真正出问题的往往不是这个变量名。反过来，如果我能说清「这个变量存的是筛选后的列表，但名字叫 list，下个人接手时很容易和原始列表混淆，导致改错数据源」，作者就算不完全同意，也能围绕这个具体风险来讨论。评审的价值不在于我看出了多少毛病，而在于我能把哪些毛病的维护代价讲给同事听。
我早年 review 做得很碎，看到命名不顺、缩进不齐就忍不住评。后来线上出过几次问题，才意识到真正该拦住的不是「这里能不能更优雅」，而是「接口失败后 loading 没恢复」「权限变了菜单没刷新」「提交按钮能重复点」。这些问题上线后才暴露，修复成本比改个变量名高太多。所以现在看 PR，我先按风险扫一遍，再看表达细节——顺序反过来，注意力很容易被小问题吃掉，而那些小问题恰恰是最好跟同事讲、也最不该占用评审注意力的部分。
先跟自己讲清楚：评审在替谁把关
跟同事解释一条意见之前，我得先想清楚评审到底在拦什么。它不是「能不能跑」的验收，而是在功能上线前，尽早发现后续的维护成本和行为风险。具体到能不能落成一句话跟作者说，我看的是：改动范围是否清楚、逻辑是否容易出错、后面的人能不能接手、有没有引入新复杂度、失败路径有没有处理、有没有破坏已有业务逻辑。
这里有条我常跟团队强调的原则：review 不是替作者重写代码。能交给自动化工具的，别靠评论反复纠正；需要上下文判断的，才值得人工介入。否则评审会变成消耗双方耐心的流程，作者也会把所有评论都当成挑刺。
状态流转：顺着用户操作走一遍再评
前端不少 bug 不出在渲染，而出在状态流转混乱。评审时我不看 happy path，而是顺着用户操作走一遍：进页面、改内容、提交、失败、重试、离开页面。这样才容易发现下面这类问题：一个状态被多处同时改、本地状态和服务端状态混在一起、提交流程和展示状态耦合、loading 和 disabled 不一致、请求失败后页面仍显示成功。
比如这段，我会直接指出风险在哪：
1async function submit() { 2 setSaving(true); 3 await updateProfile(form); 4 setSaving(false); 5 toast.success("保存成功"); 6}
跟作者讲的时候我不会说「这里写得不好」，而是说清后果：接口一旦失败，await 抛错，setSaving(false) 根本执行不到，按钮会永远卡在保存中，用户只能刷新页面。更稳的写法是把错误和收尾都兜住：
1async function submit() { 2 setSaving(true); 3 4 try { 5 await updateProfile(form); 6 toast.success("保存成功"); 7 } catch (error) { 8 toast.error(getErrorMessage(error)); 9 } finally { 10 setSaving(false); 11 } 12}
我还会特别看「成功状态什么时候出现」。有些代码接口还没真正返回成功，就先关弹窗、清表单、提示保存成功。网络慢一点或接口报错，用户看到的状态就和真实数据对不上。表单、支付、权限配置、排序保存这类操作，成功反馈一定要和服务端确认绑定——这句话我跟新同事讲过很多遍，因为它是最容易被「本地先乐观更新一下」带偏的地方。
顺带说一句这两年常被讨论的乐观更新（optimistic update）：不是不能先更 UI，而是必须配一套失败回滚。TanStack Query 的 onMutate/onError 就是干这个的，评审时如果看到有人手写乐观更新却没有回滚分支，这就是要拦的点。
组件边界：一个组件认识了太多业务
一个组件是不是承担了太多职责，是评审里非常值得看的信号。如果它同时负责请求接口、数据转换、展示渲染、弹窗控制、路由跳转和埋点，即使当前能跑，后面也很容易变成维护热点。跟作者讲的时候，我会挑几个具体信号说：props 数量一直在涨、组件里同时跑多个业务流程、一个改动需要理解整页所有状态、子组件反向修改父组件内部状态、UI 组件里混进大量接口细节。
组件不是拆得越碎越好，而是各自的职责要分明：展示组件关心样式和交互，业务容器关心请求和状态，中间用明确的数据结构连接。我也会盯「组件有没有偷偷扩大影响面」。比如一个通用表格组件为了某个页面加了 isOrderPage，短期能跑，长期就是坏味道。遇到这种我会给作者一个可执行的方向：改成插槽、render prop 或者在外层组合，而不是让通用组件认识具体业务。
这类职责划分的问题不好靠一句评论说服人，我通常会顺手写一小段对比伪代码，让作者看到「侵入式」和「组合式」两种写法后续维护的差别，比空讲原则有用。
异常路径：多问一句「这里失败会怎样」
很多代码正常路径下没问题，一旦接口失败、字段为空、数据延迟返回就出事。评审时我会对着可疑的地方多问一句：如果这里失败，会发生什么。常见的异常路径包括接口返回空数组、字段缺失、用户没权限、重复点击提交、上传文件超限、网络中断后重试、页面卸载时请求还没结束。
读接口字段时，不要默认后端永远返回完整结构：
1const avatar = user.profile.avatarUrl;
profile 一旦为空，这行直接让页面崩。更稳的是在数据转换这一层统一兜住，让展示层不用到处写防御代码：
1function normalizeUser(user) { 2 return { 3 id: user.id, 4 name: user.name || "未命名用户", 5 avatarUrl: user.profile?.avatarUrl || "/images/default-avatar.png", 6 }; 7}
统一 API 错误也值得在 review 里看。不要让每个页面各自解析一遍错误结构，否则后端错误格式一变，全站提示就不一致。请求层应该把错误收敛成稳定结构，页面只决定怎么展示：
1function normalizeApiError(error) { 2 return { 3 code: error?.response?.data?.code || 'API_ERROR', 4 message: error?.response?.data?.message || '请求失败，请稍后重试', 5 }; 6}
这两年我评审时还会多看一处异步的取消问题：组件卸载后异步回调仍去 setState，React 会警告，更麻烦的是竞态——用户快速切 tab，先发的慢请求后回来，把后发的新数据覆盖掉。评审看到裸的 fetch/then 里直接 setState，我会建议加 AbortController 或者用带请求 key 的数据层管住竞态，这类问题不看异常路径根本发现不了。
把格式和测试交给工具，人守上下文
如果团队还在评审里花很多时间讨论缩进、引号、分号，说明自动化工具还没承担它该承担的活。格式交给 Prettier 和 ESLint，评审重点放在工具判断不了的地方：业务规则是否正确、抽象是否过早、错误处理是否统一、组件职责是否清楚、性能风险是否明显、测试是否覆盖关键逻辑。人脑适合判断上下文，不适合反复检查格式。
同理，评审不该替代测试。能用单元测试、集成测试、lint 规则固化的，就让工具去守。review 里更该问：关键路径有没有测试、异常分支有没有覆盖、这次改动有没有破坏已有行为。我见过最有价值的一类评论不是指出代码错，而是补一句「这个 bug 是重复提交触发的，能不能加一个测试覆盖连续点击」，这比单纯修当前实现更能防回归。
评审意见要可执行，也要分轻重
低质量的评审意见经常长这样：
1这里感觉不太好。
作者不知道该怎么改，讨论也容易滑向风格之争。更好的写法是把问题、影响、建议一次说清：
1这里把接口返回值直接传给表格了。后端字段为空时会导致渲染异常， 2建议在 service 层做一次 normalize，表格只消费稳定字段。
这样有明确风险，也有可执行方向，作者即便不完全同意，也能围绕问题本身讨论。如果只是偏好，我会明确标成 nit，避免阻塞合并：
1nit: 这里的变量名可以更具体一些，比如 filteredUsers。
但涉及行为风险、数据丢失、安全或线上回滚困难，就要说清楚为什么必须改。评审意见分轻重，团队才不会把所有评论都当成挑刺——这也是让协作能长期做下去的关键，没人愿意在一堆「不太好」里猜哪条是真要改的。
PR 描述本身也是评审对象
评审不只是读代码，PR 描述写得好不好，直接决定评审能不能高效。一个只写「修复问题」的 PR，评审者得从头猜它到底改了什么、为什么这么改、影响哪些页面，来回问一圈才能开始看正事。所以我把「描述是否说清楚」也当成评审的一部分，会跟作者要几样东西：这次改动解决什么问题、涉及哪些页面或模块、有没有需要特别注意的兼容风险、自己是怎么自测的。
尤其是自测说明。前端很多改动的效果只有在特定路径下才显现——某个权限的用户、某种数据为空的情况、某个窄屏尺寸。作者写清楚「我在 A 角色、B 数据为空、C 移动端下都验过」，评审者就能把注意力放在他没覆盖到的路径上，而不是重复验一遍他已经验过的。这不是形式主义，是把双方的时间用在刀刃上。跟作者讲的时候我会强调：描述写清楚，评审快、回滚时定位也快，这笔时间是省下来的不是多花的。
改动大的 PR 我还会建议作者拆分。一个混了重构、修 bug、加功能的大 PR，评审者很难判断每一处改动的意图，回滚时也没法只回退其中一块。能拆成几个单一职责的 PR，评审质量和后续可维护性都更好——这本身也是一条要跟同事讲清楚的维护风险。
依赖数组和闭包：讲清楚「为什么会读到旧值」
React 项目里有一类 bug，作者自己很难看出来，但评审时值得专门盯——useEffect、useCallback 的依赖数组漏项，导致闭包捕获了旧的 state。跟作者解释这类问题，光说「依赖数组不对」他多半没感觉，得把「会读到旧值」这条因果讲透：
1useEffect(() => { 2 const timer = setInterval(() => { 3 console.log(count); // 永远打印初始值 4 }, 1000); 5 return () => clearInterval(timer); 6}, []); // 依赖数组空的，闭包里的 count 被冻在第一次渲染
这段依赖数组是空的，setInterval 里的回调捕获的是首次渲染那个 count，之后 count 再变，定时器里读到的还是老值。我评审时会建议要么把 count 加进依赖，要么用 setCount(c => c + 1) 这种函数式更新绕开对外部值的依赖。团队里开了 eslint-plugin-react-hooks 的 exhaustive-deps 规则后，这类漏项 lint 能提前警告，但 lint 只提示、不判断你到底想要哪种语义，所以最终还得评审时和作者确认意图——这正是「工具守格式、人守上下文」在 Hooks 上的具体落点。
性能风险：明显的先拦，玄学的别瞎优化
前端评审里性能是个要拿捏分寸的话题——既不能放过明显的坑，也不该逼作者到处套 memo 做无谓优化。我的做法是只拦那些「会明显、稳定地拖慢」的写法，其余靠数据说话。
一类是渲染里做重活：在 render 里对大数组做排序、过滤、深拷贝，每次渲染都重算一遍。这种我会建议用 useMemo 缓存，或者把计算挪到数据进来的时候做一次：
1// 每次渲染都重新排序整个列表 2const sorted = bigList.sort((a, b) => a.time - b.time); 3// 建议：依赖不变就不重算 4const sorted = useMemo(() => [...bigList].sort((a, b) => a.time - b.time), [bigList]);
顺带一提，上面那个原始写法还有个隐藏 bug：Array.prototype.sort 是原地排序，直接改了 bigList，如果它来自 props 或 store，就是在偷偷改外部数据。评审时这种「性能」和「副作用」叠在一起的坑最值得指出来。
另一类是把内联对象、内联函数当 props 传给做了 memo 的子组件，每次渲染引用都变，memo 直接失效。但反过来，如果子组件本身很轻、根本没做 memo，那到处 useCallback 反而是负优化——多存一个函数、多一层依赖比较，收益是负的。所以我评审性能时会先问「这里真的慢吗，测过吗」，让作者用 React DevTools 的 Profiler 录一下再决定，而不是凭感觉给一堆 memo。能被 Profiler 证实的性能问题才值得改，靠想象的性能优化往往只增加复杂度。
权限和数据刷新：状态变了，界面跟没跟上
前面提过「权限变了菜单没刷新」，这是电商中后台里反复出现的一类问题，也很值得在评审里单独看。典型场景是：管理员在后台改了某个用户的角色，或者当前用户切换了所属组织，但前端把权限、菜单、按钮可见性算完就缓存住了，之后不再重算。
评审时我会顺着「哪些界面依赖这份会变的数据」问一圈：角色变了，侧边栏菜单会不会重算？组织切换了，列表的数据范围会不会重新拉？跟作者讲的时候我不会停在「这里可能有问题」，而是给出具体的失效路径——是靠 key 强制重挂载子树，还是把权限放进会随登录态失效的数据层、切换时主动 invalidate。这两年用 TanStack Query 的项目里，我会特别看权限、用户信息这类数据有没有正确的 queryKey 和失效时机，因为把它们当成「一次拉取、永不过期」是这类 bug 的根源。
列表和 key：一个容易被放过的老问题
前端评审里有个几乎每个新人都会踩、老手偶尔也会疏忽的点——列表渲染的 key。看到用数组下标当 key，我会停下来看这个列表会不会增删或重排：
1{items.map((item, index) => ( 2 <Row key={index} value={item} /> 3))}
如果列表是静态的、只读的，用下标问题不大；但只要它能删除、插入、拖拽排序，用下标当 key 就会让 React 把 DOM 复用错位——你删了中间一项，界面上却像是删了最后一项，输入框里的值、勾选状态跟着串行。跟作者讲清楚这个「视觉上删错了一行」的具体后果，比只说「key 不该用 index」更能让人记住。正确做法是用数据本身稳定的 id 当 key。这类问题 lint 有时能提示，但判断「这个列表会不会重排」还得靠人看上下文，又是一处工具提示、人做决定的地方。
顺带会看列表相关的性能：长列表有没有虚拟滚动的必要、map 里有没有每次渲染都新建函数导致子组件 memo 失效。这些不一定当场要改，但值得在评论里点出来让作者心里有数。
依赖和首屏：一个 import 的代价
前端评审有个后端评审没有的维度——你引入的每个依赖，用户都要下载。看到 PR 里新增了 import，尤其是为了一个小功能引入一整个大库，我会多问一句这笔账划不划算。跟作者讲这个不能空谈「包大」，得落到具体数字和具体替代：
- 一个日期格式化需求引入整个 moment.js（几百 KB 且不好 tree-shake），能不能换成
date-fns按需引入，或者干脆用Intl.DateTimeFormat原生搞定。 - 图表、编辑器这类重组件，是不是首屏就需要，能不能
React.lazy加Suspense按路由拆出去。 - 有没有把只在某个弹窗里用的库，静态 import 进了首屏 bundle。
我一般会建议作者本地跑一下打包分析（vite-plugin-visualizer 或者 webpack 的 bundle analyzer），把「这个依赖到底多大、进没进首屏 chunk」用图摆出来，比在评论里争论「这个库大不大」有效得多。首屏体积这种事，数据比感觉有说服力。
抽象是不是太早：跟作者算复用这笔账
评审里另一类需要上下文判断、也最容易起分歧的，是抽象时机。有人喜欢一看到两段相似代码就抽公共函数、抽通用组件，PR 里塞进一个「以后可能用得上」的配置化组件。跟作者讨论这个，我不会简单说「过度设计」，而是把复用这笔账摆出来一起算。
我会问几个具体问题：现在到底有几个调用方，是两个还是只有一个未来假想的？这两处的相似是本质相同，还是恰好长得像、业务上会各自演化？为了抽象引入的参数、配置项、条件分支，是不是比重复本身还难维护？很多所谓通用组件，最后长成一个塞了七八个布尔开关的怪物，每个页面来加一个 isXxx，正是前面说的「让通用组件认识具体业务」的另一种表现。
我给团队的经验值是：相同的东西出现第三次再考虑抽象，两次先容忍重复。跟作者讲清楚「早抽一个错的抽象，比重复两段代码更难拆」，通常比争论「这里该不该抽」更能达成一致。反过来，如果 PR 里是把一段被复制了三四遍、且确实同源的逻辑收敛掉，那是该鼓励的，评审也要给正向反馈，别只会挑问题。
前端也有安全线：别把用户输入当可信
前端评审容易漏掉安全，因为大家默认「后端会挡」。但有些洞就开在前端。最典型的是把用户可控内容直接塞进 dangerouslySetInnerHTML：
1<div dangerouslySetInnerHTML={{ __html: comment.content }} />
如果 comment.content 来自用户输入且没经过消毒，这就是一个存储型 XSS。评审看到 dangerouslySetInnerHTML、v-html、innerHTML 这几个关键字，我一律停下来问数据来源：是不是用户可控、有没有过 DOMPurify 这类消毒、能不能干脆用纯文本渲染避开。跟作者讲的时候要落到具体攻击面——「用户在评论里写一段 <img onerror>，别人打开页面就会执行」，比空说「这里不安全」更能让人当回事。
顺带会看几处相关的：跳转用的 URL 是不是拼了用户输入（javascript: 伪协议）、target="_blank" 有没有配 rel="noopener"、把 token 之类敏感信息往 URL 或日志里放的写法。这些不是每个 PR 都有，但一旦出现，代价远高于一个命名问题，属于必须拦下的那一类。
可访问性和文案：容易漏，但维护成本很实在
无障碍在评审里常被当成「有空再优化」，但有些点补起来成本很低、漏掉后返工很贵。我会顺手看：只用颜色区分状态（红/绿）而没有文字或图标兜底、可点击的 div 没有 role 和键盘事件、图片缺 alt、表单控件和 label 没关联。这些跟作者讲清楚谁会受影响就行——「用红绿区分成功失败，色弱用户分不出来」，不是抽象的规范要求，是具体的可用性问题。
文案和国际化也算维护风险的一种。硬编码在组件里的中文字符串，等哪天要做多语言就得满项目捞；直接字符串拼接的提示（'共' + count + '条'）在有复数、有语序差异的语言里会出问题。团队现在不做多语言可以先不拦，但如果 PR 里已经在往会被复用的公共组件里硬编码文案，我会提一句，让它至少走统一的文案入口，别散落到处。
评审是对话，不是判决
技术点之外，评审能不能长期做下去，很大程度看沟通方式。同样是指出问题，「你这里错了」和「如果接口这时候失败，用户会卡在保存中，我们要不要在 finally 里兜一下」，作者的接受度完全不同。前者像判决，后者是把风险摊到桌面上一起看。
我给自己定过几条习惯：对事不对人，评论针对代码的行为后果，不评价作者水平；能给方向就给方向，不只说「这样不好」；拿不准的地方用提问而不是断言，「这里并发点击会不会重复提交」比「这里有并发 bug」更容易开启讨论，万一是我理解错了也留了余地；作者据理力争、且理由成立时，痛快地 approve，评审不是要赢。评审最终是团队在一起把维护风险讲清楚，谁指出的、谁改的都不重要，重要的是这个风险在上线前被看见了。
我扫 PR 的层次
每次 review 不可能面面俱到，但我会优先扫这些点：改动入口和影响范围是否清楚；loading、empty、error、disabled 状态是否完整；API 错误是否走统一处理；重复点击、快速切换、页面卸载是否安全；新增依赖是否必要、是否影响首屏；组件职责有没有被具体业务侵入；关键路径有没有测试或至少明确的自测说明。
实际看 PR 时我按层次扫，而不是从第一个文件一路读到最后：
1第一遍：看文件列表和 diff stat，判断影响范围 2第二遍：看数据入口，接口、参数、权限和错误结构有没有变 3第三遍：看状态流转，loading、empty、error、disabled 是否闭环 4第四遍：看组件职责，通用组件有没有被具体业务侵入 5第五遍：看测试和自测说明，关键风险有没有被固定下来
一个 PR 改了 20 个文件，先看 git diff --stat 就能知道它到底是业务逻辑改动、样式改动还是大面积格式化：
1git diff --stat origin/main...HEAD 2git diff --name-only origin/main...HEAD
如果文件列表里同时出现请求封装、路由守卫、公共组件和某个页面，我会先问这次改动摊子铺得是不是过大。不是不能这样改，而是 review 成本和回滚风险都上去了，作者得在 PR 描述里把原因讲清楚。我也会单独看「删除了什么」：
1git diff --diff-filter=D --name-only origin/main...HEAD
前端项目里误删路由配置、样式入口、静态资源引用并不少见，只看新增代码容易漏掉这些。
一个有效的评审重点关注状态流转、组件职责、异常路径、错误处理、测试覆盖和业务逻辑是否被破坏。格式和简单规则交给工具，评审者把精力放在真正需要上下文判断、也真正需要跟同事讲清楚代价的地方，团队才能从代码评审里拿到稳定的收益。