Skip to content

Commit 3a848ee

Browse files
Jimmy-Joseph19Jimmy-Joseph19
committed
fix: ai feedback
1 parent 6c98b63 commit 3a848ee

3 files changed

Lines changed: 114 additions & 97 deletions

File tree

‎packages/fe-fpm-writer/src/common/file.ts‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -168,32 +168,14 @@ export const CONFIG = {
168168
function generateUniqueElementId(baseId: string, filteredFilesContent: string[], validatedIds: string[] = []): string {
169169
const maxAttempts = 1000;
170170

171-
/**
172-
* Checks if an element with the specified id is available (does not exist) in the XML content.
173-
*
174-
* @param id - id to check for availability
175-
* @param xmlContent - XML content as string
176-
* @returns true if the id is available (not found), false if it exists
177-
*/
178-
function checkElementIdAvailable(id: string, xmlContent: string): boolean {
179-
// Use validateId from @sap-ux/project-access (synchronous overload)
180-
return validateId(id, undefined, { files: [xmlContent] });
181-
}
182-
183-
if (
184-
filteredFilesContent.every((content) => content === '' || checkElementIdAvailable(baseId, content)) &&
185-
!validatedIds.includes(baseId)
186-
) {
171+
if (validateId(baseId, validatedIds, { files: filteredFilesContent })) {
187172
return baseId;
188173
}
189174

190175
for (let counter = 1; counter < maxAttempts; counter++) {
191176
const candidateId = `${baseId}${counter}`;
192177

193-
if (
194-
filteredFilesContent.every((content) => content === '' || checkElementIdAvailable(candidateId, content)) &&
195-
!validatedIds.includes(candidateId)
196-
) {
178+
if (validateId(candidateId, validatedIds, { files: filteredFilesContent })) {
197179
return candidateId;
198180
}
199181
}

‎packages/project-access/src/library/helpers.ts‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import { XMLParser } from 'fast-xml-parser';
1515
import { getI18nPropertiesPaths } from '../project/i18n';
1616
import { getPropertiesI18nBundle } from '@sap-ux/i18n';
1717
import type { Editor } from 'mem-fs-editor';
18+
import { create as createStorage } from 'mem-fs';
19+
import { create } from 'mem-fs-editor';
1820

1921
/**
2022
* Reads the manifest file and returns the reuse library.
@@ -459,18 +461,24 @@ export function validateId(
459461

460462
// Asynchronous path: when appPath is provided or no options
461463
return (async (): Promise<boolean> => {
462-
let files: string[] | undefined = fileContents;
463-
if (!files && appPath) {
464-
const xmlFiles = await findFilesByExtension(
464+
let files: string[] | undefined;
465+
if (appPath) {
466+
// Ensure we have a memFs instance
467+
const fsEditor = memFs ?? create(createStorage());
468+
469+
const xmlFilePaths = await findFilesByExtension(
465470
'.xml',
466471
appPath,
467472
['.git', 'node_modules', 'dist', 'annotations', 'localService'],
468-
memFs
473+
fsEditor
469474
);
470475
const lookupFiles = ['.fragment.xml', '.view.xml'];
471-
files = xmlFiles.filter((fileName: string) =>
476+
const filteredPaths = xmlFilePaths.filter((fileName: string) =>
472477
lookupFiles.some((lookupFile) => fileName.endsWith(lookupFile))
473478
);
479+
480+
// Read file contents from paths using memFs
481+
files = filteredPaths.map((path: string) => fsEditor.read(path));
474482
}
475483

476484
if (files) {

‎packages/project-access/test/library/helpers.test.ts‎

Lines changed: 99 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -156,109 +156,136 @@ describe('validateId', () => {
156156
</f:SimpleForm>
157157
</mvc:View>`;
158158

159-
test('should return true when id does not exist in any files', async () => {
160-
const result = await validateId('newButton', undefined, {
161-
files: [sampleView, sampleFragment]
159+
describe('synchronous overload (with files)', () => {
160+
test('should return true when id does not exist in any files', () => {
161+
const result = validateId('newButton', undefined, {
162+
files: [sampleView, sampleFragment]
163+
});
164+
// Assert it's a plain boolean, not a Promise
165+
expect(result).toBe(true);
166+
expect(result).not.toBeInstanceOf(Promise);
162167
});
163-
expect(result).toBe(true);
164-
});
165168

166-
test('should return false when id exists in view', async () => {
167-
const result = await validateId('submitButton', undefined, {
168-
files: [sampleView, sampleFragment]
169+
test('should return false when id exists in view', () => {
170+
const result = validateId('submitButton', undefined, {
171+
files: [sampleView, sampleFragment]
172+
});
173+
expect(result).toBe(false);
174+
expect(result).not.toBeInstanceOf(Promise);
169175
});
170-
expect(result).toBe(false);
171-
});
172176

173-
test('should return false when id exists in fragment', async () => {
174-
const result = await validateId('confirmDialog', undefined, {
175-
files: [sampleView, sampleFragment]
177+
test('should return false when id exists in fragment', () => {
178+
const result = validateId('confirmDialog', undefined, {
179+
files: [sampleView, sampleFragment]
180+
});
181+
expect(result).toBe(false);
182+
expect(result).not.toBeInstanceOf(Promise);
176183
});
177-
expect(result).toBe(false);
178-
});
179184

180-
test('should return false when id is in validatedIds array', async () => {
181-
const result = await validateId('newButton', ['newButton', 'anotherButton'], {
182-
files: [sampleView, sampleFragment]
185+
test('should return false when id is in validatedIds array', () => {
186+
const result = validateId('newButton', ['newButton', 'anotherButton'], {
187+
files: [sampleView, sampleFragment]
188+
});
189+
expect(result).toBe(false);
190+
expect(result).not.toBeInstanceOf(Promise);
183191
});
184-
expect(result).toBe(false);
185-
});
186192

187-
test('should return true when id is unique across multiple files', async () => {
188-
const result = await validateId('uniqueId', undefined, {
189-
files: [sampleView, sampleFragment, sampleViewWithNamespace]
193+
test('should return true when id is unique across multiple files', () => {
194+
const result = validateId('uniqueId', undefined, {
195+
files: [sampleView, sampleFragment, sampleViewWithNamespace]
196+
});
197+
expect(result).toBe(true);
198+
expect(result).not.toBeInstanceOf(Promise);
190199
});
191-
expect(result).toBe(true);
192-
});
193200

194-
test('should return false when id exists in nested elements', async () => {
195-
const result = await validateId('dataTable', undefined, {
196-
files: [sampleView]
201+
test('should return false when id exists in nested elements', () => {
202+
const result = validateId('dataTable', undefined, {
203+
files: [sampleView]
204+
});
205+
expect(result).toBe(false);
206+
expect(result).not.toBeInstanceOf(Promise);
197207
});
198-
expect(result).toBe(false);
199-
});
200208

201-
test('should return false when id exists in fragment dialog content', async () => {
202-
const result = await validateId('dialogText', undefined, {
203-
files: [sampleFragment]
209+
test('should return false when id exists in fragment dialog content', () => {
210+
const result = validateId('dialogText', undefined, {
211+
files: [sampleFragment]
212+
});
213+
expect(result).toBe(false);
214+
expect(result).not.toBeInstanceOf(Promise);
204215
});
205-
expect(result).toBe(false);
206-
});
207216

208-
test('should return true for empty files array', async () => {
209-
const result = await validateId('anyId', undefined, {
210-
files: []
217+
test('should return true for empty files array', () => {
218+
const result = validateId('anyId', undefined, {
219+
files: []
220+
});
221+
expect(result).toBe(true);
222+
expect(result).not.toBeInstanceOf(Promise);
211223
});
212-
expect(result).toBe(true);
213-
});
214224

215-
test('should return true when XML parsing fails', async () => {
216-
// fast-xml-parser is lenient, but completely invalid content should fail
217-
const invalidXml = '<<<>>><invalid';
218-
const result = await validateId('test', undefined, {
219-
files: [invalidXml]
225+
test('should return true when XML parsing fails', () => {
226+
// fast-xml-parser is lenient, but completely invalid content should fail
227+
const invalidXml = '<<<>>><invalid';
228+
const result = validateId('test', undefined, {
229+
files: [invalidXml]
230+
});
231+
expect(result).toBe(true);
232+
expect(result).not.toBeInstanceOf(Promise);
220233
});
221-
expect(result).toBe(true);
222-
});
223234

224-
test('should return true when files contain empty strings', async () => {
225-
const result = await validateId('testId', undefined, {
226-
files: ['', '', sampleView]
235+
test('should return true when files contain empty strings', () => {
236+
const result = validateId('testId', undefined, {
237+
files: ['', '', sampleView]
238+
});
239+
expect(result).toBe(true);
240+
expect(result).not.toBeInstanceOf(Promise);
227241
});
228-
expect(result).toBe(true);
229-
});
230242

231-
test('should handle ids with special characters', async () => {
232-
const xmlWithSpecialId = `<?xml version="1.0" encoding="UTF-8"?>
243+
test('should handle ids with special characters', () => {
244+
const xmlWithSpecialId = `<?xml version="1.0" encoding="UTF-8"?>
233245
<mvc:View xmlns:mvc="sap.ui.core.mvc" xmlns="sap.m">
234246
<Button id="button-with-dash" text="Test" />
235247
<Button id="button_with_underscore" text="Test" />
236248
<Button id="button.with.dot" text="Test" />
237249
</mvc:View>`;
238250

239-
const result1 = await validateId('button-with-dash', undefined, {
240-
files: [xmlWithSpecialId]
241-
});
242-
expect(result1).toBe(false);
251+
const result1 = validateId('button-with-dash', undefined, {
252+
files: [xmlWithSpecialId]
253+
});
254+
expect(result1).toBe(false);
255+
expect(result1).not.toBeInstanceOf(Promise);
243256

244-
const result2 = await validateId('button_with_underscore', undefined, {
245-
files: [xmlWithSpecialId]
246-
});
247-
expect(result2).toBe(false);
257+
const result2 = validateId('button_with_underscore', undefined, {
258+
files: [xmlWithSpecialId]
259+
});
260+
expect(result2).toBe(false);
261+
expect(result2).not.toBeInstanceOf(Promise);
248262

249-
const result3 = await validateId('button.with.dot', undefined, {
250-
files: [xmlWithSpecialId]
251-
});
252-
expect(result3).toBe(false);
263+
const result3 = validateId('button.with.dot', undefined, {
264+
files: [xmlWithSpecialId]
265+
});
266+
expect(result3).toBe(false);
267+
expect(result3).not.toBeInstanceOf(Promise);
253268

254-
const result4 = await validateId('button-not-exists', undefined, {
255-
files: [xmlWithSpecialId]
269+
const result4 = validateId('button-not-exists', undefined, {
270+
files: [xmlWithSpecialId]
271+
});
272+
expect(result4).toBe(true);
273+
expect(result4).not.toBeInstanceOf(Promise);
256274
});
257-
expect(result4).toBe(true);
258275
});
259276

260-
test('should return true when no options provided', async () => {
261-
const result = await validateId('anyId');
262-
expect(result).toBe(true);
277+
describe('asynchronous overload (no files)', () => {
278+
test('should return Promise when no options provided', async () => {
279+
const result = validateId('anyId');
280+
// Assert it's a Promise
281+
expect(result).toBeInstanceOf(Promise);
282+
expect(await result).toBe(true);
283+
});
284+
285+
test('should return Promise when only validatedIds provided', async () => {
286+
const result = validateId('anyId', ['otherId']);
287+
expect(result).toBeInstanceOf(Promise);
288+
expect(await result).toBe(true);
289+
});
263290
});
264291
});

0 commit comments

Comments
 (0)