feat: add automation transmitter - #1020
Open
jon-daeh wants to merge 9 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds ChangesTransmitter button
Transmitter
Indicator graph fill support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
|
The storybook can be viewed at https://openbridge-next-storybook--1020-feat-add-automation-tr-ff9j6mf6.web.app |
|
The demo can be viewed at https://openbridge-next-demo--1020-feat-add-automation-transmi-sjceufsu.web.app |
![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: 4
🧹 Nitpick comments (1)
packages/openbridge-webcomponents/src/automation/transmitter/transmitter.stories.ts (1)
112-120: 📐 Maintainability & Code Quality | 🔵 TrivialRefresh the graph baselines for these stories.
CI is red on
HorizontalGraphandVerticalGraph; if the new visual output is intentional, the matching Storybook snapshots need to be updated in this PR.🤖 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/automation/transmitter/transmitter.stories.ts` around lines 112 - 120, The HorizontalGraph and VerticalGraph stories in transmitter.stories.ts have intentional visual changes, so their Storybook baselines need to be refreshed. Update the matching snapshots for the two Story exports so CI reflects the new graph rendering, using the HorizontalGraph, VerticalGraph, and renderComponent/ObcTransmitter story definitions to locate the affected snapshots.Source: Pipeline failures
🤖 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/automation/transmitter-button/transmitter-button.ts`:
- Around line 66-70: The formattedValue getter in transmitter-button currently
passes fractionDigits straight into toFixed(), so invalid public values can
throw during render. Clamp or normalize fractionDigits inside formattedValue
before formatting, ensuring non-integer, negative, or overly large values fall
back to a safe range. Keep the existing undefined/NaN guard and apply the
bounded digit count only when calling toFixed().
In
`@packages/openbridge-webcomponents/src/automation/transmitter/transmitter.stories.ts`:
- Around line 8-10: The transmitter story renderer is exposing hasAdvice in args
but never renders any advice content, so the advice state is not actually
exercised. Update the story’s render/template logic in transmitter.stories.ts
(including the renderer used by the affected stories) to conditionally pass real
content into the advice slot when hasAdvice is enabled, and keep the slot empty
only when it is disabled. Use the existing args handling and the transmitter
component’s advice slot to wire this up consistently across the affected
stories.
In
`@packages/openbridge-webcomponents/src/navigation-instruments/indicator-graph/indicator-graph.ts`:
- Around line 19-23: Remove the inline comment from ObcIndicatorGraphLayout
since the default for fill is already clear from the type and the repo forbids
unnecessary code comments. Update the interface in indicator-graph.ts so fill
remains documented only through type/JSDoc if needed, and keep the definition of
ObcIndicatorGraphLayout clean without the inline note.
- Line 75: The fill callback in indicator-graph’s update path is only set when
_effectiveFill is truthy, so uPlot keeps the previous series.fill when
layout.fill is later disabled. Update the series configuration in the
indicator-graph component’s fill handling logic to explicitly clear series.fill
(set it to undefined/null) in the false branch, alongside the existing
_getFillColor-based assignment, so toggling fill off removes the callback.
---
Nitpick comments:
In
`@packages/openbridge-webcomponents/src/automation/transmitter/transmitter.stories.ts`:
- Around line 112-120: The HorizontalGraph and VerticalGraph stories in
transmitter.stories.ts have intentional visual changes, so their Storybook
baselines need to be refreshed. Update the matching snapshots for the two Story
exports so CI reflects the new graph rendering, using the HorizontalGraph,
VerticalGraph, and renderComponent/ObcTransmitter story definitions to locate
the affected snapshots.
🪄 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: cea115ea-94f3-406d-aaf0-188b7bac1171
⛔ Files ignored due to path filters (14)
packages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/default-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/large-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/medium-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/regular-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/small-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/tag-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/with-advice-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/with-icon-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter/transmitter.stories.ts/horizontal-graph-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter/transmitter.stories.ts/indicator-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter/transmitter.stories.ts/usage-with-pipe-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter/transmitter.stories.ts/value-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter/transmitter.stories.ts/vertical-graph-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/with-fill-auto.pngis excluded by!**/*.png
📒 Files selected for processing (8)
packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.csspackages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.stories.tspackages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.tspackages/openbridge-webcomponents/src/automation/transmitter/transmitter.csspackages/openbridge-webcomponents/src/automation/transmitter/transmitter.stories.tspackages/openbridge-webcomponents/src/automation/transmitter/transmitter.tspackages/openbridge-webcomponents/src/navigation-instruments/indicator-graph/indicator-graph.stories.tspackages/openbridge-webcomponents/src/navigation-instruments/indicator-graph/indicator-graph.ts
Comment on lines
+66
to
+70
| private get formattedValue() { | ||
| if (this.value === undefined || Number.isNaN(this.value)) { | ||
| return '--'; | ||
| } | ||
| return this.value.toFixed(this.fractionDigits); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clamp fractionDigits before calling toFixed().
fractionDigits is a public property here and is also forwarded unchanged by <obc-transmitter>, so values like -1, 1.5, or 101 will throw a RangeError during render instead of falling back gracefully.
Suggested fix
private get formattedValue() {
if (this.value === undefined || Number.isNaN(this.value)) {
return '--';
}
- return this.value.toFixed(this.fractionDigits);
+ const digits =
+ Number.isFinite(this.fractionDigits) &&
+ this.fractionDigits >= 0 &&
+ this.fractionDigits <= 100
+ ? Math.trunc(this.fractionDigits)
+ : 1;
+ return this.value.toFixed(digits);
}📝 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
| private get formattedValue() { | |
| if (this.value === undefined || Number.isNaN(this.value)) { | |
| return '--'; | |
| } | |
| return this.value.toFixed(this.fractionDigits); | |
| private get formattedValue() { | |
| if (this.value === undefined || Number.isNaN(this.value)) { | |
| return '--'; | |
| } | |
| const digits = | |
| Number.isFinite(this.fractionDigits) && | |
| this.fractionDigits >= 0 && | |
| this.fractionDigits <= 100 | |
| ? Math.trunc(this.fractionDigits) | |
| : 1; | |
| return this.value.toFixed(digits); | |
| } |
🤖 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/automation/transmitter-button/transmitter-button.ts`
around lines 66 - 70, The formattedValue getter in transmitter-button currently
passes fractionDigits straight into toFixed(), so invalid public values can
throw during render. Clamp or normalize fractionDigits inside formattedValue
before formatting, ensuring non-integer, negative, or overly large values fall
back to a safe range. Keep the existing undefined/NaN guard and apply the
bounded digit count only when calling toFixed().
Comment on lines
+8
to
+10
| import '../../icons/icon-temperature-air.js'; | ||
| import '../horizontal-line/horizontal-line.js'; | ||
| import {LineMedium, LineType} from '../index.js'; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wire the advice slot in the story renderer.
hasAdvice is exposed in args, but this renderer never supplies advice content, so toggling that control documents an empty spacer instead of the actual advice state.
Suggested fix
import '../../icons/icon-temperature-air.js';
+import '../../icons/icon-placeholder.js';
import '../horizontal-line/horizontal-line.js';
import {LineMedium, LineType} from '../index.js';
@@
${args.hasIcon
? html`<obi-temperature-air slot="icon"></obi-temperature-air>`
: ''}
+ ${args.hasAdvice
+ ? html`<obi-placeholder slot="advice"></obi-placeholder>`
+ : ''}
</obc-transmitter>
`;
}Also applies to: 80-100
🤖 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/automation/transmitter/transmitter.stories.ts`
around lines 8 - 10, The transmitter story renderer is exposing hasAdvice in
args but never renders any advice content, so the advice state is not actually
exercised. Update the story’s render/template logic in transmitter.stories.ts
(including the renderer used by the affected stories) to conditionally pass real
content into the advice slot when hasAdvice is enabled, and keep the slot empty
only when it is disabled. Use the existing args handling and the transmitter
component’s advice slot to wire this up consistently across the affected
stories.
| { | ||
| stroke: this._getStrokeColor(), | ||
| width: this._getStrokeWidth(), | ||
| fill: this._effectiveFill ? () => this._getFillColor() : undefined, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear series.fill when layout.fill turns off.
Line 171 only assigns series.fill in the truthy branch, so after a component has rendered once with fill: true, changing layout.fill to false leaves the previous uPlot fill callback in place and the graph stays filled.
Suggested fix
const series = this.uplot.series[1];
series.stroke = () => this._getStrokeColor();
series.width = this._getStrokeWidth();
- if (this._effectiveFill) {
- series.fill = () => this._getFillColor();
- }
+ series.fill = this._effectiveFill ? () => this._getFillColor() : undefined;
this.uplot.redraw();Also applies to: 171-173
🤖 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/navigation-instruments/indicator-graph/indicator-graph.ts`
at line 75, The fill callback in indicator-graph’s update path is only set when
_effectiveFill is truthy, so uPlot keeps the previous series.fill when
layout.fill is later disabled. Update the series configuration in the
indicator-graph component’s fill handling logic to explicitly clear series.fill
(set it to undefined/null) in the false branch, alongside the existing
_getFillColor-based assignment, so toggling fill off removes the callback.
jon-daeh
marked this pull request as draft
June 30, 2026 08:46
jon-daeh
marked this pull request as ready for review
July 3, 2026 20:18
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.ts (1)
198-212: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd a live region to the transmitter readout. The changing value/advice/setpoint is only rendered inside the button, so updates aren’t announced to assistive tech. Add
aria-live="polite"andaria-atomic="true"on the root control.🤖 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/automation/transmitter-button/transmitter-button.ts` around lines 198 - 212, The transmitter-button root control in render() does not expose dynamic readout updates to assistive tech. Update the main button element in transmitter-button to include aria-live="polite" and aria-atomic="true" so changing value/advice/setpoint content announced by renderContent() is read correctly. Use the existing render() and renderContent() symbols to place the live region attributes on the root control.Sources: Coding guidelines, Learnings
♻️ Duplicate comments (1)
packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.ts (1)
106-143: 🩺 Stability & Availability | 🟠 MajorUnvalidated
fractionDigitscan still throw via the shared formatters.
numericFormatOptionsforwards the rawfractionDigitspublic property straight intogetHintZeros/formatNumericValue, which ultimately callvalue.toFixed(options.fractionDigits). Negative, non-integer, or >100 values will throw aRangeErrorduring render. This is the same risk previously flagged on the oldformattedValuegetter, just relocated behind the new shared-formatter architecture instead of fixed.🩺 Suggested clamp
private get numericFormatOptions(): ReadoutNumericFormatOptions { + const digits = + Number.isFinite(this.fractionDigits) && + this.fractionDigits >= 0 && + this.fractionDigits <= 100 + ? Math.trunc(this.fractionDigits) + : 1; return { showZeroPadding: this.showZeroPadding, minValueLength: this.minValueLength, - fractionDigits: this.fractionDigits, + fractionDigits: digits, }; }Note the underlying
.toFixed()call itself lives inreadout-formatters.ts(not in this diff), so the more durable fix may belong there for all consumers (obc-readout-setpointhas the identical exposure).🤖 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/automation/transmitter-button/transmitter-button.ts` around lines 106 - 143, The new render path in transmitter-button.ts still passes the raw fractionDigits value through numericFormatOptions into getHintZeros and formatNumericValue, so invalid values can still trigger a RangeError via the shared formatter pipeline. Clamp or sanitize fractionDigits before it reaches numericFormatOptions, preferably by normalizing it in the shared readout-formatters utilities used by formatNumericValue/getHintZeros so transmitter-button and other consumers like the setpoint component are protected consistently.
🧹 Nitpick comments (4)
packages/openbridge-webcomponents/src/automation/transmitter/transmitter.ts (3)
170-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlert wrapping hardcodes type and skips established forwarding contract.
hasAlertonly toggles a boolean, and the wrapper hardcodesObcAlertFrameType.Regularwithout forwardingthickness,status,mode,showIcon, orshowAlertCategoryIcon. Per a prior learning,wrapWithAlertFrame()inalert-frame.tsis the established forwarding contract for alert-frame consumers in this repo. Manually re-implementing the wrapper here duplicates logic and limits the component to a single alert type/severity, which may not match future needs (e.g. critical vs. regular alerts).Consider using the shared
wrapWithAlertFrame()helper (or exposingstatus/modeas properties) for consistency with other automation-family components.Based on learnings, "Treat
packages/openbridge-webcomponents/src/components/alert-frame/alert-frame.ts(wrapWithAlertFrame()) as the source-of-truth forwarding contract for alert-frame consumers."🤖 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/automation/transmitter/transmitter.ts` around lines 170 - 182, The alert wrapping in transmitter render currently hardcodes ObcAlertFrameType.Regular and bypasses the shared forwarding contract. Update transmitter.render() in transmitter.ts to use wrapWithAlertFrame() from alert-frame.ts, or otherwise forward the existing alert-frame inputs instead of duplicating the wrapper logic. Make sure the transmitter component preserves and passes through thickness, status, mode, showIcon, and showAlertCategoryIcon so the wrapper behavior stays consistent with other consumers.Source: Learnings
83-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
@availableWhenfor conditional property docs.
adviceValue,setpointValue, andtagare all conditional on another property/state (hasAdvice,hasSetPoint,type === 'indicator'), but are documented with free-form prose ("whenhasAdvice") rather than the@availableWhenJSDoc tag mandated for dependent properties.📝 Example fix
- /** Advisory value shown in the leading advice segment when `hasAdvice`. */ + /** + * Advisory value shown in the leading advice segment. + * `@availableWhen` hasAdvice + */ `@property`({type: Number}) adviceValue?: number; @@ `@property`({type: Boolean}) hasSetPoint = false; - /** Target value shown in the setpoint segment when `hasSetPoint`. */ + /** + * Target value shown in the setpoint segment. + * `@availableWhen` hasSetPoint + */ `@property`({type: Number}) setpointValue?: number; - /** Tag identifier shown when `type` is `indicator` (e.g. `TT`). */ + /** + * Tag identifier (e.g. `TT`). + * `@availableWhen` type === 'indicator' + */ `@property`({type: String}) tag = '';As per coding guidelines, "Document conditional properties with
@availableWhenon the dependent property only, using the actual runtime/behavior condition."🤖 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/automation/transmitter/transmitter.ts` around lines 83 - 95, The conditional property docs in transmitter.ts use free-form prose instead of the required `@availableWhen` tag. Update the JSDoc for adviceValue, setpointValue, and tag in Transmitter to document each dependent property with `@availableWhen` on the property itself, using the actual runtime condition (hasAdvice, hasSetPoint, and type === 'indicator') and remove the plain-language “when ...” wording.Source: Coding guidelines
63-63: 🧹 Nitpick | 🔵 TrivialReminder: keep
custom-elements.jsonin sync.If
obc-transmitteris a newly registered@customElement, remember to runnpm run analyzeso the manifest stays up to date.As per coding guidelines, "Run
npm run analyzeafter adding or renaming a@customElementsocustom-elements.jsonstays in sync."🤖 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/automation/transmitter/transmitter.ts` at line 63, The ObcTransmitter custom element registration may have changed, so keep custom-elements.json in sync by running npm run analyze after adding or renaming the `@customElement` for obc-transmitter. Update the manifest based on the ObcTransmitter definition so the generated metadata matches the current component registration.Source: Coding guidelines
packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.ts (1)
91-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
@availableWhentags on conditional properties.
adviceValue,setpointValue, andlabelare only meaningful whenhasAdvice,hasSetPoint, andvariant === 'tag'respectively hold, but their inline docs don't use the@availableWhenconvention.📝 Proposed doc fix
- /** Advisory value shown in the leading advice segment when `hasAdvice`. */ + /** + * Advisory value shown in the leading advice segment. + * `@availableWhen` `hasAdvice` is `true`. + */ `@property`({type: Number}) adviceValue?: number; `@property`({type: Boolean}) hasSetPoint = false; - /** Target value shown in the setpoint segment when `hasSetPoint`. */ + /** + * Target value shown in the setpoint segment. + * `@availableWhen` `hasSetPoint` is `true`. + */ `@property`({type: Number}) setpointValue?: number; - /** Short tag identifier shown in the `tag` variant (e.g. `TT`). */ + /** + * Short tag identifier shown in the label (e.g. `TT`). + * `@availableWhen` `variant` is `'tag'`. + */ `@property`({type: String}) label = '';As per coding guidelines, "Document conditional properties with
@availableWhenon the dependent property only, using the actual runtime/behavior condition."🤖 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/automation/transmitter-button/transmitter-button.ts` around lines 91 - 100, Add the missing conditional documentation tags on the dependent properties in transmitter-button.ts: update the JSDoc for adviceValue, setpointValue, and label in TransmitterButton so each includes an `@availableWhen` note that matches the actual runtime condition (hasAdvice, hasSetPoint, and variant === 'tag' respectively). Keep the tags on the properties themselves and align the wording with the existing doc style used in this component.Source: Coding guidelines
🤖 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/automation/transmitter-button/transmitter-button.ts`:
- Around line 145-178: The advice and setpoint readouts are bypassing the
NaN/undefined normalization used by renderValue(), so renderAdvice() and
renderSetpoint() can show literal NaN instead of the placeholder state. Update
the transmitter-button component to normalize adviceValue and setpointValue the
same way normalizedValue is used, and pass the normalized result into
obc-readout-advice and obc-readout-setpoint so both segments share the same
formatting behavior.
---
Outside diff comments:
In
`@packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.ts`:
- Around line 198-212: The transmitter-button root control in render() does not
expose dynamic readout updates to assistive tech. Update the main button element
in transmitter-button to include aria-live="polite" and aria-atomic="true" so
changing value/advice/setpoint content announced by renderContent() is read
correctly. Use the existing render() and renderContent() symbols to place the
live region attributes on the root control.
---
Duplicate comments:
In
`@packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.ts`:
- Around line 106-143: The new render path in transmitter-button.ts still passes
the raw fractionDigits value through numericFormatOptions into getHintZeros and
formatNumericValue, so invalid values can still trigger a RangeError via the
shared formatter pipeline. Clamp or sanitize fractionDigits before it reaches
numericFormatOptions, preferably by normalizing it in the shared
readout-formatters utilities used by formatNumericValue/getHintZeros so
transmitter-button and other consumers like the setpoint component are protected
consistently.
---
Nitpick comments:
In
`@packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.ts`:
- Around line 91-100: Add the missing conditional documentation tags on the
dependent properties in transmitter-button.ts: update the JSDoc for adviceValue,
setpointValue, and label in TransmitterButton so each includes an `@availableWhen`
note that matches the actual runtime condition (hasAdvice, hasSetPoint, and
variant === 'tag' respectively). Keep the tags on the properties themselves and
align the wording with the existing doc style used in this component.
In `@packages/openbridge-webcomponents/src/automation/transmitter/transmitter.ts`:
- Around line 170-182: The alert wrapping in transmitter render currently
hardcodes ObcAlertFrameType.Regular and bypasses the shared forwarding contract.
Update transmitter.render() in transmitter.ts to use wrapWithAlertFrame() from
alert-frame.ts, or otherwise forward the existing alert-frame inputs instead of
duplicating the wrapper logic. Make sure the transmitter component preserves and
passes through thickness, status, mode, showIcon, and showAlertCategoryIcon so
the wrapper behavior stays consistent with other consumers.
- Around line 83-95: The conditional property docs in transmitter.ts use
free-form prose instead of the required `@availableWhen` tag. Update the JSDoc for
adviceValue, setpointValue, and tag in Transmitter to document each dependent
property with `@availableWhen` on the property itself, using the actual runtime
condition (hasAdvice, hasSetPoint, and type === 'indicator') and remove the
plain-language “when ...” wording.
- Line 63: The ObcTransmitter custom element registration may have changed, so
keep custom-elements.json in sync by running npm run analyze after adding or
renaming the `@customElement` for obc-transmitter. Update the manifest based on
the ObcTransmitter definition so the generated metadata matches the current
component registration.
🪄 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: 7af4f193-2dec-4548-8168-06672eeea498
⛔ Files ignored due to path filters (10)
packages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter-button/transmitter-button.stories.ts/zero-padded-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/automation/transmitter/transmitter.stories.ts/zero-padded-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/below-zero-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/enhanced-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/large-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/medium-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/no-zero-line-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/primary-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/small-auto.pngis excluded by!**/*.pngpackages/openbridge-webcomponents/__vis__/linux/__baselines__/navigation-instruments/indicator-graph/indicator-graph.stories.ts/with-range-auto.pngis excluded by!**/*.png
📒 Files selected for processing (7)
packages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.csspackages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.stories.tspackages/openbridge-webcomponents/src/automation/transmitter-button/transmitter-button.tspackages/openbridge-webcomponents/src/automation/transmitter/transmitter.csspackages/openbridge-webcomponents/src/automation/transmitter/transmitter.stories.tspackages/openbridge-webcomponents/src/automation/transmitter/transmitter.tspackages/openbridge-webcomponents/src/navigation-instruments/indicator-graph/indicator-graph.ts
💤 Files with no reviewable changes (1)
- packages/openbridge-webcomponents/src/navigation-instruments/indicator-graph/indicator-graph.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/openbridge-webcomponents/src/automation/transmitter/transmitter.css
Comment on lines
+145
to
+178
| private renderAdvice() { | ||
| if (!this.hasAdvice) { | ||
| return nothing; | ||
| } | ||
| return html` | ||
| <obc-readout-advice | ||
| class="advice" | ||
| .size=${readoutSizeBySize[this.size]} | ||
| .value=${this.adviceValue} | ||
| .fractionDigits=${this.fractionDigits} | ||
| .minValueLength=${this.minValueLength} | ||
| .hasHintedZeros=${this.hasHintedZeros} | ||
| ></obc-readout-advice> | ||
| `; | ||
| } | ||
|
|
||
| private renderSetpoint() { | ||
| if (!this.hasSetPoint) { | ||
| return nothing; | ||
| } | ||
| return html` | ||
| <obc-readout-setpoint | ||
| class="setpoint" | ||
| .variant=${ReadoutSetpointVariant.setpoint} | ||
| .mode=${ReadoutSetpointMode.display} | ||
| .size=${readoutSizeBySize[this.size]} | ||
| .value=${this.setpointValue} | ||
| .fractionDigits=${this.fractionDigits} | ||
| .minValueLength=${this.minValueLength} | ||
| .hasHintedZeros=${this.hasHintedZeros} | ||
| > | ||
| <obi-input-right slot="icon"></obi-input-right> | ||
| </obc-readout-setpoint> | ||
| `; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
adviceValue/setpointValue skip the NaN normalization applied to value.
renderValue() uses this.normalizedValue to coerce NaN/undefined to undefined before formatting, but renderAdvice()/renderSetpoint() pass this.adviceValue/this.setpointValue directly. A NaN input here will render the literal text "NaN" instead of the intended dashed placeholder, unlike the main value segment.
♻️ Suggested fix
private renderAdvice() {
if (!this.hasAdvice) {
return nothing;
}
+ const value =
+ this.adviceValue === undefined || Number.isNaN(this.adviceValue)
+ ? undefined
+ : this.adviceValue;
return html`
<obc-readout-advice
class="advice"
.size=${readoutSizeBySize[this.size]}
- .value=${this.adviceValue}
+ .value=${value}🤖 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/automation/transmitter-button/transmitter-button.ts`
around lines 145 - 178, The advice and setpoint readouts are bypassing the
NaN/undefined normalization used by renderValue(), so renderAdvice() and
renderSetpoint() can show literal NaN instead of the placeholder state. Update
the transmitter-button component to normalize adviceValue and setpointValue the
same way normalizedValue is used, and pass the normalized result into
obc-readout-advice and obc-readout-setpoint so both segments share the same
formatting behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit