test(advisor): add simple E2E suite review lens#5170
Conversation
jyaunches
added
area: e2e
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new "Vitest E2E suite simplicity" rubric item to the PR review advisor system prompt and updates the test to assert the new guidance text about architecture review and favoring focused Vitest tests with local helpers. ChangesVitest E2E Simplicity Prompt & Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-raw-output.txt |
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-scenario-advisor-raw-output.txt |
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
![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)
tools/pr-review-advisor/analyze.mts (1)
457-529: ⚡ Quick winReduce detector branch complexity with data-driven rules.
This function packs multiple rule families into one long conditional chain, which makes future edits error-prone. Extracting path-match rules into a table of
{pattern, signalFactory}handlers would keep complexity lower and make behavior easier to extend/test.As per coding guidelines, "Keep function complexity low in JavaScript and TypeScript code".
🤖 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 `@tools/pr-review-advisor/analyze.mts` around lines 457 - 529, The function detectE2eVitestSimplicitySignals is too branch-heavy; refactor by replacing the long conditional chain with a data-driven rule table. Create an array of rule objects (e.g., {test: RegExp | (f)=>boolean, factory: (file)=>E2eVitestSimplicitySignal}) and iterate it for each changed file instead of the many ifs; reuse the existing push(signals) logic and keep the duplicate check in push, preserving existing kinds like "repo-local E2E migration ledger", "shared Vitest E2E fixture/support change", "Vitest E2E registry or manifest expansion", "Vitest E2E workflow expansion", and "one-off E2E workflow validator expansion" so behavior is unchanged while complexity is reduced in detectE2eVitestSimplicitySignals.Source: Coding guidelines
🤖 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 `@tools/pr-review-advisor/analyze.mts`:
- Around line 533-569: The hunk line numbering (nextLine) only advances when an
added line (+) is processed because the loop continues before the shared
increment; as a result context lines (" ") are not counted and reported line
numbers are shifted. Modify the loop to inspect the first character once (e.g.,
const lineType = rawLine[0]) and skip diff meta headers ("+++","---") but do not
early-return before adjusting nextLine: process added lines when lineType ===
'+' (and not "+++") to push signals using nextLine, and then increment nextLine
when lineType === '+' or lineType === ' ' (context) but do not increment for
removed lines ('-'); keep file/hunk detection using fileMatch/hunkMatch and
preserve the existing push(...) calls (references: nextLine, rawLine, fileMatch,
hunkMatch, push, signals).
---
Nitpick comments:
In `@tools/pr-review-advisor/analyze.mts`:
- Around line 457-529: The function detectE2eVitestSimplicitySignals is too
branch-heavy; refactor by replacing the long conditional chain with a
data-driven rule table. Create an array of rule objects (e.g., {test: RegExp |
(f)=>boolean, factory: (file)=>E2eVitestSimplicitySignal}) and iterate it for
each changed file instead of the many ifs; reuse the existing push(signals)
logic and keep the duplicate check in push, preserving existing kinds like
"repo-local E2E migration ledger", "shared Vitest E2E fixture/support change",
"Vitest E2E registry or manifest expansion", "Vitest E2E workflow expansion",
and "one-off E2E workflow validator expansion" so behavior is unchanged while
complexity is reduced in detectE2eVitestSimplicitySignals.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e23a9309-5710-4040-9876-090a3da0da00
📒 Files selected for processing (2)
test/pr-review-advisor.test.tstools/pr-review-advisor/analyze.mts
jyaunches
changed the title
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed (code + 9-cat security). Adds a "Vitest E2E suite simplicity" rubric lens (step 7) to the PR Review Advisor prompt, renumbering 7→8/8→9, plus 3 prompt-assertion tests. The change itself is clean and the new assertions match the head prompt strings.
🔴 Request changes — stale base / merge collision. Since this PR opened, #5126 ("simplify legacy migration tracking") landed on main and edited the same file and same rubric region (tools/pr-review-advisor/analyze.mts) — it added its own step 8 ("Legacy E2E migration governance") plus a deterministic ledger blocker, and also edited test/pr-review-advisor.test.ts. A naive merge collides on the step numbering and the test file (mergeable reports UNKNOWN).
Please: merge main into the branch, reconcile so both lenses coexist with unique step numbers, re-run the assertions, and sanity-check the two overlapping "simplicity/governance" rules don't produce duplicate findings on the same E2E PR.
Security: all 9 pass (prompt-only string edit). Nit: the test-file import reordering is unrelated noise that adds to the merge surface.
4 participants
Summary
Adds a prompt-only simple-first review lens to the PR Review Advisor for PRs touching the Vitest E2E suite.
Related Issue
Refs #5098
Changes
test/e2e-scenario/,.github/workflows/e2e-vitest-scenarios.yaml, ortools/e2e-scenarios/get closer architecture review for new systems.Verification
npm run typecheck:cli -- --pretty falsenpx vitest run --project cli test/pr-review-advisor.test.ts --reporter=defaultgit diff --checkSummary by CodeRabbit
New Features
Tests