Update assets for chunk test #8740
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a new property, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CVATCore
participant Config
participant Frames
User->>CVATCore: Request job metadata
CVATCore->>Config: Get jobMetaDataReloadPeriod
Config-->>CVATCore: Return 1 hour in milliseconds
CVATCore->>Frames: Check if metadata is outdated
Frames->>Frames: Use jobMetaDataReloadPeriod for check
Frames-->>CVATCore: Return metadata status
CVATCore-->>User: Provide job metadata
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🧹 Outside diff range and nitpick comments (5)
cvat-core/src/config.ts (2)
22-22: Remove unnecessary blank lineThis blank line doesn't serve any purpose and can be removed.
requestsStatusDelay: null, - jobMetaDataReloadPeriod: 1 * 60 * 60 * 1000, // 1 hour
23-23: Consider using time constants for better maintainabilityThe calculation is correct, but consider using time constants to make it more maintainable and less error-prone.
- jobMetaDataReloadPeriod: 1 * 60 * 60 * 1000, // 1 hour + // Time constants + MILLISECONDS_PER_SECOND: 1000, + SECONDS_PER_MINUTE: 60, + MINUTES_PER_HOUR: 60, + jobMetaDataReloadPeriod: MINUTES_PER_HOUR * SECONDS_PER_MINUTE * MILLISECONDS_PER_SECOND, // 1 hourAlternatively, you could use a more concise approach:
- jobMetaDataReloadPeriod: 1 * 60 * 60 * 1000, // 1 hour + HOUR_IN_MS: 3600000, // 1 hour in milliseconds + jobMetaDataReloadPeriod: HOUR_IN_MS,cvat-core/src/index.ts (1)
190-190: Add documentation for the new configuration property.Please add JSDoc comments to document the purpose, usage, and default value of the
jobMetaDataReloadPeriodproperty.Add documentation above the property:
+ /** + * The period (in milliseconds) after which job metadata should be reloaded. + * Used to determine if frame metadata is outdated and needs refreshing. + * @default 3600000 (1 hour) + */ jobMetaDataReloadPeriod: typeof config.jobMetaDataReloadPeriod;cvat-core/src/api.ts (1)
323-328: Consider adding validation and documentation for jobMetaDataReloadPeriodWhile the implementation follows the established pattern, consider these improvements:
- Add validation in the setter to ensure the value is:
- A positive number
- Within reasonable bounds (e.g., not too short to avoid excessive reloads)
- Add JSDoc comments to document:
- The purpose of this configuration
- The expected value type
- The unit of measurement (milliseconds)
- Any constraints on valid values
Example implementation:
+ /** + * Gets the period (in milliseconds) for reloading job metadata + * @returns {number} Reload period in milliseconds + */ get jobMetaDataReloadPeriod() { return config.jobMetaDataReloadPeriod; }, + /** + * Sets the period for reloading job metadata + * @param {number} value - Period in milliseconds (minimum: 1000) + * @throws {ArgumentError} If value is not a positive number or too small + */ set jobMetaDataReloadPeriod(value) { + if (typeof value !== 'number' || value < 1000) { + throw new ArgumentError( + 'Job metadata reload period must be a number >= 1000 milliseconds' + ); + } config.jobMetaDataReloadPeriod = value; },cvat-core/src/frames.ts (1)
601-601: Consider adding documentation for the configurable period.The change from a hardcoded constant to a configurable period is good. However, it would be helpful to add a comment explaining the purpose and expected format of
jobMetaDataReloadPeriod(e.g., milliseconds).+ // Check if the job metadata cache is older than the configured reload period (in milliseconds) const isOutdated = (Date.now() - cached.metaFetchedTimestamp) > config.jobMetaDataReloadPeriod;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/mounted_file_share/bigArchive.zipis excluded by!**/*.zip
📒 Files selected for processing (4)
cvat-core/src/api.ts(1 hunks)cvat-core/src/config.ts(1 hunks)cvat-core/src/frames.ts(2 hunks)cvat-core/src/index.ts(1 hunks)
🔇 Additional comments (4)
cvat-core/src/index.ts (1)
190-190: Verify the usage of jobMetaDataReloadPeriod.
The property addition looks good. Let's verify its usage across the codebase to ensure it's properly integrated.
✅ Verification successful
The jobMetaDataReloadPeriod property is properly integrated and used in the codebase
The property is well integrated across the codebase:
- Defined in
config.tswith a default value of 1 hour - Exposed through getter/setter in
api.ts - Actively used in
frames.tsto determine if cached frame metadata is outdated - Properly typed in
index.ts
The implementation shows that this property controls the refresh period of job metadata caching, which is correctly utilized in the frame data fetching logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of jobMetaDataReloadPeriod across the codebase
# Search for direct references to the property
echo "Direct references to jobMetaDataReloadPeriod:"
rg "jobMetaDataReloadPeriod" -A 2
# Search for frame metadata refresh logic that might use this setting
echo "\nFrame metadata refresh related code:"
rg "refreshJobCache|getFrameMeta" -A 5
Length of output: 3186
cvat-core/src/api.ts (1)
323-328: Verify the usage of jobMetaDataReloadPeriod in the codebase
Let's ensure this configuration is properly utilized where frame metadata freshness is checked.
This will help verify:
- All places where this configuration is used
- Integration with frame metadata freshness checks
- Potential areas that might need updates
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the usage of jobMetaDataReloadPeriod in the codebase.
Configuration property is properly integrated and used for frame metadata caching
The jobMetaDataReloadPeriod property is well-integrated into the codebase:
- Defined in
config.tswith a default value of 1 hour - Used in
frames.tsto determine if cached frame metadata is outdated - Properly exposed through the API interface in
api.ts - Type definitions are correctly maintained in
index.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find where jobMetaDataReloadPeriod is used in the codebase
# Search for direct references to the property
echo "Searching for direct references to jobMetaDataReloadPeriod..."
rg "jobMetaDataReloadPeriod" --type ts
# Search for frame metadata freshness checks
echo "Searching for frame metadata freshness logic..."
rg -A 5 "getFrameMeta|isMetadataFresh" --type ts
Length of output: 1816
cvat-core/src/frames.ts (2)
15-15: LGTM!
Clean addition of the config import.
601-601: Verify configuration property setup.
Let's verify the setup and documentation of the new jobMetaDataReloadPeriod configuration property.
✅ Verification successful
Configuration property is properly initialized and accessible
The jobMetaDataReloadPeriod configuration property is:
- Properly defined in
config.tswith a default value of 1 hour (1 * 60 * 60 * 1000 milliseconds) - Exposed through getter/setter methods in
api.ts - Correctly typed in
index.ts - Used appropriately in
frames.tsfor cache invalidation
The implementation is complete and consistent across the codebase. While there's no explicit documentation, the code is self-documenting with the default value comment indicating the time period.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the configuration setup and documentation for jobMetaDataReloadPeriod
# Check where the config property is defined
echo "Checking config definition..."
rg -l "jobMetaDataReloadPeriod" --type ts
# Check for any documentation of this configuration
echo "Checking documentation..."
fd -e md -e rst -e txt . | xargs rg -l "jobMetaDataReloadPeriod"
# Check for any potential missed usages of similar reload periods
echo "Checking for similar patterns..."
rg "reloadPeriod|META_DATA_RELOAD|RELOAD_PERIOD" --type ts
Length of output: 464
Script:
#!/bin/bash
# Let's check the actual implementation and initialization of this config property
# Check the config definition
echo "Config definition in config.ts:"
rg -A 5 -B 5 "jobMetaDataReloadPeriod" cvat-core/src/config.ts
# Check how it's used in index.ts
echo "\nUsage in index.ts:"
rg -A 5 -B 5 "jobMetaDataReloadPeriod" cvat-core/src/index.ts
# Check the API usage
echo "\nUsage in api.ts:"
rg -A 5 -B 5 "jobMetaDataReloadPeriod" cvat-core/src/api.ts
# Look for any default value assignments
echo "\nChecking for default value assignments:"
ast-grep --pattern 'jobMetaDataReloadPeriod = $_'
Length of output: 1789
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8740 +/- ##
===========================================
- Coverage 73.97% 73.93% -0.05%
===========================================
Files 409 409
Lines 43931 43934 +3
Branches 3985 3985
===========================================
- Hits 32498 32482 -16
- Misses 11433 11452 +19
|
|
Motivation and context
jobMetaDataReloadPeriodHow has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Improvements