Skip to content

Conversation

Jarred-Sumner
Copy link
Collaborator

What does this PR do?

How did you verify your code works?

@robobun
Copy link
Collaborator

robobun commented Sep 18, 2025

Updated 2:32 AM PT - Sep 18th, 2025

@Jarred-Sumner, your commit 5cb09ec has 7 failures in Build #26244 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 22763

That installs a local version of the PR into your bun-22763 executable, so you can run:

bun-22763 --bun

Copy link
Contributor

coderabbitai bot commented Sep 18, 2025

Walkthrough

Reset of the ExportRenamer internal counter in src/renamer.zig and replacement of NameMinifier head/tail string constants with aligned fixed-size byte arrays in src/ast.zig; no exported/public API signatures beyond constant type/shape changes.

Changes

Cohort / File(s) Change Summary
ExportRenamer state reset
src/renamer.zig
In ExportRenamer.clearRetainingCapacity, added this.count = 0 to reset the minifier/name-generation counter alongside existing resets of used and string_buffer. No public API changes.
NameMinifier head/tail constants -> fixed arrays
src/ast.zig
Replaced default_head and default_tail string constants with aligned fixed-size byte arrays ([54]u8 align(64) and [64]u8 align(64) respectively). Character sequences reordered (frequency-optimized) and constants' types/signatures updated accordingly. No other control flow changes.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR body includes the template headings "What does this PR do?" and "How did you verify your code works?" but both sections are empty, so the description provides no information about intent, the specific changes (e.g., renamer count reset or NameMinifier array changes), or verification steps; this prevents reviewers from assessing correctness or reproducing tests. Because required template content is missing, the description check fails. Please populate the template by summarizing the change(s) (for example note that ExportRenamer.clearRetainingCapacity now resets this.count = 0 and mention the default_head/default_tail array changes in src/ast.zig if they are part of this PR), and provide verification details such as which unit/integration tests were run, manual reproduction steps, CI/buildkite links, and expected outcomes so reviewers can reproduce and validate the changes. Include any relevant notes on compatibility or risk and the exact commands or test names used during verification.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Reset export renamer count to 0 at end of each chunk" accurately and succinctly captures the primary change in the changeset — adding this.count = 0 in ExportRenamer.clearRetainingCapacity as shown in src/renamer.zig — and is focused on the main behavioral impact. It is concise, specific, and useful for a teammate scanning PR history to understand the primary intent. Therefore the title meets the repository's clarity and relevance requirements.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jarred/reset

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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

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

🧹 Nitpick comments (3)
src/ast.zig (3)

158-161: Solid switch to fixed-size, 64B-aligned tables; add compile-time guards to prevent length drift.

The change looks good. To harden against accidental mismatches (including future edits or CharFreq changes), add comptime checks tying these constants to their consumers.

Apply this diff right after the two consts:

 pub const default_head: [54]u8 align(64) = "etnriaoscludfpmhg_vybxSCwTEDOkAjMNPFILRzBVHUWGKqJYXZQ$".*;
 pub const default_tail: [64]u8 align(64) = "etnriaoscludfpmhg_vybxSCwTEDOkAjMNPFILRzBVHUWGKqJYXZQ$1024368579".*;
+
+comptime {
+    if (default_head.len != 54) @compileError("NameMinifier.default_head must be 54 bytes");
+    if (default_tail.len != char_freq_count)
+        @compileError("NameMinifier.default_tail length must equal char_freq_count");
+}

Please confirm this compiles cleanly with the Zig version pinned in the repo, given the sentinel-to-non-sentinel coercion from the string literal via .*.


184-199: Prefer toOwnedSlice() to transfer ownership explicitly.

Returning name.items relies on callers freeing a slice that originated from an internal ArrayList buffer. Using toOwnedSlice() makes ownership explicit and avoids accidental leaks if the calling pattern changes.

Apply this diff:

-        return name.items;
+        return name.toOwnedSlice();

If there are callers relying on the current behavior/capacity, please double-check them. If throughput is a concern, we can also pre-size name by estimating digits from i to minimize reallocs.


162-167: Offer a convenience initializer that seeds head/tail with defaults.

numberToMinifiedName assumes this.head/this.tail are populated. A helper reduces footguns and branches callers onto the default path.

Add alongside init:

     pub fn init(allocator: std.mem.Allocator) NameMinifier {
         return .{
             .head = std.ArrayList(u8).init(allocator),
             .tail = std.ArrayList(u8).init(allocator),
         };
     }
+
+    pub fn initWithDefaults(allocator: std.mem.Allocator) !NameMinifier {
+        var nm = NameMinifier.init(allocator);
+        try nm.head.appendSlice(default_head[0..]);
+        try nm.tail.appendSlice(default_tail[0..]);
+        return nm;
+    }

If a non-default charset is ever required, this keeps the existing init path intact while making the default the easy path.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 83d7c16 and 5cb09ec.

📒 Files selected for processing (1)
  • src/ast.zig (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Implement debug logs in Zig using const log = bun.Output.scoped(.${SCOPE}, false); and invoking log("...", .{})

Files:

  • src/ast.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup

Files:

  • src/ast.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Format

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.

2 participants