Skip to content

fix: replace MustNavigate with SafeNavigate to bypass cookie consent …#563

Open
Mehechiger wants to merge 2 commits intoxpzouying:mainfrom
Mehechiger:fix/cookie-consent-popup
Open

fix: replace MustNavigate with SafeNavigate to bypass cookie consent …#563
Mehechiger wants to merge 2 commits intoxpzouying:mainfrom
Mehechiger:fix/cookie-consent-popup

Conversation

@Mehechiger
Copy link

…popups causing timeouts

@Mehechiger Mehechiger requested a review from xpzouying as a code owner March 9, 2026 20:02
@kimi-agent-bot
Copy link
Contributor

kimi-agent-bot bot commented Mar 9, 2026

🔍 Kimi Code Agent 正在自动审查此 PR...

@kimi-agent-bot
Copy link
Contributor

kimi-agent-bot bot commented Mar 9, 2026

我来对 PR #563 进行代码审查。首先获取 PR 信息和 diff。
让我进一步查看相关文件,了解 SafeNavigate 函数的实现细节。
基于 PR diff 信息,我现在进行代码审查。


PR #563 代码审查报告

标题: fix: replace MustNavigate with SafeNavigate to bypass cookie consent popups causing timeouts
作者: @Mehechiger
审查者: Claude Code


📋 PR 概述

此 PR 解决了小红书 MCP 因 GDPR Cookie 同意弹窗导致的超时问题。通过引入 SafeNavigate 函数,在页面导航后自动注入 JavaScript 代码点击 Cookie 同意按钮,避免弹窗阻塞后续操作。


✅ 审查结果:需要修改(Changes Requested)

1. 关键问题:MustNavigate 仍可能因弹窗而失败

问题描述

func SafeNavigate(page *rod.Page, url string) *rod.Page {
    page.MustNavigate(url)  // ← 这行仍可能因为弹窗而 panic
    // ... JS 注入代码
}

MustNavigate 会等待页面加载完成,如果 Cookie 弹窗干扰了页面加载检测,它可能仍然会在 JS 注入之前 panic。建议添加错误恢复或调整执行顺序。

建议
考虑使用 page.Navigate(url)(非 Must 版本)配合错误处理,或者确保 JS 在页面开始加载时就注入,而不是等 MustNavigate 完成后。


2. 中等优先级:Cookie 按钮匹配文本不够全面

当前代码

return text.includes('accept all cookies') || 
       text.includes('agree and continue') ||
       text.includes('accept cookies');

问题

  • 小红书的 Cookie 弹窗按钮文本可能是中文(如"同意"、"接受全部")或其他变体
  • 当前只覆盖英文文本,可能在某些地区/语言环境下失效

建议
添加中文按钮文本匹配:

return text.includes('accept all cookies') || 
       text.includes('agree and continue') ||
       text.includes('accept cookies') ||
       text.includes('同') ||
       text.includes('接') ||
       text.includes('允');

3. 低优先级:轮询间隔可能影响性能

当前实现

const interval = setInterval(() => { ... }, 200);
setTimeout(() => clearInterval(interval), 10000);

问题

  • 每 200ms 轮询一次,持续 10 秒(最多 50 次 DOM 查询)
  • 虽然影响有限,但频繁的 DOM 查询可能对低性能设备造成压力

建议
可以考虑使用 MutationObserver 更高效地检测弹窗出现,不过当前实现对于此场景是可接受的。


4. 代码风格:返回值未使用但保持链式调用

优点

  • 函数返回 *rod.Page 保持了与 Rod 库的链式调用风格一致
  • 修改点覆盖全面,所有 MustNavigate 都已替换

✅ 正面评价

  1. 问题解决思路正确:通过 JS 自动点击 Cookie 弹窗是解决此类问题的常见有效方案
  2. 依赖升级合理headless_browser v0.2.0 → v0.3.0 的升级符合修复需求
  3. 修改范围恰当:所有相关文件都已更新,无遗漏
  4. 超时机制:10 秒自动清理定时器避免了内存泄漏

📝 建议修改

// xiaohongshu/navigate.go
func SafeNavigate(page *rod.Page, url string) *rod.Page {
    page.MustNavigate(url)
    
    page.MustEval(`() => {
        const interval = setInterval(() => {
            let btns = Array.from(document.querySelectorAll('button, div[role="button"], span'))
            let target = btns.find(el => {
                let text = el.textContent ? el.textContent.toLowerCase() : "";
                return text.includes('accept all cookies') || 
                       text.includes('agree and continue') ||
                       text.includes('accept cookies') ||
                       text.includes('同意') ||           // 添加中文支持
                       text.includes('接受全部') ||
                       text.includes('允许');
            })
            if (target) {
                target.click();
                console.log('Cookie consent dialog auto-accepted');
                clearInterval(interval);
            }
        }, 200);
        setTimeout(() => clearInterval(interval), 10000);
    }`)

    return page
}

📊 总结

检查项 状态
功能正确性 ⚠️ 需要验证 MustNavigate 顺序问题
代码风格 ✅ 良好
错误处理 ⚠️ 建议添加中文文本匹配
测试覆盖 ❓ 请确认是否已手动测试
文档更新 N/A

建议:在合并前,请:

  1. 确认已手动测试中文环境下的 Cookie 弹窗
  2. 考虑处理 MustNavigate 可能在弹窗干扰下失败的情况

@Mehechiger 请查看上述问题,特别是 Cookie 按钮的中文文本匹配。修复后我可以再次审查。

@Mehechiger
Copy link
Author

哈喽,之前用的时候发现被cookies弹窗拦住agent整个卡在那儿了,于是加了个逻辑点掉它

@Mehechiger
Copy link
Author

已根据建议修复完成。改用 EvalOnNewDocument 并在匹配文本中补充了中文支持。麻烦再次帮忙 re-review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant