Skip to content

refactor(cli/seed): consistent crypto.randomInt() and nullish coalescing#712

Open
kamilwronka wants to merge 1 commit intodevelopfrom
claude/nifty-noyce
Open

refactor(cli/seed): consistent crypto.randomInt() and nullish coalescing#712
kamilwronka wants to merge 1 commit intodevelopfrom
claude/nifty-noyce

Conversation

@kamilwronka
Copy link
Copy Markdown
Contributor

@kamilwronka kamilwronka commented Apr 8, 2026

Summary

  • Replace Math.random() with crypto.randomInt() in battles-generator.ts and npcs-scraper.ts for consistency with other seed generators (loot-generator, guild-generator) that already use crypto.randomInt()
  • Change color || 0 to color ?? 0 in guild-generator.ts for correct nullish handling (avoids false-positive on 0 values, though none exist in current data)

Files touched

  • packages/cli/src/commands/seed/generators/battles-generator.ts
  • packages/cli/src/commands/seed/generators/guild-generator.ts
  • packages/cli/src/commands/seed/scrapers/npcs-scraper.ts

Why these changes are safe

  • crypto.randomInt(0, n) produces the same range [0, n) as Math.floor(Math.random() * n) — no behavioral change
  • ?? vs || only differs for falsy-but-non-nullish values (0, "", false); ROLE_COLORS contains no such values, so behavior is identical
  • All changes are in seed/scraper tooling, not production application code

Residual risks

  • None — these are data generation utilities used only for local seeding/scraping

Test plan

  • Pre-commit hooks passed (oxlint, oxfmt)
  • Format check passes on changed files

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Updated random value selection and null/undefined handling in seed generation components.

…sh coalescing

Replace Math.random() with crypto.randomInt() in battles-generator and
npcs-scraper for consistency with the rest of the seed generators. Change
color || 0 to color ?? 0 in guild-generator for correct nullish handling.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

📝 Walkthrough

Walkthrough

The pull request replaces JavaScript's standard Math.random() with Node.js's cryptographic crypto.randomInt() for random selection across seed generators and scrapers. Additionally, one change updates fallback logic from logical OR to nullish coalescing for handling undefined/null values.

Changes

Cohort / File(s) Summary
Random Number Generation Migration
packages/cli/src/commands/seed/generators/battles-generator.ts, packages/cli/src/commands/seed/scrapers/npcs-scraper.ts
Replaced Math.random() and Math.floor() combinations with crypto.randomInt() for random index selection in world and profession selection. Added node:crypto imports.
Fallback Logic Update
packages/cli/src/commands/seed/generators/guild-generator.ts
Changed color field assignment from logical OR (`

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Hops of Joy

Crypto randomness now guides our way,
No more Math.random in the fray!
Nullish coalescence makes logic bright,
Seeds are stronger, safer, more right! 🎲✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: replacing Math.random() with crypto.randomInt() and using nullish coalescing (??), which are the primary focuses across the three modified files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/nifty-noyce

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@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.

🧹 Nitpick comments (1)
packages/cli/src/commands/seed/generators/battles-generator.ts (1)

34-39: Replace || with ?? (or remove fallback entirely).

Line 39 currently uses randomWorld || "gordion". Given the local worlds list and Line 34 selection, this fallback is unnecessary; if you keep it, use ?? for consistency.

Proposed cleanup
-    return {
+    return {
       accountId,
       characterId,
-      world: randomWorld || "gordion",
+      world: randomWorld,
       events: this.samplePayload.events,
     };

As per coding guidelines, "Prefer ?? over || for nullish coalescing".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/seed/generators/battles-generator.ts` around lines
34 - 39, The return currently sets world: randomWorld || "gordion", which uses
|| fallback unnecessarily; update the assignment to either remove the fallback
entirely or use nullish coalescing (world: randomWorld ?? "gordion") so that the
code uses ?? instead of ||; locate the randomWorld variable (set from
worlds[crypto.randomInt(0, worlds.length)]) and update the world property in the
returned object accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/cli/src/commands/seed/generators/battles-generator.ts`:
- Around line 34-39: The return currently sets world: randomWorld || "gordion",
which uses || fallback unnecessarily; update the assignment to either remove the
fallback entirely or use nullish coalescing (world: randomWorld ?? "gordion") so
that the code uses ?? instead of ||; locate the randomWorld variable (set from
worlds[crypto.randomInt(0, worlds.length)]) and update the world property in the
returned object accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 172075af-e9dd-4917-8e82-a11ec3b161f9

📥 Commits

Reviewing files that changed from the base of the PR and between f8c52e1 and 64929c9.

📒 Files selected for processing (3)
  • packages/cli/src/commands/seed/generators/battles-generator.ts
  • packages/cli/src/commands/seed/generators/guild-generator.ts
  • packages/cli/src/commands/seed/scrapers/npcs-scraper.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant