Skip to content

Handle unavailable TypeScript checks#3845

Merged
wwwillchen merged 5 commits into
mainfrom
update-agent-docs-20260708174741
Jul 9, 2026
Merged

Handle unavailable TypeScript checks#3845
wwwillchen merged 5 commits into
mainfrom
update-agent-docs-20260708174741

Conversation

@keppo-bot

@keppo-bot keppo-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • fix: handle unavailable TypeScript checks
  • Primary files: .claude/skills/pr-push/SKILL.md, .claude/skills/pr-push/scripts/pr_push.sh, .github/workflows/release.yml, e2e-tests/cloud_sandbox.spec.ts, e2e-tests/database_url_guide.spec.ts, e2e-tests/dynamic_models.spec.ts

Review in cubic

@keppo-bot keppo-bot Bot requested a review from a team July 9, 2026 00:47
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@dyad-assistant

dyad-assistant Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

This PR adds structured error handling for TypeScript unavailability across the full stack: worker thread, main process, IPC handlers, agent tool, and renderer UI. The implementation is well-architected with clean separation between worker-side TscWorkerPreconditionError, main-process TypeCheckPreconditionError, and context-aware guidance messages (agent vs user-facing). Error classification uses DyadError with DyadErrorKind.Precondition correctly, ensuring these expected failures are filtered from PostHog telemetry. Tests are comprehensive, covering the tsc processor, the agent tool's three precondition paths, and the new dyad-command markdown rendering.

Issues Summary

Severity File Issue
🟑 MEDIUM src/ipc/types/proposals.ts:54 SuggestedActionSchema Zod union missing AddTypeScriptAction
🟑 MEDIUM src/components/preview_panel/Problems.tsx:267 Error state reuses empty-state heading i18n key
🟒 Low Priority Notes (4 items)
  • Dual error class hierarchy is justified - TscWorkerPreconditionError (worker thread) and TypeCheckPreconditionError (main process) look duplicative but are necessary since worker threads can't share class instances across the serialization boundary. The errorKind field on WorkerOutput bridges the gap cleanly. (workers/tsc/tsc_worker.ts, src/ipc/processors/tsc.ts)
  • String-matching fallback is a good safety net - getStringMatchedTypeCheckPreconditionKind provides backward compatibility for errors that don't carry the structured errorKind field (e.g., older code paths or unexpected error wrapping). (src/ipc/processors/tsc.ts)
  • Hardcoded English prompt in AddTypeScriptButton is correct - The prompt goes to the AI agent, not the user-facing UI, so it should remain in English regardless of locale. (src/components/chat/ChatInput.tsx)
  • appPath hoisting in problems_handlers is safe - If the error occurs before getDyadAppPath runs (e.g., DB query failure), appPath remains "". But DB/NotFound errors won't match the TypeScript precondition patterns, so the preconditionKind check will be undefined and the new code path won't execute. (src/ipc/handlers/problems_handlers.ts)

Generated by Dyadbot persona-based code review

@github-actions github-actions Bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Jul 9, 2026
@keppo-bot keppo-bot Bot removed the needs-human:review-issue ai agent flagged an issue that requires human review label Jul 9, 2026
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f028ddfcc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/pro/main/ipc/handlers/local_agent/tools/run_type_checks.ts

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 2 inline finding(s).

Comment thread src/ipc/handlers/problems_handlers.ts
Comment thread src/ipc/processors/tsc.ts
@dyad-assistant

dyad-assistant Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

This PR cleanly handles the case where TypeScript type checking is unavailable. The structured error propagation from worker -> main process -> UI is well-layered, IPC boundary concerns are correctly handled (the TypeCheckPreconditionError class identity is only checked on the main process side; the renderer receives pre-formatted guidance via error.message), and cache invalidation on the error path follows established patterns. Test coverage for the three precondition scenarios is solid.

Issues Summary

Severity File Issue
🟑 MEDIUM src/ipc/handlers/problems_handlers.ts:43 Hoisted appPath="" could pass empty string to guidance if future code adds a precondition throw before assignment
🟑 MEDIUM src/ipc/processors/tsc.ts:126 toProblemReportError silently drops errorKind for existing DyadError instances
🟒 Low Priority Notes (2 items)
  • Positive: Cache invalidation on error path - run_type_checks.ts correctly sends { problems: [] } to clear stale UI data when type checking is unavailable, and the new rule in rules/local-agent-tools.md documents this pattern for future tool authors. (src/pro/main/ipc/handlers/local_agent/tools/run_type_checks.ts)

  • Positive: AddTypeScriptButton prompt quality - The one-click button sends a prompt specifying allowJs: true and strict: false, which is the right approach for adding TS to an existing JS project without breaking builds. (src/components/chat/ChatInput.tsx)


Generated by Dyadbot persona-based code review

@github-actions github-actions Bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Jul 9, 2026
@wwwillchen wwwillchen removed the needs-human:review-issue ai agent flagged an issue that requires human review label Jul 9, 2026
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@wwwillchen

Copy link
Copy Markdown
Collaborator

πŸ€– Claude Code Review Summary

PR Confidence: 5/5

All trusted review threads were addressed with code changes, resolved explicitly, and the targeted tests plus fmt/lint/type-check passed.

Unresolved Threads

No unresolved threads

Resolved Threads

Issue Rationale Link
Type-check guidance referenced unavailable add_dependency tool Added a turn-scoped canInstallDependencies capability derived from shouldIncludeTool(addDependencyTool, ...), and used it to show install-tool guidance only when the tool is actually available. View
Problem handler could pass empty appPath to precondition guidance after future edits Added a defensive if (!appPath) { throw error; } guard before path-based guidance generation. View
toProblemReportError preserved DyadError without explaining ignored errorKind Added a brief comment clarifying that already-classified DyadError instances intentionally keep their original kind. View
Product Principle Suggestions

No suggestions


πŸ€– Generated by Claude Code

- Gate type-check dependency guidance on add_dependency availability

- Guard problem precondition guidance before appPath is assigned

- Document preserved DyadError classification
@wwwillchen wwwillchen force-pushed the update-agent-docs-20260708174741 branch from d016dc3 to e616880 Compare July 9, 2026 02:47
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e61688041f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +1299 to +1303
streamMessage({
prompt:
"Add TypeScript to this project: install `typescript` as a dev dependency and create a lenient tsconfig (`allowJs: true`, `strict: false`) so existing JavaScript keeps working.",
chatId,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Route Add TypeScript clicks to a writable mode

When this command is rendered after run_type_checks runs in Ask/read-only or Plan mode, clicking it sends a normal message without requestedChatMode, so useStreamChat reuses the current chat's read-only/plan mode and handleLocalAgentStream still filters out state-changing tools like add_dependency/file writes. In those modes the one-click β€œAdd TypeScript” remediation cannot actually install TypeScript or create tsconfig, even though the guidance says the user can accept it with one click; force a writable mode or avoid showing this action when the current chat cannot modify the app.

Useful? React with πŸ‘Β / πŸ‘Ž.

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@dyad-assistant

dyad-assistant Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

This PR gracefully handles cases where TypeScript type checking is unavailable in user projects (no TypeScript installed, missing tsconfig, incomplete node_modules). The implementation is well-layered across the worker, main process, and renderer, with proper error classification, context-aware guidance messages, and comprehensive test coverage.

What was reviewed:

  • Worker-level error classification (workers/tsc/tsc_worker.ts): TscWorkerPreconditionError with structured errorKind correctly propagated through the serializable WorkerOutput.errorKind field across the worker boundary.
  • Main-process error handling (src/ipc/processors/tsc.ts): TypeCheckPreconditionError extending DyadError(Precondition) with the typeCheckKind discriminant. getTypeCheckPreconditionKind correctly prioritizes instanceof over string-matching fallback. toProblemReportError properly passes through the worker's structured errorKind.
  • Guidance generation (getTypeCheckPreconditionGuidance): Context-aware messages based on whether TypeScript is declared in package.json, with separate agent-directed and user-facing variants via includeAgentInstructions.
  • Agent tool handler (run_type_checks.ts): Catches DyadError(Precondition) errors, clears the renderer problems cache (following the rule about IPC cache coherency on error paths), shows a <dyad-output type="warning"> in the UI, and returns actionable guidance to the agent.
  • IPC handler (problems_handlers.ts): Re-wraps precondition errors with user-facing guidance text as a DyadError(Precondition), correctly serialized across IPC via serializeIpcError/deserializeIpcError.
  • Chat stream handler (chat_stream_handlers.ts): Downgrades known precondition errors to logger.info, reducing noise in error logs.
  • UI (Problems.tsx, ChatInput.tsx): Error state in Problems panel with AlertTriangle icon, recheck button, and user-friendly message. New AddTypeScriptButton action for the <dyad-command type="add-typescript"> flow.
  • Tests: New run_type_checks.spec.ts with 4 scenarios (TS declared but missing, plain JS project, missing tsconfig, unexpected error rethrow). Updated tsc.test.ts and DyadMarkdownParser.test.tsx with appropriate mocks.
  • IPC security boundary: No new filesystem or process access exposed. Error guidance reads package.json from the already-trusted appPath. All inputs validated through existing typed handler contracts.
  • i18n: addTypeScript key added to all 4 locales (en, es, pt-BR, zh-CN).

βœ… No issues found by persona-based review.

🟒 Low Priority Notes (3 items)
  • tsconfig-not-found guidance ignores includeAgentInstructions - The getTypeCheckPreconditionGuidance function returns identical text for tsconfig-not-found regardless of whether it's agent-directed or user-facing. The other error paths provide richer agent instructions (explicit <dyad-command> tags, "Do not call again" directives). This inconsistency is minor since "You can create a suitable tsconfig for this project and retry" is adequate for both audiences. (src/ipc/processors/tsc.ts)

  • packageJsonDeclaresTypeScript silently swallows all read/parse errors - If package.json is unreadable (permissions, corrupt JSON), the function returns false, leading to "project does not use TypeScript" guidance. This is a reasonable default but could mask filesystem issues. Consider logging at logger.warn() level for non-ENOENT errors. (src/ipc/processors/tsc.ts)

  • Hardcoded English guidance strings - The user-facing guidance messages from getTypeCheckPreconditionGuidance (shown in the Problems panel via IPC error) are hardcoded in English while the UI button label (addTypeScript) is i18n-translated. This is consistent with how other IPC error messages work in the codebase but worth noting for future i18n efforts. (src/ipc/processors/tsc.ts)


Generated by Dyadbot persona-based code review

@github-actions github-actions Bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Jul 9, 2026
@wwwillchen wwwillchen merged commit 6d32fe1 into main Jul 9, 2026
21 of 23 checks passed
@wwwillchen wwwillchen deleted the update-agent-docs-20260708174741 branch July 9, 2026 04:22
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🎭 Playwright Test Results

❌ Some tests failed

OS Passed Failed Flaky Skipped
🍎 macOS 386 1 5 119
πŸͺŸ Windows 381 3 0 119

Summary: 767 passed, 4 failed, 5 flaky, 238 skipped

Failed Tests

🍎 macOS

  • package_manager.spec.ts > custom pnpm install auto-denies ignored builds and recovers preview
    • Test timeout of 360000ms exceeded.

πŸͺŸ Windows

  • edit_code.spec.ts > edit code edits the right file during rapid switches
    • Error: expect(received).toEqual(expected) // deep equality
  • package_manager.spec.ts > custom pnpm install auto-denies ignored builds and recovers preview
    • Test timeout of 360000ms exceeded.
  • template-create-nextjs.spec.ts > create next.js app
    • Error: expect(locator).toBeEnabled() failed

πŸ“‹ Re-run Failing Tests (macOS)

Copy and paste to re-run all failing spec files locally:

npm run e2e \
  e2e-tests/package_manager.spec.ts

⚠️ Flaky Tests

🍎 macOS

  • engine.spec.ts > send message to engine (passed after 1 retry)
  • fix_error.spec.ts > copy error message from banner (passed after 1 retry)
  • smart_context_balanced.spec.ts > smart context balanced - simple (passed after 1 retry)
  • smart_context_deep.spec.ts > smart context deep - read write read (passed after 1 retry)
  • visual_editing.spec.ts > swap image via URL (passed after 1 retry)

πŸ“Š View full report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human:review-issue ai agent flagged an issue that requires human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant