make handle selection on signup optional (issue #22) - #29
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds per-request / per-client / server-default handle-assignment modes (random | picker | picker-with-random), persists Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant AuthSvc as Auth Service
participant MetaSvc as Client Metadata<br/>(fetch)
participant DB as Database
participant PDS as PDS Core
Client->>AuthSvc: GET /oauth/authorize\n(client_id, optional epds_handle_mode)
activate AuthSvc
AuthSvc->>MetaSvc: safeResolveClientMetadata(client_id)
activate MetaSvc
MetaSvc-->>AuthSvc: ClientMetadata or {}
deactivate MetaSvc
AuthSvc->>AuthSvc: resolveHandleMode(query, clientMeta, env)
AuthSvc->>DB: createAuthFlow(..., handleMode)
activate DB
DB-->>AuthSvc: created auth_flow
deactivate DB
AuthSvc-->>Client: Redirect to /auth/login
deactivate AuthSvc
Client->>AuthSvc: Complete auth flow (OTP)
activate AuthSvc
AuthSvc->>DB: getAuthFlow(flowId) -> includes handleMode
activate DB
DB-->>AuthSvc: auth_flow (handleMode)
deactivate DB
alt handleMode == "random"
AuthSvc->>PDS: pingParRequest(flow.requestUri) (non-fatal)
AuthSvc->>PDS: Redirect epds-callback (no handle param)
Note over PDS: PDS assigns random handle
else handleMode == "picker" or "picker-with-random"
AuthSvc-->>Client: Redirect to /auth/choose-handle
Note over Client: UI shows random button only for picker-with-random
end
deactivate AuthSvc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Kzoeps
force-pushed
the
22-make-handle-selection-on-signup-optional
branch
from
March 17, 2026 09:06
a5bd43a to
dfcd63a
Compare
Pull Request Test Coverage Report for Build 23473716758Details
💛 - Coveralls |
Contributor
Author
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.env.example (1)
77-80: Consider reordering the newPDS_*keys to clear dotenv-linter warnings.The current order in this block still triggers
UnorderedKeywarnings; sorting these entries to the repository’s expected order will keep env lint output clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 77 - 80, Reorder the newly added environment keys so they follow the repository's expected alphabetical/order convention to avoid dotenv-linter UnorderedKey warnings: locate the PDS_* entries (PDS_DID_PLC_URL, PDS_BSKY_APP_VIEW_URL, PDS_BSKY_APP_VIEW_DID, PDS_CRAWLERS) in .env.example and sort them into the same order used elsewhere in the repo (e.g., alphabetically or matching adjacent blocks), then save the file so dotenv-linter no longer flags UnorderedKey for these PDS_* variables.packages/auth-service/src/routes/complete.ts (1)
106-140: Consider aligning auth_flow cleanup with the 5d (existing user, no consent) path for consistency.The random-mode path currently relies on database TTL to expire the auth_flow row (as noted in the code comment at line 144), whereas the 5d path explicitly deletes it at lines 175-176. Since pds-core has no error-handling path that redirects back to auth-service for random-mode failures (it retries internally and renders an error page if all attempts fail), there is no need to keep the auth_flow row alive after the redirect.
Explicit deletion would be more consistent with the 5d path and clearer in intent, even though TTL cleanup eventually handles expiry.
♻️ Suggested cleanup for random-mode path
const params = new URLSearchParams({ ...callbackParams, ts, sig }) logger.info( { email, flowId }, 'New user (random mode): skipping handle picker, redirecting to epds-callback', ) + // Cleanup: remove auth_flow row and cookie (no retry needed for random mode) + ctx.db.deleteAuthFlow(flowId) + res.clearCookie(AUTH_FLOW_COOKIE) res.redirect( 303, `${ctx.config.pdsPublicUrl}/oauth/epds-callback?${params.toString()}`, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/complete.ts` around lines 106 - 140, The random-mode branch (when flow.handleMode === 'random') currently redirects to epds-callback without removing the auth_flow row; add the same explicit deletion used in the 5d (existing user, no consent) path so the auth_flow record for flowId is removed before calling res.redirect. Locate the deletion logic used in the 5d path (the function or ORM call that deletes the auth_flow row), and invoke that same deletion (using flowId/flow) immediately before constructing the callback params and calling res.redirect so behavior is consistent with the other path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.env.example:
- Around line 77-80: Reorder the newly added environment keys so they follow the
repository's expected alphabetical/order convention to avoid dotenv-linter
UnorderedKey warnings: locate the PDS_* entries (PDS_DID_PLC_URL,
PDS_BSKY_APP_VIEW_URL, PDS_BSKY_APP_VIEW_DID, PDS_CRAWLERS) in .env.example and
sort them into the same order used elsewhere in the repo (e.g., alphabetically
or matching adjacent blocks), then save the file so dotenv-linter no longer
flags UnorderedKey for these PDS_* variables.
In `@packages/auth-service/src/routes/complete.ts`:
- Around line 106-140: The random-mode branch (when flow.handleMode ===
'random') currently redirects to epds-callback without removing the auth_flow
row; add the same explicit deletion used in the 5d (existing user, no consent)
path so the auth_flow record for flowId is removed before calling res.redirect.
Locate the deletion logic used in the 5d path (the function or ORM call that
deletes the auth_flow row), and invoke that same deletion (using flowId/flow)
immediately before constructing the callback params and calling res.redirect so
behavior is consistent with the other path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d747390-4291-41b1-a443-985174564186
📒 Files selected for processing (14)
.beads/issues.jsonl.env.examplepackages/auth-service/.env.examplepackages/auth-service/src/__tests__/login-page.test.tspackages/auth-service/src/lib/client-metadata.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/pds-core/src/index.tspackages/shared/src/__tests__/crypto.test.tspackages/shared/src/db.tspackages/shared/src/handle.tspackages/shared/src/index.ts
Kzoeps
force-pushed
the
22-make-handle-selection-on-signup-optional
branch
from
March 17, 2026 10:03
dfcd63a to
ba7109c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 592-626: tryRandomHandle currently applies availability results
regardless of whether the user has changed the input, so stale fetches can
overwrite newer state; fix by recording the random handle you requested (e.g.
set a module-level pendingRandomHandle = local or a per-call token immediately
after computing local in tryRandomHandle) and before changing isAvailable,
setStatus, or updating UI in the fetch.then/ catch handlers verify that
pendingRandomHandle still equals local (or the token matches); only apply
updates when it matches and clear pendingRandomHandle when done; this keeps
tryRandomHandle, randomLocalPart, input, isAvailable and updateSubmit logic
intact while preventing stale updates.
- Around line 176-186: The route currently only special-cases
'picker-with-random' and therefore allows users in a flow with
result.flow.handleMode === 'random' to still pick a custom handle; add an
explicit guard in the /auth/choose-handle handlers to reject flows where
flow.handleMode === 'random' (check result.flow.handleMode or
res.locals.flow.handleMode depending on where flow is read) and return an error
response (e.g., res.status(400).send or render an error page) instead of calling
renderChooseHandlePage or allowing signing; apply the same guard to the other
handler that renders/handles the choose-handle page (the second occurrence
around the other call site that also computes showRandomButton) so 'random'
flows cannot proceed.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 92-96: The handler currently calls resolveHandleMode only with
req.query.epds_handle_mode and clientMeta, so epds_handle_mode supplied via PAR
is ignored; update the call to prefer the epds_handle_mode from the pushed
authorization request when present (fall back to req.query and then to
clientMeta/env) before persisting to auth_flow. Specifically, when computing
handleMode (the resolveHandleMode call that uses req.query.epds_handle_mode and
clientMeta), also extract epds_handle_mode from the parsed PAR payload (the code
path that later reads login_hint from the pushed request) and pass that value
into resolveHandleMode so clients that provide epds_handle_mode only in PAR are
handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 427a1ce3-028f-4b12-a67a-286a4184e6fb
📒 Files selected for processing (7)
.beads/issues.jsonl.env.examplepackages/auth-service/src/__tests__/login-page.test.tspackages/auth-service/src/lib/client-metadata.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/pds-core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/auth-service/src/tests/login-page.test.ts
Kzoeps
force-pushed
the
22-make-handle-selection-on-signup-optional
branch
from
March 17, 2026 10:40
452cb3e to
5a121cd
Compare
Kzoeps
force-pushed
the
22-make-handle-selection-on-signup-optional
branch
from
March 17, 2026 10:50
5a121cd to
cb70d19
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pds-core/src/index.ts`:
- Around line 245-251: The JSDoc `@see` reference in the CONTRACT block is using
an incorrect relative path; update the first `@see` from
../../../auth-service/src/routes/complete.ts to
../../auth-service/src/routes/complete.ts so it correctly points from the
pds-core/src directory to packages/auth-service/src/routes/complete.ts — look
for the CONTRACT JSDoc block (the comment referencing absent `handle` param /
handleMode='random') and update the `@see` link accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59d3a2cd-c934-489e-a584-41fce630c00b
📒 Files selected for processing (14)
.beads/issues.jsonl.env.examplepackages/auth-service/.env.examplepackages/auth-service/src/__tests__/login-page.test.tspackages/auth-service/src/lib/client-metadata.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/pds-core/src/index.tspackages/shared/src/__tests__/crypto.test.tspackages/shared/src/db.tspackages/shared/src/handle.tspackages/shared/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/auth-service/src/routes/recovery.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/auth-service/src/tests/login-page.test.ts
- packages/shared/src/index.ts
- packages/auth-service/src/lib/client-metadata.ts
- packages/shared/src/handle.ts
- packages/shared/src/tests/crypto.test.ts
Kzoeps
force-pushed
the
22-make-handle-selection-on-signup-optional
branch
from
March 17, 2026 11:08
cb70d19 to
cc5655a
Compare
- Export resolveHandleMode and safeResolveClientMetadata for testability - Add 8 tests for resolveHandleMode covering all precedence levels - Add 4 tests for safeResolveClientMetadata covering error paths - All tests pass (298 total) Addresses Coveralls coverage reduction in login-page.ts
Kzoeps
force-pushed
the
22-make-handle-selection-on-signup-optional
branch
from
March 23, 2026 05:04
cc5655a to
7267ad0
Compare
aspiers
changed the title
- Extract makeDb/closeDb helpers to share DB setup/teardown between the two describe blocks that were copy-pasting beforeEach/afterEach - Collapse the four identical handle-mode storage tests into it.each - Extract withEnv() helper to eliminate repeated origEnv save/restore boilerplate across six resolveHandleMode tests
The function is inlined in a server-rendered template literal and has no access to the server-side module system, making the duplication unavoidable.
|
aspiers
approved these changes
Mar 24, 2026
This was referenced Mar 24, 2026
Contributor
|
@coderabbitai Update the summary to include how a trusted client app can change the handle selection mode. |
🧩 Analysis chain🏁 Script executed: Length of output: 25706
A client app sets
Valid values at every level are I'll now regenerate the PR summary with this included. Please include in the summary how a trusted client app can change the handle selection mode: by adding an 🧠 Learnings used✅ Actions performedSummary regeneration triggered. |
Contributor
|
@bitbeckers @pheuberger Please note the above (even though Coderabbit failed to update the PR description). |
This was referenced Apr 7, 2026
aspiers
added a commit
that referenced
this pull request
Apr 9, 2026
The retroactive changesets added in #58 were all committed in the same commit, so @changesets/changelog-github looked up that one commit and used its PR (#58) and SHA on every generated entry in the v0.2.0 release section. That's the generator behaving correctly for its model (one changeset = one originating commit = one originating PR) but it produces misleading "this change was shipped in #58" refs when in reality each described feature landed weeks earlier in a different PR. Replace each bogus prefix with links to the real introducing PR(s). Commit SHAs are dropped entirely because a feature that spans multiple commits can't be meaningfully linked to a single SHA, and the PR link already carries the useful context. Future (non-retroactive) changesets won't hit this — they'll be added in the same PR as the feature they describe, so the generator's single-commit lookup resolves to the right PR automatically. This fix is a one-off cleanup for the bootstrap release. PR → changeset mapping: #14 → Longer sign-in codes #13, #29, #33, #36 → Choose your own handle #3, #6 → Sign in faster from third-party apps #20, #23 → Fail-fast PDS_INTERNAL_URL validation #27 → Honour PORT env var
aspiers
added a commit
that referenced
this pull request
Apr 9, 2026
The retroactive changesets added in #58 were all committed in the same commit, so @changesets/changelog-github looked up that one commit and used its PR (#58) and SHA on every generated entry in the v0.2.0 release section. That's the generator behaving correctly for its model (one changeset = one originating commit = one originating PR) but it produces misleading "this change was shipped in #58" refs when in reality each described feature landed weeks earlier in a different PR. Replace each bogus prefix with links to the real introducing PR(s). Commit SHAs are dropped entirely because a feature that spans multiple commits can't be meaningfully linked to a single SHA, and the PR link already carries the useful context. Future (non-retroactive) changesets won't hit this — they'll be added in the same PR as the feature they describe, so the generator's single-commit lookup resolves to the right PR automatically. This fix is a one-off cleanup for the bootstrap release. PR → changeset mapping: #14 → Longer sign-in codes #13, #29, #33, #36 → Choose your own handle #3, #6 → Sign in faster from third-party apps #20, #23 → Fail-fast PDS_INTERNAL_URL validation #27 → Honour PORT env var
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
…PER-301) These features have been released since v0.2.0 but were missing from docs/configuration.md. Auth Service variables: - OTP_LENGTH (4-12, default 8) and OTP_CHARSET (numeric/alphanumeric) added in v0.2.0 (#14). - EPDS_DEFAULT_HANDLE_MODE (picker/random/picker-with-random) added in v0.2.0 (#13/#29/#33/#36). Port rows now mention the v0.2.0 PORT fallback precedence (#27): - auth service: AUTH_PORT -> PORT -> 3001 - pds-core: PDS_PORT -> PORT -> 3000 So that operators migrating pre-0.2.0 Railway setups learn they can drop service-specific port overrides. Refs HYPER-295.
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
Add an "Optional: control the handle picker" subsection under "Register your app" covering: - accepted values (picker / random / picker-with-random) - the per-request precedence (query param -> client metadata -> env var -> built-in default) - example client metadata JSON snippet and ?epds_handle_mode= query param override This feature shipped in v0.2.0 (#13/#29/#33/#36) but was not visible to client app developers reading docs/tutorial.md. Refs HYPER-295.
3 tasks
This was referenced Apr 20, 2026
Merged
This was referenced May 5, 2026
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
2 participants
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
HANDLE_SELECTION_MODEenv var to control whether users can choose their handle during signupuser-choice,random, andnonemodes, with fallback precedence chainCloses #22
Also closes HYPER-192 on linear
Summary by CodeRabbit
New Features
Tests
Documentation