Family-keyed atoms + Data.struct keys (DX feedback wanted)#32
Open
Atchferox wants to merge 3 commits into
Open
Family-keyed atoms + Data.struct keys (DX feedback wanted)#32Atchferox wants to merge 3 commits into
Atchferox wants to merge 3 commits into
Conversation
- Added new error messages for conflict, forbidden access, not found, rate limiting, unauthorized access, and validation errors in common.json. - Introduced a generic GitHub error message in git.json. - Created a new file for reactivity keys to manage state updates more effectively. - Refactored tag management in tags.ts to utilize AppApiClient for API interactions and improved optimistic updates for tag creation, renaming, and deletion. - Updated TagEditor component to incorporate reactivity keys for tag operations. - Enhanced error message handling in errorMessage.ts to include new error types.
- Add keys.ts with Data.struct-based keys for structural equality in Atom.family
- Replace string-split key pattern across all atom modules with typed key objects
- Update mutation callsites to pass structured { path, payload, reactivityKeys } args
- Wrap ticketAtom in optimistic layer; make createTicketAtom family-keyed
Contributor
📝 WalkthroughWalkthroughThis PR refactors frontend state management by introducing structured typed keys ( ChangesFrontend Atom Refactoring with Typed Keys and Reactivity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/frontend/src/atoms/tickets.ts (1)
32-52: 🏗️ Heavy liftBind mutation
pathto the family key.Line 32 already scopes the setter to a specific
TicketKey, but Lines 42, 48, and 52 still let callers pass an arbitrarypath. If those ever diverge, the optimistic reducer updates ticket A while the request mutates ticket B. Capturingkeyinside the family helper would remove that mismatch class and also trim the callshape.🤖 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 `@packages/frontend/src/atoms/tickets.ts` around lines 32 - 52, The family-created atoms updateTicketAtom, deleteTicketAtom, and createTicketAtom currently let callers supply an arbitrary mutation path, causing possible mismatches between the optimistic reducer (ticketAtom(key)) and the actual mutation target; fix by capturing the family key inside each Atom.family and binding it into the mutation call so the path is derived from that key (e.g., close over TicketKey and call AppApiClient.mutation(..., { path: key } or equivalent) instead of exposing a free path argument), and then adjust the callshape so callers no longer pass a path; update deleteTicketAtom and createTicketAtom to follow the same pattern used by updateTicketAtom (close over key) so the optimistic update and network mutation always reference the same ticket key.packages/frontend/src/atoms/github.ts (1)
77-98: 💤 Low valueHardcoded
baseBranch: "main"inattachBranchAtomreducer.Unlike
createBranchAtomwhich usesarg.payload.baseBranch ?? "main", this reducer hardcodes"main". If the attach payload can include abaseBranchfield, consider usingarg.payload.baseBranch ?? "main"for consistency.♻️ Suggested change for consistency
const optimistic: GitState = { tag: "branch_no_pr", name: arg.payload.name, - baseBranch: "main" + baseBranch: arg.payload.baseBranch ?? "main" }🤖 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 `@packages/frontend/src/atoms/github.ts` around lines 77 - 98, attachBranchAtom's optimistic reducer hardcodes baseBranch as "main"; change the reducer to derive baseBranch from the attach payload like createBranchAtom does. In the attachBranchAtom reducer (inside Atom.family -> projectGitStatesAtom(...) -> Atom.optimisticFn), replace the hardcoded baseBranch with arg.payload.baseBranch ?? "main" when building the optimistic GitState (the object using tag:"branch_no_pr", name: arg.payload.name) and ensure the result still updates states at [arg.path.id] and returns Result.success with { waiting: true } for the AppApiClient.mutation("tickets", "attachBranch") optimistic path.packages/frontend/src/components/GithubChip.tsx (1)
184-184: ⚡ Quick winMemoize the repos key to reduce per-render key allocations.
At Line 184,
reposKey(query)is rebuilt every render. Memoizing keeps key-object creation tied to query changes only.💡 Proposed refactor
function ConnectPanel({ orgSlug, slug }: { orgSlug: string; slug: string }) { const [query, setQuery] = useState("") - const repos = useAtomValue(githubReposAtom(reposKey(query))) + const repoLookupKey = useMemo(() => reposKey(query), [query]) + const repos = useAtomValue(githubReposAtom(repoLookupKey))🤖 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 `@packages/frontend/src/components/GithubChip.tsx` at line 184, The repos key passed into githubReposAtom is being re-created each render via reposKey(query); change the code in the GithubChip component to memoize that key so it only reallocates when query changes (e.g., compute const key = useMemo(() => reposKey(query), [query]) and then call useAtomValue(githubReposAtom(key))). This ensures the symbol reposKey and the atom githubReposAtom receive a stable key and prevents unnecessary per-render allocations.
🤖 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 `@packages/frontend/src/atoms/tags.ts`:
- Around line 66-74: The optimistic reducer in deleteTagAtom doesn't remove the
tag from the local value; update the reducer passed to Atom.optimisticFn in
deleteTagAtom (the reducer function that currently uses (current, _arg)) to,
when Result.isSuccess(current), return Result.success(filteredValue, { waiting:
true }) where filteredValue is current.value with the tag matching
_arg.path.name removed (i.e., filter out items whose name === _arg.path.name);
keep the same Result and waiting semantics and leave removeTag as the optimistic
fn.
In `@packages/frontend/src/components/TicketGit/CreateBranchFields.tsx`:
- Around line 92-93: The branch-create flow closes the form immediately after
dispatching updateTicket(ticketKey(...)) without waiting for the status-update
result; change the logic in CreateBranchFields so that after calling
updateTicket (from useAtomSet(updateTicketAtom(ticketKey(orgSlug, slug,
ticket.id)))), you await or observe the update result and only close the form on
success, and surface or handle errors (e.g., show a toast/error state) if the
update fails; apply the same fix to the other occurrence around lines 135-141 so
both success paths wait for and react to the status write outcome before
closing.
---
Nitpick comments:
In `@packages/frontend/src/atoms/github.ts`:
- Around line 77-98: attachBranchAtom's optimistic reducer hardcodes baseBranch
as "main"; change the reducer to derive baseBranch from the attach payload like
createBranchAtom does. In the attachBranchAtom reducer (inside Atom.family ->
projectGitStatesAtom(...) -> Atom.optimisticFn), replace the hardcoded
baseBranch with arg.payload.baseBranch ?? "main" when building the optimistic
GitState (the object using tag:"branch_no_pr", name: arg.payload.name) and
ensure the result still updates states at [arg.path.id] and returns
Result.success with { waiting: true } for the AppApiClient.mutation("tickets",
"attachBranch") optimistic path.
In `@packages/frontend/src/atoms/tickets.ts`:
- Around line 32-52: The family-created atoms updateTicketAtom,
deleteTicketAtom, and createTicketAtom currently let callers supply an arbitrary
mutation path, causing possible mismatches between the optimistic reducer
(ticketAtom(key)) and the actual mutation target; fix by capturing the family
key inside each Atom.family and binding it into the mutation call so the path is
derived from that key (e.g., close over TicketKey and call
AppApiClient.mutation(..., { path: key } or equivalent) instead of exposing a
free path argument), and then adjust the callshape so callers no longer pass a
path; update deleteTicketAtom and createTicketAtom to follow the same pattern
used by updateTicketAtom (close over key) so the optimistic update and network
mutation always reference the same ticket key.
In `@packages/frontend/src/components/GithubChip.tsx`:
- Line 184: The repos key passed into githubReposAtom is being re-created each
render via reposKey(query); change the code in the GithubChip component to
memoize that key so it only reallocates when query changes (e.g., compute const
key = useMemo(() => reposKey(query), [query]) and then call
useAtomValue(githubReposAtom(key))). This ensures the symbol reposKey and the
atom githubReposAtom receive a stable key and prevents unnecessary per-render
allocations.
🪄 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: 19a9c5ab-73bd-448e-aa52-80331c4ad4e9
📒 Files selected for processing (31)
packages/frontend/messages/en/common.jsonpackages/frontend/messages/en/git.jsonpackages/frontend/src/atoms/auth.tspackages/frontend/src/atoms/comments.tspackages/frontend/src/atoms/github.tspackages/frontend/src/atoms/keys.tspackages/frontend/src/atoms/projects.tspackages/frontend/src/atoms/reactivity-keys.tspackages/frontend/src/atoms/tags.tspackages/frontend/src/atoms/ticketListUi.tspackages/frontend/src/atoms/tickets.tspackages/frontend/src/components/Comments/CommentComposer.tsxpackages/frontend/src/components/Comments/CommentRow.tsxpackages/frontend/src/components/CreateTicketRow.tsxpackages/frontend/src/components/GithubChip.tsxpackages/frontend/src/components/MembersSection.tsxpackages/frontend/src/components/TagEditor.tsxpackages/frontend/src/components/TicketGit/ClearBranchFields.tsxpackages/frontend/src/components/TicketGit/ConnectBranchFields.tsxpackages/frontend/src/components/TicketGit/CreateBranchFields.tsxpackages/frontend/src/components/TicketList/AssigneeField.tsxpackages/frontend/src/components/TicketList/Expanded.tsxpackages/frontend/src/components/TicketList/PriorityField.tsxpackages/frontend/src/components/TicketList/StatusField.tsxpackages/frontend/src/components/TicketList/TypeField.tsxpackages/frontend/src/lib/errorMessage.tspackages/frontend/src/routes/_authed/orgs/$orgSlug/index.tsxpackages/frontend/src/routes/_authed/orgs/$orgSlug/projects/$slug/about.tsxpackages/frontend/src/routes/_authed/orgs/$orgSlug/projects/$slug/route.tsxpackages/frontend/src/routes/_authed/orgs/$orgSlug/projects/index.tsxpackages/frontend/src/services/AppApiClient.ts
Comment on lines
+92
to
93
| const updateTicket = useAtomSet(updateTicketAtom(ticketKey(orgSlug, slug, ticket.id))) | ||
|
|
Contributor
There was a problem hiding this comment.
Handle status-update failure before closing the form.
The branch-create success path closes immediately after dispatching updateTicket, but the update result is not observed. If it fails, users see a silent partial success.
✅ Suggested fix (await status write outcome)
- const updateTicket = useAtomSet(updateTicketAtom(ticketKey(orgSlug, slug, ticket.id)))
+ const updateTicket = useAtomSet(
+ updateTicketAtom(ticketKey(orgSlug, slug, ticket.id)),
+ { mode: "promiseExit" }
+ )
if (Exit.isSuccess(exit)) {
- if (status !== ticket.status)
- updateTicket({
- path: { orgSlug, slug, id: ticket.id },
- payload: { status },
- reactivityKeys: ticketWriteKeys
- })
+ if (status !== ticket.status) {
+ const statusExit = await updateTicket({
+ path: { orgSlug, slug, id: ticket.id },
+ payload: { status },
+ reactivityKeys: ticketWriteKeys
+ })
+ if (!Exit.isSuccess(statusExit)) {
+ setBusy(false)
+ return
+ }
+ }
close()
} else {
setBusy(false)
}Also applies to: 135-141
🤖 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 `@packages/frontend/src/components/TicketGit/CreateBranchFields.tsx` around
lines 92 - 93, The branch-create flow closes the form immediately after
dispatching updateTicket(ticketKey(...)) without waiting for the status-update
result; change the logic in CreateBranchFields so that after calling
updateTicket (from useAtomSet(updateTicketAtom(ticketKey(orgSlug, slug,
ticket.id)))), you await or observe the update result and only close the form on
success, and surface or handle errors (e.g., show a toast/error state) if the
update fails; apply the same fix to the other occurrence around lines 135-141 so
both success paths wait for and react to the status write outcome before
closing.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/frontend/src/atoms/tags.ts`:
- Around line 80-81: There are stray closing parentheses after the deleteTagAtom
definition (the extra `)` tokens following the `deleteTagAtom` closure) causing
a parse error; remove the extraneous `)` characters so that `deleteTagAtom` (and
surrounding atom definitions) close only once and the file parses correctly.
🪄 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: 142b2d29-db1f-4a06-8e91-ebad2f6c6a1c
📒 Files selected for processing (1)
packages/frontend/src/atoms/tags.ts
Comment on lines
+80
to
+81
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
Remove stray closing parentheses — parse error.
Lines 80–81 contain extra ) tokens after deleteTagAtom is already properly closed at line 79. This causes a syntax error that will fail compilation.
🐛 Proposed fix
})
)
)
- )
-)📝 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.
Suggested change
| ) | |
| ) | |
| }) | |
| ) | |
| ) |
🧰 Tools
🪛 Biome (2.4.15)
[error] 80-81: Expected a statement but instead found ')
)'.
(parse)
🤖 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 `@packages/frontend/src/atoms/tags.ts` around lines 80 - 81, There are stray
closing parentheses after the deleteTagAtom definition (the extra `)` tokens
following the `deleteTagAtom` closure) causing a parse error; remove the
extraneous `)` characters so that `deleteTagAtom` (and surrounding atom
definitions) close only once and the file parses correctly.
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
1 participant
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
Two parallel refactors landed in one commit (
a83cf59):Family-key the ticket mutations. Fixes the bleed where editing one ticket's title flipped the
waitingstate on every other ticket that readuseAtomValue(updateTicketAtom).tickets.tsnow mirrors thetags.tsshape:updateTicketAtom = Atom.family((TicketKey) => ticketAtom(key).pipe(Atom.optimisticFn({ reducer: spread payload, fn: AppApiClient.mutation(...) }))).delete/createfamily-keyed plain mutations.Replace string-concat keys with
Data.structcompound keys. Verified thatAtom.familymemoizes via Effect'sMutableHashMap(Equal.equals-based), so structural keys hit the same atom on every call. Newpackages/frontend/src/atoms/keys.tsis the single source of truth:OrgKey,ProjectKey,TicketKey,CommentKey,MemberKey,BranchesKey,ReposKey. No moresplitProjectKey/splitTicketKey/ etc. — factories destructure directly. Also: typed-distinct (you can't pass aProjectKeywhere aTicketKeyis expected) and no implicit "slug must not contain/" constraint.While I was in there I also migrated the remaining atom modules (
projects.ts,comments.ts,github.ts) fromruntime.atom/runtime.fnover toAppApiClient.query/mutation, and switched the project/github query atoms from.pipe(Atom.setIdleTTL(...))to the inlinetimeToLiveoption (same primitive —AtomHttpApi.js:47literally callsAtom.setIdleTTLfor finite values).Why I'm not sure about the DX (@Chippr-Wouter — would love your read)
A few things land differently than the old pattern, and I'd like your gut reaction before this becomes the house style:
Mutation call shape got more verbose. Old:
await create({ id: ticket.id, name, baseBranch }). New:await create({ path: { orgSlug, slug, id: ticket.id }, payload: { name, baseBranch }, reactivityKeys: githubWriteKeys }). It's the API request shape verbatim, which is good for traceability but easy to forgetreactivityKeys(silent failure — dependent queries don't refresh). Worth a lint? A helper that bakes in the rightreactivityKeysper atom?Data.structkeys allocate a new object every call. Cheap, but stylistically a step away from the upstream examples (which all use string keys). You lose the ability to log/inspect a key as a single string. Up-side is the type distinctness — that part I'm happy with. Want to see this style across the codebase or roll back to branded strings?Family-keyed optimistic mutations have two readers (the wrapper for waiting state, the underlying mutation atom for raw status). The pattern works, but "which atom do I read for X" requires a doc to internalise. CLAUDE.md covers it, but I wonder if a thin
useMutation(family, key)helper would pay off — it'd collapseuseAtomSet + useAtomValue + .waiting + Result.isFailureinto one hook. I deliberately didn't add that yet; let me know if the boilerplate is bothering you when you read the diff.Project mutation paths now repeat
{ orgSlug, slug }at every call site because the family key drives memoization, not the input. This is the CLAUDE.md rule ("path fields come from the key, not the input"), but it does feel like double-typing. A helper that accepts the key + a payload-only arg might be ergonomic — though again I didn't want to invent abstractions before we feel the pain.The bleed fix from (1) is unambiguously the right call. (2)/(3) are the discussion-worthy bits.
Test plan
bun run typecheckclean (✅ verified locally)bun run devboots; no console errors at startup (✅ verified)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor