Skip to content

Bug: qfc-collection-conversation-index-defects #470

Description

@drmoisan
  • Work Mode: full-bug

Summary

Three related index and null-guard defects in QfcCollectionController's conversation-expansion
path: a -1 index used to subscript _itemGroups, a slot-reservation count that can disagree with
the number of members actually inserted, and an unguarded dereference that precedes its own null
check.

Environment

  • OS/version: n/a (logic defects, reproducible wherever QuickFiler runs)
  • Python version: n/a
  • Command/flags used: n/a
  • Data source or fixture: QuickFiler/Controllers/QfcCollectionController.cs

Steps to Reproduce

Defect 1 - ToggleGroupConv(string) can index _itemGroups[-1] (:1733-1766)

  1. At :1743, indexOriginal is -1 when the original message has already been removed.
  2. That routes to PromoteFirstChild(originalId, ref childCount) at :1970.
  3. PromoteFirstChild calls FindIndex at :1972. If no group carries
    ConvOriginID == originalId, that also returns -1.
  4. Line 1975 then evaluates _itemGroups[-1].ItemViewer, throwing ArgumentOutOfRangeException.
  5. ChangeConversationSilently(indexOriginal, true) at :1749 fails the same way for the same
    reason.

Defect 2 - EnumerateConversationMembers count mismatch (:1875-1922)

  1. ToggleUnGroupConv reserves insertCount = conversationCount - 1 slots at :1823 and
    :1827-1829.
  2. EnumerateConversationMembers iterates insertions.Count at :1888-1889, a count derived
    independently from resolver.ConversationItems.SameFolder at :1883-1886.
  3. If the resolver returns more members than conversationCount - 1, then
    _itemGroups[i + insertionIndex] at :1893 walks past the reserved slots and re-initializes
    groups that already hold other messages.
  4. If it returns fewer, the empty placeholder QfcItemGroups created at :2008 are left with a
    null ItemController, which the next RenumberGroups at :2068 dereferences.

Defect 3 - SetVisualDigits inconsistent null handling (:130-146)

  1. Line 140 dereferences grp.ItemController.ItemNumberDigits with no guard.
  2. Lines 141-142 guard the same object: grp.ItemController?.ItemNumber... ?? 0.ToString(format).
  3. Since ItemController genuinely can be null (see Defect 2), line 140 throws before the guard on
    line 141 is ever evaluated.

Expected Behavior

  1. A -1 index should be handled explicitly rather than used to subscript _itemGroups.
  2. The number of slots reserved and the number of members inserted should be derived from a single
    source, or reconciled before insertion.
  3. SetVisualDigits should guard ItemController consistently across all three reads.

Actual Behavior

  1. ArgumentOutOfRangeException when a conversation's original message is gone and no child carries
    the matching ConvOriginID.
  2. Either existing groups are silently overwritten, or placeholder groups with a null
    ItemController cause a NullReferenceException in RenumberGroups.
  3. NullReferenceException at line 140 whenever ItemController is null.

Logs / Screenshots

Impact / Severity

  • Blocker
  • High
  • Medium
  • Low

All three throw rather than corrupt, so the failure is visible. Defect 2's overwrite branch is the
most serious because it can silently re-initialize a group holding a different message.

Source

From: docs/features/potential/2026-08-07-qfc-collection-conversation-index-defects.md

Activity

  1. drmoisan commented on Aug 31, 2026

    @drmoisan
    OwnerAuthor

    Closing as delivered.

    All three defects reported in #470 were implemented through the consolidated #468 feature:

    • Missing conversation originals are handled without indexing _itemGroups[-1].
    • Conversation insertion counts are derived and reconciled against the resolved member list.
    • SetVisualDigits skips groups without an item controller or viewer.

    The changes merged into the epic integration branch through #636 and reached main through #673. Nine MSTest regression tests in QfcCollectionControllerDefects468ConversationTests.cs cover the missing-original paths, insertion resolution and count reconciliation, empty insertions, helper structure, and null controller/viewer handling. PR #636's final verification reported 6,581/6,581 tests passing.

    #470 remained open because #636 targeted a non-default integration branch, so its closing reference did not close the issue. The final integration PR did not include #470 in its auto-close list. No separate #470 implementation remains. Closing as completed by #636 and #673.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions