feat: newbies in contributors#647
Conversation
vvtimofeev
requested review from
dgaponov,
imsitnikov and
stepanenkoxx
as code owners
| const {login, contributions} = contributor; | ||
| // Process in small batches to avoid GitHub secondary rate limits | ||
| const BATCH_SIZE = 5; | ||
| for (let i = 0; i < repos.length; i += BATCH_SIZE) { |
There was a problem hiding this comment.
How long does such batch loading take approximately? We can test it on the production build by opening the main page first. If it's significantly slower than before, let's try increasing BATCH_SIZE.
There was a problem hiding this comment.
Measured it against the org: gravity-ui has 74 public repos, and each repo costs pages(contributors) + pages(commits in last 30d) requests via octokit.paginate (1 request/page). With the default per_page=30 the whole refresh is only ~220 requests (uikit is the outlier at ~8; most repos are 2–4).
Relevant secondary limits are: ≤100 concurrent requests and ≤900 points/min (GET = 1 point). Since the whole job is ~220 requests total, the points/min limit can't be hit at any batch size. The only limit that scales with BATCH_SIZE is concurrency — and because each repo's two calls run sequentially, peak in-flight ≈ BATCH_SIZE.
So I bumped BATCH_SIZE to 50 (peak ~50 vs the 100 cap = 2× margin; finishes 74 repos in 2 rounds) and added per_page: 100 to both paginate calls, which cuts total requests ~30% and drops uikit from 7 pages to 3. It's also fully cached (24h TTL), so this runs at most once a day and never on the user request path.
| const onLoadRef = React.useRef(onLoad); | ||
| onLoadRef.current = onLoad; | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
What's the goal of this effect?
There was a problem hiding this comment.
When a component is already in the module-level cache, it renders synchronously (the cache.has(cacheKey) branch lower down) and skips the React.lazy path where onLoad is normally fired. This effect makes sure onLoad still fires for that cache-hit case. The onLoadRef indirection keeps the effect subscribed to [cacheKey] only, so it doesn't re-run every time an inline onLoad changes identity. Added code comments explaining both.
- Remove self-referential `.gitignore` entry - Document the onLoad ref + cache-hit effect in IntersectionLoadComponent - Raise contributor-fetch BATCH_SIZE to 50 and set per_page=100, cutting request volume while staying well under GitHub's 100-concurrent secondary limit Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| contentClassName={b('header-title-content')} | ||
| /> | ||
| {isLoaded ? ( | ||
| <YFMWrapper |
There was a problem hiding this comment.
Do we really need skeleton for static text?
There was a problem hiding this comment.
Good point — no, we don't. The title comes from props and is available on first render, so I removed the skeleton and now render the title directly. (Removed the now-unused SKELETON_HEADER_TITLE_* constants too.)
| )} | ||
| </h2> | ||
| <div className={b('header-count')}>{contributorsCount}</div> | ||
| <div className={b('header-count')}> |
There was a problem hiding this comment.
Incorrect skeleton parts placement in mobile version:
There was a problem hiding this comment.
Fixed. The misplacement was caused by the fixed-width (202px) title skeleton: on mobile it didn't match the real title width and pushed the header parts around. Now that the static title renders directly, the header keeps its natural width and the only remaining header skeleton (the count) lines up with the loaded layout.
There was a problem hiding this comment.
Follow-up (bd99b8f): went further on the whole mobile skeleton, not just the header. The community grid skeleton now mirrors BaseContributorList's repeat(auto-fill, minmax(44px, 1fr)) grid (was fixed 44px columns + per-row offsets, so avatars stayed small/staggered), and it's clipped to the same 212px teaser as the collapsed list instead of rendering a tall avatar wall. The newcomers skeleton avatars are now left-aligned (were right-aligned via justify-content: flex-end + margin-left: auto) and sized to 42px to match <Avatar size="l">. Verified the skeleton now matches the loaded layout at mobile widths.
The header title comes from props and is available on first render, so it doesn't need a loading skeleton. Removing the fixed-width title skeleton also restores the natural header width, fixing the misaligned skeleton placement in the mobile layout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bile The skeleton diverged badly from the real content at mobile widths: - Community grid used fixed 44px columns with per-row offsets, so avatars stayed small, staggered and left-clustered instead of stretching to fill the row. Now mirrors BaseContributorList's `repeat(auto-fill, minmax(44px, 1fr))` grid (gap 11px), and is clipped to EXPANDABLE_MIN_HEIGHT (212px) so it shows the same teaser as the collapsed list rather than a tall avatar wall. - Newcomers skeleton avatars were right-aligned (justify-content: flex-end + margin-left: auto); the loaded row is left-aligned. Removed both so they match in LTR and RTL, and sized them to 42px (= Avatar size "l"). - Trimmed the section-title skeleton width to better match the real titles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
No description provided.