Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aims to make work item IDs incremental (per project) and improve project identifier validation and UI accessibility by introducing a reusable tooltip component and enforcing a 7-character project identifier limit across API + UI.
Changes:
- Backend: enforce a 7-character max project identifier and return a 400 with a specific error.
- Backend: create issues with a per-project incremental
sequence_idassigned inside a DB transaction (with advisory locking). - Frontend: add a
Tooltipcomponent and replace severaltitleattributes; enforce 7-character limit on the project identifier input.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/components/ui/index.ts | Exports the new Tooltip component from the UI barrel. |
| ui/src/components/ui/Tooltip.tsx | Introduces a portal-based tooltip component with hover/focus behavior. |
| ui/src/components/layout/PageHeader.tsx | Replaces title tooltips with Tooltip wrappers for various header controls. |
| ui/src/components/CreateProjectModal.tsx | Enforces 7-char project ID in the input and adds a tooltip hint button. |
| ui/package.json | Bumps UI package version. |
| ui/package-lock.json | Updates lockfile version metadata accordingly. |
| package.json | Marks repo private and changes test to run the validate pipeline. |
| api/internal/store/issue.go | Adds transactional helper + per-project sequence_id allocation using advisory locks. |
| api/internal/service/project.go | Adds max-length validation for project identifiers. |
| api/internal/service/issue.go | Uses transaction + NextSequenceID to assign SequenceID on create. |
| api/internal/handler/project.go | Maps identifier-too-long to a 400 Bad Request response. |
Files not reviewed (1)
- ui/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| var max int | ||
| err := tx.WithContext(ctx).Raw( | ||
| `SELECT COALESCE(MAX(sequence_id), 0) FROM issues WHERE project_id = ? AND deleted_at IS NULL`, |
There was a problem hiding this comment.
NextSequenceID filters on deleted_at IS NULL when computing MAX(sequence_id), which will reuse sequence IDs after a soft-deleted issue is removed (e.g., deleting the highest-numbered issue allows that number to be reassigned). For work-item IDs, sequence numbers generally must be monotonic and never reused; include soft-deleted rows in the max calculation (or use an unscoped query) so new issues always get a brand-new sequence ID.
| `SELECT COALESCE(MAX(sequence_id), 0) FROM issues WHERE project_id = ? AND deleted_at IS NULL`, | |
| `SELECT COALESCE(MAX(sequence_id), 0) FROM issues WHERE project_id = ?`, |
| if err := s.is.Transaction(ctx, func(tx *gorm.DB) error { | ||
| seq, err := s.is.NextSequenceID(ctx, tx, projectID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| issue.SequenceID = seq | ||
| return tx.WithContext(ctx).Create(issue).Error | ||
| }); err != nil { |
There was a problem hiding this comment.
The new transaction only covers sequence assignment + the issues insert; assignee/label mutations happen after the transaction and their errors are ignored, so a partially-created issue can be returned without its requested relationships and the sequence number can be consumed even though the overall create request effectively failed. To match the stated atomicity goal, perform relationship writes inside the same transaction and propagate any errors (returning a failure so the whole create rolls back).
| <span | ||
| ref={triggerRef} | ||
| className="inline-flex max-w-full" | ||
| onMouseEnter={show} | ||
| onMouseLeave={hide} | ||
| onFocusCapture={show} | ||
| onBlurCapture={hideIfLeavingTrigger} | ||
| aria-describedby={open ? id : undefined} | ||
| > | ||
| {children} | ||
| </span> |
There was a problem hiding this comment.
aria-describedby is applied to the wrapping <span>, but focus is typically on the child control (e.g., a <button>), so assistive tech won’t associate the tooltip content with the focused element. Consider constraining children to a single ReactElement and cloning it to inject aria-describedby (and the mouse/focus handlers) onto the actual trigger element (and forward the ref), so the tooltip is correctly announced on focus.
martian56
force-pushed
the
70-make-work-item-ids-increamental
branch
from
1fd7293 to
da43c9d
Compare
This pull request introduces several improvements across both the backend and frontend. The most significant changes are the enforcement of a 7-character maximum for project identifiers (with user feedback in both API and UI), enhanced transactional integrity for issue creation with per-project sequence numbers, and improved accessibility and usability of UI tooltips.
Backend improvements:
400 Bad Requestwith a descriptive error when the identifier is too long, instead of a generic error. [1] [2]Frontend improvements:
titleattributes with accessibleTooltipcomponents for various UI buttons and controls, improving usability and accessibility across the application. [1] [2] [3] [4] [5] [6] [7]Other changes:
package.jsonand version bumps for UI packages. [1] [2] [3]These changes collectively improve data integrity, user experience, and accessibility throughout the application.
Closes #70