fix: fix failing CI/CD on dev branch#1473
Conversation
- projects.test.ts: add missing allow_localhost field to inline snapshot - cross-domain-auth.test.ts: use proper cookie mock that handles js-cookie's Cookies.set - cross-domain-auth.test.ts: use cross-origin afterCallbackRedirectUrl to trigger cross-domain flow Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Map-backed test helper createMockDocument() for stateful document.cookie, applies it across many cross-domain auth tests (updates one redirect URL), updates a projects endpoint snapshot, adjusts a pnpm filter in CI, and extends a dev probe to wait for generated Prisma output. ChangesTest Infrastructure and Expectations Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request fixes recently introduced E2E test failures on the dev branch by updating test fixtures/mocks to match updated runtime behavior in cross-domain handoff and project config responses.
Changes:
- Update the
/api/v1/projects/currentinline snapshot to include the newly returnedallow_localhostconfig field. - Replace a simplistic
document.cookiestub with a cookie-jar-backedDocumentmock sojs-cookiewrites (viagetAllCookiesClient()) don’t overwrite existing test cookies. - Adjust
stack_cross_domain_after_callback_redirect_urlto a hosted-domain (cross-origin) URL so the intended cross-domain authorize flow is exercised.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/e2e/tests/js/cross-domain-auth.test.ts | Improves cookie mocking fidelity and updates redirect URL fixture to reliably enter the cross-domain authorize path. |
| apps/e2e/tests/backend/endpoints/api/v1/projects.test.ts | Updates the current-project snapshot to include allow_localhost in the returned config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR fixes three failing e2e tests introduced by recent commits on the
Confidence Score: 4/5Safe to merge — all changes are confined to e2e test files and fix legitimate test failures without altering any production logic. The createMockDocument cookie setter has no expiry/deletion handling, which won't break anything today but leaves a gap if future tests rely on cookie removal. The plain-object mock also persists across many other tests that don't yet set cookies, meaning the root cause could silently resurface there. apps/e2e/tests/js/cross-domain-auth.test.ts — the remaining plain-object document mocks at multiple locations could cause similar test reliability issues if extended. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function createMockDocument(): Document { | ||
| const cookieJar = new Map<string, string>(); | ||
| return { | ||
| get cookie() { | ||
| return [...cookieJar.entries()].map(([k, v]) => `${k}=${v}`).join('; '); | ||
| }, | ||
| set cookie(str: string) { | ||
| const [nameValue] = str.split(';'); | ||
| const eqIndex = nameValue.indexOf('='); | ||
| if (eqIndex >= 0) { | ||
| cookieJar.set(nameValue.slice(0, eqIndex).trim(), nameValue.slice(eqIndex + 1).trim()); | ||
| } | ||
| }, | ||
| createElement: () => ({}), | ||
| } as any; |
There was a problem hiding this comment.
The createMockDocument setter only handles writing cookies but never removes them: if any code path under test calls document.cookie = "name=; expires=Thu, 01 Jan 1970 00:00:00 GMT" (the standard deletion pattern), the mock will silently store an empty string value instead of evicting the key. This won't break the two tests being fixed here, but the same mock is now shared infrastructure — any future test that relies on cookie expiry/deletion will see stale values and get a confusing false-positive pass.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/e2e/tests/js/cross-domain-auth.test.ts
Line: 5-19
Comment:
**Cookie deletion not supported in mock**
The `createMockDocument` setter only handles writing cookies but never removes them: if any code path under test calls `document.cookie = "name=; expires=Thu, 01 Jan 1970 00:00:00 GMT"` (the standard deletion pattern), the mock will silently store an empty string value instead of evicting the key. This won't break the two tests being fixed here, but the same mock is now shared infrastructure — any future test that relies on cookie expiry/deletion will see stale values and get a confusing false-positive pass.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Addressed in 8cbe5db — the setter now parses expires= directives and evicts the key when the date is in the past.
…lain-object mocks - Add cookie expiry/deletion handling to createMockDocument() - Replace all remaining plain-object document mocks with createMockDocument() for consistency Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
|
Also addressed in 8cbe5db — replaced all 12 remaining plain-object |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/e2e/tests/js/cross-domain-auth.test.ts`:
- Around line 5-20: Add a brief explanatory comment immediately above the "as
any" cast in createMockDocument explaining that the cast is intentional because
implementing the full Document interface is impractical for this test mock; note
that the mock intentionally provides only the minimal subset used by tests (the
cookie getter/setter and createElement) and that test coverage validates the
mock's behavior to ensure runtime type-safety. Reference createMockDocument, the
internal cookieJar and the cookie getter/setter and createElement to make clear
which parts are intentionally omitted.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4cbd99f-bd90-452b-bf62-ffd302f8de47
📒 Files selected for processing (2)
apps/e2e/tests/backend/endpoints/api/v1/projects.test.tsapps/e2e/tests/js/cross-domain-auth.test.ts
- Add Prisma client directory check to wait-for-dev-package-imports.ts probe so codegen-docs waits for prisma generate to complete - Fix QEMU build pnpm install filter to include @stackframe/dashboard dep tree (turbo task graph requires it for stack-cli build) Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
devin-ai-integration
Bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/qemu-emulator-build.yaml:
- Around line 151-156: The workflow's pnpm install for the test job omits the
dashboard package filter even though turbo.json shows
`@stackframe/stack-cli`#build dependsOn
`@stackframe/dashboard`#build:rde-standalone; update the test job's pnpm install
command to include --filter '`@stackframe/dashboard`...' alongside --filter
'`@stackframe/stack-cli`...' so devDependencies for dashboard (e.g., tailwindcss)
are installed, or alternatively add a short comment explaining why the dashboard
build is intentionally excluded despite the dependsOn in turbo.json.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a43f8d85-f5e9-4847-a108-9232bd3bb2ea
📒 Files selected for processing (3)
.github/workflows/qemu-emulator-build.yamlapps/e2e/tests/js/cross-domain-auth.test.tsscripts/wait-for-dev-package-imports.ts
Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
Fix three test failures introduced by recent commits on dev:
allow_localhostfield to inline snapshotgetAllCookiesClient()'s internalCookies.setdoesn't overwrite test cookiesafterCallbackRedirectUrlsoplanRedirectToHandlercorrectly enters the cross-domain-authorize flowcreateMockDocument()for consistencyLink to Devin session: https://app.devin.ai/sessions/e0f695fe8704431ab9daad5a74662682
Requested by: @N2D4
Summary by CodeRabbit