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.

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

Jump to bottom

feat: 集成PageSpy #4820

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+57 −2
Open

feat: 集成PageSpy #4820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87e5b05
feat: 引入Upload Icon
jinmao88 Nov 6, 2024
b8182b6
feat: 初步集成PageSpy，设置内增加上传离线日志按钮
jinmao88 Nov 6, 2024
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
1 change: 1 addition & 0 deletions packages/@core/base/icons/src/lucide.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export {
ChevronsLeft,
ChevronsRight,
CircleHelp,
CloudUpload,
Copy,
CornerDownLeft,
Ellipsis,
Expand Down
23 changes: 21 additions & 2 deletions packages/effects/layouts/src/widgets/preferences/preferences-drawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type { SegmentedItem } from '@vben-core/shadcn-ui';

import { computed, ref } from 'vue';

import { Copy, RotateCw } from '@vben/icons';
import { CloudUpload, Copy, RotateCw } from '@vben/icons';
import { $t, loadLocaleMessages } from '@vben/locales';
import {
clearPreferencesCache,
Expand All @@ -30,7 +30,7 @@ import {
} from '@vben-core/shadcn-ui';
import { globalShareState } from '@vben-core/shared/global-state';

import { useClipboard } from '@vueuse/core';
import { useClipboard, useThrottleFn } from '@vueuse/core';

import {
Animation,
Expand Down Expand Up @@ -217,6 +217,18 @@ async function handleReset() {
resetPreferences();
await loadLocaleMessages(preferences.app.locale);
}
const harbor = computed(() => window.$harbor);
// 防抖
const handleUploadLog = useThrottleFn(() => {
if (!harbor.value) {
return;
}
harbor.value.onOfflineLog('upload');
message.copyPreferencesSuccess?.(
$t('preferences.logUploadSuccessTitle'),
$t('preferences.logUploadSuccess'),
);
}, 5000);
Comment on lines +221 to +231
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and user feedback in log upload

The log upload function needs improvements in several areas:

  1. Error handling is missing
  2. No loading state during upload
  3. Hard-coded throttle duration
  4. Comment should be in English for consistency
-// 防抖
+// Throttle log upload to prevent excessive calls
-const handleUploadLog = useThrottleFn(() => {
+const UPLOAD_THROTTLE_MS = 5000;
+const isUploading = ref(false);
+
+const handleUploadLog = useThrottleFn(async () => {
   if (!harbor.value) {
+    message.error?.($t('preferences.logUploadNoHarbor'));
     return;
   }
-  harbor.value.onOfflineLog('upload');
-  message.copyPreferencesSuccess?.(
-    $t('preferences.logUploadSuccessTitle'),
-    $t('preferences.logUploadSuccess'),
-  );
-}, 5000);
+  try {
+    isUploading.value = true;
+    await harbor.value.onOfflineLog('upload');
+    message.success?.(
+      $t('preferences.logUploadSuccessTitle'),
+      $t('preferences.logUploadSuccess'),
+    );
+  } catch (error) {
+    message.error?.(
+      $t('preferences.logUploadError'),
+      error instanceof Error ? error.message : 'Unknown error'
+    );
+  } finally {
+    isUploading.value = false;
+  }
+}, UPLOAD_THROTTLE_MS);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 防抖
const handleUploadLog = useThrottleFn(() => {
if (!harbor.value) {
return;
}
harbor.value.onOfflineLog('upload');
message.copyPreferencesSuccess?.(
$t('preferences.logUploadSuccessTitle'),
$t('preferences.logUploadSuccess'),
);
}, 5000);
// Throttle log upload to prevent excessive calls
const UPLOAD_THROTTLE_MS = 5000;
const isUploading = ref(false);
const handleUploadLog = useThrottleFn(async () => {
if (!harbor.value) {
message.error?.($t('preferences.logUploadNoHarbor'));
return;
}
try {
isUploading.value = true;
await harbor.value.onOfflineLog('upload');
message.success?.(
$t('preferences.logUploadSuccessTitle'),
$t('preferences.logUploadSuccess'),
);
} catch (error) {
message.error?.(
$t('preferences.logUploadError'),
error instanceof Error ? error.message : 'Unknown error'
);
} finally {
isUploading.value = false;
}
}, UPLOAD_THROTTLE_MS);

</script>

<template>
Expand All @@ -228,6 +240,13 @@ async function handleReset() {
>
<template #extra>
<div class="flex items-center">
<VbenIconButton
:disabled="!harbor"
:tooltip="$t('preferences.logUpload')"
class="relative"
>
<CloudUpload class="size-4" @click="handleUploadLog" />
</VbenIconButton>
Comment on lines +243 to +249
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve upload button UX and accessibility

The upload button implementation has several areas for improvement:

  1. Click handler should be on the button, not the icon
  2. Loading state should be reflected in the UI
  3. Missing accessibility attributes
 <VbenIconButton
   :disabled="!harbor"
   :tooltip="$t('preferences.logUpload')"
+  :loading="isUploading"
   class="relative"
+  aria-label="Upload logs"
+  @click="handleUploadLog"
 >
-  <CloudUpload class="size-4" @click="handleUploadLog" />
+  <CloudUpload class="size-4" />
 </VbenIconButton>

Committable suggestion skipped: line range outside the PR's diff.

<VbenIconButton
:disabled="!diffPreference"
:tooltip="$t('preferences.resetTip')"
Expand Down
3 changes: 3 additions & 0 deletions packages/locales/src/langs/en-US/preferences.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
"copyPreferencesSuccessTitle": "Copy successful",
"copyPreferencesSuccess": "Copy successful, please override in `src/preferences.ts` under app",
"clearAndLogout": "Clear Cache & Logout",
"logUpload": "Upload Log",
"logUploadSuccess": "Log Upload Successful",
"logUploadSuccessTitle": "Upload Successful",
"mode": "Mode",
"general": "General",
"language": "Language",
Expand Down
3 changes: 3 additions & 0 deletions packages/locales/src/langs/zh-CN/preferences.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
"copyPreferencesSuccessTitle": "复制成功",
"copyPreferencesSuccess": "复制成功，请在 app 下的 `src/preferences.ts`内进行覆盖",
"clearAndLogout": "清空缓存 & 退出登录",
"logUpload": "上传日志",
"logUploadSuccess": "离线日志上传成功",
"logUploadSuccessTitle": "上传成功",
"mode": "模式",
"general": "通用",
"language": "语言",
Expand Down
29 changes: 29 additions & 0 deletions playground/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,35 @@
})();
}
</script>
<!-- 可以使用自己的部署的PageSpy的sdk 修改引入的地址即可 -->
<!-- https://pagespy.mufei88.com 为vben测试sdk地址 -->
<script
crossorigin="anonymous"
src="https://pagespy.mufei88.com/page-spy/index.min.js"
></script>
<script
crossorigin="anonymous"
src="https://pagespy.mufei88.com/plugin/data-harbor/index.min.js"
></script>
<script
crossorigin="anonymous"
src="https://pagespy.mufei88.com/plugin/rrweb/index.min.js"
></script>
Comment on lines +32 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Enhance the security of external script loading

The current implementation loads scripts from an external domain without proper security measures:

  1. The domain pagespy.mufei88.com appears to be a test environment. Consider hosting these scripts on your own CDN for production.
  2. Add Subresource Integrity (SRI) hashes to ensure script integrity.
  3. Pin specific versions in URLs to prevent unexpected changes.

Apply these security enhancements:

 <script
   crossorigin="anonymous"
+  integrity="sha384-[HASH]"
+  defer
-  src="https://pagespy.mufei88.com/page-spy/index.min.js"
+  src="https://pagespy.mufei88.com/page-spy/1.x.x/index.min.js"
 ></script>

Note: Replace [HASH] with actual SHA-384 hash of the file and 1.x.x with specific version.

Committable suggestion skipped: line range outside the PR's diff.

<script>
window.$harbor = new DataHarborPlugin();
window.$rrweb = new RRWebPlugin();

[window.$harbor, window.$rrweb].forEach((p) => {
PageSpy.registerPlugin(p);
});
// 实例化的参数都是可选的
window.$pageSpy = new PageSpy({
project: '<%= VITE_APP_TITLE %>',
autoRender: false,
title: 'Vben Admin',
});
Comment on lines +52 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Configuration values need to be consistent with environment variables

The verification reveals inconsistencies in the configuration:

  • The project parameter correctly uses VITE_APP_TITLE from environment variables
  • However, the title parameter is hardcoded as 'Vben Admin' while it should also use VITE_APP_TITLE
  • 'Vben Admin' is actually the default value for VITE_APP_TITLE as seen in internal/vite-config/src/utils/env.ts

The configuration should be updated to:

window.$pageSpy = new PageSpy({
  project: '<%= VITE_APP_TITLE %>',
  autoRender: false,
  title: '<%= VITE_APP_TITLE %>',
});
🔗 Analysis chain

Verify configuration parameters

The configuration uses hardcoded values and mixes environment variables.

Let's verify the configuration values across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for PageSpy configuration and title usage

# Check for VITE_APP_TITLE usage
echo "Checking VITE_APP_TITLE usage:"
rg "VITE_APP_TITLE" -A 2

# Check for hardcoded 'Vben Admin' title
echo "\nChecking hardcoded title usage:"
rg "'Vben Admin'" -A 2

Length of output: 6028

// 之后即可使用 PageSpy，前往 https://pagespy.mufei88.com 体验
</script>
Comment on lines +44 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve implementation robustness and environment handling

The current implementation has several areas for improvement:

  1. Global variables pollute the window object
  2. No error handling
  3. No environment-based conditional loading

Consider refactoring to:

 <script>
+  // Only initialize in non-production environments or based on configuration
+  if (import.meta.env.DEV || window._VBEN_ADMIN_PRO_APP_CONF_.enablePageSpy) {
+    try {
+      const harbor = new DataHarborPlugin();
+      const rrweb = new RRWebPlugin();
 
-      [window.$harbor, window.$rrweb].forEach((p) => {
+      [harbor, rrweb].forEach((p) => {
         PageSpy.registerPlugin(p);
       });
-      window.$pageSpy = new PageSpy({
+      const pageSpy = new PageSpy({
         project: '<%= VITE_APP_TITLE %>',
         autoRender: false,
         title: 'Vben Admin',
       });
+
+      // If needed, expose through a namespaced object instead of directly on window
+      window.__VBEN__ = {
+        ...window.__VBEN__,
+        debug: { pageSpy, harbor, rrweb }
+      };
+    } catch (error) {
+      console.error('Failed to initialize PageSpy:', error);
+    }
+  }
 </script>

Committable suggestion skipped: line range outside the PR's diff.

</head>
<body>
<div id="app"></div>
Expand Down
Loading