feat(frontend): coverage — what the product can see about a person, and about how many people - #2410
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR replaces the coverage radar with coverage-level analysis. It adds metric reachability and scope aggregation, updates person-section semantics, and renders expandable coverage details. Portal labels and configuration now use “What we can see.” Bug-report image instructions now use manual attachment. ChangesCoverage levels
Bug insight documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DomainLensView
participant useScopeCoverage
participant MetricDefinitions
participant MetricCollections
participant CoverageUtilities
DomainLensView->>useScopeCoverage: selected member IDs
useScopeCoverage->>MetricDefinitions: load definitions
useScopeCoverage->>MetricCollections: request period-only collections
useScopeCoverage->>CoverageUtilities: compute coverage results
CoverageUtilities-->>useScopeCoverage: person and aggregate coverage
useScopeCoverage-->>DomainLensView: render coverage levels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
added 9 commits
August 10, 2026 19:18
The judgement before every other one: is there anything here to read? Three states, not two — a part with no value is either a source that never reaches us or a person who did none of that work, and collapsing them loses the only distinction separating "connect this" from "nothing happened". Reachability comes from the tenant-wide definition listing (last_observed_date, schema_status, is_enabled), never from nobody in view having a value. A viewer whose visible set is small may see no user of a system that is connected and busy elsewhere, and would report a live connector as missing — more often the smaller their reach. That is the same shape of error as a statistic drawn from a truncated pool, and it is avoided rather than mitigated: none of the three judgements here is a population statistic. coverageDistribution reports how many people it counted. Not decoration: the same distribution is a true statement about the people counted and a false one about the organisation, and that number is the whole difference. unreachableParts offers no estimate of who connecting a source would reveal. The people doing that work are invisible precisely because it is missing, so any such number would be invented; the return shape has nowhere to put one. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Both ways this can go wrong are in the request, not the answer, so both are tested there. The visibility check on the metrics endpoint is all-or-nothing: one id outside the caller's visible set refuses the whole request rather than filtering it, and does not say which id was at fault. The id list is therefore the tree identity served, exactly — never widened, never guessed. Only the period view is requested. entityChunkSize refuses to chunk a collection carrying timeseries, breakdown or histogram views, and an unchunked roster-sized request meets the backend's projected-row limit; asking for the one view this needs keeps the existing chunk-and-merge path available at roster scale. Reachability rides the definition listing's existing query key rather than issuing a second request. It is a different question from the availability gate next to it: that one asks which metrics may be requested, this asks which have ever answered, and the gap between them is exactly the nulls that must read as "no data reaches us" rather than as an idle person. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The question that comes before every value on every other tab: is there anything here to read? A distribution of people by how many parts of their work reach us, and the parts nothing reaches at all. Distinct from the Health radar next to it, which reports per domain what share of people show activity there. That cannot tell whether the same people are covered everywhere or different people each time — and it is that difference, not the per-domain share, which decides whether anything compound may be built on top. It also has two states where this has three: a domain reading zero is either unconnected or idle, and the radar cannot say which. The section fetches its own period-only collection rather than widening the zone grid: coverage asks whether ANY metric of a group reads, so it needs every group's keys, and adding those to the shared grid would make one tab pay for all of them. The counted-people line under the bars is load-bearing, not a caption. The same distribution is a true statement about the people counted and a false one about the organisation, and that line is the whole difference. Placed after the value tabs in the nav on purpose: a reader reaches for it when a number looks thin. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…the scale Two halves of one question, so one tab. The radar answers it per domain: what share of people show activity in each. The levels answer it per person: how many domains we can see for each of them. Neither implies the other, which is the reason to show both. Five domains at 60% is one situation if it is the same people every time and a very different one if it is not — everyone thinly covered versus most people covered well and some not at all. The radar renders those identically; only the per-person distribution separates them, and which of the two is true decides whether anything compound may be built on top. The bars are unreadable without saying what the scale is, so the parts are named in place and "4 of 5" is spelled out — including that it says nothing about how well the person did in any of them. It is the first question anyone asks and the answer is one sentence. Also exempts a coverage lens from the never-observed gate. That gate shows "source isn't ingested for this org yet" when no metric of the lens was ever observed — which on the screen built to report which sources are not ingested would withhold the answer exactly when it is worth most. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…cale The section described a shape and left the reader to work out why they should care. A distribution is not a finding: what a reader needs first is whether the rest of the product can bear weight, and for whom it cannot. So it now opens with how many people are seen in fewer than half of their work, and what that means — every metric, comparison and flag about those people rests on that fraction of what they do. The scale follows, since "4 of 5" only needs explaining once there is a reason to read it. "Fewer than half" rather than a tuned threshold: it needs no defending and holds its meaning when the number of parts changes, which a fixed count would not. With an odd number of parts the midpoint is not a level anyone can sit at, so the boundary is unambiguous; with an even one, exactly half counts as covered, since drawing it into the thin group would overstate the problem. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…people The tab carried two counts of the same thing. The radar measured coverage over each group's card-preview keys off the zone grid; the per-person block measured it over every key of every group from its own fetch. They agreed on the data in front of me and had no reason to keep agreeing, and a reader who catches two numbers for one fact is right to stop trusting both. Now the by-part cut is derived from the same per-person states as the distribution, with a test that their totals must match. Read top to bottom it is an argument rather than two charts: how many people we cannot speak confidently about, then which parts are missing, then how thinly people are seen. The verdict is a number rather than a sentence because it is meant to be seen and not parsed, and the amber blocks in the last cut ARE the number at the top — the shape reads without the labels. A part nothing reaches is not drawn as a bar at zero. Zero says people did none of that work, which is the one thing it does not mean, so it says so in words instead. Same reason the radar could not carry this: an axis has one position for two different facts. The prose that used to explain the screen is gone. Two rounds of feedback said the meaning did not land, and both times I added text; text was the problem. What survives is one line of fine print, below everything, for the reader who wants the rule rather than the finding. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The headline counts the rows below it and nothing said so. Asked where the number came from, I could answer by adding three row values — which means the screen was making the reader do that, and a reader who has to work it out is in the same position as one who does not know it. Two changes, no extra prose. The number takes the colour of the rows it sums, so the eye makes the link unaided. And the rule that defines it — fewer than half the parts — is drawn as a line where it actually falls in the list, with the tally on it, so the boundary is visible rather than inferred from which bars happen to be amber. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
A level says how much of somebody we cannot see. What a reader does next is find out who, so the rows open into the people at that level, each linking to their page. The names are not the point — the missing parts are, and they are split by which kind of absence applies. "No connector" is somebody's job to fix. "Nothing recorded" is a person who does that work elsewhere or does not do it, and no amount of plumbing changes it. Those lead to different places, which is why the three-state model exists at all; this is the first surface where the difference does any work. A level nobody is at cannot be opened, rather than opening onto nothing. The component test needed the set form of the metric-results query and the definition listing, which this file did not mock before — the coverage section fetches its own period-only collection instead of riding the zone grid, and reads reachability from the tenant listing. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…ant, not the pool A person's page said "no data reaches us" for a section whose comparison pool held no readings. The pool is whoever the viewer can see, so a viewer with a narrow reach would be told a live connector is missing — and the narrower the reach, the more often. It is the same shape of error as a statistic drawn from a truncated pool, and it produced the more damaging direction: telling somebody their tracker is broken while their colleagues' numbers sit on the next screen. Availability is a property of the tenant, so it now comes from the tenant's definition listing — the one place that holds it, and the same place the org-wide coverage screen reads. Before this the two surfaces answered one question two ways and were free to disagree about the same person on the same day. groupPeersHaveData is deleted rather than left for a future caller: it is the inference, and leaving it available is leaving the bug available. The old test asserted the pool rule, so it now asserts the listing rule, with a companion for the case that used to be wrong — an empty pool no longer means anything at all. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
dzarlax
force-pushed
the
feat/coverage-readout
branch
from
August 10, 2026 17:18
8b582ad to
1765771
Compare
…about people Review findings, and the two serious ones are the same mistake: this screen guards carefully against a biased inference and did not guard at all against a request that never answered. A failed definition listing left the reachable set empty, so every part read "no data reaches us" for every person and the headline announced that everyone is thinly seen. A failed metrics chunk did the same by dropping entities. Both printed at full confidence the moment the query settled. The hook now carries isError and the section says it could not check, claiming nothing about anyone. The person page had the pending half of the same hole: the definition listing's loading state was never folded into the section's, so between the metrics answering and the listing answering the page said nothing reaches us about any part of that person, then flipped. The old pool inference read the query whose pending state was already tracked, so this arrived with the new source. Custom metrics were being reported as unreachable. The listing warns in as many words that it stamps freshness from materialized relations only, so a custom metric's last_observed_date stays absent however much data it serves — and the frontend type did not carry `origin` at all, so the check could not be made. Latent today since every compiled-in key is builtin, but latent inside the one function whose whole job is not to fabricate absence. Also: the hook returned a second, unused way of computing unreachability — in the PR that removed the radar for computing coverage a second way. Deleted. `unlinked` documented a protection no code path provided, with a test pinning it; the parameter is gone rather than left describing something untrue. The unreachable-part copy no longer names a connector as the cause, since a disabled or schema-broken metric lands there too and sending someone to plumb a live connector is the wrong direction to be wrong in. The amber headline gained its light-theme pair, which its own comment had claimed it already had. The disclosure rows gained aria-controls and the list an id. And the sentence about the top of the scale is rewritten — a careful reader took it as a contradiction, which is the only test that phrasing has. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…t/coverage-readout
Updating this branch from main pulled in three unrelated documentation commits touching an agent-tooling skill file. They have nothing to do with coverage and agent tooling does not belong in an upstream pull request, so the file is restored to what main carries. The commits themselves are untouched wherever else they live; only this branch's diff is narrowed back to the change under review. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
dzarlax
enabled auto-merge
August 11, 2026 08:22
hello1101n
approved these changes
Aug 11, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/frontend/src/components/portal/domain-lens-view.tsx (2)
690-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
idis shadowed inside the row loop.The component takes an
idprop and applies it to the<ul>on Line 690. The loop on Line 694 binds a secondidfor the group key. The current code is correct, but the two meanings ofidin one function invite a wrong reference during later edits.♻️ Proposed rename
- for (const [id, state] of p.states) { - const title = titleById.get(id) ?? id; + for (const [groupId, state] of p.states) { + const title = titleById.get(groupId) ?? groupId;🤖 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 `@src/frontend/src/components/portal/domain-lens-view.tsx` around lines 690 - 698, Rename the inner `id` binding in the `rows.map` state loop to a distinct name such as `groupId`, and update its uses when retrieving `titleById` and constructing the fallback title. Preserve the component `id` prop used by the `<ul>` unchanged.
528-535: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe error state offers no retry.
CoverageLevelsSectionrenders the failure withPending, which is the loading component. The reader gets no way to retry. Every other failing section in this file passes a refetch callback, for examplecompRefetchinCompositionSection.useScopeCoverageexposesisErrorbut norefetch, so no retry is possible today.Expose a
refetchfromuseScopeCoveragethat fans out to the collection-set and definitions queries, then render the failure with the same error component the other sections use.🤖 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 `@src/frontend/src/components/portal/domain-lens-view.tsx` around lines 528 - 535, Update useScopeCoverage to expose a refetch function that triggers both the collection-set and definitions queries, then update CoverageLevelsSection’s isError branch to use the established error component with that refetch callback instead of Pending. Match the retry behavior and error rendering used by sections such as CompositionSection and compRefetch.src/frontend/src/components/portal/domain-lens-view.test.tsx (2)
79-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe coverage error path has no test.
The
useMetricDefinitionsResponsemock omitsisError, and eachcollectionSetentry omitsisError. The hook reads both fields, so both resolve toundefinedand the error branch never runs.CoverageLevelsSectionLine 533 renders "Could not read coverage — the check did not complete, so nothing is claimed about anyone." This branch is a stated safety property of the PR: it stops an infrastructure fault from reading as a verdict about named people. It is currently unverified at the component level.Add
isErrorto both mocks, then add a test that sets it and asserts the failure text.Also applies to: 453-484
🤖 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 `@src/frontend/src/components/portal/domain-lens-view.test.tsx` around lines 79 - 84, Add isError fields to the useMetricDefinitionsResponse mock and each collectionSet entry, then add a component test that sets the relevant error state and asserts CoverageLevelsSection renders “Could not read coverage — the check did not complete, so nothing is claimed about anyone.”Source: Coding guidelines
486-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the part count from
GROUPSinstead of hardcoding 5.
coverageWorldbuilds the fixture fromGROUPS, but the assertions hardcode/1 of 5/and/5 of 5/. The rendered label is{level} of {GROUPS.length}. If a group is added or removed, both tests fail with a "unable to find role button" message that does not name the cause.🧪 Proposed change
- await userEvent.click(screen.getByRole("button", { name: /1 of 5/ })); + const parts = GROUPS.length; + await userEvent.click( + screen.getByRole("button", { name: new RegExp(`1 of ${parts}`) }), + );- expect(screen.getByRole("button", { name: /5 of 5/ })).toBeDisabled(); + expect( + screen.getByRole("button", { + name: new RegExp(`${GROUPS.length} of ${GROUPS.length}`), + }), + ).toBeDisabled();🤖 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 `@src/frontend/src/components/portal/domain-lens-view.test.tsx` around lines 486 - 513, Update the two button-label assertions in the coverage-level tests to derive the total from GROUPS.length instead of hardcoding 5: use the existing level values with GROUPS.length for both the clickable and disabled button queries. Keep the current test behavior and coverageWorld setup unchanged.src/frontend/src/lib/portal/use-scope-coverage.test.tsx (1)
99-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an
isPendingassertion for the empty scope.This test verifies the request only. It does not assert the returned state. With an empty roster the hook returns
isPending: truepermanently, becausedata.get(def.id)?.isPending ?? truehas no map entry to read. The consumer then renders a permanent loading label. An assertion here would fix the gap in the hook.🧪 Proposed test addition
expect(state.lastCall?.entity.ids).toEqual([]); expect(state.lastCall?.collections).toEqual([]); + const { result } = renderHook(() => useScopeCoverage(state.members)); + expect(result.current.isPending).toBe(false); + expect(result.current.distribution.counted).toBe(0); });As per coding guidelines: "Ensure new and changed lines achieve at least 80% test coverage."
🤖 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 `@src/frontend/src/lib/portal/use-scope-coverage.test.tsx` around lines 99 - 106, Extend the empty-scope test around useScopeCoverage to capture the hook result and assert that isPending is false once the scope has no members. Update the hook’s pending-state logic to handle an empty roster without relying on a missing data map entry, while preserving pending behavior for unresolved non-empty scopes.Source: Coding guidelines
🤖 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 @.claude/skills/file-bug-insight/SKILL.md:
- Line 222: Update the image-attachment guidance near the “Images — the honest
constraint” section to provide an executable manual step: for PNGs intended for
the issue body, tell the user to open Edit, drag them into the body editor, and
save; otherwise direct them to attach the PNGs in the issue comment box.
In `@src/frontend/src/lib/portal/use-person-sections.test.tsx`:
- Around line 17-28: Update the useMetricDefinitionsResponse mock in the test
setup to return mocks.isPending instead of a hardcoded false, then add a
regression test covering the definitions.isPending branch in usePersonSections.
Ensure the new test exercises the pending definition-query behavior and
maintains the required coverage for changed lines.
In `@src/frontend/src/lib/portal/use-scope-coverage.ts`:
- Around line 107-109: Update the useScopeCoverage hook so isPending is false
when rosterIds.length === 0, before evaluating group pending states. In
src/frontend/src/lib/portal/use-scope-coverage.ts#L107-L109, apply this change;
in src/frontend/src/lib/portal/use-scope-coverage.test.tsx#L99-L106, extend the
empty-roster test to assert isPending is false and distribution.counted is 0.
---
Nitpick comments:
In `@src/frontend/src/components/portal/domain-lens-view.test.tsx`:
- Around line 79-84: Add isError fields to the useMetricDefinitionsResponse mock
and each collectionSet entry, then add a component test that sets the relevant
error state and asserts CoverageLevelsSection renders “Could not read coverage —
the check did not complete, so nothing is claimed about anyone.”
- Around line 486-513: Update the two button-label assertions in the
coverage-level tests to derive the total from GROUPS.length instead of
hardcoding 5: use the existing level values with GROUPS.length for both the
clickable and disabled button queries. Keep the current test behavior and
coverageWorld setup unchanged.
In `@src/frontend/src/components/portal/domain-lens-view.tsx`:
- Around line 690-698: Rename the inner `id` binding in the `rows.map` state
loop to a distinct name such as `groupId`, and update its uses when retrieving
`titleById` and constructing the fallback title. Preserve the component `id`
prop used by the `<ul>` unchanged.
- Around line 528-535: Update useScopeCoverage to expose a refetch function that
triggers both the collection-set and definitions queries, then update
CoverageLevelsSection’s isError branch to use the established error component
with that refetch callback instead of Pending. Match the retry behavior and
error rendering used by sections such as CompositionSection and compRefetch.
In `@src/frontend/src/lib/portal/use-scope-coverage.test.tsx`:
- Around line 99-106: Extend the empty-scope test around useScopeCoverage to
capture the hook result and assert that isPending is false once the scope has no
members. Update the hook’s pending-state logic to handle an empty roster without
relying on a missing data map entry, while preserving pending behavior for
unresolved non-empty scopes.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c6c2d36-8626-49e1-b6e7-5190316898e1
📒 Files selected for processing (20)
.claude/skills/file-bug-insight/SKILL.mdsrc/frontend/src/api/metric-definitions-client.test.tssrc/frontend/src/api/metric-definitions-client.tssrc/frontend/src/components/portal/context-pane.test.tsxsrc/frontend/src/components/portal/domain-lens-view.test.tsxsrc/frontend/src/components/portal/domain-lens-view.tsxsrc/frontend/src/components/portal/metric-groups-view.test.tsxsrc/frontend/src/lib/insight/coverage.test.tssrc/frontend/src/lib/insight/coverage.tssrc/frontend/src/lib/insight/group-data.tssrc/frontend/src/lib/portal/lens-configs.test.tssrc/frontend/src/lib/portal/lens-configs.tssrc/frontend/src/lib/portal/nav-model.tssrc/frontend/src/lib/portal/overview-configs.tssrc/frontend/src/lib/portal/use-person-sections.test.tsxsrc/frontend/src/lib/portal/use-person-sections.tssrc/frontend/src/lib/portal/use-scope-coverage.test.tsxsrc/frontend/src/lib/portal/use-scope-coverage.tssrc/frontend/src/queries/metric-definitions.test.tssrc/frontend/src/screens/metric-definitions.test.tsx
💤 Files with no reviewable changes (2)
- src/frontend/src/lib/portal/lens-configs.test.ts
- src/frontend/src/lib/insight/group-data.ts
dzarlax
disabled auto-merge
August 11, 2026 08:27
added 3 commits
August 11, 2026 10:30
Review findings. With no members the hook sends no collections, so no group ever gets an entry and `data.get(id)?.isPending ?? true` held every one of them pending for good. The section sat on its loading label permanently instead of saying there is nobody in this scope. Pending now only considers the groups when there is somebody to fetch for. The person-page test mocked the definition listing as never pending, so the branch added for its loading window could not be exercised at all — the fix shipped without a test that could fail if it regressed. The mock is configurable now and a case covers it: while the listing loads, no section claims anything. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…t/coverage-readout
Updating this branch from main left four files at an older revision than upstream carries — an agent-tooling doc and three CI files. This restores them, so the diff under review is the frontend change and nothing else. An earlier commit on this branch explained the same restore by saying the fork's main carries commits upstream does not. That was wrong: the fork's main held no commit of its own, it was simply far behind, and merging it left the branch with stale copies. The commits in question are upstream's own. The action was right either way; the reasoning was not, and it is corrected here rather than left in the history for a reader to trust. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
dzarlax
force-pushed
the
feat/coverage-readout
branch
from
August 11, 2026 08:38
78a13d0 to
79534c1
Compare
dzarlax
enabled auto-merge
August 11, 2026 08:55
hello1101n
approved these changes
Aug 11, 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
2 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.
Phase 1 of #2408 — the question that comes before every value on every other screen: is there anything here to read? Frontend only; no service change is required for any of it.
What it adds
A coverage model. For one person in one period, each part of their work is in one of three states: it reads, nothing was recorded, or no data reaches us. Three rather than two, because a part with no value is either a source that never arrives or a person who did none of that work, and collapsing them loses the only distinction separating "connect this" from "nothing happened".
An Overview tab built from that model as one argument read top to bottom:
A level opens into the people at it, each linking to their page, with their missing parts split by which kind of absence applies. That split is the point: one is somebody's job to fix, the other is a person who does that work elsewhere or does not do it, and no amount of plumbing changes it.
Two bugs it fixes
A person's page called a section unmeasured when the comparison pool was empty. The pool is whoever the viewer can see, so a viewer with a narrow reach was told a live connector is missing — and the narrower the reach, the more often. It is the same shape of error as a statistic drawn from a truncated pool, in the more damaging direction: it tells somebody their tracker is broken while their colleagues' numbers sit on the next screen. Availability is a property of the tenant, so it now comes from the tenant's definition listing.
groupPeersHaveDatais deleted rather than left for a future caller — it is the inference, and leaving it available leaves the bug available.The tab carried two counts of one thing. The radar measured coverage over each group's card-preview keys off the zone grid; the per-person block measured it over every key of every group from its own fetch. They agreed on the data in front of me and had no reason to keep agreeing, and a reader who catches two numbers for one fact is right to stop trusting both. The by-part cut is now derived from the same per-person states as the distribution, with a test that their totals must match.
Things worth knowing for review
/v1/metric-resultsis all-or-nothing. One id outside the caller's visible set refuses the whole request rather than filtering it, and does not disclose which id was at fault. The id list is therefore the roster the portal was served, exactly — never widened, never guessed. There is a test on the request rather than the answer, because this failure is invisible in the returned shape.periodview is requested.entityChunkSizewill not chunk a collection carrying a timeseries, breakdown or histogram view, and an unchunked roster-sized request meets the projected-row limit. Asking for the one view this needs keeps the existing chunk-and-merge path available. A richer collection here would work at team scale and fail at organisation scale — the failure appears only for a reader with wide visibility, who is both the most important one and the last to be tested against.countedis not optional.Deliberately not here
Checks
tsc -bclean, eslint clean, 843 unit tests pass. Verified live against a populated instance, including the level drill-down and the empty-level case.Refs #2408, #2391, #2028
Summary by CodeRabbit
New Features
Improvements