Added additional tests to detail the behavior of the split helper

[cathysarisky]

Splitting an element with a trailing separator results in a "" element being included in the returned array. This is typical JavaScript behavior, which I'm preserving here (although I don't love it). This PR adds some additional tests that show this behavior.

Will also add to docs.

[coderabbitai]

Walkthrough

The diff adds multiple new unit tests for the split helper covering block and inline modes (leading/trailing separators, repeated separators producing middle empties, and a string consisting only of separators), plus length/accessor assertions and integration scenarios with foreach and match. It also changes the split helper implementation to filter out empty items from split results and to short-circuit/return early when no non-empty items remain, altering rendering control flow for all-empty inputs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

• Inspect the modified helper implementation (ghost/core/core/frontend/helpers/split.js) for correctness of filtering/early return and any edge-case regressions (escaping, SafeString handling).
• Run and review the new tests (ghost/core/test/unit/frontend/helpers/split.test.js) to ensure expectations align with the helper change, including repeated-separator and all-separator cases.
• Verify integration points that rely on previous behavior (templates/other helpers that use split results, length/foreach/match interactions).

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The title states "Added additional tests to detail the behavior of the split helper" and correctly identifies that new test cases were added to the test file. However, the raw_summary reveals that the PR also includes significant changes to the implementation file (split.js), specifically filtering out empty items from the split result and adding early return logic when the array is empty. The title only mentions the test additions and omits this substantial implementation change, which according to the PR objectives warranted re-review due to its departure from the previously documented behavior.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	The pull request description is related to the changeset—it discusses the split helper's behavior with trailing separators, which is the core component being modified. The description explains the rationale for preserving typical JavaScript behavior regarding empty elements. Although the PR objectives note that the actual implementation diverged from this original intent by filtering out empty strings (representing a behavior change that prompted re-review), the description still remains thematically connected to the changeset and the split helper component involved.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[ErisDS]

I think it would be reasonable to change this default behaviour for this helper. It is standard JS behaviour, the helper isn't doing the wrong thing, but it's not doing what you need or expect.

Working around it with a match helper is great, but every extra helper you have to use adds overhead in terms of processing but more importantly maintenance of theme code.

I'd happily approve a PR that changed this behaviour, especially this early in the helper's life.

The way I'd think about this would be changing the behaviour "trim" empty strings from the end of the array by default. If we end up crossing the bridge that the default behaviour is desirable for some users they could add trim=false as a property to the helper to disable the trim by default behaviour, but I wouldn't add it unless it was asked for by a theme dev.

[cathysarisky]

Hi @ErisDS , thanks for the review!

I went ahead and implemented the altered behavior for split to eliminate empty strings anywhere they occur. Given that we're generally using split elements that'll be looped over to produce text, empty items that result in nothing being rendered but that change the length of the array are a potential source of confusion.

I added more tests, including the case where the string is nothing but separators.

Marking this for re-review since it's in no way what you approved previously, but leaving it on this branch/PR so that we don't lose the context. Thank you!

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

ghost/core/core/frontend/helpers/split.js (1)

31-38: Consider documenting the non-standard filtering behavior and remove redundant check.

The filtering of empty strings (line 33) deviates from JavaScript's native split(), which preserves empty strings at leading, trailing, and middle positions. This could surprise users expecting standard behavior or break existing templates that rely on empty elements for positional logic.

Additionally, the early return check (lines 36-38) is redundant since renderResult([]) on line 40 would handle an empty array identically.

Apply this diff to remove the redundant check:

     // Filter out all empty strings
     const result = string.split(separator)
         .filter(item => item !== '')
         .map(item => new SafeString(item));
 
-    if (result.length === 0) {
-        return renderResult([], options, data);
-    }
-
     return renderResult(result, options, data);

Additionally, consider adding a JSDoc comment documenting this non-standard behavior:

/**
 * Splits a string by separator and returns non-empty segments as SafeStrings.
 * Note: Unlike JavaScript's native split(), this filters out all empty strings
 * from the result, including those from leading/trailing/repeated separators.
 */
module.exports = function split(...args) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e09b799 and 3710637.

📒 Files selected for processing (2)

• ghost/core/core/frontend/helpers/split.js (1 hunks)
• ghost/core/test/unit/frontend/helpers/split.test.js (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

ghost/core/core/frontend/**

📄 CodeRabbit inference engine (AGENTS.md)

Frontend and theme rendering for core should be under ghost/core/core/frontend/

Files:

• ghost/core/core/frontend/helpers/split.js

⏰ 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). (3)

• GitHub Check: Ghost-CLI tests
• GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
• GitHub Check: Build & Push

🔇 Additional comments (1)

ghost/core/test/unit/frontend/helpers/split.test.js (1)

115-151: Excellent test coverage for the empty-string filtering behavior.

These tests comprehensively validate the new filtering behavior across all edge cases:

• Leading and trailing separators (no boundary empties)
• Repeated separators (no middle empties)
• String consisting only of separators (empty result)
• Practical use case of suffix removal

The dual assertions (output + length check) effectively verify both the content and array size after filtering.