frontend Mod Controller Compatibility Info and Required Disclosures system#255
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
requires smr-api:1e106ce740e6695cedc04a6c7c2efe7df7521fe1
was breaking Edit Mod
need to decide what to do about the old api-only bool
…ription required when presented
…s and logic. fix old value lost when switching between dropdown options
There was a problem hiding this comment.
(claude-authored)
Thanks for tackling this — most of the PR reads well. Below are findings from a structured review (reuse / quality / efficiency lenses). Ordered roughly by severity.
🐛 Bugs / correctness
1. updateMod is missing from the graphcache updates.Mutation block — src/lib/core/graphql.ts
The block lists deleteMod/approveMod/denyMod/updateVersion etc., but not updateMod. The gap is pre-existing on staging, but this PR meaningfully widens its impact by making network_use_disclosure and ai_use_disclosure editable: after saving them in ModForm, the urql cache will keep serving the old Mod entity until a full reload. Suggest adding alongside the existing entries:
updateMod(_result, args, cache) {
cache.invalidate({ __typename: 'Mod', id: args.modId as string });
},2. iconText is computed once at component init, not reactive — src/lib/components/mods/compatibility/controller/ControllerCompatibilityIcon.svelte#L6-L9
let iconText = 'videogame_asset';
if (compatibility?.state === ControllerCompatibilityState.Unsupported) {
iconText = 'videogame_asset_off';
}The class bindings below are reactive, but iconText runs once. If compatibility changes during edit (and that's exactly the new use case — bind: on the state in ControllerCompatibilityEdit), the icon won't switch from videogame_asset to videogame_asset_off. The existing branch/CompatibilityIcon.svelte depends only on the EXP prop which is fixed per-instance, so this pattern was safe there but isn't here.
$: iconText = compatibility?.state === ControllerCompatibilityState.Unsupported
? 'videogame_asset_off'
: 'videogame_asset';3. Debug console.log shipping to production
src/lib/components/mods/ModForm.svelte—console.log('DEBUG: ModForm Errors', e). The comment says "Intentionally kept in case we run into some weird edge cases," but it fires on every validation error for every user. Either drop it or gate it behindimport.meta.env.DEV.src/lib/utils/uplugin.ts#L60—window.console.log('Invalid json:', e). Drop thewindow.prefix (it's a no-op in browsers and breaks under SSR if this ever executes server-side), and considerconsole.errorso it shows up in error filters.
✂️ Reuse / duplication
4. ControllerCompatibility* components are near-duplicates of branch/Compatibility*
ControllerCompatibilityStateText≡branch/CompatibilityStateText(state → text →classForState→<p class="… mod-state">), differing only in enum + i18n key prefix.ControllerCompatibilityEdit≡branch/CompatibilityEdit(label/select/description/textarea), differing only in enum + description fn + label.- The new
mod-controller-state-*classes in_global.postcsshave identical colors to the existingmod-state-*rules — they could be deleted and the existing classes reused via a small state-equivalence mapping (the class bindings inControllerCompatibilityIconalready imply that mapping: Implicit/Supported → works, Partial → damaged, Unsupported → broken, Untested/null → unknown).
A single parameterized <CompatibilityStateText prefix={…} state={…}/> + <CompatibilityEdit states={…} descriptionFn={…} label={…}/> covers both branch and controller and would remove most of the controller folder.
🎨 Code quality
5. Leaky bind:dropdownChoiceForValidation — ModNetworkDisclosureEdit.svelte exports an internal dropdown state purely so ModForm can validate. The child's UI state shouldn't have to escape — either keep validation inside the child, expose a typed event, or compute the validation input from disclosure itself.
6. IIFE inside the Svelte template — ModForm.svelte ~line 235-252 wraps the error-message branching in (() => { ... })(). Lift it into a small helper (formatDisclosureError(message)) or a $: derived var — easier to read and unit-test.
7. Stringly-typed map — src/lib/utils/compatibility-descriptions.ts
export const compatibilityStateDescriptions: {
// TODO switch to Record, but Unknown isn't a valid db type
[key: string]: string;
} = { Works: …, Damaged: …, Broken: …, Unknown: … };The TODO is honest, but controllerCompatibilityStateDescriptions two lines below uses Record<ControllerCompatibilityState, string> cleanly. For the branch one you can use Record<CompatibilityState, string> & { Unknown: string } (or a small string-union of CompatibilityState | 'Unknown') and keep type safety on the real enum values.
8. <!-- @ts-ignore --> — ModAiDisclosureEdit.svelte line 45. The state in {#each dropdown_options as state} is AiUseDisclosureType and optionTranslationKeys[state] returns a known string from a Record, so this should be resolvable by typing the array more precisely (it's already as Array<AiUseDisclosureType> — confirm Tolgee's T component is happy with the resulting string, or extract to a typed const).
9. Generic aria-label on the new controller table — CompatibilityGrid.svelte. The existing table was correctly relabeled to "Branch Compatibility Information" in this PR, but the new table inherited the now-misleading "Compatibility Information" label. Suggest "Controller Compatibility Information".
10. Dangling <br /> — ControllerCompatibilityEdit.svelte line ~28. The PR explicitly removes the same <br /> from branch/CompatibilityEdit.svelte, but the new controller copy was written with the old style. Margin-bottom on the label/select handles spacing already.
Nice-to-haves (lower confidence)
- The controller table is rendered as a second
<table>element. It currently has only one column, so unifying it with the branch table isn't trivial; if you ever add a second controller variant it might be worth revisiting. errors.subscribe(...)inModForm.sveltedoesn't capture/cleanup the unsubscribe handle. Felte's store is component-scoped so it's likely fine, but using$errorsreactively would auto-cleanup and avoid the question.
Happy to discuss / push fixes if useful.
|
Addressed 1 (actual behavior untested), 2, 5 (by adding comment), 7, 8 (no longer needed with refactor), 9, 10 Skipped 3The purpose is for it to be there on production so we don't need to push special builds that let people see errors in the future when reporting them Skipped 4The refactor is not worth the effort right now, and using the prefix system makes it harder to find i18n key usage in the code Skipped 6Yes, the error display is ugly, but better it be right where the error gets displayed when nothing else in the file cares about it |
…ilently requesting no change
|
@claude review |
CI will fail until backed is merged into staging
Frontend side of satisfactorymodding/smr-api#138
ToggleNetworkUseremoved