Conversation
📝 WalkthroughWalkthroughEnables TypeScript noUncheckedSideEffectImports across many tsconfigs, adds ChangesWorkspace Modernization and Native TypeScript Tooling
TypeScript Strictness Enforcement and Markdown Lifecycle
Minor Dependency and Configuration Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
coryrylan
commented
May 15, 2026
| this.#pendingMarkdownParsing = Promise.resolve(); | ||
| this.renderedHtml = ''; | ||
| } | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
This improves the getUpdateComplete check so tests can reliably use it for render completion instead of a flakey timeout
coryrylan
commented
May 15, 2026
| import { dirname, resolve } from 'node:path'; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
| const tsgoPath = resolve(dirname(require.resolve('@typescript/native-preview/package.json')), 'bin/tsgo.js'); |
Collaborator
Author
There was a problem hiding this comment.
Some projects (mostly starters) rely on TS6. This allows our libraries/projects to be type checked with TS7 and get the benefits while preserving ts6 until those projects can be upgrades.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/markdown/src/markdown/markdown.ts (1)
134-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale parse completions from overwriting newer/cleared content.
On Line 135 and Line 145, older in-flight parses are not invalidated. If
#renderMarkdown()is called, then#clearRenderedHtml()(or another#renderMarkdown()) runs before the first parse resolves, the old.then()can still setthis.renderedHtmlwith stale HTML.Suggested fix
export class Markdown extends LitElement { + `#renderRequestId` = 0; + `#renderMarkdown`(markdown: string): void { + const requestId = ++this.#renderRequestId; this.#pendingMarkdownParsing = this.#parseMarkdown(markdown) .then(parsed => { + if (requestId !== this.#renderRequestId) return; this.renderedHtml = parsed; }) .catch(_error => { + if (requestId !== this.#renderRequestId) return; // Don't update renderedHtml on parsing error - keep previous content console.debug('Markdown parsing failed, keeping previous content'); }); } `#clearRenderedHtml`(): void { + ++this.#renderRequestId; this.#pendingMarkdownParsing = Promise.resolve(); this.renderedHtml = ''; } }🤖 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 `@projects/markdown/src/markdown/markdown.ts` around lines 134 - 148, Older in-flight parses started by `#renderMarkdown` can still set this.renderedHtml when they resolve; add an invalidation token (e.g., a numeric parseId or symbol stored on the instance) that `#renderMarkdown` captures locally before calling `#parseMarkdown` and that `#clearRenderedHtml` increments/changes to invalidate pending results, then in the .then() handler check the captured token against the current instance token and only assign this.renderedHtml if they match; also ensure `#clearRenderedHtml` updates `#pendingMarkdownParsing` to a resolved promise and increments the token so already-resolving promises are ignored.
🤖 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 `@projects/internals/vite/src/plugins/tsc.js`:
- Around line 5-6: The plugin currently resolves tsgo eagerly by computing
tsgoPath at module load (using createRequire +
resolve(dirname(require.resolve('@typescript/native-preview/package.json')),
'bin/tsgo.js')), which can fail before the VITE_INITIAL_BUILD guard; instead
move the resolution into the plugin's buildStart hook and resolve the
package.json lazily there: use createRequire(import.meta.url) inside buildStart
(or a lazily-invoked helper),
require.resolve('@typescript/native-preview/package.json') to load the
package.json, read its "bin" mapping to get the actual tsgo entry (e.g.,
pkg.bin.tsgo), and then compute the full path (resolve(dirname(pkgJsonPath),
pkg.bin.tsgo)) and assign to tsgoPath before using it; update any references to
the top-level tsgoPath so they use the lazily-resolved value.
---
Outside diff comments:
In `@projects/markdown/src/markdown/markdown.ts`:
- Around line 134-148: Older in-flight parses started by `#renderMarkdown` can
still set this.renderedHtml when they resolve; add an invalidation token (e.g.,
a numeric parseId or symbol stored on the instance) that `#renderMarkdown`
captures locally before calling `#parseMarkdown` and that `#clearRenderedHtml`
increments/changes to invalidate pending results, then in the .then() handler
check the captured token against the current instance token and only assign
this.renderedHtml if they match; also ensure `#clearRenderedHtml` updates
`#pendingMarkdownParsing` to a resolved promise and increments the token so
already-resolving promises are ignored.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: bda0207c-1848-4b97-af07-9baa10418353
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
pnpm-workspace.yamlprojects/cli/tsconfig.jsonprojects/code/tsconfig.jsonprojects/core/tsconfig.jsonprojects/create/tsconfig.jsonprojects/forms/tsconfig.jsonprojects/internals/metadata/tsconfig.jsonprojects/internals/patterns/tsconfig.jsonprojects/internals/testing/tsconfig.jsonprojects/internals/tools/tsconfig.jsonprojects/internals/vite/package.jsonprojects/internals/vite/src/plugins/tsc.jsprojects/lint/tsconfig.jsonprojects/markdown/src/markdown/markdown.test.tsprojects/markdown/src/markdown/markdown.tsprojects/markdown/tsconfig.jsonprojects/media/tsconfig.jsonprojects/monaco/build/tsconfig.jsonprojects/monaco/tsconfig.jsonprojects/pages/index.jsprojects/site/tsconfig.jsonprojects/starters/lit-library/tsconfig.jsonprojects/starters/mpa/tsconfig.jsonprojects/starters/solidjs/package.jsonprojects/starters/svelte/package.jsonprojects/starters/typescript/tsconfig.jsonprojects/starters/vue/package.jsonprojects/styles/tsconfig.jsonprojects/themes/package.jsonprojects/themes/tsconfig.json
💤 Files with no reviewable changes (2)
- projects/markdown/src/markdown/markdown.test.ts
- projects/starters/solidjs/package.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/markdown/src/markdown/markdown.ts (1)
134-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale async parse results from overwriting newer content.
Line 135 stores a new pending promise, but prior in-flight parses can still resolve later and write
renderedHtml, including after Line 146 clears output. This can resurrect stale markdown.💡 Suggested fix (version guard)
export class Markdown extends LitElement { + `#renderVersion` = 0; `#pendingMarkdownParsing`: Promise<void> = Promise.resolve(); @@ `#renderMarkdown`(markdown: string): void { - this.#pendingMarkdownParsing = this.#parseMarkdown(markdown) + const renderVersion = ++this.#renderVersion; + this.#pendingMarkdownParsing = this.#parseMarkdown(markdown) .then(parsed => { - this.renderedHtml = parsed; + if (renderVersion !== this.#renderVersion) return; + this.renderedHtml = parsed; }) .catch(_error => { + if (renderVersion !== this.#renderVersion) return; // Don't update renderedHtml on parsing error - keep previous content console.debug('Markdown parsing failed, keeping previous content'); }); } @@ `#clearRenderedHtml`(): void { + this.#renderVersion++; this.#pendingMarkdownParsing = Promise.resolve(); this.renderedHtml = ''; }🤖 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 `@projects/markdown/src/markdown/markdown.ts` around lines 134 - 147, The async parse in `#renderMarkdown` can be overwritten by earlier in-flight parses; add a version guard using a monotonic counter or token (e.g., this.#markdownParseCounter) captured into a local parseId before calling this.#parseMarkdown, then when the promise resolves or rejects compare the captured parseId to the current counter and only write to renderedHtml if they match; also increment or invalidate the counter in `#clearRenderedHtml` so clearing prevents any earlier resolves from resurrecting stale content. Ensure references: `#renderMarkdown`, this.#pendingMarkdownParsing, this.#parseMarkdown, renderedHtml, and `#clearRenderedHtml`.
🤖 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.
Outside diff comments:
In `@projects/markdown/src/markdown/markdown.ts`:
- Around line 134-147: The async parse in `#renderMarkdown` can be overwritten by
earlier in-flight parses; add a version guard using a monotonic counter or token
(e.g., this.#markdownParseCounter) captured into a local parseId before calling
this.#parseMarkdown, then when the promise resolves or rejects compare the
captured parseId to the current counter and only write to renderedHtml if they
match; also increment or invalidate the counter in `#clearRenderedHtml` so
clearing prevents any earlier resolves from resurrecting stale content. Ensure
references: `#renderMarkdown`, this.#pendingMarkdownParsing, this.#parseMarkdown,
renderedHtml, and `#clearRenderedHtml`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 165cc2ce-ae57-413d-a781-bc6657e4eaf8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
pnpm-workspace.yamlprojects/cli/tsconfig.jsonprojects/code/tsconfig.jsonprojects/core/tsconfig.jsonprojects/create/tsconfig.jsonprojects/forms/tsconfig.jsonprojects/internals/metadata/tsconfig.jsonprojects/internals/patterns/tsconfig.jsonprojects/internals/testing/tsconfig.jsonprojects/internals/tools/tsconfig.jsonprojects/internals/vite/package.jsonprojects/internals/vite/src/plugins/tsc.jsprojects/lint/tsconfig.jsonprojects/markdown/src/markdown/markdown.test.tsprojects/markdown/src/markdown/markdown.tsprojects/markdown/tsconfig.jsonprojects/media/tsconfig.jsonprojects/monaco/build/tsconfig.jsonprojects/monaco/tsconfig.jsonprojects/pages/index.jsprojects/site/tsconfig.jsonprojects/starters/lit-library/tsconfig.jsonprojects/starters/mpa/tsconfig.jsonprojects/starters/solidjs/package.jsonprojects/starters/svelte/package.jsonprojects/starters/typescript/tsconfig.jsonprojects/starters/vue/package.jsonprojects/styles/tsconfig.jsonprojects/themes/package.jsonprojects/themes/tsconfig.json
💤 Files with no reviewable changes (2)
- projects/starters/solidjs/package.json
- projects/markdown/src/markdown/markdown.test.ts
coryrylan
commented
May 18, 2026
| @@ -1,5 +1,6 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "noUncheckedSideEffectImports": true, | |||
Collaborator
Author
There was a problem hiding this comment.
noUncheckedSideEffectImports forces the compiler to verify that side-effect-only imports (import "./style.css") point to an actual existing file
johnyanarella
approved these changes
May 18, 2026
Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
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)
projects/markdown/src/markdown/markdown.ts (1)
134-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale parse completions from overwriting newer/cleared content.
#clearRenderedHtml()(Line 145) clears state but does not invalidate an already in-flight parse started by#renderMarkdown(). That older promise can still resolve later and setrenderedHtmlto stale HTML after content was cleared or replaced.Suggested fix
export class Markdown extends LitElement { + `#renderVersion` = 0; + `#renderMarkdown`(markdown: string): void { - this.#pendingMarkdownParsing = this.#parseMarkdown(markdown) + const renderVersion = ++this.#renderVersion; + this.#pendingMarkdownParsing = this.#parseMarkdown(markdown) .then(parsed => { + if (renderVersion !== this.#renderVersion) return; this.renderedHtml = parsed; }) .catch(_error => { // Don't update renderedHtml on parsing error - keep previous content console.debug('Markdown parsing failed, keeping previous content'); }); } `#clearRenderedHtml`(): void { + this.#renderVersion++; this.#pendingMarkdownParsing = Promise.resolve(); this.renderedHtml = ''; } }🤖 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 `@projects/markdown/src/markdown/markdown.ts` around lines 134 - 148, The in-flight promise from `#renderMarkdown` can resolve after `#clearRenderedHtml` and overwrite renderedHtml; fix by introducing a generation/check token and validating it before applying a parsed result: when `#renderMarkdown` calls this.#parseMarkdown(markdown) capture a local generation id (increment a private counter on the instance) and only set this.renderedHtml if the captured id === current instance generation; update `#clearRenderedHtml` to increment the generation (invalidating any pending parses) and then clear renderedHtml and set `#pendingMarkdownParsing` to Promise.resolve(); reference symbols: `#renderMarkdown`, `#clearRenderedHtml`, `#pendingMarkdownParsing`, `#parseMarkdown`, renderedHtml.
♻️ Duplicate comments (1)
projects/internals/vite/src/plugins/tsc.js (1)
5-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve
tsgolazily from package metadata instead of hardcoding at import time.Line 6 resolves
bin/tsgo.jseagerly during module import. If package layout/bin mapping changes, this can fail before Line 16’s runtime guard and break unrelated commands.Proposed fix
const require = createRequire(import.meta.url); -const tsgoPath = resolve(dirname(require.resolve('`@typescript/native-preview/package.json`')), 'bin/tsgo.js'); +function resolveTsgoPath() { + const packageJsonPath = require.resolve('`@typescript/native-preview/package.json`'); + const packageJson = require(packageJsonPath); + const tsgoBin = typeof packageJson.bin === 'string' ? packageJson.bin : packageJson.bin?.tsgo; + if (!tsgoBin) { + throw new Error('Unable to resolve tsgo binary from `@typescript/native-preview` package metadata.'); + } + return resolve(dirname(packageJsonPath), tsgoBin); +} @@ async buildStart() { if (process.env.VITE_INITIAL_BUILD) { + const tsgoPath = resolveTsgoPath(); execFileSync( process.execPath, [ tsgoPath,For `@typescript/native-preview` version 7.0.0-dev.20260518.1, what is the package.json "bin" mapping for "tsgo", and is directly using "bin/tsgo.js" documented as stable?🤖 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 `@projects/internals/vite/src/plugins/tsc.js` around lines 5 - 6, The module currently resolves tsgoPath eagerly at import time using createRequire(import.meta.url) and require.resolve to point to 'bin/tsgo.js' (tsgoPath), which can throw before runtime guards run; change this to resolve the package metadata lazily at the point of use (e.g., inside the function or guarded block that needs tsgo) by calling createRequire(import.meta.url).resolve(...) or reading package.json only when you actually need tsgo, and fall back or handle errors if the "bin" mapping differs; update references to the tsgoPath constant so resolution happens inside the runtime guard rather than at module top-level.
🤖 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 `@projects/starters/svelte/package.json`:
- Line 21: The package.json lists invalid versions for Svelte tooling — update
the dependency entries for "`@sveltejs/vite-plugin-svelte`", "svelte", and
"svelte-check" to valid releases from the npm registry (e.g., replace the
non-existent "7.1.2", "5.55.7", "4.4.8" values with confirmed published versions
or caret ranges), verify by running npm/yarn install to ensure resolution, and
commit the corrected version strings so installation succeeds.
In `@projects/themes/package.json`:
- Line 50: The package.json lists an invalid cssnano version "7.1.7"; change the
cssnano dependency to a valid release (e.g., "7.1.4") in package.json, then
reinstall and regenerate lockfiles (npm install or yarn install) so
package-lock.json / yarn.lock is updated; update any build/test that asserts
dependency versions if present. Reference: dependency key "cssnano" in
package.json.
---
Outside diff comments:
In `@projects/markdown/src/markdown/markdown.ts`:
- Around line 134-148: The in-flight promise from `#renderMarkdown` can resolve
after `#clearRenderedHtml` and overwrite renderedHtml; fix by introducing a
generation/check token and validating it before applying a parsed result: when
`#renderMarkdown` calls this.#parseMarkdown(markdown) capture a local generation
id (increment a private counter on the instance) and only set this.renderedHtml
if the captured id === current instance generation; update `#clearRenderedHtml` to
increment the generation (invalidating any pending parses) and then clear
renderedHtml and set `#pendingMarkdownParsing` to Promise.resolve(); reference
symbols: `#renderMarkdown`, `#clearRenderedHtml`, `#pendingMarkdownParsing`,
`#parseMarkdown`, renderedHtml.
---
Duplicate comments:
In `@projects/internals/vite/src/plugins/tsc.js`:
- Around line 5-6: The module currently resolves tsgoPath eagerly at import time
using createRequire(import.meta.url) and require.resolve to point to
'bin/tsgo.js' (tsgoPath), which can throw before runtime guards run; change this
to resolve the package metadata lazily at the point of use (e.g., inside the
function or guarded block that needs tsgo) by calling
createRequire(import.meta.url).resolve(...) or reading package.json only when
you actually need tsgo, and fall back or handle errors if the "bin" mapping
differs; update references to the tsgoPath constant so resolution happens inside
the runtime guard rather than at module top-level.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: e7cc3b93-89a7-4290-bcea-6e9dc7b128e7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
pnpm-workspace.yamlprojects/cli/tsconfig.jsonprojects/code/tsconfig.jsonprojects/core/tsconfig.jsonprojects/create/tsconfig.jsonprojects/forms/tsconfig.jsonprojects/internals/metadata/tsconfig.jsonprojects/internals/patterns/tsconfig.jsonprojects/internals/testing/tsconfig.jsonprojects/internals/tools/package.jsonprojects/internals/tools/tsconfig.jsonprojects/internals/vite/package.jsonprojects/internals/vite/src/plugins/tsc.jsprojects/lint/tsconfig.jsonprojects/markdown/src/markdown/markdown.test.tsprojects/markdown/src/markdown/markdown.tsprojects/markdown/tsconfig.jsonprojects/media/tsconfig.jsonprojects/monaco/build/tsconfig.jsonprojects/monaco/tsconfig.jsonprojects/pages/index.jsprojects/site/src/_11ty/layouts/docs.11ty.jsprojects/site/src/_11ty/layouts/page.11ty.jsprojects/site/tsconfig.jsonprojects/starters/lit-library/tsconfig.jsonprojects/starters/mpa/tsconfig.jsonprojects/starters/nextjs/package.jsonprojects/starters/solidjs/package.jsonprojects/starters/svelte/package.jsonprojects/starters/typescript/tsconfig.jsonprojects/starters/vue/package.jsonprojects/styles/tsconfig.jsonprojects/themes/package.jsonprojects/themes/tsconfig.json
💤 Files with no reviewable changes (4)
- projects/markdown/src/markdown/markdown.test.ts
- projects/starters/solidjs/package.json
- projects/site/src/_11ty/layouts/page.11ty.js
- projects/site/src/_11ty/layouts/docs.11ty.js
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.11 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.1.2 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.2.0 🎉 |
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Chores