前端音视频插件系统【免费下载链接】clapprAn extensible, plugin-oriented, HTML5-first media player for the web项目地址https://gitcode.com/gh_mirrors/cl/clappr点击查看免费下载本篇技术指南以 Clappr 仓库内的 .agents/skills/code-review/SKILL.md 为骨架系统讲解如何在合并任何变更之前对代码执行多维度正确性、可读性、架构、安全、性能的结构化评审。本文面向 Clappr可扩展、面向插件的 HTML5 网页媒体播放器的贡献者与 AI Agent读完你可以掌握如何获取 diff、如何按五轴逐项审查、如何用「Required / Optional」两级严重度输出评审报告以及如何用可复制的报告模板把结论清晰地写给从初级到高级的每一位读者。该技能文档是 Clappr 仓库 Agent 工作流的一部分——AGENTS.md 明确规定对于 pull-request 评审、代码评审或任何针对 diff 的结构化合并前反馈请阅读并执行.agents/skills/code-review/SKILL.md它定义了严重级别和输出模板。本指南在忠实保留原文档全部内容的基础上结合仓库源码补充底层原理佐证。一、定位与触发时机这个技能在仓库中扮演什么角色1.1 概览与审批标准该技能的核心主张是每次变更在合并前都必须经过评审没有例外。评审覆盖五个维度axesCorrectness正确性Readability可读性Architecture架构Security安全Performance性能审批标准Approval standard当一项变更明显提升了整体代码健康度时即使它并不完美也应予以批准。完美的代码不存在——目标是持续改进。不要因为代码不是按你的方式写的就阻塞变更只要它改善了代码库并遵循项目约定就批准它。1.2 何时使用When to use合并任何 PR 或变更之前完成一个功能实现之后当另一个 Agent 或模型产出了需要评估的代码时重构现有代码时任何 bug 修复之后同时评审修复本身与回归测试1.3 评审结果在哪里呈现评审只在本对话中交付给出结论、按级别分类的发现、建议。不要自动把评论、备注或讨论发布到 GitHub。只有用户明确要求时才把评审发到 GitHub 上。二、收集 diff评审前的唯一输入在进入任何分析之前先拿到完整 diff只读操作不需要在 PR 上评论首选用户提供了 GitHub pull-request 链接或编号gh pr diff number|url否则用当前分支对比 origin/maingit diff origin/main...HEAD必须使用origin/main而不是本地main因为本地分支通常已过时。需要 PR 上下文标题、正文、base 分支时使用gh pr view number|url。如果输出被保存到了文件持久化输出必须用Read配合连续的offset和limit读取整个文件直到结尾——绝不要从一次部分读取就开始评审。这一先拿完整 diff的要求与仓库的提交纪律形成闭环Clappr 的 .commitlintrc.js 继承commitlint/config-conventional.husky/commit-msg 中 Husky 钩子执行commitlint --edit $1不合规的提交信息在落库前即被拒绝详见 .agents/skills/commit/SKILL.md因此评审对象——diff——本身就是结构清晰、可按包隔离的。三、五个评审维度详解3.1 正确性Correctness代码是否做了它声称要做的事是否符合规格或任务需求边界情况是否被处理null、空值、边界值错误路径是否被处理不只是 happy path是否通过全部测试测试是否真的验证了正确的行为是否存在 off-by-one 错误、竞态条件或状态不一致3.2 可读性与简洁性Readability and simplicity另一位工程师或 Agent能否在作者不解释的情况下理解这段代码命名是否具有描述性且与项目约定一致不允许没有上下文的temp、data、result控制流是否直观避免嵌套三元、深层回调代码组织是否合乎逻辑相关代码分组、清晰的模块边界是否存在应当简化的聪明技巧能否用更少的行数完成1000 行能解决的事写了 10000 行就是失败抽象是否为其复杂度买单不要过早泛化等到第三次使用场景再泛化注释是否能澄清非显而易见的意图但不要注释显而易见的东西是否存在死代码产物no-op 变量_unused、兼容性 shim、或// removed注释是否把一个新的条件分支硬接在了一个无关的流程上这是设计异味而不是可选优化——把逻辑移入它自己的 helper、state 或 policy而不是纠缠进现有路径。是否反复出现针对同一形状的条件判断这预示着缺少一个模型或 dispatcher。临时分支通常会变成永久性债务。仓库佐证Clappr 的命名规范在 AGENTS.md 中有明确约定——布尔值用is*/has*/can*前缀方法用动词类用名词私有成员用_前缀常量用UPPER_SNAKE_CASE。评审命名是否一致时这套约定就是直接可引用的标准。方法体建议控制在约 30 行以内、优先 early return、倾向组合而非继承这些都可作为 Readability 轴的具体判据。3.3 架构Architecture变更是否符合系统的设计是否遵循现有模式还是引入了新模式如果是新模式是否有正当理由是否保持干净的模块边界是否存在应当共享的代码重复依赖是否朝正确方向流动没有循环依赖抽象层级是否恰当不过度工程、不过度耦合这次重构是降低了复杂度还是仅仅挪动了复杂度数一数读者为理解变更需要持有的概念数量。如果更干净的版本并没有改变这个数量那它并不更干净——优先选择让整个分支、模式或层消失的重构而不是把相同逻辑重新集中化。优先删除一个抽象而不是打磨它。特性逻辑是否泄漏进了共享或通用模块把逻辑留在归属层复用现有的 canonical helper 而不是近似副本不要纵容架构漂移。类型边界是否明确质疑随意的any/unknown/optional/cast 和掩盖不明确不变量的静默回退——让边界显式化通常能简化周围的控制流。仓库佐证Clappr 是 Lerna Yarn workspaces 的 monorepo见根 package.json模块边界清晰clappr/core提供 Player、Core、Container、Playback 核心架构clappr/plugins承载官方插件clappr/player是公开入口 bundle另有hlsjs-playback、dash-shaka-playback、html5-tvs-playback、clappr-zepto、clappr-telemetry等包。架构评审中特性逻辑是否放进了共享模块这一问在这个 monorepo 里可以直接落到具体包归属上判断。TypeScript 边界规范在 AGENTS.md 中同样有据可查避免any、使用unknown、接口定义形状、类型守卫、类型贴近使用处。3.4 安全Security变更是否引入了漏洞聚焦这个 monorepo 的攻击面web/TV 播放器、插件、DOM、广告 SDK、配置和依赖这些在 diff 中绝不能接受Never accept in these diffs提交了 secrets、tokens 或.env文件tokens 或凭据存放在localStorage应放在内存中eval()、Function构造函数、或对不可信输入使用innerHTML/ 不安全 HTML敏感数据出现在日志、错误消息或 URL 中未校验event.origin和消息结构的postMessage使用targetOrigin: *发送当 diff 触及以下主题时必须检查Always check when the diff touches the subject用户或第三方内容广告、播放列表、远程配置是否针对正确上下文HTML、JS、URL做了消毒/编码从不可信来源合并对象时是否带入了危险键__proto__、constructor、prototype来自 API、日志、配置和外部 SDK 的数据是否在边界处被视为不可信、直到被验证新增依赖 / 升级可信来源、yarn audit无可达的 critical/high 且无缓解措施、yarn.lockdiff 被评审如果 secret 已经泄漏进历史先轮换rotate只删掉那一行是不够的仓库佐证这条轴与 Clappr 的工程防线高度一致。根 eslint.config.js 把no-eval、no-implied-eval、no-new-func、no-obj-calls、no-proto等设为 error 级规则从静态检查层面堵死eval/Function构造器路径AGENTS.md 的 Judgment boundaries 也明确列出 NEVER 清单不提交 secrets/tokens/.env、不用eval()或Function构造器、不把 token 放localStorage优先 httpOnly cookies 或内存、不对不可信用户输入用innerHTML、不在 console/错误/URL 中记录敏感数据。评审者拿着这条清单就可以逐项对照 diff。3.5 性能Performance评估 diff 中可见的静态模式针对这个 web/TV monorepo清理与内存Cleanup and memorytimers、listeners、observers、WebSockets、workers、媒体元素、MediaSource、Blob URL 是否在 destroy/unload 时被释放没有上限或 TTL 的缓存/集合是否会在长时间会话中无限增长是否存在对 DOM、缓冲区或大闭包的明显保留在 Tizen/WebOS 上尤为相关热路径Hot paths高频 handlertimeupdate、progress、metrics、媒体控制中是否有重同步工作或 layout thrashing在循环中读写布局每秒被调用多次的路径上是否有大分配或不必要的 O(n) 工作播放器热路径上是否有无明确必要的新依赖或新代码React仅在 React 存在的地方app-frames、player-tvs-native检查不必要的重渲染——不要把 memoization 当作这个 monorepo 的默认审查轴仓库佐证热路径审查在 Clappr 源码里有非常直观的证据链。播放器事件系统中PLAYBACK_TIMEUPDATE playback:timeupdate、PLAYBACK_PROGRESS playback:progress、CONTAINER_TIMEUPDATE、CONTAINER_PROGRESS等高频事件定义在 packages/clappr-core/src/base/events/events.js如第 358、400、848、862 行HTML5 播放器把 DOM 事件映射为timeupdate: _onTimeUpdate、progress: _onProgress见 packages/clappr-core/src/playbacks/html5_video/html5_video.js 的事件映射表而 packages/clappr-core/src/components/log/log.js 的EXCLUDE_LIST明确排除了timeupdate、playback:timeupdate、playback:progress、container:hover、container:timeupdate、container:progress、core:mousemove——因为这些事件每秒触发多次打日志本身就是一种性能损耗。评审时质疑在 timeupdate 上无节流地挂 listener 会怎样仓库已经给出了这些事件就是高频噪音的事实依据。四、结构性修复建议不只是指出问题当你标记一个结构性问题时要提出迁移方案the move——而不仅仅是问题。只说这很复杂的评审让作者无从下手。使用具名的重构方案用类型化模型或显式 dispatcher 替换条件判断链。把重复分支合并成单一更清晰的流程。把编排orchestration与业务逻辑分离让各自可独立阅读。把特性逻辑从共享模块移出放进拥有该概念的包中。复用 canonical helper而不是自造的近似副本。让类型边界显式化使下游分支消失。删除仅增加间接性而不澄清 API 的透传包装器pass-through wrapper。提取 helper或把大文件拆分为聚焦的模块。优先选择移除活动部件的修复方案而不是把同样的复杂度摊开的方案。五、变更规模控制评审的第一道防线小而聚焦的变更更容易评审、合并更快、部署更安全。目标基准~100 行变更 → 良好。一次坐下即可评审完。 ~300 行变更 → 可接受前提是它是一个单一逻辑变更。 ~1000 行变更 → 太大。拆分它。注意文件大小而不只是 diff 大小。小 diff 也可能把文件推过健康边界——单个文件约 1000总行区别于上面约 1000变更行的阈值是常见的检查信号不是硬性上限。当一项变更实质性增大了一个已很大的文件时先问是否应先提取 helpers、子组件或模块再往上堆。先分解再加。什么算一个变更一个自包含的修改解决一件事、包含相关测试、提交后系统保持可用。是一个功能的一部分——而不是整个功能。变更过大时的拆分策略策略做法适用时机Stack堆叠提交一个小变更基于它开始下一个存在顺序依赖By file group按文件组为需要不同评审者的组分别提交变更跨领域关注点Horizontal水平先创建共享代码/桩再创建消费者分层架构Vertical垂直把功能拆成更小的全栈切片功能型工作大变更何时可接受完整的文件删除和自动化重构——此时评审者只需要验证意图而不是逐行验证。把重构与功能工作分开。同时重构现有代码和新增行为的变更等于两个变更——分开提交。小的清理如变量重命名可由评审者酌情并入。六、评审流程四步走Step 1理解上下文看代码之前先理解意图- 这个变更想达成什么 - 它实现的是哪个规格或任务 - 预期的行为变化是什么Step 2先评审测试测试揭示了意图和覆盖度- 变更是否有测试 - 它们测试的是行为而非实现细节吗 - 边界情况是否被覆盖 - 测试命名是否有描述性 - 如果代码变了测试能抓住回归吗仓库佐证Clappr 的测试基座在 vitest.config.base.mjs——每个包共享的 Vitest 配置jsdom 环境、globals、v8 覆盖率并通过defineClapprVitest工厂支持 smoke 测试**/dist.smoke.test.js与常规单测的分流。根package.json的yarn test会先lerna run test --no-bail跑所有定义了 test 脚本的包再对test/目录做一次根级vitest run。评审测试是否存在、是否覆盖时yarn test、yarn test:smokedist 产物冒烟CI 在yarn build:dist后执行、以及单文件vitest run src/path/to/test.test.js都是可直接验证的手段。Step 3评审实现带着五个维度走一遍代码对每个变更文件 1. 正确性这段代码是否做了测试声称它应做的事 2. 可读性不需要帮助我能理解它吗 3. 架构它适配系统吗 4. 安全有任何漏洞吗 5. 性能有任何瓶颈吗Step 4对发现分级只有两级且只有两级级别含义作者动作Required阻塞合并合并前修复。Bug、回归、漏洞、遗留死代码、合并后会被常态化的结构性债务。Optional建议值得考虑由作者决定。绝不阻塞。Required 之上没有第三级。如果它阻塞合并它就是 Required——严重程度体现在发现的排序和解释里而不是一个额外的标签。最多 3 个 Optional选杠杆最高的那些。一长串小建议会稀释重点第 4 个 Optional 消耗作者注意力的成本大于它带来的价值。用最重要的事开头。按杠杆排序发现正确性和安全优先然后是结构性回归和被错过的简化再是其他。不要把真正的问题埋在化妆性的 Optional 之下——少数几个高置信度的发现胜过一长串清单。如果有一个结构性问题加十个琐碎点那么结构性问题本身就是这份评审。Step 5验证作者的验证故事- 跑了哪些测试 - 构建通过了吗 - 变更是否被手动测试过 - UI 变更是否有截图 - 有 before/after 对比吗七、死代码卫生任何重构或实现变更之后检查孤儿代码现在不可达的片段、no-op 变量、兼容性 shim、被注释掉的为了测试的 early-return。发现的死代码与其他发现一样是 Required 级别——不是报告末尾的单独一节也不是全大写的块。它遵循与其他发现相同的结构删除与否的问题放进 SuggestionSuggestion:removeformatLegacyDate()— replaced byformatDate()with no remaining references. I can delete it, or would you rather do it on the branch?不要留着死代码——它会迷惑未来的读者和 Agent。但也不要默默删除你不确定的东西。拿不准时问。八、评审中的诚实评审代码时——无论评的是自己的、另一个 Agent 的、还是人类的不要橡皮图章。没有评审证据的 LGTM 对任何人都没有帮助。不要软化真问题。这可能是个小问题——当它是个会打到生产环境的 bug 时——是不诚实的。尽可能量化问题。这个监听器挂在每个timeupdate上且没有节流会在主线程上每秒触发几十次比这可能有点慢更好。对有明确问题的方案要直接反驳。迎合sycophancy是评审中的一种失败模式。如果实现有问题直说并提出替代方案。优雅地接受否决。如果作者拥有完整上下文且不同意尊重他们的判断。评论代码不要评论人——把针对个人的批评重构为聚焦代码本身。九、依赖纪律评审的一部分代码评审的一部分是依赖评审在添加任何依赖之前现有技术栈能否解决这个问题通常可以。依赖有多大检查 bundle 影响。它是否在积极维护检查最近提交、开放 issue。它是否有已知漏洞yarn audit许可证是什么必须与项目兼容。规则优先使用标准库和现有工具而不是新依赖。每个依赖都是一份负债。升级现有依赖和其他代码变更一样而风险最高的升级是那种以 bump deps 一句话批量合并的。用同样的纪律评审它们读 changelog而不仅是版本号。Semver 是维护者可能并未兑现的承诺——一个 patch 也可能携带行为变更。对 major 升级读迁移说明找出会破坏什么。每次只升级一个依赖。单独或按小的相关组升级并合并。当批量 bump 弄坏构建时你不知道是哪个包干的单包变更让原因显而易见、回滚干净。让测试做决定。升级由前后都是绿套件验证而不是它装上了。如果依赖行为附近的覆盖很薄那个缺口才是真正的发现——先加测试。留意传递依赖图。大多数已安装的包不是任何人直接选的。评审 lockfile diff而不只是package.json一次直接升级可能拉入几十个间接变更。让 lockfile 保持诚实。提交它、评审它的 diff、绝不手改。lockfile 才是真正固定上线内容的。yarn audit/ 供应链分级runtime/build/deploy 可达的 critical/high → 阻塞直到升级、打补丁或替换不可达 → 尽快修复并记录。生产环境中的 moderate → 下个周期仅 dev → 积压。绝不盲目强制修复yarn audit --force或等价物。对新依赖审查 typosquat、发布时间和所有权。本 monorepo 的实际情况Yarn 1.22根目录单一yarn.lock用yarn install --frozen-lockfile可复现安装——根 package.json 的packageManager字段明确固定为yarn1.22.21engines.node要求24。评审 lockfile diff 时根目录单一 yarn.lock意味着任何包升级的间接影响都集中在这一个文件里可查。十、评审清单Review checklist## Review: [PR/change title] ### Context - [ ] I understand what this change does and why ### Correctness - [ ] The change matches the spec/task requirements - [ ] Edge cases handled - [ ] Error paths handled - [ ] Tests cover the change adequately ### Readability - [ ] Names are clear and consistent - [ ] Logic is straightforward - [ ] No unnecessary complexity ### Architecture - [ ] Follows existing patterns - [ ] No unnecessary coupling or dependencies - [ ] Appropriate abstraction level - [ ] Refactors reduce complexity rather than relocate it - [ ] No feature logic in shared modules; file stays within a healthy size ### Security - [ ] No secrets in code, logs, or localStorage for tokens - [ ] No eval / Function / unsafe innerHTML - [ ] postMessage validates origin and payload; no targetOrigin: * - [ ] Config/object merging free of prototype pollution - [ ] External data (APIs, ads, playlist) treated as untrusted - [ ] Dependencies / lockfile reviewed (yarn audit when there is a bump) ### Performance - [ ] Resources released on destroy/unload (timers, listeners, media, Blob URLs, MediaSource) - [ ] No heavy work or layout thrashing in hot paths (timeupdate, progress, metrics) - [ ] Caches/collections bounded or with TTL; no obvious DOM/buffer retention - [ ] Bundle / deps in the hot path justified ### Verification - [ ] Tests pass - [ ] Build succeeds - [ ] Manual verification done (if applicable) ### Verdict - [ ] **Approve** — Ready to merge - [ ] **Request changes** — There are open Required findings十一、常见合理化借口Common rationalizations评审者经常遇到作者或自己的合理化借口。逐条对质合理化借口现实它能跑就够了能跑但不可读、不安全或架构错误的代码会产生复合增长的债务。我写的所以我知道它是对的作者对自身的假设是盲目的。每次变更都受益于另一双眼睛。我们以后再清理以后永远不会来。评审就是质量门禁——用它。要求合并前清理而不是合并后。AI 生成的代码应该没问题AI 代码需要更多审视而不是更少。它自信且看似合理即使错了也如此。测试过了所以没问题测试必要但不充分。它们抓不到架构、安全或可读性问题。这次重构让它更干净了挪动复杂度不是降低复杂度。如果读者要持有的概念数量没变结构就没改善——去找让分支消失的版本。只是给这个文件加一小块小 diff 照样能把文件推过健康线把分支硬接在无关流程上。判断最终结构而不是 diff 大小。只是升个版本号一次升级就是你没写的行为变更。读 changelogsemver 不保证无破坏。一次 PR 全升完省时间批量 bump 弄坏构建时会掩盖是哪个包干的。每次一个依赖让原因和回滚都干净。十二、红旗Red flags没有任何评审就被合并的 PR只检查测试是否通过的评审忽略其他维度没有实际评审证据的 LGTM安全敏感变更没有进行安全聚焦的评审大到没法好好评审的大 PR拆分它们没有回归测试的 bug 修复不带级别的发现——不清楚什么阻塞合并、什么只是建议把症状、修复和测试建议堆在一个段落里的发现接受我以后再修——它永远不会发生只是搬动代码、却没有减少读者需持有概念数量的重构增大本已很大的文件而不是分解它的变更散落到无关代码路径的新条件分支缺失的抽象复制现有 canonical helper 的自造 helper或放进共享模块的特性逻辑没有 changelog 评审、没有按包隔离的批量 bump dependencies PR手改、未提交、或未经 diff 评审就合并的 lockfile 变更十三、评审后验证Verification评审完成后所有 Required 发现已解决或已明确延期且有正当理由测试通过构建成功验证故事有文档记录改了什么、如何验证的依赖升级已对照其 changelog 评审、按包隔离并由绿套件验证lockfile diff 已评审推定阻塞项Presumptive blockers对以下每一项先提出并建议更简单的设计只有当变更主动让结构更差时才升级为 Required把复杂度搬来搬去而非降低的重构把文件推过大小边界且未分解的变更加进共享模块的特性逻辑现有 canonical helper 的近似副本掩盖不明确不变量的静默回退。十四、输出格式评审是被人读的评审的读者是初级到高级的工程师几乎总是在终端或 PR 标签页里读它。把症状、机制、修复和测试建议堆进一个密集段落无论多正确都不可读。教学性didactics是这项技能的差异化优势。如果作者要读两遍一个发现才能理解问题是什么那评审即使是对的也失败了。14.1 报告结构四个部分按此顺序。除此之外不要有任何内容。## Review: [PR/change title] **Verdict:** APPROVE | REQUEST CHANGES [Overview in 1-2 sentences: what the change does and the overall assessment.] ### Required [numbered findings] ### Optional [at most 3, numbered] ### Verification - **Tests reviewed:** yes/no — [half a line] - **Build verified:** yes/no — [half a line] - **Security verified:** yes/no — [half a line]结论规则有任何 Required 发现 → REQUEST CHANGES。没有 → APPROVE。如果任一列表为空写 None found 并继续。没有表扬区——变更做对的地方放进 overview用一句话在合适的时候。一长串积极点会撑大报告而没人读它。14.2 单个发现的解剖Anatomy of a finding四个部分按此顺序各部分之间用空行分隔#### 1. [Title: the problem in at most 8 words] [Didactic explanation. 2 to 4 sentences. One idea per paragraph — if you have two, split them. Write for someone who does not know this stretch of code: say what happens and why it breaks, not just the symptoms name.] file:line js // the code as it is today, 3 to 8 lines // just enough to see the problem without opening the file **Suggestion:** [the direction of the fix, in one sentence. Do not write the patch.]代码块在 Required 发现上是必需的在 Optional 上是可选的——当代码片段本身就是论证时包含它当问题是结构性的、代码本身展示不出什么时省略它。14.3 硬格式规则每个部分之间都有空行。标题、解释、file:line、代码和 Suggestion 绝不紧贴在一起。绝不在同一段落里堆叠症状 修复 测试建议。症状和机制进解释修复进 Suggestion缺失的测试是它自己的发现而不是另一个发现的附录。每段最多 3 句话。超了就拆。一个发现是一个####标题块不是 bullet。bullet 会变成一长串 run-on 段落。每个发现一个file:line。如果同一问题出现在三处引用主要的那处在解释里提及其余——不要往标题里贴a.js:65,135,194。Suggestion 只有一句。一句放不下说明该发现混了两个问题——拆分它们。给发现编号让作者能说关于第 3 条……。写全称。PLAYBACK_READYarrives after the resource 比 PR res 好。项目行话可以临时编造的缩写不行。强调不用全大写除了上面四部分之外不发明新章节。14.4 内容规则先评审测试——它们揭示意图和覆盖度评审代码前先读规格或任务描述每个 Required 发现都要在 Suggestion 里带来修复方向不要在有未解决 Required 发现时批准代码不确定就说出来并建议调查而不是猜测14.5 完整示例Complete example这是要复制的模式——注意间距和每部分的体量## Review: DASH live captions (Shaka and Tizen providers) **Verdict:** REQUEST CHANGES Splitting Shaka and Tizen into per-playback providers is the right direction: it takes the if (tizen) / if (shaka) out of the sidecar flow. But the gate that separates live from VOD is incomplete, and an early-return was commented out for testing, so the merge would regress DASH VOD and HLS live. ### Required #### 1. DASH VOD enters the live flow The code decides between downloading the sidecar caption and using the players text tracks by asking only is this resource DASH?. The second half of the question is missing: is it DASH **live**?. On DASH VOD with Shaka, PLAYBACK_READY arrives after the resource. When that happens the provider runs again, emits WM_SUBTITLE_AVAILABLE with Shakas tracks, and replaces the sidecar that had already loaded — on screen, the language list swaps by itself. subtitle_loader.js:65 js if (this.isDashResource(resource)) { this.setupDashSubtitles() } **Suggestion:** use isDashLivePlayback — the predicate onResourceReady already uses — in setupDashSubtitles, onSubtitleChanged, and the PLAYBACK_READY listener as well. #### 2. The users chosen caption snaps back to default Shaka fires trackschanged several times during a live broadcast, not only on the initial load. On every fire the provider reapplies the initial caption and re-emits the menu. The track objects that arrive on that event have no selection mark. Anyone who picked English is put back on the default mid-session without touching anything. The Tizen provider already guards against this by storing track ids; the Shaka one does not. shaka_dash_live_subtitle_provider.js:68 js this._player.addEventListener(trackschanged, () { this.setDashTracks() this.setInitialSubtitle() }) **Suggestion:** always update the track map, but call setInitialSubtitle only the first time and keep the current selection while the id still exists. #### 3. Commented-out early-return for testing shipped The return that kept the language menu from mounting on live broadcasts is commented out, not removed. Commented this way, it opens the menu for **all** live — HLS included — not only the DASH live this branch is meant to cover. The new tests still call bindContainerEvents() by hand, which means they were written to work around the return, not to validate removing it. language_menu_tv.js:139 js onMetadataLoaded(video) { this.video video //if (this.video?.isLive) return this.bindContainerEvents() this.renderPlugin() } **Suggestion:** delete the commented line and put an explicit DASH-live gate in its place — I can do that, or would you rather adjust it on the branch? #### 4. Setup retry runs forever, four times per second When the provider does not exist yet, scheduleDashSetup schedules a new 250ms timer. That timer calls back into the same function, which schedules another — no counter and no stop condition. On any DASH whose playback is neither Shaka nor Tizen the provider never appears, and the cycle continues until destroy. On an entry-level TV that is constant main-thread work for the whole session. subtitle_loader.js:152 js scheduleDashSetup() { clearTimeout(this._dashSetupTimer) this._dashSetupTimer setTimeout(() this.setupDashSubtitles(), 250) } **Suggestion:** cap the number of attempts and log once when they run out, instead of rescheduling forever. ### Optional #### 1. pt-BR is normalized in Shaka but not in Tizen The Shaka provider turns pt-BR into pt before matching the language map; the Tizen provider uses the raw value from AVPlay. It works today by accident, because AVPlay delivers por, which is also in the map. shaka_dash_live_subtitle_provider.js:321 **Suggestion:** extract a normalizeLanguage and use it in both providers. #### 2. One invalid extra_info wipes every track Parsing Tizens track list wraps JSON.parse of all of them in a single try. If one track comes with malformed extra_info, the exception also discards the valid tracks that had already been read. tizen_dash_live_subtitle_provider.js:388 **Suggestion:** move the try inside the loop, dropping only the bad track. ### Verification - **Tests reviewed:** yes — they cover flow choice and the menu label; they do not cover DASH VOD or selection preservation. - **Build verified:** no — static review of the remote branch, working tree on main. - **Security verified:** yes — no secrets, eval, postMessage, or new deps; the native cue goes to the DOM with .text().十五、把技能接回 Clappr 的工作流这份技能不是孤立存在的它与仓库的工程纪律互为印证触发入口AGENTS.md 要求 PR 评审、代码评审、任何结构化的合并前反馈都要先读并执行 .agents/skills/code-review/SKILL.md提交阶段则执行 .agents/skills/commit/SKILL.md先检查分支、按 Conventional Commits 格式、英文消息——评审与提交形成前后两道关卡。安全防线评审中绝不接受的清单eval、localStorage token、innerHTML、postMessage origin与 eslint.config.js 的no-eval/no-implied-eval/no-new-func等规则、AGENTS.md 的 NEVER 清单三处一致评审者可直接引用。性能佐证热路径检查可落到 events.js 的高频事件定义、html5_video.js 的 DOM 事件映射、以及 log.js 对 timeupdate/progress 的日志豁免——每秒多次触发不是抽象说法而是源码明示的事实。工具链验证yarn lint含根级 eslint.config.js、yarn testLerna 逐包 根级 vitest、yarn test:smokedist 冒烟、yarn build等脚本定义在根 package.json是评审中验证验证故事的直接执行入口提交信息的合规性由 .commitlintrc.js .husky/commit-msg 在 Husky 钩子里强制。掌握这套五轴评审模型后你可以在 Clappr 的任何 PR 或 Agent 产出上执行一次完整、可引用、教学性的评审先拿全 diff再按正确性、可读性、架构、安全、性能逐项核查用 Required/Optional 两级输出结论最后用四段式报告把哪里坏了、为什么坏、往哪修讲清楚——这正是让代码库在每次合并时都变得更健康的机制。赞分享前端音视频插件系统【免费下载链接】clapprAn extensible, plugin-oriented, HTML5-first media player for the web项目地址https://gitcode.com/gh_mirrors/cl/clappr点击查看免费下载相关推荐Penpot 仓库代码评审规范五轴评审方法、严重度分级与合并前质量门禁实践Penpot 仓库代码评审规范五轴评审方法、严重度分级与合并前质量门禁实践 导读 本文基于 Penpot 仓库 .opencode 技能体系中用于 合并前评审前端设计系统图形学协同办公Sanity 代码评审质量门禁五轴评审方法、变更规模管控与人机协作的多模型评审实践Sanity 代码评审质量门禁五轴评审方法、变更规模管控与人机协作的多模型评审实践 本文以 Sanity 仓库中的 code review and qualiCMS前端awesome-blender Blender资源清单千余个插件、教程与3D资产一站收齐awesome blender Blender资源清单千余个插件、教程与3D资产一站收齐 找流体模拟插件翻遍搜索结果挑不中找CC0纹理要在六个网站之间来回跳文档教程上一篇3步打造逼真金属质感MuJoCo物理引擎高级渲染技术指南下一篇MuJoCo Unity插件在WebGL构建中的技术挑战与解决方案创作声明：本文部分内容由AI辅助生成（AIGC），仅供参考
Clappr 代码评审技能指南：五轴评审模型、分级输出与合并质量门禁
前端音视频插件系统【免费下载链接】clapprAn extensible, plugin-oriented, HTML5-first media player for the web项目地址https://gitcode.com/gh_mirrors/cl/clappr点击查看免费下载本篇技术指南以 Clappr 仓库内的 .agents/skills/code-review/SKILL.md 为骨架系统讲解如何在合并任何变更之前对代码执行多维度正确性、可读性、架构、安全、性能的结构化评审。本文面向 Clappr可扩展、面向插件的 HTML5 网页媒体播放器的贡献者与 AI Agent读完你可以掌握如何获取 diff、如何按五轴逐项审查、如何用「Required / Optional」两级严重度输出评审报告以及如何用可复制的报告模板把结论清晰地写给从初级到高级的每一位读者。该技能文档是 Clappr 仓库 Agent 工作流的一部分——AGENTS.md 明确规定对于 pull-request 评审、代码评审或任何针对 diff 的结构化合并前反馈请阅读并执行.agents/skills/code-review/SKILL.md它定义了严重级别和输出模板。本指南在忠实保留原文档全部内容的基础上结合仓库源码补充底层原理佐证。一、定位与触发时机这个技能在仓库中扮演什么角色1.1 概览与审批标准该技能的核心主张是每次变更在合并前都必须经过评审没有例外。评审覆盖五个维度axesCorrectness正确性Readability可读性Architecture架构Security安全Performance性能审批标准Approval standard当一项变更明显提升了整体代码健康度时即使它并不完美也应予以批准。完美的代码不存在——目标是持续改进。不要因为代码不是按你的方式写的就阻塞变更只要它改善了代码库并遵循项目约定就批准它。1.2 何时使用When to use合并任何 PR 或变更之前完成一个功能实现之后当另一个 Agent 或模型产出了需要评估的代码时重构现有代码时任何 bug 修复之后同时评审修复本身与回归测试1.3 评审结果在哪里呈现评审只在本对话中交付给出结论、按级别分类的发现、建议。不要自动把评论、备注或讨论发布到 GitHub。只有用户明确要求时才把评审发到 GitHub 上。二、收集 diff评审前的唯一输入在进入任何分析之前先拿到完整 diff只读操作不需要在 PR 上评论首选用户提供了 GitHub pull-request 链接或编号gh pr diff number|url否则用当前分支对比 origin/maingit diff origin/main...HEAD必须使用origin/main而不是本地main因为本地分支通常已过时。需要 PR 上下文标题、正文、base 分支时使用gh pr view number|url。如果输出被保存到了文件持久化输出必须用Read配合连续的offset和limit读取整个文件直到结尾——绝不要从一次部分读取就开始评审。这一先拿完整 diff的要求与仓库的提交纪律形成闭环Clappr 的 .commitlintrc.js 继承commitlint/config-conventional.husky/commit-msg 中 Husky 钩子执行commitlint --edit $1不合规的提交信息在落库前即被拒绝详见 .agents/skills/commit/SKILL.md因此评审对象——diff——本身就是结构清晰、可按包隔离的。三、五个评审维度详解3.1 正确性Correctness代码是否做了它声称要做的事是否符合规格或任务需求边界情况是否被处理null、空值、边界值错误路径是否被处理不只是 happy path是否通过全部测试测试是否真的验证了正确的行为是否存在 off-by-one 错误、竞态条件或状态不一致3.2 可读性与简洁性Readability and simplicity另一位工程师或 Agent能否在作者不解释的情况下理解这段代码命名是否具有描述性且与项目约定一致不允许没有上下文的temp、data、result控制流是否直观避免嵌套三元、深层回调代码组织是否合乎逻辑相关代码分组、清晰的模块边界是否存在应当简化的聪明技巧能否用更少的行数完成1000 行能解决的事写了 10000 行就是失败抽象是否为其复杂度买单不要过早泛化等到第三次使用场景再泛化注释是否能澄清非显而易见的意图但不要注释显而易见的东西是否存在死代码产物no-op 变量_unused、兼容性 shim、或// removed注释是否把一个新的条件分支硬接在了一个无关的流程上这是设计异味而不是可选优化——把逻辑移入它自己的 helper、state 或 policy而不是纠缠进现有路径。是否反复出现针对同一形状的条件判断这预示着缺少一个模型或 dispatcher。临时分支通常会变成永久性债务。仓库佐证Clappr 的命名规范在 AGENTS.md 中有明确约定——布尔值用is*/has*/can*前缀方法用动词类用名词私有成员用_前缀常量用UPPER_SNAKE_CASE。评审命名是否一致时这套约定就是直接可引用的标准。方法体建议控制在约 30 行以内、优先 early return、倾向组合而非继承这些都可作为 Readability 轴的具体判据。3.3 架构Architecture变更是否符合系统的设计是否遵循现有模式还是引入了新模式如果是新模式是否有正当理由是否保持干净的模块边界是否存在应当共享的代码重复依赖是否朝正确方向流动没有循环依赖抽象层级是否恰当不过度工程、不过度耦合这次重构是降低了复杂度还是仅仅挪动了复杂度数一数读者为理解变更需要持有的概念数量。如果更干净的版本并没有改变这个数量那它并不更干净——优先选择让整个分支、模式或层消失的重构而不是把相同逻辑重新集中化。优先删除一个抽象而不是打磨它。特性逻辑是否泄漏进了共享或通用模块把逻辑留在归属层复用现有的 canonical helper 而不是近似副本不要纵容架构漂移。类型边界是否明确质疑随意的any/unknown/optional/cast 和掩盖不明确不变量的静默回退——让边界显式化通常能简化周围的控制流。仓库佐证Clappr 是 Lerna Yarn workspaces 的 monorepo见根 package.json模块边界清晰clappr/core提供 Player、Core、Container、Playback 核心架构clappr/plugins承载官方插件clappr/player是公开入口 bundle另有hlsjs-playback、dash-shaka-playback、html5-tvs-playback、clappr-zepto、clappr-telemetry等包。架构评审中特性逻辑是否放进了共享模块这一问在这个 monorepo 里可以直接落到具体包归属上判断。TypeScript 边界规范在 AGENTS.md 中同样有据可查避免any、使用unknown、接口定义形状、类型守卫、类型贴近使用处。3.4 安全Security变更是否引入了漏洞聚焦这个 monorepo 的攻击面web/TV 播放器、插件、DOM、广告 SDK、配置和依赖这些在 diff 中绝不能接受Never accept in these diffs提交了 secrets、tokens 或.env文件tokens 或凭据存放在localStorage应放在内存中eval()、Function构造函数、或对不可信输入使用innerHTML/ 不安全 HTML敏感数据出现在日志、错误消息或 URL 中未校验event.origin和消息结构的postMessage使用targetOrigin: *发送当 diff 触及以下主题时必须检查Always check when the diff touches the subject用户或第三方内容广告、播放列表、远程配置是否针对正确上下文HTML、JS、URL做了消毒/编码从不可信来源合并对象时是否带入了危险键__proto__、constructor、prototype来自 API、日志、配置和外部 SDK 的数据是否在边界处被视为不可信、直到被验证新增依赖 / 升级可信来源、yarn audit无可达的 critical/high 且无缓解措施、yarn.lockdiff 被评审如果 secret 已经泄漏进历史先轮换rotate只删掉那一行是不够的仓库佐证这条轴与 Clappr 的工程防线高度一致。根 eslint.config.js 把no-eval、no-implied-eval、no-new-func、no-obj-calls、no-proto等设为 error 级规则从静态检查层面堵死eval/Function构造器路径AGENTS.md 的 Judgment boundaries 也明确列出 NEVER 清单不提交 secrets/tokens/.env、不用eval()或Function构造器、不把 token 放localStorage优先 httpOnly cookies 或内存、不对不可信用户输入用innerHTML、不在 console/错误/URL 中记录敏感数据。评审者拿着这条清单就可以逐项对照 diff。3.5 性能Performance评估 diff 中可见的静态模式针对这个 web/TV monorepo清理与内存Cleanup and memorytimers、listeners、observers、WebSockets、workers、媒体元素、MediaSource、Blob URL 是否在 destroy/unload 时被释放没有上限或 TTL 的缓存/集合是否会在长时间会话中无限增长是否存在对 DOM、缓冲区或大闭包的明显保留在 Tizen/WebOS 上尤为相关热路径Hot paths高频 handlertimeupdate、progress、metrics、媒体控制中是否有重同步工作或 layout thrashing在循环中读写布局每秒被调用多次的路径上是否有大分配或不必要的 O(n) 工作播放器热路径上是否有无明确必要的新依赖或新代码React仅在 React 存在的地方app-frames、player-tvs-native检查不必要的重渲染——不要把 memoization 当作这个 monorepo 的默认审查轴仓库佐证热路径审查在 Clappr 源码里有非常直观的证据链。播放器事件系统中PLAYBACK_TIMEUPDATE playback:timeupdate、PLAYBACK_PROGRESS playback:progress、CONTAINER_TIMEUPDATE、CONTAINER_PROGRESS等高频事件定义在 packages/clappr-core/src/base/events/events.js如第 358、400、848、862 行HTML5 播放器把 DOM 事件映射为timeupdate: _onTimeUpdate、progress: _onProgress见 packages/clappr-core/src/playbacks/html5_video/html5_video.js 的事件映射表而 packages/clappr-core/src/components/log/log.js 的EXCLUDE_LIST明确排除了timeupdate、playback:timeupdate、playback:progress、container:hover、container:timeupdate、container:progress、core:mousemove——因为这些事件每秒触发多次打日志本身就是一种性能损耗。评审时质疑在 timeupdate 上无节流地挂 listener 会怎样仓库已经给出了这些事件就是高频噪音的事实依据。四、结构性修复建议不只是指出问题当你标记一个结构性问题时要提出迁移方案the move——而不仅仅是问题。只说这很复杂的评审让作者无从下手。使用具名的重构方案用类型化模型或显式 dispatcher 替换条件判断链。把重复分支合并成单一更清晰的流程。把编排orchestration与业务逻辑分离让各自可独立阅读。把特性逻辑从共享模块移出放进拥有该概念的包中。复用 canonical helper而不是自造的近似副本。让类型边界显式化使下游分支消失。删除仅增加间接性而不澄清 API 的透传包装器pass-through wrapper。提取 helper或把大文件拆分为聚焦的模块。优先选择移除活动部件的修复方案而不是把同样的复杂度摊开的方案。五、变更规模控制评审的第一道防线小而聚焦的变更更容易评审、合并更快、部署更安全。目标基准~100 行变更 → 良好。一次坐下即可评审完。 ~300 行变更 → 可接受前提是它是一个单一逻辑变更。 ~1000 行变更 → 太大。拆分它。注意文件大小而不只是 diff 大小。小 diff 也可能把文件推过健康边界——单个文件约 1000总行区别于上面约 1000变更行的阈值是常见的检查信号不是硬性上限。当一项变更实质性增大了一个已很大的文件时先问是否应先提取 helpers、子组件或模块再往上堆。先分解再加。什么算一个变更一个自包含的修改解决一件事、包含相关测试、提交后系统保持可用。是一个功能的一部分——而不是整个功能。变更过大时的拆分策略策略做法适用时机Stack堆叠提交一个小变更基于它开始下一个存在顺序依赖By file group按文件组为需要不同评审者的组分别提交变更跨领域关注点Horizontal水平先创建共享代码/桩再创建消费者分层架构Vertical垂直把功能拆成更小的全栈切片功能型工作大变更何时可接受完整的文件删除和自动化重构——此时评审者只需要验证意图而不是逐行验证。把重构与功能工作分开。同时重构现有代码和新增行为的变更等于两个变更——分开提交。小的清理如变量重命名可由评审者酌情并入。六、评审流程四步走Step 1理解上下文看代码之前先理解意图- 这个变更想达成什么 - 它实现的是哪个规格或任务 - 预期的行为变化是什么Step 2先评审测试测试揭示了意图和覆盖度- 变更是否有测试 - 它们测试的是行为而非实现细节吗 - 边界情况是否被覆盖 - 测试命名是否有描述性 - 如果代码变了测试能抓住回归吗仓库佐证Clappr 的测试基座在 vitest.config.base.mjs——每个包共享的 Vitest 配置jsdom 环境、globals、v8 覆盖率并通过defineClapprVitest工厂支持 smoke 测试**/dist.smoke.test.js与常规单测的分流。根package.json的yarn test会先lerna run test --no-bail跑所有定义了 test 脚本的包再对test/目录做一次根级vitest run。评审测试是否存在、是否覆盖时yarn test、yarn test:smokedist 产物冒烟CI 在yarn build:dist后执行、以及单文件vitest run src/path/to/test.test.js都是可直接验证的手段。Step 3评审实现带着五个维度走一遍代码对每个变更文件 1. 正确性这段代码是否做了测试声称它应做的事 2. 可读性不需要帮助我能理解它吗 3. 架构它适配系统吗 4. 安全有任何漏洞吗 5. 性能有任何瓶颈吗Step 4对发现分级只有两级且只有两级级别含义作者动作Required阻塞合并合并前修复。Bug、回归、漏洞、遗留死代码、合并后会被常态化的结构性债务。Optional建议值得考虑由作者决定。绝不阻塞。Required 之上没有第三级。如果它阻塞合并它就是 Required——严重程度体现在发现的排序和解释里而不是一个额外的标签。最多 3 个 Optional选杠杆最高的那些。一长串小建议会稀释重点第 4 个 Optional 消耗作者注意力的成本大于它带来的价值。用最重要的事开头。按杠杆排序发现正确性和安全优先然后是结构性回归和被错过的简化再是其他。不要把真正的问题埋在化妆性的 Optional 之下——少数几个高置信度的发现胜过一长串清单。如果有一个结构性问题加十个琐碎点那么结构性问题本身就是这份评审。Step 5验证作者的验证故事- 跑了哪些测试 - 构建通过了吗 - 变更是否被手动测试过 - UI 变更是否有截图 - 有 before/after 对比吗七、死代码卫生任何重构或实现变更之后检查孤儿代码现在不可达的片段、no-op 变量、兼容性 shim、被注释掉的为了测试的 early-return。发现的死代码与其他发现一样是 Required 级别——不是报告末尾的单独一节也不是全大写的块。它遵循与其他发现相同的结构删除与否的问题放进 SuggestionSuggestion:removeformatLegacyDate()— replaced byformatDate()with no remaining references. I can delete it, or would you rather do it on the branch?不要留着死代码——它会迷惑未来的读者和 Agent。但也不要默默删除你不确定的东西。拿不准时问。八、评审中的诚实评审代码时——无论评的是自己的、另一个 Agent 的、还是人类的不要橡皮图章。没有评审证据的 LGTM 对任何人都没有帮助。不要软化真问题。这可能是个小问题——当它是个会打到生产环境的 bug 时——是不诚实的。尽可能量化问题。这个监听器挂在每个timeupdate上且没有节流会在主线程上每秒触发几十次比这可能有点慢更好。对有明确问题的方案要直接反驳。迎合sycophancy是评审中的一种失败模式。如果实现有问题直说并提出替代方案。优雅地接受否决。如果作者拥有完整上下文且不同意尊重他们的判断。评论代码不要评论人——把针对个人的批评重构为聚焦代码本身。九、依赖纪律评审的一部分代码评审的一部分是依赖评审在添加任何依赖之前现有技术栈能否解决这个问题通常可以。依赖有多大检查 bundle 影响。它是否在积极维护检查最近提交、开放 issue。它是否有已知漏洞yarn audit许可证是什么必须与项目兼容。规则优先使用标准库和现有工具而不是新依赖。每个依赖都是一份负债。升级现有依赖和其他代码变更一样而风险最高的升级是那种以 bump deps 一句话批量合并的。用同样的纪律评审它们读 changelog而不仅是版本号。Semver 是维护者可能并未兑现的承诺——一个 patch 也可能携带行为变更。对 major 升级读迁移说明找出会破坏什么。每次只升级一个依赖。单独或按小的相关组升级并合并。当批量 bump 弄坏构建时你不知道是哪个包干的单包变更让原因显而易见、回滚干净。让测试做决定。升级由前后都是绿套件验证而不是它装上了。如果依赖行为附近的覆盖很薄那个缺口才是真正的发现——先加测试。留意传递依赖图。大多数已安装的包不是任何人直接选的。评审 lockfile diff而不只是package.json一次直接升级可能拉入几十个间接变更。让 lockfile 保持诚实。提交它、评审它的 diff、绝不手改。lockfile 才是真正固定上线内容的。yarn audit/ 供应链分级runtime/build/deploy 可达的 critical/high → 阻塞直到升级、打补丁或替换不可达 → 尽快修复并记录。生产环境中的 moderate → 下个周期仅 dev → 积压。绝不盲目强制修复yarn audit --force或等价物。对新依赖审查 typosquat、发布时间和所有权。本 monorepo 的实际情况Yarn 1.22根目录单一yarn.lock用yarn install --frozen-lockfile可复现安装——根 package.json 的packageManager字段明确固定为yarn1.22.21engines.node要求24。评审 lockfile diff 时根目录单一 yarn.lock意味着任何包升级的间接影响都集中在这一个文件里可查。十、评审清单Review checklist## Review: [PR/change title] ### Context - [ ] I understand what this change does and why ### Correctness - [ ] The change matches the spec/task requirements - [ ] Edge cases handled - [ ] Error paths handled - [ ] Tests cover the change adequately ### Readability - [ ] Names are clear and consistent - [ ] Logic is straightforward - [ ] No unnecessary complexity ### Architecture - [ ] Follows existing patterns - [ ] No unnecessary coupling or dependencies - [ ] Appropriate abstraction level - [ ] Refactors reduce complexity rather than relocate it - [ ] No feature logic in shared modules; file stays within a healthy size ### Security - [ ] No secrets in code, logs, or localStorage for tokens - [ ] No eval / Function / unsafe innerHTML - [ ] postMessage validates origin and payload; no targetOrigin: * - [ ] Config/object merging free of prototype pollution - [ ] External data (APIs, ads, playlist) treated as untrusted - [ ] Dependencies / lockfile reviewed (yarn audit when there is a bump) ### Performance - [ ] Resources released on destroy/unload (timers, listeners, media, Blob URLs, MediaSource) - [ ] No heavy work or layout thrashing in hot paths (timeupdate, progress, metrics) - [ ] Caches/collections bounded or with TTL; no obvious DOM/buffer retention - [ ] Bundle / deps in the hot path justified ### Verification - [ ] Tests pass - [ ] Build succeeds - [ ] Manual verification done (if applicable) ### Verdict - [ ] **Approve** — Ready to merge - [ ] **Request changes** — There are open Required findings十一、常见合理化借口Common rationalizations评审者经常遇到作者或自己的合理化借口。逐条对质合理化借口现实它能跑就够了能跑但不可读、不安全或架构错误的代码会产生复合增长的债务。我写的所以我知道它是对的作者对自身的假设是盲目的。每次变更都受益于另一双眼睛。我们以后再清理以后永远不会来。评审就是质量门禁——用它。要求合并前清理而不是合并后。AI 生成的代码应该没问题AI 代码需要更多审视而不是更少。它自信且看似合理即使错了也如此。测试过了所以没问题测试必要但不充分。它们抓不到架构、安全或可读性问题。这次重构让它更干净了挪动复杂度不是降低复杂度。如果读者要持有的概念数量没变结构就没改善——去找让分支消失的版本。只是给这个文件加一小块小 diff 照样能把文件推过健康线把分支硬接在无关流程上。判断最终结构而不是 diff 大小。只是升个版本号一次升级就是你没写的行为变更。读 changelogsemver 不保证无破坏。一次 PR 全升完省时间批量 bump 弄坏构建时会掩盖是哪个包干的。每次一个依赖让原因和回滚都干净。十二、红旗Red flags没有任何评审就被合并的 PR只检查测试是否通过的评审忽略其他维度没有实际评审证据的 LGTM安全敏感变更没有进行安全聚焦的评审大到没法好好评审的大 PR拆分它们没有回归测试的 bug 修复不带级别的发现——不清楚什么阻塞合并、什么只是建议把症状、修复和测试建议堆在一个段落里的发现接受我以后再修——它永远不会发生只是搬动代码、却没有减少读者需持有概念数量的重构增大本已很大的文件而不是分解它的变更散落到无关代码路径的新条件分支缺失的抽象复制现有 canonical helper 的自造 helper或放进共享模块的特性逻辑没有 changelog 评审、没有按包隔离的批量 bump dependencies PR手改、未提交、或未经 diff 评审就合并的 lockfile 变更十三、评审后验证Verification评审完成后所有 Required 发现已解决或已明确延期且有正当理由测试通过构建成功验证故事有文档记录改了什么、如何验证的依赖升级已对照其 changelog 评审、按包隔离并由绿套件验证lockfile diff 已评审推定阻塞项Presumptive blockers对以下每一项先提出并建议更简单的设计只有当变更主动让结构更差时才升级为 Required把复杂度搬来搬去而非降低的重构把文件推过大小边界且未分解的变更加进共享模块的特性逻辑现有 canonical helper 的近似副本掩盖不明确不变量的静默回退。十四、输出格式评审是被人读的评审的读者是初级到高级的工程师几乎总是在终端或 PR 标签页里读它。把症状、机制、修复和测试建议堆进一个密集段落无论多正确都不可读。教学性didactics是这项技能的差异化优势。如果作者要读两遍一个发现才能理解问题是什么那评审即使是对的也失败了。14.1 报告结构四个部分按此顺序。除此之外不要有任何内容。## Review: [PR/change title] **Verdict:** APPROVE | REQUEST CHANGES [Overview in 1-2 sentences: what the change does and the overall assessment.] ### Required [numbered findings] ### Optional [at most 3, numbered] ### Verification - **Tests reviewed:** yes/no — [half a line] - **Build verified:** yes/no — [half a line] - **Security verified:** yes/no — [half a line]结论规则有任何 Required 发现 → REQUEST CHANGES。没有 → APPROVE。如果任一列表为空写 None found 并继续。没有表扬区——变更做对的地方放进 overview用一句话在合适的时候。一长串积极点会撑大报告而没人读它。14.2 单个发现的解剖Anatomy of a finding四个部分按此顺序各部分之间用空行分隔#### 1. [Title: the problem in at most 8 words] [Didactic explanation. 2 to 4 sentences. One idea per paragraph — if you have two, split them. Write for someone who does not know this stretch of code: say what happens and why it breaks, not just the symptoms name.] file:line js // the code as it is today, 3 to 8 lines // just enough to see the problem without opening the file **Suggestion:** [the direction of the fix, in one sentence. Do not write the patch.]代码块在 Required 发现上是必需的在 Optional 上是可选的——当代码片段本身就是论证时包含它当问题是结构性的、代码本身展示不出什么时省略它。14.3 硬格式规则每个部分之间都有空行。标题、解释、file:line、代码和 Suggestion 绝不紧贴在一起。绝不在同一段落里堆叠症状 修复 测试建议。症状和机制进解释修复进 Suggestion缺失的测试是它自己的发现而不是另一个发现的附录。每段最多 3 句话。超了就拆。一个发现是一个####标题块不是 bullet。bullet 会变成一长串 run-on 段落。每个发现一个file:line。如果同一问题出现在三处引用主要的那处在解释里提及其余——不要往标题里贴a.js:65,135,194。Suggestion 只有一句。一句放不下说明该发现混了两个问题——拆分它们。给发现编号让作者能说关于第 3 条……。写全称。PLAYBACK_READYarrives after the resource 比 PR res 好。项目行话可以临时编造的缩写不行。强调不用全大写除了上面四部分之外不发明新章节。14.4 内容规则先评审测试——它们揭示意图和覆盖度评审代码前先读规格或任务描述每个 Required 发现都要在 Suggestion 里带来修复方向不要在有未解决 Required 发现时批准代码不确定就说出来并建议调查而不是猜测14.5 完整示例Complete example这是要复制的模式——注意间距和每部分的体量## Review: DASH live captions (Shaka and Tizen providers) **Verdict:** REQUEST CHANGES Splitting Shaka and Tizen into per-playback providers is the right direction: it takes the if (tizen) / if (shaka) out of the sidecar flow. But the gate that separates live from VOD is incomplete, and an early-return was commented out for testing, so the merge would regress DASH VOD and HLS live. ### Required #### 1. DASH VOD enters the live flow The code decides between downloading the sidecar caption and using the players text tracks by asking only is this resource DASH?. The second half of the question is missing: is it DASH **live**?. On DASH VOD with Shaka, PLAYBACK_READY arrives after the resource. When that happens the provider runs again, emits WM_SUBTITLE_AVAILABLE with Shakas tracks, and replaces the sidecar that had already loaded — on screen, the language list swaps by itself. subtitle_loader.js:65 js if (this.isDashResource(resource)) { this.setupDashSubtitles() } **Suggestion:** use isDashLivePlayback — the predicate onResourceReady already uses — in setupDashSubtitles, onSubtitleChanged, and the PLAYBACK_READY listener as well. #### 2. The users chosen caption snaps back to default Shaka fires trackschanged several times during a live broadcast, not only on the initial load. On every fire the provider reapplies the initial caption and re-emits the menu. The track objects that arrive on that event have no selection mark. Anyone who picked English is put back on the default mid-session without touching anything. The Tizen provider already guards against this by storing track ids; the Shaka one does not. shaka_dash_live_subtitle_provider.js:68 js this._player.addEventListener(trackschanged, () { this.setDashTracks() this.setInitialSubtitle() }) **Suggestion:** always update the track map, but call setInitialSubtitle only the first time and keep the current selection while the id still exists. #### 3. Commented-out early-return for testing shipped The return that kept the language menu from mounting on live broadcasts is commented out, not removed. Commented this way, it opens the menu for **all** live — HLS included — not only the DASH live this branch is meant to cover. The new tests still call bindContainerEvents() by hand, which means they were written to work around the return, not to validate removing it. language_menu_tv.js:139 js onMetadataLoaded(video) { this.video video //if (this.video?.isLive) return this.bindContainerEvents() this.renderPlugin() } **Suggestion:** delete the commented line and put an explicit DASH-live gate in its place — I can do that, or would you rather adjust it on the branch? #### 4. Setup retry runs forever, four times per second When the provider does not exist yet, scheduleDashSetup schedules a new 250ms timer. That timer calls back into the same function, which schedules another — no counter and no stop condition. On any DASH whose playback is neither Shaka nor Tizen the provider never appears, and the cycle continues until destroy. On an entry-level TV that is constant main-thread work for the whole session. subtitle_loader.js:152 js scheduleDashSetup() { clearTimeout(this._dashSetupTimer) this._dashSetupTimer setTimeout(() this.setupDashSubtitles(), 250) } **Suggestion:** cap the number of attempts and log once when they run out, instead of rescheduling forever. ### Optional #### 1. pt-BR is normalized in Shaka but not in Tizen The Shaka provider turns pt-BR into pt before matching the language map; the Tizen provider uses the raw value from AVPlay. It works today by accident, because AVPlay delivers por, which is also in the map. shaka_dash_live_subtitle_provider.js:321 **Suggestion:** extract a normalizeLanguage and use it in both providers. #### 2. One invalid extra_info wipes every track Parsing Tizens track list wraps JSON.parse of all of them in a single try. If one track comes with malformed extra_info, the exception also discards the valid tracks that had already been read. tizen_dash_live_subtitle_provider.js:388 **Suggestion:** move the try inside the loop, dropping only the bad track. ### Verification - **Tests reviewed:** yes — they cover flow choice and the menu label; they do not cover DASH VOD or selection preservation. - **Build verified:** no — static review of the remote branch, working tree on main. - **Security verified:** yes — no secrets, eval, postMessage, or new deps; the native cue goes to the DOM with .text().十五、把技能接回 Clappr 的工作流这份技能不是孤立存在的它与仓库的工程纪律互为印证触发入口AGENTS.md 要求 PR 评审、代码评审、任何结构化的合并前反馈都要先读并执行 .agents/skills/code-review/SKILL.md提交阶段则执行 .agents/skills/commit/SKILL.md先检查分支、按 Conventional Commits 格式、英文消息——评审与提交形成前后两道关卡。安全防线评审中绝不接受的清单eval、localStorage token、innerHTML、postMessage origin与 eslint.config.js 的no-eval/no-implied-eval/no-new-func等规则、AGENTS.md 的 NEVER 清单三处一致评审者可直接引用。性能佐证热路径检查可落到 events.js 的高频事件定义、html5_video.js 的 DOM 事件映射、以及 log.js 对 timeupdate/progress 的日志豁免——每秒多次触发不是抽象说法而是源码明示的事实。工具链验证yarn lint含根级 eslint.config.js、yarn testLerna 逐包 根级 vitest、yarn test:smokedist 冒烟、yarn build等脚本定义在根 package.json是评审中验证验证故事的直接执行入口提交信息的合规性由 .commitlintrc.js .husky/commit-msg 在 Husky 钩子里强制。掌握这套五轴评审模型后你可以在 Clappr 的任何 PR 或 Agent 产出上执行一次完整、可引用、教学性的评审先拿全 diff再按正确性、可读性、架构、安全、性能逐项核查用 Required/Optional 两级输出结论最后用四段式报告把哪里坏了、为什么坏、往哪修讲清楚——这正是让代码库在每次合并时都变得更健康的机制。赞分享前端音视频插件系统【免费下载链接】clapprAn extensible, plugin-oriented, HTML5-first media player for the web项目地址https://gitcode.com/gh_mirrors/cl/clappr点击查看免费下载相关推荐Penpot 仓库代码评审规范五轴评审方法、严重度分级与合并前质量门禁实践Penpot 仓库代码评审规范五轴评审方法、严重度分级与合并前质量门禁实践 导读 本文基于 Penpot 仓库 .opencode 技能体系中用于 合并前评审前端设计系统图形学协同办公Sanity 代码评审质量门禁五轴评审方法、变更规模管控与人机协作的多模型评审实践Sanity 代码评审质量门禁五轴评审方法、变更规模管控与人机协作的多模型评审实践 本文以 Sanity 仓库中的 code review and qualiCMS前端awesome-blender Blender资源清单千余个插件、教程与3D资产一站收齐awesome blender Blender资源清单千余个插件、教程与3D资产一站收齐 找流体模拟插件翻遍搜索结果挑不中找CC0纹理要在六个网站之间来回跳文档教程上一篇3步打造逼真金属质感MuJoCo物理引擎高级渲染技术指南下一篇MuJoCo Unity插件在WebGL构建中的技术挑战与解决方案创作声明：本文部分内容由AI辅助生成（AIGC），仅供参考
