feat(profile): load Codeforces avatar and introduce reusable EmptyStateView#14
feat(profile): load Codeforces avatar and introduce reusable EmptyStateView#14mvanhorn wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 59 minutes and 51 seconds.Comment |
|
Heads up: I didn't run this on a Simulator locally. No new tests in this one (EmptyStateView is a pure-View struct), so manual QA on Profile avatar load + the four empty-state screens is on you before merge. If anything looks off in the build, ping me and I'll iterate. |
|
Hi @mvanhorn. Appreciate the patience here. I gave this a proper read. Two things need to be addressed:
I want to be direct about something, because I think it will save us both time. Several patterns in this PR are consistent with code that was generated rather than written, and not fully reviewed before submission. I am not saying that using AI tooling is a problem in itself, it is not, but submitting output you have not personally verified is, particularly for a multi-screen change like this one. I have a few reasons to say so: The most obvious signal is the The private func normalizeAvatarURL(_ raw: String?) -> URL? {
guard let raw = raw, !raw.isEmpty else { return nil }
let normalized = raw.hasPrefix("//") ? "https:" + raw : raw
return URL(string: normalized)
}This is a pure utility function with real edge cases -- nil input, empty string, protocol-relative URLs, plain The if let actionLabel = actionLabel, let action = action {
Button(action: action) { Text(actionLabel) ... }
}If a caller passes one without the other, the button silently disappears with no error or warning. That is a footgun that a person designing a reusable component API would typically catch because they would ask "what happens if someone only passes the label?" Grouping them into a single Finally, the branch was not rebased before opening the PR. I want to be clear that I am not trying to be harsh here. The features themselves -- the avatar loading, the One last thing before you push the next revision -- can you tell me in a comment what |
…ndling - Group EmptyStateView's actionLabel/action into a single Action struct so a label can never be passed without a handler (or vice versa) - Stop using EmptyStateView as a loading state in ContestListView; loading now uses ProgressView like the rest of the app - Move normalizeAvatarURL out of ProfileView into Utilities/ with internal access, and add unit tests covering nil, empty string, protocol-relative, https passthrough, and http preservation
The .placeholder modifier was applied after .clipShape, where the type is already erased to some View; SDWebImageSwiftUI 3.x also moved placeholder to an initializer closure. Rewritten with the content/ placeholder initializer so the branch builds and the placeholder actually renders.
|
To answer your question first: normalizeAvatarURL("") returns nil. The guard rejects empty (and nil) input before any URL construction - URL(string: "") would happen to return nil anyway, but the guard makes the intent explicit: no input, no URL, and WebImage falls through to the placeholder instead of attempting a meaningless load. Your read on verification was fair, and I will own it concretely: when I built this branch end-to-end before this revision, the avatar code did not compile against SDWebImageSwiftUI 3.x (.placeholder was applied after .clipShape, and 3.x moved placeholder into the initializer anyway). That is fixed in 7b7a0aa, and the branch now builds and tests on an iPhone 17 Pro simulator. In 132a0fa: actionLabel/action are grouped into an EmptyStateView.Action struct so the pairing cannot be misconfigured; ContestListView loading state uses ProgressView like the rest of the app instead of EmptyStateView; normalizeAvatarURL moved to Utilities/ with internal access and 5 unit tests (nil, empty, protocol-relative, https passthrough, http preserved - the last documents that ATS, not the normalizer, decides what happens to http URLs). On the rebase: I may be missing something - the branch is based on current main (d744e41), GitHub reports it mergeable, and the ProfileView error state on main in my checkout is a plain red Text with no retry button or neonPink icon. I also do not see inline comments on the PR; if you left a pending review it may not be submitted yet. If the improved error state lives on a branch you have not pushed, point me at it and I will rebase onto it happily. |
Description
Two related UI improvements bundled per issue #9.
Part A. Profile screen now loads the user's actual Codeforces avatar via SDWebImageSwiftUI's
WebImageinstead of the SF Symbol placeholder. Protocol-relative URLs from the API (//userpic.codeforces.org/...) are normalized by prependinghttps:. The originalperson.fillsymbol remains as the placeholder for nil URLs and load failures, with the same circular shape, gradient border, and neon shadow.Part B. New reusable
CForge/Views/Common/EmptyStateView.swiftstandardizes empty, error, and loading states acrossContestListView,ProblemListView,ProfileView, andProblemSubmissionsView. Switching offContentUnavailableViewinProblemSubmissionsViewalso lifts the iOS 17+ floor for that screen.Happy to split this into two PRs (avatar first, sweep second) if you'd prefer smaller review units. Let me know.
Related Issue
Closes #9
Changes Made
ProfileModels.swift: addavatar: String?andtitlePhoto: String?toCodeforcesUser.ProfileView.swift: replace the SF Symbol inprofileHeader(user:)withWebImage(url: normalizeAvatarURL(user.titlePhoto ?? user.avatar))and aperson.fillplaceholder. AddnormalizeAvatarURL(_:)helper for protocol-relative URLs. Replace the raw red error text withEmptyStateView.Views/Common/EmptyStateView.swift: configurable icon, title, optional subtitle, optional action button. Neon-themed.ContestListView.swift: replace bareProgressView()initial-load placeholder withEmptyStateView(loading variant). Keeps the existing.onAppeartrigger.ProblemListView.swift: replace the inline error VStack withEmptyStateView.ProblemSubmissionsView.swift: replace threeContentUnavailableViewcalls (signed-out, error, no-attempts) withEmptyStateView.Screenshots (if UI changes)
Will add before / after pairs for the Profile avatar and one or two empty states before review.
Checklist
Cmd+B)