Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
There was a problem hiding this comment.
Pull Request Overview
This PR converts the nutils build process from tsup to Vite while adding related plugin and configuration changes, and extends the Notion API and CLI export features.
- Switched build tool and updated configuration in nutils (vite.config.ts, package.json, and removal of tsup.config.ts).
- Added a new Notion API method (getPageBacklinks) along with corresponding tests and documentation updates.
- Updated CLI export features with options for treemap and stats generation, though duplicate imports and redundant exports were introduced.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/nutils/vite.config.ts | New Vite configuration replacing tsup |
| packages/nutils/tsup.config.ts | Tsup configuration removed |
| packages/nutils/package.json | Updated build and watch scripts to use Vite |
| packages/nreact/src/context.tsx | Added cleanup of falsy component values |
| packages/nreact/src/components/search-dialog.tsx | Modified debounce logic with an unexpected onInput attribute |
| packages/nclient/src/notion-api.ts | Added new getPageBacklinks method |
| packages/nclient/src/notion-api.test.ts | New tests for the backlinks feature |
| packages/nclient/readme.md | Documentation updated for backlinks |
| packages/cli/src/treemap.ts | Changes in treemap generation with a potential undefined variable |
| packages/cli/src/notion/export.ts | Extended export functionality with duplicate import statements |
| packages/cli/src/main.ts | Duplicate export statements added |
| package.json | Added vite-plugin-dts dependency |
Comments suppressed due to low confidence (1)
packages/cli/src/treemap.ts:15
- The removal of the computation for 'treemapDataBlocks' causes its later use in generateTreemapHTML to reference an undefined variable. Ensure that a proper blocks metric is computed or remove the corresponding usage.
- const treemapDataBlocks = computeMetrics(pageTree, 'blocks')
|
|
||
| // debounce search calls so the expensive query only runs after typing stops | ||
| this._search = debounce(this._searchImpl.bind(this), 500) | ||
| onInput={this._onChangeQuery} |
There was a problem hiding this comment.
The 'onInput={this._onChangeQuery}' attribute appears to be misplaced within the debounce function block. Consider moving it to the appropriate component JSX element to avoid unintended behavior.
packages/cli/src/notion/export.ts
Outdated
| const stats = computeStats(tree) | ||
| await writeStats(`${this.folder}/stats.json`, stats) | ||
| } | ||
| import { generateTreemaps, PageNode } from '../treemap' |
There was a problem hiding this comment.
A duplicate import of 'generateTreemaps' and 'PageNode' was introduced, which can lead to confusion and potential maintenance issues. Removing the redundant import should resolve this.
| import { generateTreemaps, PageNode } from '../treemap' |
packages/cli/src/main.ts
Outdated
| export * from './treemap' | ||
| export * from './stats' |
There was a problem hiding this comment.
The duplicate export statement for './treemap' (and similarly for './stats') can cause redundancy and potential conflicts. Remove the extra export lines to keep the module exports clear.
| export * from './treemap' | |
| export * from './stats' |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Duplicate Cleanup Logic Creates Dead Code
Duplicate code block for cleaning up falsy components. The logic to filter out falsy component values is duplicated, appearing both inside and immediately outside a React.useMemo callback. The original code outside the callback was not removed when the logic was moved inside, resulting in unreachable dead code.
packages/nreact/src/context.tsx#L190-L203
notion-node/packages/nreact/src/context.tsx
Lines 190 to 203 in 23a22d9
Bug: Treemap Generation Fails Due to Syntax and Reference Errors
The generateTreemaps function is incomplete, causing a syntax error due to a missing closing brace. Additionally, it results in a runtime reference error because the treemapDataBlocks variable is used in a generateTreemapHTML call without being defined, as its declaration was removed. This prevents the blocktree.html generation feature from working.
packages/cli/src/treemap.ts#L14-L18
notion-node/packages/cli/src/treemap.ts
Lines 14 to 18 in 23a22d9
Bug: Duplicate Test Case Causes Runner Conflicts
The "Page Backlink" test case is duplicated. An identical test is defined twice, leading to test runner conflicts due to duplicate test names and potentially masking test failures.
packages/nclient/src/notion-api.test.ts#L67-L73
notion-node/packages/nclient/src/notion-api.test.ts
Lines 67 to 73 in 23a22d9
Bug: Code Corruption: Duplicate and Malformed `debounce` Function
The commit introduces malformed code that prevents compilation. It includes a duplicate, fragmented implementation of the debounce utility function, featuring an incomplete statement, an orphaned onInput JSX attribute, and a misplaced function body.
packages/nreact/src/components/search-dialog.tsx#L19-L29
notion-node/packages/nreact/src/components/search-dialog.tsx
Lines 19 to 29 in 23a22d9
Bug: Redundant Module Exports Cause Compilation Issues
Duplicate export statements for ./treemap and ./stats modules are present in packages/cli/src/main.ts. These redundant exports may cause compilation errors or unexpected module behavior.
packages/cli/src/main.ts#L20-L22
notion-node/packages/cli/src/main.ts
Lines 20 to 22 in 23a22d9
Bug: Malformed File Structure Causes Compilation Failure
The file has a malformed structure with duplicate import statements for generateTreemaps, PageNode, computeStats, and writeStats. It also includes orphaned Option.Boolean property definitions (treemap, stats) that are incorrectly placed outside the NotionExportCommand class. This will prevent compilation.
packages/cli/src/notion/export.ts#L1-L31
notion-node/packages/cli/src/notion/export.ts
Lines 1 to 31 in 23a22d9
BugBot free trial expires on June 13, 2025
You have used $0.00 of your $0.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
- Change assert to with for JSON imports - Add vite-plugin-dts to root devDependencies - Fix duplicate code from bad merge in context.tsx and search-dialog.tsx - Add missing imports in search-dialog.tsx
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Keep vite-plugin-dts from branch - Update vite and vitest to latest versions - Fix ESLint 9.x compatibility by adding ESLINT_USE_FLAT_CONFIG=false Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request migrates the monorepo's build toolchain from tsup to Vite across multiple packages. Root configuration is updated with ESLint flat config and vite-plugin-dts. Each package's tsup configuration is replaced with Vite equivalents while updating build and watch scripts. Minor refactoring occurs in nreact components. Documentation is added in CLAUDE.md. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 37-47: The fenced code block containing the ASCII architecture
diagram (the block starting with "ntypes (types)
─────────┬──────────────────────┐") is missing a language specifier; update that
opening fence to include a language identifier such as "text" or "plaintext"
(e.g., change "```" to "```text") so the markdown linter passes and the diagram
is rendered consistently.
In `@package.json`:
- Around line 46-47: The package.json has inconsistent indentation for the
"vite" dependency (the "vite" entry is missing the 4-space leading indentation
used by other entries); update the JSON so the "vite" line uses the same 4-space
indentation as surrounding dependency lines (ensure the key "vite": "^7.3.1",
matches the spacing style of "vite-plugin-dts" and other entries) and validate
the file remains valid JSON.
In `@packages/nutils/vite.config.ts`:
- Around line 1-20: The vite.config.ts currently hardcodes externals in the
build.rollupOptions.external array which diverges from other packages; update
this file to read peerDependencies and dependencies from this package's
package.json and compute the external list dynamically (like other packages do)
and replace the static array in build.rollupOptions.external with that computed
list; locate the export default defineConfig block and the
build.rollupOptions.external symbol to implement the change and ensure the dts
plugin usage (dts({ tsconfigPath: 'tsconfig.json', outDir: 'build' })) remains
unaffected.
🧹 Nitpick comments (2)
packages/ntypes/package.json (1)
26-28: Inconsistentvite-plugin-dtsplacement across packages.This package has
vite-plugin-dtsas a local devDependency, while other packages (ncompat, cli, nclient, nreact) rely on the root workspace's devDependency. Consider whether this should be:
- Hoisted to root only (like other packages), or
- Added consistently to all packages that need DTS generation.
Either approach works, but consistency aids maintainability.
packages/cli/vite.config.ts (1)
16-26: Consider a more comprehensive approach for Node built-in externalization.The explicit list of Node built-ins (
fs,fs/promises,path,stream,util) works, but may miss other built-ins if they're added later (e.g.,os,crypto,child_process). Consider using a regex pattern or thenode:protocol prefix for more robust externalization.♻️ Optional: Use regex for Node built-ins
rollupOptions: { external: [ ...Object.keys(pkg.dependencies || {}), ...Object.keys(pkg.peerDependencies || {}), - 'fs', - 'fs/promises', - 'path', - 'stream', - 'util' - ] + /^node:/, + /^(fs|path|stream|util|os|crypto|child_process|http|https|url|querystring|buffer|events|assert|module|tty|readline|process)(\/.*)?$/ + ] }
| ``` | ||
| ntypes (types) ─────────┬──────────────────────┐ | ||
| │ │ | ||
| ▼ ▼ | ||
| nutils (utilities) ncompat (compat layer) | ||
| │ | ||
| ┌─────────────┼─────────────┐ | ||
| ▼ ▼ ▼ | ||
| nclient nreact cli | ||
| (API) (React) (exporter) | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The architecture diagram code block lacks a language identifier. While this is an ASCII diagram, adding a language specifier (or text/plaintext) satisfies the markdown linting rule and improves consistency.
📝 Proposed fix
-```
+```text
ntypes (types) ─────────┬──────────────────────┐🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@CLAUDE.md` around lines 37 - 47, The fenced code block containing the ASCII
architecture diagram (the block starting with "ntypes (types)
─────────┬──────────────────────┐") is missing a language specifier; update that
opening fence to include a language identifier such as "text" or "plaintext"
(e.g., change "```" to "```text") so the markdown linter passes and the diagram
is rendered consistently.
| "vite": "^7.3.1", | ||
| "vite-plugin-dts": "^4.5.0", |
There was a problem hiding this comment.
Formatting inconsistency on line 46.
The vite entry on line 46 is missing leading indentation spaces, breaking the consistent JSON formatting. All other entries have proper indentation with 4 spaces.
🔧 Proposed fix
-"vite": "^7.3.1",
+ "vite": "^7.3.1",📝 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.
| "vite": "^7.3.1", | |
| "vite-plugin-dts": "^4.5.0", | |
| "vite": "^7.3.1", | |
| "vite-plugin-dts": "^4.5.0", |
🤖 Prompt for AI Agents
In `@package.json` around lines 46 - 47, The package.json has inconsistent
indentation for the "vite" dependency (the "vite" entry is missing the 4-space
leading indentation used by other entries); update the JSON so the "vite" line
uses the same 4-space indentation as surrounding dependency lines (ensure the
key "vite": "^7.3.1", matches the spacing style of "vite-plugin-dts" and other
entries) and validate the file remains valid JSON.
| import { defineConfig } from 'vite' | ||
| import dts from 'vite-plugin-dts' | ||
|
|
||
| export default defineConfig({ | ||
| build: { | ||
| lib: { | ||
| entry: 'src/index.ts', | ||
| formats: ['es'], | ||
| fileName: () => 'index.js' | ||
| }, | ||
| outDir: 'build', | ||
| target: 'es2015', | ||
| sourcemap: true, | ||
| minify: true, | ||
| rollupOptions: { | ||
| external: ['is-url-superb', 'mem', 'normalize-url', '@texonom/ntypes', 'p-queue'] | ||
| } | ||
| }, | ||
| plugins: [dts({ tsconfigPath: 'tsconfig.json', outDir: 'build' })] | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent externalization approach compared to other packages.
This config hardcodes the external dependencies while other packages (ncompat, nclient, ntypes, nreact) dynamically read them from package.json. This inconsistency creates a maintenance burden—if dependencies change, both package.json and vite.config.ts need to be updated.
♻️ Align with other packages by reading externals from package.json
import { defineConfig } from 'vite'
import dts from 'vite-plugin-dts'
+import pkg from './package.json' with { type: 'json' }
export default defineConfig({
build: {
lib: {
entry: 'src/index.ts',
formats: ['es'],
fileName: () => 'index.js'
},
outDir: 'build',
target: 'es2015',
sourcemap: true,
minify: true,
rollupOptions: {
- external: ['is-url-superb', 'mem', 'normalize-url', '@texonom/ntypes', 'p-queue']
+ external: [...Object.keys(pkg.dependencies || {}), ...Object.keys(pkg.peerDependencies || {})]
}
},
plugins: [dts({ tsconfigPath: 'tsconfig.json', outDir: 'build' })]
})🤖 Prompt for AI Agents
In `@packages/nutils/vite.config.ts` around lines 1 - 20, The vite.config.ts
currently hardcodes externals in the build.rollupOptions.external array which
diverges from other packages; update this file to read peerDependencies and
dependencies from this package's package.json and compute the external list
dynamically (like other packages do) and replace the static array in
build.rollupOptions.external with that computed list; locate the export default
defineConfig block and the build.rollupOptions.external symbol to implement the
change and ensure the dts plugin usage (dts({ tsconfigPath: 'tsconfig.json',
outDir: 'build' })) remains unaffected.
seonglae
deleted the
codex/refactor-tsup-to-vite-and-reduce-build-size
branch
1 participant
Summary
Testing
pnpm test(fails: fetch failed)pnpm buildhttps://chatgpt.com/codex/tasks/task_e_6842ca0173248327a02f307c5bfe9848
Summary by CodeRabbit
Bug Fixes
Chores