Skip to content

feat(editor): Allow front-end modules to register modals (no-changelog) #17885

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

Merged
merged 7 commits into from
Aug 4, 2025

Conversation

MiloradFilipovic
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic commented Aug 1, 2025

Summary

  1. Adding support for custom modals in module descriptior + modal registry and dynamic modal component that handles them:
export type ModalDefinition = {
	key: string;
	component: Component | (() => Promise<Component>);
	initialState?: ModalState;
};

export type ResourceMetadata = {
	key: string;
	displayName: string;
	i18nKeys?: Record<string, string>;
};

export type FrontendModuleDescription = {
	id: string;
	name: string;
	description: string;
	icon: string;
	routes?: RouteRecordRaw[];
	projectTabs?: {
		overview?: DynamicTabOptions[];
		project?: DynamicTabOptions[];
		shared?: DynamicTabOptions[];
	};
	resources?: ResourceMetadata[];
	modals?: ModalDefinition[];
};
  1. Implemented Add Data Store modal as an example
Screen.Recording.2025-08-01.at.09.48.30.mov

How to test

  1. Make sure to add data-store to activeModules here
  2. Go to any project (not overview) -> Data Stores tab -> Click the callout button

Related Linear tickets, Github issues, and Community forum posts

Closes ADO-3843

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

cubic analysis

4 issues found across 12 files • Review in cubic

React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Copy link

bundlemon bot commented Aug 1, 2025

BundleMon

Files added (2)
Status Path Size Limits
WASM Dependencies
tree-sitter-bash.wasm
+181.26KB -
WASM Dependencies
tree-sitter.wasm
+74.47KB -

Total files change +255.73KB

Groups added (2)
Status Path Size Limits
**/*.js
+5.34MB -
**/*.css
+188.37KB -

Final result: ✅

View report in BundleMon website ➡️


Current branch size history

@n8n-assistant n8n-assistant bot added the n8n team Authored by the n8n team label Aug 1, 2025
@MiloradFilipovic MiloradFilipovic self-assigned this Aug 1, 2025
@MiloradFilipovic
Copy link
Contributor Author

@claude can you review this?

Copy link

claude bot commented Aug 1, 2025

Claude finished @MiloradFilipovic's task —— View job


PR Review: Dynamic Modal Registration ✅

Review Checklist:

  • Analyze modal registry system architecture
  • Review DynamicModalLoader implementation
  • Check data store modal integration
  • Review test coverage and quality
  • Assess type definitions and API design
  • Check for security and performance concerns
  • Address cubic-dev-ai review comments

Overall Assessment: Looks Great! 🎉

This is a well-architected implementation that follows Vue.js and n8n conventions. The modal registration system is clean, type-safe, and extensible.

Strengths

  1. Clean Architecture: The modal registry pattern is well-designed with proper separation of concerns
  2. Type Safety: Excellent TypeScript definitions with proper union types for sync/async components
  3. Memory Management: Proper subscription cleanup in both DynamicModalLoader.vue:50-52 and UI store
  4. Test Coverage: Comprehensive test suite covering edge cases and error scenarios
  5. Vue Integration: Proper use of defineAsyncComponent for lazy loading
  6. Extensibility: Module system allows easy addition of new modals

🔧 Issues to Address (aligning with cubic-dev-ai feedback)

1. Type Guard Implementation (DynamicModalLoader.vue:15-20)

Good: You added a proper type guard instead of using as casting - this addresses cubic-dev-ai's concern about preferring type guards over type casting.

2. AddDataStoreModal Issues (components/AddDataStoreModal.vue)

  • Line 62: ❌ data-test-id="data-store-name-input" - This should be "add-data-store-name-input" to be more descriptive
  • Missing functionality: ✅ I see you acknowledged this will be added in following commits

3. Memory Leak Prevention

Fixed: The unsubscribe function from modalRegistry.subscribe is properly captured and called in DynamicModalLoader.vue:41,45,51

🚀 Performance Considerations

  • Lazy Loading: ✅ Async component support enables code splitting
  • Registration Timing: ✅ Modals are registered during app initialization, not on every render
  • Event Handling: ✅ Proper cleanup prevents memory leaks

🔒 Security

  • No XSS risks: Template usage is safe with proper Vue bindings
  • Component isolation: Each modal is properly sandboxed
  • Input validation: Basic validation present in form handling

📝 Minor Suggestions

  1. AddDataStoreModal.vue:62: Update test ID to be more specific:

    - data-test-id="data-store-name-input"
    + data-test-id="add-data-store-name-input"
  2. Consider adding JSDoc to the modal registry functions for better developer experience

  3. UI Store: The memory management is correctly implemented - the unsubscribe function is properly captured and will be called via the cleanup() method

🎯 Architecture Highlights

  1. Registry Pattern: Clean, extensible design for modal management
  2. Module System Integration: Seamless integration with existing module architecture
  3. Reactive Updates: Proper Vue reactivity with subscription model
  4. Type Safety: Excellent TypeScript support throughout

This PR demonstrates excellent software engineering practices and will be a solid foundation for dynamic modal functionality. The issues raised by cubic-dev-ai have been properly addressed or acknowledged for future commits.


Copy link
Member

@alexgrozav alexgrozav left a comment

Choose a reason for hiding this comment

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

Awesome work! While this is likely to change when we extract modules, it's a great starting point for managing our modals.

Copy link

currents-bot bot commented Aug 4, 2025

E2E Tests: n8n tests passed after 4m 34.4s

🟢 500 · 🔴 0 · ⚪️ 0

View Run Details

Run Details

  • Project: n8n

  • Groups: 1

  • Framework: Currents

  • Run Status: Passed

  • Commit: 324fb03

  • Spec files: 105

  • Overall tests: 500

  • Duration: 4m 34.4s

  • Parallelization: 1


This message was posted automatically by currents.dev | Integration Settings

@MiloradFilipovic MiloradFilipovic merged commit a4f7510 into master Aug 4, 2025
35 checks passed
@MiloradFilipovic MiloradFilipovic deleted the ADO-3843-fe-modules-dynamic-modals branch August 4, 2025 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants