Skip to content

Zsort - #169

Merged
mstdokumaci merged 15 commits into
mainfrom
zsort
Jul 31, 2026
Merged

Zsort#169
mstdokumaci merged 15 commits into
mainfrom
zsort

Conversation

@mstdokumaci

@mstdokumaci mstdokumaci commented Jul 31, 2026 •

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features
    • Added automatic import sorting, cleanup, comment preservation, and invalid-pattern detection for Zig source files.
    • Added tools to identify potentially unused public declarations.
    • Added build commands for checking and fixing import organization.
  • Bug Fixes
    • Improved consistency and reliability of import validation.
  • Tests
    • Expanded coverage for import sorting and validation behavior.
  • Chores
    • Standardized import ordering and formatting throughout the codebase.
    • Improved build and lint workflows.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5df3dfe1-8fcb-41f5-bcce-925b15cf190b

📥 Commits

Reviewing files that changed from the base of the PR and between 69e76bf and 4dd0422.

📒 Files selected for processing (4)
  • build.zig
  • scripts/unused_pub_detector.js
  • tools/zsort.zig
  • tools/zsort_test.zig
🚧 Files skipped from review as they are similar to previous changes (4)
  • build.zig
  • scripts/unused_pub_detector.js
  • tools/zsort_test.zig
  • tools/zsort.zig

📝 Walkthrough

Walkthrough

The pull request adds a Zig import-sorting CLI, integrates it with build and lint steps, normalizes imports across src, adds tests for zsort, and adds a Node.js tool that detects unused public declarations.

Changes

Import tooling and declaration analysis

Layer / File(s) Summary
Import sorter and tests
tools/zsort.zig, tools/zsort_test.zig
Adds import classification, collection, validation, sorting, comment preservation, diff output, fix mode, and tests for these behaviors.
Build and lint integration
build.zig, package.json
Builds zsort for the host, adds import-checking steps, runs zsort tests, formats src and tools, and runs import fixing before linting.
Source import normalization
src/**/*.zig
Reorders and groups imports, adds spacing, moves selected aliases, and replaces repeated inline imports with named aliases.
Unused declaration detector
scripts/unused_pub_detector.js
Builds import reachability data, extracts public declarations, classifies production and test references, and reports unused or re-exported declarations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title identifies the main change: adding the zsort import-sorting tool and its build integration.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 15

🧹 Nitpick comments (5)
package.json (1)

12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Run fix-imports before zig fmt src.

The chain runs zig fmt src first and zig build fix-imports third. zsort assembles the import block from trimmed lines joined with newlines and does not invoke the formatter on its output. Any formatting difference it introduces therefore survives to the end of the lint run, and a later zig fmt --check in CI fails.

Move zig build fix-imports before zig fmt src so the formatter normalizes the rewritten import block.

♻️ Proposed reordering
-    "lint": "zig fmt src && zlint --deny-warnings && zig build fix-imports && zig build lint && zig build check --summary all"
+    "lint": "zig build fix-imports && zig fmt src && zlint --deny-warnings && zig build lint && zig build check --summary all"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package.json` at line 12, Reorder the lint command so “zig build fix-imports”
runs before “zig fmt src”; keep the remaining checks in their existing order.
build.zig (1)

53-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

tools/zsort.zig is not covered by the formatter, the linter, or the check step.

Three gaps follow from the new file location:

  1. package.json runs zig fmt src and zlint --deny-warnings, both scoped to src. tools/zsort.zig is not formatted or linted.
  2. zsort_exe is not added to the check step at line 109, so zig build check does not compile it. A compile error in the tool surfaces only when a developer runs check-imports or fix-imports.
  3. The tool disables a lint rule at line 1 with // zlint-disable no-print, which implies linting was intended for it.

Add check_step.dependOn(&zsort_exe.step); and extend the zig fmt argument list to include tools.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build.zig` around lines 53 - 60, Update the build and formatting
configuration for tools/zsort.zig: add zsort_exe as a dependency of check_step
so zig build check compiles it, and extend the package.json zig fmt arguments
from src to also include tools. Leave the existing zlint configuration
unchanged.
src/wire/encode.zig (1)

4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: import ../presence/manager.zig once.

Lines 4 and 5 call @import("../presence/manager.zig") twice to extract two symbols. Zig caches module imports, so there is no compilation cost. The repetition of the path string is the only concern: a future move of manager.zig requires two edits.

Note that the sort order of these two lines depends on the declaration length tie-break in Import.lessThan, because both lines share the same path. That coupling disappears if a single module alias is used.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/wire/encode.zig` around lines 4 - 6, Update the imports in encode.zig to
import presence/manager.zig once under a module alias, then reference UserEntry
and PresenceManager through that alias; leave the PresenceRecord import
unchanged and preserve the existing declaration ordering.
tools/zsort.zig (2)

1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add unit tests for zsort before it rewrites source files.

tools/zsort.zig performs byte-level source rewriting across the whole src tree in fix mode. No test file accompanies it. The parsing helpers are pure functions over a []const u8 and are straightforward to test directly: classify, extractPath, isTopLevelImportLine, findImportBlockEnd, findCImportEnd, hasBannedPatterns, and buildSortedImportText.

Cover at minimum: an alias declaration with a trailing ;, a comment between two import groups, a @cImport block, a brace inside a string literal before the imports, and idempotency, meaning that fix applied twice produces the same bytes.

Do you want me to generate a tools/zsort_test.zig with these cases, and the build.zig wiring to run it?

As per coding guidelines: "After core Zig logic changes, run zig build test; for module-specific changes, run the corresponding filtered unit test."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/zsort.zig` around lines 1 - 3, Add unit coverage for the pure parsing
and rewriting helpers in zsort.zig: classify, extractPath, isTopLevelImportLine,
findImportBlockEnd, findCImportEnd, hasBannedPatterns, and
buildSortedImportText. Include cases for a semicolon-terminated alias, comments
separating import groups, a `@cImport` block, braces inside string literals before
imports, and applying fix twice producing identical bytes; wire
tools/zsort_test.zig into build.zig and verify with the relevant filtered test
plus zig build test.

Source: Coding guidelines


25-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider deriving the vendor module list from a single source.

classify hardcodes sqlite, msgpack, and httpx. build.zig declares the same three module names. If a maintainer adds a dependency in build.zig, zsort classifies it as CLASS_LOCAL and moves it into the wrong group without any warning. Move the list to a top-level const vendor_modules = [_][]const u8{...} with a comment that points to build.zig, so the coupling is explicit.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/zsort.zig` around lines 25 - 36, Update classify to use a top-level
vendor_modules array containing sqlite, msgpack, and httpx instead of hardcoding
the names in the condition. Add a comment on vendor_modules pointing maintainers
to build.zig as the corresponding dependency declaration source, and preserve
the existing CLASS_VENDOR and CLASS_LOCAL classification behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/unused_pub_detector.js`:
- Around line 200-229: Update the reference-checking logic around selfText,
hasWordOutsideLines, and the cross-file hasWord calls to use comment- and
string-stripped text for every file, not raw f.text. Precompute
nostrings(nocomment(...)) once per file in a cache before processing
declarations, then reuse the cached text for both self-file and cross-file
checks while preserving the existing line-exclusion behavior.
- Around line 11-19: Update the file collection in the walk function to store
each Zig file’s absolute filesystem path alongside its canonical path, then
update import resolution near path.resolve to use that absolute path rather than
f.path. Preserve canonical paths for reporting and map lookups, ensuring the
script works when invoked outside the repository root.
- Line 95: Update the nostrings function to remove Zig single-quoted character
literals in addition to double-quoted strings, preserving both literal contents
as empty quotes before the brace-counting logic consumes the result.

In `@src/threading/worker_pool_test.zig`:
- Around line 3-4: Rename the local const bindings managedThread to
managed_thread and workerPool to worker_pool to comply with snake_case naming
for functions and variables. Then update all references to these bindings
throughout the file at the specified locations (lines 9, 14, 43, 51, and 60) to
use the new snake_case names instead.

In `@tools/zsort.zig`:
- Around line 329-330: Change the cleanup registration for imports_buf in the
surrounding function to use defer rather than errdefer, ensuring its allocation
is released on both success and error paths after the bytes are copied into buf.
Keep the existing deinit(allocator) cleanup pattern used by the other lists.
- Around line 386-401: Flush the buffered writer before returning from showDiff
so check-mode diff output is emitted; update tools/zsort.zig lines 386-401
accordingly. Also flush stdout_w2 immediately after its summary print and before
std.process.exit(1) at tools/zsort.zig lines 558-562, since exit bypasses
deferred cleanup.
- Around line 174-181: Replace the per-call braceDepth prefix scan with a single
forward parser pass used by collectImports and findCImportEnd. Track brace depth
while skipping braces inside string literals, character literals, line comments,
and block comments, and use the incrementally maintained depth when identifying
import boundaries.
- Around line 424-426: Move the arena initialization and deinitialization from
the whole-run scope into the per-file loop around the processing logic beginning
near the file source allocation, so every iteration owns and releases its arena
even when error paths use continue. Keep the allocator usage for that file
unchanged while ensuring allocations from prior files are reclaimed before the
next iteration.
- Around line 277-280: Update the import parsing and emission flow around
trailing_comments, inter_section, and the Import entries so comments between
imports are attached to the following import declaration rather than emitted
after every import. Ensure sorting and fix-mode output move each attached
comment together with its Import, including group-label comments such as those
preceding vendor imports.
- Around line 454-462: Update the target handling around walkDir and the
files.items empty check to report an error and exit with status 1 when the
target is neither a supported file nor directory, or when no files are
collected. Preserve normal traversal and file collection behavior for valid
targets.
- Around line 139-172: Update hasBannedPatterns to ignore commented-out code:
skip lines whose trimmed content begins with // before evaluating `@import`
patterns, and replace the file-wide usingnamespace search with a line-by-line
scan that only checks non-comment lines. Preserve detection of actual banned
imports and usingnamespace usage while avoiding matches inside comments.
- Around line 105-116: Update isTopLevelImportLine to trim leading and trailing
spaces, semicolons, and line terminators from the right-hand side before
validation, matching buildSortedImportText’s trim behavior. Preserve alias
validation for identifiers and dotted paths so declarations such as const
Payload = msgpack.Payload; are recognized and subsequent imports remain in the
import block.
- Line 469: Update the readFileAlloc call in the source-loading logic to pass
file_path before allocator, and construct the 10 MiB limit using std.io.Limit as
required by Zig 0.15.2. Preserve the existing error handling around the
allocation.
- Around line 16-22: Update lessThan so it provides a total ordering: after
comparing class, path, and import byte length, compare a.start and b.start as
the final tie-breaker. Preserve the existing ordering precedence while ensuring
distinct imports with otherwise equal keys are deterministically ordered for
std.sort.pdq.
- Around line 535-545: Replace the direct createFile/truncate flow in the
file-writing block with std.fs.Dir.atomicFile using the existing directory
handle. Defer af.deinit(), write full_new through af.file, and call af.finish()
only after writeAll succeeds; preserve the existing error_count increment and
continue behavior so failed writes leave the original file unchanged.

---

Nitpick comments:
In `@build.zig`:
- Around line 53-60: Update the build and formatting configuration for
tools/zsort.zig: add zsort_exe as a dependency of check_step so zig build check
compiles it, and extend the package.json zig fmt arguments from src to also
include tools. Leave the existing zlint configuration unchanged.

In `@package.json`:
- Line 12: Reorder the lint command so “zig build fix-imports” runs before “zig
fmt src”; keep the remaining checks in their existing order.

In `@src/wire/encode.zig`:
- Around line 4-6: Update the imports in encode.zig to import
presence/manager.zig once under a module alias, then reference UserEntry and
PresenceManager through that alias; leave the PresenceRecord import unchanged
and preserve the existing declaration ordering.

In `@tools/zsort.zig`:
- Around line 1-3: Add unit coverage for the pure parsing and rewriting helpers
in zsort.zig: classify, extractPath, isTopLevelImportLine, findImportBlockEnd,
findCImportEnd, hasBannedPatterns, and buildSortedImportText. Include cases for
a semicolon-terminated alias, comments separating import groups, a `@cImport`
block, braces inside string literals before imports, and applying fix twice
producing identical bytes; wire tools/zsort_test.zig into build.zig and verify
with the relevant filtered test plus zig build test.
- Around line 25-36: Update classify to use a top-level vendor_modules array
containing sqlite, msgpack, and httpx instead of hardcoding the names in the
condition. Add a comment on vendor_modules pointing maintainers to build.zig as
the corresponding dependency declaration source, and preserve the existing
CLASS_VENDOR and CLASS_LOCAL classification behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a1cdbabb-6847-485f-a881-5e1a31f62f79

📥 Commits

Reviewing files that changed from the base of the PR and between c35c05b and a9a9211.

📒 Files selected for processing (163)
  • build.zig
  • package.json
  • scripts/unused_pub_detector.js
  • src/app_test_helpers.zig
  • src/authentication/jwt_validator.zig
  • src/authentication/jwt_validator_test.zig
  • src/authentication/session.zig
  • src/authentication/ticket_exchange.zig
  • src/authentication/ticket_exchange_test.zig
  • src/authorization/defaults.zig
  • src/authorization/doc_predicate.zig
  • src/authorization/doc_predicate_test.zig
  • src/authorization/evaluate.zig
  • src/authorization/evaluate_test.zig
  • src/authorization/parse.zig
  • src/authorization/parse_test.zig
  • src/authorization/pattern.zig
  • src/authorization/pattern_test.zig
  • src/authorization/presence.zig
  • src/authorization/presence_test.zig
  • src/authorization/session_resolver.zig
  • src/authorization/store.zig
  • src/authorization/test_helpers.zig
  • src/authorization/types.zig
  • src/checkpoint_test_helpers.zig
  • src/checkpoint_worker.zig
  • src/checkpoint_worker_property_test.zig
  • src/checkpoint_worker_test.zig
  • src/config_loader.zig
  • src/config_loader_property_test.zig
  • src/config_loader_test.zig
  • src/connection/manager.zig
  • src/connection/manager_test.zig
  • src/connection/send_queue.zig
  • src/connection/state.zig
  • src/connection/state_test.zig
  • src/connection/violations_test.zig
  • src/contains_array_equivalence_test.zig
  • src/integration_wiring_test.zig
  • src/json/iterate_test.zig
  • src/json/read_test.zig
  • src/json/write_test.zig
  • src/lock_free_cache.zig
  • src/lock_free_cache_leak_test.zig
  • src/lock_free_cache_test.zig
  • src/logging_property_test.zig
  • src/main.zig
  • src/memory_safety_property_test.zig
  • src/memory_strategy.zig
  • src/memory_strategy_test.zig
  • src/message_handler.zig
  • src/message_handler_property_test.zig
  • src/message_handler_test.zig
  • src/message_handler_verification_test.zig
  • src/migration_detector.zig
  • src/migration_detector_property_test.zig
  • src/migration_executor.zig
  • src/migration_executor_property_test.zig
  • src/migration_executor_test.zig
  • src/msgpack_test_helpers.zig
  • src/msgpack_utils.zig
  • src/msgpack_utils_property_test.zig
  • src/msgpack_utils_test.zig
  • src/presence/manager.zig
  • src/presence/manager_test.zig
  • src/presence/record.zig
  • src/presence/record_test.zig
  • src/presence/service.zig
  • src/presence/service_test.zig
  • src/presence/test_helpers.zig
  • src/presence/worker.zig
  • src/presence/worker_test.zig
  • src/query/ast.zig
  • src/query/ast_test.zig
  • src/query/eval.zig
  • src/query/eval_test.zig
  • src/query/hash_context.zig
  • src/query/hasher.zig
  • src/query/parser.zig
  • src/query/parser_test.zig
  • src/query/test_helpers.zig
  • src/queues/mpsc_queue_test.zig
  • src/queues/mpsc_queue_thread_safety_test.zig
  • src/queues/spmc_blocking_queue_test.zig
  • src/queues/spmc_blocking_queue_thread_safety_test.zig
  • src/queues/spsc_queue_test.zig
  • src/schema/field_path_test.zig
  • src/schema/format.zig
  • src/schema/format_test.zig
  • src/schema/index.zig
  • src/schema/parse.zig
  • src/schema/parse_test.zig
  • src/schema/system.zig
  • src/schema/system_test.zig
  • src/schema/test_helpers.zig
  • src/schema/types_test.zig
  • src/server.zig
  • src/server_init_property_test.zig
  • src/sql/buf_test.zig
  • src/sql/build.zig
  • src/sql/build_test.zig
  • src/sql/ddl.zig
  • src/sql/ddl_test.zig
  • src/storage_engine.zig
  • src/storage_engine/cache.zig
  • src/storage_engine/connection.zig
  • src/storage_engine/errors.zig
  • src/storage_engine/filter_sql.zig
  • src/storage_engine/pk_set.zig
  • src/storage_engine/read_buffer.zig
  • src/storage_engine/read_worker_perf_test.zig
  • src/storage_engine/read_worker_pool.zig
  • src/storage_engine/reader.zig
  • src/storage_engine/sql.zig
  • src/storage_engine/write_queue.zig
  • src/storage_engine/write_worker.zig
  • src/storage_engine/write_worker_perf_test.zig
  • src/storage_engine_error_property_test.zig
  • src/storage_engine_property_test.zig
  • src/storage_engine_sql_test.zig
  • src/storage_engine_stability_property_test.zig
  • src/storage_engine_test.zig
  • src/storage_engine_test_helpers.zig
  • src/store_service.zig
  • src/store_service_test.zig
  • src/store_test_helpers.zig
  • src/subscription/change_queue.zig
  • src/subscription/change_queue_test.zig
  • src/subscription/consistency_test.zig
  • src/subscription/engine.zig
  • src/subscription/engine_test.zig
  • src/subscription/engine_thread_safety_test.zig
  • src/subscription/predicate_trie.zig
  • src/subscription/predicate_trie_test.zig
  • src/subscription/worker_pool.zig
  • src/subscription/worker_pool_perf_test.zig
  • src/subscription/worker_pool_test.zig
  • src/thread_budget_test.zig
  • src/threading/latch_test.zig
  • src/threading/managed_thread_test.zig
  • src/threading/notifier_test.zig
  • src/threading/wait_group_test.zig
  • src/threading/worker_pool_test.zig
  • src/typed/codec.zig
  • src/typed/codec_test.zig
  • src/typed/doc_id_test.zig
  • src/typed/test_helpers.zig
  • src/typed/types.zig
  • src/typed/types_test.zig
  • src/uwebsockets_wrapper.zig
  • src/uwebsockets_wrapper_property_test.zig
  • src/uwebsockets_wrapper_test.zig
  • src/uws_timer.zig
  • src/wire/comptime.zig
  • src/wire/decode.zig
  • src/wire/decode_test.zig
  • src/wire/encode.zig
  • src/wire/encode_test.zig
  • src/wire/errors.zig
  • src/wire/errors_test.zig
  • src/wire/msgpack_skip_test.zig
  • src/wire/test_helpers.zig
  • tools/zsort.zig

Comment thread scripts/unused_pub_detector.js
Comment thread scripts/unused_pub_detector.js Outdated
Comment thread scripts/unused_pub_detector.js
Comment thread src/threading/worker_pool_test.zig
Comment thread tools/zsort.zig
Comment thread tools/zsort.zig
Comment thread tools/zsort.zig
Comment thread tools/zsort.zig Outdated
Comment thread tools/zsort.zig Outdated
Comment thread tools/zsort.zig Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tools/zsort.zig (1)

632-637: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The banned-pattern scan is skipped for files with no collected imports.

Line 632 returns early when result.items.len == 0. hasBannedPatterns runs only after that check. A file that contains usingnamespace @import("foo"); and no recognized import declaration produces an empty result, so the violation is never reported. Move the banned-pattern check above line 632.

🐛 Proposed fix
-        if (result.items.len == 0) continue;
-
         if (hasBannedPatterns(source)) |msg| {
             std.debug.print("{s}: banned: {s}\n", .{ file_path, msg });
             banned_count += 1;
         }
+
+        if (result.items.len == 0) continue;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/zsort.zig` around lines 632 - 637, Move the hasBannedPatterns(source)
check in the import-processing flow before the result.items.len == 0 early
continue, so files with no collected imports are still scanned and banned_count
is updated when a violation is found. Preserve the existing banned message and
file_path reporting.
🧹 Nitpick comments (2)
build.zig (1)

62-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wire check-imports into an automated path.

The check-imports step is defined but no other step depends on it. package.json only runs fix-imports, which rewrites files instead of failing. A drift in import ordering can therefore reach CI unnoticed. Consider adding check-imports to the check step or to the CI script.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build.zig` around lines 62 - 72, Wire the existing check-imports_step into
the automated validation path by making the main check step or the CI check
script depend on it. Keep fix-imports as the explicit rewriting command, while
ensuring normal checks execute the zsort “check” action and fail when import
ordering drifts.
tools/zsort_test.zig (1)

126-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the resulting order, not only substring presence.

Lines 141-143 pass for any permutation of the three imports. The test therefore does not verify the sort. Use expectEqualStrings against the expected block, or compare index positions.

♻️ Proposed stronger assertion
-    try std.testing.expect(std.mem.indexOf(u8, result, "bar") != null);
-    try std.testing.expect(std.mem.indexOf(u8, result, "foo") != null);
-    try std.testing.expect(std.mem.indexOf(u8, result, "std") != null);
+    try std.testing.expectEqualStrings(
+        \\const std = `@import`("std");
+        \\
+        \\const bar = `@import`("bar");
+        \\const foo = `@import`("foo");
+        \\
+    , result);

The same applies to lines 189-191.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/zsort_test.zig` around lines 126 - 144, Strengthen the assertions in
the “buildSortedImportText: basic sort” test and the analogous assertions around
the later import-sort test so they verify the imports’ exact sorted order rather
than only substring presence. Compare the generated import block or import
positions against the expected ordering using the existing result from
zsort.buildSortedImportText.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@package.json`:
- Line 12: Update the fix-mode exit handling in tools/zsort.zig, specifically
the logic around changed_count and std.process.exit(1), so rewriting files alone
returns exit code 0 while error_count or banned_count still returns 1; preserve
the package.json lint chain so all subsequent checks run after imports are
fixed.

In `@scripts/unused_pub_detector.js`:
- Line 96: Update nostrings to remove indented Zig multiline-string lines using
a multiline, line-anchored pattern that permits spaces or tabs before the
delimiter, before counting braces. Preserve the existing single- and
double-quoted string stripping behavior.

In `@tools/zsort.zig`:
- Line 694: Update the loop’s af.finish() handling so failures are reported
consistently with the other per-file errors: catch the error, print its message,
increment error_count, and continue processing remaining files. Do not propagate
the error from the loop, so the summary after the loop still executes.
- Around line 219-245: Fix stray-import removal so attached comments and
preceding blank lines are not emitted twice in fix mode. Update the
stray-removal logic to remove each stray import from its `comment_start` when
present, through `imp.end`, while preserving the existing import-block rendering
behavior in `buildSortedImportText`.

---

Outside diff comments:
In `@tools/zsort.zig`:
- Around line 632-637: Move the hasBannedPatterns(source) check in the
import-processing flow before the result.items.len == 0 early continue, so files
with no collected imports are still scanned and banned_count is updated when a
violation is found. Preserve the existing banned message and file_path
reporting.

---

Nitpick comments:
In `@build.zig`:
- Around line 62-72: Wire the existing check-imports_step into the automated
validation path by making the main check step or the CI check script depend on
it. Keep fix-imports as the explicit rewriting command, while ensuring normal
checks execute the zsort “check” action and fail when import ordering drifts.

In `@tools/zsort_test.zig`:
- Around line 126-144: Strengthen the assertions in the “buildSortedImportText:
basic sort” test and the analogous assertions around the later import-sort test
so they verify the imports’ exact sorted order rather than only substring
presence. Compare the generated import block or import positions against the
expected ordering using the existing result from zsort.buildSortedImportText.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e0ef7099-d406-46f9-9ab0-2bc982c09b56

📥 Commits

Reviewing files that changed from the base of the PR and between a9a9211 and 69e76bf.

📒 Files selected for processing (16)
  • build.zig
  • package.json
  • scripts/unused_pub_detector.js
  • src/connection/send_queue.zig
  • src/connection/violations.zig
  • src/json/read.zig
  • src/json/write.zig
  • src/presence/subscriber.zig
  • src/queues/spmc_blocking_queue.zig
  • src/schema/field_path.zig
  • src/sql/buf.zig
  • src/threading/worker_pool.zig
  • src/timed_test_runner.zig
  • src/wire/encode.zig
  • tools/zsort.zig
  • tools/zsort_test.zig
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/connection/send_queue.zig
  • src/wire/encode.zig

Comment thread package.json
Comment thread scripts/unused_pub_detector.js Outdated
Comment thread tools/zsort.zig
Comment thread tools/zsort.zig Outdated
@mstdokumaci
mstdokumaci merged commit 0fc41e8 into main Jul 31, 2026
8 checks passed
@mstdokumaci
mstdokumaci deleted the zsort branch July 31, 2026 15:33
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