feat(stats): Chart Migration Five #1147
Conversation
WalkthroughThis pull request introduces several changes across various configuration and implementation files to enhance transaction tracking and data management capabilities. A new chart titled "Daily transactions" has been added to the Possibly related issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
🪧 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 (
|
bragov4ik
force-pushed
the
bragov4ik/stats-migrate-backend-charts-5
branch
from
db42683 to
9b856fd
Compare
bragov4ik
force-pushed
the
bragov4ik/stats-migrate-backend-charts-5
branch
from
9b856fd to
2a306f9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
stats/stats/src/charts/lines/new_txns_window.rs (1)
128-128: Consider enabling the ignored testupdate_txns_window_clears_and_overwritesThe test is currently ignored with
#[ignore = "needs database to run"], which may prevent catching regressions related to database interactions. If possible, configure the test environment to allow this test to run automatically during CI/CD to ensure that the chart updates function as expected.stats/stats/src/tests/point_construction.rs (1)
18-21: Handle potential parsing errors indtfunctionThe
dtfunction usestime.parse().unwrap(), which will panic if the input string is not in the correct format. Consider handling parsing errors gracefully to improve robustness, perhaps by returning aResultor providing a clear error message.Apply this diff to handle parsing errors:
- pub fn dt(time: &str) -> NaiveDateTime { - time.parse().unwrap() - } + pub fn dt(time: &str) -> NaiveDateTime { + time.parse().expect("Invalid date-time format. Expected format: YYYY-MM-DDTHH:MM:SS") + }Alternatively, you can change the return type to
Result<NaiveDateTime, chrono::ParseError>and handle the error accordingly.stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs (1)
64-69: Documentation should emphasize data consistency risksThe documentation should warn about potential race conditions between clearing and inserting data, especially in concurrent environments.
Consider expanding the documentation:
/// Batch update "step" that clears all data for this chart /// and inserts the newly queried data. /// /// Since it removes all data each time, it makes sense to /// use it only with max batch size. +/// +/// # Warning +/// This implementation may lead to temporary data gaps in concurrent environments +/// between clearing and inserting new data. Consider using database transactions +/// if consistent reads are required. pub struct ClearAllAndPassStep;stats/config/charts.json (1)
109-112: Consider clarifying the chart titleWhile the description is clear about showing yesterday's transactions, the title "Daily transactions" might be slightly misleading as it implies current day's data. Consider updating the title to be more specific, such as "Yesterday's Transactions" or "Previous Day Transactions".
"new_txns_window": { - "title": "Daily transactions", + "title": "Previous Day Transactions", "description": "Number of transactions yesterday (0:00 - 23:59 UTC). The chart displays daily transactions for the past 30 days." },stats/stats/src/tests/mock_blockscout.rs (1)
375-447: Consider reducing code duplication in transaction mock functions.The
mock_transactionsandreindexing_mock_txnsfunctions share similar logic for transaction generation. Consider extracting the common logic into a helper function.Here's a suggested refactoring:
+fn generate_block_transactions( + block: &blocks::ActiveModel, + accounts: &[addresses::ActiveModel], + gas: i64, + num_transactions: usize, + transaction_types: &[TxType], +) -> Vec<transactions::ActiveModel> { + (0..num_transactions) + .map(|i| { + mock_transaction( + block, + gas, + (block.number.as_ref() * 1_123_456_789) % 70_000_000_000, + accounts, + i as i32, + transaction_types[i % transaction_types.len()].clone(), + ) + }) + .collect() +} fn mock_transactions( blocks: &[blocks::ActiveModel], accounts: &[addresses::ActiveModel], ) -> Vec<transactions::ActiveModel> { blocks[0..blocks.len() - 1] .iter() .filter(|b| b.number.as_ref() % 3 != 1) - .flat_map(|b| { - [ - mock_transaction( - b, - 21_000, - (b.number.as_ref() * 1_123_456_789) % 70_000_000_000, - accounts, - 0, - TxType::Transfer, - ), - mock_transaction( - b, - 21_000, - (b.number.as_ref() * 1_123_456_789) % 70_000_000_000, - accounts, - 1, - TxType::Transfer, - ), - mock_transaction( - b, - 21_000, - (b.number.as_ref() * 1_123_456_789) % 70_000_000_000, - accounts, - 2, - TxType::ContractCall, - ), - ] - }) + .flat_map(|b| generate_block_transactions( + b, + accounts, + 21_000, + 3, + &[TxType::Transfer, TxType::Transfer, TxType::ContractCall], + )) .collect() } fn reindexing_mock_txns( blocks: &[blocks::ActiveModel], accounts: &[addresses::ActiveModel], ) -> Vec<transactions::ActiveModel> { blocks[0..blocks.len() - 1] .iter() .filter(|b| b.number.as_ref() % 3 == 1) - .flat_map(|b| { - [ - mock_transaction( - b, - 23_000, - (b.number.as_ref() * 1_123_456_789) % 70_000_000_000, - accounts, - 0, - TxType::Transfer, - ), - mock_transaction( - b, - 23_000, - (b.number.as_ref() * 1_123_456_789) % 70_000_000_000, - accounts, - 1, - TxType::Transfer, - ), - ] - }) + .flat_map(|b| generate_block_transactions( + b, + accounts, + 23_000, + 2, + &[TxType::Transfer, TxType::Transfer], + )) .collect() }stats/config/layout.json (1)
39-40: Consider chart ordering from a UX perspectiveThe new "new_txns_window" chart is placed after "txns_success_rate". Consider if this ordering provides the most logical flow for users viewing the dashboard. For example, it might make more sense to group it with "new_txns" since both relate to transaction volume metrics.
Consider this alternative ordering:
"average_txn_fee", "new_txns", + "new_txns_window", "txns_fee", "txns_growth", - "txns_success_rate", - "new_txns_window" + "txns_success_rate"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
stats/config/charts.json(1 hunks)stats/config/layout.json(1 hunks)stats/config/update_groups.json(1 hunks)stats/stats-server/src/runtime_setup.rs(1 hunks)stats/stats-server/tests/it/lines.rs(3 hunks)stats/stats/src/charts/db_interaction/write.rs(1 hunks)stats/stats/src/charts/lines/mod.rs(2 hunks)stats/stats/src/charts/lines/new_txns_window.rs(1 hunks)stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs(2 hunks)stats/stats/src/tests/mock_blockscout.rs(7 hunks)stats/stats/src/tests/point_construction.rs(1 hunks)stats/stats/src/tests/simple_test.rs(1 hunks)stats/stats/src/update_group.rs(1 hunks)stats/stats/src/update_groups.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- stats/stats/src/update_group.rs
🔇 Additional comments (12)
stats/config/update_groups.json (1)
11-11: Verify the Cron expression for new_txns_window_group
Please ensure that the Cron expression "0 8 0 * * * *" is correct for your scheduler. Standard Cron expressions typically have 5 or 6 fields. Verify that this 7-field format aligns with your scheduling system's requirements to avoid any unintended scheduling issues.
stats/stats/src/charts/lines/mod.rs (1)
20-20: LGTM! Module organization follows best practices
The new module is properly integrated into the codebase, maintaining alphabetical ordering and following Rust module conventions.
Also applies to: 94-94
stats/stats-server/tests/it/lines.rs (2)
128-128: LGTM! Improved error message clarity
The updated error message is more descriptive and helpful for debugging purposes.
72-74: Ensure follow-up testing is tracked
The exclusion of newTxnsWindow from testing should be temporary. Consider creating a tracking issue to ensure comprehensive testing is implemented when the main page components are ready.
Let's check if there's already a tracking issue:
Would you like me to create a GitHub issue to track this testing requirement?
stats/stats/src/update_groups.rs (1)
42-43: LGTM! Properly integrated new chart group
The NewTxnsWindow chart is correctly added as a singleton group, following the established pattern. The comment clearly indicates its standalone nature.
stats/stats/src/tests/simple_test.rs (1)
68-68: LGTM: Making the utility function public.
The change to make chart_output_to_expected public is appropriate as it allows reuse of this test utility function across different test modules.
stats/stats-server/src/runtime_setup.rs (1)
277-277: LGTM: New update group added correctly.
The addition of NewTxnsWindowGroup follows the documented process for adding new charts and is placed appropriately with other transaction-related groups.
stats/stats/src/tests/mock_blockscout.rs (4)
Line range hint 57-96: LGTM: Good refactoring of mock data generation.
The extraction of mock data generation into separate functions improves code organization and maintainability while preserving the original functionality.
259-268: LGTM: Well-structured reindex simulation function.
The new imitate_reindex function is well-documented and properly reuses existing mock functions to simulate the reindexing process.
295-316: LGTM: Well-structured mock helper functions.
The extraction of mock_blocks and mock_addresses into separate functions improves code reusability and maintainability.
Also applies to: 338-342
646-680: LGTM: Well-structured test for transaction overlap.
The new test module effectively verifies that reindexing doesn't create overlapping transactions, which is crucial for the daily transactions chart functionality.
stats/config/layout.json (1)
39-40: Verify chart ID consistency across configuration files
The addition of "new_txns_window" to the transactions category is appropriate. Let's verify the chart ID matches in other configuration files.
✅ Verification successful
Chart ID configuration is consistent across files
The "new_txns_window" chart is properly configured:
- Defined in
charts.jsonwith appropriate title and description - Referenced in
update_groups.jsonfor data collection - Correctly placed in
layout.json - Implementation exists in
stats/stats/src/charts/lines/new_txns_window.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the chart ID exists in charts.json and is referenced correctly in update_groups.json
# Check if the chart ID exists in charts.json
echo "Checking charts.json for new_txns_window:"
rg -A 5 '"id":\s*"new_txns_window"' stats/config/charts.json
# Check if the chart ID is referenced in update_groups.json
echo "Checking update_groups.json for new_txns_window references:"
rg "new_txns_window" stats/config/update_groups.json
Length of output: 387
Script:
#!/bin/bash
# Let's check the content of charts.json to see if the chart configuration exists
echo "Content of charts.json:"
cat stats/config/charts.json
# Also check if there are any other references to new_txns_window in the codebase
echo -e "\nSearching for other references to new_txns_window:"
rg "new_txns_window" -l
Length of output: 7980
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs
Outdated
Show resolved
Hide resolved
bragov4ik
changed the title
bragov4ik
changed the title
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/cumulative.rs (1)
32-43: LGTM! Consider documenting transaction behavior.The implementation correctly handles generic connections while maintaining cumulative calculation logic. Given this is used for transaction charts, consider adding documentation about transaction consistency expectations.
Add a doc comment explaining that transaction consistency is not critical in this context, as indicated by the project's learnings. This helps future maintainers understand the design decision.
pub struct AddLastValueStep<ChartProps>(PhantomData<ChartProps>); +/// Note: This implementation does not require strict transaction consistency +/// as temporary inconsistencies are acceptable in the context of chart updates. impl<Resolution, Value, ChartProps> BatchStepBehaviour<Resolution, Vec<TimespanValue<Resolution, Value>>, ()> for AddLastValueStep<ChartProps>stats/stats/src/data_source/tests.rs (1)
Line range hint
224-235: Consider implementing the test behaviorThe current implementation uses
todo!()and marks thePassVecStepcall as unreachable. Consider implementing the actual test behavior to ensure proper coverage.Would you like me to help implement the test behavior or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameter_traits.rs(2 hunks)stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/cumulative.rs(2 hunks)stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mock.rs(2 hunks)stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs(3 hunks)stats/stats/src/data_source/tests.rs(2 hunks)
🧰 Additional context used
📓 Learnings (2)
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mock.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1147
File: stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs:76-98
Timestamp: 2024-12-11T15:25:33.248Z
Learning: In the `ClearAllAndPassStep::batch_update_values_step_with` function in `stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs`, it's acceptable to perform the clear and insert operations separately without wrapping them in a transaction, as inconsistency is acceptable in this context, and the insert step is very unlikely to fail unless the service is down.
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1147
File: stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs:76-98
Timestamp: 2024-12-11T15:25:33.248Z
Learning: In the `ClearAllAndPassStep::batch_update_values_step_with` function in `stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs`, it's acceptable to perform the clear and insert operations separately without wrapping them in a transaction, as inconsistency is acceptable in this context, and the insert step is very unlikely to fail unless the service is down.
🔇 Additional comments (5)
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameter_traits.rs (1)
4-4: LGTM! Good improvement in trait flexibility.
The change to use generic database connections with ConnectionTrait + TransactionTrait bounds improves code flexibility while maintaining type safety.
Let's verify that all implementations handle the generic connection type correctly:
Also applies to: 20-21
✅ Verification successful
The previous pattern might have been too generic. Let's try with a more specific pattern and also locate the trait definition to ensure we're searching for the correct name.
Let's look for the actual implementations to verify how they handle the connection types.
We need to see the trait definition and all implementations to verify they properly handle the generic connection type.
Verified: Generic database connection change is properly implemented
The trait definition shows that batch_update_values_step_with correctly uses the generic type parameter C: ConnectionTrait + TransactionTrait, and the implementation in mock.rs (seen in previous results) follows this pattern. The change is consistent and properly implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of BatchStepBehaviour to ensure they're updated
ast-grep --pattern 'impl$$$BatchStepBehaviour$$$'
Length of output: 51
Script:
#!/bin/bash
# Let's try a different pattern to find implementations
ast-grep --pattern 'impl$_for$_BatchStepBehaviour'
# Also search for the trait definition to understand its full name
rg "trait.*BatchStepBehaviour" -A 5
Length of output: 4114
Script:
#!/bin/bash
# Let's check the implementations in the files we found
rg "impl.*BatchStepBehaviour" -A 10 stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mock.rs stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/cumulative.rs
# Also check for any other potential implementations
rg "impl.*for.*Batch" -A 10
Length of output: 8386
Script:
#!/bin/bash
# Let's check the trait definition and its method signatures
rg -A 20 "trait BatchStepBehaviour" stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameter_traits.rs
# And let's check the full implementation in cumulative.rs since we didn't see it in the previous results
rg -A 20 "impl.*BatchStepBehaviour" stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/cumulative.rs
Length of output: 920
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mock.rs (1)
35-46: LGTM! Correct implementation of generic connection handling.
The mock implementation properly adapts to the generic connection type while maintaining its recording functionality.
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs (2)
Line range hint 37-62: LGTM! Generic type parameter enhances flexibility
The addition of the generic type parameter C: ConnectionTrait + TransactionTrait improves code flexibility while maintaining the existing functionality.
64-69: Verify usage with max batch size
The documentation suggests this struct should only be used with max batch size. Let's verify this constraint is enforced in the usage.
✅ Verification successful
Let me search for more specific information about the batch size configuration.
Let me try one more search with a different approach to find the batch size configuration.
Constraint is properly enforced through type system
The verification shows that ClearAllAndPassStep is indeed used with BatchMaxDays, which is defined as TimespanDuration::from_days(u64::MAX). This effectively enforces the maximum batch size constraint through the type system, as seen in:
new_txns_window.rs: UsesClearAllAndPassStepwithBatchMaxDaysnew_accounts.rs: UsesBatchMaxDaysfor the base implementation- The constant
BatchMaxDaysis defined with the maximum possible value (u64::MAXdays)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of ClearAllAndPassStep to verify batch size configuration
rg -A 5 "ClearAllAndPassStep"
# Search for batch size configurations
ast-grep --pattern 'struct $name {
$$$
batch_size: $size,
$$$
}'
Length of output: 2172
Script:
#!/bin/bash
# Search for BatchMaxDays definition and usage since it appears alongside ClearAllAndPassStep
rg -A 10 "struct BatchMaxDays"
# Search for any batch size configuration in the same directory
rg -A 10 "batch_size" stats/stats/src/data_source/kinds/local_db/parameters/update/batching/
Length of output: 125
Script:
#!/bin/bash
# Search for BatchMaxDays in a broader context
rg -l "BatchMaxDays"
# Then look at the content of files that define or use it
rg "BatchMaxDays" -B 5 -A 15
# Also search for any batch configuration types
ast-grep --pattern 'struct $name {
batch_$_: $_,
}'
Length of output: 9808
stats/stats/src/data_source/tests.rs (1)
212-223: LGTM! Generic type constraints properly implemented
The implementation correctly uses the generic type parameter C with appropriate trait bounds (ConnectionTrait + TransactionTrait), aligning with the changes in the main implementation.
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
stats/stats/src/charts/lines/new_txns_window.rs (3)
1-7: Enhance documentation with rationale for excluding the last dayThe documentation clearly explains what the module does, but it would be helpful to explain why the last day is excluded (e.g., to avoid incomplete data due to ongoing transactions).
67-68: Incorrect complexity comment for sort_unstableThe comment "linear time for sorted sequences" is incorrect.
sort_unstablehas O(n log n) complexity regardless of input sequence.- // linear time for sorted sequences + // O(n log n) complexity, but faster than stable sort
123-198: Consider adding more granular test casesWhile the current test is comprehensive, consider adding smaller, focused test cases for specific behaviors:
- Edge case for exactly 30 days window
- Behavior when no transactions exist
- Error handling scenarios
This would make it easier to diagnose issues when specific behaviors fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
stats/stats/src/charts/lines/new_txns_window.rs(1 hunks)stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs(3 hunks)
🧰 Additional context used
📓 Learnings (1)
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1147
File: stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs:76-98
Timestamp: 2024-12-11T15:25:33.248Z
Learning: In the `ClearAllAndPassStep::batch_update_values_step_with` function in `stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs`, it's acceptable to perform the clear and insert operations separately without wrapping them in a transaction, as inconsistency is acceptable in this context, and the insert step is very unlikely to fail unless the service is down.
🔇 Additional comments (5)
stats/stats/src/charts/lines/new_txns_window.rs (2)
47-48:
Incorrect usage of saturating_sub on NaiveDate
The NaiveDate type does not have a saturating_sub method, which will lead to a compilation error.
Apply this diff to fix the issue:
- day_start(&update_day.saturating_sub(TimespanDuration::from_timespan_repeats(WINDOW)))
+ day_start(
+ &update_day
+ .checked_sub_signed(TimespanDuration::from_timespan_repeats(WINDOW).to_chrono_duration())
+ .unwrap_or_else(|| NaiveDate::from_ymd(1970, 1, 1))
+ )62-66: Verify query performance and security
Please ensure:
- The query is protected against SQL injection
- Performance remains acceptable with large datasets
✅ Verification successful
Based on the gathered information, I can now provide the final response:
SQL injection protection verified, performance considerations in place
The implementation is safe and optimized:
- Uses parameterized queries through sea-orm's Statement construction
- Employs the
sql_with_range_filter_optmacro which safely handles parameter binding - Query is optimized with proper date-based filtering and consensus checks
- Uses
find_by_statementwhich is a safe ORM method for executing prepared statements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for SQL injection protection and query performance considerations
# Look for SQL query construction and parameterization
rg -A 5 "get_statement|Statement::from_sql|Statement::from_string"
# Check for existing performance-related tests
rg -l "performance|bench|large.*dataset" --type rust
Length of output: 43245
Script:
#!/bin/bash
# Let's check the implementation of new_txns_window_statement to verify SQL injection protection and query construction
# First, find the implementation
ast-grep --pattern 'fn new_txns_window_statement'
# Also check the file directly for context
rg -A 10 "new_txns_window_statement" stats/stats/src/charts/lines/new_txns_window.rs
Length of output: 1002
Script:
#!/bin/bash
# Let's check the NewTxnsStatement implementation to see how the query is constructed
# First find the implementation
ast-grep --pattern 'impl NewTxnsStatement'
# Also check for any SQL query construction in the same file
rg -B 5 -A 15 'Statement::from_' stats/stats/src/charts/lines/new_txns_window.rs
Length of output: 125
Script:
#!/bin/bash
# Let's check the entire file content to understand the query construction
rg -l "NewTxnsStatement" --type rust
# Then check the content of the file containing NewTxnsStatement
rg -A 20 "struct NewTxnsStatement"
Length of output: 1963
Script:
#!/bin/bash
# Let's check the sql_with_range_filter_opt macro implementation
rg -A 10 "macro_rules! sql_with_range_filter_opt" stats/stats/src/utils.rs
# Also check if there are any indexes defined for the tables used in the query
rg -A 5 "CREATE.*INDEX.*transactions" stats/stats/migration/src/m20231201_000001_create_transactions.rs
Length of output: 637
stats/stats/src/data_source/kinds/local_db/parameters/update/batching/parameters/mod.rs (3)
Line range hint 37-62: LGTM! Well-structured generic implementation.
The change to use a generic connection type with ConnectionTrait + TransactionTrait bounds improves code flexibility and testability while maintaining the existing functionality.
64-69: LGTM! Well-documented implementation purpose.
The documentation clearly explains the clear-all-then-insert pattern and includes an important note about using it with max batch size.
76-101: Verify max batch size usage in charts configuration.
The implementation looks good, but since this struct is documented to make sense "only with max batch size", we should verify its usage.
✅ Verification successful
Let me generate additional scripts to verify the batch size configuration for the specific chart where ClearAllAndPassStep is used.
Batch size configuration is correctly used with ClearAllAndPassStep
The verification confirms that ClearAllAndPassStep is used with BatchMaxDays which is configured as TimespanDuration::from_days(u64::MAX). This is the maximum possible batch size, which aligns perfectly with the struct's documentation stating it should be "used only with max batch size". The implementation in new_txns_window.rs follows this requirement correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ClearAllAndPassStep is used with appropriate batch size
# Search for chart configurations using ClearAllAndPassStep
echo "Searching for ClearAllAndPassStep usage in chart configurations..."
rg -l "ClearAllAndPassStep" | while read -r file; do
echo "=== $file ==="
# Show context to verify batch size configuration
rg -B 5 -A 5 "ClearAllAndPassStep" "$file"
done
# Search for batch size constants to verify max values
echo -e "\nVerifying batch size constants..."
rg "batch.*size|batch_size|BatchSize"
Length of output: 5519
Script:
#!/bin/bash
# Check BatchMaxDays configuration since it's used with ClearAllAndPassStep in new_txns_window.rs
echo "Checking BatchMaxDays definition and configuration..."
rg -p "BatchMaxDays" -B 5 -A 5
# Check if there are any other charts using ClearAllAndPassStep with different batch size configurations
echo -e "\nChecking for other ClearAllAndPassStep implementations or configurations..."
ast-grep --pattern 'BatchUpdate<$_, $_, ClearAllAndPassStep, $_, $_, $_>'
Length of output: 2882
PR introduces daily transactions for the main page. It works even on not fully indexed network, because it recalculates data for the whole time segment each time.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
NewTxnsWindow.Bug Fixes
Documentation
Style
Tests