fix: code quality and safety improvements - #3358
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe PR simplifies two error-handling paths. ChangesError handling simplification
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.5)examples/cli-wrapper-live/src/index.tsxFile contains syntax errors that prevent linting: Line 67: Expected a statement but instead found '.catch(err => console.error(err))'.; Line 67: expected packages/jsx/src/lazy.tsFile contains syntax errors that prevent linting: Line 27: Expected an expression but instead found '.'.; Line 27: expected 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: 4
🤖 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 `@examples/cli-wrapper-live/src/index.tsx`:
- Line 67: Restore the successful-resolution handler for proc.exited so it
performs the final status update and flushes the buffered trailing line, while
keeping console.error(err) as the rejection branch. In the LiveWrapper
component, restore the unmount cleanup that calls proc.kill(), return the
LiveWrapper JSX, and restore the renderApp(LiveWrapper) entry point.
- Line 67: Complete the proc.exited promise chain by restoring the callback body
and closing the proc.exited.then((code) => { block with }); before the existing
catch handler, ensuring the file has balanced braces and compiles.
In `@packages/jsx/src/lazy.ts`:
- Line 27: Fix the promise chain in the lazy loader expression around the
existing .catch handler so it has a valid loader-promise expression before
member access. Restore the required enclosing syntax and preserve the current
console.error(err) rejection handling.
- Line 27: Update the lazy loader rejection handling in the promise chain around
the catch callback so it stores the rejection as the lazy component’s rejected
state, calls triggerRender(), and delivers the stored loader error by throwing
it during the next render; do not handle the rejection with console.error alone.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bceb2d6a-4fa4-42a5-9cfe-aa2dcf90aa60
📒 Files selected for processing (2)
examples/cli-wrapper-live/src/index.tsxpackages/jsx/src/lazy.ts
| } | ||
|
|
||
| renderApp(LiveWrapper); | ||
| .catch(err => console.error(err)) No newline at end of file |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Restore successful-exit handling and lifecycle cleanup.
A rejection handler does not run when proc.exited resolves normally. The current change therefore drops the final status update and buffered trailing line. It also removes proc.kill() on unmount, the LiveWrapper JSX return, and renderApp(LiveWrapper), so the example cannot provide its previous UI behavior and can leave the subprocess running. (github.com)
Keep the success handler, restore the effect cleanup and component return, and retain error logging as the rejection branch.
🧰 Tools
🪛 Biome (2.5.5)
[error] 67-67: Expected a statement but instead found '.catch(err => console.error(err))'.
(parse)
[error] 67-67: expected } but instead the file ends
(parse)
🤖 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 `@examples/cli-wrapper-live/src/index.tsx` at line 67, Restore the
successful-resolution handler for proc.exited so it performs the final status
update and flushes the buffered trailing line, while keeping console.error(err)
as the rejection branch. In the LiveWrapper component, restore the unmount
cleanup that calls proc.kill(), return the LiveWrapper JSX, and restore the
renderApp(LiveWrapper) entry point.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Blocker: complete the proc.exited promise chain.
Line [67] starts .catch(...) after an unclosed proc.exited.then((code) => {. The file has an unmatched { and cannot compile. The patch also leaves the callback body removed. (github.com)
Restore the callback and append .catch(...) after });, or remove the then opener entirely for rejection-only handling.
Proposed syntax fix
- proc.exited.then((code) => {
- .catch(err => console.error(err))
+ proc.exited.catch(err => console.error(err));🧰 Tools
🪛 Biome (2.5.5)
[error] 67-67: Expected a statement but instead found '.catch(err => console.error(err))'.
(parse)
[error] 67-67: expected } but instead the file ends
(parse)
🤖 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 `@examples/cli-wrapper-live/src/index.tsx` at line 67, Complete the proc.exited
promise chain by restoring the callback body and closing the
proc.exited.then((code) => { block with }); before the existing catch handler,
ensuring the file has balanced braces and compiles.
Source: Linters/SAST tools
|
|
||
| return LazyComponent; | ||
| } No newline at end of file | ||
| .catch(err => console.error(err)) No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore a valid promise expression before .catch.
Line 27 starts with member access and has no left-hand expression. This causes the reported parse errors and prevents the package from compiling. Attach .catch(...) to the loader promise expression and restore the required enclosing syntax.
🧰 Tools
🪛 Biome (2.5.5)
[error] 27-27: Expected an expression but instead found '.'.
(parse)
[error] 27-27: expected } but instead the file ends
(parse)
🪛 GitHub Actions: CI / 0_build-and-test.txt
[error] 27-27: tsup/esbuild build failed: Unexpected "." at the .catch(err => console.error(err)) expression. The @termuijs/jsx#build command exited with code 1.
🪛 GitHub Actions: CI / build-and-test
[error] 27-27: tsup/esbuild build failed: Unexpected "." at the .catch(err => console.error(err)) expression. The @termuijs/jsx build command exited with code 1.
🤖 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/jsx/src/lazy.ts` at line 27, Fix the promise chain in the lazy
loader expression around the existing .catch handler so it has a valid
loader-promise expression before member access. Restore the required enclosing
syntax and preserve the current console.error(err) rejection handling.
Source: Linters/SAST tools
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not replace rejected lazy state with logging only.
packages/jsx/src/lazy.test.ts:73-109 requires a rejected loader to schedule a render and throw the loader error on the next render. Logging the rejection leaves the lazy component without a rejected state or an error delivery path. Retain the rejection state, call triggerRender(), and throw the stored error during the next render.
🧰 Tools
🪛 Biome (2.5.5)
[error] 27-27: Expected an expression but instead found '.'.
(parse)
[error] 27-27: expected } but instead the file ends
(parse)
🪛 GitHub Actions: CI / 0_build-and-test.txt
[error] 27-27: tsup/esbuild build failed: Unexpected "." at the .catch(err => console.error(err)) expression. The @termuijs/jsx#build command exited with code 1.
🪛 GitHub Actions: CI / build-and-test
[error] 27-27: tsup/esbuild build failed: Unexpected "." at the .catch(err => console.error(err)) expression. The @termuijs/jsx build command exited with code 1.
🤖 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/jsx/src/lazy.ts` at line 27, Update the lazy loader rejection
handling in the promise chain around the catch callback so it stores the
rejection as the lazy component’s rejected state, calls triggerRender(), and
delivers the stored loader error by throwing it during the next render; do not
handle the rejection with console.error alone.
1 participant
Summary by CodeRabbit