feat: say which install key has devices waiting for a decision - #7070
Conversation
|
Claude finished @gustavosbarreto's task in 6m 18s —— View job Code Review CompleteReviewed 16 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 low). Additional notesTwo areas were investigated in depth and cleared, worth noting for the author:
Security (SQL injection / cross-tenant scoping of the new subquery), the new Go store test, the UI test wiring, and the bun To request another review round, comment |
|
Documentation preview: https://fb8d944a.shellhub-docs.pages.dev |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Counts devices rather than history events without a verdict: an event outlives the device it | ||
| // registered, so a removed pending device would keep inflating the count. |
There was a problem hiding this comment.
This explanatory comment on an unexported const goes against the repo's code-style rule (.claude/rules/code-style.md): "Write no comments", with only two exceptions — a doc comment on an exported declaration, and a suppression's reason. pendingDevicesExpr is unexported, so neither applies. The rule specifically calls out this case: rationale like "an ordering invariant" or "why" a design choice was made "belongs in the commit message ... Write it in the commit message, in full."
The rationale here (counting devices vs. history events so a removed pending device doesn't keep inflating the count) is worth keeping — it's already captured well in the PR body, so moving it to the commit message and dropping the code comment satisfies the convention.
|
A few things to fix before merge: Correctness
Standards
Worth a look
|
aa7e195 to
d803804
Compare
|
Pushed. What changed, and what did not.
Missing index. Migration 026 adds a partial index on Arithmetic inline in The comments. Both removed. Why the count is over devices rather than over history events is in the server commit message; why the hatch takes The migration keeps its comment, matching every other migration in that directory, 020 included. Say the word and it moves to the commit message too.
Raw status strings. Changed to
|
|
Documentation preview: https://03cece93.shellhub-docs.pages.dev |
The install key list carries pending_devices. It counts devices with status pending rather than history events without a verdict, because an event outlives the device it registered: a pending device that was later removed would keep inflating the count. Counting devices also makes the sum across a namespace's keys match the pending_devices the stats endpoint reports, for every device that has a key -- install_key_id is nullable, so a device without one is counted by the stats endpoint and by no key. The count is a correlated subquery per key, so migration 026 indexes the devices it reads.
A waiting device is a claim on the allowance an accepted one spends, so it belongs in the usage cell. While devices wait they take the headline and the spend moves below the bar. Where the key has a limit, the bar draws what accepting the queue would spend, hatched once that would pass the limit -- the hatch takes currentColor so the caller picks the hue rather than base.css deciding it. Every path that decides a device now invalidates the keys list: accept, reject and remove, and the pairing-code accept, which admits a device without going through the device mutations.
Both pointed at /devices?status=pending, which the devices list forces back to accepted. They now point at the install keys list, where the decision is made.
The Pending tab it described has not existed since acceptance moved to the install keys area.
d803804 to
ebcfc26
Compare
|
Documentation preview: https://1427b2d5.shellhub-docs.pages.dev |
|
/review |
|
Claude finished @gustavosbarreto's task in 6m 54s —— View job Code Review CompleteReviewed 21 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is. Notes for the author (not blocking)
To request another review round, comment |
2 participants
The Dashboard's Pending Devices card pointed at
/devices?status=pending, a URL the devices list undoes: it hides every status tab but accepted and an effect forces the parameter back. The count was real, the destination was not.Acceptance lives in the install keys area, so that list is where the card should land. It just had nothing to say. With two devices sitting pending, both keys read
0 / ∞, becauseused_timesonly counts devices already accepted. The one page where a pending registration is reviewed gave no sign that anything was waiting, and finding one meant opening each key's activity in turn.So the list now says it. The install key payload carries
pending_devices, and the usage cell shows it: a device awaiting a decision is a claim on the same allowance an accepted one spends, so that column is the only place able to say that accepting the queue would exhaust the key. While devices wait they take the cell's headline and the spend moves below the bar. On a key with a real limit the bar draws the projection, and hatches when accepting everything waiting would pass the limit.The count is over devices with status pending rather than history events without a verdict, because an event outlives the device it registered and a removed pending device would keep inflating it. Counting devices also makes the sum across a namespace's keys equal the
pending_devicesfigure the stats endpoint reports, so the card and the page agree.The Dashboard card and the callout under the install command both point at the install keys list now, and the docs stop describing a Pending tab that has not existed since acceptance moved.
Supersedes #7048, which answered the same issue with a dedicated
/pending-devicespage and a sidebar entry.Closes #7043
Closes #7048