Skip to content

refactor(web-ui): give the row key one owner - #1286

Open
kno wants to merge 1 commit into
amule-org:masterfrom
kno:fix/webui-row-key-single-owner
Open

refactor(web-ui): give the row key one owner#1286
kno wants to merge 1 commit into
amule-org:masterfrom
kno:fix/webui-row-key-single-owner

Conversation

@kno

@kno kno commented Sep 4, 2026

Copy link
Copy Markdown

Closes #1285.

clients.js:29 and client-table.js:277 each carried the same row-identity
expression, byte for byte:

(c) => c.ecid != null ? c.ecid : c.user_hash

They had to agree. clients.js keyed its selection state with its copy;
ClientTable matched selectedKey against its own and also used it as the
rowKey for reconciliation. A divergence would have stopped the selected-row
highlight from ever appearing — no error, no exception, no test, just a
highlight that quietly never shows.

Why the key moves

ClientTable already hands its function to the generic table as rowKey
(client-table.js:287), and VirtualTable already calls it for the row's key
(table.js:265). It just did not pass the result to the click handler, and that
was the only reason the panel had to derive it a second time.

Now it does, so clients.js carries a key it never interprets and
client-table.js is the only module that knows what identifies a row. The value
used for reconciliation, for rowClass, and for the selection is the same one
by construction rather than by two expressions happening to match.

Scope

table.js computes the key once per row rather than twice, and appends it to
the handler call. Appending is backward compatible: downloads.js:40 and
shared.js:39 declare (d, e) and (s, e), so they ignore a third argument
and are untouched.

client-table.js is unchanged and its key stays unexported.

Verification

  • Active rows key on the numeric ecid; Known rows have none, because
    WriteKnownClientObject never emits one, so they key on user_hash. Both
    paths were exercised, including the click-again-to-close toggle and the
    switch between two rows.
  • rowKey is a required prop — table.js:265 already called it unguarded — so
    no caller can reach the new argument without one.

Not covered: the web UI has no JS test harness, so this is verified by reading
the call chain and exercising the key logic in isolation, not by a test.
check-i18n.mjs passes and all five touched or dependent modules parse under
node --check; neither exercises the change.

@got3nks

got3nks commented Sep 4, 2026

Copy link
Copy Markdown

Verified the invariant closes. client-table.js:287 passes rowKey=${key}, table.js hands that same computed value to onRowClick, and :279 compares key(c) === selectedKey: same function, same row, so the two copies can no longer drift.

Checked the two ways this could break. Nothing still references the removed rowKey in clients.js, and every VirtualTable caller passes rowKey, so the unguarded call is no more exposed than before. downloads.js:40 and shared.js:39 declare (d, e) and ignore the third argument.

The Known-tab case is fine: a known row never gains an ecid of its own (the live one sits on c._live), so its key stays user_hash and the highlight cannot be lost mid-session.

Scope looks right too. The other views key on a single field, so even where the key is derived twice there is no fallback expression to diverge; this was the only compound one.

No findings from me. Assigning @ngosang, who owns Web UI development, and leaving the merge to him.

clients.js and client-table.js each carried the same row-identity expression,
byte for byte, and had to agree: clients.js keyed its selection with its copy
while ClientTable matched selectedKey with its own. A divergence would have
stopped the selected-row highlight from ever appearing, with no error and no
test to catch it.

VirtualTable already receives that function as rowKey and already calls it for
reconciliation; it just did not pass the result to the click handler. It does
now, so clients.js carries a key it never derives and ClientTable is the only
module that knows what identifies a row.

The argument is appended, so downloads.js and shared.js -- which declare
(d, e) and (s, e) -- ignore it and are untouched.

Refs amule-org#1285
@kno
kno force-pushed the fix/webui-row-key-single-owner branch from 2865a41 to e35f92a Compare September 4, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[web-ui] Two implicit invariants in the Clients views are worth a comment

3 participants