Paths

Table of Contentst

Differential D25987

Add CSS class to object handle if user object is system agent
Needs ReviewPublic
Actions

Authored by aklapper on Fri, May 2, 23:53.

Details

Reviewers

None

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T16051: Add a CSS class on comments to indicate whether the comment author is a bot

Summary

Set the base for future customization to easier differentiate between human actors and machines.

Test Plan

Inspect the CSS of a bot account handle and a user account handle in a Maniphest task. See that <a> has an additional phui-system-agent class.

Diff Detail

Repository

rP Phorge

Branch

T16051 (branched from master)

Lint

Lint Errors

Severity	Location	Code	Message
Error	src/applications/phid/PhabricatorObjectHandle.php:28	XHP1	PHP Syntax Error!

Unit

Test Failures

Build Status

Buildable 1922
Build 1922: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	2,378 ms	PhabricatorLibraryTestCase::testLibraryMap EXCEPTION (XHPASTSyntaxErrorException): applications/phid/PhabricatorObjectHandle.php: XHPAST Parse Error: syntax error, unexpected T_STRING, expecting T_VARIABLE on line 28 #0 /var/www/html/phorge/arcanist/src/moduleutils/PhutilLibraryMapBuilder.php(63): PhutilLibraryMapBuilder->analyzeLibrary() #1 /var/www/html/phorge/arcanist/src/__tests__/PhutilLibraryTestCase.php(29): PhutilLibraryMapBuilder->buildMap()
	604 ms	PhabricatorCelerityTestCase::testCelerityMaps 1 assertion passed.
	1,159 ms	PhabricatorConduitTestCase::testConduitMethods 1 assertion passed.
	625 ms	PhabricatorInfrastructureTestCase::testApplicationsInstalled 1 assertion passed.
	515 ms	PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries 1 assertion passed.
		View Full Test Results (1 Failed · 6 Passed)

Event Timeline

aklapper created this revision.Fri, May 2, 23:53

Owners added a reviewer: O1: Blessed Committers.Fri, May 2, 23:53

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptFri, May 2, 23:53

aklapper requested review of this revision.Fri, May 2, 23:53

Harbormaster completed remote builds in B1922: Diff 2971.Fri, May 2, 23:53

Don't bool with the bool otherwise it bools

Harbormaster completed remote builds in B1923: Diff 2972.Fri, May 2, 23:57

always set a bool so we have an explicit value and don't just fallback to "false"

Harbormaster completed remote builds in B1924: Diff 2973.Sat, May 3, 00:04

Uhm... can we sleep with the very possible possibility of having "User Bot" possibly everywhere? 🌈

valerio.bozzolan mentioned this in T16050: Make it easier for CSS customization to apply to comments by specific users/bots.Sat, May 3, 08:52

In D25987#26487, @valerio.bozzolan wrote:

having "User Bot" possibly everywhere? 🌈

No. Because consistency.

Uhm. I don't know why but locally I cannot find the selector. Also this should help in getting the selector but without success, both from the home, or from a task created and assigned to a bot.

.phui-system-agent {
  background: red !important;
}

P.S., maybe useful for the test plan: to create a task from a bot, create a bot, with conduit token, then:

arc --conduit-uri http://phorge.localhost --conduit-token=api-asdasd todo ASD

Edited: maybe better phui-link-system-agent like the other one already present that has phui-link - sorry lol

In D25987#26601, @valerio.bozzolan wrote:

Uhm. I don't know why but locally I cannot find the selector.

Huh?

Screenshot From 2025-05-06 18-25-44.png (362×897 px, 45 KB)

Screenshot From 2025-05-06 18-26-17.png (657×947 px, 107 KB)

Screenshot From 2025-05-06 18-26-37.png (889×1 px, 252 KB)

@matmarex: please look at the screenshots of Andre above: can I ask if such new selector would really simplify your downstream job? or, is there anything else missing / that we should do instead?

I ask this since the new selector will be very specific to the "user handle", but maybe you would love to easily catch the .phui-timeline-shell, with a .phui-timeline-shell-bot or similar, and not the handle.

Thanks for guiding us a bit here, so we better understand how deep we have to dig.

src/applications/phid/PhabricatorObjectHandle.php
380	Maybe a little comment that explains why currently we have no CSS rules and why it still makes sense
381	Maybe with `-link` prefix like the other selector in old line 368

Note that in the screenshots I used an extremely broad CSS selector as that broad one was specifically mentioned in the previous comment here.

Also, avivey does not like this approach so I'll likely decline this patch anyway.

In D25987#26708, @aklapper wrote:

Note that in the screenshots I used an extremely broad CSS selector as that broad one was specifically mentioned

I was drunk sorry. The phui-link-system-agent makes more sense.

Also, avivey does not like this approach so I'll likely decline this patch anyway.

I see. But aren't you curious about what @matmarex would like, to see where that would lead? I think the selector should be on the biggest container (e.g. phui-timeline-shell). Let's wait for feedback but, if yes, I think 90% of the patch is perfect as-is, and 10% could be moved to the thing who generates phui-timeline-shell (since it could now read PhabricatorObjectHandle#getSystemAgent() - so it's possible).

I mean, at least WMF Phabricator or other installs can pull this patch, if they want, and it would be probably exactly what they need to easily beautify 10 years of legacy data.

For my use case of styling Maniphest comments and other actions, it would be most convenient to have the extra class on phui-timeline-shell. But I'll be fine with maintaining my styles the way they currently are, if we drop this patch.