LPA Dashboard Speedup, issue count bug and unnecessary function calls - #1135
Conversation
WalkthroughThe middleware simplifies the LPA overview data-fetching pipeline by removing unused resource and issue-count fetches (fetchResources, fetchEntryIssueCounts, fetchEntityCounts), eliminating a dependent second parallel execution block, and streamlining the issues aggregation logic to use only entity-level counts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Req as Request
participant MW as lpa-overview.middleware
participant Data as Data services
Note over Req,MW: Previous flow (simplified view)
Req->>MW: enter middleware
MW->>Data: parallel(fetchResources, fetchEntryIssueCounts, otherFetches)
MW->>Data: parallel(fetchEntityCounts, dependentFetches)
Data-->>MW: resources + entryIssueCounts + entityIssueCounts
MW-->>Req: aggregated response
Note over Req,MW: New flow
Req->>MW: enter middleware
MW->>Data: fetch only essential datasets & entity-level counts
Data-->>MW: entityIssueCounts + otherEssentialData
MW->>MW: groupIssuesCountsByDataset(entityIssueCounts)
MW-->>Req: aggregated response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
pooleycodes
changed the title
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/middleware/lpa-overview.middleware.js (3)
65-75: Clarify or retiredatasetSubmissionDeadlineCheckto avoid accidental useYou’ve correctly documented that this middleware is broken and unused, but it’s still exported and contains a real bug (calling
next(error)without returning, then continuing to compute and callingnext()again). Consider either deleting it or at least removing the export / adding a defensivereturn next(error)so it can’t cause subtle issues if re‑wired in later.
117-130: UnusedgroupResourcesByDatasetis fragile if re‑enabled
groupResourcesByDatasetis marked “Not used, fix or delete” but still exported and assumesreq.resourcesis a defined array (resources.reduce(...)will throw otherwise). Either remove it entirely, or guard it with a simpleif (!Array.isArray(resources)) return next(new Error(...))before the reduce, so that any future re‑use fails safely.
316-325: Using onlyentityIssueCountsfixes double‑counting; consider a small robustness guardSwitching
groupIssuesCountsByDatasetto aggregate solely fromentityIssueCountsaligns with the PR goal of eliminating duplicated issue counts and keepsreq.issuesin the shape expected byprepareDatasetObjects. One small hardening you might consider is:-export function groupIssuesCountsByDataset (req, res, next) { - const { entityIssueCounts } = req - - req.issues = entityIssueCounts.reduce((acc, current) => { +export function groupIssuesCountsByDataset (req, res, next) { + const { entityIssueCounts = [] } = req + + req.issues = entityIssueCounts.reduce((acc, current) => {so a missing or undefined
entityIssueCountsdoesn’t cause a runtime error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/middleware/lpa-overview.middleware.js(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-14T17:31:37.924Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: src/services/performanceDbApi.js:290-293
Timestamp: 2024-11-14T17:31:37.924Z
Learning: In the `getEntityCounts` function in `src/services/performanceDbApi.js`, errors are already logged through the `.catch(...)` handlers on each promise in the `requests` array.
Applied to files:
src/middleware/lpa-overview.middleware.js
📚 Learning: 2024-11-14T16:38:49.883Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
Applied to files:
src/middleware/lpa-overview.middleware.js
🧬 Code graph analysis (1)
src/middleware/lpa-overview.middleware.js (2)
src/services/performanceDbApi.js (1)
req(436-436)src/middleware/common.middleware.js (15)
req(121-121)req(122-122)req(141-141)req(142-142)req(181-181)req(222-222)req(261-261)req(286-286)req(299-299)req(378-378)req(439-439)req(509-509)req(567-567)req(574-574)req(789-789)
🔇 Additional comments (2)
src/middleware/lpa-overview.middleware.js (2)
7-10: Imports now match the streamlined LPA overview pipelineThe imports list is lean and consistent with the new middleware chain (no unused fetchers like
fetchResources/fetchEntryIssueCountsleft behind), which helps avoid accidental reintroduction of redundant calls.
352-372: Middleware chain simplification looks correct and should improve latencyThe new chain—single
parallelblock (org info, endpoint summary, performance‑DB entity issue counts, provisions) followed by dataset/issue grouping and template prep—removes redundant fetches and the second parallel group while keeping all data needed byprepareDatasetObjectsandprepareOverviewTemplateParams. WithdatasetSubmissionDeadlineCheckandaddNoticesToDatasetsexplicitly commented out, this should be functionally equivalent for the dashboard while avoiding the prior double‑counting bug.
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/middleware/lpa-overview.middleware.js (2)
68-115: Consider removing dead code rather than marking with TODO.The
datasetSubmissionDeadlineCheckfunction referencesreq.resources(line 69), which is no longer fetched per this PR's changes. Since the function is already commented out in the middleware chain (line 367) and marked as broken, consider removing it entirely rather than accumulating technical debt.Would you like me to open an issue to track the removal of this dead code and the related
addNoticesToDatasetsfunction (lines 143-186)?
118-130: Remove unused function that depends on removed data.The
groupResourcesByDatasetfunction processesreq.resources(line 119), which is no longer fetched. Since this function isn't used in the middleware chain and the data it depends on has been removed, delete it rather than leaving it as dead code with a TODO marker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/middleware/lpa-overview.middleware.js(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-14T17:31:37.924Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: src/services/performanceDbApi.js:290-293
Timestamp: 2024-11-14T17:31:37.924Z
Learning: In the `getEntityCounts` function in `src/services/performanceDbApi.js`, errors are already logged through the `.catch(...)` handlers on each promise in the `requests` array.
Applied to files:
src/middleware/lpa-overview.middleware.js
📚 Learning: 2024-11-14T16:38:49.883Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
Applied to files:
src/middleware/lpa-overview.middleware.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
src/middleware/lpa-overview.middleware.js (3)
8-9: LGTM: Import cleanup aligns with optimization goals.The removal of unused imports (
fetchResources,fetchEntryIssueCounts, and related fetchers) is consistent with the PR's objective to eliminate unnecessary data fetch operations.
316-328: LGTM: Bug fix correctly prevents double-counting.The simplification to use only
entityIssueCounts(removing the merge withentryIssueCounts) correctly addresses the double-counting bug described in the PR. The defensive default value of[]on line 317 ensures robustness if the fetch fails.
359-359: LGTM: Removal of second parallel block completes the optimization.Eliminating the second parallel execution block (which depended on the removed fetchers) streamlines the middleware chain and achieves the performance improvement goal stated in the PR objectives.
GitHub Actions
Failure
GitHub Actions
Inactive
2 participants
Description
Removes more unnecessary data fetch operations in dashboard.
And fixes a bug to do with fetch entry issue counts being merged with the new fetch entity issue counts function created in the dashboard speedup part 1, where duplicated errors were being counted twice for the dashboard error count.
What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Dashboard should look identical to production, but be much faster.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.