[pwilke]

What

Guards the Process cast in the five per-process InfoScreen actions —
actionLsof, actionShowLocks, actionStrace, actionShowEnvScreen,
actionShowCommandScreen — by checking the selected row's class before casting,
instead of relying on an assert after the cast.

Why

Each of these actions is gated only by Action_readableProcess /
Action_writeableProcess, which check nothing but settings->ss->dynamic. They
then cast the selected main-panel row straight to Process* and assert its
class.

settings->ss and host->activeTable can fall out of sync:

• rebuildSettingsArray() (ScreensPanel.c) points settings->ss at the screen
  selected in the Screens list, without touching host->activeTable.
• Action_runSetup() returns without calling setActiveScreen() — the only
  place that re-pairs ss and activeTable.

So after visiting Setup, ss can refer to a built-in (non-dynamic) screen while
activeTable still refers to a dynamic table whose rows are Instances, not
Processes. The readable/writeable check passes, the row is cast to
Process, and the assert aborts in debug builds — or, under NDEBUG, a
non-Process object is used as a Process (undefined behavior).

Reproduction (PCP build, which has dynamic screens):

1. Make a dynamic screen active.
2. Open Setup and select a built-in screen in the Screens list.
3. Press F7/[/-/F8/]/+/F9 — rebuildSettingsArray sets
   settings->ss to the built-in screen; host->activeTable is unchanged.
4. Exit Setup and press l, x, s, e or c.

Fix

Fetch the selected row as an Object*, bail out with HTOP_OK unless
Object_isA(..., &Process_class), then cast. Each call's existing const-ness is
preserved.

Notes

• Found by CodeRabbit during review of Show memory map of highlighted process #2148 (adds a memory-map screen). Per
  @BenBE's suggestion there, the pre-existing occurrences are fixed here on their
  own; Show memory map of highlighted process #2148 writes its new action correctly from the start.
• No automated test: htop has no C unit-test harness, and the only integration
  driver (test_spec.lua) runs the plain build, which has no dynamic screens to
  produce the desync. Verified by code analysis and the manual sequence above.

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

Five process-specific actions now check that the selected panel object exists and is a Process before casting it. If the selection is missing or has another type, each action returns HTOP_OK.

Possibly related PRs

• htop-dev/htop#2148: Adds a process-type check to actionShowMaps, using the same selected-object validation pattern.

Suggested reviewers: benbe, explorer09

Change: Bug fix

Merge Risk: Low · up to e87dc

Setup now restores the correct active table, but switching tables can unexpectedly follow an unrelated row with the same numeric ID. This is a bounded selection issue; preserve table-specific follow state and add regression coverage.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Five actions check the selected row
Before a screen can open
A missing choice returns cleanly
Other objects pass no cast
The process path proceeds as planned}

[coderabbitai]

Note

Follow-up findings are listed below so you can review them separately from inline comments.

Follow-up (1)

Minor (1)

Minor · Resync the active table when Setup closes. · Action.c:612

Action.c:612
Minor · Functional Correctness · Quick win

Resync the active table when Setup closes.

Moving a dynamic screen in Setup can set settings->ss to that screen. Action_runSetup() does not update host->activeTable, so the main panel keeps the process table. actionShowLocks() then rejects valid process rows through Action_readableProcess(). Reconcile settings->ss and host->activeTable when Setup returns.

Nitpick comments: 1

Action.c: 1

594-594: Trivial · Maintainability & Code Quality · Poor tradeoff

Add regression coverage for the mismatched-row guards.

No CI step runs the Lua suite, and no C test target exists. Removing any of the five new Object_isA guards would not fail CI. The Lua suite sends keys only to the main, Setup, help, and theme screens. No case selects a non-Process row on a dynamic screen.

Add a case that opens a dynamic screen with an Instance row selected, then sends l, x, s, e, and w. The case should check that no screen opens. The test configuration must expose a dynamic screen for this case. If the existing configuration cannot provide one, record the gap as a known limitation.

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: d8e44ef5-2dbe-4862-8255-724faa83e9d6

Commits

Reviewing files that changed from the base of the PR and between 5091900 and 5f15a39.

Files selected for processing: 1

• Action.c

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

[BenBE]

The reason we should fix this goes even a bit deeper, as there are plans (not soon, but they exist), to show things other than processes in an active screen. Ad even today, if for some reason, one of the info screens re-triggered one of the existing Actions this type confusion could cause havoc.

Also note, that in theory you could have an active screen without an active, selected process (empty process list ;-)). So being extra paranoid with the active screen list and its rows absolutely doesn't hurt.

@pwilke Can you add a second commit resolving the follow-up finding from @coderabbitai? We can ignore the CI nitpick. ;-)

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@BenBE Agreed. Actions should handle an empty selection and verify that the selected row is a Process before using process-specific data. We can leave the CI nitpick out of this PR.

---

Learnings used

Learnt from: BenBE
Repo: htop-dev/htop PR: 0
File: :0-0
Timestamp: 2026-05-24T22:27:59.364Z
Learning: For the htop-dev/htop repository, commits that correct issues introduced within the same PR must be squashed/fixup'd into the original commit rather than left as separate commits. Maintainer preference (BenBE) is for clean, linear commit history before merging. Encourage contributors to use `git rebase -i` with fixup to achieve this.

_{You are interacting with an AI system.}

[coderabbitai]

Actionable comments posted: 2

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: dd6dc5ae-8b28-413b-a6dc-fdb465d84fc1

Commits

Reviewing files that changed from the base of the PR and between 5f15a39 and e87dc28.

Files selected for processing: 1

• Action.c

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

[coderabbitai]

Minor · Functional Correctness · Low value

Supported by static analysis

Script executed:

rg -n -C 3 'following|activeTable|\.id\s*=|->id\s*=' --glob '*.[ch]' | head -n 240

Repository: htop-dev/htop

Length of output: 12061

---

Script executed:

git diff 50919007b00ef657acdcea084c31a22b152c3799 e87dc28d5e7bebdb3adb6c582b5b745bf25fc11e -- Action.c
rg -n -F -e 'setActiveScreen' -e 'actionSetup' -e 'HTOP_KEEP_FOLLOWING' -- Action.c MainPanel.c ScreenManager.c
rg -n -F -e 'following' -e 'Instance_setId' -e 'Process_setPid' -- Table.c Action.c MainPanel.c Process.h Process.c pcp/
nl -ba Action.c | sed -n '1,160p'

Repository: htop-dev/htop

Length of output: 14407

---

Script executed:

nl -ba Action.c | sed -n '360,450p;560,605p'
nl -ba MainPanel.c | sed -n '55,185p'
nl -ba Table.c | sed -n '250,410p'
nl -ba Process.h | sed -n '235,270p'
nl -ba pcp/InDomTable.c | sed -n '1,150p'
rg -n -F -e 'rebuildSettingsArray' -e 'ssIndex' -- '*Screen*' Settings.c 2>/dev/null

Repository: htop-dev/htop

Length of output: 23767

---

Script executed:

rg -n -F -e 'rebuildSettingsArray' -e 'ssIndex' -e 'InDomTable_new' -e 'Metric_iterate' --glob '*.[ch]' .
rg -n -F -e 'Action_follow' -e 'following' -- Action.c MainPanel.c ScreenManager.c
rg -n -F -e 'dynamic screen' -e 'Dynamic Screen' -e 'dynamic screens' -- README* docs pcp 2>/dev/null

Repository: htop-dev/htop

Length of output: 3891

---

Script executed:

nl -ba ScreensPanel.c | sed -n '60,235p;280,365p'
nl -ba pcp/PCPDynamicScreen.c | sed -n '300,405p'
nl -ba pcp/Metric.c | sed -n '150,220p'
nl -ba ScreenManager.c | sed -n '100,175p;280,450p'
rg --files pcp | grep -E 'screen|conf|README'
rg -n -F -e 'PCPDynamicScreen' -e 'dynamic screen' -e 'dynamic screens' -- pcp docs README.md

Repository: htop-dev/htop

Length of output: 33379

---

Script executed:

nl -ba ScreensPanel.c | sed -n '1,60p;234,280p;365,430p'
nl -ba ScreenManager.c | sed -n '225,282p'
nl -ba Table.c | sed -n '1,115p;115,255p'
nl -ba pcp/Instance.c | sed -n '1,180p'
nl -ba Machine.c | sed -n '50,120p'
nl -ba pcp/screens/devices | sed -n '1,100p'
rg -n -F -e 'Action_follow' -e 'following' -- docs/htop.1.in htop.1.in README.md 2>/dev/null

Repository: htop-dev/htop

Length of output: 30591

---

Do not transfer follow IDs between unrelated tables on Setup exit.

If Setup selects a different table, setActiveScreen can copy a follow ID from the previous table. A PCP instance with ID 1 can then cause the process table to follow PID 1. Preserve each table’s follow state without transferring IDs across tables.

---

Trivial · Maintainability & Code Quality · Poor tradeoff

Supported by static analysis

Script executed:

rg -n -i 'setup|dynamic.screen|activeTable|ssIndex|screen' test_spec.lua .github --glob '*.lua' --glob '*.yml' --glob '*.yaml' | head -n 180

Repository: htop-dev/htop

Length of output: 222

---

Script executed:

git diff 50919007b00ef657acdcea084c31a22b152c3799 e87dc28d5e7bebdb3adb6c582b5b745bf25fc11e -- Action.c
nl -ba Action.c | sed -n '1,180p'
rg --files -g '*test*' -g '*Test*' -g '*spec*' -g 'Makefile*' -g '*.yml' -g '*.yaml' -g 'configure.ac' -g 'meson.build' | sort
nl -ba test_spec.lua | sed -n '1,180p;350,460p'
rg -n -i -e 'dynamic.screen' -e 'test_spec' -e 'make check' -e 'busted' -e 'activeTable' -e 'ssIndex' -- .github Makefile.am configure.ac test_spec.lua 2>/dev/null || test "$?" -eq 1

Repository: htop-dev/htop

Length of output: 19867

---

Script executed:

rg -n -F -e 'setActiveScreen' -e 'Action_runSetup' -- Action.c
rg -n -F -e 'rebuildSettingsArray' -e 'ssIndex' -e 'activeTable' -- ScreensPanel.c Settings.c Machine.c Action.c
git ls-files | grep -Ei '(^|/)(test|tests|spec|specs)(/|[._-])|(^|/)[^/]*(test|spec)[^/]*$|(^|/)\.github/workflows/|(^|/)\.travis\.yml$'
rg -n -i -e 'test|busted|coverage|dynamic' -- Makefile.am configure.ac .travis.yml .github test_spec.lua default.htoprc 2>/dev/null
nl -ba test_spec.lua | sed -n '180,350p;458,800p'
nl -ba .travis.yml

Repository: htop-dev/htop

Length of output: 28821

---

Script executed:

nl -ba Action.c | sed -n '365,445p;585,600p'
nl -ba ScreensPanel.c | sed -n '90,235p;280,365p'
nl -ba Machine.c | sed -n '50,100p'
nl -ba .github/workflows/ci.yml | sed -n '1,175p;200,245p;410,455p'
nl -ba TESTPLAN | sed -n '1,200p'
git ls-files '*htoprc*'
if test -f default.htoprc; then nl -ba default.htoprc; else echo 'default.htoprc is not present in checkout'; fi
rg -n -i -e 'busted' -e 'test_spec\.lua' -e 'make check' -e 'installcheck' --glob '!test_spec.lua' --glob '!configure.ac' --glob '!Makefile.in' --hidden -g '!.git/**' . || test "$?" -eq 1

Repository: htop-dev/htop

Length of output: 31120

---

Add a regression test for Setup table synchronization.

If a screen backed by a different table is reordered in Setup, removing this call can leave settings->ss and host->activeTable out of sync. Add a test that reorders screens backed by different tables and checks the resulting pairing.

[pwilke]

I see two ways out of this, tell me what you prefer.

• inline most of the setActiveScreen function here, without the follow ID transfers

   Machine* host = st->host;
   if (!settings->ss->table)
      settings->ss->table = host->processTable;
   host->activeTable = settings->ss->table;
  
   bool readonly = Settings_isReadonly() || (host->activeTable != host->processTable);
   MainPanel_setFunctionBar(st->mainPanel, readonly);
  

would replace the current setActiveScreen call.

• extract the follow-ID-free part of setActiveScreen into another function, say setActiveScreenNoFollow, can call this latter function in Action_runSetup

static inline bool setActiveScreenNoFollow(Settings* settings, State* st, unsigned int ssIdx) {
   assert(settings->ssIndex == ssIdx);
   Machine* host = st->host;

   settings->ss = settings->screens[ssIdx];
   if (!settings->ss->table)
      settings->ss->table = host->processTable;
   host->activeTable = settings->ss->table;

   // set correct functionBar - readonly if requested, and/or with non-process screens
   bool readonly = Settings_isReadonly() || (host->activeTable != host->processTable);
   MainPanel_setFunctionBar(st->mainPanel, readonly);

   return host->activeTable->following != -1;
}

static inline bool setActiveScreen(Settings* settings, State* st, unsigned int ssIdx) {
   // Save following state from current table before switching screens
   int following = st->host->activeTable->following;

   setActiveScreenNoFollow(settings, st, ssIdx);

   // Transfer following state to new table if it doesn't already have one
   Machine* host = st->host;
   if (following != -1 && host->activeTable->following == -1)
      host->activeTable->following = following;

   return host->activeTable->following != -1;
}

Which version do you prefer? Or a third one?

[coderabbitai]

Analysis chain

Script executed:

#!/bin/bash
for file in MainPanel.c ScreenManager.c ScreensPanel.c Table.c; do
  echo "===== outline: $file ====="
  ast-grep outline "$file" --items all --match 'ScreenManager|MainPanel|following|rebuildSettingsArray|Table' 2>/dev/null || true
done
echo '===== follow-state and reaction handling ====='
rg -n -C 5 -e 'HTOP_KEEP_FOLLOWING' -e 'following' -e 'rebuildSettingsArray' -- MainPanel.c ScreenManager.c ScreensPanel.c Table.c Action.c

Length of output: 25374

---

@pwilke I prefer the second option. It keeps screen and function-bar synchronization in one place and avoids transferring a follow ID on Setup exit. It also explicitly sets settings->ss from settings->screens[ssIdx].

The wrapper preserves the existing behavior for normal tab changes. Those changes can still transfer follow IDs between different tables, so this proposal addresses Setup exit only.

After the change, please verify the Setup reproduction with process and PCP screens, including that Setup does not copy the previous table’s follow ID to the newly active table.

_{You are interacting with an AI system.}

[pwilke]

How about this new version of setActiveScreen

static inline bool setActiveScreen(Settings* settings, State* st, unsigned int ssIdx, bool transferFollow) {
   assert(settings->ssIndex == ssIdx);
   Machine* host = st->host;

   // Save following state from current table before switching screens
   int following = host->activeTable->following;

   settings->ss = settings->screens[ssIdx];
   if (!settings->ss->table)
      settings->ss->table = host->processTable;
   host->activeTable = settings->ss->table;

   // Transfer following state to the new table if requested and it has none.
   // Skipped across unrelated tables (e.g. on Setup exit): a row id is not
   // meaningful from one table kind to another.
   if (transferFollow && following != -1 && host->activeTable->following == -1)
      host->activeTable->following = following;

   // set correct functionBar - readonly if requested, and/or with non-process screens
   bool readonly = Settings_isReadonly() || (host->activeTable != host->processTable);
   MainPanel_setFunctionBar(st->mainPanel, readonly);

   return host->activeTable->following != -1;
}

It adds a flag saying whether follow should be transferred. There are 3 existing calls to patch with an extra true parameter, and our new call in Action_runSetup that would call setActiveScreen with transferFollow set to false.

I think this is better, what do you think?

[coderabbitai]

Analysis chain

Script executed:

#!/bin/bash
for file in Action.c MainPanel.c ScreenManager.c Table.c; do
  printf '\n===== %s outline =====\n' "$file"
  ast-grep outline "$file" --items all --match 'actionSetup|setActiveScreen|ScreenManager_run|following|MainPanel' 2>/dev/null || true
done
printf '\n===== follow state and reaction handling =====\n'
rg -n -C 6 -e 'HTOP_KEEP_FOLLOWING' -e 'following' -- Action.c MainPanel.c ScreenManager.c Table.c

Length of output: 23097

---

@pwilke Yes, I prefer this version. It keeps screen synchronization in one function and makes follow-ID transfer explicit.

Pass false on Setup exit and true at the three existing screen-switch calls. Also update the forward declaration. I would adjust the comment: transfers are skipped on Setup exit, but normal screen switches can still transfer IDs between unrelated tables.

After the change, please verify the Setup reproduction with process and PCP screens. Check that the destination table keeps its own follow state and does not receive the previous table’s ID.

_{You are interacting with an AI system.}