docs: desktop UI guidelines and dev-app-verify skill - #3445
Conversation
Distills the lessons from the downloads indicator work (PRs #3441 and #3443) into permanent project knowledge: - docs/desktop-ui-guidelines.md: Fuselage token semantics and traps (runtime-injected tokens, status-background vs bullet vs badge, the icon-button color chain), icon/button geometry facts, the .18s micro-interaction standard, tab bar button conventions (the 0.6 dimming rule and its override, square icon-only buttons, compact titlebar sizing, flex-flow traps), and the SVG transform-origin clipping pitfall - skills/dev-app-verify/SKILL.md: ready-made script to drive and screenshot the running dev app through the port-9339 inspector, with the three pitfalls that produce false alarms (occlusion, watcher restarts, singleton wedges) — plain Markdown, usable by any agent like skills/desktop-qa-flows - CLAUDE.md: pointers to both from the UI and Testing sections
|
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:
WalkthroughAdded canonical agent guidance, desktop UI and application verification procedures, GitNexus skills, Electron upgrade and release workflows, and specific Claude configuration. ChangesDesktop UI Guidance
Repository Analysis Skills
Release Automation Skills
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
CLAUDE.md got the pointers but AGENTS.md — what non-Claude agents read — did not; both files now direct agents to docs/desktop-ui-guidelines.md from UI Work and to skills/dev-app-verify from Testing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
skills/dev-app-verify/SKILL.md (1)
100-104: 🚀 Performance & Scalability | 🔵 TrivialKeep the 1 GB download explicitly opt-in.
This recipe consumes substantial bandwidth and depends on a third-party mirror. Prefer a small local or controlled fixture for routine verification. Keep the large external download only as an explicit stress test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/dev-app-verify/SKILL.md` around lines 100 - 104, Update the download verification guidance around the “Real download with known size” recipe to clearly mark the 1 GB external download as an explicit opt-in stress test. Recommend a small local or controlled fixture for routine verification, while retaining the large mirror-based download only for intentional stress testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/desktop-ui-guidelines.md`:
- Around line 37-39: Update the percent-text width guidance in the “UpdateLabel”
convention to reserve space for the terminal 100% value by using 4ch, or
explicitly document that 100% is replaced with the completion dot; keep the
monospace and tabular-nums requirements unchanged.
- Around line 95-97: Update the progress-arc guidance to explicitly define the
progress unit and align the dash-offset formula with the value emitted by
src/updates/main.ts. Either document that progress is normalized to [0, 1]
before applying the formula, or use the update-store’s 0–100 value with progress
divided by 100.
In `@skills/dev-app-verify/SKILL.md`:
- Around line 52-55: Update the inspector command flow around send to add a
timeout for each pending request that rejects and removes it when it expires.
Register ws.onerror and ws.onclose handlers that reject and clear every
remaining pending request, ensuring watcher restarts or socket closure cannot
leave verification hanging.
- Around line 57-63: Update the ev helper to throw when the CDP response
contains a top-level r.error, while preserving its exceptionDetails handling.
Before invoking the menu action, validate that developerMode and
simulateDownload exist and are enabled; fail immediately when either
prerequisite is missing or disabled, and only print clicked after the action
succeeds.
- Around line 32-35: Update the “Singleton wedges” recovery instructions to
locate the specific Electron PID and terminate it gracefully instead of using a
broad pkill command. Include an equivalent Windows command, then document
force-killing only as a last resort, while preserving the wait-and-restart
sequence.
- Around line 68-70: Update the Electron window selection in the verification
script and its repeated uses near the later window operations to identify the
root BrowserWindow deterministically, rather than choosing the first
non-destroyed window. Reuse the same stable root-window predicate or helper for
showing, focusing, querying, and capturing so every operation targets the
identical window.
- Around line 91-94: Update the screenshot write logic inside the capturePage
callback to build the output path with os.tmpdir() and path.join() instead of
the hardcoded /tmp/ui_check.png path, while preserving the existing
writeFileSync and return behavior.
---
Nitpick comments:
In `@skills/dev-app-verify/SKILL.md`:
- Around line 100-104: Update the download verification guidance around the
“Real download with known size” recipe to clearly mark the 1 GB external
download as an explicit opt-in stress test. Recommend a small local or
controlled fixture for routine verification, while retaining the large
mirror-based download only for intentional stress testing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b113bad5-89c3-45d4-a08a-1290c1d74806
📒 Files selected for processing (3)
CLAUDE.mddocs/desktop-ui-guidelines.mdskills/dev-app-verify/SKILL.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (windows-latest)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: Never commit or push without explicit user permission; never commit directly to `master` or `dev`; show what will be committed before committing.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: Use worktrees to avoid disrupting the user's working directory when making changes.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: Before modifying any function, class, or method, run GitNexus `impact` analysis and report the blast radius; warn the user before proceeding when risk is HIGH or CRITICAL.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: Run GitNexus `detect_changes()` before committing to verify that only expected symbols and execution flows changed; for regression reviews, compare against `master`.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: Never rename symbols with find-and-replace; use GitNexus `rename` to preserve call-graph awareness.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: Understand why existing code works before changing it, diagnose failed approaches before retrying, and ask when uncertain.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: Use measurable descriptions, avoid subjective descriptors, never invent metrics, and write PR descriptions that focus on what changed and why.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: For Windows builds, include x64, ia32, and arm64 architectures; build packages without signing first, then sign them with jsign and Google Cloud KMS.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T02:54:34.176Z
Learning: UI changes require runtime or visual verification; screen-capture, WebRTC, and portal behavior must be validated on hardware GL, preferring dbus-level portal assertions over dialog visibility.
🪛 LanguageTool
CLAUDE.md
[grammar] ~48-~48: Ensure spelling is correct
Context: ...age patterns - **Before styling tab bar/titlebar controls, custom SVG artwork, or pickin...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/desktop-ui-guidelines.md
[grammar] ~3-~3: Ensure spelling is correct
Context: ...ield guide distilled from the downloads titlebar indicator work (PRs #3441 and #3443). E...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~109-~109: Ensure spelling is correct
Context: ...app menu, tab bar meatball popup, and titlebar server-switcher menu all carry `Simul...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (1)
CLAUDE.md (1)
48-48: LGTM!Also applies to: 57-57
| - Percent text that must not jitter: monospace + `font-variant-numeric: | ||
| tabular-nums` + fixed `3ch` width (two digits + `%`) — the `UpdateLabel` | ||
| convention; reuse it, don't invent widths. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reserve width for 100%.
src/updates/main.ts publishes progress from 0 through 100. A fixed 3ch width fits 99%, not 100%. If UpdateLabel renders the terminal percentage, the text can overflow or shift the titlebar. Use 4ch, or document that 100% is replaced by the completion dot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/desktop-ui-guidelines.md` around lines 37 - 39, Update the percent-text
width guidance in the “UpdateLabel” convention to reserve space for the terminal
100% value by using 4ch, or explicitly document that 100% is replaced with the
completion dot; keep the monospace and tabular-nums requirements unchanged.
| 3. **Singleton wedges.** If the inspector port refuses connections while an | ||
| Electron process exists, two instances raced the SingletonLock. Recovery: | ||
| `pkill -9 -f "<worktree-name>"`, wait, single fresh `yarn start` | ||
| (cold boot ≈ 30s). |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the recovery command safe and cross-platform.
pkill -9 -f "<worktree-name>" matches full command lines and force-kills every matching process. It is also unavailable in standard Windows shells. Prefer locating the Electron PID, terminate it gracefully, and provide a Windows command plus a last-resort force-kill.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/dev-app-verify/SKILL.md` around lines 32 - 35, Update the “Singleton
wedges” recovery instructions to locate the specific Electron PID and terminate
it gracefully instead of using a broad pkill command. Include an equivalent
Windows command, then document force-killing only as a last resort, while
preserving the wait-and-restart sequence.
There was a problem hiding this comment.
Out of scope by design: this skill drives the local macOS dev machine (yarn start + main-process inspector). Added an explicit scope note in 23da37b stating the commands are macOS-specific; no Windows variants intended.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="skills/dev-app-verify/SKILL.md"
if [[ ! -f "$file" ]]; then
echo "Missing expected file: $file" >&2
exit 1
fi
nl -ba "$file" | sed -n '1,50p'Length of output: 530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="skills/dev-app-verify/SKILL.md"
if [[ ! -f "$file" ]]; then
echo "Missing expected file: $file" >&2
exit 1
fi
sed -n '1,50p' "$file"Length of output: 2841
@jeanfbrito, verified. The new scope note makes the Windows-compatibility part of my finding inapplicable.
The safety concern remains. pkill -9 -f "<worktree-name>" still force-terminates every matching process on the local macOS machine. The skill should identify the intended Electron process and use normal termination first. It can retain pkill -9 as a documented last resort.
✏️ Learnings added
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3445
File: skills/dev-app-verify/SKILL.md:35-38
Timestamp: 2026-08-10T12:25:08.779Z
Learning: In `skills/dev-app-verify/SKILL.md`, the `dev-app-verify` skill is intentionally scoped to a local macOS development machine. Its commands, including `pkill` and `/tmp` paths, are macOS-specific by design, and the skill must not provide Windows command variants.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
The blanket .claude/ gitignore was hiding project knowledge: CLAUDE.md references .claude/skills/gitnexus/* six times, and skills like electron-build, ship-release, and boot-wedge-debug encode repo processes any Claude Code user needs. The ignore is now selective — only genuinely personal/machine state stays out (settings.local.json, mytasks/, worktrees/, rules/, commands/, scheduled_tasks.lock) — and the skills plus the hooks-only settings.json are committed. The dev-app-verify Claude stub points at the canonical skills/dev-app-verify/SKILL.md so the two copies cannot drift.
The two files carried parallel, drifting versions of the same guidance that had to be edited twice. AGENTS.md is now the single source (union of both, restoring details each copy had dropped: KMS two-phase rationale, worktree commands, working principles, the screen-capture VM constraint, the GitNexus block with its tool-managed markers), and CLAUDE.md is a thin @AGENTS.md import with a marked spot for genuinely Claude-specific additions.
The stub's guidance was an HTML comment a rushed session could ignore; it is now a visible imperative rule — loaded into every session's context — telling agents to record shared guidance in AGENTS.md and reserve this file for genuinely Claude-only content.
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
.claude/skills/gitnexus/gitnexus-cli/SKILL.md (1)
66-66: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRequire confirmation before publishing a public gist.
--gistpublishes repository-derived documentation as a public GitHub Gist. Add an explicit warning and require confirmation before an agent uses this option. Generated documentation can disclose internal architecture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/gitnexus/gitnexus-cli/SKILL.md at line 66, Update the --gist option guidance in the gitnexus CLI documentation to include an explicit warning that it publishes repository-derived documentation publicly and may disclose internal architecture. Require the agent to obtain explicit user confirmation immediately before using --gist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/electron-bump/SKILL.md:
- Around line 16-18: Update the Electron version validation workflow to compare
the resolved target version with CURRENT using semver. Reject and stop for
targets older than CURRENT, unless the user explicitly confirms that a downgrade
is intended; preserve the existing handling for equal or newer targets and the
multi-major warning.
- Line 155: Update the commit-message guidance in the Electron bump workflow to
remove the unavailable (#<PR>) placeholder from the initial conventional commit
message; retain the existing version placeholders and breaking-change body
guidance without adding an amend or force-push workflow.
- Line 85: Update the unused-API guidance in the electron-bump skill so
`webContents.printToPDF`, `remote`, `BrowserView`, and `WebContentsView` are
treated as investigation hints rather than automatic exclusions; if research
identifies one as breaking, require querying GitNexus before dismissing it.
- Around line 12-14: Update the fenced code block containing the /electron-bump
[version] command in the electron-bump skill documentation to include a text or
console language identifier, preserving the command content unchanged.
- Line 133: Update the builder-editing workflow in the electron-bump skill to
require running mcp__gitnexus__impact before each function, class, or method
edit; pause and warn when the impact result is HIGH or CRITICAL, rather than
assessing blast radius mentally.
- Line 39: Pin the GitNexus and TypeScript invocations in
.claude/skills/electron-bump/SKILL.md: at lines 39, 142, and 161, replace npx
gitnexus analyze --skip-agents-md with a fixed-version or repository-local
wrapper command, and invoke tsc --noEmit through the repository’s
lockfile-pinned version. Ensure all affected instructions use reproducible
commands consistently.
In @.claude/skills/gitnexus/gitnexus-cli/SKILL.md:
- Line 10: Pin GitNexus to the approved package version in every bootstrap
command within the analyzed-runner guidance, including npx, global npm
installation, and pnpm dlx usage. Replace implicit/default resolution and
gitnexus@latest while preserving the existing fallback flow and build flags.
- Around line 8-10: Update the runner selection described in the skill
documentation to try bunx before the existing global gitnexus, pnpm dlx, and npx
fallbacks, and update the “no package-manager assumption” wording to accurately
reflect the supported runners.
In @.claude/skills/gitnexus/gitnexus-exploring/SKILL.md:
- Line 18: Add language identifiers to every opening fenced code block: use
text, jsonc, or cypher according to the example content. Update
.claude/skills/gitnexus/gitnexus-exploring/SKILL.md lines 18, 30, 52, 60, and
69; .claude/skills/gitnexus/gitnexus-debugging/SKILL.md lines 18, 29, 53, 61,
and 77; .claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md lines 19, 30,
60, 78, and 88; and .claude/skills/gitnexus/gitnexus-refactoring/SKILL.md lines
18, 31, 41, 52, 66, 75, 83, and 108.
In @.claude/skills/gitnexus/gitnexus-guide/SKILL.md:
- Around line 10-18: Make GitNexus repository discovery and freshness checks
mandatory across all workflows. In
.claude/skills/gitnexus/gitnexus-guide/SKILL.md, require reading
gitnexus://repos before substituting a repository name, then use that selected
repository for query, context, impact, and detect_changes, with
gitnexus://repo/{name}/context checked before repository-scoped tools. Apply the
same precondition and workflow ordering in
.claude/skills/gitnexus/gitnexus-exploring/SKILL.md (19-23),
.claude/skills/gitnexus/gitnexus-debugging/SKILL.md (18-25),
.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md (19-24), and
.claude/skills/gitnexus/gitnexus-refactoring/SKILL.md (18-25); the guide anchor
is the primary change and each sibling must explicitly enforce the same
repository-selection and freshness behavior.
In @.claude/skills/ship-release/SKILL.md:
- Line 58: Update both fenced code blocks in the ship-release documentation to
include an explicit sh or shell language identifier, resolving the MD040
warnings while preserving their existing contents.
- Around line 46-48: Update the Phase 2 workflow before the commit and push step
to require explicit user approval for committing and pushing the release branch;
do not proceed with these actions based only on prior release instructions.
Preserve the existing version bump and PR creation steps after approval.
- Around line 44-45: Update the release workflow instructions around the
worktree creation command to make entering the newly created worktree mandatory,
record its path, and ensure every subsequent edit, commit, HEAD detachment, and
tagging command executes within that worktree via an explicit working-directory
scope. Keep the user's original checkout untouched throughout the release flow.
- Around line 92-95: Update the release sequence around the Phase 1 release
notes so alpha releases receive the --prerelease flag while still drafted,
before the publishing gate and gh release edit --draft=false step. Keep the
existing confirmation gate, and retain the stable-release flow without applying
prerelease.
- Around line 29-32: Update the release-baseline steps around TARGET resolution
and the last-tag range to parse tags with the channel-aware semantic-version
logic used by scripts/release-tag.ts. Select the maximum stable tag rather than
relying on creation-date ordering, then apply the existing alpha promotion
behavior and use that baseline for the commit range.
- Around line 66-70: Update the manual tag fallback commands in the release
instructions to fail closed: exit with status 1 when the tag already exists,
resolve the tag commit and explicitly compare it with $MERGE_SHA, and retain the
same version and channel guard enforced by scripts/release-tag.ts before pushing
the tag.
---
Nitpick comments:
In @.claude/skills/gitnexus/gitnexus-cli/SKILL.md:
- Line 66: Update the --gist option guidance in the gitnexus CLI documentation
to include an explicit warning that it publishes repository-derived
documentation publicly and may disclose internal architecture. Require the agent
to obtain explicit user confirmation immediately before using --gist.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a3e28796-49bd-42c0-a996-6ac62eb6095a
📒 Files selected for processing (10)
.claude/skills/dev-app-verify/SKILL.md.claude/skills/electron-bump/SKILL.md.claude/skills/gitnexus/gitnexus-cli/SKILL.md.claude/skills/gitnexus/gitnexus-debugging/SKILL.md.claude/skills/gitnexus/gitnexus-exploring/SKILL.md.claude/skills/gitnexus/gitnexus-guide/SKILL.md.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md.claude/skills/gitnexus/gitnexus-refactoring/SKILL.md.claude/skills/ship-release/SKILL.md.gitignore
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (windows-latest)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Use root workspace commands for builds; never run `yarn build` directly inside workspace directories.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Prefer editing existing files over creating new ones.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Never commit or push without explicit user permission.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Never commit directly to `master` or `dev`; create a branch, test, and open a pull request. Show what will be committed before committing.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Use worktrees to avoid disrupting the user's working directory.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Understand why existing code works before changing it; treat working code as correct until proven otherwise and ask when unsure.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Verify work by running tests, checking types with `npx tsc --noEmit`, and demonstrating correctness; never mark a task done without proof.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Diagnose why an approach failed before iterating; do not cycle through three or more approaches blindly.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Before editing any function, class, or method, run GitNexus `impact({target: "symbolName", direction: "upstream"})` and report the blast radius.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Warn the user before proceeding when GitNexus impact analysis reports HIGH or CRITICAL risk.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Use GitNexus `query()` when exploring unfamiliar code and `context()` when full symbol caller, callee, and execution-flow context is needed.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Never rename symbols with find-and-replace; use GitNexus `rename`, which understands the call graph.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:07:43.854Z
Learning: Before committing, run GitNexus `detect_changes()` to verify that only expected symbols and execution flows changed; for regression review, compare against `master`.
🪛 LanguageTool
.claude/skills/electron-bump/SKILL.md
[uncategorized] ~67-~67: The official name of this software platform is spelled with a capital “H”.
Context: ...actually requires it. | | CI Node pin | .github/workflows/build-release.yml, `pull-req...
(GITHUB)
[style] ~157-~157: Consider an alternative for the overused word “exactly”.
Context: ...oke-test the built installers — this is exactly the case the label is for. PR body: ...
(EXACTLY_PRECISELY)
🪛 markdownlint-cli2 (0.23.2)
.claude/skills/gitnexus/gitnexus-exploring/SKILL.md
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/skills/gitnexus/gitnexus-refactoring/SKILL.md
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 108-108: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/skills/ship-release/SKILL.md
[warning] 58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/skills/gitnexus/gitnexus-debugging/SKILL.md
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/skills/electron-bump/SKILL.md
[warning] 12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SkillSpector (2.5.1)
.claude/skills/dev-app-verify/SKILL.md
[warning] 6: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
.claude/skills/gitnexus/gitnexus-cli/SKILL.md
[warning] 10: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
.claude/skills/electron-bump/SKILL.md
[warning] 39: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 142: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 161: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
🔇 Additional comments (6)
.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md (1)
41-54: 🎯 Functional CorrectnessKeep
WILL BREAKfor depth 1.GitNexus impact analysis defines depth-1 results as direct callers/importers under the
WILL BREAKlabel, so changing this wording would not match the tool’s documented risk bands.> Likely an incorrect or invalid review comment..gitignore (1)
34-39: LGTM!.claude/skills/dev-app-verify/SKILL.md (1)
1-13: LGTM!.claude/skills/ship-release/SKILL.md (3)
1-18: LGTM!Also applies to: 20-28, 33-43, 52-56, 71-81, 96-108
49-50: 🗄️ Data Integrity & IntegrationNo change needed.
validate-pris defined in.github/workflows/validate-pr.ymland runs onubuntu-latest,macos-latest, andwindows-latest, matching the release-skill instruction.
82-91: 🗄️ Data Integrity & IntegrationNo change needed.
The asset matrix matches the configured Windows, macOS, and Linux release artifacts.
| - `version` given (e.g. `41.2.0`) → target that exact release. | ||
| - `version` omitted → resolve **latest stable** from npm (`npm view electron version`), confirm it with the user before proceeding. | ||
| - Never silently skip across multiple majors. If target is 2+ majors ahead of current, warn and ask whether to step through intermediate majors one at a time. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject targets older than CURRENT.
The workflow accepts any exact version but does not detect downgrades. A typo or stale target can install an older Electron release while the later phases treat it as an upgrade. Compare the target and current semver values, then stop unless the user explicitly requests a downgrade.
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 39: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 142: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 161: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/electron-bump/SKILL.md around lines 16 - 18, Update the
Electron version validation workflow to compare the resolved target version with
CURRENT using semver. Reject and stop for targets older than CURRENT, unless the
user explicitly confirms that a downgrade is intended; preserve the existing
handling for equal or newer targets and the multi-major warning.
- AGENTS.md Working Principles: reindex with --index-only at quiet points — a plain background analyze rewrites the tracked gitnexus doc blocks (stats churn) and mutates worktree git state (drops staged index entries, touches watched sources restarting the dev app) - dev-app-verify skill: the same interference documented as pitfall 4
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 84-89: Update the runtimeDir example to use the /run/user/<uid>
fallback only when process.platform is "linux"; provide an appropriate
platform-specific directory for macOS and Windows while preserving the optional
process.getuid() handling.
- Around line 200-202: Update the GitNexus guidance near the project name to
remove the hard-coded symbol, relationship, and execution-flow counts while
preserving “Rocket.Chat.Electron.” Direct agents to obtain current counts from
GitNexus, or replace them with generated metadata that includes a timestamp and
refresh rule.
In `@CLAUDE.md`:
- Around line 5-10: Update the routing rule in the guidance around “Where to
write” so only shared project guidance, conventions, and memory are redirected
to AGENTS.md; keep Claude-only skill and hook mechanics in CLAUDE.md. Preserve
the existing canonical AGENTS.md and imported-guide behavior for shared content.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 226aea16-a324-4346-8edc-a3516a4cfd8f
📒 Files selected for processing (2)
AGENTS.mdCLAUDE.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use camelCase for file names and PascalCase for component names.
Prefer editing existing files over introducing abstractions unless the abstraction removes real complexity or matches an existing pattern.
Understand existing code before changing it, verify work with appropriate tests and type checks, and diagnose failed approaches before retrying.
Files:
CLAUDE.mdAGENTS.md
**/*.{md,txt}
📄 CodeRabbit inference engine (AGENTS.md)
Use measurable descriptions in writing, avoid subjective descriptors, never invent metrics, and write PR descriptions plainly with what changed and why.
Files:
CLAUDE.mdAGENTS.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:46.174Z
Learning: Run root commands from the repository root; do not run `yarn build` inside workspace directories.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:46.174Z
Learning: Never commit or push without explicit user permission; never commit directly to `master` or `dev`.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:46.174Z
Learning: Before committing, show what will be committed and run GitNexus `detect_changes()` to verify the affected scope.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:46.174Z
Learning: Before editing any function, class, or method, run GitNexus upstream impact analysis; warn the user if the risk is HIGH or CRITICAL.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:46.174Z
Learning: Never rename symbols with find-and-replace; use GitNexus symbol-aware rename instead.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:46.174Z
Learning: Use two-phase code signing: build packages without signing, then sign them with `jsign` and Google Cloud KMS.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:46.174Z
Learning: After building `desktop-release-action`, remove `workspaces/desktop-release-action/dist/dist`; only `workspaces/desktop-release-action/dist/index.js` is needed.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:49.647Z
Learning: When recording project guidance, conventions, or memory (including requests to add content to CLAUDE.md, memory notes, or /init output), edit the canonical AGENTS.md file instead of appending shared guidance to this file.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-09T03:17:49.647Z
Learning: Only include content below this instruction that exclusively concerns Claude Code, such as skill or hook mechanics that no other agent uses.
🪛 LanguageTool
AGENTS.md
[grammar] ~34-~34: Ensure spelling is correct
Context: ...tch-package: patches/, currently for @kayahr/jest-electron-runner. - Never add @ewsjs/xhrpatches topatch...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~124-~124: Consider using “impractical” to avoid wordiness.
Context: ...e-path proof when runtime validation is not practical. Write the visible path directly in th...
(NOT_ABLE_PREMIUM)
[style] ~216-~216: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...L risk warnings from impact analysis. - NEVER rename symbols with find-and-replace — ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~217-~217: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mewhich understands the call graph. - NEVER commit changes without runningdetect_...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (1)
AGENTS.md (1)
4-7: 📐 Maintainability & Code QualityResolve the filename-guideline conflict.
This change establishes
AGENTS.mdandCLAUDE.md, but the supplied**/*rule requires camelCase filenames. If agent tooling requires these exact names, add an explicit exception for reserved instruction files. Otherwise, rename the files and update all loader and reference paths.
AGENTS.md#L4-L7: Document the reserved-name exception or use a camelCase filename.CLAUDE.md#L3-L10: Apply the same exception or update the imported filename.As per coding guidelines, files matching
**/*must use camelCase names.Source: Coding guidelines
Derived from transcript analysis (29 recent sessions): the DoD verification commands agents run constantly (yarn test/lint, npx tsc --noEmit, the gitnexus runner) and the read-only GitNexus/context-mode MCP tools that AGENTS.md mandates. No interpreters, task-runner wildcards, or mutating commands — those still prompt.
Salvaged from the retired OpenWolf buglog: Fuselage Select is backed by react-aria useSelect, which only accepts a visible label, aria-label, or aria-labelledby — a labeled wrapper does not satisfy it.
- ship-release: semver-sorted baseline tags, worktree-pinned commands, approval gate before first push, fail-closed manual tag fallback, prerelease flag applied while still a draft - dev-app-verify: per-request CDP timeouts + pending rejection on socket loss, fail-closed menu trigger (developerMode gate + item checks), stable root-window predicate, macOS-only scope note - electron-bump: downgrade guard, unused-API list as hint not exclusion, drop unresolved PR placeholder from commit message - desktop-ui-guidelines: min-width 3ch wording, progress normalization note - AGENTS.md: drop point-in-time GitNexus counts - CLAUDE.md: route shared vs Claude-only guidance by content - markdown fence language identifiers across skill docs
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
AGENTS.md (1)
212-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDisambiguate GitNexus symbol targets.
target: "symbolName"andname: "symbolName"can return multiple matches for shared symbol names. Require a GitNexus UID,file_path, or supportedkindqualifier before relying on impact/context results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 212 - 216, Update the GitNexus usage requirements in AGENTS.md so impact() and context() targets are disambiguated with a GitNexus UID, file_path, or supported kind qualifier whenever symbol names are ambiguous. Preserve the existing upstream impact-analysis, risk-warning, and detect_changes requirements while ensuring results are not relied on from an unqualified shared symbol name..claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md (3)
19-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck index freshness before running impact analysis.
The workflow runs
impactbefore it readsgitnexus://repo/{name}/context. The canonical GitNexus guide in.claude/skills/gitnexus/gitnexus-guide/SKILL.mdrequires the context read first so the agent can check index freshness. If the index is stale, runnode .gitnexus/run.cjs analyzefrom the repository root, then re-read context before runningimpact,processes, ordetect_changes.Based on learnings, run root commands from the repository root.
Proposed workflow order
-1. impact({target: "X", direction: "upstream"}) → What depends on this -2. READ gitnexus://repo/{name}/processes → Check affected execution flows -3. detect_changes() → Map current git changes to affected flows -4. Assess risk and report to user +1. READ gitnexus://repo/{name}/context → Confirm index freshness +2. If stale, run `node .gitnexus/run.cjs analyze` from the repository root +3. Re-read `gitnexus://repo/{name}/context` +4. impact({target: "X", direction: "upstream"}) → What depends on this +5. READ gitnexus://repo/{name}/processes → Check affected execution flows +6. detect_changes(...) → Map the intended changes +7. Assess risk and report to user🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md around lines 19 - 26, Update the workflow in the impact-analysis skill to read gitnexus://repo/{name}/context before invoking impact, processes, or detect_changes. Check the context for index freshness; if stale, run node .gitnexus/run.cjs analyze from the repository root and re-read context before continuing. Preserve the existing risk-assessment step and explicitly require root commands to run from the repository root.Source: Learnings
8-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace subjective workflow and risk terms.
non-trivial,few processes,many processes, andcritical pathdo not define measurable boundaries. Replace them with numeric ranges or explicit project criteria. This prevents different agents from choosing different analysis and risk levels.As per coding guidelines, use measurable descriptions instead of subjective descriptors.
Also applies to: 49-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md around lines 8 - 16, Update the “When to Use” guidance and the corresponding section around the additional referenced lines to replace subjective terms such as “non-trivial,” “few processes,” “many processes,” and “critical path” with explicit numeric thresholds or defined project criteria. Ensure agents can consistently determine when GitNexus impact analysis and each risk level apply.Source: Coding guidelines
30-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse dependency-level labels instead of
WILL BREAK.
d=1means a direct caller/importer, not a confirmed incompatible change. ReplaceWILL BREAKin the checklist, depth table, tool example, andvalidateUserexample with a dependency-only label such asDIRECT DEPENDENCY — VALIDATE FIRSTuntil the change and impacted API contract are checked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md around lines 30 - 37, Replace every dependency-level “WILL BREAK” label in the impact-analysis checklist, depth table, tool example, and validateUser example with “DIRECT DEPENDENCY — VALIDATE FIRST” or equivalent wording. Preserve the meaning that d=1 identifies direct callers/importers requiring validation, not confirmed incompatibility..claude/skills/ship-release/SKILL.md (1)
111-111: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the worktree from the main checkout.
Line 48 changes the working directory to
$RELEASE_WT, and no later step restores it. From that directory,../Rocket.Chat.Electron-worktrees/release-<version>is not the original relative path. Git can also reject removal of the current worktree. Run cleanup from the main checkout and pass the absolute$RELEASE_WTpath.Suggested cleanup setup
+MAIN_REPO=$(pwd) git worktree add ../Rocket.Chat.Electron-worktrees/release-<version> ... ... -git worktree remove ../Rocket.Chat.Electron-worktrees/release-<version> +git -C "$MAIN_REPO" worktree remove "$RELEASE_WT"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/ship-release/SKILL.md at line 111, Update the cleanup step in the release workflow to first return to the main checkout, then remove the worktree using the absolute $RELEASE_WT path. Ensure the removal command is not executed while the shell is inside the worktree, preserving the existing release cleanup flow.
🧹 Nitpick comments (4)
docs/desktop-ui-guidelines.md (2)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required camelCase filename.
docs/desktop-ui-guidelines.mduses hyphenated words. The**/*rule requires camelCase file names. Rename it todocs/desktopUiGuidelines.mdand update all references.As per coding guidelines, files matching
**/*use camelCase names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/desktop-ui-guidelines.md` around lines 1 - 5, Rename the documentation file to desktopUiGuidelines.md to comply with the required camelCase naming convention, and update every reference to the old desktop-ui-guidelines.md path.Source: Coding guidelines
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse objective wording in the introduction.
hard-wonandyou skip a review roundare subjective and make an unmeasured process claim. State that the document records verified lessons from PRs#3441and#3443.As per coding guidelines, use measurable descriptions and straightforward language.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/desktop-ui-guidelines.md` around lines 3 - 5, Update the introduction of the desktop UI guidelines to use objective, straightforward wording. Describe the document as recording verified lessons from PRs `#3441` and `#3443`, and remove the subjective “hard-won” phrasing and the unmeasured review-process claim.Source: Coding guidelines
.claude/skills/electron-bump/SKILL.md (1)
37-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace subjective workflow labels with explicit requirements.
historically,Light path,Full path, andminimaldo not define required actions. State the exact phases and commit-body contents for each upgrade class.As per coding guidelines, avoid subjective descriptors in writing and use measurable descriptions.
Proposed wording
- - **patch / minor within same major** → historically package.json + yarn.lock only, no code changes. Light path. - - **major** → expect API breaks, code adaptation, `@types/node` bump, README/builder updates. Full path. + - **patch / minor within same major** → complete Phases 1–6; limit edits to package.json and yarn.lock unless research identifies a coupled change. + - **major** → complete Phases 1–6 and apply every API, type, configuration, and documentation change listed in the approved plan. ... -3. Commit with conventional message: `chore: update Electron from <CURRENT> to <TARGET>` — match the repo's historical style (see PRs `#3285`, `#3179`; the PR number isn't known at commit time, and squash-merge titles carry it anyway). Body lists breaking-change adaptations for a major bump; minimal for patch/minor. +3. Commit with conventional message: `chore: update Electron from <CURRENT> to <TARGET>`. For a major bump, list each breaking-change adaptation. For a patch/minor bump, state that no breaking-change adaptations were required.Also applies to: 156-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/electron-bump/SKILL.md around lines 37 - 39, Update the upgrade-classification guidance around the patch/minor/major paths to remove subjective terms such as “historically,” “Light path,” “Full path,” and “minimal.” Define explicit required phases and the required commit-body contents for each upgrade class, including which files or validation steps apply to patch, minor, and major upgrades.Source: Coding guidelines
.claude/skills/ship-release/SKILL.md (1)
88-88: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the two-phase signing recovery.
“Usually re-run, not code” does not resolve a KMS CNG signing failure. Document these steps: build packages with signing environment variables empty, sign the built packages with
jsignand Google Cloud KMS, then verify the complete Windows asset matrix before rerunning or publishing.Based on learnings, code signing uses two phases: unsigned package builds followed by
jsignand Google Cloud KMS signing.Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/ship-release/SKILL.md at line 88, Update the failure-recovery guidance in the release workflow to document the two-phase Windows signing process: rebuild packages with signing environment variables empty, sign the built packages using jsign with Google Cloud KMS, then verify the complete Windows asset matrix before rerunning or publishing. Preserve the existing verbatim error and affected-platform reporting guidance, including the KMS CNG/MSI failure context.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.json:
- Line 39: Update the TypeScript checker command in the settings configuration
from npx to the repository-scoped Yarn execution form, using the locked
dependency tree while preserving the --noEmit option.
- Around line 41-45: Remove the orphaned gitnexus and context-mode entries from
the MCP allowlist in .claude/settings.json, since no configured servers provide
them; retain only permissions for MCP servers declared in .mcp.json, including
context7.
- Line 36: Restrict the Bash permission in the permissions.allow configuration
by replacing the broad Bash(yarn test *) pattern with explicit safe test command
forms that cannot accept snapshot-update flags such as --updateSnapshot or -u.
Add a lint-fix permission only if intentionally required, and restore
.gitnexus/run.cjs if the review workflow depends on it.
In @.claude/skills/electron-bump/SKILL.md:
- Around line 36-39: Add an explicit no-op check in the version-bump procedure
before the downgrade guard or jump classification: when TARGET equals CURRENT,
stop and report that no upgrade is needed. Keep downgrade handling and
patch/minor/major classification unchanged for differing versions.
In @.claude/skills/ship-release/SKILL.md:
- Line 83: Update the release-run lookup instructions to select only the exact
refs/tags/<version> headBranch value, rather than relying on --limit 5 alone.
Filter the gh run list JSON using workflowName for build-release.yml, increase
the query limit as needed, and fail when the target ref is missing or has
multiple matching runs.
- Around line 61-69: The release workflow must verify that the requested PR’s
merge commit is exactly the current origin/master before tagging. Update the
steps around gh pr view, the MERGE_SHA assignment, and release:tag to capture
PR_MERGE_SHA after merging, fetch origin/master, compare the two SHAs, and stop
if they differ before checkout or tagging.
In `@AGENTS.md`:
- Around line 187-192: Update the stale-index recovery instruction in AGENTS.md
to use node .gitnexus/run.cjs analyze --index-only, matching the safe reindex
guidance above. Alternatively, explicitly restrict plain analyze to quiet
points, while preserving the warning about worktree and tracked-file mutations.
In `@skills/dev-app-verify/SKILL.md`:
- Line 77: Update the WebSocket open wait around ws.onopen to store the timeout
handle, clear it when ws emits open, and resolve from an onopen callback. On
timeout, close ws before rejecting so the failed inspector connection is cleaned
up.
---
Outside diff comments:
In @.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md:
- Around line 19-26: Update the workflow in the impact-analysis skill to read
gitnexus://repo/{name}/context before invoking impact, processes, or
detect_changes. Check the context for index freshness; if stale, run node
.gitnexus/run.cjs analyze from the repository root and re-read context before
continuing. Preserve the existing risk-assessment step and explicitly require
root commands to run from the repository root.
- Around line 8-16: Update the “When to Use” guidance and the corresponding
section around the additional referenced lines to replace subjective terms such
as “non-trivial,” “few processes,” “many processes,” and “critical path” with
explicit numeric thresholds or defined project criteria. Ensure agents can
consistently determine when GitNexus impact analysis and each risk level apply.
- Around line 30-37: Replace every dependency-level “WILL BREAK” label in the
impact-analysis checklist, depth table, tool example, and validateUser example
with “DIRECT DEPENDENCY — VALIDATE FIRST” or equivalent wording. Preserve the
meaning that d=1 identifies direct callers/importers requiring validation, not
confirmed incompatibility.
In @.claude/skills/ship-release/SKILL.md:
- Line 111: Update the cleanup step in the release workflow to first return to
the main checkout, then remove the worktree using the absolute $RELEASE_WT path.
Ensure the removal command is not executed while the shell is inside the
worktree, preserving the existing release cleanup flow.
In `@AGENTS.md`:
- Around line 212-216: Update the GitNexus usage requirements in AGENTS.md so
impact() and context() targets are disambiguated with a GitNexus UID, file_path,
or supported kind qualifier whenever symbol names are ambiguous. Preserve the
existing upstream impact-analysis, risk-warning, and detect_changes requirements
while ensuring results are not relied on from an unqualified shared symbol name.
---
Nitpick comments:
In @.claude/skills/electron-bump/SKILL.md:
- Around line 37-39: Update the upgrade-classification guidance around the
patch/minor/major paths to remove subjective terms such as “historically,”
“Light path,” “Full path,” and “minimal.” Define explicit required phases and
the required commit-body contents for each upgrade class, including which files
or validation steps apply to patch, minor, and major upgrades.
In @.claude/skills/ship-release/SKILL.md:
- Line 88: Update the failure-recovery guidance in the release workflow to
document the two-phase Windows signing process: rebuild packages with signing
environment variables empty, sign the built packages using jsign with Google
Cloud KMS, then verify the complete Windows asset matrix before rerunning or
publishing. Preserve the existing verbatim error and affected-platform reporting
guidance, including the KMS CNG/MSI failure context.
In `@docs/desktop-ui-guidelines.md`:
- Around line 1-5: Rename the documentation file to desktopUiGuidelines.md to
comply with the required camelCase naming convention, and update every reference
to the old desktop-ui-guidelines.md path.
- Around line 3-5: Update the introduction of the desktop UI guidelines to use
objective, straightforward wording. Describe the document as recording verified
lessons from PRs `#3441` and `#3443`, and remove the subjective “hard-won” phrasing
and the unmeasured review-process claim.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f251ee0-e69f-49f2-89bd-94a19325dd71
📒 Files selected for processing (11)
.claude/settings.json.claude/skills/electron-bump/SKILL.md.claude/skills/gitnexus/gitnexus-debugging/SKILL.md.claude/skills/gitnexus/gitnexus-exploring/SKILL.md.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md.claude/skills/gitnexus/gitnexus-refactoring/SKILL.md.claude/skills/ship-release/SKILL.mdAGENTS.mdCLAUDE.mddocs/desktop-ui-guidelines.mdskills/dev-app-verify/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .claude/skills/gitnexus/gitnexus-refactoring/SKILL.md
- .claude/skills/gitnexus/gitnexus-exploring/SKILL.md
- CLAUDE.md
- .claude/skills/gitnexus/gitnexus-debugging/SKILL.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use camelCase for file names and PascalCase for component names.
Avoid subjective descriptors in writing, use measurable descriptions, never invent metrics, and write PR descriptions in straightforward language focused on what changed and why.
Files:
docs/desktop-ui-guidelines.mdAGENTS.mdskills/dev-app-verify/SKILL.md
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
Treat
AGENTS.mdas the canonical project guide for shared project guidance, conventions, and memory notes; route such content there rather than appending it toCLAUDE.md, even when the request namesCLAUDE.md.
Files:
AGENTS.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Run root commands from the repository root; do not run `yarn build` inside workspace directories.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Windows builds must include `x64`, `ia32`, and `arm64` architectures.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Perform code signing in two phases: build packages without signing using empty environment variables, then sign built packages with `jsign` and Google Cloud KMS.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Prefer editing existing files over creating new abstractions unless a new abstraction removes real complexity or matches an existing pattern.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Never commit or push without explicit user permission; never commit directly to `master` or `dev`; show what will be committed before committing.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Before editing any function, class, or method, run GitNexus upstream impact analysis, report the blast radius, and warn the user if the risk is HIGH or CRITICAL.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Before committing, run GitNexus `detect_changes()` to verify that changes affect only expected symbols and execution flows.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Do not rename symbols with find-and-replace; use GitNexus `rename`, which understands the call graph.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Verify work with the narrowest meaningful checks first, then broader checks when risk or shared behavior justifies it; do not mark a task complete without proving it works.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron
Timestamp: 2026-08-10T12:25:14.297Z
Learning: Screen-capture, WebRTC, and portal behavior must be validated on hardware GL; prefer D-Bus-level portal assertions over dialog visibility.
🪛 SkillSpector (2.5.1)
.claude/skills/electron-bump/SKILL.md
[warning] 40: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 143: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 162: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 40: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 143: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 162: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 40: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 143: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 162: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 40: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 143: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 162: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 40: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 143: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 162: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
🔇 Additional comments (11)
AGENTS.md (1)
154-186: LGTM!Also applies to: 196-202, 225-245
docs/desktop-ui-guidelines.md (2)
106-107: 📐 Maintainability & Code QualityMake the jsdom guidance version-specific.
getComputedStyle()length conversion is version-dependent. jsdom v30 release notes describe a fix that converts length values to pixels, so this statement must match the repository's pinned jsdom version. (github.com)Verify the declared version and either state the supported version or document the behavior observed in that test environment.
7-36: LGTM!Also applies to: 40-54, 56-86, 88-105, 109-125
skills/dev-app-verify/SKILL.md (1)
12-44: LGTM!Also applies to: 46-76, 78-119, 121-145
.claude/skills/electron-bump/SKILL.md (1)
12-12: LGTM!Also applies to: 86-86, 134-134
.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md (2)
6-6: 📐 Maintainability & Code QualityConfirm the required skill filename.
The file basename is
SKILL.md, which does not follow thecamelCasefilename rule. Confirm that the Claude skill loader requires this exact basename. If it does, document the exception inAGENTS.md; otherwise rename the file without breaking skill discovery.As per coding guidelines, use camelCase for file names.
Source: Coding guidelines
60-66: 🎯 Functional CorrectnessMake
detect_changesscope and tool arguments explicit.The workflow calls
detect_changes()without a scope, but the example callsdetect_changes({scope: "staged"}). The skill also assumes thatminConfidenceandmaxDepthare accepted byimpact. Confirm these names and defaults against the installed GitNexus interface. Then specify whether each workflow analyzes the working tree or the staged commit, so the pre-commit check examines the intended change set.Also applies to: 76-84
.claude/skills/ship-release/SKILL.md (4)
30-32: Select the stable baseline with executable channel rules.This remains the previous baseline issue.
git tag --sort=-v:refname | head -5lists mixed channels, and<last-tag>is still not assigned by an executable step. Reuse the channel-aware logic fromscripts/release-tag.tsto set one verified stable baseline before collecting commits.
72-82: Keep the manual fallback equivalent toscripts/release-tag.ts.The fail-closed exit and merge-SHA check now exist. The fallback still omits the channel-aware version guard, and the
package.jsonversion requirement at Line 68 is only a comment. Assert that the package version equals TARGET and that TARGET passes the same in-channel ordering rules before pushing the tag.
18-24: LGTM!Also applies to: 35-40, 44-55
95-101: 🗄️ Data Integrity & IntegrationNo change needed for the Windows asset matrix.
The release workflow uses
workspaces/desktop-release-action, and that action builds NSIS, MSI, and AppX packages forx64,ia32, andarm64during tag/sign/publish. Thepackage.jsonreleasescript is unused for workflow releases.> Likely an incorrect or invalid review comment.
| }, | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(yarn test *)", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if [ -f .gitnexus/run.cjs ]; then
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline .gitnexus/run.cjs --items all
fi
rg -n -C 3 -- 'process\.argv|analyze|clean|wiki|uninstall|gist' .gitnexus/run.cjs
fi
rg -n -C 3 -- '"test"|"lint"|updateSnapshot|--fix' package.jsonRepository: RocketChat/Rocket.Chat.Electron
Length of output: 1141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(\.claude/settings\.json|package\.json|\.gitnexus/run\.cjs)$|(^|/)package\.json$'
echo "== .claude/settings.json outline/section =="
if [ -f .claude/settings.json ]; then
wc -l .claude/settings.json
sed -n '1,90p' .claude/settings.json | cat -n
fi
echo "== package.json scripts =="
if [ -f package.json ]; then
python3 - <<'PY'
import json
with open('package.json') as f:
p=json.load(f)
for k in ['scripts','dependencies','devDependencies']:
print(k, 'present=', bool(p.get(k)))
if p.get('scripts'):
for k,v in p['scripts'].items():
if k in {'test','test:coverage','lint','lint-fix','.lint:eslint','.lint:tsc'} or any(tok in v for tok in ['jest','eslint','tsc','xvfb-maybe','eslint --fix','--noEmit']):
print(k, "=>", v)
PY
fi
echo "== .gitnexus/run.cjs relevant =="
if [ -f .gitnexus/run.cjs ]; then
wc -l .gitnexus/run.cjs
rg -n -C 4 'process\.argv|switch|case|analyze|clean|wiki|uninstall|gist|install|execute|run|chmod|write|unlink|rm ' .gitnexus/run.cjs
fi
echo "== installed jest eslint availability =="
node - e 2>/dev/null || true
command -v yarn npm npx >&2 || true
echo "== deterministic argv pattern checks =="
python3 - <<'PY'
from pathlib import Path
p=Path('.claude/settings.json')
if p.exists():
s=p.read_text()
for line in s.splitlines():
if 'Bash(yarn test *)' in line or 'Bash(yarn lint *)' in line or 'Bash(node .gitnexus/run.cjs *)' in line:
print(line.strip())
PYRepository: RocketChat/Rocket.Chat.Electron
Length of output: 2317
🌐 Web query:
Claude Code .claude/settings.json permissions allow Bash(yarn test *) wildcard behavior allow without approval
💡 Result:
Yes, you can configure Claude Code to execute Bash commands matching a wildcard pattern—such as those starting with "yarn test"—without manual approval by adding an allow rule to your.claude/settings.json file [1][2]. To allow these commands, add the following entry to the permissions.allow array in your settings.json file: { "permissions": { "allow": [ "Bash(yarn test *)" ] } } Key details regarding this behavior: Wildcard Matching: The * wildcard matches any sequence of characters [1][3]. When used in a pattern like "Bash(yarn test *)", it allows any Bash command that begins with "yarn test " (including the required space for a word boundary) [1][4]. Word Boundaries: If you include a space before the asterisk (e.g., "Bash(yarn test )"), it enforces a word boundary, meaning it will match "yarn test" and its subcommands, but not unintended commands that happen to start with the same characters (like "yarn test-all" unless matched correctly) [1][4]. If you omit the space (e.g., "Bash(yarn test)"), it will perform a broader prefix match that includes commands like "yarn test-all" [1][4]. Workspace Trust: Because.claude/settings.json is typically committed to a repository, Claude Code requires you to accept a workspace trust dialog before these rules are applied [1][5]. If you want to bypass this for personal, non-shared settings, you can use ~/.claude/settings.json or.claude/settings.local.json (which is git-ignored) [6][7]. Deny Overrides: Note that if a command is explicitly denied in any settings file (including managed/enterprise settings), that deny rule will always take precedence over any allow rule, regardless of where the rules are defined [6][4]. For more precise control, you can also add these rules interactively by running the /permissions command in your Claude Code session [1][2]. Choosing the "don't ask again" option when a prompt appears will automatically generate and save the corresponding rule to your configuration [3][7].
Citations:
- 1: https://code.claude.com/docs/en/permissions
- 2: https://agentscamp.com/guides/configuration/claude-code-settings-permissions
- 3: https://code.claude.com/docs/en/permissions.md
- 4: https://allahabadi.dev/blogs/ai/claude-code-permissions-settings-explained/
- 5: https://claude-code.mintlify.app/en/permissions
- 6: https://arte.itlibra.com/en/articles/claude-code-permission-rules-settings
- 7: https://code.claude.com/docs/en/settings?_rsc=crtel
🌐 Web query:
Jest CLI --updateSnapshot -u write snapshot documentation
💡 Result:
In Jest, the --updateSnapshot flag (or its alias -u) is the primary command-line tool used to re-record existing snapshots that fail during a test run [1][2][3]. When a snapshot test fails, it indicates that the rendered output or data has changed [3][4]. If this change is intentional, you can update the snapshot by running Jest with this flag: jest --updateSnapshot # or using the short alias jest -u Key behaviors and best practices include: Scope: The --updateSnapshot flag can be combined with other CLI options, such as specific test file paths or --testNamePattern, to re-record only the snapshots for specific tests or suites rather than the entire project [1][3]. Watch Mode: If running Jest in watch mode (jest --watch), you do not need to restart with the flag. You can press u to update all failing snapshots interactively, or press i to step through failing snapshots one at a time and decide whether to update them individually [3][4]. CI Environments: Jest does not automatically write or update snapshots when running in a CI (Continuous Integration) environment [2]. If new snapshots are generated or existing ones do not match during a CI run, the tests will fail, ensuring that you are aware of unexpected changes before they are committed [2][5]. Best Practice: Always review the diff provided in your terminal before running the update command [3][4]. Because snapshots serve as a baseline for your tests, reflexively updating them without verifying that the changes are intentional can mask regressions [3][4]. Once updated, you should commit the modified.snap files to version control [2][4].
Citations:
- 1: https://jestjs.io/docs/cli
- 2: https://jestjs.io/docs/snapshot-testing
- 3: https://qaskills.sh/blog/jest-snapshot-testing-guide-2026
- 4: https://www.browserstack.com/guide/snapshot-testing
- 5: [Bug]: Jest attempts to write snapshot file in CI mode if obsolete jestjs/jest#13645
Restrict permissions to specific test and lint commands.
Bash(yarn test *) allows any argument after yarn test, including --updateSnapshot or -u, which rewrites Jest snapshots. Replace it with a command form that cannot receive snapshot-update flags.
permissions.allow contains Bash(yarn lint *); add a lint-fix entry only if intended. Add .gitnexus/run.cjs back to the repository if this command is required for review context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/settings.json at line 36, Restrict the Bash permission in the
permissions.allow configuration by replacing the broad Bash(yarn test *) pattern
with explicit safe test command forms that cannot accept snapshot-update flags
such as --updateSnapshot or -u. Add a lint-fix permission only if intentionally
required, and restore .gitnexus/run.cjs if the review workflow depends on it.
| "Bash(yarn test *)", | ||
| "Bash(yarn lint)", | ||
| "Bash(yarn lint *)", | ||
| "Bash(npx tsc --noEmit)", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd --hidden --type f \
--glob 'package.json' \
--glob 'yarn.lock' \
--glob 'package-lock.json' \
--glob 'pnpm-lock.yaml' . -0 |
xargs -0 -r rg -n -C 2 -- '"typescript"|"tsc"'Repository: RocketChat/Rocket.Chat.Electron
Length of output: 940
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Root package manager files:\n'
git ls-files | rg '(^|/)(package.json|yarn.lock|package-lock.json|pnpm-lock.yaml)$' | sort
printf '\nRoot package.json relevant fields:\n'
python3 - <<'PY'
import json
with open('package.json') as f:
p=json.load(f)
fields=['packageManager','scripts','devDependencies']
for k in fields:
if k in p:
print(k, p[k])
PY
printf '\nLockfiles mentioning typescript/tsc:\n'
for f in $(git ls-files | rg '(^|/)(yarn.lock|package-lock.json|pnpm-lock.yaml)$' | sort); do
echo "--- $f ---"
rg -n -C 2 '(^|\s)typescript@|(^|\s)typescript:|(^|\s)tsc@|"typescript"|tsc' "$f" | head -n 80 || true
done
printf '\n.claude/settings.json:\n'
cat -n .claude/settings.json | sed -n '1,90p'Repository: RocketChat/Rocket.Chat.Electron
Length of output: 8751
Run the locked TypeScript checker from Yarn.
npx tsc --noEmit bypasses the repository package manager lock for the npx lookup step. Use the repository-scoped tsc --noEmit command path, or a repository-executor form, so the locked Yarn dependency tree is used.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/settings.json at line 39, Update the TypeScript checker command in
the settings configuration from npx to the repository-scoped Yarn execution
form, using the locked dependency tree while preserving the --noEmit option.
| "mcp__gitnexus__detect_changes", | ||
| "mcp__gitnexus__impact", | ||
| "mcp__gitnexus__query", | ||
| "mcp__gitnexus__context", | ||
| "mcp__plugin_context-mode_context-mode__ctx_search" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd --hidden --type f \
--glob '.mcp.json' \
--glob '*.json' \
--glob '*.jsonc' . -0 |
xargs -0 -r rg -n -C 2 \
'gitnexus|context-mode|detect_changes|ctx_search'Repository: RocketChat/Rocket.Chat.Electron
Length of output: 280
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Tracked config-like files:\n'
git ls-files \
'.claude/settings.json' \
'.mcp.json' \
'.mcp.d' \
'mcp*.json' \
'mcp*.jsonc' 2>/dev/null || true
printf '\nJSON files referencing MCP identifiers:\n'
git ls-files '*.json' '*.jsonc' '.mcp.json' | while read -r file; do
if rg -q 'gitnexus|context-mode|detect_changes|ctx_search|permission' "$file"; then
echo "--- $file"
rg -n -C 2 'gitnexus|context-mode|detect_changes|ctx_search|permission' "$file" || true
fi
doneRepository: RocketChat/Rocket.Chat.Electron
Length of output: 13887
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'MCP/permission settings files:\n'
git ls-files . | grep -E '(?i)^(\.claude/settings\.json|(^|/)(\.mcp|\.mcp\.d|.*mcp.*|settings.*|permissions.*|.*\.(json|jsonc))$)' | sed -n '1,120p'
printf '\nReferences to MCP names and related tools:\n'
for file in $(git ls-files | grep -E '\.(json|jsonc)$|(^|/)\.claude/settings\.json$|(^|/)\.mcp.*$'); do
if rg -q 'gitnexus|context-mode|detect_changes|impact|query|context|ctx_search|permission|allow|mcp__' "$file"; then
echo "--- $file"
rg -n -C 2 'gitnexus|context-mode|detect_changes|impact|query|context|ctx_search|permission|allow|mcp__' "$file"
fi
doneRepository: RocketChat/Rocket.Chat.Electron
Length of output: 240
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'MCP files:\n'
git ls-files .mcp.json | xargs -r cat -n
printf '\nRelevant .claude/settings.json permissions section:\n'
awk 'NR==30,NR==48 {printf "%5d %s\n", NR, $0}' .claude/settings.jsonRepository: RocketChat/Rocket.Chat.Electron
Length of output: 969
Remove the orphan MCP allow entries.
.mcp.json only declares context7; these allowlist entries target gitnexus and context-mode tools that no configured MCP server can provide. Add the missing MCP server definitions with matching canonical names, or remove these entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/settings.json around lines 41 - 45, Remove the orphaned gitnexus and
context-mode entries from the MCP allowlist in .claude/settings.json, since no
configured servers provide them; retain only permissions for MCP servers
declared in .mcp.json, including context7.
| 3. **Downgrade guard**: compare TARGET vs CURRENT semver. If TARGET < CURRENT, STOP and ask for explicit confirmation that a downgrade is intended before proceeding. | ||
| 4. Classify the jump: **patch** (z), **minor** (y), or **major** (x). This drives effort: | ||
| - **patch / minor within same major** → historically package.json + yarn.lock only, no code changes. Light path. | ||
| - **major** → expect API breaks, code adaptation, @types/node bump, README/builder updates. Full path. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stop when TARGET equals CURRENT.
TARGET == CURRENT passes the downgrade guard, but the next rule defines only patch, minor, and major jumps. An omitted version can resolve to the installed version. Add an explicit no-op stop before classification to prevent an empty upgrade PR.
Proposed fix
-3. **Downgrade guard**: compare TARGET vs CURRENT semver. If TARGET < CURRENT, STOP and ask for explicit confirmation that a downgrade is intended before proceeding.
+3. Compare TARGET and CURRENT as semver values. If TARGET == CURRENT, STOP and report that no upgrade is required. If TARGET < CURRENT, STOP and ask for explicit confirmation that a downgrade is intended before proceeding.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 3. **Downgrade guard**: compare TARGET vs CURRENT semver. If TARGET < CURRENT, STOP and ask for explicit confirmation that a downgrade is intended before proceeding. | |
| 4. Classify the jump: **patch** (z), **minor** (y), or **major** (x). This drives effort: | |
| - **patch / minor within same major** → historically package.json + yarn.lock only, no code changes. Light path. | |
| - **major** → expect API breaks, code adaptation, @types/node bump, README/builder updates. Full path. | |
| 3. Compare TARGET and CURRENT as semver values. If TARGET == CURRENT, STOP and report that no upgrade is required. If TARGET < CURRENT, STOP and ask for explicit confirmation that a downgrade is intended before proceeding. | |
| 4. Classify the jump: **patch** (z), **minor** (y), or **major** (x). This drives effort: | |
| - **patch / minor within same major** → historically package.json + yarn.lock only, no code changes. Light path. | |
| - **major** → expect API breaks, code adaptation, `@types/node` bump, README/builder updates. Full path. |
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 40: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 143: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 162: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/electron-bump/SKILL.md around lines 36 - 39, Add an explicit
no-op check in the version-bump procedure before the downgrade guard or jump
classification: when TARGET equals CURRENT, stop and report that no upgrade is
needed. Keep downgrade handling and patch/minor/major classification unchanged
for differing versions.
| 1. Squash-merge: `gh pr merge <PR> --squash`. | ||
| 2. `git -C "$RELEASE_WT" fetch origin master` and confirm the merge commit is HEAD of `origin/master` and its `package.json` has TARGET. | ||
| 3. **GATE: confirm with the user before pushing the tag** (tag push = build + release creation; deleting a tag after builds start is messy). | ||
| 4. Move the release worktree HEAD onto the master merge commit, then tag via the repo script. The release worktree is still on `chore/release-<version>` (the pre-merge bump commit) — tagging there ships the wrong tree. Detach onto the squashed merge commit first (its `package.json` version must equal TARGET): | ||
| ```sh | ||
| MERGE_SHA=$(git -C "$RELEASE_WT" rev-parse origin/master) | ||
| git -C "$RELEASE_WT" checkout "$MERGE_SHA" # detached HEAD at the merge commit | ||
| node -p "require('$RELEASE_WT/package.json').version" # MUST print TARGET | ||
| (cd "$RELEASE_WT" && yarn release:tag) # reads package.json version, guards, tags HEAD, pushes |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F '.claude/skills/ship-release/SKILL.md' || true
echo "== relevant section =="
if [ -f .claude/skills/ship-release/SKILL.md ]; then
nl -ba .claude/skills/ship-release/SKILL.md | sed -n '1,140p'
fi
echo "== search PR merge references =="
rg -n --max-count=200 'mergeCommit|gh pr merge|rev-parse origin/master|checkout.*origin/master|yarn release:tag|package\.json' .claude/skills/ship-release/SKILL.md .github 2>/dev/null || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status diff stat =="
git status --short
git diff --stat -- .claude/skills/ship-release/SKILL.md || true
echo "== relevant section with line numbers (sed) =="
sed -n '1,140p' .claude/skills/ship-release/SKILL.md | awk '{printf "%6d\t%s\n", NR, $0}'
echo "== search PR merge references =="
rg -n --max-count=200 'mergeCommit|gh pr merge|rev-parse origin/master|checkout.*origin/master|yarn release:tag|package\.json|TARGET|PR' .claude/skills/ship-release/SKILL.md .github 2>/dev/null || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 11268
🌐 Web query:
GitHub CLI gh pr view mergeCommit oid JSON field
💡 Result:
To retrieve the merge commit OID (SHA) for a pull request using the GitHub CLI, you can use the gh pr view command with the --json flag. The mergeCommit field is a nested object that contains an oid field. You can access it by requesting the mergeCommit field [1][2]. Example command: gh pr view --json mergeCommit This will return JSON output in the following structure: { "mergeCommit": { "oid": "YOUR_COMMIT_SHA" } } If you want to extract only the OID directly, you can combine this with the --jq flag [3][4]: gh pr view --json mergeCommit --jq.mergeCommit.oid Note that mergeCommit will be null if the pull request has not been merged [3]. Sources: [1] https://qiita.com/neko_the_shadow/items/ae1f83cf20532d48e520 [2] cli/cli#2306 [3] cli/cli#10746 [4] https://cli.github.com/manual/gh_help_formatting
Citations:
- 1: https://qiita.com/neko_the_shadow/items/ae1f83cf20532d48e520
- 2: Output merge commit hash for Merged PR's in
gh pr viewcli/cli#2306 - 3: Output commit SHA when
gh pr mergeis successful cli/cli#10746 - 4: https://cli.github.com/manual/gh_help_formatting
🌐 Web query:
GitHub CLI gh pr merge squash creates merge commit mergeCommit oid
💡 Result:
When you use the gh pr merge --squash command, GitHub performs a "squash and merge" operation [1][2]. This process does not create a traditional merge commit [3][4]. Instead, it combines all commits from the pull request branch into a single new commit that is applied to the base branch [3][4]. Because this resulting commit is applied as a fast-forward operation, there is no explicit merge commit (which would have two parent commits) created in the repository's history [3][4]. Regarding the "mergeCommit OID": The GitHub API, which the GitHub CLI (gh) utilizes under the hood [5], performs the squash and merge via a GraphQL mutation (typically pullRequestMerge) [5]. When the squash strategy is selected, the API returns a response that includes details about the merged commit, often referred to as the merge commit in the context of the API's generic response structure, even though it is technically a single squashed commit rather than a merge commit [5]. If you are interacting with the GitHub API or CLI and looking for the identifier of the final commit created on the base branch after a squash merge, it is returned as the OID (Object ID) of that single resulting commit [5].
Citations:
- 1: https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/merging-a-pull-request
- 2: https://cli.github.com/manual/gh_pr_merge
- 3: https://docs.github.com/en/pull-requests/reference/pull-request-merges
- 4: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges
- 5: https://github.com/cli/cli/blob/trunk/pkg/cmd/pr/merge/http.go
🌐 Web query:
GitHub GraphQL MergeInfo squashMergeCommitoid mergeCommit oid fields
💡 Result:
In the GitHub GraphQL API, there is no field specifically named squashMergeCommitoid or similar [1][2]. The OID (Object ID) of the merge commit created by any merge method—including squash, merge, and rebase—is accessed through the mergeCommit field on the PullRequest object [1][3][4]. To retrieve the OID of the commit generated by a merge, you should query the mergeCommit field and request the oid sub-field [5][6][4]. Example GraphQL query: { repository(owner: "OWNER", name: "REPO") { pullRequest(number: 1) { mergeCommit { oid } } } } Key details: - mergeCommit: This field returns a Commit object representing the commit created when the pull request was merged [1][3]. It will return null if the pull request has not been merged or if the merge method used (such as a fast-forward merge) did not result in a new merge commit [3][7]. - OID vs. SHA: The oid field on the Commit object returns the full Git Object ID (SHA) of that commit [8]. - Squash Merges: When a pull request is merged using the "Squash and merge" method, the resulting single commit is still considered the merge commit for that pull request and is accessible via the mergeCommit field [1][5]. If you are migrating from the REST API, note that the REST API's merge_commit_sha is equivalent to the GraphQL API's mergeCommit { oid } [5][7].
Citations:
- 1: https://docs.github.com/en/graphql/reference/pulls
- 2: https://docs.github.com/en/enterprise-cloud@latest/graphql/reference/pulls
- 3: https://docs.github.com/enterprise/2.10/developer/v4/object/pullrequest
- 4: https://github.com/eamonnsullivan/github-api-lib/blob/main/resources/graphql/merge-pull-request-mutation.graphql
- 5: https://gist.github.com/bukzor/b40a364f2c0c545b39c32b06a98d6d97
- 6: https://github.com/googleapis/release-please/blob/main/src/github-api.ts
- 7: https://stackoverflow.com/questions/74322757/github-rest-and-graphql-api-are-returning-different-data
- 8: https://docs.github.com/en/graphql/reference/git
🌐 Web query:
GitHub CLI gh pr merge --squash output merge commit SHA --verify-commits --dry-run
💡 Result:
The GitHub CLI (gh) command gh pr merge does not natively output the resulting merge commit SHA upon successful execution [1]. While the command supports various flags such as --squash, --verify-commits (which is not a standard flag for this command, as verification is typically handled by repository settings), and --dry-run (which is also not a standard flag for this command), it does not provide a mechanism to return the commit SHA directly in the console output after merging [2][3][4][1]. To retrieve the merge commit SHA after a pull request has been merged, you can use the following command after the merge operation is complete: gh pr view --json mergeCommit --jq.mergeCommit.oid Regarding the flags mentioned: --squash: This is a valid flag used to squash all commits from the PR into a single commit before merging [2][4]. --verify-commits: This is not a documented or supported flag for gh pr merge [2][3][4]. Commit signature verification is managed via GitHub repository settings and branch protection rules. --dry-run: This is not a documented or supported flag for gh pr merge [2][3][4]. If you are looking to automate a workflow that requires the merge commit SHA, querying the PR information after the merge using the command provided above is the recommended approach [1][5].
Citations:
- 1: Output commit SHA when
gh pr mergeis successful cli/cli#10746 - 2: https://cli.github.com/manual/gh_pr_merge
- 3: https://man.archlinux.org/man/gh-pr-merge.1.en
- 4: https://linuxcommandlibrary.com/man/gh-pr-merge
- 5: Output merge commit hash for Merged PR's in
gh pr viewcli/cli#2306
Compare the PR merge commit with origin/master before tagging.
MERGE_SHA=$(git -C "$RELEASE_WT" rev-parse origin/master) only records the current remote head. A later PR push on master can land after gh pr merge <PR> --squash, or the requested PR is not the one at HEAD. Get PR_MERGE_SHA=$(gh pr view <PR> --json mergeCommit --jq '.mergeCommit.oid') after the merge, fetch origin master, then test "$PR_MERGE_SHA" = "$MERGE_SHA" before detaching and tagging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/ship-release/SKILL.md around lines 61 - 69, The release
workflow must verify that the requested PR’s merge commit is exactly the current
origin/master before tagging. Update the steps around gh pr view, the MERGE_SHA
assignment, and release:tag to capture PR_MERGE_SHA after merging, fetch
origin/master, compare the two SHAs, and stop if they differ before checkout or
tagging.
| test "$(git rev-list -1 <version>)" = "$MERGE_SHA" || { echo "tag does not point at merge SHA — abort" >&2; exit 1; } | ||
| git push origin refs/tags/<version> | ||
| ``` | ||
| 5. Note: the master push (bump merge) also triggers `build-release.yml` — that run is a master build, NOT the release run. The release run is the one with `head_branch == <version>` (the tag ref). Find it: `gh run list --workflow=build-release.yml --limit 5 --json databaseId,headBranch,status`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F '.claude/skills/ship-release/SKILL.md' || true
echo "== relevant instructions =="
if [ -f .claude/skills/ship-release/SKILL.md ]; then
wc -l .claude/skills/ship-release/SKILL.md
sed -n '1,140p' .claude/skills/ship-release/SKILL.md
fi
echo "== workflow files =="
git ls-files '.github/workflows/*'
for f in .github/workflows/*; do
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
doneRepository: RocketChat/Rocket.Chat.Electron
Length of output: 28323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== GitHub CLI help for run list fields =="
if command -v gh >/dev/null 2>&1; then
gh run list --help | awk '/-h, --help:|JSON fields:/,/(?/|exit)/'
else
echo "gh not installed"
fi
echo "== read-only behavioral probe for suggested filter =="
python3 - <<'PY'
import json
data = json.loads('''
[
{"databaseId":1,"headBranch":"ref/tags/4.15.1","status":"completed","conclusion":"success"},
{"databaseId":2,"headBranch":"master","status":"completed","conclusion":"success"},
{"databaseId":3,"headBranch":"refs/tags/4.14.0","status":"completed","conclusion":"success"},
{"databaseId":4,"headBranch":"","status":"completed","conclusion":"success"}
]
''')
target = "refs/tags/4.15.1"
matches = [r for r in data if r["headBranch"] == target]
print(json.dumps(matches))
print("count", len(matches))
print("selected", matches[0]["databaseId"] if len(matches) == 1 else None)
PYRepository: RocketChat/Rocket.Chat.Electron
Length of output: 260
🌐 Web query:
GitHub CLI gh run list JSON fields headBranch workflow databaseId
💡 Result:
The GitHub CLI command gh run list supports the --json flag, which allows you to output specific fields from the workflow run data [1][2]. Both headBranch and databaseId are valid, supported fields for this command [1][3][2]. The complete list of fields currently available for the --json flag in gh run list is: attempt, conclusion, createdAt, databaseId, displayTitle, event, headBranch, headSha, name, number, startedAt, status, updatedAt, url, workflowDatabaseId, workflowName [1][2]. Regarding your specific query about "workflow": note that there is no field named exactly "workflow" in the output JSON. Instead, you should use workflowName to retrieve the name of the workflow or workflowDatabaseId to retrieve its unique database identifier [1][2]. You can use these fields by passing them as a comma-separated list to the --json flag, for example: gh run list --json databaseId,headBranch,workflowName,workflowDatabaseId
Citations:
- 1: https://cli.github.com/manual/gh_run_list
- 2: https://man.archlinux.org/man/extra/github-cli/gh-run-list.1.en
- 3: https://github.com/cli/cli/blob/trunk/pkg/cmd/run/shared/shared.go
Select the release run by exact ref.
The workflow runs on master, dev, and all tags, so --limit 5 can pick a master/dev run or omit the target ref run entirely. Filter the JSON output for the exact refs/tags/<version> headBranch value, fail on missing or ambiguous results, increase --limit if needed, and use workflowName instead of an unsupported workflow field when identifying build-release.yml.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/ship-release/SKILL.md at line 83, Update the release-run
lookup instructions to select only the exact refs/tags/<version> headBranch
value, rather than relying on --limit 5 alone. Filter the gh run list JSON using
workflowName for build-release.yml, increase the query limit as needed, and fail
when the target ref is missing or has multiple matching runs.
| - Reindex GitNexus with `node .gitnexus/run.cjs analyze --index-only` and | ||
| only at quiet points: a plain `analyze` rewrites the gitnexus blocks in | ||
| `AGENTS.md`/`CLAUDE.md` (stats churn in tracked files), and a background | ||
| analyze mutates worktree git state — it can silently drop freshly staged | ||
| files from the index and touch watched sources, restarting a running | ||
| `yarn start` app mid-verification. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use one safe GitNexus reindex command.
These lines require --index-only because plain analyze can rewrite tracked guidance, mutate worktree state, and restart yarn start. Line 208 still instructs agents to run plain analyze when the index is stale. Use the same --index-only command for stale-index recovery, or state that plain analyze is allowed only at a quiet point.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 187 - 192, Update the stale-index recovery
instruction in AGENTS.md to use node .gitnexus/run.cjs analyze --index-only,
matching the safe reindex guidance above. Alternatively, explicitly restrict
plain analyze to quiet points, while preserving the warning about worktree and
tracked-file mutations.
| pending.set(i, { resolve: (m) => { clearTimeout(timer); resolve(m); }, reject }); | ||
| ws.send(JSON.stringify({ id: i, method, params })); | ||
| }); | ||
| await new Promise((res, rej) => { ws.onopen = res; setTimeout(rej, 5000); }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)SKILL\.md$|package\.json$|pnpm-lock\.yaml$|bun\.lockb$|bun\.lock$' || true
echo
echo "Target excerpt:"
if [ -f skills/dev-app-verify/SKILL.md ]; then
nl -ba skills/dev-app-verify/SKILL.md | sed -n '60,90p'
fi
echo
echo "Nearby WebSocket usages in skill repo:"
rg -n "WebSocket|ws\.onopen|setTimeout|ws\.close|Inspector" . -g 'SKILL.md' || true
echo
echo "Runtime/package declarations:"
for f in package.json skills/package.json; do
if [ -f "$f" ]; then
echo "--- $f"
cat "$f"
fi
doneRepository: RocketChat/Rocket.Chat.Electron
Length of output: 975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target excerpt:"
awk '{printf "%6d\t%s\n", NR, $0}' skills/dev-app-verify/SKILL.md | sed -n '60,100p'
echo
echo "WebSocket usage in target skill:"
awk '{printf "%6d\t%s\n", NR, $0}' skills/dev-app-verify/SKILL.md | rg -n "WebSocket|ws\.onopen|setTimeout|ws\.close|Inspector|Inspector" || true
echo
echo "Runtime/package declarations:"
for f in package.json skills/package.json; do
if [ -f "$f" ]; then
echo "--- $f"
cat "$f"
fi
done
echo
echo "Runtime availability:"
(command -v node && node -v) || true
(command -v bun && bun --version) || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 9588
🌐 Web query:
Bun WebSocket close error events unhandled rejection timer lifecycle documentation
💡 Result:
In Bun, WebSocket lifecycle management and error handling are designed to align with web standards while providing server-side performance features [1][2]. Error and Rejection Handling Errors in WebSocket callbacks (such as open, message, or close) that are not caught internally will typically trigger Bun's uncaught exception mechanism [3]. If a handler returns a rejected promise or throws an exception, Bun reports it. Historically, certain failures (like transport resets or protocol violations) might have resulted in silent closure, but recent updates ensure that the error event is fired for non-clean closures before the close event [4]. If errors are unhandled, they may surface via process-level listeners (e.g., process.on("uncaughtException")) or print to stderr, depending on your configuration [3][5]. WebSocket Lifecycle and Timeouts The server-side WebSocket lifecycle is managed via the WebSocketHandler interface in Bun.serve [1][6]: 1. open: Called when the connection is successfully established [1][7]. 2. message: Triggered upon receiving data [1]. 3. close: Invoked when the connection terminates [1][2]. To perform a graceful closure, use the close method; for an abrupt disconnection, use terminate [8][2][7]. 4. error: A dedicated handler for WebSocket-specific errors [1]. Bun automatically manages connection timeouts via the idleTimeout configuration (default 120 seconds), which closes connections inactive for that duration [1][9]. Additionally, the maxPayloadLength setting (default 16 MB) enforces limits on incoming message sizes, triggering a closure if exceeded [1][9]. Recent Behavior Updates Bun has refined its WebSocket event dispatching to ensure better compliance with the HTML specification [10]: - Event Dispatching: The close event is fired asynchronously after the close method returns, preventing race conditions where promise resolvers assigned after the close call might be missed [10]. - Error Signaling: Post-establishment failures now correctly trigger the error event before the close event, ensuring that telemetry or reconnection logic can reliably intercept these failures [4]. Always configure your WebSocket handler with an error callback to ensure robust handling of connection-level issues [1].
Citations:
- 1: https://bun.com/docs/runtime/http/websockets
- 2: https://bun.sh/reference/bun/ServerWebSocket
- 3: Bun.serve: report handler errors thrown after server.upgrade() oven-sh/bun#33525
- 4: websocket: fire error event before close for post-establishment failures oven-sh/bun#33991
- 5: Support
--unhandled-rejectionsflag andrejectionHandledevent oven-sh/bun#19874 - 6: https://bun.sh/reference/bun/WebSocketHandler
- 7: https://bun.com/reference/bun/WebSocketHandler/open
- 8: https://bun.sh/reference/bun/WebSocketHandler/close
- 9: https://bun.sh/docs/runtime/http/websockets
- 10: fix(WebSocket): dispatch close event as a queued task, not synchronously oven-sh/bun#27259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target script header:"
awk '{printf "%6d\t%s\n", NR, $0}' skills/dev-app-verify/SKILL.md | sed -n '1,85p'
echo
echo "WebSocket events and close handling in target script:"
awk '{printf "%6d\t%s\n", NR, $0}' skills/dev-app-verify/SKILL.md | rg -n "ws\.on|ws\.close|WebSocket|Inspector|new Promise|failAllPending|const ws" || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 5520
🌐 Web query:
Bun WebSocket does not call onclose when socket pending onerror rejection process exits
💡 Result:
The behavior where Bun's onclose event is not triggered when a process exits during a pending WebSocket connection (or following an error/rejection) is a known issue related to how Bun manages the event loop, socket lifecycle, and process termination [1][2][3]. Specifically, historical issues in Bun have included: 1. Asynchronous Task Handling: Previously, onclose events could fire synchronously or fail to be queued correctly as tasks, causing them to be missed if the process exited or if other code was awaiting the resolution of an onclose-dependent promise [4][1]. This has been addressed in recent updates (e.g., PR #27259) to ensure onclose is dispatched as a proper asynchronous task, aligning with the HTML specification [4]. 2. Process Lifecycle and Pending Activity: Bun often automatically exits when it determines there is no "active" work, and WebSockets in a pending or error state were sometimes not properly tracked as "active" tasks, causing the process to terminate before the onclose event or cleanup logic could execute [5][2]. When a connection fails or is interrupted during the handshake, the underlying C++ socket state must transition to CLOSED and trigger the appropriate cleanup (error and close events) to release the process reference [2]. If this transition is interrupted or not correctly registered as "pending activity," the process may exit abruptly, bypassing the onclose callback [2]. 3. Graceful Shutdown: There have been reports that even when connections are established, Bun may terminate without sending proper close frames or executing onclose callbacks when the process receives termination signals (like SIGINT) or enters a shutdown state [3]. To mitigate this in your application: - Ensure you are using the latest version of Bun, as significant fixes for WebSocket event dispatching and lifecycle management have been implemented [4][6][2]. - If your process is exiting too early, you may need to explicitly keep the process alive (e.g., using setInterval or by ensuring the WebSocket promise chain is correctly awaited) until the socket lifecycle is fully resolved [5]. - When handling errors, ensure your onerror and onclose handlers are registered synchronously during the WebSocket instantiation to avoid missing events that fire early in the connection lifecycle [1]. If you are consistently reproducing this on the latest version of Bun, it is recommended to open a new issue on the official Bun GitHub repository with a minimal reproduction script [6][7].
Citations:
- 1:
WebSocket.oncloseruns before expected oven-sh/bun#15665 - 2: https://github.com/oven-sh/bun/blob/7e57e529/test/js/web/websocket/websocket-close-connecting.test.ts
- 3: Provide a mechanism to close Bun.serve active connections on process end oven-sh/bun#25722
- 4: fix(WebSocket): dispatch close event as a queued task, not synchronously oven-sh/bun#27259
- 5: Bun automatically exits when open WebSockets still exist. oven-sh/bun#1335
- 6: Websocket server close() never called after connection established oven-sh/bun#14946
- 7: [windows] WebSocket client tests are failing oven-sh/bun#8395
🌐 Web query:
Bun issue 1335 WebSocket close on timeout pending connection
💡 Result:
Bun issue #1335 refers to a bug where the Bun process would automatically exit even if open WebSocket connections were still active [1]. This issue was resolved in Bun v0.2.1 [2][3], which ensured that active WebSockets correctly keep the Bun process alive. While this specific issue was about process lifecycle management, WebSocket timeouts in Bun are managed via the idleTimeout configuration parameter [4]. If you are encountering issues with WebSockets closing prematurely or not closing when expected, consider the following points: 1. Process Lifecycle: The original issue #1335 regarding premature process exit was fixed in early versions of Bun [2][3]. If your process is exiting, ensure you are running a modern version of Bun. 2. Idle Timeouts: By default, Bun closes WebSocket connections that have been idle for 120 seconds [4]. This behavior can be configured using the idleTimeout option in Bun.serve [4]. 3. Troubleshooting Timeouts: Some users have reported issues with idleTimeout not behaving as expected in certain environments or versions [5][6]. If idleTimeout appears ineffective, ensure sendPings is configured correctly (it defaults to true) [4][5], as the interaction between pinging and idle tracking can sometimes cause confusion in connection state management [5]. 4. Platform Differences: There have been isolated reports of idleTimeout and sendPings behaving inconsistently on specific platforms (e.g., Windows) in older releases [6]. Always use the latest version of Bun to benefit from upstream fixes regarding WebSocket stability [6].
Citations:
- 1: Bun automatically exits when open WebSockets still exist. oven-sh/bun#1335
- 2: Bun v0.2.1 oven-sh/bun#1349
- 3: https://newreleases.io/project/github/oven-sh/bun/release/bun-v0.2.1
- 4: https://bun.com/docs/runtime/http/websockets
- 5: idleTimeout not working in WebSocket oven-sh/bun#1397
- 6: fix: idleTimeout in websockets oven-sh/bun#8187
🌐 Web query:
Bun WebSocket onclose connecting open close connection refused
💡 Result:
In Bun, encountering a "connection refused" or unexpected onclose event during WebSocket operations typically stems from network-level issues, handshake failures, or specific lifecycle behaviors within the runtime [1][2]. ### Understanding WebSocket Lifecycle and onclose - Unexpected onclose: If your WebSocket closes immediately after calling close(), note that recent updates have adjusted how Bun dispatches the onclose event [3][4]. To align with the HTML specification, Bun now dispatches onclose events asynchronously as a task, ensuring they do not fire synchronously during the .close() method call [4]. - Error Handling: For failures that occur after the connection is established (e.g., transport resets, protocol violations), Bun is designed to fire an onerror event before the onclose event [1]. If you are not seeing expected error details, ensure your onerror handler is properly attached, as some failures previously surfaced only as CloseEvent objects with a wasClean: false status [1]. - Connection Refused: A "connection refused" error generally indicates that the server is not listening on the specified port, a firewall is blocking the connection, or the handshake failed [2]. If you are using external libraries (such as ws or websocket), ensure you are using a version compatible with Bun's node:http and upgrade event implementation [2]. ### Diagnostic Tips 1. Check readyState: Use the readyState property to monitor the connection status [5][6]. - 0: CONNECTING (Pending) - 1: OPEN (Established) - 2: CLOSING (Closing) - 3: CLOSED (Closed or failed to open) 2. Verify Event Listeners: Always attach both onerror and onclose listeners to catch issues during the handshake and post-establishment phases [7][8]. 3. Verbose Logging: If you suspect the issue is related to fetch or underlying HTTP upgrades, you can often get more details by checking if the library supports verbose output or by inspecting the underlying network request [2]. 4. Server-Side Handling: If you are using Bun.serve, remember that WebSocketHandler methods are configured once per server, which differs from the event-target model used by the client-side WebSocket class [9]. If you are experiencing a persistent "connection refused" error, verify that your server is running, the URL is correct, and there are no protocol mismatches (e.g., attempting to connect via wss:// without valid certificates or vice versa) [2].
Citations:
- 1: websocket: fire error event before close for post-establishment failures oven-sh/bun#33991
- 2: 'websocket' package does not work oven-sh/bun#20547
- 3:
WebSocket.oncloseruns before expected oven-sh/bun#15665 - 4: fix(WebSocket): dispatch close event as a queued task, not synchronously oven-sh/bun#27259
- 5: https://bun.com/reference/bun/WebSocketReadyState
- 6: https://bun.sh/reference/bun/WebSocket/readyState
- 7: https://bun.sh/reference/bun/WebSocket
- 8: https://bun.com/reference/bun/WebSocket
- 9: https://bun.com/docs/runtime/http/websockets
Close ws before rejecting the open timeout.
If the inspector connection never emits open, rej(...) leaves the connecting WebSocket without closing it. Rejecting does not guarantee ws.close() runs, so manually close before rejecting. Clear the timer on successful open; assign a simple ws.onopen = () => { clearTimeout(timer); res(); }; to avoid replacing any existing handler this script needs.
Close ws before rejecting the open timeout.
If the inspector connection never emits open, rej(...) leaves the connecting WebSocket without closing it. Rejecting does not guarantee ws.close() runs, so manually close before rejecting. Clear the timer on successful open; assign a simple ws.onopen = () => { clearTimeout(timer); res(); }; to avoid replacing existing handlers this script needs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/dev-app-verify/SKILL.md` at line 77, Update the WebSocket open wait
around ws.onopen to store the timeout handle, clear it when ws emits open, and
resolve from an onopen callback. On timeout, close ws before rejecting so the
failed inspector connection is cleaned up.
1 participant
What
Consolidates the UI knowledge earned across the downloads titlebar indicator work (#3441, #3443) into permanent project docs, and makes the Claude Code skills part of the repo:
docs/desktop-ui-guidelines.md— Fuselage token semantics and traps (runtime-injected tokens,status-background-*vs bullet vs badge, the icon-button color chain), icon/button geometry facts, the.18smicro-interaction standard, tab bar button conventions (the 0.6 dimming rule and its specificity override, square icon-only buttons, compact titlebar sizing, flex-flow traps), and the SVGtransform-originclipping pitfall.skills/dev-app-verify/SKILL.md— agent-agnostic runbook (same convention asskills/desktop-qa-flows) for driving and screenshotting the running dev app through the port-9339 main-process inspector: ready-made script, menu-item triggering, DOM-truth recipes, and the three pitfalls that produce false alarms (window occlusion freezingcapturePage, watcher restarts killing in-flight state, singleton-lock wedges).CLAUDE.md+AGENTS.md— pointers so every agent (Claude, Codex, Cursor, GitHub agents) hits the guide before styling tab bar controls and verifies UI changes at runtime..claude/gitignore is now selective instead of blanket — the blanket ignore was hiding project knowledge (CLAUDE.md references.claude/skills/gitnexus/*six times — broken paths for anyone cloning) while.claude/agentsand.claude/hookswere already tracked. Now committed: the 9 skills (electron-build, electron-bump, ship-release, release-notes, boot-wedge-debug, i18n-audit, new-ipc-channel, gitnexus/*, and a dev-app-verify stub pointing at the canonicalskills/copy so they can't drift) and the hooks-onlysettings.json. Still ignored:settings.local.json,mytasks/,worktrees/,rules/,commands/,scheduled_tasks.lock(personal/machine state). Audited for personal content before committing.Why
Every item in the guide cost a debugging round or a review round during #3443. Recording them turns those rounds into a lookup, and committing the skills turns one machine's process knowledge into the team's.
Notes
.claude/skillscopies will hit a checkout conflict on first pull after merge — delete the local copies and pull.Summary by CodeRabbit
Documentation
Chores