epub_bilingual_translator/archive/v0.09/CODE_REVIEW_REPORT.md

代码审查报告 v2 - 深度分析

1. 核心问题:Builder 与 Manifest 逻辑不一致

问题描述

TextProcessor.extract_to_manifest() 和 BilingualEPUBBuilder._create_bilingual_document() 对同一份 HTML 的处理逻辑不一致,导致 ID 映射错位。

根本原因

TextProcessor.extract_to_manifest (text_processor.py:51-89)

for element in text_elements:
    clean_text = self.clean_element_text(element)
    
    if not clean_text:
        continue  # ❌ 跳过,不添加到 manifest
    
    status = "pending"
    if self.is_navigation_element(element):
        status = "ignored"  # ✅ 添加到 manifest,但标记为 ignored
    
    item = manifest.add_item(...)  # 添加

结果:

• 空文本元素: 不添加
• 导航元素: 添加 (ID: p_00002, status: ignored)
• 普通元素: 添加 (ID: p_00001, p_00003...)

BilingualEPUBBuilder._create_bilingual_document (bilingual_builder.py:143-167)

current_para_index = 0
for element in text_elements:
    if TextProcessor.is_navigation_element(element): 
        continue  # ❌ 跳过,不增加索引
    if not TextProcessor.clean_element_text(element): 
        continue  # ❌ 跳过,不增加索引
    
    target_id = ordered_ids[current_para_index]  # 使用索引获取 ID
    current_para_index += 1

结果:

• 空文本元素: 跳过
• 导航元素: 跳过 (索引不增加!)
• 普通元素: 使用索引 0, 1, 2...

错位示例

假设 HTML 结构:

<p>段落1</p>           <!-- clean_text: "段落1" -->
<div class="nav">导航</div>  <!-- is_navigation: true -->
<p>段落2</p>           <!-- clean_text: "段落2" -->

Manifest 中的 ID 分配:

• p_00001 → 段落1 (status: pending)
• p_00002 → 导航 (status: ignored)
• p_00003 → 段落2 (status: pending)

ordered_ids: ["p_00001", "p_00002", "p_00003"]

translation_map: {"p_00001": "Translation1", "p_00003": "Translation2"}

Builder 的执行:

遍历 element[0] (段落1):
  - 不是导航 ✓
  - 有 clean_text ✓
  - current_para_index = 0
  - target_id = ordered_ids[0] = "p_00001" ✓
  - translation = "Translation1" ✓
  - 插入翻译 ✓
  - current_para_index = 1

遍历 element[1] (导航):
  - 是导航 ✗
  - continue (跳过)
  - current_para_index 仍然是 1 ❌

遍历 element[2] (段落2):
  - 不是导航 ✓
  - 有 clean_text ✓
  - current_para_index = 1
  - target_id = ordered_ids[1] = "p_00002" ❌ (应该是 p_00003!)
  - translation = translation_map.get("p_00002") = None ❌
  - 不插入翻译 ❌
  - current_para_index = 2

结果: 段落2 没有翻译!

---

2. 修复方案

方案 A: 修改 Builder 逻辑 (推荐)

原理: Builder 应该与 Manifest 保持一致,遍历所有元素并正确增加索引。

# bilingual_builder.py:143-167
current_para_index = 0
for element in text_elements:
    clean_text = TextProcessor.clean_element_text(element)
    
    # 与 extract_to_manifest 保持一致:跳过空文本
    if not clean_text:
        continue
    
    # 关键:不再跳过导航元素,而是检查 ID 对应的翻译
    if current_para_index < len(ordered_ids):
        target_id = ordered_ids[current_para_index]
        translation = translation_map.get(target_id)
        
        # 只有非导航元素且有翻译时才插入
        if translation and not TextProcessor.is_navigation_element(element):
            self._insert_translation(element, translation, soup, element.attrs)
        
        current_para_index += 1  # 无论是否插入,都要增加索引

优点:

• 逻辑简单,与 Manifest 一致
• 不需要修改 Manifest 或 TextProcessor

缺点:

• 需要同时修改 bilingual_builder.py 和 chinese_builder.py

方案 B: 修改 Manifest 逻辑

原理: 让 extract_to_manifest 也跳过导航元素,不添加到 manifest。

# text_processor.py:51-89
for element in text_elements:
    clean_text = self.clean_element_text(element)
    
    if not clean_text:
        continue
    
    # 新增:跳过导航元素
    if self.is_navigation_element(element):
        continue
    
    item = manifest.add_item(...)

优点:

• Manifest 更干净,不包含 ignored 项

缺点:

• 可能破坏现有的缓存/manifest 文件
• 如果将来需要处理导航元素,需要重新设计

---

3. 其他发现的问题

3.1 错误处理不足

位置: translator.py:176-227

问题:

• llm_client.translate_chunk() 返回错误字符串 (如 "[Error - Timeout]")
• 这些错误字符串被当作正常翻译保存到 manifest
• 最终 EPUB 中会包含 [Error - Timeout] 作为段落内容

建议:

# translator.py:178
raw_translation = results[item.global_id]

# 检测错误
if raw_translation.startswith("[Error"):
    logger.warning(f"翻译失败: {item.global_id} - {raw_translation}")
    manifest.update_item(item.global_id, None, status="failed", error=raw_translation)
    continue

processed_translation = add_spacing_between_cn_and_en_num(raw_translation)

3.2 RateLimiter 效率问题

位置: llm_client.py:19-37

问题:

• 当前实现在 acquire() 时串行化请求发起
• 即使 concurrent_requests=5,也无法真正并发

当前逻辑:

async def acquire(self):
    await self.semaphore.acquire()  # 等待并发槽位
    async with self._lock:
        # 计算等待时间
        wait_time = self.min_interval - (current_time - self.last_request_time)
        if wait_time > 0:
            await asyncio.sleep(wait_time)  # ❌ 持有锁时 sleep
        self.last_request_time = time.time()

问题: _lock 导致所有协程串行等待,无法并发。

建议: 使用 Token Bucket 或 asyncio-throttle 库。

3.3 注释中的 TODO

位置: bilingual_builder.py:150-158

大量注释表明代码作者也意识到设计不完善:

# 获取原文属性（如果 Manifest 中有的话，需要通过 paragraph_map 传进来吗？
# 此时 ordered_ids 只是 ID 列表。
# 我们需要让 _create_bilingual_document 访问到 paragraph_map
# ...
# 让我们重构一下：
# _create_bilingual_document(self, original_item, ordered_ids, translation_map, paragraph_map)

建议: 重构函数签名,传入完整的 paragraph_map 而不仅仅是 ordered_ids。

---

4. 测试建议

4.1 单元测试

创建测试用例验证 Builder 与 Manifest 的一致性:

def test_builder_manifest_consistency():
    html = """
    <p>Para1</p>
    <div class="nav">Nav</div>
    <p>Para2</p>
    """
    
    # 模拟 extract_to_manifest
    manifest_ids = []  # 应该是 [p_1, p_2, p_3]
    
    # 模拟 builder
    builder_ids = []  # 应该也是 [p_1, p_2, p_3]
    
    assert manifest_ids == builder_ids

4.2 集成测试

使用真实 EPUB 测试完整流程,验证:

• 翻译是否对应正确的段落
• 导航元素是否被正确忽略
• 错误处理是否生效

---

5. 优先级建议

1. P0 (立即修复): Builder 逻辑不一致 → 方案 A
2. P1 (重要): 错误处理 → 添加错误检测
3. P2 (优化): RateLimiter → 使用 asyncio-throttle
4. P3 (重构): 函数签名 → 传入 paragraph_map

---

总结: 核心问题是 Builder 与 Manifest 的遍历逻辑不一致。建议采用方案 A,修改 Builder 使其与 Manifest 保持同步。