Skip to content

Save scripts to custom locations (use import/export buttons)#4258

Open
sergeyteleshev wants to merge 29 commits intodevelfrom
7880-save-scripts-to-custom-locations-use-importexport-buttons
Open

Save scripts to custom locations (use import/export buttons)#4258
sergeyteleshev wants to merge 29 commits intodevelfrom
7880-save-scripts-to-custom-locations-use-importexport-buttons

Conversation

@sergeyteleshev
Copy link
Copy Markdown
Contributor

@sergeyteleshev sergeyteleshev requested a review from devnaumov April 2, 2026 13:21
@sergeyteleshev sergeyteleshev self-assigned this Apr 2, 2026
@codacy-production
Copy link
Copy Markdown

codacy-production bot commented Apr 2, 2026

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

@sergeyteleshev sergeyteleshev marked this pull request as ready for review April 3, 2026 12:29
@@ -0,0 +1,17 @@
/*
* CloudBeaver - Cloud Database Manager
* Copyright (C) 2020-2025 DBeaver Corp and others
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2026 here and below

* you may not use this file except in compliance with the License.
*/

.tabList {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use tailwind as amount of styles and its logic is small

const [focusedRef] = useFocus<HTMLFormElement>({ focusFirstChild: true });
const notificationService = useService(NotificationService);

const state = useObservableRef<State>(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Its better not to use useObservableRef for such simple cases. You can use native react state so we can reuse this logic across code bases and in desktop env.

const name = getSqlEditorName(state, dataSource, connection);
const script = dataSource.script;
const hasOnlyLocalExport =
this.scriptExportService.tabsContainer.has(LOCAL_EXPORT_TAB_ID) && this.scriptExportService.tabsContainer.getIdList().length === 1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

getDisplayed instead of getIdList, as tabs might be hidden

const [footerNode, setFooterNode] = useState<HTMLDivElement | null>(null);
const footerRef = useCallback((node: HTMLDivElement | null) => setFooterNode(node), []);

const FooterSlot: React.FC<React.PropsWithChildren> = useCallback(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

footerNode and FooterSlot seems very strange, could you please explain what are we trying to achive with this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The dialog uses a tab-based architecture where:

  • Different export destinations are shown as tabs (Local Download - CE, Cloud Storage - EE, etc.)
  • Each tab has different validation requirements and submit logic
    • Local: just needs a filename
    • Cloud Storage: needs filename + folder selection + async upload

The issue:

  • The dialog can't know each tab's specific validation rules
  • The dialog can't know each tab's specific submit logic
  • But the dialog needs to render the buttons

The initial solution was the portal pattern. I just moved the whole logic inside tabs for simplicity and then teleported the buttons to their original position, so the layout is not broken and looks fine (CommonDialog demands quite a strict order of the elements - body & footer)

I pushed my second proposal for how to handle this issue using MobX state, React contexts & useEffect. But as for me, it looks fragile to have all handled by useEffect. It can cause an unexpected state during saving & can lead to a memory leak and an app freeze. This is why I first came up with the portal pattern. It seems more stable, but tricky, I agree

The source issue is more general, and we need to design a confident API for these case scenarios. It's just currently out of the scope of this task, so I propose 1 of these 2 solutions for the current task

{...payload}
>
<CommonDialogBody noBodyPadding noOverflow>
<TabList className={s(style, { tabList: true })} underline />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we use tailwind here instead?

I thought useS and s are kinda deprecated. Am I wrong?

if (!footerNode) {
return null;
}
return createPortal(children, footerNode);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see why you need this here, but it looks a bit weird:

  1. Footer buttons are actually the same, but we duplicate them in different tabs
  2. Only onSubmit is different
  3. It is not a Tab responsibility to render buttons of a dialog

maybe we could pass actions in IScriptExportDialogContext?

{
  canSubmit: boolean;
  onSubmit: () => void;
  loading?: boolean;
}

So the tab could do
action.onSubmit = () => {downloadSql(fileName, script)}

I see that you have tried to use export controller, but it was using react state. Can you try with mobx observable via useObservableRef?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sergeyteleshev sergeyteleshev requested a review from Wroud April 9, 2026 12:51
devnaumov
devnaumov previously approved these changes Apr 10, 2026
className,
}) {
const scriptExportService = useService(ScriptExportService);
const [selectedTabId, setSelectedTabId] = useState<string | undefined>(undefined);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like you are using this state just to pass to TabsState, you can omit it and keep only container

Wroud
Wroud previously approved these changes Apr 10, 2026
const [fileName, setFileName] = useState(initialFileName);

const form = useForm({
onSubmit(event) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please fix lint

Comment on lines +39 to +43
<Form ref={focusedRef} context={form}>
<InputField name="fileName" value={fileName} required small onChange={setFileName}>
<Translate token="ui_file_name" />
</InputField>
</Form>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think that use of <Form> component is optional here

@sergeyteleshev sergeyteleshev dismissed stale reviews from Wroud and devnaumov via 244cc0e April 10, 2026 16:38
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.

4 participants