fix: improve chat audit log descriptions and diff rendering#25728
Conversation
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.6.0 | Round 7 | Last posted: Round 7, 17 findings (1 P1, 8 P3, 8 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-4 (P3, AuditLogRow.tsx:50) - IdP sync rendering change with no regression coverage
Round logRound 1Panel. 1 P1, 3 P3, 3 Nit, 1 Note new. 1 dropped. Reviewed against bfa17c3..56a6183. Round 2BLOCKED. CRF-4 silent. No review. Round 3Churn guard overridden to PROCEED (CRF-4 addressed via PR description update, missed by churn guard). Panel. 1 P3 new. Reviewed against bfa17c3..0de2687. Round 4BLOCKED. CRF-10 silent. No review. Note: new commit 4960ebe adds frontend description override logic (getAuditLogDescriptionOverride) that duplicates and simplifies backend chat description paths. Pending panel review once unblocked. Round 5CRF-10 addressed: backend chat description logic removed entirely, moved to frontend-only approach (74c575c). Author rationale: client-side descriptions provide backfill effect without modifying audit plumbing. Panel. 2 P3, 3 Nit, 1 Note new. 1 dropped. Reviewed against bfa17c3..74c575c. Round 6All R5 findings addressed (4f7e8b7): frontend override removed, description logic moved back to backend per human reviewer feedback. Netero clean. Panel (18 reviewers). 1 P3, 1 Nit new. 2 dropped. Reviewed against 12520ee..e7a3e3a. Round 7All R6 findings addressed (daae9f5). Netero clean. Panel (18 reviewers). 1 Nit new. Reviewed against 12520ee..0ead823. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Well-structured fix. The generic formatAuditDiffValue eliminates the class of [object Object] rendering bugs for all resource types, not just chat ACLs. The removal of determineIdPSyncMappingDiff is correct (subsumed by the generic handler), and the distinction from determineGroupDiff (which does semantic flattening, not just formatting) shows the author understood the code. Backend test coverage is thorough: 14 cases covering archive, unarchive, share/unshare variants, permission changes, error status codes, malformed diffs, and fallback paths.
One P1 blocks merge: the share/unshare descriptions place {target} mid-sentence, but AuditLogDescription strips it and appends the target link at the end. Every other description in the codebase puts {target} terminally. The result is a double space where {target} was removed and the chat name stuck to the end of the sentence. This likely explains the CI failures (UI Review, UI Tests). Three P3s and three Nits follow.
CI failures (UI Review, UI Tests) were not investigated before opening the PR. Given the P1 above, these are likely caused by the changes.
"A reader scanning this code will see Target and assume the function produces the {target} substitution. It doesn't. It produces the recipient count summary." (Gon)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
7 of 8 posted findings from round 1 were addressed in commit 0de2687. Good work on the fixes, particularly restructuring the share/unshare descriptions to keep {target} terminal and adopting the sort-before-compare pattern for auditLogChatACLEqual.
One finding remains unaddressed:
CRF-4 (P3) Removing determineIdPSyncMappingDiff changes IdP sync mapping diff rendering with no regression coverage. The thread was marked resolved in GitHub with no author comment and no code change. The review needs either: (a) a Storybook story for IdP sync mapping diffs, (b) a note in the PR description acknowledging the side effect, or (c) an explicit response explaining why no action is needed.
Further review is blocked until the author responds to or addresses CRF-4.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All seven R1 fixes verified by the panel. CRF-1 (P1) fix confirmed: {target} now terminal in all descriptions. CRF-2 clone-sort-compare pattern correct. CRF-3 test gaps closed. CRF-5/6/7/8 renames and doc comments all clean. CRF-4 acknowledged via PR description update.
One new P3 from the fix commit: the CRF-2 fix replaced the test that covered the updated=true permission-change path instead of keeping it alongside the new reorder test. The !aclDiff.updated guard on the share/unshare descriptions is now untested.
"The agent should have kept the original test case alongside the new reorder test." (Mafu-san)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
CRF-10 (P3, missing test for updated=true permission-change path) remains unaddressed. No change to coderd/audit_internal_test.go, no author response on the thread. This is the second consecutive blocked round for this finding.
New commit 4960ebeb8c adds getAuditLogDescriptionOverride to the frontend, which re-derives chat descriptions from the diff client-side. The backend chatAuditLogDescription still generates detailed share/unshare descriptions with user/group counts, but the frontend now overrides all ACL-only diffs to the simpler "updated sharing for chat {target}" regardless. This architectural addition has not yet been reviewed by the panel.
Further review is blocked until the author responds to or addresses CRF-10. The new frontend description logic will be reviewed in the next unblocked round.
🤖 This review was automatically generated with Coder Agents.
|
Rearchitected to handle the description updates client-side instead of server-side. Rationale: this gets us the effect of a backfill without having to update descriptions in the database and avoids any risky changes to the audit plumbing. This resolves CRF-10. |
|
/coder-agents-review |
There was a problem hiding this comment.
The rearchitecture to frontend-only is a good call. Client-side description derivation gives the backfill effect without touching audit plumbing, and the code is simpler. All prior findings are resolved. The formatAuditDiffValue generalization and determineIdPSyncMappingDiff removal carry over correctly from the earlier iterations.
Two P3s on test coverage: the action !== "write" guard has no test (if removed, non-write chat events get wrong descriptions), and the WithArchivedChatDescription story hardcodes the override's own output as the mock description, making it unfalsifiable. Three Nits follow.
"The story renders identically whether the override works or not. If getAuditLogDescriptionOverride is deleted entirely, this story shows no regression." (Mafu-san)
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Pull request overview
Fixes audit log UI rendering for chat events: object diff values (chat ACLs, IdP sync mappings) no longer render as [object Object], and common chat write actions get semantic descriptions (archive/unarchive, sharing changes) instead of the generic "updated chat".
Changes:
- Extract diff value formatting into a single
formatAuditDiffValuehelper that handles strings, time objects, arrays, nullish values, and plain objects (compact JSON with sorted keys), replacing the inlinegetDiffValueand the IdP-sync-specificdetermineIdPSyncMappingDiffworkaround. - Add
getAuditLogDescriptionOverrideto derive client-side semantic descriptions for chat archive/unarchive and sharing writes, gated on resource type, action, and status code. - Add unit tests for both new helpers and new Storybook stories for chat ACL diff, archived chat, and sharing-update audit rows.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.ts | Removes determineIdPSyncMappingDiff; introduces formatAuditDiffValue with sorted-key JSON for plain objects and recursion for arrays. |
| site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.test.ts | Adds coverage for primitives, arrays, time objects, plain objects, chat ACL maps, and nested objects. |
| site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/AuditLogDiff.tsx | Switches to formatAuditDiffValue for old/new value rendering. |
| site/src/pages/AuditPage/AuditLogRow/AuditLogRow.tsx | Drops the IdP sync mapping diff preprocessing now that objects format natively. |
| site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx | Adds getAuditLogDescriptionOverride for chat archive/unarchive and sharing writes; uses it before token replacement. |
| site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.test.ts | New unit tests covering override conditions and negative cases (mixed diffs, non-2xx, 303, non-chat, non-write). |
| site/src/pages/AuditPage/AuditLogRow/AuditLogRow.stories.tsx | Adds chat-related stories for visual coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/coder-agents-review |
There was a problem hiding this comment.
All five R5 findings addressed in commit 4f7e8b7. The rearchitecture to backend-only descriptions is a clean final state: chatAuditLogDescription inspects the diff server-side, the frontend renders whatever the API returns, formatAuditDiffValue eliminates the class of [object Object] rendering for all resource types. Backend test coverage is thorough (10 cases covering every branch), and the prior architectural churn (backend -> frontend -> backend) settled on the right layer. CI is green.
One P3, one Nit. The P3 is a sibling of CRF-12 from R5: a story renders a description that doesn't match what the backend produces for the depicted diff.
"The story renders 'updated chat' where production renders 'updated sharing for chat.' This is the only chromatic story, so the snapshot baseline is wrong." (Leorio)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
| } | ||
| } | ||
|
|
||
| // All fields are ACL changes: sharing update. |
There was a problem hiding this comment.
Does there ever exist a situation where we should build upon a sentence, like "updated sharing for chat {target} and ..."? Or are audit diffs limited to a single type of field being changed?
There was a problem hiding this comment.
I'm limiting the fancy descriptions to single fields for now. It will fall back to the generic {user} updated chat {target}.
There was a problem hiding this comment.
Both R6 findings addressed in commit daae9f5. Netero clean, 18-reviewer panel clean. All 20 findings across 7 rounds resolved. One Nit on a test helper comment.
The formatAuditDiffValue generalization eliminates the class of [object Object] rendering bugs. The backend chatAuditLogDescription produces semantic descriptions at the right layer. Test coverage is thorough: 10 backend cases, 12 frontend type-formatting cases, 3 Storybook stories. CI green.
"I tried to build a case against this and could not. The problem is correctly understood as two distinct issues. The solutions are proportional: one generic, one targeted." (Pariston)
🤖 This review was automatically generated with Coder Agents.
| } | ||
| } | ||
|
|
||
| // chatAuditLogRow builds a GetAuditLogsOffsetRow for a successful chat write |
There was a problem hiding this comment.
Nit [CRF-21] Doc comment restates the diff parameter ("with the given diff") and the file context ("suitable for testing auditLogDescription"). Trim to:
// chatAuditLogRow builds a GetAuditLogsOffsetRow for a successful chat write.(Gon)
🤖
Chat ACL audit diffs rendered as
[object Object]because the diff viewer called.toString()on object values. Common chat operations (archive, share) showed generic "updated chat" descriptions instead of semantic ones.Add
chatAuditLogDescriptionto derive semantic descriptions from the audit diff for successful chat writes: "archived/unarchived chat" for archive toggles, "updated sharing for chat" for ACL-only changes. Extract diff value formatting intoformatAuditDiffValue, which renders object values as deterministic compact JSON with sorted keys, fixing the[object Object]rendering for chat ACLs and any other object-valued fields. The previousdetermineIdPSyncMappingDiffworkaround for IdP sync mappings was removed because the generic formatting handles it.Closes CODAGT-513
Generated by Coder Agents.