-
Notifications
You must be signed in to change notification settings - Fork 235
fix: Accessing admin customize error #10526
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
fix: Accessing admin customize error #10526
Conversation
| const customizePropsFragment = { | ||
| props: { | ||
| isDefaultBrandLogoUsed: | ||
| await crowi.attachmentService.isDefaultBrandLogoUsed(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attachmentService は isDefaultBrandLogoUsed() というメソッドを提供していない
| isCustomizedLogoUploaded: | ||
| await crowi.attachmentService.isBrandLogoExist(), | ||
| customTitleTemplate: crowi.configManager.getConfig('customize:title'), | ||
| customTitleTemplate: crowi.configManager.getConfig('customize:title') ?? '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- customTitleTemplate に null が入ったことがバグの原因
- config 的には
customize:titleは undefined 許容している
参考
growi/apps/app/src/pages/utils/page-title-customization.ts
Lines 10 to 17 in aeb57c3
| export const useCustomTitle = (title: string): string => { | |
| const appTitle = useAppTitle(); | |
| const customTitleTemplate = useCustomTitleTemplate(); | |
| return customTitleTemplate | |
| .replace('{{sitename}}', appTitle) | |
| .replace('{{pagepath}}', title) | |
| .replace('{{pagename}}', title); |
エラー内容
Server Error
TypeError: Cannot read properties of null (reading 'replace')
This error happened while generating the page. Any console logs will be displayed in the terminal window.
Source
src/pages/utils/page-title-customization.ts (15:6) @ replace
13 |
14 | return customTitleTemplate
> 15 | .replace('{{sitename}}', appTitle)
| ^
16 | .replace('{{pagepath}}', title)
17 | .replace('{{pagename}}', title);
18 | };
Call Stack| await crowi.attachmentService.isBrandLogoExist(), | ||
| customTitleTemplate: crowi.configManager.getConfig('customize:title'), | ||
| customTitleTemplate: | ||
| crowi.configManager.getConfig('customize:title') ?? '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
空文字列への初期化は global.ts でやっている
const customTitleTemplateAtom = atom<string>('');
なので、ここで入れるべきは ?? undefined なんじゃない?
どちらにせよ DB 内の null 値に対応するため、というコメントは追加しておくべきだと思う
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config の初期値である undefined で global.ts で初期化された値を useHydrateAtoms (dangerouslyForceHydrate) で上書きされることがバグの原因でした。
growi/apps/app/src/pages/admin/customize.page.tsx
Lines 31 to 43 in 0075928
| const AdminCustomizeSettingsPage: NextPageWithLayout<Props> = ( | |
| props: Props, | |
| ) => { | |
| useHydrateAtoms( | |
| [ | |
| [atoms.customTitleTemplateAtom, props.customTitleTemplate], | |
| [isCustomizedLogoUploadedAtom, props.isCustomizedLogoUploaded], | |
| ], | |
| { dangerouslyForceHydrate: true }, | |
| ); | |
| return <CustomizeSettingContents />; | |
| }; |
customTitleTemplate は commonResult から渡ってくるのでここで直接 config 値を取得する必要はないと思います。
| return mergeGetServerSidePropsResults(commonResult, customizePropsFragment); |
commonResult は _app.page.txt から渡ってくるものが含まれていると思います。
growi/apps/app/src/pages/_app.page.tsx
Lines 66 to 69 in 0075928
| // Hydrate global atoms with server-side data | |
| useHydrateGlobalInitialAtoms( | |
| isCommonInitialProps(pageProps) ? pageProps : undefined, | |
| ); |
実際に渡ってくる値は CustomizeService で初期化された値です。
| customTitleTemplate: customizeService.customTitleTemplate, |
growi/apps/app/src/server/service/customize.ts
Lines 110 to 120 in 0075928
| initCustomTitle() { | |
| let configValue = configManager.getConfig('customize:title'); | |
| if (configValue == null || configValue.trim().length === 0) { | |
| configValue = '{{pagename}} - {{sitename}}'; | |
| } | |
| this.customTitleTemplate = configValue; | |
| this.lastLoadedAt = new Date(); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useHydrateGlobalInitialAtoms では hydrate なので初期化されたらその後更新されることはないはず
そのため、カスタマイズ設定画面を使う上で、以下のフローでバグるんじゃないかと思っている (要検証)
再現条件
- カスタマイズ画面以外にアクセス
- customTitleTemplateAtom 初期化
- カスタマイズ画面に遷移 (next routing)
- custom title template を変更
- カスタマイズ画面以外にアクセス (next routing)
- カスタマイズ画面に遷移 (next routing)
症状
5 で、変更前の custom title template が表示されてしまう
期待される動作
5 で、変更後の custom title template が表示されてしまう
改修方針
必ずしも dangerouslyForceHydrate: true で hydrate する必要はないのかもしれない
他のカスタマイズ項目はどうなっているかというと、たとえば CustomizeThemeSetting.tsx では useSWRxGrowiThemeSetting でデータを取得しているし、CustomizeSiedebarSetting.tsx でも同様に useSWRxSidebarConfig を使っている
つまり swr 化が最も適切な解だが、改修がやり方で構わない
Note
因みに globalLang 同様、カスタムタイトル変更後にリロードしないと新しい値が反映されないという問題には目を瞑っていいと思っている
| await crowi.attachmentService.isDefaultBrandLogoUsed(), | ||
| isCustomizedLogoUploaded: | ||
| await crowi.attachmentService.isBrandLogoExist(), | ||
| customTitleTemplate: crowi.configManager.getConfig('customize:title'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
customTitleTemplate は getServerSideAdminCommonProps から渡ってくる (non nullanle) ためここで getConfig する必要はない
Task
https://redmine.weseek.co.jp/issues/174481