Skip to content

Conversation

@yacchin1205
Copy link

@yacchin1205 yacchin1205 commented Jan 5, 2026

This Draft Pull Request is for reference purposes. The details will be discussed in the COS-NII meeting.

  • Ticket: []
  • Feature flag: n/a

Purpose

Add a plugin architecture for file browser actions (context menu and toolbar). Extensions are included in the codebase but can be enabled/disabled at build time via extensions.config.ts.

Summary of Changes

  • Add FileActionExtension interface for menu/toolbar/both targets
  • Add ExtensionRegistry service for dynamic extension loading
  • Add extensions.config.ts for build-time configuration
  • Add "Copy Link" as example extension (in Share submenu)
  • Add "OnlyOffice" as example extension (dummy implementation)

See docs/file-extensions.md for usage.

Screenshot(s)

Example: Copy Link

extensions.config.ts:

export const extensionConfig: ExtensionConfig[] = [
  {
    load: () => import('./extensions/copy-links'),
    factory: 'copyLinksExtensionFactory',
    enabled: true,
  },
];

Copy Link (Menu Item):
screenshot-copy-link

Example: OnlyOffice Integration (dummy)

extensions.config.ts:

export const extensionConfig: ExtensionConfig[] = [
  // ... other extensions ...

  // Edit by OnlyOffice
  {
    load: () => import('./extensions/onlyoffice'),
    factory: 'editByOnlyOfficeExtensionFactory',
    enabled: true,
    config: { editorUrl: 'https://...' },
  },
  // Create File
  {
    load: () => import('./extensions/onlyoffice'),
    factory: 'createFileExtensionFactory',
    enabled: true,
    config: { editorUrl: 'https://...' },
  },
];

Edit by OnlyOffice(Menu Item/Toolbar Item in the file detail):

screenshot-edit-by-onlyoffice screenshot-edit-with-onlyoffice-in-detail

Create File (Toolbar Item in the file list):

screenshot-create-file

Side Effects

  • New prebuild, prestart, pretest npm scripts that generate extensions.config.ts
  • src/app/extensions.config.ts is now git-ignored (generated from extensions.config.default.ts)

QA Notes

  • Test file context menu on file list and file detail pages
  • Verify "Copy Link" appears in Share submenu for files
  • Verify extensions can be enabled/disabled via extensions.config.ts

- Add FileActionExtension for menu/toolbar/both targets
- Add ExtensionRegistry service for dynamic extension loading
- Add extensions.config.ts for configuration
- Add Copy Link as example extension
- Add OnlyOffice as example extension (dummy implementation)

See docs/file-extensions.md for usage.
@yacchin1205 yacchin1205 force-pushed the feature/file-menu-extension-upstream branch from ceb269e to 9f95349 Compare January 7, 2026 22:55
Copy link
Collaborator

@nsemets nsemets left a comment

Choose a reason for hiding this comment

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

Overall, the extension system appears well-designed. I reviewed it as I would a standard PR to ensure everything is in order and to identify any potential issues. I have added some questions and suggestions for consideration. Additionally, I am wondering about translations: are they required in this case?

Comment on lines +17 to +27
async function loadExtension(extConfig: ExtensionConfig, registry: ExtensionRegistry): Promise<void> {
const module = await extConfig.load();
const factory = (module as Record<string, FactoryFunction>)[extConfig.factory];

if (typeof factory !== 'function') {
throw new Error(`Extension factory "${extConfig.factory}" not found in module`);
}

const extensions = factory(extConfig.config);
registry.register(extensions);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s better to add error handling here to be safe.

Comment on lines +29 to +36
export const EXTENSION_INITIALIZATION_PROVIDER: Provider = {
provide: APP_INITIALIZER,
useFactory: (registry: ExtensionRegistry) => {
return () => loadExtensions(registry);
},
deps: [ExtensionRegistry],
multi: true,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

APP_INITIALIZER is deprecated in favor of theprovideAppInitializer function. Use export const EXTENSION_INITIALIZATION_PROVIDER = provideExtensionInitialization();.

Comment on lines +11 to +14
for (const ext of extensionConfig) {
if (!ext.enabled) continue;
await loadExtension(ext, registry);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to use parallel loading with Promise.all or Promise.allSettled.


private actionContext = computed((): FileActionContext => {
const file = this.file();
if (!file) throw new Error('file is required for actionContext');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to add a translation key here.

Comment on lines +17 to +18
[class.p-disabled]="item.disabled"
[attr.aria-disabled]="item.disabled ?? false"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need it? PrimeNG's TieredMenu should handle disabled items automatically.


protected actionContext = computed((): FileActionContext => {
const file = this.file();
if (!file) throw new Error('file is required for actionContext');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need translation key.

() => this.isButtonDisabled() || (this.googleFilePickerComponent()?.isGFPDisabled() ?? false)
);

protected actionContext = computed((): FileActionContext => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
protected actionContext = computed((): FileActionContext => {
actionContext = computed((): FileActionContext => {


protected actionContext = computed((): FileActionContext => {
const folder = this.currentFolder();
if (!folder) throw new Error('folder is required for actionContext');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add translation key.

Comment on lines +275 to +278
const extensions = this.extensionRegistry
.extensions()
.filter((ext) => !ext.parentId)
.filter((ext) => !ext.visible || ext.visible(ctx));
Copy link
Collaborator

@nsemets nsemets Jan 12, 2026

Choose a reason for hiding this comment

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

Can we use only 1 filter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need it as separate file? Also, need improvements.

return [
{
id: 'copy-link',
label: 'Copy Link',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a translatable string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants