configure lighthouse for local development/testing #2122
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughSwitches Lighthouse CI to run from the frontend via pnpm (CI workflow, Makefile, and package.json scripts), replaces JSON LHCI config with a JS config that reads environment variables and multiple routes, adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
frontend/lighthouserc.js (3)
5-10: Good assertion coverage; consider adding preset for sensible defaultsYour explicit category thresholds are fine. For slightly stricter and more maintainable defaults, consider layering LHCI’s recommended preset and then overriding category thresholds as needed.
Apply this diff to include the preset while keeping your custom thresholds:
module.exports = { ci: { assert: { + preset: 'lighthouse:recommended', assertions: { 'categories:accessibility': ['warn', { minScore: 0.9 }], 'categories:best-practices': ['warn', { minScore: 0.9 }], 'categories:performance': ['warn', { minScore: 0.9 }], 'categories:seo': ['warn', { minScore: 0.9 }], }, },
12-17: Make local/CI configuration switchable via env vars (URL and Chrome flags)Right now the config is hard-wired to localhost and fixed Chrome flags. Making these values environment-driven lets the same config work for local, staging, and production without editing files.
Apply this diff to parameterize URL and flags and adopt modern headless:
+const LHCI_URL = process.env.LHCI_URL +const CHROME_FLAGS = + process.env.LHCI_CHROME_FLAGS || + '--disable-dev-shm-usage --headless=new --no-sandbox' + module.exports = { ci: { @@ collect: { numberOfRuns: 3, settings: { - chromeFlags: '--disable-dev-shm-usage --headless --no-sandbox', + chromeFlags: CHROME_FLAGS, }, - url: ['http://localhost:3000/'], + url: LHCI_URL ? [LHCI_URL] : ['http://localhost:3000/'], },
18-20: Avoid uploading local runs to public storage by defaulttemporary-public-storage is convenient but uploads reports to a public endpoint. For local development, writing to the filesystem is safer and avoids network dependency. You can still override in CI via env vars.
Proposed change:
upload: { - target: 'temporary-public-storage', + target: process.env.LHCI_UPLOAD_TARGET || 'filesystem', + outputDir: process.env.LHCI_OUTPUT_DIR || '.lighthouseci', + reportFilenamePattern: + '%%DATETIME%%-%%URL_HOST%%%%URL_PATHNAME%%-%%VARIANT%%.%%EXTENSION%%', },If you prefer to keep public uploads in CI, set
LHCI_UPLOAD_TARGET=temporary-public-storagethere. Do you want me to wire this into the workflow step?.github/workflows/run-ci-cd.yaml (2)
500-504: Use the package script instead of npx for version consistency and faster CICI currently installs LHCI via npx even though
@lhci/cliis a devDependency and scripts exist. Running the script:
- avoids redundant network installs,
- guarantees the same LHCI version locally and in CI,
- respects lighthouserc.js and any env-driven overrides.
Also, consider reducing runs in CI to 1 to save minutes; local can remain at 3.
Apply this diff:
- - name: Run lighthouse-ci + - name: Run lighthouse-ci working-directory: frontend run: | - npx -y @lhci/[email protected] autorun --collect.url=https://nest.owasp.dev/ + pnpm run lighthouse-ci -- --collect.url=https://nest.owasp.dev/ --collect.numberOfRuns=1
500-504: Consider persisting Lighthouse reports as workflow artifactsIf you switch
upload.targettofilesystem(as suggested) for CI runs, you can retain HTML reports for triage. This improves debuggability over relying on temporary URLs.Example addition after the run step:
- name: Upload Lighthouse artifacts if: always() uses: actions/upload-artifact@v4 with: name: lighthouse-reports path: frontend/.lighthouseci/**If you keep
temporary-public-storage, you can skip this.frontend/Makefile (2)
30-34: Targets look good; mark them as .PHONY to avoid name collisionsDeclaring these as phony prevents accidental no-op runs if a file named
lighthouse-ciorlighthouse-ci-desktopappears.Add a .PHONY declaration:
+lighthouse-ci: + @cd frontend && pnpm run lighthouse-ci + +lighthouse-ci-desktop: + @cd frontend && pnpm run lighthouse-ci:desktopAnd add this (outside the target block):
.PHONY: lighthouse-ci lighthouse-ci-desktop
30-34: Optional: Pass URL via env for parity with CIIf you adopt env-driven config, you can run against staging without editing files:
Example usage (no file change needed):
- Local:
make lighthouse-ci(defaults to http://localhost:3000/)- Staging:
LHCI_URL=https://nest.owasp.dev/ make lighthouse-cifrontend/package.json (2)
11-12: Scripts LGTM; align CI to use themThe
lighthouse-ciandlighthouse-ci:desktopscripts are correctly defined. Recommend switching the workflow to call these scripts (see workflow comment) for consistent versioning and flags.
71-71: Version choice is reasonable; keep CI and local on the same LHCI line
@lhci/cli^0.15.1 is fine. Ensure CI uses the same installed version by invokingpnpm run lighthouse-ciinstead ofnpx @lhci/[email protected]to avoid subtle diffs if a patch update lands locally.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/run-ci-cd.yaml(1 hunks)frontend/Makefile(1 hunks)frontend/lighthouserc.js(1 hunks)frontend/lighthouserc.json(0 hunks)frontend/package.json(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/lighthouserc.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
|
I like some of CR's nitpicks, and I will implement them:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
cspell/custom-dict.txt (1)
74-74: LGTM: dictionary entry added in the right place."lighthouseci" looks correct and remains alphabetically positioned between "libexpat" and "linkify".
Optionally add "lighthouserc" as well to avoid future false positives when referring to the new config file name.
libexpat lighthouseci +lighthouserc linkify.github/workflows/run-ci-cd.yaml (2)
488-489: Confirm removal of gating; consider restoringneedsto ensure staging is up-to-date before auditing.With
needscommented out, LHCI will run regardless of whether staging was successfully deployed. If the intent is to audit the freshly deployed staging site onmain, re-enable the dependency. If you want it to run for PRs too, you can gate only onmain.Proposed diff (re-gate when on main):
run-lighthouse-ci: name: Run Lighthouse CI - # needs: - # - deploy-staging-nest-proxy + needs: + - deploy-staging-nest-proxy + if: | + github.ref == 'refs/heads/main' || always()If you prefer LHCI on PRs as well, drop the
ifand keepneedsonly formainwith:+ if: github.ref == 'refs/heads/main'
508-514: Consider explicit LHCI target/env; current defaults are fine, but make intent clear.The config defaults to
filesystemuploads. If that’s the intended behavior in CI, set it explicitly so future changes to defaults don’t break artifact collection. Also consider parameterizing the URL per branch if needed.Proposed diff:
- name: Run lighthouse-ci working-directory: frontend env: LHCI_URL: 'https://nest.owasp.dev/' + LHCI_UPLOAD_TARGET: 'filesystem' run: | pnpm run lighthouse-cifrontend/lighthouserc.js (4)
1-6: Good defaults; consider supporting multiple URLs viaLHCI_URLS.Today you accept a single
LHCI_URL. For local/dev and CI it’s common to scan a small set of critical paths. You can safely support both a single URL and a comma-separated list.Apply:
const LHCI_CHROME_FLAGS = process.env.LHCI_CHROME_FLAGS || '--disable-dev-shm-usage --headless --no-sandbox' const LHCI_OUTPUT_DIR = process.env.LHCI_OUTPUT_DIR || '.lighthouseci/' const LHCI_UPLOAD_TARGET = process.env.LHCI_UPLOAD_TARGET || 'filesystem' const LHCI_URL = process.env.LHCI_URL || 'http://localhost:3000/' +const LHCI_URLS = (process.env.LHCI_URLS || '') + .split(',') + .map((s) => s.trim()) + .filter(Boolean)And later switch
collect.urlto preferLHCI_URLSwhen provided (see below).
7-16: Assertions are “warn” severity — confirm this is intentional for CI.Using
warnavoids failing the job, which is great for local/dev. If you want stricter enforcement onmainwhile keeping PRs lenient, consider driving severity from an env var or separate workflow matrix.Example (pattern only):
assert: { assertions: { - 'categories:accessibility': ['warn', { minScore: 0.9 }], + 'categories:accessibility': [process.env.LHCI_SEVERITY || 'warn', { minScore: 0.9 }], // repeat for others... }, },
18-23: Expose form factor/preset to easily run mobile and desktop profiles.LHCI defaults to mobile-like emulation. Adding a switch enables “desktop” runs locally and in CI without code changes.
Apply:
collect: { numberOfRuns: 3, settings: { chromeFlags: LHCI_CHROME_FLAGS, + // optional: 'desktop' or 'mobile' (LH default is mobile) + ...(process.env.LHCI_PRESET ? { preset: process.env.LHCI_PRESET } : {}), }, - url: [LHCI_URL], + url: LHCI_URLS.length ? LHCI_URLS : [LHCI_URL], },
24-28: File naming: consider including form factor to disambiguate reports.If you later produce both mobile and desktop reports, filenames will collide. Including
%%FORM_FACTOR%%keeps artifacts clear.Apply:
upload: { outputDir: LHCI_OUTPUT_DIR, - reportFilenamePattern: '%%DATETIME%%-%%HOSTNAME%%%%PATHNAME%%.%%EXTENSION%%', + reportFilenamePattern: '%%DATETIME%%-%%FORM_FACTOR%%-%%HOSTNAME%%%%PATHNAME%%.%%EXTENSION%%', target: LHCI_UPLOAD_TARGET, },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/run-ci-cd.yaml(1 hunks)cspell/custom-dict.txt(1 hunks)frontend/lighthouserc.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/run-ci-cd.yaml (2)
495-507: Install Node before pnpm; don’t auto-install at repo root; install dependencies in frontend/Running
pnpm/action-setupwithrun_install: trueat the repository root can perform an unnecessary or failing install (root likely isn’t a pnpm workspace). Also, setting up Node after pnpm risks version mismatch during the install. Move Node setup first, disable auto-install, and explicitly install infrontend/so@lhci/cliis present for the next step.Apply:
- - name: Install pnpm - uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda - with: - version: 10 - run_install: true - - - name: Set up Node + - name: Set up Node uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 with: node-version: 22 cache: 'pnpm' cache-dependency-path: frontend/pnpm-lock.yaml + + - name: Install pnpm + uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda + with: + version: 10 + run_install: false + + - name: Install frontend dependencies + working-directory: frontend + run: pnpm install --frozen-lockfile
518-524: Fix step name and make artifact upload resilient and complete
- Typo: “Ligthouse” → “Lighthouse”.
- Upload should not fail when no files are present.
- Capture the entire directory (HTML and JSON), not just
*.html.- - name: Upload Ligthouse artifacts + - name: Upload Lighthouse artifacts if: always() uses: actions/upload-artifact@v4 with: name: lighthouse-reports - path: frontend/.lighthouseci/*.html + path: frontend/.lighthouseci/** + if-no-files-found: ignore
🧹 Nitpick comments (3)
.github/workflows/run-ci-cd.yaml (3)
508-514: Ensure artifacts are produced and make target configurableTwo tweaks improve determinism:
- Force local report generation to guarantee artifacts exist regardless of upload target in config.
- Keep target URL configurable via env/secrets as you already intend.
- name: Run lighthouse-ci working-directory: frontend env: - LHCI_URL: 'https://nest.owasp.dev/' + LHCI_URL: 'https://nest.owasp.dev/' + # Ensure local HTML/JSON reports are written for artifact upload + LHCI_UPLOAD_TARGET: filesystem + # Headless flags are commonly needed in CI; override if your lighthouserc.js sets these already. + LHCI_CHROME_FLAGS: "--no-sandbox --headless=new" run: | pnpm run lighthouse-ciFollow-up: If you plan to run both mobile and desktop profiles, consider a short matrix or a second step (
pnpm run lighthouse-ci:desktop) so we publish both sets of reports. Do you want a patch for that?
515-517: Don’t let a debug listing fail the jobIf
.lighthouseci/is absent (upload to server, early exit, etc.),lswill fail and abort the job. Guard it.- - name: Check for report files (temporary debug) - run: ls -la frontend/.lighthouseci + - name: Check for report files (temporary debug) + if: always() + continue-on-error: true + run: ls -la frontend/.lighthouseci || true
488-490: Clarify gating for where we measure Lighthouse (staging vs. static URL)You commented out the dependency on
deploy-staging-nest-proxy. If the URL points at a live environment not tied to the current commit, metrics won’t reflect this PR. Two options:
- Option A (recommended for main): gate behind staging deploy so we test the just-built artifacts.
- Option B (for PRs): run independently but target a preview or a deterministic URL meant for PR checks.
Option A (uncomment and gate on main only):
run-lighthouse-ci: name: Run Lighthouse CI - # needs: - # - deploy-staging-nest-proxy + needs: + - deploy-staging-nest-proxy + if: github.ref == 'refs/heads/main'If you prefer Option B for PRs, keep
needscommented but confirm thatLHCI_URLin this job targets a stable preview/staging endpoint appropriate for pull_request events. Do you want me to propose a dual-path (PR vs main) job layout?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/run-ci-cd.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run Lighthouse CI
- GitHub Check: CodeQL (javascript-typescript)
rudransh-shrivastava
force-pushed
the
feature/ci-lighthouse-local
branch
from
4dbf332 to
e089eb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/run-ci-cd.yaml (1)
495-507: Fix PNPM setup order and install scope; current flow likely skips installing frontend deps.
pnpm/action-setupwithrun_install: trueruns at repo root before Node is set up. If the repo root isn’t a pnpm workspace, dependencies forfrontend/won’t be installed, andpnpm run lighthouse-ciwill fail due to missingnode_modules.- Set up Node first, disable auto-install on the pnpm action, and explicitly install within
frontend/.Apply:
- - name: Install pnpm - uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda - with: - version: 10 - run_install: true - - - name: Set up Node + - name: Set up Node uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 with: node-version: 22 cache: 'pnpm' cache-dependency-path: frontend/pnpm-lock.yaml + + - name: Install pnpm + uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda + with: + version: 10 + run_install: false + + - name: Install frontend dependencies + working-directory: frontend + run: pnpm install --frozen-lockfile
🧹 Nitpick comments (2)
.github/workflows/run-ci-cd.yaml (2)
515-517: Don’t let the temporary debug step fail the job.
ls -la frontend/.lighthouseciwill fail if the folder isn’t created (e.g., aborts/collect-only mode), causing an avoidable red CI.Apply one of:
- - name: Check for report files (temporary debug) - run: ls -la frontend/.lighthouseci + - name: Check for report files (temporary debug) + if: always() + continue-on-error: true + run: ls -la frontend/.lighthousecior simply remove the step before merging.
518-527: Tighten artifact upload: upload dirs, avoid duplicates, and persist on failure.
- Current paths duplicate
.lighthouseci/*.htmland omit root-level.json.- Uploading directories is simpler and future-proof.
- Add
if: always()so reports are preserved even when LHCI fails.- - name: Upload Lighthouse artifacts - uses: actions/upload-artifact@v4 + - name: Upload Lighthouse artifacts + if: always() + uses: actions/upload-artifact@v4 with: - name: lighthouse-report - path: | - frontend/.lighthouseci/*.html - frontend/.lighthouseci/*.json - .lighthouseci/*.html - .lighthouseci/*.html + name: lighthouse-report + path: | + frontend/.lighthouseci + .lighthouseci if-no-files-found: ignore
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/run-ci-cd.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run Lighthouse CI
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
.github/workflows/run-ci-cd.yaml (2)
488-490: Intent check: running Lighthouse without gating on staging deploy may hit stale/unavailable targets.You commented out the
needs: deploy-staging-nest-proxy. If the intent is to run against a stable public URL irrespective of deploy freshness, fine. Otherwise, consider re-enabling gating or adding an availability check (e.g., curl with retries) before LHCI collect.Happy to propose a minimal curl-with-retry precheck if you want it in this job.
508-514: LHCI_URL and @lhci/cli dependency are correctly configured
- frontend/lighthouserc.js reads
process.env.LHCI_URLwith a fallback ('http://localhost:3000/').- frontend/package.json declares the
@lhci/clidependency (^0.15.1).No further changes needed.
|
Looks like what I've been trying to do was pointless. I will revert back to temporary public storage. Please let me know if that's not good and I will use artifacts once again. |
rudransh-shrivastava
force-pushed
the
feature/ci-lighthouse-local
branch
from
c62a72d to
f747ec0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/run-ci-cd.yaml (1)
495-500: Set up Node before PNPM and disable repo-root auto-install; install in frontend.Installing PNPM with
run_install: trueat repo root attempts an install where nopackage.jsonexists and happens before Node is set to v22. Install underfrontend/after Node setup to avoid failures and wasted work.Apply:
- - name: Install pnpm - uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda - with: - version: 10 - run_install: true - - - name: Set up Node + - name: Set up Node uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 with: node-version: 22 cache: 'pnpm' cache-dependency-path: frontend/pnpm-lock.yaml + + - name: Install pnpm + uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda + with: + version: 10 + run_install: false + + - name: Install frontend dependencies + working-directory: frontend + run: pnpm install --frozen-lockfileAlso applies to: 501-507
🧹 Nitpick comments (2)
.github/workflows/run-ci-cd.yaml (2)
488-490: Removing the needs gate changes execution order; confirm intent or split jobs by target environment.Decoupling Lighthouse from
deploy-staging-nest-proxymeans LH runs against whatever is currently atnest.owasp.dev, not necessarily what this workflow built/deployed. If that’s intentional for PRs, consider either:
- Split into two jobs: one for PRs (targets public URL, no needs) and one for main (targets staging and
needs: deploy-staging-nest-proxy), or- Keep a single job but add an
if:to only enforceneedsonrefs/heads/mainand otherwise skip.This guards against measuring the wrong version and makes results easier to reason about.
508-514: LHCI run step works; consider adding artifact upload and a timeout, and keep URL formatting consistent.
- Optional: upload local
.lighthousecireports for easier triage even when using temporary-public-storage.- Optional: add
timeout-minutes:to avoid a stuck run on flaky Chrome launches.- Nit: drop the trailing slash in
LHCI_URLor ensure the config doesn’t concatenate paths that could yield//.You can append an artifact step after this run (resilient if nothing was generated):
- name: Upload Lighthouse artifacts if: always() uses: actions/upload-artifact@v4 with: name: lighthouse-reports path: frontend/.lighthouseci/ if-no-files-found: ignoreAnd, if desired, add a guard against long hangs:
- name: Run lighthouse-ci timeout-minutes: 15 working-directory: frontend env: LHCI_URL: 'https://nest.owasp.dev' run: pnpm run lighthouse-ci
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/run-ci-cd.yaml(1 hunks)cspell/custom-dict.txt(1 hunks)frontend/lighthouserc.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cspell/custom-dict.txt
- frontend/lighthouserc.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run Lighthouse CI
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
.github/workflows/run-ci-cd.yaml (1)
505-506: Good: PNPM cache correctly points at frontend lockfile.Using
cache: 'pnpm'withcache-dependency-path: frontend/pnpm-lock.yamlis the right setup given the monorepo layout.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@arkid15r Should be ready for review, please let me know where I can improve the PR, I will do so. Thank you. |
|
I have a suggestion, we can use the sitemap for adding more urls. This has a downside, there will be a lot of urls to test. We can also choose certain urls to test manually. |
Yes, that'd be great -- we need a way to configure URLs for testing. Ideally it'd be the same list for all environments (single config file) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/lighthouserc.js (4)
5-12: Make URLs configurable via env to support different environments and larger test sets.Hardcoding
ROUTESlimits flexibility. SupportLHCI_ROUTES(comma-separated) so teams can override without code changes, aligning with the request for a single configurable list usable across environments.Apply this diff:
-const ROUTES = ['/', '/about', '/chapters', '/projects'] +const DEFAULT_ROUTES = ['/', '/about', '/chapters', '/projects'] +const ROUTES = (process.env.LHCI_ROUTES + ? process.env.LHCI_ROUTES.split(',').map(s => s.trim()).filter(Boolean) + : DEFAULT_ROUTES)Optional next step: add
LHCI_SITEMAP_URLsupport to auto-generate routes from a sitemap for broader coverage when desired. I can sketch that if helpful.
7-12: Nit: simplify getUrls and make joining more robust.Minor cleanup and slightly safer URL joining.
Apply this diff:
-const getUrls = () => { - const baseUrl = LHCI_BASE_URL.endsWith('/') ? LHCI_BASE_URL.slice(0, -1) : LHCI_BASE_URL - const routes = ROUTES - - return routes.map((route) => `${baseUrl}${route}`) -} +const getUrls = () => { + const baseUrl = LHCI_BASE_URL.endsWith('/') ? LHCI_BASE_URL.slice(0, -1) : LHCI_BASE_URL + return ROUTES.map((route) => `${baseUrl}${route}`) +}Optional: use
new URL(route, baseUrl).toString()if you ever introduce non-leading-slash routes.
24-30: Add env-driven desktop/mobile switch without CLI flags.You mentioned a desktop variant in scripts; enabling an env toggle keeps parity with the “use env vars over flags” goal.
Apply this diff (builds on the chromeFlags fix):
collect: { numberOfRuns: 1, chromeFlags: LHCI_CHROME_FLAGS, + // Set LHCI_PRESET=desktop to use Lighthouse's desktop preset; otherwise default (mobile) is used. + ...(process.env.LHCI_PRESET === 'desktop' ? { settings: { preset: 'desktop' } } : {}), url: getUrls(), },Example:
LHCI_PRESET=desktop pnpm run lighthouse-cior via Makefile export.
16-23: Consider extending assertions with the recommended preset, then override.Using
preset: 'lighthouse:recommended'gives a sensible baseline and you can keep your category thresholds as overrides. Keepwarnto avoid failing dev runs.Apply this diff:
ci: { assert: { + preset: 'lighthouse:recommended', assertions: { 'categories:accessibility': ['warn', { minScore: 0.9 }], 'categories:best-practices': ['warn', { minScore: 0.9 }], 'categories:performance': ['warn', { minScore: 0.9 }], 'categories:seo': ['warn', { minScore: 0.9 }], }, },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/run-ci-cd.yaml(1 hunks)frontend/lighthouserc.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/run-ci-cd.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (2)
frontend/lighthouserc.js (2)
31-33: Intent check: temporary-public-storage is public and short-lived.This is fine for quick sharing, but links expire and data is publicly accessible. If you later want durable, private artifacts, switch to a self-hosted LHCI server or artifact uploads.
Do you want me to propose a companion config stanza for a self-hosted LHCI server (behind auth) and a Makefile target to toggle between upload targets?
24-30: Dismiss incorrect chromeFlags placement suggestionThe existing configuration correctly places
chromeFlagsundercollect.settings, which aligns with the official Lighthouse CI documentation indicating thatchromeFlagsbelong inside thesettingsobject passed to Lighthouse during collection. No changes are needed to yourfrontend/lighthouserc.js.Likely an incorrect or invalid review comment.
|
I have added a variable |
|
@rudransh-shrivastava hey! Could you resolve current conflicts before we review? thanks! |
|
@kasya Yes, I've resolved them now. |
|
@arkid15r I've only added |
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Great improvement, LGTM 👍
| "lighthouse-ci": "lhci autorun", | ||
| "lighthouse-ci:desktop": "lhci autorun --collect.settings.preset=desktop", |
There was a problem hiding this comment.
What's the difference between those 2 targets? Do we need the desktop preset for local running?
There was a problem hiding this comment.
By default, lighthouse opens all pages as a mobile device, this is if we want to test the desktop version
There was a problem hiding this comment.
It's not required, testing as a mobile device is enough but I think accessibility and some UI may differ for both devices, it's a good to have
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/lighthouserc.js (1)
5-14: Make routes configurable via env (ROUTES/LHCI_URL_PATHS) with sane defaultsThis aligns with earlier discussion about a configurable URL list and keeps one config for local/CI. Accept both a JSON array and a comma-separated list for convenience.
Apply this diff:
-const URL_PATHS = [ +const DEFAULT_URL_PATHS = [ '/', '/about', '/chapters', '/contribute', '/members', '/organizations', '/projects', '/snapshots', ] + +// Allow overriding routes via env: ROUTES or LHCI_URL_PATHS. +// Accepts a JSON array (e.g., '["/","/about"]') or comma-separated string ('/,/about'). +function parseRoutes(value) { + try { + const v = value.trim() + if (v.startsWith('[')) return JSON.parse(v) + return v.split(',').map((s) => s.trim()).filter(Boolean) + } catch { + console.warn('Invalid ROUTES/LHCI_URL_PATHS; falling back to defaults.') + return [] + } +} + +const URL_PATHS = (() => { + const raw = process.env.ROUTES || process.env.LHCI_URL_PATHS + if (!raw) return DEFAULT_URL_PATHS + const parsed = parseRoutes(raw) + return parsed.length ? parsed : DEFAULT_URL_PATHS +})()
🧹 Nitpick comments (5)
frontend/lighthouserc.js (5)
31-31: Use URL() to safely join base URL and pathsString concatenation risks generating double slashes when LHCI_BASE_URL ends with a trailing slash and/or when paths vary. Using the URL constructor is more robust.
Apply this diff:
- url: URL_PATHS.map((url_path) => `${LHCI_BASE_URL}${url_path}`), + url: URL_PATHS.map((p) => new URL(p, LHCI_BASE_URL).toString()),
27-31: Parameterize numberOfRuns and preset for local vs desktop/mobile runsKeeps scripts and config consistent and avoids duplicating flags in multiple places.
Apply this diff:
+const LHCI_NUMBER_OF_RUNS = Number(process.env.LHCI_NUMBER_OF_RUNS || 1) +const LHCI_PRESET = process.env.LHCI_PRESET // 'desktop' or 'mobile' @@ - numberOfRuns: 1, + numberOfRuns: LHCI_NUMBER_OF_RUNS, settings: { chromeFlags: LHCI_CHROME_FLAGS, + ...(LHCI_PRESET ? { preset: LHCI_PRESET } : {}), },
33-35: Make upload target configurable (temporary-public-storage, filesystem, or lhci server)You mentioned friction with artifact handling. This keeps local dev on temporary-public-storage while allowing CI to switch without editing the file.
Apply this diff:
+const upload = (() => { + const target = process.env.LHCI_UPLOAD_TARGET || 'temporary-public-storage' + const cfg = { target } + if (target === 'lhci') { + if (process.env.LHCI_SERVER_BASE_URL) cfg.serverBaseUrl = process.env.LHCI_SERVER_BASE_URL + if (process.env.LHCI_TOKEN) cfg.token = process.env.LHCI_TOKEN + } + if (target === 'filesystem' && process.env.LHCI_OUTPUT_DIR) { + cfg.outputDir = process.env.LHCI_OUTPUT_DIR + } + return cfg +})() @@ - upload: { - target: 'temporary-public-storage', - }, + upload,
20-24: Optional: allow assertion severity and minScore via env for easier CI/local tuningDefault remains the same; this just exposes knobs if you want stricter CI without editing the file.
Apply this diff:
+const LHCI_ASSERT_LEVEL = process.env.LHCI_ASSERT_LEVEL || 'warn' +const LHCI_MIN_SCORE = Number(process.env.LHCI_MIN_SCORE || 0.9) @@ - 'categories:accessibility': ['warn', { minScore: 0.9 }], - 'categories:best-practices': ['warn', { minScore: 0.9 }], - 'categories:performance': ['warn', { minScore: 0.9 }], - 'categories:seo': ['warn', { minScore: 0.9 }], + 'categories:accessibility': [LHCI_ASSERT_LEVEL, { minScore: LHCI_MIN_SCORE }], + 'categories:best-practices': [LHCI_ASSERT_LEVEL, { minScore: LHCI_MIN_SCORE }], + 'categories:performance': [LHCI_ASSERT_LEVEL, { minScore: LHCI_MIN_SCORE }], + 'categories:seo': [LHCI_ASSERT_LEVEL, { minScore: LHCI_MIN_SCORE }],
16-37: Overall: config meets the PR objective and is immediately usable locallyCJS export is appropriate; assertions are scoped to warn; numberOfRuns=1 keeps dev fast. With the optional tweaks above, this single file can serve both local and CI without duplication.
If you want, I can follow up with a small README snippet showing example commands and env var combinations for local and CI runs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/run-ci-cd.yaml(1 hunks)frontend/lighthouserc.js(1 hunks)frontend/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/package.json
- .github/workflows/run-ci-cd.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (1)
frontend/lighthouserc.js (1)
1-3: Good defaults and env-driven knobs for base URL and Chrome flagsSensibly defaults to localhost and enables common headless flags. Clear and minimal.
* configure lighthouse-ci to run locally * use environment variables instead of flags, upload to artifacts * test CI install pnpm * add spelling, fix artifact dir, include hidden files * Remove artifact upload * Update code * add config for multiple routes/urls * test CI * Update code * Update code * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
3 participants
Resolves #2119
Proposed change
npmandMakefilelighthouserc.jsinstead oflighthouserc.json(allows usage of Environment Variables)module.exportscould not figure out (no documentation) on how to do it the ES6 way.Checklist
make check-testlocally; all checks and tests passed.