fix(ads-simulator): address code review — CLI validation, constants, …#143
Merged
Merged
Conversation
…time-window aggregation
Three issues from code review resolved:
1. CLI argument validation (brittle --output handling)
- Add USAGE string and die() helper; process.exit(1) on all error paths
- Detect --output as last arg with no path
- Detect --output followed by another flag (e.g. --output --summary-only)
- Detect mutually exclusive --summary-only + --json-only
- Detect --output combined with --summary-only or --json-only
- Reject unknown flags with clear error message
2. Centralize magic numbers as named constants
- IMPRESSIONS_BASE (10000), OPT_ITERATIONS (3), TOP_COMBOS (20), TOP_AD_COUNT (3)
- All formerly hardcoded values now reference these constants
- simulation_config now exposes top_combos_per_ad, optimization_iterations,
top_ads_selected so the JSON output is self-documenting
- Log progress strings use RAW_ADS.length and OPT_ITERATIONS, not literals
- note string in simulation_config computed dynamically from dimension lengths
3. selectTopAds time-window aggregation (arbitrary first element)
- Now iterates ALL TOP_COMBOS combinations (not just top-5) per channel
- Accumulates ROI-weighted counts for each time_bucket and day_of_week
- Selects the highest ROI-weighted time bucket as the recommended window
- Selects top-3 days by accumulated ROI weight
- reason field now reports the count of combinations used in the aggregation
https://claude.ai/code/session_01QSkvVSYsiNQ9udQ3fvVopJ
Reviewer's GuideRefactors the Telegram ads simulator script to add robust CLI argument validation, centralize simulation tuning constants, and improve the selection of top ads by aggregating time-window performance across all top combinations instead of an arbitrary subset. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/telegram-ads-simulator.js" line_range="39-48" />
<code_context>
+
+// Simulation tuning — change here to adjust the whole simulation
+const IMPRESSIONS_BASE = 10000; // Baseline impressions per ad run at bid=1.0
+const OPT_ITERATIONS = 3; // Number of copy-optimization iterations to run
+const TOP_COMBOS = 20; // Top combinations stored per ad in by_time_day_channel_persona
+const TOP_AD_COUNT = 3; // Number of top ads to select and report
</code_context>
<issue_to_address>
**issue:** Guard against OPT_ITERATIONS being set to 0 or a negative value.
This logic relies on OPT_ITERATIONS being ≥ 1: you create `optimized_versions` with `Array.from({ length: OPT_ITERATIONS })` and later access `result.optimized_versions[OPT_ITERATIONS - 1]`. If OPT_ITERATIONS is ever set to 0 or negative, `optimized_versions` will be empty and `selectTopAds` will hit `undefined.metrics` at runtime. Consider either validating OPT_ITERATIONS (e.g., fail fast if < 1) or handling an empty `optimized_versions` array when selecting top ads.
</issue_to_address>
### Comment 2
<location path="scripts/telegram-ads-simulator.js" line_range="950-958" />
<code_context>
+ if (outputFile && (summaryOnly || jsonOnly)) {
+ die('--output cannot be combined with --summary-only or --json-only.');
+ }
+ const KNOWN_FLAGS = new Set(['--output', '--summary-only', '--json-only']);
+ for (const arg of args) {
+ if (arg.startsWith('--') && !KNOWN_FLAGS.has(arg)) {
+ die(`Unknown flag "${arg}".`);
</code_context>
<issue_to_address>
**suggestion:** Consider allowing a help flag instead of treating it as an unknown option.
Because of the strict `KNOWN_FLAGS` check, `--help` / `-h` will currently be rejected with "Unknown flag" even though you already define `USAGE`. It would be better to special-case these flags to print `USAGE` and exit 0 instead of treating them as invalid.
```suggestion
if (outputFile && (summaryOnly || jsonOnly)) {
die('--output cannot be combined with --summary-only or --json-only.');
}
const KNOWN_FLAGS = new Set(['--output', '--summary-only', '--json-only', '--help']);
for (const arg of args) {
if (arg === '--help' || arg === '-h') {
console.log(USAGE);
process.exit(0);
}
if (arg.startsWith('--') && !KNOWN_FLAGS.has(arg)) {
die(`Unknown flag "${arg}".`);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+39
to
+48
| const OPT_ITERATIONS = 3; // Number of copy-optimization iterations to run | ||
| const TOP_COMBOS = 20; // Top combinations stored per ad in by_time_day_channel_persona | ||
| const TOP_AD_COUNT = 3; // Number of top ads to select and report | ||
|
|
||
| const USAGE = [ | ||
| 'Usage: node telegram-ads-simulator.js [options]', | ||
| '', | ||
| 'Options:', | ||
| ' --output <path> Write full JSON + summary to <path>; print summary to stdout.', | ||
| ' --summary-only Print only the human-readable plain-text summary.', |
There was a problem hiding this comment.
issue: Guard against OPT_ITERATIONS being set to 0 or a negative value.
This logic relies on OPT_ITERATIONS being ≥ 1: you create optimized_versions with Array.from({ length: OPT_ITERATIONS }) and later access result.optimized_versions[OPT_ITERATIONS - 1]. If OPT_ITERATIONS is ever set to 0 or negative, optimized_versions will be empty and selectTopAds will hit undefined.metrics at runtime. Consider either validating OPT_ITERATIONS (e.g., fail fast if < 1) or handling an empty optimized_versions array when selecting top ads.
Comment on lines
+950
to
+958
| if (outputFile && (summaryOnly || jsonOnly)) { | ||
| die('--output cannot be combined with --summary-only or --json-only.'); | ||
| } | ||
| const KNOWN_FLAGS = new Set(['--output', '--summary-only', '--json-only']); | ||
| for (const arg of args) { | ||
| if (arg.startsWith('--') && !KNOWN_FLAGS.has(arg)) { | ||
| die(`Unknown flag "${arg}".`); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: Consider allowing a help flag instead of treating it as an unknown option.
Because of the strict KNOWN_FLAGS check, --help / -h will currently be rejected with "Unknown flag" even though you already define USAGE. It would be better to special-case these flags to print USAGE and exit 0 instead of treating them as invalid.
Suggested change
| if (outputFile && (summaryOnly || jsonOnly)) { | |
| die('--output cannot be combined with --summary-only or --json-only.'); | |
| } | |
| const KNOWN_FLAGS = new Set(['--output', '--summary-only', '--json-only']); | |
| for (const arg of args) { | |
| if (arg.startsWith('--') && !KNOWN_FLAGS.has(arg)) { | |
| die(`Unknown flag "${arg}".`); | |
| } | |
| } | |
| if (outputFile && (summaryOnly || jsonOnly)) { | |
| die('--output cannot be combined with --summary-only or --json-only.'); | |
| } | |
| const KNOWN_FLAGS = new Set(['--output', '--summary-only', '--json-only', '--help']); | |
| for (const arg of args) { | |
| if (arg === '--help' || arg === '-h') { | |
| console.log(USAGE); | |
| process.exit(0); | |
| } | |
| if (arg.startsWith('--') && !KNOWN_FLAGS.has(arg)) { | |
| die(`Unknown flag "${arg}".`); | |
| } | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…time-window aggregation
Three issues from code review resolved:
CLI argument validation (brittle --output handling)
Centralize magic numbers as named constants
selectTopAds time-window aggregation (arbitrary first element)
https://claude.ai/code/session_01QSkvVSYsiNQ9udQ3fvVopJ
Summary by Sourcery
Improve the Telegram ads simulator CLI robustness, centralize simulation tuning constants, and refine top-ad time-window aggregation logic for more accurate recommendations and self-documenting output.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes