chore: fix merge conflict #6099
Conversation
Looks like there was a bad merge conflict in 5f3fca4
WalkthroughUpdates toolchain and configs: Node.js in .nvmrc, pnpm and devDependencies in package.json, and an overhauled tsconfig.json. Minor formatting changes in Angular/Vue examples and table-core utilities. verify-links script switches from fast-glob to tinyglobby and adjusts relative-link detection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Script as verify-links.ts
participant FS as File System
participant Parser as Markdown Link Extractor
rect rgb(245,245,255)
note over Script: Start
Dev->>Script: npm run verify-links
Script->>FS: glob("docs/**/*.md", { ignore: "**/node_modules/**" })
FS-->>Script: List of markdown files
Script->>Parser: Extract links per file
Parser-->>Script: Link list
Script->>Script: Filter links (incl. leading-slash)
Script->>FS: Resolve and check targets
Script-->>Dev: Report broken/valid links
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 31a508e
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
examples/vue/filters/src/Filter.vue (1)
27-27: Bug: using typeof on a computed ref instead of its value.This condition always evaluates as "object". It should inspect
firstValue.value.Apply:
- <div v-if="typeof firstValue === 'number'"> + <div v-if="typeof firstValue.value === 'number'">scripts/verify-links.ts (1)
41-45: Critical: resolving links relative to the file path (not its directory).
resolve(markdownFile, normalizedPath)treats the markdown filename as a directory segment, producing incorrect paths (false positives/negatives).Fix:
- let absPath = resolve(markdownFile, normalizedPath) + let absPath = resolve(path.dirname(markdownFile), normalizedPath)
🧹 Nitpick comments (6)
packages/table-core/src/utils/getSortedRowModel.ts (1)
21-23: Tiny type polish: make filter predicate explicitly boolean.Avoid
boolean | undefinedin the predicate for clearer intent.-const availableSorting = sortingState.filter(sort => - table.getColumn(sort.id)?.getCanSort() -) +const availableSorting = sortingState.filter(sort => + Boolean(table.getColumn(sort.id)?.getCanSort()) +)packages/table-core/src/filterFns.ts (1)
72-74: Formatting is fine; consider a micro‑opt.Avoid recomputing
row.getValueper element.- return filterValue.some(val => - row.getValue<unknown[]>(columnId)?.includes(val) - ) + const arr = row.getValue<unknown[]>(columnId) ?? [] + return filterValue.some(val => arr.includes(val))scripts/verify-links.ts (2)
56-57: Cross‑platform path check may fail on Windows.
absPath.includes('/examples/')is POSIX‑specific and will miss\examples\on Windows.Use a POSIX-normalized string for matching:
- const isExample = absPath.includes('/examples/') + const isExample = absPath.split(path.sep).join('/').includes('/examples/')If you keep the regex rewrite below, normalize before matching and convert back to native path for fs calls.
62-65: POSIX-only replacement; normalize before/after.The regex with forward slashes won’t match Windows paths.
Example approach:
- absPath = absPath.replace( - /\/docs\/framework\/([^/]+)\/examples\//, - '/examples/$1/' - ) + const posix = absPath.split(path.sep).join('/') + const replaced = posix.replace( + /\/docs\/framework\/([^/]+)\/examples\//, + '/examples/$1/' + ) + absPath = resolve(replaced.split('/').join(path.sep))tsconfig.json (1)
16-19: Enable stricter returns/unused checks when feasible.Good to stage behind TODOs now; plan to flip them on to catch dead code early.
- "noImplicitReturns": false, // TODO enable - "noUnusedLocals": false, // TODO enable - "noUnusedParameters": false, // TODO enable + "noImplicitReturns": true, + "noUnusedLocals": true, + "noUnusedParameters": true,package.json (1)
65-65: ESM/CJS interop for markdown-link-extractor.markdown-link-extractor is historically CJS. If scripts/verify-links.ts uses ESM import syntax, ensure tsconfig has esModuleInterop/syntheticDefaultImports enabled, or use createRequire() to load it. This avoids runtime import quirks under "type": "module".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.nvmrc(1 hunks)examples/angular/filters/src/app/app.component.html(1 hunks)examples/vue/filters/src/Filter.vue(1 hunks)package.json(3 hunks)packages/table-core/src/filterFns.ts(1 hunks)packages/table-core/src/utils.ts(1 hunks)packages/table-core/src/utils/getSortedRowModel.ts(1 hunks)scripts/verify-links.ts(3 hunks)tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (11)
examples/vue/filters/src/Filter.vue (1)
15-16: No functional change; formatting LGTM.examples/angular/filters/src/app/app.component.html (1)
13-14: Formatting only; safe.packages/table-core/src/utils.ts (1)
68-74: Type-only reflow; ok.scripts/verify-links.ts (4)
7-16: Behavior change: leading-slash links are now treated as “relative.” Intentional?Links like
/docs/guidewill be processed and then likely flagged as “above /docs”. If the docs site legitimately uses absolute site-root links under/docs, this increases strictness and may generate noise.Clarify desired policy for site-root links (e.g., allow only
/docs/**, or disallow all absolute links). I can adjustisRelativeLinkaccordingly once confirmed.
89-91: Glob migration LGTM.
112-113: Callback typing relaxation is fine here.
3-5: Dependency swap confirmed — tinyglobby present; fast-glob only transitive.tinyglobby is listed in package.json (devDependencies) and imported in scripts/verify-links.ts; fast-glob does not appear in package.json and only exists in pnpm-lock.yaml as a transitive dependency — no package.json cleanup required; lockfile already reflects tinyglobby.
.nvmrc (1)
1-1: Align Node 24.8.0 across CI/engines.nvmrc = 24.8.0; no actions/setup-node or node-version pins found in .github/workflows; package.json.engines is empty ({}); no .tool-versions/.node-version/.volta files detected. Update package.json.engines and any CI workflows to 24.8.0 (or a compatible semver range) to avoid CI/local mismatches.
Locations: .nvmrc (root), package.json → engines, .github/workflows/*
tsconfig.json (1)
25-26: Scope of root tsconfig changed — confirm package tsconfigs shouldn’t inherit these flags.All package tsconfigs extend ../../tsconfig.json, so root compiler flags (e.g., noUncheckedIndexedAccess) will be inherited.
Affected:
- packages/angular-table/tsconfig.json
- packages/lit-table/tsconfig.json
- packages/match-sorter-utils/tsconfig.json
- packages/qwik-table/tsconfig.json
- packages/react-table/tsconfig.json
- packages/react-table-devtools/tsconfig.json
- packages/solid-table/tsconfig.json
- packages/svelte-table/tsconfig.json
- packages/table-core/tsconfig.json
- packages/vue-table/tsconfig.json
If unintended, remove/override the extends or move shared flags to a base config used only where desired.
package.json (2)
47-79: Dependency matrix updates look fine overall; check Node typings vs runtime Node.
- You updated toolchain libs coherently (Rollup 4 + plugins, Prettier 3, Vitest 1, Nx 19, TS 5.4). LGTM.
- @types/node is pinned to ^20.14.9 while the repo (per PR context) targets Node 24.x in .nvmrc. Consider aligning types to the runtime (e.g., ^22 or ^24) to avoid missing globals/APIs in types.
If .nvmrc is Node 24.x, consider:
- "@types/node": "^20.14.9", + "@types/node": "^22.0.0",Or bump to the matching major for the Node LTS you actually use. Please confirm with your tsconfig and CI matrix.
6-6: Verify CI/local tooling provisions pnpm@10.16.1package.json pins "packageManager": "pnpm@10.16.1" (package.json, line 6). The workflow-scan produced no output so I couldn't confirm any corepack/pnpm setup in .github/workflows — manually verify your CI or add corepack/pnpm setup (e.g., corepack enable, setup-node with enableCorepack, or pnpm/action-setup) to avoid install drift.
| "verify-links": "node scripts/verify-links.ts", | ||
| "cipublish": "node scripts/publish.js" |
There was a problem hiding this comment.
verify-links will fail: Node can't execute TypeScript directly.
The script runs a .ts file with plain node, but ts-node/tsx isn’t present (ts-node was removed in this PR). This will exit with a syntax error on the first type.
Apply one of the following. Option A (preferred) adds tsx:
"prettier:write": "pnpm run prettier --write",
- "verify-links": "node scripts/verify-links.ts",
+ "verify-links": "tsx scripts/verify-links.ts",
"cipublish": "node scripts/publish.js"and in devDependencies:
"tinyglobby": "^0.2.15",
+ "tsx": "^4.16.0",
"typescript": "5.4.5",Option B: transpile the script to JS (e.g., scripts/verify-links.mjs) and keep using node. If you choose B, ensure the compiled file is committed or generated before use.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "verify-links": "node scripts/verify-links.ts", | |
| "cipublish": "node scripts/publish.js" | |
| "verify-links": "tsx scripts/verify-links.ts", | |
| "cipublish": "node scripts/publish.js" |
🤖 Prompt for AI Agents
In package.json around lines 26-27, the "verify-links" script invokes a
TypeScript (.ts) file directly with node which will fail; fix it by either
(preferred) adding a devDependency for "tsx" and updating the script to run the
TS file via tsx, or (alternate) transpiling the script to JavaScript (e.g., to
an .mjs file) and changing the script to run the compiled JS with node (ensure
the compiled file is committed or produced before running).
| "allowJs": true, | ||
| "allowSyntheticDefaultImports": true, | ||
| "allowUnreachableCode": false, | ||
| "allowUnusedLabels": false, | ||
| "checkJs": true, | ||
| "declaration": true, |
There was a problem hiding this comment.
Root emits: avoid generating .d.ts from scripts.
With "declaration": true and include: ["scripts"], running tsc -p . will emit declarations into the scripts tree. For a repo‑level config used mainly for tooling checks, prefer no output.
Recommend:
- "declaration": true,
+ "declaration": false,
+ "noEmit": true,🤖 Prompt for AI Agents
In tsconfig.json around lines 4 to 9, disable root-level declaration emission
and prevent any compiler output: remove or set "declaration": false and add
"noEmit": true (or set "emitDeclarationOnly": false if present); this ensures
tsc -p . used for tooling won't generate .d.ts files under scripts. Optionally,
if you still need declarations from specific packages, create a separate
tsconfig for builds that enables "declaration".
Looks like there was a bad merge conflict in 5f3fca4
Summary by CodeRabbit
Chores
Documentation
Style
Refactor