Skip to content

feat: connect workspace filesystem navigator #106

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: master
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
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
<form
[formGroup]="form"
class="connect-workspace-form"
(ngSubmit)="connectWorkspace()"
>
<h2 class="mat-title">Connect your Angular workspace</h2>
<mat-form-field class="workspace-path-field" appearance="outline">
<mat-label>The absolute path to your workspace</mat-label>
<input formControlName="path" matInput type="text" />
</mat-form-field>
<button mat-raised-button color="primary" type="submit">Connect</button>
</form>
<div class="fs-navigator-wrapper">
<cli-filesystem-navigator
[directories]="directories$ | async"
[separator]="pathSeparator$ | async"
[path]="path$ | async"
(pathChange)="onPathChanged($event)"
>
<ng-container
actions
[ngTemplateOutlet]="fsNavigatorActions"
></ng-container>
Comment on lines +8 to +11
Copy link
Member

Choose a reason for hiding this comment

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

I think I would've put it directly in the toolbar component instead of using ng-content twice (this goes with the previous comment of moving the isAngularWorkspace logic into the navigator component)

</cli-filesystem-navigator>
</div>

<ng-template #fsNavigatorActions>
<button
mat-raised-button
color="primary"
[disabled]="(isAngularWorkspace$ | async) === false"
Copy link
Member

Choose a reason for hiding this comment

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

IMO it should be hidden entirely if not angular workspace (having a disabled button without some explanation can be confusing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the explanation is in the icons of the file system
if you click an angular workspace folder the connect button is enabled
if you click a regular folder it's disabled

(click)="connectWorkspace()"
>
Connect
</button>
</ng-template>
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
:host {
display: flex;
flex: 1;
}

.connect-workspace-form {
display: grid;
place-content: center;
width: 100%;
flex-direction: column;
align-items: center;

.workspace-path-field {
min-width: 350px;
.fs-navigator-wrapper {
width: 70vw;
height: 70vh;
margin-top: 24px;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,138 @@ import { HttpClientTestingModule } from '@angular/common/http/testing';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { NoopAnimationsModule } from '@angular/platform-browser/animations';
import { RouterTestingModule } from '@angular/router/testing';
import { CURRENT_WORKSPACE_PATH } from '@angular-cli-gui/shared/data';
import { ConnectWorkspaceService } from '@angular-cli-gui/workspace-manager';

import { WorkspaceManagerApiService } from '../../data-access/workspace-manager-api.service';
import {
ConnectWorkspaceServiceMock,
DIRECTORIES_MOCK,
getConnectWorkspaceServiceMock,
getWorkspaceManagerApiServiceMock,
HOMEDIR_PATH_MOCK,
PATH_SEPARATOR_MOCK,
WorkspaceManagerApiServiceMock,
} from '../../mocks/connect-workspace.mock';

import { ConnectWorkspaceComponent } from './connect-workspace.component';

describe('ConnectWorkspaceComponent', () => {
let component: ConnectWorkspaceComponent;
let fixture: ComponentFixture<ConnectWorkspaceComponent>;
let workspaceManagerApiServiceMock: WorkspaceManagerApiServiceMock;
let connectWorkspaceServiceMock: ConnectWorkspaceServiceMock;

beforeEach(async () => {
workspaceManagerApiServiceMock = getWorkspaceManagerApiServiceMock();

connectWorkspaceServiceMock = getConnectWorkspaceServiceMock();

await TestBed.configureTestingModule({
imports: [
ConnectWorkspaceComponent,
HttpClientTestingModule,
RouterTestingModule,
NoopAnimationsModule,
],
providers: [
{
provide: WorkspaceManagerApiService,
useValue: workspaceManagerApiServiceMock,
},
Copy link
Member

Choose a reason for hiding this comment

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

instead of exposing getWorkspaceManagerApiServiceMock(), expose a provideWorkspaceManagerTesting() that returns an Provider[] and includes all the mocks that are needed for testing workspace-manager (including connect-workspace)

{
provide: ConnectWorkspaceService,
useValue: connectWorkspaceServiceMock,
},
],
}).compileComponents();

fixture = TestBed.createComponent(ConnectWorkspaceComponent);
component = fixture.componentInstance;
fixture.detectChanges();
});

it('should create', () => {
fixture.detectChanges();
expect(component).toBeTruthy();
});

it('should call workspaceManagerApiService.getPathSeparator', () => {
fixture.detectChanges();
expect(workspaceManagerApiServiceMock.getPathSeparator).toHaveBeenCalled();
});

it('should initialize pathSeparator subject', () => {
fixture.detectChanges();
expect(component.pathSeparator$.getValue()).toEqual(PATH_SEPARATOR_MOCK);
});

it('should call workspaceManagerApiService.getHomeDir when no workspace path in session storage', () => {
fixture.detectChanges();
expect(workspaceManagerApiServiceMock.getHomeDir).toHaveBeenCalled();
});

it('should not call workspaceManagerApiService.getHomeDir when session storage has saved workspace path', () => {
sessionStorage.setItem(CURRENT_WORKSPACE_PATH, 'workspace/path');
fixture.detectChanges();
expect(workspaceManagerApiServiceMock.getHomeDir).not.toHaveBeenCalled();
sessionStorage.clear();
});

it('should initialize path subject with home dir path', () => {
fixture.detectChanges();
expect(component.path$.getValue()).toEqual(HOMEDIR_PATH_MOCK);
});

it('should set isAngularWorkspace subject with false when path is not a valid workspace path', () => {
fixture.detectChanges();
expect(component.isAngularWorkspace$.getValue()).toBe(false);
});

it('should set isAngularWorkspace subject with true when path is Angular workspace', () => {
fixture.detectChanges();
const angularWorkspaceDirName = DIRECTORIES_MOCK[1].name;
component.path$.next(
`${HOMEDIR_PATH_MOCK}${PATH_SEPARATOR_MOCK}user${PATH_SEPARATOR_MOCK}${angularWorkspaceDirName}`
);
expect(component.isAngularWorkspace$.getValue()).toBe(true);
});

it('should call workspaceManagerApiService.getDirectoriesInPath with path from path$ subject', () => {
fixture.detectChanges();
expect(
workspaceManagerApiServiceMock.getDirectoriesInPath
).toHaveBeenCalledWith(HOMEDIR_PATH_MOCK);
});

it('should initialize the directories subject', () => {
fixture.detectChanges();
expect(component.directories$.getValue()).toEqual(DIRECTORIES_MOCK);
});

it('should call connectService.connectWorkspace with path', () => {
fixture.detectChanges();
component.connectWorkspace();
expect(connectWorkspaceServiceMock.connectWorkspace).toHaveBeenCalledWith(
HOMEDIR_PATH_MOCK
);
});

describe('onPathChanged', () => {
beforeEach(() => {
fixture.detectChanges();
});

it('should not call path$ subject next when new path equals current path', () => {
const pathNextSpy = jest.spyOn(component.path$, 'next');
component.onPathChanged(HOMEDIR_PATH_MOCK);
expect(pathNextSpy).not.toHaveBeenCalled();
});

it('should call path$ subject next with new path when new path is different than current path', () => {
const newPathMock = '/new/path/1';
const pathNextSpy = jest.spyOn(component.path$, 'next');
component.onPathChanged(newPathMock);
expect(pathNextSpy).toHaveBeenCalledWith(newPathMock);
});
});
});
Original file line number Diff line number Diff line change
@@ -1,36 +1,109 @@
import { CommonModule } from '@angular/common';
import { ChangeDetectionStrategy, Component, inject } from '@angular/core';
import {
FormControl,
FormGroup,
ReactiveFormsModule,
Validators,
} from '@angular/forms';
ChangeDetectionStrategy,
Component,
inject,
OnDestroy,
OnInit,
} from '@angular/core';
import { MatButtonModule } from '@angular/material/button';
import { MatInputModule } from '@angular/material/input';
import {
CURRENT_WORKSPACE_PATH,
Directory,
} from '@angular-cli-gui/shared/data';
import {
BehaviorSubject,
combineLatest,
of,
Subject,
switchMap,
takeUntil,
} from 'rxjs';

import { ConnectWorkspaceService } from '../../data-access/connect-workspace.service';
import { WorkspaceManagerApiService } from '../../data-access/workspace-manager-api.service';
import { FilesystemNavigatorComponent } from '../../ui';

@Component({
selector: 'cli-connect-workspace',
standalone: true,
imports: [CommonModule, MatInputModule, MatButtonModule, ReactiveFormsModule],
imports: [CommonModule, MatButtonModule, FilesystemNavigatorComponent],
templateUrl: './connect-workspace.component.html',
styleUrls: ['./connect-workspace.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class ConnectWorkspaceComponent {
export class ConnectWorkspaceComponent implements OnInit, OnDestroy {
connectService = inject(ConnectWorkspaceService);
form = new FormGroup(
{
path: new FormControl('', [Validators.required]),
},
{ updateOn: 'submit' }
);
workspaceManagerApiService = inject(WorkspaceManagerApiService);

directories$ = new BehaviorSubject<Directory[]>([]);
path$ = new BehaviorSubject<string | null>(null);
pathSeparator$ = new BehaviorSubject<string | null>(null);
isAngularWorkspace$ = new BehaviorSubject<boolean>(false);
private destroyed$ = new Subject<boolean>();

ngOnInit(): void {
this.workspaceManagerApiService
Copy link
Member

Choose a reason for hiding this comment

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

there are too many subscribes that ends up assigning the returned value into another subject that is being subscribed in the template with async pipe. try to think how this can be avoided and instead of subjects and subscribes in ts work as much as you can with streams that are being subscribed with async pipe

.getPathSeparator()
.subscribe((separator) => {
this.pathSeparator$.next(separator);
Copy link
Contributor

Choose a reason for hiding this comment

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

pathSeparator$ can also be an observable, the same as directories$

this.initWorkspacePath();
});

combineLatest([this.path$, this.pathSeparator$])
Copy link
Contributor

Choose a reason for hiding this comment

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

directories$ could just be an observable and you can remove the takeUntil

Suggested change
combineLatest([this.path$, this.pathSeparator$])
this.directories$ = combineLatest([this.path$, this.pathSeparator$]).pipe(
switchMap(([path, separator]) => {
path && separator && this.setIsAngularWorkspace(path, separator);
return path
? this.workspaceManagerApiService.getDirectoriesInPath(path)
: of([]);
}),
)

.pipe(
switchMap(([path, separator]) => {
path && separator && this.setIsAngularWorkspace(path, separator);
return path
? this.workspaceManagerApiService.getDirectoriesInPath(path)
: of([]);
}),
takeUntil(this.destroyed$)
)
.subscribe((directories) => {
this.directories$.next(directories);
});
}

ngOnDestroy(): void {
this.destroyed$.next(true);
}

connectWorkspace(): void {
if (!this.form.valid) return;
const path = this.form.controls.path.value ?? '';
this.connectService.connectWorkspace(path).subscribe();
const path = this.path$.getValue();
if (path) {
this.connectService.connectWorkspace(path).subscribe();
}
}

onPathChanged(path: string): void {
if (path !== this.path$.getValue()) {
this.path$.next(path);
}
}

private initWorkspacePath(): void {
const currentWorkspacePath = sessionStorage.getItem(CURRENT_WORKSPACE_PATH);
const path$ = currentWorkspacePath
? of(currentWorkspacePath)
: this.workspaceManagerApiService.getHomeDir();
path$.subscribe((path: string) => this.path$.next(path));
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to handle unsubscribing?

Copy link
Contributor Author

@tomzohar tomzohar Mar 28, 2023

Choose a reason for hiding this comment

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

the of observable completes after emitting and the http observable also completes

}

private setIsAngularWorkspace(path: string, separator: string): void {
Copy link
Member

Choose a reason for hiding this comment

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

IMO it should be part of filesystem-navigator, and you should add an output to that component instead of isAngularWorkspace$ behavior subject you have here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this is what i did in the last PR and you and @OmerGronich said to keep the component dumb
I also think that this calculation should be done inside the navigator

const parentPath = this.getParentPath(path, separator);
this.workspaceManagerApiService
.getDirectoriesInPath(parentPath)
.subscribe((directories) => {
const dir = directories.find(
(d: Directory) => `${parentPath}${separator}${d.name}` === path
);
this.isAngularWorkspace$.next(dir?.isNG ?? false);
});
}

private getParentPath(path: string, separator: string): string {
const pathParts = path.split(separator);
return pathParts.slice(0, pathParts.length - 1).join(separator);
}
}
46 changes: 46 additions & 0 deletions libs/workspace-manager/src/lib/mocks/connect-workspace.mock.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { Directory } from '@angular-cli-gui/shared/data';
import { ConnectWorkspaceService } from '@angular-cli-gui/workspace-manager';
import { of } from 'rxjs';

import { WorkspaceManagerApiService } from '../data-access/workspace-manager-api.service';

export const PATH_SEPARATOR_MOCK = '/';
export const HOMEDIR_PATH_MOCK = '/home';
export const DIRECTORIES_MOCK: Directory[] = [
{
name: 'dir a',
isNG: false,
},
{
name: 'dir b',
isNG: true,
},
{
name: 'dir c',
isNG: false,
},
];

export type WorkspaceManagerApiServiceMock = Partial<
Record<keyof WorkspaceManagerApiService, jest.Mock>
>;

export const getWorkspaceManagerApiServiceMock =
(): WorkspaceManagerApiServiceMock => {
return {
getPathSeparator: jest.fn(() => of(PATH_SEPARATOR_MOCK)),
getHomeDir: jest.fn(() => of(HOMEDIR_PATH_MOCK)),
getDirectoriesInPath: jest.fn(() => of(DIRECTORIES_MOCK)),
};
};

export type ConnectWorkspaceServiceMock = Partial<
Record<keyof ConnectWorkspaceService, jest.Mock>
>;

export const getConnectWorkspaceServiceMock =
(): ConnectWorkspaceServiceMock => {
return {
connectWorkspace: jest.fn(() => of(null)),
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,7 @@
<div>{{ separator }}</div>
</div>
</div>

<div class="actions">
<ng-content select="[actions]"></ng-content>
</div>
Loading