Skip to content

fix(channels): use ReorderableListView.onReorder; gate CI on stable too#142

Merged
krazyjakee merged 2 commits into
masterfrom
fix/138-reorder-onreorder
Jun 14, 2026
Merged

fix(channels): use ReorderableListView.onReorder; gate CI on stable too#142
krazyjakee merged 2 commits into
masterfrom
fix/138-reorder-onreorder

Conversation

@krazyjakee

@krazyjakee krazyjakee commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Fixes #138.

Problem

lib/features/spaces/views/accord_home_channels.dart called ReorderableListView.builder with onReorderItem:, which is not a parameter on Flutter's built-in ReorderableListView — it requires onReorder:. On stable this is two hard flutter analyze errors (missing_required_argument + undefined_named_parameter), so flutter analyze is non-clean on master and any build touching the file fails.

Fix

  • Renamed onReorderItem: → onReorder:.
  • The two APIs differ in newIndex semantics: onReorderItem reported the target index in post-removal coordinates, while Flutter's onReorder reports it pre-removal (the dragged slot is still counted). Added the canonical if (newIndex > oldIndex) newIndex -= 1; at the top of _onReorder so the existing logic — which indexes into the post-removal list — stays correct and drag-down moves don't land one slot too low.
  • Updated the now-inaccurate comment.

Verified clean with stable 3.38.1 (the toolchain in the issue):

Analyzing accord_home_channels.dart...
No issues found!

Why it reached master, and the CI guard

The error landed in #107 and CI was green — because the analyze gate only ran the pinned beta channel, which tolerated onReorderItem. A channel-skew break sailed through.

This PR matrixes the analyze job over [beta, stable] (fail-fast: false), so a widget/SDK API that's valid on one channel but a hard error on the other now fails the merge gate. Cache keys include the channel so the legs don't clobber each other.

Note: this makes stable a hard blocker, including for releases (release.yml reuses this gate via workflow_call). The tree is currently clean on stable. If you'd prefer stable be advisory (report-only) until the port settles, that's a one-line continue-on-error change — shout.

🤖 Generated with Claude Code

krazyjakee and others added 2 commits June 14, 2026 01:32
@krazyjakee @claude
fix(channels): use ReorderableListView.onReorder; gate CI on stable too
f23f621 
`accord_home_channels.dart` called `ReorderableListView.builder` with
`onReorderItem:`, which isn't a parameter on Flutter's built-in widget — it
requires `onReorder:`. This was a hard `flutter analyze` error on stable
(missing_required_argument + undefined_named_parameter), breaking any build
touching the file. Fixes #138.

The two APIs differ in `newIndex` semantics: `onReorderItem` reported the
target index in post-removal coordinates, while `onReorder` reports it
pre-removal (the dragged slot is still counted). Renamed the argument and
added the canonical `if (newIndex > oldIndex) newIndex -= 1;` so the existing
reorder logic — which indexes into the post-removal list — stays correct and
drag-down moves don't land one slot too low.

Why it reached master: the error landed in #107 and CI was green, because the
analyze gate only ran the pinned `beta` channel, which tolerated
`onReorderItem`. Matrix the analyze/test job over `[beta, stable]` so a
widget/SDK API that diverges between channels fails the gate instead of
shipping. Cache keys now include the channel so the legs don't collide.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude
review(#142): add reorder-index tests and clarify FLUTTER_CHANNEL com…
03041e5 
…ment

- Add 7 unit tests in channel_sort_test.dart covering the canonical
  `if (newIndex > oldIndex) newIndex -= 1` adjustment that fixes #138:
  drag-down by one slot (no-op), drag-down multiple slots, drag-down to
  last position, drag-up variants, and a regression guard that documents
  the exact wrong result produced without the fix.
- Clarify the FLUTTER_CHANNEL env var comment in ci.yml: the analyze job
  now uses matrix.channel (beta + stable), so the env var only controls
  the build job — the previous comment implied it still drove analyze.

https://claude.ai/code/session_0171kkokchuut5V7nmtibzLc
@krazyjakee Claude

krazyjakee commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Code Review — findings and fixes (commit 03041e5)

Severity ranking

🟡 Medium — No tests for the index-fix (fixed)

The core bug fix (if (newIndex > oldIndex) newIndex -= 1) is the right canonical pattern, but it had zero test coverage. A future refactor that removes or misplaces the decrement would silently reintroduce the drag-down-lands-one-slot-too-low bug.

Fixed: Added 7 unit tests to test/features/channels/channel_sort_test.dart in a new reorder index adjustment group:

  • drag-down by one slot (the "no-op" case — item stays in place)
  • drag-down two slots
  • drag-down to last position (Flutter passes newIndex == itemCount)
  • drag-up by one slot, drag-up to first, drag-up multiple slots
  • regression guard that documents the exact wrong list produced without the decrement, so reviewers can see at a glance what the bug looked like

The helpers (_adjustReorder / _applyReorder) are pure functions defined locally in the test file — no production-code changes required.

🔵 Low — FLUTTER_CHANNEL env var comment was misleading (fixed)

After the matrix change, the analyze job no longer reads ${{ env.FLUTTER_CHANNEL }} — it reads ${{ matrix.channel }} instead. The env var is still needed (the build job still uses it at line 182), but the top-level comment implied it controlled both jobs.

Fixed: Updated the comment to state explicitly that FLUTTER_CHANNEL is now only for the build job and that analyze uses the matrix.

ℹ️ Info — Pre-existing _flatten ordering inconsistency (not fixed, out of scope)

_ChannelDragListState._flatten places uncategorized channels before all categories; _ChannelReorderState._flatten (in accord_channel_reorder.dart) places them after. This inconsistency predates this PR and isn't introduced or worsened by it. Worth a follow-up to align the two, but it's out of scope here.

The PR's core changes (onReorderItem → onReorder + canonical decrement + matrix CI) are correct. No issues found with the logic itself.

Generated by Claude Code

@krazyjakee krazyjakee merged commit 01a6c3a into master Jun 14, 2026
3 checks passed
@krazyjakee krazyjakee deleted the fix/138-reorder-onreorder branch June 14, 2026 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Build error on master: accord_home_channels.dart uses undefined onReorderItem / missing onReorder

2 participants

@krazyjakee @claude