Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const [localeValue, group] of localeGroups) { | ||
| if (!localeOrderValues.includes(localeValue)) { | ||
| orderedItems.push(...group); | ||
| } | ||
| } | ||
| orderedItems.push(...ungrouped); | ||
|
|
There was a problem hiding this comment.
Ordering of ungrouped items changed vs. original behaviour
Before this refactor, remainingItems held all items that did NOT match any localeOrderValues entry — including items with an unrecognised localeValue and items whose localeValue was undefined/null — and they were appended in their original relative order.
The new Map-based implementation separates these two categories:
- Items with an unrecognised locale are grouped in
localeGroupsand flushed first (lines 503-507). - Items with a falsy
localeValueare inungroupedand always appended last (line 509).
This means if the original array had, for example, [{localeValue: undefined}, {localeValue: "zh"}] where "zh" is not in localeOrderValues, the old code would produce […, {undefined}, {zh}] while the new code produces […, {zh}, {undefined}]. The relative position of locale-less items and unrecognised-locale items is now reversed compared to the original ordering.
Although this path is gated behind experimentalSort: 'locales' and is an edge case, the reordering is silently different from the old behaviour and could surprise users who have both locale-less items and extra-locale items in their composite JSON arrays.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/formats/json/mergeJson.ts
Line: 503-509
Comment:
**Ordering of ungrouped items changed vs. original behaviour**
Before this refactor, `remainingItems` held all items that did NOT match any `localeOrderValues` entry — including items with an unrecognised `localeValue` **and** items whose `localeValue` was `undefined`/`null` — and they were appended in their **original relative order**.
The new Map-based implementation separates these two categories:
1. Items with an unrecognised locale are grouped in `localeGroups` and flushed first (lines 503-507).
2. Items with a falsy `localeValue` are in `ungrouped` and always appended last (line 509).
This means if the original array had, for example, `[{localeValue: undefined}, {localeValue: "zh"}]` where `"zh"` is not in `localeOrderValues`, the old code would produce `[…, {undefined}, {zh}]` while the new code produces `[…, {zh}, {undefined}]`. The relative position of locale-less items and unrecognised-locale items is now reversed compared to the original ordering.
Although this path is gated behind `experimentalSort: 'locales'` and is an edge case, the reordering is silently different from the old behaviour and could surprise users who have both locale-less items and extra-locale items in their composite JSON arrays.
How can I resolve this? If you propose a fix, please make it concise.2 participants
Greptile Summary
This PR delivers three performance optimisations and one bug fix for the CLI's composite JSON merge pipeline:
flattenJson.ts): A newtryFastFlattenhelper handles simple$.*and$.*.propJSONPath patterns with a directObject.keysiteration instead of invokingjsonpath-plus, producing identical RFC 6901 JSON Pointers (with correct~0/~1escaping) and correctly falling back to the full JSONPath engine for anything more complex.mergeJson.ts):JSON.parse(target.translatedContent)was previously called inside nested loops over everysourceObjectPointer. It is now called once per target up-front, eliminating redundant parsing.mergeJson.ts): ThesortByLocaleOrder'locales' mode replaced an O(n²) splice loop with aMap-based grouping strategy. This is correct for the common case, but introduces a subtle ordering difference for edge cases (see inline comment).mergeJson.ts): A newisPrimitiveSourceItemguard correctly short-circuits the JSONPointer mutation path for locale objects whose value is a primitive (string/number/boolean/null), assigning the translated value directly. Previously, spreading a primitive into themergedItemsobject would silently corrupt the result.Confidence Score: 4/5
sortByLocaleOrderrefactor, which may differ from the old behaviour in edge cases but is unlikely to affect production workloads given its experimental status.sortByLocaleOrderrefactor around lines 503-509.Important Files Changed
tryFastFlatten) for simple$.*and$.*.proppatterns, correctly generating RFC 6901 JSON Pointers (with~0/~1escaping) and falling back tojsonpath-plusfor any pattern it cannot handle. Logic is functionally equivalent to the JSONPath slow path for the patterns it covers.isPrimitiveSourceItembranch that fixes handling of primitive locale values. The sort refactor has a minor behavioural difference: items without alocaleValueare now always ordered after items with unrecognised locales, whereas previously they were interleaved in original order.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[flattenJson called with jsonPath] --> B{tryFastFlatten matches simple wildcard?} B -- Yes --> C[Object.keys iteration and RFC 6901 pointer generation] C --> D{Any matches found?} D -- Yes --> E[Return result map] D -- No --> F[Return empty map] B -- No --> G[jsonpath-plus full engine] G --> H[Return pointer to value map] E --> I[Object.assign into extractedJson] F --> I H --> I subgraph mergeJson composite path J[Parse originalContent] --> K[structuredClone into mergedJson] K --> L[Pre-parse ALL targets ONCE into parsedTargets] L --> M[generateSourceObjectPointers via flattenJson] M --> N{sourceObjectOptions type?} N -- array --> O[Loop parsedTargets and build itemsToAdd and indiciesToRemove] N -- object --> P{isPrimitiveSourceItem?} P -- Yes --> Q[Assign translatedValue directly from targetItems root or first value] P -- No --> R[structuredClone and JSONPointer mutations] Q --> S[JSONPointer.set into mergedJson] R --> S O --> T[sortByLocaleOrder using Map grouping] T --> S endPrompt To Fix All With AI
Last reviewed commit: ad020d5