update: enable linting and formatting via pre-commit hooks #31
Conversation
note: releasing first creates the tag via release flow (not an annotated tag, tagged on older commit)
|
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. WalkthroughAdds linting/formatting infrastructure (Prettier, ESLint, lint-staged), Git hooks, CI formatting workflow, consolidated TypeScript configs, dependency updates (Lit → 3.3.1, TypeScript tooling), and type-safety/type-signature tweaks in formula-editor packages and a queue method. Changes
Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant GitHub
participant FormatterWorkflow as Format Workflow
participant Repo as Git Repo
Reviewer->>GitHub: Submit PR review (approved)
GitHub->>FormatterWorkflow: Trigger workflow (review:approved)
FormatterWorkflow->>FormatterWorkflow: Inspect last commit author/message
alt skip due to bot + "[skip ci]"
FormatterWorkflow-->>GitHub: Set run_format=false, exit
else proceed
FormatterWorkflow->>Repo: Checkout PR branch (full history)
FormatterWorkflow->>FormatterWorkflow: npm ci
FormatterWorkflow->>FormatterWorkflow: Run lint-staged against merge-base
alt formatting produced changes
FormatterWorkflow->>Repo: Configure git user/email
FormatterWorkflow->>Repo: Commit "CI: format and lint files [skip ci]"
FormatterWorkflow->>Repo: Push changes to PR branch
else no changes
FormatterWorkflow-->>GitHub: Complete (no push)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (7)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR enables linting and formatting via pre-commit hooks across the monorepo and updates dependencies to their latest major versions. The changes standardize code quality tools and modernize the project's TypeScript configuration structure.
- Implements comprehensive linting and formatting infrastructure with ESLint, Prettier, and pre-commit hooks
- Consolidates TypeScript configurations by moving shared settings to a root-level tsconfig.json
- Updates all package dependencies to their latest major versions (TypeScript 5.9.3, Lit 3.3.1, etc.)
Reviewed Changes
Copilot reviewed 26 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tsconfig.json | Removed - consolidated into root tsconfig.json |
| packages/*/tsconfig.json | Updated to extend from root config with minimal overrides |
| packages/*/package.json | Bumped versions and updated dependencies to latest majors |
| package.json | Added linting/formatting scripts, hooks setup, and dev dependencies |
| eslint.config.js | New ESLint configuration with TypeScript and React support |
| .prettierrc.json | New Prettier configuration with import sorting |
| .hooks/* | New Git hooks for automated linting on commits |
| .github/workflows/* | Updated CI workflows for formatting and Node.js version bump |
Files not reviewed (2)
- packages/fw-theme-builder/package-lock.json: Language not supported
- packages/localize/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "extends": "../tsconfig.json", | |||
| "extends": "../../tsconfig.json", | |||
There was a problem hiding this comment.
The tsconfig.json file being extended (../../tsconfig.json) does not exist in the repository. The original packages/tsconfig.json file was deleted but no root-level tsconfig.json was created to replace it.
| "extends": "../../tsconfig.json", |
There was a problem hiding this comment.
I can see the file on root folder.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "extends": "../tsconfig.json", | |||
| "extends": "../../tsconfig.json", | |||
There was a problem hiding this comment.
The tsconfig.json file being extended (../../tsconfig.json) does not exist in the repository. The original packages/tsconfig.json file was deleted but no root-level tsconfig.json was created to replace it.
| "extends": "../../tsconfig.json", |
| @@ -1,108 +1,10 @@ | |||
| { | |||
| "extends": "../../tsconfig.json", | |||
There was a problem hiding this comment.
The tsconfig.json file being extended (../../tsconfig.json) does not exist in the repository. The original packages/tsconfig.json file was deleted but no root-level tsconfig.json was created to replace it.
| "extends": "../../tsconfig.json", |
| "exclude": ["showcase", "**/dist", "./built/**/*"], | ||
| "include": [ "src" ] | ||
| } | ||
| "extends": "../../tsconfig.json", |
There was a problem hiding this comment.
The tsconfig.json file being extended (../../tsconfig.json) does not exist in the repository. The original packages/tsconfig.json file was deleted but no root-level tsconfig.json was created to replace it.
| "extends": "../../tsconfig.json", |
| "include": ["src/**/*.ts"], | ||
| "exclude": ["node_modules", "dist"] | ||
| } | ||
| "extends": "../../tsconfig.json", |
There was a problem hiding this comment.
The tsconfig.json file being extended (../../tsconfig.json) does not exist in the repository. The original packages/tsconfig.json file was deleted but no root-level tsconfig.json was created to replace it.
| "extends": "../../tsconfig.json", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
.prettierrc.json (1)
2-2: Consider the unusually wide printWidth setting.A
printWidthof 180 is significantly wider than the typical 80-120 range. While this is a team preference, wide lines can reduce readability on standard displays and during code reviews.If you prefer a more conventional width, consider adjusting to 100-120:
{ - "printWidth": 180, + "printWidth": 120, "quoteProps": "consistent",package.json (1)
45-48: Consider the performance impact of tsc:check in lint-staged.Running
tsc:checkon every staged TypeScript file commit could be slow for large changesets, potentially causing frustrating delays during commits.Consider one of these alternatives:
- Remove type checking from pre-commit and rely on CI/pre-push hooks
- Use a faster type checker like
tsc --incrementalor limit to changed files only"*.{tsx,ts}": [ "npm run lint:fix", - "npm run format:fix", - "npm run tsc:check -- -p ./tsconfig.json" + "npm run format:fix" ],eslint.config.js (1)
37-37: Remove or uncomment the rule.This commented-out rule should either be enabled or removed entirely to keep the configuration clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/fw-theme-builder/package-lock.jsonis excluded by!**/package-lock.jsonpackages/localize/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.github/workflows/format.yml(1 hunks).github/workflows/npm-publish.yml(3 hunks).hooks/pre-commit(1 hunks).hooks/pre-push(1 hunks).prettierignore(1 hunks).prettierrc.json(1 hunks)eslint.config.js(1 hunks)lerna.json(1 hunks)package.json(1 hunks)packages/formula-editor/package.json(2 hunks)packages/formula-editor/src/formula-editor.ts(9 hunks)packages/formula-editor/src/suggestion-menu.ts(2 hunks)packages/formula-editor/src/utils/parser.ts(13 hunks)packages/formula-editor/src/utils/queue.ts(1 hunks)packages/formula-editor/tsconfig.json(1 hunks)packages/fw-avatar/package.json(2 hunks)packages/fw-dnd/package.json(2 hunks)packages/fw-theme-builder/package.json(2 hunks)packages/fw-theme-builder/tsconfig.json(1 hunks)packages/localize/package.json(2 hunks)packages/localize/tsconfig.json(1 hunks)packages/styles/package.json(2 hunks)packages/styles/tsconfig.json(1 hunks)packages/trackers/package.json(2 hunks)packages/trackers/tsconfig.json(1 hunks)packages/tsconfig.json(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/tsconfig.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-21T11:25:15.786Z
Learnt from: parasdeep03
Repo: getfundwave/fw-components PR: 17
File: packages/formula-editor/src/formula-editor.ts:105-106
Timestamp: 2025-02-21T11:25:15.786Z
Learning: The Parser in FormulaEditor should be initialized when variables change, regardless of the variables size, to ensure it's always available for parsing input.
Applied to files:
packages/formula-editor/src/formula-editor.tspackages/formula-editor/src/utils/parser.ts
📚 Learning: 2025-02-21T09:50:03.410Z
Learnt from: parasdeep03
Repo: getfundwave/fw-components PR: 17
File: showcase/formula-editor/index.js:0-0
Timestamp: 2025-02-21T09:50:03.410Z
Learning: The `handleFormulaChange` function was removed from the formula editor showcase component.
Applied to files:
packages/formula-editor/src/formula-editor.ts
🧬 Code graph analysis (3)
packages/formula-editor/src/suggestion-menu.ts (1)
packages/formula-editor/src/styles/suggestion-menu.ts (1)
SuggestionMenuStyles(3-54)
packages/formula-editor/src/formula-editor.ts (4)
packages/formula-editor/src/suggestion-menu.ts (1)
customElement(6-69)packages/formula-editor/src/utils/parser.ts (1)
Parser(12-389)packages/formula-editor/src/utils/get-formula-tokens.ts (1)
getFormulaTokens(1-7)packages/formula-editor/src/styles/editor.ts (1)
FormulaEditorStyles(3-42)
packages/formula-editor/src/utils/parser.ts (4)
packages/formula-editor/src/utils/recommendor.ts (1)
Recommender(3-21)packages/formula-editor/src/utils/constants.ts (3)
mathematicalOperators(1-1)unaryOperators(3-3)operatorPrecedence(5-11)packages/formula-editor/src/types/index.ts (1)
ParseResult(20-25)packages/formula-editor/src/utils/get-formula-tokens.ts (1)
getFormulaTokens(1-7)
🪛 actionlint (1.7.8)
.github/workflows/format.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
33-33: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/npm-publish.yml
25-25: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (32)
packages/trackers/package.json (1)
3-3: LGTM!The version bump and TypeScript upgrade from 5.2.2 to 5.9.3 are consistent with the monorepo-wide dependency update strategy.
Also applies to: 21-21
.github/workflows/npm-publish.yml (1)
68-69: Verify intent to bypass git hooks in the publish workflow.The
--no-verifyflag bypasses the pre-push hooks that were added in this PR. While this may be intentional for the automated publish workflow to prevent interference with CI/CD, please confirm this is the desired behavior.If git hooks should run during automated publishing, remove the flag:
- run: git push --follow-tags --atomic --no-verify + run: git push --follow-tags --atomic.prettierignore (1)
1-10: LGTM!The ignore patterns appropriately exclude configuration files, shell scripts, environment files, and binary assets from Prettier formatting.
packages/styles/package.json (1)
16-16: No breaking changes detected with Lit 3.3.1 upgrade—review comment can be resolved.The styles package uses only the
htmltagged template literal from Lit, which remains stable across versions 2.x and 3.x. The search found no usage of decorators, LitElement components, or other Lit 2.x APIs that were changed or removed in Lit 3.0. The TypeScript 5.9.3 upgrade is also compatible with Lit 3.3.1.packages/fw-dnd/package.json (1)
14-21: No compatibility issues found—code already follows Lit 3 best practices.The fw-dnd-crud.js component properly handles Polymer 3 component interoperability with Lit 3:
- Event handling: Uses explicit
@value-changedhandlers and@taplisteners instead of two-way binding (the correct pattern for Lit 3).- State mutations: Applies immutable patterns (
[...array],[].concat(list)) for triggering reactivity.- Module imports: Uses correct ES module syntax (
import { LitElement, html } from "lit"; Polymer components imported as side-effects).The Polymer 3.x packages (
@polymer/iron-icons,@polymer/paper-*) are compatible with Lit 3.3.1 when used as shown here. No breaking changes or compatibility concerns are present.packages/localize/package.json (1)
19-26: TypeScript 5.9.3 upgrade is compatible; minimum requirement is met.Lit requires TypeScript >=5.2 for Lit 3 decorators, and TypeScript 5.9.3 exceeds this requirement. No compatibility issues are reported. The upgrade can proceed.
lerna.json (1)
2-3: Lerna still assumes all packages changed — root cause is missing git tags, not lerna.json configuration.Verification confirmed the problem persists. Lerna reports "Assuming all packages changed" when it can't find the last release/commit tags or full git history. This repository has no git tags (
git tag -lreturned nothing), so lerna has no baseline to compare against.The PR removes the deprecated
useWorkspacesflag (correct per lerna v7+) and updates the version, but doesn't address the root cause: missing git tag infrastructure. Lerna now reads workspaces from your package manager by default, so the config change alone won't fix change detection.To actually resolve issue #21, ensure git tags are created and pushed during release (e.g., via
lerna versionwith--create-releaseor equivalent CI workflow), or configure--sinceflags for change detection if tags aren't available..prettierrc.json (1)
8-19: Well-structured import ordering configuration.The import order groups are comprehensive and logically organized, which will ensure consistent import organization across the codebase. The separation between React, constants, contexts, API clients, project code, third-party modules, and utilities is clear and maintainable.
.github/workflows/format.yml (2)
19-26: Effective infinite loop prevention.The bot detection logic properly prevents the workflow from triggering recursively by checking if the last commit author contains "bot" and the message contains "[skip ci]". This is a solid safeguard against infinite formatting loops.
55-56: Review the use of --no-verify flags.Both
git commitandgit pushuse--no-verifyflags, which bypass git hooks and any branch protection rules that might be in place. While this may be intentional for CI automation, ensure this doesn't circumvent required checks.Confirm that bypassing hooks and checks during CI formatting commits aligns with your repository's governance policies, especially if you have required status checks or branch protection rules.
packages/fw-avatar/package.json (2)
31-34: Approve testing library updates.The updates to testing libraries (@open-wc/testing, @web/test-runner, @web/test-runner-playwright) are appropriate and will provide better testing capabilities with modern tooling.
26-26: No compatibility issues detected with Lit 3.3.1 upgrade.Both fw-avatar components use standard, non-deprecated patterns: static get properties(), the updated() lifecycle method, and proper directive imports. The codebase contains no deprecated Lit 2.x APIs, no SSR hydration code requiring migration, and no decorator-related breaking changes. The build target (ES2022) exceeds Lit 3's ES2021 minimum requirement. The upgrade is compatible.
package.json (3)
19-20: Good practice: automated git hooks setup.The
preparescript that runssetup-git-hooksensures that all developers automatically get the git hooks configured when they runnpm install. This is a solid approach for maintaining consistency across the team.
23-39: Comprehensive linting and formatting tooling.The devDependencies include a well-rounded set of ESLint, Prettier, and TypeScript tooling that will enforce consistent code quality across the monorepo. The combination of ESLint plugins for React, JSDoc, and TypeScript provides thorough coverage.
6-6: Change to"type": "module"is safe and fully compatible with existing codebase.Verification confirms all packages and scripts are already using ES module syntax. No CommonJS patterns (require, module.exports, __dirname, __filename) were found across the monorepo. Packages inherit the root configuration, and those with explicit
"type": "module"are already properly configured. No.cjsfiles exist and no compatibility issues will arise from this change..hooks/pre-commit (1)
5-5: Proper merge detection.The MERGE_HEAD check correctly bypasses the hook during merge commits, preventing lint-staged from running on potentially conflicted or incomplete merge states.
packages/formula-editor/package.json (1)
22-24: No code changes required. All dependencies are compatible with existing usage patterns.Based on verification of the dependency changes:
big.js maintains a stable constructor API across versions, and the code's usage of
Big()constructor with chained methods (.add(),.sub(),.mul(),.div(),.pow(),.eq()) is fully compatible.match-sorter v8.0.0 reverted the previous typo-support feature back to v6.4.0 behavior. The single
matchSorter()call in recommendor.ts uses the standard API unaffected by this reversion.For Lit 3, there are no required code changes for the vast majority of users, and Lit 2.x and 3.0 are interoperable. The code imports decorators from
"lit/decorators.js"(the correct v3 pattern) and uses standard@customElement,@property,@state, and@querydecorators that remain fully backward-compatible.packages/formula-editor/src/utils/queue.ts (1)
21-25: All call sites handled correctly—no call sites exist.The
.peek()method is not used anywhere in the codebase. While the type safety improvement is valid and correct, the verification request cannot be performed as there are no usage sites to review. Since the method is unused, this change introduces no impact or breaking changes.packages/fw-theme-builder/package.json (1)
25-25: Verify TypeScript 5.9.3 compilation requires manual testing in development environment.TypeScript 5.9.3 introduces DOM lib updates, stricter type checks, and other correctness fixes compared to 4.6.2. While the codebase appears well-configured for compatibility (strict mode enabled, experimentalDecorators configured, no complex DOM API usage), the build verification cannot be completed in the sandbox environment.
Manual verification needed:
- Run
npm installat monorepo root- Execute
npm run buildinpackages/fw-theme-builder/- Review any new type errors that surface during compilation
- Pay attention to lib.d.ts/DOM changes and stricter type behavior
The existing build script (
tsc --composite false) and lint-staged configuration already check TypeScript compilation on commits, which will catch any incompatibilities when code is committed.eslint.config.js (1)
1-76: Well-structured ESLint configuration.The flat config format is properly implemented with comprehensive coverage for TypeScript, React, and Prettier. The custom rules strike a good balance between code quality enforcement and flexibility.
packages/styles/tsconfig.json (1)
2-2: Consolidation to root tsconfig looks good.This change aligns with the monorepo pattern of extending a shared base configuration from the repository root.
packages/trackers/tsconfig.json (1)
2-2: Extends path updated consistently.The change maintains consistency with the repository-wide tsconfig consolidation strategy.
tsconfig.json (1)
1-18: Solid base TypeScript configuration.The root tsconfig establishes sensible defaults: ES2022 target, strict mode, React JSX runtime, and bundler-compatible module resolution. The experimental decorators support aligns well with the Lit components in the codebase.
packages/fw-theme-builder/tsconfig.json (1)
2-8: Clean package-level configuration.The simplified configuration properly extends the root base while maintaining necessary package-specific overrides.
packages/formula-editor/tsconfig.json (1)
2-8: Configuration follows the established pattern.The package-level overrides are minimal and appropriate, consistent with the monorepo consolidation strategy.
packages/localize/tsconfig.json (1)
2-9: Package-specific overrides retained appropriately.The configuration extends the root base while keeping the
moduleResolution: "node"override, which may be necessary for this package's compatibility requirements.packages/formula-editor/src/suggestion-menu.ts (2)
27-31: Nice UX improvement for scroll behavior.Adding
inline: "nearest"andbehavior: "smooth"enhances the user experience when navigating through suggestions.
56-58: Style wrapper follows Lit best practices.Wrapping the style content in a
<style>tag is the recommended pattern for Lit components.packages/formula-editor/src/utils/parser.ts (1)
1-389: LGTM! Formatting and type safety improvements.The changes are well-executed linting and formatting improvements with enhanced type safety. The error handling refinement (lines 373-375) properly types the catch clause, and the explicit undefined checks (lines 384-386) improve runtime safety.
packages/formula-editor/src/formula-editor.ts (3)
45-70: Improved type annotations.Adding explicit type annotations to
@propertydecorators enhances type safety and aligns with TypeScript best practices.
128-128: Good fallback for errorString.The
|| ""operator ensureserrorStringis always a string, preventing potential undefined values from propagating.
201-203: Style rendering format updated.The explicit
<style>tag wrapper is a formatting preference that's consistent with the updated style rendering pattern used insuggestion-menu.ts.
| outputs: | ||
| run_format: ${{ steps.check.outputs.run_format }} | ||
| steps: | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
Update actions/checkout to v4.
Both checkout actions use @v3, which is outdated. GitHub recommends using @v4 for better performance and Node.js 20 support.
Apply this diff:
steps:
- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.ref }} steps:
- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.ref }}
fetch-depth: 0Also applies to: 33-33
🧰 Tools
🪛 actionlint (1.7.8)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/format.yml around lines 14 and 33, the workflow is
referencing actions/checkout@v3 which is outdated; update both occurrences to
actions/checkout@v4. Replace the uses entries on those lines to point to
actions/checkout@v4, save the file, and run or validate the workflow YAML (e.g.,
via GitHub Actions lint or a dry run) to ensure there are no syntax issues after
the change.
| * Formula Parser | ||
| */ | ||
| private _parser: Parser; | ||
| private _parser!: Parser; |
There was a problem hiding this comment.
Unsafe definite assignment assertion could cause runtime errors.
The ! assertion tells TypeScript that _parser will definitely be assigned before use, but this isn't guaranteed. The parser is only initialized in updated() when variables changes (line 88). If parseInput() is called before variables is set—such as through early user interaction—_parser will be undefined, causing a runtime error at line 125.
Apply this diff to add a runtime safety check:
parseInput(recommendation: string = "") {
+ if (!this._parser) {
+ console.warn('Parser not initialized. Ensure variables are set before parsing.');
+ return;
+ }
+
this.currentCursorPosition = this.editor.selectionStart;
const { recommendations, errorString, formattedString, newCursorPosition } = this._parser.parseInput(this.formulaString, this.currentCursorPosition, recommendation);Alternatively, initialize _parser with default values in the constructor or remove the ! assertion to restore TypeScript's null safety checks.
Based on learnings
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/formula-editor/src/formula-editor.ts around line 16 (and referencing
updated() at ~88 and parseInput() at ~125), the private field `_parser!:
Parser;` uses a definite-assignment assertion that can be undefined at runtime;
remove the `!` and either initialize `_parser` in the constructor with a safe
default parser or add a runtime guard where `_parser` is used (e.g., in
parseInput()) that checks for undefined and initializes or returns a safe
fallback/error before accessing it; ensure the updated() logic still assigns a
proper parser when variables change and update types/signatures accordingly so
TypeScript knows `_parser` is always defined when used.
| editor!: HTMLTextAreaElement; | ||
|
|
||
| @query("suggestion-menu") | ||
| suggestionMenu: SuggestionMenu; | ||
| suggestionMenu!: SuggestionMenu; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify DOM query assertions are safe.
The definite assignment assertions (!) on editor and suggestionMenu remove TypeScript's safety checks. Ensure these properties are only accessed after the component's first render completes.
Run the following script to verify access patterns:
🏁 Script executed:
#!/bin/bash
# Description: Check if editor or suggestionMenu are accessed before rendering
# Search for accesses to these properties
rg -nP --type=ts -C3 '\bthis\.editor\b|\bthis\.suggestionMenu\b' packages/formula-editor/src/Length of output: 3679
I need to check the context around when these methods are called and understand the component's lifecycle to determine if there are guards preventing early access.
Now I need to check when parseInput() is called and identify any potential unsafe access patterns:
I need to search the actual codebase for where these methods are called and understand the lifecycle:
🏁 Script executed:
#!/bin/bash
# Search for method invocations and lifecycle hooks
# Find where parseInput is called
echo "=== parseInput call sites ==="
rg -n "parseInput\(" packages/formula-editor/src/ --type=ts
# Find where _adjustTextAreaHeight is called
echo "=== _adjustTextAreaHeight call sites ==="
rg -n "_adjustTextAreaHeight\(\)" packages/formula-editor/src/ --type=ts
# Search for lifecycle hooks
echo "=== Lifecycle hooks ==="
rg -n "connectedCallback|firstUpdated|updated\(" packages/formula-editor/src/ --type=ts
# Search for event handlers (input, keydown, etc.)
echo "=== Event handlers setup ==="
rg -n "@eventOptions|addEventListener|on-" packages/formula-editor/src/ --type=ts -A2Length of output: 4600
I need to see the context of the method calls to understand when they're triggered:
🏁 Script executed:
#!/bin/bash
# Get full context of lines 94, 102, 170 and surrounding methods
echo "=== Context around parseInput calls ==="
rg -nB10 "parseInput\(\)" packages/formula-editor/src/formula-editor.ts | head -60
# Get context around line 84
echo -e "\n=== Context around line 84 _adjustTextAreaHeight call ==="
sed -n '75,95p' packages/formula-editor/src/formula-editor.ts
# Get full parseInput method to see unsafe access at line 123
echo -e "\n=== Full parseInput method ==="
sed -n '122,175p' packages/formula-editor/src/formula-editor.ts
# Check for keyboard event handler registration
echo -e "\n=== Keyboard event handler ==="
sed -n '175,200p' packages/formula-editor/src/formula-editor.tsLength of output: 4191
Now I need to verify when event handlers are attached:
🏁 Script executed:
#!/bin/bash
# Check the render method and event handler attachments
echo "=== Render method to see event handler registration ==="
sed -n '200,240p' packages/formula-editor/src/formula-editor.ts
# Check if there are any event listeners attached in connectedCallback, constructor, etc.
echo -e "\n=== Check for early event listener attachment ==="
rg -n "connectedCallback|constructor|addEventListener" packages/formula-editor/src/formula-editor.ts -A5
# Check if formatFormula is called anywhere that could be early
echo -e "\n=== formatFormula call sites ==="
rg -n "formatFormula\(" packages/formula-editor/src/ --type=ts -B3Length of output: 1389
Add defensive null checks to prevent potential runtime crashes.
The definite assignment assertions (!) on editor and suggestionMenu mask real safety risks. Evidence shows:
-
Line 123:
parseInput()accessesthis.editor.selectionStartwithout null check. Since@inputevents can fire after DOM insertion but before render completes, this can crash at runtime. -
Lines 184, 187, 192, 195:
handleKeydown()accessesthis.suggestionMenumethods without null checks, same timing risk. -
Inconsistent pattern: Line 107 in
_adjustTextAreaHeight()demonstrates the correct guard:if (!this.editor) return;— this pattern should be applied consistently.
Required fixes:
- Line 123: Add
if (!this.editor) return;guard before accessingthis.editor.selectionStart - Lines 184, 187, 192, 195: Add guard
if (!this.suggestionMenu) return;inhandleKeydown()before accessing methods
🤖 Prompt for AI Agents
In packages/formula-editor/src/formula-editor.ts around lines 73–76 and the
referenced usage sites (line 107, line 123, and lines 184, 187, 192, 195), the
definite assignment assertions for editor and suggestionMenu bypass null-safety
checks; add defensive guards: before accessing this.editor.selectionStart at
line 123 insert if (!this.editor) return; (matching the pattern at line 107),
and at the start of handleKeydown() add if (!this.suggestionMenu) return; so all
calls to this.suggestionMenu.* on lines 184, 187, 192 and 195 are protected from
null/undefined access.
| @query(".fw-formula-suggestion-menu") | ||
| suggestionList!: HTMLUListElement; |
There was a problem hiding this comment.
Non-null assertion may be unsafe.
The @query decorator can return undefined if the element isn't found in the DOM (e.g., before first render or if the selector doesn't match). The non-null assertion operator (!) bypasses TypeScript's safety checks. Note the inconsistency: Line 24 uses optional chaining (suggestionList?.), which acknowledges the field can be nullish, while the type declaration claims it's always defined.
Consider removing the ! assertion and handling the undefined case consistently:
@query(".fw-formula-suggestion-menu")
- suggestionList!: HTMLUListElement;
+ suggestionList: HTMLUListElement | undefined;The existing optional chaining on line 24 already handles this correctly.
🤖 Prompt for AI Agents
In packages/formula-editor/src/suggestion-menu.ts around lines 20 to 21, the
property declaration uses a non-null assertion (`!`) even though the decorated
query can return undefined; change the declaration to allow undefined (e.g.,
drop `!` and type as HTMLUListElement | undefined or make it optional) and keep
using the existing optional chaining where the property is accessed so undefined
is handled safely.
| "@polymer/paper-item": "^3.0.1", | ||
| "@polymer/paper-listbox": "^3.0.1", | ||
| "lit": "^2.1.2" | ||
| "lit": "^3.3.1" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Lit 3.x compatibility with existing code.
The upgrade from Lit 2.1.2 to 3.3.1 is a major version jump that includes breaking changes such as decorator updates, reactive controller changes, and SSR-related modifications. Ensure all components in this package are compatible with Lit 3.x APIs.
🏁 Script executed:
#!/bin/bash
# Check for usage patterns that may be affected by Lit 3.x breaking changes
# Focus on decorators, reactive controllers, and deprecated APIs
echo "=== Checking for @customElement and other decorators ==="
rg -n --type=ts --type=js -C2 '@customElement|@property|@state|@query' packages/fw-theme-builder/
echo ""
echo "=== Checking for ReactiveController usage ==="
rg -n --type=ts --type=js -C2 'ReactiveController|addController' packages/fw-theme-builder/
echo ""
echo "=== Checking for deprecated LitElement imports ==="
rg -n --type=ts --type=js 'from.*lit-element' packages/fw-theme-builder/Length of output: 3934
---
🌐 Web query:
```web
Lit 3.x migration breaking changes decorators compatibility
💡 Result:
Short summary — what breaks and how to fix it when migrating to Lit 3.x (decorators)
Key issues
- Lit still relies on transpiled decorators; differences between the “legacy” (TypeScript/Babel legacy) decorator semantics and the new Stage‑3 / ES decorators can break decorator usage (signatures, timing, and class‑field interaction). [1][4]
- Class field semantics interact badly with decorator-based reactive properties unless you use the recommended TS/Babel settings (you must avoid the define‑semantics for class fields). [1][2]
- If your build uses the older “legacy” decorator transform you may need to change compiler/plugin settings to match Lit’s expectations (order of transforms, plugin options). [1][2][5]
Concrete fixes / checklist
- TypeScript: enable experimentalDecorators and set useDefineForClassFields: false in tsconfig (recommended by Lit). [1][2]
- tsconfig excerpt: "experimentalDecorators": true, "useDefineForClassFields": false. [1][6]
- Babel (if used): run the TypeScript transform before the decorators transform and configure the decorators plugin to the expected version/flags. Example ordering in Babel config: transform-typescript → plugin‑proposal‑decorators (compatible mode) → plugin‑proposal‑class‑properties. [1][2]
- Decide which decorator semantics you target:
- If you rely on legacy decorator behavior, keep the legacy/transpiler plugin configuration that preserves that behavior.
- If you migrate to the Stage‑3 ES decorators, be aware they are not drop‑in compatible (different argument shapes and restrictions). Update any custom decorators to the new signature/semantics. See Babel/TC39 notes on Stage‑3 differences. [4][5]
- Imports: use the Lit decorator entry points (e.g., import {property,customElement} from 'lit/decorators.js' or per‑decorator paths) as in Lit docs. [1]
- Build/publish: compile decorators and class fields to stable ES you publish (Lit publishing guidance). Ensure consumers aren’t forced to run your transpilation. [6]
Where to read the authoritative guidance
- Lit decorators and TS/Babel setup (how to enable and avoid class‑field issues). [1]
- Lit TypeScript and development notes (useDefineForClassFields guidance). [2]
- Babel notes on the new Stage‑3 decorators and migration implications. [5]
- Lit publishing recommendations to compile modern features before publishing. [6]
Sources
[1] Lit decorators docs — Enabling decorators & avoiding class‑field issues.
[2] Lit development / TypeScript guidance (useDefineForClassFields recommendation).
[4] Talk/notes on Stage‑3 decorator differences (migration implications).
[5] Babel 7.21.0 blog — decorators updates and migration help.
[6] Lit publishing docs (compile decorators/class fields before publishing).
If you want, I can: (a) inspect your tsconfig/babel config and show exact changes, or (b) produce a ready tsconfig + Babel config that works with Lit 3.x and either legacy or Stage‑3 decorators. Which would you prefer?
🏁 Script executed:
#!/bin/bash
# Check tsconfig settings for Lit 3.x requirements
echo "=== Checking tsconfig.json in packages/fw-theme-builder ==="
fd -n "tsconfig" packages/fw-theme-builder/ -type f
echo ""
echo "=== Checking for babel config files ==="
fd -n "babel.config|.babelrc" packages/fw-theme-builder/ -type f
echo ""
echo "=== Checking root and shared tsconfig files ==="
fd -n "tsconfig" . -maxdepth 2 -type f
echo ""
echo "=== Examining tsconfig.json content if found in fw-theme-builder ==="
if [ -f "packages/fw-theme-builder/tsconfig.json" ]; then
cat packages/fw-theme-builder/tsconfig.json
fi
echo ""
echo "=== Examining root tsconfig if it exists ==="
if [ -f "tsconfig.json" ]; then
cat tsconfig.json
fiLength of output: 1583
Fix root tsconfig.json for Lit 3.x decorator compatibility.
Lit 3.x requires useDefineForClassFields: false in TypeScript configuration to avoid class field semantics conflicts with decorator-based reactive properties. The root tsconfig.json currently sets "useDefineForClassFields": true, which will break the @property, @state, and other decorators used throughout this package.
Update tsconfig.json root level:
- Change
"useDefineForClassFields": trueto"useDefineForClassFields": false - Keep
"experimentalDecorators": true(already correct)
This is required for the components in packages/fw-theme-builder/src/ (fw-size-pick.ts, fw-font-pick.ts, fw-theme-builder.ts, fw-color-pick.ts) to function correctly with Lit 3.3.1.
🤖 Prompt for AI Agents
In the root tsconfig.json (around compilerOptions), change
"useDefineForClassFields": true to "useDefineForClassFields": false to restore
decorator-compatible class field semantics required by Lit 3.x; leave
"experimentalDecorators": true as-is. This update fixes decorator-based reactive
properties (@property, @state, etc.) used by packages/fw-theme-builder/src/*
(fw-size-pick.ts, fw-font-pick.ts, fw-theme-builder.ts, fw-color-pick.ts) so
they work with lit@3.3.1.
Description
Resolutions
Deployment
Current pipelines will suffice
Summary by CodeRabbit