Conversation
The Project filter loaded every project up front via useProjectOptions (a sequential, all-pages fetch), and the table's own loading state was folded into that fetch finishing — so a slow or large project catalogue kept the table stuck on its spinner even after the actual time-card search had already resolved. Swap the filter to AsyncProjectMultiSelect, the same lazy/scroll-paginated search CasesFilterBar already uses, and drop the project fetch from the table's loading state entirely.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe timecards page accumates project names from loaded tab data and uses an asynchronous Project filter. Project-directory loading no longer gates table loading across Mine, All, and Approvals. ChangesTimecard project filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx`:
- Around line 104-113: Update CsmTimeCardsPage and projectNameSeedFrom so
selected project names persist at page/filter scope instead of being derived
only from the currently loaded tab cards. Reuse or merge a persistent
projectId-to-projectName cache across tab changes and empty result pages, and
pass that cache to every conditional FilterBar/AsyncProjectMultiSelect instance
so chips retain names after remounts. Add coverage for selecting a project on
All, switching to Mine or Approvals with no matching cards, and rendering the
selected chip without an async rediscovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1260485d-3000-45aa-95f0-18cf205c29be
📒 Files selected for processing (1)
apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx
… switches Each tab's FilterBar/AsyncProjectMultiSelect instance unmounts on tab switch (conditional rendering per tab), which dropped a selected project's name the moment the newly active tab's own loaded cards didn't happen to include it (or were empty) -- the chip fell back to a raw id until the dropdown was reopened and re-searched. Replace the three per-tab name derivations with one page-level cache that accumulates project names across every tab's cards over the page's lifetime and never shrinks, shared by all three FilterBar instances.
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid suppressing
set-state-in-effectfor derived cache state.Reconcile the project-name cache during render (or otherwise express it as a derivation/persistence store) instead of mutating it from this effect. That removes the lint suppression and avoids an extra render after data changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx` at line 17, Update the project-name cache logic in CsmTimeCardsPage to reconcile derived cache state during render, or represent it through a derivation/persistence store, rather than calling setState from the effect. Remove the set-state-in-effect lint suppression while preserving the cache’s behavior when project data changes and avoiding an extra render.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx`:
- Line 17: Update the project-name cache logic in CsmTimeCardsPage to reconcile
derived cache state during render, or represent it through a
derivation/persistence store, rather than calling setState from the effect.
Remove the set-state-in-effect lint suppression while preserving the cache’s
behavior when project data changes and avoiding an extra render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bd820d1-3629-4eb3-b1a0-c65202b306a8
📒 Files selected for processing (1)
apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx
…render Replace the useEffect + setState (with a set-state-in-effect lint suppression) with React's documented previous-renders pattern: a ref tracks the last-seen query data, and the cache only updates during render when it actually changed. Same accumulating behavior, but the new name is available in the same render the data arrived in instead of a render later, and no lint suppression is needed.
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx`:
- Around line 344-368: Replace the render-time writes to lastSeenCardsRef in
CsmTimeCardsPage with useState-managed last-seen card data, initializing it
lazily and updating it idempotently during the existing comparison flow.
Preserve the current mine/all/queue identity checks and projectNameCache
synchronization while ensuring React Strict Mode double-rendering does not cause
an uncontrolled mutation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c96a7998-3006-4143-ab25-54e86b82c954
📒 Files selected for processing (1)
apps/csm-portal/webapp/src/features/csm-timecards/pages/CsmTimeCardsPage.tsx
… a ref A ref mutated during render isn't reset between React Strict Mode's double-invocation of a render pass, so the second invocation would see the ref already updated and silently skip reconciling the project-name cache. useState's set function is safe here -- calling it twice with the same target value during the double-invoke is a no-op bail-out -- matching React's own documented pattern for this exact "compare against last-seen data" case.
cloby99
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The Time Cards page's table stayed stuck on its loading spinner until the entire project catalogue had loaded, even after the actual time-card search had already resolved — because the Project filter dropdown was fed by an eager, all-pages fetch whose
isLoadingwas folded into the table's own loading state.Goals
Approach
SearchableMultiSelect+useProjectOptions()(blocking, sequential all-pages fetch) combo withAsyncProjectMultiSelect, the same lazy/scroll-paginated project searchCasesFilterBaralready uses elsewhere in the portal — first page loads on dropdown-open, further pages load only as the user scrolls, and typing narrows the search server-side.|| projects.isLoadingfrom the three tabs'isLoadingexpressions so the table's loading state only reflects the actual time-card query.projectNameSeedFromhelper (mirrors the existingengineerOptionsFrom/workItemOptionsFromhelpers in the same file) so an already-selected project chip still shows its real name immediately, seeded from whatever cards are already loaded, instead of a raw id while the async search catches up.User stories
As an engineer or approver, I can open Time Cards and see my time cards immediately, without waiting on an unrelated project-list fetch to finish first.
Release note
Fixed: the Time Cards page could appear stuck loading when the project catalogue was large or slow to fetch, even though the time-card data itself was already ready.
Documentation
N/A — internal CSM portal UI, no external doc surface affected.
Automation tests
main— inCaseActivitiesFeed.test.tsx,CaseActionBar.test.tsx,CsmAnnouncementsPage.test.tsx; none touch Time Cards).tsc -bandvite buildboth pass clean.Security checks
Test environment
pnpm run lint,pnpm run test,pnpm run buildall passing locally (via the./node_modules/.bin/workaround for the localpnpmCLI issue).Summary by CodeRabbit