Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
This PR aims to restore the “locked” indicators in the desktop app’s worktree change lists so users can see file dependencies for assigned changes.
Changes:
- Always enables
showLockedIndicatorforFileListrendering inWorktreeChanges. - Refactors Playwright e2e setup to use numeric Butler ports and adds more forceful teardown logic.
- Adds a Cargo alias for workspace clippy linting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
e2e/playwright/src/setup.ts |
Changes Butler port typing/usage and adds more aggressive process shutdown/port cleanup. |
apps/desktop/src/components/WorktreeChanges.svelte |
Enables lock indicator display by always passing showLockedIndicator to FileList. |
.cargo/config.toml |
Adds a cargo lint alias that runs clippy with warnings denied. |
e2e/playwright/src/setup.ts
Outdated
| export function getButlerPort(): number { | ||
| // Zero based parallel counter | ||
| const id = parseInt(process.env.TEST_PARALLEL_INDEX ?? '0', 10); | ||
| // Start from default + 1 to avoid interfering with dev server | ||
| return `${parseInt(BUT_SERVER_PORT, 10) + id + 1}`; | ||
| const port = parseInt(BUT_SERVER_PORT, 10) + id + 1; | ||
| return port; |
There was a problem hiding this comment.
getButlerPort() now returns a number, but this function is also used to populate environment variables (e.g. BUTLER_PORT in serverEnv). child_process.spawn expects env values to be strings, so this change will either break TypeScript (Record<string, string>) or pass a non-string env value at runtime. Consider keeping the return type as string, or converting to string at each env/cookie call site (and adjusting types accordingly).
e2e/playwright/src/setup.ts
Outdated
| // Also kill any processes that might still be holding the port | ||
| const port = getButlerPort(); | ||
| try { | ||
| execSync(`lsof -ti :${port} | xargs kill -9 2>/dev/null || true`, { stdio: 'ignore' }); | ||
| } catch { |
There was a problem hiding this comment.
execSync is used here but isn’t imported in this file. This will fail at runtime/compile-time; import it from node:child_process (or avoid it if not needed).
e2e/playwright/src/setup.ts
Outdated
| try { | ||
| execSync(`lsof -ti :${port} | xargs kill -9 2>/dev/null || true`, { stdio: 'ignore' }); | ||
| } catch { |
There was a problem hiding this comment.
The lsof | xargs kill -9 cleanup command is POSIX-specific (and also assumes lsof is installed). If this e2e harness is expected to run on non-Linux environments, please guard this by platform/availability or use a cross-platform approach to free the port; otherwise failures here will be silently swallowed and ports may remain occupied.
.cargo/config.toml
Outdated
| [alias] | ||
| lint = "clippy --workspace --all-targets -- -D warnings" |
There was a problem hiding this comment.
This PR is described as only bringing back lock icons for assigned changes, but it also adds a Cargo alias and changes Playwright e2e server lifecycle/port handling. If these are required for the UI change, please explain the linkage in the PR description; otherwise consider splitting these unrelated changes into separate PRs to keep review/rollback scoped.
They were turned off by mistake.
1 participant
They were turned off by mistake, and they're useful for knowing what commits files depend on.