mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-04-12 16:19:26 -07:00
fix: address PR #49 follow-up CodeRabbit review
This commit is contained in:
55
.github/workflows/prerelease.yml
vendored
55
.github/workflows/prerelease.yml
vendored
@@ -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
|
||||||
|
|||||||
@@ -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 -->
|
||||||
@@ -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/);
|
||||||
|
|||||||
@@ -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,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|||||||
@@ -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(
|
||||||
|
|||||||
@@ -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;
|
||||||
}
|
}
|
||||||
|
|
||||||
processCommandLineCache.set(pid, commandLine);
|
if (commandLine !== null) {
|
||||||
|
processCommandLineCache.set(pid, commandLine);
|
||||||
|
} else {
|
||||||
|
processCommandLineCache.delete(pid);
|
||||||
|
}
|
||||||
return commandLine;
|
return commandLine;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user