refactor: simplify type management in Codecs #723
Conversation
Simplify null checks and introduce the `xmlUtils.isElement` internal function that narrow down Element type (no longer need to cast types manually). This new function is used in other places when possible.
WalkthroughSeveral files have been updated to streamline control flow and enforce stricter type handling when processing XML nodes. The changes replace direct comparisons to a constant with a new utility function Changes
Sequence Diagram(s)sequenceDiagram
participant Decoder
participant DOMNode as Node
participant Util as isElement()
Decoder->>DOMNode: Retrieve first/child node
DOMNode-->>Decoder: Return node
Decoder->>Util: Evaluate node with isElement(node)
Util-->>Decoder: Return true/false
alt Node is an element
Decoder->>Decoder: Process node attributes and children
else Node is not an element
Decoder->>Decoder: Skip or apply alternative handling
end
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/editor/EditorPopupMenu.ts (1)
194-194: Consider using optional chaining instead of non-null assertion.Using the non-null assertion operator (
!) assumes thatitem.getAttribute('as')will never return null, which could lead to runtime errors if this assumption is ever violated.Consider using optional chaining with a fallback value instead:
-let as = item.getAttribute('as')!; +let as = item.getAttribute('as') || '';This would provide better error handling while still simplifying the code.
packages/core/src/serialization/codecs/editor/EditorCodec.ts (2)
198-198: Simplified null checking with non-null assertion.Changed from type assertion
<string>to non-null assertion operator!which aligns with the PR objective to simplify null checks. However, be careful with non-null assertions as they bypass TypeScript's type checking.A safer alternative would be to add a null check:
-Translations.add(tmp.getAttribute('basename')!); +const basename = tmp.getAttribute('basename'); +if (basename) { + Translations.add(basename); +}
200-200: Simplified null checking with non-null assertion.Similar to the previous change, this replaces type assertion with a non-null assertion. While it aligns with the PR objective to simplify null checks, it might be better to handle potential null values explicitly.
Consider adding a null check:
-addLinkToHead('stylesheet', tmp.getAttribute('name')!); +const name = tmp.getAttribute('name'); +if (name) { + addLinkToHead('stylesheet', name); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/core/src/editor/EditorPopupMenu.ts(1 hunks)packages/core/src/internal-types.ts(1 hunks)packages/core/src/serialization/Codec.ts(4 hunks)packages/core/src/serialization/ObjectCodec.ts(2 hunks)packages/core/src/serialization/codecs/CellCodec.ts(2 hunks)packages/core/src/serialization/codecs/ChildChangeCodec.ts(3 hunks)packages/core/src/serialization/codecs/RootChangeCodec.ts(2 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(2 hunks)packages/core/src/serialization/codecs/editor/EditorCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts(2 hunks)packages/core/src/util/domUtils.ts(3 hunks)packages/core/src/util/xmlUtils.ts(3 hunks)packages/core/src/view/cell/Cell.ts(4 hunks)packages/core/src/view/geometry/node/StencilShape.ts(4 hunks)
🧰 Additional context used
🧬 Code Definitions (11)
packages/core/src/serialization/Codec.ts (1)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)
packages/core/src/serialization/codecs/CellCodec.ts (2)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)packages/core/src/view/cell/Cell.ts (1)
Cell(74-887)
packages/core/src/serialization/codecs/StylesheetCodec.ts (2)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)packages/core/src/util/domUtils.ts (1)
getTextContent(87-89)
packages/core/src/serialization/ObjectCodec.ts (1)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)
packages/core/src/util/xmlUtils.ts (1)
packages/core/src/internal-types.ts (1)
UserObject(21-28)
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)
packages/core/src/serialization/codecs/RootChangeCodec.ts (1)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)
packages/core/src/view/geometry/node/StencilShape.ts (1)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)
packages/core/src/serialization/codecs/editor/EditorCodec.ts (1)
packages/core/src/util/domUtils.ts (1)
addLinkToHead(339-361)
packages/core/src/serialization/codecs/ChildChangeCodec.ts (1)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)
packages/core/src/view/cell/Cell.ts (2)
packages/core/src/util/xmlUtils.ts (1)
isElement(257-258)packages/core/src/util/Utils.ts (1)
isNotNullish(66-67)
🪛 Biome (1.9.4)
packages/core/src/serialization/codecs/StylesheetCodec.ts
[error] 166-166: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
🔇 Additional comments (45)
packages/core/src/serialization/codecs/CellCodec.ts (3)
23-23: Good addition of the isElement utility function import.Importing the utility function for element type checking improves the code organization and follows the PR objective of simplifying type management.
85-85: Great replacement of direct nodeType check with isElement utility.This change enhances readability and type safety by using the dedicated utility function instead of directly checking against NODETYPE.ELEMENT constant. The function properly handles null/undefined values with optional chaining.
93-93: Good consistent use of isElement utility.Replacing the direct nodeType check with isElement function here maintains consistency with the other changes and improves the type checking mechanism throughout the codebase.
packages/core/src/internal-types.ts (1)
17-28: Well-structured UserObject type definition.The new UserObject type provides a clear interface for objects that behave like XML elements but might not be actual DOM Elements. The optional properties appropriately model the subset of Element functionality needed by the codebase, and the internal/private markings clearly indicate its intended usage scope.
This addition aligns well with the PR objective of simplifying type management by providing a specific type for user objects that can be used with the new isElement utility function.
packages/core/src/serialization/Codec.ts (4)
25-25: Good addition of the isElement utility function import.Adding this import supports the PR's goal of simplifying type management across the codebase.
251-251: Appropriate use of isElement for element type checking.Replacing the direct nodeType check with the isElement utility function improves code readability and maintainability. The type predicate in isElement also helps TypeScript understand the type narrowing.
362-362: Good consistent use of isElement utility.This change maintains consistency with the other changes throughout the codebase, making element type checks more standardized.
432-432: Well-applied isElement check in negative condition.Using !isElement(node) instead of the previous complex condition simplifies the code while maintaining the same functionality. This is a good example of how the utility function can improve code clarity.
packages/core/src/editor/EditorPopupMenu.ts (1)
189-189: Simplified loop condition improves readability.Changing from explicit null check to a truthy check is cleaner, as both achieve the same result in this context. This aligns with the PR's goal of simplifying null checks.
packages/core/src/view/geometry/node/StencilShape.ts (4)
36-36: Added utility import to replace direct node type comparisons.Good addition. Importing the new
isElementtype predicate function will help improve type safety by narrowing down Element types automatically.
279-281: Replaced direct nodeType check with isElement utility function.Nice refactoring. Using
isElement(tmp)instead of checking againstNODETYPE.ELEMENTdirectly makes the code more readable and type-safe. The type predicate ensures TypeScript knows thattmpis an Element inside the conditional block.
378-398: Simplified node type checking with isElement.Good change. Using the
isElement()type predicate here improves code consistency and provides better type safety in the conditional block, letting TypeScript know thatchildNodeis an Element when accessing properties and methods.
426-428: Consistent use of isElement for type checking.This third instance of replacing direct nodeType comparison with the
isElement()function maintains consistency throughout the file and improves type safety.packages/core/src/util/xmlUtils.ts (3)
29-29: Added import for UserObject type.Good addition. Importing the
UserObjecttype from internal-types will support the implementation of the isElement function.
232-237: Updated findNode to use the new isElement function.The refactoring improves consistency by using the new utility function instead of direct nodeType comparison. This is more maintainable and ensures consistent node type checking across the codebase.
252-258: Added new isElement utility function.Well-implemented type predicate function that simplifies checking if a node is an Element. The function:
- Uses optional chaining (
?.) for null safety- Returns a TypeScript type predicate (
node is Element) to narrow down types- Properly handles optional/nullable parameters
- Includes JSDoc with
@internaland@privatemarkersThis will help eliminate manual type casting throughout the codebase as mentioned in the PR objectives.
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (2)
27-27: Added import for isElement utility function.Good addition. Importing the
isElementtype predicate from xmlUtils will help improve type safety in this file.
137-137: Replaced direct nodeType check with isElement utility function.Good refactoring. Using
isElement(node)instead of directly checking againstNODETYPE.ELEMENTmakes the code more readable and type-safe, while ensuring TypeScript properly narrows down the node type inside the conditional block.packages/core/src/serialization/codecs/RootChangeCodec.ts (4)
20-21: Great import refactoring.Replacing the direct import of
NODETYPEwith theisElementutility function helps improve type safety and code readability.
54-54: Improved type checking with isElement.Using the
isElementutility function instead of manually checkingnodeType === NODETYPE.ELEMENTmakes the code more readable and type-safe. The function serves as a type predicate that properly narrows down the type.
62-62: Enhanced null safety with optional chaining.Using optional chaining (
?.) forparentNodeaccess improves code safety by gracefully handling potential null/undefined values, preventing runtime errors.
68-68: Enhanced null safety with optional chaining.Similar to the earlier change, this optional chaining operator provides better null safety when accessing
parentNodebefore callingremoveChild.packages/core/src/util/domUtils.ts (3)
88-88: Modernized code with optional chaining and nullish coalescing.Replacing the conditional check with optional chaining (
?.) and nullish coalescing (??) operators makes the code more concise and readable while maintaining the same functionality.
265-265: Updated documentation reference.Updating the JSDoc comment to reference
{@link NODETYPE.ELEMENT}instead of{@link Constants#NODETYPE_ELEMENT}makes the documentation more accurate and consistent with the code.
287-287: Improved API reference in documentation.Changing the reference from
document.importNodeto{@link Document.importNode}in the documentation provides a better and more consistent JSDoc link to the standard DOM API.packages/core/src/serialization/codecs/StylesheetCodec.ts (7)
25-25: Good import consolidation.Importing
isElementutility instead of usingNODETYPEdirectly aligns with the PR's goal to simplify type management.
157-158: Improved loop conditions and type safety.The changes make the code more concise by:
- Using a truthy check for
entryinstead of explicit null comparison- Using the
isElementfunction for type checking instead of directly comparingnodeTypeThis enhances readability while maintaining the same functionality.
159-159: Non-null assertion is appropriately used.Adding the non-null assertion operator (
!) ongetAttribute('as')makes sense here since this code only executes when we've confirmedentryis an Element, and the attribute is required for the code to function properly.
165-165: Simplified truthy check.Changing from
text != null && text.length > 0to simplytextis a good simplification as empty strings are falsy in JavaScript, making the code more concise.
168-168: Removed unnecessary type casting.Good removal of the explicit type casting to
Elementsince theisElementcheck already confirms the type.
171-171: Appropriate use of non-null assertion with explanatory comment.Adding the non-null assertion (
!) with an explanatory comment clarifies why it's safe to use - theisNumericcheck already ensures the value is not null.
166-166:❓ Verification inconclusive
Security concern with eval usage.
While not changed in this PR, the use of
eval()poses a security risk, especially if processing untrusted data.The static analysis has flagged this line. Consider replacing
eval()with a safer alternative or ensuring that the input is properly sanitized before evaluation.
🏁 Script executed:
#!/bin/bash # Check if there are any existing safeguards for eval usage rg -A 5 -B 5 "allowEval" --glob "*.ts"Length of output: 14426
Security Review – Verify Handling of
eval()in StylesheetCodecThe static analysis correctly flagged the use of
eval()in the stylesheet codec. We’ve verified that this usage is gated by the global flagStylesheetCodec.allowEval(which is set totrueby default) and that similar patterns exist elsewhere in the repository. However, this design still carries a significant security risk if untrusted data ever reaches these code paths. Please ensure that one of the following actions is taken:
- Confirm that inputs processed by
eval()are strictly sanitized and come from trusted sources.- Consider replacing
eval()with a safer alternative to avoid the potential for arbitrary code execution.- Reevaluate whether enabling
allowEvalby default is appropriate, especially in contexts where user input might be processed.🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
packages/core/src/serialization/codecs/ChildChangeCodec.ts (6)
20-21: Good import refactoring.Replacing the direct import of
NODETYPEwith theisElementutility function is consistent with the PR's goal to simplify type management.
97-97: Improved type checking.Using the
isElementutility function instead of manually checkingnodeType === NODETYPE.ELEMENTenhances code readability and type safety.
108-108: Simplified loop condition.Using a simple truthy check
while (tmp)instead of the explicitwhile (tmp != null)makes the code more concise without changing functionality.
111-111: Consistent type checking pattern.Using
isElement(tmp)here maintains consistency with the pattern established throughout the codebase, making the code more maintainable.
117-117: Appropriate use of non-null assertion.The non-null assertion on
getAttribute('id')is appropriate here as this code block only executes for elements that have ID attributes in this context.
124-124: Enhanced null safety with optional chaining.Adding optional chaining for
parentNode?.removeChild(tmp)improves code safety by gracefully handling potential null values, preventing runtime errors.packages/core/src/serialization/ObjectCodec.ts (3)
27-27: Added import for the new isElement utility functionThis import supports the simplified type checking approach by using the utility function instead of direct NODETYPE comparison.
794-794: Simplified loop conditionChanged from explicit null check (
child != null) to a more concise truthy check. This maintains the same behavior since both conditions will evaluate to false whenchildis null or undefined.
797-797: Replaced direct nodeType check with isElement utility functionThis change improves readability and provides better type safety through the type predicate in the isElement function. The function handles the null/undefined check internally, making the code more robust.
packages/core/src/view/cell/Cell.ts (4)
28-29: Added imports for isElement utility and moved UserObject type to internal-typesThis change centralizes type definitions and leverages the new utility function for type checking. Moving the UserObject type to a shared location improves code organization and maintainability.
569-571: Simplified hasAttribute type check using isElement utilityThe new implementation is more concise and readable. It properly handles type checking by using the isElement utility function and adds fallback with optional chaining and isNotNullish for non-Element objects.
583-583: Enhanced getAttribute with isElement type checkingReplaced the direct nodeType check with isElement for clearer type checking. Also using optional chaining for safer property access.
597-597: Improved setAttribute with isElement utility functionThe condition is now simpler and more readable, using the isElement utility function for type checking instead of directly accessing nodeType property.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/serialization/Codec.ts(4 hunks)packages/core/src/serialization/ObjectCodec.ts(2 hunks)packages/core/src/serialization/codecs/CellCodec.ts(2 hunks)packages/core/src/serialization/codecs/ChildChangeCodec.ts(3 hunks)packages/core/src/serialization/codecs/RootChangeCodec.ts(2 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(2 hunks)packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts(2 hunks)packages/core/src/util/domUtils.ts(5 hunks)packages/core/src/util/xmlUtils.ts(2 hunks)packages/core/src/view/cell/Cell.ts(4 hunks)packages/core/src/view/geometry/node/StencilShape.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/core/src/serialization/Codec.ts
- packages/core/src/serialization/codecs/RootChangeCodec.ts
- packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts
- packages/core/src/serialization/codecs/CellCodec.ts
- packages/core/src/serialization/ObjectCodec.ts
- packages/core/src/view/geometry/node/StencilShape.ts
- packages/core/src/view/cell/Cell.ts
- packages/core/src/serialization/codecs/ChildChangeCodec.ts
- packages/core/src/util/xmlUtils.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/core/src/util/domUtils.ts (1)
packages/core/src/internal-types.ts (1)
UserObject(21-28)
🪛 Biome (1.9.4)
packages/core/src/serialization/codecs/StylesheetCodec.ts
[error] 165-165: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
🔇 Additional comments (9)
packages/core/src/serialization/codecs/StylesheetCodec.ts (4)
24-24: Good addition of the isElement importThe import of
isElementfrom domUtils centralizes the type checking logic, which supports the PR objective of simplifying type management.
156-158: Improved type safety with isElementThe use of the
isElementfunction replaces a direct nodeType comparison, providing better type safety and readability. The non-null assertion ongetAttribute('as')is appropriate as it's used within a condition that ensures it exists.
164-164: Simplified conditional checkThe condition has been streamlined from checking
text != null && text.length > 0to justtext, which correctly handles both null/undefined and empty string cases in a more concise way.
167-170: Improved type handling with appropriate assertionsThe code now leverages the earlier
isElementcheck to simplify getting the attribute value, and the non-null assertion onparseFloat(value!)is well-justified by theisNumericcheck.packages/core/src/util/domUtils.ts (5)
18-18: Good addition of UserObject type importThe import of
UserObjectfrom internal-types is appropriate for the newisElementfunction parameter type.
89-89: Improved code with modern JavaScript featuresThe refactoring to use optional chaining and nullish coalescing operators makes the code more concise and readable while maintaining the same behavior.
266-266: Documentation improvementUpdated JSDoc to correctly reference
NODETYPE.ELEMENTinstead ofConstants#NODETYPE_ELEMENT, which improves accuracy and consistency.
288-288: Documentation improvementUpdated JSDoc to correctly reference
Document.importNodeinstead ofdocument.importNode, which is more accurate as it refers to the interface method rather than a specific instance.
363-369: Well-implemented type predicate functionThe new
isElementfunction is a great addition that centralizes node type checking logic across the codebase. The implementation properly uses:
- TypeScript type predicates (
node is Element) for type narrowing- Optional chaining for null-safety
- Clear documentation with appropriate visibility markers
This aligns perfectly with the PR objective to simplify type management and reduce manual type casting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/util/mathUtils.ts (1)
661-661: Type predicate enhances type safety, but consider using a more specific type.The addition of the type predicate
: n is {}converts this function from a regular boolean function to a TypeScript type guard, which helps narrow down types after checks. This aligns well with the PR's goal of simplifying type management.However,
{}as a type can be confusing as it means "any non-nullable value" rather than an empty object. For better type specificity, consider:- export const isNumeric = (n: any): n is {} => { + export const isNumeric = (n: any): n is number | string => {Or even more specifically:
- export const isNumeric = (n: any): n is {} => { + export const isNumeric = (n: any): n is number | (string & { __numeric: true }) => {🧰 Tools
🪛 Biome (1.9.4)
[error] 661-661: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/__tests__/util/mathUtils.test.ts(1 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(2 hunks)packages/core/src/util/mathUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/core/__tests__/util/mathUtils.test.ts (1)
packages/core/src/util/mathUtils.ts (1)
isNumeric(661-667)
packages/core/src/serialization/codecs/StylesheetCodec.ts (2)
packages/core/src/util/domUtils.ts (2)
isElement(368-369)getTextContent(88-90)packages/core/src/util/mathUtils.ts (1)
isNumeric(661-667)
🪛 Biome (1.9.4)
packages/core/src/util/mathUtils.ts
[error] 661-661: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/core/src/serialization/codecs/StylesheetCodec.ts
[error] 165-165: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (7)
packages/core/__tests__/util/mathUtils.test.ts (1)
17-52: Excellent test coverage for the isNumeric function.This comprehensive test suite validates the
isNumericfunction's behavior across various input types and edge cases, including:
- Nullish values (null, undefined)
- Valid numbers
- Special values (Infinity)
- Different string formats (empty, hex, non-numeric, large numbers)
- Objects
The tests align perfectly with the updated function signature that now includes a type predicate.
packages/core/src/serialization/codecs/StylesheetCodec.ts (6)
24-24: Good use of helper functions for improved type safety.Adding the import for
isElementandgetTextContentfrom domUtils aligns with the PR's goal of simplifying type management by leveraging utility functions that provide proper type narrowing.
156-158: Simplified null checks and improved type safety.This change replaces manual node type checking with the more readable
isElementutility function. This is a good improvement as:
- It simplifies the loop condition (
while (entry)vs checking for null)- The
isElementfunction ensures TypeScript correctly narrows the type, eliminating the need for manual type casting- It makes the code more maintainable by centralizing the element type-checking logic
164-164: Cleaner conditional checking.The condition has been simplified from an explicit null and length check to a more concise truthy check. This improves readability while maintaining the same logic, as empty strings and null/undefined values will evaluate to falsy.
167-167: Simplified attribute retrieval with type safety.This change replaces explicit type casting with a direct attribute access, relying on the fact that the
isElementcheck above has already narrowed the type properly. This is a cleaner approach that aligns with the PR's goal of eliminating manual type casting.
170-170: Non-null assertion is now properly justified.The use of the non-null assertion operator (
!) is appropriate here since theisNumericcheck guarantees thatvalueis a valid numeric string at this point. This is a good example of leveraging TypeScript's type system effectively.
165-165: Security risk with eval() remains unchanged.While not part of the current changes, using
eval()exposes the application to security risks as flagged by the static analysis tool.Consider refactoring to avoid using
eval()entirely. Some alternatives include:
- Using a JavaScript expression parser library
- Implementing a restricted subset of operations that don't require eval
- Using Function constructor with proper sanitization
- if (text && StylesheetCodec.allowEval) { - value = eval(text); - } else { + if (text) { + // Parse expressions without eval, perhaps using a safer approach + // or restrict to known function names from a whitelist + value = StylesheetCodec.allowEval ? StylesheetCodec.safeEvaluate(text) : entry.getAttribute('value'); + } else {🧰 Tools
🪛 Biome (1.9.4)
[error] 165-165: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/serialization/codecs/StylesheetCodec.ts (3)
138-141: Similar truthy check concern for 'as' attributeThe change from explicit null check to truthy check for
ascould reject empty strings that might have been valid before. Additionally, the conditional assignment forstylehas been refactored, which looks cleaner.
155-155: Be cautious with non-null assertionsThe non-null assertion (
!) onentry.getAttribute('as')assumes the attribute will always exist. Given that this follows anisElementcheck but no explicit check for the attribute's existence, this could potentially lead to runtime errors.Consider adding an explicit check for the attribute's existence:
-const key = entry.getAttribute('as')!; +const key = entry.getAttribute('as'); +if (key) { // Proceed with the existing logic +}
167-167: Another non-null assertion to reviewThe non-null assertion on
parseFloat(value!)assumes the value is not null after theisNumericcheck. While likely correct, explicit conditional handling might be more robust.-value = parseFloat(value); +value = value !== null ? parseFloat(value) : null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/serialization/codecs/StylesheetCodec.ts(3 hunks)packages/core/src/util/mathUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/util/mathUtils.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/serialization/codecs/StylesheetCodec.ts
[error] 162-162: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
🔇 Additional comments (6)
packages/core/src/serialization/codecs/StylesheetCodec.ts (6)
24-24: Good addition of the isElement utility function importAdding the
isElementutility function import aligns with the PR's objective of simplifying type management. This function will help narrow down Element types and eliminate the need for manual type casting throughout the code.
128-130: Consider the implications of changing from null check to truthy checkChanging from
if (id != null)toif (id)simplifies the code but also changes the behavior. The truthy check will evaluate empty strings as falsy, which might not be the original intent if empty string IDs were valid.Does your application logic allow for empty string IDs? If not, this change is appropriate.
153-155: Great use of the isElement type guard functionThis is an excellent application of the new
isElementtype guard function, which makes the code more readable and type-safe. It replaces the previous manual type check that likely used node type constants.
161-169: Caution: Security concern with eval() persistsWhile not part of the current changes (as noted in past review comments), using
eval()poses a security risk. According to the comments, this issue was addressed in later commits, but ensure this implementation is secure if stylesheets may contain user input.🧰 Tools
🪛 Biome (1.9.4)
[error] 162-162: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
171-173: Similar truthy check concern for 'value'The change from explicit null check to truthy check for
valuecould cause issues if0is a valid value for certain styles, as it would be treated as falsy and ignored.Are there any style properties where numeric zero is a valid and meaningful value? If so, consider using an explicit null check instead.
-if (value) { +if (value !== null && value !== undefined) { style[key] = value; }
96-122: Documentation improvementsThe documentation formatting changes make the JSDoc comments more consistent and readable. Good improvement to the code's documentation.
1 participant
Simplify null checks and introduce the
xmlUtils.isElementinternal function that narrow down Element type (no longer need to cast types manually).This new function is used in other places when possible.
Summary by CodeRabbit
Refactor
New Features
isElementfor improved element type checking across various components.Tests
isNumericfunction, covering various input scenarios to ensure expected behavior.These improvements enhance the app’s stability and reliability while keeping user functionality and appearance unchanged.