fix: address PR #49 follow-up CodeRabbit review

This commit is contained in:
2026-04-11 16:31:41 -07:00
parent 9ce5de2f22
commit 62ad77dcfd
6 changed files with 138 additions and 29 deletions

View File

@@ -32,9 +32,9 @@ jobs:
node_modules node_modules
stats/node_modules stats/node_modules
vendor/subminer-yomitan/node_modules vendor/subminer-yomitan/node_modules
key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/subminer-yomitan/package-lock.json') }} key: ${{ runner.os }}-${{ runner.arch }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/subminer-yomitan/package-lock.json') }}
restore-keys: | restore-keys: |
${{ runner.os }}-bun- ${{ runner.os }}-${{ runner.arch }}-bun-
- name: Install dependencies - name: Install dependencies
run: | run: |
@@ -103,9 +103,9 @@ jobs:
stats/node_modules stats/node_modules
vendor/texthooker-ui/node_modules vendor/texthooker-ui/node_modules
vendor/subminer-yomitan/node_modules vendor/subminer-yomitan/node_modules
key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/texthooker-ui/package.json', 'vendor/subminer-yomitan/package-lock.json') }} key: ${{ runner.os }}-${{ runner.arch }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/texthooker-ui/package.json', 'vendor/subminer-yomitan/package-lock.json') }}
restore-keys: | restore-keys: |
${{ runner.os }}-bun- ${{ runner.os }}-${{ runner.arch }}-bun-
- name: Install dependencies - name: Install dependencies
run: | run: |
@@ -162,9 +162,9 @@ jobs:
stats/node_modules stats/node_modules
vendor/texthooker-ui/node_modules vendor/texthooker-ui/node_modules
vendor/subminer-yomitan/node_modules vendor/subminer-yomitan/node_modules
key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/texthooker-ui/package.json', 'vendor/subminer-yomitan/package-lock.json') }} key: ${{ runner.os }}-${{ runner.arch }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/texthooker-ui/package.json', 'vendor/subminer-yomitan/package-lock.json') }}
restore-keys: | restore-keys: |
${{ runner.os }}-bun- ${{ runner.os }}-${{ runner.arch }}-bun-
- name: Validate macOS signing/notarization secrets - name: Validate macOS signing/notarization secrets
run: | run: |
@@ -238,9 +238,9 @@ jobs:
stats/node_modules stats/node_modules
vendor/texthooker-ui/node_modules vendor/texthooker-ui/node_modules
vendor/subminer-yomitan/node_modules vendor/subminer-yomitan/node_modules
key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/texthooker-ui/package.json', 'vendor/subminer-yomitan/package-lock.json') }} key: ${{ runner.os }}-${{ runner.arch }}-bun-${{ hashFiles('bun.lock', 'stats/bun.lock', 'vendor/texthooker-ui/package.json', 'vendor/subminer-yomitan/package-lock.json') }}
restore-keys: | restore-keys: |
${{ runner.os }}-bun- ${{ runner.os }}-${{ runner.arch }}-bun-
- name: Install dependencies - name: Install dependencies
run: | run: |
@@ -306,9 +306,9 @@ jobs:
path: | path: |
~/.bun/install/cache ~/.bun/install/cache
node_modules node_modules
key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock') }} key: ${{ runner.os }}-${{ runner.arch }}-bun-${{ hashFiles('bun.lock') }}
restore-keys: | restore-keys: |
${{ runner.os }}-bun- ${{ runner.os }}-${{ runner.arch }}-bun-
- name: Install dependencies - name: Install dependencies
run: bun install --frozen-lockfile run: bun install --frozen-lockfile
@@ -356,20 +356,6 @@ jobs:
run: | run: |
set -euo pipefail set -euo pipefail
if gh release view "${{ steps.version.outputs.VERSION }}" >/dev/null 2>&1; then
gh release edit "${{ steps.version.outputs.VERSION }}" \
--draft=false \
--prerelease \
--title "${{ steps.version.outputs.VERSION }}" \
--notes-file release/prerelease-notes.md
else
gh release create "${{ steps.version.outputs.VERSION }}" \
--latest=false \
--prerelease \
--title "${{ steps.version.outputs.VERSION }}" \
--notes-file release/prerelease-notes.md
fi
shopt -s nullglob shopt -s nullglob
artifacts=( artifacts=(
release/*.AppImage release/*.AppImage
@@ -386,6 +372,27 @@ jobs:
exit 1 exit 1
fi fi
if gh release view "${{ steps.version.outputs.VERSION }}" >/dev/null 2>&1; then
gh release edit "${{ steps.version.outputs.VERSION }}" \
--draft \
--prerelease \
--title "${{ steps.version.outputs.VERSION }}" \
--notes-file release/prerelease-notes.md
else
gh release create "${{ steps.version.outputs.VERSION }}" \
--draft \
--latest=false \
--prerelease \
--title "${{ steps.version.outputs.VERSION }}" \
--notes-file release/prerelease-notes.md
fi
for asset in "${artifacts[@]}"; do for asset in "${artifacts[@]}"; do
gh release upload "${{ steps.version.outputs.VERSION }}" "$asset" --clobber gh release upload "${{ steps.version.outputs.VERSION }}" "$asset" --clobber
done done
gh release edit "${{ steps.version.outputs.VERSION }}" \
--draft=false \
--prerelease \
--title "${{ steps.version.outputs.VERSION }}" \
--notes-file release/prerelease-notes.md

View File

@@ -0,0 +1,61 @@
---
id: TASK-286.1
title: 'Assess and address PR #49 subsequent CodeRabbit review round'
status: Done
assignee: []
created_date: '2026-04-11 23:14'
updated_date: '2026-04-11 23:16'
labels:
- bug
- code-review
- windows
- release
dependencies: []
references:
- .github/workflows/prerelease.yml
- src/window-trackers/mpv-socket-match.ts
- src/window-trackers/win32.ts
- src/core/services/overlay-shortcut.ts
parent_task_id: TASK-286
priority: high
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
Track the next unresolved CodeRabbit review threads on PR #49 after commit 9ce5de2f and resolve the still-valid follow-up issues without reopening already-assessed stale comments.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 All still-actionable CodeRabbit comments in the latest PR #49 round are fixed or explicitly shown stale with evidence.
- [x] #2 Regression coverage is added or updated for any behavior-sensitive fix in workflow or Windows socket matching.
- [x] #3 Relevant verification passes for the touched workflow, tracker, and shared matcher changes.
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
1. Verify the five unresolved CodeRabbit threads against current branch state and separate still-valid bugs from stale comments.
2. Add or extend the narrowest failing tests for exact socket-path matching and prerelease workflow invariants before changing production code.
3. Implement minimal fixes in the prerelease workflow and Windows socket matching/cache path, leaving stale comments documented with evidence instead of forcing no-op changes.
4. Run targeted verification, then record the fixed-vs-stale assessment and close the subtask.
<!-- SECTION:PLAN:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
Assessed five unresolved PR #49 threads after 9ce5de2f. Fixed prerelease workflow cache keys to include `runner.arch`, changed prerelease publishing to validate artifacts before release creation/edit and only undraft after uploads complete, tightened Windows socket matching to require exact argument boundaries, and stopped memoizing null command-line lookup misses in the Win32 cache path.
Stale assessment: the `src/core/services/overlay-shortcut.ts` thread is still obsolete. Current code at `registerOverlayShortcuts()` returns `hasConfiguredOverlayShortcuts(shortcuts)`, not `false`, and the overlay-local handling remains intentionally driven by local fallback dispatch rather than global registration in this runtime path.
Verification: `bun test src/prerelease-workflow.test.ts src/window-trackers/mpv-socket-match.test.ts`, `bun test src/window-trackers/windows-tracker.test.ts src/prerelease-workflow.test.ts src/window-trackers/mpv-socket-match.test.ts`, `bun run typecheck`.
<!-- SECTION:NOTES:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
Handled the next CodeRabbit round on PR #49 by fixing the still-valid prerelease workflow and Windows socket-matching issues while documenting the stale overlay-shortcut comment instead of forcing a no-op code change. The prerelease workflow now scopes all dependency caches by `runner.arch`, validates the final artifact set before touching the GitHub release, creates/edits the prerelease as a draft during uploads, and only flips `--draft=false` after all assets succeed. On Windows, socket matching now requires an exact `--input-ipc-server` argument boundary so `subminer-1` no longer matches `subminer-10`, and transient PowerShell/CIM misses no longer get cached forever as null command lines.
Regression coverage was added for the workflow invariants and exact socket matching. Verification passed with targeted prerelease workflow tests, Windows tracker tests, socket-matcher tests, and `bun run typecheck`.
<!-- SECTION:FINAL_SUMMARY:END -->

View File

@@ -34,6 +34,17 @@ test('prerelease workflow publishes GitHub prereleases and keeps them off latest
assert.match(prereleaseWorkflow, /gh release create[\s\S]*--latest=false/); assert.match(prereleaseWorkflow, /gh release create[\s\S]*--latest=false/);
}); });
test('prerelease workflow scopes dependency caches by runner architecture', () => {
const archScopedCacheKeyMatches = prereleaseWorkflow.match(
/key:\s*\${{\s*runner\.os\s*}}-\${{\s*runner\.arch\s*}}-bun-/g,
);
const archScopedRestoreKeyMatches = prereleaseWorkflow.match(
/\${{\s*runner\.os\s*}}-\${{\s*runner\.arch\s*}}-bun-/g,
);
assert.equal(archScopedCacheKeyMatches?.length, 5);
assert.ok((archScopedRestoreKeyMatches?.length ?? 0) >= 10);
});
test('prerelease workflow builds and uploads all release platforms', () => { test('prerelease workflow builds and uploads all release platforms', () => {
assert.match(prereleaseWorkflow, /build-linux:/); assert.match(prereleaseWorkflow, /build-linux:/);
assert.match(prereleaseWorkflow, /build-macos:/); assert.match(prereleaseWorkflow, /build-macos:/);
@@ -54,6 +65,21 @@ test('prerelease workflow publishes the same release assets as the stable workfl
); );
}); });
test('prerelease workflow validates artifacts before publishing the release and only undrafts after upload', () => {
const artifactsIndex = prereleaseWorkflow.indexOf('artifacts=(');
const createIndex = prereleaseWorkflow.indexOf('gh release create');
const uploadIndex = prereleaseWorkflow.indexOf('gh release upload');
const undraftIndex = prereleaseWorkflow.indexOf('--draft=false');
assert.notEqual(artifactsIndex, -1);
assert.notEqual(createIndex, -1);
assert.notEqual(uploadIndex, -1);
assert.notEqual(undraftIndex, -1);
assert.ok(artifactsIndex < createIndex);
assert.ok(uploadIndex < undraftIndex);
assert.match(prereleaseWorkflow, /gh release create[\s\S]*--draft[\s\S]*--prerelease/);
});
test('prerelease workflow does not publish to AUR', () => { test('prerelease workflow does not publish to AUR', () => {
assert.doesNotMatch(prereleaseWorkflow, /aur-publish:/); assert.doesNotMatch(prereleaseWorkflow, /aur-publish:/);
assert.doesNotMatch(prereleaseWorkflow, /AUR_SSH_PRIVATE_KEY/); assert.doesNotMatch(prereleaseWorkflow, /AUR_SSH_PRIVATE_KEY/);

View File

@@ -54,3 +54,13 @@ test('filterMpvPollResultBySocketPath keeps only matches for the requested socke
assert.deepEqual(result.matches.map((match) => match.hwnd), [2]); assert.deepEqual(result.matches.map((match) => match.hwnd), [2]);
assert.equal(result.windowState, 'visible'); assert.equal(result.windowState, 'visible');
}); });
test('matchesMpvSocketPathInCommandLine rejects socket-path prefix matches', () => {
assert.equal(
matchesMpvSocketPathInCommandLine(
'mpv.exe --input-ipc-server=\\\\.\\pipe\\subminer-10 video.mkv',
'\\\\.\\pipe\\subminer-1',
),
false,
);
});

View File

@@ -13,9 +13,10 @@ export function matchesMpvSocketPathInCommandLine(
} }
const escapedSocketPath = escapeRegex(targetSocketPath); const escapedSocketPath = escapeRegex(targetSocketPath);
return new RegExp(`--input-ipc-server(?:=|\\s+)("?${escapedSocketPath}"?)`, 'i').test( return new RegExp(
commandLine, `(?:^|\\s)--input-ipc-server(?:=|\\s+)(?:"${escapedSocketPath}"|${escapedSocketPath})(?=\\s|$)`,
); 'i',
).test(commandLine);
} }
export function filterMpvPollResultBySocketPath( export function filterMpvPollResultBySocketPath(

View File

@@ -173,7 +173,7 @@ function getProcessNameByPid(pid: number): string | null {
} }
} }
const processCommandLineCache = new Map<number, string | null>(); const processCommandLineCache = new Map<number, string>();
function getProcessCommandLineByPid(pid: number): string | null { function getProcessCommandLineByPid(pid: number): string | null {
if (processCommandLineCache.has(pid)) { if (processCommandLineCache.has(pid)) {
@@ -204,7 +204,11 @@ function getProcessCommandLineByPid(pid: number): string | null {
commandLine = null; commandLine = null;
} }
if (commandLine !== null) {
processCommandLineCache.set(pid, commandLine); processCommandLineCache.set(pid, commandLine);
} else {
processCommandLineCache.delete(pid);
}
return commandLine; return commandLine;
} }