Skip to content

Update cxjs_substring() to match exp_fn_substring() - #122

Merged
gbeeley merged 2 commits into
masterfrom
fix-substring
Sep 3, 2026
Merged

gbeeley merged 2 commits into
masterfrom
fix-substring

Conversation

@Lightning11wins

Copy link
Copy Markdown
Contributor

I caused a bug in the gift_associations.app because the client-side version of substring() functioned differently from the server side version, as documented in #121. This PR will close #121 by solving this inconsistency issue.

@Lightning11wins Lightning11wins self-assigned this Jun 10, 2026
@Lightning11wins Lightning11wins added bug ai-review Request AI review for PRs. size: trivial Easy to review, probably ~100 lines or fewer. labels Jun 10, 2026
@greptile-apps

greptile-apps Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates cxjs_substring() in ht_render.js to align its behavior with the server-side exp_fn_substring() in exp_functions.c, fixing a client/server inconsistency reported in #121.

  • Adds explicit type validation (typeof s != 'string', Number.isInteger(p/l)) mirroring the server's DATA_T_STRING/DATA_T_INTEGER checks, and returns null for invalid types instead of coercing via String(s).
  • Replaces deprecated substr() with slice() and adds proper clamping of the start position and length to match the server's i < 0 → 0 and i > n → n logic, which also fixes incorrect behavior for zero or negative position arguments.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to one JS helper function and correctly mirrors the C reference implementation.

The new logic precisely replicates what exp_fn_substring() does in C: 1-based position adjusted to 0-based, both start and length clamped to valid range, and explicit type checks returning null for invalid input types. Edge cases (zero/negative position, length larger than remaining string, null third argument) were verified against the C implementation and all match. Replacing the deprecated substr() with slice() is a clean improvement with no behavioral difference for valid inputs.

No files require special attention — only one function in one JS file changed.

Important Files Changed

Filename Overview
centrallix-os/sys/js/ht_render.js cxjs_substring() updated to match server-side behavior: adds type guards, fixes negative/zero position clamping, replaces deprecated substr() with slice(), and clamps length correctly.

Sequence Diagram

sequenceDiagram
    participant Client as Browser cxjs_substring
    participant Engine as Server exp_fn_substring

    Note over Client,Engine: Before this PR

    Client->>Client: "String(s).substr(p-1) — no clamping"
    Engine->>Engine: "validate types, clamp start and length"

    Note over Client,Engine: After this PR — aligned behavior

    Client->>Client: "typeof s != string → return null"
    Engine->>Engine: "DataType != DATA_T_STRING → return error"
    Client->>Client: "i = p-1, clamp to 0 or s.length"
    Engine->>Engine: "i = i1-1, clamp to 0 or n"
    Client->>Client: "len = l ?? rest.length, clamp to rest.length"
    Engine->>Engine: "i = i2->Integer or strlen(ptr), clamp"
    Client->>Client: "return rest.slice(0, len)"
    Engine->>Engine: "memcpy into tree->String"
Loading

Reviews (1): Last reviewed commit: "Update cxjs_substring() to be more simil..." | Re-trigger Greptile

@Lightning11wins

Copy link
Copy Markdown
Contributor Author

This PR is ready for human review.

@Lightning11wins Lightning11wins changed the title Update cxjs_substring() to match exp_fn_substring(). Update cxjs_substring() to match exp_fn_substring() Jun 10, 2026

@gbeeley gbeeley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change needed for string object compatibility.

Comment thread centrallix-os/sys/js/ht_render.js Outdated
@Lightning11wins
Lightning11wins requested a review from gbeeley June 10, 2026 22:04

@gbeeley gbeeley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good - thanks

@gbeeley
gbeeley merged commit 91a0802 into master Sep 3, 2026
1 check passed
@Lightning11wins
Lightning11wins deleted the fix-substring branch September 3, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request AI review for PRs. bug size: trivial Easy to review, probably ~100 lines or fewer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent cxjs_substring() Implementation

2 participants