fix: skip frontend-app resolution when Playwright settings aren't configured - #627
Merged
Merged
Conversation
…figured no-mistakes graph unconditionally resolved the full frontend-app set (frontend_apps_or_default, including root inference) whenever Playwright edges were requested, even for repositories with type: nextjs projects configured for unrelated reasons and zero tests.playwright.* signal. Every per-app settings_from_loaded_v2 call in that case collapses to the same app-agnostic settings_from_defaults fallback regardless of which app is named, so the resolved app set was silently discarded unused — and worse, an unresolvable app root (ambiguous next.config.* inference) failed the whole graph build outright for repos that never touch Playwright at all. Gates the per-app resolution behind the same has_v2_playwright_settings check settings_from_loaded_v2 already uses internally, newly exposed from playwright::config. When it's false, one bare-defaults Settings is built directly, matching pre-#624 behavior with zero app-resolution work. Found while investigating a CodSpeed memory-regression flag on #626; two of the three flagged benchmarks (repeated_imports_from_one_file, facts/imports_and_symbols) don't reach any code from that PR at all, so the report is likely dominated by its own acknowledged baseline mismatch rather than this specific gap — but the gap was real regardless and is worth closing on its own principle-following merits (declare the complete fact/relationship demand at the boundary before collecting it).
This comment has been minimized.
This comment has been minimized.
Reviewer's GuideThis PR gates Playwright-related frontend app resolution on actual Playwright configuration, and adds a regression test fixture to ensure graphs for Next.js projects without Playwright no longer fail or perform unnecessary work. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughPlaywright graph preparation now skips frontend-app resolution when checks are disabled. When checks are enabled without v2 settings, it creates one default app-agnostic settings entry. A regression test covers an unconfigured Next.js project. ChangesPlaywright graph preparation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/no-mistakes/src/codebase/dependencies/graph/tests/config_path_full_graph.rs (1)
325-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the test invariant comment.
Keep the fixture rationale, but reduce this 13-line doc comment to the invariant under test. The implementation details and review-history wording duplicate the test body and make the fixture harder to scan.
Use two short comments such as:
// This fixture has ambiguous Next.js roots but no Playwright configuration. // Graph preparation must skip app resolution and still build a default fact plan.As per coding guidelines, “Add short comments to intentionally counterintuitive fixtures or tests to preserve the invariant they protect.”
🤖 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 `@crates/no-mistakes/src/codebase/dependencies/graph/tests/config_path_full_graph.rs` around lines 325 - 337, Shorten the doc comment above the ambiguous Next.js fixture to two concise lines stating that it has no Playwright configuration and that graph preparation must skip app resolution while building the default fact plan. Remove implementation details, function references, and review-history discussion while preserving the fixture rationale and invariant.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.
Nitpick comments:
In
`@crates/no-mistakes/src/codebase/dependencies/graph/tests/config_path_full_graph.rs`:
- Around line 325-337: Shorten the doc comment above the ambiguous Next.js
fixture to two concise lines stating that it has no Playwright configuration and
that graph preparation must skip app resolution while building the default fact
plan. Remove implementation details, function references, and review-history
discussion while preserving the fixture rationale and invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e780371-40fa-428b-9c19-ebda4c2c2422
📒 Files selected for processing (8)
crates/no-mistakes/src/codebase/dependencies/graph/files_config_prepared.rscrates/no-mistakes/src/codebase/dependencies/graph/tests/config_path_full_graph.rscrates/no-mistakes/src/playwright/config.rscrates/no-mistakes/src/playwright/config/load.rscrates/no-mistakes/src/playwright/config/load/helpers.rstest-cases/codebase-analysis/graph-nextjs-project-without-playwright/fixture/.no-mistakes.ymltest-cases/codebase-analysis/graph-nextjs-project-without-playwright/fixture/a/next.config.tstest-cases/codebase-analysis/graph-nextjs-project-without-playwright/fixture/b/next.config.ts
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (93.33%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #627 +/- ##
==========================================
- Coverage 99.24% 99.24% -0.01%
==========================================
Files 1277 1277
Lines 100527 100539 +12
==========================================
+ Hits 99772 99780 +8
- Misses 755 759 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #626. While investigating a CodSpeed memory-regression flag on that PR (
graph/all_domains_build,-23.27%memory efficiency), I traced the actual code path and found a real, if narrow, inefficiency introduced by that PR — not the confirmed cause of the CodSpeed number (see below), but worth fixing on its own merits.no-mistakes graph'sprepare_graph_configunconditionally calledfrontend_apps_or_default(full frontend-app resolution, includingnext.config.*root inference) whenever Playwright route/selector edges were requested — even for repositories that configuretype: nextjsprojects for reasons unrelated to Playwright and have zerotests.playwright.*configuration. In that shape, every per-appsettings_from_loaded_v2call collapses to the same app-agnosticsettings_from_defaultsfallback regardless of which app is named, so the resolved app set was silently discarded unused.Worse than wasted work: if that unused app-resolution attempt hit an ambiguous root (two or more
next.config.*files, no explicitroot:), it failed the entire graph build for a repository that never touches Playwright at all.Fix
Gates the per-app resolution behind
has_v2_playwright_settings— the same checksettings_from_loaded_v2already uses internally to decide between the two branches — newly exposed ascrate::playwright::config::has_v2_playwright_settings. When it'sfalse, one bare-defaultsSettingsis built directly with zero app-resolution work, exactly matching pre-#624 behavior.On the CodSpeed regression itself
Traced all three flagged benchmarks:
repeated_imports_from_one_file(TS-import resolver caching) andfacts/imports_and_symbols(rawcollect_ts_factsextraction) call into code fix: resolve frontend apps independently for multi-Next.js-app repositories #626 never touches at all — confirmed by reading both benchmarks' source and thebenchmark_supportmodule they depend on.graph/all_domains_builddoes touch fix: resolve frontend apps independently for multi-Next.js-app repositories #626's code, but its fixture has an explicitroot:(no inference scan) and — this PR's finding — was hitting the "resolved then discarded" path described above, not a scan-heavy one. The magnitude of the flagged 2.5MB delta seems too large to be explained by that alone, especially with two demonstrably-unrelated benchmarks flagged at similar magnitude.Net: the report is most likely dominated by that baseline mismatch, not by #626's code. This PR closes the one real gap found while checking.
Test plan
type: nextjsproject, noroot:, two ambiguousnext.config.*files, zerotests.playwright.*) provesprepare_graph_confignow succeeds where it previously failed outright, since app resolution is skipped rather than attempted-and-failed.cargo test -p no-mistakes --lib— 5189 passed, 0 failed.cargo clippy -p no-mistakes --all-targets --all-features -- -D warnings— clean.cargo llvm-cov— 99.11% lines / 99.04% functions, above the 99%/99% gate.cargo fmt --check— clean.https://claude.ai/code/session_01LqTgxthoZxVxKHJDDYnctU