-
Notifications
You must be signed in to change notification settings - Fork 229
fix(NavigationNavbar): widen NavigationItem.title to React.ReactNode #1748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9553c91
24e3410
84263f2
7bbd797
b9161af
d852654
788c2b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,11 @@ address (sorry, no pseudonyms or anonymous contributions). An example of signing | |
| $ commit -s -m “my commit message w/signoff” | ||
| ``` | ||
|
|
||
| The trailer must match that commit's **own author identity**. `-s` copies your current | ||
| `user.name` / `user.email` into the trailer, so a commit authored under a different identity - a | ||
| repo-local override, a second machine, a co-author's commit you replayed - still fails the check | ||
| even though a `Signed-off-by:` line is present. | ||
|
|
||
| To ensure all your commits are signed, you may choose to add this alias to your global `.gitconfig`: | ||
|
|
||
| _~/.gitconfig_ | ||
|
|
@@ -55,6 +60,23 @@ Or you may configure your IDE, for example, Visual Studio Code to automatically | |
|
|
||
| <a href="https://user-images.githubusercontent.com/7570704/64490167-98906400-d25a-11e9-8b8a-5f465b854d49.png" ><img src="https://user-images.githubusercontent.com/7570704/64490167-98906400-d25a-11e9-8b8a-5f465b854d49.png" width="50%"><a> | ||
|
|
||
| ### Repairing a failed DCO check | ||
|
|
||
| DCO is a blocking required check, enforced by the [probot/dco](https://github.com/probot/dco) | ||
| GitHub App rather than by a workflow - there is no file for it under `.github/`, so it is invisible | ||
| from the repository tree until a pull request fails. `.husky/commit-msg` is deliberately empty | ||
| (commit `612608be`), so nothing catches a missing or mismatched sign-off locally either. | ||
|
|
||
| To repair, rebase only the commits you authored, then force-push: | ||
|
|
||
| ``` | ||
| git rebase --signoff <last-correctly-signed-sha> | ||
| git push --force-with-lease | ||
| ``` | ||
|
|
||
| Pick that starting point deliberately. `--signoff` adds your trailer to every commit it replays, so | ||
| rebasing the whole branch grafts your sign-off onto a co-author's commit as a second trailer. | ||
|
|
||
|
Comment on lines
+63
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Do not present Line 73 adds a sign-off to every commit replayed, using the current Git identity. That can add mismatched or duplicate trailers to co-authored or otherwise differently authored commits, contradicting the author-identity requirement in Lines 43-47. Restrict this workflow to branches where every replayed commit belongs to the current author, or document an amend-per-commit workflow for mixed-author histories. 🧰 Tools🪛 LanguageTool[uncategorized] ~66-~66: The official name of this software platform is spelled with a capital “H”. (GITHUB) 🪛 markdownlint-cli2 (0.23.0)[warning] 72-72: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🤖 Prompt for AI Agents |
||
| ## <a name="contributing-docs">Documentation Contribution Flow</a> | ||
|
|
||
| Please contribute! Layer5 documentation uses Jekyll and GitHub Pages to host docs sites. Learn more about [Layer5's documentation framework](https://docs.google.com/document/d/17guuaxb0xsfutBCzyj2CT6OZiFnMu9w4PzoILXhRXSo/edit?usp=sharing). The process of contributing follows this flow: | ||
|
|
@@ -155,10 +177,10 @@ make lint | |
|
|
||
| `src/custom/ResponsiveDataTable.tsx` wraps `@mui/x-data-grid` (via `@layer5/mui-datatables`) with responsive column visibility and a standardised row action menu. Key exports: | ||
|
|
||
| | Export | Description | | ||
| |---|---| | ||
| | `ResponsiveDataTable` | The main table component | | ||
| | `TableAction` | Type for items in the per-row action menu | | ||
| | Export | Description | | ||
| | ----------------------- | ---------------------------------------------- | | ||
| | `ResponsiveDataTable` | The main table component | | ||
| | `TableAction` | Type for items in the per-row action menu | | ||
| | `getCopyDeepLinkAction` | Helper that builds a "Copy link" `TableAction` | | ||
|
|
||
| **`getCopyDeepLinkAction`** | ||
|
|
@@ -171,7 +193,7 @@ import { getCopyDeepLinkAction, TableAction } from '@sistent/sistent'; | |
| const rowActions: TableAction[] = [ | ||
| getCopyDeepLinkAction(() => copyRowDeepLink(row.id)), | ||
| // or, with a custom title: | ||
| getCopyDeepLinkAction(() => copyRowDeepLink(row.id), t('Copy link')), | ||
| getCopyDeepLinkAction(() => copyRowDeepLink(row.id), t('Copy link')) | ||
| ]; | ||
| ``` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // Type-level fixture for `NavigationItem['title']`. It is compiled by | ||
| // `navigationItemTitleTypes.test.ts` via `tsc --noEmit`, never executed, and is | ||
| // deliberately outside jest's `*.test.*` glob so jest does not collect it. | ||
| // | ||
| // `NavigationNavbar` renders `title` into MUI's `ListItemText` `primary` slot, | ||
| // which is typed `React.ReactNode`. Declaring `title` as `string` therefore | ||
| // rejected composed labels that already rendered correctly - see | ||
| // https://github.com/layer5io/sistent/issues/1746. | ||
| import OpenInNewIcon from '@mui/icons-material/OpenInNew'; | ||
| import type { NavigationItem } from '../../custom/NavigationNavbar'; | ||
|
|
||
| // A plain string still compiles - the widening is source-compatible. | ||
| export const plainStringTitle: NavigationItem = { | ||
| id: 'settings', | ||
| title: 'Settings', | ||
| onClick: () => {} | ||
| }; | ||
|
|
||
| // A composed ReactNode title compiles: the exact shape that previously forced | ||
| // consumers into an `as unknown as string` assertion. | ||
| export const composedNodeTitle: NavigationItem = { | ||
| id: 'api-docs', | ||
| title: ( | ||
| <span> | ||
| API Docs | ||
| <OpenInNewIcon fontSize="small" /> | ||
| </span> | ||
| ), | ||
| onClick: () => {} | ||
| }; | ||
|
|
||
| // Sub-items share the type, so they gain the same latitude. | ||
| export const composedSubItemTitle: NavigationItem = { | ||
| id: 'docs', | ||
| title: 'Docs', | ||
| onClick: () => {}, | ||
| subItems: [ | ||
| { | ||
| id: 'docs-api', | ||
| title: <span>API</span>, | ||
| onClick: () => {} | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| // Self-check: if the compiler ever stops type-checking this fixture, the | ||
| // suppression below goes unused and tsc reports TS2578 here, failing the test | ||
| // rather than letting it pass vacuously. A plain object is not a `ReactNode`. | ||
| export const rejectsNonNode: NavigationItem = { | ||
| id: 'bad', | ||
| // @ts-expect-error - a plain object is not assignable to React.ReactNode | ||
| title: { label: 'not a node' }, | ||
| onClick: () => {} | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "extends": "../../../tsconfig.json", | ||
| "compilerOptions": { | ||
| "noEmit": true | ||
| }, | ||
| "include": ["./navigationItemTitle.tsx"] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| import { spawnSync } from 'child_process'; | ||
| import path from 'path'; | ||
|
|
||
| // Type-level regression guard for https://github.com/layer5io/sistent/issues/1746. | ||
| // | ||
| // `NavigationNavbar` renders `NavigationItem['title']` into MUI's `ListItemText` | ||
| // `primary` slot, which is typed `React.ReactNode`. While `title` was declared | ||
| // `string`, composed labels rendered correctly but did not type-check, and | ||
| // consumers reached for `as unknown as string`. | ||
| // | ||
| // jest transforms with @swc/jest, which strips types without checking them, so a | ||
| // plain type-only assertion file would pass no matter what `title` is declared | ||
| // as. This test therefore shells out to `tsc` over a fixture that exercises both | ||
| // a plain string and a composed `ReactNode` title. | ||
| // | ||
| // Diagnostics are filtered to the fixture itself: type-checking it pulls in the | ||
| // component's transitive dependencies, which carry pre-existing errors unrelated | ||
| // to this contract (see CLAUDE.md, "Repo state that looks broken but is | ||
| // pre-existing"). The fixture's own `@ts-expect-error` self-check fails loudly if | ||
| // the compiler ever stops checking it, so filtering cannot make this vacuous. | ||
| // | ||
| // Everything below the filter exists so that the guard fails closed. Filtering | ||
| // is the one place a compiler check can quietly turn into a no-op: every outcome | ||
| // that is not "tsc ran, compiled the fixture, and reported on it" has to be made | ||
| // to fail rather than collapse into an empty, green list of diagnostics. | ||
|
|
||
| const FIXTURE = 'src/__testing__/fixtures/navigationItemTitle.tsx'; | ||
| const PROJECT = 'src/__testing__/fixtures/tsconfig.navigationItemTitle.json'; | ||
| const repoRoot = path.resolve(__dirname, '..', '..'); | ||
|
|
||
| // `<file>(<line>,<col>): error TS….` The file part is what scopes a diagnostic, | ||
| // so it is parsed rather than prefix-matched: tsc prints paths relative to its | ||
| // cwd only while the fixture stays under it, and a bare `startsWith` would read | ||
| // any other emission as "the fixture is clean". | ||
| const FILE_SCOPED_DIAGNOSTIC = /^(.+?)\(\d+,\d+\): error TS\d+/; | ||
|
|
||
| // `error TS…` with no file part is a config-level failure - TS18003 "No inputs | ||
| // were found in config file", TS5083 "Cannot read file" - and means the fixture | ||
| // was never compiled at all. The fixture filter drops these on the floor, and | ||
| // the fixture's `@ts-expect-error` self-check cannot see them either, because | ||
| // neither fires unless the compiler had the fixture in its program. | ||
| const CONFIG_SCOPED_DIAGNOSTIC = /^error TS\d+/; | ||
|
|
||
|
Comment on lines
+35
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: sed -n '1,140p' src/__testing__/navigationItemTitleTypes.test.tsRepository: layer5io/sistent Length of output: 4718 🏁 Script executed: sed -n '1,140p' src/__testing__/navigationItemTitleTypes.test.ts && printf '\n---\n' && sed -n '1,220p' src/__testing__/packageJsonTestUtils.tsRepository: layer5io/sistent Length of output: 4806 🏁 Script executed: sed -n '1,220p' src/__testing__/packageJsonTestUtils.tsRepository: layer5io/sistent Length of output: 237 🏁 Script executed: printf '%s\n' '--- navigationItemTitleTypes.test.ts ---'
cat -n src/__testing__/navigationItemTitleTypes.test.ts
printf '\n%s\n' '--- matching fixture files ---'
fd -a 'navigationItemTitle*' src/__testing__
printf '\n%s\n' '--- fixture tsconfig ---'
cat -n src/__testing__/fixtures/tsconfig.navigationItemTitle.json
printf '\n%s\n' '--- fixture source ---'
cat -n src/__testing__/fixtures/navigationItemTitle.tsxRepository: layer5io/sistent Length of output: 8159 🏁 Script executed: printf '%s\n' '--- fixture tsconfig files ---'
fd -a 'tsconfig.navigationItemTitle.json' src/__testing__/fixtures
printf '\n%s\n' '--- fixture tsconfig ---'
cat -n src/__testing__/fixtures/tsconfig.navigationItemTitle.json
printf '\n%s\n' '--- fixture source ---'
cat -n src/__testing__/fixtures/navigationItemTitle.tsxRepository: layer5io/sistent Length of output: 2717 Make the compiler guard fail closed. 🧰 Tools🪛 ast-grep (0.44.1)[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| // Absence of diagnostics only means "nothing was wrong with what tsc compiled" - | ||
| // it says nothing about *what* tsc compiled. A project whose `include` resolves | ||
| // to some other existing file emits no diagnostic at all, so both lists above | ||
| // come back empty and the guard passes without ever having looked at the | ||
| // fixture. `--listFiles` closes that by making the program's contents an | ||
| // assertable fact instead of an assumption. | ||
| const asPosix = (value: string): string => value.replace(/\\/g, '/'); | ||
|
|
||
| type Diagnostics = { fixture: string[]; config: string[]; compiledFixture: boolean }; | ||
|
|
||
| // Below the 180s `beforeAll` budget. `spawnSync` blocks the jest worker outright, | ||
| // and jest's own timeout cannot preempt a synchronous child, so a wedged tsc | ||
| // would otherwise hold the worker until the whole run is killed. Node's timeout | ||
| // signals the child, which surfaces as the `status === null` throw below. | ||
| const TSC_TIMEOUT_MS = 150_000; | ||
|
|
||
| const typeCheckFixture = (): Diagnostics => { | ||
| // Spawned through `process.execPath` rather than `npx`: no shell, no PATH | ||
| // lookup, and no `npx` vs `npx.cmd` divergence on Windows, where an ENOENT | ||
| // would otherwise be caught and laundered into the diagnostic output. | ||
| const tsc = spawnSync( | ||
| process.execPath, | ||
| [require.resolve('typescript/bin/tsc'), '-p', PROJECT, '--pretty', 'false', '--listFiles'], | ||
| { | ||
| cwd: repoRoot, | ||
| encoding: 'utf8', | ||
| timeout: TSC_TIMEOUT_MS, | ||
| // tsc reports on every file in the program, and `--listFiles` prints each | ||
| // one, so this runs to several MB. The 1 MB default would kill the child | ||
| // mid-stream and truncate stdout - and the fixture is a root file, so its | ||
| // diagnostics are emitted last and lost first. | ||
| maxBuffer: 32 * 1024 * 1024 | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| ); | ||
|
|
||
| // A compiler that never ran is not a compiler that found nothing. | ||
| if (tsc.error) throw tsc.error; | ||
| if (tsc.status === null) { | ||
| throw new Error(`tsc was killed by ${tsc.signal ?? 'an unknown signal'} before reporting`); | ||
| } | ||
|
|
||
| // tsc exits non-zero whenever any file in the program has an error, including | ||
| // the pre-existing ones outside the fixture, so the exit code is not the | ||
| // verdict here - the diagnostics are. | ||
| const lines = `${tsc.stdout}\n${tsc.stderr}`.split('\n').map((line) => line.trim()); | ||
|
|
||
| const belongsToFixture = (line: string): boolean => { | ||
| const file = FILE_SCOPED_DIAGNOSTIC.exec(line)?.[1]; | ||
| return file !== undefined && asPosix(file).endsWith(FIXTURE); | ||
| }; | ||
|
|
||
| return { | ||
| fixture: lines.filter(belongsToFixture), | ||
| config: lines.filter((line) => CONFIG_SCOPED_DIAGNOSTIC.test(line)), | ||
| // `--listFiles` prints one absolute path per program file, diagnostics aside. | ||
| compiledFixture: lines.some( | ||
| (line) => !FILE_SCOPED_DIAGNOSTIC.test(line) && asPosix(line).endsWith(FIXTURE) | ||
| ) | ||
| }; | ||
| }; | ||
|
|
||
| describe('NavigationItem title type contract', () => { | ||
| let diagnostics: Diagnostics; | ||
|
|
||
| beforeAll(() => { | ||
| diagnostics = typeCheckFixture(); | ||
| }, 180_000); | ||
|
|
||
| // Ordered deliberately: the two assertions below are only meaningful once the | ||
| // fixture is known to have been compiled, so prove that first. | ||
| it('compiles the fixture', () => { | ||
| expect(diagnostics.compiledFixture).toBe(true); | ||
| }); | ||
|
|
||
| it('reports no config-level failure that would skip the fixture', () => { | ||
| expect(diagnostics.config).toEqual([]); | ||
| }); | ||
|
|
||
| it('accepts both a plain string and a composed ReactNode title', () => { | ||
| expect(diagnostics.fixture).toEqual([]); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced-block language.
Change the fence to
```shso Markdown linting can identify the example language.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Source: Linters/SAST tools