🔐 Address PR #231 review findings: env leak, path resolution, a11y, CI hardening - #232
Merged
Conversation
… publishing SemVer prerelease identifiers must not be purely-numeric with a leading zero (e.g. v1.0.0-01), but the release tag regex allowed any [0-9A-Za-z-]+ run. Also pin the npm upgrade step to a tested version instead of floating npm@latest, so the OIDC trusted-publishing toolchain stays reproducible.
The close control was a <div>, so keyboard-only users couldn't focus or activate it — undismissable when duration is 0. Render it as a native <button type="button"> with an aria-label, and reset default button chrome in CSS while keeping a visible focus ring.
loadEnv() folds already-set process.env values matching the prefix back into its result, so a value loaded for one project root survived into the next root's load even when that root's own .env didn't set it — and could persist across watcher restarts. Clear the keys we previously wrote before each reload, so only the current root's files (or genuinely external process.env vars) apply.
DistService and writeBuildVersionManifest resolved relative backup/ build/dist-zip/output-manifest paths against process.cwd(), which only matches the intended project root by coincidence. Thread an explicit root through DistService (from the resolved Vite/Next.js server config rather than the dead config().root capture) and through writeBuildVersionManifest/versionPlugin (from Vite's resolvedConfig.root), defaulting to process.cwd() for backward compatibility.
loadSchema/loadValues had no rejection handling around fetch()/.json(), so a network failure left the "Refreshing…" indicator stuck forever and the bootstrap/refresh promise chain rejected silently. Wrap the network work in try/catch and surface failures through the existing banner. Also lazily import jsdom in the test file per the heavy-module guideline.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
4 tasks
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
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
Addresses the CodeRabbit review findings left on PR #231 (
develop→main) that we agreed to fix. Skips thepackage.jsonnpx-vs-devDependency suggestion forsemantic-release— that one is intentional (see reply) and left as is.Key changes
v1.0.0-01); tightened to match the SemVer spec. Also pins the npm upgrade step to a tested version instead of floatingnpm@latest.<div>, unreachable from the keyboard (undismissable whenduration: 0). Now a native<button type="button">witharia-label="Close", with button-chrome reset and a visible focus ring in CSS.loadPPDevEnvfolds already-setprocess.envvalues back intoloadEnv()'s result, so a value loaded for one project root survived into the next root's load (and across watcher restarts) even when that root's own.envnever set it. Now clears its previously-written keys before each reload.DistServiceandwriteBuildVersionManifestresolved relative backup/build/dist-zip/manifest paths againstprocess.cwd(), which only matches the intended project root by coincidence. Threaded an explicitrootthroughDistService(from the resolved Vite/Next.js server config) and throughwriteBuildVersionManifest/versionPlugin(from Vite'sresolvedConfig.root), defaulting toprocess.cwd()for backward compatibility.loadSchema/loadValueshad no rejection handling aroundfetch()/.json(), so a network failure left the "Refreshing…" indicator stuck forever. Wrapped intry/catch, surfaced through the existing error banner.jsdominvariables-editor.spec.ts, per the heavy-module coding guideline.Testing
npx tsc --noEmit— cleannpm run lint— cleannpm run test:unit— 323/323 passing (addstests/unit/lib/env.spec.ts,tests/unit/lib/dist.service.root.spec.ts, plus new cases inpopup.spec.ts,variables-editor.spec.ts,version-manifest.spec.ts)npm run test:integration— 39/39 passingMerge Request:
origin/pp-3990-review-fixes→origin/develop