Offline First Implementation for Learn Progress and Jobs List Page#3606
Offline First Implementation for Learn Progress and Jobs List Page#3606shubham1g5 wants to merge 64 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis pull request implements a comprehensive offline-first architecture for the Connect feature using a multi-phase approach. It introduces standardized data state modeling, policy-driven refresh semantics, app-scoped request deduplication, a Kotlin-based suspend networking layer, and a repository pattern orchestrating cache-first flows. Changes span data models, network layer (new Sequence Diagram(s)sequenceDiagram
participant Fragment
participant ViewModel
participant Repository
participant RequestMgr as RequestManager
participant NetworkClient as NetworkClient
participant API as Connect API
participant Database
Fragment->>ViewModel: loadOpportunities(forceRefresh)
ViewModel->>Repository: getOpportunities(forceRefresh)
Repository->>Repository: offlineFirstFlow()
Note over Repository: Check cache & policy
Repository->>Database: loadCache()
Database-->>Repository: cachedJobs
Repository->>Fragment: emit DataState.Loading
alt shouldRefresh == true
Repository->>RequestMgr: executeRequest(url)
alt Request in-flight for same URL
RequestMgr-->>Repository: await existing result
else New request
RequestMgr->>NetworkClient: getConnectOpportunities(user)
NetworkClient->>NetworkClient: getAuthorizationHeader()
NetworkClient->>API: GET /api/opportunity/ with auth
API-->>NetworkClient: ResponseBody
NetworkClient->>NetworkClient: parseResponse()
NetworkClient-->>RequestMgr: Result<OpportunitiesModel>
RequestMgr-->>Repository: Result<OpportunitiesModel>
end
alt Success
Repository->>Database: updateJobs()
Repository->>Repository: storeLastSyncTime()
Repository->>Fragment: emit DataState.Success(jobs)
else Network Error
Repository->>Fragment: emit DataState.Error(with cached fallback)
end
else shouldRefresh == false
Repository->>Fragment: emit DataState.Cached(jobs)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (5)
docs/plans/2026-03-09-claude-issue-implementer.md (1)
414-418: Consider adding language specification for linting compliance.The markdown linter flags this fenced code block as missing a language specification.
📝 Suggested fix
Add
textas the language identifier:-``` +```text GitHub UI → Actions → Claude Issue Implementer → Run workflow issue_number: 9999 max_retries: 2</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/plans/2026-03-09-claude-issue-implementer.mdaround lines 414 - 418,
The fenced code block that begins with "GitHub UI → Actions → Claude Issue
Implementer → Run workflow" is missing a language specifier; update the
triple-backtick fence to include a language identifier (usetext) so it
becomes ```text, ensuring the markdown linter stops flagging the block (locate
the block containing "issue_number: 9999" and "max_retries: 2" in the document).</details> </blockquote></details> <details> <summary>docs/plans/2026-03-09-claude-issue-implementer-design.md (1)</summary><blockquote> `48-58`: **Consider adding language specifications to fenced code blocks.** The markdown linter flags these blocks as missing language specifications. While the content is readable as-is, adding identifiers would improve linting compliance. <details> <summary>📝 Suggested language identifiers</summary> - Lines 48-58: Use `text` or `prompt` - Lines 94-97: Use `text` or `markdown` - Lines 102-116: Use `text` or `prompt` Example: ````markdown ```text You are implementing a GitHub issue... ```Also applies to: 94-97, 102-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-09-claude-issue-implementer-design.md` around lines 48 - 58, The fenced code blocks in the plan (the block starting "You are implementing a GitHub issue..." and the other fenced blocks later) are missing language identifiers and triggering the markdown linter; update each triple-backtick block by adding an explicit language tag such as text, prompt, or markdown (e.g., ```text or ```prompt) for the blocks containing the numbered steps and the other examples so the linter recognizes them (apply to the block with the implementation steps and the two subsequent example blocks referenced in the comment).app/unit-tests/src/org/commcare/connect/repository/ConnectRequestManagerTest.kt (1)
57-73: Replace the sleep-based coordination in these tests.These assertions depend on
delay(10/100/200)whileConnectRequestManagerdoes its work on a real background scope, soMainCoroutineRuledoes not make them deterministic. That makes the suite prone to CI timing noise. Prefer a handshake (CompletableDeferred, channel, latch) or an injected test dispatcher/scope instead.Also applies to: 121-131, 148-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/unit-tests/src/org/commcare/connect/repository/ConnectRequestManagerTest.kt` around lines 57 - 73, The test's timing relies on delay() to order concurrent calls to ConnectRequestManager.executeRequest (deferred1/deferred2) causing flakiness; replace the sleep-based coordination with a deterministic handshake such as CompletableDeferred or a Channel to signal when the slow request has reached the desired point (e.g., expose a suspend point inside the first executor lambda that completes a CompletableDeferred before continuing), or inject a TestCoroutineScope/TestDispatcher into ConnectRequestManager so you can control scheduling from the test; update the test bodies that touch callCount to await the handshake instead of using delay(10/100/200) (also apply the same replacement for the other occurrences around lines 121-131 and 148-161).app/src/org/commcare/connect/repository/ConnectSyncPreferences.kt (1)
54-66: Consider more robust endpoint key sanitization.The current sanitization
endpoint.replace("/", "_")could theoretically cause key collisions if two endpoints differ only by/vs_(e.g.,/foo/barand/foo_barwould both become_foo_bar).This is unlikely with well-designed API endpoints, but a safer approach would be URL encoding or a hash:
val key = KEY_LAST_SYNC_PREFIX + endpoint.hashCode() // or val key = KEY_LAST_SYNC_PREFIX + URLEncoder.encode(endpoint, "UTF-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/connect/repository/ConnectSyncPreferences.kt` around lines 54 - 66, The endpoint-to-key sanitization in storeLastSyncTime and getLastSyncTime currently uses endpoint.replace("/", "_") which can produce collisions (e.g., "/foo/bar" vs "/foo_bar"); update both functions to derive the prefs key deterministically and collision-free by replacing the simple replace with a robust strategy such as URL-encoding the endpoint (URLEncoder.encode(endpoint, "UTF-8")) or using a stable hash (endpoint.hashCode() or a SHA-based hex) appended to KEY_LAST_SYNC_PREFIX; ensure you update both functions and keep KEY_LAST_SYNC_PREFIX and prefs.edit()/prefs.getLong(...) usage intact so stored and retrieved keys match.app/unit-tests/src/org/commcare/fragments/connect/ConnectJobsListsFragmentTest.kt (1)
63-82: Complete the TODO test implementations or track them.These placeholder tests throw
NotImplementedErrorat runtime. If they're intentionally deferred, consider:
- Annotating with
@Ignoreto prevent test failures- Opening an issue to track completion
Would you like me to help implement these test cases or open an issue to track this work?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/unit-tests/src/org/commcare/fragments/connect/ConnectJobsListsFragmentTest.kt` around lines 63 - 82, The TODO test methods in ConnectJobsListsFragmentTest (testFragment_showsLoadingSpinner_onLoadingState, testFragment_showsJobList_onSuccessState, testFragment_showsJobList_onCachedState, testFragment_showsError_onErrorState, testFragment_showsCachedList_andErrorToast_whenErrorHasCachedData) currently throw NotImplementedError at runtime; either implement each test to post the appropriate DataState (Loading, Success, Cached, Error with/without cachedData) to the ViewModel and assert the UI (loading spinner visibility, RecyclerView item count, error UI/toast behavior) using the fragment test harness, or if you intend to defer work, annotate each of these test methods with `@Ignore` and open a tracking issue—choose one approach and apply it consistently to the listed test functions in ConnectJobsListsFragmentTest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/CommCareApplication.java`:
- Line 223: The session-start timestamp is only set on app process start via
ConnectSyncPreferences.Companion.getInstance(this).markSessionStart(), causing
new logins within the same process to inherit the previous user's session; call
markSessionStart() when a user successfully logs in (or immediately after a
logout/login user-switch) so ConnectSyncPreferences.shouldRefresh() compares
against the new user's session boundary — locate the login success handler
(e.g., LoginActivity.onLoginSuccess,
AuthenticationManager.handleSuccessfulLogin, or the code path that completes a
user switch) and invoke
ConnectSyncPreferences.Companion.getInstance(context).markSessionStart() there.
In `@app/src/org/commcare/connect/network/connect/ConnectNetworkClient.kt`:
- Around line 83-97: Add an explicit rethrow for CancellationException before
the broad catch(Exception) in the try/catch that currently catches
LoginInvalidatedException, IOException and Exception (the block that returns
Result.success/Result.failure after parsing the response); import
kotlinx.coroutines.CancellationException and add a catch (e:
CancellationException) { throw e } immediately before the catch (e: Exception)
so coroutine cancellation is not swallowed by ConnectNetworkClient's error
handling.
In `@app/src/org/commcare/connect/network/ConnectNetworkHelper.kt`:
- Around line 44-58: The form-encoding branch builds local headers but never
copies them into the caller-visible outputHeaders, so generated
Content-Type/Content-Length are lost; update the useFormEncoding branch (around
useFormEncoding, params, requestBody, headers, ModernHttpRequester.getPostBody,
getContentHeadersForXFormPost) to merge or replace outputHeaders with the
locally created headers before returning (e.g., clear outputHeaders and putAll
from headers or assign the generated entries into outputHeaders) and keep the
existing call to addVersionHeader(headers, version) appropriately applied to the
final outputHeaders.
- Around line 89-110: The coroutine started in suspendCancellableCoroutine that
calls ConnectSsoHelper.retrievePersonalIdToken can hang because some upstream
paths may not invoke any callback; update the TokenCallback handlers
(tokenRetrieved, tokenUnavailable, tokenRequestDenied) to use
continuation.tryResume(...) instead of continuation.resume(...) so resume
attempts are safe and won't deadlock if callbacks are missing or late; locate
the suspendCancellableCoroutine block that constructs
ConnectSsoHelper.TokenCallback and replace resume calls with tryResume, keeping
the same Result payloads and error types (HttpUtils.getCredential,
ConnectApiException/PersonalIdOrConnectApiErrorCodes).
In `@app/src/org/commcare/connect/repository/ConnectRepository.kt`:
- Around line 48-52: The current loadCache lambda drops a valid cached empty
list by using takeIf { it.isNotEmpty() }; change it to return the result of
ConnectJobUtils.getCompositeJobs(...) directly (i.e., do not call takeIf) so
that an explicitly synced-but-empty list is preserved and emitted to the UI;
update the loadCache lambda that calls ConnectJobUtils.getCompositeJobs(context,
ConnectJobRecord.STATUS_ALL_JOBS, null) to return the list as-is.
- Around line 103-118: The current catch-all in ConnectRepository around the
ConnectRequestManager.executeRequest call hides special cases; remove the
blanket catch (Exception) and instead explicitly handle and rethrow
CancellationException and LoginInvalidatedException before converting failures
to DataState.Error (so they propagate to CommCareExceptionHandler), while still
mapping other failures to DataState.Error.from(cause, cachedData); reference the
try block around ConnectRequestManager.executeRequest,
DataState.Error.from(...), and the exceptions CancellationException and
LoginInvalidatedException to locate where to add the conditional rethrows and
where to retain the error-to-DataState conversion.
In `@app/src/org/commcare/connect/viewmodel/ViewModelExtensions.kt`:
- Around line 12-16: The collectInto helper currently launches a fire-and-forget
coroutine via viewModelScope.launch; change its signature to return the Job
(e.g., fun <T> ViewModel.collectInto(...): Job) and return the Job produced by
viewModelScope.launch so callers (e.g., fragments invoking collectInto for
flows) can cancel prior collections before starting new ones; update the
implementation inside collectInto to return the launch result instead of Unit.
- Around line 18-20: The current catch block in ViewModelExtensions.kt
indiscriminately converts all exceptions to DataState.Error (the .catch {
exception -> liveData.value = DataState.Error.from(exception) } handler); change
it to rethrow RuntimeException (to preserve fail-fast for parsing/JSON issues)
and only convert IOException and ConnectApiException into DataState.Error via
DataState.Error.from(...). In practice, update the .catch lambda to check the
exception type: if it's a RuntimeException rethrow it, otherwise if it's an
IOException or ConnectApiException set liveData.value =
DataState.Error.from(exception), and for any other unexpected Throwable rethrow
or wrap as appropriate so only transport/domain failures become error states.
In `@app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java`:
- Around line 111-115: The current logic clears the UI on any refresh failure
when error.getCachedData() returns null; instead, add a persisted flag on
ConnectJobsListsFragment (e.g., boolean hasRenderedJobs or hasJobListData) that
you set true inside setJobListData(…) after a successful render, and change both
branches where cachedData == null to call navigateFailure() only if that flag is
false (initial load); otherwise do nothing so the last good job list remains
visible. Ensure the flag is updated in setJobListData and referenced in both
places handling error.getCachedData() == null.
In
`@app/unit-tests/src/org/commcare/connect/viewmodel/ConnectLearningProgressViewModelTest.kt`:
- Around line 49-68: The test uses the removed runBlockingTest API; update the
test method testLoadLearningProgress_postsLoadingThenSuccess to call
mainCoroutineRule.runTest instead of mainCoroutineRule.runBlockingTest when
invoking viewModel.loadLearningProgress(mockJob) (and do the same for the other
occurrence mentioned around line 81) so the coroutine test uses
kotlinx-coroutines-test 1.7.3 compatible runTest; keep the rest of the logic and
observers (viewModel.learningProgress.observeForever, assertions) unchanged.
In
`@app/unit-tests/src/org/commcare/fragments/connect/ConnectLearningProgressFragmentTest.kt`:
- Around line 64-78: The four test stubs
(testFragment_showsLoading_onLoadingState,
testFragment_updatesUI_onSuccessState, testFragment_updatesUI_onCachedState,
testFragment_showsErrorToast_onErrorState_withCachedData) contain TODO() and
will throw NotImplementedError; either implement their Robolectric assertions or
temporarily disable them by annotating each with `@Ignore` to avoid failing the
suite—locate those functions in ConnectLearningProgressFragmentTest and replace
TODO() with real test bodies that post the appropriate DataState to the fragment
and assert UI/toast behavior, or add `@Ignore` above each test method if you
prefer to defer implementation.
In `@docs/claude/plans/2026-02-23-offline-first-connect-architecture.md`:
- Around line 1859-1862: The backward-compatibility note incorrectly names the
new coroutine client as ConnectApiClient; update the text to refer to the actual
new class ConnectNetworkClient while keeping the rest of the note intact, so it
reads that the new ConnectNetworkClient coroutine-based client coexists
alongside the old ConnectApiClient/legacy base-URL holder and ConnectApiHandler
callback pattern.
In `@docs/claude/plans/2026-03-09-offline-first-tdd-iteration-design.md`:
- Around line 102-104: Update the description to state that RefreshPolicy is an
abstract class (not a sealed class) to match the PR objective; keep DataState
described as a sealed class. Specifically, change the Phase 0/Phase 1a wording
to say "DataState is a sealed class and RefreshPolicy is an abstract class (made
abstract for testability), and Phase 1a tests reference these types to provide
an implicit compile-time contract check." Ensure the terms RefreshPolicy and
DataState are used exactly so readers can map to the implementation.
- Around line 112-116: Change the fenced code block that currently starts with
``` and contains the lines "Phase 0 → Phase 1a → Phase 1b → Phase 1.5a → Phase
1.5b ..." to specify a language (use ```text) so markdownlint MD040 is
satisfied; update the three-line dependency-chain block fence surrounding the
"Phase 0 → Phase 1a → Phase 1b → Phase 1.5a → Phase 1.5b / → Phase 2a → Phase 2b
→ Phase 3a → Phase 3b / → Phase 4a → Phase 4b → Phase 5" content to begin with
```text and end with ```.
- Around line 108-110: Reword the "Dependency Chain" rule so it ties
dependencies to each phase's own success criteria rather than blanket "compile +
tests passing"; replace the sentence under "Dependency Chain" with wording like
"Each phase depends on prior phases meeting their defined success criteria
(e.g., red phases may be considered complete even if tests fail, while green
phases require compilation and tests to pass)" to match the a‑phase / Red/Green
semantics referenced in the document.
In `@docs/claude/research/2026-02-23-offline-first-network-architecture.md`:
- Around line 42-44: The fenced code blocks containing the plain architecture
diagrams (e.g., the block with "Fragment/Activity → CommCareTask → Connector →
Background Work → Callback Delivery" and the similar block later) are missing a
language tag and triggering markdownlint MD040; add a language identifier (use
"text") to the opening fence of each of those triple-backtick blocks so they
become ```text ... ``` to satisfy the linter and keep the diagrams unchanged.
In `@docs/plans/2026-03-09-claude-issue-implementer-design.md`:
- Line 84: Replace the `<PLACEHOLDER_DEVICE>` in the line "Trigger Espresso
build (device: `<PLACEHOLDER_DEVICE>`)” with a valid BrowserStack device
identifier (for example "Google Pixel 6-12.0" or "Samsung Galaxy S21-12.0"),
ensuring the chosen device string matches BrowserStack's exact catalog format;
update the document entry to the concrete device value and validate by
cross-checking the BrowserStack device list to confirm OS version and device
name match.
In `@docs/plans/2026-03-09-claude-issue-implementer.md`:
- Around line 329-330: The BRANCH and PR extraction uses GNU-only grep -oP which
fails on macOS; update the BRANCH=$(...) and PR=$(...) assignments to use a
BSD-compatible parser (e.g., sed or POSIX shell tools) instead of grep
-oP—replace the grep -oP 'BRANCH=\K\S+' and grep -oP 'PR=\K[0-9]+' usages with
equivalent sed or a cut/tr based extraction so the BRANCH and PR variables are
set portably on macOS.
- Line 52: The design doc currently references the wrong secret name: replace
all occurrences of BROWSERSTACK_ACCESS_KEY with BROWSERSTACK_PASSWORD so the doc
matches the implementation; update any example environment lines (e.g., the env
var expansion snippet) and any text or tables that mention
BROWSERSTACK_ACCESS_KEY to use BROWSERSTACK_PASSWORD and verify the doc’s
wording explains that this secret maps to the implementation’s
BROWSERSTACK_PASSWORD variable.
---
Nitpick comments:
In `@app/src/org/commcare/connect/repository/ConnectSyncPreferences.kt`:
- Around line 54-66: The endpoint-to-key sanitization in storeLastSyncTime and
getLastSyncTime currently uses endpoint.replace("/", "_") which can produce
collisions (e.g., "/foo/bar" vs "/foo_bar"); update both functions to derive the
prefs key deterministically and collision-free by replacing the simple replace
with a robust strategy such as URL-encoding the endpoint
(URLEncoder.encode(endpoint, "UTF-8")) or using a stable hash
(endpoint.hashCode() or a SHA-based hex) appended to KEY_LAST_SYNC_PREFIX;
ensure you update both functions and keep KEY_LAST_SYNC_PREFIX and
prefs.edit()/prefs.getLong(...) usage intact so stored and retrieved keys match.
In
`@app/unit-tests/src/org/commcare/connect/repository/ConnectRequestManagerTest.kt`:
- Around line 57-73: The test's timing relies on delay() to order concurrent
calls to ConnectRequestManager.executeRequest (deferred1/deferred2) causing
flakiness; replace the sleep-based coordination with a deterministic handshake
such as CompletableDeferred or a Channel to signal when the slow request has
reached the desired point (e.g., expose a suspend point inside the first
executor lambda that completes a CompletableDeferred before continuing), or
inject a TestCoroutineScope/TestDispatcher into ConnectRequestManager so you can
control scheduling from the test; update the test bodies that touch callCount to
await the handshake instead of using delay(10/100/200) (also apply the same
replacement for the other occurrences around lines 121-131 and 148-161).
In
`@app/unit-tests/src/org/commcare/fragments/connect/ConnectJobsListsFragmentTest.kt`:
- Around line 63-82: The TODO test methods in ConnectJobsListsFragmentTest
(testFragment_showsLoadingSpinner_onLoadingState,
testFragment_showsJobList_onSuccessState,
testFragment_showsJobList_onCachedState, testFragment_showsError_onErrorState,
testFragment_showsCachedList_andErrorToast_whenErrorHasCachedData) currently
throw NotImplementedError at runtime; either implement each test to post the
appropriate DataState (Loading, Success, Cached, Error with/without cachedData)
to the ViewModel and assert the UI (loading spinner visibility, RecyclerView
item count, error UI/toast behavior) using the fragment test harness, or if you
intend to defer work, annotate each of these test methods with `@Ignore` and open
a tracking issue—choose one approach and apply it consistently to the listed
test functions in ConnectJobsListsFragmentTest.
In `@docs/plans/2026-03-09-claude-issue-implementer-design.md`:
- Around line 48-58: The fenced code blocks in the plan (the block starting "You
are implementing a GitHub issue..." and the other fenced blocks later) are
missing language identifiers and triggering the markdown linter; update each
triple-backtick block by adding an explicit language tag such as text, prompt,
or markdown (e.g., ```text or ```prompt) for the blocks containing the numbered
steps and the other examples so the linter recognizes them (apply to the block
with the implementation steps and the two subsequent example blocks referenced
in the comment).
In `@docs/plans/2026-03-09-claude-issue-implementer.md`:
- Around line 414-418: The fenced code block that begins with "GitHub UI →
Actions → Claude Issue Implementer → Run workflow" is missing a language
specifier; update the triple-backtick fence to include a language identifier
(use `text`) so it becomes ```text, ensuring the markdown linter stops flagging
the block (locate the block containing "issue_number: 9999" and "max_retries: 2"
in the document).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c3e5522a-4cc2-4e43-8d48-77f57c30165a
📒 Files selected for processing (32)
app/src/org/commcare/CommCareApplication.javaapp/src/org/commcare/connect/network/ConnectApiService.ktapp/src/org/commcare/connect/network/ConnectNetworkHelper.javaapp/src/org/commcare/connect/network/ConnectNetworkHelper.ktapp/src/org/commcare/connect/network/base/ConnectApiException.ktapp/src/org/commcare/connect/network/connect/ConnectNetworkClient.ktapp/src/org/commcare/connect/network/connect/models/ConnectOpportunitiesResponseModel.ktapp/src/org/commcare/connect/network/connect/models/LearningAppProgressResponseModel.ktapp/src/org/commcare/connect/repository/ConnectRepository.ktapp/src/org/commcare/connect/repository/ConnectRequestManager.ktapp/src/org/commcare/connect/repository/ConnectSyncPreferences.ktapp/src/org/commcare/connect/repository/DataState.ktapp/src/org/commcare/connect/repository/RefreshPolicy.ktapp/src/org/commcare/connect/viewmodel/ConnectJobsListViewModel.ktapp/src/org/commcare/connect/viewmodel/ConnectLearningProgressViewModel.ktapp/src/org/commcare/connect/viewmodel/ViewModelExtensions.ktapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.javaapp/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.javaapp/unit-tests/src/org/commcare/connect/network/connect/ConnectNetworkClientTest.ktapp/unit-tests/src/org/commcare/connect/repository/ConnectRepositoryTest.ktapp/unit-tests/src/org/commcare/connect/repository/ConnectRequestManagerTest.ktapp/unit-tests/src/org/commcare/connect/repository/ConnectSyncPreferencesTest.ktapp/unit-tests/src/org/commcare/connect/viewmodel/ConnectJobsListViewModelTest.ktapp/unit-tests/src/org/commcare/connect/viewmodel/ConnectLearningProgressViewModelTest.ktapp/unit-tests/src/org/commcare/fragments/connect/ConnectJobsListsFragmentTest.ktapp/unit-tests/src/org/commcare/fragments/connect/ConnectLearningProgressFragmentTest.ktdocs/claude/plans/2026-02-23-offline-first-connect-architecture.mddocs/claude/plans/2026-03-09-offline-first-tdd-iteration-design.mddocs/claude/plans/2026-03-10-offline-first-tdd-implementation.mddocs/claude/research/2026-02-23-offline-first-network-architecture.mddocs/plans/2026-03-09-claude-issue-implementer-design.mddocs/plans/2026-03-09-claude-issue-implementer.md
💤 Files with no reviewable changes (1)
- app/src/org/commcare/connect/network/ConnectNetworkHelper.java
| - `assembleCommcareReleaseAndroidTest` → `app-commcare-release-androidTest.apk` | ||
| 2. Upload app APK to Browserstack App Automate | ||
| 3. Upload test APK to Browserstack App Automate | ||
| 4. Trigger Espresso build (device: `<PLACEHOLDER_DEVICE>`) |
There was a problem hiding this comment.
Replace placeholder with actual BrowserStack device configuration.
The <PLACEHOLDER_DEVICE> needs to be replaced with a valid BrowserStack device identifier before implementation.
Would you like me to help identify an appropriate device configuration from BrowserStack's device catalog (e.g., Google Pixel 6-12.0 or similar)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-09-claude-issue-implementer-design.md` at line 84, Replace
the `<PLACEHOLDER_DEVICE>` in the line "Trigger Espresso build (device:
`<PLACEHOLDER_DEVICE>`)” with a valid BrowserStack device identifier (for
example "Google Pixel 6-12.0" or "Samsung Galaxy S21-12.0"), ensuring the chosen
device string matches BrowserStack's exact catalog format; update the document
entry to the concrete device value and validate by cross-checking the
BrowserStack device list to confirm OS version and device name match.
There was a problem hiding this comment.
Design doc placeholder. The BrowserStack device config will be specified when the workflow is actually implemented.
There was a problem hiding this comment.
Rate Limit Exceeded
@shubham1g5 have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 23 seconds before sending another message.
| : "${MAX_RETRIES:?}" | ||
| : "${ANTHROPIC_API_KEY:?}" | ||
| : "${BROWSERSTACK_USERNAME:?}" | ||
| : "${BROWSERSTACK_PASSWORD:?}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check BrowserStack documentation for the correct authentication secret name
echo "Searching for BrowserStack API authentication environment variable names..."
# Search in existing scripts that use BrowserStack
rg -n "BROWSERSTACK" --type=py --type=sh scripts/Repository: dimagi/commcare-android
Length of output: 449
🏁 Script executed:
# Find the design document and check its content
fd -e md . docs/plans/ | head -20Repository: dimagi/commcare-android
Length of output: 173
🏁 Script executed:
# Check the specific file being reviewed
cat -n docs/plans/2026-03-09-claude-issue-implementer.md | sed -n '50,55p'Repository: dimagi/commcare-android
Length of output: 256
🏁 Script executed:
# Search for BROWSERSTACK references in the docs directory
rg -n "BROWSERSTACK" docs/Repository: dimagi/commcare-android
Length of output: 1330
Update design doc to match implementation: BROWSERSTACK_ACCESS_KEY should be BROWSERSTACK_PASSWORD.
The design document (line 124 in the design plan file) specifies BROWSERSTACK_ACCESS_KEY, but the implementation plan and actual code use BROWSERSTACK_PASSWORD. This mismatch will cause workflows to fail—users following the design doc will set up the wrong secret name in GitHub Actions. Update the design document to align with the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-09-claude-issue-implementer.md` at line 52, The design doc
currently references the wrong secret name: replace all occurrences of
BROWSERSTACK_ACCESS_KEY with BROWSERSTACK_PASSWORD so the doc matches the
implementation; update any example environment lines (e.g., the env var
expansion snippet) and any text or tables that mention BROWSERSTACK_ACCESS_KEY
to use BROWSERSTACK_PASSWORD and verify the doc’s wording explains that this
secret maps to the implementation’s BROWSERSTACK_PASSWORD variable.
There was a problem hiding this comment.
No occurrences of BROWSERSTACK_ACCESS_KEY found in the file — it already uses BROWSERSTACK_PASSWORD throughout.
There was a problem hiding this comment.
Rate Limit Exceeded
@shubham1g5 have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 11 seconds before sending another message.
| BRANCH=$(echo "$RESULT_LINE" | grep -oP 'BRANCH=\K\S+') | ||
| PR=$(echo "$RESULT_LINE" | grep -oP 'PR=\K[0-9]+') |
There was a problem hiding this comment.
Critical: grep -oP is incompatible with macOS BSD grep.
The workflow runs on macos-latest (line 270), but macOS ships with BSD grep which doesn't support the -P flag (Perl regex). This parsing logic will fail at runtime.
🔧 Portable fix using sed
Replace the grep commands with BSD-compatible alternatives:
- RESULT_LINE=$(grep "^RESULT:" /tmp/claude-impl.log | tail -1)
- BRANCH=$(echo "$RESULT_LINE" | grep -oP 'BRANCH=\K\S+')
- PR=$(echo "$RESULT_LINE" | grep -oP 'PR=\K[0-9]+')
+ RESULT_LINE=$(grep "^RESULT:" /tmp/claude-impl.log | tail -1)
+ BRANCH=$(echo "$RESULT_LINE" | sed -n 's/.*BRANCH=\([^ ]*\).*/\1/p')
+ PR=$(echo "$RESULT_LINE" | sed -n 's/.*PR=\([0-9]*\).*/\1/p')Alternatively, use a simpler approach with cut and tr:
- RESULT_LINE=$(grep "^RESULT:" /tmp/claude-impl.log | tail -1)
- BRANCH=$(echo "$RESULT_LINE" | grep -oP 'BRANCH=\K\S+')
- PR=$(echo "$RESULT_LINE" | grep -oP 'PR=\K[0-9]+')
+ RESULT_LINE=$(grep "^RESULT:" /tmp/claude-impl.log | tail -1)
+ BRANCH=$(echo "$RESULT_LINE" | tr ' ' '\n' | grep '^BRANCH=' | cut -d= -f2)
+ PR=$(echo "$RESULT_LINE" | tr ' ' '\n' | grep '^PR=' | cut -d= -f2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BRANCH=$(echo "$RESULT_LINE" | grep -oP 'BRANCH=\K\S+') | |
| PR=$(echo "$RESULT_LINE" | grep -oP 'PR=\K[0-9]+') | |
| BRANCH=$(echo "$RESULT_LINE" | sed -n 's/.*BRANCH=\([^ ]*\).*/\1/p') | |
| PR=$(echo "$RESULT_LINE" | sed -n 's/.*PR=\([0-9]*\).*/\1/p') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-09-claude-issue-implementer.md` around lines 329 - 330,
The BRANCH and PR extraction uses GNU-only grep -oP which fails on macOS; update
the BRANCH=$(...) and PR=$(...) assignments to use a BSD-compatible parser
(e.g., sed or POSIX shell tools) instead of grep -oP—replace the grep -oP
'BRANCH=\K\S+' and grep -oP 'PR=\K[0-9]+' usages with equivalent sed or a cut/tr
based extraction so the BRANCH and PR variables are set portably on macOS.
There was a problem hiding this comment.
This YAML lives in a design/plan doc, not a deployed workflow file. The macOS compatibility concern is noted and will be addressed when this workflow is actually implemented.
There was a problem hiding this comment.
Rate Limit Exceeded
@shubham1g5 have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 11 seconds before sending another message.
- Rethrow CancellationException before generic Exception catch in ConnectNetworkClient - Fix version header missing from outputHeaders in form-encoding path of ConnectNetworkHelper - Rethrow CancellationException in offlineFirstFlow to preserve coroutine cancellation - Add @ignore to unimplemented TODO() test stubs in ConnectLearningProgressFragmentTest - Fix ConnectApiClient -> ConnectNetworkClient in architecture docs - Update RefreshPolicy description from sealed to abstract class in TDD design doc - Add text language tags to fenced code blocks in plan and research docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rethrow CancellationException before generic Exception catch in ConnectNetworkClient - Fix version header missing from outputHeaders in form-encoding path of ConnectNetworkHelper - Rethrow CancellationException in offlineFirstFlow to preserve coroutine cancellation - Add @ignore to unimplemented TODO() test stubs in ConnectLearningProgressFragmentTest - Fix ConnectApiClient -> ConnectNetworkClient in architecture docs - Update RefreshPolicy description from sealed to abstract class in TDD design doc - Add text language tags to fenced code blocks in plan and research docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
shubham1g5
force-pushed
the
claude_plan_offline_loading
branch
from
2dc2625 to
d35d6f0
Compare
shubham1g5
force-pushed
the
claude_plan_offline_loading
branch
from
06c191d to
2cda9d4
Compare
- Rethrow CancellationException before generic Exception catch in ConnectNetworkClient - Fix version header missing from outputHeaders in form-encoding path of ConnectNetworkHelper - Rethrow CancellationException in offlineFirstFlow to preserve coroutine cancellation - Add @ignore to unimplemented TODO() test stubs in ConnectLearningProgressFragmentTest - Fix ConnectApiClient -> ConnectNetworkClient in architecture docs - Update RefreshPolicy description from sealed to abstract class in TDD design doc - Add text language tags to fenced code blocks in plan and research docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
shubham1g5
force-pushed
the
claude_plan_offline_loading
branch
from
2cda9d4 to
0140d7e
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…se pairs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ctRequestManager Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ager - Add ConnectSyncPreferences singleton for per-endpoint sync timestamps and session-based refresh policies - Add ConnectRequestManager singleton for app-scoped request deduplication that survives ViewModel cancellation - Add ConnectApiException stub to unblock compilation of DataState.kt (full implementation in Phase 1.5b) - Initialize session start time in CommCareApplication.onCreate() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents visibility issues when cancelAll() reassigns scope from a different thread than executeRequest() reads it on. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use toResponseBody/toMediaType Kotlin extensions instead of deprecated ResponseBody.create/MediaType.parse. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onnectNetworkHelper, ConnectApiException Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kotlin.coroutines.resume is provided by suspendCancellableCoroutine's CancellableContinuation receiver without an explicit import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use body.use{} in executeApiCall to close ResponseBody on parse failure,
preventing OkHttp connection pool exhaustion
- Restore kotlin.coroutines.resume import (needed for CancellableContinuation
in kotlinx-coroutines 1.7.x — removing it broke compilation)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
shubham1g5
force-pushed
the
claude_plan_offline_loading
branch
from
0140d7e to
4fb5cf3
Compare
shubham1g5
changed the title
shubham1g5
added
the
High Risk
…, align color with figma
…ately on calling observeDataState
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
shubham1g5
force-pushed
the
claude_plan_offline_loading
branch
from
78b7e90 to
e9cd81d
Compare
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
So far looking pretty good! I'm only about halfway through the changes. I'll have to come back and pick up where I left off later.
I also had a couple of suggestions that I dropped on the demo in Slack. I'll post those comments here too so that they don't get lost in the weeds:
Something I noticed during code review - the landing page is sometimes fully blank on initial load (in my second video it happens very fast, but the first time I tried it the blank page lasted for a few seconds), and at other times it flashes the message telling my that I haven't been invited to any opportunities which is incorrect.
What if we added some different UI there to the middle of the page during initial sync?
Screen_Recording_20260409_141445_CommCare.Debug.mp4
Screen_Recording_20260409_141604_CommCare.Debug.mp4
Here's a better video showing what I mean:
Screen_Recording_20260409_142150_CommCare.Debug.mp4
Another small thing I noticed:
There is a gap between the status bar and the rest of the UI which causes some wonky UI. Can we eliminate that gap? (see right under the status bar as I scroll in this video)
Screen_Recording_20260409_142847_CommCare.Debug.mp4
There was a problem hiding this comment.
This is a great opportunity to refactor BaseApiHandler.onFailure() to take this object as a parameter. For example:
// This:
abstract fun onFailure(
apiException: ConnectApiException
)
// Not this:
abstract fun onFailure(
errorCode: PersonalIdOrConnectApiErrorCodes,
t: Throwable?,
)
There was a problem hiding this comment.
that method gets called from bunch of other network calls in PersonalIdApiHandler where we are not using ConnectApiException , moreover we are not utilizing BaseApiHandler in the refactored code at all to justify any changes there.
There was a problem hiding this comment.
That's a good point, we could rename ConnectApiException to PersonalIdOrConnectApiException similarly to what we did with the current PersonalIdOrConnectApiErrorCodes class.
But also, I'm cool with this suggestion being optional. I figured the refactor would be good for consistency across our codebase since we are introducing a brand new class for exceptions.
| @JvmStatic | ||
| fun addVersionHeader( | ||
| headers: HashMap<String, String>, | ||
| version: String?, |
There was a problem hiding this comment.
Nit: suggest making the version non-null here and removing the null check
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Things look great so far! I made it through the main code changes, need to finish reviewing the test code tomorrow but wanted to submit comments in the meantime.
| <string name="connect_last_update">Updated: %s</string> | ||
| <string name="connect_last_synced">Last synced: %s</string> | ||
| <string name="connect_sync_successful">Last synced: Just Now</string> | ||
| <string name="connect_sync_failed">Failed to sync updates, Last synced: %s</string> |
There was a problem hiding this comment.
The capitalized 'L' here isn't grammatically correct, suggest we use lower-case. This applies to the translations of this string also.
| private var loadLearnProgressJob: Job? = null | ||
|
|
||
| fun loadLearningProgress( | ||
| job: ConnectJobRecord, |
There was a problem hiding this comment.
Nit: Noting the increasing confusion with the word "job" given the Job class being used. I wonder if we should try to avoid using "job" to refer to an "opportunity", and start using the correct terminology to avoid confusion
| private val _opportunities = MutableLiveData<DataState<List<ConnectJobRecord>>>() | ||
| val opportunities: LiveData<DataState<List<ConnectJobRecord>>> = _opportunities | ||
|
|
||
| private var getOpportunitiesJob: Job? = null |
There was a problem hiding this comment.
In reference to this comment: I'm glad this isn't called getJobsJob! Good usage of "opportunities" instead here
|
|
||
| fun show(message: String) { | ||
| fun showError(message: String) { | ||
| errorView.setBackgroundColor(errorView.context.getColor(R.color.connect_warning_color)) |
There was a problem hiding this comment.
Nit: This line is always paired with showBar, thinking that function could take the color as an additional input and make sure it always gets set(plus a little less code repetition).
| showBar(message, showOffline = false, autoDismiss = true) | ||
| } | ||
|
|
||
| fun isErrorShowing(): Boolean = errorView.visibility == View.VISIBLE |
There was a problem hiding this comment.
Given that this component can now also show a success message (and it seems like it could easily grow to support more messaging in the future), I think some renaming may be in order:
- errorView >> messageView
- isErrorShowing >> isMessageShowing
- Maybe even TopBarErrorViewController >> TopBarMessageViewController
| @Volatile | ||
| private var instance: ConnectNetworkClient? = null | ||
|
|
||
| fun getInstance(context: Context): ConnectNetworkClient = |
There was a problem hiding this comment.
Nit: context isn't used, can be removed
| import androidx.annotation.VisibleForTesting | ||
| import kotlinx.coroutines.CancellationException | ||
| import okhttp3.ResponseBody | ||
| import org.commcare.CommCareApplication |
There was a problem hiding this comment.
Nit: Unused. Generally looks like this file could use a lint run, seeing lots of long lines also.
There was a problem hiding this comment.
yes, I plan to run lint on the whole PR before merge as per process, this one addresses the import you pointed out
|
|
||
| import android.content.Context | ||
| import androidx.annotation.VisibleForTesting | ||
| import kotlinx.coroutines.CancellationException |
| job.learnings = responseModel.connectJobLearningRecords | ||
| job.learningModulesCompleted = responseModel.connectJobLearningRecords.size | ||
| job.assessments = responseModel.connectJobAssessmentRecords | ||
| updateJobLearnProgress(CommCareApplication.instance(), job) |
There was a problem hiding this comment.
Wondering if updating the DB should go into LearningAppProgressResponseParser to be consistent with other calls? The opportunities and release toggles parsers each handle their relevant DB updates, and the delivery progress update happens in ConnectJobHelper (the only caller for that API).
Or thinking from the other direction, maybe it makes more sense for parsers only to focus on parsing and not DB storage, whereas this repository makes sense as the one to handle storing all parsed results. In other words, move the other DB storage function from the parsers to the corresponding functions here.
You have a great eye! Resolved by 4574bd4 |
|
First of all, +1 for the changes done; it looks like a lot of thought process has been given. |
| ) { | ||
| showOfflineIndicator() | ||
| } else { | ||
| showError(getString(R.string.connect_sync_failed, getRelativeLastSyncTime())) |
There was a problem hiding this comment.
This is not handling TOKEN_UNAVAILABLE_ERROR and TOKEN_DENIED_ERROR. This just shows the error on the bar, but no action is taken based on error.
| when (state) { | ||
| is DataState.Loading -> { | ||
| hideError() | ||
| showLoading() |
There was a problem hiding this comment.
This will always show the loading on the top whenever the API is called. Do we have a use case where it's required to silently load the data?
There was a problem hiding this comment.
not at the moment, current we do want to always show the loading bar if the request is initiated from a UI component to let the user know that a refresh is in progress.
| hideLoading() | ||
| if (state.errorCode == BaseApiHandler.PersonalIdOrConnectApiErrorCodes.NETWORK_ERROR && | ||
| !ConnectivityStatus.isNetworkAvailable(requireContext()) | ||
| ) { | ||
| showOfflineIndicator() | ||
| } else { | ||
| showError(getString(R.string.connect_sync_failed, getRelativeLastSyncTime())) | ||
| } |
There was a problem hiding this comment.
In case of error, this is always showing an error, and the main fragment is never aware that an error has occurred. I am thinking of a case where it's required to inform the main fragment to take some action. Should we create this mechanism?
There was a problem hiding this comment.
can you give examples of use cases you are thinking of here ? Think it would be an easy add on to implement when we need it.
| ) { | ||
| liveData.observe(viewLifecycleOwner) { state -> | ||
| if (!isAdded) return@observe | ||
| if (lastDataState == null && (state is DataState.Success || state is DataState.Error)) { |
There was a problem hiding this comment.
There is no DataState.Cached in the condition mentioned, but the comment is referring to it. I am also not sure if we need this condition, as the cache is the first thing emitted before error and success.
There was a problem hiding this comment.
as the cache is the first thing emitted before error and success
This is a weird caveat of using view models and flows, the flow components immediately emits a "cached" state (which is the last emitted state by that flow) when the fragment is resumed i.e if I open a fragment and that results into a DataState.Success as part of the network refresh and then I go out and go back in the fragment and re-subscribe to the liveData, it immediately emits the DataState.Success ahead of DataState.Cached
| onSuccess: DataStateConsumer<T>, | ||
| ) { | ||
| liveData.observe(viewLifecycleOwner) { state -> | ||
| if (!isAdded) return@observe |
There was a problem hiding this comment.
Because collectInto is viewModelScope, this condition should not be true anytime.
| forceRefresh: Boolean = false, | ||
| policy: RefreshPolicy = RefreshPolicy.SESSION_AND_TIME_BASED(), | ||
| ): Flow<DataState<List<ConnectJobRecord>>> = | ||
| offlineFirstFlow( |
There was a problem hiding this comment.
Can it have other options in the future except for offlineFirstFlow?
There was a problem hiding this comment.
I would think so depending on API.
| import retrofit2.http.Path | ||
|
|
||
| interface ConnectApiService { | ||
| @GET("/api/opportunity/") |
There was a problem hiding this comment.
Can we use ApiEndPoints constants here?
There was a problem hiding this comment.
| isNewSession || isStale | ||
| } | ||
|
|
||
| else -> throw IllegalArgumentException("Unknown refresh policy: $policy") |
There was a problem hiding this comment.
This will crash the application, is that intentional?
There was a problem hiding this comment.
definitely intentional as any other policy clearly signifies a programming error here.
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Just got done reviewing everything, it's looking great! Just had a few suggestions.
Also, I noticed that local builds are failing with these changes. It's an unresolved reference error:
...org/commcare/connect/ConnectJobHelper.kt:88:11 Unresolved reference 'getLearningAppProgress'.
| headers: HashMap<String, String>, | ||
| version: String, | ||
| ) { | ||
| if (version != null) { |
There was a problem hiding this comment.
This null check is no longer necessary
| } | ||
|
|
||
| protected fun setWaitDialogEnabled(enabled: Boolean) { | ||
| val activity: Activity? = getActivity() |
There was a problem hiding this comment.
Very minor nit: specifying the type here is unnecessary
| val activity: Activity? = getActivity() | |
| val activity = getActivity() |
| setJobListData(cached); | ||
| }, | ||
| success -> { | ||
| corruptJobs.clear(); |
There was a problem hiding this comment.
It seems that nothing is ever added to the corrupted jobs list but yet we still clear it. Is the corruptJobs list still necessary? Perhaps we can delete that variable from this class entirely?
There was a problem hiding this comment.
Can we add a couple new tests here for happy paths?
| @Config(application = CommCareTestApplication::class) | ||
| @RunWith(AndroidJUnit4::class) | ||
| class ConnectRepositoryTest { | ||
| private val context = ApplicationProvider.getApplicationContext<CommCareTestApplication>() |
There was a problem hiding this comment.
Looks like context is never used in this class
| private val context = ApplicationProvider.getApplicationContext<CommCareTestApplication>() |
| assertEquals(DataState.Loading, results[0]) | ||
| assertTrue(results[1] is DataState.Success) |
There was a problem hiding this comment.
Can we either use assertEquals both times here or use assertTrue both times for consistency
There was a problem hiding this comment.
Think using them interchangeably is pretty common and doesn't really introduce any readability concerns in the code, IMO we should be open to these sorts of small stylistic inconsistencies. Thoughts ?
There was a problem hiding this comment.
The example here is very minor and does not make a huge difference, but as a team we should be very cautious of making a habit of creating stylistic inconsistencies throughout the codebase. When our code follows the same conventions throughout, it requires a bit less cognitive load and readers are more focused on understanding what the code does vs how a particular dev writes code. There are more benefits to maintaining consistent coding style, but that reason is the biggest for me
With that being said, we don't need a coding standard or anything extreme like that for this example, but it makes sense to have back-to-back statements like this be consistent
|
|
||
| @Test | ||
| fun testLoadOpportunities_postsError_onFailure() { | ||
| val cachedJobs = listOf(mockk<ConnectJobRecord>()) |
There was a problem hiding this comment.
Seems that this variable is never used
| val cachedJobs = listOf(mockk<ConnectJobRecord>()) |
| assertTrue(results.any { it is DataState.Loading }) | ||
| assertTrue(results.last() is DataState.Success) | ||
| assertEquals(secondJobs, (results.last() as DataState.Success).data) | ||
| assertTrue(results.none { it is DataState.Success && (it as DataState.Success).data == firstJobs }) |
There was a problem hiding this comment.
Nit: the cast is redundant here
| assertTrue(results.none { it is DataState.Success && (it as DataState.Success).data == firstJobs }) | |
| assertTrue(results.none { it is DataState.Success && it.data == firstJobs }) |
4 participants
Product Description
https://dimagi.atlassian.net/browse/CCCT-2262
Implements offline first behaviour with data refresh indicators as per the design spec
Current UX:
data.refresh.indicators.mp4
There are a couple things from the design spec which are not part of this PR and will be handled in seperate tickets -
In general, the PR focuses more on establishing undelying offline first architecture rather than UX of these pages.
Technical Summary
The Tech Spec provides a great overview of the implementation and a more detailed Claude Plan this implementaion is based on is available here
Safety Assurance
Safety story
QA Plan
Planning to get this QA'ed once the delivery progress implementation implementation is complete.
Labels and Review
Review Notes