-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Support Ticket Attachments #24972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Generated by 🚫 Danger |
f6aaec6 to
9179bec
Compare
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 29844 | |
| Version | PR #24972 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 6c07ae1 | |
| Installation URL | 1goenavor1mpg |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 29844 | |
| Version | PR #24972 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 6c07ae1 | |
| Installation URL | 3a83mi9bpb8a8 |
|
Edited: I realised I cannot select videos to add them as attached files. Is that expected? |
Screen.Recording.2025-11-06.at.16.14.10.mov |
|
Great! Now I can add screen-recordings, but when selecting one, I don't get any feedback about it being added (or not). It's a 10MB video. ScreenRecording_11-06-2025.20-11-33_1.MP4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I left a few comments with things to potentially simplify. As a general feedback for the form, I'm going to put it this PR, but it's not directly related to attachments, so please feel to process these the way you see fit.
1. "Submit Request" form
I found the "Contact Support" a bit overwhelming and unapproachable. I wanted to see "Message", and it's not on the screen when you open it.
A couple of more specific items:
- Should use a standard
Button.make(role: .cancel)(x mark) and in the leading edge instead of "Cancel" in the confirmation button placement. - The "I need help with" looks like it supports multiple selection but doesn't. It takes a lot of vertical space.
- Is "I need help with" required and/or important? Is that something HEs care about? If not, I'd consider removing it and making it easier to reach out.
- The "Subject" should probably be optional - it's asking too much.
- Consider removing "(Optional)" label after "Screenshots" and "Application Logs" – it's obvious from the context.
- The "Send" button is not visible until you scroll to he bottom. If you skip a required field like "Subject", reach the bottom and find it disabled, it's hard to figure out what's wrong. Consider using a
Button.make(role: .confirm)in the trailing edge. It's always visible and provides immediate feedback. - Does it need a "Contact Information" section? I'd suggest adding this information on a confirmation screen if there is one – "we got your request. we will reach out to " etc.
- It's be nice to pre-populate the "Site Address" field and use a site picker instead of asking you to type your site address manually.
- I would suggest removing the "Attachment Limit" unless you reach the limit – then show it as an error. It also looks too much like a progress indicator, especially in things context. If you show it, consider showing a small label with text and no bar (excessive, take too much space and attention)
2. "Ask the Happiness Engineers"
I would make sure to consult with HEs before exposing this option directly. I would assume HEs would prefer a unified experience where you start with a bot that would gather the required information based on your query and get you in touch with support when needed.
Is there a reason we not using the same design as on the web with a single simple chat view? Is there a need for a dedicated "Contact Support" form in general? As a user, I would probably a chat-like interface and AI to ask me the questions if it needs more data instead of a large form to complete upfront.
"Ask the bots" – that's likely not something people normally would want to select given the option. If 95+% of people people skip this option and go to support directly, it could balloon the support cost and we would miss out on (great) AI assistant option.
3. Misc
- The screen used to show the app version number, which is important
- "Clearing Disk" got stuck showing both "Clearing..." and "Complete" at the same time
- I'd suggest cutting the "Support Profile" view – it's just your account.
- "Ask the bots" (if kept) should start with a new conversation and not your previous conversation history and not "No Conversations". "History" should be hidden somewhere in the navigation bar buttons.
| let result = try await generator.image(at: CMTime(seconds: 0.0, preferredTimescale: 600)) | ||
| let image = UIImage(cgImage: result.image) | ||
|
|
||
| guard let data = image.pngData() else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's relatively expensive to convert it to data and back. Does it needs to support data(for request). Would make sense to move it under image(for request)? Alternately, data(for request) could call image(for request) and then convert it to data so both would be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressed in #25000
| /// Asynchronous Image View that replicates the public API of `SwiftUI.AsyncImage` to fetch | ||
| /// a video preview thumbnail. | ||
| /// It uses `ImageDownloader` to fetch and cache the images. | ||
| public struct CachedAsyncVideoPreview<Content>: View where Content: View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a copy of AsyncImageKit.CachedAsyncImage with a single line that's different:
imageDownloader.image(for: ImageRequest(videoUrl: url))
I'd suggest adding an ImageRequest init in the existing CachedAsyncImage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressed in #25000
| await MainActor.run { | ||
| withAnimation { | ||
| self.state = .clearing(progress: progress, result: "Working") | ||
| try await Task.runForAtLeast(.seconds(1.5)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems excessive. Does it ever take more than 10ms to clear disk caches? Removing a cache folder is usually an instant operation. I'd suggest showing an indefinite ProgressView and eliminating the complexity with the reporting – less code, less stuff to localize and maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it ever take more than 10ms to clear disk caches? Removing a cache folder is usually an instant operation
Have you ever tried deleting a Netflix downloaded video file from the iOS storage management screen? It takes multiple seconds and freezes the UI 😅
This functionality should probably still exist even if the progress were indeterminate – the idea is that if the operation is instant it won't flash the UI or perform a choppy animation, it'll nicely animate to one state then back to the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you ever tried deleting a Netflix downloaded video file from the iOS storage management screen? It takes multiple seconds and freezes the UI 😅
It's a different use-case. It does make sense to show nice fake animation even if the work is instant.
I'd be curious why it takes so long for them. It really is nearly instant for the most scenarios.
I'm not sure it's really worth including in this release tbh, because, if I'm not mistaken, there is little that gets stored in it. It would help to perhaps add a way to clear Core Data caches etc, but normally HEs simply recommend to re-install the app, which is probably the best way to resolve any cache-related issues.
| } | ||
| } | ||
|
|
||
| public struct FullScreenErrorView: View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a standard EmptyStateView from WordPressUI and include the dependency if it's not yet included. Alternatively, you should now be able to use native ContentUnavailableView with iOS 17.
It looks like it's also missing localization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses ContentUnavailableView as of 04d4c15
Localization I was going to leave since it makes this PR larger and harder to follow but it's done in f5b830093196b36d206d2a3c5c2c23429fa0395e
| .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .top) | ||
| .padding(.top, 24) | ||
| .onChange(of: context.date, { oldValue, newValue in | ||
| if case .awaitingHiding(let until) = state { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems overly complex. Other screens in the app use a simple understated ProgressView. You can instantly hide and show it and it looks good.
If it's loading the content, the progress view is shown in the middle. If it's a result an asynchronous action, it usually replaces a button. The overlays, and especially with custom design, make the app feel slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an example of this somewhere? I couldn't find anything that had a "load the cache and display activity while updating from the server" pattern already, which was why I ended up adding this.
| @GestureState private var currentZoom = 1.0 | ||
|
|
||
| var magnification: some Gesture { | ||
| MagnifyGesture().updating($currentZoom, body: { newValue, state, transaction in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't seem to work for me. The views are too small for zooming with gestures. I'd suggest using .contextMenu with a details view instead (for long-press) or removing it altogether.
| @@ -0,0 +1,21 @@ | |||
| import Foundation | |||
|
|
|||
| public protocol CachedAndFetchedResult<T>: Sendable { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) This seems a bit confusing. What is UncachedResult for? Why does result have two async closures? Is it more like a promise? Why would you need CachedAndFetchedResult in a single struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's technically two promises – one for fetching from the cache and one for fetching from the network.
It allows you to return a single object from an endpoint that makes it easy to read the same type from the cache quickly then from the network.
Might need a better name, but it's a lot more ergonomic to work with from view code than AsyncStream<T>
| enum RunForAtLeastResult<T>: Sendable where T: Sendable { | ||
| case result(T) | ||
| case wait | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enum is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in a2c74e0
| } label: { | ||
| ZStack { | ||
| if attachment.isImage { | ||
| AsyncImage(url: attachment.url) { image in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use CachedAsyncImage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a2c74e0
summarization Use first message for conversation title
conversation status status
fb1c03d to
97206f0
Compare
kean
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I briefly re-reviewed the changes, and the code appears good. I left a couple of minor comments that don't block the merge. I did leave a separate comment on #25000, which seems to also be included in this PR, so please address it before merging.
| await summarize(firstMessageText) | ||
| } | ||
| } else { | ||
| title = "New Bot Chat" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit ) missing l10n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| public extension Date { | ||
| /// Is this date in the past? | ||
| var hasPast: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) it doesn't seem to be worth adding as a public extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|






Description
Adds support for ticket attachment and status.
Testing instructions
Try:
Next Steps
A future PR will address localization, and will update
wprsto address conversation titles being based on the last message, not the first.