Allow custom display names#1373
Conversation
Greptile SummaryThis PR allows users to set a custom display name (
Confidence Score: 4/5Safe to merge — the change is well-scoped, backed by tests, and correctly centralizes display-name resolution in User#display_name. All callers are updated, normalization handles edge cases (blank, format characters), and the Slack sync test confirms the custom override is preserved on re-sync. The only finding is a minor redundant double-presence check in the new display_name logic. app/models/user.rb — minor redundant double-presence check in the new display_name block. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant ProfileController as Settings::ProfileController
participant User as User (model)
participant DB
Browser->>ProfileController: PATCH /my/settings/profile/display_name
ProfileController->>ProfileController: display_name_params (strong params)
ProfileController->>User: update(display_name_override: "Custom Name")
User->>User: normalize_display_name_override()
User->>User: "validates length <= 80"
alt valid
User->>DB: UPDATE users SET display_name_override
ProfileController-->>Browser: 302 redirect
else invalid
User-->>ProfileController: errors
ProfileController-->>Browser: 422 Unprocessable Entity
end
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
app/models/user.rb:312-313
Redundant double-presence check: `display_name_override.presence` already returns `nil` when the value is blank, so `name.present?` on the next line always equals `!name.nil?`. The guard can be collapsed to a single expression, consistent with the natural reading of the method.
```suggestion
return display_name_override if display_name_override.present?
```
Reviews (1): Last reviewed commit: "Allow custom display names" | Re-trigger Greptile |
| name = display_name_override.presence | ||
| return name if name.present? |
There was a problem hiding this comment.
Redundant double-presence check:
display_name_override.presence already returns nil when the value is blank, so name.present? on the next line always equals !name.nil?. The guard can be collapsed to a single expression, consistent with the natural reading of the method.
| name = display_name_override.presence | |
| return name if name.present? | |
| return display_name_override if display_name_override.present? |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/user.rb
Line: 312-313
Comment:
Redundant double-presence check: `display_name_override.presence` already returns `nil` when the value is blank, so `name.present?` on the next line always equals `!name.nil?`. The guard can be collapsed to a single expression, consistent with the natural reading of the method.
```suggestion
return display_name_override if display_name_override.present?
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
Adds support for user-controlled display names by introducing profile settings UI and routing for display_name_override, then centralizing display-name precedence in User#display_name.
Changes:
- Added display-name update route, controller params, settings props, and Svelte form.
- Updated
User#display_nameto preferdisplay_name_override, with normalization and length validation. - Updated profile/OG display-name call sites and added model/controller tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
app/models/user.rb |
Adds display-name override precedence, normalization, and validation. |
app/controllers/settings/profile_controller.rb |
Adds profile display-name update action and props. |
app/controllers/settings/base_controller.rb |
Exposes display-name override and validation errors to settings pages. |
app/javascript/pages/Users/Settings/Profile.svelte |
Adds the display-name settings form. |
app/javascript/pages/Users/Settings/types.ts |
Adds display-name props/errors and subsection metadata. |
config/routes.rb |
Adds the display-name settings PATCH route. |
config/initializers/js_from_routes.rb |
Exports the new route to frontend path helpers. |
app/controllers/profiles_controller.rb |
Uses centralized display_name for public profile payload/social text. |
app/services/profile_og_image_generator.rb |
Uses centralized display_name for OG image generation. |
test/models/user_test.rb |
Adds display-name precedence, normalization, and Slack sync coverage. |
test/controllers/settings_profile_controller_test.rb |
Adds display-name update, clear, and validation controller coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const subsectionMap: Record<SectionId, SettingsSubsection[]> = { | ||
| profile: [ | ||
| { id: "user_region", label: "Region" }, | ||
| { id: "user_display_name", label: "Display name" }, |
Summary of the problem
Users could not set a custom display name. Their visible name came from synced Slack/GitHub/username data, so provider logins could effectively change it back.
Describe your changes
display_name_override.User#display_nameprefer the override before synced Slack, GitHub, username, or email fallback values.User#display_namebehavior.Validation:
docker compose exec web rails test test/controllers/settings_profile_controller_test.rbdocker compose exec web rails test test/models/user_test.rbdocker compose exec web bun run check:svelte(passes with two existing warnings inHome/signedIn/IntervalSelectBody.svelte)docker compose exec web env JS_FROM_ROUTES_FORCE=true bin/rake js_from_routes:generateScreenshots / Media
Not included.