fix: restore Codex app-server compatibility and mobile containment - #33
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds mobile overflow checks and layout constraints for Moments and Settings, isolates E2E app-server state, patches dependency exports, expands Codex reasoning-effort values, adds migration 0022, and improves runtime and backup test synchronization. ChangesResponsive verification
Dependency compatibility
Codex persistence and model capabilities
Runtime and backup reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant E2EFixture
participant Browser
participant MomentsPage
participant SettingsPage
E2EFixture->>Browser: materialize long URL fixtures
Browser->>MomentsPage: load mobile /moments
MomentsPage->>Browser: render constrained Moment row
Browser->>SettingsPage: load mobile /settings
SettingsPage->>Browser: render constrained model selector
Browser->>Browser: measure horizontal overflow
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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 `@e2e/cross-device-responsive.spec.ts`:
- Around line 44-58: Update the test "keeps long Moment rows within the phone
viewport" to assert that the fixture’s long Moment content, specifically the
target READER_URL or corresponding Moment row, is visible after navigating to
/moments and before measuring document scrollWidth; retain the existing heading
and overflow assertions.
In `@e2e/settings.spec.ts`:
- Around line 12-50: Update the fixture in the settings viewport test around the
mocked codex model and selection to use a deliberately long, unbreakable model
identifier or displayName that exceeds the phone viewport’s native select width.
Ensure the model option and model.selectOption call both use this same long
value, while preserving the existing overflow assertion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a79e6869-bb72-438b-b47f-ab9baf9acbdb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
docs/quality/verification.mddocs/references/design-system/verification.mde2e/cross-device-responsive.spec.tse2e/reader.spec.tse2e/settings.spec.tspackage.jsonpatches/brace-expansion@5.0.8.patchplaywright.config.tssrc/components/moments/MomentBrowse.tsxsrc/components/settings/SettingsPage.tsxsrc/server/e2e/fixture-reader.tssrc/server/translation/types.tstests/server/runtime/process-lease-overlap.test.tstests/server/translation/codex-app-server.test.ts
![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: 65b4ef28fc
ℹ️ 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".
|
Review trace for the two top-level review bodies:
Validation: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/server/db/schema.ts (1)
329-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why these checks use unqualified
sql.rawcolumn names.
appSettings(andtranslationJobs) now emit unqualified"id"/"translation_target_language"while every other table still emits"table"."column"via${table.column}. This asymmetry looks arbitrary without context, and a future refactor "fixing" it back to${table.column}would produce table-rebuild migrations whose CHECK bodies reference the old table name from inside__new_*. A one-line comment would lock in the intent.♻️ Suggested comment
+ // Checks use unqualified column names via sql.raw so table-rebuild + // migrations (CREATE TABLE __new_app_settings ...) stay valid. () => [ check("app_settings_id_check", sql.raw(`"id" = 'default'`)),🤖 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/server/db/schema.ts` around lines 329 - 341, Add a concise comment immediately above the checks in the appSettings schema explaining that the unqualified sql.raw column names are intentional because qualified references can retain the old table name during table-rebuild migrations. Preserve the existing check expressions and apply the same documented intent where the corresponding translationJobs checks use unqualified columns.
🤖 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 `@drizzle/0022_calm_speed.sql`:
- Around line 17-45: Update drizzle/0022_calm_speed.sql lines 17-45 by removing
the early PRAGMA foreign_keys=ON and placing it after the final CREATE INDEX so
both table rebuilds run with enforcement disabled. In
tests/server/db/schema.test.ts lines 586-599, seed translation_chunks and
translation_projection_spans rows for the pre-migration translation_jobs record
before applyRuntimeMigrations, then assert both child rows remain in the
migrated output.
In `@tests/server/db/schema.test.ts`:
- Around line 586-599: Extend the migration test setup around the existing
translation job seed to insert at least one related row into translation_chunks
and one into translation_projection_spans before applyRuntimeMigrations. After
migration, query both child tables and assert their row counts remain unchanged,
alongside the existing foreign-key validation, so the 0022 translation_jobs
rebuild preserves cascaded child data.
In `@tests/server/runtime/runtime-lease-test-helpers.ts`:
- Around line 191-194: Update createJsonLineReader’s nextOutput implementation
so concurrent stderr.next() and stdout.next() reads are not discarded when
Promise.race resolves. Preserve each losing stream read’s result in a shared
merged-output queue, or initialize both stream readers once and enqueue every
produced event, ensuring later calls return all output without hangs.
---
Nitpick comments:
In `@src/server/db/schema.ts`:
- Around line 329-341: Add a concise comment immediately above the checks in the
appSettings schema explaining that the unqualified sql.raw column names are
intentional because qualified references can retain the old table name during
table-rebuild migrations. Preserve the existing check expressions and apply the
same documented intent where the corresponding translationJobs checks use
unqualified columns.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 211105ed-28a7-45cc-b128-e4d6d78a7a7a
📒 Files selected for processing (13)
drizzle/0022_calm_speed.sqldrizzle/meta/0022_snapshot.jsondrizzle/meta/_journal.jsone2e/cross-device-responsive.spec.tse2e/settings.spec.tsplaywright.config.tssrc/server/db/bundled-migrations.tssrc/server/db/schema.tssrc/server/runtime/runtime-lease-coordinator.tstests/server/db/schema.test.tstests/server/psychiatrist/api-routes.test.tstests/server/runtime/process-lease-overlap.test.tstests/server/runtime/runtime-lease-test-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- playwright.config.ts
- e2e/cross-device-responsive.spec.ts
- e2e/settings.spec.ts
|
Second EDA review round completed. Adjudication and fixes:
Verification on the pushed head:
Each inline finding has an evidence-backed reply and is resolved. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Verification
Summary by CodeRabbit