[ENG-10944] Files general improvement#983
Conversation
| const messageType = action === 'move' ? 'moveFile' : 'copyFile'; | ||
| this.toastService.showError(errorMessage ?? `files.dialogs.${messageType}.error`); | ||
| moveFiles(): void { | ||
| this.isFilesUpdating.set(true); |
There was a problem hiding this comment.
Just noticed that isFilesUpdating is set to true when moveFiles() kicks off, but I don't see it being reset back to false anywhere in the pipe. The finalize at the end only resets the dialog header. So if the request errors out or gets cancelled mid-flight (e.g. component destroyed), I believe the button stays stuck in a loading/disabled state forever. Could we add finalize(() => this.isFilesUpdating.set(false)) to the pipe to cover that ?
| readonly headerTitle = computed(() => { | ||
| const fileName = this.file()?.name ?? ''; | ||
| if (!this.fileVersion) return fileName; | ||
| return `${fileName} (${this.translateService.instant('project.wiki.version.title')} ${this.fileVersion})`; | ||
| }); |
There was a problem hiding this comment.
Small heads up on this one, headerTitle is a computed() signal, but it's reading this.fileVersion which is a plain class property, not a signal. While angular's reactive graph only tracks signal reads inside computed, so when onOpenRevision() updates fileVersion, headerTitle won't recompute and the header will stay stale showing the original version. Would it make sense to convert fileVersion to a signal('') and use .set() in onOpenRevision?
| @@ -131,9 +115,6 @@ export class FileSelectDestinationComponent implements OnInit { | |||
| const rootFolder = this.storageAddons().find((option) => option.folder.id === value); | |||
| if (rootFolder) { | |||
| this.currentRootFolder.set(rootFolder); | |||
| if (rootFolder.folder.provider) { | |||
| this.actions.setCurrentProvider(rootFolder.folder.provider); | |||
There was a problem hiding this comment.
Looks like the SetCurrentProvider dispatch got removed from onStorageChange(). Any code downstream that reads the current provider from the store will now get a stale value from whatever provider was selected before. Was this intentional? If so, happy to understand the reasoning, otherwise I think we need to restore that dispatch when the storage selection changes.
| (entryFileClicked)="selectProjectFile($event)" | ||
| (setCurrentFolder)="setCurrentFolder($event)" | ||
| (fileOpened)="selectProjectFile($event)" | ||
| (currentFolderChanged)="setCurrentFolder($event)" |
There was a problem hiding this comment.
I think there is an issue, you changed naming (currentFolderChanged) which is bound on osf-files-tree but those output names don't exist on the component. In this case, file open and folder navigation will be silently broken in the preprints file. Please verify this here and in other files as well I can see similar behavior in other files as well which might led to breaking up functionality
| readonly storage = input.required<FileLabelModel | null>(); | ||
|
|
||
| readonly totalCount = input<number>(0); | ||
| readonly isLoading = input<boolean>(); |
There was a problem hiding this comment.
isLoading is declared as input<boolean>() with no default, so it initializes as undefined. The effect below treats undefined as falsy and immediately flips isLoadingMore to false on first render even if a load is actually in progress at that point. Setting a default of false and tightening the guard to if (this.isLoading() === false) would prevent that false-negative reset
| this.filesService.getFileGuid(file.id).subscribe((file) => { | ||
| if (file.guid) { | ||
| this.openFile(file.guid); | ||
| } |
There was a problem hiding this comment.
The getFileGuid() subscription here doesn't have any cleanup attached. If the user navigates away while that HTTP request is still in flight, the subscription stays alive and will try to call router.navigate() after the component is destroyed. I think adding .pipe(takeUntilDestroyed(this.destroyRef)) before the .subscribe() should cover it.
2 participants
Summary of Changes