Chore transfer main module docs#35
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).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR expands and reorganizes the Module Development docs: adds Module Setup, Connection Basics/Advanced, and Module Lifecycle sections; many new how‑to pages (actions, feedbacks, presets, discovery, HTTP/OAuth, permissions, packaging, upgrades); updates category metadata, TOC generation, and markdown/CSS styling. 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).
|
OK, I think I am pretty much done on my pass of all of these docs.
I think something manual is best. Doesnt need to be much, but to give some direction.
Although I suppose I don't mind the cards, as it gives a list of links with a bit more of a description about each. Unless we have something to actually say on the page, I dont mind this. I think the auto-toc makes a lot of sense for the api changelog as the pages themselves are a pretty abstract time based organisation system. But for http://localhost:4005/for-developers/module-development/connection-basics/, I expect that anyone going here will have an idea of what area of functionality they want to read about, but as each of the headings on the pages are interlinked in a way (because they are all about the same larger feature/topic), they probably wont be after a specific one. And that list is quite a wall of links, which feels like too much information or too many options for anyone who doesnt fully know what they are after. I could also be picky and comment about the headings which don't have anything underneath them looks a little odd.
Yeah I discover that after putting some thoguht into it, but until I thought about it, it looked odd and like we had changed style/approach halfway down the page |
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (40)
for-developers/module-development/api-changes/v1.14.md-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorMinor wording nit flagged by LanguageTool.
"is not consistent with elsewhere within the panel" is a little wordy. "is inconsistent with elsewhere in the panel" reads more naturally.
✏️ Suggested fix
-The connection config panel allowed modules to customise the layout of input fields. This was not possible elsewhere in Companion, and is not consistent with elsewhere within the panel where fields are in the simplified layout. +The connection config panel allowed modules to customise the layout of input fields. This was not possible elsewhere in Companion, and is inconsistent with elsewhere in the panel where fields are in the simplified layout.for-developers/module-development/api-changes/v1.14.md-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorSmall grammar fix needed in the opt-out sentence.
"by in your module constructor by doing" has two competing prepositions that make the sentence a bit of a stumble. A quick tidy-up would make it much smoother for readers:
✏️ Suggested fix
-It is our hope that this new layout will not cause significant issues, if you have any problems or really dislike it, you can temporarily opt out of the new layout by in your module constructor by doing: +It is our hope that this new layout will not cause significant issues. If you have any problems or really dislike it, you can temporarily opt out of the new layout in your module constructor by setting:for-developers/Satellite-API.md-156-156 (1)
156-156:⚠️ Potential issue | 🟡 MinorTwo small wording tweaks for line 156 — feel free to take or leave these!
8bit→8-bit— Compound modifiers used before a noun are typically hyphenated in standard technical writing. Static analysis also flags this one."also on the version of the API" reads as a dangling phrase — a connecting verb would make it clearer. Something like:
"Resolution follows the size defined by the
BITMAPSparameter, and also depends on the version of the API."✏️ Suggested wording
-- `BITMAP` base64 encoded pixel data. This is only sent for devices which were added where `BITMAPS` is enabled. Resolution follows the size defined by the `BITMAPS`, also on the version of the API. Currently encoded as 8bit RGB (this may be configurable in the future). +- `BITMAP` base64 encoded pixel data. This is only sent for devices which were added where `BITMAPS` is enabled. Resolution follows the size defined by the `BITMAPS` parameter, and also depends on the version of the API. Currently encoded as 8-bit RGB (this may be configurable in the future).for-developers/module-development/module-debugging.md-57-57 (1)
57-57:⚠️ Potential issue | 🟡 MinorSame missing-space typo as in
logging.md—"thebuilt-in"→"the built-in"✏️ Proposed fix
-You can use any compatible debugger such as thebuilt-in VS Code debugger, or Chrome inspector to connect to your process. +You can use any compatible debugger such as the built-in VS Code debugger, or Chrome inspector to connect to your process.for-developers/module-development/connection-basics/logging.md-25-27 (1)
25-27:⚠️ Potential issue | 🟡 MinorA couple of small prose fixes — easy wins!
Two issues on these lines that static analysis also caught:
- Line 25:
"your mode code"→"your module code", and"thebuilt-in"→"the built-in"(missing space).- Line 27:
"inside of the"→"inside the"(redundant phrasing).✏️ Proposed fix
-If you want to produce some debug logging from your mode code, you can use thebuilt-in `console` methods such as `console.log` and `console.error`. +If you want to produce some debug logging from your module code, you can use the built-in `console` methods such as `console.log` and `console.error`. -These are treated as debug logs, and will only be shown inside of the module debug log view; accessible from the popout menu in the connections list. +These are treated as debug logs, and will only be shown inside the module debug log view; accessible from the popout menu in the connections list.for-developers/module-development/connection-basics/logging.md-16-21 (1)
16-21:⚠️ Potential issue | 🟡 MinorTypos inside the code example strings will confuse copy-pasters 🙈
Two of the four example strings have clear typos that'll make developers scratch their heads:
- Line 18:
warni nghas extra spaces in the middle of "warning".- Line 20:
dbeugis a transposition of "debug".These are in a code block, so readers are likely to copy them verbatim.
✏️ Proposed fix
```ts this.log('error', 'Some error message') -this.log('warn', 'Some warni ng message') +this.log('warn', 'Some warning message') this.log('info', 'Some info message') -this.log('debug', 'Some dbeug message') +this.log('debug', 'Some debug message')</details> </blockquote></details> <details> <summary>for-developers/module-development/home.md-64-66 (1)</summary><blockquote> `64-66`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language specifier to the fenced code block** — the linter flagged this one! 😊 The code fence at line 64 is missing a language tag, which suppresses syntax highlighting and trips the MD040 rule. <details> <summary>✏️ Suggested fix</summary> ```diff -``` +```bash yarn install</details> </blockquote></details> <details> <summary>for-developers/module-development/connection-basics/overview.md-12-12 (1)</summary><blockquote> `12-12`: _⚠️ Potential issue_ | _🟡 Minor_ **A few minor punctuation / style nits worth a quick pass** These are all small, but since this is reference docs that developers will rely on: - **Line 12**: `"methods that get called by Companion"` → `"methods that are called by Companion"` (slightly more formal for technical docs) - **Line 39**: Missing trailing period inside the `:::tip` admonition. - **Line 68**: `"Each page explains the structure and usage of each portion in more detail"` — missing period. - **Line 92**: `"Many modules simply call … during configUpdated"` — missing period inside the tip. Also applies to: 39-39, 68-68, 92-92 </blockquote></details> <details> <summary>for-developers/module-development/connection-basics/overview.md-18-18 (1)</summary><blockquote> `18-18`: _⚠️ Potential issue_ | _🟡 Minor_ **Small typo: "this typically this like" 👀** Line 18 has a duplicated word — `"In ESM this typically this like:"` should probably be `"In ESM, this typically looks like:"`. <details> <summary>✏️ Suggested fix</summary> ```diff -In ESM this typically this like: +In ESM, this typically looks like:for-developers/module-development/connection-basics/overview.md-44-52 (1)
44-52:⚠️ Potential issue | 🟡 Minor"TypeScript" capitalization inconsistency
Lines 44 and 46 use
Typescriptwhile line 52 correctly usesTypeScript(the official brand name). Worth making consistent throughout.✏️ Suggested fix
-### Typescript Generics +### TypeScript Generics -If you are not using Typescript, you can skip this section +If you are not using TypeScript, you can skip this sectionversioned_docs/version-v4.1/5_remote-control/satellite.md-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorTiny nit: missing period at the end of the sentence.
✏️ Suggested fix
-The satellite protocol follows SemVer and is documented on [the website](https://companion.free/for-developers/Satellite-API) +The satellite protocol follows SemVer and is documented on [the website](https://companion.free/for-developers/Satellite-API).versioned_docs/version-v4.1/6_modules.md-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorConsider making the link text more descriptive.
Great contribution! The link text
"here"is quite generic — screen readers and search engines will appreciate something more meaningful. The static analysis tool (markdownlintMD059) flags this as well.✍️ Suggested wording
-1. If you have any experience writing code, you could make the module yourself, we are happy to accept all contributions. Read more [here](https://companion.free/for-developers/module-development/home) +1. If you have any experience writing code, you could make the module yourself, we are happy to accept all contributions. Read more in the [module development guide](https://companion.free/for-developers/module-development/home)src/css/custom.css-282-295 (1)
282-295:⚠️ Potential issue | 🟡 MinorStylelint: missing blank lines before
font-sizedeclarations (lines 284 and 290)Two small formatting violations are caught by Stylelint's
declaration-empty-line-beforerule — both involve a standard property (font-size) immediately following a custom property declaration with no blank line in between. Easy fix! 🙂🎨 Proposed formatting fix
.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; }for-developers/module-development/connection-advanced/http-handler.md-18-26 (1)
18-26:⚠️ Potential issue | 🟡 MinorMinor: Missing period at end of sentence on Line 20.
The sentence introducing
handleHttpRequestis missing a full stop at the end — a small copy-edit nit!✏️ Suggested fix
-The `this.handleHttpRequest` method on the Instance class is what handles HTTP requests being passed from Companion to the module instance +The `this.handleHttpRequest` method on the Instance class is what handles HTTP requests being passed from Companion to the module instance.for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md-159-159 (1)
159-159:⚠️ Potential issue | 🟡 MinorCopy-paste slip: "action" used twice inside the Feedbacks section (§6).
Both of these sentences were lifted verbatim from the Actions section (§5) and still say "action" where they should say "feedback". This could confuse a developer who's reading carefully:
- Line 159: "This is the only way an action can be executed" — should be feedback
- Line 164: "Perhaps you could set a
descriptionfor the action too?" — should be feedback✏️ Suggested fixes
- - `callback` is now required. This is the only way an action can be executed (more help is below) + - `callback` is now required. This is the only way a feedback can be checked (more help is below)-Tip: While you are making these changes, does the value of `name` still make sense? Perhaps you could set a `description` for the action too? +Tip: While you are making these changes, does the value of `name` still make sense? Perhaps you could set a `description` for the feedback too?Also applies to: 164-164
for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md-130-130 (1)
130-130:⚠️ Potential issue | 🟡 MinorTiny copy-paste typo: "then" → "them" in two places.
Both occur in the phrase "if you need then" — one in the Actions section and one in the Feedbacks section.
✏️ Suggested fixes
-If you aren't already aware, there are some other properties you can implement if you need then. +If you aren't already aware, there are some other properties you can implement if you need them.(Same fix applies to the identical sentence on line 168.)
Also applies to: 168-168
for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md-497-497 (1)
497-497:⚠️ Potential issue | 🟡 MinorTypo: "any" → "and".
✏️ Suggested fix
-Loading code spread across hundreds of files is surprisingly slow on some OSes, any by combining it all into a few files, we can often reduce the size from multiple mb, to a few hundred kb. +Loading code spread across hundreds of files is surprisingly slow on some OSes, and by combining it all into a few files, we can often reduce the size from multiple mb, to a few hundred kb.for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md-10-11 (1)
10-11:⚠️ Potential issue | 🟡 MinorConsider anchoring the age of this guide to a specific version or year instead of a relative time.
"over 3 years ago"will keep drifting and become less informative over time. Something like"originally written for Companion 3.0 (circa 2022)"gives readers a stable reference and helps them assess compatibility without the number going stale.✏️ Suggested wording
-This guide was written over 3 years ago for an earlier version of the module api. +This guide was originally written for Companion 3.0 (circa 2022) and targets an earlier version of the module API.for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorSmall grammar fix needed — "issues was" → "issue was" (or "issues were").
✏️ Suggested fix
-The main issues was modules were running in the main companion thread... +The main issue was that modules were running in the main companion thread...for-developers/module-development/connection-advanced/migrating-legacy-to-boolean-feedbacks.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorTiny typo in the
descriptionfield — "set up migrate" → "migrate"The description currently reads "How to set up migrate legacy feedbacks…", which has an extra phrase. A quick trim would polish it up nicely:
✏️ Suggested fix
-description: How to set up migrate legacy feedbacks to boolean feedbacks. +description: How to migrate legacy feedbacks to boolean feedbacks.for-developers/module-development/connection-advanced/permissions.md-19-21 (1)
19-21:⚠️ Potential issue | 🟡 MinorTip wording is a bit ambiguous — and missing a period
"Make sure to test the module with this disabled before publishing" — "this" could refer to the debugger or to the permissions themselves. A small clarification would remove any ambiguity. The sentence is also missing a trailing period.
✏️ Suggested fix
-Make sure to test the module with this disabled before publishing +Make sure to test the module with the debugger disabled (and thus permissions enforced) before publishing.for-developers/module-development/connection-advanced/permissions.md-2-3 (1)
2-3:⚠️ Potential issue | 🟡 MinorQuick nit: sync the
titleandsidebar_label
titleis"Runtime Permission"(singular) whilesidebar_labelis"Runtime Permissions"(plural). Keeping them consistent avoids any potential confusion between the page heading and the sidebar entry.✏️ Suggested fix
-title: Runtime Permission +title: Runtime Permissions sidebar_label: Runtime Permissionsfor-developers/module-development/connection-advanced/permissions.md-80-80 (1)
80-80:⚠️ Potential issue | 🟡 MinorConsider using version-neutral documentation link
Good catch on the Node.js flag! The
--openssl-legacy-providerflag is indeed supported in Node.js 22 (and will remain available in later versions), so no worries there.The link does point to a valid resource today, but since it's pinned to
latest-v22.x, it could become outdated as projects update to newer Node.js versions. Linking tohttps://nodejs.org/api/cli.html#--openssl-legacy-providerinstead would be more maintainable long-term and let readers naturally access the docs for whatever version they're running. Either approach works, but the version-neutral link is nice for future-proofing! 👍for-developers/module-development/module-lifecycle/module-packaging.md-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorTiny prose fix in the opening paragraph 🙂
Two small things on line 10:
- "multiple mb" → "multiple MB" (standard abbreviation)
- "…a few hundred kb, lead to much shorter load times" is missing a conjunction — should read "…a few hundred KB, and lead to much shorter load times"
✏️ Suggested fix
-Starting with Companion 3.0, modules must be packaged with some special tooling. This is done to reduce the number and total size of files in your module. Combining your module into just a few files can often reduce the size from multiple mb, to a few hundred kb, lead to much shorter load times. +Starting with Companion 3.0, modules must be packaged with some special tooling. This is done to reduce the number and total size of files in your module. Combining your module into just a few files can often reduce the size from multiple MB to a few hundred KB, and lead to much shorter load times.for-developers/module-development/module-lifecycle/module-packaging.md-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorSmall grammar gap and a confusing version example
Two things on line 26:
- Missing "and" — "…at your module root folder and a
<module-name>-<version>.tgzfile…"- The example says the version is
0.70but the resulting filename uses0.7.0— it's unclear whether that's intentional (e.g. showing trailing-zero trimming) or just a typo. Aligning both to0.7.0would avoid confusion.✏️ Suggested fix
-If successful, there will now be a `pkg/` folder at your module root folder a `<module-name>-<version>.tgz` file in the root folder. The module name and version are taken from your _package.json_ file, so for example, if the module is named 'generic-animation' and the version number in _package.json_ is 0.70, then the file will be named _generic-animation-0.7.0.tgz_. +If successful, there will now be a `pkg/` folder at your module root folder and a `<module-name>-<version>.tgz` file in the root folder. The module name and version are taken from your _package.json_ file, so for example, if the module is named 'generic-animation' and the version number in _package.json_ is 0.7.0, then the file will be named _generic-animation-0.7.0.tgz_.for-developers/module-development/module-lifecycle/module-packaging.md-158-165 (1)
158-165:⚠️ Potential issue | 🟡 MinorFix broken link in "Further reading" section — The reference to
../connection-basics/(line 165) doesn't exist in the repo. Could you either create that file/directory or point to the correct path? The other five links check out fine. Thanks! 🙌for-developers/module-development/module-lifecycle/module-packaging.md-63-118 (1)
63-118:⚠️ Potential issue | 🟡 MinorA few small prose nits across the native-dependencies section
Nothing blocking at all — just a few things that are easy to clean up:
- Line 63:
javascript→JavaScript- Line 86: "We only support ones who publish…" → "ones that publish…" (referring to libraries, not people)
- Line 91: "try and get this working" → "try to get this working"
- Lines 91, 93, 101 (LanguageTool): Three consecutive paragraphs all open with "If the library is using…" — a short rewording of one or two of them would break the repetition nicely.
✏️ Suggested fixes
-Once your module is packaged, it wont have access to any files, from the repository unless they are javascript which gets included in the bundle or the files are explicitly copied. +Once your module is packaged, it wont have access to any files from the repository unless they are JavaScript which gets included in the bundle or the files are explicitly copied.-It is not yet possible to use all native dependencies. We only support ones who publish prebuilt binaries as part of their npm package. +It is not yet possible to use all native dependencies. We only support ones that publish prebuilt binaries as part of their npm package.-If you need one of these libraries, let us know and we can try and get this working. +If you need one of these libraries, let us know and we can try to get this working.for-developers/module-development/connection-basics/presets-1.x.md-18-18 (1)
18-18:⚠️ Potential issue | 🟡 MinorMissing space between the closing backtick and "much"
Line 18 reads
`this.setPresetDefinitions(presetsDefinitions)`much like…` — the backtick and the word run together. Just a quick space needed there.✏️ Suggested fix
-In order to add presets to a module, you call `this.setPresetDefinitions(presetsDefinitions)`much like how you define actions and feedbacks. +In order to add presets to a module, you call `this.setPresetDefinitions(presetsDefinitions)` much like how you define actions and feedbacks.for-developers/module-development/connection-advanced/bonjour-device-discovery.md-67-67 (1)
67-67:⚠️ Potential issue | 🟡 Minor"in the manifest" is duplicated in the same sentence
Line 67 currently reads: "…each entry in the manifest under
bonjourQueriesin the manifest can be an array…" — "in the manifest" appears twice.✏️ 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. +Since [API 1.10](../api-changes/v1.10.md) each entry under `bonjourQueries` in the manifest can be an array, to allow you to run multiple queries in parallel.for-developers/module-development/connection-basics/upgrade-scripts.md-139-148 (1)
139-148:⚠️ Potential issue | 🟡 Minor
feedbackIdvalue in the example reuses'my-action'— consider'my-feedback'for clarityLine 143 sets
feedbackId: 'my-action', which is the same string used for theactionIddirectly above. Using a distinct value like'my-feedback'makes it easier for readers to tell the two sections apart at a glance.✏️ Suggested fix
feedbacks: [ { id: 'abc', controlId: 'bank:def', - feedbackId: 'my-action', + feedbackId: 'my-feedback', options: {for-developers/module-development/connection-basics/variables.md-85-85 (1)
85-85:⚠️ Potential issue | 🟡 MinorTypo:
setVariablesDefinitions→setVariableDefinitionsLine 85 uses
setVariablesDefinitions(extrasafterVariable), while every other mention in this same file usessetVariableDefinitions. This will cause confusion for readers trying to find the method in the API.✏️ Suggested fix
-In your calls to `setVariablesDefinitions`, you can then type your definitions as `CompanionVariableDefinition<VariablesSchema>`. +In your calls to `setVariableDefinitions`, you can then type your definitions as `CompanionVariableDefinition<VariablesSchema>`.for-developers/module-development/connection-basics/user-configuration.md-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorSubject-verb disagreement: "field types is" → "field types are"
Small grammar catch — "types" is plural so it needs "are".
✏️ Suggested fix
-The fields available for secrets is quite limited, as we expect it to only be useful for API keys, usernames, passwords and similar things. If other field types is useful, let us know and we can look at adding more. +The fields available for secrets is quite limited, as we expect it to only be useful for API keys, usernames, passwords and similar things. If other field types are useful, let us know and we can look at adding more.for-developers/module-development/connection-basics/feedbacks.md-121-121 (1)
121-121:⚠️ Potential issue | 🟡 MinorSmall grammar nit: "generating an images" → "generating images".
✏️ Proposed fix
-You should not be performing any network requests here, but it can be necessary when generating an images or using other native code. +You should not be performing any network requests here, but it can be necessary when generating images or using other native code.for-developers/module-development/connection-basics/presets.md-18-18 (1)
18-18:⚠️ Potential issue | 🟡 MinorMissing space before "much".
✏️ Proposed fix
-In order to add presets to a module, you call `this.setPresetDefinitions(presetsStructure, presetsDefinitions)`much like how you define actions and feedbacks. +In order to add presets to a module, you call `this.setPresetDefinitions(presetsStructure, presetsDefinitions)` much like how you define actions and feedbacks.for-developers/module-development/connection-basics/actions.md-156-156 (1)
156-156:⚠️ Potential issue | 🟡 MinorCopy-paste slip: "feedback type" should be "action type" here.
This line is describing
subscribeActions()/unsubscribeActions(), so "action type" is the right term. "Feedback type" appears to have been carried over from the feedbacks docs. Worth fixing before this lands — developers reading the Actions page will understandably be confused!✏️ Proposed fix
-It is also possible to force either unsubscribe or subscribe to be called for every action, by calling `this.subscribeActions()` or `this.unsubscribeActions()`. Both functions accept `actionIds` parameters, to only run on a certain feedback type (eg `this.unsubscribeActions('set_source', 'set_source2')`). +It is also possible to force either unsubscribe or subscribe to be called for every action, by calling `this.subscribeActions()` or `this.unsubscribeActions()`. Both functions accept `actionIds` parameters, to only run on a certain action type (eg `this.unsubscribeActions('set_source', 'set_source2')`).for-developers/module-development/connection-basics/presets.md-85-85 (1)
85-85:⚠️ Potential issue | 🟡 MinorRedundant "that which" → "that".
✏️ Proposed fix
-There are internal actions that which a user can use to change the step manually. +There are internal actions that a user can use to change the step manually.for-developers/module-development/connection-basics/presets.md-380-380 (1)
380-380:⚠️ Potential issue | 🟡 MinorTypo: "prests" → "presets".
✏️ Proposed fix
-When using typescript, if you strongly type your [actions](./actions.md#typescript-typings) and [feedbacks](./feedbacks.md#typescript-typings) as explained in their respective pages, then in your prests, the API will expect your presets to also be typed as `CompanionPresetDefinitions<MyTypes>`. +When using typescript, if you strongly type your [actions](./actions.md#typescript-typings) and [feedbacks](./feedbacks.md#typescript-typings) as explained in their respective pages, then in your presets, the API will expect your presets to also be typed as `CompanionPresetDefinitions<MyTypes>`.for-developers/module-development/connection-basics/actions.md-48-48 (1)
48-48:⚠️ Potential issue | 🟡 MinorMinor typo: "associate button" → "associated button".
✏️ Proposed fix
-The callback function is called when the action is executed (i.e. associate button is pressed). +The callback function is called when the action is executed (i.e. the associated button is pressed).for-developers/module-development/connection-basics/feedbacks.md-192-192 (1)
192-192:⚠️ Potential issue | 🟡 Minor"on an feedback" → "on a feedback" (article agreement) — same on line 225.
feedbackstarts with a consonant, soa feedbackis correct. This appears on lines 192 and 225.✏️ Proposed 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 feedback 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.-Whenever the options of an feedback on a button is changed, only the callback will be called +Whenever the options of a feedback on a button is changed, only the callback will be calledfor-developers/module-development/connection-basics/feedbacks.md-166-171 (1)
166-171:⚠️ Potential issue | 🟡 MinorMissing spaces in three spots: "providesbuilt-in" and "thebuilt-in" (×2).
Looks like the spaces were dropped when editing these lines. Quick fix:
✏️ Proposed fix
-Since [API v1.5](../api-changes/v1.5.md) (Companion 3.1), Companion providesbuilt-in support for 'inverting' the value of boolean feedbacks. +Since [API v1.5](../api-changes/v1.5.md) (Companion 3.1), Companion provides built-in support for 'inverting' the value of boolean feedbacks.-If your feedback already provides a field to match a true or false state, we strongly advise removing it and replacing existing usage with thebuilt-in invert property. -A helper function (`CreateUseBuiltinInvertForFeedbacksUpgradeScript`) is provided to generate an upgrade script for your module to convert an existing invert checkbox to thebuilt-in system. +If your feedback already provides a field to match a true or false state, we strongly advise removing it and replacing existing usage with the built-in invert property. +A helper function (`CreateUseBuiltinInvertForFeedbacksUpgradeScript`) is provided to generate an upgrade script for your module to convert an existing invert checkbox to the built-in system.
🧹 Nitpick comments (11)
for-developers/module-development/module-setup/code-quality.md (1)
66-66: Nice fix — and one tiny optional polish nearby!The
isn'tcorrection is spot on 👍. If you ever want to trim a small redundancy while you're in the neighbourhood, the static analysis tool suggests that "evolve over time" can simply be "evolve" (time is implied).✏️ Optional word-trim
-We hope that this set of rules will evolve over time based on what is and isn't useful to module developers. +We hope that this set of rules will evolve based on what is and isn't useful to module developers.for-developers/core-development/development-flow.md (1)
119-124: Nice clarity improvement — LGTM! 👍Splitting the original single note into two distinct numbered points makes both prerequisites much easier to scan. Both statements are accurate and consistent with the rest of the document (e.g., the line 158 reminder: "After starting
yarn dev:webui(andyarn dev, as above)").One tiny optional nit: line 122 uses
"not … nor"—"neither … nor"is the slightly more standard pairing, if you'd like to polish it:✏️ Optional wording tweak
-2. `console.log()` code will display in the browser's console, not the command-line nor the Companion logs. +2. `console.log()` code will display in the browser's console, neither in the command-line output nor in the Companion logs.Totally up to you — the current wording is perfectly understandable as-is!
for-developers/module-development/api-changes/v1.14.md (1)
25-25: Optional: comma splice on line 25.The sentence joins two independent clauses with only a comma. A period or semicolon would tighten it up.
✏️ Suggested fix
-Let us know if there is extra configurability that you need, we are open to restoring any needed functionality that is lost. +Let us know if there is extra configurability that you need; we are open to restoring any needed functionality that is lost.for-developers/module-development/module-debugging.md (1)
11-11: Minor: intro list is a sentence fragment and mixes phrasing stylesThe sentence ends with an incomplete list — no period, and the items switch between noun phrases ("console.log") and verb phrases ("log through the API", "interactive debugging"). A tiny polish pass would help readers at a glance.
✏️ Suggested rewrite
-There are three main routes to debugging: log through the API, console.log, interactive debugging +There are three main routes to debugging: the logging API, `console.log`, and attaching an interactive debugger.for-developers/module-development/home.md (1)
56-56: Tiny optional style nit — feel free to take it or leave it!LanguageTool suggests trimming
"all of the places"→"all the places"for conciseness.✏️ Suggested tweak
-The search feature in your IDE is really helpful to find all of the places the name shows up! +The search feature in your IDE is really helpful to find all the places the name shows up!for-developers/module-development/connection-advanced/http-handler.md (1)
97-98:__dirnameis a CJS global — not available in ESM; worth adding a note for module authors.The imports at Lines 97–98 use the ES module
importsyntax, but Line 107 uses__dirname, which is only available in CommonJS. If a module author is using"type": "module"or.mjsfiles, they'll get aReferenceError. Since Companion modules may vary, it would be helpful to add a brief comment pointing authors to the ESM equivalent (import.meta.url+fileURLToPath).💡 Optional note to add
const htmlDir = path.resolve(__dirname, 'html') + // Note: If your module uses ESM, replace __dirname with: + // import { fileURLToPath } from 'url'; const __dirname = path.dirname(fileURLToPath(import.meta.url))Also applies to: 107-107
for-developers/module-development/module-lifecycle/upgrading-a-module-built-for-companion-2.x.md (1)
105-105: Aggregated TODOs — one open item spotted in this file.
Line TODO 105 TODO - write more about async or find some good tutorials/docs/examplesHappy to help draft a short async/Promises primer or suggest some community links if that would save time!
for-developers/module-development/connection-advanced/permissions.md (1)
11-17: Consider showing the fullruntimewrapper in the intro code blockThe prose says "adding to the
runtimeobject in your manifest", but the introductory snippet only shows the innerpermissionskey, with noruntimecontext around it. New module authors may not know where exactly to paste this. A short complete example would make it immediately clear:✏️ Suggested expansion
To enable any of these, start by adding to the `runtime` object in your manifest: ```js +"runtime": { "permissions": { - } + } +}for-developers/module-development/connection-basics/upgrade-scripts.md (1)
44-56: Duplicatedupgrades.tscode block in the API 1.x sectionLines 48–56 repeat the exact same code block from lines 19–25 verbatim. Since it's already shown at the top of the page, you could just reference it there instead of re-pasting it, which would keep the section a bit tidier.
✂️ Suggested simplification
### API 1.x The main entrypoint for modules... -The upgrades.ts file can export a single variable that contains an array of scripts, to be described next. - -```ts -// upgrades.ts - -export const upgradeScripts = [ - // add your scripts here -] -``` - ```ts // main.tsfor-developers/module-development/connection-basics/connecting.md (1)
67-70: Minor nit: prefer!== undefinedover!= undefinedin the TypeScript exampleThe loose
!=will also matchnull(sincenull != undefinedisfalsein JS). In a TypeScript snippet,!== undefinedis the conventional choice and avoids potential confusion for readers learning from the example.✏️ Suggested fix
- this.log('debug', 'Status Changed to ' + state + (message != undefined ? ': ' + message : '')) + this.log('debug', 'Status Changed to ' + state + (message !== undefined ? ': ' + message : ''))src/remark/autoTocPlugin.mjs (1)
12-12:statSyncis imported but never used.
readdirSync+existsSync(line 100) cover subdirectory detection withoutstatSync. Removing it keeps the import clean.🧹 Proposed fix
-import { readFileSync, readdirSync, statSync, existsSync } from 'node:fs' +import { readFileSync, readdirSync, existsSync } from 'node:fs'
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-basics/variables.md
Outdated
Show resolved
Hide resolved
not all, though. some are more appropriate for Julian.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
for-developers/module-development/module-setup/typescript-config.md (1)
68-68:⚠️ Potential issue | 🟡 MinorTiny typo: "alteranate" → "alternate" (or "alternative").
📝 Proposed fix
-We hope to collect some alteranate presets along with recommended. +We hope to collect some alternate presets along with recommended.for-developers/module-development/module-setup/code-quality.md (2)
25-29:⚠️ Potential issue | 🟡 MinorMissing comma in the JSON snippet makes it invalid
The
"lint"entry is missing a trailing comma before"format". Anyone copying this directly intopackage.jsonwould get a parse error.✏️ Suggested fix
"lint:raw": "eslint", -"lint": "yarn lint:raw ." +"lint": "yarn lint:raw .", "format": "prettier -w ."
52-60:⚠️ Potential issue | 🟡 MinorMisleading description — mentions
typescriptRootbut code showsenableTypescriptLine 52 tells readers to "specify a
typescriptRoot", but the code block immediately following only showsenableTypescript: true. These two don't match, which could confuse module authors. Consider either correcting the prose to referenceenableTypescript, or adding atypescriptRootexample to the code block if it's a separate/additional option.✏️ Suggested prose fix (if typescriptRoot is not relevant here)
-If using typescript, you should specify a `typescriptRoot` +If using TypeScript, you should enable TypeScript support via `enableTypescript`:for-developers/git-workflows/installing-git.md (1)
35-38:⚠️ Potential issue | 🟡 MinorConsider using backward-compatible
git configsyntax for broader compatibilityThanks for adding this guidance! One small heads-up:
git config set --globalwas introduced in Git 2.46 (July 2024), which means folks on fresh Windows installs running older bundled versions might run into issues with these commands failing silently.For maximum compatibility, consider just removing the
setkeyword:Alternative syntax (works on all Git versions)
-git config set --global core.autocrlf false -git config set --global core.eol lf +git config --global core.autocrlf false +git config --global core.eol lfAlternatively, if you'd prefer keeping the newer syntax (it is the recommended direction going forward), just adding a quick note like "requires Git 2.46 or later" would help folks on older installs understand what they might encounter.
🧹 Nitpick comments (3)
for-developers/module-development/module-setup/typescript-config.md (1)
36-64: Minor nit: consider usingjsonccode fence language for the tsconfig examples.Both code blocks use
//comments (as filename labels) inside```jsonfences. Sincetsconfig.jsonfiles are actually parsed as JSONC (JSON with Comments) by TypeScript, using```jsoncwould be more semantically accurate and prevent syntax highlighters from treating the comment lines as errors.✨ Suggested tweak
-```json +```jsonc //tsconfig.json { ... }-```json +```jsonc // tsconfig.build.json { ... }for-developers/module-development/module-setup/code-quality.md (1)
66-66: Tiny wording nit: "evolve over time" is slightly redundantLanguageTool flags this — since "evolve" already implies change over time, you could simply write "evolve". Totally optional though!
✏️ Suggested fix
-We hope that this set of rules will evolve over time based on what is and isn't useful to module developers. +We hope that this set of rules will evolve based on what is and isn't useful to module developers.src/remark/autoTocPlugin.mjs (1)
6-6: Tiny grammar nit: "an bulleted" → "a bulleted"✏️ Suggested fix
- * headings, and appends an bulleted TOC to the document. + * headings, and appends a bulleted TOC to the document.
for-developers/module-development/connection-basics/variables.md
Outdated
Show resolved
Hide resolved
|
@Julusian, I think I'm done with this PR for now, too. CodeRabbit still has plenty of issues but I've already spent way, way more time on this than originally anticipated. Of course, as a result, we've really improved the docs, their quality, and the ability for others to contribute. At this point, however, I could be more effective going back to working on code contributions...some of them intentionally delayed to finish moving the documentation to the website. Perhaps you could find someone who is interested in copy-editing? The rabbit is a reasonable guide, too. Meanwhile, the PR is probably good enough to merge to main... pending the following.
All that said, I am not volunteering to write "hand crafted" TOCs, especially considering that it would likely end up a maintenance nightmare -- the very reason you introduced the autoTOC in the first place! If you feel strongly enough, though, I won't object any further on the topic, so you or someone also can make the changes.
|
…dler.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/remark/autoTocPlugin.mjs (1)
167-191:⚠️ Potential issue | 🟠 MajorEmpty
listnode possible when all headings are deeper thandepthThe guard at line 167 checks
page.headings.length > 0, butdepthfiltering happens inside themapcall. If a page has headings but they're all at a level deeper thandepth(e.g., onlyh4s whenauto_toc: 2), thechildrenarray of thelistnode will be empty — an invalid MDAST structure that downstream processors may reject or silently drop.🐛 Proposed fix
- // Bullet list of each heading as a link - if (page.headings.length > 0) { + // Bullet list of each heading as a link + const filteredHeadings = page.headings.filter((h) => h.level <= depth) + if (filteredHeadings.length > 0) { nodes.push({ type: 'list', ordered: false, spread: false, - children: page.headings - .filter((h) => h.level <= depth) - .map((h) => ({ + children: filteredHeadings.map((h) => ({ type: 'listItem', spread: false, children: [ { type: 'paragraph', children: [ { type: 'link', url: `./${page.slug}#${h.id}`, children: parseInlineContent(h.text), }, ], }, ], })), }) }
🧹 Nitpick comments (4)
for-developers/module-development/index.md (1)
7-8: Consider decoupling the prose from the auto-TOC's rendering symbolsThe paragraph hard-codes the visual symbols (
>,└─) that the autoTocPlugin currently emits. If the plugin's output style ever changes, this description will silently go stale and confuse readers. Even a small note like "see the section listing below" instead of spelling out the exact symbol conventions would be more resilient — and honestly a bit easier on first-time visitors too! 😊for-developers/module-development/connection-advanced/http-handler.md (1)
116-128: Synchronous file I/O blocks the Node event loop — nudge readers toward async alternatives.
fs.existsSyncandfs.readFileSyncare fine for a quick example, but they'll stall the entire Companion process on every request if a module copies this verbatim. A small callout comment (or showingfs.promises.readFile+try/catch) would make the example both safer to copy and a better teaching moment.♻️ Suggested async variant
- 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 Companion's side, but for some file types being served you may need to manually set the header - return { - status: 200, - body: fs.readFileSync(filePath, 'utf8') - } + // Prefer async I/O to avoid blocking the event loop + try { + const body = await fs.promises.readFile(filePath, 'utf8') + // Content-Type should generally be handled by Express on Companion's side, + // but for some file types being served you may need to manually set the header + return { status: 200, body } + } catch { + return { + status: 404, + body: JSON.stringify({ status: 404, error: `API endpoint ${endpoint} for connection ${this.label} not found` }) + } + }src/remark/autoTocPlugin.mjs (2)
99-99: Considerf !== vfilenameinstead of!f.startsWith(vfilename)for the self-exclusion filter
startsWithwould also exclude any hypothetical file whose name begins with the current filename (e.g., if the current file wereindex.md, a file namedindex.mdx— unlikely but possible with future format changes).!==is the exact intent.✏️ 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)
126-127: Consider extracting the basename for more robust index file filteringI checked the actual
_category_.jsonfiles in the repo, and all thelink.idvalues are simple slugs like"index"(not multi-segment paths). So the scenario described isn't happening in practice here.That said, the underlying concern is valid: mixing bare slugs from
link.idwith filenames and usingstartsWith()is fragile. If someone had an ID like"intro", it would incorrectly match both"intro.md"and"introduction.md". The proposed fix—extracting the basename and using exact filename comparison—is a solid improvement that makes the code more maintainable and less error-prone.Confirming that
basenameis already imported at line 13, so the fix would work without extra dependencies.
for-developers/module-development/connection-advanced/http-handler.md
Outdated
Show resolved
Hide resolved
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Yeah I'm done too I think. I'm not done with the auto-toc stuff, but I think lets merge it and refine it aferwards
I see this as a difference in that the changelog TOC will be used to quickly figure out if anything is relevant and what page it is on. I suppose a large part of my problem with it is the pulling headings out of each page (which we didnt have before either).
Some of this is solvable with more refinement of what the auto-toc generates, but confusing headings will mean trying to tailor the content to both the page and this toc which will be a constant battle. What about if for these pages the auto-toc was just listing files+folders in their structure, and pulling the description out of each file to give a bit more context?
I'm wondering what the value is in making users aware of this difference? A thought I've just had is that I might need to flag these auto-toc pages to not be scraped by the search, Theyre going to throw a bunch of noise into the search data probably. |
|
Thanks for merging the PR, Julian, and I agree that the pressure is off for now. Perhaps we can see if any "crowd-sourced" editing will happen! Regarding specific points:
I still see things differently here. The documentation both as a whole and even in individual pages can be quite daunting. These TOCs help make it all more accessible.
Not sure what you mean by "we didn't have before". You original version of autoTOC did that -- I haven't touched that part of the code, though I agree that flattening the outline hierarchy can be confusing. Adding hierarchy should be relatively easy to fix in the code, though. Likewise, I'm not sure what you mean about duplication. Files with no headers and therefore no further entries in the TOC doesn't particularly bother me, and they're pretty rare. The 'Further reading' headings don't bother me either but at worst, those can be excluded programmatically.
Again not sure what you mean. I do think headings should be designed with the TOC in mind, but that's true of any written document. In fact, we've partially implemented putting the API calls in the headers, which means the TOC includes a quick-reference to the API calls. I'm not sure if it's a great idea or not, but I think it's an interesting idea!
It's a possible fallback, for sure.
Wait! I did it because you said it was difficult to figure out on their own! |
I meant on the old wiki
Not a current concern, but one coming soon. But those subheadings dont appear to show up in the listing anymore, so maybe its fine.
I mean that I am not sure we should show the links differently. |
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