Feat(components): Add 5 new OpenBridge components (Speedometer, True-relative, Long-lat (two versions), GNSS Skyplot) - #944
Open
asijah2 wants to merge 25 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds shared skipper instrument types and helpers, new LitElement instruments for speed, long-lat, skyplot, true-relative, and vessel views, plus Storybook coverage and barrel exports. Also updates a devcontainer lockfile and accordion-card spacing. ChangesSkipper instruments and supporting infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
packages/openbridge-webcomponents/src/skipper/tickmark.ts-70-70 (1)
70-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove inline comment in TS source.
Line 70 adds an inline comment in a
.tsmodule where comments are disallowed by repo rules.As per coding guidelines, "
**/*.{js,ts,tsx,jsx}: Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation`."🤖 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 `@packages/openbridge-webcomponents/src/skipper/tickmark.ts` at line 70, Remove the inline comment "// check if scale is not infinite" from the TypeScript module in packages/openbridge-webcomponents/src/skipper/tickmark.ts; locate that exact comment text in the tickmark.ts source and delete the comment line so the file contains no inline comments (do not replace with other comments or disable rules).packages/openbridge-webcomponents/src/skipper/tickmark.ts-71-73 (1)
71-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
NaNinscalevalidation.Line 71 rejects
Infinityand non-positive values, butNaNstill passes and propagates invalid coordinates into SVG output.Suggested fix
- if (scale === Infinity || scale <= 0) { + if (!Number.isFinite(scale) || scale <= 0) { throw new Error('Scale is not valid'); }🤖 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 `@packages/openbridge-webcomponents/src/skipper/tickmark.ts` around lines 71 - 73, The scale validation in tickmark.ts currently only rejects Infinity and non-positive numbers but allows NaN; update the check around the variable scale (in the function where scale is validated) to use Number.isFinite(scale) && scale > 0 (or equivalently !Number.isNaN(scale) and scale !== Infinity and scale > 0) and throw the same Error('Scale is not valid') when it fails so NaN no longer propagates into SVG coordinates.packages/openbridge-webcomponents/src/skipper/advice.ts-66-68 (1)
66-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove commented-out code blocks from this TS module.
These lines leave dead/commented implementation fragments in production source.
As per coding guidelines, "
**/*.{js,ts,tsx,jsx}: Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation`."Also applies to: 85-87, 150-160, 171-172
🤖 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 `@packages/openbridge-webcomponents/src/skipper/advice.ts` around lines 66 - 68, Remove the dead commented-out implementation fragments in the skipper/advice.ts module (specifically the commented ternary assignments for mainColor and fillColor that reference AdviceType.caution/warning/alarm and the other commented blocks at the other noted locations); delete those commented lines entirely so the file contains no leftover commented code, leaving only active code and, if necessary, refactor any remaining references to the variables mainColor/fillColor or AdviceType to use the existing active implementation paths to keep builds/tests passing.packages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.stories.ts-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Title Case for the story title path.
INSTRUMENT/LongLatEnvironmentalis not Title Case and is likely to fail the story title lint rule. Use a Title Case path (for example,Instrument/Long Lat Environmental).As per coding guidelines, “Storybook story
titleandnamefields must use Title Case — enforced by ESLint ruleopenbridge/storybook-title-case(auto-fixable)”.🤖 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 `@packages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.stories.ts` at line 7, The story title string currently uses "INSTRUMENT/LongLatEnvironmental" which violates the Title Case rule; update the exported story's title value to a Title Case path such as "Instrument/Long Lat Environmental" (or similar Title Case segmentation) so it satisfies the openbridge/storybook-title-case lint rule and Storybook conventions.packages/openbridge-webcomponents/src/skipper/true-relative/true-relative-styles.ts-20-23 (1)
20-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd unit to
font-sizevalue.Line 22 has
font-size: 64without a unit, which is invalid CSS (except forline-height). It should befont-size: 64pxor another appropriate unit.🐛 Proposed fix
.speed-gauge-value { position: relative; - font-size: 64; + font-size: 64px; }🤖 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 `@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative-styles.ts` around lines 20 - 23, The CSS rule for the .speed-gauge-value selector in true-relative-styles.ts uses an invalid unitless font-size ("font-size: 64"); update this to a valid CSS size such as "64px" (or another appropriate unit like rem/em) so the rule reads font-size: 64px; and ensure any other occurrences in true-relative-styles.ts follow the same change.packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.stories.ts-8-8 (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Title Case for Storybook title.
The title
'INSTRUMENT/TrueRelative'should use Title Case with proper spacing:'INSTRUMENT/True Relative'.As per coding guidelines: "Storybook story
titleandnamefields must use Title Case — enforced by ESLint ruleopenbridge/storybook-title-case(auto-fixable)".📝 Proposed fix
- title: 'INSTRUMENT/TrueRelative', + title: 'INSTRUMENT/True Relative',🤖 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 `@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.stories.ts` at line 8, The Storybook story title currently uses 'INSTRUMENT/TrueRelative' — update the title property to use Title Case with proper spacing, e.g. 'INSTRUMENT/True Relative'; also check and update any corresponding story "name" fields in this story file (true-relative.stories.ts) to follow the same Title Case convention to satisfy the openbridge/storybook-title-case rule.packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts-46-63 (1)
46-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove hardcoded test data from production component.
The
satellitesproperty contains hardcoded test data with the comment "this is just for test". Production components should not ship with placeholder data. Either move test data to the stories file or initialize to an empty array.🧹 Proposed fix
- `@property`({type: Array}) satellites: SatelliteData[] = [ // this is just for test - {id: 13, type: SatelliteType.GPS, strength: 0.8, azimuth: 90, elevation: 45, spoofing: true, jamming: false, spoJamConfirmed: false}, - ... - ]; + `@property`({type: Array}) satellites: SatelliteData[] = [];Move the test data to
gnss-skyplot.stories.tsdefault args instead.🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts` around lines 46 - 63, The satellites property in gnss-skyplot.ts currently contains hardcoded test data; remove the inline array and comment and initialize `@property`({type: Array}) satellites: SatelliteData[] = [] (empty array) so the production component ships without placeholder data, then move the provided sample objects into the story file (gnss-skyplot.stories.ts) as the default args.satellites for storybook tests and examples.packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.stories.ts-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Title Case for Storybook title.
The title
'INSTRUMENT/Gnss-Skyplot'violates the Title Case requirement. It should be'INSTRUMENT/GNSS Skyplot'(GNSS is an acronym and Skyplot should be title-cased as one word or two).As per coding guidelines: "Storybook story
titleandnamefields must use Title Case — enforced by ESLint ruleopenbridge/storybook-title-case(auto-fixable)".📝 Proposed fix
- title: 'INSTRUMENT/Gnss-Skyplot', + title: 'INSTRUMENT/GNSS Skyplot',🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.stories.ts` at line 7, Update the Storybook title string used in the stories file: replace the current title value `'INSTRUMENT/Gnss-Skyplot'` with the Title Case-compliant `'INSTRUMENT/GNSS Skyplot'` so the acronym GNSS is uppercase and "Skyplot" is title-cased; this will satisfy the openbridge/storybook-title-case ESLint rule for the exported title constant in the gnss-skyplot story module.packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts-22-22 (1)
22-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the typo in satellite type name.
Galileioshould beGalileo(the European GNSS constellation).📝 Proposed fix
- Galileio = 'Galileio', + Galileo = 'Galileo',Update all references throughout the file (lines 49, 53, 56, 59, 61, 414).
🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts` at line 22, Replace the misspelled satellite constellation identifier "Galileio" with the correct "Galileo" everywhere it appears in this file (update the enum/member name and all uses, string labels, switch/case checks, mappings and any references in methods like legend rendering or color/mapping functions); ensure the identifier casing matches existing enum/style and update all occurrences of "Galileio" to "Galileo" so type checks and UI labels remain consistent.
🧹 Nitpick comments (6)
packages/openbridge-webcomponents/src/skipper/wind-icons.ts (1)
89-89: ⚡ Quick winRemove TODO/dead commented code from TS source.
Lines 89/97/105 and Lines 116–132 keep TODOs and a legacy commented implementation inside runtime code. Please move TODOs to tracking issues and delete the commented-out block.
As per coding guidelines, "
**/*.{js,ts,tsx,jsx}: Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation."Also applies to: 97-97, 105-105, 116-132
🤖 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 `@packages/openbridge-webcomponents/src/skipper/wind-icons.ts` at line 89, Remove the inline TODOs and the legacy commented implementation in packages/openbridge-webcomponents/src/skipper/wind-icons.ts (the "// TODO add the missing speeds..." comment at line ~89 and the commented block around lines ~116–132); file should not contain commented-out code or TODOs per guidelines—create a tracking issue for the missing speeds and delete the commented block and any other TODO comments (also remove the comments at lines ~97 and ~105) so the module exports/definitions in wind-icons.ts remain clean and build/test behavior unchanged.packages/openbridge-webcomponents/src/skipper/current-icons.ts (1)
5-5: ⚡ Quick winRemove inline TODO comment from runtime code.
Line 5 violates the repository rule that TS/JS code should not contain comments unless the code is unusually hard to understand. Move this note to a tracking issue/PR task and keep the function body comment-free.
As per coding guidelines, "
**/*.{js,ts,tsx,jsx}: Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation."🤖 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 `@packages/openbridge-webcomponents/src/skipper/current-icons.ts` at line 5, Remove the inline runtime comment "// TODO - check this icon" at the top of current-icons.ts; delete that TODO from the module (top-level comment near the exported icon definitions) and instead record the note in your issue tracker or PR checklist so the runtime code (exports like the current icon constants/exports) contains no comments per repo rules.packages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.stories.ts (1)
6-7: ⚡ Quick winFix the Title Case claim and downgrade the Storybook typing concern
meta.title: 'INSTRUMENT/LongLatEnvironmental'already matches theopenbridge/storybook-title-caserule (no update needed).Meta<typeof LongLatEnvironmental>/satisfies Meta<LongLatEnvironmental>/StoryObj<LongLatEnvironmental>are self-referential to the exported story constant (there’s only a side-effect import), but the repo uses the same pattern inpackages/openbridge-webcomponents/src/skipper/long-lat/long-lat.stories.ts, so treat this as an optional cleanup: import the component type and use that in the generics if you want the typing to target the component class specifically.🤖 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 `@packages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.stories.ts` around lines 6 - 7, The title is already compliant so leave meta.title as 'INSTRUMENT/LongLatEnvironmental'; to address the typing comment optionally import the actual component class/type and replace the self-referential generics (Meta<typeof LongLatEnvironmental> and any StoryObj<typeof LongLatEnvironmental>) with Meta<LongLatEnvironmentalComponent> / StoryObj<LongLatEnvironmentalComponent> (or whatever the exported component type is) to target the component class explicitly; otherwise no change is required.packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.ts (1)
25-25: 💤 Low valueRemove or resolve the TODO comment.
Line 25 has
// TODO: removenext tospeedType. If this property is deprecated and should be removed, do so before merging; otherwise, remove the TODO comment.🤖 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 `@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.ts` at line 25, The field speedType (declared as `@property`({type: String}) speedType = SpeedType.SOG) still has a TODO comment; either remove the property if it’s deprecated or keep it and delete the TODO. Locate the true-relative component class (TrueRelative / true-relative.ts) and either delete the speedType property and any usages of SpeedType.SOG, or simply remove the "// TODO: remove" comment and ensure any external consumers are updated if you keep it; make sure imports and tests compile after the change.packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts (2)
114-120: 💤 Low valueClarify the TODO or remove if designer confirmation is not needed.
The comment "TODO check the right satellite strength" suggests the threshold mapping (0/30/70) may be placeholder. If design confirmation is pending, document the requirement or open a tracking issue; otherwise remove the TODO.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts` around lines 114 - 120, The TODO in mapStrength suggests the thresholds (0/30/70) are provisional; update the comment to either document the source of these thresholds or remove the TODO if they are final: inside the private method mapStrength(value: number): Strength (and alongside the Strength enum), add a short doc comment explaining why 0, 30, and 70 were chosen or reference a design/issue ID that defines the thresholds, or simply delete the TODO if no further confirmation is required; ensure the mapping logic remains unchanged unless design provides new threshold values.
33-42: ⚡ Quick winConsider consolidating duplicate interfaces.
Satellite(lines 33-42) duplicates all fields fromSatelliteData(lines 8-17). If the two interfaces serve identical purposes, use a single type; ifSatelliteis a transformed version with computedx/y, consider makingSatelliteextend or composeSatelliteData.🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts` around lines 33 - 42, The Satellite interface duplicates fields from SatelliteData; either remove the duplicate by reusing SatelliteData where possible or make Satellite extend SatelliteData (e.g., declare Satellite extends SatelliteData) and only add the transformed fields (like x, y) or additional flags; update usages of Satellite and SatelliteData accordingly (functions/methods referencing Satellite, SatelliteData, and any transformations inside gnss-skyplot.ts) so there is a single source of truth for the shared fields.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/advice.ts`:
- Around line 175-193: The AdviceType.advice branch always renders both
tickmarks, ignoring the AngleAdviceRaw flags hideMinTickmark/hideMaxTickmark;
update the SVG return in advice.ts (the branch handling AdviceType.advice / the
function that returns svg`...`) to conditionally include the tickmark(...) calls
based on advice.hideMinTickmark and advice.hideMaxTickmark so the min tickmark
is omitted when hideMinTickmark is true and the max tickmark is omitted when
hideMaxTickmark is true (keep the same TickmarkType.primary config and props for
the included tickmarks).
In `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts`:
- Around line 426-453: Remove the commented-out generateRandomSatellites block
from gnss-skyplot.ts to eliminate dead code; if the random-satellite generator
is useful for testing or stories, move the implementation into the component's
Storybook story or a dedicated test utility (e.g., export a helper in a
test-utils file) and update references to SatelliteData and SatelliteType there
instead of keeping the commented code inside the GnssSkyplot component file.
- Around line 44-45: Add a JSDoc block above the GnssSkyplot class declaration
describing the component tag <ob-gnss-skyplot> with a one-line summary, a short
bullet list of features/variants, and minimal usage guidelines; include an
`@fires` satellite tag to document the click event, do not add any `@property` tags
(properties are documented inline above fields), and avoid domain-specific
qualifiers — keep the description domain-agnostic. Ensure the JSDoc sits
immediately above the "export class GnssSkyplot extends LitElement" line and
references the tag name and the "satellite" event.
- Around line 70-78: The render() method mutates this.satellites which is not
allowed; move the mapping logic out of render and into a lifecycle or state: add
a private `@state`() property (e.g., mappedSatellites) and compute
mappedSatellites from this.satellites inside willUpdate(changedProps) or
update(), then have render() and getGnssSkylot()/getSatellites() read
mappedSatellites instead of reassigning this.satellites; ensure you update
getSatellites() to iterate this.mappedSatellites and remove any assignment to
this.satellites from render().
- Around line 127-134: Replace the global window event dispatch with a
component-host dispatch and remove the debug log: in the GnssSkyplot component
where the 'satellite' event is emitted (the code that currently calls
window.dispatchEvent(new CustomEvent<Satellite>('satellite', { detail:
clickedSatellite }))), call this.dispatchEvent(...) instead so consumers can
listen on the element, and delete the console.log(clickedSatellite) debug
statement; keep the CustomEvent name and detail payload unchanged.
In
`@packages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.ts`:
- Around line 49-63: The animation loop is being started twice (in firstUpdated
and connectedCallback) and disconnectedCallback only cancels one frame id; fix
by ensuring the RAF loop is started exactly once and fully cancelled: in
firstUpdated and connectedCallback (functions: firstUpdated, connectedCallback)
add a guard like “if (!this._animId)” before calling
requestAnimationFrame/_animate so you never start a second loop, unify how you
start the loop (prefer requestAnimationFrame and set this._animId =
requestAnimationFrame(this._animate) inside _animate’s recursion), and in
disconnectedCallback cancelAnimationFrame only when this._animId is set and then
clear it (this._animId = undefined/null) so no updates continue after detach;
update references to _lastTime, _animId, and _animate accordingly.
In `@packages/openbridge-webcomponents/src/skipper/long-lat/long-lat.stories.ts`:
- Line 7: The storybook title string used in long-lat.stories.ts (`title:
'INSTRUMENT/LongLat'`) is not Title Case; update the `title` field to a Title
Case value (for example `Instrument/LongLat` or `Instrument/Long Lat`) so it
complies with the openbridge/storybook-title-case ESLint rule; ensure any `name`
fields in the same file are also converted to Title Case.
In
`@packages/openbridge-webcomponents/src/skipper/speedometer/speedometer.stories.ts`:
- Line 7: The Storybook story title string in speedometer.stories.ts
("INSTRUMENT/Speedometer") is not Title Case; update the title field in the
export default (the Storybook config object) to use Title Case (e.g.,
"Instrument/Speedometer" or another Title Case variant) so it complies with the
openbridge/storybook-title-case ESLint rule; change any corresponding story
"name" fields in this file if present to Title Case as well.
- Around line 1-7: The Storybook generics are incorrectly using the story export
name "Speedometer" instead of the actual component class type, so update the
file to either import the real component class and reference it in the generics
(e.g., import { Speedometer as SpeedometerElement } from './speedometer.js' and
use Meta<typeof SpeedometerElement>, StoryObj<typeof SpeedometerElement>) or
rename the story export to avoid the conflict (e.g., export const
SpeedometerStory) and update all generics to use the correct imported component
type; ensure you change occurrences of Meta<typeof Speedometer>, satisfies
Meta<Speedometer>, and StoryObj<Speedometer> accordingly and keep the story
export symbol unique (SpeedometerStory) if you choose renaming.
In `@packages/openbridge-webcomponents/src/skipper/speedometer/speedometer.ts`:
- Around line 302-360: getAlertLowMask and getAlertHighMask both declare
identical SVG mask IDs (mask9_6621_15748 and mask10_6621_15748) causing
collisions when both alerts render; fix by giving each mask a unique ID (e.g.,
append "_low" and "_high" or generate unique suffixes) and update every
reference inside getAlertLowMask (mask="url(#...)") and getAlertHighMask to the
corresponding unique IDs so the low and high alert groups reference their own
masks.
In
`@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.ts`:
- Around line 15-16: Add a JSDoc block immediately above the TrueRelative class
declaration (the `@customElement`('ob-true-relative') export class TrueRelative)
that begins with a one-line summary including the tag name <ob-true-relative>,
then a short bullet-style features/variants list covering: multi-layer
navigation watch, SOG/COG, STW/heading, current, wind, north arrow, rotating
rate bar, and configurable direction reference; follow that with brief usage
guidelines showing how to include the component and any key attributes/slots
(but do not add `@property` tags — document properties inline in code instead),
and ensure the copy avoids domain qualifiers like "maritime", "navigation", or
"bridge".
- Around line 80-90: The render() method is mutating component state by calling
setMaxSpeed(), setSpeedRatio(), and setRotationsPerMinute(); move those calls
out of render and into a lifecycle hook such as willUpdate(changedProps) (or
updated(changedProps) if you need post-render effects) so state updates occur
outside rendering; implement willUpdate to call setMaxSpeed(), setSpeedRatio(),
and setRotationsPerMinute() (guarding with changedProps checks if needed) and
remove those calls from render() to prevent infinite re-renders and stale state.
- Line 34: The `@property` decorator for sensorPosition is incorrectly declared as
Boolean while its default is SensorPosition.bow (an enum); open SensorPosition
in interfaces.ts to confirm whether the enum is numeric or string-valued, then
change the decorator type for sensorPosition from Boolean to the correct runtime
type (Number if the enum uses numeric values or String if it uses strings) so
the property metadata matches SensorPosition and update any related consumers
that assume the corrected type; locate the declaration named sensorPosition and
the SensorPosition enum to make this change.
In `@packages/openbridge-webcomponents/src/skipper/wind-icons.ts`:
- Around line 82-109: The wind icon mapping omits handlers for 55–60 and 65–70
knots and the final branch (else if (windSpeed >= 74.0)) returns an empty SVG,
producing no glyph; update the wind-to-SVG logic (the branches checking
windSpeed and returning svg`...`) to add explicit branches for >=55.0 && <60.0
and >=65.0 && <70.0 using the same SVG pattern as nearby cases (incremental
bar/flag additions using Colors.elementNeutralColor), and replace the empty
>=74.0 branch with a full SVG (the strongest/highest-speed icon) consistent with
the style of the other cases so high speeds render a visible icon.
---
Minor comments:
In `@packages/openbridge-webcomponents/src/skipper/advice.ts`:
- Around line 66-68: Remove the dead commented-out implementation fragments in
the skipper/advice.ts module (specifically the commented ternary assignments for
mainColor and fillColor that reference AdviceType.caution/warning/alarm and the
other commented blocks at the other noted locations); delete those commented
lines entirely so the file contains no leftover commented code, leaving only
active code and, if necessary, refactor any remaining references to the
variables mainColor/fillColor or AdviceType to use the existing active
implementation paths to keep builds/tests passing.
In
`@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.stories.ts`:
- Line 7: Update the Storybook title string used in the stories file: replace
the current title value `'INSTRUMENT/Gnss-Skyplot'` with the Title
Case-compliant `'INSTRUMENT/GNSS Skyplot'` so the acronym GNSS is uppercase and
"Skyplot" is title-cased; this will satisfy the openbridge/storybook-title-case
ESLint rule for the exported title constant in the gnss-skyplot story module.
In `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts`:
- Around line 46-63: The satellites property in gnss-skyplot.ts currently
contains hardcoded test data; remove the inline array and comment and initialize
`@property`({type: Array}) satellites: SatelliteData[] = [] (empty array) so the
production component ships without placeholder data, then move the provided
sample objects into the story file (gnss-skyplot.stories.ts) as the default
args.satellites for storybook tests and examples.
- Line 22: Replace the misspelled satellite constellation identifier "Galileio"
with the correct "Galileo" everywhere it appears in this file (update the
enum/member name and all uses, string labels, switch/case checks, mappings and
any references in methods like legend rendering or color/mapping functions);
ensure the identifier casing matches existing enum/style and update all
occurrences of "Galileio" to "Galileo" so type checks and UI labels remain
consistent.
In
`@packages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.stories.ts`:
- Line 7: The story title string currently uses
"INSTRUMENT/LongLatEnvironmental" which violates the Title Case rule; update the
exported story's title value to a Title Case path such as "Instrument/Long Lat
Environmental" (or similar Title Case segmentation) so it satisfies the
openbridge/storybook-title-case lint rule and Storybook conventions.
In `@packages/openbridge-webcomponents/src/skipper/tickmark.ts`:
- Line 70: Remove the inline comment "// check if scale is not infinite" from
the TypeScript module in
packages/openbridge-webcomponents/src/skipper/tickmark.ts; locate that exact
comment text in the tickmark.ts source and delete the comment line so the file
contains no inline comments (do not replace with other comments or disable
rules).
- Around line 71-73: The scale validation in tickmark.ts currently only rejects
Infinity and non-positive numbers but allows NaN; update the check around the
variable scale (in the function where scale is validated) to use
Number.isFinite(scale) && scale > 0 (or equivalently !Number.isNaN(scale) and
scale !== Infinity and scale > 0) and throw the same Error('Scale is not valid')
when it fails so NaN no longer propagates into SVG coordinates.
In
`@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative-styles.ts`:
- Around line 20-23: The CSS rule for the .speed-gauge-value selector in
true-relative-styles.ts uses an invalid unitless font-size ("font-size: 64");
update this to a valid CSS size such as "64px" (or another appropriate unit like
rem/em) so the rule reads font-size: 64px; and ensure any other occurrences in
true-relative-styles.ts follow the same change.
In
`@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.stories.ts`:
- Line 8: The Storybook story title currently uses 'INSTRUMENT/TrueRelative' —
update the title property to use Title Case with proper spacing, e.g.
'INSTRUMENT/True Relative'; also check and update any corresponding story "name"
fields in this story file (true-relative.stories.ts) to follow the same Title
Case convention to satisfy the openbridge/storybook-title-case rule.
---
Nitpick comments:
In `@packages/openbridge-webcomponents/src/skipper/current-icons.ts`:
- Line 5: Remove the inline runtime comment "// TODO - check this icon" at the
top of current-icons.ts; delete that TODO from the module (top-level comment
near the exported icon definitions) and instead record the note in your issue
tracker or PR checklist so the runtime code (exports like the current icon
constants/exports) contains no comments per repo rules.
In `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts`:
- Around line 114-120: The TODO in mapStrength suggests the thresholds (0/30/70)
are provisional; update the comment to either document the source of these
thresholds or remove the TODO if they are final: inside the private method
mapStrength(value: number): Strength (and alongside the Strength enum), add a
short doc comment explaining why 0, 30, and 70 were chosen or reference a
design/issue ID that defines the thresholds, or simply delete the TODO if no
further confirmation is required; ensure the mapping logic remains unchanged
unless design provides new threshold values.
- Around line 33-42: The Satellite interface duplicates fields from
SatelliteData; either remove the duplicate by reusing SatelliteData where
possible or make Satellite extend SatelliteData (e.g., declare Satellite extends
SatelliteData) and only add the transformed fields (like x, y) or additional
flags; update usages of Satellite and SatelliteData accordingly
(functions/methods referencing Satellite, SatelliteData, and any transformations
inside gnss-skyplot.ts) so there is a single source of truth for the shared
fields.
In
`@packages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.stories.ts`:
- Around line 6-7: The title is already compliant so leave meta.title as
'INSTRUMENT/LongLatEnvironmental'; to address the typing comment optionally
import the actual component class/type and replace the self-referential generics
(Meta<typeof LongLatEnvironmental> and any StoryObj<typeof
LongLatEnvironmental>) with Meta<LongLatEnvironmentalComponent> /
StoryObj<LongLatEnvironmentalComponent> (or whatever the exported component type
is) to target the component class explicitly; otherwise no change is required.
In
`@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.ts`:
- Line 25: The field speedType (declared as `@property`({type: String}) speedType
= SpeedType.SOG) still has a TODO comment; either remove the property if it’s
deprecated or keep it and delete the TODO. Locate the true-relative component
class (TrueRelative / true-relative.ts) and either delete the speedType property
and any usages of SpeedType.SOG, or simply remove the "// TODO: remove" comment
and ensure any external consumers are updated if you keep it; make sure imports
and tests compile after the change.
In `@packages/openbridge-webcomponents/src/skipper/wind-icons.ts`:
- Line 89: Remove the inline TODOs and the legacy commented implementation in
packages/openbridge-webcomponents/src/skipper/wind-icons.ts (the "// TODO add
the missing speeds..." comment at line ~89 and the commented block around lines
~116–132); file should not contain commented-out code or TODOs per
guidelines—create a tracking issue for the missing speeds and delete the
commented block and any other TODO comments (also remove the comments at lines
~97 and ~105) so the module exports/definitions in wind-icons.ts remain clean
and build/test behavior unchanged.
🪄 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: 06472481-804f-459b-b28c-7a29a32e09ea
⛔ Files ignored due to path filters (41)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/openbridge-webcomponents/__vis__/linux/__baselines__/bars-graphs/area-graph/area-graph.stories.ts/no-labels-ticks-area-graph--but-yes-32px-padding-for-optional-points--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/bars-graphs/line-graph/line-graph.stories.ts/no-labels-ticks-line-graph--but-yes-32px-padding-for-optional-points--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/bar-horizontal/bar-horizontal.stories.ts/chart-integration--as-external-bottom-axis--scalebackground--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/bar-horizontal/bar-horizontal.stories.ts/component-size-comparison--regular-medium-large-xl--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/bar-horizontal/bar-horizontal.stories.ts/state-comparison--incommand-active-loading-off-focus--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/bar-vertical/bar-vertical.stories.ts/chart-integration--as-external-right-axis--scalebackground--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/chart-line/chart-line-base.stories.ts/no-labels-ticks-line-graph--but-yes-32px-padding-for-optional-points--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/chart-line/chart-line-base.stories.ts/threshold-height-line-graph--192px--where-labels-appear--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/external-scale/external-scale.stories.ts/horizontal--bottom-side--hasbar--advices--setpoint--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/external-scale/external-scale.stories.ts/horizontal--top-side--hasbar--advices--setpoint--fillmode-tint--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/building-blocks/external-scale/external-scale.stories.ts/vertical--left-side--hasbar--advices--setpoint--fillmode-tint--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/automation-button-readout-stack/automation-button-readout-stack.stories.ts/mixed-value-and-button-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/automation-button-readout-stack/automation-button-readout-stack.stories.ts/mixed-value-and-state-off-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/automation-button-readout-stack/automation-button-readout-stack.stories.ts/mixed-value-and-state-on-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/automation-button-readout-stack/automation-button-readout-stack.stories.ts/state-off-without-icon-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-horizontal-multi-application-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-horizontal-multi-regular-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-horizontal-single-application-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-horizontal-single-regular-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-vertical-multi-application-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-vertical-multi-regular-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-vertical-single-application-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/notification-floating-item/notification-floating-item.stories.ts/notification-vertical-single-regular-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/toggle-button-vertical-group/toggle-button-vertical-group.stories.ts/initially-disabled-selection-falls-back-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/toggle-button-vertical-group/toggle-button-vertical-group.stories.ts/label-under-regular-disabled-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/components/topbar-message-item/topbar-message-item.stories.ts/large-with-long-text-and-description-and-both-timestamps-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/azimuth-thruster/azimuth-thruster.stories.ts/in-command-at-setpoint-disable-auto-setpoint-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-horizontal/gauge-horizontal.stories.ts/component-size-comparison--regular-medium-large-xl--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-horizontal/gauge-horizontal.stories.ts/fill-mode-comparison--enhanced--fill-vs-tint--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-horizontal/gauge-horizontal.stories.ts/state-comparison--incommand-active-loading-off--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-horizontal/gauge-horizontal.stories.ts/state-comparison--incommand-active-loading-off-focus--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-horizontal/gauge-horizontal.stories.ts/with-setpoint--value-away-from-setpoint--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-vertical/gauge-vertical.stories.ts/component-size-comparison--regular-medium-large-xl--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-vertical/gauge-vertical.stories.ts/enhanced-mode-comparison--regular-vs-enhanced--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-vertical/gauge-vertical.stories.ts/fill-mode-comparison--enhanced--fill-vs-tint--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-vertical/gauge-vertical.stories.ts/state-comparison--incommand-active-loading-off--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/gauge-vertical/gauge-vertical.stories.ts/state-comparison--incommand-active-loading-off-focus--auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/main-engine-indicator/propulsion-main-engine-indicator.stories.ts/in-command---silhouette-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/main-engine-indicator/propulsion-main-engine-indicator.stories.ts/not-in-command---silhouette-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/propulsion-tunnel-thruster/propulsion-tunnel-thruster.stories.ts/not-in-command---silhouette-auto.pngis excluded by!**/*.png
📒 Files selected for processing (25)
.devcontainer/devcontainer-lock.jsonpackages/openbridge-webcomponents/src/components/accordion-card/accordion-card.csspackages/openbridge-webcomponents/src/skipper/advice.tspackages/openbridge-webcomponents/src/skipper/current-icons.tspackages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot-styles.tspackages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.stories.tspackages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.tspackages/openbridge-webcomponents/src/skipper/index.tspackages/openbridge-webcomponents/src/skipper/interfaces.tspackages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.stories.tspackages/openbridge-webcomponents/src/skipper/long-lat/long-lat-environmental.tspackages/openbridge-webcomponents/src/skipper/long-lat/long-lat-styles.tspackages/openbridge-webcomponents/src/skipper/long-lat/long-lat.stories.tspackages/openbridge-webcomponents/src/skipper/long-lat/long-lat.tspackages/openbridge-webcomponents/src/skipper/models.tspackages/openbridge-webcomponents/src/skipper/speedometer/speedometer-styles.tspackages/openbridge-webcomponents/src/skipper/speedometer/speedometer.stories.tspackages/openbridge-webcomponents/src/skipper/speedometer/speedometer.tspackages/openbridge-webcomponents/src/skipper/tickmark.tspackages/openbridge-webcomponents/src/skipper/true-relative/true-relative-styles.tspackages/openbridge-webcomponents/src/skipper/true-relative/true-relative.stories.tspackages/openbridge-webcomponents/src/skipper/true-relative/true-relative.tspackages/openbridge-webcomponents/src/skipper/utils/storybook-helpers.tspackages/openbridge-webcomponents/src/skipper/utils/uuid.tspackages/openbridge-webcomponents/src/skipper/wind-icons.ts
Comment on lines
+175
to
+193
| return svg` | ||
| ${adviceMask(advice.minAngle, advice.maxAngle, advice.state === AdviceState.regular ? mainColor : 'none', mainColor)} | ||
| ${tickmark(advice.minAngle, { | ||
| size: TickmarkType.primary, | ||
| style: tickmarkStyle, | ||
| scale: 1, | ||
| inside: false, | ||
| textRadius: 0, | ||
| maxDigits: 0, | ||
| })} | ||
| ${tickmark(advice.maxAngle, { | ||
| size: TickmarkType.primary, | ||
| style: tickmarkStyle, | ||
| scale: 1, | ||
| inside: false, | ||
| textRadius: 0, | ||
| maxDigits: 0, | ||
| })} | ||
| `; |
Contributor
There was a problem hiding this comment.
Apply hide-tickmark flags in the AdviceType.advice branch too.
Lines 177-192 always render both tickmarks, so hideMinTickmark/hideMaxTickmark are ignored for AdviceType.advice even though they are part of the shared AngleAdviceRaw contract.
Suggested fix
return svg`
${adviceMask(advice.minAngle, advice.maxAngle, advice.state === AdviceState.regular ? mainColor : 'none', mainColor)}
- ${tickmark(advice.minAngle, {
- size: TickmarkType.primary,
- style: tickmarkStyle,
- scale: 1,
- inside: false,
- textRadius: 0,
- maxDigits: 0,
- })}
- ${tickmark(advice.maxAngle, {
- size: TickmarkType.primary,
- style: tickmarkStyle,
- scale: 1,
- inside: false,
- textRadius: 0,
- maxDigits: 0,
- })}
+ ${
+ advice.hideMinTickmark
+ ? nothing
+ : tickmark(advice.minAngle, {
+ size: TickmarkType.primary,
+ style: tickmarkStyle,
+ scale: 1,
+ inside: false,
+ textRadius: 0,
+ maxDigits: 0,
+ })
+ }
+ ${
+ advice.hideMaxTickmark
+ ? nothing
+ : tickmark(advice.maxAngle, {
+ size: TickmarkType.primary,
+ style: tickmarkStyle,
+ scale: 1,
+ inside: false,
+ textRadius: 0,
+ maxDigits: 0,
+ })
+ }
`;🤖 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 `@packages/openbridge-webcomponents/src/skipper/advice.ts` around lines 175 -
193, The AdviceType.advice branch always renders both tickmarks, ignoring the
AngleAdviceRaw flags hideMinTickmark/hideMaxTickmark; update the SVG return in
advice.ts (the branch handling AdviceType.advice / the function that returns
svg`...`) to conditionally include the tickmark(...) calls based on
advice.hideMinTickmark and advice.hideMaxTickmark so the min tickmark is omitted
when hideMinTickmark is true and the max tickmark is omitted when
hideMaxTickmark is true (keep the same TickmarkType.primary config and props for
the included tickmarks).
Comment on lines
+44
to
+45
| @customElement('ob-gnss-skyplot') | ||
| export class GnssSkyplot extends LitElement { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add JSDoc documentation to the component class.
The GnssSkyplot class is missing JSDoc. As per coding guidelines, components should include:
- One-line summary with tag name (
<ob-gnss-skyplot>) - Features/variants bullet list
- Usage guidelines
@fires satellite- to document the click event- No
@propertytags (properties are documented inline above field declarations)
Also avoid domain qualifiers like "maritime" or "GNSS navigation" in the description; keep it domain-agnostic.
As per coding guidelines for component JSDoc.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts`
around lines 44 - 45, Add a JSDoc block above the GnssSkyplot class declaration
describing the component tag <ob-gnss-skyplot> with a one-line summary, a short
bullet list of features/variants, and minimal usage guidelines; include an
`@fires` satellite tag to document the click event, do not add any `@property` tags
(properties are documented inline above fields), and avoid domain-specific
qualifiers — keep the description domain-agnostic. Ensure the JSDoc sits
immediately above the "export class GnssSkyplot extends LitElement" line and
references the tag name and the "satellite" event.
Comment on lines
+70
to
+78
| override render() { | ||
| // this.satellites = this.mapSatellitesToCircle(this.generateRandomSatellites()); | ||
| this.satellites = this.mapSatellitesToCircle(this.satellites); | ||
| return html` | ||
| <div class="container"> | ||
| ${this.getGnssSkylot()} | ||
| </div> | ||
| `; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not mutate properties during render.
Line 72 reassigns this.satellites during render(), which violates LitElement's reactive update contract. Property mutations should occur in lifecycle methods (willUpdate, update) or event handlers, not in render(). Move the mapping logic to willUpdate or store the mapped result in a private @state() property.
♻️ Recommended refactor
+ `@state`()
+ private mappedSatellites: (SatelliteData & { x: number; y: number })[] = [];
+
+ override willUpdate(changed: PropertyValues) {
+ super.willUpdate(changed);
+ if (changed.has('satellites')) {
+ this.mappedSatellites = this.mapSatellitesToCircle(this.satellites);
+ }
+ }
+
override render() {
- this.satellites = this.mapSatellitesToCircle(this.satellites);
return html`
<div class="container">
${this.getGnssSkylot()}
</div>
`;
}Then update getSatellites() (line 407) to iterate this.mappedSatellites.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts`
around lines 70 - 78, The render() method mutates this.satellites which is not
allowed; move the mapping logic out of render and into a lifecycle or state: add
a private `@state`() property (e.g., mappedSatellites) and compute
mappedSatellites from this.satellites inside willUpdate(changedProps) or
update(), then have render() and getGnssSkylot()/getSatellites() read
mappedSatellites instead of reassigning this.satellites; ensure you update
getSatellites() to iterate this.mappedSatellites and remove any assignment to
this.satellites from render().
Comment on lines
+127
to
+134
| window.dispatchEvent( | ||
| new CustomEvent<Satellite>('satellite', { | ||
| detail: clickedSatellite | ||
| }) | ||
| ); | ||
|
|
||
| // TODO: remove | ||
| console.log(clickedSatellite); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider dispatching event on component host instead of window.
Dispatching the satellite event on window (line 127) breaks component encapsulation. Standard practice is to dispatch custom events on the component itself (this.dispatchEvent(...)) so consumers can listen via <ob-gnss-skyplot @satellite="..."> or element.addEventListener('satellite', ...).
Also remove the debug console.log (line 134) before merging.
♻️ Proposed refactor
private handleClickOnSatellite(id: number) {
const clickedSatellite = this.satellites.find(s => s.id === id);
if (!clickedSatellite) return;
- window.dispatchEvent(
+ this.dispatchEvent(
new CustomEvent<Satellite>('satellite', {
+ bubbles: true,
+ composed: true,
detail: clickedSatellite
})
);
- // TODO: remove
- console.log(clickedSatellite);
}🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts`
around lines 127 - 134, Replace the global window event dispatch with a
component-host dispatch and remove the debug log: in the GnssSkyplot component
where the 'satellite' event is emitted (the code that currently calls
window.dispatchEvent(new CustomEvent<Satellite>('satellite', { detail:
clickedSatellite }))), call this.dispatchEvent(...) instead so consumers can
listen on the element, and delete the console.log(clickedSatellite) debug
statement; keep the CustomEvent name and detail payload unchanged.
Comment on lines
+426
to
+453
| /* | ||
| private generateRandomSatellites(count = 30): SatelliteData[] { | ||
| const types = [ | ||
| SatelliteType.GPS, | ||
| SatelliteType.GLONASS, | ||
| SatelliteType.Galileio, | ||
| SatelliteType.BeiDou | ||
| ]; | ||
|
|
||
| return Array.from({ length: count }, (_, i) => { | ||
| const type = types[Math.floor(Math.random() * types.length)]; | ||
|
|
||
| return { | ||
| id: i + 1, | ||
| type, | ||
| strength: this.mapStrength(Math.round(Math.random() * 100)), // 0–100 | ||
|
|
||
| azimuth: Math.floor(Math.random() * 360), // 0–360° | ||
| elevation: Math.floor(Math.random() * 91), // 0–90° | ||
|
|
||
| spoofing: Math.random() < 0.1, // 10% | ||
| jamming: Math.random() < 0.1, // 10% | ||
| spoJamConfirmed: false | ||
| }; | ||
| }); | ||
| } | ||
| */ | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove commented-out code.
The generateRandomSatellites function (lines 426-452) is commented out. Dead code should be removed; if needed for testing, move it to the stories file or a separate test utility.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/gnss-skyplot/gnss-skyplot.ts`
around lines 426 - 453, Remove the commented-out generateRandomSatellites block
from gnss-skyplot.ts to eliminate dead code; if the random-satellite generator
is useful for testing or stories, move the implementation into the component's
Storybook story or a dedicated test utility (e.g., export a helper in a
test-utils file) and update references to SatelliteData and SatelliteType there
instead of keeping the commented code inside the GnssSkyplot component file.
Comment on lines
+302
to
+360
| private getAlertLowMask() { | ||
| if (this.alertLowType != AlertTypes.none && this.useAlerts && this.alertLow > -this.maxSpeed/5) { | ||
| return svg` | ||
| <mask id="mask9_6621_15748" style="mask-type:alpha" maskUnits="userSpaceOnUse" x="65" y="17" width="825" height="824"> | ||
| <path d="M477.284 126.438C644.385 126.438 779.847 261.899 779.847 429C779.847 507.92 749.632 579.783 700.138 633.651C695.327 638.887 687.153 638.869 682.125 633.841L618.397 570.112C613.369 565.084 613.406 556.96 618.078 551.599C646.661 518.801 663.972 475.922 663.972 429C663.972 325.895 580.389 242.312 477.284 242.312C374.18 242.312 290.597 325.895 290.597 429C290.597 475.922 307.908 518.801 336.491 551.599C341.163 556.96 341.2 565.084 336.172 570.112L272.444 633.841C267.416 638.869 259.242 638.887 254.431 633.651C204.937 579.783 174.722 507.92 174.722 429C174.722 261.899 310.184 126.438 477.284 126.438Z" fill="${Colors.instrumentFramePrimary}" stroke="black" stroke-width="2.08333"/> | ||
| </mask> | ||
| <g mask="url(#mask9_6621_15748)"> | ||
| <mask id="mask10_6621_15748" style="mask-type:alpha" maskUnits="userSpaceOnUse" x="-106" y="-154" width="1166" height="1166"> | ||
| <circle cx="477.285" cy="429" r="263.937" transform="rotate(135 477.285 429)" fill="${Colors.instrumentFramePrimary}" stroke="black" stroke-width="2.08333"/> | ||
| </mask> | ||
| ${this.alertLowType != AlertTypes.caution ? svg` | ||
| <g transform="rotate(${-180 + this.alertLow * this.rotationAngle} 477.355 429)"> | ||
| <path d="M477.284 429 L742.1 429" stroke="${Colors.instrumentTickMarkSecondary}" stroke-width="2.08333"/> | ||
| </g> | ||
| <g transform="rotate(${-180 + this.minSpeed * this.rotationAngle} 477.355 429)"> | ||
| <path d="M477.284 429 L742.1 429" stroke="${Colors.instrumentTickMarkSecondary}" stroke-width="2.08333"/> | ||
| </g> | ||
| ` | ||
| : svg` | ||
| <g transform="rotate(${-180 + this.alertLow * this.rotationAngle} 477.355 429)"> | ||
| <path d="M477.284 429 L742.1 429" stroke="${Colors.instrumentFramePrimary}" stroke-width="2.08333"/> | ||
| <path d="M477.284 429 L742.1 429" stroke="${Colors.instrumentTickMarkSecondary}" stroke-width="2.08333" stroke-dasharray="8 4"/> | ||
| </g> | ||
| <g transform="rotate(${-180 + this.minSpeed * this.rotationAngle} 477.355 429)"> | ||
| <path d="M477.284 429 L742.1 429" stroke="${Colors.instrumentFramePrimary}" stroke-width="2.08333"/> | ||
| <path d="M477.284 429 L742.1 429" stroke="${Colors.instrumentTickMarkSecondary}" stroke-width="2.08333" stroke-dasharray="8 4"/> | ||
| </g> | ||
| ` | ||
| } | ||
| <!-- | ||
| ${ | ||
| this.alertLowType == AlertTypes.caution ? svg` | ||
| <g mask="url(#mask10_6621_15748)"> | ||
| <path d="M65.2845 429C65.2845 483.105 75.9412 536.679 96.6461 586.666C117.351 636.652 147.699 682.07 185.956 720.328L477.284 429L65.2845 429Z" fill="black" fill-opacity="0.05"/> | ||
| <path d="M65.2845 429C65.2845 483.105 75.9412 536.679 96.6461 586.666C117.351 636.652 147.699 682.07 185.956 720.328L477.284 429L65.2845 429Z" stroke="${Colors.instrumentFramePrimary}" stroke-width="2.08333"/> | ||
| <path d="M65.2845 429C65.2845 483.105 75.9412 536.679 96.6461 586.666C117.351 636.652 147.699 682.07 185.956 720.328L477.284 429L65.2845 429Z" stroke="${Colors.instrumentTickMarkSecondary}" stroke-width="2.08333" stroke-dasharray="8.33 8.33"/> | ||
| </g> | ||
| ` : nothing | ||
| } | ||
| --> | ||
|
|
||
| <path d="${this.cutTickmarks(this.getAngle(this.minSpeed + 0.25), this.getAngle(this.alertLow - 0.15))}" fill="${Colors.instrumentFramePrimary}"/> | ||
| </g> | ||
| `; | ||
| } | ||
| else { | ||
| return nothing; | ||
| } | ||
| } | ||
|
|
||
| private getAlertHighMask() { | ||
| if (this.alertHighType != AlertTypes.none && this.useAlerts && this.alertHigh < this.maxSpeed) { | ||
| return svg` | ||
| <mask id="mask9_6621_15748" style="mask-type:alpha" maskUnits="userSpaceOnUse" x="65" y="17" width="825" height="824"> | ||
| <path d="M477.284 126.438C644.385 126.438 779.847 261.899 779.847 429C779.847 507.92 749.632 579.783 700.138 633.651C695.327 638.887 687.153 638.869 682.125 633.841L618.397 570.112C613.369 565.084 613.406 556.96 618.078 551.599C646.661 518.801 663.972 475.922 663.972 429C663.972 325.895 580.389 242.312 477.284 242.312C374.18 242.312 290.597 325.895 290.597 429C290.597 475.922 307.908 518.801 336.491 551.599C341.163 556.96 341.2 565.084 336.172 570.112L272.444 633.841C267.416 638.869 259.242 638.887 254.431 633.651C204.937 579.783 174.722 507.92 174.722 429C174.722 261.899 310.184 126.438 477.284 126.438Z" fill="${Colors.instrumentFramePrimary}" stroke="black" stroke-width="2.08333"/> | ||
| </mask> | ||
| <g mask="url(#mask9_6621_15748)"> | ||
| <mask id="mask10_6621_15748" style="mask-type:alpha" maskUnits="userSpaceOnUse" x="-106" y="-154" width="1166" height="1166"> | ||
| <circle cx="477.285" cy="429" r="263.937" transform="rotate(135 477.285 429)" fill="${Colors.instrumentFramePrimary}" stroke="black" stroke-width="2.08333"/> |
Contributor
There was a problem hiding this comment.
Use unique SVG mask IDs for low/high alert paths.
Line 305 and Line 355 both define mask9_6621_15748 (and similarly mask10_6621_15748), so low/high alert rendering can reference the wrong mask when both are enabled.
🔧 Suggested fix
-<mask id="mask9_6621_15748" ...>
+<mask id="mask9_low_6621_15748" ...>
...
-<g mask="url(`#mask9_6621_15748`)">
+<g mask="url(`#mask9_low_6621_15748`)">
...
-<mask id="mask10_6621_15748" ...>
+<mask id="mask10_low_6621_15748" ...>-<mask id="mask9_6621_15748" ...>
+<mask id="mask9_high_6621_15748" ...>
...
-<g mask="url(`#mask9_6621_15748`)">
+<g mask="url(`#mask9_high_6621_15748`)">
...
-<mask id="mask10_6621_15748" ...>
+<mask id="mask10_high_6621_15748" ...>🤖 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 `@packages/openbridge-webcomponents/src/skipper/speedometer/speedometer.ts`
around lines 302 - 360, getAlertLowMask and getAlertHighMask both declare
identical SVG mask IDs (mask9_6621_15748 and mask10_6621_15748) causing
collisions when both alerts render; fix by giving each mask a unique ID (e.g.,
append "_low" and "_high" or generate unique suffixes) and update every
reference inside getAlertLowMask (mask="url(#...)") and getAlertHighMask to the
corresponding unique IDs so the low and high alert groups reference their own
masks.
Comment on lines
+15
to
+16
| @customElement('ob-true-relative') | ||
| export class TrueRelative extends LitElement { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add JSDoc documentation to the component class.
The TrueRelative class is missing JSDoc. As per coding guidelines, components should include:
- One-line summary with tag name (
<ob-true-relative>) - Features/variants bullet list (multi-layer navigation watch, SOG/COG, STW/heading, current, wind, north arrow, rotating rate bar, configurable direction reference)
- Usage guidelines
- No
@propertytags (properties are documented inline) - Avoid domain qualifiers (maritime, navigation, bridge) in documentation
As per coding guidelines for component JSDoc.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.ts`
around lines 15 - 16, Add a JSDoc block immediately above the TrueRelative class
declaration (the `@customElement`('ob-true-relative') export class TrueRelative)
that begins with a one-line summary including the tag name <ob-true-relative>,
then a short bullet-style features/variants list covering: multi-layer
navigation watch, SOG/COG, STW/heading, current, wind, north arrow, rotating
rate bar, and configurable direction reference; follow that with brief usage
guidelines showing how to include the component and any key attributes/slots
(but do not add `@property` tags — document properties inline in code instead),
and ensure the copy avoids domain qualifiers like "maritime", "navigation", or
"bridge".
| @property({type: Number}) rotationsPerMinute = 1; | ||
|
|
||
| @property({type: String}) direction: TrueRelativeDirection = TrueRelativeDirection.NorthUp; | ||
| @property({type: Boolean}) sensorPosition = SensorPosition.bow; |
Contributor
There was a problem hiding this comment.
Fix type annotation for sensorPosition property.
sensorPosition is declared as @property({type: Boolean}) but defaults to SensorPosition.bow (an enum, not a boolean). The correct type should be Number or String depending on the enum's runtime value type.
🐛 Proposed fix
- `@property`({type: Boolean}) sensorPosition = SensorPosition.bow;
+ `@property`({type: Number}) sensorPosition = SensorPosition.bow;Verify SensorPosition enum uses numeric values (check interfaces.ts).
📝 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.
Suggested change
| @property({type: Boolean}) sensorPosition = SensorPosition.bow; | |
| `@property`({type: Number}) sensorPosition = SensorPosition.bow; |
🤖 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 `@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.ts`
at line 34, The `@property` decorator for sensorPosition is incorrectly declared
as Boolean while its default is SensorPosition.bow (an enum); open
SensorPosition in interfaces.ts to confirm whether the enum is numeric or
string-valued, then change the decorator type for sensorPosition from Boolean to
the correct runtime type (Number if the enum uses numeric values or String if it
uses strings) so the property metadata matches SensorPosition and update any
related consumers that assume the corrected type; locate the declaration named
sensorPosition and the SensorPosition enum to make this change.
Comment on lines
+80
to
+90
| override render() { | ||
| this.setMaxSpeed(); | ||
| this.setSpeedRatio(); | ||
| this.setRotationsPerMinute(); | ||
|
|
||
| return html` | ||
| <div class="container"> | ||
| ${this.getTrueRelative()} | ||
| </div> | ||
| `; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not call state-mutating methods during render.
Lines 81-83 call setMaxSpeed(), setSpeedRatio(), and setRotationsPerMinute() during render(). These methods mutate component state, which violates LitElement's reactive update contract and can cause infinite re-render loops or stale data. Move these calls to willUpdate() or updated().
♻️ Recommended refactor
+ override willUpdate(changed: PropertyValues) {
+ super.willUpdate(changed);
+ if (changed.has('maxSpeed')) {
+ this.setMaxSpeed();
+ }
+ if (changed.has('hdgSpeed') || changed.has('maxSpeed')) {
+ this.setSpeedRatio();
+ }
+ if (changed.has('rotationsPerMinute')) {
+ this.setRotationsPerMinute();
+ }
+ }
+
override render() {
- this.setMaxSpeed();
- this.setSpeedRatio();
- this.setRotationsPerMinute();
return html`
<div class="container">
${this.getTrueRelative()}
</div>
`;
}📝 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.
Suggested change
| override render() { | |
| this.setMaxSpeed(); | |
| this.setSpeedRatio(); | |
| this.setRotationsPerMinute(); | |
| return html` | |
| <div class="container"> | |
| ${this.getTrueRelative()} | |
| </div> | |
| `; | |
| } | |
| override willUpdate(changed: PropertyValues) { | |
| super.willUpdate(changed); | |
| if (changed.has('maxSpeed')) { | |
| this.setMaxSpeed(); | |
| } | |
| if (changed.has('hdgSpeed') || changed.has('maxSpeed')) { | |
| this.setSpeedRatio(); | |
| } | |
| if (changed.has('rotationsPerMinute')) { | |
| this.setRotationsPerMinute(); | |
| } | |
| } | |
| override render() { | |
| return html` | |
| <div class="container"> | |
| ${this.getTrueRelative()} | |
| </div> | |
| `; | |
| } |
🤖 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 `@packages/openbridge-webcomponents/src/skipper/true-relative/true-relative.ts`
around lines 80 - 90, The render() method is mutating component state by calling
setMaxSpeed(), setSpeedRatio(), and setRotationsPerMinute(); move those calls
out of render and into a lifecycle hook such as willUpdate(changedProps) (or
updated(changedProps) if you need post-render effects) so state updates occur
outside rendering; implement willUpdate to call setMaxSpeed(), setSpeedRatio(),
and setRotationsPerMinute() (guarding with changedProps checks if needed) and
remove those calls from render() to prevent infinite re-renders and stale state.
Comment on lines
+82
to
+109
| else if (windSpeed >= 50.0 && windSpeed < 55.0) { | ||
| return svg` | ||
| <svg width="13" height="23" viewBox="0 0 13 23" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path fill-rule="evenodd" clip-rule="evenodd" d="M4.08237 0.00343268L4.19761 0.0200342L11.6917 1.51906C12.7644 1.73378 12.7644 3.26722 11.6917 3.48195L5.00034 4.81984V15.0005H6.99839C7.74195 15.0005 8.22661 15.7828 7.8939 16.4487L4.89585 22.4458C4.52677 23.1831 3.47373 23.1833 3.10483 22.4458L0.106787 16.4487C-0.22571 15.7833 0.258565 15.0008 1.00229 15.0005H3.00034V1.00148C3.00048 0.4093 3.50876 -0.0441781 4.08237 0.00343268Z" fill="${Colors.elementNeutralColor}"/> | ||
| </svg> | ||
| `; | ||
| } | ||
| // TODO add the missing speeds (>= 50.0 && <55.0) | ||
| else if (windSpeed >= 60.0 && windSpeed < 65.0) { | ||
| return svg` | ||
| <svg width="13" height="23" viewBox="0 0 13 23" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path fill-rule="evenodd" clip-rule="evenodd" d="M4.08237 0.00343268L4.19761 0.0200342L11.6917 1.51906C12.7644 1.73378 12.7644 3.26722 11.6917 3.48195L5.00034 4.81984V6.0005H11.0003C11.5526 6.0005 12.0003 6.44822 12.0003 7.0005C12.0001 7.5526 11.5525 8.0005 11.0003 8.0005H5.00034V15.0005H6.99839C7.74195 15.0005 8.22661 15.7828 7.8939 16.4487L4.89585 22.4458C4.52677 23.1831 3.47373 23.1833 3.10483 22.4458L0.106787 16.4487C-0.22571 15.7833 0.258565 15.0008 1.00229 15.0005H3.00034V1.00148C3.00048 0.4093 3.50876 -0.0441781 4.08237 0.00343268Z" fill="${Colors.elementNeutralColor}"/> | ||
| </svg> | ||
| `; | ||
| } | ||
| // TODO add the missing speeds (>= 60.0 && <65.0) | ||
| else if (windSpeed >= 70.0 && windSpeed < 75.0) { | ||
| return svg` | ||
| <svg width="13" height="23" viewBox="0 0 13 23" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path fill-rule="evenodd" clip-rule="evenodd" d="M4.08237 0.00343268L4.19761 0.0200342L11.6917 1.51906C12.7644 1.73378 12.7644 3.26722 11.6917 3.48195L5.00034 4.81984V6.0005H11.0003C11.5526 6.0005 12.0003 6.44822 12.0003 7.0005C12.0001 7.5526 11.5525 8.0005 11.0003 8.0005H5.00034V9.5005H11.0003C11.5526 9.5005 12.0003 9.94822 12.0003 10.5005C12.0001 11.0526 11.5525 11.5005 11.0003 11.5005H5.00034V15.0005H6.99839C7.74195 15.0005 8.22661 15.7828 7.8939 16.4487L4.89585 22.4458C4.52677 23.1831 3.47373 23.1833 3.10483 22.4458L0.106787 16.4487C-0.22571 15.7833 0.258565 15.0008 1.00229 15.0005H3.00034V1.00148C3.00048 0.4093 3.50876 -0.0441781 4.08237 0.00343268Z" fill="${Colors.elementNeutralColor}"/> | ||
| </svg> | ||
| `; | ||
| } | ||
| // TODO add the missing speeds (>= 70.0 && <74.0) | ||
| else if (windSpeed >= 74.0) { | ||
| return svg` | ||
|
|
||
| `; |
Contributor
There was a problem hiding this comment.
Wind-speed mapping is incomplete and returns blank icon for high speeds.
The current branches skip explicit handling for 55–60 and 65–70, and the >= 74.0 branch returns an empty <svg> (Line 106–109), which produces no visible glyph for high wind speeds.
Suggested patch
- else if (windSpeed >= 50.0 && windSpeed < 55.0) {
+ else if (windSpeed >= 50.0 && windSpeed < 55.0) {
return svg`
<svg width="13" height="23" viewBox="0 0 13 23" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M4.08237 0.00343268L4.19761 0.0200342L11.6917 1.51906C12.7644 1.73378 12.7644 3.26722 11.6917 3.48195L5.00034 4.81984V15.0005H6.99839C7.74195 15.0005 8.22661 15.7828 7.8939 16.4487L4.89585 22.4458C4.52677 23.1831 3.47373 23.1833 3.10483 22.4458L0.106787 16.4487C-0.22571 15.7833 0.258565 15.0008 1.00229 15.0005H3.00034V1.00148C3.00048 0.4093 3.50876 -0.0441781 4.08237 0.00343268Z" fill="${Colors.elementNeutralColor}"/>
</svg>
`;
}
- // TODO add the missing speeds (>= 50.0 && <55.0)
+ else if (windSpeed >= 55.0 && windSpeed < 60.0) {
+ // temporary fallback until dedicated icon is added
+ return getWindIcon(60.0);
+ }
else if (windSpeed >= 60.0 && windSpeed < 65.0) {
return svg`
<svg width="13" height="23" viewBox="0 0 13 23" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M4.08237 0.00343268L4.19761 0.0200342L11.6917 1.51906C12.7644 1.73378 12.7644 3.26722 11.6917 3.48195L5.00034 4.81984V6.0005H11.0003C11.5526 6.0005 12.0003 6.44822 12.0003 7.0005C12.0001 7.5526 11.5525 8.0005 11.0003 8.0005H5.00034V15.0005H6.99839C7.74195 15.0005 8.22661 15.7828 7.8939 16.4487L4.89585 22.4458C4.52677 23.1831 3.47373 23.1833 3.10483 22.4458L0.106787 16.4487C-0.22571 15.7833 0.258565 15.0008 1.00229 15.0005H3.00034V1.00148C3.00048 0.4093 3.50876 -0.0441781 4.08237 0.00343268Z" fill="${Colors.elementNeutralColor}"/>
</svg>
`;
}
- // TODO add the missing speeds (>= 60.0 && <65.0)
+ else if (windSpeed >= 65.0 && windSpeed < 70.0) {
+ // temporary fallback until dedicated icon is added
+ return getWindIcon(70.0);
+ }
else if (windSpeed >= 70.0 && windSpeed < 75.0) {
return svg`
<svg width="13" height="23" viewBox="0 0 13 23" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M4.08237 0.00343268L4.19761 0.0200342L11.6917 1.51906C12.7644 1.73378 12.7644 3.26722 11.6917 3.48195L5.00034 4.81984V6.0005H11.0003C11.5526 6.0005 12.0003 6.44822 12.0003 7.0005C12.0001 7.5526 11.5525 8.0005 11.0003 8.0005H5.00034V9.5005H11.0003C11.5526 9.5005 12.0003 9.94822 12.0003 10.5005C12.0001 11.0526 11.5525 11.5005 11.0003 11.5005H5.00034V15.0005H6.99839C7.74195 15.0005 8.22661 15.7828 7.8939 16.4487L4.89585 22.4458C4.52677 23.1831 3.47373 23.1833 3.10483 22.4458L0.106787 16.4487C-0.22571 15.7833 0.258565 15.0008 1.00229 15.0005H3.00034V1.00148C3.00048 0.4093 3.50876 -0.0441781 4.08237 0.00343268Z" fill="${Colors.elementNeutralColor}"/>
</svg>
`;
}
- // TODO add the missing speeds (>= 70.0 && <74.0)
else if (windSpeed >= 74.0) {
- return svg`
-
- `;
+ // temporary fallback until >=74 icon path is added
+ return getWindIcon(70.0);
}🤖 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 `@packages/openbridge-webcomponents/src/skipper/wind-icons.ts` around lines 82
- 109, The wind icon mapping omits handlers for 55–60 and 65–70 knots and the
final branch (else if (windSpeed >= 74.0)) returns an empty SVG, producing no
glyph; update the wind-to-SVG logic (the branches checking windSpeed and
returning svg`...`) to add explicit branches for >=55.0 && <60.0 and >=65.0 &&
<70.0 using the same SVG pattern as nearby cases (incremental bar/flag additions
using Colors.elementNeutralColor), and replace the empty >=74.0 branch with a
full SVG (the strongest/highest-speed icon) consistent with the style of the
other cases so high speeds render a visible icon.
talpitoo
requested changes
Jun 6, 2026
talpitoo
left a comment
talpitoo
left a comment
Collaborator
There was a problem hiding this comment.
note to self: requesting 'blank' changes to avoid accidental merge.
Collaborator
|
Hey @asijah2, thank you for your contribution! Attaching these screenshots so our team can more easily evaluate it. 
|
Author
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts (2)
405-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray comment block.
/// // /* TOP DOWN VIEW FUNCTIONS */is a comment that doesn't meet the "extremely unusual" bar for inclusion.As per coding guidelines, "Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation."
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts` around lines 405 - 409, Remove the stray comment block in vessel-types.ts under the top-down view section; it is not needed and does not meet the commenting guideline. Locate the TOP DOWN VIEW FUNCTIONS marker near the related vessel type/view helpers and delete that comment-only block without changing any surrounding logic or symbols.Source: Coding guidelines
810-827: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove large commented-out dead SVG block.
A ~17-line commented-out SVG fragment is left at the end of the file.
As per coding guidelines, "Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation."
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts` around lines 810 - 827, Remove the large commented-out SVG dead block from vessel-types.ts; the file should not keep commented code per the guideline. Locate the trailing commented SVG fragment near the Vessel-related constants and delete the entire block, leaving only the active vessel type definitions and render logic intact.Source: Coding guidelines
packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.stories.ts (2)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove commented-out dead code.
// width: 512,is a leftover commented-out line.As per coding guidelines, "Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation."
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.stories.ts` at line 44, Remove the commented-out dead code from the vessel story by deleting the leftover width assignment comment in the relevant story configuration. Update the vessel.stories.ts content around the vessel story so only active settings remain, and keep the Storybook story definitions clean without unnecessary comments.Source: Coding guidelines
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTitle Case violation in story title.
'INSTRUMENT/Vessel'uses an all-caps segment instead of Title Case.As per coding guidelines, "Use Title Case for Storybook
titleandnamefields."✏️ Proposed fix
- title: 'INSTRUMENT/Vessel', + title: 'Instrument/Vessel',🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.stories.ts` at line 7, The Storybook metadata in the vessel story uses an all-caps segment in the title, so update the `title` field in `vessel.stories.ts` to use Title Case consistently with the project guideline. Adjust the `INSTRUMENT/Vessel` entry in the story metadata so it follows the same casing convention as other Storybook `title` and `name` values.Source: Coding guidelines
packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts (4)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoolean property naming unclear.
toggleSLAndDLSensordoesn't clearly convey its true/false semantics (what does true vs. false mean?).As per coding guidelines, "Name boolean properties and parameters using positive phrasing." Consider a descriptive name reflecting the actual state, e.g.
useDoubleLineSensorMode.🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` at line 36, The boolean property name toggleSLAndDLSensor is unclear and uses toggle-style wording instead of positive phrasing. Rename the property in the vessel class to something that describes the enabled state, like useDoubleLineSensorMode, and update any references in the vessel-view component to match the new name so true/false meaning is obvious.Source: Coding guidelines
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExcessive inline comments violate the no-comments guideline.
Numerous
//comments (e.g. "values for SIDE_VIEW", "COMMON VIEW LOGIC", checkmark-annotated comments, commented-outdimTopY/dimBottomYlines) are present where the code is not "extremely unusual and impossible to understand without explanation."As per coding guidelines, "Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation."
Also applies to: 45-45, 57-57, 160-162, 219-221, 260-261, 270-270, 273-273, 293-293, 297-297, 309-310
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` at line 37, Remove the unnecessary inline comments in vessel.ts, including section labels like “values for SIDE_VIEW” and “COMMON VIEW LOGIC,” the checkmark-style annotations, and any commented-out code such as the dimTopY/dimBottomY lines. Keep the implementation in the affected blocks by relying on the existing structure and symbol names like the SIDE_VIEW logic and related vessel-view helpers, but delete comments unless they are truly required for unusually complex behavior.Source: Coding guidelines
226-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead no-op method referencing a nonexistent property.
toggleVesselViewer()does nothing; its only content is a commented-out reference tothis.sideTopDownViewToggle, a property that doesn't exist on this class.Should this method be removed, or is a real toggle implementation still pending? Happy to help wire it up if there's a missing requirement here.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` around lines 226 - 228, The toggleVesselViewer() method in the vessel class is currently a dead no-op and only references a nonexistent sideTopDownViewToggle property. Remove toggleVesselViewer() if it is not needed, or replace it with a real implementation that toggles an actual class property/state used by the vessel view; make sure the method and any referenced field names are consistent with the rest of the class.
230-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
getVessel()is a very large, complex method.This single method computes dozens of local variables and assembles both view modes; it would benefit from being broken into smaller, named helper methods for readability and testability.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` around lines 230 - 440, The getVessel() method is doing too much by calculating both side and top-down view state in one large block, which hurts readability and testability. Refactor it by extracting the distinct calculation/rendering pieces into smaller named helpers (for example for side-view geometry, top-down geometry, and view assembly) and keep getVessel() focused on selecting between SIDE_VIEW, TOP_DOWN_VIEW, or the combined layout.packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-styles.ts (1)
64-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded layout magic numbers.
400pxwidth,scale(0.69), andmargin-left: 60pxare magic values with no documented derivation. Givenvessel.stories.tsexposes awidthcontrol in its args, these fixed values won't respond to it, which may cause confusing/clipped output at other widths.🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-styles.ts` around lines 64 - 84, The vessel layout in vessel-styles should not rely on hardcoded magic numbers for .both-views, .both-views-inner, and .top-down-view. Update the styles to derive width, scale, and horizontal offset from the configurable width used by vessel.stories.ts so the vessel view responds correctly to different sizes instead of clipping or misaligning. Use the existing vessel-view styling selectors to replace fixed values with dynamic sizing logic tied to the story args or shared sizing constants.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts`:
- Around line 268-274: The side-dimension SVG lines in `dimSternToCCRPSide` and
`dimBowToCCRPSide` are hardcoded to y=303 instead of using the computed line
position from `buildDimensionLine`. Update these functions to use
`sideViewParams.sternLine.y` and `sideViewParams.bowLine.y` for the `y1`/`y2`
attributes so they stay in sync with the shared `LINE_Y` value used by
`vessel.ts`.
- Around line 1-4: The module in vessel-types.ts is missing the required
top-of-module JSDoc for a pure function module. Add a full JSDoc block above the
first export in this file, describing the module’s functionality and intended
usage, and place it before the symbols imported and used by the exported helpers
such as vesselTypes and any related template/render functions.
- Around line 261-293: The stern/bow CCRP rendering helpers are near-duplicates
of each other, and the same pattern also applies to the port/starboard line
renderers. Refactor `dimSternToCCRPSide` and `dimBowToCCRPSide` into a single
parameterized helper that accepts the varying text, line, and label fields from
`getSideViewParams`, and do the same for `dimPortToCCRPLine` and
`dimStarboardToCCRPLine`. Keep the existing SVG output behavior unchanged while
centralizing the shared `textLabel`, `path`, and `line` generation logic.
In `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.stories.ts`:
- Around line 60-64: The Vessel_cruiser story name is misleading because
VesselTypes has no CRUISER value and the story currently falls back to the
default CARGO vessel. Update the story in vessel.stories.ts so the exported
story name and any related labels/args match an actual VesselTypes entry, using
the existing VesselTypes and Vessel component story definitions as the reference
points.
In `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts`:
- Line 24: Add a class-level JSDoc block directly above the Vessel class
declaration to document its purpose and intended usage. Use domain-agnostic
wording only, and describe the concrete component at a high level without any
maritime-specific terms; keep the documentation on the Vessel class itself so it
matches the project’s component JSDoc guideline.
- Around line 147-159: The negative-value guard in updated() only covers
sternToCCRP and bowToCCRP, but the same clamping should be applied consistently
to the other dimension properties that drive the ratio/division logic. Update
vessel.ts in the VesselView component’s updated method to also normalize
portToCCRP, starboardToCCRP, sensorHeightOverKeel, and vesselHeight to a minimum
of 0 when they change, using the same pattern already used for sternToCCRP and
bowToCCRP.
- Line 393: The `getArrowHalfHeight` callback is being passed around as an
unbound method in the dim/starboard params blocks, which makes its `this` depend
on the caller’s object shape. Update the params assembly in `vessel.ts` so
`getArrowHalfHeight` is consistently bound to the class instance, matching the
existing `getArrowHalfHeight.bind(this)` usage, and apply the same fix in the
other affected params objects that currently pass `this.getArrowHalfHeight`
directly.
- Around line 23-24: The custom element registration in Vessel still uses the
legacy ob- prefix, which is inconsistent with the repo convention. Update the
`@customElement` decorator on Vessel to use the obc- prefix so it matches other
published components and the project’s LitElement naming standard. Keep the
class name the same and only change the registered tag name to the new obc-
form.
- Around line 443-447: The HTMLElementTagNameMap entry is using the wrong custom
element name for Vessel. Update the declaration in the global
HTMLElementTagNameMap so it matches the registered tag used by `@customElement` on
Vessel, and ensure the key is the same tag name that document.createElement
should use for correct typing.
- Line 25: Remove the unused vesselWidth property from the vessel component; it
is declared in the vessel class but never read or used in rendering, so it
should be deleted from the vessel-view/vessel.ts component definition to keep
the API clean.
---
Nitpick comments:
In `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-styles.ts`:
- Around line 64-84: The vessel layout in vessel-styles should not rely on
hardcoded magic numbers for .both-views, .both-views-inner, and .top-down-view.
Update the styles to derive width, scale, and horizontal offset from the
configurable width used by vessel.stories.ts so the vessel view responds
correctly to different sizes instead of clipping or misaligning. Use the
existing vessel-view styling selectors to replace fixed values with dynamic
sizing logic tied to the story args or shared sizing constants.
In `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts`:
- Around line 405-409: Remove the stray comment block in vessel-types.ts under
the top-down view section; it is not needed and does not meet the commenting
guideline. Locate the TOP DOWN VIEW FUNCTIONS marker near the related vessel
type/view helpers and delete that comment-only block without changing any
surrounding logic or symbols.
- Around line 810-827: Remove the large commented-out SVG dead block from
vessel-types.ts; the file should not keep commented code per the guideline.
Locate the trailing commented SVG fragment near the Vessel-related constants and
delete the entire block, leaving only the active vessel type definitions and
render logic intact.
In `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.stories.ts`:
- Line 44: Remove the commented-out dead code from the vessel story by deleting
the leftover width assignment comment in the relevant story configuration.
Update the vessel.stories.ts content around the vessel story so only active
settings remain, and keep the Storybook story definitions clean without
unnecessary comments.
- Line 7: The Storybook metadata in the vessel story uses an all-caps segment in
the title, so update the `title` field in `vessel.stories.ts` to use Title Case
consistently with the project guideline. Adjust the `INSTRUMENT/Vessel` entry in
the story metadata so it follows the same casing convention as other Storybook
`title` and `name` values.
In `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts`:
- Line 36: The boolean property name toggleSLAndDLSensor is unclear and uses
toggle-style wording instead of positive phrasing. Rename the property in the
vessel class to something that describes the enabled state, like
useDoubleLineSensorMode, and update any references in the vessel-view component
to match the new name so true/false meaning is obvious.
- Line 37: Remove the unnecessary inline comments in vessel.ts, including
section labels like “values for SIDE_VIEW” and “COMMON VIEW LOGIC,” the
checkmark-style annotations, and any commented-out code such as the
dimTopY/dimBottomY lines. Keep the implementation in the affected blocks by
relying on the existing structure and symbol names like the SIDE_VIEW logic and
related vessel-view helpers, but delete comments unless they are truly required
for unusually complex behavior.
- Around line 226-228: The toggleVesselViewer() method in the vessel class is
currently a dead no-op and only references a nonexistent sideTopDownViewToggle
property. Remove toggleVesselViewer() if it is not needed, or replace it with a
real implementation that toggles an actual class property/state used by the
vessel view; make sure the method and any referenced field names are consistent
with the rest of the class.
- Around line 230-440: The getVessel() method is doing too much by calculating
both side and top-down view state in one large block, which hurts readability
and testability. Refactor it by extracting the distinct calculation/rendering
pieces into smaller named helpers (for example for side-view geometry, top-down
geometry, and view assembly) and keep getVessel() focused on selecting between
SIDE_VIEW, TOP_DOWN_VIEW, or the combined layout.
🪄 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: 149e1a1f-bee2-486a-9edd-350f54a0ca0e
📒 Files selected for processing (6)
packages/openbridge-webcomponents/oicl-openbridge-webcomponents-2.0.0-next.29.tgzpackages/openbridge-webcomponents/src/skipper/interfaces.tspackages/openbridge-webcomponents/src/skipper/vessel-view/vessel-styles.tspackages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.tspackages/openbridge-webcomponents/src/skipper/vessel-view/vessel.stories.tspackages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/openbridge-webcomponents/src/skipper/interfaces.ts
Comment on lines
+1
to
+4
| import {nothing, svg, type TemplateResult} from 'lit'; | ||
| import {Colors} from '../interfaces.js'; | ||
| import {VesselTypes, ViewType} from './vessel.js'; | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing top-of-module JSDoc for this pure function module.
No JSDoc block describing functionality/usage precedes the first export.
As per coding guidelines, "For pure function modules, place the full JSDoc block at the top of the module above the first export."
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts`
around lines 1 - 4, The module in vessel-types.ts is missing the required
top-of-module JSDoc for a pure function module. Add a full JSDoc block above the
first export in this file, describing the module’s functionality and intended
usage, and place it before the symbols imported and used by the exported helpers
such as vesselTypes and any related template/render functions.
Source: Coding guidelines
Comment on lines
+261
to
+293
| function dimSternToCCRPSide(sideViewParams: getSideViewParams): TemplateResult { | ||
| return svg`${textLabel({ | ||
| posX: sideViewParams.sternTextX - 7, | ||
| posY: 297.656, | ||
| label: sideViewParams.sternToCCRP, | ||
| })} | ||
| <path id="line_3" d="${sideViewParams.sternLine.arrowPath}" fill="${Colors.instrumentRegularSecondaryDif}" fill-opacity="0.97"/> | ||
| <line | ||
| x1="${sideViewParams.sternLine.startX + 4.5}" | ||
| y1="303" | ||
| x2="${sideViewParams.sternLine.endX - 4.5}" | ||
| y2="303" | ||
| stroke="${Colors.instrumentRegularSecondaryDif}" | ||
| stroke-width="1" | ||
| />`; | ||
| } | ||
|
|
||
| function dimBowToCCRPSide(sideViewParams: getSideViewParams): TemplateResult { | ||
| return svg`${textLabel({ | ||
| posX: sideViewParams.bowTextX - 7, | ||
| posY: 297.656, | ||
| label: sideViewParams.bowToCCRP, | ||
| })} | ||
| <path id="line_4" d="${sideViewParams.bowLine.arrowPath}" fill="${Colors.instrumentRegularSecondaryDif}" fill-opacity="0.97"/> | ||
| <line | ||
| x1="${sideViewParams.bowLine.startX + 4.5}" | ||
| y1="303" | ||
| x2="${sideViewParams.bowLine.endX - 4.5}" | ||
| y2="303" | ||
| stroke="${Colors.instrumentRegularSecondaryDif}" | ||
| stroke-width="1" | ||
| />`; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Near-duplicate stern/bow and port/starboard rendering functions.
dimSternToCCRPSide/dimBowToCCRPSide and dimPortToCCRPLine/dimStarboardToCCRPLine are almost identical, differing only by field names. Consider a single parameterized helper for each pair to reduce duplication.
Also applies to: 424-501
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts`
around lines 261 - 293, The stern/bow CCRP rendering helpers are near-duplicates
of each other, and the same pattern also applies to the port/starboard line
renderers. Refactor `dimSternToCCRPSide` and `dimBowToCCRPSide` into a single
parameterized helper that accepts the varying text, line, and label fields from
`getSideViewParams`, and do the same for `dimPortToCCRPLine` and
`dimStarboardToCCRPLine`. Keep the existing SVG output behavior unchanged while
centralizing the shared `textLabel`, `path`, and `line` generation logic.
Comment on lines
+268
to
+274
| <line | ||
| x1="${sideViewParams.sternLine.startX + 4.5}" | ||
| y1="303" | ||
| x2="${sideViewParams.sternLine.endX - 4.5}" | ||
| y2="303" | ||
| stroke="${Colors.instrumentRegularSecondaryDif}" | ||
| stroke-width="1" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Hardcoded y="303" duplicates vessel.ts's LINE_Y constant.
dimSternToCCRPSide/dimBowToCCRPSide hardcode y1="303"/y2="303" instead of using sideViewParams.sternLine.y/bowLine.y (already computed from LINE_Y in buildDimensionLine). If LINE_Y in vessel.ts ever changes, these lines will silently desync from the rest of the dimension rendering.
🔧 Proposed fix
<line
x1="${sideViewParams.sternLine.startX + 4.5}"
- y1="303"
+ y1="${sideViewParams.sternLine.y}"
x2="${sideViewParams.sternLine.endX - 4.5}"
- y2="303"
+ y2="${sideViewParams.sternLine.y}"(apply the equivalent fix using bowLine.y in dimBowToCCRPSide)
Also applies to: 286-292
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel-types.ts`
around lines 268 - 274, The side-dimension SVG lines in `dimSternToCCRPSide` and
`dimBowToCCRPSide` are hardcoded to y=303 instead of using the computed line
position from `buildDimensionLine`. Update these functions to use
`sideViewParams.sternLine.y` and `sideViewParams.bowLine.y` for the `y1`/`y2`
attributes so they stay in sync with the shared `LINE_Y` value used by
`vessel.ts`.
Comment on lines
+60
to
+64
| export const Vessel_cruiser: Story = { | ||
| args: { | ||
| width: 512, | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Vessel_cruiser story name is misleading.
There is no CRUISER entry in VesselTypes (only CARGO, TANKER, FISHING, CAR_FERRY); this story will silently render the default CARGO vessel, not a "cruiser."
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.stories.ts`
around lines 60 - 64, The Vessel_cruiser story name is misleading because
VesselTypes has no CRUISER value and the story currently falls back to the
default CARGO vessel. Update the story in vessel.stories.ts so the exported
story name and any related labels/args match an actual VesselTypes entry, using
the existing VesselTypes and Vessel component story definitions as the reference
points.
Comment on lines
+23
to
+24
| @customElement('ob-vessel-view') | ||
| export class Vessel extends LitElement { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Custom element tag doesn't follow the repo's obc- prefix convention.
ob-vessel-view uses the legacy ob- prefix. This repo's history shows an explicit migration from ob- to obc-, and other published components use tags like obc-top-bar.
As per coding guidelines, "Components must extend LitElement, register with @customElement('obc-…')."
🏷️ Proposed fix
-@customElement('ob-vessel-view')
+@customElement('obc-vessel-view')📝 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.
Suggested change
| @customElement('ob-vessel-view') | |
| export class Vessel extends LitElement { | |
| `@customElement`('obc-vessel-view') | |
| export class Vessel extends LitElement { |
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` around
lines 23 - 24, The custom element registration in Vessel still uses the legacy
ob- prefix, which is inconsistent with the repo convention. Update the
`@customElement` decorator on Vessel to use the obc- prefix so it matches other
published components and the project’s LitElement naming standard. Keep the
class name the same and only change the registered tag name to the new obc-
form.
Source: Coding guidelines
| } | ||
|
|
||
| @customElement('ob-vessel-view') | ||
| export class Vessel extends LitElement { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing class-level JSDoc.
Vessel has no JSDoc block describing its purpose/usage.
As per coding guidelines, "Write JSDoc for concrete components on the class, and keep the documentation domain-agnostic without mentioning maritime or related qualifiers."
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` at line
24, Add a class-level JSDoc block directly above the Vessel class declaration to
document its purpose and intended usage. Use domain-agnostic wording only, and
describe the concrete component at a high level without any maritime-specific
terms; keep the documentation on the Vessel class itself so it matches the
project’s component JSDoc guideline.
Source: Coding guidelines
|
|
||
| @customElement('ob-vessel-view') | ||
| export class Vessel extends LitElement { | ||
| @property({type: Number}) vesselWidth: number = 30; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'vesselWidth' packages/openbridge-webcomponents/src/skipper/vessel-view/Repository: Ocean-Industries-Concept-Lab/openbridge-webcomponents
Length of output: 1189
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.tsRepository: Ocean-Industries-Concept-Lab/openbridge-webcomponents
Length of output: 5574
🏁 Script executed:
#!/bin/bash
sed -n '220,520p' packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.tsRepository: Ocean-Industries-Concept-Lab/openbridge-webcomponents
Length of output: 6740
🏁 Script executed:
#!/bin/bash
rg -n '\bvesselWidth\b' .Repository: Ocean-Industries-Concept-Lab/openbridge-webcomponents
Length of output: 318
Remove the unused vesselWidth property vesselWidth is never read in packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts, so setting it has no visual effect.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` at line
25, Remove the unused vesselWidth property from the vessel component; it is
declared in the vessel class but never read or used in rendering, so it should
be deleted from the vessel-view/vessel.ts component definition to keep the API
clean.
Comment on lines
+147
to
+159
| override updated(changedProps: Map<string, string>) { | ||
| if (changedProps.has('sternToCCRP')) { | ||
| if (this.sternToCCRP < 0) { | ||
| this.sternToCCRP = 0; | ||
| } | ||
| } | ||
|
|
||
| if (changedProps.has('bowToCCRP')) { | ||
| if (this.bowToCCRP < 0) { | ||
| this.bowToCCRP = 0; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent negative-value guarding across dimension properties.
Only sternToCCRP/bowToCCRP are clamped to >= 0; portToCCRP, starboardToCCRP, sensorHeightOverKeel, and vesselHeight feed similar ratio/division logic elsewhere but aren't guarded against negative input.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` around
lines 147 - 159, The negative-value guard in updated() only covers sternToCCRP
and bowToCCRP, but the same clamping should be applied consistently to the other
dimension properties that drive the ratio/division logic. Update vessel.ts in
the VesselView component’s updated method to also normalize portToCCRP,
starboardToCCRP, sensorHeightOverKeel, and vesselHeight to a minimum of 0 when
they change, using the same pattern already used for sternToCCRP and bowToCCRP.
| showSensorDim, | ||
| yAxisCCRPPos: this.yAxisCCRPPos, | ||
| starboardDimensionLine: this.starboardDimensionLine, | ||
| getArrowHalfHeight: this.getArrowHalfHeight.bind(this), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fragile unbound method reference for getArrowHalfHeight.
getArrowHalfHeight: this.getArrowHalfHeight (lines 406, 413) passes the raw unbound method, unlike line 393 which uses .bind(this). It only works today because the receiving params object happens to also carry an ARROW_SIZE field, so this.ARROW_SIZE resolves correctly when later invoked as dimPortParams.getArrowHalfHeight() — relying on the caller's object shape mimicking this. This is fragile: if the shape changes, this.ARROW_SIZE would be undefined.
🔧 Proposed fix
- getMidY: this.getMidY,
portToCCRP: this.portToCCRP,
ARROW_SIZE: this.ARROW_SIZE,
- getArrowHalfHeight: this.getArrowHalfHeight,
+ getArrowHalfHeight: this.getArrowHalfHeight.bind(this),(apply similarly to the starboard params block)
Also applies to: 406-406, 413-413
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` at line
393, The `getArrowHalfHeight` callback is being passed around as an unbound
method in the dim/starboard params blocks, which makes its `this` depend on the
caller’s object shape. Update the params assembly in `vessel.ts` so
`getArrowHalfHeight` is consistently bound to the class instance, matching the
existing `getArrowHalfHeight.bind(this)` usage, and apply the same fix in the
other affected params objects that currently pass `this.getArrowHalfHeight`
directly.
Comment on lines
+443
to
+447
| declare global { | ||
| interface HTMLElementTagNameMap { | ||
| 'ob-vessel': Vessel; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the component file and nearby registrations.
git ls-files 'packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts' \
'packages/openbridge-webcomponents/src/**/vessel*.ts' \
'packages/openbridge-webcomponents/src/**/vessel*.tsx' \
'packages/openbridge-webcomponents/src/**/vessel*.js' \
'packages/openbridge-webcomponents/src/**/vessel*.jsx'
echo
echo "== ast-grep outline =="
ast-grep outline packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts --view expanded || true
echo
echo "== customElement registrations in file =="
rg -n "`@customElement`|customElement\\(" packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts
echo
echo "== HTMLElementTagNameMap entries in file =="
rg -n "HTMLElementTagNameMap|ob-vessel|ob-vessel-view|obc-vessel-view" packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts
echo
echo "== matching tags elsewhere in src =="
rg -n "ob-vessel-view|ob-vessel|obc-vessel-view" packages/openbridge-webcomponents/src -g '!**/node_modules/**'Repository: Ocean-Industries-Concept-Lab/openbridge-webcomponents
Length of output: 6952
HTMLElementTagNameMap should use the registered tag. @customElement('ob-vessel-view') registers ob-vessel-view, but this map still declares 'ob-vessel'. Change it to 'ob-vessel-view' so document.createElement('ob-vessel-view') gets the right typing.
🤖 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 `@packages/openbridge-webcomponents/src/skipper/vessel-view/vessel.ts` around
lines 443 - 447, The HTMLElementTagNameMap entry is using the wrong custom
element name for Vessel. Update the declaration in the global
HTMLElementTagNameMap so it matches the registered tag used by `@customElement` on
Vessel, and ensure the key is the same tag name that document.createElement
should use for correct typing.
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
2 participants
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.
This proposal introduces several new components that could be included in the official OpenBridge component library.
Components
Motivation
These components are commonly used in maritime applications and follow the OpenBridge design language. Including them in the official library would improve consistency across projects and reduce the need for custom implementations.
Benefits
I would be happy to contribute the implementation, documentation, stories and tests for these components.
Summary by CodeRabbit
Release Notes