Skip to content

fix: Use TSelectedFields for knex loader order by method#424

Merged
wschurman merged 1 commit intomainfrom
wschurman/02-06-fix_use_tselectedfields_for_knex_loader_order_by_method
Feb 12, 2026
Merged

fix: Use TSelectedFields for knex loader order by method#424
wschurman merged 1 commit intomainfrom
wschurman/02-06-fix_use_tselectedfields_for_knex_loader_order_by_method

Conversation

@wschurman
Copy link
Member

@wschurman wschurman commented Feb 6, 2026

Why

Noticed while creating an upcoming pagination PR that loaders should use TSelectedFields for orderBy clauses. This was an accidental omission when the selected fields concept was added.

Succinctly, one should only be able to order entities by their fields rather than by all their underlying table's fields.

How

Create new orderBy clause type that lives at the loader level. It is fully compatible (subset or equal to) the database orderBy clause, so calling the data manager methods directly with the arguments is by design.

Test Plan

yarn tsc

@codecov
Copy link

codecov bot commented Feb 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6629767) to head (2b671cb).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##              main      #424    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          108       108            
  Lines        14785     14869    +84     
  Branches      1288       745   -543     
==========================================
+ Hits         14785     14869    +84     
Flag Coverage Δ
integration 22.40% <92.95%> (+0.40%) ⬆️
unittest 96.46% <88.02%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wschurman wschurman force-pushed the wschurman/02-06-fix_use_tselectedfields_for_knex_loader_order_by_method branch from 844397b to c475a0e Compare February 9, 2026 23:28
@wschurman wschurman force-pushed the wschurman/02-01-feat_add_sql_template_and_loader_method branch 2 times, most recently from 674d0b3 to b8ff387 Compare February 9, 2026 23:31
@wschurman wschurman force-pushed the wschurman/02-06-fix_use_tselectedfields_for_knex_loader_order_by_method branch from c475a0e to 9de79ed Compare February 9, 2026 23:31
@wschurman wschurman changed the base branch from wschurman/02-01-feat_add_sql_template_and_loader_method to graphite-base/424 February 9, 2026 23:57
@wschurman wschurman force-pushed the wschurman/02-06-fix_use_tselectedfields_for_knex_loader_order_by_method branch from 9de79ed to bacc551 Compare February 10, 2026 00:01
@graphite-app graphite-app bot changed the base branch from graphite-base/424 to main February 10, 2026 00:02
@wschurman wschurman force-pushed the wschurman/02-06-fix_use_tselectedfields_for_knex_loader_order_by_method branch from bacc551 to 8bc9c11 Compare February 10, 2026 00:02
@wschurman wschurman marked this pull request as ready for review February 10, 2026 00:12
@wschurman wschurman requested a review from quinlanj February 10, 2026 00:12
Copy link
Member Author

wschurman commented Feb 12, 2026

Merge activity

  • Feb 12, 1:26 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Feb 12, 1:26 AM UTC: @wschurman merged this pull request with Graphite.

@wschurman wschurman merged commit a9d09f4 into main Feb 12, 2026
5 checks passed
@wschurman wschurman deleted the wschurman/02-06-fix_use_tselectedfields_for_knex_loader_order_by_method branch February 12, 2026 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments