test: add sdk embed and backend vitest coverage#1835
Conversation
superagent-security
Bot
added
the
contributor:verified
There was a problem hiding this comment.
Pull request overview
Adds Vitest setup and initial unit tests to the @cap/sdk-embed and @cap/web-backend packages as part of the broader effort to introduce a testing framework across the monorepo (issue #54).
Changes:
- Adds
vitestdevDependency, a minimalvitest.config.ts, and atestscript to bothpackages/sdk-embedandpackages/web-backend. - Adds
cap-embed.test.tscoveringcreateEmbedUrldefault URL and optional autoplay/branding parameters. - Adds
EffectiveVideoRules.test.tscoveringresolveEffectiveVideoRulesinheritance/precedence, inherited password sources, andcollectPasswordHashesfiltering.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-embed/package.json | Adds test script and vitest devDependency; also reorders deps alphabetically. |
| packages/sdk-embed/vitest.config.ts | Minimal Vitest config (node env, src/**/*.test.ts). |
| packages/sdk-embed/src/vanilla/cap-embed.test.ts | Unit tests for createEmbedUrl. |
| packages/web-backend/package.json | Adds test script and vitest devDependency; moves @cap/env to alphabetical position. |
| packages/web-backend/vitest.config.ts | Minimal Vitest config. |
| packages/web-backend/src/Videos/EffectiveVideoRules.test.ts | Unit tests for resolveEffectiveVideoRules and collectPasswordHashes. |
| pnpm-lock.yaml | Lockfile updates for new Vitest devDependency and incidental transitive bumps. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("lets space settings override video and organization defaults", () => { | ||
| const rules = resolveEffectiveVideoRules({ | ||
| videoSettings: { | ||
| disableCaptions: false, | ||
| disableComments: true, | ||
| }, | ||
| organizationSettings: { | ||
| disableCaptions: true, | ||
| disableTranscript: true, | ||
| }, | ||
| spaces: [ | ||
| { | ||
| id: "space-1", | ||
| name: "Engineering", | ||
| settings: { | ||
| disableCaptions: true, | ||
| disableSummary: true, | ||
| }, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| expect(rules.settings).toMatchObject({ | ||
| disableCaptions: true, | ||
| disableComments: true, | ||
| disableSummary: true, | ||
| disableTranscript: true, | ||
| }); | ||
| expect(rules.inheritedSettings.disableCaptions).toEqual([ | ||
| { id: "space-1", name: "Engineering" }, | ||
| ]); | ||
| expect(rules.inheritedSettings.disableSummary).toEqual([ | ||
| { id: "space-1", name: "Engineering" }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
videoSettings and organizationSettings not covered
The test name promises to exercise "space settings override video and organization defaults," but the branch where videoSettings explicitly sets a flag to false while organizationSettings sets the same flag to true (with no space override) is never reached. For disableCaptions, the space provides an override so the videoSettings?.[key] ?? organizationSettings?.[key] line is bypassed entirely.
Because of ??, an explicit false in videoSettings correctly shadows the org-level true. If that operator were ever changed to ||, this test suite would still pass — making it possible to silently invert the intended "video setting wins" precedence. Consider adding a case where a space array is empty (or excludes a key), videoSettings.someKey = false, and organizationSettings.someKey = true, and asserting that the result is false.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/web-backend/src/Videos/EffectiveVideoRules.test.ts
Line: 8-42
Comment:
**Precedence between `videoSettings` and `organizationSettings` not covered**
The test name promises to exercise "space settings override video and organization defaults," but the branch where `videoSettings` explicitly sets a flag to `false` while `organizationSettings` sets the same flag to `true` (with no space override) is never reached. For `disableCaptions`, the space provides an override so the `videoSettings?.[key] ?? organizationSettings?.[key]` line is bypassed entirely.
Because of `??`, an explicit `false` in `videoSettings` correctly shadows the org-level `true`. If that operator were ever changed to `||`, this test suite would still pass — making it possible to silently invert the intended "video setting wins" precedence. Consider adding a case where a space array is empty (or excludes a key), `videoSettings.someKey = false`, and `organizationSettings.someKey = true`, and asserting that the result is `false`.
How can I resolve this? If you propose a fix, please make it concise.| describe("collectPasswordHashes", () => { | ||
| it("keeps the video password first and removes empty space passwords", () => { | ||
| expect( | ||
| collectPasswordHashes({ | ||
| videoPassword: "video-hash", | ||
| spacePasswords: [ | ||
| { password: "space-hash" }, | ||
| { password: "" }, | ||
| { password: null }, | ||
| {}, | ||
| ], | ||
| }), | ||
| ).toEqual(["video-hash", "space-hash"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
videoPassword
Only the "happy path" (a truthy videoPassword) is covered. The function also handles videoPassword being undefined or null via ...(videoPassword ? [videoPassword] : []). Adding a test case where videoPassword is omitted or null would confirm the guard works, especially if the function signature changes in the future.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/web-backend/src/Videos/EffectiveVideoRules.test.ts
Line: 61-75
Comment:
**No test for absent / null `videoPassword`**
Only the "happy path" (a truthy `videoPassword`) is covered. The function also handles `videoPassword` being `undefined` or `null` via `...(videoPassword ? [videoPassword] : [])`. Adding a test case where `videoPassword` is omitted or `null` would confirm the guard works, especially if the function signature changes in the future.
How can I resolve this? If you propose a fix, please make it concise.
/claim #54
Summary
@cap/sdk-embedand@cap/web-backendcreateEmbedUrldefault and optional embed URL generationresolveEffectiveVideoRulesprecedence/inheritance behavior and password hash collectionVerification
pnpm --filter @cap/sdk-embed testpnpm --filter @cap/web-backend testpnpm exec biome check packages/sdk-embed/package.json packages/sdk-embed/vitest.config.ts packages/sdk-embed/src/vanilla/cap-embed.test.ts packages/web-backend/package.json packages/web-backend/vitest.config.ts packages/web-backend/src/Videos/EffectiveVideoRules.test.tsNotes
Greptile Summary
This PR adds Vitest unit test scaffolding for
@cap/sdk-embedand@cap/web-backend, wiring upvitest ~2.1.9as a dev dependency and atestscript in each package'spackage.json.cap-embed.test.ts: Two tests forcreateEmbedUrl— one validates the default URL shape with only required params, the other exercises all optional params (apiBase,autoplay,branding). Both assertions match the source implementation faithfully.EffectiveVideoRules.test.ts: Three tests covering space-setting overrides, inherited password source collection, andcollectPasswordHashesfiltering. The tests are structurally correct, but the suite does not include a case wherevideoSettingsexplicitly sets a flag tofalsewhileorganizationSettingssets the same flag totruewith no space override — leaving the??vs||precedence distinction untested.Confidence Score: 4/5
Safe to merge — adds test infrastructure and test files only; no production code is changed.
All new test assertions correctly mirror the source. The two gaps noted are coverage suggestions rather than incorrect tests.
packages/web-backend/src/Videos/EffectiveVideoRules.test.ts — the precedence and null-password cases could be filled in to give the test suite more confidence against future refactors.
Important Files Changed
createEmbedUrl: covers default URL shape and all optional params; assertions match the implementation exactly.resolveEffectiveVideoRulesandcollectPasswordHashes; correct assertions but missing coverage forvideoSettings/organizationSettingsprecedence and absentvideoPassword.nodeenvironment and correct glob; no issues.nodeenvironment for pure-logic tests.vitest ~2.1.9devDependency andtestscript; removed emptydependenciesblock.vitest ~2.1.9devDependency andtestscript;@cap/envmoved to its correct sorted position independencies.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "test: add sdk embed and backend coverage" | Re-trigger Greptile