[CRD] Innovation library pagination#6115
Open
ccanos wants to merge 3 commits into
Open
Conversation
Contributor
WalkthroughThis PR implements cursor-based pagination for the Innovation Library collections (templates and innovation packs) by adding sequential rowId columns at the database layer, paginated DTO wrapper types, service methods that query and filter results, and new GraphQL resolver fields with relay-style pageInfo and total count metadata. ChangesInnovation Library Pagination Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 PR Metrics SummaryTitle: [CRD] Innovation library pagination
Flags
Thresholds{
"critical_loc": 200,
"simple_loc": 100,
"file_count": 10
} |
|
Schema Diff Summary: No blocking changes
Baseline branch: develop |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
src/library/library/dto/library.dto.templates.paginated.ts (1)
4-4: ⚡ Quick winUse the configured
@library/*alias for this DTO import.Line 4 introduces a relative import in a file that otherwise follows the repo alias convention. Please switch this to the
@library/...path so the DTO layer stays aligned withtsconfigresolution rules.As per coding guidelines: "Use module path aliases as configured in
tsconfig.json" and "Follow TypeScript path aliases defined intsconfig.jsonincluding@library/*".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/library/library/dto/library.dto.templates.paginated.ts` at line 4, Replace the local relative DTO import with the repo alias: change the import of ITemplateResult in library.dto.templates.paginated.ts from "./library.dto.template.result" to use the configured `@library` path (e.g. import { ITemplateResult } from '`@library/library/dto/library.dto.template.result`';) so the DTO layer follows the tsconfig path alias convention and resolves consistently.src/library/library/library.resolver.fields.ts (1)
12-15: ⚡ Quick winUse path aliases for the new DTO imports.
These new imports reintroduce relative module resolution in a file that otherwise follows the configured alias scheme. Please switch them to the appropriate alias for this library module instead of
./dto/....As per coding guidelines, "Use module path aliases as configured in
tsconfig.json(e.g.@domain/*,@services/*) for imports."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/library/library/library.resolver.fields.ts` around lines 12 - 15, The new DTO imports (PaginatedInnovationPacks, ITemplateResult, LibraryTemplatesFilterInput, PaginatedLibraryTemplateResults) currently use relative paths (./dto/...), so update them to use the project's configured path alias for library DTOs (e.g. the `@library` or equivalent alias configured in tsconfig) to match existing imports in this module; replace each './dto/...' import with the corresponding aliased module import for these four symbols so the file consistently uses path aliases.schema.graphql (1)
3144-3162: ⚡ Quick winDeprecate the unbounded
templatesfield now thattemplatesPaginatedis the superset.Both fields take the same filter, but only the new one enforces pagination. Leaving
templatesundecorated means generated clients and contract consumers get no migration signal and can keep bypassing the bounded path.♻️ Suggested schema change
templates( """Only return Templates of particular TemplateTypes""" filter: LibraryTemplatesFilterInput - ): [TemplateResult!]! + ): [TemplateResult!]! + `@deprecated`(reason: "Use templatesPaginated; remove after <set concrete date>.")As per coding guidelines, schema files must be reviewed for backwards compatibility and alignment with contract tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@schema.graphql` around lines 3144 - 3162, The unbounded templates field should be deprecated so clients are guided to use templatesPaginated; update the GraphQL schema by marking the templates field with a `@deprecated` directive (include a clear reason like "Use templatesPaginated which enforces pagination" and optionally a migration hint and timeline) so generated clients and contract consumers receive a migration signal while leaving templatesPaginated unchanged.specs/101-innovation-library-pagination/spec.md (1)
158-160: ⚡ Quick winClarify error handling for invalid cursors.
The edge case mentions "rejected with a clear error" for invalid/stale cursors, but FR-011 only references "standard pagination-argument validation." Consider adding a functional requirement explicitly defining the error type and behavior when a cursor references a deleted/non-existent item (e.g., should it be
EntityNotFoundExceptionas suggested in data-model.md line 86?).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/101-innovation-library-pagination/spec.md` around lines 158 - 160, The spec's pagination edge-case is ambiguous about the exact error for invalid/stale cursors; update the spec text around FR-011 and the cursor helper description to explicitly state the error type and behavior when a cursor references a deleted/non-existent item (e.g., throw or return an EntityNotFoundException as named in data-model.md) and describe whether this should be surfaced to clients or mapped to a standard pagination validation error; reference FR-011 and the "cursor helper" behavior in the same paragraph so implementers and tests know to expect EntityNotFoundException for stale/invalid cursor resolution.specs/101-innovation-library-pagination/quickstart.md (2)
31-44: ⚡ Quick winClarify cursor behavior across filter changes.
Example
#2usesafter: "<endCursor from#1>"with a different filter (types: [CALLOUT]), but example#1was unfiltered. Since filtering changes the result set, using a cursor from an unfiltered query with a filtered query may lead to unexpected results (the cursor might point to a non-CALLOUT template).Consider clarifying that:
- Cursors should be used within the same filter context, OR
- Show example
#2as a fresh first page with the filter:templatesPaginated(filter: { types: [CALLOUT] }, first: 5)(withoutafter)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/101-innovation-library-pagination/quickstart.md` around lines 31 - 44, Clarify cursor semantics for templatesPaginated: explain that cursors (the endCursor value) are only valid within the same filter/context and must not be reused after changing the filter; update example `#2` to either use the same filter as example `#1` when passing after: "<endCursor from `#1`>" or remove the after argument and show a fresh first page using templatesPaginated(filter: { types: [CALLOUT] }, first: 5). Mention the relevant symbols templatesPaginated, filter, after, and endCursor so readers know which values must remain consistent.
46-59: 💤 Low valueAdd backward pagination example.
The quickstart demonstrates forward pagination (
first/after) but doesn't include a backward pagination example (last/before). Consider adding an example showing how to retrieve the last N items or page backward from a cursor, sincePaginationArgssupports both directions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/101-innovation-library-pagination/quickstart.md` around lines 46 - 59, The quickstart only shows forward cursor pagination for innovationPacksPaginated; add a backward-pagination example using the last/before pattern to demonstrate retrieving the previous page (e.g., call innovationPacksPaginated with last: N and before: "<cursor>") and explain re-running with last: 10, before: "<startCursor>" to confirm previous distinct slice; reference the PaginationArgs behavior and include the pageInfo startCursor/hasPreviousPage fields so users can obtain the cursor for backward paging.specs/101-innovation-library-pagination/plan.md (2)
102-119: ⚡ Quick winConsider reordering tests earlier in implementation phases.
Phase 7 schedules tests after schema generation (Phase 6), but writing tests during service implementation (Phases 3-4) would catch issues earlier and align with TDD practices. Consider moving service tests to run immediately after each service method is implemented.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/101-innovation-library-pagination/plan.md` around lines 102 - 119, The plan schedules tests in Phase 7 after schema work, which delays feedback; update the implementation plan to add service-level tests immediately after each service implementation (move tests for getPaginatedListedInnovationPacks and getPaginatedTemplates into Phase 3 and Phase 4 respectively), so each service method is covered by specs right after it is implemented (and keep Phase 6 schema regen/tests for integration/schema checks); also note in Phase 7 keep the higher-level Decision 6 tests but mark them as integration/schema validation rather than primary service tests.
31-42: Add observability hooks for pagination queries.The plan states "otherwise read-only
rowId-keyset queries" with no mention of logging, metrics, or monitoring. Consider adding observability tasks to track:
- Pagination query response times (P50, P95, P99)
- Page size distributions
- Cursor errors/invalid requests
This aligns with Constitution Principle 5 ("Observability & Operational Readiness") and helps validate the "constant-cost" performance goal (line 39).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/101-innovation-library-pagination/plan.md` around lines 31 - 42, Add observability hooks for the new rowId keyset pagination: instrument the GraphQL list/resolver paths that implement "cursor pagination per docs/Pagination.md" (the pagination resolver/handler for innovation_pack and template list queries) to record request latency histograms (P50/P95/P99), a page-size distribution metric (counts by pageSize), and counters for cursor errors/invalid requests; also emit a debug log with the rowId keyset and requested pageSize on failures. Ensure metrics are recorded around the DB keyset query execution (the code that performs the read-only rowId keyset queries) so the "constant-cost" performance claim can be validated.specs/101-innovation-library-pagination/data-model.md (1)
19-21: ⚡ Quick winClarify backfill ordering semantics for existing data.
Line 19 states "SERIAL backfills existing rows with sequential values on ADD COLUMN," but PostgreSQL assigns SERIAL values to existing rows in physical scan order (typically storage order), which may not reflect insertion chronology. If "newest first" (FR-015 in spec.md) must be accurate for pre-existing data, consider documenting:
- Whether physical scan order is acceptable, OR
- Whether a follow-up UPDATE is needed to assign rowId based on a timestamp column (e.g.,
createdDate)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/101-innovation-library-pagination/data-model.md` around lines 19 - 21, Clarify that PostgreSQL's SERIAL backfill during ADD COLUMN assigns values in physical scan/storage order (not insertion time), so the current sentence about "sequential values" must note this nondeterminism relative to chronology; update the spec near the `rowId`/`SERIAL` text (references: rowId, SERIAL, ADD COLUMN, FR-016, FR-015) to state either (A) physical scan order is acceptable for pre-existing rows, or (B) require an explicit follow-up UPDATE that reassigns rowId deterministically using a timestamp column (e.g., createdDate) to ensure "newest first" semantics, and document which option the implementation chooses.src/migrations/1780407350716-AddRowIdToInnovationPackAndTemplate.ts (1)
6-18: ⚡ Quick winAdd inline rollback notes to this migration.
The
down()path is implemented, but the migration still needs rollback notes inline so the reversal steps are obvious during incident handling and future edits.As per coding guidelines, "Keep database migrations idempotent and include rollback notes inline as comments".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/migrations/1780407350716-AddRowIdToInnovationPackAndTemplate.ts` around lines 6 - 18, The migration AddRowIdToInnovationPackAndTemplate implements a working down() but lacks inline rollback notes; update the migration (methods up and down) to add clear commented rollback notes above each down() step describing why each constraint/column is dropped and any ordering or safety considerations (e.g., drop UNIQUE constraints before DROP COLUMN for "UQ_1fc637141a64418fb8220d46a84" on innovation_pack and "UQ_d9a42feddd7089b159246887910" on template), and add a short header comment in the class explaining the intent and idempotency expectations so incident responders can understand reversal steps quickly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@schema.graphql`:
- Around line 3126-3140: The paginated field innovationPacksPaginated currently
hardcodes newest-first and ignores the existing ordering contract exposed by
innovationPacks (InnovationPacksInput.orderBy), causing incompatible result
ordering; update the schema to accept an ordering argument (e.g., add an
orderBy: InnovationPacksOrderEnum or reuse InnovationPacksInput.orderBy) on
innovationPacksPaginated with the same default (number-of-templates desc) and
then update the resolver that resolves innovationPacksPaginated to honor that
orderBy when querying/sorting PaginatedInnovationPacks so its behavior is a
drop-in match for innovationPacks.
In `@specs/101-innovation-library-pagination/data-model.md`:
- Line 27: Update the "Down-migration" sentence to state the operation order
explicitly: first drop the UNIQUE constraint, then drop the column on both
tables; refer to the existing "Down-migration" line and mention the UNIQUE
constraint and the column so the text becomes "Down-migration: drop the UNIQUE
constraint first, then drop the column on both tables."
In `@specs/101-innovation-library-pagination/research.md`:
- Around line 83-88: The cursor description in Decision 3 is incorrect: update
the text so it states that cursors come from the keyset pagination helper's
rowId field rather than Template.id; specifically change the reference in the
step that mentions "cursors are template ids" to "cursors are rowId values
produced by getPaginationResults (keyset helper)", and ensure any examples or
mentions of Template.id are replaced with rowId to keep contract tests and
resolver docs aligned with getPaginationResults.
In `@specs/101-innovation-library-pagination/tasks.md`:
- Line 112: Update the PR description to complete task T020: document the new
additive paginated fields templatesPaginated and innovationPacksPaginated,
explain the rowId migration including that it is idempotent and reversible,
state that paginated fields are newest-first with no field-based ordering, and
include the Schema contract validation results produced by T017; ensure these
points are clearly labeled in the PR description so reviewers can verify T020 is
complete.
In `@src/library/library/library.service.ts`:
- Around line 274-276: Replace the dynamic template.id embedded in the thrown
RelationshipNotFoundException in library.service.ts with a static message string
and pass the template identifier via the exception's details parameter;
specifically update the RelationshipNotFoundException call (used in the method
where the throw occurs) to use a fixed message like "Unable to find listed
InnovationPack for Template" and supply { templateId: template.id } as the third
argument while keeping LogContext.LIBRARY as the context argument.
- Around line 284-295: clampPageSize assumes paginationArgs is non-null and
directly accesses paginationArgs.first/last, which throws when callers pass
null/undefined; make the function robust by normalizing the input (e.g., default
paginationArgs = {} or handle undefined at the top), then apply the same
clamping logic using the normalized object and MAX_LIBRARY_PAGE_SIZE; update the
signature of clampPageSize (and any callers if you change requiredness) to
accept PaginationArgs | undefined or provide a default value so accessing
.first/.last is safe.
---
Nitpick comments:
In `@schema.graphql`:
- Around line 3144-3162: The unbounded templates field should be deprecated so
clients are guided to use templatesPaginated; update the GraphQL schema by
marking the templates field with a `@deprecated` directive (include a clear reason
like "Use templatesPaginated which enforces pagination" and optionally a
migration hint and timeline) so generated clients and contract consumers receive
a migration signal while leaving templatesPaginated unchanged.
In `@specs/101-innovation-library-pagination/data-model.md`:
- Around line 19-21: Clarify that PostgreSQL's SERIAL backfill during ADD COLUMN
assigns values in physical scan/storage order (not insertion time), so the
current sentence about "sequential values" must note this nondeterminism
relative to chronology; update the spec near the `rowId`/`SERIAL` text
(references: rowId, SERIAL, ADD COLUMN, FR-016, FR-015) to state either (A)
physical scan order is acceptable for pre-existing rows, or (B) require an
explicit follow-up UPDATE that reassigns rowId deterministically using a
timestamp column (e.g., createdDate) to ensure "newest first" semantics, and
document which option the implementation chooses.
In `@specs/101-innovation-library-pagination/plan.md`:
- Around line 102-119: The plan schedules tests in Phase 7 after schema work,
which delays feedback; update the implementation plan to add service-level tests
immediately after each service implementation (move tests for
getPaginatedListedInnovationPacks and getPaginatedTemplates into Phase 3 and
Phase 4 respectively), so each service method is covered by specs right after it
is implemented (and keep Phase 6 schema regen/tests for integration/schema
checks); also note in Phase 7 keep the higher-level Decision 6 tests but mark
them as integration/schema validation rather than primary service tests.
- Around line 31-42: Add observability hooks for the new rowId keyset
pagination: instrument the GraphQL list/resolver paths that implement "cursor
pagination per docs/Pagination.md" (the pagination resolver/handler for
innovation_pack and template list queries) to record request latency histograms
(P50/P95/P99), a page-size distribution metric (counts by pageSize), and
counters for cursor errors/invalid requests; also emit a debug log with the
rowId keyset and requested pageSize on failures. Ensure metrics are recorded
around the DB keyset query execution (the code that performs the read-only rowId
keyset queries) so the "constant-cost" performance claim can be validated.
In `@specs/101-innovation-library-pagination/quickstart.md`:
- Around line 31-44: Clarify cursor semantics for templatesPaginated: explain
that cursors (the endCursor value) are only valid within the same filter/context
and must not be reused after changing the filter; update example `#2` to either
use the same filter as example `#1` when passing after: "<endCursor from `#1`>" or
remove the after argument and show a fresh first page using
templatesPaginated(filter: { types: [CALLOUT] }, first: 5). Mention the relevant
symbols templatesPaginated, filter, after, and endCursor so readers know which
values must remain consistent.
- Around line 46-59: The quickstart only shows forward cursor pagination for
innovationPacksPaginated; add a backward-pagination example using the
last/before pattern to demonstrate retrieving the previous page (e.g., call
innovationPacksPaginated with last: N and before: "<cursor>") and explain
re-running with last: 10, before: "<startCursor>" to confirm previous distinct
slice; reference the PaginationArgs behavior and include the pageInfo
startCursor/hasPreviousPage fields so users can obtain the cursor for backward
paging.
In `@specs/101-innovation-library-pagination/spec.md`:
- Around line 158-160: The spec's pagination edge-case is ambiguous about the
exact error for invalid/stale cursors; update the spec text around FR-011 and
the cursor helper description to explicitly state the error type and behavior
when a cursor references a deleted/non-existent item (e.g., throw or return an
EntityNotFoundException as named in data-model.md) and describe whether this
should be surfaced to clients or mapped to a standard pagination validation
error; reference FR-011 and the "cursor helper" behavior in the same paragraph
so implementers and tests know to expect EntityNotFoundException for
stale/invalid cursor resolution.
In `@src/library/library/dto/library.dto.templates.paginated.ts`:
- Line 4: Replace the local relative DTO import with the repo alias: change the
import of ITemplateResult in library.dto.templates.paginated.ts from
"./library.dto.template.result" to use the configured `@library` path (e.g. import
{ ITemplateResult } from '`@library/library/dto/library.dto.template.result`';) so
the DTO layer follows the tsconfig path alias convention and resolves
consistently.
In `@src/library/library/library.resolver.fields.ts`:
- Around line 12-15: The new DTO imports (PaginatedInnovationPacks,
ITemplateResult, LibraryTemplatesFilterInput, PaginatedLibraryTemplateResults)
currently use relative paths (./dto/...), so update them to use the project's
configured path alias for library DTOs (e.g. the `@library` or equivalent alias
configured in tsconfig) to match existing imports in this module; replace each
'./dto/...' import with the corresponding aliased module import for these four
symbols so the file consistently uses path aliases.
In `@src/migrations/1780407350716-AddRowIdToInnovationPackAndTemplate.ts`:
- Around line 6-18: The migration AddRowIdToInnovationPackAndTemplate implements
a working down() but lacks inline rollback notes; update the migration (methods
up and down) to add clear commented rollback notes above each down() step
describing why each constraint/column is dropped and any ordering or safety
considerations (e.g., drop UNIQUE constraints before DROP COLUMN for
"UQ_1fc637141a64418fb8220d46a84" on innovation_pack and
"UQ_d9a42feddd7089b159246887910" on template), and add a short header comment in
the class explaining the intent and idempotency expectations so incident
responders can understand reversal steps quickly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4ac5310-0b56-4919-82b3-8187f6014e37
📒 Files selected for processing (19)
schema.graphqlspecs/101-innovation-library-pagination/checklists/requirements.mdspecs/101-innovation-library-pagination/contracts/library-pagination.graphqlspecs/101-innovation-library-pagination/data-model.mdspecs/101-innovation-library-pagination/plan.mdspecs/101-innovation-library-pagination/quickstart.mdspecs/101-innovation-library-pagination/research.mdspecs/101-innovation-library-pagination/spec.mdspecs/101-innovation-library-pagination/tasks.mdsrc/domain/template/template/template.entity.tssrc/domain/template/template/template.interface.tssrc/library/innovation-pack/innovation.pack.entity.tssrc/library/innovation-pack/innovation.pack.interface.tssrc/library/library/dto/library.dto.innovationPacks.paginated.tssrc/library/library/dto/library.dto.templates.paginated.tssrc/library/library/library.resolver.fields.tssrc/library/library/library.service.spec.tssrc/library/library/library.service.tssrc/migrations/1780407350716-AddRowIdToInnovationPackAndTemplate.ts
Comment on lines
3126
to
+3140
| """The Innovation Packs in the platform Innovation Library.""" | ||
| innovationPacks(queryData: InnovationPacksInput): [InnovationPack!]! | ||
| """ | ||
| Paginated Innovation Packs in the platform Innovation Library (newest first). | ||
| """ | ||
| innovationPacksPaginated( | ||
| """A pivot cursor after which items are selected""" | ||
| after: UUID | ||
| """A pivot cursor before which items are selected""" | ||
| before: UUID | ||
| """Amount of items after the cursor""" | ||
| first: Int | ||
| """Amount of items before the cursor""" | ||
| last: Int | ||
| ): PaginatedInnovationPacks! |
Contributor
There was a problem hiding this comment.
Keep the paginated packs field semantically aligned with innovationPacks.
innovationPacks still exposes InnovationPacksInput.orderBy, with a documented default of number-of-templates descending, but innovationPacksPaginated hardcodes newest-first and drops that contract entirely. That makes the paginated field non-drop-in for existing library consumers and can reorder results as soon as clients migrate.
As per coding guidelines, schema files must be reviewed for backwards compatibility and alignment with contract tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@schema.graphql` around lines 3126 - 3140, The paginated field
innovationPacksPaginated currently hardcodes newest-first and ignores the
existing ordering contract exposed by innovationPacks
(InnovationPacksInput.orderBy), causing incompatible result ordering; update the
schema to accept an ordering argument (e.g., add an orderBy:
InnovationPacksOrderEnum or reuse InnovationPacksInput.orderBy) on
innovationPacksPaginated with the same default (number-of-templates desc) and
then update the resolver that resolves innovationPacksPaginated to honor that
orderBy when querying/sorting PaginatedInnovationPacks so its behavior is a
drop-in match for innovationPacks.
| - Interfaces `IInnovationPack` and `ITemplate` gain `rowId!: number;` so they | ||
| satisfy `Paginationable` for `getPaginationResults`. | ||
|
|
||
| Down-migration: drop the UNIQUE constraint and the column on both tables. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Specify down-migration operation order.
When dropping a column with constraints, the UNIQUE constraint must be dropped before the column. Update line 27 to specify: "Down-migration: drop the UNIQUE constraint first, then drop the column on both tables."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/101-innovation-library-pagination/data-model.md` at line 27, Update the
"Down-migration" sentence to state the operation order explicitly: first drop
the UNIQUE constraint, then drop the column on both tables; refer to the
existing "Down-migration" line and mention the UNIQUE constraint and the column
so the text becomes "Down-migration: drop the UNIQUE constraint first, then drop
the column on both tables."
Comment on lines
+83
to
+88
| 2. `getPaginationResults(qb, paginationArgs, 'DESC')` → `{ total, items: Template[], pageInfo }`. | ||
| 3. Load the packs for the returned templates (one query: packs whose | ||
| `templatesSetId IN (returned templates' templatesSetIds)`), map each | ||
| `Template` → `ITemplateResult { template, innovationPack }`. | ||
| 4. Return `{ total, templateResults, pageInfo }` (total + pageInfo straight from | ||
| step 2; cursors are template ids). |
Contributor
There was a problem hiding this comment.
Fix the cursor description in Decision 3.
Line 88 contradicts the rest of this document: these cursors come from the rowId keyset pagination helper, not from Template.id. Leaving this here will steer contract tests and resolver docs toward the wrong token format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/101-innovation-library-pagination/research.md` around lines 83 - 88,
The cursor description in Decision 3 is incorrect: update the text so it states
that cursors come from the keyset pagination helper's rowId field rather than
Template.id; specifically change the reference in the step that mentions
"cursors are template ids" to "cursors are rowId values produced by
getPaginationResults (keyset helper)", and ensure any examples or mentions of
Template.id are replaced with rowId to keep contract tests and resolver docs
aligned with getPaginationResults.
|
|
||
| - [x] T018 [P] Run `quickstart.md` validation (queries 1–7, incl. cursor/clamp/conflict/empty edge cases + migration up/down) against `pnpm start:dev` | ||
| - [x] T019 [P] Run `pnpm lint` (tsc --noEmit + Biome) and fix issues in new/edited files | ||
| - [ ] T020 Update the PR description: additive paginated fields + the `rowId` migration (idempotent, reversible); note paginated fields page newest-first with no field ordering |
Contributor
There was a problem hiding this comment.
Complete T020 before finalizing the PR.
T020 (updating PR description) is the only incomplete task. The PR description should document:
- Additive paginated fields (
templatesPaginated,innovationPacksPaginated) - The
rowIdmigration (idempotent, reversible) - Newest-first ordering with no field-based sorting on paginated fields
- Schema contract validation results from T017
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/101-innovation-library-pagination/tasks.md` at line 112, Update the PR
description to complete task T020: document the new additive paginated fields
templatesPaginated and innovationPacksPaginated, explain the rowId migration
including that it is idempotent and reversible, state that paginated fields are
newest-first with no field-based ordering, and include the Schema contract
validation results produced by T017; ensure these points are clearly labeled in
the PR description so reviewers can verify T020 is complete.
Comment on lines
+274
to
+276
| throw new RelationshipNotFoundException( | ||
| `Unable to find listed InnovationPack for Template ${template.id}`, | ||
| LogContext.LIBRARY |
Contributor
There was a problem hiding this comment.
Remove the template ID from the exception message.
Line 275 embeds template.id directly into a public-facing exception string. Keep the message static and pass the identifier via the exception details instead.
Suggested fix
if (!innovationPack) {
throw new RelationshipNotFoundException(
- `Unable to find listed InnovationPack for Template ${template.id}`,
- LogContext.LIBRARY
+ 'Unable to find listed InnovationPack for Template',
+ LogContext.LIBRARY,
+ { templateId: template.id }
);
}As per coding guidelines: "Never include dynamic data (IDs, emails) in exception messages; use structured details property as the third parameter". Based on learnings: "do not embed dynamic data (e.g., IDs, emails) in exception/error message strings. Instead, pass such data as structured properties within the details parameter".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| throw new RelationshipNotFoundException( | |
| `Unable to find listed InnovationPack for Template ${template.id}`, | |
| LogContext.LIBRARY | |
| if (!innovationPack) { | |
| throw new RelationshipNotFoundException( | |
| 'Unable to find listed InnovationPack for Template', | |
| LogContext.LIBRARY, | |
| { templateId: template.id } | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/library/library/library.service.ts` around lines 274 - 276, Replace the
dynamic template.id embedded in the thrown RelationshipNotFoundException in
library.service.ts with a static message string and pass the template identifier
via the exception's details parameter; specifically update the
RelationshipNotFoundException call (used in the method where the throw occurs)
to use a fixed message like "Unable to find listed InnovationPack for Template"
and supply { templateId: template.id } as the third argument while keeping
LogContext.LIBRARY as the context argument.
Comment on lines
+284
to
+295
| private clampPageSize(paginationArgs: PaginationArgs): PaginationArgs { | ||
| return { | ||
| ...paginationArgs, | ||
| first: | ||
| paginationArgs.first !== undefined | ||
| ? Math.min(paginationArgs.first, MAX_LIBRARY_PAGE_SIZE) | ||
| : paginationArgs.first, | ||
| last: | ||
| paginationArgs.last !== undefined | ||
| ? Math.min(paginationArgs.last, MAX_LIBRARY_PAGE_SIZE) | ||
| : paginationArgs.last, | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle omitted pagination args before reading first/last.
Both GraphQL callers pass pagination as nullable, so this helper is reachable with undefined. ...paginationArgs is harmless, but paginationArgs.first / paginationArgs.last will throw before either paginated query runs if the client omits the argument. Default to {} here or make the parameter optional and normalize it first.
Suggested fix
- private clampPageSize(paginationArgs: PaginationArgs): PaginationArgs {
+ private clampPageSize(paginationArgs?: PaginationArgs): PaginationArgs {
+ const args = paginationArgs ?? {};
return {
- ...paginationArgs,
+ ...args,
first:
- paginationArgs.first !== undefined
- ? Math.min(paginationArgs.first, MAX_LIBRARY_PAGE_SIZE)
- : paginationArgs.first,
+ args.first !== undefined
+ ? Math.min(args.first, MAX_LIBRARY_PAGE_SIZE)
+ : args.first,
last:
- paginationArgs.last !== undefined
- ? Math.min(paginationArgs.last, MAX_LIBRARY_PAGE_SIZE)
- : paginationArgs.last,
+ args.last !== undefined
+ ? Math.min(args.last, MAX_LIBRARY_PAGE_SIZE)
+ : args.last,
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/library/library/library.service.ts` around lines 284 - 295, clampPageSize
assumes paginationArgs is non-null and directly accesses
paginationArgs.first/last, which throws when callers pass null/undefined; make
the function robust by normalizing the input (e.g., default paginationArgs = {}
or handle undefined at the top), then apply the same clamping logic using the
normalized object and MAX_LIBRARY_PAGE_SIZE; update the signature of
clampPageSize (and any callers if you change requiredness) to accept
PaginationArgs | undefined or provide a default value so accessing .first/.last
is safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Schema Contract Checklist (Feature 002)
If this PR affects the GraphQL schema, complete ALL items below:
npm run schema:print(and optionallynpm run schema:sort) to regenerateschema.graphql.git show origin/develop:schema.graphql > tmp/prev.schema.graphql).npm run schema:diffto producechange-report.jsonanddeprecations.json.npm run schema:validateand artifacts passed validation.schema.graphql(not the JSON artifacts).Change Report Summary
Paste the key counts from
change-report.json(example format below) — remove any zero lines if desired:Deprecations Added / Updated
List new or updated deprecations with schedules (
REMOVE_AFTER=YYYY-MM-DD | reason). Indicate any in grace period.Breaking Changes (If Any)
If BREAKING changes are intentional:
BREAKING-APPROVEDrequested? (link)Other Notes
Reference docs:
specs/002-schema-contract-diffing/quickstart.mdfor full workflow and troubleshooting.Summary by CodeRabbit
Release Notes
New Features
Documentation