feat: shared ledger access for invited users #1467
Conversation
When an invited user calls ensureCloudToken, check if they have access to any ledger whose name starts with the appId before creating a new one. This allows invited users to access the owner's ledger instead of creating a duplicate. 1 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Not sure if this is the best way. The underlying issue is that a user responding to an invite is getting bound to a new ledger not the ledger from the source user. This fixes that -- not sure its the best way to fix it |
![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.
ℹ️ 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".
| .where( | ||
| and( | ||
| eq(sqlLedgerUsers.userId, req.auth.user.userId), | ||
| eq(sqlLedgerUsers.status, "active"), | ||
| like(sqlLedgers.name, `${req.appId}%`), |
There was a problem hiding this comment.
The new prefix lookup checks any active ledger the user can access whose name starts with appId, but it doesn’t filter by the requested env. If a user is invited to a prod ledger named after the app (e.g., app123-owner) and later calls ensureCloudToken with env: "dev", this code will still return the prod ledger/tenant instead of creating or binding a dev-specific ledger. Because AppIdBinding is keyed by (appId, env), this bypasses environment isolation and can leak prod data into dev flows. The lookup should honor req.env (and corresponding binding) before reusing a ledger.
Useful? React with 👍 / 👎.
|
Warning Rate limit exceeded@jchris has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughWhen ledgerId is absent, ensureCloudToken now prefers reusing an existing environment-scoped AppIdBinding (auto-redeeming pending invites and verifying LedgerUsers membership). If none found, it locates or creates a ledger (name = appId), enforces max bindings, inserts a new AppIdBinding, and returns ledgerId/tenantId. Other changes: SQL condition-building consolidation and addUserToLedger upsert fallback. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dashboard/backend/public/ensure-cloud-token.ts(2 hunks)dashboard/backend/tests/db-api.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T09:53:26.979Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:186-199
Timestamp: 2025-09-10T09:53:26.979Z
Learning: In use-fireproof/react/use-attach.ts, the current attach flow calls database.attach immediately via KeyedResolvOnce, which can run before a token is available and won't re-run after resetToken. The attach logic should be moved into the onTokenChange success path to ensure it only runs when a valid token exists and can re-attach after token reset.
Applied to files:
dashboard/backend/public/ensure-cloud-token.ts
📚 Learning: 2025-09-09T19:56:31.545Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-protocol.ts:47-60
Timestamp: 2025-09-09T19:56:31.545Z
Learning: In the Result type from adviser/cement, Result.Err can accept Result objects directly without needing to unwrap them with .Err(). The codebase consistently uses Result.Err(resultObject) pattern, as seen in core/keybag/key-bag.ts lines 94 and 102.
Applied to files:
dashboard/backend/public/ensure-cloud-token.ts
🧬 Code graph analysis (1)
dashboard/backend/public/ensure-cloud-token.ts (1)
dashboard/backend/sql/ledgers.ts (2)
sqlLedgerUsers(65-87)sqlLedgers(7-25)
| if (!ledgerId) { | ||
| // Check if user has access to a ledger with name starting with appId (for shared/invited users) | ||
| const sharedLedger = await ctx.db | ||
| .select() | ||
| .from(sqlLedgerUsers) | ||
| .innerJoin(sqlLedgers, eq(sqlLedgers.ledgerId, sqlLedgerUsers.ledgerId)) | ||
| .where( | ||
| and( | ||
| eq(sqlLedgerUsers.userId, req.auth.user.userId), | ||
| eq(sqlLedgerUsers.status, "active"), | ||
| like(sqlLedgers.name, `${req.appId}%`), | ||
| ), | ||
| ) | ||
| .get(); | ||
| if (sharedLedger) { | ||
| ledgerId = sharedLedger.Ledgers.ledgerId; | ||
| tenantId = sharedLedger.Ledgers.tenantId; | ||
| } | ||
| } |
There was a problem hiding this comment.
Prefix‑match fallback is reasonable but could be tightened (multi‑match, status, LIKE semantics, and design tradeoffs)
The new fallback block does the right thing conceptually (reusing an existing shared ledger before creating a new one), but there are a few edge cases worth addressing:
- If a user is a member of multiple ledgers whose names share the same
appIdprefix (e.g. multiple installs or clones),.get()with noorderBywill return an arbitrary match. That makes behavior non‑deterministic over time. Consider adding a deterministic ordering (e.g.orderBy(sqlLedgers.createdAt)and a clear policy like “oldest” or “newest” match), or otherwise constraining which ledger is eligible. - You filter
LedgerUsers.status === "active"but don’t constrainLedgers.status. If you ever soft‑delete or disable ledgers via their status, this path could resurrect a non‑active ledger. Mirroring thestatus === "active"check onsqlLedgerswould avoid that. like(sqlLedgers.name, \${req.appId}%`)will treat%/_inappIdas wildcards. IfappIdis ever allowed to contain those characters, you may unexpectedly match unrelated ledgers. If that’s possible, either escape those characters for the LIKE pattern, or use a different approach (e.g. an explicitappId` column or equality on a computed prefix).
On the broader design question: given the current schema and existing data, this Option A “prefix‑match fallback” is a pragmatic, low‑risk way to avoid creating duplicate ledgers for invited users. Removing the user‑id suffix from ledger names (Option B) would simplify lookup but materially changes semantics (1 ledger per app instead of per‑user), requires a migration, and still doesn’t remove the need for an explicit appId mapping to avoid name collisions. I’d keep Option A for now, but long‑term consider making appId a first‑class field or tightening sqlAppIdBinding usage rather than relying on naming conventions.
🤖 Prompt for AI Agents
In dashboard/backend/public/ensure-cloud-token.ts around lines 90 to 108, the
prefix‑match fallback can return non‑deterministic or inappropriate ledgers: add
a deterministic ordering (e.g. orderBy(sqlLedgers.createdAt, "desc" or "asc")
and pick a clear policy such as newest or oldest), include a mirror check for
sqlLedgers.status === "active" so disabled/soft‑deleted ledgers are excluded,
and avoid unescaped LIKE semantics by escaping `%`/`_` in req.appId before
building the pattern (or better yet use an explicit appId column or a computed
prefix column/equality check if available). Ensure the query still returns a
single, well‑defined ledger after these constraints.
| /** | ||
| * Policy: Shared Ledger Access via Prefix Matching | ||
| * | ||
| * When a user is invited to a ledger and later calls ensureCloudToken with the same appId, | ||
| * they should be granted access to the existing shared ledger rather than creating a new one. | ||
| * | ||
| * The ledger name format is: `{appId}-{ownerUserId}` | ||
| * When an invited user calls ensureCloudToken with appId, the system should: | ||
| * 1. First check for an exact appId binding (existing behavior) | ||
| * 2. If not found, check if user has access to any ledger whose name starts with appId (prefix match) | ||
| * 3. Only create a new ledger if no matching ledger is found | ||
| * | ||
| * This enables collaborative apps where multiple users share the same database: | ||
| * - User A creates app with appId "vf-my-app-install1-todos" | ||
| * - System creates ledger "vf-my-app-install1-todos-{userA_id}" | ||
| * - User A invites User B to the ledger | ||
| * - User B visits app with same appId → gets access to User A's ledger (not a new one) | ||
| */ | ||
| it("ensureCloudToken grants invited user access to shared ledger via prefix match", async () => { | ||
| const userA = datas[7]; | ||
| const userB = datas[8]; | ||
| const id = sthis.nextId().str; | ||
| const appId = `SHARED_LEDGER_TEST-${id}`; | ||
|
|
||
| // User A creates a ledger via ensureCloudToken | ||
| // This creates a ledger named "{appId}-{userA_id}" and binds it to appId | ||
| const userAToken = await fpApi.ensureCloudToken( | ||
| { | ||
| type: "reqEnsureCloudToken", | ||
| auth: userA.reqs.auth, | ||
| appId, | ||
| }, | ||
| jwkPack.opts, | ||
| ); | ||
| expect(userAToken.isOk()).toBeTruthy(); | ||
| const userALedgerId = userAToken.Ok().ledger; | ||
|
|
||
| // User A invites User B to the ledger (not just the tenant) | ||
| const invite = await fpApi.inviteUser({ | ||
| type: "reqInviteUser", | ||
| auth: userA.reqs.auth, | ||
| ticket: { | ||
| query: { | ||
| existingUserId: userB.ress.user.userId, | ||
| }, | ||
| invitedParams: { | ||
| ledger: { | ||
| id: userALedgerId, | ||
| role: "member", | ||
| right: "write", | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| expect(invite.isOk()).toBeTruthy(); | ||
|
|
||
| // User B redeems the invite (adds them to LedgerUsers) | ||
| const redeem = await fpApi.redeemInvite({ | ||
| type: "reqRedeemInvite", | ||
| auth: userB.reqs.auth, | ||
| }); | ||
| expect(redeem.isOk()).toBeTruthy(); | ||
|
|
||
| // User B calls ensureCloudToken with the same appId | ||
| // Without prefix matching, this would create a NEW ledger "{appId}-{userB_id}" | ||
| // With prefix matching, User B should get access to User A's existing ledger | ||
| const userBToken = await fpApi.ensureCloudToken( | ||
| { | ||
| type: "reqEnsureCloudToken", | ||
| auth: userB.reqs.auth, | ||
| appId, | ||
| }, | ||
| jwkPack.opts, | ||
| ); | ||
| expect(userBToken.isOk()).toBeTruthy(); | ||
|
|
||
| // Critical assertion: User B should get the SAME ledger as User A | ||
| expect(userBToken.Ok().ledger).toBe(userALedgerId); | ||
|
|
||
| // Verify they're using the same tenant too | ||
| expect(userBToken.Ok().tenant).toBe(userAToken.Ok().tenant); | ||
| }); |
There was a problem hiding this comment.
New shared‑ledger test doesn’t actually exercise the prefix‑match fallback path
The scenario and assertions here are good (invited user should get the same ledger/tenant as the owner), but with the current implementation this test does not hit the new prefix‑match block:
- When
userAfirst callsensureCloudToken, you create a ledger and ansqlAppIdBindingrow forappId. - After inviting
userBand redeeming,LedgerUsershas an active entry foruserBon that ledger. - When
userBcallsensureCloudToken,getAppIdBindingjoinsAppIdBinding → Ledgers → LedgerUsersand successfully finds that binding foruserB, so the function returns from the “binding found” branch without ever running the newlike(sqlLedgers.name, \${appId}%`)` query.
As a result, this test would still pass even if the prefix‑match code were removed, so it doesn’t protect the new behavior.
To actually cover the prefix‑match fallback, you could, for example:
- Create the owner’s ledger via
createLedger(with name\${appId}-${ownerId}`) instead ofensureCloudToken, invite + redeem for the invited user, and **never** create ansqlAppIdBindingfor thatappId`; or - After the initial owner
ensureCloudToken, directly delete thesqlAppIdBindingrow in the test setup, then have the invited user callensureCloudTokenand verify that the ledger is found via prefix matching.
Either approach would make the test actually depend on the new prefix‑match lookup instead of the existing binding logic. Also, ensure that whatever redeem path you rely on sets LedgerUsers.status to "active", since the prefix query filters on that status.
🤖 Prompt for AI Agents
dashboard/backend/tests/db-api.test.ts lines 1149-1230: The test never exercises
the prefix-match fallback because an AppIdBinding for appId is present when
userB calls ensureCloudToken; update the test so no AppIdBinding exists at that
moment (either create the owner ledger with createLedger using name
`${appId}-${ownerId}` instead of owner calling ensureCloudToken, or call
ensureCloudToken for userA then delete the corresponding sqlAppIdBinding row
before inviting/redeeming), then invite/redeem userB (ensure LedgerUsers.status
is "active") and call ensureCloudToken for userB to verify the ledger is found
by prefix matching.
Ledger names should just be the appId for shared access. The userId suffix was causing issues with invite-based sharing. Also removed the broken LIKE query that was failing on D1. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously when existingUserId was provided, it only searched by userId, ignoring email. This meant invites created by email weren't found when the invited user already existed. Now we OR together all conditions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dashboard/backend/sql/sql-helper.ts (1)
61-64: Consider a more explicit no-op condition.Using
Math.random() + ""as a no-match condition works but is unconventional. Consider using a more explicit marker that clearly signals intent to future maintainers, such as:- return eq(table.userId, Math.random() + ""); + return eq(table.userId, "__NO_MATCH__");Alternatively, if drizzle-orm supports a false literal (e.g.,
sql1 = 0``), that would be more idiomatic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dashboard/backend/sql/sql-helper.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 801
File: src/ledger.ts:108-110
Timestamp: 2025-05-13T13:45:44.432Z
Learning: The `refId()` method in Ledger should hash only the store URLs, not the ledger name. This allows different ledger instances with different names but the same underlying storage to have the same reference ID.
🔇 Additional comments (4)
dashboard/backend/sql/sql-helper.ts (4)
2-2: LGTM: Import reflects the new OR-based logic.The removal of
andfrom the imports is consistent with the refactoring to OR-based condition aggregation.
52-59: Consistent with OR-based refactoring.Email and nick conditions are now added to the OR array instead of being combined with AND logic. This is consistent with the broader refactoring to OR-based matching, which should be verified as noted above.
66-66: LGTM: OR combination completes the refactoring.The
or(...conditions)return statement correctly combines all accumulated conditions. Ensure the behavioral implications of this OR logic are verified as noted above.
31-37: The OR logic is intentional and explicitly documented in the code comments. Lines 31-32 state the purpose: "We OR together all possible matches so invites can be found by email even when userId is provided." This allows flexible user identification by any identifier (userId, email, or nick), which is the correct behavior. The OR conditions are also combined with AND filters at the caller level (e.g., ledgerId, inviteId in find-invite.ts), so the final query properly constrains results. No security vulnerability exists here—this is the designed matching behavior.Likely an incorrect or invalid review comment.
When a user is already in a ledger, the insert now returns the existing record instead of failing with a constraint violation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dashboard/backend/internal/add-user-to-ledger.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 801
File: src/ledger.ts:108-110
Timestamp: 2025-05-13T13:45:44.432Z
Learning: The `refId()` method in Ledger should hash only the store URLs, not the ledger name. This allows different ledger instances with different names but the same underlying storage to have the same reference ID.
🧬 Code graph analysis (1)
dashboard/backend/internal/add-user-to-ledger.ts (4)
dashboard/backend/sql/ledgers.ts (1)
sqlLedgerUsers(65-87)dashboard/backend/sql/sql-helper.ts (1)
toUndef(5-7)core/protocols/dashboard/msg-types.ts (1)
UserStatus(73-73)core/types/protocols/cloud/msg-types.ts (2)
toRole(28-40)toReadWrite(14-24)
🔇 Additional comments (2)
dashboard/backend/internal/add-user-to-ledger.ts (2)
66-79: LGTM: Idempotent insert with conflict handling.The use of
onConflictDoNothing()with.returning()correctly implements upsert semantics, allowing the function to gracefully handle cases where the user-ledger-role association already exists.
103-115: LGTM: Correctly maps inserted record fields.The success path correctly retrieves the ledger name from the joined
Ledgerstable and maps the inserted record's fields to the return type.
| if (inserted.length === 0) { | ||
| // User already exists in ledger, fetch existing record | ||
| const existing = await ctx.db | ||
| .select() | ||
| .from(sqlLedgerUsers) | ||
| .where(and(eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId), eq(sqlLedgerUsers.userId, req.userId))) | ||
| .get(); | ||
| if (!existing) { | ||
| return Result.Err("failed to insert or find ledger user"); | ||
| } | ||
| return Result.Ok({ | ||
| ledgerName: toUndef(ledger.Ledgers.name), | ||
| userName: toUndef(existing.name), | ||
| ledgerId: ledger.Ledgers.ledgerId, | ||
| tenantId: ledger.Ledgers.tenantId, | ||
| status: existing.status as UserStatus, | ||
| statusReason: existing.statusReason, | ||
| userId: req.userId, | ||
| default: !!existing.default, | ||
| role: toRole(existing.role), | ||
| right: toReadWrite(existing.right), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix: Query must filter by role and status to match primary key.
The conflict case query filters only by ledgerId and userId, but the primary key is (ledgerId, userId, role). This means a user can have multiple roles for the same ledger, and the query could return the wrong role record. Additionally, the early-return path (line 32) filters by status = "active", but this conflict path doesn't, creating an inconsistency.
Apply this diff to filter by the correct role and status:
const existing = await ctx.db
.select()
.from(sqlLedgerUsers)
- .where(and(eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId), eq(sqlLedgerUsers.userId, req.userId)))
+ .where(
+ and(
+ eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId),
+ eq(sqlLedgerUsers.userId, req.userId),
+ eq(sqlLedgerUsers.role, req.role),
+ eq(sqlLedgerUsers.status, "active")
+ )
+ )
.get();📝 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.
| if (inserted.length === 0) { | |
| // User already exists in ledger, fetch existing record | |
| const existing = await ctx.db | |
| .select() | |
| .from(sqlLedgerUsers) | |
| .where(and(eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId), eq(sqlLedgerUsers.userId, req.userId))) | |
| .get(); | |
| if (!existing) { | |
| return Result.Err("failed to insert or find ledger user"); | |
| } | |
| return Result.Ok({ | |
| ledgerName: toUndef(ledger.Ledgers.name), | |
| userName: toUndef(existing.name), | |
| ledgerId: ledger.Ledgers.ledgerId, | |
| tenantId: ledger.Ledgers.tenantId, | |
| status: existing.status as UserStatus, | |
| statusReason: existing.statusReason, | |
| userId: req.userId, | |
| default: !!existing.default, | |
| role: toRole(existing.role), | |
| right: toReadWrite(existing.right), | |
| }); | |
| } | |
| if (inserted.length === 0) { | |
| // User already exists in ledger, fetch existing record | |
| const existing = await ctx.db | |
| .select() | |
| .from(sqlLedgerUsers) | |
| .where( | |
| and( | |
| eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId), | |
| eq(sqlLedgerUsers.userId, req.userId), | |
| eq(sqlLedgerUsers.role, req.role), | |
| eq(sqlLedgerUsers.status, "active") | |
| ) | |
| ) | |
| .get(); | |
| if (!existing) { | |
| return Result.Err("failed to insert or find ledger user"); | |
| } | |
| return Result.Ok({ | |
| ledgerName: toUndef(ledger.Ledgers.name), | |
| userName: toUndef(existing.name), | |
| ledgerId: ledger.Ledgers.ledgerId, | |
| tenantId: ledger.Ledgers.tenantId, | |
| status: existing.status as UserStatus, | |
| statusReason: existing.statusReason, | |
| userId: req.userId, | |
| default: !!existing.default, | |
| role: toRole(existing.role), | |
| right: toReadWrite(existing.right), | |
| }); | |
| } |
🤖 Prompt for AI Agents
In dashboard/backend/internal/add-user-to-ledger.ts around lines 80 to 102, the
conflict-case SELECT only filters by ledgerId and userId but the table's primary
key includes role and the earlier success path required status = "active";
update the .where(...) clause to also filter by the role (use the incoming role
value used when inserting, e.g., req.role or the same variable used for the
insert) and by status = "active" so the query returns the exact role record and
matches the early-return behavior.
Fix: Respect req.default when returning existing record.
When req.default is true, lines 56-65 clear the default flag on all user's ledger associations (including the conflicting one). However, the conflict path then returns the existing record's default value as-is, which will be 0 after the update, even though the user intended to set this ledger as their default.
Consider updating the existing record to set the default flag when req.default is true:
if (!existing) {
return Result.Err("failed to insert or find ledger user");
}
+ // Update default flag if requested
+ if (req.default && !existing.default) {
+ await ctx.db
+ .update(sqlLedgerUsers)
+ .set({ default: 1, updatedAt: now })
+ .where(
+ and(
+ eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId),
+ eq(sqlLedgerUsers.userId, req.userId),
+ eq(sqlLedgerUsers.role, req.role)
+ )
+ )
+ .run();
+ }
return Result.Ok({
ledgerName: toUndef(ledger.Ledgers.name),
userName: toUndef(existing.name),
ledgerId: ledger.Ledgers.ledgerId,
tenantId: ledger.Ledgers.tenantId,
status: existing.status as UserStatus,
statusReason: existing.statusReason,
userId: req.userId,
- default: !!existing.default,
+ default: req.default ?? !!existing.default,
role: toRole(existing.role),
right: toReadWrite(existing.right),
});📝 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.
| if (inserted.length === 0) { | |
| // User already exists in ledger, fetch existing record | |
| const existing = await ctx.db | |
| .select() | |
| .from(sqlLedgerUsers) | |
| .where(and(eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId), eq(sqlLedgerUsers.userId, req.userId))) | |
| .get(); | |
| if (!existing) { | |
| return Result.Err("failed to insert or find ledger user"); | |
| } | |
| return Result.Ok({ | |
| ledgerName: toUndef(ledger.Ledgers.name), | |
| userName: toUndef(existing.name), | |
| ledgerId: ledger.Ledgers.ledgerId, | |
| tenantId: ledger.Ledgers.tenantId, | |
| status: existing.status as UserStatus, | |
| statusReason: existing.statusReason, | |
| userId: req.userId, | |
| default: !!existing.default, | |
| role: toRole(existing.role), | |
| right: toReadWrite(existing.right), | |
| }); | |
| } | |
| if (inserted.length === 0) { | |
| // User already exists in ledger, fetch existing record | |
| const existing = await ctx.db | |
| .select() | |
| .from(sqlLedgerUsers) | |
| .where(and(eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId), eq(sqlLedgerUsers.userId, req.userId))) | |
| .get(); | |
| if (!existing) { | |
| return Result.Err("failed to insert or find ledger user"); | |
| } | |
| // Update default flag if requested | |
| if (req.default && !existing.default) { | |
| await ctx.db | |
| .update(sqlLedgerUsers) | |
| .set({ default: 1, updatedAt: now }) | |
| .where( | |
| and( | |
| eq(sqlLedgerUsers.ledgerId, ledger.Ledgers.ledgerId), | |
| eq(sqlLedgerUsers.userId, req.userId), | |
| eq(sqlLedgerUsers.role, req.role) | |
| ) | |
| ) | |
| .run(); | |
| } | |
| return Result.Ok({ | |
| ledgerName: toUndef(ledger.Ledgers.name), | |
| userName: toUndef(existing.name), | |
| ledgerId: ledger.Ledgers.ledgerId, | |
| tenantId: ledger.Ledgers.tenantId, | |
| status: existing.status as UserStatus, | |
| statusReason: existing.statusReason, | |
| userId: req.userId, | |
| default: req.default ?? !!existing.default, | |
| role: toRole(existing.role), | |
| right: toReadWrite(existing.right), | |
| }); | |
| } |
🤖 Prompt for AI Agents
In dashboard/backend/internal/add-user-to-ledger.ts around lines 80 to 102, the
conflict path returns the existing ledger-user row without applying req.default,
so when req.default is true the prior clear-default update can leave this row as
non-default; modify the conflict branch to, when req.default is true, perform an
update on the existing record to set default = 1 (or true) and persist that
change before returning, ensuring you honor the same transaction/context and
re-read or use the updated values for the returned payload (update status fields
only if necessary, otherwise preserve existing values).
When an invited user visits a vibe, the AppIdBinding lookup with user join fails because they're not in LedgerUsers yet. Now we also check for existing bindings without the user join to use the existing ledger. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dashboard/backend/public/ensure-cloud-token.ts (1)
81-85: Consider removing the redundantgetAppIdBindingcall.The
getAppIdBindingcall at line 81 uses the samefiltersas the initial call at line 50. Because we only enter this block when that first call returnedundefined(line 53), and the interveningensureUsercall (lines 69-75) does not create bindings or add users to ledgers, this second lookup is unlikely to find anything the first call missed—unless you're defending against a race condition where another process creates the binding concurrently.If the second call is intentional (e.g., for concurrency safety), add a comment explaining why. Otherwise, remove it to clarify the logic.
const binding = await getAppIdBinding(ctx, req.auth, req, filters); - if (binding) { - ledgerId = binding.Ledgers.ledgerId; - tenantId = binding.Ledgers.tenantId; - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dashboard/backend/public/ensure-cloud-token.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T09:53:26.979Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:186-199
Timestamp: 2025-09-10T09:53:26.979Z
Learning: In use-fireproof/react/use-attach.ts, the current attach flow calls database.attach immediately via KeyedResolvOnce, which can run before a token is available and won't re-run after resetToken. The attach logic should be moved into the onTokenChange success path to ensure it only runs when a valid token exists and can re-attach after token reset.
Applied to files:
dashboard/backend/public/ensure-cloud-token.ts
📚 Learning: 2025-09-09T20:13:36.836Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/gateways/cloud/to-cloud.ts:155-160
Timestamp: 2025-09-09T20:13:36.836Z
Learning: In core/gateways/cloud/to-cloud.ts, the configHash method currently hashes this.opts directly, which includes volatile fields like strategy/token/events/context. The user (mabels) indicated this is part of a second iteration approach, suggesting they plan to address hash stability in a future iteration.
Applied to files:
dashboard/backend/public/ensure-cloud-token.ts
🧬 Code graph analysis (1)
dashboard/backend/public/ensure-cloud-token.ts (4)
dashboard/backend/sql/app-id-bind.ts (1)
sqlAppIdBinding(5-19)dashboard/backend/sql/ledgers.ts (1)
sqlLedgers(7-25)dashboard/backend/api.ts (1)
createLedger(136-138)dashboard/backend/public/create-ledger.ts (1)
createLedger(9-63)
| const maxBindings = await ctx.db | ||
| .select({ | ||
| total: count(sqlAppIdBinding.appId), | ||
| }) | ||
| .from(sqlAppIdBinding) | ||
| .where(eq(sqlAppIdBinding.appId, req.appId)) | ||
| .get(); | ||
| if (maxBindings && maxBindings.total >= ctx.params.maxAppIdBindings) { | ||
| return Result.Err(`max appId bindings reached for appId:${req.appId}`); | ||
| } |
There was a problem hiding this comment.
Check maxAppIdBindings before creating the ledger to avoid orphaned records.
The maxAppIdBindings validation occurs after createLedger has already inserted a new ledger (line 105). If the check at line 124 fails, the function returns an error but leaves the newly created ledger in the database without a corresponding AppIdBinding. This wastes the tenant's ledger quota and violates atomicity.
Move the max-bindings check before ledger creation:
} else {
+ // Validate binding quota before creating resources
+ const maxBindings = await ctx.db
+ .select({
+ total: count(sqlAppIdBinding.appId),
+ })
+ .from(sqlAppIdBinding)
+ .where(eq(sqlAppIdBinding.appId, req.appId))
+ .get();
+ if (maxBindings && maxBindings.total >= ctx.params.maxAppIdBindings) {
+ return Result.Err(`max appId bindings reached for appId:${req.appId}`);
+ }
+
// create ledger
const rCreateLedger = await createLedger(ctx, {
type: "reqCreateLedger",
auth: req.auth,
ledger: {
tenantId,
name: req.appId,
},
});
if (rCreateLedger.isErr()) {
return Result.Err(rCreateLedger.Err());
}
ledgerId = rCreateLedger.Ok().ledger.ledgerId;
- const maxBindings = await ctx.db
- .select({
- total: count(sqlAppIdBinding.appId),
- })
- .from(sqlAppIdBinding)
- .where(eq(sqlAppIdBinding.appId, req.appId))
- .get();
- if (maxBindings && maxBindings.total >= ctx.params.maxAppIdBindings) {
- return Result.Err(`max appId bindings reached for appId:${req.appId}`);
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dashboard/backend/public/ensure-cloud-token.ts around lines 105-126, the
maxAppIdBindings check currently runs after createLedger and can leave an
orphaned ledger; move the database count and the if (maxBindings.total >=
ctx.params.maxAppIdBindings) validation so it executes before calling
createLedger (i.e., perform the select/count and early return on exceeding
maxAppIdBindings immediately prior to ledger creation), or alternatively wrap
both the check, ledger creation and AppIdBinding insert in a single DB
transaction to ensure atomicity so no ledger is created when the binding limit
is exceeded.
…g token When an existing AppIdBinding is found, we must verify the user is actually in LedgerUsers with active status before granting access. Users must accept the invite first. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tests verify: 1. Uninvited users are rejected when accessing existing ledgers 2. Invited users can access ledgers after redemption 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
dashboard/backend/tests/db-api.test.ts (1)
1149-1230: Test doesn't exercise the new fallback path (confirmed duplicate issue)This test still suffers from the issue identified in the previous review: when
userBcallsensureCloudTokenafter redeeming the invite, the initialgetAppIdBindingcall (line 50 in ensure-cloud-token.ts) will succeed becauseuserBis now an active member of the ledger viaLedgerUsers. Therefore, the fallback code at lines 93-121 in ensure-cloud-token.ts never executes, and this test would pass even if that code were removed.To actually test the fallback path, you need to ensure that when userB calls
ensureCloudToken:
- An
AppIdBindingexists for the appId- But
userBis NOT yet inLedgerUsersfor that ledger (so the initial getAppIdBinding returns null)- Then the fallback code auto-redeems the invite and verifies membership
Consider restructuring the test to call
ensureCloudTokenfor userB before callingredeemInvite, since the new code is designed to auto-redeem.Based on past review comments.
dashboard/backend/public/ensure-cloud-token.ts (1)
123-156: Ledger created before maxAppIdBindings check (confirmed duplicate issue)The
maxAppIdBindingsvalidation (lines 136-145) occurs aftercreateLedgerhas already created and persisted a new ledger (line 124-135). If the binding limit has been reached, the function returns an error (line 144) but leaves the newly created ledger in the database without a correspondingAppIdBinding.This wastes the tenant's ledger quota and violates atomicity. The check should be moved before ledger creation:
// Check binding limit BEFORE creating ledger const maxBindings = await ctx.db .select({ total: count(sqlAppIdBinding.appId) }) .from(sqlAppIdBinding) .where(eq(sqlAppIdBinding.appId, req.appId)) .get(); if (maxBindings && maxBindings.total >= ctx.params.maxAppIdBindings) { return Result.Err(`max appId bindings reached for appId:${req.appId}`); } // Now safe to create ledger const rCreateLedger = await createLedger(ctx, { ... });Alternatively, wrap the check, ledger creation, and binding insertion in a database transaction to ensure atomicity.
Based on past review comments.
🧹 Nitpick comments (1)
dashboard/backend/tests/db-api.test.ts (1)
1378-1433: Duplicate test coverage with earlier auto-redeem testThis test appears to duplicate the scenario already covered by the test at lines 1232-1283 ("ensureCloudToken auto-redeems pending invite for ledger access"). Both tests validate that
ensureCloudTokenauto-redeems pending invites without an explicitredeemInvitecall.While having multiple similar tests can improve confidence, consider whether this adds meaningful coverage or if it could be consolidated or parameterized to reduce maintenance burden.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dashboard/backend/public/ensure-cloud-token.ts(1 hunks)dashboard/backend/tests/db-api.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-10T09:53:26.979Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:186-199
Timestamp: 2025-09-10T09:53:26.979Z
Learning: In use-fireproof/react/use-attach.ts, the current attach flow calls database.attach immediately via KeyedResolvOnce, which can run before a token is available and won't re-run after resetToken. The attach logic should be moved into the onTokenChange success path to ensure it only runs when a valid token exists and can re-attach after token reset.
Applied to files:
dashboard/backend/public/ensure-cloud-token.tsdashboard/backend/tests/db-api.test.ts
📚 Learning: 2025-09-09T20:13:36.836Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/gateways/cloud/to-cloud.ts:155-160
Timestamp: 2025-09-09T20:13:36.836Z
Learning: In core/gateways/cloud/to-cloud.ts, the configHash method currently hashes this.opts directly, which includes volatile fields like strategy/token/events/context. The user (mabels) indicated this is part of a second iteration approach, suggesting they plan to address hash stability in a future iteration.
Applied to files:
dashboard/backend/public/ensure-cloud-token.ts
📚 Learning: 2025-09-10T10:00:01.115Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:160-185
Timestamp: 2025-09-10T10:00:01.115Z
Learning: In use-fireproof/react/use-attach.ts, there's a potential memory leak where the onTokenChange callback can call setAttachState after component unmount. The fix requires adding cleanup to the useEffect with a disposed flag to guard state updates, and ideally refactoring WebToCloudCtx.onTokenChange to return an unsubscribe function.
Applied to files:
dashboard/backend/public/ensure-cloud-token.ts
🧬 Code graph analysis (2)
dashboard/backend/public/ensure-cloud-token.ts (4)
dashboard/backend/sql/app-id-bind.ts (1)
sqlAppIdBinding(5-19)dashboard/backend/sql/ledgers.ts (2)
sqlLedgers(7-25)sqlLedgerUsers(65-87)dashboard/backend/public/ensure-user.ts (1)
ensureUser(15-133)dashboard/backend/public/create-ledger.ts (1)
createLedger(9-63)
dashboard/backend/tests/db-api.test.ts (2)
core/blockstore/store.ts (1)
id(109-111)core/base/ledger.ts (1)
sthis(113-115)
🔇 Additional comments (4)
dashboard/backend/tests/db-api.test.ts (3)
1232-1283: Good test coverage for auto-redeem flowThis test correctly exercises the new fallback path where
ensureCloudTokenauto-redeems pending invites. The flow is:
- UserB has a pending invite but hasn't called
redeemInvite- UserB calls
ensureCloudToken- The existing binding is found (lines 93-98 in ensure-cloud-token.ts)
ensureUserauto-redeems the invite (lines 101-104)- User membership is verified (lines 106-119)
This validates the intended user experience where invited users can access shared ledgers without a separate redemption step.
1285-1317: Correct validation of authorization policyThis test properly validates the strict policy: users who haven't been invited to a ledger should not gain access even if an
AppIdBindingexists for that appId. The assertion at line 1316 confirms the error message includes "not authorized", matching the error returned at line 118 in ensure-cloud-token.ts.
1319-1376: Valid scenario but doesn't test the fallback pathThis test validates the standard flow where a user redeems an invite before calling
ensureCloudToken. Since UserD is already inLedgerUserswhenensureCloudTokenis called (line 1365), the initialgetAppIdBindingcheck (line 50 in ensure-cloud-token.ts) succeeds and the function returns early without exercising the fallback code at lines 93-121.While this is a valid and important scenario to test, it doesn't provide additional coverage of the new fallback logic beyond what existing tests already cover.
dashboard/backend/public/ensure-cloud-token.ts (1)
99-121: Properly addresses security concerns from previous reviewThis code segment correctly addresses the critical security issue flagged in past reviews. The implementation now:
- Auto-redeems invites: Calls
ensureUser(line 101-104) which internally callsredeemInvite, adding the user toLedgerUsersif they have a pending invite- Verifies membership: Explicitly checks that the user is in
LedgerUserswithstatus === "active"(lines 106-116) before granting access- Denies unauthorized access: Returns an error (line 118) if the user is not a member, preventing unauthorized access to existing ledgers
This ensures invited users can access shared ledgers while maintaining strict authorization boundaries.
| // Check if there's already an AppIdBinding for this appId (without user join) | ||
| // This handles the case where an invited user visits before being added to the ledger | ||
| const existingBinding = await ctx.db | ||
| .select() | ||
| .from(sqlAppIdBinding) | ||
| .where(eq(sqlAppIdBinding.appId, req.appId)) | ||
| .innerJoin(sqlLedgers, eq(sqlLedgers.ledgerId, sqlAppIdBinding.ledgerId)) | ||
| .where(and(eq(sqlAppIdBinding.appId, req.appId), eq(sqlAppIdBinding.env, req.env ?? "prod"))) | ||
| .get(); |
There was a problem hiding this comment.
Implementation uses exact matching, not prefix matching as described
The PR title and description emphasize "prefix matching" to find ledgers whose names start with the appId (e.g., matching vf-my-app-install1-todos-{userId}), but this implementation uses exact equality on AppIdBinding.appId:
.where(and(eq(sqlAppIdBinding.appId, req.appId), eq(sqlAppIdBinding.env, req.env ?? "prod")))There's no like() operator or prefix matching here. The query finds an AppIdBinding where appId exactly equals req.appId. This means:
- If this is correct: The PR description and test comments should be updated to describe "exact matching on appId" rather than "prefix matching on ledger names"
- If prefix matching was intended: The implementation needs to be changed to query
sqlLedgers.nameusing a LIKE pattern (e.g.,like(sqlLedgers.name, \${req.appId}%`)`) as described in the PR objectives
Please clarify which approach is correct. Note that exact matching on AppIdBinding.appId is simpler and avoids the edge cases mentioned in past reviews (multiple matches, LIKE escaping, etc.), but it requires the AppIdBinding table to be properly populated for invited users.
🤖 Prompt for AI Agents
In dashboard/backend/public/ensure-cloud-token.ts around lines 91 to 98, the
code currently uses exact equality on AppIdBinding.appId but the PR description
states prefix matching on ledger names; decide which behavior you want and
implement accordingly: if you want prefix matching, change the query to join
sqlLedgers and use a LIKE on sqlLedgers.name with a prefix pattern (e.g.,
`${req.appId}%`) and keep the env filter; if exact matching is intended, update
the PR title/description/tests to state exact appId matching and verify
AppIdBinding is populated for invited users. Ensure proper escaping for LIKE
patterns if using prefix matching and keep only the relevant filter (env +
prefix) to avoid incorrect exact-equality logic.
| auth: req.auth, | ||
| ledger: { | ||
| tenantId, | ||
| name: req.appId, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file context around line 129
cd dashboard/backend && cat -n public/ensure-cloud-token.ts | sed -n '115,140p'Repository: fireproof-storage/fireproof
Length of output: 1084
🏁 Script executed:
# Search for PR description or objectives in common locations
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.yml" -o -name "*.yaml" \) | head -20Repository: fireproof-storage/fireproof
Length of output: 661
🏁 Script executed:
# Search for other ledger creation patterns in the codebase
rg "name:\s*req\.appId|name:\s*\`.*appId.*userId" --type ts --type tsxRepository: fireproof-storage/fireproof
Length of output: 98
🏁 Script executed:
# Search for Ledgers table schema definition
rg "Ledgers.*table|CREATE.*TABLE.*Ledgers" --type ts --type tsx --type sql -A 10Repository: fireproof-storage/fireproof
Length of output: 98
🏁 Script executed:
# Search for how ledger naming is handled elsewhere in the codebase
rg "appId.*ownerUserId|ownerUserId.*appId" --type ts --type tsx -B 2 -A 2Repository: fireproof-storage/fireproof
Length of output: 98
🏁 Script executed:
# Let me adjust the search - remove tsx type and search more broadly
rg "name.*appId" --type ts -B 2 -A 2Repository: fireproof-storage/fireproof
Length of output: 1609
🏁 Script executed:
# Search for other createLedger calls to see naming patterns
rg "createLedger" --type ts -B 3 -A 3Repository: fireproof-storage/fireproof
Length of output: 22537
🏁 Script executed:
# Search for ownerUserId or userId in ledger context
rg "ownerUserId|userId.*ledger|ledger.*userId" --type ts -B 1 -A 1Repository: fireproof-storage/fireproof
Length of output: 9109
🏁 Script executed:
# Look for PR description in the repository (check git logs or PR-related files)
git log --oneline --all | head -20Repository: fireproof-storage/fireproof
Length of output: 228
🏁 Script executed:
# Check if there's a specific PR or branch with objectives
git branch -a | grep -E "801|enhance|ledger" || echo "No matching branches found"Repository: fireproof-storage/fireproof
Length of output: 97
🏁 Script executed:
# Look at the test section about ledger naming
sed -n '1,150p' dashboard/backend/tests/db-api.test.ts | grep -A 40 "appId.*ownerUserId"Repository: fireproof-storage/fireproof
Length of output: 53
🏁 Script executed:
# Look for the actual test implementation that uses appId-ownerUserId format
rg "appId.*userId|userId.*appId" dashboard/backend/tests/db-api.test.ts -B 5 -A 5Repository: fireproof-storage/fireproof
Length of output: 53
🏁 Script executed:
# Check if there are any TODOs or comments about ledger naming in ensure-cloud-token.ts
cat -n dashboard/backend/public/ensure-cloud-token.ts | sed -n '100,160p'Repository: fireproof-storage/fireproof
Length of output: 2579
🏁 Script executed:
# Look for all places where ledger names are created with appId
rg "name:\s*\`?\$?\{?req\.appId" dashboard/backend -B 3 -A 3Repository: fireproof-storage/fireproof
Length of output: 558
🏁 Script executed:
# Read the test section that mentions appId-ownerUserId more carefully
sed -n '1200,1350p' dashboard/backend/tests/db-api.test.tsRepository: fireproof-storage/fireproof
Length of output: 4960
🏁 Script executed:
# Check the git history of the ensure-cloud-token.ts file to see if there were recent changes
git log --oneline -20 -- dashboard/backend/public/ensure-cloud-token.tsRepository: fireproof-storage/fireproof
Length of output: 143
🏁 Script executed:
# Look for comments or TODOs about ledger naming in the codebase
rg "TODO|FIXME|XXX|HACK" dashboard/backend/public/ensure-cloud-token.ts dashboard/backend/tests/db-api.test.tsRepository: fireproof-storage/fireproof
Length of output: 53
🏁 Script executed:
# Search for any documentation on the intended ledger naming scheme
rg "ledger.*name|name.*format" --type md --type ts -B 2 -A 2Repository: fireproof-storage/fireproof
Length of output: 23497
Ledger naming doesn't match documented format
The test documentation describes ledger names as {appId}-{ownerUserId}, with prefix matching to allow invited users to access shared ledgers. However, the code at line 129 creates ledgers with only the appId:
name: req.appId,This works for the current binding model (one appId per ledger per tenant), but deviates from the documented design. Either the code should be updated to use name: ${req.appId}-${req.auth.user.userId}`` to match the specification and enable proper prefix-based ledger sharing, or the documentation should be updated to reflect that ledger names are now just appId.
🤖 Prompt for AI Agents
In dashboard/backend/public/ensure-cloud-token.ts around line 129, the ledger
name is being created as just the appId (name: req.appId) which diverges from
the documented `{appId}-{ownerUserId}` format that supports prefix-based
sharing; update the ledger name creation to concatenate the owner user id (e.g.,
use `${req.appId}-${req.auth.user.userId}`) so ledgers follow the documented
naming and allow invited users to access shared ledgers, or alternatively update
the docs if the intended design is to keep names as just appId—apply the code
change if you want to retain the documented sharing behavior.
ensureCloudToken now calls ensureUser before checking auth, allowing brand new users invited by email to access shared ledgers on first visit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
dashboard/backend/tests/db-api.test.ts (1)
1149-1230: This test doesn't exercise the prefix-match fallback (previously flagged).As noted in a previous review, this test doesn't actually hit the new prefix-match code path because User A's
ensureCloudTokencreates anAppIdBindingfor the appId, and that binding is still present when User B callsensureCloudToken. The test would pass even if the prefix-match logic were removed.To actually test the prefix-match fallback, you need to ensure no
AppIdBindingexists when the invited user callsensureCloudToken. See the detailed suggestion in the previous review comment.
🧹 Nitpick comments (1)
dashboard/backend/public/ensure-user.ts (1)
125-125: Consider using the capturedredeemResultor remove the variable.The
redeemResultvariable is assigned but never used. Either utilize it (e.g., for error handling or returning redeemed invites to the caller) or inline the call.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dashboard/backend/api.ts(1 hunks)dashboard/backend/public/ensure-user.ts(1 hunks)dashboard/backend/public/redeem-invite.ts(1 hunks)dashboard/backend/tests/db-api.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T09:53:26.979Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:186-199
Timestamp: 2025-09-10T09:53:26.979Z
Learning: In use-fireproof/react/use-attach.ts, the current attach flow calls database.attach immediately via KeyedResolvOnce, which can run before a token is available and won't re-run after resetToken. The attach logic should be moved into the onTokenChange success path to ensure it only runs when a valid token exists and can re-attach after token reset.
Applied to files:
dashboard/backend/api.tsdashboard/backend/tests/db-api.test.ts
🧬 Code graph analysis (3)
dashboard/backend/api.ts (4)
core/protocols/dashboard/msg-types.ts (2)
ReqEnsureCloudToken(523-533)ResEnsureCloudToken(535-544)dashboard/backend/types.ts (1)
FPTokenContext(85-92)dashboard/backend/public/ensure-user.ts (1)
ensureUser(15-135)core/protocols/dashboard/dashboard-api.ts (1)
ensureUser(134-136)
dashboard/backend/public/ensure-user.ts (2)
dashboard/backend/public/redeem-invite.ts (1)
redeemInvite(12-84)dashboard/backend/api.ts (1)
redeemInvite(118-120)
dashboard/backend/tests/db-api.test.ts (1)
core/protocols/dashboard/msg-types.ts (1)
DashAuthType(75-78)
🪛 GitHub Actions: @fireproof/dashboard
dashboard/backend/public/redeem-invite.ts
[warning] 1-1: Code style issues found in the file. Run Prettier with --write to fix.
🪛 GitHub Actions: @fireproof/dashboard-deploy
dashboard/backend/public/redeem-invite.ts
[error] 1-1: Prettier formatting check failed for this file. Run 'prettier --write' to fix code style issues. Command: 'pnpm run format --check' (underlying: prettier --config ../../.prettierrc . --check).
🔇 Additional comments (3)
dashboard/backend/public/redeem-invite.ts (1)
16-22: LGTM! Pre-fetching invites improves clarity.The refactor to pre-fetch invites before processing them is sound. This makes the code easier to debug (you can inspect
foundInvitesbefore the async map) without changing the functional behavior.Also applies to: 31-31
dashboard/backend/api.ts (1)
157-167: LGTM! Pre-step to ensureUser enables auto-redeem of invites.This is the key change that enables invited users to automatically redeem invites on their first call to
ensureCloudToken. The logic correctly:
- Calls
ensureUserfirst to create the user and redeem any pending invites- Handles errors appropriately
- Proceeds with the existing auth check and token creation
This aligns with the PR objective of allowing invited users to access shared ledgers without manual redemption.
dashboard/backend/tests/db-api.test.ts (1)
1232-1560: Excellent test coverage for auto-redeem and email invite flows.The new test cases thoroughly cover the critical user flows:
- Auto-redeem when
ensureCloudTokenis called (line 1232)- Strict policy rejecting uninvited users (line 1285)
- Post-redemption access (line 1319)
- Auto-redeem without explicit
redeemInvitecall (line 1378)- Email-based invites (line 1435)
- Brand new users with email invites (line 1493)
These tests validate the core PR objective: allowing invited users to access shared ledgers seamlessly through auto-redemption.
| async ensureCloudToken(req: ReqEnsureCloudToken, ictx: Partial<FPTokenContext> = {}): Promise<Result<ResEnsureCloudToken>> { | ||
| // p0.46 - For new users, ensureUser must be called first to create them | ||
| // This allows invited users to access shared ledgers on first visit | ||
| console.log("[ensureCloudToken p0.46] starting for appId:", req.appId); |
There was a problem hiding this comment.
Remove PII logging before production deployment.
User IDs are logged to console (lines 163, 166), which may violate privacy regulations depending on how user IDs are classified in your system. Consider removing or redacting this PII.
Apply this diff:
- console.log("[ensureCloudToken p0.46] starting for appId:", req.appId);
+ console.log("[ensureCloudToken p0.46] starting");
const rUser = await ensureUser(this.ctx, { type: "reqEnsureUser", auth: req.auth });
if (rUser.isErr()) {
console.log("[ensureCloudToken p0.46] ensureUser failed:", rUser.Err());
return Result.Err(rUser.Err());
}
- console.log("[ensureCloudToken p0.46] ensureUser succeeded, userId:", rUser.Ok().user.userId);
+ console.log("[ensureCloudToken p0.46] ensureUser succeeded");Also applies to: 163-163, 166-166
🤖 Prompt for AI Agents
In dashboard/backend/api.ts around lines 160, 163 and 166, the code currently
logs user/app identifiers to the console which may contain PII; remove or redact
those console.log calls and replace them with non-PII-safe logging.
Specifically, delete or comment out the console.log that prints req.appId and
any other user ID values, or replace them with a logger call that only emits
non-identifying context (e.g., operation names, truncated/redacted IDs like
first 4 chars + “…” or a hash), and ensure no raw user identifiers are written
to stdout in these lines before production deployment.
| } | ||
| // Auto-redeem any pending invites for this user | ||
| await redeemInvite(ctx, { | ||
| console.log("[ensureUser p0.47] calling redeemInvite for email:", auth.verifiedAuth.params.email); |
There was a problem hiding this comment.
Remove PII logging before production deployment.
Email addresses and potentially other user identifiers in the redeemInvite result are being logged to console. This violates privacy regulations (GDPR, CCPA) and best practices.
Remove or redact PII from these logs:
- console.log("[ensureUser p0.47] calling redeemInvite for email:", auth.verifiedAuth.params.email);
+ console.log("[ensureUser p0.47] calling redeemInvite for user");
const redeemResult = await redeemInvite(ctx, {
type: "reqRedeemInvite",
auth,
});
- console.log("[ensureUser p0.47] redeemInvite result:", JSON.stringify(redeemResult));
+ console.log("[ensureUser p0.47] redeemInvite completed, found invites:", redeemResult.isOk() ? redeemResult.Ok().invites.length : 0);Also applies to: 129-129
🤖 Prompt for AI Agents
In dashboard/backend/public/ensure-user.ts around lines 124 and 129, remove or
redact PII from the console logs that print auth.verifiedAuth.params.email;
replace the direct email output with a non-identifying token (e.g., masked email
like a****@domain or a hashed/trimmed identifier) or entirely remove the log,
and ensure any remaining logging is behind a debug/verbose flag or environment
check so production never emits user emails.
| byNick: req.auth.verifiedAuth.params.nick, | ||
| existingUserId: req.auth.user.userId, | ||
| }; | ||
| console.log("[redeemInvite p0.47] searching with query:", JSON.stringify(query)); |
There was a problem hiding this comment.
Remove PII logging and fix formatting issues.
Two issues to address:
- Privacy violation: Email addresses and user identifiers are logged to console (lines 21, 25), which violates GDPR/CCPA requirements.
- Pipeline failure: Prettier formatting check failed for this file.
Fix PII logging and run pnpm run format --write:
- console.log("[redeemInvite p0.47] searching with query:", JSON.stringify(query));
+ console.log("[redeemInvite p0.47] searching for invites");
const foundInvites = await findInvite(ctx, { query });
- console.log("[redeemInvite p0.47] found invites:", foundInvites.length, "pending:", foundInvites.filter((i) => i.status === "pending").length);
- if (foundInvites.length > 0) {
- console.log("[redeemInvite p0.47] invite details:", JSON.stringify(foundInvites.map((i) => ({ id: i.inviteId, status: i.status, queryEmail: i.query.byEmail, ledger: i.invitedParams.ledger?.id }))));
- }
+ console.log("[redeemInvite p0.47] found invites:", foundInvites.length, "pending:", foundInvites.filter((i) => i.status === "pending").length);Also applies to: 24-26
🤖 Prompt for AI Agents
In dashboard/backend/public/redeem-invite.ts around lines 21 and 24-26, the
console.log calls currently print user PII (email and identifiers) and the file
also fails Prettier formatting; remove or redact PII from logs (e.g., log only
non-identifying metadata like "query received" or a hashed/masked id) and
replace JSON.stringify(query) with a safe representation that omits or masks
email/IDs, then run the formatter with `pnpm run format --write` to fix
formatting issues.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I deployed this but I dont think its what we want |
Summary
When an invited user calls
ensureCloudTokenwith an appId, they currently get a new ledger created for them (with name{appId}-{userId}), causing a UNIQUE constraint violation if they're invited to the owner's ledger.This PR adds prefix matching: before creating a new ledger, check if the user has access to any ledger whose name starts with the appId. This allows invited users to access the owner's ledger instead of creating a duplicate.
Question for review
Should we use prefix matching, or should we remove the user-id suffix from ledger names entirely?
Option A: Prefix matching (this PR)
{appId}-{ownerUserId}ensureCloudToken, check for ledgers matchingLIKE '{appId}%'Option B: Remove user-id suffix
{appId}Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.