feat(editor): export a track to Guitar Pro (.gp5) - #244
Conversation
The editor had no export OUT — you could import from GP/MIDI/XML/sloppak but never take a chart back out. Add File ▸ Export ▸ Guitar Pro (.gp5), which downloads the current fretted track as a .gp5 file. It reuses the exact bytes the read-only Tab preview already engraves: the Tab View plugin's GP5 conversion of the last-saved pack, fetched from the same endpoint. The endpoint format (_tabPreviewUrlPure) is now exported from tab-preview.js and imported here, so the tabview contract lives in one place and the two surfaces can't drift. New src/gp5-export.js owns the download plus its own pure, export-worded helpers: a fretted/saved guard (keys/drums have no tab; the converter reads the saved pack), the download filename (pack-extension drop + track name + cross-OS sanitise), and the honest 404/501 failure messages. Wired as a File command (shortcuts.js registry + _editorRunEofCommand case + menu-bar item), mirroring the sibling Tab preview command; also exposed on window. Tests: tests/gp5_export.test.mjs (11) — guard truth table, filename cases (extension drop, no-part, missing-filename, illegal-char sanitise), failure messages, and that the export fetches the SAME tabview endpoint the preview does. Live-verified on AC/DC with the Tab View plugin loaded: File▸Export downloads a real 17.8KB "…— Lead.gp5" (valid GP5 header), status confirms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds Guitar Pro ChangesGuitar Pro export
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant FileMenu
participant CommandDispatcher
participant editorExportGp5
participant TabViewEndpoint
participant Browser
Editor->>FileMenu: Select Export GP5
FileMenu->>CommandDispatcher: Dispatch exportGp5
CommandDispatcher->>editorExportGp5: Invoke export
editorExportGp5->>TabViewEndpoint: Request GP5 conversion
TabViewEndpoint-->>editorExportGp5: Return GP5 bytes
editorExportGp5->>Browser: Download .gp5 file
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gp5-export.js (1)
86-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a fetch timeout for the conversion request.
If the Tab View plugin backend hangs,
editorExportGp5has no way to recover — the "Exporting …" status persists indefinitely with no user-facing timeout or cancel path.♻️ Proposed fix: bound the request with an AbortController
- const resp = await fetch(_tabPreviewUrlPure(S.filename, S.currentArr, Date.now())); + const ctrl = new AbortController(); + const timeoutId = setTimeout(() => ctrl.abort(), 15000); + let resp; + try { + resp = await fetch(_tabPreviewUrlPure(S.filename, S.currentArr, Date.now()), { signal: ctrl.signal }); + } finally { + clearTimeout(timeoutId); + }🤖 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/gp5-export.js` around lines 86 - 92, Update the conversion request in editorExportGp5 to use an AbortController with a finite timeout, pass its signal to fetch, and clear the timeout when the request settles. Handle an abort as a user-facing export failure/status update rather than leaving “Exporting …” indefinitely, while preserving the existing non-OK response handling.
🤖 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.
Nitpick comments:
In `@src/gp5-export.js`:
- Around line 86-92: Update the conversion request in editorExportGp5 to use an
AbortController with a finite timeout, pass its signal to fetch, and clear the
timeout when the request settles. Handle an abort as a user-facing export
failure/status update rather than leaving “Exporting …” indefinitely, while
preserving the existing non-OK response handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90c39143-8048-4d81-b367-035987c012d0
📒 Files selected for processing (8)
CHANGELOG.mdsrc/gp5-export.jssrc/input.jssrc/main.jssrc/menu-bar.jssrc/shortcuts.jssrc/tab-preview.jstests/gp5_export.test.mjs
The tabview converter reads the SAVED pack and indexes it by S.currentArr, clamping that index into the saved track list. So exporting mid-edit didn't just drop unsaved notes: with an added or reordered track the clamp handed back a DIFFERENT track's bytes under the requested track's name. Route the export through the session's own guardSessionTransition prompt (Save / Don't Save / Cancel) and re-check the guards after that await — the song can close while the dialog is up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/gp5-export.js (1)
89-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInclude the transition guard in the error boundary.
The
tryblock begins afterawait guardSessionTransition(...). If the prompt orhost.saveSession()rejects,editorExportGp5()produces an unhandled rejection and no failure status. Move thetryaround the entire orchestration or catch this await separately.🤖 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/gp5-export.js` around lines 89 - 99, Update editorExportGp5 so the await guardSessionTransition call and subsequent session-transition orchestration are covered by the existing error boundary. Ensure rejections from the prompt or host.saveSession are caught and produce the same failure status handling as export errors, while preserving the cancellation return path.
🧹 Nitpick comments (1)
tests/gp5_export.test.mjs (1)
109-172: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the pre-prompt short-circuit.
The three orchestrator tests cover cancel-aborts, prompt-precedes-fetch, and guard-recheck-after-prompt, but none verify that when the initial guard already fails (e.g. a never-saved song with no
filename,S.filename === ''),guardSessionTransitionis never invoked at all. Per the src control flow shown insrc/gp5-export.js:19-115, the pre-check runs and returns before the prompt is ever awaited:if (!guard.ok) { setStatus(guard.reason); return; }. This orchestration-level short-circuit isn't protected by the pure_gp5ExportGuardPuretruth table (lines 31-57), which only tests the guard function in isolation — a future refactor could reorder the checks to always prompt first without any test catching it.✅ Suggested additional test
await ta('never-saved session: the guard fails before the save prompt is ever shown', async () => { const env = exportEnv({ S: { arrangements: [], currentArr: 0, filename: '' } }); env.guardSessionTransition = async () => { throw new Error('prompt must not run when the initial guard already fails'); }; await env.run(); assert.deepStrictEqual(env.fetched, []); assert.deepStrictEqual(env.downloads, []); assert.match(env.statuses.join(' '), /Load a song first/); });🤖 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 `@tests/gp5_export.test.mjs` around lines 109 - 172, Add an orchestrator-level test alongside the existing editorExportGp5 tests for an unsaved session with empty arrangements and filename. Make guardSessionTransition throw if invoked, run the export, and assert no fetches or downloads occur while statuses report “Load a song first,” verifying the initial guard short-circuits before prompting.
🤖 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/gp5-export.js`:
- Around line 89-96: Update the export flow around guardSessionTransition so it
exposes the transition choice and continues only when the result is save; treat
discard as export cancellation. Ensure arr, guard, and subsequent conversion
logic execute only after a successful save, preserving the existing cancellation
status behavior.
---
Outside diff comments:
In `@src/gp5-export.js`:
- Around line 89-99: Update editorExportGp5 so the await guardSessionTransition
call and subsequent session-transition orchestration are covered by the existing
error boundary. Ensure rejections from the prompt or host.saveSession are caught
and produce the same failure status handling as export errors, while preserving
the cancellation return path.
---
Nitpick comments:
In `@tests/gp5_export.test.mjs`:
- Around line 109-172: Add an orchestrator-level test alongside the existing
editorExportGp5 tests for an unsaved session with empty arrangements and
filename. Make guardSessionTransition throw if invoked, run the export, and
assert no fetches or downloads occur while statuses report “Load a song first,”
verifying the initial guard short-circuits before prompting.
🪄 Autofix (Beta)
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: 5d0b1833-a8d7-4024-9637-4af2b295420f
📒 Files selected for processing (4)
CHANGELOG.mdnode_modulessrc/gp5-export.jstests/gp5_export.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| if (!(await guardSessionTransition('exporting to Guitar Pro'))) { | ||
| setStatus('Export cancelled.'); | ||
| return; | ||
| } | ||
| // The prompt awaited: the song (and the current part) may have moved. | ||
| arr = cur(); | ||
| guard = _gp5ExportGuardPure(S.filename, arr && arr.name, !!S.arrangements.length); | ||
| if (!guard.ok) { setStatus(guard.reason); return; } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not continue after “Don’t Save” for this export.
guardSessionTransition returns true for both discard and successful save. The export can therefore use unsaved S metadata/current index while the converter reads the saved pack, exporting stale or even another track’s bytes under the current filename. Expose the transition choice and require save, or cancel export after discard.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 89-89: React's useState should not be directly called
Context: setStatus('Export cancelled.')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[error] 95-95: React's useState should not be directly called
Context: setStatus(guard.reason)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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/gp5-export.js` around lines 89 - 96, Update the export flow around
guardSessionTransition so it exposes the transition choice and continues only
when the result is save; treat discard as export cancellation. Ensure arr,
guard, and subsequent conversion logic execute only after a successful save,
preserving the existing cancellation status behavior.
# Conflicts: # CHANGELOG.md
2 participants
What & why
The editor could import from Guitar Pro / MIDI / XML / sloppak but had no export out — no way to take a chart back into another tool. Gap-audit #4. This adds File ▸ Export ▸ Guitar Pro (.gp5), which downloads the current fretted track as a
.gp5file.Approach
It reuses the exact bytes the read-only Tab preview already engraves: the Tab View plugin's GP5 conversion of the last-saved pack, fetched from the same endpoint.
_tabPreviewUrlPureis now exported fromtab-preview.jsand imported here, so the tabview conversion contract lives in one place — the two surfaces can't drift.New
src/gp5-export.jsowns the browser download plus its own pure, export-worded helpers:_gp5ExportGuardPure— fretted-only (keys/drums have no tab), and a saved pack is required (the converter reads the last-saved pack)._gp5ExportNamePure— the download filename: pack-extension drop + track name + cross-OS illegal-char sanitise._gp5ExportHttpMessagePure— honest 404 (Tab View plugin missing / unsaved) and 501 (host too old) messages.Wired as a proper File command (shortcuts.js registry entry +
_editorRunEofCommandcase + menu-bar item), mirroring the sibling Tab-preview command; also exposed onwindowfor parity.Tests
tests/gp5_export.test.mjs(11) — guard truth table (no song / keys / piano / synth / drums / unsaved / fretted-OK), filename cases (extension drop incl. case-insensitive, no-part, missing-filename →track, illegal-char sanitise + whitespace collapse), the failure messages, and that export fetches the same tabview GP5 URL the preview does.routes.pyuntouched (no pytest).PyGuitarProdep already in the testbed venv):File ▸ Exporttriggers a real browser download namedAC DC - Back In Black - Back In Black — Lead.gp5, 17,878 bytes with a validFICHIER GUITAR PRO v5.1header, and the status line confirms "Exported …".🤖 Generated with Claude Code
Summary by CodeRabbit
.gp5file.