Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical configuration and dependency issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR centralizes TypeScript and ESLint configuration, adds Playwright typechecking, updates typings, and applies lint fixes.
Changes:
- Pins React type resolution and aligns example-web tooling with the root configuration.
- Adds Playwright TypeScript configuration and Node typings.
- Replaces deprecated React ref types and updates dependency resolutions.
File summaries
| File | Summary |
|---|---|
yarn.lock |
Dependency updates; critical Tiptap peer-version mismatch remains (1 vote). |
tsconfig.json |
Pins React types and updates exclusions. |
src/web/utils/pasteImages.ts |
Formatting update. |
src/web/nativeMappers/returnKeyTypeToEnterKeyHint.ts |
Formatting update. |
src/spec/EnrichedTextInputNativeComponent.ts |
Replaces deprecated React.ElementRef. |
package.json |
Adds Playwright typechecking and Node typings; critical .playwright may still be included by typecheck:lib (1 vote). |
apps/example/src/components/ToolbarButton.tsx |
Formatting update. |
apps/example-web/tsconfig.app.json |
Pins React typings; moderate Turbo inputs do not include this configuration (1 vote). |
apps/example-web/package.json |
Removes local tooling dependencies; critical scripts may no longer resolve required binaries (1 vote). |
apps/example-web/eslint.config.mjs |
Removes duplicate ESLint configuration. |
.playwright/tsconfig.json |
Adds Playwright TypeScript configuration. |
.playwright/tests/headingBoldFromStyle.spec.ts |
Moderate empty heading captures may be treated as missing (2 votes). |
.playwright/tests/codeblockInlineStyles.spec.ts |
Updates regex capture handling. |
Review details
Suppressed comments (1)
apps/example-web/tsconfig.app.json:12
- This PR changes
tsconfig.app.json, but thebuild:webTurbo inputs only includetsconfig.json(turbo.json:9). As a result, a cached web build can be reused without hashing these new path mappings, so CI/local Turbo runs may not rebuild after this configuration changes. Addtsconfig.app.json(or a matchingtsconfig*.jsoninput) to the task inputs.
"react": ["../../node_modules/@types/react"],
"react/*": ["../../node_modules/@types/react/*"],
"react-dom": ["../../node_modules/@types/react-dom"],
"react-dom/*": ["../../node_modules/@types/react-dom/*"],
},
- Files reviewed: 11/13 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Address the web build typechecking gap and exclude .playwright from the root TypeScript project.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/14 changed files
- Comments generated: 1
- Review effort level: Lite
| "build:web": "tsc -b && vite build", | ||
| "preview": "vite preview", | ||
| "lint": "eslint . --ext ts,tsx" | ||
| "build:web": "vite build", |
There was a problem hiding this comment.
I believe we can safely remove that tsc call. It was there, because we had a standalone TS, but now we use the root one and we use the root yarn typecheck
Summary
I've done some changes to TypeScript and ESlint related changes to fix already existing errors in the codebase. These errors were not caught by the project's scripts like
yarn lintoryarn typecheck, but were visible in the IDE. This is probably because of the differences hownpmand IDE runs TS. I've verified that these errors were present in two different IDEs - VSCode and Zed on different machines, so it's a shared issue and worth fixing even if they don't persist into CI.tsconfig.jsontypes resolution paths, as we hadreactinstalled in both root andapps/examplenode_modules, and TS would get confused resulting in errors, e.g. insrc/native/EnrichedTextInput.tsx.A similar issue was already mentioned in
apps/example/tsconfig.json:With the RN 0.87 bump, we also bumped root TypeScript to
6.x, and inexample-webwe still use TS5.x. I believe it's better to have theexample-webuse the root TS, just like theapps/examplealready does, and have a single TS across the repo, whenever possibleexample-webhad its own separate ESLint and the its code wasn't linted to those rules. There were two options: tweak theexample-webcode so it respects the configured standalone ESLint, or remove that ESLint and let the root ESLint handle theexample-web. I opt for the latter one, as we keep linting rules consistent across the repo, we don't have to maintain another ESLint configuration, it doesn't require modifying the example app's code, and we've been using the root ESLint inexample-webviayarn lintanywayIn
.playwright, the TS wasn't setup correctly and we got errors e.g. inimages.spec.ts, we got:I added the
@types/nodespackage and wired up.playwrightwith TSLinted the code where it wasn't.
Removed deprecated
React.ElementRefin favor ofReact.ComponentRefinsrc/spec/EnrichedTextInputNativeComponent.tsTest Plan
What I've verified:
yarn testyarn test:e2e:webyarn test:e2e:mobile- the tests build and run, but I haven't run them all throughyarn iosyarn androidyarn lintyarn typecheckYou can also run these scripts - everything should pass
Checklist