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
|
||||
stats/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: |
|
||||
${{ runner.os }}-bun-
|
||||
${{ runner.os }}-${{ runner.arch }}-bun-
|
||||
|
||||
- name: Install dependencies
|
||||
run: |
|
||||
@@ -103,9 +103,9 @@ jobs:
|
||||
stats/node_modules
|
||||
vendor/texthooker-ui/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: |
|
||||
${{ runner.os }}-bun-
|
||||
${{ runner.os }}-${{ runner.arch }}-bun-
|
||||
|
||||
- name: Install dependencies
|
||||
run: |
|
||||
@@ -162,9 +162,9 @@ jobs:
|
||||
stats/node_modules
|
||||
vendor/texthooker-ui/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: |
|
||||
${{ runner.os }}-bun-
|
||||
${{ runner.os }}-${{ runner.arch }}-bun-
|
||||
|
||||
- name: Validate macOS signing/notarization secrets
|
||||
run: |
|
||||
@@ -238,9 +238,9 @@ jobs:
|
||||
stats/node_modules
|
||||
vendor/texthooker-ui/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: |
|
||||
${{ runner.os }}-bun-
|
||||
${{ runner.os }}-${{ runner.arch }}-bun-
|
||||
|
||||
- name: Install dependencies
|
||||
run: |
|
||||
@@ -306,9 +306,9 @@ jobs:
|
||||
path: |
|
||||
~/.bun/install/cache
|
||||
node_modules
|
||||
key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock') }}
|
||||
key: ${{ runner.os }}-${{ runner.arch }}-bun-${{ hashFiles('bun.lock') }}
|
||||
restore-keys: |
|
||||
${{ runner.os }}-bun-
|
||||
${{ runner.os }}-${{ runner.arch }}-bun-
|
||||
|
||||
- name: Install dependencies
|
||||
run: bun install --frozen-lockfile
|
||||
@@ -356,20 +356,6 @@ jobs:
|
||||
run: |
|
||||
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
|
||||
artifacts=(
|
||||
release/*.AppImage
|
||||
@@ -386,6 +372,27 @@ jobs:
|
||||
exit 1
|
||||
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
|
||||
gh release upload "${{ steps.version.outputs.VERSION }}" "$asset" --clobber
|
||||
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/);
|
||||
});
|
||||
|
||||
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', () => {
|
||||
assert.match(prereleaseWorkflow, /build-linux:/);
|
||||
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', () => {
|
||||
assert.doesNotMatch(prereleaseWorkflow, /aur-publish:/);
|
||||
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.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);
|
||||
return new RegExp(`--input-ipc-server(?:=|\\s+)("?${escapedSocketPath}"?)`, 'i').test(
|
||||
commandLine,
|
||||
);
|
||||
return new RegExp(
|
||||
`(?:^|\\s)--input-ipc-server(?:=|\\s+)(?:"${escapedSocketPath}"|${escapedSocketPath})(?=\\s|$)`,
|
||||
'i',
|
||||
).test(commandLine);
|
||||
}
|
||||
|
||||
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 {
|
||||
if (processCommandLineCache.has(pid)) {
|
||||
@@ -204,7 +204,11 @@ function getProcessCommandLineByPid(pid: number): string | null {
|
||||
commandLine = null;
|
||||
}
|
||||
|
||||
if (commandLine !== null) {
|
||||
processCommandLineCache.set(pid, commandLine);
|
||||
} else {
|
||||
processCommandLineCache.delete(pid);
|
||||
}
|
||||
return commandLine;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user