Skip to content

Commit 02fd6ab

Browse files
duranbdandelany
authored andcommitted
Fix inconsistent file extension handling in workspaces (#1899)
* refactor(workspace): extract file extension utilities for reusability Extracted file extension matching and replacement logic into reusable utility functions (`doesFilenameMatchExtension` and `replaceFileExtension`) to improve code consistency and maintainability. Changes: - Added `doesFilenameMatchExtension()` utility to standardize extension checking - Added `replaceFileExtension()` utility to handle filename extension replacement - Replaced inline extension matching logic across ImportWorkspaceFileModal, SequenceEditor, and effects - Added error handling for file conversion operations - Simplified filename manipulation in sequence editor download functionality This refactoring reduces code duplication and provides a single source of truth for file extension operations throughout the workspace handling code. * refactor(workspaces): replace regex with string matching for file extensions Replace regex-based pattern matching with simple string comparison in `doesFilenameMatchExtension` and `replaceFileExtension` functions to avoid unintended behavior with special characters. Changes: - Use `endsWith()` instead of regex for extension matching - Add support for compound extensions (e.g., 'seqn.txt') - Use string slicing instead of regex replacement - Add validation check before replacing extension - Remove tests for regex special character handling - Add tests for compound extension scenarios This prevents issues where special regex characters in extensions (like `*`, `+`, `.`) would be interpreted as patterns rather than literal strings, making extension matching more predictable and correct. --------- Co-authored-by: Dan Delany <daniel.t.delany@jpl.nasa.gov>
1 parent 07c3866 commit 02fd6ab

5 files changed

Lines changed: 138 additions & 21 deletions

File tree

src/components/modals/ImportWorkspaceFileModal.svelte

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import * as Sidebar from '../../components/ui/Sidebar/index.js';
77
import type { Workspace, WorkspaceNodeEvent } from '../../types/workspace';
88
import type { WorkspaceTreeNode } from '../../types/workspace-tree-view';
9-
import { joinPath, removeFirstPathSegment } from '../../utilities/workspaces.js';
9+
import { doesFilenameMatchExtension, joinPath, removeFirstPathSegment } from '../../utilities/workspaces.js';
1010
import InputInternal from '../form/Input.svelte';
1111
import WorkspaceTreeView from '../workspace/WorkspaceTreeView/WorkspaceTreeView.svelte';
1212
import Modal from './Modal.svelte';
@@ -49,9 +49,7 @@
4949
selectedFileGroupings = Array.from(files ?? []).reduce(
5050
(previousFileGroupings: { convertableFiles: File[]; uploadableFiles: File[] }, file) => {
5151
if (
52-
outputLanguageExtensions.findIndex(fileExtension =>
53-
file.name.endsWith(`.${fileExtension.replace(/^\./, '')}`),
54-
) > -1
52+
outputLanguageExtensions.findIndex(fileExtension => doesFilenameMatchExtension(fileExtension, file.name)) > -1
5553
) {
5654
return {
5755
...previousFileGroupings,

src/components/sequencing/SequenceEditor.svelte

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import { blockTheme } from '../../utilities/codemirror/themes/block';
2727
import { phoenixResources } from '../../utilities/sequence-editor/adaptation-resources';
2828
import { showFailureToast, showSuccessToast } from '../../utilities/toast';
29+
import { replaceFileExtension } from '../../utilities/workspaces';
2930
import CssGrid from '../ui/CssGrid.svelte';
3031
import CssGridGutter from '../ui/CssGridGutter.svelte';
3132
import Panel from '../ui/Panel.svelte';
@@ -251,13 +252,12 @@
251252
252253
function downloadOutputFormat(outputLanguage: OutputLanguage): void {
253254
const content = editorOutputView.state.doc.toString();
254-
// Remove any existing extension and add output extension
255-
const outputExt = outputLanguage.fileExtension; // Keep the dot
256-
const lastDotIndex = sequenceName.lastIndexOf('.');
257255
258-
// If there's a dot in the filename, remove everything after it; otherwise keep the whole name
259-
const filenameWithoutExt = lastDotIndex > 0 ? sequenceName.slice(0, lastDotIndex) : sequenceName;
260-
const filename = filenameWithoutExt + outputExt;
256+
const filename = replaceFileExtension(
257+
sequenceName,
258+
sequenceAdaptation.input.fileExtension,
259+
outputLanguage.fileExtension,
260+
);
261261
262262
dispatch('downloadOutput', { content, filePath: sequenceFilePath, filename, outputLanguage });
263263
}

src/utilities/effects.ts

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,7 @@ import {
321321
} from './view';
322322
import {
323323
cleanPath,
324+
doesFilenameMatchExtension,
324325
findNodeInDirectory,
325326
flattenWorkspaceTreeWithPaths,
326327
getWorkspaceFileFolderDisplay,
@@ -329,6 +330,7 @@ import {
329330
isFileConflictResponse,
330331
joinPath,
331332
mapWorkspaceTreePaths,
333+
replaceFileExtension,
332334
separateFilenameFromPath,
333335
WorkspaceApi,
334336
type BulkOperationResponses,
@@ -6096,20 +6098,28 @@ const effects = {
60966098
const convertedFiles: File[] = await Promise.all(
60976099
filesToConvert.map(async file => {
60986100
const outputLanguage = sequenceAdaptation.outputs.find(language =>
6099-
file.name.endsWith(`.${language.fileExtension.replace(/^\./, '')}`),
6101+
doesFilenameMatchExtension(language.fileExtension, file.name),
61006102
);
61016103

61026104
if (outputLanguage) {
6103-
const fileName = file.name.replace(
6104-
outputLanguage.fileExtension.replace(/^\./, ''),
6105-
sequenceAdaptation.input.fileExtension.replace(/^\./, ''),
6106-
);
6107-
const lastModified = Date.now();
6108-
const content = await file.text();
6109-
const convertedContent = outputLanguage.toInputFormat(content, phoenixContext, fileName);
6110-
6111-
convertedFileMap[file.name] = fileName;
6112-
return new File([convertedContent], fileName, { lastModified, type: 'text/plain' });
6105+
try {
6106+
const fileName = replaceFileExtension(
6107+
file.name,
6108+
outputLanguage.fileExtension,
6109+
sequenceAdaptation.input.fileExtension,
6110+
);
6111+
const lastModified = Date.now();
6112+
const content = await file.text();
6113+
const convertedContent = outputLanguage.toInputFormat(content, phoenixContext, fileName);
6114+
6115+
convertedFileMap[file.name] = fileName;
6116+
return new File([convertedContent], fileName, { lastModified, type: 'text/plain' });
6117+
} catch (error) {
6118+
throw Error(
6119+
`There was an error trying to convert the file ${file.name}. Please check that it is formatted correctly.`,
6120+
{ cause: error },
6121+
);
6122+
}
61136123
}
61146124

61156125
return file;

src/utilities/workspaces.test.ts

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
mapWorkspaceTreePaths,
2323
removeFirstPathSegment,
2424
removeRedundantNodes,
25+
replaceFileExtension,
2526
separateFilenameFromPath,
2627
shouldNodeBeVisible,
2728
sortWorkspaceTree,
@@ -1531,6 +1532,93 @@ describe('Workspace utility function tests', () => {
15311532
});
15321533
});
15331534

1535+
describe('replaceFileExtension', () => {
1536+
test('Should replace extension without leading dots', () => {
1537+
expect(replaceFileExtension('file.txt', 'txt', 'json')).toBe('file.json');
1538+
expect(replaceFileExtension('test.seq.json', 'seq.json', 'seqN.txt')).toBe('test.seqN.txt');
1539+
expect(replaceFileExtension('data.xml', 'xml', 'json')).toBe('data.json');
1540+
});
1541+
1542+
test('Should replace extension with leading dots', () => {
1543+
expect(replaceFileExtension('file.txt', '.txt', '.json')).toBe('file.json');
1544+
expect(replaceFileExtension('test.seq', '.seq', '.seqN')).toBe('test.seqN');
1545+
expect(replaceFileExtension('data.xml', '.xml', '.json')).toBe('data.json');
1546+
});
1547+
1548+
test('Should replace extension with mixed dot usage', () => {
1549+
expect(replaceFileExtension('file.txt', 'txt', '.json')).toBe('file.json');
1550+
expect(replaceFileExtension('test.seq', '.seq', 'seqN')).toBe('test.seqN');
1551+
});
1552+
1553+
test('Should be case-insensitive when matching extension', () => {
1554+
expect(replaceFileExtension('file.TXT', 'txt', 'json')).toBe('file.json');
1555+
expect(replaceFileExtension('test.SEQ', 'seq', 'seqN')).toBe('test.seqN');
1556+
expect(replaceFileExtension('data.Xml', 'XML', 'json')).toBe('data.json');
1557+
});
1558+
1559+
test('Should not replace if extension does not match', () => {
1560+
expect(replaceFileExtension('file.txt', 'json', 'xml')).toBe('file.txt');
1561+
expect(replaceFileExtension('test.seq', 'txt', 'json')).toBe('test.seq');
1562+
expect(replaceFileExtension('data.xml', 'seq', 'json')).toBe('data.xml');
1563+
});
1564+
1565+
test('Should only replace extension at the end of filename', () => {
1566+
expect(replaceFileExtension('file.txt.bak', 'txt', 'json')).toBe('file.txt.bak');
1567+
expect(replaceFileExtension('test.seq.old', 'seq', 'seqN')).toBe('test.seq.old');
1568+
});
1569+
1570+
test('Should handle files with multiple dots correctly', () => {
1571+
expect(replaceFileExtension('script.min.js', 'min.js', 'ts')).toBe('script.ts');
1572+
expect(replaceFileExtension('config.test.json', 'json', 'xml')).toBe('config.test.xml');
1573+
expect(replaceFileExtension('my.file.name.txt', 'txt', 'md')).toBe('my.file.name.md');
1574+
});
1575+
1576+
test('Should not replace if filename has no extension', () => {
1577+
expect(replaceFileExtension('README', 'txt', 'md')).toBe('README');
1578+
expect(replaceFileExtension('Makefile', 'txt', 'json')).toBe('Makefile');
1579+
});
1580+
1581+
test('Should handle files with path', () => {
1582+
expect(replaceFileExtension('path/to/file.txt', 'txt', 'json')).toBe('path/to/file.json');
1583+
expect(replaceFileExtension('folder/subfolder/test.seq', 'seq', 'seqN')).toBe('folder/subfolder/test.seqN');
1584+
expect(replaceFileExtension('a/b/c/data.xml', 'xml', 'json')).toBe('a/b/c/data.json');
1585+
});
1586+
1587+
test('Should handle hidden files (starting with dot)', () => {
1588+
expect(replaceFileExtension('.config.json', 'json', 'xml')).toBe('.config.xml');
1589+
expect(replaceFileExtension('.gitignore.txt', 'txt', 'bak')).toBe('.gitignore.bak');
1590+
});
1591+
1592+
test('Should not match partial extension', () => {
1593+
expect(replaceFileExtension('file.txt', 'tx', 'json')).toBe('file.txt');
1594+
expect(replaceFileExtension('file.txt', 't', 'json')).toBe('file.txt');
1595+
expect(replaceFileExtension('file.json', 'son', 'xml')).toBe('file.json');
1596+
});
1597+
1598+
test('Should not replace extension in middle of filename', () => {
1599+
expect(replaceFileExtension('file-txt.json', 'txt', 'xml')).toBe('file-txt.json');
1600+
expect(replaceFileExtension('sequential.js', 'seq', 'seqN')).toBe('sequential.js');
1601+
});
1602+
1603+
test('Should handle empty filename', () => {
1604+
expect(replaceFileExtension('', 'txt', 'json')).toBe('');
1605+
});
1606+
1607+
test('Should preserve target extension case', () => {
1608+
expect(replaceFileExtension('file.txt', 'txt', 'JSON')).toBe('file.JSON');
1609+
expect(replaceFileExtension('test.seq', 'seq', 'SeqN')).toBe('test.SeqN');
1610+
});
1611+
1612+
test('Should handle complex file extensions', () => {
1613+
expect(replaceFileExtension('archive.tar.gz', 'gz', 'bz2')).toBe('archive.tar.bz2');
1614+
expect(replaceFileExtension('script.min.js', 'min.js', 'js')).toBe('script.js');
1615+
});
1616+
1617+
test('Should replace when only extension matches without filename', () => {
1618+
expect(replaceFileExtension('.txt', 'txt', 'json')).toBe('.json');
1619+
});
1620+
});
1621+
15341622
describe('hasReadonlyInTree', () => {
15351623
test('Should return false for a file without metadata', () => {
15361624
const node: WorkspaceTreeNode = {

src/utilities/workspaces.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,27 @@ export function isFileConflictResponse(response: BulkOperationResponse) {
371371
return response.status === 409;
372372
}
373373

374+
export function doesFilenameMatchExtension(extension: string, filename: string) {
375+
// Normalize extension to include leading dot
376+
const normalizedExtension = extension.startsWith('.') ? extension : `.${extension}`;
377+
// Case-insensitive check if filename ends with the extension
378+
return filename.toLowerCase().endsWith(normalizedExtension.toLowerCase());
379+
}
380+
381+
export function replaceFileExtension(filename: string, fromExtension: string, toExtension: string) {
382+
// Normalize extensions to include leading dots
383+
const normalizedFromExtension = fromExtension.startsWith('.') ? fromExtension : `.${fromExtension}`;
384+
const normalizedToExtension = toExtension.startsWith('.') ? toExtension : `.${toExtension}`;
385+
386+
// Check if filename ends with the fromExtension (case-insensitive)
387+
if (!doesFilenameMatchExtension(normalizedFromExtension, filename)) {
388+
return filename;
389+
}
390+
391+
// Replace only the extension at the end, preserving the original case of the filename
392+
return `${filename.slice(0, -normalizedFromExtension.length)}${normalizedToExtension}`;
393+
}
394+
374395
export const WorkspaceApi = {
375396
async createFolder(workspaceId: number, folderPath: string, user: User | null) {
376397
return reqWorkspace<Workspace>(`${workspaceId}/${folderPath}?type=directory`, 'PUT', null, user, undefined, false);

0 commit comments

Comments
 (0)