fix(timeline): scope scripted requests to their own request#8210
Open
pooja-bruno wants to merge 2 commits into
Open
fix(timeline): scope scripted requests to their own request#8210pooja-bruno wants to merge 2 commits into
pooja-bruno wants to merge 2 commits into
Conversation
pooja-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds data-testid attributes across Timeline UI, updates empty-state texts, extends ENTRY_KINDS with stable kind IDs, refactors Timeline tests to use getByTestId selectors (and adds scoped-attribution e2e tests), and updates request pathname fallback in script merging. ChangesTimeline Test Infrastructure
Request Pathname Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/request/timeline/timeline-scoped-request-attribution.spec.ts (2)
91-91: ⚡ Quick winAvoid
.collection-item-namewith.first()for request selection.Selecting by class + positional fallback can become flaky when sidebar ordering changes; use an existing helper (like
openRequest) or a stable test-id-based locator.As per coding guidelines, “Replace brittle text/index selectors with role, label, test id, or stable user-facing selectors.”
🤖 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/request/timeline/timeline-scoped-request-attribution.spec.ts` at line 91, Replace the brittle selector that uses page.locator('.collection-item-name').filter({ hasText: requestName }).first().click(): instead call the existing test helper openRequest(requestName) or switch to a stable test-id/role-based locator (e.g., use getByTestId('request-item') or getByRole/getByLabel with the requestName) so selection doesn't rely on class+position; update the call referencing the requestName variable and remove use of '.collection-item-name' and .first() to make the test robust.Source: Coding guidelines
66-66: ⚡ Quick winUse a stable selector instead of
.request-tab.active.This class-based assertion is brittle under UI refactors; prefer a test-id/role-based locator for the active request tab state.
As per coding guidelines, “Replace brittle text/index selectors with role, label, test id, or stable user-facing selectors.”
🤖 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/request/timeline/timeline-scoped-request-attribution.spec.ts` at line 66, The test uses a brittle class-based selector page.locator('.request-tab.active') to assert the active request tab; replace it with a stable role/test-id-based locator that targets the tab by its accessible role or a data-testid and the request label (e.g., use the tab role with the requestName or a data-testid like request-tab combined with an active attribute) so the assertion checks the element with role "tab" (or data-testid "request-tab") whose name/text equals requestName and is active; update the locator in the spec to use that stable selector instead of '.request-tab.active'.Source: Coding guidelines
tests/request/timeline/timeline-url-update.spec.ts (1)
59-59: ⚡ Quick winReplace hardcoded timeout with event-driven wait.
The 200ms timeout after typing the URL should be replaced with an explicit wait for CodeMirror to process the input or for the request to be saved. Consider waiting for a stable state indicator, debounce completion, or an observable change in the request configuration.
Suggested alternatives
Option 1: Wait for CodeMirror to stabilize by checking the value:
await page.keyboard.type(secondUrl); - await page.waitForTimeout(200); + await expect(urlEditor).toContainText(secondUrl, { timeout: 1000 });Option 2: If there's a save indicator or debounce, wait for that specific element/state.
🤖 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/request/timeline/timeline-url-update.spec.ts` at line 59, Replace the hardcoded await page.waitForTimeout(200) with an event-driven wait: detect when CodeMirror has processed the typed URL or when the request save completes (e.g., wait for the CodeMirror editor's value to equal the expected URL using page.waitForFunction or wait for a save/debounce indicator selector to appear/disappear). Update the test in timeline-url-update.spec.ts where page.waitForTimeout(200) is used to instead wait for the editor state or save indicator to be stable before proceeding so the assertion uses the final, persisted request value.tests/request/timeline/timeline-nested-runrequest.spec.ts (1)
76-77: 💤 Low valueConsider extracting the
countForhelper to a shared utility.This inline helper appears in both this file and
timeline-scripted-requests.spec.ts(line 65-66). Since the pattern is identical and used across multiple test files, consider extracting it to a shared test utility (e.g.,tests/utils/page/timeline.ts) to reduce duplication.Example extraction
In
tests/utils/page/timeline.ts:export const getTimelineChipCount = (page, chipId: string) => page.getByTestId(`timeline-chip-${chipId}`).getByTestId('timeline-chip-count');Then in tests:
import { getTimelineChipCount } from '../../utils/page/timeline'; await expect(getTimelineChipCount(page, 'all')).toHaveText('3');🤖 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/request/timeline/timeline-nested-runrequest.spec.ts` around lines 76 - 77, Extract the duplicate inline helper `countFor` into a shared test utility (e.g., export a function named `getTimelineChipCount(page, chipId)` in a new module like tests/utils/page/timeline.ts) and replace the local `countFor` definitions in both `timeline-nested-runrequest.spec.ts` and `timeline-scripted-requests.spec.ts` with an import of `getTimelineChipCount`; update usages (e.g., countFor(page, 'all') -> getTimelineChipCount(page, 'all')) and add the new import statement in each test file so the helper is centralized and reused.tests/request/timeline/timeline-scripted-requests.spec.ts (1)
65-66: 💤 Low valueConsider extracting the
countForhelper to a shared utility.This inline helper is duplicated from
timeline-nested-runrequest.spec.ts(line 76-77). Since the pattern is identical and used across multiple test files, consider extracting it to a shared test utility (e.g.,tests/utils/page/timeline.ts) to reduce duplication.Example extraction
In
tests/utils/page/timeline.ts:export const getTimelineChipCount = (page, chipId: string) => page.getByTestId(`timeline-chip-${chipId}`).getByTestId('timeline-chip-count');Then in tests:
import { getTimelineChipCount } from '../../utils/page/timeline'; await expect(getTimelineChipCount(page, 'all')).toHaveText('4');🤖 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/request/timeline/timeline-scripted-requests.spec.ts` around lines 65 - 66, Extract the duplicated inline helper countFor from tests/request/timeline/timeline-scripted-requests.spec.ts into a shared test utility (e.g., export function getTimelineChipCount(page, chipId) in tests/utils/page/timeline.ts) and replace local countFor usages with imports of getTimelineChipCount; also update timeline-nested-runrequest.spec.ts to import the same utility so both tests use the single exported helper (ensure the helper returns page.getByTestId(`timeline-chip-${chipId}`).getByTestId('timeline-chip-count')).
🤖 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
`@packages/bruno-app/src/components/ResponsePane/Timeline/TimelineItem/Common/Headers/index.js`:
- Line 34: Update the failing E2E assertion to match the new empty-state copy:
in the test suite "folder-no-auth-stops-inheritance"
(tests/auth/auth-mode/folder-no-auth-stops-inheritance.spec.ts) locate the
assertion that currently expects "No Headers found" and change the expected
string to "No Headers" so it matches the TimelineItem Headers component's
updated output; run the test to confirm it passes.
---
Nitpick comments:
In `@tests/request/timeline/timeline-nested-runrequest.spec.ts`:
- Around line 76-77: Extract the duplicate inline helper `countFor` into a
shared test utility (e.g., export a function named `getTimelineChipCount(page,
chipId)` in a new module like tests/utils/page/timeline.ts) and replace the
local `countFor` definitions in both `timeline-nested-runrequest.spec.ts` and
`timeline-scripted-requests.spec.ts` with an import of `getTimelineChipCount`;
update usages (e.g., countFor(page, 'all') -> getTimelineChipCount(page, 'all'))
and add the new import statement in each test file so the helper is centralized
and reused.
In `@tests/request/timeline/timeline-scoped-request-attribution.spec.ts`:
- Line 91: Replace the brittle selector that uses
page.locator('.collection-item-name').filter({ hasText: requestName
}).first().click(): instead call the existing test helper
openRequest(requestName) or switch to a stable test-id/role-based locator (e.g.,
use getByTestId('request-item') or getByRole/getByLabel with the requestName) so
selection doesn't rely on class+position; update the call referencing the
requestName variable and remove use of '.collection-item-name' and .first() to
make the test robust.
- Line 66: The test uses a brittle class-based selector
page.locator('.request-tab.active') to assert the active request tab; replace it
with a stable role/test-id-based locator that targets the tab by its accessible
role or a data-testid and the request label (e.g., use the tab role with the
requestName or a data-testid like request-tab combined with an active attribute)
so the assertion checks the element with role "tab" (or data-testid
"request-tab") whose name/text equals requestName and is active; update the
locator in the spec to use that stable selector instead of
'.request-tab.active'.
In `@tests/request/timeline/timeline-scripted-requests.spec.ts`:
- Around line 65-66: Extract the duplicated inline helper countFor from
tests/request/timeline/timeline-scripted-requests.spec.ts into a shared test
utility (e.g., export function getTimelineChipCount(page, chipId) in
tests/utils/page/timeline.ts) and replace local countFor usages with imports of
getTimelineChipCount; also update timeline-nested-runrequest.spec.ts to import
the same utility so both tests use the single exported helper (ensure the helper
returns
page.getByTestId(`timeline-chip-${chipId}`).getByTestId('timeline-chip-count')).
In `@tests/request/timeline/timeline-url-update.spec.ts`:
- Line 59: Replace the hardcoded await page.waitForTimeout(200) with an
event-driven wait: detect when CodeMirror has processed the typed URL or when
the request save completes (e.g., wait for the CodeMirror editor's value to
equal the expected URL using page.waitForFunction or wait for a save/debounce
indicator selector to appear/disappear). Update the test in
timeline-url-update.spec.ts where page.waitForTimeout(200) is used to instead
wait for the editor state or save indicator to be stable before proceeding so
the assertion uses the final, persisted request value.
🪄 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: 8413b754-f0e2-4c0c-a8be-ad48e86b5201
📒 Files selected for processing (13)
packages/bruno-app/src/components/ResponsePane/Timeline/TimelineItem/Common/Body/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/TimelineItem/Common/Headers/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/TimelineItem/Common/Status/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/TimelineItem/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/entryMeta.jspackages/bruno-app/src/components/ResponsePane/Timeline/index.jspackages/bruno-electron/src/utils/collection.jstests/request/timeline/timeline-nested-runrequest.spec.tstests/request/timeline/timeline-runrequest-network-error.spec.tstests/request/timeline/timeline-runrequest-skip.spec.tstests/request/timeline/timeline-scoped-request-attribution.spec.tstests/request/timeline/timeline-scripted-requests.spec.tstests/request/timeline/timeline-url-update.spec.ts
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
1 participant
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.
Description
JIRA
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.
packages/bruno-app
data-testid) across Timeline components (Timeline, TimelineItem, Status, Headers, Body, entryMeta). ENTRY_KINDS now include an explicit stablekindfield for main/oauth/pre/post which centralizes kind identity used by UI and test IDs.packages/bruno-electron
tests (tests/request and tests/auth)
data-testidselectors (timeline-container, timeline-entry, timeline-badge-, timeline-url, timeline-chip-, timeline-status, timeline-filter-bar, timeline-detail, timeline-source-link/file, etc.) instead of CSS classes.data-testids and the revised displayPath behavior from mergeScripts for accurate source assertions.Inter-package dependencies and notes
Overall risk/effort