Skip to content

fix(comms): remove limit on get_channels endpoint#990

Merged
synoet merged 1 commit intomainfrom
synoet/remove-limit-from-get-channels
Jan 14, 2026
Merged

fix(comms): remove limit on get_channels endpoint#990
synoet merged 1 commit intomainfrom
synoet/remove-limit-from-get-channels

Conversation

@synoet
Copy link
Copy Markdown
Contributor

@synoet synoet commented Jan 14, 2026

removes the default limit

@synoet synoet requested a review from a team as a code owner January 14, 2026 01:14
Copy link
Copy Markdown
Member

@whutchinson98 whutchinson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in teo we trust

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 14, 2026

Code review

Test Coverage Required

The PR changes database query behavior by making the limit parameter optional, but no tests validate the new limit: None behavior.

CLAUDE.md violation:

"Always run tests between changes that involve changes to db queries"

"When making changes to a db crate you should always update tests, and run prepare"

Source: rust/cloud-storage/CLAUDE.md

Issue details:

  • The limit field changed from u32 to Option<u32> in models.rs
  • SQL query binding changed from i64 to Option<i64> in dynamic.rs
  • All existing tests in tests.rs use limit: Some(20) - none test the limit: None case

SQL behavior change:
When limit is None, PostgreSQL treats LIMIT NULL as "no limit" (returns all rows). This is a significant semantic change from the previous behavior where limits were always enforced (defaulted to 20, clamped between 20-100).

Recommendation:
Add a test case to validate the limit: None behavior. Example:

#[sqlx::test(
    migrator = "MACRO_DB_MIGRATIONS",
    fixtures(path = "../../../fixtures", scripts("channels"))
)]
async fn test_get_user_channels_dynamic_no_limit(pool: Pool<sqlx::Postgres>) {
    let user_id = MacroUserIdStr::parse_from_str("macro|user-1@test.com").unwrap();

    let params = GetChannelsRequest {
        macro_id: user_id.into_owned(),
        limit: None,  // Test the new behavior
        query: Query::Sort(SimpleSortMethod::UpdatedAt, None),
    }
    .into_params();

    let channels = get_user_channels_dynamic(&pool, &params).await.unwrap();

    // user-1 is a participant in 4 channels (A, B, C, D)
    assert_eq\!(channels.len(), 4, "Should return all channels when limit is None");
}

@synoet synoet merged commit d7cd7b0 into main Jan 14, 2026
39 checks passed
@synoet synoet deleted the synoet/remove-limit-from-get-channels branch January 14, 2026 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants