Skip to content

feat: improve UX handling when multiple main files detected #1700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/renderer/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { Runner } from './runner';
import { AppState } from './state';
import { TaskRunner } from './task-runner';
import { activateTheme, getCurrentTheme, getTheme } from './themes';
import { isMainEntryPoint } from './utils/editor-utils';
import { getPackageJson } from './utils/get-package';
import { getElectronVersions } from './versions';
import {
Expand All @@ -33,6 +34,7 @@ export class App {
public runner = new Runner(this.state);
public readonly taskRunner: TaskRunner;
public readonly electronTypes: ElectronTypes;
private notifiedMultipleMainFiles = false;

constructor() {
this.getEditorValues = this.getEditorValues.bind(this);
Expand All @@ -56,6 +58,17 @@ export class App {
});
}

private notifyIfMultipleMainFiles(editorValues: EditorValues) {
const mainFileCount =
Object.keys(editorValues).filter(isMainEntryPoint).length;
if (mainFileCount > 1 && !this.notifiedMultipleMainFiles) {
this.state.showInfoDialog(
'Multiple main entry point files detected. You can right-click on any main file and select "Set as Main Entry Point" to choose which one to use.',
);
this.notifiedMultipleMainFiles = true;
}
}

public async replaceFiddle(
editorValues: EditorValues,
{ localFiddle, gistId, templateName }: Partial<SetFiddleOptions>,
Expand All @@ -69,6 +82,8 @@ export class App {

this.state.editorMosaic.set(editorValues);

this.notifyIfMultipleMainFiles(editorValues);

this.state.gistId = gistId || '';
this.state.localPath = localFiddle?.filePath;
this.state.templateName = templateName;
Expand Down
26 changes: 25 additions & 1 deletion src/renderer/components/sidebar-file-tree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ export const SidebarFileTree = observer(
.map(([editorId, presence], index) => {
const visibilityIcon =
presence !== EditorPresence.Hidden ? 'eye-open' : 'eye-off';

const isInactive =
isMainEntryPoint(editorId) && this.getMainEntryPoint() !== editorId;
return {
isSelected: focusedFile === editorId,
id: index,
Expand All @@ -58,6 +59,7 @@ export const SidebarFileTree = observer(
label: (
<ContextMenu2
className="pointer"
style={isInactive ? { opacity: 0.5 } : undefined}
onClick={() => this.setFocusedFile(editorId)}
content={
<Menu>
Expand All @@ -67,6 +69,14 @@ export const SidebarFileTree = observer(
intent="primary"
onClick={() => this.renameEditor(editorId)}
/>
{isMainEntryPoint(editorId) && (
<MenuItem
icon="star"
text="Set as Main Entry Point"
intent="primary"
onClick={() => this.setMainEntryPoint(editorId)}
/>
)}
<MenuItem
disabled={isMainEntryPoint(editorId)}
icon="remove"
Expand Down Expand Up @@ -215,6 +225,20 @@ export const SidebarFileTree = observer(
}
};

public setMainEntryPoint = (editorId: EditorId) => {
const { appState } = this.props;
try {
appState.editorMosaic.setMainEntryPoint(editorId);
} catch (err) {
appState.showErrorDialog(err.message);
}
};

public getMainEntryPoint = () => {
const { editorMosaic } = this.props.appState;
return editorMosaic.mainEntryPointFile();
};

public removeEditor = (editorId: EditorId) => {
const { editorMosaic } = this.props.appState;
editorMosaic.remove(editorId);
Expand Down
27 changes: 26 additions & 1 deletion src/renderer/editor-mosaic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ interface EditorBackup {
}

export class EditorMosaic {
public mainEntryPoint: EditorId | null = null;
public isEdited = false;
public focusedFile: EditorId | null = null;

Expand Down Expand Up @@ -71,6 +72,7 @@ export class EditorMosaic {
mosaic: observable,
backups: observable,
editors: observable,
mainEntryPoint: observable,
setFocusedFile: action,
resetLayout: action,
set: action,
Expand All @@ -79,6 +81,7 @@ export class EditorMosaic {
setVisible: action,
toggle: action,
hide: action,
setMainEntryPoint: action,
remove: action,
addEditor: action,
setEditorFromBackup: action,
Expand Down Expand Up @@ -352,7 +355,29 @@ export class EditorMosaic {
}

public mainEntryPointFile(): EditorId | undefined {
return Array.from(this.files.keys()).find((id) => isMainEntryPoint(id));
if (!this.mainEntryPoint || !this.files.get(this.mainEntryPoint)) {
const entryPoint = Array.from(this.files.keys()).find((id) =>
isMainEntryPoint(id),
);
if (entryPoint) this.mainEntryPoint = entryPoint;
return entryPoint;
}
return this.mainEntryPoint;
Comment on lines +358 to +365
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!this.mainEntryPoint || !this.files.get(this.mainEntryPoint)) {
const entryPoint = Array.from(this.files.keys()).find((id) =>
isMainEntryPoint(id),
);
if (entryPoint) this.mainEntryPoint = entryPoint;
return entryPoint;
}
return this.mainEntryPoint;
public mainEntryPointFile(): EditorId | null {
if (this.mainEntryPoint && this.files.get(this.mainEntryPoint)) {
return this.mainEntryPoint;
}
for (const [id] of this.files) {
if (isMainEntryPoint(id)) {
this.mainEntryPoint = id;
break;
}
}
return this.mainEntryPoint;

Make the code more intuitive and optimize performance a bit.

}

public setMainEntryPoint(id: EditorId): void {
if (!this.files.has(id)) {
throw new Error(
`Cannot set main entry point to "${id}": File does not exist`,
);
}
if (!isMainEntryPoint(id)) {
throw new Error(
`Cannot set main entry point to "${id}": Not a valid main entry point file`,
);
}
this.mainEntryPoint = id;
this.isEdited = true;
}

//=== Listen for user edits
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ exports[`SidebarFileTree component can bring up the Add File input 1`] = `
shouldDismissPopover={true}
text="Rename"
/>
<Blueprint3.MenuItem
disabled={false}
icon="star"
intent="primary"
multiline={false}
onClick={[Function]}
popoverProps={{}}
shouldDismissPopover={true}
text="Set as Main Entry Point"
/>
<Blueprint3.MenuItem
disabled={true}
icon="remove"
Expand Down Expand Up @@ -422,6 +432,16 @@ exports[`SidebarFileTree component reflects the visibility state of all icons 1`
shouldDismissPopover={true}
text="Rename"
/>
<Blueprint3.MenuItem
disabled={false}
icon="star"
intent="primary"
multiline={false}
onClick={[Function]}
popoverProps={{}}
shouldDismissPopover={true}
text="Set as Main Entry Point"
/>
<Blueprint3.MenuItem
disabled={true}
icon="remove"
Expand Down Expand Up @@ -744,6 +764,16 @@ exports[`SidebarFileTree component renders 1`] = `
shouldDismissPopover={true}
text="Rename"
/>
<Blueprint3.MenuItem
disabled={false}
icon="star"
intent="primary"
multiline={false}
onClick={[Function]}
popoverProps={{}}
shouldDismissPopover={true}
text="Set as Main Entry Point"
/>
<Blueprint3.MenuItem
disabled={true}
icon="remove"
Expand Down
96 changes: 96 additions & 0 deletions tests/renderer/components/sidebar-file-tree-spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
EditorValues,
MAIN_CJS,
MAIN_JS,
MAIN_MJS,
PACKAGE_NAME,
} from '../../../src/interfaces';
import { Editors } from '../../../src/renderer/components/editors';
Expand Down Expand Up @@ -193,6 +194,101 @@ describe('SidebarFileTree component', () => {
expect(editorMosaic.files.get(TO_BE_NAMED)).toBe(EditorPresence.Pending);
});

it('can set a new main entry point file', () => {
const wrapper = shallow(<SidebarFileTree appState={store} />);
const instance: any = wrapper.instance();

// Add MAIN_CJS to the mosaic
// NOTE: Using direct map manipulation for test setup only.
// In production code, files would be added through proper channels.
editorMosaic.files.set(MAIN_CJS, EditorPresence.Visible);

instance.setMainEntryPoint(MAIN_JS);

expect(instance.getMainEntryPoint()).toBe(MAIN_JS);
expect(editorMosaic.mainEntryPointFile()).toBe(MAIN_JS);

instance.setMainEntryPoint(MAIN_CJS);

expect(instance.getMainEntryPoint()).toBe(MAIN_CJS);
expect(editorMosaic.mainEntryPointFile()).toBe(MAIN_CJS);
});

it('fails when trying to set an invalid file as main entry point', () => {
const wrapper = shallow(<SidebarFileTree appState={store} />);
const instance: any = wrapper.instance();

const REGULAR_FILE = 'index.html';

store.showErrorDialog = jest.fn().mockResolvedValueOnce(true);

instance.setMainEntryPoint(REGULAR_FILE);

expect(store.showErrorDialog).toHaveBeenCalledWith(
`Cannot set main entry point to "${REGULAR_FILE}": Not a valid main entry point file`,
);
});

it('fails when trying to set a non-existent file as main entry point', () => {
const wrapper = shallow(<SidebarFileTree appState={store} />);
const instance: any = wrapper.instance();

const NON_EXISTENT_FILE = 'non-existent-file.js';

store.showErrorDialog = jest.fn().mockResolvedValueOnce(true);

instance.setMainEntryPoint(NON_EXISTENT_FILE);

expect(store.showErrorDialog).toHaveBeenCalledWith(
`Cannot set main entry point to "${NON_EXISTENT_FILE}": File does not exist`,
);
});

it('selects an available alternate main file after renaming active main file', async () => {
const wrapper = shallow(<SidebarFileTree appState={store} />);
const instance: any = wrapper.instance();

// Add MAIN_CJS, MAIN_MJS to the mosaic
// NOTE: Using set() for test setup only.
// In production code, files would be added through proper channels.
editorMosaic.set({
...createEditorValues(),
[MAIN_CJS]: '// main.cjs content',
[MAIN_MJS]: '// main.mjs content',
});

instance.setMainEntryPoint(MAIN_JS);

expect(instance.getMainEntryPoint()).toBe(MAIN_JS);

store.showInputDialog = jest
.fn()
.mockResolvedValueOnce('not-a-main-file.js');

await instance.renameEditor(MAIN_JS);

const newMainFile = instance.getMainEntryPoint();
expect(newMainFile).not.toBe(MAIN_JS);
expect([MAIN_CJS, MAIN_MJS]).toContain(newMainFile);
});

it('sets main entry point to undefined when no main files exist after renaming', async () => {
const wrapper = shallow(<SidebarFileTree appState={store} />);
const instance: any = wrapper.instance();

instance.setMainEntryPoint(MAIN_JS);

expect(instance.getMainEntryPoint()).toBe(MAIN_JS);

store.showInputDialog = jest
.fn()
.mockResolvedValueOnce('not-a-main-file.js');

await instance.renameEditor(MAIN_JS);

expect(instance.getMainEntryPoint()).toBeUndefined();
});

it('can reset the editor layout', () => {
const wrapper = shallow(<SidebarFileTree appState={store} />);
const instance: any = wrapper.instance();
Expand Down