Skip to content

Commit d1dc1bd

Browse files
committed
merge main
2 parents 74d0c01 + b3411b9 commit d1dc1bd

951 files changed

Lines changed: 28544 additions & 10626 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.claude/skills/coding-standards/SKILL.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ Coding standards for the Expensify App. Each standard is a standalone file in `r
3636
- [PERF-15](rules/perf-15-cleanup-async-effects.md) — Clean up async Effects
3737
- [PERF-16](rules/perf-16-guard-double-init.md) — Guard double initialization
3838
- [PERF-17](rules/perf-17-pass-raw-index-on-demand.md) — Pass raw source, index on demand (no pre-built digest)
39+
- [PERF-18](rules/perf-18-use-pre-mount-destination.md) — Use usePreMountDestination for RHP pre-mounting
3940

4041
### Consistency
4142
- [CONSISTENCY-1](rules/consistency-1-no-platform-checks.md) — No platform-specific checks in components

.claude/skills/coding-standards/rules/perf-17-avoid-intermediate-lookup.md renamed to .claude/skills/coding-standards/rules/perf-17-pass-raw-index-on-demand.md

File renamed without changes.
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
---
2+
ruleId: PERF-18
3+
title: Use usePreMountDestination for RHP-to-fullscreen pre-mounting
4+
---
5+
6+
## [PERF-18] Use usePreMountDestination for RHP-to-fullscreen pre-mounting
7+
8+
### Reasoning
9+
10+
Modal-to-destination flows need the destination mounted before the RHP dismisses. Otherwise users see a gap on narrow layout or a flash of the previous page on wide layout.
11+
12+
`usePreMountDestination` centralizes this lifecycle:
13+
14+
- Idle-priority pre-insert on narrow layout, with a fallback timer so the work is not starved
15+
- Reveal-before-dismiss fallback on wide layout or if narrow pre-insert has not finished
16+
- Automatic cleanup for back-out and unmount paths
17+
18+
When reviewing these flows, focus on whether the navigation lifecycle is correct for the user path:
19+
20+
- The destination route is stable at mount time
21+
- `reveal()` is called only after validation, synchronous writes, and target-route selection are complete
22+
- The caller handles flow-specific no-op cases before calling `reveal()`
23+
- Cleanup or preservation is handled on back-out and unmount
24+
25+
### Incorrect
26+
27+
```tsx
28+
const destinationRoute = buildDestinationRoute(itemID);
29+
const {reveal} = usePreMountDestination(destinationRoute);
30+
31+
const handleSubmit = () => {
32+
reveal(() => {
33+
saveDataRequiredByDestination();
34+
});
35+
};
36+
```
37+
38+
### Correct
39+
40+
```tsx
41+
const destinationRoute = buildDestinationRoute(itemID);
42+
const {reveal} = usePreMountDestination(destinationRoute);
43+
44+
const handleSubmit = () => {
45+
saveDataRequiredByDestination();
46+
reveal();
47+
};
48+
```
49+
50+
---
51+
52+
### Hook API
53+
54+
**Reveal methods:**
55+
56+
- `reveal(afterTransition?)`: if the hook owns a pre-inserted narrow route, clears the pre-insert flag and dismisses the RHP over that route. Otherwise, inserts the destination under the RHP and then dismisses it.
57+
- `cleanupPreMount()`: removes the owned pre-inserted destination before a back-out path closes the RHP without revealing the destination.
58+
59+
**Caller responsibilities:**
60+
61+
- Keep flow-specific checks and synchronous work outside the hook.
62+
- Handle no-op cases before calling `reveal()`, such as when the destination route is already the active fullscreen route behind the modal.
63+
- Pass `reveal(afterTransition)` only for work that must run after the dismiss/reveal transition.
64+
- Call `cleanupPreMount()` on every back-out path that closes the RHP without calling `reveal()`.
65+
66+
**Scheduling:**
67+
68+
- Mount-time pre-insert always waits for the RHP open transition before scheduling idle pre-insert work.
69+
- If no upcoming transition starts within 500ms, the hook proceeds with idle pre-insert scheduling. This only prevents missing the RHP open transition; broader transition timing changes should be discussed separately.
70+
71+
### Review Focus
72+
73+
Prioritize judgment-based issues:
74+
75+
- Whether the flow is actually an RHP/modal-to-different-fullscreen reveal
76+
- Whether the destination is stable and known when the hook mounts
77+
- Whether the caller keeps flow-specific checks and synchronous writes outside `reveal()`
78+
- Whether the caller handles no-op cases where the destination is already active
79+
- Whether a back-out path calls `cleanupPreMount()`
80+
- Whether an unmount-before-submit flow needs `shouldPreservePreInsertedRouteOnUnmount`
81+
82+
### When to use
83+
84+
Use `usePreMountDestination` when **all** of these are true:
85+
86+
- The flow dismisses an RHP/modal to reveal a **different** fullscreen destination
87+
- The destination route is **known at mount time**
88+
- The user spends enough time on the confirmation screen for pre-insert to complete before dismiss (narrow layout)
89+
90+
### When NOT to use
91+
92+
- Destination is not known in advance
93+
- There is no modal/RHP to dismiss
94+
- The destination is already the screen behind the modal
95+
- The transition is already fast enough. Profile first, do not add complexity speculatively
96+
- Flow-specific dismiss strategies that do not use pre-insert/reveal. Keep those helpers
97+
98+
### Review Metadata
99+
100+
Flag when:
101+
102+
- `usePreMountDestination` is used in a flow that does not dismiss an RHP/modal to reveal a different fullscreen destination
103+
- `usePreMountDestination` is used with a destination that is not stable/known at mount time
104+
- A caller relies on `reveal(afterTransition)` for work that must happen before navigation, such as validation, target-route selection, or a synchronous write needed before the destination is revealed
105+
- A back-out path closes the RHP without calling `cleanupPreMount()` when the component owns a pre-inserted route
106+
- A submit path unmounts the component before `reveal()` runs but does not preserve the pre-inserted route with `shouldPreservePreInsertedRouteOnUnmount`
107+
- New code reimplements pre-insert timing, back-out cleanup, or reveal-before-dismiss orchestration inline instead of using the hook, even if it avoids a direct `preInsertFullscreenUnderRHP` call
108+
109+
**DO NOT flag if:**
110+
111+
- The code uses `usePreMountDestination` correctly with the matching reveal method for the flow
112+
- The call site is an approved exception (`IOURequestStepConfirmation`, `useSkipConfirmationPreInsert` until migrated)
113+
- The flow uses specialized dismiss helpers that intentionally bypass pre-insert/reveal
114+
115+
**Search patterns:**
116+
117+
- `preInsertFullscreenUnderRHP`
118+
- `usePreMountDestination`
119+
- `revealRouteBeforeDismissingModal`
120+
- `PRE_INSERT_FULLSCREEN_DELAY`

.github/scripts/createDocsRoutes.ts

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ type Article = {
1212
type Section = {
1313
href: string;
1414
title: string;
15+
order?: number;
1516
articles?: Article[];
1617
sections?: Section[];
1718
};
@@ -23,6 +24,7 @@ type Hub = {
2324
icon: string;
2425
articles?: Article[];
2526
sections?: Section[];
27+
flatSections?: Section[];
2628
};
2729

2830
type Platform = {
@@ -36,6 +38,18 @@ type DocsRoutes = {
3638

3739
type HubEntriesKey = 'sections' | 'articles';
3840

41+
function isRecord(value: unknown): value is Record<string, unknown> {
42+
return typeof value === 'object' && value !== null && !Array.isArray(value);
43+
}
44+
45+
function getOptionalString(value: unknown): string | undefined {
46+
return typeof value === 'string' ? value : undefined;
47+
}
48+
49+
function getOptionalNumber(value: unknown): number | undefined {
50+
return typeof value === 'number' ? value : undefined;
51+
}
52+
3953
const warnMessage = (platform: string): string => `Number of hubs in _routes.yml does not match number of hubs in docs/${platform}/articles. Please update _routes.yml with hub info.`;
4054
const disclaimer = '# This file is auto-generated. Do not edit it directly. Use npm run createDocsRoutes instead.\n';
4155
const docsDir = `${process.cwd()}/docs`;
@@ -109,24 +123,55 @@ function getOrderFromArticleFrontMatter(path: string): number | undefined {
109123
if (!frontmatter) {
110124
return undefined;
111125
}
112-
const frontmatterObject = yaml.load(frontmatter) as Record<string, unknown>;
113-
return frontmatterObject.order as number | undefined;
126+
const frontmatterObject = yaml.load(frontmatter);
127+
if (!isRecord(frontmatterObject)) {
128+
return undefined;
129+
}
130+
return getOptionalNumber(frontmatterObject.order);
114131
} catch {
115132
return undefined;
116133
}
117134
}
118135

136+
function getSectionMeta(sectionPath: string): {title?: string; order?: number} {
137+
try {
138+
const metaPath = `${sectionPath}/_meta.yml`;
139+
if (!fs.existsSync(metaPath)) {
140+
return {};
141+
}
142+
const meta = yaml.load(fs.readFileSync(metaPath, 'utf8'));
143+
if (!isRecord(meta)) {
144+
return {};
145+
}
146+
return {
147+
title: getOptionalString(meta.title),
148+
order: getOptionalNumber(meta.order),
149+
};
150+
} catch {
151+
return {};
152+
}
153+
}
154+
155+
function sortSectionsByOrder(sections: Section[]) {
156+
sections.sort((a, b) => (a.order ?? Number.POSITIVE_INFINITY) - (b.order ?? Number.POSITIVE_INFINITY));
157+
}
158+
119159
/**
120160
* Build a section from a directory path, with optional parent path for nested href
121161
*/
122162
function buildSection(platformName: string, hub: string, sectionPath: string, parentHref: string): Section {
123163
const sectionName = sectionPath.split('/').pop() ?? sectionPath;
124164
const fullPath = `${docsDir}/articles/${platformName}/${hub}/${sectionPath}`;
165+
const meta = getSectionMeta(fullPath);
125166
const articles: Article[] = [];
126167
const childSections: Section[] = [];
127168
const href = parentHref ? `${parentHref}/${sectionName}` : sectionName;
128169

129170
for (const entry of fs.readdirSync(fullPath)) {
171+
if (entry === '_meta.yml') {
172+
continue;
173+
}
174+
130175
const entryPath = `${fullPath}/${entry}`;
131176
if (entry.endsWith('.md')) {
132177
const order = getOrderFromArticleFrontMatter(entryPath);
@@ -140,9 +185,12 @@ function buildSection(platformName: string, hub: string, sectionPath: string, pa
140185
// The sort is stable, so articles without an `order` keep their relative position and fall after ordered ones.
141186
articles.sort((a, b) => (a.order ?? Number.POSITIVE_INFINITY) - (b.order ?? Number.POSITIVE_INFINITY));
142187

188+
sortSectionsByOrder(childSections);
189+
143190
const section: Section = {
144191
href,
145-
title: toTitleCase(sectionName.replaceAll('-', ' ')),
192+
title: meta.title ?? toTitleCase(sectionName.replaceAll('-', ' ')),
193+
...(meta.order !== undefined && {order: meta.order}),
146194
...(articles.length > 0 && {articles}),
147195
...(childSections.length > 0 && {sections: childSections}),
148196
};
@@ -175,7 +223,9 @@ function createHubsWithArticles(hubs: string[], platformName: ValueOf<typeof pla
175223

176224
for (const fileOrFolder of fs.readdirSync(basePath)) {
177225
if (fileOrFolder.endsWith('.md')) {
178-
const articleObj = getArticleObj(fileOrFolder);
226+
const entryPath = `${basePath}/${fileOrFolder}`;
227+
const order = getOrderFromArticleFrontMatter(entryPath);
228+
const articleObj = getArticleObj(fileOrFolder, order);
179229
pushOrCreateEntry(routeHubs, hub, 'articles', articleObj);
180230
continue;
181231
}
@@ -188,7 +238,8 @@ function createHubsWithArticles(hubs: string[], platformName: ValueOf<typeof pla
188238
// Add flat section list for nested section page lookup
189239
const hubObj = routeHubs.find((obj) => obj.href === hub);
190240
if (hubObj?.sections?.length) {
191-
(hubObj as Hub & {flatSections?: Section[]}).flatSections = flattenSections(hubObj.sections);
241+
sortSectionsByOrder(hubObj.sections);
242+
hubObj.flatSections = flattenSections(hubObj.sections);
192243
}
193244
}
194245
}

.github/scripts/generateAllowedUrls.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import path from 'path';
55
* Script to extract all URLs from help articles and generate a whitelist.
66
* Run this at build time to update the allowed URLs list.
77
*
8-
* Usage: npx ts-node .github/scripts/generateAllowedUrls.ts
8+
* Usage: bun .github/scripts/generateAllowedUrls.ts
99
*/
1010

1111
const DOCS_DIR = path.join(__dirname, '..', '..', 'docs');

.github/workflows/androidBump.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ jobs:
1010
android_bump:
1111
runs-on: blacksmith-2vcpu-ubuntu-2404
1212
steps:
13-
- uses: useblacksmith/checkout@c9796daa2a4bdebdab5bd16be2c09a70cd4e1121 # v1
13+
- uses: useblacksmith/checkout@1c9394c220d293645707b625ba9d79685f093a8f # v1
1414

1515
- name: Setup Node
1616
uses: ./.github/actions/composite/setupNode

.github/workflows/authorChecklist.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ jobs:
1818
&& github.actor != 'imgbot[bot]'
1919
steps:
2020
- name: Checkout
21-
uses: useblacksmith/checkout@c9796daa2a4bdebdab5bd16be2c09a70cd4e1121 # v1
21+
uses: useblacksmith/checkout@1c9394c220d293645707b625ba9d79685f093a8f # v1
2222

2323
- name: Check contributor authorization
2424
id: gate

.github/workflows/buildAdHoc.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ jobs:
140140
needs: [buildWeb, deployWebAdHoc, buildAndroid, buildIOS]
141141
steps:
142142
- name: Checkout
143-
uses: useblacksmith/checkout@c9796daa2a4bdebdab5bd16be2c09a70cd4e1121 # v1
143+
uses: useblacksmith/checkout@1c9394c220d293645707b625ba9d79685f093a8f # v1
144144
with:
145145
ref: ${{ inputs.APP_REF }}
146146

@@ -180,7 +180,7 @@ jobs:
180180
needs: [buildWeb, deployWebAdHoc, buildAndroid, buildIOS]
181181
steps:
182182
- name: Checkout
183-
uses: useblacksmith/checkout@c9796daa2a4bdebdab5bd16be2c09a70cd4e1121 # v1
183+
uses: useblacksmith/checkout@1c9394c220d293645707b625ba9d79685f093a8f # v1
184184
with:
185185
ref: ${{ inputs.APP_REF }}
186186

.github/workflows/buildAndroid.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ jobs:
6363
PROGUARD_MAPPING_FILENAME: mapping.txt
6464
steps:
6565
- name: Checkout
66-
uses: useblacksmith/checkout@c9796daa2a4bdebdab5bd16be2c09a70cd4e1121 # v1
66+
uses: useblacksmith/checkout@1c9394c220d293645707b625ba9d79685f093a8f # v1
6767
with:
6868
submodules: true
6969
ref: ${{ inputs.ref }}

.github/workflows/buildIOS.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ jobs:
6060
SOURCEMAP_FILENAME: main.jsbundle.map
6161
steps:
6262
- name: Checkout
63-
uses: useblacksmith/checkout@c9796daa2a4bdebdab5bd16be2c09a70cd4e1121 # v1
63+
uses: useblacksmith/checkout@1c9394c220d293645707b625ba9d79685f093a8f # v1
6464
with:
6565
submodules: true
6666
ref: ${{ inputs.ref }}

0 commit comments

Comments
 (0)