Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRepository-wide TypeScript/build standardization and targeted editor typing refactors: package build/check scripts unified to run Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Host Component
participant EditorBubbleMenu as EditorBubbleMenu
participant Child as Menu Child Components
Note over Parent,EditorBubbleMenu: New explicit prop-based editor flow
Parent->>EditorBubbleMenu: render(editor)
EditorBubbleMenu->>EditorBubbleMenu: store local editor variable
EditorBubbleMenu->>Child: pass `editor` prop to children
Child-->>EditorBubbleMenu: query state / isActive using editor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TypeScript configuration across all React libraries to centralize common settings and standardize build processes.
- Moves common TypeScript compiler options to the base configuration files
- Standardizes build and type-checking commands across all packages
- Adds build infrastructure and commands to the i18n package
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typescript-config/react-library.json | Adds common compiler options and exclude patterns |
| packages/typescript-config/base.json | Adds strictNullChecks to base configuration |
| packages/*/tsconfig.json | Removes redundant compiler options and exclude patterns |
| packages/*/package.json | Standardizes build and type-checking scripts |
| packages/i18n/ | Adds tsup configuration and build/dev commands |
| packages/editor/ | Updates to use new configuration and fixes TypeScript issues |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } catch (errPayload: any) { | ||
| const error = errPayload?.response?.data?.error || "Something went wrong"; |
There was a problem hiding this comment.
Using any type defeats the purpose of TypeScript's type safety. Consider using unknown instead and properly type guard the error payload, or define a proper error type interface.
| } catch (errPayload: any) { | |
| const error = errPayload?.response?.data?.error || "Something went wrong"; | |
| } catch (errPayload: unknown) { | |
| let error = "Something went wrong"; | |
| if ( | |
| typeof errPayload === "object" && | |
| errPayload !== null && | |
| "response" in errPayload && | |
| typeof (errPayload as any).response === "object" && | |
| (errPayload as any).response !== null && | |
| "data" in (errPayload as any).response && | |
| typeof (errPayload as any).response.data === "object" && | |
| (errPayload as any).response.data !== null && | |
| "error" in (errPayload as any).response.data | |
| ) { | |
| error = (errPayload as any).response.data.error; | |
| } |
There was a problem hiding this comment.
this is a good suggestion, however, I would write it differently, e.g.
function isResponseError(err): err is ResponseError {
...
}then your code would look like:
let error = "Something went wrong";
if (isResponseError(err)) {
error = err.response.data.error;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/i18n/package.json (1)
23-29: Add React and MobX-React as peers and externalize in your bundleWe’ve confirmed that the i18n package directly imports from both React and mobx-react at runtime, so React must be declared as a peer dependency and excluded from your bundle to avoid duplicate copies and inflated output.
Files importing React or mobx-react at runtime:
- packages/i18n/src/hooks/use-translation.ts (line 1:
import { useContext } from "react")- packages/i18n/src/context/index.tsx (lines 1–2:
import { observer } from "mobx-react"; import React, { createContext } from "react";)Please apply the following updates:
- package.json (packages/i18n/package.json)
"dependencies": { "@plane/utils": "workspace:*", "intl-messageformat": "^10.7.11", "mobx": "^6.13.5", "mobx-react": "^9.1.0", "lodash": "^4.17.21" },
- "peerDependencies": {
"react": ">=18","react-dom": ">=18"- },
2. **tsup.config.ts** (packages/i18n/tsup.config.ts) ```diff export default defineConfig({ entry: ["src/index.ts"], - external: ["react"], + external: ["react", "react-dom", "mobx", "mobx-react"], format: ["cjs", "esm"], dts: true, });These changes will:
- Prevent bundling React and React-DOM, ensuring consumers manage the React version.
- Avoid shipping a second copy of mobx-react and its React dependency.
- Keep your bundle lean by externalizing all peer libraries.
Let me know if you need any help tweaking the config!
🧹 Nitpick comments (21)
packages/hooks/tsconfig.json (1)
3-3: Standardize include path style across packagesHooks uses "./src" while other packages (e.g., propel) use "src". Pick one style repo-wide to avoid churn in diffs and IDE globs. Suggest "src" for consistency.
Apply:
- "include": ["./src"] + "include": ["src"]packages/i18n/tsconfig.json (1)
3-7: Remove redundant resolveJsonModule; base already enables itpackages/typescript-config/base.json sets resolveJsonModule: true, so this override is unnecessary noise. Also consider aligning include path style with others.
Apply:
"extends": "@plane/typescript-config/react-library.json", - "compilerOptions": { - "resolveJsonModule": true - }, - "include": ["./src"] + "include": ["src"]packages/typescript-config/base.json (2)
16-16: strictNullChecks is redundant when strict is trueTypeScript’s strict already enables strictNullChecks. Keeping both is benign but adds noise. Recommend removing for clarity.
Apply:
- "strictNullChecks": true,
7-8: Consider enabling incremental builds for dev ergonomicsincremental: false can slow local type-check cycles in a large monorepo. Consider flipping to true in a dev-only override or per-package tsconfig to speed up inner loop.
packages/utils/package.json (1)
14-16: Minification flag removed — confirm bundle size expectationsPrevious build used "--minify". If size budgets matter, either restore "--minify" or enable minify in tsup.config.ts.
packages/hooks/package.json (1)
14-18: Avoid emitting on type-check; revert to noEmit.“check:types”: “tsc” writes build artifacts during CI/local checks. Prefer noEmit for speed and clean trees; keep “tsc && tsup” in build for d.ts generation.
"scripts": { - "build": "tsc && tsup", + "build": "tsc && tsup", "dev": "tsup --watch", "check:lint": "eslint . --max-warnings 6", - "check:types": "tsc", + "check:types": "tsc --noEmit", "check:format": "prettier --check \"**/*.{ts,tsx,md,json,css,scss}\"",packages/shared-state/package.json (1)
10-16: Type-checking emits files but this package has no build; use noEmit or add build.With “check:types”: “tsc”, outputs may be emitted (to dist or alongside sources) causing churn. Either switch to noEmit or add a proper build that writes to dist and points main/types there.
Option A (no build):
- "check:types": "tsc", + "check:types": "tsc --noEmit",Option B (add build; adjust entrypoints accordingly):
+ "build": "tsc",Then ensure tsconfig outDir points to dist and update:
- "main": "./src/index.ts", - "types": "./src/index.ts", + "main": "./dist/index.js", + "types": "./dist/index.d.ts",packages/editor/src/core/hooks/use-file-upload.ts (1)
69-75: Prefer unknown over any + narrow the error shape.Use unknown in catch and extract a message safely; avoids disabling type-safety across the block.
- } catch (errPayload: any) { - const error = errPayload?.response?.data?.error || "Something went wrong"; - console.error(error); + } catch (errPayload: unknown) { + const error = extractUploadError(errPayload); + console.error(error); } finally {Add once in this module (outside this range):
function extractUploadError(e: unknown): string { if (typeof e === "object" && e !== null) { const maybe = e as { response?: { data?: { error?: unknown } } }; const msg = maybe.response?.data?.error; if (typeof msg === "string") return msg; } return e instanceof Error ? e.message : "Something went wrong"; }packages/propel/package.json (1)
8-8: Don’t emit during type-check for Storybook-only package.This package doesn’t build a dist; switch to noEmit to avoid stray outputs.
- "check:types": "tsc", + "check:types": "tsc --noEmit",packages/ui/package.json (1)
21-21: "check:types" currently emits files; consider noEmit to avoid redundant builds.If CI uses this for type-check only, swap to --noEmit to speed up and prevent dist churn.
- "check:types": "tsc", + "check:types": "tsc --noEmit",packages/editor/src/core/extensions/emoji/suggestion.ts (1)
30-35: Prefer a type guard over.filter(Boolean)to drop the final type assertion.This tightens types and avoids
as EmojiItem[].- const defaultEmojis = DEFAULT_EMOJIS.map((name) => - filteredEmojis.find((emoji) => emoji.shortcodes.includes(name) || emoji.name === name) - ) - .filter(Boolean) - .slice(0, 5); - return defaultEmojis as EmojiItem[]; + const defaultEmojis = DEFAULT_EMOJIS.map((name) => + filteredEmojis.find((emoji) => emoji.shortcodes.includes(name) || emoji.name === name) + ) + .filter((e): e is EmojiItem => Boolean(e)) + .slice(0, 5); + return defaultEmojis;packages/editor/package.json (2)
25-27: Guarantee both CJS and ESM outputs for the declared exports.Exports declare both
./dist/lib.js(require) and./dist/lib.mjs(import). Ensure tsup actually emits both formats.Quick script tweak:
- "build": "tsc && tsup", + "build": "tsc && tsup --format esm,cjs",Or add a tsup.config.ts mirroring i18n (with dts: false) to standardize formats and avoid duplication.
To verify after a build, confirm these paths exist:
- dist/index.mjs
- dist/lib.js
- dist/lib.mjs
28-28: Optional: make type-checks non-emitting to speed CI.Aligns with typical “check” semantics and prevents accidental dist changes during checks.
- "check:types": "tsc", + "check:types": "tsc --noEmit",packages/i18n/tsup.config.ts (1)
7-13: Avoid duplicate .d.ts generation with "tsc && tsup".Since packages run
tscbefore tsup, generating declarations in tsup is redundant and can cause conflicts.- dts: true, + dts: false, + sourcemap: true,If you prefer tsup to own types instead, drop the
tscstep from the package’s build script and keepdts: true.packages/i18n/package.json (1)
7-12: Dist entry points look good; add exports map for Node resolution and future-proofing.Expose explicit subpath exports to avoid deep imports and enable conditional exports.
"main": "./dist/index.js", "module": "./dist/index.mjs", "types": "./dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.mjs", + "require": "./dist/index.js" + } + }, "files": [ "dist/**" ],packages/services/package.json (3)
22-26: Consider externalizing axios to avoid bundling it into every consumer.For libraries, keeping runtime deps external reduces duplicate copies across apps. If you adopt tsup.config.ts, set external: ["axios"].
6-11: Add exports map for correctness on Node’s ESM resolver and tooling."main": "./dist/index.js", "module": "./dist/index.mjs", "types": "./dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.mjs", + "require": "./dist/index.js" + } + }, "files": [ "dist/**" ],
13-17: Rely on tsup for bundling and leverage the existingnoEmitin your tsconfigThe
react-library.jsonbase config already sets"noEmit": true, so runningtscwon’t produce any JS artifacts by default. To keep things DRY and prevent duplicate builds:• In your
packages/services/package.jsonscripts:
- Remove the standalone
tscfrom thebuildcommand and lettsuphandle both bundling and emission.- Keep type‐checking in its own step. Since
noEmitis inherited,tscwon’t write files—but you can add--noEmitif you want to make that explicit.Suggested diff (optional refactor):
"scripts": { - "build": "tsc && tsup", + "build": "tsup", "dev": "tsup --watch", "check:lint": "eslint . --max-warnings 62", - "check:types": "tsc", + "check:types": "tsc --noEmit", // optional: explicit noEmit flag, though it's already in your tsconfig "check:format": "prettier --check \"**/*.{ts,tsx,md,json,css,scss}\"", }This keeps bundling and type‐checking clearly separated and avoids running
tsctwice.packages/services/tsconfig.json (1)
2-4: Extends react-library for a non-React package; consider a leaner base.If services doesn’t use JSX, extend a non-React base to avoid unnecessary DOM typings and mismatched assumptions.
- "extends": "@plane/typescript-config/react-library.json", + "extends": "@plane/typescript-config/base.json", "include": ["./src"]packages/utils/tsconfig.json (1)
2-4: Same as services: prefer a non-React base if no JSX is used.Keeps the config minimal and prevents accidental DOM/JSX leakage into utils.
- "extends": "@plane/typescript-config/react-library.json", + "extends": "@plane/typescript-config/base.json", "include": ["./src"]packages/editor/tsconfig.json (1)
6-6: Reconsider disabling strict mode for a library.Library outputs benefit from strict for better DX and earlier bug surfacing. If turning it off is intentional, add a short rationale in the README or tsconfig comment, or scope relaxations via targeted ts-ignore/tsconfig overrides instead.
- "strict": false, + "strict": true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
packages/editor/package.json(2 hunks)packages/editor/src/core/extensions/code/code-block.ts(0 hunks)packages/editor/src/core/extensions/emoji/suggestion.ts(1 hunks)packages/editor/src/core/hooks/use-file-upload.ts(1 hunks)packages/editor/src/core/plugins/file/restore.ts(1 hunks)packages/editor/tsconfig.json(1 hunks)packages/hooks/package.json(1 hunks)packages/hooks/tsconfig.json(1 hunks)packages/i18n/package.json(2 hunks)packages/i18n/tsconfig.json(1 hunks)packages/i18n/tsup.config.ts(1 hunks)packages/propel/package.json(1 hunks)packages/propel/tsconfig.json(1 hunks)packages/services/package.json(1 hunks)packages/services/tsconfig.json(1 hunks)packages/shared-state/package.json(1 hunks)packages/shared-state/tsconfig.json(1 hunks)packages/typescript-config/base.json(1 hunks)packages/typescript-config/react-library.json(1 hunks)packages/ui/package.json(1 hunks)packages/ui/tsconfig.json(1 hunks)packages/utils/package.json(1 hunks)packages/utils/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/core/extensions/code/code-block.ts
⏰ 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: Analyze (javascript)
🔇 Additional comments (8)
packages/hooks/tsconfig.json (1)
1-4: No changes required: JSX mode correctly inheritedThe shared config at
packages/typescript-config/react-library.jsonalready defines"compilerOptions": { "jsx": "react-jsx" }so removing per-file JSX settings is safe.
packages/propel/tsconfig.json (1)
3-3: LGTM — minimal tsconfig aligns with shared configinclude is scoped to src and the rest is inherited from react-library.json. No issues spotted.
packages/utils/package.json (1)
37-45: Toolchain Versions Are Consistent—No Action RequiredAll packages in
packages/**/package.jsondeclare TypeScript version5.8.3, and every package that depends ontsupuses version8.4.0. No mismatches were detected.packages/editor/src/core/extensions/emoji/suggestion.ts (1)
30-30: LGTM: inferred callback type is sufficient.Removing the explicit EmojiItem annotation here is safe and keeps the code cleaner.
packages/i18n/package.json (1)
14-18: No duplicate JS artifacts – base config enforces noEmit=true
Thepackages/i18n/tsconfig.jsonextends@plane/typescript-config/react-library.json, which already sets"noEmit": true. As a result, runningtscin both your"build"and"check:types"scripts performs type-checking only and does not emit any JS files, so there’s no risk of clobbering or duplicate output.If you want to make the no-emit behavior more explicit, you can optionally update the
"check:types"script to:"check:types": "tsc --noEmit"—but this change is purely stylistic and not strictly necessary.
Likely an incorrect or invalid review comment.
packages/editor/tsconfig.json (2)
15-15: Include looks fine.Matches typical library layouts; inherits exclude from the base config.
2-2: Verify TS config resolution post-installCentralizing on the shared
react-library.jsonconfig is a great move—this reduces per-package drift and duplication. I’ve confirmed:• The
packages/typescript-configpackage includesreact-library.jsonin itsfilesarray.
•packages/editor/package.jsondeclares"@plane/typescript-config": "workspace:*"as a dependency.After running a fresh install, TypeScript should be able to resolve
"extends": "@plane/typescript-config/react-library.json"from withinpackages/editor. Please verify locally, for example:cd packages/editor npm install npx tsc --showConfigpackages/typescript-config/react-library.json (1)
10-10: Exclude is appropriate for shared config.Keeps transient outputs and deps out of the program graph across all libraries.
|
@aaryan610 just a thought, but since you're already upgrading tsup, might be worth taking a look at tsdown it would potentially improve our build times substantially. |
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 (4)
packages/editor/src/core/extensions/slash-commands/root.tsx (1)
58-90: Guard against missing clientRect and simplify popup typing.
Without a guard, tippy may receive undefined getReferenceClientRect and break; use ReturnType instead of the (Instance[] & Instance) hack.-const renderItems: SuggestionOptions["render"] = () => { +const renderItems: SuggestionOptions["render"] = () => { let component: ReactRenderer<CommandListInstance, SlashCommandsMenuProps> | null = null; - let popup: (Instance[] & Instance) | null = null; + let popup: ReturnType<typeof tippy> | null = null; return { - onStart: (props) => { + onStart: (props) => { + if (!props.clientRect) return; component = new ReactRenderer<CommandListInstance, SlashCommandsMenuProps>(SlashCommandsMenu, { props, editor: props.editor, });packages/editor/src/core/extensions/callout/utils.ts (3)
27-31: UseEAttributeNamesfor fallback keys.Align fallback with the enum-based schema.
Apply:
- const fallBackValues: TStoredLogoValue = { - "data-logo-in-use": "emoji", - "data-emoji-unicode": DEFAULT_CALLOUT_BLOCK_ATTRIBUTES["data-emoji-unicode"], - "data-emoji-url": DEFAULT_CALLOUT_BLOCK_ATTRIBUTES["data-emoji-url"], - }; + const fallBackValues: TStoredLogoValue = { + [EAttributeNames.LOGO_IN_USE]: "emoji", + [EAttributeNames.EMOJI_UNICODE]: + DEFAULT_CALLOUT_BLOCK_ATTRIBUTES[EAttributeNames.EMOJI_UNICODE], + [EAttributeNames.EMOJI_URL]: + DEFAULT_CALLOUT_BLOCK_ATTRIBUTES[EAttributeNames.EMOJI_URL], + };
46-49: Return emoji branch with enum keys.Keep access/indexing consistent with the enum.
Apply:
- return { - "data-logo-in-use": "emoji", - "data-emoji-unicode": parsedData.emoji.value || DEFAULT_CALLOUT_BLOCK_ATTRIBUTES["data-emoji-unicode"], - "data-emoji-url": parsedData.emoji.url || DEFAULT_CALLOUT_BLOCK_ATTRIBUTES["data-emoji-url"], - }; + return { + [EAttributeNames.LOGO_IN_USE]: "emoji", + [EAttributeNames.EMOJI_UNICODE]: + parsedData.emoji.value || + DEFAULT_CALLOUT_BLOCK_ATTRIBUTES[EAttributeNames.EMOJI_UNICODE], + [EAttributeNames.EMOJI_URL]: + parsedData.emoji.url || + DEFAULT_CALLOUT_BLOCK_ATTRIBUTES[EAttributeNames.EMOJI_URL], + };
53-56: Return icon branch with enum keys.Same consistency fix for icon attributes.
Apply:
- return { - "data-logo-in-use": "icon", - "data-icon-name": parsedData.icon.name || DEFAULT_CALLOUT_BLOCK_ATTRIBUTES["data-icon-name"], - "data-icon-color": parsedData.icon.color || DEFAULT_CALLOUT_BLOCK_ATTRIBUTES["data-icon-color"], - }; + return { + [EAttributeNames.LOGO_IN_USE]: "icon", + [EAttributeNames.ICON_NAME]: + parsedData.icon.name || + DEFAULT_CALLOUT_BLOCK_ATTRIBUTES[EAttributeNames.ICON_NAME], + [EAttributeNames.ICON_COLOR]: + parsedData.icon.color || + DEFAULT_CALLOUT_BLOCK_ATTRIBUTES[EAttributeNames.ICON_COLOR], + };
🧹 Nitpick comments (8)
packages/editor/src/core/extensions/emoji/extension.ts (2)
15-15: Nit: destructure only what you need to tighten the closure.Slightly clearer and avoids closing over the entire options object.
- const extensionOptions = this.options; + const { emojis } = this.options; ... - const emojiItem = shortcodeToEmoji(node.attrs.name, extensionOptions.emojis); + const emojiItem = shortcodeToEmoji(node.attrs.name, emojis);
19-19: Guard against missingshortcodes[0]in alt text.If
shortcodesis empty/undefined, the alt text currently rendersundefined. Fallback to the emoji name.- state.write(`\n![${emojiItem.name}-${emojiItem.shortcodes[0]}](${emojiItem?.fallbackImage})\n`); + const shortcode = emojiItem.shortcodes?.[0] ?? emojiItem.name; + state.write(`\n\n`);packages/i18n/tsconfig.json (1)
4-4: Confirm resolveJsonModule necessityKeep resolveJsonModule: true only if this package imports JSON (likely for locale catalogs). If not, drop it to minimize config surface.
packages/editor/src/core/components/menus/menu-items.ts (1)
250-278: Avoid end-of-array type assertion; prefer satisfies for safer typing.
Removes a potentially unsound cast while preserving type guarantees.Apply:
export const getEditorMenuItems = (editor: Editor | null): EditorMenuItem<TEditorCommands>[] => { if (!editor) return []; - - return [ + const items = [ TextItem(editor), HeadingOneItem(editor), HeadingTwoItem(editor), HeadingThreeItem(editor), HeadingFourItem(editor), HeadingFiveItem(editor), HeadingSixItem(editor), BoldItem(editor), ItalicItem(editor), UnderLineItem(editor), StrikeThroughItem(editor), BulletListItem(editor), TodoListItem(editor), CodeItem(editor), NumberedListItem(editor), QuoteItem(editor), TableItem(editor), ImageItem(editor), HorizontalRuleItem(editor), LinkItem(editor), TextColorItem(editor), BackgroundColorItem(editor), TextAlignItem(editor), - ] as EditorMenuItem<TEditorCommands>[]; + ] satisfies EditorMenuItem<TEditorCommands>[]; + return items; };If the repo is on TypeScript < 4.9, keep the current assertion or upgrade TS.
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (1)
34-47: Redundant cast on items; it’s already typed.
Drop the trailing assertion to reduce noise.const items: EditorMenuItem<TEditorCommands>[] = [ TextItem(editor), HeadingOneItem(editor), HeadingTwoItem(editor), HeadingThreeItem(editor), HeadingFourItem(editor), HeadingFiveItem(editor), HeadingSixItem(editor), BulletListItem(editor), NumberedListItem(editor), TodoListItem(editor), QuoteItem(editor), CodeItem(editor), - ] as EditorMenuItem<TEditorCommands>[]; + ];packages/editor/src/core/extensions/mentions/utils.ts (1)
17-19: Tighten popup typing with ReturnType.
Removes the need for the array+instance intersection.() => { const { searchCallback } = props; let component: ReactRenderer<CommandListInstance, MentionsListDropdownProps> | null = null; - let popup: (Instance[] & Instance) | null = null; + let popup: ReturnType<typeof tippy> | null = null;Optionally, also convert the import to a type-only or drop it if unused after this change.
packages/editor/src/core/extensions/callout/utils.ts (2)
34-39: Don’t sanitize before JSON.parse; sanitize at sink.HTML-sanitizing the JSON string can corrupt it and cause unnecessary parse failures. Parse raw, then sanitize only when inserting into the DOM.
Apply:
- const storedData = sanitizeHTML(localStorage.getItem("editor-calloutComponent-logo") ?? ""); + const storedData = localStorage.getItem("editor-calloutComponent-logo"); if (storedData) { let parsedData: TEmojiLogoProps; try { parsedData = JSON.parse(storedData);
69-74: Returnnullwhen the background key is absent to match the signature.Current code returns
""instead ofnullwhen the item is missing.Apply:
-export const getStoredBackgroundColor = (): string | null => { - if (typeof window !== "undefined") { - return sanitizeHTML(localStorage.getItem("editor-calloutComponent-background") ?? ""); - } - return null; -}; +export const getStoredBackgroundColor = (): string | null => { + if (typeof window === "undefined") return null; + const raw = localStorage.getItem("editor-calloutComponent-background"); + return raw === null ? null : sanitizeHTML(raw); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx(1 hunks)packages/editor/src/core/components/menus/menu-items.ts(4 hunks)packages/editor/src/core/extensions/callout/utils.ts(1 hunks)packages/editor/src/core/extensions/emoji/extension.ts(1 hunks)packages/editor/src/core/extensions/mentions/utils.ts(3 hunks)packages/editor/src/core/extensions/slash-commands/root.tsx(3 hunks)packages/editor/tsconfig.json(1 hunks)packages/hooks/tsconfig.json(1 hunks)packages/i18n/tsconfig.json(1 hunks)packages/propel/tsconfig.json(1 hunks)packages/services/tsconfig.json(1 hunks)packages/shared-state/tsconfig.json(1 hunks)packages/typescript-config/react-library.json(1 hunks)packages/ui/tsconfig.json(1 hunks)packages/utils/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/utils/tsconfig.json
- packages/ui/tsconfig.json
- packages/editor/tsconfig.json
- packages/hooks/tsconfig.json
- packages/propel/tsconfig.json
- packages/typescript-config/react-library.json
- packages/shared-state/tsconfig.json
- packages/services/tsconfig.json
🧰 Additional context used
🧬 Code graph analysis (4)
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (2)
packages/editor/src/core/components/menus/menu-items.ts (1)
EditorMenuItem(57-63)packages/editor/src/core/types/editor.ts (1)
TEditorCommands(25-53)
packages/editor/src/core/extensions/slash-commands/root.tsx (2)
packages/editor/src/core/helpers/tippy.ts (1)
CommandListInstance(1-3)packages/editor/src/core/extensions/slash-commands/command-menu.tsx (1)
SlashCommandsMenuProps(10-14)
packages/editor/src/core/extensions/mentions/utils.ts (3)
packages/editor/src/core/types/mention.ts (1)
TMentionHandler(21-25)packages/editor/src/core/helpers/tippy.ts (1)
CommandListInstance(1-3)packages/editor/src/core/extensions/mentions/mentions-list-dropdown.tsx (1)
MentionsListDropdownProps(13-17)
packages/editor/src/core/components/menus/menu-items.ts (1)
packages/editor/src/core/types/editor.ts (1)
TEditorCommands(25-53)
🔇 Additional comments (8)
packages/editor/src/core/extensions/emoji/extension.ts (1)
10-10: Good fix: capture options to avoid incorrectthisin serializer.Capturing
this.optionsoutside theserializemethod prevents accidentalthisrebinding issues. This makes the serializer robust.packages/i18n/tsconfig.json (2)
7-7: No ambient.d.tsfiles found outside src—"include": ["./src"]is safe as is.
4-5: Verify .d.ts emission for @plane/i18n build
noEmit: true disables tsc output—ensure tsup’sdtsoption (or a dedicated tsconfig.build.json) actually generates.d.tsfiles underpackages/i18n/dist. Run:pnpm -F @plane/i18n build find packages/i18n/dist -type f -name "*.d.ts"and confirm at least one declaration file is present.
packages/editor/src/core/components/menus/menu-items.ts (3)
1-1: Good switch to type-only import for Editor.
Reduces bundle risk and keeps runtime clean.
21-21: Importing LucideIcon as a type is correct.
Prevents unnecessary runtime import from lucide-react.
73-73: Nice: heading levels gated via Extract.
Keeps typings resilient if TEditorCommands evolves.packages/editor/src/core/extensions/mentions/utils.ts (2)
2-3: Type-only imports look good.
Removes value imports and avoids unnecessary runtime bits.
46-54: Good: stable rect on update.
Computing clientRect and supplying a function eliminates flicker and matches tippy expectations.
We are already working on switching to |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/editor/src/core/extensions/callout/block.tsx (1)
26-26: Unify attribute key access via enum and null→undefined mapping across callout extension
Replace every hard-coded"data-*"key in packages/editor/src/core/extensions/callout/* with the matching ECalloutAttributeNames enum and, in all onSelect handlers, typevalasstring | nullmappingnull → undefinedforupdateAttributes(while still writingnullto storage).
Affected files:
- block.tsx (line 26 & onSelect)
- logo-selector.tsx (all
blockAttributes["data-…"]and newLogoValue keys)- extension-config.ts (attrs["data-…"] in rendering)
- extension.tsx (
"data-background"in stored values)packages/editor/src/core/extensions/callout/extension-config.ts (1)
42-51: Replace serializer magic strings with enum keys to prevent driftUse ECalloutAttributeNames for all attribute reads.
- const attrs = node.attrs as TCalloutBlockAttributes; - const logoInUse = attrs["data-logo-in-use"]; + const attrs = node.attrs as TCalloutBlockAttributes; + const logoInUse = attrs[ECalloutAttributeNames.LOGO_IN_USE]; // add callout logo if (logoInUse === "emoji") { state.write( - `> <img src="${attrs["data-emoji-url"]}" alt="${attrs["data-emoji-unicode"]}" width="30px" />\n` + `> <img src="${attrs[ECalloutAttributeNames.EMOJI_URL]}" alt="${attrs[ECalloutAttributeNames.EMOJI_UNICODE]}" width="30px" />\n` ); } else { - state.write(`> <icon>${attrs["data-icon-name"]} icon</icon>\n`); + state.write(`> <icon>${attrs[ECalloutAttributeNames.ICON_NAME]} icon</icon>\n`); }packages/editor/src/core/components/menus/bubble-menu/root.tsx (1)
134-160: Prevent potential event-listener leaks on unmountmousemove and mouseup listeners added inside handleMouseDown aren’t removed if the component unmounts before mouseup. Use an AbortController stored in a ref to guarantee cleanup.
const [isSelecting, setIsSelecting] = useState(false); // refs - const menuRef = useRef<HTMLDivElement>(null); + const menuRef = useRef<HTMLDivElement>(null); + const dragControllerRef = useRef<AbortController | null>(null); @@ useEffect(() => { function handleMouseDown(e: MouseEvent) { if (menuRef.current?.contains(e.target as Node)) return; - function handleMouseMove() { - if (!editor?.state.selection.empty) { - setIsSelecting(true); - document.removeEventListener("mousemove", handleMouseMove); - } - } - - function handleMouseUp() { - setIsSelecting(false); - document.removeEventListener("mousemove", handleMouseMove); - document.removeEventListener("mouseup", handleMouseUp); - } - - document.addEventListener("mousemove", handleMouseMove); - document.addEventListener("mouseup", handleMouseUp); + // abort any previous sequence + dragControllerRef.current?.abort(); + const controller = new AbortController(); + dragControllerRef.current = controller; + const { signal } = controller; + + const handleMouseMove = () => { + if (!editor?.state.selection.empty) { + setIsSelecting(true); + controller.abort(); // removes both listeners via signal + } + }; + + const handleMouseUp = () => { + setIsSelecting(false); + controller.abort(); + }; + + document.addEventListener("mousemove", handleMouseMove, { signal }); + document.addEventListener("mouseup", handleMouseUp, { signal }); } document.addEventListener("mousedown", handleMouseDown); return () => { document.removeEventListener("mousedown", handleMouseDown); + dragControllerRef.current?.abort(); }; }, [editor]);Also applies to: 61-63
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/callout/utils.ts (1)
12-20: Migration to enum-based computed keys completed—niceDefaults, fallback, and return paths now consistently use ECalloutAttributeNames. This removes a whole class of silent breakages on key renames.
Also applies to: 28-31, 46-60
🧹 Nitpick comments (5)
packages/editor/src/core/extensions/callout/extension-config.ts (1)
28-34: Type the reducer to avoid implicit any and future key typosAdd a reducer generic so the accumulator isn’t any.
- ...Object.values(ECalloutAttributeNames).reduce((acc, value) => { + ...Object.values(ECalloutAttributeNames).reduce<Record<string, { default: unknown }>>((acc, value) => { acc[value] = { default: DEFAULT_CALLOUT_BLOCK_ATTRIBUTES[value], }; return acc; - }, {}), + }, {}),packages/editor/src/core/extensions/callout/utils.ts (2)
73-78: Return null when no stored background (avoid empty-string sentinel)Current code returns "" which blurs “unset” vs “empty”. Prefer null when absent.
export const getStoredBackgroundColor = (): string | null => { if (typeof window !== "undefined") { - return sanitizeHTML(localStorage.getItem("editor-calloutComponent-background") ?? ""); + const raw = localStorage.getItem("editor-calloutComponent-background"); + return raw ? sanitizeHTML(raw) : null; } return null; };
69-71: Nit: centralize localStorage keysConsider constants for "editor-calloutComponent-logo" and "editor-calloutComponent-background" to prevent typos.
Also applies to: 80-88
packages/editor/src/core/components/menus/bubble-menu/root.tsx (2)
64-71: Memoize formattingItems to avoid per-render reallocationformattingItems recreates command objects on every render. It’s small but hot code; memoize by editor.
-import { FC, useEffect, useState, useRef } from "react"; +import { FC, useEffect, useState, useRef, useMemo } from "react"; @@ - const formattingItems = { - code: CodeItem(editor), - bold: BoldItem(editor), - italic: ItalicItem(editor), - underline: UnderLineItem(editor), - strikethrough: StrikeThroughItem(editor), - "text-align": TextAlignItem(editor), - } satisfies { + const formattingItems = useMemo(() => ({ + code: CodeItem(editor), + bold: BoldItem(editor), + italic: ItalicItem(editor), + underline: UnderLineItem(editor), + strikethrough: StrikeThroughItem(editor), + "text-align": TextAlignItem(editor), + }), [editor]) satisfies { [K in TEditorCommands]?: EditorMenuItem<K>; };Also applies to: 2-2
75-89: Avoid shadowing editor inside selector for readabilityThe selector receives { editor } but you also close over the outer editor via formattingItems. Rename the param to prevent confusion.
- const editorState: EditorStateType = useEditorState({ - editor, - selector: ({ editor }: { editor: Editor }) => ({ + const editorState: EditorStateType = useEditorState({ + editor, + selector: ({ editor: ed }: { editor: Editor }) => ({ code: formattingItems.code.isActive(), bold: formattingItems.bold.isActive(), italic: formattingItems.italic.isActive(), underline: formattingItems.underline.isActive(), strikethrough: formattingItems.strikethrough.isActive(), left: formattingItems["text-align"].isActive({ alignment: "left" }), right: formattingItems["text-align"].isActive({ alignment: "right" }), center: formattingItems["text-align"].isActive({ alignment: "center" }), - color: COLORS_LIST.find((c) => TextColorItem(editor).isActive({ color: c.key })), - backgroundColor: COLORS_LIST.find((c) => BackgroundColorItem(editor).isActive({ color: c.key })), + color: COLORS_LIST.find((c) => TextColorItem(ed).isActive({ color: c.key })), + backgroundColor: COLORS_LIST.find((c) => BackgroundColorItem(ed).isActive({ color: c.key })), }), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/editor/src/core/components/menus/bubble-menu/root.tsx(10 hunks)packages/editor/src/core/extensions/callout/block.tsx(2 hunks)packages/editor/src/core/extensions/callout/extension-config.ts(3 hunks)packages/editor/src/core/extensions/callout/types.ts(2 hunks)packages/editor/src/core/extensions/callout/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/editor/src/core/components/menus/bubble-menu/root.tsx (3)
packages/editor/src/core/components/menus/menu-items.ts (7)
CodeItem(171-177)BoldItem(107-113)ItalicItem(115-121)UnderLineItem(123-129)StrikeThroughItem(131-137)TextAlignItem(239-248)EditorMenuItem(57-63)packages/editor/src/core/types/editor.ts (1)
TEditorCommands(25-53)packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (1)
TextAlignmentSelector(16-82)
packages/editor/src/core/extensions/callout/extension-config.ts (1)
packages/editor/src/core/extensions/callout/utils.ts (1)
DEFAULT_CALLOUT_BLOCK_ATTRIBUTES(12-20)
packages/editor/src/core/extensions/callout/utils.ts (1)
packages/editor/src/core/extensions/callout/types.ts (3)
TCalloutBlockAttributes(21-26)TCalloutBlockEmojiAttributes(16-19)TCalloutBlockIconAttributes(11-14)
🔇 Additional comments (5)
packages/editor/src/core/extensions/callout/block.tsx (1)
9-9: Enum rename alignment looks goodImport updated to ECalloutAttributeNames; matches downstream usage.
packages/editor/src/core/extensions/callout/extension-config.ts (1)
65-66: LGTM on parseHTML selectorSelector now uses the enum and default value—consistent with addAttributes.
packages/editor/src/core/extensions/callout/types.ts (2)
21-25: Confirm null handling strategy for BACKGROUNDBACKGROUND is typed as string | undefined. If the UI clears color with null (used by storage to delete), ensure call sites coerce null→undefined (see block.tsx suggestion) rather than widening this type.
1-9: Enum rename + computed-key types: good moveString-enum values keep the DOM attributes stable while strengthening typing at use sites.
packages/editor/src/core/components/menus/bubble-menu/root.tsx (1)
54-56: Destructuring props for editor is a nice cleanupKeeps subsequent references concise and consistent.
Description
This PR refactors the
tsconfigof all the react libraries by-Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores