more fixes#10
Open
batuhan wants to merge 1 commit into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds three independent enhancements: watch command support for ChangesWatch command message.stream support
Setup command server installation awareness
Launcher logging and progress improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/setup.ts (1)
283-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-interactive “current target unreachable” output still ignores installed server state.
You added
serverInstalledfor interactive recovery here, but the JSON/non-TTY path still callscurrentTargetBrokenOutput(...), which always emitsinstall-server. This makes automation output inconsistent and can incorrectly suggest reinstalling an already installed server.Suggested fix
- if (flags.json || !process.stdin.isTTY) { - await printData(currentTargetBrokenOutput(target, readiness), flags.json ? 'json' : 'human') + if (flags.json || !process.stdin.isTTY) { + const serverInstalled = await isServerInstalled() + await printData(currentTargetBrokenOutput(target, readiness, serverInstalled), flags.json ? 'json' : 'human') return }-function currentTargetBrokenOutput(target: Target, readiness: Readiness): Record<string, unknown> { +function currentTargetBrokenOutput(target: Target, readiness: Readiness, serverInstalled: boolean): Record<string, unknown> { + const serverAction = installedServerAction(serverInstalled) return { state: 'current-target-unreachable', message: `Beeper CLI is set up for ${target.name ?? target.id}, but it is not reachable.`, target: publicTarget(target), readiness, recommendedAction: action('retry-current', `beeper setup -t ${target.id}`), availableActions: [ action('retry-current', `beeper setup -t ${target.id}`), action('use-desktop', 'beeper setup --desktop'), - action('install-server', 'beeper setup --server --install --yes'), + serverAction, action('connect-remote', 'beeper setup --remote <url>'), ], } }🤖 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 `@packages/cli/src/commands/setup.ts` around lines 283 - 304, The non-interactive path always outputs "install-server" because currentTargetBrokenOutput is called without the actual installed state; update the flow so the result of isServerInstalled() (serverInstalled) is used to determine the JSON/non-TTY output—either by passing serverInstalled into currentTargetBrokenOutput or by adding logic in the non-interactive branch to emit "use-installed-server" when serverInstalled is true (identify the isServerInstalled() call and the currentTargetBrokenOutput(...) invocation to modify). Ensure the JSON/TTY output mirrors the interactive prompt choice text for option 3 instead of always suggesting installation.
🤖 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/npm/scripts/build.ts`:
- Around line 132-137: The milestone logic in the download progress handler
(variables percent, nextLoggedPercent, downloaded, total, function logStep) can
skip intermediate thresholds when percent jumps past multiple milestones; change
the check to a loop that while percent >= nextLoggedPercent (or percent === 100)
emits logStep for each milestone and increments nextLoggedPercent by 25 each
iteration (ensuring the final 100% is emitted with milestone 100), so every 25%
milestone is logged even if a single data event advances progress across several
thresholds.
- Around line 113-118: The download function follows redirects recursively
without a cap; add a maxRedirects parameter (e.g., maxRedirects = 10) and a
currentRedirects counter to the download function signature, increment it on
each redirect and if it exceeds the limit reject with a clear error instead of
recursing forever; update the redirect branch that calls download(nextURL,
destination) to call download(nextURL, destination, currentRedirects + 1,
maxRedirects) (or pass a default when initiating), and ensure all caller sites
provide the new parameter or rely on the default.
---
Outside diff comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 283-304: The non-interactive path always outputs "install-server"
because currentTargetBrokenOutput is called without the actual installed state;
update the flow so the result of isServerInstalled() (serverInstalled) is used
to determine the JSON/non-TTY output—either by passing serverInstalled into
currentTargetBrokenOutput or by adding logic in the non-interactive branch to
emit "use-installed-server" when serverInstalled is true (identify the
isServerInstalled() call and the currentTargetBrokenOutput(...) invocation to
modify). Ensure the JSON/TTY output mirrors the interactive prompt choice text
for option 3 instead of always suggesting installation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33f5d751-b2d8-4103-bce8-43f1439e85eb
📒 Files selected for processing (5)
packages/cli/README.mdpackages/cli/src/commands/setup.tspackages/cli/src/commands/watch.tspackages/cli/test/cli-smoke.tspackages/npm/scripts/build.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (macos-latest / bun 1.3.10)
🧰 Additional context used
🪛 ESLint
packages/cli/src/commands/setup.ts
[error] 416-416: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 641-641: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 656-656: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 672-672: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
🔇 Additional comments (4)
packages/cli/src/commands/watch.ts (1)
17-18: LGTM!packages/cli/README.md (1)
2418-2419: LGTM!packages/cli/src/commands/setup.ts (1)
106-107: LGTM!Also applies to: 254-275, 347-351, 406-435, 513-530, 626-690
packages/cli/test/cli-smoke.ts (1)
3-3: LGTM!Also applies to: 238-262
Comment on lines
113
to
118
| if ([301, 302, 303, 307, 308].includes(response.statusCode ?? 0) && response.headers.location) { | ||
| response.resume() | ||
| download(response.headers.location, destination).then(resolve, reject) | ||
| const nextURL = new URL(response.headers.location, url).toString() | ||
| logStep(\`redirecting to \${new URL(nextURL).host}\`) | ||
| download(nextURL, destination).then(resolve, reject) | ||
| return |
There was a problem hiding this comment.
Bound redirect depth to avoid infinite redirect loops.
download() follows redirects recursively with no maximum hop count. A bad redirect chain can hang installation indefinitely.
Suggested fix
-async function download(url, destination) {
+async function download(url, destination, redirects = 0) {
+ if (redirects > 10) {
+ throw new Error(\`Too many redirects while downloading \${artifact.file}\`)
+ }
logStep(\`downloading \${artifact.file}\`)
await new Promise((resolve, reject) => {
get(url, response => {
if ([301, 302, 303, 307, 308].includes(response.statusCode ?? 0) && response.headers.location) {
response.resume()
const nextURL = new URL(response.headers.location, url).toString()
logStep(\`redirecting to \${new URL(nextURL).host}\`)
- download(nextURL, destination).then(resolve, reject)
+ download(nextURL, destination, redirects + 1).then(resolve, reject)
return
}🤖 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 `@packages/npm/scripts/build.ts` around lines 113 - 118, The download function
follows redirects recursively without a cap; add a maxRedirects parameter (e.g.,
maxRedirects = 10) and a currentRedirects counter to the download function
signature, increment it on each redirect and if it exceeds the limit reject with
a clear error instead of recursing forever; update the redirect branch that
calls download(nextURL, destination) to call download(nextURL, destination,
currentRedirects + 1, maxRedirects) (or pass a default when initiating), and
ensure all caller sites provide the new parameter or rely on the default.
Comment on lines
+132
to
+137
| const percent = Math.floor(downloaded / total * 100) | ||
| if (percent >= nextLoggedPercent || percent === 100) { | ||
| const milestone = percent === 100 ? 100 : nextLoggedPercent | ||
| logStep(\`downloaded \${milestone}%\`) | ||
| nextLoggedPercent = milestone + 25 | ||
| } |
There was a problem hiding this comment.
Progress milestone logic can skip 50%/75% updates.
With large chunks, this logs only one threshold per data event, so intermediate milestones may never be emitted.
Suggested fix
- if (percent >= nextLoggedPercent || percent === 100) {
- const milestone = percent === 100 ? 100 : nextLoggedPercent
- logStep(\`downloaded \${milestone}%\`)
- nextLoggedPercent = milestone + 25
- }
+ while (percent >= nextLoggedPercent && nextLoggedPercent <= 100) {
+ logStep(\`downloaded \${nextLoggedPercent}%\`)
+ nextLoggedPercent += 25
+ }📝 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.
Suggested change
| const percent = Math.floor(downloaded / total * 100) | |
| if (percent >= nextLoggedPercent || percent === 100) { | |
| const milestone = percent === 100 ? 100 : nextLoggedPercent | |
| logStep(\`downloaded \${milestone}%\`) | |
| nextLoggedPercent = milestone + 25 | |
| } | |
| const percent = Math.floor(downloaded / total * 100) | |
| while (percent >= nextLoggedPercent && nextLoggedPercent <= 100) { | |
| logStep(`downloaded ${nextLoggedPercent}%`) | |
| nextLoggedPercent += 25 | |
| } |
🤖 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 `@packages/npm/scripts/build.ts` around lines 132 - 137, The milestone logic in
the download progress handler (variables percent, nextLoggedPercent, downloaded,
total, function logStep) can skip intermediate thresholds when percent jumps
past multiple milestones; change the check to a loop that while percent >=
nextLoggedPercent (or percent === 100) emits logStep for each milestone and
increments nextLoggedPercent by 25 each iteration (ensuring the final 100% is
emitted with milestone 100), so every 25% milestone is logged even if a single
data event advances progress across several thresholds.
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.
No description provided.