Repository navigation
feat(web,api): entity-scoped audit history reachable from any record (#493) - #516
Conversation
…cks link (#493) AuditPage now reads action/entityId from the URL (react-router useSearchParams becomes the single source of truth for both filters, replacing the old local useState), with a client-side GUID guard that falls back to the unscoped view on a malformed entityId rather than firing a request the endpoint would 400 on. Flocks gets the first "Audit history" row link; the remaining five screens land in Slice 3. Heading is naive for now (no reloading gate) — documented in-code and tracked for Slice 2, which also adds column-hiding and a distinct scoped empty-state message. es/tl translations land inline with the English keys per this project's translate-now policy. Planned via the software-factory 4-gate workflow (docs/plans/ audit-entity-history/) with a pi (local vllm) contrarian review at every gate and this slice; two real findings from the slice's own code review fixed (a weak test assertion, missing i18n marker tests), five more verified false or already-settled and rejected with evidence — full trail in 00-status.md.
…iew (#493) Gates the scoped heading on usePagedList's reloading flag: the table already blanked stale rows during a reload, but the heading was still reading rows[0] directly, so an action-filter change or an entity switch could transiently show the PREVIOUS entity's type. Also hides the now-redundant entity column when scoped (every row would repeat the same value), and gives a scoped empty state its own message instead of the global "No audit events yet.", which misleadingly read as the whole log being empty rather than this one record's clean history. 2 pi review rounds, stopped per the loop's stop rule (2 consecutive rounds with no confirmed defect in the shipped code): round 1 yielded one real, minor test-coverage gap (fixed — the column-hiding test only asserted the header, not the row's own cell); round 2's one claim was verified false against the code's own ternary structure. Full trail in 00-status.md.
Adds the "Audit history" row link to the five remaining screens the audit vocabulary covers: Egg Grades, Sales Orders, Expenses, Daily Entries (HistoryPage), and Egg Lots (StockPage). Mechanical repetition of Slice 1's FlocksPage pattern for the first four. StockPage is the one exception: it already has a "history"/"hide history" toggle on the same row for the inventory MOVEMENT ledger, a different and older trail. The new link is a real navigation (<a href>) next to that in-place toggle (<button>), with a test proving they're behaviorally distinct, not just differently labeled — the link never opens the ledger, the toggle never navigates. 2 pi review rounds, both clean (zero findings), stopped per the loop's stop rule. Round 2 was pointed at a different angle (accessibility, i18n interpolation, id-field naming) rather than repeating round 1's checks. Full trail in 00-status.md.
) Covers switching to a different record's audit history while already on /audit, without a remount — the primary record-to-record browsing flow, and the one path Gate 3's original test plan never covered (found reviewing the design docs, not the shipped code). Two tests: the switch re-fires the fetch and updates the heading rather than staying stale on the old record, and the heading falls back to the generic label while the switch is in flight rather than showing the previous record's type. Test-only; no production code changed (the mechanism was already built and verified in Slices 1-2). 2 pi review rounds. Round 1 found two real gaps in the test harness itself: a sibling-Link-with-no-Routes setup couldn't actually prove AuditPage stays mounted across the navigation, and the in-flight test could pass without the switch ever really re-fetching. Both fixed — harness now uses a real <Routes> tree matching App.tsx's own routing, and the in-flight test asserts the fetch actually fired. One claim verified false and rejected (fixture emails were checked and are genuinely distinct). Round 2 clean. Full trail in 00-status.md.
Adds one Serilog diagnostic-context enrichment to the existing request-completion logging: whether GET /api/v1/audit was called with an entityId, on a successful (2xx) response. This is the ticket's success metric — is the new per-record link actually being exercised — and the one non-SPA line in an otherwise SPA-only feature; the endpoint and its entityId filter already existed and needed no change. Range check (2xx), not an exact 200, so the metric doesn't silently break on an unrelated future change to the endpoint's status code. Known, accepted limitation, pinned by its own test rather than left implicit: a syntactically valid but non-matching entityId (or a cross-tenant read) still counts as a "successful scoped read" even with zero rows — fixing that would need response-body inspection, disproportionate for a one-line log enrichment. 2 pi review rounds, stopped per the loop's stop rule. Round 1's real yield was one minor gap (the entityId= empty-value case, distinct from both malformed and absent, was untested — added, confirmed it 400s via the same Guid? binding failure as the malformed case). Three other claims were checked against Serilog.AspNetCore's actual completion-logging architecture and xUnit's documented collection semantics and rejected as false. Round 2 clean. Full trail in 00-status.md.
Adds the AGENTS.md-required doc sync for this feature: a new Help page bullet (auditRecordHistoryLink, es/tl translated inline per the translate-now policy) alongside the existing audit bullets, and a new GLOSSARY.md entry explaining how the per-record link relates to both the global Audit log (#93) and the record-history summary column (#494) it sits next to. 2 pi review rounds, stopped per the loop's stop rule. Round 1 found two real copy issues: the Spanish translation put "Lotes" (this app's established term for Flock) in the same sentence as "Lotes de huevos" (Egg lots), genuinely ambiguous on a skim read — reordered without inventing new terminology; and "Daily entries" drifted from the established "Daily entry history" naming used one bullet above — renamed across en/es/tl to match. Two other claims were checked against the actual files (GLOSSARY.md's own preceding entry, the Spanish source text) and rejected as false. Round 2 clean. Full trail in 00-status.md. This is the final slice — all 6 shipped. Feature complete.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c3187a9f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <Link className="link" to={`/audit?entityId=${f.id}`}> | ||
| {tc("recordHistory.viewHistoryLink")} | ||
| </Link> |
There was a problem hiding this comment.
On Flocks, Stock, History, and Sales, this link is rendered for roles that can view the record but cannot access the audit endpoint: for example, a Worker can view Flocks and click this link, but /api/v1/audit is guarded by AdminOnly, so the destination only displays a 403 error. The equivalent links on Stock/History also affect ReadOnly and Worker users, and Sales affects the Sales role; gate these links with isAdmin as the navigation entry already is.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cea6a8c — gated the link with isAdmin (the same flag each page already uses for its other admin-only actions) on Flocks, History, Sales, and Egg lots (Stock). Also added Grades as defense-in-depth against direct URL access, and added Grades to your named list since its reads are open even though nav hides the route. Expenses needs no change: /api/v1/expenses is AdminOnly at the read level too, so no row (and no link) ever renders for a non-admin — verified in Program.cs.
Each page also gets a new test proving the row itself rendered before asserting the link's absence, so the assertion can't pass vacuously.
Verified: 1764/1764 web tests pass, typecheck clean.
Codex review of PR #516: /api/v1/audit is AdminOnly, but the new "Audit history" link was rendered for any role that can view the record — Worker/ReadOnly on Flocks, History, and Egg lots, and Sales on Sales orders — so clicking it only reached a 403. Gated all five with the isAdmin flag each page already uses for its other admin-only actions, plus Grades as defense-in-depth against direct URL access (its reads are open even though nav hides the route). Expenses needs no change: its endpoints are AdminOnly at the read level too, so no row — and no link — ever renders for a non-admin. Adds a "hides the link from a non-admin" test per page, each proving the row itself rendered before asserting the link's absence, so a vacuous pass (link absent because nothing rendered) can't masquerade as the real gate. 2 pi review rounds, stopped per the loop's stop rule. Round 1's real yield was two tests (Grades, Flocks) missing that row-presence proof; fixed. Five other claims — an unjustified Expenses skip, a missing route-level guard, unproven isAdmin scoping on two pages, and a Sales-role test that might pass for the wrong reason — were each checked against the actual backend policy, other admin routes, and AuthContext.tsx, and rejected as false with that evidence. Round 2 clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cea6a8c185
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const scopedEntityType = entityId && !events.reloading | ||
| ? events.rows?.[0]?.entityType | ||
| : undefined; |
There was a problem hiding this comment.
When navigation changes entityId without remounting /audit, the first render has the new query parameter but still has the previous request's rows and events.reloading === false; usePagedList only sets reloading later from its useEffect. This therefore briefly labels the new record with the previous record's entity type (and the table's identical gate can expose the previous rows), which is the stale-record flash this change intends to prevent. Verify the row's entityId matches the current scope or synchronously invalidate the old window when the fetcher identity changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ef16b9e — confirmed the race: extracted a pure isScopedDataStale(entityId, reloading, rows) that cross-checks the loaded row's own entityId against the current scope rather than trusting reloading alone, used for both the heading and the table-vs-loading branch.
Note on testability: a component-level RTL test with fireEvent can't actually observe this race (confirmed by mutation testing — a fireEvent-timed test passed even with the fix fully reverted, since act() flushes the passive effect that sets reloading before any assertion runs). Replaced with direct unit tests of the pure function instead, verified via mutation to go red on the fix's own revert.
One residual gap found and documented rather than fixed: an empty array has no row to compare against, so a stale empty page from the previous scope also reads as "not stale" for exactly one render — self-correcting once reloading catches up, unlike the non-empty case this fix closes. Explained in the code comment.
Verified: 1770/1770 web tests pass, typecheck clean.
Codex review of PR #516: usePagedList's `reloading` flag flips true inside a useEffect that runs AFTER a render commits, but `entityId` (and so `fetchPage`'s identity) already reflects a new scope on that SAME render. Trusting `reloading` alone to gate the heading/table missed that one render — switching between records without leaving /audit could briefly show the PREVIOUS entity's actual rows and heading text, not just a generic fallback. Extracts a pure isScopedDataStale(entityId, reloading, rows) that also cross-checks the loaded row's own entityId against the current scope, used for both the heading computation and the table-vs-loading render branch. A component-level RTL test can't observe this race — fireEvent's synchronous act() flushes the passive effect that sets `reloading` before any assertion runs, landing rows and reloading in the same flush a test would see. Confirmed by mutation testing: a version of this fix proven through a fireEvent-timed component test passed even with the fix fully reverted. Replaced with direct unit tests of the pure function, verified via mutation to actually go red on the fix's own revert, on the exact assertion this bug needs. 2 pi review rounds. Round 1 found a real, narrower residual: an empty array (`rows.length === 0`) has no row to compare against, so a stale empty page from the previous scope also reads as "not stale" for one render. Documented explicitly as an accepted trade-off rather than engineered around — self-correcting within a single render, unlike the non-empty case this fix closes, which could otherwise show wrong data for a whole slow request; a fully general fix needs stateful generation-tracking that would cost the function its purity. Two other claims (row-0 homogeneity, entity-column gating) were checked against the backend's exact-match query and the column's actual gate condition and rejected as false. Round 2 clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef16b9ef54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (reloading) return true; | ||
| if (entityId === undefined) return false; | ||
| if (rows === null || rows.length === 0) return false; | ||
| return rows[0].entityId !== entityId; |
There was a problem hiding this comment.
Fresh evidence after ef16b9e: when a pasted entityId contains uppercase hexadecimal characters, the case-insensitive GUID regex accepts it and the API successfully binds it, but the returned Guid is serialized in normalized lowercase. This strict comparison therefore continues returning true after the request completes, leaving the scoped page permanently on “Loading…” despite having rows; normalize the URL value or compare GUID strings case-insensitively.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 41d7d3a — real bug, confirmed. EntityId in the API response is always lowercase (a .NET Guid serialized by System.Text.Json) regardless of request casing, and the previous fix's row comparison never normalized the URL value, so an uppercase entityId got permanently stuck on "Loading…". Normalized once at the point entityId is read from the URL. Confirmed via mutation testing: the new test goes red (stuck loading, exactly the reported symptom) when the normalization is reverted.
Verified: 1772/1772 web tests pass, typecheck clean.
| <Link className="link" to={`/audit?entityId=${l.id}`}> | ||
| {tc("recordHistory.viewHistoryLink")} | ||
| </Link> |
There was a problem hiding this comment.
For a normally produced egg lot that has never been manually recounted or written off, this link opens an empty audit history rather than the promised record story. A repo-wide search shows that the only audit write whose entity is EggLot is RecordEggLotMovementHandler (EggLot.Movement); lot creation is recorded against the Daily Entry and sale allocation/restoration against the Sales Order, so filtering by l.id cannot show who created the lot or those changes. Either emit/correlate the missing lot events or avoid presenting this as the lot's full audit trail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 41d7d3a. RecordEggLotMovementHandler is the only writer of EntityType="EggLot" audit events — creation lives on the Daily Entry, allocation/restoration on the Sales Order. Relabeled the link to "Adjustment history" (new i18n key, en/es/tl, StockPage only — the other five screens keep "Audit history", which is accurate for them per #494's Create-event work) and corrected the code comment, GLOSSARY.md, and the Help page bullet to describe this honestly rather than claiming full lifecycle coverage.
Verified: 1772/1772 web tests pass, typecheck clean.
…story (#493) Codex round 3, two findings against the just-shipped stale-scope fix and the original Egg Lots link. 1. A pasted/hand-typed uppercase entityId is syntactically valid (the guard regex is case-insensitive) and the API accepts it, but its response always serializes EntityId lowercase (a .NET Guid via System.Text.Json). The previous fix's row-comparison never matched against an un-normalized uppercase URL value, so the page got stuck on "Loading…" forever — a regression introduced by that fix, not a pre-existing bug. Fixed by normalizing entityId to lowercase once, at the point it's read from the URL, so the API call and the comparison both agree. 2. StockPage's "Audit history" link for Egg Lots promised a full trail, but the only audit action ever written against an EggLot's own entity id is a manual write-off/recount (RecordEggLotMovementHandler) — creation is recorded against the Daily Entry, allocation/restoration against the Sales Order, never the lot itself. A normal, never-adjusted lot showed nothing at all. Relabeled to "Adjustment history" (new i18n key, en/es/tl, StockPage only) and corrected the code comment, GLOSSARY.md, and the Help page bullet to describe this narrower scope honestly instead of claiming lifecycle coverage the data can't back up. 2 pi review rounds. Round 1's real yield was a doc-consistency nit (a code comment dropped "restoration" that the i18n comment and GLOSSARY.md both include). Two more substantial-sounding claims — the URL itself staying un-normalized, and unupdated Playwright specs still expecting the old label — were checked directly: grepped every entityId consumer in web/src/routes (only AuditPage ever reads it back, always freshly normalized per render) and grepped tools/simulation/ui/ for both labels (zero matches — the label was born in this PR). Both false. Round 2 clean, one aside dismissed after checking it against the actual test code (it described a method this file doesn't call). Verified: 1772/1772 web tests pass, typecheck clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41d7d3a7c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rows: { entityId: string }[] | null, | ||
| ): boolean { | ||
| if (reloading) return true; | ||
| if (entityId === undefined) return false; |
There was a problem hiding this comment.
When an admin opens a scoped audit URL and then clicks the sidebar's /audit link, React Router keeps the same AuditPage instance while entityId changes to undefined. This branch declares the existing scoped rows current before usePagedList's effect can set reloading, so the global heading and entity column briefly render only the previous record's events as though they were the global log. Treat a scoped-to-unscoped transition as stale too, rather than unconditionally accepting rows whenever entityId is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in eb702eb. This was the third shape of the same underlying bug — a content-based check (comparing loaded rows' own entityId against the current scope) had already missed a stale-empty-page case in an earlier round, and this scoped-to-unscoped transition is the same shape again. Redesigned around comparing fetchPage's own reference identity instead of inferring staleness from row content: a ref tracks the fetchPage that was current the last time a reload completed, and rows are stale whenever the current fetchPage differs from that — closes every variant uniformly, including this one, since it never looks at content at all.
Verified: 1770/1770 web tests pass, typecheck clean.
|
|
||
| ## Announcement | ||
|
|
||
| Every record now remembers its own story. Open a Flock, a Sales Order, an Expense, an Egg Grade, a Daily Entry, or an Egg Lot, and you'll find a "View history" link that takes you straight to everything that ever happened to it — who created it, who changed it, and when, in order, filterable by what kind of change you're looking for. No more digging through the global Audit feed hoping you land on the right row. |
There was a problem hiding this comment.
After the 41d7d3a correction, this announcement still promises that an Egg Lot's link shows everything that happened to it, including creation and changes. The shipped Stock page now deliberately labels it “Adjustment history” because the scoped query contains only manual write-off/recount events; creation and sale allocation are audited against other entities. Update this announcement and the remaining plan references so the committed documentation does not reintroduce the exact overpromise the UI fix removed.
AGENTS.md reference: AGENTS.md:L251-L251
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb702eb. Corrected the plan's Announcement section, which still described Egg Lots getting the same full-audit-trail link as the other five screens — stale since the previous commit relabeled that link "Adjustment history" for a narrower, honest scope. Annotated forward per this repo's own convention (never rewrite what old history displays) rather than silently rewriting the original text.
…ow content (#493) Codex round 4, two findings. The stale-scope mechanism had already been through two content-based versions (row-entityId comparison), each of which review found a variant it missed: a stale empty page (no row to compare), and leaving a scope entirely (entityId -> undefined, which the content check exited on by design, so old scoped rows could render under the global heading). Two misses of the same shape means the method was wrong, not just missing a case. Redesigned around comparing fetchPage's own reference identity instead of inferring staleness from row content: a ref tracks the fetchPage that was current the last time a reload completed (committedFetchPageRef, updated only when not reloading), and rows are stale whenever the current fetchPage differs from that — one mechanism that closes every variant uniformly, because it doesn't look at content at all. isFetchStale, the comparison itself, is a pure reference-equality check extracted for direct unit testing. Also corrected the Egg Lots overpromise (codex's second finding this round) in this plan's own Announcement section, which had described Egg Lots as getting the same full-audit-trail link as the other five screens — stale since the previous commit relabeled that link "Adjustment history" for a narrower, honest scope. Annotated forward rather than rewritten, matching how the rest of this plan's decision log handles corrections. 2 pi review rounds on the redesign. Round 1 raised two sophisticated- sounding claims about a rapid double-switch (click record B, then click record C before B's fetch resolves) potentially resurrecting the stale window via the ref-update timing, and a fetch error potentially stalling the page permanently. Both checked directly against usePagedList.ts's actual ticket system rather than reasoned about in the abstract, and both are structurally impossible: a superseded fetch's completion can never touch the reloading flag (setLoadingOwned no-ops whenever its ticket no longer matches req.current, and every ticket is claimed synchronously before any await), and the finally block that clears reloading runs unconditionally on success or failure. Documented this reasoning as a comment at the ref-update site so a future reader doesn't have to re-derive it. One legitimate point accepted: the empty-page integration test couldn't actually discriminate the old broken mechanism from the new one (a synchronously-resolving mock flushes before any assertion either way) — reworded its name and comment to claim only what it proves. Round 2 clean. Verified: 1770/1770 web tests pass, typecheck clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb702eb41f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isScopedReloading = events.reloading || isFetchStale(committedFetchPageRef.current, fetchPage); | ||
| if (!events.reloading) { | ||
| committedFetchPageRef.current = fetchPage; |
There was a problem hiding this comment.
Fresh evidence after eb702eb: if navigation A→B starts B's reload and the user navigates to C before B settles, the ref remains at B while reloading is true. When C completes, this render computes isScopedReloading as true from B !== C, then mutates the ref to C afterward; changing a ref schedules no further render, so the page remains on “Loading…” until some unrelated update occurs. Commit the identity in a completion-driven state/effect or otherwise trigger recomputation, and cover the delayed B→C transition rather than only a single switch.
AGENTS.md reference: AGENTS.md:L184-L186
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cb23040. You were right — the ref mutation, once it "caught up" on the render where C's own reload completed, had nothing to trigger a further render with the corrected value, since ref mutations don't schedule renders. Replaced the ref with React state, committed via a useEffect keyed only on events.reloading (not on fetchPage, deliberately): it fires exactly once per genuine reloading transition, and being a state update (not a ref mutation) it schedules the render that needs the corrected value.
Confirmed via mutation testing against the exact scenario you described (click B, click C before B resolves): the new test fails — stuck permanently, exactly as reported — against the previous ref-based version, and passes against this one. Also added a three-way switch test (B, then C, then D, with B and C never resolving) for extra confidence.
Verified: 1772/1772 web tests pass, typecheck clean.
Codex round 5: the ref-based redesign from the previous commit got permanently stuck on the generic "Loading…" fallback after a delayed double-switch (click record B, then click record C before B's fetch resolves). Traced precisely: on the render where C's own reload completes, that render needs to read the PRE-update committed value to correctly show one more "reloading" state — but mutating a ref doesn't schedule a re-render, so nothing ever re-evaluates with the corrected value afterward. The page could get stuck forever, which is worse than every version before it. Replaced the ref with React state, committed via a useEffect keyed only on events.reloading (deliberately not on fetchPage): the effect fires exactly once per genuine reloading transition, reading that render's own fresh fetchPage from its closure, and because it's a state update rather than a ref mutation, the render that needs the corrected value actually gets scheduled. Also fixes a real bug TypeScript itself caught mid-fix: useState(fetchPage) and setCommittedFetchPage(fetchPage) would have React call fetchPage as a lazy initializer/updater rather than store its reference, since fetchPage is itself a function — fixed via the () => fetchPage wrapper form. 2 pi review rounds, the most consequential yet — round 1 claimed the entire mechanism was still broken (effect deps never observing the B-to-C transition, a stale closure capture, React setState not actually treating a bare function as an updater). All three checked rigorously against React's documented effect-closure and setState semantics, not just re-reasoned about: React always runs the effect callback from the render where a dependency change was OBSERVED, never a stale discarded one; TypeScript's own compiler had already rejected the unwrapped setState form earlier in this exact investigation, which is only possible if it modeled the argument as an attempted updater; and a new test using three separately-awaited act() calls (not one batched flush) empirically confirms the fix survives a real double-switch, with mutation testing confirming it fails (stuck permanently) against the previous ref-based version. One suggestion accepted: added a three-way switch test for extra documentation, though the mechanism's correctness doesn't depend on switch count or resolution order. Round 2 clean. Verified: 1772/1772 web tests pass, typecheck clean.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Review loop stopped here: codex's round 6 pass (commit cb23040) explicitly reported no findings — first clean round after 5 consecutive rounds that each surfaced a real product bug (missing admin gate, and four rounds tracing a stale-scope render race through content-based → ref-based → state-based fixes, the last two verified via mutation testing against the actual reported failure mode). Full trail in docs/plans/audit-entity-history/00-status.md. Not tagging further; will revisit if the PR gets new commits or you ask for another pass. |
1 participant
Closes #493.
Admins/managers can now reach a single record's full audit trail from that record's own screen — a "View history" link on Flocks, Egg grades, Sales orders, Expenses, Daily entries, and Egg lots opens
/audit?entityId=<id>, an entity-scoped mode of the existing global Audit page. No new API surface: reuses theentityIdfilterGET /api/v1/auditalready supported server-side but that nothing ever called.What changed, by slice
AuditPagereadsaction/entityIdfrom the URL (useSearchParamsbecomes the single source of truth for both filters), with a client-side GUID guard that falls back to the unscoped view on a malformedentityIdrather than firing a request the endpoint would 400 on.usePagedList'sreloadingflag (fixes a stale-heading case: the previous entity's type could flash during a filter change or entity switch), hides the now-redundant entity column when scoped, and gives a scoped empty state its own message.StockPage, which already has an unrelated "history" toggle for the inventory movement ledger — the new link is a real navigation, kept visibly distinct)./audit, without a remount. The primary record-to-record browsing flow, and the one path the original design review never covered./api/v1/auditwas called withentityId, the one non-SPA line in the ticket. Known, accepted limitation pinned by its own test: a well-formed but non-matchingentityIdstill counts as a "successful scoped read."GLOSSARY.md+ Help page bullet (es/tl translated inline per this project's translate-now policy).Process
Planned through the software-factory 4-gate workflow (
docs/plans/audit-entity-history/) with a pi (local vllm) contrarian review at every gate and every implementation slice — 5 gate rounds + 11 slice rounds, each slice stopped at 2 consecutive clean/resolved rounds per the loop's own stop rule. Roughly two dozen real findings acted on across the whole process (a brokensetSearchParamsmerge, a stale-heading race, screen-location corrections, a missing browsing-flow test, several test-quality gaps) and a comparable number of false claims verified against the actual code and rejected with evidence. Full trail indocs/plans/audit-entity-history/00-status.md.Test plan