Skip to content

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

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/792-postmerge-cr-followup
Jun 3, 2026
Merged

fix(inventory+history): address post-merge CR findings on #792 (closes #896)#897
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/792-postmerge-cr-followup

Conversation

@cristim

@cristim cristim commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to #792: CodeRabbit posted a substantive review on #792 about an hour after the PR was merged, so the findings could not be addressed in-PR. This PR applies the three valid findings.

Closes #896.

Findings + disposition

All three findings re-verified against the post-merge tip of feat/multicloud-web-frontend and still present.

# Location Finding Disposition
1 frontend/src/history.ts:217 showSkeletonRows(listEl, 8, 11) — purchase-history table now renders 12 <th> (Status / Date / Provider / Service / Type / Region / Count / Term / Upfront Cost / Monthly Cost / Monthly Savings / Plan). Comment lines 213-215 also out of sync. Fixed: 11 → 12 + comment updated
2 frontend/src/history.ts:224 showSkeletonRows(queueEl, 3, 8) — approval queue table now renders 12 <th> (Date / Account / Provider / Service / Count / Term / Payment / Monthly Cost / Upfront Cost / Monthly Savings / Created by / Actions). Comment lines 218-222 also out of sync. Fixed: 8 → 12 + comment updated
3 frontend/src/inventory.ts:86 Comment says "5 rows × 10 cols" but ACTIVE_COMMITMENTS_COLS = 11 (line 72) after #792 added "Monthly savings". Runtime is already correct (call site uses the constant); comment is the only thing stale. Fixed: comment 10 → 11

None of the findings were dismissed as stale or out-of-scope. No new follow-up issues were filed (the review surfaced no architectural concerns beyond the column-count mismatches).

Verification

cd frontend
npx jest --testPathPattern='(inventory|history)'   # 10 suites / 202 tests passing
npx tsc --noEmit                                   # no errors

The renderer's runtime behaviour is unchanged — these are skeleton placeholder column counts (cosmetic, pre-data), and tests don't assert the skeleton cell count, so no test changes are required.

Test plan

  • npx jest --testPathPattern='(inventory|history)' green (10 suites / 202 tests)
  • npx tsc --noEmit clean
  • Cross-checked all three skeleton call sites against the rendered <th> lists in renderHistoryList, the pending-approval queue (history.ts:946-959), and renderActiveCommitmentsTable's headers array (inventory.ts:156).

Summary by CodeRabbit

  • Bug Fixes

    • Fixed skeleton loading display for history tables to show correct column count.
    • Fixed pending-approval-queue skeleton to display correct number of columns during loading.
  • Documentation

    • Updated internal documentation to accurately reflect table structure specifications.

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/this-sprint Within the current sprint impact/few Limited audience effort/s Hours type/bug Defect labels Jun 1, 2026
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ef92dd44-fe3b-4b61-b059-b9a28a8be775

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR updates skeleton UI column counts and related comments in the history and inventory loaders to match the current table structures. PR #792 added "Monthly Cost" and "Monthly Savings" columns to multiple tables, but three skeleton-loader callsites and comments still referenced the pre-#792 column counts. The shimmer UI now renders the correct number of placeholder cells.

Changes

Skeleton UI Column Count Corrections

Layer / File(s) Summary
Skeleton column count and comment updates
frontend/src/history.ts, frontend/src/inventory.ts
loadHistory() skeleton calls for purchase-history and approval-queue tables updated to render 12 columns each (previously 11 and 8); inline comments listing column names and dimensions corrected. loadActiveCommitments() comment updated from 5 rows × 10 columns to 5 rows × 11 columns to reflect the "Monthly Savings" column added in PR #792.

Sequence Diagram(s)

No diagram needed; these are comment and literal configuration updates with no functional control flow or component interactions.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • LeanerCloud/CUDly#792: Added "Monthly Cost" and "Monthly Savings" columns to history and inventory tables; this PR updates the skeleton loaders and comments that are out of sync with those column additions.
  • LeanerCloud/CUDly#346: Previously standardized skeleton helpers and set history table skeletons to 11 columns; this PR further updates the skeleton column counts following PR #792's column additions.
  • LeanerCloud/CUDly#734: Added approval-queue column population and structure changes; this PR aligns the skeleton rendering with the updated approval-queue table structure from that work.

Poem

🐰 Eleven became twelve, and eight took its turn,
Comments grow wise as the table-counts learn.
Skeletons shimmer with correct cells now,
First paint stays pretty—we've smoothed the brow!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title precisely describes the primary change: fixing post-merge code review findings on PR #792 related to skeleton column counts and comments.
Linked Issues check ✅ Passed All three coding requirements from issue #896 are met: history.ts skeleton calls updated from 11→12 and 8→12 columns with matching comments, and inventory.ts comment updated from 10→11 columns.
Out of Scope Changes check ✅ Passed All changes are scoped to comment and skeleton call-site updates in two files; no extraneous modifications or behavioral logic changes detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/792-postmerge-cr-followup

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim

cristim commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

… tables (closes #896)

PR #792 added the "Monthly Cost" column to two history tables and to the
active-commitments table, but the skeleton-loader call sites + their
explanatory comments still referenced the pre-#792 column counts. The
shimmer placeholders therefore render with one fewer cell than the real
table on first paint, and the comments are out of sync with the
rendered <th> lists.

- history.ts:217 — purchase-history skeleton: showSkeletonRows
  3rd arg 11 -> 12; comment lists the 12 rendered columns
  (adds "Monthly Cost").
- history.ts:224 — pending-approval queue skeleton: showSkeletonRows
  3rd arg 8 -> 12; comment lists the 12 rendered columns
  (Date / Account / Provider / Service / Count / Term / Payment /
  Monthly Cost / Upfront Cost / Monthly Savings / Created by / Actions).
- inventory.ts:86 — active-commitments skeleton comment: "10 cols" ->
  "11 cols" to match ACTIVE_COMMITMENTS_COLS (the call site already
  uses the constant, so this is comment-only).

Verified against the rendered <th> lists in renderHistoryList
(history.ts:650-663), the pending-approval queue (history.ts:946-959),
and renderActiveCommitmentsTable's headers array
(inventory.ts:156). Test suite (10 suites / 202 tests) and
tsc --noEmit both pass.
@cristim
cristim force-pushed the fix/792-postmerge-cr-followup branch from a2d14c0 to ac140fc Compare June 1, 2026 16:07
@cristim

cristim commented Jun 3, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

This PR's trigger from 2026-06-01 was acknowledged but no review body was posted (auto-reviews are disabled on the non-default base branch). Re-pinging for a fresh pass on commit ac140fc.

@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr view 897 --repo LeanerCloud/CUDly --json headRefName,baseRefName,commits
echo "---DIFF---"
gh pr diff 897 --repo LeanerCloud/CUDly

Length of output: 4541



@cristim: Reviewed commit ac140fc. The three targeted fixes are all correct — here's a brief summary of what I verified:


frontend/src/history.ts

  1. Purchase-history skeletonshowSkeletonRows(listEl, 8, 11)showSkeletonRows(listEl, 8, 12).
    Comment column list updated and verified: Status / Date / Provider / Service / Type / Region / Count / Term / Upfront Cost / Monthly Cost / Monthly Savings / Plan = 12

  2. Pending-approval queue skeletonshowSkeletonRows(queueEl, 3, 8)showSkeletonRows(queueEl, 3, 12).
    Comment column list updated and verified: Date / Account / Provider / Service / Count / Term / Payment / Monthly Cost / Upfront Cost / Monthly Savings / Created by / Actions = 12

frontend/src/inventory.ts

  1. Active-commitments comment"5 rows × 10 cols""5 rows × 11 cols".
    Comment-only fix; the call site correctly uses ACTIVE_COMMITMENTS_COLS already ✅

No issues found. The changes are minimal, accurate, and consistent with the rendered <th> lists. The PR is ready to merge.

[approve]


🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

@cristim
cristim merged commit 840993c into feat/multicloud-web-frontend Jun 3, 2026
5 checks passed
@cristim
cristim deleted the fix/792-postmerge-cr-followup branch June 3, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant