refactor: clean repetitive boilerplate code at project entrypoint - #905
Conversation
…ager into refactor/entrypoint
📝 WalkthroughWalkthroughThe change adds ordered cleanup positions and a ChangesCleanup lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ContentIndex as content/index.tsx
participant CleanupManager
participant CleanupFunctions as registered cleanup functions
ContentIndex->>CleanupManager: register cleanup callbacks with positions
ContentIndex->>CleanupManager: executeCleanups on beforeunload
CleanupManager->>CleanupFunctions: invoke callbacks in position order
CleanupManager-->>ContentIndex: rethrow the last cleanup error, if any
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
andy-ish
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/types/cleanupPositions.ts`:
- Around line 1-24: Update CleanupPositions and its consumers so
CleanupManager.executeCleanups() preserves the legacy registration order,
particularly ensuring CleanupAccountContextBridge runs before CleanupPluginHost
and all other existing dependencies retain their prior sequence. Reorder the
enum values to match that order, or add coverage that verifies the intended
cleanup dependency ordering.
In `@src/core/utils/cleanupManager.ts`:
- Around line 56-71: Update executeCleanups in
src/core/utils/cleanupManager.ts:56-71 to track whether a cleanup catch occurred
separately from the captured error, then rethrow whenever that flag is set so
falsy values are preserved exactly. In
src/core/utils/__tests__/cleanupManager.test.ts:73-87, add a falsy-error case
verifying that a later cleanup still runs and the exact falsy value is rethrown.
- Around line 44-45: Update the list() method in the cleanup manager to remove
the incompatible as const assertion and return the copied cleanups as the
declared mutable Array<Cleanup> type.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f5e152f-07e7-4962-a830-d814006241d5
📒 Files selected for processing (4)
src/core/types/cleanupPositions.tssrc/core/utils/__tests__/cleanupManager.test.tssrc/core/utils/cleanupManager.tssrc/pages/content/index.tsx
| export enum CleanupPositions { | ||
| RemoveUnhandledRejectionEventListener, | ||
| RemoveErrorEventListener, | ||
| StopWatermarkRemover, | ||
| DestroyFolderManagerInstance, | ||
| DestroyPromptManagerInstance, | ||
| DestroySlashPromptFeatureInstance, | ||
| CleanupQuoteReply, | ||
| CleanupInputVimMode, | ||
| CleanupSendBehavior, | ||
| CleanupDraftSave, | ||
| CleanupFork, | ||
| CleanupGemsSidebar, | ||
| CleanupResponseCompleteNotification, | ||
| CleanupEdgeFinalVersionNotice, | ||
| CleanupPluginHost, | ||
| CleanupBrandTheme, | ||
| CleanupRemoteAnnouncements, | ||
| CleanupStorageQuotaWarning, | ||
| CleanupAccountContextBridge, | ||
| CleanupCodeBlockCollapse, | ||
| CleanupUsageStatus, | ||
| RemoveStorageOnChangedListener, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
upstream="$(git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}' 2>/dev/null || true)"
if [[ -n "$upstream" ]]; then
base="$(git merge-base HEAD "$upstream")"
elif git rev-parse --verify HEAD^ >/dev/null 2>&1; then
base="HEAD^"
else
echo "No comparison base is available."
exit 0
fi
git diff --unified=100 "$base"...HEAD -- \
src/pages/content/index.tsx \
src/core/types/cleanupPositions.tsRepository: Nagi-ovo/voyager
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository state ---'
git status --short
printf '%s\n' '--- recent commits ---'
git log --oneline -8
printf '%s\n' '--- cleanup-related files ---'
git ls-files | rg '(^|/)(cleanup|Cleanup)|src/pages/content/index\.tsx$' || true
printf '%s\n' '--- cleanup symbol references ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' \
'CleanupPositions|executeCleanups|beforeunload|RemoveUnhandledRejectionEventListener|RemoveErrorEventListener' .Repository: Nagi-ovo/voyager
Length of output: 11255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cleanup-related commit summaries ---'
git show --stat --oneline --no-renames 7b621e2f
git show --stat --oneline --no-renames b951e9e2
git show --stat --oneline --no-renames 82150698
printf '%s\n' '--- entrypoint changes across cleanup commits ---'
git show --format=fuller --no-ext-diff --unified=12 7b621e2f -- src/pages/content/index.tsx src/core/types/cleanupPositions.ts src/core/utils/cleanupManager.ts
git show --format=fuller --no-ext-diff --unified=12 b951e9e2 -- src/pages/content/index.tsx src/core/types/cleanupPositions.ts src/core/utils/cleanupManager.ts
git show --format=fuller --no-ext-diff --unified=12 82150698 -- src/pages/content/index.tsx src/core/types/cleanupPositions.ts src/core/utils/cleanupManager.tsRepository: Nagi-ovo/voyager
Length of output: 27105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cleanup manager implementation ---'
cat -n src/core/utils/cleanupManager.ts
printf '%s\n' '--- previous entrypoint teardown ---'
git show 7b621e2f^:src/pages/content/index.tsx | sed -n '620,760p'
printf '%s\n' '--- current entrypoint teardown and registrations ---'
sed -n '540,750p' src/pages/content/index.tsxRepository: Nagi-ovo/voyager
Length of output: 12966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- previous cleanup registrations in source order ---'
git show 7b621e2f^:src/pages/content/index.tsx |
nl -ba |
rg 'registerCleanupFunction|registerCleanupFunctionAndReturnIt'
printf '%s\n' '--- current cleanup registrations in source order ---'
nl -ba src/pages/content/index.tsx |
rg 'registerCleanupFunction|registerCleanupFunctionAndReturnIt'
printf '%s\n' '--- previous cleanup registrations with context ---'
git show 7b621e2f^:src/pages/content/index.tsx |
nl -ba |
rg -C 3 'registerCleanupFunction|registerCleanupFunctionAndReturnIt'Repository: Nagi-ovo/voyager
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- previous cleanup registrations in source order ---'
git show 7b621e2f^:src/pages/content/index.tsx |
grep -nE 'registerCleanupFunction|registerCleanupFunctionAndReturnIt' || true
printf '%s\n' '--- current cleanup registrations in source order ---'
grep -nE 'registerCleanupFunction|registerCleanupFunctionAndReturnIt' src/pages/content/index.tsx || true
printf '%s\n' '--- previous cleanup registrations with context ---'
git show 7b621e2f^:src/pages/content/index.tsx |
grep -n -E -B3 -A3 'registerCleanupFunction|registerCleanupFunctionAndReturnIt' || trueRepository: Nagi-ovo/voyager
Length of output: 11873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
enum_text = Path("src/core/types/cleanupPositions.ts").read_text()
names = re.findall(r"^\s{2}([A-Za-z][A-Za-z0-9_]*),?$", enum_text, re.M)
positions = {name: index for index, name in enumerate(names)}
legacy_gemini_order = [
"CleanupAccountContextBridge",
"CleanupPluginHost",
"CleanupBrandTheme",
"RemoveUnhandledRejectionEventListener",
"RemoveErrorEventListener",
"CleanupRemoteAnnouncements",
"CleanupStorageQuotaWarning",
"RemoveStorageOnChangedListener",
"DestroySlashPromptFeatureInstance",
"CleanupEdgeFinalVersionNotice",
"DestroyFolderManagerInstance",
"CleanupInputVimMode",
"CleanupSendBehavior",
"CleanupQuoteReply",
"StopWatermarkRemover",
"CleanupResponseCompleteNotification",
"CleanupDraftSave",
"CleanupGemsSidebar",
"CleanupUsageStatus",
"CleanupCodeBlockCollapse",
"CleanupFork",
"DestroyPromptManagerInstance",
]
actual = [positions[name] for name in legacy_gemini_order]
inversions = [
(legacy_gemini_order[i], legacy_gemini_order[j])
for i in range(len(actual))
for j in range(i + 1, len(actual))
if actual[i] > actual[j]
]
print("Enum positions:")
for name in legacy_gemini_order:
print(f"{name}: {positions[name]}")
print(f"Legacy order preserved: {not inversions}")
print("First ordering differences:")
for left, right in inversions[:12]:
print(f"{left} must run before {right}, but the enum runs them in the opposite order")
PYRepository: Nagi-ovo/voyager
Length of output: 2231
Preserve the legacy cleanup order. CleanupManager.executeCleanups() previously used registration order because every cleanup had position -1. The enum reverses existing ordering; for example, CleanupAccountContextBridge was registered before CleanupPluginHost, but positions 18 and 14 execute them in the opposite order. Reorder the enum or add tests for the intended dependency order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/types/cleanupPositions.ts` around lines 1 - 24, Update
CleanupPositions and its consumers so CleanupManager.executeCleanups() preserves
the legacy registration order, particularly ensuring CleanupAccountContextBridge
runs before CleanupPluginHost and all other existing dependencies retain their
prior sequence. Reorder the enum values to match that order, or add coverage
that verifies the intended cleanup dependency ordering.
| list(): Array<Cleanup> { | ||
| return [...this.cleanups] as const; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 \
'list\(\): Array<Cleanup>|return \[\.\.\.this\.cleanups\] as const' \
src/core/utils/cleanupManager.ts
# Then run the repository's normal TypeScript type-check job locally or in CI.
# Expect no readonly-array assignment error for src/core/utils/cleanupManager.ts.Repository: Nagi-ovo/voyager
Length of output: 323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cleanupManager.ts ---'
cat -n src/core/utils/cleanupManager.ts | sed -n '1,90p'
printf '%s\n' '--- project type-check configuration ---'
for f in package.json tsconfig.json; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
cat -n "$f" | sed -n '1,220p'
fi
done
printf '%s\n' '--- TypeScript tooling availability ---'
command -v tsc || true
tsc --version 2>/dev/null || trueRepository: Nagi-ovo/voyager
Length of output: 9253
🏁 Script executed:
#!/bin/bash
set -uo pipefail
printf '%s\n' '--- repository type-check ---'
tsc --noEmit --pretty false 2>&1
status=$?
printf 'tsc_exit=%s\n' "$status"
printf '%s\n' '--- isolated return-type check ---'
tmp="$(mktemp --suffix=.ts)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'TS'
interface Cleanup {
pos: number;
func: () => void;
}
class CleanupManager {
private cleanups: Array<Cleanup> = [];
list(): Array<Cleanup> {
return [...this.cleanups] as const;
}
}
TS
tsc --noEmit --strict --target esnext --module esnext --pretty false "$tmp" 2>&1
printf 'isolated_tsc_exit=%s\n' "$?"Repository: Nagi-ovo/voyager
Length of output: 2088
🏁 Script executed:
#!/bin/bash
set -uo pipefail
tmp="$(mktemp --suffix=.ts)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'TS'
interface Cleanup {
pos: number;
func: () => void;
}
class CleanupManager {
private cleanups: Array<Cleanup> = [];
list(): Array<Cleanup> {
return [...this.cleanups] as const;
}
}
TS
tsc --ignoreConfig --noEmit --strict --target esnext --module esnext --pretty false "$tmp" 2>&1
printf 'isolated_tsc_exit=%s\n' "$?"Repository: Nagi-ovo/voyager
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -uo pipefail
tmp="$(mktemp --suffix=.ts)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'TS'
const direct: number[] = [1, 2] as const;
const source: number[] = [];
const spread: number[] = [...source] as const;
interface Cleanup {
pos: number;
func: () => void;
}
class CleanupManager {
private cleanups: Array<Cleanup> = [];
list(): Array<Cleanup> {
return [...this.cleanups] as const;
}
}
TS
tsc --ignoreConfig --noEmit --strict --target esnext --module esnext --pretty false "$tmp" 2>&1
printf 'probe_tsc_exit=%s\n' "$?"Repository: Nagi-ovo/voyager
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -uo pipefail
printf '%s\n' '--- available runtimes and local dependencies ---'
command -v bun || true
bun --version 2>/dev/null || true
command -v node || true
node --version 2>/dev/null || true
if [ -d node_modules/typescript ]; then
node -p "require('./node_modules/typescript/package.json').version" 2>/dev/null || true
else
printf '%s\n' 'node_modules/typescript: absent'
fi
printf '%s\n' '--- lockfile TypeScript entries ---'
for f in bun.lock bun.lockb package-lock.json yarn.lock pnpm-lock.yaml; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
rg -n -C 2 'typescript@|typescript:' "$f" | head -80
fi
doneRepository: Nagi-ovo/voyager
Length of output: 829
🌐 Web query:
TypeScript 5.8 readonly tuple as const assignable to mutable array TS4104
💡 Result:
The error TS4104 occurs in TypeScript when you attempt to assign a readonly array or tuple (often created via as const) to a location that expects a mutable array [1][2][3]. This error exists because readonly types are not assignable to mutable types [4][5]. A mutable type (e.g., T[]) implies that the array can be modified (elements added, removed, or reordered), whereas a readonly type (e.g., readonly T[]) guarantees that it will not be [4]. Allowing this assignment would be unsafe because the consuming code might attempt to mutate the array, violating the readonly contract [4]. To resolve this, you can use one of the following approaches depending on your requirements: 1. Spread the array into a fresh literal: This creates a new, mutable copy of the array elements [6]. const mutableArray = [...myReadonlyTuple]; 2. Cast the value to a mutable type: If you are certain the consuming code will not mutate the array, or if you simply need to satisfy the type checker, you can cast it [7][5]. const mutableArray = myReadonlyTuple as any[]; // Or more safely: const mutableArray = myReadonlyTuple as T[]; 3. Update the consuming type: If you own the code consuming the array, change the type definition to accept a readonly array, which is generally considered better practice for functions that do not intend to modify their inputs [4][7]. function process(data: readonly T[]) {... } The TS4104 error was more prominent in earlier versions of TypeScript (around 5.0-5.1) following stricter checks on readonly-to-mutable assignments [8][9][10]. While the compiler does not automatically "strip" readonly for you [11], using a spread operator is the idiomatic way to produce a mutable array from a readonly one [6].
Citations:
- 1: i18n.locales in astro.config.ts requires only mutable array withastro/astro#17419
- 2: "cannot be assigned to the mutable type 'Type<unknown>[]'" error appears when createUnionType nestjs/graphql#2122
- 3: https://devsolus.com/how-to-create-a-typescript-type-to-allow-read-only-tuples-from-a-const/
- 4: https://mariusschulz.com/blog/read-only-array-and-tuple-types-in-typescript
- 5: https://stackoverflow.com/questions/78766778/when-can-arrays-be-cast-to-tuples-with-a-single-as-assertion
- 6: easdkr/nest-batch@b2a31c2
- 7: https://stackoverflow.com/questions/72048141/inconvenience-when-working-as-const-arrays-readonly-t-not-assignable-to-t
- 8: [NewErrors] 5.1.0-dev.20230305 vs 4.9.5 microsoft/TypeScript#53101
- 9: [NewErrors] 5.0.0-dev.20230219 vs 4.9.5 microsoft/TypeScript#52858
- 10: [NewErrors] 5.0.0-dev.20230122 vs 4.9.4 microsoft/TypeScript#52359
- 11: const array to non readonly array cast compilation does not raise an error with TS@5.3.3 or TS@5.4.0-dev.20240120 (like it does with TS@5.2.2 or TS@5.1.6) microsoft/TypeScript#57107
Remove the incompatible as const assertion.
list() returns a readonly array, but its return type is mutable Array<Cleanup>. TypeScript 5.9.3 rejects this assignment.
Proposed fix
list(): Array<Cleanup> {
- return [...this.cleanups] as const;
+ return [...this.cleanups];
}📝 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.
| list(): Array<Cleanup> { | |
| return [...this.cleanups] as const; | |
| list(): Array<Cleanup> { | |
| return [...this.cleanups]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/utils/cleanupManager.ts` around lines 44 - 45, Update the list()
method in the cleanup manager to remove the incompatible as const assertion and
return the copied cleanups as the declared mutable Array<Cleanup> type.
|
Thanks! The failing watermark test was a flaky test inherited from main, not caused by this PR. It has been fixed on main in d139473. Please rebase this branch onto the latest main and push the updated branch to trigger a fresh CI run. |
Co-authored-by: Codex <codex@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Nagi-ovo
left a comment
Nagi-ovo
left a comment
There was a problem hiding this comment.
已核对清理顺序与旧 beforeunload 顺序一致，并补充生产 enum 顺序回归测试。当前 head 的本地验证、CI、Codex 和 CodeRabbit 均无阻塞问题。
|
Thanks so much! |
2 participants
Description / 描述
该 PR 通过引入一个清理函数管理器（
cleanupManager）来统一管理 src/pages/content/index.tsx 中大量重复的清理代码。Related Issue / 相关 Issue
Closes #897
community-only, I was assigned after maintainer approval before starting. / 如果 Issue 带有community-only标签，我已在开始前获得维护者确认并被分配。Visual Proof / 可视化证据
未涉及 UI 和宏观行为修改。
Browser Testing / 浏览器测试
Tested commit / 测试提交: b17c614
Code Explanation / 代码说明
src/core/utils/cleanupManager.tsregisterCleanupFunction方法注册一个清理函数，并接受一个 pos: number 表示清理函数的执行时机。pos 越小，函数执行越靠前。该方法会对传入的函数进行基于引用的基础去重，但是不会对 pos 去重。registerCleanupFunctionAndReturnIt和registerCleanupFunction的功能一样，只不过会额外把清理函数原样返回。withdrawCleanupFunctionsByPositionNumber接受一个 pos: number，并删除已注册函数中所有 pos 相同的函数。executeCleanups会按照 pos 顺序执行所有的清理函数。若有清理函数报错则会跳过并继续执行后面的，并在所有函数执行完毕后把最后一个报错重新抛出给上游。src/core/types/cleanupPositions.tsChecklist / 检查清单
bun run format,bun run lint, then the standard localbun run verify:pr, or listed every omitted command and reason above. / 我已依次运行格式化、自动修复及标准本地bun run verify:pr验证，或在上方逐项说明未运行命令及原因。Summary by CodeRabbit