Batch related database reads and transaction writes - #1851
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request batches package, listing, attribute, and transactional database operations, updates Botpoison and cookie configuration access, and simplifies boot, user, and settings helpers with corresponding test updates. ChangesBatched data access
Transactional batch execution
Shared API and configuration cleanup
Mutation test control
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/shared/db/groups.ts`:
- Around line 253-254: Update the exported getActiveListingsByGroupId
declaration to explicitly use the public signature (groupId: number) =>
Promise<ListingWithCount[]>, while preserving its existing listingsInGroup(true)
implementation.
In `@test/integration/servicing/atomicity.test.ts`:
- Around line 35-38: Update the withAnswerSaveFailure matcher in the rollback
test to target the post-delete INSERT INTO attendee_answers write, or explicitly
exclude the initial DELETE, so withPoisonedTransactionWrite triggers only after
the existing answer has been removed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9996505-a768-4e0b-b66b-84b5151c7d55
📒 Files selected for processing (19)
src/features/api/payment-processing/package-pricing.tssrc/features/public/pages.tssrc/shared/boot-checks.tssrc/shared/contact-form.tssrc/shared/cookies.tssrc/shared/db/attributes.tssrc/shared/db/groups.tssrc/shared/db/link-table.tssrc/shared/db/questions/strings.tssrc/shared/db/users.tssrc/shared/package-privacy.tssrc/shared/settings-nags.tstest/integration/servicing/atomicity.test.tstest/lib/server-questions/listing-questions.test.tstest/shared/contact-form.test.tstest/shared/cookies.test.tstest/shared/db/groups.test.tstest/shared/db/questions/attendee-answers.test.tstest/test-utils/db-poison.ts
💤 Files with no reviewable changes (1)
- test/shared/contact-form.test.ts
stefan-burke
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/db/groups.ts (1)
494-495: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlias the
groupstable in this query.The shared DB guidelines require descriptive full singular table aliases using
AS. Use an alias such asgroups AS package_grouphere.Proposed fix
- SELECT ${GROUP_COLUMNS} FROM groups + SELECT ${GROUP_COLUMNS} FROM groups AS package_group🤖 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/shared/db/groups.ts` around lines 494 - 495, Update the groups query near GROUP_COLUMNS to alias the table as “groups AS package_group” using the required descriptive singular alias syntax, and qualify any referenced columns with package_group as needed while preserving the query behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/shared/db/groups.ts`:
- Around line 494-495: Update the groups query near GROUP_COLUMNS to alias the
table as “groups AS package_group” using the required descriptive singular alias
syntax, and qualify any referenced columns with package_group as needed while
preserving the query behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4a1966f-3f5f-477b-8ca1-3889d8bfda96
📒 Files selected for processing (1)
src/shared/db/groups.ts
|
Updated the PR title to name the database read/write batching directly. The description now includes the explicit loader contract, the 53-line |
|
Addressed the follow-up group-query alias finding in 377ec9f. Both full-row group queries now use the same descriptive |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 377ec9fd15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/db/groups.ts (1)
492-500: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winProject only the package-display columns.
This loader selects
GROUP_COLUMNSand parses each row as a fullGroup, but the result only usesid,hide_package_listings, andname. Use a narrow row projection and the minimal required conversion before constructing the map; otherwise this remains an unnecessary full-row read and violates the shared DB projection rule.🤖 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/shared/db/groups.ts` around lines 492 - 500, The package group loader around packageGroups currently selects and parses full Group rows unnecessarily. Change the rowsByIds query to project only id, hide_package_listings, and name, then use the minimal conversion needed by the packageGroups mapping before constructing the map; preserve the existing filtering and package-only conditions.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/shared/db/groups.ts`:
- Around line 492-500: The package group loader around packageGroups currently
selects and parses full Group rows unnecessarily. Change the rowsByIds query to
project only id, hide_package_listings, and name, then use the minimal
conversion needed by the packageGroups mapping before constructing the map;
preserve the existing filtering and package-only conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ab3bb3a-5d60-48ba-b415-ea14a78ad4d9
📒 Files selected for processing (2)
src/shared/db/groups.tstest/shared/db/groups.test.ts
|
Addressed the package-display projection finding in 61ef322. The loader now uses |
1 participant
What changed
Why
The same concepts had separate implementations for one item and many items, or extra names around an existing helper. Keeping one path makes their behavior consistent and removes 36 lines from
srcwhile doing the same work.Package display loading now takes one narrow database round trip for several package IDs instead of one round trip per ID. Standalone tickets take no package display round trip.
Tests
deno task precommitpasses.Summary by CodeRabbit