Fix (import): Postman import: OAuth2 tokenPlacement not set correctly, Header Prefix field hidden and value lost#8197
Conversation
…, Header Prefix field hidden and value lost
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ChangesOAuth2 Token Placement Support and Test Coverage
Sequence DiagramsequenceDiagram
participant ElectronDialog
participant ImportService
participant postmanToBruno
participant BrunoAppUI
participant AuthTab
participant PlaywrightTest
ElectronDialog->>ImportService: showOpenDialog returns fixture path
ImportService->>postmanToBruno: processAuth(parse collection)
postmanToBruno->>ImportService: return Bruno collection (includes tokenHeaderPrefix, tokenQueryKey)
ImportService->>BrunoAppUI: load collection into app
PlaywrightTest->>BrunoAppUI: open request and switch to Auth tab
BrunoAppUI->>AuthTab: render OAuth2 fields based on tokenPlacement and tokenHeaderPrefix/tokenQueryKey
PlaywrightTest->>AuthTab: assert header-prefix visibility/value or query-param-key visibility/value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/import/postman/import-oauth2-token-placement-collection.spec.ts (1)
58-58: ⚡ Quick winReplace hardcoded timeout literals in negative visibility assertions.
Using
{ timeout: 1000 }adds magic timing and can make the spec flaky across CI load levels. Prefer event-driven assertions without fixed short waits.As per coding guidelines, “Replace magic timeouts with event-driven waits in E2E tests. Replace brittle text/index selectors with role, label, test id, or stable user-facing selectors.”
Also applies to: 67-67
🤖 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 `@tests/import/postman/import-oauth2-token-placement-collection.spec.ts` at line 58, Replace the hardcoded short timeout on the negative visibility assertion for oauth2.tokenQueryParamKeyField(): instead of passing { timeout: 1000 }, wait for the UI change via an event-driven API and then assert absence—e.g., trigger the action that should hide/remove the field, await the stable wait (page.waitForSelector or the wrapper's waitFor with state: 'hidden' or 'detached') using a stable selector (role/label/test-id) and then call await expect(oauth2.tokenQueryParamKeyField()).not.toBeVisible() without a magic timeout; apply the same change to the other occurrence mentioned at line 67.Sources: Coding guidelines, Learnings
🤖 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 `@tests/import/postman/import-oauth2-token-placement-collection.spec.ts`:
- Around line 52-68: Add assertions that verify the actual persisted OAuth2
values, not just visibility: after selecting 'OAuth2 Token in Header' use
locators.sidebar.request(...) and selectRequestPaneTab to open the Auth pane and
assert oauth2.tokenHeaderPrefixField() contains the expected prefix value
("Bearer") and that oauth2.tokenQueryParamKeyField() is empty or unchanged;
likewise after selecting 'OAuth2 Token in URL' assert
oauth2.tokenQueryParamKeyField() contains the expected query key (e.g.,
"access_token") and oauth2.tokenHeaderPrefixField() is empty or unchanged so the
import mapping of values is validated.
- Around line 7-19: The stub restoration is fragile because
originalShowOpenDialog is captured inside electronApp.evaluate without returning
it to the test context; change the setup to capture the original by returning it
from electronApp.evaluate (e.g., originalShowOpenDialog = await
electronApp.evaluate(({ dialog }) => dialog.showOpenDialog)) and restore it by
passing it back into the browser context in afterAll (e.g., await
electronApp.evaluate((o) => { dialog.showOpenDialog = o },
originalShowOpenDialog)); also extend the test assertions to verify the imported
OAuth2 token placement values (not just visibility) after import and replace the
hardcoded not.toBeVisible timeout with an event-driven wait/assert (use
locator.waitFor or expect(...).toBeHidden/waitFor with the relevant locators) so
the test waits for state changes reliably; keep references to
originalShowOpenDialog, electronApp.evaluate, closeAllCollections, and the token
placement locators when making these changes.
---
Nitpick comments:
In `@tests/import/postman/import-oauth2-token-placement-collection.spec.ts`:
- Line 58: Replace the hardcoded short timeout on the negative visibility
assertion for oauth2.tokenQueryParamKeyField(): instead of passing { timeout:
1000 }, wait for the UI change via an event-driven API and then assert
absence—e.g., trigger the action that should hide/remove the field, await the
stable wait (page.waitForSelector or the wrapper's waitFor with state: 'hidden'
or 'detached') using a stable selector (role/label/test-id) and then call await
expect(oauth2.tokenQueryParamKeyField()).not.toBeVisible() without a magic
timeout; apply the same change to the other occurrence mentioned at line 67.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 712c8581-1a7b-430e-8cb7-943e2c0530b4
📒 Files selected for processing (8)
packages/bruno-app/src/components/RequestPane/Auth/OAuth2/AuthorizationCode/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientCredentials/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/Implicit/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/PasswordCredentials/index.jspackages/bruno-converters/src/postman/postman-to-bruno.jstests/import/postman/fixtures/postman-import-oauth2-token-placement-collection.jsontests/import/postman/import-oauth2-token-placement-collection.spec.tstests/utils/page/locators.ts
# Conflicts: # tests/utils/page/locators.ts
https://usebruno.atlassian.net/browse/BRU-3283
Description
Added tokenHeaderPrefix and tokenQueryKey to baseOAuth2Config object in postman-to-bruno.js. When tokenPlacement is header, it shows value of tokenHeaderPrefix. If tokenPlacement is url, it shows value of tokenQueryKey.
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Tests
Bug Fixes