epub_bilingual_translator/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)
```python
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)
```python
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 结构:
```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 保持一致,遍历所有元素并正确增加索引。

```python
# 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。

```python
# 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]` 作为段落内容

**建议**:
```python
# 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`,也无法真正并发

**当前逻辑**:
```python
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`

大量注释表明代码作者也意识到设计不完善:
```python
# 获取原文属性（如果 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 的一致性:

```python
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 保持同步。