fix: address race-condition and error-handling bugs from automated audit#646
Conversation
📝 WalkthroughWalkthroughThis PR systematically reduces public API surface by making internal helper functions private across the app and ui packages, while strengthening error handling to gracefully manage promise rejections and parsing failures throughout the codebase. ChangesAPI Encapsulation and Error Handling Robustness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/dialog-manage-models.tsx, packages/app/src/components/terminal.tsx, packages/app/src/components/titlebar-history.ts, packages/app/src/context/file/path.ts, packages/app/src/context/global-sync.tsx, packages/app/src/context/highlights.tsx, packages/app/src/context/layout.tsx, packages/app/src/custom-elements.d.ts, packages/app/src/pages/layout.tsx, packages/app/src/pages/layout/sidebar-workspace.tsx, packages/app/src/pages/session/helpers.ts, packages/app/src/utils/agent.ts, packages/app/src/utils/solid-dnd.tsx, packages/app/src/utils/sound.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request focuses on code cleanup and improved error handling across several packages. Key changes include removing unused components and functions, such as DialogManageModels and WorkspaceDragOverlay, and converting various exported functions to internal ones to improve encapsulation. Additionally, the PR introduces catch blocks to handle potential errors in font loading, dynamic imports, and SSE stream parsing. A critical issue was identified in packages/ui/src/components/tool-info.ts, where duplicate function definitions were added, which will lead to a compilation error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ui/src/components/tool-info.ts`:
- Around line 34-50: Remove the duplicate declarations of
enterWorktreeOwnerProject and enterWorktreeTarget (the later copies) so each
function is defined only once, and convert the original exported implementations
of enterWorktreeOwnerProject and enterWorktreeTarget into non-exported helper
functions by removing the export keyword from their earlier definitions; ensure
references use the single remaining definitions to avoid TypeScript duplicate
identifier errors.
🪄 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 Plus
Run ID: 4078b2c1-459f-4840-b89a-02c1b5998844
📒 Files selected for processing (22)
packages/app/src/components/dialog-manage-models.tsxpackages/app/src/components/terminal.tsxpackages/app/src/components/titlebar-history.tspackages/app/src/context/file/path.tspackages/app/src/context/global-sync.tsxpackages/app/src/context/highlights.tsxpackages/app/src/context/layout.tsxpackages/app/src/custom-elements.d.tspackages/app/src/pages/layout.tsxpackages/app/src/pages/layout/sidebar-workspace.tsxpackages/app/src/pages/session/helpers.tspackages/app/src/utils/agent.tspackages/app/src/utils/solid-dnd.tsxpackages/app/src/utils/sound.tspackages/opencode/src/control-plane/workspace.tspackages/opencode/src/provider/transform.tspackages/opencode/src/tool/agent.tspackages/opencode/src/tool/apply_patch.tspackages/ui/src/components/apply-patch-file.tspackages/ui/src/components/message-part/session-link.tspackages/ui/src/components/tool-info.tspackages/ui/src/storybook/fixtures.ts
💤 Files with no reviewable changes (7)
- packages/app/src/custom-elements.d.ts
- packages/app/src/components/dialog-manage-models.tsx
- packages/app/src/utils/solid-dnd.tsx
- packages/app/src/pages/session/helpers.ts
- packages/app/src/pages/layout/sidebar-workspace.tsx
- packages/ui/src/storybook/fixtures.ts
- packages/ui/src/components/message-part/session-link.ts
Fixes identified during full-stack bug hunt: - terminal.tsx: === instead of == for Enter key check - terminal.tsx: add .catch() to document.fonts.ready promise - layout.tsx: add .catch() to 3 dynamic imports (provider, server, directory dialogs) - global-sync.tsx: add .catch() to projectInit promise - tool/agent.ts: add default case to part.status switch (prevents undefined header/body) - workspace.ts: wrap parseSSE call in try/catch (prevents malformed events from killing sync loop) - apply_patch.ts: replace (error as any) with proper Record<string, unknown> narrowing Refs Astro-Han#643 Astro-Han#644 Astro-Han#645
f8822ce to
fd51ecb
Compare
|
Force-pushed a rescue rebase on top of current
Net diff is now +71/-37 across 7 files. Local typecheck passes on |
|
Merged as |
Fixes identified during full-stack bug hunt (issues #643, #644, #645).
app
===) for Enter key check instead of loose (==).catch()todocument.fonts.readypromise to prevent unhandled rejection.catch()handlers to 3 dynamic imports (provider/server/directory dialogs) — prevents unhandled rejections if chunks fail to load.catch()toprojectInitpromise — prevents unhandled rejection if project initialization failsrequestAnimationFramecallback withif (!active) returnto avoid scheduling work after cleanupclearOptimistic(directory, sessionID)when dropping sessions, so the optimistic Map does not leak entriesopencode
defaultcase topart.statusswitch — preventsheaderandbodyfrom remainingundefinedwhen a new status value is introducedparseSSEcall in try/catch — prevents malformed SSE events from terminating the sync loop entirely(error as any)with properRecord<string, unknown>narrowing innotFound()helperVerification