hiero-ledger / hiero-ledger/hiero-sdk-cpp

[Intermediate]: Split framework markers out of `helpers/comments.js` and rename the dashboard renderer

Open
#1,600 4 comments 0 reactions 0 assignees View on GitHub
priority: low scope: ci skill: intermediate status: ready for dev
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.