fix: handle special characters in collection path for dotenv watcher#7190
fix: handle special characters in collection path for dotenv watcher#7190pooja-bruno wants to merge 2 commits intousebruno:mainfrom
Conversation
pooja-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
|
No actionable comments were generated in the recent review. 🎉 WalkthroughReplaced glob-pattern .env watching with direct directory watching (chokidar) using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🧹 Nitpick comments (3)
packages/bruno-electron/src/app/dotenv-watcher.js (1)
197-207: Nit:pathincloseAllshadows the module-level import.The destructured
pathvariable in bothfor...ofloops is unused and shadows thepathimport at Line 2. Use_or skip the key entirely.🔧 Proposed fix
- for (const [path, watcher] of this.collectionWatchers) { + for (const [, watcher] of this.collectionWatchers) { watcher.close(); } this.collectionWatchers.clear(); - for (const [path, watcher] of this.workspaceWatchers) { + for (const [, watcher] of this.workspaceWatchers) { watcher.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/dotenv-watcher.js` around lines 197 - 207, The closeAll method currently destructures a `path` variable in both loops which shadows the module-level `path` import and is unused; update the loops in closeAll (referencing collectionWatchers and workspaceWatchers) to avoid binding `path` — e.g., skip the key by using an ignored placeholder or iterate over the Map/collection values (use `for (const [, watcher] of this.collectionWatchers)` or `for (const watcher of this.collectionWatchers.values())`, and similarly for workspaceWatchers) so the module-level `path` import is not shadowed.tests/dotenv/special-chars-collection-path/dotenv-special-chars.spec.ts (2)
26-44: Preferdata-testidselectors over CSS class selectors.Lines 26, 31, 35, and 43 all rely on CSS class names (
.section-header,.section-actions button,.environment-name-input,.section-badge). These are brittle — any styling/refactoring change breaks the test silently. Theenvironment-selector-triggeron Line 23 correctly usesdata-testid; the same pattern should apply here.As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dotenv/special-chars-collection-path/dotenv-special-chars.spec.ts` around lines 26 - 44, The test uses brittle CSS class selectors ('.section-header', '.section-actions button', '.environment-name-input', '.section-badge') via the locators dotEnvSection, addButton, nameInput, and dotEnvBadge; update these to use stable data-testid attributes instead (e.g., data-testid="section-header", data-testid="section-actions", data-testid="environment-name-input", data-testid="section-badge") and adjust the calls that reference dotEnvSection, addButton, nameInput, and dotEnvBadge so the locators use page.getByTestId or locator('[data-testid="..."]') consistently.
9-46: Test only exercises()— consider expanding special-character coverage.The test title and the PR fix claim to handle "special characters in path", but only parentheses
()are exercised. Characters like[,],{,},!, and#are also interpreted specially by glob engines and would be valuable to cover, either as additionaltest()cases or parameterised withtest.each. Based on coding guidelines: "Cover both the 'happy path' and the realistically problematic paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dotenv/special-chars-collection-path/dotenv-special-chars.spec.ts` around lines 9 - 46, The test only covers parentheses in collectionName; update the test suite to parameterize or add additional test cases that exercise other glob-sensitive special characters (e.g., '[', ']', '{', '}', '!', '#') so the watcher/path handling is validated more broadly: refactor the existing test('should detect .env file in collection with brackets in folder name') to use test.each or create separate test() entries that call the same helpers (createTmpDir, createCollection, createEnvironment) with collectionName values containing those characters, then repeat the same steps (environment selector interactions and assertions using page.getByTestId, dotEnvSection, addButton, nameInput, and final dotEnvBadge expectation) for each special-character name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/dotenv/special-chars-collection-path/dotenv-special-chars.spec.ts`:
- Around line 34-36: The comment says "Type the .env file name and press Enter"
but the test only calls nameInput.press('Enter') without filling the input;
either update the comment to state that the default value is accepted or
explicitly fill the input before pressing Enter (e.g., call
nameInput.fill('.env') then nameInput.press('Enter')) to make intent explicit;
update the comment near the locator variable nameInput
('.environment-name-input') and the press('Enter') call accordingly.
---
Nitpick comments:
In `@packages/bruno-electron/src/app/dotenv-watcher.js`:
- Around line 197-207: The closeAll method currently destructures a `path`
variable in both loops which shadows the module-level `path` import and is
unused; update the loops in closeAll (referencing collectionWatchers and
workspaceWatchers) to avoid binding `path` — e.g., skip the key by using an
ignored placeholder or iterate over the Map/collection values (use `for (const
[, watcher] of this.collectionWatchers)` or `for (const watcher of
this.collectionWatchers.values())`, and similarly for workspaceWatchers) so the
module-level `path` import is not shadowed.
In `@tests/dotenv/special-chars-collection-path/dotenv-special-chars.spec.ts`:
- Around line 26-44: The test uses brittle CSS class selectors
('.section-header', '.section-actions button', '.environment-name-input',
'.section-badge') via the locators dotEnvSection, addButton, nameInput, and
dotEnvBadge; update these to use stable data-testid attributes instead (e.g.,
data-testid="section-header", data-testid="section-actions",
data-testid="environment-name-input", data-testid="section-badge") and adjust
the calls that reference dotEnvSection, addButton, nameInput, and dotEnvBadge so
the locators use page.getByTestId or locator('[data-testid="..."]')
consistently.
- Around line 9-46: The test only covers parentheses in collectionName; update
the test suite to parameterize or add additional test cases that exercise other
glob-sensitive special characters (e.g., '[', ']', '{', '}', '!', '#') so the
watcher/path handling is validated more broadly: refactor the existing
test('should detect .env file in collection with brackets in folder name') to
use test.each or create separate test() entries that call the same helpers
(createTmpDir, createCollection, createEnvironment) with collectionName values
containing those characters, then repeat the same steps (environment selector
interactions and assertions using page.getByTestId, dotEnvSection, addButton,
nameInput, and final dotEnvBadge expectation) for each special-character name.
| // Type the .env file name and press Enter | ||
| const nameInput = page.locator('.environment-name-input'); | ||
| await nameInput.press('Enter'); |
There was a problem hiding this comment.
Misleading comment — no fill() is called before pressing Enter.
The comment reads "Type the .env file name and press Enter", but only nameInput.press('Enter') is invoked, silently relying on a default input value of .env. Either add an explicit fill() call (clearer intent, less brittle) or update the comment to reflect that the default name is accepted as-is.
🔧 Proposed clarification
- // Type the .env file name and press Enter
- const nameInput = page.locator('.environment-name-input');
- await nameInput.press('Enter');
+ // Accept the default .env file name and press Enter
+ const nameInput = page.locator('.environment-name-input');
+ await expect(nameInput).toBeVisible();
+ await nameInput.press('Enter');📝 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.
| // Type the .env file name and press Enter | |
| const nameInput = page.locator('.environment-name-input'); | |
| await nameInput.press('Enter'); | |
| // Accept the default .env file name and press Enter | |
| const nameInput = page.locator('.environment-name-input'); | |
| await expect(nameInput).toBeVisible(); | |
| await nameInput.press('Enter'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/dotenv/special-chars-collection-path/dotenv-special-chars.spec.ts`
around lines 34 - 36, The comment says "Type the .env file name and press Enter"
but the test only calls nameInput.press('Enter') without filling the input;
either update the comment to state that the default value is accepted or
explicitly fill the input before pressing Enter (e.g., call
nameInput.fill('.env') then nameInput.press('Enter')) to make intent explicit;
update the comment near the locator variable nameInput
('.environment-name-input') and the press('Enter') call accordingly.
1 participant
Description
Issue: #7178
JIRA
Contribution Checklist:
Screen.Recording.2026-02-18.at.2.18.07.PM.mov
Summary by CodeRabbit
Bug Fixes
Tests