[WEB-4790] fix: moved typescript parser to the base eslint config#7658
[WEB-4790] fix: moved typescript parser to the base eslint config#7658sriramveeraghanta merged 4 commits intopreviewfrom
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. WalkthroughProject-wide ESLint configuration was consolidated: local parser settings were removed from many apps/packages, new ignore files were added, and shared configs were updated to include TypeScript parser, plugins, and import order rules. Minor React/TS cleanups and import reorders landed, plus a Redis integration change within the live app’s extensions module. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 centralizes TypeScript ESLint parser configuration by moving the parser declaration from individual package/app ESLint configs to the base eslint-config files, eliminating duplication and ensuring consistency across the monorepo.
- Removes redundant
parser: "@typescript-eslint/parser"declarations from individual .eslintrc files - Adds the TypeScript parser to base configurations (library.js, next.js, server.js)
- Standardizes .eslintignore files with common ignore patterns across packages and apps
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/*/.eslintrc.js | Removes duplicate TypeScript parser declarations |
| apps/*/.eslintrc.js | Removes duplicate TypeScript parser declarations |
| packages/eslint-config/*.js | Adds TypeScript parser to base configs and improves formatting |
| Various .eslintignore files | Adds standard ignore patterns for build artifacts and environment files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
sriramveeraghanta
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![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
🧹 Nitpick comments (18)
packages/editor/.eslintignore (2)
1-4: Prefer directory form over trailing globs for recursive ignoresUsing "dir/" (or "dir/**") is clearer and ignores recursively; "dir/*" only matches the first level. Apply for build, dist, and out.
-node_modules -build/* -dist/* -out/* +node_modules +build/ +dist/ +out/
1-4: Optional: standardize and centralize common ignore entries
- ESLint already ignores node_modules by default; either keep it everywhere for clarity or remove for brevity, but be consistent repo-wide.
- Consider adding shared tool/build artifacts (e.g., .turbo/, coverage/) here or at the repo root .eslintignore to avoid duplication across packages.
Would you like me to propose a root-level .eslintignore that apps/packages can inherit from to DRY these patterns?
packages/constants/.eslintignore (2)
1-4: Use directory form for recursive ignoresSame rationale as other packages: switch from "*/" to "/" so nested content is ignored reliably.
-node_modules -build/* -dist/* -out/* +node_modules +build/ +dist/ +out/
1-4: Align ignore set with the rest of the repoIf other packages/apps add .turbo/ and coverage/, include them here too or centralize in a root .eslintignore to keep things consistent.
packages/ui/.eslintignore (1)
1-3: Normalize patterns and consider including common tooling dirs
- Replace "*/" with "/" for recursive ignores.
- If other workspaces list node_modules/.turbo/ explicitly, mirror that for consistency or centralize ignores.
-build/* -dist/* -out/* +build/ +dist/ +out/ +node_modules/ +.turbo/apps/web/.eslintignore (2)
4-13: Tighten directory patterns and dedupe env entries
- Prefer "dir/" over "dir/*" for recursive directory ignores.
- ".env*" already covers .env, .env.local, .env.development, .env.production, .env.test; the explicit lines are redundant.
-core/local-db/worker/wa-sqlite/src/* -dist/* -node_modules/* -.turbo/* -.env* -.env -.env.local -.env.development -.env.production -.env.test +core/local-db/worker/wa-sqlite/src/ +dist/ +node_modules/ +.turbo/ +.env*
4-13: Optional: consider additional Next.js artifactsIf this app uses Vercel, adding ".vercel/" can prevent accidental linting of deployment artifacts. Also consider "coverage/" if you generate coverage reports.
apps/admin/.eslintignore (1)
1-12: Normalize directory patterns and remove redundant env lines
- Use "dir/" instead of "dir/*" to capture nested content.
- Keep only ".env*" since it already matches all the explicit env files listed.
-.next/* -out/* -public/* -dist/* -node_modules/* -.turbo/* -.env* -.env -.env.local -.env.development -.env.production -.env.test +.next/ +out/ +public/ +dist/ +node_modules/ +.turbo/ +.env*apps/space/.eslintignore (1)
7-12: Deduplicate .env entries; .env already covers them*Keeping both
.env*and the explicit.env.*lines is redundant.Apply this minimal diff:
.turbo/* .env* -.env -.env.local -.env.development -.env.production -.env.testOptional: consider also ignoring common folders like
coverage/or.vercel/if they exist in this app.packages/eslint-config/library.js (5)
7-8: Put "prettier" last in extends to ensure it wins; parser centralization looks rightPrettier should be last so it can disable conflicting rules from prior configs. The new central parser is good.
-module.exports = { - extends: ["prettier", "plugin:@typescript-eslint/recommended"], +module.exports = { + extends: ["plugin:@typescript-eslint/recommended", "prettier"], parser: "@typescript-eslint/parser",
10-13: Mark React/JSX globals as readonly, not writableUsing
truemakes them writable. Prefer"readonly"to avoid accidental assignments.- globals: { - React: true, - JSX: true, - }, + globals: { + React: "readonly", + JSX: "readonly", + },
19-24: Monorepo resolver reliability: ensure the TS project resolution works from repo root
projectis computed fromprocess.cwd(). If ESLint runs from the monorepo root, packages may not resolve correctly. Two safer options:Option A (auto-detect):
- typescript: { - project, - }, + typescript: { + project: true, + },Option B (explicit globs):
-const project = resolve(process.cwd(), "tsconfig.json"); +const project = [ + "tsconfig.json", + "apps/*/tsconfig.json", + "packages/*/tsconfig.json", +].map((p) => resolve(process.cwd(), p));and keep
projectas-is below.Please verify your CI runs ESLint from a working directory that makes the current approach resolve each package’s tsconfig correctly.
55-77: Tidy import/order: remove non-existent "react" type from exclusions
pathGroupsExcludedImportTypesexpects import types (e.g., builtin, external, internal, parent...). "react" isn’t a valid type and is ignored.- pathGroupsExcludedImportTypes: ["builtin", "internal", "react"], + pathGroupsExcludedImportTypes: ["builtin", "internal"],
33-35: Avoid false positives: turn off no-undef for TS files onlyCore
no-undefcan misfire on types. Keep it for JS, but disable for TS via an override.Add this override near the end of the config (before
ignorePatterns):rules: { "no-useless-escape": "off", @@ }, + overrides: [ + { + files: ["**/*.ts", "**/*.tsx"], + rules: { + "no-undef": "off", + }, + }, + ], ignorePatterns: [".*.js", "node_modules/", "dist/"],packages/eslint-config/next.js (3)
5-7: Put "prettier" last; central parser looks goodEnsure Prettier is last so it can disable conflicting rules.
-module.exports = { - extends: ["next", "prettier", "plugin:@typescript-eslint/recommended"], +module.exports = { + extends: ["next", "plugin:@typescript-eslint/recommended", "prettier"], parser: "@typescript-eslint/parser", plugins: ["react", "@typescript-eslint", "import"],
97-103: Remove "react" from excluded import typesIt’s not a valid import type and is ignored by the rule.
- pathGroupsExcludedImportTypes: ["builtin", "internal", "react"], + pathGroupsExcludedImportTypes: ["builtin", "internal"],
32-34: Consider disabling no-undef for TS files onlySame rationale as the library config to avoid false positives on types.
Example override (place after
rules):rules: { @@ }, + overrides: [ + { + files: ["**/*.ts", "**/*.tsx"], + rules: { + "no-undef": "off", + }, + }, + ],packages/eslint-config/server.js (1)
29-33: no-unused-vars patterns look good; consider enabling ignoreRestSiblings to reduce noise.Common when stripping props via object rest: avoids false positives without weakening the rule.
{ argsIgnorePattern: "^_", varsIgnorePattern: "^_", caughtErrorsIgnorePattern: "^_", + ignoreRestSiblings: 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 selected for processing (25)
apps/admin/.eslintignore(1 hunks)apps/admin/.eslintrc.js(0 hunks)apps/live/.eslintrc.json(1 hunks)apps/space/.eslintignore(1 hunks)apps/space/.eslintrc.js(0 hunks)apps/web/.eslintignore(1 hunks)apps/web/.eslintrc.js(0 hunks)packages/constants/.eslintignore(1 hunks)packages/constants/.eslintrc.js(1 hunks)packages/decorators/.eslintrc.js(0 hunks)packages/editor/.eslintignore(1 hunks)packages/editor/.eslintrc.js(0 hunks)packages/eslint-config/library.js(2 hunks)packages/eslint-config/next.js(2 hunks)packages/eslint-config/server.js(2 hunks)packages/hooks/.eslintrc.js(0 hunks)packages/i18n/.eslintrc.js(0 hunks)packages/logger/.eslintrc.js(0 hunks)packages/propel/.eslintrc.js(0 hunks)packages/services/.eslintrc.js(0 hunks)packages/shared-state/.eslintrc.js(0 hunks)packages/types/.eslintrc.js(0 hunks)packages/ui/.eslintignore(1 hunks)packages/ui/.eslintrc.js(0 hunks)packages/utils/.eslintrc.js(0 hunks)
💤 Files with no reviewable changes (14)
- packages/shared-state/.eslintrc.js
- packages/decorators/.eslintrc.js
- packages/types/.eslintrc.js
- apps/web/.eslintrc.js
- packages/utils/.eslintrc.js
- packages/services/.eslintrc.js
- packages/hooks/.eslintrc.js
- apps/admin/.eslintrc.js
- packages/propel/.eslintrc.js
- packages/i18n/.eslintrc.js
- packages/logger/.eslintrc.js
- packages/editor/.eslintrc.js
- packages/ui/.eslintrc.js
- apps/space/.eslintrc.js
⏰ 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 (7)
apps/admin/.eslintignore (1)
1-12: No lingering parser declarations found – parser centralization verified.
- Confirmed that no workspace-level
.eslintrcfiles outside ofpackages/eslint-configstill declareparser: "@typescript-eslint/parser".- Verified there are no
parserOptions.projectentries in any local ESLint configs that could point to outdatedtsconfigfiles.All TypeScript parsing is correctly centralized in the shared ESLint config. The change can be approved.
apps/space/.eslintignore (1)
1-6: LGTM: solid baseline ignores for build artifactsThe patterns for .next, out, public, dist, node_modules, and .turbo look good for a Next.js app.
packages/eslint-config/library.js (2)
45-51: No behavior change in no-unused-vars optionsUnquoting the option keys is fine; values are unchanged.
80-81: Double-check global ignorePatterns scopeIgnoring dot-JS files globally can hide lint issues in dotfile configs (e.g.,
.storybook/*.js) if any consumer wants to lint them. If that’s intentional, ignore this; otherwise consider scoping this ignore at app level.packages/constants/.eslintrc.js (1)
1-5: Parser configuration verifiedAll consumer ESLint configs no longer declare their own parser, and each shared config (
packages/eslint-config/library.js,next.js,server.js) explicitly definesparser: "@typescript-eslint/parser". The removal of local parser settings is safe and requires no further changes.packages/eslint-config/next.js (1)
43-50: No behavior change in no-unused-vars optionsUnquoted option keys are fine.
apps/live/.eslintrc.json (1)
3-3: TS parser correctly centralized in server.jsThe
packages/eslint-config/server.jsfile explicitly definesparser: "@typescript-eslint/parser"and includes the appropriateimport/resolversettings, and none of the individual.eslintrc.*files inapps/orpackages/override the parser. Consumers extending@plane/eslint-config/server.jswill correctly inherit the TypeScript parser. No further changes required.
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 (3)
packages/hooks/src/use-local-storage.tsx (1)
4-4: Fix SSR guard: correct invalidtypeofcomparison across the repo
typeofalways returns a string, so comparing it to the bareundefinedis both incorrect (and will be flagged by ESLint’svalid-typeofrule) and redundant when you already compare to the string"undefined". Simplify each guard to a single string check.Please apply these diffs in all listed locations:
packages/hooks/src/use-local-storage.tsx
- if (typeof window === undefined || typeof window === "undefined") return defaultValue; + if (typeof window === "undefined") return defaultValue; - if (typeof window === undefined || typeof window === "undefined") return false; + if (typeof window === "undefined") return false;apps/web/core/hooks/use-local-storage.tsx
- if (typeof window === undefined || typeof window === "undefined") return defaultValue; + if (typeof window === "undefined") return defaultValue; - if (typeof window === undefined || typeof window === "undefined") return false; + if (typeof window === "undefined") return false;apps/web/core/lib/local-storage.ts
- if (typeof window === undefined || typeof window === "undefined" || !key || !value) return undefined; + if (typeof window === "undefined" || !key || !value) return undefined; - if (typeof window === undefined || typeof window === "undefined") return undefined; + if (typeof window === "undefined") return undefined; - if (typeof window === undefined || typeof window === "undefined" || !key) return undefined; + if (typeof window === "undefined" || !key) return undefined;To ensure no other occurrences remain, you can rerun:
#!/bin/bash rg -nP 'typeof\s+\w+\s*===\s*undefined' -g '!**/node_modules/**'apps/live/src/core/extensions/index.ts (2)
32-49: Promise never settles on error; remove async executor anti-pattern infetchWrapping an
asyncbody innew Promise(async …)and only callingresolveresults in a hung promise on exceptions. This can stall the document load indefinitely. Return/throw directly from anasyncfunction.- // TODO: Fix this lint error. - // eslint-disable-next-line no-async-promise-executor - return new Promise(async (resolve) => { - try { - let fetchedData = null; - if (documentType === "project_page") { - fetchedData = await fetchPageDescriptionBinary(params, pageId, cookie); - } else { - fetchedData = await fetchDocument({ - cookie, - documentType, - pageId, - params, - }); - } - resolve(fetchedData); - } catch (error) { - manualLogger.error("Error in fetching document", error); - } - }); + try { + if (documentType === "project_page") { + return await fetchPageDescriptionBinary(params, pageId, cookie); + } + return await fetchDocument({ + cookie, + documentType, + pageId, + params, + }); + } catch (error) { + manualLogger.error("Error in fetching document", error); + throw error; + }
59-76: Same issue instore: promise never resolves/rejects; let the async function resolve naturallyIf an exception occurs, the promise stays pending and the server may assume the store is still in-flight. Let the function complete or propagate the error so the caller can react.
- // TODO: Fix this lint error. - // eslint-disable-next-line no-async-promise-executor - return new Promise(async () => { - try { - if (documentType === "project_page") { - await updatePageDescription(params, pageId, state, cookie); - } else { - await updateDocument({ - cookie, - documentType, - pageId, - params, - updatedDescription: state, - }); - } - } catch (error) { - manualLogger.error("Error in updating document:", error); - } - }); + try { + if (documentType === "project_page") { + await updatePageDescription(params, pageId, state, cookie); + } else { + await updateDocument({ + cookie, + documentType, + pageId, + params, + updatedDescription: state, + }); + } + } catch (error) { + manualLogger.error("Error in updating document:", error); + throw error; + }
🧹 Nitpick comments (25)
packages/hooks/src/use-local-storage.tsx (4)
27-34: Route writes through the helper to handle SSR and quota errors uniformly
setValuedirectly useswindow.localStoragewithout the guard/try-catch you already encapsulated insetValueIntoLocalStorage. Reusing it keeps behavior consistent and resilient (SSR-safe and handles quota/serialization failures).Apply:
const setValue = useCallback( (value: T) => { - window.localStorage.setItem(key, JSON.stringify(value)); - setStoredValue(value); - window.dispatchEvent(new Event(`local-storage:${key}`)); + if (setValueIntoLocalStorage(key, value)) { + setStoredValue(value); + window.dispatchEvent(new Event(`local-storage:${key}`)); + } }, [key] );
36-41: Add SSR guard to clearValue to prevent ReferenceError during SSRWhile this hook is typically used client-side, guarding matches the pattern above and avoids surprises if invoked pre-mount.
Apply:
const clearValue = useCallback(() => { - window.localStorage.removeItem(key); - setStoredValue(null); - window.dispatchEvent(new Event(`local-storage:${key}`)); + if (typeof window !== "undefined") { + window.localStorage.removeItem(key); + setStoredValue(null); + window.dispatchEvent(new Event(`local-storage:${key}`)); + } }, [key]);
47-52: Sync state across browser tabs with the native "storage" event (optional)You already dispatch a custom event for in-tab updates. Listening to
"storage"keeps multiple tabs in sync when the same key changes elsewhere.Apply:
useEffect(() => { - window.addEventListener(`local-storage:${key}`, reHydrate); - return () => { - window.removeEventListener(`local-storage:${key}`, reHydrate); - }; + const onCustom = () => reHydrate(); + const onStorage = (e: StorageEvent) => { + if (!e.key || e.key === key) reHydrate(); + }; + window.addEventListener(`local-storage:${key}`, onCustom); + window.addEventListener("storage", onStorage); + return () => { + window.removeEventListener(`local-storage:${key}`, onCustom); + window.removeEventListener("storage", onStorage); + }; }, [key, reHydrate]);
24-26: Optional: consider returning T instead of T | null for a simpler consumer APIToday
clearValue()setsstoredValuetonulleven though reads default toinitialValuewhen absent. If consumers rarely need nullability, you can simplify by keepingstoredValueasTand settinginitialValueon clear. If you like this direction, I can draft a minimal diff.apps/admin/app/(all)/(dashboard)/email/page.tsx (2)
32-38: Good: catch param renamed to underscore without exposing error details to usersKeeping the toast generic is the right UX. If you have centralized logging (Sentry/etc.), consider capturing
_errorthere to aid debugging.Example:
} catch (_error) { // log/capture error for diagnostics // captureException?.(_error); setToast({ /* ... */ }); }
21-24: Guard against re-entrancy while submitting (optional)The toggle is disabled during submit, but adding a quick check avoids accidental programmatic calls.
Apply:
const handleToggle = async () => { + if (isSubmitting) return; if (isSMTPEnabled) { setIsSubmitting(true);packages/ui/src/calendar.tsx (2)
21-21: Remove redundant weekStartsOn prop; it’s already included in ...props.You pass
weekStartsOnexplicitly and then again via spread. Drop the explicit prop to avoid duplication.- weekStartsOn={props.weekStartsOn}
15-15: Variable name doesn’t match the value (Dec 31 vs “FirstDay”).
thirtyYearsFromNowFirstDayis actually December 31. Rename for clarity and update the usage.- const thirtyYearsFromNowFirstDay = new Date(currentYear + 30, 11, 31); + const thirtyYearsFromNowLastDay = new Date(currentYear + 30, 11, 31); @@ - endMonth={thirtyYearsFromNowFirstDay} + endMonth={thirtyYearsFromNowLastDay}Also applies to: 81-81
apps/live/src/core/lib/authentication.ts (1)
24-24: Usenew Errorfor consistency with other throwsElsewhere in this codebase (and even in this file’s caller),
new Erroris used. Align here too.- throw Error("Authentication failed: Token doesn't match the current user."); + throw new Error("Authentication failed: Token doesn't match the current user.");apps/live/src/core/hocuspocus-server.ts (2)
35-36: Prefer centralized logger overconsole.errorUse
manualLoggerfor consistency and to keep logs flowing through the same sink.- console.error("Token parsing failed, using request headers:", error); + manualLogger.error("Token parsing failed, using request headers:", error);Add this import at the top with the other core imports:
import { manualLogger } from "@/core/helpers/logger.js";
56-56: Minor: thrownew Error(match the style used above at Line 44)- throw Error("Authentication unsuccessful!"); + throw new Error("Authentication unsuccessful!");apps/admin/core/components/authentication/email-config-switch.tsx (2)
28-30: Simplify toggle logic and avoid parseInt; compare string flags directlyComparing against the string literal is clearer and avoids numeric parsing edge cases. Also drop the redundant
=== true.Apply within this hunk:
- const newEnableMagicLogin = Boolean(parseInt(enableMagicLogin)) === true ? "0" : "1"; + const newEnableMagicLogin = enableMagicLogin === "1" ? "0" : "1";Optionally (outside this hunk) also simplify the value prop:
// before value={Boolean(parseInt(enableMagicLogin))} // after value={enableMagicLogin === "1"}
28-30: Consider centralizing the "0"/"1" toggle pattern to a small helperThis string-flag toggle appears across multiple auth components. A tiny util like
toggle01(value: string): "0" | "1"improves consistency and reduces repetition.Example helper (outside this hunk):
export const toggle01 = (v: string) => (v === "1" ? "0" : "1");Then in the handler:
- const newEnableMagicLogin = enableMagicLogin === "1" ? "0" : "1"; + const newEnableMagicLogin = toggle01(enableMagicLogin);apps/admin/core/components/authentication/google-config.tsx (3)
38-40: Prefer direct string comparison over parseInt in togglesRemoves unnecessary parsing and redundant
=== true.- const newEnableGoogleConfig = Boolean(parseInt(enableGoogleConfig)) === true ? "0" : "1"; + const newEnableGoogleConfig = enableGoogleConfig === "1" ? "0" : "1";Optional (outside this hunk) align the value prop:
value={enableGoogleConfig === "1"}
38-40: Extract a shared 0/1 toggle helper to reduce duplication across auth switchesSame pattern exists in GitHub, GitLab, Email codes, and Password toggles.
- const newEnableGoogleConfig = enableGoogleConfig === "1" ? "0" : "1"; + const newEnableGoogleConfig = toggle01(enableGoogleConfig);Where
toggle01is the small helper suggested in a prior comment.
38-40: Leverage ToggleSwitch’s boolean onChange for direct mappingYou can simplify the Google config toggle by using the
checked: booleanparameter thatToggleSwitchalready provides, instead of manually parsing and re-stringifying the config value.• File:
apps/admin/core/components/authentication/google-config.tsx
Lines: ~38–40Suggested diff:
- const newEnableGoogleConfig = Boolean(parseInt(enableGoogleConfig)) === true ? "0" : "1"; - updateConfig("IS_GOOGLE_ENABLED", newEnableGoogleConfig); + <ToggleSwitch + value={enableGoogleConfig === "1"} + onChange={(checked: boolean) => updateConfig("IS_GOOGLE_ENABLED", checked ? "1" : "0")} + /* ...other props... */ + />This change:
- Removes unnecessary
parseInt+Booleanconversions.- Directly maps the
checkedboolean to"1"/"0".apps/admin/core/components/authentication/gitlab-config.tsx (3)
38-40: Use string comparison to toggle instead of parseInt and redundant=== trueKeeps intent explicit and avoids numeric parsing.
- const newEnableGitlabConfig = Boolean(parseInt(enableGitlabConfig)) === true ? "0" : "1"; + const newEnableGitlabConfig = enableGitlabConfig === "1" ? "0" : "1";Optional (outside this hunk):
value={enableGitlabConfig === "1"}
38-40: Consider reusing a tinytoggle01helper across all auth togglesPrevents repeating the same conditional everywhere.
- const newEnableGitlabConfig = enableGitlabConfig === "1" ? "0" : "1"; + const newEnableGitlabConfig = toggle01(enableGitlabConfig);
38-40: Leverage ToggleSwitch’s onChange signature to simplify config update
- File:
apps/admin/core/components/authentication/gitlab-config.tsx(around lines 38–40)Replace the current handler, which parses the previous state, with a direct use of the
checkedboolean thatToggleSwitchprovides:- onChange={() => { - const newEnableGitlabConfig = Boolean(parseInt(enableGitlabConfig)) === true ? "0" : "1"; - updateConfig("IS_GITLAB_ENABLED", newEnableGitlabConfig); - }} + onChange={(checked: boolean) => + updateConfig("IS_GITLAB_ENABLED", checked ? "1" : "0") + }This removes the need to parse the existing string value, computing the new config directly from the boolean you already receive.
apps/admin/core/components/authentication/github-config.tsx (3)
38-40: Tighten logic: drop parseInt and redundant comparisonString flag comparison is clearer.
- const newEnableGithubConfig = Boolean(parseInt(enableGithubConfig)) === true ? "0" : "1"; + const newEnableGithubConfig = enableGithubConfig === "1" ? "0" : "1";Optional (outside this hunk):
value={enableGithubConfig === "1"}
38-40: De-duplicate with a sharedtoggle01helperSame micro-pattern across all four providers and password/email codes.
- const newEnableGithubConfig = enableGithubConfig === "1" ? "0" : "1"; + const newEnableGithubConfig = toggle01(enableGithubConfig);
38-40: SimplifyonChangehandler by using ToggleSwitch’s boolean parameterToggleSwitch’s API defines
onChange: (value: boolean) => void(see packages/ui/src/button/toggle-switch.tsx), so your handler can receive the new boolean directly instead of re-readingenableGithubConfig.Changes needed in apps/admin/core/components/authentication/github-config.tsx:
- Remove manual
Boolean(parseInt(enableGithubConfig))checks.- Accept the boolean argument (
checked) in youronChange.- Convert
checkedto"1"or"0"when callingupdateConfig.Suggested diff:
--- a/apps/admin/core/components/authentication/github-config.tsx +++ b/apps/admin/core/components/authentication/github-config.tsx @@ -36,8 +36,7 @@ export const GithubConfigSwitch = () => { <ToggleSwitch value={Boolean(parseInt(enableGithubConfig))} - onChange={() => { - const newEnableGithubConfig = Boolean(parseInt(enableGithubConfig)) === true ? "0" : "1"; - updateConfig("IS_GITHUB_ENABLED", newEnableGithubConfig); - }} + onChange={(checked: boolean) => updateConfig("IS_GITHUB_ENABLED", checked ? "1" : "0")} size="sm" />This removes unnecessary state probing and makes the handler clearer.
apps/admin/core/components/authentication/password-config-switch.tsx (3)
28-30: Prefer direct string comparison and remove redundant=== trueAvoids unnecessary numeric parsing and clarifies intent.
- const newEnableEmailPassword = Boolean(parseInt(enableEmailPassword)) === true ? "0" : "1"; + const newEnableEmailPassword = enableEmailPassword === "1" ? "0" : "1";Optional (outside this hunk):
value={enableEmailPassword === "1"}
28-30: Optional: centralize the flip logic via a helperKeeps all toggles uniform and easier to unit test.
- const newEnableEmailPassword = enableEmailPassword === "1" ? "0" : "1"; + const newEnableEmailPassword = toggle01(enableEmailPassword);
28-30: Simplify ToggleSwitch onChange handlerToggleSwitch’s
onChangecallback already receives the new boolean state, so you can drop the manual parsing/toggling logic and derive the config value directly.• File:
apps/admin/core/components/authentication/password-config-switch.tsx(around lines 28–30)
• Signature inpackages/ui/src/button/toggle-switch.tsx:interface IToggleSwitchProps { value: boolean; onChange: (value: boolean) => void; … }Suggested change:
- onChange={() => { - const newEnableEmailPassword = Boolean(parseInt(enableEmailPassword)) === true ? "0" : "1"; - updateConfig("ENABLE_EMAIL_PASSWORD", newEnableEmailPassword); - }} + onChange={(checked: boolean) => + updateConfig("ENABLE_EMAIL_PASSWORD", checked ? "1" : "0") + }This removes the extra parsing/Boolean conversion and makes the intent clearer.
📜 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 (14)
apps/admin/app/(all)/(dashboard)/email/page.tsx(2 hunks)apps/admin/core/components/authentication/email-config-switch.tsx(1 hunks)apps/admin/core/components/authentication/github-config.tsx(1 hunks)apps/admin/core/components/authentication/gitlab-config.tsx(1 hunks)apps/admin/core/components/authentication/google-config.tsx(1 hunks)apps/admin/core/components/authentication/password-config-switch.tsx(1 hunks)apps/admin/core/store/instance.store.ts(1 hunks)apps/live/src/core/extensions/index.ts(1 hunks)apps/live/src/core/hocuspocus-server.ts(1 hunks)apps/live/src/core/lib/authentication.ts(1 hunks)apps/live/src/server.ts(1 hunks)packages/hooks/src/use-local-storage.tsx(2 hunks)packages/ui/src/auth-form/auth-form.tsx(1 hunks)packages/ui/src/calendar.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/admin/core/store/instance.store.ts
- apps/live/src/server.ts
- packages/ui/src/auth-form/auth-form.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-14T11:22:43.964Z
Learnt from: gakshita
PR: makeplane/plane#7393
File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104
Timestamp: 2025-07-14T11:22:43.964Z
Learning: In the Plane project's SMTP configuration implementation, the email configuration form (email-config-form.tsx) hardcodes ENABLE_SMTP to "1" in form submission because the form is only rendered when SMTP is enabled. The enable/disable functionality is managed at the page level (page.tsx) with a toggle, and the form only handles configuration details when SMTP is already enabled.
Applied to files:
apps/admin/core/components/authentication/email-config-switch.tsxapps/admin/core/components/authentication/password-config-switch.tsx
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
packages/hooks/src/use-local-storage.tsx (2)
8-11: Catch param rename is correct and consistent with linting conventionsUsing
_errorcommunicates intentional non-use and silencesno-unused-vars. No functional changes introduced.
19-21: Same here — good unused error handlingConsistent with the pattern across the PR; behavior remains unchanged.
apps/admin/app/(all)/(dashboard)/email/page.tsx (1)
12-12: Confirm React.FC usage for MobX observer componentsBased on a quick scan of the codebase, we currently have:
- 592 components annotated with
React.FC- 235 components wrapped in
observer(...)- Only 54 observer-wrapped components explicitly annotated as
React.FCThis indicates a mixed pattern: most observer components rely on type inference, while a subset use the
React.FCannotation. If consistency is a goal for this PR, please confirm which convention we should follow:
- Adopt
React.FCfor all observer-wrapped components- Avoid
React.FCon observer components and rely on inference- Allow both patterns but document the rationale in our style guide
Once decided, we can either update this page.tsx or align the rest of the codebase in a follow-up.
apps/live/src/core/lib/authentication.ts (1)
3-4: LGTM: import reorg is clean and consistent with ESM/.js pathsNo runtime impact. Keeping services under a dedicated header improves scan-ability.
apps/live/src/core/hocuspocus-server.ts (1)
5-9: LGTM: import restructuring aligns editor events, extensions, and lib importsCentralizing editor event types and moving core imports here reads better; no behavioral change.
apps/admin/core/components/authentication/email-config-switch.tsx (2)
28-30: LGTM on consolidating the toggle update into a single updateConfig callThis refactor removes duplicated calls and keeps the handler concise. Behavior remains unchanged.
28-30: Please confirm theToggleSwitchonChange signatureIt looks like the
rgsearch didn’t locate theToggleSwitchdeclaration (likely because it lives in@plane/ui’s distributed types). Could you manually inspect the type definitions in yournode_modules/@plane/uipackage (for example, indist/index.d.tsor whereverToggleSwitchis exported) and verify whether theonChangeprop is declared as(checked: boolean) => void?If it is, you can simplify your handler as follows:
- onChange={() => { - const newEnableMagicLogin = Boolean(parseInt(enableMagicLogin)) === true ? "0" : "1"; - updateConfig("ENABLE_MAGIC_LINK_LOGIN", newEnableMagicLogin); - }} + onChange={(checked: boolean) => + updateConfig("ENABLE_MAGIC_LINK_LOGIN", checked ? "1" : "0") + }apps/admin/core/components/authentication/google-config.tsx (1)
38-40: LGTM on reducing duplicate updateConfig callsOne computation + one update keeps the handler tidy; behavior is preserved.
apps/admin/core/components/authentication/gitlab-config.tsx (1)
38-40: LGTM; single-source-of-truth for the new value is clearerNice reduction from two branches to one computed value + one update.
apps/admin/core/components/authentication/github-config.tsx (1)
38-40: LGTM; handler now does a single, deterministic updateRefactor improves readability and avoids duplicate
updateConfigcalls.apps/admin/core/components/authentication/password-config-switch.tsx (1)
28-30: LGTM; single update path is cleanerKeeps the toggle handler straightforward and consistent with other auth configs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (14)
apps/live/.eslintrc.js(1 hunks)apps/live/.eslintrc.json(0 hunks)apps/space/.eslintrc.js(0 hunks)apps/web/.eslintrc.js(0 hunks)packages/constants/.eslintrc.js(0 hunks)packages/decorators/.eslintrc.js(0 hunks)packages/hooks/.eslintrc.js(0 hunks)packages/i18n/.eslintrc.js(0 hunks)packages/logger/.eslintrc.js(0 hunks)packages/propel/.eslintrc.js(0 hunks)packages/services/.eslintrc.js(0 hunks)packages/shared-state/.eslintrc.js(0 hunks)packages/ui/.eslintrc.js(0 hunks)packages/utils/.eslintrc.js(0 hunks)
💤 Files with no reviewable changes (13)
- packages/ui/.eslintrc.js
- packages/services/.eslintrc.js
- packages/shared-state/.eslintrc.js
- packages/decorators/.eslintrc.js
- apps/web/.eslintrc.js
- packages/hooks/.eslintrc.js
- packages/logger/.eslintrc.js
- packages/constants/.eslintrc.js
- packages/propel/.eslintrc.js
- apps/live/.eslintrc.json
- packages/utils/.eslintrc.js
- apps/space/.eslintrc.js
- packages/i18n/.eslintrc.js
⏰ 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 (2)
apps/live/.eslintrc.js (2)
1-4: LGTM — minimal config correctly delegates to shared server rules.
Root flag + single extend is aligned with the PR objective of centralizing the TS parser in the base config.
3-3: I've added a script to locate and inspect the@plane/eslint-configpackage’spackage.jsonso we can verify if it exports aserverentrypoint without the.jssuffix. Once we review that export configuration, we can confirm whether dropping the.jsis safe or not.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/live/.eslintrc.cjs (1)
3-3: Prefer shareable-config subpath without file extensionReferencing the package subpath without “.js” reduces coupling to the package’s internal file layout and export map.
- extends: ["@plane/eslint-config/server.js"], + extends: ["@plane/eslint-config/server"],
📜 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 (1)
apps/live/.eslintrc.cjs(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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/live/.eslintrc.cjs (2)
1-4: Centralizing the TS parser via shared config looks goodStraightforward switch to the shared server config; aligns with the PR goal to remove local parser settings. Using .cjs and root: true here is consistent with classic ESLint configs.
1-4: Parser Inheritance ConfirmedThe shared ESLint config in
packages/eslint-config(server.js, library.js, next.js) explicitly declaresparser: "@typescript-eslint/parser", and there are no localparseroverrides inapps/live/.eslintrc.cjs. The TypeScript parser will be correctly applied.
Description
Type of Change
Summary by CodeRabbit
New Features
Refactor
Style
Chores