Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/custom validate func #667

Merged
merged 9 commits into from
Jun 14, 2024
Merged

Feat/custom validate func #667

merged 9 commits into from
Jun 14, 2024

Conversation

egordidenko
Copy link
Contributor

@egordidenko egordidenko commented May 29, 2024

Description

Checklist

Summary by CodeRabbit

  • New Features

    • Introduced a new ValidationManager to handle file and collection validation, improving the validation process.
    • Added support for custom file and collection validators in the configuration settings.
  • Bug Fixes

    • Enhanced error handling for file and collection uploads, ensuring more accurate error messages.
  • Documentation

    • Updated example HTML files to demonstrate new validation functionalities.
  • Refactor

    • Streamlined validation logic by consolidating it into the ValidationManager, removing redundant code.
  • Style

    • Improved code readability and maintainability by cleaning up imports and removing unused functions.

Copy link
Contributor

coderabbitai bot commented May 29, 2024

Walkthrough

The changes introduce a centralized ValidationManager for handling file and collection validations within the uploader block. This refactor simplifies the code by removing multiple individual validator functions and replacing them with a unified validation system. Configuration settings are updated to support the new validators, and type definitions are enhanced to reflect these changes. Additionally, demo files are updated to showcase the new validation functionality.

Changes

Files / Groups Change Summary
abstract/UploaderBlock.js Removed old validators, added ValidationManager, updated logic to use the new manager.
abstract/ValidationManager.js Introduced ValidationManager class for handling file and collection validations.
blocks/Config/Config.js Added fileValidators and collectionValidators to complexConfigKeys.
blocks/Config/initialConfig.js Added fileValidators and collectionValidators properties to initialConfig.
blocks/Config/normalizeConfigValue.js Introduced asArray function and updated mapping object for new validators.
demo/raw-regular.html Added a script tag to reference index.js as a module.
abstract/CTX.js Updated type annotation for collectionErrors to use types from ValidationManager.
abstract/buildOutputCollectionState.js Updated return type annotation for errors function.
types/exported.d.ts Added FileValidator and CollectionValidator types.
utils/validators/collection/index.js Exported validateCollectionUploadError and validateMultiple functions.
utils/validators/collection/validateCollectionUploadError.js Introduced validateCollectionUploadError function.
utils/validators/collection/validateMultiple.js Introduced validateMultiple function.
utils/validators/file/index.js Exported validateIsImage, validateFileType, validateMaxSizeLimit, and validateUploadError functions.
utils/validators/file/validateFileType.js Introduced validateFileType function.
utils/validators/file/validateIsImage.js Introduced validateIsImage function.
utils/validators/file/validateMaxSizeLimit.js Introduced validateMaxSizeLimit function.
utils/validators/file/validateUploadError.js Introduced validateUploadError function.
demo/validators.html Added new validation functions and updated configuration in demo HTML.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UploaderBlock
    participant ValidationManager
    participant FileValidator
    participant CollectionValidator

    User->>UploaderBlock: Upload File/Collection
    UploaderBlock->>ValidationManager: Run Validators
    ValidationManager->>FileValidator: Validate File
    FileValidator-->>ValidationManager: Return Validation Result
    ValidationManager->>CollectionValidator: Validate Collection
    CollectionValidator-->>ValidationManager: Return Validation Result
    ValidationManager-->>UploaderBlock: Return Validation Results
    UploaderBlock-->>User: Display Validation Results
Loading

Poem

In the code, a change so grand,
Validators now take a stand.
With a manager to guide the way,
Errors and files kept at bay.
Configuration set just right,
Uploads now a smoother flight.
🎉✨


Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Outside diff range and nitpick comments (1)
blocks/Config/Config.js (1)

Line range hint 61-61: Prefer template literals over string concatenation for better readability and consistency.

- const getLocalPropName = (key) => '__' + key;
+ const getLocalPropName = (key) => `__${key}`;
Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between f2e29f3 and 2741c59.
Files selected for processing (15)
  • abstract/UploaderBlock.js (8 hunks)
  • abstract/ValidationManager.js (1 hunks)
  • blocks/Config/Config.js (2 hunks)
  • blocks/Config/initialConfig.js (1 hunks)
  • blocks/Config/normalizeConfigValue.js (2 hunks)
  • demo/raw-regular.html (1 hunks)
  • types/exported.d.ts (3 hunks)
  • utils/validators/collection/index.js (1 hunks)
  • utils/validators/collection/validateCollectionUploadError.js (1 hunks)
  • utils/validators/collection/validateMultiple.js (1 hunks)
  • utils/validators/file/index.js (1 hunks)
  • utils/validators/file/validateFileType.js (1 hunks)
  • utils/validators/file/validateIsImage.js (1 hunks)
  • utils/validators/file/validateMaxSizeLimit.js (1 hunks)
  • utils/validators/file/validateUploadError.js (1 hunks)
Files skipped from review due to trivial changes (4)
  • blocks/Config/initialConfig.js
  • demo/raw-regular.html
  • utils/validators/collection/index.js
  • utils/validators/file/index.js
Additional Context Used
Biome (30)
abstract/UploaderBlock.js (14)

270-272: Prefer for...of instead of forEach.


270-270: The computed expression can be simplified without the use of a string literal.


277-277: The computed expression can be simplified without the use of a string literal.


307-307: The computed expression can be simplified without the use of a string literal.


440-443: The call chain .map().flat() can be replaced with a single .flatMap() call.


511-513: Prefer for...of instead of forEach.


525-528: Prefer for...of instead of forEach.


639-639: Avoid redundant double-negation.


70-70: This let declares a variable that is only assigned once.


249-249: This let declares a variable that is only assigned once.


481-481: This let declares a variable that is only assigned once.


484-484: This let declares a variable that is only assigned once.


602-602: This let declares a variable that is only assigned once.


635-635: This let declares a variable that is only assigned once.

blocks/Config/Config.js (6)

61-61: Template literals are preferred over string concatenation.


166-166: Do not use template literals if interpolation and special-character handling are not needed.


121-121: This aliasing of this is unnecessary.


144-144: This aliasing of this is unnecessary.


176-176: This aliasing of this is unnecessary.


222-222: This aliasing of this is unnecessary.

types/exported.d.ts (8)

19-19: Unexpected any. Specify a different type.


20-20: Unexpected any. Specify a different type.


278-279: This empty export is useless because there's another export or import.


1-1: All these imports are only used as types.


1-2: All these imports are only used as types.


2-3: All these imports are only used as types.


111-111: Don't use '{}' as a type.


279-279: All exports are only types and should thus use export type.

utils/validators/file/validateUploadError.js (2)

20-35: This else clause can be omitted because previous branches break early.


27-35: This else clause can be omitted because previous branches break early.

Additional comments not posted (19)
utils/validators/collection/validateCollectionUploadError.js (1)

3-11: The implementation of validateCollectionUploadError aligns well with the PR's objectives to enhance validation mechanisms. The use of buildCollectionFileError to handle error scenarios is appropriate.

utils/validators/file/validateMaxSizeLimit.js (1)

5-16: The validateMaxSizeLimit function is correctly implemented to check for file size limits and handle errors appropriately. Good use of configuration values and error handling.

utils/validators/file/validateIsImage.js (1)

5-27: The validateIsImage function is well-implemented, handling various edge cases effectively and aligning with the PR's objectives to enhance file validation mechanisms.

utils/validators/collection/validateMultiple.js (1)

3-38: The validateMultiple function is correctly implemented to enforce file count limits within collections, using configuration values effectively and handling errors appropriately.

utils/validators/file/validateFileType.js (1)

5-31: The validateFileType function is well-implemented, using a combination of mime type and file extension checks to ensure that only allowed file types are processed. Good handling of edge cases and error scenarios.

abstract/ValidationManager.js (6)

1-1: Ensure TypeScript checks are enabled for better type safety.


2-9: Imports are well-organized and relevant to the functionality of the ValidationManager.


11-27: Type definitions for FuncFileValidator and FuncCollectionValidator are clear and well-documented.


29-59: The ValidationManager class is well-structured with clear separation of concerns. The constructor initializes necessary properties and subscribes to configuration changes effectively.


73-101: The runCollectionValidators method aggregates errors from collection validators and handles different types of return values (single error or array of errors) correctly. It also emits an event if there are any errors, which is a good practice for asynchronous error handling.


103-134: Private methods _runFileValidatorsForEntry, _runCustomValidatorsEntry, and _commonValidation are well encapsulated and maintain clean separation of concerns within the class. They also make good use of the provided validators.

blocks/Config/normalizeConfigValue.js (2)

77-88: The asValidators function correctly checks if the input is an array and throws an appropriate error if not. This is crucial for ensuring that the validators configuration is always in the expected format.


156-157: The mapping for fileValidators and collectionValidators uses the asValidators function to ensure type safety. This is a good practice as it prevents configuration errors from propagating further into the application.

blocks/Config/Config.js (1)

Line range hint 22-33: The addition of fileValidators and collectionValidators to complexConfigKeys ensures that these configurations are treated appropriately as complex types, which is necessary for their correct handling in the configuration system.

types/exported.d.ts (1)

75-76: The type definitions for fileValidators and collectionValidators are correctly nullable, allowing for configurations where validators may not be provided.

abstract/UploaderBlock.js (4)

23-24: Proper use of the ValidationManager import to facilitate new validation logic within the UploaderBlock.


77-79: Initialization of ValidationManager only if it has not been previously added is a good practice to avoid unnecessary re-instantiations.


86-89: Getter for validationManager is concise and correctly handles the case where the manager might not be present.


397-400: The integration of ValidationManager methods within the _handleCollectionUpdate method ensures that validations are run at appropriate times, enhancing the robustness of file handling.

abstract/ValidationManager.js Outdated Show resolved Hide resolved
types/exported.d.ts Outdated Show resolved Hide resolved
utils/validators/file/validateUploadError.js Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range and nitpick comments (1)
types/exported.d.ts (1)

Line range hint 279-279: Remove the empty export statement as it serves no purpose.

- export {};
Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 2741c59 and 052fd05.
Files selected for processing (3)
  • blocks/Config/normalizeConfigValue.js (2 hunks)
  • types/exported.d.ts (3 hunks)
  • utils/validators/file/validateUploadError.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • blocks/Config/normalizeConfigValue.js
Additional Context Used
Biome (7)
types/exported.d.ts (6)

278-279: This empty export is useless because there's another export or import.


1-1: All these imports are only used as types.


1-2: All these imports are only used as types.


2-3: All these imports are only used as types.


111-111: Don't use '{}' as a type.


279-279: All exports are only types and should thus use export type.

utils/validators/file/validateUploadError.js (1)

20-27: This else clause can be omitted because previous branches break early.

utils/validators/file/validateUploadError.js Outdated Show resolved Hide resolved
types/exported.d.ts Outdated Show resolved Hide resolved
@egordidenko egordidenko force-pushed the feat/custom-validate-func branch from 052fd05 to 8412b19 Compare May 29, 2024 22:11
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (3)
types/exported.d.ts (3)

Line range hint 278-279: Remove the empty export statement as it serves no purpose.

- export {};

Line range hint 279-279: Convert all exports to export type since they are only type definitions.

- export type UploadError = import('@uploadcare/upload-client').UploadError;
- export type UploadcareFile = import('@uploadcare/upload-client').UploadcareFile;
- export type NetworkError = import('@uploadcare/upload-client').NetworkError;
- export type UploadcareGroup = import('@uploadcare/upload-client').UploadcareGroup;
- export type Metadata = import('@uploadcare/upload-client').Metadata;
- export type MetadataCallback = (fileEntry: OutputFileEntry) => Promise<Metadata> | Metadata;
- export type LocaleDefinitionOverride = Record<string, LocaleDefinition>;
- export type SecureDeliveryProxyUrlResolver = (
+ export type UploadError = import('@uploadcare/upload-client').UploadError;
+ export type UploadcareFile = import('@uploadcare/upload-client').UploadcareFile;
+ export type NetworkError = import('@uploadcare/upload-client').NetworkError;
+ export type UploadcareGroup = import('@uploadcare/upload-client').UploadcareGroup;
+ export type Metadata = import('@uploadcare/upload-client').Metadata;
+ export type MetadataCallback = (fileEntry: OutputFileEntry) => Promise<Metadata> | Metadata;
+ export type LocaleDefinitionOverride = Record<string, LocaleType>;
+ export type SecureDeliveryProxyUrlResolver = (

Line range hint 111-111: Avoid using '{}' as a type for error payloads. Specify a more appropriate type or use 'unknown' if the type is not determined.

- SOME_FILES_HAS_ERRORS: {};
+ SOME_FILES_HAS_ERRORS: unknown;
Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 052fd05 and 8412b19.
Files selected for processing (3)
  • blocks/Config/normalizeConfigValue.js (2 hunks)
  • types/exported.d.ts (3 hunks)
  • utils/validators/file/validateUploadError.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • blocks/Config/normalizeConfigValue.js
  • utils/validators/file/validateUploadError.js
Additional Context Used
Biome (3)
types/exported.d.ts (3)

278-279: This empty export is useless because there's another export or import.


111-111: Don't use '{}' as a type.


279-279: All exports are only types and should thus use export type.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range and nitpick comments (1)
types/exported.d.ts (1)

Line range hint 114-114: Don't use '{}' as a type. Specify a more precise type or use 'unknown' if the exact type cannot be determined.

- SOME_FILES_HAS_ERRORS: {};
+ SOME_FILES_HAS_ERRORS: Record<string, unknown>;
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8412b19 and 3982b60.

Files selected for processing (10)
  • abstract/CTX.js (1 hunks)
  • abstract/ValidationManager.js (1 hunks)
  • abstract/buildOutputCollectionState.js (1 hunks)
  • types/exported.d.ts (11 hunks)
  • utils/validators/collection/validateCollectionUploadError.js (1 hunks)
  • utils/validators/collection/validateMultiple.js (1 hunks)
  • utils/validators/file/validateFileType.js (1 hunks)
  • utils/validators/file/validateIsImage.js (1 hunks)
  • utils/validators/file/validateMaxSizeLimit.js (1 hunks)
  • utils/validators/file/validateUploadError.js (1 hunks)
Files skipped from review as they are similar to previous changes (7)
  • abstract/ValidationManager.js
  • utils/validators/collection/validateCollectionUploadError.js
  • utils/validators/collection/validateMultiple.js
  • utils/validators/file/validateFileType.js
  • utils/validators/file/validateIsImage.js
  • utils/validators/file/validateMaxSizeLimit.js
  • utils/validators/file/validateUploadError.js
Additional context used
Biome
types/exported.d.ts

[error] 283-284: This empty export is useless because there's another export or import.


[error] 114-114: Don't use '{}' as a type.


[error] 284-284: All exports are only types and should thus use export type.

Additional comments not posted (5)
abstract/CTX.js (1)

30-30: Update the type annotation to match the new ValidationManager structure.

Verification successful

The type OutputCollectionErrorType is defined and used in multiple places within the codebase, including ValidationManager. The definition is found in types/exported.d.ts and it is used in various files such as abstract/CTX.js, abstract/uploadEntrySchema.js, and abstract/ValidationManager.js.

  • types/exported.d.ts: Defines OutputCollectionErrorType.
  • abstract/CTX.js: Uses OutputCollectionErrorType in a type annotation.
  • abstract/ValidationManager.js: Uses OutputCollectionErrorType in type definitions and function signatures.

This confirms that the type annotation in abstract/CTX.js is consistent with the ValidationManager structure.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the type `OutputCollectionErrorType` is correctly defined and used in the `ValidationManager`.

# Test: Search for the type definition and usage. Expect: Correct definition and usage.
ast-grep --lang javascript --pattern $'type OutputCollectionErrorType { $$$ }'

Length of output: 540



Script:

#!/bin/bash
# Description: Verify that the type `OutputCollectionErrorType` is correctly defined and used in the `ValidationManager`.

# Test 1: Search for the type definition of `OutputCollectionErrorType`.
ast-grep --lang javascript --pattern 'type OutputCollectionErrorType = $_'

# Test 2: Search for the usage of `OutputCollectionErrorType` in the codebase.
rg 'OutputCollectionErrorType'

Length of output: 1731

abstract/buildOutputCollectionState.js (1)

46-46: Ensure the errors function correctly handles the new error types from ValidationManager.

types/exported.d.ts (3)

1-3: Correctly marked imports used only for types with import type to optimize bundling.


19-20: The use of any in type definitions for FileValidator and CollectionValidator reduces type safety. Consider specifying more precise types or at least using unknown if the exact types cannot be determined.

- export type FileValidator = FuncFileValidator | any;
- export type CollectionValidator = FuncCollectionValidator | any;
+ export type FileValidator = FuncFileValidator | unknown;
+ export type CollectionValidator = FuncCollectionValidator | unknown;

Likely invalid or redundant comment.


75-76: Ensure that fileValidators and collectionValidators are properly initialized and used throughout the codebase.

@nd0ut
Copy link
Member

nd0ut commented Jun 4, 2024

  • Do not pass internal entry to the file validator
  • Also I think it's better to rename block argument to the ctx or or (like lr-upload-ctx-provider)
  • Do not pass internal collection to the collection validator, use outputCollectionEntry instead.
  • Validate custom validator's returned value
  • Wrap each validator into the try/catch and skip it on fall with the console warning
  • Set default type for custom validation errors, I think it could be easily done with wrapping around the original validator like this:
(this._blockInstance.cfg.fileValidators ?? []).map(fn => (...args) => ({...fn(...args), type: 'CUSTOM_TYPE'}))

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range and nitpick comments (1)
types/test/lr-config.test-d.tsx (1)

74-92: The addition of custom validators maxSize and maxCollection is correctly implemented. The structure and type usage are consistent with the new validation system.

Consider adding comments to describe the purpose of each validator for better maintainability.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3982b60 and 02836fa.

Files selected for processing (10)
  • abstract/ValidationManager.js (1 hunks)
  • blocks/Config/normalizeConfigValue.js (2 hunks)
  • types/exported.d.ts (11 hunks)
  • types/test/lr-config.test-d.tsx (2 hunks)
  • utils/validators/collection/validateCollectionUploadError.js (1 hunks)
  • utils/validators/collection/validateMultiple.js (1 hunks)
  • utils/validators/file/validateFileType.js (1 hunks)
  • utils/validators/file/validateIsImage.js (1 hunks)
  • utils/validators/file/validateMaxSizeLimit.js (1 hunks)
  • utils/validators/file/validateUploadError.js (1 hunks)
Files skipped from review as they are similar to previous changes (8)
  • abstract/ValidationManager.js
  • blocks/Config/normalizeConfigValue.js
  • utils/validators/collection/validateCollectionUploadError.js
  • utils/validators/collection/validateMultiple.js
  • utils/validators/file/validateFileType.js
  • utils/validators/file/validateIsImage.js
  • utils/validators/file/validateMaxSizeLimit.js
  • utils/validators/file/validateUploadError.js
Additional context used
Biome
types/exported.d.ts

[error] 283-284: This empty export is useless because there's another export or import. (lint/complexity/noUselessEmptyExport)

This import makes useless the empty export.

Safe fix: Remove this useless empty export.


[error] 114-114: Don't use '{}' as a type. (lint/complexity/noBannedTypes)

Prefer explicitly define the object shape. '{}' means "any non-nullable value".

Additional comments not posted (3)
types/test/lr-config.test-d.tsx (1)

5-5: Import of FuncCollectionValidator and FuncFileValidator looks correct and aligns with the new validation logic.

types/exported.d.ts (2)

1-3: Correct use of import type for LocaleDefinition, complexConfigKeys, and validator types. This optimizes bundling by ensuring these imports are only used for type checking.


19-20: The type definitions for FileValidator and CollectionValidator as arrays of function validators are appropriate and align with the new validation system.

abstract/ValidationManager.js Outdated Show resolved Hide resolved
abstract/ValidationManager.js Outdated Show resolved Hide resolved
abstract/ValidationManager.js Outdated Show resolved Hide resolved
abstract/ValidationManager.js Outdated Show resolved Hide resolved
abstract/ValidationManager.js Outdated Show resolved Hide resolved
abstract/ValidationManager.js Outdated Show resolved Hide resolved
types/exported.d.ts Outdated Show resolved Hide resolved
types/exported.d.ts Outdated Show resolved Hide resolved
abstract/ValidationManager.js Outdated Show resolved Hide resolved
@egordidenko egordidenko force-pushed the feat/custom-validate-func branch from 02836fa to a9b3505 Compare June 6, 2024 18:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range and nitpick comments (2)
blocks/UploadList/UploadList.js (1)

Line range hint 14-14: Consider binding methods in the constructor instead of using arrow functions in class properties.

This can enhance performance by avoiding the creation of new function instances on each component instantiation.

types/exported.d.ts (1)

Line range hint 116-116: Replace {} with unknown in type definitions for better type safety.

- SOME_FILES_HAS_ERRORS: {};
+ SOME_FILES_HAS_ERRORS: unknown;
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 02836fa and a9b3505.

Files selected for processing (9)
  • abstract/CTX.js (1 hunks)
  • abstract/ValidationManager.js (1 hunks)
  • abstract/buildOutputCollectionState.js (1 hunks)
  • blocks/Config/normalizeConfigValue.js (2 hunks)
  • blocks/UploadList/UploadList.js (1 hunks)
  • types/exported.d.ts (11 hunks)
  • types/test/lr-config.test-d.tsx (2 hunks)
  • utils/validators/collection/validateCollectionUploadError.js (1 hunks)
  • utils/validators/collection/validateMultiple.js (1 hunks)
Files skipped from review as they are similar to previous changes (7)
  • abstract/CTX.js
  • abstract/ValidationManager.js
  • abstract/buildOutputCollectionState.js
  • blocks/Config/normalizeConfigValue.js
  • types/test/lr-config.test-d.tsx
  • utils/validators/collection/validateCollectionUploadError.js
  • utils/validators/collection/validateMultiple.js
Additional context used
Biome
types/exported.d.ts

[error] 289-290: This empty export is useless because there's another export or import. (lint/complexity/noUselessEmptyExport)

This import makes useless the empty export.

Safe fix: Remove this useless empty export.


[error] 116-116: Don't use '{}' as a type. (lint/complexity/noBannedTypes)

Prefer explicitly define the object shape. '{}' means "any non-nullable value".

Additional comments not posted (4)
blocks/UploadList/UploadList.js (3)

82-82: The implementation of throttling in _throttledHandleCollectionUpdate is efficient and correctly checks component connectivity before proceeding.


82-82: The logic in _updateUploadsState for managing button states and upload progress is clear and well-implemented.


Line range hint 14-14: TODO: Address potential performance issues with many files.

Consider profiling the application to determine if the performance impact is significant and explore optimization strategies if necessary.

types/exported.d.ts (1)

1-3: Correct use of import type for type-only imports ensures they are erased during TypeScript compilation, optimizing the final bundle size.

);

export { EventType, EventPayload } from '../blocks/UploadCtxProvider/EventEmitter';

export {};
export { };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the redundant empty export statement to clean up the code.

- export { };
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
export { };
Tools
Biome

[error] 289-290: This empty export is useless because there's another export or import. (lint/complexity/noUselessEmptyExport)

This import makes useless the empty export.

Safe fix: Remove this useless empty export.

@egordidenko egordidenko force-pushed the feat/custom-validate-func branch from a9b3505 to e4c3d22 Compare June 6, 2024 21:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
types/exported.d.ts (1)

Line range hint 116-116: Avoid using '{}' as a type since it implies any non-nullish value, reducing type safety.

- SOME_FILES_HAS_ERRORS: {};
+ SOME_FILES_HAS_ERRORS: Record<string, unknown>;
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a9b3505 and e4c3d22.

Files selected for processing (9)
  • abstract/CTX.js (1 hunks)
  • abstract/ValidationManager.js (1 hunks)
  • abstract/buildOutputCollectionState.js (1 hunks)
  • blocks/Config/normalizeConfigValue.js (2 hunks)
  • blocks/UploadList/UploadList.js (1 hunks)
  • types/exported.d.ts (11 hunks)
  • types/test/lr-config.test-d.tsx (2 hunks)
  • utils/validators/collection/validateCollectionUploadError.js (1 hunks)
  • utils/validators/collection/validateMultiple.js (1 hunks)
Files skipped from review as they are similar to previous changes (8)
  • abstract/CTX.js
  • abstract/ValidationManager.js
  • abstract/buildOutputCollectionState.js
  • blocks/Config/normalizeConfigValue.js
  • blocks/UploadList/UploadList.js
  • types/test/lr-config.test-d.tsx
  • utils/validators/collection/validateCollectionUploadError.js
  • utils/validators/collection/validateMultiple.js
Additional context used
Biome
types/exported.d.ts

[error] 293-294: This empty export is useless because there's another export or import. (lint/complexity/noUselessEmptyExport)

This import makes useless the empty export.

Safe fix: Remove this useless empty export.


[error] 116-116: Don't use '{}' as a type. (lint/complexity/noBannedTypes)

Prefer explicitly define the object shape. '{}' means "any non-nullable value".

Additional comments not posted (5)
types/exported.d.ts (5)

1-3: Use import type for imports used only in type positions to optimize bundling.


5-5: Exporting types directly from another module is efficient and clear.


21-22: The renaming of FileValidators and CollectionValidators to plural forms aligns with their usage as arrays of validators.


294-294: Remove the redundant empty export statement to clean up the code.

Tools
Biome

[error] 293-294: This empty export is useless because there's another export or import. (lint/complexity/noUselessEmptyExport)

This import makes useless the empty export.

Safe fix: Remove this useless empty export.


77-78: Ensure that fileValidators and collectionValidators are properly initialized and used in the configuration to prevent runtime errors.

Verification successful

The search results indicate that fileValidators and collectionValidators are indeed initialized and used in various configuration files, including blocks/Config/normalizeConfigValue.js, blocks/Config/Config.js, blocks/Config/initialConfig.js, and abstract/ValidationManager.js. This confirms that they are properly integrated into the configuration.

  • blocks/Config/normalizeConfigValue.js: Validators are typed and imported.
  • blocks/Config/Config.js: Validators are listed as configuration properties.
  • blocks/Config/initialConfig.js: Validators are initialized as empty arrays.
  • abstract/ValidationManager.js: Validators are assigned specific validation functions.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the initialization and usage of `fileValidators` and `collectionValidators`.

# Test: Search for the initialization and usage of these validators. Expect: Proper initialization and usage.
rg --type typescript $'fileValidators|collectionValidators'

Length of output: 95



Script:

#!/bin/bash
# Description: Verify the initialization and usage of `fileValidators` and `collectionValidators`.

# Test: Search for the initialization and usage of these validators. Expect: Proper initialization and usage.
rg --type ts 'fileValidators|collectionValidators'

Length of output: 324



Script:

#!/bin/bash
# Description: Verify the initialization and usage of `fileValidators` and `collectionValidators` in configuration files.

# Test: Search for the initialization and usage of these validators in configuration files. Expect: Proper initialization and usage.
rg 'fileValidators|collectionValidators' --glob '!**/test/**'

Length of output: 1380

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e4c3d22 and 13023dc.

Files selected for processing (1)
  • demo/validators.html (1 hunks)
Additional comments not posted (2)
demo/validators.html (2)

1-17: HTML structure and resource links are correctly set up for modern web applications.


18-22: Script for registering blocks is correctly implemented.

Comment on lines 29 to 57
<script type="module">
const config = document.querySelector("lr-config")

const imagesOnly = (fileInfo, block) => {
if (!fileInfo.isImage) {
return {
message: "This isn't image",
payload: {
entry: fileInfo
}
}
}
}

config.fileValidators = [imagesOnly]

config.collectionValidators = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom validator imagesOnly lacks a return statement when the file is an image, which might lead to undefined behavior.

	const imagesOnly = (fileInfo, block) => {
		if (!fileInfo.isImage) {
			return {
				message: "This isn't image",
				payload: {
					entry: fileInfo
				}
			}
		}
+		return true; // Assuming true represents a successful validation
	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
<script type="module">
const config = document.querySelector("lr-config")
const imagesOnly = (fileInfo, block) => {
if (!fileInfo.isImage) {
return {
message: "This isn't image",
payload: {
entry: fileInfo
}
}
}
}
config.fileValidators = [imagesOnly]
config.collectionValidators = []
<script type="module">
const config = document.querySelector("lr-config")
const imagesOnly = (fileInfo, block) => {
if (!fileInfo.isImage) {
return {
message: "This isn't image",
payload: {
entry: fileInfo
}
}
}
return true; // Assuming true represents a successful validation
}
config.fileValidators = [imagesOnly]
config.collectionValidators = []

@egordidenko egordidenko force-pushed the feat/custom-validate-func branch from 13023dc to a5aac80 Compare June 7, 2024 20:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 13023dc and a5aac80.

Files selected for processing (1)
  • demo/validators.html (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • demo/validators.html

@egordidenko egordidenko force-pushed the feat/custom-validate-func branch from a5aac80 to ff6aa1a Compare June 10, 2024 13:05
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a5aac80 and ff6aa1a.

Files selected for processing (1)
  • demo/validators.html (1 hunks)
Additional comments not posted (2)
demo/validators.html (2)

1-36: HTML structure and meta tags are correctly set up for modern web applications.


18-35: JavaScript imports and locale definitions are correctly implemented.

Comment on lines +44 to +53
const imagesOnly = (fileInfo, ctx) => {
if (fileInfo.isImage) {
return {
message: ctx.l10n('file-is-not-an-image-type'),
payload: {
entry: fileInfo
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the inverted logic in the imagesOnly validator and add a return statement for valid cases.

	const imagesOnly = (fileInfo, ctx) => {
-		if (fileInfo.isImage) {
+		if (!fileInfo.isImage) {
			return {
				message: ctx.l10n('file-is-not-an-image-type'),
				payload: {
					entry: fileInfo
				}
			}
		}
+		return true; // Assuming true represents a successful validation
	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const imagesOnly = (fileInfo, ctx) => {
if (fileInfo.isImage) {
return {
message: ctx.l10n('file-is-not-an-image-type'),
payload: {
entry: fileInfo
}
}
}
}
const imagesOnly = (fileInfo, ctx) => {
if (!fileInfo.isImage) {
return {
message: ctx.l10n('file-is-not-an-image-type'),
payload: {
entry: fileInfo
}
}
}
return true; // Assuming true represents a successful validation
}

@egordidenko egordidenko force-pushed the feat/custom-validate-func branch from ff6aa1a to de53a9a Compare June 10, 2024 21:46
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ff6aa1a and de53a9a.

Files selected for processing (2)
  • abstract/ValidationManager.js (1 hunks)
  • demo/validators.html (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • abstract/ValidationManager.js
  • demo/validators.html

@nd0ut nd0ut self-requested a review June 11, 2024 10:41
@egordidenko egordidenko merged commit d3260b0 into main Jun 14, 2024
1 check passed
@egordidenko egordidenko deleted the feat/custom-validate-func branch June 14, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants