Extract JSONL parser monolith into focused modules#57
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplit the monolithic JSONL parser into focused modules: ChangesJSONL Parser Modularization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/architecture.md (1)
74-79: ⚡ Quick winAlign contributor guidance with the actual dispatch-order contract.
Line 78’s “specific before generic” guidance conflicts with the documented broad-before-specific
task_messageexception and can trigger accidental reordering regressions. Please reword this step to emphasize preserving established table order unless fixtures/tests are updated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` around lines 74 - 79, The docs guidance for adding tool renderers is misleading about dispatch order: update the step in docs/architecture.md to instruct contributors to preserve the existing predicate order in utils/tool_dispatch.py (specifically the _parse_tool_result dispatch table) unless they also update tests/fixtures and the ordering-protecting tests (tests/test_jsonl_parser.py and tests/test_real_session_fixtures.py); explicitly call out the special-case task_message behavior so readers know it is the intentional broad-before-specific exception and that any change to ordering requires adding/updating JSONL fixtures under tests/fixtures/ and the related tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/session_peek.py`:
- Around line 24-27: The early 80-line cutoff can leave last_timestamp stale
when the file is small (≤10KB) but has >80 lines; modify the loop that
increments lines_read and breaks when lines_read > 80 so it only enforces the
cutoff for files larger than 10KB (use os.path.getsize or f.seek/tell to
determine size). Specifically, in utils/session_peek.py change the logic around
the lines_read break in the first-pass loop (and the analogous second-pass loop
referenced) so that the break executes only if file_size > 10 * 1024; this
ensures small files always get fully scanned and last_timestamp is accurate.
---
Nitpick comments:
In `@docs/architecture.md`:
- Around line 74-79: The docs guidance for adding tool renderers is misleading
about dispatch order: update the step in docs/architecture.md to instruct
contributors to preserve the existing predicate order in utils/tool_dispatch.py
(specifically the _parse_tool_result dispatch table) unless they also update
tests/fixtures and the ordering-protecting tests (tests/test_jsonl_parser.py and
tests/test_real_session_fixtures.py); explicitly call out the special-case
task_message behavior so readers know it is the intentional
broad-before-specific exception and that any change to ordering requires
adding/updating JSONL fixtures under tests/fixtures/ and the related tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd572922-9f55-45f3-b311-020101280ae7
📒 Files selected for processing (5)
docs/architecture.mdutils/jsonl_helpers.pyutils/jsonl_parser.pyutils/session_peek.pyutils/tool_dispatch.py
|
Actionable comments posted: 0 |
|
@CodeRabbit, please review this PR in more depth again |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== utils/jsonl_helpers.py ==="
cat utils/jsonl_helpers.py
echo ""
echo "=== utils/session_peek.py ==="
cat utils/session_peek.py
echo ""
echo "=== utils/tool_dispatch.py ==="
cat utils/tool_dispatch.py
echo ""
echo "=== utils/jsonl_parser.py ==="
cat utils/jsonl_parser.py
echo ""
echo "=== docs/architecture.md ==="
cat docs/architecture.md
echo ""
echo "=== tests/test_jsonl_parser.py (first 100 lines) ==="
head -100 tests/test_jsonl_parser.py 2>/dev/null || echo "Not found"
echo ""
echo "=== Imports / circular check ==="
rg "^from utils" utils/ --type py
rg "^import utils" utils/ --type pyLength of output: 42491
|
| Layer | Responsibility | Key modules |
|---|---|---|
| Parsing | JSONL → session dict | utils/jsonl_parser.py, utils/tool_dispatch.py, utils/session_peek.py, utils/jsonl_helpers.py |
Summary table
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | tool_dispatch.py |
task_message predicate shadows task_retrieval / task_completed / task_async for payloads with a "message" key |
|
| 2 | tool_dispatch.py |
plan predicate shadowed by file_write if plan payloads carry "content" |
|
| 3 | 🔵 Low | tool_dispatch.py |
grep / file_read builders exclude empty-string content fields |
| 4 | 🔵 Low | tool_dispatch.py |
file_edit builder silently drops structuredPatch without explanation |
| 5 | 🔵 Medium | session_peek.py |
Title-derivation logic duplicated vs infer_title; divergence risk |
| 6 | 🔵 Low | session_peek.py |
Pass-2 partial-line behaviour is correct but undocumented |
| 7 | 🔵 Low | jsonl_helpers.py |
extract_images doesn't handle string-typed tool_result content |
| 8 | 🔵 Low | jsonl_parser.py |
_entry_message absent from __all__ |
| 9 | 🔵 Low | architecture.md |
Component diagram doesn't reflect new module split |
The mechanical split itself is clean — no circular imports, backward-compatible re-exports, and the __all__ list is well-maintained. The dispatch ordering risk (item 1) is the one I'd track most carefully with concrete fixture tests before this merges.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils/tool_dispatch.py (1)
245-247: ⚡ Quick winLock
plan-before-file_writeprecedence with a regression fixture.This ordering is critical and easy to accidentally break later. Please add a small overlap fixture asserting payloads containing
plan + filePath + contentclassify asresult_type == "plan".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/tool_dispatch.py` around lines 245 - 247, The plan-vs-file_write precedence is fragile: add a regression test that constructs a tool payload containing a plan plus filePath and content and asserts the dispatch classification uses the plan branch (i.e., result_type == "plan"); specifically create a small test that feeds such overlapping payloads into the dispatch logic that uses _tool_result_pred_plan/_tool_result_build_plan and _tool_result_pred_file_write/_tool_result_build_file_write and assert the _tool_result_pred_plan predicate wins (and the built result equals what _tool_result_build_plan would produce) to lock the ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@utils/tool_dispatch.py`:
- Around line 245-247: The plan-vs-file_write precedence is fragile: add a
regression test that constructs a tool payload containing a plan plus filePath
and content and asserts the dispatch classification uses the plan branch (i.e.,
result_type == "plan"); specifically create a small test that feeds such
overlapping payloads into the dispatch logic that uses
_tool_result_pred_plan/_tool_result_build_plan and
_tool_result_pred_file_write/_tool_result_build_file_write and assert the
_tool_result_pred_plan predicate wins (and the built result equals what
_tool_result_build_plan would produce) to lock the ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2aa7654b-8d42-4e87-9ff4-17d77ce85f14
📒 Files selected for processing (5)
docs/architecture.mdutils/jsonl_helpers.pyutils/jsonl_parser.pyutils/session_peek.pyutils/tool_dispatch.py
✅ Files skipped from review due to trivial changes (1)
- docs/architecture.md
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/jsonl_helpers.py
|
Actionable comments posted: 0 |
|
Actionable comments posted: 0 |
|
@CodeRabbit, @timon0305 's comments are all addressed? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch all review comments from PR `#57`
gh pr view 57 --repo cppalliance/claude-code-chat-browser --json reviews,comments
echo "=== Review Comments ==="
gh api repos/cppalliance/claude-code-chat-browser/pulls/57/comments --paginate | jq '[.[] | select(.user.login == "timon0305")] | {count: length, comments: [.[] | {id, path, line, body: .body[0:300], resolved: .resolved}]}'
echo "=== Issue Comments ==="
gh api repos/cppalliance/claude-code-chat-browser/issues/57/comments --paginate | jq '[.[] | select(.user.login == "timon0305")] | {count: length, comments: [.[] | {id, body: .body[0:500]}]}'
echo "=== All Reviews ==="
gh api repos/cppalliance/claude-code-chat-browser/pulls/57/reviews --paginate | jq '[.[] | select(.user.login == "timon0305")] | {count: length, reviews: [.[] | {id, state, body: .body[0:300]}]}'Length of output: 51660 🏁 Script executed: #!/bin/bash
# Check current session_peek.py for the specific issues timon0305 flagged
echo "=== utils/session_peek.py ==="
cat utils/session_peek.py
echo ""
echo "=== Check PR description for test mention ==="
gh pr view 57 --repo cppalliance/claude-code-chat-browser --json body | jq -r '.body' | head -60
echo ""
echo "=== timon0305 review comments with resolved status ==="
gh api repos/cppalliance/claude-code-chat-browser/pulls/57/comments --paginate | jq '[.[] | select(.user.login == "timon0305")] | [.[] | {id, path, line, body: .body[0:400]}]'Length of output: 8771
One outstanding item: the PR description needs a small update. The Summary still says "No test file changes" but a new test was in fact added (and Everything else from |
3 participants
Closes #53
Summary
Mechanical split of the
utils/jsonl_parser.pymonolith (~761 lines) into focused modules with no behavior change. The 15-predicate tool-result dispatch table moves toutils/tool_dispatch.pywith an explicit ordering contract.quick_session_infomoves toutils/session_peek.py. Shared content helpers move toutils/jsonl_helpers.py. All existing imports fromutils.jsonl_parsercontinue to work via re-exports.Changes
utils/tool_dispatch.py—_TOOL_RESULT_DISPATCH, all_tool_result_pred_*/_tool_result_build_*,_parse_tool_resultutils/session_peek.py—quick_session_info()two-pass metadata peekutils/jsonl_helpers.py—_entry_message,_normalize_content,_extract_text,_extract_images,_infer_title,_strip_system_tagsutils/jsonl_parser.py(slimmed) —parse_session(), message processors,_track_file_activity, re-exportsdocs/architecture.md— dispatch table location updated toutils/tool_dispatch.pyDispatch predicate order is unchanged (first match wins;
task_messagebefore narrower task arms).Backward compatibility
Existing imports unchanged:
from utils.jsonl_parser import parse_sessionfrom utils.jsonl_parser import quick_session_infofrom utils.jsonl_parser import _parse_tool_result, _TOOL_RESULT_DISPATCHfrom utils.jsonl_parser import _strip_system_tagsTest plan
from utils.jsonl_parser import parse_session, quick_session_info, _parse_tool_result, _TOOL_RESULT_DISPATCHtest_real_session_fixtures.py) passes without modificationOut of scope
Summary by CodeRabbit