Skip to content

fix: Cannot update existing table data with handson table modal #10551

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.

Sign up for GitHub

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
yuki-takei merged 6 commits into from
Dec 3, 2025
Merged

fix: Cannot update existing table data with handson table modal #10551

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05a3a5e
Remove unnecessary exports
miya Nov 27, 2025
4963fb0
fix: prevent rendering of HandsontableModal when not opened
miya Nov 28, 2025
d314435
fix: improve markdown table state handling in HandsontableModal
miya Nov 28, 2025
05bc1d5
clean code
miya Nov 28, 2025
737d0f6
refactor HandsontableModal
miya Dec 1, 2025
e3137fc
table_chart -> table
miya Dec 1, 2025
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
40 changes: 29 additions & 11 deletions apps/app/src/client/components/PageEditor/HandsontableModal/HandsontableModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,24 @@ const MARKDOWNTABLE_TO_HANDSONTABLE_ALIGNMENT_SYMBOL_MAPPING = {
'': '',
};

export const HandsontableModalSubstance = (): JSX.Element => {

const HandsontableModalSubstance = (): JSX.Element => {
Copy link
Member Author

@miya miya Nov 28, 2025

Choose a reason for hiding this comment

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

不要な export の削除

const { t } = useTranslation('commons');
const handsontableModalData = useHandsontableModalStatus();
const { close: closeHandsontableModal } = useHandsontableModalActions();
const handsontableModalForEditorData = useHandsontableModalForEditorStatus();
const { close: closeHandsontableModalForEditor } = useHandsontableModalForEditorActions();

const isOpened = handsontableModalData?.isOpened ?? false;
const isOpendInEditor = handsontableModalForEditorData?.isOpened ?? false;
// for View
const { close: closeHandsontableModal } = useHandsontableModalActions();
const handsontableModalData = useHandsontableModalStatus();
const isOpened = handsontableModalData.isOpened;
const table = handsontableModalData?.table;
const autoFormatMarkdownTable = handsontableModalData?.autoFormatMarkdownTable ?? false;
const editor = handsontableModalForEditorData?.editor;
const onSave = handsontableModalData?.onSave;

// for Editor
const { close: closeHandsontableModalForEditor } = useHandsontableModalForEditorActions();
const handsontableModalForEditorData = useHandsontableModalForEditorStatus();
const isOpendInEditor = handsontableModalForEditorData.isOpened;
const editor = handsontableModalForEditorData?.editor;
Copy link
Member Author

@miya miya Nov 28, 2025

Choose a reason for hiding this comment

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

  • どの hook 由来の変数か追いにくかったため view で使うものと editor で使うものを分けた
  • .isOpened については jotai 化したタイミング nullable ではなくなったためオプショナルチェーンを外した



// Memoize default table creation
const defaultMarkdownTable = useMemo(() => {
return new MarkdownTable(
Expand Down Expand Up @@ -114,12 +117,18 @@ export const HandsontableModalSubstance = (): JSX.Element => {

// Memoize modal open handler
const handleModalOpen = useCallback(() => {
const markdownTableState = table == null && editor != null ? getMarkdownTable(editor) : table;
Copy link
Member Author

@miya miya Nov 28, 2025

Choose a reason for hiding this comment

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

バグの原因

エディターからモーダルを開く際に、関係のない "table" という変数を見ていたから。"table" は2回目にモーダルを開く時に必ず not null になる。

変数 table について

"table" は useHandsontableModalStatus 由来のもの

growi/apps/app/src/client/components/PageEditor/HandsontableModal/HandsontableModal.tsx

Lines 38 to 40 in 05bc1d5

const handsontableModalData = useHandsontableModalStatus();
const isOpened = handsontableModalData.isOpened;
const table = handsontableModalData?.table;

modal を閉じる際に呼ばれる close メソッドで closeHandsontableModal() が呼ばれる

growi/apps/app/src/client/components/PageEditor/HandsontableModal/HandsontableModal.tsx

Lines 166 to 171 in 05bc1d5

const cancel = () => {
closeHandsontableModal();
closeHandsontableModalForEditor();
setIsDataImportAreaExpanded(false);
setIsWindowExpanded(false);
};

closeHandsontableModal() によって useHandsontableModalStatus の table が defaultMarkdownTable になる

growi/apps/app/src/states/ui/modal/handsontable.ts

Lines 16 to 67 in 05bc1d5

const defaultMarkdownTable = () => {
return new MarkdownTable(
[
['col1', 'col2', 'col3'],
['', '', ''],
['', '', ''],
],
{
align: ['', '', ''],
},
);
};
// Atom definition
const handsontableModalAtom = atom<HandsontableModalState>({
isOpened: false,
});
// Read-only hook
export const useHandsontableModalStatus = () => {
const [state] = useAtom(handsontableModalAtom);
return state;
};
// Actions-only hook
export const useHandsontableModalActions = () => {
const setState = useSetAtom(handsontableModalAtom);
const open = (
table: MarkdownTable,
autoFormatMarkdownTable?: boolean,
onSave?: HandsontableModalSaveHandler,
) => {
setState({
isOpened: true,
table,
autoFormatMarkdownTable,
onSave,
});
};
const close = () => {
setState({
isOpened: false,
table: defaultMarkdownTable(),
autoFormatMarkdownTable: false,
onSave: undefined,
});
};
return { open, close };
};

let markdownTableState: MarkdownTable | undefined;
if (isOpendInEditor) {
markdownTableState = editor != null ? getMarkdownTable(editor) : undefined;
}
else {
markdownTableState = table;
}
Copy link
Member Author

@miya miya Nov 28, 2025

Choose a reason for hiding this comment

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

View で開く場合と Editor で開く場合で処理を分けた

Copy link
Contributor

@yuki-takei yuki-takei Nov 28, 2025

Choose a reason for hiding this comment

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

isOpenedInEditor じゃないかな (e が足りない)

Copy link
Member Author

@miya miya Dec 1, 2025

Choose a reason for hiding this comment

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

修正しました

const initTableInstance = markdownTableState == null ? defaultMarkdownTable : markdownTableState.clone();
setMarkdownTable(markdownTableState ?? defaultMarkdownTable);
setMarkdownTableOnInit(initTableInstance);
debouncedHandleWindowExpandedChange();
}, [table, editor, defaultMarkdownTable, debouncedHandleWindowExpandedChange]);
}, [isOpendInEditor, defaultMarkdownTable, debouncedHandleWindowExpandedChange, editor, table]);

// Memoize expand/contract handlers
const expandWindow = useCallback(() => {
Expand Down Expand Up @@ -536,5 +545,14 @@ export const HandsontableModalSubstance = (): JSX.Element => {
};

export const HandsontableModal = (): JSX.Element => {
const handsontableModalData = useHandsontableModalStatus();
const handsontableModalForEditorData = useHandsontableModalForEditorStatus();

const isOpened = (handsontableModalData.isOpened || handsontableModalForEditorData.isOpened);

if (!isOpened) {
return <></>;
}
Copy link
Member Author

@miya miya Nov 28, 2025

Choose a reason for hiding this comment

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

dynamic import 後の HandsontableModalSubstance の不要な rerender 対策

Copy link
Contributor

@yuki-takei yuki-takei Nov 28, 2025

Choose a reason for hiding this comment

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

今のままだとモーダルの fade エフェクトがなくなったりするという微妙なデメリットがある

ちゃんとやるなら他のモーダル (ShortcutsModal など) を参考に、一番外側の <Modal> は Container component (lightweight, always rendered) に含むように改修することになる

HandsontableModalSubstance は view / editor の場合分けなしに両対応するとして、
それをラップする HandsontableModal 側で場合分けすべきかなと思う

ちなみに DrawioModal も同じかもしれない

Copy link
Member Author

@miya miya Dec 1, 2025

Choose a reason for hiding this comment

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

ShortcutsModal を参考に修正しました。
View / Editor の場合分けは HandsontableModal で行い HandsontableModalSubstance では意識しなくて良いようにしました


return <HandsontableModalSubstance />;
};
Loading