feat: transport-ship trail transition effect + animated store swatch#4455
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
WalkthroughAdds a ChangesTrail Transition Variant
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/client/components/EffectPreview.ts`:
- Around line 29-45: The EffectPreview swatch is using raw trail colors
directly, which can disagree with the renderer when invalid color strings are
present. Update EffectPreview.render() to normalize/filter this.trail?.colors
the same way the renderer logic does before building the CSS background, and
fall back to EMPTY_BG when no valid colors remain. Keep the logic in render()
aligned with the trail color handling used by
WebGLFrameBuilder.writeEffectEntry() so the preview matches the actual effect.
In `@src/core/CosmeticSchemas.ts`:
- Around line 121-124: The transition schema in CosmeticSchemas is currently
allowing negative frequency values, which can later break preview/render
behavior. Update the z.object validator for the transition type to reject
negative frequencies (or require strictly positive if zero should be disallowed)
so the contract matches TrailSwatch.updated() and trail.frag.glsl. Add a
regression test covering the transition schema validation to ensure invalid
negative frequency values are rejected.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85bb73bb-ae32-47d9-b6b1-e105b19b3e1a
⛔ Files ignored due to path filters (1)
src/client/render/gl/shaders/map-overlay/trail.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (4)
src/client/WebGLFrameBuilder.tssrc/client/components/EffectPreview.tssrc/core/CosmeticSchemas.tstests/CosmeticSchemas.test.ts
| render(): TemplateResult { | ||
| const colors = this.trail?.colors ?? []; | ||
| let background: string; | ||
| if (colors.length === 0) { | ||
| background = EMPTY_BG; | ||
| } else if (this.trail?.type === "transition") { | ||
| // The animation (see updated) cross-fades from here through the list. | ||
| background = colors[0]; | ||
| } else if (colors.length === 1) { | ||
| background = colors[0]; | ||
| } else { | ||
| background = `linear-gradient(90deg,${colors.join(",")})`; | ||
| } | ||
| return html`<div | ||
| class="w-full h-full rounded-md" | ||
| style="background:${background};" | ||
| ></div>`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter preview colors the same way the renderer does.
TransportShipTrailAttributesSchema allows raw strings, and WebGLFrameBuilder.writeEffectEntry() drops any color it can't parse. Here Lines 30-40 and Lines 55-66 use those raw values directly in CSS/WAAPI, so one bad entry can make the swatch blank or disagree with the actual trail. Normalize/filter first, then fall back to EMPTY_BG when nothing valid remains.
💡 Proposed fix
+import { colord } from "colord";
import { html, LitElement, TemplateResult } from "lit";
import { customElement, property } from "lit/decorators.js";
import { TransportShipTrailAttributes } from "../../core/CosmeticSchemas";
// Neutral fallback when a trail has no usable colors.
const EMPTY_BG = "`#444`";
+
+function getRenderableColors(
+ trail: TransportShipTrailAttributes | null,
+): string[] {
+ return (trail?.colors ?? []).filter((color) => colord(color).isValid());
+}
@@
render(): TemplateResult {
- const colors = this.trail?.colors ?? [];
+ const colors = getRenderableColors(this.trail);
@@
- const colors = attrs.colors;
+ const colors = getRenderableColors(attrs);
if (colors.length < 2 || attrs.frequency <= 0) return;Also applies to: 53-69
🤖 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/client/components/EffectPreview.ts` around lines 29 - 45, The
EffectPreview swatch is using raw trail colors directly, which can disagree with
the renderer when invalid color strings are present. Update
EffectPreview.render() to normalize/filter this.trail?.colors the same way the
renderer logic does before building the CSS background, and fall back to
EMPTY_BG when no valid colors remain. Keep the logic in render() aligned with
the trail color handling used by WebGLFrameBuilder.writeEffectEntry() so the
preview matches the actual effect.
| z.object({ | ||
| type: z.literal("transition"), | ||
| colors: z.array(z.string()), | ||
| frequency: z.number(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject negative transition frequencies.
Line 124 currently accepts negative values. TrailSwatch.updated() treats non-positive frequencies as “don’t animate”, but trail.frag.glsl still uses the encoded value in modulo math, so a negative catalog entry can preview as static and render with invalid color indexing. Validate this as non-negative here (or positive if zero should also be rejected), and add a regression test for that contract.
💡 Proposed fix
z.object({
type: z.literal("transition"),
colors: z.array(z.string()),
- frequency: z.number(),
+ frequency: z.number().nonnegative(),
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| z.object({ | |
| type: z.literal("transition"), | |
| colors: z.array(z.string()), | |
| frequency: z.number(), | |
| z.object({ | |
| type: z.literal("transition"), | |
| colors: z.array(z.string()), | |
| frequency: z.number().nonnegative(), |
🤖 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/core/CosmeticSchemas.ts` around lines 121 - 124, The transition schema in
CosmeticSchemas is currently allowing negative frequency values, which can later
break preview/render behavior. Update the z.object validator for the transition
type to reject negative frequencies (or require strictly positive if zero should
be disallowed) so the contract matches TrailSwatch.updated() and
trail.frag.glsl. Add a regression test covering the transition schema validation
to ensure invalid negative frequency values are rejected.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Use the <trail-swatch> element directly in CosmeticButton and EffectsInput and drop the thin wrapper; the named imports become side-effect imports so the element still registers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
1 participant
What
Adds a second transport-ship trail style, transition, alongside the existing gradient (#4454). Where
gradientpaints a spatial band of colors along the trail,transitionmakes the whole trail one color at a time, cross-fading through the color list over time.How
TransportShipTrailAttributesSchemais now a discriminated union ontype:gradient:{ colors, colorSize, movementSpeed }transition:{ colors, frequency }—frequency= color changes per second.styleIddiscriminator (row 1's alpha; 0 = gradient, 1 = transition), with the gradient scalars shifted down a row.styleId+ the style's scalars.transition, the trail color ismix(colors[i], colors[i+1], fract(t))withi = floor(uTime · frequency) mod count— one color step every1/frequencyseconds.<trail-swatch>Lit element. Fortransitionit cross-fades through the colors via the Web Animations API, timed to match the shader (each step1/frequencys); gradient/solid stay static. The animation is canceled on disconnect.Notes
gradientswatches remain static (they don't scroll like the in-game trail) — easy to add later if wanted.Testing
tsc --noEmit, ESLint, Prettier,build-prodall clean.frequency); 95 tests pass.🤖 Generated with Claude Code