ci: avoid redundant application builds - #29
Conversation
|
@coderabbitai review |
|
📝 WalkthroughWalkthroughThe project adds Vite and Vitest configuration, introduces URL utilities, updates ESM import paths and ESLint resolution, and separates Node, frontend, unit, and CI test commands. ChangesBuild and test migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/utils/combineurl.mjs`:
- Around line 10-13: Update combineRelativeUrlParts to normalize both inputs
before joining: remove trailing slash characters from a non-empty base and
leading slash characters from path, then concatenate them with exactly one
separator while preserving behavior for empty bases.
- Around line 1-8: Update combineUrl to parse base into a URL first, normalize
only its pathname by ensuring it ends with '/', and resolve url against that URL
object so existing query and fragment components remain intact. Add regression
tests covering bases with queries or fragments and relative URLs.
In `@test/combineurl.test.js`:
- Around line 4-12: Expand the combineurl tests in the existing describe/before
setup to cover a non-empty base with a leading-slash path, a combineUrl base
containing a query or fragment, and malformed base input. Assert the exact
returned URL/path values for valid boundary cases and the expected thrown errors
for malformed input, using the imported combineUrl and combineRelativeUrlParts
functions.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 24393f8c-6bbd-4252-815f-876e04ed3256
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/dev-image.yml.github/workflows/release.ymleslint.config.mjspackage.jsonserver.jssrc/utils/combineurl.mjstest/combineurl.test.jstest/config-public.test.jstest/socket-close.test.jsvite.config.mjs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@src/utils/combineurl.mjs`:
- Around line 10-13: Update combineRelativeUrlParts to normalize both inputs
before joining: remove trailing slash characters from a non-empty base and
leading slash characters from path, then concatenate them with exactly one
separator while preserving behavior for empty bases.
- Around line 1-8: Update combineUrl to parse base into a URL first, normalize
only its pathname by ensuring it ends with '/', and resolve url against that URL
object so existing query and fragment components remain intact. Add regression
tests covering bases with queries or fragments and relative URLs.
In `@test/combineurl.test.js`:
- Around line 4-12: Expand the combineurl tests in the existing describe/before
setup to cover a non-empty base with a leading-slash path, a combineUrl base
containing a query or fragment, and malformed base input. Assert the exact
returned URL/path values for valid boundary cases and the expected thrown errors
for malformed input, using the imported combineUrl and combineRelativeUrlParts
functions.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 24393f8c-6bbd-4252-815f-876e04ed3256
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/dev-image.yml.github/workflows/release.ymleslint.config.mjspackage.jsonserver.jssrc/utils/combineurl.mjstest/combineurl.test.jstest/config-public.test.jstest/socket-close.test.jsvite.config.mjs
🛑 Comments failed to post (2)
src/utils/combineurl.mjs (2)
1-8: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- target file ---' cat -n src/utils/combineurl.mjs printf '%s\n' '--- usages and tests ---' rg -n --glob '!node_modules' 'combineUrl|combineurl' . printf '%s\n' '--- package metadata ---' fd -HI 'package.json|.*test.*|.*spec.*' . | head -80Repository: chrisae9/synclounge
Length of output: 5408
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- combineurl tests ---' cat -n test/combineurl.test.js printf '%s\n' '--- call-site context ---' sed -n '55,100p' src/store/modules/synclounge/actions.js printf '%s\n' '--- native URL behavior ---' node - <<'JS' const cases = [ ['child', 'https://example.com/api?tenant=1'], ['child', 'https://example.com/api#section'], ['`#new`', 'https://example.com/api?tenant=1'], ['`#new`', 'https://example.com/api#section'], ['/child', 'https://example.com/api?tenant=1'], ]; for (const [url, base] of cases) { const currentBase = base.endsWith('/') ? base : `${base}/`; const current = new URL(url, currentBase).href; const fixedBase = new URL(base); if (!fixedBase.pathname.endsWith('/')) fixedBase.pathname += '/'; const proposed = new URL(url, fixedBase).href; console.log(JSON.stringify({url, base, current, proposed})); } JSRepository: chrisae9/synclounge
Length of output: 5231
Normalize
URL.pathname, not the serialized base string.When
basecontains a query or fragment, Line 5 appends/to that component. A relativeurlcan then resolve against the wrong pathname or modify the query. Parsebasefirst, append/topathname, and add regression tests.🤖 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 `@src/utils/combineurl.mjs` around lines 1 - 8, Update combineUrl to parse base into a URL first, normalize only its pathname by ensuring it ends with '/', and resolve url against that URL object so existing query and fragment components remain intact. Add regression tests covering bases with queries or fragments and relative URLs.
10-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize leading slashes in
path.Checking only the end of
basedoes not guarantee one separator. A non-empty base plus a path beginning with/produces//. The same occurs when both inputs contain a slash. This can change the HTTP request path. Remove trailing slashes frombaseand leading slashes frompath.Proposed fix
export const combineRelativeUrlParts = (base, path) => (!base - || base.charAt(base.length - 1) === '/' - ? `${base}${path}` - : `${base}/${path}`); + ? `${base}${path}` + : `${base.replace(/\/+$/, '')}/${path.replace(/^\/+/, '')}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const combineRelativeUrlParts = (base, path) => (!base ? `${base}${path}` : `${base.replace(/\/+$/, '')}/${path.replace(/^\/+/, '')}`);🤖 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 `@src/utils/combineurl.mjs` around lines 10 - 13, Update combineRelativeUrlParts to normalize both inputs before joining: remove trailing slash characters from a non-empty base and leading slash characters from path, then concatenate them with exactly one separator while preserving behavior for empty bases.
fd88c57 to
d8cf9d6
Compare
|
Addressed all three CodeRabbit findings in 60d5631, including the two comments CodeRabbit could not post inline: |
1 participant
Summary
Verification
SKIP_BUILD=true npm cinpm run build && npm run test:ci(166 Node tests and 243 Vitest tests)npm testwith exactly one Vite build and no module-type warningsnpm audit(0 vulnerabilities)git diff --checkAll CodeRabbit findings were addressed. Draft while final GitHub checks complete.