Conversation
- remove requirement for hardcoded index.md file - optionally limit the header level reported `auto_toc: 2` will report only level 1 and level 2 headers in the files - scan and report folder contents (not recursively yet).
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR significantly expands and restructures the module development documentation for Companion. It reorganizes the doc hierarchy, renames key sections, adds extensive new guides covering connections (basics and advanced), module lifecycle, and upgrades, while updating the TOC generation plugin and styling for improved navigation and readability. Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (22)
for-developers/module-development/connection-basics/presets.md-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorSmall typo: missing word "need" 🙂
Line 10 reads "you will to call" — looks like "need" slipped away!
Suggested fix
-In order to add presets to a module, you will to call another function in your module. The process is similar to how you define actions and feedbacks. +To add presets to a module, you will need to call another function in your module. The process is similar to how you define actions and feedbacks.for-developers/module-development/connection-advanced/setting-custom-variables.md-1-15 (1)
1-15:⚠️ Potential issue | 🟡 MinorMissing YAML front matter — this might cause unexpected behavior in Docusaurus
Every other doc in this PR includes front matter (
title,sidebar_label,sidebar_position,description). Without it, Docusaurus will try to infer the title from the first content line (the blockquote warning), which probably isn't what you want. Would you mind adding front matter here too? Something like:Suggested addition
+--- +title: Setting Custom Variables +sidebar_label: Custom Variables +description: How to output data to user-owned custom variables from an action. +--- + > This is an experimental idea, that may be removed without noticefor-developers/module-development/connection-basics/upgrade-scripts.md-9-19 (1)
9-19:⚠️ Potential issue | 🟡 MinorA few small text and lint nits throughout the file 📝
A handful of minor things the linter caught, plus a grammar one:
- Line 9: "a on/off" → "an on/off"
- Line 13: "practise" → "practice" (more standard in tech documentation)
- Line 19: The fenced code block is missing a language identifier (e.g.,
```js). Flagged by markdownlint (MD040).Suggested fixes
-A common example is changing a on/off option to be an on/off/toggle option. +A common example is changing an on/off option to be an on/off/toggle option.-Each upgrade script will only get run once for each action and feedback, but it is good practise to write the scripts so that they can be executed multiple times. This will help you when testing your script, or if jumping between versions of companion. +Each upgrade script will only get run once for each action and feedback, but it is good practice to write the scripts so that they can be executed multiple times. This will help you when testing your script, or if jumping between versions of companion.-``` +```js const upgradeScripts = require('./upgrades')for-developers/module-development/connection-basics/upgrade-scripts.md-83-85 (1)
83-85:⚠️ Potential issue | 🟡 MinorSmall grammar nit on the last line 🙂
"Anything changes" → "Any changes"
Suggested fix
##### return result -This tells Companion what had changed in your script. Anything changes made to actions or feedbacks not listed in the result object will be discarded. +This tells Companion what has changed in your script. Any changes made to actions or feedbacks not listed in the result object will be discarded.for-developers/module-development/connection-basics/user-configuration.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorDescription doesn't match the page content — looks copy-pasted from feedback docs 📋
The
descriptionfield says "Module feedback definition details" but this page is about user-config definitions. This text often shows up in search results and meta tags, so it'd be nice to fix it.Suggested fix
-description: Module feedback definition details. +description: Module user-config definition details.for-developers/module-development/connection-advanced/subscribe-unsubscribe-flow.md-44-44 (1)
44-44:⚠️ Potential issue | 🟡 MinorMissing apostrophes: "isnt" → "isn't", "arent" → "aren't"
Suggested fix
-This optimisation isnt necessary for most modules, but if your users end up with hundreds of feedbacks of the same type, this will help ensure there arent performance issues. +This optimisation isn't necessary for most modules, but if your users end up with hundreds of feedbacks of the same type, this will help ensure there aren't performance issues.for-developers/module-development/connection-basics/variables.md-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorSmall typo — "change update" looks like a leftover edit 🙂
Looks like there might be a stray word here. Maybe it should read "define variables and update their values"?
Suggested fix
-This section explains how to define variables and change update their values. +This section explains how to define variables and update their values.for-developers/module-development/connection-advanced/migrating-legacy-to-boolean-feedbacks.md-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorTypo: "trigges" → "triggers", and missing period
Suggested fix
-A benefit of this, is that these boolean feedbacks can be used as conditions in the trigges system. We hope to allow for more complex behaviours with these feedbacks, but before that is worth doing we need more modules to be utilising the new type of feedback +A benefit of this is that these boolean feedbacks can be used as conditions in the triggers system. We hope to allow for more complex behaviours with these feedbacks, but before that is worth doing we need more modules to be utilising the new type of feedback.for-developers/module-development/connection-basics/user-configuration.md-25-25 (1)
25-25:⚠️ Potential issue | 🟡 MinorMissing space between sentences
There's a missing space between "...more details." and "The linked documentation...".
Suggested fix
-The fields you can use here are similar to the ones for actions and feedbacks, but with more limitations. See the [list of field types](./input-field-types.md) for more details.The linked documentation states any limitations that apply when used for the configuration, or if they are not allowed. +The fields you can use here are similar to the ones for actions and feedbacks, but with more limitations. See the [list of field types](./input-field-types.md) for more details. The linked documentation states any limitations that apply when used for the configuration, or if they are not allowed.for-developers/module-development/connection-advanced/migrating-legacy-to-boolean-feedbacks.md-138-140 (1)
138-140:⚠️ Potential issue | 🟡 MinorStep numbering jumps from 2 to 4, and small typo 📋
The steps go
### 1,### 2, then### 4— step 3 is missing! Either there's content that hasn't been added yet, or the numbering just needs adjusting.Also on line 140: "help with self" → "help with this".
Suggested fix
-### 4. Add an upgrade script +### 3. Add an upgrade script -Users will have feedbacks assigned to buttons already, and these will all need updating to the new format. A helper has been added to help with self. +Users will have feedbacks assigned to buttons already, and these will all need updating to the new format. A helper has been added to help with this.for-developers/module-development/connection-advanced/subscribe-unsubscribe-flow.md-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorMissing opening backtick breaks inline code formatting
this.unsubscribeActions()is missing its opening backtick, so it'll render as plain text with a stray backtick.Suggested fix
-For actions, the methods `this.subscribeActions()` and this.unsubscribeActions()` can be used instead +For actions, the methods `this.subscribeActions()` and `this.unsubscribeActions()` can be used instead.for-developers/module-development/connection-basics/feedbacks.md-148-148 (1)
148-148:⚠️ Potential issue | 🟡 MinorTODO marker visible to readers
_TODO the linked page doesn't say much?_This note would be visible in the published docs. It might be worth removing it or converting it to an HTML comment (
<!-- TODO ... -->) so it's hidden from readers.for-developers/module-development/connection-basics/actions.md-99-99 (1)
99-99:⚠️ Potential issue | 🟡 MinorBroken link:
../api-changes/companion-4.1.mddoesn't resolve 🔗Hey there! The build pipeline is flagging that this link can't be resolved. It looks like the
api-changes/companion-4.1.mdfile might not exist yet in this PR. Would you mind either adding that file, updating the link to point to the correct location, or perhaps leaving just the version reference without the link for now?for-developers/module-development/connection-basics/input-field-types.md-77-77 (1)
77-77:⚠️ Potential issue | 🟡 MinorTODO marker visible to readers
_TODO: write an advanced page for this? or add to user-config?_Same note as the other files — this will show up in published docs. Consider hiding it in an HTML comment or removing it before merge.
for-developers/module-development/connection-basics/feedbacks.md-110-110 (1)
110-110:⚠️ Potential issue | 🟡 MinorBroken link:
../api-changes/companion-4.1.mddoesn't resolveSame issue as in
actions.md— the pipeline flags this link as unresolvable. Likely the target file hasn't been added to this PR yet.for-developers/module-development/connection-basics/actions.md-105-107 (1)
105-107:⚠️ Potential issue | 🟡 MinorTODO marker left in published documentation 📝
Just a heads-up — this TODO is visible to readers:
TODO: Was the following true for actions or only feedbacks?
Totally fine to have TODOs during development, but it'd be great to resolve this before merging so readers don't see unfinished notes. If you're not sure of the answer yet, maybe wrap the uncertain paragraph in a
:::cautionadmonition or remove it for now?Would you like me to open an issue to track this TODO so it doesn't get lost?
for-developers/module-development/connection-basics/feedbacks.md-65-70 (1)
65-70:⚠️ Potential issue | 🟡 MinorMissing comma in JS example — syntax error 🐛
There's a missing comma after the
labelvalue on line 67. This would be a syntax error if someone copies this example directly.Suggested fix
options: [{ type: 'number', - label: 'Source' + label: 'Source', id: 'source', default: 1 }],for-developers/module-development/connection-basics/feedbacks.md-87-87 (1)
87-87:⚠️ Potential issue | 🟡 MinorTypo: missing space in "documentationlinked"
Small typo — "documentationlinked" should be "documentation linked".
Suggested fix
-There are more properties available in feedback definitions, which are described in full in the documentationlinked in the [previous section](`#feedback-types`). +There are more properties available in feedback definitions, which are described in full in the documentation linked in the [previous section](`#feedback-types`).for-developers/module-development/connection-basics/input-field-types.md-22-24 (1)
22-24:⚠️ Potential issue | 🟡 MinorMissing comma in JS example — syntax error
Same issue as in
feedbacks.md: missing comma after thelabelstring on line 23.Suggested fix
{ type: 'number', - label: 'Source' + label: 'Source', id: 'source',for-developers/module-development/connection-basics/feedbacks.md-22-22 (1)
22-22:⚠️ Potential issue | 🟡 MinorCopy-paste slip: "actions" should be "feedbacks" ✏️
Looks like this sentence was borrowed from the actions page — it says "split the actions definitions" but since we're in the feedbacks doc, it should say "feedbacks definitions" (or "feedback definitions").
Suggested fix
-The [TypeScript module template](https://github.com/bitfocus/companion-module-template-ts) includes a file `src/feedbacks.ts` which is where your feedbacks should be defined. It is not required to use this structure, but it keeps it more readable than having everything in one file. More complex modules will likely want to split the actions definitions into even more files/folders. +The [TypeScript module template](https://github.com/bitfocus/companion-module-template-ts) includes a file `src/feedbacks.ts` which is where your feedbacks should be defined. It is not required to use this structure, but it keeps it more readable than having everything in one file. More complex modules will likely want to split the feedback definitions into even more files/folders.for-developers/module-development/connection-basics/input-field-types.md-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorIncomplete sentence: "When defining an ," ✂️
It looks like a word got lost here! The sentence reads "When defining an ," which is missing its noun. Perhaps it should be something like "When defining an action, feedback, or config"?
Suggested fix
-When defining an , the actions, feedbacks and module config definition object includes a property called `options:` that takes a list of input-field definitions. For example, +When defining an action, feedback, or module config, the definition object includes a property called `options:` that takes a list of input-field definitions. For example,for-developers/module-development/connection-advanced/bonjour-device-discovery.md-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorMissing YAML front matter 📋
All the other new docs in this PR include front matter (
---block with title, sidebar_label, sidebar_position, description). This file is missing it, which could affect how Docusaurus renders the page title, sidebar entry, and meta description. Would you mind adding something like:+--- +title: Bonjour Device Discovery +sidebar_label: Bonjour Discovery +description: How to use Bonjour (MDNS) for device discovery in Companion modules. +--- + Bonjour is a standardised method of device discovery, utilising MDNS.
🧹 Nitpick comments (10)
for-developers/module-development/index.md (1)
1-7: Small suggestion on the description — totally optional!
"Outline Module Development"reads a bit like a verb phrase rather than a description. Something like"An overview of module development for Companion"might flow more naturally, but no big deal either way. 🙂for-developers/module-development/connection-basics/presets.md (3)
12-12: Heading level jump flagged by markdownlint (MD001)The first heading after the doc title (h1) is
###(h3). Markdown best practice is to increment heading levels one at a time, so this should be##. The same applies to the other###headings (lines 49, 59, 113) — they'd naturally become##as well. Totally up to you whether to address now or later!
55-55: Link text "here" isn't very descriptive (MD059)Screen readers and accessibility tools benefit from meaningful link text. A small tweak would help:
Suggested fix
-You can see the full list of values that can be set and their valid values in the `style` object [here](https://bitfocus.github.io/companion-module-base/interfaces/CompanionButtonStyleProps.html) +You can see the full list of values that can be set and their valid values in the [`style` object documentation](https://bitfocus.github.io/companion-module-base/interfaces/CompanionButtonStyleProps.html)
70-89: Minor: inconsistent indentation in the code exampleLine 75 (
rotate_left) has an extra leading space compared to the surrounding properties (down,up,rotate_right). Just a tiny formatting nit — no rush!src/remark/autoTocPlugin.mjs (2)
96-98:startsWithis slightly too loose for filename exclusion — consider===insteadUsing
startsWith(vfilename)means if your current file is e.g.index.md, it would also exclude a hypotheticalindex.md.bakor similar. A strict equality check would be more precise and avoids surprises.Suggested fix
- const files = readdirSync(dir).filter((f) => f.endsWith('.md') && !f.startsWith(vfilename)) + const files = readdirSync(dir).filter((f) => f.endsWith('.md') && f !== vfilename)
186-200: Subdirectory rendering uses baretextandbreaknodes at the document root — may render unpredictably 🌲The tree-drawing characters (├──, └──) with braille blanks for indentation are a creative approach! Just a heads-up though: bare
textandbreaknodes outside of aparagraphin mdast aren't standard, and different renderers (or future Docusaurus versions) may handle them inconsistently. Screen readers could also announce the braille characters (U+2800) unexpectedly.If you run into rendering quirks, wrapping each sub-file entry in a
paragraphnode (or using a nested list) would be more robust. No action required right now if it's rendering fine for you — just flagging it as something to be aware of!for-developers/module-development/connection-basics/actions.md (1)
91-93: Consider more descriptive link text for accessibilityThe markdownlint tool flags generic link text like "here" (MD059). More descriptive text helps screen readers and makes scanning easier. For example:
Suggested improvement
-There are more properties available, which are described in full in [the autogenerated Actions documentation on GitHub](https://bitfocus.github.io/companion-module-base/interfaces/CompanionActionDefinition.html) +There are more properties available, which are described in full in [the autogenerated Actions documentation on GitHub](https://bitfocus.github.io/companion-module-base/interfaces/CompanionActionDefinition.html). -The `options` property of the action definition is an array of input types, as defined [here](./input-field-types.md) +The `options` property of the action definition is an array of input types, as defined in [Input Field Types](./input-field-types.md).for-developers/module-development/connection-advanced/bonjour-device-discovery.md (1)
38-56: Code block missing language specifierThis fenced code block doesn't have a language tag, so it won't get syntax highlighting. Adding
jswould be consistent with the other code blocks in this file and across the PR.Suggested fix
-``` +```js { type: 'textinput',for-developers/module-development/connection-basics/feedbacks.md (2)
93-93: Heading level skips from h2 to h4The markdownlint tool flags this (MD001): heading levels should increment by one. Since the parent is
##, this should be###instead of####.Suggested fix
-#### Inverting boolean feedbacks +### Inverting boolean feedbacks
99-104: Code block missing language specifierAdding a language tag (e.g.,
js) would enable syntax highlighting and be consistent with the rest of the doc.Suggested fix
-``` +```js CreateUseBuiltinInvertForFeedbacksUpgradeScript({
for-developers/module-development/connection-advanced/learn-action-feedback-values.md
Outdated
Show resolved
Hide resolved
for-developers/module-development/connection-advanced/migrating-legacy-to-boolean-feedbacks.md
Outdated
Show resolved
Hide resolved
for-developers/module-development/connection-advanced/migrating-legacy-to-boolean-feedbacks.md
Show resolved
Hide resolved
for-developers/module-development/connection-advanced/migrating-legacy-to-boolean-feedbacks.md
Outdated
Show resolved
Hide resolved
for-developers/module-development/connection-advanced/subscribe-unsubscribe-flow.md
Outdated
Show resolved
Hide resolved
for-developers/module-development/connection-basics/upgrade-scripts.md
Outdated
Show resolved
Hide resolved
|
I'm not keen on that http://localhost:4005/for-developers/module-development page. I feel like this would be better as a handcrafted page. For http://localhost:4005/for-developers/module-development/connection-basics/, I wonder if this auto-toc is beneficial, vs the other category layout. Because the headings only really make sense within the context of the page they are from. |
There seems to be alot of overlap between module-development-101.md, your new overview.md, and user-configuration.md I realized we're missing two big issues: the module class itself, and establishing connections. So I added stuff. Copy-edited: - actions.md - overview.md Added: - connecting.md Updated: - variables.md - presets.md (typos caught something in v2.0.md)
I hear your argument, especially wrt the API log needing the auto-toc more than other sections, but think it's still better than the silly cards. Maybe we can refine later? At worst, we can remove it from this PR if we can't agree too easily.
Not inconsistent! Just to be sure we're on the same page: they indicate whether it's an in-page link vs. a file in a subdir. "├──" is pretty standard notation for "text folder-trees" Put a bit more extravagantly: It's like saying this message is inconsistent because some lines start with a vertical line, some with a bullet and some are flush left! (I suppose you could say I'm inconsistent in not showing headings for files in the subdirs, but that's the point at which I start thinking "its quite a wall of links.") That said, we can certainly debate styling or whether or how much should be there at all. My thought is that the current design provides a little more depth/overview than the sidebar, which starts with all categories closed.
Not sure I understand this concern: isn't the context right there in the auto-toc as well? I think in general, its value is more in giving an overview of the section-structure than providing in-page links, though both can be useful... FWIW, I'd just as soon not do anything hand-crafted because these pages are likely-as-not to be overlooked -- you only get it if you click the name of the group as opposed to clicking on the caret... OK, stepping off for today. All of my local changes have been pushed to here. |
- minor fix to autToc - transfer bonjour.png from a "hidden" asset - fix typos (feedbacks.md)
build now works but you may have to run `yarn docusaurus clear` to clear the build caches (deleting /build is likely not enough).
|
Important note from the commit comment:
done for (my) today. |
plus minor fixes
…s' into chore_transfer_main_module_docs
- CSS: make H2 and H3 smaller -- more like the default ifm. (Docusaurus makes them much too big and with too much spacing) - fix two autotoc bugs -- quotes were shown if a title had quotes in it -- clicking on the link to an index worked, but then all links in that file were broken.
This completes the transfer of all files from the wiki note: I also put module-debugging at the top level of modules
surprisingly most of its comments were still valid!
With the CSS change, the text size is unchanged, so this is more consistent (previously there was no H2, only H1 and H3 in the files).
|
@coderabbitai do not flag "unhidden" TODO comments separately -- they are useful to let the reader know that something may be out of date. (It's ok to have a single comment listing all TODOs). |
|
I'll kick off the review now! ✏️ Learnings added
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
for-developers/module-development/api-changes/v1.8.md (1)
14-18:⚠️ Potential issue | 🟡 MinorTiny grammar nit in the tip block — remove the comma and clarify the closing sentence.
The comma after "API" on line 16 creates an awkward pseudo-imperative reading, and "those" in the last sentence is a bit ambiguous. A small tweak would make it much cleaner:
✏️ Suggested wording
-Newer versions of the API, replace this with newer ways of achieving the same result. Only follow this if you can't update to those +Newer versions of the API replace this with newer ways of achieving the same result. Only follow this section if you cannot update to a newer API version.for-developers/module-development/api-changes/v2.0.md (2)
27-27:⚠️ Potential issue | 🟡 MinorTiny typo in the package name heading:
@companion-mdoule/base→@companion-module/baseEasy to miss! The section heading on line 27 has
mdouleinstead ofmodule. Worth fixing while we're touching this file so developers don't copy the wrong package name.✏️ Suggested fix
-### `@companion-mdoule/base` is now ESM +### `@companion-module/base` is now ESM
255-255:⚠️ Potential issue | 🟡 MinorUnclosed backtick in heading may break rendering
The heading reads
### Referencing expressions from `isVisibleExpression— the inline-code backtick is never closed, which most Markdown renderers will propagate as a runaway code span, potentially mangling everything that follows until the next backtick.✏️ Suggested fix
-### Referencing expressions from `isVisibleExpression +### Referencing expressions from `isVisibleExpression`for-developers/module-development/api-changes/v1.13.md (1)
52-58:⚠️ Potential issue | 🟡 MinorA few typos in the new
subscribe/unsubscribesection 📝
- Line 52 (heading):
"Action/"→"Actions/"(missing the plurals)- Line 56:
"noisey"→"noisy"- Line 58:
"subscrube"→"subscribe"✏️ Suggested fixes
-## subscribe/unsubscribe: More flexibility for Action/ less for feedbacks {`#subscribe`} +## subscribe/unsubscribe: More flexibility for Actions / less for feedbacks {`#subscribe`}-They can also specify which options should not trigger a call to subscribe/unsubscribe, allowing you to filter out noisey/excessive calls. +They can also specify which options should not trigger a call to subscribe/unsubscribe, allowing you to filter out noisy/excessive calls.-Feedback subscrube/unsubscribe callbacks are no longer called for every options change +Feedback subscribe/unsubscribe callbacks are no longer called for every options changefor-developers/module-development/home.md (1)
64-64:⚠️ Potential issue | 🟡 MinorSingle-item numbered list looks incomplete.
The
Install the dependenciessection has1. yarn installas the only step. If this is intentionally a single instruction, a plain bullet- \yarn install`reads better. If more steps (e.g.,yarn build`) were intended, they seem to have been left out.
🟡 Minor comments (45)
for-developers/module-development/api-changes/v1.8.md-25-25 (1)
25-25:⚠️ Potential issue | 🟡 MinorSmall grammar nip: "suggest them" is missing an indirect object.
"suggest them whilst they type" reads a little oddly — "them" needs a recipient. Something like "suggest these to users as they type" flows more naturally.
✏️ Suggested wording
-This allows us to show a hint to the user about this support, and suggest them whilst they type. +This allows us to show a hint to the user about this support, and suggest these variables to them as they type.for-developers/linking-to-companion.md-21-21 (1)
21-21:⚠️ Potential issue | 🟡 MinorTiny pre-existing nit: double "not" on line 21.
"you may not not change its color"— onenottoo many! This is untouched by the current PR, but since you're already editing nearby, it'd be an easy fix to sneak in if you'd like. Totally up to you 😊✏️ Suggested fix
-You can scale the image to fit in your website, as long as it is still readable, you may not not change its color, rotation, animate it or change it in any other way than scaling. +You can scale the image to fit in your website, as long as it is still readable, you may not change its color, rotation, animate it or change it in any other way than scaling.for-developers/module-development/connection-basics/connecting.md-107-107 (1)
107-107:⚠️ Potential issue | 🟡 MinorTypo in the TelnetHelper link text — "Telent" → "Telnet"
The URL is already correct (
TelnetHelperEvents.html), but the visible link text reads "TelentHelperEvents" — just two transposed letters to swap. 😊✏️ Proposed fix
-- [TelentHelperEvents](https://bitfocus.github.io/companion-module-base/interfaces/TelnetHelperEvents.html) +- [TelnetHelperEvents](https://bitfocus.github.io/companion-module-base/interfaces/TelnetHelperEvents.html)for-developers/module-development/connection-basics/connecting.md-8-19 (1)
8-19:⚠️ Potential issue | 🟡 MinorA few small wording/grammar nits in the intro 📝
Nothing blocking, just a handful of easy polish items:
- Line 8: "One of the most first tasks" → "One of the first tasks" (drop the redundant "most").
- Line 12: "builtin" → "built-in"; "fetch api" → "fetch API" (standard capitalisation for the Web API name).
- Line 19: "possible TCP event --" → "possible TCP events --" (plural, since several events are listed right below).
✏️ Proposed fixes
-One of the most first tasks most modules have to perform is to connect to their device. +One of the first tasks most modules have to perform is to connect to their device.-Or if the device uses HTTP, we recommend using the builtin [`fetch` api](https://nodejs.org/en/learn/getting-started/fetch). +Or if the device uses HTTP, we recommend using the built-in [`fetch` API](https://nodejs.org/en/learn/getting-started/fetch).-You define the various `on()` callbacks to respond to possible TCP event -- see the example, below. +You define the various `on()` callbacks to respond to possible TCP events — see the example below.for-developers/module-development/connection-basics/connecting.md-37-77 (1)
37-77:⚠️ Potential issue | 🟡 MinorTwo syntax errors in the TCPHelper code example — worth fixing before this goes live! 🐛
Great example overall — two small bugs crept in that would trip up anyone copy-pasting it:
- Line 50 — mismatched quotes: the string opens with
'but closes with".- Line 70 — the
tcp.on('status_change', ...)call is missing its closing).🐛 Proposed fixes
tcp.on('connect', () => { this.updateStatus(InstanceStatus.Ok) - this.log('debug', 'Connected!") + this.log('debug', 'Connected!') })tcp.on('status_change', (state, message) => { this.updateStatus(state, message) this.log('debug', 'Status Changed to ' + state + (message != undefined ? ': ' + message : '')) - } + })for-developers/module-development/module-debugging.md-8-9 (1)
8-9:⚠️ Potential issue | 🟡 MinorTiny typo: "will be come" → "will become"
✏️ Proposed fix
-Once you've started coding, debugging the code will be come essential to your success. +Once you've started coding, debugging the code will become essential to your success.for-developers/module-development/module-debugging.md-59-63 (1)
59-63:⚠️ Potential issue | 🟡 MinorDouble-"not" typo in the warning callout
"may not not possible" is missing "be" and has an extra "not".
✏️ Proposed fix
-It may not not possible to debug the `init` method from your module with this… +It may not be possible to debug the `init` method from your module with this…for-developers/module-development/module-debugging.md-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorTwo minor grammar nits flagged by the linter
- Line 17: "module specific" → "module-specific" (compound modifier before a noun should be hyphenated)
- Line 57: "builtin" → "built-in"
✏️ Proposed fixes
-You can open the module specific log view from the connections page by clicking… +You can open the module-specific log view from the connections page by clicking…-You can use any compatible debugger such as the builtin VS Code debugger… +You can use any compatible debugger such as the built-in VS Code debugger…for-developers/module-development/connection-advanced/oauth.md-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorMissing period at end of sentence
"…as support is improved" is missing a closing
.✏️ Suggested fix
-Described below is the current recommended way of supporting OAuth, but many existing modules are not using this. Expect this to be refined in the future, as support is improved +Described below is the current recommended way of supporting OAuth, but many existing modules are not using this. Expect this to be refined in the future, as support is improved.for-developers/module-development/connection-advanced/http-handler.md-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorTypo: missing possessive apostrophe — "Companions" → "Companion's"
✏️ Suggested fix
-Companions webserver that serves the Web UI also provides a path for each connection... +Companion's webserver that serves the Web UI also provides a path for each connection...for-developers/module-development/connection-advanced/oauth.md-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorBroken markdown link —
(HTTP handler)isn't a valid URLThe link target
HTTP handler(a plain string with a space) won't resolve. Based on the sibling file, the correct relative path should be./http-handler.md.🔗 Suggested fix
-To receive the authentication code, you should implement the [handleHttpRequest](HTTP handler) method, listening for the `/oauth/callback` path. +To receive the authentication code, you should implement the [handleHttpRequest](./http-handler.md) method, listening for the `/oauth/callback` path.for-developers/module-development/connection-advanced/oauth.md-47-56 (1)
47-56:⚠️ Potential issue | 🟡 Minor
responseis used but never declared — sample code will throw aReferenceErrorAfter the
// TODOplaceholder, lines 54–55 referenceresponse.data.accessToken/response.data.refreshTokenwithoutresponseever being assigned. Anyone copying this snippet as-is will hit a runtime error. Even as example code, adding a commented placeholder variable makes the intent clear and prevents confusion.✏️ Suggested clarification
try { // Exchange code for token - // TODO: Implement your logic here + // TODO: Replace with your actual token-exchange HTTP call, e.g.: + // const response = await axios.post('https://...', { code: authCode, ... }) //Save new values to Configurationsrc/remark/autoTocPlugin.mjs-99-100 (1)
99-100:⚠️ Potential issue | 🟡 Minor
startsWithmay accidentally exclude sibling filesHey, nice work on this! One thing I noticed — on line 99,
!f.startsWith(vfilename)would also exclude files that happen to start with the same prefix. For example, if the current file isindex.md, a sibling namedindex-extra.mdwould be filtered out too. An exact-match check (f !== vfilename) seems safer here.Also a small optimization opportunity:
readdirSync(dir)is called twice (lines 99 and 100). You could stash the result once and filter from it.Suggested tweak
- const files = readdirSync(dir).filter((f) => f.endsWith('.md') && !f.startsWith(vfilename)) - const subdirs = readdirSync(dir).filter((f) => existsSync(join(dir, f, '_category_.json'))) + const dirEntries = readdirSync(dir) + const files = dirEntries.filter((f) => f.endsWith('.md') && f !== vfilename) + const subdirs = dirEntries.filter((f) => existsSync(join(dir, f, '_category_.json')))src/css/custom.css-272-285 (1)
272-285:⚠️ Potential issue | 🟡 MinorHeading typography overrides look well-reasoned — minor Stylelint formatting nit
Love the detailed comment block documenting the defaults! The sizing choices make sense for documentation readability.
Stylelint flags lines 274 and 280 for
declaration-empty-line-before— it expects an empty line between custom property declarations and regular declarations within the same rule block:Formatting fix to satisfy Stylelint
.markdown h1 { --ifm-h1-vertical-rhythm-bottom: 1; + font-size: 2em; /* setting --ifm-h1-font-size doesn't work (because of specificity?) */ } .markdown h2 { --ifm-h2-vertical-rhythm-top: 1; /* default 2 is too much. Also, we already indent everything under H2 to make H2 breaks clear */ --ifm-heading-vertical-rhythm-bottom: calc(0.5 / 1.25); /* this is multiplied by --ifm-leading = 1.25 */ + font-size: 1.5em; }src/remark/autoTocPlugin.mjs-126-128 (1)
126-128:⚠️ Potential issue | 🟡 MinorSame
startsWithconcern here for excluding the index fileSimilar to the sibling-file filter above — if
indexfileis"overview", thenf.startsWith("overview")also excludes"overview-advanced.md". An equality or slug-based comparison would be more precise:Suggested tweak
- const subfiles = readdirSync(subdir).filter((f) => f.endsWith('.md') && !f.startsWith(indexfile)) + const subfiles = readdirSync(subdir).filter( + (f) => f.endsWith('.md') && f !== indexfile && f !== `${indexfile}.md` + )This handles both the case where
indexfilealready includes.md(the default'index.md') and where it's just the doc id (e.g.,"overview").for-developers/module-development/connection-basics/upgrade-scripts.md-79-79 (1)
79-79:⚠️ Potential issue | 🟡 MinorFile extension inconsistency:
.jsvs.ts🔍Line 15 introduces the file as
src/upgrades.ts, but line 79 refers to it asupgrades.js. Since the TypeScript template is the primary focus throughout this doc, aligning to.tshere would keep things consistent.✏️ Suggested fix
-We recommend defining the functions in a dedicated `upgrades.js` file, as they should not depend on your main class and this helps avoids files growing too long to be manageable. +We recommend defining the functions in a dedicated `upgrades.ts` file, as they should not depend on your main class and this helps avoid files growing too long to be manageable.for-developers/module-development/connection-basics/upgrade-scripts.md-46-46 (1)
46-46:⚠️ Potential issue | 🟡 MinorSmall typo: "you module" → "your module" 📝
The LanguageTool hint flags this correctly — "this function will pass to Companion you module class" should be "your module class".
✏️ Suggested fix
-The main entrypoint for modules, as described in the [overview page](./overview.md) is the call `runEntrypoint(ModuleInstance, UpgradeScripts)` that you typically place at the top-level of _src/main.ts_ (if you're using the [recommended file structure](../module-setup/file-structure.md)). When Companion loads the "main" file, this function will pass to Companion you module class and a list of upgrade scripts, as will be described here. +The main entrypoint for modules, as described in the [overview page](./overview.md) is the call `runEntrypoint(ModuleInstance, UpgradeScripts)` that you typically place at the top-level of _src/main.ts_ (if you're using the [recommended file structure](../module-setup/file-structure.md)). When Companion loads the "main" file, this function will pass to Companion your module class and a list of upgrade scripts, as will be described here.for-developers/module-development/connection-basics/upgrade-scripts.md-140-152 (1)
140-152:⚠️ Potential issue | 🟡 MinorMissing comma in code example — syntax error! 🐛
After the closing
}of theoptionsblock (Line 147), there's no comma beforeisInverted(Line 148). This would fail to parse as valid TypeScript/JavaScript and could trip up anyone copying this example.🐛 Proposed fix
feedbacks: [ { id: 'abc', // You must not edit this, or Companion will ignore any other changes controlId: 'bank:def', // This is readonly feedbackId: 'my-action', options: { valA: { isExpression: false, value: 1 }, - } + }, isInverted: { isExpression: false, value: false } } ]for-developers/module-development/connection-basics/upgrade-scripts.md-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorTiny typo: double "the" 👀
"...the actions and feedbacks the the user has already added..." should be "that the user".
✏️ Suggested fix
-When this happens, existing usages of the action or feedback may become broken. The job of the upgrade script is to fix up the actions and feedbacks the the user has already added to their site to handle the changes. +When this happens, existing usages of the action or feedback may become broken. The job of the upgrade script is to fix up the actions and feedbacks that the user has already added to their site to handle the changes.for-developers/module-development/connection-basics/feedbacks.md-149-162 (1)
149-162:⚠️ Potential issue | 🟡 MinorMissing commas in three code examples will cause JS syntax errors if copy-pasted 🧩
The same comma is missing after
label: 'Source'(orlabel: 'text') in three separate examples — all consistent with the same pattern that was caught and fixed inmigrating-legacy-to-boolean-feedbacks.md.🐛 Proposed fixes (all three occurrences)
- label: 'text' + label: 'text',- label: 'Source' + label: 'Source',(applies to both the "Since API 2.0" and "In API 1.15 and earlier" subscribe examples)
Also applies to: 206-221, 243-263
for-developers/module-development/connection-basics/feedbacks.md-191-193 (1)
191-193:⚠️ Potential issue | 🟡 MinorCopy-paste slip: "action definition" should be "feedback definition" here 🔍
Line 192 reads: "you can customise the sort order of the feedbacks by setting the
sortNameproperty on an action definition" — but this is the feedbacks doc and the property belongs on a feedback definition.✏️ Suggested fix
-Since [API 2.0](../api-changes/v2.0.md), you can customise the sort order of the feedbacks by setting the `sortName` property on an action definition. +Since [API 2.0](../api-changes/v2.0.md), you can customise the sort order of the feedbacks by setting the `sortName` property on a feedback definition.for-developers/module-development/connection-advanced/learn-action-feedback-values.md-42-42 (1)
42-42:⚠️ Potential issue | 🟡 MinorStray
>at the end of line 42This looks like an accidental character left from the admonition syntax:
"otherwise they would become undefined>"→ should end with a period.-Prior to API 2.0, it was recommended to include all options in the result, otherwise they would become undefined> +Prior to API 2.0, it was recommended to include all options in the result, otherwise they would become undefined.for-developers/module-development/module-lifecycle/module-packaging.md-133-135 (1)
133-135:⚠️ Potential issue | 🟡 MinorExtraneous "it" in this sentence
-we do not mind modules enabling if it they have a reason to. +we do not mind modules enabling if they have a reason to.for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md-295-309 (1)
295-309:⚠️ Potential issue | 🟡 MinorSyntax error in the "before" code example — missing closing quote 🐛
Line 298 has an unclosed string literal:
this.setVariable('three, undefined)— the closing'after'threeis missing, making it unparseable JS if copied.🐛 Proposed fix
-this.setVariable('three, undefined) +this.setVariable('three', undefined)for-developers/module-development/module-lifecycle/module-packaging.md-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorTiny typo: "name of version" → "name or version"
-The module name of version are taken from your _package.json_ file +The module name or version are taken from your _package.json_ filefor-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md-467-511 (1)
467-511:⚠️ Potential issue | 🟡 MinorA handful of small typos in the later steps 📝
- Line 469:
"you should fine a line"→"you should find a line"- Line 490:
"slow on some oses, any by combining"→"slow on some OSes, and by combining"- Line 509:
"beneficical"→"beneficial"✏️ Suggested fixes
-At the bottom of your file you should fine a line looking something like +At the bottom of your file you should find a line looking something like-Loading code spread across hundreds of files is surprisingly slow on some oses, any by combining +Loading code spread across hundreds of files is surprisingly slow on some OSes, and by combining-Have an idea of a new connection helper that would be beneficical to you? +Have an idea of a new connection helper that would be beneficial to you?for-developers/module-development/connection-advanced/migrating-legacy-to-boolean-feedbacks.md-61-85 (1)
61-85:⚠️ Potential issue | 🟡 Minor
label:in the "To" example should bename:🔑The migration target (boolean type) on line 64 still uses
label:, but in the Companion 3.x APIlabelwas renamed toname. The "From" (advanced type) example right above it already correctly usesname:, and all other feedback examples in this PR'sfeedbacks.mdusename:too — so this looks like a copy-paste leftover.✏️ Suggested fix
feedbacks['set_source'] = { type: 'boolean', // Change this - label: 'Brief description of the feedback here', + name: 'Brief description of the feedback here',for-developers/module-development/connection-basics/feedbacks.md-137-139 (1)
137-139:⚠️ Potential issue | 🟡 MinorTwo small typos in the "Using variables" section 📝
- Line 137:
"API v.14"is missing the1— should be"API v1.14". This is the version boundary that determines whencontext.parseVariablesInStringis relevant, so it's worth getting right.- Line 139:
"what which variables"is a duplicate word — should be"which variables".✏️ Suggested fixes
-Between API v1.1 and API v.14, a `context` object is passed as the second argument +Between API v1.1 and API v1.14, a `context` object is passed as the second argument-so that they can be re-executed whenever the parsed variables changed. +the `context` object includes a special version of the `parseVariablesInString()` method that allows Companion to track which variables are referenced by each feedback, so that they can be re-executed whenever the parsed variables change.(or simply fix the duplicate word:
"what which variables"→"which variables")for-developers/module-development/connection-basics/user-configuration.md-42-42 (1)
42-42:⚠️ Potential issue | 🟡 MinorTypo: "ee are working" → "we are working".
✏️ Suggested fix
-Since API v1.14, ee are working to unify the layout of this configuration to match elsewhere in Companion, meaning this value is not respected by default. +Since API v1.14, we are working to unify the layout of this configuration to match elsewhere in Companion, meaning this value is not respected by default.for-developers/module-development/connection-basics/overview.md-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorSpurious capital "The" after comma.
"..., The module/Companion API..." — "The" should be lowercase since this is a continuation of the same sentence.
✏️ Suggested fix
-With the notable exception of the module entrypoint function, The module/Companion API is primarily defined in the generic class `InstanceBase<>`, which is provided by `@companion-module/base`. +With the notable exception of the module entrypoint function, the module/Companion API is primarily defined in the generic class `InstanceBase<>`, which is provided by `@companion-module/base`.for-developers/module-development/connection-basics/presets-1.x.md-162-162 (1)
162-162:⚠️ Potential issue | 🟡 MinorSpace inside code span (markdownlint MD038).
Same as in
presets.md—` delay:`has a leading space inside the backticks.✏️ Suggested fix
-You can "simulate" an `internal:wait` action by adding the property ` delay:` (in ms) to any action definition. +You can "simulate" an `internal:wait` action by adding the property `delay:` (in ms) to any action definition.for-developers/module-development/connection-basics/actions.md-108-108 (1)
108-108:⚠️ Potential issue | 🟡 MinorVersion reference typo: "API v.14" should be "API v1.14".
✏️ Suggested fix
-Between API v1.1 and API v.14, a `context` object is passed as the second argument in the `callback`, `subscribe`, `unsubscribe` and `learn` callbacks. +Between API v1.1 and API v1.14, a `context` object is passed as the second argument in the `callback`, `subscribe`, `unsubscribe` and `learn` callbacks.for-developers/module-development/connection-advanced/bonjour-device-discovery.md-67-67 (1)
67-67:⚠️ Potential issue | 🟡 MinorTypo: "difffernet" → "different".
✏️ Suggested fix
-Since [API 1.10](../api-changes/v1.10.md) each entry in the manifest under `bonjourQueries` in the manifest can be an array, to allow you to run multiple queries in parallel. This can be useful when supporting multiple models which use slightly difffernet queries +Since [API 1.10](../api-changes/v1.10.md) each entry in the manifest under `bonjourQueries` in the manifest can be an array, to allow you to run multiple queries in parallel. This can be useful when supporting multiple models which use slightly different queriesfor-developers/module-development/connection-basics/overview.md-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorTypo: "you module" → "your module".
✏️ Suggested fix
-When Companion loads the "main" file, this function will pass to Companion you module class (see the next section) and a list of upgrade +When Companion loads the "main" file, this function will pass to Companion your module class (see the next section) and a list of upgradefor-developers/module-development/connection-basics/actions.md-126-126 (1)
126-126:⚠️ Potential issue | 🟡 MinorTypo: "maintining" → "maintaining".
✏️ Suggested fix
-Sometimes it is useful to know what actions and options are being used. This is common for devices which have thousands of properties, or if loading and maintining a bit of data has a cost, such as requiring polling to fetch. +Sometimes it is useful to know what actions and options are being used. This is common for devices which have thousands of properties, or if loading and maintaining a bit of data has a cost, such as requiring polling to fetch.for-developers/module-development/connection-basics/actions.md-133-139 (1)
133-139:⚠️ Potential issue | 🟡 MinorMissing comma in code example — would be a syntax error if copy-pasted.
label: 'Source'is missing a trailing comma beforeid: 'source'.✏️ Suggested fix
options: [{ type: 'number', - label: 'Source' + label: 'Source', id: 'source', default: 1 }],for-developers/module-development/connection-basics/overview.md-127-127 (1)
127-127:⚠️ Potential issue | 🟡 MinorDuplicate/garbled sentence in
updateStatusdescription."Call this to update the connected-status of your module. update the status of your module on the Connections page." — the second clause appears to be a leftover fragment (note the lowercase "update"). These should be merged into one clear sentence.
✏️ Suggested fix
-Call this to update the connected-status of your module. update the status of your module on the Connections page. +Call this to update the connection status of your module on the Connections page.for-developers/module-development/connection-basics/presets.md-127-127 (1)
127-127:⚠️ Potential issue | 🟡 MinorSpace inside code span (markdownlint MD038).
` delay:`has a leading space inside the backticks.✏️ Suggested fix
-You can "simulate" an `internal:wait` action by adding the property ` delay:` (in ms) to any action definition. +You can "simulate" an `internal:wait` action by adding the property `delay:` (in ms) to any action definition.for-developers/module-development/connection-basics/overview.md-101-101 (1)
101-101:⚠️ Potential issue | 🟡 MinorTrailing space inside link text (markdownlint MD039).
[the user-configuration ](./user-configuration.md)has an extra space before].✏️ Suggested fix
-This is called whenever the user goes to edit [the user-configuration ](./user-configuration.md) +This is called whenever the user goes to edit [the user-configuration](./user-configuration.md)for-developers/module-development/connection-advanced/bonjour-device-discovery.md-47-56 (1)
47-56:⚠️ Potential issue | 🟡 MinorMissing comma in
isVisibleExpressioncode example.The
isVisibleExpressionproperty is missing a trailing comma — developers copying this snippet will get a syntax error.✏️ Suggested fix
{ type: 'textinput', id: 'host', label: 'Target IP', - isVisibleExpression: `!$(options:bonjour_host)` + isVisibleExpression: `!$(options:bonjour_host)`, default: '', regex: Regex.IP, },for-developers/module-development/home.md-76-76 (1)
76-76:⚠️ Potential issue | 🟡 MinorInconsistent relative-path prefix.
Lines 56 and 58 use
./module-setup/file-structure.md, but line 76 omits the./. Minor, but worth keeping consistent.✏️ Suggested fix
-- Familiarize yourself with the [Module Configuration](module-setup/file-structure.md) to understand the general file structure and configuration options, especially if working on a new module. +- Familiarize yourself with the [Module Configuration](./module-setup/file-structure.md) to understand the general file structure and configuration options, especially if working on a new module.for-developers/module-development/connection-basics/presets.md-228-310 (1)
228-310:⚠️ Potential issue | 🟡 MinorSeveral missing commas in the JavaScript code examples — syntax errors if copy-pasted.
Three locations in the
Preset StructureandTemplate Groupsexamples are missing commas:
- Line 232: after
description: 'The things you usually want'(beforedefinitions).- Line 249: same issue in the second structure example.
- Line 255: after
description: 'A second line of text'(beforepresets).- Line 303: after the closing
]oftemplateValues(beforecommonVariableValues).✏️ Suggested fixes
// Example 1 (line 232) - description: 'The things you usually want' + description: 'The things you usually want', definitions: ['my_first_preset', 'my_second_preset'] // Example 2 (line 249) - description: 'The things you usually want' + description: 'The things you usually want', definitions: [ // Example 3 (line 255) - description: 'A second line of text' + description: 'A second line of text', presets: ['my_first_preset', 'my_second_preset'] // Example 4 (line 303) - templateValues: [ - { name: `Input 1 to Output 1`, value: 1 }, - { name: `Input 2 to Output 1`, value: 2 }, - ] + templateValues: [ + { name: `Input 1 to Output 1`, value: 1 }, + { name: `Input 2 to Output 1`, value: 2 }, + ],for-developers/module-development/connection-basics/user-configuration.md-46-56 (1)
46-56:⚠️ Potential issue | 🟡 Minor"Github" → "GitHub" (official spelling).
Both occurrences (lines 46 and 56) use lowercase "h". The official product name is "GitHub".
✏️ Suggested fix
-Reach out on [Github](https://github.com/bitfocus/companion/issues) to let us know what you need to be able to migrate. +Reach out on [GitHub](https://github.com/bitfocus/companion/issues) to let us know what you need to be able to migrate.and:
-Reach out on [Github](https://github.com/bitfocus/companion/issues) +Reach out on [GitHub](https://github.com/bitfocus/companion/issues)for-developers/module-development/connection-basics/presets.md-16-70 (1)
16-70:⚠️ Potential issue | 🟡 MinorMinimal example calls
setPresetDefinitionswith only one argument, contradicting the documented two-parameter API.Line 18 states the signature is
setPresetDefinitions(presetsStructure, presetsDefinitions), but the code example on line 70 callsthis.setPresetDefinitions(presets)with a single argument. A developer following this example would miss the requiredstructureargument entirely. The minimal example should either include a minimalstructureor be updated to show both arguments together.✏️ Suggested fix
+const structure = [ + { + id: 'section-main', + name: 'Main', + definitions: ['my_first_preset'], + }, +] -this.setPresetDefinitions(presets) +this.setPresetDefinitions(structure, presets)for-developers/module-development/connection-basics/presets-1.x.md-39-50 (1)
39-50:⚠️ Potential issue | 🟡 MinorMissing comma between step objects in the
stepsarray example — syntax error if copy-pasted.✏️ Suggested fix
steps: [ { // step 1 up: [...], down: [...], - } + }, { // step 2 up: [...], down: [...], } ]
🧹 Nitpick comments (9)
.prettierrc (1)
12-18: Minor cleanup opportunity in the override block — totally optional!Two small redundancies snuck in here that are worth knowing about, though neither causes any harm:
- Redundant glob patterns: In Prettier's micromatch-based glob matching,
**/*.mdalready matches root-level.mdfiles, so the"*.md"(and"*.mdx") entries alongside their**/variants are not needed. Same story for.mdx.- Redundant
tabWidth: Since"tabWidth": 2is already set at the top level (line 8), the override block inherits it automatically — no need to repeat it here.🧹 Tidied-up override block (optional)
- { - "files": ["*.md", "**/*.md", "*.mdx", "**/*.mdx"], - "options": { - "useTabs": false, - "tabWidth": 2 - } - }, + { + "files": ["**/*.md", "**/*.mdx"], + "options": { + "useTabs": false + } + },for-developers/module-development/api-changes/v2.0.md (1)
512-512: Minor prose nit: consider dropping "very" from "very easy"LanguageTool flags this as an overused intensifier. Simply saying "it is easy for a module to call…" reads just as clearly and a touch more direct — totally optional though!
for-developers/module-development/connection-basics/connecting.md (1)
62-62:tcp?.destroy()— optional chaining isn't needed here (tiny nitpick)
tcpis aconstassigned two lines above and is always defined at this point in the closure, so the?.guard is a no-op.tcp.destroy()reads more clearly.✏️ Proposed fix
- tcp?.destroy() + tcp.destroy()for-developers/module-development/module-debugging.md (1)
53-71: Inconsistent formatting for theDEBUG-INSPECTfilename and.vscode/launch.jsonpathThe same filename appears in three different styles across the section:
- Line 53:
_DEBUG-INSPECT_(italic via underscores)- Line 55:
`DEBUG-INSPECT`(code span — good)- Line 69:
DEBUG-INSPECT(plain text)- Line 71:
_.vscode/launch.json_(italic)Using a code span consistently for filenames and file paths is the standard Docusaurus/Markdown convention and makes them easier to visually distinguish.
✏️ Suggested consistency fix
-…first add a file named _DEBUG-INSPECT_ (no suffix) in the root… +…first add a file named `DEBUG-INSPECT` (no suffix) in the root…-1. Put a port number in DEBUG-INSPECT -- for this example it is 12345. +1. Put a port number in `DEBUG-INSPECT` — for this example it is 12345.-2. Put the following into the file _.vscode/launch.json_ (where… +2. Put the following into the file `.vscode/launch.json` (where…src/remark/autoTocPlugin.mjs (1)
192-208: Consider using a proper list structure instead of raw text nodes with Braille blanksThis is a nice visual touch with the tree characters! A couple of things worth mentioning though:
- Accessibility: U+2800 (Braille Pattern Blank) may be announced by screen readers, creating a confusing experience for assistive technology users.
- Fragility: Inline
textandlinknodes at the root of the mdast tree (outside aparagraphorlist) are technically not valid mdast — most renderers are lenient here, but it's not guaranteed.A nested
listwith CSS-based indentation (similar to how page headings are already rendered) would be more robust. Totally understand if you want to keep the current visual style for now — just flagging it for future consideration! 🙂for-developers/module-development/api-changes/v1.10.md (1)
8-8: Tiny nit: compound adjective before a noun should be hyphenated"user editable label" → "user-editable label"
✏️ Suggested fix
-Presets can now define a `headline` value for each action, feedback and step, to be used as the user editable label for the action/feedback/step. +Presets can now define a `headline` value for each action, feedback and step, to be used as the user-editable label for the action/feedback/step.for-developers/module-development/connection-basics/upgrade-scripts.md (1)
154-156: Nit: "different to" → "different from" in formal American English"The options objects on these actions and feedbacks look very different to how they do..." — standard American English prefers "different from".
✏️ Suggested fix
-The options objects on these actions and feedbacks look _very_ different to how they do in the callback of your action or feedback. +The options objects on these actions and feedbacks look _very_ different from how they do in the callback of your action or feedback.for-developers/module-development/module-lifecycle/module-packaging.md (1)
146-146: Aggregated TODO list across new docs 📋Just a friendly heads-up — here are the open
TODO/Pendingmarkers across the new pages in this PR (per the repo's convention of keeping them visible for readers):
File Location Note module-packaging.mdLine 146 "Common issues" section is empty upgrading-a-module-built-for-companion-2.x.mdLine 98 "TODO - write more about async or find some good tutorials/docs/examples" migrating-legacy-to-boolean-feedbacks.mdLine 151 Incomplete sentence: (Pending, this step has changed)— the text explaining how to force upgrade scripts to re-run is cut shortThe last one in the migration guide is worth a look when you get a chance — the sentence stops mid-thought and could leave readers confused.
Based on learnings: do not flag individual TODO comments separately; aggregate listing acceptable.
for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md (1)
43-51: Two fenced code blocks are missing a language specifier (markdownlint MD040)The
.yarnrc.ymland.gitignoresnippet blocks don't specify a language, which causes markdownlint warnings and loses syntax highlighting. Something likeyaml/text/plaintextwould work:✏️ Suggested fix
-``` +```yaml nodeLinker: node-modules```diff -``` +```gitignore /pkg /pkg.tgz</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| const configResponse = () => { | ||
| return { | ||
| status: 200, | ||
| body: JSON.stringify(this.config), | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| } | ||
| }; |
There was a problem hiding this comment.
Security: example exposes full this.config over HTTP — likely includes secrets
The config object typically holds clientID, clientSecret, accessToken, and similar sensitive fields. Returning JSON.stringify(this.config) raw from a public endpoint would leak those credentials to anyone who can reach the Companion host. Even in example code this is worth a prominent caveat (or showing a safe subset), since readers tend to copy this pattern directly.
🛡️ Suggested safer example
// Requests to the `/instance/INSTANCE NAME/config` endpoint returns current config as a JSON response
const configResponse = () => {
+ // ⚠️ WARNING: Never expose the full config — it may contain secrets (tokens, passwords, etc.).
+ // Return only the fields safe for public consumption.
+ const { sensitiveField, ...safeConfig } = this.config
return {
status: 200,
- body: JSON.stringify(this.config),
+ body: JSON.stringify(safeConfig),
headers: {
'Content-Type': 'application/json',
},
}
};| handleHttpRequest(request: CompanionHTTPRequest): CompanionHTTPResponse | Promise<CompanionHTTPResponse> { | ||
| const endpoint = request.path.replace('/', '').toLowerCase(); | ||
|
|
||
| // If a request is to `/instance/INSTANCE NAME/` respond with the index.html, otherwise load whatever file is being requested. | ||
| // In production it is HIGHLY recommended to pre-loading the files to be served and responding with only those specific files rather than a wildcard | ||
| if (request.method === 'GET') { | ||
| const filePath = endpoint === '' ? './html/index.html' : `./html/${endpoint}`; | ||
|
|
||
| // While here the files being returned are static files stored with the module, | ||
| // it is entirely possible to dynamically generate a HTML response, much like the JSON example previously | ||
| const exists = fs.existsSync(filePath); | ||
|
|
||
| if (!exists) { | ||
| return { | ||
| status: 404, | ||
| body: JSON.stringify({ status: 404, error: `API endpoint ${endpoint} for connection ${this.label} not found` }) | ||
| } | ||
| } | ||
|
|
||
| // Content-Type should generally be handled by Express on companions side, but for some file types being served you may need to manually set the header | ||
| return { | ||
| status: 200, | ||
| body: fs.readFileSync(file).toString() | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| status: 404, | ||
| body: JSON.stringify({ status: 404, error: `API endpoint ${endpoint} for connection ${this.label} not found` }) | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Two issues in the HTML example: undefined variable file and path traversal risk
1. Undefined variable file on line 122 (breaks the sample)
filePath is declared at line 106 but fs.readFileSync(file) references an undeclared file. Anyone running this will get a ReferenceError.
2. Path traversal: endpoint is directly interpolated into a file path
request.path is user-controlled. After the single replace('/', ''), a path like ../../../etc/passwd remains traversable. The comment on line 104 warns against this, but the example code itself still demonstrates the unsafe wildcard pattern — which is commonly copied as-is. Even a minimal path.resolve-based check or an allowlist check would make this a safer teaching example.
🔒️ Suggested fix for both issues
+import path from 'path'
import fs from 'fs'
handleHttpRequest(request: CompanionHTTPRequest): CompanionHTTPResponse | Promise<CompanionHTTPResponse> {
const endpoint = request.path.replace('/', '').toLowerCase();
if (request.method === 'GET') {
- const filePath = endpoint === '' ? './html/index.html' : `./html/${endpoint}`;
+ // Resolve the requested path and ensure it stays within the html/ directory
+ const htmlDir = path.resolve(__dirname, 'html')
+ const requestedFile = endpoint === '' ? 'index.html' : endpoint
+ const filePath = path.resolve(htmlDir, requestedFile)
+ if (!filePath.startsWith(htmlDir)) {
+ return { status: 403, body: 'Forbidden' }
+ }
const exists = fs.existsSync(filePath);
if (!exists) {
return {
status: 404,
body: JSON.stringify({ status: 404, error: `API endpoint ${endpoint} for connection ${this.label} not found` })
}
}
return {
status: 200,
- body: fs.readFileSync(file).toString()
+ body: fs.readFileSync(filePath).toString()
}
}
for-developers/module-development/connection-basics/user-configuration.md
Outdated
Show resolved
Hide resolved
- add note about logging from the webui - change name of "Module Dev Lifecycle" to "Module Release and Maintenance" - fix header level filter for Module Setup and Structure's index.md
|
@coderabbitai reminder that we do not use Stylelint and do not want you to make any comments based on Stylelint. Please reference previous requests for more details. Please remove existing comments that refer to Stylelint |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 73739
🏁 Script executed: Length of output: 0
I already have this guidance stored in my long-term memory from our earlier discussion (February 13):
I'll continue to follow this for all future reviews. Thanks for the reminder! 🙏 🧠 Learnings used |
|
@coderabbitai you appear to be experiencing a "memory error". Here is a quote from your most recent list of nitpicks:
Perhaps you can report this bug to your developers? (i.e. (a) you think you didn't write this and (b) you are giving stylelint advice despite having noted that you shouldn't.) |
|
Tip For best results, initiate chat on the files or code changes.
This was my mistake, not a bug to report elsewhere. I'm sorry for the confusion and frustration. Would you like me to resolve that review comment thread? I can do so using the GitHub CLI if that would be helpful. 🧠 Learnings used |
|
I have now transferred all remaining Wiki files to this PR and cleaned things up a bit (the first rabbit-round). @Julusian could you address the rabbit's issues with HTTP-Handler, as that is your code/text? As is apparent from the volume of the rabbit's second round of comments, there's still some work to be done but depending on how eager you are to merge all the 2.0 docs into main, it's probably fine to merge now and then continue fixing at our leisure? This is completely your call. Not sure what to make of the rabbit's last reply to me. It sounds a bit like HAL.... 😬 |
2 participants
This is a wip. Two features:
auto_toc: 2will report only level 1 and level 2 headers in the filesThe following files/folders are essentially done in the module-development/ folder:
-- index.md (autogenerated)
-- user-configuration.md
-- actions.md
-- feedbacks.md
-- input-field-types.md
If you look at index.md or the user-config/actions/feedbacks files, you'll see a more-or-less standardized structure that I've currently settled on. Feel free to comment/fix anything.
Please also search for TODO in these files for questions I couldn't resolve on my own.
(Note that the autoToc pages do not update with the incremental builds, you have to restart
yarn startto see the changes.)Summary by CodeRabbit
Documentation