Skip to content

Commit 5a7b100

Browse files
committed
refactor(ads): improve ad loading and caching logic
- Prevent premature ad disposal in feed and in-article ad loaders - Implement caching logic for in-article ads with expiry - Refactor error handling and state updates in in-article ad loader - Remove unnecessary ad disposal comments - Optimize ad loading completer logic
1 parent b7cc090 commit 5a7b100

File tree

2 files changed

+65
-57
lines changed

2 files changed

+65
-57
lines changed

lib/ads/widgets/feed_ad_loader_widget.dart

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -141,12 +141,11 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
141141

142142
@override
143143
void dispose() {
144-
// Dispose of the ad's resources when the widget is permanently removed.
145-
_adCacheService.removeAndDisposeAd(
146-
contextKey: widget.contextKey,
147-
placeholderId: widget.adPlaceholder.id,
148-
);
149-
144+
// The ad is intentionally not disposed of here. The InlineAdCacheService
145+
// is responsible for managing the ad's lifecycle. The cache for this
146+
// context ('all', 'followed', etc.) will be cleared by the
147+
// HeadlinesFeedBloc during a pull-to-refresh, which is the correct
148+
// time to dispose of these ads.
150149
// Cancel any pending ad loading operation when the widget is disposed.
151150
// This prevents `setState()` calls on a disposed widget.
152151
if (_loadAdCompleter != null && !_loadAdCompleter!.isCompleted) {

lib/ads/widgets/in_article_ad_loader_widget.dart

Lines changed: 60 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,6 @@ class _InArticleAdLoaderWidgetState extends State<InArticleAdLoaderWidget> {
9494
'${widget.slotType.name} or contextKey: ${widget.contextKey}. '
9595
'Re-loading ad.',
9696
);
97-
// Dispose of the old ad's resources before loading a new one.
98-
_adCacheService.removeAndDisposeAd(
99-
contextKey: oldWidget.contextKey,
100-
placeholderId: oldWidget.slotType.name,
101-
);
10297

10398
if (_loadAdCompleter != null && !_loadAdCompleter!.isCompleted) {
10499
// Complete normally to prevent crashes
@@ -117,12 +112,11 @@ class _InArticleAdLoaderWidgetState extends State<InArticleAdLoaderWidget> {
117112

118113
@override
119114
void dispose() {
120-
// Dispose of the ad's resources when the widget is permanently removed.
121-
_adCacheService.removeAndDisposeAd(
122-
contextKey: widget.contextKey,
123-
placeholderId: widget.slotType.name,
124-
);
125-
115+
// The ad is intentionally not disposed of here. The InlineAdCacheService
116+
// is responsible for managing the ad's lifecycle. The cache for an
117+
// article context is cleared when the user navigates away from the
118+
// article details page, which is handled by other parts of the app
119+
// (e.g., a higher-level widget's dispose method).
126120
if (_loadAdCompleter != null && !_loadAdCompleter!.isCompleted) {
127121
// Complete normally to prevent crashes
128122
_loadAdCompleter!.complete();
@@ -140,34 +134,50 @@ class _InArticleAdLoaderWidgetState extends State<InArticleAdLoaderWidget> {
140134
Future<void> _loadAd() async {
141135
_loadAdCompleter = Completer<void>();
142136

143-
if (!mounted) return;
137+
if (!mounted) {
138+
if (_loadAdCompleter?.isCompleted == false) {
139+
_loadAdCompleter!.complete();
140+
}
141+
return;
142+
}
144143

145144
// Use the slotType as the placeholderId within the context of the article.
146145
final placeholderId = widget.slotType.name;
147-
InlineAd? loadedAd;
148146

149-
// For all ad platforms, try to get from cache first.
150-
// The "AdWidget is already in the Widget tree" error is prevented by using
151-
// a unique contextKey (the article ID) for each article page.
152-
final cachedAd = _adCacheService.getAd(
147+
// TODO(fulleni): This should be sourced from RemoteConfig.
148+
const adMaxAge = Duration(minutes: 5);
149+
150+
var cachedAd = _adCacheService.getAd(
153151
contextKey: widget.contextKey,
154152
placeholderId: placeholderId,
155153
);
156154

157155
if (cachedAd != null) {
158-
_logger.info(
159-
'Using cached in-article ad for context "${widget.contextKey}" '
160-
'and slot: ${widget.slotType.name}',
161-
);
162-
if (!mounted) return;
163-
setState(() {
164-
_loadedAd = cachedAd;
165-
_isLoading = false;
166-
});
167-
if (_loadAdCompleter?.isCompleted == false) {
168-
_loadAdCompleter!.complete();
156+
if (DateTime.now().difference(cachedAd.createdAt) > adMaxAge) {
157+
_logger.info(
158+
'Cached in-article ad for context "${widget.contextKey}" and slot '
159+
'${widget.slotType.name} is stale. Discarding and fetching new.',
160+
);
161+
_adCacheService.removeAndDisposeAd(
162+
contextKey: widget.contextKey,
163+
placeholderId: placeholderId,
164+
);
165+
cachedAd = null;
166+
} else {
167+
_logger.info(
168+
'Using fresh cached in-article ad for context "${widget.contextKey}" '
169+
'and slot: ${widget.slotType.name}',
170+
);
171+
if (mounted) {
172+
setState(() {
173+
_loadedAd = cachedAd;
174+
_isLoading = false;
175+
});
176+
}
177+
if (_loadAdCompleter?.isCompleted == false)
178+
_loadAdCompleter!.complete();
179+
return;
169180
}
170-
return;
171181
}
172182

173183
_logger.info(
@@ -180,7 +190,7 @@ class _InArticleAdLoaderWidgetState extends State<InArticleAdLoaderWidget> {
180190
final userRole = appBlocState.user?.appRole ?? AppUserRole.guestUser;
181191

182192
// Call AdService.getInArticleAd with the full AdConfig.
183-
loadedAd = await _adService.getInArticleAd(
193+
final loadedAd = await _adService.getInArticleAd(
184194
adConfig: widget.adConfig,
185195
adThemeStyle: widget.adThemeStyle,
186196
userRole: userRole,
@@ -199,27 +209,27 @@ class _InArticleAdLoaderWidgetState extends State<InArticleAdLoaderWidget> {
199209
ad: loadedAd,
200210
);
201211

202-
if (!mounted) return;
203-
setState(() {
204-
_loadedAd = loadedAd;
205-
_isLoading = false;
206-
});
207-
if (_loadAdCompleter?.isCompleted == false) {
208-
_loadAdCompleter!.complete();
212+
if (mounted) {
213+
setState(() {
214+
_loadedAd = loadedAd;
215+
_isLoading = false;
216+
});
209217
}
218+
if (_loadAdCompleter?.isCompleted == false)
219+
_loadAdCompleter!.complete();
210220
} else {
211221
_logger.warning(
212222
'Failed to load in-article ad for context "${widget.contextKey}" '
213223
'and slot: ${widget.slotType.name}. No ad returned.',
214224
);
215-
if (!mounted) return;
216-
setState(() {
217-
_hasError = true;
218-
_isLoading = false;
219-
});
220-
if (_loadAdCompleter?.isCompleted == false) {
221-
_loadAdCompleter!.complete();
225+
if (mounted) {
226+
setState(() {
227+
_hasError = true;
228+
_isLoading = false;
229+
});
222230
}
231+
if (_loadAdCompleter?.isCompleted == false)
232+
_loadAdCompleter!.complete();
223233
}
224234
} catch (e, s) {
225235
_logger.severe(
@@ -228,14 +238,13 @@ class _InArticleAdLoaderWidgetState extends State<InArticleAdLoaderWidget> {
228238
e,
229239
s,
230240
);
231-
if (!mounted) return;
232-
setState(() {
233-
_hasError = true;
234-
_isLoading = false;
235-
});
236-
if (_loadAdCompleter?.isCompleted == false) {
237-
_loadAdCompleter!.complete();
241+
if (mounted) {
242+
setState(() {
243+
_hasError = true;
244+
_isLoading = false;
245+
});
238246
}
247+
if (_loadAdCompleter?.isCompleted == false) _loadAdCompleter!.complete();
239248
}
240249
}
241250

0 commit comments

Comments
 (0)