Skip to content

Adjust ESLint rules and resolve warnings - #147

Open
16th-admin wants to merge 2 commits into
codex/fix-mermaid-svg-sanitizationfrom
codex/issue-139-eslint-rules
Open

Adjust ESLint rules and resolve warnings#147
16th-admin wants to merge 2 commits into
codex/fix-mermaid-svg-sanitizationfrom
codex/issue-139-eslint-rules

Conversation

@16th-admin

@16th-admin 16th-admin commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • lower max-lines-per-function from 100 to 50
  • refactor long and high-complexity functions into focused helpers
  • remove legacy lint suppressions where the underlying code can be simplified
  • resolve all warnings produced by the adjusted configuration

Dependency

This PR is based on #165 so Mermaid SVG rendering is sanitized before this refactor is merged.

Verification

  • ESLint and i18n check passed after rebase
  • conflicts resolved while preserving the secure SVG DOM insertion

Closes #139

Comment thread src/services/pltxt2htm/advancedParser.ts Fixed
@16th-admin
16th-admin force-pushed the codex/issue-139-eslint-rules branch from b3189b7 to 1fe0f93 Compare July 28, 2026 03:49
@16th-admin
16th-admin force-pushed the codex/issue-139-eslint-rules branch from 1fe0f93 to a8a6324 Compare July 28, 2026 03:51
@16th-admin
16th-admin changed the base branch from main to codex/fix-mermaid-svg-sanitization July 28, 2026 03:51
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode

💡 This is an automated advisory review. It is non-blocking and does not affect merge requirements.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode

💡 This is an automated advisory review. It is non-blocking and does not affect merge requirements.

🔴 PR 代码审查

概述

本次变更涉及 16 个文件,核心主题是:提取公共模式、增强类型安全、修复竞态条件和内存泄漏、降低圈复杂度。整体方向积极,但有几项需要重点关注。


🔴 [blocking] getData.ts:28 — 匿名 API Token 硬编码泄露

const ANONYMOUS_API_TOKEN = '7pEWTsF4gR9qauzJCDQkxPLOZlnbMtAG'

API 密钥硬编码在源码中,所有客户端均可提取。Token 应通过服务器代理注入或仅在构建时注入(如 import.meta.env.VITE_ANONYMOUS_TOKEN)。


🔴 [blocking] getData.tscreateLoginRequest 的 token 认证逻辑缺失回退

重构后的 createApiHeaders 使用了 ANONYMOUS_API_TOKEN,但 login() 在 token 登录(isToken=true)时通过 createLoginRequest 独立构造 headers。对于匿名→有身份的登录流程,getData 在登录完成前发送的请求会携带匿名 token——这是期望行为,但需确认 createLoginRequest 中 token 登录的 x-API-Version 未设置,与 createApiHeaders 不一致。

// createLoginRequest 的 token 分支缺失 version header
headers['x-API-Version'] = '2502'  // 缺失

🔴 [blocking] advancedParser.ts — SVG XSS 净化不充分

createMermaidDiagram 移除了 <script><foreignObject>,但 SVG 中仍可通过 <use href="data:text/html,..."><animate attributeName="href" ...> 或 SVG 嵌套等向量执行 XSS。建议使用 DOMPurify 或更严格的白名单策略。


🟡 [important] MessageList.vueremoveToken(response)reportLoadError 中修改了原始响应对象

const result = removeToken(response)

removeToken 现在通过 WeakSet 就地修改(mutates in-place)。如果调用方后续仍引用原始 response,token 已被截断。应克隆后再做 mask:

function maskSensitiveValues(value: object, visited: WeakSet<object>) {
  // ...
  record[key] = `${nestedValue.slice(0, 6)}******`  // 就地修改!

这影响所有 3 个组件中的 reportLoadErrorMessageListNotificationListwortListExperimentSummary)。需确认 response 后续是否被复用。


🟡 [important] ExperimentSummary.vue — 竞态条件保护方式不一致

fetchSummary 使用 summaryRequestId 进行丢弃(stale-request dropping),但 changeCoversummaryRequest() 没有类似的保护。快速连续切换封面可能产生竞态。

// changeCover 中:
const summaryResponse = await summaryRequest()  // 无 requestId 保护

🟡 [important] NotificationItem.vue — watch 回调是 async,但无错误处理

watch(..., async () => { ... }, { immediate: true })

如果 getUserUrl reject,watch 回调抛出的错误不会被 Vue 捕获(onWatcherCleanup 在 Vue 3.4+ 提供更好的方式)。应使用 try/catch 包裹。


🟡 [important] errorLogger.ts:78previousErrorHandler 在构造函数中赋值,但 setupGlobalHandlers 在之后才覆盖 app.config.errorHandlerdispose() 可正确恢复。但如果在构造函数和 setupGlobalHandlers(instance) 调用之间的窗口期内发生错误,previousErrorHandler 会指向过时的 handler。应延迟 previousErrorHandler 的保存:

private previousErrorHandler: App['config']['errorHandler'] = null!

改为在 setupGlobalHandlers 第一行保存。


🟡 [important] useResponsive.tsaddResizeListeners/removeResizeListeners 是模块级函数而非 composable 内部

如果多个组件调用 useResponsive(),它们共享相同的 window listener,但每个调用都会重新绑定。应在 composable 内保留引用。


🟡 [important] AdvancedParser.tscwrap 类型断言被移除

// 旧方式:
const instanceAny: any = wasmInstance
instanceAny.__advanced_parser_fn = ...

// 新方式:
wasmInstance.__advanced_parser_fn = wasmInstance.cwrap(...)

__advanced_parser_fncwrap 应该是 wasm 绑定上的动态属性。TypeScript 会对 wasmInstance.cwrap 报类型错误。需要 declare 或类型扩展。检查构建是否通过。


🟢 [nit] NotificationList.vue:104supportedLanguages 数组在每次渲染时重新创建

提取到模块作用域:

const SUPPORTED_LANGUAGES: Array<keyof MessageTemplate['Subject']> = [
  'Chinese', 'English', ...
] as const

🟢 [nit] eslint.config.jsmax-lines-per-function 从 100 降低到 50

这是好的方向,但当前 diff 中 ExperimentSummary.vuechangeCover 函数仍远超 50 行。建议对这次重构中显著超限的函数标注 eslint-disable-next-line 或进一步拆分。


🟢 [nit] wortList.vuecreateQuery() 在每次 handleLoad 调用时重建 Query 对象

function queryProjects() {
  return getData('/Contents/QueryExperiments', { Query: createQuery() })
}

每次调用 queryProjects() 都会执行 createQuery() → 展开 ...props.q。这不是 bug,但如果 props.q 包含响应式引用,需要确保响应性传递正确。当前没问题(props.qdefineProps 中,本身就是响应式的),但值得注意。


🟢 [nit] main.tsmatchRoutecategory 推导假设捕获组 2 存在

function matchRoute(text: string, pattern: RegExp, needLogin: boolean) {
  const match = text.match(pattern)
  if (!match) return null
  const id = match[2] || match[1]
  const category = match[2] ? match[1] : null
  return { path: category ? `/p/${category}/${id}` : `/u/${id}`, needLogin }
}

这个方法假设有两种正则格式:/p/(Category)/(id)/u/(id)。模式匹配的职责分散在调用方。建议使用 discriminated union 或拆分为 matchWorkPathmatchUserPath 两个函数以消除假设。


🎉 [praise] 值得肯定的改进

改进点 文件
Typed emits(defineEmits<{msgClick: [message: CommentResult]}>() MessageList.vue
try/finally 确保 loading.value = false 不会因异常跳过 MessageList.vue, NotificationList.vue, wortList.vue
竞态条件保护(requestId 模式) NotificationItem.vue, ExperimentSummary.vue, Profile.vue
合理的函数提取:reportLoadErrorappendMessagescreateQuery 多个组件
XSS 净化:移除 <script><foreignObject>、事件属性 advancedParser.ts
removeToken 循环保护(WeakSet) utils.ts
About.vue TypeScript 迁移 + 定时器清理 About.vue
errorLogger.ts 类型安全改进 + dispose() 生命周期 errorLogger.ts
apiError.ts 防重复挂载保护(closed flag) apiError.ts
formatDate 拆分为 formatRelativeDate / formatAbsoluteDate utils.ts

总结

级别 数量 关键项
🔴 blocking 3 匿名 Token 硬编码、login headers 遗漏 version、SVG XSS 不充分
🟡 important 5 removeToken 就地修改、竞态条件不一致、watch 无 error handling、errorHandler 保存时机、cwrap 类型
🟢 nit 4 常量提取、函数长度、Query 重建、类型假设

建议:修复 blocking 项后再合并,important 项需讨论或附带注释,nit 项可选。整体重构质量高,值得肯定。

@16th-admin
16th-admin force-pushed the codex/issue-139-eslint-rules branch from a8a6324 to 7ad45cc Compare July 28, 2026 04:00
@16th-admin
16th-admin force-pushed the codex/issue-139-eslint-rules branch from 7ad45cc to 3214504 Compare July 28, 2026 04:01
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode

💡 This is an automated advisory review. It is non-blocking and does not affect merge requirements.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode

💡 This is an automated advisory review. It is non-blocking and does not affect merge requirements.

🔴 Review: Large-Scale Refactoring PR

Overall assessment: High-quality refactoring with meaningful improvements in type safety, memory management, race-condition handling, and code organization. The PR extracts repeated patterns, fixes lifecycle bugs, and improves consistency across components. Below are findings organized by severity.


🔴 Blocking

1. Blind type assertion in NotificationList.vue (src/components/messages/NotificationList.vue:86)

function asNotificationMessages(items: unknown[]): NotificationMessage[] {
  return items as NotificationMessage[]
}

This is a type-erasing cast — no runtime validation. If InfiniteScroll passes data of an unexpected shape, the downstream template accesses will fail silently. Either validate the shape or document why this is safe (e.g., if InfiniteScroll is guaranteed to pass back the same items array).

2. Parameter mutation in MessageList.vue:appendMessages (src/components/messages/MessageList.vue:96)

function appendMessages(messages: CommentResult[]) {
  const receivedCount = messages.length
  if (from) messages.shift()  // mutates caller's array

messages.shift() mutates the array that was passed in, which is surprising to callers. Use messages.slice(1) for the shifted variant instead.

3. Error: submitCover passes .jpg but confirmCover uses .png (src/views/ExperimentSummary.vue)

  • submitCoverExtension: '.jpg'
  • confirmCover / confirmCoverRequestExtension: '.png'
  • The upload flow does confirmCoversubmitCover → upload → confirmCover again. The extension mismatch should be verified as intentional (different API steps), not a copy-paste inconsistency.

🟡 Important

4. errorLogger.ts — previous handler not called (src/services/errorLogger.ts:176-178)

app.config.errorHandler = (error, _instance, info) => {
  this.captureError({ ... })
  // NOTE: this.previousErrorHandler is never called
}

The constructor saves this.previousErrorHandler = app.config.errorHandler but the new handler never invokes it. If another plugin (e.g., vue-router, vue-i18n, Sentry) had already set the handler, its errors are now silently swallowed. Either call the previous handler or document the intentional override.

5. wortList.vuenoMore guard vs handleLoad retry loop (src/components/projects/wortList.vue:99)

function reportLoadError(response, ...) {
  showAPiError(..., handleLoad, ...)  // retry calls handleLoad
}

If handleLoad fails, isGettingData is released in the finally block, allowing the retry. But if the retry yields fewer than 24 results, noMore stays true forever — even if the underlying data changed. This is the same logic as before, so not a regression, but worth noting.

6. getData.tscreateApiHeaders returns ANONYMOUS_API_TOKEN for unauthenticated requests, but the const is hardcoded (src/services/api/getData.ts:28)

const ANONYMOUS_API_TOKEN = '7pEWTsF4gR9qauzJCDQkxPLOZlnbMtAG'

If this is a public/shared token it should be documented as such. If it's sensitive, move to an env variable. Its module-level scope means it's bundled into every build.

7. ExperimentSummary.vue — JSONP script cleanup (src/views/ExperimentSummary.vue:319)

const img = new Image()
img.src = `https://...`
// No abort if component unmounts before response

The fetchSummary function races with summaryRequestId, so stale responses are discarded — good. But the <img> / JSONP request in getCoverUrl has no abort mechanism if the component unmounts, and its onload/onerror could fire after unmount. Low risk but worth a useEffect-style cleanup.

8. removeToken with WeakSet — circular reference risk (src/services/utils.ts:349)
Using WeakSet to track visited objects is a good improvement over the previous unbounded recursion. However, if the object graph is deeply nested (>100 levels), it still recurses synchronously and could blow the stack. Consider an iterative approach or a depth limit.

9. apiError.tsapp.use(i18n) and app.mount(div) order (src/services/popup/apiError.ts:74-76)

app.use(i18n)
// mount happens at the end now, after current is set
current = { close, title: titleRef, message: messageRef, retry }
app.mount(div)

The old code mounted before assigning current, leaving a window where current is null during retry callbacks. The new code fixes this by assigning current before mount. ✅ But verify that close() in current is correct (it is — it uses the close closure).


🟢 Nits & Suggestions

10. eslint.config.jsmax-lines-per-function reduced to 50 — Good intention, but the PR introduces functions longer than 50 lines (e.g., changeCover in ExperimentSummary.vue). Either extract more sub-functions or bump the limit incrementally to 70 first.

11. matchRoute in main.ts (src/main.ts:117-122) — The strategy-pattern-based matchers array is clean. However, matchRoute uses positional capture groups (match[1], match[2]), making it fragile when regexes change. Consider named groups or a more explicit return mapping.

12. advancedParser.tsgetParserContext returns a tuple but callers destructure with spread (src/services/pltxt2htm/advancedParser.ts:140)

const rawHtml = await advancedParser(source, ...getParserContext(context))

This is fine but implicitly couples the tuple order to advancedParser's parameter list. If a parameter is added mid-tuple, only the function signature changes — TS won't catch the mismatch at the call site because the spread destructures to positional args. A named options object would be safer.

13. fillInTemplate in NotificationList.vue (src/components/messages/NotificationList.vue:112) — The reduce over replacements is elegant. But each iteration does a full regex scan of the string. For a handful of replacements on typical message lengths this is fine. For very long templates with many fields, a single-pass parser would be faster (not a concern here).

14. About.vuescheduleTimeout/scheduleInterval wrapping (src/views/About.vue:158-170) — The wrapper functions that track IDs in Sets are a good pattern. However, setTimeout in scheduleTimeout only removes the ID after the callback fires. If onUnmounted fires and clears timeouts, the pending timeout won't fire — correct. But if the timeout fires during unmount, there's a tiny window where callback() runs after onUnmounted clears everything. Add a mounted guard inside the callback or use an AbortSignal.


Summary

Category Count Highlights
🔴 Blocking 3 Type assertion, parameter mutation, extension mismatch
🟡 Important 6 Error handler chain, hardcoded token, missing cleanup guards
🟢 Nit/Suggestion 5 Lint threshold, fragile regex, tuple coupling, timer wrapper window

General praise: The PR consistently improves race-condition safety (request counters), eliminates DOM leaks (duplicate IDs, timer cleanup), adds finally guards, extracts DRY error-handling patterns, and migrates onMountedwatch with immediate for route-param reactivity. The errorLogger.ts refactor is particularly well-structured.

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.

Adjust ESLint rules

2 participants