Fix issue #22 course visibility and replacement drop#24
Merged
Conversation
ceilf6
force-pushed
the
fix/issue-22-course-loading-replace-drop-master
branch
from
bb8a24e to
878ac28
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 评论
决策摘要: 换课退选修复方向正确，直接读取右侧已选课程列表以定位退选按钮，是对 issue #22 第 3 点的合理实现；但新回归测试只覆盖 helper 的成功路径，未验证真正修复的退选流程集成，且 issue #22 第 1、2 点未落地为代码修复，需要评审者确认覆盖预期后合并。
级联分析
- 变更符号:
dropCourse流程内退选按钮查找逻辑（新增findSelectedCourseRows、findClickableElementByText、isElementClickable） - 受影响流程: 换课/骑驴找马退选路径（
dropCourse），原教学班查找findAllTeachingClasses降级为 fallback - 变更集外调用方:
dropCourse由换课编排调用；findAllTeachingClasses仍被选课主流程复用，本 PR 未改其语义，风险低 - 置信度: medium（未提供代码图谱，基于 diff 与文本判断；
dropCourse调用方与换课编排未在 diff 中完整呈现）
问题发现
-
[中] 回归测试未覆盖实际修复的退选集成路径
- 证据:
tests/courseGrabber-selected-courses.test.mjs仅用文本抽取findSelectedCourseRows等 helper 并断言成功返回row/button；真正被修复的dropCourse分支（右侧列表优先、fallback 到findAllTeachingClasses、dropClass.button || findClickableElementByText）没有测试。 - 受影响调用方/流程: 换课退选流程。若 helper 正确但
dropCourse内 fallback 或button传递逻辑回归，测试无法捕获。 - 最小可行修复: 增加一个用例，验证右侧列表命中时 fallback 不被调用，以及右侧列表未命中时降级到教学班查找仍能定位退选按钮；至少覆盖
findSelectedCourseRows返回空数组的路径。
- 证据:
-
[低] issue #22 第 1、2 点未在本 PR 落地为代码修复
-
[低]
findSelectedCourseRows在非课程号输入时可能匹配过宽- 证据: 当
isCode为 false 时，section 匹配依赖headingText.includes(input)，课程名简称/同名课可能命中多个 section，返回多行；dropCourse只取selectedCourseRows[0]，可能退选到非预期教学班。docs 已建议用课程号，但代码未防护。 - 受影响调用方/流程: 用课程名配置替换课程的换课场景。
- 最小可行修复: 非课程号且匹配到多行时记录 warning 或要求更精确匹配，避免静默退选首个命中行。
- 证据: 当
行级发现
- [courseGrabber.js:667]
findClickableElementByText用querySelectorAll("*")遍历全部后代并做textContent.includes，父容器 textContent 会包含子节点“退选”文本，可能先命中外层可点击容器而非真正的退选按钮；建议优先返回文本最贴近目标且自身可点击的最内层元素。 - [courseGrabber.js:701] 非课程号分支仅靠
headingText.includes(input)匹配 section，简称/同名课会匹配多 section，配合下游只取首行存在退错课风险；建议对多命中情况告警。 - [courseGrabber.js:988] 右侧列表命中后直接
dropClass = selectedCourseRows[0]，未校验其button是否存在即进入后续；虽后面有dropClass.button || findClickableElementByText(row, "退选")兜底，但当 row 含“退选”文本却无可点击按钮时会走到未找到退选按钮分支，行为可接受，建议确认这是预期降级。 - [tests/courseGrabber-selected-courses.test.mjs:34] 通过
Function()文本抽取 helper 执行，绕过了dropCourse真实集成；测试通过不能证明修复在完整流程中生效，建议补充集成向用例或直接导出被测函数。
Karpathy 评审
- 假设: 假设已选课程存在于
.outer_xkxx_list/[id^="right_"]/input[name="right_kchid"]结构中；这是基于单一学校 DOM 的推断，跨校兼容未验证。 - 简洁性: 新增 helper 拆分合理，
isElementClickable与findClickableElementByText复用了原内联逻辑，属于正向抽取；findSelectedCourseRows中 section 匹配存在两段并行扫描（selectedSections+rightCourseInputs），有一定冗余但可接受。 - 结构质量: 将退选按钮查找从内联循环提取为具名 helper 降低了理解成本，
dropCourse引入清晰的“右侧列表优先、教学班 fallback”结构，无明显退化。 - 变更范围: README/docs 变更与代码修复目标一致，属于对 issue 第 1、2 点的文档缓解，范围可控，无无关重构。
- 验证:
node --check与node --test已运行，但测试只覆盖 helper 成功路径，未覆盖 fallback、空结果、多命中和实际dropCourse集成。
缺失覆盖
dropCourse集成测试：右侧列表命中时不走 fallback；右侧列表为空时降级到findAllTeachingClasses并定位退选按钮。findSelectedCourseRows返回空数组的场景（目标课程不在已选列表）。- 非课程号输入匹配到多个 section/行时的行为验证。
findClickableElementByText在嵌套可点击容器下返回最内层按钮的验证。
| continue; | ||
| } | ||
|
|
||
| if (excludedTexts.some((text) => elementText.includes(text))) { |
Contributor
There was a problem hiding this comment.
findClickableElementByText 用 querySelectorAll("*") 遍历全部后代并做 textContent.includes，父容器 textContent 会包含子节点“退选”文本，可能先命中外层可点击容器而非真正的退选按钮；建议优先返回文本最贴近目标且自身可点击的最内层元素。
| const dropButton = findClickableElementByText(row, "退选"); | ||
| const rowText = row.textContent || ""; | ||
| if (!dropButton && !rowText.includes("退选")) { | ||
| continue; |
Contributor
There was a problem hiding this comment.
非课程号分支仅靠 headingText.includes(input) 匹配 section，简称/同名课会匹配多 section，配合下游只取首行存在退错课风险；建议对多命中情况告警。
| return; | ||
| } | ||
| if (dropClass) { | ||
| log(`已在已选课程列表中找到课程 ${courseCode}`, "info", courseCode); |
Contributor
There was a problem hiding this comment.
右侧列表命中后直接 dropClass = selectedCourseRows[0]，未校验其 button 是否存在即进入后续；虽后面有 dropClass.button || findClickableElementByText(row, "退选") 兜底，但当 row 含“退选”文本却无可点击按钮时会走到 未找到退选按钮 分支，行为可接受，建议确认这是预期降级。
| ].join("\n"); | ||
|
|
||
| return Function(`${helperSource}\nreturn { findSelectedCourseRows };`)(); | ||
| } |
Contributor
There was a problem hiding this comment.
通过 Function() 文本抽取 helper 执行，绕过了 dropCourse 真实集成；测试通过不能证明修复在完整流程中生效，建议补充集成向用例或直接导出被测函数。
ceilf6
force-pushed
the
fix/issue-22-course-loading-replace-drop-master
branch
from
878ac28 to
a7d6a16
Compare
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 评论
决策摘要: 核心退选修复方向正确、可读性提升明显且有针对性回归测试，可在澄清一处 name-match 语义后合并；剩余风险集中在回退分支的按钮定位和多匹配行为。
级联分析
- 变更符号:
dropCourse（重写查找逻辑）、新增isElementClickable、findClickableElementByText、findSelectedCourseRows - 受影响流程: 退选流程、"骑驴找马"换课流程（先退旧课再抢新课）、依赖
dropCourse的替换课程逻辑 - 变更集外调用方:
dropCourse的换课调用方（issue #22 第 3 点核心场景）；findAllTeachingClasses仍作为回退保留，旧路径未破坏 - 置信度: medium（仅有 diff 与 DOM mock 测试，无真实正方页面 DOM 或完整代码图谱验证选择器假设）
问题发现
-
[中] name-match 多匹配时仍会退选第一门课
- 证据:
courseGrabber.js:1010-1045中，当!isCourseCode(courseCode)且selectedCourseRows.length > 1时，仅log(... "warning")，随后仍执行selectedCourseRows.find((selectedRow) => selectedRow.button)并点击。测试dropCourse warns when a course-name replacement matches multiple selected rows明确断言first.dropButton.clicked === true。 - 受影响调用方/流程: 换课流程使用课程名作为替换课程时，可能误退一门用户不想退的课；这正是不可逆的退选操作，与 issue #22 换课风险相关。
- 最小可行修复: 对 name-match 多匹配情况改为不执行退选并
resolve(false)，要求用户改用课程号；或至少让此行为成为文档中承诺的显式约束，而不是"warning 后照退"。当前 warning + 仍退选的语义应确认是否为设计意图。
- 证据:
-
[低] 回退分支重复调用
findClickableElementByText- 证据:
courseGrabber.js:1030-1044回退分支构造dropClass = { ...tc, button: findClickableElementByText(tc.row, "退选") }，随后courseGrabber.js:1057-1058又执行dropClass.button || findClickableElementByText(row, "退选")。回退路径对同一 row 计算了两次按钮。 - 受影响调用方/流程: 仅性能/清晰度，无正确性影响。
- 最小可行修复: 回退分支只判断
rowText.includes("退选")并设dropClass = tc，把按钮定位统一交给1057-1058处的收敛逻辑，避免两处重复。
- 证据:
行级发现
- [courseGrabber.js:1013] name-match 多匹配时仅 warning 后仍点击第一门课的退选按钮，属不可逆操作；建议此分支直接
resolve(false)或将"仍退第一门"确认为显式契约。 - [courseGrabber.js:1032] 回退分支已计算
button: findClickableElementByText(tc.row, "退选")，与下方 1057 行的dropClass.button || findClickableElementByText(row, "退选")重复定位同一按钮；建议回退分支只赋dropClass = tc，按钮定位收敛到一处。
Karpathy 评审
- 假设: 选择器假设（
.outer_xkxx_list、[id^="right_"]、input[name="right_kchid"]、h6含(课程号)）来自单一正方页面结构，未在真实 DOM 或覆盖记录中交叉验证；README 已声明依赖页面结构，属可接受的已知限制，但选择器脆弱性应记为剩余风险。 - 简洁性: 抽出
isElementClickable和findClickableElementByText消除了原内联的重复按钮扫描，是有效的复杂度删除，非猜测性抽象；findSelectedCourseRows有两条查找路径（section 遍历 +right_kchidinput 兜底）稍有重叠，但都服务真实 DOM 变体，可接受。 - 结构质量: 逻辑放在拥有该概念的选课模块内，helper 复用度良好，未泄漏到调用方；未跨越文件规模边界。
- 变更范围: README/docs 变更与 issue #22 第 1、2 点（可见性、刷新、查看更多）直接对应，非无关噪声；代码变更聚焦第 3 点退选修复。范围克制。
- 验证:
node --test覆盖 selected-row 命中、回退到教学班、内层可点击按钮优先、多匹配 warning 四条路径，覆盖了变更契约的主要分支。缺口见下。
缺失覆盖
- 缺少"selected-course rows 存在但均无可点击 button"（
selectedCourseRows[0]命中但.button为 null）时，1057-1058回退到findClickableElementByText(row, "退选")仍失败并resolve(false)的测试。 - 缺少 name-match 多匹配场景下"期望不退选/退选哪一门"的行为断言——当前测试固化了"退第一门"，但该行为本身正是发现 1 的争议点，应在确认语义后补对应断言。
- 无真实正方页面 DOM 快照或兼容性证据验证
findSelectedCourseRows的选择器，选择器漂移风险未被自动化覆盖（受当前测试策略限制，属可接受的剩余风险）。
| const selectedCourseRows = findSelectedCourseRows(courseCode); | ||
| if (!isCourseCode(courseCode) && selectedCourseRows.length > 1) { | ||
| log( | ||
| `替换课程 ${courseCode} 匹配到多个已选课程，建议改用课程号避免误退`, |
Contributor
There was a problem hiding this comment.
name-match 多匹配时仅 warning 后仍点击第一门课的退选按钮，属不可逆操作；建议此分支直接 resolve(false) 或将"仍退第一门"确认为显式契约。
|
|
||
| if (teachingClasses.length === 0) { | ||
| log(`未找到课程 ${courseCode} 的教学班`, "warning", courseCode); | ||
| resolve(false); |
Contributor
There was a problem hiding this comment.
回退分支已计算 button: findClickableElementByText(tc.row, "退选")，与下方 1057 行的 dropClass.button || findClickableElementByText(row, "退选") 重复定位同一按钮；建议回退分支只赋 dropClass = tc，按钮定位收敛到一处。
ceilf6
force-pushed
the
fix/issue-22-course-loading-replace-drop-master
branch
from
a7d6a16 to
f7d9a81
Compare
Owner
Author
|
/review 请基于最新提交 f7d9a81 重新评审，重点检查：1. 课程名多匹配时是否已经停止退选并要求使用课程号；2. fallback 分支是否已去掉重复按钮定位；3. 新增测试是否覆盖 dropCourse 集成路径。 |
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 评论
决策摘要: 三个重点关注项均已在 f7d9a81 中正确落实，无破坏调用方或契约漂移的阻塞问题；剩余风险集中在测试脚手架偏离真实运行时行为，以及 fallback 分支的按钮定位仍有次要重复。
级联分析
- 变更符号:
dropCourse（行为改动）、新增isElementClickable/findClickableElementByText/findSelectedCourseRows - 受影响流程: 换课/骑驴找马退选路径；
dropCourse后续依赖模态确认按钮点击流程 - 变更集外调用方:
dropCourse的调用方（换课编排逻辑）未在 diff 中出现；其契约（返回 boolean、失败时 resolve(false)）保持不变，未见破坏 - 置信度: medium（无代码图谱，
dropCourse上游调用点未包含在 diff 中，依据签名与返回形状不变推断兼容）
问题发现
-
[低] 重点项1已满足：课程名多匹配时正确停止退选
- 证据: courseGrabber.js:1011-1019 在
!isCourseCode(courseCode) && selectedCourseRows.length > 1时记录 warning 并resolve(false)，未点击任何退选按钮；测试dropCourse refuses ambiguous course-name replacement matches断言fallbackCalls === 0、两个按钮均未点击、warning 含“匹配到多个已选课程” - 受影响调用方/流程: 换课退选路径，防止误退无关课程
- 最小可行修复: 无需修改
- 证据: courseGrabber.js:1011-1019 在
-
[低] 重点项2已满足：fallback 分支已去除重复的手写按钮遍历
- 证据: courseGrabber.js:1056-1057 将旧的
row.querySelectorAll("*")+ 逐元素判断替换为dropClass.button || findClickableElementByText(row, "退选")，与主路径复用同一 canonical helper，未再复制定位逻辑 - 受影响调用方/流程: 退选按钮定位统一到
findClickableElementByText - 最小可行修复: 无需修改
- 证据: courseGrabber.js:1056-1057 将旧的
-
[中] 重点项3部分满足：dropCourse 集成路径已覆盖，但测试脚手架用简化 setTimeout 掩盖真实时序
- 证据: tests/courseGrabber-selected-courses.test.mjs:44-47 注入
const setTimeout = (callback) => { callback(); return 0; }，把异步模态确认流程改成同步立即执行。真实dropCourse依赖 setTimeout 延迟后再查找/点击确认按钮 - 受影响调用方/流程: 集成测试验证的是被改写时序后的执行链，无法捕获真实运行中确认模态尚未渲染或延迟渲染导致的 race
- 最小可行修复: 在测试注释中标注该简化仅验证控制流分支选择，不覆盖时序；或补充一个模态延迟出现的用例以提升对确认按钮定位的信心
- 证据: tests/courseGrabber-selected-courses.test.mjs:44-47 注入
行级发现
- [tests/courseGrabber-selected-courses.test.mjs:44] 同步化的
setTimeout桩使集成测试无法覆盖真实的延迟确认时序，建议标注该桩的验证边界或补一个延迟渲染确认按钮的用例。 - [tests/courseGrabber-selected-courses.test.mjs:6] 通过字符串抽取函数体再
Function()重组来测试，会在被测函数重命名或引入新的外部依赖时静默失效（extractFunction找不到即抛错，但漏抽依赖时行为会偏离运行时）；作为已知脆弱点接受，但后续新增被测 helper 时需同步维护抽取列表。
Karpathy 评审
- 假设: 假设
dropCourse上游换课编排依赖返回 boolean 且不区分 false 原因（多匹配放弃 vs 未找到按钮），三种失败均 resolve(false)，未见调用方需要区分；产品意图与 issue #22 problem 3 一致 - 简洁性:
findClickableElementByText的候选排序（exact/depth/nativeControl/textLength）合理且解决了嵌套可点击容器优先内层的问题，非过度设计 - 结构质量: 定位逻辑从
dropCourse内联遍历抽为共享 helper，主路径与 fallback 复用，删除了原有重复分支，属于删除复杂度的正向重组 - 变更范围: README 与 usage-notes 文档变更服务于 issue problem 1/2 的“已知限制”定位，与代码目标一致，无无关重构噪声
- 验证: happy path、fallback、多匹配放弃、无可点击按钮四条分支均有测试；缺口在真实异步时序与上游调用方视角
缺失覆盖
dropCourse上游换课编排对resolve(false)三种原因（多匹配放弃 / 无教学班 / 无退选按钮）是否需要差异化处理，未见调用方测试或说明。- 确认模态延迟渲染场景未覆盖（当前测试同步执行 setTimeout），真实页面若模态异步出现，确认按钮定位的可靠性未验证。
- issue problem 1/2 作为“文档化已知限制”处理，符合 PR 声明的 scope，但未有自动化手段验证脚本在课程未加载时给出清晰用户提示（仅文档承诺，非阻塞）。
| extractFunction("findSelectedCourseRows"), | ||
| extractFunction("dropCourse"), | ||
| ].join("\n"); | ||
|
|
Contributor
There was a problem hiding this comment.
同步化的 setTimeout 桩使集成测试无法覆盖真实的延迟确认时序，建议标注该桩的验证边界或补一个延迟渲染确认按钮的用例。
| import test from "node:test"; | ||
|
|
||
| const source = readFileSync(new URL("../courseGrabber.js", import.meta.url), "utf8"); | ||
|
|
Contributor
There was a problem hiding this comment.
通过字符串抽取函数体再 Function() 重组来测试，会在被测函数重命名或引入新的外部依赖时静默失效（extractFunction 找不到即抛错，但漏抽依赖时行为会偏离运行时）；作为已知脆弱点接受，但后续新增被测 helper 时需同步维护抽取列表。
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 评论
决策摘要: Problem 3 的代码修复方向正确且有针对性，回归测试合理，可以合并；但需注意测试对源码结构的脆弱依赖，以及新 helper 与旧 fallback 之间的语义边界。
级联分析
- 变更符号:
dropCourse（改写查找逻辑）、新增isElementClickable、findClickableElementByText、findSelectedCourseRows - 受影响流程: 换课（骑驴找马）退选流程；
dropCourse内部退选按钮定位 - 变更集外调用方:
dropCourse的调用方（换课编排逻辑）契约未变——仍返回Promise<boolean>，无签名或返回形状变化；findAllTeachingClasses保留为 fallback，未破坏原路径 - 置信度: medium（无代码图谱，基于 diff 与测试推断；
dropCourse上游调用点未在 diff 中出现，但返回契约保持不变，风险低）
问题发现
-
[中] 回归测试通过
Function构造器抽取源码字符串，与源码文本结构强耦合- 证据: 测试用
extractFunction(name)按function ${name}(文本匹配并做花括号配平，再用Function(...)重新求值。这依赖courseGrabber.js保持特定函数声明形式（非箭头、非方法简写、名称完全一致）。 - 受影响调用方/流程: 未来对这些 helper 的任何重构（改成箭头函数、方法、重命名、内联）都会让测试在断言前就抛错，产生与被测行为无关的失败信号。
- 最小可行修复: 可接受合并，但建议在测试文件顶部注释说明该抽取机制的前提约束，或后续将这些 helper 显式导出供测试直接 import，降低文本耦合。
- 证据: 测试用
-
[低] 新 helper
findSelectedCourseRows在按课程名匹配时的歧义处理只在dropCourse层拦截- 证据:
findSelectedCourseRows本身对课程名做headingText.includes(input)宽松匹配，可能返回多行；歧义拒绝逻辑放在dropCourse（selectedCourseRows.length > 1时 warning 并resolve(false)）。 - 受影响调用方/流程: 目前
findSelectedCourseRows仅被dropCourse调用，行为正确；但若后续有其他调用方复用该 helper，宽松匹配语义不在 helper 内约束，容易误用。 - 最小可行修复: 非阻塞。若未来扩大复用，考虑把歧义语义下沉到 helper 或在返回值中标注匹配置信度。
- 证据:
行级发现
- [tests/courseGrabber-selected-courses.test.mjs:6] 通过文本匹配
function ${name}(抽取源码再Function()求值，对源码声明形式强依赖；helper 改写为箭头函数/方法或重命名会导致测试在断言前失败。建议注释说明前提，或后续改为显式 export + import。 - [courseGrabber.js:1010] 换课名匹配歧义拒绝依赖
findSelectedCourseRows返回多行；该 helper 对课程名用includes宽松匹配，歧义语义仅在此处拦截，复用时需注意。
Karpathy 评审
- 假设: 修复假设已选课程始终以右侧列表（
input[name="right_kchid"]/.outer_xkxx_list/[id^="right_"]）呈现，并保留旧findAllTeachingClasses作 fallback，假设已通过双路径覆盖，合理。 - 简洁性:
findClickableElementByText的候选排序（exact/depth/nativeControl/textLength）为解决“嵌套可点击容器优先选内层按钮”引入了一定复杂度，但对应真实 DOM 场景且有测试覆盖，未过度设计。 - 结构质量: 抽出
isElementClickable、findClickableElementByText、findSelectedCourseRows消除了原dropCourse内联的按钮扫描重复逻辑，是复杂度的合理下降而非平移；dropCourse分支变多但可读性提升。 - 变更范围: 代码改动聚焦 Problem 3；README/docs 变更对应 Problem 1/2 的已知限制说明，与 issue scope 一致，无无关重构噪声。
- 验证: 覆盖了选中列表命中、fallback、嵌套按钮优选、课程名歧义拒绝、无可点击按钮五种路径，与 issue Problem 3 的退选场景对齐；未见明显 happy-path-only 缺口。
缺失覆盖
- Problem 1（加密/需二次展开才出现选课按钮）和 Problem 2（持续刷新丢失已加载课程）按文档化已知限制处理，未在代码层解决——这是 PR 明确声明的范围决策，合并前应确认维护者接受“仅文档说明、不自动加载 更多”的处理方式。
- 未见
dropCourse上游换课编排在“退选返回 false”时的行为验证；若换课流程依赖退选成功才继续抢课，建议确认resolve(false)路径在编排层被正确处理（本 PR 未改该契约，风险低）。
| import test from "node:test"; | ||
|
|
||
| const source = readFileSync(new URL("../courseGrabber.js", import.meta.url), "utf8"); | ||
|
|
Contributor
There was a problem hiding this comment.
通过文本匹配 function ${name}( 抽取源码再 Function() 求值，对源码声明形式强依赖；helper 改写为箭头函数/方法或重命名会导致测试在断言前失败。建议注释说明前提，或后续改为显式 export + import。
|
|
||
| if (teachingClasses.length === 0) { | ||
| log(`未找到课程 ${courseCode} 的教学班`, "warning", courseCode); | ||
| const selectedCourseRows = findSelectedCourseRows(courseCode); |
Contributor
There was a problem hiding this comment.
换课名匹配歧义拒绝依赖 findSelectedCourseRows 返回多行；该 helper 对课程名用 includes 宽松匹配，歧义语义仅在此处拦截，复用时需注意。
ceilf6
force-pushed
the
fix/issue-22-course-loading-replace-drop-master
branch
from
f7d9a81 to
f100270
Compare
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 评论
决策摘要: Problem 3 的换课退选修复方向正确、有回退和测试覆盖；Problems 1/2 明确按已知限制文档化处理，符合 issue 范围。可合并，但退选按钮的模糊匹配和 fallback 语义存在需关注的正确性风险。
级联分析
- 变更符号:
dropCourse, 新增isElementClickable/findClickableElementByText/findSelectedCourseRows - 受影响流程: 换课（骑驴找马）退选流程、以及所有调用
dropCourse的路径 - 变更集外调用方:
dropCourse的调用点未在 diff 中出现，其返回false分支的处理未验证；findAllTeachingClasses仍作为 fallback 被复用 - 置信度: degraded（无代码图谱，
dropCourse调用方与 UI 集成路径未在 diff 中呈现，仅由抽取式单测覆盖内部分支）
问题发现
-
[中]
findClickableElementByText的 targetText 用includes匹配可能选中错误按钮- 证据:
findClickableElementByText(row, "退选")用elementText.includes(targetText)，若行内同时含“退选”和其他含“退选”字样的元素（如“取消退选”“已退选”提示），排序后仍可能选中非预期可点击元素；excludedTexts默认为空且退选路径未传入。 - 受影响调用方/流程: 换课退选、fallback 退选路径
- 最小可行修复: 对退选场景传入
excludedTexts，或优先精确文本 + 校验 onclick 中包含cancelCourse/退选语义，避免误点。
- 证据:
-
[中] 已选课程行找到但无可点击按钮时不再 fallback，可能倒退于旧行为
- 证据:
dropClass = selectedCourseRows.find(r => r.button) || selectedCourseRows[0] || null。当findSelectedCourseRows命中一个仅含“退选”文本但无 button 的行时，dropClass非空，走到下面dropClass.button || findClickableElementByText(row, "退选")若仍找不到则直接resolve(false)，不会再退回findAllTeachingClasses。旧逻辑会遍历所有教学班。测试returns false when selected rows contain no clickable drop button恰好固化了这一行为。 - 受影响调用方/流程: 已选课程区渲染异常但左侧教学班可退选的页面
- 最小可行修复: 当选中行无可点击按钮时，继续尝试
findAllTeachingClassesfallback，而不是直接失败。
- 证据:
-
[低] 课程名模糊匹配的多命中拒绝仅覆盖已选列表，fallback 路径无同等保护
- 证据:
selectedCourseRows.length > 1的歧义拒绝只在选中列表分支生效；若走findAllTeachingClassesfallback，课程名匹配到多个教学班时无对应告警/拒绝。 - 受影响调用方/流程: 用课程名配置替换课程且命中 fallback 的场景
- 最小可行修复: 文档已建议用课程号；可在 fallback 路径补一致的歧义保护或至少 warning。
- 证据:
行级发现
- [courseGrabber.js:653]
findClickableElementByText用includes(targetText)匹配，退选场景未传excludedTexts，可能选中含“退选”字样的非目标可点击元素；建议对退选传入排除项或校验 onclick 语义。 - [courseGrabber.js:1021] 选中行命中但无可点击按钮时，
dropClass非空导致跳过findAllTeachingClassesfallback，相比旧逻辑可能退步；建议无按钮时继续 fallback。 - [tests/courseGrabber-selected-courses.test.mjs:290] 该测试固化了“选中行无按钮即返回 false 且不 fallback”的行为；若按上面修复调整 fallback 语义，此断言需同步更新。
Karpathy 评审
- 假设: 假设已选课程区始终暴露
input[name="right_kchid"]或.outer_xkxx_list/right_结构；不同学校页面差异未验证，属 degraded 置信度来源。 - 简洁性: 新增三个 helper 职责清晰，
findClickableElementByText的排序启发式合理，无明显过度抽象。 - 结构质量:
dropCourse的分支从内联遍历改为“选中列表优先 + fallback”结构，可读性提升，未见文件膨胀或错误分层。 - 变更范围: diff 聚焦 issue #22 的 Problem 3 与文档说明，未见无关重构；README/usage-notes 变更与代码行为一致。
- 验证: 单测覆盖选中列表命中、fallback、嵌套按钮、课程名歧义、无按钮五种分支；但均为抽取源码字符串的
Function重建测试，未覆盖dropCourse真实调用方对false的处理，也无真实 DOM 集成验证。
缺失覆盖
dropCourse返回false时上层换课流程（是否停止、是否重试目标课程）的行为未被测试或在 diff 中呈现。- fallback 路径下课程名多命中的行为无测试。
- 缺少“选中行含误导性‘退选’文本（如取消退选/已退选提示）”的匹配测试，无法证明
includes匹配不会误点。
| Boolean(element.onclick) || | ||
| Boolean(element.getAttribute("onclick")) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
findClickableElementByText 用 includes(targetText) 匹配，退选场景未传 excludedTexts，可能选中含“退选”字样的非目标可点击元素；建议对退选传入排除项或校验 onclick 语义。
| if (rowText.includes("退选")) { | ||
| dropClass = tc; | ||
| break; | ||
| let dropClass = |
Contributor
There was a problem hiding this comment.
选中行命中但无可点击按钮时，dropClass 非空导致跳过 findAllTeachingClasses fallback，相比旧逻辑可能退步；建议无按钮时继续 fallback。
| assert.equal( | ||
| state.logs.some((entry) => entry.type === "warning" && entry.message.includes("匹配到多个已选课程")), | ||
| true, | ||
| ); |
Contributor
There was a problem hiding this comment.
该测试固化了“选中行无按钮即返回 false 且不 fallback”的行为；若按上面修复调整 fallback 语义，此断言需同步更新。
ceilf6
force-pushed
the
fix/issue-22-course-loading-replace-drop-master
branch
from
f100270 to
a4d0f56
Compare
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 请求修改
决策摘要: replacement-drop 逻辑与文档改进方向正确且有回归测试覆盖，但 fallback 分支存在一处会误退课程的正确性缺陷，合并前需修复。
级联分析
- 变更符号:
dropCourse, 新增findSelectedCourseRows,findClickableElementByText,isElementClickable,DROP_BUTTON_EXCLUDED_TEXTS - 受影响流程: 换课（骑驴找马）退选流程、直接退选流程
- 变更集外调用方:
dropCourse由换课编排调用；新增 helper 均为内部私有函数，无外部消费者 - 置信度: medium（单文件浏览器脚本，无代码图谱；核心退选路径的调用方在 diff 之外未完整展示，靠推断）
问题发现
-
[高] fallback 教学班分支缺少课程名匹配过滤，可能误退无关课程
- 证据: 在
dropCourse的 fallback 路径中，findAllTeachingClasses(courseCode)之后对每个tc.row提取退选按钮，仅按“是否含退选按钮”收集dropCandidates，随后仅在dropCandidates.length > 1时才因歧义放弃。当只有 1 个候选时直接dropCandidates[0]，但没有验证该教学班的 courseName/courseCode 是否真正匹配传入的替换课程标识。 - 受影响调用方/流程: 换课流程用课程名作为替换课程标识时，
findAllTeachingClasses的返回是否已按课程名过滤未在 diff 中体现；若其返回全量或宽匹配教学班，单一候选就会被直接退选。 - 最小可行修复: 在 fallback 分支对
dropCandidates增加与 selected-course 路径一致的课程标识匹配校验（校验tc的 courseCode/courseName 是否等于/包含courseCode输入），确认findAllTeachingClasses的匹配语义后再决定是否需要额外过滤。
- 证据: 在
-
[中]
dropCourse语义混用 courseCode 与 courseName 参数- 证据: 参数命名为
courseCode，但findSelectedCourseRows与歧义判断都用isCourseCode(courseCode)支持课程名。参数名与实际语义不符，增加后续维护误读风险。 - 受影响调用方/流程: 换课编排传入替换课程标识（可能是名称）时，阅读者会误以为只接受课程号。
- 最小可行修复: 将参数重命名为
courseIdentifier或在函数入口加注释说明可为课程号或课程名，保持 selected-course 与 fallback 两条路径的判定一致。
- 证据: 参数命名为
行级发现
- [courseGrabber.js:1056] fallback 分支只在
dropCandidates.length > 1时防歧义，length === 1时未校验该教学班是否真正匹配替换课程标识；请对单一候选也做 courseCode/courseName 匹配校验，避免误退。 - [courseGrabber.js:1075]
dropClass.button || findClickableElementByText(...)二次查找可复用已收集的 button；确认 selected-course 路径下dropClass.button已非空时不会重复扫描 DOM，否则冗余。
Karpathy 评审
- 假设: 假设
findAllTeachingClasses(courseCode)已对课程名做过滤——此假设未在 diff 中验证，是发现 1 的根源。 - 简洁性: 抽出的
findClickableElementByText/findSelectedCourseRows复用合理，替换了原先内联的重复退选按钮查找逻辑，属于净简化。 - 结构质量: helper 拆分降低了
dropCourse的分支复杂度，candidate 排序策略清晰；无薄 wrapper 或错误分层。 - 变更范围: README/docs 变更与 issue 的问题 1/2（作为已知限制文档化）一致，范围合理，未夹带无关重构。
- 验证: 回归测试覆盖 selected-course 命中、fallback、嵌套按钮、歧义课名警告和误导性“取消退选”文本，覆盖较扎实；但缺少 fallback 单一候选课名不匹配的负向测试（对应发现 1）。
缺失覆盖
- fallback 分支单一教学班候选但课程名/号不匹配时应拒绝退选的测试。
- 换课编排调用
dropCourse时对课程名参数的端到端路径测试（确认findAllTeachingClasses的匹配语义）。
| const dropButton = findClickableElementByText( | ||
| tc.row, | ||
| "退选", | ||
| DROP_BUTTON_EXCLUDED_TEXTS, |
Contributor
There was a problem hiding this comment.
fallback 分支只在 dropCandidates.length > 1 时防歧义，length === 1 时未校验该教学班是否真正匹配替换课程标识；请对单一候选也做 courseCode/courseName 匹配校验，避免误退。
| resolve(false); | ||
| return; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
dropClass.button || findClickableElementByText(...) 二次查找可复用已收集的 button；确认 selected-course 路径下 dropClass.button 已非空时不会重复扫描 DOM，否则冗余。
ceilf6
force-pushed
the
fix/issue-22-course-loading-replace-drop-master
branch
from
a4d0f56 to
60d9e91
Compare
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 评论
决策摘要: 变更方向正确，用已选课程右侧列表优先查找退选按钮解决了 issue #22 problem 3，回退与歧义保护测试覆盖充分，可合并；但 dropCourse 存在一处回退分支逻辑瑕疵，建议合并前确认。
级联分析
- 变更符号:
dropCourse、新增findSelectedCourseRows、findClickableElementByText、isElementClickable、isTeachingClassMatchingIdentifier、DROP_BUTTON_EXCLUDED_TEXTS - 受影响流程: 退选流程、骑驴找马（换课）替换课程退选、依赖
dropCourse的目标课程抢占流程 - 变更集外调用方:
dropCourse的换课调用点未在 diff 中展示；新增 helper 均为新符号，无既有调用方。findAllTeachingClasses仍被复用为回退路径，行为保留 - 置信度: medium（图谱证据不可用，基于 diff 与测试推断；
dropCourse的换课上游调用点未在 diff 中呈现）
问题发现
-
[中] 回退分支中“无候选”与“找到但无按钮”日志/返回路径混淆
- 证据: courseGrabber.js 回退分支中，当
dropCandidates.length === 0时记录 warning，但不resolve(false)，继续走dropClass = dropCandidates[0] || null，随后依赖后续if (!dropClass)兜底。逻辑虽能到达resolve(false)，但先记录了“未匹配替换课程”warning，再让控制流继续，可读性差且依赖后续兜底 - 受影响调用方/流程: 换课替换课程退选路径的日志与终止行为
- 最小可行修复:
dropCandidates.length === 0时直接resolve(false); return;，与歧义分支保持一致的早返回结构，避免依赖下游兜底
- 证据: courseGrabber.js 回退分支中，当
-
[低]
findSelectedCourseRows两段扫描职责重叠- 证据: 该函数先按
.outer_xkxx_list, [id^="right_"]扫一遍，再按input[name="right_kchid"]扫一遍，靠seenRows去重。两条路径针对同一 DOM 区域，存在功能性重叠 - 受影响调用方/流程: 仅退选查找，非阻塞
- 最小可行修复: 保持现状可接受；若后续维护建议合并为单一 section 收集再统一提取行，降低理解成本。不作为合并阻塞项
- 证据: 该函数先按
行级发现
- [courseGrabber.js:1105] 回退分支
dropCandidates.length === 0记录 warning 后未早返回，控制流继续依赖后续!dropClass兜底；建议此处直接resolve(false); return;，与下方歧义分支的早返回结构一致。
Karpathy 评审
- 假设: PR 明确把 issue #22 problem 1/2 记为文档化的已知限制（课程必须已加载进 DOM），problem 3 在代码中修复。假设合理且在 docs/usage-notes.md 与 README 中显式说明，未隐藏兼容承诺
- 简洁性: 新增 helper 拆分合理，
findClickableElementByText的候选排序（exact/depth/nativeControl/textLength）解决了嵌套可点击容器的真实问题，非猜测性抽象。DROP_BUTTON_EXCLUDED_TEXTS排除“取消退选”等误匹配是必要的 - 结构质量: 从原地内联查找退选按钮重构为具名 helper，删除了重复的可点击判断逻辑，
courseGrabber.js增至约 1136 行需留意规模，但本次新增逻辑内聚于退选/换课领域，未泄漏到无关路径 - 变更范围: README 与 docs 更新与代码修复目标一致，无无关重构或格式噪声。测试仅覆盖受影响行为
- 验证: 回归测试覆盖已选列表命中、回退、嵌套按钮优先、课程名歧义（已选与教学班两路）、误导文本拒绝、非匹配拒绝，覆盖 happy path 与错误路径。
node --test声明已执行
缺失覆盖
- 缺少课程号精确匹配下多个教学班的场景测试（
isCourseCode为 true 时不触发歧义保护，但若同一课程号存在多个可退选行，取dropCandidates[0]的行为未验证） dropCourse换课上游调用点未在 diff 中呈现，无法确认调用方对新增“歧义返回 false”语义的处理是否会造成换课流程静默失败；合并前建议确认调用方在dropCourse返回 false 时不会误判为退选成功而继续抢占目标课程
| } | ||
| } | ||
|
|
||
| if (dropCandidates.length === 0) { |
Contributor
There was a problem hiding this comment.
回退分支 dropCandidates.length === 0 记录 warning 后未早返回，控制流继续依赖后续 !dropClass 兜底；建议此处直接 resolve(false); return;，与下方歧义分支的早返回结构一致。
ceilf6
force-pushed
the
fix/issue-22-course-loading-replace-drop-master
branch
from
60d9e91 to
6162437
Compare
Contributor
There was a problem hiding this comment.
代码评审报告: Fix issue #22 course visibility and replacement drop
风险等级: 中
处理建议: 评论
决策摘要: 变更聚焦于 issue #22 problem 3（换课退选），逻辑正确、测试覆盖充分、无破坏性契约变更，可以合并；但 findSelectedCourseRows 存在一个 course-name 匹配的一致性隐患值得合并前确认。
级联分析
- 变更符号:
dropCourse（重写查找逻辑）、新增findSelectedCourseRows/findClickableElementByText/isElementClickable/isTeachingClassMatchingIdentifier/DROP_BUTTON_EXCLUDED_TEXTS - 受影响流程: 换课（骑驴找马）退选路径；
dropCourse的调用方（替换课程触发退选） - 变更集外调用方:
dropCourse的既有调用者签名不变（仍接受courseCode），返回Promise<boolean>语义保留；新增 helper 均为局部新增，无既有调用方 - 置信度: medium（diff 显示
dropCourse契约保持不变，回退到旧findAllTeachingClasses路径；但未见dropCourse全部调用点与页面真实 DOM 证据，course-name 分支依赖运行时行为）
问题发现
- [中]
findSelectedCourseRows的 code 分支与 name 分支去重策略不一致，可能放过跨课程误退- 证据:
matchesSelectedCourseSection在isCode时用精确sectionCourseCode === input等多条件收敛；但 name 分支只用headingText.includes(input)，且addRowsFromSection内对每个匹配 section 都追加行。ambiguous 检查依赖selectedCourseRows.length > 1，但同一课程多个教学班行也会产生多行。 - 受影响调用方/流程: 用 course-name 配置替换课程时，单门课含多个教学班会让
selectedCourseRows.length > 1，触发“匹配到多个已选课程”而放弃退选——即误报歧义、退选失败。测试dropCourse refuses ambiguous course-name用的是两门不同课程，未覆盖“单课程多行”场景。 - 最小可行修复: 歧义判断应基于去重后的 distinct 课程数（如按 section id / kchid 聚合），而非原始行数；或在测试中补充单课程多行用例确认当前行为符合预期。
- 证据:
行级发现
- [courseGrabber.js:1052] course-name 换课路径用
selectedCourseRows.length > 1判定歧义，但同一门课的多教学班会产生多行，可能把正常单课程误判为歧义并放弃退选；建议按课程维度去重后再计数。 - [courseGrabber.js:660]
findClickableElementByText用textContent.includes(targetText)匹配，父容器文本包含子按钮文本时会同时入选 candidate，虽有 depth 排序偏向内层，但排除项DROP_BUTTON_EXCLUDED_TEXTS基于整段elementText.includes；确认“取消退选”按钮的祖先节点文本也含“取消退选”从而被正确排除（测试 misleading 用例已覆盖单节点，未覆盖嵌套祖先）。
Karpathy 评审
- 假设: 假设替换课程已在右侧“已选课程”区域且存在“退选”按钮；假设 course-name 唯一。文档已明确要求用课程号，降低了假设风险。
- 简洁性: helper 拆分合理，
findClickableElementByText的 candidate 排序逻辑略复杂但服务于“优先内层可点击按钮”的真实需求，非过度设计。 - 结构质量: 新增逻辑集中在退选路径，未泄漏到共享路径；
dropCourse保留旧findAllTeachingClasses回退，向后兼容。文件新增约 265 行但均为内聚的换课修复，未跨越可维护性阈值。 - 变更范围: README/docs 变更与 issue #22 problem 1/2 的“文档化已知限制”一致，属于既定范围内，非无关扩散。
- 验证: node 回归测试覆盖 selected-row 命中、fallback、嵌套按钮、歧义 name、误导文本、非匹配 fallback，覆盖面好。缺单课程多教学班行的歧义边界用例。
缺失覆盖
- 单门课程含多个教学班行时的 course-name 退选行为（确认不会被误判为“匹配到多个已选课程”）。
findClickableElementByText中排除文本命中嵌套祖先节点（如“取消退选”按钮位于同样含该文本的容器内）的场景。- issue #22 problem 3 描述的“必须展开详情才能操作”真实 DOM：当前测试用 FakeElement 模拟，未验证真实正方页面中退选按钮是否在未展开状态下即存在于
.outer_xkxx_list右侧列表（这是本修复能否真正生效的关键假设，建议作者附一次真实页面验证说明）。
| if (teachingClasses.length === 0) { | ||
| log(`未找到课程 ${courseCode} 的教学班`, "warning", courseCode); | ||
| const selectedCourseRows = findSelectedCourseRows(courseCode); | ||
| if (!isCourseCode(courseCode) && selectedCourseRows.length > 1) { |
Contributor
There was a problem hiding this comment.
course-name 换课路径用 selectedCourseRows.length > 1 判定歧义，但同一门课的多教学班会产生多行，可能把正常单课程误判为歧义并放弃退选；建议按课程维度去重后再计数。
| function findClickableElementByText(root, targetText, excludedTexts = []) { | ||
| if (!root) { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
findClickableElementByText 用 textContent.includes(targetText) 匹配，父容器文本包含子按钮文本时会同时入选 candidate，虽有 depth 排序偏向内层，但排除项 DROP_BUTTON_EXCLUDED_TEXTS 基于整段 elementText.includes；确认“取消退选”按钮的祖先节点文本也含“取消退选”从而被正确排除（测试 misleading 用例已覆盖单节点，未覆盖嵌套祖先）。
ceilf6
dismissed
github-actions[bot]’s stale review
Addressed in later commits; latest repo-guard review is COMMENTED and checks pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #22 scope
Verification
Fixes #22