Skip to content
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

feat(frontend): ノート・ユーザータイムライン埋め込み #13929

Open
wants to merge 111 commits into
base: develop
Choose a base branch
from

Conversation

kakkokari-gtyih
Copy link
Contributor

@kakkokari-gtyih kakkokari-gtyih commented Jun 1, 2024

What

  • embed時の起動ロジックの分離
    • アカウントの読み込み回避
    • localStorage, indexedDBをセッションごとに初期化 (safeSessionStorage)
      • 必要なデータは本物のlocalStorageからコピーするように
    • os.popupを回避
      • 代替アクションの実装
    • routerが遷移してしまうのを回避
  • postMessageの改良
    • ウィンドウリサイズ機構の実装
    • iframeIdを保持する機構の実装
    • iframeIdを一度設定したら変更できないように
    • parent iframeにメッセージが行かない問題を修正
  • 各種埋め込みコンポーネントの作成
    • 個別ノート
    • ユーザーTL
    • クリップのノート一覧
    • ハッシュタグのノート一覧
  • 埋め込みコード生成機構の作成
  • WebSocketを使わないようにする(Related to No WebSocketモード #12227

Why

Fix #1714
Close #10491

Additional info (optional)

カスタマイズ用 パラメータ

共通

  • colorMode: light | dark | auto。カラーモードのオーバーライド。デフォルト: auto
  • rounded: true | false。角丸にするかどうか。デフォルト: true
  • border: true | false。外周のボーダーをつけるかどうか。デフォルト: true

TL系

  • header: true | false。上部にヘッダを設けるかどうか。デフォルト: true
  • autoload: true | false。無限スクロールするかどうか。デフォルト: false
  • maxHeight: 数値。設定すると、指定した高さ以降はスクロールバーが出る。指定しない場合はどこまでも伸びる(デフォルトで何らかの値を設定した状態のコードを生成する)

埋め込みコード

たぶんミニファイして2行になると思う

<iframe src="http:https://localhost:5173/embed/notes/9qhdxot24f?" data-misskey-embed-id="v1_8482df8f-5f43-49f4-9c18-f2c6b013f9ba" style="border: none; width: 100%; max-width: 500px; height: 300px; color-scheme: light dark;"></iframe>
<script defer src="http:https://localhost:5173/embed.js"></script>

その他

/packages/backend/assets/embed.js (親ページ側から読み込むiframe制御用スクリプト)はいろいろ使いまわしやすいようにSPDXをMITライセンスとしている

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added packages/frontend Client side specific issue/PR packages/backend Server side specific issue/PR labels Jun 1, 2024
Copy link
Contributor

github-actions bot commented Jun 1, 2024

このPRによるapi.jsonの差分

差分はこちら

Get diff files from Workflow Page

Copy link

codecov bot commented Jun 1, 2024

Codecov Report

Attention: Patch coverage is 13.13929% with 2089 lines in your changes missing coverage. Please review.

Project coverage is 39.71%. Comparing base (6793185) to head (f826c5d).
Report is 1 commits behind head on develop.

Files Patch % Lines
packages/frontend/src/components/MkNoteEmbed.vue 0.00% 494 Missing and 1 partial ⚠️
...s/frontend/src/components/MkEmbedCodeGenDialog.vue 0.00% 419 Missing and 1 partial ⚠️
...ackages/frontend/src/pages/embed/user-timeline.vue 0.00% 160 Missing and 1 partial ⚠️
packages/frontend/src/pages/embed/clip.vue 0.00% 153 Missing and 1 partial ⚠️
packages/frontend/src/pages/embed/tag.vue 0.00% 140 Missing and 1 partial ⚠️
packages/frontend/src/ui/embed.vue 0.00% 106 Missing and 1 partial ⚠️
packages/frontend/src/_embed_boot_.ts 0.00% 75 Missing and 1 partial ⚠️
packages/frontend/src/scripts/stream-mock.ts 20.98% 64 Missing ⚠️
packages/frontend/src/pages/embed/note.vue 0.00% 61 Missing and 1 partial ⚠️
packages/frontend/src/scripts/get-embed-code.ts 35.63% 56 Missing ⚠️
... and 29 more
Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #13929       +/-   ##
============================================
+ Coverage    20.25%   39.71%   +19.46%     
============================================
  Files          697     1533      +836     
  Lines        97737   195770    +98033     
  Branches      1014     2570     +1556     
============================================
+ Hits         19796    77754    +57958     
- Misses       77421   117435    +40014     
- Partials       520      581       +61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kakkokari-gtyih kakkokari-gtyih changed the title feat(frontend): ノート・ユーザータイムライン・ローカルタイムライン埋め込み feat(frontend): ノート・ユーザータイムライン埋め込み Jun 1, 2024
@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

/preview

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

特定の操作でログインダイアログ出せるわね
image

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

考えられる対応

  • embedではページ遷移禁止
  • embedではログインAPI叩くの禁止

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

ページ遷移は必ず新しいタブ の方が良いわね

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

特定の操作でログインダイアログ出せるわね image

  • 時間クリックして投稿詳細に遷移する手法
  • アバタークリックしてユーザーページに遷移する手法
  • 名前クリックしてユーザーページに遷移する手法

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

あと何らかの原因で「読み込みに失敗しました」ページが表示されてしまうとそこからいろいろアクセスできてしまいそう

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

やっぱり場合によってはこの画面を出すことが可能であることを確認
image

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

あとWebSocketに繋ぐのは無駄そう

@kakkokari-gtyih
Copy link
Contributor Author

あとWebSocketに繋ぐのは無駄そう

将来的にLTL埋め込みとかが入ると使うかもしれない(ただし通常の埋め込みに必要ないのは事実なのでBootOptionsで切れるようにするのがよさそう

@syuilo
Copy link
Member

syuilo commented Jun 2, 2024

LTL埋め込む場合でもポーリングで事足りそう

@kakkokari-gtyih
Copy link
Contributor Author

ページ遷移は必ず新しいタブ の方が良いわね

navHookで全部別窓に飛ばすようにした

@kakkokari-gtyih
Copy link
Contributor Author

kakkokari-gtyih commented Jun 2, 2024

やっぱり場合によってはこの画面を出すことが可能であることを確認

これってどこ由来の画面だっけ

@syuilo
Copy link
Member

syuilo commented Jul 6, 2024

サーバー側のbootも分けた方が良さそう

@syuilo
Copy link
Member

syuilo commented Jul 6, 2024

セキュリティやパフォーマンスを考慮して分けられるところは極力分けたい
埋め込みと非埋め込みで処理を共通化してもあまり良いことがなさそう

@syuilo
Copy link
Member

syuilo commented Jul 6, 2024

今そういう実装になってるかまだ確認できてないけど、↑と同様の理由でクライアントからAPIリクエストを発生させないような実装にしたい

@syuilo
Copy link
Member

syuilo commented Jul 6, 2024

半SSRみたいな感じ

@kakkokari-gtyih
Copy link
Contributor Author

kakkokari-gtyih commented Jul 6, 2024

クライアントからAPIリクエストを発生させないような実装にしたい

フロントエンドを使い回す意味が殆どなくなってしまう(結局 #10491 になりそう)

あと起動処理やstoreは分離していてアカウントはnullを返すようになってるので問題はなさそうではある

@syuilo
Copy link
Member

syuilo commented Jul 6, 2024

/preview

@@ -645,6 +648,8 @@ export function popupMenu(items: MenuItem[], src?: HTMLElement | EventTarget | n
}

export function contextMenu(items: MenuItem[], ev: MouseEvent): Promise<void> {
if (embedPage) return Promise.resolve();
Copy link
Member

Choose a reason for hiding this comment

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

これはなぜかしら

Copy link
Contributor Author

@kakkokari-gtyih kakkokari-gtyih Jul 6, 2024

Choose a reason for hiding this comment

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

popupを全面的に無効にしているのと、iframe中で別のコンテキストメニューがでると混乱を招く可能性があるので

Copy link
Member

Choose a reason for hiding this comment

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

popup全面的に無効ってなんでだっけ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

意図せずModalが立ち上がったりするのを防ぐため(主にlogin系)

Copy link
Member

Choose a reason for hiding this comment

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

うーむ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そもそもpopupを無効化するのがなんかよくわからない

うーむ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

想定していない操作をされる可能性が高い

例えば埋め込みページ内で埋め込みコードを作ったりもできてしまう

image

Copy link
Member

Choose a reason for hiding this comment

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

それはpopup呼び出す側で制御するべきそう

Copy link
Contributor Author

@kakkokari-gtyih kakkokari-gtyih Jul 6, 2024

Choose a reason for hiding this comment

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

それはpopup呼び出す側で制御するべきそう

制御を組み込む箇所が多岐にわたる+embed側で読み込みを許容するpopupが少ない となると、読み込みの可否を制御するロジックをpopupが呼ばれる個所一つ一つに入れるのは現実的ではないなと思ってpopupのオプションにてオプトインで埋め込みページでも表示できるような仕組みにしておいたけどそれじゃ不味いかしら

const { dispose } = popup(SomeComponent, {
	propsKey: 'foo',
}, {
	closed: () => dispose(),
}, {
+	callEvenOnEmbedPage: true,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

↑というかこれはpopup呼ぶ側で制御できることになるのでは?(埋め込みページデフォルトOFF、オプションでONにできるので)

@kakkokari-gtyih
Copy link
Contributor Author

image

サーバーサイドのbootを分けたついでに小さい画面でもエラーが見切れないようにした

@kakkokari-gtyih
Copy link
Contributor Author

サーバー側のbootも分けた方が良さそう

done

@kakkokari-gtyih
Copy link
Contributor Author

コンフリクト解消

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR packages/misskey-js
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misskeyの投稿埋め込み
3 participants