Complete the workspace split, publish design tokens, and add optional authorization and quota ports - #91
Conversation
…and chart primitives
Finish moving legacy src/client and src/server code into apps/dashboard, apps/worker, and packages/* (api, core, db, models, schema, ui), removing the now-empty legacy directories. Introduce an authorize middleware in packages/api along with dashboard-side useSession/useCan hooks, a routes module, and nav config to gate UI and API access by permission. Update CONTRIBUTING.md to describe the finished workspace layout, and adjust build/test config (eslint, tsconfig, vite/vitest, package.json) for the new structure.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Note
3 files could not be reviewed, so this pass is incomplete.
Reviewed commit: 08cd593bfe
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
If Codra has suggestions, it will comment; otherwise it will react with 👍.
| @@ -1,5 +1,5 @@ | |||
| import { Badge, LayerCard, SectionCard, Text } from '@codraoss/ui'; | |||
| import pkg from '../../../../../package.json'; | |||
| import pkg from '../../../../../../package.json'; | |||
There was a problem hiding this comment.
Incorrect relative path to package.json
The relative import uses seven levels of parent directory traversal ('../../../../../../'), which goes one level above the repository root and will cause a module resolution error at build time.
| import pkg from '../../../../../../package.json'; | |
| import pkg from '../../../../../package.json'; |
| { group: ['**/db/file-reviews-*', '@codraoss/db/file-reviews-*'], message: 'Import from @codraoss/db/file-reviews, not a sibling. (No spec mocks this one today; the rule keeps the barrel the single entry point.)' }, | ||
| { group: ['**/services/model-review-*', '**/services/model-rate-limits', '**/services/model-chain-runner', '**/services/model-support', '@codraoss/models-*'], message: 'Import from @codraoss/models, not a sibling. Four specs vi.mock that specifier.' }, | ||
| { group: ['**/provider-github/src/http', '**/provider-github/src/app-auth', '**/provider-github/src/types', '**/provider-github/src/diff-fetch', '**/provider-github/src/review-post', '**/provider-github/src/labels'], message: 'Import from @codraoss/provider-github, not a sibling module. One spec vi.mocks that specifier. (oauth is deliberately NOT listed: it is the dashboard OAuth flow, not part of the GitHubClient barrel.)' }, | ||
| { group: ['**/core/review/*', '@server/core/review/*', '@codraoss/core/review/*'], message: 'Import from @server/core/review, not a sibling. One spec vi.mocks that specifier and workflows/review.ts imports only runReviewJob from it.' }, |
There was a problem hiding this comment.
Outdated alias in
no-restricted-imports rule
The PR intent states that src/server/* was merged into apps/worker/src. This change means the @server alias, which likely pointed to src/server, is no longer valid or has been replaced. However, this no-restricted-imports rule still includes '@server/core/review/*' in its group. This could lead to a broken linting rule or a false sense of security, as imports using the new path for the worker app might not be caught, or the alias might resolve to nothing, causing linting issues. The message also still refers to @server/core/review instead of the updated package or path.
| { group: ['**/services/model-review-*', '**/services/model-rate-limits', '**/services/model-chain-runner', '**/services/model-support', '@codraoss/models-*'], message: 'Import from @codraoss/models, not a sibling. Four specs vi.mock that specifier.' }, | ||
| { group: ['**/provider-github/src/http', '**/provider-github/src/app-auth', '**/provider-github/src/types', '**/provider-github/src/diff-fetch', '**/provider-github/src/review-post', '**/provider-github/src/labels'], message: 'Import from @codraoss/provider-github, not a sibling module. One spec vi.mocks that specifier. (oauth is deliberately NOT listed: it is the dashboard OAuth flow, not part of the GitHubClient barrel.)' }, | ||
| { group: ['**/core/review/*', '@server/core/review/*', '@codraoss/core/review/*'], message: 'Import from @server/core/review, not a sibling. One spec vi.mocks that specifier and workflows/review.ts imports only runReviewJob from it.' }, | ||
| { group: ['**/core/model-output/*', '@server/core/model-output/*', '@codraoss/core/model-output/*'], message: 'Import from @codraoss/core/model-output, not a sibling. (The package exports map already refuses to resolve these; the lint rule gives the error at edit time.)' }, |
There was a problem hiding this comment.
Outdated alias in
no-restricted-imports rule
Consistent with the prior finding, the PR intent states that src/server/* was merged into apps/worker/src. This change means the @server alias, which likely pointed to src/server, is no longer valid or has been replaced. This no-restricted-imports rule still includes '@server/core/model-output/*' in its group. This could lead to a broken linting rule or a false sense of security, as imports using the new path for the worker app might not be caught, or the alias might resolve to nothing, causing linting issues. The message also still refers to @server/core/model-output.
| { group: ['**/provider-github/src/http', '**/provider-github/src/app-auth', '**/provider-github/src/types', '**/provider-github/src/diff-fetch', '**/provider-github/src/review-post', '**/provider-github/src/labels'], message: 'Import from @codraoss/provider-github, not a sibling module. One spec vi.mocks that specifier. (oauth is deliberately NOT listed: it is the dashboard OAuth flow, not part of the GitHubClient barrel.)' }, | ||
| { group: ['**/core/review/*', '@server/core/review/*', '@codraoss/core/review/*'], message: 'Import from @server/core/review, not a sibling. One spec vi.mocks that specifier and workflows/review.ts imports only runReviewJob from it.' }, | ||
| { group: ['**/core/model-output/*', '@server/core/model-output/*', '@codraoss/core/model-output/*'], message: 'Import from @codraoss/core/model-output, not a sibling. (The package exports map already refuses to resolve these; the lint rule gives the error at edit time.)' }, | ||
| { group: ['**/core/diff/position', '@server/core/diff/position', '@codraoss/core/diff/position'], message: 'Import from @codraoss/core/diff, not a sibling.' }, |
There was a problem hiding this comment.
Outdated alias in
no-restricted-imports rule
Consistent with the prior findings, the PR intent states that src/server/* was merged into apps/worker/src. This change means the @server alias, which likely pointed to src/server, is no longer valid or has been replaced. This no-restricted-imports rule still includes '@server/core/diff/position' in its group. This could lead to a broken linting rule or a false sense of security, as imports using the new path for the worker app might not be caught, or the alias might resolve to nothing, causing linting issues. The message also still refers to @server/core/diff.
| // Hits avatars.githubusercontent.com directly: the github.com/<login>.png redirect can fail. | ||
| // No loading="lazy": intersection detection is unreliable in this app's scroll containers. | ||
| export function AuthorAvatar({ login, size = 20 }: { login: string | null; size?: number }) { | ||
| const [failed, setFailed] = useState(false); |
There was a problem hiding this comment.
Stale
failed state in AuthorAvatar component
The AuthorAvatar component uses a failed state to track if an avatar image failed to load. However, this state is not reset when the login prop changes. If an avatar for one user fails to load, failed becomes true. If the component is then re-rendered with a different login for whom an avatar exists, it will still show the fallback because the failed state is stale. This can lead to incorrect UI, showing a fallback avatar instead of the correct one.
| const [failed, setFailed] = useState(false); | |
| import { useState, useEffect, type ReactNode } from 'react'; | |
| // inside AuthorAvatar component | |
| useEffect(() => { | |
| setFailed(false); | |
| }, [login]); |
Description
Review limits
MAX_TOTAL_DIFF_CHARSraised to 4,000,000 and moved into@codraoss/coreconstants instead ofbeing duplicated in the repo-config schema.
the job status, not just in the PR comment.
Workspace restructure
src/server/*merged intoapps/worker/src; 13 re-export shims and 2 dead modules deletedrather than moved. Unifying the two conflicting
AppBindingslet theenv as anybridge castsin
api-deps.tsgo.src/client/*moved toapps/dashboard, Vite rooted there withpublicDir/outDirabsolute sowrangler.jsonc's../../dist/clientis unchanged.src/is gone; root is nowprivate: true.Publishing fixes
@codraoss/dbnow shipsmigrations/andscripts/, which were excluded from the tarball, soan installed copy could not create its schema. The runner also takes
--extra-dir/CODRA_EXTRA_MIGRATIONS_DIR, applied after the core set in the same transaction and trackedunder an
extra:prefix so filenames cannot collide.@codraoss/uiwas unusable standalone, since every token and utility class its componentsreference lived in the app's stylesheet. These now ship as
@codraoss/ui/styles.@sourcedirectives forpackages/ui, without which the move would havesilently dropped its classes.
Extension points (all optional, defaulting to current behaviour; no-argument
createApiRouter()is unchanged and the existing API specs verify it)createApiRouter(options?)takesbeforeAuth,afterAuth,pages,publicPagesandroutes.routesruns last, so/api/*additions still inherit session and CSRF middleware and nothingcan shadow a built-in route.
ApiRouterDepsgains optionalauthzandcheckQuotaports; absent means allow-all and no-op.There was no authorization layer before, so any authenticated user could delete any job or
rewrite any repo config.
@codraoss/schemaexportsapiActionsandApiActionas an open union. The dashboard exportspublicRoutes,shellRoutes,navItems,buildRouter,useSessionanduseCan.deliveries.
Line endings
.gitattributessets* text=auto eol=lf. 94 files were committed with CRLF, 3 of them mixedwithin one file.
Closes #88
Type of change
How Has This Been Tested?
typecheck, lint, 445 tests across 62 files, all 14 package builds, publint on all 7 packages,
the exports gate,
vite build, and the Worker dry-run all pass.Emitted CSS was diffed against a pre-change baseline: identical class inventory and identical
--ui-*custom properties, so the token split is not a visual change.All 7 tarballs were installed into a scratch project and imported to confirm the published
surface resolves, including
@codraoss/ui/styles.Unit Tests
Integration Tests
Manual Dashboard Verification
Manual GitHub Webhook Verification
Checklist: