Refactor arbitrary size values in SelectContentPage.spec.js tests for clarity#14782
Conversation
Export selectContentTransferredChannel from makeStore.js so tests can derive expected display values from the same source of truth rather than hard-coding untraceble literals like '1,000 5 GB' and '2,000 95 MB'. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace magic literals with named variables derived from selectContentTransferredChannel throughout: channel metadata in the thumbnail/title test, newVersion/newerVersion/draftNewerVersion for version comparison tests, and fix the negative version test which was checking for version 1000 instead of the actual installed version. Also remove a stale TODO comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
added
APP: Device
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean test-only refactor that achieves the single-source-of-truth goal from issue #14781. CI passes (all 10 frontend tests green).
Praise: The extracted selectContentTransferredChannel constant is the right approach — future fixture changes propagate automatically to both the store and the test assertions. The silent bug fix in the "no notification" test (asserting on a version that was never actually rendered) is a real correctness improvement.
Suggestion: channelsOnDevice[0] in makeStore.js (lines 46–52, outside the diff) still duplicates the same on_device_resources: 2000 and on_device_file_size: 95189556 values from selectContentTransferredChannel. Since the PR's stated goal is a single source of truth, consider updating it to { ...selectContentTransferredChannel, available: true }. Otherwise a future change to the fixture values here would silently diverge from channelsOnDevice. Not blocking — the constant's primary purpose is test assertions — but worth noting given the stated goal.
Nitpick: See inline comment on .toString() vs .toLocaleString() asymmetry.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| // derive expected display values (resource counts, file sizes) from the same source of truth. | ||
| export const selectContentTransferredChannel = { | ||
| ...allChannels[0], | ||
| on_device_resources: 2000, |
There was a problem hiding this comment.
Suggestion: channelsOnDevice[0] (lines 46–52, above this hunk) sets the same on_device_resources: 2000 and on_device_file_size: 95189556 independently. Since this constant is the declared source of truth, consider using it there too:
const channelsOnDevice = [
{ ...selectContentTransferredChannel, available: true },
...
];Not blocking, but leaving the duplicate untouched means a future change to these values here won't automatically flow to channelsOnDevice.
| renderComponent({ store }); | ||
| expect(screen.getAllByRole('row')[2]).toHaveTextContent( | ||
| `${summaryTr.$tr('onDeviceRow')} 0 0 B`, | ||
| `${summaryTr.$tr('onDeviceRow')} ${onDeviceResources.toString()} ${bytesForHumans(onDeviceFileSize)}`, |
There was a problem hiding this comment.
Nitpick: The non-zero resource count tests above use .toLocaleString(), matching the component's $formatNumber(..., { useGrouping: true }) call. This line uses .toString() — both produce "0" so it doesn't affect correctness, but using .toLocaleString() here too would make the pattern uniform and make the intent explicit.
There was a problem hiding this comment.
changed toString() to toLocaleString()
- Move selectContentTransferredChannel before channelsOnDevice so channelsOnDevice[0] can reference it as the source of truth rather than duplicating the on_device_resources/on_device_file_size values - Use toLocaleString() consistently for resource counts, matching the pattern used by the non-zero resource tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Delta review of PR #14782. No UI files changed — Phase 3 skipped.
See CI notes below.
The PR successfully meets all issue requirements: it identifies allChannels[0] as the source of the arbitrary fixture values, exports a selectContentTransferredChannel constant as a single source of truth (eliminating three-way duplication), and replaces hardcoded display strings with derived values using bytesForHumans and .toLocaleString(). Both prior review findings (the channelsOnDevice[0] spread and the .toString()-to-.toLocaleString() uniformity) have been resolved. No blocking issues were found. Three nitpicks are noted: newerVersion = 5 is numerically less than the base channel's version (10) which could confuse readers; a useful contextual comment was stripped from a still-present test; and two tests destructure fixture data after renderComponent rather than before, breaking the Arrange-Act-Assert order. All existing test coverage is preserved.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
|
|
||
| // The transferredChannel used by makeSelectContentPageStore, exported so tests can | ||
| // derive expected display values (resource counts, file sizes) from the same source of truth. | ||
| export const selectContentTransferredChannel = { |
There was a problem hiding this comment.
praise: Exporting selectContentTransferredChannel as a single source of truth is well-executed. It eliminates three previously-duplicated copies of on_device_resources: 2000 / on_device_file_size: 95189556 (in channelsOnDevice[0], in transferredChannel, and implicitly in the test expectations), and the spread of allChannels[0] means channel metadata like name, version, and description also flow through without duplication.
| const { total_resources, total_file_size } = selectContentTransferredChannel; | ||
| expect(screen.getAllByRole('row')[1]).toHaveTextContent( | ||
| `${summaryTr.$tr('totalSizeRow')} 1,000 5 GB`, | ||
| `${summaryTr.$tr('totalSizeRow')} ${total_resources.toLocaleString()} ${bytesForHumans(total_file_size)}`, |
There was a problem hiding this comment.
praise: Using bytesForHumans(total_file_size) and bytesForHumans(onDeviceFileSize) in assertions instead of hardcoded strings like "5 GB" / "95 MB" / "0 B" is the right approach. Tests now stay correct if the formatting utility ever changes its rounding or unit thresholds, and the intent — "what the component renders" — is made explicit through the same function the component uses.
|
|
||
| //Add translations strings to these tests & test these & commit the changes. | ||
| describe('draft channel (installed version = 0)', () => { | ||
| const newerVersion = 5; |
There was a problem hiding this comment.
nitpick: newerVersion = 5 is less than allChannels[0].version (10), so the name is slightly misleading in context. It is correctly newer than the installed version (0) that these tests set via setInstalledVersion(store, 0), but a reader unfamiliar with the fixture may find it odd that a "newer version" is numerically smaller than the channel's base version. A comment or a different value (e.g. 15) that is unambiguously newer than 10 would remove the ambiguity.
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('hides ContentTreeViewer when installed version > 0 and newer version available on Studio', () => { |
There was a problem hiding this comment.
nitpick: The comment // Preserve existing non-draft behavior was removed from line 144 (the hides ContentTreeViewer test). The code it described is still present and unchanged — the project conventions say to only remove comments when the described code is deleted or the comment is provably incorrect. The comment clarified why this non-draft test lives inside the draft channel describe block, which is genuinely useful context.
There was a problem hiding this comment.
The test's description is sufficient to provide all the context
There was a problem hiding this comment.
You're right — the test description provides the context readers need. Withdrawing this finding.
| }); | ||
|
|
||
| it('shows the total size of the channel', () => { | ||
| renderComponent({ store }); |
There was a problem hiding this comment.
nitpick: renderComponent({ store }) is called on line 60 before the destructuring of selectContentTransferredChannel on line 61. Correctness is unaffected (the values come from the imported constant, not the rendered DOM), but the conventional order is to arrange all data first, then render, then assert. The same pattern appears in the onDeviceRow test (lines 68–69). Moving the destructuring above renderComponent would match the Arrange-Act-Assert pattern used elsewhere in this file.
Build Artifacts
Smoke test screenshot |
- Change newerVersion from 5 to 15 so it is unambiguously greater than both the installed version (0) and the base channel version (10) - Move selectContentTransferredChannel destructuring above renderComponent in the totalSizeRow and onDeviceRow tests to follow Arrange-Act-Assert Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All five prior findings are resolved or acknowledged — thorough follow-through on every round of feedback.
CI: 8/9 checks complete, no failures; 1 still in progress. No UI files changed — Phase 3 skipped.
Prior findings
3 prior findings unchanged (2 resolved from round 1, 1 acknowledged/withdrawn from round 3).
Newly resolved:
newerVersion = 5naming (nitpick) ← was UNADDRESSED — changed to15, unambiguously greater than both the installed version (0) and the base channel version (10)- AAA ordering in
totalSize/onDevicetests (nitpick) ← was UNADDRESSED — destructuring moved aboverenderComponent
5/5 prior findings resolved or acknowledged. 0 re-raised below.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| }); | ||
|
|
||
| it('shows the total size of the channel', () => { | ||
| const { total_resources, total_file_size } = selectContentTransferredChannel; |
There was a problem hiding this comment.
praise: Extracting the fixture values before renderComponent now cleanly follows Arrange-Act-Assert — data setup is part of Arrange, not suspended mid-Act.
|
|
||
| //Add translations strings to these tests & test these & commit the changes. | ||
| describe('draft channel (installed version = 0)', () => { | ||
| const newerVersion = 15; |
There was a problem hiding this comment.
praise: 15 is unambiguously greater than both the installed version (0) and the base channel version (10) — no more risk of a reader misreading "newer" as relative to the channel's base version field.
There was a problem hiding this comment.
15 is unambiguously greater than 0 and 10!! :)
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
thanks, @akolson -- nice cleanup
3 participants
Summary
SelectContentPage.spec.jscontained hard-coded size strings (1,000 5 GB,2,000 95 MB,0 0 B) with no visible connection to the store data that produces them, making the tests hard to understand and maintain.This PR:
selectContentTransferredChannelfrommakeStore.jsas a named constant so it serves as a single source of truth for both the store factory and test assertionsbytesForHumansand.toLocaleString()/.toString()NEW_VERSIONdeclarations), fixes a silent bug where the negative version test was asserting against'1000'instead of the actual channel version, introducesdraftNewerVersionto name the repeated5across the draft-channel tests, and removes a stale TODO commentAll 10 tests pass unchanged.
References
Closes #14781
Related to #14656
Reviewer guidance
Tests only — no production code was modified.
To verify:
pnpm test-jest kolibri/plugins/device/frontend/views/__tests__/SelectContentPage.spec.jsAI usage
Claude Code (Sonnet 4.6) was used to implement the refactor. I reviewed each change for correctness, engaged critically with the approach (e.g. choosing
bytesForHumansover hard-coded strings, catching the wrong-version bug in the negative test), and ran the test suite after each step to verify nothing broke.