From 046b4257fddf2749c416f0afddf1a59bb0a7d718 Mon Sep 17 00:00:00 2001 From: Stamen Stoychev Date: Fri, 18 Sep 2026 18:39:20 +0300 Subject: [PATCH 1/4] fix(migrations): keep brackets balanced when updating theme properties `updateThemeProps` matched the owner call with a non paren-aware `owner\([\s\S]+?\);` regex, so for a theme nested in another call - `@include scrollbar(scrollbar-theme($sb-size: 6px))` - the match swallowed the closing bracket of the surrounding call. That bracket ended up riding on the last argument and was dropped together with it whenever the last (or the only) argument was removed, leaving invalid SCSS behind: @include scrollbar(scrollbar-theme(); Locate the call's own closing bracket by scanning and counting brackets instead, skipping strings and comments, and splice the rewritten argument list in by index. This also lets a theme call that is not terminated by `;` on the same statement be migrated. Closes #17642 Co-Authored-By: Claude Opus 5 (1M context) --- .../migrations/common/UpdateChanges.spec.ts | 47 ++++++ .../migrations/common/UpdateChanges.ts | 157 ++++++++++++++---- .../migrations/update-22_2_0/index.spec.ts | 43 +++++ 3 files changed, 212 insertions(+), 35 deletions(-) diff --git a/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts b/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts index 891847cd11f..404e3ab2a34 100644 --- a/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts +++ b/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts @@ -645,6 +645,53 @@ $var3: igx-comp-theme( done(); }); + it('should keep the brackets balanced for nested theme functions', done => { + const themeChangesJson: ThemeChanges = { + changes: [ + { + name: '$remove-me', remove: true, + owner: 'igx-theme-func', + type: ThemeType.Property + }, + { + name: '$replace-me', replaceWith: '$replaced', + owner: 'igx-theme-func', + type: ThemeType.Property + } + ] + }; + const jsonPath = path.join(__dirname, 'changes', 'theme-changes.json'); + spyOn(fs, 'existsSync').and.callFake((filePath: fs.PathLike) => filePath === jsonPath); + spyOn(fs, 'readFileSync').and.callFake(() => JSON.stringify(themeChangesJson)); + + appTree.create('styles.scss', +`@include igx-mixin(igx-theme-func($remove-me: 6px)); +@include igx-mixin(igx-theme-func($prop1: red, $remove-me: 6px)); +@include igx-mixin(igx-theme-func($remove-me: 6px, $prop1: red)); +@include igx-mixin(igx-theme-func($replace-me: 6px)); +$var: igx-theme-func($content: "not a ( bracket", $remove-me: 6px); +$var2: igx-theme-func($image: url(https://example.com/a.png), $remove-me: 6px); +$var3: igx-theme-func( + $remove-me: 6px, // not a ) bracket + $prop1: red +);`); + + const update = new UnitUpdateChanges(__dirname, appTree); + update.applyChanges(); + + expect(appTree.readContent('styles.scss')).toEqual( +`@include igx-mixin(igx-theme-func()); +@include igx-mixin(igx-theme-func($prop1: red)); +@include igx-mixin(igx-theme-func( $prop1: red)); +@include igx-mixin(igx-theme-func($replaced: 6px)); +$var: igx-theme-func($content: "not a ( bracket"); +$var2: igx-theme-func($image: url(https://example.com/a.png)); +$var3: igx-theme-func( // not a ) bracket + $prop1: red +);`); + done(); + }); + it('should replace imports', done => { const importsJson: ImportsChanges = { changes: [ diff --git a/projects/igniteui-angular/migrations/common/UpdateChanges.ts b/projects/igniteui-angular/migrations/common/UpdateChanges.ts index 9479ce34b1e..5580ed5fe38 100644 --- a/projects/igniteui-angular/migrations/common/UpdateChanges.ts +++ b/projects/igniteui-angular/migrations/common/UpdateChanges.ts @@ -397,44 +397,44 @@ export class UpdateChanges { if (change.type !== ThemeType.Property) { continue; } - if (fileContent.indexOf(change.owner) !== -1) { - /** owner-func:( * ); */ - const searchPattern = String.raw`${change.owner}\([\s\S]+?\);`; - const matches = fileContent.match(new RegExp(searchPattern, 'g')); - if (!matches) { + if (fileContent.indexOf(change.owner) === -1) { + continue; + } + /** owner-func:( * ) */ + const calls = this.findFunctionCalls(fileContent, change.owner); + // rewrite back to front so the collected indices stay valid + for (const call of calls.reverse()) { + const rawBody = fileContent.substring(call.bodyStart, call.bodyEnd); + if (rawBody.indexOf(change.name) === -1) { continue; } - for (const match of matches) { - if (match.indexOf(change.name) !== -1) { - const name = change.name.replace('$', '\\$'); - const replaceWith = change.replaceWith?.replace('$', '\\$'); - const reg = new RegExp(String.raw`^\s*${name}:`); - const existing = new RegExp(String.raw`${replaceWith}:`); - const opening = `${change.owner}(`; - const closing = /\s*\);$/.exec(match).pop(); - const body = match.substr(opening.length, match.length - opening.length - closing.length); - - let params = this.splitFunctionProps(body); - params = params.reduce((arr, param) => { - if (reg.test(param)) { - const duplicate = !!replaceWith && arr.some(p => existing.test(p)); - - if (!change.remove && !duplicate) { - arr.push(param.replace(change.name, change.replaceWith)); - } - } else { - arr.push(param); - } - return arr; - }, []); - - fileContent = fileContent.replace( - match, - opening + params.join(',') + closing - ); - overwrite = true; + const name = change.name.replace('$', '\\$'); + const replaceWith = change.replaceWith?.replace('$', '\\$'); + const reg = new RegExp(String.raw`^\s*${name}:`); + const existing = new RegExp(String.raw`${replaceWith}:`); + // keep whatever sits in front of the closing bracket so the formatting is preserved + const trailing = /\s*$/.exec(rawBody).pop(); + const body = rawBody.substring(0, rawBody.length - trailing.length); + + let params = this.splitFunctionProps(body); + params = params.reduce((arr, param) => { + if (reg.test(param)) { + const duplicate = !!replaceWith && arr.some(p => existing.test(p)); + + if (!change.remove && !duplicate) { + arr.push(param.replace(change.name, change.replaceWith)); + } + } else { + arr.push(param); } - } + return arr; + }, []); + + fileContent = fileContent.substring(0, call.bodyStart) + + params.join(',') + + trailing + + fileContent.substring(call.bodyEnd); + overwrite = true; } } if (overwrite) { @@ -442,6 +442,89 @@ export class UpdateChanges { } } + /** + * Returns the argument list boundaries of every top-level `owner(...)` call in the content. + * The brackets are tracked, so a call nested in another one - + * `@include scrollbar(scrollbar-theme($sb-size: 6px))` - reports its own closing bracket + * rather than the one of the call surrounding it. + */ + private findFunctionCalls(content: string, owner: string): { bodyStart: number; bodyEnd: number }[] { + const calls: { bodyStart: number; bodyEnd: number }[] = []; + const opening = `${owner}(`; + let index = content.indexOf(opening); + + while (index !== -1) { + const bodyStart = index + opening.length; + const bodyEnd = this.findClosingBracket(content, bodyStart); + if (bodyEnd === -1) { + // unbalanced content, nothing safe left to rewrite + break; + } + calls.push({ bodyStart, bodyEnd }); + // a same-owner call nested in this one is already covered by it + index = content.indexOf(opening, bodyEnd); + } + + return calls; + } + + /** + * Returns the index of the bracket closing the one `start` is inside of, or -1 when the + * content is unbalanced. Brackets in strings and comments are ignored. + */ + private findClosingBracket(content: string, start: number): number { + let level = 0; + + for (let i = start; i < content.length; i++) { + const char = content[i]; + const next = content[i + 1]; + + if (char === '\'' || char === '"') { + i = this.skipString(content, i); + } else if (char === '/' && next === '*') { + const end = content.indexOf('*/', i + 2); + i = end === -1 ? content.length : end + 1; + } else if (char === '/' && next === '/' && this.isLineCommentStart(content, i)) { + const end = content.indexOf('\n', i + 2); + i = end === -1 ? content.length : end; + } else if (char === '(') { + level++; + } else if (char === ')') { + if (!level) { + return i; + } + level--; + } + } + + return -1; + } + + /** + * Tells apart a `//` line comment from the `//` of a protocol - `url(https://...)` - + * by looking at what precedes it. + */ + private isLineCommentStart(content: string, index: number): boolean { + const previous = content[index - 1]; + + return previous === undefined || /[\s,(;{]/.test(previous); + } + + /** Returns the index of the quote closing the string opened at `start`. */ + private skipString(content: string, start: number): number { + const quote = content[start]; + + for (let i = start + 1; i < content.length; i++) { + if (content[i] === '\\') { + i++; + } else if (content[i] === quote) { + return i; + } + } + + return content.length; + } + protected isNamedArgument(fileContent: string, i: number, occurrences: number[], change: ThemeChange) { const openingBrackets = []; const closingBrackets = []; @@ -879,6 +962,10 @@ export class UpdateChanges { for (let i = 0; i < body.length; i++) { const char = body[i]; + if (char === '\'' || char === '"') { + i = this.skipString(body, i); + continue; + } switch (char) { case '(': level++; break; case ')': level--; break; diff --git a/projects/igniteui-angular/migrations/update-22_2_0/index.spec.ts b/projects/igniteui-angular/migrations/update-22_2_0/index.spec.ts index aa5b7bdaf7e..93b6d56c6c8 100644 --- a/projects/igniteui-angular/migrations/update-22_2_0/index.spec.ts +++ b/projects/igniteui-angular/migrations/update-22_2_0/index.spec.ts @@ -50,6 +50,49 @@ describe(`Update to ${version}`, () => { expect(tree.readContent('/testSrc/appPrefix/component/test.component.scss')).toEqual(content); }); + it('should keep the brackets balanced when the theme is nested in another call', async () => { + appTree.create( + `/testSrc/appPrefix/component/test.component.scss`, + `.selection-area { + @include scrollbar(scrollbar-theme($sb-size: 6px)); +} + +igx-grid { + @include scrollbar(scrollbar-theme($sb-thumb-bg-color: blue, $sb-size: 16px)); +}` + ); + + const tree = await schematicRunner.runSchematic(migrationName, { shouldInvokeLS: false }, appTree); + + expect(tree.readContent('/testSrc/appPrefix/component/test.component.scss')).toEqual( + `.selection-area { + @include scrollbar(scrollbar-theme()); +} + +igx-grid { + @include scrollbar(scrollbar-theme($sb-thumb-bg-color: blue)); +}` + ); + }); + + it('should migrate a theme call that is not terminated by a semicolon', async () => { + appTree.create( + `/testSrc/appPrefix/component/test.component.scss`, + `$my-scrollbar: scrollbar-theme( + $sb-size: 16px, + $sb-thumb-bg-color: blue +)` + ); + + const tree = await schematicRunner.runSchematic(migrationName, { shouldInvokeLS: false }, appTree); + + expect(tree.readContent('/testSrc/appPrefix/component/test.component.scss')).toEqual( + `$my-scrollbar: scrollbar-theme( + $sb-thumb-bg-color: blue +)` + ); + }); + it('should not touch same-named properties on other themes', async () => { const content = `$my-grid: grid-theme( $sb-size: 16px From 2fdaa68950ed89e36709ac30a55efd7f570cc707 Mon Sep 17 00:00:00 2001 From: Stamen Stoychev Date: Fri, 18 Sep 2026 18:50:36 +0300 Subject: [PATCH 2/4] fix(migrations): escape every `$` before building the property regex `$` is a regex anchor, so the single-occurrence `replace` the escaping relied on was incomplete. Flagged by CodeQL (js/incomplete-sanitization). Co-Authored-By: Claude Opus 5 (1M context) --- projects/igniteui-angular/migrations/common/UpdateChanges.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/projects/igniteui-angular/migrations/common/UpdateChanges.ts b/projects/igniteui-angular/migrations/common/UpdateChanges.ts index 5580ed5fe38..260bd01c9a3 100644 --- a/projects/igniteui-angular/migrations/common/UpdateChanges.ts +++ b/projects/igniteui-angular/migrations/common/UpdateChanges.ts @@ -408,8 +408,9 @@ export class UpdateChanges { if (rawBody.indexOf(change.name) === -1) { continue; } - const name = change.name.replace('$', '\\$'); - const replaceWith = change.replaceWith?.replace('$', '\\$'); + // `$` is a regex anchor, escape every one of them before interpolating + const name = change.name.replace(/\$/g, '\\$'); + const replaceWith = change.replaceWith?.replace(/\$/g, '\\$'); const reg = new RegExp(String.raw`^\s*${name}:`); const existing = new RegExp(String.raw`${replaceWith}:`); // keep whatever sits in front of the closing bracket so the formatting is preserved From 8bc7d16bb1f852ed85cf515cb2b17644917d4f4b Mon Sep 17 00:00:00 2001 From: Stamen Stoychev Date: Fri, 18 Sep 2026 19:03:33 +0300 Subject: [PATCH 3/4] refactor(migrations): use the shared escapeRegExp for the theme property regex The hand-rolled `$` escaping was incomplete either way - CodeQL flagged the single-occurrence replace, then the unescaped backslash. The file already imports `escapeRegExp` from ./util and uses it elsewhere, so reuse it here rather than keeping a partial escape of our own. No behavior change: a theme property name only ever contains `$` and `-`. Co-Authored-By: Claude Opus 5 (1M context) --- projects/igniteui-angular/migrations/common/UpdateChanges.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/projects/igniteui-angular/migrations/common/UpdateChanges.ts b/projects/igniteui-angular/migrations/common/UpdateChanges.ts index 260bd01c9a3..d2fab10afb4 100644 --- a/projects/igniteui-angular/migrations/common/UpdateChanges.ts +++ b/projects/igniteui-angular/migrations/common/UpdateChanges.ts @@ -408,9 +408,8 @@ export class UpdateChanges { if (rawBody.indexOf(change.name) === -1) { continue; } - // `$` is a regex anchor, escape every one of them before interpolating - const name = change.name.replace(/\$/g, '\\$'); - const replaceWith = change.replaceWith?.replace(/\$/g, '\\$'); + const name = escapeRegExp(change.name); + const replaceWith = change.replaceWith ? escapeRegExp(change.replaceWith) : undefined; const reg = new RegExp(String.raw`^\s*${name}:`); const existing = new RegExp(String.raw`${replaceWith}:`); // keep whatever sits in front of the closing bracket so the formatting is preserved From 7c4cd7b0fbede61d61f872db6de208cb17d0189e Mon Sep 17 00:00:00 2001 From: Stamen Stoychev Date: Fri, 18 Sep 2026 19:12:23 +0300 Subject: [PATCH 4/4] fix(migrations): skip strings and comments when looking for a theme call `findFunctionCalls` located the opening `owner(` with a plain `indexOf`, so a theme function merely mentioned in a comment or a quoted value was taken for a real call. With an unbalanced `(` in the comment the scan then ran past it and claimed the closing bracket of the next genuine call, rewriting across unrelated source and leaving that call unmigrated. Scan for the opening the same way the bracket matching already did, stepping over strings and comments via a shared `skipNonCode`. Co-Authored-By: Claude Opus 5 (1M context) --- .../migrations/common/UpdateChanges.spec.ts | 31 +++++++++ .../migrations/common/UpdateChanges.ts | 63 ++++++++++++++----- 2 files changed, 78 insertions(+), 16 deletions(-) diff --git a/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts b/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts index 404e3ab2a34..3f1a28849f3 100644 --- a/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts +++ b/projects/igniteui-angular/migrations/common/UpdateChanges.spec.ts @@ -692,6 +692,37 @@ $var3: igx-theme-func( // not a ) bracket done(); }); + it('should not treat a theme function mentioned in a comment or a string as a call', done => { + const themeChangesJson: ThemeChanges = { + changes: [ + { + name: '$remove-me', remove: true, + owner: 'igx-theme-func', + type: ThemeType.Property + } + ] + }; + const jsonPath = path.join(__dirname, 'changes', 'theme-changes.json'); + spyOn(fs, 'existsSync').and.callFake((filePath: fs.PathLike) => filePath === jsonPath); + spyOn(fs, 'readFileSync').and.callFake(() => JSON.stringify(themeChangesJson)); + + appTree.create('styles.scss', +`// igx-theme-func($remove-me: 1px) is gone, use the grid's own borders +/* igx-theme-func($remove-me: 2px */ +$doc: "igx-theme-func($remove-me: 3px"; +$var: igx-theme-func($remove-me: 4px, $prop1: red);`); + + const update = new UnitUpdateChanges(__dirname, appTree); + update.applyChanges(); + + expect(appTree.readContent('styles.scss')).toEqual( +`// igx-theme-func($remove-me: 1px) is gone, use the grid's own borders +/* igx-theme-func($remove-me: 2px */ +$doc: "igx-theme-func($remove-me: 3px"; +$var: igx-theme-func( $prop1: red);`); + done(); + }); + it('should replace imports', done => { const importsJson: ImportsChanges = { changes: [ diff --git a/projects/igniteui-angular/migrations/common/UpdateChanges.ts b/projects/igniteui-angular/migrations/common/UpdateChanges.ts index d2fab10afb4..9e89789f5fa 100644 --- a/projects/igniteui-angular/migrations/common/UpdateChanges.ts +++ b/projects/igniteui-angular/migrations/common/UpdateChanges.ts @@ -444,25 +444,35 @@ export class UpdateChanges { /** * Returns the argument list boundaries of every top-level `owner(...)` call in the content. - * The brackets are tracked, so a call nested in another one - + * Strings and comments are scanned over, so an `owner(` that is only mentioned in one is not + * taken for a call. The brackets are tracked too, so a call nested in another one - * `@include scrollbar(scrollbar-theme($sb-size: 6px))` - reports its own closing bracket * rather than the one of the call surrounding it. */ private findFunctionCalls(content: string, owner: string): { bodyStart: number; bodyEnd: number }[] { const calls: { bodyStart: number; bodyEnd: number }[] = []; const opening = `${owner}(`; - let index = content.indexOf(opening); - while (index !== -1) { - const bodyStart = index + opening.length; + for (let i = 0; i < content.length; i++) { + const nonCodeEnd = this.skipNonCode(content, i); + if (nonCodeEnd !== -1) { + i = nonCodeEnd; + continue; + } + if (!content.startsWith(opening, i)) { + continue; + } + + const bodyStart = i + opening.length; const bodyEnd = this.findClosingBracket(content, bodyStart); if (bodyEnd === -1) { // unbalanced content, nothing safe left to rewrite break; } + calls.push({ bodyStart, bodyEnd }); // a same-owner call nested in this one is already covered by it - index = content.indexOf(opening, bodyEnd); + i = bodyEnd; } return calls; @@ -476,18 +486,14 @@ export class UpdateChanges { let level = 0; for (let i = start; i < content.length; i++) { - const char = content[i]; - const next = content[i + 1]; + const nonCodeEnd = this.skipNonCode(content, i); + if (nonCodeEnd !== -1) { + i = nonCodeEnd; + continue; + } - if (char === '\'' || char === '"') { - i = this.skipString(content, i); - } else if (char === '/' && next === '*') { - const end = content.indexOf('*/', i + 2); - i = end === -1 ? content.length : end + 1; - } else if (char === '/' && next === '/' && this.isLineCommentStart(content, i)) { - const end = content.indexOf('\n', i + 2); - i = end === -1 ? content.length : end; - } else if (char === '(') { + const char = content[i]; + if (char === '(') { level++; } else if (char === ')') { if (!level) { @@ -500,6 +506,31 @@ export class UpdateChanges { return -1; } + /** + * When a string or a comment starts at `index`, returns the index of its last character so + * that a scan can carry on past it. Returns -1 when `index` is on code. + */ + private skipNonCode(content: string, index: number): number { + const char = content[index]; + const next = content[index + 1]; + + if (char === '\'' || char === '"') { + return this.skipString(content, index); + } + if (char === '/' && next === '*') { + const end = content.indexOf('*/', index + 2); + + return end === -1 ? content.length : end + 1; + } + if (char === '/' && next === '/' && this.isLineCommentStart(content, index)) { + const end = content.indexOf('\n', index + 2); + + return end === -1 ? content.length : end; + } + + return -1; + } + /** * Tells apart a `//` line comment from the `//` of a protocol - `url(https://...)` - * by looking at what precedes it.