Re-authorize deep-linked review proposals during polling - #2357
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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
Bypass the settled-elsewhere hook for unavailable pins
When the detail read for the currently displayed deep link returns 403/404, this code sets unavailableProposalId and removes the proposal, but then invokes onQueueReplacedByPoll. In PaperReviewView, that hook arms the active-proposal watcher, which sets activeProposalSettledElsewhere; that state renders before the unavailable state, and its return action reloads while preserving the same hash. The reviewer therefore sees the misleading settled-elsewhere notice and can get stuck reloading it instead of reaching the unavailable state's return action.
ℹ️ 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".
| // This is part of the background poll, not an explicit navigation. | ||
| // Keep it in the same cancellation and fail-fast envelope as the | ||
| // list request so teardown cannot leave a retry chain behind. | ||
| { skipRetry: true, signal: controller.signal }, |
There was a problem hiding this comment.
For an omitted deep link that remains deleted or unauthorized, this detail request intentionally receives 404/403 every 15-second poll, but its config does not include those statuses in expectedStatuses. The shared response interceptor in api/http.ts logs every non-expected response before this composable handles it, so merely leaving such a deep link open produces four spurious API errors per minute indefinitely; extend the read options and mark the handled denial statuses as expected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Non-blocking follow-up under the repository review policy. The denial still fails closed and cannot change authority or state; the residual is repeated error-level logging. Tracked on #2214: #2214 (comment). This PR is at its bounded fix round.
| } | ||
| } catch (e: unknown) { | ||
| if (!isCurrentRead()) return | ||
| if (isForbiddenError(e) || isHttpNotFound(e)) { |
There was a problem hiding this comment.
When the decoded hash is syntactically valid URI text but is not a GUID, such as #proposal-not-a-guid, the typed Guid id API route deterministically returns 400. Every poll still obtains a valid list first, but because only 403/404 are classified as definitive pin failures here, the 400 takes the transient-error branch and returns without applying that list. The queue therefore remains frozen and repeats the same failing detail request on every tick until the user removes the malformed hash; validate the target before requesting it or treat this deterministic 400 as an unavailable pin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Non-blocking input-hardening follow-up under the repository review policy. A malformed hash cannot change authority or state and preserves the last known queue, but repeats the failed detail read until navigation changes. Tracked on #2214: #2214 (comment). This PR is at its bounded fix round.
|
Triage: confirmed merge blocker and fixed in A pin-level 403/404 now accepts the readable queue and sets Fix verification: red regression failed for both 403 and 404 before the change; focused API/composable specs pass 126/126; scoped ESLint, typecheck, and diff check pass. |
|
Final fresh-context review at Two non-blocking residuals are left on open tracker #2214 rather than fix-cycled: accepted pin-level 403/404 can still produce a failure-looking HTTP log because |
…p-link-reauthorization
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0965bcc778
ℹ️ 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".
| if (!isCurrentRead()) return | ||
| if (hashTargetId) { | ||
| if (pinUnavailable) { | ||
| unavailableProposalId.value = hashTargetId |
There was a problem hiding this comment.
When the Paper theme is disabled and an omitted hash-pinned proposal now returns 403 or 404, this assignment drops the proposal and records an unavailable state that only PaperReviewView consumes. LegacyReviewView retains the hash, filters out every surviving queue row, and renders the generic “No proposals need review yet” empty state instead of explaining that the requested proposal was deleted or permission-denied; wire unavailableProposalId into the Legacy state branches as well.
AGENTS.md reference: frontend/AGENTS.md:L5-L6
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Non-blocking P2 under the repository review policy. The Legacy-only presentation is misleading but cannot change authority or state. Tracked on #2214 at #2214 (comment) with an exact resume point; this PR remains at its bounded fix ceiling.
…p-link-reauthorization
|
CLAUDE_SYNC_PACKET
|
1 participant
Summary
This removes stale or revoked deep-link state without widening approval or apply authority.
Scope
Refs #2214 item 1.
Refs #2215 final deep-link residual.
Both parent issues retain other residuals and must remain open. PR #2295's explicit-load 403 defect is separate and untouched.
Verification
Red-first focused run: 13 expected failures, 111 passes.
Review regressions: 2 expected failures for unavailable-pin retry and hash-only list-403 authority.
npx vitest --run --maxWorkers=2 src/tests/api/automationApi.spec.ts src/tests/composables/useReviewProposals.spec.ts: 2 files, 126 tests passed.npx eslint src/api/automationApi.ts src/tests/api/automationApi.spec.ts src/composables/useReviewProposals.ts src/tests/composables/useReviewProposals.spec.ts --max-warnings=0: passed.npm run typecheck: passed.npm run build: passed, 805 modules transformed and PWA output generated.git diff --check origin/main..HEAD: passed.npx vitest --run --maxWorkers=2: 357 files, 354 passed and 3 failed; 5,314 tests, 5,309 passed and 5 failed. The five failures are unchanged midnight-sensitive due-date assertions inboardStore.filtering.spec.ts(2),savedViewStore.spec.ts(2), andboard/cardFilterStore.spec.ts(1). No Review or API test failed.ee96b3d1f: exact-head focused specs 126/126, scoped ESLint, typecheck, and diff check passed after the connector finding fix.0965bcc77: merged currentorigin/main(2e27a0f1e; canonical-docs-only), then exact-head focused specs 126/126, scoped ESLint, typecheck, and diff check passed.494df3c08: merged currentorigin/main(8295c9598; canonical-docs-only), then exact-head focused specs 126/126, scoped ESLint, typecheck, and diff check passed.Independent review found that previously unavailable pins were not retried. Commit
5964323fcfixes that defect, preserves queue-level 403 authority across hash-only navigation, and removes asynchronous mock leakage. The follow-up review found no remaining direct CRITICAL or HIGH defect.Residual risk
CLAUDE_SYNC_PACKET
494df3c0868d5e9de9c8fe6f9b59c052d9847154with four implementation commits plus two base-sync merges.