hiero-ledger / hiero-ledger/hiero-sdk-cpp
[Intermediate]: Split framework markers out of `helpers/comments.js` and rename the dashboard renderer
- Dominant language
- C++
- Stars
- 42
- Forks
- 108
- Avg merge
- 11h 45m
- Merged PRs (30d)
- 2
Description
### 🧩 Intermediate Friendly
This issue is a good fit for contributors who are already familiar with the Hiero C++ SDK and feel comfortable navigating the codebase.
Intermediate Issues often involve:
- Exploring existing implementations
- Understanding how different components work together
- Making thoughtful changes that follow established patterns
The goal is to support deeper problem-solving while keeping the task clear, focused, and enjoyable to work on.
> [!IMPORTANT]
> ### 🧭 About Intermediate Issues
>
> Intermediate Issues are a great next step for contributors who enjoy digging into the codebase and reasoning about how things work.
>
> These issues often:
> - Involve multiple related files or components
> - Encourage investigation and understanding of existing behavior
> - Leave room for thoughtful implementation choices
> - Stay focused on a clearly defined goal
>
> Other kinds of contributions — from beginner-friendly tasks to large system-level changes — are just as valuable and use different labels.
### 👾 Description of the Task
`.github/scripts/helpers/comments.js` is misnamed and mixes two unrelated responsibilities:
1. **Framework-level concern.** It exports `MARKER`, an HTML marker constant used by the `postOrUpdateComment` flow in `helpers/api.js` to find and update the unified PR Helper Bot dashboard comment. This is part of the bot framework's contract — any code that wants to find-and-update that dashboard comment needs the marker.
2. **PR Helper Bot specific concern.** It also contains the dashboard renderer itself (`buildBotComment`, `buildChecksSection`, `buildDCOSection`, `buildGPGSection`, `buildMergeSection`, `buildIssueLinkSection`, `buildMergeConflictNotificationComment`, `allChecksPassed`). These are not generic helpers — they're the PR Helper Bot's specific implementation.
This mixing shows up as concrete coupling in the codebase:
- `helpers/api.js:18` does `const { buildBotComment } = require('./comments');` — the generic API helper reaches into a specific bot's renderer.
- `helpers/index.js:13` re-exports the dashboard renderer through the generic helpers barrel, so `helpers/` doesn't actually represent "generic helpers" anymore.
- `bot-on-pr-merged.js:19` imports both `MARKER` and `buildMergeConflictNotificationComment` from a path (`./helpers/comments`) that suggests neither is bot-specific.
The naming is also actively misleading for new contributors: someone looking for "the PR helper bot's comment-building logic" wouldn't think to look in `helpers/comments.js`, and someone looking for "shared comment helpers" finds a single bot's implementation.
This is the last holdout from before the comment-builder convention solidified. The convention used elsewhere is **comment builders live next to their owner**:
- Slash commands → `commands/-comments.js` (already correct for `assign`, `finalize`, `unassign`)
- Bots → `bot--comments.js` colocated at the top level (after #1598 lands, `bot-inactivity-comments.js` will follow this shape)
`helpers/comments.js` should align with the bot half of that rule.
**Relevant files:**
```
.github/scripts/helpers/comments.js
.github/scripts/helpers/api.js
.github/scripts/helpers/index.js
.github/scripts/bot-on-pr-merged.js
.github/scripts/tests/test-comments.js
```
### 💡 Proposed Approach
Two-step separation that resolves both responsibilities:
1. **Framework-level marker → `helpers/`.** The marker constant used by `postOrUpdateComment` should live next to that flow. Two acceptable shapes:
- Inline the constant directly into `helpers/api.js` (simplest if the dashboard MARKER is the only such constant).
- Create a small `helpers/markers.js` if it makes sense to colocate other markers.
The chosen shape is a judgment call for the implementer based on whether other markers want to live there. Prefer the simpler option unless there's a concrete reason not to.
2. **Dashboard renderer → top-level `bot-pr-helper-comments.js`.** Move the PR Helper Bot's rendering exports (`buildBotComment`, `buildChecksSection`, `buildDCOSection`, `buildGPGSection`, `buildMergeSection`, `buildIssueLinkSection`, `buildMergeConflictNotificationComment`, `allChecksPassed`) into a new file at `.github/scripts/bot-pr-helper-comments.js`, colocated with the rest of the top-level bot scripts.
After the move:
- `helpers/index.js` should re-export only genuinely generic infrastructure (`constants`, `logger`, `validation`, `api`, `checks`). The dashboard renderer leaves the helpers barrel.
- `helpers/api.js` imports the marker from its new home (or uses it inline if that's where it ended up).
- `bot-on-pr-merged.js` imports `MARKER` from the framework location and `buildMergeConflictNotificationComment` from `bot-pr-helper-comments.js`.
- `tests/test-comments.js` is updated to point at the new module path.
**Open question for the implementer:** which top-level bot script "owns" the dashboard? Today multiple `bot-on-pr-*.js` scripts touch it. Two reasonable answers:
- (a) Leave `bot-pr-helper-comments.js` as a peer of all of them at the top level with no single owning `.js` — the comment file stands alone, imported by whichever bot needs it.
- (b) Consolidate the dashboard-touching logic into a single owning `.js` file.
Option (a) is correct for this issue. Option (b) is bigger scope and should not be undertaken here.
**Out of scope:**
- The colocated extraction of `bot-inactivity-comments.js` (#1598). Independent; can land before, after, or in parallel.
- Reorganizing `bot/bot-recommend-issues.js` (which has comment builders inline). Separate beginner-level issue.
- Whether the `bot/` subdirectory (one file) should be flattened. Separate housekeeping.
- Any change to the contents or behavior of any bot comment. This is purely move/rename.
### 👩💻 Implementation Steps
- [ ] Read `helpers/comments.js`, `helpers/api.js`, and `helpers/index.js` to confirm the full set of imports and exports involved.
- [ ] Decide where the framework-level `MARKER` lives (inline in `helpers/api.js` or in a new `helpers/markers.js`). Document the decision briefly in the PR description.
- [ ] Create `.github/scripts/bot-pr-helper-comments.js` and move the dashboard-rendering exports there:
- `buildBotComment`
- `buildChecksSection`
- `buildDCOSection`
- `buildGPGSection`
- `buildMergeSection`
- `buildIssueLinkSection`
- `buildMergeConflictNotificationComment`
- `allChecksPassed`
- Internal helpers (`buildSection`, `checkState`, the `SIGNING_GUIDE` / `MERGE_CONFLICTS_GUIDE` URL constants) move with them.
- [ ] Update `helpers/api.js` to import `MARKER` from its new home, not from `helpers/comments`.
- [ ] Update `helpers/index.js` to drop the `comments` re-export.
- [ ] Update `bot-on-pr-merged.js` to import `MARKER` from the framework location and `buildMergeConflictNotificationComment` from `bot-pr-helper-comments.js`.
- [ ] Search for any other consumers reaching the dashboard renderer through the helpers barrel (`grep -r "require.*helpers'" .github/scripts | grep -v node_modules`) and update their imports to point directly at `bot-pr-helper-comments.js`.
- [ ] Delete `helpers/comments.js`.
- [ ] Update `tests/test-comments.js`:
- Adjust `require` paths to the new module location.
- Confirm test names and assertions still describe behavior accurately (rename if needed).
- [ ] Run the bot script test suite locally:
```bash
cd .github/scripts && npm test
```
- [ ] Lint:
```bash
cd .github/scripts && npx eslint .
```
- [ ] Open a pull request referencing this issue.
### ✔️ Acceptance Criteria
- [ ] `.github/scripts/helpers/comments.js` no longer exists.
- [ ] `helpers/api.js` does not import any bot-specific code; the marker it needs is reachable from `helpers/` only.
- [ ] `helpers/index.js` re-exports only generic infrastructure (`constants`, `logger`, `validation`, `api`, `checks`).
- [ ] `.github/scripts/bot-pr-helper-comments.js` exists and contains the dashboard renderer.
- [ ] All existing consumers (`helpers/api.js`, `bot-on-pr-merged.js`, anything reachable via the helpers barrel) import from the new locations.
- [ ] `tests/test-comments.js` is updated, ESLint is clean, and the bot script test suite passes.
- [ ] No behavioral change to any bot comment — the dashboard renders identically before and after the change.
- [ ] No unrelated changes to bot logic, behavior, or other files.
---
### 📋 Step-by-Step Contribution Guide
To help keep contributions consistent and easy to review, we recommend following these steps:
- [ ] Comment `/assign` to request the issue
- [ ] Wait for assignment
- [ ] Fork the repository and create a branch
- [ ] Set up the project using the instructions in `README.md`
- [ ] Make the requested changes
- [ ] Sign each commit using `-s -S`
- [ ] Push your branch and open a pull request
Read [Workflow Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/workflow.md) for step-by-step workflow guidance.
Read [README.md](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/README.md) for setup instructions.
❗ Pull requests **cannot be merged** without `S` and `s` signed commits.
See the [Signing Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/signing.md).
### 🤔 Additional Information
This issue is independent of #1598 (extract `bot-inactivity-comments.js`). Either can land first.
The convention this issue codifies — _comment builders live next to their owner_ — is documented implicitly by the `commands/` directory structure today. Once this issue and #1598 land, the rule will be uniformly applied across `commands/` and the top-level `bot-*.js` scripts.
Note for the implementer: `bot/bot-recommend-issues.js` also has its comment builders inline. That's a separate beginner-level extraction that should follow the same `bot-recommend-issues-comments.js` pattern, but it is **not** part of this issue.
Contributor guide
Research direction
Read .github/scripts/helpers/comments.js, helpers/api.js, helpers/index.js, and bot-on-pr-merged.js to trace the marker and dashboard-renderer imports. Move the renderer to bot-pr-helper-comments.js, update tests/test-comments.js and all consumers, then run npm test and npx eslint . from .github/scripts. Done means the old module is gone, imports are separated, tests and lint pass, and comment output is unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- testing, tooling
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100