feat(plugins): a plugin world that can carry word-level lyrics - #654
Merged
Merged
Conversation
`waveflow:metadata/v1` could describe a line and an optional start time, which is what LRCLIB already gives away, so a provider holding per-word timing had to discard it to hand lyrics over. The host never called the function either — `runtime.rs` said so in a comment and nothing contradicted it. v2 carries a bundle instead: the document a provider served, verbatim, with its translations and pronunciation alongside. Documents rather than a lines/words record, because the host does not model lyrics as a structure at all — it stores content plus a format tag, and the line and word split lives in the renderer. A Rust model here would duplicate a parser that already exists and would still drop what it cannot express: per-word timing, singer attribution, background vocals, all of which survive untouched inside TTML. The host's whole trust in a plugin document is `detect_format`: re-sniff the content, refuse it unless it matches the declared format. No semantic parsing crosses into Rust. A bundle is cached and replaced as a unit. `lyrics` and `radio_lyrics` keep meaning "the lyrics currently chosen for this identity" and each gains a child table, rather than growing a key that would make every existing read ask which row is the primary one. The clearing lives in `upsert_lyrics` / `upsert_radio_lyrics` so every tier inherits it — embedded tag, sidecar, LRCLIB, the fallback chain, manual save, LRC import — and a Musixmatch original can never be served under a leftover Apple translation. Two things found on the way, both required for v2 to exist at all: Plugin enumeration matched worlds by PREFIX. `waveflow:metadata` would have matched a v2 plugin and instantiated it against v1 bindings, which is the one outcome a version label exists to prevent. Every call site now matches the exact label, and the UI constant's "and any future /v2" reasoning is corrected rather than carried forward. The world catalog had already drifted from the published registry schema: `waveflow:canvas/v1` is accepted by the host and absent from the enum, so a Canvas plugin would have been rejected at publication by a registry the host would have loaded happily. `worlds::ALL` is now the single source, `worlds.json` is checked against it by a test, and the registry generates its enum from that file instead of keeping a copy. The child tables carry no CHECK on `format` on purpose. SQLite cannot alter one in place, so adding `ttml` meant recreating `lyrics` wholesale in 20260516120000 — and with children cascading under `foreign_keys = ON` that same pattern would now erase every translation in silence. The host validates the value before writing it, which removes the only reason anyone would need to. Closes #585.
The harness ran ATTACH once through the pool, so it landed on whichever connection served that call and every later query answered 'no such table: app.lyrics'. ATTACH is per-connection; db::profile_db::open puts it in after_connect for that reason, and the test now does the same. Both databases are files for the same class of reason: a sqlite::memory: pool gives each connection its own empty database, so the profile schema would have gone missing one connection later.
The two that mattered were both the same shape — a second site that had to move with the first. Motion artwork enumerated `metadata/v1` only. v2 exports `album-info` exactly as v1 does, so the first plugin to migrate would have stopped serving animated covers, silently: a plugin that is never asked cannot report that it was not asked. And the plugin the world exists for, `apple-artwork`, is precisely the one due to migrate. Both worlds are enumerated now, each id carried with the world it declared, because instantiating a v2 component against v1 bindings is the one outcome a version label exists to prevent. `lyrics.provider` holds a plugin id in the plugin tier, and `refetch_lyrics` hands that column to `Provider::from_id`, which knows only the built-in network ids. Re-fetching lyrics a plugin had supplied would have failed as "unknown lyrics provider" — for a value this code wrote. Plugin provenance is namespaced with a `plugin:` prefix, and refetch re-runs that plugin instead, filtering the enumeration rather than trusting the stored id so an uninstalled one drops out on its own. Also: one spelling of the primary-row write, shared by both callers and taking the connection so each keeps its own transaction; the primary document's language is stored instead of a hardcoded NULL; and the UI world constant comes from the shared catalog rather than a local copy.
The insert is OR IGNORE, so a provider sending two documents for one (kind, language) slot has the second dropped by the database. The payload was built from the unfiltered list, so the panel would have shown both until the next reload and one after it, with nothing to explain the difference. The host now applies the same rule as the index -- first document wins, NULL and no-language are one slot -- and a test pins the payload to the row count.
A plugin id lands in LyricsPayload.provider, and the badge built lyrics.provider.${provider} from it. That key exists in none of the 17 locales and i18next renders a missing key as the key itself, so the badge would have read literally lyrics.provider.plugin:apple-lyrics. The prefix is recognised now and the plugin id shown bare -- it is what identifies the plugin in Settings, and a key per plugin is impossible for something installed at runtime.
InstaZDLL
added
scope: frontend
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (14)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughLa PR ajoute le monde ChangesPlugins metadata v2 et catalogue des mondes
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~90 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant LyricsCommand
participant PluginRuntime
participant MetadataV2Plugin
participant LyricsDatabase
LyricsCommand->>PluginRuntime: metadata_v2_lyrics(artist, title)
PluginRuntime->>MetadataV2Plugin: lyrics
MetadataV2Plugin-->>PluginRuntime: lyrics-bundle ou erreur
PluginRuntime-->>LyricsCommand: LyricsBundle ou None
LyricsCommand->>LyricsDatabase: Remplacer le document principal et les documents associés
LyricsDatabase-->>LyricsCommand: Payload mis en cache
Merge Risk: ⚪ Minimal · up to The metadata v2 lyrics path is compatible with existing TTML rendering, with no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
waveflow:metadata/v1could describe a line and an optional start time — which is what LRCLIB already gives away for free — so a provider holding per-word timing had to throw it away to hand lyrics over. The host never called the function either:runtime.rssaid so in a comment and nothing contradicted it.Closes #585.
What v2 carries
A bundle: the document a provider served, verbatim, with its translations and pronunciation alongside.
Documents rather than a lines/words record, because the host does not model lyrics as a structure at all. It stores
content: Stringplus aformattag, and the line-and-word split lives in TypeScript, in the renderer. A Rust model here would duplicate a parser that already exists and would still drop what it cannot express: per-word timing, per-line singer, background vocals — all of which survive untouched inside TTML.The host's whole trust in a plugin document is
detect_format: re-sniff the content, refuse it unless it matches the declared format. It is the routine the embedded-tag and sidecar tiers already rely on, and it is deliberately conservative. No semantic parsing crosses into Rust.A bundle is cached and replaced as a unit, so a Musixmatch original can never be served under a leftover Apple translation.
Why
lyricskeeps its shapelyricsandradio_lyricsmean "the lyrics currently chosen for this identity", and every existing read depends on that being structural rather than a query that has to ask which row is the primary one. Widening their key would have changed that property for the sake of a cardinality only one tier needs. Each gains a child table instead, with a real foreign key — one per parent rather than a shared polymorphic table, which could not have carried a foreign key to either.The clearing lives in
upsert_lyrics/upsert_radio_lyricsthemselves, so every tier inherits it — embedded tag, sidecar, LRCLIB, the fallback chain, manual save, LRC import. Putting it in the callers would have meant finding all six and hoping the seventh remembers.The child tables carry no CHECK on
format, unlike their parents. SQLite cannot alter a CHECK in place, so addingttmlmeant recreatinglyricswholesale in20260516120000— create, copy, drop, rename. With children cascading underforeign_keys = ON, repeating that pattern would now erase every translation in silence. The host validates the value before writing it, which removes the only reason anyone would need to.Two things this had to fix to exist at all
Plugin enumeration matched worlds by prefix.
"waveflow:metadata"would have matched a v2 plugin and instantiated it against v1 bindings — the one outcome a version label exists to prevent. Every call site now matches the exact label, and the UI constant's "and any future/v2" reasoning is corrected rather than carried forward.The world catalog had already drifted from the published registry schema.
waveflow:canvas/v1is accepted by the host and absent from the enum, so a Canvas plugin would have been rejected at publication by a registry the host would have loaded happily.worlds::ALLis the single source now,worlds.jsonis checked against it by a test, and the registry is meant to generate its enum from that file rather than keep a copy.Found by the local review, and worth naming
Three defects, none of them in the code being written — all in code that quietly read something whose nature had changed:
album-infoexactly as v1 does, so the first plugin to migrate would have stopped serving animated covers, silently. That plugin isapple-artwork— the one this world exists for.refetch_lyricshandedlyrics.providertoProvider::from_id. Re-fetching lyrics a plugin had supplied would have failed as "unknown lyrics provider", for a value this code wrote. Plugin provenance is namespacedplugin:now, and refetch re-runs that plugin, filtering the enumeration rather than trusting the stored id.lyrics.provider.${provider}. That key exists in none of the 17 locales, and i18next renders a missing key as the key itself, so the badge would have read literallylyrics.provider.plugin:apple-lyrics.Deliberately not here
The renderer does not display the associated documents. The payload carries them and the TypeScript type exists, but nothing draws them — because nothing produces them yet. The only planned source is the Apple Music plugin of #584, which lives in a separate repository and was waiting for this world. Building the UI now would be drawing against data no one can make.
The registry schema generation is in
waveflow-plugins, not here.worlds.jsonis in place and proven consistent with the catalog; the other repository has to start generating from it, orcanvas/v1— and nowmetadata/v2— stay unpublishable.Validation
Four local review passes: 8 findings, all valid, all fixed; the fourth pass is clean.
Linux (the only job that runs the app-crate tests): fmt clean, clippy
-D warningsat 0, 614 app + 319 core. Windows: fmt, clippy-D warningsacross the workspace, typecheck, eslint, prettier.Five tests added. The two that guard data were proven to fail without their fix, by breaking the code and watching them fall: removing the clearing from
upsert_lyricsdrops the bundle-invariant test, and neutralising the de-duplication drops the consistency test on 3 documents returned against 2 stored.Summary by CodeRabbit
Nouvelles fonctionnalités
Fiabilité