-
-
Notifications
You must be signed in to change notification settings - Fork 641
refactor: clean repetitive boilerplate code at project entrypoint #905
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
Changes from all commits
0c38ce0
e0db382
f0e982c
b330786
f28d57c
ab1a9a4
18fbd08
e7f013d
f8e9ad9
51a8f44
7b621e2
b951e9e
8215069
f9f379e
4337dfd
057a71c
b17c614
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| 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, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { CleanupPositions } from '@/core/types/cleanupPositions'; | ||
| import { CleanupManager } from '@/core/utils/cleanupManager'; | ||
|
|
||
| enum Sequence { | ||
| First, | ||
| Second, | ||
| Third, | ||
| } | ||
|
|
||
| describe('willCleanUp tests module', () => { | ||
| let cleanupManager: CleanupManager; | ||
|
|
||
| beforeEach(() => { | ||
| cleanupManager = new CleanupManager(); | ||
| }); | ||
|
|
||
| it('can store registered cleanup functions', () => { | ||
| const function1 = () => {}; | ||
| const function2 = () => {}; | ||
|
|
||
| cleanupManager.registerCleanupFunction(function1); | ||
| cleanupManager.registerCleanupFunction(function2); | ||
| cleanupManager.registerCleanupFunction(function2); // won't store duplicate functions | ||
|
|
||
| expect(cleanupManager.list()).toEqual([ | ||
| { | ||
| pos: -1, | ||
| func: function1, | ||
| }, | ||
| { | ||
| pos: -1, | ||
| func: function2, | ||
| }, | ||
| ]); | ||
| }); | ||
|
|
||
| it('can return registered functions as-is', () => { | ||
| const function1 = () => {}; | ||
|
|
||
| expect(cleanupManager.registerCleanupFunctionAndReturnIt(function1)).toBe(function1); | ||
| }); | ||
|
|
||
| it('can execute registered cleanup functions at correct time', () => { | ||
| const function1 = vi.fn(); | ||
|
|
||
| cleanupManager.registerCleanupFunction(function1); | ||
|
|
||
| expect(function1).not.toHaveBeenCalled(); | ||
|
|
||
| cleanupManager.executeCleanups(); | ||
|
|
||
| expect(function1).toHaveBeenCalled(); | ||
| expect(function1).toHaveBeenCalledTimes(1); | ||
|
|
||
| cleanupManager.executeCleanups(); // no duplicate call | ||
|
|
||
| expect(function1).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('can release stored cleanup functions at correct time', () => { | ||
| const function1 = () => {}; | ||
|
|
||
| cleanupManager.registerCleanupFunction(function1); | ||
|
|
||
| expect(cleanupManager.list()).not.toEqual([]); | ||
|
|
||
| cleanupManager.executeCleanups(); | ||
|
|
||
| expect(cleanupManager.list()).toEqual([]); | ||
| }); | ||
|
|
||
| it('can safely handle cleanup functions that throws an error', () => { | ||
| const function1 = () => { | ||
| throw Error(); | ||
| }; | ||
| const function2 = vi.fn(); | ||
|
|
||
| cleanupManager.registerCleanupFunction(function1); | ||
| cleanupManager.registerCleanupFunction(function2); | ||
|
|
||
| expect(() => cleanupManager.executeCleanups()).toThrow(); | ||
|
|
||
| expect(function2).toHaveBeenCalled(); | ||
| expect(function2).toHaveBeenCalledTimes(1); | ||
| expect(cleanupManager.list()).toEqual([]); | ||
| }); | ||
|
|
||
| it('can identify cleanup functions that throws falsy error', () => { | ||
| const function1 = () => { | ||
| throw undefined; | ||
| }; | ||
|
|
||
| const errorSpy = vi.fn(); | ||
|
|
||
| cleanupManager.registerCleanupFunction(function1); | ||
|
|
||
| try { | ||
| cleanupManager.executeCleanups(); | ||
| } catch (error) { | ||
| errorSpy(error); | ||
| } | ||
|
|
||
| expect(errorSpy).toHaveBeenCalledTimes(1); | ||
| expect(errorSpy).toHaveBeenLastCalledWith(undefined); | ||
| }); | ||
|
|
||
| it('can call functions in correct sequence', () => { | ||
| const function1 = vi.fn(); | ||
| const function2 = vi.fn(); | ||
| const function3 = vi.fn(); | ||
|
|
||
| cleanupManager.registerCleanupFunction(function3, Sequence.Third); | ||
| cleanupManager.registerCleanupFunction(function2, Sequence.Second); | ||
| cleanupManager.registerCleanupFunction(function1, Sequence.First); | ||
|
|
||
| cleanupManager.executeCleanups(); | ||
|
|
||
| expect(function2).toHaveBeenCalledAfter(function1); | ||
| expect(function3).toHaveBeenCalledAfter(function2); | ||
| }); | ||
|
|
||
| it('keeps production cleanup positions in the legacy execution order', () => { | ||
| const positionsInExecutionOrder = Object.values(CleanupPositions).filter( | ||
| (position): position is string => typeof position === 'string', | ||
| ); | ||
|
|
||
| expect(positionsInExecutionOrder).toEqual([ | ||
| 'RemoveUnhandledRejectionEventListener', | ||
| 'RemoveErrorEventListener', | ||
| 'StopWatermarkRemover', | ||
| 'DestroyFolderManagerInstance', | ||
| 'DestroyPromptManagerInstance', | ||
| 'DestroySlashPromptFeatureInstance', | ||
| 'CleanupQuoteReply', | ||
| 'CleanupInputVimMode', | ||
| 'CleanupSendBehavior', | ||
| 'CleanupDraftSave', | ||
| 'CleanupFork', | ||
| 'CleanupGemsSidebar', | ||
| 'CleanupResponseCompleteNotification', | ||
| 'CleanupEdgeFinalVersionNotice', | ||
| 'CleanupPluginHost', | ||
| 'CleanupBrandTheme', | ||
| 'CleanupRemoteAnnouncements', | ||
| 'CleanupStorageQuotaWarning', | ||
| 'CleanupAccountContextBridge', | ||
| 'CleanupCodeBlockCollapse', | ||
| 'CleanupUsageStatus', | ||
| 'RemoveStorageOnChangedListener', | ||
| ]); | ||
| }); | ||
|
|
||
| it('can withdraw functions by position number', () => { | ||
| const function1 = vi.fn(); | ||
| const function2 = vi.fn(); | ||
| const function3 = vi.fn(); | ||
|
|
||
| cleanupManager.registerCleanupFunction(function3, Sequence.Third); | ||
| cleanupManager.registerCleanupFunction(function2, Sequence.Second); | ||
| cleanupManager.registerCleanupFunction(function1, Sequence.First); | ||
|
|
||
| cleanupManager.withdrawCleanupFunctionsByPositionNumber(Sequence.Second); | ||
|
|
||
| expect(cleanupManager.list().some((cleanups) => cleanups.func === function1)).toBe(true); | ||
| expect(cleanupManager.list().some((cleanups) => cleanups.func === function2)).toBe(false); | ||
| expect(cleanupManager.list().some((cleanups) => cleanups.func === function3)).toBe(true); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,80 @@ | ||||||||||
| /** | ||||||||||
| * A class that manages and executes cleanup functions in code entrypoint. | ||||||||||
| */ | ||||||||||
| export class CleanupManager { | ||||||||||
| private cleanups: Array<Cleanup> = []; | ||||||||||
|
|
||||||||||
| constructor() {} | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Register a cleanup function waited to be called. | ||||||||||
| * @param func A function that does cleanup operation when called. | ||||||||||
| * @param pos A number (preferably defined by Enum) that indicates the position of the | ||||||||||
| * cleanup function. The lower the number, the earlier the function would be called. | ||||||||||
| */ | ||||||||||
| registerCleanupFunction(func: () => void, pos: number = -1): void { | ||||||||||
| if (this.cleanups.some((cleanup) => cleanup.func === func)) return; | ||||||||||
| this.cleanups.push({ | ||||||||||
| pos: pos, | ||||||||||
| func: func, | ||||||||||
| }); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Register a cleanup function, and return the function as-is. | ||||||||||
| * @param func | ||||||||||
| * @param pos | ||||||||||
| */ | ||||||||||
| registerCleanupFunctionAndReturnIt(func: () => void, pos: number = -1): () => void { | ||||||||||
| this.registerCleanupFunction(func, pos); | ||||||||||
| return func; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Remove any cleanup functions associated with the given position number. | ||||||||||
| * @param pos Position number for functions which will be removed. | ||||||||||
| */ | ||||||||||
| withdrawCleanupFunctionsByPositionNumber(pos: number): void { | ||||||||||
| this.cleanups = this.cleanups.filter((cleanup) => cleanup.pos != pos); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * [debug] return a readonly list containing stored cleanup functions. | ||||||||||
| */ | ||||||||||
| list(): Array<Cleanup> { | ||||||||||
| return [...this.cleanups] as const; | ||||||||||
|
Comment on lines
+44
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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:
💡 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:
Remove the incompatible
Proposed fix list(): Array<Cleanup> {
- return [...this.cleanups] as const;
+ return [...this.cleanups];
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Call all functions registered by `registerCleanupFunction` functions, | ||||||||||
| * and clear their references. | ||||||||||
| * | ||||||||||
| * If any cleanup functions throws an error, other functions will execute normally. | ||||||||||
| * Then, the last recorded error will be re-thrown. | ||||||||||
| */ | ||||||||||
| executeCleanups(): void { | ||||||||||
| let error: unknown = null; | ||||||||||
| let hasError = false; | ||||||||||
|
|
||||||||||
| this.cleanups | ||||||||||
| .sort((a, b) => { | ||||||||||
| return a.pos - b.pos; | ||||||||||
| }) | ||||||||||
| .forEach((it) => { | ||||||||||
| try { | ||||||||||
| it.func(); | ||||||||||
| } catch (e) { | ||||||||||
| error = e; | ||||||||||
| hasError = true; | ||||||||||
| } | ||||||||||
| }); | ||||||||||
| this.cleanups = []; | ||||||||||
|
|
||||||||||
| if (hasError) throw error; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| interface Cleanup { | ||||||||||
| pos: number; | ||||||||||
| func: () => void; | ||||||||||
| } | ||||||||||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: Nagi-ovo/voyager
Length of output: 154
🏁 Script executed:
Repository: Nagi-ovo/voyager
Length of output: 11255
🏁 Script executed:
Repository: Nagi-ovo/voyager
Length of output: 27105
🏁 Script executed:
Repository: Nagi-ovo/voyager
Length of output: 12966
🏁 Script executed:
Repository: Nagi-ovo/voyager
Length of output: 245
🏁 Script executed:
Repository: Nagi-ovo/voyager
Length of output: 11873
🏁 Script executed:
Repository: 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,CleanupAccountContextBridgewas registered beforeCleanupPluginHost, but positions18and14execute them in the opposite order. Reorder the enum or add tests for the intended dependency order.🤖 Prompt for AI Agents