Skip to content

fix(inventory+history): address post-merge CR findings on #792 #896

Description

@cristim

Follow-up from CodeRabbit's post-merge review on #792 (review posted after merge, so the findings can't be addressed inside #792).

PR #792 added the "Monthly Cost" column to two history tables and renamed/restructured the active-commitments table, but three skeleton-loader callsites still reference the pre-#792 column counts in code + comments. The skeletons therefore render with the wrong number of placeholder cells, so the shimmer is one cell short of the real table on first paint.

Findings (all from the post-merge CR review on #792)

  • frontend/src/history.ts:217 — showSkeletonRows(listEl, 8, 11) should pass 12 (purchase-history table now renders 12 <th> per renderHistoryList lines 652-663: Status / Date / Provider / Service / Type / Region / Count / Term / Upfront Cost / Monthly Cost / Monthly Savings / Plan). Comment on line 213 also still says "(11)" and omits "Monthly Cost" from the column list.

  • frontend/src/history.ts:224 — showSkeletonRows(queueEl, 3, 8) should pass 12 (approval queue table now renders 12 <th> per lines 948-959: Date / Account / Provider / Service / Count / Term / Payment / Monthly Cost / Upfront Cost / Monthly Savings / Created by / Actions). Comment on lines 218-220 is also out of sync (says "3 rows x 8 cols", lists 8 headers).

  • frontend/src/inventory.ts:86 — Comment says "5 rows × 10 cols" but ACTIVE_COMMITMENTS_COLS = 11 (line 72) after feat(inventory+history): align Monthly Cost/Savings columns across both tables #792 added "Monthly savings". The runtime value is correct (the comment is the only thing out of sync), but the comment is misleading for future readers.

All three are quick-win comment/literal updates with no behavioural blast radius beyond the shimmer appearance during the first render of these tables.

Scope

  • Comment + skeleton-call-site updates only — no test changes required (renderer behaviour unchanged; skeleton row count is purely cosmetic and not asserted in tests).
  • No other files touched.

References

Activity

  1. added 2 commits that reference this issue on Jun 1, 2026
    ac140fc
    840993c
  2. added
    pr-createdA PR has been opened for this issue (dedup guard for the auto-PR loop)
    pr-mergedThe PR for this issue has been merged
    on Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/sHoursimpact/fewLimited audiencepr-createdA PR has been opened for this issue (dedup guard for the auto-PR loop)pr-mergedThe PR for this issue has been mergedpriority/p3Polish / idea / may never shipseverity/lowMinor harmtriagedItem has been triagedtype/bugDefecturgency/this-sprintWithin the current sprint

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions